Skip to content
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.

Sign up for GitHub

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

Jump to bottom

#1981 - Request Offering Change: Ministry View/Approve/Deny (UI only) - Part 1 #2363

Merged
merged 10 commits into from
Oct 5, 2023
Merged

#1981 - Request Offering Change: Ministry View/Approve/Deny (UI only) - Part 1 #2363

merged 10 commits into from
Oct 5, 2023

Conversation

sh16011993
Copy link
Collaborator

@sh16011993 sh16011993 commented Sep 28, 2023 •
edited
Loading

As a part of this PR, the following tasks were completed:

  • Created the models for Approve and Decline the application offering change request by the ministry.

Reason for modifying the base component (ModalDialogBase.vue): <hr> tag was changed to <v-divider> component. This v-divider component by default adds a significant amount of vertical whitespace. As a result, the margin for v-divider in this component is changed to reduce the whitespace.

Please note: Placeholder added for the API endpoint.

Screenshots:

Decline Application Offering Change Request Modal

image

Approve Application Offering Change Request Modal

image

@sh16011993 sh16011993 self-assigned this Sep 28, 2023
@sh16011993 sh16011993 changed the title #1981 - Request Offering Change: Ministry View/Approve/Deny (UI only) #1981 - Request Offering Change: Ministry View/Approve/Deny (UI only) - Part 1 Sep 28, 2023
ann-aot
ann-aot reviewed Sep 28, 2023
View reviewed changes
sources/packages/web/src/components/aest/AssessApplicationOfferingChangeRequestModal.vue Outdated
<modal-dialog-base
:showDialog="showDialog"
:title="getTitle"
:max-width="780"
Copy link
Contributor

@ann-aot ann-aot Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to check, do we really need this width?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default max-width in the modal-dialog-base component is a bit less resulting in the wrapping of the line "Outline the reasoning for approving this request. Please add the application number." in the dialog. In the figma, the text is a single line. That is the reason I increased the max-width.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to keep all the modals with the same width unless we have any requirement. Otherwise the visual will be affected.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sh16011993 I noticed the thumbs up from other DEVs here but I did not understand if the intention is to have max-width="780" removed or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any issue with the default width.
image
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it.

andrepestana-aot
andrepestana-aot reviewed Sep 28, 2023
View reviewed changes
Copy link
Collaborator

@andrepestana-aot andrepestana-aot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job. There are a few issues already pointed.

andrewsignori-aot
andrewsignori-aot reviewed Sep 28, 2023
View reviewed changes
sources/packages/web/src/components/aest/AssessApplicationOfferingChangeRequestModal.vue

export default defineComponent({
components: {
ModalDialogBase,
Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that we talked in the past about having a common "Approval" modal dialog (similar to the ConfimrModal) that would allow adding a note and a decline/approve buttons but we never went for that. This situation would be a perfect example of using a generic approval modal.

ann-aot
ann-aot reviewed Sep 29, 2023
View reviewed changes
Copy link
Contributor

@ann-aot ann-aot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at the comments.

guru-aot
guru-aot reviewed Sep 29, 2023
View reviewed changes
...s/application-offering-change-request/application-offering-change-request.aest.controller.ts
* @param payload information to update the application offering change request.
*/
@Patch(":applicationOfferingChangeRequestId")
async updateApplicationOfferingChangeRequest(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the the proper role for the method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be a part of the next PR. I have updated the TODO comment with the same.

andrewsignori-aot
andrewsignori-aot approved these changes Oct 4, 2023
View reviewed changes
Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot left a 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, just left one minor comment about one open conversation.

ann-aot
ann-aot approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@ann-aot ann-aot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Just a minor comment . Thanks for doing the changes 👍 #2363 (comment)

andrepestana-aot
andrepestana-aot approved these changes Oct 5, 2023
View reviewed changes
Copy link
Collaborator

@andrepestana-aot andrepestana-aot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for the changes.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

dheepak-aot
dheepak-aot approved these changes Oct 5, 2023
View reviewed changes
Copy link
Collaborator

@dheepak-aot dheepak-aot left a 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. 👍

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 17.43% ( 2206 / 12659 )
Methods: 8.09% ( 129 / 1594 )
Lines: 20.17% ( 1933 / 9584 )
Branches: 9.72% ( 144 / 1481 )

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 46.73% ( 300 / 642 )
Methods: 40% ( 32 / 80 )
Lines: 51.02% ( 251 / 492 )
Branches: 24.29% ( 17 / 70 )

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 74.93% ( 514 / 686 )
Methods: 67.47% ( 56 / 83 )
Lines: 76.96% ( 451 / 586 )
Branches: 41.18% ( 7 / 17 )

@sh16011993 sh16011993 added the SIMS-Api SIMS-Api label Oct 5, 2023
@github-actions
Copy link

github-actions bot commented Oct 5, 2023

E2E SIMS API Coverage Report

Totals Coverage
Statements: 54.9% ( 4026 / 7333 )
Methods: 51.84% ( 493 / 951 )
Lines: 59.75% ( 3270 / 5473 )
Branches: 28.93% ( 263 / 909 )

@sh16011993 sh16011993 merged commit 3efd4c2 into main Oct 5, 2023
@sh16011993 sh16011993 deleted the 1981_request_offering_change_ministry_view_approve_deny_request branch October 5, 2023 00:41
@sh16011993 sh16011993 temporarily deployed to DEV October 5, 2023 00:52 — with GitHub Actions Inactive
@sh16011993 sh16011993 temporarily deployed to DEV October 5, 2023 00:54 — with GitHub Actions Inactive
@sh16011993 sh16011993 temporarily deployed to DEV October 5, 2023 00:54 — with GitHub Actions Inactive
@sh16011993 sh16011993 temporarily deployed to DEV October 5, 2023 00:54 — with GitHub Actions Inactive
@sh16011993 sh16011993 temporarily deployed to DEV October 5, 2023 00:54 — with GitHub Actions Inactive
@sh16011993 sh16011993 temporarily deployed to DEV October 5, 2023 00:57 — with GitHub Actions Inactive
@sh16011993 sh16011993 temporarily deployed to DEV October 5, 2023 00:57 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guru-aot guru-aot guru-aot left review comments

@dheepak-aot dheepak-aot dheepak-aot approved these changes

@andrewsignori-aot andrewsignori-aot andrewsignori-aot approved these changes

@ann-aot ann-aot ann-aot approved these changes

@andrepestana-aot andrepestana-aot andrepestana-aot approved these changes

@JasonCTang JasonCTang Awaiting requested review from JasonCTang

Assignees

@sh16011993 sh16011993

Labels
SIMS-Api SIMS-Api
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sh16011993 @dheepak-aot @andrewsignori-aot @guru-aot @ann-aot @andrepestana-aot