From ee91632a44b4bdcecaa0e861210b53ad0091979d Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Thu, 2 Jul 2026 19:20:08 -0400 Subject: [PATCH 1/8] Upgrade django-health-check from pinned git commit to PyPI 3.24.0 (#3556) * Upgrade django-health-check from pinned git commit to PyPI 3.24.0 Replaces the stale revsys/django-health-check@53f9bdc3 (dev build of 3.20.1) git dependency with the newest PyPI release compatible with Django==4.2.30: 3.24.0. 4.0.0+ requires Django>=5.2 so isn't available here yet. Verified the settings.py integration (bare check class names in HEALTH_CHECK["SUBSETS"], contrib.migrations/celery_ping/redis/db_heartbeat apps, health_check.urls include) is unaffected other than new deprecation warnings for APIs slated for removal in v4. * Fix silently-broken health check SUBSETS matching after 3.24.0 bump The previous commit upgraded django-health-check from a stale git pin to PyPI 3.24.0, which (unbeknownst at the time) silently broke this repo's HEALTH_CHECK["SUBSETS"] configuration: 3.24.0's health_check/__init__.py now does `Cache.__qualname__ = "Cache"` (and similarly for Database) as a global, process-wide mutation of the shared CacheBackend/ DatabaseHeartBeatCheck classes, so their dataclass-generated __repr__ no longer matches the bare class names ("CacheBackend", "DatabaseHeartBeatCheck") used in SUBSETS. Confirmed empirically: only "MigrationsHealthCheck" (the one check with no reprable fields and no top-level alias) still matched; Cache, Database, Redis, and CeleryPing were silently excluded from every subset, meaning /health/liveness/, /health/readiness/, and most of /health/startup/ and /health/full/ were vacuously always-200 regardless of actual DB/cache/redis/celery status. This was a regression introduced by the dependency bump, not a pre-existing issue: the old pinned commit's health_check/__init__.py did no such aliasing. Also, every one of the old INSTALLED_APPS contrib sub-apps (health_check.contrib.{redis,db_heartbeat,celery_ping,migrations}) emits its own DeprecationWarning at app-ready time pointing at this exact fix: 'checks are now configured via HealthCheckView... add the appropriate check to your HealthCheckView.checks.' Replaces the whole SUBSETS + INSTALLED_APPS-registration mechanism with the explicit, non-fragile HealthCheckView(checks=[...]) API (already available in 3.24.0, not just v4), matching the pattern already used in mitxonline/micromasters/ocw-studio/mitxpro. Also fixes the CacheBackend check to target the "redis" cache alias instead of the always-available in-memory "default" alias, which made it a no-op regardless of the SUBSETS bug. Verified live via Django's test client: /health/liveness/ returns 200 {"Database(alias='default')": "OK"} against sqlite, and /health/readiness/ correctly returns a 500 ServiceUnavailable when pointed at an unreachable Redis instead of silently passing. * Add bare /health/ route and URL-resolution tests Addresses Copilot review feedback: the rewritten urls_healthcheck.py only defined /health/{startup,liveness,readiness,full}/ subset routes, dropping the bare /health/ index that include("health_check.urls") previously provided. Added it back, mapped to the full check list. Also adds main/urls_healthcheck_test.py asserting all five paths resolve to HealthCheckView. Resolves directly against the main.urls_healthcheck sub-urlconf (via get_resolver) rather than django.urls.resolve() against the full project urlconf, since the latter transitively imports the tika client (via learning_resources' ETL utils) for the first time in a fresh test process, which triggers a pkg_resources.declare_namespace() DeprecationWarning that this repo's autouse warnings-as-errors fixture turns into a failure -- a pre-existing, unrelated issue in the tika dependency, not something to paper over by scoping down what this test actually needs to exercise. --- main/settings.py | 38 ------------------------ main/urls.py | 4 +-- main/urls_healthcheck.py | 55 +++++++++++++++++++++++++++++++++++ main/urls_healthcheck_test.py | 23 +++++++++++++++ pyproject.toml | 5 +--- uv.lock | 11 +++++-- 6 files changed, 89 insertions(+), 47 deletions(-) create mode 100644 main/urls_healthcheck.py create mode 100644 main/urls_healthcheck_test.py diff --git a/main/settings.py b/main/settings.py index f94671af24..0e818b7ee1 100644 --- a/main/settings.py +++ b/main/settings.py @@ -140,48 +140,10 @@ "ol_hubspot", "mitol.scim.apps.ScimApp", "health_check", - "health_check.cache", - "health_check.contrib.migrations", - "health_check.contrib.celery_ping", - "health_check.contrib.redis", - "health_check.contrib.db_heartbeat", ) WEBHOOK_SECRET = get_string("WEBHOOK_SECRET", "please-change-this") -HEALTH_CHECK = { - "SUBSETS": { - # The 'startup' subset includes checks that must pass before the application can - # start. - "startup": [ - "MigrationsHealthCheck", # Ensures database migrations are applied. - "CacheBackend", # Verifies the cache backend is operational. - "RedisHealthCheck", # Confirms Redis is reachable and functional. - "DatabaseHeartBeatCheck", # Checks the database connection is alive. - ], - # The 'liveness' subset includes checks to determine if the application is - # running. - "liveness": ["DatabaseHeartBeatCheck"], # Minimal check to ensure the app is - # alive. - # The 'readiness' subset includes checks to determine if the application is - # ready to serve requests. - "readiness": [ - "CacheBackend", # Ensures the cache is ready for use. - "RedisHealthCheck", # Confirms Redis is ready for use. - "DatabaseHeartBeatCheck", # Verifies the database is ready for queries. - ], - # The 'full' subset includes all available health checks for a comprehensive - # status report. - "full": [ - "MigrationsHealthCheck", # Ensures database migrations are applied. - "CacheBackend", # Verifies the cache backend is operational. - "RedisHealthCheck", # Confirms Redis is reachable and functional. - "DatabaseHeartBeatCheck", # Checks the database connection is alive. - "CeleryPingHealthCheck", # Verifies Celery workers are responsive. - ], - } -} - if not get_bool("RUN_DATA_MIGRATIONS", default=False): MIGRATION_MODULES = {"data_fixtures": None} diff --git a/main/urls.py b/main/urls.py index dda4804b82..391ffa9ae2 100644 --- a/main/urls.py +++ b/main/urls.py @@ -17,7 +17,7 @@ from django.conf import settings from django.conf.urls.static import static from django.contrib import admin -from django.urls import include, re_path +from django.urls import include, path, re_path from django.views.generic.base import RedirectView from rest_framework.routers import DefaultRouter @@ -60,7 +60,7 @@ re_path(r"", include("webhooks.urls", namespace="webhooks")), re_path(r"", include(features_router.urls)), re_path(r"^app", RedirectView.as_view(url=settings.APP_BASE_URL)), - re_path(r"^health/", include("health_check.urls")), + path("", include("main.urls_healthcheck")), ] + static(settings.MEDIA_URL, document_root=settings.MEDIA_ROOT) + static(settings.STATIC_URL, document_root=settings.STATIC_ROOT) diff --git a/main/urls_healthcheck.py b/main/urls_healthcheck.py new file mode 100644 index 0000000000..e4f95e3980 --- /dev/null +++ b/main/urls_healthcheck.py @@ -0,0 +1,55 @@ +"""Healthcheck urls""" + +from django.urls import include, path +from health_check.views import HealthCheckView + +MIGRATIONS_CHECK = "health_check.contrib.migrations.backends.MigrationsHealthCheck" + +BASE_CHECKS = [ + # "default" is in-memory and always available; "redis" is the real backing cache. + ("health_check.Cache", {"alias": "redis"}), + "health_check.Database", + "health_check.contrib.redis.Redis", +] + +urlpatterns = [ + path( + "health/", + include( + [ + path( + "", + HealthCheckView.as_view( + checks=[ + *BASE_CHECKS, + MIGRATIONS_CHECK, + "health_check.contrib.celery.Ping", + ] + ), + ), + path( + "startup/", + HealthCheckView.as_view(checks=[*BASE_CHECKS, MIGRATIONS_CHECK]), + ), + path( + "liveness/", + HealthCheckView.as_view(checks=["health_check.Database"]), + ), + path( + "readiness/", + HealthCheckView.as_view(checks=[*BASE_CHECKS]), + ), + path( + "full/", + HealthCheckView.as_view( + checks=[ + *BASE_CHECKS, + MIGRATIONS_CHECK, + "health_check.contrib.celery.Ping", + ] + ), + ), + ] + ), + ), +] diff --git a/main/urls_healthcheck_test.py b/main/urls_healthcheck_test.py new file mode 100644 index 0000000000..24afbdc199 --- /dev/null +++ b/main/urls_healthcheck_test.py @@ -0,0 +1,23 @@ +"""Tests for the healthcheck urls""" + +import pytest +from django.urls import get_resolver +from health_check.views import HealthCheckView + + +@pytest.mark.parametrize( + "path", + [ + "/health/", + "/health/startup/", + "/health/liveness/", + "/health/readiness/", + "/health/full/", + ], +) +def test_healthcheck_urls_resolve(path): + """All healthcheck endpoints should resolve to HealthCheckView""" + # Resolve directly against this urlconf module rather than django.urls.resolve() + # (which walks the full project urlconf) to avoid pulling in unrelated apps. + match = get_resolver("main.urls_healthcheck").resolve(path) + assert match.func.view_class is HealthCheckView diff --git a/pyproject.toml b/pyproject.toml index bbdc431c71..c9fcee4bde 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -32,7 +32,7 @@ dependencies = [ "django-cors-headers>=4.0.0,<5", "django-filter>=2.4.0,<3", "django-guardian>=3.0.0,<4", - "django-health-check", + "django-health-check>=3.24.0,<4", "django-imagekit>=6.0.0,<7", "django-ipware>=7.0.0,<8", "django-json-widget>=2.0.0,<3", @@ -154,9 +154,6 @@ package = false default-groups = "all" override-dependencies = ["setuptools<80"] -[tool.uv.sources] -django-health-check = { git = "https://github.com/revsys/django-health-check", rev = "53f9bdc3a7acc8a577319987fef0bd3040eef4b4" } # pragma: allowlist secret - [tool.uv.build-backend] module-root = "" diff --git a/uv.lock b/uv.lock index 1bed38aa9e..dc3f607790 100644 --- a/uv.lock +++ b/uv.lock @@ -833,10 +833,15 @@ wheels = [ [[package]] name = "django-health-check" -version = "3.20.1.dev10+g53f9bdc3a" -source = { git = "https://github.com/revsys/django-health-check?rev=53f9bdc3a7acc8a577319987fef0bd3040eef4b4#53f9bdc3a7acc8a577319987fef0bd3040eef4b4" } +version = "3.24.0" +source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "django" }, + { name = "psutil" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/98/a6/f474f443b0a7f9e7e52a25a3773a31c5c061bdb28cf311b7801fc250db9b/django_health_check-3.24.0.tar.gz", hash = "sha256:b5e01d2013a254cc5a2c7b19c62f11ea6fd2624c87a9d990e11a1b3874df78b5", size = 20807, upload-time = "2026-02-12T10:02:18.482Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/38/17/51aabb4908e007accf3c596b92a62ed7e456c581a1e45f06e0f2219dc6c0/django_health_check-3.24.0-py3-none-any.whl", hash = "sha256:bd568d7d3813980668dfbe730214aef5da7c8da73bc2aa60ec2eeca8a3788da6", size = 37964, upload-time = "2026-02-12T10:02:17.08Z" }, ] [[package]] @@ -2705,7 +2710,7 @@ requires-dist = [ { name = "django-cors-headers", specifier = ">=4.0.0,<5" }, { name = "django-filter", specifier = ">=2.4.0,<3" }, { name = "django-guardian", specifier = ">=3.0.0,<4" }, - { name = "django-health-check", git = "https://github.com/revsys/django-health-check?rev=53f9bdc3a7acc8a577319987fef0bd3040eef4b4" }, + { name = "django-health-check", specifier = ">=3.24.0,<4" }, { name = "django-imagekit", specifier = ">=6.0.0,<7" }, { name = "django-ipware", specifier = ">=7.0.0,<8" }, { name = "django-json-widget", specifier = ">=2.0.0,<3" }, From 60d5e19fb99dc89ca819ef4487de1719daa9eafb Mon Sep 17 00:00:00 2001 From: Danielle Frappier Date: Mon, 6 Jul 2026 09:26:37 -0400 Subject: [PATCH 2/8] feat: Add "Send Test Email to Me" to the seat assignment confirmation modal (#3558) --- .../mitxonline/hooks/organizations/index.ts | 10 ++ .../api/src/mitxonline/test-utils/urls.ts | 2 + .../AssignSeatsConfirmModal.test.tsx | 155 +++++++++++++++++- .../AssignSeatsConfirmModal.tsx | 104 +++++++++++- .../AssignSeatsSection.test.tsx | 99 +++++++++++ .../ContractAdminPage/AssignSeatsSection.tsx | 21 ++- .../ContractAdminPage.test.tsx | 4 + 7 files changed, 392 insertions(+), 3 deletions(-) diff --git a/frontends/api/src/mitxonline/hooks/organizations/index.ts b/frontends/api/src/mitxonline/hooks/organizations/index.ts index 4b051a2bac..8eb8c68c91 100644 --- a/frontends/api/src/mitxonline/hooks/organizations/index.ts +++ b/frontends/api/src/mitxonline/hooks/organizations/index.ts @@ -6,6 +6,7 @@ import { B2bApiB2bManagerOrganizationsContractsCodesReassignUpdateRequest, B2bApiB2bManagerOrganizationsContractsCodesRemindCreateRequest, B2bApiB2bManagerOrganizationsContractsCodesRevokeDestroyRequest, + B2bApiB2bManagerOrganizationsContractsCodesSendTestEmailCreateRequest, } from "@mitodl/mitxonline-api-axios/v2" import { organizationQueries, @@ -106,6 +107,14 @@ const useReassignCode = () => { }) } +/** Send a test enrollment code to the given email address. */ +const useSendTestEmail = () => + useMutation({ + mutationFn: ( + opts: B2bApiB2bManagerOrganizationsContractsCodesSendTestEmailCreateRequest, + ) => b2bApi.b2bManagerOrganizationsContractsCodesSendTestEmailCreate(opts), + }) + export { organizationQueries, managerOrganizationQueries, @@ -114,6 +123,7 @@ export { useReassignCode, useRemindCode, useRevokeCode, + useSendTestEmail, } export type { ManagerEnrollmentCode, diff --git a/frontends/api/src/mitxonline/test-utils/urls.ts b/frontends/api/src/mitxonline/test-utils/urls.ts index 9fb471e41f..3983dc05b6 100644 --- a/frontends/api/src/mitxonline/test-utils/urls.ts +++ b/frontends/api/src/mitxonline/test-utils/urls.ts @@ -128,6 +128,8 @@ const contracts = { code: string, ) => `${getApiBaseUrl()}/api/v0/b2b/manager/organizations/${orgId}/contracts/${contractId}/codes/${code}/reassign/`, + managerContractSendTestEmail: (orgId: number, contractId: number) => + `${getApiBaseUrl()}/api/v0/b2b/manager/organizations/${orgId}/contracts/${contractId}/codes/send_test_email/`, } const certificates = { diff --git a/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.test.tsx b/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.test.tsx index 68059f64ac..9f393e9635 100644 --- a/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.test.tsx +++ b/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.test.tsx @@ -1,5 +1,5 @@ import React, { act } from "react" -import { renderWithTheme, screen, user } from "@/test-utils" +import { renderWithTheme, screen, user, waitFor } from "@/test-utils" import { AssignSeatsConfirmModal } from "./AssignSeatsConfirmModal" const baseProps = { @@ -297,6 +297,159 @@ describe("AssignSeatsConfirmModal — over-capacity state (CSV only)", () => { }) }) +describe("AssignSeatsConfirmModal — email preview (send test email)", () => { + const sendTestEmailProps = { + ...baseProps, + userEmail: "manager@test.com", + onSendTestEmail: jest.fn(), + } + + beforeEach(() => jest.clearAllMocks()) + + test("does not show Email Preview section when onSendTestEmail is not provided", () => { + renderWithTheme() + + expect(screen.queryByText("Email Preview")).not.toBeInTheDocument() + expect( + screen.queryByRole("button", { name: /send test email to me/i }), + ).not.toBeInTheDocument() + }) + + test("shows Email Preview section with button when onSendTestEmail is provided", () => { + renderWithTheme() + + expect(screen.getByText("Email Preview")).toBeInTheDocument() + expect( + screen.getByRole("button", { name: /send test email to me/i }), + ).toBeInTheDocument() + }) + + test("button is disabled when userEmail is not provided", () => { + renderWithTheme( + , + ) + + expect( + screen.getByRole("button", { name: /send test email to me/i }), + ).toBeDisabled() + }) + + test("shows 'Sending…' while test email is being sent", async () => { + const { promise, resolve } = Promise.withResolvers() + renderWithTheme( + promise} + />, + ) + + await user.click( + screen.getByRole("button", { name: /send test email to me/i }), + ) + + expect( + screen.getByRole("button", { name: /sending…/i }), + ).toBeInTheDocument() + + await act(async () => resolve()) + }) + + test("shows success alert after test email is sent successfully", async () => { + renderWithTheme( + , + ) + + await user.click( + screen.getByRole("button", { name: /send test email to me/i }), + ) + + expect(await screen.findByRole("alert")).toHaveTextContent( + /test email successfully sent to manager@test\.com/i, + ) + }) + + test("shows error alert when test email fails", async () => { + renderWithTheme( + , + ) + + await user.click( + screen.getByRole("button", { name: /send test email to me/i }), + ) + + expect(await screen.findByRole("alert")).toHaveTextContent( + /something went wrong sending the test email/i, + ) + }) + + test("Email Preview section is not shown in review step", () => { + renderWithTheme( + , + ) + + expect(screen.queryByText("Email Preview")).not.toBeInTheDocument() + }) + + test("Email Preview section is not shown in over-capacity state", () => { + renderWithTheme( + , + ) + + expect(screen.queryByText("Email Preview")).not.toBeInTheDocument() + }) + + test("resets to idle state when modal is reopened", async () => { + const { rerender } = renderWithTheme( + , + ) + + await user.click( + screen.getByRole("button", { name: /send test email to me/i }), + ) + await screen.findByRole("alert") + + // Close then reopen + rerender( + , + ) + rerender( + , + ) + + await waitFor(() => + expect(screen.queryByRole("alert")).not.toBeInTheDocument(), + ) + expect( + screen.getByRole("button", { name: /send test email to me/i }), + ).not.toBeDisabled() + }) +}) + describe("AssignSeatsConfirmModal — copy buttons", () => { afterEach(() => { Object.defineProperty(navigator, "clipboard", { diff --git a/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.tsx b/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.tsx index 5c79f65837..fccd2bea59 100644 --- a/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.tsx +++ b/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.tsx @@ -9,7 +9,7 @@ import { alpha, styled, } from "ol-components" -import { Button, VisuallyHidden } from "@mitodl/smoot-design" +import { Alert, Button, VisuallyHidden } from "@mitodl/smoot-design" import { pluralize } from "ol-utilities" import { RiAlertFill, @@ -157,6 +157,26 @@ const StatLabel = styled(Typography)(({ theme }) => ({ width: "100%", })) as typeof Typography +const EmailPreviewLabel = styled(Typography)(({ theme }) => ({ + ...theme.typography.subtitle2, + color: theme.custom.colors.darkGray2, +})) as typeof Typography + +// ─── Confirm-step email preview block ────────────────────────────────────────── + +const EmailPreviewSection = styled("div")({ + display: "flex", + flexDirection: "column", + gap: "8px", +}) + +const EmailPreviewRow = styled("div")({ + display: "flex", + alignItems: "center", + justifyContent: "space-between", + gap: "16px", +}) + // ─── Small helpers ──────────────────────────────────────────────────────────── const SHOW_MORE_THRESHOLD = 3 @@ -214,6 +234,8 @@ const copyToClipboard = async (text: string): Promise => { // ─── Main component ─────────────────────────────────────────────────────────── +type TestEmailStatus = "idle" | "sending" | "success" | "error" + type AssignSeatsConfirmModalProps = { open: boolean onClose: () => void @@ -223,6 +245,13 @@ type AssignSeatsConfirmModalProps = { invalidEmails: string[] duplicateEmails: string[] skippedCount: number + /** Email of the logged-in user, shown in the test-email success message. */ + userEmail?: string | null + /** + * When provided, renders the "Email Preview" section with a "Send Test Email + * to Me" button that calls this handler. Omit to hide the section entirely. + */ + onSendTestEmail?: () => Promise } type Step = "review" | "confirm" @@ -236,6 +265,8 @@ const AssignSeatsConfirmModal: React.FC = ({ invalidEmails, duplicateEmails, skippedCount, + userEmail, + onSendTestEmail, }) => { const descriptionId = useId() const overCapacitySummaryId = `${descriptionId}-summary` @@ -252,6 +283,8 @@ const AssignSeatsConfirmModal: React.FC = ({ const [copiedDuplicate, setCopiedDuplicate] = useState(false) const [isSubmitting, setIsSubmitting] = useState(false) const [sendError, setSendError] = useState(null) + const [testEmailStatus, setTestEmailStatus] = + useState("idle") // Assertive live region text for step transitions. Using reset-then-set (100ms // delay) so NVDA picks up the content change cleanly after focus settles on the // new step's first focusable element inside the same persistent dialog. @@ -268,6 +301,10 @@ const AssignSeatsConfirmModal: React.FC = ({ const sendErrorAnnouncementTimerRef = useRef | null>(null) + const testEmailAnnouncementTimerRef = useRef | null>(null) + const [testEmailAnnouncement, setTestEmailAnnouncement] = useState("") // Only reset step and copy state when the dialog transitions from closed to // open. Without prevOpenRef the effect would also fire when hasIssues or @@ -283,6 +320,8 @@ const AssignSeatsConfirmModal: React.FC = ({ setSendError(null) setSendErrorAnnouncement("") setStepAnnouncement("") + setTestEmailStatus("idle") + setTestEmailAnnouncement("") } }, [open, hasIssues, overCapacity]) @@ -295,6 +334,8 @@ const AssignSeatsConfirmModal: React.FC = ({ clearTimeout(stepAnnouncementTimerRef.current) if (sendErrorAnnouncementTimerRef.current) clearTimeout(sendErrorAnnouncementTimerRef.current) + if (testEmailAnnouncementTimerRef.current) + clearTimeout(testEmailAnnouncementTimerRef.current) } }, []) @@ -321,6 +362,34 @@ const AssignSeatsConfirmModal: React.FC = ({ ) } + const handleSendTestEmail = async () => { + if (!onSendTestEmail) return + setTestEmailStatus("sending") + try { + await onSendTestEmail() + setTestEmailStatus("success") + const msg = `Test email successfully sent to ${userEmail}.` + setTestEmailAnnouncement("") + if (testEmailAnnouncementTimerRef.current) + clearTimeout(testEmailAnnouncementTimerRef.current) + testEmailAnnouncementTimerRef.current = setTimeout( + () => setTestEmailAnnouncement(msg), + 100, + ) + } catch { + setTestEmailStatus("error") + const msg = + "Something went wrong sending the test email. Please try again." + setTestEmailAnnouncement("") + if (testEmailAnnouncementTimerRef.current) + clearTimeout(testEmailAnnouncementTimerRef.current) + testEmailAnnouncementTimerRef.current = setTimeout( + () => setTestEmailAnnouncement(msg), + 100, + ) + } + } + const handleSend = async () => { setIsSubmitting(true) setSendError(null) @@ -470,6 +539,10 @@ const AssignSeatsConfirmModal: React.FC = ({ {sendErrorAnnouncement} + {/* Test email announcement — same reset-then-set pattern for NVDA */} + + {testEmailAnnouncement} + {/* Copy-success announcement — polite so it doesn't interrupt ongoing speech */} {copiedInvalid @@ -603,6 +676,35 @@ const AssignSeatsConfirmModal: React.FC = ({ receive an email with secure link to claim their seat and access the materials. + {onSendTestEmail && ( + + Email Preview + + + This is the email learners will receive. + + + + + )} + {onSendTestEmail && testEmailStatus === "success" && ( + + Test email successfully sent to {userEmail}. + + )} + {onSendTestEmail && testEmailStatus === "error" && ( + + Something went wrong sending the test email. Please try again. + + )} { let originalFileReader: typeof FileReader + beforeEach(() => { + setMockResponse.get( + urls.userMe.get(), + factories.user.user({ email: USER_EMAIL }), + ) + }) + beforeAll(() => { originalFileReader = window.FileReader @@ -555,6 +564,96 @@ describe("AssignSeatsSection", () => { expect(screen.getByText("1 invalid")).toBeInTheDocument() }) + describe("send test email", () => { + const ORG_ID = 1 + const CONTRACT_ID = 2 + + const openConfirmModal = async () => { + const textarea = screen.getByPlaceholderText(/enter employee emails/i) + await user.type(textarea, "alice@example.com") + await user.click(screen.getByRole("button", { name: "Assign Seats" })) + await screen.findByRole("heading", { name: /ready to send invitations/i }) + } + + test("shows 'Send Test Email to Me' button in the confirm modal", async () => { + renderWithProviders( + , + ) + + await openConfirmModal() + + await waitFor(() => + expect( + screen.getByRole("button", { name: /send test email to me/i }), + ).not.toBeDisabled(), + ) + }) + + test("sends test email to the user's address and shows success alert", async () => { + setMockResponse.post( + urls.contracts.managerContractSendTestEmail(ORG_ID, CONTRACT_ID), + null, + ) + renderWithProviders( + , + ) + + await openConfirmModal() + await waitFor(() => + expect( + screen.getByRole("button", { name: /send test email to me/i }), + ).not.toBeDisabled(), + ) + await user.click( + screen.getByRole("button", { name: /send test email to me/i }), + ) + + expect(await screen.findByRole("alert")).toHaveTextContent( + new RegExp(`test email successfully sent to ${USER_EMAIL}`, "i"), + ) + }) + + test("shows error alert when test email request fails", async () => { + setMockResponse.post( + urls.contracts.managerContractSendTestEmail(ORG_ID, CONTRACT_ID), + { detail: "Internal server error" }, + { code: 500 }, + ) + renderWithProviders( + , + ) + + await openConfirmModal() + await waitFor(() => + expect( + screen.getByRole("button", { name: /send test email to me/i }), + ).not.toBeDisabled(), + ) + await user.click( + screen.getByRole("button", { name: /send test email to me/i }), + ) + + expect(await screen.findByRole("alert")).toHaveTextContent( + /something went wrong sending the test email/i, + ) + }) + }) + describe("submitting the assignment", () => { const ORG_ID = 1 const CONTRACT_ID = 2 diff --git a/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsSection.tsx b/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsSection.tsx index 59ddb8e3d7..d44842ad32 100644 --- a/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsSection.tsx +++ b/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsSection.tsx @@ -11,7 +11,12 @@ import { } from "ol-utilities" import Papa from "papaparse" import { AssignSeatsConfirmModal } from "./AssignSeatsConfirmModal" -import { useBulkAssignSeats } from "api/mitxonline-hooks/organizations" +import { + useBulkAssignSeats, + useSendTestEmail, +} from "api/mitxonline-hooks/organizations" +import { mitxUserQueries } from "api/mitxonline-hooks/user" +import { useQuery } from "@tanstack/react-query" import type { BulkAssignError } from "@mitodl/mitxonline-api-axios/v2" // Shared metrics — must be identical between EmailHighlightLayer and EmailTextarea @@ -227,6 +232,8 @@ const AssignSeatsSection: React.FC = ({ const fileInputRef = useRef(null) const bulkAssign = useBulkAssignSeats() + const sendTestEmail = useSendTestEmail() + const { data: user } = useQuery(mitxUserQueries.me()) const submitResult = useMemo( () => parseEmailsForSubmit(emailInput), @@ -392,6 +399,16 @@ const AssignSeatsSection: React.FC = ({ const handleModalClose = () => setModalData(null) + const handleSendTestEmail = async () => { + const email = user?.email + if (!email) throw new Error("Logged-in user email is unavailable") + await sendTestEmail.mutateAsync({ + id: contractId, + parent_lookup_organization: orgId, + SendTestEmailRequest: { email }, + }) + } + const handleModalConfirm = async () => { const emails = modalData?.validEmails ?? [] if (emails.length === 0) return @@ -612,6 +629,8 @@ const AssignSeatsSection: React.FC = ({ invalidEmails={modalData.invalidEmails} duplicateEmails={modalData.duplicateEmails} skippedCount={modalData.skippedCount} + userEmail={user?.email} + onSendTestEmail={handleSendTestEmail} /> )} diff --git a/frontends/main/src/app-pages/ContractAdminPage/ContractAdminPage.test.tsx b/frontends/main/src/app-pages/ContractAdminPage/ContractAdminPage.test.tsx index 7809438201..305a5cd605 100644 --- a/frontends/main/src/app-pages/ContractAdminPage/ContractAdminPage.test.tsx +++ b/frontends/main/src/app-pages/ContractAdminPage/ContractAdminPage.test.tsx @@ -57,6 +57,10 @@ describe("ContractAdminPage", () => { beforeEach(() => { mockedUseFeatureFlagsLoaded.mockReturnValue(false) mockedUseFeatureFlagEnabled.mockReturnValue(undefined) + setMockResponse.get( + urls.userMe.get(), + factories.user.user({ email: "manager@test.com" }), + ) }) test("throws ForbiddenError when feature flag is disabled", () => { From 48dbbd924be573c4523a4911f0d52730cb724636 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Mon, 6 Jul 2026 15:58:53 -0400 Subject: [PATCH 3/8] fix(vector-search): relax qdrant-client pin to allow 1.18.x patches (#3546) Production pods log repeated DecodeError on qdrant.QueryResponse via gRPC (prefer_grpc=True, cloud_inference=True) against qdrant server v1.18.2 (see ol-infrastructure QDRANT_VERSION). qdrant-client 1.18.0 is the latest PyPI release; the previous ==1.18.0 exact pin prevented automatic pickup of any future 1.18.x patch that the qdrant team may publish to address the server-side incompatibility. Switch to the compatible-release specifier (~=1.18.0) so uv can resolve patch upgrades within 1.18.x when they become available. The lockfile still resolves to 1.18.0 today. Minimum server version for the Query API (QueryResponse gRPC type): v1.7.0. Deployed server: v1.18.2. Note: if the qdrant team does not publish a matching client patch, the server in ol-infrastructure should be pinned back to v1.18.0 until a compatible client is released, or the gRPC transport should be replaced with HTTP/REST (prefer_grpc=False in vector_search/utils.py) as an interim workaround. Co-authored-by: Claude Sonnet 4.6 --- pyproject.toml | 9 ++++++++- uv.lock | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index c9fcee4bde..7fe8ad81ac 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -88,7 +88,14 @@ dependencies = [ "python-dateutil>=2.8.2,<3", "python-rapidjson>=1.8,<2", "pyyaml>=6.0.0,<7", - "qdrant-client[fastembed]==1.18.0,<2", + # Minimum server version for the Query API (QueryResponse gRPC type): qdrant v1.7.0. + # qdrant-client ~=1.18.0 targets qdrant server v1.18.x; deployed server is v1.18.2 + # (ol-infrastructure/src/bridge/lib/versions.py QDRANT_VERSION). + # The prior ==1.18.0 pin blocked automatic pickup of future 1.18.x patch releases; + # switching to ~= lets uv upgrade within 1.18.x when the qdrant team publishes a + # client that resolves the gRPC QueryResponse DecodeError observed against server + # v1.18.2 with cloud_inference=True + prefer_grpc=True. + "qdrant-client[fastembed]~=1.18.0", "redis>=7.0.0,<8", "requests>=2.31.0,<3", "retry2>=0.9.5,<0.10", diff --git a/uv.lock b/uv.lock index dc3f607790..9036a099de 100644 --- a/uv.lock +++ b/uv.lock @@ -2778,7 +2778,7 @@ requires-dist = [ { name = "python-dateutil", specifier = ">=2.8.2,<3" }, { name = "python-rapidjson", specifier = ">=1.8,<2" }, { name = "pyyaml", specifier = ">=6.0.0,<7" }, - { name = "qdrant-client", extras = ["fastembed"], specifier = "==1.18.0,<2" }, + { name = "qdrant-client", extras = ["fastembed"], specifier = "~=1.18.0" }, { name = "redis", specifier = ">=7.0.0,<8" }, { name = "requests", specifier = ">=2.31.0,<3" }, { name = "retry2", specifier = ">=0.9.5,<0.10" }, From 17d6311df6e54637e24a20a9d56132c7472cb8e0 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Mon, 6 Jul 2026 18:25:02 -0400 Subject: [PATCH 4/8] Bound program marketing-page embedding input to avoid token overflow (#3562) --- learning_resources/tasks_test.py | 8 ++ learning_resources/utils.py | 121 +++++++----------- learning_resources/utils_test.py | 204 +++++++++++++++++-------------- vector_search/utils.py | 11 +- vector_search/utils_test.py | 95 ++++++++++++++ 5 files changed, 271 insertions(+), 168 deletions(-) diff --git a/learning_resources/tasks_test.py b/learning_resources/tasks_test.py index 0797411892..b611d0f5c8 100644 --- a/learning_resources/tasks_test.py +++ b/learning_resources/tasks_test.py @@ -646,6 +646,14 @@ def test_marketing_page_for_program_appends_children(mocker, settings): relation_type="PROGRAM_COURSES", position=0, ) + models.ContentFile.objects.create( + learning_resource=child_course, + file_type=MARKETING_PAGE_FILE_TYPE, + file_extension=".md", + key="mktg-child-course", + content="Child Course marketing copy", + published=True, + ) html_content = "

Test Program

Program info

" mocker.patch( diff --git a/learning_resources/utils.py b/learning_resources/utils.py index bf42b48a2d..5462ee4113 100644 --- a/learning_resources/utils.py +++ b/learning_resources/utils.py @@ -27,6 +27,7 @@ LearningResourceType, semester_mapping, ) +from learning_resources.etl.constants import MARKETING_PAGE_FILE_TYPE from learning_resources.hooks import get_plugin_manager from learning_resources.models import ( ContentFile, @@ -740,53 +741,26 @@ def build_resource_summary_dict(resource): } -def _build_entry(resource, summaries_by_resource): - """Build a resource entry dict for markdown rendering.""" - entry = build_resource_summary_dict(resource) - entry["summaries"] = summaries_by_resource.get(resource.id, []) - return entry +# Bound the program marketing-page content assembled from child courses so it +# stays a reasonable embedding input. A program page previously concatenated +# every child-course content-file summary and ballooned to 23MB in prod, +# overflowing the embedding API's per-request token limit. +PROGRAM_CHILDREN_CONTENT_MAX_CHARS = 1_000_000 -def _build_entries_from_relationships(relationships, summaries_by_resource): - """Build entry dicts from prefetched relationships, including grandchildren.""" - entries = [] +def _course_ids_by_program(relationships, course_type): + """Map program id -> reachable published course ids (direct + via subprograms).""" + result = defaultdict(set) for rel in relationships: - entry = _build_entry(rel.child, summaries_by_resource) - - if ( - rel.child.resource_type == LearningResourceType.program.name - and rel.relation_type == LearningResourceRelationTypes.PROGRAM_PROGRAMS - ): - sub_entries = [ - _build_entry(sub_rel.child, summaries_by_resource) + if rel.child.resource_type == course_type: + result[rel.parent_id].add(rel.child_id) + else: + result[rel.parent_id].update( + sub_rel.child_id for sub_rel in getattr(rel.child, "program_children", []) - ] - if sub_entries: - entry["children"] = sub_entries - - entries.append(entry) - return entries - - -def _format_resource_entries(entries, heading_level=3): - """Recursively format resource entries as markdown sections.""" - sections = [] - for entry in entries: - prefix = "#" * heading_level - lines = [f"{prefix} {entry['title']}"] - if entry["description"]: - lines.append(entry["description"]) - if entry["topics"]: - lines.append(f"Topics: {', '.join(entry['topics'])}") - if entry.get("summaries"): - lines.append("\n**Content summaries:**") - lines.extend(f"- {summary}" for summary in entry["summaries"]) - sections.append("\n".join(lines)) - if entry.get("children"): - sections.extend( - _format_resource_entries(entry["children"], heading_level + 1) + if sub_rel.child.resource_type == course_type ) - return sections + return result def build_program_children_content(learning_resource): @@ -801,8 +775,10 @@ def build_program_children_content(learning_resource): def build_program_children_content_bulk(program_resources): """Build program children markdown for many program resources in bulk. - Returns a dict keyed by learning_resource id with markdown content. - Non-program resources are ignored. + For each program, concatenates the marketing-page content of its published + (or test-mode) child courses, including courses reached through child + programs. Returns a dict keyed by learning_resource id. Non-program + resources are ignored. """ programs = [ resource @@ -825,59 +801,52 @@ def build_program_children_content_bulk(program_resources): relation_type__in=program_relation_types, ) .filter(child_visibility) - .select_related("parent", "child") + .select_related("child") .prefetch_related( - "child__topics", Prefetch( "child__children", queryset=LearningResourceRelationship.objects.filter( relation_type__in=program_relation_types, ) .filter(child_visibility) - .select_related("child") - .prefetch_related("child__topics"), + .select_related("child"), to_attr="program_children", ), ) ) - child_ids = [rel.child_id for rel in relationships] - grandchild_ids = [ - gc.child_id - for rel in relationships - for gc in getattr(rel.child, "program_children", []) - ] - all_ids = set(child_ids + grandchild_ids) - set(program_ids) + course_type = LearningResourceType.course.name + course_ids_by_program = _course_ids_by_program(relationships, course_type) + all_course_ids = set().union(*course_ids_by_program.values()) - summaries_by_resource = {} - if all_ids: - summary_qs = ( + # One marketing-page content file per course (published), fetched in bulk + marketing_by_course = {} + if all_course_ids: + marketing_qs = ( ContentFile.objects.filter( - run__learning_resource_id__in=all_ids, + learning_resource_id__in=all_course_ids, + file_type=MARKETING_PAGE_FILE_TYPE, published=True, - run__published=True, ) - .exclude(summary="") - .values_list("run__learning_resource_id", "summary") + .exclude(content="") + .order_by("learning_resource_id", "id") + .values_list("learning_resource_id", "learning_resource__title", "content") ) - for resource_id, summary in summary_qs: - summaries_by_resource.setdefault(resource_id, []).append(summary) - - relationships_by_program = defaultdict(list) - for rel in relationships: - relationships_by_program[rel.parent_id].append(rel) + for lr_id, title, content in marketing_qs: + marketing_by_course.setdefault(lr_id, (title, content)) content_by_program_id = {} for program in programs: - rels = relationships_by_program.get(program.id, []) - entries = _build_entries_from_relationships(rels, summaries_by_resource) - - if not entries: + sections = [] + for course_id in sorted(course_ids_by_program.get(program.id, set())): + page = marketing_by_course.get(course_id) + if page: + title, content = page + sections.append(f"### {title}\n\n{content}") + if not sections: content_by_program_id[program.id] = "" continue - - sections = ["\n\n## Program Contents\n"] - sections.extend(_format_resource_entries(entries)) - content_by_program_id[program.id] = "\n\n".join(sections) + body = "\n\n".join(["\n\n## Program Contents\n", *sections]) + content_by_program_id[program.id] = body[:PROGRAM_CHILDREN_CONTENT_MAX_CHARS] return content_by_program_id diff --git a/learning_resources/utils_test.py b/learning_resources/utils_test.py index 596134c98d..53e74af418 100644 --- a/learning_resources/utils_test.py +++ b/learning_resources/utils_test.py @@ -19,6 +19,7 @@ CONTENT_TYPE_VIDEO, LearningResourceRelationTypes, ) +from learning_resources.etl.constants import MARKETING_PAGE_FILE_TYPE from learning_resources.etl.utils import get_content_type from learning_resources.factories import ( CourseFactory, @@ -118,6 +119,20 @@ def fixture_test_instructors_data(): return json.load(test_data)["instructors"] +def _add_course_marketing_page(course_lr, content): + """Attach a published marketing-page content file to a course.""" + from learning_resources.models import ContentFile + + return ContentFile.objects.create( + learning_resource=course_lr, + file_type=MARKETING_PAGE_FILE_TYPE, + file_extension=".md", + key=f"mktg-{course_lr.id}", + content=content, + published=True, + ) + + @pytest.fixture def program_with_visibility_children(): """Program with published, unpublished, and test_mode child courses.""" @@ -135,6 +150,8 @@ def program_with_visibility_children(): learning_resource__published=False, learning_resource__test_mode=True, ).learning_resource + for child in (published, unpublished, test_mode): + _add_course_marketing_page(child, f"Marketing copy for {child.title}.") program_lr = ProgramFactory.create(courses=[published]).learning_resource # Manually add unpublished/test_mode children since ProgramFactory # only creates PROGRAM_COURSES relationships for visible courses @@ -687,27 +704,40 @@ def test_build_program_children_content_non_program(): assert build_program_children_content(course_lr) == "" -def test_build_program_children_content_direct_courses(): - """Programs with direct course children should include them""" +def test_build_program_children_content_includes_child_course_marketing_pages(): + """A published child course's marketing-page content is included.""" course_lr = CourseFactory.create( learning_resource__title="Test Course", - learning_resource__description="A test course", ).learning_resource + _add_course_marketing_page(course_lr, "Marketing copy for the test course.") program_lr = ProgramFactory.create(courses=[course_lr]).learning_resource result = build_program_children_content(program_lr) assert "## Program Contents" in result - assert "Test Course" in result - assert "A test course" in result + assert "### Test Course" in result + assert "Marketing copy for the test course." in result + +def test_build_program_children_content_omits_courses_without_marketing_page(): + """Child courses lacking a marketing-page content file contribute nothing.""" + course_lr = CourseFactory.create( + learning_resource__title="No Marketing Course", + ).learning_resource + program_lr = ProgramFactory.create(courses=[course_lr]).learning_resource -def test_build_program_children_content_child_programs_with_courses(): - """Programs with child programs should recurse to find courses""" + assert build_program_children_content(program_lr) == "" + + +def test_build_program_children_content_recurses_child_program_courses(): + """Courses reached through a child program are included; the child program + itself is not emitted as a heading. + """ from learning_resources.models import LearningResourceRelationship grandchild_course_lr = CourseFactory.create( learning_resource__title="Grandchild Course" ).learning_resource + _add_course_marketing_page(grandchild_course_lr, "Grandchild marketing copy.") child_program_lr = ProgramFactory.create( courses=[grandchild_course_lr], learning_resource__title="Child Program" ).learning_resource @@ -719,31 +749,58 @@ def test_build_program_children_content_child_programs_with_courses(): ) result = build_program_children_content(parent_lr) - assert "Child Program" in result - assert "Grandchild Course" in result + assert "### Grandchild Course" in result + assert "Grandchild marketing copy." in result + # Child programs are traversed only to reach courses, not emitted themselves. + assert "Child Program" not in result -def test_build_program_children_content_with_summaries(): - """Child course contentfile summaries should be included""" - from learning_resources.models import ContentFile, LearningResourceRun +def test_build_program_children_content_excludes_unpublished_marketing_pages(): + """Unpublished marketing-page content files are excluded.""" + from learning_resources.models import ContentFile course_lr = CourseFactory.create( - learning_resource__title="Course With Summary" + learning_resource__title="Course With Unpublished Page" ).learning_resource - run = LearningResourceRun.objects.create( - learning_resource=course_lr, - run_id="test-run", - ) ContentFile.objects.create( - run=run, - key="transcript.txt", - summary="This is a summary of the course content.", + learning_resource=course_lr, + file_type=MARKETING_PAGE_FILE_TYPE, + file_extension=".md", + key=f"mktg-{course_lr.id}", + content="Hidden marketing copy.", + published=False, ) program_lr = ProgramFactory.create(courses=[course_lr]).learning_resource result = build_program_children_content(program_lr) - assert "This is a summary of the course content." in result - assert "Content summaries" in result + assert "Hidden marketing copy." not in result + assert result == "" + + +def test_build_program_children_content_caps_total_length(): + """Program children content is hard-capped in total size""" + course_lr = CourseFactory.create().learning_resource + _add_course_marketing_page(course_lr, "x" * 1_500_000) + program_lr = ProgramFactory.create(courses=[course_lr]).learning_resource + + result = build_program_children_content(program_lr) + assert len(result) <= 1_000_000 + + +def test_build_program_children_content_deterministic_order(): + """Child course sections appear in a stable order.""" + courses = [ + CourseFactory.create(learning_resource__title=f"Course {i}").learning_resource + for i in range(3) + ] + for i, course in enumerate(courses): + _add_course_marketing_page(course, f"body {i}") + program_lr = ProgramFactory.create(courses=courses).learning_resource + + result = build_program_children_content(program_lr) + ordered_by_id = sorted(courses, key=lambda c: c.id) + positions = [result.index(f"### {c.title}") for c in ordered_by_id] + assert positions == sorted(positions) def test_build_program_children_content_ignores_non_program_relations(): @@ -753,12 +810,14 @@ def test_build_program_children_content_ignores_non_program_relations(): course_lr = CourseFactory.create( learning_resource__title="Real Course" ).learning_resource + _add_course_marketing_page(course_lr, "Real course copy.") program_lr = ProgramFactory.create(courses=[course_lr]).learning_resource # Add a non-program relation (e.g. LEARNING_PATH_ITEMS) unrelated_lr = CourseFactory.create( learning_resource__title="Unrelated Item" ).learning_resource + _add_course_marketing_page(unrelated_lr, "Unrelated copy.") LearningResourceRelationship.objects.create( parent=program_lr, child=unrelated_lr, @@ -766,30 +825,8 @@ def test_build_program_children_content_ignores_non_program_relations(): ) result = build_program_children_content(program_lr) - assert "Real Course" in result - assert "Unrelated Item" not in result - - -def test_build_program_children_content_two_levels(): - """Program -> child program -> courses are all included (2 levels)""" - from learning_resources.models import LearningResourceRelationship - - nested_course = CourseFactory.create( - learning_resource__title="Nested Course" - ).learning_resource - mid_lr = ProgramFactory.create( - courses=[nested_course], learning_resource__title="Mid Program" - ).learning_resource - top_lr = ProgramFactory.create(courses=[]).learning_resource - LearningResourceRelationship.objects.create( - parent=top_lr, - child=mid_lr, - relation_type="PROGRAM_PROGRAMS", - ) - - result = build_program_children_content(top_lr) - assert "Mid Program" in result - assert "Nested Course" in result + assert "Real course copy." in result + assert "Unrelated copy." not in result # --- build_program_children_content_bulk tests --- @@ -818,8 +855,8 @@ def test_build_program_children_content_bulk_single_program(): """Bulk function produces same output as single-resource version.""" course_lr = CourseFactory.create( learning_resource__title="Bulk Test Course", - learning_resource__description="A description", ).learning_resource + _add_course_marketing_page(course_lr, "Bulk marketing copy.") program_lr = ProgramFactory.create(courses=[course_lr]).learning_resource single_result = build_program_children_content(program_lr) @@ -835,6 +872,8 @@ def test_build_program_children_content_bulk_multiple_programs(): course2 = CourseFactory.create( learning_resource__title="Course For Prog2" ).learning_resource + _add_course_marketing_page(course1, "Prog1 course copy.") + _add_course_marketing_page(course2, "Prog2 course copy.") prog1, prog2 = ( ProgramFactory.create(courses=[course]).learning_resource for course in [course1, course2] @@ -843,9 +882,9 @@ def test_build_program_children_content_bulk_multiple_programs(): result = build_program_children_content_bulk([prog1, prog2]) assert prog1.id in result assert prog2.id in result - assert "Course For Prog1" in result[prog1.id] - assert "Course For Prog2" in result[prog2.id] - assert "Course For Prog2" not in result[prog1.id] + assert "Prog1 course copy." in result[prog1.id] + assert "Prog2 course copy." in result[prog2.id] + assert "Prog2 course copy." not in result[prog1.id] def test_build_program_children_content_bulk_mixed_resources(): @@ -853,6 +892,7 @@ def test_build_program_children_content_bulk_mixed_resources(): course_child = CourseFactory.create( learning_resource__title="Child Course" ).learning_resource + _add_course_marketing_page(course_child, "Child course copy.") program_lr = ProgramFactory.create(courses=[course_child]).learning_resource non_program = CourseFactory.create().learning_resource @@ -868,6 +908,7 @@ def test_build_program_children_content_bulk_with_grandchildren(): grandchild = CourseFactory.create( learning_resource__title="Grandchild Course" ).learning_resource + _add_course_marketing_page(grandchild, "Grandchild copy.") child_prog = ProgramFactory.create( courses=[grandchild], learning_resource__title="Sub Program" ).learning_resource @@ -877,8 +918,8 @@ def test_build_program_children_content_bulk_with_grandchildren(): ) result = build_program_children_content_bulk([parent_lr]) - assert "Sub Program" in result[parent_lr.id] - assert "Grandchild Course" in result[parent_lr.id] + assert "### Grandchild Course" in result[parent_lr.id] + assert "Grandchild copy." in result[parent_lr.id] def test_build_program_children_content_excludes_unpublished_children( @@ -902,6 +943,8 @@ def test_build_program_children_content_excludes_unpublished_grandchildren(): is_unpublished=True, learning_resource__title="Unpublished Grandchild", ).learning_resource + _add_course_marketing_page(published_grandchild, "Published grandchild copy.") + _add_course_marketing_page(unpublished_grandchild, "Unpublished grandchild copy.") child_prog = ProgramFactory.create( courses=[published_grandchild], learning_resource__title="Sub Program" ).learning_resource @@ -921,53 +964,34 @@ def test_build_program_children_content_excludes_unpublished_grandchildren(): assert "Unpublished Grandchild" not in result[parent_lr.id] -def test_build_program_children_content_bulk_excludes_unpublished_contentfiles(): - """Unpublished content files and files on unpublished runs are excluded.""" - from learning_resources.models import ContentFile, LearningResourceRun +def test_build_program_children_content_bulk_excludes_unpublished_marketing_pages(): + """Only published marketing-page content files are included.""" + from learning_resources.models import ContentFile - course_lr = CourseFactory.create( - learning_resource__title="Course With Mixed Content" + published_course = CourseFactory.create( + learning_resource__title="Published Marketing Course" ).learning_resource + _add_course_marketing_page(published_course, "Visible marketing copy.") - published_run = LearningResourceRun.objects.create( - learning_resource=course_lr, - run_id="published-run", - published=True, - ) - unpublished_run = LearningResourceRun.objects.create( - learning_resource=course_lr, - run_id="unpublished-run", - published=False, - ) - - # Published content file on published run — should be included - ContentFile.objects.create( - run=published_run, - key="visible.txt", - summary="Visible summary", - published=True, - ) - # Unpublished content file on published run — should be excluded + unpublished_course = CourseFactory.create( + learning_resource__title="Unpublished Marketing Course" + ).learning_resource ContentFile.objects.create( - run=published_run, - key="hidden.txt", - summary="Hidden unpublished summary", + learning_resource=unpublished_course, + file_type=MARKETING_PAGE_FILE_TYPE, + file_extension=".md", + key=f"mktg-{unpublished_course.id}", + content="Hidden marketing copy.", published=False, ) - # Published content file on unpublished run — should be excluded - ContentFile.objects.create( - run=unpublished_run, - key="hidden-run.txt", - summary="Hidden run summary", - published=True, - ) - program_lr = ProgramFactory.create(courses=[course_lr]).learning_resource + program_lr = ProgramFactory.create( + courses=[published_course, unpublished_course] + ).learning_resource result = build_program_children_content_bulk([program_lr]) - assert "Visible summary" in result[program_lr.id] - assert "Hidden unpublished summary" not in result[program_lr.id] - assert "Hidden run summary" not in result[program_lr.id] + assert "Visible marketing copy." in result[program_lr.id] + assert "Hidden marketing copy." not in result[program_lr.id] @pytest.mark.parametrize( diff --git a/vector_search/utils.py b/vector_search/utils.py index 4a9638a678..dfb929804c 100644 --- a/vector_search/utils.py +++ b/vector_search/utils.py @@ -704,9 +704,16 @@ def _generate_content_file_points(serialized_content): 300,000 tokens per request max array size: 2048 see: https://platform.openai.com/docs/guides/rate-limits + + The 0.9 factor leaves headroom: markdown header prefixes are prepended + after the chunk-size split, so real chunks can exceed the nominal size. """ - request_chunk_size = int( - 300000 / settings.CONTENT_FILE_EMBEDDING_CHUNK_SIZE_OVERRIDE + request_chunk_size = max( + 1, + min( + 2048, + int(300000 * 0.9 / settings.CONTENT_FILE_EMBEDDING_CHUNK_SIZE_OVERRIDE), + ), ) for doc in serialized_content: diff --git a/vector_search/utils_test.py b/vector_search/utils_test.py index ac5bfbfeae..bd8da07e20 100644 --- a/vector_search/utils_test.py +++ b/vector_search/utils_test.py @@ -754,6 +754,101 @@ def test_generate_content_points_uses_standard_chunking_for_non_markdown(mocker) mock_md_chunk.assert_not_called() +def test_generate_content_points_leaves_headroom_under_token_limit(mocker): + """ + Embedding request batches must leave headroom under OpenAI's 300k + tokens-per-request limit, since markdown header prefixes are prepended + after the chunk-size split and inflate chunks past the nominal size + """ + settings.CONTENT_FILE_EMBEDDING_CHUNK_SIZE_OVERRIDE = 500 + settings.CONTENT_FILE_EMBEDDING_CHUNK_OVERLAP = 50 + + # 600 chunks * 500 tokens == exactly 300k if packed with no headroom + num_chunks = 600 + mocker.patch( + "vector_search.utils._chunk_documents", + return_value=[ + Document(page_content=f"chunk{i}", metadata={"key": "k1"}) + for i in range(num_chunks) + ], + ) + mocker.patch( + "vector_search.utils.should_generate_content_embeddings", return_value=True + ) + mocker.patch("vector_search.utils.remove_points_matching_params") + + mock_dense = mocker.MagicMock() + mock_dense.embed_documents.side_effect = lambda texts: [[0.1] for _ in texts] + mock_dense.model_short_name.return_value = "dense" + mock_sparse = mocker.MagicMock() + mock_sparse.embed_documents.side_effect = lambda texts: [[0.2] for _ in texts] + mock_sparse.model_short_name.return_value = "sparse" + mocker.patch("vector_search.utils.dense_encoder", return_value=mock_dense) + mocker.patch("vector_search.utils.sparse_encoder", return_value=mock_sparse) + + doc = { + "content": "Some plain text content", + "file_type": "page", + "file_extension": ".html", + "platform": {"code": "x"}, + "resource_readable_id": "r1", + "run_readable_id": "run1", + "key": "k1", + } + + points = list(_generate_content_file_points([doc])) + + batch_sizes = [ + len(call.args[0]) for call in mock_dense.embed_documents.call_args_list + ] + assert sum(batch_sizes) == num_chunks + assert len(points) == num_chunks + # nominal tokens per request must stay at least ~5% under the 300k limit + assert max(batch_sizes) * 500 <= 285000 + + +def test_generate_content_points_request_chunk_size_never_zero(mocker): + """ + A misconfigured (huge) chunk-size override must not make request_chunk_size 0, + which would raise ValueError in the range() batching loop + """ + settings.CONTENT_FILE_EMBEDDING_CHUNK_SIZE_OVERRIDE = 500000 + settings.CONTENT_FILE_EMBEDDING_CHUNK_OVERLAP = 50 + + mocker.patch( + "vector_search.utils._chunk_documents", + return_value=[ + Document(page_content=f"chunk{i}", metadata={"key": "k1"}) for i in range(3) + ], + ) + mocker.patch( + "vector_search.utils.should_generate_content_embeddings", return_value=True + ) + mocker.patch("vector_search.utils.remove_points_matching_params") + + mock_dense = mocker.MagicMock() + mock_dense.embed_documents.side_effect = lambda texts: [[0.1] for _ in texts] + mock_dense.model_short_name.return_value = "dense" + mock_sparse = mocker.MagicMock() + mock_sparse.embed_documents.side_effect = lambda texts: [[0.2] for _ in texts] + mock_sparse.model_short_name.return_value = "sparse" + mocker.patch("vector_search.utils.dense_encoder", return_value=mock_dense) + mocker.patch("vector_search.utils.sparse_encoder", return_value=mock_sparse) + + doc = { + "content": "Some plain text content", + "file_type": "page", + "file_extension": ".html", + "platform": {"code": "x"}, + "resource_readable_id": "r1", + "run_readable_id": "run1", + "key": "k1", + } + + points = list(_generate_content_file_points([doc])) + assert len(points) == 3 + + def test_course_metadata_indexed_with_learning_resources(mocker): # test the we embed a metadata document when embedding learning resources resources = LearningResourceFactory.create_batch(5) From 0abb229e300a2b21cf8825887ce27bb469093f85 Mon Sep 17 00:00:00 2001 From: Ahtesham Quraish Date: Tue, 7 Jul 2026 17:08:10 +0500 Subject: [PATCH 5/8] fix: changing the positions of share feature on videos and podcasts episode (#3540) * fix: changing the positions of share feature on videos and podcasts episode --------- Co-authored-by: Ahtesham Quraish --- .../PodcastPage/PodcastEpisodeDetailPage.tsx | 57 +- .../PodcastPage/PodcastShareButton.tsx | 45 ++ .../ShareDialog.tsx | 545 +----------------- .../UpNextSection.tsx | 20 - .../VideoDetailPage.tsx | 60 +- .../VideoSeriesDetailPage.styled.ts | 38 +- .../VideoSeriesDetailPage.tsx | 32 +- .../VideoShareButton.tsx | 2 +- .../components/ShareButton/ShareButton.tsx | 25 + .../components/ShareDialog/ShareDialog.tsx | 543 +++++++++++++++++ .../ResourceCard/ResourceCard.test.tsx | 49 -- .../ResourceCard/ResourceCard.tsx | 43 +- .../LearningResourceCard.tsx | 23 +- .../LearningResourceListCard.tsx | 20 +- 14 files changed, 735 insertions(+), 767 deletions(-) create mode 100644 frontends/main/src/app-pages/PodcastPage/PodcastShareButton.tsx create mode 100644 frontends/main/src/components/ShareButton/ShareButton.tsx create mode 100644 frontends/main/src/components/ShareDialog/ShareDialog.tsx diff --git a/frontends/main/src/app-pages/PodcastPage/PodcastEpisodeDetailPage.tsx b/frontends/main/src/app-pages/PodcastPage/PodcastEpisodeDetailPage.tsx index a22f73e54d..8761bfff91 100644 --- a/frontends/main/src/app-pages/PodcastPage/PodcastEpisodeDetailPage.tsx +++ b/frontends/main/src/app-pages/PodcastPage/PodcastEpisodeDetailPage.tsx @@ -19,7 +19,7 @@ import { } from "api/hooks/learningResources" import { ResourceTypeEnum } from "api/v1" -import type { LearningResource } from "api/v1" +import type { LearningResource, PodcastEpisodeResource } from "api/v1" import moment from "moment" import { formatDate } from "ol-utilities" import { HOME, podcastPageView, podcastEpisodePageView } from "@/common/urls" @@ -27,7 +27,10 @@ import DOMPurify from "isomorphic-dompurify" import { EpisodeItem } from "./PodcastDetailPage" import PodcastContainer from "./PodcastContainer" import Link from "next/link" +import PodcastShareButton from "./PodcastShareButton" +import { env } from "@/env" +const NEXT_PUBLIC_ORIGIN = env("NEXT_PUBLIC_ORIGIN") /* ── Layout ── */ const EpisodeContainer = styled(Container)(({ theme }) => ({ @@ -142,6 +145,10 @@ const EpisodeList = styled.ul({ display: "grid", gridTemplateColumns: "1fr", }) +const StyledPodcastShareButton = styled(PodcastShareButton)({ + padding: "18px 12px", + margin: "0 0 24px", +}) export const BreadcrumbBar = styled.div(({ theme }) => ({ padding: "18px 0 2px 0", @@ -181,6 +188,14 @@ const StyledButton = styled(Button)(({ theme }) => ({ }, })) +export const PodcastShareSection = styled("div")({ + display: "flex", + alignItems: "center", + justifyContent: "space-between", + flexWrap: "wrap", + gap: "8px", +}) + /* ── Component ── */ type PodcastEpisodeDetailPageProps = { @@ -285,6 +300,11 @@ export const PodcastEpisodeDetailPage: React.FC< ? podcastPageView(podcastId, podcast?.title) : "/" + const sharePageUrl = + episode && podcastId + ? `${NEXT_PUBLIC_ORIGIN}${podcastEpisodePageView(String(episode!.id), podcastId, episode?.title)}` + : "" + return ( <> @@ -315,19 +335,28 @@ export const PodcastEpisodeDetailPage: React.FC< )} {isMobile && {topicString}} - {episode && ( - : - } - disabled={!episode || !getAudioUrl(episode)} - > - {isCurrentEpisodePlaying ? "Pause Episode" : "Play Episode"} - - )} - + + {episode && podcastId && ( + : + } + disabled={!episode || !getAudioUrl(episode)} + > + {isCurrentEpisodePlaying ? "Pause Episode" : "Play Episode"} + + )} + + {episode && podcastId && ( + + )} + {episode?.description && ( = ({ + resource, + title, + sharePageUrl, + className, +}) => { + const [shareOpen, setShareOpen] = useState(false) + + return ( + <> + setShareOpen(true)} + > + + Share + + setShareOpen(false)} + resource={resource as PodcastEpisodeResource} + pageUrl={sharePageUrl} + title={title ?? ""} + /> + + ) +} + +export default PodcastShareButton diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.tsx index 60587e75d8..25acfe8b76 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.tsx @@ -1,543 +1,2 @@ -"use client" - -import React, { useEffect, useRef, useState } from "react" -import { Dialog, styled, theme } from "ol-components" -import Link from "next/link" -import { env } from "@/env" -import { - FACEBOOK_SHARE_BASE_URL, - TWITTER_SHARE_BASE_URL, - LINKEDIN_SHARE_BASE_URL, -} from "@/common/urls" -import { - RiFacebookFill, - RiTwitterXLine, - RiLinkedinFill, - RiLink, - RiShareForwardFill, - RiCodeSSlashLine, -} from "@remixicon/react" -import { Button, Input } from "@mitodl/smoot-design" -import type { VideoResource, PodcastEpisodeResource } from "api/v1" - -const NEXT_PUBLIC_ORIGIN = env("NEXT_PUBLIC_ORIGIN") - -type Tab = "share" | "embed" - -// ─── Layout ──────────────────────────────────────────────────────────────── - -const DialogInner = styled.div({ - display: "flex", - flexDirection: "column", - gap: "24px", -}) - -const TabGroup = styled.div({ - display: "flex", - gap: "10px", -}) - -const TabButton = styled.button<{ active: boolean }>(({ active }) => ({ - display: "inline-flex", - alignItems: "center", - gap: "10px", - padding: "12px 24px 12px 16px", - borderRadius: "24px", - border: "none", - cursor: "pointer", - ...theme.typography.body2, - fontWeight: theme.typography.fontWeightMedium, - lineHeight: "14px", - backgroundColor: active - ? theme.custom.colors.red - : theme.custom.colors.lightGray2, - color: active ? theme.custom.colors.white : theme.custom.colors.darkGray1, - transition: "background-color 0.15s, color 0.15s", - "&:hover": { - backgroundColor: active - ? theme.custom.colors.darkRed - : theme.custom.colors.lightGray2, - }, -})) - -// ─── Share tab ───────────────────────────────────────────────────────────── - -const ShareContents = styled.div(({ theme }) => ({ - display: "flex", - gap: "40px", - [theme.breakpoints.down("sm")]: { - flexDirection: "column", - gap: "32px", - }, -})) - -const SocialContainer = styled.div({ - display: "flex", - flexDirection: "column", - gap: "16px", -}) - -const LinkContainer = styled.div({ - display: "flex", - flexDirection: "column", - gap: "12px", - flex: 1, -}) - -const SectionHeading = styled.span(({ theme }) => ({ - ...theme.typography.body2, - color: theme.custom.colors.darkGray2, - fontWeight: theme.typography.fontWeightBold, -})) - -const SocialButtonRow = styled.div({ - display: "flex", - gap: "16px", - a: { height: "18px" }, -}) - -const ShareLink = styled(Link)({ - color: theme.custom.colors.silverGrayDark, - "&:hover": { color: theme.custom.colors.lightRed }, -}) - -const LinkControls = styled.div(({ theme }) => ({ - display: "flex", - gap: "16px", - input: { - ...theme.typography.body3, - color: theme.custom.colors.darkGray2, - padding: "0 3px", - }, -})) - -const RedLinkIcon = styled(RiLink)({ - color: theme.custom.colors.red, -}) - -const CopyButton = styled(Button)({ - minWidth: "104px", -}) - -// ─── Embed tab ───────────────────────────────────────────────────────────── - -const EmbedContents = styled.div({ - display: "flex", - flexDirection: "column", - gap: "16px", -}) - -const Field = styled.div({ - display: "flex", - flexDirection: "column", - gap: "8px", -}) - -const EmbedTextarea = styled.textarea(({ theme }) => ({ - width: "100%", - minHeight: "100px", - resize: "vertical", - borderRadius: "4px", - border: `1px solid ${theme.custom.colors.lightGray2}`, - padding: "8px", - ...theme.typography.body3, - color: theme.custom.colors.darkGray2, - fontFamily: "monospace", - boxSizing: "border-box", -})) - -const EmbedFooter = styled.div({ - display: "flex", - justifyContent: "end", - alignItems: "center", -}) - -const StartAtRow = styled.div(({ theme }) => ({ - display: "flex", - alignItems: "center", - gap: "8px", - ...theme.typography.body2, - color: theme.custom.colors.darkGray2, - "input[type='checkbox']": { - width: "16px", - height: "16px", - cursor: "pointer", - accentColor: theme.custom.colors.red, - }, - label: { - cursor: "pointer", - userSelect: "none", - }, -})) - -// ─── Helpers ─────────────────────────────────────────────────────────────── - -async function copyToClipboard(text: string): Promise { - if (!navigator?.clipboard) throw new Error("Clipboard API unavailable") - await navigator.clipboard.writeText(text) -} - -function getYouTubeVideoId(url: string | null | undefined): string | null { - if (!url) return null - const patterns = [ - /[?&]v=([^&]+)/, - /youtu\.be\/([^?&]+)/, - /youtube\.com\/embed\/([^?&]+)/, - ] - for (const pattern of patterns) { - const match = url.match(pattern) - if (match) return match[1] - } - return null -} - -function escapeHtmlAttr(value: string): string { - return value - .replace(/&/g, "&") - .replace(/"/g, """) - .replace(/ 0 ? `${h}:${mm}:${ss}` : `${m}:${ss}` -} - -function withTimeParam(url: string, seconds: number, param = "t"): string { - try { - const u = new URL(url) - u.searchParams.set(param, String(Math.floor(seconds))) - return u.toString() - } catch { - return url - } -} - -function buildVideoEmbedHtml( - video: VideoResource, - title: string, - embedPageUrl: string, - startTime = 0, -): string { - const isOvs = video.platform?.code === "ovs" || video.platform?.code === "ocw" - const escapedTitle = escapeHtmlAttr(title) - const t = Math.floor(startTime) - if (isOvs) { - if (!embedPageUrl) return "" - const src = t > 0 ? withTimeParam(embedPageUrl, t) : embedPageUrl - return `` - } - - const youtubeId = getYouTubeVideoId(video.url) - if (!youtubeId) return "" - const ytBase = `https://www.youtube.com/embed/${youtubeId}` - const src = t > 0 ? withTimeParam(ytBase, t, "start") : ytBase - return `` -} - -function buildPodcastEmbedHtml( - resource: PodcastEpisodeResource, - title: string, - _embedPageUrl: string, -): string { - const episode = resource.podcast_episode - if (!episode?.audio_url) return "" - const escapedTitle = escapeHtmlAttr(title) - const escapedAudioUrl = escapeHtmlAttr(episode.audio_url) - return `` -} - -// ─── Component ───────────────────────────────────────────────────────────── - -type ShareDialogProps = { - open: boolean - onClose: () => void - video?: VideoResource - resource?: PodcastEpisodeResource - pageUrl: string - title: string - getCurrentTime?: () => number -} - -const ShareDialog = ({ - open, - onClose, - video, - resource, - pageUrl, - title, - - getCurrentTime, -}: ShareDialogProps) => { - const [activeTab, setActiveTab] = useState("share") - const [copyLinkText, setCopyLinkText] = useState("Copy Link") - const [copyEmbedText, setCopyEmbedText] = useState("Copy") - const [startTime, setStartTime] = useState(0) - const [includeTime, setIncludeTime] = useState(false) - - const wasOpenRef = useRef(false) - useEffect(() => { - if (open && !wasOpenRef.current) { - const t = getCurrentTime?.() ?? 0 - setStartTime(t) - setIncludeTime(t > 0) - } - wasOpenRef.current = open - }, [open, getCurrentTime]) - - const copyLinkTimerRef = useRef | null>(null) - const copyEmbedTimerRef = useRef | null>(null) - - useEffect( - () => () => { - if (copyLinkTimerRef.current) clearTimeout(copyLinkTimerRef.current) - if (copyEmbedTimerRef.current) clearTimeout(copyEmbedTimerRef.current) - }, - [], - ) - - const shareTabRef = useRef(null) - const embedTabRef = useRef(null) - const tabRefs: Record> = { - share: shareTabRef, - embed: embedTabRef, - } - const TABS: Tab[] = ["share", "embed"] - - const handleTabKeyDown = - (tab: Tab) => (e: React.KeyboardEvent) => { - const idx = TABS.indexOf(tab) - let next: Tab | null = null - if (e.key === "ArrowRight") next = TABS[(idx + 1) % TABS.length] - else if (e.key === "ArrowLeft") - next = TABS[(idx - 1 + TABS.length) % TABS.length] - if (next) { - e.preventDefault() - setActiveTab(next) - tabRefs[next].current?.focus() - } - } - - const hasEmbed = video !== undefined || resource !== undefined - - const videoEmbedPageUrl = video - ? `${NEXT_PUBLIC_ORIGIN}/video/embed/${video.id}` - : "" - const embedUrl = video - ? videoEmbedPageUrl - : resource - ? `${NEXT_PUBLIC_ORIGIN}/podcast/embed/${resource.id}` - : null - - const t = includeTime && startTime > 0 ? Math.floor(startTime) : 0 - - const activeShareUrl = pageUrl - const activeEmbedUrl = - embedUrl && t > 0 ? withTimeParam(embedUrl, t) : embedUrl - - const embedHtml = video - ? buildVideoEmbedHtml(video, title, videoEmbedPageUrl, t) - : resource - ? buildPodcastEmbedHtml(resource, title, embedUrl || "") - : "" - - const embedUrlLabel = video ? "Video URL" : "Audio URL" - - const startAtLabel = - video && startTime > 0 ? `Start at ${formatTimestamp(startTime)}` : null - - return ( - - - {hasEmbed && ( - - setActiveTab("share")} - onKeyDown={handleTabKeyDown("share")} - > - - Share - - setActiveTab("embed")} - onKeyDown={handleTabKeyDown("embed")} - > - - Embed - - - )} - -
- {activeTab === "share" || !hasEmbed ? ( - - - Share on Social - - - - - - - - - - - - - - Share a link - - { - const input = event.currentTarget.querySelector("input") - if (!input) return - input.select() - }} - /> - } - onClick={async () => { - if (!activeShareUrl) return - try { - await copyToClipboard(activeShareUrl) - setCopyLinkText("Copied!") - } catch { - setCopyLinkText("Failed to copy") - } - if (copyLinkTimerRef.current) - clearTimeout(copyLinkTimerRef.current) - copyLinkTimerRef.current = setTimeout( - () => setCopyLinkText("Copy Link"), - 2000, - ) - }} - > - {copyLinkText} - - - - - ) : ( - - - {embedUrlLabel} - { - const input = event.currentTarget.querySelector("input") - if (!input) return - input.select() - }} - /> - - - Embed HTML - (e.target as HTMLTextAreaElement).select()} - /> - - - {startAtLabel && ( - - setIncludeTime(e.target.checked)} - /> - - - )} - } - onClick={async () => { - if (!embedHtml) return - try { - await copyToClipboard(embedHtml) - setCopyEmbedText("Copied!") - } catch { - setCopyEmbedText("Failed to copy") - } - if (copyEmbedTimerRef.current) - clearTimeout(copyEmbedTimerRef.current) - copyEmbedTimerRef.current = setTimeout( - () => setCopyEmbedText("Copy"), - 2000, - ) - }} - > - {copyEmbedText} - - - - )} -
- - - ) -} - -export default ShareDialog -export type { ShareDialogProps } +export { default } from "@/components/ShareDialog/ShareDialog" +export type { ShareDialogProps } from "@/components/ShareDialog/ShareDialog" diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/UpNextSection.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/UpNextSection.tsx index ee95ab1668..2c81adcdc4 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/UpNextSection.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/UpNextSection.tsx @@ -2,29 +2,15 @@ import React from "react" import { RiPlayFill } from "@remixicon/react" import type { VideoResource } from "api/v1" import * as Styled from "./VideoSeriesDetailPage.styled" -import { styled } from "ol-components" -import VideoShareButton from "./VideoShareButton" -import type { VideoPlayerHandle } from "./VideoResourcePlayer" -const StyledVideoShareButton = styled(VideoShareButton)({ - height: "40px", - marginTop: "8px", - padding: "18px 16px", -}) type UpNextSectionProps = { nextVideo: VideoResource getVideoHref: (v: VideoResource) => string - currentVideo: VideoResource - shareUrl: string - playerRef?: React.RefObject } const UpNextSection: React.FC = ({ nextVideo, getVideoHref, - currentVideo, - shareUrl, - playerRef, }) => { return ( @@ -33,12 +19,6 @@ const UpNextSection: React.FC = ({ {nextVideo.title} - ({ }, })) +const StyledVideoShareButton = styled(VideoShareButton)({ + height: "40px", + padding: "18px 12px", +}) + +const VideoShareSection = styled("div")({ + display: "flex", + alignItems: "center", + justifyContent: "space-between", + flexWrap: "wrap", + gap: "8px", + marginBottom: "24px", + [theme.breakpoints.down("sm")]: { + marginBottom: "16px", + }, +}) + const VideoTitle = styled.h1(({ theme }) => ({ ...theme.typography.h2, fontWeight: theme.typography.fontWeightBold, @@ -86,14 +102,9 @@ const VideoTitle = styled.h1(({ theme }) => ({ const MetaRow = styled.div({ ...theme.typography.body2, color: theme.custom.colors.darkGray1, - marginBottom: "24px", - [theme.breakpoints.down("sm")]: { - marginBottom: "16px", - }, }) const StyledVideoResourcePlayer = styled(VideoResourcePlayer)(({ theme }) => ({ - marginTop: "-10px", [theme.breakpoints.down("sm")]: { marginTop: "0", }, @@ -436,13 +447,25 @@ const VideoDetailPage: React.FC = ({ )} - {!isLoading && (duration || topicNames) && ( - - {duration && {duration}} - {topicNames} - - )} - + + {!isLoading && (duration || topicNames) && ( + + + {duration && {duration}} + + {topicNames} + + )} + + {!isLoading && video && ( + + )} + = ({ )} - {!isLoading && video && ( - - - - )} - {/* More from playlist */} {playlistId && (
diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.styled.ts b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.styled.ts index 99bac7a746..28754a22cc 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.styled.ts +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.styled.ts @@ -22,6 +22,18 @@ export const BreadcrumbBar = styled.div(({ theme }) => ({ }, })) +export const VideoShareSection = styled("div")({ + display: "flex", + alignItems: "center", + justifyContent: "space-between", + flexWrap: "wrap", + gap: "8px", + margin: "0 0 24px", + [theme.breakpoints.down("sm")]: { + margin: "0 0 16px", + }, +}) + export const ShareRow = styled.div(({ theme }) => ({ display: "flex", justifyContent: "flex-end", @@ -36,27 +48,7 @@ export const UpNextRight = styled.div({ gap: "24px", }) -export const ShareButton = styled.button(({ theme }) => ({ - display: "flex", - alignItems: "center", - gap: "8px", - borderRadius: "4px", - padding: "14px 12px", - height: "32px", - border: `1px solid ${theme.custom.colors.silverGrayLight}`, - background: `${theme.custom.colors.white}`, - cursor: "pointer", - ...theme.typography.body2, - color: theme.custom.colors.darkGray1, - fontWeight: theme.typography.fontWeightMedium, - "&:hover": { - color: theme.custom.colors.red, - }, - [theme.breakpoints.down("sm")]: { - width: "100%", - justifyContent: "center", - }, -})) +export { default as ShareButton } from "@/components/ShareButton/ShareButton" // ── Series navigation bar ── @@ -289,10 +281,6 @@ export const MetaInstructorLine = styled.div(({ theme }) => ({ export const StyledDuration = styled.div(({ theme }) => ({ ...theme.typography.body2, color: theme.custom.colors.silverGrayDark, - margin: "0 0 40px", - [theme.breakpoints.down("sm")]: { - margin: "0 0 16px", - }, })) export const DescriptionText = styled(Typography)(({ theme }) => ({ diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.tsx index 5b642bb29c..89bf81fc8a 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoSeriesDetailPage.tsx @@ -16,12 +16,19 @@ import { buildVideoStructuredData } from "./videoStructuredData" import VideoResourcePlayer from "./VideoResourcePlayer" import type { VideoPlayerHandle } from "./VideoResourcePlayer" +import VideoShareButton from "./VideoShareButton" + const NEXT_PUBLIC_ORIGIN = env("NEXT_PUBLIC_ORIGIN") const StyledVideoResourcePlayer = styled(VideoResourcePlayer)(({ theme }) => ({ borderBottom: `3px solid ${theme.custom.colors.darkGray2}`, })) +const StyledVideoShareButton = styled(VideoShareButton)({ + height: "40px", + padding: "18px 12px", +}) + type VideoSeriesDetailPageProps = { videoId: number playlistId: number | null @@ -168,9 +175,20 @@ const VideoSeriesDetailPage: React.FC = ({ {video?.title} )} - {duration && ( - {duration} - )} + + + {duration && ( + {duration} + )} + {!itemsLoading && video && ( + + )} + {/* Video player */} = ({ {/* UP NEXT */} {!itemsLoading && nextVideo && video && ( - + )} {/* Description */} diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoShareButton.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoShareButton.tsx index f70dad6447..f124393825 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoShareButton.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoShareButton.tsx @@ -3,7 +3,7 @@ import React, { useState } from "react" import { RiShareForwardFill } from "@remixicon/react" import type { VideoResource } from "api/v1" -import ShareDialog from "./ShareDialog" +import ShareDialog from "@/components/ShareDialog/ShareDialog" import type { VideoPlayerHandle } from "./VideoResourcePlayer" import * as Styled from "./VideoSeriesDetailPage.styled" diff --git a/frontends/main/src/components/ShareButton/ShareButton.tsx b/frontends/main/src/components/ShareButton/ShareButton.tsx new file mode 100644 index 0000000000..bc2955cc24 --- /dev/null +++ b/frontends/main/src/components/ShareButton/ShareButton.tsx @@ -0,0 +1,25 @@ +import { styled } from "ol-components" + +const ShareButton = styled.button(({ theme }) => ({ + display: "flex", + alignItems: "center", + gap: "8px", + borderRadius: "4px", + padding: "14px 12px", + height: "32px", + border: `1px solid ${theme.custom.colors.silverGrayLight}`, + background: `${theme.custom.colors.white}`, + cursor: "pointer", + ...theme.typography.body2, + color: theme.custom.colors.darkGray1, + fontWeight: theme.typography.fontWeightMedium, + "&:hover": { + color: theme.custom.colors.red, + }, + [theme.breakpoints.down("sm")]: { + width: "100%", + justifyContent: "center", + }, +})) + +export default ShareButton diff --git a/frontends/main/src/components/ShareDialog/ShareDialog.tsx b/frontends/main/src/components/ShareDialog/ShareDialog.tsx new file mode 100644 index 0000000000..60587e75d8 --- /dev/null +++ b/frontends/main/src/components/ShareDialog/ShareDialog.tsx @@ -0,0 +1,543 @@ +"use client" + +import React, { useEffect, useRef, useState } from "react" +import { Dialog, styled, theme } from "ol-components" +import Link from "next/link" +import { env } from "@/env" +import { + FACEBOOK_SHARE_BASE_URL, + TWITTER_SHARE_BASE_URL, + LINKEDIN_SHARE_BASE_URL, +} from "@/common/urls" +import { + RiFacebookFill, + RiTwitterXLine, + RiLinkedinFill, + RiLink, + RiShareForwardFill, + RiCodeSSlashLine, +} from "@remixicon/react" +import { Button, Input } from "@mitodl/smoot-design" +import type { VideoResource, PodcastEpisodeResource } from "api/v1" + +const NEXT_PUBLIC_ORIGIN = env("NEXT_PUBLIC_ORIGIN") + +type Tab = "share" | "embed" + +// ─── Layout ──────────────────────────────────────────────────────────────── + +const DialogInner = styled.div({ + display: "flex", + flexDirection: "column", + gap: "24px", +}) + +const TabGroup = styled.div({ + display: "flex", + gap: "10px", +}) + +const TabButton = styled.button<{ active: boolean }>(({ active }) => ({ + display: "inline-flex", + alignItems: "center", + gap: "10px", + padding: "12px 24px 12px 16px", + borderRadius: "24px", + border: "none", + cursor: "pointer", + ...theme.typography.body2, + fontWeight: theme.typography.fontWeightMedium, + lineHeight: "14px", + backgroundColor: active + ? theme.custom.colors.red + : theme.custom.colors.lightGray2, + color: active ? theme.custom.colors.white : theme.custom.colors.darkGray1, + transition: "background-color 0.15s, color 0.15s", + "&:hover": { + backgroundColor: active + ? theme.custom.colors.darkRed + : theme.custom.colors.lightGray2, + }, +})) + +// ─── Share tab ───────────────────────────────────────────────────────────── + +const ShareContents = styled.div(({ theme }) => ({ + display: "flex", + gap: "40px", + [theme.breakpoints.down("sm")]: { + flexDirection: "column", + gap: "32px", + }, +})) + +const SocialContainer = styled.div({ + display: "flex", + flexDirection: "column", + gap: "16px", +}) + +const LinkContainer = styled.div({ + display: "flex", + flexDirection: "column", + gap: "12px", + flex: 1, +}) + +const SectionHeading = styled.span(({ theme }) => ({ + ...theme.typography.body2, + color: theme.custom.colors.darkGray2, + fontWeight: theme.typography.fontWeightBold, +})) + +const SocialButtonRow = styled.div({ + display: "flex", + gap: "16px", + a: { height: "18px" }, +}) + +const ShareLink = styled(Link)({ + color: theme.custom.colors.silverGrayDark, + "&:hover": { color: theme.custom.colors.lightRed }, +}) + +const LinkControls = styled.div(({ theme }) => ({ + display: "flex", + gap: "16px", + input: { + ...theme.typography.body3, + color: theme.custom.colors.darkGray2, + padding: "0 3px", + }, +})) + +const RedLinkIcon = styled(RiLink)({ + color: theme.custom.colors.red, +}) + +const CopyButton = styled(Button)({ + minWidth: "104px", +}) + +// ─── Embed tab ───────────────────────────────────────────────────────────── + +const EmbedContents = styled.div({ + display: "flex", + flexDirection: "column", + gap: "16px", +}) + +const Field = styled.div({ + display: "flex", + flexDirection: "column", + gap: "8px", +}) + +const EmbedTextarea = styled.textarea(({ theme }) => ({ + width: "100%", + minHeight: "100px", + resize: "vertical", + borderRadius: "4px", + border: `1px solid ${theme.custom.colors.lightGray2}`, + padding: "8px", + ...theme.typography.body3, + color: theme.custom.colors.darkGray2, + fontFamily: "monospace", + boxSizing: "border-box", +})) + +const EmbedFooter = styled.div({ + display: "flex", + justifyContent: "end", + alignItems: "center", +}) + +const StartAtRow = styled.div(({ theme }) => ({ + display: "flex", + alignItems: "center", + gap: "8px", + ...theme.typography.body2, + color: theme.custom.colors.darkGray2, + "input[type='checkbox']": { + width: "16px", + height: "16px", + cursor: "pointer", + accentColor: theme.custom.colors.red, + }, + label: { + cursor: "pointer", + userSelect: "none", + }, +})) + +// ─── Helpers ─────────────────────────────────────────────────────────────── + +async function copyToClipboard(text: string): Promise { + if (!navigator?.clipboard) throw new Error("Clipboard API unavailable") + await navigator.clipboard.writeText(text) +} + +function getYouTubeVideoId(url: string | null | undefined): string | null { + if (!url) return null + const patterns = [ + /[?&]v=([^&]+)/, + /youtu\.be\/([^?&]+)/, + /youtube\.com\/embed\/([^?&]+)/, + ] + for (const pattern of patterns) { + const match = url.match(pattern) + if (match) return match[1] + } + return null +} + +function escapeHtmlAttr(value: string): string { + return value + .replace(/&/g, "&") + .replace(/"/g, """) + .replace(/ 0 ? `${h}:${mm}:${ss}` : `${m}:${ss}` +} + +function withTimeParam(url: string, seconds: number, param = "t"): string { + try { + const u = new URL(url) + u.searchParams.set(param, String(Math.floor(seconds))) + return u.toString() + } catch { + return url + } +} + +function buildVideoEmbedHtml( + video: VideoResource, + title: string, + embedPageUrl: string, + startTime = 0, +): string { + const isOvs = video.platform?.code === "ovs" || video.platform?.code === "ocw" + const escapedTitle = escapeHtmlAttr(title) + const t = Math.floor(startTime) + if (isOvs) { + if (!embedPageUrl) return "" + const src = t > 0 ? withTimeParam(embedPageUrl, t) : embedPageUrl + return `` + } + + const youtubeId = getYouTubeVideoId(video.url) + if (!youtubeId) return "" + const ytBase = `https://www.youtube.com/embed/${youtubeId}` + const src = t > 0 ? withTimeParam(ytBase, t, "start") : ytBase + return `` +} + +function buildPodcastEmbedHtml( + resource: PodcastEpisodeResource, + title: string, + _embedPageUrl: string, +): string { + const episode = resource.podcast_episode + if (!episode?.audio_url) return "" + const escapedTitle = escapeHtmlAttr(title) + const escapedAudioUrl = escapeHtmlAttr(episode.audio_url) + return `` +} + +// ─── Component ───────────────────────────────────────────────────────────── + +type ShareDialogProps = { + open: boolean + onClose: () => void + video?: VideoResource + resource?: PodcastEpisodeResource + pageUrl: string + title: string + getCurrentTime?: () => number +} + +const ShareDialog = ({ + open, + onClose, + video, + resource, + pageUrl, + title, + + getCurrentTime, +}: ShareDialogProps) => { + const [activeTab, setActiveTab] = useState("share") + const [copyLinkText, setCopyLinkText] = useState("Copy Link") + const [copyEmbedText, setCopyEmbedText] = useState("Copy") + const [startTime, setStartTime] = useState(0) + const [includeTime, setIncludeTime] = useState(false) + + const wasOpenRef = useRef(false) + useEffect(() => { + if (open && !wasOpenRef.current) { + const t = getCurrentTime?.() ?? 0 + setStartTime(t) + setIncludeTime(t > 0) + } + wasOpenRef.current = open + }, [open, getCurrentTime]) + + const copyLinkTimerRef = useRef | null>(null) + const copyEmbedTimerRef = useRef | null>(null) + + useEffect( + () => () => { + if (copyLinkTimerRef.current) clearTimeout(copyLinkTimerRef.current) + if (copyEmbedTimerRef.current) clearTimeout(copyEmbedTimerRef.current) + }, + [], + ) + + const shareTabRef = useRef(null) + const embedTabRef = useRef(null) + const tabRefs: Record> = { + share: shareTabRef, + embed: embedTabRef, + } + const TABS: Tab[] = ["share", "embed"] + + const handleTabKeyDown = + (tab: Tab) => (e: React.KeyboardEvent) => { + const idx = TABS.indexOf(tab) + let next: Tab | null = null + if (e.key === "ArrowRight") next = TABS[(idx + 1) % TABS.length] + else if (e.key === "ArrowLeft") + next = TABS[(idx - 1 + TABS.length) % TABS.length] + if (next) { + e.preventDefault() + setActiveTab(next) + tabRefs[next].current?.focus() + } + } + + const hasEmbed = video !== undefined || resource !== undefined + + const videoEmbedPageUrl = video + ? `${NEXT_PUBLIC_ORIGIN}/video/embed/${video.id}` + : "" + const embedUrl = video + ? videoEmbedPageUrl + : resource + ? `${NEXT_PUBLIC_ORIGIN}/podcast/embed/${resource.id}` + : null + + const t = includeTime && startTime > 0 ? Math.floor(startTime) : 0 + + const activeShareUrl = pageUrl + const activeEmbedUrl = + embedUrl && t > 0 ? withTimeParam(embedUrl, t) : embedUrl + + const embedHtml = video + ? buildVideoEmbedHtml(video, title, videoEmbedPageUrl, t) + : resource + ? buildPodcastEmbedHtml(resource, title, embedUrl || "") + : "" + + const embedUrlLabel = video ? "Video URL" : "Audio URL" + + const startAtLabel = + video && startTime > 0 ? `Start at ${formatTimestamp(startTime)}` : null + + return ( + + + {hasEmbed && ( + + setActiveTab("share")} + onKeyDown={handleTabKeyDown("share")} + > + + Share + + setActiveTab("embed")} + onKeyDown={handleTabKeyDown("embed")} + > + + Embed + + + )} + +
+ {activeTab === "share" || !hasEmbed ? ( + + + Share on Social + + + + + + + + + + + + + + Share a link + + { + const input = event.currentTarget.querySelector("input") + if (!input) return + input.select() + }} + /> + } + onClick={async () => { + if (!activeShareUrl) return + try { + await copyToClipboard(activeShareUrl) + setCopyLinkText("Copied!") + } catch { + setCopyLinkText("Failed to copy") + } + if (copyLinkTimerRef.current) + clearTimeout(copyLinkTimerRef.current) + copyLinkTimerRef.current = setTimeout( + () => setCopyLinkText("Copy Link"), + 2000, + ) + }} + > + {copyLinkText} + + + + + ) : ( + + + {embedUrlLabel} + { + const input = event.currentTarget.querySelector("input") + if (!input) return + input.select() + }} + /> + + + Embed HTML + (e.target as HTMLTextAreaElement).select()} + /> + + + {startAtLabel && ( + + setIncludeTime(e.target.checked)} + /> + + + )} + } + onClick={async () => { + if (!embedHtml) return + try { + await copyToClipboard(embedHtml) + setCopyEmbedText("Copied!") + } catch { + setCopyEmbedText("Failed to copy") + } + if (copyEmbedTimerRef.current) + clearTimeout(copyEmbedTimerRef.current) + copyEmbedTimerRef.current = setTimeout( + () => setCopyEmbedText("Copy"), + 2000, + ) + }} + > + {copyEmbedText} + + + + )} +
+ + + ) +} + +export default ShareDialog +export type { ShareDialogProps } diff --git a/frontends/main/src/page-components/ResourceCard/ResourceCard.test.tsx b/frontends/main/src/page-components/ResourceCard/ResourceCard.test.tsx index 09bd81de4d..712f5ddc0e 100644 --- a/frontends/main/src/page-components/ResourceCard/ResourceCard.test.tsx +++ b/frontends/main/src/page-components/ResourceCard/ResourceCard.test.tsx @@ -6,10 +6,8 @@ import { screen, expectProps, waitFor, - fireEvent, } from "@/test-utils" import type { User } from "api/hooks/user" -import { learningResourceQueries } from "api/hooks/learningResources" import { ResourceCard } from "./ResourceCard" import { MicroUserListRelationship } from "api" import { @@ -235,51 +233,4 @@ describe.each([ String(resource.id), ) }) - - test("Clicking a resource card sets resource detail cache", async () => { - const { resource, view, queryClient } = setup() - await user.click(view.container.firstChild as HTMLElement) - - invariant(resource) - const cached = queryClient.getQueryData( - learningResourceQueries.detail(resource.id).queryKey, - ) - expect(cached).toEqual(resource) - }) - - test("Share button appears for podcast episode resource", async () => { - const resource = factories.learningResources.podcastEpisode() - setup({ user: { is_authenticated: false }, props: { resource } }) - await screen.findByRole("button", { name: `Share ${resource.title}` }) - }) - - test("Share button does not appear for non-podcast-episode resources", async () => { - const resource = factories.learningResources.course() - setup({ user: { is_authenticated: false }, props: { resource } }) - await screen.findByRole("button", { - name: `Bookmark ${resource.resource_category}`, - }) - expect( - screen.queryByRole("button", { name: /^Share / }), - ).not.toBeInTheDocument() - }) - - test("Clicking Share button opens ShareDialog for podcast episode", async () => { - const resource = factories.learningResources.podcastEpisode() - setup({ user: { is_authenticated: false }, props: { resource } }) - const shareButton = await screen.findByRole("button", { - name: `Share ${resource.title}`, - }) - expect(screen.getByTestId("share-dialog")).toHaveAttribute( - "data-open", - "false", - ) - fireEvent.click(shareButton) - await waitFor(() => { - expect(screen.getByTestId("share-dialog")).toHaveAttribute( - "data-open", - "true", - ) - }) - }) }) diff --git a/frontends/main/src/page-components/ResourceCard/ResourceCard.tsx b/frontends/main/src/page-components/ResourceCard/ResourceCard.tsx index d7c50f4d8c..372bd4ddd5 100644 --- a/frontends/main/src/page-components/ResourceCard/ResourceCard.tsx +++ b/frontends/main/src/page-components/ResourceCard/ResourceCard.tsx @@ -8,16 +8,11 @@ import { } from "../Dialogs/AddToListDialog" import { useResourceDrawerHref } from "../LearningResourceDrawer/useResourceDrawerHref" import { useUserMe } from "api/hooks/user" -import { LearningResource, PodcastEpisodeResource, ResourceTypeEnum } from "api" +import { LearningResource } from "api" import { SignupPopover } from "../SignupPopover/SignupPopover" import { useIsUserListMember } from "api/hooks/userLists" import { useLearningResourceDetailSetCache } from "api/hooks/learningResources" import { useIsLearningPathMember } from "api/hooks/learningPaths" -import ShareDialog from "@/app-pages/VideoPlaylistCollectionPage/ShareDialog" -import { env } from "@/env" -import { podcastEpisodePageView } from "@/common/urls" - -const NEXT_PUBLIC_ORIGIN = env("NEXT_PUBLIC_ORIGIN") export const useResourceCard = (resource?: LearningResource | null) => { const getDrawerHref = useResourceDrawerHref() @@ -26,7 +21,6 @@ export const useResourceCard = (resource?: LearningResource | null) => { const { data: inLearningPath } = useIsLearningPathMember(resource?.id) const [anchorEl, setAnchorEl] = useState(null) - const [shareAnchorEl, setShareAnchorEl] = useState(null) const handleClosePopover = useCallback(() => { setAnchorEl(null) @@ -57,11 +51,6 @@ export const useResourceCard = (resource?: LearningResource | null) => { } }, [user]) - const handleShareClick: LearningResourceCardProps["onShareClick"] = - useCallback((event: React.MouseEvent) => { - setShareAnchorEl(event.currentTarget) - }, []) - const onClick = useLearningResourceDetailSetCache(resource) return { @@ -71,9 +60,6 @@ export const useResourceCard = (resource?: LearningResource | null) => { handleClosePopover, handleAddToLearningPathClick, handleAddToUserListClick, - handleShareClick, - shareAnchorEl, - setShareAnchorEl, inUserList, inLearningPath, } @@ -92,7 +78,7 @@ const subheadingMap: Record = { type ResourceCardProps = Omit< LearningResourceCardProps, - "href" | "onAddToLearningPathClick" | "onAddToUserListClick" | "onShareClick" + "href" | "onAddToLearningPathClick" | "onAddToUserListClick" > & { headingLevel?: number parentHeadingEl?: HeadingElement @@ -118,9 +104,6 @@ const ResourceCard: React.FC = ({ handleClosePopover, handleAddToLearningPathClick, handleAddToUserListClick, - handleShareClick, - shareAnchorEl, - setShareAnchorEl, inUserList, inLearningPath, onClick, @@ -135,16 +118,6 @@ const ResourceCard: React.FC = ({ const headingLevel = parentHeadingEl ? subheadingMap[parentHeadingEl] : 6 - const isPodcastEpisode = - resource?.resource_type === ResourceTypeEnum.PodcastEpisode - const podcastId = isPodcastEpisode - ? resource?.podcast_episode?.podcasts?.[0] - : undefined - const sharePageUrl = - isPodcastEpisode && podcastId !== undefined - ? `${NEXT_PUBLIC_ORIGIN}${podcastEpisodePageView(String(resource!.id), String(podcastId), resource?.title)}` - : "" - return ( <> = ({ href={resource ? getDrawerHref(resource.id) : undefined} onAddToLearningPathClick={handleAddToLearningPathClick} onAddToUserListClick={handleAddToUserListClick} - onShareClick={ - isPodcastEpisode && podcastId !== undefined ? handleShareClick : null - } inUserList={inUserList} inLearningPath={inLearningPath} headingLevel={headingLevel} {...others} /> - {isPodcastEpisode && podcastId !== undefined && ( - setShareAnchorEl(null)} - resource={resource as PodcastEpisodeResource} - pageUrl={sharePageUrl} - title={resource?.title ?? ""} - /> - )} ) } diff --git a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx index a2fedf67f7..a9cecfc047 100644 --- a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx +++ b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx @@ -1,11 +1,6 @@ import React from "react" -import { - RiMenuAddLine, - RiBookmarkLine, - RiBookmarkFill, - RiShareForwardFill, -} from "@remixicon/react" -import { LearningResource, ResourceTypeEnum } from "api" +import { RiMenuAddLine, RiBookmarkLine, RiBookmarkFill } from "@remixicon/react" +import { LearningResource } from "api" import { LocalDate, DEFAULT_RESOURCE_IMG, @@ -36,7 +31,6 @@ interface LearningResourceCardProps { href?: string onAddToLearningPathClick?: ResourceIdCallback | null onAddToUserListClick?: ResourceIdCallback | null - onShareClick?: ResourceIdCallback | null inUserList?: boolean inLearningPath?: boolean onClick?: React.MouseEventHandler @@ -54,7 +48,6 @@ const LearningResourceCard: React.FC = ({ href, onAddToLearningPathClick, onAddToUserListClick, - onShareClick, inLearningPath, inUserList, onClick, @@ -96,7 +89,6 @@ const LearningResourceCard: React.FC = ({ href={href} onAddToLearningPathClick={onAddToLearningPathClick} onAddToUserListClick={onAddToUserListClick} - onShareClick={onShareClick} inUserList={inUserList} inLearningPath={inLearningPath} onClick={onClick} @@ -156,17 +148,6 @@ const LearningResourceCard: React.FC = ({ }) } - if ( - onShareClick && - resource.resource_type === ResourceTypeEnum.PodcastEpisode - ) { - actions.push({ - onClick: (event) => onShareClick(event, resource.id), - "aria-label": `Share ${resource.title}`, - icon: , - }) - } - return ( = ({ href, onAddToLearningPathClick, onAddToUserListClick, - onShareClick, editMenu, inLearningPath, inUserList, @@ -206,17 +199,6 @@ const LearningResourceListCard: React.FC = ({ }) } - if ( - onShareClick && - resource.resource_type === ResourceTypeEnum.PodcastEpisode - ) { - actions.push({ - onClick: (event) => onShareClick(event, resource.id), - "aria-label": `Share ${resource.title}`, - icon: , - }) - } - const footerContent = ( From d17373bfa246acb1462627a7f71c929db6ad86c3 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Tue, 7 Jul 2026 10:01:14 -0400 Subject: [PATCH 6/8] feat: local-dev Docker targets and granian kill timeout (#3579) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add opt-in build targets consumed by the ol-infrastructure local-dev Tilt stack: - Dockerfile: a local-dev stage based on development (so dev deps — pytest, ipdb, etc. — are available) with a runtime-user-owned /src so live-synced source changes can be written. - frontends/main/Dockerfile.web: a local-dev stage that runs next dev, placed before the default runner stage so target-less builds (CI) still produce the production image. Production stages are unchanged. Also add --workers-kill-timeout (default 1s, overridable via GRANIAN_WORKERS_KILL_TIMEOUT) to run-django-dev.sh: granian workers sometimes never finish their graceful stop, which leaves --reload hung indefinitely mid-reload until the container is restarted. Co-authored-by: Claude Fable 5 --- Dockerfile | 10 +++++++++- frontends/main/Dockerfile.web | 7 +++++++ scripts/run-django-dev.sh | 3 ++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/Dockerfile b/Dockerfile index b7caa4da66..3ea19f46de 100644 --- a/Dockerfile +++ b/Dockerfile @@ -47,8 +47,16 @@ EXPOSE 8061 ENV PORT=8061 CMD ["sh", "-c", "exec granian --interface asginl --reload --host 0.0.0.0 --port ${PORT:-8061} --workers ${GRANIAN_WORKERS:-3} --blocking-threads 1 main.asgi:application"] -# ─── Development target ─────────────────────────────────────────────────────── +# ─── Development target (docker compose) ───────────────────────────────────── FROM final AS development RUN --mount=type=cache,target=/opt/uv-cache,uid=1000,gid=1000 \ uv sync --frozen --no-install-project + +# ─── Local-dev target (ol-infrastructure local-dev k8s/Tilt stack) ─────────── +# Runtime user owns /src (live-synced source); dev deps come from `development`. +FROM development AS local-dev + +USER root +RUN chown -R mitodl:mitodl /src +USER mitodl diff --git a/frontends/main/Dockerfile.web b/frontends/main/Dockerfile.web index 1371f0e2b8..5efbe72444 100644 --- a/frontends/main/Dockerfile.web +++ b/frontends/main/Dockerfile.web @@ -91,6 +91,13 @@ ENV NEXT_BUILD_CI=1 RUN yarn build +# STAGE: local-dev +# Local-dev target (ol-infrastructure local-dev k8s/Tilt stack): runs `next dev` +FROM base AS local-dev + +ENV NODE_ENV=development +CMD ["yarn", "dev"] + # STAGE: runner (default) # Copies only the standalone server, static assets, and public directory from # the build stage into a clean image. No workspace install is needed; the diff --git a/scripts/run-django-dev.sh b/scripts/run-django-dev.sh index 0a453be99a..c1c175f99e 100755 --- a/scripts/run-django-dev.sh +++ b/scripts/run-django-dev.sh @@ -7,7 +7,8 @@ python3 manage.py collectstatic --noinput --clear # run initial django migrations python3 manage.py migrate --noinput -granian --interface asginl --host 0.0.0.0 --port "${PORT:-8061}" --workers "${GRANIAN_WORKERS:-3}" --blocking-threads 1 --reload --reload-ignore-dirs frontends --reload-ignore-dirs staticfiles --reload-ignore-dirs .git main.asgi:application & +# --workers-kill-timeout: workers sometimes hang during graceful stop, wedging --reload. +granian --interface asginl --host 0.0.0.0 --port "${PORT:-8061}" --workers "${GRANIAN_WORKERS:-3}" --workers-kill-timeout "${GRANIAN_WORKERS_KILL_TIMEOUT:-1}" --blocking-threads 1 --reload --reload-ignore-dirs frontends --reload-ignore-dirs staticfiles --reload-ignore-dirs .git main.asgi:application & GRANIAN_PID=$! echo "Application started with PID $GRANIAN_PID" From 8886937bce1a250a9ca3e58eb0d3f0547206785f Mon Sep 17 00:00:00 2001 From: Danielle Frappier Date: Tue, 7 Jul 2026 12:49:48 -0400 Subject: [PATCH 7/8] fix: handle no-max-learners contracts in B2B seat assignment UI (#3564) --- frontends/api/package.json | 2 +- frontends/main/package.json | 2 +- .../AssignSeatsConfirmModal.test.tsx | 63 ++++++++ .../AssignSeatsConfirmModal.tsx | 34 +++-- .../AssignSeatsSection.test.tsx | 59 ++++++++ .../ContractAdminPage/AssignSeatsSection.tsx | 9 +- .../ContractAdminPage.test.tsx | 142 +++++++++++++++++- .../ContractAdminPage/ContractAdminPage.tsx | 56 ++++--- .../DashboardPage/ContractContent.test.tsx | 55 +++++++ .../DashboardPage/ContractContent.tsx | 17 ++- yarn.lock | 12 +- 11 files changed, 405 insertions(+), 46 deletions(-) diff --git a/frontends/api/package.json b/frontends/api/package.json index 9986a8c1ef..87960b84a6 100644 --- a/frontends/api/package.json +++ b/frontends/api/package.json @@ -31,7 +31,7 @@ "ol-test-utilities": "0.0.0" }, "dependencies": { - "@mitodl/mitxonline-api-axios": "2026.6.30-1", + "@mitodl/mitxonline-api-axios": "2026.7.7", "@tanstack/react-query": "^5.66.0", "axios": "^1.12.2", "tiny-invariant": "^1.3.3" diff --git a/frontends/main/package.json b/frontends/main/package.json index 616b656ba3..8b65d5fb23 100644 --- a/frontends/main/package.json +++ b/frontends/main/package.json @@ -18,7 +18,7 @@ "@mitodl/arithmix": "^0.2.2", "@mitodl/course-search-utils": "^3.5.2", "@mitodl/hacksnack": "^0.1.0", - "@mitodl/mitxonline-api-axios": "2026.6.30-1", + "@mitodl/mitxonline-api-axios": "2026.7.7", "@mitodl/smoot-design": "^6.27.0", "@mui/base": "5.0.0-beta.70", "@mui/material": "^6.4.5", diff --git a/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.test.tsx b/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.test.tsx index 9f393e9635..5678ad1357 100644 --- a/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.test.tsx +++ b/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.test.tsx @@ -297,6 +297,69 @@ describe("AssignSeatsConfirmModal — over-capacity state (CSV only)", () => { }) }) +describe("AssignSeatsConfirmModal — unlimited contract (availableSeats null)", () => { + beforeEach(() => jest.clearAllMocks()) + + const unlimitedProps = { ...baseProps, availableSeats: null } + + test("shows unlimited messaging in the seats stat instead of a number", () => { + renderWithTheme( + , + ) + + // Confirm step still renders normally. + expect( + screen.getByRole("heading", { name: /ready to send invitations/i }), + ).toBeInTheDocument() + + // Stat value is a dash and the label / group name convey no seat limit. + expect(screen.getByText("—")).toBeInTheDocument() + expect(screen.getByText("No seat limit")).toBeInTheDocument() + expect( + screen.getByRole("group", { name: /no seat limit on this contract/i }), + ).toBeInTheDocument() + }) + + test("does not display a numeric or negative 'seats remaining after sending'", () => { + // validCount far exceeds any real cap; with no cap there must be no negative. + renderWithTheme( + , + ) + + expect( + screen.queryByText(/seats remaining after sending/i), + ).not.toBeInTheDocument() + expect( + screen.queryByRole("group", { name: /seats remaining after sending/i }), + ).not.toBeInTheDocument() + // A cap of null run through `availableSeats - validCount` would render -100. + expect(screen.queryByText("-100")).not.toBeInTheDocument() + }) + + test("never enters the over-capacity 'Not enough seats available' state", () => { + renderWithTheme( + , + ) + + expect( + screen.getByRole("heading", { name: /ready to send invitations/i }), + ).toBeInTheDocument() + expect( + screen.queryByRole("heading", { name: /not enough seats available/i }), + ).not.toBeInTheDocument() + }) + + test("dialog accessible description conveys no seat limit", () => { + renderWithTheme( + , + ) + + expect(screen.getByRole("dialog")).toHaveAccessibleDescription( + /no seat limit on this contract/i, + ) + }) +}) + describe("AssignSeatsConfirmModal — email preview (send test email)", () => { const sendTestEmailProps = { ...baseProps, diff --git a/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.tsx b/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.tsx index fccd2bea59..f9aed5fa61 100644 --- a/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.tsx +++ b/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsConfirmModal.tsx @@ -241,7 +241,8 @@ type AssignSeatsConfirmModalProps = { onClose: () => void onConfirm: () => void | Promise validCount: number - availableSeats: number + /** Null means the contract has no max_learners cap — never over capacity. */ + availableSeats: number | null invalidEmails: string[] duplicateEmails: string[] skippedCount: number @@ -274,7 +275,7 @@ const AssignSeatsConfirmModal: React.FC = ({ const hasInvalid = invalidEmails.length > 0 const hasDuplicates = duplicateEmails.length > 0 const hasIssues = hasInvalid || hasDuplicates || skippedCount > 0 - const overCapacity = validCount > availableSeats + const overCapacity = availableSeats !== null && validCount > availableSeats const [step, setStep] = useState(() => hasIssues && !overCapacity ? "review" : "confirm", @@ -414,8 +415,15 @@ const AssignSeatsConfirmModal: React.FC = ({ } } - const seatsAfterSending = availableSeats - validCount - const overLimit = validCount - availableSeats + const seatsAfterSending = + availableSeats !== null ? availableSeats - validCount : null + const seatsAfterSendingText = + seatsAfterSending !== null + ? `${seatsAfterSending} seats remaining after sending.` + : "No seat limit on this contract." + // Only ever read from the overCapacity branches below, and overCapacity is + // false whenever availableSeats is null, so this fallback is never shown. + const overLimit = availableSeats !== null ? validCount - availableSeats : 0 const reviewTitle = hasInvalid ? "Some learners could not be added" @@ -429,7 +437,7 @@ const AssignSeatsConfirmModal: React.FC = ({ // to re-discover a new role="dialog" element. const handleReviewAndConfirm = () => { setStep("confirm") - const confirmText = `Ready to send invitations. You are about to send ${validCount} invitation ${pluralize("email", validCount)} from MIT Learn. Learners will receive an email with a secure link to claim their seat and access the materials. ${seatsAfterSending} seats remaining after sending. Emails will be sent immediately and cannot be recalled.` + const confirmText = `Ready to send invitations. You are about to send ${validCount} invitation ${pluralize("email", validCount)} from MIT Learn. Learners will receive an email with a secure link to claim their seat and access the materials. ${seatsAfterSendingText} Emails will be sent immediately and cannot be recalled.` setStepAnnouncement("") if (stepAnnouncementTimerRef.current) clearTimeout(stepAnnouncementTimerRef.current) @@ -503,7 +511,7 @@ const AssignSeatsConfirmModal: React.FC = ({ : "", " Only valid, unique emails will be assigned.", ].join("") - : `You are about to send ${validCount} invitation ${pluralize("email", validCount)} from MIT Learn. Learners will receive an email with secure link to claim their seat and access the materials. ${seatsAfterSending} seats remaining after sending. Emails will be sent immediately and cannot be recalled.` + : `You are about to send ${validCount} invitation ${pluralize("email", validCount)} from MIT Learn. Learners will receive an email with secure link to claim their seat and access the materials. ${seatsAfterSendingText} Emails will be sent immediately and cannot be recalled.` // For overCapacity we point aria-describedby at the visible paragraph so // role="alertdialog" reads it exactly once on open. We do NOT programmatically @@ -716,11 +724,19 @@ const AssignSeatsConfirmModal: React.FC = ({