(BEDS-1162) Fix: don't increase mailsSent count if limit reached #1287
Conversation
| func QueueEmailNotifications(epoch uint64, notificationsByUserID types.NotificationsPerUserId, tx *sqlx.Tx) error { | ||
| // for emails multiple notifications will be rendered to one email per user for each run | ||
| // TODO filter out ratelimited users, don't need to queue or even render emails | ||
| emails, err := RenderEmailsForUserEvents(epoch, notificationsByUserID) | ||
| if err != nil { |
There was a problem hiding this comment.
This would be the clean thing to do. Instead of storing rendered emails to db first and let the sender throw them away again later, doing it here with batch querying of user premium perks + email counts from redis would be a nice early out.
backend/pkg/commons/mail/mail.go
Outdated
| @@ -78,14 +78,14 @@ func createTextMessage(msg types.Email) string { | |||
| // It will return a ratelimit-error if the configured ratelimit is exceeded. | |||
| func SendMailRateLimited(content types.TransitEmailContent, maxEmailsPerDay int64, bucket string) error { | |||
| sendThresholdReachedMail := false | |||
| count, err := db.CountSentMessage(bucket, content.UserId) | |||
| count, err := db.GetSentMessagesCount(context.Background(), bucket, content.UserId) | |||
There was a problem hiding this comment.
I don't think this is save, as multiple senders run at the same time, so some kind of locking needs to be implemented here.
There was a problem hiding this comment.
You're right. I went back to the original approach of increasing the count in redis before sending the mail, but added an upper limit using optimistic locking (see cas). I assume this should solve the issue for us.
If actual locking should be required, I would suggest integrating redis' distributed-locks (instead of some psql locking) as it's close to the data in question here and prob faster; however that'd add another dependency. ptal
remoterami
force-pushed
the
BEDS-1162/respect-email-threshhold
branch
from
5bed928 to
2ae82ab
Compare
I ran some manual tests, isolating the affected methods and pointing to a local redis instance. But I could also try to setup unit testing for the notification sender if preferred. |
remoterami
force-pushed
the
BEDS-1162/respect-email-threshhold
branch
from
2ae82ab to
927416b
Compare
The mail sender does check the
maxEmailsPerDaylimit, but it increases the count anyways which results in wrong numbers being shown.Fixed & renamed the count setter + added a getter method for clarity.