From 442c784660135860586487691c1f5937884fe1af Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Mon, 21 Oct 2019 18:04:04 +0100
Subject: [PATCH 01/11] Depublish old room, fix docstring, code needs
 consolidating

---
 synapse/handlers/room_member.py | 63 +++++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 19 deletions(-)

diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 380e2fad5e88..240806aa3388 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -203,10 +203,25 @@ def _local_membership_update(
                 prev_member_event = yield self.store.get_event(prev_member_event_id)
                 newly_joined = prev_member_event.membership != Membership.JOIN
             if newly_joined:
+                # Check if the new room is an upgraded room
+                predecessor = yield self.store.get_room_predecessor(room_id)
+                if not predecessor:
+                    return
+                old_room_id = predecessor["room_id"]
+                logger.debug("Found predecessor for %s: %s", room_id, old_room_id)
+
                 # Copy over user state if we're joining an upgraded room
-                yield self.copy_user_state_if_room_upgrade(
-                    room_id, requester.user.to_string()
+                yield self.copy_user_state_on_room_upgrade(
+                    old_room_id, room_id, requester.user.to_string()
                 )
+
+                # Depublish the old room from the room directory if it was published
+                room = yield self.store.get_room(old_room_id)
+                if room and room["is_public"]:
+                    yield self.directory_handler.edit_published_room_list(
+                        requester, old_room_id, "private"
+                    )
+
                 yield self._user_joined_room(target, room_id)
         elif event.membership == Membership.LEAVE:
             if prev_member_event_id:
@@ -455,11 +470,30 @@ def _update_membership(
                     requester, remote_room_hosts, room_id, target, content
                 )
 
+                # Check if the new room is an upgraded room
+                # TODO: Consolidate the work done for the duplicated update steps
+                predecessor = yield self.store.get_room_predecessor(room_id)
+                if not predecessor:
+                    return
+                old_room_id = predecessor["room_id"]
+                logger.debug("Found predecessor for %s: %s", room_id, old_room_id)
+
                 # Copy over user state if this is a join on an remote upgraded room
-                yield self.copy_user_state_if_room_upgrade(
-                    room_id, requester.user.to_string()
+                yield self.copy_user_state_on_room_upgrade(
+                    old_room_id, room_id, requester.user.to_string()
                 )
 
+                # Depublish the old room from the room directory if it was published
+                room = yield self.store.get_room(old_room_id)
+                if room and room["is_public"]:
+                    try:
+                        yield self.directory_handler.edit_published_room_list(
+                            requester, old_room_id, "private"
+                        )
+                    except AuthError:
+                        # This user wasn't allowed to depublish the room
+                        pass
+
                 return remote_join_response
 
         elif effective_membership_state == Membership.LEAVE:
@@ -498,35 +532,26 @@ def _update_membership(
         return res
 
     @defer.inlineCallbacks
-    def copy_user_state_if_room_upgrade(self, new_room_id, user_id):
-        """Copy user-specific information when they join a new room if that new room is the
+    def copy_user_state_on_room_upgrade(self, old_room_id, new_room_id, user_id):
+        """Copy user-specific information when they join a new room when that new room is the
         result of a room upgrade
 
         Args:
+            old_room_id (str): The ID of upgraded room
             new_room_id (str): The ID of the room the user is joining
             user_id (str): The ID of the user
 
         Returns:
             Deferred
         """
-        # Check if the new room is an upgraded room
-        predecessor = yield self.store.get_room_predecessor(new_room_id)
-        if not predecessor:
-            return
 
-        logger.debug(
-            "Found predecessor for %s: %s. Copying over room tags and push " "rules",
-            new_room_id,
-            predecessor,
-        )
+        logger.debug("Copying over room tags and push rules")
 
         # It is an upgraded room. Copy over old tags
-        yield self.copy_room_tags_and_direct_to_room(
-            predecessor["room_id"], new_room_id, user_id
-        )
+        yield self.copy_room_tags_and_direct_to_room(old_room_id, new_room_id, user_id)
         # Copy over push rules
         yield self.store.copy_push_rules_from_room_to_room_for_user(
-            predecessor["room_id"], new_room_id, user_id
+            old_room_id, new_room_id, user_id
         )
 
     @defer.inlineCallbacks

From cb081190985b4c826a48211e96b7a063e472e169 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Mon, 21 Oct 2019 18:29:22 +0100
Subject: [PATCH 02/11] Add changelog

---
 changelog.d/6232.bugfix | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 changelog.d/6232.bugfix

diff --git a/changelog.d/6232.bugfix b/changelog.d/6232.bugfix
new file mode 100644
index 000000000000..e69de29bb2d1

From 87279239c2d375c68669fd965c6390550b231717 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Tue, 22 Oct 2019 13:15:21 +0100
Subject: [PATCH 03/11] Consolidate code, allow any user to depublish from room
 dir

---
 changelog.d/6232.bugfix         |  1 +
 synapse/handlers/room_member.py | 77 ++++++++++++++++-----------------
 2 files changed, 38 insertions(+), 40 deletions(-)

diff --git a/changelog.d/6232.bugfix b/changelog.d/6232.bugfix
index e69de29bb2d1..12718ba9341c 100644
--- a/changelog.d/6232.bugfix
+++ b/changelog.d/6232.bugfix
@@ -0,0 +1 @@
+Remove a room from a server's public rooms list on room upgrade.
\ No newline at end of file
diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 240806aa3388..5c4177447d52 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -203,25 +203,11 @@ def _local_membership_update(
                 prev_member_event = yield self.store.get_event(prev_member_event_id)
                 newly_joined = prev_member_event.membership != Membership.JOIN
             if newly_joined:
-                # Check if the new room is an upgraded room
-                predecessor = yield self.store.get_room_predecessor(room_id)
-                if not predecessor:
-                    return
-                old_room_id = predecessor["room_id"]
-                logger.debug("Found predecessor for %s: %s", room_id, old_room_id)
-
-                # Copy over user state if we're joining an upgraded room
-                yield self.copy_user_state_on_room_upgrade(
-                    old_room_id, room_id, requester.user.to_string()
+                # Check if this room is the result of a room upgrade, and if so, copy over any
+                # user information from the old room
+                yield self.transfer_room_state_if_room_upgrade(
+                    room_id, requester.user.to_string()
                 )
-
-                # Depublish the old room from the room directory if it was published
-                room = yield self.store.get_room(old_room_id)
-                if room and room["is_public"]:
-                    yield self.directory_handler.edit_published_room_list(
-                        requester, old_room_id, "private"
-                    )
-
                 yield self._user_joined_room(target, room_id)
         elif event.membership == Membership.LEAVE:
             if prev_member_event_id:
@@ -470,30 +456,12 @@ def _update_membership(
                     requester, remote_room_hosts, room_id, target, content
                 )
 
-                # Check if the new room is an upgraded room
-                # TODO: Consolidate the work done for the duplicated update steps
-                predecessor = yield self.store.get_room_predecessor(room_id)
-                if not predecessor:
-                    return
-                old_room_id = predecessor["room_id"]
-                logger.debug("Found predecessor for %s: %s", room_id, old_room_id)
-
-                # Copy over user state if this is a join on an remote upgraded room
-                yield self.copy_user_state_on_room_upgrade(
-                    old_room_id, room_id, requester.user.to_string()
+                # Check if this room is the result of a room upgrade, and if so, copy over any
+                # user information from the old room
+                yield self.transfer_room_state_if_room_upgrade(
+                    room_id, requester.user.to_string()
                 )
 
-                # Depublish the old room from the room directory if it was published
-                room = yield self.store.get_room(old_room_id)
-                if room and room["is_public"]:
-                    try:
-                        yield self.directory_handler.edit_published_room_list(
-                            requester, old_room_id, "private"
-                        )
-                    except AuthError:
-                        # This user wasn't allowed to depublish the room
-                        pass
-
                 return remote_join_response
 
         elif effective_membership_state == Membership.LEAVE:
@@ -531,6 +499,35 @@ def _update_membership(
         )
         return res
 
+    @defer.inlineCallbacks
+    def transfer_room_state_if_room_upgrade(self, room_id, user_id):
+        """Upon a user joining a room, the server can check if that room is the result of a
+        room upgrade. At this point, the server can transfer over information from the previous
+        room.
+
+        Args:
+            room_id (str): The ID of the room the user is joining
+
+            user_id (str): The ID of the user
+
+        Returns:
+            Deferred
+        """
+        # Check if the new room is an upgraded room
+        predecessor = yield self.store.get_room_predecessor(room_id)
+        if not predecessor:
+            return
+        old_room_id = predecessor["room_id"]
+        logger.debug("Found predecessor for %s: %s", room_id, old_room_id)
+
+        # Copy over user state if this is a join on an remote upgraded room
+        yield self.copy_user_state_on_room_upgrade(old_room_id, room_id, user_id)
+
+        # Depublish the old room from the room directory if it was published
+        room = yield self.store.get_room(old_room_id)
+        if room and room["is_public"]:
+            yield self.store.set_room_is_public(old_room_id, False)
+
     @defer.inlineCallbacks
     def copy_user_state_on_room_upgrade(self, old_room_id, new_room_id, user_id):
         """Copy user-specific information when they join a new room when that new room is the

From 03c28e0243163025495b5d7128a9ca93ab80e726 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Tue, 22 Oct 2019 13:17:59 +0100
Subject: [PATCH 04/11] Improve docstring

---
 synapse/handlers/room_member.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 5c4177447d52..5b25bffab052 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -505,6 +505,10 @@ def transfer_room_state_if_room_upgrade(self, room_id, user_id):
         room upgrade. At this point, the server can transfer over information from the previous
         room.
 
+        Checks if the given room is the result of a room upgrade. If so, copies user state
+        (tags/push rules) from the old room, as well as depublishing it from the room
+        directory if it was in there.
+
         Args:
             room_id (str): The ID of the room the user is joining
 

From 5e44a27062c9b4230a281bf5052769694f176432 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Tue, 22 Oct 2019 13:18:49 +0100
Subject: [PATCH 05/11] Improve logging

---
 synapse/handlers/room_member.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 5b25bffab052..47d6e79aeee8 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -546,7 +546,11 @@ def copy_user_state_on_room_upgrade(self, old_room_id, new_room_id, user_id):
             Deferred
         """
 
-        logger.debug("Copying over room tags and push rules")
+        logger.debug(
+            "Copying over room tags and push rules from %s to %s",
+            old_room_id,
+            new_room_id,
+        )
 
         # It is an upgraded room. Copy over old tags
         yield self.copy_room_tags_and_direct_to_room(old_room_id, new_room_id, user_id)

From 7a01da0b65988971b0121248b2b078a8b85e1402 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Tue, 29 Oct 2019 15:07:51 +0000
Subject: [PATCH 06/11] Switch hook from user joining a room, to server joining
 the room

This prevents us from checking for an upgraded room every time a user on the homeserver joins a new room,
which is unnecessary. We only need to check once per room, instead of on every local join
---
 synapse/federation/federation_client.py |  2 +-
 synapse/handlers/federation.py          | 24 ++++++++---
 synapse/handlers/room_member.py         | 54 +++++++++++--------------
 3 files changed, 44 insertions(+), 36 deletions(-)

diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py
index f5c1632916f4..a5c4cc0e37d7 100644
--- a/synapse/federation/federation_client.py
+++ b/synapse/federation/federation_client.py
@@ -553,7 +553,7 @@ def make_membership_event(
         Note that this does not append any events to any graphs.
 
         Args:
-            destinations (str): Candidate homeservers which are probably
+            destinations (Iterable[unicode]): Candidate homeservers which are probably
                 participating in the room.
             room_id (str): The room in which the event will happen.
             user_id (str): The user whose membership is being evented.
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 488058fe68a4..195aab0f0200 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -111,6 +111,7 @@ def __init__(self, hs):
         self.store = hs.get_datastore()
         self.federation_client = hs.get_federation_client()
         self.state_handler = hs.get_state_handler()
+        self.room_member_handler = hs.get_room_member_handler()
         self.server_name = hs.hostname
         self.keyring = hs.get_keyring()
         self.action_generator = hs.get_action_generator()
@@ -1101,7 +1102,7 @@ def on_event_auth(self, event_id):
     @defer.inlineCallbacks
     def do_invite_join(self, target_hosts, room_id, joinee, content):
         """ Attempts to join the `joinee` to the room `room_id` via the
-        server `target_host`.
+        servers contained in `target_hosts`.
 
         This first triggers a /make_join/ request that returns a partial
         event that we can fill out and sign. This is then sent to the
@@ -1110,6 +1111,15 @@ def do_invite_join(self, target_hosts, room_id, joinee, content):
 
         We suspend processing of any received events from this room until we
         have finished processing the join.
+
+        Args:
+            target_hosts (Iterable[unicode]): List of servers to attempt to join the room with.
+
+            room_id (str): The ID of the room to join.
+
+            joinee (str): The User ID of the joining user.
+
+            content (dict): The event content to use for the join event.
         """
         logger.debug("Joining %s to %s", joinee, room_id)
 
@@ -1169,6 +1179,12 @@ def do_invite_join(self, target_hosts, room_id, joinee, content):
 
             yield self._persist_auth_tree(origin, auth_chain, state, event)
 
+            # Check whether this room is the result of an upgrade of a room we already know
+            # about. If so, migrate over user information
+            yield self.room_member_handler.transfer_room_state_if_room_upgrade(
+                room_id, joinee
+            )
+
             logger.debug("Finished joining %s to %s", joinee, room_id)
         finally:
             room_queue = self.room_queues[room_id]
@@ -2435,8 +2451,7 @@ def exchange_third_party_invite(
                 raise e
 
             yield self._check_signature(event, context)
-            member_handler = self.hs.get_room_member_handler()
-            yield member_handler.send_membership_event(None, event, context)
+            yield self.room_member_handler.send_membership_event(None, event, context)
         else:
             destinations = set(x.split(":", 1)[-1] for x in (sender_user_id, room_id))
             yield self.federation_client.forward_third_party_invite(
@@ -2495,8 +2510,7 @@ def on_exchange_third_party_invite_request(self, room_id, event_dict):
         # though the sender isn't a local user.
         event.internal_metadata.send_on_behalf_of = get_domain_from_id(event.sender)
 
-        member_handler = self.hs.get_room_member_handler()
-        yield member_handler.send_membership_event(None, event, context)
+        yield self.room_member_handler.send_membership_event(None, event, context)
 
     @defer.inlineCallbacks
     def add_display_name_to_third_party_invite(
diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 47d6e79aeee8..e9a333153776 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -203,11 +203,6 @@ def _local_membership_update(
                 prev_member_event = yield self.store.get_event(prev_member_event_id)
                 newly_joined = prev_member_event.membership != Membership.JOIN
             if newly_joined:
-                # Check if this room is the result of a room upgrade, and if so, copy over any
-                # user information from the old room
-                yield self.transfer_room_state_if_room_upgrade(
-                    room_id, requester.user.to_string()
-                )
                 yield self._user_joined_room(target, room_id)
         elif event.membership == Membership.LEAVE:
             if prev_member_event_id:
@@ -456,12 +451,6 @@ def _update_membership(
                     requester, remote_room_hosts, room_id, target, content
                 )
 
-                # Check if this room is the result of a room upgrade, and if so, copy over any
-                # user information from the old room
-                yield self.transfer_room_state_if_room_upgrade(
-                    room_id, requester.user.to_string()
-                )
-
                 return remote_join_response
 
         elif effective_membership_state == Membership.LEAVE:
@@ -500,19 +489,17 @@ def _update_membership(
         return res
 
     @defer.inlineCallbacks
-    def transfer_room_state_if_room_upgrade(self, room_id, user_id):
-        """Upon a user joining a room, the server can check if that room is the result of a
-        room upgrade. At this point, the server can transfer over information from the previous
+    def transfer_room_state_if_room_upgrade(self, room_id):
+        """Upon our server joining a room, we can check if that room is the result of a
+        room upgrade. At this point, we can transfer over information from the previous
         room.
 
         Checks if the given room is the result of a room upgrade. If so, copies user state
-        (tags/push rules) from the old room, as well as depublishing it from the room
-        directory if it was in there.
+        (tags/push rules) for every local user that was in the old room, as well as
+        depublishing the room from the room directory if it was there.
 
         Args:
-            room_id (str): The ID of the room the user is joining
-
-            user_id (str): The ID of the user
+            room_id (str): The ID of the new room
 
         Returns:
             Deferred
@@ -524,8 +511,9 @@ def transfer_room_state_if_room_upgrade(self, room_id, user_id):
         old_room_id = predecessor["room_id"]
         logger.debug("Found predecessor for %s: %s", room_id, old_room_id)
 
-        # Copy over user state if this is a join on an remote upgraded room
-        yield self.copy_user_state_on_room_upgrade(old_room_id, room_id, user_id)
+        # Find all local users that were in the old room and copy over each user's state
+        users = self.store.get_users_in_room(old_room_id)
+        yield self.copy_user_state_on_room_upgrade(old_room_id, room_id, users)
 
         # Depublish the old room from the room directory if it was published
         room = yield self.store.get_room(old_room_id)
@@ -533,14 +521,14 @@ def transfer_room_state_if_room_upgrade(self, room_id, user_id):
             yield self.store.set_room_is_public(old_room_id, False)
 
     @defer.inlineCallbacks
-    def copy_user_state_on_room_upgrade(self, old_room_id, new_room_id, user_id):
+    def copy_user_state_on_room_upgrade(self, old_room_id, new_room_id, user_ids):
         """Copy user-specific information when they join a new room when that new room is the
         result of a room upgrade
 
         Args:
             old_room_id (str): The ID of upgraded room
-            new_room_id (str): The ID of the room the user is joining
-            user_id (str): The ID of the user
+            new_room_id (str): The ID of the new room
+            user_ids (Iterable[str]): User IDs to copy state for
 
         Returns:
             Deferred
@@ -552,12 +540,18 @@ def copy_user_state_on_room_upgrade(self, old_room_id, new_room_id, user_id):
             new_room_id,
         )
 
-        # It is an upgraded room. Copy over old tags
-        yield self.copy_room_tags_and_direct_to_room(old_room_id, new_room_id, user_id)
-        # Copy over push rules
-        yield self.store.copy_push_rules_from_room_to_room_for_user(
-            old_room_id, new_room_id, user_id
-        )
+        for user_id in user_ids:
+            try:
+                # It is an upgraded room. Copy over old tags
+                yield self.copy_room_tags_and_direct_to_room(
+                    old_room_id, new_room_id, user_id
+                )
+                # Copy over push rules
+                yield self.store.copy_push_rules_from_room_to_room_for_user(
+                    old_room_id, new_room_id, user_id
+                )
+            except Exception:
+                continue
 
     @defer.inlineCallbacks
     def send_membership_event(self, requester, event, context, ratelimit=True):

From 3d5ca5a4213e15bbc21e0714d306ba89bf886891 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Tue, 29 Oct 2019 16:02:47 +0000
Subject: [PATCH 07/11] Handle local upgrades as well

---
 synapse/handlers/federation.py  | 16 ++++++++++------
 synapse/handlers/room.py        |  7 ++++++-
 synapse/handlers/room_member.py |  5 +++--
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 195aab0f0200..8630f37551d9 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -111,7 +111,6 @@ def __init__(self, hs):
         self.store = hs.get_datastore()
         self.federation_client = hs.get_federation_client()
         self.state_handler = hs.get_state_handler()
-        self.room_member_handler = hs.get_room_member_handler()
         self.server_name = hs.hostname
         self.keyring = hs.get_keyring()
         self.action_generator = hs.get_action_generator()
@@ -1181,9 +1180,9 @@ def do_invite_join(self, target_hosts, room_id, joinee, content):
 
             # Check whether this room is the result of an upgrade of a room we already know
             # about. If so, migrate over user information
-            yield self.room_member_handler.transfer_room_state_if_room_upgrade(
-                room_id, joinee
-            )
+            # We retrieve the room member handler here as to not cause a cyclic dependency
+            member_handler = self.hs.get_room_member_handler()
+            yield member_handler.transfer_room_state_if_room_upgrade(room_id)
 
             logger.debug("Finished joining %s to %s", joinee, room_id)
         finally:
@@ -2451,7 +2450,10 @@ def exchange_third_party_invite(
                 raise e
 
             yield self._check_signature(event, context)
-            yield self.room_member_handler.send_membership_event(None, event, context)
+
+            # We retrieve the room member handler here as to not cause a cyclic dependency
+            member_handler = self.hs.get_room_member_handler()
+            yield member_handler.send_membership_event(None, event, context)
         else:
             destinations = set(x.split(":", 1)[-1] for x in (sender_user_id, room_id))
             yield self.federation_client.forward_third_party_invite(
@@ -2510,7 +2512,9 @@ def on_exchange_third_party_invite_request(self, room_id, event_dict):
         # though the sender isn't a local user.
         event.internal_metadata.send_on_behalf_of = get_domain_from_id(event.sender)
 
-        yield self.room_member_handler.send_membership_event(None, event, context)
+        # We retrieve the room member handler here as to not cause a cyclic dependency
+        member_handler = self.hs.get_room_member_handler()
+        yield member_handler.send_membership_event(None, event, context)
 
     @defer.inlineCallbacks
     def add_display_name_to_third_party_invite(
diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py
index 2816bd8f8731..a968379bb38d 100644
--- a/synapse/handlers/room.py
+++ b/synapse/handlers/room.py
@@ -129,6 +129,7 @@ def upgrade_room(self, requester, old_room_id, new_version):
             old_room_id,
             new_version,  # args for _upgrade_room
         )
+
         return ret
 
     @defer.inlineCallbacks
@@ -188,7 +189,11 @@ def _upgrade_room(self, requester, old_room_id, new_version):
             requester, old_room_id, new_room_id, old_room_state
         )
 
-        # and finally, shut down the PLs in the old room, and update them in the new
+        # Copy over user push rules, tags and depublish the old room from the room directory
+        # if necessary
+        yield self.room_member_handler.transfer_room_state_if_room_upgrade(new_room_id)
+
+        # finally, shut down the PLs in the old room, and update them in the new
         # room.
         yield self._update_upgraded_room_pls(
             requester, old_room_id, new_room_id, old_room_state
diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index e9a333153776..7b0108cc0513 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -512,7 +512,7 @@ def transfer_room_state_if_room_upgrade(self, room_id):
         logger.debug("Found predecessor for %s: %s", room_id, old_room_id)
 
         # Find all local users that were in the old room and copy over each user's state
-        users = self.store.get_users_in_room(old_room_id)
+        users = yield self.store.get_users_in_room(old_room_id)
         yield self.copy_user_state_on_room_upgrade(old_room_id, room_id, users)
 
         # Depublish the old room from the room directory if it was published
@@ -535,9 +535,10 @@ def copy_user_state_on_room_upgrade(self, old_room_id, new_room_id, user_ids):
         """
 
         logger.debug(
-            "Copying over room tags and push rules from %s to %s",
+            "Copying over room tags and push rules from %s to %s for users %s",
             old_room_id,
             new_room_id,
+            user_ids,
         )
 
         for user_id in user_ids:

From f2024a1f9be9167b205c438517543c5ac030aca0 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Tue, 29 Oct 2019 17:49:36 +0000
Subject: [PATCH 08/11] Make this work in the remote case as well

Rooms on remote servers were getting the old room depublished from the room directory,
but weren't getting new rooms auto-published to the room directory.
---
 synapse/handlers/federation.py  | 12 +++++++++++-
 synapse/handlers/room.py        |  7 ++++---
 synapse/handlers/room_member.py | 29 ++++++++++++-----------------
 3 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 8630f37551d9..f2f2697c3088 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -1180,9 +1180,19 @@ def do_invite_join(self, target_hosts, room_id, joinee, content):
 
             # Check whether this room is the result of an upgrade of a room we already know
             # about. If so, migrate over user information
+            predecessor = yield self.store.get_room_predecessor(room_id)
+            if not predecessor:
+                return
+            old_room_id = predecessor["room_id"]
+            logger.debug(
+                "Found predecessor for %s during remote join: %s", room_id, old_room_id
+            )
+
             # We retrieve the room member handler here as to not cause a cyclic dependency
             member_handler = self.hs.get_room_member_handler()
-            yield member_handler.transfer_room_state_if_room_upgrade(room_id)
+            yield member_handler.transfer_room_state_on_room_upgrade(
+                old_room_id, room_id
+            )
 
             logger.debug("Finished joining %s to %s", joinee, room_id)
         finally:
diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py
index a968379bb38d..fb077ca7bbc0 100644
--- a/synapse/handlers/room.py
+++ b/synapse/handlers/room.py
@@ -189,9 +189,10 @@ def _upgrade_room(self, requester, old_room_id, new_version):
             requester, old_room_id, new_room_id, old_room_state
         )
 
-        # Copy over user push rules, tags and depublish the old room from the room directory
-        # if necessary
-        yield self.room_member_handler.transfer_room_state_if_room_upgrade(new_room_id)
+        # Copy over user push rules, tags and migrate room directory state
+        yield self.room_member_handler.transfer_room_state_on_room_upgrade(
+            old_room_id, new_room_id
+        )
 
         # finally, shut down the PLs in the old room, and update them in the new
         # room.
diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 7b0108cc0513..c1ecdde6c754 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -489,36 +489,31 @@ def _update_membership(
         return res
 
     @defer.inlineCallbacks
-    def transfer_room_state_if_room_upgrade(self, room_id):
-        """Upon our server joining a room, we can check if that room is the result of a
-        room upgrade. At this point, we can transfer over information from the previous
-        room.
+    def transfer_room_state_on_room_upgrade(self, old_room_id, room_id):
+        """Upon our server becoming aware of an upgraded room, either by upgrading a room
+        ourselves or joining one, we can transfer over information from the previous room.
 
-        Checks if the given room is the result of a room upgrade. If so, copies user state
-        (tags/push rules) for every local user that was in the old room, as well as
-        depublishing the room from the room directory if it was there.
+        Copies user state (tags/push rules) for every local user that was in the old room, as
+        well as migrating the room directory state.
 
         Args:
+            old_room_id (str): The ID of the old room
+
             room_id (str): The ID of the new room
 
         Returns:
             Deferred
         """
-        # Check if the new room is an upgraded room
-        predecessor = yield self.store.get_room_predecessor(room_id)
-        if not predecessor:
-            return
-        old_room_id = predecessor["room_id"]
-        logger.debug("Found predecessor for %s: %s", room_id, old_room_id)
-
         # Find all local users that were in the old room and copy over each user's state
         users = yield self.store.get_users_in_room(old_room_id)
         yield self.copy_user_state_on_room_upgrade(old_room_id, room_id, users)
 
-        # Depublish the old room from the room directory if it was published
-        room = yield self.store.get_room(old_room_id)
-        if room and room["is_public"]:
+        # Add new room to the room directory if the old room was there
+        # Remove old room from the room directory
+        old_room = yield self.store.get_room(old_room_id)
+        if old_room and old_room["is_public"]:
             yield self.store.set_room_is_public(old_room_id, False)
+            yield self.store.set_room_is_public(room_id, True)
 
     @defer.inlineCallbacks
     def copy_user_state_on_room_upgrade(self, old_room_id, new_room_id, user_ids):

From 3778ce65fbedf9d92589ebcadf80b2376ca3af60 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Date: Thu, 31 Oct 2019 11:07:22 +0000
Subject: [PATCH 09/11] Apply suggestions from code review

Co-Authored-By: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
---
 synapse/federation/federation_client.py | 2 +-
 synapse/handlers/federation.py          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py
index a5c4cc0e37d7..2a1c091180be 100644
--- a/synapse/federation/federation_client.py
+++ b/synapse/federation/federation_client.py
@@ -553,7 +553,7 @@ def make_membership_event(
         Note that this does not append any events to any graphs.
 
         Args:
-            destinations (Iterable[unicode]): Candidate homeservers which are probably
+            destinations (Iterable[str]): Candidate homeservers which are probably
                 participating in the room.
             room_id (str): The room in which the event will happen.
             user_id (str): The user whose membership is being evented.
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index f2f2697c3088..18257c4b326f 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -1112,7 +1112,7 @@ def do_invite_join(self, target_hosts, room_id, joinee, content):
         have finished processing the join.
 
         Args:
-            target_hosts (Iterable[unicode]): List of servers to attempt to join the room with.
+            target_hosts (Iterable[str]): List of servers to attempt to join the room with.
 
             room_id (str): The ID of the room to join.
 

From 2d1872aba734d78f94982b16e50b651f465b85fe Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Thu, 31 Oct 2019 11:12:04 +0000
Subject: [PATCH 10/11] Add logging to exception handler

---
 synapse/handlers/room_member.py | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index c1ecdde6c754..53de4d08801c 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -547,6 +547,13 @@ def copy_user_state_on_room_upgrade(self, old_room_id, new_room_id, user_ids):
                     old_room_id, new_room_id, user_id
                 )
             except Exception:
+                logger.warning(
+                    "Error copying tags and/or push rules from rooms %s to %s for user %s. "
+                    "Skipping...",
+                    old_room_id,
+                    new_room_id,
+                    user_id,
+                )
                 continue
 
     @defer.inlineCallbacks

From 8b4b9a316e8dfaad42deb434082e7dec6d8e02ed Mon Sep 17 00:00:00 2001
From: Andrew Morgan <andrew@amorgan.xyz>
Date: Thu, 31 Oct 2019 16:13:10 +0000
Subject: [PATCH 11/11] Change from logger.warning to logger.exception

---
 synapse/handlers/room_member.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 7c7866d7527b..06d09c2947f9 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -547,7 +547,7 @@ def copy_user_state_on_room_upgrade(self, old_room_id, new_room_id, user_ids):
                     old_room_id, new_room_id, user_id
                 )
             except Exception:
-                logger.warning(
+                logger.exception(
                     "Error copying tags and/or push rules from rooms %s to %s for user %s. "
                     "Skipping...",
                     old_room_id,