fix(analytics): added missing quantity parameter to the Item structure #4536

Danchoys · 2020-11-11T13:44:45Z

Description

This pull request addresses the problem of not being able to pass the quantity parameter when trying to log any of these events:

add_to_cart
add_to_wishlist
remove_from_cart
view_cart
refund

Motivation

According to the documentation for the aforementioned events one must or can pass the quantity parameter, but currently the library will reject such parameters, since quantity is not mentioned in the Item struct.

Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
- Yes
My change supports the following platforms;
- Android
- iOS
My change includes tests;
- e2e tests added or updated in packages/\*\*/e2e
- jest tests added or updated in packages/\*\*/__tests__
I have updated TypeScript types that are affected by my change.
This is a breaking change;
- Yes
- No

🔥

CLAassistant · 2020-11-11T13:44:50Z

All committers have signed the CLA.

vercel · 2020-11-11T13:44:51Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/6y1leugwf
✅ Preview: https://react-native-firebase-git-master.invertase.vercel.app

codecov · 2020-11-11T13:46:47Z

Codecov Report

Merging #4536 (a93b7e7) into master (fc8c4c0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4536   +/-   ##
=======================================
  Coverage   65.56%   65.56%           
=======================================
  Files         109      109           
  Lines        3699     3699           
  Branches      276      276           
=======================================
  Hits         2425     2425           
  Misses       1148     1148           
  Partials      126      126

mikehardy · 2020-11-11T14:20:16Z

Clearly a potential parameter in the item object in javascript https://firebase.google.com/docs/analytics/measure-ecommerce#add_remove_product

It appears we are also missing item price (!), item coupon code, and quite a few others, here is the type definition from firebase-js-sdk https://github.com/firebase/firebase-js-sdk/blob/47b182b91879d6e4bcce07609c517938d9354c9f/packages/analytics-types/index.d.ts#L197-L228 and the guide https://firebase.google.com/docs/reference/js/firebase.analytics.Item

A question for verification @Danchoys - I assume you have tested this change, when you do so does the quantity param actually travel through correctly and show up on the analytics dashboard? You can hack right in to node_modules to try it if you have not yet (use patch-package to make the patch manageable if it works and you want to keep it...)

The reason I ask is because the Java reference docs match the item-specific params with what we have, https://firebase.google.com/docs/reference/android/com/google/firebase/analytics/FirebaseAnalytics.Param#QUANTITY - quantity does not seem item-specific. Sometimes there are things in the javascript APIs that don't map to Java/iOS so we end up with implementation differences. This may be one of them, and make this change have no effect

iOS mirrors Java in that it also does not have quantity so I'm iffy on this. It looks like the only item-specific things we are missing are "list id" and "list name"

🤔

Danchoys · 2020-11-11T16:36:37Z

@mikehardy In fact I did test it and I thought that it worked, but decided to double check it and got something that I didn't expect to see at all. First of all the quantity does show up:

But as you can see it is zero. Then I played with the native code and actually got it to work by using the following trick:

        if ([name isEqualToString:@"add_to_cart"]) {
            NSMutableDictionary *item = [params[@"items"][0] mutableCopy];
            item[@"quantity"] = [NSNumber numberWithInteger:[item[@"quantity"] integerValue]]; // Magic!
            NSDictionary *dict = @{ @"items" : @[ [item copy] ] };
            [FIRAnalytics logEventWithName:name parameters:dict];
        } else {
            [FIRAnalytics logEventWithName:name parameters:params];
        }

Using NSNumber literal (e.g. @5) also does the trick. It seems that firebase doesn't like the number that arrives from javascript...

Here is the corresponding JS code:

firebase
      .analytics()
      .logAddToCart({
        items: [
          {
            item_name: 'Test item',
            item_id: '123123123',
            item_category: 'test_category',
            quantity: 5,
          },
        ],
      })
      .catch(() => {});

😩

mikehardy · 2020-11-11T17:23:19Z

Wow! This will require some deeper thought...

you were able to get it to go through and show up: that's great! It means that the other item parameters from firebase-js-sdk that are not specifically called out in firebase-ios-sdk and firebase-android-sdk as being item-specific may go through too!

That implies that we are potentially missing quite a few useful parameters...

the code there at the native layer needs corresponding work on the android java side, and it should probably be keyed more to what exists in the item object vs the event in use? Or is there a mapping of what item data can be sent depending on what event is logged? If some item data (e.g., quantity) may only be sent on some event types (e.g., add to card, refund etc) then we should do it similar to how you did it, but with only the right things put in the event depending on type

I'm willing to help with this - getting the code shaped up and merged, offering pointers - but I won't have time to do the research on item data element validity vs event type, or the implementation testing. There is a need to change the documentation and the e2e tests to support the new parameters and also they should be validated (I think quantity is 1-200 inclusive)

Danchoys · 2020-11-12T00:17:05Z

Yeah, there may really be other fields missing as well. I just bumped into the quantity specifically since I was migrating my code to v6. Shall I extend this PR with the other fields or shall it be done separately?
That was just an attempt to get it to work. Of course checking for the event name like this is not a production-ready solution. I will further investigate why the numbers are acting the way they do... I’ve also tested Android and it seems to be plagued by the same problem.

Regarding the tests and docs - sure, will update these too.

mikehardy · 2020-11-12T01:35:34Z

If you've got the time for the other ones sure - that would be fantastic! I understand that as a sort of "do it because you need it" contribution you may not be motivated but the more the merrier, from my maintainer perspective, given some proof they are working of course. This strikes me as kind of a documentation miss on the firebase part as well, that's something I could log with them upstream once we're sure that the other things shown as product level in the main analytics docs actually work via firebase if attached to items

packages/analytics/ios/RNFBAnalytics/RNFBAnalyticsModule.m

…ms array

Danchoys · 2020-11-16T15:26:03Z

I have updated the PR with some refactoring on iOS plus what I think would be a solution for Android, but since I'm not really a Java developer, would highly appreciate if someone checks the code. I have tested both platforms and it seems to work for me. I have also checked if there are any specific restrictions on the range and was able to successfully report quantity of 1000, so I bet 200 limit does not apply here.

Finally I checked the existing unit and e2e tests and frankly speaking I'm not sure what I could add there... Firebase itself wasn't complaining about the number having the floating point, it was simply appearing as 0 afterwards. I guess mocking out the whole native FirebaseAnalytics module and checking the incoming values would do the trick, but I feel it is going way too far.

…e doesn't crash the app

Danchoys · 2020-11-16T15:29:03Z

Update: I decided to add the quantity param to one of the e2e tests so that the new code path is executed, which will make sure that at least it doesn't crash anything.

mikehardy · 2020-11-16T15:39:04Z

Thanks for adding a little bit of test cover - it is always a tension on how much to test etc but at least sending new values through to make sure the code is actually exercised with no crashing is good. Most of the jest testing we do is just input validation / throw on invalid input. I've just been all over that stuff with #4552 - if there's anything to add that's trivial I'll add it as a review comment, or if it's non-trivial I can post up a follow-on PR

I'm weak at Obj-C but Android was my whole world prior to react-native so I'm comfortable giving those a once over

Goes without saying - thanks for working through this, we've advanced the general knowledge on the subject and you've implemented something really important for the specific events.

Danchoys · 2020-11-16T15:40:15Z

packages/analytics/ios/RNFBAnalytics/RNFBAnalyticsModule.m

+      newParams[kFIRParameterItems] = [newItems copy];
+    }
+    return [newParams copy];
+  }


Alternative solution would be to transform the incoming dictionary into a JSON string, search and replace all numbers with zeroes after the dot with whole numbers and transform it back into an object, but obviously this may have some unexpected impact on other things...

Danchoys · 2020-11-16T16:36:34Z

I'm also happy to contribute :) Regarding the tests - I can see that on Android one test is failing in some irrelevant module (messaging) due to timeout. Doubt though that it is related to the changes from this PR.. and also the tests do pass locally.

mikehardy · 2020-11-16T19:09:40Z

Yep - I just switched that test in #4552 so that it no longer runs in CI but does run locally (setBackgroundMessageHandler) as it's flakey due to performance reasons of nested virtualization in CI, no worries about that one

