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.

Sign up for GitHub

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

Jump to bottom

room preview: use the room summary MSC3266 endpoint #3339

Merged
merged 4 commits into from
Apr 22, 2024
Merged

room preview: use the room summary MSC3266 endpoint #3339

merged 4 commits into from
Apr 22, 2024

Conversation

bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Apr 18, 2024 •
edited
Loading

This adds support for MSC3266 when fetching a room preview, for servers that support it. If the server doesn't support it, we resort to using the room state event endpoint.

Pretty sure the integration testing in CI will hate this, since I'm unit-testing the from_room_summary method, which requires an experimental feature to be enabled on the synapse service. If that's the case, I'll disable this test.

Using a fork of Ruma to cut corners, since the review of the feature there is getting complicated. Maybe ruma/ruma#1776 will get approved some day soon 🥲

@bnjbvr bnjbvr requested a review from a team as a code owner April 18, 2024 13:07
@bnjbvr bnjbvr requested review from Hywan and removed request for a team April 18, 2024 13:07
@bnjbvr bnjbvr changed the title room preview: implement the room preview: use the room summary endpoint to retrieve information for more rooms Apr 18, 2024
@bnjbvr
Copy link
Member Author

bnjbvr commented Apr 18, 2024

Pretty sure the integration testing in CI will hate this, since I'm unit-testing the from_room_summary method, which requires an experimental feature to be enabled on the synapse service. If that's the case, I'll disable this test.

Bingo, so disabling the test on CI.

@bnjbvr bnjbvr force-pushed the bnjbvr/room-preview2 branch from f06855c to d128cb5 Compare April 18, 2024 13:39
@bnjbvr bnjbvr mentioned this pull request Apr 18, 2024
Closed
@codecov Codecov
Copy link

codecov bot commented Apr 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 27.27273% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 83.55%. Comparing base (4325812) to head (a57b003).
Report is 12 commits behind head on main.

Files Patch % Lines
crates/matrix-sdk/src/room_preview.rs 33.33% 18 Missing ⚠️
crates/matrix-sdk/src/http_client/native.rs 0.00% 6 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3339      +/-   ##
==========================================
- Coverage   83.63%   83.55%   -0.09%     
==========================================
  Files         241      241              
  Lines       24873    24899      +26     
==========================================
+ Hits        20803    20804       +1     
- Misses       4070     4095      +25

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bnjbvr bnjbvr changed the title room preview: use the room summary endpoint to retrieve information for more rooms room preview: use the room summary MSC3266 endpoint for room preview Apr 19, 2024
@bnjbvr bnjbvr changed the title room preview: use the room summary MSC3266 endpoint for room preview room preview: use the room summary MSC3266 endpoint Apr 19, 2024
@bnjbvr bnjbvr force-pushed the bnjbvr/room-preview2 branch from d128cb5 to fc2d217 Compare April 19, 2024 15:23
bnjbvr added 4 commits April 22, 2024 10:28
@bnjbvr
dependencies: use a fork of Ruma for the time being
9f672ca
@bnjbvr
dependencies: bump ruma
e24d5a2 
Honestly, I'm not sure how the retry-after that's a fixed point in time
in the future should be handled, open to suggestions here…
@bnjbvr
room preview: add support for MSC3266, room summary
4ce1377
@bnjbvr
test: silently skip the room summary test on CI, if the server doesn'…
a57b003 
…t support it
@bnjbvr bnjbvr force-pushed the bnjbvr/room-preview2 branch from fc2d217 to a57b003 Compare April 22, 2024 08:28
@bnjbvr
Copy link
Member Author

bnjbvr commented Apr 22, 2024

@Hywan Can haz review, plz? Okthxbye 🥺

@Hywan
Copy link
Member

Hywan commented Apr 22, 2024

ruma/ruma#1776 has been merged 😉.

Hywan
Hywan approved these changes Apr 22, 2024
View reviewed changes
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Please fix the repository of Ruma before merging this PR.

Cargo.toml
@@ -39,15 +39,16 @@ futures-util = { version = "0.3.26", default-features = false, features = [
http = "0.2.6"
imbl = "2.0.0"
itertools = "0.12.0"
ruma = { git = "https://github.com/ruma/ruma", rev = "4c00bd010dbdca6005bd599b52e90a0b7015d056", features = [
ruma = { git = "https://github.com/ruma/ruma", rev = "21b644ac6ae1c7d4a4f7e98a6481a3318f2deeaa", features = [
Copy link
Member

Choose a reason for hiding this comment

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

Please use the original repository since your PR has been merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed to use the original repo, which I've done in a commit this morning,… so there's nothing to change, right? 😁

@bnjbvr bnjbvr merged commit 289e2ac into main Apr 22, 2024
35 checks passed
@bnjbvr bnjbvr deleted the bnjbvr/room-preview2 branch April 22, 2024 12:55
@marwing marwing mentioned this pull request May 9, 2024
Closed
@ara4n ara4n mentioned this pull request Jul 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hywan Hywan Hywan 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.
2 participants
@bnjbvr @Hywan