Port per-user issue notification email functionality to D7

Note, please read the other links issue. We already did a lot of research and basically came to a decision.

Thanks,
-Derek

Righto.

One thing to consider while re-writing this code is if it makes sense to integrate with https://drupal.org/project/queue_mail -- or something like it.

It'd be really great *not* to be actually sending out 10s-100s of emails inside the hooks that are fired when someone posts a comment. It'd be much nicer to just enqueue N requests to fire the email and then have some drush command or whatever that you can run every minute that tries to drain these queues. A tiny bit of latency is worth the page response time when posting new comments.

Normally, I'd consider this scope creep, but I know the infra team would be happy about this, and I don't think it's going to add any extra work if we're already basically reimplementing this stuff in D7 anyway.

Actually, after looking more closely at queue_mail, it's just using hook_mail_alter(), so it's an optional-add-on, anyway. Forget I said anything. ;) We can handle that totally in parallel to this. We still need our code to generate the emails directly, and that'll be the fallback for sites that don't want to use queue_mail.

Based on brief chat with bdragon, we think there's about 3 days of work left to get the actual email sending code working properly and tested.

Excellent, thanks rootatwc!

The question of whether the email is sent via a queue or not is orthogonal to how we know if a given message needs to be sent. I don't think we necessarily want to stick to the specifics of the D6 implementation and the hook_exit() hacks if we don't have to. But please don't make this require a queue for sending emails. I think those are totally separate questions, and I'd prefer to let sites use queue_mail if they want queue-based emailing. In terms of the best way in D7 to do what we're using hook_exit() for in D6, it's been too long since I've thought about that code so I don't remember exactly what's going on in D6 and why. Hopefully there are good code comments to help you make sense of what's happening. I don't have time/energy to swap it all back in right now, and it sounds like you want to do that yourself, so I'll just wait to see what you come up with after reading the code and trying to get this working.

Meanwhile, let me know if you have any other specific questions.

Thanks again,
-Derek

@rootatwc and @greggles: Are either of you going to have time/interest in moving this forward in the next few days? I was going to try to run with this for a while, but a) I don't want to duplicate effort and b) I'd love to encourage other people to be working on Project* code. ;) So, if anyone has a chance to get this further along, I'm available for reviews/commits when needed. If no one has time to work on it, I can pick up where y'all left off. Just let me know.

Thanks!
-Derek

I'm starting to work on this now...

I'm starting to come to grips with the patches in here. My initial impression is: "wow, this almost entirely duplicates what nodechanges module is doing." :/ It'd sure be nice not to reinvent the wheel here, and nodechanges is a required dependency for project_issue now. I bet there's a very simple way to notice in hook_node_insert() that an initial issue node was created and send the emails about that. Otherwise, notice whenever a comment is inserted and just let nodechanges do all the heavy lifting about computing and displaying changes. That'll catch both "raw" comments and node edit comments. We might need to make a few changes to nodechanges to expose some APIs to make this easy (e.g. maybe we'll need #1948108: Create separate function to use as API), but I think that'd be a big win over re-doing it all here...

Just wanted to post my initial findings for feedback. I'll continue working on this in earnest tomorrow.

Thanks,
-Derek

So far, everything in here is about the per-user email notification stuff, although there are a few stale hunks of code in patch #19 related to the per-project mayhem. To keep this issue sane and focused, and since it's not clear what parts of the per-project functionality (if any) we want to port to D7, I split that stuff off to #2000306: Remove per-project email notification functionality for D7.

I've pushed some commits related to #2000306: Remove per-project email notification functionality for D7 and completed a fairly major re-write and cleanup of the patch from #19. For now, I've punted about the actual message body stuff and trying to use nodechanges for that, but I wanted to post this as a work-in-progress that cleans up and fixes many things:

- Harvested and fixed the logic from D6/#19 that figures out what nids to send notifications to and the hook_exit() stuff.
- Majorly cleaned up, clarified and wrote better comments for project_issue_mail_notify().
- Used and slightly tweaked the DBTNG-ification from #19 for the queries to figure out who should get the emails.
- Converted to use user_load_multiple() instead of separate user_load() calls (if we need to load users at all).
- Majorly cleaned up, clarified and wrote better comments for _project_issue_mail().
- Fixed bugs in the way we were handling the 'Component' field in the email subject.
- Removed $node from everything using either $issue or $project as appropriate (for clarity and to avoid bugs).
...

I've done a fair bit of local testing and so far, everything is working as expected in terms of who gets the emails, and what the subjects and message headers look like.

For now the body just includes the links ;)

