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

Allow Zip Files to be Uploaded as Attachment #22670

Merged
merged 11 commits into from
Sep 23, 2024

Conversation

chrstinalin
Copy link
Contributor

@chrstinalin chrstinalin commented Sep 16, 2024

Fixes: mozilla/addons#14999

Description

Builds on #22640 to allow .zip files to be uploaded as attachment.

image

Note: The local nginx configuration fails for large files. There's an issue filed for this here. To test this feature locally, two lines in addons.conf need to be changed --

  1. try_files $uri @frontendamo; -> try_files $uri @olympia;
  2. client_max_body_size 50m;-> client_max_body_size 500m; (or anything else bigger than 200mb)

Testing

  1. Enable the enable-activity-log-attachments waffle switch.
  2. Log in with [email protected] (for ease of navigation).
  3. Submit a new add-on.
  4. Make a reviewer reply (or any other action) from the review action section and include a zip attachment.

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.

@chrstinalin chrstinalin marked this pull request as draft September 16, 2024 19:27
@chrstinalin chrstinalin marked this pull request as ready for review September 19, 2024 16:06
@chrstinalin chrstinalin requested review from a team and diox and removed request for a team September 19, 2024 16:07
@chrstinalin
Copy link
Contributor Author

Note that dev/stage/prod are currently configured with client_max_body_size 200m so the python code won't even see requests trying to upload files over 200 MB anyway, but we could bump it to 250 MB to give a little bit of leeway (we need that limit at the nginx level so that we don't accept requests too big to avoid DOS attacks and similar with large bodies). @diox

I did notice that uploading over the client_max_body_size will cause a 413. Now that I think about it more, it's probably better to avoid that situation altogether and check the size using JS similar to submitting a new add-on rather than arbitrarily increasing the limit

@diox
Copy link
Member

diox commented Sep 20, 2024

Yes, that would be ideal. Doesn't have to be as fancy, but that would be a nice little quality of life feature.

src/olympia/reviewers/forms.py Outdated Show resolved Hide resolved
)
)
$('#attachment_errors').append(error);
$(this).val('');
Copy link
Member

Choose a reason for hiding this comment

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

I don't love automatically clearing the input (the user might not realize that has happened) but it's fine as the first iteration. For later, I've added a suggestion to mozilla/addons#14835 (comment) we could re-apply here as well, but don't worry about it now.

@diox diox self-requested a review September 22, 2024 17:11
.append(
$('<li>').append(
format(gettext('Your file exceeds the maximum size of {0}.'), [
'200MB',
Copy link
Member

Choose a reason for hiding this comment

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

We don't have to hardcode this:

Intl.NumberFormat(document.documentElement.lang, {
  notation: 'compact',
  style: 'unit',
  unit: 'byte',
  unitDisplay: 'narrow',
}).format(max_upload_size);

(It gives 210MB because it's using SI units but that's fine)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't it be confusing for a 208MB file to reject with this error?

Copy link
Member

Choose a reason for hiding this comment

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

It already works actually - I think our python code is using SI units, so your patch already allows 210MB (200 MiB) files server-side.

@chrstinalin
Copy link
Contributor Author

I did not try all the possible reviewer actions to check the emails, but that would be mozilla/addons#15018. So far, I did notice that reviewer's reply email has this but I did not see a reference to it for approve or reject (will re-check anyway). (mozilla/addons#14997 (comment))

Template for approvals/rejections should also mention any attachments.

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.

[Task]: Reviewer tools: Support attaching a zip file to a review
2 participants