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

Back out functionality requiring libjpeg-dev apt dependency #3379

Merged
merged 3 commits into from
May 8, 2018

Conversation

redshiftzero
Copy link
Contributor

@redshiftzero redshiftzero commented May 8, 2018

Status

Ready for review

Description of Changes

Fixes #3375. Fixes #3316. Re-opens #2807 ("Re-opens" is not a keyword so the merger of this PR should be sure the ticket is indeed re-opened).

Changes proposed in this pull request:

  • Remove functionality requiring libjpeg-dev and thus Pillow. The functionality change is that we won't be able to accept JPG/JPEG logos (a lesser issue) and we won't be able to resize images server-size (a greater issue, as such this part is unfortunate). Note that we could potentially implement a roundabout way to do this, but I advocate we just fix the fact that we can't safely add libjpeg-dev, and then bring in the changes again.

🌏Note 🌏 This involves a string change, which I apologize for. I could not find a way to reuse old strings on the interface that would not be very awkward. Suggestions are welcome here.

Testing

  1. Provision staging on this branch.
  2. Try to submit a PNG, observe success.
  3. Try to submit a JPG or JPEG, observe failure.

Deployment

This can be safely deployed to instances

Checklist

If you made changes to the server application code:

  • Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

If you made changes to the system configuration:

If you made changes to documentation:

  • Doc linting (make docs-lint) passed locally

@redshiftzero redshiftzero requested a review from emkll May 8, 2018 20:08
@redshiftzero redshiftzero requested a review from conorsch as a code owner May 8, 2018 20:08
@redshiftzero redshiftzero requested a review from a user May 8, 2018 20:08
@codecov-io
Copy link

codecov-io commented May 8, 2018

Codecov Report

Merging #3379 into develop will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3379      +/-   ##
===========================================
- Coverage    85.81%   85.79%   -0.02%     
===========================================
  Files           34       34              
  Lines         2157     2154       -3     
  Branches       238      238              
===========================================
- Hits          1851     1848       -3     
  Misses         250      250              
  Partials        56       56
Impacted Files Coverage Δ
journalist_app/admin.py 87.33% <0%> (-0.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e10378f...6b63b78. Read the comment docs.

@redshiftzero redshiftzero added this to the 0.7 milestone May 8, 2018
message=gettext("You can only upload JPG/JPEG"
" or PNG image files."))
FileAllowed(['png'],
message=gettext("You can only upload PNG image files."))
Copy link

Choose a reason for hiding this comment

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

If we change that to Upload images only. it will alow us to re-use a former translation instead of asking them for a last minute change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's fair since this is a last minute change. I've modified this to reuse this translation ✅.

Once this PR is merged, I'll prepare a PR (not for 0.7.0 😇) changing the string to "You can only upload PNG image files." since that's more clear for end users.

Also backs out handling of JPEG images in the custom logo and
removes the resizing logic. This will need to be brought back
in at a later date when we can add libjpeg-dev as an apt
dependency.
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

💯

@redshiftzero redshiftzero merged commit 616f76e into develop May 8, 2018
@redshiftzero redshiftzero deleted the safe-image-logo branch May 8, 2018 23:20
emkll added a commit that referenced this pull request May 9, 2018
@emkll emkll mentioned this pull request May 10, 2018
21 tasks
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.

Remove functionality requiring libjpeg-dev Debian package securedrop-app-code is not getting upgraded
2 participants