-
Notifications
You must be signed in to change notification settings - Fork 13
#4378 - Ecert Cancellation - UI & Ministry Access (2/2) #5108
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
#4378 - Ecert Cancellation - UI & Ministry Access (2/2) #5108
Conversation
.../assessment/_tests_/e2e/assessment.students.controller.getAssessmentAwardDetails.e2e-spec.ts
Show resolved
Hide resolved
| /** | ||
| * Indicates which disbursement schedule statuses have | ||
| * the potential to generate a receipt. | ||
| */ |
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 Rejected considered as a potential status to generate e-cert? If no, can we update the description and also the constant name?
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.
Rejected is considered a status where receipts could exist, which means that Sent or Reject disbursements may have receipts associated with a disbursement.
Is the use of the word "generate" misleading? I can rephrase as
Indicates which disbursement schedule statuses are expected to have some receipts generated.
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.
Comment improved.
.../packages/backend/apps/api/src/route-controllers/assessment/assessment.controller.service.ts
Show resolved
Hide resolved
.../packages/backend/apps/api/src/route-controllers/assessment/assessment.controller.service.ts
Show resolved
Hide resolved
| console.log( | ||
| "Cancelling disbursement schedule:", | ||
| disbursementScheduleId, | ||
| payload, | ||
| ); | ||
| } |
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 console log.
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.
Part of the temporary code to be replaced with the real implementation. I did not see any harm in keeping 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.
Remove to prevent noise in the PR.
| export * from "./disbursement-schedule/disbursement-schedule.aest.controller"; | ||
| export * from "./disbursement-schedule/models/disbursement-schedule.dto"; |
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.
Will it be better if we import the DTO before the controller so that when we use imports from index we could potentially avoid conflicts.
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 did not understand this at first, but is there any technical reason to enforce the order?
bidyashish
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.
Looks good,
Please look at comments.
| // Arrange | ||
| const [enrolmentDate1, enrolmentDate2] = [addDays(1), addDays(30)]; | ||
| const [statusUpdatedOn1, statusUpdatedOn2] = [addDays(2), addDays(31)]; | ||
| // Student has an application to the institution eligible for NOA. |
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.
[addDays(2), addDays(31)]; whats the need to make it to 31.
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 idea was to ensure the dates were different to ensure the expected ones would be returned.
sources/packages/web/src/components/common/AssessmentAwardDetails.vue
Outdated
Show resolved
Hide resolved
sources/packages/web/src/components/common/AssessmentAwardDetails.vue
Outdated
Show resolved
Hide resolved
sources/packages/web/src/services/DisbursementScheduleService.ts
Outdated
Show resolved
Hide resolved
| import { Ref, ref } from "vue"; | ||
|
|
||
| export function useModalDialog<T, TParameter = any>() { | ||
| export function useModalDialog<T, TParameter = unknown>() { |
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.
👍
| :allow-disbursement-cancellation="allowDisbursementCancellation" | ||
| :allow-final-award-extended-information="allowFinalAwardExtendedInformation" | ||
| @confirmEnrolment="$emit('confirmEnrolment', $event)" | ||
| @confirm-enrolment="$emit('confirmEnrolment', $event)" |
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.
👍
sources/packages/web/src/components/common/modals/UserNoteConfirmModal.vue
Outdated
Show resolved
Hide resolved
|
dheepak-aot
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.
Thanks for making the changes and bringing improvements to existing framework. Looks good 👍


student-cancel-disbursement-schedule.disbursementReceipt[n]Received: indicates if a receipt was received. Note: full-time final awards are retrieved when a receipt is received, and part-time are displayed upon e-Cert creation. This new variable indicates when a receipt was received, not when a final award was returned.disbursementReceipt[n]HasAwards: added to indicate when an award is present in the payload. Previously, only awards were present in this payload, and they were dynamically created. Adding the boolean makes it easier to identify when awards are present, avoiding inspecting the object structure as was done before. The examples for the payloads with the new variables were updated and can be seen across many E2E tests.StatusInfoDisbursementCancellationto display the information when a disbursement is cancelled, following a similar idea from theStatusInfoEnrolment.CancelDisbursementSchedulefollowing the same idea fromConfirmEnrolment.user-note-confirm-modalto allow modal confirmation, including a user note. Its usage can be checked in theCancelDisbursementSchedule.vue.Please note
1. Changes executed in the methods retrieving the data for the summary page were executed, trying to keep the current dynamic behavior. Additional changes can be made to make a hybrid object model between dynamic and non-dynamic properties, but it was not considered worthwhile as part of this effort.
2. Some UI changes are pending decision, but no major adaptations are expected.
Ministry View
Full-time, Final award, e-Cert
Sent, no receiptsFull-time, Final award, e-Cert
Rejected, no receiptsFull-time, Final award, e-Cert
Pending, no receiptsFull-time, Final award, e-Cert
Pending, with receiptsCancel eCert modal
Buttons and icons adjustments