-
Notifications
You must be signed in to change notification settings - Fork 365
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
User Changes: Cards and Modals for Users Components. Also added notes to users. #1306
User Changes: Cards and Modals for Users Components. Also added notes to users. #1306
Conversation
… with the units creation flow
…l needs to be changed to cards model, like units.
… implementing the redux framework in MapViewComponent.
…y setup with backend.
…s, etc. Moved files inside users folder, removed old files since we are now using modals. Still needs the Save User Edits addressed as it's currently throwing an error.
…password and/or change the email. Definitely would be good to rewrite api to allow password change. Not sure if we would want to allow email change. Might be better to make them delete the account and create a new one.
…trying to switch their own role to non-admin
…of localUser props in user components since API no longer requires all users to be sent.
… to test for both
… well as username for backwards compatibility
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.
Thanks to the team for the updates. This is looking good overall and works as expected. I've made a few comments to address including on some previous ones. I'm hoping the changes are not very much.
src/client/app/components/admin/users/CreateUserModalComponent.tsx
Outdated
Show resolved
Hide resolved
src/client/app/components/admin/users/CreateUserModalComponent.tsx
Outdated
Show resolved
Hide resolved
obviusEmailAndPasswordAuthMiddleware('Obvius pipeline')(req, res, next); | ||
req.body.username = username; | ||
req.body.password = password; | ||
obviusUsernameAndPasswordAuthMiddleware('Obvius pipeline')(req, res, next); |
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'm not really knowledgeable here but I want to verify you did not break this code. The original set req.body.password and that is what src/server/routes/authenticator.js uses. Is the fact it is in req.param('password') the same? We have to be very careful with any authentication changes.
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 do not believe so. This: req.body.username = req.param('username'); I believe is equivalent to this: const username = req.param('username') || req.param('email'); req.body.username = username;
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 see some tests in the readme.md but not sure how to run them to test if everything is working properly
…ed install script to use more descriptive variable names as requested
…ial invalid value after a valid role is chosen from the drop down
- Also limits route lengths to avoid issues.
I just pushed two items via commits:
I did this because I accidentally merged the new admin settings page so I wanted to quickly get this merged so people have access to the user page. If anyone sees any issue then let me know. |
The new mins on user name means that if someone had one of less than 3 it would not longer work. Since they used to be emails this seems safe. Though unlikely, a now invalid password could be present. If this is the case then it would need to be reset. |
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.
Thanks to @mikedivine for these last changes. I appreciate your willingness to consider changes and for making the all even when that mean new items. I've tested the code and reviewed. It all seems fine.
Description
This PR changes the look of the user components and aligns it with the new modal card look in OED. User Notes have been added to the DB. Create button to create a user, a card for each user, edit button opens a modal that allows changes to the email, password, role, and notes. Button to delete the user and confirmation modal.
Option is added that is labeled "Users" (only if an admin). This takes the admin to the user card page.
Partly Addresses #890 - Users Components completed only
Type of change
Checklist
Limitations
The admin component changes will be a separate PR.