feat(user-notification): Add getNotificationStatus endpoint #17187
Conversation
Adds a new endpoint to fetch notification status with dummy data
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17187 +/- ##
=======================================
Coverage 35.73% 35.73%
=======================================
Files 6928 6928
Lines 147824 147829 +5
Branches 42129 42130 +1
=======================================
+ Hits 52818 52820 +2
- Misses 95006 95009 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
error Received malformed response from registry for "@typescript-eslint%2feslint-plugin". The registry may be down. WalkthroughThe changes involve the addition of a new method Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
apps/services/user-notification/src/app/modules/notifications/notifications.controller.ts (1)
111-127: Enhance API documentationThe current documentation could be improved by adding more details about the response structure and possible error scenarios.
Update the @documentation decorator to include:
- Response schema with possible status values
- Error responses (404 for not found, 403 for unauthorized)
- Example response in the documentation
@Documentation({ summary: 'Get status information for a specific notification', response: { status: HttpStatus.OK, schema: { type: NotificationStatus, example: { id: 'notif-123-456', status: 'DELIVERED', createdAt: '2023-12-10T10:00:00Z', lastUpdated: '2023-12-10T10:15:00Z' } } }, responses: [ { status: 404, description: 'Notification not found' }, { status: 403, description: 'Unauthorized access' } ] })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/services/user-notification/src/app/modules/notifications/notifications.controller.ts(1 hunks)infra/src/uber-charts/islandis.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/user-notification/src/app/modules/notifications/notifications.controller.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
| ): Promise<{ | ||
| id: string | ||
| status: string | ||
| createdAt: string | ||
| lastUpdated: string | ||
| }> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define an interface for the notification status response
The return type should be defined as an interface with proper types for each field.
Add this interface definition:
export interface NotificationStatus {
id: string;
status: NotificationStatusEnum; // Define enum with possible values
createdAt: string;
lastUpdated: string;
}| return { | ||
| id: notificationId, | ||
| status: 'DELIVERED', | ||
| createdAt: '2024-12-10T10:00:00Z', | ||
| lastUpdated: '2024-12-10T10:15:00Z', | ||
| } |
There was a problem hiding this comment.
Replace hardcoded response with actual implementation
The current implementation returns hardcoded dummy data. This needs to be replaced with actual notification status retrieval logic.
Consider implementing the following:
- Add a method in NotificationsService to fetch notification status
- Handle cases where notification doesn't exist
- Use proper status enum/type instead of hardcoded string
- return {
- id: notificationId,
- status: 'DELIVERED',
- createdAt: '2024-12-10T10:00:00Z',
- lastUpdated: '2024-12-10T10:15:00Z',
- }
+ const notification = await this.notificationsService.getNotificationStatus(notificationId);
+ if (!notification) {
+ throw new NotFoundException(`Notification with id ${notificationId} not found`);
+ }
+ return notification;Committable suggestion skipped: line range outside the PR's diff.
| // FIXME BEFORE APPROVEING THIS PR !!!!!!!!!!!!!!!! MERGE AND DIE | ||
| //userNotificationService, | ||
| //userNotificationWorkerService, | ||
| //userNotificationCleanupWorkerService, |
There was a problem hiding this comment.
💡 Codebase verification
Remove warning comment and uncomment notification services
The notification services are actively used in multiple parts of the system (api and application-system-api) and should not be excluded from feature deployments. The warning comment suggests these changes were temporary and not meant for production.
apps/api/infra/api.tsdepends on userNotificationService for HTTP endpointsapps/application-system/api/infra/application-system-api.tshas multiple dependencies on these services- These services are part of the core functionality and should be available in feature deployments
export const ExcludedFeatureDeploymentServices: ServiceBuilder<any>[] = [
- // FIXME BEFORE APPROVEING THIS PR !!!!!!!!!!!!!!!! MERGE AND DIE
- //userNotificationService,
- //userNotificationWorkerService,
- //userNotificationCleanupWorkerService,
contentfulEntryTagger,
searchIndexer,
contentfulApps,
githubActionsCache,
]🔗 Analysis chain
Remove warning comment and properly document service exclusions
The current changes include a concerning warning comment and commented-out services without proper explanation.
- Remove the warning comment
- Either remove the services from the exclusion list or document why they should be excluded
- Verify the impact on feature deployments
export const ExcludedFeatureDeploymentServices: ServiceBuilder<any>[] = [
- // FIXME BEFORE APPROVEING THIS PR !!!!!!!!!!!!!!!! MERGE AND DIE
- //userNotificationService,
- //userNotificationWorkerService,
- //userNotificationCleanupWorkerService,
contentfulEntryTagger,
searchIndexer,
contentfulApps,
githubActionsCache,
]Let's verify the dependencies of these services:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for dependencies on notification services
rg -l "userNotification(Service|Worker|CleanupWorker)" apps/
Length of output: 203
Script:
#!/bin/bash
# Check the actual usage of notification services in the dependent files
rg "userNotification(Service|Worker|CleanupWorker)" apps/services/user-notification/infra/user-notification.ts apps/application-system/api/infra/application-system-api.ts apps/api/infra/api.ts -A 3
# Check if these services are used in feature deployments configuration
rg "feature.*deployment" -A 5 -B 5
Length of output: 14815
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
apps/services/user-notification/src/app/modules/notifications/notifications.controller.ts (2)
115-127: Enhance API documentation for better developer experienceThe current documentation could be improved by adding:
- Possible response status codes (e.g., 404 for not found)
- Example response with possible status values
- Error response schema
Update the Documentation decorator to include:
@Documentation({ summary: 'Get status information for a specific notification', response: { status: HttpStatus.OK, schema: { example: { id: 'notif-123-456', status: 'DELIVERED', // One of: PENDING, DELIVERED, FAILED createdAt: '2024-12-10T10:00:00.000Z', lastUpdated: '2024-12-10T10:15:00.000Z' } } }, responses: [ { status: HttpStatus.NOT_FOUND, description: 'Notification not found' } ], // ... rest of the existing config })
128-144: Add proper error handling following NestJS patternsThe endpoint should handle cases where the notification ID is invalid or not found, following NestJS exception patterns.
Consider adding:
import { NotFoundException } from '@nestjs/common'; async getNotificationStatus( @Param('notificationId') notificationId: string, ): Promise<NotificationStatusResponse> { this.logger.info(`Fetching status for notification: ${notificationId}`); const status = await this.notificationsService.getNotificationStatus(notificationId); if (!status) { throw new NotFoundException(`Notification with ID ${notificationId} not found`); } return status; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/services/user-notification/src/app/modules/notifications/notifications.controller.ts(1 hunks)infra/src/uber-charts/islandis.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- infra/src/uber-charts/islandis.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/user-notification/src/app/modules/notifications/notifications.controller.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
| ): Promise<{ | ||
| id: string | ||
| status: string | ||
| createdAt: string | ||
| lastUpdated: string | ||
| }> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety for notification status
The return type should use an enum for the status field to ensure type safety and documentation.
Consider adding:
export enum NotificationStatus {
PENDING = 'PENDING',
DELIVERED = 'DELIVERED',
FAILED = 'FAILED'
}
// Update the return type
interface NotificationStatusResponse {
id: string;
status: NotificationStatus;
createdAt: string;
lastUpdated: string;
}| return { | ||
| id: notificationId, | ||
| status: 'DELIVERED', | ||
| createdAt: '2024-12-10T10:00:00Z', | ||
| lastUpdated: '2024-12-10T10:15:00Z', | ||
| } |
There was a problem hiding this comment.
Implement proper service layer integration instead of hardcoded response
The current implementation returns hardcoded data which is not suitable for production use. This should be integrated with the NotificationsService.
Consider implementing like this:
- return {
- id: notificationId,
- status: 'DELIVERED',
- createdAt: '2024-12-10T10:00:00Z',
- lastUpdated: '2024-12-10T10:15:00Z',
- }
+ return this.notificationsService.getNotificationStatus(notificationId);Committable suggestion skipped: line range outside the PR's diff.
| @UseGuards(IdsUserGuard, ScopesGuard) | ||
| @Scopes(AdminPortalScope.serviceDesk) | ||
| @Get('/status/:notificationId') | ||
| @Version('1') | ||
| @Documentation({ | ||
| summary: 'Get status information for a specific notification', | ||
| response: { status: HttpStatus.OK }, | ||
| request: { | ||
| params: { | ||
| notificationId: { | ||
| type: 'string', | ||
| description: 'ID of the notification', | ||
| example: 'notif-123-456', | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| async getNotificationStatus( | ||
| @Param('notificationId') notificationId: string, | ||
| ): Promise<{ | ||
| id: string | ||
| status: string | ||
| createdAt: string | ||
| lastUpdated: string | ||
| }> { | ||
| this.logger.info(`Fetching status for notification: ${notificationId}`) | ||
|
|
||
| return { | ||
| id: notificationId, | ||
| status: 'DELIVERED', | ||
| createdAt: '2024-12-10T10:00:00Z', | ||
| lastUpdated: '2024-12-10T10:15:00Z', | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Hardcoded status value needs to be aligned with the notification model
The current implementation returns a hardcoded status 'DELIVERED' which doesn't align with the existing notification system. The codebase shows that notifications are tracked using boolean flags 'read' and 'seen' in the NotificationsService and Notification model, rather than a delivery status.
- The status endpoint should be modified to use the actual notification model's properties (read/seen) instead of a hardcoded delivery status
- Consider using the NotificationsService to fetch the actual notification record and its status
- Align the response type with existing DTOs like
RenderedNotificationDtowhich already includes read/seen status
🔗 Analysis chain
Verify consistency with other notification endpoints
Let's ensure the implementation is consistent with other notification-related endpoints in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar notification status implementations
echo "Searching for notification status patterns..."
ast-grep --pattern 'status: string' apps/services/user-notification/
# Search for notification-related controllers
echo "Searching for other notification controllers..."
rg -l "NotificationsController" apps/services/
# Search for notification status enums
echo "Searching for existing notification status enums..."
rg -l "enum.*Status" apps/services/user-notification/
Length of output: 1198
Script:
#!/bin/bash
# Let's check the content of me-notifications.controller.ts for similar endpoints
echo "Checking me-notifications.controller.ts..."
cat apps/services/user-notification/src/app/modules/notifications/me-notifications.controller.ts
# Let's also check if there are any notification status types or constants
echo "Searching for notification status types..."
rg -A 5 "type.*Status|enum.*Status|const.*Status" apps/services/user-notification/
# Let's check the service implementation for status handling
echo "Checking notification service implementation..."
cat apps/services/user-notification/src/app/modules/notifications/notifications.service.ts
Length of output: 16354
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
apps/services/user-notification/src/app/modules/notifications/notifications.controller.ts (2)
111-114: Consider using a more RESTful endpoint pathThe security setup with IdsUserGuard and AdminPortalScope is appropriate. However, consider making the endpoint more RESTful by moving it to:
GET /:notificationId/statusinstead ofGET /status/:notificationId
115-127: Enhance API documentation for better clarityWhile the basic documentation is in place, consider adding:
- Possible error responses (e.g., 404 for not found)
- Description of response fields (status, createdAt, lastUpdated)
- Enumeration of possible status values
Example enhancement:
@Documentation({ summary: 'Get status information for a specific notification', response: { status: HttpStatus.OK, description: 'Returns the current status and timestamps of the notification', }, responses: [ { status: 404, description: 'Notification not found' }, { status: 403, description: 'Insufficient permissions' } ], schema: { properties: { status: { enum: ['PENDING', 'DELIVERED', 'FAILED'], description: 'Current status of the notification' } } } })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/services/user-notification/src/app/modules/notifications/notifications.controller.ts(1 hunks)infra/src/uber-charts/islandis.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- infra/src/uber-charts/islandis.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/user-notification/src/app/modules/notifications/notifications.controller.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
| async getNotificationStatus( | ||
| @Param('notificationId') notificationId: string, | ||
| ): Promise<{ | ||
| id: string | ||
| status: string | ||
| createdAt: string | ||
| lastUpdated: string | ||
| }> { | ||
| this.logger.info(`Fetching status for notification: ${notificationId}`) | ||
|
|
||
| return { | ||
| id: notificationId, | ||
| status: 'DELIVERED', | ||
| createdAt: '2024-12-10T10:00:00Z', | ||
| lastUpdated: '2024-12-10T10:15:00Z', | ||
| } | ||
| } |
There was a problem hiding this comment.
Implementation needs significant improvements
The current implementation has several critical issues:
- Returns hardcoded values instead of actual notification status
- Missing error handling for non-existent notifications
- Response type should be moved to a DTO class
Suggested improvements:
// Create new file: notification-status.dto.ts
export class NotificationStatusDto {
@ApiProperty()
id: string;
@ApiProperty({
enum: ['PENDING', 'DELIVERED', 'FAILED'],
description: 'Current status of the notification'
})
status: string;
@ApiProperty()
createdAt: string;
@ApiProperty()
lastUpdated: string;
}
// In controller:
async getNotificationStatus(
@Param('notificationId') notificationId: string,
): Promise<NotificationStatusDto> {
try {
return await this.notificationsService.getStatus(notificationId);
} catch (error) {
if (error instanceof NotFoundException) {
throw new NotFoundException(`Notification ${notificationId} not found`);
}
throw error;
}
}| @UseGuards(IdsUserGuard, ScopesGuard) | ||
| @Scopes(AdminPortalScope.serviceDesk) | ||
| @Get('/status/:notificationId') | ||
| @Version('1') | ||
| @Documentation({ | ||
| summary: 'Get status information for a specific notification', | ||
| response: { status: HttpStatus.OK }, | ||
| request: { | ||
| params: { | ||
| notificationId: { | ||
| type: 'string', | ||
| description: 'ID of the notification', | ||
| example: 'notif-123-456', | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| async getNotificationStatus( | ||
| @Param('notificationId') notificationId: string, | ||
| ): Promise<{ | ||
| id: string | ||
| status: string | ||
| createdAt: string | ||
| lastUpdated: string | ||
| }> { | ||
| this.logger.info(`Fetching status for notification: ${notificationId}`) | ||
|
|
||
| return { | ||
| id: notificationId, | ||
| status: 'DELIVERED', | ||
| createdAt: '2024-12-10T10:00:00Z', | ||
| lastUpdated: '2024-12-10T10:15:00Z', | ||
| } | ||
| } |
There was a problem hiding this comment.
Add test coverage for the new endpoint
According to the coding guidelines, we need comprehensive test coverage. Please add:
- Unit tests for the controller method
- Integration tests for the endpoint
- Authorization test cases
Test scenarios should include:
- Successful status retrieval
- Not found scenarios
- Authorization failures
Example test structure:
describe('NotificationsController', () => {
describe('getNotificationStatus', () => {
it('should return status for existing notification', async () => {
// Test implementation
});
it('should throw NotFoundException for non-existent notification', async () => {
// Test implementation
});
it('should require proper scope', async () => {
// Test implementation
});
});
});
Adds a new endpoint to fetch notification status with dummy data
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Chores