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(); + }); + }); });