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

Fixes #4791 shows png files instead of SVG in source index #4845

Closed
wants to merge 1 commit into from

Conversation

kushaldas
Copy link
Contributor

Status

Ready for review

Description of Changes

Fixes #4791

Adds 3 png files and also updates the source index tempate to use
the png files.

Testing

  • Set your Tor security level to "safest"
  • Set your NoScript setting to "Disable restrictions for this tab"
  • Load a SecureDrop source interface

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing production instances.
  2. New installs.

Checklist

If you made changes to the server application code:

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

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

If you made changes to documentation:

  • Doc linting (make docs-lint) passed locally

@codecov-io
Copy link

Codecov Report

Merging #4845 into develop will increase coverage by 4.21%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4845      +/-   ##
===========================================
+ Coverage    77.53%   81.74%   +4.21%     
===========================================
  Files           49       49              
  Lines         3418     3418              
  Branches       392      392              
===========================================
+ Hits          2650     2794     +144     
+ Misses         669      533     -136     
+ Partials        99       91       -8
Impacted Files Coverage Δ
securedrop/securedrop/journalist_app/admin.py 86.2% <0%> (+1.14%) ⬆️
securedrop/securedrop/journalist_app/utils.py 89% <0%> (+4.71%) ⬆️
securedrop/securedrop/models.py 90.6% <0%> (+5.8%) ⬆️
securedrop/securedrop/journalist_app/main.py 75.49% <0%> (+6.86%) ⬆️
securedrop/securedrop/journalist_app/account.py 93.87% <0%> (+12.24%) ⬆️
securedrop/securedrop/journalist_app/col.py 83.67% <0%> (+16.32%) ⬆️
securedrop/securedrop/store.py 93.79% <0%> (+26.2%) ⬆️
securedrop/securedrop/rm.py 95.23% <0%> (+42.85%) ⬆️
securedrop/securedrop/worker.py 85% <0%> (+58.33%) ⬆️

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 4a61b7e...7e4b2ba. Read the comment docs.

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

Do we still reference the old svg's elsewhere? If not (it seems like they aren't referenced elsewhere), perhaps we could delete them and their associated AppArmor rules.

zenmonkeykstop
zenmonkeykstop previously approved these changes Sep 23, 2019
Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

Tested against staging VMs, source interface renders correctly using PNGs for the changed icons.

@zenmonkeykstop zenmonkeykstop dismissed their stale review September 25, 2019 00:38

waiting on response to @emkll before merging.

@kushaldas
Copy link
Contributor Author

kushaldas commented Sep 25, 2019

Do we still reference the old svg's elsewhere? If not (it seems like they aren't referenced elsewhere), perhaps we could delete them and their associated AppArmor rules.

We should at least keep the svgs as source file for the png files. And, I think we should remove the AppArmor rules as you suggested.

@kushaldas
Copy link
Contributor Author

@emkll ping for bringing it back to view :)

@redshiftzero
Copy link
Contributor

redshiftzero commented Nov 9, 2019

I think we should remove the AppArmor rules as you suggested

It looks like the svg files that are no longer used are still whitelisted by AppArmor in this PR - can you update that prior to merge @kushaldas?

Adds 3 png files and also updates the source index tempate to use
the PNG files. Removes the unused svg files of the same PNG files.
@redshiftzero
Copy link
Contributor

hmm lint is failing but the failure looks unrelated to this diff - want to rebase on latest?

@eloquence
Copy link
Member

eloquence commented Jun 11, 2020

The motivating issue appears to have been resolved upstream. We can still pick up this PR (it may be wise to default to PNG to avoid future issues like this), but note that

  1. we should prune images that are no longer used;
  2. we should render at a higher output resolution, to ensure quality rendering on HiDPI displays.

@eloquence
Copy link
Member

Closing out this year+-old PR, @kushaldas feel free to reopen if you want to pick this up again.

@eloquence eloquence closed this Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SVG in Tor warning banner not rendered if NoScript set to permit JS
6 participants