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): remove ad_impression from reserved events #6312

Closed

Conversation

alexpovkolas
Copy link

Description

Related issues

Fixes #6307

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

Test Plan


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

@vercel
Copy link

vercel bot commented Jun 15, 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 15, 2022 at 9:52AM (UTC)
1 Ignored Deployment
Name Status Preview Updated
react-native-firebase-next ⬜️ Ignored (Inspect) Jun 15, 2022 at 9:52AM (UTC)

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

nice - thank you! Even for small things we need the CLA signed but code wise, of course (since it is a simple change) it looks good...

@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #6312 (58e12cd) into main (ee740f8) will increase coverage by 18.88%.
The diff coverage is n/a.

❗ Current head 58e12cd differs from pull request most recent head 216fe40. Consider uploading reports for the commit 216fe40 to get more accurate results

@@              Coverage Diff              @@
##               main    #6312       +/-   ##
=============================================
+ Coverage     53.42%   72.30%   +18.88%     
=============================================
  Files           208      109       -99     
  Lines         10315     4649     -5666     
  Branches       1633     1046      -587     
=============================================
- Hits           5510     3361     -2149     
+ Misses         4547     1210     -3337     
+ Partials        258       78      -180     

mikehardy added a commit that referenced this pull request Jun 17, 2022
@mikehardy
Copy link
Collaborator

CLA missing and lint needs some help. #6314 will implement this

@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Jun 17, 2022
mikehardy added a commit that referenced this pull request Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ad_impression event needs to be removed from reserved events list
3 participants