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): added a validation for length in analytics().logEvent(name, params) #4522

Merged
merged 2 commits into from
Nov 9, 2020
Merged

fix(analytics): added a validation for length in analytics().logEvent(name, params) #4522

merged 2 commits into from
Nov 9, 2020

Conversation

railsjack
Copy link
Contributor

@railsjack railsjack commented Nov 9, 2020

Description

I added a validation to check the length for name of analytics().logEvent(name, params): 1-40 alphanumeric

Related issues

Fixes #4496

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 Nov 9, 2020

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/plg0kmcxi
✅ Preview: Failed

[Deployment for 71345a8 failed]

@CLAassistant
Copy link

CLAassistant commented Nov 9, 2020

CLA assistant check
All committers have signed the CLA.

@mikehardy
Copy link
Collaborator

Looks like the test needs an update as well:

Expected substring: "firebase.analytics().logEvent() 'name' invalid event name '!@£$%^&'. Names should contain 1 to 32 alphanumeric characters or underscores."

at packages/analytics/tests/analytics.test.ts:162:41

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.

This looks good - just the CLA sign and the test update

@mikehardy mikehardy added the Workflow: Waiting for User Response Blocked waiting for user response. label Nov 9, 2020
@codecov
Copy link

codecov bot commented Nov 9, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@9fcd2af). Click here to learn what that means.
The diff coverage is 61.54%.

@@            Coverage Diff            @@
##             master    #4522   +/-   ##
=========================================
  Coverage          ?   41.92%           
=========================================
  Files             ?       35           
  Lines             ?     1119           
  Branches          ?      278           
=========================================
  Hits              ?      469           
  Misses            ?      489           
  Partials          ?      161           

@railsjack
Copy link
Contributor Author

I am not sure why Vercel has failed.

@mikehardy
Copy link
Collaborator

I'm not sure why Vercel is failing either 😅 - obviously has nothing to do with this PR though, I can merge without it passing

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.

This will pass now (or should...) I believe we're just missing the CLA now to merge

@mikehardy mikehardy added Blocked: Missing CLA Workflow: Pending Merge Waiting on CI or similar and removed Workflow: Waiting for User Response Blocked waiting for user response. labels Nov 9, 2020
@railsjack
Copy link
Contributor Author

Thank you, Mike.

@mikehardy
Copy link
Collaborator

Still missing the CLA signature though 🤔 @railsjack

@railsjack
Copy link
Contributor Author

What should I do to pass it?

@mikehardy
Copy link
Collaborator

@railsjack - follow the details link, should be https://cla-assistant.io/invertase/react-native-firebase?pullRequest=4522 - and sign the request with the same github userid / email used to make the PR

@railsjack
Copy link
Contributor Author

Done.

@mikehardy
Copy link
Collaborator

Fantastic - thanks for submitting this in the first place and sticking with it through all the administrivia bits :-)

@mikehardy mikehardy merged commit 107b07d into invertase:master Nov 9, 2020
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.

Needs to check the length of name for a method "logEvent(name, params)".
3 participants