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

[HOLD for payment 2023-08-21] [$1000] Split bill, request money and new chat URLs are not deep linked into native app #23643

Closed
2 of 6 tasks
kbecciv opened this issue Jul 26, 2023 · 45 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Jul 26, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Paste below urls into notes or slack app
    https://new.expensify.com/split/new
    https://new.expensify.com/request/new
    https://new.expensify.com/new/chat
  2. Tap the links from an iOS or Android device with NewDot installed.

Expected Result:

The link gets opened in installed native App

Actual Result:

mWeb app gets opened in native browser

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.45.3
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

RPReplay_Final1690357079.MP4
RPReplay_Final1690375791.MP4

Expensify/Expensify Issue URL:
Issue reported by: @aswin-s
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690357368010899

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f3644378786ca7b0
  • Upwork Job ID: 1686245557430329344
  • Last Price Increase: 2023-08-01
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 26, 2023

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Jul 26, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@kbecciv
Copy link
Author

kbecciv commented Jul 26, 2023

Proposal: @aswin-s
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690357368010899

Proposal

Please re-state the problem that we are trying to solve in this issue.

Urls to split bill, request money and new chat pages are not deep linked to native app

What is the root cause of that problem?

Path prefixes for the pages which are '/split', '/request' and '/new' are not added to apple-app-site-association and AndroidManifest files. That's the reason this URL is not recognized as a universal link by the app.

What changes do you think we should make in order to solve the problem?

Add '/split', '/request' and '/new' URL prefixes to both apple-app-site-association file and AndroidManifest.xml

