Add a Dashboard toggle to show or hide archived review requests

Review Request #7109 — Created March 23, 2015 and submitted

cristocrat
Review Board
master
7085
7087
reviewboard, students

This adds a toggle on the Dashboard datagrid to show or hide archived review requests, similar to that of the All Review Requests datagrid for showing and hiding closed review requests.

  • a new template block is loaded in the dashboard to expose the toggle button
  • when the pressed, a show-archived query is appended to the existing URL query
  • show-archived=0 filters the archived review requests (those that are archived or muted)
  • show-archived=1 includes all review requests, regardless of their state of visibility

NB: When show-archived is not specified, its default value of is 0.

Manual tests: Archived and muted several review requests. In the dashboard, they disappeared when setting show-archived=0 and reappeared when setting show-archived=1

Unit tests: Added to reviewboard.datagrids.tests to test if archived review requests would appear in the datagrid for a particular user for show-archived=0 and show-archived=1. Also tested for a different user who had not archived any of the same review requests. All tests passed.

Loading file attachments...

Description From Last Updated

Could you alphabetize these?

brenniebrennie

I have the same question here as in the other review request about why we aren't using default_show_hidden as the ...

brenniebrennie

Can you chang the formatting so we don't use a backslash, e.g. parent_profile_changed = super(DashboardDataGrid, self).load_extra_state( profile, allow_hide_closed=False)

brenniebrennie

Will {{datagrid.queries}} expand to have a & in it if there are queries?

brenniebrennie

Can we move this down to be in an else clause of the if profile and self.show_hidden != default_show_hidden: conditional?

daviddavid

In order to address Barret's question, how about this, which looks somewhat more similar to ShowClosedReviewRequestsMixin if profile and 'show_hidden' ...

daviddavid

Can we wrap this a little differently? parent_profile_changed = \ super(DashboardDataGrid, self).load_extra_state( profile, allow_hide_closed=False)

daviddavid

Can we do this in DashboardDataGrid.__init__ instead?

daviddavid

This shouldn't be indented at all.

daviddavid

