Add support for all streams #1408

theViz343 · 2023-06-08T11:09:18Z

What does this PR do, and why?

This PR adds support for unsubscribed streams and never_subscribed streams in ZT. It creates new data structures and accessor methods to access them. This PR is needed for further work on reducing stream related issues and adding support for stream (un)subscription.

Outstanding aspect(s)

External discussion & connections

Discussed in #zulip-terminal in Adding streams throws key error
Fully fixes #
Partially fixes issue Adding new stream throws an Keyerror #816
Builds upon previous unmerged work in PR #
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

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.

This commit adds two accessor methods - _get_stream_from_id and get_all_stream_ids. These two accessor methods help in preparation for including specific stream property accessor methods in the next set of commits. Tests added.

This commit replaces the usage of directly indexing stream_dict for stream "name" with the new stream property accessor method "get_stream_name". Test added.

…ict. This commit replaces the usage of directly indexing stream_dict for "subscribers" with the new stream property accessor method "get_stream_subscribers". Test added.

This commit replaces the usage of directly indexing stream_dict for "date_created" data with the new stream property accessor methods "get_stream_date_created" and "set_stream_date_created". Tests added.

prah23

Thanks for the PR @theViz343!
This is a neat refactor; I've just left some minor points of feedback.

prah23 · 2023-06-13T14:15:00Z

zulipterminal/model.py

@@ -174,11 +175,18 @@ def __init__(self, controller: Any) -> None:
        self.users = self.get_all_users()

        self.stream_dict: Dict[int, Any] = {}


This can now be typed as a Dict with int keys and Subscription values; reducing the use of Any will improve our type-checking.

Resolved in #1419 👍

prah23 · 2023-06-13T14:19:04Z

zulipterminal/model.py

@@ -1204,6 +1204,9 @@ def get_stream_date_created(self, stream_id: int) -> Optional[int]:
    def set_stream_date_created(self, stream_id: int, value: Optional[int]) -> None:
        self._get_stream_from_id(stream_id)["date_created"] = value

+    def is_stream_web_public(self, stream_id: int) -> Optional[bool]:
+        return self._get_stream_from_id(stream_id).get("is_web_public")


Since the is_web_public key is guaranteed to be present and is typed as a bool in api_types, you can index the dict instead of using get so the return type for this method can be bool, consistent with its type in the Stream class.

prah23 · 2023-06-13T15:04:32Z

zulipterminal/model.py

+            return self.stream_dict[stream_id]["email_address"]
+        elif stream_id in self._unsubscribed_streams:
+            return self._unsubscribed_streams[stream_id]["email_address"]
+        return None


Since this method is only being used in the StreamInfoView and you haven't added the feature of viewing stream info for a never subscribed stream, can the base condition of returning None be avoided? The return value of this method is currently attached to a copy command, and having to copy a None to the clipboard doesn't seem appropriate.

I've made it so that the method raises a RuntimeError if the stream is not a subscription (or does not exist).

This commit replaces the usage of directly indexing stream_dict for "is_web_public" data with the new stream property accessor method "is_stream_web_public". Test added.

This commit replaces the usage of directly indexing stream_dict for "message_retention_days" data with the new stream property accessor methods "get_stream_message_retention_days" and "set_stream_message_retention_days". Test added.

This commit replaces the usage of directly indexing stream_dict for "is_invite_only" data with the new stream property accessor method "is_stream_invite_only". Test added.

This commit replaces the usage of directly indexing stream_dict for "stream_post_policy" with the new stream property accessor method "get_stream_post_policy". Test added.

This commit replaces the usage of directly indexing stream_dict for "is_announcement_only" data with the new stream property accessor method "is_stream_announcement_only". Test added.

…thod. This commit replaces the usage of directly indexing stream_dict for "history_public_to_subscribers" data with the new stream property accessor method "is_stream_history_public_to_subscribers". Test added.

This commit replaces the usage of directly indexing stream_dict for "stream_weekly_traffic" data with the new stream property accessor method "get_stream_weekly_traffic". Test added.

This commit replaces the usage of directly indexing stream_dict for "rendered_description" data with the new stream property accessor method "get_stream_rendered_description". Test added.

This commit adds the subscription ids accessor method "get_all_stream_ids". This helps in preparation for including subscription color accessor method added in the next commit. Tests added.

This commit replaces the usage of directly indexing stream_dict for "color" data with the new stream property accessor method "get_subscription_color". Test added.

…ict. This commit replaces the usage of directly indexing stream_dict for "email_address" data with the new stream property accessor method "get_subscription_email". Test added.

This commit improves the typing of stream_dict by using Subscription TypedDict instead of Dict[str, Any]. Tests updated.

