[HOLD for Payment 2024-09-09][$250] Wrong parsing when single backticks used in 2 separate places

m-natarajan commented 2 months 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!

Version Number: 9.0.17-0 Reproducible in staging?: y Reproducible in production?: y 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 Expensify/Expensify Issue URL: Issue reported by: @coleaeason Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1722625457122619

Action Performed:

Open any report
Send the string "it uses ; separated fields because the line number column could have : in them"
Expected Result:

it uses;separated fields because the line number column could have:in them

Actual Result:

it uses ; separated fields because the line number column could have :in them

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?
- [ ] Android: Native
- [ ] Android: mWeb Chrome
- [ ] iOS: Native
- [ ] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [ ] MacOS: Desktop

Screenshots/Videos

Screenshot 2024-08-02 at 3 03 21 PM

Snip - (13) New Expensify - Google Chrome (2)

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~014560c8f3601ebcb2
Upwork Job ID: 1823820445986093493
Last Price Increase: 2024-08-14
Automatic offers:

ikevin127 | Reviewer | 103588545
tsa321 | Contributor | 103588546

Issue Owner

Current Issue Owner: @ikevin127

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

tsa321 commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-08-07 07:14:57 UTC.

Proposal

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

Wrong parsing when single backticks with a semicolon character used in 2 separate places

What is the root cause of that problem?

The regex rule for inlineCodeBlock:

https://github.com/Expensify/expensify-common/blob/fea340b9d71bf2415ed1e8745ac60bafb4fc0cfa/lib/ExpensiMark.ts#L193

