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

[Reader] reader_accessed event not triggerered when app is resumed #19960

Conversation

RenanLukas
Copy link
Contributor

@RenanLukas RenanLukas commented Jan 16, 2024

Fixes #19278


To Test:

1 - Install JP and sign in;
2 - Verify the selected tab correct analytics event is tracked:

- Reader: 🔵 Tracked: reader_accessed
- My Site: 🔵 Tracked: my_site_tab_accessed, Properties: {"blog_id":<id>,"site_type":"blog","is_jetpack":false}
- Notifications: 🔵 Tracked: notifications_accessed
- Me: 🔵 Tracked: me_tab_accessed

3 - Send the app to background (e.g. tap home button) and open it again: the event from step 2 should be tracked again (previously this wasn't happening);
4 - Repeat steps 2-3 with the four tabs.


Regression Notes

  1. Potential unintended areas of impact

    • None
  2. What I did to test those areas of impact (or what existing automated tests I relied on)

    • Manual testing
  3. What automated tests I added (or what prevented me from doing so)

    • This logic is currently implemented in Activity.

PR Submission Checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes Testing Checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • Talkback.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • Large and small screen sizes. (Tablet and smaller phones)
  • Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

@RenanLukas RenanLukas marked this pull request as ready for review January 16, 2024 19:11
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 16, 2024

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr19960-bc98e2a
Commitbc98e2a
Direct Downloadwordpress-prototype-build-pr19960-bc98e2a.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 16, 2024

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr19960-bc98e2a
Commitbc98e2a
Direct Downloadjetpack-prototype-build-pr19960-bc98e2a.apk
Note: Google Login is not supported on these builds.

@daniloercoli daniloercoli self-requested a review January 16, 2024 19:56
@daniloercoli daniloercoli self-assigned this Jan 16, 2024
Copy link
Contributor

@thomashorta thomashorta left a comment

Choose a reason for hiding this comment

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

I found a behavior that I am not 100% sure we want, so I added a comment in the appropriate part of the code.

@@ -1135,7 +1131,7 @@ protected void onResume() {

if (mBottomNav != null) {
PageType currentPageType = mBottomNav.getCurrentSelectedPage();
trackLastVisiblePage(currentPageType, mFirstResume);
trackLastVisiblePage(currentPageType);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Something I noticed after these changes is that now the events are tracked when the device rotates, which doesn't seem desirable.

In case we don't want to track on orientation changes, one idea is to keep some flag and add some logic related to saving and restoring instance state. I didn't double check, but these seem to be the Lifecycle callbacks order during orientation change, so we can probably differentiate between:

  • Orientation change with app in foreground (onSaveInstanceState called before onStop) -> should not track the event again when reaching this line inside onResume;
  • App in background and killed by the system to reclaim memory ( onSaveInstanceState called after onStop) -> should track the event again when reaching this line inside onResume.

But this is just something I thought now, there might be other ways of making sure we track only when we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks

I'm fixing it

Copy link
Contributor

@daniloercoli daniloercoli Jan 23, 2024

Choose a reason for hiding this comment

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

Noticed some events that are tracked twice.

When I tap on the Reader Tab the first time

  • Tracked: reader_accessed
    2024-01-23 09:34:15.463 16331-16331 WordPress-STATS com.jetpack.android.prealpha
  • Tracked: reader_filter_sheet_item_selected
    2024-01-23 09:34:15.518 16331-16331 WordPress-STATS com.jetpack.android.prealpha
  • Tracked: reader_filter_sheet_item_selected
    2024-01-23 09:34:15.810 16331-16331 WordPress-STATS com.jetpack.android.prealpha
  • Tracked: reader_following_shown

I see 2 distinct reader_filter_sheet_item_selected events tracked in analytics.

Rotate the device
Every time i rotate the device i see 2 new reader_filter_sheet_item_selected events, together with deep_link_not_default_handler event, bumped to analytics.

  • Tracked: deep_link_not_default_handler, Properties: {"interceptor_classname":"org.wordpress.android.WPComPostReaderActivity"}
    2024-01-23 09:35:42.628 16331-16331 WordPress-STATS com.jetpack.android.prealpha
  • Tracked: reader_filter_sheet_item_selected
    2024-01-23 09:35:42.676 16331-16331 WordPress-STATS com.jetpack.android.prealpha
  • Tracked: reader_filter_sheet_item_selected

Open the single post details view
The event reader_article_rendered is tracked multiple times.

  • Tracked: reader_post_card_tapped, Properties: {"blog_id":0,"feed_id":35393375,"follow":true,"source":"following"}
    2024-01-23 09:40:11.089 16331-16331 WordPress-STATS com.jetpack.android.prealpha
  • Tracked: app_reviews_significant_event_incremented, Properties: {"source":"opening_reader_post"}
    2024-01-23 09:40:11.451 16331-16331 WordPress-STATS com.jetpack.android.prealpha
  • Tracked: reader_article_opened, Properties: {"follow":true,"feed_id":35393375,"feed_item_id":5086693692,"is_jetpack":false,"site_type":"blog"}
    2024-01-23 09:40:11.599 16331-16331 WordPress-STATS com.jetpack.android.prealpha
  • Tracked: reader_article_rendered, Properties: {"follow":true,"feed_id":35393375,"feed_item_id":5086693692,"is_jetpack":false,"site_type":"blog"}
    2024-01-23 09:40:11.623 16331-16331 WordPress-STATS com.jetpack.android.prealpha
  • Tracked: reader_article_rendered, Properties: {"follow":true,"feed_id":153742481,"feed_item_id":5086695973,"is_jetpack":false,"site_type":"blog"}

Going back to the reader from the Single post details view
I see the event reader_accessed is tracked again. Not sure we'd like to bump this again here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for finding these tracking issues, @daniloercoli

Since they're not related to this PR, I've created a new issue to take care of it.

@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@RenanLukas
Copy link
Contributor Author

Thanks for the review, @thomashorta

I've fixed the issue you've mentioned and now the event is not tracked when the screen orientation changes. It's still tracked when the system kills the app and the Activity is recreated (I've tested this using Don't Keep Activities under developer options).

@dangermattic
Copy link
Collaborator

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

Copy link
Contributor

@thomashorta thomashorta left a comment

Choose a reason for hiding this comment

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

Thanks for the addition of the "isChangingConfiguration" check, it looks like it works as desired now!

I see @daniloercoli added some other concerns about other Reader analytics events that are being triggered multiple times (#19960 (comment)). I think it would be worth taking a look at them, but as another GH issue/PR, as it is not in the scope of the current PR.

Copy link
Contributor

@daniloercoli daniloercoli left a comment

Choose a reason for hiding this comment

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

LGTM!

@RenanLukas RenanLukas merged commit d651b40 into feature/reader-ia Jan 23, 2024
20 checks passed
@RenanLukas RenanLukas deleted the issue/19278-reader-accessed-event-not-triggerered-app-background-and-back branch January 23, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants