Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a test for restricted rooms appearing in the spaces summary (MSC3083). #109

Merged
merged 12 commits into from
Jun 2, 2021

Conversation

clokep
Copy link
Member

@clokep clokep commented May 13, 2021

This adds some additional tests for MSC3083 for ensuring that the space summary works for restricted rooms.

See matrix-org/synapse#9922 for the Synapse implementation.

tests/msc3083_test.go Outdated Show resolved Hide resolved
tests/msc3083_test.go Outdated Show resolved Hide resolved
@clokep
Copy link
Member Author

clokep commented May 21, 2021

As with other PRs, we'll need to wait for a release before this will pass CI.

@clokep clokep requested a review from a team June 2, 2021 11:29
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

The tests look correct from a functional standpoint. I just had a couple questions as someone who's been outside the world of Spaces implementation up until recently.

tests/msc3083_test.go Show resolved Hide resolved
tests/msc3083_test.go Outdated Show resolved Hide resolved
tests/msc3083_test.go Show resolved Hide resolved
@clokep clokep requested a review from anoadragon453 June 2, 2021 15:55
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

Definitely better! One last question (and a couple corrections).

tests/msc3083_test.go Outdated Show resolved Hide resolved
tests/msc3083_test.go Outdated Show resolved Hide resolved
Comment on lines 323 to 324
// charlie joins the space and now hs2 knows that alice is in the space (and
// can join room).
Copy link
Member

Choose a reason for hiding this comment

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

My main question revolves around why hs2 needs to know alice can join the room if we're asking hs1 for the space summary. hs1 knows alice can join the room already.

My assumption is that hs1 needs to ask hs2 this question, but it'd be good to mention that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, hs1 is asking hs2 about "room" while generating the summary. I'll add this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the docstring which talks about the setup of space/room, I hope that's OK. The above is true for every time we attempt to summarize so that seemed like the best spot.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! And aha, I think my mental model had Alice creating the room, not Charlie, that's why I was confused about why hs1 didn't know about the status of the room.

That realisation and the additional notes make things fairly clear now, thanks!

clokep and others added 2 commits June 2, 2021 12:37
@clokep clokep requested a review from anoadragon453 June 2, 2021 16:39
@clokep clokep merged commit 726472e into master Jun 2, 2021
@clokep clokep deleted the clokep/restricted-rooms-in-space-summary branch June 2, 2021 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants