[$1000] App recognizes indent as quote and edit message does not recognize multiple indent

kavimuru commented 1 year 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!

Action Performed:

Open the app in https://staging.new.expensify.com/r/88654499 and login with user A
Send message to user B
Open email of user B
Reply to user A via email and add multiple indents to text

Expected Result:

App should recognize the indent as normal space before text and it should be able to handle multiple indents properly

Actual Result:

App recognizes indents as quotes and thus it is also not able to handle multiple indents properly

Workaround:

unknown

Platforms:

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

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

Version Number: 1.2.71-1 Reproducible in staging?: y Reproducible in production?: y 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 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/218831766-f7032a2b-16fd-48cf-9eb2-689bc595e90e.mp4

https://user-images.githubusercontent.com/43996225/218831791-f760e749-2372-481a-8df2-0a363fe9ea11.mp4

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1676371867126689

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01b363da71d47ffb46
Upwork Job ID: 1625731313164894208
Last Price Increase: 2023-02-15

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[x] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[x] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

Christinadobrzyn commented 1 year ago

I can replicate this using the steps in the OP.
here's the outcome
I think this can be external so going to add the label to get some proposals

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01b363da71d47ffb46

MelvinBot commented 1 year ago

Current assignee @Christinadobrzyn is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

thienlnam commented 1 year ago

FYI @ctkochan22 @Christinadobrzyn I'm adding the Upwork Automation label to this to beta test the flow that will be prompting contributors to store their Upwork details

allroundexperts commented 1 year ago

This should be internal. We're receiving the response from the backend that includes block quotes.

MelvinBot commented 1 year ago

📣 @allroundexperts! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

MelvinBot commented 1 year ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

Christinadobrzyn commented 1 year ago

Should I move this to an internal job? If that's the case, it looks like @mananjadhav would be the C+ and we won't need an external contributor.

@ctkochan22 let me know your thoughts!

osofus commented 1 year ago

When I checked it out, I found the issue is with the client. The value that the server sends is correct. The client misrepresents the indent value. So it has to be fixed on the client. I will send a proposal later.

Contributor details Your Expensify account email: jobin@osofus.com Upwork Profile Link: https://www.upwork.com/freelancers/~01b8076a2d26bdb3b3

MelvinBot commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

osofus commented 1 year ago

Proposal

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

Indents are displayed as diminishing blocks in front of the text. Instead it should be displayed as normal space.

Also, when editing texts with multiple indents, the editor is unable to handle multiple indents. It just shows them as '>' at the beginning of the line, regardless of the number of indents, which always gets saved as a single indent.

What is the root cause of that problem?

Indents translate to <blockquote> in HTML.

The CSS of <blockquote> in our repository is: blockquote: { borderLeftColor: themeColors.border, borderLeftWidth: 4, paddingLeft: 12, marginTop: 4, marginBottom: 4, marginLeft: 0, }

which is what causes those diminishing blocks to appear.

The reason why muliple indents are translated to '>' is due to the parsing logic in node_modules-> expensify-common-> lib-> ExpensiMark.js. That library does not have the ability to convert multiple <blockquote>-s to multiple '>'-s or convert multiple '>'-s to multiple <blockquote>-s.

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

If the CSS of <blockquote> is updated to: blockquote: { paddingLeft: 12, marginTop: 0, marginBottom: 0, marginLeft: 0, }

then we will see space instead of diminishing blocks for indents. See attached image.

Regarding the abiliy to edit multiple indents, this has to be done in the external (ExpensiMark) library. Each <blockquote> should be translated to '>' and each '>' should be translated back to <blockquote>.

<div dir="ltr"><blockquote>Test indents.<br /></blockquote><blockquote><blockquote>Another indent.</blockquote></blockquote></div>

should translate to:

<div dir="ltr"> > Test indents.

> > Another indent. </div>

rather than:

<div dir="ltr"> > Test indents.

> Another indent. </div>

Also when translating back the text in the editor, it should translate each '>' to a <blockquote>. <div dir="ltr"> > Test indents.

> > Another indent. </div>

should translate back to:

<div dir="ltr"><blockquote>Test indents.<br /></blockquote><blockquote><blockquote>Another indent.</blockquote></blockquote></div>

However it is pointless to describe the specifics of what needs to be done in ExpensiMark, as it is an external library.

What alternative solutions did you explore?

None

ctkochan22 commented 1 year ago

Reviewing

ctkochan22 commented 1 year ago

Trying to reproduce locally, if you guys have tips let me know!

osofus commented 1 year ago

Trying to reproduce locally, if you guys have tips let me know!

Log out of Expensify and send yourself a message from another Expensify profile. When you get an email from Expensify about the message, reply to it from the email. Put indents in your message. Then log into the recipient Expensify acccount and look at the message. You will see the indents as diminishing blocks.

Please let me know if you have any questions or if I didn't understand you clearly and this is not what you were looking for.

ctkochan22 commented 1 year ago

@osofus thanks!

I actually meant in your dev environment

osofus commented 1 year ago

@osofus thanks!

I actually meant in your dev environment

