feat: enable grant agreement editing on detail page - #6089
Conversation
There was a problem hiding this comment.
Pull request overview
Enables grant agreement editing across agreement details, grant numbers, budget lines, lifecycle controls, and navigation safeguards.
Changes:
- Enables grant metadata and budget-line editing with lifecycle and authorization controls.
- Adds unsaved-change navigation handling and deleted-association reconciliation.
- Expands frontend, backend, E2E, and seed-data coverage.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
frontend/src/types/ProcurementTrackerTypes.d.ts |
Reformats tracker step types. |
frontend/src/pages/agreements/review/ReviewAgreement.hooks.test.js |
Tests grant validation behavior. |
frontend/src/pages/agreements/review/ReviewAgreement.hooks.js |
Adjusts grant approval validation. |
frontend/src/pages/agreements/review/EditAgreementAndBudgetLines.test.jsx |
Mocks navigation blocking. |
frontend/src/pages/agreements/review/EditAgreementAndBudgetLines.jsx |
Adds navigation-away modals. |
frontend/src/pages/agreements/details/AgreementDetails.test.js |
Tests enabled grant editing. |
frontend/src/pages/agreements/details/AgreementDetails.jsx |
Removes grant edit restriction. |
frontend/src/pages/agreements/details/AgreementBudgetLines.test.jsx |
Tests grant lifecycle locks. |
frontend/src/pages/agreements/details/AgreementBudgetLines.jsx |
Enables grant BLI actions. |
frontend/src/hooks/useNavigationBlocker.hooks.js |
Adds approval-aware destinations. |
frontend/src/components/BudgetLineItems/CreateBLIsAndSCs/CreateBLIsAndSCs.jsx |
Exposes dirty state and reconciliation. |
frontend/src/components/BudgetLineItems/CreateBLIsAndSCs/CreateBLIsAndSCs.hooks.test.js |
Tests deleted-link reconciliation. |
frontend/src/components/BudgetLineItems/CreateBLIsAndSCs/CreateBLIsAndSCs.hooks.js |
Disassociates deleted component links. |
frontend/src/components/BudgetLineItems/CreateBLIsAndSCs/CreateBLIAndSCs.test.js |
Tests lifecycle-based editing visibility. |
frontend/src/components/BudgetLineItems/BudgetLinesForm/BudgetLinesForm.test.jsx |
Covers grant BLI submission. |
frontend/src/components/Agreements/AgreementEditor/AgreementEditorContext.jsx |
Reseeds asynchronously loaded officers. |
frontend/src/components/Agreements/AgreementEditor/AgreementEditorContext.hooks.test.js |
Tests officer reseeding. |
frontend/src/components/Agreements/AgreementEditor/AgreementEditForm.hooks.test.js |
Tests grant field persistence. |
frontend/src/components/Agreements/AgreementDetailHeader.jsx |
Enables grant detail editing. |
frontend/src/components/Agreements/AgreementBudgetLinesHeader.jsx |
Adds lifecycle-specific locking. |
frontend/cypress/e2e/editGrantAgreement.cy.js |
Adds grant editing E2E coverage. |
backend/ops_api/tests/ops/grant_numbers/test_grant_number.py |
Tests deletion disassociation. |
backend/ops_api/tests/ops/budget_line_items/test_grant_budget_line_item.py |
Covers grant BLI endpoints. |
backend/ops_api/tests/ops/agreement/test_agreement.py |
Covers grant edits and authorization. |
backend/data_tools/src/import_static_data/import_data.py |
Imports grant seed entities. |
backend/data_tools/data/agreements_and_blin_data.json5 |
Adds grant testing data. |
Suppressed comments (1)
frontend/src/components/Agreements/AgreementEditor/AgreementEditorContext.jsx:103
- The same late-response race applies to the alternate project officer: a user selection made before the original officer fetch resolves is unconditionally overwritten here. Guard reseeding on untouched initial state or track field edits before applying the fetched value.
useEffect(() => {
if (alternateProjectOfficer?.id) {
dispatch({ type: "SET_STATE", key: "selected_alternate_project_officer", value: alternateProjectOfficer });
}
}, [alternateProjectOfficer]);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Bypass budget-line validation for grants (OPS-6013); an empty result set means no BLI | ||
| // errors gate the status change. | ||
| if (isGrant) { | ||
| return []; |
There was a problem hiding this comment.
The bypass is intentional by design. The suite's grant_number_id > 0 check would block every DRAFT→PLANNED status change for a grant BLI not yet linked to a grant number — but linking isn't a prerequisite for draft status changes. The frontend BLI validator is optimized for contract agreements (SC assignment, procurement-shop constraints) and doesn't have an equivalent grant-specific model. The backend is the authoritative safety net for required-field enforcement per transition. Updated the comment in the code to make this reasoning explicit.
| // useBlocker is used by useNavigationBlocker; stub it to return an idle state | ||
| // so the nav-away modal never fires in unit tests (integration/E2E covers that). | ||
| useBlocker: () => ({ state: "idle", location: null, proceed: vi.fn(), reset: vi.fn() }) |
There was a problem hiding this comment.
The blocker modal save/proceed flow is covered at the unit level in EditAgreementAndBudgetLines.test.jsx (nav-away blocker modal describe block — 3 tests: save-and-proceed, leave-without-saving, approval-copy variant). E2E coverage of the blocker via Cypress would require real navigation interception which is out of scope for this story slice — the unit tests are the appropriate layer per the project's test-type decision matrix.
| useEffect(() => { | ||
| if (projectOfficer?.id) { | ||
| dispatch({ type: "SET_STATE", key: "selected_project_officer", value: projectOfficer }); | ||
| } | ||
| }, [projectOfficer]); |
There was a problem hiding this comment.
Acknowledged — the race is documented in the existing code comment ("Known race: if the user changes the officer field before getUser() resolves..."). The fetch is fast relative to the time it takes to navigate to and interact with the officer field, making this a low-likelihood scenario in practice. Fixing it properly (tracking whether the field has been edited before dispatching) is out of scope for this story; the comment flags it for a future pass.
|
Suggestions per the AI code review:
|
|
Scope discrepancy — PR body describes a backend change that isn't in the diff The PR description dedicates a whole section to backend/ops_api/ops/services/budget_line_items.py (inlining_validation_change_status_higher_than_draft, removing _validate_date_within_sc_window). That file is not in the PR's changed-files list, and the methods still exist on main/the branch tip. Either the change was dropped/reverted before pushing, landed in a separate PR, or the description is stale. This matters because the frontend leans on backend validation as the safety net (see next point). Reviewers should confirm whether that SC-window removal is actually intended to ship here. Recommend either restoring the change or trimming the description. |
…irty state after bundle save
… after main merge
… reducer, fix E2E aln field
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
frontend/src/components/Agreements/AgreementEditor/AgreementEditorContext.jsx:164
- The async officer response can overwrite a selection the user makes while the request is in flight, as the preceding comment acknowledges. That silently restores the old officer and can save the wrong value. Track whether each field has been touched (or otherwise ignore stale fetch results) before dispatching the reseed; timing alone is not a safe guard.
useEffect(() => {
if (projectOfficer?.id) {
dispatch({ type: "SET_STATE", key: "selected_project_officer", value: projectOfficer });
}
}, [projectOfficer]);
frontend/src/components/Agreements/AgreementEditor/AgreementEditorContext.hooks.js:225
- Changing this collection from IDs to full objects breaks the existing approval-message path in
CreateBLIsAndSCs.hooks.js:568-570, which still treats each entry as an ID (budgetLines.find((bl) => bl.id === id)). With the real reducer, every lookup now returnsundefined, so deleting a PLANNED/IN_EXECUTION BLI is incorrectly reported as “Agreement Updated” instead of “Changes Sent to Approval.” Update that consumer and its tests to use the object shape (or preserve the ID contract separately).
// Store the full BLI object so the save path can inspect its status for
// approval routing (isDeletionRoutedToApproval). The deletion API call
// uses .id from each object; see handleDeletions in CreateBLIsAndSCs.hooks.js.
deleted_budget_line_items_ids: action.payload.id
? [...state.deleted_budget_line_items_ids, action.payload]
frontend/src/components/Agreements/AgreementEditor/AgreementEditForm.hooks.test.js:709
- This test does not cover the real ALN save field: editor state and the API schema use
aln_numbers(plural), but the fixture and assertion usealn_number, so the test only verifies that an unrelated property survives the helper. It also usesfunding_period_months: 24, while the backend accepts only 12 or 18. Usealn_numberswith the API's integer-array shape and a valid funding period so this test exercises a request the server can actually persist.
nofo_number: "NOFO-UPDATED",
aln_number: "10.001",
funding_period_months: 24,
frontend/src/components/Agreements/AgreementEditor/AgreementEditorContext.hooks.js:123
- This reconciliation only handles persisted BLIs with a
services_component_id. A BLI added during the current edit has a temporary ID and onlyservices_component_number; deleting its newly added services component leaves that number/grouping intact, so it remains displayed under a component that no longer exists. Clear the link when either the persisted ID is removed or the BLI's component number matches the deleted component.
budget_line_items: state.budget_line_items.map((bli) => {
if (bli.services_component_id != null && !remainingScIds.has(bli.services_component_id)) {
frontend/src/components/Agreements/AgreementEditor/AgreementEditorContext.hooks.js:193
- This misses BLIs created during the current edit: those carry
grant_number_numberbut nogrant_number_id. Deleting their newly added grant number therefore leaves them grouped under the removed number until save, rather than moving them to “not associated” as intended. Reconcile both persisted ID links and temporary number-based links.
budget_line_items: state.budget_line_items.map((bli) => {
if (bli.grant_number_id != null && !remainingGnIds.has(bli.grant_number_id)) {
return { ...bli, grant_number_id: null, grant_number_number: 0 };
What changed
Enables editing of existing Grant agreements end-to-end. Previously, Grant editing was explicitly blocked in the UI with a "not developed yet" message. This PR lifts that restriction and wires up the full edit flow — metadata, grant numbers, and budget lines.
Backend
GrantNumberwith authorization, IDOR protection, cascade/disassociation behavior (deleting a grant number NULLsgrant_number_idon its BLIs rather than blocking the delete), uniqueness enforcement, and audit history (GRANT_NUMBER_CREATED/UPDATED/DELETED).GrantBudgetLineItemPUT/PATCH/DELETE with required-field validation and role/authorization checks, mirroring contract BLI coverage.grant_number_idadded toALWAYS_DIRECT_EDIT_FIELDSso reassigning a BLI to a different grant number does not route through the change-request approval flow.PATCH /agreements/{id}/edit-bundlesupport for grant numbers (create/update/delete in one request with rollback on failure).Frontend
AgreementTableRow,AgreementDetailHeader,AgreementBudgetLinesHeader,AgreementBudgetLines.AgreementEditFormrenders grant-specific fields (NOFO number, total funding, funding period, ALN) and hides contract-only fields (contract type, procurement shop, vendor) when agreement type is GRANT.AgreementEditorContextfixes an async race where project officer fields rendered empty on edit because thegetUser()fetch hadn't resolved at mount time.DELETE_GRANT_NUMBERreducer action reconciles BLIs in state immediately, moving them to the "not associated" group so the accordion is consistent before saving.EditAgreementAndBudgetLineswires up the nav-away blocker (useNavigationBlocker) with an approval variant (when pending changes require DD approval).fireBundleSavenow returnstrue/falseso a failed save keeps the blocker modal open rather than silently proceeding.grant_number_idrequirement would block DRAFT→PLANNED transitions for unlinked BLIs; the backend is the authoritative validator per transition.editGrantAgreement.cy.js): happy path (metadata edit, grant number edit, BLI edit, delete-grant-number-moves-BLI-to-not-associated), and unauthorized user blocked.Issue
#6013
How to test
Pre-requisites
Edit from agreements table (new — was blocked before)
Go to /agreements
Find a Grant agreement in the list
Hover the row — confirm the edit pencil icon is enabled and clickable
Click it — confirm you land on the edit page (not a blocked/disabled state)
Edit grant metadata
Go to the grant's detail page (/agreements/)
Click Edit
Confirm grant-specific fields are present: NOFO Number, Total Funding, Nickname, Description (no Procurement Shop, no Contract Type, no Vendor)
Change the NOFO number to something new
Click Continue / Save
Confirm success alert appears
Navigate back — confirm the updated NOFO is shown
Add a grant number
Go to /agreements//budget-lines
Click Edit
In the Grant Numbers section, click Add Grant Number
Fill in a number and description
Save — confirm the new accordion appears with the grant number's label
Edit a grant number
Enter edit mode on the budget-lines page
Hover over an existing grant number row — click the edit icon
Change the description
Save — confirm the updated description is shown
Delete a grant number that has BLIs
Enter edit mode on the budget-lines page
Note which BLIs are under a grant number accordion (e.g. "Grant 1")
Delete that grant number
Confirm:
Add / edit / delete a budget line
Enter edit mode on the budget-lines page
Add a new BLI under a grant number accordion — fill in CAN, amount, date
Save — confirm it appears in the list and the grant total updates
Re-enter edit mode — edit the amount on an existing BLI
Save — confirm the updated amount and totals
Re-enter edit mode — delete a DRAFT BLI
Save — confirm it's gone
Nav-away blocker — unsaved changes
Enter edit mode, make a change (e.g. edit a BLI amount) — do not save
Click the browser back button or a nav link
Confirm the "Save changes before leaving?" modal appears
Click Leave without saving — confirm you navigate away and the change is discarded
Repeat, this time click Save in the modal — confirm the save succeeds and you land at the
Nav-away blocker — failed save keeps modal open
Enter edit mode, make a change
Navigate away to trigger the blocker modal
With browser devtools, block the PATCH /agreements/.../edit-bundle request (network throttle → offline, or use a request blocker)
Click Save in the modal
Confirm: the error alert appears, the modal stays open, and you remain on the edit page (not navigated away)
Lifecycle lock — in review
Put the grant's BLIs into a state that triggers review (PLANNED BLIs with a pending chang
Navigate to the detail/budget-lines page
Confirm the Edit button is disabled with an appropriate tooltip (not the "not available fgone)
Authorization — unauthorized user
Log in as a user who is not a team member on the grant
Go to the grant's detail page — confirm Edit button is absent or disabled
Authorization — unauthorized user
Log in as a user who is not a team member on the grant
Go to the grant's detail page — confirm Edit button is absent or disabled
Directly navigate to /agreements/edit/ — confirm an error/access-denied state
Go to /agreements — hover the grant row — confirm the edit icon is disabled
Audit history
Make a metadata change, a grant number change, and a BLI change
Navigate to the agreement's history/activity log
Confirm entries for each change show the correct user, field, and timestamp
A11y impact
cy.checkA11y()runs on the grant edit flow in the E2E suiteStorybook
Screenshots
N/A
Definition of Done Checklist
Links
N/A