Skip to content
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.

Sign up for GitHub

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

Import relative contest times #2221

Closed
wants to merge 6 commits into from
Closed

Import relative contest times #2221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aee26cb
Allow relative times and accept any DateTime format
vmcj Nov 22, 2023
2b02e25
Create contest earlier to read relative time against starttime
vmcj Nov 22, 2023
0d08e13
Initialize the starttime when the string is provided
vmcj Nov 22, 2023
2f0d99d
Fixup
vmcj Nov 22, 2023
6a3e844
Also allow deactivation via API input
vmcj Nov 22, 2023
4916b07
Either a DateTime or a parsing failure
vmcj Nov 22, 2023
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
1 change: 1 addition & 0 deletions webapp/src/Entity/Contest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,7 @@ public function setStarttimeString(string $starttimeString): Contest
{
$this->starttimeString = $starttimeString;

$this->starttime = $this->getAbsoluteTime($starttimeString);
Copy link
Member

Choose a reason for hiding this comment

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

If this wasn't here to begin with, I'm wondering if there was maybe a good reason for this.

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 in general if we want the same/similar logic for the UI and API, then we should rethink where and how parsing the input is handled in a common place. It feels that this is duplicating code in the import service that's already existing in other places.

Copy link
Member Author

Choose a reason for hiding this comment

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

If this wasn't here to begin with, I'm wondering if there was maybe a good reason for this.

I had the same question, so I was planning to not merge this PR before having discussed this at the hackathon.

I think in general if we want the same/similar logic for the UI and API, then we should rethink where and how parsing the input is handled in a common place. It feels that this is duplicating code in the import service that's already existing in other places.

I agree, but than I prefer to move this one for after the release as mentioned in the commit message.

$this->setActivatetimeString($this->getActivatetimeString());
$this->setFreezetimeString($this->getFreezetimeString());
$this->setEndtimeString($this->getEndtimeString());
Expand Down
23 changes: 14 additions & 9 deletions webapp/src/Service/ImportExportService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,9 @@ public function getContestYamlData(Contest $contest, bool $includeProblems = tru
* To verify that everything works as expected the $errorMessage needs to be checked
* for parsing errors.
*/
protected function convertImportedTime(array $fields, array $data, ?string &$errorMessage = null): ?DateTimeImmutable
{
protected function convertImportedTime(
array $fields, array $data, ?string &$errorMessage = null, ?Contest $contest = null
): ?DateTimeImmutable {
$timeValue = null;
$usedField = null;
foreach ($fields as $field) {
Expand All @@ -120,9 +121,11 @@ protected function convertImportedTime(array $fields, array $data, ?string &$err
}

if (is_string($timeValue)) {
$time = date_create_from_format(DateTime::ISO8601, $timeValue) ?:
// Make sure ISO 8601 but with the T replaced with a space also works.
date_create_from_format('Y-m-d H:i:sO', $timeValue);
if ($contest) {
$timeValue = Utils::abstime($contest->getAbsoluteTime($timeValue));
}
/** @var DateTime|bool $time */
$time = new DateTime($timeValue);
} else {
/** @var DateTime|DateTimeImmutable $time */
$time = $timeValue;
Expand Down Expand Up @@ -179,8 +182,12 @@ public function importContestData(mixed $data, ?string &$errorMessage = null, st
return false;
}

$contest = new Contest();
$contest
->setStarttimeString(date_format($startTime, 'Y-m-d H:i:s e'));

// Activate time is special, it can return non empty message for parsing error or null if no field was provided
$activateTime = $this->convertImportedTime($activateTimeFields, $data, $errorMessage);
$activateTime = $this->convertImportedTime($activateTimeFields, $data, $errorMessage, contest: $contest);
if ($errorMessage) {
return false;
} elseif (!$activateTime) {
Expand All @@ -190,12 +197,11 @@ public function importContestData(mixed $data, ?string &$errorMessage = null, st
}
}

$deactivateTime = $this->convertImportedTime($deactivateTimeFields, $data, $errorMessage);
$deactivateTime = $this->convertImportedTime($deactivateTimeFields, $data, $errorMessage, contest: $contest);
if ($errorMessage) {
return false;
}

$contest = new Contest();
$contest
->setName($data['name'] ?? $data['formal_name'] )
->setShortname(preg_replace(
Expand All @@ -205,7 +211,6 @@ public function importContestData(mixed $data, ?string &$errorMessage = null, st
))
->setExternalid($contest->getShortname())
->setWarningMessage($data['warning-message'] ?? null)
->setStarttimeString(date_format($startTime, 'Y-m-d H:i:s e'))
->setActivatetimeString(date_format($activateTime, 'Y-m-d H:i:s e'))
->setEndtimeString(sprintf('+%s', $data['duration']));
if ($deactivateTime) {
Expand Down
Loading