Skip to content
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

Move to new giving cubit and endpoint for US adults (kids-1499 & kids-1501) #1235

Conversation

Daniela510
Copy link
Contributor

@Daniela510 Daniela510 commented Nov 5, 2024

Remove dependence on legacy EU GiveBloc
Introduce a smaller, simpler GiveCubit for US
Switch to new endpoint for parent giving
Add variable to indicate if it is a gratitude game donation (Parents and kids) (KIDS-1501)

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced GiveCubit for improved state management in the giving flow.
    • Added isActOfService parameter to transaction-related screens and models.
  • Removed Features

    • Eliminated the ImpactGroupDetailsPage and related components, streamlining the app's navigation.
    • Removed impactGroupDetails route and associated enum entries.
  • Bug Fixes

    • Updated state handling in various screens to ensure smoother transitions and error management.
  • Refactor

    • Transitioned from GiveBloc to GiveCubit, simplifying the architecture and enhancing clarity in state management.

Copy link
Contributor

coderabbitai bot commented Nov 5, 2024

Warning

Rate limit exceeded

@Daniela510 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 54 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 6a08f6d and 2bfc4c0.

Walkthrough

The pull request introduces significant changes to the routing and state management of the application. The impactGroupDetails route has been removed from the AppRouter, affecting navigation. The GiveBloc has been replaced with GiveCubit across multiple files, simplifying state management. New properties and methods have been added to the Transaction and GiveCubit classes, enhancing functionality. Additionally, several UI components related to impact group details have been deleted, streamlining the application’s structure.

Changes

File Path Change Summary
lib/app/routes/app_router.dart Removed GoRoute for Pages.impactGroupDetails, reorganized routing under Pages.home.
lib/features/family/app/injection.dart Replaced GiveBloc with GiveCubit in dependency injection, simplifying constructor parameters.
lib/features/family/features/giving_flow/create_transaction/models/transaction.dart Added optional boolean property isActOfService to Transaction, updated constructor and toJson method.
lib/features/family/features/giving_flow/screens/choose_amount_slider_screen.dart Updated constructor to include isActOfService, modified transaction creation logic.
lib/features/family/features/parent_giving_flow/cubit/give_cubit.dart Introduced GiveCubit class, added createTransaction and reset methods for transaction state management.
lib/features/family/features/parent_giving_flow/cubit/give_state.dart Added GiveState sealed class with various state subclasses for managing transaction flow.
lib/features/family/features/parent_giving_flow/presentation/pages/give_from_list_page.dart Transitioned from GiveBloc to GiveCubit, updated state handling and navigation logic.
lib/features/family/features/parent_giving_flow/presentation/pages/parent_amount_page.dart Reorganized import statements without functional changes.
lib/features/family/features/parent_giving_flow/presentation/pages/parent_giving_page.dart Transitioned from GiveBloc to GiveCubit, simplified state management and removed unnecessary checks.
lib/features/family/features/reflect/presentation/pages/grateful_screen.dart Transitioned from GiveBloc to GiveCubit, updated transaction handling logic.
lib/features/give/models/givt_transaction.dart Added constructor GivtTransaction.fromTransaction(Transaction transaction) for easier instantiation.
lib/features/impact_groups/pages/impact_group_details_page.dart Deleted ImpactGroupDetailsPage class.
lib/features/impact_groups/widgets/impact_group_details_bottom_panel.dart Deleted ImpactGroupDetailsBottomPanel class.
lib/features/impact_groups/widgets/impact_group_details_expandable_description.dart Deleted ImpactGroupDetailsExpandableDescription class.
lib/features/impact_groups/widgets/impact_group_details_header.dart Deleted ImpactGroupDetailsHeader class.
lib/app/routes/pages.dart Removed impactGroupDetails enum entry from Pages enum.
lib/core/enums/amplitude_events.dart Added parentGaveSuccessfully enum value, removed impactGroupDetailsReadMoreClicked and impactGroupDetailsGiveClicked.
lib/features/family/app/family_pages.dart Removed impactGroupDetails enum entry from FamilyPages.

Assessment against linked issues

Objective Addressed Explanation
Add variable when making donations through game flow (KIDS-1501) No behavior tracking or logging added.

