2.0.15: Personal Messages: deleted members invalidate message content

Started by Shambles, August 08, 2018, 02:34:29 PM

Previous topic - Next topic

Shambles

System: SMF 2.0.15

Subsystem: Private Messaging service

Installation: Vanilla installation (2.0.15 no modifications), also on home server (2.0.15 modded), my live forum (2.0.15 modded) and here at SM.org

Priority: low

While checking old private messages here at SM.org I noticed that conversations which include a deleted member invalidate portions of  that conversation, such that only the deleted member's entries exist. The PM content then becomes fragmented and unusable.

Replicated this on my own live forum and also on a vanilla installation of 2.0.15.

This obviously is of very low importance and I haven't checked it on 2.1B4 yet.

Arantor


Shambles

Conversation mode. It's the only one that would show multiple interactions.

Quoteconversations which include a deleted member

Arantor

Figured but I'd figured I'd just check too ;)

Deletion of an account does do quite a number on PMs but IIRC it's not supposed to remove them all.

Let me guess, the ones you're missing are the ones you sent and you don't have 'save a copy of sent items by default' or whatever the option is called set to on.

Shambles

That option is set, always has been.

All other conversations are intact. A typical conversation includes every PM I sent within the message and all responses, self-contained.

Any convo which includes a deleted member (eg, Kat or K@ in this case) only has his messages, none of mine, disjointing the convo.

As stated, it's so trivial and unimportant that it's probably not worth the effort looking any further, but I figured I'd record it in this section. Posterity sucks, so I find :)

Arantor

Then I'm confused.

Yours should be intact - because it's only meant to delete once both the sender and all recipients have their messages marked deleted (in this case, a person deleting their messages marks any they sent as 'deleted by sender' and any they received as not being received because removing the entry from the recipients table is how it does that)

It really should be including yours too if the option to keep them in sent messages was turned on.

Shambles

Would be nice if it could be confirmed.

Thing is, as trivial as I'm trying to paint it, it's easily reproducible.

vii

Quote from: Sh@mbles on August 08, 2018, 06:40:26 PM
Would be nice if it could be confirmed.

Thing is, as trivial as I'm trying to paint it, it's easily reproducible.

I looked at it and tested it, and it does as you described. Looking at the database, it's not marking the message as 'deleted' => 1 in smf_pm_recipients, it's just flat out deleting the entry when the account is deleted.

In Subs-Members.php / deleteMembers():


   // They no longer exist, so we don't know who it was sent to.
   $smcFunc['db_query']('', '
      DELETE FROM {db_prefix}pm_recipients
      WHERE id_member IN ({array_int:users})',
      array(
         'users' => $users,
      )
   );



should probably be changed to:


   // They no longer exist, so we don't know who it was sent to.
   $smcFunc['db_query']('', '
      UPDATE {db_prefix}pm_recipients
      SET deleted = 1
      WHERE id_member IN ({array_int:users})',
      array(
         'users' => $users,
      )
   );



That changes deleteMembers to do what SMF does when the user deletes their version of the conversation - it marks it deleted instead of removing it entirely.

Thinking about it more, this code may need a little more work, namely to check if the receiving user(s) deleted their copy of the convo, in which case all of it should be deleted because the only user(s) not being deleted do not have the convo anymore either.

vii

^ Addendum to above msg (can't seem to modify it?): The proper thing to do I think would be to just take the relevant code from PersonalMessage.php // deleteMessages() function and apply it to the user being deleted, but for all their messages vs specific msgs. It's not enough to just mass-delete their version of all their messages, since it screws up PM convos, like OP pointed out.

It's more work overall for the deleteMembers function but shouldn't be too annoying.

Arantor


vii

Oh wow, I completely missed that.
Anyway, maybe then the other query can be removed, because it's definitely causing unexpected results. If deleteMessages() works properly for all the messages, then that should be all that is needed since it's all that is used when a user deletes their copies normally from the PM center.

That's all I have to say since I don't want to thoroughly pick apart that code (apparently my brief skimming + testing won't be enough here)

Arantor

No, it's called in a different way to that, based on its owner. I'm not 100% sure it's correct.

Advertisement: