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.

Sign up for GitHub

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

Jump to bottom

Fix setConsent to properly use the update gtag command #8243

Merged
merged 9 commits into from
May 15, 2024
Merged

Fix setConsent to properly use the update gtag command #8243

merged 9 commits into from
May 15, 2024

Conversation

DellaBitta
Copy link
Contributor

@DellaBitta DellaBitta commented May 13, 2024 •
edited
Loading

Discussion

We wrap the gtag function internally and pass setConsent calls through the wrapper function, which in turn calls the gtag SDK. While we invoke the wrapped function with parameters identical to those listed in the gtag functions, we were dropping the update string on the floor inside the wrapped function. Additionally, we were feeding the consentSettings as the second parameter to gag, which should have been the update string parameter.

This ended up clobbering the data, and not send update commands properly.

This change fixes our wrappedGtag func impl to properly pass update to gtag, along with the provided ConsentSettings argument.

Closes #8210

Testing

  • Added a higher level of fidelity to our unit tests by testing the values set in window['dataLayer'].
  • CI passes.

API Changes

N/A

@DellaBitta
Ensure 'update' string reaches gtagCore.
a10daf1 
Include the consent string ('update') in gtagCore calls that update consent.
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented May 13, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 5e07a73

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@firebase/analytics Patch
@firebase/analytics-compat Patch
firebase Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 13, 2024 •
edited
Loading

Changeset File Check ✅

  • No modified packages are missing from the changeset file.
  • No changeset formatting errors detected.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 13, 2024 •
edited
Loading

Size Report 1

Affected Products

  • @firebase/analytics

    TypeBase (9cbb184)Merge (f7c9e13)Diff
    browser21.8 kB21.8 kB+19 B (+0.1%)
    esm526.9 kB27.0 kB+41 B (+0.2%)
    main28.4 kB28.4 kB+41 B (+0.1%)
    module21.8 kB21.8 kB+19 B (+0.1%)

  • @firebase/firestore

    TypeBase (9cbb184)Merge (f7c9e13)Diff
    esm5363 kB363 kB-2 B (-0.0%)
    main582 kB582 kB-11 B (-0.0%)

  • bundle

    TypeBase (9cbb184)Merge (f7c9e13)Diff
    analytics (logEvent)44.5 kB44.5 kB-5 B (-0.0%)

  • firebase

    TypeBase (9cbb184)Merge (f7c9e13)Diff
    firebase-analytics-compat.js26.5 kB26.5 kB-3 B (-0.0%)
    firebase-analytics.js29.7 kB29.7 kB-5 B (-0.0%)
    firebase-compat.js786 kB786 kB-2 B (-0.0%)
    firebase-firestore-compat.js341 kB341 kB+1 B (+0.0%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/XTSHuCcn6V.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 13, 2024 •
edited
Loading

Size Analysis Report 1

Affected Products

  • @firebase/analytics

    • getAnalytics

      Size

      TypeBase (9cbb184)Merge (f7c9e13)Diff
      size10.6 kB10.6 kB-5 B (-0.0%)
      size-with-ext-deps44.4 kB44.4 kB-5 B (-0.0%)

    • getGoogleAnalyticsClientId

      Size

      TypeBase (9cbb184)Merge (f7c9e13)Diff
      size10.5 kB10.5 kB-5 B (-0.0%)
      size-with-ext-deps37.2 kB37.2 kB-5 B (-0.0%)

    • initializeAnalytics

      Size

      TypeBase (9cbb184)Merge (f7c9e13)Diff
      size10.5 kB10.5 kB-5 B (-0.0%)
      size-with-ext-deps37.5 kB37.5 kB-5 B (-0.0%)

    • isSupported

      Size

      TypeBase (9cbb184)Merge (f7c9e13)Diff
      size10.4 kB10.4 kB-5 B (-0.0%)
      size-with-ext-deps37.1 kB37.1 kB-5 B (-0.0%)

    • logEvent

      Size

      TypeBase (9cbb184)Merge (f7c9e13)Diff
      size10.2 kB10.2 kB-5 B (-0.0%)
      size-with-ext-deps37.0 kB37.0 kB-5 B (-0.0%)

    • setAnalyticsCollectionEnabled

      Size

      TypeBase (9cbb184)Merge (f7c9e13)Diff
      size10.4 kB10.4 kB-5 B (-0.0%)
      size-with-ext-deps37.1 kB37.1 kB-5 B (-0.0%)

    • setConsent

      Size

      TypeBase (9cbb184)Merge (f7c9e13)Diff
      size10.4 kB10.4 kB-5 B (-0.0%)
      size-with-ext-deps37.1 kB37.1 kB-5 B (-0.0%)

    • setCurrentScreen

      Size

      TypeBase (9cbb184)Merge (f7c9e13)Diff
      size10.5 kB10.5 kB-5 B (-0.0%)
      size-with-ext-deps37.2 kB37.2 kB-5 B (-0.0%)

    • setDefaultEventParameters

      Size

      TypeBase (9cbb184)Merge (f7c9e13)Diff
      size10.4 kB10.4 kB-5 B (-0.0%)
      size-with-ext-deps37.1 kB37.1 kB-5 B (-0.0%)

    • setUserId

      Size

      TypeBase (9cbb184)Merge (f7c9e13)Diff
      size10.5 kB10.5 kB-5 B (-0.0%)
      size-with-ext-deps37.2 kB37.2 kB-5 B (-0.0%)

    • setUserProperties

      Size

      TypeBase (9cbb184)Merge (f7c9e13)Diff
      size10.6 kB10.5 kB-5 B (-0.0%)
      size-with-ext-deps37.3 kB37.3 kB-5 B (-0.0%)

    • settings

      Size

      TypeBase (9cbb184)Merge (f7c9e13)Diff
      size10.4 kB10.4 kB-5 B (-0.0%)
      size-with-ext-deps37.1 kB37.1 kB-5 B (-0.0%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/r6uUBd6dZl.html

@DellaBitta DellaBitta requested a review from hsubox76 May 13, 2024 18:06
@DellaBitta DellaBitta marked this pull request as ready for review May 13, 2024 18:06
@DellaBitta DellaBitta requested review from a team as code owners May 13, 2024 18:06
@DellaBitta DellaBitta requested a review from hsubox76 May 15, 2024 15:00
@DellaBitta DellaBitta merged commit f66769c into master May 15, 2024
36 checks passed
@DellaBitta DellaBitta deleted the ddb-analytics-set-consent branch May 15, 2024 18:11
@DellaBitta DellaBitta mentioned this pull request May 15, 2024
Closed
@google-oss-bot google-oss-bot mentioned this pull request May 20, 2024
Merged
@sswietoniowski sswietoniowski mentioned this pull request Jun 10, 2024
Closed
@Abellache-Rabah Abellache-Rabah mentioned this pull request Jun 10, 2024
Open
@leoo1992 leoo1992 mentioned this pull request Jun 10, 2024
Merged
@surajit03 surajit03 mentioned this pull request Jun 10, 2024
Open
@aswanthchandran0 aswanthchandran0 mentioned this pull request Jun 11, 2024
Open
@surajit03 surajit03 mentioned this pull request Jun 11, 2024
Open
@nejidevelops nejidevelops mentioned this pull request Jun 11, 2024
Open
@admin-token-bot admin-token-bot mentioned this pull request Jun 11, 2024
Closed
@lydmoon lydmoon mentioned this pull request Jun 11, 2024
Open
@kushalsdesk kushalsdesk mentioned this pull request Jun 12, 2024
Open
@sswietoniowski sswietoniowski mentioned this pull request Jun 12, 2024
Merged
@Abellache-Rabah Abellache-Rabah mentioned this pull request Jun 12, 2024
Open
@kushalsdesk kushalsdesk mentioned this pull request Jun 12, 2024
Open
@DevJPCosta DevJPCosta mentioned this pull request Jun 12, 2024
Open
@surajit03 surajit03 mentioned this pull request Jun 13, 2024
Open
@kenn7575 kenn7575 mentioned this pull request Jun 13, 2024
Merged
@abjerry97 abjerry97 mentioned this pull request Jun 13, 2024
Open
@lydmoon lydmoon mentioned this pull request Jun 13, 2024
Open
@jordiverbeek jordiverbeek mentioned this pull request Jun 13, 2024
Merged
@firebase firebase locked and limited conversation to collaborators Jun 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hsubox76 hsubox76 hsubox76 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Analytics Consent not sent on update
3 participants
@DellaBitta @google-oss-bot @hsubox76