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

Email notifications for messages in a room thread don't seem to say who the message is from #47081

Closed
1 of 6 tasks
m-natarajan opened this issue Aug 8, 2024 · 45 comments
Closed
1 of 6 tasks
Assignees
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@m-natarajan
Copy link

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: 9.0.18-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
Expensify/Expensify Issue URL:
Issue reported by: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1723051083119829

Action Performed:

Prerequisite: create room with member

  1. Send a message from room
  2. Wait for email notification for member regarding the message

Expected Result:

Message should say from whom it was received

Actual Result:

Don't seem to say who the message is from

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

image
Snip - #testroom-for-notification-email - mnata utest@gmail com - Gmail - Google Chrome (2)

Add any screenshot/video evidence

View all open jobs on GitHub

@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 8, 2024
Copy link

melvin-bot bot commented Aug 8, 2024

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added the Overdue label Aug 12, 2024
Copy link

melvin-bot bot commented Aug 13, 2024

@anmurali Huh... This is 4 days overdue. Who can take care of this?

@anmurali
Copy link

Yea this email notification is super confusing. I think ideally it should look and feel like the product right? Sorta like an IOU or money request related notification does? I received this:
image

I think it should look like
image

@Expensify/design what was this designed to look like?

@melvin-bot melvin-bot bot removed the Overdue label Aug 14, 2024
@shawnborton
Copy link
Contributor

Hmm I would think we should be showing the sender's avatar + name + full message like we do for other emails.

Also, in this case, why is the email coming from "Expensify Chat" if that's just the workspace name?

@melvin-bot melvin-bot bot added the Overdue label Aug 16, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

@anmurali Eep! 4 days overdue now. Issues have feelings too...

Copy link

melvin-bot bot commented Aug 21, 2024

@anmurali 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

Copy link

melvin-bot bot commented Aug 22, 2024

@anmurali 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!

Copy link

melvin-bot bot commented Aug 23, 2024

@anmurali 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

Copy link

melvin-bot bot commented Aug 27, 2024

@anmurali 12 days overdue. Walking. Toward. The. Light...

@anmurali
Copy link

@shawnborton - doesn't my proposal #47081 (comment) clarify who the sender of each message is? It basically looks like the conversation in that room or group chat itself.

@melvin-bot melvin-bot bot removed the Overdue label Aug 28, 2024
@shawnborton
Copy link
Contributor

Hmm I'm not quite following. But I just checked some notifications from #social and does it look like they are behaving as expected? Each new notification comes from an actual person:
CleanShot 2024-08-28 at 07 26 00@2x

I'm not sure how I feel about that tbh. I would think that a group of messages would be sent from Expensify, not a singular person since there are messages from multiple people there. But if just one person sent a message, I can see where it makes sense to have the notification come from just one person?

@puneetlath
Copy link
Contributor

puneetlath commented Aug 28, 2024

It seems to be inconsistent for me. Sometimes it includes the avatars of the senders and sometimes it doesn't.

Here's an example where notifications for the same room sometimes included the avatar/name and sometimes didn't:
image

Looks like @Beamanator also experienced similar here: https://expensify.slack.com/archives/C049HHMV9SM/p1724809339227489

@melvin-bot melvin-bot bot added the Overdue label Aug 30, 2024
Copy link

melvin-bot bot commented Sep 2, 2024

@anmurali Huh... This is 4 days overdue. Who can take care of this?

Copy link

melvin-bot bot commented Sep 4, 2024

@anmurali Still overdue 6 days?! Let's take care of this!

Copy link

melvin-bot bot commented Sep 6, 2024

@anmurali 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

@anmurali
Copy link

anmurali commented Sep 9, 2024

Discussing in slack here

