[SVCS-159] Add renderer/exporter names to cached files #296
Conversation
cslzchen
requested review from
binoculars and
cslzchen
and removed request for
binoculars
cslzchen
left a comment
cslzchen
left a comment
There was a problem hiding this comment.
Reviewed in person. One typo and something I forgot. Back to you now while I am testing locally with cache enabled.
mfr/core/utils.py
Outdated
|
|
||
| # ep.attrs is a tuple of attributes. There should only ever be one or `None`. | ||
| # None case occurs when trying to render an unsupported file type | ||
| # entry_attrs is an itrable object, so we turn into a list to index it |
| :rtype : `str` | ||
| """ | ||
|
|
||
| # `make_renderer` should have already caught if an extension doesn't exist. |
mfr/server/handlers/export.py
Outdated
| self.output_file_id = '{}.{}'.format(self.source_file_path.name, self.format) | ||
| self.output_file_path = await self.local_cache_provider.validate_path( | ||
| '/export/{}'.format(self.output_file_id) | ||
| '/export/{}.{}'.format(self.output_file_id, self.exporter_name) |
There was a problem hiding this comment.
I remember we did want to change something here during discussion but I forget the detail. My bad ... :-(.
There was a problem hiding this comment.
I think i remember, 👍
|
Really like the comments and doc str, very helpful 🎆 🎆 |
cslzchen
left a comment
cslzchen
left a comment
There was a problem hiding this comment.
Looks good and move to PCR 🎆 🎆
|
|
||
| # ep.attrs is a tuple of attributes. There should only ever be one or `None`. | ||
| # For our case however there shouldn't be `None` | ||
| return list(entry_attrs)[0].attrs[0] |
There was a problem hiding this comment.
list(entry_attrs)[0].attrs[0] always fails with index error.
There was a problem hiding this comment.
The original code works. However, entry_attrs is an iterator but is accessed in an array way. The first call to list(entry_attrs) moves the iteration forward, thus further access will fail with index error. This will be updated in #320.
3 participants
Ticket
https://openscience.atlassian.net/browse/SVCS-159
Purpose
Add renderer name to cached files
Add exporter name to cached files
Changes
Added renderer to cached renderer files
Added exporter name to cached exported files
Added tests for changes
Side effects
Should be none
QA Notes
In order to test this locally, you need to turn MFR caching on: https://github.com/CenterForOpenScience/modular-file-renderer/blob/develop/mfr/server/settings.py#L28
Once this is on, in your logs you will see the caching messages, and it will show you the names of the cached files as well.
The tests do not directly test this change in action, instead it tests to make sure that the added
get_exporter_nameandget_renderer_namereturn the proper values.Deployment Notes
Some things to note: While renderer/exporter names have been added to cached files, this isn't always indicative of which files should be cleared when changes are made to a specific renderer.
For instance a cached .docx file will have
.UnoconvRendererat the end of it, but it is rendered by both the.UnoconvRendererand thePdfRenderer.