Repository: zulip/zulip-terminal
Pull request #1451 (2023-12-15 07:23:23)

Bugfix: Workaround no stream email from zfl226
<!-- See README for documentation, or ask in #zulip-terminal if unclear -->
### What does this PR do, and why?

This is a minimal bugfix for the change to the API at Zulip feature level 226, ie. from Zulip 7.5 upwards, including Zulip 8.0.

Without this bugfix, ZT crashes upon opening the stream information popup for these recent server versions.

For now this reports that the email address is not available and disables the copying of the email address, if the field is absent.

To enable testing across feature levels, this adds some basic tests which are later built upon, and which could be used elsewhere.

Discussion:
- https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/Stream.20Information.20ZT.20Crash
- https://chat.zulip.org/#narrow/stream/378-api-design/topic/Removing.20.60email_address.60.20from.20subscription.20objects.3F/near/1683612

### Outstanding aspect(s)    <!-- DELETE SECTION IF EMPTY -->
<!-- In what ways is this not fully implemented/functioning? Compared to a discussion/issue? -->
<!-- Do you not understand something? Are you unsure about a certain approach? Want feedback? -->
- [ ] Use the new endpoint added for this to achieve the previous functionality (as discussed)

### External discussion & connections
<!-- [x] all that apply, specifying topic and adding numbers after # for issues/PRs -->
- [x] Discussed in **#zulip-terminal** in `Stream Information ZT Crash`
- [ ] 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 -->
- [x] Manually - Behavioral changes
- [x] Manually - Visual changes
- [x] Adapting existing automated tests
- [x] Adding automated tests for new behavior (or missing tests)
- [ ] 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
- [x] It contains test additions for any new behavior
- [x] It flows clearly from a previous branch commit, and/or prepares for the next commit


Patch:
diff --git a/zulipterminal/ui_tools/views.py b/zulipterminal/ui_tools/views.py
index fcb1164..1d63b5e 100644
--- a/zulipterminal/ui_tools/views.py
+++ b/zulipterminal/ui_tools/views.py
@@ -1377,8 +1377,15 @@ class StreamInfoView(PopUpView):
             else "Not Public to Users"
         )
         member_keys = ", ".join(map(repr, keys_for_command("STREAM_MEMBERS")))
-        self.stream_email = stream["email_address"]
-        email_keys = ", ".join(map(repr, keys_for_command("COPY_STREAM_EMAIL")))
+
+        # FIXME: This field was removed from the subscription data in Zulip 7.5 / ZFL226
+        #   We should use the new /streams/{stream_id}/email_address endpoint instead
+        self._stream_email = stream.get("email_address", None)
+        if self._stream_email is None:
+            stream_copy_text = "< Stream email is unavailable >"
+        else:
+            email_keys = ", ".join(map(repr, keys_for_command("COPY_STREAM_EMAIL")))
+            stream_copy_text = f"Press {email_keys} to copy Stream email address"
 
         weekly_traffic = stream["stream_weekly_traffic"]
         weekly_msg_count = (
@@ -1393,7 +1400,8 @@ class StreamInfoView(PopUpView):
         )
         desc = urwid.Text(self.markup_desc)
 
-        stream_info_content = [
+        # NOTE: This is treated as a member to make it easier to test
+        self._stream_info_content = [
             (
                 "Stream Details",
                 [
@@ -1408,10 +1416,7 @@ class StreamInfoView(PopUpView):
                         "Stream Members",
                         f"{total_members} (Press {member_keys} to view list)",
                     ),
-                    (
-                        "Stream email",
-                        f"Press {email_keys} to copy Stream email address",
-                    ),
+                    ("Stream email", stream_copy_text),
                     ("History of Stream", f"{availability_of_history}"),
                     ("Posting Policy", f"{stream_policy}"),
                 ],
@@ -1420,7 +1425,7 @@ class StreamInfoView(PopUpView):
         ]  # type: PopUpViewTableContent
 
         popup_width, column_widths = self.calculate_table_widths(
-            stream_info_content, len(title)
+            self._stream_info_content, len(title)
         )
 
         muted_setting = urwid.CheckBox(
@@ -1473,7 +1478,7 @@ class StreamInfoView(PopUpView):
                 ),
             ]
         self.widgets = self.make_table_with_categories(
-            stream_info_content, column_widths
+            self._stream_info_content, column_widths
         )
 
         # Stream description.
