Store server feature level in Redux #4079

hashirsarwar · 2020-04-30T09:24:04Z

Following #14658, we now obtain feature level from Zulip server and store it in Redux. This is designed to provide a simple way for mobile apps to decide whether the server supports a given feature or API change.

Closes #4049.

hashirsarwar · 2020-04-30T09:24:56Z

@gnprice @chrisbobbe this is ready for a review. :)

chrisbobbe · 2020-04-30T17:07:11Z

Thanks, @hashirsarwar!

Just a note to @gnprice that zulipFeatureLevel: number | null, on Account, instead of zulipFeatureLevel?: number, was on my advice, based on e026c6a (in PR #4047, currently a draft) — so if this is wrong, please let me know too. 🙂

chrisbobbe

Thanks, @hashirsarwar, I've just had a closer look! Some comments below, and I'm sure @gnprice will have more to add.

Generally, for the part of the commit message that explains what work was done, it's a bit more concise (and, more importantly, consistent with a single style that we've chosen) to use the imperative mood. (I'm still torn on whether it's actually a kind of bare infinitive, but I digress.)

So, instead of

"This adds a new property featureLevel in the type Account."

we tend to write,

"Add a new property featureLevel in the type Account."

src/api/settings/getServerSettings.js

src/__tests__/lib/exampleData.js

hashirsarwar · 2020-04-30T19:07:03Z

@chrisbobbe thanks for the review.

As far as commit messages are concerned, I think we use imperative mood in summaries. In case of message bodies, there is no as such restriction. See the commit message example here provided in the documentation.

However, I'll make the requested changes soon. 🙂

chrisbobbe · 2020-04-30T19:20:07Z

As far as commit messages are concerned, I think we use imperative mood in summaries. In case of message bodies, there is no as such restriction. See the commit message example here provided in the documentation.

Hmm, interesting. I would have thought the distinction is made on a different axis — not whether the words are in the summary or in the body, but rather, whether they describe some work that gets done in the commit, as opposed to background information, motivations for the work, plans for the future, maybe even close or distant effects of the work, etc. My reading of the commit you've linked to has it following this pattern, too.

But the doc does indeed mention "imperative" several times in the section about the summary, and zero times in the section about the body.

@gnprice, @ray-kraesig, thoughts?

hashirsarwar · 2020-04-30T19:38:59Z

@chrisbobbe here are some other examples of recent commit messages from the web repository.

Still, I would like to know what others think about it. 🙂
Thanks once again!

timabbott · 2020-04-30T19:43:47Z

That's correct, imperative mood is only expected in the summary line. The paragraph body is a more normal paragraph explaining relevant background/motivation/etc. for the change.

chrisbobbe · 2020-04-30T20:31:31Z

Hmm, the commits in that screenshot also seem to follow both patterns. 🙂

@timabbott said:

That's correct, imperative mood is only expected in the summary line. The paragraph body is a more normal paragraph explaining relevant background/motivation/etc. for the change.

Hmm, OK. @timabbott, it sounds like there's an additional constraint that the entire work of the change (excluding background, motivation, etc.) be condensed into the summary line. With the summary line's character limit, might it sometimes be impossible to preserve the required amount of detail in that class of explanation, even with minimal, coherent commits? E.g., when follow-on changes on a main change are needed in the same commit, they often deserve their own explanation, right, which I'm not sure would count as background info?

Anyway, @hashirsarwar, for the commit I mentioned here:

f514d6a Account type: Add a property featureLevel.

That bit I highlighted from the body, "This adds a new property featureLevel in the type Account.", seems redundant; you've got it (in the imperative, good 🙂) in the summary line.

src/types.js

gnprice · 2020-05-01T00:24:31Z

The paragraph body is a more normal paragraph explaining relevant background/motivation/etc. for the change.

@timabbott I think if you look again you'll agree that this is a stronger statement than you really intend. Among your own recent commit messages, it's not uncommon to have some "how" or high-level "what" in the commit message body, in addition to "why" and background:

While working on this, I fixed various issues where feature levels
could be mentioned or endpoints didn't properly document changes.

[...]  We fix this inconsistency by making
the database model case-insensitive.

Fix this by ensuring all the files we generate are in this special
subdirectory.

Also improve the headings for it.

and I think that's for good reasons -- sometimes there's just more to say than makes sense to put in the summary line.

imperative mood is only expected in the summary line

