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

Make push notifications navigate to the correct chat - iOS #7633

Merged
merged 1 commit into from
Feb 21, 2022

Conversation

neil-marcellini
Copy link
Contributor

cc @roryabraham

Details

When push notifications are tapped the correct chat should open. I don't have permissions to create a production build (slack conversation) so I need one of the @Expensify/mobile-deployers to test this change for me. A production build is needed for testing push notifications when the app is closed to accurately reflect the way the app opens without first loading JS from the metro bundler.

Fixed Issues

$ #6079

Tests

  1. Create a local production build npm run ios-build and run the app on your iOS device
  2. Sign into the iOS app with account A
  3. Run web npm run web
  4. Sign in on web with account B
  5. Put the app in the background by going to the home screen, then locking your phone
  6. Send a message from account B to account A on web
  7. Tap the notification that should show up on you phone
  8. Ensure that the correct chat opens
  9. Close the app completely by clearing it from multi-tasking
  10. Repeat steps 6-8

If any of these steps don't work please let me know.

  • Verify that no errors appear in the JS console

QA Steps

Test steps 2-10

  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2022-02-08 at 10 01 02 AM

Mobile Web

Simulator Screen Shot - iPhone 13 - 2022-02-08 at 10 05 29

Desktop

Screen Shot 2022-02-08 at 10 10 03 AM

iOS

Simulator Screen Shot - iPhone 13 - 2022-02-08 at 09 58 44

Android

Screenshot_1644344036

@neil-marcellini neil-marcellini requested a review from a team as a code owner February 8, 2022 18:14
@neil-marcellini neil-marcellini self-assigned this Feb 8, 2022
@MelvinBot MelvinBot requested review from ctkochan22 and removed request for a team February 8, 2022 18:15
@neil-marcellini neil-marcellini requested review from a team and removed request for ctkochan22 February 8, 2022 18:15
@tgolen

This comment was marked as off-topic.

@tgolen

This comment was marked as off-topic.

@sketchydroide
Copy link
Contributor

I'm currently getting this error when building the app
error: No signing certificate "iOS Distribution" found: No "iOS Distribution" signing certificate matching team ID "368M544MTT" with a private key was found. (in target 'NewExpensify' from project 'NewExpensify')
I'll talk with @AndrewGable when he gets online, so I can fix the certificate issue.

@AndrewGable
Copy link
Contributor

The cert and private key should be in 1Password!

@neil-marcellini
Copy link
Contributor Author

Thanks guys, please let me know when you get a chance to test it if it works.

@sketchydroide
Copy link
Contributor

still trying to build that file, the certificate one has been acomplished, but a new hurdle appears... 😓
sh: line 1: 5618 Segmentation fault: 11 xcodebuild -workspace ./ios/NewExpensify.xcworkspace -scheme NewExpensify -destination 'generic/platform=iOS' -archivePath /Users/andrefonseca/Library/Developer/Xcode/Archives/2022-02-09/New\ Expensify\ 2022-02-09\ 17.13.15.xcarchive archive
I'll try to overcome that one tomorrow, sorry

@sketchydroide
Copy link
Contributor

I managed to Archive the app, but it doesn't run in my device, keep getting a This app cannot be installed because its integrity could not be verified
I tried also archiving from Xcode and distributing it manually from there. But got the same result. I think the only step now is for someone else to try, or upload the build to AppStoreConnect and try installing from testflight.
We would need to let people know not to test that one, but it would probably work.

@neil-marcellini
Copy link
Contributor Author

Ok thanks for giving it a shot. I'll request a review from another mobile deployer to see if they can get it to work. If not could you please upload it to TestFlight? Please let me know if I can help with that in any way.

@neil-marcellini neil-marcellini requested a review from a team February 10, 2022 17:00
@sketchydroide
Copy link
Contributor

I don't think that will add anyone else, I asked in #engineering chat for help.

@neil-marcellini
Copy link
Contributor Author

I don't think that will add anyone else, I asked in #engineering chat for help.

Oh right because you are already assigned. Thanks for doing that.

@sketchydroide
Copy link
Contributor

we haven't had any takers to help, if we don't have any until the end of the day I think I'm going to create a test flight build so we can test it.

@Julesssss
Copy link
Contributor

Sorry, I have been meaning to help but not sure if I'll have time today

@sketchydroide
Copy link
Contributor

No worries Jules :)

@sketchydroide
Copy link
Contributor

I'm going to merge this, and then we can test it, if it does not work we can revert it or create a new PR to fix it

@sketchydroide sketchydroide merged commit 75c7986 into main Feb 21, 2022
@sketchydroide sketchydroide deleted the neil-ios-notifications-link branch February 21, 2022 13:50
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@roryabraham roryabraham added the InternalQA This pull request required internal QA label Feb 22, 2022
@MelvinBot
Copy link

Triggered auto assignment to @alex-mechler (InternalQA), see https://stackoverflow.com/c/expensify/questions/5042 for more details.

@alex-mechler
Copy link
Contributor

alex-mechler commented Feb 22, 2022

I don't have an iOS device, asking for someone to test on slack https://expensify.slack.com/archives/C03TQ48KC/p1645560250530479

This isnt internal qa

@roryabraham roryabraham removed the InternalQA This pull request required internal QA label Feb 22, 2022
@roryabraham
Copy link
Contributor

My bad, we actually don't need this to be internalQA. We just need to make sure we use extra caution when testing this on staging, because it wasn't tested as thoroughly as we would've liked locally due to technical limitations building an iOS production build locally.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @sketchydroide in version: 1.1.40-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@kbecciv
Copy link

kbecciv commented Feb 24, 2022

Hello @neil-marcellini! The issue #6079 was still reproducing in our end, push notifications are not opening correct chat.

Image.from.iOS.1.MP4

@roryabraham
Copy link
Contributor

Hmmm, in that case it looked like the drawer was still open, so it's hard to say if it linked to the correct chat. Regardless, I think we should probably revert this and try again. Thoughts?

@sketchydroide
Copy link
Contributor

I think so @roryabraham

@OSBotify
Copy link
Contributor

OSBotify commented Mar 1, 2022

🚀 Deployed to production by @yuwenmemon in version: 1.1.40-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

10 participants