narrow: Handle spurious emails in pm-with searches.

If cordelia searches on pm-with:iago@zulip.com,cordelia@zulip.com,
we now properly treat that the same way as pm-with:iago@zulip.com.

Before this fix, the query would initially go through the
huddle code path.  The symptom wasn't completely obvious, as
eventually a deeper function would return a recipient id
corresponding to a single PM with @iago@zulip.com, but we would
only get messages where iago was the recipient, and not any
messages where he was the sender to cordelia.

I put the helper function for this in zerver/lib/addressee, which
is somewhat speculative.  Eventually, we'll want pm-with queries
to allow for user ids, and I imagine there will be some shared
logic with other Addressee code in terms of how we handle these
strings.  The way we deal with lists of emails/users for various
endpoints is kind of haphazard in the current code, although
granted it's mostly just repeating the same simple patterns.  It
would be nice for some of this code to converge a bit.  This
affects new messages, typing indicators, search filters, etc.,
and some endpoints have strange legacy stuff like supporting
JSON-encoded lists, so it's not trivial to clean this up.

Tweaked by tabbott to add some additional tests.

zerver/lib/addressee.py

@@ -11,6 +11,16 @@ from zerver.models import (
     get_user_including_cross_realm,
 )
 
+def raw_pm_with_emails(email_str: str, my_email: str) -> List[str]:
+    frags = email_str.split(',')
+    emails = [s.strip().lower() for s in frags]
+    emails = [email for email in emails if email]
+
+    if len(emails) > 1:
+        emails = [email for email in emails if email != my_email.lower()]
+
+    return emails
+
 def user_profiles_from_unvalidated_emails(emails: Iterable[str], realm: Realm) -> List[UserProfile]:
     user_profiles = []  # type: List[UserProfile]
     for email in emails:

zerver/tests/test_narrow.py

@@ -202,17 +202,25 @@ class NarrowBuilderTest(ZulipTestCase):
         term = dict(operator='pm-with', operand=self.example_email("hamlet"), negated=True)
         self._do_add_term_test(term, 'WHERE NOT (sender_id = :sender_id_1 AND recipient_id = :recipient_id_1)')
 
-    def test_add_term_using_pm_with_operator_and_more_than_user_as_operand(self) -> None:
+    def test_add_term_using_pm_with_operator_and_self_and_user_as_operand(self) -> None:
         term = dict(operator='pm-with', operand='hamlet@zulip.com, othello@zulip.com')
+        self._do_add_term_test(term, 'WHERE sender_id = :sender_id_1 AND recipient_id = :recipient_id_1 OR sender_id = :sender_id_2 AND recipient_id = :recipient_id_2')
+
+    def test_add_term_using_pm_with_operator_more_than_one_user_as_operand(self) -> None:
+        term = dict(operator='pm-with', operand='cordelia@zulip.com, othello@zulip.com')
         self._do_add_term_test(term, 'WHERE recipient_id = :recipient_id_1')
 
-    def test_add_term_using_pm_with_operator_more_than_user_as_operand_and_negated(
+    def test_add_term_using_pm_with_operator_self_and_user_as_operand_and_negated(
             self) -> None:  # NEGATED
         term = dict(operator='pm-with', operand='hamlet@zulip.com, othello@zulip.com', negated=True)
+        self._do_add_term_test(term, 'WHERE NOT (sender_id = :sender_id_1 AND recipient_id = :recipient_id_1 OR sender_id = :sender_id_2 AND recipient_id = :recipient_id_2)')
+
+    def test_add_term_using_pm_with_operator_more_than_one_user_as_operand_and_negated(self) -> None:
+        term = dict(operator='pm-with', operand='cordelia@zulip.com, othello@zulip.com', negated=True)
         self._do_add_term_test(term, 'WHERE recipient_id != :recipient_id_1')
 
-    def test_add_term_using_pm_with_operator_with_non_existing_user_as_operand(self) -> None:
-        term = dict(operator='pm-with', operand='non-existing@zulip.com')
+    def test_add_term_using_pm_with_operator_with_comma_noise(self) -> None:
+        term = dict(operator='pm-with', operand=' ,,, ,,, ,')
         self.assertRaises(BadNarrowOperator, self._build_query, term)
 
     def test_add_term_using_pm_with_operator_with_existing_and_non_existing_user_as_operand(self) -> None:

zerver/views/messages.py

@@ -20,6 +20,7 @@ from zerver.lib.actions import recipient_for_emails, do_update_message_flags, \
     extract_recipients, truncate_body, render_incoming_message, do_delete_message, \
     do_mark_all_as_read, do_mark_stream_messages_as_read, \
     get_user_info_for_message_updates, check_schedule_message
+from zerver.lib.addressee import raw_pm_with_emails
 from zerver.lib.queue import queue_json_publish
 from zerver.lib.message import (
     access_message,
@@ -289,11 +290,19 @@ class NarrowBuilder:
         return query.where(maybe_negate(cond))
 
     def by_pm_with(self, query: Query, operand: str, maybe_negate: ConditionTransform) -> Query:
-        if ',' in operand:
+        # This will strip our own email out of the operand
+        # and do other munging.
+        emails = raw_pm_with_emails(
+            email_str=operand,
+            my_email=self.user_profile.email,
+        )
+
+        if len(emails) == 0:
+            raise BadNarrowOperator('empty pm-with clause')
+
+        if len(emails) >= 2:
             # Huddle
             try:
-                # Ignore our own email if it is in this list
-                emails = [e.strip() for e in operand.split(',') if e.strip() != self.user_profile.email]
                 recipient = recipient_for_emails(emails, False,
                                                  self.user_profile, self.user_profile)
             except ValidationError:
@@ -302,8 +311,11 @@ class NarrowBuilder:
             return query.where(maybe_negate(cond))
         else:
             # Personal message
+            target_email = emails[0]
             self_recipient = get_personal_recipient(self.user_profile.id)
-            if operand == self.user_profile.email:
+
+            # PM with self
+            if target_email == self.user_profile.email:
                 # Personals with self
                 cond = and_(column("sender_id") == self.user_profile.id,
                             column("recipient_id") == self_recipient.id)
@@ -311,7 +323,7 @@ class NarrowBuilder:
 
             # Personals with other user; include both directions.
             try:
-                narrow_profile = get_user_including_cross_realm(operand, self.user_realm)
+                narrow_profile = get_user_including_cross_realm(target_email, self.user_realm)
             except UserProfile.DoesNotExist:
                 raise BadNarrowOperator('unknown user ' + operand)