-
Notifications
You must be signed in to change notification settings - Fork 111
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
Update User to have Administrator Field #441
Comments
We might want to make the field a boolean type. Is there a reason you want it to be typed as a Number? Either way could work, I was just wondering |
I have a solution made for this now. I would love to be assigned this. I also have code ready that prevents access to the adminDashboard route unless user has admin property of 1. However, try as I might, I could not configure the tests to pass. If anyone has more experience with cypress, I would love some help with making the tests pass. |
Also just brainstorming the data model here a little bit, it might be beneficial to migrate to a "roles" field on the user model that stores an array of roles. That way someone could be an "admin" with all permissions but someone else could be a "moderator" with read-only etc. And when they login to the Admin dashboard there can be a check of their roles array for what permissions they have |
I proposed number because we can set certain access roles. For example:
The rough idea would allow some future flexibility as well if there's other features like editing current events and such. After reading your additional comment would you prefer an array of roles instead of just a number system? |
Hey there! Welcome in! I can take a look at the test in a few days. I'm currently traveling a bit and it's hard to get time to sit down to review. |
@Caleb-Cohen If we're using the It might also be worth considering storing these security levels as strings. I don't imagine the app will ever need such fine control of user privileges to that couldn't be handled by a short list ("user", "moderator", "admin", "superadmin"). This way the values are also more expressive and less easily confused if more roles are added later. |
@cblanken I agree. It is ideal to have a |
I agree with this. Thanks for the feedback @cblanken and @guel-codes. @vguzman812 can you update PR to reflect changes? |
@vguzman812 let me know if you want to pair up on this and we can |
Already submitted the new PR a few days ago. The testing for it needs to be done if you're interested in that. I don't know enough cypress to add administrative authorization testing. |
Related Issues
Project: Admin Dashboard
Depends on:
Description
UserSchema
to have aadministrator
field.User.create
inserver/config/passport.js
to set value to 0Acceptance Criteria
The text was updated successfully, but these errors were encountered: