Merge pull request #3050 from HHS/OPS-3012/Update-Change-Request-Filt…

…ering

Ops 3012/update change request filtering

backend/openapi.yml

@@ -3599,11 +3599,35 @@ components:
           type: integer
         appropriation_date:
           type: integer
+    Division:
+      type: object
+      properties:
+        id:
+          type: integer
+        name:
+          type: string
+        abbreviation:
+          type: string
+        division_director_id:
+          type: integer
+        deputy_division_director_id:
+          type: integer
+        created_by:
+          $ref: "#/components/parameters/created_by"
+        updated_by:
+          $ref: "#/components/parameters/updated_by"
+        created_on:
+          $ref: "#/components/parameters/created_on"
+        updated_on:
+          $ref: "#/components/parameters/updated_on"
     PortfolioBLISchema:
       type: object
       properties:
         division_id:
           type: integer
+        division:
+          type: object
+          $ref: "#/components/schemas/Division"
     BudgetLineItemRequest:
       type: object
       properties:
@@ -4120,6 +4144,12 @@ components:
               "abbreviation": "DFCD",
               "id": 1,
               "name": "Division of Child and Family Development"
+              "division_director_id": "522"
+              "deputy_division_director_id": "520"
+              "created_on": "2024-11-14 20:20:55.176355"
+              "updated_on": "2024-11-14 20:20:55.176355"
+              "created_by": 1,
+              "updated_by": 1
           },
           "division_id": 1,
           "id": 1,

backend/ops_api/ops/resources/change_requests.py

