-
Notifications
You must be signed in to change notification settings - Fork 186
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
Add a form for subscribing to competitions. #5193
Conversation
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.
Just came across this so I've left a couple of comments on stuff that caught my eyes while reading through it.
I didn't test it yet, sorry :s
@@ -167,6 +171,8 @@ def update | |||
user: @user.delegate_to_handle_wca_id_claim.name) | |||
WcaIdClaimMailer.notify_delegate_of_wca_id_claim(@user).deliver_later | |||
redirect_to profile_claim_wca_id_path | |||
elsif user_params.key? :subscriptions_attributes | |||
redirect_to subscriptions_url |
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.
Since your doing a whole new view, I think it's cleaner to have a separate controller action (eg: update_subscriptions
, that filters only these attributes in the params, and updates only the users' subscriptions).
Technically it could be possible to craft, from the subscriptions page, a post request that updates other data from the user.
@@ -812,10 +814,11 @@ def editable_fields_of_user(user) | |||
end | |||
if user == self | |||
fields += %i( | |||
password password_confirmation | |||
email preferred_events results_notifications_enabled | |||
current_password password password_confirmation |
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.
Woops, pretty sure that's something that slept through a rebase (we recently changed our way to check for user's recent authentication in #5071).
`updated_at` datetime NOT NULL, | ||
PRIMARY KEY (`id`), | ||
KEY `index_subscriptions_on_user_id` (`user_id`) | ||
) ENGINE=InnoDB DEFAULT CHARSET=latin1; |
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.
Note for later: our charset is utf8mb4 and the collate needs to be specified too.
What goes in there depends on how the server is configured, so if needed I'm pretty sure that someone with a production-like mysql setup can run the migration for you and update the diff.
Suggestion: Let's add another criteria with Delegate. In most areas, competitions are organised by a few delegates. So, I would want to be notified if my local delegate is organising a competition. |
I'm going to close this due to inactivity, feel free to resubmit! |
This is a work in progress! The form is done + working, though I'm still planning to do a cleanup pass. The part that emails users isn't started yet. No tests yet.
Not looking for detailed code review yet, but if anyone has high-level feedback or usability feedback I'd love to hear it!
Fixes #114
Example subscription: all competitions within 500km of Boston

Example subscription: all world + continental championships

Example subscription: all North America competitions with FMC

Known issue: "All regions" should probably say "North America" instead.