Add support for all streams minimal #1430

theViz343 · 2023-09-17T03:30:34Z

What does this PR do, and why?

This PR is very similar to #1408 and #1419 and combines elements of both. This PR keeps the initial commits from #1419 but replaces the general accessor methods with only an attribute-specific necessary method i.e "name".

Outstanding aspect(s)

Could add more attribute accessor methods (like "color")

External discussion & connections

Discussed in #zulip-terminal in Adding streams throws key error
Fully fixes Adding new stream throws an Keyerror #816
Partially fixes issue #
Builds upon previous unmerged work in PR Add support for all streams (more general accessor methods) #1419 and PR Add support for all streams #1408
Is a follow-up to work in PR #
Requires merge of PR #
Merge will enable work on Handle (un)subscribe events #1387

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

zulipbot · 2023-09-17T03:30:38Z

Hello @theViz343, it seems like you have referenced #816 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 #816..

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.

neiljp

@theViz343 It's good to see another go at this one, though there are some comments from before to keep up with, including:

I'm happy with the core of this, but other than the points above was initially interested to see if the early new data in the Model you added could be confirmed in test_init using new fixtures. To see that you added fixtures which had similar data to test later code confirmed that would be useful, to test the data flow through the Model from the initial_data into the accessors.

tests/conftest.py

neiljp · 2023-09-21T00:43:17Z

zulipterminal/model.py

@@ -1281,6 +1281,22 @@ def _register_non_subscribed_streams(
            stream["stream_id"]: stream for stream in never_subscribed_streams
        }

+    def _get_stream_from_id(self, stream_id: int) -> Union[Subscription, Stream]:


The only way to use this function with a subscription is with additional information and then casting?

Given that, perhaps set the return type to Stream? We can have a separate function, something like _get_subscription_from_id, specifically for subscription data. That will not need to search through the never-subscribed, and can provide a different error.

Note that you only need what you have here for supporting the name and other core stream data, whereas eg. color will need Subscription.

If we do that, should I cast the subscribed and unsubscribed streams returned by this method to Stream? That might make the usage of this method clearer too, ie. to only access stream properties (and not subscription properties) from the returned stream.

A cast shouldn't be necessary once you have the two different functions - you don't have that in the latest version of the code you just pushed?

I've casted the stream_dict and the unsubscribed_streams to a Stream in the _get_stream_from_id in the latest commit, so that methods which use the _get_stream_from_id always access only the stream properties and not the subscription properties (which would then warrant the use of _get_subscription_from_id).

Ah right, but I don't remember seeing an explicit cast() to work around mypy - I assume this works in the type system since a Stream is a subset of a Subscription.

tests/model/test_model.py

tests/ui/test_ui_tools.py

zulipterminal/model.py

zulipterminal/api_types.py

neiljp · 2023-09-21T01:31:56Z

tests/core/test_core.py

        controller.model.stream_dict = {
-            stream_id: {
+            stream_id: general_stream,
+        }
+        controller.model.stream_dict[stream_id].update(
+            {
                "color": "#ffffff",
                "name": stream_name,
            }
-        }
+        )


It's good that this commit focuses on the stream/subscription migration, but

it doesn't seem that the color is important to the test(s)?

that could make this transition smaller/simpler

