User Management UI #511
User Management UI #511
Conversation
fidesops.toml
Outdated
| @@ -25,5 +25,5 @@ DRP_JWT_SECRET="secret" | |||
| TASK_RETRY_COUNT=0 | |||
| TASK_RETRY_DELAY=1 | |||
| TASK_RETRY_BACKOFF=1 | |||
| REQUIRE_MANUAL_REQUEST_APPROVAL=false | |||
| REQUIRE_MANUAL_REQUEST_APPROVAL=true | |||
There was a problem hiding this comment.
Can we change this back to false? Right now the spec. is for Fidesops to default to automatically approving privacy requests, so it's causing the API tests to fail as they expect this to be happening.
There was a problem hiding this comment.
Hey @LKCSmith great start! I've spent some time functionally testing this afternoon and found the following to address:
- User is prompted to login inconsistently on view change — were you experiencing this during development?
- No error message in UI for user creation 422s (e.g. password not strong enough) — this should ideally show the same way as if a username or password is omitted, with a different message
- View users permission omitted from list — I suspect this was an oversight in the design? What happens is that new users created are not able to view users in the system, even if they are able to create them
- Login screen asks for email, should read username for now — not directly related to this PR but would be nice to correct
- Privileges toggles not working correctly (unless this is intentional and I'm not across the decision?)
- Viewing policies auto toggles viewing datastore connections
- Create policies auto triggers approve subject requests
- Can’t remove dependencies ^
|
I'm not experiencing these bugs locally either. Maybe you can demonstrate to me on a call? |
There was a problem hiding this comment.
Wow, nice job getting this all set up! 💪
Here are also some comments from playing around locally:
- Creating a new user doesn't tell me which fields are required but doesn't let me save if there's no password
- Only says it's required if you have touched it
- Unchecking a permission and saving seems to work, but get a POST error in the console
POST http://0.0.0.0:8080/api/v1/api/v1/user/fid_3abb8caf-29a5-48d5-a4d9-b8c362ccf9c6/reset-password 404 (Not Found)
- No error when trying to delete a user and the usernames don't match
- Can't tab between the two fields of username and enter username again
- Can edit the superuser and make it so they are stuck and can't edit anymore :(
- Repro steps:
- Edit your superuser and remove some permissions (I removed "Approve subject requests", "view datastore connections", "create or update datastore connections", "delete datastore connections")
- After you save, there will be no users listed in the User Management page (not sure if one of those perms I removed controls that? I purposefully didn't remove the "view users" policy)
- Looks like this is because of a 403 forbidden, though I'm not sure which of those permissions makes me unable to view users anymore. Also, it seems undesirable to let the superuser get themselves into this state.
- Repro steps:
| return result; | ||
| }) | ||
| .then((result) => { | ||
| console.log('result', result); |
There was a problem hiding this comment.
probably want to clean up console.log's
| router.replace('/user-management'); | ||
| }); | ||
| }, | ||
| validate: (values) => { |
There was a problem hiding this comment.
outside the scope of this PR, but I would vote to use something like Yup for validation https://formik.org/docs/guides/validation
| id={`scopes-${policy.privilege}`} | ||
| name="scopes" | ||
| value={policy.scope} | ||
| isDisabled={policy.scope === 'privacy-request:read'} |
There was a problem hiding this comment.
nit: maybe pull out 'privacy-request:read' as a constant?
There was a problem hiding this comment.
out of curiosity, how did you get rid of that uncontrolled vs controlled input console log msg?
| old_password: existingUser?.password, | ||
| new_password: values.password, | ||
| }; | ||
| console.log(updatePasswordbody); |
There was a problem hiding this comment.
def want to remove this one :)
There was a problem hiding this comment.
and while you're at it.... another nit: updatePasswordbody --> updatePasswordBody
| scopes: [], | ||
| }, | ||
| onSubmit: async (values) => { | ||
| const host = |
There was a problem hiding this comment.
I don't think this const gets used anywhere?
|
|
||
| if (!/[\W]/.test(values.password)) { | ||
| errors.password = 'Password must have at least one symbol.'; | ||
| } |
There was a problem hiding this comment.
As an example of how Yup would work for this logic: https://stackoverflow.com/questions/49502436/password-validation-with-yup-and-formik
| id={`scopes-${policy.privilege}`} | ||
| name="scopes" | ||
| // @ts-ignore | ||
| isChecked={values.scopes[policy.privilege]} |
There was a problem hiding this comment.
hmm what does typescript complain about it here? does it just need to be coerced into a boolean via !!?
| > | ||
| Save | ||
| </Button> | ||
| </chakra.form> |
There was a problem hiding this comment.
is this form almost the same as edit user? are there any parts that could be refactored so that we don't have to make changes on both sides?
| import { User } from '../user/types'; | ||
| import { useUpdatePasswordMutation } from '../user/user.slice'; | ||
|
|
||
| function UpdatePasswordModal(user: User) { |
There was a problem hiding this comment.
how come we don't use formik for this one? It's simpler than the other form, but for handling errors and such, may still want to go through formik
| export const userPrivilegesArray: UserPrivileges[] = [ | ||
| { | ||
| privilege: 'View subject requests', | ||
| scope: 'privacy-request:read', |
There was a problem hiding this comment.
I think one day, not necessarily in this PR, we will want to pull these out as constants, such as like what is in the backend https://github.com/ethyca/fidesops/blob/user-management/src/fidesops/api/v1/scope_registry.py so that it'll be a lot easier in code to refer to the permissions
|
Absolutely crushing it, Laura. Love how fast you onboarded and that you're knocking out such a big feature already. Great work! 🎉 I think the functional testing Sean and Allison did is quite thorough. In addition to what they've found, I've noticed the following things:
Of these, the only critical bug is the sporadic logout issues. However, since the functionality is there, I'm comfortable merging, provided we address this bug this week (something I'm happy to take on today / tomorrow). The rest of these don't need to be addressed in these PR either – I'm just dropping them here for future reference. I also have a number of stylistic / cleanup related nitpicks w/r/t the code. I began to make individual comments to address them, but I think in this case it might be more efficient for me to clean them up on my own after this PR is merged, and then we can do a bit of a post mortem next week and review some of the changes I made. And to be clear, when doing PR reviews in general, it's generally my opinion that any syntax or style things that come up are more a function of the scope of work and the time constraint around it than of somebody's personal coding style. So don't feel like any changes I make are because mistakes were made or that there were glaring oversights. I totally recognize that it's more just the sheer volume of code leaves plenty of spaces for bugs to hide and makes cleanup difficult + time-consuming :) Finally, I'd like to address the scope of this PR – I think going forward we'll want to try to break features down into smaller, more manageable PRs so that we don't get into a place of overwhelm like this again. That would help with both finding bugs, general feelings of overwhelm, and also give more time and space to do cleanup. Again, amazing job with this! Very happy to see it land. 🥳 Overall, I'm happy to merge this, and then we can schedule in some cleanup over the next week or two. |
* Start setup on user management feature * User form * Add set-up for some actions and profile page with form * Work on new user * Update row * Clean-up navbar * Set active classnames in nav links * Add privileges * Cancel functionality on new user form * User slice * Delete user modal * Breadcrumbs and delete user modal as menuitem * User POST in slice * Update slice * Create through slice * Set-up for future api work * Start get * Create and Delete set-up * Delete user validation * Amend * User search * Edit tweaks * User privileges * Ui tweaks * Permissions * Add additional user info to table * Checkboxes * Some small form fixes * Refreshes and checkboxes on load * Default values * Split out edit and new forms for easier flow and initial value in formik handlin * Type checking * Some file cleanup * Pagination; * Some style tweaks * Change checkbox color * Checkboxes * Cleanup * Remove unused effect * Update session with user info and fix client console warning for controlled input * Update session in pages * Some form cleanup * Clean index page * Edit user permissions checks * Feedback * Remove duplicate * Fix checkbox bug * Password update * Add error handling on new user form * Change password modal * Remove rogue console Co-authored-by: Laura Smith <laurasmith@Lauras-MacBook-Pro.local>
Purpose
This adds the User Management UI to ops
Changes
Note/Asks for reviewers:
Checklist
Run Unsafe PR Checkslabel has been applied, and checks have passed, if this PR touches any external servicesTicket
Closes #357
Closes #358
Closes #359
Closes #360