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

User Changes: Cards and Modals for Users Components. Also added notes to users. #1306

Merged
merged 76 commits into from
Aug 4, 2024

Conversation

mikedivine
Copy link
Contributor

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

  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

The admin component changes will be a separate PR.

juanjoseguva and others added 30 commits June 11, 2024 10:05
…l needs to be changed to cards model, like units.
… implementing the redux framework in MapViewComponent.
…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.
… well as username for backwards compatibility
@mikedivine mikedivine requested a review from huss August 1, 2024 22:40
Copy link
Member

@huss huss left a 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.

obviusEmailAndPasswordAuthMiddleware('Obvius pipeline')(req, res, next);
req.body.username = username;
req.body.password = password;
obviusUsernameAndPasswordAuthMiddleware('Obvius pipeline')(req, res, next);
Copy link
Member

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.

Copy link
Contributor Author

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;

Copy link
Contributor Author

@mikedivine mikedivine Aug 4, 2024

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

@huss
Copy link
Member

huss commented Aug 4, 2024

I just pushed two items via commits:

  1. Added code to more consistently enforce min/max on user info.
  2. Merged development to remove conflict.

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.

@huss
Copy link
Member

huss commented Aug 4, 2024

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.

Copy link
Member

@huss huss left a 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.

@huss huss merged commit 661d8b8 into OpenEnergyDashboard:development Aug 4, 2024
3 checks passed
@huss huss mentioned this pull request Aug 9, 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.

5 participants