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.

Sign up for GitHub

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

Jump to bottom

[Wallet] Support push notifications with an "open url" semantic #5413

Merged
merged 3 commits into from
Oct 16, 2020
Merged

[Wallet] Support push notifications with an "open url" semantic #5413

merged 3 commits into from
Oct 16, 2020

Conversation

jeanregisser
Copy link
Contributor

@jeanregisser jeanregisser commented Oct 16, 2020 •
edited
Loading

Description

This PR adds support for push notifications with an "open url" semantic.
When the ou (Open Url) data field is set, it redirects the user to that url.

Other changes

  • Fix notification not handled when app is launched because of said notification
  • Fix notification body not displayed
  • Fix banner title color
  • AlertBanner/showMessage now supports an action parameter
  • Add test for notifications handling

Tested

Example marketing notification

Payment received (before/after)

Banner (before/after)

Related issues

Backwards compatibility

Yes

@jeanregisser
[Wallet] Support push notifications with an "open url" semantic
b1a4fd0 
- When the `ou` (Open Url) field is set, it redirect the user to that url
- Fix notifications not handled when app is launched because of a notification
- AlertBanner/showMessage now supports an action parameter
- Add test for notifications handling
jeanregisser
jeanregisser commented Oct 16, 2020
View reviewed changes
packages/mobile/src/firebase/firebase.ts
Logger.info(TAG, 'App opened fresh via a notification')
yield call(
handleNotification,
initialNotification.notification,
Copy link
Contributor Author

@jeanregisser jeanregisser Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were bit again by redux-saga types not being inferred correctly.
Here leading to the initial notification to not be handled correctly.

@jeanregisser jeanregisser requested a review from a team October 16, 2020 14:47
gnardini
gnardini approved these changes Oct 16, 2020
View reviewed changes
Copy link
Contributor

@gnardini gnardini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!! Just wondering, does Ankit test push notifications? I imagine he might have a hard time receiving one, right? What's the easiest way to receive one for testing?

@jeanregisser
Copy link
Contributor Author

Yes he tests notifications between 2 devices and has reported when they were missing in the past.

@jeanregisser jeanregisser added the automerge Have PR merge automatically when checks pass label Oct 16, 2020
@mergify mergify bot merged commit 57eca52 into master Oct 16, 2020
@mergify mergify bot deleted the jeanregisser/handle-push-url branch October 16, 2020 19:29
@Lss-Ankit
Copy link

Hi @gnardini Verified task using latest Android play store internal build v1.3.0(1004294316) & Test Flight build v1.3.0(27) and found the following:

  • " Payment Received You've received # celo dollars" in-app notification is shown when user A is received amount from user B.
    We are not getting "Hey Tap this to send your feedback" & "Alfajore A friendly reminder you're ...." in app notification can you please let us know criteria to get the the in -app notification.

Note: Previously "Alfajore A friendly reminder you're ...." in app notification is shown when user fresh sign up but now we are not getting even after fresh sign up in the app.

Verified devices:

  • iPhone SE 2 (13.5) , Google Pixel Xl (8.1)
    CC: @jeanregisser
    Thanks!

@jeanregisser
Copy link
Contributor Author

Thanks @Lss-Ankit, all this is expected:

  • the example push in the screenshot (Hey Tap this...) is something that can be achieved when manually sending push notifications for marketing purpose.
  • the alfajores reminder banner is only present on builds connected to the alfajores network.

@Lss-Ankit
Copy link

Lss-Ankit commented Oct 26, 2020 •
edited by jeanregisser
Loading

Thanks @jeanregisser for the update.

So #1, is there any chance that we can get the "Hey Tap this to send your feedback" notification for testing purpose?
#2 yes correct , current build is pointing out to Mainnet so we will not get, do we need to test this on alfajore network if yes can you please provide build details.
Thanks!

@jeanregisser
Copy link
Contributor Author

@Lss-Ankit ping me tomorrow and I can send you such push notification.

And no need to test the alfajores banner.

Thanks

@Lss-Ankit
Copy link

Thanks @jeanregisser for the co-ordination.
We have verified "Hey Tap this to send your feedback" push notification using Test flight build v1.3.0(27) and Android play store internal build v1.3.0(1004294316) and fond below new issue.

Verified scenarios:

  1. Verify user get the "Hey Tap this to send your feedback" In-app notification and user is able to redirect to celo.org by tapping on in-app notification. - Pass

  2. Verify user get the "Hey Tap this to send your feedback" Push notification and user is able to redirect to celo.org by tapping on push notification if app is running in the background. - Pass

  3. Verify user get the "Hey Tap this to send your feedback" Push notification and user is able to redirect to celo.org by tapping on push notification if app is not running in the background. - Pass for IOS, but Failed for Android

New Issue: https://github.com/celo-org/celo-monorepo/issues/5592

CC: @gnardini

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnardini gnardini gnardini approved these changes

@aaronmgdr aaronmgdr Awaiting requested review from aaronmgdr

@cmcewen cmcewen Awaiting requested review from cmcewen

@i1skn i1skn Awaiting requested review from i1skn

@jmrossy jmrossy Awaiting requested review from jmrossy

Assignees
No one assigned
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow push notifications to redirect the user to a URL (for gathering feedback)
3 participants
@jeanregisser @Lss-Ankit @gnardini