Page MenuHomePhabricator
Differential D13236

Implement updating DM thread subscription
ClosedPublic
Actions

Authored by marcin on Wed, Sep 4, 2:59 AM.
Tags
None
Referenced Files
F2753158: D13236.diff
Wed, Sep 18, 4:49 PM
F2749985: D13236.diff
Wed, Sep 18, 12:35 PM
F2743173: D13236.id43963.diff
Wed, Sep 18, 3:01 AM
F2740251: D13236.diff
Tue, Sep 17, 11:10 PM
Unknown Object (File)
Tue, Sep 17, 12:01 AM
Unknown Object (File)
Mon, Sep 16, 8:13 AM
Unknown Object (File)
Mon, Sep 16, 4:03 AM
Unknown Object (File)
Sun, Sep 15, 10:45 PM
View All 35 Files
Subscribers
ashoat

Details

Reviewers
kamil
tomek
Commits
rCOMMa0bb60037921: Implement updating DM thread subscription
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
Branch
marcin/eng-8522
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Wed, Sep 4, 2:59 AM
Herald added a subscriber: ashoat. · View Herald TranscriptWed, Sep 4, 2:59 AM
Harbormaster completed remote builds in B31424: Diff 43879.Wed, Sep 4, 3:16 AM
marcin requested review of this revision.Wed, Sep 4, 3:16 AM
tomek requested changes to this revision.Fri, Sep 6, 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.Fri, Sep 6, 8:19 AM
marcin updated this revision to Diff 43943.Fri, Sep 6, 10:34 AM
  1. Address review
  2. Refactor to execute DM logic in hook instead of component to match latest convention
Harbormaster completed remote builds in B31465: Diff 43943.Fri, Sep 6, 10:52 AM
marcin updated this revision to Diff 43963.Mon, Sep 9, 1:17 AM

Move dm logic to hook.

Harbormaster failed remote builds in B31480: Diff 43963!Mon, Sep 9, 1:42 AM
tomek accepted this revision.Mon, Sep 9, 1:52 AM
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.Mon, Sep 9, 1:52 AM
marcin added inline comments.Mon, Sep 9, 2:02 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.

marcin updated this revision to Diff 43966.Mon, Sep 9, 2:51 AM

Rebase before landing

This revision was landed with ongoing or failed builds.Mon, Sep 9, 2:53 AM
Closed by commit rCOMMa0bb60037921: Implement updating DM thread subscription (authored by marcin). · Explain Why
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMMa0bb60037921: Implement updating DM thread subscription.
Harbormaster completed remote builds in B31483: Diff 43966.Mon, Sep 9, 3:15 AM
ashoat added a comment.Mon, Sep 9, 12:56 PM

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?

Revision Contents
Changeset List

PathSize
lib/
actions/
59 lines
shared/
dm-ops/
9 lines
2 lines

Diff 43966

View Options

lib/actions/user-actions.js

Loading...
View Options

lib/shared/dm-ops/change-thread-subscription.js

Loading...
View Options

lib/shared/thread-settings-notifications-utils.js

Loading...