It works the same if you spin up the client with npm run web. If you send a message to someone who is logged out, Expensify will send them an email with the message. The important part is to reply to the message from your email account and insert indents in your message.

Once the message is sent from your email, you can view it on the Expensify client, if you login as either the sender or receiver. If you login as the sender, then you can also edit the message on the client.

On the attachment below, you will see indents are denoted as <blockquote> in the gmail editor. So it is not an issue with the server converting the message incorrectly.

Please let me know if I didn't address your question correctly.

ctkochan22 commented 1 year ago

@osofus

Will your proposed solution affect how > is currently used in the App?

osofus commented 1 year ago

It will affect how > is currently used in the app. If you don't want to impact that, then the least we should do is:

blockquote: { borderLeftColor: themeColors.border, borderLeftWidth: 4, paddingLeft: 12, marginTop: 0, marginBottom: 0, // Overwrite default HTML margin for blockquotes marginLeft: 0, }

which will at least make the blocks non-diminishing.

Please let me know if you want me to propose a solution to fix the ExpensiMark library to treat multiple '>'-s as multiple <blockquote>-s, so multiple indents stay intact after editing.

ctkochan22 commented 1 year ago

So I don't think we want to change the behavior of the blockquote. This specific situation only occurs from gmail right?

I've never really loved solving issues with css. When looking at the logs, we already remove some html elements from Gmail in Mailgun/API.php in Web-Expensify (which is an email service we use for report comments among other things). This makes sense. I think we can take advantage of this here and directly handle and manipulate excessive blockquotes coming from email.

We can either:

Convert all blockquotes into spaces
Or simply remove them

Logs on the php side when receiving a message with indents

Starting to parse email HTML content ~~ emailProvider: 'gmail' shouldStripQuotedContent: '1' shouldStripBreakTags: '' unstrippedMessage: '<div dir="ltr">hello friend<blockquote style="margin:0 0 0 40px;border:none;padding:0px"><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div>potatoes</div></blockquote></div></blockquote></blockquote></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Feb 20, 2023 at 11:24 AM Kosuke Tseng Yoshioka &lt;<a href="mailto:replies%2B75937469@expensify.com">replies+75937469@expensify.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="white-space:pre-wrap">- Help paulo with test</span><br>#015#012<br>#015#012To respond, head over to <a href="https://new.expensify.com/r/75937469" target="_blank">new.expensify.com</a> or reply directly to this message.</blockquote></div>#015#012'

 Parsing email HTML, removing 2 total elements, which includes 2 text elements. ~~ removedTextElements: '[0: '[htmlTag: 'div' className: '"gmail_quote"' textValue: '"On Mon, Feb 20, 2023 at 11:24 AM Kosuke Tseng Yoshioka <replies+75937469@expensify.com> wrote:- Help paulo with test\r\n\r\nTo respond, head over to new.expensify.com or reply directly to this message."' reason: 'Element class contains a quote class']' 1: '[htmlTag: 'blockquote' className: '"gmail_quote"' textValue: '"- Help paulo with test\r\n\r\nTo respond, head over to new.expensify.com or reply directly to this message."' reason: 'Element class contains a quote class']']'

 Expensify\Mailgun\API - Mailgun add report comment params ~~ email: 'ctkochan22@gmail.com' reportID: '75937469' To: 'kosuke tseng yoshioka <replies+75937469@expensify.com>' comment: '<div dir="ltr">hello friend<blockquote><blockquote><div><blockquote><div>potatoes</div></blockquote></div></blockquote></blockquote></div>' messageID: '<CAHQ1pcdhNCTY1S_v1f=u1UeFCNz83kJrobkwHCYM_VSdcJ+5qA@mail.gmail.com>' cc: '[]' attachmentCount: '0'

Internal Notes on SO about how to test replying via email locally: https://stackoverflowteams.com/c/expensify/questions/5749

osofus commented 1 year ago

So I don't think we want to change the behavior of the blockquote. This specific situation only occurs from gmail right?

I've never really loved solving issues with css. When looking at the logs, we already remove some html elements from Gmail in Mailgun/API.php in Web-Expensify (which is an email service we use for report comments among other things). This makes sense. I think we can take advantage of this here and directly handle and manipulate excessive blockquotes coming from email.

We can either:

Convert all blockquotes into spaces
Or simply remove them

Logs on the php side when receiving a message with indents

Starting to parse email HTML content ~~ emailProvider: 'gmail' shouldStripQuotedContent: '1' shouldStripBreakTags: '' unstrippedMessage: '<div dir="ltr">hello friend<blockquote style="margin:0 0 0 40px;border:none;padding:0px"><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div>potatoes</div></blockquote></div></blockquote></blockquote></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Feb 20, 2023 at 11:24 AM Kosuke Tseng Yoshioka &lt;<a href="mailto:replies%2B75937469@expensify.com">replies+75937469@expensify.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="white-space:pre-wrap">- Help paulo with test</span><br>#015#012<br>#015#012To respond, head over to <a href="https://new.expensify.com/r/75937469" target="_blank">new.expensify.com</a> or reply directly to this message.</blockquote></div>#015#012'

 Parsing email HTML, removing 2 total elements, which includes 2 text elements. ~~ removedTextElements: '[0: '[htmlTag: 'div' className: '"gmail_quote"' textValue: '"On Mon, Feb 20, 2023 at 11:24 AM Kosuke Tseng Yoshioka <replies+75937469@expensify.com> wrote:- Help paulo with test\r\n\r\nTo respond, head over to new.expensify.com or reply directly to this message."' reason: 'Element class contains a quote class']' 1: '[htmlTag: 'blockquote' className: '"gmail_quote"' textValue: '"- Help paulo with test\r\n\r\nTo respond, head over to new.expensify.com or reply directly to this message."' reason: 'Element class contains a quote class']']'

 Expensify\Mailgun\API - Mailgun add report comment params ~~ email: 'ctkochan22@gmail.com' reportID: '75937469' To: 'kosuke tseng yoshioka <replies+75937469@expensify.com>' comment: '<div dir="ltr">hello friend<blockquote><blockquote><div><blockquote><div>potatoes</div></blockquote></div></blockquote></blockquote></div>' messageID: '<CAHQ1pcdhNCTY1S_v1f=u1UeFCNz83kJrobkwHCYM_VSdcJ+5qA@mail.gmail.com>' cc: '[]' attachmentCount: '0'

It's not just Gmail. Indents can be inserted in any email, including Hotmail etc. However if you think it is better to deal with it in Web-Expensify by formatting the incoming message, that's OK too. Please feel free to make this an internal issue, if you want to go down that path.

ctkochan22 commented 1 year ago

@osofus for sure, and thanks for your insight. Going to bring it up internally and see what we want

ctkochan22 commented 1 year ago

Posted in slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1676934809760599

I'm going to bring this internal. I'm pretty sure we will handle any <blockquote> elements in our PHP layer.

MelvinBot commented 1 year ago

Current assignee @mananjadhav is eligible for the Internal assigner, not assigning anyone new.

ctkochan22 commented 1 year ago

We are discussing in the slack thread if we want to address email bugs/features now or do we want to deal with them in the future in one project

JmillsExpensify commented 1 year ago

I think we should put this issue on hold so that we can:

Devise a holistic plan for email formatting issues and App
Agree on the features we'll support (or won't)
And generally make sure whatever solutions we employ, work toward these goals.

