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

[kavimuru]

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?

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

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

2023-04-07_09-58-55.mp4

Recording.169.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]

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

[MelvinBot]

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

[MelvinBot]

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

[trjExpensify]

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

[MelvinBot]

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

[Julesssss]

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]

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]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[pubudu-ranasinghe]

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

App/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

Lines 25 to 35 in b041f49

	const isAttachment = Boolean(htmlAttribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]);
	const displayName = lodashGet(props.tnode, 'domNode.children[0].data', '');
	const parentStyle = lodashGet(props.tnode, 'parent.styles.nativeTextRet', {});
	const attrHref = htmlAttribs.href || '';
	const attrPath = lodashGet(Url.getURLObject(attrHref), 'path', '').replace('/', '');
	const hasExpensifyOrigin = Url.hasSameExpensifyOrigin(attrHref, CONFIG.EXPENSIFY.EXPENSIFY_URL) || Url.hasSameExpensifyOrigin(attrHref, CONFIG.EXPENSIFY.STAGING_API_ROOT);
	const internalNewExpensifyPath = (Url.hasSameExpensifyOrigin(attrHref, CONST.NEW_EXPENSIFY_URL) || Url.hasSameExpensifyOrigin(attrHref, CONST.STAGING_NEW_EXPENSIFY_URL))
	&& !CONST.PATHS_TO_TREAT_AS_EXTERNAL.includes(attrPath) ? attrPath : '';
	const internalExpensifyPath = hasExpensifyOrigin
	&& !attrPath.startsWith(CONFIG.EXPENSIFY.CONCIERGE_URL_PATHNAME)
	&& attrPath;

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]

@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]

@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]

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

[MelvinBot]

📣 @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]

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

[MelvinBot]

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

[MelvinBot]

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

• Fix expensify devportal url replacement #17448

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

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.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[MelvinBot]

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] The PR that introduced the bug has been identified. Link to the PR:
• [@Santhosh-Sellavel] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@Santhosh-Sellavel] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@trjExpensify] Determine if we should create a regression test for this bug.
• [@Santhosh-Sellavel] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[Santhosh-Sellavel]

@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]

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]

Settled up with you both!