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

mollfpr · 2022-04-28T05:26:26Z

Details

Using htmlToText from ExpensifyMark to replace breakline and blockquote closing tag to space ' ', strip out tags.

Fixed Issues

Tests

Open chat
Send quoted text and non-quoted text in a single message. Example:

> Confusing stuff
Tell me about it

Verify on the sender and receiver LHN that there's empty space after the quoted text. Example:
Confusing stuff Tell me about it

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Open chat
Send quoted text and non-quoted text in a single message. Example:

> Confusing stuff
Tell me about it

Verify on the sender and receiver LHN that there's empty space after the quoted text. Example:
Confusing stuff Tell me about it

Verify that no errors appear in the JS console

Screenshots

Web

screen-recording-2022-04-28-at-012537_TXS8ZwSl.mp4

Mobile Web

screen-recording-2022-04-28-at-115143_xnEOINzG.mp4

screen-recording-2022-04-28-at-115102_yu2dZd3t.mp4

Desktop

screen-recording-2022-04-28-at-115536_c5bFvKI6.mp4

iOS

screen-recording-2022-04-28-at-013409_GM3xdxCi.mp4

Android

screen-recording-2022-04-28-at-014003_Vib6YZX0.mp4

…text

parasharrajat · 2022-04-28T18:05:49Z

src/libs/actions/Report.js

+    let messageText = lodashGet(reportAction, ['message', 0, 'html'], '');
    if (reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED) {
        messageText = lodashGet(reportAction, 'originalMessage.html', '');
    }

+    const parser = new ExpensiMark();
+    messageText = parser.htmlToText(messageText);
+


I feel like this change should be done at the backend.

@parasharrajat Yeah probably, any suggestion on what we should do then?

@Luke9389 should we do the correct on the backend as well? The backend already converts the HTML to text and then sends it frontend. It does not use the text that we send in the action instead do the conversion itself. So I think we should also make the correction on the backend.

Sorry, I was OOO last week. Just checking this one out. One moment....

OK, yea. I agree this should be done on the backend.

@Luke9389 @parasharrajat How do we do that?

I think I'll have to make a PR because our backend isn't open-source.

I'm just looking into how we prep the text value of that message key. It doesn't look like we import the Expensimark lib at all in that file.

I'll come back and update you guys when that change has been deployed.

Ok, sorry to flipflop here, but I think this is actually fine to do on the front end. We're using the strip_tags PHP method on the back end, and changing that to pick up on </blockquote> specifically is not going to be pretty, and seems inappropriate.

parasharrajat · 2022-04-28T18:07:15Z

src/libs/actions/ReportActions.js

+    const parser = new ExpensiMark();
    const lastMessageIndex = _.findLastIndex(reportActions[reportID], action => (
        !ReportUtils.isDeletedAction(action)
    ));
+    const htmlText = lodashGet(reportActions, [reportID, lastMessageIndex, 'message', 0, 'html'], '');
+    const messageText = parser.htmlToText(htmlText);

-    return ReportUtils.formatReportLastMessageText(
-        lodashGet(reportActions, [reportID, lastMessageIndex, 'message', 0, 'text'], ''),
-    );
+    return ReportUtils.formatReportLastMessageText(messageText);


If we do the previous change, then we won't need this.

parasharrajat · 2022-05-03T09:48:37Z

@mollfpr We can [Hold] on the PR title.

mollfpr · 2022-05-12T02:49:58Z

@Luke9389 Any update for the API?

Luke9389 · 2022-05-17T23:28:37Z

Still on hold. Feel free to take other jobs in the meantime @mollfpr

Luke9389 · 2022-06-07T14:07:10Z

Took this off HOLD, feel free to continue. Sorry for the long delay here @mollfpr

mollfpr · 2022-06-07T14:15:30Z

@Luke9389 No problem! 👍

parasharrajat · 2022-06-07T18:36:16Z

Please merge main.

parasharrajat · 2022-06-08T14:47:05Z

src/libs/actions/Report.js