@@ -1501,8 +1506,10 @@ class StreamInfoView(PopUpView):
     def keypress(self, size: urwid_Size, key: str) -> str:
         if is_command_key("STREAM_MEMBERS", key):
             self.controller.show_stream_members(stream_id=self.stream_id)
-        elif is_command_key("COPY_STREAM_EMAIL", key):
-            self.controller.copy_to_clipboard(self.stream_email, "Stream email")
+        elif (
+            is_command_key("COPY_STREAM_EMAIL", key) and self._stream_email is not None
+        ):
+            self.controller.copy_to_clipboard(self._stream_email, "Stream email")
         return super().keypress(size, key)
 
 


Test patch:
diff --git a/tests/ui_tools/test_popups.py b/tests/ui_tools/test_popups.py
index be9ee49..2c5ce92 100644
--- a/tests/ui_tools/test_popups.py
+++ b/tests/ui_tools/test_popups.py
@@ -1379,17 +1379,62 @@ class TestStreamInfoView:
         # + 3(checkboxes) + [2-5](fields, depending upon server_feature_level)
         assert stream_info_view.height == expected_height
 
+    def test_stream_info_content__sections(self) -> None:
+        assert len(self.stream_info_view._stream_info_content) == 2
+
+        stream_details, stream_settings = self.stream_info_view._stream_info_content
+        assert stream_details[0] == "Stream Details"
+        assert stream_settings[0] == "Stream settings"
+
+    @pytest.mark.parametrize(
+        "stream_email_present, expected_copy_text",
+        [
+            (False, "< Stream email is unavailable >"),
+            (True, "Press 'c' to copy Stream email address"),
+        ],
+    )
+    def test_stream_info_content__email_copy_text(
+        self,
+        general_stream: Dict[str, Any],
+        stream_email_present: bool,
+        expected_copy_text: str,
+    ) -> None:
+        if not stream_email_present:
+            del general_stream["email_address"]
+
+        model = self.controller.model
+        stream_id = general_stream["stream_id"]
+        model.stream_dict = {stream_id: general_stream}
+
+        # Custom, to enable variation of stream data before creation
+        stream_info_view = StreamInfoView(self.controller, stream_id)
+
+        stream_details, _ = stream_info_view._stream_info_content
+        stream_details_data = stream_details[1]
+
+        assert ("Stream email", expected_copy_text) in stream_details_data
+
+    @pytest.mark.parametrize("normalized_email_address", ("user@example.com", None))
     @pytest.mark.parametrize("key", keys_for_command("COPY_STREAM_EMAIL"))
     def test_keypress_copy_stream_email(
-        self, key: str, widget_size: Callable[[Widget], urwid_Size]
+        self,
+        key: str,
+        normalized_email_address: Optional[str],
+        widget_size: Callable[[Widget], urwid_Size],
     ) -> None:
         size = widget_size(self.stream_info_view)
+        # This patches inside the object, which is fragile but tests the logic
+        # Note that the assert uses the same variable
+        self.stream_info_view._stream_email = normalized_email_address
 
         self.stream_info_view.keypress(size, key)
 
-        self.controller.copy_to_clipboard.assert_called_once_with(
-            self.stream_info_view.stream_email, "Stream email"
-        )
+        if normalized_email_address is not None:
+            self.controller.copy_to_clipboard.assert_called_once_with(
+                self.stream_info_view._stream_email, "Stream email"
+            )
+        else:
+            self.controller.copy_to_clipboard.assert_not_called()
 
     @pytest.mark.parametrize(
         "rendered_description, expected_markup",