-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Adding tracking to recommend the app feature. #15277
Conversation
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
You can test the changes on this Pull Request by downloading the APKs: |
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.
Hey @develric 👋 Thanks for the changes! I have tested the app and can confirm I see the correct messages on logcat both on success and failure. Code is looking pretty good as well! I just left two comments below with a typo I found and a suggestion for a very minor improvement.
As we chatted on Slack, it seems that the recommend_app_content_fetch_failed
event only shows up on Tracks if we remove the error
property from it, so it seems that there's something that needs to be done there.
Other than that, everything looks good to go.
WordPress/src/main/java/org/wordpress/android/models/recommend/RecommendApiCallsProvider.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/main/MeViewModel.kt
Outdated
Show resolved
Hide resolved
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.
LGTM!
Feel free to merge this once all checks are ok.
Thanks @renanferrari for helping with the reviews of these PRs, much appreciated 🙇 ! Just leaving a note that as far as we got, to fix the |
Refs: pctCYC-8X-p2#comment-340 and iOS counter part PR
Part of #15269 this PR does the following:
recommend_app_engaged
: this event is fired when the Share Sheet is succesfully opened: it has asource
parameter that currently is only triggered for "me" given the "about" source is not availablerecommend_app_content_fetch_failed
: this event is fired on failure in getting the remote template; it has asource
parameter (only "me" available) and anerror
parameter that reports the error message. Currently the failure is triggered forTo test
recommend_app_content_fetch_failed
event is triggered on following failures:Regression Notes
Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing and unit testing added.
What automated tests I added (or what prevented me from doing so)
Unit testing for tracking added.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.