Details

Reviewers

ashoat
kamil

Commits

rCOMMf3b200914da3: [keyserver] introduce setSubcriptionToDefault option in changeRole

Summary

This diff introduces a new optional option in changeRole called setSubcriptionToDefault. When setSubcriptionToDefault is set to true the subscription for the membership row will be the default subscription where home and pushNotifs are set to false

Depends on D12065

Test Plan

Please see the demo video below where I use this option to have auto joined communities be set in the background tab

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Jun 14 2024, 2:53 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJun 14 2024, 2:53 PM

update

ginsu retitled this revision from [keyserver] introduce setSubcriptionToBackground option in changeRole to [keyserver] introduce setSubcriptionToDefault option in changeRole.Jun 14 2024, 3:09 PM

ginsu edited the summary of this revision. (Show Details)

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: ashoat, kamil.

ginsu added a parent revision: D12065: [native] introduce AutoJoinCommunityHandler.

ginsu attached a referenced file: F2014805: Screen Recording 2024-06-14 at 1.33.12 PM.mov. (Show Details)

Harbormaster completed remote builds in B29691: Diff 41344.Jun 14 2024, 3:10 PM

Harbormaster completed remote builds in B29692: Diff 41345.Jun 14 2024, 3:22 PM

Harbormaster completed remote builds in B29693: Diff 41346.

ginsu requested review of this revision.Jun 14 2024, 3:22 PM

ashoat requested changes to this revision.Jun 14 2024, 5:15 PM

ashoat added inline comments.

keyserver/src/updaters/thread-permission-updaters.js
85 ↗	(On Diff #41346)	Typo: Subcription should be Subscription Can we actually do this instead here? // This will only be set if the user is joining or leaving the thread // Joined means role > 0 +defaultSubscription: ThreadSubscription, And then pass in `{ home: false, pushNotifs: false }` for `defaultSubscription`. I think this is clearer at the callsite (less ambiguity about what "default" means) and more flexible/powerful.

This revision now requires changes to proceed.Jun 14 2024, 5:15 PM

address comments

ginsu added inline comments.Jun 17 2024, 11:29 PM

keyserver/src/updaters/thread-permission-updaters.js
88 ↗	(On Diff #41430)	Made `defaultSubscription` optional so that devs aren't forced to add this as part of the `ChangeRoleOptions` object if they use another option
915 ↗	(On Diff #41430)	This is used when the membership intent is to leave + in the `deleteMemberships` function below. Feel like renaming this variable more specifically for this use case made sense and is more clear

Harbormaster completed remote builds in B29735: Diff 41430.Jun 17 2024, 11:39 PM

ginsu added a child revision: D12459: [keysever/lib/native] extend joinThread to handle auto join functionality.Jun 17 2024, 11:41 PM

Please undo the rename before landing

keyserver/src/updaters/thread-permission-updaters.js
915 ↗	(On Diff #41430)	Let's rename it back. This isn't just used for `intent === 'leave'`... it's also used for when `intent` is not set, which is why it's considered a default