@@ -71,21 +71,20 @@ def review_change_request(
 
 # TODO: add more query options, for now this just returns CRs in review for
 #  the current user as a division director or deputy division director
-def find_change_requests(limit: int = 10, offset: int = 0):
-
-    current_user_id = getattr(current_user, "id", None)
+def find_change_requests(user_id, limit: int = 10, offset: int = 0):
 
     stmt = (
         select(ChangeRequest)
         .join(Division, ChangeRequest.managing_division_id == Division.id)
         .where(ChangeRequest.status == ChangeRequestStatus.IN_REVIEW)
-        .where(
+    )
+    if user_id:
+        stmt = stmt.where(
             or_(
-                Division.division_director_id == current_user_id,
-                Division.deputy_division_director_id == current_user_id,
+                Division.division_director_id == user_id,
+                Division.deputy_division_director_id == user_id,
             )
         )
-    )
     stmt = stmt.limit(limit)
     if offset:
         stmt = stmt.offset(int(offset))
@@ -111,7 +110,8 @@ def __init__(self, model: ChangeRequest = ChangeRequest):
     def get(self) -> Response:
         limit = request.args.get("limit", 10, type=int)
         offset = request.args.get("offset", 0, type=int)
-        results = find_change_requests(limit=limit, offset=offset)
+        user_id = request.args.get("userId")
+        results = find_change_requests(user_id, limit=limit, offset=offset)
         change_requests = [row[0] for row in results] if results else None
         if change_requests:
             response = make_response_with_headers(self._response_schema_collection.dump(change_requests))

backend/ops_api/ops/schemas/budget_line_items.py

@@ -7,7 +7,7 @@
 from marshmallow_enum import EnumField
 
 from marshmallow import EXCLUDE, Schema, ValidationError, fields, validates_schema
-from models import AgreementReason, BudgetLineItem, BudgetLineItemStatus, ContractAgreement, ServicesComponent
+from models import AgreementReason, BudgetLineItem, BudgetLineItemStatus, ServicesComponent
 from ops_api.ops.schemas.change_requests import GenericChangeRequestResponseSchema
 
 
@@ -124,19 +124,6 @@ def validate_agreement_reason(self, data, **kwargs):
             if bli and bli.agreement_id and not bli.agreement.agreement_reason:
                 raise ValidationError("BLI's Agreement must have an AgreementReason when status is not DRAFT")
 
-    @validates_schema
-    def validate_agreement_reason_must_not_have_vendor(self, data, **kwargs):
-        if self.status_is_changing_beyond_draft(data):
-            bli = self.get_current_budget_line_item()
-            if (
-                bli
-                and bli.agreement_id
-                and isinstance(bli.agreement, ContractAgreement)  # only contracts have vendors
-                and bli.agreement.agreement_reason == AgreementReason.NEW_REQ
-                and bli.agreement.vendor_id
-            ):
-                raise ValidationError("BLI's Agreement cannot have a Vendor if it has an Agreement Reason of NEW_REQ")
-
     @validates_schema
     def validate_agreement_reason_must_have_vendor(self, data, **kwargs):
         if self.status_is_changing_beyond_draft(data):
@@ -249,8 +236,21 @@ class Meta:
     email = fields.Str(default=None, allow_none=True)
 
 
+class DivisionSchema(Schema):
+    id = fields.Integer(required=True)
+    name = fields.String(allow_none=True)
+    abbreviation = fields.String(required=True)
+    division_director_id = fields.Integer(required=True)
+    deputy_division_director_id = fields.Integer(required=True)
+    created_by = fields.Integer(allow_none=True)
+    updated_by = fields.Integer(allow_none=True)
+    created_on = fields.DateTime(format="%Y-%m-%dT%H:%M:%S.%fZ", allow_none=True)
+    updated_on = fields.DateTime(format="%Y-%m-%dT%H:%M:%S.%fZ", allow_none=True)
+
+
 class PortfolioBLISchema(Schema):
     division_id = fields.Int(required=True)
+    division = fields.Nested(DivisionSchema(), default=[])
 
 
 class BudgetLineItemCANSchema(Schema):

backend/ops_api/ops/schemas/cans.py

@@ -44,11 +44,24 @@ class PortfolioUrlCANSchema(Schema):
     updated_by_user = fields.Nested(SafeUserSchema(), allow_none=True)
 
 
+class DivisionSchema(Schema):
+    id = fields.Integer(required=True)
+    name = fields.String(allow_none=True)
+    abbreviation = fields.String(required=True)
+    division_director_id = fields.Integer(required=True)
+    deputy_division_director_id = fields.Integer(required=True)
+    created_by = fields.Integer(allow_none=True)
+    updated_by = fields.Integer(allow_none=True)
+    created_on = fields.DateTime(format="%Y-%m-%dT%H:%M:%S.%fZ", allow_none=True)
+    updated_on = fields.DateTime(format="%Y-%m-%dT%H:%M:%S.%fZ", allow_none=True)
+
+
 class PortfolioCANSchema(Schema):
     id = fields.Integer(required=True)
     name = fields.String(allow_none=True)
     abbreviation = fields.String(required=True)
     status = fields.Enum(PortfolioStatus)
+    division = fields.Nested(DivisionSchema(), default=[])
     division_id = fields.Integer(required=True)
     urls = fields.List(fields.Nested(PortfolioUrlCANSchema()), default=[])
     team_leaders = fields.List(fields.Nested(SafeUserSchema()), default=[])

backend/ops_api/tests/ops/features/test_validate_draft_budget_lines.py

@@ -56,13 +56,6 @@ def test_valid_procurement_shop(loaded_db, context): ...
 def test_valid_agreement_reason(loaded_db, context): ...
 
 
-@scenario(
-    "validate_draft_budget_lines.feature",
-    "Valid Agreement Reason - NEW_REQ does not have a Vendor",
-)
-def test_valid_agreement_reason_no_vendor(loaded_db, context): ...
-
-
 @scenario(
     "validate_draft_budget_lines.feature",
     "Valid Agreement Reason - RECOMPETE and LOGICAL_FOLLOW_ON requires a Vendor",

backend/ops_api/tests/ops/workflows/test_change_requests.py

@@ -261,11 +261,6 @@ def test_change_request_list(auth_client, app, test_user, test_admin_user, test_
     session.add(change_request1)
     session.commit()
 
-    # verify no change request in the list to review for this user
-    response = auth_client.get(url_for("api.change-request-list"))
-    assert response.status_code == 200
-    assert len(response.json) == 0
-
     # change division#1 director and division#2 deputy directory to this test user
     division1: Division = session.get(Division, 1)
     division1.division_director_id = test_admin_user.id

frontend/src/api/opsAPI.js

@@ -322,7 +322,9 @@ export const opsApi = createApi({
             invalidatesTags: ["ServicesComponents", "Agreements", "BudgetLineItems", "AgreementHistory"]
         }),
         getChangeRequestsList: builder.query({
-            query: () => `/change-requests/`,
+            query: ({userId}) => ({
+                url: `/change-requests/${userId ? `?userId=${userId}` : ""}`,
+            }),
             providesTags: ["ChangeRequests"]
         }),
         reviewChangeRequest: builder.mutation({

frontend/src/components/BudgetLineItems/BLIDiffTable/BLIDiffRow.jsx

@@ -41,10 +41,11 @@ import { useChangeRequestsForTooltip } from "../../../hooks/useChangeRequests.ho
 const BLIDiffRow = ({ budgetLine, changeType, statusChangeTo = "" }) => {
     const { isExpanded, setIsExpanded, setIsRowActive } = useTableRow();
     const budgetLineCreatorName = useGetUserFullNameFromId(budgetLine?.created_by);
-    const userDivisionId = useSelector((state) => state.auth?.activeUser?.division) ?? null;
+    const userId = useSelector((state) => state.auth?.activeUser?.id) ?? null;
     const isBudgetLineInReview = budgetLine?.in_review;
-    const canDivisionId = budgetLine?.can?.portfolio?.division_id;
-    const isActionable = canDivisionId === userDivisionId || !isBudgetLineInReview;
+    const canDivisionDirectorId = budgetLine?.can?.portfolio?.division.division_director_id;
+    const canDeputyDivisionDirectorId = budgetLine?.can?.portfolio?.division.deputy_division_director_id;
+    const isActionable = (canDivisionDirectorId === userId || canDeputyDivisionDirectorId  === userId) || !isBudgetLineInReview;
     const title = "This budget line has pending edits with a different Division:";
     const lockedMessage = useChangeRequestsForTooltip(budgetLine, title);
     const feeTotal = totalBudgetLineFeeAmount(budgetLine?.amount || 0, budgetLine?.proc_shop_fee_percentage);

frontend/src/components/BudgetLineItems/BLIDiffTable/BLIDiffRow.test.jsx

@@ -29,7 +29,7 @@ vi.mock("react-redux", async () => {
             const mockState = {
                 auth: {
                     activeUser: {
-                        division: 1
+                        id: 1
                     }
                 }
             };

frontend/src/components/ChangeRequests/ChangeRequestsList/ChangeRequestList.test.jsx

@@ -51,9 +51,9 @@ describe("ChangeRequestList", () => {
     });
     it("renders with change requests", async () => {
         const mockChangeRequests = [
-            { ...changeRequests[0], managing_division_id: 1 },
-            { ...changeRequests[1], managing_division_id: 1 },
-            { ...changeRequests[2], managing_division_id: 2 } // This should be filtered out
+            { ...changeRequests[0]},
+            { ...changeRequests[1]},
+            { ...changeRequests[2]}
         ];
 
         useGetChangeRequestsListQuery.mockReturnValue({ data: mockChangeRequests });
@@ -72,6 +72,6 @@ describe("ChangeRequestList", () => {
         );
 
         const headings = await screen.findAllByText(/budget change/i);
-        expect(headings).toHaveLength(2);
+        expect(headings).toHaveLength(3);
     });
 });

frontend/src/components/ChangeRequests/ChangeRequestsList/ChangeRequestsList.jsx

@@ -1,9 +1,9 @@
 import PropTypes from "prop-types";
 import * as React from "react";
-import { useSelector } from "react-redux";
 import { useGetChangeRequestsListQuery } from "../../../api/opsAPI";
 import BudgetChangeReviewCard from "../BudgetChangeReviewCard";
 import StatusChangeReviewCard from "../StatusChangeReviewCard";
+import { useSelector } from "react-redux";
 
 /**
  * @component Change Requests List component.
@@ -13,30 +13,22 @@ import StatusChangeReviewCard from "../StatusChangeReviewCard";
  * @returns {JSX.Element} - The rendered component
  */
 function ChangeRequestsList({ handleReviewChangeRequest }) {
-    const userDivisionId = useSelector((state) => state.auth?.activeUser?.division) ?? -1;
+    const userId = useSelector((state) => state.auth?.activeUser?.id) ?? null;
     const {
         data: changeRequests,
         isLoading: loadingChangeRequests,
         isError: errorChangeRequests
-    } = useGetChangeRequestsListQuery({ refetchOnMountOrArgChange: true });
+    } = useGetChangeRequestsListQuery( { refetchOnMountOrArgChange: true, userId });
 
     if (loadingChangeRequests) {
         return <h1>Loading...</h1>;
     }
     if (errorChangeRequests) {
         return <h1>Oops, an error occurred</h1>;
     }
-
-    const changeRequestsForUser = Array.isArray(changeRequests)
-        ? changeRequests.filter(
-              /** @param {ChangeRequest} changeRequest */
-              (changeRequest) => changeRequest.managing_division_id === userDivisionId
-          )
-        : [];
-
-    return changeRequestsForUser.length > 0 ? (
+    return changeRequests.length > 0 ? (
         <>
-            {changeRequestsForUser.map(
+            {changeRequests.map(
                 /** @param {ChangeRequest} changeRequest */
                 (changeRequest) => (
                     <React.Fragment key={changeRequest.id}>

frontend/src/components/Portfolios/PortfolioTypes.d.ts

@@ -1,13 +1,26 @@
 import { CAN, URL } from "../CANs/CANTypes";
 import { SafeUser } from "../Users/UserTypes";
 
+export type Division = {
+    id: number;
+    name?: string;
+    abbreviation: string;
+    division_director_id: number;
+    deputy_division_director_id: number;
+    created_by?: number;
+    updated_by?: number;
+    created_on?: any;
+    updated_on?: any;
+  };
+
 export type Portfolio = {
     id: number;
     name?: string;
     abbreviation: string;
     status?: string;
     cans?: CAN[];
     division_id: number;
+    division: Division;
     urls?: URL[];
     team_leaders?: SafeUser[];
     created_by?: any;

frontend/src/helpers/changeRequests.helpers.js

@@ -53,15 +53,15 @@ export function renderChangeValues(keyName, changeTo, oldCan = "", newCan = "")
 /**
  * Get change requests in review from budget lines.
  * @param {BudgetLine[]} budgetLines - The budget lines.
- * @param {number}[ userDivisionId] - The user division ID.
+ * @param {number}[ userId] - The user division ID.
  * @returns {ChangeRequest[]} The change requests in review.
  */
 
-export function getInReviewChangeRequests(budgetLines, userDivisionId) {
+export function getInReviewChangeRequests(budgetLines, userId) {
     return budgetLines
         .filter(
             (budgetLine) =>
-                budgetLine.in_review && (!userDivisionId || budgetLine.can?.portfolio.division_id === userDivisionId)
+                budgetLine.in_review && (!userId || budgetLine.can?.portfolio?.division.division_director_id === userId || budgetLine.can?.portfolio?.division.deputy_division_director_id === userId)
         )
         .flatMap((budgetLine) => budgetLine.change_requests_in_review || []);
 }

frontend/src/hooks/useChangeRequests.hooks.js

@@ -27,17 +27,10 @@ export const useChangeRequestsForAgreement = (agreementId) => {
  * @returns {number} The total number of change requests.
  */
 export const useChangeRequestTotal = () => {
-    const userDivisionId = useSelector((state) => state.auth?.activeUser?.division) ?? -1;
-    const { data: changeRequests } = useGetChangeRequestsListQuery({});
+    const userId = useSelector((state) => state.auth?.activeUser?.id) ?? null;
+    const { data: changeRequests } = useGetChangeRequestsListQuery({userId});
 
-    const changeRequestsForUser = Array.isArray(changeRequests)
-        ? changeRequests?.filter(
-              /** @param {ChangeRequest} changeRequest */
-              (changeRequest) => changeRequest.managing_division_id === userDivisionId
-          )
-        : [];
-
-    return changeRequestsForUser?.length || 0;
+    return changeRequests?.length || 0;
 };
 
 /**