-
Notifications
You must be signed in to change notification settings - Fork 71
[SVCS-488]Public_file
query param and office365 renderer
#282
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,6 +76,19 @@ 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' | ||
if metadata.is_public: | ||
try: | ||
return driver.DriverManager( | ||
namespace='mfr.public_renderers', | ||
name=normalized_name, | ||
invoke_on_load=True, | ||
invoke_args=(metadata, file_path, url, assets_url, export_url), | ||
).driver | ||
except: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Be more explicit on the exception? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we talked about this. The reason it isn't specific is because any specific exception will be reported below. |
||
# Check for a public renderer, if one doesn't exist, use a regular one | ||
# Real exceptions handled by main driver.DriverManager | ||
pass | ||
|
||
try: | ||
return driver.DriverManager( | ||
namespace='mfr.renderers', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
|
||
# Office 365 Renderer | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Need to update this later when OSF side is updated. |
||
|
||
|
||
This renderer uses Office Online to render .docx files for us. If the Office Online URL ever changes, it will also need to be changed here in settings. | ||
|
||
Currently there is no OSF side component for these changes. Once there is, this specific note can be removed. In the meantime in order to test this renderer, you need to go to your local OSF copy of this file: https://github.com/CenterForOpenScience/osf.io/blob/develop/addons/base/views.py#L728-L736 | ||
and add 'public_file' : 1, to the dict. This will send all files as public files. | ||
|
||
Testing this renderer locally is hard. Since Office Online needs access to the files it will not work with private files or ones hosted locally. To see what the docx files will render like, replace the render function with something that looks like this: | ||
|
||
``` | ||
def render(self): | ||
static_url = 'https://files.osf.io/v1/resources/<fake_project_id>/providers/osfstorage/<fake_file_id>' | ||
url = settings.OFFICE_BASE_URL + download_url.url | ||
return self.TEMPLATE.render(base=self.assets_url, url=url) | ||
|
||
``` | ||
|
||
The file at `static_url` must be publicly available. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
from .render import Office365Renderer # noqa |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import os | ||
import furl | ||
|
||
from mfr.core import extension | ||
from mako.lookup import TemplateLookup | ||
from mfr.extensions.office365 import settings | ||
|
||
|
||
class Office365Renderer(extension.BaseRenderer): | ||
"""A renderer for use with public .docx files. | ||
|
||
Office online can render .docx files to pdf for us. | ||
This renderer will only ever be made if a query param with `public_file=1` is sent. | ||
It then generates and embeds an office online url into an | ||
iframe and returns the template. The file it is trying to render MUST | ||
be available publically online. This renderer will not work if testing locally. | ||
|
||
""" | ||
|
||
TEMPLATE = TemplateLookup( | ||
directories=[ | ||
os.path.join(os.path.dirname(__file__), 'templates') | ||
]).get_template('viewer.mako') | ||
|
||
def render(self): | ||
download_url = furl.furl(self.metadata.download_url).set(query='') | ||
url = settings.OFFICE_BASE_URL + download_url.url | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Encode the URL since it is the value of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is what mine looks like
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 |
||
return self.TEMPLATE.render(base=self.assets_url, url=url) | ||
|
||
@property | ||
def file_required(self): | ||
return False | ||
|
||
@property | ||
def cache_result(self): | ||
return False |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
from mfr import settings | ||
|
||
|
||
config = settings.child('OFFICE365_EXTENSION_CONFIG') | ||
|
||
OFFICE_BASE_URL = 'https://view.officeapps.live.com/op/embed.aspx?src=' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
<style> | ||
iframe { | ||
width: 100%; | ||
height: 800; | ||
} | ||
</style> | ||
|
||
<iframe src=${url} frameborder='0'></iframe> | ||
|
||
<script src="/static/js/mfr.js"></script> | ||
<script src="/static/js/mfr.child.js"></script> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
import furl | ||
import pytest | ||
|
||
from mfr.extensions.office365 import settings | ||
from mfr.core.provider import ProviderMetadata | ||
from mfr.extensions.office365 import Office365Renderer | ||
|
||
|
||
@pytest.fixture | ||
def metadata(): | ||
return ProviderMetadata('test', '.pdf', 'text/plain', '1234', | ||
'http://wb.osf.io/file/test.pdf?token=1234&public_file=1', | ||
is_public=True) | ||
|
||
|
||
@pytest.fixture | ||
def file_path(): | ||
return '/tmp/test.docx' | ||
|
||
|
||
@pytest.fixture | ||
def url(): | ||
return 'http://osf.io/file/test.pdf' | ||
|
||
|
||
@pytest.fixture | ||
def assets_url(): | ||
return 'http://mfr.osf.io/assets' | ||
|
||
|
||
@pytest.fixture | ||
def export_url(): | ||
return 'http://mfr.osf.io/export?url=' + url() | ||
|
||
|
||
@pytest.fixture | ||
def renderer(metadata, file_path, url, assets_url, export_url): | ||
return Office365Renderer(metadata, file_path, url, assets_url, export_url) | ||
|
||
|
||
class TestOffice365Renderer: | ||
|
||
def test_render_pdf(self, renderer, metadata, assets_url): | ||
download_url = furl.furl(metadata.download_url).set(query='') | ||
body_url = settings.OFFICE_BASE_URL + download_url.url | ||
body = renderer.render() | ||
assert '<iframe src={} frameborder=\'0\'></iframe>'.format(body_url) in body |
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.
Can we make
is_public
position args if not too refactoring is required?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.
You mean like get rid of the public_file=false
? and it just being..
?
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.
As discussed, keep it as
kwargs
.