codidact / qpixel

Q&A-based community knowledge-sharing software
https://codidact.com
GNU Affero General Public License v3.0
385 stars 69 forks source link

Weekly subscription sends daily email #425

Open cellio opened 3 years ago

cellio commented 3 years ago

https://software.codidact.com/posts/280573

This post reports two problems with subscriptions: that it's supposed to be weekly and is instead sending mail every day, and that it's not sending only new questions. I've seen the latter problem and dropped my own subscriptions for that reason (too noisy), and we should look into that, but most importantly, let's not send email more often than we said we would. We don't want people to start ignoring email from us (I have hopes of email notifications someday and those will be important).

sau226 commented 3 years ago

Not sure how to fix the SQL, but it is an SQL error.

https://github.com/codidact/qpixel/blob/develop/scripts/send_subscription_emails.rb means that it scans for subs that have not been sent in 1 day and then triggers the job.

If anyone knows how to pull the sub info from DB and pass the DB sub info into multiple scoping queries, that would fix the issue.

ArtOfCode- commented 3 years ago

That's not the cause, @sau226 - that clause already takes into account the requested frequency and only pulls subscriptions that have never been sent or haven't been sent in over the requested frequency.

Taeir commented 2 years ago

I double checked. ArtOfCode is right, the time between subscription messages is correct. I suspect the poster may have had an additional subscription with a higher frequency.

There is another issue with it though, related to the fact that it uses exact time, which may cause it to skip sending a message for a day. For example, if the last subscription email (of a daily subscription) was sent at 02:05:10 (10 seconds past 5 past 2 (in the night)), and the next day it is checked at 02:05:09, then it will not send the subscription email because less than a day has passed (23:59:59). Instead, it should compare in a way that keeps working in the face of very minor timing differences. (Emails are sent once a day at 02:05 by default).

For the second issue (sending only new questions), the problem is that we don't do enough tracking:

In my opinion, we should consider either adding the bookkeeping of which posts were already sent to the user, and/or also add a different subscription category called 'new' which will send about all posts posted in the pervious day (i.e. based on creation date).

cellio commented 2 years ago

Thanks for the investigation.

I marked this high priority because of the first part (that it looked like we were sending way too much mail, which is bad). From the comments here it sounds like this was a user-specific error, so I'm going to lower the priority to medium.

Missing posts because of sub-minute timing differences is something it would be good to fix. Is that as simple as always rounding seconds down before deciding whether something's in?

Taeir commented 2 years ago

Rounding is a bit complex, so I opted for adding a 5 min leniency instead, which should have the same effect. It depends greatly on the amount of emails to send how long it would take to pass through all subscriptions and send emails, but 5 min should allow for sufficient amount of room.

cellio commented 2 years ago

Sounds good. An expedient solution will hold us for a while, and by time we have the volume that would cause this to be sending out duplicates, we'll probably have a better solution. :-)