Better access to another user's messages

I am looking into this atm.

I think the current method to do this is suboptimal as viewing the inboxes of other users by going to messages/% or messages/inbox/% also shows the write new message tab.

For viewing another users messages, I think there should be new menu item(s) at user/%/messages (local task), user/%/messages/inbox (default localtask) and user/%/messages/sent (local task).

I am hacking around this atm and if I find a (hacky) nugget, I will post a patch.

A patch-that-needs-work to do what I mentioned above.

It needs permissions stuff to eb done properly. Currently it just checks if the user has read privatemsg permission, but this needs to be turned into a permissions call where it will check for read privatemsg permission id the person is viewing own account, but use read all privatemsg permission if viewing another user's account.

updated patch - only show these tabs for users with permissions to read all private messages. I guess this is acceptable as a user can view own messages through the normal way.

Any reason to make the new privatemsg_access function private?

As for the problemic tabs, I have opened another issue: #303087: Move "Sent Messages" to privatemsg_filter, Add inbox, improve user filtering. to get opinions. I think the sent messages menu item(s) do not work in threaded conversations.

EDIT - oops, fixed the link.

I don't think there are implications, as I have used private functions from other modules.

PS if you are online, hop into irc channel drupal-games - planning to have a talk about the module with litwol as soon as he has reviewed some patches (I use x-chat. Free and available for Windows.)

reroll of #6 to cover changes in HEAD.

This will probably not go in, but I am posting it as a (currently) working fallback position.

Second patch is the same as above, but with no option to view sent messages of the user.

In a threaded conversation (which is where I would like to take this module), the latter will probably be the better method to use for viewing the inbox if we go down the current approach of having user/%/messages (which is unlikely).

I dont have an issue of using user/%/messages as the messages namespace. Whichever solution we pick, as long as its consistent i'm fine with it.

I have sat on it and I really don't think it is a good idea to cram everything into one namespace. Why?

1. If we did, I would prefer the messages/uid... approach but I cannot think how to make the ui work with that.
2. If we go user/uid/messages, while the lists are user specific actions, the message view and new message are not.

Unless I can get a pointer to how to make the ui work, I prefer the approach already implemented in the attached patches (with the exception that$box should be renamed to $tag as that is what it wil eventually become and already is sort of.)

In short all the current problems I can think of are:

1. UI to get to another user's messages. Simple in approach 2, I have no idea how to do this in approach 1.
2. Top level menu item to own messages. Simple in approach 1, will probably need a hack for approach 2.
3. Keep message viewing simple for moderators (ie an admin is sent a link to a message complaining about abuse). This would be the same for all users with access in approach 2, but with one it would be a little (not much) more envolved.
4. New messages. All users will have the same link with 1 (unless we want a UID in there in this case too), but different with case 2(or same if we include UID). IMO user/%/messages/new is acceptable, but it may confuse those used to typing in address manually as being a method to send the message to that user (I did put something similar to that in a previous patch...).

The main issue IMO is the first one - I do not know how to add a UI to access other users' messages with that namespacing system.

I find the hybrid system in the patches simpler than going purely on one or the other.

It is your call.

Huh? I just stumbled on this issue and am completely perplexed as to why I got volunteered in that last comment. What does this have to do with me?

Michelle

hehe, that was more about the integration with advanced forum etc as it was about how to send a user new messages. Just ignore that comment.

PS viewing another user's messages is totally broken atm.

I was thinking, maybe only allow to view another users messages via the admin pages? something like admin/content/messages?

i prefer user/%/messages/%

We need to get this rolling again..

Trying to catch up the conversation, I'm with nbz.. Putting all in one namespace (/user/%user/messages XOR /messages) seems very hard to do and doesn't make sense from a user perspective imho.. (for example, is user/%user/messages/new now the generic write new message form, or is it a form to send %user a message?, it could actually be both, depending on which user you're looking at.. I don't really like that :))

