feat(user-notification): Stale deviceToken cleanup #14840
Conversation
|
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThe recent updates in the user notification service involve significant refactoring to enhance push notification handling. This includes improved error management, better token handling, and streamlined notification dispatch logic. Additionally, there has been a shift from dynamic user profile fetching to using hardcoded mock profiles in the notifications worker service. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Datadog ReportAll test runs ✅ 6 Total Test Services: 0 Failed, 6 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14840 +/- ##
==========================================
- Coverage 37.04% 37.02% -0.02%
==========================================
Files 6331 6331
Lines 129131 129134 +3
Branches 36859 36852 -7
==========================================
- Hits 47833 47817 -16
- Misses 81298 81317 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
Affected services are: api,application-system-api,services-auth-ids-api,services-user-notification,services-user-profile, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts (3 hunks)
- apps/services/user-profile/src/app/v2/userToken.controller.ts (2 hunks)
- apps/services/user-profile/src/app/v2/userToken.service.ts (1 hunks)
Additional Context Used
Path-based Instructions (3)
apps/services/user-profile/src/app/v2/userToken.service.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."
apps/services/user-profile/src/app/v2/userToken.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."
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.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."
Additional comments not posted (3)
apps/services/user-profile/src/app/v2/userToken.service.ts (1)
21-31: The implementation ofdeleteUserTokenByNationalIdcorrectly handles the deletion of user tokens based on national ID and device token. Good use of Sequelize's ORM capabilities.apps/services/user-profile/src/app/v2/userToken.controller.ts (1)
55-71: ThedeleteUserDeviceTokenmethod is well-implemented with clear documentation and auditing. It correctly handles the deletion of a user device token based on the provided national ID and device token.apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts (1)
31-49: The new methods inNotificationDispatchServiceare well-implemented with robust error handling and integration with external services. The logging and exception management are appropriate for the operations being performed.Also applies to: 51-78, 80-115, 117-154, 156-171
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts (1)
4-6: Consider handling more specific exceptions where possible.Using generic exceptions like
BadRequestExceptionandInternalServerErrorExceptioncan be appropriate, but for a robust system, consider catching and handling more specific exceptions related to the operations being performed. This can aid in debugging and provide clearer error messages to the client.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts (3 hunks)
Additional Context Used
Path-based Instructions (1)
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.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."
Additional comments not posted (2)
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts (2)
31-48: Ensure proper handling when no tokens are found.The early return when no tokens are found is a good practice as it prevents unnecessary processing. However, consider logging this event or notifying an admin if this scenario is unexpected or could indicate an issue.
111-112: Confirm successful notification dispatch.Logging the success of the notification dispatch is good practice. It helps in monitoring and debugging the notification flow.
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts
Show resolved
Hide resolved
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts
Show resolved
Hide resolved
…b.com/island-is/island.is into user-notification/stale-token-cleanup
…er-notification/stale-token-cleanup
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts (3 hunks)
- apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.ts (1 hunks)
Additional Context Used
Path-based Instructions (2)
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.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."
apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.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."
Additional comments not posted (5)
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts (5)
31-31: Ensure proper handling when no device tokens are found.This early return when no tokens are found is a good practice as it prevents unnecessary processing.
46-47: Consider breaking the loop on specific errors.As previously noted, consider whether it might be appropriate to stop further processing if a certain type of error is encountered repeatedly.
76-78: Enhance error handling by providing more context.When catching exceptions in
getDeviceTokens, the rethrownInternalServerErrorExceptionshould include more context about the error to aid in debugging.
118-166: Refactor error handling to improve maintainability.The
handleSendErrormethod contains a large switch statement to handle various error codes. Consider refactoring this into separate methods or a strategy pattern to handle different types of errors, which could improve the maintainability and readability of the code.
169-184: Ensure robust error handling when removing invalid tokens.The method
removeInvalidTokenattempts to remove a token and logs the outcome, but the catch block only logs the error without rethrowing or handling it further. This might lead to silent failures where the system believes a token has been removed when it has not.
...otification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts (3 hunks)
- apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.ts (1 hunks)
Additional Context Used
Path-based Instructions (2)
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.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."
apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.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."
Additional comments not posted (6)
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts (5)
31-31: Ensure proper handling when no device tokens are found.This early return when no tokens are found is a good practice as it avoids unnecessary processing.
46-47: Consider breaking the loop on specific errors.As previously noted, consider whether it might be appropriate to stop further processing if a certain type of error is encountered repeatedly.
76-78: Enhance error handling by providing more context.As previously suggested, the rethrown
InternalServerErrorExceptionshould include more context about the error to aid in debugging.
118-166: Refactor error handling to improve maintainability.Consider refactoring this large switch statement into separate methods or a strategy pattern to handle different types of errors, which could improve the maintainability and readability of the code.
169-184: Ensure robust error handling when removing invalid tokens.As previously suggested, consider rethrowing or handling the error further in the catch block to avoid silent failures where the system believes a token has been removed when it has not.
apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.ts (1)
371-384: Ensure mock user profile is used only for testing and not in production.As previously noted, ensure that the use of a mock user profile is clearly documented to avoid confusion during maintenance. Consider using environment-based conditions to enable mock profiles only in non-production environments.
…er-notification/stale-token-cleanup
…b.com/island-is/island.is into user-notification/stale-token-cleanup
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts (3)
31-49: Ensure comprehensive logging by adding more context to error messages.Consider enhancing the error log in the catch block by including the
nationalIdto aid in debugging.
51-77: Enhance error handling by providing more context.Consider adding the
nationalIdto theInternalServerErrorExceptionmessage to provide more detailed debugging information.
Line range hint
80-115: Add error handling for Firebase messaging service calls.Consider wrapping the
firebase.messaging().send(message)call in a try-catch block to handle potential errors from the Firebase service.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts (3 hunks)
- apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.ts
Additional Context Used
Path-based Instructions (1)
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.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."
Additional comments not posted (2)
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts (2)
1-6: Imports are well-organized and adhere to NestJS best practices.
10-14: Constructor correctly implements dependency injection, adhering to NestJS best practices.
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts
Show resolved
Hide resolved
apps/services/user-notification/src/app/modules/notifications/notificationDispatch.service.ts
Show resolved
Hide resolved
|
@coderabbitai pause |
Actions PerformedReviews paused. |
rafnarnason
added
the
deprecated:automerge
This pull request introduces improvements to the user notification system by implementing a cleanup mechanism for stale device tokens. Key changes include:
Enhanced Error Handling: Added comprehensive error handling for various Firebase messaging errors, ensuring invalid or outdated device tokens are removed promptly.
New API Endpoint: Introduced a DELETE endpoint in the UserTokenController to facilitate the removal of invalid device tokens from the user profile service.
Service Method Addition: Added a method in the UserTokenService to delete device tokens by national ID, supporting the cleanup process.
These changes enhance the reliability of the notification system by ensuring that only valid device tokens are used, thus reducing potential errors and improving user experience.
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
Refactor
messageId.Bug Fixes