From 2322291216aaa131eaa20dc29b554d5d04e5ab16 Mon Sep 17 00:00:00 2001 From: JounQin Date: Thu, 18 Jul 2024 19:32:43 +0800 Subject: [PATCH 1/3] fix: consider broadcast union with user close #25682 Signed-off-by: JounQin --- .../database/DatabaseNotificationsStore.ts | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/plugins/notifications-backend/src/database/DatabaseNotificationsStore.ts b/plugins/notifications-backend/src/database/DatabaseNotificationsStore.ts index 3d027128ec..b23f5254f4 100644 --- a/plugins/notifications-backend/src/database/DatabaseNotificationsStore.ts +++ b/plugins/notifications-backend/src/database/DatabaseNotificationsStore.ts @@ -138,14 +138,14 @@ export class DatabaseNotificationsStore implements NotificationsStore { }; }; - private getBroadcastUnion = () => { + private getBroadcastUnion = (user?: string | null) => { return this.db('broadcast') - .leftJoin( - 'broadcast_user_status', - 'id', - '=', - 'broadcast_user_status.broadcast_id', - ) + .leftJoin('broadcast_user_status', function clause() { + const join = this.on('id', '=', 'broadcast_user_status.broadcast_id'); + if (user !== null && user !== undefined) { + join.andOnVal('user', '=', user); + } + }) .select(NOTIFICATION_COLUMNS); }; @@ -156,7 +156,7 @@ export class DatabaseNotificationsStore implements NotificationsStore { const subQuery = this.db('notification') .select(NOTIFICATION_COLUMNS) - .unionAll([this.getBroadcastUnion()]) + .unionAll([this.getBroadcastUnion(user)]) .as('notifications'); const query = this.db.from(subQuery).where(q => { @@ -345,16 +345,19 @@ export class DatabaseNotificationsStore implements NotificationsStore { broadcastQuery.update({ ...updateColumns, read: undefined }), ]); - return await this.getNotification({ id }); + return await this.getNotification({ id, user: notification.user }); } - async getNotification(options: { id: string }): Promise { + async getNotification(options: { + id: string; + user?: string | null; + }): Promise { const rows = await this.db .select('*') .from( this.db('notification') .select(NOTIFICATION_COLUMNS) - .unionAll([this.getBroadcastUnion()]) + .unionAll([this.getBroadcastUnion(options.user)]) .as('notifications'), ) .where('id', options.id) From 0ce75e308d0a1913efeeb2d8add6f08067ca1709 Mon Sep 17 00:00:00 2001 From: JounQin Date: Thu, 18 Jul 2024 20:24:54 +0800 Subject: [PATCH 2/3] test: add related test cases Signed-off-by: JounQin --- .../DatabaseNotificationsStore.test.ts | 48 ++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/plugins/notifications-backend/src/database/DatabaseNotificationsStore.test.ts b/plugins/notifications-backend/src/database/DatabaseNotificationsStore.test.ts index c6df60c56e..d4e678c8f1 100644 --- a/plugins/notifications-backend/src/database/DatabaseNotificationsStore.test.ts +++ b/plugins/notifications-backend/src/database/DatabaseNotificationsStore.test.ts @@ -42,6 +42,7 @@ async function createStore(databaseId: TestDatabaseId) { const idOnly = (notification: Notification) => notification.id; const user = 'user:default/john.doe'; +const otherUser = 'user:default/jane.doe'; const id0 = '08e0871e-e60a-4f68-8110-5ae3513f992e'; const id1 = '01e0871e-e60a-4f68-8110-5ae3513f992e'; @@ -140,7 +141,7 @@ const testNotification7: Notification = { }; const otherUserNotification: Notification = { id: id0, - user: 'user:default/jane.doe', + user: otherUser, created: new Date(now), origin: 'plugin-test', payload: { @@ -246,6 +247,34 @@ describe.each(databases.eachSupportedId())( expect(notifications.map(idOnly)).toEqual([id2, id3, id1]); }); + it('should return correct broadcast notifications for different users', async () => { + await storage.saveNotification(testNotification1); + await storage.saveBroadcast(testNotification2); + await storage.saveNotification(testNotification3); + await storage.saveNotification(otherUserNotification); + + await storage.markRead({ ids: [id1, id2], user }); + + const notifications = await storage.getNotifications({ + user, + }); + expect(notifications.map(idOnly)).toEqual([id2, id3, id1]); + expect(notifications[1].user).toBe(user); + + let otherUserNotifications = await storage.getNotifications({ + user: otherUser, + }); + expect(otherUserNotifications.map(idOnly)).toEqual([id0, id2]); + expect(otherUserNotifications[1].user).toBeNull(); + + await storage.markRead({ ids: [id0, id2], user: otherUser }); + otherUserNotifications = await storage.getNotifications({ + user: otherUser, + }); + expect(otherUserNotifications.map(idOnly)).toEqual([id0, id2]); + expect(otherUserNotifications[1].user).toBe(otherUser); + }); + it('should allow searching for notifications', async () => { await storage.saveNotification(testNotification2); await storage.saveBroadcast(testNotification1); @@ -571,6 +600,23 @@ describe.each(databases.eachSupportedId())( const notification = await storage.getNotification({ id: id2 }); expect(notification?.id).toEqual(id2); }); + + it('should consider user for broadcast by id', async () => { + await storage.saveBroadcast(testNotification1); + + let notification = await storage.getNotification({ id: id1, user }); + expect(notification?.id).toEqual(id1); + expect(notification?.user).toBeNull(); + await storage.markRead({ ids: [id1], user }); + notification = await storage.getNotification({ id: id1, user }); + expect(notification?.user).toBe(user); + + const otherNotification = await storage.getNotification({ + id: id1, + user: otherUser, + }); + expect(otherNotification?.user).toBeNull(); + }); }); describe('markRead', () => { From 8013044b0c41f99f5d280c5fec804b66c143cd40 Mon Sep 17 00:00:00 2001 From: JounQin Date: Thu, 18 Jul 2024 20:25:35 +0800 Subject: [PATCH 3/3] docs: add changeset Signed-off-by: JounQin --- .changeset/lovely-planes-shop.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/lovely-planes-shop.md diff --git a/.changeset/lovely-planes-shop.md b/.changeset/lovely-planes-shop.md new file mode 100644 index 0000000000..3ee5a57e75 --- /dev/null +++ b/.changeset/lovely-planes-shop.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-notifications-backend': patch +--- + +fix: consider broadcast union with user