Skip to content

Commit 8971f17

Browse files
priyajeetpriyajeet
authored
Chore: Cleanup circular dependency in delete api (#32)
Adds unit tests for file and weblink api Adds couple of more flow types
1 parent c5fbd0c commit 8971f17

11 files changed

Lines changed: 134 additions & 44 deletions

File tree

‎src/api/Base.js‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import Xhr from '../util/Xhr';
88
import Cache from '../util/Cache';
9+
import { DEFAULT_HOSTNAME_API, DEFAULT_HOSTNAME_UPLOAD } from '../constants';
910

1011
class Base {
1112
/**
@@ -52,8 +53,8 @@ class Base {
5253
constructor(options: any = {}) {
5354
this.options = options;
5455
this.cache = options.cache;
55-
this.apiHost = options.apiHost;
56-
this.uploadHost = options.uploadHost;
56+
this.apiHost = options.apiHost || DEFAULT_HOSTNAME_API;
57+
this.uploadHost = options.uploadHost || DEFAULT_HOSTNAME_UPLOAD;
5758
this.xhr = new Xhr(options);
5859
this.destroyed = false;
5960
}

‎src/api/File.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
*/
66

77
import Item from './Item';
8-
import { FIELD_DOWNLOAD_URL } from '../constants';
8+
import { FIELD_DOWNLOAD_URL, CACHE_PREFIX_FILE } from '../constants';
99
import type { BoxItem } from '../flowTypes';
1010

1111
class File extends Item {
@@ -16,7 +16,7 @@ class File extends Item {
1616
* @return {string} key
1717
*/
1818
getCacheKey(id: string): string {
19-
return `file_${id}`;
19+
return `${CACHE_PREFIX_FILE}${id}`;
2020
}
2121

2222
/**
@@ -37,7 +37,7 @@ class File extends Item {
3737
* @return {void}
3838
*/
3939
getDownloadUrl(id: string, successCallback: Function, errorCallback: Function): void {
40-
this.xhr
40+
return this.xhr
4141
.get(this.getUrl(id), {
4242
fields: FIELD_DOWNLOAD_URL
4343
})

‎src/api/Folder.js‎

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,12 @@
44
* @author Box
55
*/
66

7-
import noop from 'lodash.noop';
87
import Item from './Item';
98
import flatten from '../util/flatten';
109
import sort from '../util/sorter';
1110
import FileAPI from '../api/File';
1211
import WebLinkAPI from '../api/WebLink';
13-
import { FIELDS_TO_FETCH } from '../constants';
12+
import { FIELDS_TO_FETCH, CACHE_PREFIX_FOLDER } from '../constants';
1413
import getBadItemError from '../util/error';
1514
import type {
1615
BoxItem,
@@ -72,7 +71,7 @@ class Folder extends Item {
7271
* @return {string} key
7372
*/
7473
getCacheKey(id: string): string {
75-
return `folder_${id}`;
74+
return `${CACHE_PREFIX_FOLDER}${id}`;
7675
}
7776

7877
/**
@@ -264,19 +263,6 @@ class Folder extends Item {
264263
// Make the XHR request
265264
this.folderRequest();
266265
}
267-
268-
/**
269-
* API to delete a folder
270-
*
271-
* @param {String} id - item id
272-
* @param {String} parentId - item parent id
273-
* @param {Function} successCallback - success callback
274-
* @param {Function} errorCallback - error callback
275-
* @return {void}
276-
*/
277-
delete(id: string, parentId: string, successCallback: Function, errorCallback: Function = noop): void {
278-
super.delete(id, parentId, successCallback, errorCallback, true);
279-
}
280266
}
281267

282268
export default Folder;

‎src/api/Item.js‎

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,9 @@
66

77
import noop from 'lodash.noop';
88
import Base from './Base';
9-
import FolderAPI from './Folder';
10-
import { ACCESS_NONE, CACHE_PREFIX_SEARCH } from '../constants';
119
import getBadItemError from '../util/error';
12-
import type { BoxItem, FlattenedBoxItem, FlattenedBoxItemCollection } from '../flowTypes';
10+
import { ACCESS_NONE, CACHE_PREFIX_SEARCH, CACHE_PREFIX_FOLDER, TYPE_FOLDER } from '../constants';
11+
import type { BoxItem, FlattenedBoxItem, FlattenedBoxItemCollection, BoxItemPermission } from '../flowTypes';
1312

1413
class Item extends Base {
1514
/**
@@ -32,6 +31,17 @@ class Item extends Base {
3231
*/
3332
errorCallback: Function;
3433

34+
/**
35+
* Creates a key for the item's parent
36+
* This is always a folder
37+
*
38+
* @param {string} id folder id
39+
* @return {string} key
40+
*/
41+
getParentCacheKey(id: string): string {
42+
return `${CACHE_PREFIX_FOLDER}${id}`;
43+
}
44+
3545
/**
3646
* Handles error responses
3747
*
@@ -111,13 +121,11 @@ class Item extends Base {
111121
return;
112122
}
113123

114-
const parentAPI = new FolderAPI(this.options);
115-
const parentKey: string = parentAPI.getCacheKey(this.parentId);
116-
117124
// When fetching the parent folder from the cache
118125
// we have no guarantees that it will be there since
119126
// search results happen across folders and we only
120127
// add those folders to cache that have been navigated to.
128+
const parentKey: string = this.getParentCacheKey(this.parentId);
121129
const folder: ?FlattenedBoxItem = this.getCache().get(parentKey);
122130
if (!folder) {
123131
this.postDeleteCleanup();
@@ -152,25 +160,32 @@ class Item extends Base {
152160
/**
153161
* API to delete an Item
154162
*
155-
* @param {String} id - item id
163+
* @param {Object} item - item to delete
156164
* @param {Function} successCallback - success callback
157165
* @param {Function} errorCallback - error callback
158166
* @param {Boolean} recursive - true for folders
159167
* @return {void}
160168
*/
161-
delete(
162-
id: string,
163-
parentId: string,
164-
successCallback: Function,
165-
errorCallback: Function = noop,
166-
recursive: boolean = false
167-
): void {
169+
delete(item: BoxItem, successCallback: Function, errorCallback: Function = noop): void {
170+
const { id, permissions, parent, type }: BoxItem = item;
171+
if (!id || !permissions || !parent || !type) {
172+
errorCallback();
173+
return;
174+
}
175+
176+
const { id: parentId } = parent;
177+
const { can_delete }: BoxItemPermission = permissions;
178+
if (!can_delete || !parentId) {
179+
errorCallback();
180+
return;
181+
}
182+
168183
this.id = id;
169184
this.parentId = parentId;
170185
this.successCallback = successCallback;
171186
this.errorCallback = errorCallback;
172187

173-
const url = `${this.getUrl(id)}${recursive ? '?recursive=true' : ''}`;
188+
const url = `${this.getUrl(id)}${type === TYPE_FOLDER ? '?recursive=true' : ''}`;
174189
this.xhr.delete(url).then(this.deleteSuccessHandler).catch(this.errorHandler);
175190
}
176191

‎src/api/Search.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ class Search extends Base {
8888
* @return {string} key
8989
*/
9090
getCacheKey(id: string, query: string): string {
91-
return `${CACHE_PREFIX_SEARCH}folder_${id}|${query}`;
91+
return `${CACHE_PREFIX_SEARCH}${id}|${query}`;
9292
}
9393

9494
/**

‎src/api/WebLink.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
*/
66

77
import Item from './Item';
8+
import { CACHE_PREFIX_WEBLINK } from '../constants';
89

910
class WebLink extends Item {
1011
/**
@@ -14,7 +15,7 @@ class WebLink extends Item {
1415
* @return {string} key
1516
*/
1617
getCacheKey(id: string): string {
17-
return `web_link_${id}`;
18+
return `${CACHE_PREFIX_WEBLINK}${id}`;
1819
}
1920

2021
/**

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
import File from '../File';
2+
3+
let file;
4+
const sandbox = sinon.sandbox.create();
5+
6+
describe('api/File', () => {
7+
beforeEach(() => {
8+
file = new File();
9+
});
10+
11+
afterEach(() => {
12+
sandbox.verifyAndRestore();
13+
});
14+
15+
describe('getCacheKey()', () => {
16+
it('should return correct key', () => {
17+
expect(file.getCacheKey('foo')).to.equal('file_foo');
18+
});
19+
});
20+
21+
describe('getUrl()', () => {
22+
it('should return correct file api url without id', () => {
23+
expect(file.getUrl()).to.equal('https://api.box.com/2.0/files');
24+
});
25+
it('should return correct file api url with id', () => {
26+
expect(file.getUrl('foo')).to.equal('https://api.box.com/2.0/files/foo');
27+
});
28+
});
29+
30+
describe('getDownloadUrl()', () => {
31+
it('should make xhr to get download url and call success callback', () => {
32+
const success = sandbox.mock().withArgs('bar');
33+
const get = sandbox
34+
.mock()
35+
.withArgs('https://api.box.com/2.0/files/foo', { fields: 'download_url' })
36+
.returns(Promise.resolve({ download_url: 'bar' }));
37+
file.xhr = { get };
38+
return file.getDownloadUrl('foo', success);
39+
});
40+
it('should make xhr to get download url and call error callback', () => {
41+
const success = sandbox.mock().never();
42+
const error = sandbox.mock().withArgs('error');
43+
const get = sandbox
44+
.mock()
45+
.withArgs('https://api.box.com/2.0/files/foo', { fields: 'download_url' })
46+
.returns(Promise.reject('error'));
47+
file.xhr = { get };
48+
return file.getDownloadUrl('foo', success, error);
49+
});
50+
});
51+
});

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
import WebLink from '../WebLink';
2+
3+
let webLink;
4+
const sandbox = sinon.sandbox.create();
5+
6+
describe('api/WebLink', () => {
7+
beforeEach(() => {
8+
webLink = new WebLink();
9+
});
10+
11+
afterEach(() => {
12+
sandbox.verifyAndRestore();
13+
});
14+
15+
describe('getCacheKey()', () => {
16+
it('should return correct key', () => {
17+
expect(webLink.getCacheKey('foo')).to.equal('web_link_foo');
18+
});
19+
});
20+
21+
describe('getUrl()', () => {
22+
it('should return correct web link api url without id', () => {
23+
expect(webLink.getUrl()).to.equal('https://api.box.com/2.0/web_links');
24+
});
25+
it('should return correct web link api url with id', () => {
26+
expect(webLink.getUrl('foo')).to.equal('https://api.box.com/2.0/web_links/foo');
27+
});
28+
});
29+
});

‎src/components/ContentExplorer/ContentExplorer.js‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,8 @@ import type {
4646
SortBy,
4747
Access,
4848
BoxItemPermission,
49-
ItemAPI
49+
ItemAPI,
50+
ItemType
5051
} from '../../flowTypes';
5152
import '../fonts.scss';
5253
import '../base.scss';
@@ -218,7 +219,7 @@ class ContentExplorer extends Component<DefaultProps, Props, State> {
218219
* @param {String} type - item type
219220
* @return {ItemAPI} api
220221
*/
221-
getAPI(type: string): ItemAPI {
222+
getAPI(type: ItemType): ItemAPI {
222223
let api: ItemAPI;
223224

224225
switch (type) {
@@ -777,7 +778,7 @@ class ContentExplorer extends Component<DefaultProps, Props, State> {
777778
}
778779

779780
this.setState({ isLoading: true });
780-
this.getAPI(type).delete(id, parentId, () => {
781+
this.getAPI(type).delete(selected, () => {
781782
onDelete(cloneDeep([selected]));
782783
if (view === VIEW_FOLDER) {
783784
this.fetchFolder(parentId);

‎src/components/ContentPicker/ContentPicker.js‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,8 @@ import type {
4242
SortBy,
4343
Access,
4444
BoxItemPermission,
45-
ItemAPI
45+
ItemAPI,
46+
ItemType
4647
} from '../../flowTypes';
4748
import '../fonts.scss';
4849
import '../base.scss';
@@ -187,7 +188,7 @@ class ContentPicker extends Component<DefaultProps, Props, State> {
187188
* @param {String} type - item type
188189
* @return {ItemAPI} api
189190
*/
190-
getAPI(type: string): ItemAPI {
191+
getAPI(type: ItemType): ItemAPI {
191192
let api: ItemAPI;
192193

193194
switch (type) {

0 commit comments

Comments
 (0)