Skip to content

[No Jira] - Refactor PDS Goal Calculator part-time fees to be amount rather than percent #1817

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

[No Jira] - Refactor PDS Goal Calculator part-time fees to be amount rather than percent #1817

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
2 changes: 1 addition & 1 deletion src/components/HrTools/PdsGoalCalculator/SupportItem/otherBreakdown.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const constants: OtherExpensesConstants = {
salarySubtotal: 5000,
fourOThreeBPercentage: 0.08,
grossMonthlyPay: 5000,
workCompPercentage: 0.02,
workCompAmount: 100,
attritionRate: 0.05,
creditCardFeeRate: 0.03,
adminRate: 0.1,
Expand Down
8 changes: 4 additions & 4 deletions src/components/HrTools/PdsGoalCalculator/calculations/OtherExpenses.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const defaultConstants: OtherExpensesConstants = {
salarySubtotal: 5000,
fourOThreeBPercentage: 0.1,
grossMonthlyPay: 4000,
workCompPercentage: 0.17,
workCompAmount: 680,
attritionRate: 0.06,
creditCardFeeRate: 0.06,
adminRate: 0.12,
Expand Down Expand Up @@ -57,9 +57,9 @@ describe('calculateOtherExpenses', () => {
});

describe('work comp', () => {
it('calculates as grossMonthlyPay × workCompPercentage for part-time', () => {
it('uses the fixed workCompAmount for part-time', () => {
const result = calculateOtherExpenses(partTime(), defaultConstants);
// 4000 * 0.17
// fixed amount
expect(result.workComp).toBeCloseTo(680);

@wjames111 wjames111 Jun 3, 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] **Test became near-tautological after rename.** Previously this validated arithmetic (`grossMonthlyPay × workCompPercentage = 4000 × 0.17 = 680`). Now it asserts that `workCompAmount` flows through unchanged when part-time, but the expected value `680` echoes `defaultConstants.workCompAmount: 680`, so the test mostly proves identity.

It still meaningfully exercises the part-time branch, so this is not a defect. If you want to make intent obvious, use a constant override:

const result = calculateOtherExpenses(partTime(), {
  ...defaultConstants,
  workCompAmount: 123.45,
});
expect(result.workComp).toBe(123.45); // identity, not arithmetic

Optional. Flagged by Testing Agent.

});

@wjames111 wjames111 Jun 3, 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] **Consider adding a regression test that `workComp` no longer depends on `grossMonthlyPay`.** This locks in the new semantics and prevents accidental reintroduction of the percentage formula. 
it('does not depend on grossMonthlyPay for part-time', () => {
  const lowPay = calculateOtherExpenses(partTime(), { ...defaultConstants, grossMonthlyPay: 100 });
  const highPay = calculateOtherExpenses(partTime(), { ...defaultConstants, grossMonthlyPay: 10000 });
  expect(lowPay.workComp).toBe(highPay.workComp);
});

Optional. Flagged by Testing Agent.


Expand Down Expand Up @@ -183,7 +183,7 @@ describe('calculateOtherExpenses', () => {

it('produces correct totals for a part-time employee', () => {
const result = calculateOtherExpenses(partTime(), defaultConstants);
// reimbursable=500, 403b=400, workComp=4000*0.17=680, benefits=0
// reimbursable=500, 403b=400, workComp=680 (fixed), benefits=0
// subtotal=5000+500+400+680+0=6580
// attrition=6580*0.06=394.80
// creditCardFees=(6580+394.80)/(1-0.06)-(6580+394.80)≈445.20
Expand Down
6 changes: 2 additions & 4 deletions src/components/HrTools/PdsGoalCalculator/calculations/OtherExpenses.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export interface OtherExpensesConstants {
salarySubtotal: number;
fourOThreeBPercentage: number;
grossMonthlyPay: number;
workCompPercentage: number;
workCompAmount: number;
attritionRate: number;
creditCardFeeRate: number;
adminRate: number;
Expand All @@ -41,9 +41,7 @@ export const calculateOtherExpenses = (
const reimbursableExpenses = constants.reimbursableTotal;
const fourOThreeBContributions =
constants.grossMonthlyPay * constants.fourOThreeBPercentage;
const workComp = isPartTime
? constants.grossMonthlyPay * constants.workCompPercentage
: 0;
const workComp = isPartTime ? constants.workCompAmount : 0;
const benefits = isFullTime ? (calculation.benefits ?? 0) : 0;

const subtotal =
Expand Down
4 changes: 2 additions & 2 deletions src/components/HrTools/PdsGoalCalculator/calculations/buildPdsGoalConstants.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const buildMiscConstants = (
: { EMPLOYER_FICA_RATE: makeConstant(0.08) }),
...(overrides.workComp !== undefined
? { PART_TIME_WORK_COMPENSATION: makeConstant(overrides.workComp) }
: { PART_TIME_WORK_COMPENSATION: makeConstant(0.17) }),
: { PART_TIME_WORK_COMPENSATION: makeConstant(20.35) }),
...(overrides.creditCardFeeRate !== undefined
? { CREDIT_CARD_FEE_RATE: makeConstant(overrides.creditCardFeeRate) }
: { CREDIT_CARD_FEE_RATE: makeConstant(0.06) }),
Expand Down Expand Up @@ -59,7 +59,7 @@ describe('buildPdsGoalConstants', () => {

expect(result).toEqual({
employerFicaRate: 0.08,
workCompPercentage: 0.17,
workCompAmount: 20.35,
attritionRate: 0.06,
creditCardFeeRate: 0.06,
adminRate: 0.12,
Expand Down
10 changes: 5 additions & 5 deletions src/components/HrTools/PdsGoalCalculator/calculations/pdsGoalConstants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { SalaryTotals } from './salaryCalculation';

export interface PdsGoalTotalConstants {
employerFicaRate: number;
workCompPercentage: number;
workCompAmount: number;
attritionRate: number;
creditCardFeeRate: number;
adminRate: number;
Expand All @@ -29,14 +29,14 @@ export const buildPdsGoalConstants = (
const rates = goalMiscConstants.RATES;

const employerFicaRate = additionalRates?.EMPLOYER_FICA_RATE?.fee;
const workCompPercentage = additionalRates?.PART_TIME_WORK_COMPENSATION?.fee;
const workCompAmount = additionalRates?.PART_TIME_WORK_COMPENSATION?.fee;

@wjames111 wjames111 Jun 3, 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] **Semantic drift on `PART_TIME_WORK_COMPENSATION`.** The enum is grouped under `ADDITIONAL_RATES` and the field is called `fee`, but it's now a fixed monthly USD amount, not a rate. A future reader (or a new consumer of `MpdGoalMiscConstantLabelEnum.PartTimeWorkCompensation`) could easily multiply this by a salary and silently produce a wrong number. Since the GraphQL schema is shared with the backend, renaming the enum is out of scope here — but locking the new contract with a one-line comment at the point of use is cheap insurance. 
   const employerFicaRate = additionalRates?.EMPLOYER_FICA_RATE?.fee;
+  // PART_TIME_WORK_COMPENSATION.fee is a fixed monthly USD amount, not a rate
+  // (despite the enum's placement under ADDITIONAL_RATES and the `.fee` field name).
   const workCompAmount = additionalRates?.PART_TIME_WORK_COMPENSATION?.fee;

Flagged by Architecture Agent.

const attritionRate = rates?.ATTRITION_RATE?.fee;
const creditCardFeeRate = additionalRates?.CREDIT_CARD_FEE_RATE?.fee;
const adminRate = rates?.ADMIN_RATE?.fee;

if (
employerFicaRate === undefined ||
workCompPercentage === undefined ||
workCompAmount === undefined ||
attritionRate === undefined ||
creditCardFeeRate === undefined ||
adminRate === undefined
Expand All @@ -52,7 +52,7 @@ export const buildPdsGoalConstants = (

return {
employerFicaRate,
workCompPercentage,
workCompAmount,
attritionRate,
creditCardFeeRate,
adminRate,
Expand All @@ -73,7 +73,7 @@ export const buildOtherExpensesConstants = (
salarySubtotal: salaryTotals.subtotal,
fourOThreeBPercentage: isSimple ? 0 : constants.fourOThreeBPercentage,
grossMonthlyPay: salaryTotals.grossMonthlyPay,
workCompPercentage: constants.workCompPercentage,
workCompAmount: constants.workCompAmount,
attritionRate: constants.attritionRate,
creditCardFeeRate: constants.creditCardFeeRate,
adminRate: constants.adminRate,
Expand Down
24 changes: 12 additions & 12 deletions src/components/HrTools/PdsGoalCalculator/calculations/usePdsSummaryData.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const mockUseGoalCalculatorConstants =
>;

const EMPLOYER_FICA_RATE = 0.08;
const WORK_COMP_PERCENTAGE = 0.17;
const WORK_COMP_AMOUNT = 400;
const ATTRITION_RATE = 0.06;
const CREDIT_CARD_FEE_RATE = 0.06;
const ADMIN_RATE = 0.12;
Expand All @@ -51,7 +51,7 @@ const constantsMock = gqlMock<GoalCalculatorConstantsQuery>(
{
category: MpdGoalMiscConstantCategoryEnum.AdditionalRates,
label: MpdGoalMiscConstantLabelEnum.PartTimeWorkCompensation,
fee: WORK_COMP_PERCENTAGE,
fee: WORK_COMP_AMOUNT,
},
{
category: MpdGoalMiscConstantCategoryEnum.AdditionalRates,
Expand Down Expand Up @@ -151,7 +151,7 @@ describe('usePdsSummaryData', () => {
},
],
[
'workCompPercentage',
'workCompAmount',
{
ADDITIONAL_RATES: {
...defaultConstants.goalMiscConstants.ADDITIONAL_RATES,
Expand Down Expand Up @@ -329,14 +329,14 @@ describe('usePdsSummaryData', () => {
// Reimbursable: monthly = 400, annual = 1200, raw = 500, above floor
//
// 403b = 2166.667 * 0.08 = 173.333
// workComp = 2166.667 * 0.17 = 368.333 (part-time)
// workComp = 400 (fixed amount, part-time)
// benefits = 0 (part-time, ignores calculation.benefits)
// subtotal = 2340 + 500 + 173.333 + 368.333 + 0 = 3381.667
// attrition = 3381.667 * 0.06 = 202.9
// creditCardFees = (3381.667 + 202.9) / (1 - 0.06) - (3381.667 + 202.9) ≈ 228.80
// adminBase ≈ 3813.37
// assessment = adminBase / (1 - 0.12) - adminBase ≈ 520.00
// overallTotal ≈ 3381.667 + 202.9 + 228.80 + 520.00 ≈ 4333.37
// subtotal = 2340 + 500 + 173.333 + 400 + 0 = 3413.333
// attrition = 3413.333 * 0.06 = 204.8
// creditCardFees = (3413.333 + 204.8) / (1 - 0.06) - (3413.333 + 204.8) ≈ 230.94
// adminBase ≈ 3849.08
// assessment = adminBase / (1 - 0.12) - adminBase ≈ 524.87
// overallTotal ≈ 3413.333 + 204.8 + 230.94 + 524.87 ≈ 4373.95
const calc = {
...defaultCalculation,
salaryOrHourly: DesignationSupportSalaryType.Hourly,
Expand All @@ -348,8 +348,8 @@ describe('usePdsSummaryData', () => {
const { result } = renderHook(() =>
usePdsSummaryData(calc, defaultHcmUser),
);
expect(result.current.data?.overallTotal).toBeCloseTo(4333.37, 0);
expect(result.current.data?.otherTotals.workComp).toBeCloseTo(368.33, 2);
expect(result.current.data?.overallTotal).toBeCloseTo(4373.95, 0);
expect(result.current.data?.otherTotals.workComp).toBeCloseTo(400, 2);
expect(result.current.data?.otherTotals.benefits).toBe(0);
});

Expand Down
Loading