Factor in new diffs in the New Updates column in the dashboard.

Review Request #11803 — Created Aug. 26, 2021 and submitted

Information

Review Board
release-3.0.x

Reviewers

The New Updates column wasn't really checking for new updates to a
review request. It was checking for new reviews, but updates weren't
factored in. This was noticeable for most users when it came to diff
updates, but applied to any kind (new field changes or file attachments,
for example).

Solving this in Review Board 3.0.x for all kinds of updates is
problematic. In the most ideal situation, we'd just check a timestamp
field on the review request, but we don't have a suitable one.

Failing that, we'd query the last visited timestamp against the most
recent ChangeDescription, but these are joined using a Many-to-Many
intermediary table, and hand-crafting a timestamp comparison in SQL that
also performs the join is potentially error-prone across all supported
databases.

Field annotations (.annotate(Max(...))) have been ruled out due to
some very expensive SQL that we don't want in the dashboard.

Another option is to prefetch_related() the fields we need,
generating a couple extra (but light-weight) SQL queries, and then
compare the timestamps. However, with Django 1.6, we'd be fetching all
content from the related tables, which is also too expensive (though an
option for Review Board 4.0 with Django 1.11, using Prefetch objects).

For 3.0, we're taking a shortcut. We're just checking the last updated
diff timestamp. This won't capture any updates but diffs, but diffs are
the most important ones for most users, and the source of the reported
bug.

In 4.0, we can then update this to perform some better, leaner queries
using Prefetch objects and incorporate any and all changes made to a
review request. We can hopefully tackle this in an even better way in
5.0 with the introduction of some new fields.

Unit tests pass.

Manually tested updating a diff's timestamp and then viewing the
dashboard. Saw that the column indicated new updates.

Summary ID
Factor in new diffs in the New Updates column in the dashboard.
The New Updates column wasn't really checking for new updates to a review request. It was checking for new reviews, but updates weren't factored in. This was noticeable for most users when it came to diff updates, but applied to any kind (new field changes or file attachments, for example). Solving this in Review Board 3.0.x for all kinds of updates is problematic. In the most ideal situation, we'd just check a timestamp field on the review request, but we don't have a suitable one. Failing that, we'd query the last visited timestamp against the most recent `ChangeDescription`, but these are joined using a Many-to-Many intermediary table, and hand-crafting a timestamp comparison in SQL that also performs the join is potentially error-prone across all supported databases. Field annotations (`.annotate(Max(...))`) have been ruled out due to some very expensive SQL that we don't want in the dashboard. Another option is to `prefetch_related()` the fields we need, generating a couple extra (but light-weight) SQL queries, and then compare the timestamps. However, with Django 1.6, we'd be fetching all content from the related tables, which is also too expensive (though an option for Review Board 4.0 with Django 1.11, using `Prefetch` objects). For 3.0, we're taking a shortcut. We're just checking the last updated diff timestamp. This won't capture any updates but diffs, but diffs are the most important ones for most users, and the source of the reported bug. In 4.0, we can then update this to perform some better, leaner queries using `Prefetch` objects and incorporate any and all changes made to a review request. We can hopefully tackle this in an even better way in 5.0 with the introduction of some new fields.
120d8321bf86ece3a98802605a071ee073e9cf22
Description From Last Updated

F841 local variable 'visit' is assigned to but never used

reviewbotreviewbot

F841 local variable 'visit' is assigned to but never used

reviewbotreviewbot

F841 local variable 'review' is assigned to but never used

reviewbotreviewbot

F841 local variable 'visit' is assigned to but never used

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (7382b86)
Loading...