-
Notifications
You must be signed in to change notification settings - Fork 4k
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 consent mode v2 #12298
feat(analytics): add consent mode v2 #12298
Conversation
103955b
to
5458c36
Compare
packages/firebase_analytics/firebase_analytics/lib/src/firebase_analytics.dart
Outdated
Show resolved
Hide resolved
93f824b
to
5959566
Compare
Updated cocoapods and the example app is now working. For some reasons, the pods where not being copied to the corresponding folder. Example app with works both on iOS and Android. |
84685a5
to
bc049af
Compare
Any updates? @russellwheatley |
@matinzd - not as such. Will let you know when it has been decided 👍 |
Is there an estimated time for the decision to approve the PR? |
ed8567d
to
98d7993
Compare
@matinzd thank you already for your effort implementing this! |
IMO, it's better to merge iOS and Android first and then have the web implemented in a separate PR. For now, it seems like it's not a priority at all as this PR is officially one month old and all of our campaigns paused due to this being a blocker for us. |
98d7993
to
d9809f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just need to pipe through the additional web consents and then I'll see what else needs to be updated, if anything 👍
packages/firebase_analytics/firebase_analytics/lib/src/firebase_analytics.dart
Show resolved
Hide resolved
packages/firebase_analytics/firebase_analytics_web/lib/firebase_analytics_web.dart
Show resolved
Hide resolved
I am gonna adjust the PR tonight. Thanks for the feedback! |
@matinzd - we are looking to get this released next week, let me know if you have the time to complete, if not, I can take over. Thanks 😄 |
Hey! |
d9809f7
to
510be95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we could get this done by some point tomorrow at the latest so we can include it in the latest release. Thanks
packages/firebase_analytics/firebase_analytics_web/lib/firebase_analytics_web.dart
Outdated
Show resolved
Hide resolved
45f02c6
to
72834b2
Compare
Fixed some formatting issues. Can you rerun the workflows @russellwheatley ? |
9c957e9
to
38e1534
Compare
cf97b73
to
f7774a3
Compare
FYI, I copied your code from PR. I've enabled 4 consents in the Firebase analytics code but in the "Verbose" log I can see only 3 consents granted. I did not see "adPersonalization" consent property. Do you know why? How to enable a verbose log you can check this link? Can you please check from your end, if it works fine? do you have any suggestion where I made mistake? |
Which consent is missing in verbose log? @nitinrgadhiya |
|
I couldn't find any issues with |
yes, I agree with what you say ("about key passed properly."). And if your feeling is right, then It's okay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
packages/firebase_analytics/firebase_analytics_web/lib/interop/analytics_interop.dart
Show resolved
Hide resolved
…/analytics_interop.dart
Hey @matinzd - I just realised the integration tests are failing and need updating here: https://github.com/firebase/flutterfire/blob/master/tests/integration_test/firebase_analytics/firebase_analytics_e2e_test.dart#L211-L228 Update to this should work: test(
'setConsent',
() async {
await expectLater(
FirebaseAnalytics.instance.setConsent(
analyticsStorageConsentGranted: true,
adStorageConsentGranted: true,
adPersonalizationSignalsConsentGranted: true,
adUserDataConsentGranted: true,
functionalityStorageConsentGranted: true,
personalizationStorageConsentGranted: true,
securityStorageConsentGranted: true,
),
completes,
);
},
); |
removed expecting unimplemented
Fixed. If anything else popped up, feel free to push to the branch 👍 |
Thanks a lot @matinzd for helping with this, really appreciated :D |
Co-authored-by: matin-zadeh-dolatabad <matin.zadeh.dolatabad@schibsted.com> Co-authored-by: Guillaume Bernos <guillaume.bernos@gmail.com> Co-authored-by: Guillaume Bernos <guillaume@bernos.dev>
Co-authored-by: matin-zadeh-dolatabad <matin.zadeh.dolatabad@schibsted.com> Co-authored-by: Guillaume Bernos <guillaume.bernos@gmail.com> Co-authored-by: Guillaume Bernos <guillaume@bernos.dev>
Co-authored-by: matin-zadeh-dolatabad <matin.zadeh.dolatabad@schibsted.com> Co-authored-by: Guillaume Bernos <guillaume.bernos@gmail.com> Co-authored-by: Guillaume Bernos <guillaume@bernos.dev>
Co-authored-by: matin-zadeh-dolatabad <matin.zadeh.dolatabad@schibsted.com> Co-authored-by: Guillaume Bernos <guillaume.bernos@gmail.com> Co-authored-by: Guillaume Bernos <guillaume@bernos.dev>
Co-authored-by: matin-zadeh-dolatabad <matin.zadeh.dolatabad@schibsted.com> Co-authored-by: Guillaume Bernos <guillaume.bernos@gmail.com> Co-authored-by: Guillaume Bernos <guillaume@bernos.dev>
Co-authored-by: matin-zadeh-dolatabad <matin.zadeh.dolatabad@schibsted.com> Co-authored-by: Guillaume Bernos <guillaume.bernos@gmail.com> Co-authored-by: Guillaume Bernos <guillaume@bernos.dev>
Description
Adding consent mode v2 to firebase analytics.
Reference: https://developers.google.com/tag-platform/security/guides/app-consent
Related Issues
Fixes #12148
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
).This will ensure a smooth and quick review process. Updating the
pubspec.yaml
and changelogs is not required.///
).melos run analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?