Review comment, more tests

sync3/handler/connstate_live.go

@@ -448,16 +448,16 @@ func (s *connStateLive) resort(
 // shouldIncludeHeroes returns whether the given roomID is in a list or direct
 // subscription which should return heroes.
 func (s *connStateLive) shouldIncludeHeroes(roomID string) bool {
+	if s.roomSubscriptions[roomID].IncludeHeroes() {
+		return true
+	}
 	roomIDsToLists := s.lists.ListsByVisibleRoomIDs(s.muxedReq.Lists)
-	subscriptionInclude := s.roomSubscriptions[roomID].IncludeHeroes()
-	listInclude := false
 	for _, listKey := range roomIDsToLists[roomID] {
 		// check if this list should include heroes
 		if !s.muxedReq.Lists[listKey].IncludeHeroes() {
 			continue
 		}
-		listInclude = true
-		break
+		return true
 	}
-	return subscriptionInclude || listInclude
+	return false
 }

sync3/handler/connstate_live_test.go

@@ -0,0 +1,90 @@
+package handler
+
+import (
+	"context"
+	"testing"
+
+	"github.com/matrix-org/sliding-sync/internal"
+	"github.com/matrix-org/sliding-sync/sync3"
+)
+
+func Test_connStateLive_shouldIncludeHeroes(t *testing.T) {
+	ctx := context.Background()
+	list := sync3.NewInternalRequestLists()
+
+	m1 := sync3.RoomConnMetadata{
+		RoomMetadata: internal.RoomMetadata{
+			RoomID: "!abc",
+		},
+	}
+	m2 := sync3.RoomConnMetadata{
+		RoomMetadata: internal.RoomMetadata{
+			RoomID: "!def",
+		},
+	}
+	list.SetRoom(m1)
+	list.SetRoom(m2)
+
+	list.AssignList(ctx, "all_rooms", &sync3.RequestFilters{}, []string{sync3.SortByName}, false)
+	list.AssignList(ctx, "visible_rooms", &sync3.RequestFilters{}, []string{sync3.SortByName}, false)
+
+	boolTrue := true
+	tests := []struct {
+		name      string
+		ConnState *ConnState
+		roomID    string
+		want      bool
+	}{
+		{
+			name:   "neither in subscription nor in list",
+			roomID: "!abc",
+			ConnState: &ConnState{
+				muxedReq: &sync3.Request{},
+			},
+		},
+		{
+			name:   "in room subscription",
+			want:   true,
+			roomID: "!abc",
+			ConnState: &ConnState{
+				muxedReq: &sync3.Request{},
+				roomSubscriptions: map[string]sync3.RoomSubscription{
+					"!abc": {
+						Heroes: &boolTrue,
+					},
+				},
+			},
+		},
+		{
+			name:   "in list all_rooms",
+			roomID: "!def",
+			want:   true,
+			ConnState: &ConnState{
+				muxedReq: &sync3.Request{
+					Lists: map[string]sync3.RequestList{
+						"all_rooms": {
+							SlowGetAllRooms: &boolTrue,
+							RoomSubscription: sync3.RoomSubscription{
+								Heroes: &boolTrue,
+							},
+						},
+						"visible_rooms": {
+							SlowGetAllRooms: &boolTrue,
+						},
+					},
+				},
+				lists: list,
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			s := &connStateLive{
+				ConnState: tt.ConnState,
+			}
+			if got := s.shouldIncludeHeroes(tt.roomID); got != tt.want {
+				t.Errorf("shouldIncludeHeroes() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

tests-e2e/membership_transitions_test.go

@@ -5,9 +5,10 @@ import (
 	"testing"
 	"time"
 
+	"github.com/tidwall/gjson"
+
 	"github.com/matrix-org/sliding-sync/sync3"
 	"github.com/matrix-org/sliding-sync/testutils/m"
-	"github.com/tidwall/gjson"
 )
 
 func TestRoomStateTransitions(t *testing.T) {
@@ -659,6 +660,46 @@ func TestHeroesOnMembershipChanges(t *testing.T) {
 			t.Errorf("expected userID %q, got %q", gotUserID, bob.UserID)
 		}
 	})
+
+	t.Run("can set heroes=true in lists", func(t *testing.T) {
+		listRoomID := alice.CreateRoom(t, map[string]interface{}{"preset": "public_chat"})
+		bob.JoinRoom(t, listRoomID, []string{})
+
+		res := alice.SlidingSyncUntil(t, "", sync3.Request{
+			Lists: map[string]sync3.RequestList{
+				"all_rooms": {
+					RoomSubscription: sync3.RoomSubscription{
+						Heroes:        &boolTrue,
+						TimelineLimit: 10,
+					},
+				},
+			},
+		}, func(response *sync3.Response) error {
+			r, ok := response.Rooms[listRoomID]
+			if !ok {
+				return fmt.Errorf("room %q not in response", listRoomID)
+			}
+			// wait for bob to be joined
+			for _, ev := range r.Timeline {
+				if gjson.GetBytes(ev, "type").Str != "m.room.member" {
+					continue
+				}
+				if gjson.GetBytes(ev, "state_key").Str != bob.UserID {
+					continue
+				}
+				if gjson.GetBytes(ev, "content.membership").Str == "join" {
+					return nil
+				}
+			}
+			return fmt.Errorf("%s is not joined to room %q", bob.UserID, listRoomID)
+		})
+		if c := len(res.Rooms[listRoomID].Heroes); c > 1 {
+			t.Errorf("expected 1 room hero, got %d", c)
+		}
+		if gotUserID := res.Rooms[listRoomID].Heroes[0].ID; gotUserID != bob.UserID {
+			t.Errorf("expected userID %q, got %q", gotUserID, bob.UserID)
+		}
+	})
 }
 
 // test invite/join counts update and are accurate