MPDX-9834 Add year filter to MPGA by kegrimes · Pull Request #1929 · CruGlobal/mpdx-react

kegrimes · 2026-07-20T18:22:21Z

Description

MPGA UAT issues:

Add "Select Date Range" on report settings for MPGA
Add "Year to Date" option, as well as the previous years where transactions occurred
Grayed out future months if "Year to Date" filter is applied

Extra:

Created a shared context to remove prop drilling
Created a shared test wrapper for all tests

Jira ticket: MPDX-9834

Testing

Impersonate: courtney.campbell@cru.org

No filter (default) test:

Go to /reports/mpgaIncomeExpenses
Check that the data is from the last 12 months (August 2025 - July 2026)

Year to Date filter test:

Go to /reports/mpgaIncomeExpenses
Click "Report Settings" and select "Year to Date"
Check that the data is from this year only (January 2026 - July 2026)
Verify the future months are grayed out

Past year filter test:

Go to /reports/mpgaIncomeExpenses
Click "Report Settings" and select "2025"
Check that the data is from 2025 only (January 2025 - December 2025)

Checklist:

I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
I have applied the appropriate labels (Add the label "Preview" to automatically create a preview environment)
I have run the Claude Code /pr-review command locally and fixed any relevant suggestions
I have requested a review from another person on the project
I have tested my changes in preview or in staging
I have cleaned up my commit history

github-actions · 2026-07-20T18:23:06Z

Preview branch generated at https://mpdx-9834-add-year-filter-on-mpga.d3dytjb8adxkk5.amplifyapp.com

github-actions · 2026-07-20T18:26:01Z

Bundle sizes [mpdx-react]

Compared against ea57016

Route	Size (gzipped)	Diff
`/accountLists/[accountListId]/reports/mpgaIncomeExpenses`	428.21 KB	+2.39 KB

…lter-on-mpga

kegrimes

🤖 Multi-Agent Code Review — ✅ APPROVED WITH SUGGESTIONS

PR #1929 — Add year filter to MPGA · Mode: standard · 6 specialized agents + dependency analysis + 2 gap-review agents + 3 adversarial verifiers.

A well-executed refactor. It deletes TotalsContext and a large amount of prop-drilling, consolidates fetch/derive/totals into one ReportContext, and the new year/YTD date-window + zero-fill logic is verified correct (no off-by-one in firstFutureMonthIndex, totals not inflated by future zeros, date windows serialize correctly, no broken imports). No blockers. All findings below are improvements.

Risk Assessment

10/10 · CRITICAL — driven by breadth (17 medium-risk feature files) + change volume + the financial-reporting domain, not by any inherently dangerous infra file. Recommended reviewer level: SENIOR (financial display logic).

Dependency Impact

ReportContext.tsx has 15 dependents, all updated in this PR (self-contained). Every breaking edit — deleted TotalsContext, useGetLastTwelveMonths(locale, year?) signature change, hideDateRange→isMpgaReport rename, un-exported getFiltersWithCalculatedDates — has its call sites fixed in-PR. External consumer StaffExpenseReport (not in PR) verified safe.

Findings not anchored to a diff line (coverage gaps, related files, pre-existing)

Medium (5.0–6.9)

🟠 [6.5] Positional month↔label alignment is fragile — src/components/Reports/MPGAIncomeExpensesReport/Helper/filterFunds.ts:92,135,187 (Data Integrity, PLAUSIBLE; related file, not in PR)
Row monthly[] is built by array index from breakdownByMonth; monthLabels and the zero-fill are indexed independently — nothing joins on month.month. Now that arbitrary years / partial YTD windows are queryable, a sparse or reordered breakdownByMonth from the server would silently shift every column (April's amount under March's header; a real month grayed as "future"). Fix: key months by month.month, or assert the one-ordered-entry-per-requested-month server contract.

🟠 [6.0] Deleted negative-net calc test not migrated — Charts/MonthlySummaryChart test (diff 374-407) (Testing)
The removed test was the only assertion covering a negative monthly net (expenses > income). Re-add an equivalent net-calc assertion driven through the context mock.

🟡 [5.0] Headline "graying" feature has no rendering test — Tables/{TableCard,PrintTables,TotalRow} (Testing, CONFIRMED; downgraded from 7.5 by adversarial verify)
The derivation is unit-tested in ReportContext.test.tsx, but no component test activates YTD, so the future-month classes / gradient underline / colSpan-split paths never execute under test. MPGAIncomeExpensesReportTestWrapper can't seed filters — closing this needs an initialFilters seam on ReportProvider/the wrapper. Real regression risk, not a live bug.

