Fix gr-file-list to use change-model for patchSet Also fixes an issue in change-model with computing patchNum number. It now sets "edit" if there's a change edit that exists and if the patchNum hasn't been set in the url. Bug: Issue 16545 Release-Notes: Fix gr-file-list to use change-model for patchSet Change-Id: I933bb6eceb61e4653e1f33ae19150776ad961603
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts index 2798bb9..c3f934d 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -1544,7 +1544,6 @@ id="fileList" .change=${this.change} .changeNum=${this.changeNum} - .patchRange=${this.patchRange} .editMode=${this.getEditMode()} @files-shown-changed=${(e: CustomEvent<{length: number}>) => { this.shownFileCount = e.detail.length;
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 6eaf7ae..b9850dd 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
@@ -42,6 +42,7 @@ NumericChangeId, PARENT, PatchRange, + RevisionPatchSetNum, } from '../../../types/common'; import {DiffPreferencesInfo} from '../../../types/diff'; import {GrDiffHost} from '../../diff/gr-diff-host/gr-diff-host'; @@ -181,11 +182,21 @@ @query('#diffPreferencesDialog') diffPreferencesDialog?: GrDiffPreferencesDialog; - @property({type: Object}) - patchRange?: PatchRange; + get patchRange(): PatchRange | undefined { + if (!this.patchNum) return undefined; + return { + patchNum: this.patchNum, + basePatchNum: this.basePatchNum, + }; + } - @property({type: String}) - patchNum?: string; + // Private but used in tests. + @state() + patchNum?: RevisionPatchSetNum; + + // Private but used in tests. + @state() + basePatchNum: BasePatchSetNum = PARENT; @property({type: Number}) changeNum?: NumericChangeId; @@ -811,6 +822,16 @@ this.reviewed = reviewedFiles ?? []; } ); + subscribe( + this, + () => this.getChangeModel().patchNum$, + x => (this.patchNum = x) + ); + subscribe( + this, + () => this.getChangeModel().basePatchNum$, + x => (this.basePatchNum = x) + ); } override willUpdate(changedProperties: PropertyValues): void { @@ -1136,7 +1157,7 @@ const hasExtendedStatus = this.filesLeftBase.length > 0; // no file means "header row" if (!file) { - const psNum = this.patchRange?.patchNum; + const psNum = this.patchNum; return hasExtendedStatus ? this.renderDivWithTooltip(`${psNum}`, `Patchset ${psNum}`) : nothing; @@ -1154,8 +1175,8 @@ const status = fileIsReverted ? FileInfoStatus.REVERTED : file?.status ?? FileInfoStatus.MODIFIED; - const left = `patchset ${this.patchRange?.basePatchNum}`; - const right = `patchset ${this.patchRange?.patchNum}`; + const left = `patchset ${this.basePatchNum}`; + const right = `patchset ${this.patchNum}`; const postfix = ` between ${left} and ${right}`; return html`<gr-file-status @@ -1175,7 +1196,7 @@ ></gr-icon> `; // no path means "header row" - const psNum = this.patchRange?.basePatchNum; + const psNum = this.basePatchNum; if (!path) { return html` ${this.renderDivWithTooltip(`${psNum}`, `Patchset ${psNum}`)} ${arrow} @@ -1187,7 +1208,7 @@ const status = file.status ?? FileInfoStatus.MODIFIED; const left = 'base'; - const right = `patchset ${this.patchRange?.basePatchNum}`; + const right = `patchset ${this.basePatchNum}`; const postfix = ` between ${left} and ${right}`; return html` @@ -1664,16 +1685,16 @@ if ( this.change && this.changeNum && - this.patchRange?.patchNum && - new RevisionInfo(this.change).isMergeCommit(this.patchRange.patchNum) && - this.patchRange.basePatchNum === PARENT && - this.patchRange.patchNum !== EDIT + this.patchNum && + new RevisionInfo(this.change).isMergeCommit(this.patchNum) && + this.basePatchNum === PARENT && + this.patchNum !== EDIT ) { const allFilesByPath = await this.restApiService.getChangeOrEditFiles( this.changeNum, { basePatchNum: -1 as BasePatchSetNum, // -1 is first (target) parent - patchNum: this.patchRange.patchNum, + patchNum: this.patchNum, } ); if (!allFilesByPath) return;
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 c79be96..8fb5622 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
@@ -102,10 +102,8 @@ ignore_whitespace: 'IGNORE_NONE', }; element.numFilesShown = 200; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 2 as RevisionPatchSetNum, - }; + element.basePatchNum = PARENT; + element.patchNum = 2 as RevisionPatchSetNum; saveStub = sinon .stub(element, '_saveReviewedState') .callsFake(() => Promise.resolve()); @@ -365,7 +363,7 @@ test('renders file status column header', async () => { element.files = createFiles(1, {lines_inserted: 9}); element.filesLeftBase = createFiles(1, {lines_inserted: 9}); - element.patchRange!.basePatchNum = 1 as PatchSetNumber; + element.basePatchNum = 1 as PatchSetNumber; await element.updateComplete; const fileRows = queryAll<HTMLDivElement>(element, '.header-row'); const statusCol = queryAndAssert(fileRows?.[0], '.status'); @@ -695,22 +693,9 @@ test('comment filtering', () => { element.changeComments = createChangeComments(); - const parentTo1 = { - basePatchNum: PARENT, - patchNum: 1 as RevisionPatchSetNum, - }; - const parentTo2 = { - basePatchNum: PARENT, - patchNum: 2 as RevisionPatchSetNum, - }; - - const _1To2 = { - basePatchNum: 1 as BasePatchSetNum, - patchNum: 2 as RevisionPatchSetNum, - }; - - element.patchRange = parentTo1; + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; assert.equal( element.computeCommentsStringMobile({ __path: '/COMMIT_MSG', @@ -719,7 +704,9 @@ }), '2c' ); - element.patchRange = _1To2; + + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeCommentsStringMobile({ __path: '/COMMIT_MSG', @@ -728,7 +715,9 @@ }), '3c' ); - element.patchRange = parentTo1; + + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; assert.equal( element.computeDraftsString({ __path: 'unresolved.file', @@ -737,7 +726,9 @@ }), '1 draft' ); - element.patchRange = _1To2; + + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeDraftsString({ __path: 'unresolved.file', @@ -746,7 +737,9 @@ }), '1 draft' ); - element.patchRange = parentTo1; + + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; assert.equal( element.computeDraftsStringMobile({ __path: 'unresolved.file', @@ -755,7 +748,9 @@ }), '1d' ); - element.patchRange = _1To2; + + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeDraftsStringMobile({ __path: 'unresolved.file', @@ -764,7 +759,9 @@ }), '1d' ); - element.patchRange = parentTo1; + + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; assert.equal( element.computeCommentsStringMobile({ __path: 'myfile.txt', @@ -773,7 +770,9 @@ }), '1c' ); - element.patchRange = _1To2; + + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeCommentsStringMobile({ __path: 'myfile.txt', @@ -782,16 +781,9 @@ }), '3c' ); - element.patchRange = parentTo1; - assert.equal( - element.computeDraftsString({ - __path: 'myfile.txt', - size: 0, - size_delta: 0, - }), - '' - ); - element.patchRange = _1To2; + + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; assert.equal( element.computeDraftsString({ __path: 'myfile.txt', @@ -801,7 +793,19 @@ '' ); - element.patchRange = parentTo1; + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; + assert.equal( + element.computeDraftsString({ + __path: 'myfile.txt', + size: 0, + size_delta: 0, + }), + '' + ); + + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; assert.equal( element.computeDraftsStringMobile({ __path: 'myfile.txt', @@ -810,7 +814,9 @@ }), '' ); - element.patchRange = _1To2; + + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeDraftsStringMobile({ __path: 'myfile.txt', @@ -819,7 +825,9 @@ }), '' ); - element.patchRange = parentTo1; + + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; assert.equal( element.computeCommentsStringMobile({ __path: 'file_added_in_rev2.txt', @@ -828,7 +836,9 @@ }), '' ); - element.patchRange = _1To2; + + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeCommentsStringMobile({ __path: 'file_added_in_rev2.txt', @@ -837,7 +847,9 @@ }), '' ); - element.patchRange = parentTo1; + + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; assert.equal( element.computeDraftsString({ __path: 'file_added_in_rev2.txt', @@ -846,7 +858,9 @@ }), '' ); - element.patchRange = _1To2; + + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeDraftsString({ __path: 'file_added_in_rev2.txt', @@ -855,7 +869,9 @@ }), '' ); - element.patchRange = parentTo1; + + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; assert.equal( element.computeDraftsStringMobile({ __path: 'file_added_in_rev2.txt', @@ -864,7 +880,9 @@ }), '' ); - element.patchRange = _1To2; + + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeDraftsStringMobile({ __path: 'file_added_in_rev2.txt', @@ -873,7 +891,9 @@ }), '' ); - element.patchRange = parentTo2; + + element.basePatchNum = PARENT; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeCommentsStringMobile({ __path: '/COMMIT_MSG', @@ -882,7 +902,9 @@ }), '1c' ); - element.patchRange = _1To2; + + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeCommentsStringMobile({ __path: '/COMMIT_MSG', @@ -891,7 +913,9 @@ }), '3c' ); - element.patchRange = parentTo1; + + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; assert.equal( element.computeDraftsString({ __path: '/COMMIT_MSG', @@ -900,7 +924,9 @@ }), '2 drafts' ); - element.patchRange = _1To2; + + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeDraftsString({ __path: '/COMMIT_MSG', @@ -909,7 +935,9 @@ }), '2 drafts' ); - element.patchRange = parentTo1; + + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; assert.equal( element.computeDraftsStringMobile({ __path: '/COMMIT_MSG', @@ -918,7 +946,9 @@ }), '2d' ); - element.patchRange = _1To2; + + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeDraftsStringMobile({ __path: '/COMMIT_MSG', @@ -927,7 +957,9 @@ }), '2d' ); - element.patchRange = parentTo2; + + element.basePatchNum = PARENT; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeCommentsStringMobile({ __path: 'myfile.txt', @@ -936,7 +968,9 @@ }), '2c' ); - element.patchRange = _1To2; + + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeCommentsStringMobile({ __path: 'myfile.txt', @@ -945,7 +979,9 @@ }), '3c' ); - element.patchRange = parentTo2; + + element.basePatchNum = PARENT; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeDraftsStringMobile({ __path: 'myfile.txt', @@ -954,7 +990,9 @@ }), '' ); - element.patchRange = _1To2; + + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; assert.equal( element.computeDraftsStringMobile({ __path: 'myfile.txt', @@ -973,10 +1011,8 @@ normalize({}, 'myfile.txt'), ]; element.changeNum = 42 as NumericChangeId; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 2 as RevisionPatchSetNum, - }; + element.basePatchNum = PARENT; + element.patchNum = 2 as RevisionPatchSetNum; element.change = { _number: 42 as NumericChangeId, project: 'test-project', @@ -1197,10 +1233,8 @@ normalize({}, 'myfile.txt'), ]; element.changeNum = 42 as NumericChangeId; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 2 as RevisionPatchSetNum, - }; + element.basePatchNum = PARENT; + element.patchNum = 2 as RevisionPatchSetNum; element.fileCursor.setCursorAtIndex(0); const reviewSpy = sinon.spy(element, 'reviewFile'); @@ -1263,10 +1297,8 @@ normalize({}, 'f2.txt'), ]; element.changeNum = 42 as NumericChangeId; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 2 as RevisionPatchSetNum, - }; + element.basePatchNum = PARENT; + element.patchNum = 2 as RevisionPatchSetNum; await element.updateComplete; const clickSpy = sinon.spy(element, 'handleFileListClick'); @@ -1303,10 +1335,8 @@ normalize({}, 'f2.txt'), ]; element.changeNum = 42 as NumericChangeId; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 2 as RevisionPatchSetNum, - }; + element.basePatchNum = PARENT; + element.patchNum = 2 as RevisionPatchSetNum; element.editMode = true; await element.updateComplete; @@ -1324,10 +1354,8 @@ test('checkbox shows/hides diff inline', async () => { element.files = [normalize({}, 'myfile.txt')]; element.changeNum = 42 as NumericChangeId; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 2 as RevisionPatchSetNum, - }; + element.basePatchNum = PARENT; + element.patchNum = 2 as RevisionPatchSetNum; element.fileCursor.setCursorAtIndex(0); sinon.stub(element, 'expandedFilesChanged'); await element.updateComplete; @@ -1353,10 +1381,8 @@ test('diff mode correctly toggles the diffs', async () => { element.files = [normalize({}, 'myfile.txt')]; element.changeNum = 42 as NumericChangeId; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 2 as RevisionPatchSetNum, - }; + element.basePatchNum = PARENT; + element.patchNum = 2 as RevisionPatchSetNum; const updateDiffPrefSpy = sinon.spy(element, 'updateDiffPreferences'); element.fileCursor.setCursorAtIndex(0); await element.updateComplete; @@ -1383,10 +1409,8 @@ test('tapping row ignores links', async () => { element.files = [normalize({}, '/COMMIT_MSG')]; element.changeNum = 42 as NumericChangeId; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 2 as RevisionPatchSetNum, - }; + element.basePatchNum = PARENT; + element.patchNum = 2 as RevisionPatchSetNum; sinon.stub(element, 'expandedFilesChanged'); await element.updateComplete; const commitMsgFile = queryAll<HTMLAnchorElement>( @@ -1680,10 +1704,8 @@ }; element.changeNum = changeWithMultipleParents._number; element.change = changeWithMultipleParents; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 1 as RevisionPatchSetNum, - }; + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; await element.updateComplete; await waitEventLoop(); }); @@ -1744,10 +1766,8 @@ }); test('not shown for non-Auto Merge base parents', async () => { - element.patchRange = { - basePatchNum: 1 as BasePatchSetNum, - patchNum: 2 as RevisionPatchSetNum, - }; + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 2 as RevisionPatchSetNum; await element.updateCleanlyMergedPaths(); await element.updateComplete; @@ -1756,10 +1776,8 @@ }); test('not shown in edit mode', async () => { - element.patchRange = { - basePatchNum: 1 as BasePatchSetNum, - patchNum: EDIT, - }; + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = EDIT; await element.updateCleanlyMergedPaths(); await element.updateComplete; @@ -1776,10 +1794,8 @@ _number: 1 as NumericChangeId, project: 'gerrit' as RepoName, }; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 1 as RevisionPatchSetNum, - }; + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; const path = 'index.php'; element.editMode = false; assert.equal(element.computeDiffURL(path), '/c/gerrit/+/1/1/index.php'); @@ -1791,10 +1807,8 @@ _number: 1 as NumericChangeId, project: 'gerrit' as RepoName, }; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 1 as RevisionPatchSetNum, - }; + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; element.editMode = false; const path = '/COMMIT_MSG'; assert.equal(element.computeDiffURL(path), '/c/gerrit/+/1/1//COMMIT_MSG'); @@ -1806,10 +1820,8 @@ _number: 1 as NumericChangeId, project: 'gerrit' as RepoName, }; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 1 as RevisionPatchSetNum, - }; + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; element.editMode = true; const path = 'index.php'; assert.equal( @@ -1824,10 +1836,8 @@ _number: 1 as NumericChangeId, project: 'gerrit' as RepoName, }; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 1 as RevisionPatchSetNum, - }; + element.basePatchNum = PARENT; + element.patchNum = 1 as RevisionPatchSetNum; element.editMode = true; const path = '/COMMIT_MSG'; assert.equal( @@ -2113,10 +2123,8 @@ ]; element.reviewed = ['/COMMIT_MSG', 'myfile.txt']; element.changeNum = 42 as NumericChangeId; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 2 as RevisionPatchSetNum, - }; + element.basePatchNum = PARENT; + element.patchNum = 2 as RevisionPatchSetNum; sinon .stub(window, 'fetch') .callsFake(() => Promise.resolve(new Response()));
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts index 446822f..ad5b217 100644 --- a/polygerrit-ui/app/models/change/change-model.ts +++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -26,6 +26,7 @@ import { computeAllPatchSets, computeLatestPatchNum, + computeLatestPatchNumWithEdit, } from '../../utils/patch-set-util'; import {ParsedChangeInfo} from '../../types/types'; import {fireAlert} from '../../utils/event-util'; @@ -191,6 +192,10 @@ computeLatestPatchNum(patchsets) ); + public readonly latestPatchNumWithEdit$ = select(this.patchsets$, patchsets => + computeLatestPatchNumWithEdit(patchsets) + ); + /** * Emits the current patchset number. If the route does not define the current * patchset num, then this selector waits for the change to be defined and @@ -203,7 +208,7 @@ combineLatest([ this.viewModel.state$, this.state$, - this.latestPatchNum$, + this.latestPatchNumWithEdit$, ]).pipe( /** * If you depend on both, view model and change state, then you want to
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts index 183671f..7e16ad9 100644 --- a/polygerrit-ui/app/utils/patch-set-util.ts +++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -265,6 +265,16 @@ return latest; } +// Basically is computeLatestPatchNum but allows "edits". +export function computeLatestPatchNumWithEdit( + allPatchSets?: PatchSet[] +): RevisionPatchSetNum | undefined { + if (!allPatchSets || !allPatchSets.length) { + return undefined; + } + return allPatchSets[0].num; +} + export function computePredecessor( patchset?: PatchSetNum ): BasePatchSetNum | undefined {