Issue status update for:
http://localhost/d7project/node/30
Update issue:
http://localhost/d7project/node/30/edit

I think I'd like to commit this for now and then keep going on either switching over all the display logic to use nodechanges or to port the rest of the mail.inc functions and code from #19. This patch is already huge, and the rest of the logic from #19 is even bigger, so I'm trying to keep these commits somewhat manageable.

Upon further reflection and based on an IRC chat with greggles and tvn:

A) The notification email contents need to be really different for the original post and a revision-generated comment. The original post basically just needs an ASCII representation of the whole issue (title, all metadata fields, body, etc). The update emails need:

1. metadata changes
2. current metadata values (optional -- tvn wants this to be a per-user setting)
3. body of the revision comment (if any)
4. full history of the issue (optional -- already a per-user setting)

In D6, 1 and 2 are merged, but in D7 it'll require a lot more work to do that than the above proposal, and it would rule-out the possibility of the setting to include/ignore all the current metadata values that tvn wants.

B) The patch in #29 makes some bad assumptions in terms of updates. We only keep track of the nids that have been updated and need notifications. However, for example, if you programmatically edit the same node multiple times in the same page load, we're not going to send updates for the intermediate revisions, only the final revision at exit. We should really be tracking the comment ids, not just the nids. Given that the email notifications need to be radically different for the original node vs. issue update cases, I think we should just track the two separately and at hook_exit() process both lists.

C) I think a lot of the logic for "render these node changes as ASCII" can just live in nodechanges.module itself, not project_issue. I'd rather the code lived there, so it's re-usable by other people running nodechanges. See, for example, #1948108: Create separate function to use as API. Shouldn't be any more work if I write it that way from the beginning.

Anyway, my next step is going to be fixing #29 for B. Then I'm going to work on A and C...

#2013020: Store the revision ID (vid) with the nodechanges field data - needs upgrade path would make this issue a lot easier in various ways. Any reviews and opinions over there would be much appreciated.

Thanks,
-Derek

Sorry for the long delay in replying. This summer has been nuts for my schedule...

I've got a *ton* of progress here, which I'm about ready to start pushing the commits for. #31.B is done, as is most of #31.A. #31.C is my next (and nearly last) major task.

However, one question about rendering the current metadata values (for #31.A):

For now, I'm just reusing project_issue_metadata_build():

/**                                                                             
 * Generate an ASCII representation of the current issue metadata.              
 *                                                                              
 * @param object $issue                                                         
 *   The fully-loaded node object for the revision of the issue we're           
 *   generating a notification email for.                                       
 *                                                                              
 * @return string                                                               
 *   The current issue metadata values.                                         
 */
function project_issue_email_get_issue_metadata($issue) {
  $cache = &drupal_static(__FUNCTION__);
  if (!isset($cache[$issue->nid][$issue->vid])) {
    $output = '';
    $metadata = project_issue_metadata_build($issue);
    unset($metadata['update_link']);
    unset($metadata['links']);
    foreach (element_children($metadata) as $key) {
      $output .= $metadata[$key]['#title'] . ': ';
      $output .= $metadata[$key][0]['#markup'] . "\n";
    }
    $cache[$issue->nid][$issue->vid] = $output;
  }
  return $cache[$issue->nid][$issue->vid];
}

Generates emails that look like this:

Status: Active
Priority: Major
Category: Feature request
Component: Miscellaneous
Assigned: admin [1]
Project: Signup

This is almost what we want, except that this is controlled by the 'issuemetadata' view mode. That's good and bad. The good news is that the order of the fields is controllable via the UI, as is the formatter to use, etc. This basically just mirrors the sidebar block. The problem is that there are potentially other metadata fields we want to render in the email (e.g. the related issues, parent issue, etc) that aren't configured in that view mode (by design -- they're rendered in separate blocks generated by Views). What's the right solution here?

A) Hard-code support for these other special fields.

B) Ditch the view mode approach and just walk through *all* the fields on the issue node, skip the body, and try to render everything else as the metadata. In that case, what do we do about ordering and formatters?

C) Hybrid: first render everything from the viewmode, then walk through all the other fields and render those afterwards -- the order will be somewhat non-deterministic (it'll be consistent for a given site, but we won't really have control unless we special-case/hard-code, which is option A again).

