opendev/gerrit
Code Issues Proposed changes

Change when and how attention can be modified in the reply dialog

Browse Source 
You cannot change the attention set in the reply dialog anymore
without making one of these three modifications: vote, comment,
add/remove reviewer/cc.

The 'Modify' button is disabled, iff the 'Send' button is disabled.

If the 'Modify' button is disabled, then the message is always:
'No changes to the attention set.'

Change-Id: Ib86a3db823641b2ca518bcbf688610e1e88e943c
This commit is contained in:
Ben Rohlfs
2020-10-09 10:36:29 +02:00
parent 410c8fa53e
commit 43f8e5f837
3 changed files with 40 additions and 95 deletions
Download Patch File Download Diff File Expand all files Collapse all files

52
polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
View File

@@ -899,6 +899,13 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
return isAttentionSetEnabled(config) && attentionExpanded;
}
_computeAttentionButtonTitle(sendDisabled?: boolean) {
return sendDisabled
? 'Modify the attention set by adding a comment or use the account ' +
'hovercard in the change page.'
: 'Edit attention set changes';
}
_handleAttentionClick(e: Event) {
const id = (e.target as GrAccountChip)?.account?._account_id;
if (!id) return;
@@ -1064,12 +1071,14 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
return accountIds;
}
_isNewAttentionEmpty(
_computeShowNoAttentionUpdate(
config?: ServerInfo,
currentAttentionSet?: Set<AccountId>,
newAttentionSet?: Set<AccountId>
newAttentionSet?: Set<AccountId>,
sendDisabled?: boolean
) {
return (
sendDisabled ||
this._computeNewAttentionAccounts(
config,
currentAttentionSet,
@@ -1080,10 +1089,11 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
_computeDoNotUpdateMessage(
currentAttentionSet?: Set<AccountId>,
newAttentionSet?: Set<AccountId>
newAttentionSet?: Set<AccountId>,
sendDisabled?: boolean
) {
if (!currentAttentionSet || !newAttentionSet) return '';
if (areSetsEqual(currentAttentionSet, newAttentionSet)) {
if (sendDisabled || areSetsEqual(currentAttentionSet, newAttentionSet)) {
return 'No changes to the attention set.';
}
if (containsAll(currentAttentionSet, newAttentionSet)) {
@@ -1398,10 +1408,7 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
labelsChanged?: boolean,
includeComments?: boolean,
disabled?: boolean,
commentEditing?: boolean,
attentionExpanded?: boolean,
_currentAttentionSet?: Set<AccountId>,
_newAttentionSet?: Set<AccountId>
commentEditing?: boolean
) {
if (
canBeStarted === undefined ||
@@ -1411,10 +1418,7 @@ export class GrReplyDialog extends KeyboardShortcutMixin(
labelsChanged === undefined ||
includeComments === undefined ||
disabled === undefined ||
commentEditing === undefined ||
attentionExpanded === undefined ||
_currentAttentionSet === undefined ||
_newAttentionSet === undefined
commentEditing === undefined
) {
return undefined;
}
@@ -1425,29 +1429,7 @@ 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 &&
!hasAttentionChange
);
return !hasDrafts && !text.length && !reviewersMutated && !labelsChanged;
}
_computePatchSetWarning(patchNum?: PatchSetNum, labelsChanged?: boolean) {

9
polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts
View File

@@ -376,16 +376,16 @@ export const htmlTemplate = html`
<div>
<template
is="dom-if"
if="[[_isNewAttentionEmpty(serverConfig, _currentAttentionSet, _newAttentionSet)]]"
if="[[_computeShowNoAttentionUpdate(serverConfig, _currentAttentionSet, _newAttentionSet, _sendDisabled)]]"
>
<span
>[[_computeDoNotUpdateMessage(_currentAttentionSet,
_newAttentionSet)]]</span
_newAttentionSet, _sendDisabled)]]</span
>
</template>
<template
is="dom-if"
if="[[!_isNewAttentionEmpty(serverConfig, _currentAttentionSet, _newAttentionSet)]]"
if="[[!_computeShowNoAttentionUpdate(serverConfig, _currentAttentionSet, _newAttentionSet, _sendDisabled)]]"
>
<span>Bring to attention of</span>
<template
@@ -406,12 +406,13 @@ export const htmlTemplate = html`
<gr-button
class="edit-attention-button"
on-click="_handleAttentionModify"
disabled="[[_sendDisabled]]"
link=""
position-below=""
data-label="Edit"
data-action-type="change"
data-action-key="edit"
title="Edit attention set changes"
title="[[_computeAttentionButtonTitle(_sendDisabled)]]"
role="button"
tabindex="0"
>

74
polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js
View File

@@ -999,11 +999,18 @@ suite('gr-reply-dialog tests', () => {
element._reviewers = [makeAccount(), makeAccount()];
element._ccs = [makeAccount(), makeAccount()];
element.draftCommentThreads = [];
MockInteractions.tap(
element.shadowRoot.querySelector('.edit-attention-button'));
const modifyButton =
element.shadowRoot.querySelector('.edit-attention-button');
MockInteractions.tap(modifyButton);
flush();
// "Modify" button disabled, because "Send" button is disabled.
assert.isFalse(element._attentionExpanded);
element.draft = 'a test comment';
MockInteractions.tap(modifyButton);
flush();
assert.isTrue(element._attentionExpanded);
let accountLabels = Array.from(element.shadowRoot.querySelectorAll(
'.attention-detail gr-account-label'));
assert.equal(accountLabels.length, 5);
@@ -1368,10 +1375,7 @@ suite('gr-reply-dialog tests', () => {
/* labelsChanged= */ false,
/* includeComments= */ false,
/* disabled= */ false,
/* commentEditing= */ false,
/* attentionExpanded= */ false,
/* _currentAttentionSet */ new Set(),
/* _newAttentionSet */ new Set()
/* commentEditing= */ false
));
});
@@ -1386,28 +1390,7 @@ suite('gr-reply-dialog tests', () => {
/* labelsChanged= */ false,
/* includeComments= */ false,
/* disabled= */ false,
/* commentEditing= */ false,
/* attentionExpanded= */ false,
/* _currentAttentionSet */ new Set(),
/* _newAttentionSet */ new Set()
));
});
test('_computeSendButtonDisabled_attentionExpanded true', () => {
const fn = element._computeSendButtonDisabled.bind(element);
// Mock everything false
assert.isFalse(fn(
/* canBeStarted= */ false,
/* draftCommentThreads= */ [],
/* text= */ '',
/* reviewersMutated= */ false,
/* labelsChanged= */ false,
/* includeComments= */ false,
/* disabled= */ false,
/* commentEditing= */ false,
/* attentionExpanded= */ true,
/* _currentAttentionSet */ new Set(),
/* _newAttentionSet */ new Set([1])
/* commentEditing= */ false
));
});
@@ -1422,10 +1405,7 @@ suite('gr-reply-dialog tests', () => {
/* labelsChanged= */ false,
/* includeComments= */ true,
/* disabled= */ false,
/* commentEditing= */ false,
/* attentionExpanded= */ false,
/* _currentAttentionSet */ new Set(),
/* _newAttentionSet */ new Set()
/* commentEditing= */ false
));
});
@@ -1440,10 +1420,7 @@ suite('gr-reply-dialog tests', () => {
/* labelsChanged= */ false,
/* includeComments= */ false,
/* disabled= */ false,
/* commentEditing= */ false,
/* attentionExpanded= */ false,
/* _currentAttentionSet */ new Set(),
/* _newAttentionSet */ new Set()
/* commentEditing= */ false
));
});
@@ -1458,10 +1435,7 @@ suite('gr-reply-dialog tests', () => {
/* labelsChanged= */ false,
/* includeComments= */ false,
/* disabled= */ false,
/* commentEditing= */ false,
/* attentionExpanded= */ false,
/* _currentAttentionSet */ new Set(),
/* _newAttentionSet */ new Set()
/* commentEditing= */ false
));
});
@@ -1476,10 +1450,7 @@ suite('gr-reply-dialog tests', () => {
/* labelsChanged= */ false,
/* includeComments= */ false,
/* disabled= */ false,
/* commentEditing= */ false,
/* attentionExpanded= */ false,
/* _currentAttentionSet */ new Set(),
/* _newAttentionSet */ new Set()
/* commentEditing= */ false
));
});
@@ -1494,10 +1465,7 @@ suite('gr-reply-dialog tests', () => {
/* labelsChanged= */ true,
/* includeComments= */ false,
/* disabled= */ false,
/* commentEditing= */ false,
/* attentionExpanded= */ false,
/* _currentAttentionSet */ new Set(),
/* _newAttentionSet */ new Set()
/* commentEditing= */ false
));
});
@@ -1512,10 +1480,7 @@ suite('gr-reply-dialog tests', () => {
/* labelsChanged= */ true,
/* includeComments= */ false,
/* disabled= */ true,
/* commentEditing= */ false,
/* attentionExpanded= */ false,
/* _currentAttentionSet */ new Set(),
/* _newAttentionSet */ new Set()
/* commentEditing= */ false
));
assert.isTrue(fn(
/* buttonLabel= */ 'Send',
@@ -1525,10 +1490,7 @@ suite('gr-reply-dialog tests', () => {
/* labelsChanged= */ true,
/* includeComments= */ false,
/* disabled= */ false,
/* commentEditing= */ true,
/* attentionExpanded= */ false,
/* _currentAttentionSet */ new Set(),
/* _newAttentionSet */ new Set()
/* commentEditing= */ true
));
});