Page MenuHomePhabricator

Implement method to go from DMOperation -> [MessageData] for each dm op spec
ClosedPublic

Authored by marcin on Aug 20 2024, 7:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 11, 10:41 AM
Unknown Object (File)
Sat, Nov 9, 6:50 PM
Unknown Object (File)
Sat, Nov 9, 5:04 PM
Unknown Object (File)
Sat, Nov 9, 3:34 PM
Unknown Object (File)
Sat, Nov 9, 8:46 AM
Unknown Object (File)
Fri, Nov 8, 11:23 PM
Unknown Object (File)
Sun, Nov 3, 9:08 PM
Unknown Object (File)
Thu, Oct 31, 1:57 AM
Subscribers

Details

Summary

This differential implements method to go from DMOperation -> [MessageData] for each dm op spec.

Test Plan

Next diff provides comprehensive testing plan

Diff Detail

Repository
rCOMM Comm
Branch
marcin/eng-8284-new
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

This differential is huge for two reasons:

  1. Changes in each file can be reviewed independently.
  2. 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.
marcin retitled this revision from Implement method to go from DMOperation -> [MessageData] for eacm dm op spec to Implement method to go from DMOperation -> [MessageData] for each dm op spec.Aug 20 2024, 7:57 AM

Make method to return message data optional

  1. Reorder the stack
  2. Change messageDataFromDMOp to notificationsCreationData.
tomek requested changes to this revision.Aug 27 2024, 12:41 AM

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?

This revision now requires changes to proceed.Aug 27 2024, 12:41 AM

Address review. I opted for returning single MessageData instead of {[id: string]: MessageData}.

tomek added inline comments.
lib/shared/dm-ops/change-thread-settings-spec.js
87–91 ↗(On Diff #43693)

I'm wondering if we need these two fields now, Would it be enough to keep just the threadInfoUpdate?

110 ↗(On Diff #43693)

We should update this field

This revision is now accepted and ready to land.Aug 27 2024, 8:40 AM
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.

fix missing avatar property assignment