D) Other?

Note that I'm *not* talking about metadata changes. That's #32.C, and my plan there is to put the ASCII-rendering logic directly in nodechanges.module and use that. The comments themselves already know all the fields that were touched in a given revision, so this isn't a problem for that case. I'm just talking about how to display all the current values for an issue. And yes, I still have a TODO comment in there about turning the rendering of this info into a per-user preference, instead of always including it.

Thoughts?

Thanks!
-Derek

p.s. The D6 version pads the labels so all the values line up vertically. Something like this:

Status:    Active
Priority:  Major
Category:  Feature request
Component: Miscellaneous
Assigned:  admin [1]
Project:   Signup

Do we care?

p.p.s. the "[1]" after "admin" is from core's drupal_html_to_text() since the assigned user is rendered as a link in the view mode. Should I strip tags first so we don't get those links?

Thanks for the feedback!

Re: URL footnotes, I'm specifically talking about the links to user profiles based on rendering the assigned user as a link. D6 doesn't do this. Should D7 start? Any links in the bodies of comments and the issue itself can and should continue to get the URL footnote behavior from drupal_html_to_text().

Re: hook_field_extra_fields() -- the thing is, we do not want those fields to be rendered inside the main "issue metadata" block. They're in separate blocks for a reason. So, that's not really going to help us here. I think fundamentally we face the problem that we want different things in different places for web vs. email...

Cheers,
-Derek

Re: Two view modes: On the level of theory, that makes sense -- that's what Field API expects/encourages and that does give a lot of flexibility. However, in practice, that's sort of a huge pain in the ass. ;) You can't define view modes only for certain bundles -- it's already too bad that *every* node type on a system running Project* has this view mode that only matters on 1 (or maybe a few) node types. If we added another, it's more complication in the UI for site builders, it's a lot more code in project_issue itself to setup and use it, and a ton more code in drupalorg_project for customizing all our field instances. I'm not sure that makes sense. But I'm definitely open to discuss it further. Meanwhile, I'll keep plugging away at rendering the metadata changes via nodechanges.module, since I think it's pretty clear how that should work. But, my hope is to be totally done with this issue by the end of today (Friday 8/16) so hopefully we can discuss this quickly. ;)

Re: Assigned links: on an issue that involves a lot of changes of who's assigned, those links are going to generate a ton of URL footnote noise. Another case where what makes sense in the web case probably doesn't make sense in the email case. Instead of the complication of 2 view modes, an approach to this part of the problem is just to always strip tags from the values of the metadata fields when rendering the notification email. It's an 12 byte fix to the problem, instead of potentially 100s of lines of code sprinkled across various projects...

Thanks again for the input...
-Derek

Eureka! I just had a stroke of insight about this. There's *already* another view mode -- the one provided by and used by nodechanges. ;) Yay! And that one *does* want to include *all* the fields. So, I think that settles it. Yay.

Based on some more IRC discussions, tvn and I agree that the user setting for #31.A.2 is not a launch blocker, so that now lives at #2067449: Add a per-user preference to toggle if issue emails should include current metadata values or not

Meanwhile, tvn votes for stripping tags from metadata values before display and for padding labels. Both should be relatively trivial, so I'll fix those while I'm re-writing project_issue_email_get_issue_metadata() to use the nodechanges view mode. Stay tuned... ;)

How's this?

Status:         Active
Priority:       Major
Category:       Feature request
Component:      Miscellaneous
Assigned:       admin [1]
Project:        Signup
Related issues:
    #20: Nobis Pala [2]
    #23: Luctus [3]

...

[1] http://localhost/d7project/user/1
[2] http://localhost/d7project/node/20
[3] http://localhost/d7project/node/23

Of course, we *do* want the links for the related issues. :/ So, we don't really want to always strip links from metadata values. Instead of introducing a 3rd view mode just for this kind of thing, nor trying to special case this, for now, I'll just leave links in there. If people are upset, we can always revisit this.

Here's the code:

