[$1000] # header text are not allowed in quotes

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open the app
2. Open any report
3. send messages with quote and header eg: > #test

Expected Result:

App should display text as header inside quotes

Actual Result:

App does not display text as header inside quotes

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [x] Android / native
• [x] Android / Chrome
• [x] iOS / native
• [x] iOS / Safari
• [x] MacOS / Chrome / Safari
• [x] MacOS / Desktop

Version Number: v1.2.99-4 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 Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/43996225/231619689-3bf41922-18f9-48d9-92fc-56d0a65d4ed2.mp4

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681295844961719

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014df709809bca6160
• Upwork Job ID: 1647967484618698752
• Last Price Increase: 2023-04-17

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] 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
• [x] 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.
• [x] 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.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[maddylewis]

@kavimuru - will you include an example of how you expect this text to display? thx!

[Victor-Nyagudi]

Proposal

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

# header text are not allowed in quotes

What is the root cause of that problem?

The current regex expression for headings in ExpensiMark.js only searches for a # at the beginning of the string input.

//...
{
    name: 'heading1',
    regex: /^# +(?! )((?:(?!<pre>).)+)\s*/gm,
    replacement: '<h1>$1</h1>',
},
//...

The string input begins with > - the character entity for > - when writing a heading inside a chat comment's quote, therefore, we don't get a match.

