[HOLD for payment 2022-07-15] [$250] When sending text that NewDot thinks is a link, it adds the link markdown automatically and doesn't allow to edit

[mvtglobally]

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 any chat
2. send safe.directory/link message
3. Edit the message to remove the link

Expected Result:

User would expect it to save the text as-is and be able to remove the link

Actual Result:

App won't let user to remove the link. If I remove the [] and (http://safe.directory/) from the message and hit Save, it adds them back.

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.1.63-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Recording.453.mp4

Expensify/Expensify Issue URL:
Issue reported by: @rafecolton
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1652808490521729

View all open jobs on GitHub

[melvin-bot]

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

[luacmartins]

I think this can be external

[melvin-bot]

Triggered auto assignment to @kadiealexander (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[kadiealexander]

Posted to Upwork!

https://www.upwork.com/ab/applicants/1527523209823019008/job-details
https://www.upwork.com/jobs/~012f7992db0fb8f099

[melvin-bot]

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

[melvin-bot]

Current assignee @luacmartins is eligible for the Exported assigner, not assigning anyone new.

[NehaSantani]

Screen.Recording.2022-05-20.at.12.46.35.PM.mov

Hi,
I would like to work on this issue!
The text won't be converted to link automatically!

We can skip the parsing

const parser = new ExpensiMark();

if its in editMode!
Is this the expected behaviour?

[luacmartins]

@NehaSantani Thanks for the proposal. However, I think that skipping the parser would affect other styles as well (code blocks, quotes, etc) and we wouldn't want that. Let me know if that doesn't answer your question and I can try to clarify any remaining concerns.

[NehaSantani]

@luacmartins Hi,
I want a bit clarification on this issue!

• 1.) Do you want to remove the hyperlink styling on text after edit?
  Or
• 2.) you want to avoid mdText that is being displayed in input while edit is clicked.

If its a yes for 2 :
Then My Proposed solution to this problem is use parser.htmlToText instead of parser.htmlToMarkdown !
Is there any specific reason we are using below line in ReportActionItemMessageEdit.js file at line no - 77

        const draftMessage = parser.htmlToMarkdown(this.props.draftMessage);

Instead we can use

        const draftMessage = parser.htmlToText(this.props.draftMessage);

And if its a yes for 1 : I will propose another solution for it soon!

Have attached the output for 2!

Screen.Recording.2022-05-20.at.10.34.49.PM.mov

[luacmartins]

Hey @NehaSantani! Thanks for the questions.

I think the intention is 1. We want to be able to:

1. Edit the comment
2. Remove the link md
3. When we save the edited comment, the link should be display as text (since we removed the link md).

What we have right now is that after editing, the link md is added back and the link is shown as a link instead of text.

[allroundexperts]

Proposal

We should change the following:

App/src/libs/actions/Report.js

