Page MenuHomePhabricator

Implement updating DM thread subscription
ClosedPublic

Authored by marcin on Sep 4 2024, 2:59 AM.
Tags
None
Referenced Files
F3185904: D13236.id43963.diff
Fri, Nov 8, 1:49 PM
Unknown Object (File)
Sat, Oct 26, 7:26 PM
Unknown Object (File)
Thu, Oct 24, 4:46 AM
Unknown Object (File)
Thu, Oct 24, 2:01 AM
Unknown Object (File)
Thu, Oct 24, 12:38 AM
Unknown Object (File)
Wed, Oct 23, 10:59 PM
Unknown Object (File)
Tue, Oct 22, 6:30 PM
Unknown Object (File)
Tue, Oct 22, 6:30 PM
Subscribers

Details

Summary

This differential implements synchronizing thread subscription in dm threads.

Test Plan
  1. Create dm thread and add another user. (Use UI button implemented by Kamil)
  2. Play around with changing thread subscription and sending notifs.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin requested review of this revision.Sep 4 2024, 3:16 AM
tomek requested changes to this revision.Sep 6 2024, 8:19 AM
tomek added inline comments.
lib/shared/dm-ops/change-thread-subscription.js
38–46 ↗(On Diff #43879)

We need to introduce a new timestamp that should be used to check whether the subscription should be updated.

lib/shared/thread-settings-notifications-utils.js
6–7 ↗(On Diff #43879)

Can be merged

178–180 ↗(On Diff #43879)

We should use dispatchActionPromise so that the status can be tracked.

This revision now requires changes to proceed.Sep 6 2024, 8:19 AM
  1. Address review
  2. Refactor to execute DM logic in hook instead of component to match latest convention
tomek added inline comments.
lib/shared/dm-ops/change-thread-subscription.js
38–46 ↗(On Diff #43879)

Discussed it offline. We don't need to introduce a new timestamp, because we already have one in members[viewerID]. We don't want to duplicate it because then it will become possible to break the consistency.

lib/shared/thread-settings-notifications-utils.js
178–180 ↗(On Diff #43879)

Should we use the dispatchActionPromise?

This revision is now accepted and ready to land.Sep 9 2024, 1:52 AM
lib/shared/thread-settings-notifications-utils.js
178–180 ↗(On Diff #43879)

This question refers to the old version of this diff. Latest version doesn't have this issue anymore.

This revision was landed with ongoing or failed builds.Sep 9 2024, 2:53 AM
This revision was automatically updated to reflect the committed changes.

I have the same concerns here that I highlighted in D13260. @marcin, can you address them as part of the same stack as D13260, and land the fixes together?