[HOLD for payment 2023-06-01] [$1000] Web - Chat - In offline mode, on deleting any latest message, if last message has & sign in it, app displays it as & in LHN

[kbecciv]

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:

1. Action Performed:
2. Open the app
3. Open any report
4. Send message with & sign in it. eg: Hello & welcome
5. Send any other message to that report
6. Switch off connection or go to settings->preferences->force offline
7. Again open that report
8. Delete the last message and observe in LHN

Expected Result:

App should not convert & to &amp; in offline mode in LHN

Actual Result:

App converts & to &amp; in offline mode in LHN if we delete the message after it

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.1.3

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Recording.2520.mp4

and.sign.displayed.wrong.in.LHN.in.offline.mode.mp4

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681808180603719

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01206f24c549758b9a
• Upwork Job ID: 1650576764427841536
• Last Price Increase: 2023-05-15

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

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

[anmurali]

Reproduced

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~01206f24c549758b9a

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

Triggered auto assignment to @youssef-lr (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[alitoshmatov]

Proposal

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

In offline mode, on deleting any latest message, if last message has & sign in it, app displays it as & in LHN

What is the root cause of that problem?

This is caused because there is a bug in ExpensiMark, when we are parsing text to html we are escaping it:
https://github.com/Expensify/expensify-common/blob/e93e1eb448ad6bdbde911fd6239f70d5e749635e/lib/ExpensiMark.js#L331-L333

But when we are doing reverse html to string we are not unescaping it:
https://github.com/Expensify/expensify-common/blob/e93e1eb448ad6bdbde911fd6239f70d5e749635e/lib/ExpensiMark.js#L540-L548

This is also visible when we add new comment. In optimistic data, we are generating this:

And backend is pushing this:

When we delete and generate optimistic data, we are using htmlToText to generate lastMessageText:

App/src/libs/ReportActionsUtils.js

Line 160 in dd827e9

const messageText = parser.htmlToText(htmlText);

and since it is offline it is keeping optimistic data and showing its value.

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

We should update ExpsiMark.htmlToText to unescape using _.unescape here:
https://github.com/Expensify/expensify-common/blob/e93e1eb448ad6bdbde911fd6239f70d5e749635e/lib/ExpensiMark.js#L540-L542

What alternative solutions did you explore? (Optional)

[s77rt]

@alitoshmatov Thanks for the proposal. The RCA is correct. The fix is almost correct. We should call _.unescape after applying the htmlToTextRules and not before, otherwise html will get corrupted. I won't block on that though.

🎀 👀 🎀 C+ reviewed

cc @youssef-lr

[youssef-lr]

Proposal looks to be working. However, I was curious about what we do differently when setting the report's lastMessageText optimistically that already unescapes html - since simply sending an & offline will still show it properly, and I found out that we are using Str.htmlDecode for this here. I think we should keep things consistent and reuse the same method. So instead of modifying htmlToText we can use Str.htmlDecode in getLastVisibleMessageText.

Also, for this solution to work, we also need to update this line to use message[0].text instead of message[0].html.

What do you think @s77rt?

[alitoshmatov]

@youssef-lr I agree with you. I am confused as when to use Str.htmlDecode and parser.htmlToText? Do these have exactly the same functionality or not. If they have the same functionality, why they both are used equally throughout the project?

I also didn't understand why they didn't use text field itself, rather than parsing html field in the first place.

[s77rt]

@youssef-lr There have been a number of bugs related to html <-> text encoding and by the looks of it we are not exactly done with those 😅 so I'm not sure why we are using both Str.htmlDecode and parser.htmlToText.

---

So instead of modifying htmlToText we can use Str.htmlDecode in getLastVisibleMessageText.

I think we should fix still fix parser.htmlToText even if we are not going to use it here.

---

Also, for this solution to work, we also need to update this line to use message[0].text instead of message[0].html.

I don't think this is necessary. I think you based this assumption on this line but this piece of code is actually misleading. We should either: 1. Get HTML -> Decode it - OR - 2. Get TEXT (and use it directly). So in that case we should not have bothered calling Str.htmlDecode since lastCommentText is already a text not html.

---

That point ^ actually raises another concern, in getLastVisibleMessageText why we are fetching the html message and decode it? Why not just use the text message itself. It looks we are decoding a message that we already have a decoded version of.

---

Sorry for the lengthy comment. Here is the plan:

1. Ask on Slack if someone knows why we have two html decoder methods
2. Agree on using only one and replace all occurrences of the other - OR - "Unify" them
3. In getLastVisibleMessageText use text instead of html (and remove the decoding part).

What do you think?

cc @pecanoro as you worked on similar bugs before.

[s77rt]

@youssef-lr ^

[MelvinBot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[alitoshmatov]

Even removing Str.htmlDecode would have affected a lot of places. I think #19237 achieved its purpose of fixing the current issue, without introducing new bugs.

[therealsujitk]

#19789 was present even before my PR(#19237).

Are you sure, because I tested it and confirmed. Previously parser.htmlToText() did not unescape the result before returning so when text='< <' parser.htmlToText(text) returned < < and Str.htmlDecode(parser.htmlToText(text)) returned < &lt;. Now parser.htmlToText(text) unescapes the result so it returns < &lt; and Str.htmlDecode(parser.htmlToText(text)) returns < < which is incorrect.

Further _.unescape() only decodes a limited number of entities so it should be replaced with Str.htmlDecode() in htmlToText().

[alitoshmatov]

@therealsujitk
I tested with  since it was in #19789 description

[therealsujitk]

I tested with  since it was in #19789 description

Even if you test it with that, it first get's escaped in the replace() function and becomes . Previously it was being decoded only once so it became  again and was printed as is. Now it gets decoded twice so  ->  -> �.

[therealsujitk]

You can test it by commenting the _.unescape() in ExpensiMark.js, you'll see this issue wasn't there before.

[youssef-lr]

@alitoshmatov @s77rt do we need a follow up PR to fix the regression?

[s77rt]

We have this #20443.

[youssef-lr]

Not overdue.

[melvin-bot]

@youssef-lr, @anmurali, @s77rt, @alitoshmatov Whoops! This issue is 2 days overdue. Let's get this updated quick!

[s77rt]

Not overdue. This is on hold for payment (Jun 23rd same as #19789)

[alitoshmatov]

#19789 is closed, and this one is ready to proceed.

[alitoshmatov]

@pecanoro Another friendly bump

[s77rt]

Payments:

• Bug reporter @dhanashree-sawant $250
• Contributor @alitoshmatov $1000 (no bonus due to regression)
• C+ @s77rt $500 (no bonus and -50% due to regression)

[dhanashree-sawant]

Hi @s77rt, I think you meant @dhanashree-sawant 😅

[s77rt]

Oops my bad. Sorry @dhairyasenjaliya for the false positive 😅

[dhairyasenjaliya]

No issue all good 😁

[anmurali]

https://www.upwork.com/nx/wm/offer/25455172 - @dhanashree-sawant
https://www.upwork.com/nx/wm/offer/25455207 - @alitoshmatov
https://www.upwork.com/nx/wm/offer/25455233 - @s77rt

[dhanashree-sawant]

Hi @anmurali, Thanks! Offer accepted.

[alitoshmatov]

Accepted

[melvin-bot]

@youssef-lr, @anmurali, @s77rt, @alitoshmatov Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[dhanashree-sawant]

Hi @pecanoro , @anmurali I have accepted offer on upwork but it is still left to be approved, can you check once you are available?

[alitoshmatov]

Same here

[pecanoro]

@anmurali Can you approve the offers, please? 🤗

[anmurali]

Everyone's been paid.