Lines 1391 to 1393 in 56213d0

	function editReportComment(reportID, originalReportAction, textForNewComment) {
	const parser = new ExpensiMark();
	const htmlForNewComment = parser.replace(textForNewComment);

to

function editReportComment(reportID, originalReportAction, textForNewComment) {
    const parser = new ExpensiMark();
    const htmlForNewComment = parser.replace(textForNewComment, ["autoLink"]);

This will not run the link rule of the parser. We'll also need to change the expensify-common library so that the parser.replace function accepts a second argument with an array of rules to skip.

Following changes:
https://github.com/Expensify/expensify-common/blob/237fb782678fec9542d0a2608ea18ef2302995fd/lib/ExpensiMark.js#L297-L301
to

    replace(text, skippedRules = []) {
        // This ensures that any html the user puts into the comment field shows as raw html
        let replacedText = Str.safeEscape(text);


        this.rules.filter((rule) => !skippedRules.includes(rule.name)).forEach((rule) => {

[luacmartins]

@allroundexperts the library can be found here and I believe you have access to it. Let me know if that's not the case.

[allroundexperts]

@luacmartins I can access it. I'll update my proposal.

[allroundexperts]

@luacmartins Updated.

[metehanozyurt]

Cause

We cant detect url changes.

Solution

If the original text is changed but the output is not affected, we can ignore the link conversion.
It is active only if the link has been updated and is the same as the old one.
When we add this code to line 1394, we can achieve this. let htmlForNewComment of course.

App/src/libs/actions/Report.js

Lines 1391 to 1394 in 56213d0

	function editReportComment(reportID, originalReportAction, textForNewComment) {
	const parser = new ExpensiMark();
	const htmlForNewComment = parser.replace(textForNewComment);

if (htmlForNewComment === originalReportAction.message[0].html && textForNewComment === originalReportAction.message[0].text) {
        //alert('link not present anymore and same text show up');
        parser.rules = parser.rules.map(rule => rule.name !== 'link');
        htmlForNewComment = parser.replace(textForNewComment);
    }

link-issue-Screen-Recording-2022-05-20-at-23.04.44.mp4

note : I just realized that he mentioned this approach actually @allroundexperts .
It would be nicer if it was detected and done in this way, in my humble opinion.

You can think of it as if I didn't do proposal @luacmartins 😅 . I wrote it so that maybe I can contribute to the solution 👼 .

[NehaSantani]

@NehaSantani Thanks for the proposal. However, I think that skipping the parser would affect other styles as well (code blocks, quotes, etc) and we wouldn't want that. Let me know if that doesn't answer your question and I can try to clarify any remaining concerns.

@luacmartins As directed by you, have skipped only "autolink" style while editing and not affecting other styles!
However to achieve this, we need to update

• ExpensiMark.js library file with following code

//Previously called method
   replace(text) {return this.replaceSkipRules(text,[]);}
  
  // Updated method
   replaceSkipRules(text,skipRules) {
        // This ensures that any html the user puts into the comment field shows as raw html
        let replacedText = Str.safeEscape(text);

        this.rules.forEach((rule) => {

            if(skipRules.length && skipRules.includes(rule.name))
            return true;
            
            // Pre-process text before applying regex
            if (rule.pre) {
                replacedText = rule.pre(replacedText);

• Call above method from Report.js as follows

function editReportComment(reportID, originalReportAction, textForNewComment) {
  const parser = new ExpensiMark();
  const htmlForNewComment = parser.replaceSkipRules(textForNewComment,["autolink"]);

Proposed Output :

issue-record.mov

[mananjadhav]

@allroundexperts Do you have any updates for me on this one?

[allroundexperts]

@mananjadhav Updated the PR.

[melvin-bot]

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

• fix: autolink on edit #9405

If no regressions arise, payment will be issued on 2022-07-15. 🎊

[JmillsExpensify]

Note: Please hold payment on this regression.

[luacmartins]

@allroundexperts could you please look into a fix for that regression?

[allroundexperts]

@luacmartins What's the expected behaviour?

[luacmartins]

The expected behavior is as follows:

1. User sends a message with a link
2. User edits the message
3. If the user intentionally removes the hyperlink in their edit, i.e. from [text](link.com) to link.com markdown, we should not autolink because the user wanted to remove that hyperlink
4. If the link still has the hyperlink markdown when the user saves the edit, i.e. they edit something and the link markdown is still [text](link.com), we should keep the hyperlink as the user did not want to intentionally remove it

Does that make sense?

[mananjadhav]

Coming from your comment here @luacmartins.

If you carefully observe the video,

edit-link-render.mov

editing some links doesn't show up [text](link.com) syntax. It only shows the http://link.com text. Hence my confusion on resolving this. You'll see google.com works just fine, because editing it shows [google.com](google.com) but https://mail.google.com doesn't show the markdown syntax.

[mdneyazahmad]

I do not think this is an issue we should fix, as we are supporting auto-link in the initial message, the user expects it to work on the edit message too.

[text](URL) and URL are the same thing

Tested on Github it also has the same behavior.

[luacmartins]

@mananjadhav Thanks for clarifying! It makes sense now and it seems like the issue is that we are not adding the markdown syntax to those links when editing them, i.e. https://mail.google.com should show up as [https://mail.google.com](https://mail.google.com) when we tap edit, but it shows up as https://mail.google.com.

In that case I agree that it is not a regression, but it would be nice to fix that and add the markdown to those links. Let's move this conversation back to the original issue to work on a fix there.

[luacmartins]

@kadiealexander this is not a regression and we should proceed with payment as usual.

[kadiealexander]

Thanks @luacmartins! Will pay tomorrow as expected.

[kadiealexander]

@allroundexperts - Paid! Thanks for your work.
@mananjadhav - just sent you an Upwork contract. Could you please let me know when you accept?

[kadiealexander]

All paid! Thanks team!

[melvin-bot]

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.