It looks like the practice on this is mixed. At a quick survey of recent zulip.git history:

As quoted above, your messages sometimes put the "what" in the same syntactic form we use in summary lines, and sometimes in the plain indicative like we do for background in the bodies.
Mine generally have the "what" match the summary line.
Anders tends to write shorter messages; when they do include some "what", it matches the summary line.
Steve includes some "what" in the body more frequently, and consistently has it in the indicative, like the background in the body.

In the mobile repo, we've more or less consistently used the same form for describing "what" in the body (if present there) as we do in the summary line.

I don't feel an urgency to make the style uniform one way or another in zulip.git, but unsurprisingly I do like better the style I use. 😉 One reason is that a sentence in the indicative can easily be ambiguous about whether it's describing something already true before this commit (as background) or something newly made true by this commit.

In any case we've been pretty consistent in zulip-mobile, and so I'd like to generally maintain that consistency there.

chrisbobbe · 2020-05-01T00:31:32Z

@hashirsarwar, I like these changes!

@gnprice and/or @ray-kraesig will likely have more to add. @gnprice, there's a query here, which perhaps I should have posted on the PR thread, and in any case will quote here:

@gnprice said:

Then we'll want to use it instead of the version number anywhere we can -- basically, for any new server feature that's after 2.1.0.

@gnprice, do we know the particular minimum server version that will have this flag? The above implies that such a minimum server version exists, but it's not 2.1.0. Maybe 2.1.1?

Just so we can put the right comment on zulip_feature_level in ApiResponseServerSettings.

gnprice · 2020-05-01T00:43:15Z

do we know the particular minimum server version that will have this flag?

Ah, my wording here was confusing:

basically, for any new server feature that's after 2.1.0.

In general new features don't appear in a new Zulip patch release, i.e. a version a.b.(c+1) vs. a.b.c. Rather they only appear in a new major release, which is a.(b+1).0 or (a+1).0.0.

So a "new server feature that's after 2.1.0" means one whose first release is 2.2.0. (Or 3.0.0 if we were to go straight there, but we won't.)

For this purpose, though:

Just so we can put the right comment on zulip_feature_level in ApiResponseServerSettings.

I think the answer is:

In the past (and in the future, for server features introduced in the past), we'd identify a zulip.git commit and the release it appeared in.
Thanks to this very feature, and to improvements in the Zulip API documentation, I think we can shift to something that's both easier and usually more helpful!

Specifically, this can say something like "Added in server version 2.2, feature level 1."

And there's already a link in that file to https://zulipchat.com/api/server-settings , which documents that fact. (I guess in this instance it's not 100% clear about the "feature level 1" detail -- but in general the API docs will now say things like "New in Zulip 2.2 (feature level 2)" or whatever.)

src/message/fetchActions.js

hashirsarwar · 2020-05-01T12:47:19Z

Okay, based on your feedback, I have made the following changes:

Added a commit to include the zulip_feature_level property obtained from /register response in InitialDataRealm.
Since we have zulip_feature_level in initData for realmInit(), we don't need to mantain this value seprately. So, I have removed two commits that added a seprate feature level property in RealmInitAction and dispatched realmInit() with the redundant feature level value as @gnprice mentioned.
Fixed some typos in the comments and commit messages.

@chrisbobbe @gnprice kindly have a look. 🙂

rk-for-zulip

From the commit messages:

The property is optional because some older versions of the server may still not include feature level in the /register response.

Let's not imply that this is in doubt: "... because older versions of the server do not include ...".

Also, although we can (mostly) move to feature level indicators going forward, I think I'd prefer to have the docstrings in this PR act as a bridge, including the server commit ID alongside "feature level 1". (If nothing else, I expect it to help a confused future me find the place in the Zulip server codebase where the feature level is stored.)

src/__tests__/lib/exampleData.js

gnprice

Thanks @hashirsarwar ! Several comments below.

src/api/initialDataTypes.js

src/api/settings/getServerSettings.js

src/api/initialDataTypes.js

src/api/settings/getServerSettings.js

src/start/RealmScreen.js

src/account/accountsReducer.js

src/types.js

hashirsarwar · 2020-05-10T02:34:32Z

@gnprice thanks for the feedback. Can you review this once again?

Like we did for `zulipVersion`, except the interesting data is found on `action.data`, not `action`, as I mention in zulip#4079 (comment).

This reorganization does a few things: - Focuses each test case to just a few lines. As Greg points out [1], "[w]hen each one is just a few lines, tests one thing, and it's clear what that one thing is, that helps make it comfortable to have a number of different test cases and really think systematically through having them cover all the interesting different situations. - Tweaks the claim for one test that begins, 'if no account with this realm exists'. Greg points out [2] that a test like this would be redundant: "The case where the account doesn't exist in the state can I think be covered by the "account is not in state" test case in the first pair, because that can just do an equality test on the whole resulting state." This comment from Greg was already addressed with the pair of tests on `zulipVersion`, but not those on `zulipFeatureLevel`. - Brings the tests' logic in line with their claims about the previous state's contents. For example, if a test says that an account in the previous state has a null zulipFeatureLevel, make sure that the account in the previous state actually has a null zulipFeatureLevel. It made sense to stop using the `prevState` for these tests. [1] zulip#4079 (comment) [2] zulip#4079 (comment)

Like we did for `zulipVersion`, except the interesting data is found on `action.data`, not `action`, as I mention in zulip#4079 (comment).

This reorganization does a few things: - Focuses each test case to just a few lines. As Greg points out [1], "[w]hen each one is just a few lines, tests one thing, and it's clear what that one thing is, that helps make it comfortable to have a number of different test cases and really think systematically through having them cover all the interesting different situations. - Tweaks the claim for one test that begins, 'if no account with this realm exists'. Greg points out [2] that a test like this would be redundant: "The case where the account doesn't exist in the state can I think be covered by the "account is not in state" test case in the first pair, because that can just do an equality test on the whole resulting state." This comment from Greg was already addressed with the pair of tests on `zulipVersion`, but not those on `zulipFeatureLevel`. - Brings the tests' logic in line with their claims about the previous state's contents. For example, if a test says that an account in the previous state has a null zulipFeatureLevel, make sure that the account in the previous state actually has a null zulipFeatureLevel. It made sense to stop using the `prevState` for these tests. [1] zulip#4079 (comment) [2] zulip#4079 (comment)

chrisbobbe · 2020-09-18T21:19:15Z

Thanks, @hashirsarwar! I've made a few tweaks as commits on top of this branch, and I pinged @gnprice on CZO to double-check them.

Like we did for `zulipVersion`, except the interesting data is found on `action.data`, not `action`, as I mention in zulip#4079 (comment).

This reorganization does a few things: - Focuses each test case to just a few lines. As Greg points out [1], "[w]hen each one is just a few lines, tests one thing, and it's clear what that one thing is, that helps make it comfortable to have a number of different test cases and really think systematically through having them cover all the interesting different situations. - Tweaks the claim for one test that begins, 'if no account with this realm exists'. Greg points out [2] that a test like this would be redundant: "The case where the account doesn't exist in the state can I think be covered by the "account is not in state" test case in the first pair, because that can just do an equality test on the whole resulting state." This comment from Greg was already addressed with the pair of tests on `zulipVersion`, but not those on `zulipFeatureLevel`. - Brings the tests' logic in line with their claims about the previous state's contents. For example, if a test says that an account in the previous state has a null zulipFeatureLevel, make sure that the account in the previous state actually has a null zulipFeatureLevel. It made sense to stop using the `prevState` for these tests. [1] zulip#4079 (comment) [2] zulip#4079 (comment)

As the server-side feature level is implemented, we now add a new property `zulip_feature_level` in `ApiResponseServerSettigs` and `InitialDataRealm` to store the feature level from the responses of /server_settings and /register. The property is optional because older versions of the server do not include feature level in the /server_settings and /register responses. Part of zulip#4049.

This makes REALM_ADD tests clear by handling each case separately and removing some redundant lines.

The feature level accessed from the /server_settings response is passed to the realmAdd action creator. Part of zulip#4049.

Firstly, we add zulipFeatureLevel in Account type. We had to tweak `loginSuccess()` and `realmAdd()` methods in accountsReducer to include `zulipFeatureLevel` in their return values too. Then, we store the feature level in Redux on REALM_ADD and REALM_INIT. Part of zulip#4049.

Continue our pattern of using this where we can, since it's slightly easier to read. A doc about this operator from MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing_operator

Like we did for `zulipVersion`, except the interesting data is found on `action.data`, not `action`, as I mention in zulip#4079 (comment).