Suggestions (<5.0)

[4.0] Empty-state copy hardcodes "in the last 12 months" — Tables/PrintTables.tsx:259-260, DisplayModes/ScreenOnlyReport.tsx:42,57 (UX, in-scope but pre-existing lines). Now contradicts the dynamic subtitle when a specific year / YTD is selected and yields no data. Thread subtitle (already on useReport()) into the empty-state copy.
[3.0] Average column mixes two divisor conventions in YTD — server averagePerMonth (plain/subcategory rows) vs client total / monthly.length (split/combined rows). Verified the client divisor uses the elapsed-month array N, so they agree if the server also divides by N; normalize to one convention. No demonstrable miscalc today.

Pre-existing (informational, not counted toward verdict)

Hardcoded currency = 'USD' also in BreakdownModal.tsx:41 (not migrated to useReport() like its siblings). No currencyCode is read from the data → wrong symbol for non-USD accounts. Worth a follow-up ticket given the report's purpose.
SettingsDialog <Dialog> missing aria-labelledby → DialogTitle; empty-state colSpan={15} magic number in PrintTables; Cancel button sx={{ color: 'black' }} (hardcoded, use a theme token).

Agent Summary

Agent	Blockers	Medium	Suggestions	Confidence
Security	(skipped — no auth/api/env/apollo-link files)
Architecture	0	0	5	Med-High
Data Integrity	0	1	2	High
Testing	0	2	3	High
UX	0	1	4	High
Standards	0	1	0	High
Financial Reporting	0	0	2	Med-High

Financial Checklist: Money arithmetic safe ✅ · Currency mixing prevented ✅ (single server-aggregated figure) · Rounding at display boundary ✅ · Null/undefined handled ✅ · Luxon (not new Date()) ✅ · Server aggregations preferred ✅ (caveat: Average mixes server+client — see above)

Inline comments below carry a  tag. Reply /dismiss: <reason> on any finding with severity < 7 you disagree with, then re-run the review.

🤖 Generated with Claude Code

…lter-on-mpga

kegrimes · 2026-07-23T19:28:05Z

@zweatshirt I realized Will was out! So sorry for the large PR🫣, but most of it is removing prop drilling. I noticed it was getting bad when I was adding the filters. Can you just make sure nothing is broken with the filters and the code (unrelated to context) looks decent?

zweatshirt

Tried to comment on whatever I was able to find. The UI looks amazing and seems to behave as expected so I am going to approve. Just had some suggestions.

zweatshirt · 2026-07-24T14:59:28Z

+  const incomeData = data.income ?? [];
+  const expenseData = data.expenses ?? [];


Optional: We can use de-structuring here

zweatshirt · 2026-07-24T15:03:49Z

-          </GqlMockedProvider>
-        </LocalizationProvider>
-      </ThemeProvider>,
+  it('displays the tables that should be showing', async () => {


This test checks that there are 2 tables but only seem to add an assertion for one table by text. What is the second table that is supposed to display? Can we add an assertion for it in this test?

zweatshirt · 2026-07-24T15:11:12Z

+      incomeTotal,
+      expensesTotal,
+      ministryTotal,
+      healthcareTotal,
+      assessmentTotal,
+      benefitsTotal,
+      salaryTotal,
+      otherTotal,


I would group these, something like this:

totals: { income, expenses, ministry, healthcare, assessment, benefits, salary, other, }

zweatshirt · 2026-07-24T15:30:34Z

+                  .slice(0, index)
+                  .reduce((sum, group) => sum + group.count, 0);
+
+                const futureStart =


What do you think of changing this to something like pastCount instead of futureStart?

zweatshirt · 2026-07-24T15:34:03Z

+
+                const futureStart =
+                  firstFutureMonthIndex !== undefined
+                    ? Math.max(


Can futureStart ever go negative? Wondering if this Math.max is needed or not

zweatshirt · 2026-07-24T15:36:49Z

+            sx={{
+              '& .future-month-header .MuiDataGrid-columnHeaderTitle': {
+                color: theme.palette.text.disabled,
+              },
+              '& .future-month': {
+                backgroundColor: theme.palette.action.hover,
+              },
+            }}


Optional:

Suggested change

sx={{

'& .future-month-header .MuiDataGrid-columnHeaderTitle': {

color: theme.palette.text.disabled,

},

'& .future-month': {

backgroundColor: theme.palette.action.hover,

},

}}

sx={{

'.future-month-header .MuiDataGrid-columnHeaderTitle': {

color: theme.palette.text.disabled,

},

'.future-month': {

backgroundColor: theme.palette.action.hover,

},

}}

zweatshirt · 2026-07-24T15:38:25Z

      },
    ];
-  }, [monthCount, getBorderColor, t]);
+  }, [monthCount, getBorderColor, firstFutureMonthIndex, t]);


