Confirm dialog for large groups in reviewer flow
This dialog matches the same confirm flow in the reply-dialog.
https://imgur.com/a/al7n41m
Release-Notes: skip
Change-Id: I11273b3872dada2fade2d7be822627c1c01c40f9
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 34080a1..80606b1 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
@@ -14,6 +14,7 @@
ChangeInfo,
NumericChangeId,
ServerInfo,
+ SuggestedReviewerGroupInfo,
} from '../../../types/common';
import {subscribe} from '../../lit/subscription-controller';
import '../../shared/gr-overlay/gr-overlay';
@@ -32,12 +33,13 @@
import {getDisplayName} from '../../../utils/display-name-util';
import {
AccountInput,
- AccountInputDetail,
+ GrAccountList,
} 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';
+import {ValueChangedEvent} from '../../../types/events';
@customElement('gr-change-list-reviewer-flow')
export class GrChangeListReviewerFlow extends LitElement {
@@ -66,7 +68,25 @@
@state() private serverConfig?: ServerInfo;
- @query('gr-overlay') private overlay!: GrOverlay;
+ @state()
+ private groupPendingConfirmationByReviewerState: Map<
+ ReviewerState,
+ SuggestedReviewerGroupInfo | null
+ > = new Map([
+ [ReviewerState.REVIEWER, null],
+ [ReviewerState.CC, null],
+ ]);
+
+ @query('gr-overlay#flow') private overlay?: GrOverlay;
+
+ @query('gr-account-list#reviewer-list') private reviewerList?: GrAccountList;
+
+ @query('gr-account-list#cc-list') private ccList?: GrAccountList;
+
+ @query('gr-overlay#confirm-reviewer')
+ private reviewerConfirmOverlay?: GrOverlay;
+
+ @query('gr-overlay#confirm-cc') private ccConfirmOverlay?: GrOverlay;
private readonly reportingService = getAppContext().reportingService;
@@ -113,6 +133,14 @@
--iron-icon-height: 18px;
--iron-icon-width: 18px;
}
+ gr-overlay#confirm-cc,
+ gr-overlay#confirm-reviewer {
+ padding: var(--spacing-l);
+ text-align: center;
+ }
+ .confirmation-buttons {
+ margin-top: var(--spacing-l);
+ }
`;
}
@@ -141,7 +169,6 @@
}
override render() {
- // TODO: factor out button+dialog component with promise-progress tracking
return html`
<gr-button
id="start-flow"
@@ -150,7 +177,7 @@
@click=${() => this.openOverlay()}
>add reviewer/cc</gr-button
>
- <gr-overlay with-backdrop>
+ <gr-overlay id="flow" with-backdrop>
${this.isOverlayOpen ? this.renderDialog() : nothing}
</gr-overlay>
`;
@@ -195,8 +222,6 @@
if (!updatedAccounts || !suggestionsProvider) {
return;
}
- // @accounts-changed will notify us when an account is added or removed, so
- // we need to re-render to update warning messages.
return html`
<gr-account-list
id=${id}
@@ -204,11 +229,48 @@
.removableValues=${[]}
.suggestionsProvider=${suggestionsProvider}
.placeholder=${placeholder}
- @accounts-changed=${() => this.requestUpdate()}
- @account-added=${(e: CustomEvent<AccountInputDetail>) =>
- this.onAccountAdded(reviewerState, e)}
+ .pendingConfirmation=${this.groupPendingConfirmationByReviewerState.get(
+ reviewerState
+ )}
+ @accounts-changed=${() => this.onAccountsChanged(reviewerState)}
+ @pending-confirmation-changed=${(
+ ev: ValueChangedEvent<SuggestedReviewerGroupInfo | null>
+ ) => this.onPendingConfirmationChanged(reviewerState, ev)}
>
</gr-account-list>
+ ${this.renderConfirmationDialog(reviewerState)}
+ `;
+ }
+
+ private renderConfirmationDialog(reviewerState: ReviewerState) {
+ const id =
+ reviewerState === ReviewerState.CC ? 'confirm-cc' : 'confirm-reviewer';
+ const suggestion =
+ this.groupPendingConfirmationByReviewerState.get(reviewerState);
+ return html`
+ <gr-overlay
+ id=${id}
+ @iron-overlay-canceled=${() => this.cancelPendingGroup(reviewerState)}
+ >
+ <div class="confirmation-text">
+ Group
+ <span class="groupName"> ${suggestion?.group.name} </span>
+ has
+ <span class="groupSize"> ${suggestion?.count} </span>
+ members.
+ <br />
+ Are you sure you want to add them all?
+ </div>
+ <div class="confirmation-buttons">
+ <gr-button
+ @click=${() => this.confirmPendingGroup(reviewerState, suggestion)}
+ >Yes</gr-button
+ >
+ <gr-button @click=${() => this.cancelPendingGroup(reviewerState)}
+ >No</gr-button
+ >
+ </div>
+ </gr-overlay>
`;
}
@@ -269,12 +331,12 @@
private openOverlay() {
this.resetFlow();
this.isOverlayOpen = true;
- this.overlay.open();
+ this.overlay?.open();
}
private closeOverlay() {
this.isOverlayOpen = false;
- this.overlay.close();
+ this.overlay?.close();
}
private resetFlow() {
@@ -299,11 +361,15 @@
this.requestUpdate();
}
- /* Removes accounts from one list when they are added to the other */
- private onAccountAdded(
- reviewerState: ReviewerState,
- event: CustomEvent<AccountInputDetail>
- ) {
+ /*
+ * Removes accounts from one list when they are added to the other. Also
+ * trigger re-render so warnings will update as accounts are added, removed,
+ * and confirmed.
+ */
+ private onAccountsChanged(reviewerState: ReviewerState) {
+ const reviewerStateKeys = this.updatedAccountsByReviewerState
+ .get(reviewerState)!
+ .map(accountOrGroupKey);
const oppositeReviewerState =
reviewerState === ReviewerState.CC
? ReviewerState.REVIEWER
@@ -311,13 +377,61 @@
const oppositeUpdatedAccounts = this.updatedAccountsByReviewerState.get(
oppositeReviewerState
)!;
- const oppositeUpdatedAccountIndex = oppositeUpdatedAccounts.findIndex(
- acc => accountOrGroupKey(acc) === accountOrGroupKey(event.detail.account)
+
+ const notOverwrittenOppositeAccounts = oppositeUpdatedAccounts.filter(
+ acc => !reviewerStateKeys.includes(accountOrGroupKey(acc))
);
- if (oppositeUpdatedAccountIndex >= 0) {
- oppositeUpdatedAccounts.splice(oppositeUpdatedAccountIndex, 1);
- this.requestUpdate();
+ if (
+ notOverwrittenOppositeAccounts.length !== oppositeUpdatedAccounts.length
+ ) {
+ this.updatedAccountsByReviewerState.set(
+ oppositeReviewerState,
+ notOverwrittenOppositeAccounts
+ );
}
+ this.requestUpdate();
+ }
+
+ private async onPendingConfirmationChanged(
+ reviewerState: ReviewerState,
+ ev: ValueChangedEvent<SuggestedReviewerGroupInfo | null>
+ ) {
+ this.groupPendingConfirmationByReviewerState.set(
+ reviewerState,
+ ev.detail.value
+ );
+ this.requestUpdate();
+ await this.updateComplete;
+
+ const overlay =
+ reviewerState === ReviewerState.CC
+ ? this.ccConfirmOverlay
+ : this.reviewerConfirmOverlay;
+ if (ev.detail.value === null) {
+ overlay?.close();
+ } else {
+ overlay?.open();
+ }
+ }
+
+ private cancelPendingGroup(reviewerState: ReviewerState) {
+ const overlay =
+ reviewerState === ReviewerState.CC
+ ? this.ccConfirmOverlay
+ : this.reviewerConfirmOverlay;
+ overlay?.close();
+ this.groupPendingConfirmationByReviewerState.set(reviewerState, null);
+ this.requestUpdate();
+ }
+
+ private confirmPendingGroup(
+ reviewerState: ReviewerState,
+ suggestion: SuggestedReviewerGroupInfo | null | undefined
+ ) {
+ if (!suggestion) return;
+ const accountList =
+ reviewerState === ReviewerState.CC ? this.ccList : this.reviewerList;
+ accountList?.confirmGroup(suggestion.group);
}
private onConfirm(overallStatus: ProgressStatus) {
@@ -326,10 +440,10 @@
this.saveReviewers();
break;
case ProgressStatus.SUCCESSFUL:
- this.overlay.close();
+ this.overlay?.close();
break;
case ProgressStatus.FAILED:
- this.overlay.close();
+ this.overlay?.close();
break;
}
}
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 4a142e4..9139c38 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,12 @@
*/
import {fixture, html} from '@open-wc/testing-helpers';
import {SinonStubbedMember} from 'sinon';
-import {AccountInfo, GroupInfo, ReviewerState} from '../../../api/rest-api';
+import {
+ AccountInfo,
+ GroupId,
+ GroupInfo,
+ ReviewerState,
+} from '../../../api/rest-api';
import {
BulkActionsModel,
bulkActionsModelToken,
@@ -108,6 +113,7 @@
>add reviewer/cc</gr-button
>
<gr-overlay
+ id="flow"
aria-hidden="true"
with-backdrop=""
tabindex="-1"
@@ -184,6 +190,7 @@
>add reviewer/cc</gr-button
>
<gr-overlay
+ id="flow"
with-backdrop=""
tabindex="-1"
style="outline: none; display: none;"
@@ -194,8 +201,54 @@
<div class="grid">
<span>Reviewers</span>
<gr-account-list id="reviewer-list"></gr-account-list>
+ <gr-overlay
+ aria-hidden="true"
+ id="confirm-reviewer"
+ style="outline: none; display: none;"
+ >
+ <div class="confirmation-text">
+ Group
+ <span class="groupName"></span>
+ has
+ <span class="groupSize"></span>
+ members.
+ <br />
+ Are you sure you want to add them all?
+ </div>
+ <div class="confirmation-buttons">
+ <gr-button aria-disabled="false" role="button" tabindex="0">
+ Yes
+ </gr-button>
+ <gr-button aria-disabled="false" role="button" tabindex="0">
+ No
+ </gr-button>
+ </div>
+ </gr-overlay>
<span>CC</span>
<gr-account-list id="cc-list"></gr-account-list>
+ <gr-overlay
+ aria-hidden="true"
+ id="confirm-cc"
+ style="outline: none; display: none;"
+ >
+ <div class="confirmation-text">
+ Group
+ <span class="groupName"></span>
+ has
+ <span class="groupSize"></span>
+ members.
+ <br />
+ Are you sure you want to add them all?
+ </div>
+ <div class="confirmation-buttons">
+ <gr-button aria-disabled="false" role="button" tabindex="0">
+ Yes
+ </gr-button>
+ <gr-button aria-disabled="false" role="button" tabindex="0">
+ No
+ </gr-button>
+ </div>
+ </gr-overlay>
</div>
</div>
</gr-dialog>
@@ -385,7 +438,7 @@
);
await flush();
- // prettier and shadowDom string don't agree on long text in divs
+ // prettier and shadowDom string don't agree on the long text in divs
expect(element).shadowDom.to.equal(
/* prettier-ignore */
/* HTML */ `
@@ -397,15 +450,65 @@
tabindex="0"
>add reviewer/cc</gr-button
>
- <gr-overlay with-backdrop="" tabindex="-1">
+ <gr-overlay id="flow" with-backdrop="" tabindex="-1">
<gr-dialog role="dialog">
<div slot="header">Add reviewer / CC</div>
<div slot="main">
<div class="grid">
<span>Reviewers</span>
<gr-account-list id="reviewer-list"></gr-account-list>
+ <gr-overlay aria-hidden="true" id="confirm-reviewer">
+ <div class="confirmation-text">
+ Group
+ <span class="groupName"></span>
+ has
+ <span class="groupSize"></span>
+ members.
+ <br>
+ Are you sure you want to add them all?
+ </div>
+ <div class="confirmation-buttons">
+ <gr-button
+ aria-disabled="false"
+ role="button"
+ tabindex="0">
+ Yes
+ </gr-button>
+ <gr-button
+ aria-disabled="false"
+ role="button"
+ tabindex="0">
+ No
+ </gr-button>
+ </div>
+ </gr-overlay>
<span>CC</span>
<gr-account-list id="cc-list"></gr-account-list>
+ <gr-overlay aria-hidden="true" id="confirm-cc">
+ <div class="confirmation-text">
+ Group
+ <span class="groupName"></span>
+ has
+ <span class="groupSize"></span>
+ members.
+ <br>
+ Are you sure you want to add them all?
+ </div>
+ <div class="confirmation-buttons">
+ <gr-button
+ aria-disabled="false"
+ role="button"
+ tabindex="0">
+ Yes
+ </gr-button>
+ <gr-button
+ aria-disabled="false"
+ role="button"
+ tabindex="0">
+ No
+ </gr-button>
+ </div>
+ </gr-overlay>
</div>
<div class="warning">
<iron-icon icon="gr-icons:warning"></iron-icon>
@@ -429,5 +532,154 @@
}
);
});
+
+ test('shows confirmation dialog when large group is added', async () => {
+ const reviewerList = queryAndAssert<GrAccountList>(
+ dialog,
+ 'gr-account-list#reviewer-list'
+ );
+ reviewerList.handleAdd(
+ new CustomEvent('add', {
+ detail: {
+ value: {
+ group: {
+ id: '5',
+ name: 'large-group',
+ },
+ count: 12,
+ confirm: true,
+ },
+ },
+ }) as unknown as ValueChangedEvent<string>
+ );
+ await flush();
+
+ const confirmDialog = queryAndAssert(
+ element,
+ 'gr-overlay#confirm-reviewer'
+ );
+ assert.isTrue(
+ getComputedStyle(confirmDialog).getPropertyValue('display') !== 'none'
+ );
+ });
+
+ test('confirms large group', async () => {
+ const reviewerList = queryAndAssert<GrAccountList>(
+ dialog,
+ 'gr-account-list#reviewer-list'
+ );
+ reviewerList.handleAdd(
+ new CustomEvent('add', {
+ detail: {
+ value: {
+ group: {
+ id: '5',
+ name: 'large-group',
+ },
+ count: 12,
+ confirm: true,
+ },
+ },
+ }) as unknown as ValueChangedEvent<string>
+ );
+ await flush();
+ // "Yes" button is first
+ queryAndAssert<GrButton>(
+ element,
+ '.confirmation-buttons > gr-button:first-of-type'
+ ).click();
+ await flush();
+
+ const confirmDialog = queryAndAssert(
+ element,
+ 'gr-overlay#confirm-reviewer'
+ );
+ assert.isTrue(
+ getComputedStyle(confirmDialog).getPropertyValue('display') === 'none'
+ );
+ assert.deepEqual(reviewerList.accounts[1], {
+ _group: true,
+ _pendingAdd: true,
+ confirmed: true,
+ id: '5' as GroupId,
+ name: 'large-group',
+ });
+ });
+
+ test('no confirmation dialog for small group', async () => {
+ const reviewerList = queryAndAssert<GrAccountList>(
+ dialog,
+ 'gr-account-list#reviewer-list'
+ );
+ // "confirm" field is used to decide whether to use the confirmation flow,
+ // not the count. "confirm" value comes from server based on count
+ // threshold
+ reviewerList.handleAdd(
+ new CustomEvent('add', {
+ detail: {
+ value: {
+ group: {
+ id: '5',
+ name: 'small-group',
+ },
+ count: 2,
+ confirm: false,
+ },
+ },
+ }) as unknown as ValueChangedEvent<string>
+ );
+ await flush();
+ const confirmDialog = queryAndAssert(
+ element,
+ 'gr-overlay#confirm-reviewer'
+ );
+ assert.isTrue(
+ getComputedStyle(confirmDialog).getPropertyValue('display') === 'none'
+ );
+ assert.deepEqual(reviewerList.accounts[1], {
+ _group: true,
+ _pendingAdd: true,
+ id: '5' as GroupId,
+ name: 'small-group',
+ });
+ });
+
+ test('no button cancels large group', async () => {
+ const reviewerList = queryAndAssert<GrAccountList>(
+ dialog,
+ 'gr-account-list#reviewer-list'
+ );
+ reviewerList.handleAdd(
+ new CustomEvent('add', {
+ detail: {
+ value: {
+ group: {
+ id: '5',
+ name: 'large-group',
+ },
+ count: 12,
+ confirm: true,
+ },
+ },
+ }) as unknown as ValueChangedEvent<string>
+ );
+ await flush();
+ // "No" button is last
+ queryAndAssert<GrButton>(
+ element,
+ '.confirmation-buttons > gr-button:last-of-type'
+ ).click();
+ await flush();
+
+ const confirmDialog = queryAndAssert(
+ element,
+ 'gr-overlay#confirm-reviewer'
+ );
+ assert.isTrue(
+ getComputedStyle(confirmDialog).getPropertyValue('display') === 'none'
+ );
+ // Group not present
+ assert.sameDeepMembers(reviewerList.accounts, [accounts[0]]);
+ });
});
});
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 6a9bd92..3fd7c89 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
@@ -1276,6 +1276,8 @@
return selectorEl?.selectedValue;
}
+ // TODO: Combine logic into handleReviewersChanged & handleCCsChanged and
+ // remove account-added event from GrAccountList.
accountAdded(e: CustomEvent<AccountInputDetail>) {
const account = e.detail.account;
const key = accountOrGroupKey(account);
