-
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
Auto-close registrations after enough paid registrations #10772
base: main
Are you sure you want to change the base?
Conversation
db/migrate/20250204104514_add_auto_close_to_competitions_table.rb
Outdated
Show resolved
Hide resolved
…/worldcubeassociation.org into reg-feature/close-automatically
Blocked pending a change to allow setting competition form fields to |
return false if auto_close_threshold.nil? | ||
threshold_reached = registrations.with_payments.count >= auto_close_threshold && auto_close_threshold > 0 | ||
# update!(closing_full_registration: true, registration_close: Time.now) if threshold_reached | ||
threshold_reached && update!(closing_full_registration: true, registration_close: Time.now) |
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.
The !
version of update!
has a (potentially) dangerous implication: If, for some reasons, the validations for the update
payload (the updated attributes) fail, it will throw a hard error.
And since this method is being called in the after_create
hook of registration_payment
, this would result in the creating transaction for that payment to be halted and rolled back. So if push came to shove, people could be unable to pay because of an oversight in our tests / edge cases that we haven't considered.
Fun fact: You have tests which try for both return values (true
and false
) but the false
only ever happens when the first condition threshold_reached
falsifies the entire &&
. In reality, the signature is like "true or throw" more than an actually legitimate "true or 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.
TL;DR:
threshold_reached && update!(closing_full_registration: true, registration_close: Time.now) | |
threshold_reached && update(closing_full_registration: true, registration_close: Time.now) |
No description provided.