Probably won't ever trigger but theme could go in the dependency array

zweatshirt · 2026-07-24T15:41:34Z

+  const isFilterActive = useMemo(
+    () =>
+      Boolean(
+        filters &&
+          (filters.selectedDateRange === DateRange.YearToDate ||
+            (filters.selectedYear !== null &&
+              filters.selectedYear !== undefined)),
+      ),
+    [filters],
+  );


We can probably keep this un-memoized

zweatshirt · 2026-07-24T15:43:03Z

+            const hasActiveFilter = Boolean(
+              newFilters &&
+                (newFilters.categories ||
+                  newFilters.selectedDateRange === DateRange.YearToDate ||
+                  (newFilters.selectedYear !== null &&
+                    newFilters.selectedYear !== undefined)),
+            );


This and isFilterActive are nearly identical, what do you think about creating a shared variable for both conditions?

zweatshirt · 2026-07-24T15:46:23Z

+  const transformedData: Funds[] = useMemo(
+    () =>
+      (reportData?.reportsStaffExpenses?.funds ?? []).map((fund) => ({
+        ...fund,
+        categories: (fund.categories ?? []).map((category) => ({
+          ...category,
+          category: category.category,
+          breakdownByMonth: category.breakdownByMonth.map((month) => ({
+            ...month,
+          })),
+          subcategories: (category.subcategories ?? []).map((subcategory) => ({
+            ...subcategory,
+            subCategory: subcategory.subCategory,
+            breakdownByMonth: subcategory.breakdownByMonth.map((month) => ({
+              ...month,
+              transactions: (month.transactions ?? []).map((transaction) => ({
+                transactedAt: transaction.transactedAt,
+                description: transaction.description ?? '',
+                amount: transaction.amount,
+              })),
+            })),
+          })),
+        })),
+      })),
+    [reportData],
+  );


Wondering if it is worth adding a comment for this function explaining why we transform the data here

Add year to date filter to mpga

4e04dbc

kegrimes self-assigned this Jul 20, 2026

kegrimes added the Preview Environment Add this label to create an Amplify Preview label Jul 20, 2026

Katelyn Grimes added 9 commits July 21, 2026 11:56

Add past year filter to mpga

51269fe

Merge remote-tracking branch 'origin/main' into mpdx-9834-add-year-fi…

237a723

…lter-on-mpga

Merge remote-tracking branch 'origin/main' into mpdx-9834-add-year-fi…

97a5b74

…lter-on-mpga

Update label on filter change

b454f32

Fix failing test

e7f379d

Add grayed out columns when year to date filter applied

5b0846c

Move shared logic into context to avoid prop drilling

5696ddb

Disallow clear button from appear when category selection changes

13a4033

Fix up

590a073

kegrimes commented Jul 22, 2026

View reviewed changes

Katelyn Grimes added 2 commits July 22, 2026 14:49

Claude review suggestions

f103ae0

Update green/red to darker shades

aa25927

kegrimes requested a review from wjames111 July 22, 2026 18:55

Merge remote-tracking branch 'origin/main' into mpdx-9834-add-year-fi…

cf1188e

…lter-on-mpga

kegrimes removed the request for review from wjames111 July 23, 2026 18:08

Add transaction years to filter dropdown options

434ce62

kegrimes requested a review from zweatshirt July 23, 2026 19:27

zweatshirt approved these changes Jul 24, 2026

View reviewed changes

		const incomeData = data.income ?? [];
		const expenseData = data.expenses ?? [];

Uh oh!

Conversation

kegrimes commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist:

Uh oh!

github-actions Bot commented Jul 20, 2026

Uh oh!

github-actions Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle sizes [mpdx-react]

Uh oh!

kegrimes left a comment

Choose a reason for hiding this comment

🤖 Multi-Agent Code Review — ✅ APPROVED WITH SUGGESTIONS

Risk Assessment

Dependency Impact

Findings not anchored to a diff line (coverage gaps, related files, pre-existing)

Agent Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kegrimes commented Jul 23, 2026

Uh oh!

zweatshirt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zweatshirt Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kegrimes commented Jul 20, 2026 •

edited

Loading

github-actions Bot commented Jul 20, 2026 •

edited

Loading

zweatshirt Jul 24, 2026 •

edited

Loading