[HOLD for payment 2024-06-06] [$250] [Held requests] Reason appears bolded briefly after holding the request

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

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

Action Performed:

1. Go to staging.new.expensify.com
2. Create a manual request in the workspace chat.
3. Create a second request if there is only one request as Hold request option is not showing in the combined report.
4. Go to transaction thread.
5. Click 3-dot menu > Hold request.
6. Enter a reason and save it.

Expected Result:

The reason in the transaction thread will not appear bolded.

Actual Result:

The reason in the transaction thread is bolded briefly after holding the request.

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Bug6453293_1713425278170.hold_request_bold.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012e7d4583f2b70df4
• Upwork Job ID: 1780902555749208064
• Last Price Increase: 2024-04-18
• Automatic offers:
• GandalfGwaihir | Contributor | 0
• hoangzinh | Contributor | 0

Issue Owner

Current Issue Owner: @trjExpensify

[melvin-bot]

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

[melvin-bot]

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

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

[izarutskaya]

@trjExpensify I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

[izarutskaya]

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

[allgandalf]

@trjExpensify, This issue was reported by me on slack, I also Proposed the fix over there. (this issue is reproducible on production as well)

c.c. @shawnborton @dannymcclain @robertjchen

[shawnborton]

Yup, I agree that @GandalfGwaihir reported this issue first.

[allgandalf]

The issue here is that the style doesn't match with the BE message returned, we shouldn't bold/faint the optimistically set message because we don't do the same when we set the text from BE returned message

Proposal

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

We apply style to optimistically set messages, but this is wrong we shouldn't set any style to it.

What is the root cause of that problem?

When we Hold a Request and add a comment to it, at that time we set the comment optimistically, but the optimistically set comment is bold and the BE returns unbold comment, this causes a mismatch in style.

The RCA is because we set the comment to be of text type in optimistically generated comment
This is wrong and we should consider the comment as the type comment as defined in consts.

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

We need to remove the style, update type as comment , and add an extra prop html and set it to comment:

message: [
            {
                type: CONST.REPORT.MESSAGE.TYPE.COMMENT,
                text: comment,
                html: comment,
            },
        ],

Note: HTML parsing isn’t currently supported for hold requests but is being worked on over here #37015, hence the reason i have kept html prop to be just comment. and this issue is about the style of the hold request anyway and not about the rendering type? Also, if we set the parsed comment in html prop then too it will only be set optimistically(not the same parsing will remain once we get data from BE) because the BE still returns unparsed text (this issue is actually working on the linked issue above, so those people involved in the linked issue have more context), so i guess my proposal would solve the main outlined issue of this bug

What alternative solutions did you explore? (Optional)

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~012e7d4583f2b70df4

[melvin-bot]

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

[tienifr]

Proposal

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

The reason in the transaction thread is bolded briefly after holding the request.

What is the root cause of that problem?

In here, we're setting the type of the hold comment incorrectly.

After this PR, the held message was broken down into 2 parts, 1 system message and 1 normal comment, however we didn't handle the "normal comment" part right.

So it will fall to this case of the action user login comment and show as bold (the message type currently is CONST.REPORT.MESSAGE.TYPE.TEXT)

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

If we look at the correct handling for "normal comment" here, we'll see that:

• The type in message[0] should be CONST.REPORT.MESSAGE.TYPE.COMMENT
• The html should be the retrieved after getParsedComment on the passed in text (see here)
• The text will be retrieved by applying htmlToText on the html (see here)

We need to handle the hold comment similarly (since by right it's a normal comment, which is also editable as a normal comment after holding API request is successful).

To do this we need to add to here

const parsedComment = getParsedComment(comment ?? '');
    
const parser = new ExpensiMark();
const textForNewComment = parser.htmlToText(parsedComment);

Then update this to

{
    type: CONST.REPORT.MESSAGE.TYPE.COMMENT,
    text: textForNewComment,
    html: parsedComment,
},

We also needs to update the back-end similarly so it parses the hold comment correctly.

If we simply set html to comment, there'll be a bug where if we send a markdown comment like

`test markdown`

It will not show as code block correctly in the message list, and when edit that hold comment to

`test markdown` with text

It will now render as code block.

That will also be inconsistent when we try to send

`test markdown`

In the composer directly

Besides the above change, we should compare the optimistic chat comment logic and the optimistic hold comment logic to make sure we're not missing out any other field. If we do, will need to update so it's similar to the chat comment logic.

What alternative solutions did you explore? (Optional)

We could even apply live-markdown input to the Hold comment if we want to, but this is likely out of scope.

[dragnoir]

@paultsimura I think this should be onHold for #39452

based on this comment #39452 (comment) there's a change from HOLDCOMMENT to ADDCOMMENT and we requested a BE update.

[allgandalf]

Due to some compatibility issues we are using ADDCOMMENT moving forward.

By those comments which you linked, it seems they won't change the BE, so BE won't be updated :) so I guess we can proceed with this issue as usual c.c. @robertjchen

Can you confirm that this issue can be treated separately ? @robertjchen

[melvin-bot]

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

[melvin-bot]

📣 @dragnoir You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

[dragnoir]

All done, OP updated. But the upwork link said "This job is no longer available."

[trjExpensify]

Cool, I'll sort that out when I pay.

[allgandalf]

Update: The other PR is now on staging

[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.4.77-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: Hold reason not bold #39452

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

For reference, here are some details about the assignees on this issue:

• @hoangzinh requires payment automatic offer (Contributor)
• @dragnoir requires payment (Needs manual offer from BZ)
• @GandalfGwaihir requires payment automatic offer (Contributor)

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

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

[allgandalf]

This should be ready for payment :)

[trjExpensify]

Checklist time, please. :)

[hoangzinh]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR: [Held requests] break held message into separate system message + comment #38738
• 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/38738/files#r1631873521
• 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: N/A
• Determine if we should create a regression test for this bug: I think we shouldn't. It's just a small UI issue.

[robertjchen]

Awesome, thanks- I think we can pay and close this one out @trjExpensify ? 🙏

[trjExpensify]

Thanks! Payment summary as follows:

$250 to @hoangzinh for the C+ review
$250 to @allgandalf for the fix

@allgandalf paid! @hoangzinh can you accept the offer please?

[hoangzinh]

Accepted. Thanks @trjExpensify

[trjExpensify]

Paid, closing!