feat(analytics): add setConsent implementation #7629
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
There was a problem hiding this comment.
This looks really thorough, thank you
I left two comments which are both (in my opinion at least) trivial to fix - duplicating the test you added but using the modular APIs in the modular section of the e2e test file and updating the method docs in one spot to match the update you made in both
Going to approve the CI runs now as well, they always seem to turn up some trivial spellcheck or lint issue as well
...but in general this looks 99.9% ready to go
mikehardy
added
Workflow: Waiting for User Response
|
Added e2e tests and fixed linting errors. Thanks for your quick review @mikehardy |
There was a problem hiding this comment.
This looks really great. Thank you!
I'll start shepherding it through CI
I'm interactive on the repo right now so I'm fine fixing any lint errors or spelling things etc that pop up (there always seems to be something, and always really easy to fix)
Should merge today and I'll queue it for release shortly
The patch set should be useful already either way if you follow the details link and then look at the summary on the patch-package action
mikehardy
added
Workflow: Pending Merge
|
Great stuff, thanks for the quick work both of you :) |
|
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 |
Codecov Report
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 |
|
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 |
|
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 |
|
How can I use this commit before the new release? |
|
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. |
|
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). |
|
All the packages you have installed. It'l give you a patch error if you have patches for non-installed packages. |
|
I have them all installed, but only care about changes in Analytics, I still need to apply them all right? |
Description
A well put description can be found at #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
AndroidiOSe2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter