fix: push opened metrics tracked on Android 12 #184

mrehan27 · 2023-04-04T09:47:54Z

fixes: https://github.com/customerio/issues/issues/9483

Changes

Updated instanceOrNull method to support modules initialization
Updated CustomerIOFirebaseMessagingService to initialize push module when we initialize SDK using context

github-actions · 2023-04-04T09:48:19Z

Pull request title looks good 👍!

If this pull request gets merged, it will cause a new release of the software. Example: If this project's latest release version is 1.0.0. If this pull request gets merged in, the next release of this project will be 1.0.1. This pull request is not a breaking change.

All merged pull requests will eventually get deployed. But some types of pull requests will trigger a deployment (such as features and bug fixes) while some pull requests will wait to get deployed until a later time.

This project uses a special format for pull requests titles. Expand this section to learn more (expand by clicking the ᐅ symbol on the left side of this sentence)...

This project uses a special format for pull requests titles. Don't worry, it's easy!

This pull request title should be in this format:

<type>: short description of change being made

If your pull request introduces breaking changes to the code, use this format:

<type>!: short description of breaking change

where <type> is one of the following:

feat: - A feature is being added or modified by this pull request. Use this if you made any changes to any of the features of the project.
fix: - A bug is being fixed by this pull request. Use this if you made any fixes to bugs in the project.
docs: - This pull request is making documentation changes, only.
refactor: - A change was made that doesn't fix a bug or add a feature.
test: - Adds missing tests or fixes broken tests.
style: - Changes that do not effect the code (whitespace, linting, formatting, semi-colons, etc)
perf: - Changes improve performance of the code.
build: - Changes to the build system (maven, npm, gulp, etc)
ci: - Changes to the CI build system (Travis, GitHub Actions, Circle, etc)
chore: - Other changes to project that don't modify source code or test files.
revert: - Reverts a previous commit that was made.

Examples:

feat: edit profile photo
refactor!: remove deprecated v1 endpoints
build: update npm dependencies
style: run formatter

Need more examples? Want to learn more about this format? Check out the official docs.

Note: If your pull request does multiple things such as adding a feature and makes changes to the CI server and fixes some bugs then you might want to consider splitting this pull request up into multiple smaller pull requests.

codecov · 2023-04-04T09:49:04Z

Codecov Report

Merging #184 (14e84a0) into main (a410e6d) will decrease coverage by 0.52%.
The diff coverage is 30.55%.

@@             Coverage Diff              @@
##               main     #184      +/-   ##
============================================
- Coverage     63.52%   63.01%   -0.52%     
  Complexity      218      218              
============================================
  Files            91       91              
  Lines          2051     2082      +31     
  Branches        263      270       +7     
============================================
+ Hits           1303     1312       +9     
- Misses          646      667      +21     
- Partials        102      103       +1

