api: Sync updateStream and createStream with their API docs #5341

chrisbobbe · 2022-04-18T21:18:04Z

After #5339, on the path to fixing #5250.

gnprice

Thanks @chrisbobbe! Generally this looks good. Comments below.

gnprice · 2022-04-19T03:54:49Z

src/api/modelTypes.js

 export type Stream = {|
+  // Based on the getStreams response, current to FL 121:
+  //   https://zulip.com/api/get-streams#response


Hmm interesting. How does this compare with the type we see on streams in the /register response?

The objects we actually get from the server and treat as Stream are from there.

I've just checked, and it's identical in all these properties, thank goodness. I'll update the comment and commit message.

it's identical in all these properties, thank goodness.

Still, though, could even not mention GET /streams, right? If we start using GET /streams, we'll want to check the doc again at that time, to be sure, anyway.

it's identical in all these properties, thank goodness.

Still, though, could even not mention GET /streams, right? If we start using GET /streams, we'll want to check the doc again at that time, to be sure, anyway.

Yeah, seems right.

gnprice · 2022-04-19T04:06:45Z

src/streams/streamsActions.js

-    // Adapt to a server API change that was accidentally incompatible:
-    //   https://github.com/zulip/zulip-mobile/pull/4748#issuecomment-852254404
-    //   https://github.com/zulip/zulip-mobile/issues/4747#issuecomment-946362729
-    // TODO(#4659): Ideally this belongs inside `api.updateStream`.
-    // TODO(server-4.0): Simplify this (if it hasn't already moved.)
+    // See implementation comments in api.updateStream.
    getZulipFeatureLevel(state) >= 64 ? value : JSON.stringify(value);


This new comment seems not specific enough to find what it's referring to and be sure you've found the thing that was intended -- there are a lot of comments on api.updateStream now 🙂

Perhaps mention it's specifically the comment on description and new_name?

Hmm, also the new comment seems backward. The issue is that old servers required the superfluous JSON.stringify; new servers require that the parameter be simply the desired string.

gnprice · 2022-04-19T04:12:16Z

src/api/streams/updateStream.js

+    // TODO(server-4.0): For these two, it's no longer acceptable to give
+    //   the server values without `JSON.stringify`ing them first. There was
+    //   a server API change that was accidentally incompatible:
+    //     https://github.com/zulip/zulip-mobile/issues/4747#issuecomment-946362729
+    //     https://github.com/zulip/zulip-mobile/pull/4748#issuecomment-852254404
+    description?: $PropertyType<Stream, 'description'>,
+    new_name?: $PropertyType<Stream, 'name'>,


I think perhaps the simplest solution for this comment is to say:

Suggested change

// TODO(server-4.0): For these two, it's no longer acceptable to give

// the server values without `JSON.stringify`ing them first. There was

// a server API change that was accidentally incompatible:

// https://github.com/zulip/zulip-mobile/issues/4747#issuecomment-946362729

// https://github.com/zulip/zulip-mobile/pull/4748#issuecomment-852254404

description?: $PropertyType<Stream, 'description'>,

new_name?: $PropertyType<Stream, 'name'>,

// TODO(#4659): Once we pass the feature level to API methods, this one

// should encapsulate a switch at FL 64 related to these two parameters.

// See call sites.

description?: $PropertyType<Stream, 'description'>,

new_name?: $PropertyType<Stream, 'name'>,

and delete the TODO-4659 comment on the method as a whole -- i.e. effectively move it here -- and then leave the comment at the call site unchanged.

Then for other things that should be encapsulated, a one-line comment like
// TODO(#4659): Encapsulate this.
can suffice, because the TODO-server comments already explain what changed between versions. I think that means is_announcement_only and 'forever'?

gnprice · 2022-04-19T04:23:44Z

src/api/streams/createStream.js

+    // TODO(server-5.0): New in FL 53
+    is_web_public?: $PropertyType<Stream, 'is_web_public'>,


Do you mean FL 98 here?

gnprice · 2022-04-19T04:24:03Z

src/api/streams/createStream.js

+    // TODO(server-3.0): Replaced in FL 1 by 'stream_post_policy'.
+    // Commented out because this isn't actually in the doc. It probably
+    // exists though? Copied from api.updateStream.
+    // is_announcement_only?: $PropertyType<Stream, 'is_announcement_only'>,


Huh, curious.

…ties Instead of up to three in sequence, one per changed property.

… type

… 121

…logic We'll be adding more stream properties here soon, and it'll be best to avoid multiplying the number of different names.

One for stream attributes, and one for miscellaneous options. The create-stream API is a little awkward in that it piggy-backs on the subscribe-to-stream API. Here's the doc: https://zulip.com/api/create-stream > You can create a stream using Zulip's REST API by submitting a > [subscribe](/api/subscribe) request with a stream name that > doesn't yet exist and passing appropriate parameters to define the > initial configuration of the new stream. So, make our code a bit clearer by bundling the attributes of the to-be-created stream into an object. We're about to add more attributes, to sync with the API doc. While we're at it, order the attributes by their appearance in the doc.

…inding I think the only good reason to have such fallback code at this layer would be that we don't trust the server to use reasonable fallbacks itself. There are certainly awkward things about this endpoint -- it's doing double duty as "subscribe-to-stream" and "create-a-stream" -- but I see no reason to distrust the server's defaults for these params, which set attributes on a created stream. Its choices currently match ours, but they could reasonably change, and we'd be fine deferring to the server if they did. NFC because, currently, this binding's only caller also applies the same fallbacks. That might be a bad choice, but at least now that choice isn't forced by the code that's designed to interface directly with the server.

With similar reasoning as in the previous commit, except these are in `options` instead of `streamAttributes`. This time, one of the properties doesn't have a default applied by the single caller: `announce`. So this is a behavior change: we're now sometimes omitting `announce`, which lets the server decide what behavior to fall back on. Currently, it's documented as falling back to `false` (and that should apply for all non-ancient servers): https://zulip.com/api/subscribe#parameter-announce

This seems like a fine place to let the server control the behavior, and it's documented to give the same behavior anyway. See https://zulip.com/api/subscribe#parameter-announce : > […] If not provided, then the requesting user/bot is subscribed. We can start passing `principals` again when we eventually have a UI for choosing multiple users to be subscribed, as the web app does. For now, it's redundant and unnecessary for us to request the same behavior the server would give anyway. Especially since we're asking for it in a way that's discouraged: we use an email instead of a user ID.

Grepping shows that we actually use this in several places now.

Related: zulip#3764

… type

chrisbobbe · 2022-04-19T18:34:04Z

Thanks for the review! Revision pushed.

gnprice · 2022-04-19T18:56:23Z

Thanks! Looks good -- merging.

chrisbobbe added the P1 high-priority label Apr 18, 2022

chrisbobbe requested a review from gnprice April 18, 2022 21:18

chrisbobbe force-pushed the pr-stream-api-sync branch from cd5d83d to 2ab7267 Compare April 18, 2022 21:31

gnprice reviewed Apr 19, 2022

View reviewed changes

chrisbobbe and others added 16 commits April 19, 2022 10:59

api.updateStream [nfc]: Accept multiple properties at once

65ddf3f

streamsActions: Send one update-stream request for all changed proper…

be12766

…ties Instead of up to three in sequence, one per changed property.

api types [nfc]: Express updateStream's params by referring to Stream…

3fc7aa3

… type

modelTypes [nfc]: Order Stream type's properties to follow a doc

eb50271

modelTypes: Update Stream type, to match streams initial data at FL…

dd6fea1

… 121

api types: Sync updateStream's params with API doc at FL 121

0c56883

stream [nfc]: Assimilate to name "invite_only" in create/edit stream …

2668bde

…logic We'll be adding more stream properties here soon, and it'll be best to avoid multiplying the number of different names.

userSelectors [nfc]: Bring a comment up-to-date

2a5bcd2

Grepping shows that we actually use this in several places now.

userSelectors [nfc]: Remove now-unused getOwnEmail

aba93ee

Related: zulip#3764

api types [nfc]: Express createStream's params by referring to Stream…

7fee95b

… type

api [nfc]: Use spreads, to reduce duplication

d002af0

api types: Sync createStream's params with API doc at FL 121

6eff27a

chrisbobbe force-pushed the pr-stream-api-sync branch from 2ab7267 to 6eff27a Compare April 19, 2022 18:34

gnprice merged commit 6eff27a into zulip:main Apr 19, 2022

chrisbobbe deleted the pr-stream-api-sync branch April 19, 2022 18:58

chrisbobbe mentioned this pull request Apr 19, 2022

api: Sync update-stream event types with doc and align implementation #5346

Merged

This was referenced Jun 17, 2022

updateExistingStream: Reduce api calls on changing stream details. #3224

Closed

refactor updateExistingStream: Reduce api calls on changing stream details. #3222

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: Sync updateStream and createStream with their API docs #5341

api: Sync updateStream and createStream with their API docs #5341

chrisbobbe commented Apr 18, 2022 •

edited

gnprice left a comment

gnprice Apr 19, 2022

chrisbobbe Apr 19, 2022 •

edited

chrisbobbe Apr 19, 2022 •

edited

gnprice Apr 19, 2022

gnprice Apr 19, 2022

gnprice Apr 19, 2022

gnprice Apr 19, 2022

gnprice Apr 19, 2022

chrisbobbe commented Apr 19, 2022 •

edited

gnprice commented Apr 19, 2022

		// TODO(server-5.0): New in FL 53
		is_web_public?: $PropertyType<Stream, 'is_web_public'>,

api: Sync updateStream and createStream with their API docs #5341

api: Sync updateStream and createStream with their API docs #5341

Conversation

chrisbobbe commented Apr 18, 2022 • edited

gnprice left a comment

Choose a reason for hiding this comment

gnprice Apr 19, 2022

Choose a reason for hiding this comment

chrisbobbe Apr 19, 2022 • edited

Choose a reason for hiding this comment

chrisbobbe Apr 19, 2022 • edited

Choose a reason for hiding this comment

gnprice Apr 19, 2022

Choose a reason for hiding this comment

gnprice Apr 19, 2022

Choose a reason for hiding this comment

gnprice Apr 19, 2022

Choose a reason for hiding this comment

gnprice Apr 19, 2022

Choose a reason for hiding this comment

gnprice Apr 19, 2022

Choose a reason for hiding this comment

chrisbobbe commented Apr 19, 2022 • edited

gnprice commented Apr 19, 2022

chrisbobbe commented Apr 18, 2022 •

edited

chrisbobbe Apr 19, 2022 •

edited

chrisbobbe Apr 19, 2022 •

edited

chrisbobbe commented Apr 19, 2022 •

edited