Allow to remove reviewers that don't have _account_id
The issue was introduced by the following change:
https://gerrit-review.googlesource.com/c/gerrit/+/281146
Change-Id: Ib5a25c75630888c64c290356690d11d5202a30d3
diff --git a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.ts b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.ts
index d7a1f17..5ca7d887 100644
--- a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list.ts
@@ -35,12 +35,14 @@
Reviewers,
AccountId,
DetailedLabelInfo,
+ EmailAddress,
} from '../../../types/common';
import {PolymerDeepPropertyChange} from '@polymer/polymer/interfaces';
import {GrAccountChip} from '../../shared/gr-account-chip/gr-account-chip';
import {RestApiService} from '../../../services/services/gr-rest-api/gr-rest-api';
import {hasOwnProperty} from '../../../utils/common-util';
import {isRemovableReviewer} from '../../../utils/change-util';
+import {ReviewerState} from '../../../constants/constants';
export interface GrReviewerList {
$: {
@@ -262,7 +264,7 @@
if (!target.account || !this.change) {
return;
}
- const accountID = target.account._account_id;
+ const accountID = target.account._account_id || target.account.email;
this.disabled = true;
if (!accountID) return;
this._xhrPromise = this._removeReviewer(accountID)
@@ -272,12 +274,15 @@
return response;
}
if (!this.change || !this.change.reviewers) return;
- const reviewers: {[type: string]: AccountInfo[] | undefined} = this
- .change!.reviewers;
- for (const type of ['REVIEWER', 'CC']) {
- reviewers[type] = reviewers[type] || [];
- for (let i = 0; i < reviewers[type]!.length; i++) {
- if (reviewers[type]![i]._account_id === accountID) {
+ const reviewers = this.change.reviewers;
+ for (const type of [ReviewerState.REVIEWER, ReviewerState.CC]) {
+ const reviewerStateByType = reviewers[type] || [];
+ reviewers[type] = reviewerStateByType;
+ for (let i = 0; i < reviewerStateByType.length; i++) {
+ if (
+ reviewerStateByType[i]._account_id === accountID ||
+ reviewerStateByType[i].email === accountID
+ ) {
this.splice('change.reviewers.' + type, i, 1);
break;
}
@@ -316,7 +321,7 @@
this._displayedReviewers = this._reviewers;
}
- _removeReviewer(id: AccountId): Promise<Response | undefined> {
+ _removeReviewer(id: AccountId | EmailAddress): Promise<Response | undefined> {
if (!this.change) return Promise.resolve(undefined);
return this.$.restAPI.removeChangeReviewer(this.change._number, id);
}
diff --git a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_test.js b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_test.js
index ad9af30..d29abfc 100644
--- a/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_test.js
+++ b/polygerrit-ui/app/elements/change/gr-reviewer-list/gr-reviewer-list_test.js
@@ -116,6 +116,101 @@
}
});
+ suite('_handleRemove', () => {
+ let removeReviewerStub;
+ let reviewersChangedSpy;
+
+ const reviewerWithId = {
+ _account_id: 2,
+ name: 'Some name',
+ };
+
+ const reviewerWithIdAndEmail = {
+ _account_id: 4,
+ name: 'Some other name',
+ email: 'example@',
+ };
+
+ const reviewerWithEmailOnly = {
+ email: 'example2@example',
+ };
+
+ let chips;
+
+ setup(() => {
+ removeReviewerStub = sinon
+ .stub(element, '_removeReviewer')
+ .returns(Promise.resolve(new Response({status: 200})));
+ element.mutable = true;
+
+ const allReviewers = [
+ reviewerWithId,
+ reviewerWithIdAndEmail,
+ reviewerWithEmailOnly,
+ ];
+
+ element.change = {
+ owner: {
+ _account_id: 1,
+ },
+ reviewers: {
+ REVIEWER: allReviewers,
+ },
+ removable_reviewers: allReviewers,
+ };
+ flush();
+ chips = Array.from(element.root.querySelectorAll('gr-account-chip'));
+ assert.equal(chips.length, allReviewers.length);
+ reviewersChangedSpy = sinon.spy(element, '_reviewersChanged');
+ });
+
+ test('_handleRemove for account with accountId only', async () => {
+ const accountChip = chips.find(chip =>
+ chip.account._account_id === reviewerWithId._account_id
+ );
+ accountChip._handleRemoveTap(new MouseEvent('click'));
+ await flush();
+ assert.isTrue(removeReviewerStub.calledOnce);
+ assert.isTrue(removeReviewerStub.calledWith(reviewerWithId._account_id));
+ assert.isTrue(reviewersChangedSpy.called);
+ expect(element.change.reviewers.REVIEWER).to.have.deep.members([
+ reviewerWithIdAndEmail,
+ reviewerWithEmailOnly,
+ ]);
+ });
+
+ test('_handleRemove for account with accountId and email', async () => {
+ const accountChip = chips.find(chip =>
+ chip.account._account_id === reviewerWithIdAndEmail._account_id
+ );
+ accountChip._handleRemoveTap(new MouseEvent('click'));
+ await flush();
+ assert.isTrue(removeReviewerStub.calledOnce);
+ assert.isTrue(
+ removeReviewerStub.calledWith(reviewerWithIdAndEmail._account_id));
+ assert.isTrue(reviewersChangedSpy.called);
+ expect(element.change.reviewers.REVIEWER).to.have.deep.members([
+ reviewerWithId,
+ reviewerWithEmailOnly,
+ ]);
+ });
+
+ test('_handleRemove for account with email only', async () => {
+ const accountChip = chips.find(
+ chip => chip.account.email === reviewerWithEmailOnly.email
+ );
+ accountChip._handleRemoveTap(new MouseEvent('click'));
+ await flush();
+ assert.isTrue(removeReviewerStub.calledOnce);
+ assert.isTrue(removeReviewerStub.calledWith(reviewerWithEmailOnly.email));
+ assert.isTrue(reviewersChangedSpy.called);
+ expect(element.change.reviewers.REVIEWER).to.have.deep.members([
+ reviewerWithId,
+ reviewerWithIdAndEmail,
+ ]);
+ });
+ });
+
test('tracking reviewers and ccs', () => {
let counter = 0;
function makeAccount() {
