MPDX-9818 - Add DA5 and A33 warnings/errors to goal settings#1930
MPDX-9818 - Add DA5 and A33 warnings/errors to goal settings#1930zweatshirt wants to merge 4 commits into
Conversation
zweatshirt
added
Preview Environment
|
Preview branch generated at https://MPDX-9818.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against ea57016
|
zweatshirt
left a comment
zweatshirt
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review — PR #1930
Verdict: APPROVED WITH SUGGESTIONS — no blockers. Highest finding is 6.5/10.
Risk: 7/10 (HIGH) — src/components/**/*.graphql (+2), 4x feature components (+4), 146 non-test lines (+1). Senior review recommended. Codegen verified clean (yarn gql succeeds, yarn lint:ts exits 0, 213 tests pass).
Two rounds: 6 specialized agents each. Round 2 was re-run with round-1 findings adjudicated against verified facts, which recalibrated three findings downward and surfaced two better ones.
Key recalibrations from round 1
| Finding | Was | Now | Why |
|---|---|---|---|
| Failed preview silently suppresses warning | 7.5 | 3.5 | Self-corrects on save (UpdateNewStaffGoalCalculation returns the fragment incl. salaryOverCap; enableReinitialize set; Save never gated on it) |
| Duplicate preview mutations | 7.5 | 6.5 | BatchHttpLink (batchInterval 20ms) coalesces both into one HTTP request; cost is doubled server compute, not round-trips |
savedSalaryOverCap = false fails open |
4.5 | 2.0 | The only caller omitting it (MpdGoalPreview) never reads salaryOverCap |
Verified correct (no action)
- The always-mounted empty
aria-liveregion is proper live-region technique — mutation-tested: deletingaria-liveand making the region conditional both fail the suite. - Cache normalization correct (
NewStaffGoalCalculationCalculationshas noid; non-normalizable value type).fetchPolicy: 'no-cache'correct (NewStaffGoalCalculationPreviewis a distinct typename). - Destructuring-with-defaults change is behaviourally identical (
monthlyGoal: Float!,salaryOverCap: Boolean!). - Scenario-goal path (
accountListId: null) is valid per schema and pre-existing viaMpdGoalPreview. - Save-while-preview-in-flight race traced and safe.
- Cap semantics: a non-joining senior-staff spouse's salary IS included against the married cap (
applicable? = !spouse? || !single?) — coherent, no bug. SOSA correctly treated as single. - Mock fixtures deterministic (
seed = 'seed', probed across processes). salaryOverCap: falseadded toMpdGoalPreview.test.tsxis inert — removing it still passes 15/15.- Missing
public/locales/enkey follows the repo's batch-extraction convention. Not a violation.
Findings outside this PR's diff (informational, not blockers)
1. The contrast fix belongs in src/theme.ts, not in this component. src/theme.ts never defines palette.warning, so warning.main resolves to MUI's default #ed6c02 (3.11:1 on #fff). But HrTools uses warning.main in 7 of 7 pre-existing cases — changing only this file would make it the sole diverging file. Systemic fix:
warning: { main: statusColors.warning }, // #D34400 -> 4.57:1
success: { main: statusColors.success }, // CardTitle.tsx:24 has the same latent defectPre-existing HrTools-wide. Recommend a standalone ticket rather than blocking this PR.
2. Review-process gap. .claude/rules/code-review.md lists src/components/User/impersonate*, which matches nothing — impersonation lives in Settings/Admin/ImpersonateUser/, Settings/Organization/ImpersonateUser/, and ProfileMenu/. The Security agent's impersonation trigger never fires. Also stale: src/components/Reports/SalaryCalculator/** and pages/api/Schema/uploads/** don't exist.
3. Doc drift. CLAUDE.md and the rules file say "Material UI v5"; yarn.lock resolves @mui/material 7.3.11.
4. Questions for the API team (mpdx_api #3469):
salary_over_cap?callsbase_salary_capunconditionally, whereassalary_caponly reaches it whenallow_salary_over_cap == 'no'. It resolves viafind_by!, which raises. Are the fourboard_approved_salary_calcmisc constants seeded for every historicalcalculations_year? A raise would null the wholecalculationsblock via non-null propagation, takingmonthlyGoaldown with it.salary_over_cap?uses strict>— confirm equal-to-cap is intended as compliant.
Recommended order of work
- The flicker fix (
useMpdGoalPreview.ts:151) — one line, real user-facing defect - Two test assertions (warning renders in the form;
accountListIdin both directions) — closes a mutation-proven hole - Resolve the dead
mocksprop either way - Copy revision
- Defer: theme palette, doc drift, rules globs
zweatshirt
left a comment
zweatshirt
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review — PR #1930 (Round 3)
Verdict: APPROVED WITH SUGGESTIONS — no blockers. Highest finding 6.0/10.
Risk: 7/10 (HIGH) — src/components/**/*.graphql (+2), 4x feature components (+4), ~145 non-test lines (+1). Senior review recommended.
6 agents (Architecture, Testing, Standards, Data Integrity, UX, Financial Reporting). Third round on this branch; the six fix-up commits since the last review closed every prior finding, so this pass focuses on the new code.
Confirmed fixed since the last review
Verified by mutation testing — each item below now fails a targeted test when reverted:
| Fix | Guarded by |
|---|---|
| Warning flicker on every edit | does not re-raise a resolved warning… |
accountListId untested in both directions |
both useNewStaffGoalCalculation tests |
| Warning-in-form integration unasserted | shows the over-cap warning beside the save actions |
| Real timers against a 500ms debounce | fake timers added; probed with a 5,000,000ms debounce |
Dead mocks test prop |
removed |
Stale plural JSDoc, unused export |
fixed |
Mutation audit: 9 of 10 mutations caught. The one gap is the .catch fallback — see the inline comment on useMpdGoalPreview.ts.
Root cause found for "the warning shows when Annual Requested Salary is empty"
This was reported during review and is not stale persistence. app/services/new_staff_goals/salary_calculation.rb:73-81:
requested_annual_salary = calculator_attribute(:requested_annual_salary)&.to_d
requested_annual_salary || ((geographic_multiplier * base) + tenure_adjustment + debt_salary)An empty field maps to null on the client (toNumberOrNull('')), and the API then computes a default salary and compares that against the cap. The flag is correct — the goal calculation really does use that computed figure. The copy is what's wrong: it says "requested salary" when nothing was requested. See the inline comment on GoalSettingsWarning.tsx.
Findings on related files (not in this PR)
1. Contrast fix belongs in src/theme.ts, not in the component. palette.warning is never defined, so warning.main resolves to MUI's default #ed6c02 — 3.11:1 on #fff, failing the 4.5:1 bar for 14px body2. The repo's own statusWarning (#D34400) is 4.57:1. But HrTools uses warning.main in 7 of 7 pre-existing cases, so changing only this file would make it the sole outlier. Systemic fix:
warning: { main: statusColors.warning }, // #D34400 -> 4.57:1
success: { main: statusColors.success }, // CardTitle.tsx:24 has the same latent defectPre-existing and tree-wide — should not block this PR. Standalone ticket recommended.
2. Review-process gap. .claude/rules/code-review.md lists src/components/User/impersonate*, which matches nothing — impersonation lives under Settings/Admin/ImpersonateUser/, Settings/Organization/ImpersonateUser/, and ProfileMenu/. The Security agent's impersonation trigger never fires. Also stale: src/components/Reports/SalaryCalculator/** and pages/api/Schema/uploads/**.
3. Doc drift. CLAUDE.md and the rules file say "Material UI v5"; yarn.lock resolves @mui/material 7.3.11.
Questions for the API team (mpdx_api #3469)
- Expose the amounts behind the warning. To say "Total salary of $X is over the standard cap of $Y" the client needs
combined_uncapped_total_salaryandbase_salary_caponNewStaffGoalCalculationCalculations. Neither is exposed today, and the three salary fields that are (salary,salarySubtotal,totalSalary) are all monthly and post-capping —base_salaryscales down when the combined total exceeds the cap, sototalSalaryis by construction never over the cap. Rendering any of them beside "is over the cap" would show a number that isn't. This is the blocker on improving the copy further. - Seeding.
salary_over_cap?callsbase_salary_capunconditionally, whereassalary_caponly reaches it whenallow_salary_over_cap == 'no'. It resolves viaMpdGoalMiscConstant.find_by!, which raises, and a raise would null the wholecalculationsblock via non-null propagation — takingmonthlyGoalwith it. The in-repo seeder is all-or-nothing per year (so the risk is pre-existing, not new), but production seeding isn't in the repo. Is the prod path also all-or-nothing per year, and is everycalculations_yearin use backfilled? - Seeded cap values differ from the spec.
MpdGoalMiscConstantsSeederhas 80k/125k/90k/145k;goal_calculator_spec.rbstubs 85k. Which figures are current?
| setPreview({ | ||
| attributes: debounced, | ||
| monthlyGoal: null, | ||
| salaryOverCap: null, |
There was a problem hiding this comment.
This .catch sets salaryOverCap: null, which line 152's ?? coalesces to savedSalaryOverCap. So an admin who raises the salary over cap, sees the warning, then makes any further edit whose preview request fails, watches the warning disappear — and can reasonably read that as "resolved". There is no snackbar (context: { suppressErrors: true }, line 108) and GoalSettingsWarning never consumes the hook's failed flag, so unlike MpdGoalPreview — which renders "Preview unavailable" for the same failure — the warning bar shows nothing at all.
This also contradicts the flicker fix's own principle: every other path holds the last known answer rather than reverting to saved.
Untested. Changing this null to false left 165 of 165 tests passing. It became unreachable from the suite when the mocks prop was removed from GoalSettingsWarning.test.tsx — that prop was the only handle for forcing a failure.
Two defensible resolutions:
// (a) hold the last known verdict rather than dropping it
.catch(() => {
if (active) {
setPreview((prev) => ({
attributes: debounced,
monthlyGoal: null,
salaryOverCap: prev?.salaryOverCap ?? null,
}));
}
});or (b) keep reverting to saved, but have GoalSettingsWarning render the failed state the way MpdGoalPreview does, so the disappearance is explained rather than silent.
Either way, add a test — re-introducing the mocks override (the pattern still present in MpdGoalPreview.test.tsx) is enough.
Severity is bounded because the warning is advisory, never gates Save, and self-corrects on the next successful preview or on save.
| savedSalaryOverCap = false, | ||
| previewSalaryOverCap = false, | ||
| }) => ( | ||
| <GqlMockedProvider<{ |
There was a problem hiding this comment.
11 test files in this directory wrap in <ThemeProvider theme={theme}>, including the closest analogue MpdGoalPreview.test.tsx:56-58.
GoalSettingsWarning.tsx:35-36 styles on warning.main, so in tests it resolves against MUI's default palette rather than src/theme. The suite therefore cannot catch a token that exists in the default theme but not the project's — which is precisely the situation here (palette.warning is undefined in src/theme.ts, see the review body).
Low blast radius, but it's the reason the contrast issue was invisible to CI.
|
@zweatshirt Are you also going to add warnings that the user is healthcare exempt, SECA exempt, or over the debt cap? (Sorry I missed the debt cap before with the API PR. I haven't dug in, but that check may need to happen on the server.) |
|
@canac I can add that to this PR. My brain assumed we weren't adding them due to what we concluded with the backend PR, but it makes sense we want them still. |
|
Yesterday, I was just thinking about the |
|
@canac The backend PR added a boolean for healthcare exemption but you mentioned we don't need to check for it and so I removed it, are we still wanting to implement it? |
|
The sheet just surfaces a non-blocking warning when someone has an opt out or exemption. I think we can just read the booleans from the goal calculation and show a message if any of them are true. Like it says something like "SECA Opted Out (Both)", for example. I don't think that needs to be a validation in the backend calculations. What do you think? |
|
@canac That was what I was intending with the boolean in the GraphQL for healthcare exemption. Is the goal to do as much of the logic for the warnings in the the frontend then? My idea was that we could send booleans to the frontend on which warnings to display and have the frontend act on those (or in the case of SECA we could maybe do an enum) I guess the frontend has enough information for most of these apart from the salary cap and debt cap to do it in the frontend instead, though |
|
@canac This is basically ready for review. I've done multiple review agent iterations, it is considered critical. Sorry, it turned out to be a much bigger PR than I expected. Feel free to share constructive criticism. |
zweatshirt
changed the title
2 participants
Description
We want to show a warning to the admin when the salary the admin has entered is over the salary cap.
Backend PR: https://github.com/CruGlobal/mpdx_api/pull/3469
Testing
Checklist:
/pr-reviewcommand locally and fixed any relevant suggestions