This more or less follows what we're doing for message editing. The only significant change is the permissions check.
Depends on D14511
Details
Details
- Reviewers
kamil ashoat - Commits
- rCOMM3ba756707217: [lib] Add a new endpoint for deleting messages
Tested later in the stack by deleting a message and noticing that it gets deleted on two clients.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 451–460 ↗ | (On Diff #47641) | We are issuing the exact same query here twice at the same time. Can this be avoided? |
| 470 ↗ | (On Diff #47641) | This comment is a bit confusing... you suggest that the client should send the ID of the sourceMessage instead, but the check seems to indicate that is exactly what the client did (it sent targetMessageThreadInfo.sourceMessageID, right?) |
| 482 ↗ | (On Diff #47641) | Should we add another check here to prevent the client from deleting a sidebarCreation message? Context here |
| 497–503 ↗ | (On Diff #47641) | What is the effect of deleting the SIDEBAR_SOURCE message like this? Should it be allowed? |
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 451–460 ↗ | (On Diff #47641) | We currently don't have utils for that. We can check if the thread has all permissions from a given set, but in this case we can't reduce the result to simply check if all the permissions are present - we need to have boolean values for all the checks. At first, this doesn't seem complicated to change, but it actually is, because our utils are build over filtering if a set of threads has a permission - so achieving what we need would require either implementing it by hand or significantly changing the existing functions. I don't think its worth it. |
| 470 ↗ | (On Diff #47641) | This is similar to what we're doing for edit messages. This means that when a client tries to delete a message from which a sidebar was created, it should send the original message ID instead of the ID of a message that is present in the sidebar. So by sourceMessage we don't mean SIDEBAR_SOURCE but instead the ID of a message from which a sidebar was created. So in case when the source message is created, targetMessageThreadInfo is the parent thread and not the sidebar. |
| 482 ↗ | (On Diff #47641) | We're checking on line 438 if a message is composable, so the SIDEBAR_CREATION can't be deleted. Do we want to stop allowing the deletion of a message from which a sidebar was created instead? |
| 497–503 ↗ | (On Diff #47641) | Haven't tested it because both the client and the server don't allow it. If it was allowed, the I would expect that the source message in a sidebar will appear as deleted while the same message in the original thread will appear unchanged. Currently, both deleting the message in the parent thread and in the sidebar results in both messages being shown as deleted. |
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 451–460 ↗ | (On Diff #47641) | Personally, I would sooner do it by hand than issue the same query twice |
| 470 ↗ | (On Diff #47641) | Can you clarify the code comment? |
| 482 ↗ | (On Diff #47641) | I'm referring to sidebarCreation. Not sure what SIDEBAR_CREATION is referring to, but composable messages can have sidebarCreation set |
| 497–503 ↗ | (On Diff #47641) | Makes sense |
Comment Actions
Update a comment
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 451–460 ↗ | (On Diff #47641) | Deleting messages is quite rare, so calling a well-tested function twice sounds more sensible than introducing a new logic that has to be tested and maintained. Doing this correctly by hand is quite complicated because e.g. all the logic regarding blocking should be repeated or abstracted https://github.com/CommE2E/comm/blob/7f2d97318b527ce942bee8ad4b0b716c12bbb2a1/keyserver/src/fetchers/thread-permission-fetchers.js#L180-L265. I think that repeating this logic is a lot worse than a repeated call done infrequently. If you feel really strongly about this, I can obviously do it, but I have a strong opinion that it isn't a good idea. I guess that it will take about 4h. |
| 470 ↗ | (On Diff #47641) | Sure! |
| 482 ↗ | (On Diff #47641) | Ah, my mistake. I was thinking about SIDEBAR_SOURCE and CREATE_SIDEBAR message types.
In D14564 I implemented a solution where we still can delete a sidebarCreation, so I don't think we should block deleting these |
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 451–460 ↗ | (On Diff #47641) | Wow, 4 hours?? I'll go ahead and just abstract checkThreadPermission to work for or conditions for you, and time myself |
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 451–460 ↗ | (On Diff #47641) | I don't think the proposed diff can reduce the number of queries. As you can see here: !hasDeleteAllMessagesPermission && !( hasDeleteOwnMessagesPermission && targetMessageInfo.creatorID !== viewer.id ) we aren't simply checking hasDeleteAllMessagesPermission || hasDeleteOwnMessagesPermission. With the util you implemented, we can check if hasDeleteAllMessagesPermission || hasDeleteOwnMessagesPermission, but it isn't useful to determine if a user can delete a message they haven't sent. For example, when a user has only hasDeleteOwnMessagesPermission permission and tries to delete a message from another user, knowing the value of hasDeleteAllMessagesPermission || hasDeleteOwnMessagesPermission won't be useful, as it equals true. |
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 451–460 ↗ | (On Diff #47641) | Can't you just check targetMessageInfo.creatorID !== viewer.id to determine the list of permission checks? |
| keyserver/src/responders/message-responders.js | ||
|---|---|---|
| 451–460 ↗ | (On Diff #47641) | Hmmm... yeah, that should work. Thanks for pointing this out! |