mirror of
https://github.com/simplex-chat/simplex-chat.git
synced 2026-10-23 08:33:09 +00:00
core: channel messages (#6604)
* core: channel messages (WIP) * do not include member ID when quoting channel messages * query plans * reduce duplication * refactor * refactor plan * refactor 2 * all tests * remove plan * refactor 3 * refactor 4 * refactor 5 * refactor 6 * plans * plans to imrove test coverage and fix bugs * update plan * update plan * bug fixes (wip) * new plan * fixes wip * more tests * comment, fix lint * restore comment * restore comments * rename param * move type * simplify * comment * fix stale state * refactor * less diff * simplify * less diff * refactor --------- Co-authored-by: Evgeny @ SimpleX Chat <259188159+evgeny-simplex@users.noreply.github.com> Co-authored-by: spaced4ndy <8711996+spaced4ndy@users.noreply.github.com>
This commit is contained in:
31 files changed
+3453
-532
No files matched your search
@@ -0,0 +1,321 @@
|
||||
# Plan: Channel Message Bugs Fix
|
||||
|
||||
## Table of Contents
|
||||
1. [Executive Summary](#executive-summary)
|
||||
2. [Bug 1: Delivery Context Flag](#bug-1-delivery-context-flag)
|
||||
3. [Bug 2: Reaction Attribution](#bug-2-reaction-attribution)
|
||||
4. [Bug 3: Update Fallback Default](#bug-3-update-fallback-default)
|
||||
5. [Bug 4: Forward API Parameter](#bug-4-forward-api-parameter)
|
||||
6. [Bug 5: CLI Forward Hardcode](#bug-5-cli-forward-hardcode)
|
||||
7. [Test Plan](#test-plan)
|
||||
8. [Implementation Order](#implementation-order)
|
||||
|
||||
---
|
||||
|
||||
## Executive Summary
|
||||
|
||||
**5 bugs identified** in channel message handling:
|
||||
|
||||
| # | Location | Bug | Severity |
|
||||
|---|----------|-----|----------|
|
||||
| 1 | Subscriber.hs:935-945 | Events use `isChannelOwner` instead of item's `showGroupAsSender` | Critical |
|
||||
| 2 | Subscriber.hs:1818-1842 | Reactions allow `m_=Nothing` and fall back to membership | High |
|
||||
| 3 | Subscriber.hs:1950-1969 | Update fallback creates item without correct sendAsGroup flag | Medium |
|
||||
| 4 | Commands.hs:930,944 | Forward API ignores `_sendAsGroup` parameter | High |
|
||||
| 5 | Commands.hs:2191,2196,2201,4633 | CLI forward hardcodes False | Medium |
|
||||
|
||||
---
|
||||
|
||||
## Bug 1: Delivery Context Flag
|
||||
|
||||
### Current Code (Subscriber.hs:935-945)
|
||||
```haskell
|
||||
let isChannelOwner = useRelays' gInfo' && memberRole' m'' == GROwner
|
||||
showGroupAsSender' = case event of
|
||||
XMsgNew mc -> fromMaybe False (asGroup (mcExtMsgContent mc))
|
||||
XMsgUpdate {} -> isChannelOwner -- BUG: should use item's flag
|
||||
XMsgDel {} -> isChannelOwner -- BUG
|
||||
XMsgReact {} -> isChannelOwner -- BUG
|
||||
XMsgFileDescr {} -> isChannelOwner -- BUG
|
||||
XFileCancel {} -> isChannelOwner -- BUG
|
||||
_ -> False
|
||||
```
|
||||
|
||||
### Problem
|
||||
Events referencing existing items (update, delete, react, file) compute `showGroupAsSender'` from **current sender role** (`isChannelOwner`) instead of **item's stored `showGroupAsSender` flag**.
|
||||
|
||||
### Fix
|
||||
Extract `showGroupAsSender` from the chat item being referenced:
|
||||
|
||||
```haskell
|
||||
showGroupAsSender' = case event of
|
||||
XMsgNew mc -> fromMaybe False (asGroup (mcExtMsgContent mc))
|
||||
XMsgUpdate {} -> itemShowGroupAsSender ci -- from item lookup
|
||||
XMsgDel {} -> itemShowGroupAsSender ci
|
||||
XMsgReact {} -> itemShowGroupAsSender ci
|
||||
XMsgFileDescr {} -> itemShowGroupAsSender ci
|
||||
XFileCancel {} -> itemShowGroupAsSender ci
|
||||
_ -> False
|
||||
```
|
||||
|
||||
**Note:** Use `chatDir` from ChatItem and pattern match on `CIChannelRcv` to determine sendAsGroup flag.
|
||||
|
||||
### Files Modified
|
||||
- `src/Simplex/Chat/Library/Subscriber.hs`: Lines 935-945
|
||||
|
||||
---
|
||||
|
||||
## Bug 2: Reaction Attribution
|
||||
|
||||
### Current Code (Subscriber.hs:1818-1842)
|
||||
```haskell
|
||||
groupMsgReaction :: GroupInfo -> Maybe GroupMember -> SharedMsgId -> Maybe MemberId -> Maybe MsgScope -> MsgReaction -> Bool -> RcvMessage -> UTCTime -> CM (Maybe DeliveryJobScope)
|
||||
groupMsgReaction g m_ sharedMsgId itemMemberId scope_ reaction add RcvMessage {msgId} brokerTs
|
||||
...
|
||||
where
|
||||
GroupInfo {membership} = g
|
||||
reactor = fromMaybe membership m_ -- BUG (line 1842): uses membership when m_ is Nothing
|
||||
ciDir = maybe CIChannelRcv CIGroupRcv m_
|
||||
```
|
||||
|
||||
### Problem
|
||||
When `m_` is `Nothing`, reactor incorrectly falls back to `membership` (user's own member record). However, reactions should always come from an identifiable member - the `m_` parameter should never be `Nothing` for reactions.
|
||||
|
||||
### Fix
|
||||
Reactions can only come from members (including owners), never from channels. XMsgReact handler must be reworked to require `GroupMember` instead of `Maybe GroupMember`. The `m_` parameter should not be optional for reactions.
|
||||
|
||||
### Files Modified
|
||||
- `src/Simplex/Chat/Library/Subscriber.hs`: Lines 1818-1842
|
||||
|
||||
---
|
||||
|
||||
## Bug 3: Update Fallback Default
|
||||
|
||||
### Current Code (Subscriber.hs:1950-1969)
|
||||
```haskell
|
||||
updateRcvChatItem `catchCINotFound` \_ -> do
|
||||
(chatDir, mentions', scopeInfo) <- case m_ of
|
||||
Just m -> ...
|
||||
Nothing -> pure (CDChannelRcv gInfo Nothing, M.empty, Nothing) -- BUG: no sendAsGroup info
|
||||
(ci, cInfo) <- saveRcvChatItem' user chatDir msg ...
|
||||
```
|
||||
|
||||
### Problem
|
||||
When `x.msg.update` arrives for a locally-deleted item in a channel (`m_` is `Nothing`), the fallback creates a new item with `CDChannelRcv gInfo Nothing` but doesn't know the original item's `sendAsGroup` flag.
|
||||
|
||||
### Fix (Option B: Require sender to include flag in the event)
|
||||
Add `asGroup` field to `XMsgUpdate` message format.
|
||||
|
||||
**Rationale:** We don't know what owner wants otherwise - it may send as channel or it may send as owner, and different members must have the same view (e.g. when multiple relays are used, it would be random).
|
||||
|
||||
### Files Modified
|
||||
- `src/Simplex/Chat/Library/Subscriber.hs`: Lines 1950-1969
|
||||
- Protocol message format (XMsgUpdate)
|
||||
|
||||
---
|
||||
|
||||
## Bug 4: Forward API Parameter
|
||||
|
||||
### Current Code (Commands.hs:930,944)
|
||||
```haskell
|
||||
APIForwardChatItems ... _sendAsGroup -> withUser $ \user -> case toCType of
|
||||
CTGroup -> do
|
||||
...
|
||||
sendGroupContentMessages user gInfo toScope (sendAsGroup' gInfo) False itemTTL cmrs'
|
||||
-- ^^^^^^^^^^^^^^^^^^^ BUG: ignores _sendAsGroup
|
||||
```
|
||||
|
||||
### Problem
|
||||
The `_sendAsGroup` parameter is received but ignored. The function computes its own `sendAsGroup' gInfo` instead.
|
||||
|
||||
### Fix
|
||||
```haskell
|
||||
APIForwardChatItems ... sendAsGroup -> withUser $ \user -> case toCType of
|
||||
CTGroup -> do
|
||||
...
|
||||
sendGroupContentMessages user gInfo toScope sendAsGroup False itemTTL cmrs'
|
||||
```
|
||||
|
||||
### Files Modified
|
||||
- `src/Simplex/Chat/Library/Commands.hs`: Line 930 (rename parameter), Line 944 (use parameter)
|
||||
|
||||
---
|
||||
|
||||
## Bug 5: CLI Forward Hardcode
|
||||
|
||||
### Current Code (Commands.hs)
|
||||
```haskell
|
||||
-- Line 2191
|
||||
processChatCommand vr nm $ APIForwardChatItems toChatRef (ChatRef CTDirect contactId Nothing) (forwardedItemId :| []) Nothing False
|
||||
|
||||
-- Line 2196
|
||||
processChatCommand vr nm $ APIForwardChatItems toChatRef (ChatRef CTGroup groupId Nothing) (forwardedItemId :| []) Nothing False
|
||||
|
||||
-- Line 2201
|
||||
processChatCommand vr nm $ APIForwardChatItems toChatRef (ChatRef CTLocal folderId Nothing) (forwardedItemId :| []) Nothing False
|
||||
|
||||
-- Line 4633
|
||||
"/_forward " *> (APIForwardChatItems <$> chatRefP <* A.space <*> chatRefP <*> _strP <*> sendMessageTTLP <*> pure False),
|
||||
```
|
||||
|
||||
### Problem
|
||||
All CLI forward commands hardcode `False` for `sendAsGroup` instead of computing based on destination.
|
||||
|
||||
### Fix
|
||||
Compute `sendAsGroup` before calling API based on destination group's channel status:
|
||||
|
||||
```haskell
|
||||
-- Lines 2191, 2196, 2201: Need to determine sendAsGroup based on toChatRef
|
||||
-- If toChatRef is a channel and user is owner, sendAsGroup should default to True
|
||||
|
||||
-- Line 4633: Parser should accept optional flag (parser cannot know context)
|
||||
```
|
||||
|
||||
### Files Modified
|
||||
- `src/Simplex/Chat/Library/Commands.hs`: Lines 2191, 2196, 2201, 4633
|
||||
|
||||
---
|
||||
|
||||
## Test Plan
|
||||
|
||||
### New Tests (8 total)
|
||||
|
||||
Tests 1-4 cover Bug 1 (delivery context flag). Each tests a specific event type where the owner sends as member (sendAsGroup=False). Existing tests already cover the "sends as channel" (sendAsGroup=True) case; these tests verify that the delivery context correctly uses the item's stored sendAsGroup=False flag rather than recomputing from the owner's current role.
|
||||
|
||||
#### Test 1: `testChannelOwnerUpdateAsMember`
|
||||
**Objective:** Verify x.msg.update uses item's sendAsGroup=False, not current role.
|
||||
|
||||
**Scenario:**
|
||||
1. Owner sends message as member (sendAsGroup=False)
|
||||
2. Member receives message, verify it shows as from member (not channel)
|
||||
3. Owner updates message
|
||||
4. Verify update delivery context uses sendAsGroup=False from the item, not recomputed from owner role
|
||||
|
||||
**Coverage:** Bug 1
|
||||
|
||||
---
|
||||
|
||||
#### Test 2: `testChannelOwnerDeleteAsMember`
|
||||
**Objective:** Verify x.msg.del uses item's sendAsGroup=False, not current role.
|
||||
|
||||
**Scenario:**
|
||||
1. Owner sends message as member (sendAsGroup=False)
|
||||
2. Member receives message, verify it shows as from member (not channel)
|
||||
3. Owner deletes message
|
||||
4. Verify delete delivery context uses sendAsGroup=False from the item, not recomputed from owner role
|
||||
|
||||
**Coverage:** Bug 1
|
||||
|
||||
---
|
||||
|
||||
#### Test 3: `testChannelOwnerFileTransferAsMember`
|
||||
**Objective:** Verify file delivery (including x.msg.file.descr) uses item's sendAsGroup=False, not current role.
|
||||
|
||||
**Scenario:**
|
||||
1. Owner sends file as member (sendAsGroup=False)
|
||||
2. Member receives file, verify it shows as from member (not channel)
|
||||
3. Verify file delivery uses sendAsGroup=False from the item, not recomputed from owner role
|
||||
|
||||
**Note:** x.msg.file.descr is part of file delivery, not a separate event to test independently.
|
||||
|
||||
**Coverage:** Bug 1
|
||||
|
||||
---
|
||||
|
||||
#### Test 4: `testChannelOwnerFileCancelAsMember`
|
||||
**Objective:** Verify x.file.cancel uses item's sendAsGroup=False, not current role.
|
||||
|
||||
**Scenario:**
|
||||
1. Owner sends file as member (sendAsGroup=False)
|
||||
2. Member receives file, verify it shows as from member (not channel)
|
||||
3. Owner cancels file
|
||||
4. Verify cancel delivery context uses sendAsGroup=False from the item, not recomputed from owner role
|
||||
|
||||
**Coverage:** Bug 1
|
||||
|
||||
---
|
||||
|
||||
#### Test 5: `testChannelReactionAttribution`
|
||||
**Objective:** Verify reactions require a member sender (not optional).
|
||||
|
||||
**Scenario:**
|
||||
1. Owner sends channel message
|
||||
2. Owner adds reaction (as member, not as channel)
|
||||
3. Verify reaction is attributed to owner's member record
|
||||
4. Member adds reaction to channel message
|
||||
5. Verify member reaction is attributed correctly
|
||||
6. Verify channel cannot send reactions (m_ must be Just)
|
||||
|
||||
**Coverage:** Bug 2
|
||||
|
||||
---
|
||||
|
||||
#### Test 6: `testChannelUpdateFallbackSendAsGroup`
|
||||
**Objective:** Verify update on deleted item creates correct sendAsGroup from protocol field.
|
||||
|
||||
**Scenario:**
|
||||
1. Owner sends channel message (sendAsGroup=True)
|
||||
2. Member receives and locally deletes
|
||||
3. Owner updates message (XMsgUpdate includes asGroup=True)
|
||||
4. Verify member's recreated item has sendAsGroup=True
|
||||
5. Owner sends message as member (sendAsGroup=False)
|
||||
6. Member receives and locally deletes
|
||||
7. Owner updates message (XMsgUpdate includes asGroup=False)
|
||||
8. Verify member's recreated item has sendAsGroup=False
|
||||
|
||||
**Coverage:** Bug 3
|
||||
|
||||
---
|
||||
|
||||
#### Test 7: `testForwardAPIUsesParameter`
|
||||
**Objective:** Verify Forward API respects sendAsGroup parameter.
|
||||
|
||||
**Scenario:**
|
||||
1. Create channel with owner
|
||||
2. Forward message to channel with sendAsGroup=True
|
||||
3. Verify message sent as channel
|
||||
4. Forward message with sendAsGroup=False
|
||||
5. Verify message sent as member
|
||||
|
||||
**Coverage:** Bug 4
|
||||
|
||||
---
|
||||
|
||||
#### Test 8: `testForwardCLISendAsGroup`
|
||||
**Objective:** Verify CLI forward commands compute sendAsGroup correctly.
|
||||
|
||||
**Scenario:**
|
||||
1. Create channel with owner
|
||||
2. Use `/forward` to forward to channel
|
||||
3. Verify sendAsGroup computed correctly (True for owner in channel)
|
||||
|
||||
**Coverage:** Bug 5
|
||||
|
||||
---
|
||||
|
||||
## Implementation Order
|
||||
|
||||
### Phase 1: Critical Fix (Bug 1)
|
||||
1. Fix delivery context in Subscriber.hs
|
||||
2. Add Tests 1-4 (`testChannelOwnerUpdateAsMember`, `testChannelOwnerDeleteAsMember`, `testChannelOwnerFileTransferAsMember`, `testChannelOwnerFileCancelAsMember`)
|
||||
|
||||
### Phase 2: API Fixes (Bugs 4, 5)
|
||||
1. Fix Forward API parameter usage
|
||||
2. Fix CLI forward hardcodes
|
||||
3. Add Tests 7 and 8 (`testForwardAPIUsesParameter`, `testForwardCLISendAsGroup`)
|
||||
|
||||
### Phase 3: Behavior Fixes (Bugs 2, 3)
|
||||
1. Rework XMsgReact handler to require GroupMember (not Maybe GroupMember)
|
||||
2. Add asGroup field to XMsgUpdate protocol message
|
||||
3. Add Tests 5 and 6 (`testChannelReactionAttribution`, `testChannelUpdateFallbackSendAsGroup`)
|
||||
|
||||
---
|
||||
|
||||
## Files Summary
|
||||
|
||||
| File | Changes |
|
||||
|------|---------|
|
||||
| `src/Simplex/Chat/Library/Subscriber.hs` | Lines 935-945 (Bug 1), 1818-1842 (Bug 2), 1950-1969 (Bug 3) |
|
||||
| `src/Simplex/Chat/Library/Commands.hs` | Lines 930,944 (Bug 4), 2191,2196,2201,4633 (Bug 5) |
|
||||
| Protocol message types | Add asGroup field to XMsgUpdate (Bug 3) |
|
||||
| `tests/ChatTests/Groups.hs` | Add 8 new tests |
|
||||
@@ -0,0 +1,256 @@
|
||||
# Deduplication Plan: Channel Message Functions
|
||||
|
||||
## Table of Contents
|
||||
|
||||
1. [Executive Summary](#executive-summary)
|
||||
2. [Findings by File](#findings-by-file)
|
||||
3. [Architectural Note: CIChannelRcv Constructor](#architectural-note)
|
||||
4. [Implementation Order](#implementation-order)
|
||||
|
||||
---
|
||||
|
||||
## Executive Summary
|
||||
|
||||
The PR introduces channel message support by creating parallel channel-specific functions that duplicate 60-80% of existing group functions. The core pattern: channel messages are group messages without a member sender. Most channel functions are the group function with `Just member` → `Nothing`, `CIGroupRcv m` → `CIChannelRcv`, and moderation/blocking guards removed.
|
||||
|
||||
**High-value deduplication targets** (ordered by impact):
|
||||
|
||||
| # | Candidate | Feasibility | Shared code |
|
||||
|---|-----------|-------------|-------------|
|
||||
| 1 | `channelMessageUpdate_` → merge into `groupMessageUpdate` | HIGH | ~36 lines |
|
||||
| 2 | `fwdChannelReaction` → extract shared helper with `groupMsgReaction` | MEDIUM | ~15 lines inner function |
|
||||
| 3 | `newChannelContentMessage_` → parameterize `newGroupContentMessage` | MEDIUM | ~12 lines happy path |
|
||||
| 4 | `processForwardedChannelMsg` → merge into `processForwardedMsg` | MEDIUM | depends on 1-3 |
|
||||
| 5 | `getGroupCIBySharedMsgId'` → parameterize `getGroupChatItemBySharedMsgId` | HIGH | eliminates function |
|
||||
| 6 | `channelMessageDelete` → parameterize `groupMessageDelete` | LOW | ~5 lines; group has 60+ lines moderation |
|
||||
| 7 | `saveRcvChatItem'` CDChannelRcv branches | HIGH | ~14 lines across 3 spots |
|
||||
| 8 | `processContentItem` CIChannelRcv branch | HIGH | ~3 lines |
|
||||
| 9 | View.hs/Store/Internal pattern match branches | DEFERRED | ~24 branches; requires constructor change |
|
||||
|
||||
---
|
||||
|
||||
## Findings by File
|
||||
|
||||
### Subscriber.hs
|
||||
|
||||
**D1: `channelMessageUpdate_` vs `groupMessageUpdate`**
|
||||
|
||||
The `updateRcvChatItem` inner function is nearly line-for-line identical between both (~36 shared lines). Differences:
|
||||
- Lookup: `getGroupChatItemBySharedMsgId` (by member) vs `getGroupCIBySharedMsgId'` (no member) — parameterizable by `Maybe GroupMemberId` (see D5)
|
||||
- Pattern match: `CIGroupRcv m'` with `sameMemberId` check vs `CIChannelRcv` — branch on `Maybe GroupMember`
|
||||
- `getGroupCIReactions`: `Just memberId` vs `Nothing` — already parameterized
|
||||
- Chat direction in fallback: `CDGroupRcv` vs `CDChannelRcv` — branch on `Maybe GroupMember`
|
||||
- `channelMessageUpdate_` has explicit `forwarded` param; `groupMessageUpdate` always uses `rcvGroupCITimed gInfo ttl_` — the merged function needs to accept `forwarded :: Bool` (or always `False` from the non-forwarded path)
|
||||
- `groupMessageUpdate` has `prohibitedSimplexLinks` and `blockedMemberCI` guards — skip when member is `Nothing`
|
||||
- Mentions handling: `groupMessageUpdate` has `mentions' = if memberBlocked m then [] else mentions`; `channelMessageUpdate_` passes `mentions` directly — when member is `Nothing`, use `mentions` directly (no blocking check needed)
|
||||
|
||||
**Solution:** Extend `groupMessageUpdate` to take `Maybe GroupMember`. When `Nothing`: skip prohibited links check, skip blocked member CI, use `CDChannelRcv`, use `getGroupChatItemBySharedMsgId` with `Nothing`, pass mentions directly. Delete `channelMessageUpdate_`.
|
||||
|
||||
---
|
||||
|
||||
**D2: `fwdChannelReaction` vs `groupMsgReaction`**
|
||||
|
||||
These functions share the `updateChatItemReaction` inner function shape (~15 lines), but are **structurally different** in their outer logic:
|
||||
|
||||
- **Parameter types**: `groupMsgReaction` takes a concrete `GroupMember` + `Maybe MemberId` (item member) + `Maybe MsgScope`; `fwdChannelReaction` takes `Maybe GroupMember` (reactor) and always passes `Nothing` as item member
|
||||
- **Return type**: `groupMsgReaction` returns `CM (Maybe DeliveryJobScope)` — used by the main dispatch for delivery job routing; `fwdChannelReaction` returns `CM ()` — forwarded context doesn't need delivery jobs
|
||||
- **CIReaction constructor**: `groupMsgReaction` always uses `CIGroupRcv m`; `fwdChannelReaction` uses `maybe CIChannelRcv CIGroupRcv reactor_` — semantically different when reactor is `Nothing`
|
||||
- **catchCINotFound fallback**: `groupMsgReaction` has scope-aware delivery job logic; `fwdChannelReaction` does bare `setGroupReaction`
|
||||
- **Reactor**: `groupMsgReaction` uses `m` directly; `fwdChannelReaction` computes `fromMaybe membership reactor_`
|
||||
|
||||
`fwdChannelReaction` is NOT a rename of `groupMsgReaction`. Calling `void $ groupMsgReaction` from forwarded contexts would be **semantically wrong**: it would attribute channel reactions to the membership member via `CIGroupRcv` instead of showing them as `CIChannelRcv`, and would trigger unnecessary delivery job scope logic.
|
||||
|
||||
**Solution:** Extract the shared `updateChatItemReaction` body (~15 lines) into a helper parameterized by the `CIReaction` constructor and reactor member. Both `groupMsgReaction` and `fwdChannelReaction` call this helper with their respective parameters. This preserves the distinct outer logic while eliminating the inner body duplication.
|
||||
|
||||
---
|
||||
|
||||
**D3: `newChannelContentMessage_` vs `newGroupContentMessage`**
|
||||
|
||||
The channel version is the "happy path" of the group version with all member-specific guards removed:
|
||||
- No `blockedByAdmin` check
|
||||
- No `prohibitedGroupContent` check
|
||||
- No `getCIModeration` / moderation logic (~40 lines)
|
||||
- No scope resolution (`mkGetMessageChatScope`)
|
||||
- No `blockedMemberCI`
|
||||
- No member-conditional mentions filtering / autoAcceptFile guard
|
||||
|
||||
The shared "save-view-react-accept" core is ~12 lines.
|
||||
|
||||
**Solution:** Extract a shared `saveGroupContentItem` helper containing: process file invitation, save chat item, get reactions, view, auto-accept, return scope. `newGroupContentMessage` calls it after its checks; `newChannelContentMessage_` calls it directly. This keeps `newGroupContentMessage`'s complex flow intact while eliminating the body duplication.
|
||||
|
||||
Alternatively: extend `newGroupContentMessage` to take `Maybe GroupMember`. When `Nothing`: skip all member-specific guards and use `CDChannelRcv`. This is cleaner but changes the function's signature and control flow significantly.
|
||||
|
||||
---
|
||||
|
||||
**D4: `processForwardedChannelMsg` vs `processForwardedMsg`**
|
||||
|
||||
These are dispatch tables with identical structure. Each event arm calls the group or channel variant:
|
||||
|
||||
```
|
||||
processForwardedMsg author: processForwardedChannelMsg:
|
||||
XMsgNew → newGroupContentMessage XMsgNew → newChannelContentMessage_
|
||||
XMsgFileDescr → groupMessageFileDescription XMsgFileDescr → channelMessageFileDescription
|
||||
XMsgUpdate → groupMessageUpdate XMsgUpdate → channelMessageUpdate_
|
||||
... ...
|
||||
```
|
||||
|
||||
If the underlying functions (D1-D3) are parameterized by `Maybe GroupMember`, this dispatch unifies automatically. The extra group-management events (`XInfo`, `XGrpMemNew`, etc.) are guarded by `Just author`.
|
||||
|
||||
**Subtlety: `XMsgReact` handling.** The `XMsgReact` arm has a three-way split:
|
||||
- `processForwardedMsg` with `Just memId` → `groupMsgReaction` (member reaction with scope/delivery-job logic)
|
||||
- `processForwardedMsg` with `Nothing` memId → `fwdChannelReaction gInfo (Just author)` (channel reaction from known author)
|
||||
- `processForwardedChannelMsg` → `fwdChannelReaction gInfo Nothing` (channel reaction, no author)
|
||||
|
||||
This three-way split needs careful handling in the merged function, since `fwdChannelReaction` differs structurally from `groupMsgReaction` (see D2).
|
||||
|
||||
**Solution:** After D1-D3, merge into `processForwardedMsg` taking `Maybe GroupMember`. When `Nothing`, skip group-management events. The `XMsgReact` arm passes the author to `fwdChannelReaction` when in channel mode. Delete `processForwardedChannelMsg`.
|
||||
|
||||
---
|
||||
|
||||
**D5: `channelMessageDelete` vs `groupMessageDelete`**
|
||||
|
||||
`groupMessageDelete` has ~60 lines of moderation logic (moderate, checkRole, archiveMessageReports, CIModeration creation) that `channelMessageDelete` does not need. The shared portion is only ~5-7 lines (delete/mark-deleted + view). Additionally, the lookup functions differ: `channelMessageDelete` uses `getGroupCIBySharedMsgId'` (no member); `groupMessageDelete` uses `getGroupMemberCIBySharedMsgId` (JOINs group_members by MemberId). The delete condition also differs: `groupFeatureAllowed` vs `groupFeatureMemberAllowed`.
|
||||
|
||||
**Solution:** LOW priority. The functions are architecturally different enough that forced unification would harm readability. If desired, extend `groupMessageDelete` with a `Maybe GroupMember` parameter where `Nothing` takes the simple "channel delete" path early. But the code clarity cost may exceed the deduplication benefit.
|
||||
|
||||
---
|
||||
|
||||
### Store/Messages.hs
|
||||
|
||||
**D6: `getGroupCIBySharedMsgId'` vs `getGroupChatItemBySharedMsgId`**
|
||||
|
||||
`getGroupChatItemBySharedMsgId` filters by `group_member_id = ?`.
|
||||
`getGroupCIBySharedMsgId'` omits the `group_member_id` filter entirely (matches any row regardless of member).
|
||||
|
||||
Channel items store `group_member_id = NULL`. Parameterizing with `Maybe GroupMemberId` and `IS NOT DISTINCT FROM` would:
|
||||
- `Just gmId` → only that member (existing behavior)
|
||||
- `Nothing` → only NULL rows (channel items)
|
||||
|
||||
This is **stricter** than `getGroupCIBySharedMsgId'`'s current behavior (which matches any member's items too), but this is actually a correctness improvement — all four callers (Subscriber.hs lines 1846, 1962, 1988, 3233) are channel-specific contexts where items have `group_member_id = NULL`.
|
||||
|
||||
**Solution:** Change `getGroupChatItemBySharedMsgId` to take `Maybe GroupMemberId`. SQL becomes:
|
||||
```sql
|
||||
WHERE user_id = ? AND group_id = ? AND group_member_id IS NOT DISTINCT FROM ? AND shared_msg_id = ?
|
||||
```
|
||||
Delete `getGroupCIBySharedMsgId'`. Update all callers to pass `Just gmId` or `Nothing`.
|
||||
|
||||
**Note:** `getGroupMemberCIBySharedMsgId` is a different function (takes `MemberId`, JOINs `group_members` to resolve). It is NOT a duplicate and should be kept.
|
||||
|
||||
**Additional Store/Messages.hs duplications** (minor, collapse with constructor change):
|
||||
- `createNewRcvChatItem` quoteRow (lines 560-563): `CDGroupRcv` and `CDChannelRcv` branches are verbatim identical
|
||||
- `getChatItemQuote_` (lines 649-654): `CDChannelRcv` branch is a subset of `CDGroupRcv` (missing sender-specific case)
|
||||
- `createNewChatItem_` idsRow/groupScope: `CDChannelRcv` branches repeat `CDGroupSnd`-like tuples
|
||||
|
||||
These are inherent to the separate constructor and collapse automatically with the architectural change (see note below). Not worth addressing independently.
|
||||
|
||||
---
|
||||
|
||||
### Library/Internal.hs
|
||||
|
||||
**D7: `saveRcvChatItem'` CDChannelRcv branches**
|
||||
|
||||
Three duplicate spots within this function, all verbatim copies of CDGroupRcv branches:
|
||||
|
||||
1. **Mentions/userMention computation** (~7 lines): `getRcvCIMentions`, `userReply` via `cmToQuotedMsg`, `userMention'` via membership check. Verbatim identical between CDGroupRcv and CDChannelRcv.
|
||||
|
||||
2. **createGroupCIMentions** (~2 lines): Both branches call `createGroupCIMentions db g ci mentions'` guarded by `not (null mentions')`. Identical.
|
||||
|
||||
3. **memberChatStats / memberAttentionChange** (~3 lines): Only difference is `Just m` vs `Nothing` passed to `memberAttentionChange`.
|
||||
|
||||
Total: ~14 lines of duplication across 3 spots.
|
||||
|
||||
**Solution:** Extract `GroupInfo` and `Maybe GroupMember` from either constructor at the top:
|
||||
```haskell
|
||||
case cd of
|
||||
CDGroupRcv g _s m -> (g, Just m)
|
||||
CDChannelRcv g _s -> (g, Nothing)
|
||||
```
|
||||
Then use the extracted values for all three spots. The `memberAttentionChange` call already takes `Maybe GroupMember`.
|
||||
|
||||
---
|
||||
|
||||
**D8: `processContentItem` CIChannelRcv branch**
|
||||
|
||||
Near-duplicate of `CIGroupRcv` branch (lines 1196-1199 vs 1200-1202). Only difference: no `blockedByAdmin` guard, passes `Nothing` instead of `Just sender`.
|
||||
|
||||
**Solution:** Merge the two branches:
|
||||
```haskell
|
||||
(CChatItem SMDRcv ci@ChatItem {chatDir, content = CIRcvMsgContent mc, file})
|
||||
| maybe True (not . blockedByAdmin) sender_ -> do
|
||||
fInvDescr_ <- join <$> forM file getRcvFileInvDescr
|
||||
processContentItem sender_ ci mc fInvDescr_
|
||||
where sender_ = case chatDir of CIGroupRcv m -> Just m; CIChannelRcv -> Nothing; _ -> Nothing
|
||||
```
|
||||
|
||||
**Additional Internal.hs duplication** (minor):
|
||||
- `quoteData` (lines 228-229): `CIGroupRcv m` returns `(qmc, CIQGroupRcv $ Just m, False, Just m)`, `CIChannelRcv` returns `(qmc, CIQGroupRcv Nothing, False, Nothing)`. Two one-liners differing only in `Just m` vs `Nothing`. Trivial but noted.
|
||||
|
||||
---
|
||||
|
||||
### View.hs
|
||||
|
||||
**D9: View.hs pattern match duplication**
|
||||
|
||||
The actual count of `CIChannelRcv` pattern match branches:
|
||||
- **View.hs**: 6 branches (chatDirNtf, viewChatItem new, viewChatItem updated, reaction display, sentByMember', fileFrom)
|
||||
- **Terminal/Output.hs**: 1 branch
|
||||
- **Commands.hs**: 2 branches (itemDeletable, itemsMsgMemIds)
|
||||
- **Internal.hs**: 2 branches (quoteData, processContentItem)
|
||||
- **Subscriber.hs**: ~6 branches (scattered)
|
||||
- **Store/Messages.hs**: ~4 branches (toGroupChatItem, createNewRcvChatItem, createNewChatItem_, getChatItemQuote_)
|
||||
|
||||
Total: **~24 pattern match sites** across all files (~17 `CIChannelRcv` + ~7 `CDChannelRcv`). Each mirrors the corresponding `CIGroupRcv m` / `CDGroupRcv` branch passing `Nothing` instead of `Just m`.
|
||||
|
||||
The `ttyFromGroup*` family of functions in View.hs was correctly generalized to take `Maybe GroupMember` — the duplication is at the call sites, not in the helper functions.
|
||||
|
||||
**Solution:** This duplication is **inherent to the separate constructor choice** and can only be eliminated by the architectural change (merging `CIChannelRcv` into `CIGroupRcv (Maybe GroupMember)`). Without that change, the branches must remain. Extracting local helpers at each call site would add complexity without reducing total code.
|
||||
|
||||
---
|
||||
|
||||
### Other Files (no significant deduplication needed)
|
||||
|
||||
- **Commands.hs:** Parameter threading (`ShowGroupAsSender`, `SRGroup`). Clean, no duplication.
|
||||
- **Protocol.hs:** Wire protocol changes (`ExtMsgContent.asGroup`, `XGrpMsgForward Maybe MemberId`). Necessary.
|
||||
- **Delivery.hs:** `FwdSender` type replaces separate fields. Could be `Maybe (MemberId, ContactName)` but not a priority.
|
||||
- **Store/Files.hs:** `createRcvGroupFileTransfer` takes `Maybe GroupMember`. Clean parameterization.
|
||||
- **Store/Groups.hs:** `createPreparedGroup` returns `Maybe GroupMember`. Necessary for relay groups.
|
||||
- **Types.hs:** `sendAsGroup'`, `groupId'` utilities. Minor.
|
||||
|
||||
---
|
||||
|
||||
## Architectural Note: CIChannelRcv Constructor {#architectural-note}
|
||||
|
||||
The deepest source of duplication is the choice to add `CIChannelRcv` / `CDChannelRcv` as separate constructors rather than parameterizing `CIGroupRcv :: Maybe GroupMember -> CIDirection 'CTGroup 'MDRcv` and `CDGroupRcv :: GroupInfo -> Maybe GroupChatScopeInfo -> Maybe GroupMember -> ChatDirection 'CTGroup 'MDRcv`.
|
||||
|
||||
This creates ~24 pattern match branches across the codebase, almost all passing `Nothing` where `CIGroupRcv` passes `Just m`. The `chatItemMember` function already returns `Maybe GroupMember`, confirming the abstraction is correct.
|
||||
|
||||
**However**, changing these constructors is a large cross-cutting refactor affecting Messages.hs, View.hs, Commands.hs, Internal.hs, Subscriber.hs, Store/Messages.hs, and tests. It may be better suited as a follow-up PR.
|
||||
|
||||
**Decision needed from user:** Merge `CIChannelRcv` into `CIGroupRcv (Maybe GroupMember)` in this PR, or defer?
|
||||
|
||||
---
|
||||
|
||||
## Implementation Order
|
||||
|
||||
### Phase 1: Store layer (D6)
|
||||
1. Parameterize `getGroupChatItemBySharedMsgId` with `Maybe GroupMemberId` + `IS NOT DISTINCT FROM`
|
||||
2. Delete `getGroupCIBySharedMsgId'`
|
||||
3. Update all callers (pass `Just gmId` or `Nothing`)
|
||||
|
||||
### Phase 2: Subscriber.hs function merges (D1, D2, D3)
|
||||
4. Merge `channelMessageUpdate_` into `groupMessageUpdate` (takes `Maybe GroupMember`)
|
||||
5. Extract shared `updateChatItemReaction` helper from `groupMsgReaction` and `fwdChannelReaction`
|
||||
6. Merge `newChannelContentMessage_` into `newGroupContentMessage` (extract shared save-view helper or take `Maybe GroupMember`)
|
||||
|
||||
### Phase 3: Dispatch unification (D4)
|
||||
7. Merge `processForwardedChannelMsg` into `processForwardedMsg` (takes `Maybe GroupMember`; handle `XMsgReact` three-way split)
|
||||
|
||||
### Phase 4: Internal cleanup (D7, D8)
|
||||
8. Deduplicate `saveRcvChatItem'` CDChannelRcv branches (3 spots)
|
||||
9. Merge `processContentItem` CIChannelRcv branch
|
||||
|
||||
### Phase 5 (deferred unless approved): Constructor change (D9)
|
||||
10. Merge `CIChannelRcv` into `CIGroupRcv (Maybe GroupMember)` — eliminates ~24 pattern match branches across all files
|
||||
|
||||
### Phase 6 (optional): channelMessageDelete (D5)
|
||||
11. Only if user wants it — extend `groupMessageDelete` with `Maybe GroupMember`
|
||||
@@ -0,0 +1,354 @@
|
||||
# Plan: Fix Channel Message Delivery Architecture
|
||||
|
||||
## Table of Contents
|
||||
1. [Context](#context)
|
||||
2. [Executive Summary](#executive-summary)
|
||||
3. [Issue 1: Eliminate memberForChannel/memberIdForChannel](#issue-1)
|
||||
4. [Issue 2: groupMsgReaction required GroupMember](#issue-2)
|
||||
5. [Issue 3: Fix groupMessageUpdate lookup](#issue-3)
|
||||
6. [Issue 4: DeliveryTaskContext type](#issue-4)
|
||||
7. [Issue 5: Fix testChannelReactionAttribution](#issue-5)
|
||||
8. [Issue 6: Fix testChannelUpdateFallbackSendAsGroup comment](#issue-6)
|
||||
9. [Other: sendAsGroup parameter ordering](#other-issue)
|
||||
10. [Verification](#verification)
|
||||
|
||||
## Context
|
||||
|
||||
The current implementation on `ep/channel-messages-2` determines delivery context (whether to forward messages as channel or as member) using `isChannelOwner` — inferring from the sender's role whether they're the channel owner. This is architecturally wrong: the delivery context should be determined **from the item's direction** (`CIChannelRcv` vs `CIGroupRcv`), not from who sent it. The `f/msg-from-channel` branch has the correct approach.
|
||||
|
||||
## Executive Summary
|
||||
|
||||
7 changes across 7 files:
|
||||
1. **Delivery.hs** — Add `DeliveryTaskContext` type, update `NewMessageDeliveryTask` only (`MessageDeliveryTask` unchanged)
|
||||
2. **Subscriber.hs** — Eliminate `isChannelOwner`/`memberForChannel`/`memberIdForChannel`; all processing functions return `Maybe DeliveryTaskContext`; determine `sentAsGroup` from item direction; `groupMsgReaction` takes required `GroupMember`; add `withAuthor` in forwarded handler
|
||||
3. **Store/Delivery.hs** — Update SQL row mapping for `taskContext`
|
||||
4. **Commands.hs** — Reorder `sendAsGroup` param in `APIForwardChatItems`
|
||||
5. **Store/Messages.hs** — Reorder `showGroupAsSender` param in `createNewSndChatItem`
|
||||
6. **Internal.hs** — Reorder `showGroupAsSender` param in `saveSndChatItems`, `prepareGroupMsg`
|
||||
7. **Tests** — Fix reaction test comment/expectations, fix update fallback test comment
|
||||
|
||||
---
|
||||
|
||||
## Issue 1: Eliminate memberForChannel/memberIdForChannel {#issue-1}
|
||||
|
||||
**File:** `src/Simplex/Chat/Library/Subscriber.hs` lines 935-937, 939-991
|
||||
|
||||
**Problem:** `isChannelOwner`, `memberForChannel`, `memberIdForChannel` computed at lines 935-937 and passed to processing functions. This pre-infers delivery context from member role.
|
||||
|
||||
**Fix:** Remove these three bindings entirely. Always pass `(Just m'')` to functions that take `Maybe GroupMember`. Functions determine `sentAsGroup` from item direction internally.
|
||||
|
||||
**Direct handler changes (lines 939-991):**
|
||||
```
|
||||
-- BEFORE:
|
||||
let isChannelOwner = useRelays' gInfo' && memberRole' m'' == GROwner
|
||||
memberForChannel = if isChannelOwner then Nothing else Just m''
|
||||
memberIdForChannel = memberId' <$> memberForChannel
|
||||
(deliveryJobScope_, showGroupAsSender') <- case event of
|
||||
...
|
||||
forM deliveryJobScope_ $ \jobScope ->
|
||||
pure $ NewMessageDeliveryTask {messageId = msgId, jobScope, showGroupAsSender = showGroupAsSender'}
|
||||
|
||||
-- AFTER:
|
||||
deliveryTaskContext_ <- case event of
|
||||
XMsgNew mc -> ... -- returns Maybe DeliveryTaskContext
|
||||
XMsgFileDescr ... -> groupMessageFileDescription gInfo' (Just m'') sharedMsgId fileDescr
|
||||
XMsgUpdate ... -> memberCanSend m'' msgScope Nothing $ groupMessageUpdate gInfo' (Just m'') sharedMsgId ...
|
||||
XMsgDel ... -> groupMessageDelete gInfo' (Just m'') sharedMsgId ...
|
||||
XMsgReact ... -> groupMsgReaction gInfo' m'' sharedMsgId ... -- required member
|
||||
XFileCancel sharedMsgId -> xFileCancelGroup gInfo' (Just m'') sharedMsgId
|
||||
...other events -> Just <$> memberEventDeliveryContext m'' / Nothing
|
||||
forM deliveryTaskContext_ $ \taskContext ->
|
||||
pure $ NewMessageDeliveryTask {messageId = msgId, taskContext}
|
||||
```
|
||||
|
||||
**Processing function signature changes:**
|
||||
- `groupMessageFileDescription :: GroupInfo -> Maybe GroupMember -> SharedMsgId -> FileDescr -> CM (Maybe DeliveryTaskContext)` — drop both `Maybe MemberId` params, pass `Maybe GroupMember`, determine `sentAsGroup` from `chatDir` of found item
|
||||
- `groupMessageUpdate :: GroupInfo -> Maybe GroupMember -> SharedMsgId -> ... -> Maybe Bool -> CM (Maybe DeliveryTaskContext)` — drop `senderGMId_` param
|
||||
- `groupMessageDelete :: GroupInfo -> Maybe GroupMember -> SharedMsgId -> ... -> CM (Maybe DeliveryTaskContext)` — drop `senderGMId_` param; fix `findOwnerCI` dual-lookup (lines 2028-2035) same as Issue 3: when `m_ = Nothing` search with `Nothing`, when `m_ = Just m` use member lookup directly
|
||||
- `xFileCancelGroup :: GroupInfo -> Maybe GroupMember -> SharedMsgId -> CM (Maybe DeliveryTaskContext)` — drop both `Maybe MemberId` params
|
||||
|
||||
**`validSender` simplification:** Remove second `Maybe MemberId` parameter. With `(Just m'')` always passed, validation is just:
|
||||
```haskell
|
||||
validSender :: Maybe MemberId -> CIDirection 'CTGroup 'MDRcv -> Bool
|
||||
validSender (Just mId) (CIGroupRcv m) = sameMemberId mId m
|
||||
validSender Nothing CIChannelRcv = True
|
||||
validSender _ _ = False
|
||||
```
|
||||
|
||||
**`isChannelDir` helper** remains as-is (line 1870-1872) — used to derive `sentAsGroup` from item's `chatDir`.
|
||||
|
||||
**`memberCanSend`** (line 1436): Generic signature `a -> CM a -> CM a` — no change needed. Default values at call sites change from `(Nothing, False)` to `Nothing`.
|
||||
|
||||
**`memberCanSend'`** (line 1448): Return type changes from `CM (Maybe DeliveryJobScope)` to `CM (Maybe DeliveryTaskContext)`. Used in forwarded handler (lines 3153, 3159).
|
||||
|
||||
---
|
||||
|
||||
## Issue 2: groupMsgReaction required GroupMember {#issue-2}
|
||||
|
||||
**File:** `src/Simplex/Chat/Library/Subscriber.hs` line 1814
|
||||
|
||||
**Problem:** `groupMsgReaction :: GroupInfo -> Maybe GroupMember -> ...` allows `Nothing`, uses `fromMaybe membership m_` fallback.
|
||||
|
||||
**Fix:** Change to required `GroupMember`:
|
||||
```haskell
|
||||
groupMsgReaction :: GroupInfo -> GroupMember -> SharedMsgId -> Maybe MemberId -> Maybe MsgScope -> MsgReaction -> Bool -> RcvMessage -> UTCTime -> CM (Maybe DeliveryTaskContext)
|
||||
```
|
||||
|
||||
- No `reactor` binding needed — use `m` directly (eliminates `fromMaybe membership m_` fallback)
|
||||
- `ciDir = CIGroupRcv (Just m)` (reactions always attributed to member)
|
||||
- Always return `sentAsGroup = False` — reactions are never from channel
|
||||
- Return type: `Maybe DeliveryTaskContext` (not tuple)
|
||||
|
||||
**Direct handler call site (line 958-960):**
|
||||
```haskell
|
||||
XMsgReact sharedMsgId memberId scope_ reaction add ->
|
||||
groupMsgReaction gInfo' m'' sharedMsgId memberId scope_ reaction add msg brokerTs
|
||||
```
|
||||
|
||||
**Forwarded handler call site (line 3162-3163):**
|
||||
```haskell
|
||||
XMsgReact sharedMsgId memId_ scope_ reaction add ->
|
||||
withAuthor XMsgReact_ $ \author -> groupMsgReaction gInfo author sharedMsgId memId_ scope_ reaction add rcvMsg msgTs
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Issue 3: Fix groupMessageUpdate lookup {#issue-3}
|
||||
|
||||
**File:** `src/Simplex/Chat/Library/Subscriber.hs` lines 1973-1994
|
||||
|
||||
**Problem:** Dual-lookup with `catchError` tries `Nothing` first, then falls back to `senderGMId_`. This is wrong — the `asGroup_` flag from XMsgUpdate should drive the search.
|
||||
|
||||
**Fix:** Use `asGroup_` (the wire flag) to determine search strategy. No `senderGMId_` parameter needed:
|
||||
```haskell
|
||||
updateRcvChatItem = do
|
||||
(cci, scopeInfo) <- withStore $ \db -> do
|
||||
cci <- case m_ of
|
||||
Just m -> getGroupMemberCIBySharedMsgId db user gInfo (memberId' m) sharedMsgId
|
||||
Nothing -> getGroupChatItemBySharedMsgId db user gInfo Nothing sharedMsgId
|
||||
(cci,) <$> getGroupChatScopeInfoForItem db vr user gInfo (cChatItemId cci)
|
||||
```
|
||||
|
||||
When `m_ = Nothing` (channel owner as channel), search with `Nothing` group_member_id → finds channel items.
|
||||
When `m_ = Just m` (attributed member message), search with member's `memberId` → finds member items.
|
||||
|
||||
The `isSender` check also simplifies — just check `m_` matches the found item's member.
|
||||
|
||||
**Fallback path** (lines 1948-1968, `catchCINotFound`): When item not found, `showGroupAsSender` is derived from `asGroup_` flag (or defaults based on `m_`), which maps to `sentAsGroup` in the `DeliveryTaskContext`.
|
||||
|
||||
---
|
||||
|
||||
## Issue 4: DeliveryTaskContext type {#issue-4}
|
||||
|
||||
**File:** `src/Simplex/Chat/Delivery.hs`
|
||||
|
||||
### 4a. Add DeliveryTaskContext type
|
||||
```haskell
|
||||
data DeliveryTaskContext = DeliveryTaskContext
|
||||
{ jobScope :: DeliveryJobScope,
|
||||
sentAsGroup :: ShowGroupAsSender
|
||||
}
|
||||
deriving (Show)
|
||||
```
|
||||
|
||||
Uses existing `type ShowGroupAsSender = Bool` from Messages.hs.
|
||||
|
||||
### 4b. Modify existing helpers
|
||||
Rename `infoToDeliveryScope` → `infoToDeliveryContext`, inline the scope logic, add `ShowGroupAsSender` parameter:
|
||||
```haskell
|
||||
infoToDeliveryContext :: GroupInfo -> Maybe GroupChatScopeInfo -> ShowGroupAsSender -> DeliveryTaskContext
|
||||
infoToDeliveryContext GroupInfo {membership} scopeInfo sentAsGroup = DeliveryTaskContext {jobScope, sentAsGroup}
|
||||
where
|
||||
jobScope = case scopeInfo of
|
||||
Nothing -> DJSGroup {jobSpec = DJDeliveryJob {includePending = False}}
|
||||
Just GCSIMemberSupport {groupMember_} ->
|
||||
let supportGMId = groupMemberId' $ fromMaybe membership groupMember_
|
||||
in DJSMemberSupport {supportGMId}
|
||||
```
|
||||
Remove `infoToDeliveryScope` entirely.
|
||||
|
||||
Rename `memberEventDeliveryScope` → `memberEventDeliveryContext`, change return type:
|
||||
```haskell
|
||||
memberEventDeliveryContext :: GroupMember -> Maybe DeliveryTaskContext
|
||||
memberEventDeliveryContext m@GroupMember {memberRole, memberStatus}
|
||||
| memberStatus == GSMemPendingApproval = Nothing
|
||||
| memberStatus == GSMemPendingReview = Just $ DeliveryTaskContext {jobScope = DJSMemberSupport {supportGMId = groupMemberId' m}, sentAsGroup = False}
|
||||
| memberRole >= GRModerator = Just $ DeliveryTaskContext {jobScope = DJSGroup {jobSpec = DJDeliveryJob {includePending = True}}, sentAsGroup = False}
|
||||
| otherwise = Just $ DeliveryTaskContext {jobScope = DJSGroup {jobSpec = DJDeliveryJob {includePending = False}}, sentAsGroup = False}
|
||||
```
|
||||
|
||||
### 4c. Update NewMessageDeliveryTask
|
||||
```haskell
|
||||
data NewMessageDeliveryTask = NewMessageDeliveryTask
|
||||
{ messageId :: MessageId,
|
||||
taskContext :: DeliveryTaskContext
|
||||
}
|
||||
deriving (Show)
|
||||
```
|
||||
|
||||
### 4d. MessageDeliveryTask — no change
|
||||
|
||||
`MessageDeliveryTask` stays as-is. It's constructed from DB rows in `getMsgDeliveryTask_` and consumed by relay forwarding code — those consumers need `jobScope` and `fwdSender` directly, not `DeliveryTaskContext`. `DeliveryTaskContext` is only for the path from processing functions → `NewMessageDeliveryTask` creation.
|
||||
|
||||
### 4e. Update Store/Delivery.hs
|
||||
|
||||
**`createMsgDeliveryTask`** (line 71-87): Extract `jobScope` and `sentAsGroup` from `taskContext` instead of separate `jobScope`/`showGroupAsSender` fields.
|
||||
|
||||
**`getMsgDeliveryTask_`** — no change needed (`MessageDeliveryTask` unchanged).
|
||||
|
||||
### 4f. Consumers of MessageDeliveryTask — no change needed
|
||||
|
||||
**Subscriber.hs** lines ~3325-3333 and **Messages/Batch.hs** lines ~77-80 already pattern match on `FwdSender` and use `jobScope` from `MessageDeliveryTask`. Since `MessageDeliveryTask` is unchanged, no updates needed.
|
||||
|
||||
### 4g. Return type changes in processing functions
|
||||
|
||||
All functions currently returning `(Maybe DeliveryJobScope, ShowGroupAsSender)` change to `Maybe DeliveryTaskContext`:
|
||||
- `groupMessageFileDescription` → `CM (Maybe DeliveryTaskContext)`
|
||||
- `groupMessageUpdate` → `CM (Maybe DeliveryTaskContext)`
|
||||
- `groupMessageDelete` → `CM (Maybe DeliveryTaskContext)`
|
||||
- `xFileCancelGroup` → `CM (Maybe DeliveryTaskContext)`
|
||||
- `groupMsgReaction` → `CM (Maybe DeliveryTaskContext)`
|
||||
|
||||
Events that return `(Nothing, False)` or `(Just scope, False)` are updated:
|
||||
- `(Nothing, False)` → `Nothing`
|
||||
- `(Just scope, False)` → `Just $ DeliveryTaskContext scope False` (or use `memberEventDeliveryContext`)
|
||||
- `(Just scope, showGroupAsSender)` → `Just $ DeliveryTaskContext scope showGroupAsSender` (or use `infoToDeliveryContext`)
|
||||
|
||||
---
|
||||
|
||||
## Issue 5: Fix testChannelReactionAttribution {#issue-5}
|
||||
|
||||
**File:** `tests/ChatTests/Groups.hs` lines 9057-9084
|
||||
|
||||
**Problem:** Comment says "reaction is forwarded as channel (owner is anonymous)" and expects `#team>`. Owner should react **as member** — reactions are always `sentAsGroup = False`.
|
||||
|
||||
**Fix:** Change comment and expectations:
|
||||
```haskell
|
||||
-- owner reacts to own member message - reaction is forwarded as member
|
||||
alice ##> "+1 #team hello"
|
||||
alice <## "added 👍"
|
||||
bob <# "#team alice> > alice hello"
|
||||
bob <## " + 👍"
|
||||
concurrentlyN_
|
||||
[ do cath <# "#team alice> > alice hello"
|
||||
cath <## " + 👍",
|
||||
do dan <# "#team alice> > alice hello"
|
||||
dan <## " + 👍",
|
||||
do eve <# "#team alice> > alice hello"
|
||||
eve <## " + 👍"
|
||||
]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Issue 6: Fix testChannelUpdateFallbackSendAsGroup comment {#issue-6}
|
||||
|
||||
**File:** `tests/ChatTests/Groups.hs` line 9127
|
||||
|
||||
**Problem:** Comment says "bob's internally deleted item is still in DB, update finds it with correct member direction". This is wrong — the item was internally deleted, then XMsgUpdate re-creates it via the `catchCINotFound` fallback.
|
||||
|
||||
**Fix:** Change comment to:
|
||||
```haskell
|
||||
-- bob's internally deleted item is re-created as from member (sendAsGroup=False)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Other: sendAsGroup parameter ordering {#other-issue}
|
||||
|
||||
**Problem:** `sendAsGroup`/`ShowGroupAsSender` should come right after direction/scope, not at the end.
|
||||
|
||||
### 7a. `APIForwardChatItems` constructor
|
||||
|
||||
**File:** `src/Simplex/Chat/Library/Commands.hs` (ChatCommand type definition + parser)
|
||||
|
||||
Current: `APIForwardChatItems toChat fromChat itemIds itemTTL sendAsGroup`
|
||||
New: `APIForwardChatItems toChat sendAsGroup fromChat itemIds itemTTL`
|
||||
|
||||
Affects:
|
||||
- Constructor definition in `src/Simplex/Chat/Controller.hs` line 341
|
||||
- Parser at line 4639
|
||||
- Call sites at lines 930, 2192, 2198, 2204
|
||||
|
||||
### 7b. `createNewSndChatItem`
|
||||
|
||||
**File:** `src/Simplex/Chat/Store/Messages.hs` line 528
|
||||
|
||||
Current: `createNewSndChatItem db user chatDirection msg ciContent quotedItem itemForwarded timed live hasLink showGroupAsSender createdAt`
|
||||
New: `createNewSndChatItem db user chatDirection showGroupAsSender msg ciContent quotedItem itemForwarded timed live hasLink createdAt`
|
||||
|
||||
Move `showGroupAsSender` right after `chatDirection` (direction context).
|
||||
|
||||
Affects call site in `Internal.hs` line 2276.
|
||||
|
||||
### 7c. `saveSndChatItems`
|
||||
|
||||
**File:** `src/Simplex/Chat/Library/Internal.hs` line 2256-2265
|
||||
|
||||
Current param order: `user -> cd -> itemsData -> itemTimed -> live -> showGroupAsSender`
|
||||
New: `user -> cd -> showGroupAsSender -> itemsData -> itemTimed -> live`
|
||||
|
||||
Move `showGroupAsSender` right after `cd` (direction context).
|
||||
|
||||
Affects call sites: Internal.hs line 2242, Commands.hs lines 2561, 2608 (and the `saveSndChatItem'` wrapper at line 2240).
|
||||
|
||||
### 7d. `prepareGroupMsg`
|
||||
|
||||
**File:** `src/Simplex/Chat/Library/Internal.hs` line 203
|
||||
|
||||
Current: `prepareGroupMsg db user gInfo msgScope mc mentions quotedItemId_ itemForwarded fInv_ timed_ live showGroupAsSender`
|
||||
New: `prepareGroupMsg db user gInfo msgScope showGroupAsSender mc mentions quotedItemId_ itemForwarded fInv_ timed_ live`
|
||||
|
||||
Move `showGroupAsSender` right after `msgScope` (scope context).
|
||||
|
||||
Affects call sites: Internal.hs line 1249, Commands.hs line 4094.
|
||||
|
||||
---
|
||||
|
||||
## Forwarded handler (xGrpMsgForward) changes
|
||||
|
||||
**File:** `src/Simplex/Chat/Library/Subscriber.hs` lines 3136-3173
|
||||
|
||||
Add `withAuthor` helper to replace ad-hoc `| Just author <- author_` guards:
|
||||
```haskell
|
||||
where
|
||||
withAuthor :: CMEventTag e -> (GroupMember -> CM ()) -> CM ()
|
||||
withAuthor tag action = case author_ of
|
||||
Just author -> action author
|
||||
Nothing -> messageError $ "x.grp.msg.forward: event " <> tshow tag <> " requires author"
|
||||
```
|
||||
|
||||
Update forwarded event handling:
|
||||
- `XMsgFileDescr` → pass `author_` (Maybe GroupMember) directly
|
||||
- `XMsgUpdate` → pass `author_` directly, void result
|
||||
- `XMsgDel` → pass `author_` directly, void result
|
||||
- `XMsgReact` → use `withAuthor` (required member)
|
||||
- `XFileCancel` → pass `author_` directly
|
||||
- Other events with `| Just author <- author_` → use `withAuthor`
|
||||
|
||||
---
|
||||
|
||||
## Files Modified
|
||||
|
||||
| File | Changes |
|
||||
|------|---------|
|
||||
| `src/Simplex/Chat/Delivery.hs` | Add `DeliveryTaskContext`, update `NewMessageDeliveryTask` only |
|
||||
| `src/Simplex/Chat/Store/Delivery.hs` | Update `createMsgDeliveryTask` to extract from `taskContext` |
|
||||
| `src/Simplex/Chat/Library/Subscriber.hs` | Eliminate `isChannelOwner`/`memberForChannel`/`memberIdForChannel`; change function signatures to return `Maybe DeliveryTaskContext`; add `withAuthor`; simplify `validSender`; `groupMsgReaction` required member; fix lookup |
|
||||
| `src/Simplex/Chat/Controller.hs` | Reorder `sendAsGroup` in `APIForwardChatItems` constructor |
|
||||
| `src/Simplex/Chat/Library/Commands.hs` | Reorder `sendAsGroup` in `APIForwardChatItems` parser + call sites |
|
||||
| `src/Simplex/Chat/Store/Messages.hs` | Reorder `showGroupAsSender` in `createNewSndChatItem` |
|
||||
| `src/Simplex/Chat/Library/Internal.hs` | Reorder `showGroupAsSender` in `saveSndChatItems`, `prepareGroupMsg` |
|
||||
| `src/Simplex/Chat/Messages/Batch.hs` | No change needed (`MessageDeliveryTask` unchanged) |
|
||||
| `tests/ChatTests/Groups.hs` | Fix reaction test expectations + update fallback comment |
|
||||
|
||||
---
|
||||
|
||||
## Verification
|
||||
|
||||
1. `cabal build --ghc-options=-O0` — must compile clean
|
||||
2. Run channel test suite: `cabal test simplex-chat-test --test-option='-m "channels"' --ghc-options=-O0`
|
||||
3. Adversarial self-review loop until 2 consecutive clean passes
|
||||
4. Verify no `isChannelOwner` references remain in Subscriber.hs direct handler
|
||||
5. Verify `groupMsgReaction` signature has required `GroupMember` (no Maybe)
|
||||
6. Verify no dual-lookup with `catchError` in `groupMessageUpdate`
|
||||
@@ -0,0 +1,377 @@
|
||||
# Group & Channel Feature Test Coverage Plan
|
||||
|
||||
## Table of Contents
|
||||
|
||||
1. [Executive Summary](#executive-summary)
|
||||
2. [Feature Coverage Matrix](#feature-coverage-matrix)
|
||||
3. [Gap Analysis by Category](#gap-analysis-by-category)
|
||||
4. [Recommended New Tests](#recommended-new-tests)
|
||||
5. [Implementation Roadmap](#implementation-roadmap)
|
||||
|
||||
---
|
||||
|
||||
## Executive Summary
|
||||
|
||||
**Current State:** The test suite in `Groups.hs` provides comprehensive coverage across 120+ scenarios in 14 categories. Core functionality (group CRUD, messaging, member management) is well-tested.
|
||||
|
||||
**Key Gaps Identified:**
|
||||
- Business/contact card group links (untested invitation flow)
|
||||
- Legacy group link auto-accept path
|
||||
- Permission enforcement for `SGFFullDelete`
|
||||
- Error recovery paths (file transfers, database busy, duplicate forwarding)
|
||||
- Moderator-only scoped message delivery (`DJSMemberSupport`)
|
||||
- Edge cases in channel message deletion
|
||||
|
||||
**Risk Assessment:**
|
||||
| Priority | Gap Count | Impact |
|
||||
|----------|-----------|--------|
|
||||
| Critical | 3 | Production failures in business flows |
|
||||
| High | 5 | Feature regressions possible |
|
||||
| Medium | 4 | Edge case handling incomplete |
|
||||
|
||||
**Recommendation:** Add 12 new test scenarios in 3 phases over 2 sprints.
|
||||
|
||||
---
|
||||
|
||||
## Feature Coverage Matrix
|
||||
|
||||
### Legend
|
||||
- ✅ Tested (comprehensive)
|
||||
- ⚠️ Partial (some paths covered)
|
||||
- ❌ Untested
|
||||
|
||||
### Core Group Operations
|
||||
|
||||
| Feature | Status | Test Location | Notes |
|
||||
|---------|--------|---------------|-------|
|
||||
| Group creation | ✅ | `testGroup` | Basic + edge cases |
|
||||
| Group deletion | ✅ | `testGroupDelete*` | Multiple scenarios |
|
||||
| Group naming/description | ✅ | `testUpdateGroupProfile` | |
|
||||
| Group preferences | ✅ | `testGroupPreferences` | Voice, files, etc. |
|
||||
| Group link creation | ✅ | `testGroupLink*` | |
|
||||
| Group link via contact card | ❌ | - | Business links untested |
|
||||
| Legacy auto-accept | ❌ | - | Deprecated path |
|
||||
|
||||
### Message Operations
|
||||
|
||||
| Feature | Status | Test Location | Notes |
|
||||
|---------|--------|---------------|-------|
|
||||
| XMsgNew (send) | ✅ | Multiple | Core flow |
|
||||
| XMsgUpdate (edit) | ✅ | `testGroupMessageUpdate` | |
|
||||
| XMsgDel (delete) | ✅ | `testGroupMessageDelete` | |
|
||||
| XMsgReact | ✅ | `testGroupMsgReaction` | |
|
||||
| XMsgFileDescr | ✅ | `testGroupFileTransfer` | |
|
||||
| Batch messages | ✅ | `testBatch*` | |
|
||||
| Live messages | ✅ | `testGroupLiveMessage` | |
|
||||
| Quote messages | ✅ | `testGroup*Quote*` | |
|
||||
| Duplicate forwarding | ❌ | - | De-dup logic untested |
|
||||
|
||||
### Member Management
|
||||
|
||||
| Feature | Status | Test Location | Notes |
|
||||
|---------|--------|---------------|-------|
|
||||
| Member add | ✅ | `testGroupAddMember*` | |
|
||||
| Member remove | ✅ | `testGroupRemoveMember*` | |
|
||||
| Member roles | ✅ | `testGroupMemberRole*` | |
|
||||
| Member blocking | ✅ | `testGroupBlock*` | |
|
||||
| Member merging | ✅ | `testMergeMemberContact*` | |
|
||||
| Member deletion errors | ❌ | - | Error paths missing |
|
||||
| Contact from member | ✅ | `testCreateMemberContact*` | |
|
||||
|
||||
### Moderation & Full Delete
|
||||
|
||||
| Feature | Status | Test Location | Notes |
|
||||
|---------|--------|---------------|-------|
|
||||
| Moderate message | ✅ | `testGroupModerate*` | |
|
||||
| Block for all | ✅ | `testGroupBlockForAll*` | |
|
||||
| SGFFullDelete enabled | ✅ | `testFullDeleteGroup*` | |
|
||||
| SGFFullDelete restricted | ❌ | - | Permission checks |
|
||||
|
||||
### Channels & Relays
|
||||
|
||||
| Feature | Status | Test Location | Notes |
|
||||
|---------|--------|---------------|-------|
|
||||
| 1-relay delivery | ✅ | `testChannel1Relay*` | |
|
||||
| 2-relay delivery | ✅ | `testChannel2Relay*` | |
|
||||
| Owner-only sending | ✅ | `testChannel*Message*` | |
|
||||
| Identity protection | ✅ | `testChannel*Incognito*` | |
|
||||
| Channel msg delete errors | ❌ | - | Invalid state handling |
|
||||
|
||||
### Scoped Messages (Support Chats)
|
||||
|
||||
| Feature | Status | Test Location | Notes |
|
||||
|---------|--------|---------------|-------|
|
||||
| Single moderator | ✅ | `testSupportChat*` | |
|
||||
| Multi moderator | ✅ | `testSupportChat*Multi*` | |
|
||||
| Member reports | ✅ | `testReportMessage*` | |
|
||||
| Forwarding in scope | ✅ | `testSupportChatForward*` | |
|
||||
| Stats | ✅ | `testSupportChatStats` | |
|
||||
| DJSMemberSupport delivery | ❌ | - | Moderator-only path |
|
||||
|
||||
### Group Links & Invitations
|
||||
|
||||
| Feature | Status | Test Location | Notes |
|
||||
|---------|--------|---------------|-------|
|
||||
| Create/delete link | ✅ | `testGroupLink*` | |
|
||||
| Join via link | ✅ | `testGroupLink*` | |
|
||||
| Link screening | ✅ | `testGroupLink*Screening*` | |
|
||||
| Connection plans | ✅ | `testPlanGroupLink*` | |
|
||||
| Short links | ✅ | `testGroupShortLink*` | |
|
||||
| Business link invitation | ❌ | - | Contact card flow |
|
||||
|
||||
### Error Handling
|
||||
|
||||
| Feature | Status | Test Location | Notes |
|
||||
|---------|--------|---------------|-------|
|
||||
| CEGroupNotJoined | ⚠️ | Implicit | Some coverage |
|
||||
| CEGroupMemberNotFound | ⚠️ | Implicit | Some coverage |
|
||||
| File transfer errors | ❌ | - | Recovery paths |
|
||||
| Database busy | ❌ | - | Retry logic |
|
||||
| Simplex link warnings | ❌ | - | Feature gate |
|
||||
|
||||
### History & Disappearing
|
||||
|
||||
| Feature | Status | Test Location | Notes |
|
||||
|---------|--------|---------------|-------|
|
||||
| History on join | ✅ | `testGroupHistory*` | |
|
||||
| File history | ✅ | `testGroupHistoryFiles` | |
|
||||
| Disappearing messages | ✅ | `testGroupHistoryDisappear*` | |
|
||||
|
||||
---
|
||||
|
||||
## Gap Analysis by Category
|
||||
|
||||
### Critical Priority (Production Impact)
|
||||
|
||||
#### 1. Business Group Link via Contact Card
|
||||
**Location:** `APIAddMember` with `InvitationContact` path
|
||||
**Risk:** Business users cannot invite via contact cards
|
||||
**Current State:** Only `InvitationMember` path tested
|
||||
**Missing Coverage:**
|
||||
- `processGroupInvitation` with `CTContactRequest`
|
||||
- Auto-accept flow for business links
|
||||
- Profile merge on business join
|
||||
|
||||
#### 2. SGFFullDelete Permission Enforcement
|
||||
**Location:** `canFullDelete`, `checkFullDeleteAllowed`
|
||||
**Risk:** Non-admins might delete others' messages
|
||||
**Missing Coverage:**
|
||||
- `SGFFullDelete` set to `FAAdmins` restriction
|
||||
- Error `CECommandError` when non-admin attempts full delete
|
||||
- Role-based permission matrix
|
||||
|
||||
#### 3. DJSMemberSupport Delivery Path
|
||||
**Location:** `deliverGroupMessages`, `groupMsgDeliveryJobs`
|
||||
**Risk:** Support messages not reaching moderators correctly
|
||||
**Missing Coverage:**
|
||||
- `DJSMemberSupport` job creation
|
||||
- Moderator-only broadcast logic
|
||||
- Scope isolation verification
|
||||
|
||||
### High Priority (Feature Regressions)
|
||||
|
||||
#### 4. Channel Message Deletion Errors
|
||||
**Location:** `apiDeleteMemberChatItem`, `deleteGroupChatItemInternal`
|
||||
**Missing Coverage:**
|
||||
- Delete non-existent channel message
|
||||
- Delete by non-owner in channel
|
||||
- `CEInvalidChatItemDelete` error path
|
||||
|
||||
#### 5. Member Deletion Error Paths
|
||||
**Location:** `removeMemberDeleteItem`, `deleteGroupChatItem`
|
||||
**Missing Coverage:**
|
||||
- Delete item for already-removed member
|
||||
- Concurrent deletion race condition
|
||||
- `CEGroupMemberNotFound` specific handling
|
||||
|
||||
#### 6. File Transfer Error Recovery
|
||||
**Location:** `rcvFileError`, `sndFileError`
|
||||
**Missing Coverage:**
|
||||
- Partial transfer resume
|
||||
- `CEFileTransferError` handling
|
||||
- Cleanup on failed transfers
|
||||
|
||||
#### 7. Legacy Group Link Auto-Accept
|
||||
**Location:** `processGroupInvitation`, `autoAcceptGroupLink`
|
||||
**Risk:** Breaking change for older clients
|
||||
**Missing Coverage:**
|
||||
- V1 protocol compatibility
|
||||
- Auto-accept timing
|
||||
|
||||
#### 8. Duplicate Message Forwarding
|
||||
**Location:** `forwardGroupMessage`, `checkDuplicateForward`
|
||||
**Missing Coverage:**
|
||||
- Same message forwarded twice
|
||||
- De-duplication by `sharedMsgId`
|
||||
- UI state consistency
|
||||
|
||||
### Medium Priority (Edge Cases)
|
||||
|
||||
#### 9. Simplex Links Feature Warnings
|
||||
**Location:** `simplexLinkWarning`, `SGFSimplexLinks`
|
||||
**Missing Coverage:**
|
||||
- Warning when feature disabled
|
||||
- Link detection in messages
|
||||
- User preference override
|
||||
|
||||
#### 10. Database Busy Error Handling
|
||||
**Location:** `withTransaction`, `retryOnBusy`
|
||||
**Missing Coverage:**
|
||||
- Concurrent group operations
|
||||
- Retry exhaustion
|
||||
- State consistency after retry
|
||||
|
||||
#### 11. Invalid Channel/Member Scope Errors
|
||||
**Location:** `validateGroupChatScope`, `scopeNotAllowed`
|
||||
**Missing Coverage:**
|
||||
- Member sending to wrong scope
|
||||
- Scope mismatch on receive
|
||||
- `CECommandError "scope not allowed"` path
|
||||
|
||||
#### 12. Contact Card Profile Merge
|
||||
**Location:** `mergeMemberContactProfile`, `updateContactProfile`
|
||||
**Missing Coverage:**
|
||||
- Profile conflict resolution
|
||||
- Image merge logic
|
||||
- Display name precedence
|
||||
|
||||
---
|
||||
|
||||
## Recommended New Tests
|
||||
|
||||
### Phase 1: Critical (Sprint 1)
|
||||
|
||||
```haskell
|
||||
-- Test 1: Business Group Link Invitation
|
||||
testBusinessGroupLinkInvitation :: HasCallStack => TestParams -> IO ()
|
||||
-- Covers: InvitationContact path, CTContactRequest, auto-accept
|
||||
|
||||
-- Test 2: Full Delete Permission Restriction
|
||||
testFullDeletePermissionRestricted :: HasCallStack => TestParams -> IO ()
|
||||
-- Covers: SGFFullDelete FAAdmins, non-admin rejection, CECommandError
|
||||
|
||||
-- Test 3: Moderator-Only Support Delivery
|
||||
testSupportChatModeratorOnlyDelivery :: HasCallStack => TestParams -> IO ()
|
||||
-- Covers: DJSMemberSupport, moderator broadcast, scope isolation
|
||||
```
|
||||
|
||||
### Phase 2: High (Sprint 1-2)
|
||||
|
||||
```haskell
|
||||
-- Test 4: Channel Message Delete Errors
|
||||
testChannelMessageDeleteErrors :: HasCallStack => TestParams -> IO ()
|
||||
-- Covers: non-existent delete, non-owner delete, CEInvalidChatItemDelete
|
||||
|
||||
-- Test 5: Member Deletion Error Paths
|
||||
testMemberDeletionErrorPaths :: HasCallStack => TestParams -> IO ()
|
||||
-- Covers: removed member delete, concurrent delete, CEGroupMemberNotFound
|
||||
|
||||
-- Test 6: File Transfer Error Recovery
|
||||
testGroupFileTransferErrorRecovery :: HasCallStack => TestParams -> IO ()
|
||||
-- Covers: partial resume, CEFileTransferError, cleanup
|
||||
|
||||
-- Test 7: Legacy Group Link Compatibility
|
||||
testLegacyGroupLinkAutoAccept :: HasCallStack => TestParams -> IO ()
|
||||
-- Covers: V1 protocol, auto-accept timing
|
||||
|
||||
-- Test 8: Duplicate Forward Prevention
|
||||
testDuplicateMessageForwardPrevention :: HasCallStack => TestParams -> IO ()
|
||||
-- Covers: duplicate detection, sharedMsgId, UI consistency
|
||||
```
|
||||
|
||||
### Phase 3: Medium (Sprint 2)
|
||||
|
||||
```haskell
|
||||
-- Test 9: Simplex Links Feature Warning
|
||||
testSimplexLinksFeatureWarning :: HasCallStack => TestParams -> IO ()
|
||||
-- Covers: disabled feature warning, link detection
|
||||
|
||||
-- Test 10: Database Busy Retry
|
||||
testGroupOperationsDatabaseBusy :: HasCallStack => TestParams -> IO ()
|
||||
-- Covers: concurrent ops, retry logic, state consistency
|
||||
|
||||
-- Test 11: Scope Validation Errors
|
||||
testGroupChatScopeValidationErrors :: HasCallStack => TestParams -> IO ()
|
||||
-- Covers: wrong scope send, scope mismatch, CECommandError
|
||||
|
||||
-- Test 12: Contact Card Profile Merge
|
||||
testMemberContactProfileMerge :: HasCallStack => TestParams -> IO ()
|
||||
-- Covers: conflict resolution, image merge, name precedence
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Implementation Roadmap
|
||||
|
||||
### Sprint 1 (Week 1-2)
|
||||
|
||||
| Day | Task | Owner | Deliverable |
|
||||
|-----|------|-------|-------------|
|
||||
| 1-2 | Test 1: Business link | - | PR ready |
|
||||
| 3-4 | Test 2: Full delete perms | - | PR ready |
|
||||
| 5 | Test 3: Moderator delivery | - | PR ready |
|
||||
| 6-7 | Test 4: Channel delete errors | - | PR ready |
|
||||
| 8-9 | Test 5: Member delete errors | - | PR ready |
|
||||
| 10 | Integration + Review | - | Merged |
|
||||
|
||||
### Sprint 2 (Week 3-4)
|
||||
|
||||
| Day | Task | Owner | Deliverable |
|
||||
|-----|------|-------|-------------|
|
||||
| 1-2 | Test 6: File error recovery | - | PR ready |
|
||||
| 3-4 | Test 7: Legacy link compat | - | PR ready |
|
||||
| 5-6 | Test 8: Duplicate forward | - | PR ready |
|
||||
| 7-8 | Tests 9-12: Medium priority | - | PR ready |
|
||||
| 9-10 | Final integration + CI | - | Release |
|
||||
|
||||
### Dependencies
|
||||
|
||||
```
|
||||
Test 1 (Business Link) ─┬─> Test 12 (Profile Merge)
|
||||
│
|
||||
Test 3 (Moderator) ─────┴─> Test 11 (Scope Validation)
|
||||
|
||||
Test 4 (Channel Delete) ──> Test 5 (Member Delete)
|
||||
|
||||
Test 6 (File Error) ──────> (standalone)
|
||||
|
||||
Test 7 (Legacy Link) ─────> Test 1 (Business Link)
|
||||
|
||||
Test 8 (Duplicate) ───────> (standalone)
|
||||
|
||||
Tests 9, 10 ──────────────> (standalone)
|
||||
```
|
||||
|
||||
### Success Criteria
|
||||
|
||||
1. **Coverage Target:** 95%+ of identified gaps covered
|
||||
2. **CI Integration:** All tests in nightly suite
|
||||
3. **Documentation:** Test rationale in docstrings
|
||||
4. **No Regressions:** Existing 120+ tests still pass
|
||||
|
||||
### Risk Mitigation
|
||||
|
||||
| Risk | Mitigation |
|
||||
|------|------------|
|
||||
| Test flakiness | Use explicit waits, avoid timing assumptions |
|
||||
| Database state leaks | Ensure proper cleanup in each test |
|
||||
| Protocol version issues | Test both V1 and V2 where applicable |
|
||||
| CI timeout | Parallelize independent tests |
|
||||
|
||||
---
|
||||
|
||||
## Appendix: Test File Locations
|
||||
|
||||
| Test Category | Primary File | Secondary |
|
||||
|---------------|--------------|-----------|
|
||||
| Group Core | `tests/ChatTests/Groups.hs` | - |
|
||||
| Channels | `tests/ChatTests/Groups.hs` | `Channels/` if split |
|
||||
| Support Chats | `tests/ChatTests/Groups.hs` | `ScopedMessages/` if split |
|
||||
| File Transfers | `tests/ChatTests/Files.hs` | `Groups.hs` |
|
||||
| Error Handling | Inline with feature tests | - |
|
||||
|
||||
---
|
||||
|
||||
*Generated: 2026-02-06*
|
||||
*Branch: ep/channel-messages-2*
|
||||
*Coverage baseline: 120+ scenarios, 14 categories*
|
||||
@@ -0,0 +1,368 @@
|
||||
# Plan: Filling Group/Channel Test Coverage Gaps
|
||||
|
||||
## Table of Contents
|
||||
1. [Executive Summary](#executive-summary)
|
||||
2. [Test File Organization](#test-file-organization)
|
||||
3. [Priority 0: Critical Channel Paths](#priority-0-critical-channel-paths)
|
||||
4. [Priority 1: Error and Fallback Paths](#priority-1-error-and-fallback-paths)
|
||||
5. [Priority 2: Scope-Related Features](#priority-2-scope-related-features)
|
||||
6. [Priority 3: Feature Restrictions](#priority-3-feature-restrictions)
|
||||
|
||||
---
|
||||
|
||||
## Executive Summary
|
||||
|
||||
This plan addresses the coverage gaps identified in `groups_test_coverage.md`, focusing exclusively on DSL-based scenario tests using the existing test infrastructure. All tests follow patterns established in `tests/ChatTests/Groups.hs`.
|
||||
|
||||
**Excluded from scope:** JSON serialization tests (per user request).
|
||||
|
||||
**Key gap categories:**
|
||||
- Non-channel-owner members sending in channel groups
|
||||
- Moderation/delete paths in channels (`memberDelete`)
|
||||
- Error fallback paths (`catchCINotFound`)
|
||||
- Member support scope (`GCSIMemberSupport`)
|
||||
- Full-delete feature, live updates, mentions
|
||||
|
||||
---
|
||||
|
||||
## Test File Organization
|
||||
|
||||
All new tests go in `tests/ChatTests/Groups.hs` under existing or new `describe` blocks.
|
||||
|
||||
### New `describe` blocks to add:
|
||||
|
||||
```haskell
|
||||
describe "channel moderation" $ do
|
||||
-- Tests for memberDelete path, channel moderation errors
|
||||
|
||||
describe "channel error paths" $ do
|
||||
-- Tests for catchCINotFound, invalid sender, etc.
|
||||
|
||||
describe "channel mentions" $ do
|
||||
-- Tests for mentions in channel messages
|
||||
|
||||
describe "group full delete feature" $ do
|
||||
-- Tests for SGFFullDelete enabled
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Priority 0: Critical Channel Paths
|
||||
|
||||
### Test 1: `testChannelMemberModerate`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** `describe "channel moderation"`
|
||||
|
||||
**Objective:** Cover `memberDelete` path in `groupMessageDelete` (lines 2016-2076) - moderation of channel messages by admin/owner.
|
||||
|
||||
**Scenario:**
|
||||
1. Create channel with owner (alice) + relay (bob) + members (cath, dan)
|
||||
2. Owner sends channel message
|
||||
3. Admin/owner moderates (deletes) the channel message
|
||||
4. Verify message marked deleted for all members
|
||||
5. Verify moderation event is forwarded
|
||||
|
||||
**Coverage targets:**
|
||||
- `memberDelete` function execution
|
||||
- `moderate` helper with role checks
|
||||
- `delete` with `delMember_` populated
|
||||
|
||||
---
|
||||
|
||||
### Test 2: `testChannelMemberDeleteError`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** `describe "channel error paths"`
|
||||
|
||||
**Objective:** Cover error path `CIChannelRcv -> messageError "x.msg.del: unexpected channel message in member delete"` (line 2036).
|
||||
|
||||
**Scenario:**
|
||||
1. Create channel with owner + relay + member
|
||||
2. Attempt to trigger memberDelete on CIChannelRcv item (malformed delete request)
|
||||
3. Verify error is logged/handled correctly
|
||||
|
||||
**Coverage targets:**
|
||||
- Line 2036: `CIChannelRcv` error case in `memberDelete`
|
||||
|
||||
---
|
||||
|
||||
### Test 3: `testChannelUpdateNotFound`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** `describe "channel error paths"`
|
||||
|
||||
**Objective:** Cover `catchCINotFound` fallback in `groupMessageUpdate` (lines 1950-1969) - update arrives for locally deleted item.
|
||||
|
||||
**Scenario:**
|
||||
1. Create channel with owner + relay + member
|
||||
2. Owner sends message, member receives
|
||||
3. Member locally deletes the message
|
||||
4. Owner updates the message
|
||||
5. Verify member creates new item from update (fallback path)
|
||||
|
||||
**Coverage targets:**
|
||||
- Line 1960: `Nothing -> pure (CDChannelRcv gInfo Nothing, M.empty, Nothing)`
|
||||
- Lines 1951-1969: create-from-update fallback
|
||||
|
||||
---
|
||||
|
||||
### Test 4: `testChannelReactionNotFound`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** `describe "channel error paths"`
|
||||
|
||||
**Objective:** Cover `catchCINotFound` fallback in `groupMsgReaction` (lines 1823-1837) - reaction on locally deleted item.
|
||||
|
||||
**Scenario:**
|
||||
1. Create channel with owner + relay + member
|
||||
2. Owner sends message, member receives
|
||||
3. Member locally deletes the message
|
||||
4. Owner adds reaction
|
||||
5. Verify reaction is handled without crash
|
||||
|
||||
**Coverage targets:**
|
||||
- Lines 1835-1837: channel reaction fallback
|
||||
|
||||
---
|
||||
|
||||
### Test 5: `testChannelForwardedMessages`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** `describe "relay delivery"` (existing)
|
||||
|
||||
**Objective:** Cover `FwdChannel` branch in delivery task (line 3311) and forwarded message parameters.
|
||||
|
||||
**Scenario:**
|
||||
1. Create channel with owner + 2 relays + members
|
||||
2. Send various message types (new, update, delete, reaction)
|
||||
3. Verify all are forwarded through relay chain
|
||||
4. Check forwarded parameters are correctly passed
|
||||
|
||||
**Coverage targets:**
|
||||
- Line 3311: `FwdChannel -> (Nothing, Nothing)`
|
||||
- Lines 3139-3145: forwarded message handlers
|
||||
|
||||
---
|
||||
|
||||
## Priority 1: Error and Fallback Paths
|
||||
|
||||
### Test 6: `testGroupDeleteNotFound`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** `describe "channel error paths"` or existing moderation tests
|
||||
|
||||
**Objective:** Cover delete error when message not found (line 2039).
|
||||
|
||||
**Scenario:**
|
||||
1. Create group with alice, bob
|
||||
2. Bob sends message
|
||||
3. Alice locally deletes it
|
||||
4. Bob broadcasts delete for the same message
|
||||
5. Verify error path is handled
|
||||
|
||||
**Coverage targets:**
|
||||
- Line 2039: `messageError ("x.msg.del: message not found, " <> tshow e)`
|
||||
|
||||
---
|
||||
|
||||
### Test 7: `testGroupInvalidSenderUpdate`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** `describe "channel error paths"`
|
||||
|
||||
**Objective:** Cover `validSender _ _ = False` (line 1874) and update from wrong member error (line 1980).
|
||||
|
||||
**Scenario:**
|
||||
1. Create group with alice, bob, cath
|
||||
2. Bob sends message
|
||||
3. Cath (with spoofed member ID) attempts to update bob's message
|
||||
4. Verify error is thrown
|
||||
|
||||
**Coverage targets:**
|
||||
- Line 1874: `validSender _ _ = False`
|
||||
- Line 1980: `messageError "x.msg.update: group member attempted to update..."`
|
||||
|
||||
---
|
||||
|
||||
### Test 8: `testGroupReactionDisabled`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** existing `describe "group message reactions"`
|
||||
|
||||
**Objective:** Cover reaction disabled path (line 1839).
|
||||
|
||||
**Scenario:**
|
||||
1. Create group with reactions feature disabled
|
||||
2. Member attempts to add reaction
|
||||
3. Verify reaction is rejected
|
||||
|
||||
**Coverage targets:**
|
||||
- Line 1839: `otherwise = pure Nothing` when reactions not allowed
|
||||
|
||||
---
|
||||
|
||||
### Test 9: `testChannelItemNotChanged`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** `describe "channel message operations"` (existing)
|
||||
|
||||
**Objective:** Cover `CEvtChatItemNotChanged` path (lines 2001-2002) - update with same content.
|
||||
|
||||
**Scenario:**
|
||||
1. Create channel with owner + relay + member
|
||||
2. Owner sends message
|
||||
3. Owner "updates" message with identical content
|
||||
4. Verify no change event is emitted
|
||||
|
||||
**Coverage targets:**
|
||||
- Lines 2001-2002: `CEvtChatItemNotChanged` path
|
||||
|
||||
---
|
||||
|
||||
## Priority 2: Scope-Related Features
|
||||
|
||||
### Test 10: `testScopedSupportMentions`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** `describe "group scoped messages"` (existing)
|
||||
|
||||
**Objective:** Cover mentions in scoped support messages (`getRcvCIMentions` with non-empty mentions).
|
||||
|
||||
**Scenario:**
|
||||
1. Create group with alice (owner), bob (member), dan (moderator)
|
||||
2. Bob sends support message mentioning @alice
|
||||
3. Alice receives with mention highlighted
|
||||
4. Verify `userMention` flag is set correctly
|
||||
|
||||
**Coverage targets:**
|
||||
- Line 2316: `getRcvCIMentions` with actual mentions
|
||||
- Line 2319: `sameMemberId mId membership` in userReply check
|
||||
- Lines 279-281: `uniqueMsgMentions` path
|
||||
|
||||
---
|
||||
|
||||
### Test 11: `testMemberChatStats`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** `describe "group scoped messages"` (existing)
|
||||
|
||||
**Objective:** Cover `memberChatStats` function (lines 2323-2330) for both `CDGroupRcv` and `CDChannelRcv` with scope.
|
||||
|
||||
**Scenario:**
|
||||
1. Create group with support enabled
|
||||
2. Member sends support message
|
||||
3. Verify unread stats are updated
|
||||
4. Verify `memberAttentionChange` is computed
|
||||
|
||||
**Coverage targets:**
|
||||
- Lines 2325-2329: `memberChatStats` branches
|
||||
- Line 2621: `memberAttentionChange`
|
||||
|
||||
**Note:** Tests `testScopedSupportUnreadStatsOnRead` and `testScopedSupportUnreadStatsOnDelete` exist but may not cover all branches.
|
||||
|
||||
---
|
||||
|
||||
### Test 12: `testMkGetMessageChatScope`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** `describe "group scoped messages"` (existing)
|
||||
|
||||
**Objective:** Cover `mkGetMessageChatScope` branches (lines 1599-1617).
|
||||
|
||||
**Scenario:**
|
||||
1. Create group with pending member (knocking)
|
||||
2. Pending member sends message with scope
|
||||
3. Verify correct scope resolution
|
||||
4. Test with `isReport mc` content type
|
||||
|
||||
**Coverage targets:**
|
||||
- Line 1601: `Just _scopeInfo` return
|
||||
- Line 1604: `isReport mc` branch
|
||||
- Lines 1610-1617: `sameMemberId` and `otherwise` branches
|
||||
|
||||
---
|
||||
|
||||
## Priority 3: Feature Restrictions
|
||||
|
||||
### Test 13: `testGroupFullDelete`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** new `describe "group full delete feature"`
|
||||
|
||||
**Objective:** Cover `groupFeatureAllowed SGFFullDelete` = True path (line 2067) - `deleteGroupCIs` instead of `markGroupCIsDeleted`.
|
||||
|
||||
**Scenario:**
|
||||
1. Create group with full delete enabled: `/set delete #team on`
|
||||
2. Bob sends message
|
||||
3. Alice (or bob) deletes message
|
||||
4. Verify message is fully deleted (not just marked)
|
||||
|
||||
**Coverage targets:**
|
||||
- Line 2067: `deleteGroupCIs` path
|
||||
- `groupFeatureAllowed SGFFullDelete` returns True
|
||||
|
||||
---
|
||||
|
||||
### Test 14: `testGroupLiveMessage`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Note:** `testGroupLiveMessage` exists but may not cover update path.
|
||||
|
||||
**Objective:** Cover live message update path (line 830 in View.hs, `itemLive == Just True`).
|
||||
|
||||
**Scenario:**
|
||||
1. Create group
|
||||
2. Send live message
|
||||
3. Update live message content
|
||||
4. Verify live update is processed
|
||||
|
||||
**Coverage targets:**
|
||||
- Line 830: `itemLive == Just True && not liveItems -> []`
|
||||
- Live update in `groupMessageUpdate`
|
||||
|
||||
---
|
||||
|
||||
### Test 15: `testGroupVoiceDisabled`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** existing tests or new `describe "group feature restrictions"`
|
||||
|
||||
**Objective:** Cover voice message rejection (line 342 in Internal.hs).
|
||||
|
||||
**Scenario:**
|
||||
1. Create group with voice disabled: `/set voice #team off`
|
||||
2. Member attempts to send voice message
|
||||
3. Verify rejection
|
||||
|
||||
**Coverage targets:**
|
||||
- Line 342: `isVoice mc && not (groupFeatureMemberAllowed SGFVoice m gInfo)`
|
||||
|
||||
---
|
||||
|
||||
### Test 16: `testGroupReportsDisabled`
|
||||
**File:** `tests/ChatTests/Groups.hs`
|
||||
**Add to:** `describe "group member reports"` (existing)
|
||||
|
||||
**Objective:** Cover reports disabled path (line 344 in Internal.hs).
|
||||
|
||||
**Scenario:**
|
||||
1. Create group with reports disabled
|
||||
2. Member attempts to send report
|
||||
3. Verify rejection
|
||||
|
||||
**Coverage targets:**
|
||||
- Line 344: `isReport mc && ... not (groupFeatureAllowed SGFReports gInfo)`
|
||||
|
||||
---
|
||||
|
||||
## Implementation Order
|
||||
|
||||
1. **Phase 1 (P0):** Tests 1-5 - Critical channel paths
|
||||
2. **Phase 2 (P1):** Tests 6-9 - Error and fallback paths
|
||||
3. **Phase 3 (P2):** Tests 10-12 - Scope-related features
|
||||
4. **Phase 4 (P3):** Tests 13-16 - Feature restrictions
|
||||
|
||||
Each test should:
|
||||
- Use existing DSL operators (`##>`, `<#`, `#$>`, etc.)
|
||||
- Follow naming convention `test<Feature><Scenario>`
|
||||
- Include `HasCallStack` constraint
|
||||
- Use appropriate test helpers (`createGroup2`, `createChannel1Relay`, etc.)
|
||||
|
||||
---
|
||||
|
||||
## Dependencies
|
||||
|
||||
- Existing test infrastructure in `ChatTests.Utils`
|
||||
- Helper functions: `createChannel1Relay`, `createGroup2`, `createGroup3`, etc.
|
||||
- DSL operators for assertions
|
||||
|
||||
## Estimated New Tests: 16
|
||||
|
||||
## Files Modified: 1
|
||||
- `tests/ChatTests/Groups.hs`
|
||||
@@ -0,0 +1,441 @@
|
||||
# Group/Channel Test Coverage Analysis
|
||||
|
||||
Coverage run: `cabal test simplex-chat-test --enable-coverage --ghc-options=-O0 --test-options="-m group"`
|
||||
|
||||
Full 164 group tests executed (151 passed, 13 failed due to unrelated issues).
|
||||
|
||||
## Coverage Summary
|
||||
|
||||
After running all group tests:
|
||||
- Expressions: 48%
|
||||
- Alternatives: 33%
|
||||
- Local declarations: 64%
|
||||
- Top-level: 34%
|
||||
|
||||
---
|
||||
|
||||
## What IS Covered (Channel-Specific Paths)
|
||||
|
||||
- `createNewRcvChatItem` with `CDChannelRcv` - channel message creation
|
||||
- `toGroupChatItem` with `showGroupAsSender = True` - channel message reading
|
||||
- `validSender Nothing CIChannelRcv = True` - channel sender validation
|
||||
- `getGroupChatItemBySharedMsgId` with `Nothing` memberId (`IS NOT DISTINCT FROM`)
|
||||
- `toCIDirection CDChannelRcv -> CIChannelRcv`
|
||||
- `toChatInfo CDChannelRcv g s -> GroupChat g s`
|
||||
- `chatItemMember CIChannelRcv -> Nothing`
|
||||
- `viewChatItem` for both `CIGroupRcv` and `CIChannelRcv`
|
||||
- `viewItemReaction` dispatch to `groupReaction` for both constructors
|
||||
- Channel delete happy path (`channelDelete` -> `delete Nothing`)
|
||||
|
||||
---
|
||||
|
||||
## Uncovered Code Paths
|
||||
|
||||
### 1. Subscriber.hs
|
||||
|
||||
#### `processGroupMessage` dispatch (lines 935-972)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 956 | `asGroup == Just True && memberRole' m'' < GROwner` | tickonlyfalse - rejecting non-owner sending as group never tested |
|
||||
| 963 | `ttl` parameter in `groupMessageUpdate` | nottickedoff |
|
||||
| 965 | `scope_` parameter in `groupMsgReaction` | nottickedoff |
|
||||
| 967 | `XFile` handler | nottickedoff |
|
||||
| 970 | `XFileAcptInv` handler | nottickedoff |
|
||||
| 987 | `XGrpPrefs` handler | nottickedoff |
|
||||
| 993 | `BFileChunk` handler | nottickedoff |
|
||||
| 994 | Catch-all `_` for unsupported messages | nottickedoff |
|
||||
|
||||
#### `memberCanSend` / `memberCanSend'` (lines 1446-1454)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 1449 | `memberPending m` part of condition | tickonlytrue - never false |
|
||||
| 1450 | `otherwise` branch (error "member is not allowed to send messages") | nottickedoff |
|
||||
|
||||
#### `newGroupContentMessage` (lines 1876-1940)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 1879 | `vr` parameter in `mkGetMessageChatScope` | nottickedoff |
|
||||
| 1882 | `ft_` and `False` parameters to `prohibitedGroupContent` | nottickedoff |
|
||||
| 1883 | `rejected` helper invocation | nottickedoff |
|
||||
| 1895 | `mentions` parameter for channel messages | nottickedoff |
|
||||
| 1896 | `pure []` for reactions when `sharedMsgId_` is Nothing | nottickedoff |
|
||||
| 1901 | `rejected` function body | nottickedoff |
|
||||
| 1902 | `Just Nothing` for timed_ when forwarded | nottickedoff |
|
||||
| 1910 | `M.empty` for mentions when blocked | tickonlyfalse |
|
||||
| 1914 | `gInfo'` and `m'` params to `processFileInv` | nottickedoff |
|
||||
|
||||
#### `groupMessageUpdate` (lines 1943-2002)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 1960 | `Nothing -> pure (CDChannelRcv gInfo Nothing, M.empty, Nothing)` | nottickedoff - channel catchCINotFound |
|
||||
| 1967 | `CDChannelRcv {} -> pure ci'` | nottickedoff |
|
||||
| 1977 | `mentions' = if memberBlocked m then []` | tickonlyfalse |
|
||||
| 1980 | `otherwise -> messageError "x.msg.update: group member attempted to update..."` | nottickedoff |
|
||||
| 1984 | `messageError "x.msg.update: invalid message update"` | nottickedoff |
|
||||
| 2001-2002 | `CEvtChatItemNotChanged` path | nottickedoff |
|
||||
|
||||
#### `groupMessageDelete` (lines 2004-2076)
|
||||
|
||||
**channelDelete path:**
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 2013 | `messageError "x.msg.del: invalid channel message delete"` | nottickedoff |
|
||||
| 2015 | `messageError ("x.msg.del: channel message not found, " <> tshow e)` | nottickedoff |
|
||||
|
||||
**memberDelete path:**
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 2028 | `messageError "x.msg.del: member attempted invalid message delete"` | tickonlyfalse |
|
||||
| 2036 | `CIChannelRcv -> messageError "x.msg.del: unexpected channel message..."` | nottickedoff |
|
||||
| 2039 | `messageError ("x.msg.del: message not found, " <> tshow e)` | tickonlyfalse |
|
||||
| 2041-2042 | `messageError "...message of another member with insufficient..."` | tickonlyfalse |
|
||||
| 2044-2047 | `createCIModeration` scoped moderation path | nottickedoff |
|
||||
|
||||
**moderate helper:**
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 2058 | `messageError "x.msg.del: message of another member with incorrect memberId"` | nottickedoff |
|
||||
| 2059 | `messageError "x.msg.del: message of another member without memberId"` | nottickedoff |
|
||||
| 2062 | `messageError "...insufficient member permissions"` | tickonlyfalse |
|
||||
|
||||
#### `groupMsgReaction` (lines 1818-1860)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 1823-1837 | Entire `catchCINotFound` fallback | nottickedoff |
|
||||
| 1825-1831 | Scoped reaction path for member with scope | nottickedoff |
|
||||
| 1832-1834 | Regular group reaction when item not found | nottickedoff |
|
||||
| 1835-1837 | Channel reaction when item not found | nottickedoff |
|
||||
| 1839 | `otherwise = pure Nothing` when reactions not allowed | tickonlyfalse |
|
||||
| 1859 | `Nothing` return for channel (`isJust m_` is False) | nottickedoff |
|
||||
| 1860 | `pure Nothing` when `ciReactionAllowed` is False | nottickedoff |
|
||||
|
||||
#### `validSender` (lines 1871-1874)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 1872 | `validSender (Just mId) (CIGroupRcv m) = sameMemberId mId m` | nottickedoff |
|
||||
| 1873 | `validSender Nothing CIChannelRcv = True` | **covered** |
|
||||
| 1874 | `validSender _ _ = False` | nottickedoff |
|
||||
|
||||
#### `processForwardedMsg` / `xGrpMsgForward` (lines 3127-3153)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 3133 | `(const Nothing)` wrapper | nottickedoff |
|
||||
| 3139 | `mentions`, `msgScope`, `ttl`, `live`, `True` to `groupMessageUpdate` | nottickedoff |
|
||||
| 3141 | `scope_` and `rcvMsg` to `groupMessageDelete` | nottickedoff |
|
||||
| 3143 | `scope_` to `groupMsgReaction` | nottickedoff |
|
||||
| 3145 | `XInfo` handler when `author_` is Just | nottickedoff |
|
||||
| 3152 | `XGrpPrefs` forwarding | nottickedoff |
|
||||
| 3153 | Catch-all error for unsupported forwarded event | nottickedoff |
|
||||
| 3311 | `FwdChannel -> (Nothing, Nothing)` | nottickedoff |
|
||||
|
||||
---
|
||||
|
||||
### 2. View.hs
|
||||
|
||||
#### `viewChatItem` (line 646)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 555 | `groupNtf user g mention` - `mention` parameter for channel | nottickedoff |
|
||||
| 673 | `showSndItemProhibited to` for `CISndGroupInvitation` | nottickedoff |
|
||||
| 674 | `showSndItem to` fallback for GroupChat | nottickedoff |
|
||||
| 682 | `CIRcvIntegrityError` in group context | nottickedoff |
|
||||
| 683 | `CIRcvGroupInvitation` with `isJust m_` guard | nottickedoff |
|
||||
| 684 | `CIRcvModerated` in group context | nottickedoff |
|
||||
| 685 | `CIRcvBlocked` in group context | nottickedoff |
|
||||
| 686 | `showRcvItem from` fallback | nottickedoff |
|
||||
| 691 | `forwardedFrom` in context computation | nottickedoff |
|
||||
|
||||
#### `viewItemUpdate` (line 798)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 819 | `CIGroupRcv m -> updGroupItem (Just m)` | nottickedoff |
|
||||
| 822 | `CIGroupSnd _ -> []` fallback | nottickedoff |
|
||||
| 825 | `ttyToGroup g scopeInfo` (non-edited send path) | nottickedoff |
|
||||
| 830 | `itemLive == Just True && not liveItems -> []` | tickonlyfalse |
|
||||
| 832 | `_ -> []` fallback for non-message content | nottickedoff |
|
||||
| 834 | `ttyFromGroup g scopeInfo m_` (non-edited receive path) | nottickedoff |
|
||||
| 837 | `forwardedFrom` in context | nottickedoff |
|
||||
| 838 | `groupQuote g` in context | nottickedoff |
|
||||
|
||||
#### `viewItemReaction` (line 890)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 898-899 | `sentByMember' g itemDir` in both CIGroupRcv and CIChannelRcv | nottickedoff |
|
||||
| 913 | `groupReaction _ -> []` (non-message-content fallback) | nottickedoff |
|
||||
| 917 | `else sentBy` branch when `showGroupAsSender` is False | nottickedoff |
|
||||
| 958 | `sentByMember'` function | **entirely nottickedoff** |
|
||||
| 962 | `CIChannelRcv -> Nothing` in sentByMember' | nottickedoff |
|
||||
|
||||
#### `viewItemDelete` (line 869)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 880 | `_ -> prohibited` in GroupChat branch | nottickedoff |
|
||||
|
||||
#### `viewGroupChatItemsDeleted` (line 866)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 158 | `member_` parameter | nottickedoff |
|
||||
| 866 | `maybe "" (\m -> " " <> ttyMember m) member_` - empty string fallback | nottickedoff |
|
||||
| - | Entire function | **entirely nottickedoff** |
|
||||
|
||||
#### `groupScopeInfoStr` (line 2785)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| - | `Just (GCSIMemberSupport {groupMember_}) -> ...` | nottickedoff |
|
||||
| - | `Nothing -> "(support)"` sub-branch | nottickedoff |
|
||||
| - | `Just m -> "(support: " <> viewMemberName m <> ")"` sub-branch | nottickedoff |
|
||||
|
||||
#### Scope info display
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 2768 | `groupScopeInfoStr scopeInfo` in `ttyToGroup` | nottickedoff |
|
||||
| 2779 | `groupScopeInfoStr scopeInfo` in `ttyToGroupEdited` | nottickedoff |
|
||||
| 2782 | `groupScopeInfoStr scopeInfo` in `fromGroupAttention_` | nottickedoff |
|
||||
|
||||
#### Other display functions
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 625 | `GroupChat g scopeInfo -> [" " <> ttyToGroup g scopeInfo]` | nottickedoff |
|
||||
| 766 | `(SMDSnd, GroupChat gInfo _scopeInfo) -> Just $ "you #" <> ...` | nottickedoff |
|
||||
| 767 | `(SMDRcv, GroupChat gInfo _scopeInfo) -> Just $ "#" <> ...` | nottickedoff |
|
||||
| 936 | `viewReactionMembers` | **entirely nottickedoff** |
|
||||
| 1020 | `viewChatCleared` GroupChat branch | nottickedoff |
|
||||
|
||||
---
|
||||
|
||||
### 3. Internal.hs
|
||||
|
||||
#### `saveRcvChatItem'` (lines 2294-2340)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 2288 | `M.empty` for non-group mentions | nottickedoff |
|
||||
| 2299 | `groupMentions` parameters `db` and `membership` | nottickedoff |
|
||||
| 2300 | `_ -> pure (M.empty, False)` for non-group | nottickedoff |
|
||||
| 2303 | `contactChatDeleted cd` | tickonlyfalse |
|
||||
| 2303 | `vr` parameter in `updateChatTsStats` | nottickedoff |
|
||||
| 2304 | `else pure $ toChatInfo cd` | nottickedoff |
|
||||
| 2316 | `getRcvCIMentions` - `db`, `user`, `mentions` parameters | nottickedoff |
|
||||
| 2319 | `sameMemberId mId membership` in userReply check | nottickedoff |
|
||||
| 2320 | `\CIMention {memberId} -> sameMemberId memberId membership` | nottickedoff |
|
||||
| 2311 | `createGroupCIMentions db g ci mentions'` | nottickedoff (mentions always empty) |
|
||||
|
||||
#### `memberChatStats` (line 2323)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 2325-2327 | `CDGroupRcv _g (Just scope) m -> ...` | nottickedoff |
|
||||
| 2328-2329 | `CDChannelRcv _g (Just scope) -> ...` | nottickedoff |
|
||||
| 2330 | `_ -> Nothing` | nottickedoff |
|
||||
| - | Entire function | **entirely nottickedoff** |
|
||||
|
||||
#### `memberAttentionChange` (line 2621)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| - | Entire function | **entirely nottickedoff** |
|
||||
|
||||
#### `getRcvCIMentions` (line 277)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 279 | `not (null ft) && not (null mentions) -> ...` | nottickedoff |
|
||||
| 280 | `uniqueMsgMentions maxRcvMentions mentions $ mentionedNames ft` | nottickedoff |
|
||||
| 281 | `mapM (getMentionedMemberByMemberId db user groupId) mentions'` | nottickedoff |
|
||||
|
||||
#### `uniqueMsgMentions` (line 286)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| - | Entire function | **entirely nottickedoff** |
|
||||
|
||||
#### `prepareGroupMsg` / `quoteData` (line 204)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 209 | `MCForward $ ExtMsgContent ...` forward branch | nottickedoff |
|
||||
| 227 | `CIGroupSnd` with `showGroupAsSender` False | nottickedoff |
|
||||
| 228 | `CIGroupRcv m -> pure (qmc, CIQGroupRcv $ Just m, False, Just m)` | nottickedoff |
|
||||
|
||||
#### `mkGetMessageChatScope` (lines 1599-1617)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 1601 | `groupScope@(_gInfo', _m', Just _scopeInfo) -> pure groupScope` | nottickedoff |
|
||||
| 1604 | `isReport mc -> ...` | tickonlyfalse |
|
||||
| 1610 | `sameMemberId mId membership -> ...` | nottickedoff |
|
||||
| 1614 | `otherwise -> do referredMember <- ...` | nottickedoff |
|
||||
| 1614 | `vr` parameter in `getGroupMemberByMemberId` | nottickedoff |
|
||||
|
||||
#### `mkGroupSupportChatInfo` (line 1620)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| - | Entire function | **entirely nottickedoff** |
|
||||
|
||||
#### Feature checks (tickonlyfalse - never true)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 342 | `isVoice mc && not (groupFeatureMemberAllowed SGFVoice m gInfo)` | tickonlyfalse |
|
||||
| 344 | `isReport mc && ... not (groupFeatureAllowed SGFReports gInfo)` | tickonlyfalse |
|
||||
| 485 | `isACIUserMention deletedChatItem` | tickonlyfalse |
|
||||
| 1593 | `memberPending m` | tickonlyfalse |
|
||||
|
||||
#### `sendGroupMessages` (line 1986)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 1989 | `sendProfileUpdate catchAllErrors eToView` | nottickedoff |
|
||||
| 1995 | `isJust scope = False` branch | nottickedoff |
|
||||
|
||||
---
|
||||
|
||||
### 4. Messages.hs
|
||||
|
||||
#### JSON direction functions - ALL ENTIRELY UNTESTED
|
||||
|
||||
**`jsonCIDirection` (lines 314-321):**
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 315 | `CIDirectSnd -> JCIDirectSnd` | nottickedoff |
|
||||
| 316 | `CIDirectRcv -> JCIDirectRcv` | nottickedoff |
|
||||
| 317 | `CIGroupSnd -> JCIGroupSnd` | nottickedoff |
|
||||
| 318 | `CIGroupRcv m -> JCIGroupRcv m` | nottickedoff |
|
||||
| 319 | `CIChannelRcv -> JCIChannelRcv` | nottickedoff |
|
||||
| 320 | `CILocalSnd -> JCILocalSnd` | nottickedoff |
|
||||
| 321 | `CILocalRcv -> JCILocalRcv` | nottickedoff |
|
||||
|
||||
**`jsonACIDirection` (lines 324-331):**
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 325-331 | All branches including `JCIChannelRcv -> ACID SCTGroup SMDRcv CIChannelRcv` | nottickedoff |
|
||||
|
||||
**`jsonCIQDirection` (lines 646-651):**
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 647 | `CIQDirectSnd -> JCIDirectSnd` | nottickedoff |
|
||||
| 648 | `CIQDirectRcv -> JCIDirectRcv` | nottickedoff |
|
||||
| 649 | `CIQGroupSnd -> JCIGroupSnd` | nottickedoff |
|
||||
| 650 | `CIQGroupRcv (Just m) -> JCIGroupRcv m` | nottickedoff |
|
||||
| 651 | `CIQGroupRcv Nothing -> JCIChannelRcv` | nottickedoff |
|
||||
|
||||
**`jsonACIQDirection` (lines 654-661):**
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 655-659 | All branches including `JCIChannelRcv -> Right $ ACIQDirection SCTGroup $ CIQGroupRcv Nothing` | nottickedoff |
|
||||
| 660 | `JCILocalSnd -> Left "unquotable"` | nottickedoff |
|
||||
| 661 | `JCILocalRcv -> Left "unquotable"` | nottickedoff |
|
||||
|
||||
**ToJSON/FromJSON instances:**
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 1469-1470 | `CIDirection` ToJSON | nottickedoff |
|
||||
| 1473 | `CCIDirection` FromJSON | nottickedoff |
|
||||
| 1476 | `ACIDirection` FromJSON | nottickedoff |
|
||||
| 1479 | `CIQDirection` FromJSON | nottickedoff |
|
||||
| 1482-1483 | `CIQDirection` ToJSON | nottickedoff |
|
||||
|
||||
#### Other Messages.hs functions
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 372-375 | `chatItemRcvFromMember` | partially covered - `_ -> Nothing` nottickedoff |
|
||||
| 403 | `toCIDirection CDLocalRcv _ -> CILocalRcv` | nottickedoff |
|
||||
| 413 | `toChatInfo CDLocalRcv l -> LocalChat l` | nottickedoff |
|
||||
| 486 | `aChatItemRcvFromMember` | nottickedoff |
|
||||
| 665 | `quoteMsgDirection CIQDirectSnd -> MDSnd` | nottickedoff |
|
||||
| 666 | `quoteMsgDirection CIQDirectRcv -> MDRcv` | nottickedoff |
|
||||
|
||||
---
|
||||
|
||||
### 5. Store/Messages.hs
|
||||
|
||||
#### Scope-filtered query functions - ALL ENTIRELY UNTESTED
|
||||
|
||||
| Function | Lines | Status |
|
||||
|----------|-------|--------|
|
||||
| `findGroupChatPreviews_` | 862-900 | nottickedoff |
|
||||
| `getChatContentTypes` | 1183-1197 | nottickedoff |
|
||||
| `getChatItemIDs` | 1476-1505 | nottickedoff |
|
||||
| `queryUnreadGroupItems` | 1686-1707 | nottickedoff |
|
||||
| `updateSupportChatItemsRead` | 2038-2077 | nottickedoff |
|
||||
| `getGroupUnreadTimedItems` | 2080-2102 | nottickedoff |
|
||||
| `getGroupMemberCIBySharedMsgId` | 2950-2960 | nottickedoff |
|
||||
|
||||
#### `toGroupChatItem` (lines 2327-2337)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 2329 | `CIChannelRcv` with file | **covered** |
|
||||
| 2332 | `CIChannelRcv` without file | **covered** |
|
||||
| 2334 | `CIGroupRcv member` with file | nottickedoff |
|
||||
| 2336 | `CIGroupRcv member` without file | nottickedoff |
|
||||
| 2337 | `badItem` fallback | nottickedoff |
|
||||
| 2321 | `deletedByGroupMember_` parsing | nottickedoff |
|
||||
|
||||
#### `getChatItemQuote_` CDChannelRcv (lines 648-653)
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 651 | `mId == userMemberId` check | nottickedoff |
|
||||
| 651 | `getUserGroupChatItemId_` call | nottickedoff |
|
||||
| 652 | `otherwise` fallback | nottickedoff |
|
||||
| 653 | `_ -> pure . ciQuote Nothing $ CIQGroupRcv Nothing` | **covered** |
|
||||
|
||||
#### Reaction functions
|
||||
|
||||
| Line | Code | Status |
|
||||
|------|------|--------|
|
||||
| 3275 | `getGroupCIReactions` | **covered** |
|
||||
| 3328 | `deleteGroupCIReactions_` | nottickedoff |
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
### Well-tested channel paths:
|
||||
- Channel message create/read/delete happy paths
|
||||
- Basic channel reactions
|
||||
- Channel quote creation (quoting nothing)
|
||||
- `validSender Nothing CIChannelRcv`
|
||||
- `getGroupChatItemBySharedMsgId` with `Nothing` memberId
|
||||
|
||||
### Major gaps:
|
||||
|
||||
1. **Non-channel-owner member in channel groups** - `isChannelOwner` always True, `memberForChannel = Just m''` never executed
|
||||
|
||||
2. **All JSON serialization for CI directions** - `jsonCIDirection`, `jsonACIDirection`, `jsonCIQDirection`, `jsonACIQDirection` and all `ToJSON`/`FromJSON` instances entirely untested
|
||||
|
||||
3. **Member support scope (`GCSIMemberSupport`)** - `mkGroupSupportChatInfo`, `groupScopeInfoStr`, `memberChatStats` entirely untested
|
||||
|
||||
4. **Mentions in channel/group messages** - `getRcvCIMentions` with non-empty mentions, `uniqueMsgMentions`, `createGroupCIMentions` never called
|
||||
|
||||
5. **Error/fallback paths** - `catchCINotFound` in update/delete/reaction, invalid sender validation, permission errors
|
||||
|
||||
6. **Full-delete feature** - `groupFeatureAllowed SGFFullDelete` always false, `deleteGroupCIs` never called
|
||||
|
||||
7. **Live message updates** - `itemLive == Just True` always false
|
||||
|
||||
8. **Forwarded message handling** - Most parameters to forwarded handlers untested, `FwdChannel` branch untested
|
||||
|
||||
9. **View functions** - `sentByMember'`, `viewGroupChatItemsDeleted`, `viewReactionMembers` entirely untested
|
||||
|
||||
10. **Scope-filtered store queries** - 7 functions entirely untested
|
||||
|
||||
11. **Feature restriction checks** - Voice messages (`SGFVoice`), reports (`SGFReports`) feature checks never triggered
|
||||
Reference in new issue
Block a user