Factor in new diffs in the New Updates column in the dashboard.
Review Request #11803 — Created Aug. 26, 2021 and submitted
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
recentChangeDescription
, 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, usingPrefetch
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
usingPrefetch
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 |
---|---|
120d8321bf86ece3a98802605a071ee073e9cf22 |
Description | From | Last Updated |
---|---|---|
F841 local variable 'visit' is assigned to but never used |
reviewbot | |
F841 local variable 'visit' is assigned to but never used |
reviewbot | |
F841 local variable 'review' is assigned to but never used |
reviewbot | |
F841 local variable 'visit' is assigned to but never used |
reviewbot |