Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
20 changes: 20 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 Expand Up @@ -297,6 +309,14 @@ protected function getQueryBuilder(Request $request): QueryBuilder
}
}

// For non-API-reader users, only expose the problems after the contest has started.
// `WF Access Policy` allows for clarifications before the contest, but not to disclose the problem
// so referencing them in clarifications would violate referential integrity.
if (!$this->dj->checkrole('api_reader')) {
$queryBuilder->andWhere('c.starttime < :now OR clar.problem IS NULL')
->setParameter('now', Utils::now());
}

if ($request->query->has('problem')) {
$queryBuilder
->andWhere('clar.problem = :problem')
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
14 changes: 8 additions & 6 deletions webapp/src/Controller/Team/MiscController.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
use App\Controller\BaseController;
use App\DataTransferObject\SubmissionRestriction;
use App\Entity\Clarification;
use App\Entity\Language;
use App\Form\Type\PrintType;
use App\Service\ConfigurationService;
use App\Service\DOMJudgeService;
use App\Service\EventLogService;
use App\Service\ScoreboardService;
use App\Service\SubmissionService;
use App\Utils\Utils;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\NonUniqueResultException;
use Doctrine\ORM\NoResultException;
Expand Down Expand Up @@ -92,8 +92,7 @@ public function homeAction(Request $request): Response
paginated: false
)[0];

/** @var Clarification[] $clarifications */
$clarifications = $this->em->createQueryBuilder()
$qb = $this->em->createQueryBuilder()
->from(Clarification::class, 'c')
->leftJoin('c.problem', 'p')
->leftJoin('c.sender', 's')
Expand All @@ -105,9 +104,12 @@ public function homeAction(Request $request): Response
->setParameter('contest', $contest)
->setParameter('team', $team)
->addOrderBy('c.submittime', 'DESC')
->addOrderBy('c.clarid', 'DESC')
->getQuery()
->getResult();
->addOrderBy('c.clarid', 'DESC');
if (!$this->dj->checkrole('jury') && $contest->getStartTimeObject()->getTimestamp() > time()) {
$qb->andWhere('c.problem IS NULL');
}
/** @var Clarification[] $clarifications */
$clarifications = $qb->getQuery()->getResult();

/** @var Clarification[] $clarificationRequests */
$clarificationRequests = $this->em->createQueryBuilder()
Expand Down
5 changes: 4 additions & 1 deletion webapp/src/Twig/TwigExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Member

Choose a reason for hiding this comment

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

wonder if this makes sense in general and not just here?

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 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
<tr class="{% if team.unreadClarifications.contains(clarification) %}unseen{% endif %}">
<td>
<a data-ajax-modal data-ajax-modal-after="markSeen" href="{{ link }}">
{{ clarification.submittime | printtime(null, clarification.contest) }}
{{ clarification.submittime | printtime(null, clarification.contest, true) }}
</a>
</td>
{%- if clarification.sender is null %}
Expand Down
22 changes: 22 additions & 0 deletions webapp/templates/team/partials/clarifications_team.html.twig
Copy link
Member

Choose a reason for hiding this comment

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

I would rather reject clar requests before contest start through the API

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I want the jury to still be able to send them via API because of imports

Do we have a way to import clarifications through problem upload or what do you mean by "imports" here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think can I go to the toilet is always an important one. But I'm fine with either way.. either the extra branch in the UI or the extra branch in the API.

Do we have a way to import clarifications through problem upload or what do you mean by "imports" here?

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>

23 changes: 4 additions & 19 deletions webapp/templates/team/partials/index_content.html.twig
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
<h2 id="contestnotstarted" class="text-center">
Contest {{ contest | printContestStart }}
</h2>

{% include 'team/partials/clarifications_team.html.twig' with {'always_display': false} %}
{% else %}

<div id="teamscoresummary">
Expand All @@ -26,26 +28,9 @@

{% include 'team/partials/submission_list.html.twig' %}
</div>
<div class="col">
<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 %}

<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>
<div class="col">
{% include 'team/partials/clarifications_team.html.twig' with {'always_display': true} %}
</div>
</div>
{% endif %}
Expand Down
Loading