Refactor reload() method in gr-file-list Get the `loggedIn` state from the user model. And make the method async. Switching to the user model also means that in tests the default state for `loggedIn` is now `true`, so there are slightly more changes necessary than expected. Release-Notes: skip Change-Id: I3dcdd74c957dda3466713d7e4b9089fd3c1edfa5
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts index 563a89e..3044385 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -710,6 +710,13 @@ ); subscribe( this, + () => this.userModel.loggedIn$, + loggedIn => { + this.loggedIn = loggedIn; + } + ); + subscribe( + this, () => this.getChangeModel().reviewedFiles$, reviewedFiles => { this.reviewed = reviewedFiles ?? []; @@ -1394,35 +1401,19 @@ </div>`; } - reload() { - if (!this.changeNum || !this.patchRange?.patchNum) { - return Promise.resolve(); - } - const changeNum = this.changeNum; - const patchRange = this.patchRange; - + async reload() { + if (!this.changeNum || !this.patchRange?.patchNum) return; this.loading = true; - this.collapseAllDiffs(); - const promises: Promise<boolean | void>[] = []; - promises.push( - this.restApiService - .getChangeOrEditFiles(changeNum, patchRange) - .then(filesByPath => { - this.filesByPath = filesByPath; - }) + this.filesByPath = await this.restApiService.getChangeOrEditFiles( + this.changeNum, + this.patchRange ); - promises.push( - this.getLoggedIn().then(loggedIn => (this.loggedIn = loggedIn)) - ); - - return Promise.all(promises).then(() => { - this.loading = false; - this.detectChromiteButler(); - this.reporting.fileListDisplayed(); - }); + this.loading = false; + this.detectChromiteButler(); + this.reporting.fileListDisplayed(); } private async updateCleanlyMergedPaths() { @@ -1660,10 +1651,6 @@ ); } - private getLoggedIn() { - return this.restApiService.getLoggedIn(); - } - private normalizeChangeFilesResponse( response: FileNameToReviewedFileInfoMap ): NormalizedFileInfo[] {
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts index fb907b1..bed8262 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
@@ -14,6 +14,7 @@ mockPromise, query, stubRestApi, + waitUntil, } from '../../../test/test-utils'; import { BasePatchSetNum, @@ -27,6 +28,7 @@ RevisionPatchSetNum, Timestamp, UrlEncodedCommentId, + FileNameToFileInfoMap, } from '../../../types/common'; import {createCommentThreads} from '../../../utils/comment-util'; import { @@ -127,7 +129,7 @@ </div> <div class="desktop sizeBars" role="columnheader">Size</div> <div class="header-stats" role="columnheader">Delta</div> - <div aria-hidden="true" class="hideOnEdit reviewed" hidden=""></div> + <div aria-hidden="true" class="hideOnEdit reviewed"></div> <div aria-hidden="true" class="editFileControls showOnEdit"></div> <div aria-hidden="true" class="show-hide"></div> </div> @@ -206,6 +208,24 @@ <span hidden=""> +/-0 B </span> </div> </div> + <div class="hideOnEdit reviewed" role="gridcell"> + <span aria-hidden="true" class="reviewedLabel"> Reviewed </span> + <span + aria-checked="false" + aria-label="Reviewed" + class="reviewedSwitch" + role="switch" + tabindex="0" + > + <span + class="markReviewed" + tabindex="-1" + title="Mark as reviewed (shortcut: r)" + > + MARK REVIEWED + </span> + </span> + </div> <div aria-hidden="true" class="editFileControls showOnEdit" @@ -1028,7 +1048,6 @@ 'file_added_in_rev2.txt': {size: 0, size_delta: 0}, 'myfile.txt': {size: 0, size_delta: 0}, }; - element.loggedIn = true; element.changeNum = 42 as NumericChangeId; element.patchRange = { basePatchNum: 'PARENT' as BasePatchSetNum, @@ -1104,12 +1123,13 @@ ); // Click on the expand button, resulting in toggleFileExpanded being - // called and not resulting in a call to reviewFile. + // called and resulting in a call to reviewFile(). queryAndAssert<HTMLDivElement>(row, 'div.show-hide').click(); await element.updateComplete; + assert.isTrue(clickSpy.calledOnce); assert.isTrue(toggleExpandSpy.calledOnce); - assert.isFalse(reviewStub.called); + await waitUntil(() => reviewStub.calledOnce); // Click inside the diff. This should result in no additional calls to // toggleFileExpanded or reviewFile. @@ -1117,7 +1137,7 @@ await element.updateComplete; assert.isTrue(clickSpy.calledTwice); assert.isTrue(toggleExpandSpy.calledOnce); - assert.isFalse(reviewStub.called); + assert.isTrue(reviewStub.calledOnce); }); test('handleFileListClick editMode', async () => { @@ -1414,7 +1434,6 @@ }); test('renderInOrder logged in', async () => { - element.loggedIn = true; const reviewStub = sinon.stub(element, 'reviewFile'); let callCount = 0; // Have to type as any because the type is 'GrDiffHost' @@ -1438,7 +1457,6 @@ }); test('renderInOrder respects diffPrefs.manual_review', async () => { - element.loggedIn = true; element.diffPrefs = { context: 10, tab_size: 8, @@ -1481,13 +1499,14 @@ }); test('loadingChanged fired from reload in debouncer', async () => { - const reloadBlocker = mockPromise(); - stubRestApi('getChangeOrEditFiles').resolves({ - 'foo.bar': {size: 0, size_delta: 0}, - }); + const reloadBlocker = mockPromise<FileNameToFileInfoMap | undefined>(); + stubRestApi('getChangeOrEditFiles').returns( + reloadBlocker.then(() => { + return {'foo.bar': {size: 0, size_delta: 0}}; + }) + ); stubRestApi('getReviewedFiles').resolves(undefined); stubRestApi('getDiffPreferences').resolves(createDefaultDiffPrefs()); - stubRestApi('getLoggedIn').returns(reloadBlocker.then(() => false)); element.changeNum = 123 as NumericChangeId; element.patchRange = {patchNum: 12 as RevisionPatchSetNum} as PatchRange; @@ -1515,8 +1534,6 @@ }); test('loadingChanged does not set class when there are no files', () => { - const reloadBlocker = mockPromise(); - stubRestApi('getLoggedIn').returns(reloadBlocker.then(() => false)); element.changeNum = 123 as NumericChangeId; element.patchRange = {patchNum: 12 as RevisionPatchSetNum} as PatchRange; element.change = { @@ -1573,7 +1590,7 @@ test('displays cleanly merged file count', async () => { await element.reload(); - await element.updateComplete; + await waitUntil(() => !!query(element, '.cleanlyMergedText')); const message = queryAndAssert<HTMLSpanElement>( element, @@ -1594,7 +1611,7 @@ 'anotherCleanlyMergedFile.js': {size: 0, size_delta: 0}, }); await element.reload(); - await element.updateComplete; + await waitUntil(() => !!query(element, '.cleanlyMergedText')); const message = queryAndAssert( element, @@ -1605,7 +1622,7 @@ test('displays button for navigating to parent 1 base', async () => { await element.reload(); - await element.updateComplete; + await waitUntil(() => !!query(element, '.showParentButton')); queryAndAssert(element, '.showParentButton'); }); @@ -2019,7 +2036,6 @@ }, }; element.reviewed = ['/COMMIT_MSG', 'myfile.txt']; - element.loggedIn = true; element.changeNum = 42 as NumericChangeId; element.patchRange = { basePatchNum: 'PARENT' as BasePatchSetNum,