Skip to content
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

EditStreamCard: Offer all four stream-privacy settings #5389

Merged
merged 18 commits into from
May 25, 2022

Conversation

chrisbobbe
Copy link
Contributor

@chrisbobbe chrisbobbe commented May 24, 2022

With the web-public option

  • omitted if the server hasn't opted into web-public streams
  • disabled if it's currently unset and the user doesn't have permission to set it

When disabled, it's unselectable; on trying to select it, it gives you an informative alert.

I'll mark this as a draft for now and aim to do a demo with Greg in the office today.

(edit: added Fixes: line)

Fixes: #5250

@chrisbobbe chrisbobbe added P1 high-priority server release goal Things we should try to coordinate with a major Zulip Server release. labels May 24, 2022
@chrisbobbe chrisbobbe requested a review from gnprice May 24, 2022 21:57
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chrisbobbe! This code looks great; glad this is all getting close to complete. Small comments below.

Comment on lines 40 to 55
} catch (errorIllTyped) {
const error: mixed = errorIllTyped; // https://github.com/facebook/flow/issues/2470
if (error instanceof ApiError) {
showErrorAlert(
_('Cannot apply requested settings'),
// E.g., "Must be an organization or stream administrator", with
// code UNAUTHORIZED_PRINCIPAL, when trying to change an
// existing stream's privacy setting when unauthorized.

// TODO: File an issue for the server to be more specific. What
// part(s) of the request didn't pass authorization:
// stream-name change? privacy-setting change? Help the user
// make a successful request if possible.
error.message,
);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When catching an error and it turns out not to be of a type we anticipated, rethrow the error (so that it's as if we hadn't caught it.) For example:

Suggested change
} catch (errorIllTyped) {
const error: mixed = errorIllTyped; // https://github.com/facebook/flow/issues/2470
if (error instanceof ApiError) {
showErrorAlert(
_('Cannot apply requested settings'),
// E.g., "Must be an organization or stream administrator", with
// code UNAUTHORIZED_PRINCIPAL, when trying to change an
// existing stream's privacy setting when unauthorized.
// TODO: File an issue for the server to be more specific. What
// part(s) of the request didn't pass authorization:
// stream-name change? privacy-setting change? Help the user
// make a successful request if possible.
error.message,
);
}
}
} catch (errorIllTyped) {
const error: mixed = errorIllTyped; // https://github.com/facebook/flow/issues/2470
if (error instanceof ApiError) {
showErrorAlert(
_('Cannot apply requested settings'),
// E.g., "Must be an organization or stream administrator", with
// code UNAUTHORIZED_PRINCIPAL, when trying to change an
// existing stream's privacy setting when unauthorized.
// TODO: File an issue for the server to be more specific. What
// part(s) of the request didn't pass authorization:
// stream-name change? privacy-setting change? Help the user
// make a successful request if possible.
error.message,
);
} else {
throw error;
}
}

Style guide entry here: https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#catch-specific

In this context, I expect that just means the unhandled promise rejection will propagate up to a UI event callback; nothing will crash, but I believe we'll get a Sentry event, which is helpful if it happens.

Comment on lines 49 to 52
// TODO: File an issue for the server to be more specific. What
// part(s) of the request didn't pass authorization:
// stream-name change? privacy-setting change? Help the user
// make a successful request if possible.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in the office: I agree this would be an improvement, but I think it's past the point of diminishing returns compared to other parts of the app we can be working on improving.

Comment on lines 45 to 79
return useMemo(
() =>
[
webPublicStreamsEnabled && enableSpectatorAccess
? {
key: 'web-public',
title: 'Web-public',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can make the contents here a couple of levels less deeply indented like so:

Suggested change
return useMemo(
() =>
[
webPublicStreamsEnabled && enableSpectatorAccess
? {
key: 'web-public',
title: 'Web-public',
const webPublicStreamsActuallyEnabled = webPublicStreamsEnabled && enableSpectatorAccess;
return useMemo(
() =>
[
webPublicStreamsActuallyEnabled && {
key: 'web-public',
title: 'Web-public',

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I guess this one runs into Flow trouble because the type on Array#filter(Boolean) isn't specific enough to say that it filters out false. From flowlib/core.js (gotten by jump-to-definition on the filter call):

    filter(callbackfn: typeof Boolean): Array<$NonMaybeType<T>>;

I think I still like putting the one-line "nothing here" case first, though, akin to an early return. Like this:

Suggested change
return useMemo(
() =>
[
webPublicStreamsEnabled && enableSpectatorAccess
? {
key: 'web-public',
title: 'Web-public',
return useMemo(
() =>
[
!(webPublicStreamsEnabled && enableSpectatorAccess)
? undefined
: {
key: 'web-public',
title: 'Web-public',

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I guess this one runs into Flow trouble because the type on Array#filter(Boolean) isn't specific enough to say that it filters out false.

Oh, hmm, yeah.

Comment on lines 56 to 58
disabledIfNotInitialValue: !canCreateWebPublicStreams
? {
title: 'Insufficient permission',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly:

Suggested change
disabledIfNotInitialValue: !canCreateWebPublicStreams
? {
title: 'Insufficient permission',
disabledIfNotInitialValue: !canCreateWebPublicStreams && {
title: 'Insufficient permission',

Comment on lines 131 to 144
.map(
(x: {|
key: Privacy,
title: LocalizableText,
subtitle?: LocalizableText,
disabledIfNotInitialValue?:
| {|
+title: LocalizableText,
+message?: LocalizableText,
+learnMoreUrl?: URL,
|}
| false,
|}) => {
const { disabledIfNotInitialValue, ...rest } = x; // eslint-disable-line no-unused-vars
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simpler version:

Suggested change
.map(
(x: {|
key: Privacy,
title: LocalizableText,
subtitle?: LocalizableText,
disabledIfNotInitialValue?:
| {|
+title: LocalizableText,
+message?: LocalizableText,
+learnMoreUrl?: URL,
|}
| false,
|}) => {
const { disabledIfNotInitialValue, ...rest } = x; // eslint-disable-line no-unused-vars
.map(x => {
const { disabledIfNotInitialValue = false, ...rest } = x;

Comment on lines +119 to +146
title: 'Private, shared history',
subtitle:
'Must be invited by a subscriber; new subscribers can view complete message history; hidden from non-administrator users',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These strings are meant to match what's on web, right? Probably a comment somewhere here briefly pointing at the corresponding web code for comparison would be helpful.

This is NFC, because we'd return false when role is undefined
anyway. But this makes that easier to verify, and also gives a
reason why it's the correct behavior.
… API

And move the Privacy type definition here.

The translation is about to get much more complicated: `Privacy`
will have four branches, corresponding to the four stream-access
policy settings:

- web-public
- public
- private, shared history
- private, protected history

, and those will be communicated to the server with up to three
properties:

- is_private (same meaning as Stream.invite_only)
- is_web_public
- history_public_to_subscribers

.
@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented May 25, 2022

Thanks for the review! Revision pushed, with an additional feature, as discussed in the office: catching you before you leave the form without submitting changes, in case you meant to submit but forgot.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the revision! Comments below.

Comment on lines +238 to +241
result = await props.onComplete({
name: initialValues.name !== name ? name : undefined,
description: initialValues.description !== description ? description : undefined,
privacy: initialValues.privacy !== privacy ? privacy : undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stream settings: Improve how we decide if the user has changed a value

Compare against the initial values from when the form was
initialized, rather than against the current, live-updated stream,
as we were doing before.

Does this logic actually work as stated? This initialValues comes from a prop, and on EditStreamScreen the prop comes from the stream's data in Redux. So it looks like this should also end up comparing to the current values in Redux, not the values the component started with.

I think something like this should do it (untested):

 export default function EditStreamCard(props: Props): Node {
-  const { navigation, initialValues, isNewStream } = props;
+  const { navigation, isNewStream } = props;
+  const initialValues = React.useRef(props.initialValues).current;

Copy link
Contributor Author

@chrisbobbe chrisbobbe May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh goodness, yeah, thanks for catching this. initialValues doesn't have the right name for what we're passing for it, I think: it shouldn't have that name if its contents can change to be different from the initial values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using a ref in EditStreamScreen and keeping the initialValues name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialValues doesn't have the right name for what we're passing for it, I think: it shouldn't have that name if its contents can change to be different from the initial values.

Yeah, I agree.

How about using a ref in EditStreamScreen and keeping the initialValues name?

Sure, that'd work.

Comment on lines 46 to 61
privacy: stream.invite_only ? 'private' : 'public',
privacy: streamPropsToPrivacy(stream),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EditStreamScreen [nfc]: Use streamPropsToPrivacy to convert initial value

This will save quite a bit of duplication; see previous commit.

I don't understand the last reference. The previous commit is:
181bf22 EditStreamCard [nfc]: Use params object for PropsCreateStream, too

Ah, maybe you meant to refer to this one?
d2d63b5 streamsActions: Have updateExistingStream translate Privacy value for API

The translation is about to get much more complicated: `Privacy`
will have four branches, corresponding to the four stream-access
policy settings:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The actual change seems good, though.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! Thanks for catching this.

Comment on lines +72 to +76
// Only send is_web_public if the server will recognize it.
// TODO(server-5.0): Remove conditional.
if (getZulipFeatureLevel(state) >= 98) {
updates.is_web_public = streamProps.is_web_public;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think I liked the previous revision's version of this better:

    -+    // Only send is_web_public if the server will recognize it. Will it? It
    -+    // should, if we've let the user switch a stream from web-public to not
    -+    // or vice versa.
    -+    // TODO(server-2.1): Remove conditional.
    -+    if (initialPrivacy === 'web-public' || newData.privacy === 'web-public') {
     +      updates.is_web_public = streamProps.is_web_public;
     +    }

IIUC, the case where the behavior would differ is where we think the server isn't new enough to recognize the option, but nevertheless for some reason we've allowed the user to say they want to change it. In that case it seems like it's better to go ahead and make the request, and let the server return an error that we'll show the user, rather than silently drop that part of the user's request.

(That and also the converse case: where the server is new enough but the user didn't change to or from web-public, the old logic would leave this property out and the new will include it. That case seems fine either way.)

Copy link
Contributor Author

@chrisbobbe chrisbobbe May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think I liked the previous revision's version of this better

Ah, I thought you might. 😅

With this function no longer receiving an "initial" privacy value—

streamsActions [nfc]: Have updateExistingStream only take changed values

—it can no longer identify when the privacy is being switched away from web-public. (It can still tell if it's being switched to web-public, though.) To cover the away-from-web-public case without upsetting old servers, it might be enough to just omit is_web_public when switching to any of the three other options? We would still be passing is_private for those three options, which should satisfy the requirement I observed that you have to pass at least one of is_private, is_web_public. I'd want to test manually that things still work. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

streamsActions [nfc]: Have updateExistingStream only take changed values

This commit helps focus updateExistingStream on translating a caller's requested updates for the API, rather than doing that and also helping the caller figure out what those requested updates are.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this function no longer receiving an "initial" privacy value—

streamsActions [nfc]: Have updateExistingStream only take changed values

—it can no longer identify when the privacy is being switched away from web-public. (It can still tell if it's being switched to web-public, though.)

Hmm, I see.

To cover the away-from-web-public case without upsetting old servers, it might be enough to just omit is_web_public when switching to any of the three other options?

That doesn't work if you're switching from web-public to simply public, though, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, given that, this logic is fine.

Basically the concern above means that there's a possible error case we won't catch, and where things silently won't work. But that case only arises if something's already gone wrong that was outside our control: you're on a server that half-supports web-public streams, and I think in fact a server admin has to have gone and manually set a stream web-public in the database (because before this feature-level threshold, there is no way for a client to set it.) Outside our control, and also unlikely -- it's probable that there don't exist any Zulip servers where that situation exists at all.

It's good to catch even situations like that, when we can do so while writing the code in a natural way. But it's OK if we don't always catch them, and it isn't worth contorting the code to handle them.

…mCard

This is NFC because EditStreamCard has already been using a snapshot
of initialValues that it makes on its first render. But since the
prop is named `initialValues`, we shouldn't be passing something
that changes to be different from the initial values.

This will help with an upcoming change; see Greg's comment at
  zulip#5389 (comment)
Compare against the initial values from when the form was
initialized, rather than against the current, live-updated stream,
as we were doing before.
…alue

This will save quite a bit of duplication; see the recent commit
where we defined this function.
And bring it up through SelectableOptionsScreen and
InputRowRadioButtons.
This removes the last instance of "Update" as a user-facing message,
so we remove that from messages_en.json. "Save" is already there, so
no need to add it.
@chrisbobbe
Copy link
Contributor Author

Thanks for the review! Revision pushed.

@chrisbobbe chrisbobbe marked this pull request as ready for review May 25, 2022 20:47
@gnprice
Copy link
Member

gnprice commented May 25, 2022

Thanks! Looks good -- merging.

@gnprice gnprice merged commit bc6d9e6 into zulip:main May 25, 2022
@chrisbobbe chrisbobbe deleted the pr-web-public-streams branch May 25, 2022 22:09
@chrisbobbe
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 high-priority server release goal Things we should try to coordinate with a major Zulip Server release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set a stream web-public, in stream settings
2 participants