From a827155c77bd79f6f37e4b6dba23f0460e8bdfde Mon Sep 17 00:00:00 2001
From: Kasper Nilsson <kaspern@google.com>
Date: Tue, 17 Jan 2017 11:54:40 -0800
Subject: [PATCH] Allow removing already-added reviewers in the reply dialog

Enforces two way data binding between the reviewer list in the
reply-dialog and the reviewer list in the change, and fires the
corresponding API call to remove reviewers.

Feature: Issue 4988
Change-Id: Ib03a98947a3e27abe8851279a92c19951f9b2c04
---
 .../change/gr-change-view/gr-change-view.html |   2 +-
 .../gr-reply-dialog/gr-reply-dialog.html      |   5 +-
 .../change/gr-reply-dialog/gr-reply-dialog.js | 100 +++++++++++++++++-
 .../gr-reply-dialog/gr-reply-dialog_test.html |  70 ++++++++++--
 4 files changed, 160 insertions(+), 17 deletions(-)

diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
index 8917a46ff7..4b3549b628 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -391,7 +391,7 @@ limitations under the License.
         on-iron-overlay-opened="_handleReplyOverlayOpen"
         with-backdrop>
       <gr-reply-dialog id="replyDialog"
-          change="[[_change]]"
+          change="{{_change}}"
           patch-num="[[_computeLatestPatchNum(_allPatchSets)]]"
           permitted-labels="[[_change.permitted_labels]]"
           diff-drafts="[[_diffDrafts]]"
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html
index bdc6d6c171..9cee168036 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html
@@ -168,7 +168,8 @@ limitations under the License.
           <div class="peopleListLabel">Reviewers</div>
           <gr-account-list
               id="reviewers"
-              accounts="[[_reviewers]]"
+              accounts="{{_reviewers}}"
+              removable-values="[[change.removable_reviewers]]"
               change="[[change]]"
               filter="[[filterReviewerSuggestion]]"
               pending-confirmation="{{_reviewerPendingConfirmation}}"
@@ -180,7 +181,7 @@ limitations under the License.
             <div class="peopleListLabel">CC</div>
             <gr-account-list
                 id="ccs"
-                accounts="[[_ccs]]"
+                accounts="{{_ccs}}"
                 change="[[change]]"
                 filter="[[filterReviewerSuggestion]]"
                 pending-confirmation="{{_ccPendingConfirmation}}"
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 aa4041b868..26b8b735d5 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
@@ -23,6 +23,11 @@
     REVIEWERS: 'reviewers',
   };
 
+  var ReviewerTypes = {
+    REVIEWER: 'REVIEWER',
+    CC: 'CC',
+  };
+
   Polymer({
     is: 'gr-reply-dialog',
 
@@ -95,6 +100,13 @@
         value: false,
         observer: '_handleHeightChanged',
       },
+      _reviewersPendingRemove: {
+        type: Object,
+        value: {
+          CC: [],
+          REVIEWER: [],
+        },
+      },
     },
 
     FocusTarget: FocusTarget,
@@ -105,6 +117,8 @@
 
     observers: [
       '_changeUpdated(change.reviewers.*, change.owner, serverConfig)',
+      '_ccsChanged(_ccs.splices)',
+      '_reviewersChanged(_reviewers.splices)',
     ],
 
     attached: function() {
@@ -144,6 +158,76 @@
       selectorEl.selectIndex(selectorEl.indexOf(item));
     },
 
+    _ccsChanged: function(splices) {
+      if (splices && splices.indexSplices) {
+        this._processReviewerChange(splices.indexSplices, ReviewerTypes.CC);
+      }
+    },
+
+    _reviewersChanged: function(splices) {
+      if (splices && splices.indexSplices) {
+        this._processReviewerChange(splices.indexSplices,
+            ReviewerTypes.REVIEWER);
+      }
+    },
+
+    _processReviewerChange: function(indexSplices, type) {
+      indexSplices.forEach(function(splice) {
+        splice.removed.forEach(function(account) {
+          if (!this._reviewersPendingRemove[type]) {
+            console.err('Invalid type ' + type + ' for reviewer.');
+            return;
+          }
+          this._reviewersPendingRemove[type].push(account);
+        }.bind(this));
+      }.bind(this));
+    },
+
+    /**
+     * Resets the state of the _reviewersPendingRemove object, and removes
+     * accounts if necessary.
+     *
+     * @param {Boolean} isCancel true if the action is a cancel.
+     */
+    _purgeReviewersPendingRemove: function(isCancel) {
+      var reviewerArr;
+      for (var type in this._reviewersPendingRemove) {
+        if (this._reviewersPendingRemove.hasOwnProperty(type)) {
+          if (!isCancel) {
+            reviewerArr = this._reviewersPendingRemove[type];
+            for (var i = 0; i < reviewerArr.length; i++) {
+              this._removeAccount(reviewerArr[i], type);
+            }
+          }
+          this._reviewersPendingRemove[type] = [];
+        }
+      }
+    },
+
+    /**
+     * Removes an account from the change, both on the backend and the client.
+     * Does nothing if the account is a pending addition.
+     *
+     * @param {Object} account
+     * @param {ReviewerTypes} type
+     */
+    _removeAccount: function(account, type) {
+      if (account._pendingAdd) { return; }
+
+      return this.$.restAPI.removeChangeReviewer(this.change._number,
+          account._account_id).then(function(response) {
+        if (!response.ok) { return response; }
+
+        var reviewers = this.change.reviewers[type] || [];
+        for (var i = 0; i < reviewers.length; i++) {
+          if (reviewers[i]._account_id == account._account_id) {
+            this.splice(['change', 'reviewers', type], i, 1);
+            break;
+          }
+        }
+      }.bind(this));
+    },
+
     _mapReviewer: function(reviewer) {
       var reviewerId;
       var confirmed;
@@ -161,6 +245,7 @@
         drafts: 'PUBLISH_ALL_REVISIONS',
         labels: {},
       };
