• 
      

    Markdown Review UI

    Review Request #4414 — Created Aug. 10, 2013 and submitted

    Information

    Review Board
    master

    Reviewers

    Markdown Review UI
    
    This adds functionality to render Markdown file attachments in Review Board.
    
    The rendering is handled by the "markdown" Python package which, given a file,
    returns the HTML code for it. This HTML code is then passed to the Backbone
    view for the Markdown Pluggable UI through the template and rendered in the
    browser.
    
    This is based on the change at https://reviews.reviewboard.org/r/3434/
    The major changes are:
    - Port to the new 1.8.x JS model/view parameters and remove the extra template
      file.
    - Use safe_mode='escape' when rendering to disallow users from using arbitrary
      HTML tags.
    - Add comment thumbnails, so we get excerpts in the reviews page. I can't
      decide if using minidom to parse the outputted HTML and extract a specific
      node is awesome or awful, but it seems to work very reliably and keeps us
      secure.
    - Cache the rendering of the markdown file, which comes in handy for those
      comment thumbnails.
    
    "Valid" Case:
    1) Created a review request and uploaded multiple random files to it
    2) Then, uploaded a valid Markdown file (with the extension .md) to that review
       request
    3) Clicked the "Review" link for the Markdown file
    4) The new page loaded should render the Markdown file
    
    "Invalid" Case:
    1) Created a review request and uploaded multiple random files to it
    2) Then, uploaded an image to that review request by changed the file extension
       to be ".md". For example, wallpaper.png -> wallpaper.md
    3) Clicked the "Review" link for the invalid Markdown file
    4) The new page loaded did not render anything (i.e. it displayed the title and
       caption but nothing was displayed in the section where the HTML would have
       rendered)
    Description From Last Updated

    Can you add a docstring for this?

    chipx86 chipx86

    Should be able to just pass self._render, instead of wrapping in a lambda.

    chipx86 chipx86

    We should cache this too.

    chipx86 chipx86

    These are expensive enough where we should do some more caching. At the very least, the entire comment thumbnail should …

    chipx86 chipx86
    reviewbot
    1. This is a review from Review Bot.
        Tool: PEP8 Style Checker
        Processed Files:
          reviewboard/reviews/ui/__init__.py
          reviewboard/reviews/ui/base.py
          reviewboard/settings.py
          reviewboard/reviews/ui/markdownui.py
        Ignored Files:
          reviewboard/static/rb/js/models/markdownReviewableModel.js
          reviewboard/static/rb/js/views/markdownReviewableView.js
      
      
    2. 
        
    reviewbot
    1. This is a review from Review Bot.
        Tool: Pyflakes
        Processed Files:
          reviewboard/reviews/ui/__init__.py
          reviewboard/reviews/ui/base.py
          reviewboard/settings.py
          reviewboard/reviews/ui/markdownui.py
        Ignored Files:
          reviewboard/static/rb/js/models/markdownReviewableModel.js
          reviewboard/static/rb/js/views/markdownReviewableView.js
      
      
    2. 
        
    chipx86
    1. 
        
    2. reviewboard/reviews/ui/markdownui.py (Diff revision 1)
       
       
      Show all issues
      Can you add a docstring for this?
    3. reviewboard/reviews/ui/markdownui.py (Diff revision 1)
       
       
      Show all issues
      Should be able to just pass self._render, instead of wrapping in a lambda.
    4. reviewboard/reviews/ui/markdownui.py (Diff revision 1)
       
       
       
       
       
       
      Show all issues
      We should cache this too.
    5. reviewboard/reviews/ui/markdownui.py (Diff revision 1)
       
       
       
      Show all issues
      These are expensive enough where we should do some more caching. At the very least, the entire comment thumbnail should be cached. Maybe the document, if it's a large document, assuming that's cacheable.
      1. Given that xml.dom wraps either libxml or expat, I don't think it's easily cacheable. I suppose I could do a toxml() on each sub-element and cache those, but I think parsing the X(HT)ML is going to be pretty fast compared to converting the markdown. I'll cache each thumbnail, though.
    6. 
        
    david
    reviewbot
    1. This is a review from Review Bot.
        Tool: PEP8 Style Checker
        Processed Files:
          reviewboard/reviews/ui/__init__.py
          reviewboard/reviews/ui/base.py
          reviewboard/settings.py
          reviewboard/reviews/ui/markdownui.py
        Ignored Files:
          reviewboard/static/rb/js/models/markdownReviewableModel.js
          reviewboard/static/rb/js/views/markdownReviewableView.js
      
      
    2. 
        
    reviewbot
    1. This is a review from Review Bot.
        Tool: Pyflakes
        Processed Files:
          reviewboard/reviews/ui/__init__.py
          reviewboard/reviews/ui/base.py
          reviewboard/settings.py
          reviewboard/reviews/ui/markdownui.py
        Ignored Files:
          reviewboard/static/rb/js/models/markdownReviewableModel.js
          reviewboard/static/rb/js/views/markdownReviewableView.js
      
      
    2. 
        
    chipx86
    1. Code looks fine. We should add docstrings to the public functions, though.
    2. 
        
    david
    reviewbot
    1. This is a review from Review Bot.
        Tool: PEP8 Style Checker
        Processed Files:
          reviewboard/reviews/ui/__init__.py
          reviewboard/reviews/ui/base.py
          reviewboard/settings.py
          reviewboard/reviews/ui/markdownui.py
        Ignored Files:
          reviewboard/static/rb/js/models/markdownReviewableModel.js
          reviewboard/static/rb/js/views/markdownReviewableView.js
      
      
    2. 
        
    reviewbot
    1. This is a review from Review Bot.
        Tool: Pyflakes
        Processed Files:
          reviewboard/reviews/ui/__init__.py
          reviewboard/reviews/ui/base.py
          reviewboard/settings.py
          reviewboard/reviews/ui/markdownui.py
        Ignored Files:
          reviewboard/static/rb/js/models/markdownReviewableModel.js
          reviewboard/static/rb/js/views/markdownReviewableView.js
      
      
    2. 
        
    chipx86
    1. Ship It!
    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (a66a32b).