-
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
Installation referrer tracking #8299
Conversation
…NCH first time app is run
…on with Google Play's InstallationReferrer API
…ined and tracked or not
…onnected it to the InstallationReferrerService
Marked [Not Ready for Review] as we need to target |
Ok! |
These two comments make me think we should remove the |
That is correct, yes, and I was thinking the same. Will remove in an upcoming commit then! 👍 |
Removed the |
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.
I left a few comments and questions about the code. It works as described and I was able to see the proper log entries while testing!
...rc/main/java/org/wordpress/android/util/analytics/receiver/InstallationReferrerReceiver.java
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.
Can we leave only one newline at the end of the file for code consistency?
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.
addressed in 7d4e91a
import org.wordpress.android.util.AppLog; | ||
import org.wordpress.android.util.analytics.service.InstallationReferrerServiceStarter; | ||
|
||
|
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.
There are two newlines between the import statements and class. Can we have only one for code consistency?
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.
addressed in 7d4e91a
import org.wordpress.android.util.AppLog; | ||
import org.wordpress.android.util.AppLog.T; | ||
|
||
|
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.
There are two newlines between the import statements and class. Can we have only one for code consistency?
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.
addressed in 93b4956
.../src/main/java/org/wordpress/android/util/analytics/service/InstallationReferrerService.java
Show resolved
Hide resolved
new InstallationReferrerServiceLogic(this, this); | ||
logic.performTask( | ||
new Bundle(params.getExtras()), | ||
params); |
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.
Can we move this new ...
and params);
code to the previous line for readability?
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.
addressed in 93b4956
@Override | ||
public int onStartCommand(Intent intent, int flags, int startId) { | ||
AppLog.i(T.UTILS, "installation referrer service > task: " + startId + " started"); | ||
mInstallationReferrerServiceLogic.performTask(intent.getExtras(), Integer.valueOf(startId)); |
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.
I don't think we need the Integer.valueOf()
boxing since startId
is an int
.
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.
good call! addressed in 695974b
// mark referrer as obtained so we don't try fetching it again | ||
// Citing here: | ||
// Caution: The install referrer information will be available for 90 days and won't | ||
// change unless the application is reinstalled. To avoid unecessary API calls in your |
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.
Can we change unecessary
to unnecessary
?
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.
Wow! Nice catch @theck13 :D it's copied from Google's documentation 😅
https://developer.android.com/google/play/installreferrer/library
fixed in 91c4c58
break; | ||
case InstallReferrerResponse.SERVICE_UNAVAILABLE: | ||
// Connection could not be established | ||
// same as above, this is a retriable error |
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.
Can we change this is a retriable error
to this error can be retried
?
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.
addressed in 0cd3efc
Closes #8298
This PR captures installation referrer information and sends it to Track.
What is being captured?
Here's a sample of the information as returned by Google's Install Referrer Library:
as gotten from this code:
In case we don't have access to it, we try to obtain it from the
com.android.vending.INSTALL_REFERRER
broadcast, and this is an example of it:(only the
install_referrer
string is obtained in this case).How?
This is done by means of 3 different entry points:
BroadcastReceiver
on the implicitIntent.ACTION_PACKAGE_FIRST_LAUNCH
(can't be registered on the Android Manifest as per since API 26). Such an intent should be broadcasted when the application is first launched, and is the recommended way as explained here.com.android.vending.INSTALL_REFERRER
intent, which is broadcasted each time a new application is installed from Google Play using the Play Store app.A note on capture / processing separation
As per my tests the intent
Intent.ACTION_PACKAGE_FIRST_LAUNCH
was not being called. I believe it is probably due to the need for the app to come from Google Play and be installed once, which of course we could only test by preparing a new alpha build and uploading to the store. This intent cannot be simulated fromadb
because it can only be issued by the System (otherwise aSecurityException
is thrown). For this, I decided to implement a mechanism that doesn't rely on this intent being listened to alone, and went on and implemented both aService
(< API 26) and aJobService
(API >= 26) that encapsulate the needed work to interact with the new Install Referrer library and send the info to Tracks. This has the advantage that detection (the entry points) and processing are well separated, and we can rely on the execution of this (or for the case, any other needed task) will be performed without affecting the normal execution of the app. With this then, the service is also triggered inWPMainActivity
with aAppPrefs
flag check to see if the referrer has already been obtained or not.That said, we could also remove the
Intent.ACTION_PACKAGE_FIRST_LAUNCH
BroadcastReceiver altogether if we feel it's not necessary (putting this option under review consideration as well), given the Service will run anyway upon launching (and first launch is a particular case of launch, which we'll be tracking withPrefs
anyway).Moreover, note that we try to get the referrer information as many times as needed until we get it.
Once we've gotten the information, it will not be queried anymore. On the contrary, relying on
Intent.ACTION_PACKAGE_FIRST_LAUNCH
alone and failing to obtain the needed referrer string would then be not ideal.Regarding Services
The
Service
/JobService
implementation is based on the previous experience - for reference check the related PRs in #7604To test:
Install Referrer API Library.
This information is obtained from the Installation Referrer API Library.
Install broadcast receiver
This should be tested both on API 26 >= and API < 26 devices. I tested on a Google Pixel with Android 8.1 (both tests worked) and a Samsung Galaxy S3 with Android 4.2.2 (only the second test worked as it has an older version of Google Play - it fails gracefully through the
case InstallReferrerResponse.FEATURE_NOT_SUPPORTED
switch case).