Date Reminders lets users request a reminder (at least an email, though messaging framework supports other notification types) at a given time before an event. Users can request several reminders at different times before an event. (A week before and a day before, for example.) For a repeating event, reminders are sent before each occurrence of the event.

"Event" means any node with at least one date field, not specifically Event module nodes.

Pending reminders are kept in a database. Each database entry records nodeid, userid, "lead time" (time before event for the reminder), target email (if the user was so privileged), and next schedule reminder time. A cron job checks for "due" reminders. For any found, it sends the reminder then either deletes it or finds the next due time for a repeating date.

System administrator can control such things as which node types support reminders. A suitably privileged user can also specify one or more email addresses for the reminder; normally reminders to to the user's registered address.

Here's the project page: http://drupal.org/sandbox/dwillcox/1541054

Some discussion here: http://drupal.org/node/1484608

Currently only Drupal 6, but I'd like to make a D7 version once the D6 one gets stabilized. (Though I've read that the D7 Messaging module may not be complete.)

git clone:
git clone --recursive --branch 6.x-1.x dwillcox@git.drupal.org:sandbox/dwillcox/1541054.git date_reminder

I haven't done any other project reviews yet. I'll update here if I manage to do that.

This isn't exactly a review, perhaps, but at least a comment on another application:
http://drupal.org/node/1696900#comment-6275622

Comments

mantish’s picture

mantish commented

Hi dwillcox,

I did a manual review of your code and found some issues.

File datereminder_form.module
- On line 144, i can see that $content is concatenated, but the variable is no where initialized. Better assign the value instead of concatenating.
- On line 217 n 218, you have used $nid which is never initialized.
- On line 229 $query is not initialized.
- On line 232 "nids" should have been "$nids" if "nids" is not a constant, If its a constant then use caps to define constants.
- On line 279 $type is not initialized.
- Use drupal_write_record() for inserting and updating the database. Avoid "insert" and "update" sql queries.
- "return (null);": paranthesis are not necessary here? Also elsewhere.

File datereminder_form.inc
- Line 407, $rem not initialized.
- Line 425, $form not initialized.

Regards,
M

mantish’s picture

mantish commented
Status: Needs review » Needs work
dwillcox’s picture

dwillcox commented

Thanks for the observations.

File datereminder_form.module
- On line 144, i can see that $content is concatenated, but the variable is no where initialized. Better assign the value instead of concatenating.

Line 154. Fixed

- On line 217 n 218, you have used $nid which is never initialized.

I don't see this.

- On line 229 $query is not initialized.

Are we looking at the same version of code? I can't find uninitialized $query. But I did find both $query and $q used to mean the same thing. Fixed.

- On line 232 "nids" should have been "$nids" if "nids" is not a constant, If its a constant then use caps to define constants.

I can't find this.

- On line 279 $type is not initialized.

Oops, should have been $info->type.

- Use drupal_write_record() for inserting and updating the database. Avoid "insert" and "update" sql queries.

Generally, I do use drupal_write_record(). However, the API makes it sound like you can't use the "ON DUPLICATE KEY" functionality. In other words, you have to know beforehand whether there's already a record. The extra logic seems a waste. If it's OK to use $update='field' when you know the primary key value but don't know that the record exists, let me know.

- "return (null);": paranthesis are not necessary here? Also elsewhere.

No, the parens aren't necessary, but more parens is always better than less. Plus, my C background makes "return $value;" just look wrong. I'll admit I'm not consistent, but this is harmless.

File datereminder_form.inc
- Line 407, $rem not initialized.
- Line 425, $form not initialized.

Hey, guess what? datereminder_build_reminder_form() was an experiment that I abandoned. It's never used. Removed now.

mantish’s picture

mantish commented

Hi dwillcox,

- On line 232 "nids" should have been "$nids" if "nids" is not a constant, If its a constant then use caps to define constants.

I can't find this.

Following is the code on line 232.

    if (count(nids) > 0) {

here what is nids ? i guess it must be $nids.

- On line 217 n 218, you have used $nid which is never initialized.

I don't see this.

Following is the function

function _datereminder_clean_node_reminders($nids) {
  if (!is_array($nids)) {
    $nids = array($nids);
  }
  $ph = db_placeholders($nids);

  db_query ("DELETE FROM {datereminder} WHERE nid IN ($ph)", $nid);
  db_query ("DELETE FROM {datereminder_enable} WHERE nid IN ($ph)", $nid);
}

on the following lines you have used $nid, but they are never initialized.

db_query ("DELETE FROM {datereminder} WHERE nid IN ($ph)", $nid);
  db_query ("DELETE FROM {datereminder_enable} WHERE nid IN ($ph)", $nid);

Regards
M

dwillcox’s picture

dwillcox commented

Darn, I've been staring at this stuff too long. You're right. Thanks.

Fixed.

mkiyota’s picture

mkiyota commented
Issue tags: +cron

Hi dwillcox,

Thank you so much for your effort to develop this module. This is exactly what I was looking for. I have been testing the module and have an issue. When I set a reminder, then the reminder is sent on the requested time
but after that it keeps sending the same reminder again and again. For instance, if cron is configured to run every minute, the reminder is sent every minute after the requested time. Is this due to the cron setting or something to do with Date Reminder module. Any help would be appreciated.

Sincerely,

MK

patrickd’s picture

patrickd commented
Issue tags: -cron

Before adding random tags read the issue tag guidelines. Do NOT use tags for adding random keywords or duplicating any other fields. Separate terms with a comma, not a space.

dwillcox’s picture

dwillcox commented

Say patrickd -

Sorry, I don't know what you're referring to. Have issue tags been used here at all?

Was this directed at the project application, or one of the responses herein?

dwillcox’s picture

dwillcox commented

To mikiyota -

I had heard this comment before, maybe an email, maybe from you, but I don't know how to reproduce this. On my site, when a reminder is sent by cron and there are no more recurring instances, the reminder database entry is deleted.

Can I request that you create a new issue here: http://drupal.org/project/issues/1541054?status=All&categories=All

We can continue discussion there, rather than here; this "issue" is about the request for approval as a project.

The first thing I'll ask: Are you using the version from the 6.x-1.x branch, or master? I just noticed that "master" was set as the default branch in the sandbox configuration, and that's wrong. The master branch is nonfunctional, and maybe I should remove it.

If you really are using 6.x-1.x, please report as much as you can about your environment. At least cron frequency, and how the date in your node was set. For example, was this a one-time event, or repeating. If repeating, was did this "resend notices" happen after the last occurrence, or an earlier one? If you could dump the content of the "datereminder" table that would be useful, too.

And make sure you're using the most recent version. (There are no version numbers, just be sure you get the latest from git.)

Oh, and by the way, running cron once per minute might be a bit much, especially if your site has any size at all.

patrickd’s picture

patrickd commented

mkiyota had added some random tags I've removed

dwillcox’s picture

dwillcox commented

patrickd -

OK, thanks for the clarification.

There are lots of twisty little turns in the Drupal project world that I'm slowly learning to negotiate.

dwillcox’s picture

dwillcox commented
Status: Needs work » Needs review
dwillcox’s picture

dwillcox commented

I deleted the master branch to avoid any future confusion. It had nonfunctional code.

rakeshks’s picture

rakeshks commented
Status: Needs review » Reviewed & tested by the community

module looks good . also tested coding standered.

rakeshks’s picture

rakeshks commented
Issue summary: View changes

Add reference to other review comment

dwillcox’s picture

dwillcox commented
Issue summary: View changes

Minor typographical update.

dwillcox’s picture

dwillcox commented

For what it's worth... This isn't exactly a review comment, but a user reports that Date Reminder works with VoIP messaging. See here: http://drupal.org/node/1484608#comment-6292936.

patrickd’s picture

patrickd commented

Your project page could contain some more information, please have a look at the tips for a great project page, you may also use HTML-tags for better structure. Your readme is quite detailed, you should keep project page and readme in sync.
Also have a look at Commit messages - providing history and credit

you've committed a 's' file, which is a git-diff, I don't think it belongs in here

Remove ; $id$ tags, they are not needed anymore as we're now using GIT on drupal.org

Are there other modules using the "package = Date/Time" package?

Comments in-functions should always be using //
Comments outer-functions should always be using /* */

Do not set default values on installation, this is what the second parameter in variable_get is made for

There are some little coding style issues left you may want to fix: http://ventral.org/pareview/httpgitdrupalorgsandboxdwillcox1541054git

Unfortunately this module is pretty large and I don't have much experience with d6 modules,
I'll try to have another look tomorrow

dwillcox’s picture

dwillcox commented

Patrickd, thanks for taking the time to look at this. Yeah, there are an awful lot of project applications out there.

Your project page could contain some more information, please have a look at the tips for a great project page, you may also use HTML-tags for better structure. Your readme is quite detailed, you should keep project page and readme in sync.

I added some more information to the project page. I thought I was following the "normal" level of info on project pages, but I guess more is always better. In particular, I added some information from the README file. And I'll keep working on it.

I'd like to separate the documentation and project page, but I'm not sure how to set up a separate documentation page. I'm still pretty new to drupal.org.

One thing I'm unclear on: How to upload images to use in "Screenshots" without them automatically appearing alongside the text. I want them to appear only where I insert them.

Also have a look at Commit messages - providing history and credit

Well, true. I've been fairly lax about commit messages, but I figured in a sandbox the detailed messages weren't as important. But I'm learning. :)

you've committed a 's' file, which is a git-diff, I don't think it belongs in here

Oops. I'm not sure where that came from. Apparently git thinks it knows what you want better than you. Gone.

Remove ; $id$ tags, they are not needed anymore as we're now using GIT on drupal.org

I thought I had done that, but apparently missed one. Gone.

Are there other modules using the "package = Date/Time" package?

Actually, a fair number. CCK date fields, Calendar, Calendar Popup, Date Popup, Date Repeat API, Date Tools, just as an example. It seems like the most logical group for Date Reminder. I'm not wedded to it, though, and am open to other suggestions.

Comments in-functions should always be using //
Comments outer-functions should always be using /* */

I'll admit that some of these stylistic restrictions seem a bit arbitrary. But OK, I'll go with the flow.

Do not set default values on installation, this is what the second parameter in variable_get is made for.

Done.

There are some little coding style issues left you may want to fix: http://ventral.org/pareview/httpgitdrupalorgsandboxdwillcox1541054git

Done.

Unfortunately this module is pretty large and I don't have much experience with d6 modules,
I'll try to have another look tomorrow

I feel your pain. I'm in the opposite boat; I'm still firmly in the D6 world. I want to port this to D7, but want to get the D6 version out first. I was going to try reviewing some other projects, but almost everything is D7, and I haven't started coming up to speed on D7 yet.

patrickd’s picture

patrickd commented

You can create a documentation subpage for your module here: http://drupal.org/documentation/modules/contributions
Just scroll down and clock "add childpage" and name it like your module

Images can be uploaded here: http://drupal.org/node/add/image
Then use html img tags to put them into your project page where you want them to

about the package, if there are other modules using this package too, it's already a good choice!

going to have a deeper look today, as the general consensus about big modules in the application queue is "you only have to review as much you can" I'll do some security testing and fix this. ;)

patrickd’s picture

patrickd commented
Status: Reviewed & tested by the community » Fixed

only use module_load_include() within functions, not in global space
have a look at http://drupal.org/coding-standards#includes

Had a quick glance at the rest of the code and did some basic security test, but everything looks fine for me,

Thanks for your contribution!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Put correct branch in example "git clone." It was moved from master branch some time ago.