mikehardy

LGTM - working through the pending items stack and heading for a release shortly - thanks for this @Danchoys

Add the missing quantity parameter to the Item structure

5668d0d

vercel bot deployed to Preview November 11, 2020 13:44 View deployment

mikehardy added the Workflow: Waiting for User Response Blocked waiting for user response. label Nov 11, 2020

Workaround attempt on iOS

62c77c7

vercel bot deployed to Preview November 13, 2020 14:33 View deployment

mikehardy reviewed Nov 13, 2020

View reviewed changes

packages/analytics/ios/RNFBAnalytics/RNFBAnalyticsModule.m Outdated Show resolved Hide resolved

Danchoys added 2 commits November 13, 2020 19:09

Convert the quantity to integer if present in the elements of the ite…

8383f78

…ms array

Refactor iOS

fc16c13

vercel bot deployed to Preview November 13, 2020 16:19 View deployment

Add quantity to an end-to-end test just to make sure that the new cod…

c4a4757

…e doesn't crash the app

vercel bot deployed to Preview November 16, 2020 15:28 View deployment

mikehardy added Workflow: Needs Review Pending feedback or review from a maintainer. and removed Workflow: Waiting for User Response Blocked waiting for user response. labels Nov 16, 2020

Danchoys commented Nov 16, 2020

View reviewed changes

mikehardy approved these changes Nov 23, 2020

View reviewed changes

mikehardy merged commit f9935e7 into invertase:master Nov 23, 2020

mikehardy removed the Workflow: Needs Review Pending feedback or review from a maintainer. label Nov 23, 2020

mikehardy mentioned this pull request Dec 3, 2020

How to pass item value and quantity to analytics? #4578

Closed

artemkhalygov mentioned this pull request Apr 28, 2021

Can't send Item price to analytics #5227

Closed

mikehardy mentioned this pull request Sep 1, 2021

[🐛] Firebase Analytics Bug - logging event with number param value showing as (not set) #5666

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(analytics): added missing quantity parameter to the Item structure #4536

fix(analytics): added missing quantity parameter to the Item structure #4536

Danchoys commented Nov 11, 2020 •

edited

Loading

CLAassistant commented Nov 11, 2020 •

edited

Loading

vercel bot commented Nov 11, 2020 •

edited

Loading

codecov bot commented Nov 11, 2020 •

edited

Loading

mikehardy commented Nov 11, 2020

Danchoys commented Nov 11, 2020

mikehardy commented Nov 11, 2020

Danchoys commented Nov 12, 2020

mikehardy commented Nov 12, 2020

Danchoys commented Nov 16, 2020

Danchoys commented Nov 16, 2020

mikehardy commented Nov 16, 2020

Danchoys Nov 16, 2020

Danchoys commented Nov 16, 2020

mikehardy commented Nov 16, 2020

mikehardy left a comment

fix(analytics): added missing quantity parameter to the Item structure #4536

fix(analytics): added missing quantity parameter to the Item structure #4536

Conversation

Danchoys commented Nov 11, 2020 • edited Loading

Description

Motivation

Checklist

CLAassistant commented Nov 11, 2020 • edited Loading

vercel bot commented Nov 11, 2020 • edited Loading

codecov bot commented Nov 11, 2020 • edited Loading

Codecov Report

mikehardy commented Nov 11, 2020

Danchoys commented Nov 11, 2020

mikehardy commented Nov 11, 2020

Danchoys commented Nov 12, 2020

mikehardy commented Nov 12, 2020

Danchoys commented Nov 16, 2020

Danchoys commented Nov 16, 2020

mikehardy commented Nov 16, 2020

Danchoys Nov 16, 2020

Choose a reason for hiding this comment

Danchoys commented Nov 16, 2020

mikehardy commented Nov 16, 2020

mikehardy left a comment

Choose a reason for hiding this comment

Danchoys commented Nov 11, 2020 •

edited

Loading

CLAassistant commented Nov 11, 2020 •

edited

Loading

vercel bot commented Nov 11, 2020 •

edited

Loading

codecov bot commented Nov 11, 2020 •

edited

Loading