-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add user deactivation functionality #607
Conversation
When I originally wrote the admin thing I tried expanding the ZOD schema and it failed, I made effective username optional so I was stumped at the time as to why it would not work, I am now realizing I forgot to make the search string optional! Since they are mutually exclusive basically it never worked for me lol. Edit: This is nicer, good work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. A few small things.
app/components/admin/users-table.tsx
Outdated
/> | ||
</Tooltip> | ||
<Form method="post" reloadDocument> | ||
<Tooltip label="Deactivate user"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use Delete
vs. Deactivate
everywhere, since we're really dropping the user from the db.
app/lib/user.server.ts
Outdated
|
||
export async function deleteUser(username: User['username']) { | ||
await deleteUserByUsername(username); | ||
setIsReconciliationNeeded(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return
here, so the Promise
is returned"
return setIsReconciliationNeeded(true);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing this, what if we add setIsReconciliationNeeded(true);
inside deleteUserByUsername
and just use it?
How about add a pop-up to reduce chances of mistake by admin? |
You mean like we do for deleting DNS records? That's a great idea. It could be filed/done as a follow-up. |
@@ -4,5 +4,5 @@ import { deleteUserByUsername } from '~/models/user.server'; | |||
|
|||
export async function deleteUser(username: User['username']) { | |||
await deleteUserByUsername(username); | |||
setIsReconciliationNeeded(true); | |||
return setIsReconciliationNeeded(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do a follow-up to audit all callers of setIsReconciliationNeeded(true)
to see if we can move them into the model methods per @cychu42's comment, if you don't want to do it here.
intent
field for each action on admin screen, to manage them properlyScreen.Recording.2023-04-12.at.19.05.01.mov
Closes #308