With the new tabs behaviour, all seems to be good apart from on this one page where the read message tab should not show but it does.

It does not show on messages/new, but does on messages/new/user_id where the tab links to messages/view/user_id.

CommentFileSizeAuthor
#9 privatemsg_read_tab2.patch1.22 KBberdir
#7 privatemsg_read_tab.patch1.2 KBnaheemsays

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, it seems that the menu system thinks that the user_id is a thread_id because it's on the same depth, especially as privatemsg_thread_load isn't doing any checking.

I had several ideas to fix it, most of them don't work..

Not working:
- move write new message to userX to the user page. As I'd like to make that page support multiple recipients per url, that doesn't really work..
- Validate the $thread_id... there is no way to tell if $some_int is either a user id or a thread_id, it can be both
- Prefix it with something... not supported by the menu system
- Other things I forgot again... (probably better... )

Working:
1. Move user id a depth deeper, simplest solution: messages/new/to/%uid
2. Change it to use the username instead of the uid, for example: messages/new/Berdir

I am in favor of solution 1, it is simple to fix in hook_menu (change arg parameter for 2 to 3) but I'd like to delay that until the api patch has been committed, as it simplify the changing of the links.

naheemsays’s picture

naheemsays commented

I am in favour of delaying *everything* til that lands...

naheemsays’s picture

naheemsays commented

Another option for user specific messages would to move from messages/new/% to users/%/messages/new - but to keep messages/new as it is?

Or go to messages/%/new to fit in with the messages/% to get to a different users messages.

It could be a good idea to cover this along with #298502: Better access to another user's messages.

naheemsays’s picture

naheemsays commented

Another way would be to add an access check function to the read messages page which would return false if the page was anything other than /messages/view/%

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

@#2
sounds good :)

@#3
As I mentioned, I'd like to keep an option open to do somethink like messages/new/to/uid1+uid2+uid3, so the To-field would get pre-filled with "UserX, UserY, UserZ". Using %user wouldn't allow that anymore. But it's not really important, just thought it would be nice to have the possibility to easily send a message to a defined group of users..

@#4
That could work too. One of my ideas was to put such a check directly inside privatemsg_thread_load, but that is a bad idea, as I want to extend that function to an actual api function which loads all thread stuff (messages, participants, .. that could simplify the view page *a lot*)

naheemsays’s picture

naheemsays commented

Some untested probably psuedo code:


function privatemsg_view_access() {
  if (user_access('read privatemsg') && arg(1) == 'view') {
    return TRUE;
  }
  return false;
}
naheemsays’s picture

naheemsays commented
Title: Read tab should not show on messages/new/user_id » Read tab should not show on messages/new/%
Status: Active » Needs work
StatusFileSize
new privatemsg_read_tab.patch1.2 KB

Attached patch seems to work correctly after clearing the cache.

Just a note, the other things discussed in this issue may also be important, but for this bug, they would just hide it instead of fixing it.

naheemsays’s picture

naheemsays commented
Status: Needs work » Needs review

oops, should be cnr

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new privatemsg_read_tab2.patch1.22 KB

Fixed a a lowercase FALSE, other than that, patch looks good.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Closed (duplicate)

Included patch in #376250-1: Kill disallow_anon().