From e087bc24f83c0e1a55dad3de74586886542afcd0 Mon Sep 17 00:00:00 2001 From: kunall17 Date: Tue, 9 May 2017 10:31:42 +0530 Subject: [PATCH] streams: Migrate stream property changes to new REST endpoint. This is one of the last major endpoints that were still done in the pre-REST style. While we're at it, we change the endpoint to expect a stream ID, not a stream name. --- static/js/stream_edit.js | 5 +- zerver/tests/test_subs.py | 117 ++++++++++++++++++-------------------- zerver/tests/test_urls.py | 2 +- zerver/views/streams.py | 22 +++---- zproject/legacy_urls.py | 1 - zproject/urls.py | 4 ++ 6 files changed, 72 insertions(+), 79 deletions(-) diff --git a/static/js/stream_edit.js b/static/js/stream_edit.js index b0a94598a2..015d90f883 100644 --- a/static/js/stream_edit.js +++ b/static/js/stream_edit.js @@ -245,10 +245,9 @@ function stream_home_view_clicked(e) { exports.set_stream_property = function (sub, property, value) { // TODO: Fix backend so it takes a stream id. - var stream_name = sub.name; - var sub_data = {stream: stream_name, property: property, value: value}; + var sub_data = {stream_id: sub.stream_id, property: property, value: value}; return channel.post({ - url: '/json/subscriptions/property', + url: '/json/users/me/subscriptions/properties', data: {subscription_data: JSON.stringify([sub_data])}, timeout: 10*1000, }); diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index b5c6433b1e..91edeb09b2 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -797,7 +797,7 @@ class SubscriptionPropertiesTest(ZulipTestCase): def test_set_stream_color(self): # type: () -> None """ - A POST request to /json/subscriptions/property with stream_name and + A POST request to /api/v1/users/me/subscriptions/properties with stream_id and color data sets the stream color, and for that stream only. """ test_user = self.example_user('hamlet') @@ -806,20 +806,21 @@ class SubscriptionPropertiesTest(ZulipTestCase): old_subs, _ = gather_subscriptions(test_user) sub = old_subs[0] - stream_name = sub['name'] + stream_id = sub['stream_id'] new_color = "#ffffff" # TODO: ensure that this is different from old_color result = self.client_post( - "/json/subscriptions/property", + "/api/v1/users/me/subscriptions/properties", {"subscription_data": ujson.dumps([{"property": "color", - "stream": stream_name, - "value": "#ffffff"}])}) + "stream_id": stream_id, + "value": "#ffffff"}])}, + **self.api_auth(test_email)) self.assert_json_success(result) new_subs = gather_subscriptions(get_user_profile_by_email(test_email))[0] found_sub = None for sub in new_subs: - if sub['name'] == stream_name: + if sub['stream_id'] == stream_id: found_sub = sub break @@ -828,28 +829,30 @@ class SubscriptionPropertiesTest(ZulipTestCase): new_subs.remove(found_sub) for sub in old_subs: - if sub['name'] == stream_name: + if sub['stream_id'] == stream_id: found_sub = sub break old_subs.remove(found_sub) self.assertEqual(old_subs, new_subs) - def test_set_color_missing_stream_name(self): + def test_set_color_missing_stream_id(self): # type: () -> None """ - Updating the color property requires a `stream` key. + Updating the color property requires a `stream_id` key. """ - test_email = "hamlet@zulip.com" + test_user = self.example_user('hamlet') + test_email = test_user.email self.login(test_email) result = self.client_post( - "/json/subscriptions/property", + "/api/v1/users/me/subscriptions/properties", {"subscription_data": ujson.dumps([{"property": "color", - "value": "#ffffff"}])}) + "value": "#ffffff"}])}, + **self.api_auth(test_email)) self.assert_json_error( - result, "stream key is missing from subscription_data[0]") + result, "stream_id key is missing from subscription_data[0]") - def test_set_color_unsubscribed_stream_name(self): + def test_set_color_unsubscribed_stream_id(self): # type: () -> None """ Updating the color property requires a subscribed stream. @@ -857,32 +860,17 @@ class SubscriptionPropertiesTest(ZulipTestCase): test_email = "hamlet@zulip.com" self.login(test_email) - unsubs_stream = 'Rome' + subscribed, unsubscribed, never_subscribed = gather_subscriptions_helper( + get_user_profile_by_email(test_email)) + not_subbed = unsubscribed + never_subscribed result = self.client_post( - "/json/subscriptions/property", + "/api/v1/users/me/subscriptions/properties", {"subscription_data": ujson.dumps([{"property": "color", - "stream": unsubs_stream, - "value": "#ffffff"}])}) + "stream_id": not_subbed[0]["stream_id"], + "value": "#ffffff"}])}, + **self.api_auth(test_email)) self.assert_json_error( - result, "Not subscribed to stream %s" % (unsubs_stream,)) - - def test_json_subscription_property_invalid_verb(self): - # type: () -> None - """ - Called by invalid request method. No other request method other than - 'post' is allowed in this case. - """ - test_user = self.example_user('hamlet') - test_email = test_user.email - self.login(test_email) - subs = gather_subscriptions(test_user)[0] - - result = self.client_get( - "/json/subscriptions/property", - {"subscription_data": ujson.dumps([{"property": "in_home_view", - "stream": subs[0]["name"], - "value": False}])}) - self.assert_json_error(result, "Invalid verb") + result, "Not subscribed to stream id %d" % (not_subbed[0]["stream_id"],)) def test_set_color_missing_color(self): # type: () -> None @@ -894,9 +882,10 @@ class SubscriptionPropertiesTest(ZulipTestCase): self.login(test_email) subs = gather_subscriptions(test_user)[0] result = self.client_post( - "/json/subscriptions/property", + "/api/v1/users/me/subscriptions/properties", {"subscription_data": ujson.dumps([{"property": "color", - "stream": subs[0]["name"]}])}) + "stream_id": subs[0]["stream_id"]}])}, + **self.api_auth(test_email)) self.assert_json_error( result, "value key is missing from subscription_data[0]") @@ -904,7 +893,7 @@ class SubscriptionPropertiesTest(ZulipTestCase): def test_set_pin_to_top(self): # type: () -> None """ - A POST request to /json/subscriptions/property with stream_name and + A POST request to /api/v1/users/me/subscriptions/properties with stream_id and pin_to_top data pins the stream. """ user_profile = self.example_user('hamlet') @@ -913,17 +902,18 @@ class SubscriptionPropertiesTest(ZulipTestCase): old_subs, _ = gather_subscriptions(user_profile) sub = old_subs[0] - stream_name = sub['name'] + stream_id = sub['stream_id'] new_pin_to_top = not sub['pin_to_top'] result = self.client_post( - "/json/subscriptions/property", + "/api/v1/users/me/subscriptions/properties", {"subscription_data": ujson.dumps([{"property": "pin_to_top", - "stream": stream_name, - "value": new_pin_to_top}])}) + "stream_id": stream_id, + "value": new_pin_to_top}])}, + **self.api_auth(test_email)) self.assert_json_success(result) - updated_sub = get_subscription(stream_name, user_profile) + updated_sub = get_subscription(sub['name'], user_profile) self.assertIsNotNone(updated_sub) self.assertEqual(updated_sub.pin_to_top, new_pin_to_top) @@ -940,40 +930,44 @@ class SubscriptionPropertiesTest(ZulipTestCase): property_name = "in_home_view" result = self.client_post( - "/json/subscriptions/property", + "/api/v1/users/me/subscriptions/properties", {"subscription_data": ujson.dumps([{"property": property_name, "value": "bad", - "stream": subs[0]["name"]}])}) + "stream_id": subs[0]["stream_id"]}])}, + **self.api_auth(test_email)) self.assert_json_error(result, '%s is not a boolean' % (property_name,)) property_name = "desktop_notifications" result = self.client_post( - "/json/subscriptions/property", + "/api/v1/users/me/subscriptions/properties", {"subscription_data": ujson.dumps([{"property": property_name, "value": "bad", - "stream": subs[0]["name"]}])}) + "stream_id": subs[0]["stream_id"]}])}, + **self.api_auth(test_email)) self.assert_json_error(result, '%s is not a boolean' % (property_name,)) property_name = "audible_notifications" result = self.client_post( - "/json/subscriptions/property", + "/api/v1/users/me/subscriptions/properties", {"subscription_data": ujson.dumps([{"property": property_name, "value": "bad", - "stream": subs[0]["name"]}])}) + "stream_id": subs[0]["stream_id"]}])}, + **self.api_auth(test_email)) self.assert_json_error(result, '%s is not a boolean' % (property_name,)) property_name = "color" result = self.client_post( - "/json/subscriptions/property", + "/api/v1/users/me/subscriptions/properties", {"subscription_data": ujson.dumps([{"property": property_name, "value": False, - "stream": subs[0]["name"]}])}) + "stream_id": subs[0]["stream_id"]}])}, + **self.api_auth(test_email)) self.assert_json_error(result, '%s is not a string' % (property_name,)) @@ -983,14 +977,14 @@ class SubscriptionPropertiesTest(ZulipTestCase): test_email = "hamlet@zulip.com" self.login(test_email) - stream_name = "invalid_stream" + stream_id = 1000 result = self.client_post( - "/json/subscriptions/property", + "/api/v1/users/me/subscriptions/properties", {"subscription_data": ujson.dumps([{"property": "in_home_view", - "stream": stream_name, - "value": False}])}) - - self.assert_json_error(result, "Invalid stream name '%s'" % (stream_name,)) + "stream_id": stream_id, + "value": False}])}, + **self.api_auth(test_email)) + self.assert_json_error(result, "Invalid stream id") def test_set_invalid_property(self): # type: () -> None @@ -1002,10 +996,11 @@ class SubscriptionPropertiesTest(ZulipTestCase): self.login(test_email) subs = gather_subscriptions(test_user)[0] result = self.client_post( - "/json/subscriptions/property", + "/api/v1/users/me/subscriptions/properties", {"subscription_data": ujson.dumps([{"property": "bad", "value": "bad", - "stream": subs[0]["name"]}])}) + "stream_id": subs[0]["stream_id"]}])}, + **self.api_auth(test_email)) self.assert_json_error(result, "Unknown subscription property: bad") diff --git a/zerver/tests/test_urls.py b/zerver/tests/test_urls.py index 74659fd19f..3ea559bd9e 100644 --- a/zerver/tests/test_urls.py +++ b/zerver/tests/test_urls.py @@ -67,7 +67,7 @@ class PublicURLTest(ZulipTestCase): "/json/invite_users", "/json/settings/change", "/json/subscriptions/exists", - "/json/subscriptions/property", + "/api/v1/users/me/subscriptions/properties", "/json/fetch_api_key", "/json/users/me/pointer", "/json/users/me/subscriptions", diff --git a/zerver/views/streams.py b/zerver/views/streams.py index c21279671b..9d9c3d8d68 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -23,7 +23,7 @@ from zerver.lib.response import json_success, json_error, json_response from zerver.lib.streams import access_stream_by_id, access_stream_by_name, \ check_stream_name, check_stream_name_available, filter_stream_authorization, \ list_to_streams -from zerver.lib.validator import check_string, check_list, check_dict, \ +from zerver.lib.validator import check_string, check_int, check_list, check_dict, \ check_bool, check_variable_type from zerver.models import UserProfile, Stream, Realm, Subscription, \ Recipient, get_recipient, get_stream, get_active_user_dicts_in_realm @@ -410,11 +410,10 @@ def json_get_stream_id(request, user_profile, stream_name=REQ('stream')): (stream, recipient, sub) = access_stream_by_name(user_profile, stream_name) return json_success({'stream_id': stream.id}) -@authenticated_json_view @has_request_variables -def json_subscription_property(request, user_profile, subscription_data=REQ( +def update_subscription_properties_backend(request, user_profile, subscription_data=REQ( validator=check_list( - check_dict([("stream", check_string), + check_dict([("stream_id", check_int), ("property", check_string), ("value", check_variable_type( [check_string, check_bool]))])))): @@ -426,12 +425,9 @@ def json_subscription_property(request, user_profile, subscription_data=REQ( Requests are of the form: - [{"stream": "devel", "property": "in_home_view", "value": False}, - {"stream": "devel", "property": "color", "value": "#c2c2c2"}] + [{"stream_id": "1", "property": "in_home_view", "value": False}, + {"stream_id": "1", "property": "color", "value": "#c2c2c2"}] """ - if request.method != "POST": - return json_error(_("Invalid verb")) - property_converters = {"color": check_string, "in_home_view": check_bool, "desktop_notifications": check_bool, "audible_notifications": check_bool, @@ -439,16 +435,16 @@ def json_subscription_property(request, user_profile, subscription_data=REQ( response_data = [] for change in subscription_data: - stream_name = change["stream"] + stream_id = change["stream_id"] property = change["property"] value = change["value"] if property not in property_converters: return json_error(_("Unknown subscription property: %s") % (property,)) - (stream, recipient, sub) = access_stream_by_name(user_profile, stream_name) + (stream, recipient, sub) = access_stream_by_id(user_profile, stream_id) if sub is None: - return json_error(_("Not subscribed to stream %s") % (stream_name,)) + return json_error(_("Not subscribed to stream id %d") % (stream_id,)) property_conversion = property_converters[property](property, value) if property_conversion: @@ -457,7 +453,7 @@ def json_subscription_property(request, user_profile, subscription_data=REQ( do_change_subscription_property(user_profile, sub, stream, property, value) - response_data.append({'stream': stream_name, + response_data.append({'stream_id': stream_id, 'property': property, 'value': value}) diff --git a/zproject/legacy_urls.py b/zproject/legacy_urls.py index 78a6a70a6d..373f336a76 100644 --- a/zproject/legacy_urls.py +++ b/zproject/legacy_urls.py @@ -25,7 +25,6 @@ legacy_urls = [ # pushed to us via the event system. url(r'^json/subscriptions/exists$', zerver.views.streams.json_stream_exists), - url(r'^json/subscriptions/property$', zerver.views.streams.json_subscription_property), url(r'^json/fetch_api_key$', zerver.views.auth.json_fetch_api_key), url(r'^json/tutorial_send_message$', zerver.views.tutorial.json_tutorial_send_message), url(r'^json/tutorial_status$', zerver.views.tutorial.json_tutorial_status), diff --git a/zproject/urls.py b/zproject/urls.py index 687605bd7c..9ef2b46139 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -28,6 +28,7 @@ import zerver.views.unsubscribe import zerver.views.integrations import zerver.views.user_settings import zerver.views.muting +import zerver.views.streams import confirmation.views from zerver.lib.rest import rest_dispatch @@ -251,6 +252,9 @@ v1_api_and_json_patterns = [ url(r'^messages/(?P\d+)/history$', rest_dispatch, {'GET': 'zerver.views.messages.get_message_edit_history'}), + url(r'^users/me/subscriptions/properties$', rest_dispatch, + {'POST': 'zerver.views.streams.update_subscription_properties_backend'}), + # reactions -> zerver.view.reactions # PUT adds a reaction to a message # DELETE removes a reaction from a message