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

Allow admins to resend email verification to users #1228

Merged
merged 8 commits into from
Nov 14, 2024

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Nov 12, 2024

Fix #1210.

User modal now has an admin-only button to resend verification email to any users who aren't verified. Button only shows up if an admin is logged in and the user is unverified.

screenshot

Copy link

github-actions bot commented Nov 12, 2024

UI unit Tests

12 tests  ±0   12 ✅ ±0   0s ⏱️ ±0s
 4 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit e9ef6eb. ± Comparison against base commit 4cfc4b5.

♻️ This comment has been updated with latest results.

@rmunn
Copy link
Contributor Author

rmunn commented Nov 12, 2024

Just realized that the way I implemented this, the button will only do something if the user modal was accessed from the admin dashboard. I did that because I thought "We shouldn't put admin-dashboard only code into the user modal" so I put the "resend email" call in the admin dashboard page, rather than in the modal. But really, we want the modal to work the same way no matter what page it's on. So I'm going to move the "resend email" code into the modal itself, so that it will also work if an admin gets to that user modal from clicking on an org member on the org page, or a project member on the project page.

Update: Fixed in commit 2fa7bbb.

Copy link

github-actions bot commented Nov 12, 2024

C# Unit Tests

75 tests  ±0   75 ✅ ±0   5s ⏱️ ±0s
13 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit e9ef6eb. ± Comparison against base commit 4cfc4b5.

♻️ This comment has been updated with latest results.

That way the button will work no matter what page the admin started on
when he clicked on a user account.
@rmunn rmunn requested review from myieye and hahn-kev November 12, 2024 06:04
Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

it looks good to me, though I'm not sure I like the idea of putting actions in the user modal, if only because there are no other ones there, and it won't be obvious to go there to send the email, I noted on the issue that it might make sense to be a menu item on the admin dashboard. @myieye what do you think? Feel free to override my suggestion.

@myieye
Copy link
Contributor

myieye commented Nov 12, 2024

it looks good to me, though I'm not sure I like the idea of putting actions in the user modal, if only because there are no other ones there, and it won't be obvious to go there to send the email, I noted on the issue that it might make sense to be a menu item on the admin dashboard. @myieye what do you think? Feel free to override my suggestion.

Tricky 🤔...

TLDR;
I like where @rmunn has it now.

Long version:
Putting things in the User details modal has the added benefit that it's available on the admin dashboard, project page and org page (I assume org admins should have this feature eventually).

Ultimately, I like the idea of merging the edit user and user details modals. (e.g. Having an "Edit" button in the User details modal that replaces the modal contents with the Edit form.) It's strange that you have to close the user details modal and open a different one in order to edit the user. Merging them would have the added benefit of also being able to edit a user in the context of a project or org.

I'm not proposing we make that change in this PR! 😉

But that change would result in the user details modal being "one user modal to rule them all". So, I think that's where the "Resend verification email" button belongs.

Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

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

I like this so far 👍
But, there are a few changes I'd like to see 🙂

frontend/src/lib/components/Users/UserModal.svelte Outdated Show resolved Hide resolved
frontend/src/lib/components/Users/UserModal.svelte Outdated Show resolved Hide resolved
frontend/src/lib/components/Users/UserModal.svelte Outdated Show resolved Hide resolved
@rmunn rmunn requested a review from myieye November 13, 2024 09:49
@rmunn
Copy link
Contributor Author

rmunn commented Nov 13, 2024

@myieye - Addressed all your comments. Ready for re-review.

Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

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

I pushed a few changes:

  1. The notification was hidden behind the modal, so I put notifications inside our Modal component and added a demo to our sandbox page that seems to look fine. I believe the notifications are now in 2 places in the dom if they're in a modal, but it's visually impossible to see that, so I'm happy.
    image

  2. I made the button our primary color. It just didn't look enough like a button for my taste 😄
    image

  3. I added a loading state to the button (partially because it can take a while to send an email)
    image

@rmunn rmunn merged commit fef4c1b into develop Nov 14, 2024
14 checks passed
@rmunn rmunn deleted the feat/resend-email-verification branch November 14, 2024 01:49
@rmunn rmunn self-assigned this Nov 15, 2024
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.

Allow admins to request lexbox send verification email for user
3 participants