can we replace the stream_dict code by setting the return_value in the model, in the later commits? (if we don't already)

For the third point, which function's return_value are you referring to?

I meant that where you're moving to use the functions that access the name and color, instead of using stream_dict, can we now mock those functions and set the return value directly?

Essentially, we want to ensure we move away from the stream_dict fixture (or equivalent manual dicts), in addition to directly accessing the model attribute.

tests/conftest.py

neiljp

@theViz343 Thanks for the update again 👍

Overall this is good, and the commits are generally clean and well tested.

I think most of my non-minor feedback has come about since this refactoring is complex - tidying types, correcting fixtures, fixing inconsistencies, adding data, using new accessors, removing old fixtures, integrating with existing code, updating old accessors - and so there are a lot of potential rough edges, some of which only surface later.

The stream_dict removals in the tests could be left for another time, certainly the complex cases.

I think clarifying the stream/subscription validity functions are likely the standout issue to address here, and which we need to be a little careful about since they're already in use, and we don't interact with all streams in every case.

neiljp · 2023-09-28T05:46:50Z

zulipterminal/api_types.py

+    # NOTE: Server data may not contain this field, in which case ZT adds it
+    # and sets it to None.
+    date_created: NotRequired[Optional[int]]


Nit: Good comment, but you've lost the 'new in ...' details.

neiljp · 2023-09-28T05:49:25Z

zulipterminal/api_types.py

    color: str

    # Deprecated fields
    # in_home_view: bool  # Replaced by is_muted in Zulip 2.1; still present in updates
+    # role: int  # Removed in Zulip 6.0 / ZFL 133; not actively used by ZT or Zulip web


Nit: Also present 4.0/ZFL31 until then.

neiljp · 2023-09-28T05:51:29Z

tests/conftest.py

-                "history_public_to_subscribers": True,
-            }
-        ],
+        "unsubscribed": unsubscribed_streams_fixture,


Minor: I'd mention these changes to the fixtures in the commit text, even though we don't use them until now.

neiljp · 2023-09-28T05:53:31Z

zulipterminal/api_types.py

 #   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):
+class Stream(TypedDict):


With all of the extra test material, and the next commits tidying up only the existing types/stream_dict, this refactor seems somewhat separate from the feature you're adding.

I'd consider splitting and shifting the new data addition until just before you start using them in the helper methods.

neiljp · 2023-09-28T06:09:02Z

tests/model/test_model.py

+                    "first_message_id": None,
+                },
+            ),
+        ],


Minor: Would be helpful to include some case ids here, to indicate that there is one test for a subscribed and one for unsubscribed stream?

Similarly for the other method.

These basically act as comments while reading the code.

neiljp · 2023-09-28T07:42:09Z

tests/ui_tools/test_boxes.py

        write_box.model.is_valid_stream.return_value = is_valid_stream
-        write_box.model.stream_dict = stream_dict
+        write_box.model.get_all_stream_ids.return_value = all_stream_ids


Note how we're already using is_valid_stream here, vs using all_stream_ids, in reference to my other comment.

neiljp · 2023-09-28T07:46:10Z

tests/ui_tools/test_messages.py

@@ -1005,11 +1005,7 @@ def test_msg_generates_search_and_header_bar(
        assert_header_bar,
        assert_search_bar,
    ):
-        self.model.stream_dict = {


Can we remove other stream_dict in this file? (in that same trimming refactor, perhaps)

neiljp · 2023-09-28T07:49:05Z

tests/model/test_model.py

+            case(
+                999,
+                "#ddd",
+                id="Unsubscribed stream",
+            ),


This stream is not an unsubscribed stream. A different test is fine to ensure the color is different, but an actual unsubscribed (not never) would be good to include.

neiljp · 2023-09-28T07:53:56Z

tests/ui_tools/test_boxes.py

+        write_box.model.subscription_color_from_id.side_effect = lambda x: stream_dict[
+            x
+        ]["color"]


Again, this would be nice to avoid, but we might have to rethink the test.

neiljp · 2023-09-28T08:02:45Z

tests/core/test_core.py

        }
+        controller.model.stream_dict[stream_id]["name"] = stream_name


If it's necessary to keep this line for the test, can the stream_dict be removed instead later, when we switch to the new function?

This commit refactors the Subscription typeddict to separate Stream and Subscription TypedDicts. The Subscription TypedDict now inherits the Stream TypedDict. This is a preparatory refactor to later introduce the unsubscribed_streams and never_subscribed_streams to the model.

This commit updates the `date_created` and `stream_post_policy` fields present in the Stream typeddict, and the `role` field in the Subscription typeddict. For `date_created`, since servers with ZFL<30 do not include the field, but ZT does add it with a value of None to ensure consistency, the type is changed to `NotRequired[Optional[int]]`. For `stream_post_policy`, the field is only included after Zulip 3.0 / ZFL 1, so it has been changed to a NotRequired field. For `role`, the field has been removed in Zulip 6.0 (ZFL 133), so it has been removed from the Subscriptions typeddict and also from the unsubscribed_streams_fixture.

This commit creates data structures, similar to stream_dict, to support unsubscribed and never-subscribed streams. These streams are available in the fetched initial data which is used to populate the new data structures using the _register_non_subscribed_streams function. Test updated and fixtures added.

With the revamp of the Subscription TypedDict in an earlier commit, we use it to improve the typing of stream_dict in this commit. The `is_old_stream` is removed from stream fixtures since ZFL 14 removes them from the Streams object. This field was always equivalent to stream_weekly_traffic != null on the same object. Tests and fixtures updated to support the new typing.

This commit adds three helper methods - _get_stream_from_id, _get_subscription_from_id, and get_all_stream_ids. These three helper methods help in preparation for adding stream and subscription property accessor methods in the next commits. Tests and fixture added.

This commit introduces the stream_name_from_id method in order to limit the direct accessing of stream_dict outside the model. The commit also replaces the access of the name attribute from stream_dict with the new method. Tests added and updated.

This commit introduces the subscription_color_from_id method in order to limit the direct accessing of stream_dict outside the model. The commit also replaces the access of the color attribute from stream_dict with the new method. Tests added and updated.

This commit removes the direct use of stream dicts in most of the tests and if necessary, patches the stream accessor methods created in a previous commit.

zulipbot · 2024-04-17T23:34:46Z

Heads up @theViz343, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot added the size: XL [Automatic label added by zulipbot] label Sep 17, 2023

theViz343 added the PR needs review PR requires feedback to proceed label Sep 17, 2023

neiljp reviewed Sep 21, 2023

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 Sep 22, 2023

theViz343 commented Sep 25, 2023

View reviewed changes

tests/conftest.py Show resolved Hide resolved

theViz343 force-pushed the add-support-for-all-streams-minimal branch from 62f7883 to 84f31e9 Compare September 25, 2023 22:38

theViz343 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 Sep 25, 2023

theViz343 force-pushed the add-support-for-all-streams-minimal branch 2 times, most recently from 0345e9f to 01bda75 Compare September 28, 2023 03:32

neiljp reviewed Sep 28, 2023

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 Sep 28, 2023

theViz343 force-pushed the add-support-for-all-streams-minimal branch from 01bda75 to f03632e Compare December 15, 2023 01:06

theViz343 added 8 commits December 14, 2023 18:07

refactor: Reduce the direct usage of stream dict in tests.

d0c1a8d

This commit removes the direct use of stream dicts in most of the tests and if necessary, patches the stream accessor methods created in a previous commit.

theViz343 force-pushed the add-support-for-all-streams-minimal branch from f03632e to d0c1a8d Compare December 15, 2023 01:10

zulipbot added the has conflicts label Apr 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for all streams minimal #1430

Add support for all streams minimal #1430

theViz343 commented Sep 17, 2023 •

edited

zulipbot commented Sep 17, 2023

neiljp left a comment

neiljp Sep 21, 2023

theViz343 Sep 25, 2023

neiljp Sep 28, 2023

theViz343 Sep 28, 2023

neiljp Sep 28, 2023

neiljp Sep 21, 2023

theViz343 Sep 25, 2023

neiljp Sep 28, 2023

neiljp left a comment

neiljp Sep 28, 2023

neiljp Sep 28, 2023

neiljp Sep 28, 2023

neiljp Sep 28, 2023

neiljp Sep 28, 2023

neiljp Sep 28, 2023

neiljp Sep 28, 2023

neiljp Sep 28, 2023

neiljp Sep 28, 2023

neiljp Sep 28, 2023

zulipbot commented Apr 17, 2024

		}
		controller.model.stream_dict[stream_id]["name"] = stream_name

Add support for all streams minimal #1430

Are you sure you want to change the base?

Add support for all streams minimal #1430

Conversation

theViz343 commented Sep 17, 2023 • edited

What does this PR do, and why?

Outstanding aspect(s)

External discussion & connections

How did you test this?

Self-review checklist for each commit

Visual changes

zulipbot commented Sep 17, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zulipbot commented Apr 17, 2024

theViz343 commented Sep 17, 2023 •

edited