Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 3 years ago

#7235 closed enhancement (fixed)

Allow for message threads to be deleted for users other than the current user

Reported by: jdgrimes Owned by: boonebgorges
Milestone: 2.7 Priority: normal
Severity: normal Version:
Component: Messages Keywords: has-patch
Cc:

Description

Message threads are deleted on a per-user basis, and then only removed from the database completely once all recipients have marked the thread as deleted. Currently message threads can only be deleted for the user who is the current user, because the use of bp_loggedin_user_id() is hard-coded in BP_Messages_Thread::delete(). There is an accompanying todo comment suggesting that this should be fixed. As I just ran into an issue with this in the context of unit testing some code that uses the messages API, I thought I'd work up a patch.

Attachments (3)

7235.diff (4.8 KB) - added by jdgrimes 3 years ago.
7235.2.diff (5.3 KB) - added by jdgrimes 3 years ago.
Add @since docs for new parameter
7235.3.diff (5.4 KB) - added by jdgrimes 3 years ago.
Slightly better @since explanations for new $user_id parameter

Download all attachments as: .zip

Change History (9)

@jdgrimes
3 years ago

#1 @jdgrimes
3 years ago

  • Keywords has-patch added

7235.diff is a first pass. I've added an optional $user_id parameter to BP_Messages_Thread::delete() and messages_delete_thread(), and to action hooks that they call where I felt that it was appropriate.

While working on this I realized that there could be back-compat issues with any functions hooked to those actions that will be assuming that the messages are being deleted for the logged-in user. I guess the only way to work around that would be to hack the current user when calling those actions, and then reset it afterward. Possibly then we'd also want to deprecate them and introduce new actions that preserve the current user when they are called. I guess this really could be an insurmountable obstacle to ever fixing this without introducing new functions with new actions.

#2 @DJPaul
3 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from Awaiting Review to 2.7

Hi @jdgrimes, thanks for the patch -- really good work! 👏

Your point about the risk of change in having these functions no longer rely on the logged-in user is a good one. I am undecided how much I think we should care. A new pair of actions is not going to significantly complicate the function, and I guess we could even move the current actions into /bp-core/deprecated/ and hook them to the new actions (to keep the function neat and tidy). But the current actions have pretty good/logical names.

I think on balance, delete is our lowest level function, and should never have been relying on the global logged-in user at all. Upstream in messages_delete_thread etc is where that should have been handled (and where it is now, in your patch!).

Let's await a second opinion but I think we should probably just commit as-is and not worry about the potential break.

#3 @DJPaul
3 years ago

Oh, I suppose for bonus points, we should add a @since 2.7.0 line to the functions' docblocks explaining we added a new default parameter. :)

@jdgrimes
3 years ago

Add @since docs for new parameter

@jdgrimes
3 years ago

Slightly better @since explanations for new $user_id parameter

#4 @boonebgorges
3 years ago

  • Keywords 2nd-opinion removed

Let's await a second opinion but I think we should probably just commit as-is and not worry about the potential break.

I agree with this. The current behavior doesn't make much sense. Let's make sure that the change is documented in a bit more detail in the hook docblock - maybe a separate paragraph in the long description. We might also prepare a bpdevel post for near the end of the dev cycle to explain some breaking changes (this and the unit test factory come to mind).

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


3 years ago

#6 @boonebgorges
3 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 11153:

Accept a $user_id parameter in messages_delete_thread().

Previously, this stack assumed the logged-in user, which made it very
difficult to delete threads for arbitrary users without touching the
database directly.

Props jdgrimes.
Fixes #7235.

Note: See TracTickets for help on using tickets.