MPDX-9919 Add geographic location to account preferences - #1978
Conversation
kegrimes
added
the
Preview Environment
|
Preview branch generated at https://mpdx-9919-add-geographic-location-to-preferences.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against 16e571e
|
kegrimes
left a comment
kegrimes
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review
7 specialized agents (Security, Architecture, Data Integrity, Testing, UX, Standards, Financial Reporting) + a dependency-impact analysis reviewed this PR in parallel, standard mode.
Verdict: Approved with suggestions
No hard blockers by the numeric formula, but three things genuinely warrant attention before merge:
- Confirm server-side authorization on the one new write path that can target an account that isn't the current user's own (
GoalSettingsForm.tsx, reachable from the coaching route) — this is exactly the kind of thing the PR description itself flagged ("double check this is okay to go into production"). - Fix the mutation-sequencing bug in
StepNavigation.tsxandSummary.tsx— confirmed independently by 5 agents — where a failed secondary write can make a successful primary submission look like it failed. - Two of the five tools (MPD GoalCalculator, PDS) silently change the location preference with no disclosure at all, while the other three show one — worth deciding deliberately rather than leaving as an accident of independent implementation.
Line comments below cover the concrete, single-location findings. A few structural findings don't anchor to one line and are summarized here instead:
- 3× duplicated read-compare-write logic across
GoalSettingsForm.tsx,Summary.tsx,StepNavigation.tsx(Architecture, severity 6.5) — and this duplication is why the sequencing bug was reproduced independently in two places rather than fixed once. The repo already has the right precedent:src/hooks/useUserPreference.tspairs exactly this read-compare-write-skip-if-unchanged pattern in one hook. - Three HrTools domains now run the full page-scoped
GetAccountPreferencesquery (Architecture 6.0, Data Integrity 4.0, Security 4.0) just to read one scalar — it also drags in an unpaginatedaccountLists { nodes }connection. A narrowAccountListGeographicLocationquery would normalize into the same cache entry without the extra weight. - 4 new translation strings were never extracted to
public/locales/en/translation.json(Standards, severity 5.5) — this repo commits extracted keys as part of the feature PR (checked 4 prior examples). Runyarn extract. - Coaching-route authorization (see the line comment on
GoalSettingsForm.tsx) — Security found a concrete signal that writing into a coachee's account is normally avoided:CoachingLink.tsxexists specifically to hide a link into a coachee's preferences page. Whether the new write here is fine depends entirely onupdateAccountList's server-side authorization, which isn't visible from this repo.
Investigated and disproven
Testing raised three "Important" (6.5-7.5) findings claiming the negative-mutation-assertion tests in StepNavigation.test.tsx, Summary.test.tsx, and GoalSettingsForm.test.tsx could pass even if their underlying guards were removed (a race between waitFor's resolution and the async mutation chain). I verified this directly rather than relying on reasoning: I temporarily removed each of the three guards in turn and re-ran the exact named tests.
StepNavigation.test.tsx— guard removed → test failed (caughtUpdateAccountPreferenceswithgeographicLocation: null).GoalSettingsForm.test.tsx— equality check removed → test failed (caught the spurious sync).Summary.test.tsx— equality check removed → test failed (caught the disclosure Alert appearing).
All three tests are sound; waitFor's polling interval gives ample time for the guarded await to resolve before the synchronous negative assertion runs. These three findings are dismissed with concrete counter-evidence and don't count toward the verdict. All files were reverted to their original diff state afterward and the full suite re-passes.
Dependency impact
Confirmation.tsx— 20 consumers app-wide. The only behavioral change (component="div") is confirmed fixing, not introducing, a pre-existing invalid-HTML risk.AccordionEnum.ts(PreferenceAccordion) — 13 consumers, purely additive enum member, no exhaustiveswitchanywhere. No breaking change.SubmitModal.tsx— 3 consumers, new prop is optional and collision-free.StyledDialogContentText.ts— 2 consumers, new type annotation is additive, not narrowing.- No breaking changes found anywhere.
Review summary
| Agent | Important | Medium | Suggestions | Confidence |
|---|---|---|---|---|
| Security | 2 | 0 | 3 | Medium-High |
| Architecture | 4 | 3 | 4 | High |
| Data Integrity | 2 | 0 | 5 | High |
| Testing | 1 (3 disproven) | 0 | 6 | High (static); disproven on 3 dynamic-timing claims |
| UX | 4 | 2 | 4 | High |
| Standards | 0 | 2 | many (checklist mostly PASS) | High |
| Financial Reporting | 2 | 0 | 3 | High |
| Dependency Impact | 0 | 0 | 0 (all confirmed safe) | High |
|
❗️BLOCKED❗️ Waiting for a couple of issues to be fixed before I can finish up this PR:
|
canac
left a comment
canac
left a comment
There was a problem hiding this comment.
Looks great! It will be nice for users to not have to keep putting in their geographic location.
I think "last write wins" is mostly reasonable! The one caveat I just realized is that staff can use the MPD goal calculator to answer questions about future realities, i.e. "What will my goal be when I move to NYC?" In that case, their location in the goal might not be their actual location. But MPDX doesn't change the location until they click Apply, so I think it's still OK. Users probably won't apply hypothetical goals for cities they aren't at least planning to move to. |
|
@canac Can you glance through the last change I made? I had those changes on stand by because I was waiting for Will's PR to merge that fixed the "None" bug. Also, do you think this is okay to merge into production? I am thinking yes since we are just adding a geographic location drop down in account settings. Just want to make sure we don't need to send an email out mentioning the new preference. |
canac
left a comment
canac
left a comment
There was a problem hiding this comment.
I didn't retest, but this still looks good!
9789bca to
e5ad988
Compare
|
❗️Waiting for Keely to release a help article explaining this new feature before merging. |
2 participants
Description
We want to add geographic location to account preferences so the MPD supervisor report can grab these locations easily. Anytime a calculator submits and changes to a different geographic location, we will update the account preference to reflect the update.
Solution:
Double check this is okay to go into production before the reports go-live
Note: We need to make the change for the NS goal calculator, however, admins/coaches are not allowed to update account list settings for new staff members. I need to update this server-side and then make changes in a follow-up PR (THIS IS DONE ✅)
Testing
Account settings test: ✅
/settings/preferencesSalary calculator test: ✅
/hrTools/salaryCalculatorMPD goal calculator test: ✅
/hrTools/goalCalculatorPDS goal calculator test: ✅
/hrTools/pdsGoalCalculatorNSO questionnaire test: ✅
/hrTools/nsoMpdQuestionnaireNew Staff calculator test: ✅
/coaching/45ef99a5-c7b9-4873-b194-ca4d5b28d09e/nsGoalCalculatorChecklist:
/quality:agent-reviewcommand locally and fixed any relevant suggestions