Delete Planned/Executing budget line items via change request#5833
Delete Planned/Executing budget line items via change request#5833jonnalley wants to merge 1 commit into
Conversation
Deleting a PLANNED or IN_EXECUTION budget line item now routes through a
Division Director approval change request instead of a hard delete. DRAFT
BLIs (and any delete by a super user) still delete immediately.
Backend:
- BudgetLineItemChangeRequest gains a has_delete_change hybrid + to_dict
exposure; a deletion request carries a {"delete": true} sentinel with diff
{"delete": {"old": <amount>, "new": "Deleted"}}.
- service.delete() returns (bli, status_code): DRAFT/super -> 200 immediate;
PLANNED/IN_EXECUTION -> 202 + deletion CR via the shared editability gate
(blocks in-review, OBE, Pre-Award/Award step >= 5). Resource emits DELETE_BLI
only on the immediate path.
- On approval the CR is finalized and the submitter notified first, then the
CR's serialized form is snapshotted while still attached, the CR is detached,
and the BLI is deleted last (cascade removes the CR row) so the post-commit
re-serialization never touches a detached/deleted instance.
- create_change_request_history_event handles the "delete" sentinel (request /
approved / declined) and emits BUDGET_LINE_ITEM_DELETED history.
- build_approve_url + change-request schema + openapi.yml updated.
Frontend:
- delete mutation surfaces the HTTP status; the budget-lines table shows the
reused "Changes Sent to Approval" alert on 202.
- the For-Review BudgetChangeReviewCard renders deletion CRs (Change To/To =
"Deleted", From = the BLI amount) via has_delete_change.
Tests: integration (immediate/CR/blocked/approve/reject/unauthorized) + BDD +
frontend card/helper/hook coverage, all with history-message assertions.
Issue: #5819
jonnalley
requested review from
Santi-3rd,
fpigeonjr,
josbell,
rajohnson90 and
weimiao67
as code owners
jonnalley
changed the title
josbell
left a comment
josbell
left a comment
There was a problem hiding this comment.
Code Review: Approved ✓
Reviewed implementation of deletion change request workflow. The code correctly handles the documented cascade-delete and audit history challenges (BLOCKER-1 & BLOCKER-2).
Optional Recommendations
Defense in depth: Consider adding a partial unique index on change_request(budget_line_item_id) WHERE status='IN_REVIEW' to prevent theoretical duplicate change requests during concurrent deletes. The edit path uses the same check-then-create pattern without known issues, so this is optional.
UX improvement: The delete error message is generic ("not in a deletable state"). Consider reusing the existing get_bli_locked_message() helper to provide specific reasons (e.g., "agreement has reached Pre-Award").
Both recommendations are non-blocking polish items. The PR is production-ready as-is.
rajohnson90
left a comment
rajohnson90
left a comment
There was a problem hiding this comment.
AI Code Review suggests that you're missing a test for super user bypass in the test suite. I saw that it is covered in the code, but it should definitely be tested. Otherwise ready to approve.
3 participants
What changed
This is PR 2 of 2 for issue #5819. PR 1 (#5832) made IN_EXECUTION budget line items (BLIs)
editable. This PR makes deleting a PLANNED or IN_EXECUTION BLI go through a Division Director
approval change request instead of a hard delete. DRAFT BLIs — and any delete by a super user —
still delete immediately, unchanged.
Backend
models/change_requests.py—BudgetLineItemChangeRequestgains ahas_delete_changehybrid(and exposes it in
to_dict()). A deletion request is a normalBUDGET_LINE_ITEM_CHANGE_REQUESTcarrying a
{"delete": true}sentinel inrequested_change_data, withrequested_change_diff = {"delete": {"old": <bli amount>, "new": "Deleted"}}.services/budget_line_items.py—delete()now returns(bli, status_code). DRAFT/super →200immediate hard delete; PLANNED/IN_EXECUTION →202+ a deletion CR, gated by the sameshared
compute_bli_editablerules as editing (blocked while in review, OBE, or once the agreementreaches Pre-Award/Award, i.e. procurement tracker step ≥ 5).
resources/budget_line_items.py— the delete handler unpacks(bli, status_code)and emitsthe
DELETE_BLIaudit event only on the immediate (200) path; the 202 path emitsCREATE_CHANGE_REQUESTinside the service, so submitting a request does not write a spurious"Budget Line Deleted" history record.
services/change_requests.py—add_bli_delete_change_request()creates the CR; the approvalflow deletes the BLI last (see BLOCKER-1 below).
models/agreement_history.py—create_change_request_history_eventhandles the"delete"sentinel for request-created / approved / declined, emitting
BUDGET_LINE_ITEM_DELETEDhistory.utils/change_requests_helpers.py—build_approve_urllearns the delete case.schemas/change_requests.py,resources/change_requests.py,openapi.yml— surfacehas_delete_changeand document the DELETE 202 path.Frontend
api/opsAPI.js— the delete mutation surfaces the HTTP status so callers distinguish 200(immediate) from 202 (request created); invalidates
ChangeRequests.AllBudgetLinesTable.hooks.js— on 202 shows the same "Changes Sent to Approval" alert theedit-via-CR path uses (verbatim); on 200 keeps "...successfully deleted."
BudgetChangeReviewCardnow renders deletion CRs (gate ishas_budget_change || has_delete_change);changeToTypes.delete = "Deleted",KEY_NAMES.DELETE, and arenderChangeValuesdelete case render Change To = "Deleted",From = the BLI amount as currency, To = "Deleted" (matches the approved design).
Key design decisions / assumptions (for reviewers without context)
BudgetLineItemChangeRequestwith a JSONB{"delete": true}sentinel — no newChangeRequestType, no enum migration, and it rides theexisting routing / approval / notification /
in_reviewplumbing.change_request.budget_line_item_idisON DELETE CASCADE, andthe resource re-serializes the CR (
to_dict()) after approval. Naively deleting the BLI mid-flowwould cascade-delete the CR row and crash the post-commit reads. So on approve we finalize the CR
and notify the submitter while it still exists, snapshot
to_dict()while the CR is stillattached (capturing every relationship the auto-schema serializes, including the
sqlalchemy-continuum
versionsdynamic relationship), detach the CR, then delete the BLI last andpin the CR's
to_dict()to the snapshot. The response/event never serialize a detached instance.create_change_request_history_eventneeded an explicit"delete"branch (it would otherwise hitan
UnboundLocalErroron the unset title/message). The requestor is resolved via thecreated_bycolumn, not the relationship-derived dict (which is
Noneonce the CR is detached/cascade-deleted).isDeletableas the seam): DRAFT /PLANNED / IN_EXECUTION are deletable when editable; OBLIGATED is not; super users always.
deleted on approval. The durable record of the deletion is the
AgreementHistoryBUDGET_LINE_ITEM_DELETEDentry plus the automaticOpsDBHistorytrail.Review process
Two rounds of adversarial multi-agent consensus review were run against this diff. Round 1 surfaced
1 MAJOR (the detached-CR
versionsserialization gap — fixed via the snapshot approach above) + 3MINOR (history-message test-coverage gaps — all added) + 1 NIT (response-body assertion — added).
Round 2 returned zero confirmed issues.
Issue
#5819
How to test
Backend (from
backend/ops_api/, with the test stack):Covers: DRAFT → 200 immediate; PLANNED/IN_EXECUTION → 202 + deletion CR (BLI survives); blocked at
Pre-Award (400); blocked while a CR is pending (400); approving deletes the BLI + records the
"Budget Line Deleted" history (and the response/notification do not crash on the cascade — BLOCKER-1
guard); rejecting keeps the BLI + records "Budget Line Deletion Declined"; unauthorized → 403.
Frontend (from
frontend/):Manual: as an associated user, on an agreement's budget-lines table, delete a PLANNED or EXECUTING
BLI → confirm the existing modal → expect the "Changes Sent to Approval" alert and the BLI still
present. As the Division Director, open For Review → the deletion shows on a Budget Change card
with Change To / To = "Deleted" and From = the BLI amount; approving removes the BLI.
A11y impact
(Frontend changes reuse existing components/alerts/cards; no new UI primitives or markup patterns.)
Storybook
Screenshots
N/A — reuses the existing confirmation modal, the existing "Changes Sent to Approval" alert, and the
existing
BudgetChangeReviewCard.Definition of Done Checklist
Links