Remove pendingRemoval state from GrAccountList
The confirmed property [1] for groups is only relevant when adding a
large group but not when removing the group.
Aim is to make GrAccountList maintain less transient state and be
less smart and let GrReplyDialog be the overall owner of which
account is added/removed.
Testing relies on making sure the existing tests pass which are
checking for the accounts that are removed [2].
[1] https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#reviewer-input
[2] https://cs.opensource.google/gerrit/gerrit/gerrit/+/master:polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts;l=1973?q=%22assert.equal(mutations.length,%205);%22&sq=
Release-Notes: skip
Google-bug-id: b/236921879
Change-Id: I24ecb677031b657b38a4686e7fadc11a9797df6b
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 bfd92d6..cc736cb 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
@@ -26,8 +26,8 @@
SpecialFilePath,
} from '../../../constants/constants';
import {
+ accountKey,
accountOrGroupKey,
- isReviewerOrCC,
mapReviewer,
removeServiceUsers,
} from '../../../utils/account-util';
@@ -63,6 +63,7 @@
ServerInfo,
SuggestedReviewerGroupInfo,
Suggestion,
+ isGroup,
} from '../../../types/common';
import {GrButton} from '../../shared/gr-button/gr-button';
import {GrLabelScores} from '../gr-label-scores/gr-label-scores';
@@ -99,7 +100,7 @@
import {RestApiService} from '../../../services/gr-rest-api/gr-rest-api';
import {resolve} from '../../../models/dependency';
import {changeModelToken} from '../../../models/change/change-model';
-import {ConfigInfo, LabelNameToValuesMap} from '../../../api/rest-api';
+import {ConfigInfo, GroupId, LabelNameToValuesMap} from '../../../api/rest-api';
import {css, html, PropertyValues, LitElement} from 'lit';
import {sharedStyles} from '../../../styles/shared-styles';
import {when} from 'lit/directives/when';
@@ -1281,7 +1282,43 @@
return isResolvedPatchsetLevelComment ? 'resolved' : 'unresolved';
}
- computeReviewers(change: ChangeInfo) {
+ /**
+ * Get the list of users removed.
+ * A user is removed if they were initially present in change.reviewer[state]
+ * and not present in currentAccounts
+ */
+
+ private getRemovals(
+ state: ReviewerState,
+ currentAccounts: AccountInput[]
+ ): AccountInfo[] {
+ const existingAccounts = this.change?.reviewers[state] ?? [];
+ return existingAccounts.filter(
+ existingAccount =>
+ !currentAccounts.some(
+ currentAccount =>
+ accountOrGroupKey(currentAccount) ===
+ accountOrGroupKey(existingAccount)
+ )
+ );
+ }
+
+ private mapAccountToReviewInput(
+ account: AccountInfo | GroupInfo
+ ): ReviewerInput {
+ if (isAccount(account)) {
+ return {
+ reviewer: accountKey(account),
+ state: ReviewerState.REMOVED,
+ };
+ } else if (isGroup(account)) {
+ const reviewer = decodeURIComponent(account.id) as GroupId;
+ return {reviewer, state: ReviewerState.REMOVED};
+ }
+ throw new Error('Must be either an account or a group.');
+ }
+
+ computeReviewers() {
const reviewers: ReviewerInput[] = [];
const addToReviewInput = (
additions: AccountAddition[],
@@ -1295,28 +1332,34 @@
};
addToReviewInput(this.reviewersList!.additions(), ReviewerState.REVIEWER);
addToReviewInput(this.ccsList!.additions(), ReviewerState.CC);
- addToReviewInput(
- this.reviewersList!.removals().filter(
+
+ let removals;
+ removals = this.getRemovals(
+ ReviewerState.REVIEWER,
+ this.reviewersList?.accounts ?? []
+ )
+ .filter(
r =>
- isReviewerOrCC(change, r) &&
- // ignore removal from reviewer request if being added to CC
+ // ignore removal from reviewer request if being added as CC
!this.ccsList!.additions().some(
- account => mapReviewer(account).reviewer === mapReviewer(r).reviewer
+ account => mapReviewer(account).reviewer === accountOrGroupKey(r)
)
- ),
- ReviewerState.REMOVED
- );
- addToReviewInput(
- this.ccsList!.removals().filter(
+ )
+ .map(this.mapAccountToReviewInput);
+ reviewers.push(...removals);
+
+ removals = this.getRemovals(ReviewerState.CC, this.ccsList?.accounts ?? [])
+ .filter(
r =>
- isReviewerOrCC(change, r) &&
// ignore removal from CC request if being added as reviewer
!this.reviewersList!.additions().some(
- account => mapReviewer(account).reviewer === mapReviewer(r).reviewer
+ account => mapReviewer(account).reviewer === accountOrGroupKey(r)
)
- ),
- ReviewerState.REMOVED
- );
+ )
+ .map(this.mapAccountToReviewInput);
+
+ reviewers.push(...removals);
+
return reviewers;
}
@@ -1367,7 +1410,7 @@
}
assertIsDefined(this.change, 'change');
- reviewInput.reviewers = this.computeReviewers(this.change);
+ reviewInput.reviewers = this.computeReviewers();
this.disabled = true;
const errFn = (r?: Response | null) => this.handle400Error(r);
@@ -1816,7 +1859,6 @@
})
);
queryAndAssert<GrTextarea>(this, 'gr-textarea').closeDropdown();
- this.reviewersList?.clearPendingRemovals();
this.rebuildReviewerArrays();
}
diff --git a/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list.ts b/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list.ts
index 1fa21e5..a9b81dd 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list.ts
+++ b/polygerrit-ui/app/elements/shared/gr-account-list/gr-account-list.ts
@@ -164,8 +164,6 @@
private readonly reporting = getAppContext().reportingService;
- private pendingRemoval: Set<AccountInput> = new Set();
-
constructor() {
super();
this.querySuggestions = input => this.getSuggestions(input);
@@ -285,7 +283,6 @@
let itemTypeAdded = 'unknown';
if (isAccountObject(item)) {
account = {...item.account, _pendingAdd: true};
- this.removeFromPendingRemoval(account);
this.accounts.push(account);
itemTypeAdded = 'account';
} else if (isSuggestedReviewerGroupInfo(item)) {
@@ -295,7 +292,6 @@
}
group = {...item.group, _pendingAdd: true, _group: true};
this.accounts.push(group);
- this.removeFromPendingRemoval(group);
itemTypeAdded = 'group';
} else if (this.allowAnyInput) {
if (!item.includes('@')) {
@@ -307,7 +303,6 @@
} else {
account = {email: item as EmailAddress, _pendingAdd: true};
this.accounts.push(account);
- this.removeFromPendingRemoval(account);
itemTypeAdded = 'email';
}
}
@@ -375,7 +370,6 @@
for (let i = 0; i < this.accounts.length; i++) {
if (accountOrGroupKey(toRemove) === accountOrGroupKey(this.accounts[i])) {
this.accounts.splice(i, 1);
- this.pendingRemoval.add(toRemove);
this.reporting.reportInteraction(`Remove from ${this.id}`);
this.requestUpdate();
fire(this, 'accounts-changed', {value: this.accounts.slice()});
@@ -486,24 +480,4 @@
}
});
}
-
- removals(): AccountAddition[] {
- return Array.from(this.pendingRemoval).map(account => {
- if (isGroupInfoInput(account)) {
- return {group: account};
- } else if (isAccountInfoInput(account)) {
- return {account};
- } else {
- throw new Error('AccountInput must be either Account or Group.');
- }
- });
- }
-
- private removeFromPendingRemoval(account: AccountInput) {
- this.pendingRemoval.delete(account);
- }
-
- clearPendingRemovals() {
- this.pendingRemoval.clear();
- }
}
diff --git a/polygerrit-ui/app/utils/account-util.ts b/polygerrit-ui/app/utils/account-util.ts
index 9b8d82a..ebd1c34 100644
--- a/polygerrit-ui/app/utils/account-util.ts
+++ b/polygerrit-ui/app/utils/account-util.ts
@@ -17,7 +17,7 @@
ReviewerInput,
ServerInfo,
} from '../types/common';
-import {AccountTag, ReviewerState} from '../constants/constants';
+import {AccountTag} from '../constants/constants';
import {assertNever, hasOwnProperty} from './common-util';
import {AccountAddition} from '../elements/shared/gr-account-list/gr-account-list';
import {getDisplayName} from './display-name-util';
@@ -48,18 +48,6 @@
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);
}
