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): Adding default event parameters #5246

Merged
merged 9 commits into from
May 2, 2021

Conversation

MFrat
Copy link
Contributor

@MFrat MFrat commented May 1, 2021

Description

Adding feature for setting default event parameters. As described here in Firebase docs.

Release Summary

Added setDefaultEventParameters. It adds default event parameters that will be send in every event.

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:

@CLAassistant
Copy link

CLAassistant commented May 1, 2021

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented May 1, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/DM1PTs437jDP8xcXjef2GuQpi75W
✅ Preview: https://react-native-f-git-fork-mfrat-feature-default-parameters-d30d9d.vercel.app

@MFrat MFrat changed the title Feature/default parameters feat(analytics): Adding default event parameters May 1, 2021
@MFrat MFrat marked this pull request as ready for review May 1, 2021 19:59
mikehardy
mikehardy previously approved these changes May 1, 2021
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.

Wow, this looks fantastic first shot. Thank you!
Github requires that a maintainer approves the CI runs on first contributions (to avoid PRs that just implement a cryptominer I think) - so I've done that and will wait for CI to go green

Looks like other than that you just need to follow the "Details" link on the failing CLA / license check and sign the license then we can merge this

@mikehardy
Copy link
Collaborator

Oh - now that CI has run it looks like some trivial formatting issues, and the jest error message isn't quite as expected as well as the CLA - waiting for those

@mikehardy mikehardy added Blocked: Missing CLA Workflow: Pending Merge Waiting on CI or similar Workflow: Waiting for User Response Blocked waiting for user response. labels May 1, 2021
@codecov
Copy link

codecov bot commented May 1, 2021

Codecov Report

Merging #5246 (1efca6c) into master (3396da2) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 1efca6c differs from pull request most recent head 4848a41. Consider uploading reports for the commit 4848a41 to get more accurate results

@@            Coverage Diff             @@
##           master    #5246      +/-   ##
==========================================
+ Coverage   88.86%   88.87%   +0.01%     
==========================================
  Files         109      109              
  Lines        3743     3746       +3     
  Branches      360      361       +1     
==========================================
+ Hits         3326     3329       +3     
  Misses        370      370              
  Partials       47       47              

@MFrat
Copy link
Contributor Author

MFrat commented May 1, 2021

Thanks for the tips. Done!

@mikehardy
Copy link
Collaborator

Looks like there is still an issue with the CLA signing - I have had this happen before where I needed to add the email address as it mentions:

Max Fratane seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

It appears to be looking for yoru max2fratane @ popular mail provider email address as that's the address in the commits on your fork

@mikehardy mikehardy removed the Workflow: Waiting for User Response Blocked waiting for user response. label May 1, 2021
@MFrat
Copy link
Contributor Author

MFrat commented May 1, 2021

E-mail added to my github account.

@mikehardy
Copy link
Collaborator

Hey @MFrat - before I merge this, since it seemed like the iOS side had compile errors and couldn't have been checked - can you install the auto-generated patch-package format patches from CI and make sure they work in your project, with expected results on android and iOS? There is an artifacts link here that has them https://github.com/invertase/react-native-firebase/actions/runs/803147844

@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar Workflow: Waiting for User Response Blocked waiting for user response. labels May 2, 2021
@MFrat
Copy link
Contributor Author

MFrat commented May 2, 2021

@mikehardy, I tested both in Android and iOS, with devices and simulator/emulator. Everything is working just fine.

Screen Shot 2021-05-02 at 16 01 38

Screen Shot 2021-05-02 at 15 43 02

Android
Screen Shot 2021-05-02 at 15 59 34

iOS
Screen Shot 2021-05-02 at 16 29 31
Screen Shot 2021-05-02 at 16 28 21

@mikehardy
Copy link
Collaborator

That's glorious :-), patch-package is such a beautiful thing. Thanks again for the PR here I'll merge it but hold for a few days prior to release in order to catch any other changes - you're set via patch-package until then though (it gives you a nice warning when you npm/yarn install after the package it has a patch for changes version so you'll know to clear the patch from source control later)

@mikehardy mikehardy merged commit 684bb50 into invertase:master May 2, 2021
@mikehardy mikehardy removed Workflow: Pending Merge Waiting on CI or similar Workflow: Waiting for User Response Blocked waiting for user response. labels May 2, 2021
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.

3 participants