Skip to content

MPDX-9374 - Display effective approved salary instead of current salary #1815

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 1 commit into from
Jun 3, 2026
Merged

MPDX-9374 - Display effective approved salary instead of current salary #1815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
84 changes: 79 additions & 5 deletions ...Tools/SalaryCalculator/SalaryCalculation/RequestedSalaryCard/RequestedSalaryCard.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/SalaryCalculator/SalaryCalculation/RequestedSalaryCard/RequestedSalaryCard.test.tsx

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ Getting worse: Code Duplication 

introduced similar code in: 'should render a dash when there is no approved salary request','should render a dash when there is no approved salary request'. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.
import { render, waitFor } from '@testing-library/react';
import { DeepPartial } from 'ts-essentials';
import { AutosaveForm } from 'src/components/Shared/Autosave/AutosaveForm';
Expand All @@ -8,8 +8,11 @@
} from 'src/graphql/types.generated';
import { SalaryCalculationQuery } from '../../SalaryCalculatorContext/SalaryCalculation.generated';
import {
EffectiveSalaryRequestMock,
SalaryCalculatorTestWrapper,
SalaryCalculatorTestWrapperProps,
hcmSpouseMock,
hcmUserMock,
} from '../../SalaryCalculatorTestWrapper';
import { RequestedSalaryCard } from './RequestedSalaryCard';

Expand All @@ -27,9 +30,19 @@
},
};

