[SVCS-488]`Public_file` query param and office365 renderer #282

AddisonSchiller · 2017-10-04T17:34:59Z

refs: https://openscience.atlassian.net/browse/SVCS-488

Purpose

docx rendering is very intensive on the osf. By using Office Online to render publicly available docx files, we can remove a lot of pressure from the unoconv container.

Summary of changes.

Added support for public renderers
Added support to parse for a public_file query param. This query param is optional.public_file=1 denotes that the file is public, while public_file=0 denotes that it is private. All other values for public_file cause errors to be raised.
ProviderMetadata now has an is_public value, which defaults to false.
Added the office365 renderer, which is a public renderer.

Testing Notes

the Office365 renderer does not use the pdf renderer like unoconv used to, so the pdfs that get made by this renderer will not display exactly the same.

There is a README.md in the renderer with more information about testing.

In order to test this with the OSF, an OSF side change needs to be made to make use of the new public_file query param.

Adding support for a `public_file` query param so the OSF can request a public renderer. Added office365 which is a public renderer. This uses office online to do .docx file conversions.

cslzchen

Works as expected. As discussed with @AddisonSchiller , we may want to change how is_public is surfaced from OSF to WB/MFR.

Currently in the PR: OSF provides the download URL with query param is_public=1 for MFR. My suggestion is to ask OSF to export is_public to the file metadata (which is a more general approach can be used for other/future). But this requires WB update as well. @felliott

cslzchen

First pass done! 🎆 🎆

cslzchen · 2017-11-07T20:03:37Z

setup.py

@@ -40,6 +40,10 @@ def parse_requirements(requirements):
            'http = mfr.providers.http:HttpProvider',
            'osf = mfr.providers.osf:OsfProvider',
        ],
+        'mfr.public_renderers': [
+            '.docx = mfr.extensions.office365:Office365Renderer',
+


Is this blank line intentional?

cslzchen · 2017-11-07T20:50:20Z

mfr/providers/osf/provider.py

+                    code=400,
+                )
+
+            if cleaned_url.args['public_file'] == '1':


is_public = False if ... if ... raise ... is_public = cleaned_url.args['public_file'] == '1' return ...

cslzchen · 2017-11-07T20:52:07Z

mfr/core/provider.py

@@ -48,17 +48,19 @@ def download(self):

 class ProviderMetadata:

-    def __init__(self, name, ext, content_type, unique_key, download_url):
+    def __init__(self, name, ext, content_type, unique_key, download_url, is_public=False):


Can we make is_public position args if not too refactoring is required?

You mean like get rid of the public_file=false
? and it just being..

.... download_url, is_public)

?

As discussed, keep it as kwargs.

cslzchen · 2017-11-07T21:11:43Z

mfr/core/utils.py

+                invoke_on_load=True,
+                invoke_args=(metadata, file_path, url, assets_url, export_url),
+            ).driver
+        except:


Be more explicit on the exception?

I think we talked about this. The reason it isn't specific is because any specific exception will be reported below.
If this is new let me know.

cslzchen · 2017-11-07T21:13:43Z

mfr/extensions/office365/README.md

@@ -0,0 +1,20 @@
+
+# Office 365 Renderer


👍

Need to update this later when OSF side is updated.

cslzchen · 2017-11-07T21:16:06Z

mfr/extensions/office365/render.py

+
+    def render(self):
+        download_url = furl.furl(self.metadata.download_url).set(query='')
+        url = settings.OFFICE_BASE_URL + download_url.url


Encode the URL since it is the value of the src query param. It works without encoding during local test since the URL is probably not ambiguous.

Need proper encoding.

What we feed to MS Office Live is an WB URL and here is an example of my local one which is more complicated than OSF url we test.

http://192.168.168.167:7777/v1/resources/ygcb4/providers/osfstorage/5a01dd60005aca00414c567d?action=download&public_file=1&version=1&mode=render&direct

Here is what mine looks like
This is the download_url after the query params are stripped off

http://192.168.168.167:7777/v1/resources/3uq58/providers/osfstorage/5a03579db4eef2001827a8c3

as long as the project is public, going to that link will download the file. Also there are no chars that -need- to be encoded. If they should be encoded, well probably? We briefly discussed this. Thoughts on encoding the / marks?

coveralls · 2017-11-08T15:46:42Z

Changes Unknown when pulling 8c014ea on AddisonSchiller:feature/docx-office365-online-renderer into ** on CenterForOpenScience:develop**.

cslzchen

Second pass done. 🎆 🎆

cslzchen · 2017-11-08T16:38:21Z

mfr/core/utils.py

@@ -76,6 +78,20 @@ def make_renderer(name, metadata, file_path, url, assets_url, export_url):
    :rtype: :class:`mfr.core.extension.BaseRenderer`
    """
    normalized_name = (name and name.lower()) or 'none'
+    logger.info(str(metadata.is_public))


Remove this line.

Probably need to remove the imports as well.

cslzchen · 2017-11-08T16:39:34Z

mfr/extensions/office365/render.py

+
+    def render(self):
+        download_url = furl.furl(self.metadata.download_url).set(query='')
+        url = settings.OFFICE_BASE_URL + download_url.url


Need proper encoding.

What we feed to MS Office Live is an WB URL and here is an example of my local one which is more complicated than OSF url we test.

http://192.168.168.167:7777/v1/resources/ygcb4/providers/osfstorage/5a01dd60005aca00414c567d?action=download&public_file=1&version=1&mode=render&direct

cslzchen · 2017-11-08T16:42:11Z

mfr/core/provider.py

@@ -48,17 +48,19 @@ def download(self):

 class ProviderMetadata:

-    def __init__(self, name, ext, content_type, unique_key, download_url):
+    def __init__(self, name, ext, content_type, unique_key, download_url, is_public=False):


As discussed, keep it as kwargs.

coveralls · 2017-11-08T19:25:10Z

Changes Unknown when pulling e3c30ff on AddisonSchiller:feature/docx-office365-online-renderer into ** on CenterForOpenScience:develop**.

cslzchen

🎆 🎆 🎆 🎆 🎆

Notes for Phase 2

Encoding

WB download URL

Here is the code and our discussions: https://github.com/CenterForOpenScience/modular-file-renderer/pull/282/files#diff-2fc1019752cc1cd0a7f98aa9bec5974fR27. We are not sure whether to use furl or python's urllib. Currently, we leave it not encoded :-) for experiment purpose.

Notes for the Future

Does Microsoft enforces a rate limit?
How can we handle the "a few hour" cache problem?
What if this external service is down? Should we detect in advance?

Notes for the Merge/Deployment

Requires OSF side to take effect
This PR can slip into dev / prod first.
We should test on staging by ourself before QA

cslzchen · 2017-12-07T16:14:40Z

Replaced by #304.

Public_file query param and office365 renderer

ba1dfaa

Adding support for a `public_file` query param so the OSF can request a public renderer. Added office365 which is a public renderer. This uses office online to do .docx file conversions.

AddisonSchiller force-pushed the feature/docx-office365-online-renderer branch from 582453b to ba1dfaa Compare October 4, 2017 17:38

AddisonSchiller changed the title ~~Public_file query param and office365 renderer~~ [SVCS-488]Public_file query param and office365 renderer Oct 4, 2017

cslzchen added Code Review Final Review labels Oct 12, 2017

cslzchen self-requested a review October 25, 2017 15:21

cslzchen reviewed Nov 7, 2017

View reviewed changes

cslzchen requested changes Nov 7, 2017

View reviewed changes

Style changes

8c014ea

cslzchen requested changes Nov 8, 2017

View reviewed changes

Remove debugging

e3c30ff

cslzchen approved these changes Nov 8, 2017

View reviewed changes

cslzchen removed the Code Review label Nov 8, 2017

cslzchen mentioned this pull request Dec 6, 2017

[SVCS-488] Improve docx rendering - MFR Part #304

Open

1 task

cslzchen closed this Dec 7, 2017

NyanHelsing mentioned this pull request Aug 3, 2018

Ft/docx rendering, RE: #304 #347

Open

1 task

[SVCS-488]Public_file query param and office365 renderer #282

[SVCS-488]Public_file query param and office365 renderer #282

Uh oh!

Conversation

AddisonSchiller commented Oct 4, 2017

Purpose

Summary of changes.

Testing Notes

Uh oh!

cslzchen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cslzchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cslzchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cslzchen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Notes for Phase 2

Notes for the Future

Notes for the Merge/Deployment

Uh oh!

cslzchen commented Dec 7, 2017

Uh oh!

Uh oh!

[SVCS-488]`Public_file` query param and office365 renderer #282

[SVCS-488]`Public_file` query param and office365 renderer #282

cslzchen left a comment •

edited

Loading

coveralls commented Nov 8, 2017 •

edited

Loading

coveralls commented Nov 8, 2017 •

edited

Loading

cslzchen left a comment •

edited

Loading