From f1063a2690683231e1935b47a64dfde7857a8fe6 Mon Sep 17 00:00:00 2001
From: Logan Hanks <logan@google.com>
Date: Fri, 22 Jul 2016 16:05:04 -0700
Subject: [PATCH] Handle reviewer errors when sending a reply

The server may respond with a 400 Bad Request when we send a review
that adds any invalid reviewers (e.g. an unresolvable account, an
oversized group). We need to not discard the draft review in this
case, and apply special error handling to extract human-friendly
error details from the server response.

Change-Id: I46365c3261f383e9a7a210a622cbb59b1d93d107
---
 .../change/gr-reply-dialog/gr-reply-dialog.js | 55 +++++++++++++++++--
 .../gr-reply-dialog/gr-reply-dialog_test.html | 24 ++++++++
 2 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
index fa57770794..d4b0cb2aa5 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
@@ -148,10 +148,13 @@
       });
 
       this.disabled = true;
-      return this._saveReview(obj).then(function(response) {
-        this.disabled = false;
-        if (!response.ok) { return response; }
 
+      var errFn = this._handle400Error.bind(this);
+      return this._saveReview(obj, errFn).then(function(response) {
+        if (!response.ok) {
+          return response;
+        }
+        this.disabled = false;
         this.draft = '';
         this.fire('send', null, {bubbles: false});
       }.bind(this)).catch(function(err) {
@@ -170,6 +173,48 @@
       }
     },
 
+    _handle400Error: function(response) {
+      // A call to _saveReview could fail with a server error if erroneous
+      // reviewers were requested. This is signalled with a 400 Bad Request
+      // status. The default gr-rest-api-interface error handling would
+      // result in a large JSON response body being displayed to the user in
+      // the gr-error-manager toast.
+      //
+      // We can modify the error handling behavior by passing this function
+      // through to restAPI as a custom error handling function. Since we're
+      // short-circuiting restAPI we can do our own response parsing and fire
+      // the server-error ourselves.
+      //
+      this.disabled = false;
+
+      if (response.status !== 400) {
+        // This is all restAPI does when there is no custom error handling.
+        this.fire('server-error', {response: response});
+        return;
+      }
+
+      // Process the response body, format a better error message, and fire
+      // an event for gr-event-manager to display.
+      var jsonPromise = this.$.restAPI.getResponseObject(response);
+      return jsonPromise.then(function(result) {
+        var errors = [];
+        ['reviewers', 'ccs'].forEach(function(state) {
+          for (var input in result[state]) {
+            var reviewer = result[state][input];
+            if (!!reviewer.error) {
+              errors.push(reviewer.error);
+            }
+          }
+        });
+        response = {
+          ok: false,
+          status: response.status,
+          text: function() { return Promise.resolve(errors.join(', ')); },
+        };
+        this.fire('server-error', {response: response});
+      }.bind(this));
+    },
+
     _computeShowLabels: function(patchNum, revisions) {
       var num = parseInt(patchNum, 10);
       for (var rev in revisions) {
@@ -274,9 +319,9 @@
       this.send();
     },
 
-    _saveReview: function(review) {
+    _saveReview: function(review, opt_errFn) {
       return this.$.restAPI.saveChangeReview(this.change._number, this.patchNum,
-          review);
+          review, opt_errFn);
     },
 
     _reviewerPendingConfirmationUpdated: function(reviewer) {
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html
index e65cc0453c..7dc601a3d1 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html
@@ -290,5 +290,29 @@ limitations under the License.
 
       assert.isTrue(eraseDraftCommentStub.calledWith(location));
     });
+
+    test('400 converts to human-readable server-error', function(done) {
+      sandbox.stub(window, 'fetch', function() {
+        var text = '....{"reviewers":{"id1":{"error":"first error"}},' +
+          '"ccs":{"id2":{"error":"second error"}}}';
+        return Promise.resolve({
+          ok: false,
+          status: 400,
+          text: function() { return Promise.resolve(text); },
+        });
+      });
+
+      element.addEventListener('server-error', function(event) {
+        if (event.target !== element) {
+          return;
+        }
+        event.detail.response.text().then(function(body) {
+          assert.equal(body, 'first error, second error');
+          done();
+        });
+      });
+
+      element.send();
+    });
   });
 </script>