diff --git a/RELEASE.rst b/RELEASE.rst index 0c7f8ec4ab..bb1b7f8cb8 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,15 @@ Release Notes ============= +Version 0.71.14 +--------------- + +- Retry embeddings tasks on Qdrant gRPC DEADLINE_EXCEEDED (#3520) +- chore: Remove product page feature flag (#3501) +- Ingest contentfile archives for all runs (two-tier indexing) (#3503) +- Program certificate title from CMS "Certificate Title" (behind flag) (#3512) +- remove semantic chunker and dependance on langchain-experimental (#3514) + Version 0.71.12 (Released June 23, 2026) --------------- diff --git a/frontends/main/src/app-pages/CertificatePage/CertificatePage.test.tsx b/frontends/main/src/app-pages/CertificatePage/CertificatePage.test.tsx index a5bc1b17a5..8f01cf31a0 100644 --- a/frontends/main/src/app-pages/CertificatePage/CertificatePage.test.tsx +++ b/frontends/main/src/app-pages/CertificatePage/CertificatePage.test.tsx @@ -6,14 +6,23 @@ import CertificatePage from "./CertificatePage" import { CertificateType } from "@/common/certificateUtils" import SharePopover from "@/components/SharePopover/SharePopover" import * as mitxonline from "api/mitxonline-test-utils" +import { useFeatureFlagEnabled } from "posthog-js/react" import { FACEBOOK_SHARE_BASE_URL, TWITTER_SHARE_BASE_URL, LINKEDIN_SHARE_BASE_URL, } from "@/common/urls" +jest.mock("posthog-js/react", () => ({ + ...jest.requireActual("posthog-js/react"), + useFeatureFlagEnabled: jest.fn(), +})) +const mockedUseFeatureFlagEnabled = jest.mocked(useFeatureFlagEnabled) + describe("CertificatePage", () => { beforeEach(() => { + // Default the CMS-title flag ON; individual tests override as needed. + mockedUseFeatureFlagEnabled.mockReturnValue(true) const mitxUser = mitxonline.factories.user.user() setMockResponse.get(mitxonline.urls.userMe.get(), mitxUser) }) @@ -84,9 +93,11 @@ describe("CertificatePage", () => { await screen.findAllByText(certificate.uuid) }) - it("renders a program certificate", async () => { + it("renders a program certificate with the CMS product name as the title", async () => { const certificate = factories.mitxonline.programCertificate() certificate.program.program_type = "Program" + certificate.certificate_page.product_name = + "Custom Program Certificate Title" setMockResponse.get( mitxonline.urls.certificates.programCertificatesRetrieve({ uuid: certificate.uuid, @@ -101,7 +112,7 @@ describe("CertificatePage", () => { />, ) - await screen.findAllByText(certificate.program.title) + await screen.findAllByText("Custom Program Certificate Title") const badge = await screen.findByTestId("certificate-badge-label") expect(within(badge).getByText("Program")).toBeInTheDocument() expect(within(badge).getByText("Certificate")).toBeInTheDocument() @@ -117,6 +128,50 @@ describe("CertificatePage", () => { await screen.findAllByText(certificate.uuid) }) + it("falls back to the program title when the certificate page has no product name", async () => { + const certificate = factories.mitxonline.programCertificate() + certificate.program.program_type = "Program" + certificate.certificate_page.product_name = "" + setMockResponse.get( + mitxonline.urls.certificates.programCertificatesRetrieve({ + uuid: certificate.uuid, + }), + certificate, + ) + renderWithProviders( + , + ) + + await screen.findAllByText(certificate.program.title) + }) + + it("shows the program title (not the CMS title) when the flag is off", async () => { + mockedUseFeatureFlagEnabled.mockReturnValue(false) + const certificate = factories.mitxonline.programCertificate() + certificate.program.program_type = "Program" + certificate.certificate_page.product_name = "Custom CMS Title" + setMockResponse.get( + mitxonline.urls.certificates.programCertificatesRetrieve({ + uuid: certificate.uuid, + }), + certificate, + ) + renderWithProviders( + , + ) + + await screen.findAllByText(certificate.program.title) + expect(screen.queryByText("Custom CMS Title")).not.toBeInTheDocument() + }) + it("renders a MicroMasters program certificate badge with the registered mark", async () => { const certificate = factories.mitxonline.programCertificate() certificate.program.program_type = "MicroMasters®" @@ -166,7 +221,7 @@ describe("CertificatePage", () => { />, ) - await screen.findAllByText(certificate.program.title) + await screen.findAllByText(certificate.user.name!) expect( screen.queryByRole("button", { name: "Download PDF" }), @@ -204,7 +259,7 @@ describe("CertificatePage", () => { />, ) - await screen.findAllByText(certificate.program.title) + await screen.findAllByText(certificate.user.name!) await screen.findByRole("button", { name: "Download PDF" }) await screen.findByRole("button", { name: "Share" }) diff --git a/frontends/main/src/app-pages/CertificatePage/CertificatePage.tsx b/frontends/main/src/app-pages/CertificatePage/CertificatePage.tsx index 8e2c1a626d..d2123a6817 100644 --- a/frontends/main/src/app-pages/CertificatePage/CertificatePage.tsx +++ b/frontends/main/src/app-pages/CertificatePage/CertificatePage.tsx @@ -24,6 +24,7 @@ import SharePopover from "@/components/SharePopover/SharePopover" import { DigitalCredentialDialog } from "./DigitalCredentialDialog" import { getCertificateInfo, + getCertificateTitle, getCertificateBadgeLines, getCertificateBadgeTypography, getVerifiableCredentialLinkedInURL, @@ -31,6 +32,8 @@ import { getVerifiableCredentialDownloadAPIURL, CertificateType, } from "@/common/certificateUtils" +import { useFeatureFlagEnabled } from "posthog-js/react" +import { FeatureFlags } from "@/common/feature_flags" const Page = styled.div(({ theme }) => ({ backgroundImage: `url(${backgroundImage.src})`, @@ -626,11 +629,11 @@ const Certificate = ({ const CourseCertificate = ({ certificate, + title, }: { certificate: V2CourseRunCertificate + title: string }) => { - const title = certificate.course_run.course.title - const userName = certificate.user.name const signatories = certificate.certificate_page.signatory_items @@ -649,11 +652,11 @@ const CourseCertificate = ({ const ProgramCertificate = ({ certificate, + title, }: { certificate: V2ProgramCertificate + title: string }) => { - const title = certificate.program.title - const userName = certificate.user.name const ceus = certificate.certificate_page.CEUs @@ -685,6 +688,11 @@ const CertificatePage: React.FC<{ const { data: userData } = useQuery(mitxUserQueries.me()) + // Gates whether the certificate title comes from the CMS "Certificate Title" + // (product_name) field or the program/course title. Defaults to the + // program/course title until the flag loads / is enabled. + const useCmsTitle = !!useFeatureFlagEnabled(FeatureFlags.CmsCertificateTitle) + const { data: courseCertificateData, isLoading: isCourseLoading, @@ -750,6 +758,18 @@ const CertificatePage: React.FC<{ return notFound() } + let title = "" + if (certificateType === CertificateType.Course) { + title = courseCertificateData?.course_run.course.title ?? "" + } else if (useCmsTitle) { + title = getCertificateTitle( + programCertificateData?.certificate_page?.product_name, + programCertificateData?.program.title ?? "", + ) + } else { + title = programCertificateData?.program.title ?? "" + } + const download = async () => { const res = await fetch(`/certificate/${certificateType}/${uuid}/pdf`) const blob = await res.blob() @@ -763,11 +783,6 @@ const CertificatePage: React.FC<{ window.URL.revokeObjectURL(url) } - const title = - certificateType === CertificateType.Course - ? courseCertificateData?.course_run.course.title - : programCertificateData?.program.title - const { displayType } = getCertificateInfo( programCertificateData?.program?.program_type, ) @@ -849,9 +864,15 @@ const CertificatePage: React.FC<{ ) : null} {certificateType === CertificateType.Course ? ( - + ) : ( - + )} 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..c2b4da2868 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() @@ -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..871f40b970 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.tsx @@ -37,8 +37,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 }) => ({ @@ -284,15 +282,12 @@ const getContextMenuItems = ( 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 +400,6 @@ const ProgramAsCourseCard: React.FC = ({ className, onUpgradeError, }) => { - const useProductPages = useFeatureFlagEnabled( - FeatureFlags.MitxOnlineProductPages, - ) const moduleRequirementSection = courseProgram?.req_tree?.find( (node) => node.data.node_type === "operator", ) @@ -488,7 +480,6 @@ const ProgramAsCourseCard: React.FC = ({ courseProgram, courseProgramEnrollment?.enrollment_mode, contextMenuItems, - useProductPages ?? false, ) const contextMenu = ( 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/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..88d5d70cac 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.14" 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..24ae8df991 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={ diff --git a/vector_search/tasks.py b/vector_search/tasks.py index 88d3fe81ba..8711cd2734 100644 --- a/vector_search/tasks.py +++ b/vector_search/tasks.py @@ -2,6 +2,7 @@ import logging import celery +import grpc import sentry_sdk from celery.exceptions import Ignore from django.conf import settings @@ -119,10 +120,10 @@ def generate_embeddings(ids, resource_type, overwrite): 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( @@ -148,10 +149,10 @@ 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(bind=True) @@ -194,21 +195,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 +299,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) @@ -470,35 +455,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..06c1565e49 100644 --- a/vector_search/tasks_test.py +++ b/vector_search/tasks_test.py @@ -1,6 +1,7 @@ import datetime import random +import grpc import pytest from django.conf import settings @@ -24,6 +25,7 @@ LEARNING_RESOURCE_TYPES, PROGRAM_TYPE, ) +from learning_resources_search.exceptions import RetryError from main.utils import now_in_utc from vector_search.tasks import ( embed_learning_resources_by_id, @@ -31,6 +33,8 @@ embed_new_learning_resources, embed_run_content_files, embeddings_healthcheck, + generate_embeddings, + remove_embeddings, remove_run_content_files, remove_unpublished_run_content_files, start_embed_resources, @@ -40,6 +44,13 @@ 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.mark.parametrize("index", list(LEARNING_RESOURCE_TYPES)) def test_start_embed_resources(mocker, mocked_celery, index): """ @@ -334,32 +345,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 +387,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): @@ -697,6 +717,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 +791,63 @@ 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_raises_retryerror_on_grpc_deadline(mocker): + """A DEADLINE_EXCEEDED gRPC error becomes a RetryError (autoretry_for picks it up).""" + mocker.patch( + "vector_search.tasks.embed_learning_resources", + side_effect=_rpc_error(grpc.StatusCode.DEADLINE_EXCEEDED), + ) + with pytest.raises(RetryError): + generate_embeddings([1], COURSE_TYPE, overwrite=False) + + +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) diff --git a/vector_search/utils.py b/vector_search/utils.py index 3d2270e36c..7f147ea93a 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 ( @@ -380,8 +380,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 +390,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) @@ -739,7 +727,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] @@ -1138,6 +1126,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 diff --git a/vector_search/utils_test.py b/vector_search/utils_test.py index c3847de80e..0eae55645e 100644 --- a/vector_search/utils_test.py +++ b/vector_search/utils_test.py @@ -470,32 +470,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 +479,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 +493,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 +521,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 +541,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 +1956,113 @@ 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]) == [] 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}