Show author(s) when viewing inbox

IMO this will not work too well with threading - there can be multiple authors for multiple messages.

I thought about this too...

The inbox shows last updated (Well, atleast with my patch :) ), why not showing a similiar "Last author" ? I don't know the schema in detail, so I'm not sure if it is technically possible, but as each message has only one author it should be imho...

Yes i think the last editor is a good idea, and i think there is enough space to show the messages starter like a forum post.

Greetings

strauch.

Any idea *which* author the above patch will show?

I think it will generally the first, but am not sure if that is always the case...

That exactly the problem of the resulting query... it is not exactly defined. MySQL is nice and takes one, probably the first. For most DBMS (AFAIK including pgsql), each element in SELECT has to be either in GROUP BY too or must be a function like sum, max, ... (Actually, this is also true for pm.subject...)

I think the only possibility is a rather complex subquery.... This does work for me:

  $fragments['select'][]      = 'pmi.thread_id';
  $fragments['select'][]      = 'pm.subject';
  $fragments['select'][]      = '(SELECT author
                                 FROM pm_message pmsub
                                 INNER JOIN pm_index pmisub ON pmsub.mid = pmisub.mid
                                 WHERE pmisub.thread_id = pmi.thread_id
                                 ORDER BY pmsub.timestamp DESC LIMIT 0,1) as author';
  $fragments['select'][]      = 'MAX(pm.timestamp) as timestamp';
  $fragments['select'][]      = 'MAX(pmi.new) as new';
  $fragments['inner_join'][]  = 'INNER JOIN {pm_index} pmi ON pm.mid = pmi.mid';
  $fragments['where'][]       = 'pmi.uid = %d';
  $fragments['where'][]       = 'pmi.deleted = 0';
  $fragments['group_by'][]    = 'pmi.thread_id';
  $fragments['group_by'][]    = 'pm.subject';

And we need to re-add the author case in privatemsg_list_messages..

        case 'author':
          $col = t('Author');
          break;

Is it possible to get a list of all recipients inside the main query?

I was thinking a good idea may be to have a "participants: ..." line below the titles, just like we have above the messages on the messages page.

Actually, there is a simple way... atleast for MySQL...
http://dev.mysql.com/doc/refman/5.0/en/group-by-functions.html#function_...

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

mysql> SELECT pmi.thread_id, pm.subject, GROUP_CONCAT(DISTINCT author SEPARATOR ",") as author, MAX(pm.timestamp) as timestamp, MAX(pmi.new) as new FROM pm_message pm INNER JOIN pm_index pmi ON pm.mid = pmi.mid WHERE (pmi.uid = 1) AND (pmi.deleted = 0) GROUP BY pmi.thread_id, pm.subject ORDER BY pmi.new desc, MAX(pmi.mid) DESC;
+-----------+-------------------+--------+------------+------+
| thread_id | subject           | author | timestamp  | new  |
+-----------+-------------------+--------+------------+------+
|         9 | Message from test | 5      | 1225761108 |    0 | 
|         1 | asdf              | 1      | 1225757257 |    0 | 
|         6 | adsf              | 25,1   | 1225387460 |    0 | 
+-----------+-------------------+--------+------------+------+
3 rows in set (0.00 sec)

We could then do a explode(',', $row['author']) and re-use the code which generates the "Participants:.."-String. (with limits on the number of users of course).

I can work out a proper patch (with pgsql-support) when #324406: refactoring of privatemsg_list (was: Make use of theme() functions in the inbox) has been commited, doing it now would make it unecessary complicated imho.

Ok, I have a first version working, but it's a bit bigger than expected, see the reasons below..

After creating a drupal installation with PostgreSQL (8.3), I found out that I can't even install the privatemsg module.