This reorganization does a few things: - Focuses each test case to just a few lines. As Greg points out [1], "[w]hen each one is just a few lines, tests one thing, and it's clear what that one thing is, that helps make it comfortable to have a number of different test cases and really think systematically through having them cover all the interesting different situations. - Tweaks the claim for one test that begins, 'if no account with this realm exists'. Greg points out [2] that a test like this would be redundant: "The case where the account doesn't exist in the state can I think be covered by the "account is not in state" test case in the first pair, because that can just do an equality test on the whole resulting state." This comment from Greg was already addressed with the pair of tests on `zulipVersion`, but not those on `zulipFeatureLevel`. - Brings the tests' logic in line with their claims about the previous state's contents. For example, if a test says that an account in the previous state has a null zulipFeatureLevel, make sure that the account in the previous state actually has a null zulipFeatureLevel. It made sense to stop using the `prevState` for these tests. [1] zulip#4079 (comment) [2] zulip#4079 (comment)

In particular: * Pull out `baseAction`, reducing repetition of boring bits that don't matter to each test so that things that do (like `newZulipVersion`) better stand out. * Inline some single-use variables like `expectedState` that don't add clarity, making the tests shorter. * Write the properties of an Account object in a consistent order, starting with its unique key of realm + email.

Each of these started by constructing a piece of the expected result state, and then went on to construct the input. That works but gets confusing to read, as the logical flow of information zigzags up and down the test code. Instead, describe first the input and then the expected result. Meanwhile in the "move to front of list" test, as the name suggests, what we're looking for is for the matching account information to move to the front; so say that directly instead of constructing a fresh example account. There are nuances when the zulipVersion or zulipFeatureLevel needs an update, but we cover those in their own test cases just below, and the fresh `eg.makeAccount` call didn't really add any clarity about them.

No need to give names to `newState`, `expectedState`, etc., when they're each used just once and their roles can be communicated directly by where they appear in the `expect` expression. Also no need to check `expect(newState).not.toBe(prevState)`; we're already checking it's equal to `expectedState`, and the latter is indeed not equal to `prevState`.

Like with the existing nested `describe` calls below, this helps organize the tests and in particular allows each group of them to have their own local fixtures like `baseAction`.

In particular, in migration 12 there's no need to check on the existing `a.zulipVersion`; that property must be missing, because in versions of the app before we added that migration it didn't exist.

gnprice · 2020-09-19T00:31:46Z

Thanks @chrisbobbe ! These look good. Merged, with a few additional commits on top; take a look.

chrisbobbe · 2020-09-19T02:50:29Z

Thanks @chrisbobbe ! These look good. Merged, with a few additional commits on top; take a look.

Thanks, and in particular, thanks for those additional commits, I read them all! 🙂

chrisbobbe requested changes Apr 30, 2020

View reviewed changes

src/api/settings/getServerSettings.js Outdated Show resolved Hide resolved

src/__tests__/lib/exampleData.js Outdated Show resolved Hide resolved

src/__tests__/lib/exampleData.js Outdated Show resolved Hide resolved

src/__tests__/lib/exampleData.js Show resolved Hide resolved

hashirsarwar force-pushed the feature-level branch from f9ad6af to 853f90c Compare April 30, 2020 23:45

hashirsarwar requested a review from chrisbobbe April 30, 2020 23:48

chrisbobbe reviewed May 1, 2020

View reviewed changes

src/types.js Outdated Show resolved Hide resolved

gnprice reviewed May 1, 2020

View reviewed changes

src/message/fetchActions.js Outdated Show resolved Hide resolved

hashirsarwar force-pushed the feature-level branch from 853f90c to 251d249 Compare May 1, 2020 12:29

rk-for-zulip reviewed May 1, 2020

View reviewed changes

src/__tests__/lib/exampleData.js Outdated Show resolved Hide resolved

hashirsarwar force-pushed the feature-level branch 6 times, most recently from 7eaea28 to 58f0202 Compare May 6, 2020 20:40

hashirsarwar requested review from chrisbobbe and gnprice May 7, 2020 06:34

gnprice reviewed May 8, 2020

View reviewed changes

hashirsarwar force-pushed the feature-level branch from 58f0202 to f391df7 Compare May 10, 2020 02:31

hashirsarwar force-pushed the feature-level branch from f391df7 to cfcdbf5 Compare May 10, 2020 02:50

chrisbobbe force-pushed the feature-level branch from 22f50a6 to 563bbbc Compare September 18, 2020 21:08