My Ideas:
- Keep what we currently have at /messages, but remove the /messages/%uid thing
- Add a new Tab "messages" below user/%user => user/%user/messages. This one is only visible if the user has the permission to read alle messages
- Add "All messages/Inbox", "Read message", ("Sent") Sub-Tabs to messages
- These are just menu items, they point to the same functions as the /messages menu items, maybe with an optional argument indicating that it's an admin view
- That "prefix/namespace" allows us to know at which user's messages we're looking. Read message is only displayed when looking at a message, just as in /messsages
- I am not sure if that needs to be read-only or not.. While I think it's useful to be able to delete messages for a specific user (eventually add a new permission, delete all messages), I think we shouldn't allow to to other things like mark as read, block/unblock, tagging or even reply as user X. However, this really needs to be thought out, for example to be able to delete a message for a specific user, we would either need a way to act as user X, or be able to delete the message for all participants at once, maybe even both.
- As a admin may want to answer on a thread, that hasn't been sent to him, we could add a statement like "You're currently reading the thread as user X. If you want to answer, please go to /messages/read/%thread.
- If we go with the generic prefix user/%user, we could use a wrapper function for l which determinates if whe're on a admin view and automatically prefixing user/%user. It's not possible to do this in a generic way with messages/%user

I haven't written any code yet nor looked at the other patches, so I'm not 100% sure if what I've written is actually possible. But if we want to do this right, it's pretty complex imho. But I like to do things right if I do them at all, so we should first discuss what we want and what should be allowed/possible. Especially if admin's would have the possibilty to do something as user X, it really gets complicated..

So what is the status of this issue ? Can i test the patch of nbz on last dev ?

The current patch should be more or less useless except as a guideline to getting things back up again - too much has changed since it was last rolled.

My personal plan of action is:

1. Review #348907: Per thread/Multiple thread actions and #376023: #288183 followup: change hook_privatemsg_block_message to work on multiple recipients
2. Re roll #303087: Move "Sent Messages" to privatemsg_filter, Add inbox, improve user filtering.
3. Potentially go back to this one.

Saying that, this feature may not make it for the 1.0 release as there are a few questions on how some parts should work for those with permissions to view all messages.

For what I think is a basic functionality for a system of private message.But i understand the problematic

Should I assume from this discussion that the message/%uid method doesn't work right now?

yes

It might be interresting now to introduce this feature in the core of private message, is not it?

This is still cnw as there are things to be worked on, a patch developed.

I wasw about to atart working on this, but I ran into a small hitch.

  $items['user/%/messages'] = array(
    'title' => 'User messages',
    'page callback'    => 'drupal_get_form',
    'page arguments'   => array('privatemsg_list', 'list', arg(1)),
    'access callback'  => 'privatemsg_user_access',
    'access arguments' => array('read all private messages'),
    'type' => MENU_LOCAL_TASK,
  );

With that menu code and a drupal_set_message(); in privatemsg_list, I was getting an arg of "settings" passed through instead of the actual uid . (3 in the case of user/3/messages)

I cleared the caches and even emptied the table manually, but no change.

'page arguments'   => array('privatemsg_list', 'list', arg(1)),

needs to become >

'page arguments'   => array('privatemsg_list', 'list', 1),

Just to get the ball rolling - not complete at all. Just the menu implementation - and I have left off the inbox/sent messages as this is mostly for admins only and they do not need all the normal goodies that are available through the normal messages/% path.

Questions -
1. Do we want the admin to be able to reply in the conversation? this may be needed to give in coversation warnings etc, or maybe the admin could do that separately.
2. Hiding the normal delete etc buttons as the delete task will not do anything considering that the admin is not in the index for the conversation...

Ok, this patch is now far bigger than I wanted it to be.

1. Adds the menu link and the access permission function as per last patch.
2. Rearranged the thread load function to get the participants of a conversation before loading all the mid's.
3. If a user is not a participant and also has read all permission it now sets a $read_all flag to true.
4. In privatemsg_sql_load, totally got rid of the uid check as it was unneeded.
5. Got rid of the $message['user'] = $account; in _privatemsg_load in addition to getting rid of the account check full stop for this function - its not needed.
6. in privatemsg_sql_messages if the $read_all check is TRUE, then the list here will not be filtered by $uid either, otherwise it will be as is the case for normal users. What this allows is for a person with read all messages permission to still be able to use own account as normal without much hampering.
7. in privatemsg_message_change_delete if no account is presented, then the message is deleted for all users.

Still ToDo:

1. modify privatemsg_delete and privatemsg_delete_submit to have three options: delete, delete for all users and cancel. This will mean not using confirm_form as that is binary only.

considerations:

1. If the user with "read all messages" permission can act on the message (for instance delete), then the permission is badly named. Is there a better alternative name?

I am setting to CNR as even with the ToDo items, the rest should work as expected and input is appreciated.

Hopefully completed patch:

1. Adds the menu link and the access permission function as per last patch.
2. Rearranged the thread load function to get the participants of a conversation before loading all the mid's.
3. If a user is not a participant and also has read all permission it now sets a $read_all flag to true.
4. In privatemsg_sql_load, totally got rid of the uid check as it was unneeded.
5. Got rid of the $message['user'] = $account; in _privatemsg_load in addition to getting rid of the account check full stop for this function - its not needed.
6. in privatemsg_sql_messages if the $read_all check is TRUE, then the list here will not be filtered by $uid either, otherwise it will be as is the case for normal users. What this allows is for a person with read all messages permission to still be able to use own account as normal without much hampering.
7. in privatemsg_message_change_delete if no account is presented, then the message is deleted for all users.
8. Add an option to the delete function to allow to delete for all users.
9. Slightly Fix api documentation in privatemsg.api.php

Please test and comment.

+  // If a user is viewing own account, allow access.
+  if ($account->uid == $uid && privatemsg_user_access('read privatemsg', $account)) {
+    return TRUE;
+  }

Do we want that? (provide a second way to read own messages for a "normal" user)

I haven't yet tested this, but it looks really nice, good work! :)

No idea and good question.

The second question is was I too hasty in changing the privatemsg_load api (and the privatemsg_sql_load function)? There is also a $message['user'] in there which I deleted as I could not see it being used, but maybe that was a placeholder for future functionality?

Just added the following line to the above patch:

drupal_set_message(t('This conversation is being viewed with escalated priviledges and may not be the same as shown to normal users.'));

the changes to _privatemsg_load and privatemsg_sql load are not needed for this patch and also incorrect.

Attached patch removes the changes to _privatemsg_load and privatemsg_sql_load - the user object is needed there for other stuff.

I have also had to change the delete api because the full message is no longer loaded when deleting:

-  module_invoke_all('privatemsg_message_delete', $message, $deleted_by_all);
+  module_invoke_all('privatemsg_message_delete', $pmid, $deleted_by_all);

I did have a question about $deleted_by_all though:

  $result = db_query("SELECT MIN(deleted) AS deleted_by_all FROM {pm_index} WHERE mid = %d", $pmid);
  $deleted = db_fetch_array($result);

  $deleted_by_all = FALSE;
  if ($deleted['deleted_by_all'] == 0) {
    $deleted_by_all = TRUE;
  }

Is that not counter intuitive? TRUE when the message as not been deleted by all, FALSE when it has?

Will this be fine with 6.x-1.0-rc3 ?

Yes, it should.

I must warn you that the patch is a WIP and has not been reviewed by others in order to critique it/point out the flaws. ofcourse, you can be the first to do that.

rerolled and also fixes the buggy $deleted_by_all boolean.

There is a change of api in this patch:

<?php
- module_invoke_all('privatemsg_message_delete', $message, $deleted_by_all);
(some lines later:)
+ module_invoke_all('privatemsg_message_delete', $pmid, $deleted_by_all);

This had to be done to allow messages to be deleted for other users (use case: admin deleting an offensive/abusive message for all users), and in that case we cannot load the whole message object. Instead we we pass fordward the $pmid which is the same for all cases.

rerolled to apply on top of current changes.

Also, removed "$message['user'] = $account;" from _privatemsg_load as that was a hack that is no longer being used.

and rerolled again.

Re-rolled and fixed a few bugs...

- Removed the $read_all param and made $account optional for both the load and messages sql query function.

- Moved the message that you are viewing a thread in read all mode to the privatemsg_view() function and changed it to a warning message. I just thing that warning is more appropriate, but I don't really care...

- I also changed the load function so that it correctly returns FALSE if no message could be loaded. Not necessary for this patch, but it generates ugly empty messages when a message can't be loaded.

Other than that, there are quite a few documentation improvements, which are always nice. I think this is more or less ready...

rerolled with the missing segment from privatemsg_view() - other than that, rtbc IMO.

Subscribe. I'm also testing the patch out currently.

ok, asygo - both me and Berdir think it is ready to go but since both of us also had a hand in crafting it, we will leave it in your capable hands to set to "reviews and tested", so once you are satisfied and have no questions/problems with the patch let us know.

No problem. Even though I'm not 'supposed to', I'm using the patch on a production site. Nothing's broken yet, but I'll wait at least 4 more hours before RTBCing this.

