diff --git a/.changeset/silent-elephants-reflect.md b/.changeset/silent-elephants-reflect.md new file mode 100644 index 0000000000..f9f35421bc --- /dev/null +++ b/.changeset/silent-elephants-reflect.md @@ -0,0 +1,6 @@ +--- +'@backstage/plugin-notifications-backend': minor +'@backstage/plugin-notifications': minor +--- + +The NotificationsPage newly uses pagination implemented on the backend layer to avoid large dataset transfers diff --git a/plugins/notifications-backend/src/database/DatabaseNotificationsStore.test.ts b/plugins/notifications-backend/src/database/DatabaseNotificationsStore.test.ts index e0af93a1d6..0ee5958713 100644 --- a/plugins/notifications-backend/src/database/DatabaseNotificationsStore.test.ts +++ b/plugins/notifications-backend/src/database/DatabaseNotificationsStore.test.ts @@ -16,7 +16,6 @@ import { TestDatabaseId, TestDatabases } from '@backstage/backend-test-utils'; import { DatabaseNotificationsStore } from './DatabaseNotificationsStore'; import { Knex } from 'knex'; -import { v4 as uuid } from 'uuid'; import { Notification } from '@backstage/plugin-notifications-common'; jest.setTimeout(60_000); @@ -54,6 +53,15 @@ const otherUserNotification: Partial = { user: 'user:default/jane.doe', }; +const id1 = '01e0871e-e60a-4f68-8110-5ae3513f992e'; +const id2 = '02e0871e-e60a-4f68-8110-5ae3513f992e'; +const id3 = '03e0871e-e60a-4f68-8110-5ae3513f992e'; +const id4 = '04e0871e-e60a-4f68-8110-5ae3513f992e'; +const id5 = '05e0871e-e60a-4f68-8110-5ae3513f992e'; +const id6 = '06e0871e-e60a-4f68-8110-5ae3513f992e'; +const id7 = '07e0871e-e60a-4f68-8110-5ae3513f992e'; +const id8 = '08e0871e-e60a-4f68-8110-5ae3513f992e'; + describe.each(databases.eachSupportedId())( 'DatabaseNotificationsStore (%s)', databaseId => { @@ -94,11 +102,9 @@ describe.each(databases.eachSupportedId())( describe('getNotifications', () => { it('should return all notifications for user', async () => { - const id1 = uuid(); - const id2 = uuid(); await insertNotification({ id: id1, ...testNotification }); await insertNotification({ id: id2, ...testNotification }); - await insertNotification({ id: uuid(), ...otherUserNotification }); + await insertNotification({ id: id3, ...otherUserNotification }); const notifications = await storage.getNotifications({ user }); expect(notifications.length).toBe(2); @@ -107,13 +113,10 @@ describe.each(databases.eachSupportedId())( }); it('should return read notifications for user', async () => { - const id1 = uuid(); - const id2 = uuid(); - const id3 = uuid(); await insertNotification({ id: id1, ...testNotification }); await insertNotification({ id: id2, ...testNotification }); await insertNotification({ id: id3, ...testNotification }); - await insertNotification({ id: uuid(), ...otherUserNotification }); + await insertNotification({ id: id4, ...otherUserNotification }); await storage.markRead({ ids: [id1, id3], user }); @@ -127,13 +130,10 @@ describe.each(databases.eachSupportedId())( }); it('should return unread notifications for user', async () => { - const id1 = uuid(); - const id2 = uuid(); - const id3 = uuid(); await insertNotification({ id: id1, ...testNotification }); await insertNotification({ id: id2, ...testNotification }); await insertNotification({ id: id3, ...testNotification }); - await insertNotification({ id: uuid(), ...otherUserNotification }); + await insertNotification({ id: id4, ...otherUserNotification }); await storage.markRead({ ids: [id1, id3], user }); @@ -146,13 +146,10 @@ describe.each(databases.eachSupportedId())( }); it('should return both read and unread notifications for user', async () => { - const id1 = uuid(); - const id2 = uuid(); - const id3 = uuid(); await insertNotification({ id: id1, ...testNotification }); await insertNotification({ id: id2, ...testNotification }); await insertNotification({ id: id3, ...testNotification }); - await insertNotification({ id: uuid(), ...otherUserNotification }); + await insertNotification({ id: id4, ...otherUserNotification }); await storage.markRead({ ids: [id1, id3], user }); @@ -167,8 +164,6 @@ describe.each(databases.eachSupportedId())( }); it('should allow searching for notifications', async () => { - const id1 = uuid(); - const id2 = uuid(); await insertNotification({ id: id1, ...testNotification, @@ -179,7 +174,7 @@ describe.each(databases.eachSupportedId())( }, }); await insertNotification({ id: id2, ...testNotification }); - await insertNotification({ id: uuid(), ...otherUserNotification }); + await insertNotification({ id: id3, ...otherUserNotification }); const notifications = await storage.getNotifications({ user, @@ -190,8 +185,6 @@ describe.each(databases.eachSupportedId())( }); it('should filter notifications based on created date', async () => { - const id1 = uuid(); - const id2 = uuid(); await insertNotification({ id: id1, ...testNotification, @@ -206,7 +199,7 @@ describe.each(databases.eachSupportedId())( }, created: new Date() /* now */, }); - await insertNotification({ id: uuid(), ...otherUserNotification }); + await insertNotification({ id: id3, ...otherUserNotification }); const notifications = await storage.getNotifications({ user, @@ -215,19 +208,97 @@ describe.each(databases.eachSupportedId())( expect(notifications.length).toBe(1); expect(notifications.at(0)?.id).toEqual(id2); }); + + it('should apply pagination', async () => { + const now = Date.now(); + const timeDelay = 5 * 1000; /* 5 secs */ + + await insertNotification({ + id: id1, + ...testNotification, + created: new Date(now - 1 * 60 * 60 * 1000 /* an hour ago */), + }); + await insertNotification({ + id: id2, + ...testNotification, + created: new Date(now), + }); + await insertNotification({ + id: id3, + ...testNotification, + created: new Date(now - 5 * timeDelay), + }); + await insertNotification({ + id: id4, + ...testNotification, + created: new Date(now - 4 * timeDelay), + }); + await insertNotification({ + id: id5, + ...testNotification, + created: new Date(now - 3 * timeDelay), + }); + await insertNotification({ + id: id6, + ...testNotification, + created: new Date(now - 2 * timeDelay), + }); + await insertNotification({ + id: id7, + ...testNotification, + created: new Date(now - 1 * timeDelay), + }); + + await insertNotification({ id: id8, ...otherUserNotification }); + + const allUserNotifications = await storage.getNotifications({ + user, + }); + expect(allUserNotifications.length).toBe(7); + + const notifications = await storage.getNotifications({ + user, + createdAfter: new Date(now - 5 * 60 * 1000 /* 5 mins */), + // so far no pagination + }); + expect(notifications.length).toBe(6); + expect(notifications.at(0)?.id).toEqual(id2); + expect(notifications.at(1)?.id).toEqual(id7); + expect(notifications.at(2)?.id).toEqual(id6); + expect(notifications.at(3)?.id).toEqual(id5); + expect(notifications.at(4)?.id).toEqual(id4); + + const allUserNotificationsPageOne = await storage.getNotifications({ + user, + limit: 3, + offset: 0, + }); + expect(allUserNotificationsPageOne.length).toBe(3); + expect(allUserNotificationsPageOne.at(0)?.id).toEqual(id2); + expect(allUserNotificationsPageOne.at(1)?.id).toEqual(id7); + expect(allUserNotificationsPageOne.at(2)?.id).toEqual(id6); + + const allUserNotificationsPageTwo = await storage.getNotifications({ + user, + limit: 3, + offset: 3, + }); + expect(allUserNotificationsPageTwo.length).toBe(3); + expect(allUserNotificationsPageTwo.at(0)?.id).toEqual(id5); + expect(allUserNotificationsPageTwo.at(1)?.id).toEqual(id4); + expect(allUserNotificationsPageTwo.at(2)?.id).toEqual(id3); + }); }); describe('getStatus', () => { it('should return status for user', async () => { - const id1 = uuid(); - const id2 = uuid(); await insertNotification({ id: id1, ...testNotification, read: new Date(), }); await insertNotification({ id: id2, ...testNotification }); - await insertNotification({ id: uuid(), ...otherUserNotification }); + await insertNotification({ id: id3, ...otherUserNotification }); const status = await storage.getStatus({ user }); expect(status.read).toEqual(1); @@ -237,7 +308,6 @@ describe.each(databases.eachSupportedId())( describe('getExistingScopeNotification', () => { it('should return existing scope notification', async () => { - const id1 = uuid(); const notification: any = { ...testNotification, id: id1, @@ -262,7 +332,6 @@ describe.each(databases.eachSupportedId())( describe('restoreExistingNotification', () => { it('should return restore existing scope notification', async () => { - const id1 = uuid(); const notification: any = { ...testNotification, id: id1, @@ -296,7 +365,6 @@ describe.each(databases.eachSupportedId())( describe('getNotification', () => { it('should return notification by id', async () => { - const id1 = uuid(); await insertNotification({ id: id1, ...testNotification }); const notification = await storage.getNotification({ id: id1 }); @@ -306,7 +374,6 @@ describe.each(databases.eachSupportedId())( describe('markRead', () => { it('should mark notification read', async () => { - const id1 = uuid(); await insertNotification({ id: id1, ...testNotification }); await storage.markRead({ ids: [id1], user }); @@ -317,7 +384,6 @@ describe.each(databases.eachSupportedId())( describe('markUnread', () => { it('should mark notification unread', async () => { - const id1 = uuid(); await insertNotification({ id: id1, ...testNotification, @@ -332,7 +398,6 @@ describe.each(databases.eachSupportedId())( describe('markSaved', () => { it('should mark notification saved', async () => { - const id1 = uuid(); await insertNotification({ id: id1, ...testNotification }); await storage.markSaved({ ids: [id1], user }); @@ -343,7 +408,6 @@ describe.each(databases.eachSupportedId())( describe('markUnsaved', () => { it('should mark notification not saved', async () => { - const id1 = uuid(); await insertNotification({ id: id1, ...testNotification, @@ -358,7 +422,6 @@ describe.each(databases.eachSupportedId())( describe('saveNotification', () => { it('should store a notification', async () => { - const id1 = uuid(); await storage.saveNotification({ id: id1, user, diff --git a/plugins/notifications-backend/src/database/DatabaseNotificationsStore.ts b/plugins/notifications-backend/src/database/DatabaseNotificationsStore.ts index 9b654cfd04..4c2d2cc5e1 100644 --- a/plugins/notifications-backend/src/database/DatabaseNotificationsStore.ts +++ b/plugins/notifications-backend/src/database/DatabaseNotificationsStore.ts @@ -99,7 +99,6 @@ export class DatabaseNotificationsStore implements NotificationsStore { ) => { const { user } = options; const isSQLite = this.db.client.config.client.includes('sqlite3'); - // const isPsql = this.db.client.config.client.includes('pg'); const query = this.db('notification').where('user', user); @@ -165,6 +164,16 @@ export class DatabaseNotificationsStore implements NotificationsStore { return this.mapToNotifications(notifications); } + async getNotificationsCount(options: NotificationGetOptions) { + const countOptions: NotificationGetOptions = { ...options }; + countOptions.limit = undefined; + countOptions.offset = undefined; + countOptions.sort = null; + const notificationQuery = this.getNotificationsBaseQuery(countOptions); + const response = await notificationQuery.count('* as CNT'); + return Number(response[0].CNT); + } + async saveNotification(notification: Notification) { await this.db .insert(this.mapNotificationToDbRow(notification)) diff --git a/plugins/notifications-backend/src/database/NotificationsStore.ts b/plugins/notifications-backend/src/database/NotificationsStore.ts index 0a7df92f03..03f47263f1 100644 --- a/plugins/notifications-backend/src/database/NotificationsStore.ts +++ b/plugins/notifications-backend/src/database/NotificationsStore.ts @@ -42,6 +42,7 @@ export type NotificationModifyOptions = { /** @internal */ export interface NotificationsStore { getNotifications(options: NotificationGetOptions): Promise; + getNotificationsCount(options: NotificationGetOptions): Promise; saveNotification(notification: Notification): Promise; diff --git a/plugins/notifications-backend/src/service/router.ts b/plugins/notifications-backend/src/service/router.ts index 221606e2da..519919c6d4 100644 --- a/plugins/notifications-backend/src/service/router.ts +++ b/plugins/notifications-backend/src/service/router.ts @@ -197,15 +197,21 @@ export async function createRouter( // or keep undefined } if (req.query.created_after) { - const sinceEpoch = Date.parse(req.query.created_after.toString()); + const sinceEpoch = Date.parse(String(req.query.created_after)); if (isNaN(sinceEpoch)) { throw new InputError('Unexpected date format'); } opts.createdAfter = new Date(sinceEpoch); } - const notifications = await store.getNotifications(opts); - res.send(notifications); + const [notifications, totalCount] = await Promise.all([ + store.getNotifications(opts), + store.getNotificationsCount(opts), + ]); + res.send({ + totalCount, + notifications, + }); }); router.get('/:id', async (req, res) => { diff --git a/plugins/notifications/api-report.md b/plugins/notifications/api-report.md index 666daebdfe..caf9b4a2e5 100644 --- a/plugins/notifications/api-report.md +++ b/plugins/notifications/api-report.md @@ -14,6 +14,7 @@ import { Notification as Notification_2 } from '@backstage/plugin-notifications- import { NotificationStatus } from '@backstage/plugin-notifications-common'; import { default as React_2 } from 'react'; import { RouteRef } from '@backstage/core-plugin-api'; +import { TableProps } from '@backstage/core-components'; // @public (undocumented) export type GetNotificationsOptions = { @@ -24,6 +25,12 @@ export type GetNotificationsOptions = { createdAfter?: Date; }; +// @public (undocumented) +export type GetNotificationsResponse = { + notifications: Notification_2[]; + totalCount: number; +}; + // @public (undocumented) export interface NotificationsApi { // (undocumented) @@ -31,7 +38,7 @@ export interface NotificationsApi { // (undocumented) getNotifications( options?: GetNotificationsOptions, - ): Promise; + ): Promise; // (undocumented) getStatus(): Promise; // (undocumented) @@ -51,7 +58,7 @@ export class NotificationsClient implements NotificationsApi { // (undocumented) getNotifications( options?: GetNotificationsOptions, - ): Promise; + ): Promise; // (undocumented) getStatus(): Promise; // (undocumented) @@ -83,14 +90,23 @@ export const NotificationsTable: ({ notifications, onUpdate, setContainsText, + onPageChange, + onRowsPerPageChange, + page, + pageSize, + totalCount, }: NotificationsTableProps) => React_2.JSX.Element; // @public (undocumented) -export type NotificationsTableProps = { +export type NotificationsTableProps = Pick< + TableProps, + 'onPageChange' | 'onRowsPerPageChange' | 'page' | 'totalCount' +> & { isLoading?: boolean; notifications?: Notification_2[]; onUpdate: () => void; setContainsText: (search: string) => void; + pageSize: number; }; // @public (undocumented) diff --git a/plugins/notifications/src/api/NotificationsApi.ts b/plugins/notifications/src/api/NotificationsApi.ts index 4a1c792012..da7d88dee1 100644 --- a/plugins/notifications/src/api/NotificationsApi.ts +++ b/plugins/notifications/src/api/NotificationsApi.ts @@ -40,9 +40,17 @@ export type UpdateNotificationsOptions = { saved?: boolean; }; +/** @public */ +export type GetNotificationsResponse = { + notifications: Notification[]; + totalCount: number; +}; + /** @public */ export interface NotificationsApi { - getNotifications(options?: GetNotificationsOptions): Promise; + getNotifications( + options?: GetNotificationsOptions, + ): Promise; getNotification(id: string): Promise; diff --git a/plugins/notifications/src/api/NotificationsClient.ts b/plugins/notifications/src/api/NotificationsClient.ts index 1013497b3d..ba0782fe6f 100644 --- a/plugins/notifications/src/api/NotificationsClient.ts +++ b/plugins/notifications/src/api/NotificationsClient.ts @@ -15,6 +15,7 @@ */ import { GetNotificationsOptions, + GetNotificationsResponse, NotificationsApi, UpdateNotificationsOptions, } from './NotificationsApi'; @@ -40,7 +41,7 @@ export class NotificationsClient implements NotificationsApi { async getNotifications( options?: GetNotificationsOptions, - ): Promise { + ): Promise { const queryString = new URLSearchParams(); if (options?.limit !== undefined) { queryString.append('limit', options.limit.toString(10)); @@ -59,7 +60,7 @@ export class NotificationsClient implements NotificationsApi { } const urlSegment = `?${queryString}`; - return await this.request(urlSegment); + return await this.request(urlSegment); } async getNotification(id: string): Promise { diff --git a/plugins/notifications/src/components/NotificationsPage/NotificationsPage.tsx b/plugins/notifications/src/components/NotificationsPage/NotificationsPage.tsx index 1f8b141608..81c991798d 100644 --- a/plugins/notifications/src/components/NotificationsPage/NotificationsPage.tsx +++ b/plugins/notifications/src/components/NotificationsPage/NotificationsPage.tsx @@ -35,13 +35,18 @@ export const NotificationsPage = () => { const [refresh, setRefresh] = React.useState(false); const { lastSignal } = useSignal('notifications'); const [unreadOnly, setUnreadOnly] = React.useState(true); + const [pageNumber, setPageNumber] = React.useState(0); + const [pageSize, setPageSize] = React.useState(5); const [containsText, setContainsText] = React.useState(); const [createdAfter, setCreatedAfter] = React.useState('lastWeek'); const { error, value, retry, loading } = useNotificationsApi( - // TODO: add pagination and other filters api => { - const options: GetNotificationsOptions = { search: containsText }; + const options: GetNotificationsOptions = { + search: containsText, + limit: pageSize, + offset: pageNumber * pageSize, + }; if (unreadOnly !== undefined) { options.read = !unreadOnly; } @@ -53,7 +58,7 @@ export const NotificationsPage = () => { return api.getNotifications(options); }, - [containsText, unreadOnly, createdAfter], + [containsText, unreadOnly, createdAfter, pageNumber, pageSize], ); useEffect(() => { @@ -94,9 +99,14 @@ export const NotificationsPage = () => { diff --git a/plugins/notifications/src/components/NotificationsTable/NotificationsTable.tsx b/plugins/notifications/src/components/NotificationsTable/NotificationsTable.tsx index 6bcb6badc7..3452a618fb 100644 --- a/plugins/notifications/src/components/NotificationsTable/NotificationsTable.tsx +++ b/plugins/notifications/src/components/NotificationsTable/NotificationsTable.tsx @@ -15,25 +15,34 @@ */ import React, { useMemo } from 'react'; import throttle from 'lodash/throttle'; +// @ts-ignore +import RelativeTime from 'react-relative-time'; import { Box, IconButton, Tooltip, Typography } from '@material-ui/core'; import { Notification } from '@backstage/plugin-notifications-common'; + import { notificationsApiRef } from '../../api'; import { useApi } from '@backstage/core-plugin-api'; import MarkAsUnreadIcon from '@material-ui/icons/Markunread'; import MarkAsReadIcon from '@material-ui/icons/CheckCircle'; - -// @ts-ignore -import RelativeTime from 'react-relative-time'; -import { Link, Table, TableColumn } from '@backstage/core-components'; +import { + Link, + Table, + TableProps, + TableColumn, +} from '@backstage/core-components'; const ThrottleDelayMs = 1000; /** @public */ -export type NotificationsTableProps = { +export type NotificationsTableProps = Pick< + TableProps, + 'onPageChange' | 'onRowsPerPageChange' | 'page' | 'totalCount' +> & { isLoading?: boolean; notifications?: Notification[]; onUpdate: () => void; setContainsText: (search: string) => void; + pageSize: number; }; /** @public */ @@ -42,6 +51,11 @@ export const NotificationsTable = ({ notifications = [], onUpdate, setContainsText, + onPageChange, + onRowsPerPageChange, + page, + pageSize, + totalCount, }: NotificationsTableProps) => { const notificationsApi = useApi(notificationsApiRef); @@ -156,16 +170,15 @@ export const NotificationsTable = ({ isLoading={isLoading} options={{ search: true, - // TODO: add pagination - // paging: true, - // pageSize, + paging: true, + pageSize, header: false, sorting: false, }} - // onPageChange={setPageNumber} - // onRowsPerPageChange={setPageSize} - // page={offset} - // totalCount={value?.totalCount} + onPageChange={onPageChange} + onRowsPerPageChange={onRowsPerPageChange} + page={page} + totalCount={totalCount} onSearchChange={throttledContainsTextHandler} data={notifications} columns={compactColumns}