Flush the email with unread message summary and send a separate email notification for money requests

anmurali commented 9 months ago

Context here Summarizing what we want with email notifications for money requests:

We want to continue to notify the user immediately via email when they receive a money request
We want to use a subject line for this notification, which is different from the report title so as to make it cleaner, more legitimate and more appealing to the receiver
We will update NotifyOfflineUsersAboutActivity to recognize when someone sends a money request -- and in that case:
1. Set a custom subject
2. Insert HTML formatted to mimic what it looks like in chat (being reworked in https://github.com/Expensify/Expensify/issues/364323)
3. Make the CTAs automatically bring the user logged in to the action, which is handled here
4. Send that one comment by itself as a nicely stylized, custom formatted, immediately delivered email that you can still reply to and otherwise functions just like any other comment notification
We want this subj line to be satisfying, i.e. it should ideally say who is asking you, and how much they are asking for. So in this case, the custom subject line will read <Sender> requests <currency><amount>
Sender - Use first and last name if available or fall back to sender's email/ SMS login info

NOTE: Here is the last solution we agreed upon.

blimpich commented 7 months ago

@blimpich, are you already working on a solution to this issue?

Yes and no. https://github.com/Expensify/Expensify/issues/365156 has many similarities. Lets break it down into use cases:

user A requests money from user B
user A sends money to user B
user B completes a payment request from User A
user A transfers account balance from wallet to their bank account

From my understanding this issue covers use case number 1. https://github.com/Expensify/Expensify/issues/365156 covers 2, 3, and 4. They are handled differently because number 1 doesn't require a receipt because no money has been exchanged yet. 2, 3, and 4 are all cases where money moved, so we have to follow regulatory requirements and give a lot more information in the email notification.

So @marcaaron and I will probably end up making very similar looking PRs but they will probably need to be separate PRs because of the different requirements.

That being said we should definitely be consistent with the behavior of our summary emails. So to answer this question:

so we don’t care if the “unread summary” comes out of order?

I would say yes, we do care. We don't want redundant emails, and the email inbox should be chronological. The proposed solution that a few of us in #vip-split decided on was to send the current batch of unread messages before sending the custom subject email.

trjExpensify commented 7 months ago

They are handled differently because number 1 doesn't require a receipt because no money has been exchanged yet. 2, 3, and 4 are all cases where money moved, so we have to follow regulatory requirements and give a lot more information in the email notification.

Okay, got it. That's where I thought the "custom footer" part comes into it - but the logic for when a custom subject email is sent for a request would be shared. I.e this part:

The proposed solution that a few of us in #vip-split decided on was to send the current batch of unread messages before sending the custom subject email.

marcaaron commented 7 months ago

Quick heads up that raising the PR for what I described in this comment could to take me some time as I'm shifting priorities to get the Group Chats project shipped ASAP. But, it sounds like that's not what we want anymore. @blimpich can you confirm?

What I've got done for this issue so far:

Update the subject line (PR in review)
After @rlinoz's changes have been merged we've also got the preview showing now as well 🆒

Result:

2024-02-26_16-16-38

So - it's getting there...

send the current batch of unread messages before sending the custom subject email

This is kind of the solution that I started with - if that's how we want to handle things then I don't think we should create a new job at all. It sounds like we just need to add some behavior to "flush" the comments then immediately send another notification for the money request.

So @marcaaron and I will probably end up making very similar looking PRs but they will probably need to be separate PRs because of the different requirements.

If we can do the main logic for this in one PR that would be good. It sounds like the "flush comments and send money request notification" is the behavior that we want. So, let's set that up for everything. Then come back here after and add the finishing design touches on this one and it can be closed?

blimpich commented 7 months ago

That sounds good. I have some of the logic for this figured out already in this draft PR. You can see I used some of the bits of code from your PR there, so yeah, there is definitely overlap. I made a function that flushes out the unread notifications called sendQueuedNotificationsForOfflineParticipantsImmediately which should work for both of our cases.

The only thing is that I'm also not going to be the fastest on this since I'm working my way through the design doc process. It is my main priority but I am partly blocked until I can get the go-ahead on my detailed design. Once that's done my PR should go through pretty quickly and then resolving this issue should be a lot simpler.

So @marcaaron you'll prioritize Group Chats while I work on p2p receipts, which should save you some work when you get freed up next. Sound like a plan?

marcaaron commented 7 months ago

@blimpich Sounds pefect. And no worries I don't think this one is super urgent.

Lemme know if you need an extra set of eyes on the design doc front 🤙

marcaaron commented 6 months ago

Holding this at a lower priority for now.

marcaaron commented 6 months ago

Still holding - I am focused on Group Chats. I will try to hand this off.

rlinoz commented 6 months ago

As I might pick this up, let me understand a few things.

@trjExpensify based on your comment here: https://github.com/Expensify/App/issues/34167#issuecomment-1948268556

Is this still the behavior we want?

If it is, when are we considering that user A requested money? When the report is submitted, when user A creates a report with an expense, or every new expense created in a report should fire an email?

trjExpensify commented 6 months ago

I don't think it is, @rlinoz. The desired behaviour is linked here.

I would say yes, we do care. We don't want redundant emails, and the email inbox should be chronological. The proposed solution that a few of us in #vip-split decided on was to send the current batch of unread messages before sending the custom subject email.

marcaaron commented 5 months ago

@rlinoz are you still interested? I would definitely welcome the help as I am focused on launching Group Chats atm

rlinoz commented 5 months ago

The mentions v2 design doc got reviewed in the meantime and I started working on those issues, sorry

marcaaron commented 5 months ago

I am blocking this issue here so gonna un-assign myself for now and apply the Hot Pick label. I have a large number of Group Chats issues to follow up on and won't have time to look at the changes above for a while. Sounds like we have a general plan to move forward on so happy to answer any related questions if they come up.

trjExpensify commented 4 months ago

What remains here? Is it just implementing the part to use @blimpich's sendQueuedNotificationsForOfflineParticipantsImmediately to send any unread comments that might be in and around the request so they’re in chronological order?

marcaaron commented 4 months ago

@blimpich what's the state of that PR you linked there? I see it was closed - did we/are we opening a new one?

marcaaron commented 4 months ago

I think this is where we're at:

Proposal: Send the current batch of unread messages before sending a separate custom email for new money requests

Background:

When a user requests money from another user we currently:

Queue an immediate notification (i.e. NotifyOfflineUsersAboutActivity will run as soon as possible)
The subject line will get updated to read <Sender> requests <amount>
The report link takes you to the parentReportID i.e. the “parent” workspace or DM chat of the request
The “Pay” button takes you directly to the expense report itself

Problem:

If there are already unread messages for the user we are notifying those will also display in this same email. This unnecessarily hides comments inside an email that appears to notify you of about one thing.

Solution:

Flush any existing notifications for other report actions first (without the request action)
Queue a separate notification for the request action with the custom subject and correct report link

Does that sound correct to everyone?

trjExpensify commented 4 months ago

That sounds right to me! @anmurali, what about you?

blimpich commented 4 months ago

@marcaaron for the solution one clarifying question: does the flushing of existing notifications and queuing of a separate notification for the request action happen immediately upon the user creating the request? It doesn't wait 10 minutes right?

marcaaron commented 4 months ago

Correct. It should be immediate.

melvin-bot[bot] commented 4 months ago

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