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(inappmessaging): add support for triggering custom events #4201

Merged
merged 2 commits into from
Sep 3, 2020

Conversation

gilbertl
Copy link
Contributor

@gilbertl gilbertl commented Sep 2, 2020

Description

I want to use the programmatic event triggering feature in Firebase In App Messaging described here

https://firebase.google.com/docs/in-app-messaging/modify-message-behavior?platform=android#trigger_in-app_messages_programmatically

Related issues

No issues.

Release Summary

Support for In App Messaging's programmatic event triggering

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

It's not obvious how we can test whether the event was triggered.

  • jest tests added or updated in packages/\*\*/__tests__

There are no existing jest tests in the inappmessaging module

  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

I'm not sure how to test this ...

@CLAassistant
Copy link

CLAassistant commented Sep 2, 2020

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented Sep 2, 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/6sugiebrl
✅ Preview: https://react-native-fire-git-fork-gilbertl-gilbertl-trigger-iam-fa4862.invertase.vercel.app

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.

Hi there! This is awesome, thank you for extending the API here to wrap features we hadn't exposed from the native SDKs, that's a huge help

I'm prepared to accept this, it looks good except there is a compile error right now for iOS:

❌  /Users/runner/work/react-native-firebase/react-native-firebase/packages/in-app-messaging/ios/RNFBFiam/RNFBFiamModule.m:67:16: interface type 'NSString' cannot be passed by value; did you forget * in 'NSString'?

    (NSString) eventId

Seperately, can you think of a way to get some basic coverage on the API?

https://github.com/invertase/react-native-firebase/blob/master/packages/in-app-messaging/e2e/fiam.e2e.js

At minimum exercising that it checks the argument correctly would be good, I'm not sure how I would test the actual in app message, that might not be worth it here

@mikehardy mikehardy added Workflow: Waiting for User Response Blocked waiting for user response. plugin: in_app_messaging Firebase In-App Messaging (FIAM) type: enhancement Implements a new Feature labels Sep 2, 2020
@gilbertl
Copy link
Contributor Author

gilbertl commented Sep 2, 2020

Hi there! This is awesome, thank you for extending the API here to wrap features we hadn't exposed from the native SDKs, that's a huge help

I'm prepared to accept this, it looks good except there is a compile error right now for iOS:

❌  /Users/runner/work/react-native-firebase/react-native-firebase/packages/in-app-messaging/ios/RNFBFiam/RNFBFiamModule.m:67:16: interface type 'NSString' cannot be passed by value; did you forget * in 'NSString'?

    (NSString) eventId

Seperately, can you think of a way to get some basic coverage on the API?

master/packages/in-app-messaging/e2e/fiam.e2e.js

At minimum exercising that it checks the argument correctly would be good, I'm not sure how I would test the actual in app message, that might not be worth it here

Fixed the NSString* issue and added a test that at least calls the new function. Not sure what I can verify in the test, but at least this will verify that the app doesn't crash on calling the function?

@gilbertl gilbertl requested a review from mikehardy September 2, 2020 19:08
@mikehardy
Copy link
Collaborator

I think we still need the cla signed but otherwise it will probably be good to go when I reread 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.

Looks good to me, just need you to sign the CLA or we can't merge

@gilbertl gilbertl requested a review from mikehardy September 3, 2020 16:46
@gilbertl
Copy link
Contributor Author

gilbertl commented Sep 3, 2020

I think I signed 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.

Excellent! Thanks @gilbertl !

@mikehardy mikehardy removed the Workflow: Waiting for User Response Blocked waiting for user response. label Sep 3, 2020
@mikehardy mikehardy merged commit fe8cbc1 into invertase:master Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: in_app_messaging Firebase In-App Messaging (FIAM) type: enhancement Implements a new Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants