Use project from Revert and Cherrypick response
Previously after the revert was called, `waitForChangeReachable` would
be performed that does `/changes/q=` request for the change id. To wait
for change to be ready. However it only makes sense to do this request
if we don't know the project. The project is provided in RevertResponse,
so we don't need to make request against the index, which is potentially
stale. So instead in this change we use this information to send
requests to `changes/{project-id}~{change-id}`.
The requests from the change page similarly don't do "q="-based (index)
requests. There should be no danger in not waiting additional time.
Additionally the return of waitForChangeReachable used to be ignored, we
fix that in this change as well, and don't naviagate to a change if it
never became "reachable". We also show error message if that happens.
Motivation of the change: With recent changes to index in Google-hosted
Gerrit instance, it can take longer for index to get updated. This would
cause `setReviewOnRevert` calls fails, because the project can't be
looked up from the index.
Google-Bug-Id: b/297418211
Release-Notes: skip
Change-Id: I98724411fe6717ff012420e8fdcea2c804a3fd3f
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index aa569ad..0be80bb 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -1831,67 +1831,77 @@
}
// private but used in test
- handleResponse(action: UIActionInfo, response?: Response) {
+ async handleResponse(action: UIActionInfo, response?: Response) {
if (!response) {
return;
}
// response is guaranteed to be ok (due to semantics of rest-api methods)
- return this.restApiService.getResponseObject(response).then(obj => {
- switch (action.__key) {
- case ChangeActions.REVERT: {
- const revertChangeInfo: ChangeInfo = obj as unknown as ChangeInfo;
- this.waitForChangeReachable(revertChangeInfo._number)
- .then(() => this.setReviewOnRevert(revertChangeInfo._number))
- .then(() => {
- this.getNavigation().setUrl(
- createChangeUrl({change: revertChangeInfo})
- );
- });
- break;
- }
- case RevisionActions.CHERRYPICK: {
- const cherrypickChangeInfo: ChangeInfo = obj as unknown as ChangeInfo;
- this.waitForChangeReachable(cherrypickChangeInfo._number).then(() => {
- this.getNavigation().setUrl(
- createChangeUrl({change: cherrypickChangeInfo})
- );
- });
- break;
- }
- case ChangeActions.DELETE:
- if (action.__type === ActionType.CHANGE) {
- this.getNavigation().setUrl(rootUrl());
- }
- break;
- case ChangeActions.WIP:
- case ChangeActions.DELETE_EDIT:
- case ChangeActions.PUBLISH_EDIT:
- case ChangeActions.REBASE_EDIT:
- case ChangeActions.REBASE:
- case ChangeActions.SUBMIT:
- // Hide rebase dialog only if the action succeeds
- this.actionsModal?.close();
- this.hideAllDialogs();
- this.getChangeModel().navigateToChangeResetReload();
- break;
- case ChangeActions.REVERT_SUBMISSION: {
- const revertSubmistionInfo = obj as unknown as RevertSubmissionInfo;
- if (
- !revertSubmistionInfo.revert_changes ||
- !revertSubmistionInfo.revert_changes.length
- )
- return;
- /* If there is only 1 change then gerrit will automatically
- redirect to that change */
- const topic = revertSubmistionInfo.revert_changes[0].topic;
- this.getNavigation().setUrl(createSearchUrl({topic}));
- break;
- }
- default:
- this.getChangeModel().navigateToChangeResetReload();
- break;
+ const obj = await this.restApiService.getResponseObject(response);
+ switch (action.__key) {
+ case ChangeActions.REVERT: {
+ const revertChangeInfo: ChangeInfo = obj as unknown as ChangeInfo;
+ this.restApiService.setInProjectLookup(
+ revertChangeInfo._number,
+ revertChangeInfo.project
+ );
+ const reachable = await this.waitForChangeReachable(
+ revertChangeInfo._number
+ );
+ if (!reachable) return;
+ await this.setReviewOnRevert(revertChangeInfo._number);
+ this.getNavigation().setUrl(
+ createChangeUrl({change: revertChangeInfo})
+ );
+ break;
}
- });
+ case RevisionActions.CHERRYPICK: {
+ const cherrypickChangeInfo: ChangeInfo = obj as unknown as ChangeInfo;
+ this.restApiService.setInProjectLookup(
+ cherrypickChangeInfo._number,
+ cherrypickChangeInfo.project
+ );
+ const reachable = this.waitForChangeReachable(
+ cherrypickChangeInfo._number
+ );
+ if (!reachable) return;
+ this.getNavigation().setUrl(
+ createChangeUrl({change: cherrypickChangeInfo})
+ );
+ break;
+ }
+ case ChangeActions.DELETE:
+ if (action.__type === ActionType.CHANGE) {
+ this.getNavigation().setUrl(rootUrl());
+ }
+ break;
+ case ChangeActions.WIP:
+ case ChangeActions.DELETE_EDIT:
+ case ChangeActions.PUBLISH_EDIT:
+ case ChangeActions.REBASE_EDIT:
+ case ChangeActions.REBASE:
+ case ChangeActions.SUBMIT:
+ // Hide rebase dialog only if the action succeeds
+ this.actionsModal?.close();
+ this.hideAllDialogs();
+ this.getChangeModel().navigateToChangeResetReload();
+ break;
+ case ChangeActions.REVERT_SUBMISSION: {
+ const revertSubmistionInfo = obj as unknown as RevertSubmissionInfo;
+ if (
+ !revertSubmistionInfo.revert_changes ||
+ !revertSubmistionInfo.revert_changes.length
+ )
+ return;
+ /* If there is only 1 change then gerrit will automatically
+ redirect to that change */
+ const topic = revertSubmistionInfo.revert_changes[0].topic;
+ this.getNavigation().setUrl(createSearchUrl({topic}));
+ break;
+ }
+ default:
+ this.getChangeModel().navigateToChangeResetReload();
+ break;
+ }
}
// private but used in test
@@ -2194,14 +2204,15 @@
*
* private but used in test
*/
- waitForChangeReachable(changeNum: NumericChangeId) {
+ waitForChangeReachable(changeNum: NumericChangeId): Promise<boolean> {
let attemptsRemaining = AWAIT_CHANGE_ATTEMPTS;
return new Promise(resolve => {
const check = () => {
attemptsRemaining--;
- // Pass a no-op error handler to avoid the "not found" error toast.
+ // Pass a no-op error handler to avoid the "not found" error toast,
+ // unless it's the last attempt
this.restApiService
- .getChange(changeNum, () => {})
+ .getChange(changeNum, attemptsRemaining !== 0 ? () => {} : undefined)
.then(response => {
// If the response is 404, the response will be undefined.
if (response) {
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
index fd4bf73..631d946 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
@@ -2362,7 +2362,7 @@
});
} else {
numTries--;
- return Promise.resolve(null);
+ return Promise.resolve(undefined);
}
};
@@ -2459,42 +2459,18 @@
);
});
- suite('show revert submission dialog', () => {
- setup(async () => {
- element.change!.submission_id = '199' as ChangeSubmissionId;
- element.change!.current_revision = '2000' as CommitId;
- stubRestApi('getChanges').returns(
- Promise.resolve([
- {
- ...createChangeViewChange(),
- change_id: '12345678901234' as ChangeId,
- topic: 'T' as TopicName,
- subject: 'random',
- },
- {
- ...createChangeViewChange(),
- change_id: '23456' as ChangeId,
- topic: 'T' as TopicName,
- subject: 'a'.repeat(100),
- },
- ])
- );
- await element.updateComplete;
- });
- });
-
suite('single changes revert', () => {
let setUrlStub: sinon.SinonStub;
setup(() => {
+ setUrlStub = sinon.stub(testResolver(navigationToken), 'setUrl');
+ });
+
+ test('revert submission single change', async () => {
getResponseObjectStub.returns(
Promise.resolve({
revert_changes: [{change_id: 12345, topic: 'T'}],
})
);
- setUrlStub = sinon.stub(testResolver(navigationToken), 'setUrl');
- });
-
- test('revert submission single change', async () => {
await element.send(
HttpMethod.POST,
{message: 'Revert submission'},
@@ -2514,6 +2490,37 @@
assert.isTrue(setUrlStub.called);
assert.equal(setUrlStub.lastCall.args[0], '/q/topic:"T"');
});
+
+ test('revert single change', async () => {
+ getResponseObjectStub.returns(
+ Promise.resolve({
+ change_id: 12345,
+ project: 'projectId',
+ _number: 12345,
+ })
+ );
+ stubRestApi('getChange').returns(
+ Promise.resolve(createChangeViewChange())
+ );
+ await element.send(
+ HttpMethod.POST,
+ {message: 'Revert'},
+ '/revert',
+ false,
+ cleanup,
+ {} as UIActionInfo
+ );
+ await element.handleResponse(
+ {
+ __key: 'revert',
+ __type: ActionType.CHANGE,
+ label: 'l',
+ },
+ new Response()
+ );
+ assert.isTrue(setUrlStub.called);
+ assert.equal(setUrlStub.lastCall.args[0], '/c/projectId/+/12345');
+ });
});
suite('multiple changes revert', () => {
@@ -2637,6 +2644,66 @@
assert.isTrue(handleErrorStub.called);
});
});
+
+ test('revert single change change not reachable', async () => {
+ stubRestApi('getChangeDetail').returns(
+ Promise.resolve({
+ ...createChangeViewChange(),
+ // element has latest info
+ revisions: createRevisions(element.latestPatchNum as number),
+ messages: createChangeMessages(1),
+ })
+ );
+ getResponseObjectStub = stubRestApi('getResponseObject');
+ const setUrlStub = sinon.stub(
+ testResolver(navigationToken),
+ 'setUrl'
+ );
+ const setReviewOnRevertStub = sinon.stub(
+ element,
+ 'setReviewOnRevert'
+ );
+ getResponseObjectStub.returns(
+ Promise.resolve({
+ change_id: 12345,
+ project: 'projectId',
+ _number: 12345,
+ })
+ );
+ let errorFired = false;
+ // Mimics the behaviour of gr-rest-api-impl: If errFn is passed call
+ // it and return undefined, otherwise call fireNetworkError or
+ // fireServerError.
+ stubRestApi('getChange').callsFake((_, errFn) => {
+ if (errFn) {
+ errFn.call(undefined);
+ } else {
+ errorFired = true;
+ }
+ return Promise.resolve(undefined);
+ });
+
+ await element.send(
+ HttpMethod.POST,
+ {message: 'Revert'},
+ '/revert',
+ false,
+ cleanup,
+ {} as UIActionInfo
+ );
+ await element.handleResponse(
+ {
+ __key: 'revert',
+ __type: ActionType.CHANGE,
+ label: 'l',
+ },
+ new Response()
+ );
+
+ assert.isTrue(errorFired);
+ assert.isFalse(setUrlStub.called);
+ assert.isFalse(setReviewOnRevertStub.called);
+ });
});
});
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 fc4123b..37e61b2 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
@@ -166,6 +166,7 @@
let fetchPromisesCache = new FetchPromisesCache(); // Shared across instances.
let pendingRequest: {[promiseName: string]: Array<Promise<unknown>>} = {}; // Shared across instances.
let grEtagDecorator = new GrEtagDecorator(); // Shared across instances.
+// TODO: consider changing this to Map()
let projectLookup: {[changeNum: string]: Promise<RepoName | undefined>} = {}; // Shared across instances.
function suppress404s(res?: Response | null) {
@@ -3110,24 +3111,38 @@
getChange(
changeNum: ChangeId | NumericChangeId,
errFn: ErrorCallback
- ): Promise<ChangeInfo | null> {
- // Cannot use _changeBaseURL, as this function is used by _projectLookup.
- return this._restApiHelper
- .fetchJSON(
- {
- url: `/changes/?q=change:${changeNum}`,
- errFn,
- anonymizedUrl: '/changes/?q=change:*',
- },
- /* noAcceptHeader */ true
- )
- .then(res => {
- const changeInfos = res as ChangeInfo[] | undefined;
- if (!changeInfos || !changeInfos.length) {
- return null;
- }
- return changeInfos[0];
- });
+ ): Promise<ChangeInfo | undefined> {
+ if (changeNum in this._projectLookup) {
+ // _projectLookup can only store NumericChangeId, so we are sure that
+ // changeNum is NumericChangeId in this case.
+ return this._changeBaseURL(changeNum as NumericChangeId).then(url =>
+ this._restApiHelper.fetchJSON(
+ {
+ url,
+ errFn,
+ anonymizedUrl: '/changes/*~*',
+ },
+ /* noAcceptHeader */ true
+ )
+ ) as Promise<ChangeInfo | undefined>;
+ } else {
+ return this._restApiHelper
+ .fetchJSON(
+ {
+ url: `/changes/?q=change:${changeNum}`,
+ errFn,
+ anonymizedUrl: '/changes/?q=change:*',
+ },
+ /* noAcceptHeader */ true
+ )
+ .then(res => {
+ const changeInfos = res as ChangeInfo[] | undefined;
+ if (!changeInfos || !changeInfos.length) {
+ return undefined;
+ }
+ return changeInfos[0];
+ });
+ }
}
/**
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 8b7e3bd..2fd7ba1 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
@@ -1165,17 +1165,17 @@
const repo = 'test-repo' as RepoName;
test('getChange fails to yield a project', async () => {
- const promise = mockPromise<null>();
+ const promise = mockPromise<undefined>();
sinon.stub(element, 'getChange').returns(promise);
const projectLookup = element.getFromProjectLookup(changeNum);
- promise.resolve(null);
+ promise.resolve(undefined);
assert.isUndefined(await projectLookup);
});
test('getChange succeeds with project', async () => {
- const promise = mockPromise<null | ChangeInfo>();
+ const promise = mockPromise<undefined | ChangeInfo>();
sinon.stub(element, 'getChange').returns(promise);
const projectLookup = element.getFromProjectLookup(changeNum);
@@ -1186,12 +1186,12 @@
});
test('getChange fails, but a setInProjectLookup() call is used as fallback', async () => {
- const promise = mockPromise<null>();
+ const promise = mockPromise<undefined>();
sinon.stub(element, 'getChange').returns(promise);
const projectLookup = element.getFromProjectLookup(changeNum);
element.setInProjectLookup(changeNum, repo);
- promise.resolve(null);
+ promise.resolve(undefined);
assert.equal(await projectLookup, repo);
});
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 1bf5c90..3916094 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
@@ -206,11 +206,16 @@
/**
* Given a changeNum, gets the change.
+ *
+ * If the project is known for the specified changeNum uses
+ * /changes/{project}~{change} api.
+ * Otherwise, calls /changes/q={changeNum}. In this case the result can be
+ * stale as this API uses index.
*/
getChange(
changeNum: ChangeId | NumericChangeId,
errFn?: ErrorCallback
- ): Promise<ChangeInfo | null>;
+ ): Promise<ChangeInfo | undefined>;
savePreferences(
prefs: PreferencesInput
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 531697d..b6fef81 100644
--- a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
+++ b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
@@ -212,7 +212,7 @@
getCapabilities(): Promise<CapabilityInfoMap | undefined> {
return Promise.resolve({});
},
- getChange(): Promise<ChangeInfo | null> {
+ getChange(): Promise<ChangeInfo | undefined> {
throw new Error('getChange() not implemented by RestApiMock.');
},
getChangeActionURL(): Promise<string> {
