-
Notifications
You must be signed in to change notification settings - Fork 263
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
notification client: use the membership state to match an invite #4000
notification client: use the membership state to match an invite #4000
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4000 +/- ##
==========================================
- Coverage 84.27% 84.23% -0.05%
==========================================
Files 266 266
Lines 28315 28328 +13
==========================================
- Hits 23863 23862 -1
- Misses 4452 4466 +14 ☔ View full report in Codecov by Sentry. |
67b10a0
to
59553d6
Compare
|
||
// Try to match the event by membership and state_key for the current user. | ||
if deserialized.content.membership == MembershipState::Invite | ||
&& deserialized.state_key == user_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.
I'm not sure it's gonna work with the latest version of Ruma. Either you transform user_id
into the type of state_key
, or you extract the user ID from state_key
with state_key.user_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.
This is rebased on main, so should be fine.
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.
(It's not a call room member event, but a general room member event.)
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.
Alright.
The notification client was relying on the SS proxy including the
event_id
of a stripped room membership event, to correctly match an invite. This is not the case anymore with SSS (in the spec, stripped events don't include an event_id, although I don't see any good reason why), so we have to resort to a different mechanism:With this change,
matrix-sdk-integration-testing tests::sliding_sync::notification_client::test_notification
integration test from #3983 passes correctly.