From 64451c001a10be656566a58f6b51c7c559c8a1a4 Mon Sep 17 00:00:00 2001 From: Logan Hanks Date: Wed, 22 Mar 2017 13:39:40 -0700 Subject: [PATCH] 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 --- .../change/gr-reply-dialog/gr-reply-dialog.js | 26 ++++++-- .../gr-reply-dialog/gr-reply-dialog_test.html | 63 +++++++++++++++++++ 2 files changed, 84 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 f56d85d893..80ccd11c75 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 @@ -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)); }, 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 f7058bd1d7..7adc4a3974 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 @@ -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(); + }); + }); });