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(messaging): get remote message from messaging store on new intent #4634

Merged
merged 1 commit into from
Dec 2, 2020

Conversation

dlindstrm
Copy link
Contributor

@dlindstrm dlindstrm commented Dec 1, 2020

Description

ReactNativeFirebaseMessagingModule -> onNewIntent is not checking the messaging store for remote messages (like getInitialNotification does).

Steps to reproduce the bug:

  1. Receive a notification but don't open it (the remote message is added to ReactNativeFirebaseMessagingReceiver.notifications).
  2. Open the app and then kill it (ReactNativeFirebaseMessagingReceiver.notifications is reseted).
  3. Open the app again and bring it to background state.
  4. Press the notification (ReactNativeFirebaseMessagingReceiver.notifications is empty).

This results in onNewIntent is unable to get the remote message because it is not checking in the persisted store.

Related issues

I have not submitted an issue for this problem and I have not found any other issues.

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 Dec 1, 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/3yax8a6sz
✅ Preview: https://react-native-firebase-git-master.invertase.vercel.app

@CLAassistant
Copy link

CLAassistant commented Dec 1, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Dec 1, 2020

Codecov Report

Merging #4634 (81a77c8) into master (11867da) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4634   +/-   ##
=======================================
  Coverage   88.93%   88.93%           
=======================================
  Files         109      109           
  Lines        3712     3712           
  Branches      347      347           
=======================================
  Hits         3301     3301           
  Misses        369      369           
  Partials       42       42           

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.

Good find - nice extract method refactor to implement the fix - should pass CI and if the CLA is signed I'll merge this. I don't have anything else pending right now so I could release it as a 10.1.1 pretty much straight away, though obviously you can use patch-package to ingest it in the meanwhile (the CI check for patch packages will have an artifact attached to it with the necessary patches, for convenience, just follow the details link next to it)

@mikehardy mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Dec 1, 2020
@dlindstrm
Copy link
Contributor Author

Good find - nice extract method refactor to implement the fix - should pass CI and if the CLA is signed I'll merge this. I don't have anything else pending right now so I could release it as a 10.1.1 pretty much straight away, though obviously you can use patch-package to ingest it in the meanwhile (the CI check for patch packages will have an artifact attached to it with the necessary patches, for convenience, just follow the details link next to it)

Nice that was quick 👍 I have already patched it, but thanks for the tip of using the attached artifact.

@mikehardy
Copy link
Collaborator

CI working just fine (as expected, really) now that GitHub actions are working well again after their degradation yesterday

@mikehardy mikehardy merged commit 00b83af into invertase:master Dec 2, 2020
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Dec 2, 2020
@mikehardy
Copy link
Collaborator

Should be out in v10.1.1 now, we had some issues with publishing last few months which I believe I have fixed, but I mention that because when you adopt the release pay special attention to the cross-package peer dependencies to make sure nothing is skewed. Cheers!

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