Skip to content

Commit 69ad154

Browse files
Mingzemergify[bot]
Mingze
authored and
mergify[bot]
committed
perf(sidebar): merge response to avoid redundant api calls (#1617)
* perf(sidebar): use state to avoid redundant api calls * perf(sidebar): fetch version for delete * perf(sidebar): fetch data after restore
1 parent 4ab0c20 commit 69ad154

File tree

9 files changed

+144
-30
lines changed

9 files changed

+144
-30
lines changed

‎src/api/Feed.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ class Feed extends Base {
251251

252252
return new Promise(resolve => {
253253
const { file_version = {} } = this.file;
254-
this.versionsAPI.getCurrentVersion(
254+
this.versionsAPI.getVersion(
255255
this.file.id,
256256
file_version.id,
257257
resolve,

‎src/api/Versions.js‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import {
1111
DEFAULT_FETCH_END,
1212
DEFAULT_FETCH_START,
1313
ERROR_CODE_DELETE_VERSION,
14-
ERROR_CODE_FETCH_CURRENT_VERSION,
14+
ERROR_CODE_FETCH_VERSION,
1515
ERROR_CODE_FETCH_VERSIONS,
1616
ERROR_CODE_PROMOTE_VERSION,
1717
ERROR_CODE_RESTORE_VERSION,
@@ -153,21 +153,22 @@ class Versions extends OffsetBasedAPI {
153153
}
154154

155155
/**
156-
* API for fetching the current version for a file
156+
* API for fetching a certain version for a file
157157
*
158158
* @param {string} fileId - a box file id
159159
* @param {string} fileVersionId - a box file version id
160160
* @param {Function} successCallback - the success callback
161161
* @param {Function} errorCallback - the error callback
162162
* @returns {void}
163163
*/
164-
getCurrentVersion(
164+
getVersion(
165165
fileId: string,
166166
fileVersionId: string,
167167
successCallback: BoxItemVersion => void,
168168
errorCallback: ElementsErrorCallback,
169169
): void {
170-
this.errorCode = ERROR_CODE_FETCH_CURRENT_VERSION;
170+
this.errorCode = ERROR_CODE_FETCH_VERSION;
171+
171172
this.get({
172173
id: fileId,
173174
successCallback,

‎src/api/__tests__/Feed-test.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ jest.mock('../Comments', () =>
245245
jest.mock('../Versions', () => {
246246
return jest.fn().mockImplementation(() => ({
247247
getVersions: jest.fn(() => mockFirstVersion),
248-
getCurrentVersion: jest.fn(() => mockCurrentVersion),
248+
getVersion: jest.fn(() => mockCurrentVersion),
249249
}));
250250
});
251251

@@ -439,7 +439,7 @@ describe('api/Feed', () => {
439439
feed.fetchAppActivity = jest.fn().mockReturnValue(appActivities);
440440
feed.setCachedItems = jest.fn();
441441
feed.versionsAPI = {
442-
getCurrentVersion: jest.fn().mockReturnValue(versions),
442+
getVersion: jest.fn().mockReturnValue(versions),
443443
addCurrentVersion: jest.fn().mockReturnValue(versionsWithCurrent),
444444
};
445445
successCb = jest.fn();
@@ -570,7 +570,7 @@ describe('api/Feed', () => {
570570
test('should return a promise and call the versions api', () => {
571571
const currentVersion = feed.fetchCurrentVersion();
572572
expect(currentVersion instanceof Promise).toBeTruthy();
573-
expect(feed.versionsAPI.getCurrentVersion).toBeCalled();
573+
expect(feed.versionsAPI.getVersion).toBeCalled();
574574
});
575575
});
576576

‎src/api/__tests__/Versions-test.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -172,12 +172,12 @@ describe('api/Versions', () => {
172172
});
173173
});
174174

175-
describe('getCurrentVersion()', () => {
176-
test('should return the current version information', () => {
175+
describe('getVersion()', () => {
176+
test('should return the version information', () => {
177177
const url = 'https://box.com/api/file/fileID/versions';
178178
versions.getVersionUrl = jest.fn().mockReturnValue(url);
179179

180-
versions.getCurrentVersion(fileId, file.file_version.id, successCallback, errorCallback);
180+
versions.getVersion(fileId, file.file_version.id, successCallback, errorCallback);
181181

182182
expect(versions.get).toBeCalledWith({
183183
id: fileId,

‎src/constants.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ export const ERROR_CODE_FETCH_FOLDER = 'fetch_folder_error';
218218
export const ERROR_CODE_FETCH_WEBLINK = 'fetch_weblink_error';
219219
export const ERROR_CODE_FETCH_CLASSIFICATION = 'fetch_classification_error';
220220
export const ERROR_CODE_FETCH_COMMENTS = 'fetch_comments_error';
221-
export const ERROR_CODE_FETCH_CURRENT_VERSION = 'fetch_current_version_error';
221+
export const ERROR_CODE_FETCH_VERSION = 'fetch_version_error';
222222
export const ERROR_CODE_FETCH_VERSIONS = 'fetch_versions_error';
223223
export const ERROR_CODE_FETCH_TASKS = 'fetch_tasks_error';
224224
export const ERROR_CODE_FETCH_CURRENT_USER = 'fetch_current_user_error';

‎src/elements/content-sidebar/versions/VersionsSidebarAPI.js‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ export default class VersionsSidebarAPI {
5151
const { file_version = {} } = fileResponse;
5252

5353
return new Promise((resolve, reject) =>
54-
this.api.getVersionsAPI(false).getCurrentVersion(
54+
this.api.getVersionsAPI(false).getVersion(
5555
this.fileId,
5656
file_version.id,
5757
(currentVersionResponse: BoxItemVersion) => {
@@ -67,6 +67,12 @@ export default class VersionsSidebarAPI {
6767
);
6868
};
6969

70+
fetchVersion = (versionId: string): Promise<BoxItemVersion> => {
71+
return new Promise((resolve, reject) =>
72+
this.api.getVersionsAPI(false).getVersion(this.fileId, versionId, resolve, reject),
73+
);
74+
};
75+
7076
deleteVersion = (version: ?BoxItemVersion): Promise<null> => {
7177
const { id: versionId, permissions = {} } = version || {};
7278

‎src/elements/content-sidebar/versions/VersionsSidebarContainer.js‎

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import React from 'react';
88
import flow from 'lodash/flow';
99
import getProp from 'lodash/get';
10+
import merge from 'lodash/merge';
1011
import noop from 'lodash/noop';
1112
import { generatePath, withRouter } from 'react-router-dom';
1213
import type { Match, RouterHistory } from 'react-router-dom';
@@ -95,9 +96,8 @@ class VersionsSidebarContainer extends React.Component<Props, State> {
9596

9697
return this.api
9798
.deleteVersion(this.findVersion(versionId))
98-
.then(this.api.fetchData)
99-
.then(this.handleFetchSuccess)
100-
.then(() => this.handleDeleteSuccess(versionId))
99+
.then(() => this.api.fetchVersion(versionId))
100+
.then(this.handleDeleteSuccess)
101101
.then(() => this.props.onVersionDelete(versionId))
102102
.catch(() => this.handleActionError(messages.versionActionDeleteError));
103103
};
@@ -132,8 +132,8 @@ class VersionsSidebarContainer extends React.Component<Props, State> {
132132

133133
return this.api
134134
.restoreVersion(this.findVersion(versionId))
135-
.then(this.api.fetchData)
136-
.then(this.handleFetchSuccess)
135+
.then(() => this.api.fetchVersion(versionId))
136+
.then(this.handleRestoreSuccess)
137137
.then(() => this.props.onVersionRestore(versionId))
138138
.catch(() => this.handleActionError(messages.versionActionRestoreError));
139139
};
@@ -145,15 +145,22 @@ class VersionsSidebarContainer extends React.Component<Props, State> {
145145
});
146146
};
147147

148-
handleDeleteSuccess = (versionId: string) => {
148+
handleDeleteSuccess = (data: BoxItemVersion): void => {
149149
const { versionId: selectedVersionId } = this.props;
150+
const { id: versionId } = data;
151+
152+
this.mergeResponse(data);
150153

151154
// Bump the user to the current version if they deleted their selected version
152155
if (versionId === selectedVersionId) {
153156
this.updateVersionToCurrent();
154157
}
155158
};
156159

160+
handleRestoreSuccess = (data: BoxItemVersion): void => {
161+
this.mergeResponse(data);
162+
};
163+
157164
handleFetchError = (): void => {
158165
this.setState({
159166
error: messages.versionFetchError,
@@ -217,6 +224,22 @@ class VersionsSidebarContainer extends React.Component<Props, State> {
217224
return versions[0] ? versions[0].id : null;
218225
};
219226

227+
mergeVersions = (newVersion: BoxItemVersion): Array<BoxItemVersion> => {
228+
const { versions } = this.state;
229+
const newVersionId = newVersion ? newVersion.id : '';
230+
return versions.map(version => (version.id === newVersionId ? merge({ ...version }, newVersion) : version));
231+
};
232+
233+
mergeResponse = (data: BoxItemVersion): void => {
234+
const newVersions = this.mergeVersions(data);
235+
236+
this.setState({
237+
error: undefined,
238+
isLoading: false,
239+
versions: newVersions,
240+
});
241+
};
242+
220243
refresh(): void {
221244
this.initialize();
222245
this.setState({ isLoading: true }, this.fetchData);

‎src/elements/content-sidebar/versions/__tests__/VersionsSidebarAPI-test.js‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ describe('VersionsSidebarAPI', () => {
1313
addPermissions: jest.fn(),
1414
deleteVersion: jest.fn(),
1515
getVersions: jest.fn(),
16-
getCurrentVersion: jest.fn(),
16+
getVersion: jest.fn(),
1717
promoteVersion: jest.fn(),
1818
restoreVersion: jest.fn(),
1919
sortVersions: jest.fn(),
@@ -100,7 +100,21 @@ describe('VersionsSidebarAPI', () => {
100100
const versions = { entries: [mockVersion], total_count: 1 };
101101

102102
expect(instance.fetchVersionCurrent([file, versions])).toBeInstanceOf(Promise);
103-
expect(versionsAPI.getCurrentVersion).toBeCalledWith(
103+
expect(versionsAPI.getVersion).toBeCalledWith(
104+
defaultFileId,
105+
defaultVersionId,
106+
expect.any(Function),
107+
expect.any(Function),
108+
);
109+
});
110+
});
111+
112+
describe('fetchVersion', () => {
113+
test('should call getVersion', () => {
114+
const instance = getInstance();
115+
116+
expect(instance.fetchVersion(defaultVersionId)).toBeInstanceOf(Promise);
117+
expect(versionsAPI.getVersion).toBeCalledWith(
104118
defaultFileId,
105119
defaultVersionId,
106120
expect.any(Function),

‎src/elements/content-sidebar/versions/__tests__/VersionsSidebarContainer-test.js‎

Lines changed: 79 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -71,18 +71,17 @@ describe('elements/content-sidebar/versions/VersionsSidebarContainer', () => {
7171
const wrapper = getWrapper({ onVersionDelete: handleDelete, versionId: '123' });
7272
const instance = wrapper.instance();
7373
const version = { id: '456' };
74+
const newVersion = { id: '456', trash_at: '' };
7475

7576
instance.api.deleteVersion = jest.fn().mockResolvedValueOnce();
76-
instance.api.fetchData = jest.fn().mockResolvedValueOnce();
77+
instance.api.fetchVersion = jest.fn().mockResolvedValueOnce(newVersion);
7778
instance.findVersion = jest.fn(() => version);
78-
instance.handleFetchSuccess = jest.fn();
7979
instance.handleDeleteSuccess = jest.fn();
8080

8181
instance.handleActionDelete(version.id).then(() => {
8282
expect(instance.api.deleteVersion).toHaveBeenCalledWith(version);
83-
expect(instance.api.fetchData).toHaveBeenCalled();
84-
expect(instance.handleFetchSuccess).toHaveBeenCalled();
85-
expect(instance.handleDeleteSuccess).toHaveBeenCalledWith(version.id);
83+
expect(instance.api.fetchVersion).toHaveBeenCalled();
84+
expect(instance.handleDeleteSuccess).toHaveBeenCalledWith(newVersion);
8685
expect(handleDelete).toHaveBeenCalledWith(version.id);
8786
});
8887
});
@@ -136,21 +135,54 @@ describe('elements/content-sidebar/versions/VersionsSidebarContainer', () => {
136135
const wrapper = getWrapper({ onVersionRestore: handleRestore, versionId: '123' });
137136
const instance = wrapper.instance();
138137
const version = { id: '456' };
138+
const newVersion = { id: '456', restored_by: '' };
139139

140-
instance.api.fetchData = jest.fn().mockResolvedValueOnce();
141140
instance.api.restoreVersion = jest.fn().mockResolvedValueOnce();
141+
instance.api.fetchVersion = jest.fn().mockResolvedValueOnce(newVersion);
142142
instance.findVersion = jest.fn(() => version);
143-
instance.handleFetchSuccess = jest.fn();
143+
instance.handleRestoreSuccess = jest.fn();
144144

145145
instance.handleActionRestore(version.id).then(() => {
146146
expect(instance.api.restoreVersion).toHaveBeenCalledWith(version);
147-
expect(instance.api.fetchData).toHaveBeenCalled();
148-
expect(instance.handleFetchSuccess).toHaveBeenCalled();
147+
expect(instance.api.fetchVersion).toHaveBeenCalled();
148+
expect(instance.handleRestoreSuccess).toHaveBeenCalledWith(newVersion);
149149
expect(handleRestore).toHaveBeenCalledWith(version.id);
150150
});
151151
});
152152
});
153153

154+
describe('handleDeleteSuccess', () => {
155+
test('should update version if the same id', () => {
156+
const wrapper = getWrapper({
157+
versionId: '123',
158+
});
159+
const instance = wrapper.instance();
160+
const version = { id: '123' };
161+
162+
instance.updateVersionToCurrent = jest.fn();
163+
instance.mergeResponse = jest.fn();
164+
165+
instance.handleDeleteSuccess(version);
166+
167+
expect(instance.updateVersionToCurrent).toHaveBeenCalled();
168+
expect(instance.mergeResponse).toHaveBeenCalledWith(version);
169+
});
170+
});
171+
172+
describe('handleRestoreSuccess', () => {
173+
test('should call mergeResponse', () => {
174+
const wrapper = getWrapper();
175+
const instance = wrapper.instance();
176+
const version = { id: '123' };
177+
178+
instance.mergeResponse = jest.fn();
179+
180+
instance.handleRestoreSuccess(version);
181+
182+
expect(instance.mergeResponse).toHaveBeenCalledWith(version);
183+
});
184+
});
185+
154186
describe('handleFetchError', () => {
155187
test('should set state to default values with error message', () => {
156188
const wrapper = getWrapper();
@@ -228,6 +260,44 @@ describe('elements/content-sidebar/versions/VersionsSidebarContainer', () => {
228260
});
229261
});
230262

263+
describe('mergeVersions', () => {
264+
test('should update state', () => {
265+
const wrapper = getWrapper();
266+
wrapper.setState({
267+
versions,
268+
});
269+
const instance = wrapper.instance();
270+
const newVersion = { id: versions[1].id, trashed_at: null };
271+
272+
const newVersions = instance.mergeVersions(newVersion);
273+
274+
expect(newVersions[0]).toEqual(versions[0]);
275+
expect(newVersions[1]).toEqual(Object.assign(versions[1], newVersion));
276+
});
277+
});
278+
279+
describe('mergeResponse', () => {
280+
test('should update state', () => {
281+
const wrapper = getWrapper();
282+
wrapper.setState({
283+
error: 'error',
284+
isLoading: true,
285+
versions,
286+
});
287+
const instance = wrapper.instance();
288+
const response = { id: '000', name: 'Version 0' };
289+
const newVersions = [response];
290+
instance.mergeVersions = jest.fn().mockReturnValue(newVersions);
291+
292+
instance.mergeResponse(response);
293+
294+
expect(wrapper.state('error')).toBe(undefined);
295+
expect(wrapper.state('isLoading')).toBe(false);
296+
expect(wrapper.state('versions')).toEqual(newVersions);
297+
expect(instance.mergeVersions).toHaveBeenCalledWith(response);
298+
});
299+
});
300+
231301
describe('refresh', () => {
232302
test('should refetch data when refresh is called', () => {
233303
const instance = getWrapper().instance();

0 commit comments

Comments
 (0)