Fix status updates not showing as completed if they complete too fast.
Review Request #11733 — Created July 19, 2021 and submitted
We've had a long-standing bug where any Status Updates (posted via an
integration, Review Bot, or a custom script) that complete within the
same second in which they were created would spin forever, never showing
up as completed, until the user reloaded the page.
The reason this happened is that we were using timestamps for comparison
when polling for updates. Upon initial load, we'd get an added and an
updated timestamp, set to the same value. Once a Status Update was
marked as completed (as success, error, etc.), a new updated timestamp
would be sent along in the update to the browser. However, since we only
have second precision and not millisecond precision (at least on page
load -- we weren't consistent with this when serializing updates), the
browser wouldn't see a change to the timestamp and would discard the
update, continuing to poll indefinitely.
The funny thing is, the content of that update would actually contain
the results of the Status Update, but it would be ignored.
The solution to this is to employ both timestamps and ETags, just like
browser caching does. In fact, we already had ETag calculations
BaseReviewRequestPageEntry.build_etag_data(), but not for
this purpose. They would calculate an ETag encompassing every instance
of an Entry on the page, for use in the page's own ETag, making them
just a bit unsuitable for this purpose. As such, we didn't pass them
along in the page update payloads, and didn't compare them.
This change reworks this a bit to allow ETag calculation to either
encompass all entry instances of a type, or to be focused on a specific
entry. We can then
build_etag_data()for both the page's ETag and also
for an entry's own polling updates.
Status Update ETag calculation now takes advantage of this, providing an
ETag for both scenarios. The code was reworked to grab the information
needed from either all entries of the type, or a specific entry passed
in, and to calculate not just the timestamps but also the statuses and
descriptions (basically, anything likely to change) and to generate a
SHA1 hash from the result.
It also correctly updates both the ETag and the updated timestamp in the
model. Before now, we didn't actually record the new information. We
just stopped watching. In the future, this won't be sufficient.
With this change, we should now be able to expect polled data, and
Status Updates in particular, to reflect new state while the page is
open, no matter when they complete.
Created a review request with a Status Update, loaded the page, watched
the polling take place, and then switched the status of the Status Update
without updating the timestamp. Saw that the page properly noticed the
change, updated page content, and stopped polling. Prior to this, this
same test would poll forever and never notice the change.