Repository: zulip/zulip-terminal

Issue:
ZT Crashes when viewing a message in browser in MacOS.
<img width="1031" alt="Screenshot 2024-02-15 at 9 44 55 PM" src="https://github.com/zulip/zulip-terminal/assets/71403193/deeadd45-decb-42f5-880c-30f709268a29">


ZT crashes when viewing a message in browser on MacOS.
My OS version = Ventura 13.6
Python version = 3.9

This might be a problem for other OS too. 

## Steps to replicate: 
1. Open ZT
2. Open message information popup
3. Select `open in web browser`

Discussion:
zulipbot: Hello @Sushmey, it seems like you have referenced #1471 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run `git commit --amend` in your command line client to amend your commit message description with `Fixes #1471.`.

An example of a correctly-formatted commit:

```
commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.
```

To learn how to write a great commit message, please refer to our [guide](https://zulip.readthedocs.io/en/latest/contributing/version-control.html#commit-messages).

<!-- fixCommitWarning -->

Sushmey: Hi Neil, thanks for the review!
I made the necessary changes

- Changed the commit text to include PY version discrepancy. 
- Used `getattr` with a default value.
- Removed the period all together.
- Added a test case.
- Changed the name of the test back to original name.

Sushmey: @zulipbot remove "PR awaiting update" 
add "PR needs review"
Sushmey: All these comments are really helpful making me understand how to write better tests. I've made changes incorporating all your feedback. The test changes are as follows, 

There are two new parameters, `webbrowser_name` and `expected_attribute_present`, the first one is what the name attribute is supposed to be if it has one, and the second one tells if the test case has name attribute or not.

Then we check if `webbrowser_name` is None, if yes, then delete the name attribute from the mock since that's the case we are testing for. If `webbrowser_name` exists, then we just set the name attribute of the mock as `webbrowser_name`.

Finally, we check if the mock has a success report with the success message with `mock_get.return_value.name` if `expected_attribute_present` is true (i.e. name attribute does exist for this case) or if the attribute doesn't exist, if the success message has `"default browser"` instead of the web browser name.
neiljp: @Sushmey The test in this iteration looks like it should validate the code properly now - did you add the part I suggested, and then add the last conditional to make it pass?

The final point I was getting at was that the last parameter can simply be a string which represents the expected output value, *which you can use in the assert*. This is why I previously suggested/hinted the name of that parameter might not be good to keep as `expected_attribute_present`. 

While using the `mock_get` value in the assert looks like it works now, before accepting code that asserts on the same mock values that are assigned in a test, I'd aim to examine it very closely for validity. Here I suspect it couldn't work without that extra conditional at the end.

By using a string for that parametrize value, you can remove that extra conditional (`if`) very easily. It does lead to slight duplication of strings, but makes the test case data more explicit when you read the top; this currently reads as something like
```python
            "url, webbrowser_name, expected_attribute_present",
            ...
            ("https://chat.zulip.org/user_uploads/sent/abcd/efg.png", "mozilla", True),
            ("https://github.com/", None, False),
```
To understand the values of `expected_attribute_present` just based on the above, that parameter must be named very clearly, or else the test function must be read and understood.

When it's a string instead, which in some cases matches the input string, then it can be clearer what may be happening before you read the rest:
```python
            "url, webbrowser_name, expected_<descriptive name for output>",
            ...
            ("https://chat.zulip.org/user_uploads/sent/abcd/efg.png", "mozilla", "mozilla"),
            ("https://github.com/", None, "default browser"),
```
Sushmey: The last conditional was to validate the success message and the conditional seemed necessary since I wasn't sure if it's worth the trade-off for adding more parameters, albeit slightly redundant, just so that we could get rid of the conditionals in the test. I've added it now. Going forward, I'll try to reduce conditionals if adding parameters helps reduce them since it also makes the tests easier to understand at a glance. 

Added a new parameter of type `str` called `expected_webbrowser_name`
neiljp: @Sushmey Thanks for your work on this :+1: I just pushed here with some textual adjustments and will now merge :tada:

Patch:
diff --git a/zulipterminal/core.py b/zulipterminal/core.py
index ddc7e89..1e11354 100644
--- a/zulipterminal/core.py
+++ b/zulipterminal/core.py
@@ -414,11 +414,14 @@ class Controller:
             # Suppress stdout and stderr when opening browser
             with suppress_output():
                 browser_controller.open(url)
+
+                # MacOS using Python version < 3.11 has no "name" attribute
+                # - https://github.com/python/cpython/issues/82828
+                # - https://github.com/python/cpython/issues/87590
+                # Use a default value if missing, for macOS, and potential later issues
+                browser_name = getattr(browser_controller, "name", "default browser")
                 self.report_success(
-                    [
-                        "The link was successfully opened using "
-                        f"{browser_controller.name}"
-                    ]
+                    [f"The link was successfully opened using {browser_name}"]
                 )
         except webbrowser.Error as e:
             # Set a footer text if no runnable browser is located


Test patch:
diff --git a/tests/core/test_core.py b/tests/core/test_core.py
index 033f126..be05d49 100644
--- a/tests/core/test_core.py
+++ b/tests/core/test_core.py
@@ -406,15 +406,24 @@ class TestController:
         assert popup.call_args_list[0][0][1] == "area:error"
 
     @pytest.mark.parametrize(
-        "url",
+        "url, webbrowser_name, expected_webbrowser_name",
         [
-            "https://chat.zulip.org/#narrow/stream/test",
-            "https://chat.zulip.org/user_uploads/sent/abcd/efg.png",
-            "https://github.com/",
+            ("https://chat.zulip.org/#narrow/stream/test", "chrome", "chrome"),
+            (
+                "https://chat.zulip.org/user_uploads/sent/abcd/efg.png",
+                "mozilla",
+                "mozilla",
+            ),
+            ("https://github.com/", None, "default browser"),
         ],
     )
     def test_open_in_browser_success(
-        self, mocker: MockerFixture, controller: Controller, url: str
+        self,
+        mocker: MockerFixture,
+        controller: Controller,
+        url: str,
+        webbrowser_name: Optional[str],
+        expected_webbrowser_name: str,
     ) -> None:
         # Set DISPLAY environ to be able to run test in CI
         os.environ["DISPLAY"] = ":0"
@@ -422,11 +431,16 @@ class TestController:
         mock_get = mocker.patch(MODULE + ".webbrowser.get")
         mock_open = mock_get.return_value.open
 
+        if webbrowser_name is None:
+            del mock_get.return_value.name
+        else:
+            mock_get.return_value.name = webbrowser_name
+
         controller.open_in_browser(url)
 
         mock_open.assert_called_once_with(url)
         mocked_report_success.assert_called_once_with(
-            [f"The link was successfully opened using {mock_get.return_value.name}"]
+            [f"The link was successfully opened using {expected_webbrowser_name}"]
         )
 
     def test_open_in_browser_fail__no_browser_controller(