introduce user groups #1621
introduce user groups #1621
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 :)
| primaryjoin="Group.id==Contest.main_group_id", | ||
| post_update=True) | ||
|
|
||
| # Follows the description of the fields automatically added by |
There was a problem hiding this comment.
where does this automatic field come from? i think it'd be better for consistency (and better type hints) if we had all the fields declared explicitly.
actually, the situation here is very similar to Task.active_dataset_id; you can probably crib the relationship setup from there.
|
|
||
| # Follows the description of the fields automatically added by | ||
| # SQLAlchemy. | ||
| # participations (list of Participation objects) |
There was a problem hiding this comment.
same nit about explicitly declaring relationships.
| __table_args__ = (UniqueConstraint("contest_id", "user_id"),) | ||
|
|
||
| # Group this user belongs to | ||
| group_id = Column( |
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).
| # disallow deleting the main_group of the contest | ||
| if contest.main_group_id == group.id: | ||
| self.application.service.add_notification( | ||
| make_datetime(), f"Cannot delete a contest's main group.", |
There was a problem hiding this comment.
i think this condition should also be represented in the DB schema. (or is that impossible due to some chicken-and-egg problem where you'd need to insert into both the contests and groups table at once to avoid foreign key violations?)
actually, this is the same situataion with tasks, datasets, and the active dataset (Task.active_dataset_id is nullable). seems like nobody there figured out anything better either. still, it seems like something that should be doable.
| <tr> | ||
| <td><span class="info" title="Start time of the contest for users in main group in the UTC timezone. | ||
| Example: '2015-12-31 15:00:00'."></span> | ||
| Main Group Start Time (UTC) |
There was a problem hiding this comment.
i think this part of the UI should allow setting all parameters of the main group. e.g. factor out half of templates/group.html into templates/fragments/group_parameters.html, then include that fragment here.
| <table> | ||
| <tr> | ||
| <td> | ||
| <span class="info" title="The group this contestant is part of. This determines his contest time window."></span> |
There was a problem hiding this comment.
please use gender-neutral language
| args.hashed_password is not None, args.team, | ||
| args.hidden, args.unrestricted) | ||
| args.hidden, args.unrestricted, | ||
| args.group) |
There was a problem hiding this comment.
how does this work? i don't see you declaring --group using add_argument anywhere.
also, imo it'd make sense for this script to default to the contest's main group if one isn't provided explicitly. (i think it'd be good if admins don't have to think about groups if they only have one group.)
2 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.