"Days between reminders" = 2 actually gets you a reminder when it's been 3 days? [#489288]

Hi -

I recently used this module to send out a reminder to a bunch of students about a survey.

Given that today's the last day of the school year, it was supposed to send out the 2nd/last reminder this morning. It didn't.

After much troubleshooting (cron was working, server was sending emails, etc), I finally went into the database and set msg_cnt back to 0, ran cron, got my emails sent, and turned account_reminder off (so that no one would get another reminder after today).

Now that I'm not panicked, I'm looking at the code and seeing this:
(account_reminder.module, line 188)
elseif (((int)((time() - $row->last_reminder) / 86400)) > $period && ($row->msg_cnt >= 0 && ($row->msg_cnt < $total_messages))) {

Since it's casting to (int)... maybe it's rounding down and it would have sent out the email tomorrow once it reached 3 - aka >2 - days? (or this evening, should the casting to int resulted in it rounding up)

Not sure if you'd classify this as an actual bug or if I just misunderstood the meaning of "Days Between Reminders" - I'm sure most folks don't need this to be so precisely timed. Just thought I'd let you know.

Thanks for your module and your efforts; it helped us get our project done.

-R

Comments

Comment #1

jaydub CreditAttribution: jaydub commented 25 June 2009 at 08:09

Status:

Needs review

» Fixed

I took a look at this and from reading the PHP docs it looks like casting always rounds towards zero. So casting a float to an integer where the float is 2.66 or 2.33 will result in an integer value of 2. See http://www.php.net/manual/en/language.types.integer.php#language.types.i...

The code here that tries to determine the number of days that has passed:

(int)((time() - $row->last_reminder) / 86400)) > $period

is checking how many seconds have passed from the last reminder to the current time. That number of seconds is divided by the number of seconds in a day (86400). The resulting value is usually going to be a float so to it is then cast to an integer. The final resulting value is tested against the configured number of days to wait for the next reminder message to be sent.

So first question answered is the cast question. The answer is that it's always rounded down. And that's the behaviour we want here.

I'm realizing though that instead of checking to see if the resulting value is greater than the configured wait period, it should be checked for greater than or equal to.

The reasoning being that if the float value from the equation is 3.33 and the configured wait period is 3 days, then the test for only greater than will fail after the cast to integer. So I'm thinking that the test should be greater than or equal to so that this condition would be met.

I'm making the change to the development snapshot and this will be fixed in the next release. Thanks for finding this issue out.