ServerCompatNotice: Add, to nag users connected to servers <2.0.0. #4750

chrisbobbe · 2021-05-14T22:35:40Z

~~EDIT: updated the threshold from <2.0.0 to <2.1.0; see #4750 (comment).~~ Back to 2.0.0; see #4750 (comment).

As the jsdoc says, at some point we'll settle on a non-hard-coded threshold; see
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/compatibility.20documentation/near/1174966.

The banner can go pretty much anywhere that makes sense. Here, I've got it on HomeScreen only, so it doesn't show up when you're in the message list or even on another tab like the settings screen. I considered putting it in MainTabsScreen just above or below the OfflineNotice; that'd be fine, but then I figured the current way is obtrusive enough.

The "x" button makes the banner disappear, but that state is deliberately stored in state.session, which isn't persisted, so the banner will come back and nag the user on the next startup.

The "Learn more" link goes to https://zulip.readthedocs.io/en/stable/overview/release-lifecycle.html.

Note that the correct handling of safe areas (noticeable in landscape mode on iOS) will look incongruous until #3066 is fixed; #4683 is my next step for that.

I may add an ease-in-ease-out animation when our usePrevious util from #4683 is available.

NOTE: The has-dismissed-the-banner state isn't stored per-realm, so there's currently a bug where dismissing the banner makes it go away for all <2.0.0 realms the device knows about (until app restart, anyway). I figured this was minor enough to be acceptable for the time being, unless we can think of a nice and easy way to store the state both per-realm and per-session, while being sure to clean up after ourselves when the user wants to forget the realm.

(The screenshots don't show an actual <2.0.0 version; I've tweaked the should-it-show conditional. It shows the git describe output we get from /server_settings, unmodified, and this is CZO.)

iOS

Android

alya · 2021-05-14T23:25:28Z

Hmm, do we need to display the version? I think it makes the message harder to understand, at least for everyone who isn't the admin for the server.

alya · 2021-05-14T23:25:53Z

By the way, do we display a different message if the user is the administrator?

alya · 2021-05-14T23:27:26Z

The "learn more" link should probably go lower on the page: https://zulip.readthedocs.io/en/stable/overview/release-lifecycle.html#compatibility-and-upgrading

timabbott · 2021-05-14T23:53:13Z

The version will usually be like "1.9.3" unless they're running a forked version from Git (like chat.zulip.org is).

chrisbobbe · 2021-05-19T00:04:16Z

The "learn more" link should probably go lower on the page: https://zulip.readthedocs.io/en/stable/overview/release-lifecycle.html#compatibility-and-upgrading

Cool, sounds good. I'm seeing this, in that section, added in zulip/zulip@d7d79b57b:

The Zulip mobile and desktop apps maintain backwards-compatibility code to support any Zulip server since 2.1.0.

Seems like I can make the banner show for <2.1.0, not just <2.0.0.

chrisbobbe · 2021-05-19T00:24:19Z

By the way, do we display a different message if the user is the administrator?

I've updated the PR with this text for admins; what do you think?

chrisbobbe · 2021-05-19T00:25:59Z

Hmm, do we need to display the version? I think it makes the message harder to understand, at least for everyone who isn't the admin for the server.

Right; as Tim says, most people won't see the long and complicated string; they'll see "1.9.3" or "2.0.4" or something. Do you think we should still remove it?

alya · 2021-05-19T17:38:22Z

Ok, either way seems OK for showing the version number to non-admins then.

alya · 2021-05-19T17:40:13Z

For admins, can we make the call to action more explicit? E.g. for the second sentence: "Please upgrade your server as soon as possible by following the simple process described here." -- or something along those lines?

chrisbobbe · 2021-05-19T23:52:21Z

For admins, can we make the call to action more explicit?

Yeah, that makes sense. After looking a bit closer, I think we may like do that in a different direction than the example you mentioned; the "Learn more" button in my previous revision really wants to be a separate button, not so much an inline link on a word in a sentence.

I took a look at the Material Design doc for banners, and made some adjustments in this revision based on that. It says we shouldn't use an "x" to close; it says "All actions should be shown as text buttons." So I've made "DISMISS" button instead of the "x" icon. It also says "Don’t include links in a banner message. All available actions should be represented as buttons."

Here are some screenshots for what an admin sees. "Upgrade now" seemed like a more obvious wording for the action button than "fix now", but I think "upgrade now" gets used a lot when apps ask people to pay for stuff, and I didn't want an admin to be misled by that (maybe they have the power to make decisions but they're not really in the loop about how Zulip works).

And for non-admins, and one screenshot on Android:

In an earlier revision, I considered adding a Zulip logo to signify that it was a message from us, e.g.,

Images are allowed in the Material Design spec, but I think the text is running long, and that would give it less space. In the "Specs" section, there's a diagram that says "One to two lines is preferable on mobile and tablet", and I'm seeing three on the devices I'm testing on, which are pretty standard.

alya · 2021-05-20T03:21:25Z

Cool, looks good to me! I like that we now know a lot more about adding banners -- it may come in handy.

…

On Wed, May 19, 2021, 4:52 PM Chris Bobbe ***@***.***> wrote: For admins, can we make the call to action more explicit? Yeah, that makes sense. After looking a bit closer, I think we may like do that in a different direction; the "Learn more" button in my previous revision really wants to be a separate button, not so much an inline link on a word in a sentence. I took a look at the Material Design doc for banners <https://material.io/components/banners>, and made some adjustments in this revision based on that. It says we shouldn't use an "x" to close; it says "All actions should be shown as text buttons." So I've made "DISMISS" button instead of the "x" icon. It also says "Don’t include links in a banner message. All available actions should be represented as buttons." Here are some screenshots for what an admin sees. "Upgrade now" seemed like a more obvious wording for the action button than "fix now", but I think "upgrade now" gets used a lot when apps ask people to pay for stuff, and I didn't want an admin to be misled by that (maybe they have the power to make decisions but they're not really in the loop about how Zulip works). <https://user-images.githubusercontent.com/22248748/118897193-c48d3500-b8d7-11eb-8642-78dc800cd4f1.png> <https://user-images.githubusercontent.com/22248748/118897320-fe5e3b80-b8d7-11eb-812f-961d4b45d30c.png> <https://user-images.githubusercontent.com/22248748/118897160-b7704600-b8d7-11eb-8989-80d88bf27ecd.png> And for non-admins, and one screenshot on Android: <https://user-images.githubusercontent.com/22248748/118897990-8c86f180-b8d9-11eb-8e7a-15134627b223.png> <https://user-images.githubusercontent.com/22248748/118898198-0028fe80-b8da-11eb-92bc-af5a486aca48.png> In an earlier revision, I considered adding a Zulip logo to signify that it was a message from us, e.g., <https://user-images.githubusercontent.com/22248748/118898349-55fda680-b8da-11eb-9774-4fcb9851d086.png> Images are allowed in the Material Design spec, but I think the text is running long, and that would give it less space. In the "Specs" <https://material.io/components/banners#specs> section, there's a diagram that says "One to two lines is preferable on mobile and tablet", and I'm seeing three on the devices I'm testing on, which are pretty standard. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4750 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAP6IUUWEBEG7SLALI6VGC3TORFMLANCNFSM445GNIFQ> .

WesleyAC

A few comments, looks good overall :)

WesleyAC · 2021-05-21T00:16:32Z

src/common/RawLabel.js

+      (text === undefined && children !== undefined)
+        || (text !== undefined && children === undefined),


I'd think to write !!text !== !!children, but perhaps that's more cryptic.

Interesting. It's pretty opaque, to me, and I'm not sure I'd choose it everywhere.

But in this setting, it has the unusual property of not really needing to be understood (except for now, when we choose to introduce it). RawLabel's callers won't violate the invariant if they've read and understood its jsdoc, and if they do violate it, the error message will tell them clearly what they've done wrong. And there's no other reason the invariant would be violated.

It also has the really nice property of letting us fit the whole invariant(...) on a single line. So I'll do it; thanks for the idea! 🙂

WesleyAC · 2021-05-21T00:20:14Z

src/session/sessionReducer.js

+   *   dismissed this session.
+   *
+   * We put this in `SessionState` deliberately, so that users see the
+   * notice on every startup until the server is upgraded. That's a


What's the mechanism by which this gets reset?

Good point; I'll mention it in the comment. state.session isn't persisted to the disk ('session' is in discardKeys in src/boot/store.js), and I've set state.session.hasDismissedServerCompatNotice to be false by default. That means that on startup it'll be initialized to false, no matter what its value was in the previous session.

WesleyAC · 2021-05-21T00:21:13Z

src/common/ZulipTextButton.js

@@ -0,0 +1,109 @@
+/* @flow strict-local */


Is there a reason we can't pull in a button like this from a library of material components? I guess this is BRAND_COLOR by default, is there anything else?

Hmm, good idea—maybe https://callstack.github.io/react-native-paper/button.html; I'll give it a try.

We may like to make our own thin wrapper around that component, for potentially a few reasons:

setting project-specific things like BRAND_COLOR, as you mention

That component says it accepts a style prop. Depending on how much micromanaging that style prop allows, we might want our wrapper to not expose it directly, and instead expose a few coherent options. See this implementation comment in ZulipButton:

// The use of specific style properties for callers to micromanage the // layout of the button is pretty chaotic and fragile, introducing // implicit dependencies on details of the ZulipButton implementation. // TODO: Assimilate those usages into a more coherent set of options // provided by the ZulipButton interface explicitly. // // Until then, by listing here the properties we do use, we at least // make it possible when working on the ZulipButton implementation to // know the scope of different ways that callers can mess with the // styles. If you need one not listed here and it's in the same // spirit as others that are, feel free to add it.

gnprice

Thanks @chrisbobbe , and thanks all for the reviews! Comments below.

Glad also to see these cleanups to Label and RawLabel and friends, and hopeful that we can reuse some of this nice careful work on text-buttons and banners.

src/common/Label.js

src/common/ZulipTextButton.js

src/common/ServerCompatBanner.js

gnprice · 2021-05-25T23:30:16Z

src/common/ServerCompatBanner.js

+              openLinkWithUserPreference(
+                'https://zulip.readthedocs.io/en/stable/overview/release-lifecycle.html#compatibility-and-upgrading',
+                store.getState,


Yeah, I think this example further makes me think (#4679 (comment) ) that having this function take a SettingsState would be a preferable API.

src/common/ServerCompatBanner.js

gnprice · 2021-05-25T23:55:41Z

On the version threshold, commenting here in the top-level thread for a bit broader visibility: rather than go straight to 2.1.0, I'd prefer to get there gradually by first rolling this banner out to a smaller number of users on older servers. That way we get a chance to hear any feedback from people who see it, and adjust as needed before rolling it out to a wider set of users, because this is inherently a sort of message that may make people unhappy.

A threshold of 2.0 would apply to 2-4% or so of our users as of a couple of months ago, which I think is a good place to start. In fact if we were more of a giant company with tons of users, I'd start another version or two older than that; but as is, I think 2.0 is about right for getting meaningful data from the feedback we hear, or don't hear, from users as it rolls out.

This would be around 2-4% of our users, as of 2021-03 [1]. As Greg points out [2], we'd like to try this out on a small number of users and gather any feedback, before expanding it to more users, since it's the kind of message that can make people unhappy. So, even though Zulip's release lifecycle doc [3] says 2.1.0 is the threshold, we'd like to start using that threshold only after the code for this threshold has been out for at least a couple of weeks. And at some time in the future, we'll likely settle on a threshold that's not hard-coded at all; see https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/compatibility.20documentation/near/1174966. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/emoji.20list/near/1136563 [2] zulip#4750 (comment) [3] https://zulip.readthedocs.io/en/4.2/overview/release-lifecycle.html

As suggested by Wesley [1]. It'll take a bit more work to set up [2] than I have time for right now, but this could be a potentially useful follow-up. [1] zulip#4750 (comment) [2] zulip#4709 (comment)

chrisbobbe · 2021-05-26T19:26:01Z

Thanks for the reviews! Revision pushed.

One bit of `Text`'s API that `RawLabel` hasn't been exposing is its support for creating an element with `children` and having those children inherit the element's styles [1]: > For React Native, we decided to use web paradigm for [displaying > formatted text] where you can nest text to achieve the same > effect. (Before this commit, `RawLabel` was silently accepting `children` and ignoring it.) `RawLabel`, unlike `Label`, has the flexibility to match `Text`'s support for passing some React nodes besides strings. That's because `RawLabel` is not responsible for passing a string to a function to get it translated. While we might have just removed the `text` prop and had all callers exclusively use `children`, this seems more disruptive than is worthwhile. Do enforce that exactly one of `children` and `text` is passed, though. [1] https://reactnative.dev/docs/text#nested-text

This way, the components don't both have to have code for default styling.

Using `Text`'s nesting interface we've had `RawLabel` start exposing.

…fault. The default is provided by `RawLabel`.

…disk.

…anner. Which we'll add in the next commit, using this setup.

As suggested by Wesley [1]. It'll take a bit more work to set up [2] than I have time for right now, but this could be a potentially useful follow-up. [1] zulip#4750 (comment) [2] zulip#4709 (comment)

This would be around 2-4% of our users, as of 2021-03 [1]. As Greg points out [2], we'd like to try this out on a small number of users and gather any feedback, before expanding it to more users, since it's the kind of message that can make people unhappy. So, even though Zulip's release lifecycle doc [3] says 2.1.0 is the threshold, we'd like to start using that threshold only after the code for this threshold has been out for at least a couple of weeks. And at some time in the future, we'll likely settle on a threshold that's not hard-coded at all; see https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/compatibility.20documentation/near/1174966. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/emoji.20list/near/1136563 [2] zulip#4750 (comment) [3] https://zulip.readthedocs.io/en/4.2/overview/release-lifecycle.html

This avoids presenting translators with extra information that isn't meaningful without context that's absent. It also means if we later tweak the form of version number we use here, we won't have a need to change the string and cause churn in the strings to translate.

This isolates it a bit from the logic, for future edits; it also makes more room for the comment to explain the relationship between this threshold and our actual oldest supported version.

gnprice

Thanks! Merged, after fixing the one item below and with a couple of small tweaks at the end.

gnprice · 2021-05-28T04:29:01Z

src/common/ServerCompatBanner.js

+                    '{realm} is running Zulip Server {serverVersionRaw}, which is unsupported. Please upgrade your server as soon as possible.',
+                  values: { realm: realm.toString(), serverVersionRaw: zulipVersion.raw() },
+                }
+              : {
+                  text:
+                    '{realm} is running Zulip Server {serverVersionRaw}, which is unsupported. Please upgrade your server as soon as possible.',


Hmm, I think one of these was meant to be different 😉

chrisbobbe · 2021-06-03T01:09:07Z

Thanks for the review!

WesleyAC · 2021-06-14T22:53:59Z

FYI, the first commit here introduced a regression in dev, I've pushed a fix in #4801.

WesleyAC · 2021-06-14T22:54:37Z

Amusingly, I was the one who suggested the change that caused that, oops :)

We haven't gotten negative feedback about the banner from the smaller pool of users connecting to servers earlier than 2.0.0. The banner was introduced with that threshold (in PR zulip#4750) to beta users in v27.164 (2021-06-17) and to all users in v27.165 (2021-06-24). So, proceed with aligning our threshold with what we declare in the server stable-release doc, and update the code comment to match.

chrisbobbe requested review from gnprice and WesleyAC May 14, 2021 22:35

chrisbobbe force-pushed the pr-server-version-nag-banner-lt-2.0.0 branch from f1f5114 to 998fad6 Compare May 14, 2021 22:54

chrisbobbe force-pushed the pr-server-version-nag-banner-lt-2.0.0 branch from 998fad6 to b091544 Compare May 19, 2021 00:24

chrisbobbe changed the title ~~ServerCompatNotice: Add, to nag users connected to servers <2.0.0.~~ ServerCompatNotice: Add, to nag users connected to servers <2.1.0. May 19, 2021

chrisbobbe force-pushed the pr-server-version-nag-banner-lt-2.0.0 branch from b091544 to 13ceda0 Compare May 19, 2021 23:57

WesleyAC reviewed May 21, 2021

View reviewed changes

chrisbobbe mentioned this pull request May 21, 2021

Add tests for ZulipAsyncStorage.getItem and .setItem. #4709

Merged

gnprice reviewed May 25, 2021

View reviewed changes

chrisbobbe force-pushed the pr-server-version-nag-banner-lt-2.0.0 branch from 13ceda0 to c8db18b Compare May 26, 2021 19:24

chrisbobbe added 5 commits May 27, 2021 21:31

Label [nfc]: Make Label explicitly a wrapper for RawLabel.

5681963

This way, the components don't both have to have code for default styling.

RawLabel [nfc]: Bring together default styles; comment on a gotcha.

15105fb

WebLink [nfc]: Move out some styling code that callers should handle.

cb40334

Using `Text`'s nesting interface we've had `RawLabel` start exposing.

PasswordAuthScreen [nfc]: Don't redundantly set a font size to its de…

f3b6143

…fault. The default is provided by `RawLabel`.

chrisbobbe and others added 7 commits May 27, 2021 21:31

sessionReducer [nfc]: Comment on the mechanism for not persisting to …

9559a75

…disk.

session: Add Redux boilerplate for dismissing the Zulip version nag b…

aab915f

…anner. Which we'll add in the next commit, using this setup.

ZulipTextButton: Add, to implement Material's "text button".

c8a8812

ZulipTextButton [nfc]: Add TODO for maybe using react-native-paper.

fddcaf9

As suggested by Wesley [1]. It'll take a bit more work to set up [2] than I have time for right now, but this could be a potentially useful follow-up. [1] zulip#4750 (comment) [2] zulip#4709 (comment)

ServerCompatBanner [nfc]: Pull out threshold as constant.

a208713

This isolates it a bit from the logic, for future edits; it also makes more room for the comment to explain the relationship between this threshold and our actual oldest supported version.

gnprice force-pushed the pr-server-version-nag-banner-lt-2.0.0 branch from c8db18b to a208713 Compare May 29, 2021 03:47

gnprice merged commit a208713 into zulip:master May 29, 2021

gnprice reviewed May 29, 2021

View reviewed changes

This was referenced Jun 3, 2021

Add basic support for polls (viewing and voting) #4704

Merged

Followups from #4679 #4768

Merged

chrisbobbe changed the title ~~ServerCompatNotice: Add, to nag users connected to servers <2.1.0.~~ ServerCompatNotice: Add, to nag users connected to servers <2.0.0. Jun 14, 2021

gnprice mentioned this pull request Jun 16, 2021

Add mark as read option in TopicActionSheet and Modify TopicActionSheet to use stream_id. #4635

Merged

gnprice mentioned this pull request Aug 25, 2021

ServerCompatBanner: Bump minSupportedVersion to 2.1.0, as planned. #4962

Merged

chrisbobbe mentioned this pull request Sep 22, 2021

redux: Distinguish per-account vs global state (1/n) #5017

Merged

chrisbobbe deleted the pr-server-version-nag-banner-lt-2.0.0 branch November 4, 2021 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ServerCompatNotice: Add, to nag users connected to servers <2.0.0. #4750

ServerCompatNotice: Add, to nag users connected to servers <2.0.0. #4750

chrisbobbe commented May 14, 2021 •

edited

alya commented May 14, 2021

alya commented May 14, 2021

alya commented May 14, 2021

timabbott commented May 14, 2021 •

edited

chrisbobbe commented May 19, 2021 •

edited

chrisbobbe commented May 19, 2021

chrisbobbe commented May 19, 2021 •

edited

alya commented May 19, 2021

alya commented May 19, 2021

chrisbobbe commented May 19, 2021 •

edited

alya commented May 20, 2021 via email

WesleyAC left a comment

WesleyAC May 21, 2021

chrisbobbe May 21, 2021 •

edited

WesleyAC May 21, 2021

chrisbobbe May 21, 2021 •

edited

WesleyAC May 21, 2021

chrisbobbe May 21, 2021 •

edited

gnprice left a comment

gnprice May 25, 2021

gnprice commented May 25, 2021

chrisbobbe commented May 26, 2021

gnprice left a comment

gnprice May 28, 2021

chrisbobbe commented Jun 3, 2021

WesleyAC commented Jun 14, 2021

WesleyAC commented Jun 14, 2021

		(text === undefined && children !== undefined)
		\|\| (text !== undefined && children === undefined),

ServerCompatNotice: Add, to nag users connected to servers <2.0.0. #4750

ServerCompatNotice: Add, to nag users connected to servers <2.0.0. #4750

Conversation

chrisbobbe commented May 14, 2021 • edited

iOS

Android

alya commented May 14, 2021

alya commented May 14, 2021

alya commented May 14, 2021

timabbott commented May 14, 2021 • edited

chrisbobbe commented May 19, 2021 • edited

chrisbobbe commented May 19, 2021

chrisbobbe commented May 19, 2021 • edited

alya commented May 19, 2021

alya commented May 19, 2021

chrisbobbe commented May 19, 2021 • edited

alya commented May 20, 2021 via email

WesleyAC left a comment

Choose a reason for hiding this comment

WesleyAC May 21, 2021

Choose a reason for hiding this comment

chrisbobbe May 21, 2021 • edited

Choose a reason for hiding this comment

WesleyAC May 21, 2021

Choose a reason for hiding this comment

chrisbobbe May 21, 2021 • edited

Choose a reason for hiding this comment

WesleyAC May 21, 2021

Choose a reason for hiding this comment

chrisbobbe May 21, 2021 • edited

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

gnprice May 25, 2021

Choose a reason for hiding this comment

gnprice commented May 25, 2021

chrisbobbe commented May 26, 2021

gnprice left a comment

Choose a reason for hiding this comment

gnprice May 28, 2021

Choose a reason for hiding this comment

chrisbobbe commented Jun 3, 2021

WesleyAC commented Jun 14, 2021

WesleyAC commented Jun 14, 2021

chrisbobbe commented May 14, 2021 •

edited

timabbott commented May 14, 2021 •

edited

chrisbobbe commented May 19, 2021 •

edited

chrisbobbe commented May 19, 2021 •

edited

chrisbobbe commented May 19, 2021 •

edited

chrisbobbe May 21, 2021 •

edited

chrisbobbe May 21, 2021 •

edited

chrisbobbe May 21, 2021 •

edited