Status Updates part 9: centralize code to expand boxes to initial hash.

Review Request #8446 — Created Sept. 28, 2016 and submitted

Information

Review Board
release-3.0.x
e3620a0...

Reviewers

When loading a page that has something like "#review20" in the window location,
we had some code to expand the review box that matched. The addition of reviews
within the initial status reports and change description boxes duplicated this
code two additional times.

This change moves that responsibility into the ReviewRequestPageView. This
now just generically searches each box for a matching selector, expanding if it
finds it. This means that it's much more general, and linking to individual
comments will also expand their container.

Checked that the matching boxes were correctly expanded when loading the page
with a variety of different hash values.

Description From Last Updated

Probably don't want to leave this in?

chipx86chipx86

Don't we always use block comments in JS?

brenniebrennie

Do we want to have a $ at the end here?

brenniebrennie
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/changeBoxView.es6.js
        reviewboard/static/rb/js/views/reviewBoxView.es6.js
        reviewboard/static/rb/js/views/initialStatusUpdatesBoxView.es6.js
        reviewboard/static/rb/js/pages/views/reviewRequestPageView.es6.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/changeBoxView.es6.js
        reviewboard/static/rb/js/views/reviewBoxView.es6.js
        reviewboard/static/rb/js/views/initialStatusUpdatesBoxView.es6.js
        reviewboard/static/rb/js/pages/views/reviewRequestPageView.es6.js
    
    
  2. 
      
chipx86
  1. 
      
  2. I don't really like trusting user input for a selector like this. While I don't know of any attack vectors (jQuery at least will block HTML tags in a $(...)), I feel we should probably sanity-check what we get here. We can do this pretty effectively with:

    box.render();
    
    const selector = window.location.hash.match(/^#[A-Za-z0-9_\.-]+/);
    
    if (selector && box.$(selector[0]).length > 0) {
        ...
    }
    
    1. Oops, an issue was supposed to be open here. I bumped something.

  3. Probably don't want to leave this in?

  4. 
      
david
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/changeBoxView.es6.js
        reviewboard/static/rb/js/views/reviewBoxView.es6.js
        reviewboard/static/rb/js/views/initialStatusUpdatesBoxView.es6.js
        reviewboard/static/rb/js/pages/views/reviewRequestPageView.es6.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/changeBoxView.es6.js
        reviewboard/static/rb/js/views/reviewBoxView.es6.js
        reviewboard/static/rb/js/views/initialStatusUpdatesBoxView.es6.js
        reviewboard/static/rb/js/pages/views/reviewRequestPageView.es6.js
    
    
  2. 
      
brennie
  1. 
      
  2. Don't we always use block comments in JS?

    1. Only if the comment spans more than one line (or is a doc comment).

  3. Do we want to have a $ at the end here?

  4. 
      
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (c806bf3)
Loading...