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

Transfer bans on room upgrade#4642

Merged
anoadragon453 merged 5 commits into
developfrom
anoa/bans_room_upgrade
Feb 19, 2019
Merged

Transfer bans on room upgrade#4642
anoadragon453 merged 5 commits into
developfrom
anoa/bans_room_upgrade

Conversation

@anoadragon453
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Member Author

anoadragon453 commented Feb 18, 2019

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
Copy Markdown
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
Copy Markdown
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

Comment thread synapse/handlers/room.py Outdated
(EventTypes.RoomAvatar, ""),
(EventTypes.Encryption, ""),
(EventTypes.ServerACL, ""),
(EventTypes.Member, None),
Copy link
Copy Markdown
Member

@richvdh richvdh Feb 18, 2019

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
Copy Markdown
Member Author

@anoadragon453 anoadragon453 Feb 18, 2019

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
Copy Markdown
Member

@richvdh richvdh Feb 18, 2019

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
Copy Markdown
Member Author

@anoadragon453 anoadragon453 Feb 18, 2019

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

Comment thread 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
Copy Markdown
Member

@richvdh richvdh Feb 18, 2019

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

Comment thread changelog.d/4642.bugfix
@@ -0,0 +1 @@
Transfer bans on room upgrade. No newline at end of file
Copy link
Copy Markdown
Member

@richvdh richvdh Feb 18, 2019

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
Copy Markdown
Member Author

@anoadragon453 anoadragon453 Feb 18, 2019

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
Copy Markdown
Member

@richvdh richvdh Feb 18, 2019

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
Copy Markdown
Member Author

@anoadragon453 anoadragon453 Feb 18, 2019

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

1 more reviewer

@richvdh richvdh richvdh approved these changes

Reviewers whose approvals may not affect merge requirements

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