Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.51k stars 2.86k forks source link

[HOLD for payment 2023-04-26] [$1000] Production Expensify LogSearch links always direct to staging LogSearch when clicked on staging #17178

Closed kavimuru closed 1 year ago

kavimuru commented 1 year ago

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. Send a logsearch link for production logsearch (Ie https://www.expensify.com/_devportal/tools/logSearch/#sort=asc&size=10000&query=blob%3A%22%20Insufficient%20privileges%20for%20token%20type%20receiptUpload%20and%20method%20SetAssignedGuide%22%20AND%20timestamp%3A%5B2023-03-20T00%3A00%20TO%202023-03-31T23%3A59%5D)
  2. Hover the link
  3. Click the link from staging web or desktop

Expected Result:

The user should be taken to the exact link that is shown, and displayed when hovering the link

Actual Result:

The user is sent to the staging version of the same link

Workaround:

unknown

Platforms:

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

Version Number: 1.2.97-3 Reproducible in staging?: y Reproducible in production?: 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

https://user-images.githubusercontent.com/43996225/230699814-4b4b098c-2ae9-4ade-8c56-691ecfab47db.mp4

https://user-images.githubusercontent.com/43996225/230699819-2e05df93-55a8-4551-8aea-13255fb43fef.mp4

Expensify/Expensify Issue URL: Issue reported by: @alex-mechler Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1680887189111179

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01688c505ff837c0a7
  • Upwork Job ID: 1645813241668116480
  • Last Price Increase: 2023-04-11
MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

MelvinBot commented 1 year ago

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

trjExpensify commented 1 year ago

@alex-mechler would this be an internal issue because it touches our internal logSearch feature? Adding Engineering for now to get more eyes.

MelvinBot commented 1 year ago

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

Julesssss commented 1 year ago

would this be an internal issue because it touches our internal logSearch feature? Adding Engineering for now to get more eyes.

Maybe, but I wonder if this is some logic in NewDot that is replacing the domain. Especially because this is only occurring when you're on the staging env.

Update: this doesn't occur for https://www.expensify.com/, but does for https://www.expensify.com/_devportal/. So I'm starting to think this is in fact an internal issue, that will require changes in Web-E.

alex-mechler commented 1 year ago

I think this can be external. Its likely related to the url swapping we do for attachments. https://github.com/Expensify/App/blob/main/src/libs/tryResolveUrlFromApiRoot.js

If I click on the same link from the same message on production, it properly redirects to production.

trjExpensify commented 1 year ago

Ah nice okay, cool. Shipping on to external then! :)

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

pubudu-ranasinghe commented 1 year ago

Proposal

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

Links directing to log search are not directing to their intended target

What is the root cause of that problem?

When rendering anchor tags there are some conditions evaluated to decide how the link is opened. This is defined in AnchorRenderer

https://github.com/Expensify/App/blob/b041f49a4ea01913e867b0d67d22b17184550188/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js#L25-L35

  1. If the url is an expensify url and has pattern newdotreport?reportID= this is opened in the app internally
  2. If url has new expensify origin it is opened in the app internally
  3. If it's an expensify url but don't match above criteria it's assumed to be an old dot url and opened in a new tab/externally
    • This calls openOldDotLink and replaces the domain with respective old dot domains
  4. All other links are opened normally

So when the url for logs https://www.expensify.com/_devportal... is posted, the conditions evaluate this to be an old dot url and replaces the domain.

When user is in staging.new.expensify.com expensify.com --> staging.expensify.com

When user is in localhost expensify.com --> expensify.com.dev

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

I think the current conditions should be kept as it is because several scenarios are already handled successfully. In addition to that we should add a simple check to pass through _devportal links. Already there is a similar condition which ignores concierge/ urls

We simply need to extend this to include && !attrPath.startsWith('_devportal')

What alternative solutions did you explore? (Optional)

We could rework the logic to handle internal urls better so that any other expensify url similar to _devportal can pass through. But I do not have enough knowledge in the matter on what kind of other internal links are expected, if they are expected at all.

Santhosh-Sellavel commented 1 year ago

@Julesssss Seems the above proposal is on the right track here, let me know your thoughts

Not sure why we make changes to old dot production URLs

ie. from expensify.com --> staging.expensify.com

pubudu-ranasinghe commented 1 year ago

@Santhosh-Sellavel

Given the user is viewing NewDot Staging, ie staging.new.expensify.com And a link is posted which is supposed to be opened in OldDot. Something like www.expensify.com/exists_in_old_dot

In this case we would want to direct the user to the same environment of OldDot that the user is currently on NewDot rather than sending them directly to original url which is in production.

So www.expensify.com/exists_in_old_dot is replaced with www.staging..expensify.com/exists_in_old_dot

Julesssss commented 1 year ago

Thanks for the detailed proposal! I think the exception for devtools makes the most sense 👍

MelvinBot commented 1 year ago

📣 @pubudu-ranasinghe You have been assigned to this job by @Julesssss! Please apply to this job in Upwork 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 📖

pubudu-ranasinghe commented 1 year ago

Thanks. Applied in Upwork and will post a PR within the day

MelvinBot commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

MelvinBot commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.1-3 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-04-26. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

MelvinBot commented 1 year ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

Santhosh-Sellavel commented 1 year ago

@Julesssss I think we can skip the checklist here N/A. Since this is not a regression introduced anywhere, also we won't need a regression test as well! Do you agree?

cc: @trjExpensify

trjExpensify commented 1 year ago

Yup, I agree with that sentiment. Alright, merge happened within 3 days (accounting the weekend).

Payments due by my calculation 🤓 🧑‍🏫

$1,500 to @pubudu-ranasinghe for the fix $1,500 to @Santhosh-Sellavel for C+ $0 to @alex-mechler for reporting the bug 😛

Sent offers on Upwork!

trjExpensify commented 1 year ago

Settled up with you both!