dashboard refactor stage 2#3330
Conversation
OpenAPI Changes1 changes: 0 error, 1 warning, 0 info Unexpected changes? Ensure your branch is up-to-date with |
There was a problem hiding this comment.
Pull request overview
Phase 2 of the dashboard refactor that centralizes the “selected language → enrollment/run to display” logic into the dashboard view-model, then updates both B2C (My Learning / program dashboards) and B2B (contract dashboard) components to use the shared logic.
Changes:
- Added
resolveSlotForLanguageplus dashboard-specific language option helpers todashboardViewModel.ts. - Added unit tests covering the new view-model behavior.
- Replaced duplicated language-selection logic in
EnrollmentDisplay.tsxandContractContent.tsxwith calls into the view model.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/model/dashboardViewModel.ts | Adds shared language option derivation + slot resolution logic (enrollment/run selection) for dashboard cards. |
| frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/model/dashboardViewModel.test.ts | Adds unit tests for the new language option + slot resolution helpers. |
| frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrollmentDisplay.tsx | Uses view-model helpers to compute program-dashboard language options and displayed enrollment/run per course. |
| frontends/main/src/app-pages/DashboardPage/ContractContent.tsx | Uses view-model helpers to compute contract-dashboard language options and displayed enrollment/run per course. |
… in case Intl isn't available.
gumaerc
force-pushed
the
cg/dashboard-refactor-phase-2
branch
from
65597de to
57f8da2
Compare
| return [...baseOptions, ...additionalOptions] | ||
| } | ||
|
|
||
| const getDistinctDashboardLanguageOptions = ( |
There was a problem hiding this comment.
AI's summary of my gut dislike of the implementation of this function:
Heads up — I'd like to swap out getDistinctDashboardLanguageOptions. Not because it's wrong, but because answering "what shows up in the picker?" currently means tracing getDistinctLanguageOptions → getEnrollableLanguageOptions → isLanguageOptionEnrollable in languageOptions.ts, plus a parallel local path via getLanguageOptionFromEnrollment → getNativeLanguageName — four helpers across two files for what's effectively union + dedup + sort.
As I rewrote it a bit, a few things I noticed:
getEnrollableLanguageOptionsseems bogus to me. It matches the langauge run ID againstcourse.courseruns, butcourse.courserunsgenerally does not include the language-specific runs. So it almost always returns true. (I do think it successfully filters out the default language options if they are not enrollable.) I dropped it entirely.- The
enrollment.runthing we discussed
Here's the commit i made for it: 7ba9f8b ... I thought an actual commit / diff would be the easiest way to communicate this.
Goal was:
- much more clearly just union + dedupe + sort
- better names
- a few comments
ChristopherChudzicki
left a comment
ChristopherChudzicki
left a comment
There was a problem hiding this comment.
Take a look at https://github.com/mitodl/mit-learn/pull/3330/changes#r3228712625 ... What do you think?
I also think getDashboardLanguageOptions is useless. The plan doc called it out, but I think it was a mistake. The commit I suggested removes it from the plan, but not from the code.
Can you take a look? I suspect if you accept that commit and remove getDashboardLanguageOptions, a bunch of other code will become dead and we can remove it.
In general, I think we need to look closely moving forward and be careful that each phase of the plan actually makes the dashboard more intelligible / easier to reason about.
The logic in resolveSlotForLanguage is more complex than i would hope, but it is not new logic just moved, and I don't think we should futz with it too much now.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
…Options The flat rewrite of getDistinctDashboardLanguageOptions left the older singular getDashboardLanguageOptions and getDistinctLanguageOptions without production callers. Sweep them and the helpers they kept alive. dashboardViewModel.ts: - delete getDashboardLanguageOptions (singular variant — no production caller; the picker is rendered once per dashboard, not per slot) - delete getLanguageOptionFromEnrollment, getLanguageOptionKeyValue, enrollmentMatchesCourse (only used by the singular variant) - drop getDistinctLanguageOptions from the import block languageOptions.ts: - delete getDistinctLanguageOptions (no callers after the rewrite, just its own tests) - delete getLanguageOptionLabel (only used by getDistinctLanguageOptions) - delete getLanguageCodeFromOptionKey (pure dead code, no callers anywhere — written speculatively, never wired up) - unexport getLanguageOptionKey (still used internally five times, no external consumer) getEnrollableLanguageOptions and isLanguageOptionEnrollable stay; both remain load-bearing through getDefaultLanguageOptionKey and getSelectedLanguageOption, which resolveSlotForLanguage consumes. Full deletion of languageOptions.ts is committed to Phase 7 anyway. Test changes: - delete the singular-variant tests in dashboardViewModel.test.ts; adapt the union and sort assertions to call the plural getDistinctDashboardLanguageOptions with a one-element course list - delete the getDistinctLanguageOptions tests in languageOptions.test.ts - port the three Intl-fallback / memoization tests to call getNativeLanguageName directly instead of going through the now-dead getDistinctLanguageOptions, preserving edge-case coverage at ~10 lines each instead of ~60 All 28 viewModel + languageOptions tests pass; 68 dashboard render tests still pass. Coverage of languageOptions.ts: 78% -> 91%; coverage of dashboardViewModel.ts: 74% -> 96%. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 2 retro surfaced two recurring failure modes that the existing working agreement didn't activate against: - Dead code accumulating because cleanup was deferred to "later" (singular getDashboardLanguageOptions, getLanguageCodeFromOptionKey, getLanguageOptionLabel) - Composing existing helpers without auditing their premise (isLanguageOptionEnrollable filtering against missing data — the courseruns vs language_options invariant) Plan changes (working agreement): - Add "Discover dead code and bad assumptions as you go, not at the boundary" subsection between the success criterion and the phase-boundary review questions. Three inline rules: check for remaining callers when you stop calling something; check whether an existing helper already covers the case before adding one; check the premise of helpers you compose, not just their names. - The rule lives between the during-execution norms and the phase-exit ritual to make discovery part of the moment-to-moment process, not just an end-of-phase review item. languageOptions.ts docstring: - Names the load-bearing data invariant for this file: courseruns and language_options are mostly disjoint; language_options is intent, courseruns only materializes default-language runs. - Stops short of prescribing what to do about it — the fact alone is what was missing during phase 2; readers can apply skepticism themselves. - When languageOptions.ts is absorbed into dashboardViewModel.ts in phase 7, this invariant should travel with the synthesis logic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@gumaerc As discussed yesterday, i pushed an extra two commits. They:
|
1 similar comment
|
@gumaerc As discussed yesterday, i pushed an extra two commits. They:
|
What are the relevant tickets?
Part of https://github.com/mitodl/hq/issues/11211
Description (What does it do?)
This PR executes phase 2 of the dashboard refactoring plan detailed here, also now checked into the repo at
frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/dashboardRefactorPlan.md, to be deleted after refactor is complete.Phase 2 deduplicates some logic in the B2B and B2C components (
ContractContent/EnrollmentDisplay) surrounding multiple steps that are taken to associate an enrollment / course run with language selection. This process was moved into the view model (dashboardViewModel.ts) and the components were updated to use this.How can this be tested?
mainvs this branch: