-
Notifications
You must be signed in to change notification settings - Fork 13
#5525 - Update Disbursement Report to include Forecast Date #5579
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
Conversation
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.
Pull request overview
This PR updates the Disbursement Report to include a new "Forecast Date" column, which displays the scheduled disbursement date alongside the actual disbursement date. The change is implemented through a database migration that updates the SQL query stored in the report configuration table.
- Adds "Forecast Date" column (from
ds.disbursement_date) as the first column in all three UNION queries - Provides both forward migration (to add the column) and rollback migration (to remove it)
- Maintains existing report ordering and filtering logic
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
1767653404966-UpdateDisbursementReportForecastDate.ts |
TypeScript migration class that executes the SQL update and rollback scripts |
Update-disbursements-report-forecast-date.sql |
SQL script that adds the "Forecast Date" column to all three UNION queries in the disbursement report |
Rollback-update-disbursements-report-forecast-date.sql |
SQL script that removes the "Forecast Date" column to revert to the previous report version |
...ges/backend/apps/db-migrations/src/sql/Reports/Update-disbursements-report-forecast-date.sql
Outdated
Show resolved
Hide resolved
...kend/apps/db-migrations/src/migrations/1767653404966-UpdateDisbursementReportForecastDate.ts
Outdated
Show resolved
Hide resolved
...kend/apps/db-migrations/src/migrations/1767653404966-UpdateDisbursementReportForecastDate.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.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
...kend/apps/db-migrations/src/migrations/1767653404966-UpdateDisbursementReportForecastDate.ts
Outdated
Show resolved
Hide resolved
...kend/apps/db-migrations/src/migrations/1767653404966-UpdateDisbursementReportForecastDate.ts
Outdated
Show resolved
Hide resolved
| export class UpdateDisbursementReportForecastDate1767653404966 implements MigrationInterface { | ||
| /** | ||
| * Update the disbursement reports to include the "Forecast Date" column. | ||
| * @param queryRunner the query runner. |
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 additional space between the param and comment.
| } | ||
| } | ||
| } | ||
| } |
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.
Can we not include this file if this is not necessary?
|
PR looks good. Please add a E2E test. |
| const student = application.student; | ||
| const sinValidation = createFakeSINValidation({ | ||
| student, | ||
| }); | ||
| student.sinValidation = sinValidation; | ||
| await db.student.save(student); | ||
| await db.sinValidation.save(sinValidation); |
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.
saveFakeStudent can replace all the work done here. Please use a student created from saveFakeStudent in saveFakeApplicationDisbursements line 1387 and SIN validation will be automatically taken care.
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 making the changes. Looks good.
weskubo-cgi
left a comment
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 look great. Tested report with migration and rollback. Just a few very minor comments.
...kend/apps/db-migrations/src/migrations/1767653404966-UpdateDisbursementReportForecastDate.ts
Show resolved
Hide resolved
...api/src/route-controllers/report/_tests_/e2e/report.aest.controller.exportReport.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...api/src/route-controllers/report/_tests_/e2e/report.aest.controller.exportReport.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...api/src/route-controllers/report/_tests_/e2e/report.aest.controller.exportReport.e2e-spec.ts
Outdated
Show resolved
Hide resolved
| @@ -73,6 +73,7 @@ SET | |||
| ) | |||
| ORDER BY | |||
| "Date of Disbursement", | |||
| "Certificate Number" $$ | |||
| "Certificate Number", | |||
| "Funding Code" $$ | |||
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.
question: Is this a new requirement? I don't see an AC for 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.
This is not part of the AC
The current order by will cause the disbursement values to be returned in random order making it unpredictable in the tests.
The change however should not affect the reporting.
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.
@tiago-graf I believe the strict equal/equal(one among the 2) does not enforce the order. If that is added for tests I would ask to remove it as we can handle the assertion.
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 think we can also shift the disbursementDate by a day on federal or provincial to get a consistent date sort.
|


Changes Includes a new migration to update SQL of the report "Disbursement_Report" in the database. The new SQL includes a "Forecast Date" Column.
Rollback test: