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

cslzchen · 2017-12-06T17:09:11Z

Ticket

https://openscience.atlassian.net/browse/SVCS-488
OSF side PR: CenterForOpenScience/osf.io#8002

Purpose

This ticket replaces #282. Credit goes to @AddisonSchiller 🎆🎆.

.docx rendering is very intensive on the OSF. By using Microsoft's online rendering service to render publicly available .docx files, we can remove a lot of pressure from the unoconv container.

Changes

Added support for public renderers.
Added the office365 renderer under the public renderers.
Added support to parse for a public_file query param. This query param is optional.public_file=True denotes that the file is public (the project it belongs to is public) , while public_file=False denotes that it is private. All other values for public_file cause errors to be raised.
ProviderMetadata now has an is_public flag, with default value set to False.

Side Effects

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?
iframe sandboxing may cause issue, need to verify on staging.

QA Notes

The Office365 renderer does not use the.pdf renderer like unoconv used to, so the pdfs that get made by this renderer may not display exactly the same. More QA notes to come. There is also a README.md in the renderer with more information about testing.

Deployment Notes

OSF side must be deployed first (no side effects).

coveralls · 2017-12-06T17:38:23Z

Coverage decreased (-0.1%) to 67.896% when pulling 02468e1 on cslzchen:feature/svcs-488-improve-docx into 8bb2dd4 on CenterForOpenScience:develop.

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 · 2017-12-07T16:25:12Z

Note: this PR cannot be tested locally since Office rendering service requires a public available file. Here is what shows up locally, and sandboxing does not cause any issue. src is https://view.officeapps.live.com/op/embed.aspx?src=http%3A//192.168.168.167%3A7777/v1/resources/2n3gt/providers/osfstorage/5a283b61a4a094002587bea3 as expected. The error is due to the service having no access to the local file.

Here is what shows up locally when hard-coding a staging download url with with iframe sandboxing turned on. src is empty for iframe due to sandboxing.

Here is what shows up locally when hard-coding a staging download url with iframe sandboxing turned off. src is "https://view.officeapps.live.com/op/embed.aspx?src=https%3A//staging-files.osf.io/v1/resources/swy3c/providers/osfstorage/5a29638421b9f2000d8f0a16".

coveralls · 2017-12-07T20:48:17Z

Coverage decreased (-0.1%) to 67.894% when pulling 2eec820 on cslzchen:feature/svcs-488-improve-docx into 8bb2dd4 on CenterForOpenScience:develop.

NyanHelsing · 2018-05-02T20:18:14Z

mfr/core/provider.py

 from mfr.core.metrics import MetricsRecord
+from mfr.server import settings as server_settings


Is settings shadowed somewhere?

Good catch. This is 6-month old code. Needs update. It no longer make sense to me.

NyanHelsing · 2018-05-02T20:19:49Z

mfr/extensions/office365/render.py

+from mako.lookup import TemplateLookup
+
+from mfr.core import extension
+from mfr.extensions.office365 import settings as office365_settings


why does this need to be qualified?

Same as the previous comment. We no longer do alias if there is no shadowing issues. Needs update.

NyanHelsing · 2018-05-02T20:21:50Z

mfr/extensions/office365/templates/viewer.mako

+}
+</style>
+
+<iframe src=${url} frameborder='0'></iframe>


The only thing it looks like is in here is another iframe - is it possible to return a 302 here?

I get your point but not sure how 302 works. OSF creates the parent iframe and we shouldn't modify it from MFR. Is embedded iframe a bad practice? For MFR itself, it should use iframe here since it loads content from an untrusted external web service.

NyanHelsing · 2018-05-02T20:22:34Z

mfr/providers/osf/provider.py

        response = await self._make_request('GET', download_url, allow_redirects=False, headers=headers)

-        if response.status >= 400:
+        if response.status >= HTTPStatus.BAD_REQUEST:


NyanHelsing · 2018-05-02T20:22:49Z

mfr/providers/osf/provider.py


    async def download(self):
        """Download file from WaterButler, returning stream."""
        download_url = await self._fetch_download_url()
-        headers = {settings.MFR_IDENTIFYING_HEADER: '1'}
+        headers = {provider_settings.MFR_IDENTIFYING_HEADER: '1'}


Again here, wondering why this needs to be qualified

Similarly, this should be pd_settings as we have been using it for both WB and MFR.

NyanHelsing · 2018-05-02T20:23:43Z

mfr/providers/osf/provider.py

                'Unable to download the requested file, please try again later.',
                download_url=download_url,
                response=resp_text,
                provider=self.NAME,
            )

        self.metrics.add('download.saw_redirect', False)
-        if response.status in (302, 301):
+        if response.status in (HTTPStatus.MOVED_PERMANENTLY, HTTPStatus.FOUND):


should this be a list rather than a tuple?

No sure. I had the same question. However, it was a tuple and the code base uses tuple.

cslzchen

@birdbrained Thanks for the review. This PR has been very old and we haven't decided whether this is the approach yet. I will rebase with the latest develop and fix the obsoleted import style.

More If You Are Interested

The blocking issues for using Microsofts rendering service are:

This service is not a reliable service.
This service cached their result for a few hours.
This service may have rate limiting.
More discussion and investigation is needed before continue developing.
We can only render public files through this service.

cslzchen · 2018-05-03T18:20:41Z

mfr/core/provider.py

 from mfr.core.metrics import MetricsRecord
+from mfr.server import settings as server_settings


Good catch. This is 6-month old code. Needs update. It no longer make sense to me.

cslzchen · 2018-05-03T18:27:26Z

mfr/extensions/office365/render.py

+from mako.lookup import TemplateLookup
+
+from mfr.core import extension
+from mfr.extensions.office365 import settings as office365_settings