Things I changed for (general) PostgreSQL-Support
- new seems to be a reserved keyword in PostgreSQL, renamed to new_flag (meaning : schema, update function, queries, ...). It would probably be possible to escape it, but I don't like that, identifier quoting tends to be something complex and error-prone..
- indexes were using the mysql specific identifiers (`), had no idea why, removed them for now... see above ;)
- the COUNT() solution for GROUP BY didn't worked so I had to tweak it a bit, see comment in patch
- Removed the mid column from the INSERT INTO pm_message query
- Added a subquery which does the same as MySQL's GROUP_CONCAT

After that it was quite simple to display the authors...

- splitted up the privatemsg_preprocess_privatemsg_to function to be able to re-use the name-display stuff
- exploded the user list, and displaying the first few authors below the title.

Open Questions..
- Move everything that is PostgreSQL-related to an own issue.. ? Probably yes, but I'm leaving it in for a first review... I would need to revert my database to test a mysql-only version :)
- Display all participants or only authors ? All participants would need more client-side logic, because these are stored different and I'm not sure if this would be an useful information.

Do you have a patch I can look at to see how much does change?

I *think* the schema changes would be best done as part of the upgrade patch from drupal 5 (#314066: Upgrade path from Drupal 5.). It may be a good idea to combine the schema fixes into that patch and give everyone a working upgrade path. (Or it may not.)

As for who to display, I am not too bothered - either is good with me I doubt it would be any/much extra code to have them all. Probably less as you do not have to check if the user is an author or not.

Erm.... Yes of course, no idea what happened with my patch, I *had* it attached...

About the schema changes... I've never used the "old" privatemsg, actually never used a drupal version older than 6, so I don't know that schema. My idea was to provide an example update function, because the field needs to be renamed in order to test my patch... Of course, this can also be done by hand.

There is no need to have used the drupal 5 version of privatemsg.

My plan is to have

 if (table_exists('pm_index')) {
  db_drop_index($ret, 'pm_index', 'new');
  db_change_field($ret, 'pm_index', 'new', 'new_flag', array(
        'description'   => t('Whether the user read his message'),
        'type'          => 'int',
        'default'       => 1,
        'not null'      => TRUE,
        'unsigned'      => TRUE));
  db_add_index($ret, 'pm_index', 'new_flag', array('mid','uid','new_flag'));
}

followed by the stuff for upgrading from drupal 5 (which includes creating the drupal 6 tables).

Is there a different name we can use instead of new_flag? unread should also work right?

Just applied this module (ignore my earlier post about combining this with the upgrade patch - this can go later on its own.), and it more or less works - except for the addition of a second GROUP BY of pm.subject - this caused the groupings to fail on my test site and show individual messages.

Rerolled with that change removed.

I also added a test to see if the "new" column exists in the upgrade function. (I still think the column needs a better name than new_flag. ideas?)

Hm, interesting... Do you have threads where the subject changes ?

The problem is that PostgreSQL needs this GROUP BY. For most DBMS, except MySQL, every field in a GROUP BY-Query needs either to be inside a AGGREGRATE function like MAX()/COUNT()/... or explicitly listed in GROUP by.

The question is what we want to do with those multiple-subject threads... (which should only exist when upgrading, right ?)

A Solution would be to add a MAX or MIN... this does simply order the different subject's alphabetically and displays the first/last. There seems to be no simple way to select the first/last subject.

BTW, is there an actual reason that each message has its own subject but it can't be changed or is it just a leftover from the old version ?

no, the (viewable) threads had the same title, but there was a new row per thread. kind of weird.

They should only exist on upgrade, but even here if we need to, we can probably get rid of them as only the first subject in a thread is used.

BTW, is there an actual reason that each message has its own subject but it can't be changed or is it just a leftover from the old version ?

Mostly historical I would say. I also cannot see a good way to get rid of it...

I can't reproduce it here, it does work correctly for me with MySQL 5.0.67 and PostgreSQL 8.3. (With and Without MAX())

Does it work for you when you have a MAX() ?

  $fragments['select'][]      = 'MAX(pm.subject) as subject'; 

Yes, that works correctly, but I think it may be best to go for MIN instead of MAX. My reasoning being that the main cause for different subject lines will be replies from drupal 5 which add "Re: " to the start of the subject line for the replies. MIN should (mostly) get the shorter of the lines, which would the the original subject.

I tested the above (without the second GROUP BY), and it seemed to work. No idea what will happen if the original message started with a letter coming after R in the alphabet...

I guess it depends on hiow the function calculates a min and a max for a string - if it goes off length, it should still work, but I have absolutely no clue if that is the case.

just tested and if a message starts with a letter that is after r in the alphabet and if a reply has an "re" before it, the re is used with the min.

Still, not too bad and IMO better than the current alternatives.

Just wondering about the pgsql issue - according to this all we need to do is add double quoted around table names. Will that work in mysql?

If so, it is an (ugly) option.

No, identifier (table, fields and so on) quoting is not standardized so most dbms are using another characters... mysql does use the backtick (`). That's what I mentioned above:
"It would probably be possible to escape it, but I don't like that, identifier quoting tends to be something complex and error-prone.."

