Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.32k stars 2.75k forks source link

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

Open m-natarajan opened 1 month ago

m-natarajan commented 1 month 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: 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?

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

melvin-bot[bot] commented 1 month ago

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[bot] commented 4 weeks ago

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

anmurali commented 4 weeks ago

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?

shawnborton commented 4 weeks ago

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[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

anmurali commented 2 weeks ago

@shawnborton - doesn't my proposal https://github.com/Expensify/App/issues/47081#issuecomment-2287722637 clarify who the sender of each message is? It basically looks like the conversation in that room or group chat itself.

shawnborton commented 2 weeks ago

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 commented 2 weeks ago

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[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 5 days ago

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

anmurali commented 2 days ago

Discussing in slack here

melvin-bot[bot] commented 1 day ago

Triggered auto assignment to @jasperhuangg (AutoAssignerNewDotQuality)

jasperhuangg commented 1 day ago

Can look into this, but need to clear off some of the existing PRs that I already have open.

jasperhuangg commented 1 day ago

Based on this it looks like we added specific logic to omit the avatars and display names from the notifications if they only contain a single message from a single person. This is consistent with the examples of this issue that we've been seeing in the linked Slack threads.

It looks like this was added here and was probably broken by some other logic that was added afterwards. I'll try to clean up the logic since it's pretty hard to follow atm.

puneetlath commented 15 hours ago

Oh that's interesting. I feel like it would be better to just have it look consistent every time. Especially now that the messages come from notifications@expensify.com instead of the user themselves. But I'll defer to @shawnborton on that.

jasperhuangg commented 7 hours ago

Especially now that the messages come from notifications@expensify.com instead of the user themselves

Yeah totally agree with this. It seems the reason we added this logic previously was because the sender's name was already in the email's "from" field. Definitely think we should defer to how we're displaying the multi-message notifications for everything now.

shawnborton commented 7 hours ago

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:

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 commented 7 hours ago

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

shawnborton commented 7 hours ago

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 commented 6 hours ago

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.