[MPDX-9522] Use new staff goal calculations from API - #1873
Conversation
|
Preview branch generated at https://9522-use-calculations.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against 750278f
|
aa018eb to
c4199f9
Compare
c4199f9 to
b55917e
Compare
canac
left a comment
There was a problem hiding this comment.
🤖 Multi-Agent Code Review — PR #1873
Verdict: APPROVED WITH SUGGESTIONS — no hard blockers, but one confirmed financial-display divergence should be resolved or product-confirmed before merge. Human review recommended.
6 specialized agents (Financial, Data Integrity, Architecture, Testing, Standards, UX) reviewed in parallel. Testing agent ran the suite: 45/45 pass, tsc exit 0.
Headline
The "Presenting Your Goal" total re-sums a subset of fields client-side and omits the always-applied ×1.06 attrition factor, so the donor-facing "Total Support Goal" systematically understates the admin's authoritative monthlyGoal. See the inline comment on PresentingYourGoalStep.tsx. The established sibling GoalCalculator deliberately folds attrition in (…/PresentingYourGoal.tsx:41 ministryExpenses: ministryExpensesTotal + attrition); this new step drops it and doesn't even query attrition. Tests miss it because the mock is rigged so the 6 fields sum to exactly monthlyGoal (15860).
Verified clean / refuted (no action)
- Null
calculations— schema isNewStaffGoalCalculationCalculations!with allFloat!subfields; dropping the old?? 0guard is correct. Parent is nullable but guarded in both consumers. - Coaching vs hrTools layout — coaching page renders
GoalSettingsFormdirectly (intended split); hrTools wrapsNsGoalCalculatorin exactly oneNsGoalCalculatorLayout(no double/lost wrap). - Apollo cache / codegen / mutation refresh —
idpresent, embedded value-object needs none, all 9 fields exist inschema.graphql, mutation re-selects the fragment. Clean. - Standards — full checklist PASS, no violations. All new strings localized; named exports; no
any; TODOs ticket-referenced.
Risk
Mechanical score 10/10 (CRITICAL) is breadth-driven — 12 small sibling files each +1, +2 for the .graphql. Substance is display-only (no auth, no mutations, non-null types), so effective operational risk ≈ MEDIUM, elevated to HIGH by the confirmed financial finding. Auto-approve intentionally does not apply.
Lower-priority items (posted inline)
Fake $3,624 special-needs placeholder shown as real money · 403(b) values missing per-person aria-label · unused goalCalculation prop on ReviewYourCalculationStep (4-agent consensus) · loading/error states render flush to edge · inline error Alert + global snackbar double-surface (informational, not a violation).
Minor (not posted inline)
- Low-value
useMemoformonthlyNeeds(PresentingYourGoalStep.tsx:63-74) — consumers don't benefit from a stable ref; optional. - Hardcoded
'USD'in the 403(b) format — reasonable US-only assumption, consistent with siblings. - Non-null
!inNsGoalCalculatorTestWrapper.tsx:56— safe test infra. afterEach→afterAllinPresentingYourGoalStep.test.tsxis a correct fix; only residual is thewindow.printspy no longer restored between tests (harmless today).
Generated by /quality:agent-review · standard mode · this is a COMMENT review, not an approval.
canac
left a comment
There was a problem hiding this comment.
🤖 Multi-Agent Code Review — Verdict: APPROVED WITH SUGGESTIONS
6 specialized agents (Architecture, Testing, Standards, Data Integrity, UX, Financial Reporting) + dependency-impact analysis, standard mode. Solid PR: removes mock data, adds real loading/error/empty states, improves type safety, and hoists a remounting layout. Net debt reduced.
No hard blockers. One Important (7.5) finding is strongly recommended before merge (see the inline comment on PresentingYourGoalStep.tsx).
⚠️ 1 Important finding (severity 7.0–7.9) cannot be dismissed via/dismiss. Fix it in this PR or open a follow-up ticket. Medium/Suggestion findings (<7) can be dismissed by replying/dismiss: <reason>to the inline comment.
Risk
Rubric score 10/10 (CRITICAL) — but capped purely by breadth (17 files, 11 medium-risk components) of one cohesive feature. No auth / mutations / cache-policy / server code touched. Practical risk is MEDIUM-HIGH, concentrated in financial-display correctness.
Top finding (Important, 7.5) — agreed by 3 agents + verified against schema
The presentation's "Total Support Goal" is re-derived client-side (useMonthlyNeedsRows rows.reduce) instead of using the authoritative server value calculations.monthlyGoal — which this same PR switched to in GoalSettingsForm.tsx:123. The 6 displayed buckets omit line items 8/10/11/12/13 (staffConferenceTransfer, medicalExpenses, accountTransfers, advocacyTransfers, otherExpenses). Verified from schema.graphql: totalMinistryExpenses = "sum of lines 2-7", monthlyGoal = subtotal(1-13) + adminCharge + attrition. So monthlyGoal − displayedTotal = those five omitted lines. When any is non-zero (medical/other are not senior-only), the staffer is shown a goal lower than reality, and the presentation disagrees with the Goal Settings header. The fixture reconciles only because all five are 0.
Findings summary
| # | Sev | Tier | Location |
|---|---|---|---|
| 1 | 7.5 | Important | PresentingYourGoalStep.tsx — client reduce vs server monthlyGoal |
| 2 | 6.0 | Medium | PresentingYourGoalStep.test.tsx — afterEach→afterAll isolation regression |
| 3 | 5.5 | Medium | FinancialInformationSection.tsx — aria-label on <p> won't announce |
| 4 | 5.0 | Medium | PresentingYourGoalStep.tsx — attrition folded into Ministry Expenses row |
| 5 | 4.5 | Suggestion | Test: assert total == monthlyGoal (would catch #1) |
| 6 | 4.0 | Suggestion | NsGoalCalculator.tsx — duplicated loading/error/empty block |
| 7 | 4.0 | Suggestion | Test: single-person 403(b) hide path |
| 8 | 3.5 | Suggestion | ReviewYourCalculationStep.tsx — dead goalCalculation prop |
| 9 | 3.0 | Suggestion | NsGoalCalculatorTestWrapper.tsx — non-null assertion (test-only) |
Additional low-severity suggestions (not line-anchored): loading Skeleton height={400} doesn't approximate content (kept identical to sibling — consistency nit); support-raised query now waterfalls after goal-calc load on the Presenting step (minor latency).
Dependency impact
No unhandled breaking changes — every consumer updated in-PR or compatible. calculatedResults removed with 0 remaining references repo-wide. Two operational notes: yarn gql codegen must succeed for the fragment change; the default test mock flipped spouseJoining false→true and added fields — run the 4 wrapper-dependent test files (NextStepsStep, NsGoalCalculatorContext, NsGoalCalculatorStepsList, NsGoalCalculatorLayout) to confirm no assertion drift.
Standards
All checklist items PASS (2 low-severity WARN). Named exports ✓ · i18n {{interpolation}} ✓ · <Trans t={t}> ✓ · no any/@ts-ignore/new Date() ✓ · GraphQL id correctly present (calculations is a non-normalizable value object — no id needed) · codegen fields verified present in schema ✓.
4df400d to
eb562d0
Compare
zweatshirt
left a comment
There was a problem hiding this comment.
Looks good from what I can tell. I only had one real concern. Since this PR will effect #1884 I'll wait for your PR to merge
Description
Use server-side calculations in the admin view and the presenting your goal step.
Jira ticket: MPDX-9522
Depends on https://github.com/CruGlobal/mpdx_api/pull/3422deployed 🚀Testing
Checklist:
/pr-reviewcommand locally and fixed any relevant suggestions