-
Notifications
You must be signed in to change notification settings - Fork 9
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
187217062 enable public submission event types #371
187217062 enable public submission event types #371
Conversation
…s-page 187153142 Implement payment in multiple currencies
187121440 materials bulk upload
adjust PerceivedComplexity limit by 1 to accommodate the adding committee boolean in conference controller
Revert to the previous setting
@@ -216,6 +216,6 @@ def event_params | |||
end | |||
|
|||
def user_params | |||
params.require(:user).permit(:email, :password, :password_confirmation, :username) | |||
params.require(:user).permit(:email, :password, :password_confirmation, :username, :is_admin) |
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.
Hmm, why is this param needed on the proposals page? This could in theory allow someone to create a user as an admin, since these params (I don't think...) aren't filtered before creating / updating users.
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.
yeah this is my mistake. fixed.
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.
Looks very good!
Just a couple minor things
- one code nit to reduce duplication / improve clarity
- we don't need to special case the text labels for admins.
@@ -122,8 +122,12 @@ def volunteer_links(event) | |||
end, ', ') | |||
end | |||
|
|||
def event_types_sentence(conference) | |||
conference.event_types.map { |et| et.title.pluralize }.to_sentence | |||
def event_types_sentence(conference, is_admin) |
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.
Hmm, I think since this method just controls the public/displayed text, we actually don't need to special case the admin features.
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.
can you explain what you mean by special case.
@@ -1,7 +1,7 @@ | |||
%p.lead | |||
- if @program.event_types.any? | |||
You can submit proposals for | |||
= "#{event_types_sentence(@conference)}." | |||
= "#{event_types_sentence(@conference, current_user&.is_admin || false)}." |
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.
Yeah, i think this won't be necessary here.
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 was using the event_types_sentence to control which event types are displayed based on whether user is admin/non-admin. Should I just move that logic into the view code instead?
Pivotal Tracker Link
What this PR does:
This pull request This pull request implements a new attribute enable_public_submission in the event_types model. This makes it so that when a new event type is created by an admin they can make it so that non admins can or cannot make proposals with this event type.
Who authored this PR?
@tiffanylamm
How should this PR be tested?
I added tests in the event types model spec as well as the application helper spec. It tests that the dropdown and description when a non admin/admin user creates a proposal they see what they should see.
Are there any complications to deploying this?
This requires a migration to add the new attribute enable_public_submission in the event_types model.
Checklist: