-
-
Notifications
You must be signed in to change notification settings - Fork 643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Edit stream access policy settings. #5304
Conversation
new_name?: string, | ||
is_private?: boolean, | ||
is_web_public?: boolean, | ||
stream_post_policy?: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Should we aim for parity with the Zulip APIs or leave out properties that aren't used in the mobile app yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Chat thread: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/api.2EupdateStream/near/1347906.2E01
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Quick comments on this draft.
new_name?: string, | ||
is_private?: boolean, | ||
is_web_public?: boolean, | ||
stream_post_policy?: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Chat thread: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/api.2EupdateStream/near/1347906.2E01
src/streams/streamsActions.js
Outdated
if (initialValues.name !== newValues.name) { | ||
await api.updateStream(auth, id, 'new_name', maybeEncode(newValues.name)); | ||
updatedAndEncodedValues.new_name = maybeEncode(newValues.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "encode" part here isn't fundamental -- it's basically a mistake in the server API, one that's corrected in newer server versions, plus the dealing with it should ideally go inside the API binding code. So I'd leave it out of this variable's name, letting the name refer just to what this would ideally be, and let the TODO comments above cover the fact that there's this hack that makes it not quite what it would ideally be.
Perhaps "updates"?
936778f
to
474db6a
Compare
src/streams/EditStreamCard.js
Outdated
web_public: { | ||
title: 'Web-public', | ||
policy: { isPrivate: false, isWebPublic: true, historyPublicToSubscribers: true }, | ||
description: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put these descriptions here because they are present in the webapp and
- We might want to display them in some UI redesign in the near future.
- They give the reader of this code a real description of what each policy means.
They are however, unused. So I'm not against removing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine.
Let's include a comment noting that they aren't currently used, and therefore aren't in the set of strings to be translated. That way we'll know to add them to be translated if we start using them in the future.
src/streams/EditStreamCard.js
Outdated
const { name, description, isPrivate } = this.state; | ||
onComplete(name, description, isPrivate); | ||
const { name, description, accessPolicy } = this.state; | ||
onComplete(name, description, streamAccessPolicies[accessPolicy].policy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the map here is simple but at a glance it looks like a opportunity for a key lookup error. Question: is flow checking the possible values here and ensuring that there will not be a lookup error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately, when I'm not sure of the answer to a question like that, I like to test it empirically.
Here, try munging one of the names somewhere as if there were a typo -- for example like:
const streamAccessPolicies = {
- web_public: {
+ webb_public: {
title: 'Web-public',
That causes an error at this property access. The full error message is:
Error ------------------------------------------------------------------------------ src/streams/EditStreamCard.js:97:56
Cannot get `streamAccessPolicies[accessPolicy]` because property `web_public` (did you mean `webb_public`?) is missing
in object literal [1]. [prop-missing]
src/streams/EditStreamCard.js:97:56
97| onComplete(name, description, streamAccessPolicies[accessPolicy].policy);
^^^^^^^^^^^^
References:
src/streams/EditStreamCard.js:56:30
v
56| const streamAccessPolicies = {
57| webb_public: {
58| title: 'Web-public',
59| policy: { isPrivate: false, isWebPublic: true, historyPublicToSubscribers: true },
60| description:
61| 'Organization members can join (guests must be invited by a subscriber); anyone on the Internet can view complete message history without creating an account',
62| },
63| public: {
64| title: 'Public',
65| policy: { isPrivate: false, isWebPublic: false, historyPublicToSubscribers: false },
66| description:
67| 'Organization members can join (guests must be invited by a subscriber); organization members can view complete message history without joining',
68| },
69| private_shared_history: {
70| title: 'Private, shared history',
71| policy: { isPrivate: true, isWebPublic: false, historyPublicToSubscribers: true },
72| description:
73| 'Must be invited by a subscriber; new subscribers can view complete message history; hidden from non-administrator users',
74| },
75| private_protected_history: {
76| title: 'Private, protected history',
77| policy: { isPrivate: true, isWebPublic: false, historyPublicToSubscribers: false },
78| description:
79| 'Must be invited by a subscriber; new subscribers can only see messages sent after they join; hidden from non-administrator users',
80| },
81| };
^ [1]
So yep, it's being checked.
One way you can test this sort of thing more abstractly is to hover on the values streamAccessPolicies
and accessPolicy
, or otherwise ask your editor what Flow thinks the types are. You'll see AccessPolicy
for the latter, and for the former an object type with those names web_public
etc. as its four property names. So it does know that those are the object's four properties, and it would give an error if trying to get some arbitrary other property.
<SelectableOptionRow | ||
key={policy} | ||
itemKey={policy} | ||
title={streamAccessPolicies[policy].title} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as above regarding key access and Flow.
@@ -309,22 +309,22 @@ describe('streamsReducer', () => { | |||
expect(actualState).toEqual(expectedState); | |||
}); | |||
|
|||
test('Change the email_address property', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming the only reason this test was passing before was because the update function was using a generic computed property. Now that it's typed, I changed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, and also this test file doesn't have types on it yet. There aren't a lot of files like that left (and all of them are tests -- all our non-test code has had types for years.)
Let's have a prep commit to give it types. See:
https://github.com/zulip/zulip-mobile/blob/main/docs/howto/testing.md#test-style-guide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact it looks like this is the largest remaining such file!
$ git grep -L @flow src/**/*-test.js | xargs wc -l | sort -rn
2261 total
368 src/streams/__tests__/streamsReducer-test.js
359 src/chat/__tests__/flagsReducer-test.js
…
So it'll be great to get it converted.
} | ||
: stream, | ||
); | ||
return state.map(stream => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could convert this to a ternary operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I don't really have much of a preference on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Fingel!
The main high-level piece of feedback I have at this stage is: I'd like to see these changes split up into a longer series of smaller commits. That's our practice in the Zulip project, and it helps a lot in being able to review changes completely and understand exactly what's happening.
The basic strategy I'd suggest for splitting the changes up is:
- First, have a series of commits that refactor the code into the shape you want it to end up with, but without changing anything about how it ultimately behaves. I.e., "NFC" changes: https://github.com/zulip/zulip-mobile/blob/main/docs/glossary.md#nfc
After those, have one or more commits that change the actual behavior. Because these are now much smaller than the overall everything-together commit was, it's easier to see exactly what effects they have. - Among the changes that affect behavior, try to add plumbing first, and then separately the UI. Here "plumbing" may mean adding a feature to the API binding, and perhaps to our data structures and reducers.
Concretely, I think a sequence that can work here might look like:
-
Change
api.updateStream
to take an object, instead of property + value, and change its call site accordingly; but don't yet add the properties we weren't already using.This one is almost an NFC commit, but not quite because it means we make one request instead of up to three of them. The prefix in the summary line can be
api:
. -
Revise
StreamEvent
, introducingStreamUpdateEvent
, so that it gets specific about howname
anddescription
are strings whileinvite_only
is a boolean; but don't yet add properties we don't refer to, or the complication about some properties coming with extra properties. This is an NFC commit; prefix can beapi [nfc]:
, orevent types:
. -
Inline that tiny
updateSubscription
helper, as mentioned below. This is a small NFC commit; prefix can bestream [nfc]:
. -
Factor out the
updateStreamProperties
helper, but don't yet add the wrinkle about extra properties going withinvite_only
. This makes the commit NFC; prefix can bestream [nfc]:
.In particular this means this code will be buggy -- it'll get the stream into an inconsistent state where e.g.
invite_only
is true butis_web_public
is also true. But that's OK, because it's already buggy in that way. And the great thing about a good clean NFC commit is that it's relatively easy to see, as the reader, that the new code behaves exactly like the old code, so that whatever bugs the code has afterward are bugs it already had before.This commit has to come after the improvements above to the
StreamEvent
type, so that Flow is assured that when e.g.event.property
is'name'
,event.value
will be a string and not a boolean. -
Now add to
StreamUpdateEvent
those wrinkles about multiple properties changing at once, and reflect that inupdateStreamProperties
at the same time. This commit cleanly fixes a bug. Prefix can bestream:
. -
For good measure, do the same for
rendered_description
tagging along withdescription
. This could be either the same commit as theinvite_only
extra properties, or a separate commit. Perhaps err on the side of a separate commit -- it's easy to squash changes later, and can be a bit more annoying to split them up. -
Extend
api.updateStream
, and perhaps also its callerupdateExistingStream
, to take those additional properties -- but without yet actually making the caller use that new functionality. This is NFC in that no behavior changes (because nothing's invoking the new functionality), but generally I don't use the[nfc]
label for a change that's really adding new functionality; prefix could beapi:
. -
Now the changes in those three UI files. These can likely also be several commits, but I haven't yet read that part 🙂
A few other comments below. I'll also go through your questions.
new_name?: string, | ||
is_private?: boolean, | ||
is_web_public?: boolean, | ||
stream_post_policy?: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stream_post_policy?: number, | |
stream_post_policy?: 1 | 2 | 3 | 4, |
Hmm but actually: I see we don't currently have that property on Stream. Let's add it to Stream and here at the same time; we can just leave it out for now.
In particular that means that before we do add it, we can arrange to draw the connection between this type and Stream, so that we (a) don't have to write this kind of detail twice (it's one thing to do that for like boolean
, but gets more annoying for something more detailed, and more likely to change in the future, like 1 | 2 | 3 | 4
), and (b) don't risk them getting accidentally out of sync.
is_web_public?: boolean, | ||
stream_post_policy?: number, | ||
history_public_to_subscribers?: boolean, | ||
message_retention_days?: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the API doc, this should be number | 'realm_default' | 'unlimited'
. But in fact let's leave it out for now, for the same reasons as stream_post_policy
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may end up being even more complicated than that; see discussion.
name: string, | ||
|}>; | ||
|
||
export type StreamUpdateEvent = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://zulip.com/api/get-events#stream-update for more context.
Yeah -- let's in fact have this link in a comment on this type. See:
https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#api-doc-link
(We don't currently do that on all the existing types, but should do so when making changes.)
src/api/eventTypes.js
Outdated
|
||
export type StreamUpdateEvent = | ||
| {| ...StreamUpdateEventBase, +property: 'name', +value: string |} | ||
| {| ...StreamUpdateEventBase, +property: 'description', +value: string |} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one has its own wrinkle: +rendered_description: string
.
const updateSubscription = (state, event) => | ||
state.map(sub => | ||
sub.stream_id === event.stream_id ? { ...sub, [event.property]: event.value } : sub, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This inlining (to the two call sites) can happen as its own small NFC prep commit.
That way it's clear that the case of a subscription-update event doesn't change, and it stands out more what changes in the case of a stream-update event.
src/streams/EditStreamCard.js
Outdated
type StreamPolicySettings = { | ||
// The properties on a stream object that are related to the access policy. | ||
isPrivate: boolean, | ||
isWebPublic: boolean, | ||
historyPublicToSubscribers: boolean, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the interest of having fewer distinct names for the same thing, let's give these the same snake-case names as in the API: invite_only
, is_web_public
, etc. Same for where they get added to updateExistingStream
.
(I know we have some existing instances in main of isPrivate
for invite_only
and isWebPublic
for is_web_public
-- and I guess also an inviteOnly
for invite_only
. As there gets to be more of this logic and not just a single "private" / "public" boolean, I think that rapidly becomes less appealing. OK to leave those existing uses as is for this PR, but also if you feel like converting them over, that'd be a good additional commit or set of commits.)
type StreamPolicySettings = { | ||
// The properties on a stream object that are related to the access policy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This comment is describing the type as a whole, and in fact is describing its interface: the information that a reader will want to have when using this type elsewhere, not only when making changes to its internals.
For information about something's interface, use jsdoc:
type StreamPolicySettings = { | |
// The properties on a stream object that are related to the access policy. | |
/** The properties on a stream object that are related to the access policy. */ | |
type StreamPolicySettings = { |
(Hmm, I should put that in the style guide.)
src/streams/EditStreamCard.js
Outdated
type AccessPolicy = | ||
| 'public' | ||
| 'web_public' | ||
| 'private_shared_history' | ||
| 'private_protected_history'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can simplify this a bit and reduce redundancy:
type AccessPolicy = | |
| 'public' | |
| 'web_public' | |
| 'private_shared_history' | |
| 'private_protected_history'; | |
type AccessPolicy = $Keys<typeof streamAccessPolicies>; |
(Then either move it after streamAccessPolicies
, or just say // eslint-disable-next-line no-use-before-define
.)
I'm not sure how these can be split into different commits. Once |
00999d3
to
e7cbb52
Compare
Alright, I was able to split the changes dealing with the Flow types around stream update events. I'll have to work a bit more on how to split up the actual UI changes, as well as:
It's not immediately obvious to me how to update api.updateStream without updating the call sites as all arguments are required. |
Thanks for the revision! Looking through it now.
All the function arguments ( |
Hmm, yeah, I see. Really those errors (or some of them) should already be there for the code in main -- in particular, the existing type says that Still, at least this bit can be split out:
|
This commit changes `api.updateStream` to accept additional parameters for updating streams, as well as modifying `streamsActions.updateExistingStream` to update multiple stream properties in a single PATCH call instead of one at a time.
e7cbb52
to
d01347f
Compare
Just pushed a revision with the changes @Fingel and I made while pairing with my screen shared. |
This removes a test that was testing the ability to update a property that is actually non-existent on a stream: email_address. This is a prep commit for changes that introduce stronger typing around stream events which would cause this test to fail type checking.
Add a StreamUpdateEvent type which explicitly types which values the properties of a stream update event can contain. This is a prep commit for expanding the StreamUpdateEvent with additional properties and logic.
…bers Adds several more properties to the StreamUpdateEvent type and handles them appropriately in the associated reducer. Also adds a test that checks the special handling of `invite_only` works as intended.
The API returns an additional property, rendered_description, when the updated property of a stream update is description. This commit reflects this in the StreamUpdateEvent type. We don't currently interact with the rendered_description property of streams at all, so there's nothing to do in the reducer. Just note in a comment on the Stream type so that we see it if we every start using that property in the future.
…logic We'll be adding more stream properties here soon, and it'll be best to avoid multiplying the number of different names.
Adds several options to the Stream edit screen to change the access policy of a stream.
cb9527b
to
27f78c8
Compare
Use an object instead of positional arguments for api.createStream as the amount of arguments has become unruly.
Allow creating streams while specifying is_web_public and history_public_to_subscribers.
27f78c8
to
b51a524
Compare
WIP.
Fixes: #5250.