Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions webapp/src/Controller/API/ClarificationController.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use App\Entity\ContestProblem;
use App\Entity\Team;
use App\Utils\Utils;
use DateTime;
use Doctrine\ORM\NonUniqueResultException;
use Doctrine\ORM\QueryBuilder;
use Exception;
Expand Down Expand Up @@ -208,6 +209,17 @@ public function addAction(
} else {
throw new BadRequestHttpException('A team can not assign time.');
}
} elseif ($this->isGranted('ROLE_API_WRITER') && $contest->getStartTime() > $time) {
$startTime = $contest->getStarttimeString();
$startTimeTimeZone = DateTime::createFromFormat('Y-m-d h:i:s e', $startTime)->getTimezone();
$now = DateTime::createFromFormat('U.u', (string) $time);
$now->setTimezone($startTimeTimeZone); // We can't the timezone in createFromFormat as it always picks UTC.
throw new BadRequestHttpException(
"Sending a clarification before the contest can disclose restricted information, "
. "provide an explicit time when this clarification should be visible. "
. "For the start of this contest: " . $contest->getStarttimeString() . ", "
. "for the current time: " . $now->format('Y-m-d H:i:s e') . "."
);
}

$clarification->setSubmittime($time);
Expand Down
19 changes: 19 additions & 0 deletions webapp/src/Controller/Jury/ClarificationController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not just generic, right?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Which ones would you approve? I really like to close this PR soon as getting merge conflicts here is risky.

if ($cc && $cc->getStartTime() > Utils::now()) {
$this->addFlash('warning', $message);
} 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()];
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -239,6 +257,7 @@ public function composeClarificationAction(
#[MapQueryParameter]
?string $teamto = null,
): Response {
$this->warnClarificationBeforeContestStart();
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Sounds reasonable, I'll spend the time on it.

$formData = ['recipient' => JuryClarificationType::RECIPIENT_MUST_SELECT];

if ($teamto !== null) {
Expand Down
4 changes: 4 additions & 0 deletions webapp/templates/team/partials/index_content.html.twig
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
<h2 id="contestnotstarted" class="text-center">
Contest {{ contest | printContestStart }}
</h2>
{% if clarifications is not empty %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking a little bit more: when do we send out "static events" in the event feed? Do we do that on state changes? Then hiding them in the API is perhaps not too much code complexity, but one would have to implement it to really see.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I understand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably best described in person - @nickygerritsen, is my understanding correct that static events are created on state changes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarifications are not static events. But we send them out here, so when a dependency is missing basically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, needs more thought

<h1 class="teamoverview">Clarifications</h1>
{% include 'team/partials/clarification_list.html.twig' with {clarifications: clarifications} %}
{% endif %}
{% else %}

<div id="teamscoresummary">
Expand Down