Refactoring of <gr-patch-range-select>
Follow-up of the lit conversion, but mainly a preparation for the child
change, which will get rid of the ChangeComments dependency.
Change-Id: Icc81c944dadb2185556c2026d91753930ae13e4c
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
index 501f688..c819c40 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
+++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
@@ -54,6 +54,10 @@
// Maximum length for patch set descriptions.
const PATCH_DESC_MAX_LENGTH = 500;
+function getShaForPatch(patch: PatchSet) {
+ return patch.sha.substring(0, 10);
+}
+
export interface PatchRangeChangeDetail {
patchNum?: PatchSetNum;
basePatchNum?: BasePatchSetNum;
@@ -106,15 +110,16 @@
@property({type: String})
basePatchNum?: BasePatchSetNum;
+ /** Not used directly. Translated into `sortedRevisions` in willUpdate(). */
@property({type: Object})
- revisions?: RevisionInfo[];
+ revisions: (RevisionInfo | EditRevisionInfo)[] = [];
@property({type: Object})
revisionInfo?: RevisionInfoClass;
- @property({type: Array})
+ /** Internal state, derived from `revisions` in willUpdate(). */
@state()
- protected sortedRevisions?: RevisionInfo[];
+ private sortedRevisions: (RevisionInfo | EditRevisionInfo)[] = [];
private readonly reporting: ReportingService = appContext.reportingService;
@@ -152,20 +157,6 @@
];
}
- private renderWeblinks(fileLink?: GeneratedWebLink[]) {
- if (!fileLink) return;
-
- return html`<span class="filesWeblinks">
- ${fileLink.map(
- weblink => html`
- <a target="_blank" rel="noopener" href="${weblink.url}">
- ${weblink.name}
- </a>
- `
- )}</span
- > `;
- }
-
override render() {
return html`
<h3 class="assistive-tech-only">Patchset Range Selection</h3>
@@ -173,14 +164,8 @@
<gr-dropdown-list
id="basePatchDropdown"
.value="${convertToString(this.basePatchNum)}"
- .items="${this._computeBaseDropdownContent(
- this.availablePatches,
- this.patchNum,
- this.sortedRevisions,
- this.changeComments,
- this.revisionInfo
- )}"
- @value-change=${this._handlePatchChange}
+ .items="${this.computeBaseDropdownContent()}"
+ @value-change=${this.handlePatchChange}
>
</gr-dropdown-list>
</span>
@@ -190,13 +175,8 @@
<gr-dropdown-list
id="patchNumDropdown"
.value="${convertToString(this.patchNum)}"
- .items="${this._computePatchDropdownContent(
- this.availablePatches,
- this.basePatchNum,
- this.sortedRevisions,
- this.changeComments
- )}"
- @value-change=${this._handlePatchChange}
+ .items="${this.computePatchDropdownContent()}"
+ @value-change=${this.handlePatchChange}
>
</gr-dropdown-list>
${this.renderWeblinks(this.filesWeblinks?.meta_b)}
@@ -204,63 +184,54 @@
`;
}
- override updated(changedProperties: PropertyValues) {
+ private renderWeblinks(fileLinks?: GeneratedWebLink[]) {
+ if (!fileLinks) return;
+ return html`<span class="filesWeblinks">
+ ${fileLinks.map(
+ weblink => html`
+ <a target="_blank" rel="noopener" href="${weblink.url}">
+ ${weblink.name}
+ </a>
+ `
+ )}</span
+ > `;
+ }
+
+ override willUpdate(changedProperties: PropertyValues) {
if (changedProperties.has('revisions')) {
- this._updateSortedRevisions(this.revisions);
+ this.sortedRevisions = sortRevisions(Object.values(this.revisions || {}));
}
}
- _updateSortedRevisions(revisions?: RevisionInfo[]) {
- if (!revisions) return;
- this.sortedRevisions = sortRevisions(Object.values(revisions));
- }
-
- _getShaForPatch(patch: PatchSet) {
- return patch.sha.substring(0, 10);
- }
-
- _computeBaseDropdownContent(
- availablePatches?: PatchSet[],
- patchNum?: PatchSetNum,
- sortedRevisions?: (RevisionInfo | EditRevisionInfo)[],
- changeComments?: ChangeComments,
- revisionInfo?: RevisionInfoClass
- ): DropdownItem[] {
- // Polymer 2: check for undefined
+ // Private method, but visible for testing.
+ computeBaseDropdownContent(): DropdownItem[] {
if (
- availablePatches === undefined ||
- patchNum === undefined ||
- sortedRevisions === undefined ||
- changeComments === undefined ||
- revisionInfo === undefined
+ this.availablePatches === undefined ||
+ this.patchNum === undefined ||
+ this.changeComments === undefined ||
+ this.revisionInfo === undefined
) {
return [];
}
- const parentCounts = revisionInfo.getParentCountMap();
- const currentParentCount = hasOwnProperty(parentCounts, patchNum)
- ? parentCounts[patchNum as number]
+ const parentCounts = this.revisionInfo.getParentCountMap();
+ const currentParentCount = hasOwnProperty(parentCounts, this.patchNum)
+ ? parentCounts[this.patchNum as number]
: 1;
- const maxParents = revisionInfo.getMaxParents();
+ const maxParents = this.revisionInfo.getMaxParents();
const isMerge = currentParentCount > 1;
const dropdownContent: DropdownItem[] = [];
- for (const basePatch of availablePatches) {
+ for (const basePatch of this.availablePatches) {
const basePatchNum = basePatch.num;
- const entry: DropdownItem = this._createDropdownEntry(
+ const entry: DropdownItem = this.createDropdownEntry(
basePatchNum,
'Patchset ',
- sortedRevisions,
- changeComments,
- this._getShaForPatch(basePatch)
+ getShaForPatch(basePatch)
);
dropdownContent.push({
...entry,
- disabled: this._computeLeftDisabled(
- basePatch.num,
- patchNum,
- sortedRevisions
- ),
+ disabled: this.computeLeftDisabled(basePatch.num, this.patchNum),
});
}
@@ -282,91 +253,61 @@
return dropdownContent;
}
- _computeMobileText(
- patchNum: PatchSetNum,
- changeComments: ChangeComments,
- revisions: (RevisionInfo | EditRevisionInfo)[]
- ) {
+ private computeMobileText(patchNum: PatchSetNum) {
return (
`${patchNum}` +
- `${this._computePatchSetCommentsString(changeComments, patchNum)}` +
- `${this._computePatchSetDescription(revisions, patchNum, true)}`
+ `${this.computePatchSetCommentsString(patchNum)}` +
+ `${this.computePatchSetDescription(patchNum, true)}`
);
}
- _computePatchDropdownContent(
- availablePatches?: PatchSet[],
- basePatchNum?: BasePatchSetNum,
- sortedRevisions?: (RevisionInfo | EditRevisionInfo)[],
- changeComments?: ChangeComments
- ): DropdownItem[] {
- // Polymer 2: check for undefined
+ // Private method, but visible for testing.
+ computePatchDropdownContent(): DropdownItem[] {
if (
- availablePatches === undefined ||
- basePatchNum === undefined ||
- sortedRevisions === undefined ||
- changeComments === undefined
+ this.availablePatches === undefined ||
+ this.basePatchNum === undefined ||
+ this.changeComments === undefined
) {
return [];
}
const dropdownContent: DropdownItem[] = [];
- for (const patch of availablePatches) {
+ for (const patch of this.availablePatches) {
const patchNum = patch.num;
- const entry = this._createDropdownEntry(
+ const entry = this.createDropdownEntry(
patchNum,
patchNum === 'edit' ? '' : 'Patchset ',
- sortedRevisions,
- changeComments,
- this._getShaForPatch(patch)
+ getShaForPatch(patch)
);
dropdownContent.push({
...entry,
- disabled: this._computeRightDisabled(
- basePatchNum,
- patchNum,
- sortedRevisions
- ),
+ disabled: this.computeRightDisabled(this.basePatchNum, patchNum),
});
}
return dropdownContent;
}
- _computeText(
- patchNum: PatchSetNum,
- prefix: string,
- changeComments: ChangeComments,
- sha: string
- ) {
+ private computeText(patchNum: PatchSetNum, prefix: string, sha: string) {
return (
`${prefix}${patchNum}` +
- `${this._computePatchSetCommentsString(changeComments, patchNum)}` +
+ `${this.computePatchSetCommentsString(patchNum)}` +
` | ${sha}`
);
}
- _createDropdownEntry(
+ private createDropdownEntry(
patchNum: PatchSetNum,
prefix: string,
- sortedRevisions: (RevisionInfo | EditRevisionInfo)[],
- changeComments: ChangeComments,
sha: string
) {
const entry: DropdownItem = {
triggerText: `${prefix}${patchNum}`,
- text: this._computeText(patchNum, prefix, changeComments, sha),
- mobileText: this._computeMobileText(
- patchNum,
- changeComments,
- sortedRevisions
- ),
- bottomText: `${this._computePatchSetDescription(
- sortedRevisions,
- patchNum
- )}`,
+ text: this.computeText(patchNum, prefix, sha),
+ mobileText: this.computeMobileText(patchNum),
+ bottomText: `${this.computePatchSetDescription(patchNum)}`,
value: patchNum,
};
- const date = this._computePatchSetDate(sortedRevisions, patchNum);
+ const date = this.computePatchSetDate(patchNum);
if (date) {
entry.date = date;
}
@@ -378,17 +319,18 @@
* is sorted in reverse order (higher patchset nums first), invalid base
* patch nums have an index greater than the index of patchNum.
*
+ * Private method, but visible for testing.
+ *
* @param basePatchNum The possible base patch num.
* @param patchNum The current selected patch num.
*/
- _computeLeftDisabled(
+ computeLeftDisabled(
basePatchNum: PatchSetNum,
- patchNum: PatchSetNum,
- sortedRevisions: (RevisionInfo | EditRevisionInfo)[]
+ patchNum: PatchSetNum
): boolean {
return (
- findSortedIndex(basePatchNum, sortedRevisions) <=
- findSortedIndex(patchNum, sortedRevisions)
+ findSortedIndex(basePatchNum, this.sortedRevisions) <=
+ findSortedIndex(patchNum, this.sortedRevisions)
);
}
@@ -403,13 +345,14 @@
* If the current basePatchNum is a parent index, then only patches that have
* at least that many parents are valid.
*
+ * Private method, but visible for testing.
+ *
* @param basePatchNum The current selected base patch num.
* @param patchNum The possible patch num.
*/
- _computeRightDisabled(
+ computeRightDisabled(
basePatchNum: PatchSetNum,
- patchNum: PatchSetNum,
- sortedRevisions: (RevisionInfo | EditRevisionInfo)[]
+ patchNum: PatchSetNum
): boolean {
if (basePatchNum === ParentPatchSetNum) {
return false;
@@ -427,21 +370,17 @@
}
return (
- findSortedIndex(basePatchNum, sortedRevisions) <=
- findSortedIndex(patchNum, sortedRevisions)
+ findSortedIndex(basePatchNum, this.sortedRevisions) <=
+ findSortedIndex(patchNum, this.sortedRevisions)
);
}
// TODO(dhruvsri): have ported comments contribute to this count
- _computePatchSetCommentsString(
- changeComments: ChangeComments,
- patchNum: PatchSetNum
- ) {
- if (!changeComments) {
- return;
- }
+ // Private method, but visible for testing.
+ computePatchSetCommentsString(patchNum: PatchSetNum): string {
+ if (!this.changeComments) return '';
- const commentThreadCount = changeComments.computeCommentThreadCount(
+ const commentThreadCount = this.changeComments.computeCommentThreadCount(
{
patchNum,
},
@@ -449,7 +388,7 @@
);
const commentThreadString = pluralize(commentThreadCount, 'comment');
- const unresolvedCount = changeComments.computeUnresolvedNum(
+ const unresolvedCount = this.changeComments.computeUnresolvedNum(
{patchNum},
true
);
@@ -468,23 +407,19 @@
);
}
- _computePatchSetDescription(
- revisions: (RevisionInfo | EditRevisionInfo)[],
+ private computePatchSetDescription(
patchNum: PatchSetNum,
addFrontSpace?: boolean
) {
- const rev = getRevisionByPatchNum(revisions, patchNum);
+ const rev = getRevisionByPatchNum(this.sortedRevisions, patchNum);
return rev?.description
? (addFrontSpace ? ' ' : '') +
rev.description.substring(0, PATCH_DESC_MAX_LENGTH)
: '';
}
- _computePatchSetDate(
- revisions: (RevisionInfo | EditRevisionInfo)[],
- patchNum: PatchSetNum
- ): Timestamp | undefined {
- const rev = getRevisionByPatchNum(revisions, patchNum);
+ private computePatchSetDate(patchNum: PatchSetNum): Timestamp | undefined {
+ const rev = getRevisionByPatchNum(this.sortedRevisions, patchNum);
return rev ? rev.created : undefined;
}
@@ -492,7 +427,7 @@
* Catches value-change events from the patchset dropdowns and determines
* whether or not a patch change event should be fired.
*/
- _handlePatchChange(e: DropDownValueChangeEvent) {
+ private handlePatchChange(e: DropDownValueChangeEvent) {
const detail: PatchRangeChangeDetail = {
patchNum: this.patchNum,
basePatchNum: this.basePatchNum,
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts
index a47b685..342fe3a 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.ts
@@ -55,7 +55,7 @@
suite('gr-patch-range-select tests', () => {
let element: GrPatchRangeSelect;
- function getInfo(revisions: RevisionInfo[]) {
+ function getInfo(revisions: (RevisionInfo | EditRevisionInfo)[]) {
const revisionObj: Partial<RevIdToRevisionInfo> = {};
for (let i = 0; i < revisions.length; i++) {
revisionObj[i] = revisions[i];
@@ -78,97 +78,54 @@
await element.updateComplete;
});
- test('enabled/disabled options', () => {
- const patchRange = {
- basePatchNum: 'PARENT' as PatchSetNum,
- patchNum: 3 as PatchSetNum,
- };
- const sortedRevisions = [
+ test('enabled/disabled options', async () => {
+ element.revisions = [
createRevision(3) as RevisionInfo,
createEditRevision(2) as EditRevisionInfo,
createRevision(2) as RevisionInfo,
createRevision(1) as RevisionInfo,
];
+ await element.updateComplete;
+
+ const parent = 'PARENT' as PatchSetNum;
+ const edit = EditPatchSetNum;
+
for (const patchNum of [1, 2, 3]) {
assert.isFalse(
- element._computeRightDisabled(
- patchRange.basePatchNum,
- patchNum as PatchSetNum,
- sortedRevisions
- )
+ element.computeRightDisabled(parent, patchNum as PatchSetNum)
);
}
for (const basePatchNum of [1, 2]) {
- assert.isFalse(
- element._computeLeftDisabled(
- basePatchNum as PatchSetNum,
- patchRange.patchNum,
- sortedRevisions
- )
- );
+ const base = basePatchNum as PatchSetNum;
+ assert.isFalse(element.computeLeftDisabled(base, 3 as PatchSetNum));
}
assert.isTrue(
- element._computeLeftDisabled(3 as PatchSetNum, patchRange.patchNum, [])
+ element.computeLeftDisabled(3 as PatchSetNum, 3 as PatchSetNum)
);
- patchRange.basePatchNum = EditPatchSetNum;
assert.isTrue(
- element._computeLeftDisabled(
- 3 as PatchSetNum,
- patchRange.patchNum,
- sortedRevisions
- )
+ element.computeLeftDisabled(3 as PatchSetNum, 3 as PatchSetNum)
);
- assert.isTrue(
- element._computeRightDisabled(
- patchRange.basePatchNum,
- 1 as PatchSetNum,
- sortedRevisions
- )
- );
- assert.isTrue(
- element._computeRightDisabled(
- patchRange.basePatchNum,
- 2 as PatchSetNum,
- sortedRevisions
- )
- );
- assert.isFalse(
- element._computeRightDisabled(
- patchRange.basePatchNum,
- 3 as PatchSetNum,
- sortedRevisions
- )
- );
- assert.isTrue(
- element._computeRightDisabled(
- patchRange.basePatchNum,
- EditPatchSetNum,
- sortedRevisions
- )
- );
+ assert.isTrue(element.computeRightDisabled(edit, 1 as PatchSetNum));
+ assert.isTrue(element.computeRightDisabled(edit, 2 as PatchSetNum));
+ assert.isFalse(element.computeRightDisabled(edit, 3 as PatchSetNum));
+ assert.isTrue(element.computeRightDisabled(edit, edit));
});
- test('_computeBaseDropdownContent', () => {
- const availablePatches = [
+ test('computeBaseDropdownContent', async () => {
+ element.availablePatches = [
{num: 'edit', sha: '1'} as PatchSet,
{num: 3, sha: '2'} as PatchSet,
{num: 2, sha: '3'} as PatchSet,
{num: 1, sha: '4'} as PatchSet,
];
- const revisions: RevisionInfo[] = [
+ element.revisions = [
createRevision(2),
createRevision(3),
createRevision(1),
createRevision(4),
];
- element.revisionInfo = getInfo(revisions);
- const sortedRevisions = [
- createRevision(3) as RevisionInfo,
- createEditRevision(2) as EditRevisionInfo,
- createRevision(2) as RevisionInfo,
- createRevision(1) as RevisionInfo,
- ];
+ element.revisionInfo = getInfo(element.revisions);
const expectedResult: DropdownItem[] = [
{
disabled: true,
@@ -210,19 +167,14 @@
value: 'PARENT',
} as DropdownItem,
];
- assert.deepEqual(
- element._computeBaseDropdownContent(
- availablePatches,
- 1 as PatchSetNum,
- sortedRevisions,
- element.changeComments,
- element.revisionInfo
- ),
- expectedResult
- );
+ element.patchNum = 1 as PatchSetNum;
+ element.basePatchNum = 'PARENT' as BasePatchSetNum;
+ await element.updateComplete;
+
+ assert.deepEqual(element.computeBaseDropdownContent(), expectedResult);
});
- test('_computeBaseDropdownContent called when patchNum updates', async () => {
+ test('computeBaseDropdownContent called when patchNum updates', async () => {
element.revisions = [
createRevision(2),
createRevision(3),
@@ -240,7 +192,7 @@
element.basePatchNum = 'PARENT' as BasePatchSetNum;
await element.updateComplete;
- const baseDropDownStub = sinon.stub(element, '_computeBaseDropdownContent');
+ const baseDropDownStub = sinon.stub(element, 'computeBaseDropdownContent');
// Should be recomputed for each available patch
element.patchNum = 1 as PatchSetNum;
@@ -248,7 +200,7 @@
assert.equal(baseDropDownStub.callCount, 1);
});
- test('_computeBaseDropdownContent called when changeComments update', async () => {
+ test('computeBaseDropdownContent called when changeComments update', async () => {
element.revisions = [
createRevision(2),
createRevision(3),
@@ -266,14 +218,14 @@
await element.updateComplete;
// Should be recomputed for each available patch
- const baseDropDownStub = sinon.stub(element, '_computeBaseDropdownContent');
+ const baseDropDownStub = sinon.stub(element, 'computeBaseDropdownContent');
assert.equal(baseDropDownStub.callCount, 0);
element.changeComments = new ChangeComments();
await element.updateComplete;
assert.equal(baseDropDownStub.callCount, 1);
});
- test('_computePatchDropdownContent called when basePatchNum updates', async () => {
+ test('computePatchDropdownContent called when basePatchNum updates', async () => {
element.revisions = [
createRevision(2),
createRevision(3),
@@ -292,29 +244,27 @@
await element.updateComplete;
// Should be recomputed for each available patch
- const baseDropDownStub = sinon.stub(
- element,
- '_computePatchDropdownContent'
- );
+ const baseDropDownStub = sinon.stub(element, 'computePatchDropdownContent');
element.basePatchNum = 1 as BasePatchSetNum;
await element.updateComplete;
assert.equal(baseDropDownStub.callCount, 1);
});
- test('_computePatchDropdownContent', () => {
- const availablePatches: PatchSet[] = [
+ test('computePatchDropdownContent', async () => {
+ element.availablePatches = [
{num: 'edit', sha: '1'} as PatchSet,
{num: 3, sha: '2'} as PatchSet,
{num: 2, sha: '3'} as PatchSet,
{num: 1, sha: '4'} as PatchSet,
];
- const basePatchNum = 1;
- const sortedRevisions = [
+ element.basePatchNum = 1 as BasePatchSetNum;
+ element.revisions = [
createRevision(3) as RevisionInfo,
createEditRevision(2) as EditRevisionInfo,
createRevision(2, 'description') as RevisionInfo,
createRevision(1) as RevisionInfo,
];
+ await element.updateComplete;
const expectedResult: DropdownItem[] = [
{
@@ -354,15 +304,7 @@
} as DropdownItem,
];
- assert.deepEqual(
- element._computePatchDropdownContent(
- availablePatches,
- basePatchNum as BasePatchSetNum,
- sortedRevisions,
- element.changeComments
- ),
- expectedResult
- );
+ assert.deepEqual(element.computePatchDropdownContent(), expectedResult);
});
test('filesWeblinks', async () => {
@@ -391,7 +333,7 @@
);
});
- test('_computePatchSetCommentsString', () => {
+ test('computePatchSetCommentsString', () => {
// Test string with unresolved comments.
const comments: PathToCommentsInfoMap = {
foo: [
@@ -432,10 +374,7 @@
element.changeComments = new ChangeComments(comments);
assert.equal(
- element._computePatchSetCommentsString(
- element.changeComments,
- 1 as PatchSetNum
- ),
+ element.computePatchSetCommentsString(1 as PatchSetNum),
' (3 comments, 1 unresolved)'
);
@@ -443,23 +382,14 @@
delete comments['foo'];
element.changeComments = new ChangeComments(comments);
assert.equal(
- element._computePatchSetCommentsString(
- element.changeComments,
- 1 as PatchSetNum
- ),
+ element.computePatchSetCommentsString(1 as PatchSetNum),
' (2 comments)'
);
// Test string with no comments.
delete comments['bar'];
element.changeComments = new ChangeComments(comments);
- assert.equal(
- element._computePatchSetCommentsString(
- element.changeComments,
- 1 as PatchSetNum
- ),
- ''
- );
+ assert.equal(element.computePatchSetCommentsString(1 as PatchSetNum), '');
});
test('patch-range-change fires', () => {
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts
index ee4ed8b..921850a 100644
--- a/polygerrit-ui/app/utils/patch-set-util.ts
+++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -103,7 +103,7 @@
return rev;
}
}
- console.warn('no revision found');
+ if (revisions.length > 0) console.warn('no revision found');
return;
}