Impacted Files	Coverage Δ
...essagingpush/CustomerIOFirebaseMessagingService.kt	`0.00% <0.00%> (ø)`
...stomer/sdk/CustomerIOActivityLifecycleCallbacks.kt	`7.40% <0.00%> (-4.36%)`	⬇️
sdk/src/main/java/io/customer/sdk/CustomerIO.kt	`74.33% <81.81%> (+0.17%)`	⬆️
sdk/src/main/java/io/customer/sdk/queue/Queue.kt	`82.85% <100.00%> (+0.33%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

github-actions · 2023-04-04T09:52:10Z

Build available to test
Version: fix-android-12-opened-metrics-SNAPSHOT
Repository: https://s01.oss.sonatype.org/content/repositories/snapshots/

Shahroz16

found a small improvement 😄

messagingpush/src/main/java/io/customer/messagingpush/CustomerIOFirebaseMessagingService.kt

…IOFirebaseMessagingService.kt Co-authored-by: Shahroz Khan <[email protected]>

sdk/src/main/java/io/customer/sdk/CustomerIO.kt

levibostian · 2023-04-04T13:42:13Z

messagingpush/src/main/java/io/customer/messagingpush/CustomerIOFirebaseMessagingService.kt

+         * attempts to initialize with messaging push module so the module classes
+         * are initialized as well.
+         */
+        private fun getSDKInstanceOrNull(context: Context): CustomerIO? {


Why was this function added?

Whenever we initialize SDK instance using context from CustomerIOFirebaseMessagingService, we should ideally initialize push-messaging module as well. For all instances in service requesting SDK initialization, they can call this method instead of copying the code again.

From looking at the pull request diff, it looks like the SDK is currently calling CustomerIO.instanceOrNull(context). For example, in handleNewToken in this file, the code today is CustomerIO.instanceOrNull(context) but this PR is changing it to using this function.

What problem does CustomerIO.instanceOrNull(context, listOf(ModuleMessagingPushFCM())) solve instead of using what the code has today: CustomerIO.instanceOrNull(context)? I am assuming that the SDK works as intended today, so why change this code?

If there is no problem that this change fixes, I suggest that we remove this function and stick with the simplified code as it is today.

By initializing the module early, we can register activity lifecycle callback required by messaging push module whenever we initialize SDK using context from the service. So this is actually what fixes the problem for notifications received when app was in terminated state. Also, since this file is part of messaging push module, so requesting initialization of module with SDK from these files is actually the correct behavior in my opinion.

I think that explanation you gave is really good! Can you add this explanation as a comment to the getSDKInstanceOrNull function explaining what problem that function solves in it's implementation?

Sure, updated docs on getSDKInstanceOrNull.

Great, thanks.

I think we should still ship this PR if it works, but I did have an idea for a refactor.

The purpose of getSDKInstanceOrNull is to make sure that Activity lifecycle callbacks get registered with the SDK on initialization. The complexity exists because Activity lifecycle callbacks are in the messaging push module.

If we were to make the tracking module register empty Activity lifecycle callbacks during SDK initialization and then using some sort of communication mechanism (event bus, callback functions) we can make the messaging push module get code executed when the methods in the tracking module's Activity lifecycle callback get called.

If we were to make the SDK always register Activity lifecycle callbacks during SDK initialization, then that makes me think that this could replace something such as getSDKInstanceOrNull and make the SDK more stable?

Sure Levi. Since this apparently has worked well for us and fixes the bug, we'll ship this as soon as testing is complete. If you have ideas for refactoring, please feel free to create a ticket and suggest the changes in a separate PR.

levibostian · 2023-04-04T13:43:31Z

I modified the title of the PR to explain why this commit exists instead of what it is doing in the code.

levibostian · 2023-04-14T12:56:12Z

sdk/src/main/java/io/customer/sdk/CustomerIO.kt

+                // Cancelling queue timer as the new queue timer will take care of running queue tasks.
+                // If we do not cancel old timer, it results in multiple timers being run and accessing
+                // the same tasks.
+                it.diGraph.queue.cancelTimer()


Thanks for adding this along with a comment explaining why this is here.

In a call together, we talked about a long-term solution where CustomerIO.Builder.build() can be called 2+ times and the SDK config is the only thing that gets modified but the SDK instance or any dependencies do not get modified.

Do we have a ticket to capture that refactor? I want to make sure before we introduce code like this that we need to maintain in the meantime before we do the refactor.

Yeah, that's still the long term plan. But I haven't seen any issue for it yet, we probably need to create one for it 🤔

Created a ticket for this work https://github.com/customerio/issues/issues/9915

### [3.4.1](3.4.0...3.4.1) (2023-04-20) ### Bug Fixes * push opened metrics tracked on Android 12 ([#184](#184)) ([d2e52fa](d2e52fa))

mrehan27 added 4 commits March 31, 2023 14:30

fix: expose method for emitting pending lifecycle events

d46e56b

initialized push module from service

f11ea05

moved sdk initialization to method

f7351f0

Merge branch 'main' into fix/android-12-opened-metrics

c4b31ed

mrehan27 requested a review from a team April 4, 2023 09:47

mrehan27 self-assigned this Apr 4, 2023

Shahroz16 approved these changes Apr 4, 2023

View reviewed changes

Shahroz16 reviewed Apr 4, 2023

View reviewed changes

messagingpush/src/main/java/io/customer/messagingpush/CustomerIOFirebaseMessagingService.kt Outdated Show resolved Hide resolved

mrehan27 and others added 3 commits April 4, 2023 17:25

Update messagingpush/src/main/java/io/customer/messagingpush/Customer…

e8fc398

…IOFirebaseMessagingService.kt Co-authored-by: Shahroz Khan <[email protected]>

fixed formatting

54e00ca

fix: added cleanup of previously instance before creating new one (#185)

d52c25b

mrehan27 mentioned this pull request Apr 4, 2023

fix: opened metrics android 12 customerio/amiapp-reactnative#51

Closed

levibostian reviewed Apr 4, 2023

View reviewed changes

levibostian changed the title ~~fix: initialize push module when initializing sdk from context~~ fix: push opened metrics tracked on Android 12 Apr 4, 2023

Shahroz16 and others added 2 commits April 5, 2023 16:02

updated comment

f69a306

exposed method for requesting missing events

96bbf07

mrehan27 mentioned this pull request Apr 5, 2023

fix: push opened metrics tracked on Android 12 customerio/customerio-reactnative#119

Merged

levibostian approved these changes Apr 5, 2023

View reviewed changes

mrehan27 added 2 commits April 12, 2023 21:58

cancelled queue timer

ea20612

updated docs as suggested in pr

14e84a0

levibostian reviewed Apr 14, 2023

View reviewed changes

mrehan27 merged commit d2e52fa into main Apr 20, 2023

mrehan27 deleted the fix/android-12-opened-metrics branch April 20, 2023 18:19

github-actions bot pushed a commit that referenced this pull request Apr 20, 2023

chore: prepare for 3.4.1

f7fc984

### [3.4.1](3.4.0...3.4.1) (2023-04-20) ### Bug Fixes * push opened metrics tracked on Android 12 ([#184](#184)) ([d2e52fa](d2e52fa))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: push opened metrics tracked on Android 12 #184

fix: push opened metrics tracked on Android 12 #184

mrehan27 commented Apr 4, 2023 •

edited

Loading

github-actions bot commented Apr 4, 2023 •

edited

Loading

Examples:

codecov bot commented Apr 4, 2023 •

edited

Loading

github-actions bot commented Apr 4, 2023

Shahroz16 left a comment

levibostian Apr 4, 2023

mrehan27 Apr 4, 2023

levibostian Apr 5, 2023 •

edited

Loading

mrehan27 Apr 11, 2023

levibostian Apr 11, 2023

mrehan27 Apr 12, 2023

levibostian Apr 14, 2023

mrehan27 Apr 14, 2023

levibostian commented Apr 4, 2023

levibostian Apr 14, 2023

mrehan27 Apr 14, 2023

levibostian Apr 17, 2023

fix: push opened metrics tracked on Android 12 #184

fix: push opened metrics tracked on Android 12 #184

Conversation

mrehan27 commented Apr 4, 2023 • edited Loading

Changes

github-actions bot commented Apr 4, 2023 • edited Loading

Examples:

codecov bot commented Apr 4, 2023 • edited Loading

Codecov Report

github-actions bot commented Apr 4, 2023

Shahroz16 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levibostian Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levibostian commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrehan27 commented Apr 4, 2023 •

edited

Loading

github-actions bot commented Apr 4, 2023 •

edited

Loading

codecov bot commented Apr 4, 2023 •

edited

Loading

levibostian Apr 5, 2023 •

edited

Loading