Move "Sent Messages" to privatemsg_filter, Add inbox, improve user filtering.

In teresting thought. i'll ask around IRC of peoplethat use PMSG to see what they think.

One thing to consider is that at a later date (when we feel we are ready to do this) i would like to enable message tagging. so instead of folders you can create tags to categorize messages (like google does). so the 'inbox' will list all converstions but you can then click on a message 'tag' to see only messages associated with that tag.

+1 for dumping the sent messages mailbox -- and "folders" as well.

There's an error when a user *without* "read all private messages" is reading his messages:

"user warning: Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause query: SELECT MAX(pmi.mid) as id, pmi.thread_id, pm.subject, pmi.author, pmi.timestamp, pmi.new FROM pm_message pm INNER JOIN pm_index pmi ON pm.mid = pmi.mid WHERE (pmi.recipient = 3) AND (pmi.recipient_del = 0)UNION SELECT MAX(pm.mid) as id, pmi.thread_id, pm.subject, pmi.recipient, pmi.timestamp FROM pm_message pm INNER JOIN pm_index pmi ON pm.mid = pmi.mid WHERE (pmi.author = 3) AND (pmi.author_del = 0) GROUP BY pmi.thread_id ORDER BY pmi.new desc, MAX(pmi.mid) DESC"

Also, reconsider how the query is built: a user *with* "read all private messages" will see a different view of his own messages than he has previously.

Similar to the test before it, change the display only if the account doesn't belong to the user:

