Add visibility field to a ReviewRequestVisit

Review Request #7085 — Created March 19, 2015 and submitted

cristocrat
Review Board
master
7109
8648c99...
reviewboard, students

This adds a visibility field to the ReviewRequestVisit with three states: visible (V), archived (A), and muted (M). A database evolution, reviewrequestvisit_visibility, is also added.

Added to reviewboard.accounts.tests to test if VISIBLE was the default state of a newly created ReviewRequestVisit, all tests passed.

Checked database entries in the admin interface

Description From Last Updated

Single quotes for these.

brenniebrennie

Can you use tuples here?

brenniebrennie

(from David) Since you have parens, put the opening paren on the previous line instead of using \

CR cristocrat

Judging from previous code, we expect show_archived to be a boolean value. Why are we converting it to an integer ...

brenniebrennie

We can avoid using a backslash if we format thusly: parent_profile_changed = super(DashboardDataGrid, self).load_extra_state( profile, allow_hide_closed=False)

brenniebrennie

Col: 2 W292 no newline at end of file

reviewbotreviewbot

Blank line after this.

daviddavid

local variable 'response' is assigned to but never used

reviewbotreviewbot

This line can just be: self.client.get(review_request.get_absolute_url())

daviddavid

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Can you alphabetize these imports?

daviddavid

Because of the way that test docstrings get used for the test suite output, we don't put periods at the ...

daviddavid
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/models.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/models.py
    
    
  2. 
      
CR
CR
CR
  1. 
      
  2. reviewboard/datagrids/grids.py (Diff revision 1)
     
     

    (from David) Since you have parens, put the opening paren on the previous line instead of using \

  3. 
      
brennie
  1. 
      
  2. reviewboard/accounts/models.py (Diff revision 1)
     
     
     
     

    Single quotes for these.

    1. Sure, although I did see double quotes in reviews/review_request.py, which way is correct?

    2. We have used both single and double quotes in the past, but we are slowly transitioning the codebase to use only single quotes (except in exceptional circumstances). You should always use single quotes unless you need single quotes in the quoted string.

  3. reviewboard/accounts/models.py (Diff revision 1)
     
     

    Can you use tuples here?

    1. Could only get it to work with index_together = [('user', 'visibility')] (still in a list), is this okay?

    2. This is fine.

  4. reviewboard/datagrids/grids.py (Diff revision 1)
     
     
     
     
     
     
     
     

    Judging from previous code, we expect show_archived to be a boolean value. Why are we converting it to an integer and comparing it with zero?

    Also, we are using self.show_archived as a default value for request.GET.get, but in the except block, we have default_show_archived. We should probably use default_show_archived in both situations.

    1. For the int, I was following the example in load_extra_state near the top of the file. Should I change it there too? It makes sense keeping it boolean...

      For your 2nd comment, doesn't seem to work. The show-archived argument gets stuck at 1 and doesn't toggle

    2. For some reason, my comment didn't get published when I responded to your other comments :(

      I wouldn't worry about about the stuff not included in your changes. Also, how do mean it doesn't work. Does supplying the 'show-archived' parameter not do anything when the default value is default_show_archived ?

    3. Actually, you can ignore the int vs boolean argument? I understand now why we are using an int.

    4. This issue can be dropped now.

    5. okay. it's now being discussed in 7091, but Barret's suggestion doesn't seem to be working. would you mind taking a look at it? thanks

  5. reviewboard/datagrids/grids.py (Diff revision 1)
     
     
     
     

    We can avoid using a backslash if we format thusly:

            parent_profile_changed = super(DashboardDataGrid,
                                           self).load_extra_state(
                                               profile,
                                               allow_hide_closed=False)
    
  6. 
      
david
  1. I know it's kind of annoying to swap around, but I don't think the dashboard code should be part of this review request :(

  2. 
      
CR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/models.py
    
    
  2. Col: 2
     W292 no newline at end of file
    
  3. 
      
CR
CR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/models.py
    
    
  2. 
      
CR
CR
CR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/tests.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/tests.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/models.py
    
    
  2. reviewboard/accounts/tests.py (Diff revision 4)
     
     
     local variable 'response' is assigned to but never used
    
  3. 
      
CR
david
  1. 
      
  2. reviewboard/accounts/tests.py (Diff revision 4)
     
     

    Blank line after this.

    1. okay, should I add blank lines throughout the file where this happens then?

    2. Don't worry about existing code. I've got a long-term project to go through and clean up our docstrings. I just want to make sure new code that we're adding follows the right rules.

  3. reviewboard/accounts/tests.py (Diff revision 4)
     
     

    This line can just be:

    self.client.get(review_request.get_absolute_url())
    
  4. 
      
CR
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/tests.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/models.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/tests.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/models.py
    
    
  2. reviewboard/accounts/tests.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. 
      
CR
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/tests.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/models.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/tests.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/models.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/accounts/tests.py (Diff revision 6)
     
     
     
     
     

    Can you alphabetize these imports?

  3. reviewboard/accounts/tests.py (Diff revision 6)
     
     

    Because of the way that test docstrings get used for the test suite output, we don't put periods at the end of them.

  4. 
      
CR
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/tests.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/models.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/tests.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/models.py
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
CR
CR
CR
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to ucosp/cristocrat/archive-and-mute (426500d)
Loading...