So yes, it would be possible to insert dbms specific quoting characters, but it would be really ugly. Imho, it should be avoided if possible.

As a sidenote: Drupal 6 currently supports only MySQL and pgsql so this is not (yet...) an issue, but Oracle for example does even more crazy things... normally, Oracle transforms all identifiers to uppercase, except when they are quoted. So, in Oracle, 'SELECT * FROM table' and 'SELECT * FROM "table"' will not read from the same table. And then, at the latest, it gets *really* complicated :)

k, thanks for the explanation. I agree that keeping things simple and and following the approach in the above patches is probably best.

thanks for the patch, it works fine, but i would prever a third column for the authors. What do you think?

Thanks

strauch

In the menu "send messages" it would be nice if there is the is a collumn with the user i sent the message. In the menu "all messages" it would be nice if there are all names at messages i get no answer, so the name of the user i sent the message.

And i don't need to see my own username in this list, because it is logical that i'm a user of this message.

A couple of things:

1. The pgsql should IMO be a separate patch. Keeps things easier to follow. I will do that in a bit if no one beats me to it.

2. Instead of "new_flag", maybe call the column "newmsg"?

1. I can split up the patch but we should work then on either the author or the pgsql issue first because they are conflicting each other a bit (author patch would add the GROUP_CONCAT and the pgsql patch needs to replace that with the conditional statement).

2. new_flag was just the first thing that came into my mind when I created the patch, we could also use "unread", "is_new" or your "newmsg". I personally don't really like newmsg, but the name isn't so important after all...

Back to the topic, some questions and updates..

a) Well, the display part is inside the theme function, so it can be overwritten easily if someone wants a different presentation (of the same data).

b) BUT, the current implementation does not allow to change the columns because that is defined outside of the theme function. My current approach to solve that is, instead of the current static keys, to use the translated strings as keys. privatemsg_list_messages then simply reads the keys of the first row and creates the head-defintion from it. The only additional check there is to enably sorting only for date and subject (for now).

Short example, inside the theme function:
$themedRow[t('Subject')] = '<span class="privatemsg-list-subject'. $unread.'">' . l($row['subject'], 'messages/view/'. $row['thread_id']) . '</span>';

and the head-generating part:

    $head = array();
    foreach (array_keys($rows[0]) as $index) {
      $head[$index] =  array('data' => $index);
      if ($index == t('Last updated') || $index == t('Subject')) {
        $head[$index] += array('field' => $index, 'sort'=> 'desc');
      }
    }

Suggestions? Better ideas ?

c) What do we want to do with sent messages now ? Because the suggestion of strauch (displaying the recipients at sent messages and the authors at all messages) would actually add useful information to that page and imho be more useful than a generic Participants: information.

d) Another thing to think about is performance. I generated a few 100 threads each with ~20 participants and ~15 Messages (It's an extreme combination but I wanted to test exactly that case). If we display the latest 4 authors for 25 threads, we end up with ~4 * 25 user_load's (including all the modules that hook into that like profile) just to be able to link to the author's profile. By adding a static user cache this goes down a bit because chances are high that the same authors appear multiple times. But even then I'm still on 68 user_load's (author's are randomly taken) with my test data, this means ~200 sql queries with my actived modules.

