Update email_address to be fetched from new API #1450

rsashank · 2023-12-14T05:17:37Z

What does this PR do, and why?

Added helper function to send request and get stream email_address, and called the helper function when user is viewing Stream Information.

External discussion & connections

Discussed in #zulip-terminal in topic https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/Stream.20Information.20ZT.20Crash
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?

Manually - Behavioral changes
Manually - Visual changes
Adapting existing automated tests
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

It is a minimal coherent idea
It has a commit summary following the documented style (title & body)
It has a commit summary describing the motivation and reasoning for the change
It individually passes linting and tests
It contains test additions for any new behavior
It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

neiljp

@rsashank Thanks for the updates here 👍

I tested this and it appears to manually work against the community server, but as noted inline this will not work as expected for older servers, or likely where the incoming email is not configured on newer servers.

To isolate the API-specific logic, one option would be to have stream_email_address handle the logic across server versions. That could then be tested separately, and the existing UI tests could remain very similar.

zulipterminal/ui_tools/views.py

rsashank · 2023-12-25T21:17:42Z

@zulipbot add "PR needs review"

neiljp

@rsashank This looks to have a good flow, but I think we should take the opportunity to move the logic that varies with server version in the model.

It would also be good to see independent tests for the model methods.

When you update the suggested additional model method to call the _fetch, that would be a good time to include what is currently in the first commit - and using NotRequired would be a good change at that point too.

neiljp · 2024-02-13T18:41:14Z

zulipterminal/model.py

+    def stream_email_address(self, stream_id: int) -> str:
+        """
+        Return a string containing the email address of a stream.
+        """


Let's name this more explicitly with a _fetch prefix, to indicate this makes an API call to the server. The leading underscore will mean that we don't expect it to be called/used outside of the model.

My suggestion would be to add a separate model function which wraps the logic for determining if the data is present in the stream/subscription, and fetches it otherwise using the wrapped .call_endpoint method. You might consider the movement of the existing code in the popup into this new method to be a preliminary refactor; then once you have added the _fetch commit, you can update this other method to call it.

The docstring doesn't add much, compared to the method name and signature (ie. parameter & return value)?

Thanks for the review! I've updated the PR accordingly.

I'm still unsure about the docstrings though.

zulipterminal/ui_tools/views.py

rsashank · 2024-02-15T10:54:13Z

@zulipbot remove "PR awaiting update"
@zulipbot add "PR needs review"

neiljp

@rsashank This looks close to ready - most of my feedback should be clear to address, but let me know if not :)

neiljp · 2024-05-11T00:05:38Z

zulipterminal/model.py

+        if response.get("result") == "success":
+            email_address = response.get("email", "")
+            return str(email_address)
+        return "invalid"


While this arbitrary string isn't likely to be mistaken for an email address, it would be clearer to have this return something like None instead.

This likely simplifies the rest of the code here too.

neiljp · 2024-05-11T00:11:21Z

zulipterminal/model.py

+        if stream_email == "invalid" or stream_email is None:
+            stream_email = ""
+        return stream_email


Similarly here, the empty string is a special marker string being returned, to indicate that we couldn't get the email address, but we can just use None again.

neiljp · 2024-05-11T00:13:26Z

zulipterminal/ui_tools/views.py

+        # This field was removed from the subscription data in Zulip 7.5 / ZFL226
+        # Using the new /streams/{stream_id}/email_address endpoint
+        # if field isn't present in subscription data


It's good to see this updated comment, but note that this detail is now described in the model, so we don't need it here :)

At this position in the code, all we care about is 'what is the stream email address for this stream', and rely upon the model to do the right thing, as best it can.

neiljp · 2024-05-11T00:20:39Z

tests/ui_tools/test_popups.py

