paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-04 22:13:26 +00:00
Code Issues Packages Projects Releases Wiki Activity

streams: refactor stream creation code path.

Browse Source 
Refactor list_to_streams and create_streams_if_needed to take a list
of dictionaries, instead of a list of stream names.  This is
preparation for being able to pass additional arguments into the
stream creation process.

An important note: This removes a set of validation code from the
start of add_subscriptions_backend; doing so is correct because
list_to_streams has that same validation code already.

[with some tweaks by tabbott for clarity]
This commit is contained in:
Calvin Lee
2016-11-20 15:55:50 -05:00
committed by Tim Abbott
parent 308069d828
commit 8461cc411e
3 changed files with 41 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
zerver/lib/actions.py
View File

@@ -976,13 +976,15 @@ def create_stream_if_needed(realm, stream_name, invite_only=False):
send_event(event, active_user_ids(realm))
return stream, created
def create_streams_if_needed(realm, stream_names, invite_only):
# type: (Realm, List[text_type], bool) -> Tuple[List[Stream], List[Stream]]
def create_streams_if_needed(realm, stream_dicts, invite_only):
# type: (Realm, List[Mapping[str, text_type]], bool) -> Tuple[List[Stream], List[Stream]]
"""Note that stream_dict["name"] is assumed to already be stripped of
whitespace"""
added_streams = [] # type: List[Stream]
existing_streams = [] # type: List[Stream]
for stream_name in stream_names:
for stream_dict in stream_dicts:
stream, created = create_stream_if_needed(realm,
stream_name,
stream_dict["name"],
invite_only=invite_only)
if created:
added_streams.append(stream)

4
zerver/tests/test_subs.py
View File

@@ -63,7 +63,7 @@ class TestCreateStreams(ZulipTestCase):
new_streams, existing_streams = create_streams_if_needed(
realm,
stream_names,
[{"name": stream_name} for stream_name in stream_names],
invite_only=True)
self.assertEqual(len(new_streams), 3)
self.assertEqual(len(existing_streams), 0)
@@ -73,7 +73,7 @@ class TestCreateStreams(ZulipTestCase):
new_streams, existing_streams = create_streams_if_needed(
realm,
stream_names,
[{"name": stream_name} for stream_name in stream_names],
invite_only=True)
self.assertEqual(len(new_streams), 0)
self.assertEqual(len(existing_streams), 3)

53
zerver/views/streams.py
View File