This commit renames stream_dict to _subscribed_streams. This change is necessary due to the introduction of _unsubscribed_streams and never_subscribed streams in the model. Tests updated.

Subhasish-Behera · 2023-06-20T18:20:32Z

tests/model/test_model.py

+            MODEL + "._get_stream_from_id", return_value=get_stream_from_id_fixture
+        )
+        assert model.get_stream_name(stream_id) == expected_value
+        model._get_stream_from_id.assert_called_once()


can assert_called_once_with be used here?

Subhasish-Behera · 2023-07-08T10:30:21Z

@theViz343 Thanks for working on this! This seems to work well and was pretty easy to understand because of the commit structure and commit messages.
Apart from that, I have some minor code suggestions in the comments.

zulipbot · 2023-07-14T20:15:40Z

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.

neiljp

@theViz343 This has grown quite a lot since I last looked! I started a review of this before Hari, so apologies if this overlaps a little. The end result is great, though I think we can shrink it a little.

The PR seems to be split into two parts:

adding the un/never-subscribed data, improving the data types, and generalizing the current code to avoid eg. the KeyErrors
adding the encapsulation via the accessors, followed by the rename

Both of these parts have value, but I'd suggest grouping the first set together at the start so that we can consider them together, and so potentially merge the earlier commits in this PR first.

There are a lot of commits, particularly for the encapsulating functions, and isolating adding different methods and tests per commit is great. However, some of these functions are not used outside the model, and don't appear to be preparing for anything, or the effect is replicated in other functions already. You also have some 'setters', which we likely don't need for users of the model.

With a smaller set of new methods I think the changes will be clearer, but it may be worth discussing whether adding so many functions is the way to go, or rather using more general stream_properties and subscription_properties functions?

neiljp · 2023-06-10T19:05:56Z

zulipterminal/api_types.py

@@ -237,6 +226,21 @@ class Subscription(TypedDict):
    # in_home_view: bool  # Replaced by is_muted in Zulip 2.1; still present in updates


This was a personal setting.

zulipterminal/model.py

neiljp · 2023-06-10T19:38:59Z

zulipterminal/api_types.py

    stream_id: int
    name: str
    description: str
    rendered_description: str
-    date_created: int  # NOTE: new in Zulip 4.0 / ZFL 30
+    date_created: Optional[int]  # NOTE: new in Zulip 4.0 / ZFL 30


Did you find this started to fail a type check? We do set it explicitly to None!

The tricky thing here is that we reuse the existing assumed datastructures from the server for simplicity. For the purposes of loading the data it is NotRequired and is never None; once loaded it is expected to be present and may be Optional.

Maybe Union[NotRequired[int], Optional[int]]? I've not tested for validity of that syntax, and I'm unsure if it would help with our typing beyond what you have.

In any case, assuming we include the Optional[int] then we should add a comment that the None part is ZT-specific.

Yes, the part where we add the date_created key and set it to None fails the typecheck. Since we get the data from the server initially, mypy doesn't catch that date_created is not present. We could account for this by using NotRequired[Optional[int]] instead, but that would purely be for documentation since it passes the typecheck with just Optional[int]. I'll also add a comment.

neiljp · 2023-06-10T20:02:58Z

zulipterminal/api_types.py

+
+    is_muted: bool
+
+    role: NotRequired[int]  # NOTE: new in Zulip 4.0 / ZFL 31


This seems accurate, and we may want to apply NotRequired more often in general - it wasn't an option until quite recently. However, the API notes indicate it was removed again in 6.0/133, so I guess we can include it commented-out as with in_home_view. (I think it's being replaced by the user-group approach for stream management)

