Hi

I've been trying to add a feature to suppress notifications when updating nodes using VBO - in the same style as the content-admin page.

I've been able to add the logic, re-using the existing code, but it fails because VBO uses Drupal Actions, whereas content-admin calls update functions directly. I don't see any way around this.

I don't entirely understand how Actions work, but it seems that the actions run in a separate process, making it almost impossible to pass an additional flag from the UI into the actions.

Can anyone see a way of doing this?

Thanks

CommentFileSizeAuthor
#6 interdiff-4-6.txt2.33 KBsalvis
#6 subscriptions-suppress_in_vbo-2863216-6.patch3.66 KBsalvis
#4 subscriptions-suppress_in_vbo-2863216-4.patch3.22 KBRickJ
#2 subscriptions-suppress_in_vbo-2863216-2.patch3.58 KBRickJ
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Rick J created an issue. See original summary.

RickJ’s picture

RickJ CreditAttribution: RickJ commented
Assigned: Unassigned » RickJ
Status: Active » Needs review
FileSize
subscriptions-suppress_in_vbo-2863216-2.patch3.58 KB

I've figured this out, the key is to use a session variable. It's important to ensure the variable in only ever set transiently, because if left set then it could block subscriptions being sent for normal updates. I've attached a patch which is working for me.

The UI copies the check-box added to the admin people page, and defaults to not send subscriptions (which will usually be what is wanted for bulk operations). The check-box also appears on the configuration and confirmation pages. The flag has to be passed through these pages anyway - I could have hidden the box but this way you get a chance to change your mind on subscriptions before the final submit.

This is proving a life-saver for me, I need to do a lot of bulk changes (all status stuff, not content) but I couldn't risk spamming users with spurious notifications.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Status: Needs review » Needs work

Thank you, this is generally useful and not trivial, i.e. worthy to go in.

However, we should introduce the session variable only if we actually need it. It should be either $_SESSION['subscriptions']['suppress'] = 1; or untouched, i.e. empty($_SESSION['subscriptions']['suppress']), i.e. unset($_SESSION['subscriptions']['suppress']);.

  1. +++ b/subscriptions_content.module
@@ -550,6 +550,45 @@
+  // clear the session flag on form display to avoid it being accidentally left set

    Drupal coding conventions: Comments are full sentences: start with Capital and end with period. Also warp at 80.

    (I may have more comments later.) 

  2. +++ b/subscriptions_content.module
@@ -616,10 +655,11 @@
+    subscriptions_content_suppress_notifications_session(TRUE);

    Here you set it to TRUE whether VBO is there or not, but you don't set it to FALSE if the VBO form is not displayed! 

  3. +++ b/subscriptions_content.module
@@ -642,6 +682,24 @@
+ * Implements a version of the above flag as a session variable

    Comments must end with a period. Make it
    * Provides a session flag to suppress notifications. 

  4. +++ b/subscriptions_content.module
@@ -642,6 +682,24 @@
+ * ¶

    Trailing space. 

  5. +++ b/subscriptions_content.module
@@ -642,6 +682,24 @@
+ * @param $set NULL (default) - return state, TRUE - set state from above, FALSE - clear state

    Should be the same as in the function above. 

  6. +++ b/subscriptions_content.module
@@ -642,6 +682,24 @@
+  if (!isset($_SESSION['subscriptions'])) {
+    $_SESSION['subscriptions'] = array();
+  }

    Why? 

  7. +++ b/subscriptions_content.module
@@ -642,6 +682,24 @@
+  return isset($_SESSION['subscriptions']['suppress'])? $_SESSION['subscriptions']['suppress'] : NULL;

    This should be
    return !empty(...);

RickJ’s picture

RickJ CreditAttribution: RickJ commented
Status: Needs work » Needs review
FileSize
subscriptions-suppress_in_vbo-2863216-4.patch3.22 KB

Thanks for the comments. In response:

However, we should introduce the session variable only if we actually need it.
It should be either $_SESSION['subscriptions']['suppress'] = 1; or untouched, i.e.
empty($_SESSION['subscriptions']['suppress']), i.e.
unset($_SESSION['subscriptions']['suppress']);.

That's actually what the code does. See subscriptions_content_suppress_notifications_session().

  if (!isset($_SESSION['subscriptions'])) {
    $_SESSION['subscriptions'] = array();
  }

Why?

Because AFAIK you can't implicitly create multiple levels of array in a single []-style statement. And if the 'subscriptions' array didn't exist the last line in the function would throw an error. The array might already exist, having been created by other code in the module (e.g. the other VBO patch).

  return isset($_SESSION['subscriptions']['suppress'])?
$_SESSION['subscriptions']['suppress'] : NULL;

This should be
return !empty(...);

Agreed, that's more succinct.

On reflection, re-using the existing submit function for the admin-content page, and using both static and session flags at the same time, was not a good idea and left a hole in the logic. I've therefore changed it to use a separate function, and now only the session flag is used for VBO. The logic otherwise corresponds to the static flag, except that the session flag has to be explicitly cleared, which happens unconditionally in the VBO-form-alter hook. The VBO form is always the next page following the submit action, so the session flag should never be left set inadvertently.

I've tidied the comments, revised patch attached.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Status: Needs review » Needs work

Setting multiple indices is not a problem (as long as $_SESSION['subscriptions'] is not something other than an array), and empty() is more robust than you think.

That's actually what the code does.

No, you're always creating the empty array (if it doesn't exist), and you're not cleaning it up. To do what we want requires something like

/**
 * Provides a session flag to suppress notifications.
 *
 * @param bool $set
 *   (optional) Set to TRUE or FALSE.
 *
 * @return bool
 */
function subscriptions_content_suppress_notifications_session($set = NULL) {
  if (isset($set)) {
    if ($set) {
      $_SESSION['subscriptions']['suppress'] = 1;
    }
    elseif (subscriptions_content_suppress_notifications_session()) {
      unset($_SESSION['subscriptions']['suppress']);
      if (empty($_SESSION['subscriptions'])) {
        unset($_SESSION['subscriptions']);
      }
    }
  }
  return !empty($_SESSION['subscriptions']['suppress']);
}

I believe we should keep the 'When publishing unpublished nodes, this should probably be turned ON.' note from the standard bulk implementation. If you fail to do this, you have a problem.

There's still a bug in the logic. If I publish a node via ChangeValue|Status (with Send turned on), notifications aren't queued.

Some more minor Coding Standards stuff:

  1. +++ b/subscriptions_content.module
@@ -550,6 +550,44 @@
+ * Adds a submit handler to catch VBO forms and suppress notifications by default.

    Wrap at 80. 

  2. +++ b/subscriptions_content.module
@@ -550,6 +550,44 @@
+  // Clear the session flag on form display to avoid it being accidentally left set.

    Wrap at 80. 

  3. +++ b/subscriptions_content.module
@@ -642,6 +692,27 @@
+ * Implements a version of the above flag as a session variable.

    There is not concept of "above" in API documentation. Each docblock must stand on its own. 

  4. +++ b/subscriptions_content.module
@@ -642,6 +692,27 @@
+ * ¶

    Still a trailing space here.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Assigned: RickJ » Unassigned
Status: Needs work » Needs review
FileSize
subscriptions-suppress_in_vbo-2863216-6.patch3.66 KB
interdiff-4-6.txt2.33 KB

I'm sorry, I've lost track of this.

Here's a minor increment. However, I think that

There's still a bug in the logic. If I publish a node via ChangeValue|Status (with Send turned on), notifications aren't queued.

is still an open issue.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Status: Needs review » Needs work