@@ -38,8 +38,8 @@ def is_active_subscriber(user_profile, recipient):
active=True).exists()
def list_to_streams(streams_raw, user_profile, autocreate=False, invite_only=False):
# type: (Iterable[text_type], UserProfile, Optional[bool], Optional[bool]) -> Tuple[List[Stream], List[Stream]]
"""Converts plaintext stream names to a list of Streams, validating input in the process
# type: (Iterable[Mapping[str, text_type]], UserProfile, Optional[bool], Optional[bool]) -> Tuple[List[Stream], List[Stream]]
"""Converts list of dicts to a list of Streams, validating input in the process
For each stream name, we validate it to ensure it meets our
requirements for a proper stream name: that is, that it is shorter
@@ -49,33 +49,39 @@ def list_to_streams(streams_raw, user_profile, autocreate=False, invite_only=Fal
This function in autocreate mode should be atomic: either an exception will be raised
during a precheck, or all the streams specified will have been created if applicable.
@param streams_raw The list of stream names to process
@param streams_raw The list of stream names to process; should
already be stripped of whitespace by the caller.
@param user_profile The user for whom we are retreiving the streams
@param autocreate Whether we should create streams if they don't already exist
@param invite_only Whether newly created streams should have the invite_only bit set
"""
# Validate all streams, getting extant ones, then get-or-creating the rest.
stream_set = set(stream_name.strip() for stream_name in streams_raw)
stream_set = set(stream_dict["name"] for stream_dict in streams_raw)
for stream_name in stream_set:
# Stream names should already have been stripped by the
# caller, but it makes sense to verify anyway.
assert stream_name == stream_name.strip()
if len(stream_name) > Stream.MAX_NAME_LENGTH:
raise JsonableError(_("Stream name (%s) too long.") % (stream_name,))
if not valid_stream_name(stream_name):
raise JsonableError(_("Invalid stream name (%s).") % (stream_name,))
existing_streams = [] # type: List[Stream]
missing_stream_names = [] # type: List[text_type]
missing_stream_dicts = [] # type: List[Mapping[str, text_type]]
existing_stream_map = bulk_get_streams(user_profile.realm, stream_set)
for stream_name in stream_set:
for stream_dict in streams_raw:
stream_name = stream_dict["name"]
stream = existing_stream_map.get(stream_name.lower())
if stream is None:
missing_stream_names.append(stream_name)
missing_stream_dicts.append(stream_dict)
else:
existing_streams.append(stream)
if not missing_stream_names:
if len(missing_stream_dicts) == 0:
# This is the happy path for callers who expected all of these
# streams to exist already.
created_streams = [] # type: List[Stream]
@@ -84,7 +90,8 @@ def list_to_streams(streams_raw, user_profile, autocreate=False, invite_only=Fal
if not user_profile.can_create_streams():
raise JsonableError(_('User cannot create streams.'))
elif not autocreate:
raise JsonableError(_("Stream(s) (%s) do not exist") % ", ".join(missing_stream_names))
raise JsonableError(_("Stream(s) (%s) do not exist") % ", ".join(
stream_dict["name"] for stream_dict in missing_stream_dicts))
# We already filtered out existing streams, so dup_streams
# will normally be an empty list below, but we protect against somebody
@@ -92,7 +99,7 @@ def list_to_streams(streams_raw, user_profile, autocreate=False, invite_only=Fal
# paranoid approach, since often on Zulip two people will discuss
# creating a new stream, and both people eagerly do it.)
created_streams, dup_streams = create_streams_if_needed(realm=user_profile.realm,
stream_names=missing_stream_names,
stream_dicts=missing_stream_dicts,
invite_only=invite_only)
existing_streams += dup_streams
@@ -241,7 +248,11 @@ def remove_subscriptions_backend(request, user_profile,
# admin.
return json_error(_("This action requires administrative rights"))
streams, __ = list_to_streams(streams_raw, user_profile)
streams_as_dict = []
for stream_name in streams_raw:
streams_as_dict.append({"name": stream_name.strip()})
streams, __ = list_to_streams(streams_as_dict, user_profile)
for stream in streams:
if removing_someone_else and stream.invite_only and \
@@ -311,18 +322,20 @@ def add_subscriptions_backend(request, user_profile,
principals = REQ(validator=check_list(check_string), default=None),
authorization_errors_fatal = REQ(validator=check_bool, default=True)):
# type: (HttpRequest, UserProfile, Iterable[Mapping[str, text_type]], bool, bool, Optional[List[text_type]], bool) -> HttpResponse
stream_names = []
stream_dicts = []
for stream_dict in streams_raw:
stream_name = stream_dict["name"].strip()
if len(stream_name) > Stream.MAX_NAME_LENGTH:
return json_error(_("Stream name (%s) too long.") % (stream_name,))
if not valid_stream_name(stream_name):
return json_error(_("Invalid stream name (%s).") % (stream_name,))
stream_names.append(stream_name)
stream_dict_copy = {} # type: Dict[str, Any]
for field in stream_dict:
stream_dict_copy[field] = stream_dict[field]
# Strip the stream name here.
stream_dict_copy['name'] = stream_dict_copy['name'].strip()
stream_dicts.append(stream_dict_copy)
# Enforcement of can_create_streams policy is inside list_to_streams.
# Validation of the streams arguments, including enforcement of
# can_create_streams policy and valid_stream_name policy is inside
# list_to_streams.
existing_streams, created_streams = \
list_to_streams(stream_names, user_profile, autocreate=True, invite_only=invite_only)
list_to_streams(stream_dicts, user_profile, autocreate=True, invite_only=invite_only)
authorized_streams, unauthorized_streams = \
filter_stream_authorization(user_profile, existing_streams)
if len(unauthorized_streams) > 0 and authorization_errors_fatal: