Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PAYMENT DUE 6/18 - $250][Live Markdown] Multiple italics markdown in a message are not shown correctly #41110

Closed
thienlnam opened this issue Apr 26, 2024 · 50 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@thienlnam
Copy link
Contributor

thienlnam commented Apr 26, 2024

Actual
image

image

Expected
All the phrases in between _message_ should be italicized correctly

cc @tomekzaw

Issue OwnerCurrent Issue Owner: @sobitneupane
@thienlnam thienlnam added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 26, 2024
@thienlnam thienlnam self-assigned this Apr 26, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 26, 2024
Copy link

melvin-bot bot commented Apr 26, 2024

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

Copy link

melvin-bot bot commented Apr 26, 2024

Triggered auto assignment to @slafortune (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.

@skyweb331
Copy link
Contributor

Duplicated issue #40571

@skyweb331
Copy link
Contributor

skyweb331 commented Apr 26, 2024

So this happens because of ExpensiMark module... It is using regex to parse html to markdown but it has its limitation. It is really difficult to parse complex html to markdown using regex only. Using regex is not the perfect solution for future problems.

I had discussion with @ikevin127 for this. https://expensify.slack.com/archives/C01GTK53T8Q/p1713721057329569

So solution is to change the parsing logic. (ie using xml2js)

cc @jjcoffee

@melvin-bot melvin-bot bot added the Overdue label Apr 29, 2024
Copy link

melvin-bot bot commented Apr 29, 2024

@slafortune, @sobitneupane, @thienlnam Whoops! This issue is 2 days overdue. Let's get this updated quick!

@slafortune
Copy link
Contributor

closing duplicate

@melvin-bot melvin-bot bot removed the Overdue label Apr 30, 2024
@sobitneupane
Copy link
Contributor

@slafortune I am not very confident that #40571 will solve this issue. @jjcoffee Will this issue be solved by the selected proposal in #40571?

@skyweb331
Copy link
Contributor

@slafortune @sobitneupane I made a mistake... This is related to ExpensiMark parsing Markdown to HTML, and above duplicated ticket is to parsing HTML to Markdown... So it is not a duplicated issue... I am sorry for this and will submit the proposal now.

@jjcoffee
Copy link
Contributor

jjcoffee commented May 1, 2024

@sobitneupane Yeah this isn't a duplicate as far as I can tell.

@skyweb331
Copy link
Contributor

skyweb331 commented May 1, 2024

Proposal

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

Multiple italics markdown in a message are not shown correctly

What is the root cause of that problem?

ExpensiMark does not parse markdown to HTML correctly for italics.
https://github.com/Expensify/expensify-common/blob/1713f28214f0e7176c4fd13433fb0ea15491ebf9/lib/ExpensiMark.js#L293-L303

This regex does not parse following italics when there is <pre>, <code>, <a>, <mention-user> tags.

And found one more thing,
It does not parse _blank_ to <em>blank</em> because current regex pattern detects _blank as one of target="_blank".

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

My solution is to ignore <pre>,<code>,<a>,<mention-user> tag and its whole contents first, so that any italicizing inside those contents can be passed. And in this case, _blank will be passed as it is a property value of <a>.

I already completed this and passed all tests.
skyweb331/expensify-common@8035d67

What alternative solutions did you explore? (Optional)

n/a

@slafortune slafortune reopened this May 1, 2024
@melvin-bot melvin-bot bot added the Overdue label May 3, 2024
Copy link

melvin-bot bot commented May 7, 2024

@slafortune, @sobitneupane, @thienlnam Huh... This is 4 days overdue. Who can take care of this?

@sobitneupane
Copy link
Contributor

Will review the proposal shortly

@melvin-bot melvin-bot bot removed the Overdue label May 7, 2024
@sobitneupane
Copy link
Contributor

Thanks for your proposal @skyweb331

What will be the result of following input after applying your solution?

@skyweb331
Copy link
Contributor

skyweb331 commented May 8, 2024

Results are here.

image
image

Live editor still does not work and it seems this is duplicated issue - #41107
I checked that issue and this live editor does not work correct even though ExpensiMark converts Markdown to HTML correctly.

@tomekzaw
Copy link
Contributor

tomekzaw commented May 8, 2024

Live editor still does not work and it seems this is duplicated issue

@skyweb331 react-native-live-markdown has its own bundle with ExpensiMark, you'll need to bump expensify-common in react-native-live-markdown as well as rebuild the JS parser

@skyweb331
Copy link
Contributor

@tomekzaw Maybe, because my update is not applied to react-native-live-markdown yet???
Just thought react-native-live-markdown does not working correctly like #41107, which ExpensiMark parses Markdown to HTML correctly but results are not working.

Will you let me know how can I update react-native-live-markdown to use my current ExpensiMark in my node_modules folder???

@anmurali
Copy link

anmurali commented Jun 5, 2024

Will check on this internally and update.

@anmurali anmurali removed the Overdue label Jun 5, 2024
@melvin-bot melvin-bot bot added the Overdue label Jun 5, 2024
@sobitneupane
Copy link
Contributor

expensify-common was bumped and later reverted here due to some issue. I think we should wait for the issue in expensify-common to be resolved before bumping the version.

Copy link

melvin-bot bot commented Jun 10, 2024

@slafortune, @skyweb331, @sobitneupane, @thienlnam Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot melvin-bot bot added the Overdue label Jun 10, 2024
@slafortune
Copy link
Contributor

@sobitneupane looks like this was deployed to production last week.

@sobitneupane
Copy link
Contributor

@skyweb331 expensify-common version is bumped. Can you please retest and verify that the issue is resolved.

@melvin-bot melvin-bot bot removed the Overdue label Jun 11, 2024
@skyweb331
Copy link
Contributor

image
This is working fine on staging.

@sobitneupane
Copy link
Contributor

I can verify that the issue has been resolved.

Screenshot 2024-06-11 at 14 30 52

@skyweb331
Copy link
Contributor

Did you test it on production? I tested it but it does not work on production. Maybe because of cache???

Copy link

melvin-bot bot commented Jun 14, 2024

@slafortune, @skyweb331, @sobitneupane, @thienlnam Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Jun 14, 2024
@sobitneupane
Copy link
Contributor

The issue is not reproducible in production as well.

Screenshot 2024-06-17 at 13 22 17

@melvin-bot melvin-bot bot removed the Overdue label Jun 17, 2024
@skyweb331
Copy link
Contributor

@sobitneupane Do we have to wait for regression period or already passed it?

@slafortune
Copy link
Contributor

The 18th will cover the regression period - so I can pay this out tomorrow.

@slafortune slafortune changed the title [Live Markdown] Multiple italics markdown in a message are not shown correctly [PAYMENT DUE 6/18 - $250][Live Markdown] Multiple italics markdown in a message are not shown correctly Jun 17, 2024
@slafortune
Copy link
Contributor

@skyweb331 I am not able to locate your UpWorks profile - can you please link that here?

@melvin-bot melvin-bot bot added the Overdue label Jun 19, 2024
@skyweb331
Copy link
Contributor

@slafortune
Copy link
Contributor

@slafortune
Copy link
Contributor

@skyweb331 paid $250 via upworks
@sobitneupane owed for C+ $250 via NewDot

@melvin-bot melvin-bot bot removed the Overdue label Jun 19, 2024
@thienlnam thienlnam moved this to Done in Live Markdown Jun 19, 2024
@JmillsExpensify
Copy link

$250 approved for @sobitneupane

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Status: Done
Development

No branches or pull requests

8 participants