@@ -548,9 +547,12 @@ function updateReportWithNewAction(
    reportAction,
    notificationPreference = CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS,
 ) {
-    const messageText = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED
+    let messageText = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED


Suggested change

let messageText = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED

let messageHtml = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED

good catch 👍

parasharrajat · 2022-06-08T15:05:09Z

src/libs/actions/Report.js

-        lastMessageText = lastActionMessage
-            .replace(/((<br[^>]*>)+)/gi, ' ')
-            .replace(/(<([^>]+)>)/gi, '');
+        // We convert the line-breaks and blockquote closing tag in html to space ' ' before striping the tags


I don't think this comment makes sense here. This is best suited for htmlToText method.

parasharrajat

Update this as well

App/src/libs/actions/Report.js

Line 1264 in f26cd0c

    
           newReportAction.message[0].text = Str.stripHTML(htmlForNewComment.replace(/((<br[^>]*>)+)/gi, ' '));

.

parasharrajat

LGTM.

cc: @Luke9389

🎀 👀 🎀 C+ reviewed

parasharrajat · 2022-06-09T13:46:08Z

@mollfpr Improve the QA steps.

OSBotify · 2022-06-09T14:00:12Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-06-09T17:15:25Z

🚀 Deployed to staging by @Luke9389 in version: 1.1.75-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

marcaaron · 2022-06-09T22:11:18Z

src/libs/actions/Report.js

+        : lodashGet(reportAction, ['message', 0, 'html'], '');
+
+    const parser = new ExpensiMark();
+    const messageText = parser.htmlToText(messageHtml);


@Luke9389 thoughts on what changes we need to make now on the API side? I am removing this entire function in #9350... 🙃

OSBotify · 2022-06-09T23:05:28Z

🚀 Deployed to production by @roryabraham in version: 1.1.75-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mollfpr added 2 commits April 28, 2022 01:27

update expensify-common

29d6837

fix last message text in LHN includes text and no space above quoted …

042bb46

…text

mollfpr requested review from marcaaron and a team as code owners April 28, 2022 05:26

melvin-bot bot requested review from Luke9389 and parasharrajat and removed request for a team April 28, 2022 05:26

parasharrajat suggested changes Apr 28, 2022

View reviewed changes

parasharrajat mentioned this pull request May 3, 2022

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

Closed

mollfpr changed the title ~~Fix last message text in LHN includes text and no space above quoted text~~ [Hold] Fix last message text in LHN includes text and no space above quoted text May 3, 2022

marcaaron removed their request for review May 6, 2022 19:49

Luke9389 changed the title ~~[Hold] Fix last message text in LHN includes text and no space above quoted text~~ Fix last message text in LHN includes text and no space above quoted text Jun 7, 2022

Merge branch 'main' of https://github.com/mollfpr/App into fix-8134

09bcb83

mollfpr requested a review from parasharrajat June 8, 2022 01:34

parasharrajat reviewed Jun 8, 2022

View reviewed changes

parasharrajat suggested changes Jun 8, 2022

View reviewed changes

mollfpr added 3 commits June 9, 2022 19:50

renaming the variable

02f8dcc

removing comment

d23dc5f

replacing the Str.stripHtml

c6ab05c

mollfpr requested a review from parasharrajat June 9, 2022 13:26

parasharrajat approved these changes Jun 9, 2022

View reviewed changes

Luke9389 self-assigned this Jun 9, 2022

Luke9389 assigned mollfpr and unassigned Luke9389 Jun 9, 2022

Luke9389 approved these changes Jun 9, 2022

View reviewed changes

Luke9389 merged commit 28a1254 into Expensify:main Jun 9, 2022

mollfpr deleted the fix-8134 branch June 9, 2022 14:59

OSBotify mentioned this pull request Jun 9, 2022

Deploy Checklist: New Expensify 2022-06-09 #9377

Closed

10 tasks

marcaaron reviewed Jun 9, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mollfpr commented Apr 28, 2022 •

edited

Loading

parasharrajat Apr 28, 2022

mollfpr Apr 29, 2022

parasharrajat Apr 29, 2022

Luke9389 May 2, 2022

Luke9389 May 2, 2022

mollfpr May 2, 2022

Luke9389 May 3, 2022

Luke9389 Jun 7, 2022

parasharrajat Apr 28, 2022

parasharrajat commented May 3, 2022

mollfpr commented May 12, 2022

Luke9389 commented May 17, 2022

Luke9389 commented Jun 7, 2022

mollfpr commented Jun 7, 2022

parasharrajat commented Jun 7, 2022

parasharrajat Jun 8, 2022

Luke9389 Jun 8, 2022

parasharrajat Jun 8, 2022

parasharrajat left a comment

parasharrajat left a comment

parasharrajat commented Jun 9, 2022

OSBotify commented Jun 9, 2022

OSBotify commented Jun 9, 2022

marcaaron Jun 9, 2022

OSBotify commented Jun 9, 2022

	let messageText = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED
	let messageHtml = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED

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

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

Conversation

mollfpr commented Apr 28, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented May 3, 2022

mollfpr commented May 12, 2022

Luke9389 commented May 17, 2022

Luke9389 commented Jun 7, 2022

mollfpr commented Jun 7, 2022

parasharrajat commented Jun 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat left a comment

Choose a reason for hiding this comment

parasharrajat left a comment

Choose a reason for hiding this comment

parasharrajat commented Jun 9, 2022

OSBotify commented Jun 9, 2022

OSBotify commented Jun 9, 2022

Choose a reason for hiding this comment

OSBotify commented Jun 9, 2022

mollfpr commented Apr 28, 2022 •

edited

Loading