A minor change I would suggest is the wording on the admin/settings/notify page for notification check box on registration to:

Show email notification checkbox on new registration form

Another checkbox could be useful for

Enable email notification as default for new users

Sorry if this is a duplicate, I thought it best to make it a separate issue even though there are discussions on the default settings for ease of opting in or opting out configuration.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

izmeez’s picture

Title: Notify module admin settings notification on registration » Notify module admin settings on registration, adding defaults and details permission
FileSize
21.96 KB
12.58 KB
22.11 KB
115.72 KB
18.52 KB

Thanks very much for this very useful module.

I found myself in the vortex of the code in this module.

I have applied the patch for timed cron, http://drupal.org/node/869054#comment-3275754

I have also applied the stripped yamboy patch from http://drupal.org/node/365700#comment-3277356 because adding "Last send" is a great addition to admin/user/user/notify and user/%/settings.

I added this rather large patch which is attached.

Patch for notify default settings and detail permissions:

a) admin/settings/notify UI changes
-- Show on register improved,
-- -- no longer requires notify access permissions,
-- Set default status for new users
-- -- supports default status for opt-in or opt-out configuration
-- Set default details

b) admin/user/user/notify
-- shows status
-- shows all notify records status on or off

c) user/%/notify
-- Use default detail settings for new records
-- Preserve last send when changes made
-- User permissions to view notification details

d) minor code cleanup

The attached patch needs testing.

I've also attached some screen captures to illustrate the changes.

izmeez’s picture

Status: Active » Needs review

I forgot to change the status to needs review.

izmeez’s picture

The patch in #1 includes the reversal of the first patch for timed cron and patching with the second patch for timed cron. This may not fully succeed when the patch is applied to the notify module as it is.

Attached is a second patch that already presumes the second timed cron patch is applied and also the stripped yamboy patch.

This patch should fully succeed when applied.

izmeez’s picture

On testing this patch the only problem I am having is in terms of preserving the last send date.

When changes are made at admin/user/user/notify with the patched notify_admin_users_submit the last send date is preserved. Here is that segment of the patch. It is using db_query UPDATE.

@@ -370,7 +447,7 @@ function notify_admin_users_submit($form
 
   if ($form_state['values']['users']) {
     foreach ($form_state['values']['users'] as $uid => $settings) {
-      db_query('UPDATE {notify} SET node = %d, teasers = %d, comment = %d, attempts = %d WHERE uid = %d', $settings['node'], $settings['teasers'], $settings['comment'], $settings['attempts'], $uid);
+      db_query('UPDATE {notify} SET status = %d, node = %d, teasers = %d, comment = %d, attempts = %d WHERE uid = %d', $settings['status'], $settings['node'], $settings['teasers'], $settings['comment'], $settings['attempts'], $uid);
     }
   }
   drupal_set_message(t('Notify settings saved.'));

When changes are made at user/%/notify with the patched notify_user_settings_form_submit the last send date is NOT preserved. Here is that segment of the patch. I am not sure why this is not working and wondering why db_query DELETE followed by INSERT are used.

@@ -324,7 +397,7 @@ function notify_user_settings_form(&$for
 function notify_user_settings_form_submit($form, &$form_state) {
   unset($form);
   db_query('DELETE FROM {notify} WHERE uid = %d', $form_state['values']['uid']);
-  db_query('INSERT INTO {notify} (uid, status, node, teasers, comment) VALUES (%d, %d, %d, %d, %d)', $form_state['values']['uid'], $form_state['values']['status'], $form_state['values']['node'], $form_state['values']['teasers'], $form_state['values']['comment']);
+  db_query('INSERT INTO {notify} (uid, status, node, teasers, comment, send_last) VALUES (%d, %d, %d, %d, %d, %d)', $form_state['values']['uid'], $form_state['values']['status'], $form_state['values']['node'], $form_state['values']['teasers'], $form_state['values']['comment'], $form_state['values']['send_last']);
   drupal_set_message(t('Notify settings saved.'));
 }
 

Any suggestions as to what I am overlooking and how to fix the second of these?

Thanks.

izmeez’s picture

This patch adds the feature to add notifications to existing users who do not have settings already, using default settings.

This is added to admin/user/user/notify and is dependent on the patch in #3.

Izzy

matt2000’s picture

Hi izmeez,

Thanks for jumping in and helping to improve the module. I appreciate your enthusiasm.

However, it's difficult to review patches that add multiple new features and/or are dependent on other patches. Ideally, each new feature should have it's own issue queue and patch.

For a discussion of the reasoning behind this, see: http://webchick.net/please-stop-eating-baby-kittens

Thanks!

izmeez’s picture

Matt,

Yes, I can appreciate the difficulties this poses and have been wondering how best to share these ideas.

I am trying my best to post in appropriate issues, but essentially the changes that we have found needed to Notify 6.x-1.1 are:

1. Patch for timed cron which is committed to 6.x-1.2, http://drupal.org/node/869054

2. Patch for a last send column, http://drupal.org/node/365700

That issue has been around for a long time and includes various pieces, some of which are committed in 6.x-1.1

The issue includes the patch using a last send column.

Whether or not this adequately addresses the critical issue for large or slow sites or is the best way to not spam users is still under discussion. If it works, that's good for us when this is put on a production site.

There are other significant benefits to adding a last send column.
Users can suspend their notifications and see when the last send occurred.
New features could be added to remind users to re-enable their notifications.

3. Patch for defaults and detail permissions, which is in this thread.

A separate patch to add user permissions to access notify details at user/%/notify is simple enough but in order for this to work it is essential that defaults can be set for the detail settings.

That is why they are included together in the patch in this thread.

While dealing with default settings it makes sense to deal with outstanding issues of checkbox on registration and default status which are unnecessarily linked. Yes, it could be a separate patch but would again be dependent on having default settings.

Granted, changes to the admin/user/user/notify table to show list of all users with notify settings whether status is on or off could have been a separate patch but was easy to do at the time and shows users who choose to suspend their notifications for holidays, etc.

4. Patch to add existing users is a separate patch although I have posted it to this issue because it is again dependent on having default settings.

Together, the notify module with these patches gives us a very good module for production sites.

Thank you very much for all your efforts.

Izzy

izmeez’s picture

While waiting for direction from others on how else these patches could be brought forward without causing more confusion there is a minor improvement to the add existing users patch in #5. The attached patch excludes users 0 & 1 so that notify records are not created for anonymous or user1.

izmeez’s picture

For anyone else who is interested in these patches one needed to be re-rolled against Notify 6.x-1.2 and is attached.

parasolx’s picture

Status: Needs review » Reviewed & tested by the community

there is missing of second argument on line 201 in notify.module. I got this error on registration page:

warning: Missing argument 2 for variable_get(), called in /home/***/public_html/sites/all/modules/notify/notify.module on line 201 and defined in /home/***/public_html/includes/bootstrap.inc on line 583.

so what need to be fixed at line 201 is
from

  if (0 == variable_get('notify_reg_default')) return ;

to

  if (0 == variable_get('notify_reg_default', 1)) return;

since D6 need to set the second argument while D7 can be null. and also the space between return and ;

parasolx’s picture

and also some fixed in user profile page.
at line 335, from

    $node_value = variable_get('notify_node_default');

to

    $node_value = variable_get('notify_node_default', 1);

at line 352, from

    $teasers_value = variable_get('notify_teasers_default');

to

    $teasers_value = variable_get('notify_teasers_default', 0);

at line 367, from

    $comment_value = variable_get('notify_comment_default');

to

    $comment_value = variable_get('notify_comment_default', 0);

same like above, D6 need 2 argument for variable_get.

parasolx’s picture

i test for new registration user.. wonder why there is no automatic insert into notify table.
it because still left bugs in query at line 481:

        db_query('INSERT INTO {notify} (uid, status, node, teasers, comment, send_last) VALUES (%d, %d, %d, %d, %d, %d)', $one->uid, variable_get('notify_status_default'), variable_get('notify_node_default'), variable_get('notify_teasers_default'), variable_get('notify_comment_default'), 0);

it supposed to be fixed:

        db_query('INSERT INTO {notify} (uid, status, node, teasers, comment, send_last) VALUES (%d, %d, %d, %d, %d, %d)', $one->uid, variable_get('notify_status_default', 1), variable_get('notify_node_default', 1), variable_get('notify_teasers_default', 1), variable_get('notify_comment_default', 1), 0);
izmeez’s picture

@parasolx Thanks for your findings and fixes. I will take a look at these and try to post back a patch.

As you may have noticed I am not the module maintainer and the module maintainer is not happy with this group of changes with several patches. Maybe you have some thoughts on how to offer these changes in a way that may be more acceptable for inclusion?

parasolx’s picture

as the maintainer state, we need to split this issues into several part.

1. altering default setting modification
2. permission details
3. mass apply existing users

but, it should commit this first: http://drupal.org/node/869054#comment-3275754 into -dev

then this http://drupal.org/node/365700#comment-3277356 into -dev.

after that we can continue patching with this function. if not, all code will mess up everywhere. to patch this things also quite confuse since need to apply both great patch above before apply this advance patch. this is what the author said.

and to make it fast checkup by d.o team, change status to "reviewed & tested by community" once every patch was being tested and work fine. if not, all patch will remain at same state -- not commit into -dev

izmeez’s picture

@parasolx

but, it should commit this first: http://drupal.org/node/869054#comment-3275754 into -dev

I think that has been committed and a later poster changed the status from fixed to active. I have added a post to that issue.

then this http://drupal.org/node/365700#comment-3277356 into -dev

Comment #12 in that thread suggests a bit more is required. I'll take a look and see if I can help move that forward.

Then we can come back and split this issue up into separate issues if that is what is required.

Thanks,

Izzy

izmeez’s picture

Status: Reviewed & tested by the community » Needs review

@parasolx I have added the changes suggested in comments #10, 11, and 12 with the new patch attached.

This works in combination with the patch in comment #8 http://drupal.org/files/issues/870812-notify_add_existing_users_2.patch

I have changed the status to needs review.

Thanks.

izmeez’s picture

Sorry, forgot to add the patch for comment #16. Here it is.

ishmael-sanchez’s picture

Assigned: Unassigned » ishmael-sanchez
izmeez’s picture

This feature includes two patches and is dependent on the patch in http://drupal.org/node/365700 being committed. The two patches are in comment #8 and comment #17.

izmeez’s picture

Status: Needs review » Needs work

I am changing the status of this to needs work.

If user does not have permission to view notifications detailed settings and changes master switch Notify status from enabled to disabled the current detailed settings for the user are not preserved and when user later enables Notify status the detailed settings are now different. This looks like hunk 9 of the patch needs more work.

gisle’s picture

Version: 6.x-1.x-dev » 7.x-1.0-alpha7
Issue summary: View changes
Status: Needs work » Fixed

Notify for Drupal 7 has the following setting, which fulfills this feature request: "Notification checkbox default on new user registration form."

Notification checkbox default on new user registration form: "The default master switch for new users (check for enabled, uncheck for disabled)."

This was fixed a long time ago, but hasn't bee removed from the queue.

Status: Fixed » Closed (fixed)

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