Skip to content

No-Jira - PDS Goal Calculator: Quick Geographic Location fix #1793

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
May 21, 2026
+28 −3
Merged

No-Jira - PDS Goal Calculator: Quick Geographic Location fix #1793

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
22 changes: 22 additions & 0 deletions src/components/HrTools/PdsGoalCalculator/SupportItem/salaryBreakdown.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,28 @@
expect(byId['total']).toBeCloseTo(4680.0, 2);
});

it('shows "Monthly Base" formula when geographic multiplier is 0', () => {
const rows = buildSalaryBreakdownRows(
salariedCalculation,
{ ...constants, geographicMultiplier: 0 },
'en-US',
i18n.t,
);
const row = rows.find((r) => r.id === 'gross-monthly-pay');
expect(row?.formula).toBe('Monthly Base');
Copy link
Copy Markdown
Contributor Author

@zweatshirt zweatshirt May 21, 2026

Choose a reason for hiding this comment

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

[Suggestion] This test verifies the formula label but not that `row?.amount` equals the monthly base when the multiplier is 0. The amount is covered by a separate test elsewhere, but asserting both here would lock label/amount agreement in one place: 
expect(row?.formula).toBe('Monthly Base');
expect(row?.amount).toBe(salariedCalculation.monthlyBase); // or the expected number

Not blocking — just strengthens the invariant.

});

Check warning on line 111 in src/components/HrTools/PdsGoalCalculator/SupportItem/salaryBreakdown.test.tsx

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ New issue: Code Duplication 

The module contains 4 functions with similar structure: 'inserts hours-per-week and monthly-base rows for hourly','returns the salaried row sequence','shows "Monthly Base × Geographic Multiplier (rate)" formula when multiplier is non-zero','shows "Monthly Base" formula when geographic multiplier is 0'. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.

it('shows "Monthly Base × Geographic Multiplier (rate)" formula when multiplier is non-zero', () => {
Copy link
Copy Markdown
Contributor Author

@zweatshirt zweatshirt May 21, 2026

Choose a reason for hiding this comment

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

[Suggestion] The non-zero branch is covered with `0.06`. If negative multipliers are reachable from `goalGeographicConstantMap` (sub-baseline cost-of-living regions), a negative test case would document the intended behavior (e.g. `-0.06` → `94%`). Skip if negative values aren't possible in practice.

const rows = buildSalaryBreakdownRows(
salariedCalculation,
{ ...constants, geographicMultiplier: 0.06 },
'en-US',
i18n.t,
);
const row = rows.find((r) => r.id === 'gross-monthly-pay');
expect(row?.formula).toBe('Monthly Base × Geographic Multiplier (106%)');
});

it('inserts hours-per-week and monthly-base rows for hourly', () => {
const rows = buildSalaryBreakdownRows(
hourlyCalculation,
Expand Down
9 changes: 6 additions & 3 deletions src/components/HrTools/PdsGoalCalculator/SupportItem/salaryBreakdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,12 @@ export const buildSalaryBreakdownRows = (
{
id: 'gross-monthly-pay',
category: t('Gross Monthly Pay'),
formula: t('Monthly Base × Geographic Multiplier ({{rate}})', {
rate: percentageFormat(1 + geographicMultiplier, locale),
}),
formula:
geographicMultiplier === 0
Copy link
Copy Markdown
Contributor Author

@zweatshirt zweatshirt May 21, 2026

Choose a reason for hiding this comment

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

[Suggestion] The strict `=== 0` guard is correct given that `geographicMultiplier` comes from `goalGeographicConstantMap` with `?? 0` (only exact zero is the "no adjustment" sentinel). Worth a brief comment to document this assumption so a future reader doesn't wonder whether very small values (e.g. `0.0001`) should also render "Monthly Base".

? t('Monthly Base')
: t('Monthly Base × Geographic Multiplier ({{rate}})', {
rate: percentageFormat(1 + geographicMultiplier, locale),
}),
amount: grossMonthlyPay,
format: 'currency',
testId: 'gross-monthly-pay',
Expand Down
Loading