-
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
#4024 - SFAS to SIMS Bridge - CSL/BCSL Overawards - PR 2 #4251
#4024 - SFAS to SIMS Bridge - CSL/BCSL Overawards - PR 2 #4251
Conversation
@@ -245,6 +245,73 @@ describe("StudentAccountApplicationAESTController(e2e)-approveStudentAccountAppl | |||
}); | |||
}); | |||
|
|||
it("Should create disbursement overawards when an exact match is found with matching last name, birth dates and SIN for importing a student record with disbursement overawards from SFAS.", 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.
This is a valid test and we can keep it, it is just not the code that would revert the deleted records.
The process would happen during the execution of the SFAS scheduler (sfas-integration.scheduler) during the updateDisbursementOverawards.
To create an E2E test, it needs to be under queue-consumers importing an SFAS bridge record for an individual with overawards data.
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.
@andrewsignori-aot I have some confusion related to the test organization. I thought we put tests in their respective place based on the api the test is invoking. Am I missing something?
I am guessing the idea is to put it with queue-consumers because it tests the scheduler's job of updating the overawards if a sims and sfas student match is found.
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.
@sh16011993 the current test is this tile is testing the API call and should stay where it is.
My comment, if not clear, is to have a second E2E test created for the queue-consumers.
legacy overawards are imported during student account creation and during the SFAS bridge process.
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.
Changes looks good, the E2E created makes sense for the student creation scenario but the most critical to ensure the data would be recovered if deleted would be the one in queue-consumers. I added the details on the comment, please let me know if need further information.
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 @lewischen-aot 👍
const disbursementOverawards = await db.disbursementOveraward.find({ | ||
select: { overawardValue: true }, | ||
where: { | ||
student: { id: student.id }, | ||
originType: DisbursementOverawardOriginType.LegacyOveraward, | ||
}, | ||
}); | ||
expect(disbursementOverawards).toEqual( | ||
expect.arrayContaining([ | ||
{ overawardValue: 714.3 }, | ||
{ overawardValue: 741.3 }, | ||
]), | ||
); | ||
}, | ||
); |
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 the E2E test to assert the import. While validating the disbursement overawards, please also validate each overward against the code.
const disbursementOverawards = await db.disbursementOveraward.find({
select: { disbursementValueCode: true, overawardValue: true },
where: {
student: { id: student.id },
originType: DisbursementOverawardOriginType.LegacyOveraward,
},
});
expect(disbursementOverawards).toEqual(
expect.arrayContaining([
{
disbursementValueCode: BC_STUDENT_LOAN_AWARD_CODE,
overawardValue: 714.3,
},
{
disbursementValueCode: CANADA_STUDENT_LOAN_FULL_TIME_AWARD_CODE,
overawardValue: 741.3,
},
]),
);
|
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 @lewischen-aot. Looks good👍
sims.disbursement_overawards
where the origin type is "Legacy overaward".Rollback evidence
Please note that the
down
method for the DB migration class doesn't contain a revert script for rollback, where a comment is used instead to explain the reason and recovery process.