Same as the previous comment. We no longer do alias if there is no shadowing issues. Needs update.

cslzchen · 2018-05-03T18:31:06Z

mfr/providers/osf/provider.py


    async def download(self):
        """Download file from WaterButler, returning stream."""
        download_url = await self._fetch_download_url()
-        headers = {settings.MFR_IDENTIFYING_HEADER: '1'}
+        headers = {provider_settings.MFR_IDENTIFYING_HEADER: '1'}


Similarly, this should be pd_settings as we have been using it for both WB and MFR.

cslzchen · 2018-05-03T18:35:34Z

mfr/providers/osf/provider.py

                'Unable to download the requested file, please try again later.',
                download_url=download_url,
                response=resp_text,
                provider=self.NAME,
            )

        self.metrics.add('download.saw_redirect', False)
-        if response.status in (302, 301):
+        if response.status in (HTTPStatus.MOVED_PERMANENTLY, HTTPStatus.FOUND):


No sure. I had the same question. However, it was a tuple and the code base uses tuple.

cslzchen · 2018-05-03T18:39:21Z

mfr/extensions/office365/templates/viewer.mako

+}
+</style>
+
+<iframe src=${url} frameborder='0'></iframe>


I get your point but not sure how 302 works. OSF creates the parent iframe and we shouldn't modify it from MFR. Is embedded iframe a bad practice? For MFR itself, it should use iframe here since it loads content from an untrusted external web service.

Public_file query param and office365 renderer

04740f1

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 force-pushed the feature/svcs-488-improve-docx branch 2 times, most recently from e2d6404 to 16c92e3 Compare December 7, 2017 14:43

cslzchen changed the title ~~[SVCS-488] [WIP] Improve docx rendering~~ [SVCS-488] Improve docx rendering Dec 7, 2017

cslzchen force-pushed the feature/svcs-488-improve-docx branch 2 times, most recently from 84b7b7d to d6cb8f8 Compare December 7, 2017 15:28

cslzchen mentioned this pull request Dec 7, 2017

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

Closed

cslzchen force-pushed the feature/svcs-488-improve-docx branch from d6cb8f8 to 7b3a984 Compare December 7, 2017 16:27

cslzchen mentioned this pull request Dec 7, 2017

[SVCS-488] Improve docx rendering - OSF Part CenterForOpenScience/osf.io#8002

Merged

cslzchen force-pushed the feature/svcs-488-improve-docx branch 2 times, most recently from 95c2eec to 2b04f43 Compare December 7, 2017 18:24

Encode download url, fix tests and style updates.

2eec820

cslzchen force-pushed the feature/svcs-488-improve-docx branch from 2b04f43 to 2eec820 Compare December 7, 2017 18:48

cslzchen changed the title ~~[SVCS-488] Improve docx rendering~~ [SVCS-488] Improve docx rendering - MFR Part Dec 7, 2017

cslzchen added the Final Review label Dec 7, 2017

cslzchen changed the title ~~[SVCS-488] Improve docx rendering - MFR Part~~ [SVCS-488] [Do Not Merge] Improve docx rendering - MFR Part Dec 12, 2017

cslzchen added the Do Not Merge label Jan 10, 2018

cslzchen changed the title ~~[SVCS-488] [Do Not Merge] Improve docx rendering - MFR Part~~ [SVCS-488] Improve docx rendering - MFR Part Jan 10, 2018

NyanHelsing reviewed May 2, 2018

View reviewed changes

cslzchen commented May 3, 2018

View reviewed changes

cslzchen added Blocked On Hold and removed Do Not Merge Final Review labels May 3, 2018

NyanHelsing mentioned this pull request Aug 3, 2018

Ft/docx rendering, RE: #304 #347

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cslzchen commented Dec 6, 2017 •

edited

Loading

coveralls commented Dec 6, 2017 •

edited

Loading

cslzchen commented Dec 7, 2017

coveralls commented Dec 7, 2017 •

edited

Loading

NyanHelsing May 2, 2018

cslzchen May 3, 2018

NyanHelsing May 2, 2018

cslzchen May 3, 2018 •

edited

Loading

NyanHelsing May 2, 2018

cslzchen May 3, 2018

NyanHelsing May 2, 2018

NyanHelsing May 2, 2018

cslzchen May 3, 2018 •

edited

Loading

NyanHelsing May 2, 2018

cslzchen May 3, 2018

cslzchen left a comment

cslzchen May 3, 2018

cslzchen May 3, 2018 •

edited

Loading

cslzchen May 3, 2018 •

edited

Loading

cslzchen May 3, 2018

cslzchen May 3, 2018

		from mfr.core.metrics import MetricsRecord
		from mfr.server import settings as server_settings

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

Are you sure you want to change the base?

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

Conversation

cslzchen commented Dec 6, 2017 • edited Loading

Ticket

Purpose

Changes

Side Effects

QA Notes

Deployment Notes

coveralls commented Dec 6, 2017 • edited Loading

cslzchen commented Dec 7, 2017

coveralls commented Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cslzchen May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cslzchen May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cslzchen left a comment

Choose a reason for hiding this comment

More If You Are Interested

Choose a reason for hiding this comment

cslzchen May 3, 2018 • edited Loading

Choose a reason for hiding this comment

cslzchen May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cslzchen commented Dec 6, 2017 •

edited

Loading

coveralls commented Dec 6, 2017 •

edited

Loading

coveralls commented Dec 7, 2017 •

edited

Loading

cslzchen May 3, 2018 •

edited

Loading

cslzchen May 3, 2018 •

edited

Loading

cslzchen May 3, 2018 •

edited

Loading

cslzchen May 3, 2018 •

edited

Loading