Default user notification setting

I second the request for some default actions with Notify. I would like to be able to turn that on by default.

I'd also like this feature. In fact, I'd prefer if the end user just has a single user on whether or not to receive updates, and the admin can configure what will be included.

OK, here's a patch I did up that adds an ability to set the default master switch and configuration options.

It also splits off the configuration options into a separate permission, so those who want users to configure that stuff can allow them to, but those would would prefer to make it simple for the end user can leave permissions for that off.

I also added a master switch to the registration form, which follows the site default.

Whoops, needed to add processing for the registration preference. This one should be better.

I encountered a few problems with this patch, see below. I've tried to be as detailed as possible and have suggested solutions for the critical problems. Let me know if you have any questions.

Critical:

• The registration form submission does not work. No error is displayed on the screen but the user try to register is returned to the registration screen. This error appears in the watchdog logs: "Detected malicious attempt to alter protected user fields.". I was able to fix this by changing the field name of the master switch in the registration form from "status" to "notify_status", and obviously again in the sql that adds the data to the db.
• Once I had the watchdog security error fixed, my notify settings still weren't being written to the database on registration. This can be fixed by changing the sql in the function notify_user() for 'case insert' to use $edit in place of each instance of $form_values.

Major:

• The notification master switch appears on the user registration form even when anonymous users do not have the 'access notify' permission.
• The detailed notification settings do not appear on the user registration form even when anonymous users have the 'configure own notifications' permission.
• While I am able to change the default notification settings, they are never used - other than the default master switch setting on the registration form. This is because on registration, the columns node, comment and teasers are set to 0 in the database, even though the user has never actually configured them. They should be set to the default values on registration. I'm not sure how this should work if the default settings are changed subsequently. Perhaps a new column should be added to the table called 'use_default' or something?

Minor:

• Default value in variable_get() has t() around it. I'm not sure it should have. It happens for variable_get('notify_default_node', t('Enabled')) in $form['notify_page_detailed']['notify_default_node'].
• It is not clear on admin/content/notify that the master switch and detailed settings are the default settings.

Even with the above problems, it's a good addition to the notify module and assuming you can fix these few issues, I'd be willing to set it to 'patch (ready to be committed)'.

Cheers,
Stella

Here's a new patch that should address the critical issues.

I will definitely work on the others, thanks for identifying them. I actually caught one more, feel free to offer your thoughts on how best to resolve it.

When you go to the page to manage notifications (/admin/user/user/notify) it lists all the condiguration options for every user. If, for example, you just wanted to flush the e-mail queue, you would also be creating settings for all listed users.

My goal was to keep all users who hadn't explicitly set preferences to use the site default configuration. That is, if you change the default, they automatically use the new default.

One option I considered was adding a new column, default_config, that, if set, would explicitly tell the notify module to use the site default. I'm a bit leary of making a database change, however.

Any thoughts?

Martin

Well spotted! I think the new column in the database table is the way to go. Otherwise you're going to have to do some complex coding which may be difficult to maintain. It will fix this issue and one of the other ones I found I think. You'll probably need to add a 'use default' column in the user listing for mass user changes though.

So +1 on adding a new column.

I haven't looked at your new patch in detail yet, will have more time to do so tomorrow, but at a first glance I think you're still using 'status' on the registration form, instead of e.g. 'notify_status'. 'status' is a reserved field in the drupal version (5.2 dev) I'm using.

I'll take a more detailed look tomorrow. Gotta go.

Cheers,
Stella

Whoops, looks like I patched to the wrong file. You may as well sit titght on this patch and let me do up a new one that incorporates the new column and some of the other issues.

OK, I was feeling ambitious so I decided to take another crack at this. The attached should address everything except the t('Enabled') observation. My feeling on that one is that it is required, since it must match the values in the options array.

This new version uses a new field, as discussed previously. A database update will be posted immediately following this.

SQL file for database update

Hi,

I've attached my own version of the patch, with a few changes/fixes.

• I agree with you on your point about t('Enabled'), however not all of the variable_get()s had a t() around the default value so I added those in.
• Users who didn't have the 'access notify' permission still had access to the detailed settings if they had 'configure own notifications' permission, so fixed that.
• Fixed issue where anonymous users had a 'My notification settings' link at the top when they went to create an account. It should only appear for logged in users with the appropriate setting.
• Fixed table layout for list of users with notifications enabled. While the 'Defaults' header appeared, no checkboxes for this setting were there.
• On submission of the user list notifications form, it now updates the use_default column in the database.
• Fixed problems with the submission of checkboxes - the data returned is an array not a string.
• Set the values of the detailed settings on the registration form to be that of the defaults, rather than the $notify settings as $notify doesn't exist.

I think it's all working now, you should give it another go.

Cheers,
Stella

Patch attached for the notify.install file to add the 'use_default' column to the 'notify' table - supports both mysql and pgsql.

Stella

Thanks for the quick work! I'd like to try your patch, but I keep getting this error:
patch: **** malformed patch at line 53:

Any thoughts on what might be the cause of the problem? I've tried resaving the patch with different line endings and encodings, but no luck as yet. Any thoughts?

How are you applying the patch? Is that line 53 of notify_123504.patch? If so that's just a blank line and can be ignored.

Here the command I used:
patch -p0 < notify_123504.patch

I did try deleting the line, but then I got:
patch: **** malformed patch at line 53: @@ -74,6 +117,73 @@

Was the patch made in unified format? If I set the flag to force unified format, I also get this error:
missing header for unified diff at line 3 of patch

I'm not sure what the problem is. I created the patch with "cvs diff -u notify.module > notify_123504.patch". I've regenerated it and attached it again to see if it helps.

Thanks for your persistance (and once again for your help). That version worked for me.

Hate to say this, but the patch isn't working for me.

I created the relevant field in the notifications table, and get the options to toggle the Master settings, but new users are still set to disabled...

user above says the patch is not working.

Also, see the coding standards: http://drupal.org/coding-standards

can we have a way to ensure that all users can be made to subscribe to notify by default...once..and then also for new users...
then people can turn it off as they wish..

thanks..
someone pls help on this for Drupal 5.1

Duplicate of http://drupal.org/node/92206.