mute: Request and consume new user_topic format in API; adapt our model by gnprice · Pull Request #5382 · zulip/zulip-mobile

gnprice · 2022-05-12T06:54:43Z

This implements for mobile the client side of zulip/zulip#21251 / zulip/zulip#21015.

This is a draft PR for two reasons:

The backend implementation making the API changes (user_topic: Send user_topic event when muting or unmuting topics. zulip#21251) isn't yet merged, so the API is still potentially subject to change. (In particular, the feature level where the feature gets introduced will be a rapidly moving target until it's merged, and we'll definitely need to update for that.)
(edit: done) ~~I should write some unit tests, and also test these changes manually against the draft backend.~~

But I believe the code is good, and ready for review. The "wip" markers on some commits correspond to TODO items in the commit messages, basically for the two bullet points mentioned above.

Fixes: #5380

Following Greg's example of defining an enum in zulip#5382.

chrisbobbe

Thanks! This looks good; see a few comments below, including one where I'm not sure what the intended server API quite is.

I'll force-push a rebase, resolving the conflicts, just after posting this review. (I tried doing so before posting, but then went back because I'd written the comments on the old commits.)

I've also posted a review on the server PR, at zulip/zulip#21251 (review).

chrisbobbe · 2022-05-17T18:51:14Z

+    // FlowIssue: sad that we end up having to write numeric literals here :-/
+    //   But the most important thing to get from the type-checker here is
+    //   that the ensureUnreachable works -- that ensures that when we add a
+    //   new possible value, we'll add a case for it here.  Couldn't find a
+    //   cleaner way to write this that still accomplished that.


Is this the same issue as #5384 (comment) ?

Should we leave a note about trying Flow enums with the flowlint rule require-explicit-enum-switch-cases, new in Flow 153?

Is this the same issue as #5384 (comment) ?

Ah, yep. I knew I'd run into that issue recently, went looking and found that other commit 04c5a03, but clearly didn't find the right search because this is what I was actually thinking of 🙂

Should we leave a note about trying Flow enums with the flowlint rule require-explicit-enum-switch-cases, new in Flow 153?

I think the main thing is just Flow enums themselves -- I believe that's already enough that Flow will point out an error if a switch isn't exhaustive.

Yep, here's a demo:

enum E { A, B }; (e: E) => { switch (e) { case E.A: break; // case E.B: break; // default: break; } }

That gives an error at the switch (e):

Incomplete exhaustive check: the member B of enum E [1] has not been considered in check of e. [invalid-exhaustive-check]

If you uncomment either the other case, or the default, the error goes away because now it's exhaustive.

I believe what that flowlint does is just to say also you can't have a default case (without explicitly suppressing the lint rule): you have to name all the enum members, so that when someone adds another member to that enum they definitely get notified of each of the switches on it.

chrisbobbe · 2022-05-17T18:54:52Z

+    //   new possible value, we'll add a case for it here.  Couldn't find a
+    //   cleaner way to write this that still accomplished that.
+    case 0: // UserTopicVisibilityPolicy.None
+      return false;


Does UserTopicVisibilityPolicy.None quite mean "the topic is not muted", i.e., isTopicMuted(…) should be false?

From the doc in the current revision of zulip/zulip#21251:

0 = None. Used in events to indicate that this topic now follows its default user-topic configuration.

I don't know what that default is or where it's defined. Do you think the doc should do more to help me find that out?

Yeah, probably that doc can be clearer.

It does indeed mean that the topic is not muted -- or rather that it's not individually muted. If the stream it's part of is muted, then the messages in the topic will get much the same behavior as if the topic were itself muted: they don't show up in unreads or in all-messages.

Rereading, I think the main issue is that "its default user-topic configuration" is not an accurate description. Rather, what that's trying to say is that the topic now follows the default visibility policy that it inherits from the stream. Which currently just means that it behaves as muted or unmuted according to how the user has the stream.

https://github.com/zulip/zulip/pull/21251/files#r875367071

chrisbobbe · 2022-05-17T19:07:01Z

@@ -0,0 +1,56 @@
+/** General helpers to augment Immutable.js. */


Just to point it out in case it's helpful (I don't have a strong preference): would you prefer src/immutableUtils.js as a name for this? See discussion: #4683 (comment)

Sure, thanks for the reminder of that thought.

chrisbobbe · 2022-05-17T19:10:12Z

+export type InitialDataUserTopic = {|
+  /** When absent (on older servers), read `muted_topics` instead. */
+  // TODO(server-6.0): Introduced in FL 125 or so.
+  +user_topics?: UserTopic[],


Could use $ReadOnlyArray here, right?

Hmm yeah, thanks.

gnprice · 2022-05-17T22:09:11Z

Thanks for the review! Pushed a revision with the two small changes discussed above.

Following Greg's example of defining an enum in zulip#5382.

gnprice · 2022-05-17T23:07:06Z

And added unit tests.

gnprice · 2022-05-18T00:00:04Z

Also tested manually against the draft backend. Left a comment at zulip/zulip#21251 (review) , but all's well as far as the client code is concerned.

gnprice · 2022-05-18T22:46:24Z

(Just resolved rebase conflicts.)

chrisbobbe

Thanks! LGTM (with a few small things below); now I guess we wait till the server PR is merged.

chrisbobbe · 2022-05-18T22:51:48Z

+
+export type UserTopicEvent = {|
+  ...EventCommon,
+  type: typeof EventTypes.user_topic,


nit: read-only

chrisbobbe · 2022-05-18T23:00:47Z

+        user_topics: [makeUserTopic(eg.stream, 'topic', UserTopicVisibilityPolicy.Muted)],
+      });
+      const newState = tryGetActiveAccountState(fullReducer(eg.baseReduxState, action));
+      expect(newState).toBeTruthy();


Can the mute-model code (what this file is responsible for testing) cause this newState to be falsy? I wonder if this could just be an invariant, as here: https://github.com/zulip/zulip-mobile/blob/41e50a740/src/__tests__/permissionSelectors-test.js#L41

Here, and in another case below.

It could. I don't totally love this toBeTruthy solution, because it doesn't narrow the type and so the next line still has to account for the falsy case:

expect(newState).toBeTruthy(); expect(newState && getMute(newState)).toEqual(makeMuteState([[eg.stream, 'topic']]));

But I don't love the invariant solution either, here in a test. The Jest expect API is nice because if the assertion is ever wrong, it gives good informative error output that's helpful for debugging. Also invariant requires a message argument, which is a good thing for error cases in application code -- helpful for sorting out what's happening from a Sentry event or whatever, and for clarifying why the author thought it was an invariant -- but feels redundant here.

I'd probably go for invariant if the narrowing is more necessary, like if the subsequent logic that relied on the condition was more complex so that repeatedly accounting for it was more of a pain. If so, I might go for something like invariant(newState, '');, leaving the message empty.

chrisbobbe · 2022-05-20T17:52:39Z

I think zulip/zulip#21251 is waiting for your re-review: zulip/zulip#21251 (comment)

chrisbobbe · 2022-08-08T20:39:52Z

The new server API having been test-deployed to CZO, I've just rebased, resolving several conflicts, and pushed to your PR branch; please take a look. 🙂

chrisbobbe

While rebasing (see my comment above), I found a few comments to make; see below. And we'll still want to sync the feature-level numbers, etc., after the server work is merged.

chrisbobbe · 2022-08-08T20:46:33Z

