Refactor/sync with core update #94
Conversation
- Update core package ref from d047d9cc to 93ca1905 - This update ensures the project uses the latest version of the core package
…cations - Add ModelConfig for Interest and InAppNotification - Define permissions for CRUD operations on interests and notifications - Set appropriate ownership checks and permission types - Handle special cases like unsupported POST for notifications
- Refactor SavedFilter and PushNotificationSubscription into a unified Interest model - Update RemoteConfig to use a new interestConfig field - Migrate user_content_preferences and remote_configs collections - Add migration script to convert legacy data to new format
- Add new migration file for unifying interests and remote config - Update migration list to include the new migration
- Initialize Data Clients for new Interest and InAppNotification models - Add corresponding repositories to AppDependencies class - Update pushNotificationService to use new interestRepository - Remove unused pushNotificationSubscriptionRepository
- Remove redundant checkAddItem method - Update checkUpdatePreferences to be a placeholder - Prepare for interest-specific validation using new InterestConfig
…breaking news flow - Remove PushNotificationSubscription repository, replace with Interest repository - Update logic to query interests instead of subscriptions - Adjust variable names and comments to reflect new terminology
- Remove savedFilters and notificationSubscriptions from initial values - Add interests to initial values
- Add indexes for the interests collection - Add indexes for the in-app notifications collection, including a TTL index for automatic document expiration - Rename notificationSubscriptions to interests in user_content_preferences
- Implement read, readAll, create, update, and delete operations for interest and in-app notification - Add custom creator and updater for interest with limit checking using UserPreferenceLimitService - Register new data operations in DataOperationRegistry
…tion - Replace PushNotificationSubscription provider with Interest provider - Add InAppNotification provider - Update repository dependencies accordingly
- Add batch number and total batches to log messages - Improve log information for failed batch sends - Include batch details in error logging
- Add missing space in IPushNotificationClient documentation - Improve clarity of the class purpose and functionality
- Add doc comments to class and constructor - Clarify purpose and functionality of the class - Explain OAuth2 token exchange process - Describe scope claim and its usage
…nal parameters - Add batchNumber and totalBatches parameters to _sendBatch function - Update batch sending logic to use new parameters - Improve tracking and monitoring of batch processing
BREAKING CHANGE: The DefaultUserPreferenceLimitService has been completely rewritten to implement the new UserPreferenceLimitService interface. The obsolete checkUpdatePreferences method has been removed. The unused _permissionService field has been removed. A new checkInterestLimits method is implemented to enforce all InterestConfig limits (total, pinned, and notification subscriptions) for a given user role. A new checkUserContentPreferencesLimits method is implemented to enforce all UserPreferenceConfig limits (followed items and saved headlines).
This change updates the DataOperationRegistry to correctly call the new methods on the UserPreferenceLimitService. The custom creator and updater for the 'interest' model now use the correct named arguments (user, interest) when calling checkInterestLimits, resolving the compile-time errors. A custom updater has been added for 'user_content_preferences' to call the new checkUserContentPreferencesLimits method, ensuring that limits for followed items and saved headlines are now correctly enforced before any update.
- Removed permissionService parameter from DefaultUserPreferenceLimitService initialization
The PUT handler in routes/api/v1/data/[id]/index.dart contained a manual check for user_content_preferences that called the old checkUpdatePreferences method, which has been removed. This caused a compile-time error. This change removes the obsolete if block. The validation logic is now correctly and exclusively handled by the custom updater for user_content_preferences within the DataOperationRegistry, which was implemented in a previous step. This resolves the error and ensures the correct validation path is used.
…tification service - Remove DataRepository<Interest> and related initialization - Update PushNotificationService to use userContentPreferencesRepository instead of interestRepository - Adjust log message for InAppNotification data client initialization
- Removed user-owned interest permissions from the Permissions class - These permissions are no longer used in the application - This change simplifies the permissions structure and reduces redundancy
- Remove permissions related to creating, reading, updating, and deleting owned interests for app guest users - This change aligns with the requirement that app guests should not be able to manage their interests
- Remove interest model from model registry - Remove interest operations from data operation registry - Update user preference handling to manage interests within UserContentPreferences - Simplify interest-related permission checks and validations
- Removed the code responsible for creating indexes for the 'interests' collection - This change optimizes the database seeding process by eliminating unnecessary indexing
- Replace interest-based query with user preferences-based query - Improve efficiency by directly targeting embedded interests array - Simplify user ID collection process - Update logging to reflect new query approach
- Remove the unused provider for DataRepository<Interest> - This change simplifies the middleware setup and removes unnecessary code
…ptions - Add validation for 'criteria' in saved filters and notification subscriptions - Skip processing for malformed criteria instead of throwing an error - Log a warning message for skipped filters or subscriptions
- Introduce the concept of "Interests" to explain user-created content filters - Clarify that users subscribe to notifications for specific Interests - Improve readability and understanding of the user-crafted notification feature
Summary of ChangesHello @fulleni, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a major refactoring of user preference management by unifying content filtering and notification subscriptions under a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 significant refactoring to unify saved filters and notification subscriptions into a new 'Interest' model. The changes are comprehensive, including a well-structured database migration, updates to dependency injection, and refactoring of services to support the new data model. The overall approach improves the architecture by centralizing user preferences. I have identified two high-severity issues: one concerning the robustness of the database migration against potentially invalid data, which could halt the process, and another regarding a regression where administrators can no longer bypass preference limits. My review provides specific suggestions to address these points.
lib/src/database/migrations/20251111000000_unify_interests_and_remote_config.dart
Show resolved
Hide resolved
This refactoring consolidates the UserPreferenceLimitService to use a single, holistic method (checkUserContentPreferencesLimits) for all UserContentPreferences validations. The service interface and implementation have been updated to use a single method that compares the updated preferences with the current state to determine what has changed and apply the correct limits for interests, followed items, and saved headlines. The custom updater for user_content_preferences in DataOperationRegistry has been significantly simplified. It no longer contains complex logic to detect changes and now makes a single call to the unified service method. This change improves architectural purity by centralizing all limit-checking logic within the service, making the system more robust and easier to maintain.
- Removed unused import statement for SetEqualityHelper - This change simplifies the code and removes an unnecessary dependency
… bypass This commit applies two important fixes based on code review feedback: Robust Migration Script: The UnifyInterestsAndRemoteConfig migration script has been hardened. It now wraps the parsing of deliveryTypes in a try-catch block. This prevents the entire server from crashing on startup if it encounters malformed data (an invalid enum string) in a user's legacy notificationSubscriptions, making the migration process more resilient. Correct Admin Bypass: The administrator bypass for user preference limits has been re-implemented in the architecturally correct location. Instead of coupling the UserPreferenceLimitService with the PermissionService, the check is now performed in the user_content_preferences custom updater within data_operation_registry.dart. This ensures that the limit service is only called if the user does not have the userPreferenceBypassLimits permission, maintaining a clean separation of concerns.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring to unify SavedFilter and PushNotificationSubscription into a single Interest model, which is a great improvement for consistency and maintainability. The changes are extensive, touching database migrations, dependency injection, services, and registries. The new database migration is well-structured, and the updates to the services and registries are mostly correct. However, I found a critical bug in the new preference limit checking logic that could allow users to bypass subscription limits. I've also included a couple of minor suggestions to fix a typo and remove a duplicated comment. Overall, this is a solid refactoring that moves the codebase in the right direction.
This commit fixes a critical bug in the DefaultUserPreferenceLimitService where interest-specific limits (pinned filters and notification subscriptions) were not being checked correctly. Previously, the logic only ran if it detected a single added or updated interest, which meant the limits were completely bypassed if a user updated their preferences without changing an interest, or if they changed multiple interests at once. The logic has been corrected to be stateless. It now validates the entire proposed updatedPreferences state on every request, ensuring that the total counts for pinned filters and all notification subscription types are always checked against the user's role-based limits. This closes the loophole that could have allowed users to exceed their configured limits.
- Remove unnecessary comment block in FirebaseAuthenticator constructor - Retain important inline comment about HttpClient usage
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring that unifies SavedFilter and PushNotificationSubscription into a new, more flexible Interest model. The changes are comprehensive, touching data models, repositories, services, and introducing a database migration to handle the schema change. The introduction of a custom updater for user_content_preferences in the DataOperationRegistry is a great improvement, centralizing complex validation logic. Additionally, the PR introduces a new InAppNotification feature with proper indexing and permissions. My review found one medium-severity issue related to an unused parameter that leads to an unnecessary database call, but overall the changes are high quality and improve the codebase's maintainability and functionality.
This refactoring removes the unused currentPreferences parameter from the UserPreferenceLimitService and its implementation. The checkUserContentPreferencesLimits method signature has been updated in both the interface and the concrete implementation. The custom updater in data_operation_registry.dart no longer performs an unnecessary database read to fetch the current preferences, improving efficiency. This change simplifies the code and aligns it with the stateless validation logic, where only the final proposed state (updatedPreferences) is needed for limit checks.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring, unifying SavedFilter and PushNotificationSubscription into a more flexible Interest model. The changes span across the application, from database migrations and dependency injection to services and RBAC, and are consistently implemented. The introduction of InAppNotification with a TTL index is also a valuable addition. Overall, this is a high-quality change that improves the data model. I have one high-severity comment regarding the down method in the new database migration, which needs correction to ensure schema compatibility if a rollback is ever required.
lib/src/database/migrations/20251111000000_unify_interests_and_remote_config.dart
Show resolved
Hide resolved
github-project-automation
bot
moved this from Backlog
to Done
in Flutter News App Project
Status
READY
Description
Type of Change