feat: OPS-1639 PR2 - Budget Team Requisition Review Card

[josbell]

What changed

Implements the "For Review" card that displays pre-award requisition review tasks to Budget Team members after Division Director approval.

This is PR2 of the OPS-1639 feature set. PR1 (backend foundation) was completed previously.

Note: This is a clean rebased version of PR #5619 to resolve commitlint issues with duplicate commits from main.

Backend Changes

• New API endpoint: GET /api/v1/procurement-tracker-steps/pending-requisitions/
  • Returns steps where DD approved but budget team hasn't entered requisition
  • Filters by BUDGET_TEAM role only
  • Includes full agreement data with budget line items
• Service method: get_pending_requisitions_for_user(user_id)
• Schema enhancements: Added nested BLI schema with date_needed field
• Notification link: Added markdown link to review page in budget team notifications

Frontend Changes

• RTK Query hook: useGetPendingBudgetRequisitionsQuery
• New component: BudgetTeamRequisitionReviewCard
  • Displays agreement name, requestor, BLs executing count (badge), totals, dates
  • "Review Agreement" button (navigation will be wired in PR4)
• Integration: Card appears in "For Review" tab after DD approval
• Count badge: Includes budget requisitions in total review count

Issue

Part of OPS-1639 (Budget Team Requisition Approval)

How to test

1. Backend API:

   # Run backend tests
   cd backend/ops_api
   pipenv run pytest tests/ops/procurement_tracker/test_budget_team_requisition_review_card.py

2. Frontend Component:

   # Run frontend tests
   cd frontend
   bun run test --watch=false BudgetTeamRequisitionReviewCard

3. Manual Testing:

   • Login as a Budget Team member
   • Navigate to "For Review" tab
   • Verify requisition review card appears after DD approval
   • Check that card displays correct agreement details and counts

A11y impact

• No accessibility-impacting changes in this PR

Screenshots

N/A - Card UI follows existing design system patterns

Definition of Done Checklist

• OESA: Code refactored for clarity
• OESA: Dependency rules followed
• Automated unit tests updated and passed
• Automated integration tests updated and passed
• Automated quality tests updated and passed
• Automated load tests updated and passed (N/A)
• Automated a11y tests updated and passed
• Automated security tests updated and passed
• 90%+ Code coverage achieved
• Form validations updated (N/A - read-only card)

Links

• Related: PR1 (OPS-1639 backend foundation) - merged previously
• Next: PR3 (Budget team approval page)
• Next: PR4 (Wire navigation from card to approval page)

[Copilot]

Pull request overview

Implements OPS-1639 PR2 support for Budget Team “For Review” work by adding a new backend endpoint to fetch pending pre-award requisition tasks (DD-approved, requisition not yet entered) and a new frontend review card surfaced in the Change Requests “For Review” tab, including updated total-count logic and mocks/tests.

Changes:

• Backend: add GET /api/v1/procurement-tracker-steps/pending-requisitions/, service query, and schema support to include agreement + budget line item data.
• Frontend: add useGetPendingBudgetRequisitionsQuery, render BudgetTeamRequisitionReviewCard in the review list, and include budget requisitions in the overall review-count hook.
• Tests/mocks: MSW handler for the new endpoint plus new/updated unit tests.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
frontend/src/mocks/handlers.js	Adds MSW mock for pending budget requisitions endpoint.
frontend/src/hooks/useChangeRequests.hooks.test.js	Updates hook tests to account for the new pending budget requisitions query.
frontend/src/hooks/useChangeRequests.hooks.js	Includes pending budget requisitions in the total “items needing review” count.
frontend/src/components/ChangeRequests/PreAwardReviewCard/PreAwardReviewCard.jsx	Adds clarification comment about not using the shared ReviewCard base component.
frontend/src/components/ChangeRequests/ChangeRequestsList/ChangeRequestsList.jsx	Fetches and renders budget requisition review cards and updates loading/error handling.
frontend/src/components/ChangeRequests/ChangeRequestsList/ChangeRequestList.test.jsx	Updates list tests to mock the new query.
frontend/src/components/ChangeRequests/BudgetTeamRequisitionReviewCard/index.js	Exports the new card component.
frontend/src/components/ChangeRequests/BudgetTeamRequisitionReviewCard/BudgetTeamRequisitionReviewCard.test.jsx	Adds unit tests for the new budget team requisition review card.
frontend/src/components/ChangeRequests/BudgetTeamRequisitionReviewCard/BudgetTeamRequisitionReviewCard.jsx	Introduces the UI component for budget team requisition review tasks.
frontend/src/api/opsAPI.js	Adds RTK Query endpoint/hook for pending budget requisitions.
backend/ops_api/tests/ops/procurement_tracker/test_budget_team_requisition_review_card.py	Adds backend tests for the new pending requisitions endpoint.
backend/ops_api/ops/views.py	Registers the pending requisitions view function.
backend/ops_api/ops/urls.py	Adds URL rule for /procurement-tracker-steps/pending-requisitions/.
backend/ops_api/ops/services/procurement_tracker_steps.py	Adds notification link and implements get_pending_requisitions_for_user (but currently introduces a regression in pending approvals).
backend/ops_api/ops/schemas/procurement_tracker_steps.py	Extends nested agreement schema to include budget line items and agreement total.
backend/ops_api/ops/resources/procurement_tracker_steps.py	Adds API resource class for pending requisitions list endpoint.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[rajohnson90]

