[$1000] Trying to send message within angle brackets in the invite new members page doesn’t display in email

[kavimuru]

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. Open Android with User A account
2. Go to any workspace > Invite new member
3. Within the message box, add a message within angle brackets
4. Invite UserB
5. Check the mail of User B and notice that every message gets displayed except for the messages that are within angle brackets.

Expected Result:

Messages within angle brackets should be displayed on the mail

Actual Result:

Messages within angle brackets are not displayed on the mail while adding it from invite new members page

Workaround:

unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.79-0
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:

mail.mp4

Recording.1637.mp4

Expensify/Expensify Issue URL:
Issue reported by: @avi-shek-jha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1678111359321989

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c312f8da095f5003
• Upwork Job ID: 1638673051032281088
• Last Price Increase: 2023-03-22

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• 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
• 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.
• 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.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[mateocole]

hmmm yes, this does indeed look like we should not be hiding the messaging due to < text >, going to have an engineer take a look to confirm though.

[MelvinBot]

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

[MelvinBot]

@johnmlee101, @mateocole Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[mateocole]

@johnmlee101 just wanted to get a double check on this, and if we can perhaps mark this as external and get a job going on upwork for it

[johnmlee101]

Hmm, this is part of our filtering on the web end IIRC, which means this is an internal issue

[johnmlee101]

Can you confirm with the Bug0 Triage Checklist?

[johnmlee101]

Actually I think #15315 might be the solution to this right now that was just deployed. @kavimuru can you re-test again?

[johnmlee101]

Also this might be a dupe of #14890 as well

[mateocole]

good catch @johnmlee101 this does appear to be a dupe, i think this can be closed out in favor of the other issue

[johnmlee101]

Actually I think the solution made in that issue doesn't cover this one, so let's just first confirm you can't add this in NewDot first then we'll see what to do here

[MelvinBot]

@johnmlee101, @mateocole Eep! 4 days overdue now. Issues have feelings too...

[johnmlee101]

Can this be tested?

[MelvinBot]

@johnmlee101 @mateocole this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[sobitneupane]

Thanks for the proposal @jjcoffee

Let's use parser.replace in

App/src/pages/workspace/WorkspaceInvitePage.js

Line 275 in b0b31d1

Policy.addMembersToWorkspace(filteredLogins, this.state.welcomeNote, this.props.route.params.policyID);

instead.

Update: In fact, it might be better idea to use getParsedComment rather than parser.replace or _.escape.

Proposal from @jjcoffee looks good to me.

🎀👀🎀 C+ reviewed
cc: @johnmlee101

[jjcoffee]

@sobitneupane Thanks for the review!

I considered getParsedComment initially, but I am unsure if we need/expect markdown support that parser.replace provides in a simple welcome note (I assumed it's expected to just be text)? If not it would be more performant to just use _.escape.

Also, would you mind clarifying why you think it's better to apply the escaping in WorkspaceInvitePage.js instead? My reasoning for popping it further up the chain in Policy was to future-proof for further calls to Policy.addMembersToWorkspace (otherwise people would have to apply the escaping each time they add a new call).

[johnmlee101]

Do we expect the email to come with the full markdown in the message? Do we currently render markdown in the member invite message? If we don't render it today, I say we should avoid getParsedComment for now, and just work with _.escape

If we do render it today, I think its fine to use getParsedComment

[jjcoffee]

@johnmlee101 Ah yes, good point! It looks like we're not currently rendering markdown there, so I'll go with just using _.escape as you say.

[johnmlee101]

Awesome, sounds good!

[MelvinBot]

📣 @jjcoffee You have been assigned to this job by @johnmlee101!
Please apply to this job in Upwork 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 📖

[jjcoffee]

PR coming today!

[jjcoffee]

Hey @johnmlee101 & @sobitneupane, the PR is ready.

[mateocole]

Going to be OOO, reassigning bug

[MelvinBot]

Current assignee @anmurali is eligible for the Bug assigner, not assigning anyone new.

[jjcoffee]

Looks like the automation failed here as the PR is in production, is there a way to kick Melvin into gear? Or does someone manually update the labels and paste in the checklist?

[johnmlee101]

Manually creating

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

#16552
If no regressions arise, payment will be issued on 2023-04-08. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[johnmlee101]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@johnmlee101 / @sobitneupane ] The PR that introduced the bug has been identified. Link to the PR:
• [@johnmlee101 / @sobitneupane ] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@johnmlee101 / @sobitneupane ] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@anmurali ] Determine if we should create a regression test for this bug.
• [@johnmlee101 / @sobitneupane ] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@anmurali ] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[jjcoffee]

@anmurali Gentle bump on this one as it's overdue for payment (I think!).

[jjcoffee]

Hey @anmurali, are you able to make the payment on this? It's been due since 2023-04-08. I believe the 50% timeliness bonus is also due (assigned Fri 24/3 19.24, merged Weds 29/3 11.54).

[trjExpensify]

@anmurali can you get to that today, please?

In the meantime, @johnmlee101 @sobitneupane can you fill out your portion of the checklist to help close this out?

[sobitneupane]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@johnmlee101 / @sobitneupane ] The PR that introduced the bug has been identified. Link to the PR:

I think this issue was present with the addition of invite users page to the workspace by this PR. It is a very old PR.

[sobitneupane]

Regression Test Proposal

• Log in with User A account
• Go to any workspace > Invite new member
• Within the message box, add a message that includes HTML-like entities (Example: <message> &amp; &)
• Invite User B
• Check the mail of User B and ensure the message comes through exactly as sent.

Do we agree 👍 or 👎

[anmurali]

https://github.com/Expensify/Expensify/issues/276664 for regression testing
Offers sent

[jjcoffee]

@anmurali Many thanks, accepted!

[anmurali]

Paid