Subscription api events #382

sirpengi · 2023-11-14T17:05:20Z

Clean up Subscription and ZulipStream: make Subscription extend ZulipStream and move fields around so only Subscription specific fields are in the class definition. Also handles the events subscription/op:add, subscription/op:remove, and subscription/op:update.

Fixes part of #187

sirpengi · 2023-11-14T20:09:51Z

@gnprice This is ready for comments!

gnprice

Thanks! Comments below.

One high-level comment that runs through several of those below: the second commit
130117a model: Refactor Subscription to extend from ZulipStream
combines a change that touches a number of lines of code, almost all of them in a boring way (the refactor to make Subscription extend ZulipStream) with several changes that affect a smaller number of lines of code and in a much more substantive way (in particular, taking several updates to the Zulip server API).

In general one important aspect of clear and coherent commits is that those two kinds of changes should happen in separate commits. The two call for quite different approaches to reading — one of them for rapidly scanning to confirm that some straightforward pattern is reliably followed, and to spot the few lines that aren't part of that pattern, while the other kind of change calls for closer investigation and thought. Keeping them separate therefore helps make both kinds of change easier to read and review.

gnprice · 2023-11-15T05:27:51Z

lib/api/model/model.dart

+  admin(apiValue: 2),
+  member(apiValue: 3),
+  moderator(apiValue: 4),


Suggested change

admin(apiValue: 2),

member(apiValue: 3),

moderator(apiValue: 4),

administrators(apiValue: 2),

fullMembers(apiValue: 3),

moderators(apiValue: 4),

The substantive bit here is that value 3 is "full members"; if you follow the link on that term in the doc, you'll find the definition, which is a smaller set than "members".

Then in general Zulip's style is to avoid abbreviations. … I'm surprised to find this doesn't seem to appear in the zulip.readthedocs.io docs, but the Flutter style guide has a good phrasing of a similar policy:

Unless the abbreviation is more recognizable than the expansion (e.g. XML, HTTP, JSON), expand abbrevations when selecting a name for an identifier.

Hence "administrator" rather than "admin". In general I think I take less of a hard line on this point than Tim does, but here there's really no cost to spelling it out.

And the singular could work, but: given the migration to groups for permissions, which has now begun to get underway, all policies like this one should be thought of as just a constrained way of expressing a group (one where you get to choose from a fixed finite list of possible groups). And the groups are naturally named as the plural of their members. So, "administrators" and so on.

gnprice · 2023-11-15T05:33:50Z

lib/api/model/model.dart

