feat: get active or archived notification by id #398
Conversation
WalkthroughThis pull request expands the notification functionality by adding a new API call to fetch a notification based on its ID using GraphQL. It updates the archived notifications module to export additional components, introduces a new service method for retrieving archived notifications, and implements a GraphQL query in the notifications resolver. A new DTO is added to standardize the response, and updates in the notifications module include new imports and comments to guide future entity modifications. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as NotificationsResolver
participant NS as NotificationsService
participant AS as ArchivedNotificationsService
C->>R: Query notification(notificationId)
R->>NS: findActiveOrArchivedNotificationById(notificationId)
NS->>NS: getNotificationById(notificationId)
alt Active notification found
NS-->>R: Return SingleNotificationResponse (active)
else No active notification
NS->>AS: getArchivedNotificationFromNotificationId(notificationId)
alt Archived notification found
AS-->>NS: Return archived notification
NS-->>R: Return SingleNotificationResponse (archived)
else Notification not found
NS-->>R: Throw NotFoundException
end
end
R-->>C: Return notification response
Suggested reviewers
Poem
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
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 (4)
apps/api/src/modules/notifications/notifications.module.ts (1)
105-105: Consider adding the service to exports array if needed elsewhereIf other modules need to use ArchivedNotificationsService through the NotificationsModule, consider adding it to the exports array as well.
apps/api/src/modules/notifications/dtos/single-notification.response.dto.ts (2)
79-96: Constructor logic could be more robust with type guards.The current implementation relies on type casting and optional chaining for handling different entity types. Consider using type guards for more explicit type checking.
constructor(notificationEntry: Notification | ArchivedNotification) { - this.id = - (notificationEntry as Notification).id || - (notificationEntry as ArchivedNotification).notificationId; + // Using type guard for more robust type checking + this.id = 'id' in notificationEntry + ? notificationEntry.id + : notificationEntry.notificationId; this.providerId = notificationEntry.providerId; this.channelType = notificationEntry.channelType; this.data = notificationEntry.data; this.deliveryStatus = notificationEntry.deliveryStatus; this.result = notificationEntry.result; this.createdOn = notificationEntry.createdOn; this.updatedOn = notificationEntry.updatedOn; this.createdBy = notificationEntry.createdBy; this.updatedBy = notificationEntry.updatedBy; this.status = notificationEntry.status; this.applicationId = notificationEntry.applicationId; this.retryCount = notificationEntry.retryCount; }
1-97: Consider adding JSDoc comments for better documentation.Adding JSDoc comments to the class and constructor would improve code documentation, making it easier for other developers to understand the purpose and usage of this DTO.
+/** + * Response DTO for returning notification data from either active or archived notifications. + * This class standardizes the response format for notification queries. + */ @ObjectType() export class SingleNotificationResponse { // ... existing properties ... + /** + * Initializes a SingleNotificationResponse instance from either a Notification or ArchivedNotification entity. + * @param notificationEntry - The notification entity (active or archived) + */ constructor(notificationEntry: Notification | ArchivedNotification) { // ... existing implementation ... } }apps/api/src/modules/notifications/notifications.service.ts (1)
260-262: Add a ticket reference to the TODO comment.This TODO comment suggests updating the
getNotificationByIdfunction but doesn't provide a timeline or ticket reference, which could lead to it being forgotten.- // TODO: Update function to return single notification instead of array and make it asynchronous - // It is possible this was done so that we do not get null value as response so need to check first + // TODO: [TICKET-XXX] Update function to return single notification instead of array and make it asynchronous + // It is possible this was done so that we do not get null value as response so need to check first
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/api/OsmoX-API.postman_collection.json(1 hunks)apps/api/src/modules/archived-notifications/archived-notifications.module.ts(1 hunks)apps/api/src/modules/archived-notifications/archived-notifications.service.ts(1 hunks)apps/api/src/modules/notifications/dtos/single-notification.response.dto.ts(1 hunks)apps/api/src/modules/notifications/entities/notification.entity.ts(1 hunks)apps/api/src/modules/notifications/notifications.module.ts(3 hunks)apps/api/src/modules/notifications/notifications.resolver.ts(2 hunks)apps/api/src/modules/notifications/notifications.service.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
apps/api/src/modules/notifications/notifications.service.ts (1)
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`.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (9)
apps/api/src/modules/notifications/entities/notification.entity.ts (1)
100-101: Good documentation of planned technical debtThe TODO comment clearly documents the planned modifications for the relations between entities, which will help future developers understand the technical debt and necessary changes.
apps/api/src/modules/notifications/notifications.resolver.ts (2)
8-8: LGTM: Appropriate import for the new functionalityThe import for SingleNotificationResponse is correctly added to support the new notification query.
24-27: Well-implemented query method to fetch notifications by IDThe new GraphQL query method follows NestJS best practices:
- Properly decorated with @query
- Accepts a notificationId parameter
- Returns the appropriate response type
- Delegates to the service layer for implementation
This implementation fulfills the PR objective of creating a GraphQL endpoint for retrieving notifications by ID.
apps/api/OsmoX-API.postman_collection.json (1)
699-744: Good API test case for the new endpointThe Postman collection entry is well-structured with:
- Clear naming and description
- Proper authorization headers
- A sample GraphQL query with all necessary fields
- Example notification ID for testing
This will help with testing and serve as documentation for API consumers.
apps/api/src/modules/notifications/notifications.module.ts (3)
47-48: LGTM: Required imports for archived notifications functionalityThe imports for ArchivedNotificationsModule and ArchivedNotificationsService are correctly added to support the new feature.
89-89: Proper module integrationArchivedNotificationsModule is correctly included in the imports array, following NestJS module patterns.
105-105: Proper service registrationArchivedNotificationsService is correctly added to the providers array, making it available for dependency injection in the notifications module.
apps/api/src/modules/archived-notifications/archived-notifications.service.ts (1)
137-148: Method implementation looks good.The new
getArchivedNotificationFromNotificationIdmethod is well-implemented, following the service's existing patterns for repository queries and logging.apps/api/src/modules/archived-notifications/archived-notifications.module.ts (1)
16-16: Good addition to module exports.Exporting the TypeOrmModule alongside the service is beneficial as it allows other modules to access repository functionality related to ArchivedNotification entities.
| async findActiveOrArchivedNotificationById( | ||
| notificatonId: number, | ||
| ): Promise<SingleNotificationResponse> { | ||
| try { | ||
| const activeEntry = (await this.getNotificationById(notificatonId))[0]; | ||
|
|
||
| if (activeEntry) { | ||
| return new SingleNotificationResponse(activeEntry); | ||
| } | ||
|
|
||
| const archivedEntry = | ||
| await this.archivedNotificationsService.getArchivedNotificationFromNotificationId( | ||
| notificatonId, | ||
| ); | ||
|
|
||
| if (archivedEntry) { | ||
| return new SingleNotificationResponse(archivedEntry); | ||
| } | ||
|
|
||
| throw new NotFoundException(`Notification with ID ${notificatonId} not found in any table`); | ||
| } catch (error) { | ||
| return error; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix typo and improve error handling in the new method.
The method has a typo in the parameter name (notificatonId missing an 'i') and the error handling returns the error object instead of throwing it properly.
async findActiveOrArchivedNotificationById(
- notificatonId: number,
+ notificationId: number,
): Promise<SingleNotificationResponse> {
try {
- const activeEntry = (await this.getNotificationById(notificatonId))[0];
+ const activeEntry = (await this.getNotificationById(notificationId))[0];
if (activeEntry) {
return new SingleNotificationResponse(activeEntry);
}
const archivedEntry =
await this.archivedNotificationsService.getArchivedNotificationFromNotificationId(
- notificatonId,
+ notificationId,
);
if (archivedEntry) {
return new SingleNotificationResponse(archivedEntry);
}
- throw new NotFoundException(`Notification with ID ${notificatonId} not found in any table`);
+ throw new NotFoundException(`Notification with ID ${notificationId} not found in any table`);
} catch (error) {
- return error;
+ // Log the error for debugging purposes
+ this.logger.error(`Error finding notification: ${error.message}`, error.stack);
+ // Re-throw the error to be handled by the calling method
+ throw error;
}
}📝 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.
| async findActiveOrArchivedNotificationById( | |
| notificatonId: number, | |
| ): Promise<SingleNotificationResponse> { | |
| try { | |
| const activeEntry = (await this.getNotificationById(notificatonId))[0]; | |
| if (activeEntry) { | |
| return new SingleNotificationResponse(activeEntry); | |
| } | |
| const archivedEntry = | |
| await this.archivedNotificationsService.getArchivedNotificationFromNotificationId( | |
| notificatonId, | |
| ); | |
| if (archivedEntry) { | |
| return new SingleNotificationResponse(archivedEntry); | |
| } | |
| throw new NotFoundException(`Notification with ID ${notificatonId} not found in any table`); | |
| } catch (error) { | |
| return error; | |
| } | |
| } | |
| async findActiveOrArchivedNotificationById( | |
| notificationId: number, | |
| ): Promise<SingleNotificationResponse> { | |
| try { | |
| const activeEntry = (await this.getNotificationById(notificationId))[0]; | |
| if (activeEntry) { | |
| return new SingleNotificationResponse(activeEntry); | |
| } | |
| const archivedEntry = | |
| await this.archivedNotificationsService.getArchivedNotificationFromNotificationId( | |
| notificationId, | |
| ); | |
| if (archivedEntry) { | |
| return new SingleNotificationResponse(archivedEntry); | |
| } | |
| throw new NotFoundException(`Notification with ID ${notificationId} not found in any table`); | |
| } catch (error) { | |
| // Log the error for debugging purposes | |
| this.logger.error(`Error finding notification: ${error.message}`, error.stack); | |
| // Re-throw the error to be handled by the calling method | |
| throw error; | |
| } | |
| } |
There was a problem hiding this comment.
fixed ee2a2cd
There was a problem hiding this comment.
@kshitij-k-osmosys Thank you for fixing the typo and improving the error handling in the method! This will make the code more maintainable and help with debugging future issues.
(\ /)
(•ᴗ•)
/ >🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/api/docs/api-documentation.md (1)
333-407: New Section for "Fetch active or archived Notification by Id"The new section is well-structured and clearly documents the GraphQL endpoint for fetching a single notification by ID. It includes all the necessary details such as the required parameter (
notificationId), endpoint URL, HTTP method, GraphQL query, a sample cURL command, and a complete JSON sample response. This mirrors the style of the other documented endpoints and should help consumers understand how to use this feature.Suggestions:
Markdown Headings Consistency: The sample response header is currently styled using emphasis (e.g.,
**Sample response**at line 377). To adhere to markdown linting standards and maintain consistency, consider using a proper heading (e.g.,#### Sample response). Here's a suggested diff:-**Sample response** +#### Sample responseParameter Clarity: The documentation lists
notificationIdas the required parameter. Ensure that any additional details regarding its format (e.g., whether it should be numeric or a UUID) are captured elsewhere in your API docs if applicable.Overall, this documentation addition is clear and aligns well with the existing API documentation.
🧰 Tools
🪛 Gitleaks (8.21.2)
371-372:
(curl-auth-header)
🪛 markdownlint-cli2 (0.17.2)
368-368: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
377-377: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/docs/api-documentation.md(1 hunks)apps/api/src/modules/notifications/notifications.service.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/modules/notifications/notifications.service.ts
🧰 Additional context used
🪛 Gitleaks (8.21.2)
apps/api/docs/api-documentation.md
371-372:
(curl-auth-header)
🪛 markdownlint-cli2 (0.17.2)
apps/api/docs/api-documentation.md
368-368: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
377-377: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/api/OsmoX.postman_collection.json (2)
3195-3256: New Endpoint – Fetch Active or Archived Notification by Id (Success Case)This new section introduces a GraphQL query for retrieving a notification by its ID. The success request is well structured:
- The query requests key fields (such as applicationId, channelType, createdOn, and others) from the notification object.
- The accompanying test scripts validate that the response is in valid JSON format, that it contains a top-level
dataproperty, and that anotificationobject exists underdata.This implementation aligns with the PR objectives and clearly documents the success scenario.
Suggestion: Consider adding an assertion for the actual content (or at least a few critical fields) of the notification object to ensure that the API returns the expected values.
3257-3316: New Endpoint – Fetch Active or Archived Notification by Id (Not Found Exception)This item handles the error scenario when an invalid or non-existent notification ID is requested. The test scripts:
- Verify that the response is valid JSON.
- Check for the presence of both a
dataproperty and anerrorsarray.- Assert that the error message is of string type (ensuring that an error is indeed provided).
This properly covers the “not found” case and helps ensure robust error handling for the new endpoint.
Suggestion: It might be beneficial to assert a specific error message (or pattern) so that the test not only confirms an error exists but also that it is descriptive enough (for example, “Notification not found” or similar) to help with debugging.
API PR Checklist
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:
Create GraphQL endpoint to get active or archived notification by notificationId
Related changes:
Screenshots:
Query request and response:
Request
Documentation changes:
Summary by CodeRabbit
New Features
Refactor