Summary

Add visibility field to a ReviewRequestVisit

Review Request #7085 — Created March 19, 2015 and submitted May 1, 2015, 5:27 p.m.

Information

Owner

cristocrat

Repository

Review Board

Branch

master

Bugs

Depends On

Blocks

7109

Commit

8648c99...

Reviewers

Groups

reviewboard, students

People

Description

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.

Testing Done

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

Issues

Description	From	Last Updated
Single quotes for these.	brennie	March 24, 2015, 2:30 p.m.
Can you use tuples here?	brennie	March 24, 2015, 2:30 p.m.
(from David) Since you have parens, put the opening paren on the previous line instead of using \	CR cristocrat	March 22, 2015, 2:32 p.m.
Judging from previous code, we expect show_archived to be a boolean value. Why are we converting it to an integer …	brennie	March 25, 2015, 1:23 p.m.
We can avoid using a backslash if we format thusly: parent_profile_changed = super(DashboardDataGrid, self).load_extra_state( profile, allow_hide_closed=False)	brennie	March 22, 2015, 2:30 p.m.
Col: 2 W292 no newline at end of file	reviewbot	March 24, 2015, 8:58 p.m.
Blank line after this.	david	March 25, 2015, 1:14 p.m.
local variable 'response' is assigned to but never used	reviewbot	March 25, 2015, 1:14 p.m.
This line can just be: self.client.get(review_request.get_absolute_url())	david	March 25, 2015, 1:14 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	March 25, 2015, 1:19 p.m.
Can you alphabetize these imports?	david	March 25, 2015, 1:45 p.m.
Because of the way that test docstrings get used for the test suite output, we don't put periods at the …	david	March 25, 2015, 1:47 p.m.

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

Summary:

-	Added visibility field to ReviewRequestVisit
+	Adds visibility field to a ReviewRequestVisit database table

Summary:

-	Adds visibility field to a ReviewRequestVisit database table
+	Add visibility field to a ReviewRequestVisit database table

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

The issue has been resolved. Show all issues

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

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

The issue has been resolved. Show all issues

Single quotes for these.

cristocrat March 22, 2015, 2:14 p.m.

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

brennie March 23, 2015, 2:43 p.m.

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.

reviewboard/accounts/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you use tuples here?
```
1. CR
  
  cristocrat March 22, 2015, 2:26 p.m.
  Could only get it to work with index_together = [('user', 'visibility')] (still in a list), is this okay?
2. brennie March 23, 2015, 2:43 p.m.
  This is fine.

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

The issue has been dropped. Show all issues

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.

cristocrat March 22, 2015, 2:30 p.m.

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

brennie March 23, 2015, 5:53 p.m.

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 ?

brennie March 24, 2015, 1:40 p.m.

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

david March 25, 2015, 1:21 p.m.
```
This issue can be dropped now.
```

cristocrat March 25, 2015, 1:23 p.m.

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

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

The issue has been resolved. Show all issues

We can avoid using a backslash if we format thusly:

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

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 :(

Summary:

-	Add visibility field to a ReviewRequestVisit database table
+	[WIP] Add visibility field to a ReviewRequestVisit database table.

Commit:

-	e4f2bdb98eea0e661f48118a4f7e970c410d2b2c
+	4eaa490f6c8ea0becc7e87f09b0d1ad73561c45e

Diff:

Revision 2 (+25)

Show changes

	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py

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

reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 2
 W292 no newline at end of file
```

Summary:

-	[WIP] Add visibility field to a ReviewRequestVisit database table.
+	Add visibility field to a ReviewRequestVisit

Description:

~		The visibility field has three states: visible (V), archived (A), and muted (M). A database evolution, reviewrequestvisit_visibility, was also written for this change.
	~	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 filtering archived and muted requests in the dashboard
-
-		Based on the state of the dashboard's show_archived attribute, review requests that are either archived or muted will be filtered from the queryset.

Testing Done:

	+	Passed reviewboard.accounts.tests
	+
	+	Checked database entries in the admin interface

Commit:

-	4eaa490f6c8ea0becc7e87f09b0d1ad73561c45e
+	7b82e700d4cb587b43a1a38ed26510e6382550aa

Groups:

+	students

Diff:

Revision 3 (+29 -4)

Show changes

	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py

Status: Closed (submitted)

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

Status: Re-opened

Testing Done:

~		Passed reviewboard.accounts.tests
	~	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

Commit:

-	7b82e700d4cb587b43a1a38ed26510e6382550aa
+	54fb1c97ee5568e88e7723bc25e3d6a9ed155997

Diff:

Revision 4 (+49 -6)

Show changes

	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py

Status: Closed (submitted)

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

reviewboard/accounts/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 local variable 'response' is assigned to but never used
```

Status: Re-opened

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

The issue has been resolved. Show all issues

Blank line after this.

cristocrat March 25, 2015, 1:06 p.m.

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

david March 25, 2015, 1:12 p.m.

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.

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

The issue has been resolved. Show all issues

This line can just be:

self.client.get(review_request.get_absolute_url())

Commit:

-	54fb1c97ee5568e88e7723bc25e3d6a9ed155997
+	cb54a8ef0534d6c4c354acf94b52b8a296b4b735

Diff:

Revision 5 (+50 -6)

Show changes

	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/reviewrequestvisit_visibility.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



Tool: Pyflakes
Processed Files:
    reviewboard/accounts/tests.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
    reviewboard/accounts/models.py

reviewboard/accounts/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

Commit:

-	cb54a8ef0534d6c4c354acf94b52b8a296b4b735
+	75183412b4090b51f268e744cb0a7718d6952ae2

Diff:

Revision 6 (+50 -6)

Show changes

	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/reviewrequestvisit_visibility.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



Tool: Pyflakes
Processed Files:
    reviewboard/accounts/tests.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
    reviewboard/accounts/models.py

reviewboard/accounts/tests.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Can you alphabetize these imports?
```

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

The issue has been resolved. Show all issues

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

Commit:

-	75183412b4090b51f268e744cb0a7718d6952ae2
+	8648c998e7c9217d94d73eb5f20fb7c49119b5b4

Diff:

Revision 7 (+50 -6)

Show changes

	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/reviewrequestvisit_visibility.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



Tool: Pyflakes
Processed Files:
    reviewboard/accounts/tests.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
    reviewboard/accounts/models.py

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Status: Re-opened

Status: Closed (submitted)

Change Summary:

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