Fetch and indicate server version and feature level

[preetmishra]

This introduces AboutView to show server version and feature level within the app and makes other related amendments.

Fixes #616.

[neiljp]

@preetmishra I think this could be a useful in-app feature 👍

I think it would be useful to split changes related to adding a simple UI, from ones which acquire/access extra information and extend that UI.

If you have other thoughts on what to include in an 'about' popup then we could include it here too.

I know we need to pick a key to use for this, I'm just aware that we need to avoid webapp keys (for compatibility) and also adding extra keys which we may later find useful to apply to other UI elements.

[sumanthvrao]

One point I noticed was that focus by default was on the last row of the popup; However the up/down arrows didn't work. This makes me wonder if we don't to remove marking anything as 'focus' in this popup, since we may not show enough data for it to be scrollable at any point.

[preetmishra]

@sumanthvrao The keys don't work because there's nothing to scroll in the about popup; they will work fine if there's more data. Were you referring to popup_contrast (markup) when you said:

One point I noticed was that focus by default was on the last row of the popup

[preetmishra]

@sumanthvrao @neiljp Thank you for the reviews and the suggestions. I have incorporated the suggestions and split the commits into two parts:

• the first introduces an about menu in the app, using AboutView, to show ZT_VERSION
• the second adds server version and feature level in the about menu.

I went with meta ? key for now; suggestions are welcomed.

[neiljp]

@preetmishra This is looking good, though possibly the split could be tidier; that said, if we did merge the other popup PR first then this would be simpler as the common code would be present already.

meta ? seems like a reasonable key that isn't going to be needed for other uses, and we could always move it around. My minor concerns are:

• meta is not clearly 'alt' (which seems to be the case on most systems?), and also applies to other keys in the help menu
• meta ? is really meta shift / (at least here), which is not to say that we should have it that way as it may well depend on keyboard layout, but it's not necessarily clear that two modifiers are needed

[preetmishra]

@neiljp Thanks for the review and the in-line suggestions. I have split the commits to tidy up the PR and incorporated the suggestion that you left.

I have added a refactor commit before the commit that fetches zulip version and feature level. It was necessary to support the zulip_version fixture in initial_data (a detailed explanation is in the commit description).

The zulip_version fixture allows testing how model.py behaves with different zulip versions.

I have left the trigger key to be Meta + ? as per our discussion at #zulip-terminal > Hotkey for about menu.

[neiljp]

@preetmishra I merged the first two commits as one squashed commit, so we now have an about menu 🎉

For the others:

• is the test refactoring necessary for the fixture parametrization? If so, that seems odd and potentially worrisome?
• While I like the idea of parametrizing across server versions, just adding one extra version adds ~300 (?) tests - and more feature levels will add maybe that many per level? I'm wondering if we can explore other approaches as that seems like this could be a problem. Also, should we primarily work with feature_level rather than version?

Other than thinking how to test this, this is pretty straightforward to merge! We could just merge the test cases and iterate, but those ~300 extra tests concern me.

[preetmishra]

@neiljp Thanks for the review and the merge. 🎉

To avoid adding extra tests, I have hard-coded the value in initial_data (which means that the refactor commit won't be necessary anymore). In addition to that, I have added a new fixture, zulip_feature_level, which would make it easier to test against different feature levels. I believe it would be reasonable to only use this fixture for implementations that vary according to the feature level.

Moreover, I have moved the server version and the feature level under 'Server' category.

Furthermore, since the refactor commit isn't necessary anymore, I have moved it to the end. We could shelve it if you'd like to address it after reactions test refactor work.

[neiljp]

@preetmishra This is looking good. I think if we set up the list of supported versions then testing the model with just the minimum (tuple) for now would be fine.

[preetmishra]

@neiljp Thanks for the review and the comments on CZO. I have updated the PR with SUPPORTED_SERVER_VERSIONS and MINIMUM_SUPPORTED_SERVER_VERSION. Additionally, I have amended the fixture to spit out version tuples and the code (with tests) in AboutView.

[preetmishra]

Updated with the comment about 2.2 and fixed commit order (GitHub).

[neiljp]

@preetmishra This looks almost good to go, and would have been more straightforward except for our discussion of a way to introduce server versions/feature-levels and changes which could accompany that.

@kaustubh-nair Do you have any thoughts on the supported-version structures? We can iterate, but obviously if they look insufficient in the first instance for what you might have had in mind, it'd be good to hear what you think.

[preetmishra]

@neiljp Thanks for the review. I have made the amendments accordingly.

[kaustubh-nair]

This looks good for the use case that I need it for 🎉

[preetmishra]

I have updated the references to 2.2 with 3.0 as per the recent discussion in #general > Next release 3.0.

[neiljp]

@preetmishra Thanks for working on this! I just merged this manually with a minor change to a commit title and moving the README change to a more prominent position - thanks for persevering, since I know this was more than we originally intended for this PR.

Anyhow, merged as last 3 commits ending at d9dc81c 🎉