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.

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

Conversation

vmcj
Copy link
Member

@vmcj vmcj commented Nov 22, 2023

This targets #2072

This has one slight flaw, the relative time is changed to an absolute time and stored as such. I'm not sure if there is a proper fix for this.

Also side-effect to test if the CI now works again.

vmcj added 6 commits November 22, 2023 19:23
We already accept this for the UI, now also accept this in the API. This
means we read more lenient than CLICS requires us for the API.
This is a candidate to refactor to a more generic solution but as the
startTime is closely related to the starttime it makes sense to not
change this just before a planned release.
This makes sure the 2 are in sync.
@@ -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.

@meisterT
Copy link
Member

This has one slight flaw, the relative time is changed to an absolute time and stored as such. I'm not sure if there is a proper fix for this.

That we store the time string as is (i.e. as relative time) and present it back to the user is intentional. If we move the contest start, and the user entered relative times, the respective other times need to move as well.

@vmcj
Copy link
Member Author

vmcj commented Nov 23, 2023

This has one slight flaw, the relative time is changed to an absolute time and stored as such. I'm not sure if there is a proper fix for this.

That we store the time string as is (i.e. as relative time) and present it back to the user is intentional. If we move the contest start, and the user entered relative times, the respective other times need to move as well.

Than this shouldn't be merged and requires some/loads of rework.

@vmcj
Copy link
Member Author

vmcj commented Nov 24, 2023

Not possible in current format.

@vmcj vmcj closed this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants