Messages are not deleted from DB, when user is deleted

[slaFFik]

Here is one of the usecases:
If spammer sends messages and we delete him - his messages are left in DB.

There are some others as well, I believe, when user decided to delete his account by himself for some reason.
As we are currently wiping all of the data of that user, why are we not deleting messages?
From code: because they are unread, but that's a problem in this scenario.
What if we introduce a $force statement and somehow pass it through bp_core_delete_account() to all corresponding places?

[r-a-y]

Nice catch. Surprised we never added a cleanup routine for the Messages component!

Your $force parameter sounds like a good way to fix this. The other problem is querying for all message threads for the deleted user.

01.patch does the following:

• Adds $force parameter to BP_Message_Thread::delete() to make message thread deletion easier.
• Adds some new parameters to BP_Message_Threads::get_all_current_threads_for_user() to query for message threads. In particular, $deleted is the most important one. $fields and $count_total are enhancements to only query for the message thread IDs and to omit the total count.
• Refactors BP_Message_Threads::get_all_current_threads_for_user() to work better with multiple WHERE conditions.
• Includes a unit test.

Patch only handles message thread deletion, and not individual messages at the moment. Will work on that later.

[johnjamesjacoby]

I think the reason we don't purge deleted user data is because it can sometimes intersect into deleting related data from other users being orphaned, or not making sense anymore when 1 person's messages suddenly disappear without a trace for the others in a thread.

Activity, Messages, and Groups are the offenders.

Notifications, Friends, XProfile, and Settings could probably safely be purged.

This is partly why the "Anonymous" type user code exists in some places. To be able to say "this user is no longer around" without eluding as to what might have happened to them.

[r-a-y]

This is partly why the "Anonymous" type user code exists in some places. To be able to say "this user is no longer around" without eluding as to what might have happened to them.

01.patch doesn't handle individual messages, but message threads authored by the deleted user.

I don't think it makes sense to keep message threads authored by the deleted user, however I can see why individual messages should switch over to an anonymous user context.

Maybe we should switch from deleting content to wiping out content. This could be a good feature enhancement. FWIW, I know Reddit changes the user and content to [deleted] whenever some content has been removed.

[r-a-y]

In today's dev chat, we talked about deleting message threads from the removed user from the DB, but keeping individual messages from the removed user intact, but switching the content of the message to [deleted] instead of what was actually written.

Switching out the content turned out to be pretty straightforward. See deleted.patch. Patch also fixes an issue with bp_core_get_core_userdata() not caching invalid users.

[johnjamesjacoby]

Approach in the patch looks OK to me.

For final commit, can we move that false === $userdata ? 0 bit out from the function call? Inline ternaries like that are tricky to read and easily overlooked, at least by my eyes.

🙏

[hnla]

@r-a-y Are you wishing to commit this then given Johns approval or punt to 3.0?

[r-a-y]

In 11611:

Members: Cache invalid users in bp_core_get_core_userdata().

This commit caches non-existent user attempts for the
bp_core_get_core_userdata() function, so subsequent calls for the same
user ID will not ping the database again.

See #7523.

[r-a-y]

In 11612:

Messages: Do not show message content in message threads from deleted users.

If a user no longer exists, do not show the message content and replace
with a [deleted] marker.

See #7523.

[r-a-y]

I've committed deleted.patch for 2.9, which just handles message content from deleted users.

We didn't really come to a consensus on whether we should outright delete message threads authored by the deleted user, so I haven't commited that yet.

Moving to 3.0.

[DJPaul]

I think showing a deleted users’ messages is a good behaviour; it also helps from a audit/moderation perspective (it looks like what we did in the last release might hide the content from a site admin, for example). I’d rather keep the message chain intact.

How about something like, changing the user ID of messages-to-be-deleted to -1, and treating such in the message loop as a “deleted user”, and showing that appropriately in the templates?

[johnjamesjacoby]

I’d prefer we keep everything pristine if possible. What if we leave the user ID as is but add a piece of meta data that indicates the user was “deleted”?

In a multisite instance, a user might be marked as deleted but not actually deleted. Also, changing the user ID reduces the integrity of the data, and then assigns every deleted message to a ghost user.

Not to mention I don’t think our database tables do (so should) allow a negative number in those columns.

A “user_status” meta key could then be used to communicate to the UI where appropriate that the author data of that message is not available in several ways, not just “deleted” but “hidden” or something else.

[DJPaul]

Complexities there are around a join against message meta, and lots of duplicate-value message meta records (depending on volume of deleted messages a user has), and some sort of inverse cache strategy for “this is not actually a user”.

[johnjamesjacoby]

We wouldn't need to join, exactly, because we aren't excluding those messages from threads. All we need to do is say: if the user_status meta key for this message ID equals deleted then show some "Deleted" type user name instead. I don't remember if we lazy load message meta or not, but ideally if we are, there's nothing gained or lost this way.

[r-a-y]

Can I get some feedback to what I mentioned in comment:11?

We didn't really come to a consensus on whether we should outright delete message threads authored by the deleted user, so I haven't commited that yet.

I believe we should delete message threads (not singular messages in existing threads) authored by the deleted user.

---

As for the comments from comment:12 and on, this discusses what was committed in v2.9 - r11612. We can revert or change this behavior if desired.

[johnjamesjacoby]

I would prefer we not delete threads of deleted users, because that will trigger a deletion of replies to those threads that echo through-out the system.

Deleting me shouldn't delete your record of having talked to me, it should only delete my content.

[r-a-y]

Deleting me shouldn't delete your record of having talked to me, it should only delete my content.

Gotcha. Will skip this.

So to close out this ticket, just going to quote what I mentioned in comment:17:

As for the comments from comment:12 and on, this discusses what was committed in v2.9 - r11612. We can revert or change this behavior if desired.

Do we want to revert r11612 or do something else?

[DJPaul]

This is a great ticket which we can review in context of any other GDPR we can make (which is probably what I think the 3.1 release will be about), so I've created a new milestone for that.

[DJPaul]

Milestone renamed