-
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
#1759 - Reissue MSFAA - Part 2 - NOA UI Changes #1936
Conversation
Nice work! @andrewsignori-aot Just missing a few things. No problem if you do it in the next PR! |
:assessmentId="assessmentId" | ||
:can-reissue-m-s-f-a-a="true" | ||
@reissue-m-s-f-a-a="reissueMSFAA" |
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.
Not a problem, just curious: should we use kebab case for the attributes too? If that's case assessmentId
should be assessment-id
😄
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.
Good question. I did not do that manually 😄, that was the VS Code suggestion while auto-completing.
We have been doing it for the component's name (e.g. notice-of-assessment-form-view
) because it looks like more HTML style (IMO). For instance when we declare data
custom attributes we will use data-*
.
Should we change assessmentId
to assessment-id
? Yes, but we do not need to go back and do it all over the place. Does it make sense?
@ann-aot @guru-aot @dheepak-aot @sh16011993
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.
yes 😄 we can do it and make sense
sources/packages/web/src/components/common/modals/ConfirmModal.vue
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.
Great job. I left some comments.
@@ -19,15 +19,9 @@ | |||
data-cy="primaryFooterButton" | |||
color="primary" | |||
@click="$emit('primaryClick')" | |||
:loading="processing" |
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/NoticeOfAssessmentFormView.vue
Outdated
Show resolved
Hide resolved
reissueMSFAA: ( | ||
applicationId: number, | ||
loadNOA: () => Promise<void>, | ||
processing: (processing: boolean) => void, |
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 approach is also good
UI adjusted as below @hellolynn-tbtb |
sources/packages/backend/libs/services/src/msfaa-number/msfaa-number-shared.service.ts
Outdated
Show resolved
Hide resolved
...assessment, | ||
canReissueMSFAA, | ||
}; | ||
console.log(assessment); |
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 we need a console.log here?
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.
It was a leftover from before. It is removed already.
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 @andrewsignori-aot
sources/packages/forms/src/form-definitions/noticeofassessment.json
Outdated
Show resolved
Hide resolved
schedule.coeStatus; | ||
disbursementDetails[`${disbursementIdentifier}MSFAANumber`] = | ||
schedule.msfaaNumber.msfaaNumber; | ||
disbursementDetails[`${disbursementIdentifier}MSFAACancelledDate`] = | ||
schedule.msfaaNumber.cancelledDate; | ||
disbursementDetails[`${disbursementIdentifier}MSFAADateSigned`] = |
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.
The signed date received from the API is used by the method getMSFAAStatus
.
/** | ||
* Defines the MSFAA status based on the signed date and cancelled dates. | ||
* @param signedDate signed date. | ||
* @param cancelledDate cancelled date. |
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.
return 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.
Added.
const getMSFAAStatus = ( | ||
signedDate?: Date, | ||
cancelledDate?: Date, | ||
): MSFAAStatus => { |
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.
return 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.
Added.
* Defines the icon class to displayed depending on the MSFAA status. | ||
* @param msfaaStatus MSFAA status. | ||
*/ | ||
const getMSFAAStatusClass = (msfaaStatus: MSFAAStatus): string => { |
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.
return 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.
Added.
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 👍 added some minor comments
sources/packages/backend/libs/services/src/msfaa-number/msfaa-number-shared.service.ts
Show resolved
Hide resolved
sources/packages/web/src/components/common/NoticeOfAssessmentFormView.vue
Show resolved
Hide resolved
await ApplicationService.shared.reissueMSFAA(applicationId); | ||
snackBar.success("MSFAA was reissued successfully."); | ||
await reloadNOA(); | ||
} catch (error: 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.
do we need error in the catch?
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.
Nope, removing 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.
Removed.
Good to see the MSFAA and Re-Issue MSFAA happening. Added few comments. |
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 making the changes. 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 👍 Good work @andrewsignori-aot
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 the answers.
MSFAACancelledDate
andMSFAADateSigned
to the API return following the approach in place. Renamed theCOE status
previously namedStatus
toCOEStatus
since the object itself is related to the disbursement and now we have a status for it.canReissueMSFAA
logic to display the button uses theprops.canReissueMSFAA
and it loses the reactivity in the way that it is right now. I did not find it too important to keep the reactivity for this case.Two disbursements, same MSFAA, MSFAA signed.
Two disbursements, same MSFAA, MSFAA not signed.
Two disbursements (at least one is pending), same MSFAA, MSFAA canceled.
Two disbursements (first is sent, second is pending), different MSFAA, second MSFAA waiting to be signed.
Two disbursements (first is sent, second is pending), different MSFAA, both are canceled on different dates.
Student view
Reissue MSFAA confirmation modal.