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

Add IOU routes to Universal Links (AASA) #2970

Merged
merged 1 commit into from
May 19, 2021

Conversation

kidroca
Copy link
Contributor

@kidroca kidroca commented May 17, 2021

Details

Updated the AASA (apple-app-site-association) file with configuration for the IOU routes

Fixed Issues

Fixes #2401

Tests

  1. Have the E.cash app installed
  2. Open an IOU link like https://staging.expensify.cash/iou/split/reportID
  3. The link should be handled in-app - it should launch the E.cash app or prompt you to use E.cash to open the link

QA Steps

Same as above

Tested On

I've tested primarily on iOS.
Since I can't test this change locally, I've just verified the other platforms are still working

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

The changes that I used to test this locally were reverted.

Deep.Links.mp4

Android

@kidroca kidroca requested a review from a team as a code owner May 17, 2021 20:56
@MelvinBot MelvinBot requested review from Gonals and removed request for a team May 17, 2021 20:57
@kidroca
Copy link
Contributor Author

kidroca commented May 17, 2021

I couldn't find a way to test this locally from the dev environment

I created a local server where I hosted the AASA file and an html file with a link like https://staging.expensify.cash/iou/split but since my local server is not registered in associated domains here it does not work
image

I can't add my test server to Associated Domains to see if things work in dev, because in order to build I'd have to update the signing configuration, with my development account and then use a different bundle ID and then add this ID in AASA ...
Also you can't just add applinks:localhost in there, it has to be a domain. Most examples use ngrok


I saw Storybook was recently added to the app, and I've allowed myself the liberty to place a "story" that allows you to manually test deep links (and Universal Links)
I've added all the static links in one section
While the dynamic links - that require routing parameters can be added as separate items
You can see how this works from this video:

Deep.Links.mp4

Of course if this doesn't look useful to you it's just my last commit and I can easily drop it


Whatever you decide, updating the staging environment (and prod) with the new AASA file should resolve the issue

One thing I noticed that the provided link in the ticket is missing a reportID parameter - https://staging.expensify.cash/iou/split
maybe that's just the text in the email, but the link needs to look like this in order to work in-app: https://staging.expensify.cash/iou/split/72207410

Here's a sample video with the link including and not including a report ID

Screen.Recording.2021-05-18.at.0.13.32.mov

@kidroca kidroca force-pushed the kidroca/universal-links-IOU branch from cdb6421 to 9aa1590 Compare May 17, 2021 21:20
@Gonals Gonals requested a review from a team May 18, 2021 14:14
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team May 18, 2021 14:14
@Gonals Gonals removed the request for review from jasperhuangg May 18, 2021 14:16
@Gonals
Copy link
Contributor

Gonals commented May 18, 2021

Hey @marcaaron, requesting your review here since I'm not very familiar with IOUs in general

@Gonals Gonals requested a review from marcaaron May 18, 2021 14:17
@Gonals
Copy link
Contributor

Gonals commented May 19, 2021

Hi @kidroca!
Thanks for taking the time to solve this issue! While the story for testing purposes looks interesting, it is certainly outside the scope of the fix, can you remove it from the PR?
That said, if you want, feel free to create a new issue with the idea of adding this story to our code. We'll evaluate it and, if we are interested, you'll be able to re-submit your code in a different PR.

Thanks!

@kidroca kidroca force-pushed the kidroca/universal-links-IOU branch from 9aa1590 to 4965091 Compare May 19, 2021 09:23
@kidroca
Copy link
Contributor Author

kidroca commented May 19, 2021

Thanks, I've extracted it to a separate branch here: kidroca@17a3800

Will post a ticket about it when I get some free time

@kidroca
Copy link
Contributor Author

kidroca commented May 19, 2021

Updated
Reverted the change related to storybook and the PR now contains only the AASA file change
Also I've tested that all platforms are still working as before

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 marcaaron merged commit 53ebcac into Expensify:main May 19, 2021
@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.

@kidroca kidroca deleted the kidroca/universal-links-IOU branch May 26, 2021 08:26
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.

iOS - App option not available when tapping deep link
4 participants