Skip to content

Commit

Permalink
Tweaks & fixes related to the trashed collections feature
Browse files Browse the repository at this point in the history
* Tweak the item details pane message to reflect when collections are
  selected.
* When trashing, recovering, or permanently deleting collections, also
  apply these actions to all descendants.
* Add tests for trash-related functionality.
  • Loading branch information
tnajdek committed Sep 14, 2024
1 parent efbdbf1 commit f3a3f66
Show file tree
Hide file tree
Showing 29 changed files with 312 additions and 56 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ node_modules
coverage
visualizer
.nyc_output
.secret.json

src/static/pdf-reader/
src/static/pdf-worker/
Expand Down
1 change: 1 addition & 0 deletions scripts/generate-fixtures.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ const fixtures = [
[`${URL}testuser/search/retriever/titleCreatorYear/items/KBFTPTI4/item-list`, 'desktop-test-user-search-phrase-selected'],
[`${URL}testuser/tags/to%20read/search/pathfinding/titleCreatorYear/items/J489T6X3,3JCLFUG4/item-list`, 'desktop-test-user-search-selected'],
[`${URL}testuser/collections/CSB4KZUU/items/3JCLFUG4/attachment/37V7V4NT/item-details`, 'desktop-test-user-attachment-in-collection-view'],
[`${URL}testuser/trash`, 'desktop-test-user-trash-view'],
[`${URL}testuser/collections/CSB4KZUU/items/3JCLFUG4/item-details`, 'mobile-test-user-item-details-view'],
[`${URL}testuser/collections/WTTJ2J56/item-list`, 'mobile-test-user-item-list-view']
];
Expand Down
33 changes: 29 additions & 4 deletions src/js/actions/collections.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
import { requestTracker, requestWithBackoff } from '.';
import { cede, get } from '../utils';
import { chunkedAction } from '../common/actions';
import { makeChildMap, getDescendants } from '../common/collection';

const rescheduleBadRequests = (badRequestsArray, allRequestsArray, dispatch, libraryKey, args) => {
let hasScheduledNewRequest = false;
Expand Down Expand Up @@ -361,9 +362,21 @@ const deleteCollection = (collection, libraryKey) => {
};
}

