default notifications

I agree that the granularity provided by the ad_notify module makes it tedious to manage notifications for lots of ads.

I plan to address this by making it possible to setup default notifications that will then be applied to all new ads. Note that changing the default notifications will not change existing ads, as this still allows you to change the notification settings of ads on a per-ad basis. (I will consider adding a manual 'reset to defaults' type option, however.)

Updating the title to reflect my plans, and assigning to myself.

I am wondering if a better solution to this is to use a Views Scheduler type feature which sends you emails of a given View on a periodic basis. This could get the recipient the data he wants in a single email, not dozens of single ad emails.

There may be a better maintained module than Views scheduler but you get the idea.

I like the idea of using a third-party module, but the views scheduler has more dependencies than I'm looking for.

Moving to 6.x-2.x-dev. As requested here, "Role-specific defaults would be extra nifty...".

Damn, drupal.org eats comment after you upload some files.

Here's coment to previous patch. Patch includes:
- Global (default) notifications
- Token support
- Roles support

I started reviewing this patch, however it requires a file called "token_ad.inc" which is not included in the patch. Also, I assume this module is now dependent on the token module, which means ad_notify.info should be updated.

In function ad_notify_overview_form() there is a debug call to dpm() which shouldn't be there.

Otherwise, it looks like it's very much a step in the right direction. Thanks!

Here's an update with everything included and fixed.

I'm unable to apply the patch. In particular, it seems that it was created against an older version of the ad_notify.module:

patching file notify/ad_notify.module
Reversed (or previously applied) patch detected!  Assume -R? [n]   
Apply anyway? [n] y
Hunk #1 FAILED at 62.
Hunk #2 FAILED at 71.
Hunk #3 FAILED at 94.
Hunk #4 FAILED at 202.
Hunk #5 FAILED at 251.
Hunk #6 FAILED at 355.
Hunk #7 succeeded at 312 with fuzz 1 (offset -74 lines).
Hunk #8 succeeded at 359 (offset -50 lines).
Hunk #9 succeeded at 462 (offset -44 lines).
Hunk #10 succeeded at 475 with fuzz 1 (offset -44 lines).
Hunk #11 succeeded at 485 (offset -53 lines).
Hunk #12 FAILED at 580.
7 out of 12 hunks FAILED -- saving rejects to file notify/ad_notify.module.rej

I tried to apply the rejects manually, but the patch seems to have been built from a widely divergent version of the ad_notify module. It's removing large chunks of code that don't even exist in the 6.x-2.x-dev version.

Hm, very strange. I'll figure it out.

Rerolled the patch (against DRUPAL 6x-2), hope this will apply.

Awesome, thanks. Applied with some changes -- some very minor, some major and necessary to get it somewhat working:

• Grammatical cleanup
• Display different help message if setting global notifications
• Change "views" to "impressions" where visible to users
• Provide descriptions for all statistics
• Fix menu for user's viewing their own notifications

Still broken, if you go to an advertisement, click 'Ad owners', then 'notifications' for any owner you can create a notification but it never shows up.

I have not yet tested whether or not notifications themselves actually work, whether emails are sent and whether tokens are properly replaced.

Anyway, committed so we can keep moving forward on this.

As I see from commited version, ad_notify_overview_form's params wasn't changed. It seems that you manually rewritten some stuff in begining of the function—only nid-uid were there, but now there are $node and $owner objects. And of course, it broke up 'My notifications' because of wrong parameters passing from hook_menu. I see no reason to have objects there, bacause only ->nid and ->uid are being used in the function. I need to revert these changes or rewrite everything else to proceed.

Reworked ad_notify_overview_form params and all linked stuff, left $owner as object.
Fixed bug with notification tab in non-ad nodes.
Made redirects to 'My notifications' if user adds/edits/removes own notifications.
Fixed creation under 'Ad owners'.

When I apply your patch, and then go to the notifications page for an ad owner, I see the following which is what I fixed by changing %nid to $node and %uid to $user. Applying your patch returns us to these errors:

    * notice: Object of class stdClass could not be converted to int in /var/www/ad69/includes/database.inc on line 211.
    * recoverable fatal error: Object of class stdClass could not be converted to string in /var/www/ad69/includes/bootstrap.inc on line 758.
    * notice: Object of class stdClass to string conversion in /var/www/ad69/includes/bootstrap.inc on line 758.
    * warning: preg_match() expects parameter 2 to be string, object given in /var/www/ad69/includes/bootstrap.inc on line 761.

