feat(room): add methods to customise a Room's privacy settings by jmartinesp · Pull Request #4401 · matrix-org/matrix-rust-sdk

jmartinesp · 2024-12-10T15:06:45Z

These include:

Updating the canonical alias.
Creating and removing a room alias from the room directory.
Enabling encryption in a room.
Updating the join rule of a room.
Updating the room history visiblity.
Updating the room visibility in the room directory.

Also, expose the room history visibility through the FFI RoomInfo.

Public API changes documented in changelogs (optional)

Signed-off-by:

codecov · 2024-12-10T15:21:25Z

Codecov Report

Attention: Patch coverage is 82.89474% with 13 lines in your changes missing coverage. Please review.

Project coverage is 85.35%. Comparing base (ca9eb70) to head (233036a).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk/src/test_utils/mocks.rs	72.41%	8 Missing ⚠️
crates/matrix-sdk/src/room/privacy_settings.rs	87.80%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4401      +/-   ##
==========================================
- Coverage   85.36%   85.35%   -0.01%     
==========================================
  Files         284      285       +1     
  Lines       31886    31961      +75     
==========================================
+ Hits        27219    27281      +62     
- Misses       4667     4680      +13

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

jmartinesp · 2024-12-11T14:45:47Z

    }
-
-    /// Creates a new room alias associated with the provided room id.
-    pub async fn create_room_alias(


This was just moved to a different file, to have the functions grouped.

poljar

Looking good, I left a couple of recommendations but nothing major.

poljar · 2024-12-11T14:56:24Z

 }

