feat: delete older archived notifications #448
Conversation
WalkthroughThis change introduces scheduled deletion of archived notifications with CSV backup. It adds new environment variables, a DELETE API endpoint, updates the scheduler script to trigger deletion, adds CSV backup logic in the service, and includes a new CSV formatting dependency and ignore rule for backup files. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant API_Controller
participant Service
participant DB
participant FileSystem
Scheduler->>API_Controller: DELETE /archived-notifications/delete
API_Controller->>Service: deleteArchivedNotificationsCron()
Service->>Service: Check if deletion enabled and retention threshold
Service->>DB: Query archived notifications older than threshold
alt Notifications found
Service->>FileSystem: Write notifications to CSV backup
Service->>DB: Hard delete notifications
else No notifications found
Service->>Service: Log no entries to delete
end
Service-->>API_Controller: Completion
API_Controller-->>Scheduler: Response
Estimated code review effort3 (30–60 minutes) Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/api/.env.example (1)
22-24: Address formatting issues flagged by static analysis.The new environment variables for archived notification deletion are well-designed and properly documented. However, please address the formatting issues:
- Add quotes around the values for consistency
- Consider reordering the variables alphabetically within the notification configuration section
# Notification configuration MAX_RETRY_COUNT=3 # Max retry count. DEFAULT: 3 ARCHIVE_LIMIT=1000 # Max notifications to archive. DEFAULT: 1000 ARCHIVE_INTERVAL=3600 # Interval (in seconds) for archiving notifications. DEFAULT: 3600 (every 1 hour) SCHEDULE_TIME_IN_SECONDS=5 # Interval (in seconds) for processing PENDING and AWAITING_CONFIRMATION notifications. DEFAULT: 5 (every 5 seconds) -ENABLE_ARCHIVED_NOTIFICATION_DELETION=false # Toggle to enable/disable deletion of archived notifications, VALUES: "true", "false". DEFAULT: false -DELETE_INTERVAL=2592000 # Interval (in seconds) for deletion of archived notifications. DEFAULT: 2592000 (every 30 days) -DELETE_ARCHIVED_NOTIFICATIONS_OLDER_THAN=90d # Use formats from https://github.com/vercel/ms. DEFAULT: 90d (older than 90 days). LIMIT: 10y (older than 10 years) +DELETE_ARCHIVED_NOTIFICATIONS_OLDER_THAN="90d" # Use formats from https://github.com/vercel/ms. DEFAULT: 90d (older than 90 days). LIMIT: 10y (older than 10 years) +DELETE_INTERVAL="2592000" # Interval (in seconds) for deletion of archived notifications. DEFAULT: 2592000 (every 30 days) +ENABLE_ARCHIVED_NOTIFICATION_DELETION="false" # Toggle to enable/disable deletion of archived notifications, VALUES: "true", "false". DEFAULT: falseapps/api/src/modules/archived-notifications/archived-notifications.service.ts (4)
12-12: Use ES6 import syntax for consistencyFor consistency with other imports in the file, use ES6 import syntax instead of require.
-import ms = require('ms'); +import ms from 'ms';
290-298: Consider using UTC time for consistent timestampsThe current implementation uses local time which could cause inconsistencies across different server locations. Consider using UTC time for backup filenames.
private getFormattedTimestamp(): string { const now = new Date(); - const yyyy = now.getFullYear().toString(); - const MM = (now.getMonth() + 1).toString().padStart(2, '0'); - const dd = now.getDate().toString().padStart(2, '0'); - const hh = now.getHours().toString().padStart(2, '0'); - const mm = now.getMinutes().toString().padStart(2, '0'); + const yyyy = now.getUTCFullYear().toString(); + const MM = (now.getUTCMonth() + 1).toString().padStart(2, '0'); + const dd = now.getUTCDate().toString().padStart(2, '0'); + const hh = now.getUTCHours().toString().padStart(2, '0'); + const mm = now.getUTCMinutes().toString().padStart(2, '0'); return `${yyyy}${MM}${dd}${hh}${mm}`; }
198-206: Consider implementing batching for large datasetsThe current implementation fetches all matching records at once, which could cause memory issues with large datasets. Consider implementing pagination or batching.
// Fetch entries to soft delete -const archivedEntries = await queryRunner.manager.find(ArchivedNotification, { - where: { - createdOn: LessThan(cutoffTimestamp), - status: Status.ACTIVE, - }, - order: { - createdOn: 'DESC', - }, -}); +const batchSize = 1000; +let offset = 0; +const allArchivedEntries: ArchivedNotification[] = []; + +while (true) { + const batch = await queryRunner.manager.find(ArchivedNotification, { + where: { + createdOn: LessThan(cutoffTimestamp), + status: Status.ACTIVE, + }, + order: { + createdOn: 'DESC', + }, + take: batchSize, + skip: offset, + }); + + if (batch.length === 0) break; + + allArchivedEntries.push(...batch); + offset += batchSize; + + // Optional: Add a small delay to reduce database load + // await new Promise(resolve => setTimeout(resolve, 100)); +} + +const archivedEntries = allArchivedEntries;
163-165: Add retention period to log messages for better monitoringInclude the configured retention period in the log message for better operational visibility.
-this.logger.log('Running archived notification deletion cron task'); +this.logger.log(`Running archived notification deletion cron task with retention period: ${this.configService.get<string>('DELETE_ARCHIVED_NOTIFICATIONS_OLDER_THAN', '90d')}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/api/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
apps/api/.env.example(1 hunks)apps/api/.gitignore(1 hunks)apps/api/package.json(1 hunks)apps/api/scheduler.sh(3 hunks)apps/api/src/modules/archived-notifications/archived-notifications.controller.ts(2 hunks)apps/api/src/modules/archived-notifications/archived-notifications.service.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#360
File: apps/api/src/modules/notifications/notifications.service.ts:0-0
Timestamp: 2024-11-20T06:08:32.855Z
Learning: The `findNotificationsToArchive` function has been removed from `notifications.service.ts`, and archiving tasks now use `QueryRunner` directly in `archived-notifications.service.ts`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:38.272Z
Learning: In the Osmo-X project, the `notification_sent_on` column in both `notify_archived_notifications` and `notify_notifications` tables is designed to be nullable, as explicitly defined in the migration file with `isNullable: true`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the migration file for adding the 'notification_sent_on' column to 'notify_archived_notifications' and 'notify_notifications' tables, the column is defined as nullable.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the 'AddNotificationSentOnColumn1743145236864' migration file, the 'notification_sent_on' column is intentionally defined as nullable (isNullable: true) for both the 'notify_archived_notifications' and 'notify_notifications' tables.
Learnt from: LakshayaT
PR: OsmosysSoftware/osmo-x#263
File: apps/api/src/modules/notifications/queues/queue.service.ts:96-103
Timestamp: 2024-07-02T10:51:07.210Z
Learning: The `processAwaitingConfirmationNotificationQueue` method in the `NotificationConsumer` class includes robust error handling, logging errors, updating the delivery status, incrementing the retry count, and saving the notification.
Learnt from: osm-vishnukyatannawar
PR: OsmosysSoftware/osmo-x#354
File: apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts:25-27
Timestamp: 2024-11-08T05:23:26.338Z
Learning: In the 'archived_notifications' table, we do not add a foreign key constraint on 'notification_id' to 'notify_notifications' because the original notifications may have been deleted, and we want to retain archived notifications even if the originals are removed.
Learnt from: LakshayaT
PR: OsmosysSoftware/osmo-x#293
File: apps/api/src/modules/notifications/notifications.controller.ts:15-18
Timestamp: 2024-07-29T13:44:11.361Z
Learning: The `addNotificationsToQueue` method in the `notificationService` includes comprehensive error handling, logging errors, and managing the state of the queue processing.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#368
File: apps/portal/src/app/views/notifications/notification.model.ts:21-32
Timestamp: 2024-11-28T12:52:01.779Z
Learning: In the TypeScript file `apps/portal/src/app/views/notifications/notification.model.ts`, the `ArchivedNotification` interface intentionally has a different structure from the `Notification` interface, specifically using `notificationId` instead of `id`.
Learnt from: ayushnvs
PR: OsmosysSoftware/osmo-x#262
File: apps/api/src/jobs/consumers/notifications/mailgun-notifications.job.consumer.ts:0-0
Timestamp: 2024-07-03T13:27:46.550Z
Learning: The `notificationSendResponse` variable in the `processMailgunNotificationConfirmationQueue` method of the `MailgunNotificationConsumer` class was changed from `let` to `const` to reflect that it is never reassigned.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#368
File: apps/portal/src/app/graphql/graphql.queries.ts:50-50
Timestamp: 2024-11-28T12:52:15.785Z
Learning: In `apps/portal/src/app/graphql/graphql.queries.ts`, the `GetArchivedNotifications` query uses the field `notificationId` intentionally instead of `id` because `archivedNotifications` has a different structure than `notifications`.
apps/api/.env.example (8)
Learnt from: Harish-osmosys
PR: OsmosysSoftware/osmo-x#326
File: apps/api/.env.example:14-17
Timestamp: 2024-09-23T08:33:52.794Z
Learning: The `.env.example` file is intended as a reference, and variables like `ALGORITHM`, `KEY`, and `IV` can be modified as needed by the user.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the migration file for adding the 'notification_sent_on' column to 'notify_archived_notifications' and 'notify_notifications' tables, the column is defined as nullable.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the 'AddNotificationSentOnColumn1743145236864' migration file, the 'notification_sent_on' column is intentionally defined as nullable (isNullable: true) for both the 'notify_archived_notifications' and 'notify_notifications' tables.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:38.272Z
Learning: In the Osmo-X project, the `notification_sent_on` column in both `notify_archived_notifications` and `notify_notifications` tables is designed to be nullable, as explicitly defined in the migration file with `isNullable: true`.
Learnt from: ayushnvs
PR: OsmosysSoftware/osmo-x#262
File: apps/api/src/jobs/consumers/notifications/mailgun-notifications.job.consumer.ts:0-0
Timestamp: 2024-07-03T13:27:46.550Z
Learning: The `notificationSendResponse` variable in the `processMailgunNotificationConfirmationQueue` method of the `MailgunNotificationConsumer` class was changed from `let` to `const` to reflect that it is never reassigned.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#360
File: apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts:32-35
Timestamp: 2024-11-20T05:58:43.756Z
Learning: In the codebase, the `Notification` and `ArchivedNotification` entities use `default: null` in the `@Column` decorators for nullable fields, and `@Field()` without `{ nullable: true }`. This pattern is accepted in this project.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#360
File: apps/api/src/modules/notifications/notifications.service.ts:0-0
Timestamp: 2024-11-20T06:08:32.855Z
Learning: The `findNotificationsToArchive` function has been removed from `notifications.service.ts`, and archiving tasks now use `QueryRunner` directly in `archived-notifications.service.ts`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#368
File: apps/portal/src/app/views/notifications/notification.model.ts:21-32
Timestamp: 2024-11-28T12:52:01.779Z
Learning: In the TypeScript file `apps/portal/src/app/views/notifications/notification.model.ts`, the `ArchivedNotification` interface intentionally has a different structure from the `Notification` interface, specifically using `notificationId` instead of `id`.
apps/api/src/modules/archived-notifications/archived-notifications.controller.ts (13)
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#360
File: apps/api/src/modules/notifications/notifications.service.ts:0-0
Timestamp: 2024-11-20T06:08:32.855Z
Learning: The `findNotificationsToArchive` function has been removed from `notifications.service.ts`, and archiving tasks now use `QueryRunner` directly in `archived-notifications.service.ts`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#368
File: apps/portal/src/app/views/notifications/notification.model.ts:21-32
Timestamp: 2024-11-28T12:52:01.779Z
Learning: In the TypeScript file `apps/portal/src/app/views/notifications/notification.model.ts`, the `ArchivedNotification` interface intentionally has a different structure from the `Notification` interface, specifically using `notificationId` instead of `id`.
Learnt from: Harish-osmosys
PR: OsmosysSoftware/osmo-x#278
File: apps/api/src/modules/webhook/webhook.service.ts:0-0
Timestamp: 2024-10-08T17:27:45.632Z
Learning: The `WebhookService` class in `apps/api/src/modules/webhook/webhook.service.ts` now uses the `Logger` from `@nestjs/common` for logging instead of `console.error`.
Learnt from: Harish-osmosys
PR: OsmosysSoftware/osmo-x#278
File: apps/api/src/modules/webhook/webhook.service.ts:0-0
Timestamp: 2024-07-12T09:48:15.458Z
Learning: The `WebhookService` class in `apps/api/src/modules/webhook/webhook.service.ts` now uses the `Logger` from `@nestjs/common` for logging instead of `console.error`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#368
File: apps/portal/src/app/graphql/graphql.queries.ts:50-50
Timestamp: 2024-11-28T12:52:15.785Z
Learning: In `apps/portal/src/app/graphql/graphql.queries.ts`, the `GetArchivedNotifications` query uses the field `notificationId` intentionally instead of `id` because `archivedNotifications` has a different structure than `notifications`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#332
File: apps/portal/src/app/views/notifications/notifications.component.ts:0-0
Timestamp: 2024-10-08T17:27:45.631Z
Learning: When the code in `notifications.component.ts` is outdated, previous suggestions related to it may not apply.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#332
File: apps/portal/src/app/views/notifications/notifications.component.ts:0-0
Timestamp: 2024-09-27T12:15:47.625Z
Learning: When the code in `notifications.component.ts` is outdated, previous suggestions related to it may not apply.
Learnt from: LakshayaT
PR: OsmosysSoftware/osmo-x#293
File: apps/api/src/modules/notifications/notifications.controller.ts:15-18
Timestamp: 2024-07-29T13:44:11.361Z
Learning: The `addNotificationsToQueue` method in the `notificationService` includes comprehensive error handling, logging errors, and managing the state of the queue processing.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#367
File: apps/api/src/modules/archived-notifications/dtos/archived-notification-response.dto.ts:18-23
Timestamp: 2024-11-28T12:49:15.719Z
Learning: In the TypeScript classes `ArchivedNotificationResponse` and `NotificationResponse` within our NestJS application, we do not include input validation in constructors for response DTOs. It's acceptable to proceed without input validation to maintain consistency.
Learnt from: ayushnvs
PR: OsmosysSoftware/osmo-x#262
File: apps/api/src/jobs/consumers/notifications/mailgun-notifications.job.consumer.ts:0-0
Timestamp: 2024-07-03T13:27:46.550Z
Learning: The `notificationSendResponse` variable in the `processMailgunNotificationConfirmationQueue` method of the `MailgunNotificationConsumer` class was changed from `let` to `const` to reflect that it is never reassigned.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#360
File: apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts:32-35
Timestamp: 2024-11-20T05:58:43.756Z
Learning: In the codebase, the `Notification` and `ArchivedNotification` entities use `default: null` in the `@Column` decorators for nullable fields, and `@Field()` without `{ nullable: true }`. This pattern is accepted in this project.
Learnt from: LakshayaT
PR: OsmosysSoftware/osmo-x#262
File: apps/api/src/modules/notifications/queues/queue.service.ts:91-95
Timestamp: 2024-07-03T08:39:09.178Z
Learning: The `processAwaitingConfirmationNotificationQueue` method in the `NotificationConsumer` class includes comprehensive error handling within a try-catch-finally block.
Learnt from: LakshayaT
PR: OsmosysSoftware/osmo-x#263
File: apps/api/src/modules/notifications/queues/queue.service.ts:96-103
Timestamp: 2024-07-02T10:51:07.210Z
Learning: The `processAwaitingConfirmationNotificationQueue` method in the `NotificationConsumer` class includes robust error handling, logging errors, updating the delivery status, incrementing the retry count, and saving the notification.
apps/api/scheduler.sh (3)
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#360
File: apps/api/src/modules/notifications/notifications.service.ts:0-0
Timestamp: 2024-11-20T06:08:32.855Z
Learning: The `findNotificationsToArchive` function has been removed from `notifications.service.ts`, and archiving tasks now use `QueryRunner` directly in `archived-notifications.service.ts`.
Learnt from: LakshayaT
PR: OsmosysSoftware/osmo-x#293
File: apps/api/src/modules/notifications/notifications.controller.ts:15-18
Timestamp: 2024-07-29T13:44:11.361Z
Learning: The `addNotificationsToQueue` method in the `notificationService` includes comprehensive error handling, logging errors, and managing the state of the queue processing.
Learnt from: osm-vishnukyatannawar
PR: OsmosysSoftware/osmo-x#354
File: apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts:25-27
Timestamp: 2024-11-08T05:23:26.338Z
Learning: In the 'archived_notifications' table, we do not add a foreign key constraint on 'notification_id' to 'notify_notifications' because the original notifications may have been deleted, and we want to retain archived notifications even if the originals are removed.
apps/api/src/modules/archived-notifications/archived-notifications.service.ts (13)
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#360
File: apps/api/src/modules/notifications/notifications.service.ts:0-0
Timestamp: 2024-11-20T06:08:32.855Z
Learning: The `findNotificationsToArchive` function has been removed from `notifications.service.ts`, and archiving tasks now use `QueryRunner` directly in `archived-notifications.service.ts`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#368
File: apps/portal/src/app/views/notifications/notification.model.ts:21-32
Timestamp: 2024-11-28T12:52:01.779Z
Learning: In the TypeScript file `apps/portal/src/app/views/notifications/notification.model.ts`, the `ArchivedNotification` interface intentionally has a different structure from the `Notification` interface, specifically using `notificationId` instead of `id`.
Learnt from: LakshayaT
PR: OsmosysSoftware/osmo-x#293
File: apps/api/src/modules/notifications/notifications.controller.ts:15-18
Timestamp: 2024-07-29T13:44:11.361Z
Learning: The `addNotificationsToQueue` method in the `notificationService` includes comprehensive error handling, logging errors, and managing the state of the queue processing.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#368
File: apps/portal/src/app/graphql/graphql.queries.ts:50-50
Timestamp: 2024-11-28T12:52:15.785Z
Learning: In `apps/portal/src/app/graphql/graphql.queries.ts`, the `GetArchivedNotifications` query uses the field `notificationId` intentionally instead of `id` because `archivedNotifications` has a different structure than `notifications`.
Learnt from: ayushnvs
PR: OsmosysSoftware/osmo-x#262
File: apps/api/src/jobs/consumers/notifications/mailgun-notifications.job.consumer.ts:0-0
Timestamp: 2024-07-03T13:27:46.550Z
Learning: The `notificationSendResponse` variable in the `processMailgunNotificationConfirmationQueue` method of the `MailgunNotificationConsumer` class was changed from `let` to `const` to reflect that it is never reassigned.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#332
File: apps/portal/src/app/views/notifications/notifications.component.ts:0-0
Timestamp: 2024-10-08T17:27:45.631Z
Learning: When the code in `notifications.component.ts` is outdated, previous suggestions related to it may not apply.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#332
File: apps/portal/src/app/views/notifications/notifications.component.ts:0-0
Timestamp: 2024-09-27T12:15:47.625Z
Learning: When the code in `notifications.component.ts` is outdated, previous suggestions related to it may not apply.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#360
File: apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts:32-35
Timestamp: 2024-11-20T05:58:43.756Z
Learning: In the codebase, the `Notification` and `ArchivedNotification` entities use `default: null` in the `@Column` decorators for nullable fields, and `@Field()` without `{ nullable: true }`. This pattern is accepted in this project.
Learnt from: Harish-osmosys
PR: OsmosysSoftware/osmo-x#278
File: apps/api/src/modules/webhook/webhook.service.ts:0-0
Timestamp: 2024-10-08T17:27:45.632Z
Learning: The `WebhookService` class in `apps/api/src/modules/webhook/webhook.service.ts` now uses the `Logger` from `@nestjs/common` for logging instead of `console.error`.
Learnt from: Harish-osmosys
PR: OsmosysSoftware/osmo-x#278
File: apps/api/src/modules/webhook/webhook.service.ts:0-0
Timestamp: 2024-07-12T09:48:15.458Z
Learning: The `WebhookService` class in `apps/api/src/modules/webhook/webhook.service.ts` now uses the `Logger` from `@nestjs/common` for logging instead of `console.error`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#367
File: apps/api/src/modules/archived-notifications/dtos/archived-notification-response.dto.ts:18-23
Timestamp: 2024-11-28T12:49:15.719Z
Learning: In the TypeScript classes `ArchivedNotificationResponse` and `NotificationResponse` within our NestJS application, we do not include input validation in constructors for response DTOs. It's acceptable to proceed without input validation to maintain consistency.
Learnt from: osm-vishnukyatannawar
PR: OsmosysSoftware/osmo-x#354
File: apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts:25-27
Timestamp: 2024-11-08T05:23:26.338Z
Learning: In the 'archived_notifications' table, we do not add a foreign key constraint on 'notification_id' to 'notify_notifications' because the original notifications may have been deleted, and we want to retain archived notifications even if the originals are removed.
Learnt from: LakshayaT
PR: OsmosysSoftware/osmo-x#263
File: apps/api/src/modules/notifications/queues/queue.service.ts:96-103
Timestamp: 2024-07-02T10:51:07.210Z
Learning: The `processAwaitingConfirmationNotificationQueue` method in the `NotificationConsumer` class includes robust error handling, logging errors, updating the delivery status, incrementing the retry count, and saving the notification.
🪛 dotenv-linter (3.3.0)
apps/api/.env.example
[warning] 22-22: [UnorderedKey] The ENABLE_ARCHIVED_NOTIFICATION_DELETION key should go before the MAX_RETRY_COUNT key
[warning] 22-22: [ValueWithoutQuotes] This value needs to be surrounded in quotes
[warning] 23-23: [UnorderedKey] The DELETE_INTERVAL key should go before the ENABLE_ARCHIVED_NOTIFICATION_DELETION key
[warning] 23-23: [ValueWithoutQuotes] This value needs to be surrounded in quotes
[warning] 24-24: [UnorderedKey] The DELETE_ARCHIVED_NOTIFICATIONS_OLDER_THAN key should go before the DELETE_INTERVAL key
[warning] 24-24: [ValueWithoutQuotes] This value needs to be surrounded in quotes
🔇 Additional comments (11)
apps/api/package.json (1)
34-34: @fast-csv/format Dependency Verifiedapps/api/package.json (line 34)
- Latest dist-tag is 5.0.2, matching the declared version
- Maintainer: dustinsmith1024; description: “fast-csv formatting module”
- No known security advisories returned by the GitHub Security API
This package is legitimate and free of reported vulnerabilities. It’s safe to approve the addition.
apps/api/.gitignore (1)
39-42: LGTM! Proper exclusion of backup directory.The addition of the
backupsdirectory to.gitignoreis appropriate for the CSV backup functionality. This ensures generated backup files are not committed to version control.apps/api/scheduler.sh (4)
11-11: LGTM! Proper environment variable configuration.The
DELETE_INTERVALenvironment variable is correctly configured with appropriate default value.
25-27: LGTM! Clean function implementation.The
delete_archived_notificationsfunction follows the same pattern as other API calls in the script and correctly targets the DELETE endpoint.
30-30: LGTM! Proper timestamp initialization.The
last_delete_runtimestamp is correctly initialized for tracking deletion intervals.
44-48: LGTM! Consistent scheduling logic.The deletion scheduling logic follows the same pattern as the existing archive functionality and is properly integrated into the main loop.
apps/api/src/modules/archived-notifications/archived-notifications.controller.ts (2)
1-1: LGTM! Proper import addition.The
Deletedecorator import is correctly added to support the new DELETE endpoint.
28-43: Approved: deleteArchivedNotifications endpointLGTM! Well-implemented DELETE endpoint.
The
deleteArchivedNotificationsmethod follows the established pattern in the codebase with:
- Proper error handling for
HttpExceptioninstances- Consistent logging at appropriate levels
- Clean async/await implementation
Verified
deleteArchivedNotificationsCron()exists in apps/api/src/modules/archived-notifications/archived-notifications.service.ts.apps/api/src/modules/archived-notifications/archived-notifications.service.ts (3)
155-173: LGTM!The cron method properly checks the environment variable and handles errors appropriately.
257-288: LGTM!The CSV writing implementation efficiently handles nested JSON fields and uses streams for better memory management.
231-238: Fix the status condition in the update queryThe update query uses
Not(Status.INACTIVE)which would update entries that are already soft-deleted (INACTIVE). This seems incorrect based on the earlier query that only fetches ACTIVE entries.await queryRunner.manager.update( ArchivedNotification, { createdOn: LessThan(cutoffTimestamp), - status: Not(Status.INACTIVE), + status: Status.ACTIVE, }, { status: Status.INACTIVE }, );⛔ Skipped due to learnings
Learnt from: kshitij-k-osmosys PR: OsmosysSoftware/osmo-x#360 File: apps/api/src/modules/notifications/notifications.service.ts:0-0 Timestamp: 2024-11-20T06:08:32.855Z Learning: The `findNotificationsToArchive` function has been removed from `notifications.service.ts`, and archiving tasks now use `QueryRunner` directly in `archived-notifications.service.ts`.Learnt from: kshitij-k-osmosys PR: OsmosysSoftware/osmo-x#368 File: apps/portal/src/app/graphql/graphql.queries.ts:50-50 Timestamp: 2024-11-28T12:52:15.785Z Learning: In `apps/portal/src/app/graphql/graphql.queries.ts`, the `GetArchivedNotifications` query uses the field `notificationId` intentionally instead of `id` because `archivedNotifications` has a different structure than `notifications`.Learnt from: kshitij-k-osmosys PR: OsmosysSoftware/osmo-x#368 File: apps/portal/src/app/views/notifications/notification.model.ts:21-32 Timestamp: 2024-11-28T12:52:01.779Z Learning: In the TypeScript file `apps/portal/src/app/views/notifications/notification.model.ts`, the `ArchivedNotification` interface intentionally has a different structure from the `Notification` interface, specifically using `notificationId` instead of `id`.Learnt from: osm-vishnukyatannawar PR: OsmosysSoftware/osmo-x#354 File: apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts:25-27 Timestamp: 2024-11-08T05:23:26.338Z Learning: In the 'archived_notifications' table, we do not add a foreign key constraint on 'notification_id' to 'notify_notifications' because the original notifications may have been deleted, and we want to retain archived notifications even if the originals are removed.Learnt from: kshitij-k-osmosys PR: OsmosysSoftware/osmo-x#408 File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0 Timestamp: 2025-04-02T04:39:38.272Z Learning: In the Osmo-X project, the `notification_sent_on` column in both `notify_archived_notifications` and `notify_notifications` tables is designed to be nullable, as explicitly defined in the migration file with `isNullable: true`.Learnt from: kshitij-k-osmosys PR: OsmosysSoftware/osmo-x#360 File: apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts:32-35 Timestamp: 2024-11-20T05:58:43.756Z Learning: In the codebase, the `Notification` and `ArchivedNotification` entities use `default: null` in the `@Column` decorators for nullable fields, and `@Field()` without `{ nullable: true }`. This pattern is accepted in this project.
apps/api/src/modules/archived-notifications/archived-notifications.service.ts
Outdated
Show resolved
Hide resolved
kshitij-k-osmosys
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/api/.env.example (1)
22-24: Static-analysis warnings: quote the values or silence the linter
dotenv-linterflags these three lines for missing quotes and key ordering.
Ordering is cosmetic, but quoting boolean/number defaults would silence the warning and avoids edge-cases when a future value contains spaces or#.-ENABLE_ARCHIVED_NOTIFICATION_DELETION=false -DELETE_INTERVAL=2592000 -DELETE_ARCHIVED_NOTIFICATIONS_OLDER_THAN=90d +ENABLE_ARCHIVED_NOTIFICATION_DELETION="false" +DELETE_INTERVAL="2592000" +DELETE_ARCHIVED_NOTIFICATIONS_OLDER_THAN="90d"Stay consistent with the rest of the file (currently unquoted) or adopt quotes project-wide—either is fine, but pick one and configure the linter accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/.env.example(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:38.272Z
Learning: In the Osmo-X project, the `notification_sent_on` column in both `notify_archived_notifications` and `notify_notifications` tables is designed to be nullable, as explicitly defined in the migration file with `isNullable: true`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#360
File: apps/api/src/modules/notifications/notifications.service.ts:0-0
Timestamp: 2024-11-20T06:08:32.855Z
Learning: The `findNotificationsToArchive` function has been removed from `notifications.service.ts`, and archiving tasks now use `QueryRunner` directly in `archived-notifications.service.ts`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the migration file for adding the 'notification_sent_on' column to 'notify_archived_notifications' and 'notify_notifications' tables, the column is defined as nullable.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the 'AddNotificationSentOnColumn1743145236864' migration file, the 'notification_sent_on' column is intentionally defined as nullable (isNullable: true) for both the 'notify_archived_notifications' and 'notify_notifications' tables.
Learnt from: osm-vishnukyatannawar
PR: OsmosysSoftware/osmo-x#354
File: apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts:25-27
Timestamp: 2024-11-08T05:23:26.338Z
Learning: In the 'archived_notifications' table, we do not add a foreign key constraint on 'notification_id' to 'notify_notifications' because the original notifications may have been deleted, and we want to retain archived notifications even if the originals are removed.
apps/api/.env.example (8)
Learnt from: Harish-osmosys
PR: OsmosysSoftware/osmo-x#326
File: apps/api/.env.example:14-17
Timestamp: 2024-09-23T08:33:52.794Z
Learning: The `.env.example` file is intended as a reference, and variables like `ALGORITHM`, `KEY`, and `IV` can be modified as needed by the user.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the migration file for adding the 'notification_sent_on' column to 'notify_archived_notifications' and 'notify_notifications' tables, the column is defined as nullable.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the 'AddNotificationSentOnColumn1743145236864' migration file, the 'notification_sent_on' column is intentionally defined as nullable (isNullable: true) for both the 'notify_archived_notifications' and 'notify_notifications' tables.
Learnt from: ayushnvs
PR: OsmosysSoftware/osmo-x#262
File: apps/api/src/jobs/consumers/notifications/mailgun-notifications.job.consumer.ts:0-0
Timestamp: 2024-07-03T13:27:46.550Z
Learning: The `notificationSendResponse` variable in the `processMailgunNotificationConfirmationQueue` method of the `MailgunNotificationConsumer` class was changed from `let` to `const` to reflect that it is never reassigned.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#360
File: apps/api/src/modules/notifications/notifications.service.ts:0-0
Timestamp: 2024-11-20T06:08:32.855Z
Learning: The `findNotificationsToArchive` function has been removed from `notifications.service.ts`, and archiving tasks now use `QueryRunner` directly in `archived-notifications.service.ts`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:38.272Z
Learning: In the Osmo-X project, the `notification_sent_on` column in both `notify_archived_notifications` and `notify_notifications` tables is designed to be nullable, as explicitly defined in the migration file with `isNullable: true`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#368
File: apps/portal/src/app/views/notifications/notification.model.ts:21-32
Timestamp: 2024-11-28T12:52:01.779Z
Learning: In the TypeScript file `apps/portal/src/app/views/notifications/notification.model.ts`, the `ArchivedNotification` interface intentionally has a different structure from the `Notification` interface, specifically using `notificationId` instead of `id`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#360
File: apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts:32-35
Timestamp: 2024-11-20T05:58:43.756Z
Learning: In the codebase, the `Notification` and `ArchivedNotification` entities use `default: null` in the `@Column` decorators for nullable fields, and `@Field()` without `{ nullable: true }`. This pattern is accepted in this project.
🪛 dotenv-linter (3.3.0)
apps/api/.env.example
[warning] 22-22: [UnorderedKey] The ENABLE_ARCHIVED_NOTIFICATION_DELETION key should go before the MAX_RETRY_COUNT key
[warning] 22-22: [ValueWithoutQuotes] This value needs to be surrounded in quotes
[warning] 23-23: [UnorderedKey] The DELETE_INTERVAL key should go before the ENABLE_ARCHIVED_NOTIFICATION_DELETION key
[warning] 23-23: [ValueWithoutQuotes] This value needs to be surrounded in quotes
[warning] 24-24: [UnorderedKey] The DELETE_ARCHIVED_NOTIFICATIONS_OLDER_THAN key should go before the DELETE_INTERVAL key
[warning] 24-24: [ValueWithoutQuotes] This value needs to be surrounded in quotes
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/api/src/modules/archived-notifications/archived-notifications.service.ts (2)
223-235: Backup directory path & error handling
- Using a relative
"backups"path couples runtime CWD to project layout. Usepath.resolve(process.cwd(), 'backups')or make it configurable via env for container deployments.- You already wrapped
mkdirSyncintry/catch—good—but bubble up the original error (error instanceof Error ? error.message : error) to avoid losing stack context.
12-15: Import style inconsistent
import ms = require('ms');mixes CommonJS syntax with ESModule imports; elsewhere the codebase usesimport ms from 'ms';.
Aligning the style avoids TS compiler flags whenesModuleInteropis toggled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/modules/archived-notifications/archived-notifications.service.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:38.272Z
Learning: In the Osmo-X project, the `notification_sent_on` column in both `notify_archived_notifications` and `notify_notifications` tables is designed to be nullable, as explicitly defined in the migration file with `isNullable: true`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#360
File: apps/api/src/modules/notifications/notifications.service.ts:0-0
Timestamp: 2024-11-20T06:08:32.855Z
Learning: The `findNotificationsToArchive` function has been removed from `notifications.service.ts`, and archiving tasks now use `QueryRunner` directly in `archived-notifications.service.ts`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the migration file for adding the 'notification_sent_on' column to 'notify_archived_notifications' and 'notify_notifications' tables, the column is defined as nullable.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the 'AddNotificationSentOnColumn1743145236864' migration file, the 'notification_sent_on' column is intentionally defined as nullable (isNullable: true) for both the 'notify_archived_notifications' and 'notify_notifications' tables.
Learnt from: osm-vishnukyatannawar
PR: OsmosysSoftware/osmo-x#354
File: apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts:25-27
Timestamp: 2024-11-08T05:23:26.338Z
Learning: In the 'archived_notifications' table, we do not add a foreign key constraint on 'notification_id' to 'notify_notifications' because the original notifications may have been deleted, and we want to retain archived notifications even if the originals are removed.
apps/api/src/modules/archived-notifications/archived-notifications.service.ts (14)
Learnt from: kshitij-k-osmosys
PR: #360
File: apps/api/src/modules/notifications/notifications.service.ts:0-0
Timestamp: 2024-11-20T06:08:32.855Z
Learning: The findNotificationsToArchive function has been removed from notifications.service.ts, and archiving tasks now use QueryRunner directly in archived-notifications.service.ts.
Learnt from: kshitij-k-osmosys
PR: #368
File: apps/portal/src/app/views/notifications/notification.model.ts:21-32
Timestamp: 2024-11-28T12:52:01.779Z
Learning: In the TypeScript file apps/portal/src/app/views/notifications/notification.model.ts, the ArchivedNotification interface intentionally has a different structure from the Notification interface, specifically using notificationId instead of id.
Learnt from: LakshayaT
PR: #293
File: apps/api/src/modules/notifications/notifications.controller.ts:15-18
Timestamp: 2024-07-29T13:44:11.361Z
Learning: The addNotificationsToQueue method in the notificationService includes comprehensive error handling, logging errors, and managing the state of the queue processing.
Learnt from: kshitij-k-osmosys
PR: #368
File: apps/portal/src/app/graphql/graphql.queries.ts:50-50
Timestamp: 2024-11-28T12:52:15.785Z
Learning: In apps/portal/src/app/graphql/graphql.queries.ts, the GetArchivedNotifications query uses the field notificationId intentionally instead of id because archivedNotifications has a different structure than notifications.
Learnt from: kshitij-k-osmosys
PR: #332
File: apps/portal/src/app/views/notifications/notifications.component.ts:0-0
Timestamp: 2024-09-27T12:15:47.625Z
Learning: When the code in notifications.component.ts is outdated, previous suggestions related to it may not apply.
Learnt from: kshitij-k-osmosys
PR: #332
File: apps/portal/src/app/views/notifications/notifications.component.ts:0-0
Timestamp: 2024-10-08T17:27:45.631Z
Learning: When the code in notifications.component.ts is outdated, previous suggestions related to it may not apply.
Learnt from: ayushnvs
PR: #262
File: apps/api/src/jobs/consumers/notifications/mailgun-notifications.job.consumer.ts:0-0
Timestamp: 2024-07-03T13:27:46.550Z
Learning: The notificationSendResponse variable in the processMailgunNotificationConfirmationQueue method of the MailgunNotificationConsumer class was changed from let to const to reflect that it is never reassigned.
Learnt from: kshitij-k-osmosys
PR: #360
File: apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts:32-35
Timestamp: 2024-11-20T05:58:43.756Z
Learning: In the codebase, the Notification and ArchivedNotification entities use default: null in the @Column decorators for nullable fields, and @Field() without { nullable: true }. This pattern is accepted in this project.
Learnt from: kshitij-k-osmosys
PR: #367
File: apps/api/src/modules/archived-notifications/dtos/archived-notification-response.dto.ts:18-23
Timestamp: 2024-11-28T12:49:15.719Z
Learning: In the TypeScript classes ArchivedNotificationResponse and NotificationResponse within our NestJS application, we do not include input validation in constructors for response DTOs. It's acceptable to proceed without input validation to maintain consistency.
Learnt from: Harish-osmosys
PR: #278
File: apps/api/src/modules/webhook/webhook.service.ts:0-0
Timestamp: 2024-07-12T09:48:15.458Z
Learning: The WebhookService class in apps/api/src/modules/webhook/webhook.service.ts now uses the Logger from @nestjs/common for logging instead of console.error.
Learnt from: LakshayaT
PR: #238
File: apps/api/src/jobs/producers/notifications/notifications.job.producer.ts:0-0
Timestamp: 2024-10-08T17:27:45.631Z
Learning: Maintain a blank line before each break statement in switch-case blocks as part of the formatting style in the codebase of the notifications.job.producer.ts file and potentially other similar structures.
Learnt from: LakshayaT
PR: #238
File: apps/api/src/jobs/producers/notifications/notifications.job.producer.ts:0-0
Timestamp: 2024-06-10T19:12:20.809Z
Learning: Maintain a blank line before each break statement in switch-case blocks as part of the formatting style in the codebase of the notifications.job.producer.ts file and potentially other similar structures.
Learnt from: Harish-osmosys
PR: #278
File: apps/api/src/modules/webhook/webhook.service.ts:0-0
Timestamp: 2024-10-08T17:27:45.632Z
Learning: The WebhookService class in apps/api/src/modules/webhook/webhook.service.ts now uses the Logger from @nestjs/common for logging instead of console.error.
Learnt from: LakshayaT
PR: #263
File: apps/api/src/modules/notifications/queues/queue.service.ts:96-103
Timestamp: 2024-07-02T10:51:07.210Z
Learning: The processAwaitingConfirmationNotificationQueue method in the NotificationConsumer class includes robust error handling, logging errors, updating the delivery status, incrementing the retry count, and saving the notification.
apps/api/src/modules/archived-notifications/archived-notifications.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/modules/archived-notifications/archived-notifications.service.ts
Outdated
Show resolved
Hide resolved
kshitij-k-osmosys
changed the title
apps/api/.env.example
Outdated
| @@ -19,6 +19,9 @@ MAX_RETRY_COUNT=3 # Max retry count. DEFAULT: 3 | |||
| ARCHIVE_LIMIT=1000 # Max notifications to archive. DEFAULT: 1000 | |||
| ARCHIVE_INTERVAL=3600 # Interval (in seconds) for archiving notifications. DEFAULT: 3600 (every 1 hour) | |||
| SCHEDULE_TIME_IN_SECONDS=5 # Interval (in seconds) for processing PENDING and AWAITING_CONFIRMATION notifications. DEFAULT: 5 (every 5 seconds) | |||
| ENABLE_ARCHIVED_NOTIFICATION_DELETION=false # Toggle to enable/disable deletion of archived notifications, VALUES: "true", "false". DEFAULT: false | |||
| DELETE_INTERVAL=2592000 # Interval (in seconds) for deletion of archived notifications. DEFAULT: 2592000 (every 30 days) | |||
There was a problem hiding this comment.
This is like when to trigger this function right? Can we not just use the cron pattern here instead of something like this?
There was a problem hiding this comment.
As per our discussion on teams, decided to keep this as interval in seconds for now
Renamed in afa1f32
This also renames ARCHIVE_INTERVAL variable to stay consistent
|
|
||
| try { | ||
| // Fetch entries to delete | ||
| const archivedEntries = await queryRunner.manager.find(ArchivedNotification, { |
There was a problem hiding this comment.
Not a good idea to hold a lot of records here. This will consume a lot of RAM if we do it on OQSHA database where we have lakhs lakhs of records.
We need to do this process in a pagination way. If we simply fetch all records at once it will consume a lot of RAM to load the data. We need to do in pagination fashion how we do it in portal cannot pull so much data at once.
You need to also write to the excel or csv in a streaming fashion by not holding a lot of data in memory to dump in csv.
Just assume if there are 30 lakh records. 3 milion then we will consume a lot of RAM and also the query might timeout.
Kindly make sure
- You fetch limited records as pagination (say max 500 at a time)
- You need to write to the CSV file and not wait for all the records to be in memory and writing it just once in the end will cause issues by confusing a lot of RAM.
- You can delete thecsv file if something goes bad during this.
| return; | ||
| } | ||
|
|
||
| const idsToDelete = archivedEntries.map((entry) => entry.id); |
There was a problem hiding this comment.
This will flood the log. It is fine just say the number of records deleted and may be the start and end record since the ids will go mostly in a serial manner.
There was a problem hiding this comment.
Removed this log
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/api/src/modules/archived-notifications/archived-notifications.service.ts (1)
227-233: Directory creation error handling addressed previous concernThe implementation correctly addresses the earlier review comment by wrapping the directory creation in a try-catch block with a descriptive error message.
🧹 Nitpick comments (1)
apps/api/.env.example (1)
20-20: Consider the static analysis suggestions for consistencyThe static analysis tools suggest adding quotes around numeric values and reordering keys alphabetically. While not critical, addressing these would improve consistency with linting standards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/.env.example(1 hunks)apps/api/scheduler.sh(3 hunks)apps/api/src/modules/archived-notifications/archived-notifications.service.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/scheduler.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#448
File: apps/api/src/modules/archived-notifications/archived-notifications.service.ts:241-244
Timestamp: 2025-07-21T07:41:45.365Z
Learning: In apps/api/src/modules/archived-notifications/archived-notifications.service.ts, the project requirement is to hard delete archived notifications older than the retention period (using queryRunner.manager.delete), not to perform a soft delete by updating status.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#360
File: apps/api/src/modules/notifications/notifications.service.ts:0-0
Timestamp: 2024-11-20T06:08:32.855Z
Learning: The `findNotificationsToArchive` function has been removed from `notifications.service.ts`, and archiving tasks now use `QueryRunner` directly in `archived-notifications.service.ts`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:38.272Z
Learning: In the Osmo-X project, the `notification_sent_on` column in both `notify_archived_notifications` and `notify_notifications` tables is designed to be nullable, as explicitly defined in the migration file with `isNullable: true`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the migration file for adding the 'notification_sent_on' column to 'notify_archived_notifications' and 'notify_notifications' tables, the column is defined as nullable.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the 'AddNotificationSentOnColumn1743145236864' migration file, the 'notification_sent_on' column is intentionally defined as nullable (isNullable: true) for both the 'notify_archived_notifications' and 'notify_notifications' tables.
Learnt from: osm-vishnukyatannawar
PR: OsmosysSoftware/osmo-x#354
File: apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts:25-27
Timestamp: 2024-11-08T05:23:26.338Z
Learning: In the 'archived_notifications' table, we do not add a foreign key constraint on 'notification_id' to 'notify_notifications' because the original notifications may have been deleted, and we want to retain archived notifications even if the originals are removed.
apps/api/src/modules/archived-notifications/archived-notifications.service.ts (18)
Learnt from: kshitij-k-osmosys
PR: #448
File: apps/api/src/modules/archived-notifications/archived-notifications.service.ts:241-244
Timestamp: 2025-07-21T07:41:45.365Z
Learning: In apps/api/src/modules/archived-notifications/archived-notifications.service.ts, the project requirement is to hard delete archived notifications older than the retention period (using queryRunner.manager.delete), not to perform a soft delete by updating status.
Learnt from: kshitij-k-osmosys
PR: #360
File: apps/api/src/modules/notifications/notifications.service.ts:0-0
Timestamp: 2024-11-20T06:08:32.855Z
Learning: The findNotificationsToArchive function has been removed from notifications.service.ts, and archiving tasks now use QueryRunner directly in archived-notifications.service.ts.
Learnt from: kshitij-k-osmosys
PR: #368
File: apps/portal/src/app/views/notifications/notification.model.ts:21-32
Timestamp: 2024-11-28T12:52:01.779Z
Learning: In the TypeScript file apps/portal/src/app/views/notifications/notification.model.ts, the ArchivedNotification interface intentionally has a different structure from the Notification interface, specifically using notificationId instead of id.
Learnt from: LakshayaT
PR: #293
File: apps/api/src/modules/notifications/notifications.controller.ts:15-18
Timestamp: 2024-07-29T13:44:11.361Z
Learning: The addNotificationsToQueue method in the notificationService includes comprehensive error handling, logging errors, and managing the state of the queue processing.
Learnt from: kshitij-k-osmosys
PR: #368
File: apps/portal/src/app/graphql/graphql.queries.ts:50-50
Timestamp: 2024-11-28T12:52:15.785Z
Learning: In apps/portal/src/app/graphql/graphql.queries.ts, the GetArchivedNotifications query uses the field notificationId intentionally instead of id because archivedNotifications has a different structure than notifications.
Learnt from: kshitij-k-osmosys
PR: #332
File: apps/portal/src/app/views/notifications/notifications.component.ts:0-0
Timestamp: 2024-10-08T17:27:45.631Z
Learning: When the code in notifications.component.ts is outdated, previous suggestions related to it may not apply.
Learnt from: kshitij-k-osmosys
PR: #332
File: apps/portal/src/app/views/notifications/notifications.component.ts:0-0
Timestamp: 2024-09-27T12:15:47.625Z
Learning: When the code in notifications.component.ts is outdated, previous suggestions related to it may not apply.
Learnt from: ayushnvs
PR: #262
File: apps/api/src/jobs/consumers/notifications/mailgun-notifications.job.consumer.ts:0-0
Timestamp: 2024-07-03T13:27:46.550Z
Learning: The notificationSendResponse variable in the processMailgunNotificationConfirmationQueue method of the MailgunNotificationConsumer class was changed from let to const to reflect that it is never reassigned.
Learnt from: kshitij-k-osmosys
PR: #360
File: apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts:32-35
Timestamp: 2024-11-20T05:58:43.756Z
Learning: In the codebase, the Notification and ArchivedNotification entities use default: null in the @Column decorators for nullable fields, and @Field() without { nullable: true }. This pattern is accepted in this project.
Learnt from: kshitij-k-osmosys
PR: #367
File: apps/api/src/modules/archived-notifications/dtos/archived-notification-response.dto.ts:18-23
Timestamp: 2024-11-28T12:49:15.719Z
Learning: In the TypeScript classes ArchivedNotificationResponse and NotificationResponse within our NestJS application, we do not include input validation in constructors for response DTOs. It's acceptable to proceed without input validation to maintain consistency.
Learnt from: LakshayaT
PR: #238
File: apps/api/src/jobs/producers/notifications/notifications.job.producer.ts:0-0
Timestamp: 2024-06-10T19:12:20.809Z
Learning: Maintain a blank line before each break statement in switch-case blocks as part of the formatting style in the codebase of the notifications.job.producer.ts file and potentially other similar structures.
Learnt from: LakshayaT
PR: #238
File: apps/api/src/jobs/producers/notifications/notifications.job.producer.ts:0-0
Timestamp: 2024-10-08T17:27:45.631Z
Learning: Maintain a blank line before each break statement in switch-case blocks as part of the formatting style in the codebase of the notifications.job.producer.ts file and potentially other similar structures.
Learnt from: LakshayaT
PR: #217
File: apps/api/src/database/migrations/1692870736646-seeddata.ts:0-0
Timestamp: 2024-06-10T19:12:20.809Z
Learning: The SQL queries in the migration file 1692870736646-seeddata.ts have been updated to use parameterized queries as of commit 01a1c05b4edf08fbdca84ccb9b6d8a9af67badc9 in PR #217.
Learnt from: osm-vishnukyatannawar
PR: #354
File: apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts:25-27
Timestamp: 2024-11-08T05:23:26.338Z
Learning: In the 'archived_notifications' table, we do not add a foreign key constraint on 'notification_id' to 'notify_notifications' because the original notifications may have been deleted, and we want to retain archived notifications even if the originals are removed.
Learnt from: kshitij-k-osmosys
PR: #408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:38.272Z
Learning: In the Osmo-X project, the notification_sent_on column in both notify_archived_notifications and notify_notifications tables is designed to be nullable, as explicitly defined in the migration file with isNullable: true.
Learnt from: Harish-osmosys
PR: #278
File: apps/api/src/modules/webhook/webhook.service.ts:0-0
Timestamp: 2024-10-08T17:27:45.632Z
Learning: The WebhookService class in apps/api/src/modules/webhook/webhook.service.ts now uses the Logger from @nestjs/common for logging instead of console.error.
Learnt from: Harish-osmosys
PR: #278
File: apps/api/src/modules/webhook/webhook.service.ts:0-0
Timestamp: 2024-07-12T09:48:15.458Z
Learning: The WebhookService class in apps/api/src/modules/webhook/webhook.service.ts now uses the Logger from @nestjs/common for logging instead of console.error.
Learnt from: LakshayaT
PR: #263
File: apps/api/src/modules/notifications/queues/queue.service.ts:96-103
Timestamp: 2024-07-02T10:51:07.210Z
Learning: The processAwaitingConfirmationNotificationQueue method in the NotificationConsumer class includes robust error handling, logging errors, updating the delivery status, incrementing the retry count, and saving the notification.
apps/api/.env.example (7)
Learnt from: kshitij-k-osmosys
PR: #360
File: apps/api/src/modules/notifications/notifications.service.ts:0-0
Timestamp: 2024-11-20T06:08:32.855Z
Learning: The findNotificationsToArchive function has been removed from notifications.service.ts, and archiving tasks now use QueryRunner directly in archived-notifications.service.ts.
Learnt from: kshitij-k-osmosys
PR: #448
File: apps/api/src/modules/archived-notifications/archived-notifications.service.ts:241-244
Timestamp: 2025-07-21T07:41:45.365Z
Learning: In apps/api/src/modules/archived-notifications/archived-notifications.service.ts, the project requirement is to hard delete archived notifications older than the retention period (using queryRunner.manager.delete), not to perform a soft delete by updating status.
Learnt from: ayushnvs
PR: #262
File: apps/api/src/jobs/consumers/notifications/mailgun-notifications.job.consumer.ts:0-0
Timestamp: 2024-07-03T13:27:46.550Z
Learning: The notificationSendResponse variable in the processMailgunNotificationConfirmationQueue method of the MailgunNotificationConsumer class was changed from let to const to reflect that it is never reassigned.
Learnt from: Harish-osmosys
PR: #326
File: apps/api/.env.example:14-17
Timestamp: 2024-09-23T08:33:52.794Z
Learning: The .env.example file is intended as a reference, and variables like ALGORITHM, KEY, and IV can be modified as needed by the user.
Learnt from: kshitij-k-osmosys
PR: #368
File: apps/portal/src/app/views/notifications/notification.model.ts:21-32
Timestamp: 2024-11-28T12:52:01.779Z
Learning: In the TypeScript file apps/portal/src/app/views/notifications/notification.model.ts, the ArchivedNotification interface intentionally has a different structure from the Notification interface, specifically using notificationId instead of id.
Learnt from: kshitij-k-osmosys
PR: #408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the migration file for adding the 'notification_sent_on' column to 'notify_archived_notifications' and 'notify_notifications' tables, the column is defined as nullable.
Learnt from: kshitij-k-osmosys
PR: #408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:38.272Z
Learning: In the Osmo-X project, the notification_sent_on column in both notify_archived_notifications and notify_notifications tables is designed to be nullable, as explicitly defined in the migration file with isNullable: true.
🪛 dotenv-linter (3.3.0)
apps/api/.env.example
[warning] 20-20: [UnorderedKey] The ARCHIVE_INTERVAL_IN_SECONDS key should go before the ARCHIVE_LIMIT key
[warning] 20-20: [ValueWithoutQuotes] This value needs to be surrounded in quotes
[warning] 21-21: [ValueWithoutQuotes] This value needs to be surrounded in quotes
[warning] 22-22: [UnorderedKey] The ENABLE_ARCHIVED_NOTIFICATION_DELETION key should go before the MAX_RETRY_COUNT key
[warning] 22-22: [ValueWithoutQuotes] This value needs to be surrounded in quotes
[warning] 23-23: [UnorderedKey] The DELETE_INTERVAL_IN_SECONDS key should go before the ENABLE_ARCHIVED_NOTIFICATION_DELETION key
[warning] 23-23: [ValueWithoutQuotes] This value needs to be surrounded in quotes
[warning] 24-24: [UnorderedKey] The DELETE_ARCHIVED_NOTIFICATIONS_OLDER_THAN key should go before the DELETE_INTERVAL_IN_SECONDS key
[warning] 24-24: [ValueWithoutQuotes] This value needs to be surrounded in quotes
🔇 Additional comments (9)
apps/api/.env.example (1)
22-24: Well-designed configuration variables for the deletion featureThe new environment variables are well-structured with clear naming, reasonable defaults, and comprehensive documentation. The 90-day default retention period and 30-day deletion interval provide a good balance between data retention and cleanup frequency.
apps/api/src/modules/archived-notifications/archived-notifications.service.ts (8)
12-15: LGTM: Required imports for new functionalityThe new imports are appropriate for the deletion and CSV backup functionality.
155-173: Well-implemented cron entry point with proper feature toggleThe method correctly checks the configuration flag and provides clear logging for both enabled and disabled states. Error handling follows the established pattern.
175-194: Robust configuration validation with appropriate guardrailsThe validation logic correctly parses the retention period, enforces reasonable limits (max 10 years), and provides clear error messages for invalid configurations.
204-213: Good pagination approach addresses memory concernsThe implementation correctly addresses the previous feedback about memory usage by limiting queries to 500 records at a time, preventing excessive RAM consumption with large datasets.
259-290: Excellent streaming CSV implementationThe CSV writing implementation properly addresses memory concerns by using streams instead of loading all data into memory. The handling of nested JSON fields with proper stringification is well-designed.
292-300: Clean utility method for timestamp formattingThe timestamp formatting method is straightforward and generates readable filenames for backup files.
236-241: Confirm pagination across all batchesThe
archiveNotificationsmethod currently fetches and deletes only the first 500 records per execution and then returns. If the goal is to archive and remove all eligible notifications in a single run, you’ll need to wrap the fetch → CSV backup → delete steps in a loop (for example, ado { … } while (archivedEntries.length === 500);) so that it continues paging until no entries remain. If instead you intentionally want to limit each cron run to 500 records, please add a comment or configuration note to make that clear.• File:
apps/api/src/modules/archived-notifications/archived-notifications.service.ts
• Lines: ~236–241
215-219: Consistent early-return commit across modulesThe “no entries” case commits and returns in both services, matching existing patterns:
- In archived-notifications.service.ts at lines 79–82 and 215–218.
- In applications.service.ts at lines 158–162.
Since rollback only occurs on error and the empty-result path commits cleanly, no changes are needed here.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/modules/archived-notifications/archived-notifications.service.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#448
File: apps/api/src/modules/archived-notifications/archived-notifications.service.ts:241-244
Timestamp: 2025-07-21T07:41:45.365Z
Learning: In apps/api/src/modules/archived-notifications/archived-notifications.service.ts, the project requirement is to hard delete archived notifications older than the retention period (using queryRunner.manager.delete), not to perform a soft delete by updating status.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#360
File: apps/api/src/modules/notifications/notifications.service.ts:0-0
Timestamp: 2024-11-20T06:08:32.855Z
Learning: The `findNotificationsToArchive` function has been removed from `notifications.service.ts`, and archiving tasks now use `QueryRunner` directly in `archived-notifications.service.ts`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:38.272Z
Learning: In the Osmo-X project, the `notification_sent_on` column in both `notify_archived_notifications` and `notify_notifications` tables is designed to be nullable, as explicitly defined in the migration file with `isNullable: true`.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the migration file for adding the 'notification_sent_on' column to 'notify_archived_notifications' and 'notify_notifications' tables, the column is defined as nullable.
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:34.338Z
Learning: In the 'AddNotificationSentOnColumn1743145236864' migration file, the 'notification_sent_on' column is intentionally defined as nullable (isNullable: true) for both the 'notify_archived_notifications' and 'notify_notifications' tables.
Learnt from: osm-vishnukyatannawar
PR: OsmosysSoftware/osmo-x#354
File: apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts:25-27
Timestamp: 2024-11-08T05:23:26.338Z
Learning: In the 'archived_notifications' table, we do not add a foreign key constraint on 'notification_id' to 'notify_notifications' because the original notifications may have been deleted, and we want to retain archived notifications even if the originals are removed.
apps/api/src/modules/archived-notifications/archived-notifications.service.ts (18)
Learnt from: kshitij-k-osmosys
PR: #448
File: apps/api/src/modules/archived-notifications/archived-notifications.service.ts:241-244
Timestamp: 2025-07-21T07:41:45.365Z
Learning: In apps/api/src/modules/archived-notifications/archived-notifications.service.ts, the project requirement is to hard delete archived notifications older than the retention period (using queryRunner.manager.delete), not to perform a soft delete by updating status.
Learnt from: kshitij-k-osmosys
PR: #360
File: apps/api/src/modules/notifications/notifications.service.ts:0-0
Timestamp: 2024-11-20T06:08:32.855Z
Learning: The findNotificationsToArchive function has been removed from notifications.service.ts, and archiving tasks now use QueryRunner directly in archived-notifications.service.ts.
Learnt from: kshitij-k-osmosys
PR: #368
File: apps/portal/src/app/views/notifications/notification.model.ts:21-32
Timestamp: 2024-11-28T12:52:01.779Z
Learning: In the TypeScript file apps/portal/src/app/views/notifications/notification.model.ts, the ArchivedNotification interface intentionally has a different structure from the Notification interface, specifically using notificationId instead of id.
Learnt from: LakshayaT
PR: #293
File: apps/api/src/modules/notifications/notifications.controller.ts:15-18
Timestamp: 2024-07-29T13:44:11.361Z
Learning: The addNotificationsToQueue method in the notificationService includes comprehensive error handling, logging errors, and managing the state of the queue processing.
Learnt from: kshitij-k-osmosys
PR: #368
File: apps/portal/src/app/graphql/graphql.queries.ts:50-50
Timestamp: 2024-11-28T12:52:15.785Z
Learning: In apps/portal/src/app/graphql/graphql.queries.ts, the GetArchivedNotifications query uses the field notificationId intentionally instead of id because archivedNotifications has a different structure than notifications.
Learnt from: kshitij-k-osmosys
PR: #332
File: apps/portal/src/app/views/notifications/notifications.component.ts:0-0
Timestamp: 2024-10-08T17:27:45.631Z
Learning: When the code in notifications.component.ts is outdated, previous suggestions related to it may not apply.
Learnt from: kshitij-k-osmosys
PR: #332
File: apps/portal/src/app/views/notifications/notifications.component.ts:0-0
Timestamp: 2024-09-27T12:15:47.625Z
Learning: When the code in notifications.component.ts is outdated, previous suggestions related to it may not apply.
Learnt from: ayushnvs
PR: #262
File: apps/api/src/jobs/consumers/notifications/mailgun-notifications.job.consumer.ts:0-0
Timestamp: 2024-07-03T13:27:46.550Z
Learning: The notificationSendResponse variable in the processMailgunNotificationConfirmationQueue method of the MailgunNotificationConsumer class was changed from let to const to reflect that it is never reassigned.
Learnt from: kshitij-k-osmosys
PR: #360
File: apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts:32-35
Timestamp: 2024-11-20T05:58:43.756Z
Learning: In the codebase, the Notification and ArchivedNotification entities use default: null in the @Column decorators for nullable fields, and @Field() without { nullable: true }. This pattern is accepted in this project.
Learnt from: kshitij-k-osmosys
PR: #367
File: apps/api/src/modules/archived-notifications/dtos/archived-notification-response.dto.ts:18-23
Timestamp: 2024-11-28T12:49:15.719Z
Learning: In the TypeScript classes ArchivedNotificationResponse and NotificationResponse within our NestJS application, we do not include input validation in constructors for response DTOs. It's acceptable to proceed without input validation to maintain consistency.
Learnt from: LakshayaT
PR: #238
File: apps/api/src/jobs/producers/notifications/notifications.job.producer.ts:0-0
Timestamp: 2024-06-10T19:12:20.809Z
Learning: Maintain a blank line before each break statement in switch-case blocks as part of the formatting style in the codebase of the notifications.job.producer.ts file and potentially other similar structures.
Learnt from: LakshayaT
PR: #238
File: apps/api/src/jobs/producers/notifications/notifications.job.producer.ts:0-0
Timestamp: 2024-10-08T17:27:45.631Z
Learning: Maintain a blank line before each break statement in switch-case blocks as part of the formatting style in the codebase of the notifications.job.producer.ts file and potentially other similar structures.
Learnt from: LakshayaT
PR: #217
File: apps/api/src/database/migrations/1692870736646-seeddata.ts:0-0
Timestamp: 2024-06-10T19:12:20.809Z
Learning: The SQL queries in the migration file 1692870736646-seeddata.ts have been updated to use parameterized queries as of commit 01a1c05b4edf08fbdca84ccb9b6d8a9af67badc9 in PR #217.
Learnt from: osm-vishnukyatannawar
PR: #354
File: apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts:25-27
Timestamp: 2024-11-08T05:23:26.338Z
Learning: In the 'archived_notifications' table, we do not add a foreign key constraint on 'notification_id' to 'notify_notifications' because the original notifications may have been deleted, and we want to retain archived notifications even if the originals are removed.
Learnt from: kshitij-k-osmosys
PR: #408
File: apps/api/src/database/migrations/1743145236864-AddNotificationSentOnColumn.ts:0-0
Timestamp: 2025-04-02T04:39:38.272Z
Learning: In the Osmo-X project, the notification_sent_on column in both notify_archived_notifications and notify_notifications tables is designed to be nullable, as explicitly defined in the migration file with isNullable: true.
Learnt from: Harish-osmosys
PR: #278
File: apps/api/src/modules/webhook/webhook.service.ts:0-0
Timestamp: 2024-10-08T17:27:45.632Z
Learning: The WebhookService class in apps/api/src/modules/webhook/webhook.service.ts now uses the Logger from @nestjs/common for logging instead of console.error.
Learnt from: Harish-osmosys
PR: #278
File: apps/api/src/modules/webhook/webhook.service.ts:0-0
Timestamp: 2024-07-12T09:48:15.458Z
Learning: The WebhookService class in apps/api/src/modules/webhook/webhook.service.ts now uses the Logger from @nestjs/common for logging instead of console.error.
Learnt from: LakshayaT
PR: #263
File: apps/api/src/modules/notifications/queues/queue.service.ts:96-103
Timestamp: 2024-07-02T10:51:07.210Z
Learning: The processAwaitingConfirmationNotificationQueue method in the NotificationConsumer class includes robust error handling, logging errors, updating the delivery status, incrementing the retry count, and saving the notification.
🔇 Additional comments (4)
apps/api/src/modules/archived-notifications/archived-notifications.service.ts (4)
12-16: Import additions look good for the new deletion functionality.The new imports appropriately support CSV backup creation and time duration parsing for the deletion feature.
156-174: Well-structured conditional deletion method.The method follows established patterns with proper configuration checking, error handling, and logging. Good defensive programming approach.
273-304: Good use of streaming for CSV writing with appropriate JSON field handling.The method correctly uses streaming I/O and properly stringifies nested JSON fields. The data transformation is done in memory, which is acceptable for the current 500-record batch size.
306-315: Clean timestamp formatting utility.Simple, correct implementation for generating timestamp strings suitable for file naming.
| async deleteArchivedEntriesAndGenerateCsvBackup(): Promise<void> { | ||
| try { | ||
| const deleteArchivedNotificationsOlderThan = this.configService.get<string>( | ||
| 'DELETE_ARCHIVED_NOTIFICATIONS_OLDER_THAN', | ||
| '90d', | ||
| ); | ||
|
|
||
| const maxRetentionMs = ms('10y'); | ||
| const retentionDurationMs = ms(deleteArchivedNotificationsOlderThan); | ||
|
|
||
| // Guard rails | ||
| if ( | ||
| typeof retentionDurationMs !== 'number' || | ||
| retentionDurationMs <= 0 || | ||
| retentionDurationMs > maxRetentionMs | ||
| ) { | ||
| throw new Error( | ||
| 'Invalid retention period. It must be a positive duration not exceeding 10 years.', | ||
| ); | ||
| } | ||
|
|
||
| const cutoffTimestamp = new Date(Date.now() - retentionDurationMs); | ||
|
|
||
| // Create directory, filename and path for backups | ||
| const backupsDir = 'backups'; | ||
| const backupFileName = `archived_notifications_backup_${this.getFormattedTimestamp()}.csv`; | ||
| const backupFilePath = path.join(backupsDir, backupFileName); | ||
|
|
||
| const queryRunner = this.dataSource.createQueryRunner(); | ||
| await queryRunner.connect(); | ||
| await queryRunner.startTransaction(); | ||
|
|
||
| try { | ||
| // Fetch entries to delete | ||
| const archivedEntries = await queryRunner.manager.find(ArchivedNotification, { | ||
| where: { | ||
| createdOn: LessThan(cutoffTimestamp), | ||
| status: Status.ACTIVE, | ||
| }, | ||
| order: { | ||
| createdOn: 'ASC', | ||
| }, | ||
| take: 500, | ||
| }); | ||
|
|
||
| if (archivedEntries.length === 0) { | ||
| this.logger.log('No entries to archive.'); | ||
| await queryRunner.commitTransaction(); | ||
| return; | ||
| } | ||
|
|
||
| // Ensure the backups directory exists | ||
| try { | ||
| if (!existsSync(backupsDir)) { | ||
| mkdirSync(backupsDir, { recursive: true }); | ||
| } | ||
| } catch (error) { | ||
| throw new Error(`Failed to create backup directory at "${backupsDir}": ${error}`); | ||
| } | ||
|
|
||
| // Export to CSV before deletion | ||
| await this.writeToCsv(archivedEntries, backupFilePath); | ||
|
|
||
| // Perform deletion | ||
| const idsToDelete = archivedEntries.map((entry) => entry.id); | ||
| await queryRunner.manager.delete(ArchivedNotification, idsToDelete); | ||
|
|
||
| await queryRunner.commitTransaction(); | ||
| this.logger.log( | ||
| `Transaction successful. Deleted ${archivedEntries.length} entries. Backup at ${backupFilePath}`, | ||
| ); | ||
| } catch (error) { | ||
| await queryRunner.rollbackTransaction(); | ||
| this.logger.error('Error during deletion. Rolled back.', error.stack); | ||
|
|
||
| // Delete CSV file if it was created | ||
| if (backupFilePath) { | ||
| try { | ||
| await fs.unlink(backupFilePath); | ||
| this.logger.log('CSV backup deleted due to rollback.'); | ||
| } catch (unlinkError) { | ||
| this.logger.error( | ||
| `Failed to delete backup file after rollback: ${unlinkError.message}`, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| throw error; | ||
| } finally { | ||
| await queryRunner.release(); | ||
| } | ||
| } catch (error) { | ||
| this.logger.error(`Failed to delete archived notifications: ${error.message}`, error.stack); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Partial implementation of pagination - memory concerns remain for large datasets.
While the 500-record limit (line 218) addresses some memory concerns from previous feedback, this implementation has limitations:
- Incomplete pagination: The method only processes the first 500 qualifying records per run, requiring multiple cron executions to handle larger datasets
- Memory risk with complex data: Even 500 records could consume significant RAM if the
dataandresultJSON fields are large - CSV memory buffering: The
writeToCsvmethod still loads all 500 records into memory before writing
For production environments with millions of records, consider implementing true pagination with offset/cursor-based iteration within a single execution.
Example improvement for batch processing:
- const archivedEntries = await queryRunner.manager.find(ArchivedNotification, {
- where: {
- createdOn: LessThan(cutoffTimestamp),
- status: Status.ACTIVE,
- },
- order: {
- createdOn: 'ASC',
- },
- take: 500,
- });
+ let offset = 0;
+ const batchSize = 500;
+ let totalDeleted = 0;
+
+ while (true) {
+ const archivedEntries = await queryRunner.manager.find(ArchivedNotification, {
+ where: {
+ createdOn: LessThan(cutoffTimestamp),
+ status: Status.ACTIVE,
+ },
+ order: {
+ createdOn: 'ASC',
+ },
+ skip: offset,
+ take: batchSize,
+ });
+
+ if (archivedEntries.length === 0) break;
+
+ // Process batch...
+ offset += batchSize;
+ totalDeleted += archivedEntries.length;
+ }🏁 Script executed:
#!/bin/bash
# Search for writeToCsv usage and definition across the repository
rg -n "writeToCsv" .Length of output: 344
🏁 Script executed:
#!/bin/bash
# Display the implementation of writeToCsv for inspection
sed -n '273,340p' apps/api/src/modules/archived-notifications/archived-notifications.service.tsLength of output: 1672
Implement full batch processing loop to handle all archived entries
The current implementation only fetches and deletes a single batch of 500 records per run, which still leaves residual data and can spike memory if JSON fields are large. To fully address pagination and memory concerns:
• At line 218, wrap the fetch–export–delete logic in a while loop that continues until no more records remain.
• Stream query results in smaller chunks or use cursor/offset-based pagination to avoid loading large arrays.
• Keep a single CSV write stream open and append each batch rather than buffering full arrays in writeToCsv.
Example diff:
- const archivedEntries = await queryRunner.manager.find(ArchivedNotification, {
- where: { createdOn: LessThan(cutoffTimestamp), status: Status.ACTIVE },
- order: { createdOn: 'ASC' },
- take: 500,
- });
- if (archivedEntries.length === 0) { … }
- // export & delete single batch
+ const batchSize = 500;
+ let batch: ArchivedNotification[];
+ // open CSV stream once
+ const ws = createWriteStream(backupFilePath);
+ const csvStream = format({ headers: true });
+ csvStream.pipe(ws);
+
+ do {
+ batch = await queryRunner.manager.find(ArchivedNotification, {
+ where: { createdOn: LessThan(cutoffTimestamp), status: Status.ACTIVE },
+ order: { createdOn: 'ASC' },
+ take: batchSize,
+ });
+ if (batch.length === 0) break;
+
+ // write batch to CSV
+ for (const row of batch) {
+ csvStream.write(this.prepareCsvRow(row));
+ }
+
+ // delete batch
+ await queryRunner.manager.delete(ArchivedNotification, batch.map(e => e.id));
+ } while (batch.length === batchSize);
+
+ await csvStream.end();Key locations to update:
- Line 218: add
while/do…whilearoundfind+delete - Line 237: open CSV stream before loop
writeToCsv: refactor to accept per-row writes or replace with inline CSV writes
These changes ensure all eligible records are processed in a single execution and limit peak memory usage.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/src/modules/archived-notifications/archived-notifications.service.ts
from lines 176 to 271, the current code only processes one batch of 500 archived
entries, which leaves residual data and can cause memory spikes. To fix this,
wrap the fetch-export-delete logic starting around line 218 in a while loop that
continues until no more records remain. Before the loop (around line 237), open
a single CSV write stream and append each batch's data instead of buffering full
arrays in writeToCsv. Refactor writeToCsv to handle per-row writes or replace it
with inline CSV writes within the loop. This will ensure all eligible records
are processed in one run and reduce memory usage.
API PR Checklist
Task Link
Osmosys Developers must include the Pinestem task link in the PR.
OSXT-345
Pre-requisites
.env.examplefile with the required values as applicable.PR Details
PR details have been updated as per the given format (see below)
feat: add admin login endpoint)Additional Information
ready for reviewshould be added if the PR is ready to be reviewed)Note: Reviewer should ensure that the checklist and description have been populated and followed correctly, and the PR should be merged only after resolving all conversations and verifying that CI checks pass.
Description:
Delete archived notifications older than env defined timeperiod
Related changes:
Screenshots
[{"timestamp":"1","level":"2","severity":"3","tracebackId":"4","message":"5"},"2025-07-18T13:38:37.536Z","debug","low","eeb49a45-5153-4c86-9863-001413099a59","Deleting archived notifications..."] [{"timestamp":"1","level":"2","severity":"3","tracebackId":"4","source":"5","message":"6"},"2025-07-18T13:38:37.538Z","info","low","aefeaa84-549c-4041-bfe2-dc67d490efa0","ArchivedNotificationsService","Running archived notification deletion cron task"] [{"timestamp":"1","level":"2","severity":"3","tracebackId":"4","source":"5","message":"6"},"2025-07-18T13:38:37.551Z","info","low","79f176cc-26a0-4ba3-9458-43392fa85570","ArchivedNotificationsService","Archived Notification IDs to be deleted: 19,18,17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,1"] [{"timestamp":"1","level":"2","severity":"3","tracebackId":"4","source":"5","message":"6"},"2025-07-18T13:38:37.679Z","info","low","aebf664b-e63e-4d7e-be2b-55c9b5ff7e0a","ArchivedNotificationsService","Transaction successful. Soft-deleted 19 entries. Backup at backups/archived_notifications_backup_202507181908.csv"] [{"timestamp":"1","level":"2","severity":"3","tracebackId":"4","source":"5","message":"6"},"2025-07-18T13:38:37.680Z","info","low","f9d5d6be-fa7b-4d1a-9833-ff923249a857","ArchivedNotificationsService","Archive notification deletion cron task completed"] [{"timestamp":"1","level":"2","severity":"3","tracebackId":"4","message":"5"},"2025-07-18T13:38:37.680Z","info","low","4472c295-246c-4e80-b93b-d70b1e46c5fe","End of delete archived notifications Cron"]Database
Csv file
Summary by CodeRabbit
New Features
Chores