[HOLD for payment 2023-06-08] [$1000] Using backtick in the description in send/request money changes to '&#x60' in the report

[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. Go to web chrome
2. Go to any chat
3. Send/Request money and in the description , type backticks
4. Send/request money
5. Notice that in the report, it displays &#x60 for backticks.
6. Follow the same steps in the chat box, and see that we don't get such problem. Also, use other special characters, problem is not seen.

Expected Result:

Using backtick in the description in send/request money should not change to '&#x60' in the report

Actual Result:

Using backtick in the description in send/request money changes to '&#x60' in the report (works well for other special characters)

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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.3-1
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

name-2023-04-21_11.38.56.mp4

Recording.307.mp4

Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1682056771077709

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010020e818c61501ab
• Upwork Job ID: 1650782203779108864
• Last Price Increase: 2023-05-16

[MelvinBot]

Triggered auto assignment to @bfitzexpensify (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

[bfitzexpensify]

Reproduced

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~010020e818c61501ab

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[dukenv0307]

Proposal

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

Using backtick in the description in send/request money is changing to '&#x60' in the report

What is the root cause of that problem?

1. User request/sent money with description `12``
2. API is called with comment is converted to <code>12</code>&#x60; because of these lines

App/src/libs/ReportUtils.js

Lines 1391 to 1396 in a5c4737

	const commentText = getParsedComment(comment);
	const textForNewComment = parser.htmlToText(commentText);
	const textForNewCommentDecoded = Str.htmlDecode(textForNewComment);
	const originalMessage = {
	amount,
	comment: textForNewComment,

and

App/src/libs/actions/IOU.js

Lines 362 to 369 in a5c4737

	const parsedComment = ReportUtils.getParsedComment(comment);
	API.write(
	'RequestMoney',
	{
	debtorEmail: payerEmail,
	amount,
	currency,
	comment: parsedComment,

3. API response returns the comment is 12`

We use this comment value to display on UI

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

1. We should send the description to BE as what users enter
   change
   

   App/src/libs/actions/IOU.js

   Lines 362 to 369 in a5c4737

   	const parsedComment = ReportUtils.getParsedComment(comment);
   	API.write(
   	'RequestMoney',
   	{
   	debtorEmail: payerEmail,
   	amount,
   	currency,
   	comment: parsedComment,

to

API.write(
        'RequestMoney',
        {
            debtorEmail: payerEmail,
            amount,
            currency,
            comment,

apply above logic to other functions: splitBill, getSendMoneyParams

2. Update optimistic data as well

change

App/src/libs/ReportUtils.js

Lines 1391 to 1396 in a5c4737

	const commentText = getParsedComment(comment);
	const textForNewComment = parser.htmlToText(commentText);
	const textForNewCommentDecoded = Str.htmlDecode(textForNewComment);
	const originalMessage = {
	amount,
	comment: textForNewComment,

to

 const originalMessage = {
        amount,
        comment,
...

and

App/src/libs/ReportUtils.js

Line 1426 in a5c4737

message: getIOUReportActionMessage(type, amount, textForNewCommentDecoded, currency, paymentType, isSettlingUp),

to

        message: getIOUReportActionMessage(type, amount, comment, currency, paymentType, isSettlingUp),

3. Be will return the comment as what we pass to, that why we can remove Str.htmlDecode here (prevent the case users type &nbsp)

App/src/components/ReportActionItem/IOUPreview.js

Line 151 in a5c4737

const requestComment = Str.htmlDecode(moneyRequestAction.comment).trim();

Result

Screen.Recording.2023-05-18.at.16.11.54.mp4

[alitoshmatov]

Looks like have the same cause - #17658

[alex-mechler]

This is likely a back-end issue where specifically for backticks, they send HTML-encoded character (&#x60) back to the front-end rather than backtick characters. This doesn't happen for other special characters like & so we need to look in the back-end to resolve it.

The backend actually returns & for & as well, we are just parsing that correctly

Looks like have the same cause - #17658

Thanks, looking into this

[alex-mechler]

Looks like have the same cause - #17658

This appears to be similar, but with that fix applied, I am still able to reproduce this issue

[AngelNBazan]

Proposal

Problem Statement:
When using a backtick in the description while sending or requesting money, the backtick is displayed as '&#x60' (the HTML entity representation) in the report, instead of the expected backtick.
Root Cause:
The issue stems from the incorrect/unnecessary use of text parsing logic (ExpensiMark()), which converts backticks and other markdown entities to their HTML entity representation in this case for backticks '`' or into code blocks when in pairs <"code">...</"code">.
In ReportUtils.js, unnecessary conversion and reversions result in the comment being incorrectly formatted/converted and thus when used later in the code causes the errors.

App/src/libs/ReportUtils.js

Lines 1096 to 1099 in 32fbb2c

	const parser = new ExpensiMark();
	const commentText = getParsedComment(comment);
	const textForNewComment = parser.htmlToText(commentText);
	const textForNewCommentDecoded = Str.htmlDecode(textForNewComment);

Here is what the conversions look like:
//Sample Text: `123`456`
//commentText: <"code">123</"code">456"&#x60";
//textForNewComment: 123456`
//textForNewCommentDecoded: 123456`
Therefore, when using these variables to save comments, the comments are saved incorrectly because of the incorrect usage of textForNewComment and textForNewCommentDecoded.

App/src/libs/ReportUtils.js

Lines 1100 to 1107 in 32fbb2c

	const originalMessage = {
	amount,
	comment: textForNewComment,
	currency,
	IOUTransactionID,
	IOUReportID,
	type,
	};

App/src/libs/ReportUtils.js

Lines 1130 to 1131 in 32fbb2c

	originalMessage,
	message: getIOUReportActionMessage(type, amount, participants, textForNewCommentDecoded, currency, paymentType, isSettlingUp),

The unnecessary parsing in the API calls in IOU.js, specifically /api/RequestMoney and /api/SendMoneyElsewhere also cause the issue, as these calls are responsible for saving the comments to the backend.

App/src/libs/actions/IOU.js

Lines 910 to 914 in 32fbb2c

	const {
	params, optimisticData, successData, failureData,
	} = getSendMoneyParams(report, amount, currency, comment, CONST.IOU.PAYMENT_TYPE.ELSEWHERE, managerEmail, recipient);
	API.write('SendMoneyElsewhere', params, {optimisticData, successData, failureData});

App/src/libs/actions/IOU.js

Lines 668 to 670 in 32fbb2c

	function getSendMoneyParams(report, amount, currency, comment, paymentMethodType, managerEmail, recipient) {
	const recipientEmail = OptionsListUtils.addSMSDomainIfPhoneNumber(recipient.login);
	const parsedComment = ReportUtils.getParsedComment(comment);

App/src/libs/actions/IOU.js

Line 675 in 32fbb2c

comment: parsedComment,

App/src/libs/actions/IOU.js

Lines 196 to 201 in 32fbb2c

	const parsedComment = ReportUtils.getParsedComment(comment);
	API.write('RequestMoney', {
	debtorEmail,
	amount,
	currency,
	comment: parsedComment,

Proposed Solution:
To address this issue, the parsing can be bypassed as it is not necessary to parse the comments/text being passed down to these functions.
What alternative solutions did you explore? (Optional)
Implementing a new parsing function specifically for comments. Although this could be done, it would mean building a new parser and also changing how the comments are saved, which is more complex and changes to the comment UI.

[MelvinBot]

📣 @AngelNBazan! 📣

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>

[AngelNBazan]

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

[MelvinBot]

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

[dukenv0307]

The backend actually returns & for & as well, we are just parsing that correctly

@alex-mechler thanks for the feedback! I think we're not using the data returned in your screenshot for the report comment.

We're using the text field of the reportAction instead, see below where I sent a description of "&&&&&& ``````". The & is returned as is while the "`" is encoded before returning.

Can see below in the OpenReport command after requesting money.

[mollfpr]

We send ` from the front end. In the back end, it strips out the HTML tag.

[dukenv0307]

hi @mollfpr in this line

App/src/libs/actions/IOU.js

Line 196 in f8acfae

const parsedComment = ReportUtils.getParsedComment(comment);

, if we change to const parsedComment = _.escape(comment); to avoid adding the incorrect <code> html tag to the comment. It will send the `````` to the back-end and back-end returns as is in the text field rather than convert back to "``````" (this is different from, say, & character), so I think there's still something wrong with the back-end.

[mollfpr]

@dukenv0307 Why we send to the backend ``````?

Some markdown character pairs like _ _, * *, when sending in IOU Description, will disappear. This also happens to backtick if sending it in offline mode (6 consecutive backticks will become 2 backticks).

@alex-mechler @bfitzexpensify Are we on purpose striping the HTML tag in the backend?

[AngelNBazan]

Proposal Updated #17798 (comment)

[melvin-bot]

@alex-mechler, @mollfpr, @bfitzexpensify, @AngelNBazan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[alex-mechler]

PR is in review Melvin

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

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

• Fix: Parser Bug in IOU #19868

If no regressions arise, payment will be issued on 2023-06-08. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
• [@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@mollfpr] Determine if we should create a regression test for this bug.
• [@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@bfitzexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

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

[bfitzexpensify]

Hey @sonialiap - I'm heading OOO this week. This one will need hiring/payment in a couple of days so assigning a BZ buddy. Thank you for the help!

[mollfpr]

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

No PR in E/App is causing the regression. We decide not to parse the message when send it to BE.

[@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

The regression step will cover this.

[@mollfpr] Determine if we should create a regression test for this bug.

👍

[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

1. Create an action(Request money, Send money, Split bill)
2. In the comment/description section and add backticks(`) to text
3. Verify the correct amount of backticks are displayed, all text in between backticks is present, and backticks are displayed properly and not as html representation('&#x60')
4. 👍 or 👎

[bfitzexpensify]

All payments issued and contracts ended.

[bfitzexpensify]

Agreed with regression steps, steps proposed in https://github.com/Expensify/Expensify/issues/291252

[bfitzexpensify]

We're all done here, so closing this out - thanks for the work everyone!