[HOLD for payment 2022-06-16] [$1000] Last message text in LHN includes text and no space above quoted text #8134

mvtglobally · 2022-03-14T05:35:27Z

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:

Receive quoted text and non quoted text in a single message
View LHN

Expected Result:

We should see the first line of the last message.

Actual Result:

We see both lines of the last message, with no space to fill the line break

Workaround:

unknown

Platform:

Where is this issue occurring?

Web
iOS
Android
Desktop App
Mobile Web

Version Number: 1.1.41-3
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

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

View all open jobs on GitHub

MelvinBot · 2022-03-14T05:35:30Z

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

alex-mechler · 2022-03-14T19:03:05Z

This mostly looks good to me, but I don't know how much I agree with the Expected Result here. I think we would show the first line of the last message, as its a preview of the last message, so sending the last thing in that message does not make a ton of sense to me. Will mention in the slack thread.

alex-mechler · 2022-03-14T20:20:45Z

Updated the Expected based on the slack thread https://expensify.slack.com/archives/C01GTK53T8Q/p1647284812200829?thread_ts=1646654742.446119&cid=C01GTK53T8Q

Ready to be handled externally

MelvinBot · 2022-03-14T20:20:55Z

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

stephanieelliott · 2022-03-15T04:11:05Z

Posted to Upwork: https://www.upwork.com/jobs/~017fecd4f1b010dc8e

MelvinBot · 2022-03-15T04:11:21Z

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

MelvinBot · 2022-03-15T04:11:24Z

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

stephanieelliott · 2022-03-22T02:59:18Z

2x'd t0 $500: https://www.upwork.com/jobs/~017fecd4f1b010dc8e

brianmuks · 2022-03-24T20:50:03Z

I need help on how I can generate quoted text.

brianmuks · 2022-03-24T21:06:04Z

I need help on how I can generate quoted text.

I have figured it out !

brianmuks · 2022-03-24T23:46:47Z

Proposal
The issue is being caused by this line :

App/src/libs/actions/Report.js

Line 1077 in 4d646e7

: htmlForNewComment.replace(/((<br[^>]*>)+)/gi, ' ').replace(/<[^>]*>?/gm, '');

When converting text that contains html tags the tags are being replaced with no consideration for spacing.

The solution would be to replace the tags with whitespace ' ' followed by .trim() for any trailing whitespace

    const cleanText = htmlForNewComment.replace(/((<br[^>]*>)+)/gi, ' ').replace(/<[^>]*>?/gm, ' ').trim();

const textForNewComment = isAttachment ? '[Attachment]'
    : cleanText

Just ending here solves only one side of an issue. The issue is solved for the sender but not the recipient because this line :

App/src/libs/actions/Report.js

Line 1143 in 4d646e7

reportComment: commentText,

allows the recipient to receive text that has not been cleaned . So I suggest replacing it with :
reportComment: cleanText,

Screen.Recording.2022-03-25.at.2.48.26.AM.mov

parasharrajat · 2022-03-25T14:05:49Z

The solution would be to replace the tags with whitespace ' '

Why does it only affect quotes? Did you manage to find it? The last time I checked other tags are safely separated with spaces so if you replace all tags with ' ' it will add extra space between text.

Do you have a better solution? Thanks.

mollfpr · 2022-03-26T13:13:51Z

Proposal

There is some step we need to do to resolve this issue:

Create a function to get the first line of the last message.
The last message shown is from SidebarLinks is from report.lastMessageText, so we need to update how lastMessageText set inside src/libs/actions/Report.js.
Update getLastVisibleMessageText from ReportActions, which it use when receiving an event from pusher to update the last message to show to with a new one.

Creating a function to get the first line of the last message. In this function is we take an HTML message, we take the first line by splitting the line breaker. After that, we check if the first line text we get has a blockquote tag, then remove other strings outside blockquote. In here we already get the first line of the message either the blockquote content or the first line of the message sent. Now replace all the tags and return them as text.

function getFirstLineOfLastMessageHTML(html) {
    let textMessage = '';

    const blockquoteRegExp = new RegExp(/<blockquote>(.*?)<\/blockquote>/g)
    const allTagRegExp = new RegExp(/<[^>]*>?/gm)
    const brTagRegExp = new RegExp(/<br[^>]*>/gi)

    // Split the HTML by line breaker and then get the first item of it
    const splittedComment = html.split(brTagRegExp);
    const firstLineComment = _.first(splittedComment)

    // Get the blockquote tag and the content if present,
    // other than that use the first item from splitting
    if (firstLineComment.match(blockquoteRegExp)) {
        textMessage = _.first(firstLineComment.match(blockquoteRegExp));
    } else {
        textMessage = firstLineComment;
    }

    // Replace all tags present
    return textMessage.replace(allTagRegExp, '');
}

