commit 0a4705a910bef6c75f8529396a90fa568fd5b6cf
Author: Ben Rohlfs <brohlfs@google.com>
Date:   Wed Sep 30 13:39:15 2020 +0200

    When adding more than 2 users to the attention set show a warning
    
    And also automatically expand the attention set section in the reply
    dialog.
    
    Screenshot: https://imgur.com/a/CY8xP1p
    Change-Id: I25c4f48a0ddbd5ffd10001b2f6af7b1301b419f9

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 de56e9d..95d76d8 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
@@ -324,7 +324,7 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
   _commentEditing = false;
 
   @property({type: Boolean})
-  _attentionModified = false;
+  _attentionExpanded = false;
 
   @property({type: Object})
   _currentAttentionSet: Set<AccountId> = new Set();
@@ -337,7 +337,8 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
     computed:
       '_computeSendButtonDisabled(canBeStarted, ' +
       'draftCommentThreads, draft, _reviewersMutated, _labelsChanged, ' +
-      '_includeComments, disabled, _commentEditing, _attentionModified)',
+      '_includeComments, disabled, _commentEditing, _attentionExpanded, ' +
+      '_currentAttentionSet, _newAttentionSet)',
     observer: '_sendDisabledChanged',
   })
   _sendDisabled?: boolean;
@@ -646,7 +647,7 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
         }
       }
       this.reportAttentionSetChanges(
-        this._attentionModified,
+        this._attentionExpanded,
         reviewInput.add_to_attention_set,
         reviewInput.remove_from_attention_set
       );
@@ -872,7 +873,7 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
   }
 
   _handleAttentionModify() {
-    this._attentionModified = true;
+    this._attentionExpanded = true;
     // If the attention-detail section is expanded without dispatching this
     // event, then the dialog may expand beyond the screen's bottom border.
     this.dispatchEvent(
@@ -880,12 +881,12 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
     );
   }
 
