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
F3383119: D13052.id43455.diff
Thu, Nov 28, 1:45 PM
F3383071: D13052.id43518.diff
Thu, Nov 28, 1:25 PM
F3382763: D13052.diff
Thu, Nov 28, 11:55 AM
Unknown Object (File)
Wed, Nov 20, 9:25 PM
Unknown Object (File)
Wed, Nov 20, 9:25 PM
Unknown Object (File)
Fri, Nov 8, 11:06 PM
Unknown Object (File)
Fri, Nov 8, 12:23 PM
Unknown Object (File)
Oct 22 2024, 1:16 PM
View All 53 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.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 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...