-
Notifications
You must be signed in to change notification settings - Fork 1
MPDX-9129 - MHA Eligibility check for new requests #1546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Preview branch generated at https://MPDX-9129.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against cfa6ff2 No significant changes found |
0eab8a6 to
8343163
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request implements MHA (Minister Housing Allowance) eligibility checking based on GraphQL data from the backend. The feature adds a new mhaEit field with mhaEligibility boolean to HCM data and enforces eligibility checks when users attempt to create or update MHA requests.
Changes:
- Added
mhaEit.mhaEligibilityfield to GraphQL schema and mock data - Implemented eligibility state management in the context provider
- Added eligibility checks to all MHA request mutations with user feedback via snackbar notifications
- Updated display logic to show appropriate messages for ineligible users and spouses
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/Reports/Shared/HcmData/HCMData.graphql | Added mhaEit field with mhaEligibility boolean to GraphQL query |
| src/components/Reports/Shared/HcmData/mockData.ts | Added mock data for various eligibility scenarios (single/married, eligible/ineligible combinations) |
| src/components/Reports/MinisterHousingAllowance/Shared/Context/MinisterHousingAllowanceContext.tsx | Added userEligibleForMHA, spouseEligibleForMHA, hcmLoading, and hcmError to context |
| src/components/Reports/MinisterHousingAllowance/MinisterHousingAllowance.tsx | Added eligibility check to new request creation with canAccessMHA logic |
| src/components/Reports/MinisterHousingAllowance/MainPages/IneligibleDisplay.tsx | Updated to show conditional messages based on user and spouse eligibility status |
| src/components/Reports/MinisterHousingAllowance/Steps/StepTwo/RentOwn.tsx | Added eligibility check to block updates when user is ineligible |
| src/components/Reports/MinisterHousingAllowance/Steps/StepThree/Calculation.tsx | Added eligibility check to updateCheckbox function |
| src/components/Reports/MinisterHousingAllowance/Shared/AutoSave/useSaveField.ts | Added eligibility check to saveField callback |
| Multiple test files | Added SnackbarProvider wrapper and test cases for eligibility scenarios |
...components/Reports/MinisterHousingAllowance/Shared/AutoSave/AutosaveCustomTextField.test.tsx
Show resolved
Hide resolved
| Once approved, when you calculate your salary, you will see | ||
| the approved amount that can be applied to {preferredName} | ||
| 's salary. If you believe this is incorrect, please | ||
| contact Personnel Records at 407-826-2236 or{' '} |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The phone number in this error message (407-826-2236) differs from the phone number used in the earlier message on line 28 (407-826-2252). This inconsistency could confuse users. Verify which phone number is correct for Personnel Records and ensure it's used consistently throughout the component.
| contact Personnel Records at 407-826-2236 or{' '} | |
| contact Personnel Records at 407-826-2252 or{' '} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already reached out to Ryan to find out what the correct number is. Awaiting a response.
src/components/Reports/MinisterHousingAllowance/Shared/AutoSave/useSaveField.ts
Show resolved
Hide resolved
src/components/Reports/MinisterHousingAllowance/MinisterHousingAllowance.tsx
Outdated
Show resolved
Hide resolved
| {!hcmLoading && | ||
| ((isMarried && !spouseEligibleForMHA) || !userEligibleForMHA) && ( | ||
| <Box mt={2} data-testid="user-ineligible-message"> | ||
| <Trans i18nKey="userNotEligibleMha" values={{ preferredName }}> | ||
| <p style={{ lineHeight: 1.5 }}> | ||
| Once approved, when you calculate your salary, you will see | ||
| the approved amount that can be applied to {preferredName} | ||
| 's salary. If you believe this is incorrect, please | ||
| contact Personnel Records at 407-826-2236 or{' '} | ||
| <a href="mailto:MHA@cru.org">MHA@cru.org</a>. | ||
| </p> | ||
| </Trans> | ||
| </Box> | ||
| )} |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition for displaying the user-ineligible-message has a logic issue. The current condition ((isMarried && !spouseEligibleForMHA) || !userEligibleForMHA) will show the user-ineligible message even when the user IS eligible if they're married and the spouse is ineligible. This doesn't match the intended behavior based on the test cases and the message content.
The condition should be checking if the user is NOT eligible, or if both are married and both are ineligible. Consider simplifying to just !userEligibleForMHA since this message is specifically about the user's eligibility status and when they can see approved amounts.
| {!hcmLoading && | |
| ((isMarried && !spouseEligibleForMHA) || !userEligibleForMHA) && ( | |
| <Box mt={2} data-testid="user-ineligible-message"> | |
| <Trans i18nKey="userNotEligibleMha" values={{ preferredName }}> | |
| <p style={{ lineHeight: 1.5 }}> | |
| Once approved, when you calculate your salary, you will see | |
| the approved amount that can be applied to {preferredName} | |
| 's salary. If you believe this is incorrect, please | |
| contact Personnel Records at 407-826-2236 or{' '} | |
| <a href="mailto:MHA@cru.org">MHA@cru.org</a>. | |
| </p> | |
| </Trans> | |
| </Box> | |
| )} | |
| {!hcmLoading && !userEligibleForMHA && ( | |
| <Box mt={2} data-testid="user-ineligible-message"> | |
| <Trans i18nKey="userNotEligibleMha" values={{ preferredName }}> | |
| <p style={{ lineHeight: 1.5 }}> | |
| Once approved, when you calculate your salary, you will see | |
| the approved amount that can be applied to {preferredName} | |
| 's salary. If you believe this is incorrect, please | |
| contact Personnel Records at 407-826-2236 or{' '} | |
| <a href="mailto:MHA@cru.org">MHA@cru.org</a>. | |
| </p> | |
| </Trans> | |
| </Box> | |
| )} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping this as is to match the criteria of the Figma doc. Can change if needed.
|
@kegrimes You know MHA far more than I do. Before I ask Bizz for a review would you be up for looking through this quickly in case I missed something? Thanks Katelyn! I am mostly looking to know if I have applied if statements to all MHA mutations, and if the state for IneligibleDisplay.tsx is correct. I haven't actually been able to test this PR properly yet since we need to test an ineligible user. |
|
@dr-bizz I was going to ask Katelyn to review this since she knows MHA well but since she's sick I've added you. We can get her review when she comes back. |
dr-bizz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good. I think we might need to add some functionality that works out who the MHA is for, the user or the spouse, but apart from that, it's looking great.
| const userEligibleForMHA = useMemo( | ||
| () => userHcmData?.mhaEit?.mhaEligibility ?? false, | ||
| [userHcmData], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should default to true while HCM is loading? So if HCM takes a while to load, it doesn't tell the user they can't create a MHA and then says it can.
This may be my preference, but I would prefer it to tell the user they can create an MHA, then tell them they can't, as this will affect less users.
| spouseEligibleForMHA, | ||
| } = useMinisterHousingAllowance(); | ||
|
|
||
| const canAccessMHA = userEligibleForMHA || spouseEligibleForMHA; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought they would have to switch screens to be a ble to see if their spouse is eligible?
What. if the primary user is not Eligible but the spouse is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dr-bizz I asked Katelyn about this and neither of us are sure what the intended behavior there is. I assumed that the form should only apply to the user if eligible, or both if both eligible. If it's true the form can still be filled out for the spouse when the user is ineligible, I'll change that.
I'll reach out to Ryan as well to get better understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer to comment in IneligibleDisplay! I already heard from Ryan.
| {hasNoRequests || !canAccessMHA ? ( | ||
| <IneligibleDisplay /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code reads as if they have no requests they get the same message as users who are not eligilble for MHA
| } = useMinisterHousingAllowance(); | ||
|
|
||
| const updateRequest = (id: string, rentOrOwn: MhaRentOrOwnEnum) => { | ||
| if (!userEligibleForMHA) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if they are filling it out for their spouse?
|
|
||
| const updateCheckbox = (value: boolean) => | ||
| const updateCheckbox = (value: boolean) => { | ||
| if (!userEligibleForMHA) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again here, what if the user is filling the MHA out for their spouse whi is eligible?
| if (!requestData?.id) { | ||
| return; | ||
| } | ||
| if (!userEligibleForMHA) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to work out who the MHA is for before saying the user is eligible
| {!hcmLoading && | ||
| isMarried && | ||
| userEligibleForMHA && | ||
| !spouseEligibleForMHA && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the user is not eligible, but the spouse is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked with Ryan and he said to display the same text that you would for a person who is eligible and has a spouse who is ineligible. The only difference would be flipping the names around! He is thinking that the request would be created for the person who is eligible.
kegrimes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great Zach, thank you for doing this! I had a few comments, but overall this looks good. Ryan got back to me with the missing case (user ineligible, spouse eligible) so hopefully you can implement that easily!
| {!hcmLoading && | ||
| isMarried && | ||
| userEligibleForMHA && | ||
| !spouseEligibleForMHA && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked with Ryan and he said to display the same text that you would for a person who is eligible and has a spouse who is ineligible. The only difference would be flipping the names around! He is thinking that the request would be created for the person who is eligible.
| {!hcmLoading && | ||
| ((isMarried && !spouseEligibleForMHA) || !userEligibleForMHA) && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These checks feel like a lot in one place. Would we be able to extract these checks into a single variable instead? Even then, !hcmLoading and isMarried can possibly be it's own variable too.
const showSpouseIneligibleText = !hcmLoading && isMarried && userEligibleForMHA && !spouseEligibleForMHA
| spouseEligibleForMHA, | ||
| } = useMinisterHousingAllowance(); | ||
|
|
||
| const canAccessMHA = userEligibleForMHA || spouseEligibleForMHA; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer to comment in IneligibleDisplay! I already heard from Ryan.
| if (!userEligibleForMHA) { | ||
| enqueueSnackbar(t('You are not eligible to create a new MHA request.'), { | ||
| variant: 'error', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I noticed in MHA is that a lot of the backend logic already had errors set up, so I was getting a double snackbar sometimes. I'm not sure if you did that in the backend, but if not, this is great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would probably want some form of frontend validation to avoid making uneccessary requests. Maybe it's best to disable fields if they are ineligible instead, but I'm wondering what the best way to notify the user of their ineligibility might be (assuming they manage to get this far)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think with eligibility, it would be best to disable "New Request" button and maybe have an information alert box on the main page. This is what Ryan and I did with edit access and the new component I created was a safety net incase a user saved the edit page but their access changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I plan to do that, but a user can technically bypass that right? I don't know enough about MHA. I assume disabling the fields will be sufficient in this case.
| if (!requestData?.id) { | ||
| return; | ||
| } | ||
| if (!userEligibleForMHA) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this logic will ever execute since #1539 adds two new components: no edit access and no request access. If a user does not have edit access (status is not in_progress or action_required), they will get a blank page explaining that. If a user is not eligible for mha, they will get a blank page explaining that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I rebase onto that branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could, I just might have some changes on that branch since it has not been reviewed yet!
|
|
||
| const updateCheckbox = (value: boolean) => | ||
| const updateCheckbox = (value: boolean) => { | ||
| if (!userEligibleForMHA) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I don't think this logic will ever execute since they won't have access to this component.
| } = useMinisterHousingAllowance(); | ||
|
|
||
| const updateRequest = (id: string, rentOrOwn: MhaRentOrOwnEnum) => { | ||
| if (!userEligibleForMHA) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
Description
Determines MHA eligibility based on grahpql data.
Related API PR: https://github.com/CruGlobal/mpdx_api/pull/3090
Testing
reports/housingAllowanceChecklist:
/pr-reviewcommand locally and fixed any relevant suggestions