Address JSON parsing errors on 204 No Content
On various requests Gerrit server returns status 204. For the most part
these are request for which no response is required, however in some
cases that's not true. For methods which sometimes return JSON and
sometimes 204, this results in JSON parsing error, which after recent
refactor is no longer supressed silently.
Looking through all calls of fetchJSON I've found 3 cases where 204 can
be returned and attempted to be parsed
- Endpoints related to edit patchset return 204, if no edit patchset
exists.
- Account Status/Name/DisplayName/Username and Set Topic, these methods
return 204 if attempting to remove value, ie. set empty.
- executeChangeAction: this is for executing arbitrary urls with change
url prefix. Some of the requests don't have a response.
In all above cases previously the logic worked, because the parsing
error would result in returning null value. This was not intended but
happened to work with the old code.
We update all the affected endpoints to inspect response status and
return corresponding correct response or parse the response if needed.
Alternatively we can return undefined from fetchJSON itself,
which would be more consistent with an old behaviour. However undefined
from fetchJSON indicates an error, but in this case the response is not
an error, which can cause confusion.
For executeChangeAction this largely undoes earlier change, that moved
parsing away from gr-change-actions. However this time we only attempt
parsing for the cases where the response is expected.
Google-Bug-Id: b/297849592
Release-Notes: skip
Change-Id: I14bb51becb3a44d4bd32b4beadae5a50ec5e67f4
diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt
index 0d7a98c..d0c4553 100644
--- a/Documentation/rest-api-accounts.txt
+++ b/Documentation/rest-api-accounts.txt
@@ -377,7 +377,7 @@
"Out Of Office"
----
-If the name was deleted the response is "`204 No Content`".
+If the status was deleted the response is "`204 No Content`".
[[get-username]]
=== Get Username
@@ -449,6 +449,8 @@
As response the new display name is returned.
+If the Display Name was deleted the response is "`204 No Content`".
+
[[get-active]]
=== Get Active
--
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow_test.ts
index e21f682..15f022b 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow_test.ts
@@ -32,7 +32,7 @@
} from '../../../test/test-utils';
import {GrButton} from '../../shared/gr-button/gr-button';
import {ProgressStatus} from '../../../constants/constants';
-import {ParsedJSON, RequestPayload} from '../../../types/common';
+import {RequestPayload} from '../../../types/common';
import {ErrorCallback} from '../../../api/rest';
const change1: ChangeInfo = {...createChange(), _number: 1 as NumericChangeId};
@@ -187,7 +187,7 @@
`Status: ${ProgressStatus.NOT_STARTED}`
);
- const executeChangeAction = mockPromise<ParsedJSON>();
+ const executeChangeAction = mockPromise<Response>();
stubRestApi('executeChangeAction').returns(executeChangeAction);
assert.isNotOk(
@@ -215,7 +215,7 @@
`Status: ${ProgressStatus.RUNNING}`
);
- executeChangeAction.resolve({} as ParsedJSON);
+ executeChangeAction.resolve(new Response());
await waitUntil(
() =>
element.progress.get(1 as NumericChangeId) === ProgressStatus.SUCCESSFUL
@@ -265,7 +265,7 @@
_payload?: RequestPayload,
errFn?: ErrorCallback
) =>
- Promise.resolve({} as ParsedJSON).then(res => {
+ Promise.resolve(new Response()).then(res => {
errFn && errFn();
return res;
})
@@ -313,7 +313,7 @@
_payload?: RequestPayload,
errFn?: ErrorCallback
) =>
- Promise.resolve({} as ParsedJSON).then(res => {
+ Promise.resolve(new Response()).then(res => {
errFn && errFn();
return res;
})
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 8c6c213..0a55e0e 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
@@ -49,7 +49,6 @@
LabelInfo,
ListChangesOption,
NumericChangeId,
- ParsedJSON,
PatchSetNumber,
RequestPayload,
RevertSubmissionInfo,
@@ -116,6 +115,7 @@
import {userModelToken} from '../../../models/user/user-model';
import {ParsedChangeInfo} from '../../../types/types';
import {configModelToken} from '../../../models/config/config-model';
+import {readJSONResponsePayload} from '../../shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
const ERR_BRANCH_EMPTY = 'The destination branch can’t be empty.';
const ERR_COMMIT_EMPTY = 'The commit message can’t be empty.';
@@ -1825,15 +1825,14 @@
}
// private but used in test
- async handleResponse(action: UIActionInfo, response?: ParsedJSON) {
- if (!response) {
+ async handleResponse(action: UIActionInfo, response: Response | undefined) {
+ if (!response?.ok) {
return;
}
- // Response status is guaranteed to be ok, because rest-api returns
- // undefined if response is non-2xx.
switch (action.__key) {
case ChangeActions.REVERT: {
- const revertChangeInfo: ChangeInfo = response as unknown as ChangeInfo;
+ const revertChangeInfo = (await readJSONResponsePayload(response))
+ .parsed as unknown as ChangeInfo;
this.restApiService.addRepoNameToCache(
revertChangeInfo._number,
revertChangeInfo.project
@@ -1849,8 +1848,8 @@
break;
}
case RevisionActions.CHERRYPICK: {
- const cherrypickChangeInfo: ChangeInfo =
- response as unknown as ChangeInfo;
+ const cherrypickChangeInfo = (await readJSONResponsePayload(response))
+ .parsed as unknown as ChangeInfo;
this.restApiService.addRepoNameToCache(
cherrypickChangeInfo._number,
cherrypickChangeInfo.project
@@ -1881,8 +1880,8 @@
this.getChangeModel().navigateToChangeResetReload();
break;
case ChangeActions.REVERT_SUBMISSION: {
- const revertSubmistionInfo =
- response as unknown as RevertSubmissionInfo;
+ const revertSubmistionInfo = (await readJSONResponsePayload(response))
+ .parsed as unknown as RevertSubmissionInfo;
if (
!revertSubmistionInfo.revert_changes ||
!revertSubmistionInfo.revert_changes.length
@@ -1941,7 +1940,7 @@
revisionAction: boolean,
cleanupFn: () => void,
action: UIActionInfo
- ): Promise<ParsedJSON | undefined> {
+ ): Promise<Response | undefined> {
const handleError: ErrorCallback = response => {
cleanupFn.call(this);
this.handleResponseError(action, response, payload);
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 ff11a06..b29a154 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
@@ -17,6 +17,7 @@
} from '../../../test/test-data-generators';
import {ChangeStatus, HttpMethod} from '../../../constants/constants';
import {
+ makePrefixedJSON,
mockPromise,
query,
queryAll,
@@ -34,7 +35,6 @@
ChangeSubmissionId,
CommitId,
NumericChangeId,
- ParsedJSON,
PatchSetNumber,
RepoName,
ReviewInput,
@@ -624,7 +624,7 @@
test('rebase change fires reload event', async () => {
await element.handleResponse(
{__key: 'rebase', __type: ActionType.CHANGE, label: 'l'},
- {} as ParsedJSON
+ new Response()
);
assert.isTrue(navigateResetStub.called);
});
@@ -2413,7 +2413,7 @@
})
);
executeChangeActionStub = stubRestApi('executeChangeAction').returns(
- Promise.resolve({} as ParsedJSON)
+ Promise.resolve(new Response())
);
});
@@ -2446,9 +2446,11 @@
});
test('revert submission single change', async () => {
- const response = {
- revert_changes: [{change_id: 12345, topic: 'T'}],
- } as unknown as ParsedJSON;
+ const response = new Response(
+ makePrefixedJSON({
+ revert_changes: [{change_id: 12345, topic: 'T'}],
+ })
+ );
executeChangeActionStub.resolves(response);
await element.send(
HttpMethod.POST,
@@ -2471,11 +2473,13 @@
});
test('revert single change', async () => {
- const response = {
- change_id: 12345,
- project: 'projectId',
- _number: 12345,
- } as unknown as ParsedJSON;
+ const response = new Response(
+ makePrefixedJSON({
+ change_id: 12345,
+ project: 'projectId',
+ _number: 12345,
+ })
+ );
executeChangeActionStub.resolves(response);
stubRestApi('getChange').returns(
Promise.resolve(createChangeViewChange())
@@ -2504,14 +2508,16 @@
suite('multiple changes revert', () => {
let showActionDialogStub: sinon.SinonStub;
let setUrlStub: sinon.SinonStub;
- let response: ParsedJSON;
+ let response: Response;
setup(() => {
- response = {
- revert_changes: [
- {change_id: 12345, topic: 'T'},
- {change_id: 23456, topic: 'T'},
- ],
- } as unknown as ParsedJSON;
+ response = new Response(
+ makePrefixedJSON({
+ revert_changes: [
+ {change_id: 12345, topic: 'T'},
+ {change_id: 23456, topic: 'T'},
+ ],
+ })
+ );
executeChangeActionStub.resolves(response);
showActionDialogStub = sinon.stub(element, 'showActionDialog');
setUrlStub = sinon.stub(testResolver(navigationToken), 'setUrl');
@@ -2609,7 +2615,7 @@
(_num, _method, _patchNum, _endpoint, _payload, onErr) => {
// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
onErr!();
- return Promise.resolve({} as ParsedJSON);
+ return Promise.resolve(new Response());
}
);
const handleErrorStub = sinon.stub(element, 'handleResponseError');
@@ -2648,11 +2654,13 @@
element,
'setReviewOnRevert'
);
- const response = {
- change_id: 12345,
- project: 'projectId',
- _number: 12345,
- } as unknown as ParsedJSON;
+ const response = new Response(
+ makePrefixedJSON({
+ 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
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
index 2dcf4f4..5d2c6a4 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
@@ -631,9 +631,8 @@
payload,
handleError
)
- .then(cherrypickedChange => {
- // Rest api returns undefined on non-2xx response status
- if (cherrypickedChange === undefined) {
+ .then(response => {
+ if (!response.ok) {
return;
}
this.updateStatus(change, {status: ProgressStatus.SUCCESSFUL});
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.ts
index 69bbb4e..4655c71 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.ts
@@ -30,7 +30,6 @@
import {ProgressStatus} from '../../../constants/constants';
import {fixture, html, assert} from '@open-wc/testing';
import {GrDropdownList} from '../../shared/gr-dropdown-list/gr-dropdown-list';
-import {ParsedJSON} from '../../../types/common';
const CHERRY_PICK_TYPES = {
SINGLE_CHANGE: 1,
@@ -208,7 +207,7 @@
await element.updateComplete;
const executeChangeActionStub = stubRestApi(
'executeChangeAction'
- ).resolves(createChange() as unknown as ParsedJSON);
+ ).resolves(new Response());
queryAndAssert<GrDialog>(element, 'gr-dialog').confirmButton!.click();
await element.updateComplete;
const args = executeChangeActionStub.args[0];
@@ -229,7 +228,7 @@
await element.updateComplete;
const executeChangeActionStub = stubRestApi(
'executeChangeAction'
- ).resolves(createChange() as unknown as ParsedJSON);
+ ).resolves(new Response());
const checkboxes = queryAll<HTMLInputElement>(
element,
'input[type="checkbox"]'
@@ -248,7 +247,7 @@
await element.updateComplete;
const executeChangeActionStub = stubRestApi(
'executeChangeAction'
- ).resolves(createChange() as unknown as ParsedJSON);
+ ).resolves(new Response());
const checkboxes = queryAll<HTMLInputElement>(
element,
'input[type="checkbox"]'
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper_test.ts b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper_test.ts
index 0b29020..0f94a4b 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper_test.ts
@@ -15,6 +15,7 @@
import {
addListenerForTest,
assertFails,
+ makePrefixedJSON,
waitEventLoop,
} from '../../../../test/test-utils';
import {FakeScheduler} from '../../../../services/scheduler/fake-scheduler';
@@ -499,7 +500,7 @@
suite('reading responses', () => {
test('readResponsePayload', async () => {
const mockObject = {foo: 'bar', baz: 'foo'} as unknown as ParsedJSON;
- const serial = JSON_PREFIX + JSON.stringify(mockObject);
+ const serial = makePrefixedJSON(mockObject);
const response = new Response(serial);
const payload = await readJSONResponsePayload(response);
assert.deepEqual(payload.parsed, mockObject);
@@ -512,6 +513,15 @@
const result = parsePrefixedJSON(serial);
assert.deepEqual(result, obj);
});
+
+ test('parsing error', async () => {
+ const response = new Response('[');
+ const err: Error = await assertFails(readJSONResponsePayload(response));
+ assert.equal(
+ err.message,
+ 'Response payload is not prefixed json. Payload: ['
+ );
+ });
});
test('logCall only reports requests with anonymized URLs', async () => {
diff --git a/polygerrit-ui/app/models/bulk-actions/bulk-actions-model.ts b/polygerrit-ui/app/models/bulk-actions/bulk-actions-model.ts
index ed55d694..a070944 100644
--- a/polygerrit-ui/app/models/bulk-actions/bulk-actions-model.ts
+++ b/polygerrit-ui/app/models/bulk-actions/bulk-actions-model.ts
@@ -128,14 +128,14 @@
reason?: string,
// errorFn is needed to avoid showing an error dialog
errFn?: (changeNum: NumericChangeId) => void
- ): Promise<ChangeInfo | undefined>[] {
+ ): Promise<Response>[] {
const current = this.getState();
return current.selectedChangeNums.map(changeNum => {
if (!current.allChanges.get(changeNum))
throw new Error('invalid change id');
const change = current.allChanges.get(changeNum)!;
if (change.status === ChangeStatus.ABANDONED) {
- return Promise.resolve(change);
+ return Promise.resolve(new Response());
}
return this.restApiService.executeChangeAction(
getChangeNumber(change),
@@ -144,7 +144,7 @@
undefined,
{message: reason ?? ''},
() => errFn && errFn(getChangeNumber(change))
- ) as Promise<ChangeInfo | undefined>;
+ );
});
}
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 2ada7b3..58402db 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
@@ -739,7 +739,7 @@
}) as Promise<AccountExternalIdInfo[] | undefined>;
}
- deleteAccount() {
+ deleteAccount(): Promise<Response> {
return this._restApiHelper.fetch({
fetchOptions: {
method: HttpMethod.DELETE,
@@ -750,15 +750,16 @@
});
}
- deleteAccountIdentity(id: string[]) {
- return this._restApiHelper.fetchJSON({
+ deleteAccountIdentity(id: string[]): Promise<Response> {
+ return this._restApiHelper.fetch({
fetchOptions: getFetchOptions({
method: HttpMethod.POST,
body: id,
}),
url: '/accounts/self/external.ids:delete',
reportUrlAsIs: true,
- }) as Promise<unknown>;
+ reportServerError: true,
+ });
}
getAccountDetails(
@@ -861,66 +862,90 @@
}
}
- setAccountName(name: string): Promise<void> {
- return this._restApiHelper
- .fetchJSON({
- fetchOptions: getFetchOptions({
- method: HttpMethod.PUT,
- body: {name},
- }),
- url: '/accounts/self/name',
- reportUrlAsIs: true,
- })
- .then(newName =>
- this._updateCachedAccount({name: newName as unknown as string})
- );
+ async setAccountName(name: string): Promise<void> {
+ const response = await this._restApiHelper.fetch({
+ fetchOptions: getFetchOptions({
+ method: HttpMethod.PUT,
+ body: {name},
+ }),
+ url: '/accounts/self/name',
+ reportUrlAsIs: true,
+ reportServerError: true,
+ });
+ if (!response.ok) {
+ return;
+ }
+ let newName = undefined;
+ // If the name was deleted server returns 204
+ if (response.status !== 204) {
+ newName = (await readJSONResponsePayload(response))
+ .parsed as unknown as string;
+ }
+ this._updateCachedAccount({name: newName});
}
- setAccountUsername(username: string): Promise<void> {
- return this._restApiHelper
- .fetchJSON({
- fetchOptions: getFetchOptions({
- method: HttpMethod.PUT,
- body: {username},
- }),
- url: '/accounts/self/username',
- reportUrlAsIs: true,
- })
- .then(newName =>
- this._updateCachedAccount({username: newName as unknown as string})
- );
+ async setAccountUsername(username: string): Promise<void> {
+ const response = await this._restApiHelper.fetch({
+ fetchOptions: getFetchOptions({
+ method: HttpMethod.PUT,
+ body: {username},
+ }),
+ url: '/accounts/self/username',
+ reportUrlAsIs: true,
+ });
+ if (!response.ok) {
+ return;
+ }
+ let newName = undefined;
+ // If the name was deleted server returns 204
+ if (response.status !== 204) {
+ newName = (await readJSONResponsePayload(response))
+ .parsed as unknown as string;
+ }
+ this._updateCachedAccount({username: newName});
}
- setAccountDisplayName(displayName: string): Promise<void> {
- return this._restApiHelper
- .fetchJSON({
- fetchOptions: getFetchOptions({
- method: HttpMethod.PUT,
- body: {display_name: displayName},
- }),
- url: '/accounts/self/displayname',
- reportUrlAsIs: true,
- })
- .then(newName =>
- this._updateCachedAccount({
- display_name: newName as unknown as string,
- })
- );
+ async setAccountDisplayName(displayName: string): Promise<void> {
+ const response = await this._restApiHelper.fetch({
+ fetchOptions: getFetchOptions({
+ method: HttpMethod.PUT,
+ body: {display_name: displayName},
+ }),
+ url: '/accounts/self/displayname',
+ reportUrlAsIs: true,
+ reportServerError: true,
+ });
+ if (!response.ok) {
+ return;
+ }
+ let newName = undefined;
+ // If the name was deleted server returns 204
+ if (response.status !== 204) {
+ newName = (await readJSONResponsePayload(response))
+ .parsed as unknown as string;
+ }
+ this._updateCachedAccount({display_name: newName});
}
- setAccountStatus(status: string): Promise<void> {
- return this._restApiHelper
- .fetchJSON({
- fetchOptions: getFetchOptions({
- method: HttpMethod.PUT,
- body: {status},
- }),
- url: '/accounts/self/status',
- reportUrlAsIs: true,
- })
- .then(newStatus =>
- this._updateCachedAccount({status: newStatus as unknown as string})
- );
+ async setAccountStatus(status: string): Promise<void> {
+ const response = await this._restApiHelper.fetch({
+ fetchOptions: getFetchOptions({
+ method: HttpMethod.PUT,
+ body: {status},
+ }),
+ url: '/accounts/self/status',
+ reportUrlAsIs: true,
+ });
+ if (!response.ok) {
+ return;
+ }
+ let newStatus = undefined;
+ // If the status was deleted server returns 204
+ if (response.status !== 204) {
+ newStatus = (await readJSONResponsePayload(response))
+ .parsed as unknown as string;
+ }
+ this._updateCachedAccount({status: newStatus});
}
getAccountStatus(userId: AccountId) {
@@ -1396,9 +1421,13 @@
anonymizedUrl += '&base=*';
}
- return this._restApiHelper.fetchJSON({url, anonymizedUrl}) as Promise<
- {files: FileNameToFileInfoMap} | undefined
- >;
+ const response = await this._restApiHelper.fetch({url, anonymizedUrl});
+ if (!response.ok || response.status === 204) {
+ return undefined;
+ }
+ return (await readJSONResponsePayload(response)).parsed as unknown as
+ | {files: FileNameToFileInfoMap}
+ | undefined;
}
async queryChangeFiles(
@@ -2044,14 +2073,17 @@
return undefined;
}
const url = await this._changeBaseURL(changeNum);
- return this._restApiHelper.fetchJSON(
- {
- url: `${url}/edit/`,
- params,
- anonymizedUrl: `${ANONYMIZED_CHANGE_BASE_URL}/edit/`,
- },
- /* noAcceptHeader=*/ true
- ) as Promise<EditInfo | undefined>;
+ const response = await this._restApiHelper.fetch({
+ url: `${url}/edit/`,
+ params,
+ anonymizedUrl: `${ANONYMIZED_CHANGE_BASE_URL}/edit/`,
+ });
+ // If there is no edit patchset 204 is returned.
+ if (!response.ok || response.status === 204) {
+ return undefined;
+ }
+ return (await readJSONResponsePayload(response))
+ .parsed as unknown as EditInfo;
}
createChange(
@@ -2968,7 +3000,7 @@
errFn?: ErrorCallback
): Promise<string | undefined> {
const url = await this._changeBaseURL(changeNum);
- return this._restApiHelper.fetchJSON({
+ const response = await this._restApiHelper.fetch({
fetchOptions: getFetchOptions({
method: HttpMethod.PUT,
body: {topic},
@@ -2976,7 +3008,15 @@
url: `${url}/topic`,
anonymizedUrl: `${ANONYMIZED_CHANGE_BASE_URL}/topic`,
errFn,
- }) as unknown as Promise<string | undefined>;
+ });
+ if (!response.ok) {
+ return undefined;
+ }
+ if (response.status === 204) {
+ return '';
+ }
+ return (await readJSONResponsePayload(response))
+ .parsed as unknown as string;
}
removeChangeTopic(
@@ -3280,16 +3320,17 @@
patchNum?: PatchSetNum,
payload?: RequestPayload,
errFn?: ErrorCallback
- ): Promise<ParsedJSON | undefined> {
+ ): Promise<Response> {
const url = await this._changeBaseURL(changeNum, patchNum);
// No anonymizedUrl specified so the request will not be logged.
- return this._restApiHelper.fetchJSON({
+ return this._restApiHelper.fetch({
fetchOptions: getFetchOptions({
method,
body: payload,
}),
url: url + endpoint,
errFn,
+ reportServerError: true,
});
}
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 fec82e3..2fe0598 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
@@ -19,6 +19,7 @@
createAccountWithId,
createChange,
createComment,
+ createEditInfo,
createParsedChange,
createServerInfo,
TEST_PROJECT_NAME,
@@ -606,10 +607,154 @@
] as unknown as ParsedJSON);
});
+ test('setAccountUsername', async () => {
+ const fetchStub = sinon
+ .stub(element._restApiHelper, 'fetch')
+ .resolves(new Response(makePrefixedJSON('john')));
+ element._cache.set(
+ '/accounts/self/detail',
+ createAccountDetailWithId() as unknown as ParsedJSON
+ );
+ await element.setAccountUsername('john');
+ assert.isTrue(fetchStub.calledOnce);
+ assert.equal(
+ fetchStub.lastCall.args[0].fetchOptions?.method,
+ HttpMethod.PUT
+ );
+ assert.equal(fetchStub.lastCall.args[0].url, '/accounts/self/username');
+ assert.deepEqual(
+ JSON.parse(fetchStub.lastCall.args[0].fetchOptions?.body as string),
+ {username: 'john'}
+ );
+ assert.deepEqual(
+ (element._cache.get(
+ '/accounts/self/detail'
+ ) as unknown as AccountDetailInfo)!.username,
+ 'john'
+ );
+ });
+
+ test('setAccountUsername empty unsets field', async () => {
+ const fetchStub = sinon
+ .stub(element._restApiHelper, 'fetch')
+ .resolves(new Response(undefined, {status: 204}));
+ element._cache.set('/accounts/self/detail', {
+ ...createAccountDetailWithId(),
+ username: 'john',
+ } as unknown as ParsedJSON);
+ await element.setAccountUsername('');
+ assert.isTrue(fetchStub.calledOnce);
+ assert.equal(
+ fetchStub.lastCall.args[0].fetchOptions?.method,
+ HttpMethod.PUT
+ );
+ assert.isUndefined(
+ (element._cache.get(
+ '/accounts/self/detail'
+ ) as unknown as AccountDetailInfo)!.username
+ );
+ });
+
+ test('setAccountDisplayName', async () => {
+ const fetchStub = sinon
+ .stub(element._restApiHelper, 'fetch')
+ .resolves(new Response(makePrefixedJSON('john')));
+ element._cache.set(
+ '/accounts/self/detail',
+ createAccountDetailWithId() as unknown as ParsedJSON
+ );
+ await element.setAccountDisplayName('john');
+ assert.isTrue(fetchStub.calledOnce);
+ assert.equal(
+ fetchStub.lastCall.args[0].fetchOptions?.method,
+ HttpMethod.PUT
+ );
+ assert.equal(fetchStub.lastCall.args[0].url, '/accounts/self/displayname');
+ assert.deepEqual(
+ JSON.parse(fetchStub.lastCall.args[0].fetchOptions?.body as string),
+ {display_name: 'john'}
+ );
+ assert.deepEqual(
+ (element._cache.get(
+ '/accounts/self/detail'
+ ) as unknown as AccountDetailInfo)!.display_name,
+ 'john'
+ );
+ });
+
+ test('setAccountDisplayName empty unsets field', async () => {
+ const fetchStub = sinon
+ .stub(element._restApiHelper, 'fetch')
+ .resolves(new Response(undefined, {status: 204}));
+ element._cache.set('/accounts/self/detail', {
+ ...createAccountDetailWithId(),
+ display_name: 'john',
+ } as unknown as ParsedJSON);
+ await element.setAccountDisplayName('');
+ assert.isTrue(fetchStub.calledOnce);
+ assert.equal(
+ fetchStub.lastCall.args[0].fetchOptions?.method,
+ HttpMethod.PUT
+ );
+ assert.isUndefined(
+ (element._cache.get(
+ '/accounts/self/detail'
+ ) as unknown as AccountDetailInfo)!.display_name
+ );
+ });
+
+ test('setAccountName', async () => {
+ const fetchStub = sinon
+ .stub(element._restApiHelper, 'fetch')
+ .resolves(new Response(makePrefixedJSON('john')));
+ element._cache.set(
+ '/accounts/self/detail',
+ createAccountDetailWithId() as unknown as ParsedJSON
+ );
+ await element.setAccountName('john');
+ assert.isTrue(fetchStub.calledOnce);
+ assert.equal(
+ fetchStub.lastCall.args[0].fetchOptions?.method,
+ HttpMethod.PUT
+ );
+ assert.equal(fetchStub.lastCall.args[0].url, '/accounts/self/name');
+ assert.deepEqual(
+ JSON.parse(fetchStub.lastCall.args[0].fetchOptions?.body as string),
+ {name: 'john'}
+ );
+ assert.deepEqual(
+ (element._cache.get(
+ '/accounts/self/detail'
+ ) as unknown as AccountDetailInfo)!.name,
+ 'john'
+ );
+ });
+
+ test('setAccountName empty unsets field', async () => {
+ const fetchStub = sinon
+ .stub(element._restApiHelper, 'fetch')
+ .resolves(new Response(undefined, {status: 204}));
+ element._cache.set('/accounts/self/detail', {
+ ...createAccountDetailWithId(),
+ name: 'john',
+ } as unknown as ParsedJSON);
+ await element.setAccountName('');
+ assert.isTrue(fetchStub.calledOnce);
+ assert.equal(
+ fetchStub.lastCall.args[0].fetchOptions?.method,
+ HttpMethod.PUT
+ );
+ assert.isUndefined(
+ (element._cache.get(
+ '/accounts/self/detail'
+ ) as unknown as AccountDetailInfo)!.name
+ );
+ });
+
test('setAccountStatus', async () => {
const fetchStub = sinon
- .stub(element._restApiHelper, 'fetchJSON')
- .resolves('OOO' as unknown as ParsedJSON);
+ .stub(element._restApiHelper, 'fetch')
+ .resolves(new Response(makePrefixedJSON('OOO')));
element._cache.set(
'/accounts/self/detail',
createAccountDetailWithId() as unknown as ParsedJSON
@@ -633,6 +778,27 @@
);
});
+ test('setAccountStatus empty unsets field', async () => {
+ const fetchStub = sinon
+ .stub(element._restApiHelper, 'fetch')
+ .resolves(new Response(undefined, {status: 204}));
+ element._cache.set('/accounts/self/detail', {
+ ...createAccountDetailWithId(),
+ status: 'OOO',
+ } as unknown as ParsedJSON);
+ await element.setAccountStatus('');
+ assert.isTrue(fetchStub.calledOnce);
+ assert.equal(
+ fetchStub.lastCall.args[0].fetchOptions?.method,
+ HttpMethod.PUT
+ );
+ assert.isUndefined(
+ (element._cache.get(
+ '/accounts/self/detail'
+ ) as unknown as AccountDetailInfo)!.status
+ );
+ });
+
suite('draft comments', () => {
test('_sendDiffDraftRequest pending requests tracked', async () => {
const obj = element._pendingRequests;
@@ -1385,7 +1551,9 @@
test('setChangeTopic', async () => {
element.addRepoNameToCache(123 as NumericChangeId, TEST_PROJECT_NAME);
- const fetchStub = sinon.stub(element._restApiHelper, 'fetchJSON');
+ const fetchStub = sinon
+ .stub(element._restApiHelper, 'fetch')
+ .resolves(new Response(makePrefixedJSON('foo-bar')));
await element.setChangeTopic(123 as NumericChangeId, 'foo-bar');
assert.isTrue(fetchStub.calledOnce);
assert.deepEqual(
@@ -1641,6 +1809,36 @@
assert.isTrue(getChangeFilesStub.calledOnce);
});
+ test('getChangeEdit not logged in returns undefined', async () => {
+ element.addRepoNameToCache(123 as NumericChangeId, TEST_PROJECT_NAME);
+ sinon.stub(element, 'getLoggedIn').resolves(false);
+ const fetchSpy = sinon.spy(element._restApiHelper, 'fetch');
+ const edit = await element.getChangeEdit(123 as NumericChangeId);
+ assert.isUndefined(edit);
+ assert.isFalse(fetchSpy.called);
+ });
+
+ test('getChangeEdit no edit patchset returns undefined', async () => {
+ element.addRepoNameToCache(123 as NumericChangeId, TEST_PROJECT_NAME);
+ sinon.stub(element, 'getLoggedIn').resolves(true);
+ sinon
+ .stub(element._restApiHelper, 'fetch')
+ .resolves(new Response(undefined, {status: 204}));
+ const edit = await element.getChangeEdit(123 as NumericChangeId);
+ assert.isUndefined(edit);
+ });
+
+ test('getChangeEdit returns edit patchset', async () => {
+ element.addRepoNameToCache(123 as NumericChangeId, TEST_PROJECT_NAME);
+ sinon.stub(element, 'getLoggedIn').resolves(true);
+ const expected = createEditInfo();
+ sinon
+ .stub(element._restApiHelper, 'fetch')
+ .resolves(new Response(makePrefixedJSON(expected)));
+ const edit = await element.getChangeEdit(123 as NumericChangeId);
+ assert.deepEqual(edit, expected);
+ });
+
test('ported comment errors do not trigger error dialog', () => {
const change = createChange();
const handler = sinon.stub();
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 8389a15..947952c 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
@@ -120,8 +120,8 @@
params?: string[]
): Promise<AccountCapabilityInfo | undefined>;
getExternalIds(): Promise<AccountExternalIdInfo[] | undefined>;
- deleteAccountIdentity(id: string[]): Promise<unknown>;
- deleteAccount(): Promise<unknown>;
+ deleteAccountIdentity(id: string[]): Promise<Response>;
+ deleteAccount(): Promise<Response>;
getRepos(
filter: string | undefined,
reposPerPage: number,
@@ -192,7 +192,7 @@
patchNum?: PatchSetNum,
payload?: RequestPayload,
errFn?: ErrorCallback
- ): Promise<ParsedJSON | undefined>;
+ ): Promise<Response>;
getRepoBranches(
filter: string,
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 8e9ee16..77f2498 100644
--- a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
+++ b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
@@ -128,7 +128,7 @@
deleteAccountGPGKey(): Promise<Response> {
return Promise.resolve(new Response());
},
- deleteAccountIdentity(): Promise<unknown> {
+ deleteAccountIdentity(): Promise<Response> {
return Promise.resolve(new Response());
},
deleteAccountSSHKey(): void {},
@@ -165,8 +165,8 @@
deleteWatchedProjects(): Promise<Response> {
return Promise.resolve(new Response());
},
- executeChangeAction(): Promise<ParsedJSON | undefined> {
- return Promise.resolve({}) as Promise<ParsedJSON>;
+ executeChangeAction(): Promise<Response> {
+ return Promise.resolve(new Response());
},
finalize(): void {},
generateAccountHttpPassword(): Promise<Password | undefined> {
