More testing from greggles in IRC regarding #86462: add backend support for cck date fields uncovered this slightly annoying gotcha. If you have a bunch of existing nodes with CCK date fields that were signup enabled, and then you upgrade to the latest version, the next cron run will send out reminder emails for all your events, even really old ones. This is caused by two things:

A) The somewhat evil way signup handles reminder email settings. Even though you couldn't see it, all those old CCK date nodes that you signup-enabled have reminder emails selected by default, using the site-wide default email templates. See #290305: Split out email functionality into separate submodule(s) for more.

B) The SQL to figure out what nodes to send reminders for checks to make sure that the "reminder window" has already started, but assumes that window never ends.

Fixing (A) is a larger can of worms, and infeasible for the 5.x-2.6 release. However, fixing (B) should be pretty easy. We just need to add another WHERE clause to the reminder SQL in each scheduler backend to restrict it to nodes that haven't already started yet.

We *might* also want to have something in the reminder processing code from cron to clear the "send_reminder" bit on all nodes that have already started. However, that would probably just be bloat, and won't necessarily save us any grief here, since we'd still need to have the WHERE clause described above. Note that we can't just do this as a 1-time solution via signup_update_N() since if you upgrade to 5.x-2.6 and immediately run update.php, signup won't know what CCK date field to use for any of your node types.

CommentFileSizeAuthor
#3 329708_signup_no_late_reminders.3.patch4.77 KBdww

Comments

dww’s picture

Title: Reminder emails can be sent for nodes » Reminder emails can be sent for nodes that already started
greggles’s picture

"subscribe"

dww’s picture

Status: Active » Needs review
StatusFileSize
new4.77 KB

Lightly tested with date 5.x-1.*. Should be fine for the others, but more testing would be lovely. ;)

dww’s picture

Status: Needs review » Fixed

Tested with all 4 scheduling backends and it all seems to be working fine. Committed to HEAD and DRUPAL-5--2. I don't think it's worth backporting this to DRUPAL-5 where there's only event 5.x-1.* support.

greggles’s picture

That seems reasonable to me. Thanks for this fix!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.