[$1000] Chat - Adding dot to edited link and non edited link creates different result

[lanitochka17]

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. Open the app
2. Send link with multiple slash separator eg: https://staging.new.expensify.com/get-assistance/WorkspaceGeneralSettings
3. Edit the link and add '.' (fullstop) in the end of link part and save
4. Observe the result
5. Again send similar or same multiple slash separated link
6. This time on edit, first add valid character in the end of the link part like letter or number
7. Again edit, remove the character added in step 6 and add '.' fullstop in place of it, observe that now message is displayed differently then step 4

Expected Result:

App should display in similar format if we add character in edited link then in non edited link

Actual Result:

App displays link in different format if we add dot character in edited link then in non edited 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
• Windows / Chrome

Version Number: 1.3.55-7

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

different.format.for.dot.in.edit.and.non.edited.link.mp4

Recording.2936.mp4

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1691601704532849

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011e9f2df956de96c7
• Upwork Job ID: 1693728746118950912
• Last Price Increase: 2023-08-28
• Automatic offers:
• 0xmiroslav | Reviewer | 26648942
• ShogunFire | Contributor | 26648944
• Dhanashree-Sawant | Reporter | 26648946

[melvin-bot]

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

[melvin-bot]

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

[spcheema]

Proposed solution

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

Chat - Adding dot to edited link and non edited link creates different result

What is the root cause of that problem?

The regex for link parsing in the E\Common does not match a url ending with dot (.)

http://localhost:8082/r/7390778945681131

Due that link parsing rule doesn't kicks and autolink rule kicks in which generates output as below.