Suggested reviewers

  • TammiLion

Poem

🐇 In the meadow where bunnies play,
Changes hop in a bright new way.
With routes removed, and cubits in sight,
Our app dances with joy, oh what a delight!
Let's give a cheer for the code we adore,
Hopping forward, we build and explore! 🌼


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the enhancement New feature or request label Nov 5, 2024
@Daniela510 Daniela510 changed the title Move to new giving cubit and endpoint for US adults (kids-1499) Move to new giving cubit and endpoint for US adults (kids-1499 & kids-1501) Nov 6, 2024
@Daniela510 Daniela510 marked this pull request as ready for review November 6, 2024 08:12
@Daniela510 Daniela510 requested a review from a team as a code owner November 6, 2024 08:12
Comment on lines -76 to -85

if (afterGivingRedirection.isNotEmpty) {
final url = Uri.parse(afterGivingRedirection);
LoggingInfo.instance.info(
'Redirecting after external link donation. Attempting to launch $url',
);
if (!await launchUrl(url)) {
LoggingInfo.instance.error('Could not launch $url');
throw Exception('Could not launch $url');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an outside app integration that the US does not have or advertise

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impact groups deleted pages are unused code, they were pages to give to a goal/group as a parent in US before the 1 app merge

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

🧹 Outside diff range and nitpick comments (19)
lib/features/family/features/giving_flow/create_transaction/models/transaction.dart (3)

7-7: Add documentation for the new isActOfService field.

The implementation looks good, but please add documentation to explain that this field indicates whether the donation was made through the gratitude game. This will help maintain clarity for future developers.

  final String? goalId;
+ /// Indicates whether this transaction was created through the gratitude game flow.
  final bool isActOfService;

Also applies to: 14-14


16-23: Consider adding logging for transaction creation.

Per the PR objectives (KIDS-1501), technical logging should be implemented in Logit. Consider adding logging when transactions are created, especially for tracking gratitude game donations.

Would you like me to help implement the logging integration?


7-7: Consider renaming isActOfService for clarity.

The property name isActOfService might not clearly indicate its connection to the gratitude game. Consider a more explicit name like isGratitudeGameDonation or isFromGratitudeGame to better reflect its purpose.

- this.isActOfService = false,
+ this.isGratitudeGameDonation = false,

- final bool isActOfService;
+ final bool isGratitudeGameDonation;

- 'isActOfService': isActOfService,
+ 'isGratitudeGameDonation': isGratitudeGameDonation,

Also applies to: 14-14

lib/features/family/features/parent_giving_flow/cubit/give_state.dart (2)

1-12: LGTM! Consider adding logging capabilities.

The base state hierarchy is well-structured with proper use of sealed class and Equatable.

Consider adding a mixin or interface for logging state transitions to Logit, as mentioned in KIDS-1501. This would help track the flow of states during the donation process.

Example approach:

mixin GiveStateLogging {
  void logStateTransition(GiveState state) {
    // Log to Logit based on state type
  }
}

14-32: Document mediumId usage and consider technical debt.

The state maintains both legacy and new transaction objects, which could lead to maintenance challenges.

Consider:

  1. Adding documentation for mediumId purpose and valid values
  2. Creating a migration plan to phase out givtTransactionObject
  3. Adding @deprecated annotation to legacy fields:
+ /// Legacy transaction object for browser page compatibility
+ /// @deprecated Use [transaction] instead
  final GivtTransaction givtTransactionObject;
lib/features/family/features/parent_giving_flow/cubit/give_cubit.dart (2)

11-15: Add documentation for the public API.

Consider adding documentation comments to describe the purpose and behavior of the GiveCubit class and its public methods.

+/// Manages the state for transaction creation in the giving flow.
+/// Handles the creation of transactions and maintains the associated states.
 class GiveCubit extends Cubit<GiveState> {
+  /// Creates a [GiveCubit] with the required [CreateTransactionRepository].
   GiveCubit(
     this._createTransactionRepository,
   ) : super(GiveInitial());

37-39: Add documentation and tracking to reset method.

Consider adding documentation and tracking to better understand when and why users reset the giving flow.

+  /// Resets the giving flow state to its initial state.
   void reset() {
+    // Track reset action
+    _analyticsService.logEvent('giving_flow_reset');
     emit(GiveInitial());
   }
lib/features/give/models/givt_transaction.dart (1)

Line range hint 1-109: Consider adding logging support for transaction tracking

Given the PR objectives mention technical logging requirements for Logit:

  1. Consider adding debug information or validation logging in the fromTransaction constructor
  2. Add transaction type tracking (regular vs gratitude game) for Amplitude analytics

Would you like me to propose a logging implementation that integrates with your existing logging infrastructure?

lib/features/family/features/parent_giving_flow/presentation/pages/parent_giving_page.dart (1)

Line range hint 66-77: Enhance error handling and analytics tracking.

The browser closing logic should include:

  1. Error logging to Logit as specified in KIDS-1501
  2. Analytics tracking for both successful and failed completions
      give.reset();
+     // Track successful completion
+     AnalyticsHelper.logEvent('parent_giving_flow_completed');

      unawaited(
        context.read<ImpactGroupsCubit>().fetchImpactGroups(),
      );

      context.pop(true);
    } catch (e) {
+     // Log error to Logit
+     LoggingInfo.instance.error(
+       'Error closing browser in parent giving flow',
+       error: e,
+     );
+     // Track failure
+     AnalyticsHelper.logEvent('parent_giving_flow_failed', 
+       properties: {'error': e.toString()});
      setState(() {
        browserClosing = false;
      });
    }
lib/features/family/features/giving_flow/screens/choose_amount_slider_screen.dart (2)

Line range hint 116-124: Add isActOfService to analytics tracking.

To fulfill the PR objective of tracking behavior in Amplitude, include the isActOfService flag in analytics events:

 analyticsEvent: AnalyticsEvent(
   AmplitudeEvents.giveToThisGoalPressed,
   parameters: {
     AnalyticsHelper.goalKey: collectgroup.name,
     AnalyticsHelper.amountKey: state.amount,
     AnalyticsHelper.walletAmountKey:
         profilesCubit.state.activeProfile.wallet.balance,
+    'isActOfService': isActOfService,
   },
 ),

Line range hint 44-57: Improve error handling and recovery mechanisms.

The current error handling implementation could be enhanced:

  1. Add structured logging to Logit
  2. Provide more specific error messages based on failure type
  3. Consider adding retry mechanism for transient failures

Example implementation:

 if (state is CreateTransactionErrorState) {
-  log(state.errorMessage);
+  LogitHelper.error(
+    'Transaction creation failed',
+    error: state.errorMessage,
+    metadata: {
+      'collectgroup': collectgroup.name,
+      'amount': state.amount,
+      'isActOfService': isActOfService,
+    },
+  );
+  final message = _getErrorMessage(state.errorMessage);
   SnackBarHelper.showMessage(
     context,
-    text: 'Cannot create transaction. Please try again later.',
+    text: message,
     isError: true,
+    action: SnackBarAction(
+      label: 'Retry',
+      onPressed: () => _retryTransaction(context),
+    ),
   );
 }
lib/features/family/features/reflect/presentation/pages/grateful_screen.dart (2)

66-66: Consider enhancing error handling with more specific error messages.

While the transition to GiveCubit is correct, the error handling could be more informative. Consider:

  1. Logging the specific error details to Logit (as mentioned in KIDS-1501)
  2. Providing more specific error messages based on the error type
 if (state is GiveError) {
+  final errorMessage = state.error?.toString() ?? context.l10n.somethingWentWrong;
+  // Log to Logit as per KIDS-1501
+  print('Giving error: $errorMessage'); // Replace with proper logging
   ScaffoldMessenger.of(context).showSnackBar(
     SnackBar(
-      content: Text(context.l10n.somethingWentWrong),
+      content: Text(errorMessage),
     ),
   );
 }

Also applies to: 70-74


236-244: Consider adding Amplitude tracking for gratitude game donations.

The transaction creation looks good and correctly implements the isActOfService tracking. However, based on KIDS-1501 objectives, consider adding Amplitude tracking specifically for donations made through the gratitude game.

 await _give.createTransaction(
   transaction: Transaction(
     userId: context.read<AuthCubit>().state.user.guid,
     mediumId: base64Encode(utf8.encode(org.namespace)),
     amount: result.toDouble(),
     isActOfService: true,
   ),
   orgName: org.name,
   mediumId: org.namespace,
 );
+// Add Amplitude tracking for gratitude game donations
+unawaited(
+  AnalyticsHelper.logEvent(
+    eventName: AmplitudeEvents.gratitudeGameDonation,
+    eventProperties: {
+      'amount': result,
+      'organisation': org.name,
+      'mediumId': org.namespace,
+    },
+  ),
+);
lib/app/routes/app_router.dart (3)

Line range hint 516-594: Enhance error handling and logging in external link redirection

The redirection logic handles various scenarios but could benefit from improved error handling and logging, especially given the PR objectives mention technical logging requirements.

Consider adding:

  1. Error logging for invalid/malformed parameters
  2. Amplitude tracking for redirection paths
  3. Structured logging for debugging
 static FutureOr<String?> _redirectFromExternalLink(
   BuildContext context,
   GoRouterState state,
 ) {
   final auth = context.read<AuthCubit>().state;
   var code = '';
   var navigatingPage = '';
   var afterGivingRedirection = '';

+  // Log incoming navigation attempt
+  AnalyticsHelper.logEvent(
+    eventName: AmplitudeEvents.externalLinkRedirection,
+    eventProperties: {
+      'uri': state.uri.toString(),
+      'auth_status': auth.status.toString(),
+    },
+  );

   UTMHelper.trackToAnalytics(uri: state.uri);

   if (state.uri.queryParameters.containsKey('code')) {
     code = state.uri.queryParameters['code']!;
+    // Log code parameter for debugging
+    Logger.d('External link code parameter: $code');
   }

Line range hint 595-644: Simplify biometric authentication routing logic

The current implementation has separate biometric check paths for US and EU users. This could be simplified to reduce code duplication.

Consider refactoring to:

   if (state.status == AuthStatus.biometricCheck) {
-    await context.pushNamed(
-      state.user.isUsUser
-          ? FamilyPages.permitUSBiometric.name
-          : Pages.permitBiometric.name,
-      extra: PermitBiometricRequest.login(),
-    );
+    final biometricPage = state.user.isUsUser
+        ? FamilyPages.permitUSBiometric
+        : Pages.permitBiometric;
+    await context.pushNamed(
+      biometricPage.name,
+      extra: PermitBiometricRequest.login(),
+    );
     return;
   }

Line range hint 1-644: Consider splitting router configuration for better maintainability

The router configuration file has grown quite large and handles multiple concerns. This could make maintenance and testing more difficult.

Consider:

  1. Splitting routes into feature-specific router configurations
  2. Creating a separate class for authentication-related navigation logic
  3. Moving redirection logic to a dedicated service

Example structure:

// auth_router.dart
class AuthRouter {
  static List<GoRoute> get routes => [
    // Authentication related routes
  ];
}

// giving_router.dart
class GivingRouter {
  static List<GoRoute> get routes => [
    // Giving related routes
  ];
}

// navigation_service.dart
class NavigationService {
  static Future<String?> handleExternalLink(/* params */) {
    // External link handling logic
  }
}
lib/features/family/features/parent_giving_flow/presentation/pages/give_from_list_page.dart (3)

30-30: Consider Using CubitConsumer for Clarity

Since GiveCubit extends Cubit, consider using CubitConsumer instead of BlocConsumer. This enhances readability and clearly indicates that a Cubit is being used.

Apply this diff to make the change:

-return BlocConsumer<GiveCubit, GiveState>(
+return CubitConsumer<GiveCubit, GiveState>(

33-41: Ensure Comprehensive State Handling

Currently, the listener handles GiveFromBrowser and GiveError states. To prevent potential issues, ensure that all possible states emitted by GiveCubit are appropriately managed. This includes handling unexpected states to maintain robust state management.


89-96: Avoid Redundant mediumId Parameters

The mediumId is being passed both within the Transaction object and separately to the createTransaction method. This redundancy could cause confusion and potential bugs if the values differ.

Consider refactoring to eliminate the redundant parameter. Here's how you might adjust the code:

 await getIt<GiveCubit>().createTransaction(
     transaction: Transaction(
         userId: context.read<AuthCubit>().state.user.guid,
         mediumId: base64Encode(utf8.encode(collectGroup.nameSpace)),
         amount: result.toDouble(),
     ),
     orgName: collectGroup.orgName,
-    mediumId: collectGroup.nameSpace);
+    );
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 2437967 and 743b651.

📒 Files selected for processing (15)
  • lib/app/routes/app_router.dart (1 hunks)
  • lib/features/family/app/injection.dart (2 hunks)
  • lib/features/family/features/giving_flow/create_transaction/models/transaction.dart (1 hunks)
  • lib/features/family/features/giving_flow/screens/choose_amount_slider_screen.dart (2 hunks)
  • lib/features/family/features/parent_giving_flow/cubit/give_cubit.dart (1 hunks)
  • lib/features/family/features/parent_giving_flow/cubit/give_state.dart (1 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 (4 hunks)
  • lib/features/family/features/reflect/presentation/pages/grateful_screen.dart (6 hunks)
  • lib/features/give/models/givt_transaction.dart (1 hunks)
  • lib/features/impact_groups/pages/impact_group_details_page.dart (0 hunks)
  • lib/features/impact_groups/widgets/impact_group_details_bottom_panel.dart (0 hunks)
  • lib/features/impact_groups/widgets/impact_group_details_expandable_description.dart (0 hunks)
  • lib/features/impact_groups/widgets/impact_group_details_header.dart (0 hunks)
💤 Files with no reviewable changes (4)
  • lib/features/impact_groups/pages/impact_group_details_page.dart
  • lib/features/impact_groups/widgets/impact_group_details_bottom_panel.dart
  • lib/features/impact_groups/widgets/impact_group_details_expandable_description.dart
  • lib/features/impact_groups/widgets/impact_group_details_header.dart
✅ Files skipped from review due to trivial changes (1)
  • lib/features/family/features/parent_giving_flow/presentation/pages/parent_amount_page.dart
🔇 Additional comments (13)
lib/features/family/features/parent_giving_flow/cubit/give_cubit.dart (1)

1-40: Verify US-specific implementation requirements.

The PR objectives mention this cubit is specifically for US adults. Please verify that:

  1. This implementation properly handles US-specific requirements
  2. There are appropriate guards to prevent non-US users from using this flow
lib/features/give/models/givt_transaction.dart (1)

Line range hint 1-109: Verify API compatibility with transaction mapping changes

The changes to transaction mapping might affect existing API endpoints. We should verify:

  1. If the omission of isActOfService in toJson() is intentional
  2. If existing API endpoints expect the current JSON structure

Let's check for API usage patterns:

lib/features/family/features/parent_giving_flow/presentation/pages/parent_giving_page.dart (1)

11-11: LGTM: Import change aligns with cubit migration.

The transition from GiveBloc to GiveCubit import is consistent with the PR's objective of moving to the new giving implementation.

lib/features/family/app/injection.dart (2)

15-15: LGTM: Import statement for new GiveCubit.

The import is correctly placed and follows the feature organization structure.


70-73: Verify required dependencies for tracking and logging.

The GiveCubit registration is significantly simplified compared to the previous GiveBloc. Given the PR objectives:

  1. Are the dependencies for tracking gratitude game donations included?
  2. How is Amplitude tracking being handled?
  3. Where is the Logit logging integration?

Let's check the GiveCubit implementation:

✅ Verification successful

Tracking and logging are properly handled - no changes needed

The review concern is addressed as the code shows:

  1. Logging is handled via LoggingInfo.instance.error() in the GiveCubit's error handling
  2. Transaction tracking is handled at the repository layer through the FamilyAPIService
  3. The GiveCubit's simplified dependency injection is appropriate as it:
    • Uses the repository for transaction creation
    • Includes proper error logging
    • Maintains transaction state through the cubit states
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify GiveCubit implementation includes required dependencies
# Expected: Find references to tracking/logging services in GiveCubit

echo "Checking GiveCubit implementation..."
rg -A 10 "class GiveCubit" "lib/features/family/features/parent_giving_flow/cubit/give_cubit.dart"

echo "Checking for tracking/logging imports..."
rg -l "amplitude|logit" "lib/features/family/features/parent_giving_flow/cubit/give_cubit.dart"

Length of output: 730


Script:

#!/bin/bash
# Let's check the complete implementation of GiveCubit and its imports
echo "Checking complete GiveCubit file content..."
cat "lib/features/family/features/parent_giving_flow/cubit/give_cubit.dart"

# Let's also check if there are any tracking/logging services injected in the repository
echo -e "\nChecking CreateTransactionRepository implementation..."
rg -l "CreateTransactionRepository" --type dart

# Check the repository implementation for tracking
echo -e "\nChecking repository content..."
fd "create_transaction_repository.dart" --exec cat {}

Length of output: 3260


Script:

#!/bin/bash
# Let's check the FamilyAPIService implementation for tracking
echo "Checking FamilyAPIService implementation..."
fd "family_api_service.dart" --exec cat {}

# Let's also check if there are any tracking services in the injection file
echo -e "\nChecking injection file for tracking services..."
rg -A 5 "Amplitude|LoggingService|Track" "lib/features/family/app/injection.dart"

Length of output: 9893

lib/features/family/features/giving_flow/screens/choose_amount_slider_screen.dart (1)

33-33: LGTM! Parameter addition follows best practices.

The addition of isActOfService parameter is well-implemented with proper immutability and default value.

Also applies to: 37-37

lib/features/family/features/reflect/presentation/pages/grateful_screen.dart (2)

2-2: LGTM: Import and type changes align with PR objectives.

The changes correctly implement the transition from GiveBloc to GiveCubit and add necessary imports for the new functionality.

Also applies to: 12-12, 15-15, 49-49


177-177: LGTM: Consistent implementation of isActOfService parameter.

The isActOfService parameter is correctly set to true for both ChooseAmountSliderScreen and SuccessScreen, maintaining consistency in tracking gratitude game donations.

Also applies to: 193-194

lib/features/family/features/parent_giving_flow/presentation/pages/give_from_list_page.dart (5)

2-2: Import dart:convert Confirmed

The import of dart:convert is necessary for encoding functionalities used later in the code, such as base64Encode and utf8.encode.


12-13: Essential Imports Added

Imports for Transaction and GiveCubit have been correctly added. These are essential for handling transaction creation and managing the application's state with the new GiveCubit.


29-29: Proper Use of Dependency Injection with getIt

The GiveCubit instance is appropriately retrieved using getIt<GiveCubit>(), ensuring consistent dependency management across the application.


46-46: Loading Indicator Implementation

Using a conditional to display a loading indicator when giveState is GiveLoading provides a good user experience by informing users of ongoing processes.


92-92: Verify Encoding of mediumId

The mediumId is being set using base64Encode(utf8.encode(collectGroup.nameSpace)). Confirm that this double encoding is required and that all parts of the system interacting with mediumId expect it in this format.

Run the following script to check how mediumId is used across the codebase and ensure consistency in its encoding:

✅ Verification successful

Based on the search results, I can now provide a final response:

Double Encoding of mediumId is Required

The double encoding (base64Encode(utf8.encode())) in give_from_list_page.dart is correct and consistent with the codebase's handling of mediumId. This is verified by:

  • The decoding logic in give_bloc.dart which performs the reverse operation: utf8.decode(base64.decode(encodedMediumId))
  • The same encoding pattern is also used in grateful_screen.dart
  • Other parts of the system expect the mediumId to be base64 encoded when processing transactions
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage and encoding of mediumId in the codebase.

# Search for instances where mediumId is assigned in Transaction initializations
rg --type dart 'mediumId: ' -A 5

# Search for places where mediumId is decoded or processed
rg --type dart 'base64Decode|utf8\.decode' -A 5

Length of output: 20540

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
lib/features/family/features/parent_giving_flow/cubit/give_state.dart (2)

10-12: Add documentation for state classes.

Consider adding documentation comments to clarify when these states are emitted.

+/// State representing the initial state of the giving flow before any action is taken
 final class GiveInitial extends GiveState {}

+/// State indicating that a giving operation is in progress
 final class GiveLoading extends GiveState {}

14-32: Consider completing the EU to US giving migration.

The state contains both legacy GivtTransaction and new Transaction objects. Since the PR objectives mention moving away from EU giving, consider:

  1. Creating a migration plan to fully transition to the new US-specific Transaction
  2. Improving documentation using dartdoc format
 final class GiveFromBrowser extends GiveState {
   const GiveFromBrowser(
     this.givtTransactionObject,
     this.transaction,
     this.orgName,
     this.mediumId,
   );

-  // this is legacy for the browser page
+  /// @deprecated Legacy transaction object for browser page
+  /// TODO: Remove once EU to US giving migration is complete
   final GivtTransaction givtTransactionObject;
 
-  // this is the object we use for parent giving in US
+  /// Transaction object for US parent giving flow
   final Transaction transaction;
+  /// Name of the organization receiving the donation
   final String orgName;
+  /// Identifier for the donation medium/channel
   final String mediumId;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 743b651 and 6a08f6d.

📒 Files selected for processing (7)
  • lib/app/routes/pages.dart (0 hunks)
  • lib/core/enums/amplitude_events.dart (1 hunks)
  • lib/features/family/app/family_pages.dart (0 hunks)
  • lib/features/family/features/parent_giving_flow/cubit/give_cubit.dart (1 hunks)
  • lib/features/family/features/parent_giving_flow/cubit/give_state.dart (1 hunks)
  • lib/features/family/features/parent_giving_flow/presentation/pages/parent_amount_page.dart (2 hunks)
  • lib/features/family/features/parent_giving_flow/presentation/pages/parent_giving_page.dart (4 hunks)
💤 Files with no reviewable changes (2)
  • lib/app/routes/pages.dart
  • lib/features/family/app/family_pages.dart
🚧 Files skipped from review as they are similar to previous changes (2)
  • lib/features/family/features/parent_giving_flow/presentation/pages/parent_amount_page.dart
  • lib/features/family/features/parent_giving_flow/presentation/pages/parent_giving_page.dart
🔇 Additional comments (7)
lib/features/family/features/parent_giving_flow/cubit/give_state.dart (1)

3-8: LGTM! Well-structured base state class.

The sealed class with Equatable is a good choice for type-safe state management.

lib/features/family/features/parent_giving_flow/cubit/give_cubit.dart (5)

1-10: LGTM! Imports are well-organized and complete.

All necessary dependencies for state management, logging, and analytics are properly imported.


12-17: LGTM! Clean dependency injection pattern.

The constructor follows best practices with proper dependency injection and initial state setup.


47-49: LGTM! Clean reset implementation.

The reset method is simple and focused, properly resetting the state to initial.


18-45: ⚠️ Potential issue

Add missing tracking requirements and enhance error handling.

While the basic implementation is good, there are still missing requirements from KIDS-1501:

  1. The Amplitude tracking should include the isActOfService (gratitude game) property
  2. Transaction initiation tracking is missing
  3. Error messages could be more descriptive

These issues were previously identified in an earlier review. Please refer to the previous review comment for the detailed implementation suggestion that includes:

  • Adding transaction initiation tracking
  • Including isActOfService in analytics
  • Enhancing error messages
  • Adding failed transaction tracking

12-50: Verify complete migration from GiveBloc to GiveCubit.

Let's ensure all GiveBloc references have been properly migrated to the new GiveCubit implementation.

lib/core/enums/amplitude_events.dart (1)

256-256: LGTM: New parent giving event follows conventions

The addition of parentGaveSuccessfully event follows the established naming conventions and is properly categorized under the parent giving flow section.

secondCollectionAmount: 0,
thirdCollectionAmount: 0,
await _give.createTransaction(
transaction: Transaction(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing I would do differently is let the cubit create the transaction instead of the widget. Now the widget is a bit too smart and every widget that uses it needs to know to base64encode etc.

@Daniela510 Daniela510 merged commit eea74b2 into develop Nov 7, 2024
1 check passed
@Daniela510 Daniela510 deleted the feature/kids-1499-move-to-new-giving-endpoint-for-us-parents branch November 7, 2024 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants