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

[SVCS-528] Look for mfr query param from mfr in v1 provider #290

Conversation

AddisonSchiller
Copy link
Contributor

@AddisonSchiller AddisonSchiller commented Oct 24, 2017

We can export .gdoc as a pdf if we know its coming from
mfr. Added alt_export functions and properties to Gdrive
utils and metadata. Gdrive download now looks for 'mfr' in its
kwargs and will request file as pdf if used properly.

This ticket should be merged BEFORE its MFR counterpart

Ticket

https://openscience.atlassian.net/browse/SVCS-528
MFR counterpart: CenterForOpenScience/modular-file-renderer#292

Purpose

Allow mfr to request .gdoc files in other export formats (to .pdf from .docx). However allow the osf to still download in .docx format

This will allow MFR to cut out the expensive unoconv conversion for .gdoc files

Changes

v1 provider now looks for a 'mfr' query param. if true, the google drive provider will use alt_export_name and alt_export_link() instead of its normal counterparts. This will cause the file being sent to be a .pdf over a .docx
Added tests for new changes.

Side effects

There should be none. File downloads from gdrive will not be effected as long as they don't include ...&mfr=true as a query param

QA Notes

This needs to be tested with its mfr counterpart to have noticeable changes.

Deployment Notes

We can export .gdoc as a pdf if we know its coming from
mfr. Added `alt_export` functions and properties to Gdrive
utils and metadata. Gdrive download now looks for 'mfr' in its
kwargs and will request file as pdf if used properly.
@coveralls
Copy link

coveralls commented Oct 24, 2017

Coverage Status

Coverage increased (+0.04%) to 89.243% when pulling 705f866 on AddisonSchiller:feature/SVCS-528-improve-gdoc into cc68aca on CenterForOpenScience:develop.

@cslzchen cslzchen self-requested a review October 26, 2017 15:44
@cslzchen cslzchen changed the title [SVCS-528]Look for mfr query param from mfr in v1 provider [SVCS-528] Look for mfr query param from mfr in v1 provider Oct 26, 2017
Copy link
Contributor

@cslzchen cslzchen left a comment

Choose a reason for hiding this comment

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

First pass done! I will do a local test during the second pass. 🔥 🔥

@@ -234,9 +234,16 @@ def can_intra_copy(self,

metadata = await self.metadata(path, revision=revision)

if 'mfr' in kwargs and kwargs['mfr'] and kwargs['mfr'].lower() == 'true':
Copy link
Contributor

Choose a reason for hiding this comment

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

For dictionaries, we can use the .get with default value None if key does not exist.

mfr_only = kwargs.get('mfr', None)
if mfr_only and mfr_only.lower() == 'true':
    ...

download_resp = await self.make_request(
'GET',
metadata.raw.get('downloadUrl') or drive_utils.get_export_link(metadata.raw), # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

On metadata.raw.get('downloadUrl') vs drive_utils.get_export_link(metadata.raw): from the name I assume that the former is for download while the latter for export. Is it possible that the downloadUrl turns out to be not None so we never get a chance to hit the get_alt_export_link or get_export_link.

def get_alt_download_extension(metadata):
format = get_format(metadata)
try:
return format['alt_download_ext']
Copy link
Contributor

Choose a reason for hiding this comment

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

How about the following? Please note that format is a python built-in.

metadata_format = get_format(metadata)
return metadata_format.get('alt_download_ext', None) or metadata_format.get('download_ext', None)

def get_alt_export_link(metadata):
format = get_format(metadata)
try:
return metadata['exportLinks'][format['alt_type']]
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is my example, but it looks more complicated than yours. I can't use type since it is a built-in as well.

metadata_format = get_format(metadata)
format_type = metadata_format.get('alt_type', None) or metadata_format.get('type', None)
export_links = metadata.get('exportLinks', None)
if format_type and export_links:
    return export_links.get('format_type', None)
return None

@@ -1577,6 +1608,19 @@ class TestIntraFunctions:

class TestOperationsOrMisc:

def test_misc_utils(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to make this more specific (both method name and the test inside). What is this test trying to do?

Change download_url for `mfr` case
@Johnetordoff
Copy link
Contributor

Looks like this has been failing Travis putting it in AD.

@AddisonSchiller
Copy link
Contributor Author

👍

@coveralls
Copy link

coveralls commented Nov 28, 2017

Coverage Status

Coverage increased (+0.8%) to 89.973% when pulling 99cc7d7 on AddisonSchiller:feature/SVCS-528-improve-gdoc into cc68aca on CenterForOpenScience:develop.

@coveralls
Copy link

coveralls commented Nov 29, 2017

Coverage Status

Coverage increased (+0.03%) to 89.973% when pulling 96a4eee on AddisonSchiller:feature/SVCS-528-improve-gdoc into 26bf209 on CenterForOpenScience:develop.

@cslzchen
Copy link
Contributor

cslzchen commented Dec 5, 2017

Replaced by #304.

@cslzchen cslzchen closed this Dec 5, 2017
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.

4 participants