Merge "Swap remove/add delta counts"
diff --git a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts
index eef2a44..04887ad 100644
--- a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts
+++ b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.ts
@@ -74,8 +74,6 @@
@customElement('gr-admin-view')
export class GrAdminView extends LitElement {
- private account?: AccountDetailInfo;
-
@state()
view?: GerritView;
@@ -459,13 +457,18 @@
async reload() {
try {
this.reloading = true;
+ // There is async barrier inside reload function, we need to clear
+ // subsectionLinks now, because the element might render while waiting for
+ // RestApi responses breaking the invariant that this.view is part of
+ // subsectionLinks if non-empty.
+ this.subsectionLinks = [];
const promises: [Promise<AccountDetailInfo | undefined>, Promise<void>] =
[
this.restApiService.getAccount(),
this.getPluginLoader().awaitPluginsLoaded(),
];
const result = await Promise.all(promises);
- this.account = result[0];
+ const account = result[0];
let options: AdminNavLinksOption | undefined = undefined;
if (this.repoName) {
options = {repoName: this.repoName};
@@ -484,7 +487,7 @@
}
const res = await getAdminLinks(
- this.account,
+ account,
() =>
this.restApiService.getAccountCapabilities().then(capabilities => {
if (!capabilities) {
@@ -501,7 +504,6 @@
: '';
if (!res.expandedSection) {
- this.subsectionLinks = [];
return;
}
this.subsectionLinks = [res.expandedSection]
diff --git a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view_test.ts b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view_test.ts
index ee4a179..8cf4e05 100644
--- a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view_test.ts
+++ b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view_test.ts
@@ -111,10 +111,10 @@
assert.isNotOk(element.filteredLinks![0].subsection);
// Groups
- assert.isNotOk(element.filteredLinks![0].subsection);
+ assert.isNotOk(element.filteredLinks![1].subsection);
// Plugins
- assert.isNotOk(element.filteredLinks![0].subsection);
+ assert.isNotOk(element.filteredLinks![2].subsection);
});
test('filteredLinks non admin authenticated', async () => {
@@ -123,7 +123,7 @@
// Repos
assert.isNotOk(element.filteredLinks![0].subsection);
// Groups
- assert.isNotOk(element.filteredLinks![0].subsection);
+ assert.isNotOk(element.filteredLinks![1].subsection);
});
test('filteredLinks non admin unathenticated', async () => {
@@ -262,6 +262,7 @@
test('Needs reload when changing from repo to group', async () => {
element.repoName = 'Test Repo' as RepoName;
+ element.view = GerritView.REPO;
stubRestApi('getAccount').returns(
Promise.resolve({
name: 'test-user',
@@ -278,9 +279,12 @@
const groupId = '1' as GroupId;
element.view = GerritView.GROUP;
element.groupViewState = {groupId, view: GerritView.GROUP};
+ // Check for reload before update. This would normally be done as part of
+ // subscribe method that updates the view/viewState.
+ assert.isTrue(element.needsReload());
+ element.reload();
await element.updateComplete;
- assert.isTrue(element.needsReload());
assert.equal(element.groupId, groupId);
});
diff --git a/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts b/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts
index 57f9ee6..3f99416 100644
--- a/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts
@@ -117,10 +117,12 @@
return;
}
- this.restApiService.getAccountDetails(userId).then(details => {
- this._accountDetails = details ?? undefined;
- this._status = details?.status ?? '';
- });
+ this.restApiService
+ .getAccountDetails(userId, () => {})
+ .then(details => {
+ this._accountDetails = details ?? undefined;
+ this._status = details?.status ?? '';
+ });
}
_computeDetail(
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 fa44e18..80b762f 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
@@ -330,6 +330,12 @@
newAttentionSet: Set<UserId> = new Set();
@state()
+ manuallyAddedAttentionSet: Set<UserId> = new Set();
+
+ @state()
+ manuallyDeletedAttentionSet: Set<UserId> = new Set();
+
+ @state()
patchsetLevelDraftIsResolved = true;
@state()
@@ -477,12 +483,14 @@
#pluginMessage:empty {
display: none;
}
- .attention .edit-attention-button {
+ .edit-attention-button {
vertical-align: top;
--gr-button-padding: 0px 4px;
}
- .attention .edit-attention-button gr-icon {
+ .edit-attention-button gr-icon {
color: inherit;
+
+ .attentionSummary .edit-attention-button gr-icon {
/* The line-height:26px hack (see below) requires us to do this.
Normally the gr-icon would account for a proper positioning
within the standard line-height:20px context. */
@@ -999,30 +1007,9 @@
)}
`
)}
- <gr-tooltip-content
- has-tooltip
- title=${this.computeAttentionButtonTitle()}
- >
- <gr-button
- class="edit-attention-button"
- @click=${this.handleAttentionModify}
- ?disabled=${this.isSendDisabled()}
- link
- position-below
- data-label="Edit"
- data-action-type="change"
- data-action-key="edit"
- role="button"
- tabindex="0"
- >
- <div>
- <gr-icon icon="edit" filled small></gr-icon>
- <span>Modify</span>
- </div>
- </gr-button>
- </gr-tooltip-content>
</div>
<div>
+ ${this.renderModifyAttentionSetButton()}
<a
href="https://gerrit-review.googlesource.com/Documentation/user-attention-set.html"
target="_blank"
@@ -1036,6 +1023,30 @@
`;
}
+ private renderModifyAttentionSetButton() {
+ return html` <gr-button
+ class="edit-attention-button"
+ @click=${this.toggleAttentionModify}
+ ?disabled=${this.isSendDisabled()}
+ link
+ position-below
+ data-label="Edit"
+ data-action-type="change"
+ data-action-key="edit"
+ role="button"
+ tabindex="0"
+ >
+ <div>
+ <gr-icon
+ icon=${this.attentionExpanded ? 'expand_circle_up' : 'edit'}
+ filled
+ small
+ ></gr-icon>
+ <span>${this.attentionExpanded ? 'Collapse' : 'Modify'}</span>
+ </div>
+ </gr-button>`;
+ }
+
private renderAttentionDetailsSection() {
if (!this.attentionExpanded) return;
return html`
@@ -1044,8 +1055,10 @@
<div>
<span>Modify attention to</span>
</div>
+
<div></div>
<div>
+ ${this.renderModifyAttentionSetButton()}
<a
href="https://gerrit-review.googlesource.com/Documentation/user-attention-set.html"
target="_blank"
@@ -1137,7 +1150,7 @@
`
)}
${when(
- this.computeShowAttentionTip(),
+ this.computeShowAttentionTip(3),
() => html`
<div class="attentionTip">
<gr-icon icon="lightbulb"></gr-icon>
@@ -1533,8 +1546,8 @@
this.reviewers = getAccounts(ReviewerState.REVIEWER);
}
- handleAttentionModify() {
- this.attentionExpanded = true;
+ toggleAttentionModify() {
+ this.attentionExpanded = !this.attentionExpanded;
}
onAttentionExpandedChange() {
@@ -1543,13 +1556,6 @@
fire(this, 'iron-resize', {});
}
- computeAttentionButtonTitle() {
- return this.isSendDisabled()
- ? '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 targetAccount = (e.target as GrAccountChip)?.account;
if (!targetAccount) return;
@@ -1561,11 +1567,15 @@
if (this.newAttentionSet.has(id)) {
this.newAttentionSet.delete(id);
+ this.manuallyAddedAttentionSet.delete(id);
+ this.manuallyDeletedAttentionSet.add(id);
this.reporting.reportInteraction(Interaction.ATTENTION_SET_CHIP, {
action: `REMOVE${self}${role}`,
});
} else {
this.newAttentionSet.add(id);
+ this.manuallyAddedAttentionSet.add(id);
+ this.manuallyDeletedAttentionSet.delete(id);
this.reporting.reportInteraction(Interaction.ATTENTION_SET_CHIP, {
action: `ADD${self}${role}`,
});
@@ -1655,18 +1665,24 @@
.map(a => getUserId(a))
.filter(id => !!id);
this.newAttentionSet = new Set([
- ...[...newAttention].filter(id => allAccountIds.includes(id)),
+ ...this.manuallyAddedAttentionSet,
+ ...[...newAttention].filter(
+ id =>
+ allAccountIds.includes(id) &&
+ !this.manuallyDeletedAttentionSet.has(id)
+ ),
]);
-
- this.attentionExpanded = this.computeShowAttentionTip();
+ // Possibly expand if need be, never collapse as this is jarring to the user.
+ this.attentionExpanded =
+ this.attentionExpanded || this.computeShowAttentionTip(1);
}
- computeShowAttentionTip() {
+ computeShowAttentionTip(minimum: number) {
if (!this.currentAttentionSet || !this.newAttentionSet) return false;
const addedIds = [...this.newAttentionSet].filter(
id => !this.currentAttentionSet.has(id)
);
- return this.isOwner && addedIds.length > 2;
+ return this.isOwner && addedIds.length >= minimum;
}
computeCommentAccounts(threads: CommentThread[]) {
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
index 3c3c246..5766d80 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
@@ -255,30 +255,25 @@
<div class="attentionSummary">
<div>
<span> No changes to the attention set. </span>
- <gr-tooltip-content
- has-tooltip=""
- title="Modify the attention set by adding a comment or use the account hovercard in the change page."
- >
- <gr-button
- aria-disabled="true"
- disabled=""
- class="edit-attention-button"
- data-action-key="edit"
- data-action-type="change"
- data-label="Edit"
- link=""
- position-below=""
- role="button"
- tabindex="-1"
- >
- <div>
- <gr-icon icon="edit" filled small></gr-icon>
- <span>Modify</span>
- </div>
- </gr-button>
- </gr-tooltip-content>
</div>
<div>
+ <gr-button
+ aria-disabled="true"
+ disabled=""
+ class="edit-attention-button"
+ data-action-key="edit"
+ data-action-type="change"
+ data-label="Edit"
+ link=""
+ position-below=""
+ role="button"
+ tabindex="-1"
+ >
+ <div>
+ <gr-icon icon="edit" filled small></gr-icon>
+ <span>Modify</span>
+ </div>
+ </gr-button>
<a
href="https://gerrit-review.googlesource.com/Documentation/user-attention-set.html"
target="_blank"
@@ -1732,19 +1727,17 @@
element._ccs = [...element.ccs, makeAccount()];
await element.updateComplete;
- // The 'attention modified' section collapses and resets when reviewers or
- // ccs change.
- assert.isFalse(element.attentionExpanded);
-
- queryAndAssert<GrButton>(element, '.edit-attention-button').click();
- await element.updateComplete;
-
assert.isTrue(element.attentionExpanded);
accountLabels = Array.from(
queryAll(element, '.attention-detail gr-account-label')
);
assert.equal(accountLabels.length, 7);
+ // Verify that toggling the attention-set-button collapses.
+ queryAndAssert<GrButton>(element, '.edit-attention-button').click();
+ await element.updateComplete;
+ assert.isFalse(element.attentionExpanded);
+
element.reviewers.pop();
element.reviewers.pop();
element._ccs.pop();
@@ -2556,6 +2549,59 @@
});
});
+ test('manually added users are not lost when view updates.', async () => {
+ assert.sameMembers([...element.newAttentionSet], []);
+
+ element.reviewers = [
+ createAccountWithId(1),
+ createAccountWithId(2),
+ createAccountWithId(3),
+ ];
+ element.patchsetLevelDraftMessage = 'abc';
+
+ await element.updateComplete;
+ assert.sameMembers(
+ [...element.newAttentionSet],
+ [2 as AccountId, 3 as AccountId, 999 as AccountId]
+ );
+
+ const modifyButton = queryAndAssert<GrButton>(
+ element,
+ '.edit-attention-button'
+ );
+
+ modifyButton.click();
+ assert.isTrue(element.attentionExpanded);
+ await element.updateComplete;
+
+ const accountsChips = Array.from(
+ queryAll<GrAccountLabel>(element, '.attention-detail gr-account-label')
+ );
+ assert.equal(accountsChips.length, 4);
+ for (let i = 0; i < 4; ++i) {
+ if (accountsChips[i].account?._account_id === 1) {
+ accountsChips[i].click();
+ break;
+ }
+ }
+
+ await element.updateComplete;
+
+ assert.sameMembers(
+ [...element.newAttentionSet],
+ [1 as AccountId, 2 as AccountId, 3 as AccountId, 999 as AccountId]
+ );
+
+ // Doesn't get reset when message changes.
+ element.patchsetLevelDraftMessage = 'def';
+ await element.updateComplete;
+
+ assert.sameMembers(
+ [...element.newAttentionSet],
+ [1 as AccountId, 2 as AccountId, 3 as AccountId, 999 as AccountId]
+ );
+ });
+
suite('mention users', () => {
setup(async () => {
element.account = createAccountWithId(1);
@@ -2692,6 +2738,13 @@
await element.updateComplete;
assert.sameMembers([...element.newAttentionSet], [999 as AccountId]);
+
+ // Random update
+ element.patchsetLevelDraftMessage = 'abc';
+ await element.updateComplete;
+
+ assert.sameMembers([...element.newAttentionSet], [999 as AccountId]);
+ element.patchsetLevelDraftMessage = 'abc';
});
test('mention user who is already CCed', async () => {
diff --git a/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.ts b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.ts
index b6af123..3dcba9e 100644
--- a/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.ts
+++ b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.ts
@@ -187,8 +187,11 @@
}
protected override willUpdate(changedProperties: PropertyValues): void {
+ if (changedProperties.has('value') || changedProperties.has('items')) {
+ this.updateText();
+ }
if (changedProperties.has('value')) {
- this.handleValueChange();
+ fireNoBubble(this, 'value-change', {value: this.value});
}
}
@@ -307,7 +310,7 @@
}, 1);
}
- private handleValueChange() {
+ private updateText() {
if (this.value === undefined || this.items === undefined) {
return;
}
@@ -318,7 +321,6 @@
this.text = selectedObj.triggerText
? selectedObj.triggerText
: selectedObj.text;
- fireNoBubble(this, 'value-change', {value: this.value});
}
/**
