Private message drafts

Fayna,

You've done some useful work for privatemsg module so far and you have some good ideas.

Perhaps you would like to help by maintaining a list of feature requests?

I'm going to put the feature list that you've assembled on the project homepage but i will also put a list of pending feature request there too.

wanna make a list of all current feature requests ? ^_^

I may need to write this for another project. If I do, I'll get in touch and get advice/spec from litwol so I can write this for you the way you intend to implement it. =)

Just throwing this out, but the way I see it, it would be as a submodule and store things in its own table, which is more or less identical to pm_message.

Save as draft should not add the necessary data to pm_index.

When "sending" the message, remove from pm_drafts, add to pm_message and add the relevant rows into pm_index (The last two steps being identical to what a normal send operation is like.).

So you're suggesting a CCK-esque approach, where there is a "contrib" directory within Privatemsg, for contributed, optional modules? It works for them. It's not a bad idea, as long as it's applied sensibly... =)

"We" are already doing this, for example with user blocking and tagging/filtering. It doesn't really matter (technically, for drupal) where those modules are installed, but a subdir like modules or contrib might be a good idea.

I haven't used Privatemsg for about a year, so I'm sure it looks vastly different these days! Actually looking forward to seeing the latest version - I hope to install it and have a play this weekend. =)

Definitely a good idea to have a directory with the sub-modules in. "contrib" seems to be the convention (see CCK, Image, Views, etc.)

But we digress!

I'm working on writing this functionality for client project right now. I tend to agree with nbz and think this should be written as a submodule that stores "drafts" in its own table. When drafts are actually sent, the draft would then be added to pm_index and pm_message as with any other message. Once sent, a draft would be deleted from the pm_drafts table. If anyone has thoughts on this approach, I'd love to hear them before I really put pen to paper.

Sounds ok. I imagine it will work like this:

- Add "Save as draft" to pm form
- That stores the subject, recipients (as a string I guess), text, text format and the user in a pm_draft table. (What about reply drafts? Probably needs to store the thread_id for them)
- There should be a list of saved drafts somewhere, likely as a local task beneath inbox/sent which shows a preview. + "Send now" (necessary?) and a "Edit" link or something like that? The first will send it directly through the API (?), the other will forward to the send form and pre-fill it (maybe by passing the draft id as an argument to that page and then using hook_form_alter) => remember to check the user id to make sure he is allowed to see that draft!
- If the saved draft is a reply, it should redirect to the reply form instead of the new message form, otherwise work equally
- store the draft id as a hidden field somewhere in the form, so that it will be available in $message in hook_privatemsg_message_insert(), then remove the draft in that hook.
- Just an idea: make the automatic deletion of the draft optional (checkbox when form is pre-filled from a draft) so that drafts could be used as sort of templates? no idea if that is useful...

As you see, you have some work to do :)

Feel free to contact me if there are any questsions regarding our forms/API.

Thanks! This is pretty much exactly how I planned to do it. I'll be sure to touch base with you if I have any questions. Thanks again.

Here is a patch that adds basic support for drafts of private messages. Users with 'create drafts of private messages' permission can save drafts when creating a new message or when replying to an existing thread. Users can view, edit and delete existing drafts as well as optionally retain drafts after sending.

Looks like that file didn't upload properly, not sure what happened. Here's the actual patch.

Looks pretty good already, I gave it a detailed review below. It's a long list, but don't worry, that's mostly about improving the tests, coding standards and some possible improvements of your code. The two major issues are recipients handling and form validation. See below for more details.

The tests look good already, I primarly added some ideas about additional things to test. When writing tests, try not to simply test the functionality (that too, obviously) but try to think of what could go wrong and try to verify that it can't. (like deleting all drafts and not just the selected ones, multiple recipients (types, but you probably didn't know about that)).

The test bot doesn't like something about the tests, seems related to menu arguments...

If there is anything unclear, follow up here or contact me directly.

+++ b/privatemsg_drafts/privatemsg_drafts.installundefined
@@ -0,0 +1,92 @@
+/**
+ * Implementation hook_schema().

Still missing the " of " :), same for other hooks.

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+ *
+ * @ToDos
+ * - override validation if "Save draft" is run (allow for Null recipients etc.) ¶
+ * - write automated tests
+ * ¶
+ */

Some trailing spaces here (and in other places too). I think coder.module should complain about them.