I would say the pgsql bit is more important, and more ready. Leave the GROUP_CONCAT and the pgsql bit of author out for now.

The name of the column does not really matter, but I just do not like new_flag (if that makes any sense...) is_new looks best to me.

c) - sent messages will probably disappear as a menu item (if I get my evil way with it).

Hi,

when i want to apply the patch from nbz i get the following error:

File to patch: privatemsgapi/privatemsgapi.inc
patching file privatemsgapi/privatemsgapi.inc
Hunk #1 FAILED at 121.
Hunk #2 FAILED at 147.
2 out of 2 hunks FAILED -- saving rejects to file privatemsgapi/privatemsgapi.inc.rej

When i ignore this error and install the modul i get the error:

"user warning: Unknown column 'pmi.is_new' in 'where clause' query: SELECT COUNT(DISTINCT thread_id) as unread_count FROM drupal_pm_index pmi WHERE (pmi.is_new = 1) AND (pmi.uid = 1) in www/drupal/sites/all/modules/privatemsg/privatemsg.module on line 376."

Does this patch not work with the newest version of privatemsg HEAD?

This is an old patch, parts of it were commited together with the upgrade path mega patch.

I'll try to create a new patch together with other UI improvements of list-messages soon.

Attached is a re-rolled patch.

Edit: Forgot to say, implemented strauch's ideas on #22 and #22

Changing version..

uh nice thanks, i will test it.

have a nice xmas

Thank you Berdir, this patch works great, maybe you can add more features like how many anwsers the message has and when the messages "starts" like in a forum? maybe as an option in the adminpanel which information is shown?

What easily could be done is fetch those informations from the database. Then, if someone wants to display more/other informations, he can always override the theme.

But the current theme might chance anyway quite a bit to allow things like #348907: Per thread/Multiple thread actions. That might even include to go away from a row theme function to a theme function for the whole page. But I have to think about this...

I'll create a new patch with the following information, please add anything you would like to have, I'll see if it is possible to add it.
- Number of messages/answers
- The time the first message was sent.

Attached is a new patch.

Changes:
- Added count and MIN timestamp
- Slight change for threads with more than 4 authors (It displays now "and others" instead of ", others"

Thank you very much berdir, ich will install and test it, but until the old patch works very fine :-).

Ok, i tested the patch,i didn't get any further information in the inbox, but i see under sent messages i get an error, no idea if the reason is the patch:

user warning: Table 'drupal.pm_index' doesn't exist query: SELECT pmi.thread_id, pm.subject, (SELECT GROUP_CONCAT(DISTINCT pmia.uid SEPARATOR ",") FROM pm_index pmia WHERE pmia.thread_id = pmi.thread_id AND pmia.uid <> 1) AS recipient, MAX(pm.timestamp) as timestamp FROM drupal_pm_message pm INNER JOIN drupal_pm_index pmi ON pm.mid = pmi.mid WHERE (pmi.uid = 1) AND (pm.author = 1) AND (pmi.deleted = 0) GROUP BY pmi.thread_id ORDER BY timestamp desc LIMIT 0, 25 in /is/htdocs/www/drupal/sites/all/modules/privatemsg/privatemsg.module on line 316.

> i didn't get any further information in the inbox,
The query simply loads the mentioned information from the database, but the default theme function does not use them, If you want to display them, you have to override the default theme function.

> but i see under sent messages i get an error, no idea if the reason is the patch:
Arg, I should really use a development environment with a database prefix, I tend to forget those curly braces... For a short fix, replace "pm_index pmia" with "{pm_index} pmia" in the function privatemsg_privatemsg_sent_alter". I will update my patch.

Updated Patch to fix missing curly braces

thanks alot for the hardwork. this is definitely a must module for a community/forum site.

Thanks!

Hi Berdir,

is it possible to get myself out of the authors list in the inbox? I will test to upgrade the theme to see the further information the next days. The patch works fine so far.

Thanks

strauch

Yes this patch works too.

Thanks

strauch

The patch looks good, does what it says it does :).

Commited to 6-dev.

Thanks!