Add "New Updates" dot for unviewed review requests.

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

Information

Review Board
release-3.0.x

Reviewers

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
Add "New Updates" dot for unviewed review requests.
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.
4570c4feacc643cf1b65c50f911d711788efe3c5
NIT fixes.
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 …

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 ID
Add "New Updates" dot for unviewed review requests.
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.
aa9bf313fac58e769403ed2a52c78c6fa07b685d
Add "New Updates" dot for unviewed review requests.
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.
8e1d6795cca82be0fb9f48e21735e87b75be4423

Diff:

Revision 2 (+250 -12)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

bnie
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. 
      
bnie
Review request changed

Change Summary:

Query issue in unit tests was not yet solved.

Commits:

Summary ID
Add "New Updates" dot for unviewed review requests.
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.
3be01edbbadcd8e0451f16294f11886594e78903
Add "New Updates" dot for unviewed review requests.
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.
4570c4feacc643cf1b65c50f911d711788efe3c5
NIT fixes.
517c6c73503f120d973429fba88e6dd493e48c90

Diff:

Revision 4 (+275 -25)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...