[HOLD][$250] Expense - Saving multiline description with mark down with no edit triggers system message

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.81-1 Reproducible in staging?: Y Reproducible in production?: Y Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to workspace chat.
3. Submit an expense to workspace chat containing multiline description with mark down on each line. Example:
   • hello *
   • hellloo *
4. Go to transaction thread.
5. Wait for the expense to be completely created (sometimes the expense is created but not sent).
6. Click on the description.
7. Without changing the description, click Save.

Expected Result:

No system message for description edit will show up because no edit is made to the description.

Actual Result:

System message for description edit shows up when no edit is made to the description. When repeating Step 6 and 7, the transaction thread turns blank with the error "There was an error loading more messages."

Workaround:

Unknown

Platforms:

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

• [ ] Android: Native
• [x] Android: mWeb Chrome
• [ ] iOS: Native
• [x] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [x] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/1231fdcd-b41c-44f1-8e9c-38c953b85a9d

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018d19307f19622d14
• Upwork Job ID: 1800742519908507814
• Last Price Increase: 2024-08-08
• Automatic offers:
• mkhutornyi | Reviewer | 102884170
• dominictb | Contributor | 102884171

Issue Owner

Current Issue Owner: @MariaHCD

[melvin-bot[bot]]

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[izarutskaya]

We think this issue might be related to the #collect project.

[Krishna2323]

Proposal

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

Expense - Saving multiline description with mark down with no edit triggers system message

What is the root cause of that problem?

The backend returns *b*\r\n*b*, the \r is counted as extra character which isn't present in the form value, and as a result, the check below doesn't become true, so we don't return early and update description api is called. https://github.com/Expensify/App/blob/2929a911ad33264fd5cee1cefe98b34cdaaa99c5/src/pages/iou/request/step/IOURequestStepDescription.tsx#L115-L121

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

To make the currentDescription same as the form value we need to first use parser.replace and then parser.htmlToMarkdown for currentDescription.

    const currentDescription =
        isEditingSplitBill && !lodashIsEmpty(splitDraftTransaction)
            ? splitDraftTransaction?.comment.comment ?? ''
            : parser.htmlToMarkdown(parser.replace(transaction?.comment.comment ?? '')).trim();

We should also check for similar bug in other similar components.

What alternative solutions did you explore? (Optional)

We can replace \r in currentDescription with "" and then compare.

[dominictb]

Proposal

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

System message for description edit shows up when no edit is made to the description. When repeating Step 6 and 7, the transaction thread turns blank with the error "There was an error loading more messages."

What is the root cause of that problem?

The description that is returned by back-end includes \r character which doesn't exist in value of input. It leads this check here isn't true and then the update API is called that makes the system message appears.

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

1. As other description field, we should accept to store the description of money request as html instead of markdown. Update here to convert the description to html before saving

comment: ReportUtils.getParsedComment(comment),

We need back-end change to accept the description of money request can be stored as htm.

2. Update the default value here to parser.htmlToMarkdown(currentDescription) and update the condition here to newComment === parser.htmlToMarkdown(currentDescription). This is the same way as we do for other description field like task description field, room description field,...

What alternative solutions did you explore? (Optional)

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~018d19307f19622d14

[melvin-bot[bot]]

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

[mkhutornyi]

reviewing proposals

[mkhutornyi]

If this issue doesn't happen on other multiline fields like task description, room description, we should apply same logic to be consistent.

[kadiealexander]

@mkhutornyi is anything wrong with the current proposals?

[melvin-bot[bot]]

@kadiealexander, @mkhutornyi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[mkhutornyi]

@dominictb thanks for the proposal.

We need back-end change to accept the description of money request can be stored as htm.

Is backend already doing this on task description and room description?

[dominictb]

@mkhutornyi Yes.

[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? 💸

[mkhutornyi]

@dominictb's proposal looks good to me. 🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@MariaHCD @kadiealexander @mkhutornyi 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!

[melvin-bot[bot]]

@MariaHCD, @kadiealexander, @mkhutornyi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[MariaHCD]

Apologies for the delay. Reviewing this selected proposal shortly.

[MariaHCD]

Sending the comment as html instead of markdown makes sense since it's aligned with the way we update task description and room description.

@NikkiWines @chiragsalian looking at UpdateMoneyRequestDescription, I think it should be fine to save the comment as html?

[NikkiWines]

Yeah, I think it would be fine to use html here.

[chiragsalian]

sounds fine to me.

[mkhutornyi]

Not overdue

[MariaHCD]

Great, thanks for confirming! Let's go ahead with @dominictb's proposal. I believe UpdateMoneyRequestDescription should already be able to accept html but let me know if that's not the case.

[melvin-bot[bot]]

📣 @mkhutornyi 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @dominictb 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[dominictb]

I got some problem with the BE API (look like the HTML tag is stripped). @MariaHCD, can one of you guys check?

[MariaHCD]

Yup, looking at the logs, it seems we're stripping any HTML

Looking further.

[melvin-bot[bot]]

@MariaHCD, @kadiealexander, @mkhutornyi, @dominictb Eep! 4 days overdue now. Issues have feelings too...

[MariaHCD]

Focused on some higher priority issues - will come back here later

[mkhutornyi]

Not overdue

[melvin-bot[bot]]

@MariaHCD @kadiealexander @mkhutornyi @dominictb this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot[bot]]

