Handle (un)subscribe events #1387

theViz343 · 2023-04-24T23:08:53Z

What does this PR do, and why?

This PR adds event handling for (un)subscribe events. Without event handling, ZT crashes
if the user narrows onto a message in a stream which is unsubscribed from. This happens mostly
when a user is logged in from multiple clients, but can also occur if an admin unsubscribes the user.

Outstanding aspect(s)

Updated subscribed streams frequently have the wrong/default color. (This seems to be an issue of
ZT not loading the colors.)
ZT crashes when the currently narrowed message is unsubscribed from.
Should we notify user of the new (un)subscription? If yes, then where?(As a popup or as a message in the footer)

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

zulipbot · 2023-04-24T23:11:12Z

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 Thanks for the update on this - there's a lot to process here!

I've left a lot of feedback, but it is often contextual as I was reading through each commit, and in many cases overlaps with other comments to suggest an overall direction to take.

Separate from the specific code suggestions, quite a few of the other comments relate to structure between commits, and adjusting that will make further feedback easier. We can discuss commit order in the stream if that would help.

One additional factor that we may need to handle is unreads, which wasn't handled in the previous PR? I suspect unsubscribing should 'remove' any unreads in a stream, eg. they shouldn't contribute to the all-messages count. However, I don't know whether that data is explicitly cleared on the server side - do we get a mark-read event of some kind? - or whether clients are simply expected to zero any unreads in unsubscribed streams, or simply hide any unreads related to was-subscribed streams, ie. where unreads data is available but we are not now subscribed.

neiljp · 2023-06-01T15:08:25Z

zulipterminal/model.py

+            **self.stream_dict,
+            **self.unsubscribed_stream_dict,
+            **self.never_subscribed_stream_dict,


I'm not sure if we'd keep this function as it is in light of my other comments on this commit, but I suspect the typing illustrates this is problematic if returned together, and could be so even with other lookup? Or is this mitigated if we type in such a way that a Subscription is-a Stream?

A more involved but possibly cleaner approach to consider later may be to store all the Stream data separate to the extra Subscription data, ie. use ZT-specific data structure for the personal data, though we'd have to do additional data processing for that 🤷 The advantage would be to have a cache of all streams, and cache of personal data per streams (now-subscribed and was-subscribed), which could make lookup easier.

neiljp · 2023-06-01T15:15:42Z

zulipterminal/model.py

+    def get_subscribed_streams(self) -> Dict[int, Any]:
+        return self.stream_dict


You have a sequence of commits for the refactoring right now, but I'd suggest grouping the refactoring per each new function you introduce, concluding with a rename of stream_dict :) I'd be keen to merge those commits as they are first, if you keep them together early in the branch (or split into another PR), since this encapsulation, renaming and stream-vs-subscription clarification should improve the code quality independently of the event/UI changes.

For this particular function I'd question its worth - it returns a mutable structure which is not typed precisely.

Instead I'd explore specific properties that are accessed at each lookup into stream_dict, and what is minimally necessary. For example, perhaps returning individual properties would be sufficient, eg. stream_property(some_id, property) and similarly subscription_property(...), potentially limiting the properties to a subset of values until we add more features (via Literal types and/or at runtime). In most/all cases that should limit accidental mutability.

I'm not sure how many locations look up multiple stream/subscription properties, but I'd expect something like the above should be sufficient - I don't expect the multiple-property case to be overly frequent to motivate a more complex solution.
(On a similar note, once this is complete, we could consider improving how the pinned/unpinned data is handled, since it is something of a cached structure for multiple properties)

neiljp · 2023-06-01T15:17:36Z

zulipterminal/api_types.py

+    stream_weekly_traffic: Optional[int]
+
+
+class NeverSubscribedStream(TypedDict):


It's likely clearer if we simply term this a Stream, with a Subscription being an extended form of it.

neiljp · 2023-06-01T15:18:40Z

zulipterminal/api_types.py

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


+class UnsubscribedStream(TypedDict):


This Unsubscribed form appears to be identical to a Subscription?

neiljp · 2023-06-01T15:23:46Z

zulipterminal/model.py

+    def get_stream_from_id(
+        self, stream_id: int
+    ) -> Union[Subscription, UnsubscribedStream, NeverSubscribedStream]:


This is not returning a 'stream', it's returning a stream or a subscription, as per the type :)

There may be cases where this is warranted, but I suspect the calling site either is interested in knowing subscription information, or general stream information - so I'd suggest we likely want dedicated methods for each. For example a stream doesn't have a color property, so should error on that request (and ideally fail via typing too), and each set of methods will have different bounds on valid stream ids.

neiljp · 2023-06-02T22:54:20Z

zulipterminal/api_types.py

@@ -461,6 +461,11 @@ class RealmUserEvent(TypedDict):
 # (also -peer_add and -peer_remove; FIXME: -add & -remove are not yet supported)


+class RemovedSubscription(TypedDict):
+    stream_id: int
+    name: str


Minor: We need to be more consistent about this, but we could leave name commented out since we don't use it, with a comment to that effect?

neiljp · 2023-06-02T23:07:53Z

zulipterminal/model.py

+            stream_id = subscription["stream_id"]
+            if stream_id in self.unsubscribed_stream_dict:
+                del self.unsubscribed_stream_dict[stream_id]
+            elif stream_id in self.never_subscribed_stream_dict:
+                del self.never_subscribed_stream_dict[stream_id]
+            self.stream_dict[stream_id] = subscription


