fix: export link regex for use in edit message #540

s-alves10 · 2023-05-19T21:14:48Z

Fixed Issues

$ Expensify/App#18911

Tests

What unit/integration tests cover your change? What autoQA tests cover your change?
ExpensiMark-HTML-test.js. Nothing added because there is no regex change.
What tests did you perform that validates your changed worked?
Make sure that all unit-tests are passed.

QA

What does QA need to do to validate your changes?
This PR has no regex change. Can verify if all unit-tests are passed.
What areas to they need to test for regressions?
Nothing.

github-actions · 2023-05-19T21:15:03Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

s-alves10 · 2023-05-19T21:15:26Z

I have read the CLA Document and I hereby sign the CLA

s-alves10 · 2023-05-19T21:15:35Z

recheck

s-alves10 · 2023-05-19T21:18:36Z

@MariaHCD
Please review this PR asap. This PR needs to be merged in order to complete this PR for issue 18911

aimane-chnaif · 2023-05-19T21:26:17Z

@neil-marcellini and I will be reviewing this.

aimane-chnaif

@s-alves10 your current change itself looks good.
But how will you solve this regression - Expensify/App#18911 (comment)?

s-alves10 · 2023-05-19T21:56:53Z

@s-alves10 your current change itself looks good. But how will you solve this regression - Expensify/App#18911 (comment)?

@aimane-chnaif
We need to share the same regex in editing comment with expensify-common. When we send the comment, we escape the string first and pass the rules. I did the same. When checking if a link exists, escape the editing comment and run the regex. This works fine.

result

18911.mp4

aimane-chnaif · 2023-05-22T04:09:12Z

lib/ExpensiMark.js

@@ -3,6 +3,8 @@ import Str from './str';
 import {MARKDOWN_URL_REGEX, LOOSE_URL_REGEX, URL_REGEX} from './Url';
 import {CONST} from './CONST';

+const MARKDOWN_LINK_REGEX = `\\[([^\\][]*(?:\\[[^\\][]*][^\\][]*)*)]\\(${MARKDOWN_URL_REGEX}\\)(?![^<]*(<\\/pre>|<\\/code>))`;


Move this to Url.js

@aimane-chnaif
I intentionally placed this constant in ExpensiMark.js. This is not the URL regex, is the regex for parsing markdown URL.
Should I still move this regex to Url.js?

Still I suggest Url.js.
There's no non-default exported const or function defined in ExpensiMark.js

@aimane-chnaif
Moved

aimane-chnaif · 2023-05-22T04:10:24Z

@s-alves10 your current change itself looks good. But how will you solve this regression - Expensify/App#18911 (comment)?

@aimane-chnaif We need to share the same regex in editing comment with expensify-common. When we send the comment, we escape the string first and pass the rules. I did the same. When checking if a link exists, escape the editing comment and run the regex. This works fine.

result

can you explain why escape should work?

s-alves10 · 2023-05-22T07:03:01Z

can you explain why escape should work?

@aimane-chnaif
When we escape the string, < and > are escaped to < and >. That's the reason.
And the another reason I did so is to keep the parsing logic same between sending and editing comment.

aimane-chnaif

LGTM!
cc: @neil-marcellini @MariaHCD

s-alves10 · 2023-05-22T16:15:51Z

@neil-marcellini @MariaHCD
This PR needs to be merged in order to complete this PR for issue 18911.

Will you review this PR?

neil-marcellini

Sorry to keep changing my recommendations, I want to make sure this works as well as possible and stays consistent in the future.

How about we just move these two functions from App to expensify-common, then we can simply use them in App?

neil-marcellini · 2023-05-22T20:59:51Z

lib/Url.js