Next step is to change how lastMessageText returns.

App/src/libs/actions/Report.js

Lines 1075 to 1077 in b347866

    
           // Remove HTML from text when applying optimistic offline comment 
        
           const textForNewComment = isAttachment ? '[Attachment]' 
        
               : htmlForNewComment.replace(/((<br[^>]*>)+)/gi, ' ').replace(/<[^>]*>?/gm, '');

-    const textForNewComment = isAttachment ? '[Attachment]'
-        : htmlForNewComment.replace(/((<br[^>]*>)+)/gi, ' ').replace(/<[^>]*>?/gm, '');

+    const textForNewComment = isAttachment ? '[Attachment]' : ReportUtils.getFirstLineOfLastMessageHTML(htmlForNewComment);

App/src/libs/actions/Report.js

Lines 612 to 618 in b347866

    
           // If the report action from pusher is a higher sequence number than we know about (meaning it has come from 
        
           // a chat participant in another application), then the last message text and author needs to be updated as well 
        
           if (newMaxSequenceNumber > initialLastReadSequenceNumber) { 
        
               updatedReportObject.lastMessageTimestamp = reportAction.timestamp; 
        
               updatedReportObject.lastMessageText = ReportUtils.formatReportLastMessageText(messageText); 
        
               updatedReportObject.lastActorEmail = reportAction.actorEmail; 
        
           }

    if (newMaxSequenceNumber > initialLastReadSequenceNumber) {
        const textForNewComment = ReportUtils.getFirstLineOfLastMessageHTML(messageText)

        updatedReportObject.lastMessageTimestamp = reportAction.timestamp;
        updatedReportObject.lastMessageText = ReportUtils.formatReportLastMessageText(textForNewComment);
        updatedReportObject.lastActorEmail = reportAction.actorEmail;
    }

App/src/libs/actions/Report.js

Lines 194 to 196 in b347866

    
           lastMessageText = lastActionMessage 
        
               .replace(/((<br[^>]*>)+)/gi, ' ') 
        
               .replace(/(<([^>]+)>)/gi, '');

-        lastMessageText = lastActionMessage
-            .replace(/((<br[^>]*>)+)/gi, ' ')
-            .replace(/(<([^>]+)>)/gi, '');

+        lastMessageText = ReportUtils.getFirstLineOfLastMessageHTML(lastActionMessage);

App/src/libs/actions/ReportActions.js

Lines 104 to 112 in b347866

    
           function getLastVisibleMessageText(reportID) { 
        
               const lastMessageIndex = _.findLastIndex(reportActions[reportID], action => ( 
        
                   !ReportUtils.isDeletedAction(action) 
        
               )); 
        
               return ReportUtils.formatReportLastMessageText( 
        
                   lodashGet(reportActions, [reportID, lastMessageIndex, 'message', 0, 'text'], ''), 
        
               ); 
        
           }

    const messageText = lodashGet(reportActions, [reportID, lastMessageIndex, 'message', 0, 'html'], '');
    const lastMessageText = ReportUtils.getFirstLineOfLastMessageHTML(messageText);

    return ReportUtils.formatReportLastMessageText(lastMessageText);

Screenshots

Sent new message

Screen.Recording.2022-03-26.at.20.04.33.mov

Update a message

Screen.Recording.2022-03-26.at.20.05.32.mov

Delete a message

Screen.Recording.2022-03-26.at.20.05.56.mov

brianmuks · 2022-03-26T17:03:30Z

The solution would be to replace the tags with whitespace ' '

Why does it only affect quotes? Did you manage to find it? The last time I checked other tags are safely separated with spaces so if you replace all tags with ' ' it will add extra space between text.

Do you have a better solution? Thanks.

Thanks @parasharrajat for your feedback. Your feedback has helped me to come up with a better solution.
Since the issue only affects the quotes , I discovered that the bug is coming from :
https://github.com/brianmuks/Expensify.cash/blob/main/node_modules/expensify-common/lib/ExpensiMark.js
on line 413 a new line \n is being replaced with no-space ''

 textToFormat = textToFormat.replace(/\n$/, '');

Please keep in mind that when a user is typing a quote, to switch to regular text a new line is introduced Shit+enter.
So with the code above the reverse happens causing the quoted text and regular text to be in a single text .Hence I propose to remove line 413 from the ExpensiMark.js. So the resulting block will be :

    // Make sure we will only modify if we have Text needed to be formatted for quote
                    if (textToFormat !== '') {
                        replacedText += replacement(textToFormat);
                        textToFormat = '';
                    }

parasharrajat · 2022-03-28T17:27:26Z

