Don't remove reviewers after moving from CC

The UX for moving an account from the CC reviewer state to REVIEWER
requires the user to first mark the account for pending removal from
the CC field. This queues the account to be moved to the REMOVED state
immediately after the review is posted.

Sending the reply causes the backend to move such accounts to their new
reviewer state as the user intended. However, we were immediately
deleting them from the reviewer set after posting the review. There was
no accommodation for removed accounts that are readded.

This change keeps track of accounts moving between states. They continue
to be queued for deletion when the user removes them, but any removed
account that gets readded by posting the review is now skipped during
the followup purge.

Bug: Issue 5323
Change-Id: I5918d601314504a2ca072a7f36d76bc62221803b

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

@@ -188,15 +188,21 @@
      * accounts if necessary.
      *
      * @param {Boolean} isCancel true if the action is a cancel.
+     * @param {Object} opt_accountIdsTransferred map of account IDs that must
+     *     not be removed, because they have been readded in another state.
      */
-    _purgeReviewersPendingRemove: function(isCancel) {
+    _purgeReviewersPendingRemove: function(isCancel,
+        opt_accountIdsTransferred) {
       var reviewerArr;
+      var keep = opt_accountIdsTransferred || {};
       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);
+              if (!keep[reviewerArr[i]._account_id]) {
+                this._removeAccount(reviewerArr[i], type);
+              }
             }
           }
           this._reviewersPendingRemove[type] = [];
@@ -271,9 +277,18 @@
         obj.message = this.draft;
       }
 
-      obj.reviewers = this.$.reviewers.additions().map(this._mapReviewer);
+      var accountAdditions = {};
+      obj.reviewers = this.$.reviewers.additions().map(function(reviewer) {
+        if (reviewer.account) {
+          accountAdditions[reviewer.account._account_id] = true;
+        }
+        return this._mapReviewer(reviewer);
+      }.bind(this));
       if (this.serverConfig.note_db_enabled) {
         this.$$('#ccs').additions().forEach(function(reviewer) {
+          if (reviewer.account) {
+            accountAdditions[reviewer.account._account_id] = true;
+          }
           reviewer = this._mapReviewer(reviewer);
           reviewer.state = 'CC';
           obj.reviewers.push(reviewer);
@@ -290,6 +305,7 @@
         this.disabled = false;
         this.draft = '';
         this.fire('send', null, {bubbles: false});
+        return accountAdditions;
       }.bind(this)).catch(function(err) {
         this.disabled = false;
         throw err;
@@ -509,8 +525,8 @@
 
     _sendTapHandler: function(e) {
       e.preventDefault();
-      this.send().then(function() {
-        this._purgeReviewersPendingRemove();
+      this.send().then(function(keep) {
+        this._purgeReviewersPendingRemove(false, keep);
       }.bind(this));
     },
 

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

@@ -558,5 +558,68 @@ limitations under the License.
         done();
       });
     });
+
+    test('migrate reviewers between states', function(done) {
+      element.serverConfig = {note_db_enabled: true};
+      element._reviewersPendingRemove = {
+        CC: [],
+        REVIEWER: [],
+      };
+      flushAsynchronousOperations();
+      var reviewers = element.$.reviewers;
+      var ccs = element.$$('#ccs');
+      var reviewer1 = makeAccount();
+      var reviewer2 = makeAccount();
+      var cc1 = makeAccount();
+      var cc2 = makeAccount();
+      element._reviewers = [reviewer1, reviewer2];
+      element._ccs = [cc1, cc2];
+
+      var mutations = [];
+
+      var saveReviewStub = sandbox.stub(element, '_saveReview',
+          function(review) {
+        mutations.push.apply(mutations, review.reviewers);
+        return Promise.resolve({ok: true});
+      });
+
+      var removeAccountStub = sandbox.stub(element, '_removeAccount',
+          function(account, type) {
+        mutations.push({state: 'REMOVED', account: account});
+        return Promise.resolve();
+      });
+
+      // Remove and add to other field.
+      reviewers.fire('remove', {account: reviewer1});
+      ccs.$.entry.fire('add', {value: {account: reviewer1}});
+      ccs.fire('remove', {account: cc1});
+      reviewers.$.entry.fire('add', {value: {account: cc1}});
+
+      // Add to other field without removing from former field.
+      // (Currently not possible in UI, but this is a good consistency check).
+      reviewers.$.entry.fire('add', {value: {account: cc2}});
+      ccs.$.entry.fire('add', {value: {account: reviewer2}});
+      var mapReviewer = function(reviewer, opt_state) {
+        var result = {reviewer: reviewer._account_id, confirmed: undefined};
+        if (opt_state) {
+          result.state = opt_state;
+        }
+        return result;
+      };
+
+      // Send and purge and verify moves without deletions.
+      element.send()
+          .then(element._purgeReviewersPendingRemove.bind(element))
+          .then(function() {
+        assert.deepEqual(
+            mutations, [
+                mapReviewer(cc1),
+                mapReviewer(cc2),
+                mapReviewer(reviewer1, 'CC'),
+                mapReviewer(reviewer2, 'CC'),
+            ]);
+        done();
+      });
+    });
   });
 </script>