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

[$50] Onboarding - Deeplink in concierge chat doesn't navigate to the correct screen within the app #50648

Closed
1 of 6 tasks
lanitochka17 opened this issue Oct 11, 2024 · 44 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 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Oct 11, 2024

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


Version Number: 9.0.48-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): htad26+dfhjkdsghg@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Sign up with a new Gmail account
  3. Select "Manage my team's expenses" on onboarding modal > Select employee amount > Next
  4. Choose any integration > Next > Complete the onboarding
  5. Once logged in navigate to the workspace created > + icon > Submit expense > Manual > Enter any amount > Finish the flow
  6. This will trigger a start of a free trial
  7. Navigate to concierge chat > Click on the hyperlink "#admins" in the "start of a free trial" system message

Expected Result:

The app should generate a link with a staging URL and should redirect to the correct screen within the app

Actual Result:

The app generates a link with a production URL and redirects to production

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
Bug6631539_1728628103931.2024-10-11_07_12_08.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021844750386942511406
  • Upwork Job ID: 1844750386942511406
  • Last Price Increase: 2024-11-01
  • Automatic offers:
    • situchan | Reviewer | 104675132
    • nkdengineer | Contributor | 104675134
Issue OwnerCurrent Issue Owner: @situchan
@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Oct 11, 2024
Copy link

melvin-bot bot commented Oct 11, 2024

Triggered auto assignment to @lakchote (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link

melvin-bot bot commented Oct 11, 2024

💬 A slack conversation has been started in #expensify-open-source

Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@lakchote
Copy link
Contributor

Not reproducible in dev.

I don't think this should be a blocker since when the deploy checklist hits production, the deeplink URL in Concierge chat will be correct.

@lakchote lakchote added External Added to denote the issue can be worked on by a contributor Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Oct 11, 2024
@melvin-bot melvin-bot bot changed the title Onboarding - Deeplink in concierge chat doesn't navigate to the correct screen within the app [$250] Onboarding - Deeplink in concierge chat doesn't navigate to the correct screen within the app Oct 11, 2024
Copy link

melvin-bot bot commented Oct 11, 2024

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

Copy link

melvin-bot bot commented Oct 11, 2024

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

@nkdengineer
Copy link
Contributor

Proposal

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

The app generates a link with a production URL and redirects to production

What is the root cause of that problem?

In here, we are checking has the same origin, it mean in staging we can not open link with the origin new.expensify.com

const hasSameOrigin = Url.hasSameExpensifyOrigin(href, environmentURL);

if (internalNewExpensifyPath && hasSameOrigin) {
if (Session.isAnonymousUser() && !Session.canAnonymousUserAccessRoute(internalNewExpensifyPath)) {
Session.signOutAndRedirectToSignIn();
return;
}
Navigation.navigate(internalNewExpensifyPath as Route);
return;
}

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

We can remove hasSameOrigin in this condition

What alternative solutions did you explore? (Optional)

Screen.Recording.2024-10-12.at.03.51.20.mov

Copy link

melvin-bot bot commented Oct 14, 2024

@lakchote, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Oct 14, 2024
@situchan
Copy link
Contributor

hasSameOrigin check was added on purpose.
@nkdengineer can you please find out PR which added this check and confirm your solution doesn't cause any regression?

@melvin-bot melvin-bot bot removed the Overdue label Oct 15, 2024
@saraSwiti001
Copy link

Please re-state the problem that we are trying to solve in this issue.)
The Concierge chat message for the 7-days trial contains an internal app link that does not take into account the request https origin.

What is the root cause of that problem?

This is a BE problem, the content of this specific automated message is from the backend where it's constructed with an explicit href for the anchor tag that equals to the production url which is new.expensify.com instead of the staging or the development (It happens on both).

The Front-End apps are just displaying what they're getting (which make sense as these messages should be generated on the backend and displayed as is) and for this message we are getting the production url.

The server is not taking into account the origin of the https request instead it's treating all users as if they're coming from production, maybe even this specific message is fixed for all users.