-  _showAttentionSummary(config?: ServerInfo, attentionModified?: boolean) {
-    return this._isAttentionSetEnabled(config) && !attentionModified;
+  _showAttentionSummary(config?: ServerInfo, attentionExpanded?: boolean) {
+    return this._isAttentionSetEnabled(config) && !attentionExpanded;
   }
 
-  _showAttentionDetails(config?: ServerInfo, attentionModified?: boolean) {
-    return this._isAttentionSetEnabled(config) && attentionModified;
+  _showAttentionDetails(config?: ServerInfo, attentionExpanded?: boolean) {
+    return this._isAttentionSetEnabled(config) && attentionExpanded;
   }
 
   _isAttentionSetEnabled(config?: ServerInfo) {
@@ -956,7 +957,6 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
     ) {
       return;
     }
-    this._attentionModified = false;
     this._currentAttentionSet = new Set(
       Object.keys(change.attention_set || {}).map(
         id => parseInt(id) as AccountId
@@ -1008,6 +1008,27 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
     this._newAttentionSet = new Set(
       [...newAttention].filter(id => allAccountIds.includes(id))
     );
+    this._attentionExpanded = this._computeShowAttentionTip(
+      currentUser,
+      change.owner,
+      this._currentAttentionSet,
+      this._newAttentionSet
+    );
+  }
+
+  _computeShowAttentionTip(
+    currentUser?: AccountInfo,
+    owner?: AccountInfo,
+    currentAttentionSet?: Set<AccountId>,
+    newAttentionSet?: Set<AccountId>
+  ) {
+    if (!currentUser || !owner || !currentAttentionSet || !newAttentionSet)
+      return false;
+    const isOwner = currentUser._account_id === owner._account_id;
+    const addedIds = [...newAttentionSet].filter(
+      id => !currentAttentionSet.has(id)
+    );
+    return isOwner && addedIds.length > 2;
   }
 
   _computeCommentAccounts(threads: CommentThread[]) {
@@ -1352,7 +1373,9 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
     includeComments?: boolean,
     disabled?: boolean,
     commentEditing?: boolean,
-    attentionModified?: boolean
+    attentionExpanded?: boolean,
+    _currentAttentionSet?: Set<AccountId>,
+    _newAttentionSet?: Set<AccountId>
   ) {
     if (
       canBeStarted === undefined ||
@@ -1363,7 +1386,9 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
       includeComments === undefined ||
       disabled === undefined ||
       commentEditing === undefined ||
-      attentionModified === undefined
+      attentionExpanded === undefined ||
+      _currentAttentionSet === undefined ||
+      _newAttentionSet === undefined
     ) {
       return undefined;
     }
@@ -1374,12 +1399,28 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
       return false;
     }
     const hasDrafts = includeComments && draftCommentThreads.length;
+    // Changing the attention set is also sufficient to enable the 'Send'
+    // button, but only if the attention set detail section was expanded and
+    // either someone other than the owner was added, or someone other than the
+    // current user was removed.
+    const isNotCurrentUser = (id: AccountId) =>
+      id !== this._account?._account_id;
+    const isNotOwner = (id: AccountId) => id !== this._owner?._account_id;
+    const attentionAdditions = [..._newAttentionSet]
+      .filter(id => !_currentAttentionSet.has(id))
+      .filter(isNotOwner);
+    const attentionRemovals = [..._currentAttentionSet]
+      .filter(id => !_newAttentionSet.has(id))
+      .filter(isNotCurrentUser);
+    const hasAttentionChange =
+      attentionExpanded &&
+      (attentionAdditions.length > 0 || attentionRemovals.length > 0);
     return (
       !hasDrafts &&
       !text.length &&
       !reviewersMutated &&
       !labelsChanged &&
-      !attentionModified
+      !hasAttentionChange
     );
   }
 
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts
index 1be2f75..247e891 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts
@@ -206,6 +206,16 @@ export const htmlTemplate = html`
       color: var(--deemphasized-text-color);
       margin-bottom: var(--spacing-m);
     }
+    .attentionTip {
+      padding: var(--spacing-m);
+      border: 1px solid var(--border-color);
+      border-radius: var(--border-radius);
+      margin-top: var(--spacing-m);
+      background-color: var(--assignee-highlight-color);
+    }
+    .attentionTip div iron-icon {
+      margin-right: var(--spacing-s);
+    }
   </style>
   <div class="container" tabindex="-1">
     <section class="peopleContainer">
@@ -352,7 +362,7 @@ export const htmlTemplate = html`
     </section>
     <div class="stickyBottom">
       <section
-        hidden$="[[!_showAttentionSummary(serverConfig, _attentionModified)]]"
+        hidden$="[[!_showAttentionSummary(serverConfig, _attentionExpanded)]]"
         class="attention"
       >
         <div class="attentionSummary">
@@ -416,7 +426,7 @@ export const htmlTemplate = html`
         </div>
       </section>
       <section
-        hidden$="[[!_showAttentionDetails(serverConfig, _attentionModified)]]"
+        hidden$="[[!_showAttentionDetails(serverConfig, _attentionExpanded)]]"
         class="attention-detail"
       >
         <div class="attentionDetailsTitle">
@@ -523,6 +533,18 @@ export const htmlTemplate = html`
             </div>
           </div>
         </template>
+        <template
+          is="dom-if"
+          if="[[_computeShowAttentionTip(account, _owner, _currentAttentionSet, _newAttentionSet)]]"
+        >
+          <div class="attentionTip">
+            <iron-icon
+              class="pointer"
+              icon="gr-icons:lightbulb-outline"
+            ></iron-icon>
+            Be mindful of requiring attention from too many reviewers.
+          </div>
+        </template>
       </section>
       <section class="actions">
         <div class="left">
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 abbb1d4..246ec88 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
@@ -958,7 +958,7 @@ suite('gr-reply-dialog tests', () => {
         element.shadowRoot.querySelector('.edit-attention-button'));
     flush();
 
-    assert.isTrue(element._attentionModified);
+    assert.isTrue(element._attentionExpanded);
     let accountLabels = Array.from(element.shadowRoot.querySelectorAll(
         '.attention-detail gr-account-label'));
     assert.equal(accountLabels.length, 5);
@@ -969,13 +969,13 @@ suite('gr-reply-dialog tests', () => {
 
     // The 'attention modified' section collapses and resets when reviewers or
     // ccs change.
-    assert.isFalse(element._attentionModified);
+    assert.isFalse(element._attentionExpanded);
 
     MockInteractions.tap(
         element.shadowRoot.querySelector('.edit-attention-button'));
     flush();
 
-    assert.isTrue(element._attentionModified);
+    assert.isTrue(element._attentionExpanded);
     accountLabels = Array.from(element.shadowRoot.querySelectorAll(
         '.attention-detail gr-account-label'));
     assert.equal(accountLabels.length, 7);
@@ -1324,7 +1324,9 @@ suite('gr-reply-dialog tests', () => {
         /* includeComments= */ false,
         /* disabled= */ false,
         /* commentEditing= */ false,
-        /* attentionModified= */ false
+        /* attentionExpanded= */ false,
+        /* _currentAttentionSet */ new Set(),
+        /* _newAttentionSet */ new Set()
     ));
   });
 
@@ -1340,11 +1342,13 @@ suite('gr-reply-dialog tests', () => {
         /* includeComments= */ false,
         /* disabled= */ false,
         /* commentEditing= */ false,
-        /* attentionModified= */ false
+        /* attentionExpanded= */ false,
+        /* _currentAttentionSet */ new Set(),
+        /* _newAttentionSet */ new Set()
     ));
   });
 
-  test('_computeSendButtonDisabled_attentionModified true', () => {
+  test('_computeSendButtonDisabled_attentionExpanded true', () => {
     const fn = element._computeSendButtonDisabled.bind(element);
     // Mock everything false
     assert.isFalse(fn(
@@ -1356,7 +1360,9 @@ suite('gr-reply-dialog tests', () => {
         /* includeComments= */ false,
         /* disabled= */ false,
         /* commentEditing= */ false,
-        /* attentionModified= */ true
+        /* attentionExpanded= */ true,
+        /* _currentAttentionSet */ new Set(),
+        /* _newAttentionSet */ new Set([1])
     ));
   });
 
@@ -1372,7 +1378,9 @@ suite('gr-reply-dialog tests', () => {
         /* includeComments= */ true,
         /* disabled= */ false,
         /* commentEditing= */ false,
-        /* attentionModified= */ false
+        /* attentionExpanded= */ false,
+        /* _currentAttentionSet */ new Set(),
+        /* _newAttentionSet */ new Set()
     ));
   });
 
@@ -1388,7 +1396,9 @@ suite('gr-reply-dialog tests', () => {
         /* includeComments= */ false,
         /* disabled= */ false,
         /* commentEditing= */ false,
-        /* attentionModified= */ false
+        /* attentionExpanded= */ false,
+        /* _currentAttentionSet */ new Set(),
+        /* _newAttentionSet */ new Set()
     ));
   });
 
@@ -1404,7 +1414,9 @@ suite('gr-reply-dialog tests', () => {
         /* includeComments= */ false,
         /* disabled= */ false,
         /* commentEditing= */ false,
-        /* attentionModified= */ false
+        /* attentionExpanded= */ false,
+        /* _currentAttentionSet */ new Set(),
+        /* _newAttentionSet */ new Set()
     ));
   });
 
