-
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
#3453 - COE for $0/Not Eligible Assessments - Part 1 #4010
#3453 - COE for $0/Not Eligible Assessments - Part 1 #4010
Conversation
...egration/ecert-integration/_tests_/ecert-full-time-process-integration.scheduler.e2e-spec.ts
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.
Everything looks good. Just one question regarding the changes in the e2e test file.
...es/backend/apps/db-migrations/src/sql/DisbursementSchedules/Add-has-estimated-awards-col.sql
Outdated
Show resolved
Hide resolved
...es/backend/apps/db-migrations/src/sql/DisbursementSchedules/Add-has-estimated-awards-col.sql
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/workers/src/controllers/assessment/assessment.controller.ts
Outdated
Show resolved
Hide resolved
...ges/backend/apps/workers/src/services/disbursement-schedule/disbursement-schedule.service.ts
Outdated
Show resolved
Hide resolved
...ackages/backend/apps/api/src/services/disbursement-schedule/disbursement-schedule-service.ts
Show resolved
Hide resolved
...-of-enrollment/_tests_/e2e/confirmation-of-enrollment.institutions.getCOESummary.e2e-spec.ts
Show resolved
Hide resolved
Nice start @lewischen-aot . Please have a look into the comments. One question. Is ECE integration going to part of next PR? |
...es/backend/apps/db-migrations/src/sql/DisbursementSchedules/Add-has-estimated-awards-col.sql
Outdated
Show resolved
Hide resolved
...es/backend/apps/db-migrations/src/sql/DisbursementSchedules/Add-has-estimated-awards-col.sql
Outdated
Show resolved
Hide resolved
...es/backend/apps/db-migrations/src/sql/DisbursementSchedules/Add-has-estimated-awards-col.sql
Outdated
Show resolved
Hide resolved
INNER JOIN sims.disbursement_values disbursement_values ON disbursement_values.disbursement_schedule_id = disbursement_schedules.id | ||
GROUP BY | ||
disbursement_schedules.id | ||
) disbursement_total_amounts |
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 a suggestion, I would recommend the following UPDATE SQL considering less complex to read and understand IMO.
But it is not a blocker. Existing update still does the work.
update
sims.disbursement_schedules
set
has_estimated_awards = (case
when exists (
select
1
from
sims.disbursement_values disbursement_value
where
disbursement_value.disbursement_schedule_id = sims.disbursement_schedules.id
and disbursement_value.value_amount > 0) then true
else false
end);
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.
...ages/backend/libs/services/src/disbursement-schedule/disbursement-schedule-shared.service.ts
Show resolved
Hide resolved
...llers/disbursement/_tests_/e2e/disbursement.controller.saveDisbursementSchedules.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...llers/disbursement/_tests_/e2e/disbursement.controller.saveDisbursementSchedules.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...llers/disbursement/_tests_/e2e/disbursement.controller.saveDisbursementSchedules.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...llers/disbursement/_tests_/e2e/disbursement.controller.saveDisbursementSchedules.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...llers/disbursement/_tests_/e2e/disbursement.controller.saveDisbursementSchedules.e2e-spec.ts
Show resolved
Hide resolved
...llers/disbursement/_tests_/e2e/disbursement.controller.saveDisbursementSchedules.e2e-spec.ts
Show resolved
Hide resolved
await db.notificationMessage.update( | ||
{ | ||
id: NotificationMessageType.MinistryNotificationDisbursementBlocked, | ||
}, | ||
{ emailContacts: ["[email protected]"] }, | ||
); |
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.
Asking for my understanding. Was this required?
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.
Shashank asked the same question, and I replied in his comment: here.
sources/packages/backend/libs/sims-db/src/entities/disbursement-schedule.model.ts
Outdated
Show resolved
Hide resolved
Thanks for making the changes and writing extensive E2E tests. Added more comments. Please have a look. |
}, | ||
], | ||
}); | ||
const saveResult = await disbursementController.saveDisbursementSchedules( |
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.
fakeOriginalAssessment, | ||
); | ||
|
||
// Act |
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 act comment must be right before line 471.
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 and also taking effort to split the PRs.
Approved with 2 minor comments.
Please feel free to merge once the 2 minor comments are addressed.
|
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 @lewischen-aot 👍
The first part of the PR resolves the following business AC:
The PR includes the changes for two modules: API endpoint and workers.
has_estimated_awards
in the disbursement table to indicate the $0 disbursement during the saving assessments in the workers.updateDisbursementsHasEstimatedAwards
to updatehas_estimated_awards
for each disbursement in the methodsaveAssessmentData
.getCOESummary
Rollback Evidence
SQL query to run on Production as required from comment
The next part will cover the changes for another two modules: COE Request report and e-Cert queue consumer.