diff --git a/packages/server/src/models/ShareModel.ts b/packages/server/src/models/ShareModel.ts index 090f653b04..2ffe739be9 100644 --- a/packages/server/src/models/ShareModel.ts +++ b/packages/server/src/models/ShareModel.ts @@ -227,12 +227,18 @@ export default class ShareModel extends BaseModel { perfTimer.pop(); }; - const handleUpdated = async (change: Change, item: Item, share: Share) => { - const previousItem = this.models().change().unserializePreviousItem(change.previous_item); - const previousShareId = previousItem.jop_share_id; + const getPreviousShareId = (change: Change) => { + return this.models().change().unserializePreviousItem(change.previous_item)?.jop_share_id; + }; + + const handleUpdated = async (change: Change, item: Item, share: Share, nextShareId: Uuid) => { + const previousShareId = getPreviousShareId(change); const shareId = share ? share.id : ''; - if (previousShareId === shareId) return; + const changesShareId = previousShareId !== nextShareId; + if (previousShareId === shareId || !changesShareId) { + return; + } perfTimer.push('handleUpdated'); @@ -342,6 +348,18 @@ export default class ShareModel extends BaseModel { await this.withTransaction(async () => { perfTimer.push(`Processing ${changes.length} changes`); + const itemToUpdates = new Map(); + for (const change of changes) { + if (change.type === ChangeType.Update) { + const updates = itemToUpdates.get(change.item_id); + if (updates) { + updates.push(change); + } else { + itemToUpdates.set(change.item_id, [change]); + } + } + } + for (const change of changes) { const item = items.find(i => i.id === change.item_id); @@ -355,7 +373,18 @@ export default class ShareModel extends BaseModel { } if (change.type === ChangeType.Update) { - await handleUpdated(change, item, itemShare); + const allUpdates = itemToUpdates.get(item.id); + const changeIndex = allUpdates.indexOf(change); + const nextChange = allUpdates[changeIndex + 1]; + + let nextShareId; + if (nextChange) { + nextShareId = getPreviousShareId(nextChange); + } else { + nextShareId = item.jop_share_id; + } + + await handleUpdated(change, item, itemShare, nextShareId); } } diff --git a/packages/server/src/routes/api/shares.folder.test.ts b/packages/server/src/routes/api/shares.folder.test.ts index 9a76e13520..aeed8eadf5 100644 --- a/packages/server/src/routes/api/shares.folder.test.ts +++ b/packages/server/src/routes/api/shares.folder.test.ts @@ -1,4 +1,4 @@ -import { ChangeType, Share, ShareType, ShareUser, ShareUserStatus } from '../../services/database/types'; +import { ChangeType, Session, Share, ShareType, ShareUser, ShareUserStatus } from '../../services/database/types'; import { beforeAllDb, afterAllTests, beforeEachDb, createUserAndSession, models, createNote, createFolder, updateItem, createItemTree, makeNoteSerializedBody, updateNote, expectHttpError, createResource, expectNotThrow } from '../../utils/testing/testUtils'; import { postApi, patchApi, getApi, deleteApi } from '../../utils/testing/apiUtils'; import { PaginatedDeltaChanges } from '../../models/ChangeModel'; @@ -9,6 +9,67 @@ import { resourceBlobPath, serializeJoplinItem, unserializeJoplinItem } from '.. import { PaginatedItems } from '../../models/ItemModel'; import { NoteEntity } from '@joplin/lib/services/database/types'; +const createSingleFolderShare = async (session1: Session, session2: Session) => { + const folderItem = await createFolder(session1.id, { title: 'created by sharer' }); + + // ---------------------------------------------------------------- + // Create the file share object + // ---------------------------------------------------------------- + const share = await postApi(session1.id, 'shares', { + type: ShareType.Folder, + folder_id: folderItem.jop_id, + }); + + // ---------------------------------------------------------------- + // Once the share object has been created, the client can add folders + // and notes to it. This is done by setting the share_id property, + // which we simulate here. + // ---------------------------------------------------------------- + await models().item().saveForUser(session1.user_id, { + id: folderItem.id, + jop_parent_id: '', + jop_share_id: share.id, + }); + + // ---------------------------------------------------------------- + // Send the share to a user + // ---------------------------------------------------------------- + const user2 = await models().user().load(session2.user_id); + let shareUser = await postApi(session1.id, `shares/${share.id}/users`, { + email: user2.email, + }) as ShareUser; + + shareUser = await models().shareUser().load(shareUser.id); + expect(shareUser.share_id).toBe(share.id); + expect(shareUser.user_id).toBe(user2.id); + expect(shareUser.status).toBe(ShareUserStatus.Waiting); + + // ---------------------------------------------------------------- + // On the sharee side, accept the share + // ---------------------------------------------------------------- + await patchApi(session2.id, `share_users/${shareUser.id}`, { status: ShareUserStatus.Accepted }); + + { + shareUser = await models().shareUser().load(shareUser.id); + expect(shareUser.status).toBe(ShareUserStatus.Accepted); + } + + await models().share().updateSharedItems3(); + + // ---------------------------------------------------------------- + // On the sharee side, check that the file is present + // and with the right content. + // ---------------------------------------------------------------- + const results = await getApi(session2.id, 'items/root/children'); + expect(results.items.length).toBe(1); + expect(results.items[0].name).toBe(folderItem.name); + + const itemContent = await getApi(session2.id, `items/root:/${folderItem.name}:/content`); + expect(itemContent.toString().includes('created by sharer')).toBe(true); + + return { folderItem, itemContent, share }; +}; + describe('shares.folder', () => { beforeAll(async () => { @@ -24,62 +85,15 @@ describe('shares.folder', () => { }); test('should share a folder with another user', async () => { - const { user: user1, session: session1 } = await createUserAndSession(1); - const { user: user2, session: session2 } = await createUserAndSession(2); - const folderItem = await createFolder(session1.id, { title: 'created by sharer' }); + const { session: session1 } = await createUserAndSession(1); + const { session: session2 } = await createUserAndSession(2); + await createSingleFolderShare(session1, session2); + }); - // ---------------------------------------------------------------- - // Create the file share object - // ---------------------------------------------------------------- - const share = await postApi(session1.id, 'shares', { - type: ShareType.Folder, - folder_id: folderItem.jop_id, - }); - - // ---------------------------------------------------------------- - // Once the share object has been created, the client can add folders - // and notes to it. This is done by setting the share_id property, - // which we simulate here. - // ---------------------------------------------------------------- - await models().item().saveForUser(user1.id, { - id: folderItem.id, - jop_share_id: share.id, - }); - - // ---------------------------------------------------------------- - // Send the share to a user - // ---------------------------------------------------------------- - let shareUser = await postApi(session1.id, `shares/${share.id}/users`, { - email: user2.email, - }) as ShareUser; - - shareUser = await models().shareUser().load(shareUser.id); - expect(shareUser.share_id).toBe(share.id); - expect(shareUser.user_id).toBe(user2.id); - expect(shareUser.status).toBe(ShareUserStatus.Waiting); - - // ---------------------------------------------------------------- - // On the sharee side, accept the share - // ---------------------------------------------------------------- - await patchApi(session2.id, `share_users/${shareUser.id}`, { status: ShareUserStatus.Accepted }); - - { - shareUser = await models().shareUser().load(shareUser.id); - expect(shareUser.status).toBe(ShareUserStatus.Accepted); - } - - await models().share().updateSharedItems3(); - - // ---------------------------------------------------------------- - // On the sharee side, check that the file is present - // and with the right content. - // ---------------------------------------------------------------- - const results = await getApi(session2.id, 'items/root/children'); - expect(results.items.length).toBe(1); - expect(results.items[0].name).toBe(folderItem.name); - - const itemContent = await getApi(session2.id, `items/root:/${folderItem.name}:/content`); - expect(itemContent.toString().includes('created by sharer')).toBe(true); + test('sharer should see change if item is changed by sharee', async () => { + const { session: session1 } = await createUserAndSession(1); + const { session: session2 } = await createUserAndSession(2); + const { folderItem, itemContent } = await createSingleFolderShare(session1, session2); // ---------------------------------------------------------------- // If file is changed by sharee, sharer should see the change too @@ -97,6 +111,30 @@ describe('shares.folder', () => { } }); + test('should not delete an item on unshare', async () => { + const { session: session1 } = await createUserAndSession(1); + const { session: session2 } = await createUserAndSession(2); + const { folderItem, share } = await createSingleFolderShare(session1, session2); + + await models().item().saveForUser(session2.user_id, { + id: folderItem.id, + jop_parent_id: 'changed', + jop_share_id: share.id, + }); + + await models().item().saveForUser(session1.user_id, { + id: folderItem.id, + jop_parent_id: 'changed', + jop_share_id: '', + }); + + await models().share().updateSharedItems3(); + + const user1HasAccess = !!await models().userItem().byUserAndItemId(session1.user_id, folderItem.id); + const user2HasAccess = !!await models().userItem().byUserAndItemId(session2.user_id, folderItem.id); + expect({ user1HasAccess, user2HasAccess }).toEqual({ user1HasAccess: true, user2HasAccess: false }); + }); + test('should share a folder and all its children', async () => { const { session: session1 } = await createUserAndSession(1); const { session: session2 } = await createUserAndSession(2);