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

Pdf mobile #2102

Merged
merged 11 commits into from
Aug 21, 2017
Merged

Pdf mobile #2102

merged 11 commits into from
Aug 21, 2017

Conversation

anastasia
Copy link
Contributor

@anastasia anastasia commented Aug 17, 2017

fixes #2056
Showing "view/download" button on any mobile device when viewing a PDF

screen shot 2017-08-17 at 5 48 28 pm

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 85.174% when pulling 7dd89df on anastasia:pdf-mobile into cae4475 on harvard-lil:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 85.167% when pulling 7dd89df on anastasia:pdf-mobile into cae4475 on harvard-lil:develop.

@anastasia anastasia requested review from rebeccacremona and removed request for rebeccacremona August 17, 2017 22:37
@@ -105,7 +105,7 @@ def sauce_tunnel():
Set up Sauce tunnel before running functional tests targeted at localhost.
"""
if subprocess.call(['which','sc']) == 1: # error return code -- program not found
sys.exit("Please check that the `sc` program is installed and in your path. To install: https://docs.saucelabs.com/reference/sauce-connect/")
sys.exit("Please check that the `sc` program is installed and in your path. To install: https://wiki.saucelabs.com/display/DOCS/Sauce+Connect+Proxy")
Copy link
Contributor

Choose a reason for hiding this comment

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

LINK ROT!!!!!!

@@ -0,0 +1,7 @@
{% with request.META.HTTP_REFERER as referer %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is referer actually used in this "with" block? Part of a previous draft maybe?

{% endblock %}

{% block mainContent %}

{% if link.user_deleted %}
{% include "archive/deleted.html" %}
{% elif redirect_to_download_view %}
{% include "archive/download_to_view_pdf.html" %}
{% elif can_view %}
{% include "archive/iframe.html" %}
{% elif link.is_private %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think maybe this new part needs to be nested as an option inside the "elif can_view" block. As is, does this expose links that people wouldn't be allowed to view, via a laptop/desktop?

@@ -54,6 +54,17 @@ def test_dark_archive(self):
response = self.get('single_linky', reverse_kwargs={'kwargs': {'guid': 'ABCD-0001'}})
self.assertIn("This record is private.", response.content)

def test_redirect_to_download(self):
# Give user option to download to view pdf if on mobile
client = Client(HTTP_USER_AGENT='Mozilla/5.0 (iPhone; CPU iPhone OS 6_1_4 like Mac OS X) AppleWebKit/536.26 (KHTML, like Gecko) Version/6.0 Mobile/10B350 Safari/8536.25')
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool!

Copy link
Contributor

@rebeccacremona rebeccacremona left a comment

Choose a reason for hiding this comment

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

Thanks so much for tackling this pesky issue!!! Looks great: had a couple quick questions, left as individual comments.

capture_mime_type = capture.mime_type()
redirect_to_download_view = redirect_to_download(capture_mime_type, raw_user_agent)
except AttributeError:
# if capture is deleted, we will catch it here
Copy link
Contributor

Choose a reason for hiding this comment

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

Really liked your commit message here. "If capture is deleted, then mime type does not exist. Catch error." Total nitpick, totally up to you: do you think that might be a clearer comment here than the above?

Copy link
Contributor

Choose a reason for hiding this comment

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

And while I'm being a complete pain in the butt: redirect_to_download(capture_mime_type, raw_user_agent) won't throw an attribute error right? Should that line be outside of the try block?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 85.164% when pulling b7fa0a6 on anastasia:pdf-mobile into cae4475 on harvard-lil:develop.

@rebeccacremona rebeccacremona merged commit d73ed80 into harvard-lil:develop Aug 21, 2017
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.

PDF in Perma Link displaying only 1st page on mobile
3 participants