Screenshot 2024-10-15 at 7 32 06 PM

What changes do you think we should make in order to solve the problem?
The server should take into account the origin of the https request and create the dynamic reference links accordingly.

What alternative solutions did you explore? (Optional)
removing hasSameOrigin check, though opening hrefs that are intended for production on none-production environments doesn't sound correct too, we should open the link exactly as we are getting it from the backend, in order to avoid any edge cases, that's why removing hasSameOrigin is not a great idea too. Actually the current behavior makes sense the most, as the link is for the front end app is an "External link" just like any other external url that is being generated during the chat, a link that does not have the same origin as the application, which should be opened externally in a new tab just as is.

One final note to mention is that after this message, neither the user nor the Concierge chat can regenerate the same href style (Imbedding the url within a text) for example writing "GOOGLE" but enabling a redirection to "www.google.com" when pressing the word.

Copy link

melvin-bot bot commented Oct 15, 2024

📣 @saraSwiti001! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@nkdengineer
Copy link
Contributor

@situchan I think we should confirm what is expected here, do we want to be redirected to another origin when we click on a link or we only want to do that with staging environments? cc @lakchote

@lakchote
Copy link
Contributor

@situchan I think we should confirm what is expected here, do we want to be redirected to another origin when we click on a link or we only want to do that with staging environments? cc @lakchote

We should not introduce staging specific logic.

@nkdengineer could you please answer this question from @situchan:

@nkdengineer can you please find out PR which added this check and confirm your solution doesn't cause any regression?

@nkdengineer
Copy link
Contributor

can you please find out PR which added this check and confirm your solution doesn't cause any regression?

@situchan @lakchote it is added in this PR and I confirm that it will not cause any regression as expected in the OP of the issue

@situchan
Copy link
Contributor

@nkdengineer no, it's not. That PR just moved logic from here

@nkdengineer
Copy link
Contributor

@situchan It was first added in this PR. please check again

@situchan
Copy link
Contributor

No viable solution yet

@melvin-bot melvin-bot bot removed the Overdue label Oct 28, 2024
@nkdengineer
Copy link
Contributor

@lakchote Since the report link in the start free trial is the production link, I have two options with two behavior

  1. If we want to open it in the new tab and log in with the correct account if we're in another environment, we can use openExternalLinkWithToken if internalNewExpensifyPath exists and hasSameOrigin is false

openExternalLink(href);

  1. If we want to open it without a new tab we should return the URL with the format %baseURL${internalPath} then frontend can replace it with the current environmentURL

message.html = getReportActionHtml(reportAction)?.replace('%baseURL', environmentURL);

@lakchote
Copy link
Contributor

Since it's a deeplink, redirecting to the correct room without opening a tab is expected.

@situchan what do you think of option #2 from @nkdengineer?

@melvin-bot melvin-bot bot added the Overdue label Oct 30, 2024
@situchan
Copy link
Contributor

Option 2 sounds good to me

@melvin-bot melvin-bot bot removed the Overdue label Oct 30, 2024
@lakchote
Copy link
Contributor

I like option #2 too, @nkdengineer let's go with that.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 31, 2024
Copy link

melvin-bot bot commented Oct 31, 2024

📣 @situchan 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Oct 31, 2024

📣 @nkdengineer 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@nkdengineer
Copy link
Contributor

