-
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
#1761 - Handling full time BCSL lifetime and federal maximums #1886
Conversation
application.programYear.maxLifetimeBCLoanAmount; | ||
const auditUser = await this.systemUsersService.systemUser(); | ||
|
||
const totalLifeTimeAmount = |
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 take a look at the getOverawardBalance
. The idea for the e-Cert is that we can execute the methods in bulk and process the result in memory. Is the current way, unless I am missing something, for every single e-cert record there will be other 2 queries executed to get the sum from SIMS + SFAS.
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 we discussed, keeping it as it is. As we will be changing the implementation in the future ticket
@@ -8,7 +8,7 @@ import { | |||
} from "zeebe-node"; | |||
// TODO: In the upcoming tasks, either DisbursementScheduleService will be renamed at shared library | |||
// or MSFAA related methods will move to shared library. | |||
import { DisbursementScheduleService as DisbursementScheduleSharedService } from "@sims/services"; | |||
import { DisbursementScheduleSharedService } from "@sims/services"; |
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 refactor 👍
@@ -28,4 +28,10 @@ export enum RestrictionCode { | |||
* this restriction is applied case the SIN expiry date is before the offering end date. | |||
*/ | |||
SINF = "SINF", | |||
/** | |||
* BCSL Lifetime Maximum is the student's cumulative full-time BCSL awards received over | |||
* a lifetime. Thus restriction is added to a student when they reach BC lifetime maximum |
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 the intention to use "Thus" here or is it supposed to be "This"?
@@ -192,14 +200,30 @@ export class ECertGenerationService { | |||
END`, | |||
"stopFullTimeBCFunding", | |||
) | |||
.addSelect( |
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.
Is it not the case of, instead of having 2 subqueries, we start to bring all active restrictions and do the verifications on the in-memory list, which in this case will be "all active restrictions that can impact disbursement"?
I am assuming also the possibles StopFullTimeDisbursement
and StopPartTimeDisbursement
will be already excluded by the where condition at line 244, since these records will not even be selected.
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.
get the whole list and assign the latest restriction to the bc grants and loans when needed. I can give it a try
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.
#1886 (comment) but I have added
const newEffectiveAmount = | ||
disbursementValue.effectiveAmount - amountSubtracted; | ||
/** | ||
* Create {@link RestrictionCode.BCLM} restriction when lifetime maximum is reached/exceeded. |
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 Create {@link RestrictionCode.BCLM}
and I would support its usage in the methods comments.
At this level, I believe that it leaves the code less readable, also because you need the comment to be placed with /***/
.
application.id, | ||
); | ||
|
||
if (bclmRestriction) { |
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.
Edge case but a possible scenario.
Depending on certain circumstances we may be processing at the same time 2 disbursements for the same students, for instance, assuming that we have two applications as below.
Application A: Jan, 1st - April, 30th
Application B: May, 1st - July, 31st.
Wonder that Application A can be reassessed near the offering end date on April, 30th and it may be having the e-Cert generated on May, 1st, which will lead both to be part of the same e-Cert file.
The e-Cert SQL query will get both records where no BCSL restriction will be in place, but then the first one will be adding the BCSL restriction and the second one must be aware of it to stop the BC funding.
When we have 2 records for the same student, we may need to have a way to refresh the restrictions in a way that the second disbursement will not have the BC fundings disbursed.
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.
mm, That is a good edge case, are we managing the similar scenario overwards.? ya we need to find a way
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.
@dheepak-aot @guru-aot @andrepestana-aot @sh16011993 I and @andrewsignori-aot were discussing this and for the PR I am planning to add a directory that will be updated when a student gets a restriction for the first time, and as this directory as the reference the next disbursement of the same student will execute restriction logic. feel free to add your points
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 discussed we will handle this in a separate ticket
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.
Lets handle it in a separate ticket, can we add a TODO here?
.filter((disbursementValue) => { | ||
if ( | ||
disbursementValue.valueType === DisbursementValueType.BCGrant && | ||
disbursementValue.restrictionAmountSubtracted !== null |
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 do we need additional checks here? The idea of this method was to be concerned only about doing the SUM of the BC Grants if there is a value to be ignored it probably must be set to zero before this calculation happen.
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.
Here Iam trying to avoid the happy path disbursements, which doesn't have a restrictionAmountSubtracted
, so, these disbursements will not make up to the lifetime maximum calculations. are you saying to make the default for all restrictionAmountSubtracted
as zero in the db level?
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.
removed the logic got total bc grant
@@ -332,8 +439,35 @@ export class ECertGenerationService { | |||
.reduce((previousValue, currentValue) => { | |||
return previousValue + currentValue.effectiveAmount; | |||
}, 0); | |||
// Calculate total BC grants subtracted due to restriction. |
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.
Is it not supposed to be taken care of during the effective value calculation?
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.
@@ -419,7 +553,7 @@ export class ECertGenerationService { | |||
* @param disbursementValue award to be checked. | |||
* @returns true if the funding should not be disbursed, otherwise, false. | |||
*/ | |||
private shouldStopFunding( | |||
private shouldStopFullTimeBCFunding( |
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 got the point about changing this method's name but it actually needs to be adapted to support both part-time/full-time. Maybe keeping the name, adjusting the comments, and adding a todo would be better.
We need a separate ticket or maybe review the e-Cert query to have part-time also considered.
...es/backend/libs/integrations/src/services/disbursement-schedule/e-cert-generation.service.ts
Show resolved
Hide resolved
sources/packages/backend/libs/services/src/sfas/sfas-application.service.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/libs/services/src/sfas/sfas-application.service.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/libs/services/src/restriction/restriction-shared.service.ts
Show resolved
Hide resolved
...ages/backend/libs/services/src/disbursement-schedule/disbursement-schedule-shared.service.ts
Outdated
Show resolved
Hide resolved
for (const disbursement of disbursements) { | ||
for (const disbursementValue of disbursement.disbursementValues) { | ||
if (this.shouldStopFunding(disbursement, disbursementValue)) { | ||
if (this.shouldStopFullTimeBCFunding(disbursement, disbursementValue)) { |
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.
@dheepak-aot that is not part of the PR, The first 2 order is already implemented in our system, this PR is implementing the 3rd order and some changes in the 2nd order. That point for just to make sure we follow the order
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 discussed, can slightly add context to first point in the story like it is already happening in ecert disbursements selection criteria.
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.
Updated the story, as discussed
Great to see the BCSL life time maximum coming up @ann-aot . Add some 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.
LGTM, nice work @ann-aot
* @param studentId student id. | ||
* @returns total BCSL amount that the student received from the legacy(sfas) system. | ||
*/ | ||
async totalLegacyBCSLAmount(studentId: number): Promise<number> { |
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 part of the upcoming ticket, we can also discuss the possibility of stopping doing the calculation every time and maybe have the SUM for the students consolidated in some other table on SIMS.
sources/packages/backend/libs/sims-db/src/entities/student.model.ts
Outdated
Show resolved
Hide resolved
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 and creating new ticket for the new way of processing disbursements. LGTM. 👍
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, looks 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.
LGTM.
calculateEffectiveValue
calculation.BCLM
restriction is added to the student.sfasApplication
table with sin, birthdate, and last name. ( IF the student has a temporary SIN in SFAS and a permanent SIN in SIMS, we expect to miss the details from SFAS.)createRestrictionToSave
,getRestrictionByCode
.