When subscribing to notifications about a particular content-type (and per-type subscriptions settings are NOT enabled), these notifications are not being sent. This seems to be because notifications_content_type_enabled() - which checks if a particular type is enabled for notifications, includes the following code:

/**
 * Get subscription options for this content type
 * - All enabled options if ($option = NULL)
 * - TRUE / FALSE for a given $option
 */
function notifications_content_type_enabled($type = NULL, $option = NULL) {
  $defaults = variable_get('notifications_content_type', array());
  if ($type && variable_get('notifications_content_per_type', 0)) {
    $settings = variable_get('notifications_content_type_' . $type, $defaults);
  } else {
    $settings = $defaults;
  }
  if ($option) {
    return in_array($option, $settings, TRUE);
  }
  else {
    return $settings;
  }
}

The issue is in the first line: the name of the variable that contains the allowed content types appears to actually be called 'notifications_content_types' - with a trailing S?? This code was changed a couple of weeks ago: CVS diff.

See attached content-notifications form (my types happen to be OG?) and the variable-editor.

CommentFileSizeAuthor
Variable editor.jpg74.75 KBlyricnz
Notifications Settings.jpg135.23 KBlyricnz

Comments

jose reyero’s picture

jose reyero commented
Status: Active » Postponed (maintainer needs more info)

Well, the whole settings have been fully reworked. But do you mean we are still using the old name somewhere in the module?

lyricnz’s picture

lyricnz commented

When I retest with a clean database, module works fine. I suspect original problem was an upgrade issue - some settings being left over from the older version of the module, and not updated cleanly.

markus_petrux’s picture

markus_petrux commented
Status: Postponed (maintainer needs more info) » Needs review

I also found a problem related to this. I think the issue is that notifications_content_type_enabled($type) is used to perform certain checks, but this function will return TRUE even when no options are enabled, which may happen if per-content type options are or were enabled and all options have been disabled per content type.

As an example, the following code:

var_dump(notifications_content_type_enabled('mytype'));

If all options per content type were disabled, it will return something like the following:

array(5) (
  'grouptype' => 0,
  'thread' => 0,
  'nodetype' => 0,
  'author' => 0,
  'typeauthor' => 0,
);

Since this is not an empty array, notifications_content_type_enabled('mytype') evaluates to TRUE, which is wrong!

I think this could be fixed as follows:


 function notifications_content_type_enabled($type = NULL, $option = NULL) {
   $defaults = variable_get('notifications_content_type', array());
   if ($type && variable_get('notifications_content_per_type', 0)) {
     $settings = variable_get('notifications_content_type_' . $type, $defaults);
   } else {
     $settings = $defaults;
   }
+
+  // Get rid of not enabled options.
+  $settings = array_filter($settings);
+
   if ($option) {
-    return in_array($option, $settings, TRUE);
+    return !empty($settings[$option]);
   }
   else {
     return $settings;
   }
 }
jose reyero’s picture

jose reyero commented
Status: Needs review » Fixed

Yes, the function was buggy when checking notifications_content_type_enabled('type') for TRUE, FALSE.

About #3, the values are actually the array values, not the keys (in_array). There's something weird with how the settings are stored: keys are content type strings for global options but numeric for per content type options, I may have done sth wrong with the forms...?

Anyway, fixed it (I hope) adding an array_filter just for the second case, when there's no $option. This should work.

Thanks for feedback and ideas.

Status: Fixed » Closed (fixed)

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