sdk: share room history when we send an invite #4946
Conversation
... subject to an experimental feature flag.
... otherwise, it fails with an error, which makes the integ tests fail
richvdh
force-pushed
the
rav/history_sharing/share_on_invite
branch
from
31c7a99 to
18f20a7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4946 +/- ##
=======================================
Coverage 85.92% 85.93%
=======================================
Files 324 324
Lines 35614 35619 +5
=======================================
+ Hits 30603 30610 +7
+ Misses 5011 5009 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| #[cfg(all(feature = "experimental-share-history-on-invite", feature = "e2e-encryption"))] | ||
| { | ||
| shared_room_history::share_room_history(self, user_id.to_owned()).await?; | ||
| } | ||
|
|
There was a problem hiding this comment.
Hmm, are we certain that people won't want to have a way to say "Ok, invite this person, but absolutely don't share the room history with them"?
I can see that we might want to do this by default, but I can also see that some people might want to opt out.
This might be a nice place to introduce a named future, which would allow us to:
- Make this configurable
- Not break the API despite having newly added configuration options.
I don't mean that we should do this right now, but perhaps down the line as we stabilize support for this feature.
There was a problem hiding this comment.
When we've discussed this in the past, we have talked about not wanting different people to have different views of a room, especially because if the person with incomplete history then invites another person, they also have incomplete history, and it gets very confusing.
So I think we don't want this option.
There was a problem hiding this comment.
Hmm, I guess that weakens my argument towards "the SDK shouldn't be as opinionated as the Element clients".
But ok, I think we can revisit this and discuss later if the SDK should make this optional.
| @@ -1480,6 +1477,11 @@ impl Room { | |||
| /// * `user_id` - The `UserId` of the user to invite to the room. | |||
| #[instrument(skip_all)] | |||
| pub async fn invite_user_by_id(&self, user_id: &UserId) -> Result<()> { | |||
| #[cfg(all(feature = "experimental-share-history-on-invite", feature = "e2e-encryption"))] | |||
There was a problem hiding this comment.
There are other ways to invite a user, don't we need to take care of them as well?
There was a problem hiding this comment.
Like what? invite_user_by_3pid? Ideally yes, but this flow isn't going to work there, because we don't have a matrix ID for the target user. We ought to think about it at some point, but I think it's well out of scope for this prototyping effort. I've added a note at element-hq/element-meta#2685 (comment).
... subject to an experimental feature flag.
Fixes: #4511