(I would've used a permalink, but this file is inside the node_modules folder that is included in the .gitignore)

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

The current regular expression passes # heading to ExpensiMark.js's modifyTextForQuote() method instead of <h1>heading</h1>.

The final HTML passed to RenderHTML ends up being:

<blockquote># heading</blockquote>

https://github.com/Expensify/App/blob/4607dcb69b6d5bb7607f652f8a78327c7e002183/src/components/RenderHTML.js#L15-L23

Screenshot of props passed to RenderHTML

What needs to be done is:

1. Change heading1's regular expression to capture a # or a # only when it is preceded by > (modifyTextForQuote() method relies on > to wrap the text passed inside a <blockquote> ) at the beginning of the string input.

{
    name: 'heading1',
    regex: /(?:(?<=^&gt; )#|(?<=^)#) +(?! )((?:(?!<pre>).)+)\s*/gm,
    replacement: '<h1>$1</h1>',
},

2. Move the heading1 rule above the quote rule in ExpensiMark.js so that it's applied first and then the formatted string - the one wrapping text inside <heading> tags (tags included) - is passed to the modifyTextForQuote() method for final formatting.

This makes things consistent too because the other rules such as strikethrough, italic, etc. are above quote thus are applied first inside the forEach loop in ExpensiMark.js's replace() method.

//...
{
    name: 'strikethrough',
    regex: /\B~((?=\S)((~~(?!~)|[^\s~]|\s(?!~))+?))~\B(?![^<]*(<\/pre>|<\/code>|<\/a>))/g,
    replacement: '<del>$1</del>',
},
{
    name: 'heading1',
    regex: /(?:(?<=^&gt; )#|(?<=^)#) +(?! )((?:(?!<pre>).)+)\s*/gm,
    replacement: '<h1>$1</h1>',
},
{
    name: 'quote',

    // We also want to capture a blank line before or after the quote so that we do not add extra spaces.
    // block quotes naturally appear on their own line. Blockquotes should not appear in code fences or
    // inline code blocks. A single prepending space should be stripped if it exists
    process: (textToProcess, replacement) => {
        const regex = new RegExp(
                        /\n?^&gt; ?(?![^<]*(?:<\/pre>|<\/code>))([^\v\n\r]+)\n?/gm,
        );
        return this.modifyTextForQuote(regex, textToProcess, replacement);
    ,
    replacement: g1 => `<blockquote>${g1}</blockquote>`,
},
//...

Web demo of heading inside quotes

https://user-images.githubusercontent.com/79470910/232201714-f5839d19-a775-40f0-968d-6f6f905ed0a7.mov

Mobile demo of heading inside quotes

https://user-images.githubusercontent.com/79470910/232205556-e6a702ff-a491-4e09-90a1-d56e37249f68.mov

[Victor-Nyagudi]

Sorry, but it has come to my attention I missed the part in the README.md that says proposals made before the Help Wanted label is applied will not be reviewed.

I initially intended to delete my proposal after learning about this, but if it helps the team at Expensify make a better product, then feel free to use its content however you please.

If it instead adds no value and clutters up the issue, I'll get rid of it.

Your call.

[MelvinBot]

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

[maddylewis]

hiya @pecanoro - i asked @kavimuru a clarifying question but did not get a response. in the meantime, @Victor-Nyagudi added a proposal on how to fix this bug.

do you think we should we move forward with that proposal? lmk - thanks!

[pecanoro]

I tested this and it seems other markdown is kept as a quote, so applying external:

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~014df709809bca6160

[MelvinBot]

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

[MelvinBot]

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

[tienifr]

Proposal

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

send messages with quote and header (> #test) doesn't display text as header inside quotes

What is the root cause of that problem?

We have two rules about quote and heading https://github.com/Expensify/expensify-common/blob/be625f55058111793bbcfe3f64788ea0a8f9d705/lib/ExpensiMark.js#L158-L176 after executing the quote rule our text will be <blockquote>#test</blockquote>, that why it doesn't match with heading regex

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

We should create the new rule for this case when the header inside quote like:

{
name: 'heading1 inside quote',
regex: /^(<blockquote>#)(?!#)(.+)(<\/blockquote>)$/gm,
replacement: '<blockquote><h1>$2</h1></blockquote>',
},

and place it behind quote rule.

If we just do that

1. our commentText will be <blockquote><h1>test</h1></blockquote>
2. We have the logic to parse html to text in

https://github.com/Expensify/App/blob/42bea4bd7bd18231f7b5fb5dfd94ca5b58fe1c2b/src/libs/ReportUtils.js#L936

=> textForNewComment will be \ntest\n because of these rules

https://github.com/Expensify/expensify-common/blob/be625f55058111793bbcfe3f64788ea0a8f9d705/lib/ExpensiMark.js#L296-L318

3. we use textForNewComment to build lastMessageText in optimisticReport

https://github.com/Expensify/App/blob/42bea4bd7bd18231f7b5fb5dfd94ca5b58fe1c2b/src/libs/actions/Report.js#L206 4. after calling formatReportLastMessageText function the lastMessageText will be empty because of this line:

https://github.com/Expensify/App/blob/42bea4bd7bd18231f7b5fb5dfd94ca5b58fe1c2b/src/libs/ReportUtils.js#L480

=> The last message text in LHN shows wrong value

I suggest we should check if the lastMessageText is wrap by \n then returns the inside text:

if(/^\n.*\n$/.test(lastMessageText)){
    return String(lastMessageText).replace(/\n/g,'')
}

Result

https://user-images.githubusercontent.com/113963320/231748227-185327e8-a756-4721-9fec-19d91d0e7d7f.mov

[MelvinBot]

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

[pecanoro]

@Santhosh-Sellavel It seems we have a couple of proposals already 😄

[Victor-Nyagudi]

I haven't made any changes to my first proposal, but given I submitted it before the Help Wanted label was applied, I'd like to re-submit it as a new one so that it may be reviewed.

Apologies for any inconveniences caused.

Proposal

Updated

[Santhosh-Sellavel]

Will look tomorrow!

[romulo114]

Proposal

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

The app doesn't display # header as header when it's inside the quote.

What is the root cause of that problem?

As @tienifr mentioned, we have two rules related to quote and heading https://github.com/Expensify/expensify-common/blob/be625f55058111793bbcfe3f64788ea0a8f9d705/lib/ExpensiMark.js#L158-L176

The main reason of this issue is in regex of the heading rule: It looks for header sign(#) at the beginning of the line, which is not in our case.

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

Heading elements need to be placed at the beginning of the line in the markdown, but there are some exceptions including blockquote. In order to solve this issue, we should deal with these exceptions.

Since heading rule is processed after the quote rule, heading rule would process <blockquote># heading 1</blockquote>, which is complex. So I suggest to swap the order of quote and heading rule. Then heading rule would process > # heading 1, which is simple.

I've changed the rules as belows:

    {
        name: 'heading1',
        regex: /^((?:\&gt; +(?! ))?)# +(?! )((?:(?!<pre>).)+)\s*/gm,
        replacement: '$1<h1>$2</h1>',
    },
    {
        name: 'quote',
        ...
    }

Result

Web https://user-images.githubusercontent.com/117526965/232671350-5a474053-d609-450f-99ea-473265e7ab33.mp4

Mobile https://user-images.githubusercontent.com/117526965/232671371-9ed33008-57ca-4fb0-889a-14b49d3de8ef.mp4

What alternative solutions did you explore? (Optional)

I've found that numbering list should be displayed like blockquote. For instance,

1. # heading

Of course, other elements might be possible. I think we should explore all potential issues and handle all of them. We can expand our rule for hading to achive this.

[MelvinBot]

📣 @romulo114! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[romulo114]

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~012c1394308d6300f4

[MelvinBot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[Santhosh-Sellavel]

@Victor-Nyagudi proposal here looks good to me, your thoughts @pecanoro

[tienifr]

@Santhosh-Sellavel what about my proposal above: https://github.com/Expensify/App/issues/17367#issuecomment-1511447413. Even though my proposal fix the main issue is quite complex, but I found the problem on LHN in both my solution and @Victor-Nyagudi's proposal (please take a look on his video) I have the way to fix it

I suggest we should check if the lastMessageText is wrap by \n then returns the inside text:

Do you like it? If yes, I and @Victor-Nyagudi can work together to fix this issue?

[eh2077]

Proposal

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

Markdown heading is not allowed in quotes

What is the root cause of that problem?

The root cause is that the heading rule to translate markdown heading to h1 tag matches line starts with # and is applied after the quote rule. So the following input like

> # heading

will be processing by the quote rule first with output

<blockquote># heading</blockquote>

and the output doesn't match the regex /^# +(?! )((?:(?!<pre>).)+)\s*/gm of heading rule.

So the root cause is that we don't handle markdown heading translation in quote rule for the content of blockquote, see the replacement method and method calls(placeA and placeB) of replacement in method modifyTextForQuote.

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

To fix this issue, we should apply heading rule to the content of blockquote in the replacement method of quote rule.

We can change the replacement method of quote rule from

replacement: g1 => `<blockquote>${g1}</blockquote>`,

to

replacement: (g1) => {
    const replacedText = this.replace(g1, {filterRules: ['heading1'], shouldEscapeText: false});
    return `<blockquote>${replacedText}</blockquote>`;
},

We also need to fix method htmlToText for new message html like <blockquote><h1>heading</h1></blockquote>. We can add following new rules below this line to ensure we handle line break in text correctly.

{
    name: 'blockquoteWrapHeadingOpen',
    regex: /<blockquote><h1>/gi,
    replacement: '<blockquote>',
},
{
    name: 'blockquoteWrapHeadingClose',
    regex: /<\/h1><\/blockquote>/gi,
    replacement: '</blockquote>',
},

Furthermore, we also need to change htmlToMarkdown method to support translate html, like

"<blockquote><h1>heading</h1>test a</blockquote>test"

back to markdown

> # heading
> test a
test

So we need to change this line to

const resultString = g2.replace(/\[block\]/g, '\n').trim().split('\n').map(m => `> ${m}`).join('\n');

We replace [block] at the end of heading in advance to handle multiple line quotes.

Edited: I'll fix more corner cases about line break handling of heading inside quote if this proposal is selected.

Click to see demo video

https://user-images.githubusercontent.com/117511920/233136877-6fcec86f-8b11-4ea7-ae00-5ff0fe955c1e.mov

What alternative solutions did you explore? (Optional)

N/A

[eh2077]

Hi @Santhosh-Sellavel , would you like to have a look at my proposal? It identifies the root cause and provides a simple solution with minimal changes.

[maddylewis]

bumping @Santhosh-Sellavel on this comment here - https://github.com/Expensify/App/issues/17367#issuecomment-1514060520

[Santhosh-Sellavel]

Sorry for the delay, will get this by tomorrow!

[Santhosh-Sellavel]

@pecanoro I think @eh2077's proposal makes more sense and Looks good to me

🎀 👀 🎀 C+ Reviewed

[Victor-Nyagudi]

Before any proposal is accepted, I'd like to bring to everyone's attention another heading issue that's currently open and being investigated.

The proposal @Santhosh-Sellavel likes makes changes in other places that could clash with what's trying to be accomplished over there.

I urge the reviewers to consider the progress made on that other issue when determining what happens in this one.

[Santhosh-Sellavel]

@Victor-Nyagudi

The proposal @Santhosh-Sellavel likes makes changes in other places that could clash with what's trying to be accomplished over there.

How it clashes with other issue, can you explain?

IMO it looks Both are different issues or independent issues. If not let me know with some reasoning.

CC: @eh2077 @pecanoro

[Victor-Nyagudi]

@Santhosh-Sellavel The first part of the proposal is absolutely fine.

However, the second part about fixing line breaks is similar to what's trying to be accomplished in the issue I linked. Furthermore, the suggested changes touch on htmlToMarkdown() and htmlToText() that are involved in the copy/paste workflow.

I was just concerned that depending on which PR gets approved first, the changes in one could affect the changes in the other, but if you see nothing to worry about, then we'll go with what you say.