-
Notifications
You must be signed in to change notification settings - Fork 157
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
[CORE-1951] Add initial_referrer for every session #918
Conversation
- Added initial_referrer-> EXTRA_REFERRER to every OPEN/INSTALL session request.
@bklastaitis-branch @jdee @echo-branch Can you take a pass on this PR if you guys have time too? Thanks |
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.
Looks good, but could use a little clean-up.
Branch-SDK/src/main/java/io/branch/referral/ServerRequestInitSession.java
Outdated
Show resolved
Hide resolved
Branch-SDK/src/main/java/io/branch/referral/ServerRequestInitSession.java
Outdated
Show resolved
Hide resolved
Branch-SDK/src/main/java/io/branch/referral/ServerRequestInitSession.java
Outdated
Show resolved
Hide resolved
Branch-SDK/src/main/java/io/branch/referral/ServerRequestInitSession.java
Outdated
Show resolved
Hide resolved
- Removed extra space and unused imports - Added null check and sdk version check
@@ -2873,6 +2874,11 @@ public void init() { | |||
|
|||
Activity activity = branch.getCurrentActivity(); | |||
Intent intent = activity != null ? activity.getIntent() : null; | |||
|
|||
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP_MR1 && activity.getReferrer() != null) { |
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.
@msiddiq-branch I'm assuming Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP_MR1
is OK by product? Or if not, is there any other way we can achieve the same functionality on the lower API levels?
Also, nitpick: let's use !TextUtils.isEmpty(activity.getReferrer())
instead of activity.getReferrer() != null
, that way we'll capture empty strings as well as nulls.
Finally, this reminded me of a recent inquiry about the intent keys that Branch uses, this would have potentially gone under the radar because the intent keys we use to extract data via getReferrer()
are not listed among Defines.IntentKeys
.. so maybe we could add a comment ssomewhere e inside Defines.IntentKeys
that says:
// The below intent keys are also used to extract data from the intent (via Activity.getReferrer())
// public static final String EXTRA_REFERRER = "android.intent.extra.REFERRER";
// public static final String EXTRA_REFERRER_NAME = "android.intent.extra.REFERRER_NAME";
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.
@bklastaitis-branch Currently since our SDK supports minimum SDK as 21 I believe we need to pass this value from API 21. I will figure out a method to fetch this value in the lower API version. Currently, getReferrer is supported from Api 22. cc @aaaronlopez
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.
@bklastaitis-branch I have replaced Activity.getReferrer
with Activitycompat.getReferrer
but still we are able to get referrer info only from API version 22 ideally this should have worked. I have been testing this on an emulator I'm not sure if that could be the reason. As per the code from google the implementation for version lower than 22 would something as mentioned here and Activitycompat.getReferrer has the same implementation.
Also while debugging in the lower version I see that intent does not have extra info being passed when app is launched from link/launcher which looks like we are not getting referrer info from API version lower than 22
@aaaronlopez Are we okay to have this supported from API 22?
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.
min api support being LOLLIPOP_MR1 is fine by me for this API 👍
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.
Thanks for confirming Aaron.
Branch-SDK/src/main/java/io/branch/referral/ServerRequestInitSession.java
Outdated
Show resolved
Hide resolved
- Replaced Activity.referrer with ActivityCompat.referrer - Used TextUtils for null check
- Added comment about usage of intent keys EXTRA_REFERRER and EXTRA_REFERRER_NAME
👍 @bklastaitis-branch if you have time for one final pass, please do! |
changes implemented
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.
LGTM!
Reference
CORE-1951 -- Collect initial_referrer from Android SDK.
Description
Added initial_referrer-> EXTRA_REFERRER to every OPEN/INSTALL session request.
Testing Instructions
Step 1: Click on branch link from sms app
Step 2: INSTALL/OPEN the app
Step 3: Under logcat observe that v1/install or v1/open has new param called initial_referrer with the package name of referring app which is SMS app here(android-app://com.google.android.apps.messaging)
Step 4: Without click on any link try opening the app in which case EXTRA_REFERRER will be null and will have default value bnc_no_value
Risk Assessment [
LOW
]Reviewer Checklist (To be checked off by the reviewer only)