Add guards to regular change update polling
We are regularly polling every 10 seconds for change updates. If we
detect a new change message, then we show a toast to the user such that
they know that reloading the change is a good idea.
However, we are showing this toast too often, especially when the user
has just sent a reply themselves. The reason is that we did not have
any checks for the update still being relevant. This change adds two
checks: Is the change page loading? Was the change already updated?
This is most likely not fixing the issue 100%, but we are hoping for
99%. And we have plans for switching away from the polling updates
altogether, so we don't want to invest a huge amount of time into
fixing the remaining 1%.
Bug: Issue 13394
Change-Id: I579bf270c5ab8bca5343bd424a6f0122c8fb17ba
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index 2504419..ce08a86 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -2138,7 +2138,8 @@
}
this._updateCheckTimerHandle = this.async(() => {
- fetchChangeUpdates(this._change, this.$.restAPI).then(result => {
+ const change = this._change;
+ fetchChangeUpdates(change, this.$.restAPI).then(result => {
let toastMessage = null;
if (!result.isLatest) {
toastMessage = ReloadToastMessage.NEWER_REVISION;
@@ -2152,7 +2153,12 @@
toastMessage = ReloadToastMessage.NEW_MESSAGE;
}
- if (!toastMessage) {
+ // We have to make sure that the update is still relevant for the user.
+ // Since starting to fetch the change update the user may have sent a
+ // reply, or the change might have been reloaded, or it could be in the
+ // process of being reloaded.
+ const changeWasReloaded = change !== this._change;
+ if (!toastMessage || this._loading || changeWasReloaded) {
this._startUpdateCheckTimer();
return;
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js
index 3d530b5..179caaa 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.js
@@ -1971,7 +1971,7 @@
assert.isFalse(getChangeDetailStub.called);
});
- test('_startUpdateCheckTimer up-to-date', () => {
+ test('_startUpdateCheckTimer up-to-date', async () => {
const getChangeDetailStub =
sinon.stub(element.$.restAPI, 'getChangeDetail')
.callsFake(() => Promise.resolve(generateChange({
@@ -1981,8 +1981,9 @@
})));
element._serverConfig = {change: {update_delay: 12345}};
+ await flush();
- assert.isTrue(element._startUpdateCheckTimer.called);
+ assert.equal(element._startUpdateCheckTimer.callCount, 2);
assert.isTrue(getChangeDetailStub.called);
assert.equal(element.async.lastCall.args[1], 12345 * 1000);
});
@@ -2001,6 +2002,24 @@
done();
});
element._serverConfig = {change: {update_delay: 12345}};
+
+ assert.equal(element._startUpdateCheckTimer.callCount, 1);
+ });
+
+ test('_startUpdateCheckTimer respects _loading', async () => {
+ sinon.stub(element.$.restAPI, 'getChangeDetail')
+ .callsFake(() => Promise.resolve(generateChange({
+ // new patchset was uploaded
+ revisionsCount: 2,
+ messagesCount: 1,
+ })));
+
+ element._loading = true;
+ element._serverConfig = {change: {update_delay: 12345}};
+ await flush();
+
+ // No toast, instead a second call to _startUpdateCheckTimer().
+ assert.equal(element._startUpdateCheckTimer.callCount, 2);
});
test('_startUpdateCheckTimer new status shows an alert', done => {
