-
Notifications
You must be signed in to change notification settings - Fork 2
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
Don't allow admins to edit roster membership for other rosters #313
Conversation
app/controllers/users_controller.rb
Outdated
@@ -8,7 +8,7 @@ class UsersController < ApplicationController | |||
WHITELISTED_ATTRIBUTES = [:first_name, :last_name, :spire, :email, | |||
:phone, :active, :reminders_enabled, | |||
:change_notifications_enabled, | |||
{ rosters: [], membership: [:admin] }].freeze | |||
{ roster_ids: [], membership: [:admin] }].freeze |
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 did this because it was annoying me and I kept getting confused about whether I had rosters or ids. Turns out, most of that confusion stemmed from parse_roster_ids
, but I think this change is warranted either way.
Also, Travis failures seem legit. |
The specs are being weird, I'll see what I can do but might end up translating to request specs. |
@@ -7,6 +7,6 @@ | |||
sequence(:spire) { |n| format('%[email protected]', n) } | |||
sequence(:email) { |n| "user#{n}@umass.edu" } | |||
sequence(:phone) { |n| format('+1413545%04d', n) } | |||
rosters { [create(:roster)] } | |||
roster_ids { [create(:roster).id] } |
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.
using roster_ids now, not rosters
Co-authored-by: Matt Moretti <[email protected]>
Man, my bad. That should have been a |
I suppose that's what I get for blindly trusting |
closes #293
closes #292
I was going to clean up the create and edit actions both, but doing so would require some significant refactoring. I am still open to cleaning up both, but I wanted to make sure other people agreed it was necessary/ perhaps it could be a follow up so we can close this issue sooner.