From 95606a73479e58b626e7915c6bbf8ca3fa7e304b Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Fri, 16 Jul 2021 15:29:45 -0700 Subject: [PATCH] api: Return user IDs, not display emails, in subscribers endpoints. Sometime in the deep past, Zulip the GET /users/me/subscriptions endpoint started returning subscribers. We noticed this and made it optional via the include_subscribers parameter in 1af72a2745e974e67d1cdc6e8af4134c37d4551e, however, we didn't notice that they were being returned as emails rather than user IDs. We migrated the core /register code paths to use subscriber IDs years ago; this change completes that for the endpoints we forgot about. The documentation allowed this error because we apparently had no tests for this code path that used the actual API. --- templates/zerver/api/changelog.md | 9 +++++++ version.py | 2 +- zerver/lib/actions.py | 25 ++----------------- zerver/openapi/python_examples.py | 8 ++++++- zerver/tests/test_subs.py | 40 ++++++++++++++++++++++++------- zerver/views/streams.py | 6 ++--- 6 files changed, 53 insertions(+), 37 deletions(-) diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 065fcdfa38..e58e15f8dd 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -11,6 +11,15 @@ below features are supported. ## Changes in Zulip 5.0 +**Feature level 79** + +* [`GET /users/me/subscriptions`](/api/get-subscriptions): The + `subscribers` field now returns user IDs if `include_subscribers` is + passed. Previously, this endpoint returned user display email + addresses in this field. +* `GET /streams/{stream_id}/members`: This endpoint now returns user + IDs. Previously, it returned display email addresses. + **Feature level 78** * `PATCH /settings`: Added `ignored_parameters_unsupported` field, diff --git a/version.py b/version.py index 796d3b38d0..f2ff8a245b 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.4.3" # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md, as well as # "**Changes**" entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 78 +API_FEATURE_LEVEL = 79 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 5b88059edc..953488ccee 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -3650,12 +3650,9 @@ def get_subscribers_query(stream: Stream, requesting_user: Optional[UserProfile] return get_active_subscriptions_for_stream_id(stream.id, include_deactivated_users=False) -def get_subscriber_emails( - stream: Stream, requesting_user: Optional[UserProfile] = None -) -> List[str]: +def get_subscriber_ids(stream: Stream, requesting_user: Optional[UserProfile] = None) -> List[str]: subscriptions_query = get_subscribers_query(stream, requesting_user) - subscriptions = subscriptions_query.values("user_profile__email") - return [subscription["user_profile__email"] for subscription in subscriptions] + return subscriptions_query.values_list("user_profile_id", flat=True) def send_subscription_add_events( @@ -6630,26 +6627,8 @@ def gather_subscriptions( user_profile, include_subscribers=include_subscribers, ) - subscribed = helper_result.subscriptions unsubscribed = helper_result.unsubscribed - - if include_subscribers: - user_ids = set() - for subs in [subscribed, unsubscribed]: - for sub in subs: - if "subscribers" in sub: - for subscriber in sub["subscribers"]: - user_ids.add(subscriber) - email_dict = get_emails_from_user_ids(list(user_ids)) - - for subs in [subscribed, unsubscribed]: - for sub in subs: - if "subscribers" in sub: - sub["subscribers"] = sorted( - email_dict[user_id] for user_id in sub["subscribers"] - ) - return (subscribed, unsubscribed) diff --git a/zerver/openapi/python_examples.py b/zerver/openapi/python_examples.py index ca6bdc427d..1bf726800f 100644 --- a/zerver/openapi/python_examples.py +++ b/zerver/openapi/python_examples.py @@ -561,10 +561,16 @@ def test_user_not_authorized_error(nonadmin_client: Client) -> None: validate_against_openapi_schema(result, "/rest-error-handling", "post", "400_2") +@openapi_test_function("/streams/{stream_id}/members:get") def get_subscribers(client: Client) -> None: + ensure_users([11, 26], ["iago", "newbie"]) + # {code_example|start} + # Get the subscribers to a stream result = client.get_subscribers(stream="new stream") - assert result["subscribers"] == ["iago@zulip.com", "newbie@zulip.com"] + print(result) + # {code_example|end} + assert result["subscribers"] == [11, 26] def get_user_agent(client: Client) -> None: diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 1ebc5b751c..c2c66535be 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -2850,6 +2850,29 @@ class SubscriptionAPITest(ZulipTestCase): # also check that this matches the list of your subscriptions self.assertEqual(sorted(list_streams), sorted(self.streams)) + def test_successful_subscriptions_list_subscribers(self) -> None: + """ + Calling /api/v1/users/me/subscriptions should successfully return your subscriptions. + """ + result = self.api_get( + self.test_user, + "/api/v1/users/me/subscriptions", + {"include_subscribers": "true"}, + ) + self.assert_json_success(result) + json = result.json() + self.assertIn("subscriptions", json) + for stream in json["subscriptions"]: + self.assertIsInstance(stream["name"], str) + self.assertIsInstance(stream["color"], str) + self.assertIsInstance(stream["invite_only"], bool) + # check that the stream name corresponds to an actual + # stream; will throw Stream.DoesNotExist if it doesn't + get_stream(stream["name"], self.test_realm) + list_streams = [stream["name"] for stream in json["subscriptions"]] + # also check that this matches the list of your subscriptions + self.assertEqual(sorted(list_streams), sorted(self.streams)) + def helper_check_subs_before_and_after_add( self, subscriptions: List[str], @@ -4531,8 +4554,8 @@ class InviteOnlyStreamTest(ZulipTestCase): self.assert_json_success(result) json = result.json() - self.assertTrue(othello.email in json["subscribers"]) - self.assertTrue(hamlet.email in json["subscribers"]) + self.assertTrue(othello.id in json["subscribers"]) + self.assertTrue(hamlet.id in json["subscribers"]) class GetSubscribersTest(ZulipTestCase): @@ -4561,13 +4584,12 @@ class GetSubscribersTest(ZulipTestCase): {"msg": "", "result": "success", - "subscribers": [self.example_email("hamlet"), self.example_email("prospero")]} + "subscribers": [hamlet_user.id, prospero_user.id]} """ self.assertIn("subscribers", result) self.assertIsInstance(result["subscribers"], list) true_subscribers = [ - user_profile.email - for user_profile in self.users_subscribed_to_stream(stream_name, realm) + user_profile.id for user_profile in self.users_subscribed_to_stream(stream_name, realm) ] self.assertEqual(sorted(result["subscribers"]), sorted(true_subscribers)) @@ -4659,7 +4681,7 @@ class GetSubscribersTest(ZulipTestCase): if not sub["name"].startswith("stream_"): continue self.assert_length(sub["subscribers"], len(users_to_subscribe)) - self.assert_length(queries, 5) + self.assert_length(queries, 4) def test_never_subscribed_streams(self) -> None: """ @@ -4888,7 +4910,7 @@ class GetSubscribersTest(ZulipTestCase): self.assert_length(sub["subscribers"], len(users_to_subscribe)) else: self.assert_length(sub["subscribers"], 0) - self.assert_length(queries, 5) + self.assert_length(queries, 4) def test_nonsubscriber(self) -> None: """ @@ -4946,9 +4968,9 @@ class GetSubscribersTest(ZulipTestCase): result_dict = result.json() self.assertIn("subscribers", result_dict) self.assertIsInstance(result_dict["subscribers"], list) - subscribers: List[str] = [] + subscribers: List[int] = [] for subscriber in result_dict["subscribers"]: - self.assertIsInstance(subscriber, str) + self.assertIsInstance(subscriber, int) subscribers.append(subscriber) self.assertEqual(set(subscribers), set(expected_subscribers)) diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 5389d1c3d4..a5b11b8f3c 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -40,7 +40,7 @@ from zerver.lib.actions import ( do_send_messages, gather_subscriptions, get_default_streams_for_realm, - get_subscriber_emails, + get_subscriber_ids, internal_prep_private_message, internal_prep_stream_message, ) @@ -676,9 +676,9 @@ def get_subscribers_backend( stream_id, allow_realm_admin=True, ) - subscribers = get_subscriber_emails(stream, user_profile) + subscribers = get_subscriber_ids(stream, user_profile) - return json_success({"subscribers": subscribers}) + return json_success({"subscribers": list(subscribers)}) # By default, lists all streams that the user has access to --