From c017c25d0f29075160229c56f208762ecfeaaded Mon Sep 17 00:00:00 2001 From: spaced4ndy <8711996+spaced4ndy@users.noreply.github.com> Date: Mon, 25 May 2026 10:43:36 +0000 Subject: [PATCH] core, ui: member full delete with messages (#6994) --- apps/ios/Shared/Model/ChatModel.swift | 67 ++++++++++---- apps/ios/SimpleXChat/ChatTypes.swift | 4 +- .../chat/simplex/common/model/ChatModel.kt | 65 ++++++++++---- .../chat/simplex/common/model/SimpleXAPI.kt | 3 +- .../2026-05-20-member-deletion-fulldelete.md | 73 ++++++++++++++++ src/Simplex/Chat/Library/Commands.hs | 10 ++- src/Simplex/Chat/Library/Internal.hs | 31 ++++--- src/Simplex/Chat/Library/Subscriber.hs | 19 ++-- src/Simplex/Chat/Store/Messages.hs | 87 +++++++++++++------ .../SQLite/Migrations/chat_query_plans.txt | 45 ++++++---- tests/ChatTests/Groups.hs | 63 +++++++++++--- 11 files changed, 352 insertions(+), 115 deletions(-) create mode 100644 plans/2026-05-20-member-deletion-fulldelete.md diff --git a/apps/ios/Shared/Model/ChatModel.swift b/apps/ios/Shared/Model/ChatModel.swift index a1d28b8e22..111dff382a 100644 --- a/apps/ios/Shared/Model/ChatModel.swift +++ b/apps/ios/Shared/Model/ChatModel.swift @@ -812,32 +812,63 @@ final class ChatModel: ObservableObject { } func removeMemberItems(_ removedMember: GroupMember, byMember: GroupMember, _ groupInfo: GroupInfo) { + // Mirrors backend groupFeatureMemberAllowed: fullDelete may be role-gated in business groups. + let fullDeletePref = groupInfo.fullGroupPreferences.fullDelete + let fullDelete = fullDeletePref.on + && byMember.memberRole >= (fullDeletePref.role ?? .observer) if chatId == groupInfo.id { - for i in 0..= 0 { + let item = im.reversedChatItems[i] + if isRemovedMemberItem(item) { + if item.isRcvNew { + unreadCollector.changeUnreadCounter(groupInfo.id, by: -1, unreadMentions: item.meta.userMention ? -1 : 0) + } + if item.isActiveReport { + decreaseGroupReportsCounter(groupInfo.id) + } + VoiceItemState.stopVoiceInChatView(cInfo, item) + removed.append((item.id, i, item.isRcvNew)) + im.reversedChatItems.remove(at: i) + } + i -= 1 + } + if !removed.isEmpty { + im.chatState.itemsRemoved(removed.reversed(), im.reversedChatItems.reversed()) + } + } else { + for i in 0.. 0 { + let preview = chat.chatItems[0] + if isRemovedMemberItem(preview) { + if fullDelete { + chat.chatItems = [ChatItem.deletedItemDummy()] + } else if let updatedItem = markedUpdatedItem(preview) { + chat.chatItems = [updatedItem] } } - } else if let chat = getChat(groupInfo.id), - chat.chatItems.count > 0, - let updatedItem = removedUpdatedItem(chat.chatItems[0]) { - chat.chatItems = [updatedItem] } - func removedUpdatedItem(_ item: ChatItem) -> ChatItem? { - let newContent: CIContent - if case .groupSnd = item.chatDir, removedMember.groupMemberId == groupInfo.membership.groupMemberId { - newContent = .sndModerated - } else if case let .groupRcv(groupMember) = item.chatDir, groupMember.groupMemberId == removedMember.groupMemberId { - newContent = .rcvModerated - } else { - return nil + func isRemovedMemberItem(_ item: ChatItem) -> Bool { + switch item.chatDir { + case .groupSnd: return removedMember.groupMemberId == groupInfo.membership.groupMemberId + case let .groupRcv(groupMember): return groupMember.groupMemberId == removedMember.groupMemberId + default: return false } + } + + func markedUpdatedItem(_ item: ChatItem) -> ChatItem? { + guard isRemovedMemberItem(item) else { return nil } var updatedItem = item updatedItem.meta.itemDeleted = .moderated(deletedTs: Date.now, byGroupMember: byMember) - if groupInfo.fullGroupPreferences.fullDelete.on { - updatedItem.content = newContent - } if item.isActiveReport { decreaseGroupReportsCounter(groupInfo.id) } diff --git a/apps/ios/SimpleXChat/ChatTypes.swift b/apps/ios/SimpleXChat/ChatTypes.swift index 594f90c4e4..b7372bf6b7 100644 --- a/apps/ios/SimpleXChat/ChatTypes.swift +++ b/apps/ios/SimpleXChat/ChatTypes.swift @@ -1358,6 +1358,7 @@ public func toGroupPreferences(_ fullPreferences: FullGroupPreferences) -> Group public struct GroupPreference: Codable, Equatable, Hashable { public var enable: GroupFeatureEnabled + public var role: GroupMemberRole? public var on: Bool { enable == .on @@ -1375,8 +1376,9 @@ public struct GroupPreference: Codable, Equatable, Hashable { } } - public init(enable: GroupFeatureEnabled) { + public init(enable: GroupFeatureEnabled, role: GroupMemberRole? = nil) { self.enable = enable + self.role = role } } diff --git a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/ChatModel.kt b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/ChatModel.kt index 3c9ece9dce..09142d2cc7 100644 --- a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/ChatModel.kt +++ b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/ChatModel.kt @@ -697,15 +697,15 @@ object ChatModel { } suspend fun removeMemberItems(rhId: Long?, removedMember: GroupMember, byMember: GroupMember, groupInfo: GroupInfo) { - fun removedUpdatedItem(item: ChatItem): ChatItem? { - val newContent = when { - item.chatDir is CIDirection.GroupSnd && removedMember.groupMemberId == groupInfo.membership.groupMemberId -> CIContent.SndModerated - item.chatDir is CIDirection.GroupRcv && item.chatDir.groupMember.groupMemberId == removedMember.groupMemberId -> CIContent.RcvModerated - else -> return null - } + fun isRemovedMemberItem(item: ChatItem): Boolean = when { + item.chatDir is CIDirection.GroupSnd -> removedMember.groupMemberId == groupInfo.membership.groupMemberId + item.chatDir is CIDirection.GroupRcv -> item.chatDir.groupMember.groupMemberId == removedMember.groupMemberId + else -> false + } + fun markedUpdatedItem(item: ChatItem): ChatItem? { + if (!isRemovedMemberItem(item)) return null val updatedItem = item.copy( - meta = item.meta.copy(itemDeleted = CIDeleted.Moderated(Clock.System.now(), byGroupMember = byMember)), - content = if (groupInfo.fullGroupPreferences.fullDelete.on) newContent else item.content + meta = item.meta.copy(itemDeleted = CIDeleted.Moderated(Clock.System.now(), byGroupMember = byMember)) ) if (item.isActiveReport) { decreaseGroupReportsCounter(rhId, groupInfo.id) @@ -713,21 +713,52 @@ object ChatModel { return updatedItem } + // Mirrors backend groupFeatureMemberAllowed: fullDelete may be role-gated in business groups. + val fullDeletePref = groupInfo.fullGroupPreferences.fullDelete + val fullDelete = fullDeletePref.on && + byMember.memberRole >= (fullDeletePref.role ?: GroupMemberRole.Observer) val cInfo = ChatInfo.Group(groupInfo, groupChatScope = null) // TODO [knocking] review if (chatId.value == groupInfo.id) { - for (i in 0 until chatItems.value.size) { - val updatedItem = removedUpdatedItem(chatItems.value[i]) - if (updatedItem != null) { - updateChatItem(cInfo, updatedItem, atIndex = i) + if (fullDelete) { + for (item in chatItems.value) { + if (isRemovedMemberItem(item)) { + if (item.isRcvNew) { + decreaseCounterInPrimaryContext(rhId, groupInfo.id) + } + if (item.isActiveReport) { + decreaseGroupReportsCounter(rhId, groupInfo.id) + } + } + } + chatItems.removeAllAndNotify { item -> + val remove = isRemovedMemberItem(item) + if (remove) AudioPlayer.stop(item) + remove + } + } else { + for (i in 0 until chatItems.value.size) { + val updatedItem = markedUpdatedItem(chatItems.value[i]) + if (updatedItem != null) { + updateChatItem(cInfo, updatedItem, atIndex = i) + } } } } else { val i = getChatIndex(rhId, groupInfo.id) - val chat = chats[i] - if (chat.chatItems.isNotEmpty()) { - val updatedItem = removedUpdatedItem(chat.chatItems[0]) - if (updatedItem != null) { - chats.value[i] = chat.copy(chatItems = listOf(updatedItem)) + if (i >= 0) { + val chat = chats[i] + if (chat.chatItems.isNotEmpty()) { + val preview = chat.chatItems[0] + if (isRemovedMemberItem(preview)) { + if (fullDelete) { + chats.value[i] = chat.copy(chatItems = listOf(ChatItem.deletedItemDummy)) + } else { + val updatedItem = markedUpdatedItem(preview) + if (updatedItem != null) { + chats.value[i] = chat.copy(chatItems = listOf(updatedItem)) + } + } + } } } } diff --git a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/SimpleXAPI.kt b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/SimpleXAPI.kt index a31dc145a3..ec75c1a359 100644 --- a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/SimpleXAPI.kt +++ b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/SimpleXAPI.kt @@ -6067,7 +6067,8 @@ data class GroupPreferences( @Serializable data class GroupPreference( - val enable: GroupFeatureEnabled + val enable: GroupFeatureEnabled, + val role: GroupMemberRole? = null, ) { val on: Boolean get() = enable == GroupFeatureEnabled.ON diff --git a/plans/2026-05-20-member-deletion-fulldelete.md b/plans/2026-05-20-member-deletion-fulldelete.md new file mode 100644 index 0000000000..9d6e516b00 --- /dev/null +++ b/plans/2026-05-20-member-deletion-fulldelete.md @@ -0,0 +1,73 @@ +# Full delete on member removal under fullDelete preference + +Plan for the next attempt at the change previously tried in PR #6831 (closed as too messy: the member row was deleted twice on one path, and the user's own membership row was deleted when the user was the one removed). The change is small: two SQL-function edits, one new chat-layer helper, an explicit fullDelete branch plus order swap in two backend handlers, and one in-memory removal branch in `removeMemberItems` on each UI platform. + +## Problem + +When a member is removed via `XGrpMemDel` with `withMessages = True` and the group's `fullDelete` preference is on for the deleter's role, the member's chat items are currently rewritten to `CIModerated` placeholders by `updateMemberCIsModerated`, and the member row is preserved by `deleteOrUpdateMemberRecord` when any item references it. The intent of `fullDelete` is physical deletion. The current behavior leaves placeholder rows and, because `deleteOrUpdateMemberRecord` runs before the items pass, the relay subpath of the latter deletes the member row first and the subsequent file collection returns nothing — files on disk leak. The same ordering bug exists on the moderator side (`APIRemoveMembers`). + +## What changes + +In `xGrpMemDel` (`src/Simplex/Chat/Library/Subscriber.hs:3157`), only on the branch where `withMessages = True` AND `groupFeatureMemberAllowed SGFFullDelete m gInfo`: + +**Case A — the deleted member is the user themselves (`memId == membership.memberId`).** The user's own sent items (those with `group_member_id IS NULL AND item_sent = 1`) and their files are physically deleted. The `membership` row stays with status `GSMemRemoved`, so the group can still be loaded in the chat list and opened. + +**Case B — the deleted member is somebody else.** The member's chat items and their files are physically deleted, then the `group_members` row is deleted. Historical system event items that referenced this member as `item_deleted_by_group_member_id` survive with NULL via the existing `ON DELETE SET NULL`. + +The non-fullDelete branch, the `withMessages = False` branch, and the entire message-moderation path (`XMsgDel`, `APIDeleteMemberChatItem`, `deleteGroupCIs`, `markGroupCIsDeleted`, `createCIModeration`, `chat_item_moderations`) are not changed. + +## Implementation + +The whole change is two SQL-function edits in `Store/Messages.hs`, a new member-record helper in `Library/Internal.hs`, and explicit branching plus an order swap in both `xGrpMemDel` (recipient side) and `APIRemoveMembers` (moderator side). + +**Edit 1 — rewrite `updateMemberCIsModerated` to physically delete.** Recommended rename: `deleteMemberCIs`. Keep the existing `memId == groupMemberId' membership` branch unchanged (the membership branch selects `WHERE group_member_id IS NULL AND item_sent = 1`; the other branch selects `WHERE group_member_id = ?`). Change the body from "UPDATE chat_items SET moderated content" to "physically delete chat_items + side-table rows analogous to `deleteGroupChatItem` in bulk": delete from `chat_item_messages`, `chat_item_versions`, `chat_item_reactions`, then `DELETE FROM chat_items`. The function loses the `byGroupMember`, `msgDir`, and `deletedTs` parameters since they were only used to construct the moderated content. The chat-layer wrappers `deleteGroupMemberCIs` and `deleteGroupMembersCIs` follow the same signature simplification. + +**Edit 2 — extend `getGroupMemberFileInfo` to handle the membership case.** Today it queries `WHERE group_member_id = ?` only, so for Case A it returns nothing and files for the user's own sent items leak (this is a pre-existing bug on both the off and on paths — `markGroupMemberCIsDeleted_` also relies on this function to cancel in-progress transfers). Add the same `memId == groupMemberId' membership` branch as in `deleteMemberCIs`: for the membership case, query `WHERE group_member_id IS NULL AND item_sent = 1`. The only two callers (`deleteGroupMemberCIs_` and `markGroupMemberCIsDeleted_`) both benefit from the fix. + +**Edit 3 — add `fullyDeleteMemberRecord` helper in `Library/Internal.hs` next to `deleteOrUpdateMemberRecord`.** Wraps `deleteSupportChatIfExists` + `deleteGroupMember`, returns updated `GroupInfo`. No `isRelay` branch and no `checkGroupMemberHasItems` query — the caller has already physically deleted the member's items, so the existence check would be a wasted query and the function communicates intent explicitly: unconditional row deletion. The `CM` wrapper plus an `IO` variant (`fullyDeleteMemberRecordIO`) mirror the shape of the existing `deleteOrUpdateMemberRecord` / `deleteOrUpdateMemberRecordIO`. + +**Edit 4 — swap order and add explicit branching in `xGrpMemDel` Case B (the `else` branch).** Move `when withMessages $ deleteMessages gInfo'' deletedMember' SMDRcv` to run *before* the member-record decision, on the same `gInfo` (the new `deleteMessages` reads only `groupId` and `membership` from the passed `gInfo`). Replace the current member-record dispatch with an explicit branch: + +``` +gInfo' <- case deliveryScope of + Just (DJSMemberSupport _) | shouldForward -> updateMemberRecordDeleted user gInfo deletedMember GSMemRemoved + _ -> if withMessages && groupFeatureMemberAllowed SGFFullDelete m gInfo + then fullyDeleteMemberRecord user gInfo deletedMember + else deleteOrUpdateMemberRecord user gInfo deletedMember +``` + +`deleteMemberItem` (the RGE event creation) keeps its current position after `updatePublicGroupData`. Case A (the `then` branch) needs no order change — the membership row is never deleted there, and `deleteMessages` already runs in the right relative position. + +The `DJSMemberSupport _ | shouldForward` subcase keeps its existing `updateMemberRecordDeleted` call regardless of fullDelete — the row is preserved for support-scope forwarding. Under fullDelete the items are still gone (the `deleteMessages` step ran first), the row stays. + +**Edit 5 — mirror the swap and explicit branching in `APIRemoveMembers` (`src/Simplex/Chat/Library/Commands.hs:2834`).** Inside `deleteMemsSend`, compute `fullDelete = withMessages && groupFeatureUserAllowed SGFFullDelete gInfo` once. Move the items pass to before `delMember`: run `deleteMessages user gInfo memsToDelete` inside `deleteMemsSend` before the `withStoreBatch'` that calls `delMember`. Change `delMember` to branch explicitly: + +``` +delMember db m = do + if fullDelete + then void $ fullyDeleteMemberRecordIO db user gInfo m + else void $ deleteOrUpdateMemberRecordIO db user gInfo m + pure m {memberStatus = GSMemRemoved} +``` + +`deletePendingMember` flows through `deleteMemsSend` and inherits the new behavior. The outer line 2864 call (`when withMessages $ deleteMessages user gInfo' deleted`) collapses — items are already handled inside `deleteMemsSend` for current and pending members, and invited members (handled by `deleteInvitedMems`) have no chat items. Remove it. + +**Edit 6 — extend `removeMemberItems` on both UI platforms to physically remove items from the in-memory list when `fullDelete.on`.** Today (iOS `apps/ios/Shared/Model/ChatModel.swift:814-846`, Kotlin `apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/ChatModel.kt:699-734`) the function walks the in-memory items, identifies matches by direction and member id, and sets `itemDeleted = .moderated(...)`; under `fullDelete.on` it additionally rewrites content to `Snd/RcvModerated`. Items are never removed from `im.reversedChatItems` / `chatItems.value`. After the backend change, the chat_item rows are physically gone in DB while the UI keeps stale moderated placeholders until the next refetch — flicker. Extend the existing `fullDelete.on` branch so it also removes matching items from the in-memory list (iOS: drop them from `im.reversedChatItems`, decrement unread counters, stop voice playback on dropped items; Kotlin: `removeAllAndNotify { isMemberItem(it) }` equivalent, decrement counters, stop audio). The fullDelete-off branch is unchanged (still marks moderated in place). + +The three callers — iOS `removeMember` in `GroupChatInfoView.swift:977`, Kotlin `removeMembers` in `GroupChatInfoView.kt:1316`, Kotlin `removeMember` in `GroupMemberInfoView.kt:339`, and the event handlers for `.deletedMember`/`.deletedMemberUser` in `SimpleXAPI.swift:2578-2596` and `SimpleXAPI.kt:2945-2973` — all converge on the same `removeMemberItems` function on each platform and inherit the new behavior automatically. The chat-list preview path inside `removeMemberItems` (the `else` branch that updates `chat.chatItems[0]`) also needs to drop the preview item under fullDelete so the chat list doesn't show a stale moderated last-message. + +The `fullDelete.on` gate matches the backend's `groupFeatureMemberAllowed SGFFullDelete` / `groupFeatureUserAllowed SGFFullDelete` because FullDelete is a `GroupFeatureNoRoleI` feature — the role check collapses to the `.on` check. + +## Anti-patterns from PR #6831 to avoid + +No path may call `deleteGroupMember` twice. No path under Case A may delete the `membership` row — that row must survive. File info must be collected before any chat-item deletion, since `getGroupMemberFileInfo` reads `chat_items`. Do not rely on `ON DELETE SET NULL` to clean up the deleted member's authored items — they are deleted explicitly first. `fullyDeleteMemberRecord` is the only function that should call `deleteGroupMember` directly on the new path; do not duplicate that call in the handler. + +## Tests + +Add cases in `tests/ChatTests/Groups.hs` for: Case A (user removed by admin, fullDelete on — user's sent items and their files gone, `membership` row exists with `GSMemRemoved`, group still loadable); Case B (member removed by admin, fullDelete on — member's items and files gone, `group_members` row gone, system event items previously referencing the removed member now have NULL `item_deleted_by_group_member_id` and still display correctly); regression for fullDelete=off (items become `CIModerated` placeholders via `markMemberCIsDeleted`); regression for `withMessages = False` (items untouched, row handled by existing path); regression that message moderation under fullDelete=on still produces `CIModerated` placeholders, confirming the moderation path is unchanged. Verify the same Case A and Case B behaviors over both XGrpMemDel (recipient side, Subscriber.hs) and APIRemoveMembers (moderator side, Commands.hs). + +UI checks for the manual smoke test: in a group with fullDelete on, remove a member with messages — that member's bubbles disappear immediately from the open chat view on both moderator's and recipients' devices, the chat list preview updates to the previous non-deleted message, and the unread/report counters decrement; with fullDelete off, the same removal produces moderated placeholders as today. Verify on iOS, Android, and Desktop. + +## Open items for review + +Naming of the rewritten `updateMemberCIsModerated`: `deleteMemberCIs` is the natural rename (the function physically deletes chat items associated with a member, handling the membership case internally). Naming of the new chat-layer helper: `fullyDeleteMemberRecord` (parallels `deleteOrUpdateMemberRecord`). Confirm or amend before implementation. diff --git a/src/Simplex/Chat/Library/Commands.hs b/src/Simplex/Chat/Library/Commands.hs index f835445f0d..1bd49af52a 100644 --- a/src/Simplex/Chat/Library/Commands.hs +++ b/src/Simplex/Chat/Library/Commands.hs @@ -2871,7 +2871,6 @@ processChatCommand vr nm = \case let acis' = map (updateACIGroupInfo gInfo') acis unless (null acis') $ toView $ CEvtNewChatItems user acis' unless (null errs) $ toView $ CEvtChatErrors errs - when withMessages $ deleteMessages user gInfo' deleted pure $ CRUserDeletedMembers user gInfo' deleted withMessages msgSigned -- same order is not guaranteed where selectMembers :: S.Set GroupMemberId -> [GroupMember] -> (Int, [GroupMember], [GroupMember], [GroupMember], [GroupMember], GroupMemberRole, Bool) @@ -2916,11 +2915,14 @@ processChatCommand vr nm = \case Left e -> Just $ Left e itemsData = mapMaybe skipUnwantedItem itemsData_ cis_ <- saveSndChatItems user (CDGroupSnd gInfo chatScopeInfo) False itemsData Nothing False + -- MUST run before delMember so getGroupMemberFileInfo can still resolve file info under fullDelete. + when withMessages $ deleteMessages user gInfo memsToDelete deleteMembersConnections' user memsToDelete True (errs, deleted) <- lift $ partitionEithers <$> withStoreBatch' (\db -> map (delMember db) memsToDelete) let acis = map (AChatItem SCTGroup SMDSnd (GroupChat gInfo chatScopeInfo)) $ rights cis_ pure (errs, deleted, acis, signed) where + fullDelete = withMessages && groupFeatureUserAllowed SGFFullDelete gInfo sndItemData :: GroupMember -> SndMessage -> Maybe (NewSndChatItemData c) sndItemData GroupMember {groupMemberId, memberProfile, memberStatus} msg | memberStatus == GSMemRemoved || memberStatus == GSMemLeft = Nothing @@ -2933,10 +2935,12 @@ processChatCommand vr nm = \case -- voided result (updated group info) may have incorrect state of membersRequireAttention. -- To avoid complicating code by chaining group info updates, -- instead we re-read it once after deleting all members before response. - void $ deleteOrUpdateMemberRecordIO db user gInfo m + if fullDelete + then void $ fullyDeleteMemberRecordIO db user gInfo m + else void $ deleteOrUpdateMemberRecordIO db user gInfo m pure m {memberStatus = GSMemRemoved} deleteMessages user gInfo@GroupInfo {membership} ms - | groupFeatureUserAllowed SGFFullDelete gInfo = deleteGroupMembersCIs user gInfo ms membership + | groupFeatureUserAllowed SGFFullDelete gInfo = deleteGroupMembersCIs user gInfo ms | otherwise = markGroupMembersCIsDeleted user gInfo ms membership APILeaveGroup groupId -> withUser $ \user@User {userId} -> do gInfo@GroupInfo {membership} <- withFastStore $ \db -> getGroupInfo db vr user groupId diff --git a/src/Simplex/Chat/Library/Internal.hs b/src/Simplex/Chat/Library/Internal.hs index 576eb942a5..f824777f28 100644 --- a/src/Simplex/Chat/Library/Internal.hs +++ b/src/Simplex/Chat/Library/Internal.hs @@ -515,22 +515,20 @@ updateACIGroupInfo gInfo' = \case AChatItem SCTGroup dir (GroupChat gInfo' chatScopeInfo) ci aci -> aci -deleteGroupMemberCIs :: MsgDirectionI d => User -> GroupInfo -> GroupMember -> GroupMember -> SMsgDirection d -> CM () -deleteGroupMemberCIs user gInfo member byGroupMember msgDir = do - deletedTs <- liftIO getCurrentTime - filesInfo <- withStore' $ \db -> deleteGroupMemberCIs_ db user gInfo member byGroupMember msgDir deletedTs +deleteGroupMemberCIs :: User -> GroupInfo -> GroupMember -> CM () +deleteGroupMemberCIs user gInfo member = do + filesInfo <- withStore' $ \db -> deleteGroupMemberCIs_ db user gInfo member deleteCIFiles user filesInfo -deleteGroupMembersCIs :: User -> GroupInfo -> [GroupMember] -> GroupMember -> CM () -deleteGroupMembersCIs user gInfo members byGroupMember = do - deletedTs <- liftIO getCurrentTime - filesInfo <- withStore' $ \db -> fmap concat $ forM members $ \m -> deleteGroupMemberCIs_ db user gInfo m byGroupMember SMDRcv deletedTs +deleteGroupMembersCIs :: User -> GroupInfo -> [GroupMember] -> CM () +deleteGroupMembersCIs user gInfo members = do + filesInfo <- withStore' $ \db -> fmap concat $ forM members $ deleteGroupMemberCIs_ db user gInfo deleteCIFiles user filesInfo -deleteGroupMemberCIs_ :: MsgDirectionI d => DB.Connection -> User -> GroupInfo -> GroupMember -> GroupMember -> SMsgDirection d -> UTCTime -> IO [CIFileInfo] -deleteGroupMemberCIs_ db user gInfo member byGroupMember msgDir deletedTs = do +deleteGroupMemberCIs_ :: DB.Connection -> User -> GroupInfo -> GroupMember -> IO [CIFileInfo] +deleteGroupMemberCIs_ db user gInfo member = do fs <- getGroupMemberFileInfo db user gInfo member - updateMemberCIsModerated db user gInfo member byGroupMember msgDir deletedTs + deleteMemberCIs db user gInfo member pure fs deleteLocalCIs :: User -> NoteFolder -> [CChatItem 'CTLocal] -> Bool -> Bool -> CM ChatResponse @@ -1853,6 +1851,17 @@ deleteOrUpdateMemberRecordIO db user@User {userId} gInfo m = do Nothing -> deleteGroupMember db user m' pure gInfo' +-- Unlike deleteOrUpdateMemberRecord, skips checkGroupMemberHasItems. +fullyDeleteMemberRecord :: User -> GroupInfo -> GroupMember -> CM GroupInfo +fullyDeleteMemberRecord user gInfo m = + withStore' $ \db -> fullyDeleteMemberRecordIO db user gInfo m + +fullyDeleteMemberRecordIO :: DB.Connection -> User -> GroupInfo -> GroupMember -> IO GroupInfo +fullyDeleteMemberRecordIO db user gInfo m = do + (gInfo', m') <- deleteSupportChatIfExists db user gInfo m + deleteGroupMember db user m' + pure gInfo' + updateMemberRecordDeleted :: User -> GroupInfo -> GroupMember -> GroupMemberStatus -> CM GroupInfo updateMemberRecordDeleted user@User {userId} gInfo m newStatus = withStore' $ \db -> do diff --git a/src/Simplex/Chat/Library/Subscriber.hs b/src/Simplex/Chat/Library/Subscriber.hs index a661e53752..f0fea2dbf1 100644 --- a/src/Simplex/Chat/Library/Subscriber.hs +++ b/src/Simplex/Chat/Library/Subscriber.hs @@ -3176,7 +3176,7 @@ processAgentMessageConn vr user@User {userId} corrId agentConnId agentMessage = updateGroupMemberStatus db userId membership GSMemRemoved when (maybe False (/= RSRejected) (relayOwnStatus gInfo)) $ updateRelayOwnStatus_ db gInfo RSInactive let membership' = membership {memberStatus = GSMemRemoved} - when withMessages $ deleteMessages gInfo membership' SMDSnd + when withMessages $ deleteMessages gInfo membership' deleteMemberItem msg gInfo RGEUserDeleted toView $ CEvtDeletedMemberUser user gInfo {membership = membership'} m withMessages msgSigned pure $ Just DJSGroup {jobSpec = DJRelayRemoved} @@ -3196,15 +3196,18 @@ processAgentMessageConn vr user@User {userId} corrId agentConnId agentMessage = deleteMemberConnection' deletedMember True else deleteMemberConnection deletedMember let deliveryScope = memberEventDeliveryScope deletedMember + deletedMember' = deletedMember {memberStatus = GSMemRemoved} + when withMessages $ deleteMessages gInfo deletedMember' gInfo' <- case deliveryScope of -- Keep member record if it's support scope - it will be required for forwarding inside that scope. Just (DJSMemberSupport _) | shouldForward -> updateMemberRecordDeleted user gInfo deletedMember GSMemRemoved - -- Undeleted "member connected" chat item will prevent deletion of member record. - _ -> deleteOrUpdateMemberRecord user gInfo deletedMember + _ + | withMessages && groupFeatureMemberAllowed SGFFullDelete m gInfo -> + fullyDeleteMemberRecord user gInfo deletedMember + -- Undeleted "member connected" chat item will prevent deletion of member record. + | otherwise -> deleteOrUpdateMemberRecord user gInfo deletedMember gInfo'' <- updatePublicGroupData user gInfo' let wasDeleted = memberStatus == GSMemRemoved || memberStatus == GSMemLeft - deletedMember' = deletedMember {memberStatus = GSMemRemoved} - when withMessages $ deleteMessages gInfo'' deletedMember' SMDRcv -- Clear forwardedByMember if it references the deleted member, -- as the member record was already deleted above. let RcvMessage {forwardedByMember = fwdBy} = msg @@ -3221,9 +3224,9 @@ processAgentMessageConn vr user@User {userId} corrId agentConnId agentMessage = (gi', m', scopeInfo) <- mkGroupChatScope gi m (ci, cInfo) <- saveRcvChatItemNoParse user (CDGroupRcv gi' scopeInfo m') msg' brokerTs (CIRcvGroupEvent gEvent) groupMsgToView cInfo ci - deleteMessages :: MsgDirectionI d => GroupInfo -> GroupMember -> SMsgDirection d -> CM () - deleteMessages gInfo' delMem msgDir - | groupFeatureMemberAllowed SGFFullDelete m gInfo' = deleteGroupMemberCIs user gInfo' delMem m msgDir + deleteMessages :: GroupInfo -> GroupMember -> CM () + deleteMessages gInfo' delMem + | groupFeatureMemberAllowed SGFFullDelete m gInfo' = deleteGroupMemberCIs user gInfo' delMem | otherwise = markGroupMemberCIsDeleted user gInfo' delMem m forwardToMember :: GroupMember -> CM () forwardToMember member = diff --git a/src/Simplex/Chat/Store/Messages.hs b/src/Simplex/Chat/Store/Messages.hs index 5d433088a4..cdd3185209 100644 --- a/src/Simplex/Chat/Store/Messages.hs +++ b/src/Simplex/Chat/Store/Messages.hs @@ -65,7 +65,7 @@ module Simplex.Chat.Store.Messages updateGroupCIMentions, deleteGroupChatItem, updateGroupChatItemModerated, - updateMemberCIsModerated, + deleteMemberCIs, updateGroupCIBlockedByAdmin, markGroupChatItemDeleted, markMemberCIsDeleted, @@ -211,9 +211,19 @@ getGroupFileInfo db User {userId} GroupInfo {groupId} = <$> DB.query db (fileInfoQuery <> " WHERE i.user_id = ? AND i.group_id = ?") (userId, groupId) getGroupMemberFileInfo :: DB.Connection -> User -> GroupInfo -> GroupMember -> IO [CIFileInfo] -getGroupMemberFileInfo db User {userId} GroupInfo {groupId} GroupMember {groupMemberId} = - map toFileInfo - <$> DB.query db (fileInfoQuery <> " WHERE i.user_id = ? AND i.group_id = ? AND i.group_member_id = ?") (userId, groupId, groupMemberId) +getGroupMemberFileInfo db User {userId} GroupInfo {groupId, membership} member + | groupMemberId' member == groupMemberId' membership = + map toFileInfo + <$> DB.query + db + (fileInfoQuery <> " WHERE i.user_id = ? AND i.group_id = ? AND i.group_member_id IS NULL AND i.item_sent = 1") + (userId, groupId) + | otherwise = + map toFileInfo + <$> DB.query + db + (fileInfoQuery <> " WHERE i.user_id = ? AND i.group_id = ? AND i.group_member_id = ?") + (userId, groupId, groupMemberId' member) deleteGroupChatItemsMessages :: DB.Connection -> User -> GroupInfo -> IO () deleteGroupChatItemsMessages db User {userId} GroupInfo {groupId} = do @@ -2814,39 +2824,60 @@ updateGroupChatItemModerated db User {userId} GroupInfo {groupId} ci m@GroupMemb (deletedTs, groupMemberId, toContent, toText, currentTs, userId, groupId, itemId) pure ci {content = toContent, meta = (meta ci) {itemText = toText, itemDeleted = Just (CIModerated (Just deletedTs) m), editable = False, deletable = False}, formattedText = Nothing} -updateMemberCIsModerated :: MsgDirectionI d => DB.Connection -> User -> GroupInfo -> GroupMember -> GroupMember -> SMsgDirection d -> UTCTime -> IO () -updateMemberCIsModerated db User {userId} GroupInfo {groupId, membership} member byGroupMember md deletedTs = do - itemIds <- updateCIs =<< getCurrentTime +deleteMemberCIs :: DB.Connection -> User -> GroupInfo -> GroupMember -> IO () +deleteMemberCIs db User {userId} GroupInfo {groupId, membership} member = do + items <- selectItems + let itemMemberId = memberId' member #if defined(dbPostgres) - let inItemIds = Only $ In (map fromOnly itemIds) - DB.execute db "DELETE FROM messages WHERE message_id IN (SELECT message_id FROM chat_item_messages WHERE chat_item_id IN ?)" inItemIds - DB.execute db "DELETE FROM chat_item_versions WHERE chat_item_id IN ?" inItemIds + let itemIds = map fst items + sharedMsgIds = mapMaybe snd items + unless (null itemIds) $ do + DB.execute + db + [sql| + DELETE FROM messages WHERE message_id IN ( + SELECT message_id FROM chat_item_messages WHERE chat_item_id IN ? + ) + |] + (Only (In itemIds)) + DB.execute db "DELETE FROM chat_item_versions WHERE chat_item_id IN ?" (Only (In itemIds)) + unless (null sharedMsgIds) $ + DB.execute + db + "DELETE FROM chat_item_reactions WHERE group_id = ? AND shared_msg_id IN ? AND item_member_id IS NOT DISTINCT FROM ?" + (groupId, In sharedMsgIds, itemMemberId) + unless (null itemIds) $ + DB.execute + db + "DELETE FROM chat_items WHERE user_id = ? AND group_id = ? AND chat_item_id IN ?" + (userId, groupId, In itemIds) #else - DB.executeMany db deleteChatItemMessagesQuery itemIds - DB.executeMany db "DELETE FROM chat_item_versions WHERE chat_item_id = ?" itemIds + forM_ items $ \(itemId, itemSharedMsgId_) -> do + deleteChatItemMessages_ db itemId + deleteChatItemVersions_ db itemId + forM_ itemSharedMsgId_ $ \sharedMsgId -> + DB.execute + db + "DELETE FROM chat_item_reactions WHERE group_id = ? AND shared_msg_id = ? AND item_member_id IS NOT DISTINCT FROM ?" + (groupId, sharedMsgId, itemMemberId) + DB.execute + db + "DELETE FROM chat_items WHERE user_id = ? AND group_id = ? AND chat_item_id = ?" + (userId, groupId, itemId) #endif where - memId = groupMemberId' member - updateQuery = - [sql| - UPDATE chat_items - SET item_deleted = 1, item_deleted_ts = ?, item_deleted_by_group_member_id = ?, item_content = ?, item_text = ?, updated_at = ? - WHERE user_id = ? AND group_id = ? - |] - updateCIs :: UTCTime -> IO [Only Int64] - updateCIs currentTs - | memId == groupMemberId' membership = + selectItems :: IO [(ChatItemId, Maybe SharedMsgId)] + selectItems + | groupMemberId' member == groupMemberId' membership = DB.query db - (updateQuery <> " AND group_member_id IS NULL AND item_sent = 1 RETURNING chat_item_id") - (columns :. (userId, groupId)) + "SELECT chat_item_id, shared_msg_id FROM chat_items WHERE user_id = ? AND group_id = ? AND group_member_id IS NULL AND item_sent = 1" + (userId, groupId) | otherwise = DB.query db - (updateQuery <> " AND group_member_id = ? RETURNING chat_item_id") - (columns :. (userId, groupId, memId)) - where - columns = (deletedTs, groupMemberId' byGroupMember, msgDirToModeratedContent_ md, ciModeratedText, currentTs) + "SELECT chat_item_id, shared_msg_id FROM chat_items WHERE user_id = ? AND group_id = ? AND group_member_id = ?" + (userId, groupId, groupMemberId' member) updateGroupCIBlockedByAdmin :: DB.Connection -> User -> GroupInfo -> ChatItem 'CTGroup d -> UTCTime -> IO (ChatItem 'CTGroup d) updateGroupCIBlockedByAdmin db User {userId} GroupInfo {groupId} ci deletedTs = do diff --git a/src/Simplex/Chat/Store/SQLite/Migrations/chat_query_plans.txt b/src/Simplex/Chat/Store/SQLite/Migrations/chat_query_plans.txt index 598ee920dd..16f14a0484 100644 --- a/src/Simplex/Chat/Store/SQLite/Migrations/chat_query_plans.txt +++ b/src/Simplex/Chat/Store/SQLite/Migrations/chat_query_plans.txt @@ -3925,22 +3925,6 @@ Query: Plan: SEARCH user_contact_links USING INTEGER PRIMARY KEY (rowid=?) -Query: - UPDATE chat_items - SET item_deleted = 1, item_deleted_ts = ?, item_deleted_by_group_member_id = ?, item_content = ?, item_text = ?, updated_at = ? - WHERE user_id = ? AND group_id = ? - AND group_member_id = ? RETURNING chat_item_id -Plan: -SEARCH chat_items USING COVERING INDEX idx_chat_items_group_shared_msg_id (user_id=? AND group_id=? AND group_member_id=?) - -Query: - UPDATE chat_items - SET item_deleted = 1, item_deleted_ts = ?, item_deleted_by_group_member_id = ?, item_content = ?, item_text = ?, updated_at = ? - WHERE user_id = ? AND group_id = ? - AND group_member_id IS NULL AND item_sent = 1 RETURNING chat_item_id -Plan: -SEARCH chat_items USING INDEX idx_chat_items_group_shared_msg_id (user_id=? AND group_id=? AND group_member_id=?) - Query: UPDATE chat_items SET item_deleted = 1, item_deleted_ts = ?, item_deleted_by_group_member_id = ?, item_content = ?, item_text = ?, updated_at = ? @@ -5682,6 +5666,15 @@ Plan: SEARCH i USING COVERING INDEX idx_chat_items_group_shared_msg_id (user_id=? AND group_id=? AND group_member_id=?) SEARCH f USING INDEX idx_files_chat_item_id (chat_item_id=?) +Query: + SELECT f.file_id, f.ci_file_status, f.file_path + FROM chat_items i + JOIN files f ON f.chat_item_id = i.chat_item_id + WHERE i.user_id = ? AND i.group_id = ? AND i.group_member_id IS NULL AND i.item_sent = 1 +Plan: +SEARCH i USING INDEX idx_chat_items_group_shared_msg_id (user_id=? AND group_id=? AND group_member_id=?) +SEARCH f USING INDEX idx_files_chat_item_id (chat_item_id=?) + Query: SELECT f.file_id, f.ci_file_status, f.file_path FROM chat_items i @@ -6171,6 +6164,18 @@ SEARCH chat_items USING COVERING INDEX idx_chat_items_fwd_from_chat_item_id (fwd SEARCH files USING COVERING INDEX idx_files_chat_item_id (chat_item_id=?) SEARCH groups USING COVERING INDEX idx_groups_chat_item_id (chat_item_id=?) +Query: DELETE FROM chat_items WHERE user_id = ? AND group_id = ? AND chat_item_id = ? +Plan: +SEARCH chat_items USING INTEGER PRIMARY KEY (rowid=?) +SEARCH chat_item_mentions USING COVERING INDEX idx_chat_item_mentions_chat_item_id (chat_item_id=?) +SEARCH group_snd_item_statuses USING COVERING INDEX idx_group_snd_item_statuses_chat_item_id (chat_item_id=?) +SEARCH chat_item_versions USING COVERING INDEX idx_chat_item_versions_chat_item_id (chat_item_id=?) +SEARCH calls USING COVERING INDEX idx_calls_chat_item_id (chat_item_id=?) +SEARCH chat_item_messages USING COVERING INDEX sqlite_autoindex_chat_item_messages_2 (chat_item_id=?) +SEARCH chat_items USING COVERING INDEX idx_chat_items_fwd_from_chat_item_id (fwd_from_chat_item_id=?) +SEARCH files USING COVERING INDEX idx_files_chat_item_id (chat_item_id=?) +SEARCH groups USING COVERING INDEX idx_groups_chat_item_id (chat_item_id=?) + Query: DELETE FROM chat_items WHERE user_id = ? AND group_id = ? AND group_member_id = ? Plan: SEARCH chat_items USING COVERING INDEX idx_chat_items_group_shared_msg_id (user_id=? AND group_id=? AND group_member_id=?) @@ -6717,6 +6722,14 @@ Query: SELECT chat_item_id FROM chat_items WHERE user_id = ? AND group_id = ? AN Plan: SEARCH chat_items USING INDEX idx_chat_items_group_shared_msg_id (user_id=? AND group_id=? AND group_member_id=? AND shared_msg_id=?) +Query: SELECT chat_item_id, shared_msg_id FROM chat_items WHERE user_id = ? AND group_id = ? AND group_member_id = ? +Plan: +SEARCH chat_items USING COVERING INDEX idx_chat_items_group_shared_msg_id (user_id=? AND group_id=? AND group_member_id=?) + +Query: SELECT chat_item_id, shared_msg_id FROM chat_items WHERE user_id = ? AND group_id = ? AND group_member_id IS NULL AND item_sent = 1 +Plan: +SEARCH chat_items USING INDEX idx_chat_items_group_shared_msg_id (user_id=? AND group_id=? AND group_member_id=?) + Query: SELECT chat_item_ttl FROM contacts WHERE contact_id = ? LIMIT 1 Plan: SEARCH contacts USING INTEGER PRIMARY KEY (rowid=?) diff --git a/tests/ChatTests/Groups.hs b/tests/ChatTests/Groups.hs index 82bf20e6cf..67c32fcca8 100644 --- a/tests/ChatTests/Groups.hs +++ b/tests/ChatTests/Groups.hs @@ -42,6 +42,7 @@ import Simplex.Messaging.Agent.Store.DB (Binary (..)) import Simplex.Messaging.Server.Env.STM hiding (subscriptions) import Simplex.Messaging.Transport import Simplex.Messaging.Version +import System.Directory (copyFile, doesFileExist) import Test.Hspec hiding (it) #if defined(dbPostgres) import Database.PostgreSQL.Simple (Only (..)) @@ -50,7 +51,6 @@ import Database.PostgreSQL.Simple.SqlQQ (sql) import Database.SQLite.Simple (Only (..)) import Database.SQLite.Simple.QQ (sql) import Simplex.Chat.Options.DB -import System.Directory (copyFile) import System.FilePath (()) #endif @@ -1918,7 +1918,7 @@ testGroupDelayedModerationFullDelete ps = do testDeleteMemberWithMessages :: HasCallStack => TestParams -> IO () testDeleteMemberWithMessages = testChat3 aliceProfile bobProfile cathProfile $ - \alice bob cath -> do + \alice bob cath -> withXFTPServer $ do createGroup3' "team" alice (bob, GRMember) (cath, GRMember) threadDelay 750000 alice ##> "/set delete #team on" @@ -1936,22 +1936,61 @@ testDeleteMemberWithMessages = cath <## "Full deletion: on" ] threadDelay 750000 - bob #> "#team hello" - concurrently_ - (alice <# "#team bob> hello") - (cath <# "#team bob> hello") - alice #$> ("/_get chat #1 count=1", chat, [(0, "hello")]) - bob #$> ("/_get chat #1 count=1", chat, [(1, "hello")]) - cath #$> ("/_get chat #1 count=1", chat, [(0, "hello")]) + + alice #$> ("/_files_folder ./tests/tmp/alice_app_files", id, "ok") + bob #$> ("/_files_folder ./tests/tmp/bob_app_files", id, "ok") + cath #$> ("/_files_folder ./tests/tmp/cath_app_files", id, "ok") + copyFile "./tests/fixtures/test.jpg" "./tests/tmp/bob_app_files/test.jpg" + + bob ##> "/_send #1 json [{\"filePath\": \"test.jpg\", \"msgContent\": {\"type\": \"text\", \"text\": \"file from bob\"}}]" + bob <# "#team file from bob" + bob <# "/f #team test.jpg" + bob <## "use /fc 1 to cancel sending" + + alice <# "#team bob> file from bob" + alice <# "#team bob> sends file test.jpg (136.5 KiB / 139737 bytes)" + alice <## "use /fr 1 [/ | ] to receive it" + + cath <# "#team bob> file from bob" + cath <# "#team bob> sends file test.jpg (136.5 KiB / 139737 bytes)" + cath <## "use /fr 1 [/ | ] to receive it" + + bob <## "completed uploading file 1 (test.jpg) for #team" + + alice ##> "/fr 1" + alice + <### [ "saving file 1 from bob to test.jpg", + "started receiving file 1 (test.jpg) from bob" + ] + alice <## "completed receiving file 1 (test.jpg) from bob" + + cath ##> "/fr 1" + cath + <### [ "saving file 1 from bob to test.jpg", + "started receiving file 1 (test.jpg) from bob" + ] + cath <## "completed receiving file 1 (test.jpg) from bob" + + src <- B.readFile "./tests/fixtures/test.jpg" + B.readFile "./tests/tmp/alice_app_files/test.jpg" `shouldReturn` src + B.readFile "./tests/tmp/bob_app_files/test.jpg" `shouldReturn` src + B.readFile "./tests/tmp/cath_app_files/test.jpg" `shouldReturn` src + threadDelay 1000000 alice ##> "/rm #team bob messages=on" alice <## "#team: you removed bob from the group with all messages" bob <## "#team: alice removed you from the group with all messages" bob <## "use /d #team to delete the group" cath <## "#team: alice removed bob from the group with all messages" - alice #$> ("/_get chat #1 count=2", chat, [(0, "moderated [deleted by you]"), (1, "removed bob")]) - bob #$> ("/_get chat #1 count=2", chat, [(1, "moderated [deleted by alice]"), (0, "removed you")]) - cath #$> ("/_get chat #1 count=2", chat, [(0, "moderated [deleted by alice]"), (0, "removed bob")]) + + doesFileExist "./tests/tmp/alice_app_files/test.jpg" `shouldReturn` False + doesFileExist "./tests/tmp/bob_app_files/test.jpg" `shouldReturn` False + doesFileExist "./tests/tmp/cath_app_files/test.jpg" `shouldReturn` False + + -- Under fullDelete, bob's items are physically deleted on all sides; only the system event remains. + alice #$> ("/_get chat #1 count=1", chat, [(1, "removed bob")]) + bob #$> ("/_get chat #1 count=1", chat, [(0, "removed you")]) + cath #$> ("/_get chat #1 count=1", chat, [(0, "removed bob")]) testDeleteMemberMarkMessagesDeleted :: HasCallStack => TestParams -> IO () testDeleteMemberMarkMessagesDeleted =