      // InitialDataMessage
      max_message_id: 100,

-      // InitialDataMutedTopics


wip mute: Request and consume new user_topic format in API

Commit-message nit:

In this commit, we start requesting the new `user_topic` format for the data describing the "muted topics" feature. On new servers, that causes the new format to appear, and the old to disappear. Describe the API changes, and start consuming the new format.

This seems to be true of the data as we receive it in the /register response, but we can still get both the old and new shape of event. I don't yet understand why; that doesn't seem to match the doc test-deployed on CZO right now, does it:

Changes: Deprecated in Zulip 6.0 (feature level 134). Starting with this version, clients that explicitly requested the replacement user_topic event type when registering their event queue will not receive this legacy event type.

So, could just update with an explicit mention of the /register response, if the API settles on the current behavior re: the events.

I don't yet understand why

Oh, I think I see: event_types is for explicitly requesting events, and fetch_event_types is for explicitly requesting initial data. We do the initial-data part but leave event_types unspecified, which the server takes as not "explicitly request[ing] the replacement user_topic event type".

Yeah, this is #5395.

chrisbobbe · 2022-08-08T21:13:56Z

+export type InitialDataUserTopic = {|
+  /** When absent (on older servers), read `muted_topics` instead. */
+  // TODO(server-6.0): Introduced in FL 125 or so.
+  +user_topics?: $ReadOnlyArray<UserTopic>,


Here, and in UserTopicEvent, I believe we're asking UserTopic to depart slightly from its jsdoc—

/** * A user's relationship to a topic; in particular, muting. * * Found in the initial data at `user_topics`, and in `user_topic` events: * https://zulip.com/api/get-events#user_topic */ export type UserTopic = {| +stream_id: number, +topic_name: string, +last_updated: number, +visibility_policy: UserTopicVisibilityPolicyT, |};

—to represent a change to a user's relationship to a topic.

Should we make a UserTopicUpdate type, on the pattern of UserStatusUpdate, for use here and in the event?

It seems that the server data we're modeling with InitialDataUserTopic['user_topics'] is a change from the UserTopic "zero value," which has visibility_policy: UserTopicVisibilityPolicy.None. The API doc doesn't make this change-from-zero semantics explicit, but a clue is that it only lists one of the two values for visibility_policy: the non-"zero" value. From the doc as test-deployed on CZO now (formatting/links lost):

visibility_policy: integer
An integer indicating the user's visibility configuration for the topic.
1 = Muted. Used to record muted topics.

So that could be good to say in the jsdoc.

Currently, the substance of what we'd want as a UserTopicUpdate type would coincide with the current UserTopic type, I think. But if the server wants to send richer data, then they might stop coinciding. E.g., a "user–topic relationship" gains a property "foo" besides "visibility_policy", and the "change to user–topic relationship" payload starts dropping "foo" or "visibility_policy" if the other property changed but it didn't. We see this happening with user statuses.

Here, and in UserTopicEvent, I believe we're asking UserTopic to depart slightly from its jsdoc […] to represent a change to a user's relationship to a topic.

Here in the initial data, I don't see it as representing a change in anything: it's telling us what the relationship is now, as of the snapshot represented by this /register response.

In the user_topic event, it is representing a change, but the way it does so is by providing the new state of that relationship.

It seems that the server data we're modeling with InitialDataUserTopic['user_topics'] is a change from the UserTopic "zero value," which has visibility_policy: UserTopicVisibilityPolicy.None. The API doc doesn't make this change-from-zero semantics explicit, but a clue is that it only lists one of the two values for visibility_policy: the non-"zero" value.

I see. Yeah, I still don't think of this as a change, because there's no particular previous period when the value was something else. The value we're being told is the value that it has now, and that it's had since, as far as we know, time immemorial.

But it's true that it could be thought of as a diff, the result of comparing two values. Still, for each individual user-topic relationship, the diff is represented as simply the entire actual state, if it's present at all. So I think this jsdoc is still accurate, and a good succinct description of what a UserTopic should be thought of as.

I'll add a few words at the two places it's used in the API types, to clarify how those respectively use the UserTopic type.

Currently, the substance of what we'd want as a UserTopicUpdate type would coincide with the current UserTopic type, I think. But if the server wants to send richer data, then they might stop coinciding. E.g., a "user–topic relationship" gains a property "foo" besides "visibility_policy", and the "change to user–topic relationship" payload starts dropping "foo" or "visibility_policy" if the other property changed but it didn't. We see this happening with user statuses.

True. I think we can cross that bridge if/when we come to it. I don't think there are currently any planned features that would add something to a user-topic value other than the visibility policy.

But it's true that it could be thought of as a diff, the result of comparing two values.

Ah, yeah, I think my message would have been less confusing if I'd used the word "diff" instead of "change". 🙂

gnprice · 2023-02-15T00:50:40Z

New revision pushed; now ready for review.

chrisbobbe

Thanks! LGTM with one tiny comment below; please merge at will.

chrisbobbe · 2023-02-22T05:06:47Z

+/**
+ * The "visibility policy" our user has chosen for each topic.
+ *
+ * See jsdoc of UserTopicVisibilityPolicyT and related items for background.


nit: UserTopicVisibilityPolicy (without the T, happily, thanks to Flow enums!)

This just adds type definitions, without changing the types we expect from the server. After this, as the next steps: * we'll migrate the data structure we maintain in our own store to one that can smoothly handle the new API format; * then as a separate step after that, we'll switch to actually consuming the new API format. It's convenient to use this UserTopicVisibilityPolicy type in describing that new data structure's type. Hence we add it early, in this commit, before we actually start requesting the new format (or update our event and initial-data types to reflect doing so).

Ideally we'd rename this to something like `state.userTopic` too. But it's currently a pain to actually successfully rename a state subtree (and not have its ghost return at the old name): zulip#5381. So just keep calling it "mute" for now. Also add tests for the model. Note that we don't have to update any of the existing tests! They use `makeMuteState`, which uses the reducer; so they get the new data structure automatically.

This is a better pattern for reducing import cycles. The explicitness also seems helpful to the reader, though there isn't a big difference.

We'll reuse some of these in other models as we use Immutable more.

In this commit, we start requesting the new `user_topic` format for the data describing the "muted topics" feature. On new servers, that causes the new format to appear, and the old to disappear. Describe the API changes, and start consuming the new format. This should have no user-visible effect -- currently the new format describes exactly the same information as the old. Taking the client plus (a properly behaving) server as one system, this change is therefore NFC. We don't call the change NFC, though, because it does change the client's behavior as witnessed by the server. (This change does switch from stream names to stream IDs in the API... but only for data that we get from the server through the event system, not for data we send or receive in any request outside that system. That means that (unless affected by some server bug) it's always working with a stream name-to-ID mapping that exactly matches what the server had at the time it sent that data. So even if a stream gets renamed concurrently with getting some data here, there's no race and no bug. OTOH when we *change* this at the server on the user's behalf, that does suffer from such a race. But that part of the API was fixed long ago to accept stream IDs; we already have a TODO-server comment, at `api.setTopicMute`, to start relying on that update.) Fixes: zulip#5380

gnprice · 2023-02-27T21:58:04Z

Thanks for the review! Merging with that fix.

gnprice added P1 high-priority api migrations server release goal Things we should try to coordinate with a major Zulip Server release. labels May 12, 2022

gnprice requested a review from chrisbobbe May 12, 2022 06:54

gnprice changed the title ~~wip mute: Request and consume new user_topic format in API; adapt our model~~ mute: Request and consume new user_topic format in API; adapt our model May 12, 2022

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request May 17, 2022

api types: Be more precise about UserOrBot.role type

bdb17ad

Following Greg's example of defining an enum in zulip#5382.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request May 17, 2022

api types: Be more precise about realm_create_web_public_stream_policy

3f7213d

Following Greg's example of defining an enum in zulip#5382.

chrisbobbe mentioned this pull request May 17, 2022

permission selectors: Add, with getCanCreateWebPublicStreams #5384

Merged

chrisbobbe force-pushed the pr-user-topic branch 2 times, most recently from ae93f12 to 056ea85 Compare May 17, 2022 19:12

chrisbobbe reviewed May 17, 2022

View reviewed changes

chrisbobbe force-pushed the pr-user-topic branch from 056ea85 to ae93f12 Compare May 17, 2022 19:16

gnprice force-pushed the pr-user-topic branch from ae93f12 to 914349e Compare May 17, 2022 22:08

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request May 17, 2022

api types: Be more precise about UserOrBot.role type

51ace1c

Following Greg's example of defining an enum in zulip#5382.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request May 17, 2022

api types: Be more precise about realm_create_web_public_stream_policy

55d1e43

Following Greg's example of defining an enum in zulip#5382.

gnprice force-pushed the pr-user-topic branch from 914349e to 5f743fa Compare May 17, 2022 23:06

gnprice mentioned this pull request May 18, 2022

user_topic: Send user_topic event when muting or unmuting topics. zulip/zulip#21251

Merged

gnprice force-pushed the pr-user-topic branch from 5f743fa to b5b68ed Compare May 18, 2022 22:46

chrisbobbe approved these changes May 18, 2022

View reviewed changes

gnprice force-pushed the pr-user-topic branch from b5b68ed to df98100 Compare May 19, 2022 19:58

gnprice force-pushed the pr-user-topic branch from df98100 to ab0b2d9 Compare May 28, 2022 00:28

gnprice removed P1 high-priority server release goal Things we should try to coordinate with a major Zulip Server release. labels Jun 9, 2022

chrisbobbe force-pushed the pr-user-topic branch from ab0b2d9 to 1d461c7 Compare August 8, 2022 20:39

chrisbobbe reviewed Aug 8, 2022

View reviewed changes

chrisbobbe mentioned this pull request Aug 11, 2022

Support unmuted topics in muted streams #5362

Closed

3 tasks

chrisbobbe mentioned this pull request Sep 2, 2022

api: Add getSingleMessage binding for GET messages/{message_id} #5465

Merged

gnprice force-pushed the pr-user-topic branch 3 times, most recently from 787cbab to e118f33 Compare February 15, 2023 00:50

gnprice marked this pull request as ready for review February 15, 2023 00:50

chrisbobbe approved these changes Feb 22, 2023

View reviewed changes

gnprice added 5 commits February 27, 2023 13:52

api types [nfc]: Un-indirect some imports

37b0939

This is a better pattern for reducing import cycles. The explicitness also seems helpful to the reader, though there isn't a big difference.

immutable [nfc]: Move some helpers to a general location

d3dcc9e

We'll reuse some of these in other models as we use Immutable more.

gnprice force-pushed the pr-user-topic branch from e118f33 to deab4ad Compare February 27, 2023 21:58

gnprice merged commit deab4ad into zulip:main Feb 27, 2023

gnprice deleted the pr-user-topic branch February 27, 2023 21:58

		@@ -0,0 +1,56 @@
		/** General helpers to augment Immutable.js. */

Uh oh!

Conversation

gnprice commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented May 17, 2022

Uh oh!

gnprice commented May 17, 2022

Uh oh!

gnprice commented May 18, 2022

Uh oh!

gnprice commented May 18, 2022

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Feb 15, 2023

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice commented May 12, 2022 •

edited

Loading

chrisbobbe left a comment •

edited

Loading

chrisbobbe May 17, 2022 •

edited

Loading

chrisbobbe May 17, 2022 •

edited

Loading

chrisbobbe May 18, 2022 •

edited

Loading

chrisbobbe commented May 20, 2022 •

edited

Loading

chrisbobbe commented Aug 8, 2022 •

edited

Loading

chrisbobbe Aug 8, 2022 •

edited

Loading