-
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
IA Reader add dedicated tracking for time spend in sub-filtered streams. #11325
Conversation
… reader subfilter.
You can test the changes on this Pull Request by downloading the APK here. |
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.
Thank you @develric! I've tested the app and everything worked as expected.
On the code side, I really liked that you were able to move the tracking logic to the ViewModel
and even have some tests for it. Great job there! 👍
I just left some comments on minor improvements you may want to consider.
WordPress/src/main/java/org/wordpress/android/ui/reader/tracker/ReaderTracker.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostListViewModel.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostListViewModel.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostListViewModel.kt
Outdated
Show resolved
Hide resolved
# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderPostListFragment.java # WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostListViewModel.kt # WordPress/src/test/java/org/wordpress/android/viewmodel/reader/ReaderPostListViewModelTest.kt
WordPress/src/test/java/org/wordpress/android/viewmodel/reader/ReaderPostListViewModelTest.kt
Outdated
Show resolved
Hide resolved
Hi @renanferrari, thanks for the review and suggestions 😊. I should have addressed your comments. I also merged back develop and resolve some conflicts so I would appreciate if you could make a quick re-test run on this also from your side. Thanks in advance 🙇♂️! |
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.
Hi @develric! I have tested the app again and everything seems to be working as expected. I have also reviewed the changes you made to the code and they look good 👍
However, it looks like the connected tests started failing after you merged the develop branch. I rerun the workflow a couple of times, but no deal. I'm still looking into it to see if I can figure out what's happening.
Edit: The failing tests don't seem to be related to this PR. LGTM
Fixes #11324
This is an addition to the already merged PR #10956 and adds the
time_in_subfiltered_list
property to the already availableapplication_closed
event.This is a specific event of the IA revised reader (zalpha flavor) and reports the seconds spent in the main reader
FOLLOWING
tab when aSITE
orTAG
is selected in the sub-filter.To test
(TRACK READER|Tracked: application_closed)
and Regex activeTracked: application_closed, Properties: {"time_in_main_reader":20,"last_visible_screen":"Reader","time_in_reader_paged_post":5,"time_in_app":34,"time_in_subfiltered_list":10,"time_in_reader_filtered_list":0}
and the reported timings are somewhat similar to the expected
PR submission checklist:
RELEASE-NOTES.txt
if necessary.