Add tests and apply suggestions from code review.

Author: dklimpel

docs/admin_api/rooms.md

@@ -19,7 +19,7 @@ The following query parameters are available:
   - `joined_local_members` - Rooms are ordered by the number of local members. Largest to smallest.
   - `version` - Rooms are ordered by room version. Largest to smallest.
   - `creator` - Rooms are ordered alphabetically by creator of the room.
-  - `encryption` - Rooms are ordered by encryption of the room.
+  - `encryption` - Rooms are ordered alphabetically by the end-to-end encryption algorithm.
   - `federatable` - Rooms are ordered by whether the room is federatable.
   - `public` - Rooms are ordered by visibility in room list.
   - `join_rules` - Rooms are ordered alphabetically by join rules of the room.
@@ -227,13 +227,33 @@ Response:
       "name": "Music Theory",
       "canonical_alias": "#musictheory:matrix.org",
       "joined_members": 127
+      "joined_local_members": 2,
+      "version": "1",
+      "creator": "@foo:matrix.org",
+      "encryption": null,
+      "federatable": true,
+      "public": true,
+      "join_rules": "invite",
+      "guest_access": null,
+      "history_visibility": "shared",
+      "state_events": 93534
     },
     ... (48 hidden items) ...
     {
       "room_id": "!twcBhHVdZlQWuuxBhN:termina.org.uk",
       "name": "weechat-matrix",
       "canonical_alias": "#weechat-matrix:termina.org.uk",
       "joined_members": 137
+      "joined_local_members": 20,
+      "version": "4",
+      "creator": "@foo:termina.org.uk",
+      "encryption": null,
+      "federatable": true,
+      "public": true,
+      "join_rules": "invite",
+      "guest_access": null,
+      "history_visibility": "shared",
+      "state_events": 8345
     }
   ],
   "offset": 100,

synapse/storage/data_stores/main/room.py

@@ -52,11 +52,14 @@ class RoomSortOrder(Enum):
     """
     Enum to define the sorting method used when returning rooms with get_rooms_paginate
 
-    ALPHABETICAL = sort rooms alphabetically by name
-    SIZE = sort rooms by membership size, highest to lowest
+    NAME = sort rooms alphabetically by name
+    JOINED_MEMBERS = sort rooms by membership size, highest to lowest
     """
 
+    # ALPHABETICAL and SIZE are deprecated.
+    # ALPHABETICAL is same as NAME.
     ALPHABETICAL = "alphabetical"
+    # SIZE is same as JOINED_MEMBERS.
     SIZE = "size"
     NAME = "name"
     CANONICAL_ALIAS = "canonical_alias"

tests/rest/admin/test_room.py

@@ -473,34 +473,37 @@ def test_room_list_sort_order(self):
         """Test room list sort ordering. alphabetical name versus number of members,
         reversing the order, etc.
         """
-        # Create 3 test rooms
-        room_id_1 = self.helper.create_room_as(self.admin_user, tok=self.admin_user_tok)
-        room_id_2 = self.helper.create_room_as(self.admin_user, tok=self.admin_user_tok)
-        room_id_3 = self.helper.create_room_as(self.admin_user, tok=self.admin_user_tok)
-
-        # Set room names in alphabetical order. room 1 -> A, 2 -> B, 3 -> C
-        self.helper.send_state(
-            room_id_1, "m.room.name", {"name": "A"}, tok=self.admin_user_tok,
-        )
-        self.helper.send_state(
-            room_id_2, "m.room.name", {"name": "B"}, tok=self.admin_user_tok,
-        )
-        self.helper.send_state(
-            room_id_3, "m.room.name", {"name": "C"}, tok=self.admin_user_tok,
-        )
-
-        # Set room member size in the reverse order. room 1 -> 1 member, 2 -> 2, 3 -> 3
-        user_1 = self.register_user("bob1", "pass")
-        user_1_tok = self.login("bob1", "pass")
-        self.helper.join(room_id_2, user_1, tok=user_1_tok)
 
-        user_2 = self.register_user("bob2", "pass")
-        user_2_tok = self.login("bob2", "pass")
-        self.helper.join(room_id_3, user_2, tok=user_2_tok)
+        def _set_canonical_alias(room_id: str, test_alias: str, admin_user_tok: str):
+            # Create a new alias to this room
+            url = "/_matrix/client/r0/directory/room/%s" % (
+                urllib.parse.quote(test_alias),
+            )
+            request, channel = self.make_request(
+                "PUT",
+                url.encode("ascii"),
+                {"room_id": room_id},
+                access_token=admin_user_tok,
+            )
+            self.render(request)
+            self.assertEqual(
+                200, int(channel.result["code"]), msg=channel.result["body"]
+            )
 
