nextcloud / spreed

πŸ—¨οΈ Nextcloud Talk – chat, video & audio calls for Nextcloud
https://nextcloud.com/talk
GNU Affero General Public License v3.0
1.64k stars 437 forks source link

Reactions of deleted message still persist in database #11332

Open Antreesy opened 10 months ago

Antreesy commented 10 months ago

How to use GitHub


Steps to reproduce

  1. Post a message
  2. Add a reaction to it
  3. Delete a message

Expected behaviour

Server response should be:

{
  "id": 122,
  "message": "Message deleted by you",
  "messageType": "comment_deleted",
  "reactions": {},
},
{
  "id": 123,
  "message": "Reaction deleted by author",
  "parent": {
    "id": 122,
  }
},
{
  "id": 124,
  "message": "You deleted a message",
  "systemMessage": "message_deleted",
  "parent": {
    "id": 122,
  }
},

Actual behaviour

{
  "id": 122,
  "message": "Message deleted by you",
  "messageType": "comment_deleted",
  "reactions": {},
  "reactionsSelf": [
    "\ud83d\udc4d"
  ]
},
{
  "id": 123,
  "message": "\ud83d\udc4d",
  "systemMessage": "reaction",
  "parent": {
    "id": 122,
  }
},
{
  "id": 124,
  "message": "You deleted a message",
  "systemMessage": "message_deleted",
  "parent": {
    "id": 122,
  }
},
SystemKeeper commented 10 months ago

Similar issue with message expiration at https://github.com/nextcloud/spreed/issues/11298

nickvergessen commented 10 months ago

Will have a look, but not sure this is easily fixable and worth it. If we have to cascade the deletion, deleting a message could result in many hidden system messages, even worse on expiration where we don't even track which messages got deleted

nickvergessen commented 10 months ago

If a single "message deleted" system message is good enough, we could add a single query like:

DELETE FROM oc_comments WHERE (verb = 'reaction' OR verb = 'reaction_deleted') AND parent_id = X;

However the system message of a reaction_revoked is much harder to delete, as it would be verb = 'system' AND message = '{"message":"reaction_revoked","parameters":{"message":REACTION_MESSAGE_ID}}' so requiring a list of IDs before hitting the query and potentially having a huge result, or we simply delete all system messages that are "child" to the deleted message. That would delete edit traces etc also for the future, so not sure we'd like that.