const TestComponent: React.FC<SalaryCalculatorTestWrapperProps> = (props) => (
const approvedSalaryMock: EffectiveSalaryRequestMock = {
personNumber: hcmUserMock.staffInfo.personNumber,
salary: 11111,
spouseSalary: 22222,
};

const TestComponent: React.FC<SalaryCalculatorTestWrapperProps> = ({
effectiveSalaryRequestMock = approvedSalaryMock,
...props
}) => (
<SalaryCalculatorTestWrapper
salaryRequestMock={defaultSalaryMock}
effectiveSalaryRequestMock={effectiveSalaryRequestMock}
hcmUser={{
currentSalary: { grossSalaryAmount: 10001 },
}}
Expand Down Expand Up @@ -74,7 +87,7 @@
expect(
getAllByRole('rowheader').map((cell) => cell.textContent),
).toEqual([
'Current Salary',
'Current Requested Salary',
'Minimum Salary',
'Maximum Allowable Salary (CAP)',
'Requested Salary',
Expand All @@ -86,7 +99,27 @@
const { getAllByRole } = render(<TestComponent />);

const expectedCells = [
['$10,001.00', '$20,001.00'],
['$11,111.00', '$22,222.00'],
['$10,003.00', '$20,003.00'],
['$10,004.00', '$20,004.00'],
].flat();

await waitFor(() =>
expect(
getAllByRole('cell')
.slice(0, -2)
.map((cell) => cell.textContent),
).toEqual(expectedCells),
);
});

it('should render a dash when there is no approved salary request', async () => {
const { getAllByRole } = render(
<TestComponent effectiveSalaryRequestMock={null} />,
);

const expectedCells = [
['–', '–'],
['$10,003.00', '$20,003.00'],
['$10,004.00', '$20,004.00'],
].flat();
Expand All @@ -101,6 +134,31 @@
);
});

it('swaps the requested salary when the spouse created the request', async () => {
const { getAllByRole } = render(
<TestComponent
effectiveSalaryRequestMock={{
...approvedSalaryMock,
personNumber: hcmSpouseMock.staffInfo.personNumber,
}}
/>,
);

await waitFor(() =>
expect(
getAllByRole('cell')
.slice(0, -2)
.map((cell) => cell.textContent),
).toEqual(
[
['$22,222.00', '$11,111.00'],
['$10,003.00', '$20,003.00'],
['$10,004.00', '$20,004.00'],
].flat(),
),
);
});

it('should render the effective paycheck note when payroll dates match', async () => {
const { findByRole } = render(
<TestComponent
Expand Down Expand Up @@ -148,7 +206,7 @@
expect(
getAllByRole('rowheader').map((cell) => cell.textContent),
).toEqual([
'Current Salary',
'Current Requested Salary',
'Minimum Salary',
'Maximum Allowable Salary (CAP)',
'Requested Salary',
Expand All @@ -159,7 +217,23 @@
it('should render table cells with formatted currency', async () => {
const { getAllByRole } = render(<TestComponent hasSpouse={false} />);

const expectedCells = ['$10,001.00', '$10,003.00', '$10,004.00'];
const expectedCells = ['$11,111.00', '$10,003.00', '$10,004.00'];

await waitFor(() =>
expect(
getAllByRole('cell')
.slice(0, -1)
.map((cell) => cell.textContent),
).toEqual(expectedCells),
);
});

it('should render a dash when there is no approved salary request', async () => {
const { getAllByRole } = render(
<TestComponent hasSpouse={false} effectiveSalaryRequestMock={null} />,
);

const expectedCells = ['–', '$10,003.00', '$10,004.00'];

await waitFor(() =>
expect(
Expand Down
21 changes: 15 additions & 6 deletions ...ts/HrTools/SalaryCalculator/SalaryCalculation/RequestedSalaryCard/RequestedSalaryCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,14 @@
import { useAutosaveForm } from 'src/components/Shared/Autosave/AutosaveForm';
import { amount } from 'src/lib/yupHelpers';
import { AutosaveTextField } from '../../Autosave/AutosaveTextField';
import { CalculationFieldsFragment } from '../../SalaryCalculatorContext/SalaryCalculation.generated';
import {
CalculationFieldsFragment,
useEffectiveSalaryCalculationQuery,
} from '../../SalaryCalculatorContext/SalaryCalculation.generated';
import { useSalaryCalculator } from '../../SalaryCalculatorContext/SalaryCalculatorContext';
import { EffectiveDateNote } from '../../Shared/EffectiveDateNote';
import { StepCard, StepTableHead } from '../../Shared/StepCard';
import { orientSalaryRequest } from '../../Shared/orientSalaryRequest';
import { useFormatters } from '../../Shared/useFormatters';
import { useCaps } from '../useCaps';
import { useSosaBlockOverCap } from '../useSosaBlockOverCap';
Expand All @@ -35,107 +39,112 @@
const { isUserSosa, blockOnCap } = useSosaBlockOverCap();
const { markValid, markInvalid } = useAutosaveForm();

const { data: effectiveData } = useEffectiveSalaryCalculationQuery();

@zweatshirt zweatshirt Jun 2, 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] Only `data` is destructured — `loading`/`error` aren't handled. During load, `salary` is `undefined`, so the cell transiently shows `–`, which is indistinguishable from the real "no approved request" empty state. Low impact: the sibling `SalarySummaryCard` does the same, and this variable-free query is almost always cache-warm by the time the card mounts. If you address it (e.g. a `Skeleton`), do it across both cards.

const { salary, spouseSalary } =
orientSalaryRequest(

@zweatshirt zweatshirt Jun 2, 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] The `useEffectiveSalaryCalculationQuery()` + `orientSalaryRequest(..., hcmUser?.staffInfo.personNumber)` idiom now appears in both this card and `SalarySummaryCard`. A small shared `useEffectiveSalaryRequest()` hook would DRY it. Optional. Note: do NOT lift the value into `SalaryCalculatorContext` — the query takes no variables (Apollo dedupes) and the two cards render in different steps, so leaf-level fetching is correct.

effectiveData?.salaryRequest,
hcmUser?.staffInfo.personNumber,
) ?? {};

// Disable the Continue button while the saved gross exceeds the SOSA cap.
useEffect(() => {
if (blockOnCap) {
markInvalid('over-cap');
} else {
markValid('over-cap');
}
return () => markValid('over-cap');
}, [blockOnCap, markValid, markInvalid]);

const minimumSalaryValue =
salaryCalculation?.calculations.minimumRequestedSalary;
const spouseMinimumSalaryValue =
salaryCalculation?.spouseCalculations?.minimumRequestedSalary;

const minimumSalary = formatCurrency(minimumSalaryValue);
const spouseMinimumSalary = formatCurrency(spouseMinimumSalaryValue);

const renderFormula = (
calculations: CalculationFieldsFragment | null | undefined,
) =>
t('MHA + {{ min }} - SECA', {
min: formatCurrency(calculations?.minimumRequiredSalary),
});
const formula = renderFormula(salaryCalculation?.calculations);
const spouseFormula = renderFormula(salaryCalculation?.spouseCalculations);

const schema = useMemo(
() =>
yup.object({
salary: amount(t('Requested salary'), t, {
required: true,
min: minimumSalaryValue,
minMessage: t('Requested salary must be at least {{min}}', {
min: minimumSalary,
}),
}),
spouseSalary: amount(t('Spouse requested salary'), t, {
required: true,
min: spouseMinimumSalaryValue,
minMessage: t('Spouse requested salary must be at least {{min}}', {
min: spouseMinimumSalary,
}),
}),
}),
[
t,
minimumSalaryValue,
minimumSalary,
spouseMinimumSalaryValue,
spouseMinimumSalary,
],
);

return (
<StepCard>
<CardHeader
title={t('Requested Salary')}
subheader={<EffectiveDateNote />}
/>
<CardContent>
<Typography variant="body1" data-testid="RequestedSalaryCard-message">
<Trans t={t}>
Below, enter the annual salary amount you would like to request.
This salary level includes taxes (local, state, and federal) and
Minister&apos;s Housing Allowance. It does not include either Social
Security (SECA) or 403b. They will be added in later.{' '}
</Trans>
{hcmSpouse ? (
<Trans t={t}>
Because of IRS and Cru requirements, the lowest salary you can
request is {{ minimumSalary }} ({{ formula }}) for{' '}
{{ name: hcmUser?.staffInfo.preferredName }} and{' '}
{{ spouseMinimumSalary }} ({{ spouseFormula }}) for{' '}
{{ spouseName: hcmSpouse?.staffInfo.preferredName }}.
</Trans>
) : (
<Trans t={t}>
Because of IRS and Cru requirements, the lowest salary you can
request is {{ minimumSalary }} ({{ formula }}).
</Trans>
)}{' '}
<Trans t={t}>
As you set your salary level, the amount you receive should reflect
the amount of time you work in ministry.
</Trans>
</Typography>

<Table>
<StepTableHead />
<TableBody>
<TableRow>
<TableCell component="th" scope="row">
{t('Current Salary')}
</TableCell>
<TableCell>
{formatCurrency(hcmUser?.currentSalary.grossSalaryAmount)}
Comment on lines -131 to -134

@zweatshirt zweatshirt Jun 2, 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.

@canac By the ticket description, I wasn't sure if we wanted to include both the Current Salary and the effective Current Requested Salary, or replace the Current Salary. I chose the latter but let me know if I should include both rows.

@canac canac Jun 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably not both. I think we wanted to show the current requested (not gross) so they can easily compare it with their new requested salary.

{t('Current Requested Salary')}
</TableCell>
<TableCell>{salary ? formatCurrency(salary) : '–'}</TableCell>

@zweatshirt zweatshirt Jun 2, 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.

[Medium] Empty-state divergence from the sibling card. This renders `–` for "no approved request," but every other money cell in this table — and all of `SalarySummaryCard` — renders `$0.00` (via `formatCurrency`'s `?? 0`). `SalarySummaryCard` handles the same "no effective request" state by omitting the whole "Old" column instead. The dash is defensible (it distinguishes "no prior request" from "$0.00 approved") and arguably an improvement — flagging only so it's a conscious choice rather than an accident. Confirm it's intentional.

@zweatshirt zweatshirt Jun 2, 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.

The '-' aligns with the ticket description, but I can change that.

{hcmSpouse && (
<TableCell>
{formatCurrency(hcmSpouse.currentSalary.grossSalaryAmount)}
{spouseSalary ? formatCurrency(spouseSalary) : '–'}

Check warning on line 147 in src/components/HrTools/SalaryCalculator/SalaryCalculation/RequestedSalaryCard/RequestedSalaryCard.tsx

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ Getting worse: Complex Method 

RequestedSalaryCard:React.FC increases in cyclomatic complexity from 21 to 24, 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.

@zweatshirt zweatshirt Jun 2, 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] Truthiness guard hides a legitimate `$0`. `spouseSalary ? formatCurrency(spouseSalary) : '–'` (and line 144) treats `0` as empty, so a real approved `$0` would render `–` instead of `$0.00`. Real-world risk is low — the requested-salary domain has a minimum-salary floor (Yup `min`) — so this may be exactly the intended behavior. If you want strict correctness, use `salary != null ? formatCurrency(salary) : '–'` and add a `salary: 0` test to pin the behavior. Flagged by 4 of 5 agents; author's call.

</TableCell>
)}
</TableRow>
Expand Down
15 changes: 14 additions & 1 deletion src/components/HrTools/SalaryCalculator/SalaryCalculatorTestWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ import {
HcmQueryVariables,
} from '../Shared/HcmData/Hcm.generated';
import { PayrollDatesQuery } from './EffectiveDateStep/PayrollDates.generated';
import { SalaryCalculationQuery } from './SalaryCalculatorContext/SalaryCalculation.generated';
import {
EffectiveSalaryCalculationQuery,
SalaryCalculationQuery,
} from './SalaryCalculatorContext/SalaryCalculation.generated';
import { SalaryCalculatorProvider } from './SalaryCalculatorContext/SalaryCalculatorContext';

const hcmMock = gqlMock<HcmQuery, HcmQueryVariables>(HcmDocument, {
Expand Down Expand Up @@ -91,8 +94,13 @@ export type SalaryRequestMock = DeepPartial<
SalaryCalculationQuery['salaryRequest']
>;

export type EffectiveSalaryRequestMock = DeepPartial<
EffectiveSalaryCalculationQuery['salaryRequest']
>;

export interface SalaryCalculatorTestWrapperProps {
salaryRequestMock?: SalaryRequestMock;
effectiveSalaryRequestMock?: EffectiveSalaryRequestMock;
hcmUser?: DeepPartial<HcmQuery['hcm'][number]>;
hcmSpouse?: DeepPartial<HcmQuery['hcm'][number]>;
onCall?: MockLinkCallHandler;
Expand All @@ -107,6 +115,7 @@ export const SalaryCalculatorTestWrapper: React.FC<
SalaryCalculatorTestWrapperProps
> = ({
salaryRequestMock,
effectiveSalaryRequestMock = null,
hcmUser,
hcmSpouse,
onCall,
Expand All @@ -133,6 +142,7 @@ export const SalaryCalculatorTestWrapper: React.FC<
Hcm: HcmQuery;
PayrollDates: PayrollDatesQuery;
SalaryCalculation: SalaryCalculationQuery;
EffectiveSalaryCalculation: EffectiveSalaryCalculationQuery;
GoalCalculatorConstants: GoalCalculatorConstantsQuery;
StaffAccount: StaffAccountQuery;
GetUser: GetUserQuery;
Expand All @@ -152,6 +162,9 @@ export const SalaryCalculatorTestWrapper: React.FC<
PayrollDates: {
payrollDates,
},
EffectiveSalaryCalculation: {
salaryRequest: effectiveSalaryRequestMock,
},
GoalCalculatorConstants: {
constant: {
mpdGoalGeographicConstants: [
Expand Down
Loading