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 {
