#4616 - Ministry View Change Request Revamp (Application view menu) #4628
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Ministry view for handling change requests and supporting users in both the UI and API. Key changes include updating DTOs to include supporting user data, refactoring the Ministry applications menu with dynamic route-based navigation, and updating the API query to return the active application along with its versions.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sources/packages/web/src/types/contracts/Common.ts | Added additional properties (subtitle, slim, color, opacity) to the MenuItemModel to support richer UI. |
| sources/packages/web/src/services/http/dto/Application.dto.ts | Added new DTO classes/interfaces to include supporting users within application versions and overall details. |
| sources/packages/web/src/composables/useApplication.ts | Introduced a helper to map application edit statuses for the Ministry view. |
| sources/packages/web/src/components/layouts/aest/AESTApplicationSideBar.vue | Refactored the sidebar to build menu items dynamically, integrating routes and supporting users. |
| sources/packages/backend/apps/api/src/services/application/application.service.ts | Modified the query to join supporting users and return a single application record with versions. |
| sources/packages/backend/apps/api/src/route-controllers/application/models/application.dto.ts | Updated the application DTOs to include supporting users and overall details for active and change-request versions. |
| sources/packages/backend/apps/api/src/route-controllers/application/application.controller.service.ts | Adjusted the controller service to transform application data with supporting users and handle change request separation. |
| sources/packages/backend/apps/api/src/route-controllers/application/tests/application.aest.controller.getApplicationOverallDetails.e2e-spec.ts | Added comprehensive E2E tests to cover scenarios for original applications, in-progress change requests, and declined change requests with supporting users. |
Comments suppressed due to low confidence (1)
sources/packages/backend/apps/api/src/route-controllers/application/models/application.dto.ts:267
- Duplicate definitions of ApplicationSupportingUsersAPIOutDTO are present (both as an interface and as a class). Consider consolidating these into a single consistent type to avoid confusion.
export interface ApplicationSupportingUsersAPIOutDTO {
sources/packages/backend/apps/api/src/services/application/application.service.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
dheepak-aot
reviewed
Apr 25, 2025
| * @param applicationId application id. | ||
| * @return list of supporting users of an application. | ||
| */ | ||
| @Get("application/:applicationId") |
dheepak-aot
reviewed
Apr 25, 2025
Comment on lines
+839
to
+853
| let previousVersions = application.parentApplication.versions.map( | ||
| (version) => this.transformToApplicationOverallDetailsAPIOutDTO(version), | ||
| ); | ||
| // Check if there is an in-progress change request for the application. | ||
| const inProgressChangeRequest = previousVersions.find((version) => | ||
| APPLICATION_EDIT_STATUS_IN_PROGRESS_VALUES.includes( | ||
| version.applicationEditStatus, | ||
| ), | ||
| ); | ||
| // Remove the in-progress change request from the previous versions list. | ||
| // A maximum of one in-progress change request is allowed. | ||
| if (inProgressChangeRequest) { | ||
| previousVersions = previousVersions.filter( | ||
| (version) => version.id !== inProgressChangeRequest.id, | ||
| ); |
There was a problem hiding this comment.
We can do an early return if the previous versions are not present.
There was a problem hiding this comment.
The goal of the early return is to reduce nesting, complexity, and make the code easy to read.
The nesting would not be reduced in this situation. I added the early return to see how it looks and it does not seems like an improvement in this scenario. Please let me know if I am missing something at a level that it would be a blocker.
dheepak-aot
reviewed
Apr 25, 2025
sources/packages/backend/apps/api/src/services/application/application.service.ts
Outdated
Show resolved
Hide resolved
… of https://github.com/bcgov/SIMS into feature/#4616-change-request-ministry-application-view
dheepak-aot
reviewed
Apr 25, 2025
sources/packages/backend/apps/api/src/route-controllers/application/models/application.dto.ts
Outdated
Show resolved
Hide resolved
|
Great start @andrewsignori-aot only minor comments. |
There was a problem hiding this comment.
Pull Request Overview
This PR revamps the ministry view change request by integrating supporting users into the application overall details API and updating the UI menus accordingly. The key changes include:
- Adding supporting users to the application version and overall details DTOs.
- Refactoring the ministry applications sidebar to integrate with current routing and active state behavior.
- Removing obsolete API methods and updating the relevant tests to cover new scenarios.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| sources/packages/web/src/types/contracts/Common.ts | Added new optional menu item properties (subtitle, class, slim, color, opacity) to support enhanced UI. |
| sources/packages/web/src/services/http/dto/SupportingUser.dto.ts | Removed the redundant ApplicationSupportingUsersAPIOutDTO in favor of other supporting user DTOs. |
| sources/packages/web/src/services/http/dto/Application.dto.ts | Extended and added new DTOs to include supportingUsers in application version and overall details. |
| sources/packages/web/src/services/http/SupportingUserApi.ts | Removed the deprecated getSupportingUsersForSideBar method. |
| sources/packages/web/src/services/SupportingUserService.ts | Removed the redundant getSupportingUsersForSideBar method for consistency with API changes. |
| sources/packages/web/src/composables/useApplication.ts | Introduced mapApplicationEditStatusForMinistry to provide a Ministry-specific edit status mapping. |
| sources/packages/web/src/components/layouts/aest/AESTApplicationSideBar.vue | Refactored the sidebar layout to tie menu items to routes and improved the handling of change request and version menus. |
| sources/packages/backend/apps/api/src/services/application/application.service.ts | Updated the application versions query to join supporting users and return the active application with its versions in one call. |
| sources/packages/backend/apps/api/src/route-controllers/* | Adjusted controllers and models to support the new structure of application details and remove obsolete DTOs. |
| sources/packages/backend/apps/api/src/route-controllers/application/tests/* | Updated E2E tests to validate the new application overall details structure including supporting users. |
Comments suppressed due to low confidence (3)
sources/packages/web/src/components/layouts/aest/AESTApplicationSideBar.vue:70
- [nitpick] For the 'Partner' title, if multiple partners exist, consider including an index or unique identifier to differentiate each entry.
const createSupportingUsersMenu = (
sources/packages/backend/apps/api/src/route-controllers/application/application.controller.service.ts:875
- Returning undefined when there are no supporting users might lead to inconsistent API responses; consider returning an empty array instead.
supportingUsers: application.supportingUsers.length ? application.supportingUsers.map((user) => ({
sources/packages/backend/apps/api/src/services/application/application.service.ts:2210
- Verify that the join condition does not inadvertently filter out application versions with no supporting users, as this might impact the overall returned data.
leftJoin("version.supportingUsers", "versionSupportingUser", "versionSupportingUser.id IS NOT NULL")
dheepak-aot
reviewed
Apr 25, 2025
| }); | ||
| }, | ||
| }, | ||
| ...versionSupportingUsersMenuItems, |
dheepak-aot
approved these changes
Apr 25, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR revamps the Ministry view for change requests by updating the API to include supporting users information and by refactoring the UI menu to be more consistent with the Ministry main left menu. Key changes include:
- Enhancements to API DTOs and service methods to return supporting users alongside application versions.
- Removal of deprecated endpoints and methods related to supporting users.
- A complete overhaul of the Ministry sidebar component, integrating dynamic menu creation and active route support.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| sources/packages/web/src/types/contracts/Common.ts | Added new menu item properties (subtitle, class, slim, color, opacity) for UI styling improvements. |
| sources/packages/web/src/services/http/dto/SupportingUser.dto.ts | Removed the old ApplicationSupportingUsersAPIOutDTO interface in favor of a unified supporting users interface. |
| sources/packages/web/src/services/http/dto/Application.dto.ts | Introduced supportingUsers array in application version and overall details DTOs. |
| sources/packages/web/src/services/http/SupportingUserApi.ts & SupportingUserService.ts | Removed methods for fetching supporting users separately as this is now integrated into the overall application details. |
| sources/packages/web/src/composables/useApplication.ts & AESTApplicationSideBar.vue | Refactored menu generation and routing logic for the Ministry sidebar with enhanced integration and active state management. |
| sources/packages/backend/apps/api | Updated service and controller logic to return comprehensive application details (including supporting users) and revised error handling, with corresponding updates in E2E tests. |
Comments suppressed due to low confidence (1)
sources/packages/backend/apps/api/src/route-controllers/application/application.controller.service.ts:875
- Consider returning an empty array instead of undefined when there are no supporting users to ensure a consistent API response structure.
supportingUsers: application.supportingUsers.length ? application.supportingUsers.map((user) => ({
bidyashish
approved these changes
Apr 25, 2025
|
E2E Workflow Workers Coverage Report
|
Backend Unit Tests Coverage Report
|
E2E Queue Consumers Coverage Report
|
E2E SIMS API Coverage Report
|
andrewsignori-aot
deleted the
feature/#4616-change-request-ministry-application-view
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
API Related Changes
inProgressChangeRequest).UI related changes
activebehavior.In-progress change request
Parents and Partner with pending change request
Active application with versions and no pending request
How it was displayed on Edge (previously, there was extra white space between the icons)
Local change only to allow displaying the
Changed with approvalrenamed toChanged