diff --git a/.gitignore b/.gitignore index e78ecff6ad..1e8fc16c8b 100644 --- a/.gitignore +++ b/.gitignore @@ -152,3 +152,6 @@ load_testing/data/ # Hacksnack game assets (copied by postinstall) frontends/main/public/games/hacksnack + +# drf-lint cross-file index cache +.drf_lint_cache.json diff --git a/RELEASE.rst b/RELEASE.rst index 9bfa45f5bc..453d9a25d4 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,18 @@ Release Notes ============= +Version 0.80.2 +-------------- + +- Only offer a run's courseware once it has started (#3925) +- chore: refresh drf-lint baseline for ORM003-ORM006 (#3928) +- fix(sentry): set max_request_body_size to small and scrub Postgres DETAIL rows (#3915) +- Sanitize and render rich-text descriptions from OVS (#3879) +- Build internal resource links from learn_url (#3885) +- Update dependency sharp to v0.35.4 [SECURITY] (#3917) +- Skip staff-only OLX content when ingesting edX course archives (#3909) +- Update certificate description in Product Page CertificateTrackCard (#3924) + Version 0.80.1 -------------- diff --git a/drf_lint_baseline.json b/drf_lint_baseline.json index 2222d8bddd..2a50349ca7 100644 --- a/drf_lint_baseline.json +++ b/drf_lint_baseline.json @@ -2,5 +2,46 @@ "channels/serializers.py:132:24:ORM002", "channels/serializers.py:134:24:ORM002", "channels/serializers.py:136:24:ORM002", - "profiles/serializers.py:205:16:ORM002" + "channels/serializers.py:97:32:ORM003", + "learning_resources/serializers.py:103:4:ORM005", + "learning_resources/serializers.py:1281:44:ORM005", + "learning_resources/serializers.py:1315:61:ORM003", + "learning_resources/serializers.py:1320:59:ORM003", + "learning_resources/serializers.py:1328:12:ORM004", + "learning_resources/serializers.py:1344:21:ORM004", + "learning_resources/serializers.py:1357:11:ORM006", + "learning_resources/serializers.py:1358:60:ORM006", + "learning_resources/serializers.py:1362:24:ORM004", + "learning_resources/serializers.py:1525:11:ORM006", + "learning_resources/serializers.py:1526:19:ORM006", + "learning_resources/serializers.py:1527:11:ORM006", + "learning_resources/serializers.py:1528:19:ORM006", + "learning_resources/serializers.py:1529:15:ORM006", + "learning_resources/serializers.py:153:4:ORM005", + "learning_resources/serializers.py:163:4:ORM005", + "learning_resources/serializers.py:1822:4:ORM005", + "learning_resources/serializers.py:1830:20:ORM004", + "learning_resources/serializers.py:213:4:ORM005", + "learning_resources/serializers.py:428:4:ORM005", + "learning_resources/serializers.py:429:4:ORM005", + "learning_resources/serializers.py:487:49:ORM006", + "learning_resources/serializers.py:502:12:ORM006", + "learning_resources/serializers.py:541:4:ORM005", + "learning_resources/serializers.py:590:4:ORM005", + "learning_resources/serializers.py:605:17:ORM006", + "learning_resources/serializers.py:610:17:ORM006", + "learning_resources/serializers.py:620:17:ORM006", + "learning_resources/serializers.py:877:18:ORM004", + "learning_resources/serializers.py:926:16:ORM004", + "learning_resources/serializers.py:943:20:ORM004", + "learning_resources/serializers.py:965:20:ORM004", + "learning_resources_search/serializers.py:661:4:ORM005", + "learning_resources_search/serializers.py:663:4:ORM005", + "profiles/serializers.py:108:39:ORM006", + "profiles/serializers.py:108:60:ORM006", + "profiles/serializers.py:113:19:ORM006", + "profiles/serializers.py:205:16:ORM002", + "profiles/serializers.py:353:45:ORM004", + "profiles/serializers.py:359:45:ORM004", + "profiles/serializers.py:502:8:ORM006" ] diff --git a/frontends/api/src/generated/v0/api.ts b/frontends/api/src/generated/v0/api.ts index 886e79590a..a39b59a902 100644 --- a/frontends/api/src/generated/v0/api.ts +++ b/frontends/api/src/generated/v0/api.ts @@ -1579,6 +1579,10 @@ export interface PodcastEpisodeParent { id: number title: string readable_id: string + /** + * Where this podcast lives within Learn + */ + learn_url: string } /** * Serializer for podcast episode resources diff --git a/frontends/api/src/generated/v1/api.ts b/frontends/api/src/generated/v1/api.ts index b851ec7b93..fa2b4a73ed 100644 --- a/frontends/api/src/generated/v1/api.ts +++ b/frontends/api/src/generated/v1/api.ts @@ -2540,6 +2540,10 @@ export interface PodcastEpisodeParent { id: number title: string readable_id: string + /** + * Where this podcast lives within Learn + */ + learn_url: string } /** * Serializer for PodcastEpisode diff --git a/frontends/api/src/test-utils/factories/learningResources.ts b/frontends/api/src/test-utils/factories/learningResources.ts index 5d9174f0f3..53305df828 100644 --- a/frontends/api/src/test-utils/factories/learningResources.ts +++ b/frontends/api/src/test-utils/factories/learningResources.ts @@ -655,6 +655,7 @@ const podcastEpisode: LearningResourceFactory = ( id: parentPodcastId, title: faker.lorem.words(3), readable_id: faker.string.uuid(), + learn_url: `${TEST_APP_ORIGIN}/podcast/${parentPodcastId}/podcast`, }, ], duration: faker.helpers.arrayElement(["PT1H13M44S", "PT2H30M", "PT1M"]), diff --git a/frontends/main/package.json b/frontends/main/package.json index cb33fd70b4..f98729719f 100644 --- a/frontends/main/package.json +++ b/frontends/main/package.json @@ -72,7 +72,7 @@ "react-hotkeys-hook": "^5.2.1", "react-markdown": "^10.0.0", "react-slick": "^0.31.0", - "sharp": "0.35.0", + "sharp": "0.35.4", "slick-carousel": "^1.8.1", "tiny-invariant": "^1.3.3", "video.js": "^8.23.7", 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 1fde5e8ae5..e654b974fc 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx @@ -98,6 +98,55 @@ describe.each([ }, ) + test.each([ + { runDates: currentRunDates, expectLink: true, case: "started" }, + { runDates: futureRunDates, expectLink: false, case: "not started" }, + ])( + "Title links to courseware only once the run has started ($case)", + async ({ runDates, expectLink }) => { + setupUserApis() + const coursewareUrl = faker.internet.url() + const enrollment = mitxonline.factories.enrollment.courseEnrollment({ + grades: [], + certificate: null, + run: { ...runDates, courseware_url: coursewareUrl }, + }) + renderWithProviders() + const card = getCard() + const title = enrollment.run.course.title + + if (expectLink) { + expect(within(card).getByRole("link", { name: title })).toHaveAttribute( + "href", + coursewareUrl, + ) + } else { + // The heading still names the course, it just isn't a way in. + await waitFor(() => { + expect( + within(card).queryByRole("link", { name: title }), + ).not.toBeInTheDocument() + }) + expect(within(card).getByText(title)).toBeInTheDocument() + } + }, + ) + + test("Title links to courseware for staff before the run starts", async () => { + setupUserApis({ is_staff: true }) + const coursewareUrl = faker.internet.url() + const enrollment = mitxonline.factories.enrollment.courseEnrollment({ + grades: [], + certificate: null, + run: { ...futureRunDates, courseware_url: coursewareUrl }, + }) + renderWithProviders() + const link = await within(getCard()).findByRole("link", { + name: enrollment.run.course.title, + }) + expect(link).toHaveAttribute("href", coursewareUrl) + }) + test("Courseware button is a navigable link for staff even when course has not started", async () => { setupUserApis({ is_staff: true }) const coursewareUrl = faker.internet.url() diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx index 0654379ef3..e6c4746bf8 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx @@ -24,7 +24,7 @@ import { getDashboardEnrollmentStatus, pickCertificateEnrollment, } from "./model/dashboardViewModel" -import { getCourseDateText } from "./courseDateUtils" +import { canOpenCourseware, getCourseDateText } from "./courseDateUtils" import { isVerifiedEnrollmentMode } from "@/common/mitxonline" import { RiArrowUpCircleLine, RiAwardLine, RiMore2Line } from "@remixicon/react" import { useReplaceBasketItem } from "@/common/mitxonline/useReplaceBasketItem" @@ -287,7 +287,7 @@ export const EnrolledCourseCard = ({ const enrollmentMode = enrollment?.enrollment_mode const offerUpgrade = !enrollment?.b2b_contract_id const startDate = run?.start_date - const hasStarted = startDate ? isInPast(startDate) : true + const coursewareOpen = canOpenCourseware(startDate, { isStaff }) const endDate = run?.end_date const hasEnded = endDate ? isInPast(endDate) : false const hasCourseDateText = getCourseDateText(startDate, endDate) !== null @@ -335,7 +335,7 @@ export const EnrolledCourseCard = ({ productId={run?.upgrade_product_id} isVerifiedProgramEnrollment={isVerifiedProgramEnrollment} readableId={run?.courseware_id} - coursewareUrl={coursewareUrl ?? undefined} + coursewareUrl={coursewareOpen ? (coursewareUrl ?? undefined) : undefined} programReadableIds={ancestorContext?.parentProgramReadableIds} programCoursewareId={ ancestorContext?.programEnrollment?.program.readable_id @@ -364,7 +364,7 @@ export const EnrolledCourseCard = ({ ) : null const titleSection = ( - {coursewareUrl ? ( + {coursewareUrl && coursewareOpen ? ( ) - // Determine if button should be disabled - // Staff can access courseware even before the course has started const courseHasEnded = run?.end_date ? isInPast(run.end_date) : false - const isDisabled = Boolean( - !coursewareUrl || // Enrolled but no action available - (!!startDate && !hasStarted && !isStaff), // Enrolled but course hasn't started yet - ) + const isDisabled = Boolean(!coursewareUrl || !coursewareOpen) const isCompleted = enrollmentStatus === EnrollmentStatus.Completed || courseHasEnded const buttonText = isCompleted ? "View" : "Continue" diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.test.tsx index 55f9a3e099..c891670844 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.test.tsx @@ -4,6 +4,7 @@ import { screen, setMockResponse, user, + waitFor, within, } from "@/test-utils" import * as mitxonline from "api/mitxonline-test-utils" @@ -29,6 +30,12 @@ beforeEach(() => { // Each row resolves its own Receipt item from the order history; default to // none, tests override. setupOrderHistory() + // Rows read is_staff to decide whether pre-start courseware is reachable. + // The factory randomises it, so pin it off; the staff test overrides. + setMockResponse.get( + mitxonline.urls.userMe.get(), + mitxonline.factories.user.user({ is_staff: false }), + ) setPerRunMenus(true) }) @@ -358,6 +365,53 @@ describe("SiblingRunsToggle + SiblingRunsPanel", () => { ).not.toBeInTheDocument() }) + // A run that hasn't started cannot be completed, so drop the factory's + // default certificate; completion outranks the dates for the row label. + const makeUpcomingEnrollment = () => + mitxonline.factories.enrollment.courseEnrollment({ + certificate: null, + grades: [], + run: { + start_date: moment().add(30, "days").toISOString(), + end_date: moment().add(90, "days").toISOString(), + courseware_url: faker.internet.url(), + }, + }) + + test.each([ + { isStaff: false, expectLink: false }, + { isStaff: true, expectLink: true }, + ])( + "upcoming sibling run offers 'View content' only to staff (isStaff=$isStaff)", + async ({ isStaff, expectLink }) => { + setMockResponse.get( + mitxonline.urls.userMe.get(), + mitxonline.factories.user.user({ is_staff: isStaff }), + ) + renderWithProviders( + , + ) + await expandAccordion() + expect(await screen.findByText(/^Upcoming:/)).toBeInTheDocument() + + const link = screen.queryByRole("link", { + name: /View content for Upcoming/, + }) + if (expectLink) { + await waitFor(() => { + expect( + screen.getByRole("link", { name: /View content for Upcoming/ }), + ).toBeInTheDocument() + }) + } else { + expect(link).not.toBeInTheDocument() + } + }, + ) + test("upcoming sibling run label starts with 'Upcoming:'", async () => { // A run that hasn't started cannot be completed, so drop the factory's // default certificate; completion outranks the dates. diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.tsx index 626736652d..ad0baae01e 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.tsx @@ -12,7 +12,11 @@ import { RiSubtractLine, RiTimeLine, } from "@remixicon/react" -import { formatRunIdentifier, getRunTimeState } from "./courseDateUtils" +import { + canOpenCourseware, + formatRunIdentifier, + getRunTimeState, +} from "./courseDateUtils" import type { RunTimeState } from "./courseDateUtils" import { ActionButton, VisuallyHidden } from "@mitodl/smoot-design" import { EnrollmentStatusIcon } from "./EnrollmentStatus" @@ -22,6 +26,8 @@ import { useOrderIdForRun } from "@/common/mitxonline/useOrderIdForResource" import { getRunMenuItems } from "./runMenuItems" import { useFeatureFlagEnabled } from "posthog-js/react" import { FeatureFlags } from "@/common/feature_flags" +import { useQuery } from "@tanstack/react-query" +import { mitxUserQueries } from "api/mitxonline-hooks/user" const UpcomingRunIcon = styled(RiTimeLine)(({ theme }) => ({ width: "16px", @@ -194,7 +200,11 @@ const RunListRow: React.FC = ({ enrollment, isFirst, }) => { + const mitxOnlineUser = useQuery(mitxUserQueries.me()) const coursewareUrl = enrollment.run?.courseware_url + const coursewareOpen = canOpenCourseware(enrollment.run?.start_date, { + isStaff: mitxOnlineUser.data?.is_staff, + }) /** * Resolved per row so each run's Receipt item reflects that run's own order. * Every row shares the one `orders/history` query, so N rows still cost a @@ -241,7 +251,7 @@ const RunListRow: React.FC = ({ - {coursewareUrl && ( + {coursewareUrl && coursewareOpen && ( <> ({ const mitxOnlineCourse = mitxonline.factories.courses.course -const mitxUser = mitxonline.factories.user.user +// The factory randomises is_staff, and staff bypass the start-date gate, which +// would make these tests flaky. Staff tests pass it explicitly. +const mitxUser: typeof mitxonline.factories.user.user = (overrides = {}) => + mitxonline.factories.user.user({ is_staff: false, ...overrides }) const setupUserApis = (overrides?: Parameters[0]) => { const userData = mitxonline.factories.user.user({ @@ -301,6 +304,8 @@ describe.each([ user: ReturnType course: ReturnType run?: ReturnType + /** Defer the POST to control when the mutation settles. */ + enrollResponse?: unknown }) => { setMockResponse.get(mitxonline.urls.userMe.get(), opts.user) setMockResponse.get(mitxonline.urls.enrollment.enrollmentsListV3(), []) @@ -308,10 +313,13 @@ describe.each([ const runId = opts.run?.courseware_id ?? opts.course.readable_id ?? undefined const enrollmentUrl = mitxonline.urls.b2b.courseEnrollment(runId) - setMockResponse.post(enrollmentUrl, { - result: "b2b-enroll-success", - order: 1, - }) + setMockResponse.post( + enrollmentUrl, + opts.enrollResponse ?? { + result: "b2b-enroll-success", + order: 1, + }, + ) const countries = [ { code: "US", name: "United States" }, @@ -332,6 +340,62 @@ describe.each([ { trigger: "title-link" as const }, ] + /** + * A response the test resolves by hand, giving `enrollAndSettle` a settle + * point to wait on. The B2B and verified redirects fire in `onSuccess` and + * leave no other trace, so asserting once the POST is merely issued can run + * before the redirect would have, and pass either way. + */ + const deferredResponse = () => { + let resolve!: (value: T) => void + const promise = new Promise((res) => { + resolve = res + }) + return { promise, resolve } + } + + /** + * Checks both kinds of leaving: the hard `window.location` redirect this + * guards, and a router navigation, so neither can creep back in. + */ + const expectStayedPut = ( + location: ReturnType["location"], + pathnameBefore: string, + hrefBefore: string, + ) => { + expect(window.location.href).toBe(hrefBefore) + expect(location.current.pathname).toBe(pathnameBefore) + expect(location.current.search).toBe("") + } + + const enrollAndSettle = async ( + card: HTMLElement, + release: () => void, + ): Promise => { + const button = within(card).getByTestId("courseware-button") + await user.click(button) + await waitFor(() => { + expect(button).toHaveAttribute("aria-busy", "true") + }) + release() + await waitFor(() => { + expect(button).toHaveAttribute("aria-busy", "false") + }) + } + + const START_DATE_CASES = [ + { + case: "redirects to courseware when the run has started", + startDate: moment().subtract(7, "days").toISOString(), + expectRedirect: true, + }, + { + case: "does not redirect when the run has not started", + startDate: moment().add(30, "days").toISOString(), + expectRedirect: false, + }, + ] + test.each(ENROLLMENT_TRIGGERS)( "B2B enrollment for complete profile bypasses just-in-time dialog ($trigger)", async ({ trigger }) => { @@ -414,6 +478,93 @@ describe.each([ }, ) + test("B2B enrollment redirects staff to courseware even before the run starts", async () => { + const userData = mitxUser({ + is_staff: true, + legal_address: { country: "US" }, + user_profile: { year_of_birth: 1988 }, + }) + const b2bContractId = faker.number.int() + const coursewareUrl = faker.internet.url() + const run = mitxonline.factories.courses.courseRun({ + b2b_contract: b2bContractId, + is_enrollable: true, + start_date: moment().add(30, "days").toISOString(), + courseware_url: coursewareUrl, + }) + const course = mitxOnlineCourse({ courseruns: [run], next_run_id: run.id }) + const { enrollmentUrl } = setupEnrollmentApis({ + user: userData, + course, + run, + }) + + renderWithProviders( + , + ) + + await user.click(within(getCard()).getByTestId("courseware-button")) + + await waitFor(() => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ method: "post", url: enrollmentUrl }), + ) + }) + // Staff keep pre-start courseware access, so the redirect still fires. + await waitFor(() => { + expect(window.location.href).toBe(coursewareUrl) + }) + }) + + test.each(START_DATE_CASES)( + "B2B enrollment $case", + async ({ startDate, expectRedirect }) => { + const userData = mitxUser({ + legal_address: { country: "US" }, + user_profile: { year_of_birth: 1988 }, + }) + const b2bContractId = faker.number.int() + const coursewareUrl = faker.internet.url() + const run = mitxonline.factories.courses.courseRun({ + b2b_contract: b2bContractId, + is_enrollable: true, + start_date: startDate, + end_date: moment(startDate).add(60, "days").toISOString(), + courseware_url: coursewareUrl, + }) + const course = mitxOnlineCourse({ + courseruns: [run], + next_run_id: run.id, + }) + const enroll = deferredResponse() + const { enrollmentUrl } = setupEnrollmentApis({ + user: userData, + course, + run, + enrollResponse: enroll.promise, + }) + + const { location } = renderWithProviders( + , + ) + const pathnameBefore = location.current.pathname + const hrefBefore = window.location.href + + await enrollAndSettle(getCard(), () => + enroll.resolve({ result: "b2b-enroll-success", order: 1 }), + ) + + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ method: "post", url: enrollmentUrl }), + ) + if (expectRedirect) { + expect(window.location.href).toBe(coursewareUrl) + } else { + expectStayedPut(location, pathnameBefore, hrefBefore) + } + }, + ) + test("B2B enrollment targets the displayed (variant) run, not getBestRun's default pick", async () => { const userData = mitxUser({ legal_address: { country: "US" }, @@ -482,6 +633,65 @@ describe.each([ // --------------------------------------------------------------------------- describe("B2C (non-B2B) Enrollment", () => { + // The dialog is its own redirect path: the start date comes from the run + // picked there, not the one the card displayed. + test.each(START_DATE_CASES)( + "CourseEnrollmentDialog submission $case", + async ({ startDate, expectRedirect }) => { + setMockResponse.get(mitxonline.urls.userMe.get(), mitxUser()) + + const coursewareUrl = faker.internet.url() + // Both modes opens the dialog; a single run makes it preselect. + const run = mitxonline.factories.courses.courseRun({ + b2b_contract: null, + is_enrollable: true, + start_date: startDate, + end_date: moment(startDate).add(60, "days").toISOString(), + courseware_url: coursewareUrl, + enrollment_modes: [ + mitxonline.factories.courses.enrollmentMode({ + requires_payment: false, + }), + mitxonline.factories.courses.enrollmentMode({ + requires_payment: true, + }), + ], + }) + const course = mitxOnlineCourse({ + courseruns: [run], + next_run_id: run.id, + }) + setMockResponse.post(mitxonline.urls.enrollment.enrollmentsListV1(), {}) + setMockResponse.get(mitxonline.urls.enrollment.enrollmentsListV3(), []) + + const { location } = renderWithProviders( + , + ) + const pathnameBefore = location.current.pathname + const hrefBefore = window.location.href + + await user.click(within(getCard()).getByTestId("courseware-button")) + const dialog = await screen.findByRole("dialog", { + name: course.title, + }) + await user.click( + within(dialog).getByRole("button", { + name: /Enroll for Free without a certificate/, + }), + ) + + // Fires inside the same onSuccess that decides where to go. + await waitFor(() => { + expect(trackCourseEnrolled).toHaveBeenCalledWith(course.title) + }) + if (expectRedirect) { + expect(window.location.href).toBe(coursewareUrl) + } else { + expectStayedPut(location, pathnameBefore, hrefBefore) + } + }, + ) + test.each(ENROLLMENT_TRIGGERS)( "Clicking $trigger opens CourseEnrollmentDialog for both-mode enrollment", async ({ trigger }) => { @@ -570,6 +780,54 @@ describe.each([ }, ) + test.each(START_DATE_CASES)( + "Free single-run enrollment $case", + async ({ startDate, expectRedirect }) => { + setMockResponse.get(mitxonline.urls.userMe.get(), mitxUser()) + + const coursewareUrl = faker.internet.url() + const run = mitxonline.factories.courses.courseRun({ + b2b_contract: null, + is_enrollable: true, + start_date: startDate, + end_date: moment(startDate).add(60, "days").toISOString(), + courseware_url: coursewareUrl, + enrollment_modes: [ + mitxonline.factories.courses.enrollmentMode({ + requires_payment: false, + }), + ], + }) + const course = mitxOnlineCourse({ + courseruns: [run], + next_run_id: run.id, + }) + + setMockResponse.post(mitxonline.urls.enrollment.enrollmentsListV1(), {}) + setMockResponse.get(mitxonline.urls.enrollment.enrollmentsListV3(), []) + + const { location } = renderWithProviders( + , + ) + const pathnameBefore = location.current.pathname + const hrefBefore = window.location.href + + await user.click(within(getCard()).getByTestId("courseware-button")) + + await waitFor(() => { + expect(trackCourseEnrolled).toHaveBeenCalledWith(course.title) + }) + + if (expectRedirect) { + await waitFor(() => { + expect(window.location.href).toBe(coursewareUrl) + }) + } else { + expectStayedPut(location, pathnameBefore, hrefBefore) + } + }, + ) + test.each(ENROLLMENT_TRIGGERS)( "Clicking $trigger bypasses dialog for paid-only single-run enrollment", async ({ trigger }) => { @@ -634,6 +892,7 @@ describe.each([ const run = mitxonline.factories.courses.courseRun({ b2b_contract: null, is_enrollable: true, + start_date: moment().subtract(7, "days").toISOString(), courseware_url: faker.internet.url(), }) const course = mitxOnlineCourse({ @@ -687,6 +946,57 @@ describe.each([ }, ) + test.each(START_DATE_CASES)( + "Verified program enrollment $case", + async ({ startDate, expectRedirect }) => { + setMockResponse.get(mitxonline.urls.userMe.get(), mitxUser()) + + const coursewareUrl = faker.internet.url() + const run = mitxonline.factories.courses.courseRun({ + b2b_contract: null, + is_enrollable: true, + start_date: startDate, + end_date: moment(startDate).add(60, "days").toISOString(), + courseware_url: coursewareUrl, + }) + const course = mitxOnlineCourse({ + courseruns: [run], + next_run_id: run.id, + }) + const programEnrollment = + mitxonline.factories.enrollment.programEnrollmentV3({ + enrollment_mode: "verified", + }) + const programEnrollmentEndpoint = + mitxonline.urls.verifiedProgramEnrollments.create(run.courseware_id) + const enroll = deferredResponse() + setMockResponse.post(programEnrollmentEndpoint, enroll.promise) + + const { location } = renderWithProviders( + , + ) + const pathnameBefore = location.current.pathname + const hrefBefore = window.location.href + + await enrollAndSettle(getCard(), () => enroll.resolve({})) + + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "post", + url: programEnrollmentEndpoint, + }), + ) + if (expectRedirect) { + expect(window.location.href).toBe(coursewareUrl) + } else { + expectStayedPut(location, pathnameBefore, hrefBefore) + } + }, + ) + test("Audit program enrollment opens CourseEnrollmentDialog when both enrollment modes are available", async () => { const userData = mitxUser() setMockResponse.get(mitxonline.urls.userMe.get(), userData) diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.tsx index b92c88cdd6..bc672642b3 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.tsx @@ -85,12 +85,14 @@ export const UnenrolledCourseCard = ({ b2bProgramId: ancestorContext?.parentProgramReadableIds?.[0] ?? ancestorContext?.programEnrollment?.program.readable_id, + startDate: courseRun?.start_date, }) }, [ course, ancestorContext, readableId, coursewareUrl, + courseRun?.start_date, isContractPageResource, enrollment, ]) diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/courseDateUtils.ts b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/courseDateUtils.ts index ffae5f224a..9637f0b823 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/courseDateUtils.ts +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/courseDateUtils.ts @@ -22,6 +22,17 @@ export const getRunTimeState = ( return "underway" } +/** + * Whether this run's courseware can be opened yet; staff may preview early. + * + * Shared by every route in (card button, card title, sibling-run rows, upgrade + * and post-enrollment redirects) so they cannot disagree. + */ +export const canOpenCourseware = ( + startDate?: string | null, + { isStaff = false }: { isStaff?: boolean } = {}, +): boolean => isStaff || getRunTimeState(startDate) !== "upcoming" + /** * A run's date range. Returns "" when the run has neither date; prefer * `formatRunIdentifier` for anything a learner reads, since that case is not diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/hooks/useEnrollmentHandler.ts b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/hooks/useEnrollmentHandler.ts index 4728e13f36..57779b752b 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/hooks/useEnrollmentHandler.ts +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/hooks/useEnrollmentHandler.ts @@ -14,6 +14,9 @@ import { getCourseEnrollmentAction } from "@/common/mitxonline" import { useComplianceGate } from "@/common/mitxonline/useComplianceGate" import CourseEnrollmentDialog from "@/page-components/EnrollmentDialogs/CourseEnrollmentDialog" import { trackCourseEnrolled } from "@/common/analytics/gtm" +import { canOpenCourseware } from "../courseDateUtils" +import { mitxUserQueries } from "api/mitxonline-hooks/user" +import { useQuery } from "@tanstack/react-query" const ENROLL_COURSE_ERROR = "Something went wrong enrolling you in this course. Please try again." @@ -32,6 +35,8 @@ export const useEnrollmentHandler = () => { }) const replaceBasketItem = useReplaceBasketItem() const { ensureCompliance } = useComplianceGate() + const mitxOnlineUser = useQuery(mitxUserQueries.me()) + const isStaff = mitxOnlineUser.data?.is_staff const enroll = React.useCallback( async ({ @@ -44,6 +49,7 @@ export const useEnrollmentHandler = () => { programCoursewareId, programReadableIds, b2bProgramId, + startDate, }: { course: CourseWithCourseRunsSerializerV2 readableId?: string @@ -54,7 +60,18 @@ export const useEnrollmentHandler = () => { programCoursewareId?: string programReadableIds?: string[] b2bProgramId?: string + startDate?: string | null }) => { + /** + * Enrolling early is allowed, so only redirect once the courseware is + * open. Otherwise the learner stays put and the card they clicked + * re-renders as enrolled, showing when the run starts. + */ + const finishEnrollment = (url: string, runStartDate?: string | null) => { + if (!canOpenCourseware(runStartDate, { isStaff })) return + window.location.href = url + } + if (isB2B) { if (!readableId) { console.warn("Cannot enroll in B2B course: missing required data", { @@ -90,7 +107,7 @@ export const useEnrollmentHandler = () => { }, { onSuccess: () => { - window.location.href = destinationUrl + finishEnrollment(destinationUrl, startDate) }, }, ) @@ -125,7 +142,7 @@ export const useEnrollmentHandler = () => { { courserun_id: readableId, request_body: requestBody }, { onSuccess: () => { - window.location.href = verifiedDestination ?? href + finishEnrollment(verifiedDestination ?? href, startDate) }, }, ) @@ -144,7 +161,7 @@ export const useEnrollmentHandler = () => { enrollmentAction.run.courseware_url ?? href if (destination) { - window.location.href = destination + finishEnrollment(destination, enrollmentAction.run.start_date) } }, }, @@ -158,7 +175,7 @@ export const useEnrollmentHandler = () => { } const onCourseEnroll = (run: CourseRunV2) => { - window.location.href = run.courseware_url! + finishEnrollment(run.courseware_url!, run.start_date) } NiceModal.show(CourseEnrollmentDialog, { course, onCourseEnroll }) } @@ -169,6 +186,7 @@ export const useEnrollmentHandler = () => { createEnrollment, createVerifiedProgramEnrollment, replaceBasketItem, + isStaff, ], ) diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.test.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.test.tsx index 2a43638a36..12e48bb365 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.test.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.test.tsx @@ -3,6 +3,7 @@ import { factories, setMockResponse, urls } from "api/test-utils" import { ResourceTypeEnum } from "api/v1" import type { LearningResource, PodcastEpisodeResource } from "api/v1" import { renderWithProviders, screen, user } from "@/test-utils" +import { podcastEpisodePath } from "@/common/urls" import { PodcastDetailPage } from "./PodcastDetailPage" jest.mock( @@ -67,6 +68,7 @@ const setupApis = ({ id: podcast.id, title: podcast.title!, readable_id: podcast.readable_id, + learn_url: podcast.learn_url, }, ] } @@ -98,6 +100,28 @@ const setupApis = ({ } describe("PodcastDetailPage", () => { + test("episode rows keep this podcast as context and take only the backend slug", async () => { + // The episode's canonical parent is a *different* podcast — an episode in + // several podcasts is viewable under any of them. The row href must keep the + // podcast being viewed and borrow only the slug. + const episodes = makePodcastEpisodes(1) + const { podcast } = setupApis({ episodesPage1: episodes }) + const episode = episodes[0] + episode.podcast_episode!.podcasts = [podcast.id + 1, podcast.id] + + renderWithProviders() + + const title = await screen.findByText(episode.title!) + expect(title.closest("a")).toHaveAttribute( + "href", + podcastEpisodePath( + String(episode.id), + String(podcast.id), + episode.url_slug, + ), + ) + }) + test("renders initial episode list", async () => { const episodes = makePodcastEpisodes(3) const { podcast } = setupApis({ episodesPage1: episodes }) @@ -106,7 +130,16 @@ describe("PodcastDetailPage", () => { await screen.findByText(episodes[0].title!) for (const episode of episodes) { - expect(screen.getByText(episode.title!)).toBeInTheDocument() + const title = screen.getByText(episode.title!) + expect(title).toBeInTheDocument() + expect(title.closest("a")).toHaveAttribute( + "href", + podcastEpisodePath( + String(episode.id), + String(podcast.id), + episode.url_slug, + ), + ) } }) diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.tsx index d4d0e9c2ee..026ca3438f 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.tsx @@ -11,7 +11,7 @@ import { import { ResourceTypeEnum } from "api/v1" import type { LearningResource } from "api/v1" import { formatDate } from "ol-utilities" -import { HOME, podcastEpisodePageView } from "@/common/urls" +import { HOME, podcastEpisodePath } from "@/common/urls" import { addExternalLinkTargets } from "@/common/utils" import PodcastContainer from "./PodcastContainer" import PodcastBreadcrumbs from "./PodcastBreadcrumbs" @@ -406,10 +406,10 @@ export const PodcastDetailPage: React.FC = ({ key={episode.id} isMobile={isMobile} episode={episode} - href={podcastEpisodePageView( + href={podcastEpisodePath( String(episode.id), String(id), - episode.title, + episode.url_slug, )} onPlayClick={handlePlayClick} onPauseClick={pause} diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastEpisodeDetailPage.test.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastEpisodeDetailPage.test.tsx index 5f25f8c595..8b0d74b244 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastEpisodeDetailPage.test.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastEpisodeDetailPage.test.tsx @@ -1,5 +1,6 @@ import React from "react" import { factories, setMockResponse, urls } from "api/test-utils" +import { absoluteUrl, podcastEpisodePath } from "@/common/urls" import { ResourceTypeEnum } from "api/v1" import type { LearningResource, PodcastEpisodeResource } from "api/v1" import { renderWithProviders, screen, user, waitFor } from "@/test-utils" @@ -77,6 +78,7 @@ const setupApis = ({ id: podcast.id, title: podcast.title!, readable_id: podcast.readable_id, + learn_url: podcast.learn_url, }, ], has_transcript: @@ -171,6 +173,18 @@ describe("PodcastEpisodeDetailPage", () => { await screen.findByText(moreEpisodes[0].title!) expect(screen.getByText(moreEpisodes[1].title!)).toBeInTheDocument() + + // Each row keeps the podcast being viewed as the context segment and takes + // only the slug from the episode. + for (const more of moreEpisodes) { + const link = screen.getByRole("link", { + name: new RegExp(more.title!, "i"), + }) + expect(link).toHaveAttribute( + "href", + podcastEpisodePath(String(more.id), String(podcast.id), more.url_slug), + ) + } }) test("play button is present and enabled when episode has an audio URL", async () => { @@ -247,6 +261,7 @@ describe("PodcastEpisodeDetailPage", () => { id: podcast.id, title: podcast.title!, readable_id: podcast.readable_id, + learn_url: podcast.learn_url, }, ] @@ -316,7 +331,63 @@ describe("PodcastEpisodeDetailPage", () => { expect(internalLink).not.toHaveAttribute("target") }) - test("names the URL's podcast (not the first parent) for a multi-parent episode", async () => { + test("Share link keeps the podcast the episode is viewed under", async () => { + // Sharing hands out the page in front of the user, parent podcast included, + // even when that is not the canonical parent: the recommendation is usually + // about the series it was found in. + const episode = makePodcastEpisode() + const canonical = makePodcast({ title: "Canonical Podcast" }) + const viewed = makePodcast({ title: "Viewed Podcast" }) + episode.podcast_episode.podcasts = [canonical.id, viewed.id] + episode.podcast_episode.parent_podcasts = [ + { + id: canonical.id, + title: canonical.title!, + readable_id: canonical.readable_id, + learn_url: canonical.learn_url, + }, + { + id: viewed.id, + title: viewed.title!, + readable_id: viewed.readable_id, + learn_url: viewed.learn_url, + }, + ] + + setMockResponse.get( + urls.learningResources.details({ id: episode.id }), + episode, + ) + setMockResponse.get( + urls.learningResources.details({ id: viewed.id }), + viewed, + ) + setMockResponse.get( + `${urls.learningResources.items({ id: viewed.id })}?limit=${EPISODES_PAGE_SIZE}`, + makeItemsResponse([episode]), + ) + + renderWithProviders( + , + ) + + await user.click(await screen.findByRole("button", { name: /share/i })) + + expect(screen.getByRole("textbox")).toHaveValue( + absoluteUrl( + podcastEpisodePath( + String(episode.id), + String(viewed.id), + episode.url_slug, + ), + ), + ) + }) + + test("shows the viewed podcast but publishes the canonical one", async () => { const episode = makePodcastEpisode() episode.podcast_episode.audio_url = "https://example.com/ep.mp3" // The resource factory leaves last_modified unset, and the JSON-LD is @@ -331,11 +402,13 @@ describe("PodcastEpisodeDetailPage", () => { id: podcastA.id, title: "Podcast A", readable_id: podcastA.readable_id, + learn_url: podcastA.learn_url, }, { id: podcastB.id, title: "Podcast B", readable_id: podcastB.readable_id, + learn_url: podcastB.learn_url, }, ] @@ -363,24 +436,22 @@ describe("PodcastEpisodeDetailPage", () => { await screen.findByRole("button", { name: /play episode/i }), ) - // The header/breadcrumb and the player bar must agree on Podcast B. + // The header/breadcrumb and the player bar follow the route: Podcast B. expect(screen.getByTestId("player-podcast-name")).toHaveTextContent( "Podcast B", ) - // So must the JSON-LD: partOfSeries takes its url from the podcast in the - // current route, so taking the name from parent_podcasts[0] instead would - // publish Podcast A's name against Podcast B's url. + // The JSON-LD does not. It is read by crawlers, so both its url and its + // series name the canonical parent — A — and never the route's. const jsonLd = JSON.parse( document.querySelector('script[type="application/ld+json"]')!.innerHTML, ) - expect(jsonLd.partOfSeries).toEqual( - expect.objectContaining({ - "@type": "PodcastSeries", - name: "Podcast B", - url: expect.stringContaining(`/podcast/${podcastB.id}/`), - }), - ) + expect(jsonLd.url).toBe(episode.learn_url) + expect(jsonLd.partOfSeries).toEqual({ + "@type": "PodcastSeries", + name: "Podcast A", + url: podcastA.learn_url, + }) }) test("escapes every < in the JSON-LD, not just { diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastEpisodeDetailPage.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastEpisodeDetailPage.tsx index bc93c78c5a..74a8a06c7c 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastEpisodeDetailPage.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastEpisodeDetailPage.tsx @@ -13,7 +13,7 @@ import { useQuery } from "@tanstack/react-query" import { ResourceTypeEnum } from "api/v1" import type { PodcastEpisodeResource } from "api/v1" import { formatDate } from "ol-utilities" -import { HOME, podcastPageView, podcastEpisodePageView } from "@/common/urls" +import { HOME, absoluteUrl, podcastEpisodePath } from "@/common/urls" import { addExternalLinkTargets } from "@/common/utils" import { EpisodeItem } from "./PodcastsListingPage/EpisodeItem" import PodcastContainer from "./PodcastContainer" @@ -37,9 +37,6 @@ import PodcastShareButton from "./PodcastShareButton" import EpisodeContentTabs from "./EpisodeContentTabs" import type { TranscriptState } from "./EpisodeContentTabs" import { buildPodcastEpisodeStructuredData } from "./podcastEpisodeStructuredData" -import { env } from "@/env" - -const NEXT_PUBLIC_ORIGIN = env("NEXT_PUBLIC_ORIGIN") /* ── Layout ── */ @@ -259,13 +256,17 @@ export const PodcastEpisodeDetailPage: React.FC< toggle(episode, Number(podcastId)) } - const podcastHref = podcastId - ? podcastPageView(podcastId, parentPodcast?.title) - : "/" + // A podcast has a single page, so its `learn_url` is that page. + const podcastHref = parentPodcast?.learn_url ?? "/" + // Shares the page in front of the user, parent podcast included: an episode + // in several podcasts is viewable under any of them, and a recommendation is + // usually about the series it was found in. const sharePageUrl = episode && podcastId - ? `${NEXT_PUBLIC_ORIGIN}${podcastEpisodePageView(String(episode.id), podcastId, episode.title)}` + ? absoluteUrl( + podcastEpisodePath(String(episode.id), podcastId, episode.url_slug), + ) : "" // Episode descriptions are sanitized on the backend with nh3 during ETL @@ -285,15 +286,7 @@ export const PodcastEpisodeDetailPage: React.FC< // tag so crawlers can read it without executing any additional JS. // See: https://schema.org/PodcastEpisode const structuredData = !episodeLoading - ? buildPodcastEpisodeStructuredData(episode as PodcastEpisodeResource, { - url: sharePageUrl || undefined, - // The same parent the breadcrumb and podcastHref use, so partOfSeries' - // name and url always describe one series. - series: parentPodcast, - seriesUrl: podcastId - ? `${NEXT_PUBLIC_ORIGIN}${podcastHref}` - : undefined, - }) + ? buildPodcastEpisodeStructuredData(episode as PodcastEpisodeResource) : null return ( @@ -398,10 +391,10 @@ export const PodcastEpisodeDetailPage: React.FC< episode={episode} href={ podcastId - ? podcastEpisodePageView( + ? podcastEpisodePath( String(episode.id), podcastId, - episode.title, + episode.url_slug, ) : "" } diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/LatestEpisodesSection.test.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/LatestEpisodesSection.test.tsx index 7243dd776c..773413b2b6 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/LatestEpisodesSection.test.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/LatestEpisodesSection.test.tsx @@ -1,17 +1,21 @@ import React from "react" +import { faker } from "@faker-js/faker/locale/en" import { factories } from "api/test-utils" import type { LearningResource } from "api/v1" -import { SEARCH_PODCAST_EPISODES } from "@/common/urls" +import { SEARCH_PODCAST_EPISODES, podcastEpisodePath } from "@/common/urls" import { renderWithProviders, screen, user } from "@/test-utils" import LatestEpisodesSection from "./LatestEpisodesSection" +/** The podcast the rows are scoped to, taken from the episode's `podcasts[0]`. */ +const CONTEXT_PODCAST_ID = faker.number.int({ min: 1, max: 1e6 }) + const makeEpisodes = (count: number): LearningResource[] => Array.from({ length: count }, (_, i) => factories.learningResources.podcastEpisode({ title: `Episode ${i + 1}`, podcast_episode: { id: i + 1, - podcasts: [1], + podcasts: [CONTEXT_PODCAST_ID], duration: "PT1M", audio_url: "https://example.com/audio.mp3", episode_link: "https://example.com/link", @@ -36,6 +40,36 @@ describe("LatestEpisodesSection", () => { expect(screen.getByText("All episodes")).toBeInTheDocument() }) + it("keeps each episode's podcast context and takes only the backend slug", () => { + const episodes = makeEpisodes(2) + renderWithProviders( + true} + />, + ) + + // Each row is an anchor given role="listitem", so query by that role. + const rows = screen.getAllByRole("listitem") + expect(rows).toHaveLength(episodes.length) + episodes.forEach((episode, i) => { + expect(rows[i]).toHaveTextContent(episode.title!) + expect(rows[i]).toHaveAttribute( + "href", + podcastEpisodePath( + String(episode.id), + String(CONTEXT_PODCAST_ID), + episode.url_slug, + ), + ) + }) + }) + it("renders all provided episodes", () => { const episodes = makeEpisodes(3) renderWithProviders( diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/LatestEpisodesSection.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/LatestEpisodesSection.tsx index b36633c8e4..4817ee59e1 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/LatestEpisodesSection.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/LatestEpisodesSection.tsx @@ -2,7 +2,7 @@ import React from "react" import { Link, Skeleton, styled } from "ol-components" import { ButtonLink } from "@mitodl/smoot-design" import type { LearningResource, PodcastEpisodeResource } from "api/v1" -import { SEARCH_PODCAST_EPISODES, podcastEpisodePageView } from "@/common/urls" +import { SEARCH_PODCAST_EPISODES, podcastEpisodePath } from "@/common/urls" import { Section, SectionHeader, @@ -130,10 +130,10 @@ const LatestEpisodesSection: React.FC = ({ overline={overline} href={ parentPodcastId - ? podcastEpisodePageView( + ? podcastEpisodePath( String(episode.id), String(parentPodcastId), - episode.title, + episode.url_slug, ) : SEARCH_PODCAST_EPISODES } diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.test.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.test.tsx index 51317c7f6f..7c175b8228 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.test.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.test.tsx @@ -1,7 +1,7 @@ import React from "react" import { factories } from "api/test-utils" import type { LearningResource } from "api/v1" -import { SEARCH_PODCASTS, podcastPageView } from "@/common/urls" +import { SEARCH_PODCASTS } from "@/common/urls" import { renderWithProviders, screen } from "@/test-utils" import PodcastSection from "./PodcastSection" @@ -67,7 +67,7 @@ describe("PodcastSection", () => { expect(screen.getByText(/Updated May 3/)).toBeInTheDocument() expect(screen.getByRole("link", { name: /Chalk Radio/ })).toHaveAttribute( "href", - podcastPageView("1", "Chalk Radio"), + series.learn_url, ) }) diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.tsx index c5c3d5e4a9..1723d5b687 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastsListingPage/PodcastSection.tsx @@ -7,7 +7,7 @@ import { RiArrowRightLine, RiArrowRightSLine } from "@remixicon/react" import DOMPurify from "isomorphic-dompurify" import { formatDate } from "ol-utilities" import type { LearningResource } from "api/v1" -import { SEARCH_PODCASTS, podcastPageView } from "@/common/urls" +import { SEARCH_PODCASTS } from "@/common/urls" import { stripAnchorTags } from "@/common/utils" import { Section, @@ -316,10 +316,7 @@ const PodcastSection: React.FC = ({ ? formatDate(item.last_modified, "MMM D") : null return ( - + {item.image?.url && ( = ({ ? formatDate(item.last_modified, "MMM D") : null return ( - + { id: 1, podcasts: [42], parent_podcasts: [ - { id: 42, title: "The Show Name", readable_id: "the-show" }, + { + id: 42, + title: "The Show Name", + readable_id: "the-show", + learn_url: "http://test.learn.odl.local:8062/podcast/42/the-show", + }, ], duration: "PT1M", audio_url: "https://example.com/audio.mp3", @@ -230,8 +235,18 @@ describe("getEpisodeParentPodcastName", () => { id: 1, podcasts: [1, 2], parent_podcasts: [ - { id: 1, title: "Podcast A", readable_id: "podcast-a" }, - { id: 2, title: "Podcast B", readable_id: "podcast-b" }, + { + id: 1, + title: "Podcast A", + readable_id: "podcast-a", + learn_url: "http://test.learn.odl.local:8062/podcast/1/podcast-a", + }, + { + id: 2, + title: "Podcast B", + readable_id: "podcast-b", + learn_url: "http://test.learn.odl.local:8062/podcast/2/podcast-b", + }, ], duration: "PT1M", audio_url: "https://example.com/audio.mp3", @@ -263,8 +278,18 @@ describe("getEpisodeParentPodcast", () => { id: 1, podcasts: [1, 2], parent_podcasts: [ - { id: 1, title: "Podcast A", readable_id: "podcast-a" }, - { id: 2, title: "Podcast B", readable_id: "podcast-b" }, + { + id: 1, + title: "Podcast A", + readable_id: "podcast-a", + learn_url: "http://test.learn.odl.local:8062/podcast/1/podcast-a", + }, + { + id: 2, + title: "Podcast B", + readable_id: "podcast-b", + learn_url: "http://test.learn.odl.local:8062/podcast/2/podcast-b", + }, ], duration: "PT1M", audio_url: "https://example.com/audio.mp3", @@ -284,6 +309,7 @@ describe("getEpisodeParentPodcast", () => { id: 2, title: "Podcast B", readable_id: "podcast-b", + learn_url: "http://test.learn.odl.local:8062/podcast/2/podcast-b", }) }) diff --git a/frontends/main/src/app-pages/PodcastPage/podcastEpisodeStructuredData.test.ts b/frontends/main/src/app-pages/PodcastPage/podcastEpisodeStructuredData.test.ts index cb7653b442..a87b574c32 100644 --- a/frontends/main/src/app-pages/PodcastPage/podcastEpisodeStructuredData.test.ts +++ b/frontends/main/src/app-pages/PodcastPage/podcastEpisodeStructuredData.test.ts @@ -1,5 +1,6 @@ +import { faker } from "@faker-js/faker/locale/en" import { factories } from "api/test-utils" -import type { PodcastEpisodeResource } from "api/v1" +import type { PodcastEpisodeParent, PodcastEpisodeResource } from "api/v1" import { buildPodcastEpisodeStructuredData } from "./podcastEpisodeStructuredData" const makeEpisode = ( @@ -12,24 +13,29 @@ const makeEpisode = ( podcast_episode: podcastEpisode, }) +const makeParent = (): PodcastEpisodeParent => { + const id = faker.number.int({ min: 1, max: 1e6 }) + const slug = faker.lorem.slug() + return { + id, + title: faker.lorem.words(3), + readable_id: faker.lorem.slug(), + learn_url: `http://test.learn.odl.local:8062/podcast/${id}/${slug}`, + } +} + test("omits the payload entirely without a last_modified date", () => { const episode = factories.learningResources.podcastEpisode({ last_modified: null, }) - expect( - buildPodcastEpisodeStructuredData(episode, { series: null }), - ).toBeNull() - expect( - buildPodcastEpisodeStructuredData(undefined, { series: null }), - ).toBeNull() + expect(buildPodcastEpisodeStructuredData(episode)).toBeNull() + expect(buildPodcastEpisodeStructuredData(undefined)).toBeNull() }) test.each(["PT17M16S", "PT0S", "PT1H13M44S", "P1D", "P1Y2M3DT4H5M6.7S"])( "keeps the valid ISO-8601 duration %s", (duration) => { - const built = buildPodcastEpisodeStructuredData(makeEpisode({ duration }), { - series: null, - }) + const built = buildPodcastEpisodeStructuredData(makeEpisode({ duration })) expect(built).toHaveProperty("duration", duration) }, ) @@ -39,27 +45,29 @@ test.each(["PT17M16S", "PT0S", "PT1H13M44S", "P1D", "P1Y2M3DT4H5M6.7S"])( test.each(["P", "PT", "P1DT", "17 minutes", "1:13:44", ""])( "drops the invalid duration %p rather than publishing it", (duration) => { - const built = buildPodcastEpisodeStructuredData(makeEpisode({ duration }), { - series: null, - }) + const built = buildPodcastEpisodeStructuredData(makeEpisode({ duration })) expect(built).not.toHaveProperty("duration") }, ) -test("names the series it is given, not one it picks itself", () => { - const episode = makeEpisode({ - parent_podcasts: [ - { id: 1, title: "Podcast A", readable_id: "a" }, - { id: 2, title: "Podcast B", readable_id: "b" }, - ], - }) - const built = buildPodcastEpisodeStructuredData(episode, { - series: { id: 2, title: "Podcast B", readable_id: "b" }, - seriesUrl: "https://learn.mit.edu/podcast/2/podcast-b", - }) +test("names the canonical series, not a later parent", () => { + const canonical = makeParent() + const other = makeParent() + const episode = makeEpisode({ parent_podcasts: [canonical, other] }) + + const built = buildPodcastEpisodeStructuredData(episode) + + expect(built).toHaveProperty("url", episode.learn_url) expect(built).toHaveProperty("partOfSeries", { "@type": "PodcastSeries", - name: "Podcast B", - url: "https://learn.mit.edu/podcast/2/podcast-b", + name: canonical.title, + url: canonical.learn_url, }) }) + +test("omits partOfSeries for an episode with no parent podcast", () => { + const built = buildPodcastEpisodeStructuredData( + makeEpisode({ parent_podcasts: [] }), + ) + expect(built).not.toHaveProperty("partOfSeries") +}) diff --git a/frontends/main/src/app-pages/PodcastPage/podcastEpisodeStructuredData.ts b/frontends/main/src/app-pages/PodcastPage/podcastEpisodeStructuredData.ts index f7481b0f87..3252ac3e9f 100644 --- a/frontends/main/src/app-pages/PodcastPage/podcastEpisodeStructuredData.ts +++ b/frontends/main/src/app-pages/PodcastPage/podcastEpisodeStructuredData.ts @@ -7,24 +7,14 @@ import type { PodcastEpisodeParent, PodcastEpisodeResource } from "api/v1" const ISO_8601_DURATION_RE = /^P(?!$)(?:\d+Y)?(?:\d+M)?(?:\d+W)?(?:\d+D)?(?:T(?!$)(?:\d+H)?(?:\d+M)?(?:\d+(?:\.\d+)?S)?)?$/ -type BuildOptions = { - /** Absolute canonical url of the episode page */ - url?: string - /** - * The parent podcast series the episode is being viewed under, as resolved - * by `getEpisodeParentPodcast`. Required rather than picked from - * `parent_podcasts[0]` here: an episode can belong to several series, and - * choosing one independently of the caller would pair that series' name with - * `seriesUrl`, which the caller builds from the parent in the current url. - */ - series: PodcastEpisodeParent | null - /** Absolute canonical url of the parent podcast page */ - seriesUrl?: string -} - /** * Builds a schema.org PodcastEpisode structured-data payload. * + * Every URL here is canonical, taken from the episode rather than from the page + * rendering it. An episode in several podcasts is viewable under any of them, so + * a page that passed in its own parent would pair one series' name with another + * series' `url`. `parent_podcasts[0]` is the parent `learn_url` was built from. + * * The transcript text itself is deliberately not included. `schema.org`'s * `transcript` property has a `domainIncludes` of `AudioObject` and * `VideoObject` only -- it is not a `PodcastEpisode` property -- and no Google @@ -38,11 +28,11 @@ type BuildOptions = { */ export function buildPodcastEpisodeStructuredData( episode: PodcastEpisodeResource | undefined, - { url, series, seriesUrl }: BuildOptions, ): Record | null { if (!episode || !episode.last_modified) return null const details = episode.podcast_episode + const series: PodcastEpisodeParent | undefined = details?.parent_podcasts?.[0] const durationIso = details?.duration && ISO_8601_DURATION_RE.test(details.duration) @@ -54,7 +44,7 @@ export function buildPodcastEpisodeStructuredData( "@type": "PodcastEpisode", name: episode.title, ...(episode.description ? { description: episode.description } : {}), - ...(url ? { url } : {}), + url: episode.learn_url, datePublished: episode.last_modified, ...(episode.image?.url ? { image: episode.image.url } : {}), ...(durationIso ? { duration: durationIso } : {}), @@ -71,7 +61,7 @@ export function buildPodcastEpisodeStructuredData( partOfSeries: { "@type": "PodcastSeries", name: series.title, - ...(seriesUrl ? { url: seriesUrl } : {}), + url: series.learn_url, }, } : {}), diff --git a/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.test.tsx b/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.test.tsx index f5f91fe2a4..d336183032 100644 --- a/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.test.tsx +++ b/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.test.tsx @@ -31,7 +31,7 @@ describe("CertificateTrackCard", () => { ).toBeInTheDocument() expect(screen.getByText("Graded assignments & exams")).toBeInTheDocument() expect( - screen.getByText("MIT certificate on completion"), + screen.getByText("MIT Open Learning certificate of completion"), ).toBeInTheDocument() }) diff --git a/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.tsx b/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.tsx index f8e5529f56..85cee2b0ac 100644 --- a/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.tsx +++ b/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.tsx @@ -92,7 +92,7 @@ const CertificateTrackCard: React.FC = ({ ) diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/FeaturedVideo.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/FeaturedVideo.tsx index 5cea5f2bbd..a76694278f 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/FeaturedVideo.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/FeaturedVideo.tsx @@ -130,8 +130,27 @@ const FeaturedTitle = styled.h2(({ theme }) => ({ }, })) -const FeaturedDescription = styled.p(({ theme }) => ({ +/* + * A div, not a p: OVS descriptions are rich text, and

/