Conversation
prandla
left a comment
prandla
left a comment
There was a problem hiding this comment.
hi, sorry for taking this long to get around to this!
overall, we (or well, at least Luca and i) agree that this would be a good feature to have. i left some comments, some of them minor, some of them requiring more work. i could do all of these fixes myself too, though then i think someone else will need to re-review it :)
in addition to the inline comments, we will need to fix the test suite failures (and possibly add new tests, but currently we don't have a good way to write tests for AWS UI, so i think it's fine to skip that for now). also, we need a DumpUpdater and an sql migration. also, you should run Ruff on modified lines, and some places could use more type hints.
and please do let me know if you disagree with some of the proposed changes, i'm always up for some healthy debate :)
Should fix all failures except DumpImporterTest and schema_diff_test (which both likely need more work).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1621 +/- ##
==========================================
- Coverage 54.67% 54.47% -0.21%
==========================================
Files 335 336 +1
Lines 27361 27595 +234
==========================================
+ Hits 14960 15031 +71
- Misses 12401 12564 +163
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
prandla
left a comment
prandla
left a comment
There was a problem hiding this comment.
noticed a few more things when reading through the code one more time, and poking around in AWS a bit. i think after addressing these comments, and the foreign key constraint mentioned in the previous discussion, the code itself is fine. again we still need a DumpUpdater and a sql updater. i can write those myself if you'd prefer (i assume you haven't needed to deal with those features in your fork...)
|
This should address the requested changes (I also fixed a typo in the fallback page for |
* restore Contest.participations relationship * fix type hints * use sqlalchemy column objects instead of strings where possible
|
oh, it seems like there might have been a bit of a miscommunication: if we're keeping Participation.contest_id then i don't see any reason not to also keep Contest.participations as a relationship. i reverted that part of your commit; please let me know if you actually had a reason for doing it like that. also i changed Group.contest_id to be non-null, for i hope obvious reasons. and i fixed a few type hints. i just realized i can't push changes into this PR since you made it from an organization repo, not a personal one. (github is very good software......) I've pushed my changes into the |
3 participants
We've been using CMS for the German IOI selection for many years now, and we often have offsite contestants who cannot compete at the same time as the onsite contestants, e.g. because they are ill at the time of the contest or because they live in a different timezone.
If one wants to allot different time slots for the users of a contest in vanilla CMS, this would require setting
delay(and possiblyextratime) for users individually. This can get pretty inconvenient, in particular if there are multiple contestants affected. Moreover, this does not allow having one group of contestants (in the above example, the onsite contestants) compete in a fixed timeslot and others in a timeslot of their choice (USACO style).This pull request changes the DB format to introduce user groups. Participations are now assigned a group, and
start,stop,per_user_time, etc. are no longer properties of a contest, but instead of a user group. In the above example usecase, one could then simply have one group for the onsite contestants and one for offsite contestants, and one could e.g. have the first group compete at a fixed time, with the offsite group being able to (more or less) freely choose a timeslot. As a proof of concept, we have also adjusted the Italian yaml loader so that it is able to assign users to groups; old yaml configs are still valid and result in all users being assigned to the same group.This is based on code that has been in use in the German fork of CMS for over 10 years now, but has been cleaned up and updated for the latest version of CMS. Most of the original code was written by @fagu and @magula; the present version also contains contributions by @chuyang-wang-dev.