-
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
#1777 - Process MSFAA Cancellation Notification e2e tests #2069
#1777 - Process MSFAA Cancellation Notification e2e tests #2069
Conversation
...msfaa-integration/_tests_/msfaa-full-time-process-response-integration.scheduler.e2e-spec.ts
Show resolved
Hide resolved
...msfaa-integration/_tests_/msfaa-full-time-process-response-integration.scheduler.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...msfaa-integration/_tests_/msfaa-part-time-process-response-integration.scheduler.e2e-spec.ts
Show resolved
Hide resolved
...msfaa-integration/_tests_/msfaa-part-time-process-response-integration.scheduler.e2e-spec.ts
Outdated
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.
added some comments
...msfaa-integration/_tests_/msfaa-full-time-process-response-integration.scheduler.e2e-spec.ts
Show resolved
Hide resolved
MSFAA_FULL_TIME_RECEIVE_FILE_WITH_SINGLE_CANCELLATION_RECORD, | ||
]); | ||
// Act | ||
// Now cancel the MSFAA record. |
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.
Please remove the "Now" or the entire comment line.
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.
Was this suggestion accepted? I am not able to see the change.
...msfaa-integration/_tests_/msfaa-full-time-process-response-integration.scheduler.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...msfaa-integration/_tests_/msfaa-part-time-process-response-integration.scheduler.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...msfaa-integration/_tests_/msfaa-full-time-process-response-integration.scheduler.e2e-spec.ts
Outdated
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.
Nice work, please take a look at the comments.
Good job! Just have one additional minor comments. Other DEVS have pretty much covered everything. |
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! Please look at the other devs comments. Don't wait for my approval 😉
expect(cancelledMSFAARecord.cancelledDate).toBe("2021-11-24"); | ||
expect(cancelledMSFAARecord.newIssuingProvince).toBe("ON"); | ||
expect(notification.dateSent).toBe(null); | ||
expect(notification.messagePayload).toStrictEqual({ |
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 adding this verification.
@ann-aot @guru-aot @dheepak-aot I believe that we can check the dynamic payload saved also as much as possible as part of the E2Es.
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.
Yes, this is good
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, look good 👍
@@ -132,12 +138,15 @@ describe( | |||
const msfaaIDs = createdMSFAARecords.map((msfaa) => msfaa.id); | |||
const msfaaUpdatedRecords = await db.msfaaNumber.find({ | |||
select: { | |||
id: true, |
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.
do we need id
?
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 @ann-aot . Removed 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 👍 just a minor comment
Kudos, SonarCloud Quality Gate passed!
|
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 👍
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 @sh16011993
When the full-time or part-time cancellation record files are received, the corresponding MSFAA records are cancelled and notification is persisted in the database.
As a part of this PR, e2e tests are written to validate the notifications when the MSFAA record gets cancelled.