diff --git a/src/api/Item.js b/src/api/Item.js index a41cb5b1dc..abb910a602 100644 --- a/src/api/Item.js +++ b/src/api/Item.js @@ -20,6 +20,7 @@ import { } from '../constants'; import type { ElementsErrorCallback, RequestData, RequestOptions } from '../common/types/api'; import type { + Access, BoxItem, BoxItemPermission, FlattenedBoxItem, @@ -165,7 +166,7 @@ class Item extends Base { /** * API to delete an Item * - * @param {Object} item - Item to delete + * @param {BoxItem} item - Item to delete * @param {Function} successCallback - Success callback * @param {Function} errorCallback - Error callback * @return {void} @@ -221,7 +222,7 @@ class Item extends Base { /** * API to rename an Item * - * @param {Object} item - Item to rename + * @param {BoxItem} item - Item to rename * @param {string} name - Item new name * @param {Function} successCallback - Success callback * @param {Function} errorCallback - Error callback @@ -286,32 +287,38 @@ class Item extends Base { }; /** - * Validate an item update request + * Validate request to update an item shared link * * @param {string|void} itemID - ID of item to share * @param {BoxItemPermission|void} itemPermissions - Permissions for item - * @param {boolean|void} canSkipSetShareAccess - skip the check for can_set_share_access when creating a new shared link + * @param {boolean|void} canSkipSetShareAccessPermission - skip `can_set_share_access` permission check * @throws {Error} * @return {void} */ - validateRequest(itemID: ?string, itemPermissions: ?BoxItemPermission, canSkipSetShareAccess: boolean = false) { + validateSharedLinkRequest( + itemID: ?string, + itemPermissions: ?BoxItemPermission, + canSkipSetShareAccessPermission: boolean = false, + ) { if (!itemID || !itemPermissions) { this.errorCode = ERROR_CODE_SHARE_ITEM; throw getBadItemError(); } + // It is sometimes necessary to skip `can_set_share_access` permission check + // e.g. Viewer permission can create shared links but cannot update access level const { can_share, can_set_share_access }: BoxItemPermission = itemPermissions; - if (!can_share || (!canSkipSetShareAccess && !can_set_share_access)) { + if (!can_share || (!canSkipSetShareAccessPermission && !can_set_share_access)) { this.errorCode = ERROR_CODE_SHARE_ITEM; throw getBadPermissionsError(); } } /** - * API to create, modify (change access) or remove a shared link + * API to create, modify (change access), or remove a shared link * - * @param {Object} item - Item to share - * @param {string} access - Shared access level + * @param {BoxItem} item - Item to share + * @param {Access} access - Shared access level * @param {Function} successCallback - Success callback * @param {Function|void} errorCallback - Error callback * @param {Array|void} [options.fields] - Optionally include specific fields @@ -319,7 +326,7 @@ class Item extends Base { */ async share( item: BoxItem, - access: ?string, // if "access" is undefined, the backend will set the default access level for the shared link + access?: Access, // if "access" is undefined, the backend will set the default access level for the shared link successCallback: Function, errorCallback: ElementsErrorCallback = noop, options: RequestOptions = {}, @@ -329,14 +336,14 @@ class Item extends Base { } try { - const { id, permissions }: BoxItem = item; + const { id, permissions, shared_link: sharedLink }: BoxItem = item; this.id = id; this.successCallback = successCallback; this.errorCallback = errorCallback; - // if we use the default access level, we don't need permission to set the access level - const canSkipSetShareAccess = access === undefined; - this.validateRequest(id, permissions, canSkipSetShareAccess); + // skip permission check when creating links with default access level + const canSkipSetShareAccessPermission = !sharedLink && access === undefined; + this.validateSharedLinkRequest(id, permissions, canSkipSetShareAccessPermission); const { fields } = options; const requestData: RequestData = { @@ -383,7 +390,7 @@ class Item extends Base { this.successCallback = successCallback; this.errorCallback = errorCallback; - this.validateRequest(id, permissions); + this.validateSharedLinkRequest(id, permissions); const { fields } = options; const requestData: RequestData = { diff --git a/src/api/__tests__/Item.test.js b/src/api/__tests__/Item.test.js index d3e533f6f5..85a96305dc 100644 --- a/src/api/__tests__/Item.test.js +++ b/src/api/__tests__/Item.test.js @@ -301,7 +301,7 @@ describe('api/Item', () => { }); }); - describe('validateRequest()', () => { + describe('validateSharedLinkRequest()', () => { const MOCK_ITEM_ERROR = new Error('missing item data'); const MOCK_PERMISSIONS_ERROR = new Error('missing permissions'); const MOCK_SUFFICIENT_PERMISSIONS = { @@ -319,7 +319,7 @@ describe('api/Item', () => { ${undefined} | ${MOCK_SUFFICIENT_PERMISSIONS} | ${'itemID is missing'} ${MOCK_ITEM_ID} | ${undefined} | ${'itemPermissions is missing'} `('should throw a bad item error if $description', ({ itemID, itemPermissions }) => { - expect(() => item.validateRequest(itemID, itemPermissions)).toThrowError(MOCK_ITEM_ERROR); + expect(() => item.validateSharedLinkRequest(itemID, itemPermissions)).toThrowError(MOCK_ITEM_ERROR); expect(item.errorCode).toBe(ERROR_CODE_SHARE_ITEM); }); @@ -329,24 +329,26 @@ describe('api/Item', () => { ${{ can_share: false, can_set_share_access: true }} | ${'set share access but not share'} ${{ can_share: true, can_set_share_access: false }} | ${'share but not set share access'} `('should throw a bad permissions error when the user can $description', ({ itemPermissions }) => { - expect(() => item.validateRequest(MOCK_ITEM_ID, itemPermissions)).toThrowError(MOCK_PERMISSIONS_ERROR); + expect(() => item.validateSharedLinkRequest(MOCK_ITEM_ID, itemPermissions)).toThrowError( + MOCK_PERMISSIONS_ERROR, + ); expect(item.errorCode).toBe(ERROR_CODE_SHARE_ITEM); }); test('should not throw an error if the request is valid', () => { - expect(() => item.validateRequest(MOCK_ITEM_ID, MOCK_SUFFICIENT_PERMISSIONS)).not.toThrow(); + expect(() => item.validateSharedLinkRequest(MOCK_ITEM_ID, MOCK_SUFFICIENT_PERMISSIONS)).not.toThrow(); expect(item.errorCode).toBeUndefined(); }); - test('should skip `can_set_share_access` check when canSkipSetShareAccess is true', () => { + test('should skip `can_set_share_access` check when canSkipSetShareAccessPermission is true', () => { const itemPermissions = { can_share: true, can_set_share_access: false }; - expect(() => item.validateRequest(MOCK_ITEM_ID, itemPermissions, true)).not.toThrow(); + expect(() => item.validateSharedLinkRequest(MOCK_ITEM_ID, itemPermissions, true)).not.toThrow(); expect(item.errorCode).toBeUndefined(); }); - test('should not skip `can_set_share_access` check when canSkipSetShareAccess is false', () => { + test('should not skip `can_set_share_access` check when canSkipSetShareAccessPermission is false', () => { const itemPermissions = { can_share: true, can_set_share_access: false }; - expect(() => item.validateRequest(MOCK_ITEM_ID, itemPermissions, false)).toThrowError( + expect(() => item.validateSharedLinkRequest(MOCK_ITEM_ID, itemPermissions, false)).toThrowError( MOCK_PERMISSIONS_ERROR, ); expect(item.errorCode).toBe(ERROR_CODE_SHARE_ITEM); @@ -357,7 +359,7 @@ describe('api/Item', () => { let shareSuccessHandlerSpy; let errorHandlerSpy; let getUrlSpy; - let validateRequestSpy; + let validateSharedLinkRequestSpy; const MOCK_DATA = { shared_link: '', permissions: {} }; beforeEach(() => { file = { @@ -370,7 +372,7 @@ describe('api/Item', () => { shareSuccessHandlerSpy = jest.spyOn(item, 'shareSuccessHandler'); errorHandlerSpy = jest.spyOn(item, 'errorHandler'); getUrlSpy = jest.spyOn(item, 'getUrl').mockReturnValue('url'); - validateRequestSpy = jest.spyOn(item, 'validateRequest').mockReturnValue(null); + validateSharedLinkRequestSpy = jest.spyOn(item, 'validateSharedLinkRequest').mockReturnValue(null); jest.spyOn(item, 'getCache').mockImplementation(() => ({ get: jest.fn().mockReturnValue('success'), has: jest.fn().mockReturnValue(false), @@ -391,7 +393,7 @@ describe('api/Item', () => { describe('with missing data', () => { const MOCK_MISSING_DATA_ERROR = new Error('missing data'); beforeEach(() => { - validateRequestSpy.mockImplementation(() => { + validateSharedLinkRequestSpy.mockImplementation(() => { throw MOCK_MISSING_DATA_ERROR; }); }); @@ -503,7 +505,7 @@ describe('api/Item', () => { }; shareSuccessHandlerSpy = jest.spyOn(item, 'shareSuccessHandler'); getUrlSpy = jest.spyOn(item, 'getUrl').mockReturnValue('url'); - jest.spyOn(item, 'validateRequest').mockReturnValue(null); + jest.spyOn(item, 'validateSharedLinkRequest').mockReturnValue(null); jest.spyOn(item, 'getCache').mockImplementation(() => ({ get: jest.fn().mockReturnValue('success'), has: jest.fn().mockReturnValue(false), diff --git a/src/elements/content-explorer/ContentExplorer.js b/src/elements/content-explorer/ContentExplorer.js index 40b9c07ac6..a86fd18ad5 100644 --- a/src/elements/content-explorer/ContentExplorer.js +++ b/src/elements/content-explorer/ContentExplorer.js @@ -818,7 +818,7 @@ class ContentExplorer extends Component { }; /** - * Chages the sort by and sort direction + * Changes the sort by and sort direction * * @private * @param {string} sortBy - field to sort by @@ -1275,33 +1275,23 @@ class ContentExplorer extends Component { * @param {Object} item file or folder * @returns {void} */ - handleSharedLinkSuccess = (newItem: BoxItem) => { + handleSharedLinkSuccess = (item: BoxItem) => { const { currentCollection } = this.state; - if (!newItem[FIELD_SHARED_LINK]) { - const { canSetShareAccess }: Props = this.props; - if (!newItem || !canSetShareAccess) { - return; - } - - const { permissions, type } = newItem; - if (!permissions || !type) { - return; - } - + if (item.type && !item[FIELD_SHARED_LINK]) { // create a shared link with default access, and update the collection const access = undefined; - this.api.getAPI(type).share(newItem, access, (updatedItem: BoxItem) => { + this.api.getAPI(item.type).share(item, access, (updatedItem: BoxItem) => { this.updateCollection(currentCollection, updatedItem, () => this.setState({ isShareModalOpen: true })); }); } else { // update collection with existing shared link - this.updateCollection(currentCollection, newItem, () => this.setState({ isShareModalOpen: true })); + this.updateCollection(currentCollection, item, () => this.setState({ isShareModalOpen: true })); } }; /** - * Chages the sort by and sort direction + * Callback for sharing an item * * @private * @return {void} diff --git a/src/elements/content-explorer/__tests__/ContentExplorer.test.js b/src/elements/content-explorer/__tests__/ContentExplorer.test.js index 4a2543a04a..9039cf34ef 100644 --- a/src/elements/content-explorer/__tests__/ContentExplorer.test.js +++ b/src/elements/content-explorer/__tests__/ContentExplorer.test.js @@ -560,7 +560,7 @@ describe('elements/content-explorer/ContentExplorer', () => { }); describe('handleSharedLinkSuccess()', () => { - const getApiShareMock = jest.fn().mockImplementation((newItem, access, callback) => callback()); + const getApiShareMock = jest.fn().mockImplementation((item, access, callback) => callback()); const getApiMock = jest.fn().mockReturnValue({ share: getApiShareMock }); const updateCollectionMock = jest.fn(); @@ -581,8 +581,8 @@ describe('elements/content-explorer/ContentExplorer', () => { }); afterEach(() => { - getApiShareMock.mockClear(); getApiMock.mockClear(); + getApiShareMock.mockClear(); updateCollectionMock.mockClear(); }); diff --git a/src/elements/content-picker/ContentPicker.js b/src/elements/content-picker/ContentPicker.js index da70605b04..2cac3afa15 100644 --- a/src/elements/content-picker/ContentPicker.js +++ b/src/elements/content-picker/ContentPicker.js @@ -9,7 +9,6 @@ import React, { Component } from 'react'; import type { Node } from 'react'; import classNames from 'classnames'; import debounce from 'lodash/debounce'; -import getProp from 'lodash/get'; import uniqueid from 'lodash/uniqueId'; import noop from 'lodash/noop'; import Header from '../common/header'; @@ -850,7 +849,6 @@ class ContentPicker extends Component { const existing: BoxItem = selected[cacheKey]; const existingFromCache: BoxItem = this.api.getCache().get(cacheKey); const existInSelected = selectedKeys.indexOf(cacheKey) !== -1; - const itemCanSetShareAccess = getProp(item, 'permissions.can_set_share_access', false); // Existing object could have mutated and we just need to update the // reference in the selected map. In that case treat it like a new selection. @@ -863,7 +861,7 @@ class ContentPicker extends Component { // We are selecting a new item that was never // selected before. However if we are in a single // item selection mode, we should also unselect any - // prior item that was item that was selected. + // prior item that was selected. // Check if we hit the selection limit and if selection // is not already currently in the selected data structure. @@ -886,7 +884,7 @@ class ContentPicker extends Component { // If can set share access, fetch the shared link properties of the item // In the case where another item is selected, any in flight XHR will get // cancelled - if (canSetShareAccess && itemCanSetShareAccess && forceSharedLink) { + if (canSetShareAccess && forceSharedLink) { this.fetchSharedLinkInfo(item); } } @@ -939,8 +937,7 @@ class ContentPicker extends Component { handleSharedLinkSuccess = (item: BoxItem) => { // if no shared link currently exists, create a shared link with enterprise default if (!item[FIELD_SHARED_LINK]) { - // $FlowFixMe - this.changeShareAccess(null, item); + this.changeShareAccess(undefined, item); } else { const { selected } = this.state; const { id, type } = item; @@ -962,7 +959,7 @@ class ContentPicker extends Component { * @param {Object} item file or folder object * @return {void} */ - changeShareAccess = (access: Access, item: BoxItem): void => { + changeShareAccess = (access?: Access, item: BoxItem): void => { const { canSetShareAccess }: Props = this.props; if (!item || !canSetShareAccess) { return; @@ -974,7 +971,7 @@ class ContentPicker extends Component { } const { can_set_share_access }: BoxItemPermission = permissions; - if (!can_set_share_access) { + if (access !== undefined && !can_set_share_access) { return; } @@ -1005,7 +1002,7 @@ class ContentPicker extends Component { }; /** - * Chages the sort by and sort direction + * Changes the sort by and sort direction * * @private * @param {string} sortBy - field to sorty by diff --git a/src/elements/content-sharing/hooks/useSharedLink.js b/src/elements/content-sharing/hooks/useSharedLink.js index 06e56dacd4..bf143d0f80 100644 --- a/src/elements/content-sharing/hooks/useSharedLink.js +++ b/src/elements/content-sharing/hooks/useSharedLink.js @@ -122,6 +122,7 @@ function useSharedLink( // Shared link access level change function const updatedChangeSharedLinkAccessLevelFn: SharedLinkUpdateLevelFnType = () => (newAccessLevel: string) => connectToItemShare({ + // $FlowFixMe access: transformAccess(newAccessLevel), successFn: data => { currentAccessLevel.current = newAccessLevel; diff --git a/src/elements/content-sharing/types.js b/src/elements/content-sharing/types.js index 90b7540b7b..8181e28ddf 100644 --- a/src/elements/content-sharing/types.js +++ b/src/elements/content-sharing/types.js @@ -1,5 +1,6 @@ // @flow import type { + Access, BoxItemClassification, BoxItemPermission, Collaboration, @@ -137,7 +138,7 @@ export type GetContactsByEmailFnType = () => (filterTerm: { export type SendInvitesFnType = () => InviteCollaboratorsRequest => Promise>; export type ConnectToItemShareFnType = ({ - access?: string, + access?: Access, errorFn?: Function, requestOptions?: RequestOptions, successFn?: Function, diff --git a/src/utils/fields.js b/src/utils/fields.js index 3e9a9bb49b..c4c1b971f4 100644 --- a/src/utils/fields.js +++ b/src/utils/fields.js @@ -174,7 +174,7 @@ const FILE_VERSIONS_FIELDS_TO_FETCH = [ FIELD_VERSION_NUMBER, ]; -// Fields needed for Content Picker to show shared link permissions +// Fields needed to show shared link permissions const FILE_SHARED_LINK_FIELDS_TO_FETCH = [FIELD_ALLOWED_SHARED_LINK_ACCESS_LEVELS, FIELD_SHARED_LINK]; // Fields needed to get tasks data