Store server version in Redux (WIP for #3745) #3839

chrisbobbe · 2020-01-24T02:44:33Z

This is WIP for the part of #3745 only concerned with storing the version number in Redux.

chrisbobbe · 2020-01-27T21:46:27Z

OK, I've got all tests passing; this should be ready for a first review, when it's convenient.

rk-for-zulip

As an overarching note, I'd suggest:

using zulipVersion?: string instead. This lets the migration be the identity function, and also keeps local placeholder values definitely distinct from server-sent values. (undefined can't be sent as part of a JSON object.)
alternately, consider using zulipVersion: string instead. This requires us to write a migration, but Zulip servers have been sending zulip_version since at least 1.5.1 in 2017. (Not sure if that's old enough not to worry about.)
either way, using number[] instead of string (possibly with rawZulipVersion cached for reference or reporting). This keeps us from trying to compare Zulip version strings with < (since, incorrectly, "2.10.3" < "2.2.1").

rk-for-zulip · 2020-01-30T23:51:01Z

src/__tests__/exampleData.js

  });

-const makeAccount = (user: User): Account =>
+export const makeAccount = (email: string, realm: string, shouldHaveApiKey: boolean): Account =>


This should probably take a parameters-object, for clarity.

rk-for-zulip · 2020-01-30T23:51:39Z

src/account/__tests__/accountsReducer-test.js

+      const account1 = deepFreeze(eg.makeAccount('[email protected]', 'realm1', false));
+      const account2 = deepFreeze(eg.makeAccount('[email protected]', 'realm2', false));


No need for this: makeAccount is already deepFrozen.

rk-for-zulip · 2020-01-30T23:54:46Z

src/account/__tests__/accountsReducer-test.js

+      const account2 = deepFreeze(eg.makeAccount('[email protected]', 'http://realm2.com', true));
+      const account3 = deepFreeze(eg.makeAccount('[email protected]', 'http://realm3.com', true));
+
+      const prevState = deepFreeze([account1, account2, account3]);


If the initial state is both immutable and common to many of the tests in the describe, you might want to move them up out of the test blocks and into their shared describe.

rk-for-zulip · 2020-01-31T00:23:38Z

src/account/accountActions.js

 });

