Issue 3532: Linking to a collapsed review does not expand the review

Review Request #6332 — Created Sept. 17, 2014 and submitted

Information

Review Board
master
1671274...

Reviewers

Linking to a collapsed review does not expand the review. Fixed by parsing the url and force the linked box to open.

Tested on local server, add /#review{review_id} to url will trigger the corresponding box to open.

All unit tests passed.

Description From Last Updated

Empty line contains whitespace.

brenniebrennie

Use spaces instead of tabs.

brenniebrennie

var statements must always be at the top of the function. There should also only be one single var, like …

chipx86chipx86

Variable names are always namedLikeThis in our JavaScript code. No underscores.

chipx86chipx86

Space after if, and always have a single blank line separating other code and an if statement.

chipx86chipx86

This has tabs and not spaces. Likewise for the next few lines.

brenniebrennie

The .removeClass and .addClass should be indented 4 spaces relative to the element they're called on.

chipx86chipx86

list comprehension redefines 'file_attachment' from line 589

reviewbotreviewbot

These could be combined into a single statement since you only use url once.

daviddavid

Add a space between ) and {. Also, javascript comparisons should almost always use === instead of == Can you …

daviddavid

These are indented too much.

daviddavid

Since '#review' may not be in the URL, we should first check for the presence. I know JavaScript will (at …

chipx86chipx86

In JavaScript, you want to use === for comparison (though it'll require that the types be compatible, meaning you'll need …

chipx86chipx86
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        AUTHORS
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        AUTHORS
    
    
  2. 
      
NI
brennie
  1. 
      
  2. Show all issues

    Empty line contains whitespace.

  3. Show all issues

    Use spaces instead of tabs.

  4. You should also probably use a more descriptive review request title; using "fixed issue 3532" requires someone going to the bug tracker to find out about the change.

NI
NI
NI
NI
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        AUTHORS
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        AUTHORS
    
    
  2. 
      
NI
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        AUTHORS
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        AUTHORS
    
    
  2. 
      
NI
NI
brennie
  1. 
      
  2. Show all issues

    This has tabs and not spaces. Likewise for the next few lines.

  3. 
      
NI
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        AUTHORS
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        AUTHORS
    
    
  2. 
      
chipx86
  1. Some general comments involving coding style, but for this particular change, I thought we were going to fix this up in the Python code?

    The Python code (review_detail in reviewboard/reviews/views.py) controls the initial state of the boxes. It should be checking the #review and setting the matching collapsed state to False. This way, the page is rendered in the correct state.

    The Python code is already doing this for other things (like comparing timestamps and such) to set the default states, which is why we want it there.

  2. Show all issues

    var statements must always be at the top of the function. There should also only be one single var, like this:

    var foo = 1,
        bar = 2,
        s = '';
    
  3. Show all issues

    Variable names are always namedLikeThis in our JavaScript code. No underscores.

  4. Show all issues

    Space after if, and always have a single blank line separating other code and an if statement.

  5. reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 3)
     
     
     
     
    Show all issues

    The .removeClass and .addClass should be indented 4 spaces relative to the element they're called on.

  6. 
      
NI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        AUTHORS
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        AUTHORS
    
    
  2. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 589
    
  3. 
      
NI
NI
NI
RM
  1. Have you looked into Fragment Identifiers? As best I understand the part of the URL after the # isn't sent to the server. A plaintext dump of the url at the point you're working on should verify that.

  2. 
      
NI
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        AUTHORS
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        AUTHORS
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    These could be combined into a single statement since you only use url once.

  3. Show all issues

    Add a space between ) and {. Also, javascript comparisons should almost always use === instead of ==

    Can you also add a comment above this conditional explaining why we're expanding here?

    1. loadReviewID is a string while this.model.id is a number, so if I change "==" to "===" it won't work.

    2. Can we explicitly parseInt the loadReviewID?

  4. reviewboard/static/rb/js/views/reviewBoxView.js (Diff revision 6)
     
     
     
     
     
    Show all issues

    These are indented too much.

  5. 
      
NI
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        AUTHORS
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        AUTHORS
    
    
  2. 
      
chipx86
  1. 
      
  2. Show all issues

    Since '#review' may not be in the URL, we should first check for the presence. I know JavaScript will (at least on Chrome) just return undefined, but this code makes it look like '#review' is always expected to be there.

  3. Show all issues

    In JavaScript, you want to use === for comparison (though it'll require that the types be compatible, meaning you'll need to parseInt(loadReviewID, 10).

  4. 
      
NI
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        AUTHORS
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        AUTHORS
    
    
  2. 
      
david
  1. Ship It!

  2. 
      
NI
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (c5d577c)