Repository: zulip/zulip-terminal
Pull request #1417 (2023-07-13 00:21:44)

Model: Improve users data handling & typing
<!-- See README for documentation, or ask in #zulip-terminal if unclear -->
### What does this PR do, and why?

This defines user data more centrally and clearly in the model.

Next step is to encapsulate this data properly and improve the naming.

### External discussion & connections
<!-- [x] all that apply, specifying topic and adding numbers after # for issues/PRs -->
- [ ] Discussed in **#zulip-terminal** in `topic`
- [ ] Fully fixes #
- [ ] Partially fixes issue #
- [ ] Builds upon previous unmerged work in PR #
- [ ] Is a follow-up to work in PR #
- [ ] Requires merge of PR #
- [ ] Merge will enable work on #

### How did you test this?
<!-- [x] all that apply -->
- [ ] Manually - Behavioral changes
- [ ] Manually - Visual changes
- [x] Adapting existing automated tests
- [ ] Adding automated tests for new behavior (or missing tests)
- [x] Existing automated tests should already cover this (*only a refactor of tested code*)

### Self-review checklist for each commit
- [x] It is a [minimal coherent idea](https://github.com/zulip/zulip-terminal#structuring-commits---speeding-up-reviews-merging--development)
- [x] It has a commit summary following the [documented style](https://github.com/zulip/zulip-terminal#structuring-commits---speeding-up-reviews-merging--development) (title & body)
- [x] It has a commit summary describing the  motivation and reasoning for the change
- [x] It individually passes linting and tests
- [ ] It contains test additions for any new behavior
- [x] It flows clearly from a previous branch commit, and/or prepares for the next commit


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 -->

zulipbot: Heads up @neiljp, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's [recent commits](https://github.com/zulip/zulip-terminal/commits/main) to see where the conflicts occur. Please rebase your feature branch against the `upstream/main` branch and [resolve](https://zulip.readthedocs.io/en/latest/git/troubleshooting.html#recover-from-a-git-rebase-failure) your pull request's merge conflicts accordingly.

<!-- mergeConflictWarning -->


Patch:
diff --git a/zulipterminal/config/ui_mappings.py b/zulipterminal/config/ui_mappings.py
index 8f94c5f..8f8392c 100644
--- a/zulipterminal/config/ui_mappings.py
+++ b/zulipterminal/config/ui_mappings.py
@@ -4,8 +4,6 @@ Relationships between state/API data and presentation in the UI
 
 from typing import Dict
 
-from typing_extensions import Literal
-
 from zulipterminal.api_types import EditPropagateMode
 from zulipterminal.config.symbols import (
     BOT_MARKER,
@@ -17,6 +15,7 @@ from zulipterminal.config.symbols import (
     STREAM_MARKER_PUBLIC,
     STREAM_MARKER_WEB_PUBLIC,
 )
+from zulipterminal.helper import StreamAccessType, UserStatus
 
 
 EDIT_MODE_CAPTIONS: Dict[EditPropagateMode, str] = {
@@ -25,9 +24,6 @@ EDIT_MODE_CAPTIONS: Dict[EditPropagateMode, str] = {
     "change_all": "Also change previous and following messages to this topic",
 }
 
-
-UserStatus = Literal["active", "idle", "offline", "inactive", "bot"]
-
 # Mapping that binds user activity status to corresponding markers.
 # NOTE: Ordering of keys affects display order
 STATE_ICON: Dict[UserStatus, str] = {
@@ -39,9 +35,7 @@ STATE_ICON: Dict[UserStatus, str] = {
 }
 
 
-StreamAccessType = Literal["public", "private", "web-public"]
-
-STREAM_ACCESS_TYPE = {
+STREAM_ACCESS_TYPE: Dict[StreamAccessType, Dict[str, str]] = {
     "public": {"description": "Public", "icon": STREAM_MARKER_PUBLIC},
     "private": {"description": "Private", "icon": STREAM_MARKER_PRIVATE},
     "web-public": {"description": "Web public", "icon": STREAM_MARKER_WEB_PUBLIC},
diff --git a/zulipterminal/helper.py b/zulipterminal/helper.py
index 318c55b..ce77c52 100644
--- a/zulipterminal/helper.py
+++ b/zulipterminal/helper.py
@@ -30,7 +30,7 @@ from typing import (
 from urllib.parse import unquote
 
 import requests
-from typing_extensions import ParamSpec, TypedDict
+from typing_extensions import Literal, ParamSpec, TypedDict
 
 from zulipterminal.api_types import Composition, EmojiType, Message
 from zulipterminal.config.keys import primary_key_for_command
@@ -39,7 +39,6 @@ from zulipterminal.config.regexes import (
     REGEX_COLOR_6_DIGIT,
     REGEX_QUOTED_FENCE_LENGTH,
 )
-from zulipterminal.config.ui_mappings import StreamAccessType
 from zulipterminal.platform_code import (
     PLATFORM,
     normalized_file_path,
@@ -47,6 +46,9 @@ from zulipterminal.platform_code import (
 )
 
 
+StreamAccessType = Literal["public", "private", "web-public"]
+
+
 class StreamData(TypedDict):
     name: str
     id: int
@@ -86,6 +88,16 @@ class TidiedUserInfo(TypedDict):
     bot_owner_name: str
 
 
+UserStatus = Literal["active", "idle", "offline", "inactive", "bot"]
+
+
+class MinimalUserData(TypedDict):
+    full_name: str
+    email: str
+    user_id: int
+    status: UserStatus
+
+
 class Index(TypedDict):
     pointer: Dict[str, Optional[int]]  # narrow_str, message_id (or no data)
     # Various sets of downloaded message ids (all, starred, ...)
diff --git a/zulipterminal/model.py b/zulipterminal/model.py
index 704c3b2..82d809a 100644
--- a/zulipterminal/model.py
+++ b/zulipterminal/model.py
@@ -55,17 +55,15 @@ from zulipterminal.api_types import (
 )
 from zulipterminal.config.keys import primary_key_for_command
 from zulipterminal.config.symbols import STREAM_TOPIC_SEPARATOR
-from zulipterminal.config.ui_mappings import (
-    EDIT_TOPIC_POLICY,
-    ROLE_BY_ID,
-    STATE_ICON,
-    StreamAccessType,
-)
+from zulipterminal.config.ui_mappings import EDIT_TOPIC_POLICY, ROLE_BY_ID, STATE_ICON
 from zulipterminal.helper import (
     CustomProfileData,
+    MinimalUserData,
     NamedEmojiData,
+    StreamAccessType,
     StreamData,
     TidiedUserInfo,
+    UserStatus,
     asynch,
     canonicalize_color,
     classify_unread_counts,
@@ -168,13 +166,15 @@ class Model:
         # lose any updates while messages are being fetched.
         self._fetch_initial_data()
 
-        self._all_users_by_id: Dict[int, RealmUser] = {}
-        self._cross_realm_bots_by_id: Dict[int, RealmUser] = {}
-
         self.server_version = self.initial_data["zulip_version"]
         self.server_feature_level = self.initial_data.get("zulip_feature_level")
 
-        self.users = self.get_all_users()
+        self.user_dict: Dict[str, MinimalUserData] = {}
+        self.user_id_email_dict: Dict[int, str] = {}
+        self._all_users_by_id: Dict[int, RealmUser] = {}
+        self._cross_realm_bots_by_id: Dict[int, RealmUser] = {}
+        self.users: List[MinimalUserData] = []
+        self._update_users_data_from_initial_data()
 
         self.stream_dict: Dict[int, Any] = {}
         self.muted_streams: Set[int] = set()
@@ -436,7 +436,7 @@ class Model:
             response = self._notify_server_of_presence()
             if response["result"] == "success":
                 self.initial_data["presences"] = response["presences"]
-                self.users = self.get_all_users()
+                self._update_users_data_from_initial_data()
                 if hasattr(self.controller, "view"):
                     view = self.controller.view
                     view.users_view.update_user_list(user_list=self.users)
@@ -1065,7 +1065,7 @@ class Model:
             last_active="",
         )
 
-        bot_owner: Optional[Union[RealmUser, Dict[str, Any]]] = None
+        bot_owner: Optional[Union[RealmUser, MinimalUserData]] = None
 
         if api_user_data.get("bot_owner_id", None):
             bot_owner = self._all_users_by_id.get(api_user_data["bot_owner_id"], None)
@@ -1087,18 +1087,18 @@ class Model:
 
         return user_info
 
-    def get_all_users(self) -> List[Dict[str, Any]]:
+    def _update_users_data_from_initial_data(self) -> None:
         # Dict which stores the active/idle status of users (by email)
         presences = self.initial_data["presences"]
 
         # Construct a dict of each user in the realm to look up by email
         # and a user-id to email mapping
-        self.user_dict: Dict[str, Dict[str, Any]] = dict()
-        self.user_id_email_dict: Dict[int, str] = dict()
+        self.user_dict = dict()
+        self.user_id_email_dict = dict()
         for user in self.initial_data["realm_users"]:
             if self.user_id == user["user_id"]:
                 self._all_users_by_id[self.user_id] = user
-                current_user = {
+                current_user: MinimalUserData = {
                     "full_name": user["full_name"],
                     "email": user["email"],
                     "user_id": user["user_id"],
@@ -1106,6 +1106,8 @@ class Model:
                 }
                 continue
             email = user["email"]
+
+            status: UserStatus
             if user["is_bot"]:
                 # Bot has no dynamic status, so avoid presence lookup
                 status = "bot"
@@ -1131,7 +1133,7 @@ class Model:
                 * If there are several ClientPresence objects with the greatest
                 * UserStatus, an arbitrary one is chosen.
                 """
-                aggregate_status = "offline"
+                aggregate_status: UserStatus = "offline"
                 for client in presences[email].items():
                     client_name = client[0]
                     status = client[1]["status"]
@@ -1201,7 +1203,7 @@ class Model:
         self.user_dict[current_user["email"]] = current_user
         self.user_id_email_dict[self.user_id] = current_user["email"]
 
-        return user_list
+        self.users = user_list
 
     def user_name_from_id(self, user_id: int) -> str:
         """


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index 5f18bfc..b42e955 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -17,7 +17,12 @@ from zulipterminal.config.keys import (
     keys_for_command,
     primary_key_for_command,
 )
-from zulipterminal.helper import CustomProfileData, Index, TidiedUserInfo
+from zulipterminal.helper import (
+    CustomProfileData,
+    Index,
+    MinimalUserData,
+    TidiedUserInfo,
+)
 from zulipterminal.helper import initial_index as helper_initial_index
 from zulipterminal.ui_tools.buttons import StreamButton, TopicButton, UserButton
 from zulipterminal.ui_tools.messages import MessageBox
@@ -1227,7 +1232,7 @@ def error_response() -> Dict[str, str]:
 
 
 @pytest.fixture
-def user_dict(logged_on_user: Dict[str, Any]) -> Dict[str, Dict[str, Any]]:
+def user_dict(logged_on_user: Dict[str, Any]) -> Dict[str, MinimalUserData]:
     """
     User_dict created according to `initial_data` fixture.
     """
diff --git a/tests/core/test_core.py b/tests/core/test_core.py
index 17517d7..9f13a40 100644
--- a/tests/core/test_core.py
+++ b/tests/core/test_core.py
@@ -222,7 +222,14 @@ class TestController:
         controller.view.message_view = mocker.patch("urwid.ListBox")
         controller.model.user_id = 5140
         controller.model.user_email = "some@email"
-        controller.model.user_dict = {user_email: {"user_id": user_id}}
+        controller.model.user_dict = {
+            user_email: {
+                "user_id": user_id,
+                "full_name": "",
+                "email": "",
+                "status": "active",
+            }
+        }
 
         emails = [user_email]
 
diff --git a/tests/model/test_model.py b/tests/model/test_model.py
index aa5c9bf..2bf150a 100644
--- a/tests/model/test_model.py
+++ b/tests/model/test_model.py
@@ -44,7 +44,7 @@ class TestModel:
     def model(self, mocker, initial_data, user_profile, unicode_emojis):
         mocker.patch(MODEL + ".get_messages", return_value="")
         self.client.register.return_value = initial_data
-        mocker.patch(MODEL + ".get_all_users", return_value=[])
+        mocker.patch(MODEL + "._update_users_data_from_initial_data")
         # NOTE: PATCH WHERE USED NOT WHERE DEFINED
         self.classify_unread_counts = mocker.patch(
             MODULE + ".classify_unread_counts", return_value=[]
@@ -85,7 +85,7 @@ class TestModel:
         assert model.user_email == user_profile["email"]
         assert model.server_name == initial_data["realm_name"]
         # FIXME Add test here for model.server_url
-        model.get_all_users.assert_called_once_with()
+        model._update_users_data_from_initial_data.assert_called_once_with()
         assert model.users == []
         self.classify_unread_counts.assert_called_once_with(model)
         assert model.unread_counts == []
@@ -192,7 +192,7 @@ class TestModel:
             MODEL + "._register_desired_events", return_value="Invalid API key"
         )
 
-        mocker.patch(MODEL + ".get_all_users", return_value=[])
+        mocker.patch(MODEL + "._update_users_data_from_initial_data")
         mocker.patch(MODEL + "._subscribe_to_streams")
         self.classify_unread_counts = mocker.patch(
             MODULE + ".classify_unread_counts", return_value=[]
@@ -210,7 +210,7 @@ class TestModel:
             MODEL + "._register_desired_events", side_effect=ZulipError(exception_text)
         )
 
-        mocker.patch(MODEL + ".get_all_users", return_value=[])
+        mocker.patch(MODEL + "._update_users_data_from_initial_data")
         mocker.patch(MODEL + "._subscribe_to_streams")
         self.classify_unread_counts = mocker.patch(
             MODULE + ".classify_unread_counts", return_value=[]
@@ -223,7 +223,7 @@ class TestModel:
 
     def test_register_initial_desired_events(self, mocker, initial_data):
         mocker.patch(MODEL + ".get_messages", return_value="")
-        mocker.patch(MODEL + ".get_all_users")
+        mocker.patch(MODEL + "._update_users_data_from_initial_data")
         self.client.register.return_value = initial_data
 
         model = Model(self.controller)
@@ -1283,7 +1283,7 @@ class TestModel:
         num_after=10,
     ):
         self.client.register.return_value = initial_data
-        mocker.patch(MODEL + ".get_all_users", return_value=[])
+        mocker.patch(MODEL + "._update_users_data_from_initial_data")
         mocker.patch(MODEL + "._subscribe_to_streams")
         self.classify_unread_counts = mocker.patch(
             MODULE + ".classify_unread_counts", return_value=[]
@@ -1406,7 +1406,7 @@ class TestModel:
 
         # Initialize Model
         self.client.register.return_value = initial_data
-        mocker.patch(MODEL + ".get_all_users", return_value=[])
+        mocker.patch(MODEL + "._update_users_data_from_initial_data")
         mocker.patch(MODEL + "._subscribe_to_streams")
         self.classify_unread_counts = mocker.patch(
             MODULE + ".classify_unread_counts", return_value=[]
@@ -1434,7 +1434,7 @@ class TestModel:
     ):
         # Initialize Model
         self.client.register.return_value = initial_data
-        mocker.patch(MODEL + ".get_all_users", return_value=[])
+        mocker.patch(MODEL + "._update_users_data_from_initial_data")
         mocker.patch(MODEL + "._subscribe_to_streams")
         self.classify_unread_counts = mocker.patch(
             MODULE + ".classify_unread_counts", return_value=[]
@@ -1687,7 +1687,9 @@ class TestModel:
         model._all_users_by_id = _all_users_by_id
         assert model.get_user_info(12) == tidied_user_info_response
 
-    def test_get_all_users(self, mocker, initial_data, user_list, user_dict, user_id):
+    def test__update_users_data_from_initial_data(
+        self, mocker, initial_data, user_list, user_dict, user_id
+    ):
         mocker.patch(MODEL + ".get_messages", return_value="")
         self.client.register.return_value = initial_data
         mocker.patch(MODEL + "._subscribe_to_streams")
diff --git a/tests/ui_tools/test_boxes.py b/tests/ui_tools/test_boxes.py
index 975b14c..869cc98 100644
--- a/tests/ui_tools/test_boxes.py
+++ b/tests/ui_tools/test_boxes.py
@@ -15,7 +15,7 @@ from zulipterminal.config.symbols import (
     STREAM_MARKER_WEB_PUBLIC,
 )
 from zulipterminal.config.ui_mappings import StreamAccessType
-from zulipterminal.helper import Index
+from zulipterminal.helper import Index, MinimalUserData
 from zulipterminal.ui_tools.boxes import PanelSearchBox, WriteBox, _MessageEditState
 from zulipterminal.urwid_types import urwid_Size
 
@@ -40,7 +40,7 @@ class TestWriteBox:
         user_groups_fixture: List[Dict[str, Any]],
         streams_fixture: List[Dict[str, Any]],
         unicode_emojis: "OrderedDict[str, Dict[str, Any]]",
-        user_dict: Dict[str, Dict[str, Any]],
+        user_dict: Dict[str, MinimalUserData],
     ) -> WriteBox:
         self.view.model.active_emoji_data = unicode_emojis
         self.view.model.all_emoji_names = list(unicode_emojis.keys())