-
Notifications
You must be signed in to change notification settings - Fork 279
Handle pre-contest clarifications #3174
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?
Changes from all commits
0071234
d54c96e
4706e75
1f77c9a
8342cf2
c8c97ec
3a60f88
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,13 +34,30 @@ public function __construct( | |
| protected readonly EventLogService $eventLogService | ||
| ) {} | ||
|
|
||
| private function warnClarificationBeforeContestStart(): void | ||
| { | ||
| $cc = $this->dj->getCurrentContest(); | ||
| $message = "Generic clarifications are visible before contest start."; | ||
| if ($cc && $cc->getStartTime() > Utils::now()) { | ||
| $this->addFlash('warning', $message); | ||
meisterT marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } elseif (!$cc) { | ||
| foreach ($this->dj->getCurrentContests() as $cc) { | ||
| if ($cc->getStartTime() > Utils::now()) { | ||
| $this->addFlash('warning', $message); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[Route(path: '', name: 'jury_clarifications')] | ||
| public function indexAction( | ||
| #[MapQueryParameter(name: 'filter')] | ||
| ?string $currentFilter = null, | ||
| #[MapQueryParameter(name: 'queue')] | ||
| string $currentQueue = 'all', | ||
| ): Response { | ||
| $this->warnClarificationBeforeContestStart(); | ||
| $categories = $this->config->get('clar_categories'); | ||
| if ($contest = $this->dj->getCurrentContest()) { | ||
| $contestIds = [$contest->getCid()]; | ||
|
|
@@ -116,6 +133,7 @@ public function indexAction( | |
| #[Route(path: '/{id<\d+>}', name: 'jury_clarification')] | ||
| public function viewAction(Request $request, int $id): Response | ||
| { | ||
| $this->warnClarificationBeforeContestStart(); | ||
| $clarification = $this->em->getRepository(Clarification::class)->find($id); | ||
| if (!$clarification) { | ||
| throw new NotFoundHttpException(sprintf('Clarification with ID %s not found', $id)); | ||
|
|
@@ -239,6 +257,7 @@ public function composeClarificationAction( | |
| #[MapQueryParameter] | ||
| ?string $teamto = null, | ||
| ): Response { | ||
| $this->warnClarificationBeforeContestStart(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in addition, I was thinking whether we should add a modal dialog on submission warning of the implications There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like this for 2 reasons... modals are more work and in case you know what you're doing this feels a little patronizing. You have more experience with judges so if you think it's smarter I can take a look how much work it is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This a very edge case with some consequences, so yes, I would prefer to put into the face of the jury. Under normal operation, nobody should see the modal dialog. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable, I'll spend the time on it. |
||
| $formData = ['recipient' => JuryClarificationType::RECIPIENT_MUST_SELECT]; | ||
|
|
||
| if ($teamto !== null) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -205,13 +205,16 @@ public function printelapsedminutes(float $start, float $end): string | |
| * Print a time formatted as specified. The format is according to date(). | ||
| * @param Contest|null $contest If given, print time relative to that contest start. | ||
| */ | ||
| public function printtime(string|float|null $datetime, ?string $format = null, ?Contest $contest = null): string | ||
| public function printtime(string|float|null $datetime, ?string $format = null, ?Contest $contest = null, bool $squash = false): string | ||
| { | ||
| if ($datetime === null) { | ||
| $datetime = Utils::now(); | ||
| } | ||
| if ($contest !== null && $this->config->get('show_relative_time')) { | ||
| $relativeTime = $contest->getContestTime((float)$datetime); | ||
| if ($relativeTime < 0 && $squash) { | ||
| return "Before contest"; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wonder if this makes sense in general and not just here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should just display it in the table, I didn't find other locations yet (also didn't really search) but in general I agree. Probably best to move this commit out and do this in a separate PR to find the other locations. |
||
| } | ||
| $sign = ($relativeTime < 0 ? -1 : 1); | ||
| $relativeTime *= $sign; | ||
| // We're not showing seconds, while the last minute before | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would rather reject clar requests before contest start through the API There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So instead of letting teams ask them via UI (because they can via API) disallow both via UI & API? I think I'm fine with that, I want the jury to still be able to send them via API because of imports and suspected we didn't want the extra branch. I did not really like having to add this to the UI but found consistency important. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, there is no point in asking for clarification for a contest where no data is supposed to be known yet to teams.
Do we have a way to import clarifications through problem upload or what do you mean by "imports" here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think
I'm not sure if we have it yet, but I think it's something we should have. In BAPC prelims (multisite over different days) the jury now updates the PDF. But I can imagine a year where we would just also upload the clars directly in case as they don't have the time. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| {% if clarifications is not empty or always_display %} | ||
| <h1 class="teamoverview">Clarifications</h1> | ||
| {% if clarifications is empty %} | ||
| <p class="nodata">No clarifications.</p> | ||
| {% else %} | ||
| {% include 'team/partials/clarification_list.html.twig' with {clarifications: clarifications} %} | ||
| {% endif %} | ||
| {% endif %} | ||
|
|
||
| <h1 class="teamoverview">Clarification Requests</h1> | ||
| {% if clarificationRequests is empty %} | ||
| <p class="nodata">No clarification request.</p> | ||
| {% else %} | ||
| {% include 'team/partials/clarification_list.html.twig' with {clarifications: clarificationRequests} %} | ||
| {% endif %} | ||
|
|
||
| <div class="m-1"> | ||
| <a href="{{ path('team_clarification_add') }}" class="btn btn-secondary btn-sm" data-ajax-modal data-ajax-modal-after="initModalClarificationPreviewAdd"> | ||
| Request clarification | ||
| </a> | ||
| </div> | ||
|
|
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.
not just generic, right?
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.
In the later commits I don't disclose clarifications about problems, I know you guys had opinions on this but I think we should try the referential integrity. If we don't like the extra code path for maintainability we can discuss it now and change this message.
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 this PR does many things at once, some of which can be merged easily and maybe should be lifted to their own PRs so that they can be merged 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.
Which ones would you approve? I really like to close this PR soon as getting merge conflicts here is risky.