[$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 [email protected] 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
• [x] 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

https://user-images.githubusercontent.com/93399543/233105111-8835b183-199c-4a5a-863b-278974217c6c.mp4

https://user-images.githubusercontent.com/93399543/233105199-7a5ac1b4-4073-4cd4-b295-3889cd9f0f91.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-04-24

[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)

• [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

[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: https://github.com/Expensify/App/blob/dd827e953adb07495c42256a36017c56f30348d2/src/libs/ReportActionsUtils.js#L160 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.

:ribbon: :eyes: :ribbon: 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 :sweat_smile: 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? 💸

[pecanoro]

@s77rt That sounds like a great plan! Super agreed on using text for the LHN since text is what we send from the back-end either way as the last visible action.

[s77rt]

I think Str.htmlDecode is just another version of _.unescape. We are probably using it as a workaround due the bug on parser.htmlToText (since it's missing the _.unescape call).

Those two functions are different. To convert from html to text we actually need to use parser.htmlToText.

So I think the new plan is:

1. Fix the bug on parser.htmlToText
2. In getLastVisibleMessageText use text instead of html (and remove the decoding part).

What do you think? @youssef-lr @pecanoro

[alitoshmatov]

I think Str.htmlDecode is just another version of _.unescape. We are probably using it as a workaround due the bug on parser.htmlToText (since it's missing the _.unescape call).

@s77rt I agree with you

There are multiple instances of using Str.htmlDecode with parser.htmlToText https://github.com/Expensify/App/blob/f8daa3f87d09edc0d9492f9c9fc1861276acca88/src/components/CopySelectionHelper.js#L40 https://github.com/Expensify/App/blob/2e8396f72c51a49a74033fadd3dec65cae56be7a/src/pages/home/report/ContextMenu/ContextMenuActions.js#L162 https://github.com/Expensify/App/blob/237c8cde7cc43287d3526fa77ab27d2c6cfd24b8/src/libs/ReportUtils.js#L1125 https://github.com/Expensify/App/blob/b2c28acea914a54dc6585485529fd01019a8bfcf/src/libs/actions/Report.js#L939-L943

[MelvinBot]

@youssef-lr @anmurali @s77rt this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[s77rt]

@MelvinBot We have a fix for the issue already and a plan as well. Just waiting for some :eyes: on this and we should be good to move forward :rocket:!

[pecanoro]

@s77rt Good plan, thank you for summarizing!

[s77rt]

@youssef-lr I think this is waiting for your input here

[youssef-lr]

Sorry I was focused on some high priority tasks that have a tight deadline. I will get to this today. Thanks for the bump @s77rt.

[s77rt]

Not overdue. We are almost okay with a proposal. Just waiting for @youssef-lr

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@youssef-lr @anmurali @s77rt this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[s77rt]

Not overdue. Waiting for @youssef-lr

[pecanoro]

@youssef-lr @s77rt Can you retest this? I recently change something related to this on the back-end but I am not sure if it is solved. If we are encoding/decoding in the front-end, the issue might have persisted. Either way, we are sending text and not HTML for the LHN now, so it's something to be taken into account.

[s77rt]

@pecanoro The issue is in the front-end. The optimistic text message is not constructed correctly due to a bug in parser.htmlToText.

[pecanoro]

Thanks for clarifying! Just making sure we were not pointlessly waiting 😄

[youssef-lr]

So I think the new plan is:

1. Fix the bug on parser.htmlToText
2. In getLastVisibleMessageText use text instead of html (and remove the decoding part).

This sounds good to me. Let's move forward with this @s77rt @alitoshmatov?