@MariaHCD, @kadiealexander, @mkhutornyi, @dominictb Eep! 4 days overdue now. Issues have feelings too...

[kadiealexander]

Getting a retest here.

[kbecciv]

Issue is still reproducible!

https://github.com/Expensify/App/assets/93399543/56ec5b8b-dbde-4dbd-9b0f-308613873ae7

[dominictb]

I have opened a PR to fix it, but there's some API issue and @MariaHCD is looking into it. Once the API is ready, I'll get my PR ready for final review.

[kadiealexander]

@MariaHCD do you have any update on this?

[melvin-bot[bot]]

@MariaHCD, @kadiealexander, @mkhutornyi, @dominictb Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[MariaHCD]

No update just yet, I've been prioritizing issues from #fast-apis and #wave-collect. Will get to this in the next couple days.

[melvin-bot[bot]]

@MariaHCD, @kadiealexander, @mkhutornyi, @dominictb Whoops! This issue is 2 days overdue. Let's get this updated quick!

[MariaHCD]

Still prioritizing other issues. I'll get to this asap.

[mkhutornyi]

Same

[MariaHCD]

Sorry, I wasn't able to prioritize this one and I'm going OOO from tomorrow until Aug 3rd. So I am going to reapply the engineering label and leave a summary:

1. The original issue is that when updating the description of a money request, it does not currently handle markdown well
2. We're going with this proposal https://github.com/Expensify/App/issues/43403#issuecomment-2159805076 that sends the comment as html instead of markdown which is aligned with the way we update task description and room description.
3. But our API endpoint (UpdateMoneyRequestDescription) is stripping the HTML: https://github.com/Expensify/App/issues/43403#issuecomment-2193868561

Internal logs

https://www.expensify.com/_devportal/tools/logSearch/#query=request_id:(%2289a331daa8a71096-HKG%22)+AND+timestamp:%5B2024-06-27T04:56:35.757Z+TO+2024-06-27T06:56:35.757Z%5D&index=logs_expensify-029119 ``` 89a331daa8a71096-HKG virt2.rno 2024-06-27 05:56:35 753 tb-dominic+test1@outlook.com Suspicious but nonmalicious(?) XSS attack attempt in 'comment' from IP 42.117.149.6 ...

hello world
hello it's me

89a331daa8a71096-HKG virt2.rno 2024-06-27 05:56:35 753 tb-dominic+test1@outlook.com Processing 'UpdateMoneyRequestDescription' for 'expensify.com' from '42.117.149.6' ~~ initialReferer: '' browserGUID: '661f3c122f3ee' command: 'UpdateMoneyRequestDescription' comment: 'hello worldhello it's me' reportID: '395035666409464' transactionID: '2329627498831684845' reportActionID: '2475839487821295989' appversion: '9.0.2-3' pusherSocketID: '741885.101594' clientUpdateID: '476617897' authToken: '' referer: 'ecash' platform: 'web' api_setCookie: 'false' email: 'tb-dominic+test1@outlook.com' isFromDevEnv: 'true' partnerName: 'expensify.com' HTTP_CF_BOT_SCORE: '67' HTTP_CF_JA3_HASH: 'a685258a6b9efe7d51633cf248ba2c83' 89a331daa8a71096-HKG virt2.rno 2024-06-27 05:56:35 757 tb-dominic+test1@outlook.com Bedrock\Client - Starting a request ~~ command: 'UpdateMoneyRequestDescription' clusterName: 'auth' headers: '[authToken: '' transactionID: '2329627498831684845' modifiedExpenseReportActionID: '2475839487821295989' comment: 'hello worldhello it's me' idempotent: '1' logParam: 'tb-dominic+test1@outlook.com' urlToNewDot: 'https://new.expensify.com/' shouldReadUsingThreads: '' shouldHandleOnyxUpdates: '1' clientUpdateID: '476617897' maxNumberOfUpdates: '500' requestID: '89a331daa8a71096-HKG' lastIP: '42.117.149.6' writeConsistency: 'ASYNC' priority: '500' timeout: '110000']' ```

4. We'll need to look into why it's being stripped and how to make UpdateMoneyRequestDescription work the same as updating task description and room description

[MariaHCD]

Or maybe the label doesn't work if I apply it? @kadiealexander what's the best way to get another engineer on this issue? 👀

[MariaHCD]

Lifesaver @mountiny is taking over for me 🙏🏼

[mountiny]

Making this weekly as its not that high value

[mountiny]

I was not able to help with this one as I was also partially out. Still seems to require some backend fix @MariaHCD do you want to take it back over :D

[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? 💸

[mountiny]

This is still a low priority

[mountiny]

Havent got around yet

[mountiny]

Focusing on closing wave-collect