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.

Sign up for GitHub

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

Jump to bottom

Check schedule and events/rounds match before confirming a competition #3207

Merged
merged 3 commits into from
Jan 3, 2019
Merged

Check schedule and events/rounds match before confirming a competition #3207

merged 3 commits into from
Jan 3, 2019

Conversation

viroulep
Copy link
Contributor

@viroulep viroulep commented Aug 10, 2018 •
edited
Loading

Fixes #3574.
⚠️ This will effectively prevent organizers/Delegates from confirming a competition before they have created a valid schedule in the "Manage Schedule" page.
It should not be merged before #3018 is merged, and we should wait for when the Board is ready to enforce this.

The validation is triggered only when isConfirmed changes, for two reasons:

  • it's fine if old competitions do not have a schedule on the WCA website
  • for announced competitions, organizers/Delegates may add/remove rounds before the start of the competition. As we don't have a unified events+schedule editor, running the validation would fail when they add/remove a round before adjusting the schedule. Note that the "warning" alert would still be displayed in this case, to remind the organizers/Delegates that something needs to be done about it.

@cubewhiz
Copy link
Member

Are there any logistical challenges with implementing this? How are tentative events handled?

@viroulep
Copy link
Contributor Author

Tentative events are not supported currently, which seems to be the main issue as a few Delegates would like to have them.

@jfly jfly mentioned this pull request Sep 3, 2018
Open
@viroulep viroulep force-pushed the enforce-competition-schedule branch 4 times, most recently from 0446c25 to 843b529 Compare November 9, 2018 15:04
@viroulep viroulep mentioned this pull request Nov 9, 2018
Closed
@UnsolvedCypher
Copy link
Contributor

It seems that the schedule has been in regular use for some time now, is this ready to be merged?

@cubewhiz
Copy link
Member

I think we are waiting for WRT to confirm that they are ready for this.

@SAuroux
Copy link
Member

SAuroux commented Dec 19, 2018

Uhm, no, I don't think so. Unless I am missing something this PR is unrelated to #3127 .

@viroulep
Copy link
Contributor Author

I didn't have any review from the WST though :(
@jfly, @jonatanklosko: would you have time to review somehow?

jonatanklosko
jonatanklosko approved these changes Dec 19, 2018
View reviewed changes
Copy link
Member

@jonatanklosko jonatanklosko left a comment

Choose a reason for hiding this comment

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

One comment from me, LGTM! Exciting step forward 🚢

WcaOnRails/app/models/schedule_activity.rb Show resolved Hide resolved
@cubewhiz
Copy link
Member

Sorry @SAuroux someone mentioned this PR in the email thread and I misinterpreted one of the responses to be related to this PR instead of the original.

@jonatanklosko
Copy link
Member

Actually, I've just realized that it would also make sense to require at least one round for each declared event. Does it sound right?

@DanielEgdal DanielEgdal mentioned this pull request Dec 30, 2018
Open
11 tasks
@viroulep viroulep force-pushed the enforce-competition-schedule branch from 843b529 to e53f825 Compare January 3, 2019 15:35
@viroulep
Copy link
Contributor Author

viroulep commented Jan 3, 2019

Just added the test for at least one round per event!

@viroulep viroulep force-pushed the enforce-competition-schedule branch from e53f825 to e9a2890 Compare January 3, 2019 15:47
@jonatanklosko
Copy link
Member

Great, I believe it's ready to go! 🚢 :shipit:

@viroulep viroulep merged commit f8685a5 into thewca:master Jan 3, 2019
@viroulep viroulep deleted the enforce-competition-schedule branch January 3, 2019 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonatanklosko jonatanklosko jonatanklosko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@viroulep @cubewhiz @UnsolvedCypher @SAuroux @jonatanklosko