Address review comments

Author: nabramow

app/backend/src/couchers/i18n/locales/en.json

@@ -123,6 +123,7 @@
     "invalid_phone": "Phone number must be in international format without punctuation.",
     "invalid_recipients": "Invalid recipients list.",
     "invalid_region": "Invalid region.",
+    "invalid_thread_filter": "Invalid message thread filter.",
     "invalid_token": "Invalid token.",
     "invalid_username": "Invalid username.",
     "invalid_website_url": "Invalid website URL or text, make sure URL starts with \"https://\", and that the text is included in the URL.",

app/backend/src/couchers/servicers/conversations.py

@@ -406,8 +406,7 @@ def _host_request_thread_to_pb(
     public-trip offer the roles are reversed relative to initiator/recipient
     (initiator = offering host, recipient = traveller).
     """
-    is_public_trip_offer = host_request.public_trip_id is not None
-    if is_public_trip_offer:
+    if host_request.public_trip_id is not None:
         surfer_user_id = host_request.recipient_user_id
         host_user_id = host_request.initiator_user_id
     else:
@@ -429,8 +428,6 @@ def _host_request_thread_to_pb(
             if host_request.initiator_user_id == user_id
             else host_request.is_recipient_archived
         ),
-        is_public_trip_offer=is_public_trip_offer,
-        viewer_is_host=(host_user_id == user_id),
         public_trip_id=host_request.public_trip_id,
         unseen_message_count=unseen_message_count,
     )
@@ -724,12 +721,15 @@ def ListMessageThreads(
     ) -> conversations_pb2.ListMessageThreadsRes:
         thread_filter = request.filter
 
+        if thread_filter == conversations_pb2.MESSAGE_THREAD_FILTER_UNSPECIFIED:
+            context.abort_with_error_code(grpc.StatusCode.INVALID_ARGUMENT, "invalid_thread_filter")
+
         if thread_filter == conversations_pb2.MESSAGE_THREAD_FILTER_PUBLIC_TRIPS and not context.get_boolean_value(
             "public_trips_enabled", False
         ):
             return conversations_pb2.ListMessageThreadsRes(threads=[], no_more=True)
 
-        page_size = request.number if request.number != 0 else DEFAULT_PAGINATION_LENGTH
+        page_size = request.page_size if request.page_size != 0 else DEFAULT_PAGINATION_LENGTH
         page_size = min(page_size, MAX_PAGE_SIZE)
         only_archived = request.only_archived if request.HasField("only_archived") else None
         unread = thread_filter == conversations_pb2.MESSAGE_THREAD_FILTER_UNREAD
@@ -807,6 +807,9 @@ def MarkAllThreadsSeen(
     ) -> empty_pb2.Empty:
         thread_filter = request.filter
 
+        if thread_filter == conversations_pb2.MESSAGE_THREAD_FILTER_UNSPECIFIED:
+            context.abort_with_error_code(grpc.StatusCode.INVALID_ARGUMENT, "invalid_thread_filter")
+
         if thread_filter == conversations_pb2.MESSAGE_THREAD_FILTER_PUBLIC_TRIPS and not context.get_boolean_value(
             "public_trips_enabled", False
         ):

app/backend/src/couchers/servicers/public_trips.py

@@ -91,14 +91,10 @@ def public_trip_to_pb(
                 .group_by(HostRequest.status)
             ).all()
         )
-        pb.offer_tally.CopyFrom(
-            public_trips_pb2.PublicTripOfferTally(
-                pending=counts_by_status.get(HostRequestStatus.pending, 0),
-                accepted=counts_by_status.get(HostRequestStatus.accepted, 0)
-                + counts_by_status.get(HostRequestStatus.confirmed, 0),
-                declined=counts_by_status.get(HostRequestStatus.rejected, 0),
-            )
-        )
+        pb.offer_tally.pending = counts_by_status.get(HostRequestStatus.pending, 0)
+        pb.offer_tally.accepted = counts_by_status.get(HostRequestStatus.accepted, 0)
+        pb.offer_tally.confirmed = counts_by_status.get(HostRequestStatus.confirmed, 0)
+        pb.offer_tally.declined = counts_by_status.get(HostRequestStatus.rejected, 0)
     else:
         # The viewer's own existing offer on this trip (if any), so the client can
         # show an "already offered" state and link to the thread.

app/backend/src/tests/test_message_threads.py

@@ -1,5 +1,6 @@
 from datetime import timedelta
 
+import grpc
 import pytest
 
 from couchers.db import session_scope
@@ -146,7 +147,7 @@ def test_list_message_threads_single_cursor_pagination_across_kinds(db, moderato
         with conversations_session(token1) as c:
             res = c.ListMessageThreads(
                 conversations_pb2.ListMessageThreadsReq(
-                    filter=conversations_pb2.MESSAGE_THREAD_FILTER_ALL, number=3, page_token=page_token
+                    filter=conversations_pb2.MESSAGE_THREAD_FILTER_ALL, page_size=3, page_token=page_token
                 )
             )
         for t in res.threads:
@@ -183,6 +184,23 @@ def test_list_message_threads_chats_filter_excludes_host_requests(db, moderator)
     assert res.threads[0].group_chat.group_chat_id == chat_id
 
 
+def test_message_threads_reject_unspecified_filter(db):
+    user1, token1 = generate_user()
+
+    with conversations_session(token1) as c:
+        with pytest.raises(grpc.RpcError) as e:
+            c.ListMessageThreads(
+                conversations_pb2.ListMessageThreadsReq(filter=conversations_pb2.MESSAGE_THREAD_FILTER_UNSPECIFIED)
+            )
+        assert e.value.code() == grpc.StatusCode.INVALID_ARGUMENT
+
+        with pytest.raises(grpc.RpcError) as e:
+            c.MarkAllThreadsSeen(
+                conversations_pb2.MarkAllThreadsSeenReq(filter=conversations_pb2.MESSAGE_THREAD_FILTER_UNSPECIFIED)
+            )
+        assert e.value.code() == grpc.StatusCode.INVALID_ARGUMENT
+
+
 def test_list_message_threads_unread_filter(db, moderator):
     user1, token1 = generate_user()
     user2, token2 = generate_user()
@@ -251,11 +269,11 @@ def test_list_message_threads_public_trip_offer_role_based(db, moderator):
         assert len(res.threads) == 1
         hr = res.threads[0].host_request
         assert hr.host_request_id == request_id
-        assert hr.is_public_trip_offer
+        assert hr.HasField("public_trip_id")
         assert hr.public_trip_id == trip_id
+        # viewer is the offering host: host_user_id == own id (viewer_is_host derived client-side)
         assert hr.host_user_id == host.id
         assert hr.surfer_user_id == traveler.id
-        assert hr.viewer_is_host
 
         # not under SURFING for the host
         res = c.ListMessageThreads(
@@ -270,8 +288,9 @@ def test_list_message_threads_public_trip_offer_role_based(db, moderator):
         )
         assert [t.host_request.host_request_id for t in res.threads] == [request_id]
         assert res.threads[0].host_request.surfer_user_id == traveler.id
+        # viewer is the traveller, not the host: host_user_id != own id
         assert res.threads[0].host_request.host_user_id == host.id
-        assert not res.threads[0].host_request.viewer_is_host
+        assert res.threads[0].host_request.host_user_id != traveler.id
 
         res = c.ListMessageThreads(
             conversations_pb2.ListMessageThreadsReq(filter=conversations_pb2.MESSAGE_THREAD_FILTER_PUBLIC_TRIPS)

app/backend/src/tests/test_public_trips.py

@@ -1026,8 +1026,8 @@ def test_list_public_trips_by_user_offer_tally_owner(db):
         trip = next(t for t in res.public_trips if t.trip_id == trip_id)
         assert trip.HasField("offer_tally")
         assert trip.offer_tally.pending == 1
-        # accepted bucket includes confirmed
-        assert trip.offer_tally.accepted == 2
+        assert trip.offer_tally.accepted == 1
+        assert trip.offer_tally.confirmed == 1
         assert trip.offer_tally.declined == 1
         # cancelled is excluded from offers_count and the tally
         assert trip.offers_count == 4

app/proto/conversations.proto

@@ -198,12 +198,13 @@ message ListGroupChatsRes {
 }
 
 enum MessageThreadFilter {
-  MESSAGE_THREAD_FILTER_ALL = 0;
-  MESSAGE_THREAD_FILTER_UNREAD = 1;
-  MESSAGE_THREAD_FILTER_CHATS = 2; // group chats + DMs only
-  MESSAGE_THREAD_FILTER_HOSTING = 3; // role-based: I am the host of the stay
-  MESSAGE_THREAD_FILTER_SURFING = 4; // role-based: I am the guest/traveller
-  MESSAGE_THREAD_FILTER_PUBLIC_TRIPS = 5; // my incoming public-trip offers (I'm the traveller)
+  MESSAGE_THREAD_FILTER_UNSPECIFIED = 0;
+  MESSAGE_THREAD_FILTER_ALL = 1;
+  MESSAGE_THREAD_FILTER_UNREAD = 2;
+  MESSAGE_THREAD_FILTER_CHATS = 3; // group chats + DMs only
+  MESSAGE_THREAD_FILTER_HOSTING = 4; // role-based: I am the host of the stay
+  MESSAGE_THREAD_FILTER_SURFING = 5; // role-based: I am the guest/traveller
+  MESSAGE_THREAD_FILTER_PUBLIC_TRIPS = 6; // my incoming public-trip offers (I'm the traveller)
 }
 
 // A host request (or public-trip offer) shaped for the unified thread list.
@@ -222,12 +223,11 @@ message HostRequestThread {
   Message latest_message = 9;
   string hosting_city = 10;
   bool is_archived = 11;
-  // true if this thread is a public-trip offer (public_trip_id is set)
-  bool is_public_trip_offer = 12;
-  // true if the viewer is the host of the stay
-  bool viewer_is_host = 13;
-  optional uint64 public_trip_id = 14;
-  uint32 unseen_message_count = 15;
+  // Set iff this thread is a public-trip offer; presence is the "is offer" flag,
+  // and the value links to the trip. The viewer's role is derived client-side:
+  // the viewer is the host iff host_user_id is their own user id.
+  optional uint64 public_trip_id = 12;
+  uint32 unseen_message_count = 13;
 }
 
 message MessageThread {
@@ -241,8 +241,8 @@ message ListMessageThreadsReq {
   MessageThreadFilter filter = 1;
   // orthogonal: when present, restrict to archived (true) or non-archived (false)
   optional bool only_archived = 2;
-  uint32 number = 3;
-  // opaque cursor; the Message.id high-water mark
+  uint32 page_size = 3;
+  // pagination cursor; pass back the next_page_token from the previous response, or empty for the first page
   string page_token = 4;
 }
 

app/proto/public_trips.proto

@@ -74,8 +74,9 @@ message PublicTrip {
 
 message PublicTripOfferTally {
   int32 pending = 1;
-  int32 accepted = 2; // accepted + confirmed
-  int32 declined = 3; // rejected
+  int32 accepted = 2;
+  int32 confirmed = 3;
+  int32 declined = 4; // rejected
 }
 
 message CreatePublicTripReq {