-
Notifications
You must be signed in to change notification settings - Fork 14
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.
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
#2618 - Award Configurations - PT CSG-DEP (Award) #2728
Conversation
@@ -504,7 +504,7 @@ | |||
<bpmn:intermediateThrowEvent id="Event_10par1m" name="calculate CSGD"> | |||
<bpmn:extensionElements> | |||
<zeebe:ioMapping> | |||
<zeebe:output source="= if (calculatedDataTotalFamilyIncome <= dmnPartTimeAwardFamilySizeVariables.limitAwardCSGDIncomeCap) then dmnPartTimeAwardAllowableLimits.limitAwardCSGD3OrMoreChildAmount else max(dmnPartTimeAwardAllowableLimits.limitAwardCSGD3OrMoreChildAmount - ((calculatedDataTotalFamilyIncome - dmnPartTimeAwardFamilySizeVariables.limitAwardCSGDIncomeCap) * dmnPartTimeAwardFamilySizeVariables.limitAwardCSGD3OrMoreChildSlope), 80 )" target="federalAwardCSGDAmount" /> | |||
<zeebe:output source="=min( max( if (calculatedDataTotalFamilyIncome <= dmnPartTimeAwardFamilySizeVariables.limitAwardCSGDIncomeCap) then dmnPartTimeAwardAllowableLimits.limitAwardCSGD3OrMoreChildAmount else max(dmnPartTimeAwardAllowableLimits.limitAwardCSGD3OrMoreChildAmount - ((calculatedDataTotalFamilyIncome - dmnPartTimeAwardFamilySizeVariables.limitAwardCSGDIncomeCap) * dmnPartTimeAwardFamilySizeVariables.limitAwardCSGD3OrMoreChildSlope), 0 ) * offeringWeeks, dmnPartTimeAwardAllowableLimits.limitAwardCSGD3OrMoreChildAmount ), dmnPartTimeAwardAllowableLimits.limitAwardCSGDAmount )" target="federalAwardCSGDAmount" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per your description, the rounding is handled by the future ticket. Can we have the reference her as a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference to future ticket is commented in the story #2618.
} from "../../../test-utils/factories"; | ||
import { YesNoOptions } from "@sims/test-utils"; | ||
|
||
describe(`E2E Test Workflow parttime-assessment-${PROGRAM_YEAR}-awards-amount-CSGD for 3 or more dependants.`, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
}); | ||
}); | ||
|
||
describe(`E2E Test Workflow parttime-assessment-${PROGRAM_YEAR}-awards-amount-CSGD for less than 3 dependants.`, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we have one describe for a file, I am not against it but we is this okay for the other devs? @dheepak-aot @andrewsignori-aot @andrepestana-aot @sh16011993
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say we can follow the same here mainly because, the difference between 2 describe is the data for which we have been creating just more tests and we can follow the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The describe should be only one and similar to the file name. The part about the dependants should be include in the it()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test files are using only one describe now. Please have a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor doubt, Nice work @lewischen-aot
@@ -504,7 +504,7 @@ | |||
<bpmn:intermediateThrowEvent id="Event_10par1m" name="calculate CSGD"> | |||
<bpmn:extensionElements> | |||
<zeebe:ioMapping> | |||
<zeebe:output source="= if (calculatedDataTotalFamilyIncome <= dmnPartTimeAwardFamilySizeVariables.limitAwardCSGDIncomeCap) then dmnPartTimeAwardAllowableLimits.limitAwardCSGD3OrMoreChildAmount else max(dmnPartTimeAwardAllowableLimits.limitAwardCSGD3OrMoreChildAmount - ((calculatedDataTotalFamilyIncome - dmnPartTimeAwardFamilySizeVariables.limitAwardCSGDIncomeCap) * dmnPartTimeAwardFamilySizeVariables.limitAwardCSGD3OrMoreChildSlope), 80 )" target="federalAwardCSGDAmount" /> | |||
<zeebe:output source="=min( max( if (calculatedDataTotalFamilyIncome <= dmnPartTimeAwardFamilySizeVariables.limitAwardCSGDIncomeCap) then dmnPartTimeAwardAllowableLimits.limitAwardCSGD3OrMoreChildAmount else max(dmnPartTimeAwardAllowableLimits.limitAwardCSGD3OrMoreChildAmount - ((calculatedDataTotalFamilyIncome - dmnPartTimeAwardFamilySizeVariables.limitAwardCSGDIncomeCap) * dmnPartTimeAwardFamilySizeVariables.limitAwardCSGD3OrMoreChildSlope), 0 ) * offeringWeeks, dmnPartTimeAwardAllowableLimits.limitAwardCSGD3OrMoreChildAmount ), dmnPartTimeAwardAllowableLimits.limitAwardCSGDAmount )" target="federalAwardCSGDAmount" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO When I saw the weekly amount is written as large expression, I felt we can create a variable to get the weekly amount calculated first and use here.
@andrewsignori-aot @guru-aot @andrepestana-aot thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe creating a variable for the Weekly Amount is better. It can be reused if required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea to make it more readable with the new variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've verified with @CarlyCotton that multiplying offeringWeeks
only takes place for the else statement. The logic is the same, and I've updated the code for better readability. I've updated the screenshots in the description.
Please let me know if we want to move on to creating a new variable for calculating the nested if else statement. @dheepak-aot @guru-aot @andrepestana-aot @andrewsignori-aot @sh16011993
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would agree that the code is a bit hard to read while in Camunda and when I copied and pasted it into VS Code was still a good amount of horizontal code. Either way, I would not invest time in creating more variables at this time since it came from business and I believe that was clear for them for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @lewischen-aot I missed that multiplying offering weeks is only part of else.
calculatedAssessment.variables.dmnPartTimeAwardFamilySizeVariables | ||
.limitAwardCSGDIncomeCap, | ||
); | ||
const offeringWeeksAmount = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we not trying to assert federalAwardCSGDAmount
here.? and same everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review. It is now asserting federalAwardCSGDAmount
in the latest commit. Please have a look.
Nice work @lewischen-aot . Please have a look at the comments. |
...ttime-assessment/awards-amounts/parttime-assessment-2023-2024-awards-amount-CSGD.e2e-spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job. Please take a look at the other devs' comments.
…ed offeringweeks calculation in e2e tests and modified camunda indentation for better readability
34ce108
to
8b96a8b
Compare
); | ||
}); | ||
|
||
it("Should determine federalAwardCSGDAmount for less than 3 dependants and calculatedDataTotalFamilyIncome <= limitAwardCSGDIncomeCap", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the tests testing less than three dependents, should we not consider adding at least one dependent to the studentDataDependants
in at least one of the scenarios?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add it in the first place because it didn't change the result very much, but I added one dependent for less than 3 dependents scenario in my latest commit to complete the dependents scenarios.
calculatedAssessment.variables.dmnPartTimeAwardFamilySizeVariables | ||
.limitAwardCSGDIncomeCap, | ||
); | ||
const nestedCalculation = Math.max( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment outside the PR and for the future, I have been seeing this logic in the asserts trying to replicate in some way the logic in the workflow. I would recommend that when we have more control over all the data we should try to assert the values in the E2E in a more hard-coded way.
@dheepak-aot @andrepestana-aot @guru-aot @sh16011993 thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. we should not re-write the logic for assertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree too. We should assert hard-coded values instead of the logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work and thanks for adding the E2E tests. Left just some minor comments, hence approving it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work @lewischen-aot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the changes 👍
…e parents income in e2e tests
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Camunda screenshot for 3 or more dependants
Camunda screenshot for less than 3 depandents