Skip to content

Problem attachments #920

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

Jump to bottom
Merged
nickygerritsen merged 4 commits into from
Jan 10, 2021
Merged

Problem attachments #920

nickygerritsen merged 4 commits into from
Jan 10, 2021

Conversation

@nickygerritsen
Copy link
Member

@nickygerritsen nickygerritsen commented Nov 13, 2020

The problem package format has the notion of attachments.

During the BAPC preliminaries we got the question whether we supported this, which we didn't yet.
So I came up with a simple implementation.

Some screenshots:

Jury interface:
Screenshot 2020-11-13 at 12 13 39

Team / public problemset page:
Screenshot 2020-11-13 at 12 13 45

@nickygerritsen nickygerritsen force-pushed the problem-attachments branch 2 times, most recently from 60921c2 to d0213c2 Compare November 13, 2020 11:40
@RagnarGrootKoerkamp
Copy link
Contributor

RagnarGrootKoerkamp commented Nov 13, 2020

Nice!
Some thoughts:

  • There can be multiple/many attachments. Download as a zip for contestants may be nicer than individual downloads. Although download as a file is nice if it's only one.
  • For interactive problems (the typical case where attachments are used), download of samples should be disabled. The sample input typically contains instructions on how to run the interactor, which are not meant for contestants to see. In our case, we provide replacement samples as an attachment.

@nickygerritsen
Copy link
Member Author

nickygerritsen commented Nov 13, 2020

Nice!
Some thoughts:

  • There can be multiple/many attachments. Download as a zip for contestants may be nicer than individual downloads. Although download as a file is nice if it's only one.
  • For interactive problems (the typical case where attachments are used), download of samples should be disabled. The sample input typically contains instructions on how to run the interactor, which are not meant for contestants to see. In our case, we provide replacement samples as an attachment.

I think the latter requires something in the spec then, since that is not really a nice case. But if the samples contain stuff the team should not see then it is not a sample imho. Since you ship real samples in the zip anyway, wouldn’t marking the samples as non sample solve this?
For the former I can cook something up that if you have more than file it offers a zip. But if you do that then why not put a zip in attachments/?

@RagnarGrootKoerkamp
Copy link
Contributor

RagnarGrootKoerkamp commented Nov 13, 2020 •
edited
Loading

  • Yeah, agree that we should talk about what it means for something to be in data/samples/. Scheduled it for next meeting.
    I'd still like them to be in the samples directory, since the PDF is rendered using the sources there, but I see your point as well.

  • I'd prefer to not put a zip file in my git repository, but I see your point. I could always convert multiple files to a zip when exporting it for DOMjudge import.

@nickygerritsen
Copy link
Member Author

nickygerritsen commented Nov 13, 2020 •
edited
Loading

I wonder what the others think about the zip? Adding a “all attachments” button if there is >1 that creates a zip is trivial so we could do it

@nickygerritsen nickygerritsen mentioned this pull request Nov 16, 2020
Closed
@thijskh
Copy link
Member

thijskh commented Dec 19, 2020

How many attachments do we normally expect a problem to have? I can see why you'd want a zip file if they are many but it's already such a button fest on that page. If it's just one or two or three normally I don't see much value for the zip button.

@nickygerritsen
Copy link
Member Author

nickygerritsen commented Dec 19, 2020

@RagnarGrootKoerkamp what do you think? Do you often have more than say 3?

@RagnarGrootKoerkamp
Copy link
Contributor

RagnarGrootKoerkamp commented Dec 20, 2020 •
edited
Loading

Since Kattis provides individual download links, let's go with that. See e.g. https://open.kattis.com/problems/testscheduling.

As said before, we can always provide zip files in the attachments directory if needed.

@thijskh
Copy link
Member

thijskh commented Dec 27, 2020

So can this be merged in current form?

@nickygerritsen
Copy link
Member Author

nickygerritsen commented Dec 27, 2020

If others agree yes

meisterT
meisterT reviewed Jan 9, 2021
View reviewed changes
webapp/src/Controller/Team/ProblemController.php Outdated
* @return \Symfony\Component\HttpFoundation\Response
* @throws \Doctrine\ORM\NonUniqueResultException
*/
public function attachmentAction(int $probId, int $attachmentId)
Copy link
Member

@meisterT meisterT Jan 9, 2021

Choose a reason for hiding this comment

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

Is this just duplicated code with above? Can you dedup it somewhere?

Copy link
Member Author

@nickygerritsen nickygerritsen Jan 9, 2021

Choose a reason for hiding this comment

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

I will try this

Copy link
Member Author

@nickygerritsen nickygerritsen Jan 10, 2021

Choose a reason for hiding this comment

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

I gave it a try and removed even more duplicated code. Can you check if you agree with the approach?

meisterT
meisterT approved these changes Jan 10, 2021
View reviewed changes
webapp/src/Migrations/Version20201113094653.php Outdated
{
public function getDescription() : string
{
return '';
Copy link
Member

@meisterT meisterT Jan 10, 2021

Choose a reason for hiding this comment

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

nit: add a description

@nickygerritsen nickygerritsen merged commit 0d7e05b into master Jan 10, 2021
@nickygerritsen nickygerritsen deleted the problem-attachments branch January 10, 2021 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@meisterT meisterT meisterT approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nickygerritsen @RagnarGrootKoerkamp @thijskh @meisterT