Refactor and test sending in gr-reply-dialog

The asynchronous logic in the send method has gotten complex and messy,
and there may be some bugs hidden there. This change reorganizes the
logic a bit to maintain some important invariants, and adds tests to be
sure.

The two most important invariants are:

1) disabled is true until all promises are resolved
2) draft/_includeComments are only reset if the review was posted

Bug: Issue 6770
Change-Id: I25dd7f64b32be778720747c474a56373cf538e6e

polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js

@@ -343,7 +343,7 @@
       if (this.knownLatestState === 'not-latest') {
         this.fire('show-alert',
             {message: 'Cannot reply to non-latest patch.'});
-        return;
+        return Promise.resolve({});
       }
 
       const labels = this.$.labelScores.getLabelValues();
@@ -391,41 +391,63 @@
 
       const errFn = this._handle400Error.bind(this);
       return this._saveReview(obj, errFn).then(response => {
-        if (!response || !response.ok) {
-          return response;
+        if (!response) {
+          // Null or undefined response indicates that an error handler
+          // took responsibility, so just return.
+          return {};
         }
-        return this.$.restAPI.getResponseObject(response);
+        if (!response.ok) {
+          this.fire('server-error', {response});
+          return {};
+        }
+
+        // TODO(logan): Remove once the required API changes are live and stable
+        // on googlesource.com.
+        return this._maybeSetReady(startReview, response).catch(err => {
+          // We catch error here because we still want to treat this as a
+          // successful review.
+          console.error('error setting ready:', err);
+        }).then(() => {
+          this.draft = '';
+          this._includeComments = true;
+          this.fire('send', null, {bubbles: false});
+          return accountAdditions;
+        });
       }).then(result => {
-        // TODO(logan): Remove this once the required API changes are live and
-        // stable on googlesource.com.
-        if (startReview && !result.ready) {
-          // If we don't see ready in the response, then we're talking to a
-          // backend that doesn't support moving out of WIP at the same time as
-          // posting a review. Fall back to sending a second API call to start
-          // review and block until that returns.
-          return this.$.restAPI.startReview(this.change._number, null,
-              response => {
-                // If we see a 409 response code, then that means the server
-                // *does* support moving from WIP->ready when posting a review.
-                // In that case we should just ignore this error.
-                if (response.status === 409) {
-                  return;
-                }
-                this.fire('server-error', {response});
-              });
-        }
-      }).then(() => {
         this.disabled = false;
-        this.draft = '';
-        this._includeComments = true;
-        this.fire('send', null, {bubbles: false});
-        return accountAdditions;
+        return result;
       }).catch(err => {
         this.disabled = false;
         throw err;
       });
     },
 
