This differential implements method to go from DMOperation -> [MessageData] for each dm op spec.
Details
Next diff provides comprehensive testing plan
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
This differential is huge for two reasons:
- Changes in each file can be reviewed independently.
- Changes in each file are similar so if a change is requested for one file it will probably be requested for other files as well. This would be pain in the neck if each file was treated in separate diff since I would have to rebase whole stack each time.
Overall it looks ok, but there are a couple of things that should be fixed, so I'm requesting changes.
lib/shared/dm-ops/add-members-spec.js | ||
---|---|---|
56–58 ↗ | (On Diff #43556) | This is brittle. If we decide to modify createAddNewMembersMessageDatasFromDMOperation we have to remember to update this place, because we need to have different message IDs for different messages. We should address this brittleness by modifying createAddNewMembersMessageDatasFromDMOperation and similar functions in other specs to return a map from ID to message data. In notificationsCreationData we would simply omit the IDs. |
lib/shared/dm-ops/change-thread-settings-spec.js | ||
72 ↗ | (On Diff #43556) | At this point, this function doesn't process the operation, but instead, it creates the message data, which should be reflected in its name. But I'm wondering whether we should change the approach. The problem with the current solution is that we duplicate a sequence of ifs between functions. We could avoid it, if this function returned not only the message data, but also a thread update (which can then be spread with threadInfoToUpdate in processChangeSettingsOperation). |
lib/shared/dm-ops/create-thread-spec.js | ||
127–130 ↗ | (On Diff #43556) | Why do we have to accept a threadColor as a separate argument? It should be possible to generate its value from the operation. |
lib/shared/dm-ops/join-thread-spec.js | ||
57–59 ↗ | (On Diff #43556) | This is brittle, like in add members spec, and some others - we should update all the places. Also, if we return only one message from createMessageDatasFromDMOperation, maybe we can return it instead of an array? |
Address review. I opted for returning single MessageData instead of {[id: string]: MessageData}.
lib/shared/dm-ops/change-thread-settings-spec.js | ||
---|---|---|
110 ↗ | (On Diff #43693) | Could you explain in more detail what needs to be updated here. |
lib/shared/dm-ops/change-thread-settings-spec.js | ||
---|---|---|
110 ↗ | (On Diff #43693) | Sorry for this. I now understood I forgot to assign this property. |