It would be great if OG added mailman style List-Id headers to the notifications it sends via email. This would allow users to filter emails (GDO emails in particular, hence the patch for 6.x-2.x) using their MUAs normal list header filters.

The attached patch can create a list-id type representation of an organic group and adds this reprsentation in the List-Id header on outgoing emails.

For example, http://groups.drupal.org/node/217939 would become: node.217939.groups.drupal.org and http://groups.drupal.org/australia would come australia.groups.drupal.org.

The patch also provides this generated string via a token.

CommentFileSizeAuthor
#19 1490506-list-id-19.patch4.13 KBcafuego
#11 1490506-list-id-11.patch4.05 KBcafuego
#7 1490506-list-id-7.patch4.51 KBcafuego
#5 1490506-list-id-2.patch4.33 KBcafuego
add-list-id.patch2.95 KBcafuego

Comments

cafuego’s picture

cafuego commented
Issue summary: View changes

Updated issue summary.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Issue tags: +gdolove, +commonslove

Tagging. This is a common request I'd love to see get deployed.

There's a chance we'll get a review for this related to gdo or Commons.

Grayside’s picture

Grayside commented

Nothing against it, though I'm surprised this should be handled by OG and not more on the Notifications/Messaging side. This awaits review by someone more involved with Notifications and/or mail systems.

cafuego’s picture

cafuego commented

I don't know that it would make sense to add mail headers outside of the scope of OG, unless you're suggesting an API call in Notifications/Messaging that OG (and other modules) would call to set specific headers.

Grayside’s picture

Grayside commented

Yeah, that's all I was especially imagining. Like I said, largely waiting for others to RTBC this issue.

cafuego’s picture

cafuego commented
StatusFileSize
new 1490506-list-id-2.patch4.33 KB

Updated patch. The actual group identifier is now generated in a helper and the list-id header is constructed in a wrapper function. I could be convinced that the identifier construction should in fact be a theme function, so it can be overridden.

og_notifications now checks if a List-Id header is already present and appends its own string if so.

realityloop’s picture

realityloop commented
Status: Needs review » Needs work

This looks good to me apart from one small thing to meet the coding-standards:

Add newline at end of file http://drupal.org/coding-standards#indenting

+++ b/modules/og_notifications/og_notifications.module
@@ -577,4 +590,4 @@ function og_notifications_autosubscribe_map() {
-}
\ No newline at end of file

can't wait to see this on g.d.o

cafuego’s picture

cafuego commented
Status: Needs work » Needs review
StatusFileSize
new 1490506-list-id-7.patch4.51 KB

Added newline (told vim to not remove it, anyway ;-) and changed the _og_listid() helper into a theme function, so users can override it.

Grayside’s picture

Grayside commented
Status: Needs review » Needs work
  1. Why is og_listid not handled in OG Notifications?
  2. If not handled in OG Notifications, I'd rather some better prefixing so the context is more clear--e.g., og_mail_listid()
  3. While a theme function is a good way to allow overriding, it is not really a clean practice to use it for purpose of allowing data alternation. What kind of customization of the ID do you imagine happening?
cafuego’s picture

cafuego commented

1. Why is og_listid not handled in OG Notifications?

In this case, because I've also provided a token, which could be used outside that context. Not that it makes a whole lot of sense, but og already had a hook_token implementation, so it was easier to add this there :-)

2. If not handled in OG Notifications, I'd rather some better prefixing so the context is more clear--e.g., og_mail_listid()

Sure, I'm not fussed either way, as per reason above.

3. What kind of customization of the ID do you imagine happening?

I could see someone not wanting to have per-group custom List-Ids or have them named differently, perhaps with a different hostname part than is returned url(). having a theme function the user can override seems an easier alternative than providing an admin UI for a tokenised string to be used instead. Not allowing it to be customised seems ... mean ;-)

Grayside’s picture

Grayside commented

Hi Cafuego,

Let me start by saying I really appreciate your effort here. I totally understand the complexities of wrangling email, and if a little header makes peoples lives easier, I am all for it. That said...

  1. Creating a new hook_token implementation is not hard. Since listid is not relevant outside the context of og_notifications, it should not appear at all in og.module unless there is an architectural need. This improves maintainability, developer experience, has minor performance gains, and so on.
  2. This is a misuse of the theme() system to use it as an alter hook mechanism. I don't think OG needs to support this as a customizable element and hook_mail_alter() exists if someone wants to tweak it further.

If it really needs OG-friendly customizability, I suggest using a token-enhanced textfield to the OG Notifications UI, with an extra post-processing stage to wrangle spaces and other undesireable characters.

cafuego’s picture

cafuego commented
Status: Needs work » Needs review
StatusFileSize
new 1490506-list-id-11.patch4.05 KB

Right then, updated patch. The changes are now all only in og_notifications.module, which does now implement token hooks. The actual list-id value is no longer generated via a theme function.

alexweber’s picture

alexweber commented
Status: Needs review » Reviewed & tested by the community

This looks and works great! Can we get this comitted and also forward-ported to 7.x?

cafuego’s picture

cafuego commented