This review was generated by claude and I went in to manually verify concerns just to double check.

Backend Analysis

API Resource (ops/resources/procurement_tracker_steps.py)

New endpoint: GET /api/v1/procurement-tracker-steps/pending-requisitions/

Strengths:

• ✅ Follows existing pattern (PendingApprovalsAPI)
• ✅ Proper authorization with @is_authorized decorator
• ✅ User validation with clear 401 response
• ✅ Good logging for debugging

Issues:

1. ⚠️ Authorization scope mismatch (line 164):
   @is_authorized(PermissionType.GET, Permission.AGREEMENT)
2. This uses generic GET permission on AGREEMENT. Should this be a more specific permission like Permission.PROCUREMENT_TRACKER_STEP or a new
   GET_PENDING_REQUISITIONS scope? The role-based filtering happens in the service layer, but the authorization decorator should match the actual
   permission model.
3. Minor: Redundant user check (line 166-168):
   The hasattr(current_user, "id") check is defensive but likely unnecessary if get_current_user() is reliable. Consider whether this should be
   an assertion or if it's needed at all.

Service Layer (ops/services/procurement_tracker_steps.py)

Method: get_pending_requisitions_for_user(user_id)

Strengths:

• ✅ Clear docstring with filter criteria
• ✅ Role-based filtering (BUDGET_TEAM only)
• ✅ Proper eager loading with selectinload to avoid N+1 queries
• ✅ Correct query conditions for "DD approved, requisition not entered"
• ✅ Orders by pre_award_approval_responded_date (most recent first)

Issues:

1. 🔴 Critical: Import placement (line 790-795):
   from sqlalchemy import and_

from models import (
Agreement,
DefaultProcurementTrackerStep,
ProcurementTracker,
)

1. Local imports inside a method violate PEP 8 and project conventions. These should be at module level. This is a red flag that could
   indicate circular import issues. If there's a circular dependency, it should be resolved properly, not worked around with local imports.
2. ⚠️ Model inconsistency (line 821):
   DefaultProcurementTrackerStep.pre_award_approval_status == "APPROVED"
3. Using string literal "APPROVED" instead of enum. Should this be ProcurementTrackerStepStatus.APPROVED or similar? Check if there's an enum
   for approval status.

Schema Updates (ops/schemas/procurement_tracker_steps.py)

Strengths:

• ✅ Clean nested schema structure
• ✅ Includes date_needed field for BLI (needed for obligate-by calculation)
• ✅ Adds agreement_total as dump-only field

Minor observations:

• The NestedBudgetLineItemSchema is minimal but sufficient for card display
• Good use of dump_only=True for calculated fields

Notification Enhancement (ops/services/procurement_tracker_steps.py)

Line 510-512: Adds markdown link to notification message:
f"Review Agreement"

Strengths:

• ✅ Uses configured frontend URL
• ✅ Markdown link format for rich notifications

Issue:

• ⚠️ The review URL points to /agreements/{id}/review-budget-requisition which the PR description states will be "wired in PR4". This creates
  a temporary dead link. Consider:
  • Adding a comment noting this is a placeholder for PR4
  • Or deferring this notification change to PR4

---

Frontend Analysis

API Integration (api/opsAPI.js)

Strengths:

• ✅ Adds new Budget Requisitions tag for cache invalidation
• ✅ Follows existing RTK Query patterns
• ✅ Proper export of new hook

Component: BudgetTeamRequisitionReviewCard

Strengths:

