-
Notifications
You must be signed in to change notification settings - Fork 356
Spaces: Add methods to add/remove space children. #5871
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5871 +/- ##
========================================
Coverage 88.59% 88.59%
========================================
Files 363 363
Lines 102888 103144 +256
Branches 102888 103144 +256
========================================
+ Hits 91153 91382 +229
- Misses 7496 7513 +17
- Partials 4239 4249 +10 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #5871 will not alter performanceComparing Summary
|
6e626f8 to
a78df15
Compare
| server.sync_room(client, builder).await; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should move the tests to a different file 🤔
| .await | ||
| .map_err(Error::UpdateRelationship)?; | ||
| } else { | ||
| warn!("The current user doesn't have permission to set the child's parent."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should really bubble this error up or maybe or maybe add a parameter to have the client tell us what they expect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this. It's hard to know what the caller would expect with a single method, so for now we'll leave it as is. In the future we'll potentially add some parameters and can throw based on those.
| let space_service = SpaceService::new(client.clone()); | ||
|
|
||
| // Wait for the space hierarchy to register. | ||
| _ = space_service.joined_spaces().await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just move the subscription logic to a separate setup method? That's what I did for the new GroupedRoomList service and honestly I don't have a better idea at the moment 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds fair, but I think I'll leave that exercise to you (or me in the next PR if I need it for sharing the graph logic).
a78df15 to
f9e04f8
Compare
stefanceriu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for the changes, it all looks good to me now 👍
This PR makes 2 changes
SpaceService::add_child_to_spaceandSpaceService::remove_child_from_spaceto allow for spaces to be edited.SpaceService::joined_parents_of_childto allow the editing of spaces from a room/subspace context (and for setting join rules).Closes #5821