+  static int _readCanRemoveSubscribersGroup(Map json, String key) {
+    return json[key] ?? json['can_remove_subscribers_group_id'];


Should return int?, right?

Relatedly: this should have tests 🙂. I believe this version would throw when neither key is present (so, for servers before FL 142), and a test would catch that.

gnprice · 2023-11-15T05:36:21Z

lib/api/model/model.dart

-  final int? canRemoveSubscribersGroupId; // TODO(server-6)
+  // TODO(server-6): `canRemoveSubscribersGroupId` added in FL 142
+  // TODO(server-8): in FL 197 renamed to `canRemoveSubscribersGroup`
+  @JsonKey(readValue: _readCanRemoveSubscribersGroup)
+  final int? canRemoveSubscribersGroup;
+
+  // TODO(server-8): added in FL 199, was previously only on [Subscription] objects
+  final int? streamWeeklyTraffic;


Let's put these changes (which reflect updates to the API) in a separate commit coming before the commit that reworks the Subscription class.

gnprice · 2023-11-15T05:39:18Z

lib/api/model/model.dart

-  final int? streamWeeklyTraffic;
-
-  final bool inviteOnly;
-  final bool? isWebPublic; // TODO(server-??): doc doesn't say when added


This is a functional change, even though a small one: this field becomes non-nullable, so we start assuming it must be present when deserializing a Subscription. Let's therefore put it in a separate commit from the "Subscription extends ZulipStream" refactor.

Also changed `isWebPublic` to be non-null. It has been a field on subscriptions since at least FL 8 (see zulip/zulip@f93c19ec6 in `zerver/openapi/zulip.yaml` and `gather_subscriptions_helper` in `zerver/lib/actions.py`).

Thanks. I don't understand the last bit, though — what specifically should the reader be looking at in that function? (It's a long function; and searching there for is_web_public doesn't find anything.)

It was a bit of a journey tracking it down, the openapi docs shows the existence of is_web_public on subscriptions but on the /users/me/subscriptions endpoint. To confirm it's in the data bundle from register the journey went from zerver.views.events_register.events_register_backend -> zerver.lib.events.do_events.register -> zerver.lib.events.fetch_initial_state_data and from there tracking down what populates subscriptions. I've amended the comment with more context.

On further exploring, the docs on Subscription model specifically mentions that it merges in Stream.API_FIELDS for clients so that'll be a clearer place to reference.

gnprice · 2023-11-15T05:41:08Z

lib/api/model/model.dart

+/// in <https://zulip.com/api/register-queue>.
+@JsonSerializable(fieldRename: FieldRename.snake)
+class Subscription extends ZulipStream {
+  List<int>? subscribers;


- // final List<int> subscribers; // we register with includeSubscribers false + List<int>? subscribers;

Why this change? Seems clearer to leave this field out, given that it'll never be present; but in any case if we were going to add it, that change should come in a commit that isn't mixed in with the big refactor to this class.

gnprice · 2023-11-15T05:56:56Z

lib/api/model/events.dart

+  @JsonKey(includeToJson: true)
+  String get op;


Does this @JsonKey line have any effect?

I suspect it doesn't — I believe the overrides effectively clobber whatever annotations were or weren't present on the base class.

It doesn't, removed this instance of it

gnprice · 2023-11-15T06:07:40Z

lib/model/store.dart

+    } else if (event is SubscriptionUpdateEvent) {
+      assert(debugLog("server event: subscription/update"));
+      final subscription = subscriptions[event.streamId];
+      if (subscription == null) return;


Suggested change

if (subscription == null) return;

if (subscription == null) return; // TODO(log)

This situation shouldn't be possible, so we should log when it happens (once we set up logging at all).

gnprice · 2023-11-15T06:09:57Z

lib/model/store.dart

+        case SubscriptionProperty.unknown:
+          // unrecognized setting; do nothing


nit: names in comments should match those in code:

Suggested change

case SubscriptionProperty.unknown:

// unrecognized setting; do nothing

case SubscriptionProperty.unknown:

// unrecognized property; do nothing

gnprice · 2023-11-15T06:18:58Z

test/example_data.dart

+    pinToTop: pinToTop ?? false,
+    isMuted: isMuted ?? false,
+    color: color ?? "#FF0000",
+    emailAddress: emailAddress ?? "",


The empty string doesn't look like a very plausible email address. Here's a better version:

Suggested change

emailAddress: emailAddress ?? "",

emailAddress: emailAddress ?? "stream@chat.example",

In general the example data in this file should have the right basic shape for real data, at least to the extent that's easy to do.

gnprice · 2023-11-15T06:20:07Z

test/example_data.dart

+///
+/// We only allow overrides of values specific to the [Subscription], all
+/// other properties are copied from the [ZulipStream] provided.
+Subscription streamSubscription(


Can name this more simply:

Suggested change

Subscription streamSubscription(

Subscription subscription(

because I don't think we'll need an alternate way of making an example subscription.

sirpengi · 2023-11-15T14:18:45Z

@gnprice this is ready for review again!

gnprice · 2023-11-15T21:36:52Z

lib/api/model/model.dart

  // final List<int> subscribers; // we register with includeSubscribers false

-  final int streamPostPolicy; // TODO enum
-  // final bool? isAnnouncementOnly; // deprecated; ignore
-  final String emailAddress;


Remove `emailAddress` field per update in FL 266. Also

FL 226

gnprice · 2023-11-15T21:40:30Z

lib/api/model/model.dart

-  final int? streamWeeklyTraffic;
-
-  final bool inviteOnly;
-  final bool? isWebPublic; // TODO(server-??): doc doesn't say when added


Also changed `isWebPublic` to be non-null. It has been a field on subscriptions since at least FL 8 (see zulip/zulip@f93c19ec6 in `zerver/openapi/zulip.yaml` and `gather_subscriptions_helper` in `zerver/lib/actions.py`).

Thanks. I don't understand the last bit, though — what specifically should the reader be looking at in that function? (It's a long function; and searching there for is_web_public doesn't find anything.)

gnprice · 2023-11-15T21:43:15Z

lib/api/model/model.dart

-  final bool? isWebPublic; // TODO(server-??): doc doesn't say when added
-  final bool historyPublicToSubscribers;
-  final int? messageRetentionDays;
+class Subscription extends ZulipStream {


api: Change Subscription to extend from ZulipStream

This can now say api [nfc]:, which is nice.

gnprice · 2023-11-15T21:45:09Z

lib/api/model/model.dart


-  final int? canRemoveSubscribersGroupId; // TODO(server-6)


gnprice · 2023-11-15T21:52:02Z

lib/api/model/model.dart

  // final bool? inHomeView; // deprecated; ignore

-  final String color;
+  String color;
+  String emailAddress;


I see. I wouldn't describe the fields as grouped by data type — in main they look like this:

// First, fields that are about the stream and not the user's relation to it. // These are largely the same as in [ZulipStream]. // … // final List<int> subscribers; // we register with includeSubscribers false final int streamPostPolicy; // TODO enum // final bool? isAnnouncementOnly; // deprecated; ignore final String emailAddress; final int? canRemoveSubscribersGroupId; // TODO(server-6) // Then, fields that are specific to the subscription, // i.e. the user's relationship to the stream.

So emailAddress was in amidst subscribers and streamPostPolicy and others because they're fields that are about the stream and not the user's relation to it. If we were keeping it, I'd probably want it still next to subscribers for the same reason.

Moot because we're deleting it, though.

gnprice

Thanks for the revision! Generally this looks good now — various small comments above and below.

gnprice · 2023-11-15T21:56:44Z

lib/api/model/model.dart

-  final bool pinToTop;
-
-  final bool isMuted;
+  bool pinToTop;
+  bool isMuted;


nit: this blank line gets deleted in the otherwise-unrelated final commit

I think it's fine with or without the blank line — it's true that pinToTop and isMuted are conceptually related — but the change should happen in a commit that's already touching that code. Probably
488de5f api: Change Subscription to extend from ZulipStream
is the most relevant.

gnprice · 2023-11-15T21:57:40Z

lib/api/model/model.dart

+/// For docs, search for "stream_post_policy"
+/// in <https://zulip.com/api/get-stream-by-id>
+@JsonEnum(valueField: 'apiValue')
+enum StreamPostPolicy{


nit:

Suggested change

enum StreamPostPolicy{

enum StreamPostPolicy {

gnprice · 2023-11-15T22:01:12Z

lib/api/model/model.dart

-  final int streamPostPolicy; // TODO enum
-  // final bool isAnnouncementOnly; // deprecated; ignore
+  final StreamPostPolicy streamPostPolicy;


nit: s/JsonEnum/enum/ in commit message:

api: Change streamPostPolicy to JsonEnum

I wouldn't say this field is now "a JsonEnum" — rather, it's an enum, specifically a Dart enum. And that enum type is annotated with JsonEnum… which isn't so much a name of a kind of enum, but rather is an annotation saying how to relate some enum to JSON.

gnprice · 2023-11-15T22:02:22Z

lib/api/model/model.dart

-    required this.canRemoveSubscribersGroupId,
+    required this.canRemoveSubscribersGroup,
+    required this.streamWeeklyTraffic,


nit:

api: Update ZulipStream for api changes

"for API changes" — in English as opposed to code, "API" is an initialism written in all caps.

gnprice

Thanks @sirpengi for the revision! This now all looks good except there's one piece of the commit structure that's still a bit messy. Let's handle it by just adding to a couple of the commit messages.

Unpacking #382 (comment) and #382 (comment) : as long as the commit that renames canRemoveSubscribersGroupId to canRemoveSubscribersGroup happens at a point where both Stream and Subscription separately define that field, the same commit should handle it for both of them.

That's the cleanest way to arrange things for reading and understanding the commits, because the changes in those two parts of the code are basically two manifestations of the same logical change. So when a commit has just one half of that, it looks incomplete.

In order to not spend too much time polishing the branch from here, let's handle this this time by just explaining in the commit message.

Specifically in the commit that does the first part of that change:
015130a api: Update ZulipStream for API changes
the commit message can just explain that the other half of the canRemoveSubscribersGroup rename, for Subscription, will happen in the next commit. And then the commit message for that next commit:
916319b api: Update Subscription for API changes
can say it's completing that rename.

That then puts the reader on the same page as the author when they're looking at each commit: they know the author is aware it's only half of the change, and they know where to look for the other half.

For future API updates, probably the best general branch structure is to have a separate commit per logical API change — basically a separate commit for each separate feature level at which changes happened. So e.g. here there'd be

a commit for making Subscription.isWebPublic non-nullable, complete with the commit message explaining why that's justified
a commit for the FL 197 rename of canRemoveSubscribersGroupId to canRemoveSubscribersGroup, on both streams and subscriptions
a commit for the FL 199 addition of ZulipStream.streamWeeklyTraffic
a commit for the FL 226 removal of Subscription.emailAddress

in place of the two commits
015130a api: Update ZulipStream for API changes
916319b api: Update Subscription for API changes
.

The main exception to the "one commit per feature level" pattern would be if several changes that are logically closely related are spread across several feature levels. That occasionally happens, typically when a complex product feature is being added and gets merged as several server PRs over time.

Add `streamWeeklyTraffic` and handle rename of `canRemoveSubscribersGroupId`. This rename also affects `Subscription` but will happen in the next commit.

Complete rename of `canRemoveSubscribersGroupId` that was done in `ZulipStream` in previous commit. Also change `isWebPublic` to be non-null. It has been a field on subscriptions since at least FL 8 (see zulip/zulip@f93c19ec6 in `zerver/openapi/zulip.yaml` and the docs attached to `Subscription.API_FIELDS` in `zerver/models.py`).

Add events for subscription with `op` values of `add`, `remove`, and `update`. `peer_add` and `peer_remove` left for zulip#374.

sirpengi · 2023-11-17T10:20:17Z

@gnprice comments handled. And understood about the concept of breaking api updates like this into discrete chunks. I was initially adverse to layering changes across commits that touch the same bits multiple times (and in the case of canRemoveSubscribersGroup in Subscription I introduce edits that ultimately are removed). But I see how organizing it into smaller chunks as you've explained makes it easier to review. Will continue to pursue that goal!

gnprice · 2023-11-17T20:27:25Z

Thanks! Looks good; merging.

(and in the case of canRemoveSubscribersGroup in Subscription I introduce edits that ultimately are removed).

Yeah. For that reason probably the ideal shape for this particular series would be (as mentioned at #382 (comment)) to have the commit
89f3b78 api [nfc]: Change Subscription to extend from ZulipStream
happen before that rename, and before the streamPostPolicy change which also applies to both those classes. Then each commit is still a single, complete, logical API change (or in the case of Subscription extends ZulipStream and enum StreamPostPolicy, a single logical change to how we reflect the API in our code), but those streamPostPolicy and canRemoveSubscribersGroup commits are shorter and don't contain any changes that subsequently get clobbered.

But I see how organizing it into smaller chunks as you've explained makes it easier to review. Will continue to pursue that goal!

Sounds good!

gnprice reviewed Nov 15, 2023

View reviewed changes

gnprice mentioned this pull request Nov 15, 2023

inbox: Add "Inbox" page! #381

Merged

sirpengi force-pushed the pr-api-subscription-events branch 2 times, most recently from 1b8b9f6 to cb251c0 Compare November 15, 2023 13:49

gnprice reviewed Nov 15, 2023

View reviewed changes

sirpengi force-pushed the pr-api-subscription-events branch from cb251c0 to 3ffb88b Compare November 16, 2023 14:08

gnprice reviewed Nov 16, 2023

View reviewed changes

sirpengi force-pushed the pr-api-subscription-events branch from 3ffb88b to 0d149fe Compare November 17, 2023 09:52

sirpengi added 2 commits November 17, 2023 09:55

api: Change streamPostPolicy to enum

3ba74ea

api: Update ZulipStream for API changes

a218132

Add `streamWeeklyTraffic` and handle rename of `canRemoveSubscribersGroupId`. This rename also affects `Subscription` but will happen in the next commit.

sirpengi force-pushed the pr-api-subscription-events branch from 0d149fe to e58cf25 Compare November 17, 2023 09:57

sirpengi added 4 commits November 17, 2023 10:15

api [nfc]: Change Subscription to extend from ZulipStream

89f3b78

api: Make Subscription properties mutable

c717f3d

api: Add subscription events

3686ca2

Add events for subscription with `op` values of `add`, `remove`, and `update`. `peer_add` and `peer_remove` left for zulip#374.

sirpengi force-pushed the pr-api-subscription-events branch from e58cf25 to 3686ca2 Compare November 17, 2023 10:15

gnprice merged commit 3686ca2 into zulip:main Nov 17, 2023
1 check passed

gnprice mentioned this pull request Nov 20, 2023

Unify stream with subscription data #395

Closed

gnprice mentioned this pull request Nov 29, 2023

☂️ Handle all Zulip events to keep our data up to date #135

Open

72 tasks

sirpengi deleted the pr-api-subscription-events branch January 23, 2024 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscription api events #382

Subscription api events #382

sirpengi commented Nov 14, 2023

sirpengi commented Nov 14, 2023

gnprice left a comment

gnprice Nov 15, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

sirpengi Nov 16, 2023

sirpengi Nov 16, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

sirpengi Nov 15, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

sirpengi commented Nov 15, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

gnprice left a comment

gnprice Nov 15, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

gnprice Nov 15, 2023

gnprice left a comment

sirpengi commented Nov 17, 2023

gnprice commented Nov 17, 2023

		static int _readCanRemoveSubscribersGroup(Map json, String key) {
		return json[key] ?? json['can_remove_subscribers_group_id'];

	if (subscription == null) return;
	if (subscription == null) return; // TODO(log)

		case SubscriptionProperty.unknown:
		// unrecognized setting; do nothing

	emailAddress: emailAddress ?? "",
	emailAddress: emailAddress ?? "stream@chat.example",

Subscription api events #382

Subscription api events #382

Conversation

sirpengi commented Nov 14, 2023

sirpengi commented Nov 14, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi commented Nov 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

sirpengi commented Nov 17, 2023

gnprice commented Nov 17, 2023