Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: archive SUCCESS/FAILED notifications #354

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/api/.env.example
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ NODE_ENV= # Use "development" for graphql playground to work

# Notification configuration
MAX_RETRY_COUNT=3 # Max retry count, default is 3
ARCHIVE_LIMIT=1000 # Max notifications to archive, default is 1000

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also suggested that we keep the cron time for this job in .env and default to 1 hour. Because for different instances based on their load we might want to override when we want to trigger this job. For example for the SaaS instance we would like to run it once in a day. But for OQSHA instance we might want to run it every 1 hour or 2 hour.

So lets try to keep this customization. Can be done as part of different task.


# Logger configuration
LOG_LEVEL=info # Log level, default is info
Expand Down
2 changes: 2 additions & 0 deletions apps/api/src/app.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { AuthModule } from './modules/auth/auth.module';
import { WebhookModule } from './modules/webhook/webhook.module';
import { APP_INTERCEPTOR } from '@nestjs/core';
import { DatabaseErrorInterceptor } from './database/database-error.interceptor';
import { ArchivedNotificationsModule } from './modules/archived-notifications/archived-notifications.module';

const configService = new ConfigService();
@Module({
Expand All @@ -39,6 +40,7 @@ const configService = new ConfigService();
playground: configService.getOrThrow('NODE_ENV') === 'development',
}),
AuthModule,
ArchivedNotificationsModule,
],
controllers: [AppController],
providers: [
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
import { MigrationInterface, QueryRunner, Table, TableForeignKey } from 'typeorm';

export class ArchiveCompletedNotifications1730724383210 implements MigrationInterface {
public async up(queryRunner: QueryRunner): Promise<void> {
const table = await queryRunner.getTable('notify_notification_retries');
const foreignKey = table.foreignKeys.find(
(fk) => fk.columnNames.indexOf('notification_id') !== -1,
);
await queryRunner.dropForeignKey('notify_notification_retries', foreignKey);

await queryRunner.createTable(
new Table({
name: 'archived_notifications',
xixas marked this conversation as resolved.
Show resolved Hide resolved
columns: [
{
name: 'id',
type: 'int',
isPrimary: true,
isGenerated: true,
generationStrategy: 'increment',
},
{
name: 'notification_id',
type: 'int',
},
xixas marked this conversation as resolved.
Show resolved Hide resolved
{
name: 'channel_type',
type: 'tinyint',
},
{
name: 'data',
type: 'text',
isNullable: true,
},
{
name: 'delivery_status',
type: 'tinyint',
default: 1,
},
{
name: 'result',
type: 'text',
isNullable: true,
},
{
name: 'created_on',
type: 'timestamp',
default: 'CURRENT_TIMESTAMP',
},
{
name: 'updated_on',
type: 'timestamp',
default: 'CURRENT_TIMESTAMP',
onUpdate: 'CURRENT_TIMESTAMP',
},
{
name: 'created_by',
type: 'varchar',
},
{
name: 'updated_by',
type: 'varchar',
},
{
name: 'status',
type: 'tinyint',
default: 1,
},
{
name: 'application_id',
type: 'int',
isNullable: true,
},
{
name: 'provider_id',
type: 'int',
isNullable: true,
},
{
name: 'retry_count',
type: 'int',
length: '3',
default: 0,
Copy link

@coderabbitai coderabbitai bot Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove the unsupported 'length' property from the 'retry_count' column.

The length property is not applicable to columns of type 'int' in TypeORM. It is primarily used with string types like 'varchar'. Including it may cause errors or unexpected behavior during migration.

Apply this diff to remove the unsupported property:

{
  name: 'retry_count',
  type: 'int',
- length: '3',
  default: 0,
},
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
type: 'int',
length: '3',
default: 0,
type: 'int',
default: 0,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed d6de3fc

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kshitij-k-osmosys, thank you for fixing the issue.


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

},
],
}),
);

// Create Foreign keys for archived_notifications
await queryRunner.createForeignKey(
'archived_notifications',
new TableForeignKey({
columnNames: ['channel_type'],
referencedColumnNames: ['master_id'],
referencedTableName: 'notify_master_providers',
onDelete: 'CASCADE',
}),
);

await queryRunner.createForeignKey(
'archived_notifications',
new TableForeignKey({
columnNames: ['provider_id'],
referencedColumnNames: ['provider_id'],
referencedTableName: 'notify_providers',
onDelete: 'CASCADE',
}),
);
osm-vishnukyatannawar marked this conversation as resolved.
Show resolved Hide resolved
}

public async down(queryRunner: QueryRunner): Promise<void> {
// To drop the auto generated foreign key for archived_notifications
const archived_notifications_table = await queryRunner.getTable('archived_notifications');
const archived_notifications_providerIdforeignKey =
archived_notifications_table?.foreignKeys.find(
(fk) => fk.columnNames.indexOf('provider_id') !== -1,
);

// providerIdforeignKey
xixas marked this conversation as resolved.
Show resolved Hide resolved
if (archived_notifications_providerIdforeignKey) {
await queryRunner.dropForeignKey(
'archived_notifications',
archived_notifications_providerIdforeignKey,
);
}

// channelTypeforeignKey
xixas marked this conversation as resolved.
Show resolved Hide resolved
const archived_notifications_channelTypeforeignKey =
archived_notifications_table?.foreignKeys.find(
(fk) => fk.columnNames.indexOf('channel_type') !== -1,
);

if (archived_notifications_channelTypeforeignKey) {
await queryRunner.dropForeignKey(
'archived_notifications',
archived_notifications_channelTypeforeignKey,
);
}

await queryRunner.dropTable('archived_notifications');

await queryRunner.createForeignKey(
'notify_notification_retries',
new TableForeignKey({
columnNames: ['notification_id'],
referencedColumnNames: ['id'],
referencedTableName: 'notify_notifications',
onDelete: 'CASCADE',
}),
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { Status } from 'src/common/constants/database';
import { Field, ObjectType } from '@nestjs/graphql';
import { Notification } from 'src/modules/notifications/entities/notification.entity';
import { ServerApiKey } from 'src/modules/server-api-keys/entities/server-api-key.entity';
import { ArchivedNotification } from 'src/modules/archived-notifications/entities/archived-notification.entity';

@Entity({ name: 'notify_applications' })
@ObjectType()
Expand Down Expand Up @@ -52,6 +53,12 @@ export class Application {
@OneToMany(() => ServerApiKey, (serverApiKey) => serverApiKey.applicationDetails)
serverApiKey: ServerApiKey[];

@OneToMany(
() => ArchivedNotification,
(archivedNotification) => archivedNotification.applicationDetails,
)
archivedNotifications: ArchivedNotification[];

constructor(application: Partial<Application>) {
Object.assign(this, application);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { forwardRef, Logger, Module } from '@nestjs/common';
import { ArchivedNotificationsService } from './archived-notifications.service';
import { ArchivedNotification } from './entities/archived-notification.entity';
import { TypeOrmModule } from '@nestjs/typeorm';
import { NotificationsModule } from '../notifications/notifications.module';

@Module({
imports: [
TypeOrmModule.forFeature([ArchivedNotification]),
forwardRef(() => NotificationsModule),
],
providers: [ArchivedNotificationsService, Logger],
exports: [ArchivedNotificationsService],
})
export class ArchivedNotificationsModule {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { Test, TestingModule } from '@nestjs/testing';
import { ArchivedNotificationsService } from './archived-notifications.service';

describe('ArchivedNotificationsService', () => {
let service: ArchivedNotificationsService;

beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
providers: [ArchivedNotificationsService],
}).compile();
Comment on lines +8 to +10
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add required dependencies to the test module.

The service likely requires dependencies for database operations and configuration. Consider adding:

  • Repository for notifications
  • Repository for archived notifications
  • ConfigService for archive limits

Apply this diff:

 const module: TestingModule = await Test.createTestingModule({
-  providers: [ArchivedNotificationsService],
+  providers: [
+    ArchivedNotificationsService,
+    {
+      provide: getRepositoryToken(Notification),
+      useValue: createMock<Repository<Notification>>(),
+    },
+    {
+      provide: getRepositoryToken(ArchivedNotification),
+      useValue: createMock<Repository<ArchivedNotification>>(),
+    },
+    {
+      provide: ConfigService,
+      useValue: createMock<ConfigService>({
+        get: jest.fn().mockReturnValue(100), // mock archive limit
+      }),
+    },
+  ],
 }).compile();

Committable suggestion skipped: line range outside the PR's diff.


service = module.get<ArchivedNotificationsService>(ArchivedNotificationsService);
});

it('should be defined', () => {
expect(service).toBeDefined();
});
});
xixas marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import { forwardRef, Inject, Injectable, Logger } from '@nestjs/common';
import { ArchivedNotification } from './entities/archived-notification.entity';
import { Cron, CronExpression } from '@nestjs/schedule';
import { Repository } from 'typeorm';
import { InjectRepository } from '@nestjs/typeorm';
import { Notification } from 'src/modules/notifications/entities/notification.entity';
import { NotificationsService } from '../notifications/notifications.service';
import { ConfigService } from '@nestjs/config';

const configService = new ConfigService();
xixas marked this conversation as resolved.
Show resolved Hide resolved

@Injectable()
export class ArchivedNotificationsService {
protected readonly logger = new Logger(ArchivedNotificationsService.name);

constructor(
@InjectRepository(ArchivedNotification)
private readonly archivedNotificationRepository: Repository<ArchivedNotification>,
@Inject(forwardRef(() => NotificationsService))
protected readonly notificationsService: NotificationsService,
) {}

private convertToArchivedNotifications(notifications: Notification[]): ArchivedNotification[] {
return notifications.map((notification) => {
const archivedNotification = new ArchivedNotification();
archivedNotification.applicationId = notification.applicationId;
archivedNotification.channelType = notification.channelType;
archivedNotification.createdBy = notification.createdBy;
archivedNotification.createdOn = notification.createdOn;
archivedNotification.data = notification.data;
archivedNotification.deliveryStatus = notification.deliveryStatus;
archivedNotification.notification_id = notification.id;
archivedNotification.providerId = notification.providerId;
archivedNotification.result = notification.result;
archivedNotification.retryCount = notification.retryCount;
archivedNotification.updatedBy = notification.updatedBy;
archivedNotification.updatedOn = notification.updatedOn;
archivedNotification.status = notification.status;

this.logger.debug(
`Created ArchivedNotification array using Notification ID: ${notification.id}, deliveryStatus: ${notification.deliveryStatus}`,
);
return archivedNotification;
});
}

async moveNotificationsToArchive(): Promise<void> {
const archiveLimit = configService.get<number>('ARCHIVE_LIMIT', 1000);

try {
// Step 1: Retrieve the notifications to archive
this.logger.log(`Retrieve the top ${archiveLimit} notifications to archive`);
const notificationsToArchive =
await this.notificationsService.findNotificationsToArchive(archiveLimit);

if (notificationsToArchive.length === 0) {
this.logger.log('No notifications to archive at this time.');
return;
}

// Step 2: Convert notifications to archived notifications
const archivedNotificationsArray =
this.convertToArchivedNotifications(notificationsToArchive);

// Step 3: Insert notifications into the archive table
this.logger.log(`Inserting archived notifications into the archive table`);
await this.archivedNotificationRepository.save(archivedNotificationsArray, {
transaction: true,
});

// Step 4: Delete notifications from the main table by IDs
this.logger.log(`Deleting notifications from the main table by IDs`);
await this.notificationsService.deleteArchivedNotifications(notificationsToArchive);

xixas marked this conversation as resolved.
Show resolved Hide resolved
this.logger.log(`Archive notifications task completed`);
} catch (error) {
this.logger.error('Failed to archive notifications:', error);
}
}

@Cron(CronExpression.EVERY_HOUR)
async ArchiveCompletedNotificationsCron(): Promise<void> {
this.logger.log('Running archive notifications task');
this.moveNotificationsToArchive();
xixas marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading
Loading