Conversation
There was a problem hiding this comment.
Pull request overview
This PR reorganizes the Award Table layout to combine Estimated and Final awards into a single unified view, updates the DTO structure to use structured objects instead of dynamic properties, adds the ability to retrieve adjustments for Estimated awards based on overawards and restrictions, and makes the ChipLabel component globally available.
Changes:
- Restructured award data from dynamic key-value pairs to structured DTOs with explicit properties for disbursement schedules and values
- Added new logic to calculate and display adjustments (restrictions, overawards, disbursed amounts) for both estimated and final awards
- Made ChipLabel component global and created new StatusChipDisbursement component
- Updated all e2e tests to match the new DTO structure and added test coverage for estimated award adjustments
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| sources/packages/web/src/types/contracts/institution/ConfirmationOfEnrollment.ts | Added ReadyToSend disbursement status enum value |
| sources/packages/web/src/types/contracts/Assessment.ts | Replaced dynamic award structure with structured types for adjustments and award data |
| sources/packages/web/src/services/http/dto/Assessment.dto.ts | Updated frontend DTOs to match new structured response format |
| sources/packages/web/src/main.ts | Registered ChipLabel component globally |
| sources/packages/web/src/composables/useDisbursement.ts | Added composable to map disbursement status to badge display details |
| sources/packages/web/src/components/generic/* | Updated chip components to support new statuses and made ChipLabel global |
| sources/packages/web/src/components/common/students/applicationDetails/* | Updated award display components to use new structured data |
| sources/packages/web/src/components/common/AwardTable.vue | Completely reorganized to combine estimated and final awards in a single table |
| sources/packages/web/src/components/common/AssessmentAwardDetails.vue | Simplified to delegate to AwardTable component |
| sources/packages/web/src/assets/css/base.scss | Added error chip background styling |
| sources/packages/backend/libs/integrations/src/esdc-integration/e-cert-integration/e-cert-integration.module.ts | Added ECertGenerationService to module providers |
| sources/packages/backend/apps/api/src/route-controllers/institution-user/institution-user.institutions.controller.ts | Commented out BCeID service code |
| sources/packages/backend/apps/api/src/route-controllers/assessment/models/assessment.dto.ts | Updated backend DTOs to use structured classes instead of dynamic types |
| sources/packages/backend/apps/api/src/route-controllers/assessment/assessment.controller.service.ts | Refactored to populate structured award data and calculate estimated adjustments |
| sources/packages/backend/apps/api/src/route-controllers/assessment/tests/e2e/* | Updated all e2e tests for new DTO structure and added tests for estimated adjustments |
.../packages/backend/apps/api/src/route-controllers/assessment/assessment.controller.service.ts
Outdated
Show resolved
Hide resolved
.../packages/backend/apps/api/src/route-controllers/assessment/assessment.controller.service.ts
Show resolved
Hide resolved
.../assessment/_tests_/e2e/assessment.students.controller.getAssessmentAwardDetails.e2e-spec.ts
Outdated
Show resolved
Hide resolved
.../apps/api/src/route-controllers/institution-user/institution-user.institutions.controller.ts
Outdated
Show resolved
Hide resolved
.../packages/backend/apps/api/src/route-controllers/assessment/assessment.controller.service.ts
Outdated
Show resolved
Hide resolved
.../packages/backend/apps/api/src/route-controllers/assessment/assessment.controller.service.ts
Outdated
Show resolved
Hide resolved
.../packages/backend/apps/api/src/route-controllers/assessment/assessment.controller.service.ts
Outdated
Show resolved
Hide resolved
.../apps/api/src/route-controllers/institution-user/institution-user.institutions.controller.ts
Outdated
Show resolved
Hide resolved
sources/packages/web/src/components/common/AssessmentAwardDetails.vue
Outdated
Show resolved
Hide resolved
| return "success-shade"; | ||
| case StatusChipTypes.Warning: | ||
| return "warning-shade"; | ||
| case StatusChipTypes.Error: |
There was a problem hiding this comment.
The updates to support StatusChipTypes.Error should be safe for existing code as Error isn't used.
| .component("ErrorSummary", ErrorSummary) | ||
| .component("ChipStatus", ChipStatus) | ||
| .component("ChipTag", ChipTag) | ||
| .component("ChipLabel", ChipLabel) |
There was a problem hiding this comment.
It made sense to make this a global component in line with ChipStatus and ChipTag.
| * Indicates if the money amount information was already | ||
| * sent to be paid to the student. | ||
| */ | ||
| export enum DisbursementScheduleStatus { |
There was a problem hiding this comment.
I don't like the location of this enum but didn't want to take on any additional changes with this PR.
There was a problem hiding this comment.
Feel free to add a "TODO" to move to a better location.
| index++; | ||
| const disbursement = new AwardDisbursementScheduleAPIOutDTO(); | ||
|
|
||
| disbursement.disbursementDate = getDateOnlyFullMonthFormat( |
There was a problem hiding this comment.
The preference for these values would be to have them returned from the API as Date (unless date-only), and the format be applied in the UI.
| let firstEligibleDisbursement: EligibleECertDisbursement; | ||
| let secondEligibleDisbursement: EligibleECertDisbursement; |
There was a problem hiding this comment.
Suggestion: Can be a single statement
let firstEligibleDisbursement: EligibleECertDisbursement,
secondEligibleDisbursement: EligibleECertDisbursement;
| if (disbursement.status === DisbursementScheduleStatus.Pending) { | ||
| // Estimated Award - calculate estimated adjustments. | ||
|
|
||
| // Restriction: If the student has a restriction that impacts funding, flag the adjustment.) |
There was a problem hiding this comment.
The parenthesis seems to be lost at the end of the sentence.
| ECertGenerationService, | ||
| EligibleECertDisbursement, | ||
| } from "@sims/integrations/services"; | ||
| import { getStopFundingTypesAndRestrictionsMap } from "@sims/integrations/services/disbursement-schedule/e-cert-processing-steps/e-cert-steps-utils"; |
There was a problem hiding this comment.
Please add this file to the index inside the e-cert-processing-steps or maybe into the services folder.
| ); | ||
| const finalAward = | ||
| await this.populateDisbursementFinalAwardsValues(assessment); | ||
| let secondDisbursement; |
There was a problem hiding this comment.
There is a minor warning stating this variable has an implicit type of any.
As a general rule, it would be better to have it typed during its declaration.
There was a problem hiding this comment.
Missed that one, thanks.
| institutionName: | ||
| assessment.offering.educationProgram.institution.operatingName, | ||
| offeringIntensity: assessment.offering.offeringIntensity, | ||
| offeringStudyStartDate: getDateOnlyFormat( |
There was a problem hiding this comment.
Outside PR changes, the getDateOnlyFormat is no longer required for studyStartDate and studyEndDate.
They were required way in the past, but not anymore.
| }); | ||
| } | ||
|
|
||
| const firstDisbursement = await this.populateAwardDisbursement( |
There was a problem hiding this comment.
The calls can be executed in parallel as below.
const [firstDisbursement, secondDisbursement] = await Promise.all([
this.populateAwardDisbursement(
firstDisbursementSchedule,
firstEligibleDisbursement,
assessment.application.student.id,
{ includeDocumentNumber, includeDateSent, maskMSFAA },
),
secondDisbursementSchedule
? this.populateAwardDisbursement(
secondDisbursementSchedule,
secondEligibleDisbursement,
assessment.application.student.id,
{ includeDocumentNumber, includeDateSent, maskMSFAA },
)
: null,
]);| const finalAward: DynamicAwardValue = {}; | ||
| disbursementValues.forEach((award) => { | ||
| if (award.valueType === DisbursementValueType.BCTotalGrant) { | ||
| if (eligibleDisbursement) { |
There was a problem hiding this comment.
Is there a scenario where the eligibleDisbursement would not be present? If yes, should the parameter be declared as
eligibleDisbursement: EligibleECertDisbursement | undefined,
If it is mandatory and present, it also has the studentId available.
There was a problem hiding this comment.
I believe this comment helps answer the question: it may not be present if the disbursement is no longer pending.
| DisbursementScheduleStatus.Pending | ||
| ) { | ||
| [firstEligibleDisbursement, secondEligibleDisbursement] = | ||
| await this.eCertGenerationService.getEligibleDisbursements({ |
There was a problem hiding this comment.
The getEligibleDisbursements has the following condition, which enforces that only eligible disbursements to create an e-Cert will be considered. When an application has two disbursements and the first one is sent, this method will start to return only one record that will represent the second disbursement, which will be wrongly associated with the firstEligibleDisbursement, also making the secondEligibleDisbursement null.
.where(
"disbursementSchedule.disbursementScheduleStatus = :disbursementScheduleStatus",
{ disbursementScheduleStatus: DisbursementScheduleStatus.Pending },
)If the above assumption is right (please validate it), considering that we no longer need the firstEligibleDisbursement once it is sent, we can stop using the array destructuring and execute the association using the disbursement ID. Does it make sense?
| finalAward?: DynamicAwardValue; | ||
| firstDisbursement: AwardDisbursementScheduleAPIOutDTO; | ||
| secondDisbursement: AwardDisbursementScheduleAPIOutDTO; | ||
| } |
There was a problem hiding this comment.
Please add a blank line, following the file pattern.
| }) | ||
| finalAward?: DynamicAwardValue; | ||
| firstDisbursement: AwardDisbursementScheduleAPIOutDTO; | ||
| secondDisbursement: AwardDisbursementScheduleAPIOutDTO; |
There was a problem hiding this comment.
secondDisbursement should be nullable, unless I am missing something.
| msfaaCancelledDate: string; | ||
| msfaaDateSigned: string; |
There was a problem hiding this comment.
These should be nullable, same for enrolmentDate.
| > | ||
| First disbursement | ||
| </h3> | ||
| <div> |
There was a problem hiding this comment.
Is there a need to use div + v-row? Would the below not work in the same way?
<template>
<content-group>
<award-table
...
/>
</content-group>
<content-group v-if="isSecondDisbursementAvailable">
<award-table
...
/>
</content-group>
</template>There was a problem hiding this comment.
Not required. This was older code I should have removed.
| <tbody> | ||
| <tr v-for="award in awards" :key="award.awardType"> | ||
| <td> | ||
| <span v-if="$vuetify.display.smAndDown"> |
There was a problem hiding this comment.
Can we use the below instead if that achieves the same?
const { mobile: isMobile } = useDisplay();| <span class="label-bold">Earliest date of disbursement: </span> | ||
| <span>{{ dateOnlyLongString(disbursement.disbursementDate) }}</span> | ||
| </div> | ||
| <div v-if="isDisbursementCompleted && disbursement.documentNumber"> |
There was a problem hiding this comment.
Minor, even if it was named as such before, it may be misleading. I would recommend naming it something more specific to the COE completion, for instance, isCOECompleted.
Either way, the disbursement.documentNumber would also be enough as a condition to display it.
| /> | ||
| </div> | ||
|
|
||
| <div></div> |
There was a problem hiding this comment.
Is this div a leftover?
| const { currencyFormatter, dateOnlyLongString } = useFormatters(); | ||
|
|
||
| // Associate disbursement values with their award types. | ||
| const awards: AssessmentAwardData[] = awardTypes.value.map((award) => { |
There was a problem hiding this comment.
The awards variable is based on a computed property awardTypes but it is not defined to be reactive/computed. Is there any reason not have the awards as a computed variable?
| hasDisbursedAdjustment: | ||
| disbursementValue?.hasDisbursedAdjustment ?? false, | ||
| hasRestrictionAdjustment: | ||
| disbursementValue?.hasRestrictionAdjustment ?? false, | ||
| hasNegativeOverawardAdjustment: | ||
| disbursementValue?.hasNegativeOverawardAdjustment ?? false, | ||
| hasPositiveOverawardAdjustment: | ||
| disbursementValue?.hasPositiveOverawardAdjustment ?? false, |
| disbursementReceipt2bgpd: 800, | ||
| disbursementReceipt2sbsd: 900, | ||
| }, | ||
| .then((response) => { |
There was a problem hiding this comment.
Is there a problem keeping the previous syntax as below (and removing the expect.arrayContaining)?
.expect(HttpStatus.OK)
.expect({
applicationNumber: application.applicationNumber,There was a problem hiding this comment.
I noticed many tests changed in this way. Was there a benefit in changing it?
There was a problem hiding this comment.
Tests were behaving unpredictably due to the changing order of nested objects (disbursement values) in the array. This was the only way to test the values content irrespective of order. I could add a sort to the API if you prefer but it isn't needed by web.
There was a problem hiding this comment.
It is not a matter of preference; from a project perspective, the idea is to test as much as possible, as accurately as possible. If I am not wrong, the arrayContaining would allow more items to be returned in the array, and the test would still pass.
We should not change PROD code in favor of E2E tests, but adding the sort for the awards and ensuring the query will always be returned in the same way would not be so bad in this scenario.
There was a problem hiding this comment.
Can we have one scenario for when an application has two disbursements, where the first disbursement is sent and the second one is pending?
andrewsignori-aot
left a comment
There was a problem hiding this comment.
Great work, and it is amazing to see how much the summary page has changed. Please take a look at the comments.
| title="Funding summary" | ||
| sub-title="Below is the summary from your assessment. To view your Notice of Assessment, click on view assessment." | ||
| <toggle-content | ||
| :toggled="!assessmentAwardData.firstDisbursement" |
There was a problem hiding this comment.
When will the firstDisbursement be evaluated to false here? New instance of a class intiatialized with the new keyword will evaluate to true always even if the object is empty. Since, it is populated using teh below in the backend, I believe it will never evaluate to false.
const disbursement = new AwardDisbursementScheduleAPIOutDTO();
sh16011993
left a comment
There was a problem hiding this comment.
Great work @weskubo-cgi 👍

Frontend
Backend
Screenshots
Pending (Ministry)
Completed (Ministry)
Legend
E2E Tests