Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$250] [Held requests] Markdown isn't applied to the "Hold" message #37015

Closed
5 of 6 tasks
kavimuru opened this issue Feb 21, 2024 · 42 comments
Closed
5 of 6 tasks

[$250] [Held requests] Markdown isn't applied to the "Hold" message #37015

kavimuru opened this issue Feb 21, 2024 · 42 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Feb 21, 2024

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.43.0
Reproducible in staging?: y
Reproducible in production?: new feature
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
Expensify/Expensify Issue URL:
Issue reported by: applause internal team
Slack conversation:

Action Performed:

  1. Create an IOU in a chat
  2. Put it on hold
  3. Format the reason with markdowns

Expected Result:

Markdown should be applied.

Actual Result:

Markdown isn't applied to the "Hold" message.

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

Add any screenshot/video evidence

Bug6386836_1708519840035!img_1393

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011f83ef4d2ad62f49
  • Upwork Job ID: 1760293330198360064
  • Last Price Increase: 2024-03-25
  • Automatic offers:
    • hoangzinh | Reviewer | 0
@kavimuru kavimuru added DeployBlockerCash This issue or pull request should block deployment External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 21, 2024
@melvin-bot melvin-bot bot changed the title IOU - Markdown isn't applied to the "Hold" message [$500] IOU - Markdown isn't applied to the "Hold" message Feb 21, 2024
Copy link

melvin-bot bot commented Feb 21, 2024

Job added to Upwork: https://www.upwork.com/jobs/~011f83ef4d2ad62f49

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 21, 2024
Copy link

melvin-bot bot commented Feb 21, 2024

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

Copy link

melvin-bot bot commented Feb 21, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Feb 21, 2024
Copy link
Contributor

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

Copy link

melvin-bot bot commented Feb 21, 2024

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

@ghost
Copy link

ghost commented Feb 21, 2024

Proposal

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

IOU - Markdown isn't applied to the "Hold" message

What is the root cause of that problem?

The root cause of this issue is we are not passing RNMarkdownTextInput in the below-mentioned component.

https://github.com/Expensify/App/blob/20ca041595e76cdd289977e55b2af3fbb9618868/src/pages/iou/HoldReasonPage.tsx

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

We need to pass it and add it to the component so that the Markdowns are added and applied to the "Hold" message.

What alternative solutions did you explore? (Optional)

N/A

@Krishna2323
Copy link
Contributor

Proposal

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

IOU - Markdown isn't applied to the "Hold" message

What is the root cause of that problem?

We are not parsing comment with using const parsedDescription = ReportUtils.getParsedComment(newValue); before making api request to HoldRequest.

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

Use ReportUtils.getParsedComment to parse comment in putOnHold util function.

Result

@flodnv
Copy link
Contributor

flodnv commented Feb 21, 2024

@BartoszGrajdek @MonilBhavsar @robertjchen I see you in #33897, can you please have a look here?

@shahinyan11
Copy link
Contributor

shahinyan11 commented Feb 21, 2024

Proposal

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

What is the root cause of that problem?

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

  1. Update the comment here and here with getParsedComment(comment)
  2. Use RenderCommentHTML component instead Text here
<RenderCommentHTML
    source={source}
    html={fragment.text} // Or fragment.html after changes to the backend side mentioned in Step 3
/>
Screen.Recording.2024-02-21.at.19.09.13.mov
  1. If you noticed in above video, the markdown disappears after the refresh. it is because there need updates in backbend too . The hold action will not contain html key in messages or originalMessage in response

Screenshot 2024-02-21 at 18 49 00

What alternative solutions did you explore? (Optional)

@dragnoir
Copy link
Contributor

dragnoir commented Feb 21, 2024

Proposal

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

On hold message not shown as HTML

What is the root cause of that problem?

The message from description for HOLD reason is saved as simple string

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

There's 2 main areas that needs to be fixed.

1 - use ReportUtils.getParsedComment(comment)

We need to save the message as parsed before passed to Onyx here

