diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index d23ba5b811..604861f082 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -2947,44 +2947,32 @@ def do_create_default_stream_group(realm: Realm, group_name: Text, streams: List group.save() notify_default_stream_groups(realm) -def do_add_streams_to_default_stream_group(realm: Realm, group_name: Text, streams: List[Stream]) -> None: - try: - group = DefaultStreamGroup.objects.get(name=group_name, realm=realm) - except DefaultStreamGroup.DoesNotExist: - raise JsonableError(_("Default stream group '%s' does not exist") % (group_name,)) - +def do_add_streams_to_default_stream_group(realm: Realm, group: DefaultStreamGroup, streams: List[Stream]) -> None: default_streams = get_default_streams_for_realm(realm.id) for stream in streams: if stream in default_streams: - raise JsonableError(_("'%s' is a default stream and cannot be added to '%s'") % (stream.name, group_name)) + raise JsonableError(_("'%s' is a default stream and cannot be added to '%s'") % (stream.name, group.name)) if stream in group.streams.all(): raise JsonableError(_("Stream '%s' is already present in default stream group '%s'") - % (stream.name, group_name)) + % (stream.name, group.name)) group.streams.add(stream) group.save() notify_default_stream_groups(realm) -def do_remove_streams_from_default_stream_group(realm: Realm, group_name: Text, streams: List[Stream]) -> None: - try: - group = DefaultStreamGroup.objects.get(name=group_name, realm=realm) - except DefaultStreamGroup.DoesNotExist: - raise JsonableError(_("Default stream group '%s' does not exist") % (group_name,)) - +def do_remove_streams_from_default_stream_group(realm: Realm, group: DefaultStreamGroup, + streams: List[Stream]) -> None: for stream in streams: if stream not in group.streams.all(): raise JsonableError(_("Stream '%s' is not present in default stream group '%s'") - % (stream.name, group_name)) + % (stream.name, group.name)) group.streams.remove(stream) group.save() notify_default_stream_groups(realm) -def do_remove_default_stream_group(realm: Realm, group_name: Text) -> None: - try: - DefaultStreamGroup.objects.filter(name=group_name, realm=realm).delete() - except DefaultStreamGroup.DoesNotExist: - raise JsonableError(_("Default stream group '%s' does not exist") % (group_name,)) +def do_remove_default_stream_group(realm: Realm, group: DefaultStreamGroup) -> None: + group.delete() notify_default_stream_groups(realm) def get_default_streams_for_realm(realm_id): diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py index abfb604e02..7d4c72c751 100644 --- a/zerver/lib/streams.py +++ b/zerver/lib/streams.py @@ -8,7 +8,7 @@ from zerver.lib.actions import check_stream_name, create_streams_if_needed from zerver.lib.request import JsonableError from zerver.models import UserProfile, Stream, Subscription, \ Realm, Recipient, bulk_get_recipients, get_stream_recipient, get_stream, \ - bulk_get_streams, get_realm_stream + bulk_get_streams, get_realm_stream, DefaultStreamGroup def access_stream_for_delete(user_profile, stream_id): # type: (UserProfile, int) -> Stream @@ -220,3 +220,9 @@ def list_to_streams(streams_raw, user_profile, autocreate=False): existing_streams += dup_streams return existing_streams, created_streams + +def access_default_stream_group_by_id(realm: Realm, group_id: int) -> DefaultStreamGroup: + try: + return DefaultStreamGroup.objects.get(realm=realm, id=group_id) + except DefaultStreamGroup.DoesNotExist: + raise JsonableError(_("Default stream group with id '%s' does not exist." % (group_id,))) diff --git a/zerver/models.py b/zerver/models.py index e6ee8841a6..854812f421 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -1810,7 +1810,9 @@ class DefaultStreamGroup(models.Model): def to_dict(self): # type: () -> Dict[str, Any] - return dict(name=self.name, streams=[stream.to_dict() for stream in self.streams.all()]) + return dict(name=self.name, + id=self.id, + streams=[stream.to_dict() for stream in self.streams.all()]) def get_default_stream_groups(realm): # type: (Realm) -> List[DefaultStreamGroup] diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 1ec1673b77..042eb3789a 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -71,6 +71,7 @@ from zerver.lib.actions import ( do_update_pointer, do_update_user_presence, log_event, + lookup_default_stream_groups, notify_realm_custom_profile_fields, ) from zerver.lib.events import ( @@ -989,6 +990,7 @@ class EventsRegisterTest(ZulipTestCase): ('type', equals('default_stream_groups')), ('default_stream_groups', check_list(check_dict_only([ ('name', check_string), + ('id', check_int), ('streams', check_list(check_dict_only([ ('description', check_string), ('invite_only', check_bool), @@ -1006,19 +1008,19 @@ class EventsRegisterTest(ZulipTestCase): error = default_stream_groups_checker('events[0]', events[0]) self.assert_on_error(error) + group = lookup_default_stream_groups(["group1"], self.user_profile.realm)[0] venice_stream = get_stream("Venice", self.user_profile.realm) events = self.do_test(lambda: do_add_streams_to_default_stream_group(self.user_profile.realm, - "group1", [venice_stream])) + group, [venice_stream])) error = default_stream_groups_checker('events[0]', events[0]) self.assert_on_error(error) events = self.do_test(lambda: do_remove_streams_from_default_stream_group(self.user_profile.realm, - "group1", [venice_stream])) + group, [venice_stream])) error = default_stream_groups_checker('events[0]', events[0]) self.assert_on_error(error) - events = self.do_test(lambda: do_remove_default_stream_group(self.user_profile.realm, - "group1")) + events = self.do_test(lambda: do_remove_default_stream_group(self.user_profile.realm, group)) error = default_stream_groups_checker('events[0]', events[0]) self.assert_on_error(error) diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index db5630989e..20c032dbf5 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -56,6 +56,7 @@ from zerver.lib.actions import ( do_create_default_stream_group, do_add_streams_to_default_stream_group, do_remove_streams_from_default_stream_group, do_remove_default_stream_group, + lookup_default_stream_groups, ) from zerver.views.streams import ( @@ -852,6 +853,7 @@ class DefaultStreamGroupTest(ZulipTestCase): self.assertEqual(list(default_stream_groups[0].streams.all()), streams) # Test adding streams to existing default stream group + group = lookup_default_stream_groups(["group1"], realm)[0] new_stream_names = ["stream4", "stream5"] new_streams = [] for new_stream_name in new_stream_names: @@ -859,14 +861,14 @@ class DefaultStreamGroupTest(ZulipTestCase): new_streams.append(new_stream) streams.append(new_stream) - do_add_streams_to_default_stream_group(realm, group_name, new_streams) + do_add_streams_to_default_stream_group(realm, group, new_streams) default_stream_groups = get_default_stream_groups(realm) self.assert_length(default_stream_groups, 1) self.assertEqual(default_stream_groups[0].name, group_name) self.assertEqual(list(default_stream_groups[0].streams.all()), streams) # Test removing streams from existing default stream group - do_remove_streams_from_default_stream_group(realm, group_name, new_streams) + do_remove_streams_from_default_stream_group(realm, group, new_streams) remaining_streams = streams[0:3] default_stream_groups = get_default_stream_groups(realm) self.assert_length(default_stream_groups, 1) @@ -874,7 +876,7 @@ class DefaultStreamGroupTest(ZulipTestCase): self.assertEqual(list(default_stream_groups[0].streams.all()), remaining_streams) # Test removing default stream group - do_remove_default_stream_group(realm, group_name) + do_remove_default_stream_group(realm, group) default_stream_groups = get_default_stream_groups(realm) self.assert_length(default_stream_groups, 0) @@ -900,11 +902,7 @@ class DefaultStreamGroupTest(ZulipTestCase): (stream, _) = create_stream_if_needed(realm, stream_name) streams.append(stream) - result = self.client_post('/json/default_stream_groups', - {"group_name": "", "stream_names": ujson.dumps(stream_names)}) - self.assert_json_error(result, "Invalid default stream group name ''") - - result = self.client_post('/json/default_stream_groups', + result = self.client_post('/json/default_stream_groups/create', {"group_name": group_name, "stream_names": ujson.dumps(stream_names)}) self.assert_json_success(result) default_stream_groups = get_default_stream_groups(realm) @@ -913,6 +911,7 @@ class DefaultStreamGroupTest(ZulipTestCase): self.assertEqual(list(default_stream_groups[0].streams.all()), streams) # Test adding streams to existing default stream group + group_id = default_stream_groups[0].id new_stream_names = ["stream4", "stream5"] new_streams = [] for new_stream_name in new_stream_names: @@ -920,75 +919,69 @@ class DefaultStreamGroupTest(ZulipTestCase): new_streams.append(new_stream) streams.append(new_stream) - result = self.client_patch("/json/default_stream_groups", - {"group_name": group_name, - "stream_names": ujson.dumps(new_stream_names)}) + result = self.client_patch("/json/default_stream_groups/{}/streams".format(group_id), + {"stream_names": ujson.dumps(new_stream_names)}) self.assert_json_error(result, "Missing 'op' argument") - result = self.client_patch("/json/default_stream_groups", - {"group_name": group_name, "op": "invalid", - "stream_names": ujson.dumps(new_stream_names)}) - self.assert_json_error(result, 'Nothing to do. Specify at least one of "add" or "remove".') + result = self.client_patch("/json/default_stream_groups/{}/streams".format(group_id), + {"op": "invalid", "stream_names": ujson.dumps(new_stream_names)}) + self.assert_json_error(result, 'Invalid value for "op". Specify one of "add" or "remove".') - result = self.client_patch("/json/default_stream_groups", - {"group_name": "dumbledore's army", "op": "add", - "stream_names": ujson.dumps(new_stream_names)}) - self.assert_json_error(result, "Default stream group 'dumbledore's army' does not exist") + result = self.client_patch("/json/default_stream_groups/12345/streams", + {"op": "add", "stream_names": ujson.dumps(new_stream_names)}) + self.assert_json_error(result, "Default stream group with id '12345' does not exist.") + + result = self.client_patch("/json/default_stream_groups/{}/streams".format(group_id), {"op": "add"}) + self.assert_json_error(result, "Missing 'stream_names' argument") do_add_default_stream(new_streams[0]) - result = self.client_patch('/json/default_stream_groups', - {"group_name": group_name, "op": "add", "stream_names": ujson.dumps(new_stream_names)}) + result = self.client_patch("/json/default_stream_groups/{}/streams".format(group_id), + {"op": "add", "stream_names": ujson.dumps(new_stream_names)}) self.assert_json_error(result, "'stream4' is a default stream and cannot be added to 'group1'") do_remove_default_stream(new_streams[0]) - result = self.client_patch("/json/default_stream_groups", - {"group_name": group_name, "op": "add", - "stream_names": ujson.dumps(new_stream_names)}) - default_stream_groups = get_default_stream_groups(realm) - self.assert_length(default_stream_groups, 1) - self.assertEqual(default_stream_groups[0].name, group_name) - self.assertEqual(list(default_stream_groups[0].streams.all()), streams) - - result = self.client_patch('/json/default_stream_groups', - {"group_name": group_name, "op": "add", "stream_names": ujson.dumps(new_stream_names)}) - self.assert_json_error(result, "Stream 'stream4' is already present in default stream group 'group1'") - - # Test removing streams from default stream group - result = self.client_patch("/json/default_stream_groups", - {"group_name": "dumbledore's army", "op": "remove", - "stream_names": ujson.dumps(new_stream_names)}) - self.assert_json_error(result, "Default stream group 'dumbledore's army' does not exist") - - result = self.client_patch("/json/default_stream_groups", - {"group_name": group_name, "op": "remove", - "stream_names": ujson.dumps(["random stream name"])}) - self.assert_json_error(result, "Invalid stream name 'random stream name'") - - streams.remove(new_streams[0]) - result = self.client_patch("/json/default_stream_groups", - {"group_name": group_name, "op": "remove", - "stream_names": ujson.dumps([new_stream_names[0]])}) + result = self.client_patch("/json/default_stream_groups/{}/streams".format(group_id), + {"op": "add", "stream_names": ujson.dumps(new_stream_names)}) self.assert_json_success(result) default_stream_groups = get_default_stream_groups(realm) self.assert_length(default_stream_groups, 1) self.assertEqual(default_stream_groups[0].name, group_name) self.assertEqual(list(default_stream_groups[0].streams.all()), streams) - result = self.client_patch("/json/default_stream_groups", - {"group_name": group_name, "op": "remove", - "stream_names": ujson.dumps(new_stream_names)}) + result = self.client_patch("/json/default_stream_groups/{}/streams".format(group_id), + {"op": "add", "stream_names": ujson.dumps(new_stream_names)}) + self.assert_json_error(result, "Stream 'stream4' is already present in default stream group 'group1'") + + # Test removing streams from default stream group + result = self.client_patch("/json/default_stream_groups/12345/streams", + {"op": "remove", "stream_names": ujson.dumps(new_stream_names)}) + self.assert_json_error(result, "Default stream group with id '12345' does not exist.") + + result = self.client_patch("/json/default_stream_groups/{}/streams".format(group_id), + {"op": "remove", "stream_names": ujson.dumps(["random stream name"])}) + self.assert_json_error(result, "Invalid stream name 'random stream name'") + + streams.remove(new_streams[0]) + result = self.client_patch("/json/default_stream_groups/{}/streams".format(group_id), + {"op": "remove", "stream_names": ujson.dumps([new_stream_names[0]])}) + self.assert_json_success(result) + default_stream_groups = get_default_stream_groups(realm) + self.assert_length(default_stream_groups, 1) + self.assertEqual(default_stream_groups[0].name, group_name) + self.assertEqual(list(default_stream_groups[0].streams.all()), streams) + + result = self.client_patch("/json/default_stream_groups/{}/streams".format(group_id), + {"op": "remove", "stream_names": ujson.dumps(new_stream_names)}) self.assert_json_error(result, "Stream 'stream4' is not present in default stream group 'group1'") # Test deleting a default stream group - result = self.client_delete('/json/default_stream_groups', {"group_name": group_name}) + result = self.client_delete('/json/default_stream_groups/{}'.format(group_id)) self.assert_json_success(result) default_stream_groups = get_default_stream_groups(realm) self.assert_length(default_stream_groups, 0) - do_remove_default_stream(new_stream) - result = self.client_patch("/json/default_stream_groups", - {"group_name": group_name, "op": "add", "stream_names": ujson.dumps(new_stream_names)}) - self.assert_json_error(result, "Default stream group 'group1' does not exist") + result = self.client_delete('/json/default_stream_groups/{}'.format(group_id)) + self.assert_json_error(result, "Default stream group with id '{}' does not exist.".format(group_id)) class SubscriptionPropertiesTest(ZulipTestCase): def test_set_stream_color(self): diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 45728fb67e..c8c51f7af5 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -24,7 +24,7 @@ from zerver.lib.actions import bulk_remove_subscriptions, \ 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, access_stream_for_delete + list_to_streams, access_stream_for_delete, access_default_stream_group_by_id 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, \ @@ -88,26 +88,30 @@ def create_default_stream_group(request: HttpRequest, user_profile: UserProfile, @require_realm_admin @has_request_variables -def update_default_stream_group(request: HttpRequest, user_profile: UserProfile, - group_name: Text=REQ(), op: Text=REQ(), - stream_names: List[Text]=REQ(validator=check_list(check_string))) -> None: +@require_realm_admin +@has_request_variables +def update_default_stream_group_streams(request: HttpRequest, user_profile: UserProfile, + group_id: int, op: Text=REQ(), + stream_names: List[Text]=REQ(validator=check_list(check_string))) -> None: + group = access_default_stream_group_by_id(user_profile.realm, group_id,) streams = [] for stream_name in stream_names: (stream, recipient, sub) = access_stream_by_name(user_profile, stream_name) streams.append(stream) if op == 'add': - do_add_streams_to_default_stream_group(user_profile.realm, group_name, streams) + do_add_streams_to_default_stream_group(user_profile.realm, group, streams) elif op == 'remove': - do_remove_streams_from_default_stream_group(user_profile.realm, group_name, streams) + do_remove_streams_from_default_stream_group(user_profile.realm, group, streams) else: - return json_error(_('Nothing to do. Specify at least one of "add" or "remove".')) + return json_error(_('Invalid value for "op". Specify one of "add" or "remove".')) return json_success() @require_realm_admin @has_request_variables -def remove_default_stream_group(request: HttpRequest, user_profile: UserProfile, group_name: Text=REQ()) -> None: - do_remove_default_stream_group(user_profile.realm, group_name) +def remove_default_stream_group(request: HttpRequest, user_profile: UserProfile, group_id: int) -> None: + group = access_default_stream_group_by_id(user_profile.realm, group_id) + do_remove_default_stream_group(user_profile.realm, group) return json_success() @require_realm_admin diff --git a/zproject/urls.py b/zproject/urls.py index 39f60b4ff6..4b3c9da74f 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -283,10 +283,12 @@ v1_api_and_json_patterns = [ url(r'^default_streams$', rest_dispatch, {'POST': 'zerver.views.streams.add_default_stream', 'DELETE': 'zerver.views.streams.remove_default_stream'}), - url(r'^default_stream_groups', rest_dispatch, - {'POST': 'zerver.views.streams.create_default_stream_group', - 'PATCH': 'zerver.views.streams.update_default_stream_group', - 'DELETE': 'zerver.views.streams.remove_default_stream_group'}), + url(r'^default_stream_groups/create$', rest_dispatch, + {'POST': 'zerver.views.streams.create_default_stream_group'}), + url(r'^default_stream_groups/(?P\d+)$', rest_dispatch, + {'DELETE': 'zerver.views.streams.remove_default_stream_group'}), + url(r'^default_stream_groups/(?P\d+)/streams$', rest_dispatch, + {'PATCH': 'zerver.views.streams.update_default_stream_group_streams'}), # GET lists your streams, POST bulk adds, PATCH bulk modifies/removes url(r'^users/me/subscriptions$', rest_dispatch, {'GET': 'zerver.views.streams.list_subscriptions_backend',