@@ -16,6 +16,7 @@ const LOOSE_URL_REGEX = `((${LOOSE_URL_WEBSITE_REGEX})${URL_PATH_REGEX}(?:${URL_


 const MARKDOWN_URL_REGEX = `(${LOOSE_URL_REGEX}|${URL_REGEX})`;
+const MARKDOWN_LINK_REGEX = `\\[([^\\][]*(?:\\[[^\\][]*][^\\][]*)*)]\\(${MARKDOWN_URL_REGEX}\\)(?![^<]*(<\\/pre>|<\\/code>))`;


This doesn't seem relevant here because it is about links, not just URLs.

Moved to ExpensiMark.js

neil-marcellini · 2023-05-22T21:01:31Z

lib/ExpensiMark.js

-                        `\\[([^\\][]*(?:\\[[^\\][]*][^\\][]*)*)]\\(${MARKDOWN_URL_REGEX}\\)(?![^<]*(<\\/pre>|<\\/code>))`,
-                        'gi',
-                    );
+                    const regex = new RegExp(MARKDOWN_LINK_REGEX, 'gi');


Here's we're building a regex with the 'gi' flags, but in App we're using the 'gm' flags. That's an example of an inconsistency with this approach.

@neil-marcellini
I think gi flag is intentional not to parse multiline link. In App, we'll be using gi flags as well.
Actually, ExpensiMark uses this flag for both link and autolink at the moment.

aimane-chnaif · 2023-05-22T21:19:02Z

@neil-marcellini ok, I don't mind about that refactoring. But how about the main solution?
Do you think escaping html is fine approach?

s-alves10 · 2023-05-23T00:16:29Z

Sorry to keep changing my recommendations, I want to make sure this works as well as possible and stays consistent in the future.

How about we just move these two functions from App to expensify-common, then we can simply use them in App?

@neil-marcellini
I'm sorry but I wasn't able to find any reason to move those functions to expensify-common. Will you tell me the reason why it's better to move those functions here?
And please let me know where you suggest to move those functions to.

MariaHCD · 2023-05-23T08:11:13Z

Looks like @neil-marcellini has got this covered, un-assigning myself as reviewer.

s-alves10 · 2023-05-23T15:39:52Z

@neil-marcellini
Please let me know your thoughts about my comments. I'll make changes according to you.
I hope make this PR merged asap.

neil-marcellini · 2023-05-23T15:41:52Z

@s-alves10 I will look at this within the next hour. Please feel free to message me on NewDot if you want to chat quickly.

s-alves10 · 2023-05-23T16:00:25Z

@s-alves10 I will look at this within the next hour. Please feel free to message me on NewDot if you want to chat quickly.

Please let me know your address on NewDot

neil-marcellini

We started a group DM in NewDot to discuss.

…unctions here

lib/ExpensiMark.js

neil-marcellini

Looking pretty good!

lib/ExpensiMark.js

neil-marcellini

Nice, good to go!

neil-marcellini · 2023-05-23T17:59:34Z

lib/ExpensiMark.js

@@ -3,7 +3,7 @@ import Str from './str';
 import {MARKDOWN_URL_REGEX, LOOSE_URL_REGEX, URL_REGEX} from './Url';
 import {CONST} from './CONST';

-const MARKDOWN_LINK_REGEX = `\\[([^\\][]*(?:\\[[^\\][]*][^\\][]*)*)]\\(${MARKDOWN_URL_REGEX}\\)(?![^<]*(<\\/pre>|<\\/code>))`;
+const MARKDOWN_LINK_REGEX = new RegExp(`\\[([^\\][]*(?:\\[[^\\][]*][^\\][]*)*)]\\(${MARKDOWN_URL_REGEX}\\)(?![^<]*(<\\/pre>|<\\/code>))`, 'gi');


NAB: Add a 'P' to the end of the name so we know it's not just a string MARKDOWN_LINK_REGEXP. Idk, maybe too subtle.

fix: export link regex for use in edit message

9fa31fc

s-alves10 requested a review from a team as a code owner May 19, 2023 21:14

melvin-bot bot requested review from MariaHCD and removed request for a team May 19, 2023 21:15

Merge branch 'main' into fix/issue-18911

ec5c6de

aimane-chnaif reviewed May 19, 2023

View reviewed changes

s-alves10 requested a review from aimane-chnaif May 19, 2023 22:03

aimane-chnaif reviewed May 22, 2023

View reviewed changes

aimane-chnaif mentioned this pull request May 22, 2023

[HOLD for payment 2023-06-02] [$1000] Link disappeared after changing link text to be surrounded with square brackets Expensify/App#18911

Closed

6 tasks

MariaHCD requested a review from neil-marcellini May 22, 2023 06:14

s-alves10 requested a review from aimane-chnaif May 22, 2023 07:21

fix: move MARKDOWN_LINK_REGEX to Url.js

127ab03

aimane-chnaif previously approved these changes May 22, 2023

View reviewed changes

neil-marcellini requested changes May 22, 2023

View reviewed changes

fix: MARKDOWN_LINK_REGEX move back to ExpensiMark.js

2e0bc91

s-alves10 dismissed aimane-chnaif’s stale review via 2e0bc91 May 23, 2023 00:08

MariaHCD removed their request for review May 23, 2023 08:11

s-alves10 requested a review from neil-marcellini May 23, 2023 14:26

neil-marcellini reviewed May 23, 2023

View reviewed changes

sangar-1028 added 2 commits May 23, 2023 12:26

fix: move extractLinksInMarkdownComment and getRemovedMarkdownLinks f…

1d5e6ac

…unctions here

fix: lint error

b8c6e59

aimane-chnaif reviewed May 23, 2023

View reviewed changes

lib/ExpensiMark.js Show resolved Hide resolved

aimane-chnaif previously approved these changes May 23, 2023

View reviewed changes

neil-marcellini requested changes May 23, 2023

View reviewed changes

lib/ExpensiMark.js Outdated Show resolved Hide resolved

lib/ExpensiMark.js Outdated Show resolved Hide resolved

lib/ExpensiMark.js Show resolved Hide resolved

neil-marcellini reviewed May 23, 2023

View reviewed changes

lib/ExpensiMark.js Outdated Show resolved Hide resolved

fix: remove duplicated code related to MARKDOWN_LINK_REGEX

7312a08

s-alves10 dismissed aimane-chnaif’s stale review via 7312a08 May 23, 2023 17:52

neil-marcellini approved these changes May 23, 2023

View reviewed changes

neil-marcellini merged commit 68abe48 into Expensify:main May 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: export link regex for use in edit message #540

fix: export link regex for use in edit message #540

s-alves10 commented May 19, 2023

github-actions bot commented May 19, 2023 •

edited

Loading

s-alves10 commented May 19, 2023

s-alves10 commented May 19, 2023

s-alves10 commented May 19, 2023

aimane-chnaif commented May 19, 2023

aimane-chnaif left a comment •

edited

Loading

s-alves10 commented May 19, 2023 •

edited

Loading

aimane-chnaif May 22, 2023

s-alves10 May 22, 2023 •

edited

Loading

aimane-chnaif May 22, 2023

s-alves10 May 22, 2023

aimane-chnaif commented May 22, 2023

s-alves10 commented May 22, 2023 •

edited

Loading

aimane-chnaif left a comment

s-alves10 commented May 22, 2023 •

edited

Loading

neil-marcellini left a comment

neil-marcellini May 22, 2023

s-alves10 May 23, 2023

neil-marcellini May 22, 2023

s-alves10 May 22, 2023 •

edited

Loading

aimane-chnaif commented May 22, 2023

s-alves10 commented May 23, 2023 •

edited

Loading

MariaHCD commented May 23, 2023

s-alves10 commented May 23, 2023

neil-marcellini commented May 23, 2023

s-alves10 commented May 23, 2023 •

edited

Loading

neil-marcellini left a comment

neil-marcellini left a comment

neil-marcellini left a comment

neil-marcellini May 23, 2023

		@@ -16,6 +16,7 @@ const LOOSE_URL_REGEX = `((${LOOSE_URL_WEBSITE_REGEX})${URL_PATH_REGEX}(?:${URL_


		const MARKDOWN_URL_REGEX = `(${LOOSE_URL_REGEX}\|${URL_REGEX})`;
		const MARKDOWN_LINK_REGEX = `\\[([^\\][](?:\\[[^\\][]][^\\][]))]\\(${MARKDOWN_URL_REGEX}\\)(?![^<]*(<\\/pre>\|<\\/code>))`;

fix: export link regex for use in edit message #540

fix: export link regex for use in edit message #540

Conversation

s-alves10 commented May 19, 2023

Fixed Issues

Tests

QA

github-actions bot commented May 19, 2023 • edited Loading

s-alves10 commented May 19, 2023

s-alves10 commented May 19, 2023

s-alves10 commented May 19, 2023

aimane-chnaif commented May 19, 2023

aimane-chnaif left a comment • edited Loading

Choose a reason for hiding this comment

s-alves10 commented May 19, 2023 • edited Loading

aimane-chnaif May 22, 2023

Choose a reason for hiding this comment

s-alves10 May 22, 2023 • edited Loading

Choose a reason for hiding this comment

aimane-chnaif May 22, 2023

Choose a reason for hiding this comment

s-alves10 May 22, 2023

Choose a reason for hiding this comment

aimane-chnaif commented May 22, 2023

s-alves10 commented May 22, 2023 • edited Loading

aimane-chnaif left a comment

Choose a reason for hiding this comment

s-alves10 commented May 22, 2023 • edited Loading

neil-marcellini left a comment

Choose a reason for hiding this comment

neil-marcellini May 22, 2023

Choose a reason for hiding this comment

s-alves10 May 23, 2023

Choose a reason for hiding this comment

neil-marcellini May 22, 2023

Choose a reason for hiding this comment

s-alves10 May 22, 2023 • edited Loading

Choose a reason for hiding this comment

aimane-chnaif commented May 22, 2023

s-alves10 commented May 23, 2023 • edited Loading

MariaHCD commented May 23, 2023

s-alves10 commented May 23, 2023

neil-marcellini commented May 23, 2023

s-alves10 commented May 23, 2023 • edited Loading

neil-marcellini left a comment

Choose a reason for hiding this comment

neil-marcellini left a comment

Choose a reason for hiding this comment

neil-marcellini left a comment

Choose a reason for hiding this comment

neil-marcellini May 23, 2023

Choose a reason for hiding this comment

github-actions bot commented May 19, 2023 •

edited

Loading

aimane-chnaif left a comment •

edited

Loading

s-alves10 commented May 19, 2023 •

edited

Loading

s-alves10 May 22, 2023 •

edited

Loading

s-alves10 commented May 22, 2023 •

edited

Loading

s-alves10 commented May 22, 2023 •

edited

Loading

s-alves10 May 22, 2023 •

edited

Loading

s-alves10 commented May 23, 2023 •

edited

Loading

s-alves10 commented May 23, 2023 •

edited

Loading