Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comments
Comment #1
izmeez CreditAttribution: izmeez commentedThanks 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.
Comment #2
izmeez CreditAttribution: izmeez commentedI forgot to change the status to needs review.
Comment #3
izmeez CreditAttribution: izmeez commentedThe 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.
Comment #4
izmeez CreditAttribution: izmeez commentedOn 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.
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.
Any suggestions as to what I am overlooking and how to fix the second of these?
Thanks.
Comment #5
izmeez CreditAttribution: izmeez commentedThis 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
Comment #6
matt2000 CreditAttribution: matt2000 commentedHi 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!
Comment #7
izmeez CreditAttribution: izmeez commentedMatt,
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
Comment #8
izmeez CreditAttribution: izmeez commentedWhile 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.
Comment #9
izmeez CreditAttribution: izmeez commentedFor anyone else who is interested in these patches one needed to be re-rolled against Notify 6.x-1.2 and is attached.
Comment #10
parasolx CreditAttribution: parasolx commentedthere 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
to
since D6 need to set the second argument while D7 can be null. and also the space between return and ;
Comment #11
parasolx CreditAttribution: parasolx commentedand also some fixed in user profile page.
at line 335, from
to
at line 352, from
to
at line 367, from
to
same like above, D6 need 2 argument for variable_get.
Comment #12
parasolx CreditAttribution: parasolx commentedi 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:
it supposed to be fixed:
Comment #13
izmeez CreditAttribution: izmeez commented@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?
Comment #14
parasolx CreditAttribution: parasolx commentedas 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
Comment #15
izmeez CreditAttribution: izmeez commented@parasolx
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.
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
Comment #16
izmeez CreditAttribution: izmeez commented@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.
Comment #17
izmeez CreditAttribution: izmeez commentedSorry, forgot to add the patch for comment #16. Here it is.
Comment #18
ishmael-sanchez CreditAttribution: ishmael-sanchez commentedComment #19
izmeez CreditAttribution: izmeez commentedThis 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.
Comment #20
izmeez CreditAttribution: izmeez commentedI 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.
Comment #21
gisleNotify 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.