-
Notifications
You must be signed in to change notification settings - Fork 76
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
[SVCS-528] Look for mfr
query param from mfr in v1 provider
#290
Conversation
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.
mfr
query param from mfr in v1 providermfr
query param from mfr in v1 provider
There was a problem hiding this 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': |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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'] |
There was a problem hiding this comment.
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']] |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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
Looks like this has been failing Travis putting it in AD. |
👍 |
Replaced by #304. |
We can export .gdoc as a pdf if we know its coming from
mfr. Added
alt_export
functions and properties to Gdriveutils 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
andalt_export_link()
instead of its normal counterparts. This will cause the file being sent to be a .pdf over a .docxAdded 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 paramQA Notes
This needs to be tested with its mfr counterpart to have noticeable changes.
Deployment Notes