Merge "Ensure existing reviewers are removed as part of ReviewInput"
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index e303dad..5504877 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -40,8 +40,9 @@
} from '../../../constants/constants';
import {KeyboardShortcutMixin} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {
- accountKey,
accountOrGroupKey,
+ isReviewerOrCC,
+ mapReviewer,
removeServiceUsers,
} from '../../../utils/account-util';
import {getDisplayName} from '../../../utils/display-name-util';
@@ -74,7 +75,6 @@
ParsedJSON,
PatchSetNum,
ProjectInfo,
- ReviewerInput,
Reviewers,
ReviewInput,
ReviewResult,
@@ -542,18 +542,6 @@
}
}
- _mapReviewer(addition: AccountAddition): ReviewerInput {
- if (addition.account) {
- return {reviewer: accountKey(addition.account)};
- }
- if (addition.group) {
- const reviewer = decodeURIComponent(addition.group.id) as GroupId;
- const confirmed = addition.group.confirmed;
- return {reviewer, confirmed};
- }
- throw new Error('Reviewer must be either an account or a group.');
- }
-
send(includeComments: boolean, startReview: boolean) {
this.reporting.time(Timing.SEND_REPLY);
const labels = this.$.labelScores.getLabelValues();
@@ -606,16 +594,24 @@
state?: ReviewerState
) => {
additions.forEach(addition => {
- const reviewer = this._mapReviewer(addition);
+ const reviewer = mapReviewer(addition);
if (state) reviewer.state = state;
reviewInput.reviewers?.push(reviewer);
});
};
reviewInput.reviewers = [];
+ assertIsDefined(this.change, 'change');
+ const change = this.change;
addToReviewInput(this.$.reviewers.additions(), ReviewerState.REVIEWER);
addToReviewInput(this.$.ccs.additions(), ReviewerState.CC);
- addToReviewInput(this.$.reviewers.removals(), ReviewerState.REMOVED);
- addToReviewInput(this.$.ccs.removals(), ReviewerState.REMOVED);
+ addToReviewInput(
+ this.$.reviewers.removals().filter(r => isReviewerOrCC(change, r)),
+ ReviewerState.REMOVED
+ );
+ addToReviewInput(
+ this.$.ccs.removals().filter(r => isReviewerOrCC(change, r)),
+ ReviewerState.REMOVED
+ );
this.disabled = true;
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js
index b51eb5f..f5474d8 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js
@@ -1075,6 +1075,11 @@
element._reviewers = [reviewer1, reviewer2];
element._ccs = [cc1, cc2, cc3];
+ element.change.reviewers = {
+ [ReviewerState.CC]: [],
+ [ReviewerState.REVIEWER]: [{_account_id: 33}],
+ };
+
const mutations = [];
stubSaveReview(review => mutations.push(...review.reviewers));
@@ -1128,7 +1133,7 @@
// Send and purge and verify moves, delete cc3.
await element.send();
- expect(mutations).to.have.lengthOf(7);
+ expect(mutations).to.have.lengthOf(5);
expect(mutations[0]).to.deep.equal(mapReviewer(cc1,
ReviewerState.REVIEWER));
expect(mutations[1]).to.deep.equal(mapReviewer(cc2,
@@ -1138,13 +1143,9 @@
expect(mutations[3]).to.deep.equal(mapReviewer(reviewer2,
ReviewerState.CC));
- // 3 remove events stored
+ // Only 1 account was initially part of the change
expect(mutations[4]).to.deep.equal({reviewer: 33, state:
ReviewerState.REMOVED});
- expect(mutations[5]).to.deep.equal({reviewer: 35, state:
- ReviewerState.REMOVED});
- expect(mutations[6]).to.deep.equal({reviewer: 37, state:
- ReviewerState.REMOVED});
});
test('emits cancel on esc key', () => {
diff --git a/polygerrit-ui/app/utils/account-util.ts b/polygerrit-ui/app/utils/account-util.ts
index ff37608..2a509d3 100644
--- a/polygerrit-ui/app/utils/account-util.ts
+++ b/polygerrit-ui/app/utils/account-util.ts
@@ -18,13 +18,17 @@
import {
AccountId,
AccountInfo,
+ ChangeInfo,
EmailAddress,
+ GroupId,
GroupInfo,
isAccount,
isGroup,
+ ReviewerInput,
} from '../types/common';
-import {AccountTag} from '../constants/constants';
+import {AccountTag, ReviewerState} from '../constants/constants';
import {assertNever} from './common-util';
+import {AccountAddition} from '../elements/shared/gr-account-list/gr-account-list';
export function accountKey(account: AccountInfo): AccountId | EmailAddress {
if (account._account_id) return account._account_id;
@@ -32,6 +36,30 @@
throw new Error('Account has neither _account_id nor email.');
}
+export function mapReviewer(addition: AccountAddition): ReviewerInput {
+ if (addition.account) {
+ return {reviewer: accountKey(addition.account)};
+ }
+ if (addition.group) {
+ const reviewer = decodeURIComponent(addition.group.id) as GroupId;
+ const confirmed = addition.group.confirmed;
+ return {reviewer, confirmed};
+ }
+ throw new Error('Reviewer must be either an account or a group.');
+}
+
+export function isReviewerOrCC(
+ change: ChangeInfo,
+ reviewerAddition: AccountAddition
+): boolean {
+ const reviewers = [
+ ...(change.reviewers[ReviewerState.CC] ?? []),
+ ...(change.reviewers[ReviewerState.REVIEWER] ?? []),
+ ];
+ const reviewer = mapReviewer(reviewerAddition);
+ return reviewers.some(r => accountOrGroupKey(r) === reviewer.reviewer);
+}
+
export function isServiceUser(account?: AccountInfo): boolean {
return !!account?.tags?.includes(AccountTag.SERVICE_USER);
}