AFAICT, this code block is responsible to remove the \n for the last line on the quote itself and middle lines. This block never executes at the start of the quote. Also, this is needed for custom quote parsing. You have started to look at the correct place. We can also do better for the solution. Thanks.

brianmuks · 2022-03-29T14:02:50Z

The solution would be to replace the tags with whitespace ' '

Why does it only affect quotes? Did you manage to find it? The last time I checked other tags are safely separated with spaces so if you replace all tags with ' ' it will add extra space between text.
Do you have a better solution? Thanks.

Thanks @parasharrajat for your feedback. Your feedback has helped me to come up with a better solution. Since the issue only affects the quotes , I discovered that the bug is coming from : https://github.com/brianmuks/Expensify.cash/blob/main/node_modules/expensify-common/lib/ExpensiMark.js on line 413 a new line \n is being replaced with no-space ''
 textToFormat = textToFormat.replace(/\n$/, '');
Please keep in mind that when a user is typing a quote, to switch to regular text a new line is introduced Shit+enter. So with the code above the reverse happens causing the quoted text and regular text to be in a single text .Hence I propose to remove line 413 from the ExpensiMark.js. So the resulting block will be :
    // Make sure we will only modify if we have Text needed to be formatted for quote
                    if (textToFormat !== '') {
                        replacedText += replacement(textToFormat);
                        textToFormat = '';
                    }

@parasharrajat here is my new proposal . Instead of removing the line below :
textToFormat = textToFormat.replace(/\n$/, '');
we can modify it to replace the \n with space ' '.

Before The Changes

Screen.Recording.2022-03-29.at.3.42.04.PM.mov

Ater The Changes

Screen.Recording.2022-03-29.at.3.41.02.PM.mov

mollfpr · 2022-04-18T15:54:19Z

@parasharrajat just wants to make it clear.

So I should create a new function inside ExpensifyMark.js to replace the LHN text right?

     .replace(/((<br[^>]*>)+)/gi, ' ') 
     .replace(/(<([^>]+)>)/gi, '')
     .replace(/</blockquote>/gm, ' ');

and then replace the logic above with calling the created function on Exp/App repo after exp-common is merged?

parasharrajat · 2022-04-18T15:56:35Z

Yup. Something like that. If you feel that we should not update the common repo. Then please confirm this on slack channel.

mollfpr · 2022-04-19T06:08:35Z

PR raise for ExpensiMark

cc @parasharrajat @Luke9389

stephanieelliott · 2022-04-26T14:38:20Z

PR is awaiting internal review

parasharrajat · 2022-05-03T09:49:01Z

Final PR may be on Hold for #8814 (comment)

stephanieelliott · 2022-05-09T18:20:51Z

PR is on hold

Luke9389 · 2022-05-17T23:29:19Z

PR on HOLD

stephanieelliott · 2022-05-25T17:00:36Z

Still on hold

stephanieelliott · 2022-06-07T12:14:57Z

Still on hold pending PR from @Luke9389, see #8814 (comment) for context

Luke9389 · 2022-06-07T12:22:36Z

Thanks for the bump here. I forgot this was on me!

melvin-bot · 2022-06-09T22:45:23Z

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

Fix last message text in LHN includes text and no space above quoted text #8814

If no regressions arise, payment will be issued on 2022-06-16. 🎊

mollfpr · 2022-06-17T04:28:56Z

@stephanieelliott bump 😃

stephanieelliott · 2022-06-22T23:41:01Z

All paid, thank you!

mvtglobally added Engineering Daily KSv2 labels Mar 14, 2022

MelvinBot assigned alex-mechler Mar 14, 2022

alex-mechler removed their assignment Mar 14, 2022

alex-mechler added the External Added to denote the issue can be worked on by a contributor label Mar 14, 2022

MelvinBot assigned stephanieelliott Mar 14, 2022

stephanieelliott added the Exported label Mar 15, 2022

botify removed the Daily KSv2 label Mar 15, 2022

MelvinBot added the Weekly KSv2 label Mar 15, 2022

MelvinBot assigned parasharrajat Mar 15, 2022

MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 15, 2022

MelvinBot assigned Luke9389 Mar 15, 2022

stephanieelliott changed the title ~~Last message text in LHN includes text and no space above quoted text~~ [$500] Last message text in LHN includes text and no space above quoted text Mar 23, 2022

MelvinBot added the Overdue label Apr 5, 2022

mollfpr mentioned this issue Apr 19, 2022

convert html to flat text for LHN uses Expensify/expensify-common#440

Merged

mollfpr mentioned this issue Apr 28, 2022

Fix last message text in LHN includes text and no space above quoted text #8814

Merged

89 tasks

melvin-bot bot added the Overdue label May 17, 2022

