| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 96.87500% with 3 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## master #1968 +/- ##
==========================================
+ Coverage 91.55% 92.34% +0.79%
==========================================
Files 354 355 +1
Lines 5824 5852 +28
Branches 1394 1401 +7
==========================================
+ Hits 5332 5404 +72
+ Misses 473 429 -44
Partials 19 19 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
There was a problem hiding this comment.
👍🏼
Sorry, something went wrong.
Replace the `tours` Redux slice with React Query for server state and a
React context for the client UI flags (OEP-0067):
- data/apiHooks.ts: useTourData (useQuery) + useEndCourseHomeTour /
useEndCoursewareTour (useMutation); data/queryKeys.ts keyed by username
- TourContext.tsx: mirrors the former slice's show-flags via useReducer
- ProductTours.jsx / LaunchCourseHomeTourButton.jsx read from the query +
context instead of Redux; TabPage supplies TourProvider
- remove the tours reducer from store.ts and setupTest.js; delete
data/{slice,thunks,index}.js
Preserve the original show/hide behavior: invalidateQueries on the end-tour
mutations keeps the cache in sync with the PATCH, and refetchOnWindowFocus:
false prevents a focus refetch from reopening a tour mid-session.
Update tests that render the tour tree to supply TourProvider and, for the
suites that hand-build their tree with RTL render, a QueryClientProvider.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| Back | FazBrowse Home | New Git URL |
Summary
Converts product tours (the tours Redux slice) from Redux to React Query for server state and a React context for the client UI flags, per OEP-0067 ADR-0010. Part of the Redux → React Query migration (#1946), stacked on the course-recommendations pattern-setter (the branch below).
Behavior is unchanged — the new-user modal, the new-user / existing-user / courseware tours, and the launch button all trigger and dismiss exactly as before. Verified with the full test suite and a live manual smoke pass (details in the decision log).
What changed
Testing
Automated: npm run types, npm run lint, npm run build, and the full npm test suite all pass (102 suites / 888 tests). Unit tests cover TourContext, the query/mutation hooks, and the tour interaction flows.
Manual smoke (dev). Tours are driven by the server-side UserTour row (/api/user_tours/v1/<username>). Set it via Django admin at /admin/user_tours/usertour/ — course_home_tour_status ∈ {show-new-user-tour, show-existing-user-tour, no-tour} and the show_courseware_tour checkbox. Finishing/dismissing a tour PATCHes it back, so re-arm between runs. Keep DevTools → Network filtered to user_tours.
All of the above verified live. Full analysis and results are in the decision log below.
Decisions
Full decision logDecisions — Redux → React Query: product tours
Working notes for this PR (part of the wider Redux → React Query migration,
#1946). Not checked in — referenced when opening the PR. Stacked on the
course-recommendations conversion (#1966).
Target selection
self-contained Redux slice to remove after course recommendations.
Redux mutation, but its bookmarked / bookmarkedUpdateState state lives in
the shared courseware units model and is read by UnitTitleSlot and the
sequence-nav UnitButton, not just BookmarkButton. Converting it now would
be a partial job — the useMutation would still have to dispatch
updateModel into the still-Redux units store to keep those consumers in
sync — coupling it to the courseware model that belongs to a later phase.
Defer until courseware/models.
Redux, so converting them wouldn't remove any Redux (same reason we passed on
preferences-unsubscribe for the read pattern-setter).
(getTourData / patchTourData), read only by ProductTours.jsx and
LaunchCourseHomeTourButton.jsx. It only reads courseHomeMeta (a
not-yet-converted slice — acceptable mid-migration, same as recommendations
reading courseware.courseId) and never writes the shared models store.
Removing it deletes a whole reducer, and it exercises the next patterns we
need in one focused PR: useQuery (tour data), useMutation (patch/dismiss),
and client-state → React context/local state (the UI on/off flags, per
OEP-0067's "client state stays in React").
Stacked PR
React Query scaffolding (the app-level QueryClientProvider, the test-render
wrapper, and the appId constant). PR base is that branch; retarget to
master after refactor: convert course recommendations to React Query #1966 merges, then rebase.
client retry: false; colocated per-feature data/{queryKeys,apiHooks} rooted
at appId; branch on RQ flags; tracking in a colocated track.js.
Conversion plan
The tours slice state is a mix of server data and client UI state, so it
splits three ways (this is the OEP-0067 "server → React Query, client → React
state/context" split in miniature):
Why a context (not just local state)
showNewUserCourseHomeTour is a cross-component signal:
LaunchCourseHomeTourButton (rendered in outline-tab/widgets/CourseTools and
tab-page/TabPage) flips it, and ProductTours (rendered in
tab-page/LoadedTabPage) reacts. Different subtrees → shared client state needs a
context (the same "react context conversion" learner-dashboard did). useQuery
is deduped by key, so both components can call useTourData and share the cached
server result; only the live show/dismiss flags live in the context.
Files
write new contexts as .tsx, api/hooks as .ts) and the direction of travel.
Existing .jsx/.js files we only edit (ProductTours.jsx,
LaunchCourseHomeTourButton.jsx, TabPage.jsx) stay as-is; converting them to
TS is out of scope here.
(useTourData + the two end-tour mutations), and TourContext.tsx at the
feature root — src/product-tours/TourContext.tsx (exports TourProvider +
useTourState; holds the show flags + disableCourseHomeTour,
disableCoursewareTour, closeNewUserCourseHomeModal, launchCourseHomeTour;
seeds itself from the useTourData result).
(src/<feature>/…Context.tsx) rather than learner-dashboard's central
src/data/context/ — consistent with the colocated data-layer choice from
refactor: convert course recommendations to React Query #1966. Placed at the feature root (not a components/ subdir like authn) because
product-tours keeps all its components flat at the feature root; and not
under data/ (it's client state, not a fetch concern).
query/mutation fns; note getTourData already swallows 401/403/404 into
{ toursEnabled: false }, so the query resolves rather than errors).
src/store.ts and setupTest.js's initializeTestStore.
Provider placement (key design point)
TourProvider must wrap both ProductTours and every LaunchCourseHomeTourButton.
Their common ancestor is the tab page — TabPage.jsx renders both the sr-only
button and LoadedTabPage (which renders ProductTours and the tab content,
including the outline CourseTools button).
Decision: wrap TabPage's entire return (<>…</> → <TourProvider>…</TourProvider>),
including HeaderSlot/FooterSlot and the loading/failed branches, rather than
wrapping only the button + LoadedTabPage. The sr-only LaunchCourseHomeTourButton
sits in the ['loaded','denied'] block before HeaderSlot, so the provider has
to open before the header regardless; wrapping the whole return is cleaner than
restructuring the fragment to wrap two non-adjacent children. Header/footer don't
consume the context — harmless.
Client state: faithful slice → context mirror
Chose a faithful 1:1 mirror of the tours slice as TourContext (over the
leaner "minimal context + local state" option) — lowest-risk, mechanical, and
matches learner-dashboard's react-context conversion. TourContext uses
useReducer whose cases mirror the slice reducers exactly (setTourData,
disableCourseHomeTour, disableCoursewareTour, closeNewUserCourseHomeModal,
launchCourseHomeTour), seeded from the useTourData result. ProductTours
keeps its existing local is*Enabled layer unchanged (it just reads the context
flags instead of useSelector(state.tours)).
toursEnabled is read from the query, not the context. Unlike the show*
flags, toursEnabled is pure server data — never mutated by any disable*/launch
action — and it's read by only LaunchCourseHomeTourButton (not ProductTours).
So it stays in React Query rather than being mirrored into TourContext. The
button reads it via useTourData(username, /* enabled */ false) — an
observe-only subscriber that reads whatever ProductTours put in the query cache
(same key) without triggering its own fetch. This preserves the original's fetch
optimization exactly: only ProductTours fetches (on its guarded tabs), so on
non-outline courseHomeMeta tabs (dates/progress, where the sr-only button also
renders) the cache is empty → button hidden, same as today. Rejected having the
button enable its own fetch (would GET tour data on those tabs — a regression,
worsened by our bare client's lack of staleTime).
The original end-tour thunks did two things — persist (patchTourData) and
flip a client flag (dispatch(disable*)). That splits cleanly: patchTourData
persistence is unchanged, now the mutationFn of the end-tour mutations; the
client flag flip becomes a TourContext action. Both are called in the tour's
onEnd (mutate(username) + context.disable*()), rather than coupling the hide
into the mutation's onSuccess (which would force the data hook to consume the
context).
Modal action renamed disable… → closeNewUserCourseHomeModal. The slice
called it disableNewUserCourseHomeModal, but the action only sets
showNewUserCourseHomeModal = false, and that flag is the modal's isOpen. So
it's a transient close, not a permanent disable — the "won't reopen"
persistence is a separate patch (endCourseHomeTour) called alongside it in
onDismiss (and notably not in onStartTour, which just closes the modal to
reveal the tour). close names the actual behavior; disable over-implied
permanence the action doesn't provide. The tour actions
(disableCourseHomeTour/disableCoursewareTour) keep disable for now. See
Tour reappearance: matching Redux persistence below for how refetchOnWindowFocus
and mutation cache-invalidation preserve the original show/hide behavior.
Thunk/action → new mapping
guard matches the original effect guard — see ProductTours readability below.
course_home_tour_status: 'no-tour') + context.disableCourseHomeTour().
show_courseware_tour: false) + context.disableCoursewareTour().
wiring; it just sources the seed flags from context (seeded by the query) and
calls mutations + context actions instead of dispatching thunks.
context.launchCourseHomeTour(). courseId still from the courseHome slice
and org via useModel('courseHomeMeta') — not-yet-converted reads, acceptable.
ProductTours readability (from review)
chain of named booleans. It has three independent reasons to bail — (1) not
authenticated (the endpoint is per-user), (2) not on a tab that has a tour (only
outline/courseware; avoid needless calls), (3) on the outline tab but the
proctoring panel hasn't loaded, so the tour's target widget (weekly-goal) isn't
in the DOM yet. We tried encoding each reason in a variable name and it stayed
unclear ("resolved"/"ready"/"tour tab" didn't convey why), so each return false is a guard clause with a comment stating its reason. Called inline:
useTourData(username, shouldFetchTourData()).
coursewareTabActive/outlineTabActive (the is…Tab prefix read awkwardly);
updated their other uses (the show* → is*Enabled sync effects and the modal
isOpen).
onEnd/onDismiss must persist (mutation) and hide (context) — two coupled
lines. Extracted endCoursewareTour() / endCourseHomeTour() (each =
mutation.mutate(username) + disable*(), closing over the in-scope username
so call sites pass no args) so every call site is one line. Kept two named
functions over a general endTour(type): the two branches share no logic
(different mutation and different disable), so a discriminator would just be a
stringly-typed switch needing a defensive else — worse than two clear names.
LaunchCourseHomeTourButton (from review)
slice was a single global box — ProductTours's thunk wrote state.tours and
this button read the same state.tours, no key involved. React Query's cache is
keyed (tourQueryKeys.user(username)), so to read the entry ProductTours
cached the button must reconstruct the same key, which requires username. Not
new coupling — both components already get username from the same
getAuthenticatedUser(); the per-key cache just makes the address explicit for a
value that is genuinely per-user. This is the general shape of Redux-global →
RQ-per-key: readers name the key in exchange for dedupe / staleness / per-user
isolation.
called it inside handleClick (only administrator, for the track event). The
button now also needs username at render time for the observe-only query, so
we destructure { administrator, username } = getAuthenticatedUser() || {} once
at the top — auth is stable for the render, one call reads cleaner, and it
matches how ProductTours.jsx sources auth.
useModel('courseHomeMeta') for org) — a not-yet-converted slice, acceptable
mid-migration.
Tour reappearance: matching Redux persistence
The tours slice was a single global, persisted store, and that persistence —
not any single line — is what governed when tours/modals reappeared. Redux had one
persistent store; React Query splits that persistence across two layers, and the
seam between them is where behavior can drift. We audited every reappearance path
and reduced it to three discriminators; each fix maps to exactly one.
Layer split. Server-derived flags (showNewUserCourseHomeModal,
showExistingUserCourseHomeTour, showCoursewareTour — all computed by
setTourData from server data) are persisted by the query cache, which (like
the old store) lives above the tab tree and survives SPA navigation. The one
purely-client flag showNewUserCourseHomeTour (set only by launchCourseHomeTour,
cleared only by disableCourseHomeTour) has no server field, so its
persistence depended on the Redux store's in-memory lifetime.
Note: the tour's step position was never persisted anywhere (the slice held
only booleans; position lives inside Paragon's ProductTour and dies on unmount).
So a tour never resumes mid-way in either Redux or RQ — on return it either
re-shows from the start or re-prompts. There is no "resume where you left off."
Discriminator 1 — did the action PATCH the server? If yes (finish/dismiss →
no-tour), the cache must match the patched server; a stale cache that still says
"show" is a bug (tour flashes back on the next remount). If no (start-from-modal
or abandon-mid-tour), the server still says "show," the cache correctly still says
"show," and re-showing on return is the intended re-prompt.
→ Fix: invalidateQueries in the end-tour mutations' onSuccess (the standard
RQ pattern; also the most faithful, since the original re-fetched fresh rather than
hand-patching local state). Keeps the cache in sync with the PATCH the way Redux's
dispatch(disable*) kept the store in sync. onStartTour deliberately fires no
mutation, so the intended re-prompt is untouched.
Discriminator 2 — remount (navigation) or no remount (focus)? On navigation the
component unmounts, local tour state is gone, and a re-derived modal is a clean
prompt. On focus nothing unmounts — the local tour is still running — so a
focus-triggered setTourData pops the modal on top of the live tour. The
original never refetched on focus (its fetch effect keyed on [proctoringPanelStatus]).
→ Fix: refetchOnWindowFocus: false on useTourData.
Discriminator 3 — is the "show" server-backed or client-only? Server-backed
persistence lives in the query cache, survives a page reload (re-fetched), and is
kept. The client-only showNewUserCourseHomeTour lived only in Redux memory: it
survived SPA tab-switches but died on any page reload — an in-memory artifact,
not durable state, and untested/undocumented. Its only observable effect was the
replay button re-launching the tour on every outline visit within one page load,
inconsistent with the server-driven tours (which re-prompt via the modal).
→ Decision: don't replicate it. Per-TabPage TourProvider resets client
flags on navigation, so the replay button is a clean one-shot per visit. We keep
every server-backed "show"; we drop only the in-memory replay-flag's survival
across a tab switch (which never survived a reload anyway).
The ProductTours.test.jsx suite only asserts single-mount behavior (server data →
correct tour on load; launch button → tour) and the 401/403/404 empty cases —
nothing about cross-navigation persistence — so both fixes and the Discriminator-3
drop leave every defined behavior intact.
Tests
Tour data still flows through the existing axios-mock-adapter setup (the same way
the rest of these suites mock course-metadata/outline/proctoring, all still Redux
until Phase 3) — useTourData (RQ) hits the same mocked tourDataUrl, so no
jest.mock of the hook/api is needed. The only structural change is supplying the
tour context.
in TabPage in production (it must — the sr-only LaunchCourseHomeTourButton is
a direct child of TabPage, outside LoadedTabPage, so LoadedTabPage would
be too low). These suites render LoadedTabPage/OutlineTab directly, bypassing
TabPage, so each render is wrapped in <TourProvider> to reproduce that
boundary. Chosen over mocking the tour components (ProductTours,
LaunchCourseHomeTourButton): the wrap is one line, less fragile (a new tour
consumer just works), and keeps the real components in the tree.
wraps its LoadedTabPage render in TourProvider; the Courseware Tour block
needs nothing (it renders CoursewareContainer → the real TabPage → provider).
(→ CourseTools → the launch button) and, in two masquerade-banner tests,
LoadedTabPage; ProgressTab.test.jsx renders LoadedTabPage in four tests.
Those render sites are wrapped in TourProvider (progress tab never fetches —
shouldFetchTourData is false off outline/courseware). Bare <ProgressTab/> /
<OutlineTab/>-only renders with no tour consumer are left alone.
Verify
(only the pre-existing webpack asset-size-limit warnings) · full suite ✅
102 suites / 888 passed / 3 skipped / 0 failed. (Course.test's "displays
learner tools … /previous/" is a pre-existing sequence-nav timing flake under
full-suite load — passes in isolation and on re-run; unrelated to tours.)
tests for the reducer/provider and the query/mutations) and extended
ProductTours.test.jsx with the tour complete/dismiss/abandon flows + the
streak/non-tour-tab guards, to bring patch coverage up to codecov's auto
target. New-user completion advances the checkpoints via a recursive helper
(not a while+await loop) to satisfy no-await-in-loop without a disable;
interactions use userEvent.
setupTest.js; data/{slice,thunks,index}.js deleted.
a query client): OutlineTab/ProgressTab wrap their tab-content renders in
TourProvider; DatesTab/DiscussionTab/CoursewareContainer (which build
their own tree with RTL render) add a QueryClientProvider mirroring
index.jsx.
config. It anchors to #courseware-sequence-navigation, which is rendered by
SequenceNavigation — now behind SequenceNavigationSlot, an empty-by-default
PluginSlot (Sequence.jsx:198). So the anchor isn't in the DOM by default and
the checkpoint has nothing to point at, on master as well. The conversion
targets the same anchor as the original; verified live by injecting the default
SequenceNavigation into the slot via env.config.jsx (keepDefault +
PLUGIN_OPERATIONS.Insert), after which the tour renders and anchors correctly.
skip; ✅ existing-user tour; ✅ launch/replay button; ✅ courseware tour (via slot
injection — see above); ✅ end-tour PATCH + invalidate GET on completion/dismiss;
✅ Fix A — dismiss → navigate away/back → modal does not flash back;
✅ Fix B — window focus mid-tour → no GET, modal does not reopen; ✅ fetch
gate — /v1/<username> GET fires only on Home + Courseware (not Dates/Progress/
Discussion; the discussion_tours endpoint is the unrelated UserDiscussionsTours
feature).
Closes #1974
🤖 Generated with Claude Code