Create the spec.
https://linear.app/comm/issue/ENG-8737/create-dmoperations-for-changesettingsmessageinfo
Depends on D12872
Details
Details
Tested that processing the operation with changed name and color results with these values being updated both in Redux and in the DB (on web).
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/shared/dm-ops/change-thread-settings-spec.js | ||
|---|---|---|
| 82–91 ↗ | (On Diff #42823) | Can we instead create a function that calculates the threadInfo for add members operation, and use it here and in addMembersSpec. I really don't like that we have a hidden dependency addMembersSpec implementation details (by checking for UPDATE_THREAD/JOIN_THREAD) |
| 104–117 ↗ | (On Diff #42823) | This looks like validation of the chats name. Can we have a function for this validation? It should probably be used in other places, like when we first get thread infos... |
| 128 ↗ | (On Diff #42823) | It seems odd that all those things like color.toLowerCase() and const name = firstLine(untrimmedName); are performed on each client, not on the one creating the operation. |
| 181 ↗ | (On Diff #42823) | Should we set the unread status? |
Comment Actions
Extract common logic
| lib/shared/dm-ops/change-thread-settings-spec.js | ||
|---|---|---|
| 82–91 ↗ | (On Diff #42823) | Makes sense! |
| 104–117 ↗ | (On Diff #42823) | We're validating this on the client side in the inputs.
Our UI should handle that. This code mostly protects us against clients who send invalid data. |
| 128 ↗ | (On Diff #42823) | I am not sure if we should trust other clients to send the correct data. Created a task https://linear.app/comm/issue/ENG-8907/validate-operations-input to track the validation. |
Comment Actions
Should we set the unread status?
| lib/shared/dm-ops/change-thread-settings-spec.js | ||
|---|---|---|
| 45 ↗ | (On Diff #42933) | Is this to make the ids unique? |
Comment Actions
Yes, you're right!
| lib/shared/dm-ops/change-thread-settings-spec.js | ||
|---|---|---|
| 45 ↗ | (On Diff #42933) | Yes. We perform the same operation on keyservers. I'm not sure if we have to do it, but it also doesn't hurt and is safer. |
Comment Actions
I wonder if it would be better to handle this as a "side effect" of the main processDMOperation. We could take a similar approach to updating the replies count.
We would need to check all of the rawMessageInfos, as well as any messages in the updateInfos, to see if any new messages are introduced that aren't from the viewer. This could potentially be done in useProcessDMOperation. Potentially mergeUpdatesWithMessageInfos could be helpful for extracting the messages from the updates.
@tomek, what do you think? A unified approach would mitigate the risk of the programmer forgetting to handle it in a specific case, and it would reduce code duplication.
Comment Actions
Makes sense! Created a task to track https://linear.app/comm/issue/ENG-8930/unify-the-approach-to-replies-count-and-unread-status-updates, because creating a new diff on top of this stack is more efficient than updating all the diffs.