-
Notifications
You must be signed in to change notification settings - Fork 270
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
FFI Element Call: Fix the permission list returned by the get_element_call_required_permissions
ffi function.
#3814
FFI Element Call: Fix the permission list returned by the get_element_call_required_permissions
ffi function.
#3814
Conversation
…t_call_required_permissions` ffi function.
01dd246
to
c6a7405
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3814 +/- ##
==========================================
+ Coverage 84.08% 84.09% +0.01%
==========================================
Files 262 262
Lines 27434 27434
==========================================
+ Hits 23067 23070 +3
+ Misses 4367 4364 -3 ☔ View full report in Codecov by Sentry. |
WidgetEventFilter::StateWithTypeAndStateKey { | ||
event_type: StateEventType::CallMember.to_string(), | ||
state_key: own_user_id, | ||
state_key: format!("{}_{}", own_user_id, own_device_id), |
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.
Could be:
state_key: format!("{}_{}", own_user_id, own_device_id), | |
state_key: format!("{own_user_id}_{own_device_id}"), |
Probably more Rust idiomatic.
// but we use mxId+deviceId. | ||
WidgetEventFilter::StateWithTypeAndStateKey { | ||
event_type: StateEventType::CallMember.to_string(), | ||
state_key: format!("_{}_{}", own_user_id, own_device_id), |
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.
state_key: format!("_{}_{}", own_user_id, own_device_id), | |
state_key: format!("_{own_user_id}_{own_device_id}"), |
@@ -535,3 +540,54 @@ impl From<url::ParseError> for ParseError { | |||
} | |||
} | |||
} | |||
|
|||
#[cfg(test)] | |||
mod tests { |
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.
Tests from matrix-sdk-ffi
aren't run in the CI as far as I remember. Is it possible to move some of this logic inside matrix-sdk
maybe? I understand though that the code you're testing are specific to Element, and should not leave in matrix-sdk
itself 🤔.
We may want to run the tests from matrix-sdk-ffi
inside the CI then.
Can you open an issue, or better, a pull request, to not forget about that please?
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.
@toger5 is on holidays. I'm taking over this PR.
Before we set them for reading events which is not required since:
already covers all keys. And an additional
@
was used resulting in:@@user_id:demain.org_DEVICEID
and_@@user_id:demain.org_DEVICEID
.This also made it obvious that we need unit tests for this ffi method!
(We currently use the tauri widget wrapper to do manual end to end tests for the widget driver. But the ffi layer is not part of the tauri app. The testing cycle for EX is cumbersome without merging the changes.
Offtopic: any ideas or recommendations how to mitigate this in the future are welcome)
Signed-off-by: