D7 port of Notify

Yes, after two recent projects with Notifications module, I have a renewed appreciation for the need for a simple module in the niche that Notify occupies.

I think, I'd like to make it Views-centric; i.e., you create a view and whatever is in that view is what gets mailed.

However, I the issue is lack of time. I'm booked solid for the next 3 months, at least. I'm open to accepting co-maintainers who want to pick up the slack.

Subscribing

Would you be interested in a straight port? I need something simple for a site I'm working on and I think Notify fits. While basing it on Views sounds lovely and would be a great feature, I don't have a lot of time. If you'd be interested in a straight port, I might have a go at it. No promises at this point... I'm still investigating options. But I wanted to at least put the thought out there.

Michelle

Hi Michelle,

I have created a D7 branch at http://drupal.org/node/1227682 feel free to test and patch.

Wow, that was fast! I'm having no luck with any other angle I've tried so this may be my best bet.

Michelle

Attached is a patch to the 7.x DEV branch that creates a working version. It fixes several database errors and other issues within that first attempt. It was nice not having to start from scratch, so many thanks to Ishmael for getting this started. I also attached a tar with the full module for anyone that wants the whole thing.

Found a few issues. Here's a new patch building on #7, with a few changes.
Includes:

• update to readme info file declaration
• add intervals
• fix body markup
• fix session function
• add to registration form
• fix queries
• make menu item placement more standard

Another patch based on the patch of #9.

Changes:

• fixed the configure url
• made all strings translatable
• Fixed a SQL bug about the node type
• Allows you to included fields in the mail
• Temp fix to bug on the user form

When I open config/people/notify/users, the usernames and email where not visible. I might have something to do with this: http://drupal.org/node/1261040. I changed the type to a disable textfield to show something.

I have been using notify with these patches for a couple of weeks on a D7 setup with problems. It might be time for an alpha release of the D7 port?

An additional patch on top of #10. It gives a better output of fields in a mail.

Here is the D7 port of module Notify named Notify2.
http://drupal.org/sandbox/WebEvt/1460518

• Almost all (testing is needed) porting errors are fixed
• Users now have a custom configuration options for notifications per each content type
• Admin can restrict a list of available for notifications content types
• Module schema was changed (in order to have ability to store user settings). To migrate from the old version of the module you should run Update_7000
• Notifications message is formed using template files (notify-comment.tpl.php, notify-node.tpl.php, ..)
• It is advised to use it with mimemail module

Is this different than the dev version dated jun 2011 at the project page? I suspect it is. How do I download it? I clicked on the above git link but it took me to a url that didn't get me far. I'd like to try out this version if possible?

Thanks

?

IWasBornToWin, download the git repository, check out the 7.x-1.x-dev branch and apply the patches found in this thread.

@wpoely,

Could you make one cumulative patch to make reviews easier? Then if scottrigby or john.oltman or someone else can give a positive review, I'll commit it.

@matt2000,

In attachment is a patch with all changes from this thread and it applies cleanly to the 7.x-1.x branch. One patch is git aware, the other one is just a diff. I've split it up in serveral patch to make it looker cleaner.
We've used notify with this patch on D7 for the last 6 months without problems.

@wpoely86, @matt2000,
I run Drupal 7.15. I've installed the current 7.x-1.x-dev (2011-Jul-24) and applied notify-d7port-1159632-17.patch.

Running patch produced the error message below:

patching file README.txt
patching file notify.info
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file notify.info.rej
patching file notify.install
patching file notify.module

I edited notify.info "by hand" by adding the three lines that patch were unable to add, and that seemed to work out OK. At least, installing the patched version on several of my D7 sites went without a glitch.

So as far as I am able to tell, this version (with the patch) works well with Drupal 7. So there is a positive review for this patch from me (for whatever that is worth).

Notify fills a niche that (AFAIK) no other notification module fills. It is perfect for admins (and others) that only want a lightweight module that sends out email notifications about new/changed content on a fairly static site.

I vote for getting this patch into the the development release, since it is a improvement over the current development version and I haven't spotted any real problems with it. (There are minor nits, such as the configuration menu not appearing under admin/configiration, and missing defaults for some of the radio buttons in the UI, but I'll wait submitting issues/patches for those until after a new development release is available, so not to make the patch queue more difficult to follow.)

As an enthusiastic user of the Notify module (and I know of no real alternative), I also hope the maintenance status of the module will be changed from "Unknown" to "Actively maintained".

@gisle,

the patch is for the git 7.x-1.x branch and should apply cleanly on it.

Can you elaborate a bit more on the nits? The configuration menu should appear under Configuration/People/Notification Settings? Which defaults are you missing?

We can better fix all the nits now and be done with it...

@wpoely86,
I am sorry. I simply misunderstood what you meant by the "7.x-1.x branch". I tried to apply the patch to the the notify-7.x-1.x-dev.tar.gz tarball downloaded from http://drupal.org/node/1227682. That didn't work. If I instead use git to clone the 7.x-1.x branch to my local disk, the patch applies cleanly.

You're also right about the configuration menu appearing (as it should) under Configuration/People/Notification Settings. Sorry about overlooking it.

Main nit:
I am able to run the patched version of the D7 port without problems as long as I do not enable Notify new comments for any user. If I enable this, and there are comment notifications in the queue, Notify crashes cron with the following fatal error:

Fatal error: Cannot use object of type stdClass as array in /www/www.elvegaarden.net/htdocs/sites/all/modules/notify/notify.module on line 695

As a quick "fix", disabling Notify new comments for all users makes Notify operational for Notify new content.

My only remaining nit is that the radio buttons in the UI has no default values set when they initially appear - see the screen dump below:

I think they should be set to indicate the following default values: Disabled, Disabled, Title only, Disabled.

Digging some more into notify, I've noticed that Notify sets up new users with sensible defaults if a user register after you've enabled Notify. So if you enable Notify before any users register, you will not see the form in the state shown in the screenshot #20 (i.e. all the radio buttons are unset)

However, Notify does not touch users that already exist when you enable it. So all existing users will be presented with the form in the state shown in the screenshot #20 (i.e. all the radio buttons are unset). If the user then just presses "Save settings" in this state, the following PDOException is triggered:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'status' cannot be null: INSERT INTO {notify} (uid, status, node, teasers, comment) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4); Array ( [:db_insert_placeholder_0] => 4 [:db_insert_placeholder_1] => [:db_insert_placeholder_2] => [:db_insert_placeholder_3] => [:db_insert_placeholder_4] => ) in notify_user_settings_form_submit() (line 392 of /hom/inf5270/www_docs/diw/gruppe01/sites/all/modules/notify/notify.module).

Quick "fix": Tell the users to make sure that they've actually choosen something in all radio button groups before clicking "Save settings".

I've made a fix that just checks the state of the notify object before building the form, and if $notify->status is not set, it quietly adds sensible defaults. I'll upload a patch here shortly.

A better solution would be to make Notify take care of existing users during installation, but for the moment, I think the solution described above has to do.

Changed the Status to "Needs Work".

OK. I've prepared a patch for the D7 port of notify that includes the following changes:

• Cleaned up the code to match D7 coding standard.
• Fixed a trivial typo that made cron crash the site when there were comment notifications queued (see #20 for description of problem). Also see http://drupal.org/node/935458#comment-4950464 and http://drupal.org/node/947264#comment-4948702. Both describes this problem and also describes how to fix it.
• Makes sure the $notify object is never NULL, even when the form is viewed by pre-existing users (see #21 for description of problem).

The patch is attached, and should be applied to the current 7.x-1.x branch in git.

This fixes all the "nits" I've had with the D7 port of Notify. Please review.

(Changed status to "needs review".)

I /think/ this is all good stuff, but it's hard to review the patch quickly because it mixes code-style clean-up with functionality changes.

Also, the fix for #935458: $nodes array modified inappropriately by comment notification logic in _notify_send() should be in that issue queue as it's own patch.

Would it be easy for you to split it into a few seperate patches?

OK, I've split the patch in two.

The first (notify-code_standard-patch_1.git.patch) only changes the formatting to pass automatic code review at http://ventral.org/pareview. I.e. cleaning up the code to match the D7 coding standard. There is no change in logic. The patch should be applied to the current 7.x-1.x branch in git.

The second (notify-bugfix-patch_2.git.patch) does the following:

• Fixes a trivial typo that made cron crash the site when there were comment notifications queued (see #20 for description of problem). Also see http://drupal.org/node/935458#comment-4950464 and http://drupal.org/node/947264#comment-4948702. Both describes this problem and also describes how to fix it.
• Makes sure the $notify object is never NULL, even when the form is viewed by pre-existing users (see #21 for description of problem).

This patch should be applied immediately after applying the first patch (above).

@matt2000, on Sun, 11 Nov 2012 13:48:21 +0000 (UTC), i sent you a PM using the contact form on this site. Did you receive it?

gisle,

I've looked through your patches and they look fine. I'm currently tracking all changes to the 7.x branch on github: https://github.com/wpoely86/notify

@matt2000, I think we are ready for a beta release?

Looked at your 7.x branch on github (https://github.com/wpoely86) and there is one small thing.

I believe that the recommended practice is to not use the t() function inside hook_menu() (see http://drupal.org/node/323101 for the reason).

Otherwise, the HEAD looks fine.

OK, I didn't know that. I learned something new today :)

I've deleted the commit from github.

Thanks!

After the change, the 7.x branch on github https://github.com/wpoely86/notify looks fine to me.

The 7.x-1.x-dev release of Jan. 4, 2013 contains the following changes:

• #1159632 by gisle: Brought it up to Drupal 7 coding standard.
• #1159632 by gisle: Fixed cron crashing site under certain conditions.
• #1159632 by gisle: Makes sure the $notify object is never null.
• #1842634 by indrock: Notifications include name of last user to edit the post.
• #1847618 by gisle: Undefined variable: user in notify_user_cancel().
• Updated help text to reflect Drupal 7 placement of admin pages.

This addresses all outstanding issues for the 7.x-1.x branch.

I hope users of this module will download and test this release thoroughly. Please post reviews in this thread.

The 7.x-1.x-dev snapshot pushed Jan. 13, 2013 contains the following changes:

• #20040 by gisle: Indication of last and next notification date/time.
• #92206 by gisle: Made the purpose of default checkbox clearer
• #1886640 by gisle: Fixes bug in hook_cron().

I am using this module for more than a month on production, and everything works as expected. I also did not come across the fixed bugs reported at #30 and #31.
Everything looks good to publish a stable release.

I am also using this in production, and has not experienced any problems since the release of the dev version of 2013-Jan-13. Since Jan. 13, there have been no bug reports in the issue queue.

No, AFAIK there is absolutely nothing blocking a release.

However, only the official maintainer (matt2000) has the required permissions to make a release, so there will be no release until matt2000 feels like doing so.

Assigning this to the only person with enough permissions to make a release.

Also, if matt2000 for some reason believe that this project is not ready for release, I urge him to put his objections in this issue thread so that we can get the matter resolved and get a release out of the door.

I've now been granted the permission to create releases, and have created an alpha1 release of the project as a first step towards having a full release. This version is almost identical to the dev release of May 14. The only change to the code that has been done is that a query that caused a DB error if the admin tried to change the notification settings of a blocked user has been changed.

Please test it and report any issues you find to the issue queue.

(Also please note that the notify Notify 7.x-1.0-alpha1 release is now available for translation on localize.drupal.org.)

Improved the messages in the status area. Since only tagged releases become available for translation at localize.drupal.org, I pushed the new tagged release immidiately, to give translators access to it.

Updated to latest release.

The following new features and bug fixes has been implemented in release 7.x-1.0-alpha7.

• #2221675: Notify Views Integration
• #2222771: User Notification Settings
• #2182323: Sending hour is not respected in certain cases

Please review.

Remember to check the Status report to see if you need to run the database update script.

(A database update is required if you go from 7.x-1.0-alpha6 or earlier to 7.x-1.x-dev.)

The following new feature has been implemented in release 7.x-1.0-alpha8.

• #2245317: Default for 'Detailed Settings' and bulk subscription

Please review.

Remember to check the Status report to see if you need to run the database update script.

(A database update is required if you go from 7.x-1.0-alpha6 or earlier to 7.x-1.0-alpha8)

This release addresses the following open issues:

• #2182323: Sending hour is not respected in certain cases
• #2246613: Themed name in notification mails

Please review.

Remember to check the Status report to see if you need to run the database update script.

(A database update is required if you go from 7.x-1.0-alpha6 or earlier to 7.x-1.x-alpha9.)

There are no unfixed bugs left in the issue queue, so I am now making this a release candidate.

(This release is identical to the 7.x-1.0-alpha9 release, except for a minor typo that has been corrected.)

Please review.

The following bug has been fixed in release candidate 2: 7.x1.0-rc2.

• #2261523: No notifications queued when nodes created

Please review.

There is now a full release of Notify 1.0 for Drupal 7.

Release note.