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.

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

Admin UI - User Deletion Modal - Only Require One Layer of Confirmation #4402

Merged
merged 17 commits into from
Jan 31, 2024

Conversation

RobertKeyser
Copy link
Contributor

Closes #

Description Of Changes

Today in the Admin UI, we require a user to enter the name of a user to delete twice. While this does indeed confirm that the correct user is about to be deleted, confirming it twice is a bit overkill. Instead, this PR changes the modal to only require the deleting user to enter the target's username once. Additionally, it includes a <Text> element with the user's username.

New:
image

Old:
image

Adding the user's name to the deletion modal helps to make sure that you have the right user if you're on the full list of users.

Code Changes

  • remove one of the username confirmation text fields
  • add a text element with the user's name

Steps to Confirm

  • create user
  • delete user, there should only be one field for deletion.

Pre-Merge Checklist

@RobertKeyser RobertKeyser self-assigned this Nov 7, 2023
Copy link

vercel bot commented Nov 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Jan 31, 2024 8:35pm

@RobertKeyser
Copy link
Contributor Author

RobertKeyser commented Nov 7, 2023

Are there any Cypress tests that I'd need to adjust for this? Not sure where to find them (or how to edit them 😸)

@RobertKeyser RobertKeyser marked this pull request as ready for review November 7, 2023 22:45
Copy link

cypress bot commented Nov 7, 2023

Passing run #6114 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 58796d0 into acc2541...
Project: fides Commit: 7b55506940 ℹ️
Status: Passed Duration: 00:34 💡
Started: Jan 30, 2024 9:11 PM Ended: Jan 30, 2024 9:12 PM

Review all test suite changes for PR #4402 ↗︎

@allisonking
Copy link
Contributor

It does look like the delete user test is failing 😄 https://github.com/ethyca/fides/actions/runs/6791203976/job/18462232743?pr=4402#step:6:1217

I imagine you have to remove instances where it is looking for that first username input https://github.com/ethyca/fides/blob/main/clients/admin-ui/cypress/e2e/user-management.cy.ts#L215

hopefully this README can get you going on running the cypress tests locally to see if it passes, but I'm happy to help if you run into anything! sometimes cypress is easy, other times it's super hard 🥲

@RobertKeyser RobertKeyser marked this pull request as draft November 8, 2023 14:19
@Kelsey-Ethyca
Copy link
Contributor

This is a nice UX improvement thanks @RobertKeyser! Can I suggest adding some context next to the user name for example "User to be deleted: "

@RobertKeyser
Copy link
Contributor Author

New deletion modal:
image

Copy link
Contributor

@Kelsey-Ethyca Kelsey-Ethyca left a comment

Choose a reason for hiding this comment

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

Great ux update and test fix!

UAT passing
Screenshot 2024-01-31 at 12 33 50 PM

@RobertKeyser RobertKeyser merged commit 65af596 into main Jan 31, 2024
7 of 8 checks passed
@RobertKeyser RobertKeyser deleted the rkeyser/user-delete-modal branch January 31, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants