opendev/gerrit
Code Issues Proposed changes

Allow removing already-added reviewers in the reply dialog

Browse Source 
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
This commit is contained in:
Kasper Nilsson
2017-01-17 11:54:40 -08:00
parent 15b005e7d7
commit a827155c77
4 changed files with 160 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
View File

@@ -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]]"

5
polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html
View File

@@ -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}}"

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

@@ -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) {

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

@@ -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>