Repository: zulip/zulip-terminal
Pull request #1321 (2023-03-11 05:29:00)

Improve URL error-handling prior to zuliprc generation
<!-- Please see https://github.com/zulip/zulip-terminal#contributor-guidelines ! -->

**What does this PR do?**  <!-- Overall description goes here -->

This follows up on the bugfix of #1320, now introducing and applying types for the server-settings response.

Also following from #1320, this aims to give the user more accurate feedback and exit cleanly (cf the current confusing message(s) or tracebacks) if the server URL is not recognized as corresponding to a valid Zulip Organization.

Some of this is a little WIP, since the documentation on the server-settings endpoint is a little confusing, but this is close to mergeable.

This aims to work further towards separating the text input and the logic, as well as improving the testing.

**update**
- This is now tidied, and resolved an issue experienced in [**#zulip-terminal>Crash on entering bad server URL**](https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/Crash.20on.20entering.20bad.20server.20URL)
- This now also adds some documentation for api_types.py
- The last commit as referenced by alexmv was dropped for now, in order to get this merged

<!-- If fixing a filed bug or new feature, add 'Fixes #<issue>' or 'Partial fix for #<issue>' -->

<!-- Add a link to a discussion on chat.zulip.org, if relevant -->

**Tested?** <!-- Fine to leave some of these unchecked if this is a draft/work-in-progress -->
- [x] Manually
- [x] Existing tests (adapted, if necessary)
- [x] New tests added (for any new behavior)
- [x] Passed linting & tests (each commit)
<!-- Code must pass CI (GitHub Actions) before merging - look for the green tick! -->


zulipbot: Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out!

<!-- areaLabelAddition -->


Patch:
diff --git a/zulipterminal/api_types.py b/zulipterminal/api_types.py
index a772c57..4fdc812 100644
--- a/zulipterminal/api_types.py
+++ b/zulipterminal/api_types.py
@@ -22,6 +22,10 @@ RESOLVED_TOPIC_PREFIX = "✔ "
 TYPING_STARTED_WAIT_PERIOD = 10
 TYPING_STOPPED_WAIT_PERIOD = 5
 
+###############################################################################
+# Parameter to pass in request to:
+#   https://zulip.com/api/send-message
+
 
 class PrivateComposition(TypedDict):
     type: Literal["private"]
@@ -36,9 +40,12 @@ class StreamComposition(TypedDict):
     subject: str  # TODO: Migrate to using topic
 
 
-# https://zulip.com/api/send-message
 Composition = Union[PrivateComposition, StreamComposition]
 
+###############################################################################
+# Parameter to pass in request to:
+#   https://zulip.com/api/update-message
+
 
 class PrivateMessageUpdateRequest(TypedDict):
     message_id: int
@@ -64,9 +71,15 @@ class StreamMessageUpdateRequest(TypedDict):
     # stream_id: int
 
 
-# https://zulip.com/api/update-message
 MessageUpdateRequest = Union[PrivateMessageUpdateRequest, StreamMessageUpdateRequest]
 
+###############################################################################
+# In "messages" response from:
+#   https://zulip.com/api/get-messages
+# In "message" response from:
+#   https://zulip.com/api/get-events#message
+#   https://zulip.com/api/get-message  (unused)
+
 
 class Message(TypedDict, total=False):
     id: int
@@ -102,7 +115,14 @@ class Message(TypedDict, total=False):
     # sender_short_name: str
 
 
-# Elements and types taken from https://zulip.com/api/get-events
+###############################################################################
+# In "subscriptions" response from:
+#   https://zulip.com/api/register-queue
+# Also directly from:
+#   https://zulip.com/api/get-events#subscription-add
+#   https://zulip.com/api/get-subscriptions (unused)
+
+
 class Subscription(TypedDict):
     stream_id: int
     name: str
@@ -136,6 +156,17 @@ class Subscription(TypedDict):
     # in_home_view: bool  # Replaced by is_muted in Zulip 2.1; still present in updates
 
 
+###############################################################################
+# In "realm_user" response from:
+#   https://zulip.com/api/register-queue
+# Also directly from:
+#   https://zulip.com/api/get-events#realm_user-add
+#   https://zulip.com/api/get-users     (unused)
+#   https://zulip.com/api/get-own-user  (unused)
+#   https://zulip.com/api/get-user      (unused)
+# NOTE: Responses between versions & endpoints vary
+
+
 class RealmUser(TypedDict):
     user_id: int
     full_name: str
@@ -173,6 +204,12 @@ class RealmUser(TypedDict):
     # max_message_id: int  # NOTE: DEPRECATED & only for /users/me
 
 
+###############################################################################
+# Events possible in "events" from:
+#   https://zulip.com/api/get-events
+# (also helper data structures not used elsewhere)
+
+
 class MessageEvent(TypedDict):
     type: Literal["message"]
     message: Message
@@ -320,3 +357,59 @@ Event = Union[
     UpdateGlobalNotificationsEvent,
     RealmUserEvent,
 ]
+
+###############################################################################
+# In response from:
+#   https://zulip.com/api/get-server-settings
+
+AuthenticationMethod = Literal[
+    "password",
+    "dev",
+    "email",
+    "ldap",
+    "remoteuser",
+    "github",
+    "azuread",
+    "gitlab",  # New in Zulip 3.0, ZFL 1
+    "apple",
+    "google",
+    "saml",
+    "openid_connect",
+]
+
+
+class ExternalAuthenticationMethod(TypedDict):
+    name: str
+    display_name: str
+    display_icon: Optional[str]
+    login_url: str
+    signup_url: str
+
+
+# As of ZFL 121
+class ServerSettings(TypedDict):
+    # authentication_methods is deprecated in favor of external_authentication_methods
+    authentication_methods: Dict[AuthenticationMethod, bool]
+    # Added in Zulip 2.1.0
+    external_authentication_methods: List[ExternalAuthenticationMethod]
+
+    # TODO Refactor ZFL to default to zero
+    zulip_feature_level: NotRequired[int]  # New in Zulip 3.0, ZFL 1
+    zulip_version: str
+    zulip_merge_base: NotRequired[str]  # New in Zulip 5.0, ZFL 88
+
+    push_notifications_enabled: bool
+    is_incompatible: bool
+    email_auth_enabled: bool
+    require_email_format_usernames: bool
+
+    # This appears to be present for all Zulip servers, even for no organization,
+    # which makes it useful to determine a 'preferred' URL for the server/organization
+    realm_uri: str
+
+    # These may only be present if it's an organization, not just a Zulip server
+    # Re realm_name discussion, See #api document > /server_settings: `realm_name`, etc.
+    realm_name: NotRequired[str]  # Absence indicates root Zulip server but no realm
+    realm_icon: str
+    realm_description: str
+    realm_web_public_access_enabled: NotRequired[bool]  # New in Zulip 5.0, ZFL 116
diff --git a/zulipterminal/cli/run.py b/zulipterminal/cli/run.py
index 225f37d..e6f5f22 100755
--- a/zulipterminal/cli/run.py
+++ b/zulipterminal/cli/run.py
@@ -11,11 +11,12 @@ import sys
 import traceback
 from enum import Enum
 from os import path, remove
-from typing import Any, Dict, List, NamedTuple, Optional, Tuple
+from typing import Dict, List, NamedTuple, Optional, Tuple
 
 import requests
 from urwid import display_common, set_encoding
 
+from zulipterminal.api_types import ServerSettings
 from zulipterminal.config.themes import (
     InvalidThemeColorCode,
     aliased_themes,
@@ -206,31 +207,44 @@ def styled_input(label: str) -> str:
     return input(in_color("blue", label))
 
 
-def get_login_id(server_properties: Dict[str, Any]) -> str:
+def get_login_label(server_properties: ServerSettings) -> str:
     require_email_format_usernames = server_properties["require_email_format_usernames"]
     email_auth_enabled = server_properties["email_auth_enabled"]
 
     if not require_email_format_usernames and email_auth_enabled:
-        label = "Email or Username: "
+        return "Email or Username: "
     elif not require_email_format_usernames:
-        label = "Username: "
+        return "Username: "
     else:
         # TODO: Validate Email address
-        label = "Email: "
+        return "Email: "
 
-    return styled_input(label)
+
+class NotAZulipOrganizationError(Exception):
+    pass
+
+
+def get_server_settings(realm_url: str) -> ServerSettings:
+    response = requests.get(url=f"{realm_url}/api/v1/server_settings")
+    if response.status_code != requests.codes.OK:
+        raise NotAZulipOrganizationError(realm_url)
+    return response.json()
 
 
 def get_api_key(realm_url: str) -> Optional[Tuple[str, str, str]]:
     from getpass import getpass
 
-    server_properties = requests.get(url=f"{realm_url}/api/v1/server_settings").json()
+    try:
+        server_properties = get_server_settings(realm_url)
+    except NotAZulipOrganizationError:
+        exit_with_error(f"No Zulip Organization found at {realm_url}.")
 
     # Assuming we connect to and get data from the server, use the realm_url it suggests
     # This avoids cases where there are redirects between http and https, for example
     preferred_realm_url = server_properties["realm_uri"]
 
-    login_id = get_login_id(server_properties)
+    login_id_label = get_login_label(server_properties)
+    login_id = styled_input(login_id_label)
     password = getpass(in_color("blue", "Password: "))
 
     response = requests.post(


Test patch:
diff --git a/tests/cli/test_run.py b/tests/cli/test_run.py
index 9fafdab..9d4a721 100644
--- a/tests/cli/test_run.py
+++ b/tests/cli/test_run.py
@@ -5,13 +5,17 @@ from pathlib import Path
 from typing import Callable, Dict, Generator, List, Optional, Tuple
 
 import pytest
+import requests
 from pytest import CaptureFixture
 from pytest_mock import MockerFixture
 
+from zulipterminal.api_types import ServerSettings
 from zulipterminal.cli.run import (
+    NotAZulipOrganizationError,
     _write_zuliprc,
     exit_with_error,
-    get_login_id,
+    get_login_label,
+    get_server_settings,
     in_color,
     main,
     parse_args,
@@ -54,15 +58,64 @@ def test_in_color(color: str, code: str, text: str = "some text") -> None:
         (dict(require_email_format_usernames=True, email_auth_enabled=False), "Email"),
     ],
 )
-def test_get_login_id(mocker: MockerFixture, json: Dict[str, bool], label: str) -> None:
-    mocked_styled_input = mocker.patch(
-        MODULE + ".styled_input", return_value="input return value"
+def test_get_login_label(
+    mocker: MockerFixture,
+    json: ServerSettings,  # NOTE: pytest does not ensure dict above is complete
+    label: str,
+) -> None:
+    result = get_login_label(json)
+    assert result == label + ": "
+
+
+@pytest.fixture
+def server_settings_minimal() -> ServerSettings:
+    return ServerSettings(
+        authentication_methods={},
+        external_authentication_methods=[],
+        zulip_feature_level=0,  # New in Zulip 3.0, ZFL 1
+        zulip_version="2.1.0",
+        zulip_merge_base="",  # New in Zulip 5.0, ZFL 88
+        push_notifications_enabled=True,
+        is_incompatible=False,
+        require_email_format_usernames=False,
+        email_auth_enabled=True,
+        realm_uri="chat.zulip.zulip",  # Present if a Zulip server; preferred URL
+        realm_name="A Zulip Server",  # Present if Organization is active at URL
+        realm_icon="...",
+        realm_description="Very exciting server",
+        realm_web_public_access_enabled=True,  # New in Zulip 5.0, ZFL 116
+    )
+
+
+def test_get_server_settings(
+    mocker: MockerFixture,
+    server_settings_minimal: ServerSettings,
+    realm_url: str = "https://chat.zulip.org",
+) -> None:
+    response = mocker.Mock(
+        status_code=requests.codes.OK, json=lambda: server_settings_minimal
+    )
+    mocked_get = mocker.patch("requests.get", return_value=response)
+
+    result = get_server_settings(realm_url)
+
+    assert mocked_get.called_once_with(url=realm_url + "/api/v1/server_settings")
+    assert result == server_settings_minimal
+
+
+def test_get_server_settings__not_a_zulip_organization(
+    mocker: MockerFixture, realm_url: str = "https://google.com"
+) -> None:
+    response = mocker.Mock(
+        status_code=requests.codes.bad_request  # FIXME: Test others?
     )
+    mocked_get = mocker.patch("requests.get", return_value=response)
 
-    result = get_login_id(json)
+    with pytest.raises(NotAZulipOrganizationError) as exc:
+        get_server_settings(realm_url)
 
-    assert result == "input return value"
-    mocked_styled_input.assert_called_with(label + ": ")
+    assert mocked_get.called_once_with(url=realm_url + "/api/v1/server_settings")
+    assert str(exc.value) == realm_url
 
 
 @pytest.mark.parametrize("options", ["-h", "--help"])