= ({ emails }) => {
+const StatValue = styled(Typography, {
+ shouldForwardProp: (prop) => prop !== "$error",
+})<{ $error?: boolean }>(({ theme, $error }) => ({
+ ...theme.typography.h3,
+ color: $error ? theme.custom.colors.red : theme.custom.colors.darkGray2,
+ lineHeight: "36px",
+ textAlign: "center",
+ width: "100%",
+}))
+
+const StatLabel = styled(Typography)(({ theme }) => ({
+ ...theme.typography.subtitle3,
+ color: theme.custom.colors.silverGrayDark,
+ textAlign: "center",
+ width: "100%",
+})) as typeof Typography
+
+// ─── Small helpers ────────────────────────────────────────────────────────────
+
+const SHOW_MORE_THRESHOLD = 3
+
+const EmailListExpand: React.FC<{ emails: string[] }> = ({ emails }) => {
const [expanded, setExpanded] = useState(false)
const visible = expanded ? emails : emails.slice(0, SHOW_MORE_THRESHOLD)
const hidden = emails.length - SHOW_MORE_THRESHOLD
return (
-
- {visible.map((email) => (
- {email}
+
+ {visible.map((email, idx) => (
+ {email}
))}
-
+
{!expanded && hidden > 0 && (
- setExpanded(true)}>
- +{hidden} more
+ setExpanded(true)}
+ aria-label={`Show ${hidden} more email addresses`}
+ >
+ + {hidden} more
)}
)
}
+const copyToClipboard = async (text: string): Promise => {
+ if (navigator.clipboard) {
+ try {
+ await navigator.clipboard.writeText(text)
+ return true
+ } catch {
+ // fall through to execCommand fallback
+ }
+ }
+ // execCommand fallback for non-secure contexts (HTTP dev environments).
+ // Sets clipboard data via the copy event directly, bypassing focus trap
+ // issues caused by MUI Dialog.
+ return new Promise((resolve) => {
+ const handler = (e: ClipboardEvent) => {
+ e.clipboardData?.setData("text/plain", text)
+ e.preventDefault()
+ resolve(true)
+ }
+ document.addEventListener("copy", handler, { once: true })
+ const success = document.execCommand("copy")
+ if (!success) {
+ document.removeEventListener("copy", handler)
+ resolve(false)
+ }
+ })
+}
+
+// ─── Main component ───────────────────────────────────────────────────────────
+
type AssignSeatsConfirmModalProps = {
open: boolean
onClose: () => void
- onConfirm: () => void
+ onConfirm: () => void | Promise
validCount: number
availableSeats: number
invalidEmails: string[]
- duplicateCount: number
+ duplicateEmails: string[]
skippedCount: number
}
+type Step = "review" | "confirm"
+
const AssignSeatsConfirmModal: React.FC = ({
open,
onClose,
@@ -78,70 +234,413 @@ const AssignSeatsConfirmModal: React.FC = ({
validCount,
availableSeats,
invalidEmails,
- duplicateCount,
+ duplicateEmails,
skippedCount,
}) => {
const descriptionId = useId()
- const hasIssues =
- invalidEmails.length > 0 || duplicateCount > 0 || skippedCount > 0
+ const overCapacitySummaryId = `${descriptionId}-summary`
+
+ const hasInvalid = invalidEmails.length > 0
+ const hasDuplicates = duplicateEmails.length > 0
+ const hasIssues = hasInvalid || hasDuplicates || skippedCount > 0
const overCapacity = validCount > availableSeats
- const confirmText = overCapacity
- ? "Ok"
- : `Send ${validCount} ${pluralize("email", validCount)}`
- const overCapacityBody = overCapacity
- ? `${validCount} ${pluralize("email", validCount)} entered, only ${availableSeats} ${pluralize("seat", availableSeats)} remaining. Please enter fewer emails.`
- : ""
- const descriptionText = overCapacity
- ? overCapacityBody
- : hasIssues
- ? `${validCount} ${pluralize("email", validCount)} imported and ready to assign.${duplicateCount > 0 ? ` ${duplicateCount} ${pluralize("duplicate", duplicateCount)} removed — only 1 instance kept per address.` : ""}${skippedCount > 0 ? ` ${skippedCount} ${pluralize("row", skippedCount)} skipped — no email address found.` : ""}`
- : `Are you sure you want to send invitations to ${validCount} ${pluralize("recipient", validCount)}?`
+
+ const [step, setStep] = useState(() =>
+ hasIssues && !overCapacity ? "review" : "confirm",
+ )
+ const [copiedInvalid, setCopiedInvalid] = useState(false)
+ const [copiedDuplicate, setCopiedDuplicate] = useState(false)
+ const [isSubmitting, setIsSubmitting] = useState(false)
+ const [sendError, setSendError] = useState(null)
+ // Assertive live region text for step transitions. Using reset-then-set (100ms
+ // delay) so NVDA picks up the content change cleanly after focus settles on the
+ // new step's first focusable element inside the same persistent dialog.
+ const [stepAnnouncement, setStepAnnouncement] = useState("")
+ const [sendErrorAnnouncement, setSendErrorAnnouncement] = useState("")
+ const prevOpenRef = useRef(open)
+ const copyInvalidTimerRef = useRef | null>(null)
+ const copyDuplicateTimerRef = useRef | null>(
+ null,
+ )
+ const stepAnnouncementTimerRef = useRef | null>(
+ null,
+ )
+ const sendErrorAnnouncementTimerRef = useRef | null>(null)
+
+ // Only reset step and copy state when the dialog transitions from closed to
+ // open. Without prevOpenRef the effect would also fire when hasIssues or
+ // overCapacity change while the dialog is already open (e.g. a parent
+ // re-render), resetting the user's progress mid-flow.
+ useEffect(() => {
+ const wasOpen = prevOpenRef.current
+ prevOpenRef.current = open
+ if (open && !wasOpen) {
+ setStep(hasIssues && !overCapacity ? "review" : "confirm")
+ setCopiedInvalid(false)
+ setCopiedDuplicate(false)
+ setSendError(null)
+ setSendErrorAnnouncement("")
+ setStepAnnouncement("")
+ }
+ }, [open, hasIssues, overCapacity])
+
+ useEffect(() => {
+ return () => {
+ if (copyInvalidTimerRef.current) clearTimeout(copyInvalidTimerRef.current)
+ if (copyDuplicateTimerRef.current)
+ clearTimeout(copyDuplicateTimerRef.current)
+ if (stepAnnouncementTimerRef.current)
+ clearTimeout(stepAnnouncementTimerRef.current)
+ if (sendErrorAnnouncementTimerRef.current)
+ clearTimeout(sendErrorAnnouncementTimerRef.current)
+ }
+ }, [])
+
+ const handleCopyInvalid = async () => {
+ const ok = await copyToClipboard(invalidEmails.join("\n"))
+ if (!ok) return
+ setCopiedInvalid(true)
+ if (copyInvalidTimerRef.current) clearTimeout(copyInvalidTimerRef.current)
+ copyInvalidTimerRef.current = setTimeout(
+ () => setCopiedInvalid(false),
+ 2000,
+ )
+ }
+
+ const handleCopyDuplicate = async () => {
+ const ok = await copyToClipboard(duplicateEmails.join("\n"))
+ if (!ok) return
+ setCopiedDuplicate(true)
+ if (copyDuplicateTimerRef.current)
+ clearTimeout(copyDuplicateTimerRef.current)
+ copyDuplicateTimerRef.current = setTimeout(
+ () => setCopiedDuplicate(false),
+ 2000,
+ )
+ }
+
+ const handleSend = async () => {
+ setIsSubmitting(true)
+ setSendError(null)
+ try {
+ await onConfirm()
+ onClose()
+ } catch {
+ const msg = "Something went wrong. Please try again."
+ setSendError(msg)
+ // Announce via assertive live region — same reset-then-set pattern used
+ // elsewhere in this file, because dynamically-mounted role="alert" elements
+ // are not consistently picked up by NVDA when other live regions are active.
+ setSendErrorAnnouncement("")
+ if (sendErrorAnnouncementTimerRef.current)
+ clearTimeout(sendErrorAnnouncementTimerRef.current)
+ sendErrorAnnouncementTimerRef.current = setTimeout(
+ () => setSendErrorAnnouncement(msg),
+ 100,
+ )
+ } finally {
+ setIsSubmitting(false)
+ }
+ }
+
+ const seatsAfterSending = availableSeats - validCount
+ const overLimit = validCount - availableSeats
+
+ const reviewTitle = hasInvalid
+ ? "Some learners could not be added"
+ : hasDuplicates
+ ? "Duplicate emails removed"
+ : "Some rows were skipped"
+
+ // Advance from the review step to the confirm step within the same dialog
+ // instance. Using an assertive live region (reset-then-set) rather than
+ // mounting a new Dialog so that focus stays trapped and NVDA doesn't have
+ // to re-discover a new role="dialog" element.
+ const handleReviewAndConfirm = () => {
+ setStep("confirm")
+ const confirmText = `Ready to send invitations. You are about to send ${validCount} invitation ${pluralize("email", validCount)} from MIT Learn. Learners will receive an email with a secure link to claim their seat and access the materials. ${seatsAfterSending} seats remaining after sending. Emails will be sent immediately and cannot be recalled.`
+ setStepAnnouncement("")
+ if (stepAnnouncementTimerRef.current)
+ clearTimeout(stepAnnouncementTimerRef.current)
+ stepAnnouncementTimerRef.current = setTimeout(
+ () => setStepAnnouncement(confirmText),
+ 100,
+ )
+ }
+
+ // ── Derived title / actions / description (single Dialog, step-based) ──────
+
+ const dialogTitle = overCapacity ? (
+ <>
+
+
+
+ Not enough seats available
+
+ ) : step === "review" ? (
+ <>
+
+
+
+ {reviewTitle}
+
+ ) : (
+ "Ready to send invitations"
+ )
+
+ const dialogActions = overCapacity ? (
+
+
+
+ ) : step === "review" ? (
+
+
+
+
+ ) : (
+
+
+
+
+ )
+
+ const dialogDescription = overCapacity
+ ? `${validCount} learners were imported, but only ${availableSeats} seats remain. ${overLimit} learners exceed the remaining contract capacity and cannot be assigned. Update your CSV to reduce the number of learners before continuing.`
+ : step === "review"
+ ? [
+ `${validCount} learners are ready to invite.`,
+ hasInvalid
+ ? ` ${invalidEmails.length} ${pluralize("email address", invalidEmails.length, "email addresses")} were invalid and will be excluded.`
+ : "",
+ hasDuplicates
+ ? ` ${duplicateEmails.length} duplicate ${pluralize("email address", duplicateEmails.length, "email addresses")} ${duplicateEmails.length === 1 ? "was" : "were"} removed.`
+ : "",
+ skippedCount > 0
+ ? ` ${skippedCount} ${pluralize("row", skippedCount)} skipped — no email address found.`
+ : "",
+ " Only valid, unique emails will be assigned.",
+ ].join("")
+ : `You are about to send ${validCount} invitation ${pluralize("email", validCount)} from MIT Learn. Learners will receive an email with secure link to claim their seat and access the materials. ${seatsAfterSending} seats remaining after sending. Emails will be sent immediately and cannot be recalled.`
+
+ // For overCapacity we point aria-describedby at the visible paragraph so
+ // role="alertdialog" reads it exactly once on open. We do NOT programmatically
+ // focus that paragraph — focus goes to the first interactive element (close
+ // button) so NVDA enters forms mode, preventing browse-mode mouse-hover reads.
+ const dialogDescribedBy = overCapacity ? overCapacitySummaryId : descriptionId
return (
= ({
disabled={!emailValid}
maxWidth="sm"
noValidate
+ aria-describedby={reassignDescId}
>
-
+
The invitation for {assignedTo} will be reassigned to the new address
and a claim email sent there.
diff --git a/frontends/main/src/app-pages/DashboardPage/ContractContent.test.tsx b/frontends/main/src/app-pages/DashboardPage/ContractContent.test.tsx
index 17010a97f9..c28d1fa677 100644
--- a/frontends/main/src/app-pages/DashboardPage/ContractContent.test.tsx
+++ b/frontends/main/src/app-pages/DashboardPage/ContractContent.test.tsx
@@ -993,9 +993,9 @@ describe("ContractContent", () => {
const { orgX, user, mitxOnlineUser } = setupOrgAndUser()
// Create courses with specific, predictable dates for the contract runs
- // Use a date that's guaranteed to be in the future relative to mocked time
- const specificStartDate = "2024-12-01T00:00:00Z"
- const specificEndDate = "2025-01-15T00:00:00Z"
+ // Use a date within 90 days of frozen time so formatCalendarDays returns "in X days"
+ const specificStartDate = "2024-02-15T00:00:00Z"
+ const specificEndDate = "2024-03-31T00:00:00Z"
const baseCourses = factories.courses.courses({ count: 3 }).results
const program = factories.programs.program({
@@ -1084,7 +1084,7 @@ describe("ContractContent", () => {
is_enrollable: true,
title: `CORRECT RUN - ${run.title}`,
courseware_url: "https://correct-run.example.com",
- start_date: "2024-12-01T00:00:00Z", // Future date relative to mocked time
+ start_date: "2024-02-15T00:00:00Z", // Future date relative to mocked time, within 90-day threshold
}
}),
}))
diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/CardShared.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/CardShared.tsx
index b516a207af..650cf671c1 100644
--- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/CardShared.tsx
+++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/CardShared.tsx
@@ -1,14 +1,10 @@
import React from "react"
-import { Link, Stack, styled } from "ol-components"
+import { Link, Popover, Stack, styled, Typography } from "ol-components"
import NextLink from "next/link"
import { EnrollmentStatus } from "./helpers"
import { ActionButton, Button, ButtonLink } from "@mitodl/smoot-design"
-import {
- DashboardResource,
- DashboardType,
- getEnrollmentStatus,
-} from "./model/dashboardViewModel"
-import { calendarDaysUntil } from "ol-utilities"
+import { formatDate, getTimezone } from "ol-utilities"
+import { getCourseDateText, getRelativeDateContent } from "./courseDateUtils"
const CardRoot = styled.div<{
screenSize: "desktop" | "mobile"
@@ -66,6 +62,11 @@ const CardRoot = styled.div<{
},
])
+const CardTypeText = styled(Typography)(({ theme }) => ({
+ ...theme.typography.subtitle4,
+ color: theme.custom.colors.silverGrayDark,
+}))
+
const TitleHeading = styled.h3(({ theme }) => ({
margin: 0,
[theme.breakpoints.down("md")]: {
@@ -134,21 +135,6 @@ const MenuButton = styled(ActionButton)<{
},
])
-const CountdownRoot = styled.div<{ layout?: "default" | "compact" }>(
- ({ theme, layout = "default" }) => ({
- width: layout === "compact" ? "88px" : "100%",
- paddingRight: layout === "compact" ? "0px" : "32px",
- display: "flex",
- justifyContent: "center",
- alignSelf: "end",
- whiteSpace: layout === "compact" ? "nowrap" : "normal",
- [theme.breakpoints.down("md")]: {
- marginRight: "0px",
- justifyContent: "flex-start",
- },
- }),
-)
-
const COURSEWARE_BUTTON_WIDTH = "88px"
// Thin vertical divider shown between the certificate/upgrade links and the
@@ -183,79 +169,157 @@ const CoursewareButtonLink = styled(ButtonLink)(({ theme, variant }) => ({
}),
}))
-/**
- * Rewrites a raw mitxonline certificate link (`/certificate/{uuid}/`) to MIT
- * Learn's own certificate route (`/certificate/{certificateType}/{uuid}/`).
- */
-const getCertificateLink = (
- link: string | null | undefined,
- certificateType: "course" | "program",
-): string | null => {
- if (!link) return null
- const pattern = /\/certificate\/([^/]+)\/?$/
- return link.replace(pattern, `/certificate/${certificateType}/$1/`)
+const Separator = styled.span(({ theme }) => ({
+ display: "inline-block",
+ width: "1px",
+ height: "12px",
+ margin: "0 8px",
+ backgroundColor: theme.custom.colors.silverGrayLight,
+}))
+
+const DateText = styled(Typography)(({ theme }) => ({
+ ...theme.typography.subtitle3,
+ color: theme.custom.colors.silverGrayDark,
+}))
+
+const CourseDateText: React.FC<{
+ startDate?: string | null | undefined
+ endDate?: string | null | undefined
+ className?: string
+}> = ({ startDate, endDate, className }) => {
+ const text = getCourseDateText(startDate, endDate)
+ if (!text) return null
+ return {text}
}
-const getDashboardEnrollmentStatus = (
- resource: DashboardResource,
-): EnrollmentStatus => {
- const hasValidCertificate =
- resource.type !== DashboardType.Course && !!resource.data.certificate?.uuid
+const DatePopoverContent = styled.div({
+ maxWidth: "240px",
+ display: "flex",
+ padding: "8px",
+ flexDirection: "column",
+ alignItems: "flex-start",
+ gap: "28px",
+ alignSelf: "stretch",
+})
- if (resource.type === DashboardType.Course) {
- return EnrollmentStatus.NotEnrolled
- }
+const DatePopoverTrigger = styled("button")(({ theme }) => ({
+ ...theme.typography.body2,
+ color: theme.custom.colors.silverGrayDark,
+ background: "none",
+ border: "none",
+ padding: 0,
+ cursor: "pointer",
+ "&:hover": {
+ color: theme.custom.colors.silverGrayDark,
+ textDecoration: "underline",
+ },
+}))
- if (resource.type === DashboardType.CourseRunEnrollment) {
- return hasValidCertificate
- ? EnrollmentStatus.Completed
- : getEnrollmentStatus(resource.data)
- }
+const DatePopoverHeading = styled(Typography)(({ theme }) => ({
+ color: theme.custom.colors.black,
+}))
- return hasValidCertificate
- ? EnrollmentStatus.Completed
- : EnrollmentStatus.Enrolled
-}
+const DatePopoverBody = styled(Typography)(({ theme }) => ({
+ color: theme.custom.colors.black,
+}))
-const CourseStartCountdown: React.FC<{
- startDate: string
- className?: string
- layout?: "default" | "compact"
-}> = ({ startDate, className, layout = "default" }) => {
- const calendarDays = calendarDaysUntil(startDate)
+const DatePopoverDismissButton = styled(Button)({
+ alignSelf: "flex-end",
+})
+
+const CourseDateSummary: React.FC<{
+ startDate?: string | null | undefined
+ endDate?: string | null | undefined
+}> = ({ startDate, endDate }) => {
+ const popoverId = React.useId()
+ const [popoverAnchorEl, setPopoverAnchorEl] =
+ React.useState(null)
+
+ const triggerText = getCourseDateText(startDate, endDate)
+ const startDateFormatted = startDate
+ ? `${formatDate(startDate, "MMMM D, YYYY h:mm A")} ${getTimezone(startDate)}`
+ : null
+ const endDateFormatted = endDate
+ ? `${formatDate(endDate, "MMMM D, YYYY h:mm A")} ${getTimezone(endDate)}`
+ : null
+ const datePopoverContent = getRelativeDateContent(
+ startDate,
+ endDate,
+ startDateFormatted,
+ endDateFormatted,
+ )
+
+ if (!triggerText || !datePopoverContent) return null
- let value
- if (calendarDays === null || calendarDays < 0) return null
- if (calendarDays === 0) {
- value = "Starts Today"
- } else if (calendarDays === 1) {
- value = "Starts Tomorrow"
- } else {
- value = `Starts in ${calendarDays} days`
- }
return (
-
-
- {value}
-
-
+ <>
+ setPopoverAnchorEl(null)}
+ >
+
+
+
+ Important Dates:
+
+
+ This course{" "}
+
+ {datePopoverContent.startVerb}
+ {" "}
+ {datePopoverContent.startSuffix}
+
+
+ {datePopoverContent.endVerb && datePopoverContent.endSuffix && (
+
+ This course{" "}
+
+ {datePopoverContent.endVerb}
+ {" "}
+ {datePopoverContent.endSuffix}
+
+ )}
+ setPopoverAnchorEl(null)}
+ >
+ Got it!
+
+
+
+ setPopoverAnchorEl(event.currentTarget)}
+ >
+ {triggerText}
+
+
)
}
export {
CardRoot,
+ CardTypeText,
TitleHeading,
TitleLink,
TitleText,
SubtitleLinkRoot,
SubtitleLink,
MenuButton,
- CountdownRoot,
- CourseStartCountdown,
HorizontalSeparator,
CoursewareActionColumn,
CoursewareButton,
CoursewareButtonLink,
- getCertificateLink,
- getDashboardEnrollmentStatus,
+ Separator,
+ DateText,
+ CourseDateText,
+ CourseDateSummary,
}
diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx
index d3c6ff87ce..366a123291 100644
--- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx
+++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx
@@ -100,7 +100,9 @@ describe.each([
run: { ...futureRunDates, courseware_url: coursewareUrl },
})
renderWithProviders()
- const btn = await within(getCard()).findByRole("link", { name: "Continue" })
+ const btn = await within(getCard()).findByRole("link", {
+ name: /^Continue course:/,
+ })
expect(btn).toHaveAttribute("href", coursewareUrl)
})
@@ -194,6 +196,54 @@ describe.each([
expect(getCard()).toHaveTextContent(/starts in 5 days/i)
})
+ // ---------------------------------------------------------------------------
+ // End date display
+ // ---------------------------------------------------------------------------
+
+ test.each([
+ {
+ endDate: moment().add(2, "days").toISOString(),
+ expectedText: /ends in 2 days/i,
+ case: "future (plural)",
+ },
+ {
+ endDate: moment().add(1, "day").toISOString(),
+ expectedText: /ends tomorrow/i,
+ case: "future (singular)",
+ },
+ {
+ endDate: moment().subtract(2, "days").toISOString(),
+ expectedText: /ended 2 days ago/i,
+ case: "past (plural)",
+ },
+ {
+ endDate: moment().subtract(1, "day").toISOString(),
+ expectedText: /ended yesterday/i,
+ case: "past (singular)",
+ },
+ ])("Shows end date text ($case)", ({ endDate, expectedText }) => {
+ setupUserApis()
+ const enrollment = mitxonline.factories.enrollment.courseEnrollment({
+ run: { ...currentRunDates, end_date: endDate },
+ })
+ renderWithProviders()
+ expect(within(getCard()).getByText(expectedText)).toBeInTheDocument()
+ })
+
+ test("Does not show end date text when run has no end date", () => {
+ setupUserApis()
+ const enrollment = mitxonline.factories.enrollment.courseEnrollment({
+ run: { ...currentRunDates, end_date: null },
+ })
+ renderWithProviders()
+ expect(
+ within(getCard()).queryByText(/ends in \d+ day/i),
+ ).not.toBeInTheDocument()
+ expect(
+ within(getCard()).queryByText(/ended \d+ day/i),
+ ).not.toBeInTheDocument()
+ })
+
// ---------------------------------------------------------------------------
// Enrollment status indicator
// ---------------------------------------------------------------------------
@@ -338,6 +388,26 @@ describe.each([
},
)
+ test("Does not show upgrade banner when upgrade deadline has passed", () => {
+ setupUserApis()
+ const enrollment = mitxonline.factories.enrollment.courseEnrollment({
+ enrollment_mode: EnrollmentMode.Audit,
+ b2b_contract_id: null,
+ run: {
+ ...currentRunDates,
+ is_upgradable: true,
+ upgrade_deadline: moment().subtract(1, "day").toISOString(),
+ upgrade_product_id: faker.number.int(),
+ upgrade_product_price: faker.commerce.price(),
+ upgrade_product_is_active: true,
+ },
+ })
+ renderWithProviders()
+ expect(
+ within(getCard()).queryByTestId("upgrade-root"),
+ ).not.toBeInTheDocument()
+ })
+
test("Does not show upgrade banner when upgrade_product_is_active is missing", () => {
setupUserApis()
const enrollment = mitxonline.factories.enrollment.courseEnrollment({
diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx
index 52c02b51aa..9e4b0ad997 100644
--- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx
+++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx
@@ -2,23 +2,29 @@ import React from "react"
import { SimpleMenu, Stack } from "ol-components"
import {
CardRoot,
- CourseStartCountdown,
+ CardTypeText,
CoursewareActionColumn,
CoursewareButton,
CoursewareButtonLink,
- getCertificateLink,
- getDashboardEnrollmentStatus,
HorizontalSeparator,
MenuButton,
+ Separator,
SubtitleLink,
SubtitleLinkRoot,
TitleHeading,
TitleLink,
TitleText,
+ CourseDateSummary,
} from "./CardShared"
-import { EnrollmentStatus, DashboardType } from "./model/dashboardViewModel"
+import {
+ EnrollmentStatus,
+ DashboardType,
+ getCertificateLink,
+ getDashboardEnrollmentStatus,
+} from "./model/dashboardViewModel"
+import { getCourseDateText } from "./courseDateUtils"
import { isVerifiedEnrollmentMode } from "@/common/mitxonline"
-import { RiAddLine, RiAwardLine, RiMore2Line } from "@remixicon/react"
+import { RiAwardLine, RiMore2Line } from "@remixicon/react"
import { useReplaceBasketItem } from "@/common/mitxonline/useReplaceBasketItem"
import { isInPast, calendarDaysUntil, NoSSR } from "ol-utilities"
import { EnrollmentStatusIndicator } from "./EnrollmentStatusIndicator"
@@ -90,7 +96,7 @@ const UpgradeBanner: React.FC<
return (
-
+
{`Add a certificate for ${formattedPrice}`}
{calendarDays !== null && (
@@ -125,40 +131,49 @@ export const EnrolledCourseCard = ({
const isContractPageResource = Boolean(enrollment.b2b_contract_id)
const mitxOnlineUser = useQuery(mitxUserQueries.me())
const isStaff = mitxOnlineUser.data?.is_staff
- const title = layout === "compact" ? course.title : run?.title || course.title
+ const isCompact = layout === "compact"
+ const title = isCompact ? course.title : run?.title || course.title
const coursewareUrl = run?.courseware_url
const certificateLink = getCertificateLink(
enrollment?.certificate?.link,
"course",
)
const enrollmentMode = enrollment?.enrollment_mode
- const canUpgrade =
- !isVerifiedEnrollmentMode(enrollmentMode) &&
- (run?.is_upgradable ?? false) &&
- (enrollment?.run?.upgrade_product_is_active ?? false)
const offerUpgrade = !enrollment?.b2b_contract_id
const startDate = run?.start_date
const hasStarted = startDate ? isInPast(startDate) : true
+ const endDate = run?.end_date
+ const daysUntilEnd = endDate ? calendarDaysUntil(endDate) : null
+ const hasEnded = endDate ? isInPast(endDate) : false
+ const hasCourseDateText = getCourseDateText(startDate, endDate) !== null
+ const courseDateText = (
+
+ )
+ const canUpgrade =
+ offerUpgrade &&
+ !isVerifiedEnrollmentMode(enrollmentMode) &&
+ (run?.is_upgradable ?? false) &&
+ (enrollment?.run?.upgrade_product_is_active ?? false) &&
+ !hasEnded
+ const showUpgradeBanner =
+ canUpgrade &&
+ !!run?.upgrade_product_price &&
+ !!run?.upgrade_product_id &&
+ !(run?.upgrade_deadline && isInPast(run.upgrade_deadline))
const enrollmentStatus = getDashboardEnrollmentStatus({
type: DashboardType.CourseRunEnrollment,
data: enrollment,
})
- const isCompact = layout === "compact"
- const showUpgradeLink =
- !isVerifiedEnrollmentMode(enrollmentMode) && offerUpgrade && canUpgrade
- const showCertificateSection = Boolean(certificateLink) || showUpgradeLink
- const certificateAndUpgrade = (
- <>
- {certificateLink ? (
-
-
- {isCompact ? "Certificate" : "View Certificate"}
-
+ const endDateAndCertSection = (
+
+ {courseDateText}
+ {hasCourseDateText && (showUpgradeBanner || !!certificateLink) ? (
+
) : null}
- {showUpgradeLink ? (
+ {showUpgradeBanner ? (
) : null}
-
+ {certificateLink ? (
+
+
+ {isCompact ? "Certificate" : "View Certificate"}
+
+ ) : null}
+
)
const titleSection = (
- <>
+
{coursewareUrl ? (
@@ -183,8 +204,8 @@ export const EnrolledCourseCard = ({
) : (
{title}
)}
- {isCompact ? null : certificateAndUpgrade}
-
+ {isCompact ? null : endDateAndCertSection}
+
)
// Determine if button should be disabled
// Staff can access courseware even before the course has started
@@ -204,6 +225,7 @@ export const EnrolledCourseCard = ({
variant={compactVariant}
disabled
data-testid="courseware-button"
+ aria-label={`${buttonText} course: ${title}`}
>
{buttonText}
@@ -213,6 +235,7 @@ export const EnrolledCourseCard = ({
variant={compactVariant}
href={coursewareUrl ?? ""}
data-testid="courseware-button"
+ aria-label={`${buttonText} course: ${title}`}
>
{buttonText}
@@ -223,6 +246,7 @@ export const EnrolledCourseCard = ({
variant="primary"
disabled
data-testid="courseware-button"
+ aria-label={`${buttonText} course: ${title}`}
>
{buttonText}
@@ -232,28 +256,20 @@ export const EnrolledCourseCard = ({
variant="primary"
href={coursewareUrl ?? ""}
data-testid="courseware-button"
+ aria-label={`${buttonText} course: ${title}`}
>
{buttonText}
)
const buttonSection = isCompact ? (
-
-
- {certificateAndUpgrade}
- {showCertificateSection ? : null}
-
- {ctaButton}
-
-
- {startDate && !hasStarted ? (
-
-
-
+
+ {endDateAndCertSection}
+ {daysUntilEnd !== null || showUpgradeBanner || !!certificateLink ? (
+
) : null}
+
+ {ctaButton}
+
) : (
<>
@@ -321,10 +337,6 @@ export const EnrolledCourseCard = ({
}
/>
)
- const startDateSection =
- !isCompact && startDate && !hasStarted ? (
-
- ) : null
return (
<>
@@ -335,15 +347,13 @@ export const EnrolledCourseCard = ({
className={className}
layout={layout}
>
-
+
+ Course
{titleSection}
-
-
- {buttonSection}
- {contextMenu}
-
- {startDateSection}
+
+ {buttonSection}
+ {contextMenu}
@@ -368,14 +378,12 @@ export const EnrolledCourseCard = ({
- {startDateSection}
-
- {buttonSection}
-
+ {buttonSection}
diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/OrganizationCards.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/OrganizationCards.test.tsx
index d1a701ce42..071ae7bac7 100644
--- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/OrganizationCards.test.tsx
+++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/OrganizationCards.test.tsx
@@ -172,7 +172,7 @@ describe("OrganizationCards", () => {
setup({ organizations: [organization] })
const continueButtons = await screen.findAllByRole("link", {
- name: "Continue",
+ name: /^Continue /,
})
expect(continueButtons).toHaveLength(4) // 2 contracts × 2 screen sizes (mobile + desktop)
@@ -237,7 +237,7 @@ describe("OrganizationCards", () => {
expect(elements.length).toBeGreaterThan(0)
expect(
- screen.queryByRole("link", { name: "Continue" }),
+ screen.queryByRole("link", { name: /^Continue / }),
).not.toBeInTheDocument()
expect(screen.queryByText("Other Org Contract")).not.toBeInTheDocument()
})
@@ -308,7 +308,7 @@ describe("OrganizationCards", () => {
expect(screen.getAllByText("Org2 Contract 1")).toHaveLength(2) // mobile + desktop
// Check Continue button URLs point to correct organizations and contracts
- const allButtons = screen.getAllByRole("link", { name: "Continue" })
+ const allButtons = screen.getAllByRole("link", { name: /^Continue / })
const org1Contract1Buttons = allButtons.filter(
(button) =>
@@ -380,7 +380,7 @@ describe("OrganizationCards", () => {
setup({ organizations: [organization] })
const continueButtons = await screen.findAllByRole("link", {
- name: "Continue",
+ name: /^Continue /,
})
expect(continueButtons[0]).toHaveAttribute(
"href",
@@ -407,7 +407,7 @@ describe("OrganizationCards", () => {
setup({ organizations: [organization] })
const continueButtons = await screen.findAllByRole("link", {
- name: "Continue",
+ name: /^Continue /,
})
expect(continueButtons[0]).toHaveAttribute(
"href",
@@ -438,7 +438,7 @@ describe("OrganizationCards", () => {
expect(screen.queryByText("Other Org Contract 1")).not.toBeInTheDocument()
expect(screen.queryByText("Other Org Contract 2")).not.toBeInTheDocument()
expect(
- screen.queryByRole("link", { name: "Continue" }),
+ screen.queryByRole("link", { name: /^Continue / }),
).not.toBeInTheDocument()
})
})
diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/OrganizationCards.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/OrganizationCards.tsx
index e0bf855256..18da6b85e5 100644
--- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/OrganizationCards.tsx
+++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/OrganizationCards.tsx
@@ -8,7 +8,6 @@ import { mitxUserQueries } from "api/mitxonline-hooks/user"
import { ButtonLink } from "@mitodl/smoot-design"
import { contractView } from "@/common/urls"
import { OrganizationPage } from "@mitodl/mitxonline-api-axios/v2"
-import { RiArrowRightLine } from "@remixicon/react"
const Wrapper = styled.div(({ theme }) => ({
display: "flex",
@@ -117,7 +116,11 @@ const OrganizationContracts: React.FC = ({
{contract.name}
- }>
+
Continue
diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.test.tsx
index 6a3b595216..c8a8ce05da 100644
--- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.test.tsx
+++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.test.tsx
@@ -14,14 +14,10 @@ import { waitFor } from "@testing-library/react"
import invariant from "tiny-invariant"
import moment from "moment"
import NiceModal from "@ebay/nice-modal-react"
-import { useFeatureFlagEnabled } from "posthog-js/react"
import { UnenrollProgramDialog } from "./DashboardDialogs"
import { mitxonlineLegacyUrl } from "@/common/mitxonline"
jest.mock("posthog-js/react")
-const mockedUseFeatureFlagEnabled = jest
- .mocked(useFeatureFlagEnabled)
- .mockImplementation(() => false)
describe("ProgramAsCourseCard", () => {
setupLocationMock()
@@ -160,7 +156,7 @@ describe("ProgramAsCourseCard", () => {
/>,
)
- const dateSummary = await screen.findByText(/until this course ends\./i)
+ const dateSummary = await screen.findByText(/ends in \d+ days/i)
await user.click(dateSummary)
expect(await screen.findByText("Important Dates:")).toBeInTheDocument()
@@ -431,47 +427,7 @@ describe("ProgramAsCourseCard", () => {
expect(certButton).not.toBeInTheDocument()
})
- test("shows legacy details link in context menu when product pages flag is disabled", async () => {
- mockedUseFeatureFlagEnabled.mockReturnValue(false)
- const cardData = setupCardData({ includeProgramEnrollment: true })
-
- renderWithProviders(
- ,
- )
-
- await screen.findByText(cardData.courseProgram.title)
- const programCard = screen.getByTestId("program-as-course-card")
- await user.click(within(programCard).getAllByLabelText("More options")[0])
-
- const detailsLink = await screen.findByRole("menuitem", {
- name: "View Course Details",
- })
- expect(detailsLink).toHaveAttribute(
- "href",
- expect.stringContaining(
- `/programs/${cardData.courseProgram.readable_id}`,
- ),
- )
- expect(detailsLink).toHaveAttribute(
- "href",
- expect.stringContaining("ecom-service=true"),
- )
-
- expect(
- screen.getByRole("menuitem", { name: "Program Record" }),
- ).toHaveAttribute(
- "href",
- mitxonlineLegacyUrl(`/records/${cardData.courseProgram.id}/`),
- )
- })
-
- test("shows product-page details link in context menu when product pages flag is enabled", async () => {
- mockedUseFeatureFlagEnabled.mockReturnValue(true)
+ test("shows product-page details link in context menu", async () => {
const cardData = setupCardData({ includeProgramEnrollment: true })
renderWithProviders(
@@ -504,7 +460,6 @@ describe("ProgramAsCourseCard", () => {
})
test("clicking Unenroll menu item opens UnenrollProgramDialog with readable_id", async () => {
- mockedUseFeatureFlagEnabled.mockReturnValue(false)
const cardData = setupCardData({ includeProgramEnrollment: true })
invariant(cardData.courseProgramEnrollment)
cardData.courseProgramEnrollment.enrollment_mode = "audit"
@@ -532,7 +487,6 @@ describe("ProgramAsCourseCard", () => {
})
test("does not show Unenroll option in context menu for verified enrollment", async () => {
- mockedUseFeatureFlagEnabled.mockReturnValue(false)
const cardData = setupCardData({ includeProgramEnrollment: true })
invariant(cardData.courseProgramEnrollment)
cardData.courseProgramEnrollment.enrollment_mode = "verified"
diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.tsx
index e643aec1d8..2a3934ea28 100644
--- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.tsx
+++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.tsx
@@ -1,13 +1,5 @@
import React from "react"
-import {
- Link,
- Popover,
- SimpleMenu,
- SimpleMenuItem,
- Stack,
- Typography,
- styled,
-} from "ol-components"
+import { SimpleMenu, SimpleMenuItem, Typography, styled } from "ol-components"
import {
CourseRunEnrollmentV3,
CourseWithCourseRunsSerializerV2,
@@ -24,9 +16,11 @@ import {
} from "./helpers"
import { ProgressBadge } from "./ProgressBadge"
import { CoursewareCard } from "./CoursewareCard"
-import { getCertificateLink } from "./CardShared"
-import { buildCourseEntry } from "./model/dashboardViewModel"
-import { formatDate } from "ol-utilities"
+import { CourseDateSummary } from "./CardShared"
+import {
+ getCertificateLink,
+ buildCourseEntry,
+} from "./model/dashboardViewModel"
import {
getIdsFromReqTree,
isVerifiedEnrollmentMode,
@@ -37,8 +31,6 @@ import { RiAwardFill, RiMore2Line } from "@remixicon/react"
import NiceModal from "@ebay/nice-modal-react"
import { UnenrollProgramDialog } from "./DashboardDialogs"
import { ProgramCertificateButton } from "./ProgramEnrollmentDisplay"
-import { useFeatureFlagEnabled } from "posthog-js/react"
-import { FeatureFlags } from "@/common/feature_flags"
import { programPageView } from "@/common/urls"
const ProgramCardRoot = styled.div(({ theme }) => ({
@@ -83,42 +75,10 @@ const StatusContainer = styled.div({
gap: "16px",
})
-const DatePopoverContent = styled.div({
- maxWidth: "240px",
- display: "flex",
- padding: "8px",
- flexDirection: "column",
- alignItems: "flex-start",
- gap: "28px",
- alignSelf: "stretch",
-})
-
-const DatePopoverTrigger = styled(Link)(({ theme }) => ({
- ...theme.typography.body2,
- color: theme.custom.colors.silverGrayDark,
- "&:hover": {
- color: theme.custom.colors.silverGrayDark,
- },
-}))
-
-const DatePopoverHeading = styled(Typography)(({ theme }) => ({
- color: theme.custom.colors.black,
-}))
-
-const DatePopoverBody = styled(Typography)(({ theme }) => ({
- color: theme.custom.colors.black,
-}))
-
const ProgramCardSubHeaderText = styled(Typography)(({ theme }) => ({
color: theme.custom.colors.silverGrayDark,
}))
-const HorizontalSeparator = styled.div(({ theme }) => ({
- width: "1px",
- height: "13px",
- backgroundColor: theme.custom.colors.lightGray2,
-}))
-
const ProgramCardSubHeader = styled.div(({ theme }) => ({
display: "flex",
padding: "8px 16px",
@@ -160,139 +120,17 @@ const MenuButton = styled(ActionButton)<{
},
])
-const getTimezone = (dateString: string): string => {
- const tz =
- new Date(dateString)
- .toLocaleString("en-US", { timeZoneName: "short" })
- .split(" ")
- .pop() || ""
- return tz
-}
-
-const MS_IN_DAY = 1000 * 60 * 60 * 24
-
-const formatDayCount = (days: number): string => {
- return `${days} day${days === 1 ? "" : "s"}`
-}
-
-interface RelativeDateContent {
- anchorLabel: string
- startVerb: "starts" | "started"
- startSuffix: string
- endVerb?: "ends" | "ended"
- endSuffix?: string
-}
-
-const getRelativeDateContent = (
- startDateString?: string | null,
- endDateString?: string | null,
- startDateDisplay?: string | null,
- endDateDisplay?: string | null,
-): RelativeDateContent | null => {
- if (!startDateString) {
- return null
- }
-
- const now = Date.now()
- const startDate = new Date(startDateString)
- if (Number.isNaN(startDate.getTime())) {
- return null
- }
-
- const hasEndDate = Boolean(endDateString)
- const endDate = hasEndDate ? new Date(endDateString as string) : null
- const hasValidEndDate = Boolean(endDate) && !Number.isNaN(endDate!.getTime())
-
- if (!hasValidEndDate) {
- if (now < startDate.getTime()) {
- const daysUntilStart = Math.max(
- 0,
- Math.ceil((startDate.getTime() - now) / MS_IN_DAY),
- )
- const dayCount = formatDayCount(daysUntilStart)
- return {
- anchorLabel: `${dayCount} until this course starts.`,
- startVerb: "starts",
- startSuffix: `in ${dayCount}${startDateDisplay ? ` on ${startDateDisplay}` : ""}.`,
- }
- }
-
- const daysSinceStart = Math.max(
- 0,
- Math.floor((now - startDate.getTime()) / MS_IN_DAY),
- )
- const dayCount = formatDayCount(daysSinceStart)
- return {
- anchorLabel: `this course started ${dayCount} ago.`,
- startVerb: "started",
- startSuffix: `${dayCount} ago${startDateDisplay ? ` on ${startDateDisplay}` : ""}.`,
- }
- }
-
- const endTime = endDate!.getTime()
-
- if (now < startDate.getTime()) {
- const daysUntilStart = Math.max(
- 0,
- Math.ceil((startDate.getTime() - now) / MS_IN_DAY),
- )
- const dayCount = formatDayCount(daysUntilStart)
- return {
- anchorLabel: `${dayCount} until this course starts.`,
- startVerb: "starts",
- startSuffix: `in ${dayCount}${startDateDisplay ? ` on ${startDateDisplay}` : ""}.`,
- endVerb: endDateDisplay ? "ends" : undefined,
- endSuffix: endDateDisplay ? `on ${endDateDisplay}.` : undefined,
- }
- }
-
- if (now <= endTime) {
- const daysUntilEnd = Math.max(0, Math.ceil((endTime - now) / MS_IN_DAY))
- const daysUntilStart = Math.max(
- 0,
- Math.floor((now - startDate.getTime()) / MS_IN_DAY),
- )
- const endDayCount = formatDayCount(daysUntilEnd)
- const startDayCount = formatDayCount(daysUntilStart)
- return {
- anchorLabel: `${endDayCount} until this course ends.`,
- startVerb: "started",
- startSuffix: `${startDayCount} ago${startDateDisplay ? ` on ${startDateDisplay}` : ""}.`,
- endVerb: "ends",
- endSuffix: `in ${endDayCount}${endDateDisplay ? ` on ${endDateDisplay}` : ""}.`,
- }
- }
-
- const daysSinceEnd = Math.max(0, Math.floor((now - endTime) / MS_IN_DAY))
- const daysSinceStart = Math.max(
- 0,
- Math.floor((now - startDate.getTime()) / MS_IN_DAY),
- )
- const endDayCount = formatDayCount(daysSinceEnd)
- const startDayCount = formatDayCount(daysSinceStart)
- return {
- anchorLabel: `this course ended ${endDayCount} ago.`,
- startVerb: "started",
- startSuffix: `${startDayCount} ago${startDateDisplay ? ` on ${startDateDisplay}` : ""}.`,
- endVerb: "ended",
- endSuffix: `${endDayCount} ago${endDateDisplay ? ` on ${endDateDisplay}` : ""}.`,
- }
-}
-
const getContextMenuItems = (
title: string,
resource: ProgramAsCourse,
enrollmentMode: string | null | undefined,
additionalItems: SimpleMenuItem[] = [],
- useProductPages = false,
) => {
const menuItems = []
- const detailsUrl = useProductPages
- ? programPageView({
- readable_id: resource.readable_id,
- display_mode: "course",
- })
- : mitxonlineLegacyUrl(`/programs/${resource.readable_id}`)
+ const detailsUrl = programPageView({
+ readable_id: resource.readable_id,
+ display_mode: "course",
+ })
const courseMenuItems = []
@@ -405,9 +243,6 @@ const ProgramAsCourseCard: React.FC = ({
className,
onUpgradeError,
}) => {
- const useProductPages = useFeatureFlagEnabled(
- FeatureFlags.MitxOnlineProductPages,
- )
const moduleRequirementSection = courseProgram?.req_tree?.find(
(node) => node.data.node_type === "operator",
)
@@ -449,23 +284,6 @@ const ProgramAsCourseCard: React.FC = ({
completedCount,
)
- const [popoverAnchorEl, setPopoverAnchorEl] =
- React.useState(null)
-
- const startDatePopoverString = courseProgram?.start_date
- ? `${formatDate(courseProgram.start_date, "MMMM D, YYYY h:mm A")} ${getTimezone(courseProgram.start_date)}`
- : null
- const endDatePopoverString = courseProgram?.end_date
- ? `${formatDate(courseProgram.end_date, "MMMM D, YYYY h:mm A")} ${getTimezone(courseProgram.end_date)}`
- : null
- const datePopoverContent = getRelativeDateContent(
- courseProgram?.start_date,
- courseProgram?.end_date,
- startDatePopoverString,
- endDatePopoverString,
- )
- const showDatePopoverTrigger = Boolean(datePopoverContent)
-
const parentProgramIds = [
courseProgram.readable_id,
...(ancestorProgramEnrollment
@@ -488,7 +306,6 @@ const ProgramAsCourseCard: React.FC = ({
courseProgram,
courseProgramEnrollment?.enrollment_mode,
contextMenuItems,
- useProductPages ?? false,
)
const contextMenu = (
@@ -518,47 +335,10 @@ const ProgramAsCourseCard: React.FC = ({
- {showDatePopoverTrigger && datePopoverContent && (
- <>
-
- setPopoverAnchorEl(null)}
- >
-
-
-
- Important Dates:
-
-
- This course{" "}
-
- {datePopoverContent.startVerb}
- {" "}
- {datePopoverContent.startSuffix}
-
-
- {datePopoverContent.endVerb &&
- datePopoverContent.endSuffix && (
-
- This course{" "}
-
- {datePopoverContent.endVerb}
- {" "}
- {datePopoverContent.endSuffix}
-
- )}
-
-
- setPopoverAnchorEl(event.currentTarget)}
- >
- {datePopoverContent.anchorLabel}
-
-
- )}
+
{courseProgram?.title}
diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramEnrollmentCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramEnrollmentCard.tsx
index f8d8fde559..2edeb8a9c5 100644
--- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramEnrollmentCard.tsx
+++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramEnrollmentCard.tsx
@@ -6,7 +6,7 @@ import {
} from "@mitodl/mitxonline-api-axios/v2"
import {
CardRoot,
- getCertificateLink,
+ CardTypeText,
MenuButton,
SubtitleLink,
TitleHeading,
@@ -18,6 +18,7 @@ import {
isVerifiedEnrollmentMode,
mitxonlineLegacyUrl,
} from "@/common/mitxonline"
+import { getCertificateLink } from "./model/dashboardViewModel"
import { ButtonLink } from "@mitodl/smoot-design"
import NiceModal from "@ebay/nice-modal-react"
import { UnenrollProgramDialog } from "./DashboardDialogs"
@@ -50,7 +51,7 @@ export const ProgramEnrollmentCard = ({
: EnrollmentStatus.Enrolled
const displayMode = program.display_mode
const titleSection = (
- <>
+
{titleHref ? (
@@ -66,11 +67,16 @@ export const ProgramEnrollmentCard = ({
View Certificate
) : null}
-
+
)
const buttonSection = (
-
- View Program
+
+ View
)
const detailsUrl = programPageView({
@@ -138,7 +144,8 @@ export const ProgramEnrollmentCard = ({
as={Component}
className={className}
>
-
+
+ Program
{titleSection}
diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.test.tsx
index 42bc5c4650..602c043769 100644
--- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.test.tsx
+++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.test.tsx
@@ -195,6 +195,94 @@ describe.each([
expect(getCard()).toHaveTextContent(/starts in 5 days/i)
})
+ // ---------------------------------------------------------------------------
+ // End date display
+ // ---------------------------------------------------------------------------
+
+ test.each([
+ {
+ endDate: moment().add(2, "days").toISOString(),
+ expectedText: /ends in 2 days/i,
+ case: "future (plural)",
+ },
+ {
+ endDate: moment().add(1, "day").toISOString(),
+ expectedText: /ends tomorrow/i,
+ case: "future (singular)",
+ },
+ {
+ endDate: moment().subtract(2, "days").toISOString(),
+ expectedText: /ended 2 days ago/i,
+ case: "past (plural)",
+ },
+ {
+ endDate: moment().subtract(1, "day").toISOString(),
+ expectedText: /ended yesterday/i,
+ case: "past (singular)",
+ },
+ ])("Shows end date text ($case)", ({ endDate, expectedText }) => {
+ setupUserApis()
+ const run = mitxonline.factories.courses.courseRun({
+ is_enrollable: true,
+ start_date: moment().subtract(30, "days").toISOString(),
+ end_date: endDate,
+ })
+ const course = mitxOnlineCourse({ courseruns: [run], next_run_id: run.id })
+ renderWithProviders()
+ expect(within(getCard()).getByText(expectedText)).toBeInTheDocument()
+ })
+
+ test("Does not show end date text when run has no end date", () => {
+ setupUserApis()
+ const run = mitxonline.factories.courses.courseRun({
+ is_enrollable: true,
+ end_date: null,
+ })
+ const course = mitxOnlineCourse({ courseruns: [run], next_run_id: run.id })
+ renderWithProviders()
+ expect(
+ within(getCard()).queryByText(/ends in \d+ day/i),
+ ).not.toBeInTheDocument()
+ expect(
+ within(getCard()).queryByText(/ended \d+ day/i),
+ ).not.toBeInTheDocument()
+ })
+
+ test.each([
+ { layout: "compact" as const, testId: "courseware-button" },
+ { layout: "default" as const, testId: undefined },
+ ])(
+ "End date shown in correct position for $layout layout",
+ ({ layout, testId }) => {
+ setupUserApis()
+ const run = mitxonline.factories.courses.courseRun({
+ is_enrollable: true,
+ start_date: moment().subtract(30, "days").toISOString(),
+ end_date: moment().add(5, "days").toISOString(),
+ })
+ const course = mitxOnlineCourse({
+ courseruns: [run],
+ next_run_id: run.id,
+ })
+ renderWithProviders(
+ ,
+ )
+ const card = getCard()
+ if (testId) {
+ // Compact: end date and button are both inside the compact-meta-row
+ const metaRow = within(card).getByTestId("compact-meta-row")
+ expect(metaRow).toHaveTextContent(/ends in 5 days/i)
+ expect(within(metaRow).getByTestId(testId)).toBeInTheDocument()
+ } else {
+ // Default layout has no compact-meta-row; end date appears below the title
+ expect(
+ within(card).queryByTestId("compact-meta-row"),
+ ).not.toBeInTheDocument()
+ expect(within(card).getByText(/ends in 5 days/i)).toBeInTheDocument()
+ }
+ },
+ )
+
// ---------------------------------------------------------------------------
// B2B enrollment flows
// ---------------------------------------------------------------------------
diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.tsx
index 0db4175d41..6f837c6df4 100644
--- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.tsx
+++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.tsx
@@ -7,10 +7,11 @@ import {
import { LoadingSpinner, Stack } from "ol-components"
import {
CardRoot,
- CourseStartCountdown,
+ CardTypeText,
CoursewareActionColumn,
CoursewareButton,
TitleText,
+ CourseDateSummary,
} from "./CardShared"
import { EnrollmentStatus, getBestRun } from "./helpers"
import { isVerifiedEnrollmentMode } from "@/common/mitxonline"
@@ -75,7 +76,6 @@ export const UnenrolledCourseCard = ({
ancestorContext?.programEnrollment?.program.readable_id,
})
}, [
- isVerifiedEnrollmentMode,
course,
ancestorContext,
readableId,
@@ -97,11 +97,18 @@ export const UnenrolledCourseCard = ({
{title}
)
+ const courseDateText = (
+
+ )
const startButton = isCompact ? (
-
-
+
+ {courseDateText}
+
{startButton}
- {courseRun?.start_date ? (
-
-
-
- ) : null}
) : (
- <>
+
{startButton}
-
+
)
- const startDateSection =
- !isCompact && courseRun?.start_date ? (
-
- ) : null
return (
<>
@@ -174,19 +169,19 @@ export const UnenrolledCourseCard = ({
className={className}
layout={layout}
>
-
+
+ Course
{titleSection}
+ {!isCompact && courseDateText}
-
-
- {buttonSection}
-
- {startDateSection}
+
+ {buttonSection}
@@ -206,18 +201,17 @@ export const UnenrolledCourseCard = ({
>
{titleSection}
+ {!isCompact && courseDateText}
- {startDateSection}
-
- {buttonSection}
-
+ {buttonSection}
diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/courseDateUtils.ts b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/courseDateUtils.ts
new file mode 100644
index 0000000000..0d0578ae33
--- /dev/null
+++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/courseDateUtils.ts
@@ -0,0 +1,122 @@
+import { calendarDaysUntil, formatCalendarDays, isInPast } from "ol-utilities"
+
+export const getCourseDateText = (
+ startDate?: string | null,
+ endDate?: string | null,
+): string | null => {
+ if (!startDate && !endDate) return null
+ const hasStarted = startDate ? isInPast(startDate) : true
+ const daysUntilStart = startDate ? calendarDaysUntil(startDate) : null
+ const daysUntilEnd = endDate ? calendarDaysUntil(endDate) : null
+ const hasEnded = endDate ? isInPast(endDate) : false
+
+ if (!hasStarted) {
+ if (daysUntilStart === null || daysUntilStart < 0) return null
+ return `Starts ${formatCalendarDays(daysUntilStart)}`
+ }
+ if (!hasEnded) {
+ if (daysUntilEnd === null || daysUntilEnd < 0) return null
+ return `Ends ${formatCalendarDays(daysUntilEnd)}`
+ }
+ if (daysUntilEnd === null) return null
+ return `Ended ${formatCalendarDays(daysUntilEnd)}`
+}
+
+const MS_IN_DAY = 1000 * 60 * 60 * 24
+
+const formatDayCount = (days: number): string =>
+ `${days} day${days === 1 ? "" : "s"}`
+
+export interface RelativeDateContent {
+ anchorLabel: string
+ startVerb: "starts" | "started"
+ startSuffix: string
+ endVerb?: "ends" | "ended"
+ endSuffix?: string
+}
+
+export const getRelativeDateContent = (
+ startDateString?: string | null,
+ endDateString?: string | null,
+ startDateDisplay?: string | null,
+ endDateDisplay?: string | null,
+): RelativeDateContent | null => {
+ if (!startDateString) return null
+
+ const now = Date.now()
+ const startDate = new Date(startDateString)
+ if (Number.isNaN(startDate.getTime())) return null
+
+ const endDate = endDateString ? new Date(endDateString) : null
+ const hasValidEndDate = Boolean(endDate) && !Number.isNaN(endDate!.getTime())
+
+ if (!hasValidEndDate) {
+ if (now < startDate.getTime()) {
+ const daysUntilStart = Math.max(
+ 0,
+ Math.ceil((startDate.getTime() - now) / MS_IN_DAY),
+ )
+ const dayCount = formatDayCount(daysUntilStart)
+ return {
+ anchorLabel: `${dayCount} until this course starts.`,
+ startVerb: "starts",
+ startSuffix: `in ${dayCount}${startDateDisplay ? ` on ${startDateDisplay}` : ""}.`,
+ }
+ }
+ const daysSinceStart = Math.max(
+ 0,
+ Math.floor((now - startDate.getTime()) / MS_IN_DAY),
+ )
+ const dayCount = formatDayCount(daysSinceStart)
+ return {
+ anchorLabel: `this course started ${dayCount} ago.`,
+ startVerb: "started",
+ startSuffix: `${dayCount} ago${startDateDisplay ? ` on ${startDateDisplay}` : ""}.`,
+ }
+ }
+
+ const endTime = endDate!.getTime()
+
+ if (now < startDate.getTime()) {
+ const daysUntilStart = Math.max(
+ 0,
+ Math.ceil((startDate.getTime() - now) / MS_IN_DAY),
+ )
+ const dayCount = formatDayCount(daysUntilStart)
+ return {
+ anchorLabel: `${dayCount} until this course starts.`,
+ startVerb: "starts",
+ startSuffix: `in ${dayCount}${startDateDisplay ? ` on ${startDateDisplay}` : ""}.`,
+ endVerb: endDateDisplay ? "ends" : undefined,
+ endSuffix: endDateDisplay ? `on ${endDateDisplay}.` : undefined,
+ }
+ }
+
+ if (now <= endTime) {
+ const daysUntilEnd = Math.max(0, Math.ceil((endTime - now) / MS_IN_DAY))
+ const daysUntilStart = Math.max(
+ 0,
+ Math.floor((now - startDate.getTime()) / MS_IN_DAY),
+ )
+ return {
+ anchorLabel: `${formatDayCount(daysUntilEnd)} until this course ends.`,
+ startVerb: "started",
+ startSuffix: `${formatDayCount(daysUntilStart)} ago${startDateDisplay ? ` on ${startDateDisplay}` : ""}.`,
+ endVerb: "ends",
+ endSuffix: `in ${formatDayCount(daysUntilEnd)}${endDateDisplay ? ` on ${endDateDisplay}` : ""}.`,
+ }
+ }
+
+ const daysSinceEnd = Math.max(0, Math.floor((now - endTime) / MS_IN_DAY))
+ const daysSinceStart = Math.max(
+ 0,
+ Math.floor((now - startDate.getTime()) / MS_IN_DAY),
+ )
+ return {
+ anchorLabel: `this course ended ${formatDayCount(daysSinceEnd)} ago.`,
+ startVerb: "started",
+ startSuffix: `${formatDayCount(daysSinceStart)} ago${startDateDisplay ? ` on ${startDateDisplay}` : ""}.`,
+ endVerb: "ended",
+ endSuffix: `${formatDayCount(daysSinceEnd)} ago${endDateDisplay ? ` on ${endDateDisplay}` : ""}.`,
+ }
+}
diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/hooks/useProgramDashboardData.ts b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/hooks/useProgramDashboardData.ts
index 4b870817a4..447e63b478 100644
--- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/hooks/useProgramDashboardData.ts
+++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/hooks/useProgramDashboardData.ts
@@ -11,9 +11,9 @@ import {
groupProgramEnrollmentsByProgramId,
groupModuleCoursesByProgramId,
buildRequirementSections,
+ getCertificateLink,
} from "../model/dashboardViewModel"
import type { RequirementSection } from "../model/dashboardViewModel"
-import { getCertificateLink } from "../CardShared"
export type ProgramDashboardData = {
sections: RequirementSection[]
diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/model/dashboardViewModel.ts b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/model/dashboardViewModel.ts
index 3287193cff..b32868a731 100644
--- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/model/dashboardViewModel.ts
+++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/model/dashboardViewModel.ts
@@ -90,6 +90,40 @@ export const EnrollmentStatus = {
export type EnrollmentStatus =
(typeof EnrollmentStatus)[keyof typeof EnrollmentStatus]
+/**
+ * Rewrites a raw mitxonline certificate link (`/certificate/{uuid}/`) to MIT
+ * Learn's own certificate route (`/certificate/{certificateType}/{uuid}/`).
+ */
+export const getCertificateLink = (
+ link: string | null | undefined,
+ certificateType: "course" | "program",
+): string | null => {
+ if (!link) return null
+ const pattern = /\/certificate\/([^/]+)\/?$/
+ return link.replace(pattern, `/certificate/${certificateType}/$1/`)
+}
+
+export const getDashboardEnrollmentStatus = (
+ resource: DashboardResource,
+): EnrollmentStatus => {
+ const hasValidCertificate =
+ resource.type !== DashboardType.Course && !!resource.data.certificate?.uuid
+
+ if (resource.type === DashboardType.Course) {
+ return EnrollmentStatus.NotEnrolled
+ }
+
+ if (resource.type === DashboardType.CourseRunEnrollment) {
+ return hasValidCertificate
+ ? EnrollmentStatus.Completed
+ : getEnrollmentStatus(resource.data)
+ }
+
+ return hasValidCertificate
+ ? EnrollmentStatus.Completed
+ : EnrollmentStatus.Enrolled
+}
+
type KeyOpts = {
resourceType: ResourceType
id: number
diff --git a/frontends/main/src/app-pages/ProductPages/CoursePage.test.tsx b/frontends/main/src/app-pages/ProductPages/CoursePage.test.tsx
index 24da3c7eda..0a1663a76f 100644
--- a/frontends/main/src/app-pages/ProductPages/CoursePage.test.tsx
+++ b/frontends/main/src/app-pages/ProductPages/CoursePage.test.tsx
@@ -16,15 +16,12 @@ import { notFound } from "next/navigation"
import { useStayUpdatedEnv } from "./test-utils/stayUpdated"
import { useFeatureFlagEnabled } from "posthog-js/react"
-import { useFeatureFlagsLoaded } from "@/common/useFeatureFlagsLoaded"
import invariant from "tiny-invariant"
import { getOutlineCoursewareId } from "./util"
import { FeatureFlags } from "@/common/feature_flags"
jest.mock("posthog-js/react")
const mockedUseFeatureFlagEnabled = jest.mocked(useFeatureFlagEnabled)
-jest.mock("@/common/useFeatureFlagsLoaded")
-const mockedUseFeatureFlagsLoaded = jest.mocked(useFeatureFlagsLoaded)
const makeCourse = factories.courses.course
const makePage = factories.pages.coursePageItem
@@ -107,35 +104,8 @@ const setupApis = ({
describe("CoursePage", () => {
beforeEach(() => {
mockedUseFeatureFlagEnabled.mockReturnValue(true)
- mockedUseFeatureFlagsLoaded.mockReturnValue(true)
})
- test.each([
- { flagsLoaded: true, isEnabled: true, shouldNotFound: false },
- { flagsLoaded: true, isEnabled: false, shouldNotFound: true },
- { flagsLoaded: false, isEnabled: true, shouldNotFound: false },
- { flagsLoaded: false, isEnabled: false, shouldNotFound: false },
- ])(
- "Calls noFound if and only the feature flag is disabled",
- async ({ flagsLoaded, isEnabled, shouldNotFound }) => {
- mockedUseFeatureFlagEnabled.mockReturnValue(isEnabled)
- mockedUseFeatureFlagsLoaded.mockReturnValue(flagsLoaded)
-
- const course = makeCourse()
- const page = makePage({ course_details: course })
- setupApis({ course, page })
- renderWithProviders(, {
- url: `/courses/${course.readable_id}/`,
- })
-
- if (shouldNotFound) {
- expect(notFound).toHaveBeenCalled()
- } else {
- expect(notFound).not.toHaveBeenCalled()
- }
- },
- )
-
test("Page has expected headings", async () => {
const course = makeCourse()
const page = makePage({ course_details: course })
diff --git a/frontends/main/src/app-pages/ProductPages/CoursePage.tsx b/frontends/main/src/app-pages/ProductPages/CoursePage.tsx
index 3b1480728c..e98cf462fe 100644
--- a/frontends/main/src/app-pages/ProductPages/CoursePage.tsx
+++ b/frontends/main/src/app-pages/ProductPages/CoursePage.tsx
@@ -19,7 +19,6 @@ import WhatYoullLearnSection from "./WhatYoullLearnSection"
import HowYoullLearnSection from "./HowYoullLearnSection"
import { DEFAULT_RESOURCE_IMG } from "ol-utilities"
import { isVerifiedEnrollmentMode } from "@/common/mitxonline"
-import { useFeatureFlagsLoaded } from "@/common/useFeatureFlagsLoaded"
import CourseInfoBox from "./InfoBoxCourse"
import CourseEnrollmentButton from "./CourseEnrollmentButton"
import CourseOutlineSection from "./CourseOutlineSection"
@@ -54,15 +53,9 @@ const CoursePage: React.FC = ({ readableId }) => {
...coursesQueries.courseOutline(effectiveOutlineCoursewareId ?? ""),
enabled: Boolean(effectiveOutlineCoursewareId),
})
- const enabled = useFeatureFlagEnabled(FeatureFlags.MitxOnlineProductPages)
const showCourseOutline = useFeatureFlagEnabled(
FeatureFlags.CourseOutlineSection,
)
- const flagsLoaded = useFeatureFlagsLoaded()
-
- if (!enabled) {
- return flagsLoaded ? notFound() : null
- }
const doneLoading = pages.isSuccess && courses.isSuccess
diff --git a/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.test.tsx b/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.test.tsx
index 6f2407442c..890dabaa43 100644
--- a/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.test.tsx
+++ b/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.test.tsx
@@ -26,19 +26,14 @@ import {
import { assertHeadings } from "ol-test-utilities"
import ProgramAsCoursePage from "./ProgramAsCoursePage"
import { notFound } from "next/navigation"
-import { useFeatureFlagEnabled } from "posthog-js/react"
import {
useStayUpdatedEnv,
PROGRAM_HIDE_STAY_UPDATED_CASES,
} from "./test-utils/stayUpdated"
import invariant from "tiny-invariant"
-import { useFeatureFlagsLoaded } from "@/common/useFeatureFlagsLoaded"
import { getIdsFromReqTree } from "@/common/mitxonline"
jest.mock("posthog-js/react")
-const mockedUseFeatureFlagEnabled = jest.mocked(useFeatureFlagEnabled)
-jest.mock("@/common/useFeatureFlagsLoaded")
-const mockedUseFeatureFlagsLoaded = jest.mocked(useFeatureFlagsLoaded)
const makeProgramAsCourse: typeof factories.programs.program = (
overrides = {},
@@ -121,11 +116,6 @@ const setupApis = ({
}
describe("ProgramAsCoursePage", () => {
- beforeEach(() => {
- mockedUseFeatureFlagEnabled.mockReturnValue(true)
- mockedUseFeatureFlagsLoaded.mockReturnValue(true)
- })
-
test("Page has expected headings", async () => {
const reqTree = new RequirementTreeBuilder()
const op = reqTree.addOperator({ operator: "all_of" })
diff --git a/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.tsx b/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.tsx
index deca2685bb..251d3355ed 100644
--- a/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.tsx
+++ b/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.tsx
@@ -6,8 +6,6 @@ import { pagesQueries } from "api/mitxonline-hooks/pages"
import { useQuery } from "@tanstack/react-query"
import { styled } from "@mitodl/smoot-design"
import { programsQueries } from "api/mitxonline-hooks/programs"
-import { useFeatureFlagEnabled } from "posthog-js/react"
-import { FeatureFlags } from "@/common/feature_flags"
import { notFound } from "next/navigation"
import { HeadingIds, parseReqTree } from "./util"
import type { RequirementItem } from "./util"
@@ -23,7 +21,6 @@ import ProductPageTemplate from "./ProductPageTemplate"
import WhatYoullLearnSection from "./WhatYoullLearnSection"
import HowYoullLearnSection from "./HowYoullLearnSection"
import { DEFAULT_RESOURCE_IMG, pluralize } from "ol-utilities"
-import { useFeatureFlagsLoaded } from "@/common/useFeatureFlagsLoaded"
import ProgramAsCourseInfoBox from "./InfoBoxProgramAsCourse"
import ProgramEnrollmentButton from "./ProgramEnrollmentButton"
import { coursesQueries } from "api/mitxonline-hooks/courses"
@@ -241,13 +238,6 @@ const ProgramAsCoursePage: React.FC = ({
enabled: programIds.length > 0,
})
- const enabled = useFeatureFlagEnabled(FeatureFlags.MitxOnlineProductPages)
- const flagsLoaded = useFeatureFlagsLoaded()
-
- if (!enabled) {
- return flagsLoaded ? notFound() : null
- }
-
const isLoading = pages.isLoading || programs.isLoading
if (!page || !program) {
diff --git a/frontends/main/src/app-pages/ProductPages/ProgramPage.test.tsx b/frontends/main/src/app-pages/ProductPages/ProgramPage.test.tsx
index 805a57e11a..9efbb549a3 100644
--- a/frontends/main/src/app-pages/ProductPages/ProgramPage.test.tsx
+++ b/frontends/main/src/app-pages/ProductPages/ProgramPage.test.tsx
@@ -31,22 +31,18 @@ import {
PROGRAM_HIDE_STAY_UPDATED_CASES,
} from "./test-utils/stayUpdated"
-import { useFeatureFlagEnabled, usePostHog } from "posthog-js/react"
+import { usePostHog } from "posthog-js/react"
import invariant from "tiny-invariant"
-import { useFeatureFlagsLoaded } from "@/common/useFeatureFlagsLoaded"
import { getIdsFromReqTree } from "@/common/mitxonline"
import { faker } from "@faker-js/faker/locale/en"
import { PostHogEvents } from "@/common/constants"
jest.mock("posthog-js/react")
-const mockedUseFeatureFlagEnabled = jest.mocked(useFeatureFlagEnabled)
const mockCapture = jest.fn()
jest.mocked(usePostHog).mockReturnValue(
// @ts-expect-error Not mocking all of posthog
{ capture: mockCapture },
)
-jest.mock("@/common/useFeatureFlagsLoaded")
-const mockedUseFeatureFlagsLoaded = jest.mocked(useFeatureFlagsLoaded)
const makeProgram = factories.programs.program
const makePage = factories.pages.programPageItem
@@ -225,36 +221,6 @@ const setupApis = ({
}
describe("ProgramPage", () => {
- beforeEach(() => {
- mockedUseFeatureFlagEnabled.mockReturnValue(true)
- })
-
- test.each([
- { flagsLoaded: true, isEnabled: true, shouldNotFound: false },
- { flagsLoaded: true, isEnabled: false, shouldNotFound: true },
- { flagsLoaded: false, isEnabled: true, shouldNotFound: false },
- { flagsLoaded: false, isEnabled: false, shouldNotFound: false },
- ])(
- "Calls notFound if and only if the feature flag is disabled",
- async ({ flagsLoaded, isEnabled, shouldNotFound }) => {
- mockedUseFeatureFlagEnabled.mockReturnValue(isEnabled)
- mockedUseFeatureFlagsLoaded.mockReturnValue(flagsLoaded)
-
- const program = makeProgram({ ...makeReqs() })
- const page = makePage({ program_details: program })
- setupApis({ program, page })
- renderWithProviders(, {
- url: `/programs/${program.readable_id}/`,
- })
-
- if (shouldNotFound) {
- expect(notFound).toHaveBeenCalled()
- } else {
- expect(notFound).not.toHaveBeenCalled()
- }
- },
- )
-
test("Page has expected headings", async () => {
const program = makeProgram({
...makeReqs({
diff --git a/frontends/main/src/app-pages/ProductPages/ProgramPage.tsx b/frontends/main/src/app-pages/ProductPages/ProgramPage.tsx
index abea97baae..8ed5cce25d 100644
--- a/frontends/main/src/app-pages/ProductPages/ProgramPage.tsx
+++ b/frontends/main/src/app-pages/ProductPages/ProgramPage.tsx
@@ -7,8 +7,6 @@ import { pagesQueries } from "api/mitxonline-hooks/pages"
import { useQuery } from "@tanstack/react-query"
import { styled } from "@mitodl/smoot-design"
import { programsQueries } from "api/mitxonline-hooks/programs"
-import { useFeatureFlagEnabled } from "posthog-js/react"
-import { FeatureFlags } from "@/common/feature_flags"
import { notFound } from "next/navigation"
import { HeadingIds, parseReqTree, RequirementData } from "./util"
import {
@@ -27,7 +25,6 @@ import type {
CourseWithCourseRunsSerializerV2,
} from "@mitodl/mitxonline-api-axios/v2"
import { DEFAULT_RESOURCE_IMG, pluralize } from "ol-utilities"
-import { useFeatureFlagsLoaded } from "@/common/useFeatureFlagsLoaded"
import ProgramInfoBox from "./InfoBoxProgram"
import { coursesQueries } from "api/mitxonline-hooks/courses"
import MitxOnlineResourceCard from "./MitxOnlineResourceCard"
@@ -258,13 +255,6 @@ const ProgramPage: React.FC = ({ readableId }) => {
enabled: programIds.length > 0,
})
- const enabled = useFeatureFlagEnabled(FeatureFlags.MitxOnlineProductPages)
- const flagsLoaded = useFeatureFlagsLoaded()
-
- if (!enabled) {
- return flagsLoaded ? notFound() : null
- }
-
const isLoading = pages.isLoading || programs.isLoading
if (!page || !program) {
diff --git a/frontends/main/src/common/certificateUtils.test.ts b/frontends/main/src/common/certificateUtils.test.ts
index 099c2c2556..a0e3d0b0d7 100644
--- a/frontends/main/src/common/certificateUtils.test.ts
+++ b/frontends/main/src/common/certificateUtils.test.ts
@@ -2,6 +2,7 @@ import {
getCertificateBadgeLines,
getCertificateBadgeTypography,
getCertificateInfo,
+ getCertificateTitle,
} from "./certificateUtils"
describe("getCertificateInfo", () => {
@@ -45,6 +46,38 @@ describe("getCertificateInfo", () => {
})
})
+describe("getCertificateTitle", () => {
+ it("prefers the CMS product name when present", () => {
+ expect(getCertificateTitle("Universal AI", "Fundamentals of ML")).toBe(
+ "Universal AI",
+ )
+ })
+
+ it("falls back to the program/course title when product name is missing", () => {
+ expect(getCertificateTitle(null, "Fundamentals of ML")).toBe(
+ "Fundamentals of ML",
+ )
+ expect(getCertificateTitle(undefined, "Fundamentals of ML")).toBe(
+ "Fundamentals of ML",
+ )
+ })
+
+ it("falls back when product name is empty or whitespace only", () => {
+ expect(getCertificateTitle("", "Fundamentals of ML")).toBe(
+ "Fundamentals of ML",
+ )
+ expect(getCertificateTitle(" ", "Fundamentals of ML")).toBe(
+ "Fundamentals of ML",
+ )
+ })
+
+ it("trims surrounding whitespace from the product name", () => {
+ expect(getCertificateTitle(" Universal AI ", "fallback")).toBe(
+ "Universal AI",
+ )
+ })
+})
+
describe("getCertificateBadgeLines", () => {
it("returns a single line for course certificates", () => {
expect(getCertificateBadgeLines()).toEqual({ primary: "Certificate" })
diff --git a/frontends/main/src/common/certificateUtils.ts b/frontends/main/src/common/certificateUtils.ts
index a7f51a236b..d40fdacb74 100644
--- a/frontends/main/src/common/certificateUtils.ts
+++ b/frontends/main/src/common/certificateUtils.ts
@@ -82,6 +82,11 @@ export const getCertificateInfo = (
displayType: resolveCertificateLabel(programType).displayType,
})
+export const getCertificateTitle = (
+ productName: string | null | undefined,
+ fallbackTitle: string,
+): string => productName?.trim() || fallbackTitle
+
const BADGE_REGISTERED_MARK_SCALE = 0.645
export type CertificateBadgeTypography = {
diff --git a/frontends/main/src/common/feature_flags.ts b/frontends/main/src/common/feature_flags.ts
index c48400231d..903425ae83 100644
--- a/frontends/main/src/common/feature_flags.ts
+++ b/frontends/main/src/common/feature_flags.ts
@@ -10,13 +10,13 @@ export enum FeatureFlags {
UniversalAI = "universal-ai",
UniversalAISearchBanner = "universal-ai-search-banner",
VideoShorts = "video-shorts",
- MitxOnlineProductPages = "mitxonline-product-pages",
CourseOutlineSection = "course-outline-section",
OcwProductPages = "ocw-product-pages",
VideoPlaylistPage = "video-playlist-page",
PodcastDetailPage = "podcast-detail-page",
B2BContractManagerDashboard = "b2b-contract-manager-dashboard",
Arithmix = "arithmix",
+ CmsCertificateTitle = "cms-certificate-title",
}
/**
diff --git a/frontends/main/src/page-components/LearningResourceExpanded/CallToActionSection.test.tsx b/frontends/main/src/page-components/LearningResourceExpanded/CallToActionSection.test.tsx
index 56fb63fabc..05ffadb86c 100644
--- a/frontends/main/src/page-components/LearningResourceExpanded/CallToActionSection.test.tsx
+++ b/frontends/main/src/page-components/LearningResourceExpanded/CallToActionSection.test.tsx
@@ -4,15 +4,13 @@ import { renderWithProviders } from "@/test-utils"
import { factories } from "api/test-utils"
import { DEFAULT_RESOURCE_IMG } from "ol-utilities"
import { getByImageSrc } from "ol-test-utilities"
-import { PlatformEnum, ResourceTypeEnum, ResourceTypeGroupEnum } from "api"
+import { PlatformEnum, ResourceTypeEnum } from "api"
import { useFeatureFlagEnabled, usePostHog } from "posthog-js/react"
import type { PostHog } from "posthog-js"
import { FeatureFlags } from "@/common/feature_flags"
-import { coursePageView, programPageView } from "@/common/urls"
import CallToActionSection from "./CallToActionSection"
import type { ImageConfig } from "ol-components"
import { kebabCase } from "lodash"
-import { faker } from "@faker-js/faker/locale/en"
jest.mock("posthog-js/react")
@@ -129,6 +127,7 @@ describe("CallToActionSection", () => {
it("adds UTM params to external URLs", () => {
const resource = factories.learningResources.resource({
resource_type: ResourceTypeEnum.Course,
+ platform: { code: PlatformEnum.Xpro },
title: "Test Course Title",
url: "https://external-site.com/course",
})
@@ -151,6 +150,7 @@ describe("CallToActionSection", () => {
it("adds UTM params to URLs with existing query params", () => {
const resource = factories.learningResources.resource({
resource_type: ResourceTypeEnum.Course,
+ platform: { code: PlatformEnum.Xpro },
title: "Test Course",
url: "https://external-site.com/course?existing=param",
})
@@ -175,6 +175,7 @@ describe("CallToActionSection", () => {
const NEXT_PUBLIC_ORIGIN = process.env.NEXT_PUBLIC_ORIGIN
const resource = factories.learningResources.resource({
resource_type: ResourceTypeEnum.Course,
+ platform: { code: PlatformEnum.Xpro },
title: "Test Course",
url: `${NEXT_PUBLIC_ORIGIN}/internal/page`,
})
@@ -198,6 +199,7 @@ describe("CallToActionSection", () => {
it("does NOT add UTM params to relative URLs", () => {
const resource = factories.learningResources.resource({
resource_type: ResourceTypeEnum.Course,
+ platform: { code: PlatformEnum.Xpro },
title: "Test Course",
url: "/relative/path",
})
@@ -220,95 +222,18 @@ describe("CallToActionSection", () => {
})
describe("MITx Online product pages", () => {
- const readableId = faker.lorem.slug()
- const url = faker.internet.url()
-
- const mitxOnlineResource: typeof factories.learningResources.resource = (
- overrides,
- ) => {
- return factories.learningResources.resource({
+ it("links to the resource's Learn product URL without rewriting it", () => {
+ // MITx Online product-page URLs are supplied by the backend ETL in
+ // `resource.url`; the component links to them directly rather than
+ // deriving a path from readable_id. The URL is same-origin (internal),
+ // so it gets no UTM params.
+ const NEXT_PUBLIC_ORIGIN = process.env.NEXT_PUBLIC_ORIGIN
+ const productUrl = `${NEXT_PUBLIC_ORIGIN}/courses/product-page-slug`
+ const resource = factories.learningResources.resource({
platform: { code: PlatformEnum.Mitxonline },
- readable_id: readableId,
- url,
- ...overrides,
- })
- }
-
- it.each([
- {
- resourceType: ResourceTypeEnum.Course,
- expectedPath: coursePageView(readableId),
- },
- {
- resourceType: ResourceTypeEnum.Program,
- resourceTypeGroup: ResourceTypeGroupEnum.Program,
- expectedPath: programPageView({
- readable_id: readableId,
- display_mode: "",
- }),
- },
- {
- resourceType: ResourceTypeEnum.Program,
- resourceTypeGroup: ResourceTypeGroupEnum.Course,
- expectedPath: programPageView({
- readable_id: readableId,
- display_mode: "course",
- }),
- },
- ])(
- "links to product page $expectedPath when flag is ON for MITx Online $resourceType",
- ({ resourceType, resourceTypeGroup, expectedPath }) => {
- mockUseFeatureFlagEnabled.mockImplementation(
- (flag) => flag === FeatureFlags.MitxOnlineProductPages,
- )
- const resource = mitxOnlineResource({
- resource_type: resourceType,
- resource_type_group: resourceTypeGroup,
- })
-
- renderWithProviders(
- ,
- )
-
- const link = screen.getByRole("link", { name: "Learn More" })
- expect(link).toHaveAttribute("href", expectedPath)
- expect(link.getAttribute("href")).not.toContain("utm_")
- },
- )
-
- it("uses external URL with UTM params when feature flag is OFF for MITx Online course/program", () => {
- mockUseFeatureFlagEnabled.mockReturnValue(false)
-
- const resource = mitxOnlineResource({
- resource_type: ResourceTypeEnum.Course,
- })
- renderWithProviders(
- ,
- )
-
- const link = screen.getByRole("link", { name: "Learn More" })
- const href = link.getAttribute("href")
- expect(href).toContain(url)
- expect(href).toContain("utm_source=mit-learn")
- expect(href).toContain("utm_medium=referral")
- })
-
- it("uses external URL with UTM params for non-MITx Online course even when flag is ON", () => {
- mockUseFeatureFlagEnabled.mockImplementation(
- (flag) => flag === FeatureFlags.MitxOnlineProductPages,
- )
-
- const resource = mitxOnlineResource({
resource_type: ResourceTypeEnum.Course,
- platform: { code: PlatformEnum.Ocw },
+ readable_id: "a-different-readable-id",
+ url: productUrl,
})
renderWithProviders(
@@ -319,13 +244,9 @@ describe("CallToActionSection", () => {
/>,
)
- const link = screen.getByRole("link", {
- name: "Access Course Materials",
- })
- const href = link.getAttribute("href")
- expect(href).toContain(url)
- expect(href).toContain("utm_source=mit-learn")
- expect(href).not.toContain("/courses/")
+ const link = screen.getByRole("link", { name: "Learn More" })
+ expect(link).toHaveAttribute("href", productUrl)
+ expect(link.getAttribute("href")).not.toContain("utm_")
})
})
@@ -430,9 +351,9 @@ describe("CallToActionSection", () => {
(flag) => flag === FeatureFlags.OcwProductPages,
)
const resource = factories.learningResources.resource({
- platform: { code: PlatformEnum.Mitxonline },
+ platform: { code: PlatformEnum.Xpro },
resource_type: ResourceTypeEnum.Course,
- url: "https://courses.mitxonline.mit.edu/learn/course/some-course/",
+ url: "https://xpro.mit.edu/courses/some-course/",
})
renderWithProviders(
@@ -445,7 +366,7 @@ describe("CallToActionSection", () => {
const link = screen.getByRole("link", { name: "Learn More" })
const href = link.getAttribute("href")
- expect(href).toContain("mitxonline.mit.edu")
+ expect(href).toContain("xpro.mit.edu")
expect(href).not.toContain("/courses/o/")
})
diff --git a/frontends/main/src/page-components/LearningResourceExpanded/CallToActionSection.tsx b/frontends/main/src/page-components/LearningResourceExpanded/CallToActionSection.tsx
index 22a5316d9e..accc42fd60 100644
--- a/frontends/main/src/page-components/LearningResourceExpanded/CallToActionSection.tsx
+++ b/frontends/main/src/page-components/LearningResourceExpanded/CallToActionSection.tsx
@@ -17,7 +17,7 @@ import {
resourceContentFilesImageSrc,
useImageWithFallback,
} from "ol-utilities"
-import { ResourceTypeEnum, ResourceTypeGroupEnum, PlatformEnum } from "api"
+import { ResourceTypeEnum, PlatformEnum } from "api"
import {
Button,
ButtonLink,
@@ -43,8 +43,6 @@ import {
FACEBOOK_SHARE_BASE_URL,
TWITTER_SHARE_BASE_URL,
LINKEDIN_SHARE_BASE_URL,
- coursePageView,
- programPageView,
videoDetailPageView,
videoPlaylistPageView,
podcastPageView,
@@ -52,7 +50,6 @@ import {
ocwLearnPageView,
} from "@/common/urls"
import { parentPodcastIds, videoPlaylistIds } from "@/common/slugs"
-import { DisplayModeEnum } from "@mitodl/mitxonline-api-axios/v2"
import { FeatureFlags } from "@/common/feature_flags"
import { externalLinkProps } from "@/common/utils"
@@ -320,37 +317,12 @@ const getResourceUrl = (
resource: LearningResource,
{
ocwProductPages,
- mitxonlineProductPages,
showPodcastPage,
}: {
- mitxonlineProductPages?: boolean
showPodcastPage?: boolean
ocwProductPages?: boolean
},
) => {
- if (
- mitxonlineProductPages &&
- resource.platform?.code === PlatformEnum.Mitxonline
- ) {
- /**
- * TODO: After mitxonlineProductPages feature flag is fully rolled out,
- * this logic should be handled during ETL
- */
- if (resource.resource_type === ResourceTypeEnum.Course) {
- return coursePageView(resource.readable_id)
- } else if (resource.resource_type === ResourceTypeEnum.Program) {
- return programPageView({
- readable_id: resource.readable_id,
- // Learn program resources that have resource_type_group correspond to
- // MITxOnline programs with display_mode="course"
- // This can be moved into backend ETL after feature flags are removed.
- display_mode:
- resource.resource_type_group === ResourceTypeGroupEnum.Course
- ? DisplayModeEnum.Course
- : undefined,
- })
- }
- }
if (resource.resource_type === ResourceTypeEnum.VideoPlaylist) {
return videoPlaylistPageView(resource.id.toString(), resource.title)
}
@@ -413,9 +385,6 @@ const CallToActionSection = ({
const [shareExpanded, setShareExpanded] = useState(false)
const [copyText, setCopyText] = useState("Copy Link")
const ocwProductPages = useFeatureFlagEnabled(FeatureFlags.OcwProductPages)
- const mitxonlineProductPages = useFeatureFlagEnabled(
- FeatureFlags.MitxOnlineProductPages,
- )
const showPodcastPage = useFeatureFlagEnabled(FeatureFlags.PodcastDetailPage)
if (hide) {
@@ -444,7 +413,6 @@ const CallToActionSection = ({
const socialIconSize = 18
const url = appendUtmParams(
getResourceUrl(resource, {
- mitxonlineProductPages,
showPodcastPage,
ocwProductPages,
}),
diff --git a/frontends/ol-components/src/components/Dialog/Dialog.tsx b/frontends/ol-components/src/components/Dialog/Dialog.tsx
index 9504d05211..21f51bfa23 100644
--- a/frontends/ol-components/src/components/Dialog/Dialog.tsx
+++ b/frontends/ol-components/src/components/Dialog/Dialog.tsx
@@ -7,7 +7,7 @@ import type { DialogProps as MuiDialogProps } from "@mui/material/Dialog"
import { Button, ActionButton } from "@mitodl/smoot-design"
import MuiDialogActions from "@mui/material/DialogActions"
import { RiCloseLine } from "@remixicon/react"
-import Typography from "@mui/material/Typography"
+import MuiTypography from "@mui/material/Typography"
const Close = styled.div`
position: absolute;
@@ -38,13 +38,19 @@ const DialogActions = styled(MuiDialogActions)`
}
`
+const HeaderTitle = styled(MuiTypography)`
+ display: flex;
+ align-items: center;
+ gap: 12px;
+` as typeof MuiTypography
+
type DialogProps = {
className?: string
contentCss?: CSSObject
open: boolean
onClose: () => void
onConfirm?: () => void | Promise
- title?: string
+ title?: React.ReactNode
message?: string
children?: React.ReactNode
/**
@@ -71,6 +77,7 @@ type DialogProps = {
scroll?: MuiDialogProps["scroll"]
TransitionProps?: NonNullable["transition"]
"aria-describedby"?: string
+ role?: MuiDialogProps["role"]
}
/**
@@ -101,6 +108,7 @@ const Dialog: React.FC = ({
scroll,
TransitionProps,
"aria-describedby": ariaDescribedBy,
+ role,
}) => {
const [confirming, setConfirming] = useState(isSubmitting)
const titleId = useId()
@@ -130,6 +138,7 @@ const Dialog: React.FC = ({
}}
aria-labelledby={titleId}
aria-describedby={ariaDescribedBy}
+ role={role}
transitionDuration={process.env.NODE_ENV === "test" ? 0 : undefined}
maxWidth={maxWidth}
scroll={scroll}
@@ -141,13 +150,13 @@ const Dialog: React.FC = ({
{title && (
)}
- {message && {message}}
+ {message && {message}}
{children}
{actions !== undefined ? (
diff --git a/frontends/ol-components/src/components/FormDialog/FormDialog.tsx b/frontends/ol-components/src/components/FormDialog/FormDialog.tsx
index 29aa4647bb..d4f99e95d6 100644
--- a/frontends/ol-components/src/components/FormDialog/FormDialog.tsx
+++ b/frontends/ol-components/src/components/FormDialog/FormDialog.tsx
@@ -65,6 +65,7 @@ interface FormDialogProps {
maxWidth?: DialogProps["maxWidth"]
disabled?: boolean
TransitionProps?: DialogProps["TransitionProps"]
+ "aria-describedby"?: string
}
/**
@@ -93,6 +94,7 @@ const FormDialog: React.FC = ({
maxWidth,
disabled = false,
TransitionProps,
+ "aria-describedby": ariaDescribedBy,
}) => {
const [isSubmitting, setIsSubmitting] = useState(false)
const handleSubmit: React.FormEventHandler = useCallback(
@@ -140,6 +142,7 @@ const FormDialog: React.FC = ({
maxWidth={maxWidth}
disabled={isSubmitting || disabled}
TransitionProps={TransitionProps}
+ aria-describedby={ariaDescribedBy}
>
{children}
diff --git a/frontends/ol-utilities/src/date/utils.test.ts b/frontends/ol-utilities/src/date/utils.test.ts
index 51670f71e3..3f006297ac 100644
--- a/frontends/ol-utilities/src/date/utils.test.ts
+++ b/frontends/ol-utilities/src/date/utils.test.ts
@@ -1,3 +1,4 @@
+import moment from "moment"
import * as u from "./utils"
import { setDefaultTimezone } from "ol-test-utilities"
@@ -29,6 +30,54 @@ describe("formatDurationHuman", () => {
})
})
+describe("formatCalendarDays", () => {
+ const FAKE_NOW = "2024-01-15T12:00:00Z"
+
+ beforeEach(() => {
+ jest.useFakeTimers()
+ jest.setSystemTime(new Date(FAKE_NOW))
+ setDefaultTimezone("UTC")
+ })
+
+ afterEach(() => {
+ jest.useRealTimers()
+ })
+
+ it("returns 'Today' for 0", () => {
+ expect(u.formatCalendarDays(0)).toBe("Today")
+ })
+
+ it("returns 'Tomorrow' / 'Yesterday' for ±1", () => {
+ expect(u.formatCalendarDays(1)).toBe("Tomorrow")
+ expect(u.formatCalendarDays(-1)).toBe("Yesterday")
+ })
+
+ it("returns relative strings for values within ±90 days", () => {
+ expect(u.formatCalendarDays(5)).toBe("in 5 days")
+ expect(u.formatCalendarDays(90)).toBe("in 90 days")
+ expect(u.formatCalendarDays(-5)).toBe("5 days ago")
+ expect(u.formatCalendarDays(-90)).toBe("90 days ago")
+ })
+
+ it("returns a short Intl-formatted date for abs > 90 days in the future", () => {
+ const days = 100
+ const locale = Intl.DateTimeFormat().resolvedOptions().locale
+ const expected = Intl.DateTimeFormat(locale, { dateStyle: "short" }).format(
+ moment(FAKE_NOW).add(days, "days").toDate(),
+ )
+ expect(u.formatCalendarDays(days)).toBe(expected)
+ })
+
+ it("returns a short Intl-formatted date for abs > 90 days in the past", () => {
+ const days = -100
+ const locale = Intl.DateTimeFormat().resolvedOptions().locale
+ const expected = Intl.DateTimeFormat(locale, { dateStyle: "short" }).format(
+ moment(FAKE_NOW).add(days, "days").toDate(),
+ )
+ expect(u.formatCalendarDays(days)).toBe(expected)
+ })
+})
+
describe("calendarDaysUntil", () => {
test("Gets calendar days until date, respecting timezone", () => {
// UTC EST
diff --git a/frontends/ol-utilities/src/date/utils.ts b/frontends/ol-utilities/src/date/utils.ts
index 948945177c..29f880d013 100644
--- a/frontends/ol-utilities/src/date/utils.ts
+++ b/frontends/ol-utilities/src/date/utils.ts
@@ -90,10 +90,39 @@ const isInPast = (date: string): null | boolean => {
return x.isBefore(moment())
}
+/**
+ * Converts a `calendarDaysUntil` value to a human-readable relative-day suffix.
+ * Positive = future ("Tomorrow" / "in N days"), negative = past ("Yesterday" / "N days ago"), 0 = "Today".
+ */
+const formatCalendarDays = (days: number): string => {
+ const abs = Math.abs(days)
+ if (abs === 0) return "Today"
+ if (abs > 90) {
+ const browserLocale = Intl.DateTimeFormat().resolvedOptions().locale
+ return Intl.DateTimeFormat(browserLocale, { dateStyle: "short" }).format(
+ moment().add(days, "days").toDate(),
+ )
+ }
+ if (days > 0) return days === 1 ? "Tomorrow" : `in ${days} days`
+ return abs === 1 ? "Yesterday" : `${abs} days ago`
+}
+
+/** Extracts the short timezone abbreviation from a date string, e.g. "EST". */
+const getTimezone = (dateString: string): string => {
+ return (
+ new Date(dateString)
+ .toLocaleString("en-US", { timeZoneName: "short" })
+ .split(" ")
+ .pop() ?? ""
+ )
+}
+
export {
formatDate,
formatDurationClockTime,
formatDurationHuman,
isInPast,
calendarDaysUntil,
+ formatCalendarDays,
+ getTimezone,
}
diff --git a/frontends/ol-utilities/src/string/utils.test.ts b/frontends/ol-utilities/src/string/utils.test.ts
index 1a1ff35ccf..4757681e9b 100644
--- a/frontends/ol-utilities/src/string/utils.test.ts
+++ b/frontends/ol-utilities/src/string/utils.test.ts
@@ -38,7 +38,7 @@ describe("extractEmailsFromCsvRows", () => {
},
)
- test("counts data rows with no @ as skipped (not header rows)", () => {
+ test("counts data rows with no @ as skipped (no header row — column unknown)", () => {
const { valid, skippedCount } = extract([
["alice@example.com"],
["no-email-here"],
@@ -48,6 +48,28 @@ describe("extractEmailsFromCsvRows", () => {
expect(skippedCount).toBe(1)
})
+ test("treats no-@ values as invalid when email column is identified via header", () => {
+ const { valid, invalid, skippedCount } = extract([
+ ["id", "name", "email"],
+ ["1", "Alice", "alice@example.com"],
+ ["2", "Bob", "notanemail"],
+ ["3", "Carol", "carol@example.com"],
+ ])
+ expect(valid).toEqual(["alice@example.com", "carol@example.com"])
+ expect(invalid).toEqual(["notanemail"])
+ expect(skippedCount).toBe(0)
+ })
+
+ test("skips rows with blank email column when header is present", () => {
+ const { valid, skippedCount } = extract([
+ ["id", "name", "email"],
+ ["1", "Alice", "alice@example.com"],
+ ["2", "Bob", ""],
+ ])
+ expect(valid).toEqual(["alice@example.com"])
+ expect(skippedCount).toBe(1)
+ })
+
test("finds email in any column, not just the first", () => {
const { valid } = extract([
["1", "Alice Smith", "alice@example.com"],
@@ -77,13 +99,26 @@ describe("extractEmailsFromCsvRows", () => {
})
test("deduplicates emails case-insensitively and counts duplicates", () => {
- const { valid, duplicateCount } = extract([
+ const { valid, duplicateCount, duplicateEmails } = extract([
["alice@example.com"],
["Alice@Example.COM"],
["bob@example.com"],
])
expect(valid).toEqual(["alice@example.com", "bob@example.com"])
expect(duplicateCount).toBe(1)
+ expect(duplicateEmails).toEqual(["Alice@Example.COM"])
+ })
+
+ test("does not add repeated entries to duplicateEmails when same address appears 3+ times", () => {
+ const { valid, duplicateCount, duplicateEmails } = extract([
+ ["alice@example.com"],
+ ["alice@example.com"],
+ ["alice@example.com"],
+ ["bob@example.com"],
+ ])
+ expect(valid).toEqual(["alice@example.com", "bob@example.com"])
+ expect(duplicateCount).toBe(2)
+ expect(duplicateEmails).toEqual(["alice@example.com"])
})
test("returns clean result with empty arrays for valid input", () => {
@@ -91,6 +126,7 @@ describe("extractEmailsFromCsvRows", () => {
expect(result).toEqual({
valid: ["alice@example.com", "bob@example.com"],
invalid: [],
+ duplicateEmails: [],
duplicateCount: 0,
skippedCount: 0,
})
@@ -134,6 +170,7 @@ describe("parseEmailsForSubmit", () => {
expect(result).toEqual({
valid: ["alice@example.com", "bob@example.com"],
invalid: [],
+ duplicateEmails: [],
duplicateCount: 0,
skippedCount: 0,
})
@@ -146,11 +183,12 @@ describe("parseEmailsForSubmit", () => {
})
test("deduplicates valid emails case-insensitively", () => {
- const { valid, duplicateCount } = parse(
+ const { valid, duplicateCount, duplicateEmails } = parse(
"alice@example.com\nAlice@Example.COM\nbob@example.com",
)
expect(valid).toEqual(["alice@example.com", "bob@example.com"])
expect(duplicateCount).toBe(1)
+ expect(duplicateEmails).toEqual(["Alice@Example.COM"])
})
test("handles newline-separated input", () => {
@@ -163,6 +201,7 @@ describe("parseEmailsForSubmit", () => {
expect(result).toEqual({
valid: [],
invalid: [],
+ duplicateEmails: [],
duplicateCount: 0,
skippedCount: 0,
})
diff --git a/frontends/ol-utilities/src/string/utils.ts b/frontends/ol-utilities/src/string/utils.ts
index 0bfbaaf547..191a1d94f0 100644
--- a/frontends/ol-utilities/src/string/utils.ts
+++ b/frontends/ol-utilities/src/string/utils.ts
@@ -17,6 +17,7 @@ export const parseEmails = (input: string) =>
export type EmailParseResult = {
valid: string[]
invalid: string[]
+ duplicateEmails: string[]
duplicateCount: number
skippedCount: number
}
@@ -47,37 +48,77 @@ export const extractEmailsFromCsvRows = (
const invalid: string[] = []
let skippedCount = 0
- const dataRows =
- rows.length > 0 && isHeaderRow(rows[0]) ? rows.slice(1) : rows
+ // When the CSV has an identifiable email column header, record its index so
+ // we can validate values in that column even when they contain no "@".
+ // Without a header we fall back to finding any column that contains "@".
+ let dataRows: string[][]
+ let emailColIndex = -1
+
+ if (rows.length > 0 && isHeaderRow(rows[0])) {
+ dataRows = rows.slice(1)
+ emailColIndex = rows[0].findIndex((c) => EMAIL_HEADER_RE.test(c.trim()))
+ } else {
+ dataRows = rows
+ }
for (const cols of dataRows) {
- const emailCol = cols.map((c) => c.trim()).find((c) => c.includes("@"))
- if (!emailCol) {
- skippedCount++
- continue
- }
- if (EMAIL_REGEX.test(emailCol)) {
- valid.push(emailCol)
+ const trimmed = cols.map((c) => c.trim())
+
+ if (emailColIndex >= 0) {
+ const emailValue = trimmed[emailColIndex] ?? ""
+ if (!emailValue) {
+ // Blank email column — truly no data provided, skip the row.
+ skippedCount++
+ continue
+ }
+ if (EMAIL_REGEX.test(emailValue)) {
+ valid.push(emailValue)
+ } else {
+ // Non-empty value in a known email column that fails validation is
+ // invalid, even if it has no "@" (e.g. "isabeltaylor").
+ invalid.push(emailValue)
+ }
} else {
- invalid.push(emailCol)
+ const emailCol = trimmed.find((c) => c.includes("@"))
+ if (!emailCol) {
+ skippedCount++
+ continue
+ }
+ if (EMAIL_REGEX.test(emailCol)) {
+ valid.push(emailCol)
+ } else {
+ invalid.push(emailCol)
+ }
}
}
const seen = new Set()
const deduped: string[] = []
+ const duplicateEmails: string[] = []
+ const duplicateSeen = new Set()
let duplicateCount = 0
for (const email of valid) {
const key = email.toLowerCase()
if (seen.has(key)) {
duplicateCount++
+ if (!duplicateSeen.has(key)) {
+ duplicateEmails.push(email)
+ duplicateSeen.add(key)
+ }
} else {
seen.add(key)
deduped.push(email)
}
}
- return { valid: deduped, invalid, duplicateCount, skippedCount }
+ return {
+ valid: deduped,
+ invalid,
+ duplicateEmails,
+ duplicateCount,
+ skippedCount,
+ }
}
/**
@@ -103,19 +144,31 @@ export const parseEmailsForSubmit = (input: string): EmailParseResult => {
const seen = new Set()
const deduped: string[] = []
+ const duplicateEmails: string[] = []
+ const duplicateSeen = new Set()
let duplicateCount = 0
for (const email of valid) {
const key = email.toLowerCase()
if (seen.has(key)) {
duplicateCount++
+ if (!duplicateSeen.has(key)) {
+ duplicateEmails.push(email)
+ duplicateSeen.add(key)
+ }
} else {
seen.add(key)
deduped.push(email)
}
}
- return { valid: deduped, invalid, duplicateCount, skippedCount: 0 }
+ return {
+ valid: deduped,
+ invalid,
+ duplicateEmails,
+ duplicateCount,
+ skippedCount: 0,
+ }
}
export const initials = (title: string): string => {
diff --git a/learning_resources/etl/constants.py b/learning_resources/etl/constants.py
index 7f7ee4cb65..7f6351e55e 100644
--- a/learning_resources/etl/constants.py
+++ b/learning_resources/etl/constants.py
@@ -95,6 +95,15 @@ class ETLSource(ExtendedEnum):
ovs = "ovs"
+QDRANT_RETAINED_SOURCES = (
+ ETLSource.mit_edx.value,
+ ETLSource.mitxonline.value,
+ ETLSource.xpro.value,
+ ETLSource.oll.value,
+ ETLSource.canvas.value,
+)
+
+
class CourseNumberType(Enum):
"""Enum of course number types"""
diff --git a/learning_resources/etl/edx_shared.py b/learning_resources/etl/edx_shared.py
index 060a2493b0..365210123d 100644
--- a/learning_resources/etl/edx_shared.py
+++ b/learning_resources/etl/edx_shared.py
@@ -77,8 +77,8 @@ def build_run_lookup(
Args:
etl_source(str): The ETL source
ids(list of int): List of LearningResource IDs to filter by.
- If empty/falsy, all published/test_mode runs for the source
- are included.
+ If empty/falsy, all runs of every published/test_mode course
+ for the source are included.
Returns:
dict: Mapping of normalized run_id -> list of LearningResourceRun
@@ -87,7 +87,6 @@ def build_run_lookup(
LearningResourceRun.objects.filter(
learning_resource__etl_source=etl_source,
)
- .filter(Q(published=True) | Q(learning_resource__test_mode=True))
.filter(
Q(learning_resource__published=True) | Q(learning_resource__test_mode=True)
)
@@ -107,6 +106,9 @@ def build_run_lookup(
for run in runs:
normalized = normalize_run_id(etl_source, run.run_id)
lookup.setdefault(normalized, []).append(run)
+ if etl_source == ETLSource.oll.name:
+ # OLL archive filenames omit the MITx+ prefix the run_ids carry
+ lookup.setdefault(normalized.removeprefix("mitx."), []).append(run)
return lookup
@@ -256,10 +258,12 @@ def sync_edx_archive(
trigger_resource_etl(etl_source)
return
course = run.learning_resource
- if course.published and not course.test_mode and course.best_run != run:
- # This is not the best run for the published course, so skip it
+ if not course.published and not course.test_mode:
+ # Fully-retired course; skip it
log.warning(
- "%s not the best run for %s, skipping", run.run_id, course.readable_id
+ "%s belongs to a retired course %s, skipping",
+ run.run_id,
+ course.readable_id,
)
return
bucket = get_bucket_by_name(settings.COURSE_ARCHIVE_BUCKET_NAME)
@@ -278,14 +282,10 @@ def run_for_edx_archive(etl_source: str, archive_filename: str):
LearningResourceRun or None: The matching run, or None if not found
"""
normalized_run_id = extract_run_id_from_key(etl_source, archive_filename)
- runs = (
- LearningResourceRun.objects.filter(
- learning_resource__etl_source=etl_source,
- )
- .filter(Q(published=True) | Q(learning_resource__test_mode=True))
- .filter(
- Q(learning_resource__published=True) | Q(learning_resource__test_mode=True)
- )
+ runs = LearningResourceRun.objects.filter(
+ learning_resource__etl_source=etl_source,
+ ).filter(
+ Q(learning_resource__published=True) | Q(learning_resource__test_mode=True)
)
if etl_source in (ETLSource.mit_edx.name, ETLSource.oll.name):
runs = runs.filter(run_id__iregex=normalized_run_id)
@@ -327,10 +327,4 @@ def sync_edx_course_files(
log.warning("There are %d runs for %s", len(matching_runs), key)
run = matching_runs[0]
- course = run.learning_resource
-
- if course.published and not course.test_mode and course.best_run != run:
- # This is not the best run for the published course, so skip it
- log.debug("Not the best run for %s, skipping", run.run_id)
- continue
process_course_archive(bucket, key, run, overwrite=overwrite)
diff --git a/learning_resources/etl/edx_shared_test.py b/learning_resources/etl/edx_shared_test.py
index 85244874b1..d702f84861 100644
--- a/learning_resources/etl/edx_shared_test.py
+++ b/learning_resources/etl/edx_shared_test.py
@@ -36,13 +36,11 @@
(ETLSource.oll.name, PlatformType.edx.name),
],
)
-@pytest.mark.parametrize("published", [True, False])
def test_sync_edx_course_files(
mock_course_archive_bucket,
mocker,
source,
platform,
- published,
): # pylint: disable=too-many-arguments,too-many-locals
"""Sync edx courses from a tarball stored in S3"""
mock_load_content_files = mocker.patch(
@@ -61,47 +59,69 @@ def test_sync_edx_course_files(
"learning_resources.etl.edx_shared.get_bucket_by_name",
return_value=mock_course_archive_bucket.bucket,
)
+ platform_obj = LearningResourcePlatformFactory.create(code=platform)
courses = LearningResourceFactory.create_batch(
2,
- platform=LearningResourcePlatformFactory.create(code=platform),
+ platform=platform_obj,
etl_source=source,
is_course=True,
published=True,
create_runs=False,
)
+ # A fully-retired (unpublished, non-test_mode) course must be excluded
+ retired_course = LearningResourceFactory.create(
+ platform=platform_obj,
+ etl_source=source,
+ is_course=True,
+ published=False,
+ test_mode=False,
+ create_runs=False,
+ )
keys = []
s3_prefix = get_s3_prefix_for_source(source)
- for course in courses:
- runs = LearningResourceRunFactory.create_batch(
- 2,
- learning_resource=course,
- published=published,
- )
- course.refresh_from_db()
- if published:
- assert course.best_run in runs
- keys.extend(
+
+ def add_keys_for_runs(runs):
+ new_keys = (
[f"{s3_prefix}/{run.run_id}/foo.tar.gz" for run in runs]
if source != ETLSource.oll.name
else [f"{s3_prefix}/{run.run_id}_OLL.tar.gz" for run in runs]
)
- for key in keys:
+ keys.extend(new_keys)
+ for key in new_keys:
with Path.open(
Path("test_json/course-v1:MITxT+8.01.3x+3T2022.tar.gz"), "rb"
) as infile:
- bucket.put_object(
- Key=key,
- Body=infile.read(),
- ACL="public-read",
- )
- sync_edx_course_files(source, [course.id for course in courses], keys)
- # Only best runs for published courses are processed, so 2 runs (one per course) not 4
- expected_calls = 2 if published else 0
- assert mock_transform.call_count == expected_calls
- assert mock_load_content_files.call_count == expected_calls
- if published:
- for course in courses:
- mock_load_content_files.assert_any_call(course.best_run, fake_data)
+ bucket.put_object(Key=key, Body=infile.read(), ACL="public-read")
+
+ for course in courses:
+ runs = LearningResourceRunFactory.create_batch(
+ 2,
+ learning_resource=course,
+ published=True,
+ )
+ course.refresh_from_db()
+ assert course.best_run in runs
+ add_keys_for_runs(runs)
+
+ retired_runs = LearningResourceRunFactory.create_batch(
+ 2,
+ learning_resource=retired_course,
+ published=True,
+ )
+ add_keys_for_runs(retired_runs)
+
+ sync_edx_course_files(
+ source, [course.id for course in [*courses, retired_course]], keys
+ )
+ # All runs of published courses are processed; retired-course runs are excluded
+ assert mock_transform.call_count == 4
+ assert mock_load_content_files.call_count == 4
+ loaded_runs = {call.args[0] for call in mock_load_content_files.call_args_list}
+ for course in courses:
+ for run in course.runs.all():
+ assert run in loaded_runs
+ for run in retired_runs:
+ assert run not in loaded_runs
mock_log.assert_not_called()
@@ -692,8 +712,10 @@ def test_sync_edx_archive_no_run_found(mocker, mock_course_archive_bucket, etl_s
@pytest.mark.parametrize("etl_source", [ETLSource.mitxonline.name, ETLSource.xpro.name])
-def test_sync_edx_archive_not_best_run(mocker, mock_course_archive_bucket, etl_source):
- """Test sync_edx_archive skips processing when run is not the best run"""
+def test_sync_edx_archive_non_best_run_processed(
+ mocker, mock_course_archive_bucket, etl_source
+):
+ """sync_edx_archive now processes a non-best run of a published course."""
from learning_resources.etl.edx_shared import sync_edx_archive
platform = (
@@ -701,15 +723,12 @@ def test_sync_edx_archive_not_best_run(mocker, mock_course_archive_bucket, etl_s
if etl_source == ETLSource.mitxonline.name
else PlatformType.xpro.name
)
-
- # Create a course with multiple runs
course = LearningResourceFactory.create(
platform=LearningResourcePlatformFactory.create(code=platform),
etl_source=etl_source,
published=True,
create_runs=False,
)
- # Create older run (not best) with earlier start date
from datetime import UTC, datetime
old_run = LearningResourceRunFactory.create(
@@ -718,7 +737,6 @@ def test_sync_edx_archive_not_best_run(mocker, mock_course_archive_bucket, etl_s
run_id="course-v1:Test+Course+R1",
start_date=datetime(2022, 1, 1, tzinfo=UTC),
)
- # Create newer run (will be best) with later start date
LearningResourceRunFactory.create(
learning_resource=course,
published=True,
@@ -726,35 +744,72 @@ def test_sync_edx_archive_not_best_run(mocker, mock_course_archive_bucket, etl_s
start_date=datetime(2023, 1, 1, tzinfo=UTC),
)
course.refresh_from_db()
-
- # Verify the newer run is the best run
assert course.best_run.run_id == "course-v1:Test+Course+R2"
- # Archive is for the old run, not the best run
bucket = mock_course_archive_bucket.bucket
s3_key = "20220101/courses/course-v1:Test+Course+R1/abcdefghijklmnop.tar.gz"
-
with Path.open(
Path("test_json/course-v1:MITxT+8.01.3x+3T2022.tar.gz"), "rb"
) as infile:
bucket.put_object(Key=s3_key, Body=infile.read(), ACL="public-read")
+ mocker.patch(
+ "learning_resources.etl.edx_shared.get_bucket_by_name", return_value=bucket
+ )
+ mock_process = mocker.patch(
+ "learning_resources.etl.edx_shared.process_course_archive"
+ )
+ sync_edx_archive(etl_source, s3_key, overwrite=False)
+
+ mock_process.assert_called_once()
+ assert mock_process.call_args[0][2] == old_run
+
+
+@pytest.mark.parametrize("etl_source", [ETLSource.mitxonline.name, ETLSource.xpro.name])
+def test_sync_edx_archive_retired_course_skipped(
+ mocker, mock_course_archive_bucket, etl_source
+):
+ """sync_edx_archive still skips runs of a fully-retired (unpublished, non-test_mode) course."""
+ from learning_resources.etl.edx_shared import sync_edx_archive
+
+ platform = (
+ PlatformType.mitxonline.name
+ if etl_source == ETLSource.mitxonline.name
+ else PlatformType.xpro.name
+ )
+ course = LearningResourceFactory.create(
+ platform=LearningResourcePlatformFactory.create(code=platform),
+ etl_source=etl_source,
+ published=False,
+ test_mode=False,
+ create_runs=False,
+ )
+ LearningResourceRunFactory.create(
+ learning_resource=course,
+ published=False,
+ run_id="course-v1:Test+Course+R1",
+ )
+ bucket = mock_course_archive_bucket.bucket
+ s3_key = "20220101/courses/course-v1:Test+Course+R1/abcdefghijklmnop.tar.gz"
+ with Path.open(
+ Path("test_json/course-v1:MITxT+8.01.3x+3T2022.tar.gz"), "rb"
+ ) as infile:
+ bucket.put_object(Key=s3_key, Body=infile.read(), ACL="public-read")
mocker.patch(
- "learning_resources.etl.edx_shared.get_bucket_by_name",
- return_value=bucket,
+ "learning_resources.etl.edx_shared.get_bucket_by_name", return_value=bucket
+ )
+ # No matching run is returned for a retired course, so ETL is triggered instead.
+ mock_trigger = mocker.patch(
+ "learning_resources.etl.edx_shared.trigger_resource_etl"
)
mock_process = mocker.patch(
"learning_resources.etl.edx_shared.process_course_archive"
)
- mock_log = mocker.patch("learning_resources.etl.edx_shared.log.warning")
sync_edx_archive(etl_source, s3_key, overwrite=False)
- # Should log warning and not process
- assert mock_log.called
- assert "not the best run" in mock_log.call_args[0][0]
- assert old_run.run_id in mock_log.call_args[0][1]
mock_process.assert_not_called()
+ mock_trigger.assert_called_once_with(etl_source)
@pytest.mark.parametrize("etl_source", [ETLSource.mitxonline.name, ETLSource.xpro.name])
@@ -1063,6 +1118,22 @@ def test_build_run_lookup(source, platform):
assert lookup[normalized][0].id == run.id
+def test_build_run_lookup_oll_strips_mitx_prefix():
+ """OLL runs are also indexed without the MITx prefix the archives omit"""
+ source = ETLSource.oll.name
+ course = LearningResourceFactory.create(
+ etl_source=source, published=True, create_runs=False
+ )
+ run = LearningResourceRunFactory.create(
+ learning_resource=course, run_id="MITx+0.501x+2T2019", published=True
+ )
+ lookup = build_run_lookup(source, [course.id])
+ # archive filenames like 0_501x_2T2019_OLL.tar.gz normalize without the prefix
+ assert "0.501x.2t2019" in lookup
+ assert lookup["0.501x.2t2019"][0].id == run.id
+ assert "mitx.0.501x.2t2019" in lookup
+
+
def test_build_run_lookup_filters_by_ids():
"""build_run_lookup only includes runs for the specified course ids"""
source = ETLSource.mitxonline.name
diff --git a/learning_resources/etl/loaders.py b/learning_resources/etl/loaders.py
index 328f5f13de..fedaf7a3d6 100644
--- a/learning_resources/etl/loaders.py
+++ b/learning_resources/etl/loaders.py
@@ -336,6 +336,13 @@ def load_run(
run_data["published"] = True
with transaction.atomic():
+ previously_published = (
+ LearningResourceRun.objects.filter(
+ learning_resource=learning_resource, run_id=run_id
+ )
+ .values_list("published", flat=True)
+ .first()
+ )
(
learning_resource_run,
_,
@@ -352,42 +359,46 @@ def load_run(
if hasattr(learning_resource, "best_run"):
del learning_resource.best_run
- if (
- (
- learning_resource_run == learning_resource.best_run
- or learning_resource.test_mode
- )
- and learning_resource.etl_source
- in (
- ETLSource.mit_edx.value,
- ETLSource.mitxonline.value,
- ETLSource.xpro.value,
- )
- and learning_resource_run.content_files.count() == 0
- ):
- # webhook may have been sent before run was created or was best run,
- # so trigger a contentfile ingestion for the course. If already
- # ingested & checksums match, no new content files will be created
- from learning_resources.tasks import import_content_files
-
- etl_source = learning_resource.etl_source
- resource_id = learning_resource.id
- cache_key = f"content_tasks_triggered_{etl_source}_{resource_id}"
-
- def enqueue_content_tasks():
- redis_cache = caches["redis"]
- if not redis_cache.add(
- cache_key,
- True, # noqa: FBT003
- timeout=CONTENT_TASKS_CACHE_TIMEOUT,
- ):
- return
- import_content_files.delay(
- etl_source,
- learning_resource_ids=[resource_id],
+ if previously_published and not learning_resource_run.published:
+ resource_run_unpublished_actions(learning_resource_run)
+ elif learning_resource.published or learning_resource.test_mode:
+ if (
+ learning_resource.etl_source
+ in (
+ ETLSource.mit_edx.value,
+ ETLSource.mitxonline.value,
+ ETLSource.xpro.value,
)
+ and learning_resource_run.content_files.count() == 0
+ ):
+ # webhook may have been sent before run was created or was best
+ # run, so trigger a contentfile ingestion for the course. If
+ # already ingested & checksums match, no new files are created
+ from learning_resources.tasks import import_content_files
+
+ etl_source = learning_resource.etl_source
+ resource_id = learning_resource.id
+ cache_key = f"content_tasks_triggered_{etl_source}_{resource_id}"
+
+ def enqueue_content_tasks():
+ redis_cache = caches["redis"]
+ if not redis_cache.add(
+ cache_key,
+ True, # noqa: FBT003
+ timeout=CONTENT_TASKS_CACHE_TIMEOUT,
+ ):
+ return
+ import_content_files.delay(
+ etl_source,
+ learning_resource_ids=[resource_id],
+ )
- transaction.on_commit(enqueue_content_tasks)
+ transaction.on_commit(enqueue_content_tasks)
+ elif previously_published is False:
+ # Run was republished. Its content files are still present and
+ # published (retained sources keep them in Qdrant), just absent
+ # from OpenSearch. Re-index them without a full re-ingest.
+ content_files_loaded_actions(learning_resource_run)
return learning_resource_run
diff --git a/learning_resources/etl/loaders_test.py b/learning_resources/etl/loaders_test.py
index cfade57ed7..c0a195dd43 100644
--- a/learning_resources/etl/loaders_test.py
+++ b/learning_resources/etl/loaders_test.py
@@ -444,6 +444,33 @@ def test_load_run_sets_test_resource_run_to_published(mocker):
assert not result.published
+@pytest.mark.django_db(transaction=True)
+@pytest.mark.parametrize(
+ ("test_mode", "new_published", "expect_unpublish"),
+ [
+ (False, False, True), # published->False fires the unpublish hook
+ (False, True, False), # stays published, no hook
+ (True, False, False), # test_mode forces published=True, never unpublishes
+ ],
+)
+def test_load_run_unpublishes_on_transition(
+ mocker, test_mode, new_published, expect_unpublish
+):
+ """A run flipped published->False via load_run should trigger unpublish actions"""
+ mock_unpublish = mocker.patch(
+ "learning_resources.etl.loaders.resource_run_unpublished_actions",
+ )
+ resource = LearningResourceFactory.create(is_course=True, test_mode=test_mode)
+ run = LearningResourceRunFactory.create(learning_resource=resource, published=True)
+
+ loaders.load_run(resource, {"run_id": run.run_id, "published": new_published})
+
+ if expect_unpublish:
+ mock_unpublish.assert_called_once()
+ else:
+ mock_unpublish.assert_not_called()
+
+
def test_load_program_bad_platform(mocker):
"""A bad platform should log an exception and not create the program"""
mock_log = mocker.patch("learning_resources.etl.loaders.log.exception")
diff --git a/learning_resources_search/indexing_api.py b/learning_resources_search/indexing_api.py
index 48d8ed1b46..6cea4acd45 100644
--- a/learning_resources_search/indexing_api.py
+++ b/learning_resources_search/indexing_api.py
@@ -514,22 +514,23 @@ def deindex_content_files(content_file_ids, learning_resource_id):
)
-def deindex_run_content_files(run_id, unpublished_only):
+def deindex_run_content_files(run_id, unpublished_only, *, keep_published=False):
"""
Deindex a list of content files by run from the index.
- Files are soft-deleted (published=False) and will be physically deleted
- by the cleanup_deleted_content_files task after the retention period.
Args:
run_id(int): Course run id
unpublished_only(bool): if true only deindex files with published=False
-
+ keep_published(bool): if true, deindex all of the run's current content
+ files from OpenSearch without flipping ContentFile.published. Used to
+ remove a run from OpenSearch while keeping it in Qdrant / the REST API.
"""
run = LearningResourceRun.objects.get(id=run_id)
if unpublished_only:
content_files = run.content_files.filter(published=False).all()
else:
- run.content_files.filter(published=True).update(published=False)
+ if not keep_published:
+ run.content_files.filter(published=True).update(published=False)
content_files = run.content_files.all()
if not content_files.exists():
diff --git a/learning_resources_search/indexing_api_test.py b/learning_resources_search/indexing_api_test.py
index 22036698e1..fe7f735568 100644
--- a/learning_resources_search/indexing_api_test.py
+++ b/learning_resources_search/indexing_api_test.py
@@ -545,6 +545,27 @@ def test_deindex_run_content_files(mocker):
assert ContentFile.objects.filter(published=False).count() == 3
+@pytest.mark.django_db
+def test_deindex_run_content_files_keep_published(mocker):
+ """keep_published removes all docs from OpenSearch without flipping published."""
+ mock_deindex_items = mocker.patch(
+ "learning_resources_search.indexing_api.deindex_items"
+ )
+ run = LearningResourceRunFactory.create(published=True)
+ content_files = ContentFileFactory.create_batch(3, run=run, published=True)
+
+ indexing_api.deindex_run_content_files(
+ run.id, unpublished_only=False, keep_published=True
+ )
+
+ # All files were sent for deindexing from OpenSearch...
+ mock_deindex_items.assert_called_once()
+ # ...but none had published flipped.
+ for cf in content_files:
+ cf.refresh_from_db()
+ assert cf.published is True
+
+
def test_deindex_run_content_files_unpublished_only_does_not_hard_delete(mocker):
"""deindex_run_content_files with unpublished_only=True should deindex but not hard-delete"""
mock_deindex = mocker.patch("learning_resources_search.indexing_api.deindex_items")
diff --git a/learning_resources_search/plugins.py b/learning_resources_search/plugins.py
index 34e245592c..0511af2cf9 100644
--- a/learning_resources_search/plugins.py
+++ b/learning_resources_search/plugins.py
@@ -6,6 +6,7 @@
from django.apps import apps
from django.conf import settings as django_settings
+from learning_resources.etl.constants import QDRANT_RETAINED_SOURCES
from learning_resources_search import tasks
from learning_resources_search.api import get_similar_topics_qdrant
from learning_resources_search.constants import (
@@ -162,66 +163,90 @@ def resource_before_delete(self, resource):
# Ensure test mode is false so the resource is removed from the search index
resource.test_mode = False
self.resource_unpublished(resource)
+ # Deletion removes the content files, so retained sources (kept in Qdrant
+ # by resource_run_unpublished) must be purged here too.
+ if (
+ django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS
+ and resource.resource_type == COURSE_TYPE
+ and resource.etl_source in QDRANT_RETAINED_SOURCES
+ ):
+ for run in resource.runs.all():
+ try_with_retry_as_task(
+ chain(vector_tasks.remove_run_content_files.si(run.id))
+ )
@hookimpl
def resource_run_unpublished(self, run):
"""
- Remove a learning resource run's content files from the search index
+ Deindex an unpublished run's content files. Retained sources
+ (edX + Canvas) leave OpenSearch only and stay in Qdrant;
+ all other sources leave both indexes.
Args:
run(LearningResourceRun): The Learning Resource run that was removed
+
"""
- if run.learning_resource.test_mode:
+ resource = run.learning_resource
+ if resource.test_mode:
return
if not run.content_files.exists():
return
- deindex_tasks = [
- tasks.deindex_run_content_files.si(run.id, unpublished_only=False),
- ]
- if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS:
- deindex_tasks.append(vector_tasks.remove_run_content_files.si(run.id))
+
+ if resource.etl_source in QDRANT_RETAINED_SOURCES:
+ deindex_tasks = [
+ tasks.deindex_run_content_files.si(
+ run.id, unpublished_only=False, keep_published=True
+ ),
+ ]
+ else:
+ deindex_tasks = [
+ tasks.deindex_run_content_files.si(run.id, unpublished_only=False),
+ ]
+ if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS:
+ deindex_tasks.append(vector_tasks.remove_run_content_files.si(run.id))
try_with_retry_as_task(chain(*deindex_tasks))
@hookimpl
def resource_run_delete(self, run):
"""
- Remove a learning resource run's content files from the search index
- and then delete the object
+ Remove a learning resource run's content files from BOTH search indexes
+ and then delete the object.
"""
- if not run.learning_resource.test_mode:
- self.resource_run_unpublished(run)
+ deindex_tasks = [
+ tasks.deindex_run_content_files.si(run.id, unpublished_only=False),
+ ]
+ if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS:
+ deindex_tasks.append(vector_tasks.remove_run_content_files.si(run.id))
+ try_with_retry_as_task(chain(*deindex_tasks))
run.delete()
@hookimpl
def content_files_loaded(self, run):
"""
- Upsert a created/modified run's content files
+ Upsert a created/modified run's content files.
+
+ Qdrant: embed every loaded run (all runs of a published/test_mode course)
+ and drop stale files. OpenSearch: index only the best published run, or
+ any published run of a test_mode course.
Args:
run(LearningResourceRun): The LearningResourceRun that was upserted
"""
if not run.content_files.exists():
return
- if run.published:
- index_tasks = []
- if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS:
- index_tasks.append(
- vector_tasks.embed_run_content_files.si(run.id),
- )
- index_tasks.append(
- tasks.index_run_content_files.si(run.id),
- )
+
+ index_tasks = []
+
+ resource = run.learning_resource
+ if resource.published or resource.test_mode:
+ if run.published and (resource.test_mode or resource.best_run == run):
+ index_tasks.append(tasks.index_run_content_files.si(run.id))
+
if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS:
+ index_tasks.append(vector_tasks.embed_run_content_files.si(run.id))
index_tasks.append(
- vector_tasks.remove_unpublished_run_content_files.si(run.id),
+ vector_tasks.remove_unpublished_run_content_files.si(run.id)
)
+
+ if index_tasks:
try_with_retry_as_task(chain(*index_tasks))
- else:
- deindex_tasks = [
- tasks.deindex_run_content_files.si(run.id, unpublished_only=False),
- ]
- if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS:
- deindex_tasks.append(
- vector_tasks.remove_run_content_files.si(run.id),
- )
- try_with_retry_as_task(chain(*deindex_tasks))
diff --git a/learning_resources_search/plugins_test.py b/learning_resources_search/plugins_test.py
index 517614a596..40859f1acc 100644
--- a/learning_resources_search/plugins_test.py
+++ b/learning_resources_search/plugins_test.py
@@ -4,6 +4,7 @@
import pytest
+from learning_resources.etl.constants import ETLSource
from learning_resources.factories import (
ContentFileFactory,
LearningResourceFactory,
@@ -95,7 +96,7 @@ def test_search_index_plugin_resource_unpublished(
):
"""The plugin function should remove a resource from the search index"""
resource = LearningResourceFactory.create(
- resource_type=resource_type, test_mode=test_mode
+ resource_type=resource_type, test_mode=test_mode, published=False
)
if resource_type == COURSE_TYPE and has_content_files:
for run in resource.runs.all():
@@ -110,6 +111,7 @@ def test_search_index_plugin_resource_unpublished(
if resource_type == COURSE_TYPE and has_content_files and not test_mode:
assert unpublish_run_mock.call_count == resource.runs.count()
for run in resource.runs.all():
+ # Default "mock" source is non-retained -> removed from both indexes.
unpublish_run_mock.assert_any_call(run.id, unpublished_only=False)
else:
unpublish_run_mock.assert_not_called()
@@ -145,52 +147,171 @@ def test_search_index_plugin_resource_before_delete(
)
for run in resource.runs.all():
mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_any_call(
- run.id,
- unpublished_only=False,
+ run.id, unpublished_only=False
)
else:
mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_not_called()
+@pytest.mark.django_db
+def test_resource_before_delete_retained_source_purges_qdrant(
+ mock_search_index_helpers, settings
+):
+ """Deleting a retained-source course purges each run's content from Qdrant,
+ since resource_run_unpublished would otherwise keep retained sources there.
+ """
+ settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True
+ resource = LearningResourceFactory.create(
+ resource_type=COURSE_TYPE,
+ etl_source=ETLSource.mitxonline.value,
+ create_runs=False,
+ )
+ runs = LearningResourceRunFactory.create_batch(
+ 2, learning_resource=resource, published=True
+ )
+ for run in runs:
+ ContentFileFactory.create(run=run)
+
+ SearchIndexPlugin().resource_before_delete(resource)
+
+ assert (
+ mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.call_count
+ == len(runs)
+ )
+ for run in runs:
+ mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_any_call(
+ run.id
+ )
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize("course_published", [True, False])
+def test_resource_run_unpublished_retained_source_keeps_qdrant(
+ mock_search_index_helpers, settings, course_published
+):
+ """edX/Canvas: run leaves OpenSearch (keep_published) but ALWAYS stays in
+ Qdrant -- even when the whole course is unpublished.
+ """
+ settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True
+ run = LearningResourceRunFactory.create(
+ published=False,
+ learning_resource__published=course_published,
+ learning_resource__test_mode=False,
+ learning_resource__etl_source=ETLSource.mitxonline.value,
+ )
+ ContentFileFactory.create(run=run)
+
+ SearchIndexPlugin().resource_run_unpublished(run)
+
+ mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_called_once_with(
+ run.id, unpublished_only=False, keep_published=True
+ )
+ mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_not_called()
+
+
+@pytest.mark.django_db
+def test_resource_run_unpublished_non_retained_source_removes_both(
+ mock_search_index_helpers, settings
+):
+ """OCW (non-retained): run is removed from BOTH indexes (current behavior)."""
+ settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True
+ run = LearningResourceRunFactory.create(
+ published=False,
+ learning_resource__published=True,
+ learning_resource__test_mode=False,
+ learning_resource__etl_source=ETLSource.ocw.value,
+ )
+ ContentFileFactory.create(run=run)
+
+ SearchIndexPlugin().resource_run_unpublished(run)
+
+ mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_called_once_with(
+ run.id, unpublished_only=False
+ )
+ mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_called_once_with(
+ run.id
+ )
+
+
@pytest.mark.django_db
@pytest.mark.parametrize("has_content_files", [True, False])
-@pytest.mark.parametrize("test_mode", [True, False])
-def test_search_index_plugin_resource_run_unpublished(
- mock_search_index_helpers, has_content_files, test_mode
+def test_resource_run_unpublished_test_mode_or_empty_noops(
+ mock_search_index_helpers, settings, has_content_files
):
- """The plugin function should remove a run's contenfiles from the search index"""
- run = LearningResourceRunFactory.create(learning_resource__test_mode=test_mode)
+ """test_mode (any source) or a run with no content files -> no-op."""
+ settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True
+ run = LearningResourceRunFactory.create(
+ published=False,
+ learning_resource__published=True,
+ learning_resource__test_mode=True,
+ learning_resource__etl_source=ETLSource.mitxonline.value,
+ )
if has_content_files:
ContentFileFactory.create(run=run)
+
SearchIndexPlugin().resource_run_unpublished(run)
- if has_content_files and not test_mode:
- mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_called_once_with(
- run.id,
- unpublished_only=False,
+
+ mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_not_called()
+ mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_not_called()
+
+
+@pytest.mark.django_db
+def test_resource_unpublished_course_purges_runs_from_qdrant(
+ mock_search_index_helpers, settings
+):
+ """Unpublishing a whole course purges each run's content files from Qdrant."""
+ settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True
+ resource = LearningResourceFactory.create(
+ resource_type=COURSE_TYPE, published=False, test_mode=False, create_runs=False
+ )
+ runs = LearningResourceRunFactory.create_batch(
+ 2, learning_resource=resource, published=False
+ )
+ for run in runs:
+ ContentFileFactory.create(run=run)
+
+ SearchIndexPlugin().resource_unpublished(resource)
+
+ assert (
+ mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.call_count
+ == len(runs)
+ )
+ for run in runs:
+ mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_any_call(
+ run.id
)
- else:
- mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_not_called()
@pytest.mark.django_db
+@pytest.mark.parametrize(
+ "etl_source", [ETLSource.mitxonline.value, ETLSource.ocw.value]
+)
@pytest.mark.parametrize("has_content_files", [True, False])
@pytest.mark.parametrize("test_mode", [True, False])
def test_search_index_plugin_resource_run_delete(
- mock_search_index_helpers, has_content_files, test_mode
+ mock_search_index_helpers, settings, etl_source, has_content_files, test_mode
):
- """The plugin function should remove contenfiles from the index and delete the run"""
- run = LearningResourceRunFactory.create(learning_resource__test_mode=test_mode)
+ """Deleting a run always purges BOTH indexes and deletes the object,
+ regardless of source, test_mode, or whether content files exist.
+ """
+ settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True
+ run = LearningResourceRunFactory.create(
+ learning_resource__published=True,
+ learning_resource__test_mode=test_mode,
+ learning_resource__etl_source=etl_source,
+ )
if has_content_files:
ContentFileFactory.create(run=run)
run_id = run.id
+
SearchIndexPlugin().resource_run_delete(run)
- if has_content_files and not test_mode:
- mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_called_once_with(
- run_id,
- unpublished_only=False,
- )
- else:
- mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_not_called()
+
+ mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_called_once_with(
+ run_id, unpublished_only=False
+ )
+ mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_called_once_with(
+ run_id
+ )
assert LearningResourceRun.objects.filter(id=run_id).exists() is False
@@ -199,7 +320,9 @@ def test_search_index_plugin_content_files_loaded_published_run(
mock_search_index_helpers,
):
"""Published run should index content files and remove unpublished ones."""
- run = LearningResourceRunFactory.create(published=True)
+ run = LearningResourceRunFactory.create(
+ published=True, learning_resource__create_runs=False
+ )
ContentFileFactory.create(run=run)
SearchIndexPlugin().content_files_loaded(run)
@@ -215,7 +338,9 @@ def test_search_index_plugin_content_files_loaded_published_run_with_qdrant(
):
"""Published run should schedule Qdrant embed and unpublished cleanup."""
settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True
- run = LearningResourceRunFactory.create(published=True)
+ run = LearningResourceRunFactory.create(
+ published=True, learning_resource__create_runs=False
+ )
ContentFileFactory.create(run=run)
SearchIndexPlugin().content_files_loaded(run)
@@ -232,23 +357,103 @@ def test_search_index_plugin_content_files_loaded_published_run_with_qdrant(
@pytest.mark.django_db
-def test_search_index_plugin_content_files_loaded_unpublished_run_with_qdrant(
+def test_content_files_loaded_unpublished_run_embeds_qdrant_only(
mock_search_index_helpers, settings
):
- """Unpublished run should deindex and remove all run content files."""
+ """An unpublished run is embedded into Qdrant but NOT indexed into OpenSearch."""
settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True
- run = LearningResourceRunFactory.create(published=False)
+ run = LearningResourceRunFactory.create(
+ published=False, learning_resource__published=True
+ )
ContentFileFactory.create(run=run)
SearchIndexPlugin().content_files_loaded(run)
- mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_called_once_with(
- run.id,
- unpublished_only=False,
+ mock_search_index_helpers.mock_embed_run_contentfiles_immutable_signature.assert_called_once_with(
+ run.id
)
- mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_called_once_with(
+ mock_search_index_helpers.mock_remove_unpublished_run_contentfiles_immutable_signature.assert_called_once_with(
run.id
)
+ # OpenSearch indexing is skipped for a non-published run.
+ mock_search_index_helpers.mock_upsert_contentfiles_immutable_signature.assert_not_called()
+ # The old behavior (deindex / remove from Qdrant) no longer fires here.
+ mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_not_called()
+ mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_not_called()
+
+
+@pytest.mark.django_db
+def test_content_files_loaded_non_best_published_run_skips_opensearch(
+ mock_search_index_helpers, settings
+):
+ """A published-but-not-best run embeds to Qdrant only (no OpenSearch index)."""
+ settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True
+ from datetime import UTC, datetime
+
+ course = LearningResourceFactory.create(published=True, create_runs=False)
+ LearningResourceRunFactory.create(
+ learning_resource=course,
+ published=True,
+ start_date=datetime(2023, 1, 1, tzinfo=UTC),
+ ) # best run
+ non_best = LearningResourceRunFactory.create(
+ learning_resource=course,
+ published=True,
+ start_date=datetime(2022, 1, 1, tzinfo=UTC),
+ )
+ ContentFileFactory.create(run=non_best)
+
+ SearchIndexPlugin().content_files_loaded(non_best)
+
+ mock_search_index_helpers.mock_embed_run_contentfiles_immutable_signature.assert_called_once_with(
+ non_best.id
+ )
+ mock_search_index_helpers.mock_upsert_contentfiles_immutable_signature.assert_not_called()
+
+
+@pytest.mark.django_db
+def test_content_files_loaded_test_mode_published_run_indexes_opensearch(
+ mock_search_index_helpers, settings
+):
+ """Any published run of a test_mode course is indexed into OpenSearch."""
+ settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True
+ course = LearningResourceFactory.create(
+ published=False, test_mode=True, create_runs=False
+ )
+ run = LearningResourceRunFactory.create(learning_resource=course, published=True)
+ ContentFileFactory.create(run=run)
+
+ SearchIndexPlugin().content_files_loaded(run)
+
+ mock_search_index_helpers.mock_upsert_contentfiles_immutable_signature.assert_called_once_with(
+ run.id
+ )
+ mock_search_index_helpers.mock_embed_run_contentfiles_immutable_signature.assert_called_once_with(
+ run.id
+ )
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize("qdrant_enabled", [True, False])
+def test_content_files_loaded_retired_course_does_nothing(
+ mock_search_index_helpers, settings, qdrant_enabled
+):
+ """A fully-retired course (unpublished, non-test_mode) is neither embedded
+ into Qdrant nor indexed into OpenSearch.
+ """
+ settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = qdrant_enabled
+ run = LearningResourceRunFactory.create(
+ published=False,
+ learning_resource__published=False,
+ learning_resource__test_mode=False,
+ )
+ ContentFileFactory.create(run=run)
+
+ SearchIndexPlugin().content_files_loaded(run)
+
+ mock_search_index_helpers.mock_embed_run_contentfiles_immutable_signature.assert_not_called()
+ mock_search_index_helpers.mock_remove_unpublished_run_contentfiles_immutable_signature.assert_not_called()
+ mock_search_index_helpers.mock_upsert_contentfiles_immutable_signature.assert_not_called()
@pytest.mark.django_db
diff --git a/learning_resources_search/tasks.py b/learning_resources_search/tasks.py
index 105ee7aed8..0b3f830d1d 100644
--- a/learning_resources_search/tasks.py
+++ b/learning_resources_search/tasks.py
@@ -529,18 +529,23 @@ def index_run_content_files(run_id, index_types=IndexestoUpdate.all_indexes.valu
retry_backoff=True,
rate_limit=settings.CELERY_SEARCH_RATE_LIMIT,
)
-def deindex_run_content_files(run_id, unpublished_only):
+def deindex_run_content_files(run_id, unpublished_only, keep_published=False): # noqa: FBT002
"""
Deindex content files for a LearningResourceRun
Args:
run_id(int): LearningResourceRun id
unpublished_only(bool): Whether to only deindex unpublished content files
-
+ keep_published(bool): Whether to remove from OpenSearch without flipping
+ ContentFile.published
"""
try:
with wrap_retry_exception(*SEARCH_CONN_EXCEPTIONS):
- api.deindex_run_content_files(run_id, unpublished_only=unpublished_only)
+ api.deindex_run_content_files(
+ run_id,
+ unpublished_only=unpublished_only,
+ keep_published=keep_published,
+ )
except (RetryError, Ignore):
raise
except: # noqa: E722
diff --git a/learning_resources_search/tasks_test.py b/learning_resources_search/tasks_test.py
index ea0278f52b..23980ccc73 100644
--- a/learning_resources_search/tasks_test.py
+++ b/learning_resources_search/tasks_test.py
@@ -733,7 +733,7 @@ def test_delete_run_content_files(mocker, with_error, unpublished_only):
deindex_run_content_files_mock.side_effect = TabError
result = deindex_run_content_files.delay(1, unpublished_only=unpublished_only).get()
deindex_run_content_files_mock.assert_called_once_with(
- 1, unpublished_only=unpublished_only
+ 1, unpublished_only=unpublished_only, keep_published=False
)
assert result == (
diff --git a/main/settings.py b/main/settings.py
index 2e13665c6d..6d177b3d97 100644
--- a/main/settings.py
+++ b/main/settings.py
@@ -35,7 +35,7 @@
from main.settings_pluggy import * # noqa: F403
from openapi.settings_spectacular import open_spectacular_settings
-VERSION = "0.71.12"
+VERSION = "0.71.16"
log = logging.getLogger()
@@ -871,32 +871,6 @@ def get_all_config_keys():
name="CONTENT_FILE_EMBEDDING_CHUNK_OVERLAP",
default=0, # default that the tokenizer uses
)
-CONTENT_FILE_EMBEDDING_SEMANTIC_CHUNKING_ENABLED = get_bool(
- name="CONTENT_FILE_EMBEDDING_SEMANTIC_CHUNKING_ENABLED", default=False
-)
-
-SEMANTIC_CHUNKING_CONFIG = {
- "buffer_size": get_int(
- # Number of sentences to combine.
- name="SEMANTIC_CHUNKING_BUFFER_SIZE",
- default=1,
- ),
- "breakpoint_threshold_type": get_string(
- # 'percentile', 'standard_deviation', 'interquartile', or 'gradient'
- name="SEMANTIC_CHUNKING_BREAKPOINT_THRESHOLD_TYPE",
- default="percentile",
- ),
- "breakpoint_threshold_amount": get_float(
- # value we use for breakpoint_threshold_type to filter outliers
- name="SEMANTIC_CHUNKING_BREAKPOINT_THRESHOLD_AMOUNT",
- default=None,
- ),
- "number_of_chunks": get_int(
- # number of chunks to consider for merging
- name="SEMANTIC_CHUNKING_NUMBER_OF_CHUNKS",
- default=None,
- ),
-}
CONTENT_FILE_SUMMARIZER_BATCH_SIZE = get_int("CONTENT_FILE_SUMMARIZER_BATCH_SIZE", 20)
# number of flashcards to generate
diff --git a/pyproject.toml b/pyproject.toml
index 2702d97df5..bbdc431c71 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -55,7 +55,6 @@ dependencies = [
"isodate>=0.7.2,<0.8",
"jedi>=0.19.0,<0.20",
"langchain>=1.3.9,<1.4",
- "langchain-experimental>=0.4,<0.5",
"litellm==1.84.0",
"llama-index>=0.14.0,<0.15",
"llama-index-llms-openai>=0.6.0,<0.7",
@@ -117,6 +116,8 @@ dependencies = [
"langchain-litellm>=0.5.1",
"scikit-learn>=1.8.0",
"gensim>=4.4.0",
+ "langchain-community>=0.4.2",
+ "langchain-text-splitters>=1.1.2",
]
[dependency-groups]
diff --git a/uv.lock b/uv.lock
index 96de643c6a..1bed38aa9e 100644
--- a/uv.lock
+++ b/uv.lock
@@ -2034,19 +2034,6 @@ wheels = [
{ url = "https://files.pythonhosted.org/packages/de/3e/dcdffa60078ae7b3a00ebb4cbbf1a204a14c3609983c604886523a7d4418/langchain_core-1.4.7-py3-none-any.whl", hash = "sha256:bcadd51951140ecdcba98311dbd931ba5de02a5ba8a2288dad5069c1eea2a13d", size = 554941, upload-time = "2026-06-12T19:23:55.826Z" },
]
-[[package]]
-name = "langchain-experimental"
-version = "0.4.2"
-source = { registry = "https://pypi.org/simple" }
-dependencies = [
- { name = "langchain-community" },
- { name = "langchain-core" },
-]
-sdist = { url = "https://files.pythonhosted.org/packages/d9/1c/8240195c1b27f2c45b2ee93a07b2a5a466c92dec8881fc1213ec8e3cb6a8/langchain_experimental-0.4.2.tar.gz", hash = "sha256:1130fa7ecad5959e687295b2f7850ae3e5d8eeee6796bf9e99910806475d8acc", size = 172068, upload-time = "2026-05-22T20:38:42.184Z" }
-wheels = [
- { url = "https://files.pythonhosted.org/packages/f1/ce/5c56f08407648ab897b119a4add46391560a144285cf89e060091bc793e9/langchain_experimental-0.4.2-py3-none-any.whl", hash = "sha256:fd98b9948cb6f991e0f632ea3d5fb839bcfe21e6d5a190a51126a76270a66d40", size = 211226, upload-time = "2026-05-22T20:38:40.678Z" },
-]
-
[[package]]
name = "langchain-litellm"
version = "0.5.1"
@@ -2604,8 +2591,9 @@ dependencies = [
{ name = "isodate" },
{ name = "jedi" },
{ name = "langchain" },
- { name = "langchain-experimental" },
+ { name = "langchain-community" },
{ name = "langchain-litellm" },
+ { name = "langchain-text-splitters" },
{ name = "litellm" },
{ name = "llama-index" },
{ name = "llama-index-llms-openai" },
@@ -2743,8 +2731,9 @@ requires-dist = [
{ name = "isodate", specifier = ">=0.7.2,<0.8" },
{ name = "jedi", specifier = ">=0.19.0,<0.20" },
{ name = "langchain", specifier = ">=1.3.9,<1.4" },
- { name = "langchain-experimental", specifier = ">=0.4,<0.5" },
+ { name = "langchain-community", specifier = ">=0.4.2" },
{ name = "langchain-litellm", specifier = ">=0.5.1" },
+ { name = "langchain-text-splitters", specifier = ">=1.1.2" },
{ name = "litellm", specifier = "==1.84.0" },
{ name = "llama-index", specifier = ">=0.14.0,<0.15" },
{ name = "llama-index-llms-openai", specifier = ">=0.6.0,<0.7" },
diff --git a/vector_search/conftest.py b/vector_search/conftest.py
index be3651cf1f..98e62d25cc 100644
--- a/vector_search/conftest.py
+++ b/vector_search/conftest.py
@@ -33,13 +33,11 @@ def _use_test_qdrant_settings(settings, mocker):
settings.LITELLM_API_BASE = "https://test/api/"
settings.LITELLM_TOKEN_ENCODING_NAME = None
settings.CONTENT_FILE_EMBEDDING_CHUNK_OVERLAP = 0
- settings.CONTENT_FILE_EMBEDDING_SEMANTIC_CHUNKING_ENABLED = False
settings.QDRANT_SPARSE_MODEL = "sklearn/hashing_vectorizer_sparse_model"
settings.QDRANT_SPARSE_ENCODER = (
"vector_search.encoders.sparse_hash.SparseHashEncoder"
)
mock_qdrant = mocker.patch("qdrant_client.QdrantClient")
- mocker.patch("vector_search.utils.SemanticChunker")
mocker.patch(
"vector_search.utils.compute_optimizer_settings",
return_value={
@@ -57,3 +55,13 @@ def _use_test_qdrant_settings(settings, mocker):
"vector_search.utils.qdrant_client",
return_value=mock_qdrant,
)
+
+
+@pytest.fixture(autouse=True)
+def _reset_qdrant_collections_guard():
+ """Per-process embed guard must not leak across tests."""
+ from vector_search.utils import ensure_qdrant_collections
+
+ ensure_qdrant_collections.cache_clear()
+ yield
+ ensure_qdrant_collections.cache_clear()
diff --git a/vector_search/constants.py b/vector_search/constants.py
index d5aaca7097..2302fae0bf 100644
--- a/vector_search/constants.py
+++ b/vector_search/constants.py
@@ -162,7 +162,7 @@
QDRANT_OPTIMIZER_FLUSH_INTERVAL_XLARGE = 30
# Indexing threshold ratio
-QDRANT_OPTIMIZER_INDEXING_THRESHOLD_RATIO = 0.4
+QDRANT_OPTIMIZER_INDEXING_THRESHOLD_RATIO = 0.8
VECTOR_SEARCH_SCORE_BOOST = {
diff --git a/vector_search/tasks.py b/vector_search/tasks.py
index 88d3fe81ba..015958f02e 100644
--- a/vector_search/tasks.py
+++ b/vector_search/tasks.py
@@ -2,9 +2,12 @@
import logging
import celery
+import grpc
import sentry_sdk
from celery.exceptions import Ignore
+from celery.utils.time import get_exponential_backoff_interval
from django.conf import settings
+from django.core.cache import caches
from django.db.models import Q
from learning_resources.content_summarizer import ContentSummarizer
@@ -53,6 +56,18 @@
log = logging.getLogger(__name__)
+EMBED_FAILURE_TTL = 60 * 60 * 24 # 24h defensive cleanup for the per-run counter
+
+
+def _record_embedding_failure(failure_key: str) -> None:
+ """Bump the per-invocation embedding-failure counter in the shared redis cache."""
+ cache = caches["redis"]
+ key = f"embed_errors:{failure_key}"
+ try:
+ cache.incr(key)
+ except ValueError: # key absent
+ cache.set(key, 1, EMBED_FAILURE_TTL)
+
@app.task
def tune_qdrant_collections():
@@ -72,6 +87,25 @@ def _replace_with_chain(task, task_signatures):
return task.replace(celery.chain(*task_signatures))
+def _replace_with_finalized_chain(
+ task: celery.Task, content_file_ids: list[int], *, overwrite: bool
+) -> None:
+ """
+ Chain of content-file embedding chunks + a finalize tail that fails the parent
+ if any chunk failed. Returns None when there is nothing to embed.
+ """
+ failure_key = task.request.id
+ sigs = [
+ generate_embeddings.si(
+ ids, CONTENT_FILE_TYPE, overwrite=overwrite, failure_key=failure_key
+ )
+ for ids in chunks(content_file_ids, chunk_size=settings.QDRANT_CHUNK_SIZE)
+ ]
+ if not sigs:
+ return None
+ return task.replace(celery.chain(*sigs, finalize_embeddings.si(failure_key)))
+
+
def _queue_program_content_file_embedding_tasks(index_tasks, program_ids, overwrite):
"""Queue content file embedding tasks for programs using a single bulk query."""
if not program_ids:
@@ -96,33 +130,56 @@ def _queue_program_content_file_embedding_tasks(index_tasks, program_ids, overwr
)
+def _retry_countdown(retries: int) -> int:
+ """Full-jitter exponential backoff (mirrors retry_backoff=True), capped at 10m."""
+ return get_exponential_backoff_interval(
+ factor=1, retries=retries, maximum=600, full_jitter=True
+ )
+
+
@app.task(
+ bind=True,
acks_late=True,
reject_on_worker_lost=True,
- autoretry_for=(RetryError,),
- retry_backoff=True,
+ max_retries=3,
rate_limit="200/m",
)
-def generate_embeddings(ids, resource_type, overwrite):
+def generate_embeddings(
+ self,
+ ids: list[int],
+ resource_type: str,
+ overwrite: bool, # noqa: FBT001
+ failure_key: str | None = None,
+) -> None:
"""
- Generate learning resource embeddings and index in Qdrant
-
- Args:
- ids(list of int): List of resource id's
- resource_type (string): resource_type value for the learning resource objects
+ Generate learning resource embeddings and index in Qdrant.
+ Retries transient Qdrant/search errors with jittered backoff. On exhaustion or a
+ non-transient error: if failure_key is set, log + record the failure and return so
+ the chain continues (finalize_embeddings fails the parent); otherwise propagate.
"""
try:
with wrap_retry_exception(*SEARCH_CONN_EXCEPTIONS):
embed_learning_resources(ids, resource_type, overwrite)
- except (RetryError, Ignore):
+ except Ignore:
raise
- except SystemExit as err:
- raise RetryError(SystemExit.__name__) from err
- except: # noqa: E722
- error = "generate_embeddings threw an error"
- log.exception(error)
- return error
+ except SystemExit as err: # worker shutdown: transient; propagate if exhausted
+ if self.request.retries < self.max_retries:
+ raise self.retry(exc=err, countdown=_retry_countdown(self.request.retries)) # noqa: B904
+ raise
+ except Exception as err:
+ is_deadline = (
+ isinstance(err, grpc.RpcError)
+ and err.code() == grpc.StatusCode.DEADLINE_EXCEEDED
+ )
+ if (isinstance(err, RetryError) or is_deadline) and (
+ self.request.retries < self.max_retries
+ ):
+ raise self.retry(exc=err, countdown=_retry_countdown(self.request.retries)) # noqa: B904
+ if failure_key is None:
+ raise # generic callers: propagate terminal failure (current behavior)
+ log.exception("generate_embeddings failed for %s", resource_type)
+ _record_embedding_failure(failure_key)
@app.task(
@@ -148,10 +205,23 @@ def remove_embeddings(ids, resource_type):
raise
except SystemExit as err:
raise RetryError(SystemExit.__name__) from err
- except: # noqa: E722
- error = "generate_embeddings threw an error"
- log.exception(error)
- return error
+ except grpc.RpcError as err:
+ if err.code() == grpc.StatusCode.DEADLINE_EXCEEDED:
+ raise RetryError(str(err)) from err
+ raise
+
+
+@app.task
+def finalize_embeddings(failure_key: str) -> None:
+ """Chain tail: fail the parent task if any chunk recorded a failure."""
+ cache = caches["redis"]
+ key = f"embed_errors:{failure_key}"
+ failures = cache.get(key, 0)
+ cache.delete(key)
+ if failures:
+ msg = f"{failures} embedding chunk(s) failed for {failure_key}"
+ log.error(msg)
+ raise RuntimeError(msg)
@app.task(bind=True)
@@ -194,21 +264,13 @@ def start_embed_resources(self, indexes, skip_content_files, overwrite): # noqa
.exclude(readable_id=blocklisted_ids)
.order_by("id")
):
- run = (
- course.best_run
- if course.best_run
- else course.runs.filter(published=True)
- .order_by("-start_date")
- .first()
- )
+ # Embed published content files across all runs of the course
+ # (Qdrant retains all runs, not just best_run).
contentfiles = (
- ContentFile.objects.filter(
- Q(run=run, published=True, run__published=True)
- | Q(
- learning_resource=course,
- published=True,
- learning_resource__published=True,
- )
+ ContentFile.objects.filter(published=True)
+ .filter(
+ Q(run__learning_resource=course)
+ | Q(learning_resource=course)
)
.order_by("id")
.values_list("id", flat=True)
@@ -306,21 +368,13 @@ def embed_learning_resources_by_id(self, ids, skip_content_files, overwrite):
)
elif not skip_content_files and resource_type == COURSE_TYPE:
for course in embed_resources.order_by("id"):
- run = (
- course.best_run
- if course.best_run
- else course.runs.filter(published=True)
- .order_by("-start_date")
- .first()
- )
+ # Embed published content files across all runs of the course
+ # (Qdrant retains all runs, not just best_run).
content_ids = (
- ContentFile.objects.filter(
- Q(run=run, published=True, run__published=True)
- | Q(
- learning_resource=course,
- published=True,
- learning_resource__published=True,
- )
+ ContentFile.objects.filter(published=True)
+ .filter(
+ Q(run__learning_resource=course)
+ | Q(learning_resource=course)
)
.order_by("id")
.values_list("id", flat=True)
@@ -398,14 +452,11 @@ def embed_new_content_files(self):
.exclude(learning_resource__published=False, learning_resource__test_mode=False)
)
- tasks = [
- generate_embeddings.si(ids, CONTENT_FILE_TYPE, overwrite=False)
- for ids in chunks(
- new_content_files.values_list("id", flat=True),
- chunk_size=settings.QDRANT_CHUNK_SIZE,
- )
- ]
- return _replace_with_chain(self, tasks)
+ return _replace_with_finalized_chain(
+ self,
+ list(new_content_files.values_list("id", flat=True)),
+ overwrite=False,
+ )
@app.task(bind=True)
@@ -417,11 +468,7 @@ def embed_run_content_files(self, run_id):
ContentFile.objects.filter(run__id=run_id).values_list("id", flat=True)
)
- tasks = [
- generate_embeddings.si(ids, CONTENT_FILE_TYPE, overwrite=True)
- for ids in chunks(content_file_ids, chunk_size=settings.QDRANT_CHUNK_SIZE)
- ]
- return _replace_with_chain(self, tasks)
+ return _replace_with_finalized_chain(self, content_file_ids, overwrite=True)
@app.task(bind=True)
@@ -470,35 +517,34 @@ def embeddings_healthcheck():
)
for lr in all_resources:
- run = (
- lr.best_run
- if lr.best_run
- else lr.runs.filter(published=True).order_by("-start_date").first()
- )
serialized = LearningResourceSerializer(lr).data
point_id = vector_point_id(vector_point_key(serialized))
resource_point_ids[point_id] = {"resource_id": lr.readable_id, "id": lr.id}
content_file_point_ids = {}
- if run:
- for cf in run.content_files.for_serialization().filter(published=True):
- if cf and cf.content:
- serialized_cf = ContentFileSerializer(cf).data
- point_id = vector_point_id(
- vector_point_key(
- serialized_cf, chunk_number=0, document_type="content_file"
- )
+ # All runs are embedded in Qdrant, not just best_run.
+ content_files = ContentFile.objects.for_serialization().filter(
+ Q(run__learning_resource=lr) | Q(learning_resource=lr),
+ published=True,
+ )
+ for cf in content_files:
+ if cf and cf.content:
+ serialized_cf = ContentFileSerializer(cf).data
+ point_id = vector_point_id(
+ vector_point_key(
+ serialized_cf, chunk_number=0, document_type="content_file"
)
- content_file_point_ids[point_id] = {"key": cf.key, "id": cf.id}
- for batch in chunks(content_file_point_ids.keys(), chunk_size=200):
- remaining_content_files = filter_existing_qdrant_points_by_ids(
- batch, collection_name=CONTENT_FILES_COLLECTION_NAME
- )
- remaining_content_file_ids.extend(
- [
- content_file_point_ids.get(p, {}).get("id")
- for p in remaining_content_files
- ]
)
+ content_file_point_ids[point_id] = {"key": cf.key, "id": cf.id}
+ for batch in chunks(content_file_point_ids.keys(), chunk_size=200):
+ remaining_content_files = filter_existing_qdrant_points_by_ids(
+ batch, collection_name=CONTENT_FILES_COLLECTION_NAME
+ )
+ remaining_content_file_ids.extend(
+ [
+ content_file_point_ids.get(p, {}).get("id")
+ for p in remaining_content_files
+ ]
+ )
for batch in chunks(
all_resources.values_list("id", flat=True),
diff --git a/vector_search/tasks_test.py b/vector_search/tasks_test.py
index 25d9c520cc..5a5a1c7597 100644
--- a/vector_search/tasks_test.py
+++ b/vector_search/tasks_test.py
@@ -1,8 +1,11 @@
import datetime
import random
+import grpc
import pytest
+from celery.exceptions import Retry
from django.conf import settings
+from django.core.cache.backends.locmem import LocMemCache
from learning_resources.etl.constants import (
RESOURCE_FILE_ETL_SOURCES,
@@ -24,13 +27,18 @@
LEARNING_RESOURCE_TYPES,
PROGRAM_TYPE,
)
+from learning_resources_search.exceptions import RetryError
from main.utils import now_in_utc
from vector_search.tasks import (
+ _record_embedding_failure,
embed_learning_resources_by_id,
embed_new_content_files,
embed_new_learning_resources,
embed_run_content_files,
embeddings_healthcheck,
+ finalize_embeddings,
+ generate_embeddings,
+ remove_embeddings,
remove_run_content_files,
remove_unpublished_run_content_files,
start_embed_resources,
@@ -40,6 +48,22 @@
pytestmark = pytest.mark.django_db
+def _rpc_error(code):
+ """Build a grpc.RpcError carrying a status code, like qdrant's gRPC failures."""
+ err = grpc.RpcError()
+ err.code = lambda: code
+ return err
+
+
+@pytest.fixture
+def embed_cache(mocker):
+ """Real (LocMem) backing store for the redis-alias counter in tasks under test."""
+ cache = LocMemCache("embed-test", {})
+ cache.clear()
+ mocker.patch("vector_search.tasks.caches", {"redis": cache})
+ return cache
+
+
@pytest.mark.parametrize("index", list(LEARNING_RESOURCE_TYPES))
def test_start_embed_resources(mocker, mocked_celery, index):
"""
@@ -211,16 +235,29 @@ def test_embed_new_content_files(mocker, mocked_celery):
generate_embeddings_mock = mocker.patch(
"vector_search.tasks.generate_embeddings", autospec=True
)
+ finalize_embeddings_mock = mocker.patch(
+ "vector_search.tasks.finalize_embeddings", autospec=True
+ )
with pytest.raises(mocked_celery.replace_exception_class):
embed_new_content_files.delay()
embedded_ids = generate_embeddings_mock.si.mock_calls[0].args[0]
assert sorted(new_content_file_ids) == sorted(embedded_ids)
- assert mocked_celery.chain.call_args.args == tuple(
+ assert all(
+ mock_call.kwargs.get("overwrite") is False and "failure_key" in mock_call.kwargs
+ for mock_call in generate_embeddings_mock.si.mock_calls
+ )
+ assert (
+ finalize_embeddings_mock.si.call_args.args[0]
+ == generate_embeddings_mock.si.mock_calls[0].kwargs["failure_key"]
+ )
+ chain_args = mocked_celery.chain.call_args.args
+ assert chain_args[:-1] == tuple(
generate_embeddings_mock.si.return_value
for _ in generate_embeddings_mock.si.mock_calls
)
+ assert chain_args[-1] == finalize_embeddings_mock.si.return_value
def test_remove_run_content_files(mocker, mocked_celery, settings):
@@ -334,32 +371,38 @@ def test_embed_learning_resources_by_id(mocker, mocked_celery):
assert sorted(resource_ids) == sorted(embedded_resource_ids)
-def test_embedded_content_from_best_run(mocker, mocked_celery):
+def _embedded_content_file_ids(generate_embeddings_mock):
+ """Collect all content file ids passed to generate_embeddings across chunks"""
+ return {
+ cid
+ for call in generate_embeddings_mock.si.call_args_list
+ if call.args[1] == "content_file"
+ for cid in call.args[0]
+ }
+
+
+def test_embedded_content_from_all_runs(mocker, mocked_celery):
"""
- Content files to embed should come from best course run
+ Content files from every run of a course should be embedded, not just best_run
"""
mocker.patch("vector_search.tasks.load_course_blocklist", return_value=[])
course = CourseFactory.create(etl_source=ETLSource.ocw.value)
course.runs.all().delete()
- other_run = LearningResourceRunFactory.create(
+ older_run = LearningResourceRunFactory.create(
learning_resource=course.learning_resource,
start_date=datetime.datetime.now(tz=datetime.UTC) - datetime.timedelta(days=2),
)
- LearningResourceRunFactory.create(
+ newer_run = LearningResourceRunFactory.create(
learning_resource=course.learning_resource,
start_date=datetime.datetime.now(tz=datetime.UTC) + datetime.timedelta(days=2),
)
-
- best_run_contentfiles = [
+ all_contentfiles = {
cf.id
- for cf in ContentFileFactory.create_batch(
- 3, run=course.learning_resource.best_run
- )
- ]
- # create contentfiles using the other run
- ContentFileFactory.create_batch(3, run=other_run)
+ for run in (older_run, newer_run)
+ for cf in ContentFileFactory.create_batch(3, run=run)
+ }
generate_embeddings_mock = mocker.patch(
"vector_search.tasks.generate_embeddings", autospec=True
@@ -370,43 +413,46 @@ def test_embedded_content_from_best_run(mocker, mocked_celery):
["course"], skip_content_files=False, overwrite=True
)
- generate_embeddings_mock.si.assert_called_with(
- best_run_contentfiles,
- "content_file",
- True, # noqa: FBT003
- )
+ assert all_contentfiles <= _embedded_content_file_ids(generate_embeddings_mock)
-def test_embedded_content_from_latest_run_if_next_missing(mocker, mocked_celery):
+def test_embed_by_id_all_runs_excludes_unpublished(mocker, mocked_celery):
"""
- Content files to embed should come from latest run if the next run is missing
+ embed_learning_resources_by_id embeds published content files from all runs and
+ excludes unpublished ones
"""
mocker.patch("vector_search.tasks.load_course_blocklist", return_value=[])
course = CourseFactory.create(etl_source=ETLSource.ocw.value)
course.runs.all().delete()
- latest_run = LearningResourceRunFactory.create(
- learning_resource=course.learning_resource,
- start_date=datetime.datetime.now(tz=datetime.UTC) - datetime.timedelta(hours=1),
+ run_a = LearningResourceRunFactory.create(
+ learning_resource=course.learning_resource
)
- latest_run_contentfiles = [
- cf.id for cf in ContentFileFactory.create_batch(3, run=latest_run)
- ]
+ run_b = LearningResourceRunFactory.create(
+ learning_resource=course.learning_resource
+ )
+ published_ids = {
+ cf.id
+ for run in (run_a, run_b)
+ for cf in ContentFileFactory.create_batch(2, run=run, published=True)
+ }
+ unpublished_ids = {
+ cf.id for cf in ContentFileFactory.create_batch(2, run=run_a, published=False)
+ }
+
generate_embeddings_mock = mocker.patch(
"vector_search.tasks.generate_embeddings", autospec=True
)
with pytest.raises(mocked_celery.replace_exception_class):
- start_embed_resources.delay(
- ["course"], skip_content_files=False, overwrite=True
+ embed_learning_resources_by_id.delay(
+ [course.learning_resource.id], skip_content_files=False, overwrite=True
)
- generate_embeddings_mock.si.assert_called_with(
- latest_run_contentfiles,
- "content_file",
- True, # noqa: FBT003
- )
+ embedded = _embedded_content_file_ids(generate_embeddings_mock)
+ assert published_ids <= embedded
+ assert not (unpublished_ids & embedded)
def test_embedded_content_file_without_runs(mocker, mocked_celery):
@@ -618,6 +664,9 @@ def test_embed_run_content_files(mocker, mocked_celery, settings):
generate_embeddings_mock = mocker.patch(
"vector_search.tasks.generate_embeddings", autospec=True
)
+ finalize_embeddings_mock = mocker.patch(
+ "vector_search.tasks.finalize_embeddings", autospec=True
+ )
with pytest.raises(mocked_celery.replace_exception_class):
embed_run_content_files.delay(run.id)
@@ -630,13 +679,21 @@ def test_embed_run_content_files(mocker, mocked_celery, settings):
assert sorted(embedded_ids) == sorted(content_file_ids)
assert all(
mock_call.args[1:] == (CONTENT_FILE_TYPE,)
- and mock_call.kwargs == {"overwrite": True}
+ and mock_call.kwargs["overwrite"] is True
+ and "failure_key" in mock_call.kwargs
for mock_call in generate_embeddings_mock.si.mock_calls
)
- assert mocked_celery.chain.call_args.args == tuple(
+ # chain = all chunk sigs, then the finalize tail
+ chain_args = mocked_celery.chain.call_args.args
+ assert chain_args[:-1] == tuple(
generate_embeddings_mock.si.return_value
for _ in generate_embeddings_mock.si.mock_calls
)
+ assert chain_args[-1] == finalize_embeddings_mock.si.return_value
+ assert (
+ finalize_embeddings_mock.si.call_args.args[0]
+ == generate_embeddings_mock.si.mock_calls[0].kwargs["failure_key"]
+ )
assert mocked_celery.replace.call_count == 1
@@ -656,6 +713,15 @@ def test_embed_run_content_files_no_content_files(mocker, mocked_celery):
mocked_celery.replace.assert_not_called()
+def test_embed_run_content_files_no_files_returns_none(mocker, mocked_celery):
+ """No content files → no chain, no replace, returns None."""
+ run = LearningResourceRunFactory.create() # no content files
+ mocker.patch("vector_search.tasks.generate_embeddings", autospec=True)
+ mocker.patch("vector_search.tasks.finalize_embeddings", autospec=True)
+ assert embed_run_content_files(run.id) is None
+ mocked_celery.chain.assert_not_called()
+
+
def test_embeddings_healthcheck_no_missing_embeddings(mocker):
"""
Test embeddings_healthcheck when there are no missing embeddings
@@ -697,6 +763,36 @@ def test_embeddings_healthcheck_missing_both(mocker):
assert mock_sentry.call_count == 2
+def test_embeddings_healthcheck_checks_all_runs(mocker):
+ """
+ embeddings_healthcheck should check content files from every run, not just best_run
+ """
+ from vector_search.constants import CONTENT_FILES_COLLECTION_NAME
+
+ lr = LearningResourceFactory.create(published=True, create_runs=False)
+ run_a = LearningResourceRunFactory.create(published=True, learning_resource=lr)
+ run_b = LearningResourceRunFactory.create(published=True, learning_resource=lr)
+ ContentFileFactory.create(run=run_a, content="test", published=True)
+ ContentFileFactory.create(run=run_b, content="test", published=True)
+
+ def fake_filter(batch, collection_name=None):
+ # report every content file point as missing, no missing resources
+ return list(batch) if collection_name == CONTENT_FILES_COLLECTION_NAME else []
+
+ mocker.patch(
+ "vector_search.tasks.filter_existing_qdrant_points_by_ids",
+ side_effect=fake_filter,
+ )
+ mock_sentry = mocker.patch("vector_search.tasks.sentry_sdk.capture_message")
+
+ embeddings_healthcheck()
+
+ assert (
+ mock_sentry.mock_calls[0].args[0]
+ == "Warning: 2 missing content file embeddings detected"
+ )
+
+
def test_embeddings_healthcheck_missing_summaries(mocker):
"""
Test embeddings_healthcheck for missing contentfile summaries/flashcards
@@ -741,3 +837,115 @@ def test_embeddings_healthcheck_missing_summaries(mocker):
mock_sentry.mock_calls[0].args[0]
== "Warning: 1 missing content file summaries detected"
)
+
+
+def test_generate_embeddings_retries_on_deadline(mocker):
+ """A deadline with retry budget left calls self.retry (jittered backoff)."""
+ mocker.patch(
+ "vector_search.tasks.embed_learning_resources",
+ side_effect=_rpc_error(grpc.StatusCode.DEADLINE_EXCEEDED),
+ )
+ retry = mocker.patch.object(generate_embeddings, "retry", side_effect=Retry())
+ with pytest.raises(Retry):
+ generate_embeddings([1], CONTENT_FILE_TYPE, overwrite=True, failure_key="k")
+ retry.assert_called_once()
+ assert retry.call_args.kwargs["countdown"] >= 0
+
+
+def test_generate_embeddings_records_on_exhaustion(mocker):
+ """Exhausted deadline + failure_key: record + return, do not raise (chain continues)."""
+ mocker.patch(
+ "vector_search.tasks.embed_learning_resources",
+ side_effect=_rpc_error(grpc.StatusCode.DEADLINE_EXCEEDED),
+ )
+ record = mocker.patch("vector_search.tasks._record_embedding_failure")
+ generate_embeddings.push_request(retries=3)
+ try:
+ assert (
+ generate_embeddings([1], CONTENT_FILE_TYPE, overwrite=True, failure_key="k")
+ is None
+ )
+ finally:
+ generate_embeddings.pop_request()
+ record.assert_called_once_with("k")
+
+
+def test_generate_embeddings_records_non_transient_with_key(mocker):
+ """Non-transient error + failure_key: record + return, do not raise."""
+ mocker.patch(
+ "vector_search.tasks.embed_learning_resources", side_effect=ValueError("boom")
+ )
+ record = mocker.patch("vector_search.tasks._record_embedding_failure")
+ assert (
+ generate_embeddings([1], CONTENT_FILE_TYPE, overwrite=True, failure_key="k")
+ is None
+ )
+ record.assert_called_once_with("k")
+
+
+def test_generate_embeddings_reraises_other_grpc_errors(mocker):
+ """Non-transient gRPC errors propagate (task fails) rather than retrying."""
+ mocker.patch(
+ "vector_search.tasks.embed_learning_resources",
+ side_effect=_rpc_error(grpc.StatusCode.INVALID_ARGUMENT),
+ )
+ with pytest.raises(grpc.RpcError):
+ generate_embeddings([1], COURSE_TYPE, overwrite=False)
+
+
+def test_generate_embeddings_does_not_swallow_errors(mocker):
+ """Unhandled errors propagate so the task fails instead of reporting success."""
+ mocker.patch(
+ "vector_search.tasks.embed_learning_resources",
+ side_effect=ValueError("boom"),
+ )
+ with pytest.raises(ValueError, match="boom"):
+ generate_embeddings([1], COURSE_TYPE, overwrite=False)
+
+
+def test_remove_embeddings_raises_retryerror_on_grpc_deadline(mocker):
+ """remove_embeddings retries on DEADLINE_EXCEEDED rather than swallowing it."""
+ mocker.patch(
+ "vector_search.tasks.remove_qdrant_records",
+ side_effect=_rpc_error(grpc.StatusCode.DEADLINE_EXCEEDED),
+ )
+ with pytest.raises(RetryError):
+ remove_embeddings([1], COURSE_TYPE)
+
+
+def test_remove_embeddings_reraises_other_grpc_errors(mocker):
+ """Non-transient gRPC errors propagate (task fails) rather than retrying."""
+ mocker.patch(
+ "vector_search.tasks.remove_qdrant_records",
+ side_effect=_rpc_error(grpc.StatusCode.INVALID_ARGUMENT),
+ )
+ with pytest.raises(grpc.RpcError):
+ remove_embeddings([1], COURSE_TYPE)
+
+
+def test_remove_embeddings_does_not_swallow_errors(mocker):
+ """Unhandled errors propagate so the task fails instead of reporting success."""
+ mocker.patch(
+ "vector_search.tasks.remove_qdrant_records",
+ side_effect=ValueError("boom"),
+ )
+ with pytest.raises(ValueError, match="boom"):
+ remove_embeddings([1], COURSE_TYPE)
+
+
+def test_record_embedding_failure_increments(embed_cache):
+ _record_embedding_failure("run-1")
+ _record_embedding_failure("run-1")
+ assert embed_cache.get("embed_errors:run-1") == 2
+
+
+def test_finalize_embeddings_raises_and_clears_on_failures(embed_cache):
+ embed_cache.set("embed_errors:run-1", 3)
+ with pytest.raises(RuntimeError, match="3 embedding chunk"):
+ finalize_embeddings("run-1")
+ assert embed_cache.get("embed_errors:run-1") is None
+
+
+def test_finalize_embeddings_succeeds_when_clean(embed_cache):
+ assert finalize_embeddings("run-1") is None
+ assert embed_cache.get("embed_errors:run-1") is None
diff --git a/vector_search/utils.py b/vector_search/utils.py
index 3d2270e36c..b8e42baa1c 100644
--- a/vector_search/utils.py
+++ b/vector_search/utils.py
@@ -5,8 +5,7 @@
from functools import cache
from django.conf import settings
-from django.db.models import Q
-from langchain_experimental.text_splitter import SemanticChunker
+from django.db.models import Prefetch, Q
from langchain_text_splitters import (
MarkdownHeaderTextSplitter,
RecursiveCharacterTextSplitter,
@@ -18,6 +17,7 @@
from learning_resources.models import (
ContentFile,
LearningResource,
+ LearningResourceRun,
LearningResourceTopic,
)
from learning_resources.serializers import (
@@ -184,6 +184,12 @@ def tune_collection(client, collection_name):
)
+@cache
+def ensure_qdrant_collections() -> None:
+ """Ensure Qdrant collections exist, at most once per worker process."""
+ create_qdrant_collections(force_recreate=False)
+
+
def create_qdrant_collections(force_recreate):
"""
Create or recreate QDrant collections
@@ -380,8 +386,8 @@ def _get_text_splitter(**kwargs):
return RecursiveCharacterTextSplitter(**kwargs)
-def _chunk_documents(encoder, texts, metadatas):
- # chunk the documents. use semantic chunking if enabled
+def _chunk_documents(texts, metadatas):
+ # chunk the documents
chunk_params = {
"chunk_overlap": settings.CONTENT_FILE_EMBEDDING_CHUNK_OVERLAP,
}
@@ -390,18 +396,6 @@ def _chunk_documents(encoder, texts, metadatas):
chunk_params["chunk_size"] = settings.CONTENT_FILE_EMBEDDING_CHUNK_SIZE_OVERRIDE
recursive_splitter = _get_text_splitter(**chunk_params)
-
- if settings.CONTENT_FILE_EMBEDDING_SEMANTIC_CHUNKING_ENABLED:
- """
- If semantic chunking is enabled,
- use the semantic chunker then recursive splitter
- to stay within chunk size limits
- """
- return recursive_splitter.split_documents(
- SemanticChunker(
- encoder, **settings.SEMANTIC_CHUNKING_CONFIG
- ).create_documents(texts=texts, metadatas=metadatas)
- )
return recursive_splitter.create_documents(texts=texts, metadatas=metadatas)
@@ -513,6 +507,7 @@ def update_learning_resource_payload(serialized_document):
collection_name=RESOURCES_COLLECTION_NAME,
payload=serialized_document,
points=[point_id],
+ wait=False,
)
@@ -564,6 +559,7 @@ def _set_payload(points, document, param_map, collection_name):
collection_name=collection_name,
payload=payload,
points=point_batch,
+ wait=False,
)
@@ -739,7 +735,7 @@ def _generate_content_file_points(serialized_content):
if _is_markdown_content(doc):
split_docs = _chunk_markdown_documents(embedding_context, doc)
else:
- split_docs = _chunk_documents(encoder_dense, [embedding_context], [doc])
+ split_docs = _chunk_documents([embedding_context], [doc])
# Identify non-empty chunks and their original indices
valid_chunks = [(i, d) for i, d in enumerate(split_docs) if d.page_content]
@@ -810,7 +806,7 @@ def embed_learning_resources(ids, resource_type, overwrite): # noqa: PLR0915, C
return
client = qdrant_client()
- create_qdrant_collections(force_recreate=False)
+ ensure_qdrant_collections()
if resource_type != CONTENT_FILE_TYPE:
serialized_resources = list(serialize_bulk_learning_resources(ids))
points = [
@@ -1138,6 +1134,43 @@ async def _get_facet(agg_key: str):
return dict(results)
+def best_run_ids_for_resources(readable_ids):
+ """
+ Resolve the run_id values a resource_readable_id content-file query should
+ be restricted to.
+
+ Non-test_mode course -> its best run only.
+ test_mode course -> all its published runs (matches OpenSearch indexing).
+ Course with no published run -> contributes nothing.
+
+ Args:
+ readable_ids (list[str]): resource readable_id values from the request
+
+ Returns:
+ list[str]: LearningResourceRun.run_id values to filter on
+ """
+ # Prefetch published runs into _published_runs so both best_run and the
+ # test_mode branch resolve without a per-resource query (avoids an N+1).
+ resources = LearningResource.objects.filter(
+ readable_id__in=readable_ids
+ ).prefetch_related(
+ Prefetch(
+ "runs",
+ queryset=LearningResourceRun.objects.filter(published=True).order_by(
+ "start_date", "enrollment_start", "id"
+ ),
+ to_attr="_published_runs",
+ )
+ )
+ run_ids = []
+ for resource in resources:
+ if resource.test_mode:
+ run_ids.extend(run.run_id for run in resource.published_runs)
+ elif resource.best_run:
+ run_ids.append(resource.best_run.run_id)
+ return run_ids
+
+
def qdrant_query_conditions(params, collection_name=RESOURCES_COLLECTION_NAME):
"""
Return a list of Qdrant FieldCondition objects based on params
@@ -1280,6 +1313,7 @@ def remove_points_matching_params(
points_selector=models.FilterSelector(
filter=qdrant_conditions,
),
+ wait=False,
)
diff --git a/vector_search/utils_test.py b/vector_search/utils_test.py
index c3847de80e..1e8339957d 100644
--- a/vector_search/utils_test.py
+++ b/vector_search/utils_test.py
@@ -11,6 +11,7 @@
from qdrant_client import models
from qdrant_client.models import PointStruct
+import vector_search.utils as vs_utils
from learning_resources.constants import GROUP_CONTENT_FILE_CONTENT_VIEWERS
from learning_resources.factories import (
ContentFileFactory,
@@ -470,32 +471,6 @@ def test_complex_qdrant_query_conditions():
)
-def test_document_chunker(mocker):
- """
- Test that the correct splitter is returned based on encoder
- """
- settings.CONTENT_FILE_EMBEDDING_CHUNK_SIZE_OVERRIDE = None
- settings.CONTENT_FILE_EMBEDDING_SEMANTIC_CHUNKING_ENABLED = True
- settings.LITELLM_TOKEN_ENCODING_NAME = None
- encoder = dense_encoder()
- encoder.token_encoding_name = None
- mocked_splitter = mocker.patch("vector_search.utils.RecursiveCharacterTextSplitter")
- mocked_chunker = mocker.patch("vector_search.utils.SemanticChunker")
- _chunk_documents(encoder, ["this is a test document"], [{}])
-
- mocked_chunker.assert_called()
- mocked_splitter.assert_called()
-
- settings.CONTENT_FILE_EMBEDDING_SEMANTIC_CHUNKING_ENABLED = False
- _get_text_splitter.cache_clear()
- mocked_splitter = mocker.patch("vector_search.utils.RecursiveCharacterTextSplitter")
- mocked_chunker = mocker.patch("vector_search.utils.SemanticChunker")
-
- _chunk_documents(encoder, ["this is a test document"], [{}])
- mocked_chunker.assert_not_called()
- mocked_splitter.assert_called()
-
-
def test_expected_document_chunks(mocker):
"""
Test that the expected number of chunks are uploaded
@@ -505,7 +480,6 @@ def test_expected_document_chunks(mocker):
settings.CONTENT_FILE_EMBEDDING_CHUNK_OVERLAP = random.randrange( # noqa: S311
1, settings.CONTENT_FILE_EMBEDDING_CHUNK_SIZE_OVERRIDE
)
- settings.CONTENT_FILE_EMBEDDING_SEMANTIC_CHUNKING_ENABLED = False
encoder = dense_encoder()
mock_qdrant = mocker.patch("qdrant_client.QdrantClient")
@@ -520,7 +494,6 @@ def test_expected_document_chunks(mocker):
content="this is a. test: document. " * 1000
)
chunked = _chunk_documents(
- encoder,
[content_file.content],
list(serialize_bulk_content_files([content_file.id])),
)
@@ -549,13 +522,13 @@ def test_document_chunker_tiktoken(mocker):
"vector_search.utils.RecursiveCharacterTextSplitter.from_tiktoken_encoder"
)
- _chunk_documents(encoder, ["this is a test document"], [{}])
+ _chunk_documents(["this is a test document"], [{}])
mocked_splitter.assert_not_called()
# work around cache for testing
_get_text_splitter.cache_clear()
settings.LITELLM_TOKEN_ENCODING_NAME = "test" # noqa: S105
- _chunk_documents(encoder, ["this is a test document"], [{}])
+ _chunk_documents(["this is a test document"], [{}])
mocked_splitter.assert_called()
@@ -569,11 +542,11 @@ def test_text_splitter_chunk_size_override(mocker):
encoder = dense_encoder()
mocked_splitter = mocker.patch("vector_search.utils.RecursiveCharacterTextSplitter")
encoder.token_encoding_name = "cl100k_base" # noqa: S105
- _chunk_documents(encoder, ["this is a test document"], [{}])
+ _chunk_documents(["this is a test document"], [{}])
assert mocked_splitter.mock_calls[0].kwargs["chunk_size"] == 100
mocked_splitter = mocker.patch("vector_search.utils.RecursiveCharacterTextSplitter")
settings.CONTENT_FILE_EMBEDDING_CHUNK_SIZE_OVERRIDE = None
- _chunk_documents(encoder, ["this is a test document"], [{}])
+ _chunk_documents(["this is a test document"], [{}])
assert "chunk_size" not in mocked_splitter.mock_calls[0].kwargs
@@ -1984,3 +1957,131 @@ def test_custom_score_formula_defaults(mocker):
assert isinstance(results[0].mult[1], models.Filter)
assert isinstance(results[0].mult[2], models.GaussDecayExpression)
+
+
+@pytest.mark.django_db
+def test_best_run_ids_for_resources_non_test_mode():
+ """A normal course resolves to only its best run's run_id."""
+ from datetime import timedelta
+
+ from django.utils import timezone
+
+ from vector_search.utils import best_run_ids_for_resources
+
+ course = LearningResourceFactory.create(is_course=True, test_mode=False)
+ course.runs.all().delete()
+ LearningResourceRunFactory.create(
+ learning_resource=course,
+ run_id="OLD_RUN",
+ published=True,
+ start_date=timezone.now() - timedelta(days=60),
+ enrollment_start=None,
+ enrollment_end=None,
+ end_date=None,
+ )
+ best = LearningResourceRunFactory.create(
+ learning_resource=course,
+ run_id="NEW_RUN",
+ published=True,
+ start_date=timezone.now() - timedelta(days=10),
+ enrollment_start=None,
+ enrollment_end=None,
+ end_date=None,
+ )
+ # best_run falls back to the latest start_date among published runs
+ assert course.best_run.run_id == best.run_id
+
+ run_ids = best_run_ids_for_resources([course.readable_id])
+
+ assert run_ids == [best.run_id]
+
+
+@pytest.mark.django_db
+def test_best_run_ids_for_resources_test_mode_returns_all_published():
+ """A test_mode course resolves to every published run_id."""
+ from vector_search.utils import best_run_ids_for_resources
+
+ course = LearningResourceFactory.create(is_course=True, test_mode=True)
+ course.runs.all().delete()
+ run_a = LearningResourceRunFactory.create(
+ learning_resource=course, run_id="RUN_A", published=True
+ )
+ run_b = LearningResourceRunFactory.create(
+ learning_resource=course, run_id="RUN_B", published=True
+ )
+ LearningResourceRunFactory.create(
+ learning_resource=course, run_id="RUN_UNPUB", published=False
+ )
+
+ run_ids = best_run_ids_for_resources([course.readable_id])
+
+ assert set(run_ids) == {run_a.run_id, run_b.run_id}
+
+
+@pytest.mark.django_db
+def test_best_run_ids_for_resources_union_across_resources():
+ """Multiple resources yield the union of their resolved run_ids."""
+ from datetime import timedelta
+
+ from django.utils import timezone
+
+ from vector_search.utils import best_run_ids_for_resources
+
+ course1 = LearningResourceFactory.create(is_course=True, test_mode=False)
+ course1.runs.all().delete()
+ best1 = LearningResourceRunFactory.create(
+ learning_resource=course1,
+ run_id="C1_BEST",
+ published=True,
+ start_date=timezone.now() - timedelta(days=10),
+ enrollment_start=None,
+ enrollment_end=None,
+ end_date=None,
+ )
+ course2 = LearningResourceFactory.create(is_course=True, test_mode=False)
+ course2.runs.all().delete()
+ best2 = LearningResourceRunFactory.create(
+ learning_resource=course2,
+ run_id="C2_BEST",
+ published=True,
+ start_date=timezone.now() - timedelta(days=10),
+ enrollment_start=None,
+ enrollment_end=None,
+ end_date=None,
+ )
+
+ run_ids = best_run_ids_for_resources([course1.readable_id, course2.readable_id])
+
+ assert set(run_ids) == {best1.run_id, best2.run_id}
+
+
+@pytest.mark.django_db
+def test_best_run_ids_for_resources_no_published_run():
+ """A course with no published run contributes nothing (no error)."""
+ from vector_search.utils import best_run_ids_for_resources
+
+ course = LearningResourceFactory.create(is_course=True, test_mode=False)
+ course.runs.all().delete()
+ LearningResourceRunFactory.create(
+ learning_resource=course, run_id="UNPUB", published=False
+ )
+
+ assert best_run_ids_for_resources([course.readable_id]) == []
+
+
+def test_ensure_qdrant_collections_runs_once(mocker):
+ create = mocker.patch("vector_search.utils.create_qdrant_collections")
+ vs_utils.ensure_qdrant_collections()
+ vs_utils.ensure_qdrant_collections()
+ create.assert_called_once_with(force_recreate=False)
+
+
+def test_embed_learning_resources_uses_collection_guard(mocker):
+ """embed_learning_resources delegates collection-ensuring to the guard
+ (not a direct create_qdrant_collections call).
+ """
+ ensure = mocker.patch("vector_search.utils.ensure_qdrant_collections")
+ mocker.patch("vector_search.utils.qdrant_client")
+ mocker.patch("vector_search.utils.serialize_bulk_content_files", return_value=[])
+ vs_utils.embed_learning_resources([1], CONTENT_FILE_TYPE, overwrite=True)
+ ensure.assert_called_once()
diff --git a/vector_search/views.py b/vector_search/views.py
index 0be47ae325..84c12fa385 100644
--- a/vector_search/views.py
+++ b/vector_search/views.py
@@ -38,6 +38,7 @@
_resource_vector_hits,
async_qdrant_aggregations,
async_qdrant_client,
+ best_run_ids_for_resources,
custom_score_formula,
dense_encoder,
qdrant_query_conditions,
@@ -689,11 +690,26 @@ async def get(self, request):
f"{settings.QDRANT_BASE_COLLECTION_NAME}.{collection_name_override}"
)
+ params = dict(request_data.data)
+ resource_ids = params.get("resource_readable_id")
+ has_run_filter = "run_readable_id" in params or "edx_module_id" in params
+ if resource_ids and not has_run_filter:
+ # Restrict resource-scoped queries to each resource's best run.
+ # Replace resource_readable_id with a single run_readable_id filter
+ # (don't AND them: compound filters break Qdrant's approximate
+ # count). The resource readable_ids are included to match each
+ # resource's run-less course-metadata point.
+ best_run_ids = await sync_to_async(best_run_ids_for_resources)(
+ resource_ids
+ )
+ del params["resource_readable_id"]
+ params["run_readable_id"] = best_run_ids + list(resource_ids)
+
response = await self.async_vector_search(
query_text,
limit=limit,
offset=offset,
- params=request_data.data,
+ params=params,
search_collection=collection_name,
hybrid_search=hybrid_search,
)
diff --git a/vector_search/views_test.py b/vector_search/views_test.py
index c40b4567f2..e72f4bd235 100644
--- a/vector_search/views_test.py
+++ b/vector_search/views_test.py
@@ -1,13 +1,19 @@
import asyncio
+from datetime import timedelta
import pytest
from django.contrib.auth.models import Group
from django.urls import reverse
+from django.utils import timezone
from qdrant_client import models
from qdrant_client.http.models.models import CountResult
from rest_framework.exceptions import NotAuthenticated, PermissionDenied
from learning_resources.constants import GROUP_CONTENT_FILE_CONTENT_VIEWERS
+from learning_resources.factories import (
+ LearningResourceFactory,
+ LearningResourceRunFactory,
+)
from vector_search.encoders.utils import dense_encoder, sparse_encoder
from vector_search.views import QdrantView
@@ -833,3 +839,200 @@ def test_build_search_params_sort_with_cutoff_score(
assert search_params["query"].order_by.direction == models.Direction.DESC
else:
assert search_params["query"].order_by.direction == models.Direction.ASC
+
+
+@pytest.mark.django_db
+def test_content_file_search_restricts_resource_query_to_best_run(
+ mocker, client, django_user_model
+):
+ """resource_readable_id with no run filter is pinned to the best run."""
+ course = LearningResourceFactory.create(is_course=True, test_mode=False)
+ course.runs.all().delete()
+ # end_date=None is load-bearing: prevents both runs from looking currently-enrollable,
+ # which would cause best_run to pick earliest start_date (OLD_RUN) instead of latest.
+ LearningResourceRunFactory.create(
+ learning_resource=course,
+ run_id="OLD_RUN",
+ published=True,
+ start_date=timezone.now() - timedelta(days=60),
+ enrollment_start=None,
+ enrollment_end=None,
+ end_date=None,
+ )
+ best = LearningResourceRunFactory.create(
+ learning_resource=course,
+ run_id="NEW_RUN",
+ published=True,
+ start_date=timezone.now() - timedelta(days=10),
+ enrollment_start=None,
+ enrollment_end=None,
+ end_date=None,
+ )
+
+ mock_qdrant = mocker.patch(
+ "qdrant_client.AsyncQdrantClient", return_value=mocker.AsyncMock()
+ )()
+ mock_qdrant.scroll = mocker.AsyncMock(return_value=([], None))
+ mock_qdrant.query_points = mocker.AsyncMock()
+ mock_qdrant.query_points_groups = mocker.AsyncMock()
+ mock_qdrant.count = mocker.AsyncMock(return_value=CountResult(count=10))
+ mocker.patch("vector_search.views.async_qdrant_client", return_value=mock_qdrant)
+
+ admin = django_user_model.objects.create_superuser(
+ "admin", "admin@example.com", "pass"
+ )
+ client.force_login(admin)
+
+ client.get(
+ reverse("vector_search:v0:vector_content_files_search"),
+ data={"q": "topics", "resource_readable_id": [course.readable_id]},
+ )
+
+ must = mock_qdrant.query_points.mock_calls[0].kwargs["query_filter"].must
+ # Allowed runs are the best run PLUS the resource readable_id itself (which
+ # matches the run-less course-metadata point).
+ run_conditions = [c for c in must if getattr(c, "key", None) == "run_readable_id"]
+ assert len(run_conditions) == 1
+ assert set(run_conditions[0].match.any) == {best.run_id, course.readable_id}
+ # resource_readable_id is replaced by the run filter, not AND-ed with it
+ # (a single-field filter keeps Qdrant's approximate count accurate).
+ assert not any(getattr(c, "key", None) == "resource_readable_id" for c in must)
+
+
+@pytest.mark.django_db
+def test_content_file_search_test_mode_not_restricted(
+ mocker, client, django_user_model
+):
+ """A test_mode course allows all its published runs."""
+ course = LearningResourceFactory.create(is_course=True, test_mode=True)
+ course.runs.all().delete()
+ run_a = LearningResourceRunFactory.create(
+ learning_resource=course, run_id="RUN_A", published=True
+ )
+ run_b = LearningResourceRunFactory.create(
+ learning_resource=course, run_id="RUN_B", published=True
+ )
+
+ mock_qdrant = mocker.patch(
+ "qdrant_client.AsyncQdrantClient", return_value=mocker.AsyncMock()
+ )()
+ mock_qdrant.scroll = mocker.AsyncMock(return_value=([], None))
+ mock_qdrant.query_points = mocker.AsyncMock()
+ mock_qdrant.query_points_groups = mocker.AsyncMock()
+ mock_qdrant.count = mocker.AsyncMock(return_value=CountResult(count=10))
+ mocker.patch("vector_search.views.async_qdrant_client", return_value=mock_qdrant)
+
+ admin = django_user_model.objects.create_superuser(
+ "admin", "admin@example.com", "pass"
+ )
+ client.force_login(admin)
+
+ client.get(
+ reverse("vector_search:v0:vector_content_files_search"),
+ data={"q": "topics", "resource_readable_id": [course.readable_id]},
+ )
+
+ must = mock_qdrant.query_points.mock_calls[0].kwargs["query_filter"].must
+ run_conditions = [c for c in must if getattr(c, "key", None) == "run_readable_id"]
+ assert len(run_conditions) == 1
+ # All published runs plus the resource readable_id (for the metadata point).
+ assert set(run_conditions[0].match.any) == {
+ run_a.run_id,
+ run_b.run_id,
+ course.readable_id,
+ }
+
+
+@pytest.mark.django_db
+def test_content_file_search_explicit_run_not_overridden(
+ mocker, client, django_user_model
+):
+ """An explicit run_readable_id filter is left untouched."""
+ course = LearningResourceFactory.create(is_course=True, test_mode=False)
+ course.runs.all().delete()
+ LearningResourceRunFactory.create(
+ learning_resource=course, run_id="BEST_RUN", published=True
+ )
+
+ mock_qdrant = mocker.patch(
+ "qdrant_client.AsyncQdrantClient", return_value=mocker.AsyncMock()
+ )()
+ mock_qdrant.scroll = mocker.AsyncMock(return_value=([], None))
+ mock_qdrant.query_points = mocker.AsyncMock()
+ mock_qdrant.query_points_groups = mocker.AsyncMock()
+ mock_qdrant.count = mocker.AsyncMock(return_value=CountResult(count=10))
+ mocker.patch("vector_search.views.async_qdrant_client", return_value=mock_qdrant)
+
+ admin = django_user_model.objects.create_superuser(
+ "admin", "admin@example.com", "pass"
+ )
+ client.force_login(admin)
+
+ client.get(
+ reverse("vector_search:v0:vector_content_files_search"),
+ data={
+ "q": "topics",
+ "resource_readable_id": [course.readable_id],
+ "run_readable_id": ["EXPLICIT_RUN"],
+ },
+ )
+
+ must = mock_qdrant.query_points.mock_calls[0].kwargs["query_filter"].must
+ assert (
+ models.FieldCondition(
+ key="run_readable_id", match=models.MatchAny(any=["EXPLICIT_RUN"])
+ )
+ in must
+ )
+ assert not any(
+ getattr(c, "key", None) == "run_readable_id" and c.match.any == ["BEST_RUN"]
+ for c in must
+ )
+
+
+@pytest.mark.django_db
+def test_content_file_search_no_best_run_metadata_only(
+ mocker, client, django_user_model
+):
+ """A resource with no published run resolves to the resource metadata point only.
+
+ best_run_ids_for_resources returns [], so the allowed run set is just the
+ resource readable_id (which matches the course-metadata point). No real run's
+ content files leak in.
+ """
+ course = LearningResourceFactory.create(is_course=True, test_mode=False)
+ course.runs.all().delete()
+ # Only an unpublished run, so best_run is None and best_run_ids_for_resources -> [].
+ LearningResourceRunFactory.create(
+ learning_resource=course,
+ run_id="UNPUB_RUN",
+ published=False,
+ start_date=timezone.now() - timedelta(days=10),
+ enrollment_start=None,
+ enrollment_end=None,
+ end_date=None,
+ )
+
+ mock_qdrant = mocker.patch(
+ "qdrant_client.AsyncQdrantClient", return_value=mocker.AsyncMock()
+ )()
+ mock_qdrant.scroll = mocker.AsyncMock(return_value=([], None))
+ mock_qdrant.query_points = mocker.AsyncMock()
+ mock_qdrant.query_points_groups = mocker.AsyncMock()
+ mock_qdrant.count = mocker.AsyncMock(return_value=CountResult(count=10))
+ mocker.patch("vector_search.views.async_qdrant_client", return_value=mock_qdrant)
+
+ admin = django_user_model.objects.create_superuser(
+ "admin", "admin@example.com", "pass"
+ )
+ client.force_login(admin)
+
+ client.get(
+ reverse("vector_search:v0:vector_content_files_search"),
+ data={"q": "topics", "resource_readable_id": [course.readable_id]},
+ )
+
+ must = mock_qdrant.query_points.mock_calls[0].kwargs["query_filter"].must
+ run_conditions = [c for c in must if getattr(c, "key", None) == "run_readable_id"]
+ assert len(run_conditions) == 1
+ assert set(run_conditions[0].match.any) == {course.readable_id}