-export const realmAdd = (realm: string): Action => ({
+export const realmAdd = (realm: string, zulipVersion: string | null): Action => ({


This seems like it should take the entire settings object, and let the reducers pick out what they want.

(Mostly because that'll be easier to extend later. But that may be another way of saying that "what parts of the server settings object will be stored" is an implementation detail that doesn't need to be exposed here.)

Sure, that reasoning makes sense. Let's ping @gnprice, though, because I think he disagrees, reasoning that we only need the zulipVersion now, and that the barrier to extending it later is fairly low (#3745 (comment)).

chrisbobbe · 2020-01-31T21:42:19Z

either way, using number[] instead of string (possibly with rawZulipVersion cached for reference or reporting). This keeps us from trying to compare Zulip version strings with < (since, incorrectly, "2.10.3" < "2.2.1").

Makes sense.

2.1.0-301-gd20ce6a

^ There's an example zulipVersion. @timabbott, I'm wondering what 301 and gd20ce6a refer to. git log gd20ce6a shows gd20ce6a isn't a commit ID in zulip/zulip.

timabbott · 2020-01-31T22:50:04Z

Yeah, so that's the format for our versions for Git checkouts of Zulip. That one means it's a common with 301 commits on top of Zulip 2.1.0 (with commit ID starting with gd20ce6a).

@andersk is it intentional that we're not using a 2.2.x based value in master for zulip-git-version? That seems like a bug to me, the committed value (used in development) is 2.2.dev+git.

andersk · 2020-01-31T23:04:19Z

Yeah, so that's the format for our versions for Git checkouts of Zulip. That one means it's a common with 301 commits on top of Zulip 2.1.0 (with commit ID starting with gd20ce6a).

Starting with d20ce6a; the g is fixed. This is the git describe format. Git also accepts this format as input directly: git log 2.1.0-301-gd20ce6a. The commit in question in this case must be a custom commit.

@andersk is it intentional that we're not using a 2.2.x based value in master for zulip-git-version? That seems like a bug to me, the committed value (used in development) is 2.2.dev+git.

If we want that (and I agree that we do), we just need a tag named 2.2-dev at zulip/zulip@9812c6d.

andersk · 2020-01-31T23:22:14Z

It should be mentioned that there are already many possible git describe output formats, so you shouldn’t make too many assumptions:

2.1.0-rc1
2.1.0-rc1-112-g35959d43c4
2.1.0
2.1.1-50-gd452ad31e0
2.2-dev-426-gdf6b90db3c (if we make a 2.2-dev tag)

timabbott · 2020-01-31T23:55:45Z

I think the 2.2-dev tag is worth doing. @andersk can you push the tag (and also update the release checklist to mention doing that?)

andersk · 2020-02-01T00:13:07Z

I think the 2.2-dev tag is worth doing. @andersk can you push the tag (and also update the release checklist to mention doing that?)

Done.

rk-for-zulip · 2020-02-03T19:50:13Z

It should be mentioned that there are already many possible git describe output formats, so you shouldn’t make too many assumptions:

2.1.0-rc1

2.1.0-rc1-112-g35959d43c4

2.1.0

2.1.1-50-gd452ad31e0

2.2-dev-426-gdf6b90db3c (if we make a 2.2-dev tag)

Assuming dev < rc0 < rc1 < ... < ø (does it?), we can still use the lexicographic ordering on number[] with all of these if dev maps to [-2] and rcN maps to [-1,N].

I think all we really need, though, is major-minor[-patch]; anything past that is gravy.

rk-for-zulip · 2020-02-06T01:00:09Z

Given the amount of discussion that's ensued on this topic (see also CZO), I should probably note: while we'll want a parsed and partial-ordered server-version at some point, it doesn't have to block this PR. An unparsed version-string will still be useful for #3745.

chrisbobbe · 2020-02-10T19:32:13Z

(Not yet ready for review; I just remembered I haven't addressed Ray's comments on the earlier commits.)

chrisbobbe · 2020-02-10T21:13:27Z

alternately, consider using zulipVersion: string instead. This requires us to write a migration, but Zulip servers have been sending zulip_version since at least 1.5.1 in 2017. (Not sure if that's old enough not to worry about.)

As @gnprice mentioned to me on Friday, such a migration would have to exist outside the pattern of our current migrations because it requires making an API call. I think we instead landed on it being OK to allow zulipVersion to be empty, since most of the places where we'd need it are after the user is logged in and REALM_INIT has been dispatched. I'm fine to make the "empty" state be undefined instead of null, as you've suggested; I agree with your reasoning.

chrisbobbe · 2020-02-10T21:47:45Z

OK, this is ready for another round of review. I've added version parsing code with tests, and addressed Ray's earlier feedback.

Note that the .isAtLeast tests are taking ~5s for me; I believe it's quadratic with the number of versions present in versions. There are 1000+ comparisons being made; I expect there are more versions present than there need to be.

gnprice

Thanks @chrisbobbe for the new features this adds to our test fixtures in exampleData!

Some comments below on just the testing part.

gnprice · 2020-02-10T22:05:05Z

src/__tests__/exampleData.js

+export const selfAccount: Account = makeAccount({
+  email: '[email protected]',


I'd like to keep it the case that selfUser and selfAccount agree with each other, and that that fact happens automatically rather than depending on two different pieces of code happening to express the same choice.

So even though makeAccount only uses one property from the user, I'd like it to continue to take a user: User rather than email: string.

As a nice bonus, this saves test code that calls makeAccount from having to make up an email (like [email protected]); it can just say user: makeUser(). Which, further, ensures there aren't any accidental cases lying around of the pattern you point out in the commit message, where the same example string appears in two places and that's significant to the test.

OK, makes sense! Would you say makeAccount should take these args, then? I checked, and it seems that realm isn't also covered by the User type.

args: { user: User, realm: string, shouldHaveApiKey?: boolean, shouldHaveZulipVersion?: boolean, }

On REALM_ADD, an Account object with an empty string for email is added. I'll need to simulate that somehow, and if I get the email from a User object, it'll always be non-empty; even if I pass an empty name, email ends up being '@example.org'.

gnprice · 2020-02-10T22:29:25Z

src/__tests__/exampleData.js

-export const selfAccount: Account = makeAccount(selfUser);
+export const selfAccount: Account = makeAccount({
+  email: '[email protected]',
+  realm: 'zulip.example.org',


This isn't well-formed data; the realm should be a URL, like https://zulip.example.org.

It'd also be good to have this function make up the realm when the caller doesn't care about giving it a particular value. One reason is because the well-formed values for this are a little verbose 😉, and it'd be annoying to write a lot of them in a test file. Can do it similarly to how makeUser makes up the email address:

have an optional param like realm?: string to override

use randString() for the random part

stick that in a template string like https://${randString()}.example.org

Bump on this particular line 🙂 . I like the implementation of the general mechanism!

gnprice · 2020-02-10T22:32:23Z

src/account/__tests__/accountsReducer-test.js

+    test('(with zulipVersion) if no account with this realm exists, prepend new one, with empty email/apiKey', () => {
+      const prevState = deepFreeze([account1, account2]);
+
+      const zulipVersion = '2.1.0-234-g7c3acf4';


This string appears a few times, in places where I think its value is meant to be boring; it'd be good to pull into a shared value like eg.zulipVersion.

gnprice · 2020-02-10T22:33:18Z

src/session/__tests__/sessionReducer-test.js

    const action = deepFreeze({
      ...eg.action.realm_init,
      data: { ...eg.action.realm_init.data, queue_id: 100 },
+      zulipVersion: '2.1.0-234-g7c3acf4',


This is the same value as in eg.action.realm_init, right? So this test should be able to remain blissfully unmodified with this change 😄

gnprice · 2020-02-10T22:49:59Z

src/utils/__tests__/version-test.js

+
+describe('ZulipVersion.prototype.raw()', () => {
+  const raw = randString();
+  test(`new ZulipVersion(${raw}).raw() === ${raw}`, () => {


Let's keep the test names stable from run to run. I don't think anything will particularly break with our current set of tools, but if we were to e.g. start using something that collected stats on slow tests, we'd want the names to be the same when the same source code is run.

Makes sense. Do you think I shouldn't use the names ${versions[i][k].raw()} is at least ${versions[j][l].raw()} for the .isAtLeast tests? I suppose the names will still be the same run to run, there's just a very large number of them.

Yeah, no, I think that pattern is great :-) If a test fails, that helps make it completely clear what the test was about.

gnprice · 2020-02-10T23:53:13Z

src/__tests__/exampleData.js

+  shouldHaveApiKey?: boolean,
+  shouldHaveZulipVersion?: boolean,
+}): Account => {
+  const { email, realm, shouldHaveApiKey = false, shouldHaveZulipVersion = false } = args;


One thing that helps keep the code in the test cases themselves simple is when they can skip mentioning options for things that are boring for that test case, and only mention something like the API key when the point of the test case is specifically related to whether the API key is present or absent.

It looks like in the current version, this works out very nicely for shouldHaveZulipVersion -- it's actually not mentioned in any call sites except selfAccount.

For shouldHaveApiKey, all but a few of the call sites are passing true. That's a sign that that should be the default -- most of those test cases aren't really thinking about the API key, and it's just a background assumption of a lot of our code that there is an API key in the active account.

rk-for-zulip · 2020-02-11T00:35:21Z

As @gnprice mentioned to me on Friday, such a migration would have to exist outside the pattern of our current migrations because it requires making an API call.

For the record: I was thinking the migration could just use "0.0" or similar. (An honest undefined is probably a better idea all around, though.)

chrisbobbe · 2020-02-12T00:22:07Z

OK, I think I've addressed all of your comments, @gnprice, but I ended up doing some fairly substantial reorganization of the commits, so hopefully I haven't introduced more issues. But I'm hoping at least it's easier to review with this organization. :)

One thing to note in this revision is that I introduced shouldHaveZulipVersion and defaulted it to false, even though you've rightly pointed out that it should default to true. In later commits that need to test with a zulipVersion present, I switched it to default to true. This back and forth is intended to reduce the size of the patch, to help make reviewing easier; I figured it would be more lines of code if I switched a lot of the callers back and forth like this.

gnprice

Thanks for the revisions! The test changes look good; just a few comments below.

(Also a couple of comments on things I happened to notice while skimming through the rest.)

gnprice · 2020-02-12T20:03:35Z

src/__tests__/exampleData.js

+  return deepFreeze({
+    realm,
+    email,
+    apiKey: shouldHaveApiKey ? apiKey : '',


I think shouldHaveApiKey can be completely replaced by saying apiKey: ''. That's nice because it makes it totally transparent at the call site what it means; and also because it makes it impossible to pass sets of arguments that would contradict each other, like shouldHaveApiKey: false while also passing apiKey.

You mean apiKey = '', right? (And keep apiKey?: string as-is but remove shouldHaveApiKey.)

Just a note that this means if the caller wants an API key, they have to provide it themselves, e.g., by using randString() or '123'.

Also, the same pattern is used in a later commit with the zulipVersion; perhaps this should be changed as well.

Ah, I meant perhaps the reverse of that -- I did mean apiKey: '', and by that I meant (and didn't say this clearly) that's what the caller would say.

So:

signature has apiKey?: string

when it's not provided, the default is to make a random one (just like in master)

... OK, and you just asked me about this in person 🙂 and we discussed, so I'll leave this off here.

gnprice · 2020-02-12T20:07:52Z

src/api/registerForEvents.js


 /** See https://zulipchat.com/api/register-queue */
-export default (auth: Auth, params: RegisterForEventsParams) =>
+export default async (auth: Auth, params: RegisterForEventsParams): Promise<InitialData> =>


Yikes, good catch!

When a value is exported (which this is) and doesn't have a type annotation, Flow treats it as outright any. So in fact in master at this function's call site in doInitialFetch, Flow was getting any for the return value, and doing no type-checking on how we used it. Good to fix.

Yep :) I just updated the commit message with the detail about exported values.

gnprice · 2020-02-12T20:10:19Z

src/actionTypes.js

 type RealmInitAction = {|
  type: typeof REALM_INIT,
  data: InitialData,
+  zulipVersion?: string,


Why optional? Is there a case where we won't have this?

Ah. So, while it seems like we should have zulipVersion be optional on Account, it should be required in the Redux actions. (Indeed, it isn't and hasn't been optional in ApiResponseServerSettings.) This'll mean some changes to the tests; working on that now.

gnprice · 2020-02-12T20:14:36Z

src/types.js

  ...Auth,

+  /**
+   * To determine behavior of the mobile app, and for Sentry reports.


"To determine behavior of the mobile app" covers potentially anything at all. 😉

I think our intent is a lot narrower than that: this will affect how we make some API requests. I think in fact we really want to keep the logic that conditions on this isolated to the edge, where we talk to the server, and avoid letting it affect the bulk of the app. This is closely related to the "crunchy shell" pattern.

Haha yes, it looks quite jarring as a description of intent, vs. abilities provided. I was optimizing for accuracy on the latter, but intent makes much more sense given the preference for "why" vs. "what" comments.

gnprice · 2020-02-12T20:48:09Z

src/account/__tests__/accountsReducer-test.js

+    const prevState = deepFreeze([accountWithZulipVersion, accountWithoutZulipVersion]);
+    test('(no zulipVersion) if no account with this realm exists, prepend new one, with empty email/apiKey', () => {


nit: blank line between the test and anything before it in the same block

Same thing a time or two below.

gnprice · 2020-02-12T20:58:50Z

src/utils/__tests__/version-test.js

+          test(`${versions[i][k].raw()} is at least ${versions[j][l].raw()}`, () => {
+            // Compare versions[i][k] to versions[j][l] for all valid i, j, k, l
+            expect(versions[i][k].isAtLeast(versions[j][l])).toBe(i >= j);


This name is a little confusing, because it'll say things like "0.0.1-dev is at least 2.2.0" where it's actually testing that that sentence is not true.

That could be fixed by tweaking the name so it sounds like a question, or starts with "whether", or something in that direction.

Better yet, though, you could make the test name be completely descriptive of the test: pull the i >= j check outside, and use it to decide both the test name and whether the value is expected to be true or false.

Better yet, though, you could make the test name be completely descriptive of the test: pull the i >= j check outside, and use it to decide both the test name and whether the value is expected to be true or false.

I had this in a previous iteration but I must have lost it in a refactor; thanks for pointing this out!

chrisbobbe · 2020-02-13T23:34:38Z

OK, ready for another review!

gnprice

I've just read through the whole branch; this is very close!

A few comments below -- all I think simple to handle, mostly small, one of them substantive.

There are also some style edits I'd like to make to the jsdoc and comments in ZulipVersion. Those may be easiest to go through while looking at the same screen, and also they can just as well come after this PR is merged.

gnprice · 2020-02-25T22:22:36Z

src/__tests__/exampleData.js

-export const selfAccount: Account = makeAccount(selfUser);
+export const selfAccount: Account = makeAccount({
+  email: '[email protected]',
+  realm: 'zulip.example.org',


Bump on this particular line 🙂 . I like the implementation of the general mechanism!

src/api/registerForEvents.js

gnprice · 2020-02-25T22:29:35Z

src/account/__tests__/accountsReducer-test.js

+    test('(with zulipVersion) if no account with this realm exists, prepend new one, with empty email/apiKey', () => {
+      const prevState = deepFreeze([account1, account2]);
+
+      const zulipVersion = '2.1.0-234-g7c3acf4';


gnprice · 2020-02-25T22:39:00Z

src/account/accountsReducer.js


+const realmInit = (state, action) => [
+  {
+    ...state[0], // Assume state[0] is the active account


Yeah, this is how this data structure represents the "active account".

That design choice is encoded all over this file, so no need to have a comment in this line -- though we can probably do better at documenting it in general.

(Or of encapsulating it so less code cares, or design a better structure in the first place.)

I may go try to write a bit more on this structure. At present the main documentation around it is in accountsSelectors.js, particularly at tryGetAuth (though other selectors there point to it in see-alsos.)

Added some jsdoc in 8bf4893, plus filled in some glossary entries in the two commits before that.

Added some jsdoc in 8bf4893, plus filled in some glossary entries in the two commits before that.

Great, thanks!

(Or of encapsulating it so less code cares, or design a better structure in the first place.)

You've mentioned this before; I made a note of it at #2380 (comment). Perhaps the redesign is not a top priority, but that issue might have some helpful context for it.

src/utils/__tests__/version-test.js

src/utils/zulipVersion.js

chrisbobbe · 2020-02-26T22:51:14Z

OK, I believe I've addressed all of your comments now. I've also used the quantifier {4,40} instead of + to match the commit ID for commitsMatch. I considered adding a $ at the end, to discard the commit info if it's followed by garbage, but I decided against it; the garbage itself can be ignored, while preserving the commit info. This assumes any garbage that's there doesn't suggest a false positive for the commit info; I suppose {4,40} might make this a bit less likely.

gnprice

Thanks @chrisbobbe for the revisions!

One tiny comment below, and there's this from last round (copying here so I don't forget about it):

There are also some style edits I'd like to make to the jsdoc and comments in ZulipVersion. Those may be easiest to go through while looking at the same screen, and also they can just as well come after this PR is merged.

I also want to try tightening some of these summary lines 🙂 -- they're running long.

I would go and do that now and merge (and then we'd follow up on the jsdoc and comments), except it's getting late in the day. Let's merge this together tomorrow.

gnprice · 2020-02-28T00:02:36Z

src/__tests__/exampleData.js

-    email: user.email,
-    apiKey: randString() + randString(),
-    ackedPushToken: null,
+export const realm = 'https://realm.example.org';


I like the existing value https://zulip.example.org a bit better, because it more resembles typical real data: people usually choose zulip.foo or chat.foo for their Zulip server's DNS name, for some foo.

(The various one-off values in the tests that care to specify this property, like https://realm.one.org and https://new.realm.org, are fine -- they're more locally scoped so they won't show up in the data in faraway tests, and they suggest phrases like "realm one" or "new realm" that are meaningful in the given test's context.)

chrisbobbe · 2020-02-28T03:24:39Z

and there's this from last round (copying here so I don't forget about it)

Ohhhh, I saw your comment after that one, saying you’d gone ahead and added a jsdoc, in #3839 (comment), and my eyes skimmed right past the fact that you were talking about two completely different jsdocs. Oops! We’ll go over this tomorrow. 🙂

Change makeAccount to use a params object, since more params will be added in a commit later in this series. Declare variables for the Account objects to avoid the pattern of setting, e.g., prevState[0].email to the literal '[email protected]' and expectedState[0].email to the literal '[email protected]', which forces the test writer to visually confirm the strings are identical.

At this function's call site, in doInitialFetch, Flow was getting `any` for the return value, and doing no type-checking on how we used it, since an exported value without an annotation is treated as `any`. We do have a type for what we expect the /register request to return (i.e., InitialData in 'src/api/initialDataTypes.js'), but we just weren't using it. InitialData doesn't follow the same naming convention as, e.g., ApiResponseServerSettings in getServerSettings.js, but InitialData is OK because it's more concise.

…INIT. Await completion of /server_settings along with /register (already being awaited) in a Promise.all before setting session.loading to false with initialFetchComplete. We considered making a new async action creator for each of these two requests and giving them both responsibility for setting session.loading to false with initialFetchComplete. But this is undesirable because we need to wait for both of them to complete, not just one. We had a choice of awaiting each of these two requests sequentially, but this would result in a longer loading period than awaiting a Promise.all, which will only take as long as the longer of the two requests. The longer one is almost certainly /register in most cases, so this change shouldn't actually increase the loading time at all.

Update types for the REALM_ADD action and its creator. zulip_version is already contained in the server_settings response, so, just access it and pass it to the realmAdd action creator.

Extend eg.makeAccount to give control to the tester over the zulipVersion in the Account object. Default shouldHaveZulipVersion to false; with the child, a strategy to minimize diffs for review.

Spread state[index] and clobber desired properties on that, instead of only including the desired properties. For LOGOUT, that's apiKey and ackedPushToken. For LOGIN_SUCCESS, that's email, apiKey, and ackedPushToken. ACCOUNT_SWITCH and the others were not doing this. Also make shouldHaveZulipVersion in makeAccount default to true, adjust two callers accordingly (with the parent, a strategy to minimize diffs for review).

Now that it is possible, following recent commits, store the zulipVersion in Redux on REALM_ADD.

Now that it is possible, following recent commits, store the zulipVersion in Redux on REALM_INIT.

Allow randString to be used in any test, such as the Zulip version tests in the next commit.

E.g., a mobile feature could be enabled if the server is running version 2.1.1 or later, and disabled otherwise. ZulipVersion.prototype.isAtLeast(otherZulipVersion) should be used for this. Also add logic for reporting the major, minor, and patch versions in Sentry to inform event aggregation. ZulipVersion.prototype.loggingArray() should be used for this. .raw() should also be used to send the raw version string to Sentry.

…ribe.

gnprice · 2020-03-03T22:37:26Z

And merged. (After the edits we made together just now in ZulipVersion.js, mainly to jsdoc and comments.) Thanks again!

chrisbobbe force-pushed the record-server-version branch from 6db3c4f to e3a4be4 Compare January 27, 2020 21:42

chrisbobbe changed the title ~~WIP for storing server version in Redux~~ Store server version in Redux (WIP for #3745) Jan 27, 2020

chrisbobbe requested a review from gnprice January 27, 2020 21:47

chrisbobbe mentioned this pull request Jan 29, 2020

PMs from new sources not immediately displayed in PMs list #3654

Closed

rk-for-zulip reviewed Jan 31, 2020

View reviewed changes

chrisbobbe added the a-redux label Feb 1, 2020

gnprice added the P1 high-priority label Feb 3, 2020

chrisbobbe force-pushed the record-server-version branch from e3a4be4 to 6c02b27 Compare February 10, 2020 19:28

chrisbobbe force-pushed the record-server-version branch from 6c02b27 to bdb51db Compare February 10, 2020 21:45

gnprice reviewed Feb 10, 2020

View reviewed changes

chrisbobbe force-pushed the record-server-version branch from bdb51db to a9c298e Compare February 12, 2020 00:01

chrisbobbe mentioned this pull request Feb 12, 2020

Clarify startEventPolling, fix a race condition #3830

Closed

gnprice reviewed Feb 12, 2020

View reviewed changes

chrisbobbe force-pushed the record-server-version branch from a9c298e to 354a608 Compare February 13, 2020 23:02

chrisbobbe mentioned this pull request Feb 19, 2020

Support downgrading to a long-lived event queue #3916

Open

gnprice reviewed Feb 25, 2020

View reviewed changes

chrisbobbe force-pushed the record-server-version branch 2 times, most recently from 6f7c598 to dce964c Compare February 26, 2020 22:44

gnprice reviewed Feb 28, 2020

View reviewed changes

Chris Bobbe added 4 commits March 3, 2020 13:38

RealmScreen: Include zulip_version from /server_settings in REALM_ADD.

22cc6c4

Update types for the REALM_ADD action and its creator. zulip_version is already contained in the server_settings response, so, just access it and pass it to the realmAdd action creator.

chrisbobbe force-pushed the record-server-version branch from dce964c to ee201d8 Compare March 3, 2020 22:28

Chris Bobbe added 7 commits March 3, 2020 14:35

Account type: Add optional property zulipVersion.

3f69100

Extend eg.makeAccount to give control to the tester over the zulipVersion in the Account object. Default shouldHaveZulipVersion to false; with the child, a strategy to minimize diffs for review.

accounts reducer: Make REALM_ADD store zulipVersion in Redux.

130b9c3

Now that it is possible, following recent commits, store the zulipVersion in Redux on REALM_ADD.

accounts reducer: Make REALM_INIT store zulipVersion in Redux.

59d503e

Now that it is possible, following recent commits, store the zulipVersion in Redux on REALM_INIT.

exampleData [nfc]: Export randString.

3d0812f

Allow randString to be used in any test, such as the Zulip version tests in the next commit.

tests [nfc]: Blank line before a test unless the first code in a desc…

f65475b

…ribe.

gnprice force-pushed the record-server-version branch from ee201d8 to f65475b Compare March 3, 2020 22:36

gnprice merged commit f65475b into zulip:master Mar 3, 2020

This was referenced Mar 3, 2020

Store server's version number, include in Sentry events #3745

Closed

Typing notifications are mistyped, resulting in rejection (for servers < 2.0) #3732

Closed

chrisbobbe deleted the record-server-version branch March 6, 2020 22:18

chrisbobbe mentioned this pull request Mar 10, 2020

Make hard-coded jitsi_server_url and friends accessible by /register zulip/zulip#14166

Closed

This was referenced Apr 17, 2020

Add a monotonic integer "feature level" zulip/zulip#14618

Closed

Store server feature level in Redux #4049

Closed

		const account1 = deepFreeze(eg.makeAccount('[email protected]', 'realm1', false));
		const account2 = deepFreeze(eg.makeAccount('[email protected]', 'realm2', false));

		export const selfAccount: Account = makeAccount({
		email: '[email protected]',

		const prevState = deepFreeze([accountWithZulipVersion, accountWithoutZulipVersion]);
		test('(no zulipVersion) if no account with this realm exists, prepend new one, with empty email/apiKey', () => {

Uh oh!

Store server version in Redux (WIP for #3745) #3839

Store server version in Redux (WIP for #3745) #3839

Uh oh!

Conversation

chrisbobbe commented Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Jan 27, 2020

Uh oh!

rk-for-zulip left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jan 31, 2020

Uh oh!

timabbott commented Jan 31, 2020

Uh oh!

andersk commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andersk commented Jan 31, 2020

Uh oh!

timabbott commented Jan 31, 2020

Uh oh!

andersk commented Feb 1, 2020

Uh oh!

rk-for-zulip commented Feb 3, 2020

Uh oh!

rk-for-zulip commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Feb 10, 2020

Uh oh!

chrisbobbe commented Feb 10, 2020

Uh oh!

chrisbobbe commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rk-for-zulip commented Feb 11, 2020

chrisbobbe commented Jan 24, 2020 •

edited

Loading

chrisbobbe Jan 31, 2020 •

edited

Loading

andersk commented Jan 31, 2020 •

edited

Loading

rk-for-zulip commented Feb 6, 2020 •

edited

Loading

chrisbobbe commented Feb 10, 2020 •

edited

Loading

chrisbobbe Feb 13, 2020 •

edited

Loading