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

feat(analytics): add support for analytics_auto_collection_enabled in firebase.json #4730

Merged

Conversation

gewfy
Copy link
Contributor

@gewfy gewfy commented Dec 30, 2020

Description

This adds support for the documented but not implemented analytics_auto_collection_enabled in firabase.json.

I've aligned this with how messaging_auto_init_enabled is implemented.

Related issues

Fixes #4473

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

Test Plan

I've asserted that both platforms log that analytics collection gets disabled:

  • iOS: [Firebase/Analytics][I-ACS023013] Analytics collection disabled
  • Android: FA Event not sent since app measurement is disabled

🔥 Thanks for all the hard work you've put into these great and very valuable packages!

@CLAassistant
Copy link

CLAassistant commented Dec 30, 2020

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented Dec 30, 2020

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/7p43ydv0o
✅ Preview: https://react-native-f-git-fork-29ki-fix-analyticsautocollection-8e92db.invertase.vercel.app

@codecov
Copy link

codecov bot commented Dec 30, 2020

Codecov Report

Merging #4730 (9cb44e4) into master (36fcfd0) will increase coverage by 3.67%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4730      +/-   ##
==========================================
+ Coverage   85.43%   89.09%   +3.67%     
==========================================
  Files         109      109              
  Lines        3712     3712              
  Branches      347      347              
==========================================
+ Hits         3171     3307     +136     
+ Misses        475      363     -112     
+ Partials       66       42      -24     

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 perfect! I compared it to the messaging implementation and yes it looks exactly the same

Can you verify you checked the AndroidManifest.xml and the ios plist and it rendered into them correctly? Seems like it should but nothing beats knowing that you checked

@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar Workflow: Waiting for User Response Blocked waiting for user response. labels Dec 30, 2020
@gewfy
Copy link
Contributor Author

gewfy commented Dec 30, 2020

Of course!
For reference: here's the Firebase docs on how to disable it for iOS and Android

Disabled

{
  "react-native": {
    "analytics_auto_collection_enabled": false
  }
}

bundle-manifest.xml
image

Info.plist
image

Enabled

{
  "react-native": {
    "analytics_auto_collection_enabled": true
  }
}

bundle-manifest.xml
image

Info.plist
image

@mikehardy
Copy link
Collaborator

Fantastic, and it looked squeaky clean so I'm not surprised it worked :-)
CI went green everywhere but E2E android for some unrelated reason (the emulator failed to boot in the GitHub Actions VM), I just kicked it and as soon as it goes green I can merge

@mikehardy mikehardy removed Workflow: Waiting for User Response Blocked waiting for user response. Workflow: Pending Merge Waiting on CI or similar labels Dec 30, 2020
@mikehardy mikehardy merged commit 9a24ecd into invertase:master Dec 30, 2020
@gewfy
Copy link
Contributor Author

gewfy commented Dec 30, 2020

Thanks @mikehardy and Happy New Year! 🎉

@mikehardy
Copy link
Collaborator

@gewfy likewise! v10.4.0 should be out now with the changes - holler if it doesn't work :-)

androidIsForVivek pushed a commit to androidIsForVivek/react-native-firebase that referenced this pull request Aug 9, 2021
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.

[🐛] Analytics opt-in key in firebase.json not carried into iOS plist file
3 participants