From 15ed72fe51911d2d91f11c474d06d9600d7f7c78 Mon Sep 17 00:00:00 2001
From: Wyatt Allen <wyatta@google.com>
Date: Thu, 10 Aug 2017 09:29:20 -0700
Subject: [PATCH] Wait for revert or cherrypick changes to be reachable before
 redirecting

Bug: Issue 6969
Change-Id: I60c27dcf3446eec0afa07b2d5ef45cbfd080a3d7
---
 .../gr-change-actions/gr-change-actions.js    | 56 ++++++++++++++++---
 .../gr-change-actions_test.html               | 31 ++++++++++
 .../gr-rest-api-interface.js                  |  5 +-
 3 files changed, 83 insertions(+), 9 deletions(-)

diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
index e9e60e081b..4691836932 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
@@ -118,6 +118,9 @@
     __type: 'revision',
   };
 
+  const AWAIT_CHANGE_ATTEMPTS = 5;
+  const AWAIT_CHANGE_TIMEOUT_MS = 1000;
+
   Polymer({
     is: 'gr-change-actions',
 
@@ -824,10 +827,9 @@
     // https://bugs.chromium.org/p/gerrit/issues/detail?id=4671 is resolved.
     _setLabelValuesOnRevert(newChangeId) {
       const labels = this.$.jsAPI.getLabelValuesPostRevert(this.change);
-      if (labels) {
-        this.$.restAPI.getChangeURLAndSend(newChangeId,
-            this.actions.revert.method, 'current', '/review', {labels});
-      }
+      if (!labels) { return Promise.resolve(); }
+      return this.$.restAPI.getChangeURLAndSend(newChangeId,
+          this.actions.revert.method, 'current', '/review', {labels});
     },
 
     _handleResponse(action, response) {
@@ -835,10 +837,16 @@
       return this.$.restAPI.getResponseObject(response).then(obj => {
         switch (action.__key) {
           case ChangeActions.REVERT:
-            this._setLabelValuesOnRevert(obj.change_id);
-            /* falls through */
+            this._waitForChangeReachable(obj._number)
+                .then(() => this._setLabelValuesOnRevert(obj._number))
+                .then(() => {
+                  Gerrit.Nav.navigateToChange(obj);
+                });
+            break;
           case RevisionActions.CHERRYPICK:
-            page.show(this.changePath(obj._number));
+            this._waitForChangeReachable(obj._number).then(() => {
+              Gerrit.Nav.navigateToChange(obj);
+            });
             break;
           case ChangeActions.DELETE:
           case RevisionActions.DELETE:
@@ -1015,5 +1023,39 @@
         };
       });
     },
+
+    /**
+     * Occasionally, a change created by a change action is not yet knwon to the
+     * API for a brief time. Wait for the given change number to be recognized.
+     *
+     * Returns a promise that resolves with true if a request is recognized, or
+     * false if the change was never recognized after all attempts.
+     *
+     * @param  {number} changeNum
+     * @return {Promise<boolean>}
+     */
+    _waitForChangeReachable(changeNum) {
+      let attempsRemaining = AWAIT_CHANGE_ATTEMPTS;
+      return new Promise(resolve => {
+        const check = () => {
+          attempsRemaining--;
+          // Pass a no-op error handler to avoid the "not found" error toast.
+          this.$.restAPI.getChange(changeNum, () => {}).then(response => {
+            // If the response is 404, the response will be undefined.
+            if (response) {
+              resolve(true);
+              return;
+            }
+
+            if (attempsRemaining) {
+              this.async(check, AWAIT_CHANGE_TIMEOUT_MS);
+            } else {
+              resolve(false);
+            }
+          });
+        };
+        check();
+      });
+    },
   });
 })();
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
index 1658da8bce..2390c217a7 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
@@ -1038,6 +1038,37 @@ limitations under the License.
         assert.strictEqual(
             element.$.moreActions.items[4].id, 'submit-revision');
       });
+
+      suite('_waitForChangeReachable', () => {
+        setup(() => {
+          sandbox.stub(element, 'async', fn => fn());
+        });
+
+        const makeGetChange = numTries => {
+          return () => {
+            if (numTries === 1) {
+              return Promise.resolve({_number: 123});
+            } else {
+              numTries--;
+              return Promise.resolve(undefined);
+            }
+          };
+        };
+
+        test('succeed', () => {
+          sandbox.stub(element.$.restAPI, 'getChange', makeGetChange(5));
+          return element._waitForChangeReachable(123).then(success => {
+            assert.isTrue(success);
+          });
+        });
+
+        test('fail', () => {
+          sandbox.stub(element.$.restAPI, 'getChange', makeGetChange(6));
+          return element._waitForChangeReachable(123).then(success => {
+            assert.isFalse(success);
+          });
+        });
+      });
     });
   });
 </script>
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index 6431bf047f..0820e4727b 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -1738,11 +1738,12 @@
      * Given a changeNum, gets the change.
      *
      * @param {number|string} changeNum
+     * @param {function(?Response, string=)=} opt_errFn
      * @return {!Promise<?Object>} The change
      */
-    getChange(changeNum) {
+    getChange(changeNum, opt_errFn) {
       // Cannot use _changeBaseURL, as this function is used by _projectLookup.
-      return this.fetchJSON(`/changes/${changeNum}`);
+      return this.fetchJSON(`/changes/${changeNum}`, opt_errFn);
     },
 
     /**