Edit roles page with admin privileges #330
Conversation
|
WIll fix the tests in the future |
| @@ -18,8 +18,8 @@ module.exports = [ | |||
| { | |||
| type: 'postgres', | |||
| url: process.env.DATABASE_URL, | |||
| ssl: sslConfig.ssl, | |||
| extra: sslConfig.extra, | |||
| ssl: prodSSLConfig.ssl, | |||
There was a problem hiding this comment.
why are we changing this?
There was a problem hiding this comment.
This was originally there for me personally when connecting with the dev DB. Before I finish this PR I plan on going ahead and reverting this file back. I use npm instead of docker
| handleTabs = () => { | ||
| switch (this.state.role) { | ||
| case 'admin': | ||
| return AdminTabs; | ||
| case 'officer': | ||
| return OfficerTabs; | ||
| default: | ||
| return InducteeTabs; | ||
| } |
There was a problem hiding this comment.
| handleTabs = () => { | |
| switch (this.state.role) { | |
| case 'admin': | |
| return AdminTabs; | |
| case 'officer': | |
| return OfficerTabs; | |
| default: | |
| return InducteeTabs; | |
| } | |
| getTabsByRole = (role: string) => { | |
| switch (role) { | |
| case 'admin': | |
| return AdminTabs; | |
| case 'officer': | |
| return OfficerTabs; | |
| default: | |
| return InducteeTabs; | |
| } |
easier to unit test
frontend/src/constants/routes.js
Outdated
| @@ -11,6 +11,7 @@ export const EVENT_EDIT = '/events/:eventId/edit'; | |||
| export const EVENT_DETAILS = '/events/:id'; | |||
| export const EVENT_CREATION = '/events?create=true'; | |||
| export const TEST = '/test'; | |||
| export const EDIT_ROLES = '/editroles'; | |||
There was a problem hiding this comment.
| export const EDIT_ROLES = '/editroles'; | |
| export const EDIT_ROLES = '/roles'; |
in general routes don't have verbs
| }, | ||
| ]; | ||
|
|
||
| const ComponentToRender = () => { |
There was a problem hiding this comment.
it's been a while, but can't we just return the function instead of naming it ComponentToRender?
There was a problem hiding this comment.
I'll check this out more in detail and will follow up. I believe initially I had some issues with referencing the types from the MultipleAppUserResponse imports when just returning the function but worked when putting it under a component. Again, I'll look more deep into this.
| }; | ||
|
|
||
| function MemberTable(): JSX.Element { | ||
| const { data, isLoading, error, isError } = useRequest< |
There was a problem hiding this comment.
can we push all the useRequest calls up to the page level component, then pass in the responses down to the table as a prop? makes things easier to read
There was a problem hiding this comment.
In hindsight yeah this is a better change. I remember writing this when I was first learning react. Good catch
| role: newRole, | ||
| }); | ||
| } catch (error) {} | ||
| window.location.reload(); |
There was a problem hiding this comment.
we shouldn't be refreshing the window from a react component, what are we trying to do here?
There was a problem hiding this comment.
This was a temporary fix for testing. I was trying to figure out a way to make it so the edited roles we submit upon popup show up on the table, since the rows themselves are only fetched once instead of updated automatically. I'm still trying to figure out a solution for this, so if you have any suggestions I'd greatly appreciate them. The rows aren't state variables either which makes it a bit harder for me to figure out.
There was a problem hiding this comment.
EditRolesButton can take in a callback from the parent component that sets the state of the parent component
| lastName?: string; | ||
| } | ||
|
|
||
| class EditRolesButton extends React.Component< |
There was a problem hiding this comment.
I think we can just make this a functional component? Do we use class components anywhere else in the codebase? (sorry it's been a while)
There was a problem hiding this comment.
We use a mix of class and functional components in the codebase I believe. In this case it's fine for it to be a functional component. I'll go ahead and make that change accordingly.
| <Button | ||
| onClick={() => this.updateUserRole('guest')} | ||
| color='primary' | ||
| > | ||
| Guest | ||
| </Button> | ||
| <Button | ||
| onClick={() => this.updateUserRole('inductee')} | ||
| color='primary' | ||
| > | ||
| Inductee | ||
| </Button> | ||
| <Button | ||
| onClick={() => this.updateUserRole('member')} | ||
| color='primary' | ||
| > | ||
| Member | ||
| </Button> | ||
| <Button | ||
| onClick={() => this.updateUserRole('officer')} | ||
| color='primary' | ||
| > | ||
| Officer | ||
| </Button> | ||
| <Button | ||
| onClick={() => this.updateUserRole('admin')} | ||
| color='primary' | ||
| > | ||
| Administrator | ||
| </Button> |
There was a problem hiding this comment.
this is a small thing, but you can also do the following
const roles = ['Admin', 'Officer',...];
return roles.map(role => {
<Button
onClick={() => this.updateUserRole(role)}
color='primary'
>
{role.toLowerCase()}
</Button>
})| handleEditRolePopup() { | ||
| this.setState({ | ||
| isPopupOpen: true, | ||
| }); | ||
| } | ||
| handleEditRoleClose() { | ||
| this.setState({ | ||
| isPopupOpen: false, | ||
| }); | ||
| } |
There was a problem hiding this comment.
maybe sth like the below? probably don't need 2 functions
| handleEditRolePopup() { | |
| this.setState({ | |
| isPopupOpen: true, | |
| }); | |
| } | |
| handleEditRoleClose() { | |
| this.setState({ | |
| isPopupOpen: false, | |
| }); | |
| } | |
| setShowPopup(shouldShowPopup bool) { | |
| this.setState({ | |
| isPopupOpen: shouldShowPopup, | |
| }); | |
| } |
There was a problem hiding this comment.
Would need to do some refactoring but yeah it does shorten it up a bit. I'll also implement this.
| firstName: string; | ||
| lastName: string; | ||
| } | ||
| interface INITIAL_STATES { |
There was a problem hiding this comment.
This INITIAL_STATES pattern is pretty old (i think pre functional components?); I remember writing stuff like this when I was first learning react 4 years ago
(TO BE FILLED IN LATER)