From 2c44778ab9aed093f1f63fb086f986c462d9dc50 Mon Sep 17 00:00:00 2001 From: Chris Poucet Date: Thu, 17 Aug 2023 17:16:16 +0200 Subject: Ensure we don't end up in an infinite loop when updating path. 1. Don't call handleValueChange if gr-drop-down-list hasn't actually changed value Analysis: - Navigate to https://go-review.googlesource.com/c/go/+/494187/29/src/runtime/trace2buf.go - gr-diff-view eventually ends up with this.path = src/runtime/trace2buf.go. - Press Shift+m - handleNextUnreviewedFile gets called - this.setReviewed(true) gets called - this.navigateToUnreviewedFile('next') - this.naveToFile gets called which computes newPath src/runtime/trace2cpu.go - this.getChangeModel().navigateToDiff gets called wit the newPath - Before the model has time to finish updating gr-diff-view, the header gets re-rerendered due to this.setReviewed(true) - this.path gets updated based on the listener for this.getViewModel().diffPath$ - gr-dropdown-list.willUpdate is called based on the re-render of #8, it still has the old path. It's items' have changed but not its `path. - This triggers the spin-locking back and forth. Google-Bug-Id: b/296388347 Release-Notes: skip Change-Id: I88196b34ddb037809ece6952555cb82e41171ad2 (cherry picked from commit 687f1203adf46755149dbdcec0069fb6b7571dfa) --- polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 9b74e24221..465afce951 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,7 +187,7 @@ export class GrDropdownList extends LitElement { } protected override willUpdate(changedProperties: PropertyValues): void { - if (changedProperties.has('items') || changedProperties.has('value')) { + if (changedProperties.has('value')) { this.handleValueChange(); } } -- cgit v1.2.3 From cd93e0b77245e9dbc99200375f6649ec8ec130e2 Mon Sep 17 00:00:00 2001 From: Sven Selberg Date: Wed, 21 Feb 2024 13:48:07 +0100 Subject: Suppress deprecation warning for LabelType.Builder#setFunction(...) As long as we have Rest API and tests for label functions we will use this method and the compiler warnings makes it difficult to check if new changes introduces other compiler warnings. Release-Notes: skip Change-Id: I788ef20091b39d72712ecfaed287a15af5921001 --- java/com/google/gerrit/acceptance/AbstractDaemonTest.java | 1 + .../com/google/gerrit/server/restapi/project/CreateLabel.java | 1 + java/com/google/gerrit/server/restapi/project/SetLabel.java | 1 + .../gerrit/acceptance/api/change/StickyApprovalsIT.java | 1 + .../gerrit/acceptance/api/change/SubmitRequirementIT.java | 3 +++ .../com/google/gerrit/acceptance/api/change/SubmitRuleIT.java | 1 + .../gerrit/acceptance/server/project/CustomLabelIT.java | 11 +++++++++++ .../gerrit/server/project/SubmitRequirementsAdapterTest.java | 4 ++++ 8 files changed, 23 insertions(+) diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 01c4942bbe..92c3e92007 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -1543,6 +1543,7 @@ public abstract class AbstractDaemonTest { configLabel(project, label, func, ImmutableList.of(), value); } + @SuppressWarnings("deprecation") private void configLabel( Project.NameKey project, String label, diff --git a/java/com/google/gerrit/server/restapi/project/CreateLabel.java b/java/com/google/gerrit/server/restapi/project/CreateLabel.java index ad32f4f738..ad79a14fc0 100644 --- a/java/com/google/gerrit/server/restapi/project/CreateLabel.java +++ b/java/com/google/gerrit/server/restapi/project/CreateLabel.java @@ -123,6 +123,7 @@ public class CreateLabel * @throws BadRequestException if there was invalid data in the input * @throws ResourceConflictException if the label cannot be created due to a conflict */ + @SuppressWarnings("deprecation") public LabelType createLabel(ProjectConfig config, String label, LabelDefinitionInput input) throws BadRequestException, ResourceConflictException { if (config.getLabelSections().containsKey(label)) { diff --git a/java/com/google/gerrit/server/restapi/project/SetLabel.java b/java/com/google/gerrit/server/restapi/project/SetLabel.java index 10589cc9b3..6c4976d76f 100644 --- a/java/com/google/gerrit/server/restapi/project/SetLabel.java +++ b/java/com/google/gerrit/server/restapi/project/SetLabel.java @@ -115,6 +115,7 @@ public class SetLabel implements RestModifyView b.setFunction(LabelFunction.NO_BLOCK)); diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java index 242c2784ef..c2235626d0 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java @@ -1576,6 +1576,7 @@ public class SubmitRequirementIT extends AbstractDaemonTest { // Clear SRs for the project and update code-review label to be non-blocking. clearSubmitRequirements(project); + @SuppressWarnings("deprecation") LabelType cr = TestLabels.codeReview().toBuilder().setFunction(LabelFunction.NO_BLOCK).build(); try (ProjectConfigUpdate u = updateProject(project)) { @@ -1622,6 +1623,7 @@ public class SubmitRequirementIT extends AbstractDaemonTest { // Clear SRs for the project and update code-review label to be non-blocking. clearSubmitRequirements(project); + @SuppressWarnings("deprecation") LabelType cr = TestLabels.codeReview().toBuilder().setFunction(LabelFunction.NO_BLOCK).build(); try (ProjectConfigUpdate u = updateProject(project)) { @@ -2836,6 +2838,7 @@ public class SubmitRequirementIT extends AbstractDaemonTest { .setAllowOverrideInChildProjects(true) .build()); + @SuppressWarnings("deprecation") LabelType cr = TestLabels.codeReview().toBuilder().setFunction(LabelFunction.NO_BLOCK).build(); try (ProjectConfigUpdate u = updateProject(project)) { u.getConfig().upsertLabelType(cr); diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitRuleIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitRuleIT.java index af95e7eaec..e7efb625cb 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/SubmitRuleIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitRuleIT.java @@ -98,6 +98,7 @@ public class SubmitRuleIT extends AbstractDaemonTest { assertThat(recordLabels).contains(needCustomLabel); } + @SuppressWarnings("deprecation") private void setupCustomBlockingLabel() throws Exception { try (ProjectConfigUpdate u = updateProject(project)) { u.getConfig() diff --git a/javatests/com/google/gerrit/acceptance/server/project/CustomLabelIT.java b/javatests/com/google/gerrit/acceptance/server/project/CustomLabelIT.java index 576c7d08ce..7ffc0a4ee5 100644 --- a/javatests/com/google/gerrit/acceptance/server/project/CustomLabelIT.java +++ b/javatests/com/google/gerrit/acceptance/server/project/CustomLabelIT.java @@ -76,6 +76,7 @@ public class CustomLabelIT extends AbstractDaemonTest { .update(); } + @SuppressWarnings("deprecation") @Test public void customLabelNoOp_NegativeVoteNotBlock() throws Exception { saveLabelConfig(LABEL.toBuilder().setFunction(NO_OP)); @@ -91,6 +92,7 @@ public class CustomLabelIT extends AbstractDaemonTest { assertThat(q.blocking).isNull(); } + @SuppressWarnings("deprecation") @Test public void customLabelNoBlock_NegativeVoteNotBlock() throws Exception { saveLabelConfig(LABEL.toBuilder().setFunction(NO_BLOCK)); @@ -106,6 +108,7 @@ public class CustomLabelIT extends AbstractDaemonTest { assertThat(q.blocking).isNull(); } + @SuppressWarnings("deprecation") @Test public void customLabelMaxNoBlock_NegativeVoteNotBlock() throws Exception { saveLabelConfig(LABEL.toBuilder().setFunction(MAX_NO_BLOCK)); @@ -121,6 +124,7 @@ public class CustomLabelIT extends AbstractDaemonTest { assertThat(q.blocking).isNull(); } + @SuppressWarnings("deprecation") @Test public void customLabelMaxNoBlock_MaxVoteSubmittable() throws Exception { saveLabelConfig(LABEL.toBuilder().setFunction(MAX_NO_BLOCK), P.toBuilder().setFunction(NO_OP)); @@ -139,6 +143,7 @@ public class CustomLabelIT extends AbstractDaemonTest { assertThat(q.blocking).isNull(); } + @SuppressWarnings("deprecation") @Test public void customLabelAnyWithBlock_NegativeVoteBlock() throws Exception { saveLabelConfig(LABEL.toBuilder().setFunction(ANY_WITH_BLOCK)); @@ -163,6 +168,7 @@ public class CustomLabelIT extends AbstractDaemonTest { } } + @SuppressWarnings("deprecation") @Test public void customLabelAnyWithBlock_Addreviewer_ZeroVote() throws Exception { TestListener testListener = new TestListener(); @@ -190,6 +196,7 @@ public class CustomLabelIT extends AbstractDaemonTest { } } + @SuppressWarnings("deprecation") @Test public void customLabelMaxWithBlock_NegativeVoteBlock() throws Exception { saveLabelConfig(LABEL.toBuilder().setFunction(MAX_WITH_BLOCK)); @@ -205,6 +212,7 @@ public class CustomLabelIT extends AbstractDaemonTest { assertThat(q.blocking).isTrue(); } + @SuppressWarnings("deprecation") @Test public void customLabelMaxWithBlock_DeletedVoteDoesNotTriggerNegativeBlock() throws Exception { saveLabelConfig(P.toBuilder().setFunction(MAX_WITH_BLOCK)); @@ -227,6 +235,7 @@ public class CustomLabelIT extends AbstractDaemonTest { assertThat(labelInfo.blocking).isNull(); // label is not blocking the change submission } + @SuppressWarnings("deprecation") @Test public void customLabelMaxWithBlock_MaxVoteSubmittable() throws Exception { saveLabelConfig( @@ -246,6 +255,7 @@ public class CustomLabelIT extends AbstractDaemonTest { assertThat(q.blocking).isNull(); } + @SuppressWarnings("deprecation") @Test public void customLabelMaxWithBlock_MaxVoteNegativeVoteBlock() throws Exception { saveLabelConfig(LABEL.toBuilder().setFunction(MAX_WITH_BLOCK)); @@ -262,6 +272,7 @@ public class CustomLabelIT extends AbstractDaemonTest { assertThat(q.blocking).isTrue(); } + @SuppressWarnings("deprecation") @Test public void customLabel_DisallowPostSubmit() throws Exception { saveLabelConfig( diff --git a/javatests/com/google/gerrit/server/project/SubmitRequirementsAdapterTest.java b/javatests/com/google/gerrit/server/project/SubmitRequirementsAdapterTest.java index b05f3c7170..180dd9285c 100644 --- a/javatests/com/google/gerrit/server/project/SubmitRequirementsAdapterTest.java +++ b/javatests/com/google/gerrit/server/project/SubmitRequirementsAdapterTest.java @@ -40,6 +40,7 @@ public class SubmitRequirementsAdapterTest { @Before public void setup() { + @SuppressWarnings("deprecation") LabelType codeReview = LabelType.builder( "Code-Review", @@ -50,6 +51,7 @@ public class SubmitRequirementsAdapterTest { .setFunction(LabelFunction.MAX_WITH_BLOCK) .build(); + @SuppressWarnings("deprecation") LabelType verified = LabelType.builder( "Verified", @@ -60,6 +62,7 @@ public class SubmitRequirementsAdapterTest { .setFunction(LabelFunction.MAX_NO_BLOCK) .build(); + @SuppressWarnings("deprecation") LabelType codeStyle = LabelType.builder( "Code-Style", @@ -70,6 +73,7 @@ public class SubmitRequirementsAdapterTest { .setFunction(LabelFunction.ANY_WITH_BLOCK) .build(); + @SuppressWarnings("deprecation") LabelType ignoreSelfApprovalLabel = LabelType.builder( "ISA-Label", -- cgit v1.2.3 From 1c8af9bcf033ef870b0d522d572713097a126dca Mon Sep 17 00:00:00 2001 From: Kamil Musin Date: Mon, 21 Aug 2023 16:50:01 +0200 Subject: Fix gr-dropdown-list not updating on items change handleValueChanged is dependent on contents of `items` therefore it needs to be recalculated if it changes. items were being received/set later than path when loading gr-diff-view from a url, resulting in no path shown in the gr-dropdown-list We only emit an event if the new value matches one of the items. It matches the behaviour before the change. It might make sense to emit the value anyway, but that broke gr-admin-view tests, because they don't set up rest api calls. So we keep it at the old behaviour. Also, spotted some small indexing errors in admin-view tests and fixed that. Google-Bug-Id: b/296106236 Release-Notes: skip Change-Id: I6b5457f71b573c9c56c425372cb0f4ec242e10f0 (cherry picked from commit ae6aa0f26a985d7ac34b3eef5be6ef24c1bbda86) --- .../app/elements/admin/gr-admin-view/gr-admin-view_test.ts | 7 ++++--- .../app/elements/shared/gr-dropdown-list/gr-dropdown-list.ts | 12 ++++++++---- 2 files changed, 12 insertions(+), 7 deletions(-) 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 ee4a17961e..0a2e4f2458 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 @@ suite('gr-admin-view tests', () => { 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 @@ suite('gr-admin-view tests', () => { // 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 @@ suite('gr-admin-view tests', () => { 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', 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 465afce951..aac917281e 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,8 @@ export class GrDropdownList extends LitElement { } protected override willUpdate(changedProperties: PropertyValues): void { - if (changedProperties.has('value')) { - this.handleValueChange(); + if (changedProperties.has('value') || changedProperties.has('items')) { + this.handleDataChange(changedProperties.has('value')); } } @@ -307,7 +307,7 @@ export class GrDropdownList extends LitElement { }, 1); } - private handleValueChange() { + private handleDataChange(valueChanged: boolean) { if (this.value === undefined || this.items === undefined) { return; } @@ -318,7 +318,11 @@ export class GrDropdownList extends LitElement { this.text = selectedObj.triggerText ? selectedObj.triggerText : selectedObj.text; - fireNoBubble(this, 'value-change', {value: this.value}); + // Only emit the event if the value actually changed and matches one of the + // items. + if (valueChanged) { + fireNoBubble(this, 'value-change', {value: this.value}); + } } /** -- cgit v1.2.3 From befd41e506fd24f58f400dbb28566c98349d59da Mon Sep 17 00:00:00 2001 From: Kamil Musin Date: Tue, 22 Aug 2023 12:36:37 +0200 Subject: Emit value change event even when no items are matched In I6b5457f71 we kept the original logic, where the event is only emitted if value changed and one of the items matched. But that creates an issue if value and items are updated on separate iterations. 1. Value updates, no items -> so no event 2. Items update, no value change -> so no event Even though in the end both value and items satisfy the requirement, the event was never sent. Since gr-dropdown-list is a utility class it generally makes sense for it to be as simple as possible, and not try to account for logic of components using it. The issue in gr-admin-view that didn't work with this previously was that it doesn't maintain it's own invariants: It was possible for the value set on gr-dropdown-list to not be in the subsectionLinks, due to render happening in the middle of `reload()`. We fix this issue in this change. Release-Notes: skip Google-Bug-Id: b/296106236 Change-Id: I9dfe61d6f7a247842e5392976378915eec6775f2 (cherry picked from commit 13948dfbb562033992d5b5346970de755a2a574a) --- .../app/elements/admin/gr-admin-view/gr-admin-view.ts | 12 +++++++----- .../app/elements/admin/gr-admin-view/gr-admin-view_test.ts | 5 ++++- .../app/elements/shared/gr-dropdown-list/gr-dropdown-list.ts | 12 +++++------- 3 files changed, 16 insertions(+), 13 deletions(-) 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 eef2a440a2..04887adbe0 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 @@ export interface AdminSubsectionLink { @customElement('gr-admin-view') export class GrAdminView extends LitElement { - private account?: AccountDetailInfo; - @state() view?: GerritView; @@ -459,13 +457,18 @@ export class GrAdminView extends LitElement { 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, Promise] = [ 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 @@ export class GrAdminView extends LitElement { } const res = await getAdminLinks( - this.account, + account, () => this.restApiService.getAccountCapabilities().then(capabilities => { if (!capabilities) { @@ -501,7 +504,6 @@ export class GrAdminView extends LitElement { : ''; 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 0a2e4f2458..8cf4e057d5 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 @@ -279,9 +279,12 @@ suite('gr-admin-view tests', () => { 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/shared/gr-dropdown-list/gr-dropdown-list.ts b/polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.ts index aac917281e..fb71983906 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 @@ -188,7 +188,10 @@ export class GrDropdownList extends LitElement { protected override willUpdate(changedProperties: PropertyValues): void { if (changedProperties.has('value') || changedProperties.has('items')) { - this.handleDataChange(changedProperties.has('value')); + this.updateText(); + } + if (changedProperties.has('value')) { + fireNoBubble(this, 'value-change', {value: this.value}); } } @@ -307,7 +310,7 @@ export class GrDropdownList extends LitElement { }, 1); } - private handleDataChange(valueChanged: boolean) { + private updateText() { if (this.value === undefined || this.items === undefined) { return; } @@ -318,11 +321,6 @@ export class GrDropdownList extends LitElement { this.text = selectedObj.triggerText ? selectedObj.triggerText : selectedObj.text; - // Only emit the event if the value actually changed and matches one of the - // items. - if (valueChanged) { - fireNoBubble(this, 'value-change', {value: this.value}); - } } /** -- cgit v1.2.3