From 65893292b5d07deab8c118168ce87ab840e0cc5e Mon Sep 17 00:00:00 2001 From: joseph Date: Sat, 3 Aug 2024 17:41:35 +0000 Subject: [PATCH] channel_subscribe: Use IDs instead of emails when processing results. As part of our todo in the code, we want to use the unique user IDs instead of emails when processing the results of subscribing users to a channel. These changes apply those changes and streamlines the use of IDs. --- api_docs/changelog.md | 6 +++++ version.py | 2 +- web/src/stream_edit_subscribers.ts | 8 +++---- zerver/openapi/python_examples.py | 2 +- zerver/openapi/zulip.yaml | 28 ++++++++++++++--------- zerver/tests/test_subs.py | 36 +++++++++++++++++------------- zerver/views/streams.py | 26 ++++++++++----------- 7 files changed, 63 insertions(+), 45 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 9517b8ce82..f67c8da02c 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,12 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 10.0 +**Feature level 289** + +* [`POST /users/{user_id}/subscription`](/api/subscribe): In the response, + users are identified by their numeric user ID rather than by their + Zulip API email address. + **Feature level 288** * [`POST /register`](/api/register-queue): diff --git a/version.py b/version.py index c41b8f6bbf..95b21659f6 100644 --- a/version.py +++ b/version.py @@ -34,7 +34,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 288 # Last bumped for presence_history_limit_days +API_FEATURE_LEVEL = 289 # return ID as key while subscribing to a stream. # 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/web/src/stream_edit_subscribers.ts b/web/src/stream_edit_subscribers.ts index b38f69b3ef..cba3fd1a5a 100644 --- a/web/src/stream_edit_subscribers.ts +++ b/web/src/stream_edit_subscribers.ts @@ -226,11 +226,11 @@ function subscribe_new_users({pill_user_ids}: {pill_user_ids: number[]}): void { function invite_success(raw_data: unknown): void { const data = add_user_ids_api_response_schema.parse(raw_data); pill_widget.clear(); - const subscribed_users = Object.keys(data.subscribed).map( - (email) => people.get_by_email(email)!, + const subscribed_users = Object.keys(data.subscribed).map((user_id) => + people.get_by_user_id(Number(user_id)), ); - const already_subscribed_users = Object.keys(data.already_subscribed).map( - (email) => people.get_by_email(email)!, + const already_subscribed_users = Object.keys(data.already_subscribed).map((user_id) => + people.get_by_user_id(Number(user_id)), ); show_stream_subscription_request_result({ diff --git a/zerver/openapi/python_examples.py b/zerver/openapi/python_examples.py index cca31a7580..479c0218f1 100644 --- a/zerver/openapi/python_examples.py +++ b/zerver/openapi/python_examples.py @@ -129,7 +129,7 @@ def add_subscriptions(client: Client) -> None: # {code_example|end} assert_success_response(result) validate_against_openapi_schema(result, "/users/me/subscriptions", "post", "200") - assert "newbie@zulip.com" in result["subscribed"] + assert str(user_id) in result["subscribed"] def test_add_subscriptions_already_subscribed(client: Client) -> None: diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 833f28cba8..b25f9a4650 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -9916,12 +9916,15 @@ paths: subscribed: type: object description: | - A dictionary where the key is the Zulip API email address of the user/bot - and the value is a list of the names of the channels that were subscribed - to as a result of the query. + A dictionary where the key is the ID of the user and the value + is a list of the names of the channels that user was subscribed + to as a result of the request. + + **Changes**: Before Zulip 10.0 (feature level 289), the user + keys were Zulip API email addresses, not user ID. additionalProperties: description: | - `{email_address}`: List of the names of the channels that were subscribed + `{id}`: List of the names of the channels that were subscribed to as a result of the query. type: array items: @@ -9929,12 +9932,16 @@ paths: already_subscribed: type: object description: | - A dictionary where the key is the Zulip API email address of the user/bot - and the value is a list of the names of the channels that the user/bot is - already subscribed to. + A dictionary where the key is the ID of the user and the value + is a list of the names of the channels that where the user was + not added as a subscriber in this request, because they were + already a subscriber. + + **Changes**: Before Zulip 10.0 (feature level 289), the user + keys were Zulip API email addresses, not user IDs. additionalProperties: description: | - `{email_address}`: List of the names of the channels that the user is + `{id}`: List of the names of the channels that the user is already subscribed to. type: array items: @@ -9948,11 +9955,10 @@ paths: authorized to subscribe to. Only present if `"authorization_errors_fatal": false`. example: { - "already_subscribed": - {"iago@zulip.com": ["testing-help"]}, + "already_subscribed": {"1": ["testing-help"]}, "msg": "", "result": "success", - "subscribed": {"newbie@zulip.com": ["testing-help"]}, + "subscribed": {"2": ["testing-help"]}, } "400": description: Bad request. diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 9c52cfc540..72f017e3ac 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -495,10 +495,10 @@ class TestCreateStreams(ZulipTestCase): "result": "success", "msg": "", "subscribed": { - "AARON@zulip.com": ["brand new stream"], - "cordelia@zulip.com": ["brand new stream"], - "hamlet@zulip.com": ["brand new stream"], - "iago@zulip.com": ["brand new stream"], + "10": ["brand new stream"], + "11": ["brand new stream"], + "6": ["brand new stream"], + "8": ["brand new stream"], }, "already_subscribed": {}, } @@ -4062,6 +4062,7 @@ class SubscriptionAPITest(ZulipTestCase): self.user_profile = self.example_user("hamlet") self.test_email = self.user_profile.email self.test_user = self.user_profile + self.test_id = str(self.user_profile.id) self.login_user(self.user_profile) self.test_realm = self.user_profile.realm self.streams = self.get_streams(self.user_profile) @@ -4170,29 +4171,30 @@ class SubscriptionAPITest(ZulipTestCase): email: str, new_subs: list[str], realm: Realm, + id: str, invite_only: bool = False, ) -> None: """ Check result of adding subscriptions. You can add subscriptions for yourself or possibly many - principals, which is why e-mails map to subscriptions in the + principals, which is why user ID map to subscriptions in the result. The result json is of the form {"msg": "", "result": "success", - "already_subscribed": {self.example_email("iago"): ["Venice", "Verona"]}, - "subscribed": {self.example_email("iago"): ["Venice8"]}} + "already_subscribed": {self.example_user("iago").id: ["Venice", "Verona"]}, + "subscribed": {self.example_user("iago").id: ["Venice8"]}} """ result = self.common_subscribe_to_streams( self.test_user, subscriptions, other_params, invite_only=invite_only ) json = self.assert_json_success(result) - self.assertEqual(sorted(subscribed), sorted(json["subscribed"][email])) - self.assertEqual(sorted(already_subscribed), sorted(json["already_subscribed"][email])) - user = get_user(email, realm) + self.assertEqual(sorted(subscribed), sorted(json["subscribed"][id])) + self.assertEqual(sorted(already_subscribed), sorted(json["already_subscribed"][id])) + user = get_user_profile_by_id_in_realm(int(id), realm) new_streams = self.get_streams(user) self.assertEqual(sorted(new_streams), sorted(new_subs)) @@ -4217,6 +4219,7 @@ class SubscriptionAPITest(ZulipTestCase): self.test_email, self.streams + add_streams, self.test_realm, + self.test_id, ) def test_successful_subscriptions_add_with_announce(self) -> None: @@ -4246,6 +4249,7 @@ class SubscriptionAPITest(ZulipTestCase): self.test_email, self.streams + add_streams, self.test_realm, + self.test_id, ) expected_stream_ids = {get_stream(stream, self.test_realm).id for stream in add_streams} @@ -4392,6 +4396,7 @@ class SubscriptionAPITest(ZulipTestCase): self.test_email, [*self.streams, "hümbüǵ"], self.test_realm, + self.test_id, ) def test_subscriptions_add_too_long(self) -> None: @@ -4703,6 +4708,7 @@ class SubscriptionAPITest(ZulipTestCase): other_profile.email, streams_to_sub, invitee_realm, + str(other_profile.id), invite_only=invite_only, ) @@ -4875,7 +4881,7 @@ class SubscriptionAPITest(ZulipTestCase): do_change_stream_post_policy(stream, Stream.STREAM_POST_POLICY_ADMINS, acting_user=member) result = self.common_subscribe_to_streams(member, ["stream1"]) json = self.assert_json_success(result) - self.assertEqual(json["subscribed"], {member.email: ["stream1"]}) + self.assertEqual(json["subscribed"], {str(member.id): ["stream1"]}) self.assertEqual(json["already_subscribed"], {}) def test_subscribe_to_stream_post_policy_restrict_new_members_stream(self) -> None: @@ -4895,7 +4901,7 @@ class SubscriptionAPITest(ZulipTestCase): ) result = self.common_subscribe_to_streams(new_member, ["stream1"]) json = self.assert_json_success(result) - self.assertEqual(json["subscribed"], {new_member.email: ["stream1"]}) + self.assertEqual(json["subscribed"], {str(new_member.id): ["stream1"]}) self.assertEqual(json["already_subscribed"], {}) def test_subscribe_to_stream_post_policy_moderators_stream(self) -> None: @@ -4912,7 +4918,7 @@ class SubscriptionAPITest(ZulipTestCase): ) result = self.common_subscribe_to_streams(member, ["stream1"]) json = self.assert_json_success(result) - self.assertEqual(json["subscribed"], {member.email: ["stream1"]}) + self.assertEqual(json["subscribed"], {str(member.id): ["stream1"]}) self.assertEqual(json["already_subscribed"], {}) def test_guest_user_subscribe(self) -> None: @@ -5897,7 +5903,7 @@ class InviteOnlyStreamTest(ZulipTestCase): result = self.common_subscribe_to_streams(hamlet, [stream_name], invite_only=True) json = self.assert_json_success(result) - self.assertEqual(json["subscribed"], {hamlet.email: [stream_name]}) + self.assertEqual(json["subscribed"], {str(hamlet.id): [stream_name]}) self.assertEqual(json["already_subscribed"], {}) # Subscribing oneself to an invite-only stream is not allowed @@ -5925,7 +5931,7 @@ class InviteOnlyStreamTest(ZulipTestCase): extra_post_data={"principals": orjson.dumps([othello.id]).decode()}, ) json = self.assert_json_success(result) - self.assertEqual(json["subscribed"], {othello.email: [stream_name]}) + self.assertEqual(json["subscribed"], {str(othello.id): [stream_name]}) self.assertEqual(json["already_subscribed"], {}) # Make sure both users are subscribed to this stream diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 9d83a8ef28..70a0f02819 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -664,9 +664,7 @@ def add_subscriptions_backend( realm, streams, subscribers, acting_user=user_profile, color_map=color_map ) - # We can assume unique emails here for now, but we should eventually - # convert this function to be more id-centric. - email_to_user_profile: dict[str, UserProfile] = {} + id_to_user_profile: dict[str, UserProfile] = {} result: dict[str, Any] = dict( subscribed=defaultdict(list), already_subscribed=defaultdict(list) @@ -674,12 +672,14 @@ def add_subscriptions_backend( for sub_info in subscribed: subscriber = sub_info.user stream = sub_info.stream - result["subscribed"][subscriber.email].append(stream.name) - email_to_user_profile[subscriber.email] = subscriber + user_id = str(subscriber.id) + result["subscribed"][user_id].append(stream.name) + id_to_user_profile[user_id] = subscriber for sub_info in already_subscribed: subscriber = sub_info.user stream = sub_info.stream - result["already_subscribed"][subscriber.email].append(stream.name) + user_id = str(subscriber.id) + result["already_subscribed"][user_id].append(stream.name) result["subscribed"] = dict(result["subscribed"]) result["already_subscribed"] = dict(result["already_subscribed"]) @@ -688,7 +688,7 @@ def add_subscriptions_backend( user_profile=user_profile, subscribers=subscribers, new_subscriptions=result["subscribed"], - email_to_user_profile=email_to_user_profile, + id_to_user_profile=id_to_user_profile, created_streams=created_streams, announce=announce, ) @@ -704,7 +704,7 @@ def send_messages_for_new_subscribers( user_profile: UserProfile, subscribers: set[UserProfile], new_subscriptions: dict[str, list[str]], - email_to_user_profile: dict[str, UserProfile], + id_to_user_profile: dict[str, UserProfile], created_streams: list[Stream], announce: bool, ) -> None: @@ -716,7 +716,7 @@ def send_messages_for_new_subscribers( excessive query counts by mocking this function so that it doesn't drown out query counts from other code. """ - bots = {subscriber.email: subscriber.is_bot for subscriber in subscribers} + bots = {str(subscriber.id): subscriber.is_bot for subscriber in subscribers} newly_created_stream_names = {s.name for s in created_streams} @@ -727,11 +727,11 @@ def send_messages_for_new_subscribers( # or if a new stream was created with the "announce" option. notifications = [] if new_subscriptions: - for email, subscribed_stream_names in new_subscriptions.items(): - if email == user_profile.email: + for id, subscribed_stream_names in new_subscriptions.items(): + if id == str(user_profile.id): # Don't send a Zulip if you invited yourself. continue - if bots[email]: + if bots[id]: # Don't send invitation Zulips to bots continue @@ -742,7 +742,7 @@ def send_messages_for_new_subscribers( if not notify_stream_names: continue - recipient_user = email_to_user_profile[email] + recipient_user = id_to_user_profile[id] sender = get_system_bot(settings.NOTIFICATION_BOT, recipient_user.realm_id) msg = you_were_just_subscribed_message(