+impl From<NotYetImplemented> for ClientError {
+    fn from(_: NotYetImplemented) -> Self {


Since we're starting to use that more and more, I wonder if NotYetImplemented should contain more information, i.e. so we know which exact feature isn't yet implemented. (if you agree, this can be part of a separate PR)

Sounds good, but as you suggest I'd rather make this change in a new PR.

poljar · 2024-12-11T15:03:07Z

+    /// Update the canonical alias of the room.
+    ///
+    /// Note that publishing the alias in the room directory is done separately.
+    pub async fn update_canonical_alias(&self, new_alias: Option<OwnedRoomAliasId>) -> Result<()> {


We should explain what providing a None does. An example wouldn't hurt either.

poljar · 2024-12-11T15:06:09Z

+
+use crate::{Error, Result, Room};
+
+impl Room {


Since this is already in a separate module and adds a lot of functionality, I think it makes sense to introduce a "fake" struct for this that groups all the privacy settings under its umbrella.

pub struct RoomPrivacySettings<'a> { room: &'a Room } impl RoomPrivacySettings { ... }

That way you would call:

room.privacy_settings().update_canonical_alias(my_alias).await?

Take a look at the Encryption struct and module, we do the same thing there to avoid cluttering the Client object with a lot of E2EE related methods.

poljar · 2024-12-11T15:08:58Z

+        Ok(())
+    }
+
+    /// Update room history visibility for this room.


The docs on all these methods are a bit terse, it might be nice to explain briefly what the history visibility is and link to the spec for more info.

This applies to the other methods as well, an example here and there wouldn't hurt either.

poljar · 2024-12-11T15:09:34Z

+    /// use matrix_sdk::{
+    ///     ruma::room_alias_id, test_utils::mocks::MatrixMockServer,
+    /// };
+    /// use ruma::api::client::room::Visibility;
+    ///
+    /// let mock_server = MatrixMockServer::new().await;
+    /// let client = mock_server.client_builder().build().await;
+    ///
+    /// mock_server
+    ///     .mock_room_directory_remove_room_alias()
+    ///     .ok()
+    ///     .mock_once()
+    ///     .mount()
+    ///     .await;
+    ///
+    /// client
+    ///     .remove_room_alias(room_alias_id!("#a:b.c"))
+    ///     .await
+    ///     .expect("We should be able to remove the room alias");
+    /// # anyhow::Ok(()) });


Thanks a bunch for adding examples on these.

jmartinesp · 2024-12-11T17:27:21Z

It turns out this needs further work, so I'll upload some more changes tomorrow with both the needed changes and some logic changed too to take into account the public room visibility and being able to retry the whole 'publish and update alias' flow if the alias was published but the process got interrupted before.

jmartinesp · 2025-01-09T15:49:05Z

Sorry for the force push @poljar, but the code had so many changes I couldn't find a good way to make them easy to review in little steps, so in the end I just reset my commits and split the code again in smaller chunks.

poljar · 2025-01-09T15:55:29Z

Sorry for the force push @poljar, but the code had so many changes I couldn't find a good way to make them easy to review in little steps, so in the end I just reset my commits and split the code again in smaller chunks.

Hah, you're in luck, I forgot almost everything about the review anyways 😅.

poljar

Nice work, I left two final nits but then we're good to go.

poljar · 2025-01-10T11:43:10Z

+            Visibility::Public => Self::Public,
+            Visibility::Private => Self::Private,
+            Visibility::_Custom(_) => Self::Custom { value: value.to_string() },
+            _ => panic!("This visibility mapping branch should never be reached"),


Unless Visibility gets another variant. Can't we do value.as_str() and convert it into the custom variant instead?

poljar · 2025-01-10T12:07:07Z

    }

    /// Create a prebuilt mock for resolving room aliases.
+    ///


Ey, thanks for adding examples to these.

poljar · 2025-01-10T12:12:47Z

+    // Give some time for the sync to run
+    tokio::time::sleep(tokio::time::Duration::from_secs(2)).await;


I think this will be flaky, and if the sync isn't slow it it slows down the test unnecessarily. Can't we listen for the event to arrive using an event handler and use the timeout method to make sure that we're not blocking forever if the event never arrives.

c372fcb: I tried using Mutex but I couldn't get it to work, so I followed a different example which used channels and it worked fine. I wouldn't mind learning how this should be done with Mutexes though 😅 .

Err, I don't think you can do so using a mutex, you need something that's a future that resolves once you receive the event. I think a channel is perfect for this, or perhaps a shared observable could work as well.

poljar

Nice, thanks.

poljar · 2025-01-13T09:22:40Z

Oh, please don't forget to add a changelog entry.

This can be accessed through `fn Room::privacy_settings` and will wrap the functionality related to a room's access and privacy settings. This commit includes the `fn RoomPrivacySettings::update_canonical_alias` to modify the canonical alias of a room.

…ty`.

…irectory`. This also needs some new mocks for resolving room aliases.

…rectory`.

…iases

Velin92 reviewed Dec 10, 2024

View reviewed changes

Comment thread bindings/matrix-sdk-ffi/src/room.rs

jmartinesp force-pushed the feat/add-room-access-settings branch 4 times, most recently from 15d2db1 to 736f2c0 Compare December 11, 2024 11:30

jmartinesp marked this pull request as ready for review December 11, 2024 12:19

jmartinesp requested a review from a team as a code owner December 11, 2024 12:19

jmartinesp requested review from poljar and removed request for a team December 11, 2024 12:19

jmartinesp commented Dec 11, 2024

View reviewed changes

poljar requested changes Dec 11, 2024

View reviewed changes

jmartinesp force-pushed the feat/add-room-access-settings branch from 736f2c0 to 6774747 Compare January 9, 2025 15:47

poljar requested changes Jan 10, 2025

View reviewed changes

jmartinesp requested a review from poljar January 10, 2025 16:16

poljar approved these changes Jan 10, 2025

View reviewed changes

jmartinesp added 9 commits January 13, 2025 11:10

feat(room): Add `fn RoomPrivacySettings::update_room_history_visibili…

7a4ed4f

…ty`.

feat(room): Add fn RoomPrivacySettings::update_join_rule.

4a89766

feat(room): Add fn RoomPrivacySettings::get_room_visibility.

4697037

feat(room): Add fn RoomPrivacySettings::update_room_visibility.

83acb33

feat(room): Add `fn RoomPrivacySettings::publish_room_alias_in_room_d…

ca2a7c4

…irectory`. This also needs some new mocks for resolving room aliases.

feat(room): Add `fn RoomPrivacySettings::remove_room_alias_in_room_di…

1ec3b26

…rectory`.

test(room): Add integration tests for publishing and removing room al…

0d97aac

…iases

feat(ffi): Add FFI bindings for the new room privacy settings feature.

233036a

jmartinesp force-pushed the feat/add-room-access-settings branch from c372fcb to 233036a Compare January 13, 2025 10:14

jmartinesp enabled auto-merge (rebase) January 13, 2025 10:24

jmartinesp merged commit 5548f38 into main Jan 13, 2025

jmartinesp deleted the feat/add-room-access-settings branch January 13, 2025 10:29

		// Give some time for the sync to run
		tokio::time::sleep(tokio::time::Duration::from_secs(2)).await;

Conversation

jmartinesp commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poljar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmartinesp commented Dec 11, 2024

Uh oh!

jmartinesp commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poljar commented Jan 9, 2025

Uh oh!

poljar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poljar left a comment

Choose a reason for hiding this comment

Uh oh!

poljar commented Jan 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmartinesp commented Dec 10, 2024 •

edited

Loading

codecov Bot commented Dec 10, 2024 •

edited

Loading

jmartinesp commented Jan 9, 2025 •

edited

Loading