[web][api][SIMS #540] Separate Institution Details and Institution User Profile #627
Conversation
dheepak-aot
requested review from
abschwenker,
andrewsignori-aot,
ann-aot and
guru-aot
|
Please have a look at the other tests failing as well |
sources/packages/api/src/route-controllers/user/models/institution-user.dto.ts
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| @Get("/institutionUser") |
There was a problem hiding this comment.
IMO, the information being returned here is not institution-specific. I would recommend that this endpoint would be generic GET users.
There was a problem hiding this comment.
I guess we decided to have it as /institution. Right @dheepak-aot, @andrewsignori-aot @ann-aot ?
sources/packages/api/src/route-controllers/user/user.controller.ts
Outdated
Show resolved
Hide resolved
sources/packages/web/src/views/institution/InstitutionUserProfile.vue
Outdated
Show resolved
Hide resolved
| toast.add({ | ||
| severity: "success", | ||
| summary: "Updated!", | ||
| detail: "Institution and User successfully updated!", |
There was a problem hiding this comment.
We are no longer saving institution data on this screen. Please adjust the messages accordingly.
There was a problem hiding this comment.
Messages have been updated accordingly.
| // Hooks | ||
| onMounted(async () => { | ||
| const bceidAccount = await UserService.shared.getInstitutionUser(); |
There was a problem hiding this comment.
This should be treated as our user data since it is in our DB. The use of the BCeID account could lead to misinterpretations. Could you please rename it to something like "userAccount", please?
There was a problem hiding this comment.
const has been renamed.
| const bceidAccount = await UserService.shared.getInstitutionUser(); | ||
| if (bceidAccount) { | ||
| initialData.value = { | ||
| userFirstName: bceidAccount?.userFirstName, |
There was a problem hiding this comment.
There is already a check in place on line 68 to ensure that the variable is defined. Could you please remove the null check while accessing the properties?
There was a problem hiding this comment.
Null check removed.
| } else { | ||
| toast.add({ | ||
| severity: "error", | ||
| summary: "BCeID Account error", |
There was a problem hiding this comment.
IMO, this is not related to BCeID account, can you please adjust the message.
There was a problem hiding this comment.
Message modified.
andrewsignori-aot
left a comment
andrewsignori-aot
left a comment
There was a problem hiding this comment.
Please review the comments.
Is this commit a partial delivery of the ticket or is it supposed to deliver it entirely?
If it is a complete delivery, there are some missing acceptance criteria, IMO.
Once the institutionuserprofile form definition is completed it must be added to code repo, the same for any other form definition that was updated as part of this ticket.
There are some readonly components in the screen that do not need to be sent to the API and they could be configured as below.
| data: InstitutionUserDetailsDto, | ||
| authHeader?: any, | ||
| ): Promise<void> { | ||
| return ApiClient.User.updateInstitutionUser(data, authHeader); |
There was a problem hiding this comment.
promising void and you are returning a value
There was a problem hiding this comment.
return statement has been removed.
| headers?: any, | ||
| ): Promise<void> { | ||
| try { | ||
| const response = await this.apiClient.patch( |
There was a problem hiding this comment.
i think const response is not used inside the scope, you can remove it
There was a problem hiding this comment.
Yes. The const has been removed
| }); | ||
| } | ||
| if (redirectHome) { |
There was a problem hiding this comment.
IMO, I think you can avoid this check and flags and add the redirect logic inside the try
There was a problem hiding this comment.
As discussed the redirect logic is removed from condition.
| userEmail: bceidAccount?.userEmail, | ||
| }; | ||
| } else { | ||
| toast.add({ |
There was a problem hiding this comment.
I think @andrewsignori-aot, has added utils for toast, we can use that from now on. correct me if I am wrong @andrewsignori-aot
There was a problem hiding this comment.
Toast utils updated
guru-aot
left a comment
guru-aot
left a comment
There was a problem hiding this comment.
Please check on @ann-aot @andrewsignori-aot comments, I do second them on the comments. Nice overall work @dheepak-aot :)
| label: "Notifications Settings", | ||
| icon: "pi pi-bell", | ||
| command: () => { | ||
| AppConfigService.shared.logout(ClientIdType.INSTITUTION); |
There was a problem hiding this comment.
The notifications settings should not point to the logout. I would recommend adding a placeholder page in the same way that we did for Students.
There was a problem hiding this comment.
Agreed. Forgot to remove that action for notifications icon.
| await UserService.shared.updateInstitutionUser( | ||
| institutionUserPersistDto, | ||
| ); | ||
| toast.success("Updated!", "Institution and User successfully updated!"); |
There was a problem hiding this comment.
The message is mentioning the institution, but I believe that only the user is being updated here, right?
There was a problem hiding this comment.
Again a copy paste issue. will fix it.
andrewsignori-aot
left a comment
andrewsignori-aot
left a comment
There was a problem hiding this comment.
Nice start 😉 👍
|
Kudos, SonarCloud Quality Gate passed!
|
abschwenker
left a comment
abschwenker
left a comment
There was a problem hiding this comment.
LGTM, thank you for reviewing the comments. Once everyone is approved please merge
ann-aot
left a comment
ann-aot
left a comment
There was a problem hiding this comment.
Nice work @dheepak-aot 👍
dheepak-aot
changed the title
Separate Institution Details and Institution User Profile Information #540