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

Chat - Markdown entered in email responses don’t render in the app correctly #4899

Closed
kavimuru opened this issue Aug 28, 2021 · 21 comments
Closed
Assignees
Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Aug 28, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Send a message from userA to userB
  2. Wait for the email containing the message to be received on email by userB
  3. Respond to the email and include markdown in your response

Expected Result:

The markdown entered in the email response should render in the New Expensify app

Actual Result:

The message via email appears in New Expensify without markdown rendered correctly

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number:
1.0.88-2
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
Bug5212462_markdown

View all open jobs on GitHub

@MelvinBot
Copy link

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

@isagoico
Copy link

isagoico commented Aug 30, 2021

I also tested this using the markdowns in Gmail and message displayed only the italic markdown. Also the Quoted text is not rendered in App.

  • New Expensify:

image

  • Email:

image

@flodnv flodnv removed their assignment Aug 31, 2021
@MelvinBot MelvinBot removed the Overdue label Aug 31, 2021
@flodnv flodnv added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Aug 31, 2021
@MelvinBot
Copy link

Triggered auto assignment to @trjExpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Aug 31, 2021
@flodnv
Copy link
Contributor

flodnv commented Aug 31, 2021

Nice shot @MelvinBot 😄

@trjExpensify I don't think we really want to fix this? Or if we do, I'd say low priority at best?

@trjExpensify
Copy link
Contributor

We can mark this issue as external to be worked on by a contributor.

@trjExpensify trjExpensify added the External Added to denote the issue can be worked on by a contributor label Aug 31, 2021
@MelvinBot MelvinBot added Weekly KSv2 Daily KSv2 and removed Daily KSv2 Weekly KSv2 labels Aug 31, 2021
@trjExpensify trjExpensify added the Improvement Item broken or needs improvement. label Aug 31, 2021
@trjExpensify trjExpensify changed the title Chat - Markdowns don't appear when answering to a conversation via email. Chat - Markdown entered in email responses don’t render in the app correctly Aug 31, 2021
@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 31, 2021
@MelvinBot
Copy link

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

@thienlnam
Copy link
Contributor

I think this may actually end up being an internal issue since the way we handle email responses is through mailgun and this might be a case of us not parsing or converting the markdown into html

@trjExpensify
Copy link
Contributor

Cool, if that transpires to be the case I can pull the job post down.

@trjExpensify
Copy link
Contributor

No takers on Upwork yet, Melv.

@roryabraham roryabraham added Internal Requires API changes or must be handled by Expensify staff and removed Exported labels Sep 9, 2021
@roryabraham
Copy link
Contributor

roryabraham commented Sep 9, 2021

@trjExpensify Please take down the Upwork job for this issue, then feel free to unassign yourself.

@roryabraham roryabraham removed their assignment Sep 9, 2021
@roryabraham roryabraham added Weekly KSv2 and removed Daily KSv2 labels Sep 9, 2021
@chiragsalian
Copy link
Contributor

Started working on this today. i was playing around with Parsedown to see if it would help us in this scenario. It has some limitations and as I continued working on it there is a chance we don't even need it. So basically i'm trying to understand our usecase, from the issue description it talks about markdowns i.e., special characters like ** or ~~ to determine the formatting. But from the very next example here its not markdown but those are just regular formatting options. Those are most likely just plain HTML that we're either stripping out when we shouldn't be or updating App to those elements.

Eg of markdown bold: *Test Me*
Eg of formatted style for bold: <b>Test Me</b>

If you bold a text in gmail we'll receive its formatted style. Our app currently does not recognize <b> and instead uses <strong> for bold.

So anyway I think we should not be looking at converting markdown from the emails and instead just allowing formatted email styles to show. Since the latter is more common and I'm not sure why would anyone send an email with markdown text. Let me know if that makes sense or if you guys feel differently.

@MelvinBot MelvinBot removed the Overdue label Sep 18, 2021
@thienlnam
Copy link
Contributor

thienlnam commented Sep 21, 2021

My bad, my comment about parsing emails into markdown was not accurate

instead just allowing formatted email styles to show

We already do this mostly, we get HTML from emails but have to strip them of quoted content and images (since we process those separately and not inline) here.

Quoted content being stripped is expected for previous email replies but we may need to update our parser to only strip out previous email content instead of all quoted content

Our app currently does not recognize and instead uses for bold.

This is probably something we can add into expensimark

@thienlnam
Copy link
Contributor

thienlnam commented Sep 21, 2021

I was chatting with @chiragsalian about this 1:1 but I don't think we should support markdown entered in emails like the issue description mentioned. Instead, they should just use the included formatting that email providers allow. I think this Problem/Solution to this would be:

Problem: Formatting used in emails is not being formatted correctly in the app
Solution: Add support for <b> and <strike> tags in Expensimark

@chiragsalian
Copy link
Contributor

Got a bit sidetracked due to other priorities and then I've been OOO this week cause I'm helping with the career fair. I should get back to this issue soon.

@chiragsalian
Copy link
Contributor

Demoting to monthly since i'm focusing on other tasks while this still has n6-hold on it.

@MelvinBot MelvinBot removed the Overdue label Oct 8, 2021
@chiragsalian chiragsalian added Monthly KSv2 and removed Weekly KSv2 labels Oct 8, 2021
@chiragsalian
Copy link
Contributor

Was focusing on other priorities. Now that my plate is clear i should get back to this soon.

@chiragsalian
Copy link
Contributor

Fixed, I'm adding support for bold, italics, and strikethrough from emails. The changes were easier than I thought, I just had to update our react-native-render-html since support for bold was added recently. Italics always worked. Strikethrough was stripped in the backend so I'm allowing it now and react-native-render-html works perfectly with strike tags as well.

If there are additional styles needed we can add them in on a case-to-case basis.

PR in review.

@chiragsalian chiragsalian added the Reviewing Has a PR in review label Dec 3, 2021
@botify botify closed this as completed Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests