Skip to content

MPDX-9327, MPDX-9670 - Fix MHA "Update Current MHA" button visibility and taken/approved amounts displays #1824

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.

Sign up for GitHub

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

Merged
zweatshirt merged 2 commits into from
Jun 8, 2026
Merged

MPDX-9327, MPDX-9670 - Fix MHA "Update Current MHA" button visibility and taken/approved amounts displays #1824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dca8400
Add user and spouse approved/taken amounts to Minister Housing Allowa…
zweatshirt Jun 8, 2026
3f6c3d1
Update MHA request handling and button visibility logic in MinisterHo…
zweatshirt Jun 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 41 additions & 12 deletions src/components/HrTools/MinisterHousingAllowance/MinisterHousingAllowance.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';

Check warning on line 1 in src/components/HrTools/MinisterHousingAllowance/MinisterHousingAllowance.test.tsx

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ New issue: Lines of Code in a Single File 

This module has 308 lines of code, improve code health by reducing it to 300. The number of Lines of Code in a single file. More Lines of Code lowers the code health.
import { ThemeProvider } from '@mui/material/styles';
import { render, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
Expand Down Expand Up @@ -297,7 +297,7 @@
<TestComponent
hcmMock={singleMhaNoException}
mhaRequestsMock={[
{ ...mockMHARequest, status: MhaStatusEnum.BoardApproved },
{ ...mockMHARequest, status: MhaStatusEnum.HrApproved },
]}
/>,
);
Expand All @@ -320,21 +320,34 @@
});
});