• ✅ Excellent inline documentation explaining why it doesn't use ReviewCard base component
• ✅ Props are well-typed with PropTypes
• ✅ Uses existing design system components (Tag, FontAwesome icons)
• ✅ Consistent styling with other review cards
• ✅ Good use of custom hooks (useGetAgreementName, useGetUserFullNameFromId)

Issues:

1. ⚠️ Button has no action (line 113-125):
   <button
   type="button"
   className="usa-button--unstyled text-primary font-12px cursor-pointer"
   data-cy="review-agreement-button"
2. The button has no onClick handler. Per the PR description, "navigation will be wired in PR4", but this creates a dead button. Consider:
   - Adding a comment above the button noting it will be wired in PR4
   - Or adding disabled attribute with a title tooltip explaining it's coming soon
3. Minor: Conditional rendering (line 65):
   The isCondensed mode removes the header but is never used in this PR. Is this for future use? If so, consider adding a comment.

Integration: ChangeRequestsList.jsx

Strengths:

• ✅ Properly fetches budgetRequisitions data
• ✅ Error handling includes new query
• ✅ Loading states updated
• ✅ Helper functions (calculateExecutingBliCount, calculateExecutingTotal, getObligateByDate) are well-implemented

Issues:

1. ⚠️ Magic string (line 671):
   budgetLineItems.filter((bli) => bli.status === "In Execution")
2. String literal "In Execution" should be a constant or enum. Does the codebase have a BudgetLineItemStatus constant in JS? If not, consider
   extracting to a constant.
3. Edge case: getObligateByDate returns ISO date string, but what if all executing BLIs have null date_needed? The function returns null,
   which is correct, but ensure the card component handles this gracefully (which it does with the conditional rendering).

Hooks: useChangeRequests.hooks.js

Strengths:

• ✅ Properly includes budget requisitions in total count
• ✅ Skips query if no userId

---

Recommendations

Must Fix (Blocking):

1. 🔴 Move local imports to module level in procurement_tracker_steps.py:790. If there's a circular dependency, resolve it properly.

Should Fix (Important):

2. ⚠️ Replace string literals with enums/constants:
   - Backend: "APPROVED" → use enum
   - Frontend: "In Execution" → use constant
3. ⚠️ Review authorization permission scope - should it be Permission.PROCUREMENT_TRACKER_STEP or remain Permission.AGREEMENT?
4. ⚠️ Add comments for incomplete features:
   - Button with no onClick handler (line ~113 in BudgetTeamRequisitionReviewCard.jsx)
   - Notification link to non-existent page (line ~512 in procurement_tracker_steps.py)

Nice to Have:

5. Consider extracting calculateExecutingBliCount, calculateExecutingTotal, getObligateByDate to a shared helper file if they'll be reused in
   PR3/PR4.
6. The isCondensed prop on the card is never used - either use it or remove it in this PR.

---

Summary

Overall assessment: This is a solid, well-structured PR that follows project conventions and integrates cleanly with the existing codebase.
The implementation is straightforward and the test coverage is comprehensive.

Critical issue: The local imports in the service method must be addressed before merging.

Minor issues: String literals for statuses and incomplete feature placeholders should be documented or improved.

Once the critical import issue is resolved and string literals are replaced with constants, this PR will be ready to merge. Great work on the
clean separation between backend/frontend and the thoughtful test coverage! 🎉

The issue of imports in functions is something I've generally tried to avoid when using claude on my own as well. I think it is best practice to just place all imports in the same spot at the top of the file.

[josbell]

Code Review Response - PR #5634

Backend Analysis

API Resource (ops/resources/procurement_tracker_steps.py)

⚠️ Authorization scope mismatch (line 164)

Review Comment: Questions whether @is_authorized(PermissionType.GET, Permission.AGREEMENT) should be Permission.PROCUREMENT_TRACKER_STEP

Response: ✅ Not an issue - existing pattern
This follows the established codebase pattern. The ProcurementTrackerStepItemAPI class uses the same Permission.AGREEMENT authorization at lines 36 and 48. Procurement tracker steps are conceptually part of agreement management, so this authorization scope is consistent and intentional.

Minor: Redundant user check (line 166-168)

Review Comment: The hasattr(current_user, "id") check may be unnecessary

Response: ✅ Intentional defensive programming
While likely unnecessary given get_current_user() reliability, this defensive check is not harmful and follows defensive programming practices. Keeping it provides an extra safety layer with no performance cost.

---

Service Layer (ops/services/procurement_tracker_steps.py)

🔴 Critical: Import placement (line 790-795)

