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

Add file submission to problem page #1933

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

alexwaeseperlman
Copy link

  • Changed the button labelled "Submit!" on the problem page to say "Go to editor"
  • Added a small form beneath the "Go to editor" button, initially hidden behind a chevron, that allows users to submit a file. I tried to make the changes as unobtrusive as possible.

Notes:

  • The file submission form goes to /problem/<problem-code>/submit, which means that error messages, for example when submitting a file with length greater than 65535, show up in the editor. This isn't amazing UX but it might be acceptable.
  • It might be worth adding a button to submit files from the editor page
  • If a user selects a file and then clicks "Go to editor", should it load that file into their editor?

@ehhthing
Copy link
Member

Perhaps it would be be much less code if this was just implemented client side with JS so there would be no need for any kind of server code changes.

@alexwaeseperlman
Copy link
Author

I added 16 lines of server side code. It's possible that using client side JS would take less but it won't be a huge difference. Unless there is a better reason to switch I think we can leave things as they are.

Copy link
Member

@Ninjaclasher Ninjaclasher left a comment

Choose a reason for hiding this comment

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

Just a general comment: I think we should move this to the Submit Solution page rather than at the bottom of the problem statement page. This centralizes the location where a user can submit a solution.

As a consequence of this, we can revert all the changes in judge/views/problem.py and revert Go to editor to Submit solution.

Also, could you run flake8 locally to fix all the linting issues?

</a>
{% endif %}
{% if request.user.is_authenticated and (not request.in_contest or submission_limit) %}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please use spaces instead of tabs.

judge/forms.py Outdated
class Meta:
model = Submission
fields = ['language']


Copy link
Member

Choose a reason for hiding this comment

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

Nit: don't delete newline.

@ehhthing
Copy link
Member

ehhthing commented May 12, 2022

I added 16 lines of server side code. It's possible that using client side JS would take less but it won't be a huge difference. Unless there is a better reason to switch I think we can leave things as they are.

The main issue is that handling file uploads on the server side is much more prone to issues than simply using a text field.

For example, your code is vulnerable to memory DoS attacks if the user uploads a very large file composed of only 0s since it decodes line-by-line. In addition, it would cause a 500 if you were to upload a file that included invalid UTF-8 because it tries to decode without a try-except.

Typically, Django does most of the validation work for you if you use a normal text field, but it provides much less built in protection against these bugs if you want to handle file uploads.

Moving the processing to the client side significantly reduces the bug surface simply because handling file uploads on the server side is nontrivial.

@alexwaeseperlman alexwaeseperlman marked this pull request as draft May 13, 2022 01:05
@alexwaeseperlman
Copy link
Author

The main issue is that handling file uploads on the server side is much more prone to issues than simply using a text field.

I removed all modifications to server side code. Thanks for looking at this.

Just a general comment: I think we should move this to the Submit Solution page rather than at the bottom of the problem statement page. This centralizes the location where a user can submit a solution.

In my opinion the main benefit of this feature is that it creates a shortcut for submitting from the problem page. I'm open to moving it around though.

Here are some screenshots of the "Submit solution"/"Go to editor" button in case anyone wants to look at the changes without setting them up:

Old:
image

New with dropdown closed:
image

New with dropdown open:
image

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