-
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
#4226 - NOA Eligible to Receive - Double Counting BC Grants #4238
Conversation
@@ -156,6 +156,111 @@ describe("AssessmentStudentsController(e2e)-getAssessmentNOA", () => { | |||
.expect(expectation); | |||
}); | |||
|
|||
it.only("Should exclude BC Total Grant from eligible amount calculation when getting NOA details", 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.
Please remove only
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.
Only removed.
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 @bidyashish 👍
.expect(HttpStatus.OK); | ||
|
||
// Verify that eligibleAmount excludes BC Total Grant | ||
expect(response.body.eligibleAmount).toBe(3060); // Sum of CSPT(2520) + BCAG(140) + SBSD(400), excluding BCSG(540) |
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 with the comment.
@@ -156,6 +156,111 @@ describe("AssessmentStudentsController(e2e)-getAssessmentNOA", () => { | |||
.expect(expectation); | |||
}); | |||
|
|||
it.only("Should exclude BC Total Grant from eligible amount calculation when getting NOA details", 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.
On top of that, E2E tests are failing. Please address.
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.
Pushed the latest changes to working of E2E.
@@ -200,14 +200,14 @@ export class AssessmentControllerService { | |||
disbursementSchedules: DisbursementSchedule[], | |||
): number { | |||
return disbursementSchedules | |||
.flatMap( | |||
(disbursementSchedule) => disbursementSchedule.disbursementValues, | |||
.flatMap((disbursementSchedule) => |
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 advise to use the filter after the flatmap instead. like this.
return disbursementSchedules
.flatMap(
(disbursementSchedule) => disbursementSchedule.disbursementValues,
)
.filter(
(disbursementValue) =>
disbursementValue.valueType !== DisbursementValueType.BCTotalGrant,
)
.reduce(
(accumulator, disbursementValue) =>
accumulator + disbursementValue.valueAmount,
0,
);
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.
filter separated from flatMap operation.
await db.msfaaNumber.save(msfaaNumber); | ||
|
||
// Create application with disbursements | ||
const application = await saveFakeApplicationDisbursements( |
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 saveFakeApplicationDisbursements
can do the whole date preparation and we can discard all the code from line 185-237.
const application = await saveFakeApplicationDisbursements(
db.dataSource,
{
msfaaNumber,
student,
disbursementValues: [
// BC Total Grant (excluded from eligible amount)
createFakeDisbursementValue(
DisbursementValueType.BCTotalGrant,
"BCSG",
540,
),
// Canada Student Loans and Grants
createFakeDisbursementValue(
DisbursementValueType.CanadaLoan,
"CSLF",
0,
),
createFakeDisbursementValue(
DisbursementValueType.CanadaGrant,
"CSGP",
0,
),
createFakeDisbursementValue(
DisbursementValueType.CanadaGrant,
"CSPT",
2520,
),
// BC Grants
createFakeDisbursementValue(
DisbursementValueType.BCGrant,
"BCAG",
140,
),
createFakeDisbursementValue(
DisbursementValueType.BCGrant,
"SBSD",
400,
),
],
},
{
offeringIntensity: OfferingIntensity.fullTime,
},
);
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.
In the code snippet above, I used (CSLF instead of CSLP) to be in sync with the offering intensity.
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 @dheepak-aot
Grant assert values updated.
.expect(HttpStatus.OK); | ||
|
||
// Verify that eligibleAmount excludes BC Total Grant | ||
expect(response.body.eligibleAmount).toBe(3060); // Sum of CSPT(2520) + BCAG(140) + SBSD(400), excluding BCSG(540) |
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 assert the whole response as a common practice that we have been following. For e.g. the test case in same file. You can achieve it this way.
const assessment = application.currentAssessment;
const [disbursement] = assessment.disbursementSchedules;
const endpoint = `/students/assessment/${assessment.id}/noa`;
const studentUserToken = await getStudentToken(
FakeStudentUsersTypes.FakeStudentUserType1,
);
const expectedNOADetails = {
applicationId: application.id,
applicationNumber: application.applicationNumber,
applicationStatus: application.applicationStatus,
assessment: assessment.assessmentData,
noaApprovalStatus: assessment.noaApprovalStatus,
applicationCurrentAssessmentId: application.currentAssessment.id,
fullName: getUserFullName(application.student.user),
programName: assessment.offering.educationProgram.name,
locationName: assessment.offering.institutionLocation.name,
offeringIntensity: OfferingIntensity.fullTime,
offeringStudyEndDate: getDateOnlyFullMonthFormat(
assessment.offering.studyEndDate,
),
offeringStudyStartDate: getDateOnlyFullMonthFormat(
assessment.offering.studyStartDate,
),
// Sum of CSPT(2520) + BCAG(140) + SBSD(400), excluding BCSG(540)
eligibleAmount: 3060,
disbursement: {
disbursement1COEStatus: disbursement.coeStatus,
disbursement1Date: getDateOnlyFullMonthFormat(
disbursement.disbursementDate,
),
disbursement1Id: disbursement.id,
disbursement1MSFAACancelledDate:
disbursement.msfaaNumber?.cancelledDate,
disbursement1MSFAADateSigned: disbursement.msfaaNumber?.dateSigned,
disbursement1MSFAAId: disbursement.msfaaNumber?.id,
disbursement1MSFAANumber: msfaaNumber.msfaaNumber,
disbursement1Status: disbursement.disbursementScheduleStatus,
disbursement1TuitionRemittance:
disbursement.tuitionRemittanceRequestedAmount,
disbursement1cslf: 0,
disbursement1csgp: 0,
disbursement1cspt: 2520,
disbursement1bcag: 140,
disbursement1sbsd: 400,
},
offeringName: assessment.offering.name,
};
// Act/Assert
await request(app.getHttpServer())
.get(endpoint)
.auth(studentUserToken, BEARER_AUTH_TYPE)
.expect(HttpStatus.OK)
.expect(expectedNOADetails);
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.
new Assert pushed with CSGF for full time offering.
|
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.
Looks good @bidyashish 👍Great job!
**Acceptance Criteria** - [x] Update the Eligible to Receive amount to not include **BC Total Grant** from the `disbursement_values` so it's not being double counted. - [x] Check post ecert calculation. This is when the BC Total grant is added to the table. **Technical** - [x] If E2E tests are in place please adjust it to cover this scenario, including the "BC Total Grant" in the data setup and asserting the expected value. E2E test data is taken from sample data.
…4242) **Acceptance Criteria** - [x] Update the Eligible to Receive amount to not include **BC Total Grant** from the `disbursement_values` so it's not being double counted. - [x] Check post ecert calculation. This is when the BC Total grant is added to the table. **Technical** - [x] If E2E tests are in place please adjust it to cover this scenario, including the "BC Total Grant" in the data setup and asserting the expected value. E2E test data is taken from sample data.
Acceptance Criteria
disbursement_values
so it's not being double counted.Technical
E2E test data is taken from sample data.