chrisbobbe force-pushed the feature-level branch from 563bbbc to 6c14907 Compare September 18, 2020 21:13

gnprice force-pushed the feature-level branch from 6c14907 to 0c1374e Compare September 19, 2020 00:30

hashirsarwar and others added 13 commits September 18, 2020 17:30

accountsReducer-test: Make REALM_ADD tests more coherent.

d200fc8

This makes REALM_ADD tests clear by handling each case separately and removing some redundant lines.

RealmScreen: Include feature level from /server_settings in REALM_ADD.

1e51c0f

The feature level accessed from the /server_settings response is passed to the realmAdd action creator. Part of zulip#4049.

RealmScreen: Use slightly neater nullish coalescing operator.

d666225

Continue our pattern of using this where we can, since it's slightly easier to read. A doc about this operator from MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing_operator

accountsReducer tests: Test that REALM_INIT records feature level.

c3b7a4d

Like we did for `zulipVersion`, except the interesting data is found on `action.data`, not `action`, as I mention in zulip#4079 (comment).

accountsReducer tests: Further group the REALM_ADD test cases.

8d8645e

Like with the existing nested `describe` calls below, this helps organize the tests and in particular allows each group of them to have their own local fixtures like `baseAction`.

types [nfc]: Describe null case of newish Account properties.

d11c3b4

store [nfc]: Slightly simplify migrations; clarify comments.

e122cfb

In particular, in migration 12 there's no need to check on the existing `a.zulipVersion`; that property must be missing, because in versions of the app before we added that migration it didn't exist.

gnprice force-pushed the feature-level branch from 0c1374e to e122cfb Compare September 19, 2020 00:31

gnprice merged commit e122cfb into zulip:master Sep 19, 2020

gnprice mentioned this pull request Sep 19, 2020

Support setting streams announcement-only, and other post policies #3131

Open

chrisbobbe mentioned this pull request Dec 28, 2020

webview: Use native scroll deceleration on iOS #4360

Merged

chrisbobbe mentioned this pull request Feb 15, 2021

crash: Fix crash in getUserFromMention as unformattedMessage gives undefined. #4474

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store server feature level in Redux #4079

Store server feature level in Redux #4079

hashirsarwar commented Apr 30, 2020 •

edited

hashirsarwar commented Apr 30, 2020

chrisbobbe commented Apr 30, 2020

chrisbobbe left a comment

hashirsarwar commented Apr 30, 2020

chrisbobbe commented Apr 30, 2020 •

edited

hashirsarwar commented Apr 30, 2020

timabbott commented Apr 30, 2020 •

edited

chrisbobbe commented Apr 30, 2020 •

edited

gnprice commented May 1, 2020 •

edited

chrisbobbe commented May 1, 2020

gnprice commented May 1, 2020 •

edited

hashirsarwar commented May 1, 2020

rk-for-zulip left a comment

gnprice left a comment

hashirsarwar commented May 10, 2020

chrisbobbe commented Sep 18, 2020

gnprice commented Sep 19, 2020

chrisbobbe commented Sep 19, 2020 •

edited

Store server feature level in Redux #4079

Store server feature level in Redux #4079

Conversation

hashirsarwar commented Apr 30, 2020 • edited

hashirsarwar commented Apr 30, 2020

chrisbobbe commented Apr 30, 2020

chrisbobbe left a comment

Choose a reason for hiding this comment

hashirsarwar commented Apr 30, 2020

chrisbobbe commented Apr 30, 2020 • edited

hashirsarwar commented Apr 30, 2020

timabbott commented Apr 30, 2020 • edited

chrisbobbe commented Apr 30, 2020 • edited

gnprice commented May 1, 2020 • edited

chrisbobbe commented May 1, 2020

gnprice commented May 1, 2020 • edited

hashirsarwar commented May 1, 2020

rk-for-zulip left a comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

hashirsarwar commented May 10, 2020

chrisbobbe commented Sep 18, 2020

gnprice commented Sep 19, 2020

chrisbobbe commented Sep 19, 2020 • edited

hashirsarwar commented Apr 30, 2020 •

edited

chrisbobbe commented Apr 30, 2020 •

edited

timabbott commented Apr 30, 2020 •

edited

chrisbobbe commented Apr 30, 2020 •

edited

gnprice commented May 1, 2020 •

edited

gnprice commented May 1, 2020 •

edited

chrisbobbe commented Sep 19, 2020 •

edited