because of the square bracket in (?![`]) it search for optional character inside the square bracket instead of the back tick character. If inside the code is one of the character in this case ; the result will be incorrect.

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

We could modify the regex to be:


 regex: /(\B|_|)&#x60;((?:&#x60;)*)(?!&#x60;)(.*?\S+?.*?)(?<!&#x60;)((?:&#x60;)*)(&#x60;)(\B|_|)(?!&#x60;|[^<]*<\/pre>|[^<]*<\/video>)/gm,
 replacement: '$1<code>$2$3$4</code>$6',

melvin-bot[bot] commented 2 months ago

@JmillsExpensify Huh... This is 4 days overdue. Who can take care of this?

JmillsExpensify commented 2 months ago

Hmm @thienlnam should we resolve this issue as part of on-going maintenance of our markdown library?

thienlnam commented 2 months ago

We've been making some of these external - cc @BartoszGrajdek Looks like this one can also be external right? Or would you like to take it

BartoszGrajdek commented 2 months ago

Looks like this one can also be external right?

Yes, this is something External contributors can definitely work on 🙌🏻

melvin-bot[bot] commented 2 months ago

Job added to Upwork: https://www.upwork.com/jobs/~014560c8f3601ebcb2

melvin-bot[bot] commented 2 months ago

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

ikevin127 commented 2 months ago

♻️ Reviewing proposal.

ikevin127 commented 2 months ago

@tsa321 Thanks for the proposal, noticed quite a few edits there 😅 Unfortunately even though the regex you suggested seems to fix the issue according to the issue's Expected result, it fails the following :x: 6 tests in ExpensiMark-HTML-test.js:

:x: Test inline code blocks with triple backticks
:x: Test inline code with one backtick as content
:x: Test inline code with multiple backtick symbols as content
:x: Test for codefence with no content
:x: Video markdown conversion to html tag › inline code in alt
:x: Video markdown conversion to html tag › blockquote in alt

Besides the failing tests, which make the proposed solution unacceptable, I also noticed that given the following input:

it uses`;`separated fields because the line number column could have`:`in them ` ` with space

Once sent, the parsing will have the message look like this:

causing white-space (empty space) inline code-block to not be parsed and instead showing the back-ticks instead of an empty code-block.

If still interested in fixing the issue I suggest cloning the expensify-common repository and running the tests as you iterate and try to come-up with a regex that would fix our issue while not breaking any of the tests (without adjusting the tests to "match" the new regex).

tsa321 commented 2 months ago

@ikevin127 Regarding the test: it uses `;`separated fields because the line number column could have`:`in them ` ` with space

causing white-space (empty space) inline code-block to not be parsed and instead showing the back-ticks instead of an empty code-block.

I’ve updated the regex to: regex: /(\B|_|)`(.*?(?![`])[\S| ].*?)`(\B|_|)(?!`|[^<]*<\/pre>)/gm

This will renders inline code with spaces correctly when I send the message. However, it fails the ExpensiMark-HTML-test.js test:

Test for backticks with no content. The expected result for this test is that inline code blocks with only space should not be parsed.

tsa321 commented 2 months ago

Proposal

@ikevin127 I have updated my proposal. The regex and replacement has been updated and now passes all tests.

ikevin127 commented 2 months ago

@tsa321's updated proposal looks good to me, all expensify-common tests are 🟢 Passing. The RCA is correct and the proposed solution fixes the issue as per the Expected result.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

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

ikevin127 commented 2 months ago

🗒️ Some things to note

[!important] These are out of scope for our current issue since they are present on current staging as well.

1. White-space (empty space) inline code-block showing the back-ticks instead of an empty code-block

Given the following input:

it uses`;`separated fields because the line number column could have`:`in them ` ` with space

Once sent, the parsing will have the message look like this:

causing white-space (empty space) inline code-block to not be parsed and instead showing the back-ticks instead of an empty code-block.

2. Nested inline code-block parsing is broken (compared to GitHub's for example)

Given the following input:

```nest 1 `nest 2` nest 1```

Once sent, the parsing will have the message look like this:

For reference this is how GitHub is parsing it:

cc @BartoszGrajdek

dangrous commented 2 months ago

Do we want to open separate issues for those two bugs you noted? Not sure if they exist already.

For this one, the proposal sounds good!

melvin-bot[bot] commented 2 months ago

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 2 months ago

📣 @tsa321 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer 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 📖

tsa321 commented 2 months ago

@ikevin127 PR is ready.

melvin-bot[bot] commented 2 months ago

@JmillsExpensify @dangrous @ikevin127 @tsa321 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

ikevin127 commented 2 months ago

Reviewing PR today!

ikevin127 commented 2 months ago

🟢 expensify-common PR was reviewed, more details in https://github.com/Expensify/expensify-common/pull/785#issuecomment-2299909099. 🔖 Will complete the PR Reviewer Checklist on the E/App PR once the expensify-common PR is merged.

dangrous commented 2 months ago

e-c PR is merged!

BartoszGrajdek commented 2 months ago

@ikevin127 sorry for the late response to this comment - it got buried under too many GH notifications 😅

I agree with the 1st note, this is probably something we could think about improving.

As for the 2nd note this is actually something we had a lengthy discussion about right here - let me know if you have access to it. There's no support for an "inline" code block (triple backticks) if you wish to create a code block linebreaks are required between backticks and the content itself.

dangrous commented 2 months ago

Thanks for the input @BartoszGrajdek!

Do you (both) think it's worth it for me to create an issue to look into the first note and fix it? Seems pretty minor but I guess it can't hurt

BartoszGrajdek commented 2 months ago

@dangrous I think we can create one, but as you said it looks like a pretty minor bug.

This is something that we can open to proposals from external contributors. It looks like it only requires some changes in expensify-common (ExpensiMark.ts specifically)

ikevin127 commented 2 months ago

⚠️ Production deploy automation failed here -> this should be on [HOLD for Payment 2024-09-09] according to yesterday's production deploy from comment.

cc @dangrous @JmillsExpensify

ikevin127 commented 1 month ago

cc @JmillsExpensify

ikevin127 commented 1 month ago

cc @JmillsExpensify bump for payment

ikevin127 commented 1 month ago

cc @JmillsExpensify bump for payment

JmillsExpensify commented 1 month ago

Payment summary:

Contributor: @tsa321 $250
Reviewer: @ikevin127 $250

JmillsExpensify commented 1 month ago

@ikevin127 do we have a BZ checklist for this issue yet?

ikevin127 commented 1 month ago

Not sure whether regression test is needed for this one because on expensify-common PR https://github.com/Expensify/expensify-common/pull/785 we added unit tests, but anyway.

[x] [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/expensify-common/pull/690
[x] [@ikevin127] 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: https://github.com/Expensify/expensify-common/pull/690/files#r1757521200
[x] [@ikevin127] 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: N/A.
[x] [@ikevin127] Determine if we should create a regression test for this bug.
[x] [@ikevin127] 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.

Regression Test Proposal

Open any report.
Send the following message:
```
test `;` test `:` test
```
Verify that the message is displayed correctly as:

test ; test : test

cc @JmillsExpensify

Do we agree 👍 or 👎.

JmillsExpensify commented 1 month ago

Contributors paid via Upwork and regression test created. I'm closing this one.

Expensify / App