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);