react-native-firebase icon indicating copy to clipboard operation
react-native-firebase copied to clipboard

feat(analytics): add setConsent implementation

Open Summys opened this issue 1 year ago • 2 comments

Description

A well put description can be found at https://github.com/invertase/react-native-firebase/discussions/6636

The motivation for this change is coming from Google where we have to ask for end user consent by 6th March.

Relevant URLs:

Related issues

Release Summary

Checklist

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

Test Plan


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

Summys avatar Feb 16 '24 09:02 Summys

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 19, 2024 4:40pm

vercel[bot] avatar Feb 16 '24 09:02 vercel[bot]

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 16 '24 09:02 CLAassistant

Added e2e tests and fixed linting errors. Thanks for your quick review @mikehardy

Summys avatar Feb 19 '24 10:02 Summys

Great stuff, thanks for the quick work both of you :) Will be testing the patch tomorrow.

nixolas1 avatar Feb 19 '24 14:02 nixolas1

Something isn't quite right in the gradle changes but they looked good on visual inspection so it can't be far off.

Hold off on using the patch set of course, but I'll fix up whatever little thing isn't right and get this in

mikehardy avatar Feb 19 '24 16:02 mikehardy

Something isn't quite right in the gradle changes but they looked good on visual inspection so it can't be far off.

Hold off on using the patch set of course, but I'll fix up whatever little thing isn't right and get this in

the variables I added weren't defined in gradle

Summys avatar Feb 19 '24 16:02 Summys

Codecov Report

Merging #7629 (eab6574) into main (2ff569d) will decrease coverage by 19.97%. The diff coverage is 28.58%.

Additional details and impacted files
@@              Coverage Diff              @@
##               main    #7629       +/-   ##
=============================================
- Coverage     53.22%   33.24%   -19.97%     
=============================================
  Files           251      251               
  Lines         12426    12460       +34     
  Branches       1938     1944        +6     
=============================================
- Hits           6612     4141     -2471     
- Misses         5466     8230     +2764     
+ Partials        348       89      -259     

codecov[bot] avatar Feb 19 '24 17:02 codecov[bot]

One of the android APIs (23) wasn't happy but one passed - that's definitely sufficient for the feature as the e2e tests are a bit flaky

mikehardy avatar Feb 19 '24 17:02 mikehardy

For those looking for a patch set - this was the workflow run with patch-set from the merge to main: https://github.com/invertase/react-native-firebase/actions/runs/7963378973

mikehardy avatar Feb 19 '24 22:02 mikehardy

How can I use this commit before the new release?

RamProg avatar Feb 21 '24 11:02 RamProg

Download the patches folder https://github.com/invertase/react-native-firebase/actions/runs/7963378973/artifacts/1257279870 and put it in your root, make sure youre on 18.8.0, and install patch-package. When you run yarn install it will patch the files with the new changes.

nixolas1 avatar Feb 21 '24 11:02 nixolas1

Yes, thank you, I was able to do that. Do I need the whole patches folder or just the analytics patch (I'm only interested in this particular change).

RamProg avatar Feb 21 '24 11:02 RamProg

All the packages you have installed. It'l give you a patch error if you have patches for non-installed packages.

nixolas1 avatar Feb 21 '24 11:02 nixolas1

I have them all installed, but only care about changes in Analytics, I still need to apply them all right?

RamProg avatar Feb 21 '24 11:02 RamProg