To view a Drupal 6 website with E_ALL, you need to not only set E_ALL in your php.ini, you also need to modify common.inc. Change the line that reads as follows:

  if ($errno & (E_ALL ^ E_NOTICE)) {

In my tree, I've changed it as follows:

  //if ($errno & (E_ALL ^ E_NOTICE)) {
  if ($errno & (E_ALL)) {

Thank you very much, finally after editing commons.inc I was able to reproduce it. Here's new patch.

I have applied the patch and now I am getting the following when I try to edit the notifications for a specific ad owner:

    * notice: Object of class stdClass could not be converted to int in /var/www/ad69/includes/database.inc on line 211.
    * recoverable fatal error: Object of class stdClass could not be converted to string in /var/www/ad69/includes/bootstrap.inc on line 758.
    * notice: Object of class stdClass to string conversion in /var/www/ad69/includes/bootstrap.inc on line 758.
    * warning: preg_match() expects parameter 2 to be string, object given in /var/www/ad69/includes/bootstrap.inc on line 761.

When I actually create a new notification, it shows up as a blank line in the Notifications fieldset. I also see that breadcrumbs are missing, making it difficult to get back to the ad owners page.

If I click on the "My notifications" tab and create a notification, this works. If I then go to the "Ad Owners" tab and then click "notifications" next to my name, the notification I created is not there. Notifications I try to create on this page are blank, and do not show up on the "My notifications" tab.

If I try to "edit" an existing notification, I am taken to the "Global Notifications" tab and am suddenly in the Admin interface for the Ad module.

I have not yet tested if notifications are actually sent when cron runs.

Here is a screen shot showing the errors I'm getting, the lack of breadcrumbs, and the blank notification that I created. (The non-blank notification is one I already created.)

Revised version. Also added removal handling for default notifications. Administrator can now remove them completely for anyone or just remove from further usage (leave existing).

Hope this one will be fine, I found no errors/warnings/notes after long testing.

The patch no longer applies cleanly due to unrelated cleanups in the same code. In particular, the _access function is no longer needed (and a bad idea, as I believe _access is a hook used by other modules and thus could cause unexpected errors).

I manually resolved this issue, and have run into further problems. When I click the 'My notifications' tab, I see the following errors:

    * notice: Object of class stdClass could not be converted to int in /var/www/ad69/includes/database.inc on line 211.
    * recoverable fatal error: Object of class stdClass could not be converted to string in /var/www/ad69/sites/default/modules/ad/notify/ad_notify.module on line 314.
    * recoverable fatal error: Object of class stdClass could not be converted to string in /var/www/ad69/sites/default/modules/ad/notify/ad_notify.module on line 314.
    * notice: Undefined index: in /var/www/ad69/sites/default/modules/ad/notify/ad_notify.module on line 311.
    * recoverable fatal error: Object of class stdClass could not be converted to string in /var/www/ad69/sites/default/modules/ad/notify/ad_notify.module on line 314.
    * recoverable fatal error: Object of class stdClass could not be converted to string in /var/www/ad69/sites/default/modules/ad/notify/ad_notify.module on line 314.
    * recoverable fatal error: Object of class stdClass could not be converted to string in /var/www/ad69/includes/bootstrap.inc on line 758.
    * notice: Object of class stdClass to string conversion in /var/www/ad69/includes/bootstrap.inc on line 758.
    * warning: preg_match() expects parameter 2 to be string, object given in /var/www/ad69/includes/bootstrap.inc on line 761.

When I try to create a notification, I end up at the nonsensical url "http://localhost/ad69/node/%252Fadowners/3/notifications" and get the following errors:

Page not found

    * notice: Object of class stdClass could not be converted to int in /var/www/ad69/includes/database.inc on line 211.
    * recoverable fatal error: Object of class stdClass could not be converted to string in /var/www/ad69/sites/default/modules/ad/notify/ad_notify.module on line 314.
    * recoverable fatal error: Object of class stdClass could not be converted to string in /var/www/ad69/sites/default/modules/ad/notify/ad_notify.module on line 314.
    * notice: Undefined index: in /var/www/ad69/sites/default/modules/ad/notify/ad_notify.module on line 311.
    * recoverable fatal error: Object of class stdClass could not be converted to string in /var/www/ad69/sites/default/modules/ad/notify/ad_notify.module on line 314.
    * recoverable fatal error: Object of class stdClass could not be converted to string in /var/www/ad69/sites/default/modules/ad/notify/ad_notify.module on line 314.
    * notice: Object of class stdClass could not be converted to int in /var/www/ad69/includes/database.inc on line 211.
    * user warning: Duplicate entry '0--0' for key 2 query: INSERT INTO ad_notify (aid, oid, event, delay, expire, locked, status, address, subject, body, roles) VALUES(1, 0, '', 0, 0, 0, 0, '', '', '', '') in /var/www/ad69/sites/default/modules/ad/notify/ad_notify.module on line 561.
    * recoverable fatal error: Object of class stdClass could not be converted to string in /var/www/ad69/sites/default/modules/ad/notify/ad_notify.module on line 577.

Notification created.
The requested page could not be found. 

If I manually go to the correct URL, a blank notification has been created.

If I try to delete a notification, I am redirected to the global notification page rather than staying on the "My notifications" page for the ad I was working with.

This still seems to be very broken.

I have cleaned up the patch a little, and resolved the above issues. I'm attaching an updated version which I intend to apply after a little more testing.

A cleaned up the install file -- the new field needs to be in its own _update_NNN function, as otherwise anyone tracking CVS won't be prompted to perform the update. I have committed this patch with additional cleanup and modifications, and now things seems to be working well. I'm not entirely confident that previously configured notifications will work, but the issues I was having may have been due to trying to create notifications with the earlier bugs. At this point, everything seems to be working correctly. Thanks!