diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e442c6c590..827325367f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,6 +63,7 @@ jobs: MITOL_FEATURES_DEFAULT: "True" OPENSEARCH_URL: localhost:9200 CELERY_TASK_ALWAYS_EAGER: "True" + REDIS_URL: redis://localhost:6379/4 CELERY_BROKER_URL: redis://localhost:6379/4 CELERY_RESULT_BACKEND: redis://localhost:6379/4 TIKA_CLIENT_ONLY: "True" diff --git a/RELEASE.rst b/RELEASE.rst index 33a073d860..2476185ce8 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,25 @@ Release Notes ============= +Version 0.76.0 +-------------- + +- Fix topic page pagination (#3682) +- Followup to #3646: skip contentless files in embed pre-pass, retry Qdrant blips, purge before embed (#3684) +- Slim learning resource API payloads and cache rendered JSON (#3675) +- one click upgrade for courses in verified program enrollments (#3671) +- Update dependency social-auth-app-django to v5.6.0 [SECURITY] (#3642) +- fix: Autocomplete chip overflow on learning path form (bump smoot-design) (#3676) +- Set Posthog Cookie on site domain, not mit.edu (#3680) +- fix: Remove course enrollment from cache on unenroll for instant UI update (#3674) +- feat: expose keycloak global_id on the users/me serializer (#3666) +- Delete micromasters resources and remove ETLSource constant (#3670) +- Skip embedding tasks and Qdrant writes for unchanged content files (#3646) +- Fix embeddings healthcheck missing-summaries filter in _missing_summaries (#3686) +- Add acks_late to ETL tasks killed by worker autoscaling scale-downs (#3678) +- fix: Flaky JS test: ProgramAsCoursePage 'Enroll CTA posts program enrollment' times out on Loading button (#3672) +- Switch cache config to use REDIS_URL as intended (#3648) + Version 0.75.5 (Released July 27, 2026) -------------- diff --git a/docker-compose-notebook.yml b/docker-compose-notebook.yml index f3473bad7d..1b7ce3db5b 100644 --- a/docker-compose-notebook.yml +++ b/docker-compose-notebook.yml @@ -10,6 +10,7 @@ x-environment: &py-environment MITOL_FEATURES_DEFAULT: "True" OPENSEARCH_URL: elastic:9200 CELERY_TASK_ALWAYS_EAGER: "False" + REDIS_URL: redis://redis:6379/4 CELERY_BROKER_URL: redis://redis:6379/4 CELERY_RESULT_BACKEND: redis://redis:6379/4 DOCKER_HOST: ${DOCKER_HOST:-missing} diff --git a/drf_lint_baseline.json b/drf_lint_baseline.json index b4f6162d42..0a12f4f9ad 100644 --- a/drf_lint_baseline.json +++ b/drf_lint_baseline.json @@ -4,6 +4,6 @@ "channels/serializers.py:136:24:ORM002", "profiles/serializers.py:136:31:ORM002", "profiles/serializers.py:196:16:ORM002", - "profiles/serializers.py:418:15:ORM001", - "profiles/serializers.py:419:27:ORM001" + "profiles/serializers.py:419:15:ORM001", + "profiles/serializers.py:420:27:ORM001" ] diff --git a/env/backend.env b/env/backend.env index 6009a5ff1d..82f45d76b3 100644 --- a/env/backend.env +++ b/env/backend.env @@ -1,3 +1,4 @@ +REDIS_URL=redis://redis:6379/4 CELERY_BROKER_URL=redis://redis:6379/4 CELERY_RESULT_BACKEND=redis://redis:6379/4 CELERY_TASK_ALWAYS_EAGER=False diff --git a/env/codespaces.env b/env/codespaces.env index ae920de2d0..d0bf428e6d 100644 --- a/env/codespaces.env +++ b/env/codespaces.env @@ -42,6 +42,7 @@ DATABASE_URL=postgres://postgres:postgres@db:5432/postgres MITOL_DB_DISABLE_SSL=True MITOL_FEATURES_DEFAULT=True MITOL_SECURE_SSL_REDIRECT=False +REDIS_URL=redis://redis:6379/4 CELERY_BROKER_URL=redis://redis:6379/4 CELERY_RESULT_BACKEND=redis://redis:6379/4 CELERY_TASK_ALWAYS_EAGER=False diff --git a/frontends/api/src/generated/v0/api.ts b/frontends/api/src/generated/v0/api.ts index 6da0f65103..62a7cd6fcf 100644 --- a/frontends/api/src/generated/v0/api.ts +++ b/frontends/api/src/generated/v0/api.ts @@ -643,10 +643,10 @@ export interface ContentFile { content_feature_type: Array /** * - * @type {ContentTypeEnum} + * @type {ContentFileContentTypeEnum} * @memberof ContentFile */ - content_type?: ContentTypeEnum + content_type?: ContentFileContentTypeEnum /** * * @type {string} @@ -763,6 +763,41 @@ export interface ContentFile { youtube_id?: string | null } +/** + * * `page` - page * `file` - file * `video` - video * `pdf` - pdf + * @export + * @enum {string} + */ + +export const ContentFileContentTypeEnumDescriptions = { + page: "page", + file: "file", + video: "video", + pdf: "pdf", +} as const + +export const ContentFileContentTypeEnum = { + /** + * page + */ + Page: "page", + /** + * file + */ + File: "file", + /** + * video + */ + Video: "video", + /** + * pdf + */ + Pdf: "pdf", +} as const + +export type ContentFileContentTypeEnum = + (typeof ContentFileContentTypeEnum)[keyof typeof ContentFileContentTypeEnum] + /** * SearchResponseSerializer with OpenAPI annotations for Content Files search * @export @@ -842,41 +877,6 @@ export interface ContentFileVectorSearchResponseMetadataAggregationsValueInner { */ doc_count: number } -/** - * * `page` - page * `file` - file * `video` - video * `pdf` - pdf - * @export - * @enum {string} - */ - -export const ContentTypeEnumDescriptions = { - page: "page", - file: "file", - video: "video", - pdf: "pdf", -} as const - -export const ContentTypeEnum = { - /** - * page - */ - Page: "page", - /** - * file - */ - File: "file", - /** - * video - */ - Video: "video", - /** - * pdf - */ - Pdf: "pdf", -} as const - -export type ContentTypeEnum = - (typeof ContentTypeEnum)[keyof typeof ContentTypeEnum] - /** * * @export @@ -1773,10 +1773,10 @@ export interface DocumentResource { resource_type: DocumentResourceResourceTypeEnum /** * - * @type {Array} + * @type {Array} * @memberof DocumentResource */ - content_files: Array | null + content_files: Array | null /** * * @type {string} @@ -3236,6 +3236,212 @@ export interface LearningResourcesVectorSearchResponse { */ metadata: ContentFileVectorSearchResponseMetadata } +/** + * ContentFileSerializer without the large text fields (content, summary, flashcards), for nesting inside learning resource API responses. The search indexing path re-adds full content where needed. + * @export + * @interface NestedContentFile + */ +export interface NestedContentFile { + /** + * + * @type {number} + * @memberof NestedContentFile + */ + id: number + /** + * + * @type {number} + * @memberof NestedContentFile + */ + run_id?: number + /** + * + * @type {number} + * @memberof NestedContentFile + */ + direct_learning_resource_id?: number | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + run_title?: string + /** + * + * @type {string} + * @memberof NestedContentFile + */ + run_slug?: string + /** + * + * @type {Array} + * @memberof NestedContentFile + */ + departments: Array + /** + * + * @type {string} + * @memberof NestedContentFile + */ + semester?: string + /** + * + * @type {number} + * @memberof NestedContentFile + */ + year?: number + /** + * + * @type {Array} + * @memberof NestedContentFile + */ + topics: Array + /** + * + * @type {string} + * @memberof NestedContentFile + */ + key?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + uid?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + title?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + description?: string | null + /** + * + * @type {boolean} + * @memberof NestedContentFile + */ + require_summaries: boolean + /** + * + * @type {string} + * @memberof NestedContentFile + */ + url?: string | null + /** + * + * @type {Array} + * @memberof NestedContentFile + */ + content_feature_type: Array + /** + * + * @type {ContentFileContentTypeEnum} + * @memberof NestedContentFile + */ + content_type?: ContentFileContentTypeEnum + /** + * + * @type {string} + * @memberof NestedContentFile + */ + content_title?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + content_author?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + content_language?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + checksum?: string + /** + * + * @type {string} + * @memberof NestedContentFile + */ + image_src?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + resource_id: string + /** + * + * @type {string} + * @memberof NestedContentFile + */ + resource_readable_id: string + /** + * + * @type {string} + * @memberof NestedContentFile + */ + source_path?: string + /** + * Extract the course number(s) from the associated course + * @type {Array} + * @memberof NestedContentFile + */ + course_number: Array + /** + * + * @type {string} + * @memberof NestedContentFile + */ + file_type?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + file_extension?: string | null + /** + * + * @type {LearningResourceOfferor} + * @memberof NestedContentFile + */ + offered_by: LearningResourceOfferor + /** + * + * @type {LearningResourcePlatform} + * @memberof NestedContentFile + */ + platform: LearningResourcePlatform + /** + * + * @type {string} + * @memberof NestedContentFile + */ + run_readable_id?: string + /** + * + * @type {string} + * @memberof NestedContentFile + */ + edx_module_id?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + youtube_id?: string | null +} + /** * Serializer for News FeedItem * @export @@ -5768,6 +5974,12 @@ export interface User { * @memberof User */ username: string + /** + * + * @type {string} + * @memberof User + */ + global_id: string | null /** * * @type {Profile} @@ -6445,10 +6657,10 @@ export interface VideoResource { playlists: Array /** * - * @type {Array} + * @type {Array} * @memberof VideoResource */ - content_files: Array | null + content_files: Array | null /** * * @type {string} diff --git a/frontends/api/src/generated/v1/api.ts b/frontends/api/src/generated/v1/api.ts index 5d5c0c7582..da3e520776 100644 --- a/frontends/api/src/generated/v1/api.ts +++ b/frontends/api/src/generated/v1/api.ts @@ -1655,10 +1655,10 @@ export interface DocumentResource { resource_type: DocumentResourceResourceTypeEnum /** * - * @type {Array} + * @type {Array} * @memberof DocumentResource */ - content_files: Array | null + content_files: Array | null /** * * @type {string} @@ -5153,6 +5153,212 @@ export interface MicroUserListRelationship { */ child: number } +/** + * ContentFileSerializer without the large text fields (content, summary, flashcards), for nesting inside learning resource API responses. The search indexing path re-adds full content where needed. + * @export + * @interface NestedContentFile + */ +export interface NestedContentFile { + /** + * + * @type {number} + * @memberof NestedContentFile + */ + id: number + /** + * + * @type {number} + * @memberof NestedContentFile + */ + run_id?: number + /** + * + * @type {number} + * @memberof NestedContentFile + */ + direct_learning_resource_id?: number | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + run_title?: string + /** + * + * @type {string} + * @memberof NestedContentFile + */ + run_slug?: string + /** + * + * @type {Array} + * @memberof NestedContentFile + */ + departments: Array + /** + * + * @type {string} + * @memberof NestedContentFile + */ + semester?: string + /** + * + * @type {number} + * @memberof NestedContentFile + */ + year?: number + /** + * + * @type {Array} + * @memberof NestedContentFile + */ + topics: Array + /** + * + * @type {string} + * @memberof NestedContentFile + */ + key?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + uid?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + title?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + description?: string | null + /** + * + * @type {boolean} + * @memberof NestedContentFile + */ + require_summaries: boolean + /** + * + * @type {string} + * @memberof NestedContentFile + */ + url?: string | null + /** + * + * @type {Array} + * @memberof NestedContentFile + */ + content_feature_type: Array + /** + * + * @type {ContentFileContentTypeEnum} + * @memberof NestedContentFile + */ + content_type?: ContentFileContentTypeEnum + /** + * + * @type {string} + * @memberof NestedContentFile + */ + content_title?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + content_author?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + content_language?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + checksum?: string + /** + * + * @type {string} + * @memberof NestedContentFile + */ + image_src?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + resource_id: string + /** + * + * @type {string} + * @memberof NestedContentFile + */ + resource_readable_id: string + /** + * + * @type {string} + * @memberof NestedContentFile + */ + source_path?: string + /** + * Extract the course number(s) from the associated course + * @type {Array} + * @memberof NestedContentFile + */ + course_number: Array + /** + * + * @type {string} + * @memberof NestedContentFile + */ + file_type?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + file_extension?: string | null + /** + * + * @type {LearningResourceOfferor} + * @memberof NestedContentFile + */ + offered_by: LearningResourceOfferor + /** + * + * @type {LearningResourcePlatform} + * @memberof NestedContentFile + */ + platform: LearningResourcePlatform + /** + * + * @type {string} + * @memberof NestedContentFile + */ + run_readable_id?: string + /** + * + * @type {string} + * @memberof NestedContentFile + */ + edx_module_id?: string | null + /** + * + * @type {string} + * @memberof NestedContentFile + */ + youtube_id?: string | null +} + /** * * @export @@ -8740,13 +8946,12 @@ export const SortbyEnum = { export type SortbyEnum = (typeof SortbyEnum)[keyof typeof SortbyEnum] /** - * * `micromasters` - micromasters * `mit_edx` - mit_edx * `mitpe` - mitpe * `mitxonline` - mitxonline * `oll` - oll * `ocw` - ocw * `podcast` - podcast * `mit_climate` - mit_climate * `see` - see * `xpro` - xpro * `youtube` - youtube * `canvas` - canvas * `ovs` - ovs + * * `mit_edx` - mit_edx * `mitpe` - mitpe * `mitxonline` - mitxonline * `oll` - oll * `ocw` - ocw * `podcast` - podcast * `mit_climate` - mit_climate * `see` - see * `xpro` - xpro * `youtube` - youtube * `canvas` - canvas * `ovs` - ovs * @export * @enum {string} */ export const SourceEnumDescriptions = { - micromasters: "micromasters", mit_edx: "mit_edx", mitpe: "mitpe", mitxonline: "mitxonline", @@ -8762,10 +8967,6 @@ export const SourceEnumDescriptions = { } as const export const SourceEnum = { - /** - * micromasters - */ - Micromasters: "micromasters", /** * mit_edx */ @@ -9799,10 +10000,10 @@ export interface VideoResource { playlists: Array /** * - * @type {Array} + * @type {Array} * @memberof VideoResource */ - content_files: Array | null + content_files: Array | null /** * * @type {string} @@ -33384,7 +33585,7 @@ export const WebhooksApiAxiosParamCreator = function ( return { /** * Webhook handler for ContentFile updates - * @param {WebhooksContentFilesCreateSourceEnum} source * `micromasters` - micromasters * `mit_edx` - mit_edx * `mitpe` - mitpe * `mitxonline` - mitxonline * `oll` - oll * `ocw` - ocw * `podcast` - podcast * `mit_climate` - mit_climate * `see` - see * `xpro` - xpro * `youtube` - youtube * `canvas` - canvas * `ovs` - ovs + * @param {WebhooksContentFilesCreateSourceEnum} source * `mit_edx` - mit_edx * `mitpe` - mitpe * `mitxonline` - mitxonline * `oll` - oll * `ocw` - ocw * `podcast` - podcast * `mit_climate` - mit_climate * `see` - see * `xpro` - xpro * `youtube` - youtube * `canvas` - canvas * `ovs` - ovs * @param {ContentFileWebHookRequestRequest} ContentFileWebHookRequestRequest * @param {string} [content_path] * @param {string} [course_id] @@ -33573,7 +33774,7 @@ export const WebhooksApiFp = function (configuration?: Configuration) { return { /** * Webhook handler for ContentFile updates - * @param {WebhooksContentFilesCreateSourceEnum} source * `micromasters` - micromasters * `mit_edx` - mit_edx * `mitpe` - mitpe * `mitxonline` - mitxonline * `oll` - oll * `ocw` - ocw * `podcast` - podcast * `mit_climate` - mit_climate * `see` - see * `xpro` - xpro * `youtube` - youtube * `canvas` - canvas * `ovs` - ovs + * @param {WebhooksContentFilesCreateSourceEnum} source * `mit_edx` - mit_edx * `mitpe` - mitpe * `mitxonline` - mitxonline * `oll` - oll * `ocw` - ocw * `podcast` - podcast * `mit_climate` - mit_climate * `see` - see * `xpro` - xpro * `youtube` - youtube * `canvas` - canvas * `ovs` - ovs * @param {ContentFileWebHookRequestRequest} ContentFileWebHookRequestRequest * @param {string} [content_path] * @param {string} [course_id] @@ -33758,8 +33959,8 @@ export const WebhooksApiFactory = function ( */ export interface WebhooksApiWebhooksContentFilesCreateRequest { /** - * * `micromasters` - micromasters * `mit_edx` - mit_edx * `mitpe` - mitpe * `mitxonline` - mitxonline * `oll` - oll * `ocw` - ocw * `podcast` - podcast * `mit_climate` - mit_climate * `see` - see * `xpro` - xpro * `youtube` - youtube * `canvas` - canvas * `ovs` - ovs - * @type {'micromasters' | 'mit_edx' | 'mitpe' | 'mitxonline' | 'oll' | 'ocw' | 'podcast' | 'mit_climate' | 'see' | 'xpro' | 'youtube' | 'canvas' | 'ovs'} + * * `mit_edx` - mit_edx * `mitpe` - mitpe * `mitxonline` - mitxonline * `oll` - oll * `ocw` - ocw * `podcast` - podcast * `mit_climate` - mit_climate * `see` - see * `xpro` - xpro * `youtube` - youtube * `canvas` - canvas * `ovs` - ovs + * @type {'mit_edx' | 'mitpe' | 'mitxonline' | 'oll' | 'ocw' | 'podcast' | 'mit_climate' | 'see' | 'xpro' | 'youtube' | 'canvas' | 'ovs'} * @memberof WebhooksApiWebhooksContentFilesCreate */ readonly source: WebhooksContentFilesCreateSourceEnum @@ -33894,7 +34095,6 @@ export class WebhooksApi extends BaseAPI { * @export */ export const WebhooksContentFilesCreateSourceEnum = { - Micromasters: "micromasters", MitEdx: "mit_edx", Mitpe: "mitpe", Mitxonline: "mitxonline", diff --git a/frontends/api/src/mitxonline/hooks/enrollment/index.ts b/frontends/api/src/mitxonline/hooks/enrollment/index.ts index 81fa057658..94315f6e87 100644 --- a/frontends/api/src/mitxonline/hooks/enrollment/index.ts +++ b/frontends/api/src/mitxonline/hooks/enrollment/index.ts @@ -64,6 +64,12 @@ const useDestroyEnrollment = () => { return useMutation({ mutationFn: (enrollmentId: number) => courseRunEnrollmentsApi.enrollmentsDestroy({ id: enrollmentId }), + onSuccess: (_data, enrollmentId) => { + queryClient.setQueryData( + enrollmentQueries.courseRunEnrollmentsList().queryKey, + (data) => data?.filter((enrollment) => enrollment.id !== enrollmentId), + ) + }, onSettled: () => { queryClient.invalidateQueries({ queryKey: enrollmentKeys.courseRunEnrollmentsList(), diff --git a/frontends/api/src/test-utils/factories/user.ts b/frontends/api/src/test-utils/factories/user.ts index 795c87bb6e..7e903b1f2b 100644 --- a/frontends/api/src/test-utils/factories/user.ts +++ b/frontends/api/src/test-utils/factories/user.ts @@ -33,6 +33,7 @@ const user: PartialFactory = (overrides = {}): User => { // @ts-expect-error API Response can include anonymous user id: null, username: "", + global_id: null, } } @@ -44,6 +45,7 @@ const user: PartialFactory = (overrides = {}): User => { is_learning_path_editor: false, username: faker.internet.username(), is_authenticated: true, + global_id: faker.string.uuid(), ...overrides, profile: profile(overrides?.profile ?? {}), } diff --git a/frontends/main/package.json b/frontends/main/package.json index 777f94728a..066801811b 100644 --- a/frontends/main/package.json +++ b/frontends/main/package.json @@ -19,7 +19,7 @@ "@mitodl/course-search-utils": "^3.5.2", "@mitodl/hacksnack": "^0.1.0", "@mitodl/mitxonline-api-axios": "2026.7.22", - "@mitodl/smoot-design": "^6.30.0", + "@mitodl/smoot-design": "6.31.1", "@mui/base": "5.0.0-beta.70", "@mui/material": "^6.4.5", "@mui/material-nextjs": "^6.4.3", diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/CoursewareCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/CoursewareCard.tsx index e4e0dbc0e5..8eaf4c6a16 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/CoursewareCard.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/CoursewareCard.tsx @@ -120,6 +120,7 @@ const CoursewareCard: React.FC = (props) => { entry.enrollments, entry.displayedEnrollment, )} + ancestorContext={entry.ancestorContext} layout={layout} headingLevel={headingLevel} onUpgradeError={onUpgradeError} diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardDialogs.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardDialogs.test.tsx index 503b71c864..c46cbb4a4b 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardDialogs.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardDialogs.test.tsx @@ -5,6 +5,7 @@ import { setMockResponse, setupLocationMock, user, + waitFor, within, } from "@/test-utils" import { HomeEnrollmentsDisplay } from "./HomeEnrollmentsDisplay" @@ -155,6 +156,58 @@ describe("DashboardDialogs", () => { }), ) }) + + test("Unenrolling removes the card immediately, before the enrollments list refetches", async () => { + const { enrollments } = setupApis() + const enrollment = faker.helpers.arrayElement(enrollments) + + setMockResponse.delete( + mitxonline.urls.enrollment.courseEnrollment(enrollment.id), + null, + ) + renderWithProviders() + + await screen.findByRole("heading", { name: "My Learning" }) + + const cards = await screen.findAllByTestId("enrollment-card-desktop") + expect(cards.length).toBe(enrollments.length) + + const card = cards.find( + (c) => !!within(c).queryByText(enrollment.run.title), + ) + invariant(card) + + // Hold the post-unenroll invalidation refetch open. If the card only + // disappears once the list refetches, this test fails — proving the card is + // removed by the mutation's immediate cache update, not by the refetch. + const refetch = Promise.withResolvers() + setMockResponse.get( + mitxonline.urls.enrollment.enrollmentsListV3(), + refetch.promise, + ) + + const contextMenuButton = await within(card).findByLabelText("More options") + await user.click(contextMenuButton) + + const unenrollButton = await screen.findByRole("menuitem", { + name: "Unenroll", + }) + await user.click(unenrollButton) + + const confirmButton = await screen.findByRole("button", { + name: "Unenroll", + }) + await user.click(confirmButton) + + // Card is gone even though the refetch is still pending. + await waitFor(() => expect(card).not.toBeInTheDocument()) + expect(screen.getAllByTestId("enrollment-card-desktop")).toHaveLength( + enrollments.length - 1, + ) + + // Let the held refetch settle so nothing dangles after the test. + refetch.resolve(enrollments.filter((e) => e.id !== enrollment.id)) + }) }) describe("UnenrollProgramDialog", () => { diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx index c292894822..0f829c1d10 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx @@ -495,6 +495,173 @@ describe.each([ }) }) + // --------------------------------------------------------------------------- + // Upgrade banner — verified program enrollment (one-click, no checkout) + // --------------------------------------------------------------------------- + + test("Shows 'Upgrade for certificate' without a price when the program enrollment is verified", () => { + setupUserApis() + const price = faker.commerce.price() + const enrollment = mitxonline.factories.enrollment.courseEnrollment({ + enrollment_mode: EnrollmentMode.Audit, + b2b_contract_id: null, + certificate: null, + run: { + is_upgradable: true, + upgrade_deadline: faker.date.future().toISOString(), + upgrade_product_id: faker.number.int(), + upgrade_product_price: price, + upgrade_product_is_active: true, + }, + }) + const programEnrollment = + mitxonline.factories.enrollment.programEnrollmentV3({ + enrollment_mode: "verified", + }) + + renderWithProviders( + , + ) + const banner = within(getCard()).getByTestId("upgrade-root") + expect(banner).toHaveTextContent("Upgrade for certificate") + expect(banner).not.toHaveTextContent(`$${price}`) + }) + + test("Clicking upgrade link one-click enrolls in verified mode and redirects to courseware when program enrollment is verified", async () => { + setupUserApis() + const coursewareUrl = faker.internet.url() + const enrollment = mitxonline.factories.enrollment.courseEnrollment({ + enrollment_mode: EnrollmentMode.Audit, + b2b_contract_id: null, + certificate: null, + run: { + is_upgradable: true, + upgrade_deadline: faker.date.future().toISOString(), + upgrade_product_id: faker.number.int(), + upgrade_product_price: faker.commerce.price(), + upgrade_product_is_active: true, + courseware_url: coursewareUrl, + }, + }) + const programEnrollment = + mitxonline.factories.enrollment.programEnrollmentV3({ + enrollment_mode: "verified", + }) + const programEnrollmentEndpoint = + mitxonline.urls.verifiedProgramEnrollments.create( + enrollment.run.courseware_id, + ) + setMockResponse.post(programEnrollmentEndpoint, {}) + + renderWithProviders( + , + ) + await user.click( + within(getCard()).getByRole("link", { name: "Upgrade for certificate" }), + ) + + await waitFor(() => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "post", + url: programEnrollmentEndpoint, + }), + ) + }) + await waitFor(() => { + expect(window.location.href).toBe(coursewareUrl) + }) + }) + + test("Calls onUpgradeError when verified program enrollment API fails", async () => { + setupUserApis() + const enrollment = mitxonline.factories.enrollment.courseEnrollment({ + enrollment_mode: EnrollmentMode.Audit, + b2b_contract_id: null, + certificate: null, + run: { + is_upgradable: true, + upgrade_deadline: faker.date.future().toISOString(), + upgrade_product_id: faker.number.int(), + upgrade_product_price: faker.commerce.price(), + upgrade_product_is_active: true, + }, + }) + const programEnrollment = + mitxonline.factories.enrollment.programEnrollmentV3({ + enrollment_mode: "verified", + }) + setMockResponse.post( + mitxonline.urls.verifiedProgramEnrollments.create( + enrollment.run.courseware_id, + ), + { error: "Server error" }, + { code: 500 }, + ) + const onUpgradeError = jest.fn() + + renderWithProviders( + , + ) + await user.click( + within(getCard()).getByRole("link", { name: "Upgrade for certificate" }), + ) + await waitFor(() => { + expect(onUpgradeError).toHaveBeenCalled() + }) + }) + + test("Falls back to checkout when the verified program enrollment has no resolvable program identifier", async () => { + const assign = jest.mocked(window.location.assign) + setupUserApis() + const productId = faker.number.int() + const price = faker.commerce.price() + const enrollment = mitxonline.factories.enrollment.courseEnrollment({ + enrollment_mode: EnrollmentMode.Audit, + b2b_contract_id: null, + certificate: null, + run: { + is_upgradable: true, + upgrade_deadline: faker.date.future().toISOString(), + upgrade_product_id: productId, + upgrade_product_price: price, + upgrade_product_is_active: true, + }, + }) + const clearUrl = mitxonline.urls.baskets.clear() + setMockResponse.delete(clearUrl, undefined) + const basketUrl = mitxonline.urls.baskets.createFromProduct(productId) + setMockResponse.post(basketUrl, { id: 1, items: [] }) + + renderWithProviders( + , + ) + const banner = within(getCard()).getByTestId("upgrade-root") + expect(banner).toHaveTextContent(`Upgrade for certificate - $${price}`) + + await user.click( + within(getCard()).getByRole("link", { name: /Upgrade for certificate/ }), + ) + + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ method: "post", url: basketUrl }), + ) + expect(assign).toHaveBeenCalledWith(mitxonlineLegacyUrl("/cart/")) + }) + // --------------------------------------------------------------------------- // Upgraded (paid, certificate not yet earned) banner // --------------------------------------------------------------------------- diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx index 75f6014f09..ef5e89fa63 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx @@ -27,6 +27,7 @@ import { getCourseDateText } from "./courseDateUtils" import { isVerifiedEnrollmentMode } from "@/common/mitxonline" import { RiArrowUpCircleLine, RiAwardLine, RiMore2Line } from "@remixicon/react" import { useReplaceBasketItem } from "@/common/mitxonline/useReplaceBasketItem" +import { useCreateVerifiedProgramEnrollment } from "api/mitxonline-hooks/enrollment" import { isInPast, calendarDaysUntil, NoSSR } from "ol-utilities" import { SiblingRunsPanel, SiblingRunsToggle } from "./SiblingRunsAccordion" import { EnrollmentStatusIcon } from "./EnrollmentStatus" @@ -36,7 +37,10 @@ import { coursePageView } from "@/common/urls" import NiceModal from "@ebay/nice-modal-react" import { EmailSettingsDialog, UnenrollDialog } from "./DashboardDialogs" import { getReceiptMenuItem } from "./receiptMenuItem" -import { CourseRunEnrollmentV3 } from "@mitodl/mitxonline-api-axios/v2" +import { + CourseRunEnrollmentV3, + V3UserProgramEnrollment, +} from "@mitodl/mitxonline-api-axios/v2" import { ProgressBadge } from "./ProgressBadge" const formatUpgradeTime = (daysFloat: number) => { @@ -71,26 +75,70 @@ const UpgradeBanner: React.FC< certificateUpgradeDeadline?: string | null certificateUpgradePrice?: string | null productId?: number | null - onError?: (error: Error) => void + isVerifiedProgramEnrollment?: boolean + readableId?: string + coursewareUrl?: string + programReadableIds?: string[] + programCoursewareId?: string + onUpgradeFailure?: (message: string) => void } & React.HTMLAttributes > = ({ canUpgrade, certificateUpgradeDeadline, certificateUpgradePrice, productId, - onError, + isVerifiedProgramEnrollment, + readableId, + coursewareUrl, + programReadableIds, + programCoursewareId, + onUpgradeFailure, ...others }) => { const replaceBasketItem = useReplaceBasketItem() + const createVerifiedProgramEnrollment = useCreateVerifiedProgramEnrollment() + + const programRequestBody = programReadableIds?.length + ? programReadableIds + : programCoursewareId + ? [programCoursewareId] + : [] + // Mirrors useEnrollmentHandler's gating: without a program identifier the + // verified-program-enrollment endpoint can't resolve which program to + // credit the upgrade against, so fall back to checkout instead of calling + // it with an empty request_body. + const canOneClickUpgrade = Boolean( + isVerifiedProgramEnrollment && readableId && programRequestBody.length > 0, + ) const handleUpgradeClick = async (e: React.MouseEvent) => { e.preventDefault() + + if (canOneClickUpgrade) { + try { + await createVerifiedProgramEnrollment.mutateAsync({ + courserun_id: readableId!, + request_body: programRequestBody, + }) + if (coursewareUrl) { + window.location.href = coursewareUrl + } + } catch { + onUpgradeFailure?.( + "There was a problem upgrading your enrollment. Please try again.", + ) + } + return + } + if (!productId) return try { await replaceBasketItem.mutateAsync(productId) - } catch (error) { - onError?.(error as Error) + } catch { + onUpgradeFailure?.( + "There was a problem adding the certificate to your cart.", + ) } } @@ -103,7 +151,6 @@ const UpgradeBanner: React.FC< return null } - const formattedPrice = `$${certificateUpgradePrice}` const calendarDays = certificateUpgradeDeadline ? calendarDaysUntil(certificateUpgradeDeadline) : null @@ -112,7 +159,9 @@ const UpgradeBanner: React.FC< - {`Upgrade for certificate - ${formattedPrice}`} + {canOneClickUpgrade + ? "Upgrade for certificate" + : `Upgrade for certificate - $${certificateUpgradePrice}`} {calendarDays !== null && ( <> @@ -166,6 +215,11 @@ const MobileAccordionWrapper = styled.div({ type EnrolledCourseCardProps = { enrollment: CourseRunEnrollmentV3 siblingEnrollments?: CourseRunEnrollmentV3[] + ancestorContext?: { + programEnrollment?: V3UserProgramEnrollment + parentProgramReadableIds?: string[] + useVerifiedEnrollment?: boolean + } layout?: "default" | "compact" headingLevel?: "h2" | "h3" | "h4" | "h5" | "h6" onUpgradeError?: (error: string) => void @@ -177,6 +231,7 @@ type EnrolledCourseCardProps = { export const EnrolledCourseCard = ({ enrollment, siblingEnrollments, + ancestorContext, layout = "default", headingLevel, onUpgradeError, @@ -223,6 +278,11 @@ export const EnrolledCourseCard = ({ !!run?.upgrade_product_price && !!run?.upgrade_product_id && !(run?.upgrade_deadline && isInPast(run.upgrade_deadline)) + const isVerifiedProgramEnrollment = + Boolean(ancestorContext?.useVerifiedEnrollment) || + isVerifiedEnrollmentMode( + ancestorContext?.programEnrollment?.enrollment_mode, + ) const enrollmentStatus = getDashboardEnrollmentStatus({ type: DashboardType.CourseRunEnrollment, data: enrollment, @@ -246,11 +306,14 @@ export const EnrolledCourseCard = ({ certificateUpgradeDeadline={run?.upgrade_deadline} certificateUpgradePrice={run?.upgrade_product_price} productId={run?.upgrade_product_id} - onError={() => { - onUpgradeError?.( - "There was a problem adding the certificate to your cart.", - ) - }} + isVerifiedProgramEnrollment={isVerifiedProgramEnrollment} + readableId={run?.courseware_id} + coursewareUrl={coursewareUrl ?? undefined} + programReadableIds={ancestorContext?.parentProgramReadableIds} + programCoursewareId={ + ancestorContext?.programEnrollment?.program.readable_id + } + onUpgradeFailure={onUpgradeError} /> ) : upgradedAndIncomplete ? ( diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.tsx index 72cbcd6180..817b1512b3 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.tsx @@ -333,12 +333,36 @@ const ProgramAsCourseCard: React.FC = ({ completedCount, ) - const parentProgramIds = [ - courseProgram.readable_id, + // Ordered nearest-to-furthest (ancestor last). verified_program_enrollments + // grants the free upgrade based on whichever given program is structurally + // the ancestor, so an unverified ancestor is useless and gets trimmed + // below - but the nearer program's id must stay even when unverified, + // since the backend also uses it to identify which program owns the + // course. See "uses verified enrollment when ancestor has verified mode" + // in ProgramAsCourseCard.test.tsx. + const programChain = [ + { + readableId: courseProgram.readable_id, + enrollmentMode: courseProgramEnrollment?.enrollment_mode, + }, ...(ancestorProgramEnrollment - ? [ancestorProgramEnrollment.readable_id] + ? [ + { + readableId: ancestorProgramEnrollment.readable_id, + enrollmentMode: ancestorProgramEnrollment.enrollment_mode, + }, + ] : []), ] + // Trim an unverified ancestor off the end; never trim the front. + const trimmedProgramChain = [...programChain] + while ( + trimmedProgramChain.length > 0 && + !isVerifiedEnrollmentMode(trimmedProgramChain.at(-1)?.enrollmentMode) + ) { + trimmedProgramChain.pop() + } + const parentProgramIds = trimmedProgramChain.map((p) => p.readableId) const useVerifiedEnrollment = [ courseProgramEnrollment?.enrollment_mode, ancestorProgramEnrollment?.enrollment_mode, diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramEnrollmentDisplay.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramEnrollmentDisplay.test.tsx index 626f861253..b60b41b513 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramEnrollmentDisplay.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramEnrollmentDisplay.test.tsx @@ -1692,6 +1692,318 @@ describe("ProgramEnrollmentDisplay", () => { expect(screen.queryByRole("dialog")).not.toBeInTheDocument() }) + test("Upgrading a module course omits an unverified root program from the request body", async () => { + // Regression test for a nested-program (e.g. SDS-style track) scenario: + // the learner has a verified enrollment in the program-as-course track + // itself but only an audit enrollment in its parent program, and is + // already audit-enrolled in the module course. The verified enrollment + // for the track alone is sufficient to grant the free upgrade, so the + // unverified parent must not be sent - see ProgramAsCourseCard's + // `parentProgramIds` trimming. + const mitxOnlineUser = mitxonline.factories.user.user() + setMockResponse.get(mitxonline.urls.userMe.get(), mitxOnlineUser) + + const moduleRunUpgradeDeadline = faker.date.future().toISOString() + const moduleRunUpgradeProductId = faker.number.int() + const moduleRunUpgradeProductPrice = faker.commerce.price() + const moduleRun = mitxonline.factories.courses.courseRun({ + title: "Module Course", + b2b_contract: null, + is_enrollable: true, + is_upgradable: true, + upgrade_deadline: moduleRunUpgradeDeadline, + courseware_url: faker.internet.url(), + }) + const moduleCourse = mitxonline.factories.courses.course({ + title: "Module Course", + courseruns: [moduleRun], + next_run_id: moduleRun.id, + }) + + const programAsCourseReqTree = + new mitxonline.factories.requirements.RequirementTreeBuilder() + const moduleSection = programAsCourseReqTree.addOperator({ + operator: "all_of", + title: "Modules", + }) + moduleSection.addCourse({ course: moduleCourse.id }) + + const programAsCourse = mitxonline.factories.programs.program({ + display_mode: "course", + courses: [moduleCourse.id], + req_tree: programAsCourseReqTree.serialize(), + }) + + const parentReqTree = + new mitxonline.factories.requirements.RequirementTreeBuilder() + const parentRequirements = parentReqTree.addOperator({ + operator: "all_of", + title: "Program Requirements", + }) + parentRequirements.addProgram({ program: programAsCourse.id }) + + const parentProgram = mitxonline.factories.programs.program({ + req_tree: parentReqTree.serialize(), + }) + + // Root/ancestor program enrollment is audit - not verified. + const parentProgramEnrollment = + mitxonline.factories.enrollment.programEnrollmentV3({ + enrollment_mode: "audit", + program: { + id: parentProgram.id, + title: parentProgram.title, + live: parentProgram.live, + program_type: parentProgram.program_type, + readable_id: parentProgram.readable_id, + }, + }) + // The nested track itself is verified. + const programAsCourseEnrollment = + mitxonline.factories.enrollment.programEnrollmentV3({ + enrollment_mode: "verified", + program: { + id: programAsCourse.id, + title: programAsCourse.title, + live: programAsCourse.live, + program_type: programAsCourse.program_type, + readable_id: programAsCourse.readable_id, + }, + }) + + // Learner already has an audit enrollment in the module course run. + const moduleCourseEnrollment = + mitxonline.factories.enrollment.courseEnrollment({ + enrollment_mode: "audit", + certificate: null, + run: { + id: moduleRun.id, + course_id: moduleCourse.id, + courseware_id: moduleRun.courseware_id, + courseware_url: moduleRun.courseware_url, + is_upgradable: true, + upgrade_deadline: moduleRunUpgradeDeadline, + upgrade_product_id: moduleRunUpgradeProductId, + upgrade_product_price: moduleRunUpgradeProductPrice, + upgrade_product_is_active: true, + course: { + id: moduleCourse.id, + title: moduleCourse.title, + readable_id: moduleCourse.readable_id, + }, + }, + }) + + mockedUseFeatureFlagEnabled.mockReturnValue(true) + setMockResponse.get(mitxonline.urls.enrollment.enrollmentsListV3(), [ + moduleCourseEnrollment, + ]) + setMockResponse.get( + mitxonline.urls.programEnrollments.enrollmentsListV3(), + [parentProgramEnrollment, programAsCourseEnrollment], + ) + setMockResponse.get( + mitxonline.urls.programs.programDetail(parentProgram.id), + parentProgram, + ) + setMockResponse.get( + mitxonline.urls.courses.coursesList({ + id: parentProgram.courses, + page_size: parentProgram.courses.length || undefined, + }), + { count: 0, next: null, previous: null, results: [] }, + ) + setMockResponse.get( + mitxonline.urls.programs.programsList({ + id: [programAsCourse.id], + page_size: 1, + }), + { + count: 1, + next: null, + previous: null, + results: [programAsCourse], + }, + ) + setMockResponse.get( + mitxonline.urls.courses.coursesList({ + id: [moduleCourse.id], + page_size: 1, + }), + { count: 1, next: null, previous: null, results: [moduleCourse] }, + ) + const moduleCourseEnrollmentEndpoint = + mitxonline.urls.verifiedProgramEnrollments.create(moduleRun.courseware_id) + setMockResponse.post(moduleCourseEnrollmentEndpoint, {}) + + renderWithProviders( + , + ) + + await screen.findByText("Program Requirements") + await waitFor( + () => { + const skeletons = screen.queryAllByTestId("skeleton") + expect(skeletons).toHaveLength(0) + }, + { timeout: 3000 }, + ) + + const cards = screen.getAllByTestId("enrollment-card-desktop") + const card = cards.find((c) => within(c).queryByText(moduleCourse.title)) + invariant( + card, + `Expected to find a card containing "${moduleCourse.title}"`, + ) + + const upgradeLink = within(card).getByRole("link", { + name: "Upgrade for certificate", + }) + await user.click(upgradeLink) + + await waitFor(() => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "post", + url: moduleCourseEnrollmentEndpoint, + body: [programAsCourse.readable_id], + }), + ) + }) + }) + + test("Shows an error banner when the one-click verified-program upgrade fails", async () => { + const mitxOnlineUser = mitxonline.factories.user.user() + setMockResponse.get(mitxonline.urls.userMe.get(), mitxOnlineUser) + + const courseRunUpgradeDeadline = faker.date.future().toISOString() + const courseRunUpgradeProductId = faker.number.int() + const courseRunUpgradeProductPrice = faker.commerce.price() + const courseRun = mitxonline.factories.courses.courseRun({ + title: "Verified Track Course", + b2b_contract: null, + is_enrollable: true, + is_upgradable: true, + upgrade_deadline: courseRunUpgradeDeadline, + courseware_url: faker.internet.url(), + }) + const course = mitxonline.factories.courses.course({ + title: "Verified Track Course", + courseruns: [courseRun], + next_run_id: courseRun.id, + }) + + const reqTree = + new mitxonline.factories.requirements.RequirementTreeBuilder() + const requirements = reqTree.addOperator({ + operator: "all_of", + title: "Program Requirements", + }) + requirements.addCourse({ course: course.id }) + + const program = mitxonline.factories.programs.program({ + courses: [course.id], + req_tree: reqTree.serialize(), + }) + const programEnrollment = + mitxonline.factories.enrollment.programEnrollmentV3({ + enrollment_mode: "verified", + program: { + id: program.id, + title: program.title, + live: program.live, + program_type: program.program_type, + readable_id: program.readable_id, + }, + }) + + // Learner already has an audit enrollment in the course run. + const courseEnrollment = mitxonline.factories.enrollment.courseEnrollment({ + enrollment_mode: "audit", + certificate: null, + run: { + id: courseRun.id, + course_id: course.id, + courseware_id: courseRun.courseware_id, + courseware_url: courseRun.courseware_url, + is_upgradable: true, + upgrade_deadline: courseRunUpgradeDeadline, + upgrade_product_id: courseRunUpgradeProductId, + upgrade_product_price: courseRunUpgradeProductPrice, + upgrade_product_is_active: true, + course: { + id: course.id, + title: course.title, + readable_id: course.readable_id, + }, + }, + }) + + mockedUseFeatureFlagEnabled.mockReturnValue(true) + setMockResponse.get(mitxonline.urls.enrollment.enrollmentsListV3(), [ + courseEnrollment, + ]) + setMockResponse.get( + mitxonline.urls.programEnrollments.enrollmentsListV3(), + [programEnrollment], + ) + setMockResponse.get( + mitxonline.urls.programs.programDetail(program.id), + program, + ) + setMockResponse.get( + mitxonline.urls.courses.coursesList({ + id: program.courses, + page_size: program.courses.length, + }), + { count: 1, next: null, previous: null, results: [course] }, + ) + + const enrollmentEndpoint = + mitxonline.urls.verifiedProgramEnrollments.create(courseRun.courseware_id) + setMockResponse.post( + enrollmentEndpoint, + { error: "No verified enrollment in root program" }, + { code: 400 }, + ) + + renderWithProviders() + + await screen.findByText("Program Requirements") + await waitFor( + () => { + const skeletons = screen.queryAllByTestId("skeleton") + expect(skeletons).toHaveLength(0) + }, + { timeout: 3000 }, + ) + + const cards = screen.getAllByTestId("enrollment-card-desktop") + const card = cards.find((c) => within(c).queryByText(course.title)) + invariant(card, `Expected to find a card containing "${course.title}"`) + + const upgradeLink = within(card).getByRole("link", { + name: "Upgrade for certificate", + }) + await user.click(upgradeLink) + + await waitFor(() => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "post", + url: enrollmentEndpoint, + }), + ) + }) + + expect( + await screen.findByText(/problem upgrading your enrollment/i), + ).toBeInTheDocument() + expect( + screen.getByRole("link", { name: "Contact Support" }), + ).toBeInTheDocument() + }) + test("Displays courses in the order defined by the requirement tree, not API order", async () => { const mitxOnlineUser = mitxonline.factories.user.user() setMockResponse.get(mitxonline.urls.userMe.get(), mitxOnlineUser) diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramEnrollmentDisplay.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramEnrollmentDisplay.tsx index 59aff54415..2037fe82b3 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramEnrollmentDisplay.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramEnrollmentDisplay.tsx @@ -1,6 +1,6 @@ import React from "react" -import { Skeleton, Stack, Typography, styled, theme } from "ol-components" -import { ButtonLink } from "@mitodl/smoot-design" +import { Link, Skeleton, Stack, Typography, styled, theme } from "ol-components" +import { Alert, ButtonLink } from "@mitodl/smoot-design" import { DisplayModeEnum } from "@mitodl/mitxonline-api-axios/v2" import { ResourceType, getKey } from "./model/dashboardViewModel" import { CoursewareCard } from "./CoursewareCard" @@ -8,12 +8,19 @@ import NotFoundPage from "@/app-pages/ErrorPage/NotFoundPage" import { ProgramAsCourseCard } from "./ProgramAsCourseCard" import { RiAwardFill } from "@remixicon/react" import { useProgramDashboardData } from "./hooks/useProgramDashboardData" +import { env } from "@/env" const CourseEntryCardStyled = styled(CoursewareCard)({ borderRadius: "8px", boxShadow: "0px 1px 6px 0px rgba(3, 21, 45, 0.05)", }) +const AlertBanner = styled(Alert)({ + marginBottom: "16px", +}) + +const SUPPORT_EMAIL = env("NEXT_PUBLIC_MITOL_SUPPORT_EMAIL") || "" + export const ProgramCertificateButton = styled(ButtonLink)(({ theme }) => ({ color: theme.custom.colors.red, width: "120px", @@ -30,6 +37,7 @@ const ProgramEnrollmentDisplay: React.FC = ({ programId, }) => { const data = useProgramDashboardData(programId) + const [upgradeError, setUpgradeError] = React.useState(null) if (data.isLoading) { return ( @@ -52,6 +60,19 @@ const ProgramEnrollmentDisplay: React.FC = ({ } return ( + {upgradeError && ( + setUpgradeError(null)} + > + {upgradeError}{" "} + + Contact Support + {" "} + for assistance. + + )} = ({ })} kind="course" entry={item.entry} + onUpgradeError={setUpgradeError} /> ) } @@ -146,6 +168,7 @@ const ProgramEnrollmentDisplay: React.FC = ({ moduleEnrollmentsByCourseId={data.enrollmentsByCourseId} courseProgramEnrollment={item.courseProgramEnrollment} ancestorProgramEnrollment={data.ancestorProgramEnrollment} + onUpgradeError={setUpgradeError} /> ) } @@ -164,6 +187,7 @@ const ProgramEnrollmentDisplay: React.FC = ({ moduleEnrollmentsByCourseId={data.enrollmentsByCourseId} courseProgramEnrollment={item.programEnrollment} ancestorProgramEnrollment={data.ancestorProgramEnrollment} + onUpgradeError={setUpgradeError} /> ) })} diff --git a/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.test.tsx b/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.test.tsx index 11332bf84b..e4ebe0b85b 100644 --- a/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.test.tsx +++ b/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.test.tsx @@ -44,6 +44,10 @@ const makeProgramAsCourse: typeof factories.programs.program = ( }) const makePage = factories.pages.programPageItem +// Enrollment status can be slow to resolve, so wait longer for the enroll +// button to leave its loading state before asserting on it. +const ENROLL_STATUS_TIMEOUT = 5000 + const setupApis = ({ program, page, @@ -164,9 +168,11 @@ describe("ProgramAsCoursePage", () => { renderWithProviders( , ) - const buttons = await screen.findAllByRole("button", { - name: "Start Learning", - }) + const buttons = await screen.findAllByRole( + "button", + { name: "Start Learning" }, + { timeout: ENROLL_STATUS_TIMEOUT }, + ) expect(buttons.length).toBeGreaterThanOrEqual(1) }) @@ -423,9 +429,11 @@ describe("ProgramAsCoursePage", () => { , ) - const [enrollButton] = await screen.findAllByRole("button", { - name: "Start Learning", - }) + const [enrollButton] = await screen.findAllByRole( + "button", + { name: "Start Learning" }, + { timeout: ENROLL_STATUS_TIMEOUT }, + ) await user.click(enrollButton) await waitFor(() => { @@ -487,9 +495,11 @@ describe("ProgramAsCoursePage", () => { expect( screen.queryByText("Earn a verified certificate of completion"), ).not.toBeInTheDocument() - const enrollButtons = await screen.findAllByRole("button", { - name: "Start Learning", - }) + const enrollButtons = await screen.findAllByRole( + "button", + { name: "Start Learning" }, + { timeout: ENROLL_STATUS_TIMEOUT }, + ) expect(enrollButtons.length).toBeGreaterThanOrEqual(1) }) diff --git a/frontends/main/src/page-components/ConfiguredPostHogProvider/ConfiguredPostHogProvider.tsx b/frontends/main/src/page-components/ConfiguredPostHogProvider/ConfiguredPostHogProvider.tsx index 6c31af20fc..a866ce989c 100644 --- a/frontends/main/src/page-components/ConfiguredPostHogProvider/ConfiguredPostHogProvider.tsx +++ b/frontends/main/src/page-components/ConfiguredPostHogProvider/ConfiguredPostHogProvider.tsx @@ -68,6 +68,10 @@ const ConfiguredPostHogProvider: React.FC<{ children: React.ReactNode }> = ({ posthog.init(POSTHOG_API_KEY, { api_host: POSTHOG_API_HOST, ui_host: POSTHOG_UI_HOST, + // Scope the posthog cookie to this site's exact domain. Posthog + // defaults this to true, which sets the cookie on the root domain + // (e.g. mit.edu), sharing it with every other site there. + cross_subdomain_cookie: false, bootstrap: { featureFlags: featureFlags ? { diff --git a/frontends/ol-components/package.json b/frontends/ol-components/package.json index 1d553bfbe8..8b2517ad5b 100644 --- a/frontends/ol-components/package.json +++ b/frontends/ol-components/package.json @@ -67,7 +67,7 @@ "typescript": "^5.5.4" }, "peerDependencies": { - "@mitodl/smoot-design": "^6.30.0", + "@mitodl/smoot-design": "6.31.1", "next": "^16.2.7" } } diff --git a/learning_resources/constants.py b/learning_resources/constants.py index 11b2551410..404ffe8ed3 100644 --- a/learning_resources/constants.py +++ b/learning_resources/constants.py @@ -230,6 +230,10 @@ class LearningResourceRelationTypes(TextChoices): zip(VALID_COURSE_CONTENT_TYPES, VALID_COURSE_CONTENT_TYPES) ) +# Large text fields excluded from API responses (both nested serializers and +# OpenSearch _source filtering) but retained in the search index for querying +CONTENT_FILE_LARGE_FIELDS = ("content", "summary", "flashcards") + TUTOR_PROBLEM_TYPE = "problem" TUTOR_SOLUTION_TYPE = "solution" VALID_TUTOR_PROBLEM_TYPES = [TUTOR_PROBLEM_TYPE, TUTOR_SOLUTION_TYPE] diff --git a/learning_resources/etl/constants.py b/learning_resources/etl/constants.py index 7f6351e55e..01deeb992d 100644 --- a/learning_resources/etl/constants.py +++ b/learning_resources/etl/constants.py @@ -80,7 +80,6 @@ class ETLSource(ExtendedEnum): """Enum of ETL sources""" - micromasters = "micromasters" mit_edx = "mit_edx" mitpe = "mitpe" mitxonline = "mitxonline" diff --git a/learning_resources/hooks.py b/learning_resources/hooks.py index 36eb397094..e174bcf89b 100644 --- a/learning_resources/hooks.py +++ b/learning_resources/hooks.py @@ -82,7 +82,12 @@ def offeror_delete(self, offeror): @hookspec def content_files_loaded(self, run): - """Trigger actions after content files are loaded for a run""" + """ + Trigger actions after content files are loaded for a run. + + Args: + run: the LearningResourceRun whose content files were loaded + """ def get_plugin_manager(): diff --git a/learning_resources/migrations/0052_learningresource_certification_type.py b/learning_resources/migrations/0052_learningresource_certification_type.py index 1fbdc01200..c82b04561f 100644 --- a/learning_resources/migrations/0052_learningresource_certification_type.py +++ b/learning_resources/migrations/0052_learningresource_certification_type.py @@ -3,7 +3,6 @@ from django.db import migrations, models from learning_resources.constants import CertificationType -from learning_resources.etl.constants import ETLSource def add_certification_type(apps, schema_editor): @@ -12,10 +11,10 @@ def add_certification_type(apps, schema_editor): certification_type=CertificationType.professional.name ) LearningResource.objects.filter(certification=True, professional=False).exclude( - etl_source=ETLSource.micromasters.name + etl_source="micromasters" ).update(certification_type=CertificationType.completion.name) LearningResource.objects.filter( - certification=True, etl_source=ETLSource.micromasters.name + certification=True, etl_source="micromasters" ).update(certification_type=CertificationType.micromasters.name) diff --git a/learning_resources/migrations/0118_delete_micromasters_resources.py b/learning_resources/migrations/0118_delete_micromasters_resources.py new file mode 100644 index 0000000000..4473f24509 --- /dev/null +++ b/learning_resources/migrations/0118_delete_micromasters_resources.py @@ -0,0 +1,17 @@ +from django.db import migrations + + +def delete_micromasters_resources(apps, schema_editor): + """Delete all micromasters resources (already deindexed by 0117)""" + LearningResource = apps.get_model("learning_resources", "LearningResource") + LearningResource.objects.filter(etl_source="micromasters").delete() + + +class Migration(migrations.Migration): + dependencies = [ + ("learning_resources", "0117_unpublish_micromasters_resources"), + ] + + operations = [ + migrations.RunPython(delete_micromasters_resources, migrations.RunPython.noop), + ] diff --git a/learning_resources/serializers.py b/learning_resources/serializers.py index 5cf648eca8..c2b40c7df6 100644 --- a/learning_resources/serializers.py +++ b/learning_resources/serializers.py @@ -1501,6 +1501,21 @@ class Meta: ] +class NestedContentFileSerializer(ContentFileSerializer): + """ + ContentFileSerializer without the large text fields (content, summary, + flashcards), for nesting inside learning resource API responses. + The search indexing path re-adds full content where needed. + """ + + class Meta(ContentFileSerializer.Meta): + fields = [ + field + for field in ContentFileSerializer.Meta.fields + if field not in constants.CONTENT_FILE_LARGE_FIELDS + ] + + class VideoResourceSerializer(LearningResourceBaseSerializer): """Serializer for video resources""" @@ -1515,11 +1530,11 @@ class VideoResourceSerializer(LearningResourceBaseSerializer): content_files = serializers.SerializerMethodField() description = serializers.SerializerMethodField() - @extend_schema_field(ContentFileSerializer(many=True, allow_null=True)) + @extend_schema_field(NestedContentFileSerializer(many=True, allow_null=True)) def get_content_files(self, instance): """Serialize content files with prefetch.""" content_files = instance.direct_content_files_for_serialization() - return ContentFileSerializer( + return NestedContentFileSerializer( content_files, many=True, read_only=True, @@ -1547,11 +1562,11 @@ class DocumentResourceSerializer(LearningResourceBaseSerializer): content_files = serializers.SerializerMethodField() description = serializers.SerializerMethodField() - @extend_schema_field(ContentFileSerializer(many=True, allow_null=True)) + @extend_schema_field(NestedContentFileSerializer(many=True, allow_null=True)) def get_content_files(self, instance): """Serialize content files with prefetch.""" content_files = instance.direct_content_files_for_serialization() - return ContentFileSerializer( + return NestedContentFileSerializer( content_files, many=True, read_only=True, diff --git a/learning_resources/serializers_test.py b/learning_resources/serializers_test.py index 051d8acbaa..98441468fe 100644 --- a/learning_resources/serializers_test.py +++ b/learning_resources/serializers_test.py @@ -212,10 +212,35 @@ def test_serialize_video_resource_with_content_files(): assert len(serializer.data["content_files"]) == 1 assert serializer.data["content_files"][0]["id"] == content_file.id assert serializer.data["content_files"][0]["title"] == "Video Content File" + # Full text fields are excluded from nested API responses + for field in ("content", "summary", "flashcards"): + assert field not in serializer.data["content_files"][0] # Description should fall back to content file's description when resource has none assert serializer.data["description"] == "Content file description" +def test_serialize_document_resource_with_content_files(): + """ + Verify that DocumentResourceSerializer serializes content files without + the full text fields + """ + document_resource = factories.LearningResourceFactory.create( + resource_type=LearningResourceType.document.name, + ) + content_file = factories.ContentFileFactory.create( + run=None, + direct_learning_resource=document_resource, + title="Document Content File", + ) + resource = LearningResource.objects.for_serialization().get(pk=document_resource.pk) + serializer = serializers.DocumentResourceSerializer(instance=resource) + + assert len(serializer.data["content_files"]) == 1 + assert serializer.data["content_files"][0]["id"] == content_file.id + for field in ("content", "summary", "flashcards"): + assert field not in serializer.data["content_files"][0] + + def test_serialize_podcast_episode_playlists_to_json(): """ Verify that a serialized podcast episode resource has the correct podcast data diff --git a/learning_resources/tasks.py b/learning_resources/tasks.py index 1706049147..3aa9477fd6 100644 --- a/learning_resources/tasks.py +++ b/learning_resources/tasks.py @@ -65,7 +65,7 @@ CLEANUP_RETRY_EXCEPTIONS = (*SEARCH_CONN_EXCEPTIONS, OperationalError) -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) def update_next_start_date_and_prices(): """Update expired next start dates and prices""" resources = LearningResource.objects.filter(next_start_date__lt=timezone.now()) @@ -79,7 +79,7 @@ def update_next_start_date_and_prices(): return len(resources) -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) @cooldown_task( wait_time=3600, key_func=lambda *, api_course_datafile=None, api_program_datafile=None: ( @@ -108,7 +108,7 @@ def get_mit_edx_data( return len(courses) + len(programs) -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) @cooldown_task(wait_time=900) def get_mitxonline_data() -> int | None: """Execute the MITX Online ETL pipeline""" @@ -136,21 +136,21 @@ def get_oll_data(sheets_id=None) -> int | None: return len(courses) -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) def get_mitpe_data(): """Execute the Professional Education ETL pipeline""" courses, programs = pipelines.mitpe_etl() return len(courses) + len(programs) -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) def get_sloan_data(): """Execute the Sloan ETL pipelines""" courses = pipelines.sloan_courses_etl() return len(courses) -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) @cooldown_task(wait_time=900) def get_xpro_data() -> int | None: """Execute the xPro ETL pipeline""" @@ -160,7 +160,7 @@ def get_xpro_data() -> int | None: return len(courses) + len(programs) -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) def get_mit_climate_data(): """Execute the MIT Climate ETL pipeline""" articles = pipelines.mit_climate_etl() @@ -168,7 +168,7 @@ def get_mit_climate_data(): return len(articles) -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) def get_content_files( ids: list[int], etl_source: str, @@ -317,7 +317,7 @@ def import_content_files( ) -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) def get_podcast_data(): """ Execute the Podcast ETL pipeline @@ -530,7 +530,7 @@ def get_ovs_transcripts(*, overwrite=False): clear_views_cache() -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) def get_learning_resource_views(): """Load learning resource views from the PostHog ETL.""" diff --git a/learning_resources/utils.py b/learning_resources/utils.py index 01bc3947a7..3f0cd4752c 100644 --- a/learning_resources/utils.py +++ b/learning_resources/utils.py @@ -437,7 +437,10 @@ def bulk_resources_unpublished_actions(resource_ids: list[int], resource_type: s def content_files_loaded_actions(run: LearningResourceRun): """ - Trigger plugins when content files are loaded for a LearningResourceRun + Trigger plugins when content files are loaded for a LearningResourceRun. + + Args: + run: the LearningResourceRun whose content files were loaded """ pm = get_plugin_manager() hook = pm.hook diff --git a/learning_resources_search/api_test.py b/learning_resources_search/api_test.py index 242d2d4c8d..85d977586d 100644 --- a/learning_resources_search/api_test.py +++ b/learning_resources_search/api_test.py @@ -2422,6 +2422,9 @@ def test_execute_learn_search_for_learning_resource_query(settings, opensearch): "flashcards", "vector_embedding", "video.transcript", + "content_files.content", + "content_files.summary", + "content_files.flashcards", ] }, } @@ -2521,6 +2524,9 @@ def test_execute_learn_search_for_learning_resource_query_filter_ocw_files( "flashcards", "vector_embedding", "video.transcript", + "content_files.content", + "content_files.summary", + "content_files.flashcards", ] }, } @@ -3068,6 +3074,9 @@ def test_execute_learn_search_with_script_score( "flashcards", "vector_embedding", "video.transcript", + "content_files.content", + "content_files.summary", + "content_files.flashcards", ] }, } @@ -3574,6 +3583,9 @@ def test_execute_learn_search_with_hybrid_search(mocker, settings, opensearch): "flashcards", "vector_embedding", "video.transcript", + "content_files.content", + "content_files.summary", + "content_files.flashcards", ] }, } @@ -4065,6 +4077,9 @@ def test_execute_learn_search_with_min_score(mocker, settings, opensearch): "flashcards", "vector_embedding", "video.transcript", + "content_files.content", + "content_files.summary", + "content_files.flashcards", ] }, } @@ -4243,6 +4258,9 @@ def test_execute_learn_search_for_content_file_query(opensearch): "flashcards", "vector_embedding", "video.transcript", + "content_files.content", + "content_files.summary", + "content_files.flashcards", ] }, } diff --git a/learning_resources_search/constants.py b/learning_resources_search/constants.py index 338d386c8d..23b27fe0b3 100644 --- a/learning_resources_search/constants.py +++ b/learning_resources_search/constants.py @@ -6,7 +6,10 @@ from opensearchpy.exceptions import ConnectionError as ESConnectionError from urllib3.exceptions import TimeoutError as UrlTimeoutError -from learning_resources.constants import LEARNING_RESOURCE_SORTBY_OPTIONS +from learning_resources.constants import ( + CONTENT_FILE_LARGE_FIELDS, + LEARNING_RESOURCE_SORTBY_OPTIONS, +) ALIAS_ALL_INDICES = "all" COURSE_TYPE = "course" @@ -489,11 +492,10 @@ class FilterConfig: "resource_age_date", "featured_rank", "is_incomplete_or_stale", - "content", - "summary", - "flashcards", + *CONTENT_FILE_LARGE_FIELDS, "vector_embedding", "video.transcript", + *[f"content_files.{field}" for field in CONTENT_FILE_LARGE_FIELDS], ] LEARNING_RESOURCE_SEARCH_SORTBY_OPTIONS = { diff --git a/learning_resources_search/plugins.py b/learning_resources_search/plugins.py index 7b62f42fcd..0f3f5abfa6 100644 --- a/learning_resources_search/plugins.py +++ b/learning_resources_search/plugins.py @@ -261,12 +261,13 @@ def content_files_loaded(self, run): """ 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 non-B2B - run, or any published non-variant run of a test_mode course. + Qdrant: embed the run's published files (unchanged files exit via the + checksum gate in vector_search) and drop stale files. OpenSearch: index + only the best published non-B2B run, or any published non-variant run + of a test_mode course. - Args: - run(LearningResourceRun): The LearningResourceRun that was upserted + Args: + run: the LearningResourceRun that was upserted """ if not run.content_files.exists(): return @@ -283,10 +284,13 @@ def content_files_loaded(self, 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)) + # Purge before embedding so unpublished files' points are + # removed even if the embed task fails; the two tasks touch + # disjoint sets (published=False vs published=True files). index_tasks.append( vector_tasks.remove_unpublished_run_content_files.si(run.id) ) + index_tasks.append(vector_tasks.embed_run_content_files.si(run.id)) if index_tasks: try_with_retry_as_task(chain(*index_tasks)) diff --git a/learning_resources_search/plugins_test.py b/learning_resources_search/plugins_test.py index 6ee40c540f..f18ed8359a 100644 --- a/learning_resources_search/plugins_test.py +++ b/learning_resources_search/plugins_test.py @@ -568,3 +568,29 @@ def test_search_index_plugin_resource_upserted_generate_embeddings( mock_search_index_helpers.mock_generate_embeddings_immutable_signature.assert_called_once_with( [resource.id], resource_type, overwrite=True ) + + +@pytest.mark.django_db +def test_content_files_loaded_always_purges_unpublished( + mocker, mock_search_index_helpers, settings +): + """ + The remove-unpublished task always runs, and ahead of the embed task in the + chain, so a failed embed can't strand unpublished files' points in Qdrant. + """ + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True + chain_mock = mocker.patch("learning_resources_search.plugins.chain") + run = LearningResourceRunFactory.create( + published=True, learning_resource__create_runs=False + ) + ContentFileFactory.create(run=run) + + SearchIndexPlugin().content_files_loaded(run) + + mock_search_index_helpers.mock_remove_unpublished_run_contentfiles_immutable_signature.assert_called_once_with( + run.id + ) + chained = list(chain_mock.call_args.args) + purge = mock_search_index_helpers.mock_remove_unpublished_run_contentfiles_immutable_signature.return_value + embed = mock_search_index_helpers.mock_embed_run_contentfiles_immutable_signature.return_value + assert chained.index(purge) < chained.index(embed) diff --git a/learning_resources_search/serializers.py b/learning_resources_search/serializers.py index ae4b498c0b..3a69aba47a 100644 --- a/learning_resources_search/serializers.py +++ b/learning_resources_search/serializers.py @@ -140,6 +140,17 @@ def serialize_learning_resource_for_update( ): serialized_data["video"]["transcript"] = learning_resource_obj.video.transcript + if serialized_data.get("content_files"): + # The API serializer omits full text; re-serialize with the full + # serializer for nested search. Serializes content_files twice, which + # is acceptable in this celery-only indexing path. + serialized_data["content_files"] = [ + ContentFileSerializer(content_file).data + for content_file in ( + learning_resource_obj.direct_content_files_for_serialization() + ) + ] + if learning_resource_obj.in_featured_lists > 0: featured_rank = ( LearningResourceRelationship.objects.filter( diff --git a/learning_resources_search/serializers_test.py b/learning_resources_search/serializers_test.py index 110f1016e1..356604c725 100644 --- a/learning_resources_search/serializers_test.py +++ b/learning_resources_search/serializers_test.py @@ -870,6 +870,25 @@ def test_serialize_bulk_learning_resources_for_deletion(): ) == [{"_id": resource.id, "_op_type": "delete"}] +@pytest.mark.django_db +def test_serialize_learning_resource_for_update_readds_content_file_text(): + """ + Indexed learning material docs should include full content file text even + though the API serializer omits it + """ + resource = factories.LearningResourceFactory.create( + resource_type=LearningResourceType.video.name, runs=[] + ) + factories.ContentFileFactory.create( + run=None, + direct_learning_resource=resource, + content="full text for nested search", + ) + resource = LearningResource.objects.for_search_serialization().get(pk=resource.pk) + serialized = serializers.serialize_learning_resource_for_update(resource) + assert serialized["content_files"][0]["content"] == "full text for nested search" + + @pytest.mark.django_db def test_serialize_content_file_for_bulk(): """ diff --git a/main/settings.py b/main/settings.py index c83e10e8c9..3967afbd1f 100644 --- a/main/settings.py +++ b/main/settings.py @@ -36,7 +36,7 @@ from main.settings_pluggy import * # noqa: F403 from openapi.settings_spectacular import open_spectacular_settings -VERSION = "0.75.5" +VERSION = "0.76.0" log = logging.getLogger() @@ -495,7 +495,7 @@ }, "redis": { "BACKEND": "django_redis.cache.RedisCache", - "LOCATION": CELERY_BROKER_URL, # noqa: F405 + "LOCATION": REDIS_URL, # noqa: F405 "OPTIONS": {"CLIENT_CLASS": "django_redis.client.DefaultClient"}, }, # imagekit caching diff --git a/main/settings_celery.py b/main/settings_celery.py index 9241759492..62f11fce73 100644 --- a/main/settings_celery.py +++ b/main/settings_celery.py @@ -18,8 +18,8 @@ DEV_ENV = get_bool("DEV_ENV", False) # noqa: FBT003 USE_CELERY = True REDIS_URL = get_string("REDIS_URL", get_string("REDISCLOUD_URL", None)) -CELERY_BROKER_URL = get_string("CELERY_BROKER_URL", REDIS_URL) -CELERY_RESULT_BACKEND = get_string("CELERY_RESULT_BACKEND", REDIS_URL) +CELERY_BROKER_URL = get_string("CELERY_BROKER_URL", None) +CELERY_RESULT_BACKEND = get_string("CELERY_RESULT_BACKEND", None) # Celery's 24h default lets reindex chord fan-outs (thousands of subtask # result keys) accumulate; when the result backend is the broker/cache Redis # (the default here) that saturates memory. Keep results only long enough for diff --git a/main/utils.py b/main/utils.py index 4f47276f33..db209a800f 100644 --- a/main/utils.py +++ b/main/utils.py @@ -1,6 +1,7 @@ """main utilities""" import datetime +import json import logging import os from collections.abc import Callable @@ -15,8 +16,10 @@ from bs4 import BeautifulSoup from django.conf import settings from django.core.cache import caches +from django.http import HttpResponse from django.views.decorators.cache import cache_page from nh3 import nh3 +from rest_framework.renderers import JSONRenderer from rest_framework.response import Response from main.constants import ALLOWED_HTML_ATTRIBUTES, ALLOWED_HTML_TAGS @@ -37,6 +40,58 @@ def _sorted_query_string(query_dict): return "&".join(items) +def _needs_negotiated_response(request) -> bool: + """ + Whether the request needs content-negotiated rendering (e.g. the + browsable API) rather than raw cached JSON bytes. + """ + renderer = getattr(request, "accepted_renderer", None) + if renderer is not None: + # DRF negotiated the renderer in initial(); raw bytes only serve JSON + return renderer.format != "json" + return "text/html" in request.headers.get("Accept", "") + + +def _cached_response(request, cached_data): + """ + Build a response from a cache entry (rendered JSON bytes or legacy dict). + + JSON requests get the cached bytes as-is, skipping re-rendering. Requests + wanting another format (e.g. the browsable API) get a DRF Response so + content negotiation still applies. + """ + if not isinstance(cached_data, bytes): + return Response(cached_data) + if _needs_negotiated_response(request): + return Response(json.loads(cached_data)) + return HttpResponse(cached_data, content_type="application/json") + + +def _cache_response_json(cache_backend, cache_key, cache_timeout, response): + """ + Cache the response's rendered JSON bytes. + + Piggybacks on the response's own render pass so a cache miss doesn't render + the payload twice. Only a non-JSON renderer (the browsable API) needs a + separate JSON render. + """ + + def store(rendered): + content = ( + rendered.content + if rendered.accepted_renderer.format == "json" + else JSONRenderer().render(rendered.data) + ) + cache_backend.set(cache_key, content, cache_timeout) + + if hasattr(response, "add_post_render_callback"): + response.add_post_render_callback(store) + else: + cache_backend.set( + cache_key, JSONRenderer().render(response.data), cache_timeout + ) + + def _resolve_cache_timeout(timeout: int | None) -> int: """Resolve a cache timeout, deferring to settings when no timeout is given.""" if timeout is None: @@ -99,12 +154,16 @@ async def inner_function(request, *args, **kwargs): cached_data = await cache_backend.aget(cache_key) if cached_data is not None: - return Response(cached_data) + return _cached_response(request, cached_data) response = await func(request, *args, **kwargs) if response.status_code == 200: # noqa: PLR2004 - await cache_backend.aset(cache_key, response.data, cache_timeout) + # Cache rendered JSON bytes so cache hits skip + # DRF serialization entirely + _cache_response_json( + cache_backend, cache_key, cache_timeout, response + ) return response @@ -136,14 +195,15 @@ def inner_function(request, *args, **kwargs): # Try to get from cache cached_data = cache_backend.get(cache_key) if cached_data is not None: - return Response(cached_data) + return _cached_response(request, cached_data) # Execute view response = func(request, *args, **kwargs) - # Only cache successful responses + # Only cache successful responses. Cache rendered JSON bytes so + # cache hits skip DRF serialization entirely if response.status_code == 200: # noqa: PLR2004 - cache_backend.set(cache_key, response.data, cache_timeout) + _cache_response_json(cache_backend, cache_key, cache_timeout, response) return response diff --git a/main/utils_test.py b/main/utils_test.py index 26a31432f8..adeb91f757 100644 --- a/main/utils_test.py +++ b/main/utils_test.py @@ -1,14 +1,21 @@ """Utils tests""" +import asyncio import datetime +import json from math import ceil from tempfile import NamedTemporaryFile -from unittest.mock import MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, patch import pytest from django.contrib.auth import get_user_model +from django.core.cache import caches from django.http import QueryDict +from django.utils.decorators import method_decorator +from rest_framework.renderers import BrowsableAPIRenderer, JSONRenderer from rest_framework.response import Response +from rest_framework.test import APIRequestFactory, force_authenticate +from rest_framework.views import APIView from main.constants import ( ALLOWED_HTML_ATTRIBUTES_WITH_LINKS, @@ -274,12 +281,18 @@ def test_clean_data_preserves_allowed_links(): assert "javascript:" not in result # nh3 drops unsafe schemes -def _create_mock_request(*, is_authenticated=False, path="/test/", query=""): +def _create_mock_request( + *, is_authenticated=False, path="/test/", query="", accept="application/json" +): """Create a mock request object for testing cache decorators.""" request = MagicMock() request.user.is_authenticated = is_authenticated request.path = path request.GET = QueryDict(query) + request.headers = {"Accept": accept} + request.accepted_renderer = ( + BrowsableAPIRenderer() if "text/html" in accept else JSONRenderer() + ) return request @@ -320,96 +333,220 @@ def test_sorted_query_string_multi_value(): assert result == "a=first&topic=math&topic=science" -@patch("main.utils.caches") -def test_cache_page_for_anonymous_users_caches_anonymous(mock_caches): - """Anonymous user requests are cached.""" - mock_cache = MagicMock() - mock_cache.get.return_value = None - mock_caches.__getitem__.return_value = mock_cache +@pytest.fixture +def view_cache(settings): + """Enable view caching against a fresh locmem backend.""" + settings.REDIS_VIEW_CACHE_DURATION = 60 + settings.CACHES = { + **settings.CACHES, + "default": { + "BACKEND": "django.core.cache.backends.locmem.LocMemCache", + "LOCATION": "view-cache-tests", + }, + } + caches["default"].clear() + return caches["default"] - view = _create_view() - decorated = cache_page_for_anonymous_users(300)(view) - request = _create_mock_request(is_authenticated=False) - response1 = decorated(request) +def _decorated_api_view(decorator=None, status=200): + """Build a real DRF view with a cache decorator applied to its handler.""" + calls = {"count": 0} + + class CachedView(APIView): + authentication_classes = () + permission_classes = () + versioning_class = None + + @method_decorator(decorator or cache_page_for_all_users()) + def get(self, request): # noqa: ARG002 + calls["count"] += 1 + return Response({"result": "fresh", "call": calls["count"]}, status=status) + + view = CachedView.as_view() + view.calls = calls + return view + + +def _get(view, path="/test/", user=None): + """Issue a GET through the full DRF request/render cycle.""" + request = APIRequestFactory().get(path) + if user is not None: + force_authenticate(request, user=user) + response = view(request) + return response.render() if hasattr(response, "render") else response + + +def test_view_cache_end_to_end(view_cache): + """The second request is served from cache with identical JSON.""" + view = _decorated_api_view() + + first = _get(view) + second = _get(view) + + assert view.calls["count"] == 1 + assert first.content == second.content + assert json.loads(second.content) == {"result": "fresh", "call": 1} + assert second["Content-Type"] == "application/json" + + +def test_cache_miss_renders_once(view_cache): + """A miss caches the bytes from the response's own render pass.""" + view = _decorated_api_view() + + with patch("main.utils.JSONRenderer") as mock_renderer: + first = _get(view) + + mock_renderer.assert_not_called() + assert _get(view).content == first.content + assert view.calls["count"] == 1 + + +def test_anonymous_users_decorator_caches_anonymous(view_cache): + """Anonymous requests are cached by cache_page_for_anonymous_users.""" + view = _decorated_api_view(cache_page_for_anonymous_users()) + + _get(view) + _get(view) + + assert view.calls["count"] == 1 + + +def test_anonymous_users_decorator_skips_authenticated(view_cache): + """Authenticated requests bypass cache_page_for_anonymous_users.""" + view = _decorated_api_view(cache_page_for_anonymous_users()) + user = UserFactory.build() + + _get(view, user=user) + _get(view, user=user) + + assert view.calls["count"] == 2 - assert view.call_count["count"] == 1 - mock_cache.set.assert_called_once() - assert response1.data["result"] == "fresh" + +def test_all_users_decorator_caches_authenticated(view_cache): + """Authenticated requests are cached by cache_page_for_all_users.""" + view = _decorated_api_view() + + _get(view, user=UserFactory.build()) + _get(view, user=UserFactory.build()) + + assert view.calls["count"] == 1 + + +def test_cache_timeout_zero_skips_caching(view_cache): + """A timeout of 0 disables caching entirely.""" + view = _decorated_api_view(cache_page_for_all_users(0)) + + _get(view) + _get(view) + + assert view.calls["count"] == 2 + + +def test_cache_default_timeout_resolves_at_request_time(view_cache, settings): + """The default timeout is read from settings per request, not at decoration.""" + settings.REDIS_VIEW_CACHE_DURATION = 0 + view = _decorated_api_view() + settings.REDIS_VIEW_CACHE_DURATION = 60 + + _get(view) + _get(view) + + assert view.calls["count"] == 1 + + +def test_cache_default_timeout_zero_skips_caching(view_cache, settings): + """A default timeout of 0 disables caching entirely.""" + settings.REDIS_VIEW_CACHE_DURATION = 0 + view = _decorated_api_view() + + _get(view) + _get(view) + + assert view.calls["count"] == 2 + + +def test_cache_only_caches_200_responses(view_cache): + """Non-200 responses are not cached.""" + view = _decorated_api_view(status=404) + + _get(view) + _get(view) + + assert view.calls["count"] == 2 @patch("main.utils.caches") -def test_cache_page_for_anonymous_users_skips_authenticated(mock_caches): - """Authenticated user requests bypass the cache.""" +def test_async_cache_stores_rendered_json_bytes(mock_caches): + """The async decorator caches the bytes from the response's render pass.""" mock_cache = MagicMock() + mock_cache.aget = AsyncMock(return_value=None) mock_caches.__getitem__.return_value = mock_cache - view = _create_view() - decorated = cache_page_for_anonymous_users(300)(view) + async def view(request): + return Response({"result": "fresh"}) + + decorated = cache_page_for_all_users(300)(view) + response = asyncio.run(decorated(_create_mock_request())) - request = _create_mock_request(is_authenticated=True) - response1 = decorated(request) - response2 = decorated(request) + # complete the render pass DRF runs after the handler returns + response.accepted_renderer = JSONRenderer() + response.accepted_media_type = "application/json" + response.renderer_context = {} + response.render() - assert view.call_count["count"] == 2 - mock_cache.get.assert_not_called() - mock_cache.set.assert_not_called() - assert response1.data["call"] == 1 - assert response2.data["call"] == 2 + assert mock_cache.set.call_args.args[1] == response.content + assert json.loads(response.content) == {"result": "fresh"} @patch("main.utils.caches") -def test_cache_page_for_all_users_caches_anonymous(mock_caches): - """Anonymous user requests are cached with cache_page_for_all_users.""" +def test_cache_returns_cached_response(mock_caches): + """Subsequent requests return cached data (legacy dict entries).""" mock_cache = MagicMock() - mock_cache.get.return_value = None + cached_data = {"result": "cached", "call": 0} + mock_cache.get.return_value = cached_data mock_caches.__getitem__.return_value = mock_cache view = _create_view() decorated = cache_page_for_all_users(300)(view) - request = _create_mock_request(is_authenticated=False) - response1 = decorated(request) + response = decorated(_create_mock_request()) - assert view.call_count["count"] == 1 - mock_cache.set.assert_called_once() - assert response1.data["result"] == "fresh" + assert view.call_count["count"] == 0 + assert response.data == cached_data @patch("main.utils.caches") -def test_cache_page_for_all_users_caches_authenticated(mock_caches): - """Authenticated user requests are also cached with cache_page_for_all_users.""" +def test_cache_hit_returns_bytes_without_rendering(mock_caches): + """Cached rendered bytes are returned directly as an HttpResponse.""" mock_cache = MagicMock() - mock_cache.get.return_value = None + mock_cache.get.return_value = b'{"result": "cached"}' mock_caches.__getitem__.return_value = mock_cache view = _create_view() decorated = cache_page_for_all_users(300)(view) - request = _create_mock_request(is_authenticated=True) - response1 = decorated(request) + response = decorated(_create_mock_request()) - assert view.call_count["count"] == 1 - mock_cache.set.assert_called_once() - assert response1.data["result"] == "fresh" + assert view.call_count["count"] == 0 + assert response.content == b'{"result": "cached"}' + assert response["Content-Type"] == "application/json" @patch("main.utils.caches") -def test_cache_returns_cached_response(mock_caches): - """Subsequent requests return cached data.""" +def test_cache_hit_html_request_gets_negotiable_response(mock_caches): + """Browsable API (HTML) requests get a DRF Response from cached bytes.""" mock_cache = MagicMock() - cached_data = {"result": "cached", "call": 0} - mock_cache.get.return_value = cached_data + mock_cache.get.return_value = b'{"result": "cached"}' mock_caches.__getitem__.return_value = mock_cache view = _create_view() decorated = cache_page_for_all_users(300)(view) - request = _create_mock_request() - response = decorated(request) + response = decorated(_create_mock_request(accept="text/html,application/xhtml+xml")) assert view.call_count["count"] == 0 - assert response.data == cached_data + assert isinstance(response, Response) + assert response.data == {"result": "cached"} @patch("main.utils.caches") @@ -422,13 +559,10 @@ def test_cache_key_consistent_for_same_url(mock_caches): view = _create_view() decorated = cache_page_for_all_users(300)(view) - request1 = _create_mock_request(path="/api/test/", query="a=1&b=2") - request2 = _create_mock_request(path="/api/test/", query="a=1&b=2") - - decorated(request1) + decorated(_create_mock_request(path="/api/test/", query="a=1&b=2")) key1 = mock_cache.get.call_args_list[0][0][0] - decorated(request2) + decorated(_create_mock_request(path="/api/test/", query="a=1&b=2")) key2 = mock_cache.get.call_args_list[1][0][0] assert key1 == key2 @@ -444,13 +578,10 @@ def test_cache_key_consistent_with_reordered_params(mock_caches): view = _create_view() decorated = cache_page_for_all_users(300)(view) - request1 = _create_mock_request(path="/api/test/", query="b=2&a=1") - request2 = _create_mock_request(path="/api/test/", query="a=1&b=2") - - decorated(request1) + decorated(_create_mock_request(path="/api/test/", query="b=2&a=1")) key1 = mock_cache.get.call_args_list[0][0][0] - decorated(request2) + decorated(_create_mock_request(path="/api/test/", query="a=1&b=2")) key2 = mock_cache.get.call_args_list[1][0][0] assert key1 == key2 @@ -466,98 +597,15 @@ def test_cache_key_different_for_different_paths(mock_caches): view = _create_view() decorated = cache_page_for_all_users(300)(view) - request1 = _create_mock_request(path="/api/test1/") - request2 = _create_mock_request(path="/api/test2/") - - decorated(request1) + decorated(_create_mock_request(path="/api/test1/")) key1 = mock_cache.get.call_args_list[0][0][0] - decorated(request2) + decorated(_create_mock_request(path="/api/test2/")) key2 = mock_cache.get.call_args_list[1][0][0] assert key1 != key2 -@patch("main.utils.caches") -def test_cache_timeout_zero_skips_caching(mock_caches): - """Timeout of 0 or negative skips caching entirely.""" - mock_cache = MagicMock() - mock_caches.__getitem__.return_value = mock_cache - - view = _create_view() - decorated = cache_page_for_all_users(0)(view) - - request = _create_mock_request() - response1 = decorated(request) - response2 = decorated(request) - - assert view.call_count["count"] == 2 - mock_cache.get.assert_not_called() - mock_cache.set.assert_not_called() - assert response1.data["call"] == 1 - assert response2.data["call"] == 2 - - -@patch("main.utils.caches") -def test_cache_default_timeout_resolves_at_request_time(mock_caches, settings): - """Default timeout is read from settings when the request is handled.""" - settings.REDIS_VIEW_CACHE_DURATION = 0 - mock_cache = MagicMock() - mock_cache.get.return_value = None - mock_caches.__getitem__.return_value = mock_cache - - view = _create_view() - decorated = cache_page_for_all_users()(view) - settings.REDIS_VIEW_CACHE_DURATION = 300 - - request = _create_mock_request() - decorated(request) - - assert view.call_count["count"] == 1 - mock_cache.set.assert_called_once() - assert mock_cache.set.call_args.args[2] == 300 - - -@patch("main.utils.caches") -def test_cache_default_timeout_zero_skips_caching(mock_caches, settings): - """Default timeout of 0 skips caching when the request is handled.""" - settings.REDIS_VIEW_CACHE_DURATION = 0 - mock_cache = MagicMock() - mock_caches.__getitem__.return_value = mock_cache - - view = _create_view() - decorated = cache_page_for_all_users()(view) - - request = _create_mock_request() - response1 = decorated(request) - response2 = decorated(request) - - assert view.call_count["count"] == 2 - mock_cache.get.assert_not_called() - mock_cache.set.assert_not_called() - assert response1.data["call"] == 1 - assert response2.data["call"] == 2 - - -@patch("main.utils.caches") -def test_cache_only_caches_200_responses(mock_caches): - """Non-200 responses are not cached.""" - mock_cache = MagicMock() - mock_cache.get.return_value = None - mock_caches.__getitem__.return_value = mock_cache - - def error_view(request): - response = Response({"error": "not found"}, status=404) - response.status_code = 404 - return response - - decorated = cache_page_for_all_users(300)(error_view) - request = _create_mock_request() - decorated(request) - - mock_cache.set.assert_not_called() - - @patch("main.utils.caches") def test_cache_uses_specified_backend(mock_caches): """Cache decorator uses the specified cache backend.""" diff --git a/news_events/tasks.py b/news_events/tasks.py index 1168fc7493..92a9077768 100644 --- a/news_events/tasks.py +++ b/news_events/tasks.py @@ -5,14 +5,14 @@ from news_events.etl import pipelines -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) def get_medium_mit_news(): """Run the Medium MIT News ETL pipeline""" pipelines.medium_mit_news_etl() clear_views_cache() -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) def get_ol_events(): """Run the Open Learning Events ETL pipeline""" pipelines.ol_events_etl() @@ -33,21 +33,21 @@ def get_sloan_exec_webinars(): clear_views_cache() -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) def get_mitpe_news(): """Run the MIT Professional Education news ETL pipeline""" pipelines.mitpe_news_etl() clear_views_cache() -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) def get_mitpe_events(): """Run the MIT Professional Education events ETL pipeline""" pipelines.mitpe_events_etl() clear_views_cache() -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) def get_website_content_news(): """Run the website content news ETL pipeline""" diff --git a/openapi/settings_spectacular.py b/openapi/settings_spectacular.py index 70da6f34ca..b376ca5a7f 100644 --- a/openapi/settings_spectacular.py +++ b/openapi/settings_spectacular.py @@ -14,6 +14,9 @@ "ContentFeedbackSentimentEnum": ( "content_feedback.constants.CONTENT_FEEDBACK_SENTIMENT_CHOICES" ), + "ContentFileContentTypeEnum": ( + "learning_resources.constants.VALID_COURSE_CONTENT_CHOICES" + ), }, "AUTHENTICATION_WHITELIST": [], "SCHEMA_PATH_PREFIX": "/api/v[0-9]", diff --git a/openapi/specs/v0.yaml b/openapi/specs/v0.yaml index 52cb807866..80a9e8e272 100644 --- a/openapi/specs/v0.yaml +++ b/openapi/specs/v0.yaml @@ -1864,7 +1864,7 @@ components: items: type: string content_type: - $ref: '#/components/schemas/ContentTypeEnum' + $ref: '#/components/schemas/ContentFileContentTypeEnum' content: type: string nullable: true @@ -1941,6 +1941,23 @@ components: - resource_id - resource_readable_id - topics + ContentFileContentTypeEnum: + enum: + - page + - file + - video + - pdf + type: string + description: |- + * `page` - page + * `file` - file + * `video` - video + * `pdf` - pdf + x-enum-descriptions: + - page + - file + - video + - pdf ContentFileVectorSearchResponse: type: object description: SearchResponseSerializer with OpenAPI annotations for Content Files @@ -1993,23 +2010,6 @@ components: - next - previous - results - ContentTypeEnum: - enum: - - page - - file - - video - - pdf - type: string - description: |- - * `page` - page - * `file` - file - * `video` - video - * `pdf` - pdf - x-enum-descriptions: - - page - - file - - video - - pdf Counts: type: object properties: @@ -2663,7 +2663,7 @@ components: content_files: type: array items: - $ref: '#/components/schemas/ContentFile' + $ref: '#/components/schemas/NestedContentFile' nullable: true readOnly: true description: @@ -3821,6 +3821,136 @@ components: - next - previous - results + NestedContentFile: + type: object + description: |- + ContentFileSerializer without the large text fields (content, summary, + flashcards), for nesting inside learning resource API responses. + The search indexing path re-adds full content where needed. + properties: + id: + type: integer + readOnly: true + run_id: + type: integer + direct_learning_resource_id: + type: integer + nullable: true + run_title: + type: string + run_slug: + type: string + departments: + type: array + items: + $ref: '#/components/schemas/LearningResourceDepartment' + readOnly: true + semester: + type: string + year: + type: integer + topics: + type: array + items: + $ref: '#/components/schemas/LearningResourceTopic' + readOnly: true + key: + type: string + nullable: true + maxLength: 1024 + uid: + type: string + nullable: true + maxLength: 36 + title: + type: string + nullable: true + maxLength: 1024 + description: + type: string + nullable: true + require_summaries: + type: boolean + readOnly: true + url: + type: string + nullable: true + content_feature_type: + type: array + items: + type: string + content_type: + $ref: '#/components/schemas/ContentFileContentTypeEnum' + content_title: + type: string + nullable: true + maxLength: 1024 + content_author: + type: string + nullable: true + maxLength: 1024 + content_language: + type: string + nullable: true + maxLength: 24 + checksum: + type: string + image_src: + type: string + format: uri + nullable: true + maxLength: 200 + resource_id: + type: string + readOnly: true + resource_readable_id: + type: string + readOnly: true + source_path: + type: string + course_number: + type: array + items: + type: string + description: Extract the course number(s) from the associated course + readOnly: true + file_type: + type: string + nullable: true + maxLength: 128 + file_extension: + type: string + nullable: true + maxLength: 32 + offered_by: + allOf: + - $ref: '#/components/schemas/LearningResourceOfferor' + readOnly: true + platform: + allOf: + - $ref: '#/components/schemas/LearningResourcePlatform' + readOnly: true + run_readable_id: + type: string + edx_module_id: + type: string + nullable: true + maxLength: 1024 + youtube_id: + type: string + nullable: true + maxLength: 32 + required: + - content_feature_type + - course_number + - departments + - id + - offered_by + - platform + - require_summaries + - resource_id + - resource_readable_id + - topics NewsFeedItem: type: object description: Serializer for News FeedItem @@ -5731,6 +5861,10 @@ components: username: type: string readOnly: true + global_id: + type: string + readOnly: true + nullable: true profile: $ref: '#/components/schemas/Profile' first_name: @@ -5750,6 +5884,7 @@ components: readOnly: true required: - first_name + - global_id - id - is_article_editor - is_authenticated @@ -6361,7 +6496,7 @@ components: content_files: type: array items: - $ref: '#/components/schemas/ContentFile' + $ref: '#/components/schemas/NestedContentFile' nullable: true readOnly: true description: diff --git a/openapi/specs/v1.yaml b/openapi/specs/v1.yaml index ac43e926f4..72888d26bb 100644 --- a/openapi/specs/v1.yaml +++ b/openapi/specs/v1.yaml @@ -9872,7 +9872,6 @@ paths: name: source schema: enum: - - micromasters - mit_edx - mitpe - mitxonline @@ -9888,7 +9887,6 @@ paths: type: string minLength: 1 description: |- - * `micromasters` - micromasters * `mit_edx` - mit_edx * `mitpe` - mitpe * `mitxonline` - mitxonline @@ -11208,7 +11206,7 @@ components: content_files: type: array items: - $ref: '#/components/schemas/ContentFile' + $ref: '#/components/schemas/NestedContentFile' nullable: true readOnly: true description: @@ -13655,6 +13653,136 @@ components: - child - id - parent + NestedContentFile: + type: object + description: |- + ContentFileSerializer without the large text fields (content, summary, + flashcards), for nesting inside learning resource API responses. + The search indexing path re-adds full content where needed. + properties: + id: + type: integer + readOnly: true + run_id: + type: integer + direct_learning_resource_id: + type: integer + nullable: true + run_title: + type: string + run_slug: + type: string + departments: + type: array + items: + $ref: '#/components/schemas/LearningResourceDepartment' + readOnly: true + semester: + type: string + year: + type: integer + topics: + type: array + items: + $ref: '#/components/schemas/LearningResourceTopic' + readOnly: true + key: + type: string + nullable: true + maxLength: 1024 + uid: + type: string + nullable: true + maxLength: 36 + title: + type: string + nullable: true + maxLength: 1024 + description: + type: string + nullable: true + require_summaries: + type: boolean + readOnly: true + url: + type: string + nullable: true + content_feature_type: + type: array + items: + type: string + content_type: + $ref: '#/components/schemas/ContentFileContentTypeEnum' + content_title: + type: string + nullable: true + maxLength: 1024 + content_author: + type: string + nullable: true + maxLength: 1024 + content_language: + type: string + nullable: true + maxLength: 24 + checksum: + type: string + image_src: + type: string + format: uri + nullable: true + maxLength: 200 + resource_id: + type: string + readOnly: true + resource_readable_id: + type: string + readOnly: true + source_path: + type: string + course_number: + type: array + items: + type: string + description: Extract the course number(s) from the associated course + readOnly: true + file_type: + type: string + nullable: true + maxLength: 128 + file_extension: + type: string + nullable: true + maxLength: 32 + offered_by: + allOf: + - $ref: '#/components/schemas/LearningResourceOfferor' + readOnly: true + platform: + allOf: + - $ref: '#/components/schemas/LearningResourcePlatform' + readOnly: true + run_readable_id: + type: string + edx_module_id: + type: string + nullable: true + maxLength: 1024 + youtube_id: + type: string + nullable: true + maxLength: 32 + required: + - content_feature_type + - course_number + - departments + - id + - offered_by + - platform + - require_summaries + - resource_id + - resource_readable_id + - topics NullEnum: enum: - null @@ -16368,7 +16496,6 @@ components: - Next start date ascending SourceEnum: enum: - - micromasters - mit_edx - mitpe - mitxonline @@ -16383,7 +16510,6 @@ components: - ovs type: string description: |- - * `micromasters` - micromasters * `mit_edx` - mit_edx * `mitpe` - mitpe * `mitxonline` - mitxonline @@ -16397,7 +16523,6 @@ components: * `canvas` - canvas * `ovs` - ovs x-enum-descriptions: - - micromasters - mit_edx - mitpe - mitxonline @@ -17218,7 +17343,7 @@ components: content_files: type: array items: - $ref: '#/components/schemas/ContentFile' + $ref: '#/components/schemas/NestedContentFile' nullable: true readOnly: true description: diff --git a/profiles/serializers.py b/profiles/serializers.py index ac54c2ef73..4a763c318a 100644 --- a/profiles/serializers.py +++ b/profiles/serializers.py @@ -383,6 +383,7 @@ class Meta: fields = ( "id", "username", + "global_id", "profile", "email", "first_name", @@ -391,7 +392,7 @@ class Meta: "is_learning_path_editor", "is_authenticated", ) - read_only_fields = ("id", "username", "is_authenticated") + read_only_fields = ("id", "username", "global_id", "is_authenticated") class ProgramCertificateSerializer(serializers.ModelSerializer): diff --git a/profiles/serializers_test.py b/profiles/serializers_test.py index 9b942c3825..0d1b0a81a6 100644 --- a/profiles/serializers_test.py +++ b/profiles/serializers_test.py @@ -38,6 +38,7 @@ def test_serialize_user(user): assert UserSerializer(user).data == { "id": user.id, "username": user.username, + "global_id": user.global_id, "first_name": user.first_name, "last_name": user.last_name, "is_learning_path_editor": False, @@ -90,6 +91,7 @@ def test_serialize_create_user(db, mocker): assert UserSerializer(instance=user).data == { "id": user.id, "username": user.username, + "global_id": user.global_id, "first_name": user.first_name, "last_name": user.last_name, "is_learning_path_editor": False, diff --git a/profiles/views_test.py b/profiles/views_test.py index eeb21949e3..864e2716d7 100644 --- a/profiles/views_test.py +++ b/profiles/views_test.py @@ -39,6 +39,7 @@ def test_list_users(staff_client, staff_user): { "id": staff_user.id, "username": staff_user.username, + "global_id": staff_user.global_id, "first_name": staff_user.first_name, "last_name": staff_user.last_name, "is_learning_path_editor": True, @@ -98,6 +99,7 @@ def test_get_user(staff_client, user): assert resp.json() == { "id": user.id, "username": user.username, + "global_id": user.global_id, "first_name": user.first_name, "last_name": user.last_name, "is_article_editor": True, @@ -188,6 +190,7 @@ def test_patch_user(staff_client, user, email, email_optin, toc_optin): assert resp.json() == { "id": user.id, "username": user.username, + "global_id": user.global_id, "first_name": user.first_name, "last_name": user.last_name, "is_learning_path_editor": True, @@ -390,6 +393,7 @@ def test_get_user_by_me(mocker, client, user, is_anonymous): assert resp.json() == { "id": None, "username": "", + "global_id": None, "is_learning_path_editor": False, "is_article_editor": False, "is_authenticated": False, @@ -398,6 +402,7 @@ def test_get_user_by_me(mocker, client, user, is_anonymous): assert resp.json() == { "id": user.id, "username": user.username, + "global_id": user.global_id, "first_name": user.first_name, "last_name": user.last_name, "is_learning_path_editor": False, diff --git a/vector_search/constants.py b/vector_search/constants.py index 2302fae0bf..85ef1f42dc 100644 --- a/vector_search/constants.py +++ b/vector_search/constants.py @@ -5,6 +5,22 @@ CONTENT_FILES_COLLECTION_NAME = f"{settings.QDRANT_BASE_COLLECTION_NAME}.content_files" TOPICS_COLLECTION_NAME = f"{settings.QDRANT_BASE_COLLECTION_NAME}.topics" +# ContentFile columns (beyond checksum, which only covers content) compared by the +# embed_run_content_files pre-pass to detect stale Qdrant payloads. Every entry MUST +# be an exact serializer pass-through of a scalar/JSON ContentFile column: a field +# the serializer transforms would never converge, flagging every file on every load +# (test_content_file_prepass_fields_are_serializer_pass_through guards this). +CONTENT_FILE_PREPASS_PAYLOAD_FIELDS = ( + "title", + "description", + "url", + "file_type", + "file_extension", + "content_type", + "edx_module_id", + "summary", + "flashcards", +) QDRANT_CONTENT_FILE_PARAM_MAP = { "key": "key", diff --git a/vector_search/tasks.py b/vector_search/tasks.py index 4e842589bc..30a03d1ab4 100644 --- a/vector_search/tasks.py +++ b/vector_search/tasks.py @@ -15,6 +15,7 @@ ContentFile, Course, LearningResource, + LearningResourceRun, ) from learning_resources.serializers import ( ContentFileSerializer, @@ -39,10 +40,12 @@ now_in_utc, ) from vector_search.constants import ( + CONTENT_FILE_PREPASS_PAYLOAD_FIELDS, CONTENT_FILES_COLLECTION_NAME, RESOURCES_COLLECTION_NAME, ) from vector_search.utils import ( + _stored_content_payloads, embed_learning_resources, embed_topics, filter_existing_qdrant_points_by_ids, @@ -462,16 +465,111 @@ def embed_new_content_files(self): ) -@app.task(bind=True) +@app.task(bind=True, max_retries=3) def embed_run_content_files(self, run_id): """ - Embed contentfiles associated with a run + Embed the run's published content files whose Qdrant points are missing or + stale (checksum or a payload metadata field differs). + + A run-level pre-pass batch-compares each file's DB checksum and payload + metadata columns against the stored Qdrant payload, so a fully-unchanged + run costs one DB query plus a few batched retrieves instead of serializing + every file. A checksum-matching file with drifted metadata (edited title, + newly generated summary, ...) is dispatched but exits via the payload-only + update path downstream — no re-embedding. Failed or purged embeds show up + as missing/stale points, so they self-heal on the next load. + + Content-less files are excluded: they never produce Qdrant points, so they + would otherwise be re-flagged on every load. Any point left over from when + such a file still had content is removed. Transient Qdrant errors during + the pre-pass retry with backoff so a blip doesn't defer the run's embedding + to the next load. """ - content_file_ids = list( - ContentFile.objects.filter(run__id=run_id).values_list("id", flat=True) + run = ( + LearningResourceRun.objects.select_related("learning_resource__platform") + .filter(id=run_id) + .first() ) + if run is None: + return None + resource = run.learning_resource + platform_code = resource.platform.code if resource.platform else "" + + def first_chunk_point_id(key): + # Returns the qdrant point id for the first chunk of the contentfile, + # mirroring the doc fields ContentFileSerializer emits for run files + return vector_point_id( + vector_point_key( + { + "platform": {"code": platform_code}, + "resource_readable_id": resource.readable_id, + "run_readable_id": run.run_id, + "key": key, + }, + chunk_number=0, + document_type="content_file", + ) + ) + + contentless = Q(content__isnull=True) | Q(content="") + pid_rows = [ + (cf_id, first_chunk_point_id(key), checksum, meta) + for cf_id, key, checksum, *meta in ContentFile.objects.filter( + run=run, published=True + ) + .exclude(contentless) + .values_list("id", "key", "checksum", *CONTENT_FILE_PREPASS_PAYLOAD_FIELDS) + ] + contentless_rows = [ + (cf_id, first_chunk_point_id(key)) + for cf_id, key in ContentFile.objects.filter( + contentless, run=run, published=True + ).values_list("id", "key") + ] + try: + stored = _stored_content_payloads( + [pid for _, pid, _, _ in pid_rows] + [pid for _, pid in contentless_rows], + fields=("checksum", *CONTENT_FILE_PREPASS_PAYLOAD_FIELDS), + ) + except grpc.RpcError as err: + if err.code() in ( + grpc.StatusCode.DEADLINE_EXCEEDED, + grpc.StatusCode.UNAVAILABLE, + ): + raise self.retry(exc=err, countdown=_retry_countdown(self.request.retries)) # noqa: B904 + raise + + def is_stale(pid, checksum, meta): + payload = stored.get(pid) + if payload is None or payload.get("checksum") != checksum: + return True + return any( + payload.get(field) != value + for field, value in zip(CONTENT_FILE_PREPASS_PAYLOAD_FIELDS, meta) + ) - return _replace_with_finalized_chain(self, content_file_ids, overwrite=True) + ids = [ + cf_id + for cf_id, pid, checksum, meta in pid_rows + if is_stale(pid, checksum, meta) + ] + # A stored point for a now-contentless file is a leftover from when the + # file had content — remove it. Inline rather than a chained task: leftovers + # are rare and few, and a failed delete self-heals on the next load. + leftover_ids = [cf_id for cf_id, pid in contentless_rows if pid in stored] + log.info( + "embed_run_content_files run %s: %d of %d files need embedding, " + "%d leftover contentless points to remove", + run_id, + len(ids), + len(pid_rows), + len(leftover_ids), + ) + if leftover_ids: + remove_qdrant_records(leftover_ids, CONTENT_FILE_TYPE) + if not ids: + return None + return _replace_with_finalized_chain(self, ids, overwrite=True) @app.task(bind=True) @@ -632,12 +730,22 @@ def embeddings_healthcheck(): def _missing_summaries(): - summarizer = ContentSummarizer() - return summarizer.get_unprocessed_content_file_ids( + resource_ids = list( LearningResource.objects.filter(require_summaries=True) .filter(Q(published=True) | Q(test_mode=True)) .values_list("id", flat=True) ) + if not resource_ids: + # get_unprocessed_content_file_ids treats an empty learning_resource_ids + # list the same as None (no restriction), so short-circuit here instead + # of letting it scan every learning resource. + return [] + + summarizer = ContentSummarizer() + return summarizer.get_unprocessed_content_file_ids( + overwrite=False, + learning_resource_ids=resource_ids, + ) def _sentry_healthcheck_log(healthcheck, alert_type, context, message): @@ -648,7 +756,7 @@ def _sentry_healthcheck_log(healthcheck, alert_type, context, message): sentry_sdk.capture_message(message) -@app.task +@app.task(acks_late=True, reject_on_worker_lost=True) def sync_topics(): """ Sync topics to the Qdrant collection diff --git a/vector_search/tasks_test.py b/vector_search/tasks_test.py index 769339ba19..e821d5bc16 100644 --- a/vector_search/tasks_test.py +++ b/vector_search/tasks_test.py @@ -28,7 +28,9 @@ PROGRAM_TYPE, ) from learning_resources_search.exceptions import RetryError +from learning_resources_search.serializers import serialize_bulk_content_files from main.utils import now_in_utc +from vector_search.constants import CONTENT_FILE_PREPASS_PAYLOAD_FIELDS from vector_search.tasks import ( _record_embedding_failure, _retry_countdown, @@ -44,7 +46,7 @@ remove_unpublished_run_content_files, start_embed_resources, ) -from vector_search.utils import vector_point_id +from vector_search.utils import vector_point_id, vector_point_key pytestmark = pytest.mark.django_db @@ -703,7 +705,8 @@ def test_embed_run_content_files(mocker, mocked_celery, settings): settings.QDRANT_CHUNK_SIZE = 2 run = LearningResourceRunFactory.create() content_file_ids = [ - content_file.id for content_file in ContentFileFactory.create_batch(3, run=run) + content_file.id + for content_file in ContentFileFactory.create_batch(3, run=run, content="text") ] ContentFileFactory.create() generate_embeddings_mock = mocker.patch( @@ -767,6 +770,234 @@ def test_embed_run_content_files_no_files_returns_none(mocker, mocked_celery): mocked_celery.chain.assert_not_called() +def test_embed_run_content_files_skips_unpublished(mocker, mocked_celery, settings): + """Unpublished files are never embedded.""" + settings.QDRANT_CHUNK_SIZE = 50 + run = LearningResourceRunFactory.create() + published = ContentFileFactory.create(run=run, published=True, content="aaa") + ContentFileFactory.create(run=run, published=False, content="bbb") + generate_embeddings_mock = mocker.patch( + "vector_search.tasks.generate_embeddings", autospec=True + ) + + with pytest.raises(mocked_celery.replace_exception_class): + embed_run_content_files.delay(run.id) + + assert _embedded_content_file_ids(generate_embeddings_mock) == {published.id} + + +def test_embed_run_content_files_skips_contentless(mocker, mocked_celery, settings): + """ + Files without content never produce Qdrant points, so the pre-pass must not + flag them as stale (they would otherwise be re-dispatched on every load). + """ + settings.QDRANT_CHUNK_SIZE = 50 + run = LearningResourceRunFactory.create() + with_content = ContentFileFactory.create(run=run, published=True, content="aaa") + ContentFileFactory.create(run=run, published=True, content="") + ContentFileFactory.create(run=run, published=True, content=None) + generate_embeddings_mock = mocker.patch( + "vector_search.tasks.generate_embeddings", autospec=True + ) + + with pytest.raises(mocked_celery.replace_exception_class): + embed_run_content_files.delay(run.id) + + assert _embedded_content_file_ids(generate_embeddings_mock) == {with_content.id} + + +def test_embed_run_content_files_removes_leftover_contentless_points(mocker): + """ + A published file whose content became empty keeps the point embedded from + its old content; the pre-pass detects and removes it. Contentless files + with no stored point trigger no removal. + """ + run = LearningResourceRunFactory.create() + emptied = ContentFileFactory.create(run=run, published=True, content="") + ContentFileFactory.create(run=run, published=True, content=None) + pids = _serializer_chunk0_pids([emptied]) + mocker.patch( + "vector_search.tasks._stored_content_payloads", + return_value={pids[emptied.id]: {"checksum": "from-old-content"}}, + ) + generate_embeddings_mock = mocker.patch( + "vector_search.tasks.generate_embeddings", autospec=True + ) + remove_mock = mocker.patch( + "vector_search.tasks.remove_qdrant_records", autospec=True + ) + + assert embed_run_content_files(run.id) is None + + generate_embeddings_mock.si.assert_not_called() + remove_mock.assert_called_once_with([emptied.id], CONTENT_FILE_TYPE) + + +def test_embed_run_content_files_retries_transient_qdrant_errors(mocker): + """A transient Qdrant error in the pre-pass retries instead of failing the run.""" + run = LearningResourceRunFactory.create() + ContentFileFactory.create(run=run, published=True, content="aaa") + mocker.patch( + "vector_search.tasks._stored_content_payloads", + side_effect=_rpc_error(grpc.StatusCode.UNAVAILABLE), + ) + retry = mocker.patch.object(embed_run_content_files, "retry", side_effect=Retry()) + + with pytest.raises(Retry): + embed_run_content_files(run.id) + + retry.assert_called_once() + assert retry.call_args.kwargs["countdown"] >= 0 + + +def test_embed_run_content_files_does_not_retry_terminal_errors(mocker): + """A non-transient Qdrant error in the pre-pass propagates without retry.""" + run = LearningResourceRunFactory.create() + ContentFileFactory.create(run=run, published=True, content="aaa") + mocker.patch( + "vector_search.tasks._stored_content_payloads", + side_effect=_rpc_error(grpc.StatusCode.INVALID_ARGUMENT), + ) + + with pytest.raises(grpc.RpcError): + embed_run_content_files(run.id) + + +def _serializer_chunk0_pids(content_files): + """Chunk-0 point ids as the embed pipeline (serializer path) computes them""" + return { + doc["id"]: vector_point_id( + vector_point_key(doc, chunk_number=0, document_type="content_file") + ) + for doc in serialize_bulk_content_files([cf.id for cf in content_files]) + } + + +def _stored_payload_entry(content_file, **overrides): + """Build a stored-payload map entry matching the file's current DB state""" + return { + "checksum": content_file.checksum, + **{ + field: getattr(content_file, field) + for field in CONTENT_FILE_PREPASS_PAYLOAD_FIELDS + }, + **overrides, + } + + +def test_embed_run_content_files_pre_pass_skips_unchanged( + mocker, mocked_celery, settings +): + """ + Only files whose stored Qdrant payload is missing or stale are embedded. + + The stored-payload map is keyed by serializer-derived point ids, so the + unchanged file is skipped only if the task's pre-pass computes the same + point id as the embed pipeline. + """ + settings.QDRANT_CHUNK_SIZE = 50 + run = LearningResourceRunFactory.create() + # ContentFile.save() computes checksum from content + unchanged = ContentFileFactory.create(run=run, published=True, content="aaa") + stale = ContentFileFactory.create(run=run, published=True, content="bbb") + missing = ContentFileFactory.create(run=run, published=True, content="ccc") + pids = _serializer_chunk0_pids([unchanged, stale, missing]) + stored_mock = mocker.patch( + "vector_search.tasks._stored_content_payloads", + return_value={ + pids[unchanged.id]: _stored_payload_entry(unchanged), + pids[stale.id]: _stored_payload_entry(stale, checksum="stale-checksum"), + }, + ) + generate_embeddings_mock = mocker.patch( + "vector_search.tasks.generate_embeddings", autospec=True + ) + + with pytest.raises(mocked_celery.replace_exception_class): + embed_run_content_files.delay(run.id) + + assert _embedded_content_file_ids(generate_embeddings_mock) == { + stale.id, + missing.id, + } + assert set(stored_mock.call_args.args[0]) == set(pids.values()) + + +def test_embed_run_content_files_pre_pass_dispatches_metadata_only_change( + mocker, mocked_celery, settings +): + """ + A file with a matching checksum but drifted payload metadata (edited title, + newly generated summary) is dispatched so its Qdrant payload gets refreshed. + """ + settings.QDRANT_CHUNK_SIZE = 50 + run = LearningResourceRunFactory.create() + retitled = ContentFileFactory.create(run=run, published=True, content="aaa") + summarized = ContentFileFactory.create(run=run, published=True, content="bbb") + current = ContentFileFactory.create(run=run, published=True, content="ccc") + pids = _serializer_chunk0_pids([retitled, summarized, current]) + mocker.patch( + "vector_search.tasks._stored_content_payloads", + return_value={ + pids[retitled.id]: _stored_payload_entry(retitled, title="old title"), + pids[summarized.id]: _stored_payload_entry(summarized, summary=""), + pids[current.id]: _stored_payload_entry(current), + }, + ) + summarized.summary = "a new summary" + summarized.save() + generate_embeddings_mock = mocker.patch( + "vector_search.tasks.generate_embeddings", autospec=True + ) + + with pytest.raises(mocked_celery.replace_exception_class): + embed_run_content_files.delay(run.id) + + assert _embedded_content_file_ids(generate_embeddings_mock) == { + retitled.id, + summarized.id, + } + + +def test_content_file_prepass_fields_are_serializer_pass_through(): + """ + Every pre-pass-compared field must be an exact serializer pass-through of + the ContentFile column: a transformed field would never converge with the + stored payload, flagging every file on every load. + """ + content_file = ContentFileFactory.create( + run=LearningResourceRunFactory.create(), + published=True, + content="some content", + summary="a summary", + flashcards=[{"question": "q", "answer": "a"}], + ) + doc = next(iter(serialize_bulk_content_files([content_file.id]))) + for field in ("checksum", *CONTENT_FILE_PREPASS_PAYLOAD_FIELDS): + assert doc[field] == getattr(content_file, field), field + + +def test_embed_run_content_files_all_unchanged_dispatches_nothing( + mocker, mocked_celery +): + """A fully-unchanged run embeds nothing and schedules no chain.""" + run = LearningResourceRunFactory.create() + files = ContentFileFactory.create_batch(2, run=run, published=True, content="x") + pids = _serializer_chunk0_pids(files) + mocker.patch( + "vector_search.tasks._stored_content_payloads", + return_value={pids[cf.id]: _stored_payload_entry(cf) for cf in files}, + ) + generate_embeddings_mock = mocker.patch( + "vector_search.tasks.generate_embeddings", autospec=True + ) + + assert embed_run_content_files(run.id) is None + + generate_embeddings_mock.si.assert_not_called() + mocked_celery.chain.assert_not_called() + + def test_embeddings_healthcheck_no_missing_embeddings(mocker): """ Test embeddings_healthcheck when there are no missing embeddings @@ -884,6 +1115,96 @@ def test_embeddings_healthcheck_missing_summaries(mocker): ) +def test_embeddings_healthcheck_excludes_already_summarized(mocker): + """ + embeddings_healthcheck should not count content files that already have + a summary as missing (regression test for passing overwrite=True + implicitly by mis-ordering get_unprocessed_content_file_ids arguments) + """ + content_extension = [".srt"] + content_type = ["file"] + platform = LearningResourcePlatformFactory.create() + ContentSummarizerConfigurationFactory.create( + allowed_extensions=content_extension, + allowed_content_types=content_type, + is_active=True, + llm_model="test", + platform__code=platform.code, + ) + resource = LearningResourceFactory.create( + published=True, require_summaries=True, platform=platform + ) + resource.runs.all().delete() + learning_resource_run = LearningResourceRunFactory.create( + published=True, + learning_resource=resource, + ) + learning_resource_run.learning_resource = resource + learning_resource_run.save() + + ContentFileFactory.create( + published=True, + content="test", + file_extension=content_extension[0], + summary="already summarized", + flashcards=[{"question": "q", "answer": "a"}], + content_type=content_type[0], + run=learning_resource_run, + ) + mocker.patch( + "vector_search.tasks.filter_existing_qdrant_points_by_ids", + ) + mock_sentry = mocker.patch("vector_search.tasks.sentry_sdk.capture_message") + + embeddings_healthcheck() + assert mock_sentry.call_count == 0 + + +def test_embeddings_healthcheck_summaries_scoped_to_require_summaries(mocker): + """ + embeddings_healthcheck should only count missing summaries for learning + resources that require them, not every learning resource (regression + test for get_unprocessed_content_file_ids never receiving + learning_resource_ids) + """ + content_extension = [".srt"] + content_type = ["file"] + platform = LearningResourcePlatformFactory.create() + ContentSummarizerConfigurationFactory.create( + allowed_extensions=content_extension, + allowed_content_types=content_type, + is_active=True, + llm_model="test", + platform__code=platform.code, + ) + resource = LearningResourceFactory.create( + published=True, require_summaries=False, platform=platform + ) + resource.runs.all().delete() + learning_resource_run = LearningResourceRunFactory.create( + published=True, + learning_resource=resource, + ) + learning_resource_run.learning_resource = resource + learning_resource_run.save() + + ContentFileFactory.create( + published=True, + content="test", + file_extension=content_extension[0], + summary="", + content_type=content_type[0], + run=learning_resource_run, + ) + mocker.patch( + "vector_search.tasks.filter_existing_qdrant_points_by_ids", + ) + mock_sentry = mocker.patch("vector_search.tasks.sentry_sdk.capture_message") + + embeddings_healthcheck() + assert mock_sentry.call_count == 0 + + def test_generate_embeddings_retries_on_deadline(mocker): """A deadline with retry budget left calls self.retry (jittered backoff).""" mocker.patch( diff --git a/vector_search/utils.py b/vector_search/utils.py index 67aab4ee5f..0665a2dfc9 100644 --- a/vector_search/utils.py +++ b/vector_search/utils.py @@ -637,17 +637,29 @@ def _retrieve_content_file_point( return None -def _content_file_stored_checksum_changed(serialized_document: dict) -> bool: - point = _retrieve_content_file_point(serialized_document) - if not point: - return False - stored_checksum = (point.payload or {}).get("checksum") - # Missing checksums should not force an expensive summary rewrite by themselves. - # should_generate_content_embeddings still treats them as changed so embeddings - # can repair older Qdrant points without overwriting existing summaries. - return stored_checksum is not None and stored_checksum != serialized_document.get( - "checksum" - ) +def _stored_content_payloads( + point_ids: list[str], fields: tuple[str, ...] = ("checksum",) +) -> dict[str, dict]: + """ + Batch-retrieve stored payload fields for content-file points. + + Returns {point_id: partial payload dict} for points that exist in Qdrant; + absent points are absent from the map. One lookup per batch replaces the + per-file retrieves for the existence filter, summary-change check, and + embed gate. + """ + client = qdrant_client() + stored = {} + for id_batch in chunks( + point_ids, chunk_size=settings.QDRANT_POINT_UPLOAD_BATCH_SIZE + ): + for record in client.retrieve( + collection_name=CONTENT_FILES_COLLECTION_NAME, + ids=id_batch, + with_payload=list(fields), + ): + stored[record.id] = record.payload or {} + return stored def should_generate_content_embeddings( @@ -743,9 +755,13 @@ def _embed_course_metadata_as_contentfile(serialized_resources): client.upload_points(CONTENT_FILES_COLLECTION_NAME, points=points, wait=False) -def _generate_content_file_points(serialized_content): +def _generate_content_file_points(serialized_content, stored_payloads): """ - Chunk and embed content file documents, yielding PointStructs + Chunk and embed content file documents, yielding PointStructs. + + stored_payloads maps chunk-0 point ids to stored Qdrant payload fields + (see _stored_content_payloads); docs whose stored checksum matches get a + payload-only refresh instead of re-embedding. """ encoder_dense = dense_encoder() encoder_sparse = sparse_encoder() @@ -771,7 +787,17 @@ def _generate_content_file_points(serialized_content): embedding_context = _content_file_embedding_context(doc) if not embedding_context: continue - should_generate = should_generate_content_embeddings(doc) + # Point ids are content-key-derived and stable, so recompute per doc; + # summarization replaces the doc dicts between here and process_batch. + point_id = vector_point_id( + vector_point_key(doc, chunk_number=0, document_type="content_file") + ) + # Missing point or differing/missing stored checksum -> regenerate + # (self-heals failed or purged points on the next load). + should_generate = ( + point_id not in stored_payloads + or stored_payloads[point_id].get("checksum") != doc["checksum"] + ) if not should_generate: """ Just update the payload and continue @@ -947,7 +973,6 @@ def process_batch(docs_batch): fill_summary_content_ids = [] changed_summary_content_ids = [] - # Collect IDs for summarization contentfile_points = [ ( vector_point_id( @@ -959,17 +984,23 @@ def process_batch(docs_batch): ) for doc in docs_batch ] + # One batched lookup serves the existence filter, the summary-change + # check, and the embed gate in _generate_content_file_points. + stored_payloads = _stored_content_payloads( + [point[0] for point in contentfile_points] + ) if not overwrite: - filtered_point_ids = filter_existing_qdrant_points_by_ids( - [point[0] for point in contentfile_points], - collection_name=collection_name, - ) docs_batch = [ - point[1] + doc + for point_id, doc in contentfile_points + if point_id not in stored_payloads + ] + contentfile_points = [ + point for point in contentfile_points - if point[0] in filtered_point_ids + if point[0] not in stored_payloads ] - for resource in docs_batch: + for point_id, resource in contentfile_points: if ( resource.get("summary") or resource.get("require_summaries") @@ -977,7 +1008,15 @@ def process_batch(docs_batch): .filter(run__id=resource.get("run_id")) .exists() ): - if overwrite and _content_file_stored_checksum_changed(resource): + stored_checksum = stored_payloads.get(point_id, {}).get("checksum") + # A missing point or missing stored checksum must not force + # an expensive summary rewrite by itself; the embed gate + # still regenerates embeddings for those. + if ( + overwrite + and stored_checksum is not None + and stored_checksum != resource.get("checksum") + ): changed_summary_content_ids.append(resource["id"]) else: fill_summary_content_ids.append(resource["id"]) @@ -988,7 +1027,9 @@ def process_batch(docs_batch): changed_summary_content_ids, ) - points_generator_iter = _generate_content_file_points(docs_batch) + points_generator_iter = _generate_content_file_points( + docs_batch, stored_payloads + ) points_upload_batch = [] for point in points_generator_iter: diff --git a/vector_search/utils_test.py b/vector_search/utils_test.py index 28ed8b5226..af5a9cb346 100644 --- a/vector_search/utils_test.py +++ b/vector_search/utils_test.py @@ -226,16 +226,16 @@ def test_embed_learning_resources_no_overwrite(mocker, content_type): ], ) else: - # all contentfiles exist in qdrant - mocker.patch( - "vector_search.utils.filter_existing_qdrant_points_by_ids", - return_value=[ - vector_point_id( - f"{doc['platform']['code']}.{doc['resource_readable_id']}.{doc['run_readable_id']}.{doc['key']}.0" - ) - for doc in serialize_bulk_content_files([r.id for r in resources[0:3]]) - ], - ) + # the last 2 contentfiles already have points in qdrant; the first 3 don't + mock_qdrant.retrieve.return_value = [ + mocker.MagicMock( + id=vector_point_id( + vector_point_key(doc, chunk_number=0, document_type="content_file") + ), + payload={"checksum": doc["checksum"]}, + ) + for doc in serialize_bulk_content_files([r.id for r in resources[3:5]]) + ] mocker.patch( "learning_resources.content_summarizer.ContentSummarizer.summarize_content_files_by_ids" ) @@ -770,9 +770,6 @@ def test_generate_content_points_uses_markdown_chunking_for_marketing_pages(mock return_value=[Document(page_content="chunk1", metadata={"key": "k1"})], ) mock_chunk = mocker.patch("vector_search.utils._chunk_documents") - mocker.patch( - "vector_search.utils.should_generate_content_embeddings", return_value=True - ) mocker.patch("vector_search.utils.remove_points_matching_params") mock_dense = mocker.MagicMock() @@ -794,7 +791,7 @@ def test_generate_content_points_uses_markdown_chunking_for_marketing_pages(mock "key": "k1", } - list(_generate_content_file_points([doc])) + list(_generate_content_file_points([doc], {})) mock_md_chunk.assert_called_once() mock_chunk.assert_not_called() @@ -810,9 +807,6 @@ def test_generate_content_points_uses_standard_chunking_for_non_markdown(mocker) "vector_search.utils._chunk_documents", return_value=[Document(page_content="chunk1", metadata={"key": "k1"})], ) - mocker.patch( - "vector_search.utils.should_generate_content_embeddings", return_value=True - ) mocker.patch("vector_search.utils.remove_points_matching_params") mock_dense = mocker.MagicMock() @@ -834,7 +828,7 @@ def test_generate_content_points_uses_standard_chunking_for_non_markdown(mocker) "key": "k1", } - list(_generate_content_file_points([doc])) + list(_generate_content_file_points([doc], {})) mock_chunk.assert_called_once() mock_md_chunk.assert_not_called() @@ -857,9 +851,6 @@ def test_generate_content_points_leaves_headroom_under_token_limit(mocker): for i in range(num_chunks) ], ) - mocker.patch( - "vector_search.utils.should_generate_content_embeddings", return_value=True - ) mocker.patch("vector_search.utils.remove_points_matching_params") mock_dense = mocker.MagicMock() @@ -881,7 +872,7 @@ def test_generate_content_points_leaves_headroom_under_token_limit(mocker): "key": "k1", } - points = list(_generate_content_file_points([doc])) + points = list(_generate_content_file_points([doc], {})) batch_sizes = [ len(call.args[0]) for call in mock_dense.embed_documents.call_args_list @@ -906,9 +897,6 @@ def test_generate_content_points_request_chunk_size_never_zero(mocker): Document(page_content=f"chunk{i}", metadata={"key": "k1"}) for i in range(3) ], ) - mocker.patch( - "vector_search.utils.should_generate_content_embeddings", return_value=True - ) mocker.patch("vector_search.utils.remove_points_matching_params") mock_dense = mocker.MagicMock() @@ -930,7 +918,7 @@ def test_generate_content_points_request_chunk_size_never_zero(mocker): "key": "k1", } - points = list(_generate_content_file_points([doc])) + points = list(_generate_content_file_points([doc], {})) assert len(points) == 3 @@ -1127,43 +1115,81 @@ def test_should_generate_for_changed_content_file(mocker): assert result is True +def test_stored_content_payloads_batches_and_maps(mocker, settings): + """One retrieve per id-chunk; existing points map to their stored payload.""" + settings.QDRANT_POINT_UPLOAD_BATCH_SIZE = 2 + present = mocker.MagicMock(id="p1", payload={"checksum": "abc"}) + no_checksum = mocker.MagicMock(id="p2", payload={}) + mock_qdrant = mocker.MagicMock() + # p3 does not exist in Qdrant + mock_qdrant.retrieve.side_effect = [[present, no_checksum], []] + mocker.patch("vector_search.utils.qdrant_client", return_value=mock_qdrant) + + stored = vs_utils._stored_content_payloads( # noqa: SLF001 + ["p1", "p2", "p3"], fields=("checksum", "title") + ) + + assert stored == {"p1": {"checksum": "abc"}, "p2": {}} + assert mock_qdrant.retrieve.call_count == 2 # ceil(3 ids / batch size 2) + for call in mock_qdrant.retrieve.call_args_list: + assert call.kwargs["collection_name"] == CONTENT_FILES_COLLECTION_NAME + assert call.kwargs["with_payload"] == ["checksum", "title"] + + @pytest.mark.parametrize( - ("stored_payload", "expected"), + ("stored_entry", "expect_regenerate"), [ - ({"checksum": "previous-checksum"}, True), - ({"checksum": "current-checksum"}, False), - ({}, False), - (None, False), + ("missing", True), # no point in Qdrant (new file or failed prior embed) + (None, True), # point exists but has no stored checksum + ("stale-checksum", True), # stored checksum differs + ("current-checksum", False), # matches -> payload-only update ], -) -def test_content_file_stored_checksum_changed(mocker, stored_payload, expected): - """Only an existing, different stored checksum counts as changed for summaries.""" - serialized_document = { - "resource_readable_id": "resource-1", - "run_readable_id": "run-1", - "key": "transcript.txt", - "checksum": "current-checksum", - } - mock_qdrant = mocker.MagicMock() - if stored_payload is None: - mock_qdrant.retrieve.return_value = [] - else: - mock_point = mocker.MagicMock() - mock_point.payload = stored_payload - mock_qdrant.retrieve.return_value = [mock_point] - mocker.patch("vector_search.utils.qdrant_client", return_value=mock_qdrant) +) # stored_entry is the checksum in the stored payload dict +def test_generate_content_points_checksum_gate(mocker, stored_entry, expect_regenerate): + """Docs are re-embedded unless their stored Qdrant checksum matches.""" + settings.CONTENT_FILE_EMBEDDING_CHUNK_SIZE_OVERRIDE = 500 + settings.CONTENT_FILE_EMBEDDING_CHUNK_OVERLAP = 50 - assert ( - vs_utils._content_file_stored_checksum_changed( # noqa: SLF001 - serialized_document - ) - is expected + mocker.patch( + "vector_search.utils._chunk_documents", + return_value=[Document(page_content="chunk1", metadata={"key": "k1"})], ) - mock_qdrant.retrieve.assert_called_once() - assert ( - mock_qdrant.retrieve.call_args.kwargs["collection_name"] - == CONTENT_FILES_COLLECTION_NAME + mocker.patch("vector_search.utils.remove_points_matching_params") + update_payload_mock = mocker.patch( + "vector_search.utils.update_content_file_payload" + ) + mock_dense = mocker.MagicMock() + mock_dense.embed_documents.side_effect = lambda texts: [[0.1] for _ in texts] + mock_dense.model_short_name.return_value = "dense" + mock_sparse = mocker.MagicMock() + mock_sparse.embed_documents.side_effect = lambda texts: [[0.2] for _ in texts] + mock_sparse.model_short_name.return_value = "sparse" + mocker.patch("vector_search.utils.dense_encoder", return_value=mock_dense) + mocker.patch("vector_search.utils.sparse_encoder", return_value=mock_sparse) + + doc = { + "content": "Some plain text content", + "file_type": "page", + "file_extension": ".html", + "platform": {"code": "x"}, + "resource_readable_id": "r1", + "run_readable_id": "run1", + "key": "k1", + "checksum": "current-checksum", + } + point_id = vector_point_id( + vector_point_key(doc, chunk_number=0, document_type="content_file") ) + stored = {} if stored_entry == "missing" else {point_id: {"checksum": stored_entry}} + + points = list(_generate_content_file_points([doc], stored)) + + if expect_regenerate: + assert len(points) == 1 + update_payload_mock.assert_not_called() + else: + assert points == [] + update_payload_mock.assert_called_once_with(doc) def test_should_not_generate_for_unchanged_content_file(mocker): @@ -1246,11 +1272,9 @@ def test_embed_learning_resources_summarizes_only_contentfiles_with_summary(mock Test that embedding overwrites don't overwrite existing summaries. """ mock_qdrant = mocker.patch("qdrant_client.QdrantClient") + mock_qdrant.retrieve.return_value = [] mocker.patch("vector_search.utils.qdrant_client", return_value=mock_qdrant) mocker.patch("vector_search.utils.create_qdrant_collections") - mocker.patch( - "vector_search.utils.filter_existing_qdrant_points_by_ids", return_value=[] - ) mocker.patch("vector_search.utils.remove_qdrant_records") learning_resource = LearningResourceFactory.create( @@ -1285,9 +1309,6 @@ def test_embed_learning_resources_summarizes_only_contentfiles_with_summary(mock mocker.patch( "vector_search.utils.serialize_bulk_content_files", return_value=serialized ) - mocker.patch( - "vector_search.utils._content_file_stored_checksum_changed", return_value=False - ) summarize_mock = mocker.patch( "learning_resources.content_summarizer.ContentSummarizer.summarize_content_files_by_ids" @@ -1333,7 +1354,7 @@ def test_embed_learning_resources_overwrites_summaries_for_changed_content(mocke "key": cf.key, "summary": cf.summary, "content": cf.content, - "checksum": cf.checksum, + "checksum": f"current-{cf.id}", } for cf in all_contentfiles ] @@ -1341,10 +1362,22 @@ def test_embed_learning_resources_overwrites_summaries_for_changed_content(mocke mocker.patch( "vector_search.utils.serialize_bulk_content_files", return_value=serialized ) - mocker.patch( - "vector_search.utils._content_file_stored_checksum_changed", - side_effect=lambda resource: resource["id"] == changed_content_file.id, - ) + # The unchanged file takes the payload-only path (covered by its own tests) + mocker.patch("vector_search.utils.update_content_file_payload") + # Stored Qdrant checksum matches for the unchanged file, differs for the changed + mock_qdrant.retrieve.return_value = [ + mocker.MagicMock( + id=vector_point_id( + vector_point_key(doc, chunk_number=0, document_type="content_file") + ), + payload={ + "checksum": doc["checksum"] + if doc["id"] == unchanged_content_file.id + else "stale-checksum" + }, + ) + for doc in serialized + ] summarize_mock = mocker.patch( "learning_resources.content_summarizer.ContentSummarizer.summarize_content_files_by_ids" @@ -1392,15 +1425,23 @@ def test_embed_learning_resources_keeps_old_checksum_when_summary_fails(mocker): "key": content_file.key, "summary": content_file.summary, "content": content_file.content, - "checksum": content_file.checksum, + "checksum": "current-checksum", } ] mocker.patch( "vector_search.utils.serialize_bulk_content_files", return_value=serialized ) - mocker.patch( - "vector_search.utils._content_file_stored_checksum_changed", return_value=True - ) + # Stored Qdrant checksum differs, so the summary must be regenerated + mock_qdrant.retrieve.return_value = [ + mocker.MagicMock( + id=vector_point_id( + vector_point_key( + serialized[0], chunk_number=0, document_type="content_file" + ) + ), + payload={"checksum": "previous-checksum"}, + ) + ] summarize_mock = mocker.patch( "learning_resources.content_summarizer.ContentSummarizer.summarize_content_files_by_ids", return_value=[ diff --git a/vector_search/views.py b/vector_search/views.py index 55ede51a5b..92d140bffd 100644 --- a/vector_search/views.py +++ b/vector_search/views.py @@ -485,10 +485,18 @@ async def _async_vector_counts( aggregation_keys = params.get("aggregations") or [] count_result, aggregations = await asyncio.gather( + # This total drives pagination, and Qdrant's approximate count + # overestimates a filtered collection -- which advertises pages + # that return no results. Exact counting fixes that but scales with + # the number of matched points (measured at ~60ms per million), so + # we only set exact=True for the resources collection (~1 point per + # resource). We do not set it for the contentfiles collection due to + # the number (millions) of points; its totals stay approximate, + # which is invisible while nothing paginates them. client.count( collection_name=search_collection, count_filter=search_filter, - exact=False, + exact=search_collection == RESOURCES_COLLECTION_NAME, ), async_qdrant_aggregations( aggregation_keys, diff --git a/vector_search/views_test.py b/vector_search/views_test.py index 3980f22857..1d1ec2e239 100644 --- a/vector_search/views_test.py +++ b/vector_search/views_test.py @@ -1214,3 +1214,45 @@ def test_content_file_vector_search_partial_invalid_ids_searches_survivors( id_conditions = [c for c in must if getattr(c, "key", None) == "edx_module_id"] assert len(id_conditions) == 1 assert list(id_conditions[0].match.any) == [valid_id] + + +@pytest.mark.django_db(transaction=True) +def test_vector_search_count_is_exact(client, mock_qdrant): + """The result total must be an exact count. + + Qdrant's approximate count overestimates filtered collections, which made + the paginator advertise pages that returned no results. + """ + response = client.get( + reverse("vector_search:v0:vector_learning_resources_search"), + data={ + "topic": "Art, Design & Architecture", + "resource_type_group": "learning_material", + }, + ) + + assert response.status_code == 200 + + mock_qdrant.count.assert_awaited() + assert mock_qdrant.count.await_args.kwargs["exact"] is True + + +@pytest.mark.django_db(transaction=True) +def test_content_file_vector_search_count_is_approximate( + client, mock_qdrant, content_file_viewer +): + """Content-file totals must stay approximate. + + Exact counting scales with matched points, and this collection holds + millions of chunks, so an exact count here would cost most of a second. + Nothing paginates content files, so the imprecision is invisible. + """ + response = client.get( + reverse("vector_search:v0:vector_content_files_search"), + data={"q": "test"}, + ) + + assert response.status_code == 200 + + mock_qdrant.count.assert_awaited() + assert mock_qdrant.count.await_args.kwargs["exact"] is False diff --git a/yarn.lock b/yarn.lock index 9e59ab972f..c4ae0dd72a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3380,9 +3380,9 @@ __metadata: languageName: node linkType: hard -"@mitodl/smoot-design@npm:^6.30.0": - version: 6.30.0 - resolution: "@mitodl/smoot-design@npm:6.30.0" +"@mitodl/smoot-design@npm:6.31.1": + version: 6.31.1 + resolution: "@mitodl/smoot-design@npm:6.31.1" dependencies: "@ai-sdk/react": "npm:1.2.12" "@emotion/cache": "npm:^11.14.0" @@ -3407,7 +3407,7 @@ __metadata: "@remixicon/react": ^4.2.0 react: ^18 || ^19 react-dom: ^18 || ^19 - checksum: 10/4a4093af6a48e7e29e566e24062454e0aee7d2399397220921923f46b5e9a639afb0cd4a01bce122566f4204c4bdec71bd46113b97c150089dc7256ad8cb86b7 + checksum: 10/539471ec1d8255d4ab0901cb0c5924f2ba9f4d5887f33347a50d8b81fb7a852103826f802a9e0ef56af821cb026fb4912eb804f9635df7cbbaaae066842c58a2 languageName: node linkType: hard @@ -16332,7 +16332,7 @@ __metadata: "@mitodl/course-search-utils": "npm:^3.5.2" "@mitodl/hacksnack": "npm:^0.1.0" "@mitodl/mitxonline-api-axios": "npm:2026.7.22" - "@mitodl/smoot-design": "npm:^6.30.0" + "@mitodl/smoot-design": "npm:6.31.1" "@mui/base": "npm:5.0.0-beta.70" "@mui/material": "npm:^6.4.5" "@mui/material-nextjs": "npm:^6.4.3" @@ -17982,7 +17982,7 @@ __metadata: typescript: "npm:^5.5.4" wheel-indicator: "npm:^1.3.0" peerDependencies: - "@mitodl/smoot-design": ^6.30.0 + "@mitodl/smoot-design": 6.31.1 next: ^16.2.7 languageName: unknown linkType: soft