From 49d831d9717f5aa18ba1b0be87b63d8d09750a9c Mon Sep 17 00:00:00 2001 From: Naz Date: Tue, 20 Jun 2023 17:03:36 +0700 Subject: [PATCH] Optimized handling for post.deleted event in collections refs https://github.com/TryGhost/Team/issues/3428 - I'm taking an approach of adding specialized support for each event one-by-one. - The post resource deletion event is the most straight forward and works same for both types of collections. --- ghost/collections/src/Collection.ts | 4 + ghost/collections/src/CollectionRepository.ts | 2 +- .../src/CollectionResourceChangeEvent.ts | 21 ++++ ghost/collections/src/CollectionsService.ts | 26 +++-- ghost/collections/src/index.ts | 1 + ghost/collections/test/collections.test.ts | 95 +++++++++++++++---- 6 files changed, 122 insertions(+), 27 deletions(-) create mode 100644 ghost/collections/src/CollectionResourceChangeEvent.ts diff --git a/ghost/collections/src/Collection.ts b/ghost/collections/src/Collection.ts index 32460fa672..b9870b0b9c 100644 --- a/ghost/collections/src/Collection.ts +++ b/ghost/collections/src/Collection.ts @@ -67,6 +67,10 @@ export class Collection { } } + includesPost(id: string) { + return this.posts.includes(id); + } + removeAllPosts() { this._posts = []; } diff --git a/ghost/collections/src/CollectionRepository.ts b/ghost/collections/src/CollectionRepository.ts index f81dc7ea95..a93cbe2dd1 100644 --- a/ghost/collections/src/CollectionRepository.ts +++ b/ghost/collections/src/CollectionRepository.ts @@ -4,5 +4,5 @@ export interface CollectionRepository { save(collection: Collection): Promise getById(id: string): Promise getBySlug(slug: string): Promise - getAll(options: any): Promise + getAll(options?: any): Promise } diff --git a/ghost/collections/src/CollectionResourceChangeEvent.ts b/ghost/collections/src/CollectionResourceChangeEvent.ts new file mode 100644 index 0000000000..1c0c5f08da --- /dev/null +++ b/ghost/collections/src/CollectionResourceChangeEvent.ts @@ -0,0 +1,21 @@ +type CollectionResourceChangeEventData = { + id: string; + resource: 'post' | 'tag' | 'author'; + [any: string]: any; +}; + +export class CollectionResourceChangeEvent { + name: string; + data: CollectionResourceChangeEventData; + timestamp: Date; + + constructor(name: string, data: CollectionResourceChangeEventData, timestamp: Date) { + this.name = name; + this.data = data; + this.timestamp = timestamp; + } + + static create(name: string, data: CollectionResourceChangeEventData, timestamp = new Date()) { + return new CollectionResourceChangeEvent(name, data, timestamp); + } +} diff --git a/ghost/collections/src/CollectionsService.ts b/ghost/collections/src/CollectionsService.ts index 437c7fbb68..24a9ae5548 100644 --- a/ghost/collections/src/CollectionsService.ts +++ b/ghost/collections/src/CollectionsService.ts @@ -1,4 +1,5 @@ import {Collection} from './Collection'; +import {CollectionResourceChangeEvent} from './CollectionResourceChangeEvent'; import {CollectionRepository} from './CollectionRepository'; import tpl from '@tryghost/tpl'; import {MethodNotAllowedError, NotFoundError} from '@tryghost/errors'; @@ -187,14 +188,25 @@ export class CollectionsService { } } - async updateAutomaticCollections() { - const collections = await this.collectionsRepository.getAll({ - filter: 'type:automatic' - }); + async updateCollections(event: CollectionResourceChangeEvent) { + if (event.name === 'post.deleted') { + // NOTE: 'delete' works the same for both manual and automatic collections + const collections = await this.collectionsRepository.getAll(); - for (const collection of collections) { - await this.updateAutomaticCollectionItems(collection); - await this.collectionsRepository.save(collection); + for (const collection of collections) { + if (collection.includesPost(event.data.id)) { + await collection.removePost(event.data.id); + } + } + } else { + const collections = await this.collectionsRepository.getAll({ + filter: 'type:automatic' + }); + + for (const collection of collections) { + await this.updateAutomaticCollectionItems(collection); + await this.collectionsRepository.save(collection); + } } } diff --git a/ghost/collections/src/index.ts b/ghost/collections/src/index.ts index 626ea081dc..6119128a84 100644 --- a/ghost/collections/src/index.ts +++ b/ghost/collections/src/index.ts @@ -1,3 +1,4 @@ export * from './CollectionsService'; export * from './CollectionsRepositoryInMemory'; export * from './Collection'; +export * from './CollectionResourceChangeEvent'; diff --git a/ghost/collections/test/collections.test.ts b/ghost/collections/test/collections.test.ts index a06549a563..4a702bbd6d 100644 --- a/ghost/collections/test/collections.test.ts +++ b/ghost/collections/test/collections.test.ts @@ -1,5 +1,9 @@ import assert from 'assert'; -import {CollectionsService, CollectionsRepositoryInMemory} from '../src/index'; +import { + CollectionsService, + CollectionsRepositoryInMemory, + CollectionResourceChangeEvent +} from '../src/index'; import {PostsRepositoryInMemory} from './fixtures/PostsRepositoryInMemory'; import {posts} from './fixtures/posts'; @@ -23,10 +27,11 @@ const initPostsRepository = (): PostsRepositoryInMemory => { describe('CollectionsService', function () { let collectionsService: CollectionsService; + let postsRepository: PostsRepositoryInMemory; beforeEach(async function () { const collectionsRepository = new CollectionsRepositoryInMemory(); - const postsRepository = initPostsRepository(); + postsRepository = initPostsRepository(); collectionsService = new CollectionsService({ collectionsRepository, @@ -311,29 +316,81 @@ describe('CollectionsService', function () { assert.equal(updatedCollection?.posts[0].id, 'post-2', 'Collection should have the correct post'); }); - // @NOTE: add a more comprehensive test as this one is too basic - it('Updates all automatic collections', async function () { - let collection1 = await collectionsService.createCollection({ - title: 'Featured Collection 1', - description: 'testing automatic collection', - type: 'automatic', - filter: 'featured:true' + describe('updateCollections', function () { + let automaticFeaturedCollection: any; + let automaticNonFeaturedCollection: any; + let manualCollection: any; + + beforeEach(async function () { + automaticFeaturedCollection = await collectionsService.createCollection({ + title: 'Featured Collection', + description: 'testing automatic collection', + type: 'automatic', + filter: 'featured:true' + }); + + automaticNonFeaturedCollection = await collectionsService.createCollection({ + title: 'Non-Featured Collection', + description: 'testing automatic collection', + type: 'automatic', + filter: 'featured:false' + }); + + manualCollection = await collectionsService.createCollection({ + title: 'Manual Collection', + description: 'testing manual collection', + type: 'manual' + }); + + await collectionsService.addPostToCollection(manualCollection.id, posts[0]); + await collectionsService.addPostToCollection(manualCollection.id, posts[1]); }); - let collection2 = await collectionsService.createCollection({ - title: 'Featured Collection 2', - description: 'testing automatic collection', - type: 'automatic', - filter: 'featured:true' + afterEach(async function () { + await collectionsService.destroy(automaticFeaturedCollection.id); + await collectionsService.destroy(automaticNonFeaturedCollection.id); + await collectionsService.destroy(manualCollection.id); }); - assert.equal(collection1.posts.length, 2); - assert.equal(collection2.posts.length, 2); + it('Updates all collections when post is deleted', async function () { + assert.equal((await collectionsService.getById(automaticFeaturedCollection.id))?.posts?.length, 2); + assert.equal((await collectionsService.getById(automaticNonFeaturedCollection.id))?.posts.length, 2); + assert.equal((await collectionsService.getById(manualCollection.id))?.posts.length, 2); - await collectionsService.updateAutomaticCollections(); + const updateCollectionEvent = CollectionResourceChangeEvent.create('post.deleted', { + id: posts[0].id, + resource: 'post' + }); - assert.equal(collection1.posts.length, 2); - assert.equal(collection2.posts.length, 2); + await collectionsService.updateCollections(updateCollectionEvent); + + assert.equal((await collectionsService.getById(automaticFeaturedCollection.id))?.posts?.length, 2); + assert.equal((await collectionsService.getById(automaticNonFeaturedCollection.id))?.posts.length, 1); + assert.equal((await collectionsService.getById(manualCollection.id))?.posts.length, 1); + }); + + it('Updates automatic collections only when post is published', async function () { + const newPost = { + id: 'post-published', + title: 'Post Published', + slug: 'post-published', + featured: true, + published_at: new Date('2023-03-16T07:19:07.447Z'), + deleted: false + }; + await postsRepository.save(newPost); + + const updateCollectionEvent = CollectionResourceChangeEvent.create('post.published', { + id: newPost.id, + resource: 'post' + }); + + await collectionsService.updateCollections(updateCollectionEvent); + + assert.equal((await collectionsService.getById(automaticFeaturedCollection.id))?.posts?.length, 3); + assert.equal((await collectionsService.getById(automaticNonFeaturedCollection.id))?.posts.length, 2); + assert.equal((await collectionsService.getById(manualCollection.id))?.posts.length, 2); + }); }); }); });