Add "New Updates" dot for unviewed review requests.

Review Request #11182 — Created Sept. 19, 2020 and updated

bnie
Review Board
release-3.0.x
reviewboard, students

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
Add "New Updates" dot for unviewed review requests.
Description From Last Updated

This would be a great thing to add a unit test for, so we can be sure it works for ...

chipx86chipx86

Your test cases look to be validating the display of the dot, but none of them are actually testing the ...

daviddavid

We for sure have some older docstrings without the modern conventions, like Args and Returns, but for any new ones, ...

chipx86chipx86

The other code in here used COUNT(*), and it works, but we might be able to do better than that. ...

chipx86chipx86

Since the result is the same, we should avoid duplicating it. Let's instead make the if statement check either condition.

chipx86chipx86

There should be a blank line between a block and a statement, to help with readability.

chipx86chipx86

E501 line too long (80 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

Let's add a blank line between these two.

daviddavid

We can wrap the type names as such: queryset (djblets.db.query. _Django16ChainableSelectRelatedQuerySet): However, that type name is kind of a behind-the-scenes ...

daviddavid

Same here, re: queryset. Also, with "Returns", we don't need to indent the explanation, so this should look like: Returns: ...

daviddavid

Let's add a blank line between these two.

daviddavid

Type and description should be at the same indent level here. The type should also be "unicode" rather than "Str".

daviddavid

Please put this blank line back (there should always be two blank lines between top-level definitions in the file, according ...

daviddavid

Docstrings for test cases shouldn't end in a period, because the test runner will print an ellipsis after each one. ...

daviddavid

When we write comments, we prefer to have proper sentence capitalization and punctuation (so "Set the new review count to ...

daviddavid
bnie
chipx86
  1. 
      
  2. This would be a great thing to add a unit test for, so we can be sure it works for each possible condition:

    1. No new reviews, no visits (blue dot)
    2. No new reviews, visits (no blue dot)
    3. New reviews, no visits (no blue dot)
    4. New reviews, visits (blue dot)

    Each would be its own unit test in reviewboard/datagrids/tests.py for 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 to augment_queryset(), and render a result for it for the review request you set up. Then check the resulting HTML.

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

    We for sure have some older docstrings without the modern conventions, like Args and Returns, but for any new ones, or functions where we're modifying related code, it'll need to follow our conventions.

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

    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:

    1. 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, whereas SELECT 1 doesn't require reading any data.
    2. 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 1 to 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)!)

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

    Since the result is the same, we should avoid duplicating it. Let's instead make the if statement check either condition.

  6. reviewboard/datagrids/columns.py (Diff revision 1)
     
     
     

    There should be a blank line between a block and a statement, to help with readability.

  7. 
      
bnie
Review request changed

Change Summary:

Added comments for functions, nit changes, and unit tests.

Commits:

Summary
-
Add "New Updates" dot for unviewed review requests.
+
Add "New Updates" dot for unviewed review requests.

Diff:

Revision 2 (+250 -12)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

bnie
Review request changed

Change Summary:

Nit changes: removed extra whitespace and added whitespace after comma

Commits:

Summary
-
Add "New Updates" dot for unviewed review requests.
+
Add "New Updates" dot for unviewed review requests.

Diff:

Revision 3 (+256 -12)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
david
  1. Looking pretty good. Most of my comments are little style nits, but the first one here is important re: test cases.

  2. 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?

  3. reviewboard/datagrids/columns.py (Diff revision 3)
     
     
     

    Let's add a blank line between these two.

  4. reviewboard/datagrids/columns.py (Diff revision 3)
     
     
     
     

    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

  5. reviewboard/datagrids/columns.py (Diff revision 3)
     
     
     

    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.
    
  6. reviewboard/datagrids/columns.py (Diff revision 3)
     
     
     

    Let's add a blank line between these two.

  7. reviewboard/datagrids/columns.py (Diff revision 3)
     
     
     

    Type and description should be at the same indent level here. The type should also be "unicode" rather than "Str".

  8. reviewboard/datagrids/columns.py (Diff revision 3)
     
     

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

  9. reviewboard/datagrids/tests.py (Diff revision 3)
     
     

    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.

  10. reviewboard/datagrids/tests.py (Diff revision 3)
     
     

    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.

  11. 
      
Loading...