it.each([
MhaStatusEnum.HrApproved,
MhaStatusEnum.BoardApproved,
MhaStatusEnum.Cancelled,
])('shows new request button when current request is %s', async (status) => {
const { findByRole } = render(
it.each([MhaStatusEnum.HrApproved, MhaStatusEnum.Cancelled])(
'shows new request button when current request is %s',
async (status) => {
const { findByRole } = render(
<TestComponent
hcmMock={singleMhaNoException}
mhaRequestsMock={[{ ...mockMHARequest, status }]}
/>,
);

expect(
await findByRole('button', { name: 'Request New MHA' }),
).toBeInTheDocument();
},
);

it('hides new request button when current request is board approved', async () => {
const { findByText, queryByText } = render(
<TestComponent
hcmMock={singleMhaNoException}
mhaRequestsMock={[{ ...mockMHARequest, status }]}
mhaRequestsMock={[
{ ...mockMHARequest, status: MhaStatusEnum.BoardApproved },
]}
/>,
);

expect(
await findByRole('button', { name: 'Request New MHA' }),
).toBeInTheDocument();
expect(await findByText('Current Board Approved MHA')).toBeInTheDocument();
expect(queryByText('Request New MHA')).not.toBeInTheDocument();
});

it.each([
Expand All @@ -356,12 +369,28 @@
},
);

it('hides Update Current MHA on the previous approved request while an open request exists', async () => {
const { findByText, queryByText } = render(
<TestComponent
hcmMock={singleMhaNoException}
mhaRequestsMock={[
{ ...mockMHARequest, id: '1', status: MhaStatusEnum.InProgress },
{ ...mockMHARequest, id: '2', status: MhaStatusEnum.BoardApproved },
]}
/>,
);

expect(await findByText('Current Board Approved MHA')).toBeInTheDocument();
expect(queryByText('View Current MHA')).toBeInTheDocument();
expect(queryByText('Update Current MHA')).not.toBeInTheDocument();
});

it('shows success snackbar when MHA request is created', async () => {
const { findByText } = render(
<TestComponent
hcmMock={singleMhaNoException}
mhaRequestsMock={[
{ ...mockMHARequest, status: MhaStatusEnum.BoardApproved },
{ ...mockMHARequest, status: MhaStatusEnum.HrApproved },
]}
/>,
);
Expand Down
16 changes: 13 additions & 3 deletions src/components/HrTools/MinisterHousingAllowance/MinisterHousingAllowance.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,101 +127,111 @@

const eitherPersonEligible = userEligibleForMHA || spouseEligibleForMHA;

const showNewRequestButton = eitherPersonEligible && !hasBlockingRequest;
const hasCurrentBoardApprovedMha =
currentRequest?.status === MhaStatusEnum.BoardApproved;
const showNewRequestButton =
eitherPersonEligible && !hasBlockingRequest && !hasCurrentBoardApprovedMha;

const showCurrentRequest = eitherPersonEligible && currentRequest;
const showPreviousRequests = eitherPersonEligible && previousApprovedRequest;

if (creatingRequest) {
return <Loading loading />;
}

return (
<PanelLayout
panelType={PanelTypeEnum.Empty}
percentComplete={0}
backHref={''}
sidebarTitle={t('Your MHA')}
hasCurrentRequestPending={isCurrentRequestPending}
mainContent={
<Container sx={{ ml: 5 }}>
{requestsError ? (
<Notification type="error" message={requestsError.message} />
) : loading ? (
<MinisterHousingAllowanceReportSkeleton />
) : (
<>
<Stack direction="column" width={mainContentWidth}>
<Box mb={2}>
<Typography variant="h5">{t('Your MHA')}</Typography>
</Box>

<NameDisplay
names={names}
personNumbers={personNumbers}
personNumberCount={isMarried ? 2 : 1}
/>

{hasNoRequests ? (
<>
<NoRequestsDisplay />
<Box mt={2}>
<EligibilityStatusTable
userPreferredName={preferredName}
userEligible={userEligibleForMHA}
userCountry={
userHcmData?.staffInfo.country ?? undefined
}
spousePreferredName={
isMarried ? spousePreferredName : undefined
}
spouseEligible={
isMarried ? spouseEligibleForMHA : undefined
}
spouseCountry={
isMarried
? (spouseHcmData?.staffInfo.country ?? undefined)
: undefined
}
userMhiEligibility={getMhiEligibility(userHcmData)}
spouseMhiEligibility={
isMarried
? getMhiEligibility(spouseHcmData)
: undefined
}
/>
</Box>
</>
) : (
eitherPersonEligible && (
<EligibleDisplay
isPending={isCurrentRequestPending}
isEditable={
currentRequest.status === MhaStatusEnum.ActionRequired
}
/>
)
)}

{showCurrentRequest &&
(isCurrentRequestPending ? (
<CurrentRequest request={currentRequest} />
) : (
<CurrentBoardApproved request={currentRequest} />
<CurrentBoardApproved
request={currentRequest}
hasOpenRequest={false}
/>
))}
</Stack>
{showNewRequestButton && (
<Button
variant="contained"
color="primary"
sx={{ mt: 2 }}
onClick={onCreateMHARequest}
>
{t('Request New MHA')}
</Button>
)}
{showPreviousRequests && (
<Stack direction="column" width={mainContentWidth} mt={4}>
<CurrentBoardApproved request={previousApprovedRequest} />
<CurrentBoardApproved
request={previousApprovedRequest}

@zweatshirt zweatshirt Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] `CurrentBoardApproved` now sources its amounts from the HCM *current* snapshot (via context), not from the `request` prop it's handed. For this previous-approved card that's correct **today** — it only renders while the newer request is still pending, so the "previous" approval is the in-effect MHA that HCM's `current*` values describe. But it's an implicit coupling: if the `previousApprovedRequest` filter ever changes to show an older/superseded approval, the amounts would silently be wrong. A one-line comment noting "amounts intentionally reflect the in-effect approved request from HCM, not this row" would prevent a future footgun.

hasOpenRequest={isCurrentRequestPending}
/>

Check warning on line 234 in src/components/HrTools/MinisterHousingAllowance/MinisterHousingAllowance.tsx

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ Getting worse: Complex Method 

MinisterHousingAllowanceReport increases in cyclomatic complexity from 39 to 41, threshold = 15. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
</Stack>
)}
</>
Expand Down
32 changes: 32 additions & 0 deletions ...nents/HrTools/MinisterHousingAllowance/Shared/Context/MinisterHousingAllowanceContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@
spousePreferredName: string;
userEligibleForMHA: boolean;
spouseEligibleForMHA: boolean;
userApprovedOverallAmount: number | null;
spouseApprovedOverallAmount: number | null;
userTakenAmount: number | null;
spouseTakenAmount: number | null;
handleDiscard: () => Promise<void>;

requestData?:
Expand Down Expand Up @@ -204,66 +208,94 @@
[spouseHcmData],
);

const userApprovedOverallAmount = useMemo(
() => userHcmData?.mhaRequest.currentApprovedOverallAmount ?? null,
[userHcmData],
);

const spouseApprovedOverallAmount = useMemo(
() => spouseHcmData?.mhaRequest.currentApprovedOverallAmount ?? null,
[spouseHcmData],
);

const userTakenAmount = useMemo(
() => userHcmData?.mhaRequest.currentTakenAmount ?? null,
[userHcmData],
);

const spouseTakenAmount = useMemo(
() => spouseHcmData?.mhaRequest.currentTakenAmount ?? null,
[spouseHcmData],
);

const [isDrawerOpen, setIsDrawerOpen] = useState(true);
const toggleDrawer = useCallback(() => {
setIsDrawerOpen((prev) => !prev);
}, []);

const [hasCalcValues, setHasCalcValues] = useState(hasValues);
useEffect(() => {
setHasCalcValues(hasValues);
}, [hasValues]);

const contextValue = useMemo(
() => ({
steps,
currentIndex,
percentComplete,
handleNextStep,
handlePreviousStep,
pageType,
hasCalcValues,
setHasCalcValues,
isDrawerOpen,
toggleDrawer,
setIsDrawerOpen,
isMarried,
userHcmData,
spouseHcmData,
preferredName,
spousePreferredName,
userEligibleForMHA,
spouseEligibleForMHA,
userApprovedOverallAmount,
spouseApprovedOverallAmount,
userTakenAmount,
spouseTakenAmount,
handleDiscard,
setIsComplete,
requestData: requestData?.ministryHousingAllowanceRequest ?? null,
requestError,
loading,
requestId,
deleteRequestMutation,
updateMutation,
isMutating,
trackMutation,
}),
[
steps,
currentIndex,
percentComplete,
handleNextStep,
handlePreviousStep,
pageType,
hasCalcValues,
setHasCalcValues,
isDrawerOpen,
toggleDrawer,
setIsDrawerOpen,
isMarried,
userHcmData,
spouseHcmData,
preferredName,
spousePreferredName,
userEligibleForMHA,
spouseEligibleForMHA,
userApprovedOverallAmount,
spouseApprovedOverallAmount,
userTakenAmount,
spouseTakenAmount,

Check warning on line 298 in src/components/HrTools/MinisterHousingAllowance/Shared/Context/MinisterHousingAllowanceContext.tsx

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ Getting worse: Complex Method 

MinisterHousingAllowanceProvider:React.FC<Props> increases in cyclomatic complexity from 18 to 22, threshold = 15. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
handleDiscard,
requestData,
requestError,
Expand Down
92 changes: 87 additions & 5 deletions ...omponents/HrTools/MinisterHousingAllowance/SharedComponents/CurrentBoardApproved.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,20 @@ interface TestComponentProps {
push?: jest.Mock;
query?: { accountListId?: string };
};
hasOpenRequest?: boolean;
}

const TestComponent: React.FC<TestComponentProps> = ({
contextValue,
router = {},
hasOpenRequest = false,
}) => {
const approvedMHARequest = {
...mockMHARequest,
updatedAt: '2022-12-01',
requestAttributes: {
...mockMHARequest.requestAttributes,
hrApprovedAt: '2023-01-15',
approvedOverallAmount: 1500,
staffSpecific: 1000,
spouseSpecific: 500,
},
};

Expand All @@ -52,7 +51,10 @@ const TestComponent: React.FC<TestComponentProps> = ({
<MinisterHousingAllowanceContext.Provider
value={contextValue as ContextType}
>
<CurrentBoardApproved request={approvedMHARequest} />
<CurrentBoardApproved
request={approvedMHARequest}
hasOpenRequest={hasOpenRequest}
/>
</MinisterHousingAllowanceContext.Provider>
</GqlMockedProvider>
</TestRouter>
Expand All @@ -68,6 +70,10 @@ describe('CurrentBoardApproved Component', () => {
isMarried: true,
preferredName: 'John',
spousePreferredName: 'Jane',
userApprovedOverallAmount: 1500,
spouseApprovedOverallAmount: 1500,
userTakenAmount: 1000,
spouseTakenAmount: 500,
userHcmData: {
staffInfo: {
personNumber: '000123456',
Expand Down Expand Up @@ -110,6 +116,8 @@ describe('CurrentBoardApproved Component', () => {
isMarried: false,
preferredName: 'John',
spousePreferredName: '',
userApprovedOverallAmount: 1500,
userTakenAmount: 1000,
userHcmData: {
staffInfo: {
personNumber: '000123456',
Expand Down Expand Up @@ -139,6 +147,55 @@ describe('CurrentBoardApproved Component', () => {
expect(queryByText('Jane')).not.toBeInTheDocument();
});

it('shows distinct per-person approved and claimed amounts from HCM', () => {

@zweatshirt zweatshirt Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Add a test pinning the null-amount behavior, since this is a money display. A fixture already exists (HCM amounts can be `null`). Example: 
it('renders $0.00 when HCM approved/claimed amounts are null', () => {
  // contextValue with userApprovedOverallAmount: null, userTakenAmount: null
  // assert the intended rendering (currently $0.00; or a dash if the Medium finding is adopted)
});

This locks whatever the chosen empty-state behavior is.

const { getByText } = render(
<TestComponent
contextValue={{
isMarried: true,
preferredName: 'John',
spousePreferredName: 'Jane',
userApprovedOverallAmount: 2000,
spouseApprovedOverallAmount: 800,
userTakenAmount: 1200,
spouseTakenAmount: 300,
userHcmData: {
staffInfo: { personNumber: '000123456' },
} as unknown as HcmData,
spouseHcmData: {
staffInfo: { personNumber: '100123456' },
} as unknown as HcmData,
}}
/>,
);

// MHA Approved by Board (per person)
expect(getByText('$2,000.00')).toBeInTheDocument();
expect(getByText('$800.00')).toBeInTheDocument();
// MHA Claimed in Salary (per person)
expect(getByText('$1,200.00')).toBeInTheDocument();
expect(getByText('$300.00')).toBeInTheDocument();
});

it('renders $0.00 when HCM approved/claimed amounts are null', () => {
const { getAllByText } = render(
<TestComponent
contextValue={{
isMarried: false,
preferredName: 'John',
spousePreferredName: '',
userApprovedOverallAmount: null,
userTakenAmount: null,
userHcmData: {
staffInfo: { personNumber: '000123456' },
} as unknown as HcmData,
spouseHcmData: null,
}}
/>,
);

expect(getAllByText('$0.00')).toHaveLength(2);
});

it('should navigate to edit page with new requestId after duplicate mutation', async () => {
const { getByText } = render(
<ThemeProvider theme={theme}>
Expand Down Expand Up @@ -172,7 +229,10 @@ describe('CurrentBoardApproved Component', () => {
} as ContextType
}
>
<CurrentBoardApproved request={mockMHARequest} />
<CurrentBoardApproved
request={mockMHARequest}
hasOpenRequest={false}
/>
</MinisterHousingAllowanceContext.Provider>
</GqlMockedProvider>
</TestRouter>
Expand All @@ -199,4 +259,26 @@ describe('CurrentBoardApproved Component', () => {
);
});
});

it('should hide Update Current MHA button when there is an open request', () => {
const { queryByText, getByText } = render(
<TestComponent
contextValue={{
isMarried: false,
preferredName: 'John',
spousePreferredName: '',
userHcmData: {
staffInfo: {
personNumber: '000123456',
},
} as unknown as HcmData,
spouseHcmData: null,
}}
hasOpenRequest
/>,
);

expect(getByText('View Current MHA')).toBeInTheDocument();
expect(queryByText('Update Current MHA')).not.toBeInTheDocument();
});
});
Loading
Loading