Hello, I've recently installed this module and it's great, but I've changed the behaviour of the 'user/%/messages' menu link. I think that it's natural that users could view his own messages in this link, so I've patched the code to get it working. The patch works fine and I'm using it in my little web site. I've very little experience in Drupal development, so I don't know if my approach is correct.

It's my first patch too, sorry if there are many mistakes.

Kind regards.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

harrrrrrr’s picture

this makes sense

Berdir’s picture

Status: Active » Needs review

The problem is that the page is not a full replacement for /messages, for example, there is no tag or blocked user management. The sole purpose for these pages is to view messages of other users.

6.x-2.x has a feature that allows to move it below the user profile but that's very complicated and I'm not sure about porting it.

For example, there are many links and redirections which point to /messages. Giving users two separate places is IMHO confusing.

Status: Needs review » Needs work

The last submitted patch, privatemsg-view_own_messages.patch, failed testing.

Berdir’s picture

Your patch probably contains windows line endings, the testbot doesn't like that.

igalarza’s picture

Hello Berdir, the patch does a redirect from /user/%/messages to /messages when you are viewing your own user, so it don't use two different places for messages.

I think that the line endings are right now.

Regards

Berdir’s picture

Status: Needs work » Needs review

Remember to set the Status to needs review when posting patches.

igalarza’s picture

ok, sorry. And thank you for your patience.

Berdir’s picture

Status: Needs review » Needs work

Ah, didn't see the redirect, sorry. Then I'm fine with this.

Lot's of nitpicking about coding style and how to simplify the function below. It looks like a lot, but it's not, don't worry. I'm just picky about the coding standard and I've been very detailed. In fact, with my suggestions, you should be able to reduce the lines of code in that function to 4-5 ;)