@@ -1420,7 +1432,9 @@ suite('gr-reply-dialog tests', () => {
         /* includeComments= */ false,
         /* disabled= */ false,
         /* commentEditing= */ false,
-        /* attentionModified= */ false
+        /* attentionExpanded= */ false,
+        /* _currentAttentionSet */ new Set(),
+        /* _newAttentionSet */ new Set()
     ));
   });
 
@@ -1436,7 +1450,9 @@ suite('gr-reply-dialog tests', () => {
         /* includeComments= */ false,
         /* disabled= */ false,
         /* commentEditing= */ false,
-        /* attentionModified= */ false
+        /* attentionExpanded= */ false,
+        /* _currentAttentionSet */ new Set(),
+        /* _newAttentionSet */ new Set()
     ));
   });
 
@@ -1452,7 +1468,9 @@ suite('gr-reply-dialog tests', () => {
         /* includeComments= */ false,
         /* disabled= */ true,
         /* commentEditing= */ false,
-        /* attentionModified= */ false
+        /* attentionExpanded= */ false,
+        /* _currentAttentionSet */ new Set(),
+        /* _newAttentionSet */ new Set()
     ));
     assert.isTrue(fn(
         /* buttonLabel= */ 'Send',
@@ -1463,7 +1481,9 @@ suite('gr-reply-dialog tests', () => {
         /* includeComments= */ false,
         /* disabled= */ false,
         /* commentEditing= */ true,
-        /* attentionModified= */ false
+        /* attentionExpanded= */ false,
+        /* _currentAttentionSet */ new Set(),
+        /* _newAttentionSet */ new Set()
     ));
   });