feat(notifications): add minimal notification entity and service#127
feat(notifications): add minimal notification entity and service#127Spycall wants to merge 1 commit intoStarkHive:mainfrom
Conversation
WalkthroughThe changes simplify the notification system by reducing the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NotificationsService
participant NotificationRepository
Client->>NotificationsService: createNotification(userId, message, type)
NotificationsService->>NotificationRepository: save({userId, message, type})
NotificationRepository-->>NotificationsService: Notification
NotificationsService-->>Client: Notification
Client->>NotificationsService: getUserNotifications(userId, limit)
NotificationsService->>NotificationRepository: find({userId}, {order: createdAt DESC, take: limit})
NotificationRepository-->>NotificationsService: [Notification]
NotificationsService-->>Client: [Notification]
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: 2
🔭 Outside diff range comments (3)
src/notifications/notifications.module.ts (1)
1-11: Restore required module dependencies in NotificationsModuleThe current module setup only registers the Notification entity, but the service uses several other repositories and injected services whose providers are no longer available:
• TypeOrm repositories missing
- NotificationDelivery (
notification-delivery.entity.ts)- Preferences (
preferences.entity.ts)
• Injected services missing their modules- MailService ⇒ import MailModule
- SmsService ⇒ import SmsModule
• Auth entities used for typing (e.g.User) require either importing AuthModule or registering the User entity with TypeOrm
• TheApplicationsModuleimport removal is safe if you only use theApplicationStatusenum (no repository injection), otherwise re-add itPlease update src/notifications/notifications.module.ts to include these imports. For example:
import { Module } from '@nestjs/common'; -import { TypeOrmModule } from '@nestjs/typeorm'; +import { TypeOrmModule } from '@nestjs/typeorm'; +import { MailModule } from '../mail/mail.module'; +import { SmsModule } from './services/sms.module'; +import { AuthModule } from '../auth/auth.module'; // for User metadata import { NotificationsService } from './notifications.service'; -import { Notification } from './entities/notification.entity'; +import { Notification } from './entities/notification.entity'; +import { NotificationDelivery } from './entities/notification-delivery.entity'; +import { Preferences } from './entities/preferences.entity'; @Module({ - imports: [TypeOrmModule.forFeature([Notification])], + imports: [ + TypeOrmModule.forFeature([ + Notification, + NotificationDelivery, + Preferences, + ]), + MailModule, + SmsModule, + AuthModule, + ], providers: [NotificationsService], exports: [NotificationsService], }) export class NotificationsModule {}src/notifications/notifications.service.ts (1)
1-44: Add missing module imports/providers for NotificationsService dependenciesThe
NotificationsModulecurrently only registers theNotificationentity, but the service constructor also injectsNotificationDelivery,Preferences,MailService, andSmsService. Without registering these, NestJS will throw unresolved provider errors.• In
src/notifications/notifications.module.ts
– ExtendTypeOrmModule.forFeatureto includeNotificationDeliveryandPreferences.
– Import and include the modules that provideMailServiceandSmsService(e.g.MailModule,SmsModule).Example diff:
import { Module, forwardRef } from '@nestjs/common'; import { TypeOrmModule } from '@nestjs/typeorm'; -import { NotificationsService } from './notifications.service'; -import { Notification } from './entities/notification.entity'; +import { NotificationsService } from './notifications.service'; +import { Notification } from './entities/notification.entity'; +import { NotificationDelivery } from './entities/notification-delivery.entity'; +import { Preferences } from './entities/preferences.entity'; +import { MailModule } from '../mail/mail.module'; +import { SmsModule } from './services/sms.module'; @Module({ - imports: [TypeOrmModule.forFeature([Notification])], + imports: [ + TypeOrmModule.forFeature([Notification, NotificationDelivery, Preferences]), + forwardRef(() => MailModule), + forwardRef(() => SmsModule), + ], providers: [NotificationsService], exports: [NotificationsService], }) export class NotificationsModule {}This will align your module imports/providers with the service’s constructor.
src/notifications/entities/notification.entity.ts (1)
9-33: Add a migration for Notification entity changesNo existing migrations touch the
notificationstable, so you’ll need to author one to keep your database schema in sync with the new entity:• Create a TypeORM migration that alters the
notificationstable to match the updatedNotificationfields (e.g., new/modified columns, defaults).
• If you’re dropping or renaming any columns, ensure you handle data migration or deprecation in stages (make columns nullable → backfill → drop).
• Verify the migration runs cleanly against a copy of your production data.
🧹 Nitpick comments (4)
src/notifications/notifications.service.ts (2)
219-222: Fix code formatting issues.The method implementation looks correct but has formatting issues flagged by ESLint. The logic for creating notifications with default type is sound.
- async createNotification(userId: string, message: string, type: NotificationType = NotificationType.GENERAL): Promise<Notification> { - const notification = this.notificationRepository.create({ userId, message, type }); + async createNotification( + userId: string, + message: string, + type: NotificationType = NotificationType.GENERAL, + ): Promise<Notification> { + const notification = this.notificationRepository.create({ + userId, + message, + type, + });
224-230: Fix code formatting and approve method logic.The method correctly retrieves user notifications with proper ordering and limiting. However, there are formatting issues to address.
- async getUserNotifications(userId: string, limit = 20): Promise<Notification[]> { + async getUserNotifications( + userId: string, + limit = 20, + ): Promise<Notification[]> {src/notifications/entities/notification.entity.ts (2)
1-1: Fix import formatting.-import { Entity, PrimaryGeneratedColumn, Column, CreateDateColumn, UpdateDateColumn, Index } from 'typeorm'; +import { + Entity, + PrimaryGeneratedColumn, + Column, + CreateDateColumn, + UpdateDateColumn, + Index, +} from 'typeorm';
22-23: Fix column definition formatting.- @Column({ type: 'enum', enum: NotificationType, default: NotificationType.GENERAL }) + @Column({ + type: 'enum', + enum: NotificationType, + default: NotificationType.GENERAL, + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/notifications/entities/notification.entity.ts(2 hunks)src/notifications/notifications.module.ts(1 hunks)src/notifications/notifications.service.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/notifications/notifications.module.ts (4)
src/auth/auth.module.ts (1)
Module(31-89)src/app.module.ts (1)
Module(35-107)src/feed/feed.module.ts (1)
Module(14-31)src/applications/applications.module.ts (1)
Module(11-17)
🪛 ESLint
src/notifications/notifications.service.ts
[error] 219-219: Replace userId:·string,·message:·string,·type:·NotificationType·=·NotificationType.GENERAL with ⏎····userId:·string,⏎····message:·string,⏎····type:·NotificationType·=·NotificationType.GENERAL,⏎··
(prettier/prettier)
[error] 220-220: Replace ·userId,·message,·type with ⏎······userId,⏎······message,⏎······type,⏎···
(prettier/prettier)
[error] 224-224: Replace userId:·string,·limit·=·20 with ⏎····userId:·string,⏎····limit·=·20,⏎··
(prettier/prettier)
src/notifications/entities/notification.entity.ts
[error] 1-1: Replace ·Entity,·PrimaryGeneratedColumn,·Column,·CreateDateColumn,·UpdateDateColumn,·Index· with ⏎··Entity,⏎··PrimaryGeneratedColumn,⏎··Column,⏎··CreateDateColumn,⏎··UpdateDateColumn,⏎··Index,⏎
(prettier/prettier)
[error] 22-22: Replace ·type:·'enum',·enum:·NotificationType,·default:·NotificationType.GENERAL with ⏎····type:·'enum',⏎····enum:·NotificationType,⏎····default:·NotificationType.GENERAL,⏎·
(prettier/prettier)
|
resolve conflicts @Spycall let's close this |
|
please resolve your conflict so we can close this! |
2 participants
add minimal notification entity and service
Summary by CodeRabbit
New Features
Refactor