+++ b/privatemsg.moduleundefined
@@ -349,6 +349,36 @@ function privatemsg_user_access($permission = 'read privatemsg', $account = NULL
+ * Privatemsg wrapper for user_access and the user/%/messages menu options,
+ * allow access to the own profile, otherwise calls to privatemsg_user_access

The perfect first sentence in a docblock should be as short as possible, start with a verb in 3rd person and if possible, below 80 characters.

Suggestion:

Checks access for the messages tab on the user profile.

You can then add a more detailed description below that, after an empty line.

+++ b/privatemsg.moduleundefined
@@ -349,6 +349,36 @@ function privatemsg_user_access($permission = 'read privatemsg', $account = NULL
+ * @param $permission
+ *   Permission string, defaults to read privatemsg

Nope, it doesn't :) Since we are using this function only in this case, we can save the arguments completely (both), and just hardcode a privatemsg_user_access('read all private messages') at the end.

+++ b/privatemsg.moduleundefined
@@ -349,6 +349,36 @@ function privatemsg_user_access($permission = 'read privatemsg', $account = NULL
+ * @return
+ *   TRUE if user has access, FALSE if not

This line should end with a .

+++ b/privatemsg.moduleundefined
@@ -349,6 +349,36 @@ function privatemsg_user_access($permission = 'read privatemsg', $account = NULL
+ *
+ * @ingroup api

This also isn't really an API function, so remove this.

+++ b/privatemsg.moduleundefined
@@ -349,6 +349,36 @@ function privatemsg_user_access($permission = 'read privatemsg', $account = NULL
+function privatemsg_ownuser_access ($permission = NULL, $account = NULL) {

No space before the (. Also, I'd suggest to actually pass in the account object of the user we're visiting. Then the access check below becomes really simply. Because all you need is a if ($user->uid && $user->uid == $account->uid). To pass in the account, change the access arguments to array(1).

+++ b/privatemsg.moduleundefined
@@ -349,6 +349,36 @@ function privatemsg_user_access($permission = 'read privatemsg', $account = NULL
+  if (!$account->uid) { // Disallow anonymous access, regardless of permissions
+    return FALSE ;

Comments should always be on their own line, either within or above the if, whatever you prefer. And also end with a .

+++ b/privatemsg.moduleundefined
@@ -349,6 +349,36 @@ function privatemsg_user_access($permission = 'read privatemsg', $account = NULL
+  else

Either add {} (they should always be used, even for single lines, to prevent confusion or remove the else completely. Because if the mentioned if statement above is true, it will return and not continue in the function anyway.

+++ b/privatemsg.pages.incundefined
@@ -150,6 +150,11 @@ function privatemsg_list_page($argument = 'list', $uid = NULL) {
+  // redirect to 'messages' if it's the own profile
+  else if ((int)$uid > 0 && $uid == $user->uid) {
+      drupal_goto('messages') ;
+      return ;

Same stuff here, start comment with an uppercase letter and end with a .

Also, no space before the semicolon.

igalarza’s picture

Status: Needs work » Needs review
FileSize
1.94 KB

Hello again and thanks for your corrections. I'm not used to the drupal code standard and I'm very happy to have this detailed feedback of my little piece of code.

I've tried to follow all your advices and there is the result; maybe my patch still have mistakes, english is not my mother language and sometimes is hard to express what I want to explain and to understand all the details.

I hope that everything is correct now, but I would be happy to change anything you want if remains any problem. Of course you can change yourself anything in the patch if you prefer it.

Kind regards.

Status: Needs review » Needs work

The last submitted patch, privatemsg-1443172-9.patch, failed testing.

igalarza’s picture

Status: Needs work » Needs review
FileSize
1.93 KB

Sorry, a stupid mistake.

I think is right now.

Status: Needs review » Needs work

The last submitted patch, privatemsg-1443172-9.patch, failed testing.

igalarza’s picture

Status: Needs work » Needs review
FileSize
1.93 KB

Wrong patch, there is the correct one.

BeaPower’s picture

sub

mstef’s picture

This needs to be fixed in 7.x-1.x as well.

I'll write a patch

mstef’s picture

Title: View own messages in 'user/%/messages' » git
FileSize
5.45 KB

Patch here to fix the access callback for user tabs on the profile. It will allow users to access it if they're viewing themselves, or for anyone, if they have "read all messages' perm.

I also removed privatemsg_list_page() because it's completely unnecessary. %user should be used in hook_menu() to load and pass the user object. All other menu callbacks can call drupal_get_form('privatemsg_list') directly, and bypass the need for that function.

PS: This is for 7.x-1.x.

Status: Needs review » Needs work

The last submitted patch, privatemsg-fix-user-tab-access-1443172-7.x-1.x.patch, failed testing.

mstef’s picture

Title: git » View own messages in 'user/%/messages
Status: Needs work » Needs review

Didn't mean to change the title

-Mania-’s picture

Any progress with this?

tymofii.pashkevych’s picture

I have resolved this issue in my custom module

/**
 * Implements hook_menu().
 */
function YOURMODULE_menu() {
$items['user/%user/messages'] = array(
    'title' => 'Messages',
    'page callback'    => 'YOURMODULE_list_page',
    'page arguments'   => array('list', 1),
    'access arguments' => array('read privatemsg'),
    'type' => MENU_LOCAL_TASK,
  );
return $items;
}
function YOURMODULE_list_page ($argument = 'list', $account) {
  global $user;
  module_load_include('inc', 'privatemsg', 'privatemsg.pages');
  if($account->uid > 0 && $account->uid != $user->uid) {
    if (!privatemsg_user_access('read all private messages')) {
      return MENU_ACCESS_DENIED;
    }
  }
  return drupal_get_form('privatemsg_list', $argument, $account);
}
mstef’s picture

Version: 7.x-2.x-dev » 7.x-1.x-dev

Let's try again with the correct version for my patch

mstef’s picture

Status: Needs review » Needs work

The last submitted patch, privatemsg-fix-user-tab-access-1443172-7.x-1.x-22.patch, failed testing.

Loac’s picture

Thanks to tymofii.pashkevych. #20 Good solution.

ptmkenny’s picture

Potential solution (patch) here: https://drupal.org/node/1772236

ptmkenny’s picture

ptmkenny’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev
Priority: Minor » Normal
Status: Closed (duplicate) » Needs review
FileSize
5.49 KB
ptmkenny’s picture

This patch works for user/UID/messages but is there a way to make it go to the sent box like user/UID/messages/sent as well?

Alcaparra’s picture

+1

jonhattan’s picture

Fixed coding standards.

jonhattan’s picture

Tets will fail because of #2927068: Tests are broken. Added a convenience patch with the fix in 2927068

The last submitted patch, 30: privatemsg-1443172-30.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 30: privatemsg-1443172-30.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

ivnish’s picture

Status: Needs review » Closed (outdated)
andypost’s picture

Status: Closed (outdated) » Needs review

D7 is not yet outdated