Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Transfer bans on room upgrade #4642

Merged
merged 5 commits into from
Feb 19, 2019
Merged

Transfer bans on room upgrade #4642

merged 5 commits into from
Feb 19, 2019

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Feb 14, 2019 •
edited
Loading

Transfer bans on a room upgrade.

Closes #4600

Sytest PR: matrix-org/sytest#563

@anoadragon453 anoadragon453 mentioned this pull request Feb 14, 2019
Merged
@codecov-io
Copy link

codecov-io commented Feb 14, 2019 •
edited
Loading

Codecov Report

Merging #4642 into develop will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #4642      +/-   ##
===========================================
+ Coverage    75.17%   75.19%   +0.01%     
===========================================
  Files          340      340              
  Lines        34712    34717       +5     
  Branches      5677     5679       +2     
===========================================
+ Hits         26096    26104       +8     
+ Misses        7013     7012       -1     
+ Partials      1603     1601       -2

@anoadragon453 anoadragon453 requested a review from a team February 14, 2019 14:47
richvdh
richvdh previously requested changes Feb 18, 2019
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeahh let's not do this. there's a whole different code path for membership events.

I'd suggest using a completely separate loop in clone_existing_room and calling room_member_handler.update_membership instead.

@anoadragon453
Copy link
Member Author

The problem with room_member_handler.update_membership is that it required a proper User object, and we only have User ID strs from the member event. Is there a way to convert those to User's, even for remote users?

@richvdh
Copy link
Member

richvdh commented Feb 18, 2019

there's a from_string class method

@anoadragon453 anoadragon453 requested a review from a team February 18, 2019 14:02
@anoadragon453 anoadragon453 dismissed richvdh’s stale review February 18, 2019 15:03

Addressed changes

richvdh
richvdh approved these changes Feb 18, 2019
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm otherwise

synapse/handlers/room.py Outdated
@@ -285,6 +285,7 @@ def clone_existing_room(
(EventTypes.RoomAvatar, ""),
(EventTypes.Encryption, ""),
(EventTypes.ServerACL, ""),
(EventTypes.Member, None),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would probably be better to do this with a separate call to get_filtered_current_state_ids.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, in testing this seems to be a few hundred ms slower if we do another call to the DB.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how big was your room? ;-p

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Literally 1 ban and no messages lol

synapse/handlers/room.py Outdated
# Transfer membership events
for old_event in member_events:
# Only transfer ban events
logger.info("Event type: " + str(old_event.content))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like it needs to go away

changelog.d/4642.bugfix Outdated
@@ -0,0 +1 @@
Transfer bans on room upgrade.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I call it a feature rather than a bugfix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? All the other room upgrade stuff I've been doing have been bug fixes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that's incorrect, then. This is a new feature for room upgrades as far as I'm concerned.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kk, I have no strong feelings one way or the other.

@anoadragon453 anoadragon453 merged commit 968a30a into develop Feb 19, 2019
@anoadragon453 anoadragon453 deleted the anoa/bans_room_upgrade branch February 19, 2019 11:47
anoadragon453 added a commit to matrix-org/sytest that referenced this pull request Mar 11, 2019
@anoadragon453
Add test for transferring bans on a room upgrade (#563)
e9015c6 
Test for Synapse PR matrix-org/synapse#4642
@ara4n ara4n mentioned this pull request Mar 1, 2022
Open
17 tasks
@Kladki Kladki mentioned this pull request Jul 8, 2024
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@richvdh richvdh richvdh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@anoadragon453 @codecov-io @richvdh