Messages: member page messages are queried for the loggedin user

[Offereins]

An internal ​@todo already points in the right direction: moderators are shown their own messages when viewing another member's messages pages (inbox, compose, counts). As this is unintentional, can we fix this to display the displayed user's messages?

[Offereins]

Perhaps we can wrap the $user_id declaration within a cap check like 'bp_view_others_messages' which would defer to 'bp_moderate'.

[dcavins]

Thanks for creating this ticket.

I think this is almost the same issue as the underlying issue in #7023: referring to the logged-in user when we should be using the displayed user to determine what's up. I've attached a first patch, but would like @r-a-y's input, since he's worked on the messages component recently.

[dcavins]

Use the displayed user rather than the logged in user to build the messages loop.

[Offereins]

@dcavins Note that in bp-messages/bp-messages-star.php at least a few template tags also default to the loggedin user.

[r-a-y]

#6493 was marked as a duplicate.

---

@Offereins is right about bp-messages-star.php. Also, there are a bunch of logged-in user stuff in the message class methods as well.

[DJPaul]

This patch needs careful checking because it looks like a giant search/replace has happened. :)

What's the root problem with this ticket? As a Admin, if I go to another user's profile and view their Private Messages, I instead see my own Private Messages?

[Offereins]

@DJPaul Exactly that is the problem. Though, now you can ask: what then is intentional? Viewing private messages of other members (which is suggested here and partly patched) or not loading the Messages component/screens in other displayed user's pages at all?

[dcavins]

Ha ha, it wasn't an _automated_ search and replace, and yes, there's more work to do. Basically, this is like the recent fix to the notifications component, which also didn't work well for site admins (only the owner and site admins can view these screens--showing the admin's messages on another user's profile is just weird.)

I'm planning on finishing up these changes and getting them in very early in the 2.7 cycle.

[imath]

I meet this bug today! I see it's in good hand :)

My 2 cents: the compose and notices screen should not be displayed when the admin is on another user's profile.

[mercime]

Have to punt this.

[dcavins]

More complete code review.

[dcavins]

Hello all, I've just added an updated patch that address the read/unread and starring logic as well as fixing some other issues:

• If an admin visits a user's message thread, the thread should not be marked read. (Nor should the notifications be removed.)
• Fix cache deletion to use the thread's recipient.
• Target "unread counts" in the user nav (displayed user) and in the admin bar (logged-in user).
• Changes to logic in bp_the_thread_recipients_list()
• Compose link should only show on logged in user profile.
• Notices link should only show for logged in user profile that is moderator.
• Many changes to which user ID to use.

Thanks for your feedback!

[r-a-y]

02.diff looks good, @dcavins!

Some minor things:

• bp-messages-actions.php - Can the bp_current_user_can( 'bp_moderate' ) check be added to the messages_check_thread_access() function instead?
• bp-messages-template.php, /bp-templates/bp-legacy/buddypress/members/single/messages/single.php - The changes to bp_get_thread_recipients_list() should be done separately. I swear WordPress has a function to separate an array into a comma-delimited string with the and string added to the last item. However, I can't find it at the moment!

[Offereins]

@r-a-y You're looking for wp_sprintf_l(), see ​developer.wordpress.org.

[r-a-y]

You're looking for wp_sprintf_l(), see developer.wordpress.org.

Thanks for the detective work, @Offereins. I knew I wasn't crazy! :)

[dcavins]

I'm just glad someone else built a hinky list formatter, so I don't have to rely on the hinky one I made up, ha ha. :)

[dcavins]

@r-a-y: I'm looking at this patch again today, and I like your idea of adding the bp_moderate check in messages_check_thread_access(), but it will change the return value of messages_check_thread_access(). (Which I think is why I left the bp_moderate check outside of it--because I didn't quite know what to do with it.)

messages_check_thread_access() actually returns the ID of the record in the bp_messages_recipients, so what would we return for a site admin who's not in the thread?

For instance, if I run messages_check_thread_access( 13, 1 ), this row is found:

id    user_id   thread_id	unread_count	sender_only	is_deleted
29    1         13	            0	          0	            0

and the integer 29 is the return value.

Do you think anyone is using this function to fetch that ID? Would it be safe to return true for admins?

Thanks!

-David

[r-a-y]

Nice catch, @dcavins, about the return value of messages_check_thread_access(). Although, it is quite possible that someone (ugh!) could be using this to grab the message ID, I would personally change the return value to a boolean.