ctkochan22 commented 1 year ago

Still on hold for: https://github.com/Expensify/App/issues/15336

ctkochan22 commented 1 year ago

Discussions on next steps should be held here: https://github.com/Expensify/App/issues/15336

Christinadobrzyn commented 1 year ago

Still following https://github.com/Expensify/App/issues/15336#issuecomment-1447031010 - I think this can be moved to a weekly label while it's on hold. @mananjadhav feel free to move it back to daily if you think that's better!

Christinadobrzyn commented 1 year ago

Still on hold - https://github.com/Expensify/App/issues/15336#issuecomment-1467178183

Christinadobrzyn commented 1 year ago

Still on hold - https://github.com/Expensify/App/issues/15336#issuecomment-1467178183

Christinadobrzyn commented 1 year ago

Still on hold - https://github.com/Expensify/App/issues/15336#issuecomment-1467178183

Christinadobrzyn commented 1 year ago

Still on hold - https://github.com/Expensify/App/issues/15336#issuecomment-1467178183

Christinadobrzyn commented 1 year ago

This issue is closed - https://github.com/Expensify/App/issues/15336#issuecomment-1467178183

Not sure what's next with this issue @mananjadhav, what do you think we should do next?

mananjadhav commented 1 year ago

@Christinadobrzyn allow me to test this once, if it is still an open issue then we would decide the next steps.

mananjadhav commented 1 year ago

@Christinadobrzyn This is still a problem.

But the fix needs to be done from the backend. This is what I receive from the API.

<strong></strong><br /><br /><div dir=\"ltr\">Hello,<div>Message with multiple indent.</div><div><br /></div><blockquote><blockquote><div><blockquote><div><br /></div><div><br /></div></blockquote></div></blockquote></blockquote><blockquote><blockquote><blockquote>ANother</blockquote></blockquote></blockquote></div>

Christinadobrzyn commented 1 year ago

Okay thank you for testing @mananjadhav. I'll add an engineering label back to this issue to get something started internally.

melvin-bot[bot] commented 1 year ago

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

NikkiWines commented 1 year ago

Will try to take a look at this during the week!

Christinadobrzyn commented 1 year ago

We're still working on this - not overdue

NikkiWines commented 1 year ago

As mentioned here - will try to investigate this next week

NikkiWines commented 1 year ago

Dropping this down to monthly per this discussion

NikkiWines commented 1 year ago

Still not prioritized at the moment

NikkiWines commented 11 months ago

No update here

Christinadobrzyn commented 11 months ago

Related to our most recent company meeting and prioritising our engineering time, I think we should close this for now while we focus on other jobs. This is a niche situation so I think it can be tabled in favour of more urgent issues.

Feel free to let me know if you feel otherwise @NikkiWines and @mananjadhav!

Expensify / App