-        user_3 = self.register_user("bob3", "pass")
-        user_3_tok = self.login("bob3", "pass")
-        self.helper.join(room_id_3, user_3, tok=user_3_tok)
+            # Set this new alias as the canonical alias for this room
+            self.helper.send_state(
+                room_id,
+                "m.room.aliases",
+                {"aliases": [test_alias]},
+                tok=admin_user_tok,
+                state_key="test",
+            )
+            self.helper.send_state(
+                room_id,
+                "m.room.canonical_alias",
+                {"alias": test_alias},
+                tok=admin_user_tok,
+            )
 
         def _order_test(
             order_type: str, expected_room_list: List[str], reverse: bool = False,
@@ -544,13 +547,85 @@ def _order_test(
             returned_order = [r["room_id"] for r in rooms]
             self.assertListEqual(expected_room_list, returned_order)  # order is checked
 
+        # Create 3 test rooms
+        room_id_1 = self.helper.create_room_as(self.admin_user, tok=self.admin_user_tok)
+        room_id_2 = self.helper.create_room_as(self.admin_user, tok=self.admin_user_tok)
+        room_id_3 = self.helper.create_room_as(self.admin_user, tok=self.admin_user_tok)
+
+        # Set room names in alphabetical order. room 1 -> A, 2 -> B, 3 -> C
+        self.helper.send_state(
+            room_id_1, "m.room.name", {"name": "A"}, tok=self.admin_user_tok,
+        )
+        self.helper.send_state(
+            room_id_2, "m.room.name", {"name": "B"}, tok=self.admin_user_tok,
+        )
+        self.helper.send_state(
+            room_id_3, "m.room.name", {"name": "C"}, tok=self.admin_user_tok,
+        )
+
+        # Set room canonical room aliases
+        _set_canonical_alias(room_id_1, "#A_alias:test", self.admin_user_tok)
+        _set_canonical_alias(room_id_2, "#B_alias:test", self.admin_user_tok)
+        _set_canonical_alias(room_id_3, "#C_alias:test", self.admin_user_tok)
+
+        # Set room member size in the reverse order. room 1 -> 1 member, 2 -> 2, 3 -> 3
+        user_1 = self.register_user("bob1", "pass")
+        user_1_tok = self.login("bob1", "pass")
+        self.helper.join(room_id_2, user_1, tok=user_1_tok)
+
+        user_2 = self.register_user("bob2", "pass")
+        user_2_tok = self.login("bob2", "pass")
+        self.helper.join(room_id_3, user_2, tok=user_2_tok)
+
+        user_3 = self.register_user("bob3", "pass")
+        user_3_tok = self.login("bob3", "pass")
+        self.helper.join(room_id_3, user_3, tok=user_3_tok)
+
         # Test different sort orders, with forward and reverse directions
         _order_test("name", [room_id_1, room_id_2, room_id_3])
         _order_test("name", [room_id_3, room_id_2, room_id_1], reverse=True)
 
+        _order_test("canonical_alias", [room_id_1, room_id_2, room_id_3])
+        _order_test("canonical_alias", [room_id_3, room_id_2, room_id_1], reverse=True)
+
         _order_test("joined_members", [room_id_3, room_id_2, room_id_1])
         _order_test("joined_members", [room_id_1, room_id_2, room_id_3], reverse=True)
 
+        _order_test("joined_local_members", [room_id_3, room_id_2, room_id_1])
+        _order_test(
+            "joined_local_members", [room_id_1, room_id_2, room_id_3], reverse=True
+        )
+
+        _order_test("version", [room_id_1, room_id_2, room_id_3])
+        _order_test("version", [room_id_1, room_id_2, room_id_3], reverse=True)
+
+        _order_test("creator", [room_id_1, room_id_2, room_id_3])
+        _order_test("creator", [room_id_1, room_id_2, room_id_3], reverse=True)
+
+        _order_test("encryption", [room_id_1, room_id_2, room_id_3])
+        _order_test("encryption", [room_id_1, room_id_2, room_id_3], reverse=True)
+
+        _order_test("federatable", [room_id_1, room_id_2, room_id_3])
+        _order_test("federatable", [room_id_1, room_id_2, room_id_3], reverse=True)
+
+        _order_test("public", [room_id_1, room_id_2, room_id_3])
+        # Different sort order of SQlite and PostreSQL
+        # _order_test("public", [room_id_3, room_id_2, room_id_1], reverse=True)
+
+        _order_test("join_rules", [room_id_1, room_id_2, room_id_3])
+        _order_test("join_rules", [room_id_1, room_id_2, room_id_3], reverse=True)
+
+        _order_test("guest_access", [room_id_1, room_id_2, room_id_3])
+        _order_test("guest_access", [room_id_1, room_id_2, room_id_3], reverse=True)
+
+        _order_test("history_visibility", [room_id_1, room_id_2, room_id_3])
+        _order_test(
+            "history_visibility", [room_id_1, room_id_2, room_id_3], reverse=True
+        )
+
+        _order_test("state_events", [room_id_3, room_id_2, room_id_1])
+        _order_test("state_events", [room_id_1, room_id_2, room_id_3], reverse=True)
+
     def test_search_term(self):
         """Test that searching for a room works correctly"""
         # Create two test rooms