function project_issue_email_get_issue_metadata($issue) {
  $cache = &drupal_static(__FUNCTION__);
  if (!isset($cache[$issue->nid][$issue->vid])) {
    $output = '';
    $metadata = node_view($issue, 'nodechanges');
    // Ignore a few things from the view mode we don't want in email.           
    unset($metadata['update_link']);
    unset($metadata['links']);
    // Also, we handle the body separately, so ignore that, too.                
    unset($metadata['body']);

    // This is slightly a hack, but first we walk through all the fields and    
    // find the length of the longest label so we can pad everything            
    // accordingly to let the values line up vertically.                        
    $max_label_length = 0;
    foreach (element_children($metadata) as $key) {
      $label_length = strlen($metadata[$key]['#title']);
      if ($label_length > $max_label_length) {
        $max_label_length = $label_length;
      }
    }
    // Instead of just calling drupal_render(), which gives us a ton of divs    
    // and other junk we don't want, and might ignore field labels (based on    
    // the site configuration), we just manually render this in ASCII and       
    // always include the label.                                                
    foreach (element_children($metadata) as $key) {
      $output .= str_pad($metadata[$key]['#title'] . ':', $max_label_length+2);
      // We render multi-valued fields with a label on a separate line and      
      // indent each value on its own line.                                     
      if (count($metadata[$key]['#items']) > 1) {
        $output .= "\n";
        foreach (element_children($metadata[$key]) as $subkey) {
          $output .= '  ' . $metadata[$key][$subkey]['#markup'];
        }
      }
      // Otherwise, just print the value inline with the label.                 
      else {
        $output .= $metadata[$key][0]['#markup'] . "\n";
      }
    }
    $cache[$issue->nid][$issue->vid] = $output;
  }
  return $cache[$issue->nid][$issue->vid];
}

Any final concerns/objections with this piece?

After an IRC chat with tvn about trying to merge the metadata changes and the current values (like we have in D6), I decided to bail on that for 3 reasons:

1) When rendering the issue comment history, we want to see just the changes, not the changes merged with the current values.
2) It would further complicate #2067449: Add a per-user preference to toggle if issue emails should include current metadata values or not
3) It was turning into a giant pain in the ass from a code stand-point. :/

So, I left it as a separate chunk of the email, generated by a separate function. To make this easier, I did a minor refactoring of how nodechanges internally represents the changes to multi-valued fields for display (not how they're stored in the nodechanges field itself, just the intermediary data structure passed to the theme function). See #2067623: Refactor how we represent multi-valued fields to be easier to reuse for more on that. Then, as I originally proposed, I put the logic to render the changes as plain text into another theme function inside nodechanges itself, so other people could re-use it if desired. See #2067643: Add a theme function for rendering the changes in a comment as plain text for more on that. ;)

There are a few wonky things about the formatting (I think drupal_html_to_text() is to blame), but it's getting there. Here's an example:

#42 -- Fri, 08/16/2013 - 16:44 : admin
http://localhost/d7project/node/62#comment-78

Issue changes:
- Category: Task
+ Category: Feature request
- Priority: Major
+ Priority: Critical
- Assigned: admin [14]
+ Assigned: Unassigned
- Status:   Needs review
+ Status:   Needs work
   Related issues:
   - #25: Eu Meus Populus [15]
   + #26: Aptent Ludus Tincidunt [16]
   + #27: Dolore [17]

The only other major piece of the notification emails that aren't yet working properly is #2067649: Fix handling of file attachments in issue notification emails so I split that off into a separate issue.

There are a few other TODO comments for #2067651: Fix edge cases regarding comment IDs in issue notification email code but those are minor.

I also opened #2067659: Send HTML email notification for more compact presentation as another potential follow-up.

Given that everything's pretty close, and I think I'm done rebasing and cleaning the commits and history, I finally decided to push all my work here:

http://drupalcode.org/project/project_issue.git/commit/9d6b333
http://drupalcode.org/project/project_issue.git/commit/062a941
http://drupalcode.org/project/project_issue.git/commit/8c2318e
http://drupalcode.org/project/project_issue.git/commit/694251c
http://drupalcode.org/project/project_issue.git/commit/39c7dc3
http://drupalcode.org/project/project_issue.git/commit/f10ab4e
http://drupalcode.org/project/project_issue.git/commit/d3a47d4
http://drupalcode.org/project/project_issue.git/commit/0d8db5d
http://drupalcode.org/project/project_issue.git/commit/3a722ac
http://drupalcode.org/project/project_issue.git/commit/cf0ef3b

yowzah! ;)

Something tells me no one is actually going to review any of that, but I wanted to call this "needs review" both in case anyone will review the code, and to get any final feedback on the other stuff I wrote in this comment.

Cheers,
-Derek