[$250] Don't hyperlink descriptions that show in expense previews

[JmillsExpensify]

We added hyperlinking to the description field sometime last year, though this creates the confusing situation that an expense preview is sometimes hyperlinked (specifically when an expense has no merchant, but a description), and then sometimes not (when an expense has a merchant). Example below.

To resolve this confusion we've decided to:

1. No longer show hyperlinks in any expense previews. This creates a consistent experience no matter whether a merchant appears or not.
2. Continue hyperlinking the description field in the transaction thread, like so.
   

As a result, this issue is focused on step one, and no longer showing hyperlinks specifically in the expense preview. It's technically a bug due to the lack of consistency so I'll put this one through the BZ team. Feel free to file in #wave-control.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0135edc95715d2d0f1
• Upwork Job ID: 1781372447966294016
• Last Price Increase: 2024-04-26
• Automatic offers:
• akinwale | Reviewer | 0
• bernhardoj | Contributor | 0

[melvin-bot]

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

[allgandalf]

Can you upload the videos again @JmillsExpensify , the current ones aren't visible

[bernhardoj]

Proposal

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

We don't want to hyperlink the money request description anymore.

What is the root cause of that problem?

We convert the text to markdown for every type of markdown, including link.

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

If we don't want to hyperlink it, then we need to exclude link/autolink rule from the expensimark replace logic. We can do that by passing filterRules like this:

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Lines 297 to 301 in ed34d59

	{shouldShowDescription && (
	<View style={[styles.breakWord, styles.preWrap]}>
	<RenderHTML html={`<muted-text>${parser.replace(merchantOrDescription)}</muted-text>`} />
	</View>
	)}

to

<RenderHTML html={`<muted-text>${parser.replace(merchantOrDescription, {filterRules: parser.rules.filter((rule) => rule.name !== 'autolink' && rule.name !== 'link').map((rule) => rule.name)})}</muted-text>`} />

We need to do this in ReportPreview too.

App/src/components/ReportActionItem/ReportPreview.tsx

Line 235 in ed34d59

const parsedSubtitle = new ExpensiMark().replace(formattedDescription ?? moneyRequestComment);

filterRules expect the list of rule that we want to apply, not we want to exclude, so I think we can create a util file to get the list of expensimark rule and exclude the ones that we pass to the param. I think it's easier than flipping the behavior of filterRules
ParserUtils.ts

function getFilterRules(excludeRules: string[]) {
    const parser = new ExpensiMark();
    return parser.rules.filter((rule) => !excludeRules.includes(rule.name)).map((rule) => rule.name)});
}

<RenderHTML html={`<muted-text>${parser.replace(merchantOrDescription, {filterRules: ParserUtils.getFilterRules(['link', 'autolink'])})}</muted-text>`} />

What alternative solutions did you explore? (Optional)

If we don't want to render any markdown, we can remove the RenderHTML usage and use the merchant Text component to show the description too.

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Lines 297 to 302 in 10fc359

	{shouldShowDescription && (
	<View style={[styles.breakWord, styles.preWrap]}>
	<RenderHTML html={`<muted-text>${parser.replace(merchantOrDescription)}</muted-text>`} />
	</View>
	)}
	{shouldShowMerchant && <Text style={[styles.textLabelSupporting, styles.textNormal]}>{merchantOrDescription}</Text>}

 {(shouldShowMerchant || shouldShowDescription) && <Text style={[styles.textLabelSupporting, styles.textNormal]}>{merchantOrDescription}</Text>}

[melvin-bot]

@kevinksullivan Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@kevinksullivan Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0135edc95715d2d0f1

[melvin-bot]

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

[kevinksullivan]

@GandalfGwaihir what videos are you referring to?

[akinwale]

@GandalfGwaihir what videos are you referring to?

They're not videos. They're the screenshots in the OP. Here's what the post looks like for me.

[melvin-bot]

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

[akinwale]

@bernhardoj's proposal makes sense here.

🎀👀🎀 C+ reviewed.

[melvin-bot]

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

[puneetlath]

Hmm, interesting.

We convert the text to markdown for every type of markdown, including link.

So @JmillsExpensify we want to support all other markdown except hyperlinks in the preview?

[melvin-bot]

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

[JmillsExpensify]

Huh, good question. I think the idea is that we don't show markdown anywhere in the preview. That allows us to always be in control of consistently showing information in the preview. e.g. We don't support markdown for the merchant, so it's inconsistent to show it one place and not the other.

[puneetlath]

Ok, cool, makes sense. @akinwale @bernhardoj is that what the proposal does?

[melvin-bot]

@puneetlath @akinwale @kevinksullivan 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!

[allgandalf]

Proposal

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

We should not support markdown / html for description in money request preview

What is the root cause of that problem?

We show normal text for merchant, but we render a HTML for description.

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Lines 297 to 302 in addcbfb

	{shouldShowDescription && (
	<View style={[styles.breakWord, styles.preWrap]}>
	<RenderHTML html={`<muted-text>${parser.replace(merchantOrDescription)}</muted-text>`} />
	</View>
	)}
	{shouldShowMerchant && <Text style={[styles.textLabelSupporting, styles.textNormal]}>{merchantOrDescription}</Text>}

For this issue we need to remove the markdown from the description as well

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

We should remove the HTML parsing over here, and now that description supports live markdown we should make use of htmlToText from Expensify common library:

But before that, we receive the text of description without any parsing from the backend:

So first if we decide to show description then we need to parse the description

diff --git a/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx b/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx
index 7f70a3e538..1dba2b9a2c 100644
--- a/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx
+++ b/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx
@@ -113,7 +113,7 @@ function MoneyRequestPreviewContent({
 
     let merchantOrDescription = requestMerchant;
     if (!shouldShowMerchant) {
-        merchantOrDescription = description || '';
+        merchantOrDescription = ReportUtils.getParsedComment(description) || '';
     }
 
     const receiptImages = hasReceipt ? [ReceiptUtils.getThumbnailAndImageURIs(transaction)] : [];
@@ -296,7 +296,7 @@ function MoneyRequestPreviewContent({
                                                 )}
                                                 {shouldShowDescription && (
                                                     <View style={[styles.breakWord, styles.preWrap]}>
-                                                        <RenderHTML html={`<muted-text>${parser.replace(merchantOrDescription)}</muted-text>`} />
+                                                        {parser.htmlToText(merchantOrDescription)}
                                                     </View>
                                                 )}
                                                 {shouldShowMerchant && <Text style={[styles.textLabelSupporting, styles.textNormal]}>{merchantOrDescription}</Text>}

Result Video:

Screen.Recording.2024-04-27.at.12.46.54.AM.mov

Note: The same can be done for reportPreview as well

[allgandalf]

have posted my proposal according to the latest comments, do let me know what you think of that @puneetlath @akinwale

[bernhardoj]

@puneetlath if I understand correctly, we don't want to render the markdown at all in the preview, so it will look like this

If that's correct, then that's what my alternative proposal suggests.

I have updated the alternative proposal to show the code too.

[puneetlath]

@akinwale what do you think?

[akinwale]

@puneetlath I think we can move forward with @bernhardoj's alternate solution which renders just plain text for the preview.

[melvin-bot]

📣 @akinwale 🎉 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]

📣 @bernhardoj 🎉 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 📖

[bernhardoj]

PR is ready

cc: @akinwale

[melvin-bot]

This issue has not been updated in over 15 days. @puneetlath, @akinwale, @kevinksullivan, @bernhardoj eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[kevinksullivan]

Automation missed this one, sorry for the delay. All set on payment