Page MenuHomePhabricator
Differential D13052

[lib] Split add members spec
ClosedPublic
Actions

Authored by tomek on Aug 12 2024, 7:21 AM.
Tags
None
Referenced Files
F2726315: D13052.id43518.diff
Tue, Sep 17, 1:52 AM
Unknown Object (File)
Mon, Sep 16, 3:18 AM
Unknown Object (File)
Sun, Sep 15, 9:07 AM
Unknown Object (File)
Sun, Sep 15, 9:06 AM
Unknown Object (File)
Mon, Sep 9, 4:38 PM
Unknown Object (File)
Mon, Sep 9, 3:18 PM
Unknown Object (File)
Fri, Sep 6, 12:44 PM
Unknown Object (File)
Fri, Sep 6, 5:36 AM
View All 36 Files
Subscribers
ashoat

Details

Reviewers
kamil
inka
Commits
rCOMMd2fee63e6327: [lib] Split add members spec
Summary

Split the spec so that we don't send extraneous data to all the existing members.

https://linear.app/comm/issue/ENG-8929/split-add-members-operation

Test Plan

Checked two scenarios:

  1. Viewer creates a thread and adds a new user to it using add_members operation
  2. Viewer is added to a thread using add_viewer_to_thread_members

In both cases the thread appeared in the store with correct membership array.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek created this revision.Aug 12 2024, 7:21 AM
Herald added a subscriber: ashoat. · View Herald TranscriptAug 12 2024, 7:21 AM
tomek added a child revision: D13053: [lib] Split change thread settings spec.Aug 12 2024, 7:27 AM
Harbormaster completed remote builds in B31048: Diff 43312.Aug 12 2024, 8:40 AM
tomek requested review of this revision.Aug 12 2024, 8:40 AM
inka added inline comments.Mon, Aug 19, 5:41 AM
lib/shared/dm-ops/add-viewer-to-thread-members-spec.js
93 ↗(On Diff #43312)

I don't think that's a correct reason?

tomek updated this revision to Diff 43455.Mon, Aug 19, 7:09 AM

Fix returned validation result

tomek marked an inline comment as done.Mon, Aug 19, 7:09 AM
tomek added inline comments.
lib/shared/dm-ops/add-viewer-to-thread-members-spec.js
93 ↗(On Diff #43312)

Yes, you're right. I added a comment

// The operation is invalid when this condition is false

where I tried to explain this, but a better solution is to add another reason to the response. In the future, this should be handled by our validation.

inka accepted this revision.Mon, Aug 19, 7:11 AM
This revision is now accepted and ready to land.Mon, Aug 19, 7:11 AM
Harbormaster completed remote builds in B31142: Diff 43455.Mon, Aug 19, 7:26 AM
marcin added a parent revision: D13050: Extract logic of pushInfo -> usersToCollapsableNotifInfo to lib and use for client-based notifs.Tue, Aug 20, 7:42 AM
tomek removed a parent revision: D13050: Extract logic of pushInfo -> usersToCollapsableNotifInfo to lib and use for client-based notifs.Tue, Aug 20, 9:28 AM
Closed by commit rCOMMd2fee63e6327: [lib] Split add members spec (authored by tomek). · Explain WhyTue, Aug 20, 9:33 AM
This revision was automatically updated to reflect the committed changes.
tomek marked an inline comment as done.
tomek added a commit: rCOMMd2fee63e6327: [lib] Split add members spec.
marcin added a parent revision: D13050: Extract logic of pushInfo -> usersToCollapsableNotifInfo to lib and use for client-based notifs.Tue, Aug 20, 1:16 PM
marcin removed a parent revision: D13050: Extract logic of pushInfo -> usersToCollapsableNotifInfo to lib and use for client-based notifs.Wed, Aug 21, 4:19 AM

Revision Contents
Changeset List

Diff 43518

View Options

lib/shared/dm-ops/add-members-spec.js

Loading...
View Options

lib/shared/dm-ops/add-viewer-to-thread-members-spec.js

Loading...
View Options

lib/shared/dm-ops/change-thread-settings-spec.js

Loading...
View Options

lib/shared/dm-ops/dm-op-spec.js

Loading...
View Options

lib/shared/dm-ops/dm-op-specs.js

Loading...
View Options

lib/types/dm-ops.js

Loading...