-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add analytics properties when setting pref church (KIDS-1465) #1135
Add analytics properties when setting pref church (KIDS-1465) #1135
Conversation
WalkthroughThe changes in this pull request involve modifications 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 (
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
lib/features/family/features/preferred_church/preferred_church_selection_page.dart (2)
124-128
: LGTM! Consider adding a safety check forselectedIndex
.The addition of 'namespace' and 'name' parameters to the
AnalyticsEvent
aligns well with the PR objectives to enhance analytics tracking for preferred church selection. This change will provide valuable insights into user preferences.For added robustness, consider adding a safety check for
selectedIndex
before accessingbloc.state.filteredOrganisations[selectedIndex]
. Although the button should be disabled when no church is selected, it's a good practice to prevent potential index out of range errors. You could use the null-aware operator:parameters: selectedIndex != -1 ? { 'namespace': bloc.state.filteredOrganisations[selectedIndex].nameSpace, 'name': bloc.state.filteredOrganisations[selectedIndex].orgName, } : null,This ensures that even if the button is accidentally enabled when it shouldn't be, the app won't crash.
Line range hint
136-138
: LGTM! Consider extracting the selected organization to improve readability.The changes correctly retrieve the
churchId
from thenameSpace
property of the selected organization, which aligns with the PR objectives. This ensures that the correct identifier is used when setting the preferred church.To improve code readability and reduce repetition, consider extracting the selected organization into a variable:
final selectedOrganization = bloc.state.filteredOrganisations[selectedIndex]; final churchId = selectedOrganization.nameSpace; final success = await widget.setPreferredChurch(churchId); if (success) { await showPreferredChurchSuccessDialog( context, selectedOrganization.orgName, onTap: () => Navigator.of(context) ..pop() ..pop(), ); }This change would make the code more concise and easier to maintain.
📜 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 (1)
lib/features/family/features/preferred_church/preferred_church_selection_page.dart (1)
Line range hint
1-190
: Overall implementation looks good. Verify analytics event firing.The changes successfully implement the addition of analytics properties when setting a preferred church, aligning well with the PR objectives. The code is well-structured and achieves the desired outcome.
To ensure the analytics event is firing correctly, please run the following verification script:
This script will help verify that the analytics event is correctly implemented and that no other conflicting or redundant events are present.
Summary by CodeRabbit
New Features
Bug Fixes