if ($account->uid != $GLOBALS['user']->uid) {

If I'm not mistaken, at that point in the code the user must have already passed an access check for those to be different.

Also, privatemsg_block() needs to be updated to remove the Sent messages link.

I believe you can get rid of this whole section:

   switch (arg(1)) {
-    case 'sent':
-      $query = _privatemsg_assemble_query('privatemsg_list_sent', $account);
-      break;
     case 'inbox':
     default:

and the if() wrapper here:

     if (arg(1) == 'inbox') {
       $query = _privatemsg_assemble_query('privatemsg_list', $account);
     }

I also got rid of author/recipient columns as they do not work too well in a conversation context(multiple authors/recipients).

Makes sense...

I also got rid of the order_by code and reinstated what we had before - I've been thinking that ordering by new at the top may not really be the best idea, and this (older) way also uses the core functions. comments?

...but this doesn't. You're removing the ability to sort by clicking on the remaining column headers, which the user expects to be able to do -- since core makes them clickable and gives them sort indicators.

how do you unset a NULL column?

By number instead of by key?

There's also a comment (#38) at #299491: Put a reply form on the message page that you might address as long as we're making changes to the list view.

As for "order by", have you tested it? I am pretty sure it worked when I made the changes...

Didn't think I needed to when I looked at the patch. What's handling the clicks on the column headers?

Okay, so I applied the patch. Looking at privatemsg_privatemsg_list_alter() I can see that there are no ORDER BY clauses. As a result there is no default sort when the list is viewed. Since the column header for "received" indicates there's a descending sort on this column, we need to at least actually perform the sort.

And I oppose removing the sort for "new" -- users -- this user, at least -- want to easily access new messages.

"received" no longer makes sense as a column header, since sent messages are also in the list. Perhaps "date"? Or "latest"?

I'm seeing multiple messages from a single thread in the list. Seems to be one line for sent messages, another for received messages in the same thread.

privatemsg_unread_count() gives a count of unread messages, rather than a count of threads with unread messages. This is confusing to users who see two threads with unread messages in the list, while seeing "Read Messages (3)" in the block.

And I'd sure like to see some indication in the list of at least one of the other participants.

Finally, unset($row['new']); needs to be put back to remove that column from the list.

What's handling the clicks on the column headers?

the $tablesort...

No, what I mean is: what does the actual sorting when you click on a column header? Is this something that core handles? I had added the additional sorting code because it didn't appear that anything was responding to the clicks. api.drupal.org doesn't -- at least as far as I can find -- indicate that core handles this.

using the old method with this method results in the following syntax: "ORDER BY NEW ORDER BY {Whatever}

I really ought to completely check this out before I comment. I was partially wrong; the clause I get by default is: ORDER BY new DESC -- the list doesn't sort by "received." (And I forgot that tablesort_sql() adds that ORDER BY clause.)

Finally, unset($row['new']); needs to be put back to remove that column from the list.

Is it not there?

Nope!

I hope this patch and the others gets in soon, as I need to update my access control patch and work on a couple other areas.

whats the news with this one ?

i tested the patch and got

Hunk #2 succeeded at 256 (offset 1 line).
Hunk #3 succeeded at 640 (offset -11 lines).
Hunk #4 succeeded at 664 (offset -11 lines).
Hunk #5 succeeded at 837 (offset -11 lines).

So here is a rerole of the patch

The patch works perfect here

Not looked at the patch yet, but "Array" is either because page callback => drupal_get_form is missing or the menu has not been rebuilt.

Firstly, is there any reason why this has to wait for #348907: Per thread/Multiple thread actions; are you just anticipating that patch will go in first?

I very much like using filters instead of a monolithic switch/case construct in the message list code, filters are far more elegant. Aesthetically, Inbox/Sent messages look better in secondary tabs, under 'All Messages', than as primary tabs, in my opinion.

I only reviewed the patch in #27, but couldn't find anything wrong with it. However, the patch in #27 does include the code from #367745: permissions for privatemsg_filter need to be improved., so that will need to go in before this can be RTBC.

One question:

-      '#title' => t('Authors'),
+      '#title' => t('Participants'),

In the message list the header is 'Authors', yet in the message thread display it's 'Participants'. Which should this be?

Just did some more testing of this. Tried switching off the filter permissions for authenticated users, then used a standard user to access 'Inbox' and 'Sent messages'. This allowed the use of these special filters, but hid the filter form, and is exactly the behaviour I would expect. Switching the filter module off also didn't break anything, just quietly switched off the Inbox and Sent messages menu items.

Two small problems with this patch, however:

1. selecting 'All messages' doesn't select 'Inbox' automatically, this makes the user feel like they are in limbo somewhat;
2. are users going to be confused that they have to switch the Privatemsg filter to get the Inbox / Sent messages menu items? Not sure what could be done about this, however.

*** EDIT ***

Yes, just anticipating that. Should I just let the two race each other?

Fair enough, I was just wondering. :)

If #367745: permissions for privatemsg_filter need to be improved. is committed shortly, this patch may get in before #348907: Per thread/Multiple thread actions (I could be wrong of course, but this patch seems to be nearly ready).

I do not like the word participants in this context either, so maybe just change it to "users"?

I was thinking that we should just pick either 'participants' or 'authors' and use it for everything. Maybe litwol would like to chime in, or perhaps we should just leave it as 'participants' for now and create a separate issue or discuss it in IRC.

I haven't looked at the code or output yet, just a few comments to the discussion

- The switch statement is killed in my patch (thread actions) anyway, in favor of a argument which is defined in hook_menu. ($query_name). That could either be removed or instead added as a argument to the list query function. This would allow to change the query based on a unified solution instead on relying on the url or something "outside". Just an idea..

- There is imho a difference between author and participant. Participants are users which recieved a message while authors are only those that wrote a message inside that thread (maybe a sub-group of the participants, maybe all of them). Currently, the list only displays authors, not participants. I didn't define that, just adopted it as I understood it.

- About the placement of the url's/pages/listings, the ultimative goal is to make this configurable. I'm not sure what's the best default placement. Need to try the patch first.

- I don't care which patch goes in first, both need to be adapted.

Another thing about $query_name. I used it to set the header in an older patch (this was the main reason I implemented it), this is now based on the actual query.

A few comments, just looking throug the patch..

- no need to use a subquery for mysql, just group_concat should work fine
- Not sure about the autor/participants stuff, we need to think/discuss about this. IRC would be nice, we should also include litwol in that discussion
- I like the $argument thing. Thinking about ilo's work for tags and filters, I'm wondering if we could even use two variables.. something like $argument_type and $argument. $argument type could be filter, tag, fixed or whatever. So a module could distinguish between the fixed sent display, and a user tag or filter called sent. Just an idea and we don't need to do that in that patch.
- An idea to shorten the code a bit. The menu definitions in privatemgs_filter_menu are the same, except of the page arguments. You could create a $default menu defnition, and use something l.ike:

$items['messages/list/inbox'] = $default + array('page arguments' => array(''privatemsg_list', 'inbox')));

- That part might not work for everything, but I haven't tested it.

  if ($argument == 'inbox') {
    $fragments['where'][]       = "pm.author <> %d";
    $fragments['query_args'][]   = $account->uid;
  }

For example, when I send a message and recieve an answer, it should show up in the inbox. Because we use group by, distinct and other crazy stuff, this might not work as expected. I need to test this (on mysql and pgsql)

      $fragments['select'][]      = 'GROUP_CONCAT(DISTINCT author SEPARATOR ",") as author';

This is the current GROUP_CONCAT, in your patch, you used a SELECT GROUP CONCAT... which does add a unecessary complexity.

About the inbox stuff, I used HAVING in my patch at #351542-4: user interface (there is a better (actually working) implementation of HAVING in the flush patch), It was the only working way I got working. However, your and my implementation has a pretty big flaw. It is not possible or atleast very hard to do a "Archive"-Feature (aka: remove from inbox)). A way to easily add a archive feature would be to use a "Inbox" tag instead, which is auto-added when the message is sent. This tag can be removed and then the thread removed from the inbox. (a new message would re-add it automatically).

The basic part of this should not be hard to implement (it just needs a privatemsg_message_insert hook), but there are several things to consider, like upgrade path, "system tags" and their handling, and so on...

I have tested it, and so far all went well. So far, the only thing I noticed is the "Sent messages" is still in the Private Messages Block, which is not only superfluous now, but also points to the wrong link. I have manually deleted it, but perhaps something for you to consider in a future version.

We seem to have misplaced the filter though... but that could have happened with the Per thread/Multiple thread actions actually.

Edit: I didn't mean to change the status... no idea how that happened!

About the apidoc format, it should be like:

@param $form_state
  Form state array

No type and the description should be on the next line.

And, the patch still depends on the theme functions, I forgot these :-/.

Still haven't actually tested the patch. When privatemsg_filter is enabled, is it correct that there is Messages, Inbox, Sent and "All messages"?. Maybe, you could instead rename Messages to All messages with http://api.drupal.org/api/function/hook_menu_link_alter/6.

I had that problem too. AFAIK, that's not a limitation of the query builder but the database. Try calling the subquery in HAVING directly, like:

$fragments['having'][] = '(SELECT ....) = %d OR (SELECT ...) <>...';

You need to specify it twice then, but the db should be intelligent enough to only execute it once. Or maybe the <> check could be moved the where. I'll try to get it working on mysql and pgsql.

A big problem of having is that this will change the order of query_args. If another module adds more query_args to where statements, the order isn't correct anymore. The only way around this is having multiple query_args arrays for where and having.

Ok, I can see what your problem was now :)

The subqueries reference to thread_id, but that column is not available in the count query. And I was not able to get it working on pgsql either so far :(.

However, there is a really easy solution which works on both databases, but is slow. Bot databases support the following syntax: SELECT COUNT(*) FROM (some_query).

This means that all we need to do is to add the following line and we can forget about all that special handling of the count query.

$count = 'SELECT COUNT(*) FROM ('. $query .') as count';

We could optimize this a bit, because we don't need ORDER BY and so on (which is the slowest part of that query) for the inner query, but it will remain slow.

Also, there is still the possibility to use a tag-based approach for the inbox, but the hard part there is the upgrade path.

1) Participants listing column doesn't show the participants.
2) Answers count is incorrect. - if i send a new msg to myself, it lists as if there is 1 reply

oops ..

Great work. its in :)