iOS
https://github.com/Expensify/App/blob/main/.well-known/apple-app-site-association#L10
{ "/": "/split/", "comment": "Split Bill" },
{ "/": "/request/
", "comment": "Request Money" },
{ "/": "/new/*", "comment": "New Chat" },

Android
Add below lines to AndroidManifest.xml

https://github.com/Expensify/App/blob/main/android/app/src/main/AndroidManifest.xml#L65

https://github.com/Expensify/App/blob/main/android/app/src/main/AndroidManifest.xml#L78

What alternative solutions did you explore? (Optional)

None (edited)

@hungvu193
Copy link
Contributor

hungvu193 commented Jul 26, 2023

@aswin-s Just a friendly reminder. I think you should also update your proposal for the android manifest.

@aswin-s
Copy link
Contributor

aswin-s commented Jul 26, 2023

Thanks @hungvu193. It got lost while copying from slack thread.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Urls to split bill, request money and new chat pages are not deep linked to native app

What is the root cause of that problem?

Path prefixes for the pages which are '/split', '/request' and '/chat' are not added to apple-app-site-association and AndroidManifest files. That’s the reason this URL is not recognized as a universal link by the app.

What changes do you think we should make in order to solve the problem?

Add '/split', '/request' and '/chat' URL prefixes to both apple-app-site-association file and AndroidManifest.xml

iOS
https://github.com/Expensify/App/blob/main/.well-known/apple-app-site-association#L10

 { "/": "/split/*", "comment": "Split Bill" }, 
 { "/": "/request/*", "comment": "Request Money" }, 
 { "/": "/new/*", "comment": "New Chat" }, 

Android
Add below lines to AndroidManifest.xml

https://github.com/Expensify/App/blob/main/android/app/src/main/AndroidManifest.xml#L65

<data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/split"/> 
<data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/request"/> 
<data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/new"/> 

https://github.com/Expensify/App/blob/main/android/app/src/main/AndroidManifest.xml#L78

 <data android:scheme="https" android:host="staging.new.expensify.com" android:pathPrefix="/split"/> 
 <data android:scheme="https" android:host="staging.new.expensify.com" android:pathPrefix="/request"/> 
 <data android:scheme="https" android:host="staging.new.expensify.com" android:pathPrefix="/new"/> 

What alternative solutions did you explore? (Optional)

None (edited)

@melvin-bot melvin-bot bot added the Overdue label Jul 28, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 31, 2023

@JmillsExpensify Eep! 4 days overdue now. Issues have feelings too...

@JmillsExpensify
Copy link

Hmm, yeah I agree we should be deep-linking to the app if it's installed. Triaging.

@melvin-bot melvin-bot bot removed the Overdue label Aug 1, 2023
@JmillsExpensify JmillsExpensify added the External Added to denote the issue can be worked on by a contributor label Aug 1, 2023
@melvin-bot melvin-bot bot changed the title Split bill, request money and new chat URLs are not deep linked into native app [$1000] Split bill, request money and new chat URLs are not deep linked into native app Aug 1, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 1, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01f3644378786ca7b0

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 1, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 1, 2023

Current assignee @JmillsExpensify is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Aug 1, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts (External)

@allroundexperts
Copy link
Contributor

@aswin-s's proposal works well!

🎀 👀 🎀 C+ reviewed

@melvin-bot
Copy link

melvin-bot bot commented Aug 2, 2023

Triggered auto assignment to @neil-marcellini, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@neil-marcellini
Copy link
Contributor

@aswin-s's proposal works well!

I agree! Should we also add a checklist item to update these files when new routes are added? I think we want all routes to be deep link-able.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 3, 2023

📣 @allroundexperts Please request via NewDot manual requests for the Reviewer role ($1000)

@melvin-bot
Copy link

melvin-bot bot commented Aug 3, 2023

📣 @aswin-s You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot
Copy link

melvin-bot bot commented Aug 3, 2023

📣 @aswin-s We're missing your Upwork ID to automatically send you an offer for the Reporter role.
Once you apply to the Upwork job, your Upwork ID will be stored and you will be automatically hired for future jobs!

@melvin-bot
Copy link

melvin-bot bot commented Aug 28, 2023

@JmillsExpensify, @aswin-s, @neil-marcellini, @allroundexperts Huh... This is 4 days overdue. Who can take care of this?

@neil-marcellini
Copy link
Contributor

Bump @JmillsExpensify for payment.

@melvin-bot melvin-bot bot removed the Overdue label Aug 28, 2023
@aswin-s
Copy link
Contributor

aswin-s commented Aug 28, 2023

Isn't this eligible for speed bonus?

  • PR was raised on the same day as assigned

  • Approved on second day

  • Just final approval took a day extra due to complications in iOS build. And no changes were requested.

@JmillsExpensify Request you to recheck whether this PR is eligible for a speed bonus.

@melvin-bot melvin-bot bot added the Overdue label Aug 30, 2023
@neil-marcellini
Copy link
Contributor

Not overdue by me

@melvin-bot melvin-bot bot removed the Overdue label Aug 31, 2023
@JmillsExpensify
Copy link

@aswin-s Discussed with @neil-marcellini and we both agree that this is eligible for the urgency bonus. As a result, that gives us the following payouts.

@JmillsExpensify
Copy link

@allroundexperts Coming back to the checklist not being relevant, are there any regression tests that we should put in place as a result of this new feature?

@allroundexperts
Copy link
Contributor

I don't think we have any regression tests for opening deep links in native apps. Please correct me if I'm wrong.

@JmillsExpensify
Copy link

I'm not sure either. We'd have to check TestRail. But you agree we should right?

@allroundexperts
Copy link
Contributor

If we already have such tests, then sure.

@JmillsExpensify
Copy link

Can you suggest a test for this? I'll make sure one gets added.

@allroundexperts
Copy link
Contributor

  1. On an iOS device, where Expensify App is installed, paste the following links in the notes app:

https://new.expensify.com/split/new
https://new.expensify.com/request/new
https://new.expensify.com/new/chat

  1. Click on any of the link.
  2. Verify that the link opens up in the Expensify app rather than Safari.

@JmillsExpensify
Copy link

$1,500 approved for payment via NewDot based on my summary above.

@melvin-bot melvin-bot bot added the Overdue label Sep 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 4, 2023

@JmillsExpensify, @aswin-s, @neil-marcellini, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

@JmillsExpensify
Copy link

I'll circle back on this one after we launch a couple of things this week. Everyone should have already been paid out (though let me know if that's not the case).

@melvin-bot melvin-bot bot removed the Overdue label Sep 4, 2023
@aswin-s
Copy link
Contributor

aswin-s commented Sep 5, 2023

@JmillsExpensify I have not received payment for this one yet.

@aswin-s
Copy link
Contributor

aswin-s commented Sep 8, 2023

@JmillsExpensify Gentle Reminder 👆

@melvin-bot melvin-bot bot added the Overdue label Sep 11, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2023

@JmillsExpensify, @aswin-s, @neil-marcellini, @allroundexperts Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@neil-marcellini
Copy link
Contributor

Bump @JmillsExpensify, also DMing on NewDot

@melvin-bot melvin-bot bot removed the Overdue label Sep 13, 2023
@JmillsExpensify
Copy link

Apologies @aswin-s. I don't see a contract from you on the Upwork job above and it's now closed. I've created a new one and just sent you an offer.

@aswin-s
Copy link
Contributor

aswin-s commented Sep 13, 2023

@JmillsExpensify Accepted the offer. Do we need separate contract for reporting bonus?

@JmillsExpensify
Copy link

No need, I'll add that as a bonus on payment.

@JmillsExpensify
Copy link

All set!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

6 participants