[HOLD for Payment] Room - Description field is empty when there is only one line description

[IuliiaHerets]

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: 9.0.25-0
Reproducible in staging?: Y
Reproducible in production?: N
Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to a room.
3. Click on the room header.
4. Click Description.
5. Add a single line description and save it.

Expected Result:

Description field will show the room description.

Actual Result:

Description field is empty when there is only one line description.

Workaround:

Unknown

Platforms:

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6583715_1724719134103.line.mp4

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @cristipaval (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot]

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

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[nkdengineer]

Edited by proposal-police: This proposal was edited at 2024-08-27 06:54:02 UTC.

Proposal

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

Description field is empty when there is only one line description.

What is the root cause of that problem?

We pass shouldTruncateTitle as true for the description item. Then it will call truncateHTML here

App/src/pages/ReportDetailsPage.tsx

Line 726 in d5cfecf

shouldTruncateTitle

In this function, we have a while loop that will execute if matches is not undefined and then we will get the truncatedContent in this loop. The problem here is if the room description doesn't have any HTML tag, matches is undefined then the loop doesn't execute that makes this function return an empty string.

https://github.com/Expensify/expensify-common/blob/d11466167bc562447d2dfdad624145040153efae/lib/ExpensiMark.ts#L1361

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

We can add comment tag when we call truncateHTML then the regex can always match the first time

if (shouldTruncateTitle) {
    titleToWrap = Parser.truncateHTML(`<comment>${titleToWrap}</comment>`, characterLimit, {ellipsis: '...'});
    return titleToWrap;
}

return titleToWrap ? `<comment>${titleToWrap}</comment>` : '';

App/src/components/MenuItem.tsx

Lines 491 to 493 in d5cfecf

	if (shouldTruncateTitle) {
	titleToWrap = Parser.truncateHTML(titleToWrap, characterLimit, {ellipsis: '...'});
	}

What alternative solutions did you explore? (Optional)

[cristipaval]

@nkdengineer could you also find the offending PR?

[nkdengineer]

@cristipaval This comes from this PR.

[shubham1206agra]

@nkdengineer Your RCA is correct. But the solution can be corrected from the App side without changing the actual truncate logic.

[nkdengineer]

if (shouldTruncateTitle && title !== Parser.htmlToText(title)) {
    titleToWrap = Parser.truncateHTML(titleToWrap, characterLimit, {ellipsis: '...'});
}

@shubham1206agra What do you think if we prevent calling truncateHTML if the title doesn't contain html tag.

[puneetlath]

@nkdengineer how about we wrap it in <comment> tags when passing to truncateHTML? Would you be able to raise a PR?

[nkdengineer]

@puneetlath Yes I can raise a PR now.

[nkdengineer]

Yeah I think that also is an option.

[nkdengineer]

@nkdengineer how about we wrap it in tags when passing to truncateHTML? Would you be able to raise a PR?

@puneetlath Confirmed, this is also a work solution.

[nkdengineer]

@shubham1206agra Updated proposal.

[puneetlath]

Ok please raise a PR now.

[nkdengineer]

@puneetlath The PR is ready.

[puneetlath]

I confirmed this is fixed on staging. Demoting.

[melvin-bot]

This issue has not been updated in over 15 days. @abekkala, @cristipaval, @nkdengineer 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!

[abekkala]

@puneetlath are you able to confirm if this is an issue of the automation failing or has this one really not been deployed to prod yet?

[puneetlath]

This one should be on production as of a couple weeks ago. Right @nkdengineer?

[nkdengineer]

@puneetlath Yes.

[abekkala]

PAYMENT SUMMARY:

• Fix: @nkdengineer Offer
• PR Review: @shubham1206agra Offer

[abekkala]

Offers are still pending.
@nkdengineer, @shubham1206agra, if you can accept the offers in Upwork, I can get those paid out to you!

[shubham1206agra]

@abekkala Offer accepted

[abekkala]

@shubham1206agra payment sent and contract ended - thank you! 🎉

[abekkala]

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:

• [@shubham1206agra] The PR that introduced the bug has been identified. Link to the PR:
• [@shubham1206agra] 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:
• [@shubham1206agra] 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.
• [@shubham1206agra] Determine if we should create a regression test for this bug.
• [@shubham1206agra] 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.
• [@abekkala] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[shubham1206agra]

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:

• [@shubham1206agra] The PR that introduced the bug has been identified. Link to the PR: Truncation fix for Lack of maximum height for the description field #38890
• [@shubham1206agra] 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: https://github.com/Expensify/App/pull/38890/files#r1771463287
• [@shubham1206agra] 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. Not Required
• [@shubham1206agra] Determine if we should create a regression test for this bug. No as this was a deploy blocker
• [@shubham1206agra] 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.
• [@abekkala] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[nkdengineer]

if you can accept the offers in Upwork, I can get those paid out to you!

@abekkala Offer accepted too, ty

[abekkala]

@nkdengineer payment sent and contract ended - thank you! 🎉