[http://localhost:8082/r/7390778945681131](http://localhost:8082/r/7390778945681131X.)

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

Updating url is the only solution but before that we need to understand expected behaviour.

Because when a url ending with dot (.) is sent, parsed link doesn't include dot which seems a correct.

What alternative solutions did you explore? (Optional)

Do nothing since adding a dot is not a valid case.

[ShogunFire]

Proposal

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

There is an inconsistency when we add a dot to a url

What is the root cause of that problem?

When we transform the comment in html, we transform the markdown [text](url) to <a href=url>text<a/> here:

https://github.com/Expensify/expensify-common/blob/5ab0cf3280b3d8fb0c100c5eefe5337a24dc2768/lib/ExpensiMark.js#L70-L85

But when we add a dot at the end of the url it's not recognized as a url so this transformation doesn't happen.

Later we transfrom the links that are not already in <a> to <a> here:

https://github.com/Expensify/expensify-common/blob/5ab0cf3280b3d8fb0c100c5eefe5337a24dc2768/lib/ExpensiMark.js#L136C13-L155C15

So in our case we have [url](url.) that is transfromed in this :
[<a href=url>url</a>](<a href=url.>url.</a>) . Note that the . has been added to the second href

The reason for the inconsistency is that if it looks like the user purpusefully wanted to remove a link, we remove it here;

App/src/libs/actions/Report.js

Lines 1067 to 1076 in 64ecc7a

	const handleUserDeletedLinksInHtml = (newCommentText, originalHtml) => {
	const parser = new ExpensiMark();
	if (newCommentText.length >= CONST.MAX_MARKUP_LENGTH) {
	return newCommentText;
	}
	const markdownOriginalComment = parser.htmlToMarkdown(originalHtml).trim();
	const htmlForNewComment = parser.replace(newCommentText);
	const removedLinks = parser.getRemovedMarkdownLinks(markdownOriginalComment, newCommentText);
	return removeLinksFromHtml(htmlForNewComment, removedLinks);
	};

In the first case we pass from url to url., so we remove the link from the text but we only remove one link because the second one has a dot in his href that's why the left part is white while the right part is blue.

In the second case we pass from url1 to url. so we try to remove url1 but it doesn't exist so both links are kept

The weird part is that when we type http://google.com/. the dot is not in the href but if we add a ) after http://google.com/.) , the dot is in the href. I think it's the real bug

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

We can remove the dot if the url ends with a dot, we already do it for the closing parenthesis here:
https://github.com/Expensify/expensify-common/blob/7735de14112a968fd6a4f5af710d2fbaefc8809d/lib/ExpensiMark.js#L451-L454 and that's why the ) is not in the href

If we do that there will still be an inconstistency but I think this inconsistency is expected due to how we detect removed links. Both links will be removed when we pass from url to url. so the text will be all white. This is what we have for other cases of adding something to a link. For example:
Editing google.com like this [google.com](https://google.com is the best Website) results in this:

The same behaviour that we observe with dot also happens for every characters that is a valid path character but that can't be at the end of the path, so we should remove all those characters: $*.+!(,= if they are at the end.

What alternative solutions did you explore? (Optional)

We can also remove the closing parenthesis in the last escapedChar of this line https://github.com/Expensify/expensify-common/blob/7735de14112a968fd6a4f5af710d2fbaefc8809d/lib/Url.js#L7 but I think that´s not a good idea

[MitchExpensify]

This seems to impact links when you send then first before editing. Lets make sure the proposal fixes this in general versus just when edited but I imagine it will

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[ShogunFire]

@MitchExpensify I actually think we have a general problem about escaping the comment before it arrives to the autolink rule, I explained it here: https://expensify.slack.com/archives/C01GTK53T8Q/p1692652863768869

[ShogunFire]

Proposal

Updated

I found the way we did for end parenthesis, we can do the same for dots maybe

[MitchExpensify]

Got it @ShogunFire , cool. Whatever we do here should solve this in general.

[spcheema]

Damm @ShogunFire I was thinking the same, removing dot.

[MitchExpensify]

@0xmiroslav what do you think about @ShogunFire's proposal?

[melvin-bot]

@MitchExpensify, @0xmiroslav Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[MitchExpensify]

#25532 (comment)

@0xmiroslav bump

[0xmiros]

@ShogunFire thanks for the deep research on the root cause and solution. I agree with your analysis.
I think we should go with that solution to remove dot similar to closing parenthesis.
Proposal link: #25532 (comment)
🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

Current assignee @0xmiroslav is eligible for the Internal assigner, not assigning anyone new.

[0xmiros]

Not overdue. PR is awaiting review from @MonilBhavsar

[MonilBhavsar]

I reviewed and left a small comment. Looks good overall!

[melvin-bot]

@ShogunFire, @MonilBhavsar, @MitchExpensify, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[ShogunFire]

@MonilBhavsar There is some change and a question in the PR if you missed it

[MonilBhavsar]

I commented on the PR, thanks!

[melvin-bot]

@ShogunFire, @MonilBhavsar, @MitchExpensify, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@ShogunFire, @MonilBhavsar, @MitchExpensify, @0xmiroslav 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[MitchExpensify]

What do you need to help move this along at this point @0xmiroslav ?

[0xmiros]

Waiting feedback on Expensify/expensify-common#575 (comment)

[melvin-bot]

@ShogunFire, @MonilBhavsar, @MitchExpensify, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[0xmiros]

PR was merged

[melvin-bot]

@ShogunFire, @MonilBhavsar, @MitchExpensify, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick!

[MonilBhavsar]

The PR was merged. Is there a regression and is issue back again? 🤔 cc @0xmiroslav

[melvin-bot]

@ShogunFire, @MonilBhavsar, @MitchExpensify, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick!

[0xmiros]

The production deploy date is same as #28629 which bumped expensify-common repo version with our changes. So Oct 30
Payment date will be Nov 6

[MitchExpensify]

Payment summary:

$250 - @dhanashree-sawant reporter
$500 (50% 9 day delay penalty) - @ShogunFire C
$500 (50% 9 day delay penalty) - @0xmiroslav C+

[MitchExpensify]

Invites sent for payment! https://www.upwork.com/jobs/~011e9f2df956de96c7

[dhanashree-sawant]

Thanks @MitchExpensify, I have accepted the offer.

[MitchExpensify]

Paid @dhanashree-sawant @ShogunFire !

Have you accepted the offer @0xmiroslav ?

[0xmiros]

#25532 (comment)

[MitchExpensify]

Paid, @0xmiroslav ! Do we need a new regression test here to close this out? I don't see the automated BZ step comment 👀

[0xmiros]

We added automated test cases in PR. So that should cover regression test.