Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.79k forks source link

[PAID] [$250] Search - Raw markdown symbols visible in search expense list #46828

Closed izarutskaya closed 3 weeks ago

izarutskaya commented 1 month ago

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.16-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): biruknew45+new@gmail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

  1. Go to [staging.new.expensify.com].
  2. Navigate to any chat.
  3. Click on the plus icon in the compose box.
  4. Select "Submit Expense."
  5. Enter the amount and press "Next."
  6. Submit the expense.
  7. Navigate to the expense detail page.
  8. Click on the description field.
  9. Type a header markdown (e.g., # text) or any type of mark down text and click "Save."
  10. Navigate to the search page located next to "Inbox."

Expected Result:

Markdown syntax should be rendered correctly, so any Markdown formatting appears as intended (e.g., bold text, header ) without displaying raw Markdown symbols.

Actual Result:

Raw Markdown symbols are visible in the expense list on the search page, indicating that Markdown is not being rendered properly in this context which is different from expense description behavior in other places in the app

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6551623_1721834797626!2

https://github.com/user-attachments/assets/845e1ea4-69e8-44b5-b93c-1c460fc5096f

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0118ebdaf7338382b5
  • Upwork Job ID: 1824560866443816441
  • Last Price Increase: 2024-08-16
  • Automatic offers:
    • ishpaul777 | Reviewer | 103622954
    • Krishna2323 | Contributor | 103651370
Issue OwnerCurrent Issue Owner: @ishpaul777
melvin-bot[bot] commented 1 month ago

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

Krishna2323 commented 1 month ago

Proposal

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

Search - Raw markdown symbols visible in search expense list

What is the root cause of that problem?

Description is not converted to text from markdown. https://github.com/Expensify/App/blob/412fc60dd5bd8c3da326fd48eae5202b6f7f1a08/src/components/SelectionList/Search/TransactionListItemRow.tsx#L126

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

We should convert the description to plain text.

const description = Parser.htmlToText(Parser.replace(TransactionUtils.getDescription(transactionItem)));

What alternative solutions did you explore? (Optional)

daledah commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-08-07 07:29:00 UTC.

Please insert the actual timestamp where {updated_timestamp} is indicated.

Proposal

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

Raw Markdown symbols are visible in the expense list on the search page, indicating that Markdown is not being rendered properly in this context which is different from expense description behavior in other places in the app

What is the root cause of that problem?

The expense list uses TextWithTooltip to display text, however TextWithTooltip doesn't have logics to handle HTML text, so the output only shows raw text.

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

Add an option shouldRenderAsHTML to TextWithTooltip and set default to false. Then we edit this line to render as HTML if shouldRenderAsHTML is true:

shouldRenderAsHTML ? <RenderHTML html={Parser.replace(text)} /> : text}

POC

https://github.com/user-attachments/assets/1f7049d8-5067-4956-afb9-863c5f388757

What alternative solutions did you explore? (Optional)

daledah commented 1 month ago

# PROPOSAL UPDATED

I updated the proposal to include pseudocode.

strepanier03 commented 1 month ago

Hmmm, the description is not a visible field on the expense when I'm on the search page, so the markdown isn't displayed either way for me.

image image
daledah commented 1 month ago

@strepanier03 With the current logic, if the expense's merchant is set, the app will display the merchant field instead of the description.

melvin-bot[bot] commented 1 month ago

@strepanier03 Eep! 4 days overdue now. Issues have feelings too...

strepanier03 commented 1 month ago

Got it, thanks @daledah - I'll retest with that in mind.

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~0118ebdaf7338382b5

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

@strepanier03 @ishpaul777 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!

ishpaul777 commented 1 month ago

I will review proposals tomorrow (Tuesday)

ishpaul777 commented 1 month ago

Proposal from @daledah Sounds good to me! but lets be sure to use shouldRenderAsHTML only when rendering description not for merchat.

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

πŸ“£ @ishpaul777 πŸŽ‰ 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] commented 1 month ago

πŸ“£ @daledah 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 πŸ“–

daledah commented 1 month ago

@ishpaul777 This PR is ready for review.

Krishna2323 commented 1 month ago

@ishpaul777, sorry for delay, but I believe that using RenderHTML will break the design and alignment, please try with multiline description and pre code blocks.

https://github.com/user-attachments/assets/22efb8e2-8472-4321-8813-d26c2af2250c

https://github.com/user-attachments/assets/edfe09d0-00a3-4cbd-9d2b-e3be5399b9b8

cc: @carlosmiceli

ishpaul777 commented 1 month ago

The only difference b/w both proposal seems to be with how they handle code blocks right @Krishna2323 or there is any other difference (I think showing the link in blue is the expected behviour same as in menu item)

Krishna2323 commented 1 month ago

@ishpaul777, my proposal will only remove the markdown character like #, ~~, __ while the other one will render the text as html which I think isn't correct in this case.

I think showing the link in blue is the expected behviour same as in menu item

But that wouldn't work, as shown in the video above.

ishpaul777 commented 1 month ago
Screenshot 2024-08-22 at 10 46 23β€―PM

But that wouldn't work, as shown in the video above.

Also doesn't work for menu item, i think code block is only edge case here so i am inclined to show it as markdown rather than showing it text for it to be consistent with other places

https://github.com/user-attachments/assets/6fc26089-b11a-406f-9953-73188344477b

Krishna2323 commented 1 month ago

@ishpaul777, I mean it blocks the press on transaction item, clicking on the link does nothing and that will break the current behaviour.

ishpaul777 commented 1 month ago

Thats a good point i wonder if we can make it clickable to link to transaction details report @daledah if not than we can rethink on a the expected behaviour

Krishna2323 commented 1 month ago

clickable to link to transaction details report

I this also has some issues because if we are rendering it as link then why would we link to transaction details report, I believe what we have now is partially correct, we should just remove the markdown symbols. You can see that from and to fields are also not linking to the users page, it just opens the transaction details.

ishpaul777 commented 1 month ago

I believe what we have now is partially correct, we should just remove the markdown symbols.

@carlosmiceli @strepanier03 Does that sound good to you, i.e we will not render the description as markdown in description column we will just remove all markdown symbols (#, >, ``` etc.) and render it as normal text

carlosmiceli commented 1 month ago

I think so, can we see a video to confirm that it would look properly?

Krishna2323 commented 1 month ago

@carlosmiceli @ishpaul777,

https://github.com/user-attachments/assets/4072821d-02d4-4223-9a2d-e8049287b7f8

carlosmiceli commented 1 month ago

Yeah, I think this is good! Let's do it :)

ishpaul777 commented 1 month ago

Okay! Thanks for clarifying @carlosmiceli, can we please switch assignment to @Krishna2323

carlosmiceli commented 1 month ago

Sure, am I removing @daledah ?

ishpaul777 commented 1 month ago

Yes, because there are we edge cases if we render description as html (@daledah proposal) and it does not look so good.

melvin-bot[bot] commented 1 month ago

πŸ“£ @Krishna2323 πŸŽ‰ 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 πŸ“–

Krishna2323 commented 1 month ago

@ishpaul777, PR ready for review ^

strepanier03 commented 3 weeks ago

Looks like automation failed so I updated title and labels.

melvin-bot[bot] commented 3 weeks ago

@carlosmiceli, @strepanier03, @Krishna2323, @ishpaul777 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

strepanier03 commented 3 weeks ago

Payment summary

Thanks everyone!