-
-
Notifications
You must be signed in to change notification settings - Fork 670
api: Don't allow connecting to servers <2.0 #5633
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
Conversation
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 @chrisbobbe for buliding this!
The code generally looks good; one comment below.
In the UX: I feel like it'd be helpful, when saying that the given version is unsupported, to say what the minimum version is. So e.g., added to the end of the text, "The minimum version is Zulip Server 2.0."
In particular mentioning a "minimum" version, and having it be a bigger number than the rejected version, would help clarify that the server's version is "unsupported" because it's too old, and that what's needed is a more up-to-date version.
src/api/apiErrors.js
Outdated
// https://github.com/zulip/zulip-mobile/issues/5102#issuecomment-1233446360 | ||
// In steady state, this should lag a bit behind the threshold version for | ||
// ServerCompatBanner, to give users time to see and act on the banner. | ||
export const kThresholdZulipVersion: ZulipVersion = new ZulipVersion('2.0'); |
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 name feels pretty non-specific to me — it says this is a version where we make some kind of distinction between above and below it, but it doesn't really say what that distinction is.
Tempted to say kMinZulipVersion
, but I guess there's already a kMinSupportedVersion
with a different meaning, so it's better to draw more specifically the contrast from that one.
How about kMinAllowedServerVersion
?
53f2df2
to
5cd4c16
Compare
Thanks for the review! Revision pushed. |
@@ -84,6 +84,7 @@ | |||
"Could not connect": "Could not connect", | |||
"The server at {realm} said:\n\n{message}": "The server at {realm} said:\n\n{message}", | |||
"Could not connect to {realm}. Please check your network connection and try again.": "Could not connect to {realm}. Please check your network connection and try again.", | |||
"{realm} is running Zulip Server {version}, which is unsupported. The minimum version is Zulip Server {minSupportedVersion}.": "{realm} is running Zulip Server {version}, which is unsupported. The minimum version is Zulip Server {minSupportedVersion}.", | |||
"The server at {realm} does not seem to be a Zulip server.": "The server at {realm} does not seem to be a Zulip server.", |
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.
Maybe "the minimum supported version"?
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 agree that seems clearer.
5cd4c16
to
492a53b
Compare
Thanks for the review! Revision pushed. |
492a53b
to
b1a25a5
Compare
With an error alert that appears at the following points: - On submitting `RealmInputScreen` (titled "Welcome") - On selecting a logged-out account in `AccountPickScreen` (titled "Pick account") - When we connect to the active account's server (get a `/register` response; the "Connecting" banner disappears) and we learn its current server version Fixes: zulip#5102
b1a25a5
to
3b811d6
Compare
Thanks for the revision! Looks good; merging. |
…ion numbers. Previously I've wanted to have this page spell out the concrete version number that our clients support, rather than the policy we use for determining that version number, because that's the sort of question that I feel like as a user I'd want a straight answer to and would be annoyed if I couldn't get one. But as the text stands, it's come to look more like it's the policy (something that's heavyweight to change) than like the value that the policy currently happens to work out to. Also, because this page is kind of chaotically organized (and fixing that is a bigger yak than I want to shave right now), it repeats the 18-month rule in three separate places and the current value (version 4.0) is in a fourth separate place, so it looks internally inconsistent. Let's therefore take a different tack: like in those other three spots on this page, state just the policy instead of the value it currently works out to; but also add a link to help the reader pin down for themselves what value that does work out to. This also means we no longer need to update this page as old releases age and that value advances. Also fix a typo, and cut the reference to working degraded on older releases. Starting earlier this year we finally started hard-refusing such connections: zulip/zulip-mobile#5102 zulip/zulip-mobile#5633 (which was because there were some swathes of compatibility code that we could only cut if we completely broke the handling of ancient servers, and so we preferred to have the app communicate that break clearly up front.)
…ion numbers. Previously I've wanted to have this page spell out the concrete version number that our clients support, rather than the policy we use for determining that version number, because that's the sort of question that I feel like as a user I'd want a straight answer to and would be annoyed if I couldn't get one. But as the text stands, it's come to look more like it's the policy (something that's heavyweight to change) than like the value that the policy currently happens to work out to. Also, because this page is kind of chaotically organized (and fixing that is a bigger yak than I want to shave right now), it repeats the 18-month rule in three separate places and the current value (version 4.0) is in a fourth separate place, so it looks internally inconsistent. Let's therefore take a different tack: like in those other three spots on this page, state just the policy instead of the value it currently works out to; but also add a link to help the reader pin down for themselves what value that does work out to. This also means we no longer need to update this page as old releases age and that value advances. Also fix a typo, and cut the reference to working degraded on older releases. Starting earlier this year we finally started hard-refusing such connections: zulip/zulip-mobile#5102 zulip/zulip-mobile#5633 (which was because there were some swathes of compatibility code that we could only cut if we completely broke the handling of ancient servers, and so we preferred to have the app communicate that break clearly up front.)
…ion numbers. Previously I've wanted to have this page spell out the concrete version number that our clients support, rather than the policy we use for determining that version number, because that's the sort of question that I feel like as a user I'd want a straight answer to and would be annoyed if I couldn't get one. But as the text stands, it's come to look more like it's the policy (something that's heavyweight to change) than like the value that the policy currently happens to work out to. Also, because this page is kind of chaotically organized (and fixing that is a bigger yak than I want to shave right now), it repeats the 18-month rule in three separate places and the current value (version 4.0) is in a fourth separate place, so it looks internally inconsistent. Let's therefore take a different tack: like in those other three spots on this page, state just the policy instead of the value it currently works out to; but also add a link to help the reader pin down for themselves what value that does work out to. This also means we no longer need to update this page as old releases age and that value advances. Also fix a typo, and cut the reference to working degraded on older releases. Starting earlier this year we finally started hard-refusing such connections: zulip/zulip-mobile#5102 zulip/zulip-mobile#5633 (which was because there were some swathes of compatibility code that we could only cut if we completely broke the handling of ancient servers, and so we preferred to have the app communicate that break clearly up front.) (cherry picked from commit bb6fe03)
…ion numbers. Previously I've wanted to have this page spell out the concrete version number that our clients support, rather than the policy we use for determining that version number, because that's the sort of question that I feel like as a user I'd want a straight answer to and would be annoyed if I couldn't get one. But as the text stands, it's come to look more like it's the policy (something that's heavyweight to change) than like the value that the policy currently happens to work out to. Also, because this page is kind of chaotically organized (and fixing that is a bigger yak than I want to shave right now), it repeats the 18-month rule in three separate places and the current value (version 4.0) is in a fourth separate place, so it looks internally inconsistent. Let's therefore take a different tack: like in those other three spots on this page, state just the policy instead of the value it currently works out to; but also add a link to help the reader pin down for themselves what value that does work out to. This also means we no longer need to update this page as old releases age and that value advances. Also fix a typo, and cut the reference to working degraded on older releases. Starting earlier this year we finally started hard-refusing such connections: zulip/zulip-mobile#5102 zulip/zulip-mobile#5633 (which was because there were some swathes of compatibility code that we could only cut if we completely broke the handling of ancient servers, and so we preferred to have the app communicate that break clearly up front.) (cherry picked from commit bb6fe03)
With an error alert that appears at the following points:
On submitting
RealmInputScreen
(titled "Welcome"):On selecting a logged-out account in
AccountPickScreen
(titled "Pick account"):When we get a
/register
response from the active account's server. (The first screenshot is what happens on launch; the second is what happens when you select a logged-in account inAccountPickScreen
.)The alert says:
Title:
Body:
The "Learn more" button goes to https://zulip.readthedocs.io/en/stable/overview/release-lifecycle.html#compatibility-and-upgrading but later we'll want to change that to a new page; that's zulip/zulip#23842.
cc @alya for the UX change.
Fixes: #5102