Minor: since the original line is expanded into a 'synchronize subscriptions' block, it seems clearer to have the stream_id extracted earlier?

That could be achieved by simply separating by a blank line, but the rest of this function could benefit from the use of stream_id in any case?

neiljp · 2023-06-02T23:16:43Z

zulipterminal/model.py

+            if stream_id in self.unsubscribed_stream_dict:
+                del self.unsubscribed_stream_dict[stream_id]
+            elif stream_id in self.never_subscribed_stream_dict:
+                del self.never_subscribed_stream_dict[stream_id]
+            self.stream_dict[stream_id] = subscription


I understand that this is aimed at ensuring that the stream 'dicts' are synchronized, and corresponds to the code in the other function, but it seems strange included within the subscription-removal commit, since nothing is calling them except the initial call to this function, when the synchronization should be a no-op.

It may be clearer to consider adding and removing subscriptions separately, commit-wise?

neiljp · 2023-06-02T23:24:04Z

zulipterminal/model.py

+            if stream_id in self.stream_dict:
+                self.unsubscribed_stream_dict[stream_id] = self.stream_dict[stream_id]
+                del self.stream_dict[stream_id]


Readability: for comparison with the subscription function, this should maybe go at the top of the loop?

neiljp · 2023-06-02T23:26:57Z

zulipterminal/model.py

+            self.normalize_and_cache_message_retention_text()
+            self.normalize_date_created_field()


These two lines here suggest these calls may belong in _subscribe_to_streams?

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.

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.

This commit moves the date_created field normalization code to a separate class function called normalize_date_created_field. The function adds the date_created field for a stream when the server feature level is less than 30. The value assigned is 'None'. Test added.

The refactor introduced in earlier commits now allows stream data to be removed from non-subscribed stream data structures, when a stream is subscribed to. Test updated.

This commit adds a function named "_unsubscribe_from_streams" to model.py. This function updates subscription data by removing unsubscribed streams from class variables - pinned_streams, unpinned_streams, muted_streams, and visual_notified_streams. Co-authored-by: Abhirup Pal <abhiruppalmethodist@gmail.com> Co-authored-by: Vishwesh Pillai <vishwesh103@gmail.com>

This commit adds event handling code for newly added/removed subscriptions. To store the stream subscriptions which are being updated in the event, a new field named "subscriptions" is added to the SubscriptionEvent class. Co-authored-by: Abhirup Pal <abhiruppalmethodist@gmail.com> Co-authored-by: Vishwesh Pillai <vishwesh103@gmail.com>

zulipbot · 2024-04-17T23:35:08Z

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: L [Automatic label added by zulipbot] label Apr 24, 2023

theViz343 force-pushed the handle-subscriptions-816 branch 3 times, most recently from f3ed55e to 2399169 Compare May 31, 2023 20:14

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels May 31, 2023

neiljp reviewed Jun 2, 2023

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Jun 2, 2023

theViz343 mentioned this pull request Jun 8, 2023

Add support for all streams #1408

Open

18 tasks

theViz343 added 17 commits June 10, 2023 14:54

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.

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.

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 handle-subscriptions-816 branch from 2399169 to ab97a94 Compare June 15, 2023 10:44

theViz343 force-pushed the handle-subscriptions-816 branch from ab97a94 to cd27476 Compare June 16, 2023 09:55

theViz343 marked this pull request as ready for review June 16, 2023 09:55

theViz343 and others added 3 commits June 16, 2023 17:44

model: Remove subscribed stream from nonsubscribed stream data.

cf2f006

The refactor introduced in earlier commits now allows stream data to be removed from non-subscribed stream data structures, when a stream is subscribed to. Test updated.

theViz343 force-pushed the handle-subscriptions-816 branch from cd27476 to 03aedca Compare June 16, 2023 12:17

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 Jun 16, 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

zulipbot added the has conflicts label Apr 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Handle (un)subscribe events #1387

Handle (un)subscribe events #1387

Uh oh!

theViz343 commented Apr 24, 2023 •

edited

Loading

Uh oh!

zulipbot commented Apr 24, 2023

Uh oh!

neiljp left a comment

Uh oh!

neiljp Jun 1, 2023

Uh oh!

neiljp Jun 1, 2023

Uh oh!

neiljp Jun 1, 2023

Uh oh!

neiljp Jun 1, 2023

Uh oh!

neiljp Jun 1, 2023

Uh oh!

neiljp Jun 2, 2023

Uh oh!

neiljp Jun 2, 2023

Uh oh!

neiljp Jun 2, 2023

Uh oh!

neiljp Jun 2, 2023

Uh oh!

neiljp Jun 2, 2023

Uh oh!

zulipbot commented Apr 17, 2024

Uh oh!

Uh oh!

		def get_subscribed_streams(self) -> Dict[int, Any]:
		return self.stream_dict

		stream_weekly_traffic: Optional[int]


		class NeverSubscribedStream(TypedDict):

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


		class UnsubscribedStream(TypedDict):

		self.normalize_and_cache_message_retention_text()
		self.normalize_date_created_field()

Uh oh!

Handle (un)subscribe events #1387

Are you sure you want to change the base?

Handle (un)subscribe events #1387

Uh oh!

Conversation

theViz343 commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

zulipbot commented Apr 24, 2023

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zulipbot commented Apr 17, 2024

Uh oh!

Uh oh!

theViz343 commented Apr 24, 2023 •

edited

Loading