-
Notifications
You must be signed in to change notification settings - Fork 279
Add category types and allow teams to be in more than one category #3043
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
base: main
Are you sure you want to change the base?
Conversation
4b3e640 to
ed306f7
Compare
55d3c52 to
80b7c0b
Compare
80b7c0b to
6953f19
Compare
| ->leftJoin('t.affiliation', 'ta') | ||
| ->leftJoin('t.category', 'tc') | ||
| ->leftJoin('t.categories', 'tc') | ||
| ->leftJoin('t.categories', 'tcc', Join::WITH, 'BIT_AND(tcc.types, :scoring) = :scoring') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the extra tcc, aren't you joining this to be the same category?
| ->from(Team::class, 't') | ||
| ->select('t') | ||
| ->leftJoin('t.category', 'tc') | ||
| ->leftJoin('t.categories', 'tc') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phpcs below
| {# only print legend when there's more than one category #} | ||
| {% if limitToTeamIds is null and usedCategories | length > 1 and hasDifferentCategoryColors %} | ||
| {% if limitToTeamIds is null and colorCategories | length > 0 and hasDifferentCategoryColors %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is now not in line with the code. Why do we need to print the legend with only 1 category?
| } | ||
|
|
||
| if ($filter->categories) { | ||
| // Use a new join, since we need both the other two category joins for other logic already |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you don't limit tcc so can re use it here,
| 'multiple' => true, | ||
| 'choice_label' => fn(TeamCategory $category) => $category->getName(), | ||
| 'help' => 'List of team categories that will receive medals for this contest.', | ||
| 'query_builder' => function(EntityRepository $er) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 'query_builder' => function(EntityRepository $er) { | |
| 'query_builder' => function (EntityRepository $er) { |
| ->select('c.sortorder, c.name') | ||
| ->where('c.visible = 1') | ||
| ->andWhere('BIT_AND(c.types, :scoring) = :scoring') | ||
| ->setParameter('scoring', TeamCategory::TYPE_SCORING) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably good to add an assertion in the Unit tests for the results with a CSS only category and that we don't see it in the result.
| if (in_array(TeamCategory::TYPE_SCORING, $types, true) && $sortOrder === null) { | ||
| $sortOrder = 0; | ||
| } | ||
| if ($sortOrder !== null && !in_array(TeamCategory::TYPE_SCORING, $types, true)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if ($sortOrder !== null && !in_array(TeamCategory::TYPE_SCORING, $types, true)) { | |
| if (!($sortOrder === null || in_array(TeamCategory::TYPE_SCORING, $types, true)) { |
I think we always do this, so let's keep that convention.
| } | ||
| if (isset($groupItem['css_class']) && !in_array(TeamCategory::TYPE_CSS_CLASS, $types, true)) { | ||
| $types[] = TeamCategory::TYPE_CSS_CLASS; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of those statements are checks if something is set and if we already have it in the array. I rather read that as just always adding it to the list and afterwards just cleaning the array with array_unique as it makes it easier to read IMO. So basically treating the array as a sorted set by always adding and making it unique in the end.
Alternative is to do something as $arr[TeamCategory::TYPE_SOMETHING] = bool and getting the keys afterwards if that's faster.
| * | ||
| * @param array<array{team: array{teamid: string|null, icpcid: string|null, label?: string|null, | ||
| * categoryid: string|null, name: string|null, display_name?: string, | ||
| * categoryids: string[]|null, name: string|null, display_name?: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any (good) reason why this cant be categoryids: string[]?
| } | ||
|
|
||
| /** | ||
| * @param int[] $limitToTeamIds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this also list the null?
| * @param int[] $limitToTeamIds | |
| * @param int[]|null $limitToTeamIds |
or set it to $limitToTeamIds = []
| <span class="badge text-bg-warning category-best"> | ||
| {{ score.team.category.name }} | ||
| </span> | ||
| {# TODO: how to display badges on mobile scorebaord #} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| {# TODO: how to display badges on mobile scorebaord #} | |
| {# TODO: how to display badges on mobile scoreboard #} |
| {{ score.team.category.name }} | ||
| </span> | ||
| {# TODO: how to display badges on mobile scorebaord #} | ||
| {% if false %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just comment this right?
| {{ score.team.category.name }} | ||
| </span> | ||
| {% endif %} | ||
| {% set hasBadges = score.team.badgeCategories | length > 0 %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know the reason why we didn't display those if there is only 1 category?
This fixes #2937
I added validation to make sure each team is in at most one scoring category and a config checker to make sure they are in at least one (since this is not an error but still weird).
There are some minor changes to the scoreboard compared to before:
TYPE_BADGE_TOPtype.I decided that filtering on the scoreboard should be possible with all category types, since I think this makes sense (i.e. one can filter on all teams that advanced to finals before or all teams that didn't).
I also decided that we can have categories without any type. The above mentioned example is a good use case: all teams NOT advancing to finals from EUC could be put in that category, so you can filter on it.
The PR is quite big, since it's quite a lot of changes. I split some of the work up in separate commits, but plan to squash before merging.