Create the spec.
https://linear.app/comm/issue/ENG-8741/create-dmoperations-for-jointhreadmessageinfo
Depends on D12835
Details
Details
- Reviewers
kamil inka - Commits
- rCOMM560297fae9d0: [lib] DMOperationSpec for join thread operation
Just the Flow. Further testing will be performed after it is connected with the rest of the app.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/shared/dm-ops/join-thread-spec.js | ||
|---|---|---|
| 67 ↗ | (On Diff #42739) | This will be handled in https://linear.app/comm/issue/ENG-8763/queue-updates-that-cant-be-currently-applied |
| lib/shared/dm-ops/join-thread-spec.js | ||
|---|---|---|
| 92 ↗ | (On Diff #42741) | Is this needed if currentThreadInfo is explicitly ThickRawThreadInfo? |
| 95 ↗ | (On Diff #42741) | In add members we filtered out the users we already have in members. Should we check if the user joining is already a member of the thread? (they may have been added by someone for example) |
| 103 ↗ | (On Diff #42741) | Should we set the unread status? |
Comment Actions
Create a new thread
| lib/shared/dm-ops/join-thread-spec.js | ||
|---|---|---|
| 95 ↗ | (On Diff #42741) | Good point! I think we should filter them out when creating an update. But we should create the message regardless, so that each client has all the message infos. |
| lib/shared/dm-ops/join-thread-spec.js | ||
|---|---|---|
| 103 ↗ | (On Diff #42741) | I'm wondering if this isn't more complicated. It can happen that the new message isn't the most recent one. Should we update the unread status in that case? |
| lib/shared/dm-ops/join-thread-spec.js | ||
|---|---|---|
| 41–46 | Doesn't this negate this:
? | |
| 103 ↗ | (On Diff #42741) | Isn't that the case for all ops? |
| lib/shared/dm-ops/join-thread-spec.js | ||
|---|---|---|
| 102 ↗ | (On Diff #43017) | if userIsMember(currentThreadInfo, editorID) we already returned in 49th line. Is this check needed for something? |
Comment Actions
I think we should change the status even if the message is not the newest. This may be a bit confusing, but I think it's better than not being informed at all about a message the user has not seen yet
I think I agree with @inka here
| lib/shared/dm-ops/join-thread-spec.js | ||
|---|---|---|
| 102 ↗ | (On Diff #43017) | I don't think so - we can remove it. |
| lib/shared/dm-ops/join-thread-spec.js | ||
|---|---|---|
| 37 ↗ | (On Diff #43044) | I think the choice of editorID is confusing here. Assuming that this spec is only used for when a user joins a thread out of their own volition, I think a name like joinerID would be less ambiguous, and avoid implying that there might be a separate "editor" other than the joiner |
| 58 ↗ | (On Diff #43044) | It seems like we are potentially setting the thread to unread on the joiner's other devices. Is this intentional? Generally we don't want to set the thread to unread based on the viewer's own actions |
| 83–89 ↗ | (On Diff #43044) | Same concern here |
| lib/shared/dm-ops/join-thread-spec.js | ||
|---|---|---|
| 37 ↗ | (On Diff #43044) | Ok, that makes sense. I wanted to make it consistent with other robotext operations, but in this context joiner is better. |
| 58 ↗ | (On Diff #43044) | Yes, that's correct and it wasn't intentional. Added a comment to https://linear.app/comm/issue/ENG-8930/unify-the-approach-to-replies-count-and-unread-status-updates#comment-875b874d where we are refactoring the approach to be less repetitive. |
| lib/shared/dm-ops/join-thread-spec.js | ||
|---|---|---|
| 37 ↗ | (On Diff #43044) | |