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): added missing price parameter to the Item structure #5232

Merged
merged 3 commits into from
Apr 29, 2021

Conversation

artemkhalygov
Copy link
Contributor

Description

This pull request addresses the problem of not being able to pass the price parameter when trying to log view_item_list

But as described in the official documentation it must be possible.

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
Copy link

CLAassistant commented Apr 29, 2021

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented Apr 29, 2021

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/EBZBqBk9UrxMRBzwfxFoeY7Tdzgy
✅ Preview: https://react-native-firebase-git-fork-artemkhalygov-master-invertase.vercel.app

@mikehardy
Copy link
Collaborator

Thanks for this! I think this will be a popular addition! Have you tested it to make sure the analytics dashboard looks the way you expect from both platforms? There were number formatting issues with quantity if I recall

@artemkhalygov
Copy link
Contributor Author

artemkhalygov commented Apr 29, 2021

@mikehardy I have tested it, and I confirm an issue with the price on Firebase Dashboard (It's added six zeros at the end to any price). But as described here #4578 (comment) it is not an issue with React Native Firebase

@mikehardy mikehardy added platform: javascript plugin: analytics Google Analytics for Firebase type: enhancement Implements a new Feature Workflow: Needs Review Pending feedback or review from a maintainer. Workflow: Waiting for User Response Blocked waiting for user response. labels Apr 29, 2021
mikehardy
mikehardy previously approved these changes Apr 29, 2021
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.

This looks great to me - not a huge code fix but since you have the need and are already set up to test it, it means we can get this done today vs...who knows when otherwise :-). I added a note about price display and link to the discussion in order to help future developers, and when CI goes green I'll merge this and release it

Thanks again!

@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar and removed Workflow: Needs Review Pending feedback or review from a maintainer. Workflow: Waiting for User Response Blocked waiting for user response. labels Apr 29, 2021
@codecov
Copy link

codecov bot commented Apr 29, 2021

Codecov Report

Merging #5232 (5c71228) into master (99f8d2c) will not change coverage.
The diff coverage is n/a.

❗ Current head 5c71228 differs from pull request most recent head 7f3c1f9. Consider uploading reports for the commit 7f3c1f9 to get more accurate results

@@           Coverage Diff           @@
##           master    #5232   +/-   ##
=======================================
  Coverage   88.86%   88.86%           
=======================================
  Files         109      109           
  Lines        3743     3743           
  Branches      360      360           
=======================================
  Hits         3326     3326           
  Misses        370      370           
  Partials       47       47           

@mikehardy
Copy link
Collaborator

Test failure was a known flaky test and unrelated to this PR, but unfortunately the analytics E2E tests were after the flaky test, so re-running and waiting...

@mikehardy mikehardy merged commit b972cb6 into invertase:master Apr 29, 2021
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Apr 29, 2021
@mikehardy
Copy link
Collaborator

Release 11.4.1 just went out with the change related to this among other fixes + features. Enjoy! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: javascript plugin: analytics Google Analytics for Firebase type: enhancement Implements a new Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants