[On-Hold for #379005][$500] [LOW] Save the world - The email refers to people by email address + grammar issue

izarutskaya commented 6 months 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: 1.3.97.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 Expensify/Expensify Issue URL: Issue reported by: Applause-Internal Team Slack conversation: @

Action Performed:

Navigate to https://staging.new.expensify.com/
Click on the FAB button
Click on the "Save the word" button
Click on the "I know a teacher" button
Fill in all fields
Click on the "Let's do this!" button
Open your email app

Expected Result:

The mail should start with "Hey". If the name of the sender and teacher is known (filled out before sending) the email should refer them by name.

Actual Result:

The email for the teacher starts with ": Hey". It refers to both the sender and the teacher by email address.

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/115492554/29d24079-7acd-477f-b9df-76c205eca15e

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~019c866a41051c1129
Upwork Job ID: 1722577721011531776
Last Price Increase: 2023-11-09

Issue Owner

Current Issue Owner: @roryabraham

melvin-bot[bot] commented 6 months ago

Job added to Upwork: https://www.upwork.com/jobs/~019c866a41051c1129

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

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

erquhart commented 6 months ago

Not seeing email templates in Expensify's OSS repos, guessing this is an internal issue.

CortneyOfstad commented 6 months ago

Yeah, thinking this would be internal — going to get engineering eyes on this 👍

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

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

erquhart commented 6 months ago

This is internal but Help Wanted label is still applied.

melvin-bot[bot] commented 6 months ago

@CortneyOfstad, @sobitneupane, @roryabraham Whoops! This issue is 2 days overdue. Let's get this updated quick!

CortneyOfstad commented 6 months ago

@roryabraham any ideas with this one?

CortneyOfstad commented 6 months ago

Bump @roryabraham — TIA!

roryabraham commented 5 months ago

Ok, so this happens because the HTML in question looks like this:

const string whisperMessageForTeacher = "Hey <mention-user>@" + partnerUserID + "</mention-user>! Your friend <mention-user>@" + inviterEmail + "</mention-user> referred you to Expensify.org's latest campaign which helps teachers and their students. Our Teachers Unite campaign splits teachers' out-of-pocket expenses for essential school supplies. <a href=\"" + AuthCommand::getURLToNewDot() + "teachersunite/i-am-a-teacher\" rel=\"noreferrer noopener\">Click on Save The World</a> in the app's main menu to sign up now and start splitting your expenses with Expensify.org!";

This displays in-product as a mention, which by our current design does not use displayName at all, and instead just uses the full email. There was a ton of discussion about this but I think we went with full emails for simplicity in the V1. cc @puneetlath

So if we wanted to change this it would need to be a broader discussion around mentions in general

roryabraham commented 5 months ago

My main questions here are:

Do we care about this issue?
Why did we end up always using full emails for mentions? Is that a long-term plan? Whatever happened to this nice mockup?
If we don't have any plans to update how mentions are displayed in-product, should we hack something up to inject displayNames for mentions in emails?

melvin-bot[bot] commented 5 months ago

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

roryabraham commented 5 months ago

I suppose even if we were going to change how mentions are displayed in-product, it would probably be a front-end thing that happens on the fly where we lookup the personal details for that login and show their displayName, rather than using the displayName that can change at any time in the HTML.

So I feel like our options here are:

:donothing:
Update NotifyOfflineUsersAboutActivity to search for any instances of <mention-user> tags and replace them with the displayName for that user, if one exists. Maybe need to grab this from the form as I'm not sure if that form is going to actually set the invitee's personalDetails or not

puneetlath commented 5 months ago

Just to give some context:

We currently support two types of mentions as both:

<mention-user accountID=1234></mention-user>
<mention-user>@fullemailadress@gmail.com</mention-user>

The long-term plan is to transition all mentions to be the first. Then the client will decide how to render the mention, based on the data available in personalDetails. It could be any of:

@rory@expensify.com
@rory
@Rory
@Rory Abraham
etc

So once we fully transition to the <mention-user accountID=1234></mention-user> approach, we will need to update our email notifications to handle that. Maybe it's best to go ahead and do that now? And then update these whisper messages to use the first style of mentions.

melvin-bot[bot] commented 5 months ago

@CortneyOfstad @sobitneupane @roryabraham 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!

CortneyOfstad commented 5 months ago

So once we fully transition to the approach, we will need to update our email notifications to handle that. Maybe it's best to go ahead and do that now? And then update these whisper messages to use the first style of mentions.

@roryabraham @sobitneupane does that work for you guys?

CortneyOfstad commented 5 months ago

Not sure why the overdue label is sticking around 🙃

melvin-bot[bot] commented 5 months ago

@CortneyOfstad, @sobitneupane, @roryabraham Huh... This is 4 days overdue. Who can take care of this?

roryabraham commented 5 months ago

Thanks for the context @puneetlath. I'll get started on a PR to make our emails expand mentions using the information available about the user.

melvin-bot[bot] commented 5 months ago

@CortneyOfstad @sobitneupane @roryabraham this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

roryabraham commented 5 months ago

Btw, I think overdue is sticking around since I made myself the issue owner in K2.

Moving this back to weekly as it's not particularly urgent

melvin-bot[bot] commented 5 months ago

@CortneyOfstad @sobitneupane @roryabraham this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 5 months ago

@CortneyOfstad @sobitneupane @roryabraham this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 5 months ago

@CortneyOfstad, @sobitneupane, @roryabraham Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 5 months ago

@CortneyOfstad, @sobitneupane, @roryabraham Eep! 4 days overdue now. Issues have feelings too...

CortneyOfstad commented 5 months ago

@roryabraham any updates? Thanks!

roryabraham commented 5 months ago

Mini update – I think Notification_Standard::macroReplace might be the right place to implement this change.

roryabraham commented 5 months ago

Second, here's a regex that appears to capture mentions in the new format, along with capture groups for the accountID and the contents of the tag. I will plan on ignoring the deprecated format:

/<mention-user accountID=(\d+)>(@.*?)<\/mention-user>/gi

melvin-bot[bot] commented 5 months ago

@CortneyOfstad @sobitneupane @roryabraham this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!

CortneyOfstad commented 5 months ago

Thanks @roryabraham!

roryabraham commented 5 months ago

Setting expectations – I am a juror in a criminal trial this week and expect to have very limited working hours, except on Friday. I'll do my best to push this forward in the interim but there may be a number of "No Update" comments I'll have to leave to keep it from going overdue

CortneyOfstad commented 5 months ago

No worries and thanks for the context @roryabraham! Going to adjust the frequency on this as well to help avoid those pesky overdue labels 👍

roryabraham commented 4 months ago

Back from jury duty now, but obviously we're headed into Christmas. I expect I should be able to provide another substantive update next week

roryabraham commented 4 months ago

Another small update with a WIP PR: https://github.com/Expensify/Web-Expensify/pull/40342. Still need to test, but in theory that should work for users that already have personalDetails saved. It's also likely that I'll need to update that invite form to set personal details on the invited account with the values from the form too.

CortneyOfstad commented 4 months ago

Any update @roryabraham? Thanks!

roryabraham commented 4 months ago

Sorry, no update over the last few weeks, have been swamped trying to get some more urgent things over the finish line.

roryabraham commented 3 months ago

Brought this up as a potential candidate to get added into wave6: https://expensify.slack.com/archives/C02MW39LT9N/p1706561779540699?thread_ts=1706548232.782599&cid=C02MW39LT9N

CortneyOfstad commented 3 months ago

@roryabraham any updates? Thanks!

CortneyOfstad commented 3 months ago

@roryabraham bump ^^^ thanks!

CortneyOfstad commented 3 months ago

It looks like Rorys comment was finally updated to approve this for Wave6, so added that project. Based on this, it looks like the urgency is low, so selected that frequency. If you feel that needs to be adjusted, please let me know @roryabraham 👍

roryabraham commented 2 months ago

Sorry, I am currently spread too thin, so I'm going to send this issue back to the pool, hopefully having left some useful breadcrumbs for someone to pick up where I left off.

CortneyOfstad commented 2 months ago

Adjusted the tags to also include hot pick for engineers to select

CortneyOfstad commented 2 months ago

Was OoO last week, but this is not overdue — still waiting on an engineer. Since this is listed as "polish," going to keep at Weekly 👍

CortneyOfstad commented 2 months ago

Huzzah — this is going to be fixed via https://github.com/Expensify/Expensify/issues/379005. This is in the works, so going to adjust the title and put this on-hold until that is resolved.

CortneyOfstad commented 1 month ago

Doc is still being reviewed so no update 👍

CortneyOfstad commented 1 month ago

Doc still being reviewed!

CortneyOfstad commented 3 weeks ago

No update for the on-hold issue

Expensify / App