Also, the todo's seem outdated :)

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+/**
+ * Loader function for the draft passed from the URL.
+ */
+function privatemsg_drafts_draft_load($draft_id) {
+  $result = db_fetch_object(db_query('SELECT pmd.draft_id, pmd.thread_id, pmd.author, pmd.subject, pmd.recipients, pmd.body, pmd.format, pmd.timestamp FROM {pm_draft} pmd WHERE pmd.draft_id = %d', $draft_id));
+  if ($result) {
+    return $result;
+  }

Wondering if we should rename the module to the singular "privatemsg_draft", that would save the additional draft in the function name and other places like the menu path.

I know that it isn't like that with privatemsg_attachments and others, but I think it would make sense. It's node_load(), not nodes_node_load() :)

And yeah, I know that renaming modules isn't fun but it's easier get it right before we have to deal with upgrading.. trust me :)

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+/**
+ * Custom access callback.

Documentation for the params would be nice, for this and other API functions. Something like:

* @param $draft
    Private message draft object.

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+  if (user_access('create drafts of private messages')) {
+    if ($draft->author == $user->uid) {
+      return TRUE;
+    }
+    else {
+      return FALSE;
+    }

Any reason not to make this a single line?

return privatemsg_user_access('create drafts of private messages') && $drafts->author == $user->uid;

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+    case 'privatemsg_form_reply':
+      if (user_access('create drafts of private messages')) { // User is allowed to create drafts.

You should write the User is is allowed to create drafts either above or below the if. (and reword accordingly, e.g. "Check if user..").

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+    $thread = privatemsg_thread_load($draft->thread_id);
+    // @Todo - consider rendering the thread here, seems appropriate on reply drafts.

Maybe not the full thread (that can be rather large), but maybe some context like title, participants and maybe the last message? Should all be there in $thread..

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+    $form['#redirect'] = $url_prefix . '/view/' . $thread['thread_id'];

This shouldn't be necessary I think. Have you tried without that line? There is some code in the submit callback that forwards to a configurable location and defaults to the new message.

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+    foreach ($recipients as $recipient) {
+      $name = db_result(db_query("SELECT name FROM {users} WHERE uid = '%s'", $recipient->uid));
+      $recipients_list .= $name . ', ';

This looks strange. I need to check farther down you you store this, but recipients aren't limited to users, they could also be roles or others.

I think you should simply store the recipients as a string and just add it again.

Also, the query looks strange anyway, '%s' for the uid?

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+      '#type' => 'checkbox',
+      '#title' => t('Save this draft after sending.'),
+      '#description' => t('Optionally retain this draft after sending to allow for reuse later. '),

Wondering if we should use "Keep" or "Retain" instead of "Save" here? It's already saved, keeping/retaining is what this is. Not sure if what is clearer for users..

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+  $form['save_draft'] = array(
+    '#type' => 'submit',
+    '#value' => t('Save draft'),
+    '#submit' => array('privatemsg_drafts_save_draft'),
+    '#weight' => $form['submit']['#weight'] + 1,

There is no need for our validation process here, you should be able to avoid that by setting '#validate' => array() *I think*.

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+  // Store the account for which the threads are displayed.

Comment is not correct and the code is probably unecessary anyway.

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+  foreach ($drafts as $draft_id) {
+    db_query('DELETE FROM {pm_draft} WHERE draft_id = %d', $draft_id);

You might be able to do a IN condition here to only execute a single query. See db_placeholders().

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+/**
+ * Custom function to load a draft from db.
+ *
+ * @param $draft_id
+ *   Unique draft identifier
+ */
+function privatemsg_drafts_load_draft($draft_id) {
+  // Load the draft from the db.
+  $result = db_fetch_object(db_query('SELECT pmd.thread_id, pmd.author, pmd.subject, pmd.recipients, pmd.body, pmd.format, pmd.timestamp FROM {pm_draft} pmd WHERE pmd.draft_id = %d', $draft_id));
+  if ($result) {
+    return $result;
+  }

Looks like this could be removed as there is privatemsg_drafts_draft_load()?

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+  if (isset($message['draft_id']) && $message['retain_draft'] == 0) {
+     // Delete message attributes row from db.
+    db_query('DELETE FROM {pm_draft} WHERE draft_id = %d', $message['draft_id']);

No idea what that comment is trying to tell me :)

+++ b/privatemsg_drafts/privatemsg_drafts.moduleundefined
@@ -0,0 +1,321 @@
+  $args[] = serialize($message['recipients']);

As mentioned above, simpy storing the recipients string (should be somewhere in $form_state['values']) should be much easier. You also need to update the other instances to $form_state['values']['something'] when dropping the validate function.

+++ b/privatemsg_drafts/privatemsg_drafts.testundefined
@@ -0,0 +1,111 @@
+  /**
+  * Test user access to /drafts
+  * Create user without 'create drafts of private messages' permission. Try to access drafts page and see if it gives access denied error
+  * Create user with 'create drafts of private messages' permission. Try to access drafts page and see if it gives allows access
+  * Test saving, sending, editing drafts.

Comments shoul be wrapped at 80 characters.

+++ b/privatemsg_drafts/privatemsg_drafts.testundefined
@@ -0,0 +1,111 @@
+    $user_no_read_msg = $this->drupalCreateUser();

Let's give this one write and read permissions. We also want to verify that he can't access drafts even with these permissions. And maybe a second user that has create drafts permission to verify that he can't access drafts of another user.

+++ b/privatemsg_drafts/privatemsg_drafts.testundefined
@@ -0,0 +1,111 @@
+    $this->drupalLogin($user_no_read_msg);
+    $this->drupalGet('messages/drafts');
+    $this->assertResponse(403, t('HTTP Response 403: Access to mailbox was blocked to user without "<em>read privatemsg</em>" permission'));

Comment should refer to the create drafts permission here.

+++ b/privatemsg_drafts/privatemsg_drafts.testundefined
@@ -0,0 +1,111 @@
+    $edit = array();
+    $edit = array(
+      'recipient'   => $user_no_read_msg->name,

No need to initialize $edit twice :)

+++ b/privatemsg_drafts/privatemsg_drafts.testundefined
@@ -0,0 +1,111 @@
+    // Test deleting the draft
+    $edit = array();
+    $edit['drafts[1]'] = '1';
+    $this->drupalPost('messages/drafts', $edit, t('Delete selected drafts'));
+    $this->assertNoText($edit['subject'], 'Draft deleted successfully.');

Can you add another assertText() here to verify that the "no drafts" message is shown now? Or do that above when checking access and only delete this draft after you created the second. To verify that only the checked draft is deleted.

+++ b/privatemsg_drafts/privatemsg_drafts.testundefined
@@ -0,0 +1,111 @@
+    $edit['retain_draft'] = FALSE; // Force the deletion of the draft.
+    $edit['draft_id'] = '2';

Adding the draft_id shouldn't be necessary, that's already there in the form.

+++ b/privatemsg_drafts/privatemsg_drafts.testundefined
@@ -0,0 +1,111 @@
+    $this->drupalPost('messages/drafts/' . $edit['draft_id'], $edit, t('Send message'));

Instead of $edit, simply pass an empty array here(). Simpletest will automatically use the given default values and that ensures that the checkbox defaults to false and the message can be sent without specifying anything explicitly.

+++ b/privatemsg_drafts/privatemsg_drafts.testundefined
@@ -0,0 +1,111 @@
+    // Send new draft with "retain draft" selected, assert that it is not deleted from the db
+    $edit = array();
+    $edit = array(
+      'recipient'   => $user_no_read_msg->name,
+      'subject'     => $this->randomName(20),
+      'body'        => $this->randomName(100),
+    );

Let's add a second user as a recipient here, or maybe even better, let's add a role (you will need to enable privatemsg_roles and give the corresponding permissions for that to work, see privatemsg_roles.test). Because that will actually fail with your current code, as explained above.

+++ b/privatemsg_drafts/privatemsg_drafts.testundefined
@@ -0,0 +1,111 @@
+    $edit['retain_draft'] = TRUE;
+    $edit['draft_id'] = '3';
+    $this->drupalPost('messages/drafts/3', $edit, t('Send message'));

Same here, all you need to POST explicitly is the retain_draft value, everything else can use the default value.

Powered by Dreditor.

Am I right to assume the 7.x-1.x version of that feature will come by porting the patch after it has been committed into 6.x-2.x?

If someone works on the patch and I commit it to 6.x-2.x, I will also port it to 7.x-2.x (not 7.x-1.x)

Here is the latest patch that I had last worked on months ago. It still needs a little work for a generic deployment, but it is being used on a pretty large production site (3000+ users) with no major issues to this point. I wish I had time to finish this up so that it could be committed, but just don't have the bandwidth right now. I was really enjoying working on it, but happy to pass it along.

when will it have D7 version of this feature?

*ping* Any hopes? I've got a request for this feature on one of my sites.

This issue is being closed because it is against a branch for a version of Drupal that is no longer supported.
If you feel that this issue is still valid, feel free to re-open and update it (and any possible patch) to work with the 7.x-1.x branch (bug fixes only) or the 7.x-2.x branch.
Thank you.