status-im / status-desktop

Status Desktop client made in Nim & QML
https://status.app
Mozilla Public License 2.0
296 stars 79 forks source link

Clarify acceptance criteria for deleted messages for banned members #13707

Open mprakhov opened 8 months ago

mprakhov commented 8 months ago

Description

Remove and Delete are defined quite similarly, but the main difference between them is that delete means erase (i.e. rendered nonexistent or unrecoverable), while remove denotes take away and set aside (but kept in existence).

The current implementation is:

  1. If the user was banned by deleting all messages, he will be added to the banned members in the community description. All clients will receive an updated community description and if they see a new banned member - delete (physically from their db and UI) all community messages of this member. Old messages of this banned member will be ignored while the user is banned

Cons:

  1. If the user was banned and the owner wants to delete specific messages of the user, this msg will be removed (marked as deleted in DB, and in the UI we'll see instead msg -> Message was deleted by ... P.S. I think we need to rename in UI "deleted" text here to "removed"). Each removed message will be sent to other clients for they to mark it as removed too

Cons:

Need to clarify if such behavior is ok and if not, create acceptance criteria and implement expected one

mprakhov commented 8 months ago

related to EPIC https://github.com/status-im/status-desktop/issues/13034

jrainville commented 7 months ago

This could be just an edge case so that we don't really need to worry about it? In theory, if you ban someone and delete their messages, they probably posted some pretty egregious stuff, so the chances you would unban them, it pretty low.

Then, if we assume that it's possible, then, only new members and re-imported accounts have a chance to fetch the deleted messages, and in theory only from the past 30 days. So again, it's an edge case.

So maybe we can live with it?

Otherwise, we might want to go with a simple solution, like just broadcasting to the community topic when we ban and delete the messages. That message would say "I deleted that person's messages at timestamp X". Then, since the only way to recuperate the banned members' messages is from store nodes, we can hope that you would also fetch the special message above. If you do, you know that you need to ignore all messages from before X.

Of course, this has a chance of not working if the special message is not fetched correctly, but then again, there is a chance to not fetch the banned members messages either, so it's fighting an edge case with an edge case :smile:

I just don't want to over-engineer a solution for such an edge case

cammellos commented 7 months ago

This could be just an edge case so that we don't really need to worry about it? In theory, if you ban someone and delete their messages, they probably posted some pretty egregious stuff, so the chances you would unban them, it pretty low.

Then, if we assume that it's possible, then, only new members and re-imported accounts have a chance to fetch the deleted messages, and in theory only from the past 30 days. So again, it's an edge case.

So maybe we can live with it?

Otherwise, we might want to go with a simple solution, like just broadcasting to the community topic when we ban and delete the messages. That message would say "I deleted that person's messages at timestamp X". Then, since the only way to recuperate the banned members' messages is from store nodes, we can hope that you would also fetch the special message above. If you do, you know that you need to ignore all messages from before X.

Of course, this has a chance of not working if the special message is not fetched correctly, but then again, there is a chance to not fetch the banned members messages either, so it's fighting an edge case with an edge case 😄

I just don't want to over-engineer a solution for such an edge case

I think the "correct" solution would be to:

1) Keep history of banned/unbanned users in community description for the previous 30 days (as long as messages are fetchable from store nodes). 2) Torrent owner will make sure that messages that were sent while the user is banned are no in the history.

So, you could have something like:

BannedMembers: {
  pk1: [{type:banned, clock: 1000}, {type: unbanned, clock: 1200}] // a banned member who has been  banned at 1000 and unbanned at 1200,
  pk2: [{type: banned, clock: 1000}]
 }

. Any member who has been unbanned for the past 30 days can be purged from this That should cover the case above, and solve the problem as far as I can see.

I don't think this is worth implementing now, certainly not at this stage of the app, since it's an edge case, and maybe never to be honest, I think we can easily live with it.

mprakhov commented 7 months ago

Updating to the topic: status-desktop has an option right now to delete a specific user message (this user doesn't need to be banned) The deleted message will have the same issue as if we delete all - it will be restored if the user leaves and joins the community again. Note: as a temporary solution this delete can be replaced by Remove.

P.S. As far as I understood, not critical for incoming 2.29 release so can be postponed

jrainville commented 7 months ago

I don't think this is worth implementing now, certainly not at this stage of the app, since it's an edge case, and maybe never to be honest, I think we can easily live with it.

Yeah, that was my opinion too. Your solution is indeed a good idea to solve the issue, but it's such an edge case that it doesn't feel worth spending time on it.

I will put this in 2.30 for now