feat: Push notification system#123
Conversation
|
Warning Rate limit exceeded@Bosun-Josh121 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughA comprehensive in-app notification system has been implemented. This includes new API endpoints, a controller, DTOs, middleware for event-driven notifications, a fully-featured notification service with templates, filtering, and pagination, and extensive unit tests. Swagger documentation and validation are integrated into the new routes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant AuthMiddleware
participant NotificationController
participant NotificationService
participant DB
Client->>Router: HTTP request (/api/notifications)
Router->>AuthMiddleware: Authenticate user
AuthMiddleware-->>Router: Pass if authenticated
Router->>NotificationController: Call method (e.g., getNotifications)
NotificationController->>NotificationService: Query notifications (with filters)
NotificationService->>DB: Fetch notifications
DB-->>NotificationService: Return results
NotificationService-->>NotificationController: Return notifications
NotificationController-->>Router: Send JSON response
Router-->>Client: Return notifications data
sequenceDiagram
participant Controller
participant NotificationEventMiddleware
participant NotificationService
participant DB
Controller->>NotificationEventMiddleware: Attach notification event to response
NotificationEventMiddleware->>NotificationService: Process event (e.g., create notification)
NotificationService->>DB: Persist notification
DB-->>NotificationService: Confirmation
NotificationService-->>NotificationEventMiddleware: Done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 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: 4
🧹 Nitpick comments (6)
src/tests/services/notificationService.test.ts (2)
15-22: Replaceanytype with proper TypeScript typing.The mock repository should use proper typing instead of
anyto maintain type safety in tests.beforeEach(() => { // Before each test, create a mock repository with jest functions for each method mockRepository = { save: jest.fn(), findAndCount: jest.fn(), findOne: jest.fn(), update: jest.fn(), count: jest.fn(), delete: jest.fn(), - } as any; + } as jest.Mocked<Repository<InAppNotificationEntity>>;
251-251: Replaceanytype casts with proper TypeScript typing.The mock return values should use proper typing instead of
anyfor better type safety.- mockRepository.update.mockResolvedValue({ affected: 3 } as any); + mockRepository.update.mockResolvedValue({ affected: 3, generatedMaps: [], raw: [] }); - mockRepository.update.mockResolvedValue({ affected: 1 } as any); + mockRepository.update.mockResolvedValue({ affected: 1, generatedMaps: [], raw: [] }); - mockRepository.delete.mockResolvedValue({ affected: 2 } as any); + mockRepository.delete.mockResolvedValue({ affected: 2, raw: [] });Also applies to: 282-282, 296-296
src/controllers/NotificationController.ts (1)
15-17: Consider extracting repeated authentication logic.The authentication check pattern is repeated in every method. Consider extracting this into a private method or relying on middleware for cleaner code.
+ private validateAuthentication(req: Request): string { + const userId = req.user?.id.toString(); + if (!userId) { + throw new AppError("User not authenticated", 401); + } + return userId; + } async getNotifications(req: Request, res: Response, next: NextFunction): Promise<void> { try { - const userId = req.user?.id.toString(); - if (!userId) { - throw new AppError("User not authenticated", 401); - } + const userId = this.validateAuthentication(req);Also applies to: 49-53, 69-72, 87-90, 108-110
src/middlewares/notificationEvent.middleware.ts (1)
88-134: Consider improving the notification trigger patternThe static helper methods work but have some limitations:
- They assume controllers will use
res.json(res.locals.responseBody)- Only one notification event can be triggered per response
- No parameter validation
Consider this alternative approach that supports multiple notifications:
+ static attachNotificationEvent( + res: Response, + event: NotificationEvent + ): void { + if (!res.locals.notificationEvents) { + res.locals.notificationEvents = []; + } + res.locals.notificationEvents.push(event); + } + static triggerPaymentNotification( res: Response, recipientId: string, amount: number, paymentId: string ): void { - const currentBody = res.locals.responseBody || {}; - currentBody.notificationEvent = { + this.attachNotificationEvent(res, { type: "payment_completed" as const, recipientId, amount, paymentId, - }; - res.locals.responseBody = currentBody; + }); }Then update the middleware to process multiple events:
res.json = (body: unknown) => { // Process any queued notification events if (res.locals.notificationEvents && Array.isArray(res.locals.notificationEvents)) { for (const event of res.locals.notificationEvents) { this.processNotificationEvent(event) .catch(error => logger.error('Failed to process notification event', { error })); } } // Also check body for backward compatibility if (body && typeof body === 'object' && 'notificationEvent' in body) { this.processNotificationEvent((body as { notificationEvent: NotificationEvent }).notificationEvent) .catch(error => logger.error('Failed to process notification event', { error })); } return originalJson(body); };src/services/inAppNotificationService.ts (2)
158-163: Consider using soft delete for expired notificationsThe method uses hard delete which permanently removes expired notifications. For audit trail and compliance purposes, consider using soft delete instead.
async deleteExpiredNotifications(): Promise<void> { const now = new Date(); - await this.notificationRepository.delete({ - expiresAt: LessThan(now), - }); + await this.notificationRepository.update( + { expiresAt: LessThan(now), status: Not(NotificationStatus.ARCHIVED) }, + { status: NotificationStatus.ARCHIVED } + ); }
165-212: Well-designed template methods for common notificationsThe template methods provide consistent notification creation with appropriate priorities and metadata.
Consider using locale-specific currency formatting for the payment notification:
- message: `You received a payment of $${amount}`, + message: `You received a payment of ${new Intl.NumberFormat('en-US', { style: 'currency', currency: 'USD' }).format(amount)}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app.ts(2 hunks)src/controllers/NotificationController.ts(1 hunks)src/dtos/NotificationDto.ts(1 hunks)src/middlewares/notificationEvent.middleware.ts(1 hunks)src/routes/notification.routes.ts(1 hunks)src/services/inAppNotificationService.ts(4 hunks)src/tests/services/notificationService.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/middlewares/notificationEvent.middleware.ts (1)
src/services/inAppNotificationService.ts (1)
NotificationService(36-213)
src/controllers/NotificationController.ts (1)
src/services/inAppNotificationService.ts (2)
NotificationService(36-213)NotificationFilters(26-34)
src/routes/notification.routes.ts (2)
src/controllers/NotificationController.ts (1)
NotificationController(6-122)src/middlewares/validationErrorHandler.ts (1)
handleValidationErrors(5-42)
🪛 GitHub Check: build-and-test
src/tests/services/notificationService.test.ts
[failure] 296-296:
Unexpected any. Specify a different type
[failure] 282-282:
Unexpected any. Specify a different type
[failure] 251-251:
Unexpected any. Specify a different type
[failure] 22-22:
Unexpected any. Specify a different type
src/dtos/NotificationDto.ts
[failure] 26-26:
Unexpected any. Specify a different type
src/services/inAppNotificationService.ts
[failure] 149-149:
Unexpected any. Specify a different type
[failure] 113-113:
Unexpected any. Specify a different type
[failure] 82-82:
Unexpected any. Specify a different type
src/middlewares/notificationEvent.middleware.ts
[failure] 41-41:
Unexpected any. Specify a different type
🪛 GitHub Actions: CI Pipeline
src/dtos/NotificationDto.ts
[error] 26-26: ESLint: Unexpected any. Specify a different type (@typescript-eslint/no-explicit-any)
🔇 Additional comments (13)
src/app.ts (1)
28-28: LGTM! Clean integration of notification routes.The notification routes are properly imported and registered under the appropriate
/api/notificationspath prefix, following the existing routing patterns in the application.Also applies to: 153-153
src/routes/notification.routes.ts (3)
10-11: LGTM! Proper security implementation.Global authentication middleware is correctly applied to all notification routes, ensuring that only authenticated users can access notification endpoints.
85-97: LGTM! Comprehensive validation and documentation.The route implementation follows best practices with:
- Proper query parameter validation using express-validator
- Comprehensive Swagger documentation
- Appropriate constraints (e.g., limit max 100, UUID validation)
- Error handling middleware integration
159-164: LGTM! Proper controller method binding.The use of
.bind(notificationController)ensures the correctthiscontext is maintained when the controller methods are called, preventing potential runtime errors.src/tests/services/notificationService.test.ts (2)
34-88: LGTM! Excellent test coverage for notification creation.The test suite properly covers both successful creation and error handling scenarios, with appropriate assertions to verify the repository methods are called with correct parameters.
307-371: LGTM! Comprehensive template method testing.The template method tests properly validate the structured creation of different notification types (payment, fraud alert, system update) with correct metadata, priorities, and categorization.
src/controllers/NotificationController.ts (3)
6-11: LGTM! Proper dependency injection pattern.The controller correctly initializes the NotificationService in the constructor, following good dependency injection practices.
13-44: LGTM! Comprehensive query parameter handling.The
getNotificationsmethod properly:
- Validates user authentication
- Parses and type-converts query parameters
- Handles optional date filtering
- Provides sensible defaults for pagination
- Uses consistent response format
46-65: LGTM! Consistent error handling and response format.All controller methods follow consistent patterns with:
- Proper try-catch error handling
- Standardized JSON response format
- Appropriate HTTP status codes
- Error delegation to middleware via
next()Also applies to: 67-83, 85-101, 103-121
src/middlewares/notificationEvent.middleware.ts (2)
1-26: Well-structured event interfaces with proper discriminated unionsThe event interfaces follow TypeScript best practices with clear property definitions and discriminated union pattern for type safety.
54-86: Robust event processing with proper error handlingThe event processing method correctly handles all event types with appropriate service calls and includes comprehensive error handling.
src/services/inAppNotificationService.ts (2)
1-1: Good addition of TypeORM query operators and filtering interfaceThe new imports and
NotificationFiltersinterface provide comprehensive filtering capabilities needed for the notification system.Also applies to: 26-34
61-63: Correct implementation of notification expirationThe optional
expiresAtproperty is properly handled with appropriate conditional assignment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/services/inAppNotificationService.ts (2)
180-184: Hard-deleting records may break audit expectations
deleteExpiredNotificationsissues a physicaldelete, removing rows permanently.
If the domain requires auditability or GDPR “erasure vs. archive” distinction, consider switching to a soft-delete (updatestatus = ARCHIVEDor use TypeORM’s soft-delete API) and schedule a real purge only when fully safe.
148-156: Return count from bulkmarkAllAsReadThe method currently returns
void, so callers can’t confirm how many rows were affected.
ReturningUpdateResult(or at leastaffectedcount) provides feedback for logging and troubleshooting at negligible cost.- async markAllAsRead(recipientId: string): Promise<void> { - await this.notificationRepository.update( + async markAllAsRead(recipientId: string): Promise<number> { + const res = await this.notificationRepository.update( { recipientId, isRead: false }, { isRead: true, status: NotificationStatus.READ, readAt: new Date() }, ); + return res.affected ?? 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/controllers/NotificationController.ts(1 hunks)src/dtos/NotificationDto.ts(1 hunks)src/middlewares/notificationEvent.middleware.ts(1 hunks)src/routes/notification.routes.ts(1 hunks)src/services/inAppNotificationService.ts(5 hunks)src/tests/services/notificationService.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/dtos/NotificationDto.ts
- src/controllers/NotificationController.ts
- src/tests/services/notificationService.test.ts
- src/middlewares/notificationEvent.middleware.ts
- src/routes/notification.routes.ts
MPSxDev
left a comment
MPSxDev
left a comment
There was a problem hiding this comment.
Thanks for contributing to Paystell, excellent work.
2 participants
Pull Request Overview
📝 Summary
This pull request implements a complete, real-time in-app push notification system.
It introduces the necessary services, controllers, and routes to create, manage, and deliver notifications for critical events like payments, fraud alerts, and system updates.
This work builds upon the existing
InAppNotificationentity to deliver a full-featured and highly requested user-facing feature.🔗 Related Issues
Closes #115
🔄 Changes Made
This PR introduces a comprehensive set of components to support the in-app notification system:
inAppNotificationService.ts:A robust service class to handle all business logic, including:
NotificationController.ts:Manages the request/response lifecycle for all notification-related API endpoints.
notification.routes.ts:Defines the REST API endpoints, including:
GET /GET /unread-countPUT /:id/readPUT /mark-all-readDELETE /:idSwagger documentation is included.
notificationEvent.middleware.ts:Middleware to automatically trigger notifications for system events like completed payments and detected fraud alerts.
NotificationDTO.ts:Provides DTOs for request validation.
Integration:
The new notification routes are registered in
app.tsunder the/api/notificationsbase path.🖼️ Current Output
The primary output is a series of fully functional API endpoints.
For example, a
GETrequest to/api/notificationswith query parameters for filtering and pagination will now return a structured JSON response containing:🧪 Testing
💬 Comments
This implementation provides a solid foundation for the notification system.
The template-based methods in the service make it straightforward to add new automatic notification types in the future as the application grows.
Summary by CodeRabbit
New Features
Tests
Documentation