Summary

Add a Dashboard toggle to show or hide archived review requests

Review Request #7109 — Created March 24, 2015 and submitted May 1, 2015, 6:27 p.m.

Information

Owner

cristocrat

Repository

Review Board

Branch

master

Bugs

Depends On

~~7085~~

Blocks

7087

Reviewers

Groups

reviewboard, students

People

Description

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.

Testing Done

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.

Files

Issues

Description	From	Last Updated
Could you alphabetize these?	brennie	March 25, 2015, 12:13 a.m.
I have the same question here as in the other review request about why we aren't using default_show_hidden as the …	brennie	March 25, 2015, 4:17 p.m.
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)	brennie	March 25, 2015, 12:32 a.m.
Will {{datagrid.queries}} expand to have a & in it if there are queries?	brennie	March 25, 2015, 12:32 a.m.
Can we move this down to be in an else clause of the if profile and self.show_hidden != default_show_hidden: conditional?	david	March 25, 2015, 2:29 p.m.
In order to address Barret's question, how about this, which looks somewhat more similar to ShowClosedReviewRequestsMixin if profile and 'show_hidden' …	david	March 25, 2015, 3:42 p.m.
Can we wrap this a little differently? parent_profile_changed = \ super(DashboardDataGrid, self).load_extra_state( profile, allow_hide_closed=False)	david	March 25, 2015, 2:30 p.m.
Can we do this in DashboardDataGrid.__init__ instead?	david	March 25, 2015, 2:40 p.m.
This shouldn't be indented at all.	david	March 25, 2015, 2:41 p.m.
I think we should call these "Show/Hide archived" rather than "hidden". It's true that it isn't totally correct (since it …	david	March 25, 2015, 2:59 p.m.
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')):	david	March 25, 2015, 4:15 p.m.
This is the only change in this file now--can you revert it?	david	March 25, 2015, 4:15 p.m.
Col: 13 E113 unexpected indentation	reviewbot	March 25, 2015, 4:58 p.m.
Col: 13 E112 expected an indented block	reviewbot	March 25, 2015, 4:58 p.m.
I think it's just under 80 columns if you were to put this all on one line.	david	April 2, 2015, 11:02 a.m.

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

reviewboard/datagrids/grids.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Could you alphabetize these?
```

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

The issue has been resolved. Show all issues

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.

cristocrat March 25, 2015, 11:27 a.m.

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

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

The issue has been resolved. Show all issues

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)

reviewboard/templates/datagrids/hideable_listview.html (Diff revision 1)
The issue has been resolved. Show all issues
```
Will {{datagrid.queries}} expand to have a & in it if there are queries?
```

Summary:

[WIP] Add a toggle on the Dashboard datagrid to show or hide hidden review requests.

Add a Dashboard toggle to show or hide hidden review requests

Description:

~		Added a toggle on the Dashboard datagrid to show or hide hidden review requests
	~	This adds a toggle on the Dashboard datagrid to show or hide hidden 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-hidden` query is appended to the existing URL query
	+	`show-hidden=0` filters the hidden review requests (those that are archived or muted)
	+	`show-hidden=1` includes all review requests, regardless of their state of visibility
	+
	+	NB: When `show-hidden` is not specified, its default value of is 0.

Testing Done:

	+	Manual tests: Archived and muted several review requests. In the dashboard, they disappeared when setting `show-hidden=0` and reappeared when setting `show-hidden=1`
	+
	+	Unit tests: Added to reviewboard.datagrids.tests to test if hidden review requests would appear in the datagrid for a particular user for `show-hidden=0` and `show-hidden=1`. Also tested for a different user who had not hidden any of the same review requests. All tests passed.

Groups:

students

Diff:

Revision 2 (+147 -7)

Show changes

	reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
	reviewboard/datagrids/grids.py
	reviewboard/datagrids/tests.py
	reviewboard/datagrids/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/templates/datagrids/hideable_listview.html

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

Status:: Completed

Status:: Re-opened

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

The issue has been resolved. Show all issues

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

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

The issue has been resolved. Show all issues

Can we wrap this a little differently?

parent_profile_changed = \
    super(DashboardDataGrid, self).load_extra_state(
        profile, allow_hide_closed=False)

reviewboard/datagrids/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we do this in DashboardDataGrid.__init__ instead?
```
reviewboard/templates/datagrids/hideable_listview.html (Diff revision 2)
The issue has been resolved. Show all issues
```
This shouldn't be indented at all.
```

reviewboard/templates/datagrids/hideable_listview.html (Diff revision 2)

The issue has been resolved. Show all issues

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.

cristocrat March 25, 2015, 2:43 p.m.

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

Diff:

Revision 3 (+148 -7)

Show changes

	reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
	reviewboard/datagrids/grids.py
	reviewboard/datagrids/tests.py
	reviewboard/datagrids/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/templates/datagrids/hideable_listview.html

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

The issue has been resolved. Show all issues

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

cristocrat March 25, 2015, 3:21 p.m.

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 ?

david March 25, 2015, 3:28 p.m.

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__.

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

Diff:

Revision 4 (+147 -7)

Show changes

	reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
	reviewboard/datagrids/grids.py
	reviewboard/datagrids/tests.py
	reviewboard/datagrids/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/templates/datagrids/hideable_listview.html

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

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

The issue has been resolved. Show all issues

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')):

reviewboard/datagrids/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This is the only change in this file now--can you revert it?
```

Diff:

Revision 5 (+146 -7)

Show changes

	reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
	reviewboard/datagrids/grids.py
	reviewboard/datagrids/tests.py
	reviewboard/static/rb/css/common.less
	reviewboard/templates/datagrids/hideable_listview.html

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

reviewboard/datagrids/grids.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 13
 E113 unexpected indentation
```
1. david March 25, 2015, 4:39 p.m.
  This is because you're missing the parens around the conditional.
reviewboard/datagrids/grids.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 13
 E112 expected an indented block
```

Diff:

Revision 6 (+146 -7)

Show changes

	reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
	reviewboard/datagrids/grids.py
	reviewboard/datagrids/tests.py
	reviewboard/static/rb/css/common.less
	reviewboard/templates/datagrids/hideable_listview.html

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

Your summary/description needs to be updated to refer to the new naming.

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

The issue has been dropped. Show all issues

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

cristocrat March 27, 2015, 11:12 p.m.

sorry, I tried and it goes to exactly 80

Summary:: Add a Dashboard toggle to show or hide hidden review requests
Add a Dashboard toggle to show or hide archived review requests
Description:: ~
This adds a toggle on the Dashboard datagrid to show or hide hidden review requests, similar to that of the All Review Requests datagrid for showing and hiding closed review requests.
~
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-hidden query is appended to the existing URL query
~
show-hidden=0 filters the hidden review requests (those that are archived or muted)
~
show-hidden=1 includes all review requests, regardless of their state of visibility
~
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-hidden is not specified, its default value of is 0.
~
NB: When show-archived is not specified, its default value of is 0.
Testing Done:: ~
Manual tests: Archived and muted several review requests. In the dashboard, they disappeared when setting show-hidden=0 and reappeared when setting show-hidden=1
~
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 hidden review requests would appear in the datagrid for a particular user for show-hidden=0 and show-hidden=1. Also tested for a different user who had not hidden any of the same review requests. All tests passed.
~
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.

Added Files:

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to ucosp/cristocrat/archive-and-mute (9b7605f)