Remove duplicate tracking (KIDS-1465) #1148
Conversation
WalkthroughThe changes in this pull request introduce a new enumeration value to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
lib/features/family/features/preferred_church/preferred_church_selection_page.dart (1)
127-131: Approve changes with a suggestion for improved robustnessThe addition of
namespaceandorgnameparameters to theAnalyticsEventconstructor aligns well with the PR objective of including church data in Amplitude tracking when users set their preferred church. This change enhances the tracking accuracy as requested.However, to improve robustness, consider adding a safety check:
Consider adding a null check or boundary check before accessing
filteredOrganisations[selectedIndex]to prevent potential runtime errors. Here's a suggested implementation:analyticsEvent: AnalyticsEvent( AmplitudeEvents.preferredChurchSelected, parameters: { - 'namespace': - bloc.state.filteredOrganisations[selectedIndex].nameSpace, - 'orgname': bloc.state.filteredOrganisations[selectedIndex].orgName, + if (selectedIndex >= 0 && selectedIndex < bloc.state.filteredOrganisations.length) ...{ + 'namespace': bloc.state.filteredOrganisations[selectedIndex].nameSpace, + 'orgname': bloc.state.filteredOrganisations[selectedIndex].orgName, + }, }, ),This change ensures that the analytics event is still created even if there's an issue with the
selectedIndex, preventing potential crashes while maintaining data integrity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- lib/features/family/features/preferred_church/preferred_church_selection_page.dart (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
lib/features/family/features/preferred_church/preferred_church_selection_page.dart (2)
Line range hint
135-172: Approve removal of redundant analytics loggingThe removal of the analytics event logging from the
_onTapConfirmmethod is appropriate and aligns with the PR objective of removing duplicate tracking. This change consolidates the analytics tracking to a single point in thefloatingActionButton, which is a cleaner and more maintainable approach.The core functionality of setting the preferred church and displaying the appropriate dialogs remains intact, ensuring that the user experience is not affected by this change.
Line range hint
1-172: Summary: Changes successfully address PR objectivesThe modifications in this file effectively address the PR objectives by:
- Removing duplicate tracking of analytics events.
- Including church data (namespace and organization name) in Amplitude tracking when users select their preferred church.
The changes are well-implemented and maintain the overall functionality of the
PreferredChurchSelectionPage. The consolidation of analytics tracking to thefloatingActionButtonimproves code maintainability.A minor suggestion was made to enhance robustness when accessing the selected organization's data. Overall, the changes are approved and align well with the intended goals of the PR.
lib/features/family/features/preferred_church/preferred_church_selection_page.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
lib/core/enums/amplitude_events.dart (2)
282-282: LGTM with a minor suggestion for consistencyThe addition of
preferredChurchConfirmClickedaligns well with the PR objectives of including church data in Amplitude tracking and removing duplicate tracking. The placement and naming convention are appropriate.Consider updating the event value to match the enum name more closely:
- preferredChurchConfirmClicked('preferred_church_clicked'), + preferredChurchConfirmClicked('preferred_church_confirm_clicked'),This change would maintain consistency with the enum name and provide more precise tracking information.
Line range hint
1-382: Consider restructuring the AmplitudeEvents enum for improved maintainabilityWhile the current implementation works, the large number of events in a single enum might lead to maintainability issues as the application grows. Consider the following suggestions to improve the structure:
- Group related events into separate enums or use a nested enum structure if supported by your version of Dart.
- Consistently use comments to separate different groups of events.
- Consider using a prefix naming convention for different types of events (e.g.,
userAction,systemEvent,errorEvent).Here's an example of how you might restructure a portion of the enum:
enum AmplitudeEvents { // User Actions userActionLetsGo('user_action_lets_go'), userActionLoginClicked('user_action_login_clicked'), // ... other user actions // System Events systemEventAccountLocked('system_event_account_locked'), systemEventWalletTracker('system_event_wallet_tracker'), // ... other system events // Error Events errorEventTopupFailed('error_event_topup_failed'), errorEventFailedToCreateMember('error_event_failed_to_create_member'), // ... other error events // Registration Flow registrationContinueByEmailSignUpNewUserClicked('registration_continue_by_email_sign_up_new_user_clicked'), registrationContinueByEmailSignUpTempUserClicked('registration_continue_by_email_sign_up_temp_user_clicked'), // ... other registration events // ... other event categories }This structure would make it easier to locate and add new events, as well as understand the purpose of each event at a glance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- lib/core/enums/amplitude_events.dart (1 hunks)
- lib/features/family/features/preferred_church/preferred_church_selection_page.dart (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/family/features/preferred_church/preferred_church_selection_page.dart
🧰 Additional context used
🔇 Additional comments (1)
lib/core/enums/amplitude_events.dart (1)
Line range hint
1-382: Summary of review for AmplitudeEvents enum
- The addition of
preferredChurchConfirmClickedhas been approved with a minor suggestion for the event value.- A recommendation for restructuring the enum for better maintainability has been provided.
- The changes align with the PR objectives of including church data in Amplitude tracking and removing duplicate tracking.
Overall, the changes look good and contribute to improving the tracking system as intended.
Summary by CodeRabbit