Support groups in bulk action reviewer flow
The confirmation dialog for large groups will be added in in another
change.
Release-Notes: skip
Google-Bug-Id: b/216478677
Change-Id: I7ba8c484e0706ca09bc5b3a44515fb8578b96179
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-reviewer-flow/gr-change-list-reviewer-flow.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-reviewer-flow/gr-change-list-reviewer-flow.ts
index 42ff8f6..34080a1 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-reviewer-flow/gr-change-list-reviewer-flow.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-reviewer-flow/gr-change-list-reviewer-flow.ts
@@ -11,7 +11,6 @@
import {resolve} from '../../../models/dependency';
import {
AccountDetailInfo,
- AccountInfo,
ChangeInfo,
NumericChangeId,
ServerInfo,
@@ -31,10 +30,14 @@
import {allSettled} from '../../../utils/async-util';
import {listForSentence} from '../../../utils/string-util';
import {getDisplayName} from '../../../utils/display-name-util';
-import {AccountInputDetail} from '../../shared/gr-account-list/gr-account-list';
+import {
+ AccountInput,
+ AccountInputDetail,
+} from '../../shared/gr-account-list/gr-account-list';
import '@polymer/iron-icon/iron-icon';
import {getReplyByReason} from '../../../utils/attention-set-util';
import {intersection} from '../../../utils/common-util';
+import {accountOrGroupKey} from '../../../utils/account-util';
@customElement('gr-change-list-reviewer-flow')
export class GrChangeListReviewerFlow extends LitElement {
@@ -43,7 +46,7 @@
// contents are given to gr-account-lists to mutate
@state() private updatedAccountsByReviewerState: Map<
ReviewerState,
- AccountInfo[]
+ AccountInput[]
> = new Map([
[ReviewerState.REVIEWER, []],
[ReviewerState.CC, []],
@@ -254,12 +257,11 @@
.filter(account => account?._account_id !== undefined);
return this.updatedAccountsByReviewerState
.get(updatedReviewerState)!
- .filter(
- account =>
- account._account_id !== undefined &&
- accountsInCurrentState.some(
- otherAccount => otherAccount._account_id === account._account_id
- )
+ .filter(account =>
+ accountsInCurrentState.some(
+ otherAccount =>
+ accountOrGroupKey(otherAccount) === accountOrGroupKey(account)
+ )
)
.map(reviewer => getDisplayName(this.serverConfig, reviewer));
}
@@ -302,7 +304,6 @@
reviewerState: ReviewerState,
event: CustomEvent<AccountInputDetail>
) {
- const account = event.detail.account as AccountInfo;
const oppositeReviewerState =
reviewerState === ReviewerState.CC
? ReviewerState.REVIEWER
@@ -311,7 +312,7 @@
oppositeReviewerState
)!;
const oppositeUpdatedAccountIndex = oppositeUpdatedAccounts.findIndex(
- acc => acc._account_id === account._account_id
+ acc => accountOrGroupKey(acc) === accountOrGroupKey(event.detail.account)
);
if (oppositeUpdatedAccountIndex >= 0) {
oppositeUpdatedAccounts.splice(oppositeUpdatedAccountIndex, 1);
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-reviewer-flow/gr-change-list-reviewer-flow_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-reviewer-flow/gr-change-list-reviewer-flow_test.ts
index e34a269..4a142e4 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-reviewer-flow/gr-change-list-reviewer-flow_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-reviewer-flow/gr-change-list-reviewer-flow_test.ts
@@ -5,7 +5,7 @@
*/
import {fixture, html} from '@open-wc/testing-helpers';
import {SinonStubbedMember} from 'sinon';
-import {AccountInfo, ReviewerState} from '../../../api/rest-api';
+import {AccountInfo, GroupInfo, ReviewerState} from '../../../api/rest-api';
import {
BulkActionsModel,
bulkActionsModelToken,
@@ -17,6 +17,7 @@
import {
createAccountWithIdNameAndEmail,
createChange,
+ createGroupInfo,
} from '../../../test/test-data-generators';
import {
MockPromise,
@@ -43,6 +44,7 @@
createAccountWithIdNameAndEmail(4),
createAccountWithIdNameAndEmail(5),
];
+const groups: GroupInfo[] = [createGroupInfo('groupId')];
const changes: ChangeInfo[] = [
{
...createChange(),
@@ -225,7 +227,7 @@
dialog,
'gr-account-list#cc-list'
);
- reviewerList.accounts.push(accounts[2]);
+ reviewerList.accounts.push(accounts[2], groups[0]);
ccList.accounts.push(accounts[5]);
await flush();
dialog.confirmButton!.click();
@@ -243,6 +245,7 @@
{
reviewers: [
{reviewer: accounts[2]._account_id, state: ReviewerState.REVIEWER},
+ {reviewer: groups[0].id, state: ReviewerState.REVIEWER},
{reviewer: accounts[5]._account_id, state: ReviewerState.CC},
],
ignore_automatic_attention_set_rules: true,
@@ -252,6 +255,10 @@
reason: '<GERRIT_ACCOUNT_1> replied on the change',
user: accounts[2]._account_id,
},
+ {
+ reason: '<GERRIT_ACCOUNT_1> replied on the change',
+ user: groups[0].id,
+ },
],
},
]);
@@ -261,6 +268,7 @@
{
reviewers: [
{reviewer: accounts[2]._account_id, state: ReviewerState.REVIEWER},
+ {reviewer: groups[0].id, state: ReviewerState.REVIEWER},
{reviewer: accounts[5]._account_id, state: ReviewerState.CC},
],
ignore_automatic_attention_set_rules: true,
@@ -270,6 +278,10 @@
reason: '<GERRIT_ACCOUNT_1> replied on the change',
user: accounts[2]._account_id,
},
+ {
+ reason: '<GERRIT_ACCOUNT_1> replied on the change',
+ user: groups[0].id,
+ },
],
},
]);
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 1f57837..6a9bd92 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
@@ -261,7 +261,7 @@
account?: AccountInfo;
@state()
- ccs: (AccountInfoInput | GroupInfoInput)[] = [];
+ ccs: AccountInput[] = [];
@state()
attentionCcsCount = 0;
@@ -1527,8 +1527,8 @@
if (!this.change?.owner || !this.change?.reviewers) return;
this.owner = this.change.owner;
- const reviewers = [];
- const ccs = [];
+ const reviewers: AccountInput[] = [];
+ const ccs: AccountInput[] = [];
if (this.change.reviewers) {
for (const key of Object.keys(this.change.reviewers)) {
diff --git a/polygerrit-ui/app/models/bulk-actions/bulk-actions-model.ts b/polygerrit-ui/app/models/bulk-actions/bulk-actions-model.ts
index c80a1cf..a34f880 100644
--- a/polygerrit-ui/app/models/bulk-actions/bulk-actions-model.ts
+++ b/polygerrit-ui/app/models/bulk-actions/bulk-actions-model.ts
@@ -9,8 +9,9 @@
NumericChangeId,
ChangeStatus,
ReviewerState,
- AccountInfo,
AccountId,
+ AccountInfo,
+ GroupInfo,
} from '../../api/rest-api';
import {Model} from '../model';
import {Finalizable} from '../../services/registry';
@@ -22,6 +23,7 @@
ReviewerInput,
AttentionSetInput,
} from '../../types/common';
+import {accountOrGroupKey} from '../../utils/account-util';
export const bulkActionsModelToken =
define<BulkActionsModel>('bulk-actions-model');
@@ -159,7 +161,7 @@
}
addReviewers(
- changedReviewers: Map<ReviewerState, AccountInfo[]>,
+ changedReviewers: Map<ReviewerState, (AccountInfo | GroupInfo)[]>,
reason: string
): Promise<Response>[] {
const current = this.subject$.getValue();
@@ -260,14 +262,14 @@
private getNewReviewersToChange(
change: ChangeInfo,
state: ReviewerState,
- changedReviewers: Map<ReviewerState, AccountInfo[]>
+ changedReviewers: Map<ReviewerState, (AccountInfo | GroupInfo)[]>
): ReviewerInput[] {
return (
changedReviewers
.get(state)
?.filter(account => !change.reviewers[state]?.includes(account))
.map(account => {
- return {state, reviewer: account._account_id!};
+ return {state, reviewer: accountOrGroupKey(account)};
}) ?? []
);
}
diff --git a/polygerrit-ui/app/models/bulk-actions/bulk-actions-model_test.ts b/polygerrit-ui/app/models/bulk-actions/bulk-actions-model_test.ts
index aaf7123..84d5c4e 100644
--- a/polygerrit-ui/app/models/bulk-actions/bulk-actions-model_test.ts
+++ b/polygerrit-ui/app/models/bulk-actions/bulk-actions-model_test.ts
@@ -7,6 +7,7 @@
import {
createAccountWithIdNameAndEmail,
createChange,
+ createGroupInfo,
createRevisions,
} from '../../test/test-data-generators';
import {
@@ -18,6 +19,7 @@
AccountInfo,
ReviewerState,
AccountId,
+ GroupInfo,
} from '../../api/rest-api';
import {BulkActionsModel, LoadingState} from './bulk-actions-model';
import {getAppContext} from '../../services/app-context';
@@ -200,6 +202,7 @@
createAccountWithIdNameAndEmail(0),
createAccountWithIdNameAndEmail(1),
];
+ const groups: GroupInfo[] = [createGroupInfo('groupId')];
const changes: ChangeInfo[] = [
{
...createChange(),
@@ -234,20 +237,36 @@
test('adds reviewers/cc only to changes that need it', async () => {
bulkActionsModel.addReviewers(
new Map([
- [ReviewerState.REVIEWER, [accounts[0]]],
+ [ReviewerState.REVIEWER, [accounts[0], groups[0]]],
[ReviewerState.CC, [accounts[1]]],
]),
'<GERRIT_ACCOUNT_12345> replied on the change'
);
- // changes[0] is not updated since it already has the reviewer & CC
- assert.isTrue(saveChangeReviewStub.calledOnce);
+ assert.isTrue(saveChangeReviewStub.calledTwice);
+ // changes[0] only adds the group since it already has the other
+ // reviewer/CCs
assert.sameDeepOrderedMembers(saveChangeReviewStub.firstCall.args, [
+ changes[0]._number,
+ 'current',
+ {
+ reviewers: [{reviewer: groups[0].id, state: ReviewerState.REVIEWER}],
+ ignore_automatic_attention_set_rules: true,
+ add_to_attention_set: [
+ {
+ reason: '<GERRIT_ACCOUNT_12345> replied on the change',
+ user: groups[0].id,
+ },
+ ],
+ },
+ ]);
+ assert.sameDeepOrderedMembers(saveChangeReviewStub.secondCall.args, [
changes[1]._number,
'current',
{
reviewers: [
{reviewer: accounts[0]._account_id, state: ReviewerState.REVIEWER},
+ {reviewer: groups[0].id, state: ReviewerState.REVIEWER},
{reviewer: accounts[1]._account_id, state: ReviewerState.CC},
],
ignore_automatic_attention_set_rules: true,
@@ -256,6 +275,10 @@
reason: '<GERRIT_ACCOUNT_12345> replied on the change',
user: accounts[0]._account_id,
},
+ {
+ reason: '<GERRIT_ACCOUNT_12345> replied on the change',
+ user: groups[0].id,
+ },
],
},
]);
diff --git a/polygerrit-ui/app/utils/account-util.ts b/polygerrit-ui/app/utils/account-util.ts
index b7cc77b..b6018ba 100644
--- a/polygerrit-ui/app/utils/account-util.ts
+++ b/polygerrit-ui/app/utils/account-util.ts
@@ -35,7 +35,7 @@
export const ACCOUNT_TEMPLATE_REGEX = '<GERRIT_ACCOUNT_(\\d+)>';
export function accountKey(account: AccountInfo): AccountId | EmailAddress {
- if (account._account_id) return account._account_id;
+ if (account._account_id !== undefined) return account._account_id;
if (account.email) return account.email;
throw new Error('Account has neither _account_id nor email.');
}