From 988e827e54ba17c07a95e2b3ecc65800a3a913db Mon Sep 17 00:00:00 2001 From: Muhammad Arslan Date: Thu, 30 Jul 2026 17:52:15 +0500 Subject: [PATCH 1/8] feat: retire course run cmd added to retire a b2b contract (#3797) --- b2b/api.py | 154 +++++ b2b/commands_test.py | 20 +- b2b/constants.py | 9 + b2b/management/commands/b2b_courseware.py | 62 +- courses/admin.py | 31 + .../management/commands/retire_courserun.py | 424 ++++++++++++ .../management/tests/retire_courserun_test.py | 628 ++++++++++++++++++ courses/retirement.py | 481 ++++++++++++++ 8 files changed, 1766 insertions(+), 43 deletions(-) create mode 100644 courses/management/commands/retire_courserun.py create mode 100644 courses/management/tests/retire_courserun_test.py create mode 100644 courses/retirement.py diff --git a/b2b/api.py b/b2b/api.py index d6eb677c19..8c292305ed 100644 --- a/b2b/api.py +++ b/b2b/api.py @@ -20,7 +20,11 @@ from b2b.constants import ( B2B_RUN_TAG_FORMAT, CONTRACT_MEMBERSHIP_AUTOS, + CONTRACT_MEMBERSHIP_MANAGED, ORG_KEY_MAX_LENGTH, + RETIREMENT_CONTRACT_NAME, + RETIREMENT_ORG_KEY, + RETIREMENT_ORG_NAME, ) from b2b.exceptions import SourceCourseIncompleteError from b2b.keycloak_admin_api import KCAM_ORGANIZATIONS, get_keycloak_model @@ -111,6 +115,156 @@ def ensure_b2b_organization_index() -> OrganizationIndexPage: return org_index_page +class RetirementContractCollisionError(Exception): + """Raised when a run can't be moved into the holding contract.""" + + +def get_or_create_retirement_contract() -> ContractPage: + """ + Get (or create) the holding contract that retired course runs live in. + + Moving a retired run here rather than nulling its ``b2b_contract`` matters: + ``CourseRunQuerySet.exclude_b2b()`` is ``b2b_contract__isnull=True``, so a + run with no contract becomes a candidate for the *public* catalog. Parking + it against an inactive contract keeps it out of the public catalog and out + of every org/contract catalog query, which filter on + ``b2b_contract__active=True``. + + Both pages are created unpublished so they are never served, and the + contract is inactive with a zero learner cap. The org has no + ``sso_organization_id``, which is safe: ``reconcile_keycloak_orgs`` only + creates or updates pages for orgs Keycloak knows about and never prunes + ones it doesn't. + + Returns: + ContractPage: the holding contract. + """ + + org = OrganizationPage.objects.filter(org_key=RETIREMENT_ORG_KEY).first() + + if not org: + # Prefer an existing index page. ensure_b2b_organization_index() also + # re-parents every OrganizationPage when its child count doesn't match, + # which is a side effect we don't want to trigger from here. + org_index = OrganizationIndexPage.objects.first() or ( + ensure_b2b_organization_index() + ) + org = OrganizationPage( + name=RETIREMENT_ORG_NAME, + org_key=RETIREMENT_ORG_KEY, + live=False, + description=( + "System organization. Holds course runs that have been retired. " + "Not a real customer - do not add members or contracts." + ), + ) + org_index.add_child(instance=org) + org.refresh_from_db() + log.info("Created retirement holding organization %s", org) + + contract = ContractPage.objects.filter( + organization=org, name=RETIREMENT_CONTRACT_NAME + ).first() + + if not contract: + contract = ContractPage( + name=RETIREMENT_CONTRACT_NAME, + organization=org, + membership_type=CONTRACT_MEMBERSHIP_MANAGED, + active=False, + max_learners=0, + contract_start=None, + contract_end=None, + live=False, + description=( + "System contract. Retired course runs are parked here so they " + "stay hidden but keep their courseware IDs. Never add programs " + "or learners to this contract." + ), + ) + org.add_child(instance=contract) + contract.refresh_from_db() + log.info("Created retirement holding contract %s", contract) + + return contract + + +def check_retirement_contract_collision(run: CourseRun, contract: ContractPage) -> None: + """ + Check that moving the run into the holding contract won't break a constraint. + + ``CourseRun`` has two unique constraints that include ``b2b_contract`` with + ``nulls_distinct=False`` - ``unique_primary_language_per_group`` and + ``unique_language_per_group``. Collisions are unlikely in practice because + the B2B run tag embeds the source contract ID and year, but an + ``IntegrityError`` mid-command is a much worse outcome than a clear refusal. + + Args: + run (CourseRun): the run being moved. + contract (ContractPage): the holding contract. + Raises: + RetirementContractCollisionError: if a conflicting run is already parked. + """ + + siblings = CourseRun.all_objects.filter( + course=run.course, + run_tag=run.run_tag, + is_source_run=run.is_source_run, + b2b_contract=contract, + ).exclude(pk=run.pk) + + if ( + run.language + and siblings.filter( + language=run.language, + variant_length=run.variant_length, + variant_industry=run.variant_industry, + ).exists() + ): + msg = ( + f"A run for {run.course.readable_id} with run tag '{run.run_tag}', " + f"language '{run.language}' and variant " + f"'{run.variant_length}/{run.variant_industry}' is already parked in " + f"{contract}. Rename the run tag or clear the existing one first." + ) + raise RetirementContractCollisionError(msg) + + if run.is_primary_language and siblings.filter(is_primary_language=True).exists(): + msg = ( + f"A primary-language run for {run.course.readable_id} with run tag " + f"'{run.run_tag}' is already parked in {contract}. Rename the run tag " + "or clear the existing one first." + ) + raise RetirementContractCollisionError(msg) + + +def move_run_to_retirement_contract(run: CourseRun) -> ContractPage: + """ + Move a course run into the holding contract. + + Args: + run (CourseRun): the run to park. + Returns: + ContractPage: the holding contract the run was moved to. + Raises: + RetirementContractCollisionError: if a conflicting run is already parked. + """ + + contract = get_or_create_retirement_contract() + + if run.b2b_contract_id == contract.id: + return contract + + check_retirement_contract_collision(run, contract) + + run.b2b_contract = contract + run.save() + + log.info("Moved course run %s to %s", run.courseware_id, contract) + + return contract + + @transaction.atomic def create_contract_run_key( source_course: CourseRun, contract: ContractPage, *, org_prefix: str | None = None diff --git a/b2b/commands_test.py b/b2b/commands_test.py index 8c854ac87a..e7ef0f105c 100644 --- a/b2b/commands_test.py +++ b/b2b/commands_test.py @@ -33,7 +33,10 @@ def _create_run_with_product_and_discount(contract, *, with_enrollment=False): def test_b2b_courseware_remove_run_without_enrollments_unlinks_and_deactivates(mocker): """Removing a run with no enrollments should unlink it and deactivate related objects.""" - mocker.patch("b2b.management.commands.b2b_courseware.update_edx_course") + # b2b_courseware pushes dates through courses.retirement now, shared with + # retire_courserun. Patch the edX boundary rather than the helper, so the + # payload-building logic still runs under test. + mocker.patch("courses.retirement.update_edx_course") contract = ContractPageFactory.create() run, product, discount = _create_run_with_product_and_discount( @@ -69,7 +72,10 @@ def test_b2b_courseware_remove_run_with_enrollments_keeps_contract_and_deactivat ): """Removing a run with enrollments should keep contract link but deactivate run/products/codes.""" - mocker.patch("b2b.management.commands.b2b_courseware.update_edx_course") + # b2b_courseware pushes dates through courses.retirement now, shared with + # retire_courserun. Patch the edX boundary rather than the helper, so the + # payload-building logic still runs under test. + mocker.patch("courses.retirement.update_edx_course") contract = ContractPageFactory.create() run, product, discount = _create_run_with_product_and_discount( @@ -105,7 +111,10 @@ def test_b2b_courseware_remove_run_does_not_delete_used_discount_order_redemptio ): """Discounts that have been used for an order should not be deleted.""" - mocker.patch("b2b.management.commands.b2b_courseware.update_edx_course") + # b2b_courseware pushes dates through courses.retirement now, shared with + # retire_courserun. Patch the edX boundary rather than the helper, so the + # payload-building logic still runs under test. + mocker.patch("courses.retirement.update_edx_course") contract = ContractPageFactory.create() run, product, discount = _create_run_with_product_and_discount( @@ -128,7 +137,10 @@ def test_b2b_courseware_remove_run_does_not_delete_used_discount_contract_attach ): """Discounts that have been used to attach a user to a contract should not be deleted.""" - mocker.patch("b2b.management.commands.b2b_courseware.update_edx_course") + # b2b_courseware pushes dates through courses.retirement now, shared with + # retire_courserun. Patch the edX boundary rather than the helper, so the + # payload-building logic still runs under test. + mocker.patch("courses.retirement.update_edx_course") contract = ContractPageFactory.create() run, product, discount = _create_run_with_product_and_discount( diff --git a/b2b/constants.py b/b2b/constants.py index 10522580cb..570f0f2c47 100644 --- a/b2b/constants.py +++ b/b2b/constants.py @@ -32,3 +32,12 @@ B2B_RUN_TAG_FORMAT = "{run_idx}T{contract_id}C{year}" ORG_KEY_MAX_LENGTH = 30 + +# The holding org/contract that retired course runs get moved into. Runs parked +# here are hidden from every catalog path because the contract is inactive and +# has no members, but the CourseRun row survives - which matters, because +# create_contract_run_key() derives its run index from existing courseware IDs, +# so deleting a retired run risks minting a duplicate courseware ID later. +RETIREMENT_ORG_KEY = "RETIRED" +RETIREMENT_ORG_NAME = "Retired Runs" +RETIREMENT_CONTRACT_NAME = "Retired Runs Holding Contract" diff --git a/b2b/management/commands/b2b_courseware.py b/b2b/management/commands/b2b_courseware.py index 8d82f88cde..5a9704ff58 100644 --- a/b2b/management/commands/b2b_courseware.py +++ b/b2b/management/commands/b2b_courseware.py @@ -7,7 +7,6 @@ import logging from argparse import RawTextHelpFormatter -from django.contrib.contenttypes.models import ContentType from django.core.management import BaseCommand, CommandError from mitol.common.utils.datetime import now_in_utc from opaque_keys import InvalidKeyError @@ -18,8 +17,12 @@ from courses.api import resolve_courseware_object_from_id from courses.constants import UAI_COURSEWARE_ID_PREFIX from courses.models import CourseRun, CourseRunEnrollment -from ecommerce.models import Discount, DiscountProduct, Product -from openedx.api import update_edx_course +from courses.retirement import ( + deactivate_run_products, + get_run_products, + push_run_dates_to_edx, +) +from ecommerce.models import Discount, DiscountProduct log = logging.getLogger(__name__) @@ -457,23 +460,13 @@ def handle_remove(self, contract, coursewares, **kwargs): # noqa: C901 courseware.save() - # Deactivate products for this run - # Use all_objects so we can find products regardless of - # their current is_active state, and evaluate to a list so - # subsequent updates don't affect the collection we use - # below when removing discount associations. - content_type = ContentType.objects.get_for_model(CourseRun) - run_products = list( - Product.all_objects.filter( - content_type=content_type, - object_id=courseware.id, - ).all() - ) - - for product in run_products: - if product.is_active: - product.is_active = False - product.save(update_fields=("is_active",)) + # Deactivate products for this run. get_run_products uses + # all_objects so it finds products regardless of their current + # is_active state, and returns a list so the deactivation below + # doesn't mutate the collection we reuse when removing discount + # associations. Shared with the retire_courserun command. + run_products = get_run_products(courseware) + deactivate_run_products(courseware) # Invalidate/delete any enrollment codes (Discounts) associated with this run's products discounts = Discount.objects.filter( @@ -497,26 +490,17 @@ def handle_remove(self, contract, coursewares, **kwargs): # noqa: C901 # Attempt to push the new enrollment_end to edX so it isn't # overwritten by the next sync from edX. + # + # NOTE: edX will not accept an enrollment window for a run that + # has no start and end date, so for a run with a null end_date + # the new enrollment_end never reaches edX and the next sync + # reverts it. push_run_dates_to_edx returns False and logs a + # warning in that case. Fixing it properly means also moving + # end_date into the past, which is what the retire_courserun + # command does; this command's contract is narrower, so the + # behaviour is left as-is here. try: - pacing_type = ( - "self_paced" if courseware.is_self_paced else "instructor_paced" - ) - - course_params = [ - courseware.courseware_id, - courseware.title, - pacing_type, - ] - - if courseware.start_date and courseware.end_date: - course_params.append(courseware.start_date) - course_params.append(courseware.end_date) - - if courseware.enrollment_start and courseware.enrollment_end: - course_params.append(courseware.enrollment_start) - course_params.append(courseware.enrollment_end) - - update_edx_course(*course_params) + push_run_dates_to_edx(courseware) except Exception: log.exception( "Failed to update enrollment end date on edX for %s", diff --git a/courses/admin.py b/courses/admin.py index e326150979..26b0a53a0a 100644 --- a/courses/admin.py +++ b/courses/admin.py @@ -468,6 +468,37 @@ def get_queryset(self, request): # noqa: ARG002 return self.model.all_objects + def formfield_for_foreignkey(self, db_field, request, **kwargs): + """ + Show inactive contracts in the b2b_contract dropdown. + + By default the admin builds this field from + ``ContractPage._default_manager``. ContractPage declares + ``active_objects`` as its only local manager, and Django orders local + managers ahead of ones inherited from the concrete parent, so + ``_default_manager`` is ``ActiveContractManager`` and filters + ``active=True``. + + That means a run attached to an inactive contract - a retired run parked + in the holding contract, or any run on an expired contract - renders with + an empty dropdown, because its current value isn't among the choices. + Since the field is ``null=True, blank=True``, saving that form is valid + and silently sets ``b2b_contract`` to NULL, which turns a B2B run into a + public-catalog run (``CourseRunQuerySet.exclude_b2b`` treats a null + contract as "not B2B"). + """ + + if db_field.name == "b2b_contract": + # Imported here to avoid a circular dependency at module load time, + # matching ProgramContractPageInline above. + from b2b.models import ContractPage # noqa: PLC0415 + + kwargs["queryset"] = ContractPage.objects.order_by( + "organization__name", "name" + ) + + return super().formfield_for_foreignkey(db_field, request, **kwargs) + @admin.display(description="Primary?", ordering="is_primary_language") def primary(self, obj): """Return the primary language run flag.""" diff --git a/courses/management/commands/retire_courserun.py b/courses/management/commands/retire_courserun.py new file mode 100644 index 0000000000..5f5a27b8ca --- /dev/null +++ b/courses/management/commands/retire_courserun.py @@ -0,0 +1,424 @@ +""" +Management command to retire (delist) a course run. + +Retiring a run makes it inert: its course and enrollment windows are pushed +into the past in edX *and* locally, it stops being live, and its products are +switched off. Existing enrollments are left alone by default so that learners +who were partway through keep their access to the material. + +By default the command runs in dry-run mode and changes nothing. A snapshot of +the run's pre-retirement state is written either way, so there is always a +record to roll back from by hand. + +**Usage:** + +1. See what would happen (no changes, not even in edX): +./manage.py retire_courserun --run=course-v1:UAI_ACME+14.100x+1T12C2026 + +2. Retire the run, moving it to the B2B holding contract. Active learners keep + their enrollments and their access; they are reported, not blocked: +./manage.py retire_courserun --run=course-v1:UAI_ACME+14.100x+1T12C2026 --commit + +3. Retire and unenroll everyone. This revokes courseware access, so it refuses + unless --allow-active-enrollments is passed too. Add --email to notify the + learners, which is off by default: +./manage.py retire_courserun --run=... --commit --unenroll --allow-active-enrollments + +4. Retire a run that has no counterpart in edX: +./manage.py retire_courserun --run=... --commit --skip-edx +""" + +import json +from pathlib import Path + +from django.core.management.base import BaseCommand, CommandError +from mitol.common.utils.datetime import now_in_utc + +from b2b.api import ( + RetirementContractCollisionError, + move_run_to_retirement_contract, +) +from courses.management.utils import bulk_unenroll_learners +from courses.models import CourseRun +from courses.retirement import ( + SourceRunRetirementError, + audit_course_run, + build_snapshot, + check_run_retirable, + retire_course_run, +) +from openedx.api import get_edx_course + + +class Command(BaseCommand): + """Retire a course run in both edX and MITx Online.""" + + help = "Retire (delist) a course run in both edX and MITx Online." + + def add_arguments(self, parser): + """Add command line arguments.""" + + parser.add_argument( + "--run", + type=str, + required=True, + help="The 'courseware_id' value for the CourseRun to retire.", + ) + parser.add_argument( + "--commit", + action="store_true", + dest="commit", + help="Actually retire the run. Without this flag, the command runs " + "in dry-run mode and makes no changes in edX or MITx Online.", + ) + parser.add_argument( + "--unenroll", + action="store_true", + dest="unenroll", + help="Unenroll the run's learners in edX and MITx Online. This " + "revokes their access to the courseware, so it is off by default.", + ) + parser.add_argument( + "--allow-active-enrollments", + action="store_true", + dest="allow_active_enrollments", + help="Required to combine --unenroll with a run that still has " + "active learners on it. Retiring on its own never needs this: it " + "leaves enrollments intact, so learners keep their access.", + ) + parser.add_argument( + "--email", + action="store_true", + dest="email", + help="Send unenrollment notification emails. Only meaningful with " + "--unenroll. Off by default so retirement is silent to learners.", + ) + parser.add_argument( + "--keep-contract", + action="store_true", + dest="keep_contract", + help="Leave the run attached to its current B2B contract instead of " + "moving it to the retirement holding contract.", + ) + parser.add_argument( + "--keep-products", + action="store_true", + dest="keep_products", + help="Leave the run's products active.", + ) + parser.add_argument( + "--skip-edx", + action="store_true", + dest="skip_edx", + help="Don't read from or write to edX. Use for runs that have no " + "edX counterpart. The next sync will not overwrite the local dates " + "only if the run genuinely isn't in edX.", + ) + parser.add_argument( + "--snapshot-dir", + type=str, + dest="snapshot_dir", + default=".", + help="Directory to write the pre-retirement snapshot to. Defaults " + "to the current directory.", + ) + parser.add_argument( + "--reason", + type=str, + default="", + help="Why the run is being retired. Recorded in the snapshot.", + ) + + def _resolve_run(self, courseware_id): + """ + Find the run and refuse the ones we shouldn't touch. + + Uses all_objects because the default manager excludes source runs, and + we need to be able to see a source run in order to reject it. + """ + + run = CourseRun.all_objects.filter(courseware_id=courseware_id).first() + + if run is None: + msg = f"Could not find course run with courseware_id={courseware_id}" + raise CommandError(msg) + + try: + check_run_retirable(run) + except SourceRunRetirementError as exc: + raise CommandError(str(exc)) from exc + + return run + + def _report(self, audit): + """Print the audit for the operator.""" + + run = audit.run + + self.stdout.write("") + self.stdout.write(f"Course run: {run.courseware_id} (id={run.id})") + self.stdout.write(f"Course: {run.course.readable_id}") + self.stdout.write(f"Title: {run.title}") + self.stdout.write( + f"Run tag: {run.run_tag} language: {run.language or '-'}" + ) + self.stdout.write(f"Live: {run.live}") + self.stdout.write(f"Start / end: {run.start_date} / {run.end_date}") + self.stdout.write( + f"Enrollment: {run.enrollment_start} / {run.enrollment_end}" + ) + self.stdout.write( + f"B2B contract: {run.b2b_contract or '- (not a contract run)'}" + ) + + self.stdout.write("") + if audit.edx_error: + self.stdout.write( + self.style.WARNING(f"edX lookup failed: {audit.edx_error}") + ) + elif audit.edx_details: + self.stdout.write("edX currently reports:") + for key, value in audit.edx_details.items(): + self.stdout.write(f" {key}: {value}") + else: + self.stdout.write("edX not consulted (--skip-edx).") + + self.stdout.write("") + self.stdout.write( + f"Enrollments: {len(audit.active_enrollments)} active, " + f"{len(audit.inactive_enrollments)} inactive" + ) + for enrollment in audit.active_enrollments: + self.stdout.write( + f" ACTIVE {enrollment.user.email} ({enrollment.enrollment_mode})" + ) + if audit.certificate_count or audit.grade_count: + self.stdout.write( + self.style.WARNING( + f" {audit.certificate_count} certificate(s) and " + f"{audit.grade_count} grade record(s) exist for this run. " + "These are kept either way." + ) + ) + + self.stdout.write("") + self.stdout.write(f"Products: {len(audit.products)}") + for entry in audit.products: + state = "active" if entry.was_active else "inactive" + self.stdout.write( + f" #{entry.product.id} {entry.product.description} " + f"({entry.product.price}, {state}), " + f"{len(entry.discounts)} discount code(s), " + f"{entry.basket_items} open basket item(s)" + ) + if entry.discounts: + self.stdout.write( + self.style.WARNING( + " Discount codes are NOT removed by this command. " + "They will stop working once the product is inactive." + ) + ) + + def _write_snapshot(self, audit, snapshot_dir, reason): + """Write the rollback snapshot and return its path.""" + + snapshot = build_snapshot(audit, reason=reason, source="Management Command") + + safe_id = audit.run.courseware_id.replace(":", "_").replace("+", "_") + timestamp = now_in_utc().strftime("%Y%m%dT%H%M%SZ") + target = Path(snapshot_dir) / f"retire_{safe_id}_{timestamp}.json" + + target.parent.mkdir(parents=True, exist_ok=True) + # default=str so an unexpected object from the edX client can never be + # the thing that aborts a retirement. The snapshot is a safety net; it + # should degrade to a string rather than raise. + target.write_text(json.dumps(snapshot, indent=2, default=str)) + + return target.resolve() + + def _handle_contract(self, run, *, keep_contract): + """Move the run to the holding contract, if it's a contract run.""" + + if keep_contract or not run.b2b_contract_id: + return + + previous = str(run.b2b_contract) + + try: + contract = move_run_to_retirement_contract(run) + except RetirementContractCollisionError as exc: + raise CommandError(str(exc)) from exc + + self.stdout.write( + self.style.SUCCESS(f" Moved from '{previous}' to '{contract}'") + ) + + def _handle_unenroll(self, run, *, email): + """Unenroll the run's active learners.""" + + entries = [ + (enrollment.user.email, run.courseware_id) + for enrollment in run.enrollments.filter(active=True).select_related("user") + ] + + if not entries: + self.stdout.write(" No active enrollments to remove.") + return + + summary = bulk_unenroll_learners( + entries, + keep_failed_enrollments=False, + send_notification=email, + ) + + for _user_id, _cw_id, status, message in summary["details"]: + if status == "succeeded": + self.stdout.write(self.style.SUCCESS(f" {message}")) + elif status == "skipped": + self.stderr.write(self.style.WARNING(f" SKIP: {message}")) + else: + self.stderr.write(self.style.ERROR(f" FAILED: {message}")) + + self.stdout.write( + f" Unenrolled {summary['succeeded']}, failed {summary['failed']}, " + f"skipped {summary['skipped']}" + ) + + def _verify(self, run, *, skip_edx): + """Re-read the run and report anything that didn't stick.""" + + run.refresh_from_db() + + problems = [] + + if run.live: + problems.append("run is still live") + if not run.end_date or run.end_date > now_in_utc(): + problems.append("end_date is not in the past") + if not run.enrollment_end or run.enrollment_end > now_in_utc(): + problems.append("enrollment_end is not in the past") + + if not skip_edx: + try: + edx_run = get_edx_course(run.courseware_id) + self.stdout.write( + f" edX now reports end={getattr(edx_run, 'end', None)}, " + f"enrollment_end={getattr(edx_run, 'enrollment_end', None)}" + ) + except Exception as exc: # noqa: BLE001 + problems.append(f"could not re-read the run from edX: {exc}") + + if problems: + self.stderr.write( + self.style.ERROR("Verification found problems: " + "; ".join(problems)) + ) + else: + self.stdout.write(self.style.SUCCESS(" Verification passed.")) + + def handle(self, *args, **options): # noqa: ARG002 + """Handle command execution.""" + + commit = options["commit"] + skip_edx = options["skip_edx"] + + run = self._resolve_run(options["run"]) + + # An active product parked in the holding contract is a hazard: the + # holding contract is 'managed' with no fixed price, so a later + # `b2b_codes validate` sweep would treat it as a free SSO contract and + # delete its discounts. Deactivating the products keeps the contract + # inert, because ContractPage.get_products() filters on is_active. + if ( + options["keep_products"] + and run.b2b_contract_id + and not options["keep_contract"] + ): + msg = ( + "--keep-products cannot be combined with moving the run to the " + "retirement holding contract. Leaving an active product on a run " + "parked in the holding contract risks a later b2b_codes sweep " + "deleting its discounts. Add --keep-contract if you really want " + "the products left on." + ) + raise CommandError(msg) + + audit = audit_course_run(run, fetch_edx=not skip_edx) + self._report(audit) + + snapshot_path = self._write_snapshot( + audit, options["snapshot_dir"], options["reason"] + ) + self.stdout.write("") + self.stdout.write(f"Snapshot written to {snapshot_path}") + + if not commit: + self.stdout.write("") + self.stdout.write( + self.style.WARNING( + "DRY RUN - nothing was changed. Re-run with --commit to retire " + "this run." + ) + ) + return + + # Retiring on its own is safe for people already on the run: no view + # filters enrollments on live/end_date/expiration_date, and edX access + # is governed purely by the enrollment record. So the refusal belongs on + # --unenroll, which is the step that actually takes access away. + if ( + options["unenroll"] + and audit.has_active_enrollments + and not options["allow_active_enrollments"] + ): + msg = ( + f"--unenroll would remove {len(audit.active_enrollments)} active " + f"enrollment(s) from {run.courseware_id}, revoking those learners' " + "access to the courseware. Re-run with --allow-active-enrollments " + "if that is what you want, or drop --unenroll to retire the run " + "and leave them enrolled." + ) + raise CommandError(msg) + + self.stdout.write("") + self.stdout.write("Retiring...") + + result = retire_course_run( + run, + deactivate_products=not options["keep_products"], + skip_edx=skip_edx, + ) + + self.stdout.write( + self.style.SUCCESS( + f" Dates set to start={result['dates']['start']}, " + f"end={result['dates']['end']}, " + f"enrollment_end={result['dates']['enrollment_end']}" + ) + ) + self.stdout.write(self.style.SUCCESS(" live set to False")) + + if not skip_edx and not result["edx_updated"]: + self.stderr.write( + self.style.ERROR( + " edX did not receive the enrollment window. The next " + "courseware sync will overwrite the local dates." + ) + ) + + for product in result["products"]: + self.stdout.write( + self.style.SUCCESS(f" Deactivated product #{product.id}") + ) + + self._handle_contract(run, keep_contract=options["keep_contract"]) + + if options["unenroll"]: + self._handle_unenroll(run, email=options["email"]) + + self.stdout.write("") + self.stdout.write("Verifying...") + self._verify(run, skip_edx=skip_edx) + + self.stdout.write("") + self.stdout.write(self.style.SUCCESS(f"Retired {run.courseware_id}.")) + self.stdout.write(f"Snapshot for rollback: {snapshot_path}") diff --git a/courses/management/tests/retire_courserun_test.py b/courses/management/tests/retire_courserun_test.py new file mode 100644 index 0000000000..3cb9193d84 --- /dev/null +++ b/courses/management/tests/retire_courserun_test.py @@ -0,0 +1,628 @@ +"""Tests for the retire_courserun management command and courses.retirement.""" + +from datetime import timedelta +from io import StringIO + +import pytest +from django.core.management import call_command +from django.core.management.base import CommandError +from mitol.common.utils.datetime import now_in_utc + +from b2b.api import ( + RetirementContractCollisionError, + get_or_create_retirement_contract, + move_run_to_retirement_contract, +) +from b2b.factories import ContractPageFactory +from courses.factories import ( + CourseRunEnrollmentFactory, + CourseRunFactory, +) +from courses.retirement import ( + SourceRunRetirementError, + check_run_retirable, + compute_retirement_dates, + deactivate_run_products, + get_run_products, + push_run_dates_to_edx, + retire_course_run, +) +from ecommerce.factories import ProductFactory +from ecommerce.models import Product + +pytestmark = pytest.mark.django_db + + +@pytest.fixture +def mock_edx(mocker): + """Stub out both edX calls the command makes.""" + + return { + "update": mocker.patch("courses.retirement.update_edx_course"), + "get": mocker.patch("courses.retirement.get_edx_course"), + "verify_get": mocker.patch( + "courses.management.commands.retire_courserun.get_edx_course" + ), + } + + +@pytest.fixture +def run(): + """A live, in-progress run with a product.""" + + course_run = CourseRunFactory.create(live=True) + ProductFactory.create(purchasable_object=course_run, is_active=True) + return course_run + + +def _run_command(course_run, tmp_path, **kwargs): + """Call the command, keeping snapshots out of the repo.""" + + out = StringIO() + err = StringIO() + call_command( + "retire_courserun", + run=course_run.courseware_id, + snapshot_dir=str(tmp_path), + stdout=out, + stderr=err, + **kwargs, + ) + return out.getvalue(), err.getvalue() + + +class TestDryRun: + """Without --commit, nothing anywhere should change.""" + + def test_dry_run_changes_nothing(self, run, tmp_path, mock_edx): + """Dry run changes nothing.""" + + original = ( + run.live, + run.start_date, + run.end_date, + run.enrollment_start, + run.enrollment_end, + ) + + out, _ = _run_command(run, tmp_path) + + run.refresh_from_db() + assert ( + run.live, + run.start_date, + run.end_date, + run.enrollment_start, + run.enrollment_end, + ) == original + assert all(p.is_active for p in get_run_products(run)) + mock_edx["update"].assert_not_called() + assert "DRY RUN" in out + + def test_dry_run_still_writes_a_snapshot(self, run, tmp_path, mock_edx): # noqa: ARG002 + """Dry run still writes a snapshot.""" + + _run_command(run, tmp_path) + + snapshots = list(tmp_path.glob("retire_*.json")) + assert len(snapshots) == 1 + + def test_dry_run_reports_enrollments_and_products(self, run, tmp_path, mock_edx): # noqa: ARG002 + """Dry run reports enrollments and products.""" + + enrollment = CourseRunEnrollmentFactory.create(run=run, active=True) + + out, _ = _run_command(run, tmp_path) + + assert enrollment.user.email in out + assert "1 active" in out + assert "Products: 1" in out + + +class TestGuards: + """The command should refuse the dangerous cases.""" + + def test_unknown_run(self, tmp_path): + """Unknown run.""" + + with pytest.raises(CommandError, match="Could not find course run"): + call_command( + "retire_courserun", + run="course-v1:nope+nope+nope", + snapshot_dir=str(tmp_path), + ) + + def test_source_run_refused(self, tmp_path, mock_edx): # noqa: ARG002 + """Source run refused.""" + + source = CourseRunFactory.create(is_source_run=True) + + with pytest.raises(CommandError, match="is a source run"): + _run_command(source, tmp_path, commit=True) + + def test_source_run_refused_in_dry_run_too(self, tmp_path, mock_edx): # noqa: ARG002 + """Source run refused in dry run too.""" + + source = CourseRunFactory.create(is_source_run=True) + + with pytest.raises(CommandError, match="is a source run"): + _run_command(source, tmp_path) + + def test_source_run_by_run_tag_refused(self): + """Source run by run tag refused.""" + + source = CourseRunFactory.create(is_source_run=False, run_tag="SOURCE") + + with pytest.raises(SourceRunRetirementError): + check_run_retirable(source) + + def test_active_enrollments_do_not_block_retirement(self, run, tmp_path, mock_edx): # noqa: ARG002 + """Retiring never blocks on enrollments; it leaves them intact.""" + + enrollment = CourseRunEnrollmentFactory.create(run=run, active=True) + + _run_command(run, tmp_path, commit=True) + + run.refresh_from_db() + enrollment.refresh_from_db() + assert run.live is False + # No view filters enrollments on live/end_date/expiration_date, and edX + # access is enrollment-based, so this learner keeps working access. + assert enrollment.active is True + + def test_unenroll_blocked_by_active_enrollments(self, run, tmp_path, mock_edx): + """--unenroll refuses without the override, changing nothing.""" + + CourseRunEnrollmentFactory.create(run=run, active=True) + + with pytest.raises(CommandError, match="--unenroll would remove"): + _run_command(run, tmp_path, commit=True, unenroll=True) + + run.refresh_from_db() + assert run.live is True + mock_edx["update"].assert_not_called() + + def test_unenroll_allowed_with_override(self, run, tmp_path, mock_edx, mocker): # noqa: ARG002 + """--allow-active-enrollments unblocks --unenroll.""" + + mock_bulk = mocker.patch( + "courses.management.commands.retire_courserun.bulk_unenroll_learners", + return_value={"succeeded": 1, "failed": 0, "skipped": 0, "details": []}, + ) + CourseRunEnrollmentFactory.create(run=run, active=True) + + _run_command( + run, tmp_path, commit=True, unenroll=True, allow_active_enrollments=True + ) + + run.refresh_from_db() + assert run.live is False + mock_bulk.assert_called_once() + + def test_unenroll_with_no_active_enrollments_needs_no_override( + self, + run, + tmp_path, + mock_edx, # noqa: ARG002 + ): + """--unenroll on a run nobody is on doesn't need the override.""" + + CourseRunEnrollmentFactory.create(run=run, active=False) + + _run_command(run, tmp_path, commit=True, unenroll=True) + + run.refresh_from_db() + assert run.live is False + + def test_inactive_enrollments_do_not_block(self, run, tmp_path, mock_edx): # noqa: ARG002 + """Inactive enrollments do not block.""" + + CourseRunEnrollmentFactory.create(run=run, active=False) + + _run_command(run, tmp_path, commit=True) + + run.refresh_from_db() + assert run.live is False + + +class TestCommit: + """The committed path.""" + + def test_dates_and_live(self, run, tmp_path, mock_edx): # noqa: ARG002 + """Committing pushes the windows into the past and unsets live.""" + + _run_command(run, tmp_path, commit=True) + + run.refresh_from_db() + now = now_in_utc() + assert run.live is False + assert run.end_date < now + assert run.enrollment_end < now + assert run.start_date < run.end_date + assert run.enrollment_start <= run.start_date + + def test_future_expiration_date_untouched(self, run, tmp_path, mock_edx): # noqa: ARG002 + """A future expiration_date is left alone.""" + + original = run.expiration_date + assert original > now_in_utc() + + _run_command(run, tmp_path, commit=True) + + run.refresh_from_db() + # Moving expiration_date would hide the run from the dashboards of + # learners we deliberately left enrolled. + assert run.expiration_date == original + + def test_past_expiration_date_is_cleared(self, run, tmp_path, mock_edx): # noqa: ARG002 + """A past expiration_date is cleared instead of tripping clean().""" + + # CourseRun.save() calls clean(), which rejects an expiration_date + # earlier than start/end. Without clearing it, retiring an already + # finished run raises ValidationError *after* the edX write landed. + # + # The setup itself has to satisfy clean(), so the whole run is pushed + # well into the past: start < end < expiration, all historic. Retiring + # then moves end to yesterday, which lands after expiration and is what + # forces the reset. + now = now_in_utc() + run.start_date = now - timedelta(days=200) + run.end_date = now - timedelta(days=120) + run.expiration_date = now - timedelta(days=90) + run.save() + + _run_command(run, tmp_path, commit=True) + + run.refresh_from_db() + assert run.expiration_date is None + assert run.live is False + + def test_products_deactivated(self, run, tmp_path, mock_edx): # noqa: ARG002 + """Products deactivated.""" + + _run_command(run, tmp_path, commit=True) + + assert all(not p.is_active for p in get_run_products(run)) + + def test_keep_products(self, run, tmp_path, mock_edx): # noqa: ARG002 + """Keep products.""" + + _run_command(run, tmp_path, commit=True, keep_products=True) + + assert all(p.is_active for p in get_run_products(run)) + + def test_edx_gets_a_complete_date_set(self, run, tmp_path, mock_edx): + """All four dates must reach edX or the sync will revert us.""" + + _run_command(run, tmp_path, commit=True) + + mock_edx["update"].assert_called_once() + _args, kwargs = mock_edx["update"].call_args + for key in ("start", "end", "enrollment_start", "enrollment_end"): + assert kwargs[key] is not None + assert kwargs["end"] < now_in_utc() + + def test_edx_failure_leaves_local_state_alone(self, run, tmp_path, mock_edx): + """A failed edX write must abort before anything local changes.""" + + mock_edx["update"].side_effect = ValueError("edX exploded") + + with pytest.raises(ValueError, match="edX exploded"): + _run_command(run, tmp_path, commit=True) + + run.refresh_from_db() + assert run.live is True + assert all(p.is_active for p in get_run_products(run)) + + def test_skip_edx(self, run, tmp_path, mock_edx): + """--skip-edx bypasses edX entirely.""" + + _run_command(run, tmp_path, commit=True, skip_edx=True) + + mock_edx["update"].assert_not_called() + run.refresh_from_db() + assert run.live is False + + def test_unenroll(self, run, tmp_path, mock_edx, mocker): # noqa: ARG002 + """--unenroll defaults to sending no email.""" + + mock_bulk = mocker.patch( + "courses.management.commands.retire_courserun.bulk_unenroll_learners", + return_value={"succeeded": 1, "failed": 0, "skipped": 0, "details": []}, + ) + enrollment = CourseRunEnrollmentFactory.create(run=run, active=True) + + _run_command( + run, + tmp_path, + commit=True, + allow_active_enrollments=True, + unenroll=True, + ) + + mock_bulk.assert_called_once_with( + [(enrollment.user.email, run.courseware_id)], + keep_failed_enrollments=False, + send_notification=False, + ) + + def test_unenroll_with_email(self, run, tmp_path, mock_edx, mocker): # noqa: ARG002 + """--email opts into notifying learners.""" + + mock_bulk = mocker.patch( + "courses.management.commands.retire_courserun.bulk_unenroll_learners", + return_value={"succeeded": 1, "failed": 0, "skipped": 0, "details": []}, + ) + CourseRunEnrollmentFactory.create(run=run, active=True) + + _run_command( + run, + tmp_path, + commit=True, + allow_active_enrollments=True, + unenroll=True, + email=True, + ) + + assert mock_bulk.call_args.kwargs["send_notification"] is True + + +class TestContractHandling: + """Retired B2B runs get parked, not orphaned.""" + + def test_moved_to_holding_contract(self, tmp_path, mock_edx): # noqa: ARG002 + """A retired B2B run is parked, never orphaned.""" + + contract = ContractPageFactory.create() + course_run = CourseRunFactory.create(live=True, b2b_contract=contract) + + _run_command(course_run, tmp_path, commit=True) + + course_run.refresh_from_db() + holding = get_or_create_retirement_contract() + assert course_run.b2b_contract_id == holding.id + # Never nulled - a null contract FK would make this a public-catalog run. + assert course_run.b2b_contract_id is not None + assert holding.active is False + assert holding.live is False + + def test_holding_contract_is_reused(self, tmp_path, mock_edx): # noqa: ARG002 + """The holding contract is created once and reused.""" + + contract = ContractPageFactory.create() + first = CourseRunFactory.create(live=True, b2b_contract=contract) + second = CourseRunFactory.create(live=True, b2b_contract=contract) + + _run_command(first, tmp_path, commit=True) + _run_command(second, tmp_path, commit=True) + + first.refresh_from_db() + second.refresh_from_db() + assert first.b2b_contract_id == second.b2b_contract_id + + def test_keep_contract(self, tmp_path, mock_edx): # noqa: ARG002 + """Keep contract.""" + + contract = ContractPageFactory.create() + course_run = CourseRunFactory.create(live=True, b2b_contract=contract) + + _run_command(course_run, tmp_path, commit=True, keep_contract=True) + + course_run.refresh_from_db() + assert course_run.b2b_contract_id == contract.id + + def test_non_b2b_run_needs_no_contract(self, run, tmp_path, mock_edx): # noqa: ARG002 + """A non-B2B run retires fine with no contract step.""" + + _run_command(run, tmp_path, commit=True) + + run.refresh_from_db() + assert run.b2b_contract_id is None + assert run.live is False + + def test_keep_products_refused_when_parking_the_run(self, tmp_path, mock_edx): # noqa: ARG002 + """--keep-products can't be combined with the contract move.""" + + contract = ContractPageFactory.create() + course_run = CourseRunFactory.create(live=True, b2b_contract=contract) + ProductFactory.create(purchasable_object=course_run, is_active=True) + + with pytest.raises(CommandError, match="--keep-products cannot be combined"): + _run_command(course_run, tmp_path, commit=True, keep_products=True) + + course_run.refresh_from_db() + assert course_run.b2b_contract_id == contract.id + assert course_run.live is True + + def test_keep_products_allowed_with_keep_contract(self, tmp_path, mock_edx): # noqa: ARG002 + """--keep-contract makes --keep-products safe again.""" + + contract = ContractPageFactory.create() + course_run = CourseRunFactory.create(live=True, b2b_contract=contract) + ProductFactory.create(purchasable_object=course_run, is_active=True) + + _run_command( + course_run, + tmp_path, + commit=True, + keep_products=True, + keep_contract=True, + ) + + course_run.refresh_from_db() + assert course_run.live is False + assert course_run.b2b_contract_id == contract.id + assert all(p.is_active for p in get_run_products(course_run)) + + +class TestCollisionCheck: + """The holding contract can't be allowed to violate a unique constraint.""" + + @pytest.fixture + def source_contract(self): + """ + A normal contract, created before anything asks for the holding one. + + ContractPageFactory bootstraps HomePage -> OrganizationIndexPage -> + OrganizationPage. get_or_create_retirement_contract() falls back to + ensure_b2b_organization_index(), which calls cms.api.get_home_page() and + raises Page.DoesNotExist when no Wagtail tree exists yet, so the ordering + matters. + """ + + return ContractPageFactory.create() + + def test_language_collision_refused(self, source_contract, mock_edx): # noqa: ARG002 + """A parked run with the same course/tag/language/variant blocks the move.""" + + holding = get_or_create_retirement_contract() + parked = CourseRunFactory.create( + b2b_contract=holding, language="de_DE", run_tag="1T9C2026" + ) + incoming = CourseRunFactory.create( + course=parked.course, + b2b_contract=source_contract, + language="de_DE", + run_tag="1T9C2026", + ) + + with pytest.raises(RetirementContractCollisionError, match="already parked"): + move_run_to_retirement_contract(incoming) + + def test_primary_language_collision_refused(self, source_contract, mock_edx): # noqa: ARG002 + """A parked primary-language run blocks another for the same group.""" + + holding = get_or_create_retirement_contract() + parked = CourseRunFactory.create( + b2b_contract=holding, + language="", + is_primary_language=True, + run_tag="1T9C2026", + ) + incoming = CourseRunFactory.create( + course=parked.course, + b2b_contract=source_contract, + language="", + is_primary_language=True, + run_tag="1T9C2026", + ) + + with pytest.raises(RetirementContractCollisionError, match="primary-language"): + move_run_to_retirement_contract(incoming) + + def test_distinct_run_tags_do_not_collide(self, source_contract, mock_edx): # noqa: ARG002 + """Different run tags park side by side, which is the normal case.""" + + holding = get_or_create_retirement_contract() + parked = CourseRunFactory.create( + b2b_contract=holding, language="de_DE", run_tag="1T9C2026" + ) + incoming = CourseRunFactory.create( + course=parked.course, + b2b_contract=source_contract, + language="de_DE", + run_tag="1T9C2027", + ) + + assert move_run_to_retirement_contract(incoming).id == holding.id + + def test_already_parked_run_is_a_no_op(self, source_contract, mock_edx): # noqa: ARG002 + """Re-parking a run that's already in the holding contract is fine.""" + + holding = get_or_create_retirement_contract() + parked = CourseRunFactory.create(b2b_contract=holding, run_tag="1T9C2026") + + assert move_run_to_retirement_contract(parked).id == holding.id + + +class TestRetirementHelpers: + """Unit coverage for the shared util.""" + + def test_compute_dates_puts_everything_in_the_past(self): + """Every computed date lands in the past, in a valid order.""" + + course_run = CourseRunFactory.build( + start_date=None, end_date=None, enrollment_start=None, enrollment_end=None + ) + + dates = compute_retirement_dates(course_run) + + now = now_in_utc() + assert dates["end"] < now + assert dates["enrollment_end"] < now + assert dates["start"] < dates["end"] + assert dates["enrollment_start"] <= dates["start"] + + def test_compute_dates_preserves_an_early_start(self): + """An already-past start date is left alone.""" + + # Pinned rather than left to the factory: its upper bound is now-1d, + # which is exactly the cutoff compute_retirement_dates compares against. + original_start = now_in_utc() - timedelta(days=10) + course_run = CourseRunFactory.build(start_date=original_start) + + dates = compute_retirement_dates(course_run) + + assert dates["start"] == original_start + + def test_push_dates_reports_incomplete_sets(self, run, mock_edx): + """A run with no end_date can't have its enrollment window set in edX.""" + + run.end_date = None + run.save() + + assert push_run_dates_to_edx(run) is False + + kwargs = mock_edx["update"].call_args.kwargs + assert "enrollment_end" not in kwargs + + def test_push_dates_sends_a_complete_set(self, run, mock_edx): + """A complete date set reaches edX intact.""" + + dates = compute_retirement_dates(run) + + assert push_run_dates_to_edx(run, dates) is True + + kwargs = mock_edx["update"].call_args.kwargs + assert kwargs["enrollment_end"] == dates["enrollment_end"] + + def test_get_run_products_sees_inactive_products(self, run): + """get_run_products must see products the default manager hides.""" + + product = get_run_products(run)[0] + product.is_active = False + product.save() + + assert len(get_run_products(run)) == 1 + assert not Product.objects.filter(id=product.id).exists() + + def test_deactivate_run_products_is_idempotent(self, run): + """Deactivate run products is idempotent.""" + + assert len(deactivate_run_products(run)) == 1 + assert deactivate_run_products(run) == [] + + def test_retire_leaves_enrollments_alone(self, run, mock_edx): # noqa: ARG002 + """Retire leaves enrollments alone.""" + + enrollment = CourseRunEnrollmentFactory.create(run=run, active=True) + + retire_course_run(run) + + enrollment.refresh_from_db() + assert enrollment.active is True + assert enrollment.change_status is None + + def test_retire_returns_what_it_changed(self, run, mock_edx): # noqa: ARG002 + """Retire returns what it changed.""" + + result = retire_course_run(run) + + assert result["edx_updated"] is True + assert len(result["products"]) == 1 + assert set(result["dates"]) == { + "start", + "end", + "enrollment_start", + "enrollment_end", + } diff --git a/courses/retirement.py b/courses/retirement.py new file mode 100644 index 0000000000..d42c832ddd --- /dev/null +++ b/courses/retirement.py @@ -0,0 +1,481 @@ +""" +Shared logic for retiring (delisting) course runs. + +A "retired" run is one that has been made inert: it is no longer live, its +enrollment window and course window are in the past, and its products are +switched off. Existing enrollments are deliberately left alone by default so +that learners who were partway through the material keep their access. + +The date changes are pushed to edX as well as written locally, because +``courses.api.sync_course_runs`` overwrites ``start_date``, ``end_date``, +``enrollment_start``, ``enrollment_end``, ``title``, ``is_self_paced`` and +``certificate_available_date`` from edX on every sync. ``live`` is the only +one of these fields that is not synced, which makes it the durable local lever. + +This module is intentionally free of any B2B imports so that it can be used for +non-B2B runs. The B2B holding-contract behaviour lives in ``b2b.api``. +""" + +import logging +from dataclasses import dataclass, field +from datetime import datetime, timedelta + +from django.contrib.contenttypes.models import ContentType +from mitol.common.utils.datetime import now_in_utc + +from courses.models import ( + CourseRun, + CourseRunCertificate, + CourseRunEnrollment, + CourseRunGrade, +) +from ecommerce.models import BasketItem, Discount, Product +from openedx.api import get_edx_course, update_edx_course + +log = logging.getLogger(__name__) + +# How far into the past to push the retired dates. A day is enough to put the +# run unambiguously in the past without looking like a data-entry error. +RETIREMENT_DATE_OFFSET = timedelta(days=1) + + +class SourceRunRetirementError(Exception): + """Raised when the run being retired is a source run.""" + + +@dataclass +class ProductAudit: + """What we know about a product attached to the run.""" + + product: Product + was_active: bool + discounts: list = field(default_factory=list) + basket_items: int = 0 + + +@dataclass +class RunAudit: + """Everything the operator should see before a run is retired.""" + + run: CourseRun + active_enrollments: list = field(default_factory=list) + inactive_enrollments: list = field(default_factory=list) + products: list = field(default_factory=list) + certificate_count: int = 0 + grade_count: int = 0 + edx_details: dict | None = None + edx_error: str | None = None + + @property + def has_active_enrollments(self): + """Whether anybody is currently enrolled.""" + + return len(self.active_enrollments) > 0 + + +def get_run_products(run: CourseRun) -> list[Product]: + """ + Return every product attached to the run, active or not. + + ``Product.objects`` is an ``ActiveUndeleteManager`` and filters out + ``is_active=False``, so already-deactivated products are invisible through + it. We want to see them, so this uses ``all_objects``. + + Args: + run (CourseRun): the run to inspect. + Returns: + list of Product: the run's products, evaluated so that later updates + don't mutate the collection out from under the caller. + """ + + return list( + Product.all_objects.filter( + content_type=ContentType.objects.get_for_model(CourseRun), + object_id=run.id, + ).all() + ) + + +def deactivate_run_products(run: CourseRun) -> list[Product]: + """ + Switch off every active product attached to the run. + + Uses ``save()`` rather than a queryset ``update()`` so that the + ``ecommerce.signals.sync_product`` post-save receiver still fires and + HubSpot stays consistent. That means one HubSpot task per product. + + Args: + run (CourseRun): the run whose products should be deactivated. + Returns: + list of Product: the products that were actually changed. + """ + + deactivated = [] + + for product in get_run_products(run): + if product.is_active: + product.is_active = False + product.save(update_fields=("is_active",)) + deactivated.append(product) + + return deactivated + + +def compute_retirement_dates(run: CourseRun, *, now: datetime | None = None) -> dict: + """ + Work out the set of dates that put the run in the past. + + ``end_date`` and ``enrollment_end`` are the fields that actually delist a + run: ``end_date`` drives ``CourseRun.is_past`` and + ``CourseRunQuerySet.available()``, and ``enrollment_end`` drives + ``CourseRun.is_enrollable``. The start dates are only moved if they aren't + already early enough, because edX refuses to set enrollment dates unless + the run has both a start and an end date, so we must always send a + coherent set of four. + + Args: + run (CourseRun): the run being retired. + Keyword Args: + now (datetime|None): override for the current time, for tests. + Returns: + dict: keys ``start``, ``end``, ``enrollment_start``, ``enrollment_end``. + """ + + now = now or now_in_utc() + cutoff = now - RETIREMENT_DATE_OFFSET + + start = ( + run.start_date + if run.start_date and run.start_date < cutoff + else cutoff - RETIREMENT_DATE_OFFSET + ) + enrollment_start = ( + run.enrollment_start + if run.enrollment_start and run.enrollment_start < start + else start + ) + + return { + "start": start, + "end": cutoff, + "enrollment_start": enrollment_start, + "enrollment_end": cutoff, + } + + +def push_run_dates_to_edx( + run: CourseRun, dates: dict | None = None, *, client=None +) -> bool: + """ + Push a run's schedule to edX. + + edX will only accept enrollment dates for a run that has both a start and + an end date, so if any of the four dates is missing this sends the title + and pacing only and returns False. Callers that need the dates to stick + should pass a complete set (see ``compute_retirement_dates``). + + Any error from the edX client propagates; it is up to the caller to decide + whether that should abort the operation. + + Args: + run (CourseRun): the run to update in edX. + dates (dict|None): the dates to send. Defaults to the run's current + local values. + Keyword Args: + client (EdxApi|None): edX client, if you want to reuse one. + Returns: + bool: True if the dates were included in the payload, False if only + the title and pacing were sent. + """ + + if dates is None: + dates = { + "start": run.start_date, + "end": run.end_date, + "enrollment_start": run.enrollment_start, + "enrollment_end": run.enrollment_end, + } + + complete = all(dates.get(key) for key in ("start", "end")) + enrollment_complete = complete and all( + dates.get(key) for key in ("enrollment_start", "enrollment_end") + ) + + payload = { + "title": run.title, + "pacing_type": "self_paced" if run.is_self_paced else "instructor_paced", + } + + if complete: + payload["start"] = dates["start"] + payload["end"] = dates["end"] + + if enrollment_complete: + payload["enrollment_start"] = dates["enrollment_start"] + payload["enrollment_end"] = dates["enrollment_end"] + + if not enrollment_complete: + log.warning( + "push_run_dates_to_edx: %s has an incomplete date set %s, so edX will " + "not accept the enrollment window and the next sync will overwrite " + "the local values", + run.courseware_id, + dates, + ) + + update_edx_course(run.courseware_id, client=client, **payload) + + return enrollment_complete + + +def audit_course_run( + run: CourseRun, *, fetch_edx: bool = True, client=None +) -> RunAudit: + """ + Collect everything an operator needs to see before retiring a run. + + Makes no changes. Safe to call in dry-run mode. + + Args: + run (CourseRun): the run to inspect. + Keyword Args: + fetch_edx (bool): whether to read the run's current state from edX. + client (EdxApi|None): edX client, if you want to reuse one. + Returns: + RunAudit + """ + + audit = RunAudit(run=run) + + enrollments = ( + CourseRunEnrollment.all_objects.filter(run=run) + .select_related("user") + .order_by("user__email") + ) + + for enrollment in enrollments: + if enrollment.active: + audit.active_enrollments.append(enrollment) + else: + audit.inactive_enrollments.append(enrollment) + + # all_objects, because CourseRunCertificate.objects hides revoked + # certificates and ones with a future issue date. Under-reporting here would + # be exactly the wrong direction for a warning. + audit.certificate_count = CourseRunCertificate.all_objects.filter( + course_run=run + ).count() + audit.grade_count = CourseRunGrade.objects.filter(course_run=run).count() + + for product in get_run_products(run): + audit.products.append( + ProductAudit( + product=product, + was_active=product.is_active, + discounts=list( + Discount.objects.filter(products__product=product).distinct().all() + ), + basket_items=BasketItem.objects.filter(product=product).count(), + ) + ) + + if fetch_edx: + try: + edx_run = get_edx_course(run.courseware_id, client=client) + + def _edx_value(attr, edx_run=edx_run): + """Stringify an edX attribute, keeping None as None.""" + value = getattr(edx_run, attr, None) + return None if value is None else str(value) + + audit.edx_details = { + attr: _edx_value(attr) + for attr in ( + "title", + "start", + "end", + "enrollment_start", + "enrollment_end", + ) + } + except Exception as exc: # noqa: BLE001 + audit.edx_error = str(exc) + + return audit + + +def check_run_retirable(run: CourseRun) -> None: + """ + Refuse to retire runs that other machinery depends on. + + Source runs are the templates ``b2b.api.create_contract_run`` clones from, + so retiring one quietly breaks every future contract run for the course. + There is deliberately no override flag. + + Args: + run (CourseRun): the run to check. + Raises: + SourceRunRetirementError: if the run is a source run. + """ + + if run.is_source_run or run.run_tag == "SOURCE": + msg = ( + f"{run.courseware_id} is a source run. Retiring it would break future " + "contract runs for this course. Retire the contract runs instead." + ) + raise SourceRunRetirementError(msg) + + +def build_snapshot(audit: RunAudit, *, reason: str = "", source: str = "") -> dict: + """ + Build a JSON-serialisable record of the run's pre-retirement state. + + This is the rollback source of truth. There is no automated rollback; the + snapshot exists so that a human can put things back by hand. + + Args: + audit (RunAudit): the audit to serialise. + Keyword Args: + reason (str): why the run is being retired. + source (str): what produced the snapshot, e.g. "Management Command". + Returns: + dict + """ + + run = audit.run + + def _dt(value): + return value.isoformat() if value else None + + return { + "snapshot_taken": now_in_utc().isoformat(), + "reason": reason, + "source": source, + "run": { + "id": run.id, + "courseware_id": run.courseware_id, + "course": run.course.readable_id, + "run_tag": run.run_tag, + "title": run.title, + "live": run.live, + "is_self_paced": run.is_self_paced, + "is_source_run": run.is_source_run, + "language": run.language, + "start_date": _dt(run.start_date), + "end_date": _dt(run.end_date), + "enrollment_start": _dt(run.enrollment_start), + "enrollment_end": _dt(run.enrollment_end), + "expiration_date": _dt(run.expiration_date), + "upgrade_deadline": _dt(run.upgrade_deadline), + "b2b_contract_id": run.b2b_contract_id, + }, + "edx": audit.edx_details, + "edx_error": audit.edx_error, + "products": [ + { + "id": entry.product.id, + "description": entry.product.description, + "price": str(entry.product.price), + "is_active": entry.was_active, + "basket_items": entry.basket_items, + "discounts": [ + {"id": discount.id, "code": discount.discount_code} + for discount in entry.discounts + ], + } + for entry in audit.products + ], + "enrollments": { + "active": [ + {"id": e.id, "user": e.user.email, "mode": e.enrollment_mode} + for e in audit.active_enrollments + ], + "inactive": [ + { + "id": e.id, + "user": e.user.email, + "mode": e.enrollment_mode, + "change_status": e.change_status, + } + for e in audit.inactive_enrollments + ], + }, + "certificate_count": audit.certificate_count, + "grade_count": audit.grade_count, + } + + +def retire_course_run( + run: CourseRun, + *, + deactivate_products: bool = True, + skip_edx: bool = False, + edx_client=None, + now: datetime | None = None, +) -> dict: + """ + Retire a course run: past dates in edX and locally, not live, products off. + + edX is written first, deliberately. edX is the effective source of truth + for the date fields, so if the edX call fails after we've written locally + the next ``sync_course_runs`` pass would silently revert us. Writing edX + first means a failure aborts with nothing changed, and a local failure + after a successful edX write is self-healing on the next sync. + + Enrollments are not touched. Unenrolling is a separate, explicit step + (see ``courses.management.utils.bulk_unenroll_learners``) because it + revokes courseware access and emails learners. + + Args: + run (CourseRun): the run to retire. + Keyword Args: + deactivate_products (bool): switch off the run's products. + skip_edx (bool): don't call edX at all, for runs with no edX counterpart. + edx_client (EdxApi|None): edX client, if you want to reuse one. + now (datetime|None): override for the current time, for tests. + Returns: + dict: ``dates`` applied, ``products`` deactivated, and ``edx_updated``. + """ + + dates = compute_retirement_dates(run, now=now) + edx_updated = False + + if not skip_edx: + edx_updated = push_run_dates_to_edx(run, dates, client=edx_client) + + # CourseRun.save() runs clean(), which rejects an expiration_date earlier + # than the start or end date. Pushing the run into the past would trip that + # for any run whose expiration_date has already passed - which is the most + # likely thing to be retiring. Clear it so it goes back to being derived, + # exactly as sync_course_runs does when the dates change under it. + if run.expiration_date and ( + run.expiration_date < dates["end"] or run.expiration_date < dates["start"] + ): + log.info( + "Clearing expiration_date %s on %s; it predates the retired end date", + run.expiration_date, + run.courseware_id, + ) + run.expiration_date = None + + run.start_date = dates["start"] + run.end_date = dates["end"] + run.enrollment_start = dates["enrollment_start"] + run.enrollment_end = dates["enrollment_end"] + run.live = False + run.save() + + products = deactivate_run_products(run) if deactivate_products else [] + + log.info( + "Retired course run %s (edx_updated=%s, products_deactivated=%s)", + run.courseware_id, + edx_updated, + len(products), + ) + + return { + "dates": dates, + "products": products, + "edx_updated": edx_updated, + } From f1046738a5e7eca82ff03bfd5ace999d02e6aaed Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Fri, 31 Jul 2026 09:13:07 -0400 Subject: [PATCH 2/8] fix: read Learn's Fastly service ID from MIT_LEARN_FASTLY_SERVICE_ID (#3794) Co-authored-by: Claude Opus 5 (1M context) --- app.json | 20 ++++--- cms/signals_test.py | 62 ++++++++++++++++++++++ cms/tasks.py | 49 +++++++++-------- cms/tasks_test.py | 124 ++++++++++++++++++++++++++++++++++++++++++++ main/settings.py | 28 +++++++--- 5 files changed, 247 insertions(+), 36 deletions(-) create mode 100644 cms/signals_test.py create mode 100644 cms/tasks_test.py diff --git a/app.json b/app.json index 462723dc4c..0f79ecc68f 100644 --- a/app.json +++ b/app.json @@ -138,14 +138,6 @@ "description": "Expose the OIDC login functionality.", "required": false }, - "FASTLY_AUTH_TOKEN": { - "description": "Optional token for the Fastly purge API.", - "required": false - }, - "FASTLY_URL": { - "description": "The URL to the Fastly API.", - "required": false - }, "GA_TRACKING_ID": { "description": "Google analytics tracking ID", "required": false @@ -482,6 +474,14 @@ "description": "The execution environment that the app is in (e.g. dev, staging, prod)", "required": true }, + "MITX_ONLINE_FASTLY_AUTH_TOKEN": { + "description": "Optional token for the Fastly purge API.", + "required": false + }, + "MITX_ONLINE_FASTLY_URL": { + "description": "The URL to the Fastly API.", + "required": false + }, "MITX_ONLINE_FROM_EMAIL": { "description": "E-mail to use for the from field", "required": false @@ -546,6 +546,10 @@ "description": "Dashboard URL for UAI enrollment emails", "required": false }, + "MIT_LEARN_FASTLY_SERVICE_ID": { + "description": "Fastly service ID for the MIT Learn frontend, used for surrogate key (tag) purging.", + "required": false + }, "MIT_LEARN_FROM_EMAIL": { "description": "From email address for UAI enrollment emails", "required": false diff --git a/cms/signals_test.py b/cms/signals_test.py new file mode 100644 index 0000000000..37684889a0 --- /dev/null +++ b/cms/signals_test.py @@ -0,0 +1,62 @@ +"""Tests for cms.signals""" + +from unittest.mock import patch + +import factory +import pytest +from django.db.models.signals import post_save + +from cms.factories import CoursePageFactory, ProgramPageFactory, ResourcePageFactory + +pytestmark = pytest.mark.django_db + + +@patch("cms.signals.transaction.on_commit", side_effect=lambda callback: callback()) +@patch("cms.tasks.queue_fastly_surrogate_key_purge.delay") +def test_purge_fastly_cache_on_publish_course_page(mock_purge_delay, mock_on_commit): + """Publishing a CoursePage purges the key for its course.""" + course_page = CoursePageFactory.create() + mock_purge_delay.reset_mock() + + # Publishing saves the Course as well, which purges the same key via its own + # post_save receiver; mute post_save so only the publish path is counted. + # This mutes every post_save receiver, Wagtail's index updates included, so + # the publish is not a fully faithful one -- adequate for these assertions. + with factory.django.mute_signals(post_save): + course_page.save_revision().publish() + + mock_purge_delay.assert_called_once_with( + f"mitxonline:course:{course_page.course.readable_id}" + ) + + +@patch("cms.signals.transaction.on_commit", side_effect=lambda callback: callback()) +@patch("cms.tasks.queue_fastly_surrogate_key_purge.delay") +def test_purge_fastly_cache_on_publish_program_page(mock_purge_delay, mock_on_commit): + """Publishing a ProgramPage purges the key for its program.""" + program_page = ProgramPageFactory.create() + mock_purge_delay.reset_mock() + + with factory.django.mute_signals(post_save): + program_page.save_revision().publish() + + mock_purge_delay.assert_called_once_with( + f"mitxonline:program:{program_page.program.readable_id}" + ) + + +@patch("cms.signals.transaction.on_commit", side_effect=lambda callback: callback()) +@patch("cms.tasks.queue_fastly_surrogate_key_purge.delay") +def test_purge_fastly_cache_on_publish_ignores_other_pages( + mock_purge_delay, mock_on_commit +): + """Publishing a page that is not a product page purges nothing.""" + resource_page = ResourcePageFactory.create() + mock_purge_delay.reset_mock() + + # A ResourcePage has no Course or Program, so there is no post_save purge to + # suppress here; muted only to keep the three tests the same shape. + with factory.django.mute_signals(post_save): + resource_page.save_revision().publish() + + mock_purge_delay.assert_not_called() diff --git a/cms/tasks.py b/cms/tasks.py index cf23ce9104..6fbbf14c8b 100644 --- a/cms/tasks.py +++ b/cms/tasks.py @@ -2,17 +2,12 @@ from urllib.parse import urljoin, urlparse import requests +from django.conf import settings from mitol.common.decorators import single_task from cms.api import create_featured_items from cms.models import Page from main.celery import app -from main.settings import ( - MITX_ONLINE_FASTLY_AUTH_TOKEN, - MITX_ONLINE_FASTLY_SERVICE_ID, - MITX_ONLINE_FASTLY_URL, - SITE_BASE_URL, -) def call_fastly_purge_api(relative_url): @@ -21,23 +16,26 @@ def call_fastly_purge_api(relative_url): because it doesn't work for this - the version of it that works with the current API only allows you to purge *everything*, not individual pages.) + Purges by URL against MITxOnline's own site -- the target is identified by + the `host` header taken from SITE_BASE_URL, not by a Fastly service ID. + Args: - relative_url The relative URL to purge. Returns: - Dict of the response (resp.json), or False if there was an error. """ logger = logging.getLogger("fastly_purge") - netloc = urlparse(SITE_BASE_URL)[1] + netloc = urlparse(settings.SITE_BASE_URL)[1] headers = {"host": netloc} if relative_url != "*": headers["fastly-soft-purge"] = "1" - if MITX_ONLINE_FASTLY_AUTH_TOKEN: - headers["fastly-key"] = MITX_ONLINE_FASTLY_AUTH_TOKEN + if settings.FASTLY_AUTH_TOKEN: + headers["fastly-key"] = settings.FASTLY_AUTH_TOKEN - api_url = urljoin(MITX_ONLINE_FASTLY_URL, relative_url) + api_url = urljoin(settings.FASTLY_URL, relative_url) resp = requests.request("PURGE", api_url, headers=headers) # noqa: S113 @@ -98,47 +96,56 @@ def queue_fastly_full_purge(): @app.task -def queue_fastly_surrogate_key_purge(surrogate_key): +def queue_fastly_surrogate_key_purge(surrogate_key, service_id=None): """ Purges all Fastly cached responses tagged with the given surrogate key. Uses the Fastly purge-by-tag API: POST /service/{service_id}/purge/{surrogate_key} - This allows MIT Learn pages to declare which MITxOnline surrogate keys they - subscribe to (via the Surrogate-Key response header), and MITxOnline to - invalidate those pages when course/program data changes. + MIT Learn tags its product page responses with the MITxOnline surrogate keys + they depend on (via the Surrogate-Key response header), which lets MITxOnline + invalidate those pages when course/program data changes. The service is + therefore Learn's -- purging MITxOnline's own Fastly service would do nothing, + since it tags no responses with these keys. Key format: mitxonline:course: or mitxonline:program: Args: surrogate_key (str): The surrogate key to purge, e.g. "mitxonline:course:course-v1:MITx+6.00.1x" + service_id (str): The Fastly service ID whose cache should be purged. + Falls back to settings.MIT_LEARN_FASTLY_SERVICE_ID when omitted. """ logger = logging.getLogger("fastly_purge") - if not MITX_ONLINE_FASTLY_SERVICE_ID: + service_id = service_id or settings.MIT_LEARN_FASTLY_SERVICE_ID + + if not service_id: logger.warning( - "FASTLY_SERVICE_ID is not set; skipping surrogate key purge for %s", + "No Fastly service ID given; skipping surrogate key purge for %s. " + "Is MIT_LEARN_FASTLY_SERVICE_ID set?", surrogate_key, ) return False - if not MITX_ONLINE_FASTLY_AUTH_TOKEN: + if not settings.FASTLY_AUTH_TOKEN: logger.warning( "FASTLY_AUTH_TOKEN is not set; skipping surrogate key purge for %s", surrogate_key, ) return False - logger.info("Purging Fastly surrogate key: %s", surrogate_key) + logger.info( + "Purging Fastly surrogate key %s from service %s", surrogate_key, service_id + ) api_url = urljoin( - MITX_ONLINE_FASTLY_URL, - f"/service/{MITX_ONLINE_FASTLY_SERVICE_ID}/purge/{surrogate_key}", + settings.FASTLY_URL, + f"/service/{service_id}/purge/{surrogate_key}", ) headers = { - "Fastly-Key": MITX_ONLINE_FASTLY_AUTH_TOKEN, + "Fastly-Key": settings.FASTLY_AUTH_TOKEN, "fastly-soft-purge": "1", } diff --git a/cms/tasks_test.py b/cms/tasks_test.py new file mode 100644 index 0000000000..9e3592ba8a --- /dev/null +++ b/cms/tasks_test.py @@ -0,0 +1,124 @@ +"""Tests for cms.tasks""" + +import pytest +import responses + +from cms.tasks import call_fastly_purge_api, queue_fastly_surrogate_key_purge + +# Deliberately not the production default (https://api.fastly.com), so that +# reading these settings at module import time rather than call time would fail +# to match the registered responses. +FASTLY_URL = "https://fastly.test" +SITE_BASE_URL = "https://mitxonline.test" +FASTLY_AUTH_TOKEN = "fastly-token" # noqa: S105 + +LEARN_SERVICE_ID = "test-learn-service-id" +SURROGATE_KEY = "mitxonline:course:course-v1:MITx+6.00.1x" + + +@pytest.fixture +def fastly_settings(settings): + """ + Configure the Fastly settings the purge tasks read. + + MIT_LEARN_FASTLY_SERVICE_ID is set to a value the tests never register a + response for, so that a test meaning to exercise an explicitly passed + service ID cannot pass by falling back to settings. + """ + settings.FASTLY_URL = FASTLY_URL + settings.FASTLY_AUTH_TOKEN = FASTLY_AUTH_TOKEN + settings.SITE_BASE_URL = SITE_BASE_URL + settings.MIT_LEARN_FASTLY_SERVICE_ID = "unregistered-fallback-service-id" + return settings + + +@responses.activate +def test_queue_fastly_surrogate_key_purge_targets_given_service(fastly_settings): + """An explicitly passed service ID takes precedence over the setting.""" + purge = responses.add( + responses.POST, + f"{FASTLY_URL}/service/{LEARN_SERVICE_ID}/purge/{SURROGATE_KEY}", + json={"status": "ok"}, + status=200, + ) + + assert queue_fastly_surrogate_key_purge(SURROGATE_KEY, LEARN_SERVICE_ID) is True + + assert purge.call_count == 1 + assert responses.calls[0].request.headers["Fastly-Key"] == FASTLY_AUTH_TOKEN + + +@responses.activate +def test_queue_fastly_surrogate_key_purge_falls_back_to_settings(fastly_settings): + """ + Called with the surrogate key alone, the task purges Learn's service anyway. + + This is the shape of a message enqueued by a release that does not pass + service_id, so the fallback is what keeps purges working across a rolling + deploy rather than silently skipping them. + """ + fastly_settings.MIT_LEARN_FASTLY_SERVICE_ID = LEARN_SERVICE_ID + purge = responses.add( + responses.POST, + f"{FASTLY_URL}/service/{LEARN_SERVICE_ID}/purge/{SURROGATE_KEY}", + json={"status": "ok"}, + status=200, + ) + + assert queue_fastly_surrogate_key_purge(SURROGATE_KEY) is True + + assert purge.call_count == 1 + + +@responses.activate +def test_queue_fastly_surrogate_key_purge_skips_without_service_id(fastly_settings): + """ + With no service ID passed and none configured, the purge is skipped. + + It must skip rather than raise or request `/service/None/purge/...`. + """ + fastly_settings.MIT_LEARN_FASTLY_SERVICE_ID = None + + assert queue_fastly_surrogate_key_purge(SURROGATE_KEY) is False + assert not responses.calls + + +@responses.activate +def test_queue_fastly_surrogate_key_purge_skips_without_auth_token(fastly_settings): + """A missing auth token skips the purge rather than sending it unauthenticated.""" + fastly_settings.FASTLY_AUTH_TOKEN = None + + assert queue_fastly_surrogate_key_purge(SURROGATE_KEY, LEARN_SERVICE_ID) is False + assert not responses.calls + + +@responses.activate +def test_queue_fastly_surrogate_key_purge_returns_false_on_error(fastly_settings): + """A Fastly error response is reported as a failure rather than swallowed.""" + responses.add( + responses.POST, + f"{FASTLY_URL}/service/{LEARN_SERVICE_ID}/purge/{SURROGATE_KEY}", + status=503, + ) + + assert queue_fastly_surrogate_key_purge(SURROGATE_KEY, LEARN_SERVICE_ID) is False + + +@responses.activate +def test_call_fastly_purge_api_targets_mitxonline_by_host(fastly_settings): + """ + The URL purge identifies MITxOnline's own site by `host`, not by service ID. + + Covers the three settings this helper reads, none of which are exercised + elsewhere. + """ + purge = responses.add( + responses.Response(method="PURGE", url=f"{FASTLY_URL}/catalog/", json={}) + ) + + call_fastly_purge_api("/catalog/") + + assert purge.call_count == 1 + sent_headers = responses.calls[0].request.headers + assert sent_headers["host"] == "mitxonline.test" + assert sent_headers["fastly-key"] == FASTLY_AUTH_TOKEN diff --git a/main/settings.py b/main/settings.py index cc8c7c3071..76b74a8c78 100644 --- a/main/settings.py +++ b/main/settings.py @@ -1350,22 +1350,36 @@ # Fastly configuration -MITX_ONLINE_FASTLY_AUTH_TOKEN = get_string( - name="FASTLY_AUTH_TOKEN", +# MITxOnline's own Fastly config follows the convention used throughout this file: +# the MITX_ONLINE_-prefixed name is the *env var*, the unprefixed one the setting +# (as EMAIL_BACKEND is read from MITX_ONLINE_EMAIL_BACKEND, and 16 others). +# These three previously inverted that -- prefixed settings reading bare env vars -- +# which is how the env vars came to be renamed to the setting names, breaking the +# read entirely: https://github.com/mitodl/ol-infrastructure/pull/5119 +FASTLY_AUTH_TOKEN = get_string( + name="MITX_ONLINE_FASTLY_AUTH_TOKEN", default=None, description="Optional token for the Fastly purge API.", ) -MITX_ONLINE_FASTLY_URL = get_string( - name="FASTLY_URL", +FASTLY_URL = get_string( + name="MITX_ONLINE_FASTLY_URL", default="https://api.fastly.com", description="The URL to the Fastly API.", ) -MITX_ONLINE_FASTLY_SERVICE_ID = get_string( - name="FASTLY_SERVICE_ID", +# Not MITX_ONLINE_-prefixed, because the value is not MITxOnline's: MIT_LEARN_* is +# this file's namespace for MIT Learn config, and those seven settings all use the +# env var name unchanged. +MIT_LEARN_FASTLY_SERVICE_ID = get_string( + name="MIT_LEARN_FASTLY_SERVICE_ID", default=None, - description="Fastly service ID used for surrogate key (tag) purging.", + description=( + "Fastly service ID for the MIT Learn frontend, used for surrogate key " + "(tag) purging. MIT Learn tags its product page responses with MITxOnline " + "surrogate keys, so this is Learn's service ID -- not the service ID of " + "MITxOnline's own Fastly service." + ), ) # Hubspot sync settings From b938efccee9cf747c7b8672965d739125af311e7 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Fri, 31 Jul 2026 11:58:33 -0400 Subject: [PATCH 3/8] Use psycopg's C implementation and drop unused psycopg2 (#3801) Co-authored-by: Claude Opus 5 (1M context) --- Aptfile | 1 + pyproject.toml | 3 +-- uv.lock | 20 ++++++++++---------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/Aptfile b/Aptfile index 42c88c5534..82f5756b76 100644 --- a/Aptfile +++ b/Aptfile @@ -1 +1,2 @@ +libpq-dev libxmlsec1-dev diff --git a/pyproject.toml b/pyproject.toml index 6e909ffbfa..fc76aee4f0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -52,8 +52,7 @@ dependencies = [ "opentelemetry-instrumentation-psycopg>=0.52b0", "opentelemetry-instrumentation-redis>=0.52b0", "opentelemetry-instrumentation-requests>=0.52b0", - "psycopg>=3.2.4,<4", - "psycopg2>=2.9.5,<3", + "psycopg[c]>=3.2.4,<4", "pyOpenSSL>=26,<27", "pycountry>=26.2.16,<27", "pyparsing>=3.2,<4", diff --git a/uv.lock b/uv.lock index 3e606a76ce..6cad09c5ab 100644 --- a/uv.lock +++ b/uv.lock @@ -2079,8 +2079,7 @@ dependencies = [ { name = "opentelemetry-instrumentation-psycopg" }, { name = "opentelemetry-instrumentation-redis" }, { name = "opentelemetry-instrumentation-requests" }, - { name = "psycopg" }, - { name = "psycopg2" }, + { name = "psycopg", extra = ["c"] }, { name = "pycountry" }, { name = "pyopenssl" }, { name = "pyparsing" }, @@ -2188,8 +2187,7 @@ requires-dist = [ { name = "opentelemetry-instrumentation-psycopg", specifier = ">=0.52b0" }, { name = "opentelemetry-instrumentation-redis", specifier = ">=0.52b0" }, { name = "opentelemetry-instrumentation-requests", specifier = ">=0.52b0" }, - { name = "psycopg", specifier = ">=3.2.4,<4" }, - { name = "psycopg2", specifier = ">=2.9.5,<3" }, + { name = "psycopg", extras = ["c"], specifier = ">=3.2.4,<4" }, { name = "pycountry", specifier = ">=26.2.16,<27" }, { name = "pyopenssl", specifier = ">=26,<27" }, { name = "pyparsing", specifier = ">=3.2,<4" }, @@ -2787,14 +2785,16 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/c8/5b/181e2e3becb7672b502f0ed7f16ed7352aca7c109cfb94cf3878a9186db9/psycopg-3.3.3-py3-none-any.whl", hash = "sha256:f96525a72bcfade6584ab17e89de415ff360748c766f0106959144dcbb38c698", size = 212768, upload-time = "2026-02-18T16:46:27.365Z" }, ] +[package.optional-dependencies] +c = [ + { name = "psycopg-c", marker = "implementation_name != 'pypy'" }, +] + [[package]] -name = "psycopg2" -version = "2.9.11" +name = "psycopg-c" +version = "3.3.3" source = { registry = "https://pypi.org/simple" } -sdist = { url = "https://files.pythonhosted.org/packages/89/8d/9d12bc8677c24dad342ec777529bce705b3e785fa05d85122b5502b9ab55/psycopg2-2.9.11.tar.gz", hash = "sha256:964d31caf728e217c697ff77ea69c2ba0865fa41ec20bb00f0977e62fdcc52e3", size = 379598, upload-time = "2025-10-10T11:14:46.075Z" } -wheels = [ - { url = "https://files.pythonhosted.org/packages/86/fe/d6dce306fd7b61e312757ba4d068617f562824b9c6d3e4a39fc578ea2814/psycopg2-2.9.11-cp311-cp311-win_amd64.whl", hash = "sha256:210daed32e18f35e3140a1ebe059ac29209dd96468f2f7559aa59f75ee82a5cb", size = 2713723, upload-time = "2025-10-10T11:10:12.957Z" }, -] +sdist = { url = "https://files.pythonhosted.org/packages/cb/a0/8feb0ca8c7c20a8b9ac4d46b335ddd57e48e593b714262f006880f34fee5/psycopg_c-3.3.3.tar.gz", hash = "sha256:86ef6f4424348247828e83fb0882c9f8acb33e64d0a5ce66c1b4a5107ee73edd", size = 631965, upload-time = "2026-02-18T16:52:18.084Z" } [[package]] name = "ptyprocess" From c67cfc35cb899c51c60999d9c0aa2692c6f6e173 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Fri, 31 Jul 2026 16:59:12 -0400 Subject: [PATCH 4/8] Identify PostHog persons by Keycloak global_id, not Django pk (#3798) Co-authored-by: Claude Opus 5 (1M context) --- cms/models.py | 4 +- ecommerce/views/legacy/__init__.py | 16 +- ecommerce/views/legacy/views_test.py | 21 + .../components/CourseProductDetailEnroll.js | 4 +- frontend/public/src/components/Header.js | 6 - frontend/public/src/components/TopBar.js | 4 +- .../src/containers/pages/DashboardPage.js | 15 +- .../containers/pages/DashboardPage_test.js | 373 +++--------------- frontend/public/src/lib/queries/users.js | 4 +- frontend/public/src/store/configureStore.js | 4 +- frontend/public/src/store/posthogIdentify.js | 54 +++ .../public/src/store/posthogIdentify_test.js | 117 ++++++ 12 files changed, 267 insertions(+), 355 deletions(-) create mode 100644 frontend/public/src/store/posthogIdentify.js create mode 100644 frontend/public/src/store/posthogIdentify_test.js diff --git a/cms/models.py b/cms/models.py index 7c3661d98c..f72346174c 100644 --- a/cms/models.py +++ b/cms/models.py @@ -924,8 +924,8 @@ def get_context(self, request, *args, **kwargs): # noqa: ARG002 hubspot_portal_id = settings.HUBSPOT_PORTAL_ID hubspot_home_page_form_guid = settings.HUBSPOT_HOME_PAGE_FORM_GUID - if request.user.is_authenticated: - user = request.user.id + if request.user.is_authenticated and request.user.global_id: + user = request.user.global_id else: if "anonymous_session_id" not in request.session: request.session["anonymous_session_id"] = str(uuid.uuid4()) diff --git a/ecommerce/views/legacy/__init__.py b/ecommerce/views/legacy/__init__.py index 9b61675ae8..2f6b6cc435 100644 --- a/ecommerce/views/legacy/__init__.py +++ b/ecommerce/views/legacy/__init__.py @@ -1087,10 +1087,18 @@ def get(self, request): # noqa: PLR0911, C901 "form": checkout_payload["payload"], } - ga_purchase_flag = is_posthog_enabled( - features.ENABLE_GOOGLE_ANALYTICS_DATA_PUSH, - False, # noqa: FBT003 - self.request.user.id, + # NOTE: Leave a user with no global_id unflagged rather than passing + # None through. is_posthog_enabled falls back to its default unique id + # (the hostname) when opt_unique_id is empty, which would evaluate and + # cache the flag once for every such user collectively. + global_id = self.request.user.global_id + ga_purchase_flag = bool( + global_id + and is_posthog_enabled( + features.ENABLE_GOOGLE_ANALYTICS_DATA_PUSH, + default=False, + opt_unique_id=global_id, + ) ) ga_purchase_payload = None if ga_purchase_flag: diff --git a/ecommerce/views/legacy/views_test.py b/ecommerce/views/legacy/views_test.py index dbba694283..4c3b1e6a84 100644 --- a/ecommerce/views/legacy/views_test.py +++ b/ecommerce/views/legacy/views_test.py @@ -1547,6 +1547,27 @@ def test_checkout_interstitial_google_analytics_object( assert isinstance(item["quantity"], int) +def test_checkout_interstitial_no_ga_flag_without_global_id( + mocker, settings, user_client, products +): + """A user with no global_id is left unflagged rather than sharing a bucket.""" + + settings.OPENEDX_SERVICE_WORKER_API_TOKEN = "mock_api_token" # noqa: S105 + + user_no_global_id = UserFactory.create(global_id=None) + user_client.force_login(user_no_global_id) + + mock_is_enabled = mocker.patch("ecommerce.views.legacy.is_posthog_enabled") + + basket = create_basket_with_product(user_no_global_id, products[0]) + PendingOrder.create_from_basket(basket) + resp = user_client.get(reverse("checkout_interstitial_page")) + + assert resp.status_code == 200 + assert "ga_purchase_payload" not in resp.context + mock_is_enabled.assert_not_called() + + @pytest.mark.skip_nplusone_check def test_program_product_purchasing(user, user_drf_client): """Test that we can purchase products that are for programs.""" diff --git a/frontend/public/src/components/CourseProductDetailEnroll.js b/frontend/public/src/components/CourseProductDetailEnroll.js index 9978a8faaa..725cffe3c5 100644 --- a/frontend/public/src/components/CourseProductDetailEnroll.js +++ b/frontend/public/src/components/CourseProductDetailEnroll.js @@ -291,7 +291,9 @@ export class CourseProductDetailEnroll extends React.Component< const product = run && run.products ? run.products[0] : null const newCartDesign = checkFeatureFlag( "new-cart-design", - currentUser && currentUser.id ? currentUser.id : "anonymousUser" + currentUser && currentUser.global_id ? + currentUser.global_id : + "anonymousUser" ) const canUpgrade = !!(run && run.is_upgradable && product) return upgradableCourseRuns.length > 0 || diff --git a/frontend/public/src/components/Header.js b/frontend/public/src/components/Header.js index 2181d0cda8..c13f18369b 100644 --- a/frontend/public/src/components/Header.js +++ b/frontend/public/src/components/Header.js @@ -1,8 +1,6 @@ // @flow -/* global SETTINGS:false*/ import React from "react" import * as Sentry from "@sentry/browser" -import posthog from "posthog-js" import type { CurrentUser } from "../flow/authTypes" import type { Location } from "react-router" @@ -22,10 +20,6 @@ const Header = ({ currentUser, cartItemsCount, location }: Props) => { username: currentUser.username, name: currentUser.name }) - posthog.identify(currentUser.id, { - environment: SETTINGS.environment, - user_id: currentUser.id - }) } else { Sentry.getCurrentScope().setUser(null) } diff --git a/frontend/public/src/components/TopBar.js b/frontend/public/src/components/TopBar.js index 71eb585f3f..0d784c4198 100644 --- a/frontend/public/src/components/TopBar.js +++ b/frontend/public/src/components/TopBar.js @@ -32,8 +32,8 @@ const TopBar = ({ currentUser, cartItemsCount }: Props) => { const newCartDesign = checkFeatureFlag( "new-cart-design", - currentUser && currentUser.is_authenticated && currentUser.id ? - currentUser.id : + currentUser && currentUser.is_authenticated && currentUser.global_id ? + currentUser.global_id : "anonymousUser" ) return ( diff --git a/frontend/public/src/containers/pages/DashboardPage.js b/frontend/public/src/containers/pages/DashboardPage.js index d760330b94..bc274834b0 100644 --- a/frontend/public/src/containers/pages/DashboardPage.js +++ b/frontend/public/src/containers/pages/DashboardPage.js @@ -8,7 +8,6 @@ import { compose } from "redux" import { mutateAsync } from "redux-query" import { connectRequest } from "redux-query-react" import { pathOr } from "ramda" -import posthog from "posthog-js" import Loader from "../../components/Loader" import { DASHBOARD_PAGE_TITLE } from "../../constants" import { @@ -84,16 +83,10 @@ export class DashboardPage extends React.Component< componentDidMount() { const { currentUser } = this.props - // Identify the user to PostHog using their global_id (GUID) if available + // Identifying happens in a store middleware (see + // store/posthogIdentify.js) whenever /current_user/ loads. Here we just + // check the feature flag and redirect if enabled. if (currentUser && currentUser.global_id && SETTINGS.posthog_api_host) { - posthog.identify(currentUser.global_id, { - email: currentUser.email, - name: currentUser.name, - user_id: currentUser.id, - environment: SETTINGS.environment - }) - - // Wait a short time for PostHog to process the identify call before checking feature flags setTimeout(() => { try { // Check feature flag and redirect if enabled @@ -114,7 +107,7 @@ export class DashboardPage extends React.Component< } catch (error) { console.warn("Feature flag check failed:", error) } - }, 500) // Wait 500ms for PostHog to process the identify call + }, 500) // Wait for PostHog to process the identify call } } diff --git a/frontend/public/src/containers/pages/DashboardPage_test.js b/frontend/public/src/containers/pages/DashboardPage_test.js index 03ca1b7823..011bf8939c 100644 --- a/frontend/public/src/containers/pages/DashboardPage_test.js +++ b/frontend/public/src/containers/pages/DashboardPage_test.js @@ -1,7 +1,6 @@ // @flow import { assert } from "chai" import sinon from "sinon" -import posthog from "posthog-js" import DashboardPage, { DashboardPage as InnerDashboardPage @@ -9,37 +8,13 @@ import DashboardPage, { import IntegrationTestHelper from "../../util/integration_test_helper" import { makeCourseRunEnrollment } from "../../factories/course" -import { makeUser } from "../../factories/user" +import { makeAnonymousUser, makeUser } from "../../factories/user" import * as util from "../../lib/util" describe("DashboardPage", () => { let helper, renderPage, userEnrollments, currentUser, sandbox, mockSettings beforeEach(() => { - if (!global.performance) { - global.performance = {} - } - - const mockFn = () => undefined - - ;["mark", "measure", "clearMarks", "clearMeasures"].forEach(method => { - try { - if (typeof global.performance[method] !== "function") { - Object.defineProperty(global.performance, method, { - value: mockFn, - writable: true, - configurable: true - }) - } - } catch (e) { - try { - global.performance[method] = mockFn - } catch (err) { - // If all else fails, silently skip - the mock may already exist - } - } - }) - helper = new IntegrationTestHelper() userEnrollments = [makeCourseRunEnrollment(), makeCourseRunEnrollment()] currentUser = { @@ -86,20 +61,18 @@ describe("DashboardPage", () => { }) describe("PostHog feature flag redirect", () => { - let mockLocation, posthogIdentifyStub, checkFeatureFlagStub, clock + const FLAG = "redirect-to-learn-dashboard" + const DEFAULT_DASHBOARD_URL = "https://learn.mit.edu/dashboard" + + let mockLocation, checkFeatureFlagStub, clock beforeEach(() => { - // Mock window.location.href and search mockLocation = { href: "", search: "" } sandbox.stub(window, "location").value(mockLocation) - // Mock PostHog methods - posthogIdentifyStub = sandbox.stub(posthog, "identify") - - // Mock checkFeatureFlag checkFeatureFlagStub = sandbox.stub(util, "checkFeatureFlag") - // Create fake timer to control setTimeout + // The component defers its flag check with setTimeout clock = sandbox.useFakeTimers() }) @@ -108,359 +81,105 @@ describe("DashboardPage", () => { mockSettings.mit_learn_dashboard_url = undefined }) - it("identifies user to PostHog and redirects when feature flag is enabled", async () => { - const mockUser = makeUser() - mockUser.global_id = "test-guid-123" - - // Mock checkFeatureFlag to return true - checkFeatureFlagStub - .withArgs("redirect-to-learn-dashboard", "test-guid-123") - .returns(true) - - await renderPage( - { - entities: { - enrollments: userEnrollments, - currentUser: mockUser // Override the currentUser from beforeEach - } - }, + const renderForUser = (mockUser: Object) => + renderPage( + { entities: { enrollments: userEnrollments, currentUser: mockUser } }, { currentUser: mockUser } ) - // Component mounts automatically, so PostHog calls should have been made - // Check that PostHog identify was called - sinon.assert.called(posthogIdentifyStub) + it("redirects to the default dashboard URL when the flag is enabled", async () => { + const mockUser = makeUser() + checkFeatureFlagStub.withArgs(FLAG, mockUser.global_id).returns(true) - // Check the identify call had the correct GUID - const identifyCall = posthogIdentifyStub.getCall(0) - assert.equal(identifyCall.args[0], "test-guid-123") - assert.equal(identifyCall.args[1].email, mockUser.email) - assert.equal(identifyCall.args[1].name, mockUser.name) - assert.equal(identifyCall.args[1].user_id, mockUser.id) - assert.equal(identifyCall.args[1].environment, "test") + await renderForUser(mockUser) - // Feature flag check hasn't happened yet (it's in setTimeout) sinon.assert.notCalled(checkFeatureFlagStub) - // Advance time to trigger the setTimeout clock.tick(500) - // Now verify checkFeatureFlag was called - sinon.assert.calledWith( - checkFeatureFlagStub, - "redirect-to-learn-dashboard", - "test-guid-123" - ) - - // Verify redirect happened - assert.equal(mockLocation.href, "https://learn.mit.edu/dashboard") + sinon.assert.calledWith(checkFeatureFlagStub, FLAG, mockUser.global_id) + assert.equal(mockLocation.href, DEFAULT_DASHBOARD_URL) }) it("preserves query parameters in the redirect URL", async () => { const mockUser = makeUser() - mockUser.global_id = "test-guid-123" - mockLocation.search = "?a=1&b=2" + checkFeatureFlagStub.withArgs(FLAG, mockUser.global_id).returns(true) - checkFeatureFlagStub - .withArgs("redirect-to-learn-dashboard", "test-guid-123") - .returns(true) - - await renderPage( - { - entities: { - enrollments: userEnrollments, - currentUser: mockUser - } - }, - { currentUser: mockUser } - ) - + await renderForUser(mockUser) clock.tick(500) - assert.equal(mockLocation.href, "https://learn.mit.edu/dashboard?a=1&b=2") + assert.equal(mockLocation.href, `${DEFAULT_DASHBOARD_URL}?a=1&b=2`) }) - it("does not redirect when feature flag is disabled", async () => { + it("uses MIT_LEARN_DASHBOARD_URL when it is set", async () => { const mockUser = makeUser() - mockUser.global_id = "test-guid-123" - - // Mock checkFeatureFlag to return false - checkFeatureFlagStub - .withArgs("redirect-to-learn-dashboard", "test-guid-123") - .returns(false) - - await renderPage( - { - entities: { - enrollments: userEnrollments, - currentUser: mockUser // Override the currentUser from beforeEach - } - }, - { currentUser: mockUser } - ) - - // Component mounts automatically, so PostHog calls should have been made - // Verify PostHog identify was called - sinon.assert.called(posthogIdentifyStub) - - // Feature flag check hasn't happened yet (it's in setTimeout) - sinon.assert.notCalled(checkFeatureFlagStub) + const customDashboardUrl = "https://custom.example.com/dashboard" + mockSettings.mit_learn_dashboard_url = customDashboardUrl + checkFeatureFlagStub.withArgs(FLAG, mockUser.global_id).returns(true) - // Advance time to trigger the setTimeout + await renderForUser(mockUser) clock.tick(500) - // Now verify checkFeatureFlag was called - sinon.assert.calledWith( - checkFeatureFlagStub, - "redirect-to-learn-dashboard", - "test-guid-123" - ) - - // Verify no redirect happened - assert.equal(mockLocation.href, "") - }) - - it("does not redirect when user has no global_id", async () => { - const mockUser = { - id: 123, - email: "test@example.com", - name: "Test User", - is_anonymous: false, - is_authenticated: true - // Explicitly no global_id property - } - - await renderPage( - { - entities: { - enrollments: userEnrollments, - currentUser: mockUser // Override the currentUser from beforeEach - } - }, - { currentUser: mockUser } - ) - - // Component mounts automatically - // Since there's no global_id, PostHog identify should not be called - sinon.assert.notCalled(posthogIdentifyStub) - - // Verify no redirect happened - assert.equal(mockLocation.href, "") + assert.equal(mockLocation.href, customDashboardUrl) }) - it("does not redirect when PostHog is not configured", async () => { + it("does not redirect when the flag is disabled", async () => { const mockUser = makeUser() - mockUser.global_id = "test-guid-123" - - // Remove PostHog configuration - global.SETTINGS.posthog_api_host = null - - await renderPage( - { - entities: { - enrollments: userEnrollments, - currentUser: mockUser // Override the currentUser from beforeEach - } - }, - { currentUser: mockUser } - ) + checkFeatureFlagStub.withArgs(FLAG, mockUser.global_id).returns(false) - // Component mounts automatically - // Verify PostHog identify was not called - sinon.assert.notCalled(posthogIdentifyStub) + await renderForUser(mockUser) + clock.tick(500) - // Verify no redirect happened + sinon.assert.calledWith(checkFeatureFlagStub, FLAG, mockUser.global_id) assert.equal(mockLocation.href, "") }) - it("handles checkFeatureFlag returning true", async () => { + it("does not redirect when the flag check throws", async () => { const mockUser = makeUser() - mockUser.global_id = "test-guid-456" - - // Mock checkFeatureFlag to return true checkFeatureFlagStub - .withArgs("redirect-to-learn-dashboard", "test-guid-456") - .returns(true) - - await renderPage( - { - entities: { - enrollments: userEnrollments, - currentUser: mockUser // Override the currentUser from beforeEach - } - }, - { currentUser: mockUser } - ) - - // Verify PostHog identify was called - sinon.assert.called(posthogIdentifyStub) - - // Feature flag check hasn't happened yet (it's in setTimeout) - sinon.assert.notCalled(checkFeatureFlagStub) + .withArgs(FLAG, mockUser.global_id) + .throws(new Error("PostHog service unavailable")) - // Advance time to trigger the setTimeout + await renderForUser(mockUser) clock.tick(500) - // Now verify checkFeatureFlag was called - sinon.assert.calledWith( - checkFeatureFlagStub, - "redirect-to-learn-dashboard", - "test-guid-456" - ) - - // Verify redirect happened - assert.equal(mockLocation.href, "https://learn.mit.edu/dashboard") + sinon.assert.calledWith(checkFeatureFlagStub, FLAG, mockUser.global_id) + assert.equal(mockLocation.href, "") }) - it("handles checkFeatureFlag gracefully when it throws an error", async () => { - const mockUser = makeUser() - mockUser.global_id = "test-guid-789" - - // Mock checkFeatureFlag to throw an error - checkFeatureFlagStub - .withArgs("redirect-to-learn-dashboard", "test-guid-789") - .throws(new Error("PostHog service unavailable")) - - await renderPage( - { - entities: { - enrollments: userEnrollments, - currentUser: mockUser // Override the currentUser from beforeEach - } - }, - { currentUser: mockUser } - ) - - // Verify PostHog identify was called - sinon.assert.called(posthogIdentifyStub) - - // Feature flag check hasn't happened yet (it's in setTimeout) - sinon.assert.notCalled(checkFeatureFlagStub) - - // Advance time to trigger the setTimeout + it("does not check the flag when the user has no global_id", async () => { + await renderForUser({ ...makeUser(), global_id: null }) clock.tick(500) - // Now verify checkFeatureFlag was called - sinon.assert.calledWith( - checkFeatureFlagStub, - "redirect-to-learn-dashboard", - "test-guid-789" - ) - - // Verify no redirect happened (error handled gracefully) + sinon.assert.notCalled(checkFeatureFlagStub) assert.equal(mockLocation.href, "") }) - it("handles undefined currentUser gracefully", async () => { + it("does not check the flag when there is no current user", async () => { await renderPage( { entities: { enrollments: userEnrollments, - currentUser: { - // Minimal user object that won't break the component - id: null, - is_anonymous: true, - is_authenticated: false - } + currentUser: makeAnonymousUser() } }, { currentUser: null } ) + clock.tick(500) - // Component mounts automatically with null user - // Since there's no currentUser, PostHog identify should not be called - sinon.assert.notCalled(posthogIdentifyStub) - - // Verify no redirect happened + sinon.assert.notCalled(checkFeatureFlagStub) assert.equal(mockLocation.href, "") }) - it("uses MIT_LEARN_DASHBOARD_URL setting when provided", async () => { - const mockUser = makeUser() - mockUser.global_id = "test-guid-custom-url" - - // Set custom URL in settings - this needs to be a truthy value - const customDashboardUrl = "https://custom.example.com/dashboard" - mockSettings.mit_learn_dashboard_url = customDashboardUrl - - // Mock checkFeatureFlag to return true - checkFeatureFlagStub - .withArgs("redirect-to-learn-dashboard", "test-guid-custom-url") - .returns(true) - - await renderPage( - { - entities: { - enrollments: userEnrollments, - currentUser: mockUser // Override the currentUser from beforeEach - } - }, - { currentUser: mockUser } - ) - - // Component mounts automatically - // Verify PostHog identify was called - sinon.assert.called(posthogIdentifyStub) - - // Feature flag check hasn't happened yet (it's in setTimeout) - sinon.assert.notCalled(checkFeatureFlagStub) + it("does not check the flag when PostHog is not configured", async () => { + global.SETTINGS.posthog_api_host = null - // Advance time to trigger the setTimeout + await renderForUser(makeUser()) clock.tick(500) - // Now verify checkFeatureFlag was called - sinon.assert.calledWith( - checkFeatureFlagStub, - "redirect-to-learn-dashboard", - "test-guid-custom-url" - ) - - // Verify redirect happened with custom URL - assert.equal(mockLocation.href, customDashboardUrl) - }) - - it("falls back to default URL when MIT_LEARN_DASHBOARD_URL is not set", async () => { - const mockUser = makeUser() - mockUser.global_id = "test-guid-fallback" - - // Ensure mit_learn_dashboard_url is explicitly not set - mockSettings.mit_learn_dashboard_url = undefined - - // Mock checkFeatureFlag to return true - checkFeatureFlagStub - .withArgs("redirect-to-learn-dashboard", "test-guid-fallback") - .returns(true) - - await renderPage( - { - entities: { - enrollments: userEnrollments, - currentUser: mockUser // Override the currentUser from beforeEach - } - }, - { currentUser: mockUser } - ) - - // Component mounts automatically - // Verify PostHog identify was called - sinon.assert.called(posthogIdentifyStub) - - // Feature flag check hasn't happened yet (it's in setTimeout) sinon.assert.notCalled(checkFeatureFlagStub) - - // Advance time to trigger the setTimeout - clock.tick(500) - - // Now verify checkFeatureFlag was called - sinon.assert.calledWith( - checkFeatureFlagStub, - "redirect-to-learn-dashboard", - "test-guid-fallback" - ) - - // Verify redirect happened with default URL (fallback) - assert.equal(mockLocation.href, "https://learn.mit.edu/dashboard") + assert.equal(mockLocation.href, "") }) }) }) diff --git a/frontend/public/src/lib/queries/users.js b/frontend/public/src/lib/queries/users.js index 065e7736c9..e6951e495d 100644 --- a/frontend/public/src/lib/queries/users.js +++ b/frontend/public/src/lib/queries/users.js @@ -12,6 +12,8 @@ import type { export const currentUserSelector = (state: any): ?CurrentUser => state.entities.currentUser +export const CURRENT_USER_URL = "/api/v0/users/current_user/" + // replace the previous state with the next state without merging const nextState = nthArg(1) @@ -31,7 +33,7 @@ const DEFAULT_OPTIONS = { export default { currentUserQuery: () => ({ - url: "/api/v0/users/current_user/", + url: CURRENT_USER_URL, transform: transformCurrentUser, update: updateResult }), diff --git a/frontend/public/src/store/configureStore.js b/frontend/public/src/store/configureStore.js index 1e42876ddf..bc18826dd6 100644 --- a/frontend/public/src/store/configureStore.js +++ b/frontend/public/src/store/configureStore.js @@ -5,11 +5,13 @@ import { queryMiddleware } from "redux-query" import { makeRequest } from "./network_interface" import rootReducer from "../reducers" import { getEntities, getQueries } from "../lib/queries/util" +import posthogIdentifyMiddleware from "./posthogIdentify" // Setup middleware export default function configureStore(initialState: Object) { const COMMON_MIDDLEWARE = [ - queryMiddleware(makeRequest, getQueries, getEntities) + queryMiddleware(makeRequest, getQueries, getEntities), + posthogIdentifyMiddleware ] // Store factory configuration diff --git a/frontend/public/src/store/posthogIdentify.js b/frontend/public/src/store/posthogIdentify.js new file mode 100644 index 0000000000..2f55ff1596 --- /dev/null +++ b/frontend/public/src/store/posthogIdentify.js @@ -0,0 +1,54 @@ +// @flow +/* global SETTINGS:false */ +import posthog from "posthog-js" +import { actionTypes } from "redux-query" + +import { CURRENT_USER_URL } from "../lib/queries/users" + +// Identifies the user to PostHog as soon as /api/v0/users/current_user/ +// succeeds, regardless of which component triggered the request. This +// posthog project is shared with other MIT applications, and xpro +// identifies people by its own integer user ids, so integer ids collide +// across applications. Users with no global id are left unidentified +// rather than identified by a colliding id. +// +// Nothing here guards against identifying the same person twice, because +// posthog "will ignore the subsequent calls" when identify is called +// repeatedly with the same data within a page load: +// https://posthog.com/docs/getting-started/identify-users +// +// Signing out has to be handled here too. Signin and signout happen on the SSO +// server, so there is no client-side signout to hook; instead we reset whenever +// the browser turns out to be anonymous while posthog still thinks it is +// identified. Without that, identify latches $user_state to "identified" and a +// logged-out browser goes on attributing events to whoever logged in last. +// mit-learn's ConfiguredPostHogProvider does the same thing for the same reason. +const posthogIdentifyMiddleware = () => (next: Function) => (action: any) => { + const result = next(action) + + if ( + SETTINGS.posthog_api_host && + action.type === actionTypes.REQUEST_SUCCESS && + action.url === CURRENT_USER_URL + ) { + const currentUser = action.entities && action.entities.currentUser + + // Both branches key off what the response affirmatively says, so a + // response carrying no user at all is left alone rather than reset, and so + // is an authenticated user who simply has no global id. + if (currentUser && currentUser.is_anonymous) { + if (posthog.get_property("$user_state") !== "anonymous") { + posthog.reset() + } + } else if (currentUser && currentUser.global_id) { + posthog.identify(currentUser.global_id, { + environment: SETTINGS.environment, + user_id: currentUser.global_id + }) + } + } + + return result +} + +export default posthogIdentifyMiddleware diff --git a/frontend/public/src/store/posthogIdentify_test.js b/frontend/public/src/store/posthogIdentify_test.js new file mode 100644 index 0000000000..b2ca051d6c --- /dev/null +++ b/frontend/public/src/store/posthogIdentify_test.js @@ -0,0 +1,117 @@ +// @flow +import { assert } from "chai" +import sinon from "sinon" +import { actionTypes } from "redux-query" +import posthog from "posthog-js" + +import posthogIdentifyMiddleware from "./posthogIdentify" +import { CURRENT_USER_URL } from "../lib/queries/users" +import { makeAnonymousUser, makeUser } from "../factories/user" + +describe("posthogIdentifyMiddleware", () => { + let sandbox, + identifyStub, + resetStub, + getPropertyStub, + next, + invoke, + currentUser + + beforeEach(() => { + sandbox = sinon.createSandbox() + identifyStub = sandbox.stub(posthog, "identify") + resetStub = sandbox.stub(posthog, "reset") + // Scoped to $user_state on purpose. posthog reads its own feature flags + // through get_property too, and handing those a string throws + // "Cannot use 'in' operator", which surfaces as an uncaught error here + // whenever a stray render elsewhere in the suite checks a flag. + getPropertyStub = sandbox.stub(posthog, "get_property") + getPropertyStub.withArgs("$user_state").returns("identified") + global.SETTINGS = { + posthog_api_host: "https://posthog.example.com", + environment: "test" + } + + currentUser = makeUser() + next = sandbox.stub().returnsArg(0) + invoke = action => posthogIdentifyMiddleware()(next)(action) + }) + + afterEach(() => { + sandbox.restore() + delete global.SETTINGS + }) + + const currentUserSuccess = (user = currentUser) => ({ + type: actionTypes.REQUEST_SUCCESS, + url: CURRENT_USER_URL, + entities: { currentUser: user } + }) + + it("identifies the user when the current user request succeeds", () => { + invoke(currentUserSuccess()) + + sinon.assert.calledWith(identifyStub, currentUser.global_id, { + environment: "test", + user_id: currentUser.global_id + }) + }) + + it("leaves an authenticated user with no global_id alone", () => { + invoke(currentUserSuccess({ ...currentUser, global_id: null })) + + sinon.assert.notCalled(identifyStub) + sinon.assert.notCalled(resetStub) + }) + + it("resets when the browser is anonymous but PostHog is still identified", () => { + invoke(currentUserSuccess(makeAnonymousUser())) + + sinon.assert.calledWith(getPropertyStub, "$user_state") + sinon.assert.called(resetStub) + sinon.assert.notCalled(identifyStub) + }) + + it("does not reset when PostHog already considers the browser anonymous", () => { + getPropertyStub.withArgs("$user_state").returns("anonymous") + + invoke(currentUserSuccess(makeAnonymousUser())) + + sinon.assert.notCalled(resetStub) + }) + + it("does not reset when the response carries no user", () => { + invoke({ ...currentUserSuccess(), entities: {} }) + + sinon.assert.notCalled(resetStub) + sinon.assert.notCalled(identifyStub) + }) + + it("ignores successful requests for other URLs", () => { + invoke({ ...currentUserSuccess(), url: "/api/countries/" }) + + sinon.assert.notCalled(identifyStub) + }) + + it("ignores other action types for the current user URL", () => { + invoke({ ...currentUserSuccess(), type: actionTypes.REQUEST_START }) + + sinon.assert.notCalled(identifyStub) + }) + + it("does not identify when PostHog is not configured", () => { + global.SETTINGS.posthog_api_host = null + + invoke(currentUserSuccess()) + + sinon.assert.notCalled(identifyStub) + }) + + it("passes the action through to next", () => { + const action = currentUserSuccess() + const result = invoke(action) + + sinon.assert.calledWith(next, action) + assert.equal(result, action) + }) +}) From c8d3811678e84ceeebc60e8158c1bed0a1956369 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Mon, 3 Aug 2026 15:52:45 -0400 Subject: [PATCH 5/8] feat(b2b): add a service-scoped org-manager check endpoint (#3807) Co-authored-by: Claude Opus 5 --- b2b/serializers/v0/service.py | 31 ++++ b2b/views/v0/service.py | 137 +++++++++++++++++ b2b/views/v0/service_test.py | 267 ++++++++++++++++++++++++++++++++++ b2b/views/v0/urls.py | 8 + main/settings.py | 4 + openapi/specs/v0.yaml | 61 ++++++++ openapi/specs/v1.yaml | 61 ++++++++ openapi/specs/v2.yaml | 61 ++++++++ 8 files changed, 630 insertions(+) create mode 100644 b2b/serializers/v0/service.py create mode 100644 b2b/views/v0/service.py create mode 100644 b2b/views/v0/service_test.py diff --git a/b2b/serializers/v0/service.py b/b2b/serializers/v0/service.py new file mode 100644 index 0000000000..1f0eaffafc --- /dev/null +++ b/b2b/serializers/v0/service.py @@ -0,0 +1,31 @@ +"""Service-to-service B2B serializers. + +TEMPORARY -- delete alongside b2b/views/v0/service.py when org-manager status +becomes visible in Keycloak (mitodl/hq#10594). +""" + +from rest_framework import serializers + + +class OrganizationManagerCheckSerializer(serializers.Serializer): + """Response shape for the org-manager check.""" + + is_manager = serializers.BooleanField( + help_text="True if the user manages the organization.", + ) + + +class ServiceDetailErrorSerializer(serializers.Serializer): + """Response shape for the 400 error responses below. + + Same shape as b2b.serializers.v0.manager.DetailErrorSerializer, but + duplicated rather than imported (and distinctly named, since + drf-spectacular keys its component registry on class identity, not + structural equality -- reusing the same name for a different class + produces a "components with identical names" warning and an + unpredictable schema) so this module stays self-contained and its + deletion remains a plain file removal (see the module docstring in + b2b/views/v0/service.py). + """ + + detail = serializers.CharField() diff --git a/b2b/views/v0/service.py b/b2b/views/v0/service.py new file mode 100644 index 0000000000..37e62edaf2 --- /dev/null +++ b/b2b/views/v0/service.py @@ -0,0 +1,137 @@ +"""Service-to-service B2B views. + +TEMPORARY -- delete this module when org-manager status becomes visible in +Keycloak (mitodl/hq#10594). + +Unlike everything in manager.py, these endpoints answer questions *about* a +named user rather than about the caller. They exist because `is_manager` +(b2b.models.UserOrganization) is curated only here, in the Django admin, and +never reaches the Keycloak token -- so a downstream service that needs the +flag has no way to learn it except by asking MITx Online. + +The one consumer today is ol-analytics-api, which gates its B2B analytics +endpoints on org-manager status. It cannot reuse +ManagerOrganizationViewSet: that viewset scopes its queryset to +`self.request.user`, so a service-authenticated call would always answer +"manages nothing". Nor can it forward the end user's identity -- the APISIX +openid-connect plugin strips client-supplied X-Userinfo/X-Access-Token +headers before they reach an upstream, by design, so a forwarded identity +never survives the gateway. + +Hence a service credential plus an explicit subject parameter. Kept in its +own module, off the user-facing manager surface, so that the deletion in +mitodl/hq#10594 is a file removal rather than an unpicking of a live +authorization path. +""" + +import uuid + +from drf_spectacular.utils import OpenApiParameter, OpenApiTypes, extend_schema +from oauth2_provider.contrib.rest_framework import OAuth2Authentication, TokenHasScope +from rest_framework import status +from rest_framework.response import Response +from rest_framework.views import APIView + +from b2b.models import OrganizationPage, is_organization_manager +from b2b.serializers.v0.service import ( + OrganizationManagerCheckSerializer, + ServiceDetailErrorSerializer, +) +from users.models import User + +# Distinct from the user-facing scopes in OAUTH2_PROVIDER["SCOPES"]: this one +# is only ever granted to a service Application, never to a user-facing client. +MANAGER_CHECK_SCOPE = "b2b:manager-check" + + +class OrganizationManagerCheckView(APIView): + """Answer whether a given user manages a given organization. + + Deliberately fails closed: an unknown user, an unknown organization, or a + user with no membership row all return `is_manager: false` rather than a + 404. A 404 would let a caller enumerate which Keycloak organization UUIDs + and user IDs exist here, which is more than this endpoint needs to reveal + to answer its one question. + """ + + authentication_classes = [OAuth2Authentication] + permission_classes = [TokenHasScope] + required_scopes = [MANAGER_CHECK_SCOPE] + + @extend_schema( + operation_id="b2b_service_organization_manager_check", + description=( + "Check whether a user is a manager of an organization. " + "Service-to-service only; requires the " + f"`{MANAGER_CHECK_SCOPE}` scope." + ), + parameters=[ + OpenApiParameter( + name="sso_organization_id", + type=OpenApiTypes.UUID, + location=OpenApiParameter.QUERY, + required=True, + description=( + "The organization's Keycloak UUID " + "(OrganizationPage.sso_organization_id)." + ), + ), + OpenApiParameter( + name="user_global_id", + type=OpenApiTypes.STR, + location=OpenApiParameter.QUERY, + required=True, + description="The user's Keycloak subject (User.global_id).", + ), + ], + responses={ + 200: OrganizationManagerCheckSerializer, + 400: ServiceDetailErrorSerializer, + }, + ) + def get(self, request, *args, **kwargs): # noqa: ARG002 + """Return the manager status for the (user, organization) pair.""" + + sso_organization_id = request.query_params.get("sso_organization_id") + user_global_id = request.query_params.get("user_global_id") + + missing = [ + name + for name, value in ( + ("sso_organization_id", sso_organization_id), + ("user_global_id", user_global_id), + ) + if not value + ] + if missing: + return Response( + { + "detail": f"Missing required query parameter(s): {', '.join(missing)}" + }, + status=status.HTTP_400_BAD_REQUEST, + ) + + # sso_organization_id maps to a UUIDField. Rejecting a malformed value + # here keeps a bad request a 400 rather than letting the ORM raise on + # the lookup and turn it into a 500. + try: + uuid.UUID(sso_organization_id) + except ValueError: + return Response( + {"detail": "sso_organization_id must be a UUID"}, + status=status.HTTP_400_BAD_REQUEST, + ) + + organization = OrganizationPage.objects.filter( + sso_organization_id=sso_organization_id + ).first() + user = User.objects.filter(global_id=user_global_id).first() + + is_manager = bool( + organization and user and is_organization_manager(user, organization.id) + ) + + return Response( + OrganizationManagerCheckSerializer({"is_manager": is_manager}).data, + status=status.HTTP_200_OK, + ) diff --git a/b2b/views/v0/service_test.py b/b2b/views/v0/service_test.py new file mode 100644 index 0000000000..8327288718 --- /dev/null +++ b/b2b/views/v0/service_test.py @@ -0,0 +1,267 @@ +"""Tests for the service-to-service B2B views. + +TEMPORARY -- delete alongside b2b/views/v0/service.py when org-manager status +becomes visible in Keycloak (mitodl/hq#10594). +""" + +import uuid +from datetime import timedelta + +import pytest +from django.urls import reverse +from mitol.common.utils.datetime import now_in_utc +from oauth2_provider.models import AccessToken, Application, get_application_model +from rest_framework import status +from rest_framework.test import APIClient + +from b2b.factories import OrganizationPageFactory +from b2b.models import UserOrganization +from b2b.views.v0.service import MANAGER_CHECK_SCOPE +from users.factories import UserFactory + +pytestmark = [pytest.mark.django_db] + + +def generate_token(): + """Return a unique opaque token value.""" + + return uuid.uuid4().hex + + +@pytest.fixture +def api_client(): + """Unauthenticated API client.""" + + return APIClient() + + +@pytest.fixture +def url(): + """The org-manager-check endpoint URL.""" + + return reverse("b2b:service-organization-manager-check") + + +@pytest.fixture +def service_application(): + """A confidential client-credentials Application, as a service would use.""" + + return get_application_model().objects.create( + name="ol-analytics-api", + client_type=Application.CLIENT_CONFIDENTIAL, + authorization_grant_type=Application.GRANT_CLIENT_CREDENTIALS, + ) + + +def _token(application, scope): + """Mint an access token for the application with the given scope.""" + + return AccessToken.objects.create( + user=None, + application=application, + token=generate_token(), + scope=scope, + expires=now_in_utc() + timedelta(hours=1), + ) + + +@pytest.fixture +def scoped_token(service_application): + """A token carrying the manager-check scope.""" + + return _token(service_application, MANAGER_CHECK_SCOPE) + + +@pytest.fixture +def org(): + """An organization with a known sso_organization_id.""" + + return OrganizationPageFactory.create(sso_organization_id=uuid.uuid4()) + + +def _auth(client, token): + """Attach a bearer token to the client.""" + + client.credentials(HTTP_AUTHORIZATION=f"Bearer {token.token}") + return client + + +def test_manager_returns_true(api_client, url, scoped_token, org): + """A user with is_manager=True on the org is reported as a manager.""" + + user = UserFactory.create() + UserOrganization.objects.create(user=user, organization=org, is_manager=True) + + response = _auth(api_client, scoped_token).get( + url, + { + "sso_organization_id": str(org.sso_organization_id), + "user_global_id": user.global_id, + }, + ) + + assert response.status_code == status.HTTP_200_OK + assert response.json() == {"is_manager": True} + + +def test_member_but_not_manager_returns_false(api_client, url, scoped_token, org): + """Plain membership is not enough -- is_manager must be set. + + This is the whole reason the endpoint exists: the Keycloak token carries + membership but not the manager flag. + """ + + user = UserFactory.create() + UserOrganization.objects.create(user=user, organization=org, is_manager=False) + + response = _auth(api_client, scoped_token).get( + url, + { + "sso_organization_id": str(org.sso_organization_id), + "user_global_id": user.global_id, + }, + ) + + assert response.status_code == status.HTTP_200_OK + assert response.json() == {"is_manager": False} + + +def test_manager_of_a_different_org_returns_false(api_client, url, scoped_token, org): + """Managing one org must not confer manager status on another.""" + + other_org = OrganizationPageFactory.create(sso_organization_id=uuid.uuid4()) + user = UserFactory.create() + UserOrganization.objects.create(user=user, organization=other_org, is_manager=True) + + response = _auth(api_client, scoped_token).get( + url, + { + "sso_organization_id": str(org.sso_organization_id), + "user_global_id": user.global_id, + }, + ) + + assert response.status_code == status.HTTP_200_OK + assert response.json() == {"is_manager": False} + + +@pytest.mark.parametrize( + "unknown", + [("user",), ("org",), ("user", "org")], + ids=["unknown-user", "unknown-org", "both-unknown"], +) +def test_unknown_subject_fails_closed(api_client, url, scoped_token, org, unknown): + """An unknown user or org answers false rather than 404. + + Fails closed, and avoids letting a caller enumerate which org UUIDs and + user IDs exist here. + """ + + user = UserFactory.create() + UserOrganization.objects.create(user=user, organization=org, is_manager=True) + + response = _auth(api_client, scoped_token).get( + url, + { + "sso_organization_id": ( + str(uuid.uuid4()) if "org" in unknown else str(org.sso_organization_id) + ), + "user_global_id": ( + "no-such-global-id" if "user" in unknown else user.global_id + ), + }, + ) + + assert response.status_code == status.HTTP_200_OK + assert response.json() == {"is_manager": False} + + +def test_requires_authentication(api_client, url, org): + """An unauthenticated call is rejected.""" + + response = api_client.get( + url, + { + "sso_organization_id": str(org.sso_organization_id), + "user_global_id": "anything", + }, + ) + + # No authenticator succeeds, so DRF's permission_denied() raises + # NotAuthenticated (401) rather than PermissionDenied (403) -- see + # rest_framework.views.APIView.permission_denied. + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + +def test_requires_the_manager_check_scope(api_client, url, service_application, org): + """A valid token without the scope is rejected. + + The scope is the only thing standing between a service credential and the + ability to ask about any user, so this is the load-bearing check. + """ + + wrong_scope_token = _token(service_application, "user:read") + + response = _auth(api_client, wrong_scope_token).get( + url, + { + "sso_organization_id": str(org.sso_organization_id), + "user_global_id": "anything", + }, + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + + +def test_expired_token_is_rejected(api_client, url, service_application, org): + """An expired token does not authenticate.""" + + expired = AccessToken.objects.create( + user=None, + application=service_application, + token=generate_token(), + scope=MANAGER_CHECK_SCOPE, + expires=now_in_utc() - timedelta(hours=1), + ) + + response = _auth(api_client, expired).get( + url, + { + "sso_organization_id": str(org.sso_organization_id), + "user_global_id": "anything", + }, + ) + + # OAuth2Authentication.authenticate() returns None for an expired token + # (same as no token at all), so this hits the same NotAuthenticated (401) + # path as test_requires_authentication above -- it never reaches + # TokenHasScope, which is what produces 403 in + # test_requires_the_manager_check_scope below. + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + +@pytest.mark.parametrize( + "params", + [ + {}, + {"sso_organization_id": "11111111-1111-1111-1111-111111111111"}, + {"user_global_id": "abc"}, + ], +) +def test_missing_parameters_are_a_400(api_client, url, scoped_token, params): + """Both query parameters are required.""" + + response = _auth(api_client, scoped_token).get(url, params) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + + +def test_malformed_organization_id_is_a_400(api_client, url, scoped_token): + """A non-UUID sso_organization_id is a 400, not a 500 from the ORM.""" + + response = _auth(api_client, scoped_token).get( + url, + {"sso_organization_id": "not-a-uuid", "user_global_id": "abc"}, + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST diff --git a/b2b/views/v0/urls.py b/b2b/views/v0/urls.py index 9453303652..bbd97d16bc 100644 --- a/b2b/views/v0/urls.py +++ b/b2b/views/v0/urls.py @@ -12,6 +12,7 @@ ManagerContractViewSet, ManagerOrganizationViewSet, ) +from b2b.views.v0.service import OrganizationManagerCheckView from main.routers import SimpleRouterWithNesting app_name = "b2b" @@ -51,4 +52,11 @@ AttachContractApi.as_view(), name="attach-user", ), + # Service-to-service; delete along with b2b/views/v0/service.py once + # org-manager status is visible in Keycloak (mitodl/hq#10594). + path( + r"service/organization-manager-check/", + OrganizationManagerCheckView.as_view(), + name="service-organization-manager-check", + ), ] diff --git a/main/settings.py b/main/settings.py index 77116097d6..802face2e7 100644 --- a/main/settings.py +++ b/main/settings.py @@ -1093,6 +1093,10 @@ "write": "Write scope", "openid": "OpenID Connect scope", "user:read": "Can read user and profile data", + # Service-to-service only, never granted to a user-facing client. + # Remove with b2b/views/v0/service.py once org-manager status is + # visible in Keycloak (mitodl/hq#10594). + "b2b:manager-check": "Can check whether a user manages an organization", # "digitalcredentials": "Can read and write Digital Credentials data", # noqa: ERA001 }, "DEFAULT_SCOPES": ["user:read"], diff --git a/openapi/specs/v0.yaml b/openapi/specs/v0.yaml index 930ff7039d..f1b1f09511 100644 --- a/openapi/specs/v0.yaml +++ b/openapi/specs/v0.yaml @@ -848,6 +848,40 @@ paths: schema: $ref: '#/components/schemas/OrganizationPage' description: '' + /api/v0/b2b/service/organization-manager-check/: + get: + operationId: b2b_service_organization_manager_check + description: Check whether a user is a manager of an organization. Service-to-service + only; requires the `b2b:manager-check` scope. + parameters: + - in: query + name: sso_organization_id + schema: + type: string + format: uuid + description: The organization's Keycloak UUID (OrganizationPage.sso_organization_id). + required: true + - in: query + name: user_global_id + schema: + type: string + description: The user's Keycloak subject (User.global_id). + required: true + tags: + - b2b + responses: + '200': + content: + application/json: + schema: + $ref: '#/components/schemas/OrganizationManagerCheck' + description: '' + '400': + content: + application/json: + schema: + $ref: '#/components/schemas/ServiceDetailError' + description: '' /api/v0/baskets/: get: operationId: baskets_list @@ -7447,6 +7481,15 @@ components: required: - state - total_price_paid + OrganizationManagerCheck: + type: object + description: Response shape for the org-manager check. + properties: + is_manager: + type: boolean + description: True if the user manages the organization. + required: + - is_manager OrganizationPage: type: object description: Serializer for the OrganizationPage model. @@ -8787,6 +8830,24 @@ components: minLength: 1 required: - email + ServiceDetailError: + type: object + description: |- + Response shape for the 400 error responses below. + + Same shape as b2b.serializers.v0.manager.DetailErrorSerializer, but + duplicated rather than imported (and distinctly named, since + drf-spectacular keys its component registry on class identity, not + structural equality -- reusing the same name for a different class + produces a "components with identical names" warning and an + unpredictable schema) so this module stays self-contained and its + deletion remains a plain file removal (see the module docstring in + b2b/views/v0/service.py). + properties: + detail: + type: string + required: + - detail SignatoryItem: type: object description: Serializer for signatory items used in certificate pages. diff --git a/openapi/specs/v1.yaml b/openapi/specs/v1.yaml index 1c8dc24f80..eebf36a8a1 100644 --- a/openapi/specs/v1.yaml +++ b/openapi/specs/v1.yaml @@ -848,6 +848,40 @@ paths: schema: $ref: '#/components/schemas/OrganizationPage' description: '' + /api/v0/b2b/service/organization-manager-check/: + get: + operationId: b2b_service_organization_manager_check + description: Check whether a user is a manager of an organization. Service-to-service + only; requires the `b2b:manager-check` scope. + parameters: + - in: query + name: sso_organization_id + schema: + type: string + format: uuid + description: The organization's Keycloak UUID (OrganizationPage.sso_organization_id). + required: true + - in: query + name: user_global_id + schema: + type: string + description: The user's Keycloak subject (User.global_id). + required: true + tags: + - b2b + responses: + '200': + content: + application/json: + schema: + $ref: '#/components/schemas/OrganizationManagerCheck' + description: '' + '400': + content: + application/json: + schema: + $ref: '#/components/schemas/ServiceDetailError' + description: '' /api/v0/baskets/: get: operationId: baskets_list @@ -7447,6 +7481,15 @@ components: required: - state - total_price_paid + OrganizationManagerCheck: + type: object + description: Response shape for the org-manager check. + properties: + is_manager: + type: boolean + description: True if the user manages the organization. + required: + - is_manager OrganizationPage: type: object description: Serializer for the OrganizationPage model. @@ -8787,6 +8830,24 @@ components: minLength: 1 required: - email + ServiceDetailError: + type: object + description: |- + Response shape for the 400 error responses below. + + Same shape as b2b.serializers.v0.manager.DetailErrorSerializer, but + duplicated rather than imported (and distinctly named, since + drf-spectacular keys its component registry on class identity, not + structural equality -- reusing the same name for a different class + produces a "components with identical names" warning and an + unpredictable schema) so this module stays self-contained and its + deletion remains a plain file removal (see the module docstring in + b2b/views/v0/service.py). + properties: + detail: + type: string + required: + - detail SignatoryItem: type: object description: Serializer for signatory items used in certificate pages. diff --git a/openapi/specs/v2.yaml b/openapi/specs/v2.yaml index c9f2a2a7ee..98be06a035 100644 --- a/openapi/specs/v2.yaml +++ b/openapi/specs/v2.yaml @@ -848,6 +848,40 @@ paths: schema: $ref: '#/components/schemas/OrganizationPage' description: '' + /api/v0/b2b/service/organization-manager-check/: + get: + operationId: b2b_service_organization_manager_check + description: Check whether a user is a manager of an organization. Service-to-service + only; requires the `b2b:manager-check` scope. + parameters: + - in: query + name: sso_organization_id + schema: + type: string + format: uuid + description: The organization's Keycloak UUID (OrganizationPage.sso_organization_id). + required: true + - in: query + name: user_global_id + schema: + type: string + description: The user's Keycloak subject (User.global_id). + required: true + tags: + - b2b + responses: + '200': + content: + application/json: + schema: + $ref: '#/components/schemas/OrganizationManagerCheck' + description: '' + '400': + content: + application/json: + schema: + $ref: '#/components/schemas/ServiceDetailError' + description: '' /api/v0/baskets/: get: operationId: baskets_list @@ -7447,6 +7481,15 @@ components: required: - state - total_price_paid + OrganizationManagerCheck: + type: object + description: Response shape for the org-manager check. + properties: + is_manager: + type: boolean + description: True if the user manages the organization. + required: + - is_manager OrganizationPage: type: object description: Serializer for the OrganizationPage model. @@ -8787,6 +8830,24 @@ components: minLength: 1 required: - email + ServiceDetailError: + type: object + description: |- + Response shape for the 400 error responses below. + + Same shape as b2b.serializers.v0.manager.DetailErrorSerializer, but + duplicated rather than imported (and distinctly named, since + drf-spectacular keys its component registry on class identity, not + structural equality -- reusing the same name for a different class + produces a "components with identical names" warning and an + unpredictable schema) so this module stays self-contained and its + deletion remains a plain file removal (see the module docstring in + b2b/views/v0/service.py). + properties: + detail: + type: string + required: + - detail SignatoryItem: type: object description: Serializer for signatory items used in certificate pages. From 02da45ecf8481934e633e5153ec81478a2d19124 Mon Sep 17 00:00:00 2001 From: Muhammad Anas <88967643+Anas12091101@users.noreply.github.com> Date: Tue, 4 Aug 2026 13:03:41 +0500 Subject: [PATCH 6/8] fix: dirty-check edX sync and ProductPage saves to cut Fastly purges (#3806) --- cms/models.py | 6 +++- cms/models_test.py | 15 +++++++++ courses/api.py | 80 ++++++++++++++++++++++++++++++--------------- courses/api_test.py | 43 ++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 27 deletions(-) diff --git a/cms/models.py b/cms/models.py index f72346174c..3493a89906 100644 --- a/cms/models.py +++ b/cms/models.py @@ -1320,7 +1320,11 @@ def save(self, clean=True, user=None, log_action=False, **kwargs): # noqa: FBT0 courseware_object = self.course elif self.is_program_page: courseware_object = self.program - if courseware_object: + # Only push the title down (and save) when it actually changed. An + # unconditional save here fires the courseware object's post_save + # Fastly purge signal on every page save, so publishing one page + # would otherwise emit several redundant purges of the same key. + if courseware_object and courseware_object.title != self.title: courseware_object.title = self.title courseware_object.save() diff --git a/cms/models_test.py b/cms/models_test.py index 3114ca890c..bd2a5341b9 100644 --- a/cms/models_test.py +++ b/cms/models_test.py @@ -664,6 +664,21 @@ def test_courseware_title_synced_with_product_page_title(test_course): assert courseware.title == updated_title +@pytest.mark.parametrize("test_course", [True, False]) +def test_product_page_save_skips_courseware_save_when_title_unchanged(test_course): + """ + Saving a product page without changing its title must not re-save the + linked courseware object, so it fires no redundant Fastly purge signal. + """ + product_page = CoursePageFactory() if test_course else ProgramPageFactory() + courseware = product_page.course if test_course else product_page.program + + with patch.object(type(courseware), "save") as mock_courseware_save: + product_page.save() + + mock_courseware_save.assert_not_called() + + @pytest.mark.parametrize("flex_form_for_course", [True, False]) def test_flexible_pricing_request_form_context(flex_form_for_course): """ diff --git a/courses/api.py b/courses/api.py index 8d3cfd946c..d5f4f221a3 100644 --- a/courses/api.py +++ b/courses/api.py @@ -667,6 +667,52 @@ def _filter_valid_course_keys(runs): return valid_course_keys, runs_by_course_id +def _sync_course_run_from_edx(run, course_detail): + """ + Apply edX course detail values to a CourseRun, saving only when something + actually changed. + + Skipping the save when nothing differs avoids a pointless full-column + UPDATE and, more importantly, the Fastly purge that the run's post_save + signal would otherwise trigger for its parent course on every sync pass. + + Args: + run (CourseRun): the run to update. + course_detail (CourseDetail): the incoming edX course detail. + + Returns: + bool: True if the run was changed and saved, False if it was unchanged. + """ + # Only sync the certificate date if it's set in edX, otherwise fall back + # to the course's end date. + certificate_available_date = ( + course_detail.certificate_available_date or course_detail.end + ) + incoming_values = { + "title": course_detail.name, + "start_date": course_detail.start, + "end_date": course_detail.end, + "enrollment_start": course_detail.enrollment_start, + "enrollment_end": course_detail.enrollment_end, + "is_self_paced": course_detail.is_self_paced(), + "certificate_available_date": certificate_available_date, + } + + if all(getattr(run, field) == value for field, value in incoming_values.items()): + return False + + # Reset the expiration_date so it is calculated automatically and does not + # raise a validation error now that the start or end date has changed. + if run.start_date != course_detail.start or run.end_date != course_detail.end: + run.expiration_date = None + + for field, value in incoming_values.items(): + setattr(run, field, value) + + run.save() + return True + + def sync_course_runs(runs): """ Sync course run dates and title from Open edX using course list API @@ -675,7 +721,10 @@ def sync_course_runs(runs): runs ([CourseRun]): list of CourseRun objects. Returns: - tuple: (success_count, failure_count) - counts of successful and failed syncs + tuple: (success_count, failure_count) where success_count is the number + of runs that had changed edX values and were saved (runs already in sync + are skipped and counted as neither success nor failure), and + failure_count is the number of runs that errored while syncing. """ api_client = get_edx_api_course_list_client() @@ -706,32 +755,11 @@ def sync_course_runs(runs): run = runs_by_course_id[course_detail.course_id] try: - # Reset the expiration_date so it is calculated automatically and - # does not raise a validation error now that the start or end date - # has changed. - if ( - run.start_date != course_detail.start - or run.end_date != course_detail.end - ): - run.expiration_date = None - - run.title = course_detail.name - run.start_date = course_detail.start - run.end_date = course_detail.end - run.enrollment_start = course_detail.enrollment_start - run.enrollment_end = course_detail.enrollment_end - run.is_self_paced = course_detail.is_self_paced() - # Only sync the date if it's set in edX, Otherwise set it to course's end date - if course_detail.certificate_available_date: - run.certificate_available_date = ( - course_detail.certificate_available_date - ) + if _sync_course_run_from_edx(run, course_detail): + success_count += 1 + log.info("Updated course run: %s", run.courseware_id) else: - run.certificate_available_date = course_detail.end - - run.save() - success_count += 1 - log.info("Updated course run: %s", run.courseware_id) + log.debug("No changes for course run: %s", run.courseware_id) except Exception as e: # pylint: disable=broad-except # noqa: BLE001 # Report any validation or otherwise model errors diff --git a/courses/api_test.py b/courses/api_test.py index bd1f977d07..2617ae243a 100644 --- a/courses/api_test.py +++ b/courses/api_test.py @@ -1235,6 +1235,49 @@ def test_sync_course_runs(settings, mocker, mocked_api_response, expect_success) assert failure_count == 1 +@patch("courses.signals.transaction.on_commit", side_effect=lambda callback: callback()) +@patch("cms.tasks.queue_fastly_surrogate_key_purge.delay") +def test_sync_course_runs_skips_unchanged( + mock_purge_delay, mock_on_commit, settings, mocker +): + """ + A run whose edX values match what's already stored is not re-saved, so it + triggers no additional full-column UPDATE and no additional Fastly purge. + """ + settings.OPENEDX_SERVICE_WORKER_API_TOKEN = "mock_api_token" # noqa: S105 + + course_run = CourseRunFactory.create(courseware_id="course-v1:MITx+6.00.1x+3T2015") + course_detail = CourseDetail( + { + "id": "course-v1:MITx+6.00.1x+3T2015", + "start": "2015-09-15T05:00:00Z", + "end": "2015-12-31T05:00:00Z", + "enrollment_start": "2015-09-01T00:00:00Z", + "enrollment_end": None, + "name": "Introduction to Computer Science", + "pacing": "instructor", + } + ) + + mock_course_list = mocker.patch("courses.api.get_edx_api_course_list_client") + mock_course_list.return_value.get_courses.return_value = [course_detail] + + # Ignore any purges enqueued by the factory setup above so we only measure + # purges caused by the sync calls themselves. + mock_purge_delay.reset_mock() + + # First pass writes the edX values and enqueues exactly one purge. + success_count, failure_count = sync_course_runs([course_run]) + assert (success_count, failure_count) == (1, 0) + assert mock_purge_delay.call_count == 1 + + # Second pass with identical edX data is a no-op: no save, no new purge. + mock_purge_delay.reset_mock() + success_count, failure_count = sync_course_runs([course_run]) + assert (success_count, failure_count) == (0, 0) + assert mock_purge_delay.call_count == 0 + + @pytest.mark.parametrize( "mocked_api_response, expect_success", # noqa: PT006 [ From 336ae2896e0dcf8e928f09123109ae7f944039ce Mon Sep 17 00:00:00 2001 From: Carey P Gumaer Date: Tue, 4 Aug 2026 13:41:38 -0400 Subject: [PATCH 7/8] anonymous user handling (#3805) --- config/apisix/apisix.yaml | 8 +- drf_lint_baseline.json | 44 ++-- ecommerce/api.py | 95 +++++++++ ecommerce/api_test.py | 182 ++++++++++++++++ .../migrations/0040_basket_anonymous_id.py | 42 ++++ ecommerce/models.py | 21 +- ecommerce/models_test.py | 30 +++ ecommerce/serializers/__init__.py | 1 + ecommerce/serializers/v0/__init__.py | 1 + ecommerce/tasks.py | 7 + ecommerce/tasks_test.py | 10 + ecommerce/urls.py | 6 + ecommerce/views/legacy/__init__.py | 57 +++-- ecommerce/views/legacy/views_test.py | 195 ++++++++++++++++++ ecommerce/views/v0/__init__.py | 102 +++++---- ecommerce/views/v0/views_test.py | 81 ++++++++ .../public/src/components/OrderSummaryCard.js | 20 +- .../src/components/OrderSummaryCard_test.js | 92 +++++++++ frontend/public/src/containers/App.js | 13 +- frontend/public/src/containers/App_test.js | 7 +- .../src/containers/pages/checkout/CartPage.js | 18 +- .../pages/checkout/CartPage_test.js | 97 +++++++++ .../pages/checkout/OrderReceiptPage.js | 1 + main/middleware.py | 45 ++++ main/middleware_test.py | 86 +++++++- main/settings.py | 11 + openapi/specs/v0.yaml | 12 +- openapi/specs/v1.yaml | 12 +- openapi/specs/v2.yaml | 12 +- 29 files changed, 1189 insertions(+), 119 deletions(-) create mode 100644 ecommerce/migrations/0040_basket_anonymous_id.py create mode 100644 frontend/public/src/components/OrderSummaryCard_test.js create mode 100644 frontend/public/src/containers/pages/checkout/CartPage_test.js diff --git a/config/apisix/apisix.yaml b/config/apisix/apisix.yaml index c4e45c4dbc..36afa1a646 100644 --- a/config/apisix/apisix.yaml +++ b/config/apisix/apisix.yaml @@ -64,8 +64,8 @@ routes: - "/admin/login*" - id: 3 - name: "app-cart" - desc: "Require login for cart so session is established." + name: "app-checkout-anonymous" + desc: "Require login for the anonymous basket claim/checkout endpoint so a session is established." priority: 5 upstream_id: 1 plugins: @@ -88,8 +88,8 @@ routes: set: Content-Security-Policy: frame-ancestors 'self' ${{OPENEDX_API_BASE_URL}} uris: - - "/cart" - - "/cart/" + - "/checkout/anonymous" + - "/checkout/anonymous/" #END diff --git a/drf_lint_baseline.json b/drf_lint_baseline.json index baad7c7f54..95d29a0f41 100644 --- a/drf_lint_baseline.json +++ b/drf_lint_baseline.json @@ -20,11 +20,11 @@ "courses/serializers/v1/programs.py:346:17:ORM001", "courses/serializers/v1/programs.py:368:16:ORM001", "courses/serializers/v2/courses.py:272:17:ORM002", + "courses/serializers/v2/courses.py:337:18:ORM001", "courses/serializers/v2/departments.py:35:40:ORM002", "courses/serializers/v2/departments.py:49:42:ORM002", "courses/serializers/v2/programs.py:385:53:ORM002", "courses/serializers/v2/programs.py:495:12:ORM002", - "courses/serializers/v2/programs.py:596:12:ORM002", "courses/serializers/v3/courses.py:111:14:ORM001", "courses/serializers/v3/courses.py:55:12:ORM002", "courses/serializers/v3/programs.py:55:22:ORM001", @@ -37,14 +37,14 @@ "ecommerce/serializers/__init__.py:326:20:ORM002", "ecommerce/serializers/__init__.py:331:31:ORM002", "ecommerce/serializers/__init__.py:346:31:ORM002", - "ecommerce/serializers/__init__.py:417:24:ORM002", - "ecommerce/serializers/__init__.py:436:12:ORM001", - "ecommerce/serializers/__init__.py:460:22:ORM002", - "ecommerce/serializers/__init__.py:507:22:ORM002", - "ecommerce/serializers/__init__.py:571:20:ORM002", - "ecommerce/serializers/__init__.py:704:22:ORM002", - "ecommerce/serializers/__init__.py:821:28:ORM002", - "ecommerce/serializers/__init__.py:891:28:ORM002", + "ecommerce/serializers/__init__.py:418:24:ORM002", + "ecommerce/serializers/__init__.py:437:12:ORM001", + "ecommerce/serializers/__init__.py:461:22:ORM002", + "ecommerce/serializers/__init__.py:508:22:ORM002", + "ecommerce/serializers/__init__.py:572:20:ORM002", + "ecommerce/serializers/__init__.py:705:22:ORM002", + "ecommerce/serializers/__init__.py:822:28:ORM002", + "ecommerce/serializers/__init__.py:892:28:ORM002", "ecommerce/serializers/v0/__init__.py:279:17:ORM001", "ecommerce/serializers/v0/__init__.py:281:18:ORM001", "ecommerce/serializers/v0/__init__.py:282:18:ORM001", @@ -54,14 +54,14 @@ "ecommerce/serializers/v0/__init__.py:408:20:ORM002", "ecommerce/serializers/v0/__init__.py:414:35:ORM002", "ecommerce/serializers/v0/__init__.py:430:31:ORM002", - "ecommerce/serializers/v0/__init__.py:502:24:ORM002", - "ecommerce/serializers/v0/__init__.py:521:12:ORM001", - "ecommerce/serializers/v0/__init__.py:545:22:ORM002", - "ecommerce/serializers/v0/__init__.py:592:22:ORM002", - "ecommerce/serializers/v0/__init__.py:657:20:ORM002", - "ecommerce/serializers/v0/__init__.py:807:22:ORM002", + "ecommerce/serializers/v0/__init__.py:503:24:ORM002", + "ecommerce/serializers/v0/__init__.py:522:12:ORM001", + "ecommerce/serializers/v0/__init__.py:546:22:ORM002", + "ecommerce/serializers/v0/__init__.py:593:22:ORM002", + "ecommerce/serializers/v0/__init__.py:658:20:ORM002", + "ecommerce/serializers/v0/__init__.py:808:22:ORM002", "ecommerce/serializers/v0/__init__.py:83:28:ORM002", - "ecommerce/serializers/v0/__init__.py:951:28:ORM002", + "ecommerce/serializers/v0/__init__.py:952:28:ORM002", "flexiblepricing/serializers.py:129:38:ORM001", "flexiblepricing/serializers.py:132:34:ORM001", "flexiblepricing/serializers.py:147:34:ORM001", @@ -71,18 +71,6 @@ "flexiblepricing/serializers.py:207:31:ORM001", "flexiblepricing/serializers.py:212:16:ORM001", "flexiblepricing/serializers.py:216:16:ORM001", - "hubspot_sync/serializers.py:170:22:ORM002", - "hubspot_sync/serializers.py:171:22:ORM001", - "hubspot_sync/serializers.py:183:25:ORM002", - "hubspot_sync/serializers.py:186:33:ORM002", - "hubspot_sync/serializers.py:193:21:ORM002", - "hubspot_sync/serializers.py:312:33:ORM001", - "hubspot_sync/serializers.py:323:36:ORM001", - "hubspot_sync/serializers.py:324:33:ORM001", - "hubspot_sync/serializers.py:335:36:ORM001", - "hubspot_sync/serializers.py:64:22:ORM002", - "hubspot_sync/serializers.py:76:31:ORM002", - "hubspot_sync/serializers.py:80:31:ORM002", "users/serializers.py:209:16:ORM001", "users/serializers.py:254:20:ORM001", "users/serializers.py:301:19:ORM001", diff --git a/ecommerce/api.py b/ecommerce/api.py index e89ef1ce01..a37f0c79f2 100644 --- a/ecommerce/api.py +++ b/ecommerce/api.py @@ -2,6 +2,7 @@ import logging import uuid +from datetime import timedelta from decimal import Decimal from urllib.parse import urljoin @@ -451,6 +452,100 @@ def establish_basket(request, *, no_delay=False): return basket +ANONYMOUS_BASKET_SESSION_KEY = "anonymous_basket_id" + + +def get_anonymous_basket_id(request, *, create=False): + """ + Get the anonymous basket id stored in the request's session, minting one + if requested and none exists yet. + + Kwargs: + create (bool): mint and store a new id in the session if one isn't + already present. Only pass True from call sites that are about to + write to the basket - minting an id writes to the session, which + forces a Set-Cookie header and defeats caching for anonymous page + views that don't need one. + """ + anonymous_id = request.session.get(ANONYMOUS_BASKET_SESSION_KEY) + + if anonymous_id is None and create: + anonymous_id = str(uuid.uuid4()) + request.session[ANONYMOUS_BASKET_SESSION_KEY] = anonymous_id + + return anonymous_id + + +def establish_basket_for_request(request, *, for_update=False): + """ + Get or create the basket for the current request, whether the requester + is authenticated or anonymous. + + Kwargs: + for_update (bool): re-fetch the basket with select_for_update() so it's + locked for the remainder of the caller's transaction. Pass True + when the caller is about to mutate basket contents. + """ + if request.user.is_authenticated: + basket = establish_basket(request) + else: + anonymous_id = get_anonymous_basket_id(request, create=True) + basket, _ = Basket.objects.get_or_create(anonymous_id=anonymous_id) + + if for_update: + basket = Basket.objects.select_for_update().get(pk=basket.pk) + + return basket + + +def claim_anonymous_basket(request): + """ + Convert the anonymous basket identified by the current session into a + basket for the now-authenticated request.user. + + If request.user already has a basket, it is discarded in favor of the + anonymous basket - the anonymous basket reflects what was just shown on + the cart page, and merging would silently change the price the user saw. + + Returns the claimed basket, or None if there's no anonymous basket to + claim (e.g. an expired session). + """ + anonymous_id = get_anonymous_basket_id(request, create=False) + if anonymous_id is None: + return None + + with transaction.atomic(): + try: + anon_basket = Basket.objects.select_for_update().get( + anonymous_id=anonymous_id + ) + except Basket.DoesNotExist: + return None + + Basket.objects.filter(user=request.user).exclude(pk=anon_basket.pk).delete() + + anon_basket.user = request.user + anon_basket.anonymous_id = None + anon_basket.save(update_fields=["user", "anonymous_id"]) + + del request.session[ANONYMOUS_BASKET_SESSION_KEY] + apply_user_discounts(request) + + return anon_basket + + +def cull_anonymous_baskets(): + """ + Delete anonymous baskets that haven't been touched in a while (abandoned + carts). A basket's anonymous_id is only reachable via its session cookie, + so once that cookie could plausibly have expired there's no way for a + basket to ever be claimed - it's safe to remove. + """ + cutoff = now_in_utc() - timedelta(seconds=settings.ANONYMOUS_BASKET_CULL_AGE) + + Basket.objects.filter(anonymous_id__isnull=False, updated_on__lt=cutoff).delete() + + def refund_order(*, order_id: int = None, reference_number: str = None, **kwargs): # noqa: RUF013 """ A function that performs refund for a given order id diff --git a/ecommerce/api_test.py b/ecommerce/api_test.py index 93f2a602ea..3da5d270aa 100644 --- a/ecommerce/api_test.py +++ b/ecommerce/api_test.py @@ -1,6 +1,7 @@ """Tests for Ecommerce api""" import random +import uuid from datetime import datetime, timedelta from zoneinfo import ZoneInfo @@ -9,6 +10,7 @@ import reversion from CyberSource.rest import ApiException from django.conf import settings +from django.contrib.auth.models import AnonymousUser from django.contrib.contenttypes.models import ContentType from django.test import RequestFactory from django.urls import reverse @@ -24,13 +26,18 @@ ProgramFactory, ) from ecommerce.api import ( + ANONYMOUS_BASKET_SESSION_KEY, apply_discount_to_basket, check_and_process_pending_orders_for_resolution, check_for_duplicate_discount_redemptions, + claim_anonymous_basket, create_verified_program_course_run_enrollment, create_verified_program_discount, + cull_anonymous_baskets, establish_basket, + establish_basket_for_request, generate_checkout_payload, + get_anonymous_basket_id, get_auto_apply_discounts_for_basket, process_cybersource_payment_response, refund_order, @@ -1231,3 +1238,178 @@ def test_establish_basket_calls_create_user(mocker, no_delay): establish_basket(request) assert not expected_run_mock.called + + +def test_get_anonymous_basket_id_no_create_does_not_write_session(): + """Test that create=False never mints or writes an id into the session""" + request = RequestFactory().get("/") + request.session = {} + + result = get_anonymous_basket_id(request, create=False) + + assert result is None + assert ANONYMOUS_BASKET_SESSION_KEY not in request.session + + +def test_get_anonymous_basket_id_create_mints_and_is_idempotent(): + """Test that create=True mints an id once and reuses it on subsequent calls""" + request = RequestFactory().get("/") + request.session = {} + + first_id = get_anonymous_basket_id(request, create=True) + + assert first_id is not None + assert request.session[ANONYMOUS_BASKET_SESSION_KEY] == first_id + + second_id = get_anonymous_basket_id(request, create=True) + + assert second_id == first_id + + +def test_establish_basket_for_request_authenticated(user): + """Test that an authenticated request dispatches to establish_basket""" + request = RequestFactory().get("/") + request.session = {} + request.user = user + + basket = establish_basket_for_request(request) + + assert basket.user_id == user.id + assert basket.is_anonymous is False + + +def test_establish_basket_for_request_anonymous_creates_basket(): + """Test that an anonymous request creates a basket keyed by the session's anonymous id""" + request = RequestFactory().get("/") + request.session = {} + request.user = AnonymousUser() + + basket = establish_basket_for_request(request) + + assert basket.is_anonymous is True + assert str(basket.anonymous_id) == request.session[ANONYMOUS_BASKET_SESSION_KEY] + + # A second call with the same session should return the same basket + same_basket = establish_basket_for_request(request) + assert same_basket.id == basket.id + + +def test_establish_basket_for_request_for_update_locks_basket(mocker): + """Test that for_update=True re-fetches the basket with select_for_update""" + request = RequestFactory().get("/") + request.session = {} + request.user = AnonymousUser() + + select_for_update_spy = mocker.spy(Basket.objects, "select_for_update") + + basket = establish_basket_for_request(request, for_update=True) + + select_for_update_spy.assert_called_once() + assert basket.is_anonymous is True + + +def test_claim_anonymous_basket_no_session_returns_none(user): + """Test that there's nothing to claim if the session has no anonymous basket id""" + request = RequestFactory().get("/") + request.session = {} + request.user = user + + assert claim_anonymous_basket(request) is None + + +def test_claim_anonymous_basket_missing_basket_row_returns_none(user): + """Test a stale session id (e.g. the basket was culled) returns None rather than raising""" + request = RequestFactory().get("/") + request.session = {ANONYMOUS_BASKET_SESSION_KEY: str(uuid.uuid4())} + request.user = user + + assert claim_anonymous_basket(request) is None + + +def test_claim_anonymous_basket_claims_and_pops_session(user): + """Test the normal claim path: basket is reassigned, anonymous_id cleared, session popped""" + anon_request = RequestFactory().get("/") + anon_request.session = {} + anon_request.user = AnonymousUser() + anon_basket = establish_basket_for_request(anon_request) + + claim_request = RequestFactory().get("/") + claim_request.session = anon_request.session + claim_request.user = user + + claimed = claim_anonymous_basket(claim_request) + + assert claimed.id == anon_basket.id + assert claimed.user_id == user.id + assert claimed.anonymous_id is None + assert ANONYMOUS_BASKET_SESSION_KEY not in claim_request.session + + +def test_claim_anonymous_basket_discards_existing_user_basket(user): + """Test that an existing basket for the user is discarded in favor of the anonymous one""" + existing_basket = Basket.objects.create(user=user) + + anon_request = RequestFactory().get("/") + anon_request.session = {} + anon_request.user = AnonymousUser() + anon_basket = establish_basket_for_request(anon_request) + + claim_request = RequestFactory().get("/") + claim_request.session = anon_request.session + claim_request.user = user + + claimed = claim_anonymous_basket(claim_request) + + assert claimed.id == anon_basket.id + assert not Basket.objects.filter(pk=existing_basket.pk).exists() + + +def test_claim_anonymous_basket_applies_user_discount_after_conversion(user): + """Test that a pre-assigned user discount is applied once the basket is claimed""" + product = ProductFactory.create() + discount = UnlimitedUseDiscountFactory.create() + UserDiscount.objects.create(discount=discount, user=user) + + anon_request = RequestFactory().get("/") + anon_request.session = {} + anon_request.user = AnonymousUser() + anon_basket = establish_basket_for_request(anon_request) + BasketItem.objects.create(basket=anon_basket, product=product) + + assert BasketDiscount.objects.filter(redeemed_basket=anon_basket).count() == 0 + + claim_request = RequestFactory().get("/") + claim_request.session = anon_request.session + claim_request.user = user + + claimed = claim_anonymous_basket(claim_request) + + assert ( + BasketDiscount.objects.filter( + redeemed_basket=claimed, redeemed_discount=discount + ).count() + == 1 + ) + + +def test_cull_anonymous_baskets(settings, user): + """Test that only anonymous baskets older than the cutoff are removed""" + settings.ANONYMOUS_BASKET_CULL_AGE = 100 + + old_anon_basket = Basket.objects.create(anonymous_id=uuid.uuid4()) + Basket.objects.filter(pk=old_anon_basket.pk).update( + updated_on=now_in_utc() - timedelta(seconds=200) + ) + + recent_anon_basket = Basket.objects.create(anonymous_id=uuid.uuid4()) + + old_user_basket = Basket.objects.create(user=user) + Basket.objects.filter(pk=old_user_basket.pk).update( + updated_on=now_in_utc() - timedelta(seconds=200) + ) + + cull_anonymous_baskets() + + assert not Basket.objects.filter(pk=old_anon_basket.pk).exists() + assert Basket.objects.filter(pk=recent_anon_basket.pk).exists() + assert Basket.objects.filter(pk=old_user_basket.pk).exists() diff --git a/ecommerce/migrations/0040_basket_anonymous_id.py b/ecommerce/migrations/0040_basket_anonymous_id.py new file mode 100644 index 0000000000..2c74626e3f --- /dev/null +++ b/ecommerce/migrations/0040_basket_anonymous_id.py @@ -0,0 +1,42 @@ +# Generated by Django 5.2.15 on 2026-07-30 16:37 + +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("ecommerce", "0039_add_b2b_gsheet_index_to_discount"), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.AddField( + model_name="basket", + name="anonymous_id", + field=models.UUIDField(blank=True, db_index=True, null=True, unique=True), + ), + migrations.AlterField( + model_name="basket", + name="user", + field=models.OneToOneField( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="basket", + to=settings.AUTH_USER_MODEL, + ), + ), + migrations.AddConstraint( + model_name="basket", + constraint=models.CheckConstraint( + condition=models.Q( + models.Q(("anonymous_id__isnull", True), ("user__isnull", False)), + models.Q(("anonymous_id__isnull", False), ("user__isnull", True)), + _connector="OR", + ), + name="basket_user_xor_anonymous_id", + ), + ), + ] diff --git a/ecommerce/models.py b/ecommerce/models.py index b6abb1c310..7e2b32926d 100644 --- a/ecommerce/models.py +++ b/ecommerce/models.py @@ -116,7 +116,26 @@ def __str__(self): class Basket(TimestampedModel): """Represents a User's basket.""" - user = models.OneToOneField(User, on_delete=models.CASCADE, related_name="basket") + anonymous_id = models.UUIDField(null=True, blank=True, unique=True, db_index=True) + user = models.OneToOneField( + User, on_delete=models.CASCADE, related_name="basket", null=True, blank=True + ) + + class Meta: + constraints = [ + models.CheckConstraint( + condition=( + models.Q(user__isnull=False, anonymous_id__isnull=True) + | models.Q(user__isnull=True, anonymous_id__isnull=False) + ), + name="basket_user_xor_anonymous_id", + ) + ] + + @property + def is_anonymous(self): + """Return True if this basket belongs to an anonymous (unauthenticated) user.""" + return self.user_id is None def has_user_blocked_products(self, user): """Return true if any of the courses in the basket block user's country""" diff --git a/ecommerce/models_test.py b/ecommerce/models_test.py index 37b412e8a3..413854253d 100644 --- a/ecommerce/models_test.py +++ b/ecommerce/models_test.py @@ -1,4 +1,5 @@ import random +import uuid from datetime import timedelta from decimal import Decimal @@ -265,6 +266,35 @@ def test_basket_order_equivalency(user, basket, unlimited_discount): assert basket.compare_to_order(order) is False +def test_basket_is_anonymous(): + """Test that is_anonymous reflects whether the basket has a user or an anonymous_id""" + user_basket = BasketFactory.create() + anonymous_basket = BasketFactory.create(user=None, anonymous_id=uuid.uuid4()) + + assert user_basket.is_anonymous is False + assert anonymous_basket.is_anonymous is True + + +def test_basket_requires_exactly_one_of_user_or_anonymous_id(): + """Test the CheckConstraint rejects baskets with both or neither of user/anonymous_id set""" + user = UserFactory.create() + + with pytest.raises(IntegrityError), transaction.atomic(): + Basket.objects.create(user=user, anonymous_id=uuid.uuid4()) + + with pytest.raises(IntegrityError), transaction.atomic(): + Basket.objects.create(user=None, anonymous_id=None) + + +def test_compare_to_order_anonymous_basket(user): + """Test that an anonymous basket never compares equal to an order""" + anonymous_basket = BasketFactory.create(user=None, anonymous_id=uuid.uuid4()) + order = Order(purchaser=user, state=OrderStatus.FULFILLED, total_price_paid=10) + order.save() + + assert anonymous_basket.compare_to_order(order) is False + + def test_product_delete_protection_inactive(): """Test that deleting product(s) instead de-activates it""" single_product = ProductFactory.create() diff --git a/ecommerce/serializers/__init__.py b/ecommerce/serializers/__init__.py index ede1fed06c..1fcfb157bb 100644 --- a/ecommerce/serializers/__init__.py +++ b/ecommerce/serializers/__init__.py @@ -362,6 +362,7 @@ class Meta: fields = [ "id", "user", + "anonymous_id", "basket_items", "total_price", "discounted_price", diff --git a/ecommerce/serializers/v0/__init__.py b/ecommerce/serializers/v0/__init__.py index dc10628858..8a96f79079 100644 --- a/ecommerce/serializers/v0/__init__.py +++ b/ecommerce/serializers/v0/__init__.py @@ -446,6 +446,7 @@ class Meta: fields = [ "id", "user", + "anonymous_id", "basket_items", "total_price", "discounted_price", diff --git a/ecommerce/tasks.py b/ecommerce/tasks.py index 13902c1f83..5183f97f94 100644 --- a/ecommerce/tasks.py +++ b/ecommerce/tasks.py @@ -60,3 +60,10 @@ def perform_check_for_duplicate_discount_redemptions(): from ecommerce.api import check_for_duplicate_discount_redemptions check_for_duplicate_discount_redemptions() + + +@app.task(acks_late=True) +def perform_cull_anonymous_baskets(): + from ecommerce.api import cull_anonymous_baskets + + cull_anonymous_baskets() diff --git a/ecommerce/tasks_test.py b/ecommerce/tasks_test.py index 0d17cb1971..e814d08641 100644 --- a/ecommerce/tasks_test.py +++ b/ecommerce/tasks_test.py @@ -4,6 +4,7 @@ from ecommerce.factories import ProductFactory from ecommerce.serializers.serializers_test import create_order_receipt from ecommerce.tasks import ( + perform_cull_anonymous_baskets, perform_downgrade_from_order, perform_unenrollment_from_order, ) @@ -15,6 +16,15 @@ def products(): return ProductFactory.create_batch(5) +def test_perform_cull_anonymous_baskets_calls_api(mocker): + """The task should just delegate to the api function""" + mock_cull = mocker.patch("ecommerce.api.cull_anonymous_baskets") + + perform_cull_anonymous_baskets() + + mock_cull.assert_called_once() + + @pytest.mark.skip_nplusone_check def test_delayed_order_receipt_sends_email( # noqa: PLR0913 settings, mocker, user, products, user_client, django_capture_on_commit_callbacks diff --git a/ecommerce/urls.py b/ecommerce/urls.py index b5b08e4053..cbc0e69311 100644 --- a/ecommerce/urls.py +++ b/ecommerce/urls.py @@ -3,6 +3,7 @@ from ecommerce.admin import AdminRefundOrderView from ecommerce.views.legacy import ( AllProductViewSet, + AnonymousCheckoutView, BackofficeCallbackView, BasketDiscountViewSet, BasketItemViewSet, @@ -92,6 +93,11 @@ CheckoutInterstitialView.as_view(), name="checkout_interstitial_page", ), + re_path( + r"^checkout/anonymous/?$", + AnonymousCheckoutView.as_view(), + name="checkout-anonymous", + ), path( "api/orders/receipt//", OrderReceiptView.as_view(), diff --git a/ecommerce/views/legacy/__init__.py b/ecommerce/views/legacy/__init__.py index 2f6b6cc435..03b7eea21e 100644 --- a/ecommerce/views/legacy/__init__.py +++ b/ecommerce/views/legacy/__init__.py @@ -26,7 +26,7 @@ from rest_framework.decorators import action from rest_framework.exceptions import ParseError from rest_framework.generics import ListCreateAPIView, RetrieveAPIView -from rest_framework.permissions import IsAdminUser, IsAuthenticated +from rest_framework.permissions import AllowAny, IsAdminUser, IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView from rest_framework.viewsets import ( @@ -587,6 +587,12 @@ class CheckoutApiViewSet(ViewSet): authentication_classes = (SessionAuthentication, TokenAuthentication) permission_classes = (IsAuthenticated,) + def get_permissions(self): + """Allow anonymous access to everything except discount redemption""" + if self.action == "redeem_discount": + return [IsAuthenticated()] + return [AllowAny()] + @extend_schema( request=RedeemDiscountRequestSerializer, responses={200: RedeemDiscountResponseSerializer}, @@ -687,9 +693,7 @@ def redeem_discount(self, request): def add_to_cart(self, request): """Add product to the cart""" with transaction.atomic(): - basket, _ = Basket.objects.select_for_update().get_or_create( - user=self.request.user - ) + basket = api.establish_basket_for_request(request, for_update=True) # Check if multiple cart items feature is enabled allow_multiple_items = getattr( @@ -766,9 +770,17 @@ def cart(self, request): """ Returns the current cart, with the product info embedded. """ - try: - basket = Basket.objects.filter(user=request.user).get() - except ObjectDoesNotExist: + if request.user.is_authenticated: + basket = Basket.objects.filter(user=request.user).first() + else: + anonymous_id = api.get_anonymous_basket_id(request, create=False) + basket = ( + Basket.objects.filter(anonymous_id=anonymous_id).first() + if anonymous_id + else None + ) + + if basket is None: return Response("No basket", status=status.HTTP_406_NOT_ACCEPTABLE) if not basket.get_products(): @@ -776,7 +788,8 @@ def cart(self, request): "No product in basket", status=status.HTTP_406_NOT_ACCEPTABLE ) - api.apply_user_discounts(request) + if request.user.is_authenticated: + api.apply_user_discounts(request) return Response(BasketWithProductSerializer(basket).data) @@ -787,9 +800,17 @@ def cart(self, request): url_name="basket_items_count", ) def basket_items_count(self, request): - basket, _ = Basket.objects.get_or_create(user=request.user) + if request.user.is_authenticated: + basket, _ = Basket.objects.get_or_create(user=request.user) + else: + anonymous_id = api.get_anonymous_basket_id(request, create=False) + basket = ( + Basket.objects.filter(anonymous_id=anonymous_id).first() + if anonymous_id + else None + ) - return Response(basket.basket_items.count()) + return Response(basket.basket_items.count() if basket else 0) @method_decorator(csrf_exempt, name="dispatch") @@ -955,7 +976,7 @@ def post(self, request, *args, **kwargs): # noqa: ARG002 return Response(status=status.HTTP_200_OK) -class CheckoutProductView(LoginRequiredMixin, RedirectView): +class CheckoutProductView(RedirectView): """View to add products to the cart and proceed to the checkout page""" pattern_name = "cart" @@ -963,9 +984,7 @@ class CheckoutProductView(LoginRequiredMixin, RedirectView): def get_redirect_url(self, *args, **kwargs): """Populate the basket before redirecting""" with transaction.atomic(): - basket, _ = Basket.objects.select_for_update().get_or_create( - user=self.request.user - ) + basket = api.establish_basket_for_request(self.request, for_update=True) basket.basket_items.all().delete() BasketDiscount.objects.filter(redeemed_basket=basket).delete() @@ -999,6 +1018,16 @@ def get_redirect_url(self, *args, **kwargs): return super().get_redirect_url(*args, **kwargs) +class AnonymousCheckoutView(LoginRequiredMixin, RedirectView): + """Claim the anonymous basket for the now-authenticated user, then proceed to checkout""" + + pattern_name = "checkout_interstitial_page" + + def get_redirect_url(self, *args, **kwargs): + api.claim_anonymous_basket(self.request) + return super().get_redirect_url(*args, **kwargs) + + class CheckoutInterstitialView(LoginRequiredMixin, TemplateView): template_name = "checkout_interstitial.html" diff --git a/ecommerce/views/legacy/views_test.py b/ecommerce/views/legacy/views_test.py index 4c3b1e6a84..2053083fea 100644 --- a/ecommerce/views/legacy/views_test.py +++ b/ecommerce/views/legacy/views_test.py @@ -1,17 +1,20 @@ import operator as op import random +import uuid from datetime import datetime, timedelta from zoneinfo import ZoneInfo import freezegun import pytest import reversion +from django.conf import settings from django.forms.models import model_to_dict from django.test import Client, RequestFactory from django.urls import reverse from mitol.common.utils.datetime import now_in_utc from mitol.payment_gateway.api import PaymentGateway from rest_framework import status +from rest_framework.test import APIClient from reversion.models import Version from b2b.factories import ContractPageFactory @@ -71,6 +74,21 @@ pytestmark = [pytest.mark.django_db] +def set_anonymous_basket_session(client, anonymous_id): + """ + Seed a test client's session with an anonymous_basket_id. + + SESSION_ENGINE is signed_cookies, so session.save() only updates the + in-memory session_key - it doesn't rewrite the client's cookie jar the + way it would for a server-side session backend. The cookie has to be set + manually or the modified session never reaches the next request. + """ + session = client.session + session["anonymous_basket_id"] = str(anonymous_id) + session.save() + client.cookies[settings.SESSION_COOKIE_NAME] = session.session_key + + @pytest.fixture def products(): with reversion.create_revision(): @@ -990,6 +1008,183 @@ def test_add_to_cart_does_not_trigger_hubspot_for_duplicate_product( mock_sync.assert_not_called() +def test_add_to_cart_anonymous_creates_basket(): + """An anonymous caller can add a product to a new anonymous basket""" + client = APIClient() + product = ProductFactory.create() + + resp = client.post( + reverse("checkout_api-add_to_cart"), + data={"product_id": product.id}, + ) + + assert resp.status_code == status.HTTP_200_OK + + basket = Basket.objects.get(anonymous_id__isnull=False) + assert basket.basket_items.count() == 1 + assert basket.basket_items.first().product == product + assert client.session["anonymous_basket_id"] == str(basket.anonymous_id) + + +@pytest.mark.parametrize( + "cart_exists, cart_empty, expected_status, expected_message", # noqa: PT006 + [ + (False, True, status.HTTP_406_NOT_ACCEPTABLE, "No basket"), + (True, True, status.HTTP_406_NOT_ACCEPTABLE, "No product in basket"), + (True, False, status.HTTP_200_OK, ""), + ], +) +def test_checkout_cart_anonymous( + cart_exists, cart_empty, expected_status, expected_message +): + """Verifies cart/ behaves the same way for anonymous users as for authenticated ones""" + client = APIClient() + + # An authenticated user's basket must never leak to an anonymous caller + other_basket = BasketFactory.create() + BasketItemFactory.create(basket=other_basket) + + basket = None + if cart_exists: + anonymous_id = uuid.uuid4() + basket = Basket.objects.create(anonymous_id=anonymous_id) + set_anonymous_basket_session(client, anonymous_id) + + if basket and not cart_empty: + BasketItemFactory.create(basket=basket) + + resp = client.get(reverse("checkout_api-cart")) + assert resp.status_code == expected_status + + if cart_empty: + assert resp.data == expected_message + else: + assert_drf_json_equal(resp.json(), BasketWithProductSerializer(basket).data) + + +def test_checkout_cart_anonymous_no_session_does_not_leak_other_baskets(): + """An anonymous caller with no session id must never see another user's basket""" + client = APIClient() + + other_basket = BasketFactory.create() + BasketItemFactory.create(basket=other_basket) + + resp = client.get(reverse("checkout_api-cart")) + + assert resp.status_code == status.HTTP_406_NOT_ACCEPTABLE + assert resp.data == "No basket" + + +def test_basket_items_count_authenticated(user, user_drf_client): + """Authenticated basket item count reflects the user's own basket""" + basket = BasketFactory.create(user=user) + BasketItemFactory.create_batch(2, basket=basket) + + resp = user_drf_client.get(reverse("checkout_api-basket_items_count")) + + assert resp.status_code == status.HTTP_200_OK + assert resp.json() == 2 + + +def test_basket_items_count_anonymous_no_session_returns_zero(): + """An anonymous caller with no session yet gets zero, not an error, and no leak""" + client = APIClient() + + other_basket = BasketFactory.create() + BasketItemFactory.create(basket=other_basket) + + resp = client.get(reverse("checkout_api-basket_items_count")) + + assert resp.status_code == status.HTTP_200_OK + assert resp.json() == 0 + + +def test_basket_items_count_anonymous_with_items(): + """An anonymous caller with an established basket gets the real count""" + client = APIClient() + anonymous_id = uuid.uuid4() + basket = Basket.objects.create(anonymous_id=anonymous_id) + BasketItemFactory.create_batch(3, basket=basket) + + set_anonymous_basket_session(client, anonymous_id) + + resp = client.get(reverse("checkout_api-basket_items_count")) + + assert resp.status_code == status.HTTP_200_OK + assert resp.json() == 3 + + +def test_redeem_discount_anonymous_forbidden(): + """Anonymous users cannot redeem discount codes, unlike the other checkout actions""" + client = APIClient() + + resp = client.post( + reverse("checkout_api-redeem_discount"), {"discount": "SOMECODE"} + ) + + assert resp.status_code == status.HTTP_403_FORBIDDEN + + +def test_checkout_product_anonymous(): + """CheckoutProductView is reachable anonymously and populates an anonymous basket""" + client = Client() + product = ProductFactory.create() + + resp = client.get(reverse("checkout-product"), {"product_id": product.id}) + + assert resp.status_code == 302 + assert resp.url == reverse("cart") + + basket = Basket.objects.get(anonymous_id__isnull=False) + assert [item.product for item in basket.basket_items.all()] == [product] + + +def test_anonymous_checkout_view_requires_login(): + """AnonymousCheckoutView is defense-in-depth protected behind LoginRequiredMixin""" + client = Client() + + resp = client.get(reverse("checkout-anonymous")) + + assert resp.status_code == 302 + assert resp.url.startswith(reverse("gateway-login")) + + +def test_anonymous_checkout_view_claims_basket_and_redirects(user): + """ + The anonymous_basket_id must survive the login round trip so that the + basket set up before authentication can be claimed once the user logs in. + """ + client = Client() + product = ProductFactory.create() + + resp = client.post( + reverse("checkout_api-add_to_cart"), data={"product_id": product.id} + ) + assert resp.status_code == status.HTTP_200_OK + + anon_basket = Basket.objects.get(anonymous_id__isnull=False) + session_anon_id = client.session["anonymous_basket_id"] + + # Force login with a non-remote backend: real APISIX-authenticated sessions + # carry a header on every subsequent request that keeps a + # RemoteUserBackend-authenticated session alive, but this test client + # doesn't send that header, so using that backend here would cause + # ApisixUserMiddleware to immediately invalidate the session again. + client.force_login(user, backend="django.contrib.auth.backends.ModelBackend") + + assert client.session["anonymous_basket_id"] == session_anon_id + + resp2 = client.get(reverse("checkout-anonymous")) + + assert resp2.status_code == 302 + assert resp2.url == reverse("checkout_interstitial_page") + + anon_basket.refresh_from_db() + assert anon_basket.user_id == user.id + assert anon_basket.anonymous_id is None + assert "anonymous_basket_id" not in client.session + + def test_discount_rest_api(admin_drf_client, user_drf_client): """ Checks that the admin REST API is only accessible by an admin diff --git a/ecommerce/views/v0/__init__.py b/ecommerce/views/v0/__init__.py index 98c2c28d17..7ecdb11e71 100644 --- a/ecommerce/views/v0/__init__.py +++ b/ecommerce/views/v0/__init__.py @@ -8,6 +8,7 @@ from django.contrib.auth import get_user_model from django.contrib.auth.mixins import LoginRequiredMixin from django.core.exceptions import ObjectDoesNotExist +from django.db import transaction from django.db.models import Count, Q from django.http import Http404 from django.shortcuts import redirect @@ -27,7 +28,7 @@ from rest_framework.exceptions import ParseError from rest_framework.generics import RetrieveAPIView from rest_framework.pagination import LimitOffsetPagination -from rest_framework.permissions import IsAdminUser, IsAuthenticated +from rest_framework.permissions import AllowAny, IsAdminUser, IsAuthenticated from rest_framework.response import Response from rest_framework.viewsets import ModelViewSet, ReadOnlyModelViewSet from rest_framework_extensions.mixins import NestedViewSetMixin @@ -45,6 +46,7 @@ from ecommerce.api import ( apply_discount_to_basket, establish_basket, + establish_basket_for_request, generate_checkout_payload, generate_discount_code, get_auto_apply_discounts_for_basket, @@ -225,7 +227,6 @@ def _create_basket_from_product( Returns: Response: HTTP response """ - basket = establish_basket(request) quantity = request.data.get("quantity", 1) checkout = request.data.get("checkout", False) @@ -236,49 +237,68 @@ def _create_basket_from_product( {"error": "Product not found"}, status=status.HTTP_404_NOT_FOUND ) - # FUTURE: This is where the basket_add hook was called. + with transaction.atomic(): + basket = establish_basket_for_request(request, for_update=True) - (_, created) = BasketItem.objects.update_or_create( - basket=basket, product=product, defaults={"quantity": quantity} - ) + # FUTURE: This is where the basket_add hook was called. - sync_hubspot_cart_add( - request.user, - product, - is_uai=( - is_product_courserun(product) - and is_uai_course_run(product.purchasable_object) + (_, created) = BasketItem.objects.update_or_create( + basket=basket, product=product, defaults={"quantity": quantity} ) - or (is_product_program(product) and is_uai_program(product.purchasable_object)), - ) - - existing_basket_discounts = [bd.redeemed_discount for bd in basket.discounts.all()] - discounts_to_apply = [ - *existing_basket_discounts, - *list(get_auto_apply_discounts_for_basket(basket.id).all()), - ] - - # Clear the discounts that are in the basket - we retained whatever was - # already applied above and will re-apply so the codes get re-checked. (So, - # if a code has now expired, you don't get it anymore.) - BasketDiscount.objects.filter(redeemed_basket=basket).delete() - for discount in discounts_to_apply: - apply_discount_to_basket(basket, discount, allow_finaid=True) + if request.user.is_authenticated: + sync_hubspot_cart_add( + request.user, + product, + is_uai=( + is_product_courserun(product) + and is_uai_course_run(product.purchasable_object) + ) + or ( + is_product_program(product) + and is_uai_program(product.purchasable_object) + ), + ) - # Order matters - apply the code supplied last so we can always attach a - # better-value discount by hand if we want. (Also, turn off finaid flag here.) - if discount_code: - try: - supplied_discount = Discount.objects.get(discount_code=discount_code) - apply_discount_to_basket(basket, supplied_discount) - except Discount.DoesNotExist: - pass + # Discounts (including auto-applied financial assistance) are only + # ever computed against a real user, and shouldn't show up at all for + # a logged-out cart - so this whole step is skipped for anonymous + # baskets rather than run against a basket with no user to check. + existing_basket_discounts = [ + bd.redeemed_discount for bd in basket.discounts.all() + ] + discounts_to_apply = [ + *existing_basket_discounts, + *list(get_auto_apply_discounts_for_basket(basket.id).all()), + ] + + # Clear the discounts that are in the basket - we retained whatever was + # already applied above and will re-apply so the codes get re-checked. (So, + # if a code has now expired, you don't get it anymore.) + BasketDiscount.objects.filter(redeemed_basket=basket).delete() + + for discount in discounts_to_apply: + apply_discount_to_basket(basket, discount, allow_finaid=True) + + # Order matters - apply the code supplied last so we can always attach a + # better-value discount by hand if we want. (Also, turn off finaid flag here.) + if discount_code: + try: + supplied_discount = Discount.objects.get( + discount_code=discount_code + ) + apply_discount_to_basket(basket, supplied_discount) + except Discount.DoesNotExist: + pass basket.refresh_from_db() if checkout: - return redirect("checkout_interstitial_page") + return redirect( + "checkout_interstitial_page" + if request.user.is_authenticated + else "checkout-anonymous" + ) return Response( BasketWithProductSerializer(basket).data, @@ -300,7 +320,7 @@ def _create_basket_from_product( ], ) @api_view(["POST"]) -@permission_classes((IsAuthenticated,)) +@permission_classes((AllowAny,)) def create_basket_from_product(request, product_id: int): """Run _create_basket_from_product.""" @@ -440,7 +460,7 @@ def create_basket_with_products(request): responses={204: OpenApiResponse(description="Basket cleared successfully")}, ) @api_view(["DELETE"]) -@permission_classes([IsAuthenticated]) +@permission_classes([AllowAny]) def clear_basket(request): """ Clear the basket for the current user. @@ -451,9 +471,9 @@ def clear_basket(request): Returns: Response: HTTP response """ - basket = establish_basket(request) - - basket.delete() + with transaction.atomic(): + basket = establish_basket_for_request(request, for_update=True) + basket.delete() return Response(None, status=status.HTTP_204_NO_CONTENT) diff --git a/ecommerce/views/v0/views_test.py b/ecommerce/views/v0/views_test.py index 02f8cd18d3..4091c6a33e 100644 --- a/ecommerce/views/v0/views_test.py +++ b/ecommerce/views/v0/views_test.py @@ -11,6 +11,7 @@ import pytest import reversion from django.forms.models import model_to_dict +from django.test import Client from django.urls import reverse from mitol.common.utils.datetime import now_in_utc from reversion.models import Version @@ -523,6 +524,86 @@ def test_create_basket_with_product( # noqa: PLR0913 ) +# These four tests use transaction=True rather than the default django_db +# marker. The views under test call select_for_update(), which requires an +# active transaction.atomic() block in the view itself - the default +# django_db marker silently masks a missing atomic() block by wrapping the +# whole test in its own outer transaction, so this is the only way to +# actually exercise (and catch regressions in) that requirement. +@pytest.mark.django_db(transaction=True, serialized_rollback=True) +def test_create_basket_from_product_anonymous(mocker): + """ + Test that an anonymous caller can create a basket via create_from_product, + without triggering hubspot sync or picking up auto-applied discounts. + """ + mock_sync = mocker.patch("ecommerce.views.v0.sync_hubspot_cart_add") + product = ProductFactory.create() + UnlimitedUseDiscountFactory.create(automatic=True) + + client = Client() + url = reverse( + "v0:baskets_api-create_from_product", + kwargs={"product_id": product.id}, + ) + + response = client.post(url) + + assert response.status_code == 201 + + basket = Basket.objects.get(id=response.data["id"]) + assert basket.is_anonymous is True + assert basket.basket_items.count() == 1 + assert basket.discounts.count() == 0 + mock_sync.assert_not_called() + + +@pytest.mark.django_db(transaction=True, serialized_rollback=True) +def test_create_basket_from_product_anonymous_checkout_redirect(mocker): + """Test that checkout=True redirects an anonymous caller to the anonymous checkout flow""" + mocker.patch("ecommerce.views.v0.sync_hubspot_cart_add") + product = ProductFactory.create() + + client = Client() + url = reverse( + "v0:baskets_api-create_from_product", + kwargs={"product_id": product.id}, + ) + + response = client.post(url, {"checkout": True}) + + assert response.status_code == 302 + assert response.url == reverse("checkout-anonymous") + + +@pytest.mark.django_db(transaction=True, serialized_rollback=True) +def test_clear_basket_anonymous(): + """Test that an anonymous caller can clear their own anonymous basket""" + client = Client() + product = ProductFactory.create() + + create_url = reverse( + "v0:baskets_api-create_from_product", + kwargs={"product_id": product.id}, + ) + response = client.post(create_url) + basket_id = response.data["id"] + + clear_response = client.delete(reverse("v0:baskets_api-clear_basket")) + + assert clear_response.status_code == 204 + assert not Basket.objects.filter(id=basket_id).exists() + + +@pytest.mark.django_db(transaction=True, serialized_rollback=True) +def test_clear_basket_anonymous_with_no_basket_yet(): + """Test that clearing with no prior basket is a harmless no-op""" + client = Client() + + response = client.delete(reverse("v0:baskets_api-clear_basket")) + + assert response.status_code == 204 + + @pytest.mark.parametrize( ["try_flex_pricing_discount", "try_whitespace"], # noqa: PT006 [ diff --git a/frontend/public/src/components/OrderSummaryCard.js b/frontend/public/src/components/OrderSummaryCard.js index 911e26ea18..50b94215f2 100644 --- a/frontend/public/src/components/OrderSummaryCard.js +++ b/frontend/public/src/components/OrderSummaryCard.js @@ -15,7 +15,8 @@ type Props = { refunds: Array, addDiscount?: Function, discountCode: string, - cardTitle?: string + cardTitle?: string, + isAuthenticated: boolean } type FormValues = { @@ -124,6 +125,12 @@ export class OrderSummaryCard extends React.Component { ) } + getCheckoutUrl() { + return this.props.isAuthenticated ? + "/checkout/to_payment" : + "/checkout/anonymous/" + } + handlePlaceOrder = async () => { const formik = this.formikRef.current const { discounts } = this.props @@ -134,7 +141,7 @@ export class OrderSummaryCard extends React.Component { discounts.length > 0 && (!formik || !formik.values.couponCode || !formik.values.couponCode.trim()) ) { - window.location = "/checkout/to_payment" + window.location = this.getCheckoutUrl() return } @@ -145,7 +152,7 @@ export class OrderSummaryCard extends React.Component { await formik.submitForm() } else { // No coupon code, proceed directly to payment - window.location = "/checkout/to_payment" + window.location = this.getCheckoutUrl() } } @@ -166,7 +173,8 @@ export class OrderSummaryCard extends React.Component { addDiscount, discountCode, cardTitle, - refunds + refunds, + isAuthenticated } = this.props const fmtPrice = formatLocalePrice(totalPrice) @@ -203,7 +211,7 @@ export class OrderSummaryCard extends React.Component { - {!orderFulfilled ? ( + {!orderFulfilled && isAuthenticated ? ( { if (this.state.submittingPlaceOrder) { // Redirect only if there were no errors and this was from Place Order button - window.location = "/checkout/to_payment" + window.location = this.getCheckoutUrl() } } diff --git a/frontend/public/src/components/OrderSummaryCard_test.js b/frontend/public/src/components/OrderSummaryCard_test.js new file mode 100644 index 0000000000..765c342733 --- /dev/null +++ b/frontend/public/src/components/OrderSummaryCard_test.js @@ -0,0 +1,92 @@ +// @flow +import React from "react" +import sinon from "sinon" +import { shallow } from "enzyme" +import { assert } from "chai" + +import { OrderSummaryCard } from "./OrderSummaryCard" +import ApplyCouponForm from "./forms/ApplyCouponForm" + +describe("OrderSummaryCard", () => { + let sandbox + + const baseProps = { + totalPrice: 100, + orderFulfilled: false, + discountedPrice: 100, + discounts: [], + refunds: [], + discountCode: "" + } + + beforeEach(() => { + sandbox = sinon.createSandbox() + }) + + afterEach(() => { + sandbox.restore() + }) + + it("does not render the coupon form when logged out", () => { + const wrapper = shallow( + + ) + assert.isFalse(wrapper.find(ApplyCouponForm).exists()) + }) + + it("renders the coupon form when logged in", () => { + const wrapper = shallow( + + ) + assert.isTrue(wrapper.find(ApplyCouponForm).exists()) + }) + + it("still hides the coupon form when logged in but the order is fulfilled", () => { + const wrapper = shallow( + + ) + assert.isFalse(wrapper.find(ApplyCouponForm).exists()) + }) + + it("returns the anonymous checkout url when logged out", () => { + const wrapper = shallow( + + ) + assert.equal(wrapper.instance().getCheckoutUrl(), "/checkout/anonymous/") + }) + + it("returns the authenticated checkout url when logged in", () => { + const wrapper = shallow( + + ) + assert.equal(wrapper.instance().getCheckoutUrl(), "/checkout/to_payment") + }) + + it("redirects to the anonymous checkout url when placing an order while logged out", async () => { + const originalDescriptor = Object.getOwnPropertyDescriptor( + window, + "location" + ) + Object.defineProperty(window, "location", { + writable: true, + configurable: true, + value: { href: "" } + }) + + try { + const wrapper = shallow( + + ) + await wrapper.instance().handlePlaceOrder() + + assert.equal(window.location, "/checkout/anonymous/") + } finally { + // $FlowFixMe - originalDescriptor is always defined for window.location + Object.defineProperty(window, "location", originalDescriptor) + } + }) +}) diff --git a/frontend/public/src/containers/App.js b/frontend/public/src/containers/App.js index ee946beda5..a172f8221e 100644 --- a/frontend/public/src/containers/App.js +++ b/frontend/public/src/containers/App.js @@ -123,7 +123,7 @@ export class App extends React.Component { !this.isLearnerRecordsPage() && (
)} @@ -214,16 +214,7 @@ const mapDispatchToProps = { addUserNotification } -const mapPropsToConfig = props => { - const queries = [users.currentUserQuery()] - - // Add cart query for authenticated users - if (props.currentUser && props.currentUser.is_authenticated) { - queries.push(cartItemsCountQuery()) - } - - return queries -} +const mapPropsToConfig = () => [users.currentUserQuery(), cartItemsCountQuery()] export default compose( connect(mapStateToProps, mapDispatchToProps), connectRequest(mapPropsToConfig) diff --git a/frontend/public/src/containers/App_test.js b/frontend/public/src/containers/App_test.js index 3bbacce4ab..0c58f2e4fe 100644 --- a/frontend/public/src/containers/App_test.js +++ b/frontend/public/src/containers/App_test.js @@ -98,7 +98,7 @@ describe("Top-level App", () => { sinon.assert.calledOnce(removeStoredUserMessageStub) }) - it("does not call cartItemsCountQuery for unauthenticated users", async () => { + it("calls cartItemsCountQuery for unauthenticated users too", async () => { helper.handleRequestStub.returns(anonymousUser) await renderPage() // Should call /api/users/me to get user data @@ -107,8 +107,9 @@ describe("Top-level App", () => { "/api/v0/users/current_user/", "GET" ) - // Should NOT call the cart items count API for unauthenticated users - sinon.assert.neverCalledWith( + // Should also call the cart items count API for unauthenticated users, + // so the header badge works for anonymous carts + sinon.assert.calledWith( helper.handleRequestStub, "/api/checkout/basket_items_count/", "GET" diff --git a/frontend/public/src/containers/pages/checkout/CartPage.js b/frontend/public/src/containers/pages/checkout/CartPage.js index a55c8ee25d..b97bfbe050 100644 --- a/frontend/public/src/containers/pages/checkout/CartPage.js +++ b/frontend/public/src/containers/pages/checkout/CartPage.js @@ -11,6 +11,7 @@ import { createStructuredSelector } from "reselect" import { pathOr } from "ramda" import type { BasketItem, Discount } from "../../../flow/cartTypes" +import type { CurrentUser } from "../../../flow/authTypes" import Loader from "../../../components/Loader" import { CartItemCard } from "../../../components/CartItemCard" @@ -25,6 +26,7 @@ import { discountSelector, applyDiscountCodeMutation } from "../../../lib/queries/cart" +import { currentUserSelector } from "../../../lib/queries/users" import type { RouterHistory } from "react-router" import { isSuccessResponse } from "../../../lib/util" @@ -39,7 +41,8 @@ type Props = { isLoading: boolean, applyDiscountCode: (code: string) => Promise, addUserNotification: Function, - forceRequest: Function + forceRequest: Function, + currentUser: ?CurrentUser } type CartState = { @@ -98,7 +101,7 @@ export class CartPage extends React.Component { } renderOrderSummaryCard() { - const { totalPrice, discountedPrice, discounts } = this.props + const { totalPrice, discountedPrice, discounts, currentUser } = this.props const refunds = [] return ( @@ -110,12 +113,18 @@ export class CartPage extends React.Component { refunds={refunds} addDiscount={this.addDiscount.bind(this)} discountCode={this.state.discountCode} + isAuthenticated={Boolean(currentUser && currentUser.is_authenticated)} /> ) } renderFinancialAssistanceOffer() { - const { cartItems, discounts } = this.props + const { cartItems, discounts, currentUser } = this.props + + if (!currentUser || !currentUser.is_authenticated) { + return null + } + let userFlexiblePriceExists = false // Check if there are any discounts, and if those discounts are for flexible pricing. if ( @@ -199,7 +208,8 @@ const mapStateToProps = createStructuredSelector({ totalPrice: totalPriceSelector, discountedPrice: discountedPriceSelector, discounts: discountSelector, - isLoading: pathOr(true, ["queries", cartQueryKey, "isPending"]) + isLoading: pathOr(true, ["queries", cartQueryKey, "isPending"]), + currentUser: currentUserSelector }) const mapDispatchToProps = { diff --git a/frontend/public/src/containers/pages/checkout/CartPage_test.js b/frontend/public/src/containers/pages/checkout/CartPage_test.js new file mode 100644 index 0000000000..6b52a84430 --- /dev/null +++ b/frontend/public/src/containers/pages/checkout/CartPage_test.js @@ -0,0 +1,97 @@ +// @flow +import { assert } from "chai" + +import CartPage, { CartPage as InnerCartPage } from "./CartPage" +import IntegrationTestHelper from "../../../util/integration_test_helper" + +describe("CartPage", () => { + let helper, renderPage + + const anonymousUser = { + id: null, + username: "", + email: null, + legal_address: null, + user_profile: null, + is_anonymous: true, + is_authenticated: false, + is_staff: false, + is_superuser: false, + grants: [], + is_active: false + } + + const loggedInUser = { + ...anonymousUser, + id: 1, + username: "test", + email: "test@example.com", + is_anonymous: false, + is_authenticated: true, + is_active: true + } + + const cartItem = { + product: { + id: 1, + price: "100.00", + description: "test product", + purchasable_object: { + course: { + page: { + financial_assistance_form_url: "https://example.com/fa" + } + } + } + } + } + + beforeEach(() => { + helper = new IntegrationTestHelper() + + renderPage = helper.configureShallowRenderer(CartPage, InnerCartPage, { + entities: { + cartItems: [cartItem], + totalPrice: 100, + discountedPrice: 100, + discounts: [], + currentUser: loggedInUser + }, + queries: { + cartItems: { + isPending: false + } + } + }) + }) + + afterEach(() => { + helper.cleanup() + }) + + it("shows the financial assistance offer link when the user is authenticated", async () => { + const { inner } = await renderPage() + assert.isOk(inner.instance().renderFinancialAssistanceOffer()) + }) + + it("suppresses the financial assistance offer link when the user is logged out", async () => { + const { inner } = await renderPage({ + entities: { currentUser: anonymousUser } + }) + assert.isNull(inner.instance().renderFinancialAssistanceOffer()) + }) + + it("passes isAuthenticated=true down to OrderSummaryCard when logged in", async () => { + const { inner } = await renderPage() + const summaryCard = inner.find("OrderSummaryCard") + assert.isTrue(summaryCard.prop("isAuthenticated")) + }) + + it("passes isAuthenticated=false down to OrderSummaryCard when logged out", async () => { + const { inner } = await renderPage({ + entities: { currentUser: anonymousUser } + }) + const summaryCard = inner.find("OrderSummaryCard") + assert.isFalse(summaryCard.prop("isAuthenticated")) + }) +}) diff --git a/frontend/public/src/containers/pages/checkout/OrderReceiptPage.js b/frontend/public/src/containers/pages/checkout/OrderReceiptPage.js index 5b1bdfdfb4..66361d101f 100644 --- a/frontend/public/src/containers/pages/checkout/OrderReceiptPage.js +++ b/frontend/public/src/containers/pages/checkout/OrderReceiptPage.js @@ -68,6 +68,7 @@ export class OrderReceiptPage extends React.Component { refunds={orderReceipt.refunds} cardTitle={`Order Number: ${orderReceipt.reference_number} `} discountCode="" + isAuthenticated={true} /> ) : null } diff --git a/main/middleware.py b/main/middleware.py index b7acc4a52a..43879b2e45 100644 --- a/main/middleware.py +++ b/main/middleware.py @@ -1,14 +1,18 @@ """Common mitx_online middleware""" import logging +import uuid from urllib.parse import urlparse from django.conf import settings +from django.http import HttpResponseRedirect from django.middleware.csrf import CsrfViewMiddleware from django.utils.deprecation import MiddlewareMixin log = logging.getLogger(__name__) +ANONYMOUS_BASKET_HANDOFF_PARAM = "anonymous_basket_id" + class CachelessAPIMiddleware(MiddlewareMixin): """Add Cache-Control header to API responses""" @@ -25,6 +29,47 @@ def process_response(self, request, response): return response +class AnonymousBasketHandoffMiddleware(MiddlewareMixin): + """ + Adopt an anonymous_basket_id passed as a query parameter into this + request's own session, then redirect to the same URL with the parameter + stripped. + + An anonymous basket's session cookie is host-only, and MIT's shared + mit.edu domain can't be used to widen it (institution-wide cookie size + limits). Learn's frontend proxies basket API calls through a different + subdomain than the one that serves mitxonline's own pages, so the + cookie set during those API calls never reaches this domain on its own - + the id has to be handed off explicitly through the URL instead. + """ + + def process_request(self, request): + basket_id = request.GET.get(ANONYMOUS_BASKET_HANDOFF_PARAM) + if not basket_id: + return None + + if not request.user.is_authenticated and not request.session.get( + "anonymous_basket_id" + ): + try: + uuid.UUID(basket_id) + except ValueError: + log.warning( + "Ignoring malformed anonymous_basket_id query param: %s", + basket_id, + ) + else: + request.session["anonymous_basket_id"] = basket_id + + query_params = request.GET.copy() + del query_params[ANONYMOUS_BASKET_HANDOFF_PARAM] + redirect_url = request.path + if query_params: + redirect_url = f"{redirect_url}?{query_params.urlencode()}" + + return HttpResponseRedirect(redirect_url) + + class HostBasedCSRFMiddleware(CsrfViewMiddleware): """ CSRF middleware that changes the response cookie's domain property diff --git a/main/middleware_test.py b/main/middleware_test.py index b4c1799b39..a265297056 100644 --- a/main/middleware_test.py +++ b/main/middleware_test.py @@ -1,7 +1,13 @@ +import uuid + import pytest +from django.contrib.auth.models import AnonymousUser from django.http import HttpResponse -from main.middleware import HostBasedCSRFMiddleware +from main.middleware import AnonymousBasketHandoffMiddleware, HostBasedCSRFMiddleware +from users.factories import UserFactory + +pytestmark = [pytest.mark.django_db] @pytest.mark.parametrize( @@ -54,6 +60,84 @@ def test_host_based_csrf_middleware(mocker, rf, settings, host, expected_domain) ) +def test_anonymous_basket_handoff_no_param_is_a_noop(mocker, rf): + """Test that a request with no handoff param passes straight through""" + get_response = mocker.MagicMock() + middleware = AnonymousBasketHandoffMiddleware(get_response) + + request = rf.get("/cart/") + request.session = {} + request.user = AnonymousUser() + + assert middleware.process_request(request) is None + + +def test_anonymous_basket_handoff_adopts_valid_id(mocker, rf): + """Test that a valid handoff id is adopted into the session and the param is stripped""" + get_response = mocker.MagicMock() + middleware = AnonymousBasketHandoffMiddleware(get_response) + + anon_id = str(uuid.uuid4()) + request = rf.get(f"/cart/?anonymous_basket_id={anon_id}&other=1") + request.session = {} + request.user = AnonymousUser() + + response = middleware.process_request(request) + + assert response.status_code == 302 + assert response.url == "/cart/?other=1" + assert request.session["anonymous_basket_id"] == anon_id + + +def test_anonymous_basket_handoff_ignores_malformed_id(mocker, rf): + """Test that a malformed id is not stored, but the param is still stripped""" + get_response = mocker.MagicMock() + middleware = AnonymousBasketHandoffMiddleware(get_response) + + request = rf.get("/cart/?anonymous_basket_id=not-a-uuid") + request.session = {} + request.user = AnonymousUser() + + response = middleware.process_request(request) + + assert response.status_code == 302 + assert response.url == "/cart/" + assert "anonymous_basket_id" not in request.session + + +def test_anonymous_basket_handoff_does_not_overwrite_existing_session(mocker, rf): + """Test that an id already established in this session takes precedence""" + get_response = mocker.MagicMock() + middleware = AnonymousBasketHandoffMiddleware(get_response) + + existing_id = str(uuid.uuid4()) + incoming_id = str(uuid.uuid4()) + request = rf.get(f"/cart/?anonymous_basket_id={incoming_id}") + request.session = {"anonymous_basket_id": existing_id} + request.user = AnonymousUser() + + response = middleware.process_request(request) + + assert response.status_code == 302 + assert request.session["anonymous_basket_id"] == existing_id + + +def test_anonymous_basket_handoff_skips_session_write_when_authenticated(mocker, rf): + """Test that an authenticated request never has its session mutated by this middleware""" + get_response = mocker.MagicMock() + middleware = AnonymousBasketHandoffMiddleware(get_response) + + anon_id = str(uuid.uuid4()) + request = rf.get(f"/cart/?anonymous_basket_id={anon_id}") + request.session = {} + request.user = UserFactory.create() + + response = middleware.process_request(request) + + assert response.status_code == 302 + assert "anonymous_basket_id" not in request.session + + def test_host_based_csrf_middleware_no_referer(mocker, rf, settings): """Test that middleware handles missing referer header gracefully.""" settings.CSRF_COOKIE_NAME = "csrf_mitxonline" diff --git a/main/settings.py b/main/settings.py index 802face2e7..25d937ea67 100644 --- a/main/settings.py +++ b/main/settings.py @@ -304,6 +304,7 @@ "django.middleware.common.CommonMiddleware", "django.contrib.auth.middleware.AuthenticationMiddleware", "mitol.apigateway.middleware.ApisixUserMiddleware", + "main.middleware.AnonymousBasketHandoffMiddleware", "django.middleware.csrf.CsrfViewMiddleware", "main.middleware.HostBasedCSRFMiddleware", "django.contrib.messages.middleware.MessageMiddleware", @@ -334,6 +335,12 @@ SESSION_ENGINE = "django.contrib.sessions.backends.signed_cookies" +ANONYMOUS_BASKET_CULL_AGE = get_int( + name="ANONYMOUS_BASKET_CULL_AGE", + default=global_settings.SESSION_COOKIE_AGE, + description="Seconds of inactivity after which an anonymous (unclaimed) basket is deleted", +) + MITXONLINE_NEW_USER_LOGIN_URL = get_string( name="MITXONLINE_NEW_USER_LOGIN_URL", default="http://mitxonline.odl.local:8013/create-profile", @@ -1056,6 +1063,10 @@ offset=timedelta(seconds=B2B_GSHEETS_UPDATE_OFFSET), ), }, + "cull-anonymous-baskets": { + "task": "ecommerce.tasks.perform_cull_anonymous_baskets", + "schedule": crontab(minute=0, hour=4), + }, } # django cache back-ends diff --git a/openapi/specs/v0.yaml b/openapi/specs/v0.yaml index f1b1f09511..3ec0d2bfbc 100644 --- a/openapi/specs/v0.yaml +++ b/openapi/specs/v0.yaml @@ -1106,6 +1106,8 @@ paths: description: Clears the basket for the current user. tags: - baskets + security: + - {} responses: '204': description: Basket cleared successfully @@ -1122,6 +1124,8 @@ paths: required: true tags: - baskets + security: + - {} responses: '200': content: @@ -4508,6 +4512,7 @@ components: readOnly: true user: type: integer + nullable: true basket_items: type: array items: @@ -4516,7 +4521,6 @@ components: required: - basket_items - id - - user BasketDiscountDetail: type: object description: BasketDiscount model serializer @@ -4552,6 +4556,11 @@ components: readOnly: true user: type: integer + nullable: true + anonymous_id: + type: string + format: uuid + nullable: true basket_items: type: array items: @@ -4596,7 +4605,6 @@ components: - discounts - id - total_price - - user BlankEnum: enum: - '' diff --git a/openapi/specs/v1.yaml b/openapi/specs/v1.yaml index eebf36a8a1..f1d9fa971e 100644 --- a/openapi/specs/v1.yaml +++ b/openapi/specs/v1.yaml @@ -1106,6 +1106,8 @@ paths: description: Clears the basket for the current user. tags: - baskets + security: + - {} responses: '204': description: Basket cleared successfully @@ -1122,6 +1124,8 @@ paths: required: true tags: - baskets + security: + - {} responses: '200': content: @@ -4508,6 +4512,7 @@ components: readOnly: true user: type: integer + nullable: true basket_items: type: array items: @@ -4516,7 +4521,6 @@ components: required: - basket_items - id - - user BasketDiscountDetail: type: object description: BasketDiscount model serializer @@ -4552,6 +4556,11 @@ components: readOnly: true user: type: integer + nullable: true + anonymous_id: + type: string + format: uuid + nullable: true basket_items: type: array items: @@ -4596,7 +4605,6 @@ components: - discounts - id - total_price - - user BlankEnum: enum: - '' diff --git a/openapi/specs/v2.yaml b/openapi/specs/v2.yaml index 98be06a035..553de49b9e 100644 --- a/openapi/specs/v2.yaml +++ b/openapi/specs/v2.yaml @@ -1106,6 +1106,8 @@ paths: description: Clears the basket for the current user. tags: - baskets + security: + - {} responses: '204': description: Basket cleared successfully @@ -1122,6 +1124,8 @@ paths: required: true tags: - baskets + security: + - {} responses: '200': content: @@ -4508,6 +4512,7 @@ components: readOnly: true user: type: integer + nullable: true basket_items: type: array items: @@ -4516,7 +4521,6 @@ components: required: - basket_items - id - - user BasketDiscountDetail: type: object description: BasketDiscount model serializer @@ -4552,6 +4556,11 @@ components: readOnly: true user: type: integer + nullable: true + anonymous_id: + type: string + format: uuid + nullable: true basket_items: type: array items: @@ -4596,7 +4605,6 @@ components: - discounts - id - total_price - - user BlankEnum: enum: - '' From bd261a0bd7fb03cfdc059e54ac7d3078b1b85979 Mon Sep 17 00:00:00 2001 From: Doof Date: Tue, 4 Aug 2026 17:42:41 +0000 Subject: [PATCH 8/8] Release 1.162.1 --- RELEASE.rst | 11 +++++++++++ main/settings.py | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/RELEASE.rst b/RELEASE.rst index 967e8d52be..3a729c77c4 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,17 @@ Release Notes ============= +Version 1.162.1 +--------------- + +- anonymous user handling (#3805) +- fix: dirty-check edX sync and ProductPage saves to cut Fastly purges (#3806) +- feat(b2b): add a service-scoped org-manager check endpoint (#3807) +- Identify PostHog persons by Keycloak global_id, not Django pk (#3798) +- Use psycopg's C implementation and drop unused psycopg2 (#3801) +- fix: read Learn's Fastly service ID from MIT_LEARN_FASTLY_SERVICE_ID (#3794) +- feat: retire course run cmd added to retire a b2b contract (#3797) + Version 1.161.0 (Released August 03, 2026) --------------- diff --git a/main/settings.py b/main/settings.py index 25d937ea67..06e074735f 100644 --- a/main/settings.py +++ b/main/settings.py @@ -38,7 +38,7 @@ from main.sentry import init_sentry from openapi.settings_spectacular import open_spectacular_settings -VERSION = "1.161.0" +VERSION = "1.162.1" log = logging.getLogger()