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

[HOLD for payment 2022-11-02] [Bug] [$500] HTML formatted messages render incorrectly #11320

Closed
mvtglobally opened this issue Sep 27, 2022 · 45 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review

Comments

@mvtglobally
Copy link

mvtglobally commented Sep 27, 2022

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. Create an html file with the content below and open it in the browser
    <p dir="ltr">Here's what I recommend for new members. </p><br /><p dir="ltr"><strong>Demo and onboarding calls</strong></p><ul><li dir="ltr"><p dir="ltr"><a href="https://www.expensify.com/inbox?taskID=ConciergeWelcome&amp;calltaskid=ConciergeWelcome" target="_blank" rel="noreferrer noopener">Request a call</a> — our Guides team is standing by to provide live demos and onboarding assistance for new customers.</p></li></ul><p dir="ltr"><strong>Self-paced learning</strong></p><ul><li dir="ltr"><p dir="ltr"><a href="https://community.expensify.com/discussion/5694/deep-dive-admin-training-and-setup-resources/p1?new=1" target="_blank" rel="noreferrer noopener">Admin training and resources</a> — step-by-step guide covering the basics of expense reporting for Policy Admins.</p></li><li dir="ltr"><p dir="ltr"><a href="https://community.expensify.com/discussion/5922/deep-dive-day-1-with-expensify-for-submitters/p1?new=1" target="_blank" rel="noreferrer noopener">Day 1 for Submitters/Employees</a> — a guide for employees submitting expenses for the first time.</p></li><li dir="ltr"><p dir="ltr"><a href="https://community.expensify.com/discussion/6018/deep-dive-day-1-with-the-expensify-card" target="_blank" rel="noreferrer noopener">Day 1 with the Expensify Card</a> — getting started with your new Expensify Card.</p></li><li dir="ltr"><p dir="ltr"><a href="https://community.expensify.com/discussion/8629/employee-training-e-learning-programme/p1?new=1" target="_blank" rel="noreferrer noopener">Employee Training E-Learning Program</a> — an in-depth training programme for users who like a comprehensive step-by-step approach.</p></li><li dir="ltr"><p dir="ltr"><a href="https://community.expensify.com/" target="_blank" rel="noreferrer noopener">community.expensify.com</a> — searchable resources for best practices and troubleshooting. </p></li></ul><p dir="ltr"><strong>Training videos</strong></p><ul><li dir="ltr"><p dir="ltr"><a href="https://expensify.wistia.com/medias/1kx52dx68e" target="_blank" rel="noreferrer noopener">Admin Onboarding</a> — will help to increase your knowledge of Expensify setup and administration.</p></li></ul>If you have any specific questions, don't hesitate to reach back out though! That's what <a href="https://community.expensify.com/discussion/5515/deep-dive-meet-concierge/p1?new=1" target="_blank" rel="noreferrer noopener">I'm here for!</a>
  2. Select all and copy
  3. Paste it in an email body and send the email to replies+<report_id>@expensify.com. The <report_id> can be obtained from a URL of a chat between your account and some other account. The subject of the email can be any.
    image

Video of steps reproducing the bug:

Screen.Recording.2022-10-11.at.11.52.42.AM.mov

Expected Result:

Messages should be properly formatted

Actual Result:

HTML formatted messages render incorrectly

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Mobile Web and Web (haven't checked native)

Version Number: 1.2.12-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screenshot_20220913-121211
Screenshot_20220913-121003

Expensify/Expensify Issue URL:
Issue reported by: @JmillsExpensify
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1663173660551449

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2022

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

@melvin-bot melvin-bot bot added the Overdue label Sep 29, 2022
@aldo-expensify
Copy link
Contributor

@mvtglobally can you detail a bit more what you mean by: "send some HTML messages", ideally providing some content we could copy paste to reproduce? I tried sending a message with HTML like <div>hello</div> and I just see the tags as plain text.

@melvin-bot melvin-bot bot removed the Overdue label Sep 29, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2022

@aldo-expensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Oct 3, 2022
@aldo-expensify
Copy link
Contributor

Tested again in web mobile / web not mobile/ android and IOS, couldn't reproduce. Maybe it fails with some specific html layout. @mvtglobally do you know from where that html was copied from?

@melvin-bot melvin-bot bot removed the Overdue label Oct 3, 2022
@aldo-expensify aldo-expensify added the Needs Reproduction Reproducible steps needed label Oct 3, 2022
@JmillsExpensify
Copy link

@aldo-expensify This is a Concierge message composed in the Concierge tool. (I just checked my messages in a test account and I wasn't able to find one like this).

@aldo-expensify
Copy link
Contributor

@JmillsExpensify thanks, I didn't consider that. I tested sending some copy/pasted html message from wikipedia, with links and lists, and the look fine.

image

I looked for some of the text we can see in the message from the screenshots of the GH issue, but I couldn't find it in the code.. so I supposed it is not an automatic Concierge message.

@JmillsExpensify
Copy link

Yeah, I think this message came during one of our VIP projects. Hmm, maybe we should close then. I'm also not seeing this either.

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Oct 4, 2022

@JmillsExpensify hehe i just noticed you reported this by looking at the slack conversation. Is it possible for you to get to this same conversation and check if now it is looking fine?

@JmillsExpensify
Copy link

Yep! Sure thing, I'll go ahead and assign myself.

@aldo-expensify
Copy link
Contributor

thanks!

@JmillsExpensify
Copy link

I didn't get a chance to test this so far, will come back soon.

@JmillsExpensify
Copy link

Ok jumping back in to test this now.

@JmillsExpensify
Copy link

Alright, I'm having the hardest time reproducing this. I sent myself a markdown formatted message via the company tool, I asked agents to send me HTML formatted text via the Concierge tool. Etc. etc. I'm still unable to produce on mobile web.

IMG_8646705BA2BB-1

@JmillsExpensify
Copy link

My next move is to head back to the original thread and see if this is still visible for those that were participating there.

@JmillsExpensify
Copy link

Ok I was able to reproduce this one. I just updated the OP. Screenshot of the issue on mWeb
IMG_01821CEB4445-1

@JmillsExpensify
Copy link

@aldo-expensify You want to assign yourself again?

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Oct 6, 2022

I'm trying, but I can't reproduce it yet. I don't find that repeatable response, could it have been removed?

image

I tested with other responses (the ones you can see in the screenshot), but they look fine in mWeb in NewDot.

@aldo-expensify
Copy link
Contributor

it could be that that specific response had invalid html, and one browser was doing a better job than the other in fixing it. Can you reproduce this with any other pre-saved response (if we don't find the original one anymore)?

Thanks for updating the steps, it is much clearer now

@JmillsExpensify
Copy link

JmillsExpensify commented Oct 6, 2022

Interestingly enough, I thought the same. I even deleted the old saved repeatable response and then manually created a new one. But even the new, non-copy/pasted one I created still had the same problem. So it was just super super weird that it still happened despite that.

@aldo-expensify
Copy link
Contributor

Can you send me the response in an email or file? 🙏

@fedirjh
Copy link
Contributor

fedirjh commented Oct 13, 2022

Proposal

Problem

The RenderHTML add the flex property to some element flex: 1 1 0%;

Screenshot from 2022-10-13 00-54-16

Solution

we can set an initial flex property to the config so the RenderHTML will use it instead of adding new one

const defaultViewProps = {style: [styles.alignItemsStart, styles.userSelectText]};

-      const defaultViewProps = {style: [styles.alignItemsStart, styles.userSelectText]};
+      const defaultViewProps = {style: [styles.alignItemsStart, styles.userSelectText,styles.flexInherit]};

then add the new flexInherit utility to Flex layout utility

export default {
flexReset: {
flex: undefined,
},

+    flexInherit:{
+      flex: 'inherit'
+    },

Test

Screenshot from 2022-10-13 00-58-06

@JmillsExpensify
Copy link

@parasharrajat thoughts on @fedirjh's proposal above?

@parasharrajat
Copy link
Member

This is happening on Chromium-based browsers.
Chrome
image

Firefox
image

@fedirjh
Copy link
Contributor

fedirjh commented Oct 14, 2022

@parasharrajat Safari too

Untitled

@parasharrajat
Copy link
Member

Yeah @fedirjh , Safari is chromium based browser.

I have a better proposal. This is happening due to flex:1

flex: 1,

I traced back this change and it was added by me in #5364.

As I mentioned in the proposal it was optional at that time.

So I think we can just remove flex: 1 and it will be fixed.

@aldo-expensify
Copy link
Contributor

@parasharrajat if that fixes it and doesn't break anything else, I would prefer your solution for simplicity.

@parasharrajat
Copy link
Member

parasharrajat commented Oct 14, 2022

let me test this on IOS and safari web. it's working on others (chrome, firefox, android, android chrome). And does not cause the old issue.

@parasharrajat
Copy link
Member

Looking good everywhere.

@JmillsExpensify
Copy link

Ok so @aldo-expensify are we hiring @parasharrajat for this job? @chiragsalian are you good with this plan? As a reminder, the upwork job is here when we get there: https://www.upwork.com/jobs/~01521563b8fd74ea23.

@aldo-expensify
Copy link
Contributor

I vote for @parasharrajat 's solution, but leaving it up to @chiragsalian since he is the CME

@chiragsalian
Copy link
Contributor

Yes, I like @parasharrajat solution as well since its simple, and you've worked on this before and it seems to be working fine everywhere. Feel free to create the PR @parasharrajat.

@JmillsExpensify JmillsExpensify changed the title [$500] HTML formatted messages render incorrectly [Bug] [$500] HTML formatted messages render incorrectly Oct 18, 2022
@puneetlath puneetlath added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 19, 2022
@melvin-bot melvin-bot bot added the Overdue label Oct 19, 2022
@chiragsalian chiragsalian added the Reviewing Has a PR in review label Oct 19, 2022
@melvin-bot melvin-bot bot removed the Overdue label Oct 19, 2022
@JmillsExpensify
Copy link

Invited @parasharrajat to the Upwork job. Then as a more general update, the linked PR has been merged and should reach production in the next day or so. At that point we'll kick off the regression period hold before paying.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Oct 26, 2022
@melvin-bot melvin-bot bot changed the title [Bug] [$500] HTML formatted messages render incorrectly [HOLD for payment 2022-11-02] [Bug] [$500] HTML formatted messages render incorrectly Oct 26, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 26, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.19-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-11-02. 🎊

@JmillsExpensify
Copy link

Don't see any regressions at this point, so I will circle back tomorrow for payment.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 2, 2022
@JmillsExpensify
Copy link

Waiting on @parasharrajat to accept the Upwork invite before job can be paid and issue closed.

@JmillsExpensify
Copy link

@parasharrajat sent you an offer on Upwork. Will pay out as soon as you're able to accept.

@JmillsExpensify
Copy link

Payment issued! Closing this one out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

7 participants