Mobile phone friendly emails

Hm. My Drupal work heavily (totally) depends on the current issue notification mails. And I have to disagree with the major points 1. + 2., so I'd kindly ask this to become an optional "Send issue notifications as digest" checkbox in the issue subscription settings or your user profile, or something.

Don't really want to get into details, because all of this really depends on how you are using and working with these mails (if you do at all), but anyway, here's my point of view:

1) Dropping the component might be acceptable, but the project is key to be able to work with these notifications, without having to setup giant incoming mail filter rules. I don't have any filter rules, just one giant drop box for all issue notifications, that's it.

2) I'm very very often reading cross-references or second to last replies in the notification mails, especially when follow-ups refer to comment #123 and because the mails luckily contain those counters as well. That's a huge speed factor for me, as I don't necessarily have to visit an issue. I can quickly read more insight, compare to existing, delete the mail and think about the issue + insights later on.

3) I don't really care for. If I'm clicking a link, then it's only the very first link to the issue. More than once I wished there were links to user profiles of comment authors. Don't think that I ever clicked on one of the jump links for each comment.

+++ project_issue.install	1 Sep 2010 15:41:21 -0000
@@ -382,6 +382,27 @@ function project_issue_schema() {
+  ¶
+  $schema['project_issue_users'] = array(
+    'description' => 'User preferences for issues.',

Shouldn't 'users' be singular?

+++ project_issue.install	1 Sep 2010 15:41:21 -0000
@@ -382,6 +382,27 @@ function project_issue_schema() {
+    'primary key' => array('uid'),
+  );

We should also add foreign key info here.

+++ project_issue.module	1 Sep 2010 15:41:22 -0000
@@ -1839,3 +1844,60 @@ function project_issue_project_page_link
+function project_issue_user($type, &$edit, &$account, $category = NULL) {
...
+  elseif ($type == 'load') {
+    $sql = 'SELECT mail_length FROM {project_issue_users} WHERE uid = %d';
+    $row = db_fetch_object(db_query($sql, $account->uid));
+    $account->project_issue_mail_length = isset($row->mail_length) ? $row->mail_length : NULL;  ¶
+  }

Do we permanently need this info? At the very least, we can skip the query for anonymous users.

+++ project_issue.module	1 Sep 2010 15:41:22 -0000
@@ -1839,3 +1844,60 @@ function project_issue_project_page_link
+      '#title' => t('Email notification length'),
+      '#options' => array(
+        PROJECT_ISSUE_MAIL_LENGTH_VERBOSE => t('Complete'),
+        PROJECT_ISSUE_MAIL_LENGTH_BRIEF => t('Brief'),
+      ),
...
+      '#description' => t('Brief email notifications are usually preferred when reading email on a mobile device. Complete emails are better if one wants the whole issue history within a single email.'),

I think we can rename the title to "E-mail notification format" and replace both option labels with the corresponding part of the description; dropping the description entirely.

+++ project_issue.module	1 Sep 2010 15:41:22 -0000
@@ -1839,3 +1844,60 @@ function project_issue_project_page_link
+    var_dump(isset($edit['project_issue_mail_length']));die(sd);

? :)

+++ includes/mail.inc	1 Sep 2010 15:41:22 -0000
@@ -369,13 +373,22 @@ function _project_issue_mail($key, &$mes
+        $message['body'][] = "$links\n$body";
...
+        // Show links last since recipient is less likely to follow-up using mobile device.
+        $message['body'][] = "$body\n$links";

I think we should put $link and $body into separate 'body' array keys.

+++ includes/mail.inc	1 Sep 2010 15:41:22 -0000
@@ -687,4 +697,4 @@ function project_issue_mail_format_entry
+}
\ No newline at end of file

There are plenty of coding style issues in this patch - didn't comment on these, since they are pretty obvious.

Powered by Dreditor.

Sorry, been too long since this was rolled...

% patch -p0 < mobile.patch 
patching file project_issue.install
Hunk #1 succeeded at 381 (offset -1 lines).
Hunk #2 succeeded at 647 (offset -1 lines).
patching file project_issue.module
Hunk #1 succeeded at 9 (offset -1 lines).
Hunk #2 FAILED at 1843.
1 out of 2 hunks FAILED -- saving rejects to file project_issue.module.rej
patching file includes/mail.inc
Hunk #1 succeeded at 289 (offset -1 lines).
Hunk #2 succeeded at 298 (offset -1 lines).
Hunk #3 succeeded at 308 (offset -1 lines).
Hunk #4 succeeded at 372 (offset -1 lines).
Hunk #5 succeeded at 431 (offset -1 lines).
Hunk #6 succeeded at 494 (offset -1 lines).

Excellent, thanks! Looking pretty good. Haven't tested yet, but just from looking at the code, here's what I see so far:

A) This isn't going to fly:

    // TODO: In Drupal 7, this is a merge query.
    if (db_result(db_query('SELECT COUNT(*) FROM {project_issue_users} WHERE uid=%d', $account->uid))) {
      $sql = 'UPDATE {project_issue_users} SET mail_length = %d WHERE uid = %d';
      db_query($sql, $edit['project_issue_mail_length'], $account->uid);
    }
    else {
      $sql = 'INSERT INTO {project_issue_users} (uid, mail_length) VALUES (%d, %d)';
      db_query($sql, $account->uid, $edit['project_issue_mail_length']);
    }

A much better approach can be found, for example, in project_issue_project_maintainer_save():

function project_issue_project_maintainer_save($nid, $uid, $permissions = array()) {
  db_query("UPDATE {project_issue_project_maintainer} SET maintain_issues = %d WHERE nid = %d AND uid = %d", !empty($permissions['maintain issues']), $nid, $uid);
  if (!db_affected_rows()) {
    db_query("INSERT INTO {project_issue_project_maintainer} (nid, uid, maintain_issues) VALUES (%d, %d, %d)", $nid, $uid, !empty($permissions['maintain issues']));
  }
}

B) Yeah, in project_issue.install, this description is close, but should start with "Foreign key:"