Like with the other change to date_created, I'd suggest mentioning these kinds of changes in the commit summary. This applies to any commit with a change which looks like only the move of code but also has minor adjustments that wouldn't necessarily be noted by a reviewer or someone looking at changes later.
(git tools can show lines moved vs changed, but GitHub doesn't :/)

However, given this and other changes in the fields, both that you've included and that seem present in the updated API docs, I'd suggest splitting those out into a separate commit - unless the changes are necessary for type-checking?

neiljp · 2023-06-10T20:08:50Z

zulipterminal/model.py

        self.muted_streams: Set[int] = set()
        self.pinned_streams: List[StreamData] = []
        self.unpinned_streams: List[StreamData] = []
        self.visual_notified_streams: Set[int] = set()

+        self._register_non_subscribed_streams(


I was going to refer back to my suggestion in #1387 (comment) to inline this function since we only use it once, but we may need to reinitialize these values if we lose connection while ZT is running.

neiljp · 2023-07-14T21:53:51Z

zulipterminal/model.py

+    def get_stream_message_retention_days(self, stream_id: int) -> Optional[int]:
+        return self._get_stream_from_id(stream_id).get("message_retention_days")
+
+    def set_stream_message_retention_days(
+        self, stream_id: int, value: Optional[int]
+    ) -> None:
+        self._get_stream_from_id(stream_id)["message_retention_days"] = value


The accessor here has more purpose - but only if we use it to wrap accessing the .cached_retention_text data, which is what is currently accessed externally :) That will require updating the code you introduced above this.

We already have the _get_stream_from_id helper, and there's only one place it's used, and that's inside the model - so I'm unconvinced this is necessary.

neiljp · 2023-07-14T22:06:47Z

zulipterminal/model.py

+    def is_stream_invite_only(self, stream_id: int) -> bool:
+        return self._get_stream_from_id(stream_id)["invite_only"]


Similarly to is_web_public, we already have a method for this, which you've updated to use this accessor.

There is no use of this data outside of the model in any case, other than through stream_access_type(), so there is no motivation for introducing this accessor.

neiljp · 2023-07-14T22:15:43Z

zulipterminal/ui_tools/views.py

+        if controller.model.get_stream_post_policy(self.stream_id):
+            stream_policy = STREAM_POST_POLICY[
+                controller.model.get_stream_post_policy(self.stream_id)
+            ]
        else:
            if stream.get("is_announcement_only"):


This existing UI code incorporates implicit checking of the server version, which would be better wrapped into the model under get_stream_post_policy, rather than the two functions introduced in this commit and the next.

neiljp · 2023-07-14T22:28:59Z

zulipterminal/model.py

@@ -174,7 +174,7 @@ def __init__(self, controller: Any) -> None:

        self.users = self.get_all_users()

-        self.stream_dict: Dict[int, Any] = {}
+        self.stream_dict: Dict[int, Subscription] = {}


This change, and possibly others in this commit seem much more related to those earlier in the PR than here at the end?

neiljp · 2023-07-14T22:30:42Z

zulipterminal/ui_tools/views.py

@@ -1268,7 +1268,7 @@ class StreamInfoView(PopUpView):
    def __init__(self, controller: Any, stream_id: int) -> None:
        self.stream_id = stream_id
        self.controller = controller
-        stream = controller.model.stream_dict[stream_id]
+        stream = controller.model._subscribed_streams[stream_id]


This suggests we haven't covered accessing the stream or subscription properties sufficiently?

zulipbot added the size: XL [Automatic label added by zulipbot] label Jun 8, 2023

model: Add stream id accessor methods.

82ed222

This commit adds two accessor methods - _get_stream_from_id and get_all_stream_ids. These two accessor methods help in preparation for including specific stream property accessor methods in the next set of commits. Tests added.

theViz343 force-pushed the add-support-for-all-streams branch from 62e7272 to 76d71ca Compare June 10, 2023 11:09

theViz343 added 3 commits June 10, 2023 17:24

refactor: model: Use get_stream_name instead of stream_dict.

d1a8eb7

This commit replaces the usage of directly indexing stream_dict for stream "name" with the new stream property accessor method "get_stream_name". Test added.

refactor: model/views: Use get_stream_subscribers instead of stream_d…

b51eae4

…ict. This commit replaces the usage of directly indexing stream_dict for "subscribers" with the new stream property accessor method "get_stream_subscribers". Test added.

refactor: model: Use stream_date_created (get/set)ter methods.

80e0d5a

This commit replaces the usage of directly indexing stream_dict for "date_created" data with the new stream property accessor methods "get_stream_date_created" and "set_stream_date_created". Tests added.

theViz343 force-pushed the add-support-for-all-streams branch from 76d71ca to ec9f1d8 Compare June 10, 2023 15:12

theViz343 marked this pull request as ready for review June 10, 2023 15:13

theViz343 added PR needs review PR requires feedback to proceed PR needs buddy review labels Jun 11, 2023

prah23 suggested changes Jun 13, 2023

View reviewed changes

theViz343 added 13 commits June 15, 2023 12:58

refactor: model: Use is_stream_web_public instead of stream_dict.

b6ab397

This commit replaces the usage of directly indexing stream_dict for "is_web_public" data with the new stream property accessor method "is_stream_web_public". Test added.

refactor: model: Use is_stream_invite_only instead of stream_dict.

023ea38

This commit replaces the usage of directly indexing stream_dict for "is_invite_only" data with the new stream property accessor method "is_stream_invite_only". Test added.

refactor: model/views: Use get_stream_post_policy accessor method.

6282150

This commit replaces the usage of directly indexing stream_dict for "stream_post_policy" with the new stream property accessor method "get_stream_post_policy". Test added.

refactor: model/views: Use is_stream_announcement_only accessor method.

71d5bd0

This commit replaces the usage of directly indexing stream_dict for "is_announcement_only" data with the new stream property accessor method "is_stream_announcement_only". Test added.

refactor: model/views: Use is_stream_history_public_to_subscribers me…

6a67577

…thod. This commit replaces the usage of directly indexing stream_dict for "history_public_to_subscribers" data with the new stream property accessor method "is_stream_history_public_to_subscribers". Test added.

refactor: model/views: Use get_stream_weekly_traffic method.

f451244

This commit replaces the usage of directly indexing stream_dict for "stream_weekly_traffic" data with the new stream property accessor method "get_stream_weekly_traffic". Test added.

refactor: model/views: Use get_stream_rendered_description method.

8d6e057

This commit replaces the usage of directly indexing stream_dict for "rendered_description" data with the new stream property accessor method "get_stream_rendered_description". Test added.

model: Add get_all_subscription_ids accessor method.

5e5bd20

This commit adds the subscription ids accessor method "get_all_stream_ids". This helps in preparation for including subscription color accessor method added in the next commit. Tests added.

refactor: model: Use get_subscription_color instead of stream_dict.

dee7e2b

This commit replaces the usage of directly indexing stream_dict for "color" data with the new stream property accessor method "get_subscription_color". Test added.

refactor: model/views: Use get_subscription_email instead of stream_d…

e39bbe3

…ict. This commit replaces the usage of directly indexing stream_dict for "email_address" data with the new stream property accessor method "get_subscription_email". Test added.

refactor: model: Improve stream_dict typing.

5ca5ee6

This commit improves the typing of stream_dict by using Subscription TypedDict instead of Dict[str, Any]. Tests updated.

refactor: model: Rename stream_dict to _subscribed_streams.

308fa13

This commit renames stream_dict to _subscribed_streams. This change is necessary due to the introduction of _unsubscribed_streams and never_subscribed streams in the model. Tests updated.

theViz343 force-pushed the add-support-for-all-streams branch from ec9f1d8 to 308fa13 Compare June 15, 2023 09:41

Subhasish-Behera self-requested a review June 20, 2023 18:19

Subhasish-Behera reviewed Jun 20, 2023

View reviewed changes

theViz343 mentioned this pull request Jun 21, 2023

Handle (un)subscribe events #1387

Open

21 tasks

Subhasish-Behera removed the PR needs buddy review label Jul 8, 2023

zulipbot added the has conflicts label Jul 14, 2023

neiljp reviewed Jul 14, 2023

View reviewed changes

neiljp added PR blocks other PR 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 Jul 14, 2023

theViz343 mentioned this pull request Jul 27, 2023

Add support for all streams (more general accessor methods) #1419

Draft

18 tasks

theViz343 mentioned this pull request Sep 17, 2023

Add support for all streams minimal #1430

Open

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for all streams #1408

Add support for all streams #1408

theViz343 commented Jun 8, 2023 •

edited

Loading

prah23 left a comment

prah23 Jun 13, 2023

neiljp Aug 1, 2023

prah23 Jun 13, 2023

prah23 Jun 13, 2023

theViz343 Jun 15, 2023

Subhasish-Behera Jun 20, 2023

Subhasish-Behera commented Jul 8, 2023

zulipbot commented Jul 14, 2023

neiljp left a comment

neiljp Jun 10, 2023

neiljp Jun 10, 2023

theViz343 Jul 27, 2023

neiljp Jun 10, 2023

neiljp Jun 10, 2023

neiljp Jul 14, 2023

neiljp Jul 14, 2023

neiljp Jul 14, 2023

neiljp Jul 14, 2023

neiljp Jul 14, 2023

		@@ -174,11 +175,18 @@ def __init__(self, controller: Any) -> None:
		self.users = self.get_all_users()

		self.stream_dict: Dict[int, Any] = {}

		@@ -237,6 +226,21 @@ class Subscription(TypedDict):
		# in_home_view: bool # Replaced by is_muted in Zulip 2.1; still present in updates


		is_muted: bool

		role: NotRequired[int] # NOTE: new in Zulip 4.0 / ZFL 31

		def is_stream_invite_only(self, stream_id: int) -> bool:
		return self._get_stream_from_id(stream_id)["invite_only"]

Add support for all streams #1408

Are you sure you want to change the base?

Add support for all streams #1408

Conversation

theViz343 commented Jun 8, 2023 • edited Loading

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

prah23 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

Subhasish-Behera commented Jul 8, 2023

zulipbot commented Jul 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theViz343 commented Jun 8, 2023 •

edited

Loading