Review Comment: Local imports inside method violate PEP 8

Response: ✅ FIXED in commit 10a74b6
Moved all local imports to module level:

• and_ → added to sqlalchemy imports (line 8)
• Agreement, DefaultProcurementTrackerStep, ProcurementTracker → added to models imports (lines 12, 14, 20)
• Removed imports from inside get_pending_requisitions_for_user() method

⚠️ Model inconsistency (line 821)

Review Comment: Using string literal "APPROVED" instead of enum

Response: ⚠️ Deferred to future refactoring
This is a valid concern, but the pattern is used consistently throughout the codebase in 4+ locations (lines 223, 448, 451, 837). There's no existing enum for pre-award approval status - these values are stored as strings in the database. Creating an enum would require:

1. Defining a new PreAwardApprovalStatus enum
2. Updating multiple files and methods
3. Potentially requiring a database migration

Recommendation: Track as a separate refactoring ticket to avoid scope creep in this PR.

---

Schema Updates (ops/schemas/procurement_tracker_steps.py)

Review Comment: Minor observations about schema structure

Response: ✅ Working as intended
The nested schema structure is minimal but sufficient for the card display requirements. The dump_only=True for calculated fields is correct usage.

---

Notification Enhancement (ops/services/procurement_tracker_steps.py)

⚠️ Dead link in notification (line 510-512)

Review Comment: Review URL points to /agreements/{id}/review-budget-requisition which doesn't exist yet

Response: ✅ FIXED in commit 10a74b6
Added TODO comment at line 514:

# TODO (PR3/PR4): This URL points to budget requisition review page to be implemented

This documents that the page will be implemented in PR3/PR4 as stated in the PR description.

---

Frontend Analysis

API Integration (api/opsAPI.js)

Review Comment: Follows existing patterns

Response: ✅ Confirmed
No issues identified. Implementation follows RTK Query patterns correctly.

---

Component: BudgetTeamRequisitionReviewCard

⚠️ Button has no action (line 113-125)

Review Comment: Button has no onClick handler

Response: ✅ FIXED in commit 10a74b6
Added TODO comment at line 113:

/* TODO (PR4): Wire navigation to budget requisition review page */

Per PR description, navigation will be wired in PR4. The comment now documents this clearly.

Minor: isCondensed prop unused

Review Comment: The isCondensed mode is never used in this PR

Response: ✅ Intentional forward compatibility
The prop is defined with PropTypes and has conditional logic (lines 54, 111). This is forward-looking design for potential future use. Removing it now would require adding it back later. Not an issue.

---

Integration: ChangeRequestsList.jsx

⚠️ Magic string (line 671)

Review Comment: String literal "In Execution" should be a constant

Response: ✅ FIXED in commit 10a74b6
Extracted constant at top of file:

const BLI_STATUS_IN_EXECUTION = "In Execution";

Replaced all 3 occurrences (lines 49, 54, 59) with the constant. This matches the backend enum BudgetLineItemStatus.IN_EXECUTION = "In Execution".

Edge case: getObligateByDate

Review Comment: What if all executing BLIs have null date_needed?

Response: ✅ Already handled correctly
The function correctly returns null when no dates are found, and the card component handles this gracefully with conditional rendering ({obligateByDate && ...} at line 91-95 of the card component).

---

Hooks: useChangeRequests.hooks.js

Review Comment: Properly includes budget requisitions in count

Response: ✅ Confirmed
No issues identified. Implementation is correct.

---

Summary of Actions Taken

✅ Fixed (Commit 10a74b6):

1. 🔴 Critical: Moved local imports to module level
2. ⚠️ Important: Added TODO comment for notification URL
3. ⚠️ Important: Added TODO comment for button navigation
4. ⚠️ Important: Extracted BLI status constant

✅ Valid but Not Issues:

5. Authorization scope follows existing pattern
6. Defensive user check is intentional
7. isCondensed prop is forward-compatible design
8. Edge cases already handled correctly

⚠️ Deferred to Future Work:

9. Approval status string literals: Valid concern but requires broader refactoring across multiple files. Recommend tracking as separate ticket.

---

Verification

All changes verified with:

• ✅ Frontend tests: 8/8 passing
• ✅ Linting: Clean on both frontend and backend
• ✅ Pre-commit hooks: All passed

Thank you for the thorough review! All critical and important issues have been addressed. 🎉

[josbell]

@rajohnson90 Addressed the issues you caught. Thanks for the review!

[github-actions]

🎉 This PR is included in version 1.383.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