Conversation
…b.com/HHS/OPRE-OPS into OPS-4142/agreement-list-summary-cards
…b.com/HHS/OPRE-OPS into OPS-4142/agreement-list-summary-cards
Santi-3rd
requested review from
fpigeonjr,
johndeange,
josbell,
rajohnson90 and
weimiao67
as code owners
Santi-3rd
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds two summary cards to the Agreements list page to support reporting needs (Issue #4142), and updates API/UI plumbing so the cards can compute agreement counts and spending breakdowns by agreement type.
Changes:
- Adds an
AgreementSummaryCardsSectionto the agreements list layout, and removes the “Add Agreement” CTA from the listing. - Introduces new summary card components (FY agreement counts + spending-by-type donut/legend) with unit and e2e coverage.
- Expands the backend agreement list schema to include additional budget line item fields needed for the spending totals.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/pages/agreements/list/AgreementsList.jsx | Wires the new summary cards section into the agreements list page; removes “Add Agreement” button/link. |
| frontend/src/helpers/agreement.helpers.js | Adds helpers to compute agreement spending totals grouped by agreement type. |
| frontend/src/components/UI/Tag/Tag.jsx | Adds new active tag styles for agreement-type legend highlighting. |
| frontend/src/components/UI/DataViz/ResponsiveDonutWithInnerPercent/ResponsiveDonutWithInnerPercent.jsx | Updates donut chart a11y handling using a MutationObserver to set attributes after async SVG render. |
| frontend/src/components/Agreements/AgreementTypeSummaryCard/index.js | Re-export for the new agreement type spending summary card. |
| frontend/src/components/Agreements/AgreementTypeSummaryCard/AgreementTypeSummaryCard.jsx | New donut + legend card showing spending split across agreement types. |
| frontend/src/components/Agreements/AgreementTypeSummaryCard/AgreementTypeSummaryCard.test.jsx | Unit tests for the agreement type spending card. |
| frontend/src/components/Agreements/AgreementSummaryCardsSection/index.js | Re-export for the new summary section wrapper. |
| frontend/src/components/Agreements/AgreementSummaryCardsSection/AgreementSummaryCardsSection.jsx | New wrapper that computes totals and renders both summary cards. |
| frontend/src/components/Agreements/AgreementSummaryCardsSection/AgreementSummaryCardsSection.test.jsx | Unit tests for the wrapper’s computed totals and props passed to child cards. |
| frontend/src/components/Agreements/AgreementFYSpendingSummaryCard/index.js | Re-export for the FY agreement count/breakdown card. |
| frontend/src/components/Agreements/AgreementFYSpendingSummaryCard/AgreementFYSpendingSummaryCard.jsx | New card showing total/new/continuing counts and type breakdown tags. |
| frontend/src/components/Agreements/AgreementFYSpendingSummaryCard/AgreementFYSpendingSummaryCard.test.jsx | Unit tests for FY spending summary card behavior. |
| frontend/sass/uswds/styles.scss | Adds CSS variables/theme color mappings for agreement-type data viz colors. |
| frontend/cypress/e2e/agreementList.cy.js | Updates filter-tag selector assertions and adds e2e coverage for new summary cards. |
| backend/ops_api/ops/schemas/agreements.py | Expands AgreementListResponse.budget_line_items nested fields to include amount/fees/status/is_obe for totals. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| product_service_code = fields.Nested(ProductServiceCodeSchema) | ||
| budget_line_items = fields.List(fields.Nested(BudgetLineItemResponseSchema, only=["id", "status"]), allow_none=True) | ||
| budget_line_items = fields.List( | ||
| fields.Nested(BudgetLineItemResponseSchema, only=["id", "amount", "fees", "status", "is_obe"]), |
There was a problem hiding this comment.
The summary cards are labeled per selected FY (e.g., “FY 2044 Spending by Agreement Type”), but the totals are computed by summing all budget_line_items on each agreement. Since the list response doesn’t include a fiscal_year field on each nested budget line item, the frontend can’t filter totals to the selected FY (and the backend list query only filters which agreements are returned, not which BLIs are nested). Consider including fiscal_year in the nested BLI fields (or returning server-side aggregates by FY/type) so the card can accurately reflect the selected FY.
| fields.Nested(BudgetLineItemResponseSchema, only=["id", "amount", "fees", "status", "is_obe"]), | |
| fields.Nested( | |
| BudgetLineItemResponseSchema, | |
| only=["id", "amount", "fees", "status", "is_obe", "fiscal_year"], | |
| ), |
| it("Agreement summary card counts match the table row count by type", () => { | ||
| // Get the total count from the FY spending card | ||
| cy.get('[data-cy="agreement-fy-spending-summary-card"]').within(() => { | ||
| cy.get(".font-sans-xl.text-bold") | ||
| .invoke("text") | ||
| .then((cardCountText) => { | ||
| const cardTotal = parseInt(cardCountText, 10); | ||
|
|
||
| // Navigate through all pages to count total table rows | ||
| // First, get the total from pagination if it exists | ||
| // The card count should match the total number of agreements across all pages | ||
| expect(cardTotal).to.be.greaterThan(0); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This test case name implies it verifies that summary card counts match the table row counts, but it currently only asserts the card total is > 0 and never compares against the table (or per-type counts). This can create false confidence in the behavior; either add the missing assertions (e.g., compute counts from the rendered table across pagination) or rename the test to reflect what it actually checks.
josbell
left a comment
josbell
left a comment
There was a problem hiding this comment.
PR Review: Agreement List Summary Cards
I've completed a comprehensive review of this PR. Overall, this is a well-implemented feature with strong frontend quality, but there are some issues that need attention.
Summary
This PR adds two summary cards to the agreements list page:
- Agreement Count Summary Card - Shows total agreements with breakdown by type (Contract/Partner/Grant/Direct Obligation) and award status (New/Continuing)
- Agreement Spending Summary Card - Displays spending by type with a donut chart visualization
✅ Strengths
1. Strong Frontend Test Coverage
- Comprehensive unit tests for both new components (190+ lines of test code)
- Multiple E2E tests covering various scenarios
- Edge cases well handled (null data, zero values, single types)
2. Good Architecture
- Clean component separation and reusability
- Backend aggregation computed once before pagination (efficient)
- Proper handling of Partner agreements (AA + IAA grouped together)
- Backend uses existing
agreement_totalproperty (correctly sums amounts + fees)
3. Accessibility
- ARIA labels added to charts and interactive elements
- Semantic HTML structure
4. Data Consistency
- Backend calculation matches frontend logic (non-DRAFT budget lines, includes fees)
- Proper handling of OBE budget lines
⚠️ Issues Found
1. Critical: Missing Backend Tests 🚨
The _compute_agreement_totals() function (lines 825-873 in backend/ops_api/ops/services/agreements.py) has no unit tests. This function performs financial calculations and aggregations that are displayed to users, so testing is critical.
Recommendation: Add unit tests in backend/ops_api/tests/ops/services/test_agreements.py to verify:
- Correct summation by agreement type
- Proper grouping of AA/IAA as partner agreements
- New vs. Continuing classification logic
- Type counts accuracy
- Edge cases (empty results, mixed types, null values)
2. UI Change: "Add Agreement" Button Removed
The PR removes the "Add Agreement" button from the list page without explanation.
Files: frontend/src/pages/agreements/list/AgreementsList.jsx:320-321, 400-401
Questions:
- Is this intentional or accidental?
- Was this discussed with stakeholders?
- Should users still be able to create agreements? If so, where?
3. Potentially Inaccurate Description Text
Changed from "all agreements across OPRE" to "for the selected fiscal year", but when "All" is selected, the text says "for the selected fiscal year" which is misleading.
Location: frontend/src/pages/agreements/list/AgreementsList.jsx:163
Current:
let details = "This is a list of all agreements across OPRE for the selected fiscal year. Draft budget lines are not included in the Totals.";Recommendation: Make it conditional:
let details = `This is a list of all agreements across OPRE${selectedFiscalYear !== "All" ? " for the selected fiscal year" : ""}. Draft budget lines are not included in the Totals.`;4. Minor: Schema Change Exposes More BLI Data
The PR adds amount, fees, and is_obe fields to the BudgetLineItemResponseSchema for list responses. While not necessarily wrong, this increases payload size.
Location: backend/ops_api/ops/schemas/agreements.py:224-226
Question: Are these fields needed by the frontend, or is the backend totals object sufficient?
📋 Additional Observations
1. Commit History
Many merge commits and incremental changes. Consider squashing before merging to main.
2. Unused Helper Functions?
New frontend helper functions added but appear unused:
getContractAgreementBLITotalgetPartnerAgreementBLITotalgetGrantAgreementBLITotalgetDirectObligationAgreementBLITotal
These calculate totals from frontend data, but the component uses backend-computed totals. Consider removing if truly unused.
3. E2E Test Quality ✅
Well-structured E2E tests that verify:
- Card rendering and data accuracy
- Fiscal year filter updates
- Percentage calculations sum to ~100%
🎯 Recommendations
Must Fix:
- ✋ Add backend unit tests for
_compute_agreement_totals() - ✋ Clarify intent of "Add Agreement" button removal
Should Fix:
3. Fix description text to be conditional on fiscal year selection
4. Remove unused helper functions if they're not needed
Nice to Have:
5. Squash commits before merging
6. Consider if the extra BLI schema fields are necessary
Overall Assessment
This is a well-implemented feature with strong frontend quality, but needs backend test coverage before merging. The missing tests represent a risk for financial calculations that are displayed to users. The UI changes (button removal, description text) also need clarification.
🤖 Generated with Claude Code
|
🎉 This PR is included in version 1.318.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
What changed
Added new summary cards to the agreement list page.
Issue
#4142
How to test
Screenshots
Definition of Done Checklist