@melvin-bot melvin-bot bot removed the Overdue label Sep 9, 2024
@anmurali anmurali added the Internal Requires API changes or must be handled by Expensify staff label Sep 10, 2024
@puneetlath puneetlath moved this to CRITICAL in [#whatsnext] #quality Sep 10, 2024
@puneetlath puneetlath added the AutoAssignerNewDotQuality Used to assign quality issues to engineers label Sep 10, 2024
@shawnborton
Copy link
Contributor

shawnborton commented Sep 12, 2024

Yeah, I think we originally made 1:1 messages arrive without an avatar and a display name in the body because the idea was that we would send the email as if it came from the chat message sender, and I think David felt quite strongly about making the email experience feel as similar to chat as possible. But some considerations:

  • we can't actually show the avatar of the sender
  • I think there is a difference that we experience internally when we get these email notifications. For instance, we always have the checkmark and the app icon as our avatar: CleanShot 2024-09-12 at 06 47 45@2x

Can someone confirm what it looks like when a non-Expensify account sends a message to someone that generates an email notification? Does that email come from notifications@expensify.com, with an Expensify avatar? If that's the case, I think it makes sense to just completely standardize on the body of the email always containing the user's avatar + display name + message.

@shawnborton
Copy link
Contributor

Also cc @Expensify/design for any additional thoughts here - I think we've chatted through this one in the past.

@shawnborton
Copy link
Contributor

Okay actually my test just came through - this is what it looks like when my personal account sent a message and generated a notification:
CleanShot 2024-09-12 at 06 50 10@2x

I think the fact that we are always showing the Expensify logo here makes me want to put the user's avatar in the message body, and thus standardize on everything. But notice that we still send the message from my display name.

I think my vote would be to standardize the message template by always showing an avatar and sender name in the body but we should socialize this idea in Slack first and make sure everyone is on board with it first.

@dubielzyk-expensify
Copy link
Contributor

Can someone confirm what it looks like when a non-Expensify account sends a message to someone that generates an email notification? Does that email come from notifications@expensify.com, with an Expensify avatar? If that's the case, I think it makes sense to just completely standardize on the body of the email always containing the user's avatar + display name + message.

I got this when testing with a friend from Concierge, but this was sent from my friend. Not sure if that's a bug or feature.
CleanShot 2024-09-12 at 15 23 47@2x

My thoughts on this is aligned with you and Danny I believe. I largely find our whole email experience extremely confusing. Some have avatars and some don't, some send from Expensify, some from a user's name.

I think we should be sending the emails all from Expensify with the avatar and sender as Expensify, then embed the user avatar and message within the body of the message at the least to make all the messages consistent.

@dannymcclain
Copy link
Contributor

I feel like I've talked pretty extensively about this with you two 😅 and I think you both know how dissatisfied I am with our current strategy. TLDR: I'm aligned with you all.

What ever happened to this idea? 👇

Email title:
(E Avatar) Shane Burton via Expensify <notifications@expensify.com>

Email body:
(Shane's avatar) Shane Burton
Hello! I am not Shawn!

Anyways, I agree with what Jon said here:

I largely find our whole email experience extremely confusing. Some have avatars and some don't, some send from Expensify, some from a user's name.

I think we should be sending the emails all from Expensify with the avatar and sender as Expensify, then embed the user avatar and message within the body of the message at the least to make all the messages consistent.

@shawnborton
Copy link
Contributor

I like that suggestion for the sender Danny, I forgot we had discussed it but yes it does seem quite ideal!

@jasperhuangg
Copy link
Contributor

jasperhuangg commented Sep 13, 2024

I think we should be sending the emails all from Expensify with the avatar and sender as Expensify, then embed the user avatar and message within the body of the message at the least to make all the messages consistent.

Strongly agree with what Jon mentioned above and on board with the example that Danny provided above.

we should socialize this idea in Slack first and make sure everyone is on board with it first.

@shawnborton I drafted up a P/S statement and sent it to you on Slack. It's also included below for anyone who wants to give feedback

@jasperhuangg
Copy link
Contributor

jasperhuangg commented Sep 13, 2024

Problem:
There is a lot of inconsistency right now with how we format report comment notification emails:

  • Emails summarizing multiple report comments are sent from “Expensify Chat”/“Expensify Concierge” and include the report commenter’s name/avatar along with the report comment’s text in the email body

    • Screenshot 2024-09-13 at 10 22 33 AM
  • Emails summarizing single report comments are sent from the report commenter with the body of the email containing the report comment text

    • Screenshot 2024-09-13 at 10 22 51 AM

This leads to a pretty confusing experience that also implies a lack of polish for a couple of reasons:

  • It’s strange how some notifications are sent from Expensify Chat, some are sent from Expensify Concierge, and others appear as sent from the report commenter.
  • It’s strange how in the second scenario we don’t include the report commenter’s avatar anywhere in the email.

Solution:
Standardize all of our report comment notifications to be the first scenario, i.e.

  • The email shows as sent from Expensify Chat/Concierge.
  • The report commenter’s name and avatar are included with the report comment text in the body of the message.

Some additional notes for completeness:

  • Emails should be sent from “Expensify Chat” by default and should only appear as sent from “Expensify Concierge” if the summary is for a Concierge report. It seems we currently use “Expensify Concierge” for report comments sent into any DM which is very confusing via this.
  • The email avatar should always be the Expensify logo, and we’ll include the report commenter’s avatar in the email body.

@shawnborton
Copy link
Contributor

shawnborton commented Sep 13, 2024

Sent this 1:1 but this looks good, but I think my main feedback is:

  • When the platform sends you multiple messages that you missed across multiple people in a room or report, the email comes from Expensify
  • When the platform sends you a single message or multiple messages that you missed from the same person, the email comes from Member Name via Expensify

@jasperhuangg
Copy link
Contributor

jasperhuangg commented Sep 13, 2024

Thanks! I went ahead and tested a bunch of scenarios locally and posted my findings in this Google Doc.

Also updated the P/S statement based on my findings and sent you the new one on Slack.

@dannymcclain
Copy link
Contributor

I think it's also worth noting that AFAIK we only display the proper user avatar in emails for people with an @expensify.com email address. Otherwise we always display the Expensify iconmark. Someone can fact check me on that, but I'm pretty sure that's come up a few times. So in your example, where we see Shae's picture as the sender, I believe we're only seeing that because Shae has an @expensify.com email address. If it was from danny@donuts.com, you'd just see the Expensify logo.

@jasperhuangg
Copy link
Contributor

@dannymcclain Yep I confirmed that through my testing, it seems like that's yet another reason why including the avatar/display name in the body of the email would help out a lot with reducing confusion.

@jasperhuangg
Copy link
Contributor

I have a P/S statement drafted up with help from @shawnborton that I'm going to send out in the #product room come Monday since it's the weekend right now and no one will see it. Gonna post it here if anyone has any additional feedback before we send it out!

Depending on how that discussion goes it might be worthwhile to do a pre-design to really iron out how we want notifications to look for all the different types of reports and notification scenarios, but we can decide on that later.

@jasperhuangg
Copy link
Contributor

Coming from this CRITICAL issue in #newdot-quality

Problem:

There is a lot of inconsistency/jank with how we format report comment notification emails:

  1. Emails summarizing comments from a single user
Screenshot 2024-09-14 at 9 33 24 AM
  • include the report comment text and no information about the commenter in the email body.
  • are sent from “” if the user is an internal employee i.e. from: Jasper
  • are sent from “Expensify Chat” if the report is a policy room.
  • are sent from “Expensify Concierge” otherwise
  1. Emails summarizing comments from multiple users
Screenshot 2024-09-14 at 9 34 33 AM
  • include report comment text and the sender display names + avatars in the email body
  • are sent from “Expensify Chat” if the report is a policy room
  • are sent from “Expensify Concierge” otherwise

More screenshots highlighting different scenarios found in this Google doc

This leads to a pretty confusing experience that also implies a lack of polish for a couple of reasons:

  • When we send emails for comments from a single user, we don’t include any information about the commenter (name/avatar) in the body of the email. This is SUPER confusing if the commenter is a non-employee because the message would show as sent from “Expensify Chat/Concierge” and there’s no way to tell who the commenter was.
  • Emails for comments sent by multiple users include nicely styled HTML that mirrors the experience in the NewDot App, while emails for comments sent by a single user only have raw text and are inconsistent with the rest of our design.
  • Some emails are sent from Expensify Chat, some are sent from Expensify Concierge, and others appear as sent from the report commenter for internal employees, seemingly without any rhyme or reason. I had to look at the code and do a bunch of manual testing to figure out what was going on.

Solution:

Standardize all of our report comment notifications to be an improved version of the second scenario from above, i.e.

  • The report commenter’s name and avatar are included with the report comment text in the body of the message.
  • The email shows as sent from “Expensify” if there are multiple commenters, and shows as from “(SenderName) via Expensify” if there is only one commenter.

This means that all the information needed to understand the report comment will be included in the email body, and that all emails will be consistently styled to match the NewDot experience.

Some additional points for completeness/discussion:

  • Messages in the Concierge DM will appear as from “Concierge via Expensify”.
  • The email avatar should always be the Expensify logo, and we’ll include the report commenter’s avatar in the email body.
  • Note that these changes will only involve report comments sent in chat reports. We’re not including other report types (IOU/Task/etc.) in the conversation, their behavior should be left untouched.

@jasperhuangg
Copy link
Contributor

P/S statement discussion happening here

@jasperhuangg
Copy link
Contributor

More discussion happening on Slack. It looks like once that wraps up the next steps would be a pre-design to really iron out the details of the implementation.

@jasperhuangg
Copy link
Contributor

Updated P/S statement outlined here, we're going a simpler route that will solve this problem without changing too much of the existing flow.

@melvin-bot melvin-bot bot added the Overdue label Sep 23, 2024
@jasperhuangg
Copy link
Contributor

Plate full at the moment after being auto-assigned a couple of issues, should be able to get to this some time this week.

@melvin-bot melvin-bot bot removed the Overdue label Sep 23, 2024
@jasperhuangg
Copy link
Contributor

Posted a new point of discussion here

@jasperhuangg
Copy link
Contributor

Making good progress with a draft PR: https://github.com/Expensify/Web-Expensify/pull/43681

@jasperhuangg
Copy link
Contributor

Draft PR is on staging

Copy link

melvin-bot bot commented Oct 7, 2024

@anmurali, @jasperhuangg Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Oct 9, 2024

@anmurali, @jasperhuangg Eep! 4 days overdue now. Issues have feelings too...

@jasperhuangg
Copy link
Contributor

PR was deployed to production last week. I think we can close this out!

@github-project-automation github-project-automation bot moved this from CRITICAL to Done in [#whatsnext] #quality Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
Development

No branches or pull requests

7 participants