melvin-bot bot removed the Overdue label May 17, 2022

melvin-bot bot added the Overdue label Jun 2, 2022

melvin-bot bot removed the Overdue label Jun 7, 2022

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jun 9, 2022

melvin-bot bot changed the title ~~[$1000] Last message text in LHN includes text and no space above quoted text~~ [HOLD for payment 2022-06-16] [$1000] Last message text in LHN includes text and no space above quoted text Jun 9, 2022

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jun 16, 2022

melvin-bot bot added the Overdue label Jun 20, 2022

mollfpr mentioned this issue Jun 20, 2022

Replace br from quote Expensify/expensify-common#455

Merged

89 tasks

Luke9389 added the Reviewing Has a PR in review label Jun 20, 2022

melvin-bot bot removed the Overdue label Jun 20, 2022

stephanieelliott closed this as completed Jun 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2022-06-16] [$1000] Last message text in LHN includes text and no space above quoted text #8134

[HOLD for payment 2022-06-16] [$1000] Last message text in LHN includes text and no space above quoted text #8134

mvtglobally commented Mar 14, 2022 •

edited by alex-mechler

Loading

MelvinBot commented Mar 14, 2022

alex-mechler commented Mar 14, 2022

alex-mechler commented Mar 14, 2022

MelvinBot commented Mar 14, 2022

stephanieelliott commented Mar 15, 2022

MelvinBot commented Mar 15, 2022

MelvinBot commented Mar 15, 2022

stephanieelliott commented Mar 22, 2022

brianmuks commented Mar 24, 2022

brianmuks commented Mar 24, 2022

brianmuks commented Mar 24, 2022 •

edited

Loading

parasharrajat commented Mar 25, 2022

mollfpr commented Mar 26, 2022

brianmuks commented Mar 26, 2022 •

edited

Loading

parasharrajat commented Mar 28, 2022 •

edited

Loading

brianmuks commented Mar 29, 2022 •

edited

Loading

mollfpr commented Apr 18, 2022

parasharrajat commented Apr 18, 2022 •

edited

Loading

mollfpr commented Apr 19, 2022

stephanieelliott commented Apr 26, 2022

parasharrajat commented May 3, 2022

stephanieelliott commented May 9, 2022

Luke9389 commented May 17, 2022

stephanieelliott commented May 25, 2022

stephanieelliott commented Jun 7, 2022

Luke9389 commented Jun 7, 2022

melvin-bot bot commented Jun 9, 2022

mollfpr commented Jun 17, 2022

stephanieelliott commented Jun 22, 2022

[HOLD for payment 2022-06-16] [$1000] Last message text in LHN includes text and no space above quoted text #8134

[HOLD for payment 2022-06-16] [$1000] Last message text in LHN includes text and no space above quoted text #8134

Comments

mvtglobally commented Mar 14, 2022 • edited by alex-mechler Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platform:

MelvinBot commented Mar 14, 2022

alex-mechler commented Mar 14, 2022

alex-mechler commented Mar 14, 2022

MelvinBot commented Mar 14, 2022

stephanieelliott commented Mar 15, 2022

MelvinBot commented Mar 15, 2022

MelvinBot commented Mar 15, 2022

stephanieelliott commented Mar 22, 2022

brianmuks commented Mar 24, 2022

brianmuks commented Mar 24, 2022

brianmuks commented Mar 24, 2022 • edited Loading

parasharrajat commented Mar 25, 2022

mollfpr commented Mar 26, 2022

Proposal

Screenshots

Sent new message

Update a message

Delete a message

brianmuks commented Mar 26, 2022 • edited Loading

parasharrajat commented Mar 28, 2022 • edited Loading

brianmuks commented Mar 29, 2022 • edited Loading

mollfpr commented Apr 18, 2022

parasharrajat commented Apr 18, 2022 • edited Loading

mollfpr commented Apr 19, 2022

stephanieelliott commented Apr 26, 2022

parasharrajat commented May 3, 2022

stephanieelliott commented May 9, 2022

Luke9389 commented May 17, 2022

stephanieelliott commented May 25, 2022

stephanieelliott commented Jun 7, 2022

Luke9389 commented Jun 7, 2022

melvin-bot bot commented Jun 9, 2022

mollfpr commented Jun 17, 2022

stephanieelliott commented Jun 22, 2022

mvtglobally commented Mar 14, 2022 •

edited by alex-mechler

Loading

brianmuks commented Mar 24, 2022 •

edited

Loading

brianmuks commented Mar 26, 2022 •

edited

Loading

parasharrajat commented Mar 28, 2022 •

edited

Loading

brianmuks commented Mar 29, 2022 •

edited

Loading

parasharrajat commented Apr 18, 2022 •

edited

Loading