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 foreground notification display bug #1034

Merged
merged 9 commits into from
Dec 30, 2020
Merged

Fix foreground notification display bug #1034

merged 9 commits into from
Dec 30, 2020

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Dec 22, 2020

cc @marcaaron @AndrewGable

Details

Turns out that we had included @react-native-community/push-notification-ios as a dependency in order to display the unread indicator badge on iOS. This conflicted with our UA integration, which merges foreground presentation settings from the AppDelegate. Does that even make sense? It's pretty close to being true. Details here.

Fixed Issues

https://github.com/Expensify/Expensify/issues/144297

Tests

  1. Run the iOS app on a physical device. If you don't have an iPhone then I can find a different reviewer.
  2. At the same time, run the app on web or desktop.
  3. Log into one account on the iOS device, and another on the laptop.
  4. With the iOS app open and in the foreground, send a message from the laptop to the iPhone. Ensure that you do not receive a push notification.
  5. Background the iOS app (just return to the homescreen), and send another message from the laptop to the iPhone. Ensure that you receive a push notification (don't tap the notification just yet).
  6. With the iOS app still in the background, send another message from the laptop to the iPhone. Ensure that you receive a push notification. This time, tap the notification and ensure that you are taken to the relevant chat.
  7. Close the iOS app, then repeat steps 1-6, replacing the iOS app with the Android app (emulator should work fine).

@roryabraham roryabraham requested a review from a team as a code owner December 22, 2020 02:00
@roryabraham roryabraham self-assigned this Dec 22, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Dec 22, 2020

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@botify botify requested review from Luke9389 and removed request for a team December 22, 2020 02:00
@roryabraham roryabraham changed the title [draft] Fix foreground notification display bug Fix foreground notification display bug Dec 28, 2020
@Luke9389
Copy link
Contributor

I'll give this a look once it's no longer a draft. Would you mind changing the title to indicate it's a WIP, or use GH's draft function to mark this as such?

@roryabraham
Copy link
Contributor Author

Hey Luke, np - I'll mark this as WIP while I work on fixing the E2E tests.

@roryabraham roryabraham changed the title Fix foreground notification display bug [WIP] Fix foreground notification display bug Dec 28, 2020
@roryabraham roryabraham changed the title [WIP] Fix foreground notification display bug Fix foreground notification display bug Dec 29, 2020
@roryabraham
Copy link
Contributor Author

@Luke9389 this is no longer WIP

@Luke9389
Copy link
Contributor

Ok cool! Are there tests I can perform for this?

@roryabraham
Copy link
Contributor Author

My bad @Luke9389 🤦

There are testing steps now 👍

@Luke9389
Copy link
Contributor

Luke9389 commented Dec 29, 2020

No problemo my dude! 😄

One actual problemo is that I don't have an iPhone... 😬

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

LGTM

@marcaaron
Copy link
Contributor

I don't have an iPhone either so can't test this.

@marcaaron marcaaron merged commit e2a7cee into master Dec 30, 2020
@marcaaron marcaaron deleted the Rory-UpgradeUA branch December 30, 2020 01:10
@github-actions github-actions bot locked and limited conversation to collaborators Dec 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants