fix(contentpicker): correctly clear old item when maxSelectable=1 and parent reloaded

src/elements/content-picker/Content.js

@@ -27,6 +27,7 @@ type Props = {
     rootElement?: HTMLElement,
     rootId: string,
     selectableType: string,
+    selected: Array<BoxItem>,
     tableRef: Function,
     view: View,
 };
@@ -60,6 +61,7 @@ const Content = ({
     onShareAccessChange,
     onFocusChange,
     extensionsWhitelist,
+    selected,
 }: Props) => (
     <div className="bcp-content">
         {view === VIEW_ERROR || view === VIEW_SELECTED ? null : (
@@ -85,6 +87,7 @@ const Content = ({
                 onFocusChange={onFocusChange}
                 onShareAccessChange={onShareAccessChange}
                 extensionsWhitelist={extensionsWhitelist}
+                selected={selected}
             />
         )}
     </div>

src/elements/content-picker/ContentPicker.js

@@ -19,6 +19,7 @@ import Internationalize from '../common/Internationalize';
 import makeResponsive from '../common/makeResponsive';
 import Pagination from '../common/pagination/Pagination';
 import { isFocusableElement, isInputElement, focus } from '../../utils/dom';
+import { isSelected, containsItem } from './itemSelectionHelper';
 import API from '../../api';
 import Content from './Content';
 import Footer from './Footer';
@@ -102,7 +103,7 @@ type State = {
     isUploadModalOpen: boolean,
     rootName: string,
     searchQuery: string,
-    selected: { [string]: BoxItem },
+    selected: Array<BoxItem>,
     sortBy: SortBy,
     sortDirection: SortDirection,
     view: View,
@@ -199,7 +200,7 @@ class ContentPicker extends Component<Props, State> {
             currentCollection: {},
             currentOffset: initialPageSize * (initialPage - 1),
             currentPageSize: initialPageSize,
-            selected: {},
+            selected: [],
             searchQuery: '',
             view: VIEW_FOLDER,
             isCreateFolderModalOpen: false,
@@ -287,11 +288,10 @@ class ContentPicker extends Component<Props, State> {
     choose = (): void => {
         const { selected }: State = this.state;
         const { onChoose }: Props = this.props;
-        const results: BoxItem[] = Object.keys(selected).map(key => {
-            const clone: BoxItem = { ...selected[key] };
-            delete clone.selected;
-            return clone;
+        const results: BoxItem[] = selected.map(selectedItem => {
+            return { ...selectedItem };
         });
+
         onChoose(results);
     };
 
@@ -304,13 +304,9 @@ class ContentPicker extends Component<Props, State> {
      */
     cancel = (): void => {
         const { onCancel }: Props = this.props;
-        const { selected }: State = this.state;
-
-        // Clear out the selected field
-        Object.keys(selected).forEach(key => delete selected[key].selected);
 
         // Reset the selected state
-        this.setState({ selected: {} }, () => onCancel());
+        this.setState({ selected: [] }, () => onCancel());
     };
 
     /**
@@ -563,7 +559,7 @@ class ContentPicker extends Component<Props, State> {
                     sortBy,
                     sortDirection,
                     percentLoaded: 100,
-                    items: Object.keys(selected).map(key => this.api.getCache().get(key)),
+                    items: selected,
                 },
             },
             this.finishNavigation,
@@ -782,46 +778,24 @@ class ContentPicker extends Component<Props, State> {
             return;
         }
 
-        const selectedKeys: Array<string> = Object.keys(selected);
-        const selectedCount: number = selectedKeys.length;
-        const hasHitSelectionLimit: boolean = selectedCount === maxSelectable;
         const isSingleFileSelection: boolean = maxSelectable === 1;
-        const cacheKey: string = this.api.getAPI(type).getCacheKey(id);
-        const existing: BoxItem = selected[cacheKey];
-        const existingFromCache: BoxItem = this.api.getCache().get(cacheKey);
-        const existInSelected = selectedKeys.indexOf(cacheKey) !== -1;
+        const newSelected: Array<BoxItem> = isSingleFileSelection ? [] : [...selected];
+        const selectedCount: number = newSelected.length;
+        const hasHitSelectionLimit: boolean = selectedCount === maxSelectable;
+        const existingIndex: number = newSelected.findIndex(containsItem(item));
+        const isAlreadySelected = existingIndex !== -1 || (isSingleFileSelection && isSelected(item, selected));
         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.
-        if (existing && existing === existingFromCache) {
+        if (isAlreadySelected) {
             // We are selecting the same item that was already
             // selected. Unselect it in this case. Toggle case.
-            delete existing.selected;
-            delete selected[cacheKey];
+            newSelected.splice(existingIndex, 1);
         } else {
-            // 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.
-
-            // Check if we hit the selection limit and if selection
-            // is not already currently in the selected data structure.
-            // Ignore when in single file selection mode.
-            if (hasHitSelectionLimit && !isSingleFileSelection && !existInSelected) {
+            if (hasHitSelectionLimit) {
                 return;
             }
 
-            // Clear out the prior item for single file selection mode
-            if (selectedCount > 0 && isSingleFileSelection) {
-                const prior = selectedKeys[0]; // only one item
-                delete selected[prior].selected;
-                delete selected[prior];
-            }
-
-            // Select the new item
-            item.selected = true;
-            selected[cacheKey] = item;
+            newSelected.push(item);
 
             // 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
@@ -832,7 +806,7 @@ class ContentPicker extends Component<Props, State> {
         }
 
         const focusedRow = items.findIndex((i: BoxItem) => i.id === item.id);
-        this.setState({ selected, focusedRow }, () => {
+        this.setState({ selected: newSelected, focusedRow }, () => {
             if (view === VIEW_SELECTED) {
                 // Need to refresh the selected view
                 this.showSelected();
@@ -881,14 +855,8 @@ class ContentPicker extends Component<Props, State> {
         if (!item[FIELD_SHARED_LINK]) {
             this.changeShareAccess(null, item);
         } else {
-            const { selected } = this.state;
-            const { id, type } = item;
-            const cacheKey = this.api.getAPI(type).getCacheKey(id);
             // if shared link already exists, update the collection in state
             this.updateItemInCollection(item);
-            if (item.selected && item !== selected[cacheKey]) {
-                this.select(item, { forceSharedLink: false });
-            }
         }
     };
 
@@ -918,9 +886,6 @@ class ContentPicker extends Component<Props, State> {
 
         this.api.getAPI(type).share(item, access, (updatedItem: BoxItem) => {
             this.updateItemInCollection(updatedItem);
-            if (item.selected) {
-                this.select(updatedItem, { forceSharedLink: false });
-            }
         });
     };
 
@@ -1152,7 +1117,7 @@ class ContentPicker extends Component<Props, State> {
         }: State = this.state;
         const { id, offset, permissions, totalCount }: Collection = currentCollection;
         const { can_upload }: BoxItemPermission = permissions || {};
-        const selectedCount: number = Object.keys(selected).length;
+        const { length: selectedCount } = selected;
         const isSingleSelect = maxSelectable === 1;
         const hasHitSelectionLimit: boolean = selectedCount === maxSelectable && !isSingleSelect;
         const allowUpload: boolean = canUpload && !!can_upload;
@@ -1202,6 +1167,7 @@ class ContentPicker extends Component<Props, State> {
                             onItemClick={this.onItemClick}
                             onFocusChange={this.onFocusChange}
                             onShareAccessChange={this.changeShareAccess}
+                            selected={selected}
                         />
                         <Footer
                             selectedCount={selectedCount}

src/elements/content-picker/ItemList.js

@@ -15,6 +15,7 @@ import { isFocusableElement, focus } from '../../utils/dom';
 import shareAccessCellRenderer from './shareAccessCellRenderer';
 import selectionCellRenderer from './selectionCellRenderer';
 import isRowSelectable from './cellRendererHelper';
+import { isSelected as isRowSelected } from './itemSelectionHelper';
 import { VIEW_SELECTED, FIELD_NAME, FIELD_ID, FIELD_SHARED_LINK, TYPE_FOLDER } from '../../constants';
 
 import './ItemList.scss';
@@ -34,6 +35,7 @@ type Props = {
     rootElement?: HTMLElement,
     rootId: string,
     selectableType: string,
+    selected: Array<BoxItem>,
     tableRef: Function,
     view: View,
 };
@@ -54,6 +56,7 @@ const ItemList = ({
     onShareAccessChange,
     onFocusChange,
     currentCollection,
+    selected,
     tableRef,
 }: Props) => {
     const iconCell = iconCellRenderer();
@@ -64,13 +67,15 @@ const ItemList = ({
         extensionsWhitelist,
         hasHitSelectionLimit,
         isSingleSelect,
+        selected,
     );
     const shareAccessCell = shareAccessCellRenderer(
         onShareAccessChange,
         canSetShareAccess,
         selectableType,
         extensionsWhitelist,
         hasHitSelectionLimit,
+        selected,
     );
     const { id, items = [] }: Collection = currentCollection;
     const rowCount: number = items.length;
@@ -80,10 +85,18 @@ const ItemList = ({
             return '';
         }
 
-        const { selected, type } = items[index];
-        const isSelectable = isRowSelectable(selectableType, extensionsWhitelist, hasHitSelectionLimit, items[index]);
+        const { type } = items[index];
+        const isSelected = !!isRowSelected(items[index], selected);
+        const isSelectable = isRowSelectable(
+            selectableType,
+            extensionsWhitelist,
+            hasHitSelectionLimit,
+            items[index],
+            isSelected,
+        );
+
         return classNames(`bcp-item-row bcp-item-row-${index}`, {
-            'bcp-item-row-selected': selected && view !== VIEW_SELECTED,
+            'bcp-item-row-selected': isSelected && view !== VIEW_SELECTED,
             'bcp-item-row-unselectable': type !== TYPE_FOLDER && !isSelectable, // folder row should never dim
         });
     };
@@ -97,9 +110,11 @@ const ItemList = ({
         index: number,
         rowData: BoxItem,
     }) => {
+        const isSelected = !!isRowSelected(rowData, selected);
+
         // If the click is happening on a clickable element on the item row, ignore row selection
         if (
-            isRowSelectable(selectableType, extensionsWhitelist, hasHitSelectionLimit, rowData) &&
+            isRowSelectable(selectableType, extensionsWhitelist, hasHitSelectionLimit, rowData, isSelected) &&
             !isFocusableElement(event.target)
         ) {
             onItemSelect(rowData);

src/elements/content-picker/__tests__/cellRendererHelper.test.js

@@ -2,50 +2,74 @@ import isRowSelectable from '../cellRendererHelper';
 
 describe('picker/components/cellRendererHelper/isRowSelectable()', () => {
     test('should return true when folder picker and type is folder', () => {
-        expect(isRowSelectable('folder', [], false, { type: 'folder' })).toBeTruthy();
+        expect(isRowSelectable('folder', [], false, { type: 'folder' }, false)).toBeTruthy();
     });
     test('should return false when folder picker and type is file', () => {
         expect(
-            isRowSelectable('folder', [], false, {
-                type: 'file',
-                extension: 'doc',
-            }),
+            isRowSelectable(
+                'folder',
+                [],
+                false,
+                {
+                    type: 'file',
+                    extension: 'doc',
+                },
+                false,
+            ),
         ).toBeFalsy();
     });
     test('should return true when file picker and type is file with no whitelist extensions', () => {
         expect(
-            isRowSelectable('file', [], false, {
-                type: 'file',
-                extension: 'doc',
-            }),
+            isRowSelectable(
+                'file',
+                [],
+                false,
+                {
+                    type: 'file',
+                    extension: 'doc',
+                },
+                false,
+            ),
         ).toBeTruthy();
     });
     test('should return false when file picker and type is folder', () => {
-        expect(isRowSelectable('file', [], false, { type: 'folder' })).toBeFalsy();
+        expect(isRowSelectable('file', [], false, { type: 'folder' }, false)).toBeFalsy();
     });
     test('should return true when file picker and type is file and extension is whitelisted', () => {
         expect(
-            isRowSelectable('file', ['doc'], false, {
-                type: 'file',
-                extension: 'doc',
-            }),
+            isRowSelectable(
+                'file',
+                ['doc'],
+                false,
+                {
+                    type: 'file',
+                    extension: 'doc',
+                },
+                false,
+            ),
         ).toBeTruthy();
     });
     test('should return true when file picker and type is file and extension is not whitelisted', () => {
         expect(
-            isRowSelectable('file', ['ppt'], false, {
-                type: 'file',
-                extension: 'doc',
-            }),
+            isRowSelectable(
+                'file',
+                ['ppt'],
+                false,
+                {
+                    type: 'file',
+                    extension: 'doc',
+                },
+                false,
+            ),
         ).toBeFalsy();
     });
     test('should return false when selection limit has reached and item is not selected', () => {
-        expect(isRowSelectable('file', [], true, { type: 'file' })).toBeFalsy();
+        expect(isRowSelectable('file', [], true, { type: 'file' }, false)).toBeFalsy();
     });
     test('should return false when selection limit has not reached', () => {
-        expect(isRowSelectable('file', [], true, { type: 'file' })).toBeFalsy();
+        expect(isRowSelectable('file', [], true, { type: 'file' }, false)).toBeFalsy();
     });
     test('should return true when selection limit has reached and item is selected', () => {
-        expect(isRowSelectable('file', [], true, { type: 'file', selected: true })).toBeTruthy();
+        expect(isRowSelectable('file', [], true, { type: 'file' }, true)).toBeTruthy();
     });
 });

src/elements/content-picker/__tests__/selectionCellRenderer.test.js

@@ -3,21 +3,28 @@ import { shallow } from 'enzyme';
 import selectionCellRenderer from '../selectionCellRenderer';
 
 const rowData = {
+    id: '51965506671',
     name: 'test',
-    selected: true,
     type: 'file',
 };
 
+const selected = [
+    {
+        id: '51965506671',
+        type: 'file',
+    },
+];
+
 describe('selectionCellRenderer', () => {
     test.each([['Checkbox', false], ['RadioButton', true]])('should render %s if isRadio is %s', (type, isRadio) => {
-        const Element = selectionCellRenderer(() => {}, 'file, web_link', [], false, isRadio);
+        const Element = selectionCellRenderer(() => {}, 'file, web_link', [], false, isRadio, selected);
 
         const wrapper = shallow(<Element rowData={rowData} />);
         expect(wrapper.exists(type)).toBe(true);
     });
 
     test.each([['isSelected', true], ['isChecked', false]])('should render %s if isRadio is %s', (type, isRadio) => {
-        const Element = selectionCellRenderer(() => {}, 'file, web_link', [], false, isRadio);
+        const Element = selectionCellRenderer(() => {}, 'file, web_link', [], false, isRadio, selected);
 
         const wrapper = shallow(<Element rowData={rowData} />);
         expect(wrapper.prop(type)).toBe(true);