-
-
reviewboard/attachments/mimetypes.py (Diff revision 1) Can also write this as to make it a bit shorter: if extension.lower() == ".rst"
Implement Thumbnail Rendering for Text-ish File Attachments (.rst and .md)
Review Request #3454 — Created Oct. 24, 2012 and submitted
Implemented thumbnail rendering for ReStructuredText (.rst) and MarkDown (.md) file attachment types. The rendered thumbnails are displayed as HTML scaled down by css (instead of rendered as image thumbs), and are mem cached. Also changed the way MimetypeHandlers are registered to match existing infrastructure for registering Review UIs, replacing the old way of doing it through __subclass__
1. Ran unit tests for reviewboard.attachments.tests: - Updated tests now all pass after adding setUp() and tearDown() to register / unregister the test Mimetype Handlers. 2. Manual testing on localhost: Tested by uploading to a new review-request: - raw .txt file - raw .rst file - raw .md file - raw .jpg file - raw .rst file with javascript injection - raw .md file with javascript injection Visual inspection of the rendered thumbnails all pass: - Thumbnails appear immediately (without a refresh), reflecting latest feature updates to master. - Scaling, cropping and rendering all appear to be correct. - Javascript injection correctly escaped for malicious .rst and .md files See updated screenshots (2012-11-24, in the files attachment section)
Description | From | Last Updated |
---|---|---|
imports are a bit wrong. The froms should all be together and the imports should all be together. They also … |
chipx86 | |
This is probably not the right place to do this. We should probably define a new map below MIMETYPE_ICON_ALIASES that … |
chipx86 | |
No blank line. |
chipx86 | |
I'm not sure we want to escape this here? A better name would be "data". |
chipx86 | |
Should use format strings, and no camel casing. You can also combine with the below. return mark_safe('%s%s%s' % (rst_parts['title'], ......) … |
chipx86 | |
Same comments from above apply. |
chipx86 | |
Can you limit the number of lines actually shown? Maybe do splitlines()[:5] on the result. |
chipx86 | |
Should be: h1, h2, body, p { font-size: 30%; } |
chipx86 | |
Two blank lines. |
chipx86 | |
This comment should be made more generic, since this is no longer just about .rst files. |
chipx86 | |
Blank line between these. |
chipx86 | |
You can just combine these two lines. |
chipx86 | |
Combine these too. |
chipx86 | |
Same comments as above. |
chipx86 | |
Two blank lines. |
chipx86 | |
No blank line here. |
chipx86 | |
Blank line. |
chipx86 | |
So the issue with this is that it'll affect units for margins, padding, etc. of anything contained within. I'm concerned … |
chipx86 | |
Should be moved above the 'from'. The imports should be grouped together and from statements should be grouped together (as … |
AA aam1r | |
This is all done below. It should only be returned once. However.. "file is closed" is pretty useless to all … |
chipx86 | |
That doesn't really describe what the function does. """Returns the HTML for a thumbnail preview for a ReST file.""" would … |
chipx86 | |
Not really a useful comment. That's pretty clear from the code. |
chipx86 | |
self.FILE_CROP_CHAR_LIMIT |
chipx86 | |
This should all be returned only once. Same comment as above about logging and a default. |
chipx86 | |
Same comments for this file. |
chipx86 | |
You should import all these in one go, not with different "from ..." lines. |
chipx86 | |
Two blank lines. |
chipx86 | |
It'd be nice to keep this alphabetical. Is the MimetypeHandler registration needed? |
chipx86 | |
Can you prefix with a _ ? That indicates it's not a public function. Same with all others introduced that … |
chipx86 | |
This doesn't need to go into specifics on how it's built, and probably shouldn't. Just say what it does. Also, … |
chipx86 | |
It's a bit.. verbose. How about just generate_preview_html? |
chipx86 | |
Should probably be on the class itself. |
chipx86 | |
Use splitlines. |
chipx86 | |
One statement. |
chipx86 | |
We always import from django.conf.settings. It should be: from django.conf import settings However, you don't want this. See my comment … |
chipx86 | |
Can we call these TEXT_CROP_NUM_LINES and TEXT_CROP_LINE_LENGTH? I think it would be a little more clear. |
david | |
Whitespace |
david | |
This comment really doesn't add much. |
david | |
This doesn't really fit our other logs. Instead, do: "Failed to read from file attachment %s: %s' % (self.attachment.pk, e) … |
chipx86 | |
You should be able to just provide self._generate_thumbnail as the function without a lambda. |
chipx86 | |
Rather than call this yourself, I think you should probably use Django's restructuredtext filter. You can import it from django.contrib.markup.templatetags. … |
chipx86 | |
You never need to supply \ when it's in parens. Also, you have some indentation problems. |
chipx86 | |
Indentation problem. |
chipx86 | |
This should absolutely use Django's markdown filter (importable from the same place as above). That does all sorts of very … |
chipx86 | |
Can you say "file extensions"? I had a moment of confusion where I thought "reviewboard extensions" |
david | |
Is this really something that we expect to ever want to change/override? It seems like it would be better to … |
david | |
Should be: BLAHBLAH = { 'file_insertion_enabled': 0, ... } There should be a trailing comma on all entries. I also … |
chipx86 | |
Given that we're going to call publish_parts ourselves, we should just inline these settings and force them, rather than have … |
chipx86 | |
What we probably want to do is just take the very latest version of markdown that's out right now and … |
chipx86 | |
The formatting here could use some improvement. I'd also like to pull the settings dict inline (it's only used in … |
david |
Change Summary:
Updated Review-Request: Fixed imports and streamlined rst ext case check
Summary: |
|
|||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+53 -1) |
Change Summary:
Added thumbnail rendering for MarkDown (.md) file attachments. Tested locally and everything appears to work properly. Next steps will be to optimize the rendering process for thumbnails (cropping raw text before rendering using docutil / markdown libs)
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+52 -2) |
Change Summary:
Added "zoomed out" thumbnails for RST and MD. The thumbnails for these file attachment types now render at 30% of normal size in addition to being cropped.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+68 -2) |
-
-
reviewboard/attachments/mimetypes.py (Diff revision 4) imports are a bit wrong. The froms should all be together and the imports should all be together. They also need to be alphabetical. djblets is a third-party dependency (as far as RB is concerned), os it should be with the others above. No blank line. The docutils import should probably just do an import of docutils.core and docutils.io. Importing core and io will make code harder to read. The names are just too common.
-
reviewboard/attachments/mimetypes.py (Diff revision 4) This is probably not the right place to do this. We should probably define a new map below MIMETYPE_ICON_ALIASES that maps file extensions to mimetypes. Then we can do: extension = os.path.splitext(attachment.filename)[1].lower() if extension in MIMETYPE_EXTENSIONS: mimetype = MIMETYPE_EXTENSIONS[extension] And that should be above the original parse_mime_type call.
-
-
reviewboard/attachments/mimetypes.py (Diff revision 4) I'm not sure we want to escape this here? A better name would be "data".
-
reviewboard/attachments/mimetypes.py (Diff revision 4) Should use format strings, and no camel casing. You can also combine with the below. return mark_safe('<div class="file-thumbnail-clipped">%s%s%s</div>' % (rst_parts['title'], ......) What happens if we have a document without those bits? Can that ever happen?
-
-
reviewboard/attachments/mimetypes.py (Diff revision 4) Can you limit the number of lines actually shown? Maybe do splitlines()[:5] on the result.
-
reviewboard/static/rb/css/reviews.less (Diff revision 4) Should be: h1, h2, body, p { font-size: 30%; }
Change Summary:
Polished thumbnail rendering for .rst and .md: Changes made as per reviews. Added screenshot and the raw files used in the review-request locally (note that the html files were not used, but only included for comparison purposes)
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+86 -2) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Screenshots: |
|
-
Think we're just about there! There's some minor style stuff (mostly where blank lines should go, and some statements that could be easily combined).
-
-
reviewboard/attachments/mimetypes.py (Diff revision 5) This comment should be made more generic, since this is no longer just about .rst files.
-
-
-
-
-
-
-
-
reviewboard/static/rb/css/reviews.less (Diff revision 5) So the issue with this is that it'll affect units for margins, padding, etc. of anything contained within. I'm concerned about that. I'd much rather we be explicit about the types we care about for now.
Change Summary:
- Adjusted thumbnail rendering + fixed style issues - Updated screenshot
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 6 (+88 -1) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Screenshots: |
|
-
-
reviewboard/attachments/mimetypes.py (Diff revision 6) I wonder, what's the trade off between capping the max_length while reading the file in and reading the whole file then parsing it?
-
reviewboard/attachments/mimetypes.py (Diff revision 6) Just curious, do you know when will the efficiency difference be not negligible when running functions like this? What will happen if it loads a very large troll file with no line break at all (or one at the very end)?
-
reviewboard/attachments/mimetypes.py (Diff revision 6) Do you think it would be better if 200 is a variable instead? (like max_length) Should it be the same length for all text files supported?
-
-
reviewboard/attachments/mimetypes.py (Diff revision 6) Should be moved above the 'from'. The imports should be grouped together and from statements should be grouped together (as Christian pointed out earlier)
Change Summary:
Fixed file att. cropping for thumbnails - Fixed file attachment cropping for thumbnails - Previously: read entire file attachment, then slice the first 200 lines into a string to be parsed by docutil or markdown. This opens a potential vulnerability when reading files with large column counts. - Now: read up to the first `max_chars` characters from the file. `max_chars` is currently set to 2000. - Expanded supported mimetypes for rst / md handlers to include 'text/rst` and 'text/markdown` for forward compatibility in the event they become official mimetypes in the future.
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 7 (+90 -1) |
Change Summary:
Implemented mem caching for textish thumbnails - Implemented mem caching for ReStructuredTextMimeType and for MarkDownMimeType with cache_memoize - The processed html thumbnails are cached, so this saves on: 1) not having to re-read the raw text file each time 2) not having to re-generate the html thumbnail each time - During the course of implementing a unique key construction for caching the file attachment thumbnails, a bug was discovered: when deleting a file attachment, get_thumbnail() is somehow invoked, and this leads to a 500 internal server error as TextMimetype tries to read a file that is closed (doesn't exist). An issue will be opened separately to address this. For now, try-except blocks are set up for TextMimetype, ReStructuredTextMimeType, MarkDownMimeType to bandaid the issue in order to prevent the thumbnails projects from blocking. - Fixed some style issues with variable names, class-vs.-instance types, parameter alignments, etc.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 8 (+128 -4) |
Change Summary:
- Fixed style issues. - Corrected review description to reflect what the entire commit is about.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 9 (+128 -4) |
-
So I'm noticing a lot of this code is just duplicated. We should really do something about that. Here's what I propose. TextMimetype says "Here's how to handle a text mimetype." That should really be the basis. The ReST and Markdown ones are text renderers that know specialized types of text files. So they should really subclass TextMimetype. TextMimetype should have a function that, given a blob of text, will returned the appropriate HTML. By default, it'd just return the text, escaped. The other handlers could override to do their thing. One thing that might trip you up. Right now, mimetype class finding is using __subclasses__ to get the mimetype, which we really don't want long-term, and fails here. So that will need to basically do what ReviewUIs do, with the register/unregister functions. I'd recommend making a separate change before this goes in that does exactly that. Removes the __subclasses__ iteration and instead introduces a _registered_mimetype_handlers list that it iterates. Then (un)register_mimetype_handler functions that manipulate. And something just like reviews/ui/__init__.py in attachments/__init__.py to do the registration. You should be able to just do that separately (in a new branch off master, without these changes), test it with text mimetypes, and then post for review. You can also move this branch on top of that one until it goes in, so you can consolidate these classes and use the new registration functions.
-
reviewboard/attachments/mimetypes.py (Diff revision 9) This is all done below. It should only be returned once. However.. "file is closed" is pretty useless to all users. They'll just go, "what?" This is an error. It should be logged, with enough context for us to know what failed and where, and the default thumbnail should be used instead.
-
reviewboard/attachments/mimetypes.py (Diff revision 9) That doesn't really describe what the function does. """Returns the HTML for a thumbnail preview for a ReST file.""" would be more what we'd want to see. Imagine you were viewing API docs, and only had function names and summaries. Write what you'd need to be able to easily find the appropriate function for your needs.
-
reviewboard/attachments/mimetypes.py (Diff revision 9) Not really a useful comment. That's pretty clear from the code.
-
-
reviewboard/attachments/mimetypes.py (Diff revision 9) This should all be returned only once. Same comment as above about logging and a default.
-
Change Summary:
Revised review-request description and testing information
Description: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Change Summary:
Refactored duplicate code to class TextMimeType - Refactored a lot of the duplicate code between the ReStructuredTextMimeType and MarkDownMimeType into TextMimeType and had them inherit from TextMimeType instead - Encapsulated key generation in its own method - Encapsulated generation of the HTML thumbnail based on string input into a method (to be overridden by .rst / .md handlers) Added mt handler registering to replace __subclass - Implemented infrastructure for registering MimetypeHandlers in much the same manner as registering Review UIs - Replaced the old way of doing it through __subclass__
Description: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 10 (+177 -23) |
||||||||||||||||||||||||||||||||||||||||||
Screenshots: |
|
-
-
reviewboard/attachments/__init__.py (Diff revision 10) You should import all these in one go, not with different "from ..." lines.
-
-
reviewboard/attachments/__init__.py (Diff revision 10) It'd be nice to keep this alphabetical. Is the MimetypeHandler registration needed?
-
reviewboard/attachments/mimetypes.py (Diff revision 10) Can you prefix with a _ ? That indicates it's not a public function. Same with all others introduced that aren't part of the ReviewUI API or something that needs to be overridden in a subclass. Also, this function should be more specifically named. As it is, I have no idea what it's a cache key for, and in theory, we might want multiple keys. As far as I can tell, it's only used in one place, so many just keep that logic there unless a subclass has to override this.
-
reviewboard/attachments/mimetypes.py (Diff revision 10) This doesn't need to go into specifics on how it's built, and probably shouldn't. Just say what it does. Also, "&" shouldn't be used, and it should terminate in a period.
-
reviewboard/attachments/mimetypes.py (Diff revision 10) It's a bit.. verbose. How about just generate_preview_html?
-
-
-
Change Summary:
Added protection against javascript injection as per ChipX86's note in IRC. Included screenshot from the latest changes (Thumbnail rendering of .rst and .md files with js injected code)
Diff: |
Revision 13 (+186 -26) |
||
---|---|---|---|
Added Files: |
|||
Screenshots: |
|
Change Summary:
Updated testing information
Testing Done: |
|
---|
-
-
reviewboard/attachments/mimetypes.py (Diff revision 13) Can we call these TEXT_CROP_NUM_LINES and TEXT_CROP_LINE_LENGTH? I think it would be a little more clear.
-
-
-
reviewboard/attachments/mimetypes.py (Diff revision 13) Can you say "file extensions"? I had a moment of confusion where I thought "reviewboard extensions"
-
reviewboard/settings.py (Diff revision 13) Is this really something that we expect to ever want to change/override? It seems like it would be better to just inline it in the code where it's used.
-
-
reviewboard/attachments/mimetypes.py (Diff revision 13) We always import from django.conf.settings. It should be: from django.conf import settings However, you don't want this. See my comment below.
-
reviewboard/attachments/mimetypes.py (Diff revision 13) This doesn't really fit our other logs. Instead, do: "Failed to read from file attachment %s: %s' % (self.attachment.pk, e) Also note that you don't need to do str(...), since %s does that for you.
-
reviewboard/attachments/mimetypes.py (Diff revision 13) You should be able to just provide self._generate_thumbnail as the function without a lambda.
-
reviewboard/attachments/mimetypes.py (Diff revision 13) Rather than call this yourself, I think you should probably use Django's restructuredtext filter. You can import it from django.contrib.markup.templatetags. That'll reference RESTRUCTUREDTEXT_FILTER_SETTINGS itself, and do all the other right things. One thing though is that that will return parts["fragment"], not parts["html_body"]. I'm curious what that looks like and how it's different.
-
reviewboard/attachments/mimetypes.py (Diff revision 13) You never need to supply \ when it's in parens. Also, you have some indentation problems.
-
-
reviewboard/attachments/mimetypes.py (Diff revision 13) This should absolutely use Django's markdown filter (importable from the same place as above). That does all sorts of very important security checks that we do not. Certain versions of markdown have different vulnerabilities, and Django has a lot of work to check for those. We want this.
-
reviewboard/settings.py (Diff revision 13) Should be: BLAHBLAH = { 'file_insertion_enabled': 0, ... } There should be a trailing comma on all entries. I also think this should be up higher, out of the way. Maybe right under WEB_API_ENCODERS.
Change Summary:
Added version checks for markdown - Added version checks for markdown to mimick the version checking for markdown done by the to-be deprecated contrib.markup module in Django - Added similar checks for restructuredtext - Fixed various style issues - Uploaded screenshot to compare what rst thumbnails look like when "html_body" is specified in the docutil API vs. "fragments"
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 14 (+211 -25) |
||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
-
-
reviewboard/attachments/mimetypes.py (Diff revision 14) Given that we're going to call publish_parts ourselves, we should just inline these settings and force them, rather than have them in the settings file.
-
reviewboard/attachments/mimetypes.py (Diff revision 14) What we probably want to do is just take the very latest version of markdown that's out right now and just make that a required dependency in setup.py. Django has to be more flexible because it doesn't depend on setup.py, but we can ensure we have exactly what we're expecting. That'll let us get rid of all these version checks.
Change Summary:
Added markdown version requirement for RB: - As per ChipX86's comments, added latest current markdown version to RB's list of required dependencies - Removed RESTRUCTUREDTEXT_FILTER_SETTINGS from settings.py and inlined the settings as a class dictionary for ReStructuredTextMimetype - Fixed minor style issue to prevent settings.py from showing up in git diff
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 15 (+186 -25) |
-
-
reviewboard/attachments/mimetypes.py (Diff revision 15) The formatting here could use some improvement. I'd also like to pull the settings dict inline (it's only used in one place): Also, in the settings dict, should the 0s and 1s be False and True? settings = { 'file_insertion_enabled': 0, 'raw_enabled': 0, '_disable_config': 1, } return docutils.core.publish_parts( source=smart_str(data_string), writer_name='html4css1', settings_overrides=settings)['html_body']
Change Summary:
Fixed style issues in mimetypes.py - Moved RESTRUCTUREDTEXT_FILTER_SETTINGS inline and renamed it to settings - Fixed return statement alignment issues in ReStructuredTextMimetype
Diff: |
Revision 16 (+186 -25) |
---|
Change Summary:
Converted 0/1's to True/False's for rst settings - Minor change, see comments here for more information: http://reviews.reviewboard.org/r/3454/#comment7581
Diff: |
Revision 17 (+186 -25) |
---|
Change Summary:
Revised attachment tests for thumbnails project - Since we changed how MimetypeHandler finds the the best fit handler for a particular mimetype (Old way: through __subclass__; New way: through explicitly registering a subclass of MimetypeHandler), attachment/tests.py needed up be updated to register the test handler used in the tests. - Added the registering / unregistering of the test handlers in setUp() and tearDown() functions
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 18 (+216 -26) |
-
-
reviewboard/attachments/mimetypes.py (Diff revision 18) This is probably too late since this is already merged in but "enable_attributes" default to False when safe_mode is enabled. Docs: http://packages.python.org/Markdown/reference.html#enable_attributes