Locking down user group info #116
Conversation
burkkyy
changed the title
There was a problem hiding this comment.
While overall this looks good, I suggest breaking the profile edit and user edit forms into two forms based on role, see comments.
In general, I would expect admins to only be able to edit the extra fields when acting through the admin interface, rather than through their profile page.
| :readonly="isNil(groupMembershipAttributes.departmentId)" | ||
| :append-inner-icon="isNil(groupMembershipAttributes.departmentId) ? 'mdi-lock' : ''" |
There was a problem hiding this comment.
So ... this is a case where I would want to use disabled.
Conceptually, the field only activates once you fill the parent.
I see "locking" a field as something you would do when displaying a never editable field (e.g. email) in the middle of a bunch of editable fields (e.g. first name, last name)
There was a problem hiding this comment.
My previous request #116 (comment), is a bit confusing in contrast.
In that version, you where showing a field to a user for display purposes, but making it hard for them to read, via disabled, so I recommended the "locked" instead of "disabled" view. As that is easier to ready in a display context.
In this version, the field is only temporarily inaccessible until you fill in parent values; as such, it makes sense to make the field hard to read/easy to ignore, until the parent is filled in. At which point the field becomes "non-disabled" and easy to read, drawing the eye, and encouraging the user to then act on it.
There was a problem hiding this comment.
The fact the the UI now shows a different view to the User and Admin makes it possible to make use of this kind of UI (UX?) enhancement.
| v-bind="cancelButtonOptions" | ||
| text="Cancel" | ||
| /> | ||
| <!-- TODO: add ability to request update of yukon government directory information during save --> |
There was a problem hiding this comment.
Interestingly enough, I just built this in WRAP, and I wish I had put it in the "read" view rather than the "edit" view.
https://yg-wrap-uat.azurewebsites.net/profile
https://yg-wrap-uat.azurewebsites.net/profile/edit
I probably should have put the "Sync with Directory" button where the "Edit" button is, and moved the "Edit" to the bottom right of the "read" view.
| if (!isValid.value) { | ||
| snack.notify("Please fill out all required fields", { color: "error" }) | ||
| return | ||
| } |
There was a problem hiding this comment.
Interesting, I've been using form.value?.validate(), lately, and I'm thinking I just forgot that form v-model was a thing? I'll have to double check, since this version is a lot cleaner!
| readonly | ||
| append-inner-icon="mdi-lock" |
There was a problem hiding this comment.
As these fields are non-editable, do they even belong in an edit form? Maybe they do, like email in this form
Or maybe they don't, since the user can read them on the "read" view of the page (I'm assuming there is a read view?)
| > | ||
| Cancel | ||
| </v-btn> | ||
| text="Cancel" |
There was a problem hiding this comment.
Huh. I didn't know this was a thing.
| import { required } from "@/utils/validators" | ||
|
|
||
| import { GroupMembership } from "@/api/users-api" |
| <AdminUserEditForm | ||
| v-if="isSystemAdmin" | ||
| class="mt-10" | ||
| :user-id="currentUser.id" | ||
| :cancel-button-options="{ to: { name: 'ProfilePage' } }" | ||
| @saved="refresh" | ||
| /> |
There was a problem hiding this comment.
I don't think the profile is the correct place for an Admin edit interface.
I would expect all users to have the same field editability for their profile, and if the admin wants to edit a user's more restricted or complex details, they would go to the user admin interface; even if that user is them.
| const attributes: Path[] = ["email", "firstName", "lastName", "position"] | ||
|
|
||
| if (this.user.isSystemAdmin) { | ||
| attributes.push({ | ||
| groupMembershipAttributes: ["departmentId", "divisionId", "branchId", "unitId"], | ||
| }, | ||
| ] | ||
| }) | ||
| } | ||
|
|
||
| return attributes |
| <AdminUserEditForm | ||
| v-if="isSystemAdmin" | ||
| class="mt-10" | ||
| :user-id="user.id" | ||
| :cancel-button-options="{ to: { name: 'UsersPage' } }" | ||
| @saved="refresh" | ||
| /> | ||
| <UserEditForm | ||
| v-else |
There was a problem hiding this comment.
This page is already the user admin page, so I'd expect only the edit form here to be the "admin" one.
Only Admins should be able to access the "All Users" page, and only the all user's page grants you access to this UserEditPage.
See web/src/components/base-layout/KebabMenu.vue
<v-list-item
v-if="isSystemAdmin"
title="All Users"
:to="{ name: 'UsersPage' }"
prepend-icon="mdi-account-group"
/>
You could definitely argue that this should be called the AdminUserEditPage, and I'd would probably agree.
Normally, there might be a "read" view for all users that was publicly accessible, and the "Edit" button would only be shown to admins.
| import usersApi, { RoleTypes, type User, type UserUpdate } from "@/api/users-api" | ||
|
|
||
| export { type User, type UserUpdate } | ||
| export { RoleTypes, type User, type UserUpdate } |
There was a problem hiding this comment.
Consider adding type RoleTypes as this will play better with the Typescript + Vitest. (vitest is what we will likely be using for testing, once we do more of that)
|
It might be worth splitting the ProfileEdit, and UserEdit out into a "read-only" and "edit" version, as seen through the WRAP project. Read view Organization Edit view organization This lets you make the read view be make more readable and the code less complex. |
resolves #106
Implementation
Screenshots
figure 1 profile/edit as a regular user
figure 2 profile/edit as a system_admin
Testing Instructions
dev test(ordev test_api)dev upsystem_admin)system_admin