[MOB-2255] Integrate new Merchant Library into mParticle #1

najmsheikh · 2018-08-15T19:32:35Z

Since we recently migrated merchant relevant functionality into the Button Merchant Library, our mParticle Kit needed to be updated to account for such changes.

Changes such as:

Removing the now outdated button internal package
Handling any pending post-install intents when the Kit is created.
Handling incoming third-party intents directly and not relying on Play Store (INSTALL_REFERRER) intents for attribution.
~~Using cart line item totals to create and track an Order only when a purchase is notified.~~
~~Discarding the current session and all persisted data on mParticle session conclusion.~~
Providing direct access to the Merchant Library API. (Edit: Attribution token only, for now)

najmsheikh · 2018-08-15T19:36:18Z

src/main/java/com/mparticle/kits/ButtonKit.java

-        if (referrerValue == null) {
-            return;
+    @Override
+    public List<ReportingMessage> logEvent(CommerceEvent event) {


Is there anything Button specific that we want to include in the ReportingMessage to mParticle?

najmsheikh · 2018-08-16T13:40:49Z

build.gradle

@@ -16,3 +22,7 @@ android {
        consumerProguardFiles 'consumer-proguard.pro'
    }
 }
+
+dependencies {
+    api 'com.usebutton.merchant:button-merchant:1.0.0'


Using api instead of implementation allows the consumer of this Kit also have access to the Button Merchant Library. Is that something we want or do we want to completely abstract away the Merchant Library from the consumer?

Jon6193

Just a few comments but code looks good

Jon6193 · 2018-08-16T15:27:42Z

src/main/java/com/mparticle/kits/ButtonKit.java

        }
+        ButtonMerchant.configure(ctx, applicationId);


I would recommend passing in the applicationContext instead of context to combat potential memory leaks

This context (and any via the getContext() method) is managed by the mParticle Core SDK itself. I'd lean on the assumption that Core is handling 'lifecycle' properly. And since this isn't an Activity, the application context isn't available. Unless you meant to extract it from the passed context itself?

Yeah extract it from the passed context itself getContext().getApplicationContext() This is definitely under the assumption that mParticle is handling the lifecycle properly but I think we should do our due diligence to not contribute to the issue if they are mis-managing the context they're passing to us

Jon6193 · 2018-08-16T17:42:53Z

src/main/java/com/mparticle/kits/ButtonKit.java

-        if (referrer.equals(mStorage.getReferrer())) return;
-        mStorage.setReferrer(referrer);
-    }
+        long orderTotal = Math.round(total * 100);


This would only work for USD correct? @ecgreb thoughts on this?

Most countries are at hundredths place. The most notable exceptions would be the Japanese Yen or the South Korean Won -- both of which are base currency units.

Jon6193 · 2018-08-16T17:45:17Z

src/main/java/com/mparticle/kits/ButtonKit.java

