Add tagging actions to message listing

My early thread patches contained actions for the first two points. Not sure how to handle the third...

I will create a first patch...

I am unsure.. it should be possible to add the same autocomplete field to the action form but we may have to handle it differently, for example only add tags, and not remove them, as we can't display those that are currently used.

Of course, the ultimative goal is something like gmail.com, but that's not easy to implement.

A first patch to get things rolling..

Only implements 1., we may want to split these up into multiple patches maybe, as I'm not sure how to handle the other things yet...

I had to move some code of privatemsg_list_submit() into a helper function but it wasn't that hard after I was able to understand my own code again :)

And here's a quick review to get the ball rolling (haven't bothered testing the code yet though):

+ // Only execute something if we have a valid callback and <strong>atleast</strong> one checked thread.

This little spelling mistake was already there, but still, may as well fix it.

+function privatemsg_operation_execute($operation, $threads) {

No function documentation. :)
Should this be _privatemsg_operation_execute as it's a helper?

+ // Check if that operation has defined a undo callback

No full stop! :)

+  if (isset($operation['undo callback']) && $undo_function = $operation['undo callback']) {
+  // Add in callback arguments if present.
+    if (isset($operation['undo callback arguments'])) {

Shouldn't this comment be indented once more?

+}
+/**

When I'm coding, in my bike shed, I like to put a blank line between the end of the last function and the function documentation of the next one. :-!

+  foreach ($threads as $thread) {
+    if (db_result(db_query('SELECT COUNT(*) FROM {pm_tags_index} WHERE tag_id = %d AND (uid = %d AND thread_id = %d)', $tag_id, $user->uid, $thread)) == 0) {
+      drupal_set_message(sprintf('INSERT INTO {pm_tags_index} (tag_id, uid, thread_id) VALUES (%d, %d, %d)', $tag_id, $user->uid, $thread));
+      db_query('INSERT INTO {pm_tags_index} (tag_id, uid, thread_id) VALUES (%d, %d, %d)', $tag_id, $user->uid, $thread);
+    }
+  }

This is a bit scary, some documentation would make it less-so. :) Also, what's that drupal_set_message(sprintf( [...] )); statement doing there, is that a bit of debug left over?

Without running the code, I can roughly tell that the user selects a bunch of messages using the checkboxes, then adds a tag using a text box. The code then iterates through each thread setting the tag. The code looks pretty sound IMO.

Re-rolled.

Fixed all things pointed out in the review, I think.

Nbz mind starting a new thread to initiate effort regarding your last comment?

Side note of reviewing this:

Inbox/message listing UI does not have any kind of indication which tag a message has. Perpahs we could use gmail's method (yet again) with little X that removes tag, but that works only for JS enabled browsers. Remind me what is our policy on JS support and graceful degradation (i really should remember this... sorry.. possibly even add notice to project page).

The above consists of 2 points: 1) *important* show which tags thread has , 2) less important have ajaxy X thing that remove tags (on 2nd thought this is really not important).
^ Not sure this should be part of this patch. Most likely not.

Non-code patch review: So i applied it and i got a select box in message listing page where i can mass add tags to messages, and it works... What else am i supposed to see? Also i think DSM of the mass-tagg needs to say which tag was applied to those 'n' threads. For consistency sake, Do we want to offer an undo feature like we have for 'set read/unread' ?

Perhaps some one need to write a QA sheet (oooooo so official :-D managerial bs.) with points that supposed to appear with the patch. Cheers.

+++ privatemsg_filter/privatemsg_filter.module	25 Aug 2009 18:11:51 -0000
@@ -91,6 +91,94 @@
+ * @param $tags A single tag or an array of tags.

That is missing a newline :)

+++ privatemsg_filter/privatemsg_filter.module	25 Aug 2009 18:11:51 -0000
@@ -91,6 +91,94 @@
+ * @param $threads A single thread id or an array of thread ids.
+ * @param $tag_id Id of the tag.
+ */
+function privatemsg_filter_add_tags($threads, $tag_id, $account = NULL) {

newlines again. Regarding the function name, you use "tags", but it only possible to save a single tag. Maybe rename to ".._tag" ? tags does make more sense for remove, since you can optionally remove all tags there.

Can't find anything else here, nice cleanup.

This review is powered by Dreditor.

Added to 6.x-1.x-dev. Will port to D7 later (If anyone wants to try, go for it!)

Fixed, back to old status.

Added to 7.x-1.x-dev.

Attached is the updated patch that just just has been commited.