Feature: Create donation flow (kids-1421) #1085
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several changes across multiple files, primarily enhancing the Changes
Possibly related PRs
Suggested reviewers
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 (
|
| @@ -53,6 +55,15 @@ void initCubits() { | |||
| ..registerLazySingleton<CameraCubit>( | |||
| CameraCubit.new, | |||
| ) | |||
| ..registerLazySingleton<MediumCubit>(MediumCubit.new) | |||
| ..registerLazySingleton<GiveBloc>( | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (11)
lib/features/family/features/parent_giving_flow/presentation/pages/parent_amount_page.dart (2)
81-91: Improved floating action button layout and positioningThe changes to the
floatingActionButtonandfloatingActionButtonLocationenhance the user interface:
- Centering the button at the bottom of the screen improves accessibility and follows material design guidelines.
- Adding horizontal padding ensures the button doesn't stretch edge-to-edge on larger screens, improving aesthetics.
- The functionality of the button (navigation and analytics) remains intact.
These improvements align well with creating an effective donation flow.
Consider adding some bottom padding to the
Paddingwidget to ensure the button isn't too close to the edge of the screen on devices with rounded corners or gesture areas. For example:floatingActionButton: Padding( - padding: const EdgeInsets.symmetric(horizontal: 24), + padding: const EdgeInsets.fromLTRB(24, 0, 24, 16), child: FunButton( // ... existing code ), ),
Line range hint
31-35: Minor improvements for consistency and clarity
- The
initialamountvariable doesn't follow Dart naming conventions. Consider renaming it toinitialAmount.- For clarity, you might want to add a comment explaining why the initial amount is set to 5.
Apply this diff to address these points:
- final initialamount = 5; + // Default initial donation amount set to 5 (currency units) + final initialAmount = 5; late int _amount; @override void initState() { super.initState(); - _amount = initialamount; + _amount = initialAmount; }Also, update the
FunCounterwidget to useinitialAmount:FunCounter( currency: widget.currency, - initialAmount: initialamount, + initialAmount: initialAmount, onAmountChanged: (amount) => setState(() { _amount = amount; }), ),lib/features/family/features/parent_giving_flow/presentation/pages/give_from_list_page.dart (3)
33-38: Good refactoring: Consistent use of dependency injection.The changes in the
listenercallback are good:
- Using the
givevariable instead ofcontext.read<GiveBloc>()improves consistency and readability.- Switching to
getIt<MediumCubit>()aligns with the dependency injection approach.These changes maintain the existing logic while improving the code structure.
For consistency, consider extracting
getIt<MediumCubit>()to a local variable at the beginning of thebuildmethod, similar to howgiveis handled:final give = getIt<GiveBloc>(); final mediumCubit = getIt<MediumCubit>();This would make the code even more consistent and easier to maintain.
77-77: Good refactoring: Consistent use of dependency injection in navigation method.The changes in the
_navigateToGivingScreenmethod are appropriate:
- Using
getIt<MediumCubit>()andgetIt<GiveBloc>()is consistent with the dependency injection approach.- The logic for setting the medium ID and adding the
GiveAmountChangedevent is preserved.- Removing the dependency on
BuildContextfor accessing these instances improves the method's flexibility.For consistency and to potentially improve performance, consider extracting the
getItcalls to local variables at the beginning of the method:Future<void> _navigateToGivingScreen( BuildContext context, CollectGroup collectGroup, ) async { final mediumCubit = getIt<MediumCubit>(); final giveBloc = getIt<GiveBloc>(); // ... rest of the method ... mediumCubit.setMediumId(collectGroup.nameSpace); // ... rest of the method ... giveBloc.add( GiveAmountChanged( // ... event parameters ... ), ); }This would make the code more consistent with the changes in the
buildmethod and potentially reduce repeated calls togetIt.Also applies to: 98-104
Line range hint
1-107: Overall: Excellent refactoring towards dependency injection.The changes in this file represent a significant improvement in code structure and maintainability:
- Consistent use of
getItfor dependency injection throughout the file.- Reduced reliance on
BuildContextfor accessing bloc and cubit instances.- Improved readability with the introduction of local variables for frequently used instances.
These changes make the code more modular, easier to test, and align with best practices for state management in Flutter.
To further improve the architecture:
- Consider creating a
ServiceLocatorclass to encapsulate allgetItcalls. This would make it easier to mock dependencies in tests and provide a single point of control for dependency injection.- If not already done, ensure that the
GiveBlocandMediumCubitare registered as singletons in the dependency injection setup to maintain consistent state across the app.lib/features/family/app/injection.dart (2)
59-66: LGTM: GiveBloc registered as a lazy singleton, but consider clarifying dependenciesThe
GiveBlocis correctly registered as a lazy singleton with its dependencies. However, it would be helpful to add comments explaining what each of the fourgetIt()calls is retrieving. This would improve code readability and maintainability.Example:
..registerLazySingleton<GiveBloc>( () => GiveBloc( getIt(), // SomeRepository getIt(), // AnotherService getIt(), // SomeConfig getIt(), // Logger ), )
11-11: Summary: New components added for donation flowThese changes add the necessary dependency injection setup for
MediumCubitandGiveBloc, which are likely key components in the new donation flow mentioned in the PR objectives. The additions are well-structured and consistent with the existing codebase style.To further improve this PR:
- Consider adding comments to clarify the dependencies injected into
GiveBloc.- Ensure that unit tests are updated or added to cover these new injections.
- Verify that the
GiveBlocandMediumCubitare properly disposed of when no longer needed to prevent memory leaks.Also applies to: 26-26, 58-66
lib/features/family/features/parent_giving_flow/presentation/pages/parent_giving_page.dart (2)
87-91: LGTM: Consistent use ofgivevariableThe changes in the
_buildGivtmethod consistently use thegivevariable to access the state, which is a good improvement. It simplifies the code and makes it more readable.For consistency, consider using
give.state.organisation.organisationNameon line 89 as well:var orgName = give.state.organisation.organisationName!;This makes the code more uniform and easier to maintain.
Also applies to: 99-99, 105-105
Line range hint
171-185: Consider movingCustomInAppBrowserto a separate fileThe
CustomInAppBrowserclass at the end of this file seems unrelated to the mainParentGivingPagewidget. To improve code organization and maintainability, consider moving this class to a separate file, perhaps namedcustom_in_app_browser.dartin an appropriate directory.This separation of concerns will make the codebase more modular and easier to navigate.
lib/features/children/generosity_challenge/assignments/create_challenge_donation/pages/choose_amount_slider_page.dart (1)
43-43: LGTM! Consider makinggiveprivate.The introduction of the
giveinstance using dependency injection is a good practice. It improves testability and flexibility of the code.Consider making the
giveinstance private by prefixing it with an underscore:- final give = getIt<GiveBloc>(); + final _give = getIt<GiveBloc>();This encapsulation prevents accidental access from outside the class.
lib/core/enums/amplitude_events.dart (1)
336-337: LGTM! Consider adding documentation for the new event.The new enum value
parentReflectionFlowOrganisationClickedis correctly implemented and follows the established conventions in the file. It's appropriately placed at the end of the enum list, which helps maintain backwards compatibility.Consider adding a brief comment above this new enum value to describe its purpose or the specific user action it represents. This would be helpful for other developers who might work with this code in the future.
Example:
/// Tracks when a parent clicks on an organisation during the reflection flow parentReflectionFlowOrganisationClicked( 'parent_reflection_flow_organisation_clicked'),
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- lib/core/enums/amplitude_events.dart (1 hunks)
- lib/features/children/generosity_challenge/assignments/create_challenge_donation/pages/choose_amount_slider_page.dart (4 hunks)
- lib/features/family/app/family_routes.dart (1 hunks)
- lib/features/family/app/injection.dart (3 hunks)
- lib/features/family/features/parent_giving_flow/presentation/pages/give_from_list_page.dart (4 hunks)
- lib/features/family/features/parent_giving_flow/presentation/pages/parent_amount_page.dart (1 hunks)
- lib/features/family/features/parent_giving_flow/presentation/pages/parent_giving_page.dart (3 hunks)
- lib/features/family/features/reflect/bloc/grateful_cubit.dart (2 hunks)
- lib/features/family/features/reflect/presentation/pages/grateful_screen.dart (4 hunks)
Additional comments not posted (15)
lib/features/family/features/parent_giving_flow/presentation/pages/give_from_list_page.dart (2)
5-5: LGTM: Import for dependency injection added.The addition of
import 'package:givt_app/app/injection/injection.dart';is appropriate for using thegetItdependency injection container in this file.
27-29: Good refactoring: Improved dependency management.The introduction of the
givevariable and its use in theBlocConsumeris a good refactoring:
- It centralizes the retrieval of the
GiveBlocinstance.- It improves code readability by reducing repetitive calls to
getIt.- It's consistent with the move towards dependency injection.
This change enhances maintainability and follows good coding practices.
lib/features/family/features/reflect/bloc/grateful_cubit.dart (1)
46-47: Verify the intended behavior of profile filteringThe condition for removing profiles has been simplified to only remove those with a null gratitude value. This change now includes all profiles (both child and non-child) in the
_profileslist, as long as they have a non-null gratitude value.Please confirm if this is the intended behavior, as it may affect downstream logic that previously assumed only child profiles were present in the list.
To verify the impact, you can run the following script:
Review the results to ensure that the logic in these locations can handle both child and non-child profiles correctly.
Verification successful
Profile filtering change verified successfully
The modification to remove profiles with a null gratitude value does not interfere with the existing usage of the
isChildproperty. Profiles are now correctly filtered to include all with non-null gratitude, and sorting logic based onisChildremains unaffected.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of _profiles that might assume only child profiles # Test 1: Search for direct usage of _profiles echo "Searching for direct usage of _profiles:" rg --type dart '_profiles' lib/features/family/features/reflect/ # Test 2: Search for methods that use _profiles indirectly echo "Searching for methods that might use _profiles indirectly:" rg --type dart 'getCurrentProfile|_getCurrentProfile|onAvatarTapped|onDonated' lib/features/family/features/reflect/Length of output: 4162
Script:
#!/bin/bash # Description: Check for usage of `isChild` in relation to _profiles # Test 1: Search for direct usage of isChild within _profiles echo "Searching for direct usage of isChild with _profiles:" rg --type dart '_profiles.*isChild' lib/features/family/features/reflect/ # Test 2: Identify methods that rely on isChild for profile filtering echo "Searching for methods or conditions that filter _profiles based on isChild:" rg --type dart '_profiles.*isChild' lib/features/family/features/reflect/Length of output: 429
Script:
#!/bin/bash # Description: Search for all usages of the `isChild` property in the codebase # Test 1: Search for any usage of `isChild` echo "Searching for all usages of 'isChild':" rg --type dart 'isChild' lib/features/family/features/reflect/ # Test 2: Search for conditional statements involving `isChild` echo "Searching for conditional statements involving 'isChild':" rg --type dart 'if\s*\(.*isChild.*\)' lib/features/family/features/reflect/ # Test 3: Search for functions or methods that return `isChild` echo "Searching for functions or methods that return 'isChild':" rg --type dart '=>\s*.*isChild' lib/features/family/features/reflect/Length of output: 1446
Script:
#!/bin/bash # Description: Search for all indirect usages of the `isChild` property related to _profiles # Test 1: Search for methods that return `isChild` and are used within GratefulCubit echo "Searching for methods returning 'isChild' used in GratefulCubit:" rg --type dart 'bool\s+\w+\s*\(.*\)\s*=>\s*.*isChild' lib/features/family/features/reflect/bloc/grateful_cubit.dart # Test 2: Search for conditional statements in GratefulCubit that use `isChild` echo "Searching for conditional statements involving 'isChild' in GratefulCubit:" rg --type dart 'if\s*\(.*isChild.*\)' lib/features/family/features/reflect/bloc/grateful_cubit.dart # Test 3: Search for any usage of `isChild` in function parameters or return types within GratefulCubit echo "Searching for any usage of 'isChild' in function parameters or return types in GratefulCubit:" rg --type dart '\bisChild\b' lib/features/family/features/reflect/bloc/grateful_cubit.dartLength of output: 996
lib/features/family/app/injection.dart (2)
11-11: LGTM: New imports added for MediumCubit and GiveBlocThe new import statements are correctly added to support the changes in the
initCubitsfunction.Also applies to: 26-26
58-58: LGTM: MediumCubit registered as a lazy singletonThe
MediumCubitis correctly registered as a lazy singleton, which is appropriate for its usage pattern.lib/features/family/features/parent_giving_flow/presentation/pages/parent_giving_page.dart (3)
67-68: LGTM: Improved state accessThe change to use
give.state.afterGivingRedirectioninstead of the context-based access is a good improvement. It's consistent with the new approach and makes the code more readable.
103-103: Status of TODO commentThere's a TODO comment regarding Safari Givt transactions:
message: context.l10n.safariGivtTransaction,What's the status of this TODO? Is there any ongoing work or plans to address this Safari-specific functionality?
If this TODO is still relevant, consider creating a GitHub issue to track this task. If it's no longer relevant, please remove the TODO comment to keep the codebase clean.
31-31: Overall improvement in state managementThe consistent use of the
givevariable for accessingGiveBlocstate is a good refactoring that improves code readability and maintainability. It's a positive step towards better dependency injection and state management.To ensure these changes don't have unintended consequences, please verify:
- That all usages of
GiveBlocin this file have been updated.- That similar refactoring has been applied consistently across other files using
GiveBloc.You can use the following script to check for any remaining context-based
GiveBlocaccesses in this file and others:This will help ensure the refactoring is complete and consistent across the codebase.
Also applies to: 67-68, 87-91, 99-99, 105-105
lib/features/children/generosity_challenge/assignments/create_challenge_donation/pages/choose_amount_slider_page.dart (4)
55-55: LGTM! Consistent use of the injected bloc.The change to use the
giveinstance in theBlocListeneris consistent with the earlier introduction of the instance. This removes the need for context-based bloc access, which is a good practice.
Line range hint
190-203: LGTM! Consistent use of the injected bloc.The change to use the
giveinstance for adding events in the_handleStripeRegistrationSuccessmethod is consistent with the earlier introduction of the instance. This improves code consistency and removes the need for context-based bloc access.
215-215: LGTM! Consistent use of the injected bloc.The change to use the
giveinstance for adding the error event in the_handleStripeOrDonationErrormethod is consistent with the earlier introduction of the instance. This improves code consistency and removes the need for context-based bloc access.
Line range hint
1-224: Overall, great improvements to the code structure!The changes in this file consistently introduce dependency injection for the
GiveBloc, replacing context-based bloc access. This improves testability, flexibility, and overall code quality. The logic remains unchanged, ensuring that the functionality is preserved while the code structure is enhanced.lib/features/family/app/family_routes.dart (2)
298-298: LGTM! Verify state management in ParentGivingPageThe simplification of the
ParentGivingPageroute by removing theBlocProvider.valuewrapper improves code readability and aligns with the transition to dependency injection. This change is approved.To ensure that the
ParentGivingPagestill has access to the necessary state management logic, please run the following verification script:#!/bin/bash # Description: Verify that ParentGivingPage can access required state management logic # Test: Check if ParentGivingPage uses getIt for dependency injection ast-grep --lang dart --pattern 'class ParentGivingPage { $$$ getIt<$_>($_) $$$ }' # Test: Check if ParentGivingPage uses any Bloc-related functionality rg --type dart -e 'BlocBuilder|BlocListener|BlocConsumer|context\.read<|context\.watch<' lib/features/family/features/parent_giving_flow/presentation/pages/parent_giving_page.dart
Line range hint
1-1000: Approve refactoring to centralized dependency injectionThe systematic removal of
BlocProviderinstances forGiveBlocandMediumCubitthroughout the file indicates a shift towards a more centralized dependency injection approach usinggetIt. This refactoring can lead to improved performance and easier testing.To ensure the consistency and correctness of this refactoring, please run the following verification script:
Please review the results to ensure that:
- No instances of
BlocProviderforGiveBlocorMediumCubitremain in the routes file.- Affected pages are using
getItfor dependency injection.- Bloc-related functionality is still accessible in the affected pages.
lib/features/family/features/reflect/presentation/pages/grateful_screen.dart (1)
66-72: Review potential event loop inBlocListener.Adding an event to
_givewithin aBlocListeneron_givecould potentially cause an unintended event loop.Ensure that the event added does not trigger the same listener condition, leading to a loop.
lib/features/family/features/parent_giving_flow/presentation/pages/parent_giving_page.dart
Show resolved
Hide resolved
lib/features/family/features/reflect/presentation/pages/grateful_screen.dart
Show resolved
Hide resolved
lib/features/family/features/reflect/presentation/pages/grateful_screen.dart
Outdated
Show resolved
Hide resolved
| BuildContext context, | ||
| Organisation org, | ||
| ) async { | ||
| _medium.setMediumId(org.namespace); |
There was a problem hiding this comment.
Do we need
await context.read<ProfilesCubit>().setActiveProfile(profile.userId);
here?
There was a problem hiding this comment.
no, thats for kids
There was a problem hiding this comment.
the GiveBloc uses the userid from the AuthCubit, not ProfilesCubit
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
# Conflicts: # lib/core/enums/amplitude_events.dart # lib/features/family/features/reflect/presentation/pages/grateful_screen.dart
…ivtnl/givt-app into feature/kids-1421-create-donation-flow
Summary by CodeRabbit
Release Notes
New Features
parentReflectionFlowOrganisationClickedfor enhanced analytics tracking.GiveBlocandMediumCubitthrough dependency injection, improving code maintainability.GratefulScreenfunctionality with improved state handling and navigation for donation flows.Bug Fixes
GratefulCubitto retain all children regardless of gratitude status.Refactor