• 
      

    Convert the review request page's view to a class-based view.

    Review Request #9001 — Created June 6, 2017 and submitted

    Information

    Review Board
    release-3.0.x
    0e8b801...

    Reviewers

    The review request page is complex, with a lot of information being
    calculated and used to compute ETags and render the pages. As we iterate
    on the capabilities of this page, the view just gets more and more
    complicated.
    
    This change rewrites the view as a class-based view, making use of a
    handy new mixin for review request pages (such as the diff viewer and
    file attachment review pages -- though none of those use this mixin
    yet). This view contains versions of the handy private functions used to
    look up review requests and diffs, check for access permissions, and
    perform common template context calculations.
    
    The new view breaks up some of the logic into helper methods. It's
    better organized and can be expanded later in a more clean way.
    
    Future changes will move more classes to the new mixin and to
    class-based views.

    Tested that the review request page works. Tested actions, file
    file attachments, reviews, status updates, replies, editing the
    review request, and tracking visited state.

    Description From Last Updated

    F821 undefined name 'context_data'

    reviewbotreviewbot

    Can we just always use self.data? It doesn't save much in either speed or characters to also keep a local …

    daviddavid
    chipx86
    chipx86
    Review request changed
    Change Summary:

    Changed the default implementation of ReviewRequestViewMixin.get_context_data to be more generally useful.

    Commit:
    5049cf425d47f750067c05ffe3a45d555a65a8b7
    d69a3b7d030c0d5758a3c8e5eaa4609dd39618c2

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    chipx86
    chipx86
    david
    1. 
        
    2. reviewboard/reviews/views.py (Diff revision 5)
       
       
       
       
       
      Show all issues

      Can we just always use self.data? It doesn't save much in either speed or characters to also keep a local variable.

      1. Given that this is probably the most-accessed view in the product, I'll take whatever speed benefits I can get. While attribute lookups are not the slowest thing in the world, if we don't need to do them, them why add that overhead. We're computing the data locally, and setting it for other methods, but we might as well keep accessing it locally instead of going through the __getattr__/dict lookup machinery every time we do anything on it, since data is accessed a lot.

    3. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-3.0.x (6e64ae6)