Merge "Create getSuggestedReviewerInfoName"
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index 348ea3b..30576c8 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -74,7 +74,7 @@
import {createChangeUrl, createDiffUrl} from '../../../models/views/change';
import {userModelToken} from '../../../models/user/user-model';
import {highlightServiceToken} from '../../../services/highlight/highlight-service';
-import {waitUntil} from '../../../utils/async-util';
+import {noAwait, waitUntil} from '../../../utils/async-util';
declare global {
interface HTMLElementEventMap {
@@ -811,7 +811,7 @@
const newReply = createNewReply(replyingTo, content, unresolved);
if (userWantsToEdit) {
this.getCommentsModel().addNewDraft(newReply);
- this.editDraft();
+ noAwait(this.editDraft());
} else {
try {
this.saving = true;
diff --git a/polygerrit-ui/app/embed/diff-old/gr-diff-highlight/gr-annotation.ts b/polygerrit-ui/app/embed/diff-old/gr-diff-highlight/gr-annotation.ts
index 38bd707..5669bcf 100644
--- a/polygerrit-ui/app/embed/diff-old/gr-diff-highlight/gr-annotation.ts
+++ b/polygerrit-ui/app/embed/diff-old/gr-diff-highlight/gr-annotation.ts
@@ -19,7 +19,7 @@
*/
getLength(node: Node) {
if (node instanceof Comment) return 0;
- return this.getStringLength(node.textContent || '');
+ return GrAnnotation.getStringLength(node.textContent || '');
},
/**
@@ -65,7 +65,7 @@
const nestedNodes: Node[] = [];
for (let node of childNodes) {
- const initialNodeLength = this.getLength(node);
+ const initialNodeLength = GrAnnotation.getLength(node);
// If the current node is completely before the offset.
if (offset > 0 && initialNodeLength <= offset) {
offset -= initialNodeLength;
@@ -73,15 +73,15 @@
}
if (offset > 0) {
- node = this.splitNode(node, offset);
+ node = GrAnnotation.splitNode(node, offset);
offset = 0;
}
- if (this.getLength(node) > length) {
- this.splitNode(node, length);
+ if (GrAnnotation.getLength(node) > length) {
+ GrAnnotation.splitNode(node, length);
}
nestedNodes.push(node);
- length -= this.getLength(node);
+ length -= GrAnnotation.getLength(node);
if (!length) break;
}
@@ -116,7 +116,7 @@
let subLength;
for (const node of nodes) {
- nodeLength = this.getLength(node);
+ nodeLength = GrAnnotation.getLength(node);
// If the current node is completely before the offset.
if (nodeLength <= offset) {
@@ -128,9 +128,9 @@
subLength = Math.min(length, nodeLength - offset);
if (node instanceof Text) {
- this._annotateText(node, offset, subLength, cssClass);
+ GrAnnotation._annotateText(node, offset, subLength, cssClass);
} else if (node instanceof Element) {
- this.annotateElement(node, offset, subLength, cssClass);
+ GrAnnotation.annotateElement(node, offset, subLength, cssClass);
}
// If there is still more to annotate, then shift the indices, otherwise
@@ -172,19 +172,19 @@
firstPart?: boolean
) {
if (
- (this.getLength(node) === offset && firstPart) ||
+ (GrAnnotation.getLength(node) === offset && firstPart) ||
(offset === 0 && !firstPart)
) {
- return this.wrapInHighlight(node, cssClass);
+ return GrAnnotation.wrapInHighlight(node, cssClass);
}
if (firstPart) {
- this.splitNode(node, offset);
+ GrAnnotation.splitNode(node, offset);
// Node points to first part of the Text, second one is sibling.
} else {
// if node is Text then splitNode will return a Text
- node = this.splitNode(node, offset) as Text;
+ node = GrAnnotation.splitNode(node, offset) as Text;
}
- return this.wrapInHighlight(node, cssClass);
+ return GrAnnotation.wrapInHighlight(node, cssClass);
},
/**
@@ -193,7 +193,7 @@
*/
splitNode(element: Node, offset: number) {
if (element instanceof Text) {
- return this.splitTextNode(element, offset);
+ return GrAnnotation.splitTextNode(element, offset);
}
const tail = element.cloneNode(false);
@@ -203,13 +203,14 @@
let node = element.firstChild;
while (
node &&
- (this.getLength(node) <= offset || this.getLength(node) === 0)
+ (GrAnnotation.getLength(node) <= offset ||
+ GrAnnotation.getLength(node) === 0)
) {
- offset -= this.getLength(node);
+ offset -= GrAnnotation.getLength(node);
node = node.nextSibling;
}
- if (node && this.getLength(node) > offset) {
- tail.appendChild(this.splitNode(node, offset));
+ if (node && GrAnnotation.getLength(node) > offset) {
+ tail.appendChild(GrAnnotation.splitNode(node, offset));
}
while (node && node.nextSibling) {
tail.appendChild(node.nextSibling);
@@ -245,7 +246,7 @@
},
_annotateText(node: Text, offset: number, length: number, cssClass: string) {
- const nodeLength = this.getLength(node);
+ const nodeLength = GrAnnotation.getLength(node);
// There are four cases:
// 1) Entire node is highlighted.
@@ -255,17 +256,17 @@
if (offset === 0 && nodeLength === length) {
// Case 1.
- this.wrapInHighlight(node, cssClass);
+ GrAnnotation.wrapInHighlight(node, cssClass);
} else if (offset === 0) {
// Case 2.
- this.splitAndWrapInHighlight(node, length, cssClass, true);
+ GrAnnotation.splitAndWrapInHighlight(node, length, cssClass, true);
} else if (offset + length === nodeLength) {
// Case 3
- this.splitAndWrapInHighlight(node, offset, cssClass, false);
+ GrAnnotation.splitAndWrapInHighlight(node, offset, cssClass, false);
} else {
// Case 4
- this.splitAndWrapInHighlight(
- this.splitTextNode(node, offset),
+ GrAnnotation.splitAndWrapInHighlight(
+ GrAnnotation.splitTextNode(node, offset),
length,
cssClass,
true
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
index 38bd707..5669bcf 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
@@ -19,7 +19,7 @@
*/
getLength(node: Node) {
if (node instanceof Comment) return 0;
- return this.getStringLength(node.textContent || '');
+ return GrAnnotation.getStringLength(node.textContent || '');
},
/**
@@ -65,7 +65,7 @@
const nestedNodes: Node[] = [];
for (let node of childNodes) {
- const initialNodeLength = this.getLength(node);
+ const initialNodeLength = GrAnnotation.getLength(node);
// If the current node is completely before the offset.
if (offset > 0 && initialNodeLength <= offset) {
offset -= initialNodeLength;
@@ -73,15 +73,15 @@
}
if (offset > 0) {
- node = this.splitNode(node, offset);
+ node = GrAnnotation.splitNode(node, offset);
offset = 0;
}
- if (this.getLength(node) > length) {
- this.splitNode(node, length);
+ if (GrAnnotation.getLength(node) > length) {
+ GrAnnotation.splitNode(node, length);
}
nestedNodes.push(node);
- length -= this.getLength(node);
+ length -= GrAnnotation.getLength(node);
if (!length) break;
}
@@ -116,7 +116,7 @@
let subLength;
for (const node of nodes) {
- nodeLength = this.getLength(node);
+ nodeLength = GrAnnotation.getLength(node);
// If the current node is completely before the offset.
if (nodeLength <= offset) {
@@ -128,9 +128,9 @@
subLength = Math.min(length, nodeLength - offset);
if (node instanceof Text) {
- this._annotateText(node, offset, subLength, cssClass);
+ GrAnnotation._annotateText(node, offset, subLength, cssClass);
} else if (node instanceof Element) {
- this.annotateElement(node, offset, subLength, cssClass);
+ GrAnnotation.annotateElement(node, offset, subLength, cssClass);
}
// If there is still more to annotate, then shift the indices, otherwise
@@ -172,19 +172,19 @@
firstPart?: boolean
) {
if (
- (this.getLength(node) === offset && firstPart) ||
+ (GrAnnotation.getLength(node) === offset && firstPart) ||
(offset === 0 && !firstPart)
) {
- return this.wrapInHighlight(node, cssClass);
+ return GrAnnotation.wrapInHighlight(node, cssClass);
}
if (firstPart) {
- this.splitNode(node, offset);
+ GrAnnotation.splitNode(node, offset);
// Node points to first part of the Text, second one is sibling.
} else {
// if node is Text then splitNode will return a Text
- node = this.splitNode(node, offset) as Text;
+ node = GrAnnotation.splitNode(node, offset) as Text;
}
- return this.wrapInHighlight(node, cssClass);
+ return GrAnnotation.wrapInHighlight(node, cssClass);
},
/**
@@ -193,7 +193,7 @@
*/
splitNode(element: Node, offset: number) {
if (element instanceof Text) {
- return this.splitTextNode(element, offset);
+ return GrAnnotation.splitTextNode(element, offset);
}
const tail = element.cloneNode(false);
@@ -203,13 +203,14 @@
let node = element.firstChild;
while (
node &&
- (this.getLength(node) <= offset || this.getLength(node) === 0)
+ (GrAnnotation.getLength(node) <= offset ||
+ GrAnnotation.getLength(node) === 0)
) {
- offset -= this.getLength(node);
+ offset -= GrAnnotation.getLength(node);
node = node.nextSibling;
}
- if (node && this.getLength(node) > offset) {
- tail.appendChild(this.splitNode(node, offset));
+ if (node && GrAnnotation.getLength(node) > offset) {
+ tail.appendChild(GrAnnotation.splitNode(node, offset));
}
while (node && node.nextSibling) {
tail.appendChild(node.nextSibling);
@@ -245,7 +246,7 @@
},
_annotateText(node: Text, offset: number, length: number, cssClass: string) {
- const nodeLength = this.getLength(node);
+ const nodeLength = GrAnnotation.getLength(node);
// There are four cases:
// 1) Entire node is highlighted.
@@ -255,17 +256,17 @@
if (offset === 0 && nodeLength === length) {
// Case 1.
- this.wrapInHighlight(node, cssClass);
+ GrAnnotation.wrapInHighlight(node, cssClass);
} else if (offset === 0) {
// Case 2.
- this.splitAndWrapInHighlight(node, length, cssClass, true);
+ GrAnnotation.splitAndWrapInHighlight(node, length, cssClass, true);
} else if (offset + length === nodeLength) {
// Case 3
- this.splitAndWrapInHighlight(node, offset, cssClass, false);
+ GrAnnotation.splitAndWrapInHighlight(node, offset, cssClass, false);
} else {
// Case 4
- this.splitAndWrapInHighlight(
- this.splitTextNode(node, offset),
+ GrAnnotation.splitAndWrapInHighlight(
+ GrAnnotation.splitTextNode(node, offset),
length,
cssClass,
true
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
index 68ec76a..ba6e384 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
@@ -3084,37 +3084,48 @@
});
}
- async setInProjectLookup(changeNum: NumericChangeId, project: RepoName) {
- const lookupProject = await this._projectLookup[changeNum];
- if (lookupProject && lookupProject !== project) {
- console.warn(
- 'Change set with multiple project nums.' +
- 'One of them must be invalid.'
- );
- }
+ /**
+ * This can be called by the router, if the project can be determined from
+ * the URL. Or when handling a dashabord or a search response.
+ *
+ * Then we don't need to make a dedicated REST API call or have a fallback,
+ * if that fails.
+ */
+ setInProjectLookup(changeNum: NumericChangeId, project: RepoName) {
this._projectLookup[changeNum] = Promise.resolve(project);
}
getFromProjectLookup(
changeNum: NumericChangeId
): Promise<RepoName | undefined> {
- const project = this._projectLookup[`${changeNum}`];
- if (project) {
- return project;
- }
+ // Hopefully setInProjectLookup() has already been called. Then we don't
+ // have to make a dedicated REST API call to look up the project.
+ let projectPromise = this._projectLookup[changeNum];
+ if (projectPromise) return projectPromise;
- const onError = (response?: Response | null) => firePageError(response);
+ // Ignore errors, because we have some dedicated fallback logic, see below.
+ const onError = () => {};
+ projectPromise = this.getChange(changeNum, onError).then(change => {
+ if (change?.project) return change.project;
- const projectPromise = this.getChange(changeNum, onError).then(change => {
- if (!change || !change.project) {
- return;
+ // In the very rare case that the change index cannot provide an answer
+ // (e.g. stale index) we should check, if the router has called
+ // setInProjectLookup() in the meantime. Then we can fall back to that.
+ const currentProjectPromise = this._projectLookup[changeNum];
+ if (currentProjectPromise !== projectPromise) {
+ return currentProjectPromise;
}
- this.setInProjectLookup(changeNum, change.project);
- return change.project;
+
+ // No luck. Without knowing the project we cannot proceed at all.
+ firePageError(
+ new Response(
+ `Failed to lookup the repo for change number ${changeNum}`,
+ {status: 404}
+ )
+ );
+ return undefined;
});
-
this._projectLookup[changeNum] = projectPromise;
-
return projectPromise;
}
@@ -3304,10 +3315,6 @@
return this.getDocsBaseUrlCachedPromise;
}
- testOnly_clearDocsBaseUrlCache() {
- this.getDocsBaseUrlCachedPromise = undefined;
- }
-
getDocumentationSearches(filter: string): Promise<DocResult[] | undefined> {
filter = filter.trim();
const encodedFilter = encodeURIComponent(filter);
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl_test.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl_test.ts
index d570163..764aa98 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl_test.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl_test.ts
@@ -38,6 +38,7 @@
} from '../../constants/constants';
import {
BasePatchSetNum,
+ ChangeInfo,
ChangeMessageId,
CommentInfo,
DashboardId,
@@ -62,6 +63,7 @@
import {assert} from '@open-wc/testing';
import {AuthService} from '../gr-auth/gr-auth';
import {GrAuthMock} from '../gr-auth/gr-auth_mock';
+import {getBaseUrl} from '../../utils/url-util';
const EXPECTED_QUERY_OPTIONS = listChangesOptionsToHex(
ListChangesOption.CHANGE_ACTIONS,
@@ -356,7 +358,7 @@
assert.isTrue(fetchStub.calledOnce);
assert.equal(
fetchStub.firstCall.args[0].url,
- 'test52/accounts/?o=DETAILS&q=%22bro%22'
+ `${getBaseUrl()}/accounts/?o=DETAILS&q=%22bro%22`
);
});
@@ -365,7 +367,7 @@
assert.isTrue(fetchStub.calledOnce);
assert.equal(
fetchStub.firstCall.args[0].url,
- 'test53/accounts/?o=DETAILS&q=%22bro%22%20and%20cansee%3A341682'
+ `${getBaseUrl()}/accounts/?o=DETAILS&q=%22bro%22%20and%20cansee%3A341682`
);
});
@@ -379,8 +381,7 @@
assert.isTrue(fetchStub.calledOnce);
assert.equal(
fetchStub.firstCall.args[0].url,
- 'test54/accounts/?o=DETAILS&q=%22bro%22%20and%20' +
- 'cansee%3A341682%20and%20is%3Aactive'
+ `${getBaseUrl()}/accounts/?o=DETAILS&q=%22bro%22%20and%20cansee%3A341682%20and%20is%3Aactive`
);
});
});
@@ -1154,32 +1155,46 @@
});
test('setInProjectLookup', async () => {
- await element.setInProjectLookup(
- 555 as NumericChangeId,
- 'project' as RepoName
- );
+ element.setInProjectLookup(555 as NumericChangeId, 'project' as RepoName);
const project = await element.getFromProjectLookup(555 as NumericChangeId);
assert.deepEqual(project, 'project' as RepoName);
});
suite('getFromProjectLookup', () => {
- test('getChange succeeds, no project', async () => {
- sinon.stub(element, 'getChange').resolves(null);
- const val = await element.getFromProjectLookup(555 as NumericChangeId);
- assert.strictEqual(val, undefined);
+ const changeNum = 555 as NumericChangeId;
+ const repo = 'test-repo' as RepoName;
+
+ test('getChange fails to yield a project', async () => {
+ const promise = mockPromise<null>();
+ sinon.stub(element, 'getChange').returns(promise);
+
+ const projectLookup = element.getFromProjectLookup(changeNum);
+ promise.resolve(null);
+
+ assert.isUndefined(await projectLookup);
});
test('getChange succeeds with project', async () => {
- sinon
- .stub(element, 'getChange')
- .resolves({...createChange(), project: 'project' as RepoName});
- const projectLookup = element.getFromProjectLookup(
- 555 as NumericChangeId
- );
- const val = await projectLookup;
- assert.equal(val, 'project' as RepoName);
+ const promise = mockPromise<null | ChangeInfo>();
+ sinon.stub(element, 'getChange').returns(promise);
+
+ const projectLookup = element.getFromProjectLookup(changeNum);
+ promise.resolve({...createChange(), project: repo});
+
+ assert.equal(await projectLookup, repo);
assert.deepEqual(element._projectLookup, {'555': projectLookup});
});
+
+ test('getChange fails, but a setInProjectLookup() call is used as fallback', async () => {
+ const promise = mockPromise<null>();
+ sinon.stub(element, 'getChange').returns(promise);
+
+ const projectLookup = element.getFromProjectLookup(changeNum);
+ element.setInProjectLookup(changeNum, repo);
+ promise.resolve(null);
+
+ assert.equal(await projectLookup, repo);
+ });
});
suite('getChanges populates _projectLookup', () => {
@@ -1592,10 +1607,11 @@
test('null config', async () => {
const probePathMock = sinon.stub(element, 'probePath').resolves(true);
const docsBaseUrl = await element.getDocsBaseUrl(undefined);
- assert.isTrue(
- probePathMock.calledWith('test91/Documentation/index.html')
+ assert.equal(
+ probePathMock.lastCall.args[0],
+ `${getBaseUrl()}/Documentation/index.html`
);
- assert.equal(docsBaseUrl, 'test91/Documentation');
+ assert.equal(docsBaseUrl, `${getBaseUrl()}/Documentation`);
});
test('no doc config', async () => {
@@ -1605,10 +1621,11 @@
gerrit: createGerritInfo(),
};
const docsBaseUrl = await element.getDocsBaseUrl(config);
- assert.isTrue(
- probePathMock.calledWith('test92/Documentation/index.html')
+ assert.equal(
+ probePathMock.lastCall.args[0],
+ `${getBaseUrl()}/Documentation/index.html`
);
- assert.equal(docsBaseUrl, 'test92/Documentation');
+ assert.equal(docsBaseUrl, `${getBaseUrl()}/Documentation`);
});
test('has doc config', async () => {
@@ -1625,8 +1642,9 @@
test('no probe', async () => {
const probePathMock = sinon.stub(element, 'probePath').resolves(false);
const docsBaseUrl = await element.getDocsBaseUrl(undefined);
- assert.isTrue(
- probePathMock.calledWith('test94/Documentation/index.html')
+ assert.equal(
+ probePathMock.lastCall.args[0],
+ `${getBaseUrl()}/Documentation/index.html`
);
assert.isNotOk(docsBaseUrl);
});
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
index 24ba0e5..f1fae3b 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
@@ -375,7 +375,6 @@
): Promise<string>;
getDocsBaseUrl(config?: ServerInfo): Promise<string | null>;
- testOnly_clearDocsBaseUrlCache(): void;
createChange(
repo: RepoName,
diff --git a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
index f8e4160..bfa881e 100644
--- a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
+++ b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
@@ -321,9 +321,6 @@
}
return Promise.resolve('');
},
- testOnly_clearDocsBaseUrlCache() {
- return;
- },
getDocumentationSearches(): Promise<DocResult[] | undefined> {
return Promise.resolve([]);
},