feat: ability to remove admins ❌

[wflore19]

Description ✏️

Closes #250

• Added AdminDropdown with the option to remove an admin to the AdminsTable component.
• Added removeAdmin() function to update an admins deletedAt column.
• Added modal to confirm the removal of an admin.

Type of Change 🐞

• Feature - A non-breaking change which adds functionality.
• Fix - A non-breaking change which fixes an issue.
• Refactor - A change that neither fixes a bug nor adds a feature.
• Documentation - A change only to in-code or markdown documentation.
• Tests - A change that adds missing unit/integration tests.
• Chore - A change that is likely none of the above.

Checklist ✅

• I have done a self-review of my code.
• I have manually tested my code (if applicable).
• I have added/updated any relevant documentation (if applicable).

[ramiAbdou]

@wflore19 I know that the issue was stale, but please still comment on any issues that you'd like to take so other people know who's assigned to what 🙏

[wflore19]

@ramiAbdou heard, i think there's a potential issue with this pr, when removed from admins list i'm still logged in

[tomas-salgado]

@wflore19 is this still a WIP or ready for review? If you're still working on it, let's switch this to a draft PR!

[wflore19]

@tomas-salgado sounds good, I might need help with this one, how does authentication work for admins? Auth needs to be revoked when an admin is removed. Also can any admin remove another admin? What is the difference between temp admins vs FT admin?

[ramiAbdou]

@tomas-salgado sounds good, I might need help with this one, how does authentication work for admins? Auth needs to be revoked when an admin is removed. Also can any admin remove another admin? What is the difference between temp admins vs FT admin?

Great question - we actually need to slightly rework our authentication/authorization mechanism in the Admin Dashboard. Will push an update and follow up a bit later today!

[ramiAbdou]

Hey @wflore19 - following up here, I was able to merge #426 yesterday which revamped some of the authentication logic in the Admin Dashboard. Here's what you need to know:

• Instead of just checking for the presence of a user_id in the session, now we actually query for the admin in the database. This ensures that if someone was logged into the Admin Dashboard, gets deleted and then goes back to the Admin Dashboard, they will be logged out despite previously being logged in. See getAuthenticationStatus.
• I introduced a role to the admins table, which can be 'ambassador' | 'admin' | 'owner'.
  • Owners can do anything.
  • Admins can add/remove other admins/ambassadors.
  • Ambassadors can't do anything.
• We can protect a route using ensureUserAuthenticated and the new minimumRole option.
• We also now have access to a hook called useAdminRole, which allows you to use the role in front-end logic (ie: whether or not to display the "delete" button).

Also, just noting that I consolidated some of the admins module (we're going to less splitting up of files going forward and more colocating of related logic), so admin.core.ts is where most business logic should live. Feel free to model your work after the addAdmin function!

Let me know if you have any questions at all! Also apologies on the merge conflicts! 😭

[tomas-salgado]

This looks great! Tested out the functionality and it all works correctly.

The only thing I wonder is if we should implement the functionality so that you can't remove yourself as an admin 😆. I don't think there would ever be a reason to do this, and it would most likely just be a mistake.

[wflore19]

@tomas-salgado sounds good i'll make the change

[wflore19]

Notes

In order to conditionally render the delete action i can either

• pass the userId prop from AdminTable -> Table -> TableBody
• refactor the table.dropdown component to pass an id prop, to conditionally render some options

[wflore19]

Update

1
I see that all the data (list of admins) is being passed down to <TableBody /> and renders the rows of data. So I would need to prop drill the current adminsId all the way down in order to conditionally render the <Dropdown />. This might be an issue as I would need to directly change <TableBody /> specifically for the <AdminTable /> implementation, which might not work for other implementations of <TableBody />.

2
I'm going to change <AdminsTable /> -> <Table />, I see that applications doesn't use <ApplicationsTable /> it just implements <Table />.

3
I could just filter out the current signed in admin from the list of admins in listAdmins()

[ramiAbdou]

@wflore19 the Table.Dropdown component actually has access to all the data for the row it's being rendered in -- notice how it's being rendered here (ie: <Dropdown {...row} />).

So in this case, if we wanted to omit the dropdown for certain rows, we can just return null early before rendering the actual dropdown (ie: Dropdown.Container)! Let me know if that makes sense.

[wflore19]

@ramiAbdou That makes sense, if the current users id EQUALS the id given from row data, return null. How should I handle passing the current users 'id' to the '<Dropdown >', w/ useContext or prop drilling?

[wflore19]

Ok I implemented w/ useContext(). <Dropdown /> conditionally renders if the userId is EQUAL to id

[tomas-salgado]

LGTM! Great work @wflore19

[ramiAbdou]

Hey @wflore19 -- just tested this out and it looks really great! 🔥

Really appreciate you asking questions throughout this PR, it inspired an important change around the way that we handle auth, which is really great for us. Thanks for sticking through this PR even despite some of the uncertainty!

I made a few updates here that revolve around whether or not the admin has permission to remove another admin. Check it out and let me know if you want to talk through some of those changes.

Another Wilfredo PR in the books! 🚀