I think we should call these "Show/Hide archived" rather than "hidden". It's true that it isn't totally correct (since it ...

daviddavid

Instead of using \, can you add parens around this and wrap after and? if (profile and self.show_archived != profile.extra_data.get('show_archived')):

daviddavid

This is the only change in this file now--can you revert it?

daviddavid

Col: 13 E113 unexpected indentation

reviewbotreviewbot

Col: 13 E112 expected an indented block

reviewbotreviewbot

I think it's just under 80 columns if you were to put this all on one line.

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/datagrids/views.py
    
    Ignored Files:
        reviewboard/templates/datagrids/hideable_listview.html
        reviewboard/static/rb/css/common.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/datagrids/views.py
    
    Ignored Files:
        reviewboard/templates/datagrids/hideable_listview.html
        reviewboard/static/rb/css/common.less
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/datagrids/grids.py (Diff revision 1)
     
     

    Could you alphabetize these?

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

    I have the same question here as in the other review request about why we aren't using default_show_hidden as the default parameter.

    1. When I change it to try: self.show_hidden = int(default_show_hidden) != 0, the show-hidden=1 in the URL never switches to show-hidden=0. I think default_show_hidden isn't used because try block needs to get the show-hidden value of the URL query and not the default value (False) of the datagrid

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

    Can you chang the formatting so we don't use a backslash, e.g.

            parent_profile_changed = super(DashboardDataGrid,
                                           self).load_extra_state(
                                               profile,
                                               allow_hide_closed=False)
    
  5. Will {{datagrid.queries}} expand to have a & in it if there are queries?

  6. 
      
CR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/datagrids/views.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/datagrids/tests.py
    
    Ignored Files:
        reviewboard/templates/datagrids/hideable_listview.html
        reviewboard/static/rb/css/common.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/datagrids/views.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/datagrids/tests.py
    
    Ignored Files:
        reviewboard/templates/datagrids/hideable_listview.html
        reviewboard/static/rb/css/common.less
    
    
  2. 
      
CR
CR
david
  1. 
      
  2. reviewboard/datagrids/grids.py (Diff revision 2)
     
     

    Can we move this down to be in an else clause of the if profile and self.show_hidden != default_show_hidden: conditional?

  3. reviewboard/datagrids/grids.py (Diff revision 2)
     
     
     
     
     

    Can we wrap this a little differently?

    parent_profile_changed = \
        super(DashboardDataGrid, self).load_extra_state(
            profile, allow_hide_closed=False)
    
  4. reviewboard/datagrids/views.py (Diff revision 2)
     
     

    Can we do this in DashboardDataGrid.__init__ instead?

  5. This shouldn't be indented at all.

  6. I think we should call these "Show/Hide archived" rather than "hidden". It's true that it isn't totally correct (since it also shows muted), but calling them "hidden" means that we're introducing a third term.

    1. Makes sense, I'll change this throughout, including in grids.py

  7. 
      
CR
david
  1. 
      
  2. reviewboard/datagrids/grids.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     

    In order to address Barret's question, how about this, which looks somewhat more similar to ShowClosedReviewRequestsMixin

    if profile and 'show_hidden' in profile.extra_data:
        self.show_hidden = profile.extra_data['show_hidden']
    
    try:
        show = self.request.GET.get('show-hidden', self.show_hidden)
        self.show_hidden = int(show) != 0
    except ValueError:
        pass
    
    1. okay, and then on line 322 have if profile and self.show_archived != profile.extra_data['show_hidden']: ? (all of this is currently being renamed as show_archived).

      also, should the if have an else that has profile.extra_data['show_archived'] = False ?

    2. Should probably be if profile and self.show_archived != profile.extra_data.get('show_archived') (just to handle the case where it's not in the dict). We should also have the profile field be 'archived' instead of 'hidden' to keep things consistent.

      The else case should already be handled by the default value of self.show_archived set in __init__.

  3. 
      
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/datagrids/views.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/datagrids/tests.py
    
    Ignored Files:
        reviewboard/templates/datagrids/hideable_listview.html
        reviewboard/static/rb/css/common.less
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/datagrids/views.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/datagrids/tests.py
    
    Ignored Files:
        reviewboard/templates/datagrids/hideable_listview.html
        reviewboard/static/rb/css/common.less
    
    
  2. 
      
CR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/datagrids/views.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/datagrids/tests.py
    
    Ignored Files:
        reviewboard/templates/datagrids/hideable_listview.html
        reviewboard/static/rb/css/common.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/datagrids/views.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/datagrids/tests.py
    
    Ignored Files:
        reviewboard/templates/datagrids/hideable_listview.html
        reviewboard/static/rb/css/common.less
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/datagrids/grids.py (Diff revision 4)
     
     
     

    Instead of using \, can you add parens around this and wrap after and?

    if (profile and
        self.show_archived != profile.extra_data.get('show_archived')):
    
  3. reviewboard/datagrids/views.py (Diff revision 4)
     
     

    This is the only change in this file now--can you revert it?

  4. 
      
CR
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/datagrids/tests.py
    
    Ignored Files:
        reviewboard/templates/datagrids/hideable_listview.html
        reviewboard/static/rb/css/common.less
    
    
  2. reviewboard/datagrids/grids.py (Diff revision 5)
     
     
    Col: 13
     E113 unexpected indentation
    
    1. This is because you're missing the parens around the conditional.

  3. reviewboard/datagrids/grids.py (Diff revision 5)
     
     
    Col: 13
     E112 expected an indented block
    
  4. 
      
CR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/datagrids/tests.py
    
    Ignored Files:
        reviewboard/templates/datagrids/hideable_listview.html
        reviewboard/static/rb/css/common.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/datagrids/tests.py
    
    Ignored Files:
        reviewboard/templates/datagrids/hideable_listview.html
        reviewboard/static/rb/css/common.less
    
    
  2. 
      
david
  1. Your summary/description needs to be updated to refer to the new naming.

  2. reviewboard/datagrids/grids.py (Diff revision 6)
     
     
     

    I think it's just under 80 columns if you were to put this all on one line.

    1. sorry, I tried and it goes to exactly 80

  3. 
      
CR
CR
david
  1. Ship It!
  2. 
      
CR
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to ucosp/cristocrat/archive-and-mute (9b7605f)
Loading...