Merge "remove `this` in GrAnnotation methods"
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([]);
},
