Skip to content
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

fix(analytics): Use correct add_to_cart event name #2882

Merged
merged 1 commit into from
Nov 17, 2019

Conversation

andrzejbe
Copy link
Contributor

triggering analytics().logAddToCart firebase event resulted in sending "addPaymentInfo" event to firebase analytics

Summary

After inspecting source code its clear this was almost a "typo" as "add_payment_info" is defined few lines above and it was probably a simple omission.

This is a simple bug - but crucial for anyone who tries to use more advanced ecommerce pre-defined firebase events (like adding/removing from cart etc. ecommerce funnels etc.).

Tested on Android - with my fix I can now send correct add_to_cart events which are then successfully registered in Firebase Analytics

Checklist

  • Supports Android
  • Supports iOS
  • e2e tests added or updated in packages/**/e2e
  • Flow types updated
  • Typescript types updated

Test Plan

Release Plan


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@CLAassistant
Copy link

CLAassistant commented Nov 15, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Andrzej Bednarczyk seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch! I checked the file for any other thinkos and it looks like this was it?

@andrzejbe
Copy link
Contributor Author

yup that was the only one - I don't use all of them, but quite a few and all the others works just fine

@codecov
Copy link

codecov bot commented Nov 15, 2019

Codecov Report

Merging #2882 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2882   +/-   ##
=======================================
  Coverage   88.69%   88.69%           
=======================================
  Files         111      111           
  Lines        3437     3437           
=======================================
  Hits         3048     3048           
  Misses        389      389

Copy link
Member

@Salakar Salakar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this looks like code to me 😅 approved

@Salakar Salakar added this to the v6.1.0 milestone Nov 17, 2019
@Salakar
Copy link
Member

Salakar commented Nov 17, 2019

Woops, thanks for sending this, will be in 6.1

@Salakar Salakar changed the title fix add_to_cart event name fix(analytics): Use correct add_to_cart event name Nov 17, 2019
@Salakar Salakar merged commit 2369c62 into invertase:master Nov 17, 2019
@Salakar
Copy link
Member

Salakar commented Nov 17, 2019

This is now live in v6.0.4. Thanks

andersondanilo pushed a commit to vixtech/react-native-firebase that referenced this pull request Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants