Asap 173 admin user UI #222
Conversation
| env: | ||
| RAILS_ENV: test | ||
| DATABASE_URL: postgres://postgres:postgres@localhost:5432/access_pdf_test | ||
| run: bundle exec rspec | ||
| run: bundle exec rspec spec/models spec/requests |
There was a problem hiding this comment.
I'm trying to see if splitting up our JS and non-JS tests into separate steps improves the flaps. Sometimes it's best to not run them all at once.
| @@ -5,7 +5,9 @@ export default class extends Controller { | |||
|
|
|||
| connect() { | |||
| super.connect(); | |||
| this.wrapperTarget.addEventListener('close', this.onModalClose.bind(this)) | |||
| if (this.hasWrapperTarget) { | |||
There was a problem hiding this comment.
This fixes a random javascript error we've had for quite a while now.
There was a problem hiding this comment.
Left a few wording suggestions and also had a few meta questions about the distinctions between user admins and site admins that I'd be happy to chat about more.
Maybe on more high-level comments: Will current users of our demo site need to be added by an admin manually after this is deployed? Or will current users' emails and passwords be automatically added? I saw the DB migration for admin users but didn't see anything for current users.
| <%= @user.errors.full_message(:password_confirmation, @user.errors[:password_confirmation].first) %> | ||
| </div> | ||
| <% end %> | ||
| </div> |
There was a problem hiding this comment.
I'm not actually sure this comment is relevant to this code block, but I think it might be clearer if the UI refers to passwords as "current" and "new". In other words, rename "Password" to "New password" and "Password confirmation" to "New password confirmation".
There was a problem hiding this comment.
Also noting this language is a bit awkward when the user's password confirmation doesn't match the new password. I think renaming to "new password" will help but maybe also if you can lowercase it?
There was a problem hiding this comment.
Those seem like good changes to me. What do you think about this labeling and error combination?
There was a problem hiding this comment.
Yeah I like this better! Thank you!
| </div> | ||
| <% end %> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Getting funny behavior here. I thought I needed to supply the current password on a user to edit the checkboxes, but maybe I don't? I was logged in as the listed user, which had user admin access.
Screen.Recording.2025-07-16.at.1.50.49.PM.mov
There was a problem hiding this comment.
I think you only have to supply the current password if you are changing the password. This is a common security pattern to prevent cross site request forgery attacks, amongst other evils. Maybe we should make that more obvious?
There was a problem hiding this comment.
Yeah, maybe just "We need your current password to confirm any password changes"? I think this error doesn't happen if you just select / unselect the admin boxes without supplying the current password.
There was a problem hiding this comment.
Good idea! I did this in ef1cdd1.
When I setup Devise on the previous branch, I set it to use our existing user table for the "account" record. So the migrations have all been focused on adding or renaming fields to that table. Everyone should already be in the system and should have the same email, password and admin status. Or at least that's my goal. We will need to manually add the |
We need an interface to add and manage users. This includes a listing and the ability to add/edit users.
This PR adds the follow:
is_adminflag into ais_site_adminandis_user_adminflag with separate permissions.Here are a few screenshots:
Admin user list:
Admin add/edit form:
This branch will require a schema update:
rails db:drop ; rails db:migrate ; rails db:setupTry adding/editing users from the admin user listing.
No