diff --git a/RELEASE.rst b/RELEASE.rst index 8e267969ce..b88c9432ba 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,13 @@ Release Notes ============= +Version 0.50.2 +-------------- + +- More efficient key retrieval (#2804) +- fix dashboard card enrollment association and display (#2792) +- For published non-test_mode courses, only process contentfile archives of the best run (#2786) + Version 0.50.0 (Released December 11, 2025) -------------- diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardCard.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardCard.test.tsx index c3a7ea8697..ca79d429a2 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardCard.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardCard.test.tsx @@ -558,6 +558,7 @@ describe.each([ status: EnrollmentStatus.Completed, mode: EnrollmentMode.Verified, grades: [], + run: mitxonline.factories.courses.courseRun(), } renderWithProviders( , diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardCard.tsx index 69860b36b8..3c3c3f229a 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardCard.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardCard.tsx @@ -449,8 +449,8 @@ const UpgradeBanner: React.FC< } const CountdownRoot = styled.div(({ theme }) => ({ - width: "142px", - marginRight: "32px", + width: "100%", + paddingRight: "32px", display: "flex", justifyContent: "center", alignSelf: "end", @@ -539,6 +539,10 @@ const DashboardCard: React.FC = ({ const run = isCourse ? dashboardResource.run : undefined const coursewareId = isCourse ? dashboardResource.coursewareId : null const readableId = isCourse ? dashboardResource.readableId : null + const hasValidCertificate = isCourse ? !!run?.certificate?.link : false + const enrollmentStatus = hasValidCertificate + ? EnrollmentStatus.Completed + : enrollment?.status // Title link logic const coursewareUrl = run?.coursewareUrl @@ -599,9 +603,7 @@ const DashboardCard: React.FC = ({ ) : ( {title} )} - {isCourse && - enrollment?.status === EnrollmentStatus.Completed && - run?.certificate?.link ? ( + {isCourse && run?.certificate?.link ? ( View Certificate @@ -625,7 +627,7 @@ const DashboardCard: React.FC = ({ ) : isCourse ? ( <> = ({ coursewareId={coursewareId} readableId={readableId} startDate={run?.startDate} - enrollmentStatus={enrollment?.status} + enrollmentStatus={enrollmentStatus} href={buttonHref ?? run?.coursewareUrl} endDate={run?.endDate} noun={noun} diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrollmentDisplay.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrollmentDisplay.tsx index a169859164..5dbb21b56a 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrollmentDisplay.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrollmentDisplay.tsx @@ -17,6 +17,8 @@ import { mitxonlineCourse, mitxonlineProgram, programEnrollmentsToPrograms, + selectBestEnrollment, + transformEnrollmentToDashboard, userEnrollmentsToDashboardCourses, } from "./transform" import { DashboardCard } from "./DashboardCard" @@ -249,10 +251,9 @@ interface ProgramEnrollmentDisplayProps { const ProgramEnrollmentDisplay: React.FC = ({ programId, }) => { - const { data: userCourses, isLoading: userEnrollmentsLoading } = useQuery({ - ...enrollmentQueries.courseRunEnrollmentsList(), - select: userEnrollmentsToDashboardCourses, - }) + const { data: rawEnrollments, isLoading: userEnrollmentsLoading } = useQuery( + enrollmentQueries.courseRunEnrollmentsList(), + ) const { data: rawProgram, isLoading: programLoading } = useQuery( programsQueries.programDetail({ id: programId }), ) @@ -277,6 +278,19 @@ const ProgramEnrollmentDisplay: React.FC = ({ programEnrollmentsLoading || programCoursesLoading + // Group enrollments by course ID for efficient lookup + const enrollmentsByCourseId = (rawEnrollments || []).reduce( + (acc, enrollment) => { + const courseId = enrollment.run.course.id + if (!acc[courseId]) { + acc[courseId] = [] + } + acc[courseId].push(enrollment) + return acc + }, + {} as Record, + ) + // Build sections from requirement tree const requirementSections = program?.reqTree @@ -286,12 +300,30 @@ const ProgramEnrollmentDisplay: React.FC = ({ const sectionCourses = (rawProgramCourses?.results || []) .filter((course) => courseIds.includes(course.id)) .map((course) => { - const enrollment = userCourses?.find((dashboardCourse) => - course.courseruns.some( - (run) => run.courseware_id === dashboardCourse.coursewareId, - ), - )?.enrollment - return mitxonlineCourse(course, enrollment) + // Find all enrollments for this course + const courseEnrollments = enrollmentsByCourseId[course.id] || [] + + if (courseEnrollments.length === 0) { + // No enrollment - use first run + return mitxonlineCourse(course) + } + + // If multiple enrollments exist, select the best one + const bestEnrollment = + courseEnrollments.length > 1 + ? selectBestEnrollment(courseEnrollments) + : courseEnrollments[0] + + // Find the matching run from course.courseruns + const matchingRun = course.courseruns.find( + (run) => run.id === bestEnrollment.run.id, + ) + + return mitxonlineCourse( + course, + transformEnrollmentToDashboard(bestEnrollment), + matchingRun, + ) }) return { diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/test-utils.ts b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/test-utils.ts index 34ed599200..daf01f035e 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/test-utils.ts +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/test-utils.ts @@ -50,6 +50,7 @@ const dashboardCourse: PartialFactory = (...overrides) => { status: faker.helpers.arrayElement(Object.values(EnrollmentStatus)), mode: faker.helpers.arrayElement(Object.values(EnrollmentMode)), grades: [], + run: factories.courses.courseRun(), }, }, ...overrides, diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/transform.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/transform.test.tsx index d8b59c60e1..4a5296f2dd 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/transform.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/transform.test.tsx @@ -75,6 +75,7 @@ describe("Transforming mitxonline enrollment data to DashboardResource", () => { ) ?? "", }, grades: apiData.grades, + run: apiData.run, }, } satisfies DashboardResource) }, @@ -545,12 +546,13 @@ describe("Transforming mitxonline enrollment data to DashboardResource", () => { }) }) - test("selects enrollment with highest grade when multiple enrollments exist for same course", () => { + test("selects enrollment with highest grade when multiple enrollments exist for same course in same contract", () => { const orgId = faker.number.int() const contracts = createTestContracts(orgId, 1) const contractId = contracts[0].id // Create a course with 2 runs, both tied to the same contract + // (e.g., pilot run and soft launch run both in same contract) const course = factories.courses.course({ id: 123, title: "Test Course", @@ -574,6 +576,7 @@ describe("Transforming mitxonline enrollment data to DashboardResource", () => { grades: [factories.enrollment.grade({ grade: 0.65, passed: true })], b2b_contract_id: contractId, b2b_organization_id: orgId, + certificate: null, }) const enrollmentHighGrade = factories.enrollment.courseEnrollment({ @@ -584,6 +587,7 @@ describe("Transforming mitxonline enrollment data to DashboardResource", () => { grades: [factories.enrollment.grade({ grade: 0.95, passed: true })], b2b_contract_id: contractId, b2b_organization_id: orgId, + certificate: null, }) const transformedCourses = organizationCoursesWithContracts({ @@ -601,6 +605,69 @@ describe("Transforming mitxonline enrollment data to DashboardResource", () => { expect(transformedCourse.enrollment?.grades[0].grade).toBe(0.95) expect(transformedCourse.enrollment?.id).toBe(enrollmentHighGrade.id) }) + + test("prioritizes enrollment with certificate over grade when multiple enrollments exist", () => { + const orgId = faker.number.int() + const contracts = createTestContracts(orgId, 1) + const contractId = contracts[0].id + + const course = factories.courses.course({ + id: 123, + title: "Test Course", + }) + const run1 = factories.courses.courseRun({ + id: 1, + b2b_contract: contractId, + }) + const run2 = factories.courses.courseRun({ + id: 2, + b2b_contract: contractId, + }) + course.courseruns = [run1, run2] + + // Enrollment with higher grade but no certificate + const enrollmentHighGradeNoCert = factories.enrollment.courseEnrollment({ + run: { + id: run1.id, + course: { id: course.id, title: course.title }, + }, + grades: [factories.enrollment.grade({ grade: 0.95, passed: true })], + b2b_contract_id: contractId, + b2b_organization_id: orgId, + certificate: null, + }) + + // Enrollment with lower/no grade but has certificate + const enrollmentWithCert = factories.enrollment.courseEnrollment({ + run: { + id: run2.id, + course: { id: course.id, title: course.title }, + }, + grades: [], + b2b_contract_id: contractId, + b2b_organization_id: orgId, + certificate: { + uuid: "test-cert-uuid", + link: "/certificate/test-link/", + }, + }) + + const transformedCourses = organizationCoursesWithContracts({ + courses: [course], + contracts, + enrollments: [enrollmentHighGradeNoCert, enrollmentWithCert], + }) + + expect(transformedCourses).toHaveLength(1) + const transformedCourse = transformedCourses[0] + + // Should select the enrollment with the certificate, even though it has no grade + expect(transformedCourse.enrollment).toBeDefined() + expect(transformedCourse.enrollment?.id).toBe(enrollmentWithCert.id) + expect(transformedCourse.enrollment?.certificate?.uuid).toBe( + "test-cert-uuid", + ) + }) }) describe("transformEnrollmentToDashboard", () => { @@ -629,6 +696,7 @@ describe("Transforming mitxonline enrollment data to DashboardResource", () => { link: "/certificate/course/test123/", }, grades: enrollment.grades, + run: enrollment.run, }) }) diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/transform.ts b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/transform.ts index 876683f7a6..4d6e1da568 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/transform.ts +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/transform.ts @@ -45,27 +45,29 @@ const getKey = ({ source, resourceType, id, runId }: KeyOpts) => { const mitxonlineCourse = ( raw: CourseWithCourseRunsSerializerV2, enrollment?: DashboardCourseEnrollment | undefined, + run?: CourseWithCourseRunsSerializerV2["courseruns"][number] | undefined, ): DashboardCourse => { - const run = raw.courseruns[0] + // Use provided run, or fall back to enrollment's run, or first run in course + const courseRun = run ?? (enrollment?.run || raw.courseruns[0]) const transformedCourse = { key: getKey({ source: sources.mitxonline, resourceType: DashboardResourceType.Course, id: raw.id, - runId: run?.id, + runId: courseRun?.id, }), - coursewareId: run?.courseware_id ?? null, + coursewareId: courseRun?.courseware_id ?? null, readableId: raw.readable_id ?? null, type: DashboardResourceType.Course, title: raw.title, marketingUrl: raw.page?.page_url, run: { - startDate: run?.start_date, - endDate: run?.end_date, - certificateUpgradeDeadline: run?.upgrade_deadline, - certificateUpgradePrice: run?.products[0]?.price, - coursewareUrl: run?.courseware_url, - canUpgrade: !!run?.is_upgradable, + startDate: courseRun?.start_date, + endDate: courseRun?.end_date, + certificateUpgradeDeadline: courseRun?.upgrade_deadline, + certificateUpgradePrice: courseRun?.products[0]?.price, + coursewareUrl: courseRun?.courseware_url, + canUpgrade: !!courseRun?.is_upgradable, }, } as DashboardCourse @@ -100,6 +102,7 @@ const transformEnrollmentToDashboard = ( ) ?? "", }, grades: raw.grades, + run: raw.run, } } @@ -210,6 +213,31 @@ const createOrgUnenrolledCourse = ( } } +/** + * Selects the best enrollment from multiple enrollments for the same course. + * Priority: + * 1. Prefer enrollment with a certificate + * 2. If tied, prefer highest grade + * 3. Otherwise take first match + */ +const selectBestEnrollment = ( + enrollments: CourseRunEnrollmentRequestV2[], +): CourseRunEnrollmentRequestV2 => { + return enrollments.reduce((best, current) => { + const bestHasCert = !!best.certificate?.uuid + const currentHasCert = !!current.certificate?.uuid + + // Prioritize having a certificate + if (currentHasCert && !bestHasCert) return current + if (bestHasCert && !currentHasCert) return best + + // If both have or don't have certificates, compare grades + const bestGrade = Math.max(0, ...best.grades.map((g) => g.grade ?? 0)) + const currentGrade = Math.max(0, ...current.grades.map((g) => g.grade ?? 0)) + return currentGrade > bestGrade ? current : best + }, enrollments[0]) +} + const organizationCoursesWithContracts = (raw: { courses: CourseWithCourseRunsSerializerV2[] contracts?: ContractPage[] // Make optional @@ -227,35 +255,46 @@ const organizationCoursesWithContracts = (raw: { const enrollments = enrollmentsByCourseId[course.id] if (enrollments?.length > 0) { - const transformedEnrollments = - enrollmentsToOrgDashboardEnrollments(enrollments) if (raw.contracts && raw.contracts.length > 0) { // Filter enrollments to only include those with valid contracts - const validEnrollments = transformedEnrollments.filter((enrollment) => { + const contractEnrollments = enrollments.filter((enrollment) => { const courseRunContractId = enrollment.b2b_contract_id return ( courseRunContractId && contractIds.includes(courseRunContractId) ) }) - if (validEnrollments.length > 0) { - // Select enrollment with highest grade - const bestEnrollment = validEnrollments.reduce((best, current) => { - // Get the highest grade from each enrollment's grades array - const bestGrade = Math.max( - 0, - ...best.grades.map((g) => g.grade ?? 0), - ) - const currentGrade = Math.max( - 0, - ...current.grades.map((g) => g.grade ?? 0), - ) + if (contractEnrollments.length > 0) { + // Find all enrollments that match runs in this course + // Match by run ID AND verify the contract matches + const matchingEnrollments = contractEnrollments.filter( + (enrollment) => { + const matchingRun = course.courseruns.find( + (run) => run.id === enrollment.run.id, + ) + return ( + matchingRun && + matchingRun.b2b_contract === enrollment.b2b_contract_id + ) + }, + ) - return currentGrade > bestGrade ? current : best - }, validEnrollments[0]) + if (matchingEnrollments.length > 0) { + // If multiple enrollments exist (e.g., user enrolled in multiple runs + // of the same course within the same contract), select the best one + const bestEnrollment = selectBestEnrollment(matchingEnrollments) - const dashboardCourse = mitxonlineCourse(course, bestEnrollment) - return dashboardCourse + // Find the corresponding run from course.courseruns + const matchingRun = course.courseruns.find( + (run) => run.id === bestEnrollment.run.id, + ) + const dashboardCourse = mitxonlineCourse( + course, + transformEnrollmentToDashboard(bestEnrollment), + matchingRun, + ) + return dashboardCourse + } } // If contracts are provided but a matching one isn't found, treat it as unenrolled @@ -268,9 +307,13 @@ const organizationCoursesWithContracts = (raw: { ?.id === enrollment.run.id, ) if (matchingEnrollment) { + const matchingRun = course.courseruns.find( + (run) => run.id === matchingEnrollment.run.id, + ) return mitxonlineCourse( course, transformEnrollmentToDashboard(matchingEnrollment), + matchingRun, ) } } @@ -362,4 +405,5 @@ export { mitxonlineProgramCollection, sortDashboardCourses, filterEnrollmentsByOrganization, + selectBestEnrollment, } diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/types.ts b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/types.ts index 7f32248db0..8424029008 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/types.ts +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/types.ts @@ -1,6 +1,7 @@ import { V2ProgramRequirement, CourseRunGrade, + CourseRunV2, } from "@mitodl/mitxonline-api-axios/v2" const DashboardResourceType = { @@ -71,6 +72,7 @@ type DashboardCourseEnrollment = { link: string } grades: CourseRunGrade[] + run: CourseRunV2 } type DashboardProgramEnrollment = { diff --git a/frontends/main/src/app-pages/DashboardPage/OrganizationContent.test.tsx b/frontends/main/src/app-pages/DashboardPage/OrganizationContent.test.tsx index 74e9eda943..62591b658b 100644 --- a/frontends/main/src/app-pages/DashboardPage/OrganizationContent.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/OrganizationContent.test.tsx @@ -16,7 +16,6 @@ import { } from "./CoursewareDisplay/transform" import { createCoursesWithContractRuns, - createEnrollmentsForContractRuns, createTestContracts, setupOrgAndUser, setupProgramsAndCourses, @@ -82,8 +81,12 @@ describe("OrganizationContent", () => { renderWithProviders() - const programElement = await screen.findByTestId("org-program-root") - const cards = await within(programElement).findAllByTestId( + const programElements = await screen.findAllByTestId("org-program-root") + // Find the program with programA's title + const programAElement = + programElements.find((el) => el.textContent?.includes(programA.title)) || + programElements[0] + const cards = await within(programAElement).findAllByTestId( "enrollment-card-desktop", ) @@ -507,28 +510,11 @@ describe("OrganizationContent", () => { test("displays only courses with contract-scoped runs", async () => { const { orgX, user, mitxOnlineUser } = setupOrgAndUser() const contracts = createTestContracts(orgX.id, 1) - const courses = createCoursesWithContractRuns(contracts).map((course) => ({ - ...course, - courseruns: course.courseruns.map((run) => { - if (run.b2b_contract === contracts[0].id) { - return { - ...run, - start_date: faker.date.past().toISOString(), // Make it started - } - } - return run - }), - })) + const courses = createCoursesWithContractRuns(contracts) const program = factories.programs.program({ courses: courses.map((c) => c.id), }) - // Create enrollments so the button will have href - const enrollments = createEnrollmentsForContractRuns(courses, [ - contracts[0].id, - ]) - - // Setup API mocks setupOrgDashboardMocks( orgX, user, @@ -538,23 +524,19 @@ describe("OrganizationContent", () => { contracts, ) - // Override enrollments for this test - setMockResponse.get(urls.enrollment.enrollmentsList(), enrollments) - setMockResponse.get(urls.enrollment.enrollmentsListV2(), enrollments) - renderWithProviders() // Wait for programs to load const programElements = await screen.findAllByTestId("org-program-root") expect(programElements).toHaveLength(1) - // Verify courses are displayed with correct run information + // Verify courses are displayed const cards = await within(programElements[0]).findAllByTestId( "enrollment-card-desktop", ) expect(cards.length).toBeGreaterThan(0) - // Check that each card shows course information from contract-scoped run + // Verify each course card displays the course and has a contract-scoped run cards.forEach((card, index) => { const course = courses[index] const contractRun = course.courseruns.find( @@ -563,32 +545,8 @@ describe("OrganizationContent", () => { ) expect(card).toHaveTextContent(course.title) - - // Verify we're using the contract-scoped run, not the other runs expect(contractRun).toBeDefined() expect(contractRun?.b2b_contract).toBe(contracts[0].id) - - // Check that the card displays information from the correct course run - const coursewareButton = within(card).getByTestId("courseware-button") - - // The courseware button shows "Continue" for enrolled users in started courses - expect(coursewareButton).toHaveTextContent("Continue") - - // Verify the courseware button has the correct href from the contract run - // Only check href if the course has started and user is enrolled - if ( - contractRun?.courseware_url && - new Date(contractRun.start_date) <= new Date() - ) { - expect(coursewareButton).toHaveAttribute( - "href", - contractRun.courseware_url, - ) - } - - // Check for enrollment status indicator showing "Enrolled" since we created enrollments - const enrollmentStatus = within(card).getByTestId("enrollment-status") - expect(enrollmentStatus).toHaveTextContent("Enrolled") }) }) @@ -974,4 +932,73 @@ describe("OrganizationContent", () => { expect(screen.getByText("First extra content")).toBeInTheDocument() expect(screen.queryByText("Second extra content")).toBeNull() }) + + test("displays correct run URL when user is enrolled in one of multiple runs", async () => { + const { orgX, user, mitxOnlineUser } = setupOrgAndUser() + const contracts = createTestContracts(orgX.id, 1) + + // Create a course with 3 different runs with distinct URLs + const course = factories.courses.course() + const runs = [ + factories.courses.courseRun({ + b2b_contract: contracts[0].id, + courseware_url: "https://openedx.example.com/course-run-1", + start_date: faker.date.past().toISOString(), + }), + factories.courses.courseRun({ + b2b_contract: contracts[0].id, + courseware_url: "https://openedx.example.com/course-run-2", + start_date: faker.date.past().toISOString(), + }), + factories.courses.courseRun({ + b2b_contract: contracts[0].id, + courseware_url: "https://openedx.example.com/course-run-3", + start_date: faker.date.past().toISOString(), + }), + ] + + const courseWithMultipleRuns = { + ...course, + courseruns: runs, + } + + // Randomly pick one of the runs to enroll in + const enrolledRun = faker.helpers.arrayElement(runs) + + const enrollment = factories.enrollment.courseEnrollment({ + run: { + id: enrolledRun.id, + course: { id: course.id, title: course.title }, + }, + b2b_contract_id: contracts[0].id, + grades: [], + }) + + const program = factories.programs.program({ + courses: [course.id], + }) + + setupOrgDashboardMocks( + orgX, + user, + mitxOnlineUser, + [program], + [courseWithMultipleRuns], + contracts, + ) + + setMockResponse.get(urls.enrollment.enrollmentsList(), [enrollment]) + setMockResponse.get(urls.enrollment.enrollmentsListV2(), [enrollment]) + + renderWithProviders() + + const programElement = await screen.findByTestId("org-program-root") + const card = await within(programElement).findByTestId( + "enrollment-card-desktop", + ) + + // Verify the courseware button has the correct href from the enrolled run + const coursewareButton = within(card).getByTestId("courseware-button") + expect(coursewareButton).toHaveAttribute("href", enrolledRun.courseware_url) + }) }) diff --git a/frontends/main/src/app-pages/DashboardPage/OrganizationContent.tsx b/frontends/main/src/app-pages/DashboardPage/OrganizationContent.tsx index e5a8dfae9b..ec75144caa 100644 --- a/frontends/main/src/app-pages/DashboardPage/OrganizationContent.tsx +++ b/frontends/main/src/app-pages/DashboardPage/OrganizationContent.tsx @@ -347,7 +347,8 @@ const OrgProgramDisplay: React.FC<{ const skeleton = ( ) - if (programLoading || courses.isLoading) return skeleton + + const sanitizedHtml = DOMPurify.sanitize(program.description) const rawCourses = courses.data?.results.sort((a, b) => { return program.courseIds.indexOf(a.id) - program.courseIds.indexOf(b.id) @@ -357,7 +358,6 @@ const OrgProgramDisplay: React.FC<{ contracts: contracts ?? [], enrollments: courseRunEnrollments ?? [], }) - const sanitizedHtml = DOMPurify.sanitize(program.description) return ( @@ -383,17 +383,19 @@ const OrgProgramDisplay: React.FC<{ )} - {transformedCourses.map((course) => ( - - ))} + {programLoading || courses.isLoading + ? skeleton + : transformedCourses.map((course) => ( + + ))} ) @@ -429,8 +431,15 @@ const OrganizationContentInternal: React.FC< programCollectionQueries.programCollectionsList({}), ) + // Get IDs of all programs that are in collections + const programsInCollections = new Set( + programCollections.data?.results.flatMap((collection) => + collection.programs.map((p) => p.id), + ) ?? [], + ) + const transformedPrograms = programs.data?.results - .filter((program) => program.collections.length === 0) + .filter((program) => !programsInCollections.has(program.id)) .filter((program) => { if (!orgContract?.programs || orgContract.programs.length === 0) { return true @@ -453,6 +462,24 @@ const OrganizationContentInternal: React.FC< ) + // Wait for all top-level queries to complete before rendering content + if ( + programs.isLoading || + programCollections.isLoading || + courseRunEnrollments.isLoading || + programEnrollments.isLoading + ) { + return ( + <> + + + + + {skeleton} + + ) + } + return ( <> @@ -460,7 +487,7 @@ const OrganizationContentInternal: React.FC< - {programs.isLoading || !transformedPrograms + {!transformedPrograms ? skeleton : transformedPrograms.map((program) => ( ))} - {programCollections.isLoading ? ( - skeleton - ) : ( - - {programCollections.data?.results.map((collection) => { + + {(programCollections.data?.results ?? []) + .filter((collection) => { + // Only show collections where at least one program is in the contract + const collectionProgramIds = collection.programs.map((p) => p.id) + if (!orgContract?.programs || orgContract.programs.length === 0) { + return collectionProgramIds.length > 0 + } + return collectionProgramIds.some( + (id) => id !== undefined && orgContract.programs.includes(id), + ) + }) + .map((collection) => { const transformedCollection = transform.mitxonlineProgramCollection(collection) return ( @@ -490,8 +525,7 @@ const OrganizationContentInternal: React.FC< /> ) })} - - )} + {programs.data?.results.length === 0 && ( diff --git a/learning_resources/etl/edx_shared.py b/learning_resources/etl/edx_shared.py index d45f84b96e..c8bca35b10 100644 --- a/learning_resources/etl/edx_shared.py +++ b/learning_resources/etl/edx_shared.py @@ -22,6 +22,44 @@ EDX_S3_BASE_PREFIX = "20" +def process_course_archive( + bucket, key: str, run: LearningResourceRun, *, overwrite: bool = False +) -> None: + """ + Download and process a course archive from S3. + + Args: + bucket: S3 bucket object + key(str): S3 object key for the course archive + run(LearningResourceRun): The run to process + overwrite(bool): Whether to overwrite existing content files + + Returns: + bool: True if successfully processed, False if skipped due to matching checksum + """ + with TemporaryDirectory() as export_tempdir: + course_tarpath = Path(export_tempdir, key.split("/")[-1]) + log.info("course tarpath for run %s is %s", run.run_id, course_tarpath) + bucket.download_file(key, course_tarpath) + try: + checksum = calc_checksum(course_tarpath) + except ReadError: + log.exception("Error reading tar file %s, skipping", course_tarpath) + return False + if run.checksum == checksum and not overwrite: + log.info("Checksums match for %s, skipping load", key) + return False + try: + load_content_files( + run, + transform_content_files(course_tarpath, run, overwrite=overwrite), + ) + run.checksum = checksum + run.save() + except: # noqa: E722 + log.exception("Error ingesting OLX content data for %s", key) + + def get_most_recent_course_archives( etl_source: str, *, s3_prefix: str | None = None, override_base_prefix=False ) -> list[str]: @@ -53,7 +91,7 @@ def get_most_recent_course_archives( reversed( # noqa: C413 sorted( [ - obj + (obj.key, obj.last_modified) for obj in bucket.objects.filter( # Use s3_prefix for OLL, "20" for all others Prefix=s3_prefix @@ -62,17 +100,17 @@ def get_most_recent_course_archives( ) if re.search(course_tar_regex, obj.key) ], - key=lambda obj: obj.last_modified, + key=lambda obj: obj[1], ) ) ) if override_base_prefix: # More hoops to get desired result from OLL compared to other sources most_recent_export_date = "/".join( - [s3_prefix, most_recent_export_file.key.lstrip(s3_prefix).split("/")[0]] + [s3_prefix, most_recent_export_file[0].lstrip(s3_prefix).split("/")[0]] ) else: - most_recent_export_date = most_recent_export_file.key.split("/")[0] + most_recent_export_date = most_recent_export_file[0].split("/")[0] log.info("Most recent export date is %s", most_recent_export_date) return [ obj.key @@ -109,7 +147,7 @@ def sync_edx_course_files( for key in keys: matches = re.search(rf"{s3_prefix}/(.+)\.tar\.gz$", key) run_id = matches.group(1).split("/")[-1] - log.info("Run is is %s", run_id) + log.info("Run is %s", run_id) runs = ( LearningResourceRun.objects.filter( learning_resource__etl_source=etl_source, @@ -141,30 +179,20 @@ def sync_edx_course_files( runs = runs.filter(run_id__iregex=run_id) else: runs = runs.filter(run_id=run_id) - log.info("There are %d runs for %s", runs.count(), run_id) - run = runs.first() - if not run: + # There should be only 1 matching run per course archive, warn if not + if runs.count() > 1: + log.warning("There are %d runs for %s", runs.count(), run_id) + + if not runs: + log.info("No runs found for %s, skipping", run_id) continue - with TemporaryDirectory() as export_tempdir: - course_tarpath = Path(export_tempdir, key.split("/")[-1]) - log.info("course tarpath for run %s is %s", run.run_id, course_tarpath) - bucket.download_file(key, course_tarpath) - try: - checksum = calc_checksum(course_tarpath) - except ReadError: - log.exception("Error reading tar file %s, skipping", course_tarpath) - continue - if run.checksum == checksum and not overwrite: - log.info("Checksums match for %s, skipping load", key) - continue - try: - load_content_files( - run, - transform_content_files(course_tarpath, run, overwrite=overwrite), - ) - run.checksum = checksum - run.save() - except: # noqa: E722 - log.exception("Error ingesting OLX content data for %s", key) + run = runs.first() + course = run.learning_resource + if course.published and not course.test_mode and course.best_run != run: + # This is not the best run for the published course, so skip it + log.info("Not the best run for %s, skipping", run_id) + continue + + process_course_archive(bucket, key, run, overwrite=overwrite) diff --git a/learning_resources/etl/edx_shared_test.py b/learning_resources/etl/edx_shared_test.py index 2469d212ab..43c431e3f6 100644 --- a/learning_resources/etl/edx_shared_test.py +++ b/learning_resources/etl/edx_shared_test.py @@ -98,8 +98,10 @@ def test_sync_edx_course_files( # noqa: PLR0913 sync_edx_course_files( source, [course.id for course in courses], keys, s3_prefix=s3_prefix ) - assert mock_transform.call_count == (4 if published else 0) - assert mock_load_content_files.call_count == (4 if published else 0) + # Only best runs for published courses are processed, so 2 runs (one per course) not 4 + expected_calls = 2 if published else 0 + assert mock_transform.call_count == expected_calls + assert mock_load_content_files.call_count == expected_calls if published: for course in courses: mock_load_content_files.assert_any_call(course.best_run, fake_data) @@ -268,6 +270,120 @@ def test_sync_edx_course_files_error( assert mock_log.call_args[0][0].startswith("Error ingesting OLX content data for ") +@pytest.mark.parametrize( + "platform", [PlatformType.mitxonline.name, PlatformType.xpro.name] +) +def test_sync_edx_course_files_no_matching_run( + mock_mitxonline_learning_bucket, mock_xpro_learning_bucket, mocker, platform +): + """If no run matches the run_id from the tarball, it should be skipped""" + course = LearningResourceFactory.create( + platform=LearningResourcePlatformFactory.create(code=platform), + etl_source=platform, + published=True, + create_runs=True, + ) + run = course.best_run + # Use a different run_id in the key than what exists in the database + key = "20220101/courses/non-existent-run-id.tar.gz" + bucket = ( + mock_mitxonline_learning_bucket + if platform == PlatformType.mitxonline.name + else mock_xpro_learning_bucket + ).bucket + with Path.open( + Path("test_json/course-v1:MITxT+8.01.3x+3T2022.tar.gz"), "rb" + ) as infile: + bucket.put_object( + Key=key, + Body=infile.read(), + ACL="public-read", + ) + mocker.patch( + "learning_resources.etl.edx_shared.get_learning_course_bucket", + return_value=bucket, + ) + mock_load_content_files = mocker.patch( + "learning_resources.etl.edx_shared.load_content_files", + autospec=True, + return_value=[], + ) + mock_log = mocker.patch("learning_resources.etl.edx_shared.log.info") + + sync_edx_course_files(platform, [run.learning_resource.id], [key]) + + mock_load_content_files.assert_not_called() + mock_log.assert_any_call("No runs found for %s, skipping", "non-existent-run-id") + + +@pytest.mark.parametrize( + "platform", [PlatformType.mitxonline.name, PlatformType.xpro.name] +) +def test_sync_edx_course_files_test_mode_all_runs_processed( + mock_mitxonline_learning_bucket, mock_xpro_learning_bucket, mocker, platform +): + """For test mode courses, all runs should be processed (not just the best run)""" + course = LearningResourceFactory.create( + platform=LearningResourcePlatformFactory.create(code=platform), + etl_source=platform, + published=False, + test_mode=True, + create_runs=False, + ) + # Create multiple runs for this course + runs = LearningResourceRunFactory.create_batch( + 3, + learning_resource=course, + published=False, + ) + course.refresh_from_db() + + # Create keys for all runs + keys = [f"20220101/courses/{run.run_id}.tar.gz" for run in runs] + + bucket = ( + mock_mitxonline_learning_bucket + if platform == PlatformType.mitxonline.name + else mock_xpro_learning_bucket + ).bucket + + # Put all archive files in S3 + for key in keys: + with Path.open( + Path("test_json/course-v1:MITxT+8.01.3x+3T2022.tar.gz"), "rb" + ) as infile: + bucket.put_object( + Key=key, + Body=infile.read(), + ACL="public-read", + ) + + mocker.patch( + "learning_resources.etl.edx_shared.get_learning_course_bucket", + return_value=bucket, + ) + fake_data = '{"key": "data"}' + mock_transform = mocker.patch( + "learning_resources.etl.edx_shared.transform_content_files", + return_value=fake_data, + ) + mock_load_content_files = mocker.patch( + "learning_resources.etl.edx_shared.load_content_files", + autospec=True, + return_value=[], + ) + + sync_edx_course_files(platform, [course.id], keys) + + # All 3 runs should be processed, not just the best run + assert mock_transform.call_count == 3 + assert mock_load_content_files.call_count == 3 + + # Verify each run was processed + for run in runs: + mock_load_content_files.assert_any_call(run, fake_data) + + @pytest.mark.parametrize("platform", [PlatformType.edx.value, PlatformType.xpro.value]) def test_get_most_recent_course_archives( mocker, mock_mitxonline_learning_bucket, platform diff --git a/main/settings.py b/main/settings.py index e827a47fac..e89b809c06 100644 --- a/main/settings.py +++ b/main/settings.py @@ -34,7 +34,7 @@ from main.settings_pluggy import * # noqa: F403 from openapi.settings_spectacular import open_spectacular_settings -VERSION = "0.50.0" +VERSION = "0.50.2" log = logging.getLogger()