customerio / customerio-android

This is the official Customer.io SDK for Android.
MIT License
11 stars 9 forks source link

feat: improve push click behavior #247

Closed mrehan27 closed 8 months ago

mrehan27 commented 10 months ago

helps: https://github.com/customerio/issues/issues/10830

Changes

Pending Items

Note

This is base PR for improving push click behavior. Changes planned in this PR directly are final, but will be kept in draft till the feature is complete. Further changes for the feature will be merged through more PRs into this branch and then released after final testing.

github-actions[bot] commented 10 months ago
# Sample app builds 📱 Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request. --- * java_layout: `rehan/push-click-behavior (1699951544)` * kotlin_compose: `rehan/push-click-behavior (1699951564)`
codecov[bot] commented 10 months ago

Codecov Report

Attention: 105 lines in your changes are missing coverage. Please review.

Comparison is base (c494bb0) 49.76% compared to head (2032b89) 53.59%. Report is 23 commits behind head on main.

Files Patch % Lines
...inginapp/gist/presentation/engine/EngineWebView.kt 0.00% 28 Missing :warning:
...ava/io/customer/messagingpush/util/DeepLinkUtil.kt 5.00% 19 Missing :warning:
...stomer/messaginginapp/gist/data/listeners/Queue.kt 21.42% 11 Missing :warning:
...saginginapp/gist/presentation/GistModalActivity.kt 0.00% 11 Missing :warning:
sdk/src/main/java/io/customer/sdk/CustomerIO.kt 0.00% 11 Missing :warning:
...push/activity/NotificationClickReceiverActivity.kt 63.15% 5 Missing and 2 partials :warning:
...essagingpush/processor/PushMessageProcessorImpl.kt 87.03% 5 Missing and 2 partials :warning:
...ssaginginapp/gist/presentation/GistModalManager.kt 0.00% 5 Missing :warning:
...stomer/messaginginapp/gist/presentation/GistSdk.kt 33.33% 2 Missing :warning:
...messagingpush/CustomerIOPushNotificationHandler.kt 80.00% 2 Missing :warning:
... and 2 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #247 +/- ## ============================================ + Coverage 49.76% 53.59% +3.82% - Complexity 237 278 +41 ============================================ Files 108 109 +1 Lines 2781 2812 +31 Branches 364 352 -12 ============================================ + Hits 1384 1507 +123 + Misses 1282 1183 -99 - Partials 115 122 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 10 months ago

Build available to test Version: rehan-push-click-behavior-SNAPSHOT Repository: https://s01.oss.sonatype.org/content/repositories/snapshots/

mrehan27 commented 10 months ago

@levibostian Yes the change prevents new activity from being tracked by CIO auto-screen tracking. The PR does exactly what is described in the task breakdown. Current approach is much better than solutions like hard-coding filters. And even if it adds a feature, it does without breaking existing functionality and requires minimal effort on our part. I don’t see any reason for not continuing with it. If you have any suggestions for improvement or see any issues with this approach, feel free to mention them. Otherwise, let’s continue the discussion in the parent ticket or on Slack.

levibostian commented 10 months ago

I am open to a solution such as this one where an interface is used to determine if a screen should be tracked or not.

My hesitations come up from:

I have an idea to suggest. Would it work for a refactor to the PR where TrackableScreen is renamed to DoNotTrackScreen and the interface is internal or undocumented? If it's an interface that is currently only used internally (not used in sample apps), I would be OK with this. Then, we can expose this interface to the public and make it a documented feature in the future if we believe it's important to add.

Update: I find the rename to DoNotTrackScreen to be optional. As long as the interface is internal and removed from sample app use, I would be OK with the solution. I do agree, I find an interface to be better then a hard-coded list of Activities.

mrehan27 commented 10 months ago

@levibostian I actually started implementing this with DoNotTrackScreen but ended up on TrackableScreen. Because I think the number of changes are almost same but TrackableScreen is more flexible. I'm okay with removing it from sample apps though if we have more votes on it.

For the interface, I don't think this can be confusing for customers because we are not yet going to add this in docs. If the customers find out the interface themselves, the doc on it already explains it is fully optional. We can discuss any improvements in docs or name on the file, if you have any ideas in my mind, please post it on TrackableScreen so it is easier to discuss in thread.

levibostian commented 10 months ago

In general, I believe that code should not be added to a sample app unless it's a public facing feature that is fully documented.

Therefore, I would approve of the PR if TrackableScreen was internal (either in scope or a comment on it specifying that it's currently only used internally) and all references to it were removed from the sample apps.

As you have mentioned, I agree that if we do decide to make this a public feature add in the the future, naming and improvements could be discussed at that time. For internal use, this looks good to me.