@@ -1398,9 +1398,11 @@ def test_stream_info_content__email_copy_text(
        general_stream: Dict[str, Any],
        stream_email_present: bool,
        expected_copy_text: str,
+        mocker: MockerFixture,


There is no additional use of mocker here, so this is unnecessary?

neiljp · 2024-05-11T00:29:23Z

zulipterminal/model.py

+            return str(email_address)
+        return "invalid"
+
+    def stream_copy_text(self, stream_id: int) -> str:


This is certainly about a stream, but it's specifically returning an email address for the stream, so I'd be confused reading this (or where it's used) why it was named with a copy_text suffix.

So, let's use a more descriptive name for this method :)

neiljp · 2024-05-11T00:32:14Z

zulipterminal/model.py

+        Attempts to retrieve stream email from stream_dict, fetching
+        from method if unavailable. (Retains handling across server versions).
+        """
+        stream = self.stream_dict[stream_id]


There is a PR open to support fixing this problem more broadly, but in general we should check that a stream_id is in the dict until we have a clearer solution. See similar functions where we use a RuntimeError for now.

neiljp · 2024-05-11T00:47:09Z

tests/model/test_model.py

+        assert result == return_value
+
+    @pytest.mark.parametrize(
+        "stream, fetched, response, return_value",


These tests generally look good, but it's useful to use expected_ before parameters that are only used in asserts (and place them at the end).

Similarly, while the parameters are somewhat descriptive, more detail is generally better, eg.

is it all the stream data? part of it?

what is the response from?

neiljp · 2024-05-11T01:01:34Z

tests/ui_tools/test_popups.py

        if not stream_email_present:
            del general_stream["email_address"]
+            self.controller.model.stream_copy_text.return_value = ""

        model = self.controller.model
        stream_id = general_stream["stream_id"]


Can this be simplified further? The code doesn't access as much of the model data directly now.

neiljp · 2024-05-11T01:17:10Z

When you update the suggested additional model method to call the _fetch, that would be a good time to include what is currently in the first commit - and using NotRequired would be a good change at that point too.

Did you understand what I meant here? I missed following up on this.

Reading this again, I think my original idea was to add the model methods in two stages: firstly to wrap the direct data access (pure refactor) and secondly update that method and add the new one (so only in the model) to try and fetch the data instead - combined with the NotRequired change.

This isn't critical, so focus on the other points first, but consider how the code changes would read differently in two commits as above.

zulipbot added the size: S [Automatic label added by zulipbot] label Dec 14, 2023

rsashank changed the title ~~Zt emailapi~~ Update email_address to be fetched from new API Dec 14, 2023

rsashank force-pushed the zt_emailapi branch 7 times, most recently from 82035d7 to b5d4657 Compare December 16, 2023 02:04

neiljp reviewed Dec 20, 2023

View reviewed changes

zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved

zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved

rsashank force-pushed the zt_emailapi branch 4 times, most recently from 4bf3497 to e2856b6 Compare December 21, 2023 10:39

zulipbot added the PR needs review PR requires feedback to proceed label Dec 25, 2023

neiljp reviewed Feb 13, 2024

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Feb 13, 2024

rsashank force-pushed the zt_emailapi branch from e2856b6 to b8244ce Compare February 14, 2024 11:37

zulipbot added size: M [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Feb 14, 2024

rsashank force-pushed the zt_emailapi branch 5 times, most recently from e5ccdce to 1c5106c Compare February 15, 2024 10:39

zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Feb 15, 2024

rsashank force-pushed the zt_emailapi branch from 1c5106c to 590d2b1 Compare February 15, 2024 10:51

zulipbot added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Feb 15, 2024

rsashank force-pushed the zt_emailapi branch from 590d2b1 to 76026e0 Compare April 14, 2024 21:20

rsashank added 2 commits April 15, 2024 02:51

refactor: api_types: Remove email_address field in Subscription.

1111382

refactor: model/views: Get stream email address from new endpoint.

e5bfc2e

rsashank force-pushed the zt_emailapi branch from 76026e0 to e5bfc2e Compare April 14, 2024 21:22

neiljp reviewed May 11, 2024

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback api migrations version parity: 8 and removed PR needs review PR requires feedback to proceed labels May 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update email_address to be fetched from new API #1450

Update email_address to be fetched from new API #1450

rsashank commented Dec 14, 2023

neiljp left a comment

rsashank commented Dec 25, 2023

neiljp left a comment

neiljp Feb 13, 2024

rsashank Feb 15, 2024

rsashank commented Feb 15, 2024

neiljp left a comment

neiljp May 11, 2024

neiljp May 11, 2024

neiljp May 11, 2024

neiljp May 11, 2024

neiljp May 11, 2024

neiljp May 11, 2024

neiljp May 11, 2024

neiljp May 11, 2024

neiljp commented May 11, 2024

Update email_address to be fetched from new API #1450

Are you sure you want to change the base?

Update email_address to be fetched from new API #1450

Conversation

rsashank commented Dec 14, 2023

What does this PR do, and why?

External discussion & connections

How did you test this?

Self-review checklist for each commit

Visual changes

neiljp left a comment

Choose a reason for hiding this comment

rsashank commented Dec 25, 2023

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsashank commented Feb 15, 2024

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp commented May 11, 2024