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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
705f866
Look for `mfr` query param from mfr in v1 provider
AddisonSchiller Oct 24, 2017
8947b8f
Util variable names
AddisonSchiller Oct 31, 2017
99cc7d7
fix get_alt_export_link
AddisonSchiller Nov 28, 2017
96a4eee
Merge branch 'develop' into feature/SVCS-528-improve-gdoc
AddisonSchiller Nov 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions tests/providers/googledrive/test_metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ def test_file_metadata_drive(self, basepath, root_provider_fixtures):
assert parsed.materialized_path == str(path)
assert parsed.is_google_doc is False
assert parsed.export_name == item['title']
assert parsed.alt_export_name == 'PART_1420130849837.pdf'

def test_file_metadata_drive_slashes(self, basepath, root_provider_fixtures):
item = root_provider_fixtures['file_forward_slash']
Expand All @@ -66,6 +67,7 @@ def test_file_metadata_drive_slashes(self, basepath, root_provider_fixtures):
assert parsed.materialized_path == str(path)
assert parsed.is_google_doc is False
assert parsed.export_name == item['title']
assert parsed.alt_export_name == 'PART_1420130849837.pdf'

def test_file_metadata_docs(self, basepath, root_provider_fixtures):
item = root_provider_fixtures['docs_file_metadata']
Expand All @@ -80,6 +82,7 @@ def test_file_metadata_docs(self, basepath, root_provider_fixtures):
}
assert parsed.is_google_doc is True
assert parsed.export_name == item['title'] + '.docx'
assert parsed.alt_export_name == 'version-test.pdf'

def test_folder_metadata(self, root_provider_fixtures):
item = root_provider_fixtures['folder_metadata']
Expand Down
44 changes: 44 additions & 0 deletions tests/providers/googledrive/test_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,7 @@ class TestDownload:
ds.DRIVE_IGNORE_VERSION)

GDOC_EXPORT_MIME_TYPE = 'application/vnd.openxmlformats-officedocument.wordprocessingml.document'
GDOC_ALT_EXPORT_MIME_TYPE = 'application/pdf'

@pytest.mark.asyncio
@pytest.mark.aiohttpretty
Expand Down Expand Up @@ -619,6 +620,36 @@ async def test_download_editable_gdoc_no_revision(self, provider, sharing_fixtur
assert aiohttpretty.has_call(method='GET', uri=revisions_url)
assert aiohttpretty.has_call(method='GET', uri=download_file_url)

@pytest.mark.asyncio
@pytest.mark.aiohttpretty
async def test_download_editable_gdoc_as_mfr(self, provider, sharing_fixtures):
metadata_body = sharing_fixtures['editable_gdoc']['metadata']
path = GoogleDrivePath(
'/sharing/editable_gdoc',
_ids=['1', '2', metadata_body['id']]
)

metadata_query = provider._build_query(path.identifier)
metadata_url = provider.build_url('files', path.identifier)
aiohttpretty.register_json_uri('GET', metadata_url, body=metadata_body)

revisions_body = sharing_fixtures['editable_gdoc']['revisions']
revisions_url = provider.build_url('files', metadata_body['id'], 'revisions')
aiohttpretty.register_json_uri('GET', revisions_url, body=revisions_body)

file_content = b'we love you conrad'
download_file_url = metadata_body['exportLinks'][self.GDOC_ALT_EXPORT_MIME_TYPE]
aiohttpretty.register_uri('GET', download_file_url, body=file_content, auto_length=True)

result = await provider.download(path, mfr='true')
assert result.name == 'editable_gdoc.pdf'

content = await result.read()
assert content == file_content
assert aiohttpretty.has_call(method='GET', uri=metadata_url)
assert aiohttpretty.has_call(method='GET', uri=revisions_url)
assert aiohttpretty.has_call(method='GET', uri=download_file_url)

@pytest.mark.asyncio
@pytest.mark.aiohttpretty
async def test_download_editable_gdoc_good_revision(self, provider, sharing_fixtures):
Expand Down Expand Up @@ -1577,6 +1608,19 @@ async def test_intra_copy_file(self, provider, root_provider_fixtures):

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?

metadata = {
'mimeType': 'application/vnd.google-apps.drawing',
'exportLinks': {
'image/jpeg': 'badurl.osf.899'
}
}
ext = drive_utils.get_alt_download_extension(metadata)
link = drive_utils.get_alt_export_link(metadata)

assert ext == '.jpg'
assert link == 'badurl.osf.899'

@pytest.mark.asyncio
@pytest.mark.aiohttpretty
async def test_can_duplicate_names(self, provider):
Expand Down
8 changes: 8 additions & 0 deletions waterbutler/providers/googledrive/metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,14 @@ def export_name(self):
title += ext
return title

@property
def alt_export_name(self):
title = self._file_title
if self.is_google_doc:
ext = utils.get_alt_download_extension(self.raw)
title += ext
return title

@property
def _file_title(self):
return self.raw['title']
Expand Down
11 changes: 9 additions & 2 deletions waterbutler/providers/googledrive/provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,9 +234,16 @@ async def download(self, # type: ignore

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_url = metadata.raw.get('downloadUrl') or drive_utils.get_alt_export_link(metadata.raw), # type: ignore
export_name = metadata.alt_export_name
else:
download_url = metadata.raw.get('downloadUrl') or drive_utils.get_export_link(metadata.raw), # type: ignore
export_name = metadata.export_name # type: ignore

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.

*download_url,
range=range,
expects=(200, 206),
throws=exceptions.DownloadError,
Expand All @@ -251,7 +258,7 @@ async def download(self, # type: ignore
if download_resp.headers.get('Content-Type'):
# TODO: Add these properties to base class officially, instead of as one-off
stream.content_type = download_resp.headers['Content-Type'] # type: ignore
stream.name = metadata.export_name # type: ignore
stream.name = export_name # type: ignore
return stream

async def upload(self, stream, path: wb_path.WaterButlerPath, *args, **kwargs) \
Expand Down
18 changes: 18 additions & 0 deletions waterbutler/providers/googledrive/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
'ext': '.gdoc',
'download_ext': '.docx',
'type': 'application/vnd.openxmlformats-officedocument.wordprocessingml.document',
'alt_download_ext': '.pdf',
'alt_type': 'application/pdf',
},
{
'mime_type': 'application/vnd.google-apps.drawing',
Expand Down Expand Up @@ -59,6 +61,22 @@ def get_download_extension(metadata):
return format['download_ext']


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)

except:
return format['download_ext']


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

except:
return metadata['exportLinks'][format['type']]


def get_export_link(metadata):
format = get_format(metadata)
return metadata['exportLinks'][format['type']]
1 change: 1 addition & 0 deletions waterbutler/server/api/v1/provider/metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ async def download_file(self):
range=request_range,
accept_url='direct' not in self.request.query_arguments,
mode=self.get_query_argument('mode', default=None),
mfr=self.get_query_argument('mfr', default=None)
)

if isinstance(stream, str):
Expand Down