{
    "actionName": "ADDCOMMENT",
    "actorAccountID": 8392101,
    "avatar": "https://d1wpcgnaa73g0y.cloudfront.net/894b50e60056c966d12216005fbcacec8ce5a2c0.png",
    "created": "2024-10-31 08:32:45.660",
    "lastModified": "2024-10-31 08:32:45.660",
    "message": [
        {
            "type": "COMMENT",
            "html": "<h1>You’ve started a free trial!</h1><br />Welcome to your free 7-day trial of Expensify <emoji>🎉</emoji> Use it to continue exploring your workspace's benefits, including tracking expenses, reimbursing employees, managing company spend, and more.<br /><br />If you have any questions, chat with your dedicated Setup Specialist in <a href=\"https://new.expensify.com/r/8468742852675408\" target=\"_blank\" rel=\"noreferrer noopener\">#admins</a>. Enjoy!",
            "text": "You’ve started a free trial!\n\nWelcome to your free 7-day trial of Expensify 🎉 Use it to continue exploring your workspace's benefits, including tracking expenses, reimbursing employees, managing company spend, and more.\n\nIf you have any questions, chat with your dedicated Setup Specialist in #admins. Enjoy!",
            "isEdited": false,
            "whisperedTo": [],
            "isDeletedParentAction": false,
            "deleted": "",
            "reactions": []
        }
    ],
    "originalMessage": {
        "html": "<h1>You’ve started a free trial!</h1><br />Welcome to your free 7-day trial of Expensify <emoji>🎉</emoji> Use it to continue exploring your workspace's benefits, including tracking expenses, reimbursing employees, managing company spend, and more.<br /><br />If you have any questions, chat with your dedicated Setup Specialist in <a href=\"https://new.expensify.com/r/8468742852675408\" target=\"_blank\" rel=\"noreferrer noopener\">#admins</a>. Enjoy!",
        "isNewDot": true,
        "lastModified": "2024-10-31 08:32:45.660"
    },
    "person": [
        {
            "type": "TEXT",
            "style": "strong",
            "text": "Expensify Concierge"
        }
    ],
    "reportActionID": "4424774399671986900",
    "shouldShow": true,
    "timestamp": 1730363565,
    "reportActionTimestamp": 1730363565660,
    "automatic": false,
    "whisperedToAccountIDs": []
}

@lakchote This will require the BE change that will change the https://new.expensify.com in admin room href to %baseURL

When it's done I will create an FE PR to update replaceBaseURLInPolicyChangeLogAction and accept the concierge action because now we're only doing this logic with policyChangeLogAction

function replaceBaseURLInPolicyChangeLogAction(reportAction: ReportAction): ReportAction {
if (!reportAction?.message || !isPolicyChangeLogAction(reportAction)) {
return reportAction;
}

@lakchote lakchote changed the title [$250] Onboarding - Deeplink in concierge chat doesn't navigate to the correct screen within the app [$50] Onboarding - Deeplink in concierge chat doesn't navigate to the correct screen within the app Nov 1, 2024
Copy link

melvin-bot bot commented Nov 1, 2024

Upwork job price has been updated to $50

@lakchote
Copy link
Contributor

lakchote commented Nov 1, 2024

I've discussed about the issue internally.

Given that's an automated message in concierge for a free trial, it'd really only affect applause and internal employees (even then it'd be an edge case),

We've decided to not fix that because it only affects staging. Though you should be compensated for the work done so far @nkdengineer @situchan, that's why I'm proposing a $50 payment for each of you.

Assigning a BZ member to issue payment, and then I'll close the issue.

@lakchote lakchote added the Bug Something is broken. Auto assigns a BugZero manager. label Nov 1, 2024
Copy link

melvin-bot bot commented Nov 1, 2024

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@lakchote
Copy link
Contributor

lakchote commented Nov 1, 2024

@trjExpensify please see my comment just above, thanks!

@lakchote lakchote added the Awaiting Payment Auto-added when associated PR is deployed to production label Nov 1, 2024
@trjExpensify
Copy link
Contributor

Cool, payment summary as follows:

Let me know when you have accepted the pending offers that have already been sent. Then I'll adjust and settle.

Copy link

melvin-bot bot commented Nov 4, 2024

@trjExpensify, @lakchote, @situchan, @nkdengineer Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Nov 4, 2024
@trjExpensify
Copy link
Contributor

@situchan, paid. @nkdengineer, waiting for you to accept the offer.

@lakchote
Copy link
Contributor

@situchan, paid. @nkdengineer, waiting for you to accept the offer.

^ @nkdengineer did you accept the offer?

@nkdengineer
Copy link
Contributor

@lakchote I just did, thanks

@trjExpensify
Copy link
Contributor

Paid, closing!

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 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

6 participants