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

iOS - Push notifications are not opening correct chat #6079

Closed
isagoico opened this issue Oct 27, 2021 · 49 comments
Closed

iOS - Push notifications are not opening correct chat #6079

isagoico opened this issue Oct 27, 2021 · 49 comments
Assignees
Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Oct 27, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Have some notifications from New Expensify conversations in iOS
  2. Tap any of the notification

Expected Result:

User should be taken to correct chat.

Actual Result:

Some of the time, the user is not taken to correct chat.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Get taken to the wrong chat and then go click the correct one.

Platform:

Where is this issue occurring?

  • iOS

Version Number: 1.1.10-0

Reproducible in staging?: Yes
Reproducible in production?: Yes

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: @roryabraham
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1635210815095800

View all open jobs on GitHub

@MelvinBot
Copy link

Triggered auto assignment to @Luke9389 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@Luke9389 Luke9389 added the External Added to denote the issue can be worked on by a contributor label Oct 27, 2021
@MelvinBot
Copy link

Triggered auto assignment to @adelekennedy (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@adelekennedy
Copy link

Created the job in Upwork:
Internal
External

@MelvinBot MelvinBot added Weekly KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors and removed Daily KSv2 labels Oct 27, 2021
@adelekennedy adelekennedy added Daily KSv2 and removed Daily KSv2 labels Oct 27, 2021
@adelekennedy
Copy link

@Luke9389 whoops, I'm a couple days late on this but no bites so far. Going to double the job

@roryabraham
Copy link
Contributor

Whoops, this has to be internal. External contributors have no way of testing push notifications locally.

@roryabraham roryabraham added Internal Requires API changes or must be handled by Expensify staff and removed Help Wanted Apply this label when an issue is open to proposals by contributors External Added to denote the issue can be worked on by a contributor Exported labels Nov 4, 2021
@roryabraham
Copy link
Contributor

@adelekennedy please take down the Upwork job

@adelekennedy
Copy link

@roryabraham thank you! Is this something I could have spotted and missed? (I want to make sure I'm following procedure)

@roryabraham
Copy link
Contributor

@adelekennedy I think you followed procedure just fine. Push notifications just happen to be one of those things that we've found external contributors can't work on.

@Luke9389 the reason is because iOS push notifications can't be tested on a simulator, and you can't build the mobile app to a physical device unless you add the device to our mobile provisioning profile in the App Store.

@Luke9389
Copy link
Contributor

Luke9389 commented Nov 4, 2021

Dude, nice catch. Sorry about that! Will make note of this for the future. Actually... do you know if we have a list of topics that need to be internal? Maybe it's not necessary, idk.

@roryabraham
Copy link
Contributor

do you know if we have a list of topics that need to be internal?

I am not aware of such a list

@Luke9389
Copy link
Contributor

Luke9389 commented Nov 4, 2021

Yea I searched SO and didn't see one. Do you think it's necessary? What else would go on there aside from notifications? "Anything requiring Web-E changes". Seems like this may not be needed...

@adelekennedy
Copy link

The jobs been closed 💀

@neil-marcellini neil-marcellini removed their assignment Mar 9, 2022
@AndrewGable AndrewGable added Weekly KSv2 and removed Monthly KSv2 labels Mar 14, 2022
@AndrewGable AndrewGable self-assigned this Mar 14, 2022
@AndrewGable
Copy link
Contributor

Keeping notes:

  1. npm install && cd ios && pod install
  2. npm run ios -- --device "ag" ("ag" is my iPhone's name)
  3. Manually open app, metro loads index.js
  4. Replace AIRSHIP_KEY_EXPENSIFY_CASH and AIRSHIP_SECRET_EXPENSIFY_CASH in dev config
  5. Run ./script/clitools.sh push:reportComment

Still not receiving pushes locally

@AndrewGable
Copy link
Contributor

Interestingly - If I send the notification to OldDot, I get the message, so thinking it has something to do with newDot specifically

@AndrewGable
Copy link
Contributor

Trying to use an account that I am not signed into on web to make sure it's not getting ignored

@AndrewGable
Copy link
Contributor

Ok so I am able to reproduce the bug and get push notifications to work via the TestFlight, but still not from a local build.

@AndrewGable
Copy link
Contributor

Ok finally got local notifications working, looks like I didn't need to do step 4 here

@AndrewGable
Copy link
Contributor

Ok from my testing the PushNotification.onSelected callback never fires when the user hits the notification.

@AndrewGable
Copy link
Contributor

Ok so PushNotification.onSelected is being fired, but I was running this code: #6079 (comment)

But Navigation.isReady() was crashing 🤔

@AndrewGable
Copy link
Contributor

Ok so seeing Navigation.navigate(ROUTES.getReportRoute(reportID)); fail with the error: DEBUG [hmmm] [Navigation] navigate failed because navigation ref was not yet ready - {"route":"r/63481527"} so we definitely do need some sort of check to see if the navigation is "ready".

@AndrewGable
Copy link
Contributor

@roryabraham I've now found your comment here: react-navigation/react-navigation#9170 (comment)

I think I am in a loop!

@AndrewGable
Copy link
Contributor

Ok I opened a PR that seems to work for all the cases I can throw at it: #8353

Let me know what you think @roryabraham!

@AndrewGable
Copy link
Contributor

PR in preview

@AndrewGable AndrewGable added the Reviewing Has a PR in review label Mar 31, 2022
@botify botify closed this as completed Apr 5, 2022
@neil-marcellini
Copy link
Contributor

Still reproducible according to QA here.

@AndrewGable
Copy link
Contributor

@kavimuru coming from #7901 (comment)

Can you please be more specific? I watched the iOS video and from 2:14 to the end it looks like the notification worked, but I didn't see any other notifications tapped.

@mallenexpensify
Copy link
Contributor

@kavimuru can you test again to see if you can reproduce? If so, can you provide details?

@mallenexpensify mallenexpensify added the Improvement Item broken or needs improvement. label Apr 18, 2022
@AndrewGable
Copy link
Contributor

I've been testing this myself on my device and was unable to reproduce.

@mallenexpensify
Copy link
Contributor

I just tested on iOS v1.1.52-0 and it opened to the correct chat. (side note... iOS has been hella slow/laggy the past couple weeks, at least).

@AndrewGable
Copy link
Contributor

I think this is fixed. If there is a regression found let's open a new issue.

@kbecciv
Copy link

kbecciv commented May 26, 2022

@AndrewGable Open a new ticket #9179

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests