-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor/centralize local ad status management #88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor/centralize local ad status management #88
Conversation
...BannerAdState - Remove ContentStatus import - Remove contentStatus field from CreateLocalBannerAdState class - Remove contentStatus parameter from CreateLocalBannerAdState copyWith method - Update CreateLocalBannerAdState props to exclude contentStatus
- Remove CreateLocalBannerAdStatusChanged event class - This class was likely made by accident or is no longer needed
...dBloc - Remove CreateLocalBannerAdStatusChanged event and related handler - Remove contentStatus from CreateLocalBannerAdState - Set status to active on local banner ad creation
...InterstitialAdState - Remove ContentStatus import - Remove contentStatus field from class properties - Exclude contentStatus from copyWith method - Remove contentStatus from equality check and toString method
- Remove CreateLocalInterstitialAdStatusChanged event class - This class was likely removed to clean up unused code and simplify the event structure for creating local interstitial ads
...stitial ad creation - Remove CreateLocalInterstitialAdStatusChanged event and related handler - Remove contentStatus from CreateLocalInterstitialAdState - Set status to active by default when creating a new local interstitial ad
...NativeAdState - Remove ContentStatus field from CreateLocalNativeAdState class - Remove contentStatus parameter from CreateLocalNativeAdState constructor - Remove contentStatus from copyWith method - Update props list to exclude contentStatus
- Remove CreateLocalNativeAdStatusChanged event class - This class was likely replaced by CreateLocalNativeAdContentStatusChanged
...e ad creation - Remove CreateLocalNativeAdStatusChanged event and related handler - Remove contentStatus from CreateLocalNativeAdState - Set native ad status to active on creation instead of using variable
- Remove CreateLocalVideoAdStatusChanged event handler - Remove status field from CreateLocalVideoAdState - Set default status to active when creating a new local video ad
- Remove CreateLocalVideoAdStatusChanged event class - This class was not in use and could potentially cause confusion
...teLocalVideoAdState - Remove contentStatus property from CreateLocalVideoAdState class - Remove contentStatus parameter from CreateLocalVideoAdState copyWith method - Update CreateLocalVideoAdState props list to remove contentStatus
...BannerAdState - Remove contentStatus property from UpdateLocalBannerAdState class - Update isDirty getter to only compare imageUrl and targetUrl - Adjust copyWith method to exclude contentStatus parameter - Remove contentStatus from the class constructor and props list
- Deleted the UpdateLocalBannerAdStatusChanged class from update_local_banner_ad_event.dart - This class was likely made obsolete by a recent change in the app's architecture
- Remove status change event and related logic from UpdateLocalBannerAdBloc - Ensure original status is maintained when submitting ad updates - Update state management to reflect removal of contentStatus
- Remove status change event and method from UpdateLocalInterstitialAdBloc - Update ad submission to maintain original status instead of allowing changes - Remove status field from UpdateLocalInterstitialAdState
- Remove UpdateLocalInterstitialAdStatusChanged event class - This class was likely made obsolete by a previous commit that introduced UpdateLocalAdStatusChanged and replaced all usages of the former with the latter, except for the actual declaration of the former
...teLocalInterstitialAdState - Remove contentStatus field from UpdateLocalInterstitialAdState - Remove contentStatus from the constructor - Update isDirty getter to exclude contentStatus check - Remove contentStatus from copyWith method - Remove contentStatus from the props list
- Remove UpdateLocalNativeAdStatusChanged event handler - Remove contentStatus from UpdateLocalNativeAdState - Keep original status when submitting updated ad - Remove unused status input from update_local_native_ad_form.dart
- Remove UpdateLocalNativeAdStatusChanged event class - This class was likely replaced by DirectContentStatusChanged event in a previous commit
...NativeAdState - Remove contentStatus property from UpdateLocalNativeAdState class - Remove contentStatus from the copyWith method - Remove contentStatus from the equality check - Remove contentStatus from the class properties list
- Remove status update functionality from UpdateLocalVideoAdBloc - Maintain original status when submitting updated ad - Remove related UI components for status display and update
- Remove UpdateLocalVideoAdStatusChanged event class - This class was not being used in the bloc and could potentially cause confusion - Its functionality can be handled by the existing UpdateLocalVideoAdContentStatusChanged event
...VideoAdState - Remove contentStatus field from UpdateLocalVideoAdState - Update isDirty getter to only compare videoUrl and targetUrl - Update copyWith method to exclude contentStatus - Remove contentStatus from props list
- Remove SearchableSelectionInput widget for ContentStatus - Remove unused imports related to ContentStatusL10n and SearchableSelectionInput
- Remove SearchableSelectionInput widget for ContentStatus - Delete unused imports related to ContentStatus and localization
- Remove unused import statements for ContentStatusL10n and SearchableSelectionInput - Remove ContentStatus selection input from the create local native ad form
- Remove SearchableSelectionInput widget for ContentStatus - Remove unused imports related to ContentStatus and localization
- Remove SearchableSelectionInput widget for ContentStatus - Remove unused imports related to ContentStatusL10n - Adjust layout by removing SizedBox after removed widget
- Remove SearchableSelectionInput widget for ContentStatus - Remove unused imports related to ContentStatus localization and widget
- Remove SearchableSelectionInput widget for ContentStatus - Remove unused imports related to ContentStatusL10n
- Remove SearchableSelectionInput widget for ContentStatus - Remove related imports for ContentStatusL10n and SearchableSelectionInput - Simplify the form by removing status selection functionality
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 significant refactoring to the local ad management system, aiming to centralize and simplify how ad statuses are handled. The changes remove the user's ability to manually set or modify an ad's content status during creation or update, ensuring that new ads are always 'active' and updated ads retain their original status. This streamlines the BLoC logic and user interface by eliminating redundant state management for ad statuses. 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
gemini-code-assist
bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively refactors the local ad status management by centralizing the logic. It removes the ability for users to set the status when creating or editing ads. New ads are now created with an active status by default, and editing an ad preserves its original status. The changes are applied consistently across all ad types (banner, interstitial, native, and video), simplifying the BLoCs and UI by removing the status-related fields and events. My review includes a suggestion to improve maintainability by avoiding hardcoded status values. Additionally, the repetitive nature of the changes across numerous files highlights significant code duplication in the BLoC layer. While outside the scope of this PR, a future refactoring to introduce generic BLoCs for ad management could greatly reduce this duplication and improve overall code health.
Uh oh!
There was an error while loading. Please reload this page.
Status
READY
Description
This pull request introduces a significant refactoring to the local ad management system, aiming to centralize and simplify how ad statuses are handled. The changes remove the user's ability to manually set or modify an ad's content status during creation or update, ensuring that new ads are always 'active' and updated ads retain their original status. This streamlines the BLoC logic and user interface by eliminating redundant state management for ad statuses.
Type of Change