+        'description' => 'The {users}.uid for this subscriber.',

C) I don't entirely understand in project_issue_user() when loading why you set $user->project_issue_mail_length to NULL when not defined instead of the default. Can you explain? Seems like a bug, especially given this:

+      if ($params['recipient']->project_issue_mail_length == PROJECT_ISSUE_MAIL_LENGTH_VERBOSE) {

D) Whitespace bugs in _project_issue_mail()

E) I believe if you use /// here instead of just // then it's recognized by PHPDoc:

+// Long email notification emails containing all follow-ups

F) [style preference, not a bug]:

+    $sql = 'DELETE FROM {project_issue_users} WHERE uid = %d';
+    db_query($sql, $account->uid);

If it's really big and crazy complicated, splitting these can make sense, but for trivial queries like this, I don't think this buys us any readability (and is a tiny performance cost) instead of just doing this:

+    db_query("DELETE FROM {project_issue_users} WHERE uid = %d", $account->uid);

Also, since we use ' to enclose strings inside the queries, the standard is to always use " to quote the string for the query itself so you never accidentally break something...

I'll try to test now, more news if I find anything else. ;)

Thanks again!
-Derek

Upon further thought, and looking at this more closely wearing my d.o UX hat, this isn't going to work in its current form. But it's a great start. I've already mostly fixed it. But, I'm going to finish this up over at #15380-33: Allow to configure content of issue notification e-mails, since that's really exactly what we're talking about here. See also #199178: Usability: Re-order contents of project issue emails for the other part of this.

@Moshe: I know this has been bothering you for at least 3 years, and you've been very patient and helpful on this. So, I'm hereby promising I'm going to finish this up (I'm not asking you to re-roll) tomorrow at the sprint and deploy it. I've already got good feedback from yoroy about the UX stuff. Fear not, this is going to happen! :)

Thanks,
-Derek