-
Notifications
You must be signed in to change notification settings - Fork 278
Use external ID in UI everywhere instead of internal database ID #3263
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?
Conversation
…internal IDs Part of DOMjudge#3024
Part of DOMjudge#3024 # Conflicts: # webapp/templates/jury/partials/submission_diff.html.twig # webapp/templates/jury/submission_source.html.twig
…nstead of internal IDs Part of DOMjudge#3024
Part of DOMjudge#3024 � Conflicts: � judge/judgedaemon.main.php
| private function judgingDirectory(string $workdirpath, array $judgeTask): string | ||
| { | ||
| if (filter_var($judgeTask['submitid'], FILTER_VALIDATE_INT) === false || | ||
| if (filter_var($judgeTask['submitid'], FILTER_VALIDATE_REGEXP, ['options' => ['regexp' => '/^[a-zA-Z0-9_.-]+$/']]) === false || |
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.
should we define this somewhere more generally?
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.
Also, what do submission URLs look like after this change?
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.
What are you suggesting for generally defining this? The judgedaemon doesn't have access to the Symfony code.
As for urls, it's https://domjudge/jury/submissions/[id] still, but the [id] can be a string (although if we generate submission ID's they will actually be numbers, only when external systems generate them they will be strings).
| $this->addSql('ALTER TABLE version ADD langid_str VARCHAR(32) DEFAULT NULL AFTER langid'); | ||
|
|
||
| $this->addSql("UPDATE language SET langid_str = CASE externalid | ||
| WHEN 'ada' THEN 'adb' |
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.
huh, ada -> adb? Is that what you want?
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.
Yes, we used adb as langid for ADA files, but the CLICS spec uses ada.
| $data['subjectlink'] = $this->generateUrl('jury_problem', ['probId' => $clar->getProblem()->getExternalid()]); | ||
| } elseif ($clar->getCategory()) { | ||
| $concernssubject = $contest->getCid() . "-" . $clar->getCategory(); | ||
| $concernssubject = $contest->getExternalid() . "#" . $clar->getCategory(); |
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.
why not also use |?
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.
Because then you can't have a problem with the same ID as a clarification category
|
|
||
| #[IsGranted('ROLE_ADMIN')] | ||
| #[Route(path: '/delete-multiple', name: 'jury_team_affiliation_delete_multiple', methods: ['GET', 'POST'])] | ||
| #[Route(path: '/delete-multiple', name: 'jury_team_affiliation_delete_multiple', methods: ['GET', 'POST'], priority: 1)] |
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.
what does priority: 1 mean/do here?
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.
It makes it that jury/affiliations/delete-multiple goes to this route and not suggests you want to view the affiliation called delete-multiple. In the past we reordered the methods to have this come earlier than the view action, but I like this way more.
| * | ||
| * @return array{previous: string|int|null, next: string|int|null} | ||
| */ | ||
| protected function getPreviousAndNextObjectIds( |
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.
Is it easy to add a few unit tests for this method?
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 will try, I think that makes sense.
| <tbody> | ||
| {% for p in problems %} | ||
| {% set id=p.probid %} | ||
| {% set id=p.externalId %} |
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.
looking at all these places, should we just use p.id or p.probid to mean the external id and p.internalId to mean the probid? Could be done separately though
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 wonder if that is better. It will have less changes in the twigs but then have other changes instead. Not sure I actually like it, but we can try it separately indeed.
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.
This might deserve a changelog entry :-)
Fixes #3024