+            logDebug("Beginning to track order %s", event.getTransactionAttributes().getId());
+
+            Order order = prepareOrder(event);
+            ButtonMerchant.trackOrder(getContext(), order, new UserActivityListener() {


Potentially add a getApplicationContext() method to replace getContext() methods so we can ensure that we aren't causing any memory leaks

Jon6193 · 2018-08-16T17:52:07Z

build.gradle

@@ -16,3 +22,7 @@ android {
        consumerProguardFiles 'consumer-proguard.pro'
    }
 }
+
+dependencies {
+    api 'com.usebutton.merchant:button-merchant:1.0.0'


I think we should use implementation @ecgreb let me know what you think about this

Agreed. The consumer would need to do twice the setup (e.g. mParticle ButtonKit retrieves the app ID from the mParticle dashboard while the merchant library retrieves it directly) if plan on using the Merchant Library directly.

I do wonder if it'll use the ButtonMerchant instance configured from mParticle or not. Could be a race condition too.

@ecgreb Thoughts on this?

I don't think we would want partners calling configure() again on a separate instance but it could still be powerful to give them access to the underlying instance that is powering the integration. Is there any way to do this?

While we don't provide direct access to the instance, we have wrapper methods that allow access to key merchant library API.

On another note, would we want to use a wildcard versioning for only 1.x of the Merchant Library dependency? Since we only make API breaking changes on major versions. This'll allow the mParticle ButtonKit to get the latest of bug fixes for free without manually having to increment the version upon every Merchant Library update.

Jon6193 · 2018-08-22T16:51:11Z

src/main/java/com/mparticle/kits/ButtonKit.java

-        if (intent.getData() != null && intent.getData().getQueryParameter("btn_ref") != null) {
-            logDebug("Tracking incoming intent as %s", intent.toString());
-            ButtonMerchant.trackIncomingIntent(getContext(), intent);
+        if (intent.getExtras() != null && intent.hasExtra("referrer")) {


Is this line needed? The merchant library should handle all of this

The library does not have a check for this. Is there a scenario where the intent would be valid without a referrer?

Yeah this line should be removed completely and there should only be a null check on the intent

Jon6193 · 2018-08-22T16:53:45Z

src/main/java/com/mparticle/kits/ButtonKit.java

                new ButtonMerchant.AttributionTokenListener() {
                    @Override
-                    public void onAttributionTokenChanged(@NonNull String s) {
+                    public void onAttributionTokenChanged(@NonNull final String s) {


Is final still needed?

src/main/java/com/mparticle/kits/ButtonKit.java

ecgreb · 2018-08-23T13:53:45Z

@najmsheikh I updated master to include the latest changes from upstream so this branch can now be rebased.

ecgreb · 2018-08-23T13:59:07Z

src/main/java/com/mparticle/kits/ButtonKit.java

        }
+        if (Build.VERSION.SDK_INT < Build.VERSION_CODES.ICE_CREAM_SANDWICH_MR1) {
+            throw new IllegalArgumentException(


We should silently disable and log an error rather that throwing an exception here since this could crash the merchant app.

That is mParticle's prescribed way of notifying the core SDK that our Kit cannot be initialized. I would have thrown a different exception but the parent class only seems to "support" this one.

src/main/java/com/mparticle/kits/ButtonKit.java

ecgreb · 2018-08-23T14:01:43Z

src/main/java/com/mparticle/kits/ButtonKit.java

+                        } else if (throwable != null) {
+                            logError("Error checking post install intent", throwable);
+                        } else {
+                            logError("Neither an Intent nor a Throwable is available post-install");


This is not an error state but expected if the request successfully completes and no post install deep link was found.

ecgreb · 2018-08-23T14:05:13Z

src/main/java/com/mparticle/kits/ButtonKit.java

+    public void setInstallReferrer(Intent intent) {
+        if (intent != null) {
+            logDebug("Tracking incoming intent from %s", intent.toString());
+            ButtonMerchant.trackIncomingIntent(getApplicationContext(), intent);


Install referrer intent comes from the play store and does not contain the attribution token. This functionality has been removed and no longer needs to be tracked.

…Library

ecgreb

Nice work @najmsheikh :) I added a few comments below but overall implementation looks good.

I realize there were no existing tests for this project, however it seems like we would be doing ourselves a disservice if we didn't add at least basic unit test coverage for the logic in the ButtonKit class.

ecgreb · 2018-08-24T22:12:49Z

build.gradle

@@ -16,3 +22,7 @@ android {
        consumerProguardFiles 'consumer-proguard.pro'
    }
 }
+
+dependencies {
+    api 'com.usebutton.merchant:button-merchant:1.0.0'


I don't think we would want partners calling configure() again on a separate instance but it could still be powerful to give them access to the underlying instance that is powering the integration. Is there any way to do this?

src/main/java/com/mparticle/kits/ButtonKit.java

ecgreb · 2018-08-24T22:32:14Z

src/main/java/com/mparticle/kits/ButtonKit.java

+
+    private static void logError(String message) {
+        if (MParticle.getInstance().getEnvironment() == MParticle.Environment.Development) {
+            Logger.error("ButtonKit: " + message);


While it makes sense to only log debug messages in Development, shouldn't we also log errors in production?

ecgreb · 2018-08-24T22:33:52Z

src/main/java/com/mparticle/kits/ButtonKit.java


    @Override
-    public List<ReportingMessage> onActivityDestroyed(final Activity activity) { return null; }
+    public List<ReportingMessage> onActivityStarted(Activity activity) {
+        return null;


We should call trackIncomingIntent() in both onActivityCreated() and onActivityStarted() to account for the case where the merchant app is open but in the background and a new deep link is sent from the publisher app.

This commit attempts to maintain feature parity with previous ButtonKit version by interacting with the `KitManager` in the same way on attribution token changes and on post-install deep links. * Attribution token changes invoke `setIntegrationAttributes()` on kit * Post-install deep links no longer start the activity * Post-install deep links sets `AttributionResult` on kit manager * No post-install deep link sets `AttributionError` on kit manager * Debug logging check removed since handled by `Logger.setMinLogLevel()` * Adds tests for kit manager interactions

najmsheikh

I'm really glad we got testing sorted out! Just some questions/comments but looks good. We should now update the O'Christmas Tree app and run through the merchant library tests

najmsheikh · 2018-09-12T00:28:14Z

build.gradle

@@ -9,18 +9,16 @@ buildscript {
    }
 }

-allprojects {


The project does not compile for me without the google repo

najmsheikh · 2018-09-12T00:43:55Z

src/main/java/com/mparticle/kits/ButtonKit.java

+        PackageManager pm = applicationContext.getPackageManager();
+        if (intent != null && intent.resolveActivity(pm) != null) {
+            logDebug("Handling post-install intent for %s", intent.toString());
+            AttributionResult result = new AttributionResult()


This does seem to be the intended way of forwarding intent data. mParticle has a feature branch that they committed to (but didn't merge) about a month ago. Looking at that, do you think we should add parameters that house the referrer?

Another implementation

Nice find on that commit. I think this is something we can address when we submit the PR to mParticle. For now, I'm going to keep the kit communication as it was.

najmsheikh · 2018-09-12T00:45:35Z

src/main/java/com/mparticle/kits/ButtonKit.java

+    public void onAttributionTokenChanged(@NonNull String token) {
+        final Map<String, String> attributes = getIntegrationAttributes();
+        attributes.put(ATTRIBUTE_REFERRER, token);
+        setIntegrationAttributes(attributes);


I get that this in parity with the old code but where in the dashboard do these attributes reside? Is it per user, per application, per session? The javadoc wasn't quite clear on this.

najmsheikh · 2018-09-12T00:46:45Z

src/test/java/com/mparticle/kits/ButtonKitTests.java

        when(context.getApplicationContext()).thenReturn(context);
-        when(mParticleWrapper.isDebuggingEnvironment()).thenReturn(true);
+        MParticle.setInstance(new TestMParticle());


najmsheikh · 2018-09-12T00:53:20Z

src/test/java/com/mparticle/kits/ButtonKitTests.java

+        @NonNull
+        @Override
+        public IdentityApi Identity() {


I guess this is why we should try not to have static methods/singletons in the constructor 😬

* Declare minSdkVersion 15 * Re-add google repository for merchant library transitive dependencies * Switch from `implementation` to `api` for merchant lib dependency * Update "error" message when no post-install deep link is found * Report error when post-install deep link check returns throwable * Remove unused imports

ecgreb · 2018-09-12T17:19:41Z

build.gradle

@@ -9,18 +9,16 @@ buildscript {
    }
 }

-allprojects {


ecgreb · 2018-09-12T17:27:00Z

src/main/java/com/mparticle/kits/ButtonKit.java

+        PackageManager pm = applicationContext.getPackageManager();
+        if (intent != null && intent.resolveActivity(pm) != null) {
+            logDebug("Handling post-install intent for %s", intent.toString());
+            AttributionResult result = new AttributionResult()


Nice find on that commit. I think this is something we can address when we submit the PR to mParticle. For now, I'm going to keep the kit communication as it was.

ecgreb · 2018-09-12T17:27:21Z

src/main/java/com/mparticle/kits/ButtonKit.java

+    public void onAttributionTokenChanged(@NonNull String token) {
+        final Map<String, String> attributes = getIntegrationAttributes();
+        attributes.put(ATTRIBUTE_REFERRER, token);
+        setIntegrationAttributes(attributes);


Jon6193

LGTM

najmsheikh self-assigned this Aug 15, 2018

najmsheikh requested review from ecgreb, wessmith and Jon6193 August 15, 2018 19:32

najmsheikh commented Aug 15, 2018

View reviewed changes

najmsheikh commented Aug 16, 2018

View reviewed changes

Jon6193 reviewed Aug 16, 2018

View reviewed changes

Jon6193 reviewed Aug 22, 2018

View reviewed changes

ecgreb reviewed Aug 23, 2018

View reviewed changes

Remove old merchant functionality and migrate to new Button Merchant …

f2d86a0

…Library

najmsheikh force-pushed the najm/MOB-2255-integrate-merchant-library-mparticle branch from 197a2fe to f2d86a0 Compare August 23, 2018 21:03

ecgreb reviewed Aug 24, 2018

View reviewed changes

najmsheikh and others added 5 commits August 30, 2018 15:25

Added info on how to build locally

a745200

Removed order tracking + extraneous functionality

de14387

Updated README to use 'implementation' for dependency import

f3ecb9c

Added tests + reverted to static versioning for the merchant library

adbeea0

najmsheikh commented Sep 12, 2018

View reviewed changes

ecgreb approved these changes Sep 12, 2018

View reviewed changes

Jon6193 approved these changes Sep 12, 2018

View reviewed changes

ecgreb merged commit f45f721 into master Sep 12, 2018

ecgreb deleted the najm/MOB-2255-integrate-merchant-library-mparticle branch September 12, 2018 18:46

[MOB-2255] Integrate new Merchant Library into mParticle #1

[MOB-2255] Integrate new Merchant Library into mParticle #1

Conversation

najmsheikh commented Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jon6193 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

najmsheikh Aug 16, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecgreb commented Aug 23, 2018

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

ecgreb left a 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

najmsheikh left a 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

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

Choose a reason for hiding this comment

Jon6193 left a comment

Choose a reason for hiding this comment

najmsheikh commented Aug 15, 2018 •

edited

Loading

najmsheikh Aug 16, 2018 •

edited

Loading