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

Rename registration_opened? to registration_open? #9709

Merged
merged 15 commits into from
Aug 7, 2024

Conversation

dunkOnIT
Copy link
Contributor

This has been requested as a pre-requisite for auto-accepts. Basically, in some regions, organizers/delegates don't currently pre-register for their competitions because they don't want to show up on the Competitor list before the competition actually opens (causing confusion for competitors/generally looking a bit weird.

So, I've suggested that we only show the "Competitors" tab when registration for a competition is actually open.

Implementation

The flaw in this implementation is that if registration closes, and has another registration_open date set, then until registration opens again, the Competitor list will not be visible.

To overcome this, we could add a boolean to the competition table which tracks whether registration has opened at least once. But that would be yet another setting in the competition table, which isn't ideal. I'll ask WCAT about the relevance of this edge case.

@dunkOnIT dunkOnIT changed the title Hide Competitor tab before registration opens Minor changes to display of Register and Competitors tabs Aug 1, 2024
app/views/competitions/_nav.html.erb 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.

As much as I agree about the confusing logic of the competition method names, I'm really afraid about this change because it
(a) renames an existing method and
(b) introduces a new method that is named exactly like the old one

where especially point (b) is dangerous. Some parts of the code that you did not catch might still reference the old registration_opened? implementation which now "accidentally" gets rerouted to a semantically different implementation.

I'd feel much more comfortable if you split htis up into a separate PR that only does renames (basically, a glorified version of point (a) above, maybe also considering the nomenclature of other registration_foo? methods) and then after that passes and can be merged, pick this up again with only an implementation for (b)

app/models/competition.rb Outdated Show resolved Hide resolved
app/models/competition.rb Show resolved Hide resolved
@dunkOnIT dunkOnIT changed the title Minor changes to display of Register and Competitors tabs Rename registration_opened? to registration_open? Aug 7, 2024
@dunkOnIT
Copy link
Contributor Author

dunkOnIT commented Aug 7, 2024

Ok this is ready to review again - I will add a method to check if registration has been open in a follow up PR

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.

For the sake of what's to come, might I suggest registration_currently_open??

@dunkOnIT
Copy link
Contributor Author

dunkOnIT commented Aug 7, 2024

I was 50/50 between this and registration_currently_open - so that swings it.

@gregorbg gregorbg merged commit 871d97e into thewca:main Aug 7, 2024
1 check passed
@dunkOnIT dunkOnIT deleted the nit/hide-competitor-list-before-open branch August 7, 2024 14:30
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.

2 participants