Fix loading and state of <gr-repo> The goal was to just fix the bug reported in b/266010917: This was caused by the component being rendered with undefined `repoConfig`, then handlers such as `handleDescriptionTextChanged` were called with empty values at a moment where `repoConfig` was already loaded. The immediate fix is just not render the DOM while `repoConfig` is undefined or `loading` is true. That could have been achieved with a few lines of change, but using a `loading` css class along with `display:none` is so polymer-style that we are also cleaning this up along the way. Release-Notes: skip Google-Bug-Id: b/266010917 Change-Id: Iabb30640319583e1d646dc68d508ab89044aba8b
diff --git a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts index 8d6d89a..3599224 100644 --- a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts +++ b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts
@@ -25,7 +25,7 @@ RepoState, SubmitType, } from '../../../constants/constants'; -import {hasOwnProperty} from '../../../utils/common-util'; +import {assertIsDefined, hasOwnProperty} from '../../../utils/common-util'; import {firePageError, fireTitleChange} from '../../../utils/event-util'; import {getAppContext} from '../../../services/app-context'; import {WebLinkInfo} from '../../../types/diff'; @@ -36,8 +36,9 @@ import {sharedStyles} from '../../../styles/shared-styles'; import {BindValueChangeEvent} from '../../../types/events'; import {deepClone} from '../../../utils/deep-util'; -import {LitElement, PropertyValues, css, html} from 'lit'; +import {LitElement, PropertyValues, css, html, nothing} from 'lit'; import {customElement, property, state} from 'lit/decorators.js'; +import {when} from 'lit/directives/when.js'; import {subscribe} from '../../lit/subscription-controller'; import {createSearchUrl} from '../../../models/views/search'; import {userModelToken} from '../../../models/user/user-model'; @@ -150,16 +151,6 @@ color: var(--deemphasized-text-color); content: ' *'; } - .loading, - .hide { - display: none; - } - #loading.loading { - display: block; - } - #loading:not(.loading) { - display: none; - } #options .repositorySettings { display: none; } @@ -187,49 +178,48 @@ > </div> </div> - <div id="loading" class=${this.loading ? 'loading' : ''}> - Loading... - </div> - <div id="loadedContent" class=${this.loading ? 'loading' : ''}> - ${this.renderDownloadCommands()} - <h2 - id="configurations" - class="heading-2 ${configChanged ? 'edited' : ''}" - > - Configurations - </h2> - <div id="form"> - <fieldset> - ${this.renderDescription()} ${this.renderRepoOptions()} - ${this.renderPluginConfig()} - <gr-button - ?disabled=${this.readOnly || !configChanged} - @click=${this.handleSaveRepoConfig} - >Save changes</gr-button - > - </fieldset> - <gr-endpoint-decorator name="repo-config"> - <gr-endpoint-param - name="repoName" - .value=${this.repo} - ></gr-endpoint-param> - <gr-endpoint-param - name="readOnly" - .value=${this.readOnly} - ></gr-endpoint-param> - </gr-endpoint-decorator> - </div> - </div> + ${when( + this.loading || !this.repoConfig, + () => html`<div id="loading">Loading...</div>`, + () => html`<div id="loadedContent"> + ${this.renderDownloadCommands()} + <h2 + id="configurations" + class="heading-2 ${configChanged ? 'edited' : ''}" + > + Configurations + </h2> + <div id="form"> + <fieldset> + ${this.renderDescription()} ${this.renderRepoOptions()} + ${this.renderPluginConfig()} + <gr-button + ?disabled=${this.readOnly || !configChanged} + @click=${this.handleSaveRepoConfig} + >Save changes</gr-button + > + </fieldset> + <gr-endpoint-decorator name="repo-config"> + <gr-endpoint-param + name="repoName" + .value=${this.repo} + ></gr-endpoint-param> + <gr-endpoint-param + name="readOnly" + .value=${this.readOnly} + ></gr-endpoint-param> + </gr-endpoint-decorator> + </div> + </div>` + )} </div> `; } private renderDownloadCommands() { + if (!this.schemes.length) return nothing; return html` - <div - id="downloadContent" - class=${!this.schemes || !this.schemes.length ? 'hide' : ''} - > + <div id="downloadContent"> <h2 id="download" class="heading-2">Download</h2> <fieldset> <gr-download-commands @@ -252,6 +242,7 @@ } private renderDescription() { + assertIsDefined(this.repoConfig, 'repoConfig'); return html` <h3 id="Description" class="heading-3">Description</h3> <fieldset> @@ -263,7 +254,7 @@ rows="4" monospace ?disabled=${this.readOnly} - .text=${this.repoConfig?.description ?? ''} + .text=${this.repoConfig.description ?? ''} @text-changed=${this.handleDescriptionTextChanged} ></gr-textarea> </fieldset> @@ -725,8 +716,9 @@ private renderPluginConfig() { const pluginData = this.computePluginData(); + if (!pluginData.length) return nothing; return html` <div - class="pluginConfig ${!pluginData || !pluginData.length ? 'hide' : ''}" + class="pluginConfig" @plugin-config-changed=${this.handlePluginConfigChanged} > <h3 class="heading-3">Plugins</h3> @@ -762,6 +754,12 @@ // private but used in test async loadRepo() { if (!this.repo) return Promise.resolve(); + this.repoConfig = undefined; + this.originalConfig = undefined; + this.loading = true; + this.weblinks = []; + this.schemesObj = undefined; + this.readOnly = true; const promises = []; @@ -1121,6 +1119,7 @@ private handleDescriptionTextChanged(e: BindValueChangeEvent) { if (!this.repoConfig || this.loading) return; + if (this.repoConfig.description === e.detail.value) return; this.repoConfig = { ...this.repoConfig, description: e.detail.value, @@ -1130,6 +1129,7 @@ private handleStateSelectBindValueChanged(e: BindValueChangeEvent) { if (!this.repoConfig || this.loading) return; + if (this.repoConfig.state === e.detail.value) return; this.repoConfig = { ...this.repoConfig, state: e.detail.value as RepoState, @@ -1139,6 +1139,7 @@ private handleSubmitTypeSelectBindValueChanged(e: BindValueChangeEvent) { if (!this.repoConfig || this.loading) return; + if (this.repoConfig.submit_type === e.detail.value) return; this.repoConfig = { ...this.repoConfig, submit_type: e.detail.value as SubmitType,
diff --git a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts index c013c9e..4deb99a 100644 --- a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts +++ b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts
@@ -157,14 +157,17 @@ element = await fixture(html`<gr-repo></gr-repo>`); }); - test('render', () => { + test('render', async () => { + element.repo = REPO as RepoName; + await element.loadRepo(); + await element.updateComplete; // prettier and shadowDom assert do not agree about span.title wrapping assert.shadowDom.equal( element, /* prettier-ignore */ /* HTML */ ` <div class="gr-form-styles main read-only"> <div class="info"> - <h1 class="heading-1" id="Title"></h1> + <h1 class="heading-1" id="Title">test-repo</h1> <hr /> <div> <a href=""> @@ -178,7 +181,7 @@ Browse </gr-button> </a> - <a href=""> + <a href="/q/project:test-repo"> <gr-button aria-disabled="false" link="" @@ -190,15 +193,7 @@ </a> </div> </div> - <div class="loading" id="loading">Loading...</div> - <div class="loading" id="loadedContent"> - <div class="hide" id="downloadContent"> - <h2 class="heading-2" id="download">Download</h2> - <fieldset> - <gr-download-commands id="downloadCommands"> - </gr-download-commands> - </fieldset> - </div> + <div id="loadedContent"> <h2 class="heading-2" id="configurations">Configurations</h2> <div id="form"> <fieldset> @@ -266,7 +261,7 @@ </span> </section> <section - class="repositorySettings" + class="repositorySettings showConfig" id="enableSignedPushSettings" > <span class="title"> Enable signed push </span> @@ -277,7 +272,7 @@ </span> </section> <section - class="repositorySettings" + class="repositorySettings showConfig" id="requireSignedPushSettings" > <span class="title"> Require signed push </span> @@ -379,9 +374,6 @@ </span> </section> </fieldset> - <div class="hide pluginConfig"> - <h3 class="heading-3">Plugins</h3> - </div> <gr-button aria-disabled="true" disabled="" @@ -398,7 +390,51 @@ </div> </div> </div> - ` + `, + {ignoreTags: ['option']} + ); + }); + + test('render loading', async () => { + element.repo = REPO as RepoName; + element.loading = true; + await element.updateComplete; + // prettier and shadowDom assert do not agree about span.title wrapping + assert.shadowDom.equal( + element, + /* prettier-ignore */ /* HTML */ ` + <div class="gr-form-styles main read-only"> + <div class="info"> + <h1 class="heading-1" id="Title">test-repo</h1> + <hr /> + <div> + <a href=""> + <gr-button + aria-disabled="true" + disabled="" + link="" + role="button" + tabindex="-1" + > + Browse + </gr-button> + </a> + <a href="/q/project:test-repo"> + <gr-button + aria-disabled="false" + link="" + role="button" + tabindex="0" + > + View Changes + </gr-button> + </a> + </div> + </div> + <div id="loading">Loading...</div> + </div> + `, + {ignoreTags: ['option']} ); }); @@ -451,55 +487,22 @@ assert.isTrue(requestUpdateStub.called); }); - test('loading displays before repo config is loaded', () => { - assert.isTrue( - queryAndAssert<HTMLDivElement>(element, '#loading').classList.contains( - 'loading' - ) - ); - assert.isFalse( - getComputedStyle(queryAndAssert<HTMLDivElement>(element, '#loading')) - .display === 'none' - ); - assert.isTrue( - queryAndAssert<HTMLDivElement>( - element, - '#loadedContent' - ).classList.contains('loading') - ); - assert.isTrue( - getComputedStyle( - queryAndAssert<HTMLDivElement>(element, '#loadedContent') - ).display === 'none' - ); - }); - - test('download commands visibility', async () => { - element.loading = false; - await element.updateComplete; - assert.isTrue( - queryAndAssert<HTMLDivElement>( - element, - '#downloadContent' - ).classList.contains('hide') - ); - assert.isTrue( - getComputedStyle( - queryAndAssert<HTMLDivElement>(element, '#downloadContent') - ).display === 'none' - ); + test('render download commands', async () => { + element.repo = REPO as RepoName; + await element.loadRepo(); element.schemesObj = SCHEMES; await element.updateComplete; - assert.isFalse( - queryAndAssert<HTMLDivElement>( - element, - '#downloadContent' - ).classList.contains('hide') - ); - assert.isFalse( - getComputedStyle( - queryAndAssert<HTMLDivElement>(element, '#downloadContent') - ).display === 'none' + const content = queryAndAssert<HTMLDivElement>(element, '#downloadContent'); + assert.dom.equal( + content, + /* HTML */ ` + <div id="downloadContent"> + <h2 class="heading-2" id="download">Download</h2> + <fieldset> + <gr-download-commands id="downloadCommands"></gr-download-commands> + </fieldset> + </div> + ` ); }); @@ -715,9 +718,9 @@ Promise.resolve(new Response()) ); - const button = queryAll<GrButton>(element, 'gr-button')[2]; - await element.loadRepo(); + + const button = queryAll<GrButton>(element, 'gr-button')[2]; assert.isTrue(button.hasAttribute('disabled')); assert.isFalse( queryAndAssert<HTMLHeadingElement>(