Haven't had any problems. RTBC

sweet just in time.
I'm going to test it out on my dev site.
thanks everyone for your hard work :)

Ok, I have another update for this.

There was a problem when sending a message in read all mode. The problem was that after replying, you can then only see your own message and read all mode isn't triggered anymore. This is solved by forwarding the read_all flag to the form and send functions and if set, the author is added as recipient to all existing messages of that thread.

There are also a few coding style improvements in _privatemsg_send() as I had to touch that code...

A few documentation fixes - no functional changes to Berdir's patch.

Tested and back to rtbc.

Added a $thread['participants'] = array(); to avoid warnings when privatemsg_thread_load() tries to load an invalid thread (can happen if you visit messages/new/uid, for example)

and thus....

Documentation fixes from comment 51 re-added. Other than that, good to go I think.

Great works guys.

needs to be ported to the d7 branch too.

I'm pretty sure I got all changes to DBTNG in this patch.

Thanks for working on this, there are a few missing things:

+    // Load the list of participants.
+    $query = _privatemsg_assemble_query('participants', $thread_id);
+    $participants = db_query($query['query']);
+    $thread['participants'] = array();
+    while ($participant = db_fetch_object($participants)) {
+      $thread['participants'][$participant->uid] = $participant;
+    }
...
-    // Load the list of participants.
-    $query = _privatemsg_assemble_query('participants', $thread_id);
-    foreach ($query->execute() as $participant) {
-      $thread['participants'][$participant->uid] = $participant;
-    }

You should add the same code that is removed. Actually, it can probably be written in a single line:

$thread['participants'] = _privatemsg_assemble_query('participants', $thread_id)->execute()->fetchAllAssoc('uid');

Isn't DBTNG nice :)

+  $query->condition('pmi.thread_id', $threads)
     ->orderBy('pmi.mid', 'ASC');

Minor coding style issue, the condition should be on the second line.

   $deleted = db_query("SELECT MIN(deleted) AS deleted_by_all FROM {pm_index} WHERE mid = %d", $pmid)->fetchField();

Looks like I forgot to change that, since you already touch that part of the code, can you replace that with proper DBTNG code (:mid)?

+    $query_messages = _privatemsg_assemble_query('messages', array($message['thread_id']), NULL);
+    $conversation = db_query($query_messages['query']);
+    while ($result = db_fetch_array($conversation)) {
+      if (!db_query($query, $result['mid'], $message['thread_id'], $message['author']->uid, 0)) {
+        return FALSE;
+      }
+     }
+   }

That part needs to updated too. _privatemsg_assemble_query() returns a SelectQuery object that you can execute() and then loop over it with foreach and instead of executing $query, add the values to the $query object as the other calls in that functions.

If you have questions, please ping me in irc. :)

Thanks for the review, Berdir! I think I addressed all of your comments, but please do go through the patch, just to make sure that I did things in the right way.

+    $thread['participants'] = _privatemsg_assemble_query('participants', $thread_id)->execute()->fetchAllAssoc('uid');;

really minor.. there are two ";" at the end :)

  * @param $fragments
- *  Query fragments array.
+ *   Query fragments array.

Sorry, haven't seen this before. the apidocs need work anyway, but as you touch it, can you remove the whole $fragments part? that is obsolete now...

+    $conversation = $query_messages->execute();
+    while ($result = $conversation->fetchAssoc()) {
+      if (!db_query($query, $result['mid'], $message['thread_id'], $message['author']->uid, 0)) {
+        return FALSE;
+      }

Almost there :)

while the first two lines work, you can write them as "foreach ($conversation->execute() as $result)" (or probably even "foreach ($conversation->execute()->fetchCol() as $mid) and then access it as $mid instead of $result['mid'] but you need to test that.

the inner lines needs to be converted to dbtng, something like:

    $query->values(array(
      'mid' => $mid,
      'thread_id' => $message['thread_id'],
      'uid' => $$message['author']->uid,
      'is_new' => 0,
      'deleted' => 0,
    ));

As $query is already a InsertQuery object to which you can add additional values. However, you need to make sure that the whole part is above the $query->execute() line, I'm not sure about that.

Here is a re-roll with the mentioned changes... This is tested and should be ready...

Sorry, should have said that I was going to be slightly busy. Thanks Berdir!