• 
      

    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:
    Completed
    Change Summary:
    Pushed to release-3.0.x (7382b86)