-
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
#2289 - Implement logic for Application Event Code in IER12 - code and date assignment and unit tests (Part 4) #2420
Conversation
...tion-event-code-during-assessment-utils.service.applicationEventCodeDuringAssessment.spec.ts
Show resolved
Hide resolved
...ode-during-enrolment-and-completed-utils.service.applicationEventCodeDuringCompleted.spec.ts
Outdated
Show resolved
Hide resolved
...ode-during-enrolment-and-completed-utils.service.applicationEventCodeDuringCompleted.spec.ts
Outdated
Show resolved
Hide resolved
...ode-during-enrolment-and-completed-utils.service.applicationEventCodeDuringCompleted.spec.ts
Outdated
Show resolved
Hide resolved
...nrolment-and-completed-utils.service.applicationEventCodeDuringEnrolmentAndCompleted.spec.ts
Outdated
Show resolved
Hide resolved
record.appendOptionalStringWithFiller(this.applicationEventCode, 4); | ||
record.appendOptionalFormattedDate(this.applicationEventDate); | ||
record.appendStringWithFiller(this.applicationEventCode, 4); | ||
record.appendFormattedDate(this.applicationEventDate); |
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.
👍
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.
👍
...nrolment-and-completed-utils.service.applicationEventCodeDuringEnrolmentAndCompleted.spec.ts
Show resolved
Hide resolved
...ode-during-enrolment-and-completed-utils.service.applicationEventCodeDuringCompleted.spec.ts
Outdated
Show resolved
Hide resolved
Done with the review, please have a look on the other dev comments |
...ode-during-enrolment-and-completed-utils.service.applicationEventCodeDuringCompleted.spec.ts
Outdated
Show resolved
Hide resolved
disbursementScheduleStatus: DisbursementScheduleStatus.Sent, | ||
disbursementFeedbackErrors: [ | ||
{ | ||
errorCode: "XXXXX", |
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.
👍
...ls-service/_tests_/unit/application-event-code-utils.service.getApplicationEventCode.spec.ts
Outdated
Show resolved
Hide resolved
payload, | ||
); | ||
// Assert | ||
expect( |
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.
To clarify, isn't our intention here to assert the final event code obtained (including the expect that we have now)? I know we don't have to cover all the scenarios which happens inside this method as they are covered in the unit tests for child methods.
Same in other places
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 unit tests should cover the scenarios for the method being tested. For this specific method, it should validate the switch
logic mainly, and, as you mentioned, the other methods should be validated by the other unit tests.
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.
Initially, I had a similar qn and discussed it with @andrewsignori-aot , Here except for canceled
. all other services are covered in the separate unit tests, So here the intention is to test the switch cases.
Nice work @ann-aot . Please have a look at the comments. I totally missed something in the past PR about |
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 with the unit test. Please take a look at the minor comments.
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 making the changes and for taking care of the change suggested for hasAwardWithheldDueToRestriction
. Looks good 👍
Thanks @dheepak-aot for the nice catch 😉
* @returns true if there is any partial or full award amount, that | ||
* was withheld due to a restriction. | ||
*/ | ||
private hasAwardWithheldDueToRestriction( |
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 making this change 👍
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 job! thanks for doing the withheld restriction logic change in last minute.
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, @ann-aot nice work
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.
Nice work @ann-aot 👍
expect(applicationEventDate).toBe(updatedAt); | ||
}); | ||
|
||
it(`Should return payloadDisbursementSchedule.disbursementDate - DISBURSEMENT_FILE_GENERATION_ANTICIPATION_DAYS when application event code is ${ApplicationEventCode.DISR}.`, () => { |
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.
Should DISBURSEMENT_FILE_GENERATION_ANTICIPATION_DAYS
be evaluated here as the other vars or just a text like "disbursement file generation anticipation days"?
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. Just left a minor comment.
Kudos, SonarCloud Quality Gate passed!
|
Final PR for the story
appendOptionalStringWithFiller
withappendStringWithFiller
for both application event code and application event date.