Ops 3012/update change request filtering #3050
Conversation
fpigeonjr
added
the
tech-debt
Santi-3rd
requested review from
fpigeonjr,
rajohnson90,
johndeange and
maiyerlee
There was a problem hiding this comment.
Looks good @Santi-3rd. I see schema changes and would ask to update the OpenApi spec as part of this effort.
| @@ -1,13 +1,26 @@ | |||
| import { CAN, URL } from "../CANs/CANTypes"; | |||
| import { SafeUser } from "../Users/UserTypes"; | |||
|
|
|||
| export type Division = { | |||
There was a problem hiding this comment.
🧡 thx for keeping our types up-to-date
| @@ -88,7 +88,8 @@ const useApproveAgreement = () => { | |||
| const agreementId = +urlPathParams.id; | |||
| const [searchParams] = useSearchParams(); | |||
| const navigate = useNavigate(); | |||
| const userDivisionId = useSelector((state) => state.auth?.activeUser?.division) ?? null; | |||
| const userDivisionId = useSelector((state) => state.auth?.activeUser?.division) ?? -1; | |||
There was a problem hiding this comment.
wonder if we are using this variable anywhere?
There was a problem hiding this comment.
Frank and I also talked and think that maybe the other uses of userDivisionId in the ApproveAgreement.hooks.js should be replaced with userId and the same check that we changed in the 'getInReviewChangeRequests' file. The concern was that if we were checking the ownership of the division is checked in different ways it could lead to unexpected inconsistencies.
| export type Portfolio = { | ||
| id: number; | ||
| name?: string; | ||
| abbreviation: string; | ||
| status?: string; | ||
| cans?: CAN[]; | ||
| division_id: number; | ||
| division?: Division; |
There was a problem hiding this comment.
Division is not optional. With the way the backfill works, if we have a division_id we will have a division. You should take out the question mark because any scenario where there isn't a division is just a data error.
|
Didn't we also discuss removing the unneeded |
|
Frank is right we also want to remove the changeRequestsForUser in the ChangeRequestsList component as well. |
… ChangeRequestsList
What changed
Change request filtering is now based on the division ID and the deputy division director's ID.
Issue
#3012
How to test
Run end-to-end tests
Definition of Done Checklist