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

fix(analytics): allow manual tracking of ad_impression #6314

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

mikehardy
Copy link
Collaborator

Description

ad_impression events are allowed through by the native SDKs despite being reserved words, and this is the recommended way to track ad impressions on certain platforms per upstream docs

Related issues

Fixes #6307
Supercedes, thus closes #6312

Release Summary

Conventional commit

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

Test Plan

Reporting user made change locally and tested it - reserved word passes cleanly through native SDKs and shows up in analytics


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

@vercel
Copy link

vercel bot commented Jun 17, 2022

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

Name Status Preview Updated
react-native-firebase ✅ Ready (Inspect) Visit Preview Jun 17, 2022 at 4:07PM (UTC)
1 Ignored Deployment
Name Status Preview Updated
react-native-firebase-next ⬜️ Ignored (Inspect) Jun 17, 2022 at 4:07PM (UTC)

@mikehardy mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Jun 17, 2022
@codecov
Copy link

codecov bot commented Jun 17, 2022

Codecov Report

Merging #6314 (a239eee) into main (ee740f8) will not change coverage.
The diff coverage is n/a.

❗ Current head a239eee differs from pull request most recent head fe559ac. Consider uploading reports for the commit fe559ac to get more accurate results

@@            Coverage Diff            @@
##               main    #6314   +/-   ##
=========================================
  Coverage     53.42%   53.42%           
  Complexity      644      644           
=========================================
  Files           208      208           
  Lines         10315    10315           
  Branches       1633     1633           
=========================================
  Hits           5510     5510           
  Misses         4547     4547           
  Partials        258      258           

@mikehardy mikehardy merged commit 931468a into main Jun 17, 2022
@mikehardy mikehardy deleted the @mikehardy/analytics-ad-impression branch June 17, 2022 16:47
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ad_impression event needs to be removed from reserved events list
1 participant