typing: Inline check_typing_notification.

I actually like this pattern:

    def check_send_typing_notification(...):
        typing_notification = check_typing_notification(...)
        do_send_typing_notification(...)

It can help divide responsibilities nicely and make it easy
to write detailed unit tests against each of the two helpers.

Unfortunately, the good things didn't really happen here, and
instead we got the worst aspects of the pattern:

    - The responsibilities for validation leaked into
      the second function.

    - Both functions were doing sane things individually
      that became not-so-sane in the big picture (namely,
      we ended up making Recipient objects for no reason,
      but if you read each of the helpers, it was just one
      step that seemed reasonable).

    - Passing around dictionaries for results can be annoying.

Also, the pattern made a lot more sense when the validation
for typing was a lot more complicated.  My prior commit makes
it so that we only ever deal with a list of user_ids.

Anyway, now I'm inlining it. :)

Subsequent commits will clean up the more substantive issue
here, which is that we are building Recipients for no reason.
This commit is contained in:
Steve Howell 2020-02-23 13:10:26 +00:00 committed by Tim Abbott
parent b26f2dcd4b
commit bed6d5a789
1 changed files with 25 additions and 17 deletions

View File

@ -1745,20 +1745,29 @@ def do_remove_reaction(user_profile: UserProfile, message: Message,
reaction.delete()
notify_reaction_update(user_profile, message, reaction, "remove")
def do_send_typing_notification(realm: Realm, notification: Dict[str, Any]) -> None:
recipient_user_profiles = get_typing_user_profiles(notification['recipient'],
notification['sender'].id)
def do_send_typing_notification(
realm: Realm,
sender: UserProfile,
recipient: Recipient,
operator: str) -> None:
recipient_user_profiles = get_typing_user_profiles(
recipient,
sender.id,
)
# Only deliver the notification to active user recipients
user_ids_to_notify = [profile.id for profile in recipient_user_profiles if profile.is_active]
sender_dict = {'user_id': notification['sender'].id, 'email': notification['sender'].email}
sender_dict = {'user_id': sender.id, 'email': sender.email}
# Include a list of recipients in the event body to help identify where the typing is happening
recipient_dicts = [{'user_id': profile.id, 'email': profile.email}
for profile in recipient_user_profiles]
event = dict(
type='typing',
op = notification['op'],
op=operator,
sender=sender_dict,
recipients = recipient_dicts)
recipients=recipient_dicts,
)
send_event(realm, event, user_ids_to_notify)
@ -1766,15 +1775,8 @@ def do_send_typing_notification(realm: Realm, notification: Dict[str, Any]) -> N
# Checks the typing notification and sends it
def check_send_typing_notification(sender: UserProfile, notification_to: Union[Sequence[str], Sequence[int]],
operator: str) -> None:
typing_notification = check_typing_notification(sender, notification_to, operator)
do_send_typing_notification(sender.realm, typing_notification)
# check_typing_notification:
# Returns typing notification ready for sending with do_send_typing_notification on success
# or the error message (string) on error.
def check_typing_notification(sender: UserProfile,
notification_to: Union[Sequence[str], Sequence[int]],
operator: str) -> Dict[str, Any]:
realm = sender.realm
if len(notification_to) == 0:
raise JsonableError(_('Missing parameter: \'to\' (recipient)'))
elif operator not in ('start', 'stop'):
@ -1791,7 +1793,13 @@ def check_typing_notification(sender: UserProfile,
assert isinstance(e.messages[0], str)
raise JsonableError(e.messages[0])
assert recipient.type != Recipient.STREAM
return {'sender': sender, 'recipient': recipient, 'op': operator}
do_send_typing_notification(
realm=realm,
sender=sender,
recipient=recipient,
operator=operator,
)
def send_stream_creation_event(stream: Stream, user_ids: List[int]) -> None:
event = dict(type="stream", op="create",