Conversation
Bundle sizes [mpdx-react]Compared against 35fb128
|
|
Preview branch generated at https://MPDX-9368.d3dytjb8adxkk5.amplifyapp.com |
|
For the review would you be able to rebase to get rid of my commits? Tough to read through what changes are yours and which were mine. Sorry about that Will |
zweatshirt
left a comment
There was a problem hiding this comment.
UI looks amazing! My only suggestion is to pull the MpdGoalMiscConstant API constants instead of hardcoding them. Could we do that quickly?
| workCompPercentage: 0.17, | ||
| attritionRate: 0.06, | ||
| creditCardFeeRate: 0.06, |
There was a problem hiding this comment.
These constants should be available on the MpdGoalMiscConstant GraphQL query, we can replace this to pull those directly instead of hardcoding them. They'll be changeable by admins. The credit card fee rate is .006
| const attrition = subtotal * constants.attritionRate; | ||
| const creditCardFees = (subtotal + attrition) * constants.creditCardFeeRate; | ||
| const preAssessment = subtotal + attrition + creditCardFees; | ||
| const assessment = preAssessment / 0.88 - preAssessment; |
There was a problem hiding this comment.
I don't remember what this .88 is. Do we want to consider making this into a constant in MpdGoalMiscConstant as well?
There was a problem hiding this comment.
Yes sorry this was before I updated.
zweatshirt
left a comment
There was a problem hiding this comment.
Just noticed you fixed the constants issue 💯
| describe('null/undefined status', () => { | ||
| it('excludes both workComp and benefits when status is null', () => { | ||
| const result = calculateOtherExpenses(noStatus(), defaultConstants); | ||
| expect(result.workComp).toBe(0); | ||
| expect(result.benefits).toBe(0); | ||
| }); |
There was a problem hiding this comment.
Wondering if we should actually make status non-nullable at some point... I don't think there should ever be a time this would be null in practice? I guess if the API pull from HCM fails. I know at some point we need to block 'Continue' on the Setup Step until the user has input all fields.
| expect(result.reimbursableExpenses).toBe(500); | ||
| expect(result.fourOThreeBContributions).toBeCloseTo(400); | ||
| expect(result.workComp).toBe(0); | ||
| expect(result.benefits).toBe(1500); | ||
| expect(result.subtotal).toBeCloseTo(7400); | ||
| expect(result.attrition).toBeCloseTo(444); | ||
| expect(result.creditCardFees).toBeCloseTo(470.64); | ||
| expect(result.assessment).toBeCloseTo(997.76, 1); |
There was a problem hiding this comment.
Curious why we use toBeCloseTo in some instances and toBe in others? Is this possible rounding or truncation?
There was a problem hiding this comment.
I think toBeCloseTo is for anything derived from a potential Float
There was a problem hiding this comment.
Not sure why it would make a difference as everything is just an int in JS. I'd assume it's just for rounding what could be a very large integer
Description
Related ticket: MPDX-9368
Depends on: #1721 (MPDX-9367 — SalarySection component)
Testing
Checklist:
/pr-reviewcommand locally and fixed any relevant suggestions