flake8
passed.
JSHint
passed.
Add "New Updates" dot for unviewed review requests.
Review Request #11182 — Created Sept. 19, 2020 and updated
The dashboard has a column which can show a blue dot when review requests have
had new updates since the last time they were viewed by the user. This is used
by people to let them know that there's some unread content. However, the
column currently would not include the dot for review requests which have
never been opened by the user. Since those too should count as unread content,
we want to cover this scenario as well.This change adds "New Updates" blue dot to a review request if it hasn't
been opened yet.
Unit tests passed. Also manually checked the "New Updates" dot does appear on review requests that hasn't been visited.
| Summary | ID |
|---|---|
| 4570c4feacc643cf1b65c50f911d711788efe3c5 | |
| 517c6c73503f120d973429fba88e6dd493e48c90 |
| Description | From | Last Updated |
|---|---|---|
|
This would be a great thing to add a unit test for, so we can be sure it works for … |
 chipx86
|
|
|
Your test cases look to be validating the display of the dot, but none of them are actually testing the … |
 david
|
|
|
We for sure have some older docstrings without the modern conventions, like Args and Returns, but for any new ones, … |
chipx86
|
|
|
The other code in here used COUNT(*), and it works, but we might be able to do better than that. … |
chipx86
|
|
|
Since the result is the same, we should avoid duplicating it. Let's instead make the if statement check either condition. |
chipx86
|
|
|
There should be a blank line between a block and a statement, to help with readability. |
chipx86
|
|
|
E501 line too long (80 > 79 characters) |
 reviewbot
|
|
|
W291 trailing whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
E501 line too long (85 > 79 characters) |
reviewbot
|
|
|
W291 trailing whitespace |
reviewbot
|
|
|
E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
E231 missing whitespace after ',' |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
E231 missing whitespace after ',' |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
E231 missing whitespace after ',' |
reviewbot
|
|
|
E231 missing whitespace after ',' |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W291 trailing whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
E231 missing whitespace after ',' |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W291 trailing whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
Let's add a blank line between these two. |
david
|
|
|
We can wrap the type names as such: queryset (djblets.db.query. _Django16ChainableSelectRelatedQuerySet): However, that type name is kind of a behind-the-scenes … |
david
|
|
|
Same here, re: queryset. Also, with "Returns", we don't need to indent the explanation, so this should look like: Returns: … |
david
|
|
|
Let's add a blank line between these two. |
david
|
|
|
Type and description should be at the same indent level here. The type should also be "unicode" rather than "Str". |
david
|
|
|
Please put this blank line back (there should always be two blank lines between top-level definitions in the file, according … |
david
|
|
|
Docstrings for test cases shouldn't end in a period, because the test runner will print an ellipsis after each one. … |
david
|
|
|
When we write comments, we prefer to have proper sentence capitalization and punctuation (so "Set the new review count to … |
david
|
- Description:
-
The dashboard has a column which can show a blue dot when review requests have
had new updates since the last time they were viewed by the user. This is used by people to let them know that there's some unread content. However, the column currently would not include the dot for review requests which have never been opened by the user. Since those too should count as unread content, we want to cover this scenario as well. This change adds "New Updates" blue dot to a review request if it hasn't
been opened yet. - - Testing Done:
- Unit tests passed. - Testing Done:
-
+ Unit tests passed. Also manually checked the "New Updates" dot does appear on review requests that hasn't been visited.
-
-
This would be a great thing to add a unit test for, so we can be sure it works for each possible condition:
- No new reviews, no visits (blue dot)
- No new reviews, visits (no blue dot)
- New reviews, no visits (no blue dot)
- New reviews, visits (blue dot)
Each would be its own unit test in
reviewboard/datagrids/tests.pyfor that column's test class.In each one, you'd set up the appropriate state (we have helpers for some things like creating a review request — see
reviewboard/testing/testcase.py), and then you'd create a query, augment it with a call toaugment_queryset(), and render a result for it for the review request you set up. Then check the resulting HTML. -
We for sure have some older docstrings without the modern conventions, like
ArgsandReturns, but for any new ones, or functions where we're modifying related code, it'll need to follow our conventions. -
The other code in here used
COUNT(*), and it works, but we might be able to do better than that. How about trying this query (you'll for sure want to test it — I haven't):SELECT 1 FROM ... WHERE ... AND ... LIMIT 1;This should be faster, for a couple reasons:
SELECT COUNT(*)requires the database to look up data for a row (meaning that it may have to open files and read in data) in order to generate a count, whereasSELECT 1doesn't require reading any data.- It also needs to go through each and every row that is a match. That could be thousands, tens of thousands, or even hundreds of thousands, but since we just care about whether there's 0 or more rows, we can
LIMIT 1to terminate the lookup as soon as any are discovered.
(Assuming this works, it'd be great to have a separate change that simplifies the logic for the other column(s)!)
-
Since the result is the same, we should avoid duplicating it. Let's instead make the
ifstatement check either condition. -
- Change Summary:
-
Added comments for functions, nit changes, and unit tests.
- Commits:
-
Summary ID aa9bf313fac58e769403ed2a52c78c6fa07b685d 8e1d6795cca82be0fb9f48e21735e87b75be4423
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
- Change Summary:
-
Nit changes: removed extra whitespace and added whitespace after comma
- Commits:
-
Summary ID 8e1d6795cca82be0fb9f48e21735e87b75be4423 3be01edbbadcd8e0451f16294f11886594e78903
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
Looking pretty good. Most of my comments are little style nits, but the first one here is important re: test cases.
-
Your test cases look to be validating the display of the dot, but none of them are actually testing the SQL query you wrote, since you're manually setting
is_visited. Can we instead set up the correct ReviewRequestVisit state so that the query gets run? -
-
We can wrap the type names as such:
queryset (djblets.db.query. _Django16ChainableSelectRelatedQuerySet):However, that type name is kind of a behind-the-scenes implementation detail which is likely to go away in the future. Let's claim that the type is
django.db.models.query.QuerySet -
Same here, re: queryset. Also, with "Returns", we don't need to indent the explanation, so this should look like:
Returns: django.db.models.query.QuerySet: The query for the datagrid. -
-
Type and description should be at the same indent level here. The type should also be "unicode" rather than "Str".
-
Please put this blank line back (there should always be two blank lines between top-level definitions in the file, according to Python's style standards).
-
Docstrings for test cases shouldn't end in a period, because the test runner will print an ellipsis after each one. Same for all your other new ones.
-
When we write comments, we prefer to have proper sentence capitalization and punctuation (so "Set the new review count to 0."). Same for the ones in the other test cases.