Internally, we use the function to check for a falsey value only. However, since we like to backpat all the time, I would ask a lead dev what they think.

[dcavins]

@r-a-y: I'm thinking the best approach is to commit the changes as they are, and change the behavior of messages_check_thread_access() separately as an enhancement (and also so these other useful functionality changes don't get hung up on a technicality.) Does that seem OK to you?

[r-a-y]

Sure, sounds good! Just make sure to commit the bp_get_thread_recipients_list() stuff separately.

[dcavins]

In 11576:

Improve message recipient list string handling.

WordPress 2.5 introduced wp_sprintf_l() which is built to concatenate
arrays into natural language lists (and is meant to be translation
friendly). This commit uses that function to build the recipient list
text for single messages, like “Conversation between Three, Four, Two,
and you.”

Props Offereins.

See #7096.

[dcavins]

In 11577:

Messages: Show Compose and Notices screens only on "my profile."

When a site administrator visits another user’s messages, she should
not see the “Notices” or “Compose” navigation items or be able to visit
those screens. These should only be available when bp_is_my_profile()
is true.

See #7096.

[dcavins]

In 11578:

Show displayed user's messages on messages screens.

When a site administrator visits another user’s messages, she is shown
her own messages because the display logic for messages refers to the
logged-in user rather than the displayed user. This changeset updates
the logic to allow the site admin to view and interact with another
user’s messages.

Fixes #7096.

[boonebgorges]

[11578] broke message notifications. The link generated in messages_format_notifications() should always be relative to the logged-in user. Now it's relative to the displayed user, and when there's no displayed user, it simply ends up returning a malformed link, such as href="messages/view/55" in the case of a single message.

Simply adding a $user_id parameter (which would fall back on bp_displayed_user_id()) would allow for an easy fix in BP. That being said, the change in [11578] could potentially affect third-party implementations that are using bp_get_message_thread_view_link() in non-standard ways. So, the conservative strategy would be to (a) add the $user_id param, but have it default to bp_loggedin_user_id(); then, we pass the displayed user ID in the context of the loop.

I'll attach a patch for review by people who've thought about this more :)

[dcavins]

Yes, I agree that Boone's fix is the simplest answer and is good for backward compatibility.

As I understand it:
The change to bp-messages/bp-messages-template.php restores the desired behavior to the links generated in the notifications list.

That change breaks the new behavior of the links in a message list (using the displayed user as the link base) when an admin is viewing another member's messages screen, though, and is fixed by specifying the user ID in bp-templates/bp-legacy/buddypress/members/single/messages/messages-loop.php.

@boonebgorges Thanks for fixing the problem I introduced!

[boonebgorges]

7096.2.diff​ gives the suggested treatment to the functions that output a link that might conceivably be used in a different context: bp_message_thread_view_link(), bp_message_thread_delete_link(), bp_the_message_thread_mark_unread_url(), bp_the_message_thread_mark_read_url().

URLs that are used for redirects in screen/action functions can be hardcoded to reference the displayed user, I think. The same goes for the form_action.

I wasn't sure whether to touch notice-related URLs. I think notices can appear in any context, even when there's no displayed user. Is there a situation where notice-related links should *not* be generated relative to the logged-in user? IMO these parts of [11578] should probably be rolled back.

@dcavins Can you please give a sanity check to the above?

[dcavins]

@boonebgorges 7096.2 looks like it will prevent some (many?) issues caused by my earlier change. Thanks for finding them during the beta! I also prefer changing the function signature by adding a user ID. I think that will make these functions more useful in more contexts.

Thanks!

[boonebgorges]

In 11643:

Ensure that links for managing sitewide notices are specific to logged-in user.

These links were incorrectly converted to the displayed user as
part of the work in [11578]. Notices are always relative to the
logged-in user, and can be displayed anywhere on a site - regardless
of the displayed user (or lack thereof).

See #7096.

[boonebgorges]

In 11644:

Restore default behavior of message link generating functions.

Prior to [11578], functions for generating links in the context of
messages (Mark Read, Delete, etc) were generated relative to the
logged-in user. This made it impossible for an administrator to
manage another user's messages. [11578] changed the default behavior
of these functions to reference the displayed user. But this change
breaks backward compatibility for functions expecting the prior
behavior, including BP's own message notification format functions.

We work around the limitation by introducing $user_id parameters
to the functions in question. $user_id defaults to the logged-in
user (for backward compatibilty), and the bp-legacy template passes
in the displayed user ID as appropriate.

Fixes #7096.