+
       for (var label in this.permittedLabels) {
         if (!this.permittedLabels.hasOwnProperty(label)) { continue; }
 
@@ -341,17 +426,21 @@
     },
 
     _changeUpdated: function(changeRecord, owner, serverConfig) {
+      this._rebuildReviewerArrays(changeRecord.base, owner, serverConfig);
+    },
+
+    _rebuildReviewerArrays: function(change, owner, serverConfig) {
       this._owner = owner;
 
       var reviewers = [];
       var ccs = [];
 
-      for (var key in changeRecord.base) {
+      for (var key in change) {
         if (key !== 'REVIEWER' && key !== 'CC') {
           console.warn('unexpected reviewer state:', key);
           continue;
         }
-        changeRecord.base[key].forEach(function(entry) {
+        change[key].forEach(function(entry) {
           if (entry._account_id === owner._account_id) {
             return;
           }
@@ -409,11 +498,16 @@
     _cancelTapHandler: function(e) {
       e.preventDefault();
       this.fire('cancel', null, {bubbles: false});
+      this._purgeReviewersPendingRemove(true);
+      this._rebuildReviewerArrays(this.change.reviewers, this._owner,
+          this.serverConfig);
     },
 
     _sendTapHandler: function(e) {
       e.preventDefault();
-      this.send();
+      this.send().then(function() {
+        this._purgeReviewersPendingRemove();
+      }.bind(this));
     },
 
     _saveReview: function(review, opt_errFn) {
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 23a6c863ae..f7058bd1d7 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
@@ -41,6 +41,10 @@ limitations under the License.
     var setDraftCommentStub;
     var eraseDraftCommentStub;
 
+    var lastId = 0;
+    var makeAccount = function() { return {_account_id: lastId++}; };
+    var makeGroup = function() { return {id: lastId++}; };
+
     setup(function() {
       sandbox = sinon.sandbox.create();
 
@@ -381,14 +385,6 @@ limitations under the License.
     });
 
     test('filterReviewerSuggestion', function() {
-      var counter = 0;
-      function makeAccount() {
-        return {_account_id: counter++};
-      }
-      function makeGroup() {
-        return {id: counter++};
-      }
-
       var owner = makeAccount();
       var reviewer1 = makeAccount();
       var reviewer2 = makeGroup();
@@ -476,7 +472,7 @@ limitations under the License.
             ' 0': 'No score',
             '+1': 'Verified'
           },
-          default_value: 0
+          default_value: 0,
         },
         'Code-Review': {
           values: {
@@ -486,8 +482,8 @@ limitations under the License.
             '+1': 'Looks good to me, but someone else must approve',
             '+2': 'Looks good to me, approved'
           },
-          default_value: 0
-        }
+          default_value: 0,
+        },
       };
 
       flushAsynchronousOperations();
@@ -510,5 +506,57 @@ limitations under the License.
       verifiedBtn._handleHideTooltip();
       assert.isNotOk(verifiedBtn._tooltip);
     });
+
+    test('_processReviewerChange', function() {
+      var mockIndexSplices = function(toRemove) {
+        return [{
+          removed: [toRemove],
+        }];
+      };
+
+      element._processReviewerChange(
+          mockIndexSplices(makeAccount()), 'REVIEWER');
+      assert.equal(element._reviewersPendingRemove.REVIEWER.length, 1);
+    });
+
+    test('_purgeReviewersPendingRemove', function() {
+      var removeStub = sandbox.stub(element, '_removeAccount');
+      var mock = function() {
+        element._reviewersPendingRemove = {
+          test: [makeAccount()],
+          test2: [makeAccount(), makeAccount()],
+        };
+      };
+      var checkObjEmpty = function(obj) {
+        for (var prop in obj) {
+          if (obj.hasOwnProperty(prop) && obj[prop].length) { return false; }
+        }
+        return true;
+      };
+      mock();
+      element._purgeReviewersPendingRemove(true); // Cancel
+      assert.isFalse(removeStub.called);
+      assert.isTrue(checkObjEmpty(element._reviewersPendingRemove));
+
+      mock();
+      element._purgeReviewersPendingRemove(false); // Submit
+      assert.isTrue(removeStub.called);
+      assert.isTrue(checkObjEmpty(element._reviewersPendingRemove));
+    });
+
+    test('_removeAccount', function(done) {
+      sandbox.stub(element.$.restAPI, 'removeChangeReviewer')
+          .returns(Promise.resolve({ok: true}));
+      var arr = [makeAccount(), makeAccount()];
+      element.change.reviewers = {
+        REVIEWER: arr.slice(),
+      };
+
+      element._removeAccount(arr[1], 'REVIEWER').then(function() {
+        assert.equal(element.change.reviewers.REVIEWER.length, 1);
+        assert.deepEqual(element.change.reviewers.REVIEWER, arr.slice(0, 1));
+        done();
+      });
+    });
   });
 </script>