Address review feedback on anonymous checkout - #3735
Open
ChristopherChudzicki wants to merge 4 commits into
Open
Address review feedback on anonymous checkout#3735ChristopherChudzicki wants to merge 4 commits into
ChristopherChudzicki wants to merge 4 commits into
Conversation
ChristopherChudzicki
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Updates anonymous/cc checkout test coverage and MITx Online basket utilities to support anonymous basket handoff and simplify basket-state invalidation behavior.
Changes:
- Simplifies several frontend tests by removing Learn-auth mocking that is no longer needed.
- Adds a MITx Online
basketstest factory and updates tests to use real UUIDs for anonymous baskets and consistent basket shapes. - Changes MITx Online basket hooks to always invalidate
basketStateafter adding/clearing basket items.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontends/main/src/page-components/EnrollmentDialogs/CourseEnrollmentDialog.test.tsx | Removes Learn-auth pre-mocks and narrows api/test-utils imports. |
| frontends/main/src/common/mitxonline/useReplaceBasketItem.test.tsx | Uses shared basket factory + real UUIDs for anonymous basket redirect assertions. |
| frontends/main/src/app-pages/ProductPages/useProgramEnrollment.ts | Removes outdated/duplicated inline comments about anonymous vs free enrollment. |
| frontends/main/src/app-pages/ProductPages/useCourseEnrollment.ts | Removes outdated/duplicated inline comments about anonymous vs free enrollment. |
| frontends/main/src/app-pages/ProductPages/useCourseEnrollment.test.tsx | Uses basket factory; adds assertion that anonymous basket id is propagated via redirect URL. |
| frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.test.tsx | Removes Learn-auth pre-mocks and narrows api/test-utils imports. |
| frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramEnrollmentDisplay.test.tsx | Removes Learn-auth pre-mocks and narrows api/test-utils imports. |
| frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.test.tsx | Removes Learn-auth pre-mocks and narrows api/test-utils imports. |
| frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/OrganizationCards.test.tsx | Adjusts org fixture shape (removes sso_organization_id: null). |
| frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/HomeEnrollmentsDisplay.test.tsx | Removes Learn-auth pre-mocks/imports no longer needed. |
| frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx | Removes Learn-auth pre-mocks and narrows api/test-utils imports. |
| frontends/api/src/mitxonline/test-utils/factories/index.ts | Exposes new baskets factory module. |
| frontends/api/src/mitxonline/test-utils/factories/baskets.ts | Adds basket factory supporting user-owned and anonymous baskets. |
| frontends/api/src/mitxonline/hooks/baskets/index.ts | Always invalidates basketState after add/clear (removes auth gating). |
ChristopherChudzicki
force-pushed
the
cc/anon-checkout-review-fixes
branch
from
August 7, 2026 00:55
8213a58 to
3a9a656
Compare
OpenAPI ChangesNo changes detected Unexpected changes? Ensure your branch is up-to-date with |
ChristopherChudzicki
force-pushed
the
cc/anon-checkout-review-fixes
branch
from
August 7, 2026 13:19
3a9a656 to
73db6eb
Compare
useAddToBasket/useClearBasket gated invalidateQueries on Learn's useUserIsAuthenticated. Two problems: - It reads the wrong session. The gated operation targets MITx Online through a separate APISIX route with its own OIDC cookie, so "logged out on Learn, logged in on MITx Online" would skip an invalidation that should fire. - Gating belongs on the consumer, not the invalidator. invalidateQueries defaults to refetchType: "active", and a query whose observers all set enabled: false is not active, so an anonymous user never refetches anyway. basketState also sets gcTime: 0, so with no observers there is nothing cached to invalidate. The guard could not prevent the 403 it was written for either: a consumer that forgot `enabled` takes one on mount regardless, and suppressing only the refetch turns a loud failure into a quiet stale one. The stated rationale was also wrong -- the checkout-payload endpoint kept IsAuthenticated, so an anonymous call 403s rather than creating an order against a purchaser-less basket. Drops the six beforeEach blocks that existed only to feed the guard a Learn user. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing asserted the feature's actual output. useReplaceBasketItem.test
mocks api/mitxonline-hooks/baskets wholesale so the real hook never ran,
and the one test that did run it fed the hook a response with no
anonymous_id -- so the seam between "read the field off the API
response" and "put it on the redirect URL" was untested.
The fixture it used, { id: 1, items: [] }, was not a BasketWithProduct
either: `items` is not a field (it is basket_items) and four required
ones were missing. setMockResponse.post takes unknown, so TS did not
catch it.
Adds baskets factories alongside the other mitxonline ones: basket() for
a signed-in user, anonymousBasket() for the other case. The two are
mutually exclusive -- MITx Online enforces that with a check constraint,
basket_user_xor_anonymous_id -- so a caller who has to remember both
`user: null` and an `anonymous_id` can get it half right. Two named
constructors plus an XOR invariant cannot: setting both, or neither,
throws where the fixture is built rather than surfacing as a confusing
assertion failure later. The invariant tests for null/undefined rather
than truthiness, so a user id of 0 still counts as a user.
Tests read the generated id back off the factory instead of threading a
uuid through, which also retires the "abc-123" fixture: MITx Online's
handoff middleware parses the value with uuid.UUID() and logs-and-ignores
anything that does not, so the old fixture encoded a value that would
fail in production.
The anonymous case now runs through the real hook from mocked HTTP
response to asserted redirect URL. Verified it fails if cartUrl stops
appending the param.
Drops "does not append anonymous_basket_id when the basket belongs to a
real user". The factory default is anonymous_id: null, so the two
redirect tests above it already feed that exact basket and assert exact
URL equality, which is strictly stronger than asserting the param is
absent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three comment blocks were added to each enrollment hook. The one above the auth guard earns its place -- it explains why the guard has that shape, which is not obvious from the condition. The other two restate the code, and the second restates the guard ten lines above it. Also drops sso_organization_id: null from an OrganizationCards test about logo: undefined, where it is an unrelated override. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The { id: 1, items: [] } stand-in the previous commit replaced appears at
eleven more mock sites across seven enrollment and dashboard test files.
None of them read the response, so nothing was failing -- but nothing
would start failing either if the real shape drifted, since
setMockResponse.post takes unknown and TS never checks these.
Mechanical: no assertions change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChristopherChudzicki
force-pushed
the
cc/anon-checkout-review-fixes
branch
from
August 7, 2026 14:05
73db6eb to
ca35b99
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review follow-ups for #3709, split out so that PR's history stays readable.
Three changes: the basket checkout query is invalidated unconditionally again, the
anonymous_basket_idhandoff finally has a test that runs the real hook, and some comments and test overrides that restated the code are gone. The reasoning is in the commit messages.How can this be tested?
yarn test frontends/main/src/app-pages frontends/main/src/common/mitxonlineuseCourseEnrollment.test.tsx. To confirm it isn't vacuous, delete theurl.searchParams.set("anonymous_basket_id", ...)line inuseReplaceBasketItem.ts— the test fails.anonymous-checkoutflag on and logged out: click a paid CTA on a course page and confirm the redirect to mitxonline/cart/carries?anonymous_basket_id=<uuid>alongsideecom-service=true.