Feat/integrate with notification service provider #93
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Update core dependency in pubspec.yaml and pubspec.lock - Change ref from e7c808c9d459233196e2eac3137a9c87d3976af3 to abd044bc891c562a2758ce85f9b3893e982a554a
- Add Firebase project credentials (project ID, client email, private key) - Include OneSignal app ID and REST API key - These variables are required for implementing push notification functionality
- Implement a new database migration to add the 'isBreaking' field to existing Headline documents - Set default value to false for documents where the field is missing - Ensure schema consistency for the introduction of breaking news feature - Log the number of modified documents during the migration process - Note: Downward migration is not supported for this change
- Import new migration file for adding isBreaking field to headlines - Add migration to the allMigrations list
- Define a new abstract interface for push notification clients - Specify the contract for sending push notifications through different providers - Include method for sending notifications with device token, payload, and provider config
- Implement FirebasePushNotificationClient class - Add sendNotification method to handle FCM message sending - Include error handling and logging for various scenarios - Ensure compatibility with existing IPushNotificationClient interface
Add OneSignalPushNotificationClient, a concrete implementation of IPushNotificationClient for sending notifications via the OneSignal REST API. - Implement sendNotification method to construct and send HTTP requests - Include support for logging and error handling - Require HttpClient with AuthInterceptor for API requests - Support for optional image URL in notification payload
- Add IPushNotificationService abstract class - Implement DefaultPushNotificationService - Integrate with various data repositories for user subscriptions, device tokens, and remote config - Support multiple push notification providers (Firebase, OneSignal) - Handle breaking news notifications with configurable settings
- Add new permission constant 'push_notification.send_breaking_news' - This permission allows sending breaking news push notifications - Typically granted to roles like 'publisher' or 'admin' in the dashboard
- Add Permissions.pushNotificationSendBreakingNews to _dashboardPublisherPermissions set - Allow dashboard publishers to send breaking news notifications
- Add getters for Firebase Project ID, Client Email, and Private Key - Add getters for OneSignal App ID and REST API Key - All new methods will throw a StateError if the required environment variable is not set
- Add PushNotificationService and related repositories to the middleware - Include DataRepository for PushNotificationDevice and PushNotificationSubscription - Provide IPushNotificationService for dependency injection
…astructure - Add push notification device and subscription repositories - Implement Firebase and OneSignal push notification clients - Create push notification service interface and default implementation - Update app dependencies to include new push notification components - Initialize HTTP clients for push notification providers
- Implement push notification for breaking headlines - Integrate IPushNotificationService to send breaking news notifications - Add error handling for notification sending process
… field
- Replace exists('isBreaking').not() with notExists('isBreaking') for better readability
- Improve performance and code clarity in the updateMany query
…neSignal - Add dart_jsonwebtoken dependency for JWT handling - Implement Firebase token provider using service account credentials - Set up OneSignal token provider using REST API key - Update HTTP client initialization for both providers
Update the method to read the correct environment variable name `ONESIGNAL_REST_API_KEY` instead of the previously incorrect `ONESIGNAL_API_KEY`.
- Add success log after triggering breaking news notification - Improve code comments for better understanding of the notification process
- Implement subscription limit logic for premium, standard, and guest users - Add notification subscription limit calculation based on remote config - Update followed item limit logic to handle specific item types - Refactor constructor formatting for better readability
- Add unique index on 'token' field for 'push_notification_devices' collection - Add index on 'userId' field for 'push_notification_subscriptions' collection - These indexes improve data integrity and query performance for push notification operations
- Introduce new method `sendBulkNotifications` to IPushNotificationClient - Enhance efficiency for sending notifications to multiple devices - Maintain consistency in method parameters with existing `sendNotification`
…h batch support - Add sendBulkNotifications method for sending to multiple devices - Implement chunking for batches larger than 500 tokens - Use FCM v1 batch endpoint for efficient bulk sending - Refactor existing sendNotification to delegate to new bulk method - Update logging and error handling for bulk operations
- Add sendBulkNotifications method to OneSignalPushNotificationClient - Delegate single notification sending to bulk method - Implement batching for handling large numbers of device tokens - Add error handling for non-HttpExceptions - Update logging for batch processing - Improve method documentation and comments
…very - Refactor device lookup to use a single bulk query instead of per-user queries - Group devices by provider and dispatch notifications in bulk - Use environment variables for push notification provider configs - Improve logging and error handling - Optimize subscription filtering query
- Add comprehensive information about dynamic and personalized notifications - Include details on editorial-driven alerts, user-crafted notification streams, and flexible delivery mechanisms - Highlight provider-agnostic design and support for Firebase (FCM) and OneSignal - Emphasize the advantage of enhanced user engagement and web dashboard management
- Replace the total notification subscription limit check with per-delivery-type limit enforcement. - Simplify the limit assignment logic using ternary operators. - Enhance code readability by adopting a more concise style. - Remove unused notificationSubscriptionLimit variables.
- Remove userRepository from AppDependencies and DefaultPushNotificationService - This change simplifies the push notification service by removing an unused dependency
- Removed the null check for pushConfig as it is not needed. - This change simplifies the code structure and improves readability.
- Added notificationSubscriptions: const [] to the user seed data - This ensures that new users created through database seeding have an empty notificationSubscriptions array by default
…lient - Add Firebase token provider to create and exchange JWT for access token - Introduce reusable HttpClient instance for token exchange - Update OneSignal client initialization
Member
Author
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-architected push notification system. The multi-provider approach with support for Firebase and OneSignal, controlled via remote configuration, is excellent and provides great flexibility. The implementation of breaking news alerts is robust, using a "fire-and-forget" pattern to avoid blocking API responses, which is a great design choice. The new database migrations, indexes, and RBAC permissions are all well-implemented and support the new functionality effectively.
However, I found a critical configuration error in the app_dependencies.dart file where the HttpClient for OneSignal was incorrectly configured with Firebase credentials and endpoints. This would prevent the OneSignal provider from functioning. I've left a detailed comment with a suggested fix for this issue.
Overall, this is a fantastic feature addition. Once the configuration issue is resolved, this will be a solid and scalable notification system.
…r HTTP client" This reverts commit 71d8b79.
Member
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is a well-executed and comprehensive implementation of a multi-provider push notification system. The architecture is robust, featuring a clear separation of concerns with interfaces for notification clients and a central service to orchestrate the logic. The database schema, migrations, and indexing are well-designed for performance and data integrity. Security is also well-handled, with correct permission setup and specific checks for user-owned resources. The use of "fire-and-forget" for sending notifications is a great choice for API responsiveness. I have a couple of suggestions for improvement regarding code structure and error handling in the Firebase client.
… function - Extracted Firebase access token retrieval logic into a separate private function - Improved code readability and maintainability - Reused the extracted function in the firebaseHttpClient initialization
…gging for batch sending - Enhance logging to capture individual failures within a batch - Refine error messages to better reflect partial failures - Ensure all futures complete before processing results, even in case of errors - Introduce more granular error handling for HTTP and unexpected exceptions
Member
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive push notification system with a multi-provider architecture supporting Firebase and OneSignal. The changes are extensive, including new database models, services for sending notifications, dependency injection setup, and RBAC permissions. The implementation is robust, with good abstractions and handling of asynchronous operations. I've identified a critical compilation error in the user preference limit service due to missing break statements in a switch block. I've also provided suggestions to improve maintainability by extracting complex logic from the dependency setup and to enhance error reporting in the Firebase client for better debuggability.
Extracts the Firebase JWT signing and token exchange logic from `app_dependencies.dart` into a new, dedicated `FirebaseAuthenticator` class. This refactoring improves separation of concerns, enhances readability of the dependency setup, and makes the authentication flow testable in isolation.
Refactors `app_dependencies.dart` to use the new `FirebaseAuthenticator` service for obtaining Firebase access tokens. This change removes the inline token generation logic, instantiates the new service, and provides it to the `firebaseHttpClient`. This improves separation of concerns and makes the dependency setup more readable and maintainable.
Updates the root middleware to provide the `IFirebaseAuthenticator` service to the request context. This makes the service available for injection throughout the application.
Member
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent and comprehensive pull request that introduces a robust, multi-provider push notification system. The architecture is well-designed, with clear separation of concerns using interfaces for different notification clients (IPushNotificationClient) and a dedicated service (DefaultPushNotificationService) to orchestrate the logic. The use of RemoteConfig to dynamically switch providers is a great feature for flexibility. The database changes, including new collections, indexes, and migrations, are well-implemented and idempotent. The security considerations, such as RBAC for sending notifications and ownership checks for device registration, are also solid. I have a few minor suggestions for improving readability and noting a design trade-off for future scalability, but overall, this is a very high-quality contribution.
Modify `EnvironmentConfig` to allow missing push notification credentials. The getters now return `null` instead of throwing an error, allowing the application to start even if push notifications are not configured.
…lization - Add null safety to firebaseAuthenticator and push notification clients - Initialize FirebasePushNotificationClient only if credentials are present - Initialize OneSignalPushNotificationClient only if credentials are present - Move push notification client initialization before repository initialization - Log appropriate messages for initialized and disabled push notification clients
Updates `DefaultPushNotificationService` to accept nullable `IPushNotificationClient` instances. This change makes the service resilient to missing push notification credentials. It now checks if the configured primary provider's client was initialized before attempting to send a notification, logging a severe error and aborting if the client is unavailable. This fixes the compilation errors in `app_dependencies.dart`.
- Change REQUIRED to CONDITIONALLY REQUIRED for Firebase and OneSignal variables - Add clarification that the server will start without these variables - Emphasize the importance of these variables for their respective notification services
Adds a null-assertion operator to `firebasePrivateKey` within the `FirebaseAuthenticator`. This is safe as the authenticator is only created when all Firebase credentials are confirmed to be non-null, resolving the compile-time error.
Updates the root middleware to correctly provide the nullable `IFirebaseAuthenticator?` type, resolving a compile-time error. Also sorts the import directives to fix a lint warning.
Member
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-architected push notification system with support for Firebase and OneSignal. The changes include new database collections, migrations, provider-specific clients, and an orchestrating service. The implementation demonstrates strong attention to performance, security, and robustness, with features like fire-and-forget notification dispatching and conditional dependency injection. My review identified one high-severity issue where empty environment variables could lead to runtime errors and one medium-severity issue regarding an incomplete database migration rollback. Overall, this is an excellent contribution that adds significant new functionality to the application.
lib/src/database/migrations/20251107000000_add_is_breaking_to_headlines.dart
Show resolved
Hide resolved
Member
Author
|
/gemini summary |
Summary of ChangesThis pull request delivers a robust and extensible push notification system aimed at significantly boosting user engagement through timely and personalized alerts. It establishes a multi-provider architecture, initially supporting Firebase FCM and OneSignal, with dynamic configuration capabilities managed via RemoteConfig. The system enables editorial-driven breaking news alerts and lays the groundwork for user-defined notification subscriptions, all supported by new database structures, refined access controls, and integrated services. Highlights
Changelog
Activity
|
Updates the `_getEnv` helper to return `null` if an environment variable is found but is an empty string. This makes the configuration handling more robust by preventing services from being initialized with invalid empty credentials, which would cause runtime failures.
github-project-automation
bot
moved this from Backlog
to Done
in Flutter News App Project
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Status
READY
Description
This pull request introduces a robust and extensible push notification system, designed to enhance user engagement through timely and personalized alerts. It establishes a multi-provider architecture, initially supporting Firebase FCM and OneSignal, with dynamic configuration managed via RemoteConfig. Key features include editorial-driven breaking news alerts and the foundational elements for user-defined notification subscriptions, all backed by new database structures, access controls, and service integrations.
Type of Change