Skip to:
Content

Opened 16 months ago

Closed 4 months ago

Last modified 2 months ago

#7096 closed defect (bug) (fixed)

Messages: member page messages are queried for the loggedin user

Reported by: Offereins Owned by: dcavins
Milestone: 2.9 Priority: normal
Severity: normal Version:
Component: Messages Keywords: has-patch
Cc: lmoffereins@…, dcavins

Description

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?

Attachments (4)

7096.01.diff (8.4 KB) - added by dcavins 16 months ago.
Use the displayed user rather than the logged in user to build the messages loop.
7096.02.diff (20.1 KB) - added by dcavins 6 months ago.
More complete code review.
7096.diff (3.6 KB) - added by boonebgorges 2 months ago.
7096.2.diff (9.8 KB) - added by boonebgorges 2 months ago.

Download all attachments as: .zip

Change History (37)

#1 @Offereins
16 months ago

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

#2 @dcavins
16 months ago

  • Keywords has-patch dev-feedback added

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
16 months ago

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

#3 @Offereins
16 months ago

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

#4 @r-a-y
16 months ago

#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.

#5 @DJPaul
15 months ago

  • Milestone changed from Awaiting Review to 2.7

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?

#6 @Offereins
15 months ago

@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?

#7 @dcavins
15 months ago

  • Keywords has-patch dev-feedback removed
  • Owner set to dcavins
  • Status changed from new to assigned

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.

#8 @imath
15 months ago

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.

#9 @mercime
12 months ago

  • Milestone changed from 2.7 to Future Release

Have to punt this.

@dcavins
6 months ago

More complete code review.

#10 @dcavins
6 months ago

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!

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


6 months ago

#12 @dcavins
6 months ago

  • Cc dcavins added

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


5 months ago

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


4 months ago

#15 @r-a-y
4 months ago

  • Keywords has-patch added
  • Milestone changed from Future Release to Under Consideration

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!

#16 @Offereins
4 months ago

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

#17 @r-a-y
4 months ago

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

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

#18 @dcavins
4 months ago

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. :)

#19 @dcavins
4 months ago

@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

#20 @r-a-y
4 months ago

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.

#21 @dcavins
4 months ago

@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?

#22 @r-a-y
4 months ago

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

#23 @dcavins
4 months ago

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.

#24 @dcavins
4 months ago

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.

#25 @dcavins
4 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#26 @r-a-y
3 months ago

  • Milestone changed from Under Consideration to 2.9

#27 @boonebgorges
2 months ago

[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 :)

@boonebgorges
2 months ago

#28 @dcavins
2 months ago

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!

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


2 months ago

@boonebgorges
2 months ago

#30 @boonebgorges
2 months ago

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?

#31 @dcavins
2 months ago

@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!

#32 @boonebgorges
2 months ago

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.

#33 @boonebgorges
2 months ago

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.

Note: See TracTickets for help on using tickets.