+    /**
+     * Returns a promise resolving to true if review was successfully posted,
+     * false otherwise.
+     *
+     * TODO(logan): Remove this once the required API changes are live and
+     * stable on googlesource.com.
+     */
+    _maybeSetReady(startReview, response) {
+      return this.$.restAPI.getResponseObject(response).then(result => {
+        if (!startReview || result.ready) {
+          return Promise.resolve();
+        }
+        // We don't have confirmation that review was started, so attempt to
+        // start review explicitly.
+        return this.$.restAPI.startReview(
+            this.change._number, null, response => {
+              // If we see a 409 response code, then that means the server
+              // *does* support moving from WIP->ready when posting a
+              // review. Only alert user for non-409 failures.
+              if (response.status !== 409) {
+                this.fire('server-error', {response});
+              }
+            });
+      });
+    },
+
     _focusOn(section) {
       if (section === FocusTarget.ANY) {
         section = this._chooseFocusTarget();

polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html

@@ -114,14 +114,20 @@ limitations under the License.
 
     function stubSaveReview(jsonResponseProducer) {
       return sandbox.stub(element, '_saveReview', review => {
-        const result = jsonResponseProducer(review);
-        const resultStr =
-            element.$.restAPI.JSON_PREFIX + JSON.stringify(result);
-        return Promise.resolve({
-          ok: true,
-          text() {
-            return Promise.resolve(resultStr);
-          },
+        return new Promise((resolve, reject) => {
+          try {
+            const result = jsonResponseProducer(review) || {};
+            const resultStr =
+                element.$.restAPI.JSON_PREFIX + JSON.stringify(result);
+            resolve({
+              ok: true,
+              text() {
+                return Promise.resolve(resultStr);
+              },
+            });
+          } catch (err) {
+            reject(err);
+          }
         });
       });
     }
@@ -213,10 +219,13 @@ limitations under the License.
       });
 
       element.addEventListener('send', () => {
-        assert.isFalse(element.disabled,
-            'Element should be enabled when done sending reply.');
-        assert.equal(element.draft.length, 0);
-        done();
+        // Flush to ensure properties are updated.
+        flush(() => {
+          assert.isFalse(element.disabled,
+              'Element should be enabled when done sending reply.');
+          assert.equal(element.draft.length, 0);
+          done();
+        });
       });
 
       // This is needed on non-Blink engines most likely due to the ways in
@@ -702,12 +711,14 @@ limitations under the License.
     });
 
     test('should not send on enter key', () => {
+      stubSaveReview(() => undefined);
       element.addEventListener('send', () => assert.fail('wrongly called'));
       MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'enter');
       flushAsynchronousOperations();
     });
 
     test('emit send on ctrl+enter key', done => {
+      stubSaveReview(() => undefined);
       element.addEventListener('send', () => done());
       MockInteractions.pressAndReleaseKeyOn(element, 13, 'ctrl', 'enter');
       flushAsynchronousOperations();
@@ -795,7 +806,9 @@ limitations under the License.
       let startReviewStub;
 
       setup(() => {
-        startReviewStub = sandbox.stub(element.$.restAPI, 'startReview');
+        startReviewStub = sandbox.stub(element.$.restAPI, 'startReview', () => {
+          return Promise.resolve();
+        });
       });
 
       test('ready property in review input on start review', () => {
@@ -811,7 +824,6 @@ limitations under the License.
       test('no ready property in review input on save review', () => {
         stubSaveReview(review => {
           assert.isUndefined(review.ready);
-          return {};
         });
         return element.send(true, false).then(() => {
           assert.isFalse(startReviewStub.called);
@@ -864,5 +876,94 @@ limitations under the License.
       });
       return element.send(true, true);
     });
+
+    test('buttons disabled until all API calls are resolved', () => {
+      stubSaveReview(review => {
+        return {}; // old backend won't set ready: true
+      });
+      // Check that element is disabled asynchronously after the setReady
+      // promise is returned. The element should not be reenabled until
+      // that promise is resolved.
+      sandbox.stub(element, '_maybeSetReady', (startReview, response) => {
+        return new Promise(resolve => {
+          Polymer.Base.async(() => {
+            assert.isTrue(element.disabled);
+            resolve();
+          });
+        });
+      });
+      return element.send(true, true).then(() => {
+        assert.isFalse(element.disabled);
+      });
+    });
+
+    suite('error handling', () => {
+      const expectedDraft = 'draft';
+      const expectedError = new Error('test');
+
+      setup(() => {
+        element.draft = expectedDraft;
+      });
+
+      function assertDialogOpenAndEnabled() {
+        assert.strictEqual(expectedDraft, element.draft);
+        assert.isFalse(element.disabled);
+      }
+
+      function assertDialogClosed() {
+        assert.strictEqual('', element.draft);
+        assert.isFalse(element.disabled);
+      }
+
+      test('error occurs in _saveReview', () => {
+        stubSaveReview(review => {
+          throw expectedError;
+        });
+        return element.send(true, true).catch(err => {
+          assert.strictEqual(expectedError, err);
+          assertDialogOpenAndEnabled();
+        });
+      });
+
+      test('error occurs during startReview', () => {
+        stubSaveReview(review => {
+          return {}; // old backend won't set ready: true
+        });
+        const errorStub = sandbox.stub(
+            console, 'error', (msg, err) => undefined);
+        sandbox.stub(element.$.restAPI, 'startReview', () => {
+          throw expectedError;
+        });
+        return element.send(true, true).then(() => {
+          assertDialogClosed();
+          assert.isTrue(
+              errorStub.calledWith('error setting ready:', expectedError));
+        });
+      });
+
+      test('non-ok response received by startReview', () => {
+        stubSaveReview(review => {
+          return {}; // old backend won't set ready: true
+        });
+        sandbox.stub(element.$.restAPI, 'startReview', (c, b, f) => {
+          f({status: 500});
+        });
+        return element.send(true, true).then(() => {
+          assertDialogClosed();
+        });
+      });
+
+      test('409 response received by startReview', () => {
+        stubSaveReview(review => {
+          return {}; // old backend won't set ready: true
+        });
+        sandbox.stub(element.$.restAPI, 'startReview', (c, b, f) => {
+          f({status: 409});
+        });
+        return element.send(true, true).then(() => {
+          assertDialogClosed();
+        });
+      });
+    });
   });
 </script>

polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js

@@ -889,8 +889,7 @@
       return auth.fetch(this.getBaseUrl() + url, options).then(response => {
         if (!response.ok) {
           if (opt_errFn) {
-            opt_errFn.call(opt_ctx || null, response);
-            return undefined;
+            return opt_errFn.call(opt_ctx || null, response);
           }
           this.fire('server-error', {response});
         }
@@ -899,7 +898,7 @@
       }).catch(err => {
         this.fire('network-error', {error: err});
         if (opt_errFn) {
-          opt_errFn.call(opt_ctx, null, err);
+          return opt_errFn.call(opt_ctx, null, err);
         } else {
           throw err;
         }