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 setConsent implementation #7629

Merged

Conversation

Summys
Copy link
Contributor

@Summys Summys commented Feb 16, 2024

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

  • 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;
    • No

Test Plan


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

Copy link

vercel bot commented Feb 16, 2024

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

@CLAassistant
Copy link

CLAassistant commented Feb 16, 2024

CLA assistant check
All committers have signed the CLA.

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

packages/analytics/lib/modular/index.js Outdated Show resolved Hide resolved
@mikehardy mikehardy added Workflow: Waiting for User Response Blocked waiting for user response. plugin: analytics Google Analytics for Firebase type: enhancement Implements a new Feature labels Feb 16, 2024
@Summys
Copy link
Contributor Author

Summys commented Feb 19, 2024

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

mikehardy
mikehardy previously approved these changes Feb 19, 2024
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 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 mikehardy added Workflow: Pending Merge Waiting on CI or similar and removed Workflow: Waiting for User Response Blocked waiting for user response. labels Feb 19, 2024
@nixolas1
Copy link

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

@mikehardy
Copy link
Collaborator

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

@Summys
Copy link
Contributor Author

Summys commented Feb 19, 2024

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

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.

😆 that'll do it, CI chewing on things now

Copy link

codecov bot commented Feb 19, 2024

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     

@mikehardy
Copy link
Collaborator

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 mikehardy merged commit 7816985 into invertase:main Feb 19, 2024
14 of 17 checks passed
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Feb 19, 2024
@mikehardy
Copy link
Collaborator

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

@RamProg
Copy link

RamProg commented Feb 21, 2024

How can I use this commit before the new release?

@nixolas1
Copy link

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.

@RamProg
Copy link

RamProg commented Feb 21, 2024

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).

@nixolas1
Copy link

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

@RamProg
Copy link

RamProg commented Feb 21, 2024

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

@Summys Summys deleted the feature/add-analytics-set-consent-mode branch February 22, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants