Skip to content

Commit

Permalink
Fix issues with copying items across libraries. Fix #550.
Browse files Browse the repository at this point in the history 
* Skip linked file attachments when copying to a group library.
* Accept server version from a partially failed request (e.g., when some
  items fail to create or update).
* Do not send explicit library version when creating multiple items.
* Fix potential race condition in reader tests that became more apparent
  due to other changes in this commit.
  • Loading branch information
@tnajdek
tnajdek committed Aug 5, 2024
1 parent c09391d commit 1a77c52
Show file tree
Hide file tree
Showing 3 changed files with 101 additions and 127 deletions.
209 changes: 88 additions & 121 deletions src/js/actions/items-write.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,12 @@ import {
REQUEST_UPLOAD_ATTACHMENT,
} from '../constants/actions';

class PartialWriteError extends Error {
constructor(message, response) {
super(message);
this.response = response;
}
}

const postItemsMultiPatch = async (state, multiPatch, libraryKey = null) => {
libraryKey = libraryKey ?? state.current.libraryKey;
Expand Down Expand Up @@ -147,7 +153,6 @@ const queueCreateItems = (items, libraryKey, id, resolve, reject) => {
callback: async (next, dispatch, getState) => {
const state = getState();
const config = state.config;
const version = state.libraries[libraryKey].sync.version;

dispatch({
type: REQUEST_CREATE_ITEMS,
Expand All @@ -158,13 +163,12 @@ const queueCreateItems = (items, libraryKey, id, resolve, reject) => {

try {
let response = await api(config.apiKey, config.apiConfig)
.version(version)
.library(libraryKey)
.items()
.post(items);

if(!response.isSuccess()) {
throw response.getErrors();
throw new PartialWriteError(JSON.stringify(response.getErrors()), response);
}

const createdItems = extractItems(response, state);
Expand All @@ -188,7 +192,7 @@ const queueCreateItems = (items, libraryKey, id, resolve, reject) => {
error,
libraryKey,
items,
id
id,
});
reject(error);
throw error;
Expand Down Expand Up @@ -218,7 +222,7 @@ const createItem = (properties, libraryKey) => {
.post([properties]);

if(!response.isSuccess()) {
throw response.getErrors()[0];
throw new PartialWriteError(response.getErrors()[0], response);
}

const item = {
Expand Down Expand Up @@ -694,31 +698,23 @@ const queueMoveItemsToTrash = (itemKeys, libraryKey, id) => {
.map(ik => get(state, ['libraries', libraryKey, 'items', ik, 'parentItem']))
.filter(Boolean);

if(response.isSuccess()) {
dispatch({
type: RECEIVE_MOVE_ITEMS_TRASH,
libraryKey,
response,
id,
itemKeys,
...itemsData,
otherItems: state.libraries[libraryKey].items,
});

if(affectedParentItemKeys.length > 0) {
dispatch(fetchItemsByKeys(affectedParentItemKeys));
}
} else {
dispatch({
type: ERROR_MOVE_ITEMS_TRASH,
itemKeys: itemKeys.filter(itemKey => !itemKeys.includes(itemKey)),
error: response.getErrors(),
libraryKey,
id
});
if(!response.isSuccess()) {
throw new PartialWriteError(response.getErrors(), response);
}

return;
dispatch({
type: RECEIVE_MOVE_ITEMS_TRASH,
libraryKey,
response,
id,
itemKeys,
...itemsData,
otherItems: state.libraries[libraryKey].items,
});

if(affectedParentItemKeys.length > 0) {
dispatch(fetchItemsByKeys(affectedParentItemKeys));
}
} catch(error) {
dispatch({
type: ERROR_MOVE_ITEMS_TRASH,
Expand Down Expand Up @@ -774,30 +770,21 @@ const queueRecoverItemsFromTrash = (itemKeys, libraryKey, id) => {
.map(ik => get(state, ['libraries', libraryKey, 'items', ik, 'parentItem']))
.filter(Boolean);

if(response.isSuccess()) {
dispatch({
type: RECEIVE_RECOVER_ITEMS_TRASH,
libraryKey,
response,
id,
itemKeys,
...itemsData,
otherItems: state.libraries[libraryKey].items,
});
if(affectedParentItemKeys.length > 0) {
dispatch(fetchItemsByKeys(affectedParentItemKeys));
}
} else {
dispatch({
type: ERROR_RECOVER_ITEMS_TRASH,
itemKeys: itemKeys.filter(itemKey => !itemKeys.includes(itemKey)),
error: response.getErrors(),
libraryKey,
id
});
if(!response.isSuccess()) {
throw new PartialWriteError(response.getErrors(), response);
}
dispatch({
type: RECEIVE_RECOVER_ITEMS_TRASH,
libraryKey,
response,
id,
itemKeys,
...itemsData,
otherItems: state.libraries[libraryKey].items,
});
if(affectedParentItemKeys.length > 0) {
dispatch(fetchItemsByKeys(affectedParentItemKeys));
}

return;
} catch(error) {
dispatch({
type: ERROR_RECOVER_ITEMS_TRASH,
Expand Down Expand Up @@ -856,30 +843,20 @@ const queueToggleTagOnItems = (itemKeys, libraryKey, tagsToToggle, toggleAction,
const { response, itemKeys, items } = await postItemsMultiPatch(state, multiPatch);
const itemKeysChanged = Object.values(response.raw.success);

if(response.isSuccess()) {
dispatch({
type: RECEIVE_ADD_TAGS_TO_ITEMS,
libraryKey,
itemKeys,
itemKeysChanged,
tagsToToggle,
items,
response,
id,
// otherItems: state.libraries[libraryKey].items,
});
} else {
dispatch({
type: ERROR_ADD_TAGS_TO_ITEMS,
itemKeys: itemKeys.filter(itemKey => !itemKeys.includes(itemKey)),
error: response.getErrors(),
libraryKey,
tagsToToggle,
id
});
if(!response.isSuccess()) {
throw new PartialWriteError(response.getErrors(), response);
}

return;
dispatch({
type: RECEIVE_ADD_TAGS_TO_ITEMS,
libraryKey,
itemKeys,
itemKeysChanged,
tagsToToggle,
items,
response,
id,
// otherItems: state.libraries[libraryKey].items,
});
} catch(error) {
dispatch({
type: ERROR_ADD_TAGS_TO_ITEMS,
Expand Down Expand Up @@ -945,30 +922,20 @@ const queueAddToCollection = (itemKeys, collectionKey, libraryKey, id) => {
const { response, itemKeys, items } = await postItemsMultiPatch(state, multiPatch);
const itemKeysChanged = Object.values(response.raw.success);

if(response.isSuccess()) {
dispatch({
type: RECEIVE_ADD_ITEMS_TO_COLLECTION,
libraryKey,
itemKeys,
itemKeysChanged,
collectionKey,
items,
response,
id,
otherItems: state.libraries[libraryKey].items,
});
} else {
dispatch({
type: ERROR_ADD_ITEMS_TO_COLLECTION,
itemKeys: itemKeys.filter(itemKey => !itemKeys.includes(itemKey)),
error: response.getErrors(),
libraryKey,
collectionKey,
id
});
if(!response.isSuccess()) {
throw new PartialWriteError(response.getErrors(), response);
}

return;
dispatch({
type: RECEIVE_ADD_ITEMS_TO_COLLECTION,
libraryKey,
itemKeys,
itemKeysChanged,
collectionKey,
items,
response,
id,
otherItems: state.libraries[libraryKey].items,
});
} catch(error) {
dispatch({
type: ERROR_ADD_ITEMS_TO_COLLECTION,
Expand Down Expand Up @@ -1023,11 +990,21 @@ const copyToLibrary = (itemKeys, sourceLibraryKey, targetLibraryKey, targetColle
await dispatch(fetchLibrarySettings(targetLibraryKey, 'tagColors'));
}

itemKeys = itemKeys.filter(ik => {
const sourceItem = state.libraries[sourceLibraryKey].items[ik];
if (!sourceItem) {
return false;
}
if (targetLibrary.isGroupLibrary && sourceItem.itemType === 'attachment' && sourceItem.linkMode === 'linked_file') {
return false;
}
return true;
});

const newItems = await dispatch(
createItems(
itemKeys.map(ik => {
const sourceItem = state.libraries[sourceLibraryKey].items[ik];

// add 'owl:sameAs' relation for items and notes but not attachments or annotations (based on https://github.com/zotero/zotero/blob/c5a769285b31bde5e78ff51349adc5be8d23871f/chrome/content/zotero/collectionTree.jsx#L1636-L1661)
const newRelations = (['attachment', 'annotation'].includes(sourceItem.itemType) || shouldStoreRelationInSource) ? sourceItem.relations : {
...sourceItem.relations,
Expand Down Expand Up @@ -1272,30 +1249,20 @@ const queueRemoveFromCollection = (itemKeys, collectionKey, libraryKey, id) => {
const { response, itemKeys, items } = await postItemsMultiPatch(state, multiPatch);
const itemKeysChanged = Object.values(response.raw.success);

if(response.isSuccess()) {
dispatch({
type: RECEIVE_REMOVE_ITEMS_FROM_COLLECTION,
libraryKey,
itemKeys,
itemKeysChanged,
collectionKey,
items,
response,
id,
otherItems: state.libraries[libraryKey].items,
});
} else {
dispatch({
type: ERROR_REMOVE_ITEMS_FROM_COLLECTION,
itemKeys: itemKeys.filter(itemKey => !itemKeys.includes(itemKey)),
error: response.getErrors(),
libraryKey,
collectionKey,
id
});
if(!response.isSuccess()) {
throw new PartialWriteError(response.getErrors(), response);
}

return;
dispatch({
type: RECEIVE_REMOVE_ITEMS_FROM_COLLECTION,
libraryKey,
itemKeys,
itemKeysChanged,
collectionKey,
items,
response,
id,
otherItems: state.libraries[libraryKey].items,
});
} catch(error) {
dispatch({
type: ERROR_REMOVE_ITEMS_FROM_COLLECTION,
Expand Down
7 changes: 6 additions & 1 deletion src/js/reducers/libraries/sync.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@ const sync = (state = defaultState, action) => {
}
} else if(action.type && action.type.startsWith('ERROR_')) {
newState.requestsPending--;
if(action.error && action.error.response && action.error.response.status === 412) {

if(typeof action.error?.response?.getVersion?.() !== 'undefined') {
newState.version = Math.max(action.error.response.getVersion(), state.version);
}

if(action.error?.response?.status === 412) {
newState.isSynced = false;
}
} else if(action.type && action.type.startsWith('DROP_')) {
Expand Down
12 changes: 7 additions & 5 deletions test/reader.test.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ describe('Reader', () => {
http.post('https://api.zotero.org/users/1/items', async ({ request }) => {
const items = await request.json();
expect(items[0].key).toBe('Z1Z2Z3Z4');
expect(request.headers.get('If-Unmodified-Since-Version')).toBe('305');
expect(items[0].itemType).toBe('annotation');
expect(items[0].parentItem).toBe('N2PJUHD6');
postedAnnotation = true;
Expand Down Expand Up @@ -155,6 +154,7 @@ describe('Reader', () => {
test('Update item that server is still creating', async () => {
let hasRequestedTpl = false;
let postCounter = 0;
let firstRequestFired = false;
server.use(
http.get('https://api.zotero.org/items/new', ({request}) => {
const url = new URL(request.url);
Expand All @@ -167,17 +167,17 @@ describe('Reader', () => {
const items = await request.json();
expect(items[0].key).toBe('Z1Z2Z3Z4');

if(postCounter == 0) {
expect(request.headers.get('If-Unmodified-Since-Version')).toBe('292');
if (!firstRequestFired) {
expect(items[0].itemType).toBe('annotation');
expect(items[0].parentItem).toBe('N2PJUHD6');
expect(items[0].annotationType).toBe('note');
expect(items[0].annotationComment).toBe('hello note annotation');
// set `firstRequestFired = true` before the delay so that we can start the second request while this one is still pending
firstRequestFired = true;
} else {
expect(request.headers.get('If-Unmodified-Since-Version')).toBe('12345');
expect(items[0].annotationComment).toBe('updated note annotation');
}

await delay(100);
return HttpResponse.json(testUserCreateAnnotation, {
headers: { 'Last-Modified-Version': 12345 + postCounter++ }
Expand All @@ -201,7 +201,9 @@ describe('Reader', () => {
await act(() => readerConfig.onSaveAnnotations([noteAnnotation]));
await waitForPosition();
expect(hasRequestedTpl).toBe(true);
await waitFor(() => expect(postCounter).toBe(1));
await waitFor(() => expect(firstRequestFired).toBe(true));
// At this point first request (`createItems`) has not finished yet. We start the second request (`updateMultipleItems`) while the first one is still pending
// This should delay the second request until the first one is finished, instead of spawning another `createItems` request
await act(() => readerConfig.onSaveAnnotations([{ ...noteAnnotation, comment: 'updated note annotation' }]));
await waitForPosition();
await waitFor(() => expect(postCounter).toBe(2));
Expand Down

0 comments on commit 1a77c52

Please sign in to comment.