Skip to content
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

Reject user registration edits if comp has started #10605

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dunkOnIT
Copy link
Contributor

@dunkOnIT dunkOnIT commented Jan 12, 2025

Fixes #10513

We missed this case because we always checked edits against the registration edit deadline. Another option here could be to default the edit deadline to the comp's start date

@gregorbg
Copy link
Member

because we always checked edits against the registration edit deadline

You mean in the legacy system?

@dunkOnIT
Copy link
Contributor Author

In v2/registration checker as well

app/models/competition.rb Outdated Show resolved Hide resolved
Copy link
Member

@gregorbg gregorbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, not quite sure whether this is the right place for this check. The method name allowed is a bit ambiguous, in my mind it refers to the "legal" permission to make edits per WCRP and competition settings. It should distinguish between comps who allow edits in general, vs. competitions who opted out of edits in the first place.

The start date could also be checked elsewehere, i.e. at the call-site where the changes are being made. And then just return early instead of continuing to process.

Anyhow, feel free to argue/debate. But no matter what conclusion we reach, this PR should definitely be backed by tests. The fact that tests passed before this change and tests continue to pass after this change indicates that our existing tests are not very helpful or conclusive.

@dunkOnIT
Copy link
Contributor Author

dunkOnIT commented Jan 18, 2025

Agreed, this should absolutely be supported by tests - rare "Duncan doesn't submit tests" moment.

The start date could also be checked elsewehere, i.e. at the call-site where the changes are being made. And then just return early instead of continuing to process.

I think the Competition model is the appropriate place for this check - the method is being called from registration_checker:81, which already has a lot of logic going on - so I feel strongly that the we should be delegating the responsibility of handling the specifics of whether a comp may be edited to the comp itself.

The method name allowed is a bit ambiguous, in my mind it refers to the "legal" permission to make edits per WCRP and competition settings. It should distinguish between comps who allow edits in general, vs. competitions who opted out of edits in the first place.

EDIT: I have now changed the method name which may address all your concerns and make the yapping below unnecessary

The intention of the method - in my mind at least - is determine whether registrations may be edited for any reason. If we want to know specifically whether edits are allowed per competition settings, we can just call self.allow_registration_edits[1]. The only other reasons registrations may not be editable at the competition-level is because of the edit being made outside of an allowed date range - which is what the rest of the method is checking [essentially, the comp having already started is just another limit on when registration edits can be made].

I'm not aware of any WCRP limitations on registrations being editable at the competition-level [beyond dates] - but if you have specific examples in mind, let me know.

[1] If your concern is that allow_registration_edits and registration_edits_allowed? are very similar method names, and it isn't clear that the latter has a broader scope than the former, then I hear where you're coming from and am happy to think about alternatives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Competitor cancelled their registration after all registration/edit deadlines had passed
2 participants