function putOnHold(transactionID: string, comment: string, reportID: string) {

function putOnHold(transactionID: string, comment: string, reportID: string) {
+	const  parsedComment  =  ReportUtils.getParsedComment(comment);
    const createdReportAction = ReportUtils.buildOptimisticHoldReportAction();
-    const createdReportActionComment = ReportUtils.buildOptimisticHoldReportActionComment(comment);
+	const createdReportActionComment = ReportUtils.buildOptimisticHoldReportActionComment(parsedComment);

   // ...

    API.write(
        'HoldRequest',
        {
            transactionID,
-           comment,
+           parsedComment
            reportActionID: createdReportAction.reportActionID,
            commentReportActionID: createdReportActionComment.reportActionID,
        },
        {optimisticData, successData, failureData},
    );
}

2- Use <RenderHTML> not <Text>

After adding the ReportUtils.getParsedComment(comment) the UI will display the markdown as it it is, something like <em> text </em>, so we need to use RenderHTML component here

children = <ReportActionItemBasicMessage message={translate('iou.heldRequest')} />;

We can add a new prop to ReportActionItemBasicMessage like shouldDisplayHTML or use another component tthat render comment as HTML

POC video:

20240326_111057.mp4

@shahinyan11
Copy link
Contributor

Proposal

Updated

@robertjchen robertjchen changed the title [$500] IOU - Markdown isn't applied to the "Hold" message [HOLD #36979] [$500] IOU - Markdown isn't applied to the "Hold" message Feb 21, 2024
@robertjchen robertjchen removed the DeployBlockerCash This issue or pull request should block deployment label Feb 21, 2024
@dragnoir
Copy link
Contributor

I suggested using RenderCommentHTML

it's the same :)

@hoangzinh
Copy link
Contributor

@robertjchen Something went wrong with the API, it returns "Auth HoldRequest returned an error" every time request Hold request

Screenshot 2024-03-27 at 21 24 46

@robertjchen
Copy link
Contributor

Hm, the backend logs report that the comment field is missing when calling the HoldRequest API comand. Can you please try again on a fresh request with a comment when holding? 🤔

@hoangzinh
Copy link
Contributor

@robertjchen My bad. Btw, I found that the hold comment returned from BE having "actionName" is ADDCOMMENT
Screenshot 2024-03-28 at 18 01 24

But when we build optimistic data in FE, it's a HOLDCOMMENT action name

actionName: CONST.REPORT.ACTIONS.TYPE.HOLDCOMMENT,

So is BE or FE returning correct actionName?

@robertjchen
Copy link
Contributor

Good question, I'll update the backend to return HOLDCOMMENT as well for consistency 🤔

@hoangzinh
Copy link
Contributor

Thanks for proposals, everyone.

  • @godofoutcasts94 proposal and @Krishna2323 proposal found out root cause but your solutions are not completed yet. We need to use RenderHTML to render markdown as well.
  • @shahinyan11 proposal is invalid because it doesn't contain RCA + re-state the problem

Based on this comment. I'm inclining to @dragnoir proposal. But I have a suggestion for his solution, I think he can use RenderHTML directly instead of passing a prop to ReportActionItemBasicMessage

Link to selected proposal #37015 (comment)

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Apr 1, 2024

Current assignee @robertjchen is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@robertjchen
Copy link
Contributor

Thanks for the review, the backend change should be going out later today so we can proceed here 👍

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 1, 2024
Copy link

melvin-bot bot commented Apr 1, 2024

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

Copy link

melvin-bot bot commented Apr 1, 2024

📣 @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
Copy link
Contributor

dragnoir commented Apr 2, 2024

Upwork said "This job is no longer available." is it OK to proceed with the PR?

@hoangzinh
Copy link
Contributor

I think just continue with the PR @dragnoir. The BZ team will handle it later.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Apr 2, 2024
@dragnoir
Copy link
Contributor

dragnoir commented Apr 2, 2024

PR ready for review #39452

@trjExpensify trjExpensify changed the title [$250] IOU - Markdown isn't applied to the "Hold" message [$250] [Held requests] Markdown isn't applied to the "Hold" message Apr 4, 2024
@abekkala
Copy link
Contributor

abekkala commented Apr 8, 2024

Fix: @dragnoir
PR Review: @hoangzinh

PR not merged yet

Copy link

melvin-bot bot commented May 1, 2024

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

@melvin-bot melvin-bot bot added the Monthly KSv2 label May 1, 2024
@hoangzinh
Copy link
Contributor

We're still working on the PR #39452

@trjExpensify
Copy link
Contributor

I'm suggesting we close this issue, to reflect where we've landed in the PR. #40408 (comment) - any thoughts, comment there. CC: @JmillsExpensify

@JmillsExpensify
Copy link

Yes, agreed. Let's close.

@trjExpensify
Copy link
Contributor

Cool, thanks ya'll!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review
Projects
Archived in project
Development

No branches or pull requests