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
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 8917a46..4b3549b 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 @@ 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 bdc6d6c..9cee168 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 @@ <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 @@ <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 aa4041b..26b8b73 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 23a6c86..f7058bd 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 @@ 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 @@ }); 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 @@ ' 0': 'No score', '+1': 'Verified' }, - default_value: 0 + default_value: 0, }, 'Code-Review': { values: { @@ -486,8 +482,8 @@ '+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 @@ 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>