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
Unknown Object (File)
Tue, Oct 22, 1:16 PM
Unknown Object (File)
Tue, Oct 22, 9:14 AM
Unknown Object (File)
Tue, Oct 22, 8:32 AM
Unknown Object (File)
Thu, Oct 17, 10:25 PM
Unknown Object (File)
Sep 25 2024, 4:12 PM
Unknown Object (File)
Sep 25 2024, 4:12 PM
Unknown Object (File)
Sep 19 2024, 11:47 AM
Unknown Object (File)
Sep 19 2024, 11:31 AM
View All 46 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
No Lint Coverage
Unit
No Test Coverage

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.Aug 19 2024, 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.Aug 19 2024, 7:09 AM

Fix returned validation result

tomek marked an inline comment as done.Aug 19 2024, 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.Aug 19 2024, 7:11 AM
This revision is now accepted and ready to land.Aug 19 2024, 7:11 AM
Harbormaster completed remote builds in B31142: Diff 43455.Aug 19 2024, 7:26 AM
marcin added a parent revision: D13050: Extract logic of pushInfo -> usersToCollapsableNotifInfo to lib and use for client-based notifs.Aug 20 2024, 7:42 AM
tomek removed a parent revision: D13050: Extract logic of pushInfo -> usersToCollapsableNotifInfo to lib and use for client-based notifs.Aug 20 2024, 9:28 AM
Closed by commit rCOMMd2fee63e6327: [lib] Split add members spec (authored by tomek). · Explain WhyAug 20 2024, 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.Aug 20 2024, 1:16 PM
marcin removed a parent revision: D13050: Extract logic of pushInfo -> usersToCollapsableNotifInfo to lib and use for client-based notifs.Aug 21 2024, 4:19 AM

Revision Contents
Changeset List

Diff 43455

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...