From 242e6e0fa3ed2080ac0ad9397fcc6846f1fb6cf3 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Tue, 16 Jan 2024 17:46:23 +0800 Subject: [PATCH 1/8] delete all the labels and highlights attached to the item when item was deleted --- packages/api/src/resolvers/article/index.ts | 11 +----- packages/api/src/services/library_item.ts | 32 ++++++++++++++++ packages/api/test/resolvers/article.test.ts | 41 ++++++++++++++++----- 3 files changed, 66 insertions(+), 18 deletions(-) diff --git a/packages/api/src/resolvers/article/index.ts b/packages/api/src/resolvers/article/index.ts index 34c7abdd6..363f97f13 100644 --- a/packages/api/src/resolvers/article/index.ts +++ b/packages/api/src/resolvers/article/index.ts @@ -76,6 +76,7 @@ import { createOrUpdateLibraryItem, findLibraryItemsByPrefix, searchLibraryItems, + softDeleteLibraryItem, sortParamsToSort, updateLibraryItem, updateLibraryItemReadingProgress, @@ -532,15 +533,7 @@ export const setBookmarkArticleResolver = authorized< } // delete the item and its metadata - const deletedLibraryItem = await updateLibraryItem( - articleID, - { - state: LibraryItemState.Deleted, - deletedAt: new Date(), - }, - uid, - pubsub - ) + const deletedLibraryItem = await softDeleteLibraryItem(articleID, uid, pubsub) analytics.track({ userId: uid, diff --git a/packages/api/src/services/library_item.ts b/packages/api/src/services/library_item.ts index 20b47afb7..e56f3c9ef 100644 --- a/packages/api/src/services/library_item.ts +++ b/packages/api/src/services/library_item.ts @@ -708,6 +708,38 @@ export const restoreLibraryItem = async ( ) } +export const softDeleteLibraryItem = async ( + id: string, + userId: string, + pubsub = createPubSubClient() +): Promise => { + const deletedLibraryItem = await authTrx( + async (tx) => { + const itemRepo = tx.withRepository(libraryItemRepository) + + // mark item as deleted + await itemRepo.update(id, { + state: LibraryItemState.Deleted, + deletedAt: new Date(), + }) + + // delete all labels for this item + await tx.getRepository(EntityLabel).delete({ libraryItemId: id }) + + // delete all highlights for this item + await tx.getRepository(Highlight).delete({ libraryItem: { id } }) + + return itemRepo.findOneByOrFail({ id }) + }, + undefined, + userId + ) + + await pubsub.entityDeleted(EntityType.PAGE, id, userId) + + return deletedLibraryItem +} + export const updateLibraryItem = async ( id: string, libraryItem: QueryDeepPartialEntity, diff --git a/packages/api/test/resolvers/article.test.ts b/packages/api/test/resolvers/article.test.ts index c93a388b3..5d38e024d 100644 --- a/packages/api/test/resolvers/article.test.ts +++ b/packages/api/test/resolvers/article.test.ts @@ -21,7 +21,6 @@ import { } from '../../src/generated/graphql' import { getRepository } from '../../src/repository' import { createGroup, deleteGroup } from '../../src/services/groups' -import { createLabel, deleteLabels } from '../../src/services/labels' import { createOrUpdateLibraryItem, createLibraryItems, @@ -31,8 +30,9 @@ import { deleteLibraryItemsByUserId, findLibraryItemById, findLibraryItemByUrl, + softDeleteLibraryItem, updateLibraryItem, - CreateOrUpdateLibraryItemArgs, + CreateOrUpdateLibraryItemArgs,, } from '../../src/services/library_item' import { deleteUser } from '../../src/services/user' import * as createTask from '../../src/utils/createTask' @@ -43,7 +43,12 @@ import { createTestUser, saveLabelsInLibraryItem, } from '../db' -import { generateFakeUuid, graphqlRequest, request } from '../util' +import { + generateFakeShortId, + generateFakeUuid, + graphqlRequest, + request, +} from '../util' chai.use(chaiString) @@ -706,18 +711,39 @@ describe('Article API', () => { } const item = await createOrUpdateLibraryItem(itemToSave, user.id) itemId = item.id + + await createAndSaveLabelsInLibraryItem(itemId, user.id, [ + { + name: 'test label 2', + }, + ]) + await createHighlight( + { + shortId: generateFakeShortId(), + user: { id: user.id }, + quote: 'test quote 2', + }, + itemId, + user.id + ) }) after(async () => { await deleteLibraryItemById(itemId, user.id) }) - it('marks an article as deleted', async () => { + it('marks an item as deleted and deletes all the labels and highlights attached to the item', async () => { await graphqlRequest(setBookmarkQuery(itemId, false), authToken).expect( 200 ) const item = await findLibraryItemById(itemId, user.id) expect(item?.state).to.eql(LibraryItemState.Deleted) + + const labels = await findLabelsByLibraryItemId(itemId, user.id) + expect(labels).to.be.empty + + const highlights = await findHighlightsByLibraryItemId(itemId, user.id) + expect(highlights).to.be.empty }) }) @@ -2062,11 +2088,8 @@ describe('Article API', () => { // Delete some items for (let i = 0; i < 3; i++) { - await updateLibraryItem( - items[i].id, - { state: LibraryItemState.Deleted, deletedAt: new Date() }, - user.id - ) + await softDeleteLibraryItem(items[i].id, user.id) + deletedItems.push(items[i]) } }) From 38ee6c1331d3afae5febc88f477bd4bb9f154e80 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Tue, 16 Jan 2024 17:46:41 +0800 Subject: [PATCH 2/8] merge labels and highlights when saving a duplicate item --- packages/api/src/services/labels.ts | 28 ++++++++++++++++++++++++++ packages/api/src/services/save_page.ts | 6 ++++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/packages/api/src/services/labels.ts b/packages/api/src/services/labels.ts index 33206093d..cd72e087f 100644 --- a/packages/api/src/services/labels.ts +++ b/packages/api/src/services/labels.ts @@ -58,6 +58,34 @@ export const findOrCreateLabels = async ( ) } +export const createAndAddLabelsToLibraryItem = async ( + libraryItemId: string, + userId: string, + labels?: CreateLabelInput[] | null, + rssFeedUrl?: string | null, + source?: LabelSource, + pubsub?: PubsubClient +) => { + if (rssFeedUrl) { + // add rss label to labels + labels = (labels || []).concat({ name: 'RSS' }) + source = 'system' + } + + // save labels in item + if (labels && labels.length > 0) { + const newLabels = await findOrCreateLabels(labels, userId) + + await addLabelsToLibraryItem( + newLabels, + libraryItemId, + userId, + source, + pubsub + ) + } +} + export const createAndSaveLabelsInLibraryItem = async ( libraryItemId: string, userId: string, diff --git a/packages/api/src/services/save_page.ts b/packages/api/src/services/save_page.ts index e4f065e94..33bec841b 100644 --- a/packages/api/src/services/save_page.ts +++ b/packages/api/src/services/save_page.ts @@ -24,7 +24,7 @@ import { parsePreparedContent } from '../utils/parser' import { contentReaderForLibraryItem } from '../utils/uploads' import { createPageSaveRequest } from './create_page_save_request' import { createHighlight } from './highlights' -import { createAndSaveLabelsInLibraryItem } from './labels' +import { createAndAddLabelsToLibraryItem } from './labels' import { createOrUpdateLibraryItem } from './library_item' // where we can use APIs to fetch their underlying content. @@ -132,7 +132,8 @@ export const savePage = async ( ) clientRequestId = newItem.id - await createAndSaveLabelsInLibraryItem( + // merge labels + await createAndAddLabelsToLibraryItem( clientRequestId, user.id, input.labels, @@ -157,6 +158,7 @@ export const savePage = async ( libraryItem: { id: clientRequestId }, } + // merge highlights try { await createHighlight(highlight, clientRequestId, user.id) } catch (error) { From abfa15e56fac3648cf8fcad3c1062b27214d9d8f Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Wed, 17 Jan 2024 10:04:22 +0800 Subject: [PATCH 3/8] revert soft deletes all the labels and highlights attached to the deleted item --- packages/api/src/services/library_item.ts | 11 +--------- packages/api/test/resolvers/article.test.ts | 23 +-------------------- 2 files changed, 2 insertions(+), 32 deletions(-) diff --git a/packages/api/src/services/library_item.ts b/packages/api/src/services/library_item.ts index e56f3c9ef..a5c145cc2 100644 --- a/packages/api/src/services/library_item.ts +++ b/packages/api/src/services/library_item.ts @@ -723,12 +723,6 @@ export const softDeleteLibraryItem = async ( deletedAt: new Date(), }) - // delete all labels for this item - await tx.getRepository(EntityLabel).delete({ libraryItemId: id }) - - // delete all highlights for this item - await tx.getRepository(Highlight).delete({ libraryItem: { id } }) - return itemRepo.findOneByOrFail({ id }) }, undefined, @@ -751,14 +745,11 @@ export const updateLibraryItem = async ( async (tx) => { const itemRepo = tx.withRepository(libraryItemRepository) - // reset deletedAt and archivedAt + // reset archivedAt switch (libraryItem.state) { case LibraryItemState.Archived: libraryItem.archivedAt = new Date() break - case LibraryItemState.Deleted: - libraryItem.deletedAt = new Date() - break case LibraryItemState.Processing: case LibraryItemState.Succeeded: libraryItem.archivedAt = null diff --git a/packages/api/test/resolvers/article.test.ts b/packages/api/test/resolvers/article.test.ts index 5d38e024d..a13f7d52b 100644 --- a/packages/api/test/resolvers/article.test.ts +++ b/packages/api/test/resolvers/article.test.ts @@ -711,39 +711,18 @@ describe('Article API', () => { } const item = await createOrUpdateLibraryItem(itemToSave, user.id) itemId = item.id - - await createAndSaveLabelsInLibraryItem(itemId, user.id, [ - { - name: 'test label 2', - }, - ]) - await createHighlight( - { - shortId: generateFakeShortId(), - user: { id: user.id }, - quote: 'test quote 2', - }, - itemId, - user.id - ) }) after(async () => { await deleteLibraryItemById(itemId, user.id) }) - it('marks an item as deleted and deletes all the labels and highlights attached to the item', async () => { + it('soft deletes the item', async () => { await graphqlRequest(setBookmarkQuery(itemId, false), authToken).expect( 200 ) const item = await findLibraryItemById(itemId, user.id) expect(item?.state).to.eql(LibraryItemState.Deleted) - - const labels = await findLabelsByLibraryItemId(itemId, user.id) - expect(labels).to.be.empty - - const highlights = await findHighlightsByLibraryItemId(itemId, user.id) - expect(highlights).to.be.empty }) }) From eb895b49bf7b747cef12121103010b08dcf0ee96 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Wed, 17 Jan 2024 11:23:29 +0800 Subject: [PATCH 4/8] delete labels and highlights attached to the recreated item if the item was deleted before --- packages/api/src/services/library_item.ts | 42 +++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/packages/api/src/services/library_item.ts b/packages/api/src/services/library_item.ts index a5c145cc2..cb9dff16b 100644 --- a/packages/api/src/services/library_item.ts +++ b/packages/api/src/services/library_item.ts @@ -18,6 +18,7 @@ import { authTrx, getColumns, queryBuilderToRawSql } from '../repository' import { libraryItemRepository } from '../repository/library_item' import { Merge } from '../util' import { setRecentlySavedItemInRedis } from '../utils/helpers' +import { logger } from '../utils/logger' import { parseSearchQuery } from '../utils/search' import { addLabelsToLibraryItem } from './labels' @@ -1184,3 +1185,44 @@ export const findLibraryItemIdsByLabelId = async ( userId ) } + +export const recreateLibraryItem = async ( + id: string, + userId: string, + existingItemState: LibraryItemState, + itemToSave: DeepPartial, + pubsub = createPubSubClient() +) => { + // update the item except for id and slug + const updatedItem = await updateLibraryItem( + id, + { + ...itemToSave, + id: undefined, + slug: undefined, + } as QueryDeepPartialEntity, + userId, + pubsub + ) + + try { + // delete labels and highlights if the item was deleted + if (existingItemState === LibraryItemState.Deleted) { + logger.info('Deleting labels and highlights for item', { id }) + await authTrx(async (t) => { + await t.getRepository(Highlight).delete({ + libraryItem: { id }, + }) + + await t.getRepository(EntityLabel).delete({ + libraryItemId: id, + }) + }) + } + } catch (error) { + // continue to save the item even if we failed to delete labels and highlights + logger.error('Failed to delete labels and highlights', error) + } + + return updatedItem +} From b1c45599f6a60cbae81315b3ed757c63cabff3b0 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Wed, 21 Feb 2024 14:29:15 +0800 Subject: [PATCH 5/8] resolve conflicts after rebasing --- packages/api/src/services/library_item.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/api/src/services/library_item.ts b/packages/api/src/services/library_item.ts index cb9dff16b..43ef78331 100644 --- a/packages/api/src/services/library_item.ts +++ b/packages/api/src/services/library_item.ts @@ -8,6 +8,7 @@ import { } from 'typeorm' import { QueryDeepPartialEntity } from 'typeorm/query-builder/QueryPartialEntity' import { ReadingProgressDataSource } from '../datasources/reading_progress_data_source' +import { EntityLabel } from '../entity/entity_label' import { Highlight } from '../entity/highlight' import { Label } from '../entity/label' import { LibraryItem, LibraryItemState } from '../entity/library_item' From af24856603588779f9901bbb4d30cc1f5b5092e4 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Wed, 21 Feb 2024 14:29:21 +0800 Subject: [PATCH 6/8] resolve conflicts after rebasing --- packages/api/src/services/labels.ts | 8 +++----- packages/api/test/resolvers/article.test.ts | 12 ++++-------- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/packages/api/src/services/labels.ts b/packages/api/src/services/labels.ts index cd72e087f..1bf770a6d 100644 --- a/packages/api/src/services/labels.ts +++ b/packages/api/src/services/labels.ts @@ -63,8 +63,7 @@ export const createAndAddLabelsToLibraryItem = async ( userId: string, labels?: CreateLabelInput[] | null, rssFeedUrl?: string | null, - source?: LabelSource, - pubsub?: PubsubClient + source?: LabelSource ) => { if (rssFeedUrl) { // add rss label to labels @@ -77,11 +76,10 @@ export const createAndAddLabelsToLibraryItem = async ( const newLabels = await findOrCreateLabels(labels, userId) await addLabelsToLibraryItem( - newLabels, + newLabels.map((l) => l.id), libraryItemId, userId, - source, - pubsub + source ) } } diff --git a/packages/api/test/resolvers/article.test.ts b/packages/api/test/resolvers/article.test.ts index a13f7d52b..27bbf7818 100644 --- a/packages/api/test/resolvers/article.test.ts +++ b/packages/api/test/resolvers/article.test.ts @@ -21,9 +21,11 @@ import { } from '../../src/generated/graphql' import { getRepository } from '../../src/repository' import { createGroup, deleteGroup } from '../../src/services/groups' +import { createLabel, deleteLabels } from '../../src/services/labels' import { - createOrUpdateLibraryItem, createLibraryItems, + createOrUpdateLibraryItem, + CreateOrUpdateLibraryItemArgs, deleteLibraryItemById, deleteLibraryItemByUrl, deleteLibraryItems, @@ -32,7 +34,6 @@ import { findLibraryItemByUrl, softDeleteLibraryItem, updateLibraryItem, - CreateOrUpdateLibraryItemArgs,, } from '../../src/services/library_item' import { deleteUser } from '../../src/services/user' import * as createTask from '../../src/utils/createTask' @@ -43,12 +44,7 @@ import { createTestUser, saveLabelsInLibraryItem, } from '../db' -import { - generateFakeShortId, - generateFakeUuid, - graphqlRequest, - request, -} from '../util' +import { generateFakeUuid, graphqlRequest, request } from '../util' chai.use(chaiString) From 51c1d33a09242ff035710527061ae5a1fc2b2868 Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Wed, 21 Feb 2024 14:55:49 +0800 Subject: [PATCH 7/8] fix tests --- packages/api/src/services/library_item.ts | 65 ++++++++--------------- packages/api/src/services/save_page.ts | 3 +- 2 files changed, 24 insertions(+), 44 deletions(-) diff --git a/packages/api/src/services/library_item.ts b/packages/api/src/services/library_item.ts index 43ef78331..119eabc6f 100644 --- a/packages/api/src/services/library_item.ts +++ b/packages/api/src/services/library_item.ts @@ -872,18 +872,38 @@ export const createOrUpdateLibraryItem = async ( ) if (existingLibraryItem) { + const id = existingLibraryItem.id // update existing library item const newItem = await repo.save({ ...libraryItem, - id: existingLibraryItem.id, + id, slug: existingLibraryItem.slug, // keep the original slug }) // delete the new item if it's different from the existing one - if (libraryItem.id && libraryItem.id !== existingLibraryItem.id) { + if (libraryItem.id && libraryItem.id !== id) { await repo.delete(libraryItem.id) } + try { + // delete labels and highlights if the item was deleted + if (existingLibraryItem.state === LibraryItemState.Deleted) { + logger.info('Deleting labels and highlights for item', { + id, + }) + await tx.getRepository(Highlight).delete({ + libraryItem: { id: existingLibraryItem.id }, + }) + + await tx.getRepository(EntityLabel).delete({ + libraryItemId: existingLibraryItem.id, + }) + } + } catch (error) { + // continue to save the item even if we failed to delete labels and highlights + logger.error('Failed to delete labels and highlights', error) + } + return newItem } @@ -1186,44 +1206,3 @@ export const findLibraryItemIdsByLabelId = async ( userId ) } - -export const recreateLibraryItem = async ( - id: string, - userId: string, - existingItemState: LibraryItemState, - itemToSave: DeepPartial, - pubsub = createPubSubClient() -) => { - // update the item except for id and slug - const updatedItem = await updateLibraryItem( - id, - { - ...itemToSave, - id: undefined, - slug: undefined, - } as QueryDeepPartialEntity, - userId, - pubsub - ) - - try { - // delete labels and highlights if the item was deleted - if (existingItemState === LibraryItemState.Deleted) { - logger.info('Deleting labels and highlights for item', { id }) - await authTrx(async (t) => { - await t.getRepository(Highlight).delete({ - libraryItem: { id }, - }) - - await t.getRepository(EntityLabel).delete({ - libraryItemId: id, - }) - }) - } - } catch (error) { - // continue to save the item even if we failed to delete labels and highlights - logger.error('Failed to delete labels and highlights', error) - } - - return updatedItem -} diff --git a/packages/api/src/services/save_page.ts b/packages/api/src/services/save_page.ts index 33bec841b..f79b28b52 100644 --- a/packages/api/src/services/save_page.ts +++ b/packages/api/src/services/save_page.ts @@ -216,7 +216,7 @@ export const parsedContentToLibraryItem = ({ rssFeedUrl?: string | null folder?: string | null }): DeepPartial & { originalUrl: string } => { - logger.info('save_page: state', { url, state, itemId }) + logger.info('save_page', { url, state, itemId }) return { id: itemId || undefined, slug, @@ -256,5 +256,6 @@ export const parsedContentToLibraryItem = ({ folder: folder || 'inbox', archivedAt: state === ArticleSavingRequestStatus.Archived ? new Date() : null, + deletedAt: state === ArticleSavingRequestStatus.Deleted ? new Date() : null, } } From 8f39985f0fe8b6791ff212a255d9ef51c061872e Mon Sep 17 00:00:00 2001 From: Hongbo Wu Date: Wed, 21 Feb 2024 15:06:04 +0800 Subject: [PATCH 8/8] fix deleted labels and highlights are still searchable --- packages/api/src/entity/library_item.ts | 6 ++++++ packages/api/src/services/library_item.ts | 26 +++++++++++++---------- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/packages/api/src/entity/library_item.ts b/packages/api/src/entity/library_item.ts index f5c15d42e..f127f52b5 100644 --- a/packages/api/src/entity/library_item.ts +++ b/packages/api/src/entity/library_item.ts @@ -198,4 +198,10 @@ export class LibraryItem { @Column('text') folder!: string + + @Column('text') + labelNames?: string[] + + @Column('text') + highlightAnnotations?: string[] } diff --git a/packages/api/src/services/library_item.ts b/packages/api/src/services/library_item.ts index 119eabc6f..37422d920 100644 --- a/packages/api/src/services/library_item.ts +++ b/packages/api/src/services/library_item.ts @@ -873,17 +873,6 @@ export const createOrUpdateLibraryItem = async ( if (existingLibraryItem) { const id = existingLibraryItem.id - // update existing library item - const newItem = await repo.save({ - ...libraryItem, - id, - slug: existingLibraryItem.slug, // keep the original slug - }) - - // delete the new item if it's different from the existing one - if (libraryItem.id && libraryItem.id !== id) { - await repo.delete(libraryItem.id) - } try { // delete labels and highlights if the item was deleted @@ -898,12 +887,27 @@ export const createOrUpdateLibraryItem = async ( await tx.getRepository(EntityLabel).delete({ libraryItemId: existingLibraryItem.id, }) + + libraryItem.labelNames = [] + libraryItem.highlightAnnotations = [] } } catch (error) { // continue to save the item even if we failed to delete labels and highlights logger.error('Failed to delete labels and highlights', error) } + // update existing library item + const newItem = await repo.save({ + ...libraryItem, + id, + slug: existingLibraryItem.slug, // keep the original slug + }) + + // delete the new item if it's different from the existing one + if (libraryItem.id && libraryItem.id !== id) { + await repo.delete(libraryItem.id) + } + return newItem }