7.x no longer uses messaging to send notifications, instead it uses Rules. I've been flighting it today to see what would be needed to get it to insert the same header. I have as yet been unable to make OG 7.x send an email of any sort, so I don't know what/if/how to add a header yet.

alexweber’s picture

alexweber commented

@cafuego, no luck with hook_mail_alter() ?

cafuego’s picture

cafuego commented

Maybe, not until I can get it to send mail :-)

alexweber’s picture

alexweber commented

hehe good catch :) what's the issue, the rules integration? I've been doing a lot of stuff with rules lately and wouldn't mind taking a stab at this when I get some time!

cafuego’s picture

cafuego commented

When I did the OG 7.x-1.x install to play with, I didn't have Rules enabled. When I did later enabled Rules, the OG default rules weren't detected, so I decided to remove OG and start again. That made be run into a bunch errors caused by Entity (need to make a patch for those too now).

Anywho, I appear to be unable to create a rule that sends an email when a *comment* is posted on an OG node. I *think* I have the correct variables etc set up, the rule is no longer complaining about incorrect/invalid variables, but no email is sent (postfix is set up and working fine on this box). Exported below, see how you go :-)

{ "rules_og_group_comment_notification" : {
    "LABEL" : "OG group comment notification",
    "PLUGIN" : "reaction rule",
    "REQUIRES" : [ "og", "rules", "comment" ],
    "ON" : [ "comment_insert" ],
    "IF" : [ { "og_entity_is_group_content" : { "entity" : [ "comment:node" ] } } ],
    "DO" : [
      { "og_get_members" : {
          "USING" : { "group_content" : [ "comment:node:group" ] },
          "PROVIDE" : { "group_members" : { "group_members" : "List of group members" } }
        }
      },
      { "LOOP" : {
          "USING" : { "list" : [ "group-members" ] },
          "ITEM" : { "list_item" : "Current list item" },
          "DO" : [
            { "mail" : {
                "to" : [ "list-item:mail" ],
                "subject" : "New post in group",
                "message" : "Hello [list-item:name],\r\n\r\nThere is a new post called [comment:subject] that belongs to one of the groups you are subscribed to.\r\n\r\n[site:name]\r\n",
                "from" : [ "" ],
                "language" : [ "" ]
              }
            }
          ]
        }
      }
    ]
  }
}
Grayside’s picture

Grayside commented
Status: Reviewed & tested by the community » Needs work

This is looking good. A few nitpicks.

+++ b/modules/og_notifications/og_notifications.module
@@ -569,6 +582,33 @@ function _og_notification_check_message($message) {
+/**
+ * Implements hook_token_list() for OG Notifications specific tokens.
...
+/**
+ * Implements hook_token_values() for OG Notifications specific tokens.

Can stop with the hook signature. Of course it's for OG Notifications :)

+++ b/modules/og_notifications/og_notifications.module
@@ -569,6 +582,33 @@ function _og_notification_check_message($message) {
+    // Set a default

Is there a particular reason this default is significant? If not, remove comment. If so, add detail and a period.

+++ b/modules/og_notifications/og_notifications.module
@@ -577,4 +617,62 @@ function og_notifications_autosubscribe_map() {
+ * Create list-id id identifier for one or more group nodes.

Capitalization inconsistency for List-ID

+++ b/modules/og_notifications/og_notifications.module
@@ -577,4 +617,62 @@ function og_notifications_autosubscribe_map() {
+ * @return String
...
+ * @return String
...
+function _og_notifications_listid($gid) {

Space above @return. Lower case "string".

+++ b/modules/og_notifications/og_notifications.module
@@ -577,4 +617,62 @@ function og_notifications_autosubscribe_map() {
+  $parts = parse_url(url('node/'. $gid, array('absolute' => TRUE)));
...
+  return strtr($group, array('/' => '.')) .'.'. $parts['host'];

Space before and after a period.

cafuego’s picture

cafuego commented
Status: Needs work » Needs review
StatusFileSize
new 1490506-list-id-19.patch4.13 KB

Updated patch with all issues above (and some besides) addressed.

Fixed up other period concatenation spacing, ensured that hook_token_list always returns an array and added a link to the List-Id RFC document in the doc block of the function that generates it.

Grayside’s picture

Grayside commented
Status: Needs review » Needs work

Awesome! Thank you so much for bearing with me on these nitpicks. Two final issues and then I think I'll be ready to commit.

+++ b/modules/og_notifications/og_notifications.module
@@ -569,6 +582,35 @@ function _og_notification_check_message($message) {
+/**
+ * Implements hook_token_list().

Please use D6 coding standards. "Implementation of".

+++ b/modules/og_notifications/og_notifications.module
@@ -569,6 +582,35 @@ function _og_notification_check_message($message) {
+    $tokens['node']['og-notifications-listid'] = t('List-Id style representation of the top group');

What does "top group" refer to?

Grayside’s picture

Grayside commented
Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
Status: Needs work » Closed (outdated)
Issue tags: -

This version of Drupal is not supported anymore. If this is still an issue in the 8.x-1.x branch, please open a new up-to-date ticket. Closing.