const deleteCollections = (collectionKeys, libraryKey) => {
return async dispatch => {
const deleteCollections = (collectionKeys, libraryKey, recursively = true) => {
return async (dispatch, getState) => {
const id = requestTracker.id++;
const state = getState();
const collections = state.libraries[libraryKey].collections.keys.map(key => state.libraries[libraryKey].dataObjects[key]);

if (recursively) {
// at this point child map should be memoized
const childMap = makeChildMap(collections);
const childKeys = [];
for(const key of collectionKeys) {
childKeys.push(...getDescendants(key, childMap));
}
collectionKeys.push(...childKeys);
}

dispatch({
type: PRE_DELETE_COLLECTIONS,
Expand Down Expand Up @@ -426,14 +439,26 @@ const queueDeleteCollections = (collectionKeys, libraryKey, id, resolve, reject)
};
}

const updateCollectionsTrash = (collectionKeys, libraryKey, deleted) => {
return async dispatch => {
const updateCollectionsTrash = (collectionKeys, libraryKey, deleted, recursively = true) => {
return async (dispatch, useState) => {
const id = requestTracker.id++;
const state = useState();
const collections = state.libraries[libraryKey].collections.keys.map(key => state.libraries[libraryKey].dataObjects[key]);

if (deleted !== 0 && deleted !== 1) {
throw new Error('deleted must be 0 or 1');
}

if (recursively) {
// at this point child map should be memoized
const childMap = makeChildMap(collections);
const childKeys = [];
for (const key of collectionKeys) {
childKeys.push(...getDescendants(key, childMap));
}
collectionKeys.push(...childKeys);
}

dispatch({
type: PRE_UPDATE_COLLECTIONS_TRASH,
collectionKeys,
Expand Down
8 changes: 7 additions & 1 deletion src/js/actions/current.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { omit } from 'web-common/utils';
import { getApiForItems, splitItemAndCollectionKeys } from '../common/actions';
import { exportItems, chunkedToggleTagsOnItems, chunkedAddToCollection, chunkedCopyToLibrary,
chunkedDeleteItems, chunkedMoveItemsToTrash, chunkedRecoverItemsFromTrash,
chunkedRemoveFromCollection, chunkedUpdateCollectionsTrash, chunkedDeleteCollections, createItem, createItemOfType, toggleModal } from '.';
chunkedRemoveFromCollection, chunkedUpdateCollectionsTrash, chunkedDeleteCollections, createItem, createItemOfType, toggleModal, navigate } from '.';
import columnProperties from '../constants/column-properties';
import { BIBLIOGRAPHY, COLLECTION_SELECT, EXPORT, NEW_FILE, NEW_ITEM } from '../constants/modals';
import { TOGGLE_ADD, TOGGLE_REMOVE } from '../common/tags';
Expand Down Expand Up @@ -106,6 +106,9 @@ const currentRecoverFromTrash = () => {
const state = getState();
const { itemKeys: keys, libraryKey } = state.current;
const { itemKeys, collectionKeys } = splitItemAndCollectionKeys(keys, libraryKey, state);

// unselect items before recovering them to avoid needless request for item details if one item is selected
await dispatch(navigate({ items: [] }));
const itemsPromise = dispatch(chunkedRecoverItemsFromTrash(itemKeys));
const collectionsPromise = dispatch(chunkedUpdateCollectionsTrash(collectionKeys, libraryKey, 0));
return await Promise.all([itemsPromise, collectionsPromise]);
Expand All @@ -117,6 +120,9 @@ const currentDeletePermanently = () => {
const state = getState();
const { itemKeys: keys, libraryKey } = state.current;
const { itemKeys, collectionKeys } = splitItemAndCollectionKeys(keys, libraryKey, state);

// unselect items before deleting them to avoid needless request for item details if one item is selected
await dispatch(navigate({ items: [] }));
const itemsPromise = await dispatch(chunkedDeleteItems(itemKeys));
const collectionsPromise = await dispatch(chunkedDeleteCollections(collectionKeys, libraryKey));
return await Promise.all([itemsPromise, collectionsPromise]);
Expand Down
13 changes: 12 additions & 1 deletion src/js/common/collection.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,15 @@ const _makeChildMap = collections => {

const makeChildMap = memoize(_makeChildMap, deepEqual);

export { makeChildMap };
const getDescendants = (collectionKey, childMap, keys = []) => {
if(!(collectionKey in childMap)) {
return keys;
}
for(let childKey of childMap[collectionKey]) {
keys.push(childKey);
getDescendants(childKey, childMap, keys);
}
return keys;
}

export { makeChildMap, getDescendants };
4 changes: 2 additions & 2 deletions src/js/common/reducers.js
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ const injectTrashedCollections = (itemsState, collectionsState, dataObjectsState
trashedCollections = trashedCollections.filter(collection => !trashedCollectionKeys.has(collection.parentCollection));

if (!('keys' in itemsState) || itemsState.keys.length === 0) {
return { ...itemsState, keys: trashedCollections.map(collection => collection.key), injectPoints: trashedCollections.map((_, i) => i) };
return { ...itemsState, keys: trashedCollections.map(collection => collection.key), injectPoints: trashedCollections.map((_, i) => i), totalCount: trashedCollections.length };
}

const keysTrash = [
Expand All @@ -298,7 +298,7 @@ const injectTrashedCollections = (itemsState, collectionsState, dataObjectsState

const indices = trashedCollections.map((collection) => keysTrash.indexOf(collection.key));

return { ...itemsState, keys: keysTrash, injectPoints: indices };
return { ...itemsState, keys: keysTrash, injectPoints: indices, totalCount: itemsState.totalResults + trashedCollections.length };
}

export {
Expand Down
42 changes: 21 additions & 21 deletions src/js/component/item-details/info-view.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,46 +2,46 @@ import { useEffect, useState } from 'react';
import { useSelector } from 'react-redux';
import { pluralize } from '../../common/format';
import { get } from '../../utils';
import { useSourcePath } from '../../hooks/';

const ItemDetailsInfoView = () => {
const itemKey = useSelector(state => state.current.itemKey);
const itemKeys = useSelector(state => state.current.itemKeys);
const objectKey = useSelector(state => state.current.itemKey);
const objectKeys = useSelector(state => state.current.itemKeys);
const dataObjects = useSelector(state => state.libraries[state.current.libraryKey]?.dataObjects) ?? [];
const isEmbedded = useSelector(state => state.config.isEmbedded);
const sourcePath = useSourcePath();
const isTrash = useSelector(state => state.current.isTrash);
const totalCount = useSelector(state => get(state, sourcePath, null)?.totalCount);
const totalResults = useSelector(state => get(state, sourcePath, null)?.totalResults);
const itemsCount = totalCount ?? totalResults ?? null;
const objects = objectKeys.map(key => dataObjects[key]);
const hasCollections = objects.some(obj => obj?.[Symbol.for('type')] === 'collection');
const hasItems = objects.some(obj => !obj || obj?.[Symbol.for('type')] === 'item');
const objectType = objects.length === 0 ?
(objectKeys.length === 0 && isTrash && totalCount !== totalResults ? 'object' : 'item') :
(hasCollections && hasItems) ? 'object' : hasCollections ? 'collection' : 'item';

const itemsCount = useSelector(state => {
const { collectionKey, libraryKey, itemsSource } = state.current;
switch(itemsSource) {
case 'query':
return state.query.totalResults || null;
case 'top':
return get(state, ['libraries', libraryKey, 'itemsTop', 'totalResults'], null);
case 'trash':
return get(state, ['libraries', libraryKey, 'itemsTrash', 'totalResults'], null);
case 'collection':
return get(state, ['libraries', libraryKey, 'itemsByCollection', collectionKey, 'totalResults'], null)
}
});
const [label, setLabel] = useState('');

useEffect(() => {
if(isEmbedded) {
setLabel('');
} else if(itemKeys.length > 0) {
} else if(objectKeys.length > 0) {
setLabel([
itemKeys.length,
pluralize('item', itemKeys.length),
objectKeys.length,
pluralize(objectType, objectKeys.length),
'selected'
].join(' '));
} else if(!itemKey && itemsCount !== null) {
} else if(!objectKey && itemsCount !== null) {
setLabel([
itemsCount === 0 ? 'No' : itemsCount,
pluralize('item', itemsCount),
pluralize(objectType, itemsCount),
'in this view'
].join(' '));
} else {
setLabel('');
}
}, [isEmbedded, itemsCount, itemKey, itemKeys]);
}, [isEmbedded, itemsCount, objectKey, objectKeys, objectType]);

return <div className="info-view">{ label }</div>;
}
Expand Down
13 changes: 5 additions & 8 deletions src/js/component/item/details.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import ItemDetailsInfoView from '../item-details/info-view';
import ItemDetailsTabs from '../item-details/tabs';
import TouchHeaderWrap from '../../component/touch-header-wrap';
import { navigate, fetchItemDetails } from '../../actions';
import { get } from '../../utils';

const ItemDetails = props => {
const { active = false } = props;
Expand All @@ -18,7 +17,7 @@ const ItemDetails = props => {
const itemKey = useSelector(state => state.current.itemKey);
const libraryKey = useSelector(state => state.current.libraryKey);
const isEmbedded = useSelector(state => state.config.isEmbedded);
const item = useSelector(state => get(state, ['libraries', libraryKey, 'dataObjects', itemKey], null));
const item = useSelector(state => state.libraries[libraryKey]?.dataObjects?.[itemKey]);
// collections are prefetched so if item is null, it's not a collection
const isCollection = item?.[Symbol.for('type')] === 'collection';
const shouldFetchItem = itemKey && !isCollection && !item;
Expand Down Expand Up @@ -60,12 +59,10 @@ const ItemDetails = props => {
/>
) }
{
isCollection ? null : (
(!isTouchOrSmall || (isTouchOrSmall && !isSelectMode)) && item && !shouldRedirectToParentItem ? (
<ItemDetailsTabs />
) : (
<ItemDetailsInfoView />
)
(!isCollection && !isTouchOrSmall || (isTouchOrSmall && !isSelectMode)) && item && !shouldRedirectToParentItem ? (
<ItemDetailsTabs />
) : (
<ItemDetailsInfoView />
)
}
</section>
Expand Down
2 changes: 1 addition & 1 deletion src/js/component/item/items/table-row.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ const TableRow = props => {

const itemData = useSelector(
state => itemKey ?
state.libraries[state.current.libraryKey].dataObjects?.[itemKey]?.[Symbol.for('derived')]
state.libraries[state.current.libraryKey]?.dataObjects?.[itemKey]?.[Symbol.for('derived')]
: null
);

Expand Down
4 changes: 2 additions & 2 deletions src/js/component/modal/rename-collection.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { Button, Icon } from 'web-common/components';
import Input from '../form/input';
import Modal from '../ui/modal';
import { COLLECTION_RENAME } from '../../constants/modals';
import { get, getUniqueId } from '../../utils';
import { getUniqueId } from '../../utils';
import { makeChildMap } from '../../common/collection';
import { toggleModal, updateCollection } from '../../actions';

Expand All @@ -17,7 +17,7 @@ const RenameCollectionModal = () => {
state => state.libraries[state.current.libraryKey]?.dataObjects[state.modal.collectionKey], shallowEqual
);
const collections = useSelector(
state => state.libraries[libraryKey]?.collections?.keys.map(key => state.libraries[libraryKey].dataObjects[key]), shallowEqual
state => state.libraries[libraryKey]?.collections?.keys.map(key => state.libraries[libraryKey]?.dataObjects[key]), shallowEqual
);
const isOpen = useSelector(state => state.modal.id === COLLECTION_RENAME);
const isTouchOrSmall = useSelector(state => state.device.isTouchOrSmall);
Expand Down
2 changes: 1 addition & 1 deletion src/js/hooks/use-fetching-state.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,4 +175,4 @@ const useSourceSignature = () => {
}
}

export { useFetchingState, useSourceSignature, useSourceData, useSourceKeys, useTags };
export { useFetchingState, useSourceSignature, useSourceData, useSourceKeys, useSourcePath, useTags };
3 changes: 2 additions & 1 deletion src/js/reducers/current.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ const stateDefault = {
isSelectMode: false,
isTagSelectorOpen: true,
isTouchTagSelectorOpen: false,
//NOTE: After #547 itemKey(s) might also contain collection keys
//NOTE: After #547 itemKey(s) might also contain collection keys.
//TODO: Rename to objectKey(s)
itemKey: null,
itemKeys: [],
itemsSource: null,
Expand Down
2 changes: 1 addition & 1 deletion src/js/reducers/libraries/data-objects.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ const dataObjects = (state = {}, action, { meta, tagColors }) => {
case RECEIVE_TRASH_ITEMS:
return {
...state,
...discardIfOldVersion(indexByKey(processItem(action.items, { meta, tagColors }), 'key'), state)
...discardIfOldVersion(indexByKey(action.items, 'key', item => processItem(item, { meta, tagColors })), state)
};
case RECEIVE_UPLOAD_ATTACHMENT:
return {
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/state/desktop-test-group-item-view.json

Large diffs are not rendered by default.

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion test/fixtures/state/desktop-test-user-attachment-view.json

Large diffs are not rendered by default.

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion test/fixtures/state/desktop-test-user-item-view.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion test/fixtures/state/desktop-test-user-library-view.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion test/fixtures/state/desktop-test-user-note-view.json

Large diffs are not rendered by default.

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion test/fixtures/state/desktop-test-user-reader-view.json

Large diffs are not rendered by default.

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion test/fixtures/state/desktop-test-user-search-selected.json

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions test/fixtures/state/desktop-test-user-trash-view.json

Large diffs are not rendered by default.

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion test/fixtures/state/mobile-test-user-item-list-view.json

Large diffs are not rendered by default.

Loading

0 comments on commit f3a3f66

Please sign in to comment.