-
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
#1978 - Request Offering Change - Student View Request (Bug fix) #2282
#1978 - Request Offering Change - Student View Request (Bug fix) #2282
Conversation
The following two scenarios are fixed: 1) Approve application offering change request through the application offering change request dialog. 2) Text under the Application Details and Request Details headings is only displayed when "In progress with student." They are omitted after the student approves or declines.
const cancel = () => { | ||
approveApplicationOfferingChangeRequest.value.reset(); | ||
formModel.studentConsent = false; | ||
approveApplicationOfferingChangeRequestModal.value.studentConsent = false; |
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 the reset above not taking care of 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.
In my understanding, the reset is not working on the checkbox component. I tried by removing the line 74 but in that case the reset fails.
const cancel = () => { | ||
approveApplicationOfferingChangeRequest.value.reset(); | ||
formModel.studentConsent = false; | ||
approveApplicationOfferingChangeRequestModal.value.studentConsent = false; |
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 you double-check if the line 73 reset is working and if we need this reset?
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.
Correct. The reset was redundant. Removed it 👍
@@ -80,17 +80,17 @@ export default defineComponent({ | |||
Location: changeRequest.value.locationName, | |||
} as Record<string, string>), | |||
); | |||
const changeRequestNotApproved = computed(() => { | |||
const changeRequestInProgressWithStudent = computed(() => { |
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.
Just for my clarification because I was not part of previous reviews, I can see on Figma that these texts are not displayed for "Approved" status. Why are we showing them only for InProgressWithStudent
? Are they not supposed to be hidden only for Approved
ones?
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.
In the story AC, the requirement is that the "Text under the Application Details and Request Details headings only need to be displayed when "In progress with student." They can be omitted after student approves or declines."
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.
Nice catch, minor comments only.
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, please have a look on the model reset mentioned by other devs comments
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.
just minor comment
@@ -86,7 +82,7 @@ export default defineComponent({ | |||
} | |||
approveApplicationOfferingChangeRequestModal.value.applicationOfferingChangeRequestStatus = | |||
ApplicationOfferingChangeRequestStatus.InProgressWithSABC; | |||
const payload = { ...formModel }; | |||
const payload = { ...approveApplicationOfferingChangeRequestModal.value }; |
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.
👍
const cancel = () => { | ||
approveApplicationOfferingChangeRequest.value.reset(); | ||
formModel.studentConsent = false; | ||
approveApplicationOfferingChangeRequestModal.value.studentConsent = false; |
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 have a point, to mention, but not a blocker. do we need a model for just one property coming from form ? As the code is part of previous PR not stressing 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.
I used a model to keep it in sync with how we are doing it in other places.
Just a minor 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.
Nice work, looks good 👍
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.
Nice catch. Approving as my comment is not a blocker.
@@ -69,12 +69,8 @@ export default defineComponent({ | |||
{} as StudentApplicationOfferingChangeRequestAPIInDTO, |
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.
In other modals, we use reactive
for form data objects
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
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.
Just one minor 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!
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 doing the changes and nice catch 👍
The following two scenarios are fixed:
Problem: Application Offering Change Request Status was not sent in payload.
Fix: Application Offering Change Request Status is being set to
InProgressWithSABC
in the payload.Problem: Text under the Application Details and Request Details headings was only omitted when the application offering change request was approved by the ministry.
Fix As per the requirement, text under the Application Details and Request Details headings is only displayed when "In progress with student." They are omitted after the student approves or declines.