Conversation
prandla
requested changes
Feb 11, 2026
Member
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 :)
cms/db/user.py
Outdated
| __table_args__ = (UniqueConstraint("contest_id", "user_id"),) | ||
|
|
||
| # Group this user belongs to | ||
| group_id = Column( |
Member
There was a problem hiding this comment.
you added this group_id column, but there is still the participation.contest column. this seems like a normalization violation to me. it seems we should delete participation.contest and use participation.group.contest instead (or possibly add an @property such that participation.contest is a shorthand for participation.group.contest).
Author
There was a problem hiding this comment.
This is actually somewhat tricky. Right now, there is a UniquenessConstraint on the pair (user_id, contest_id) in the Participation table to ensure that each user has at most one participation per contest. As far as I understand, SQL would not allow a UniquenessConstraint on the pair (user_id, group.contest_id) since this involves more than one table. As we probably still want to enforce the UniquenessConstraint, my suggestion would be to leave the contest_id and contest columns for Participation (Contest.participations can instead be replaced by a @property as suggested) and to use a ForeignKeyConstraint to ensure that (group_id, contest_id) for Participation matches with (id, contest_id) for Group. This way, we would at least ensure that the data is consistent, although one would still have to set contest_id manually.
Member
There was a problem hiding this comment.
oh, i forgot about that unique constraint. in that case yeah, keeping contest_id makes sense.
enforcing it with a foreign key is definitely good enough. i'm not worried about insertion convenience here, just data integrity.
Should fix all failures except DumpImporterTest and schema_diff_test (which both likely need more work).
❌ 6 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
prandla
reviewed
Feb 15, 2026
prandla
requested changes
Feb 15, 2026
Member
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...)
|
|
||
| from sqlalchemy.dialects.postgresql import ARRAY, CIDR | ||
| from sqlalchemy.orm import relationship | ||
| from sqlalchemy.orm import backref, relationship |
| Integer, | ||
| ForeignKey(Contest.id, | ||
| onupdate="CASCADE", ondelete="CASCADE"), | ||
| # nullable=False, |
Member
There was a problem hiding this comment.
we have an explicit nullable=True or nullable=False on every column, so why not have it here too?
(i don't know who initially did this, but i like it being explicit because then i don't have to remember what sqlalchemy's default is :) )
| foreign_keys=[contest_id], | ||
| back_populates="groups") | ||
|
|
||
| def phase(self, timestamp: datetime) -> int: |
Member
There was a problem hiding this comment.
it looks like you copied an older version of phase() from contest.py to here. i'd prefer keeping the comment i added to it.
| try: | ||
| contest_id: str = self.get_argument("contest_id") | ||
| assert contest_id != "null", "Please select a valid contest" | ||
| group_id: str = self.get_argument("group_id") |
Member
There was a problem hiding this comment.
It looks like you forgot to update admin/templates/user.html; there's still just a "select contest" dropdown there, which only sets contest_id and thus breaks this endpoint.
| attrs["contest"] = self.safe_get_item(Contest, contest_id) | ||
|
|
||
| # Create the group. | ||
| group = Group(**attrs) |
Member
There was a problem hiding this comment.
I'd prefer if this handler copied the attributes from the contest's main group, instead of using the hardcoded defaults.
|
|
||
| self.get_string(attrs, "name", empty=None) | ||
|
|
||
| self.get_datetime(attrs, "start") |
Member
There was a problem hiding this comment.
i'd really like if this code wasn't duplicated with ContestHandler.post(). There's already some differences between them; namely this version doesn't have explicit checks for "please set start/stop time" (i'm not sure if those are strictly necessary or if it's just to have a nicer error than the default IntegrityError, but in any case, there's no reason for these to be different).
something like get_group_settings(handler: BaseHandler, group: Group) which can then be called as get_group_settings(self, group) here and as get_group_settings(self, contest.main_group) in ContestHandler.
| team_code: str | None, | ||
| hidden: bool, | ||
| unrestricted: bool, | ||
| groupname: str |
Member
There was a problem hiding this comment.
either this can be None, in which case the hint should be str | None, or it can't, in which case the if groupname is None check is unnecessary. (given all the logic in main() i assume groupname can't be None actually.)
|
|
||
| # Groups | ||
| main_group_name: str | None = load(conf, None, "main_group") | ||
| groups: Group | None = load(conf, None, "groups") |
Member
There was a problem hiding this comment.
this isn't the right type hint (it's a dict representing group settings, not the actual group object)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.