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.

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

Fix is_user_mention_enabled and is_room_mention_enabled #2357

Merged
merged 4 commits into from
Aug 1, 2023

Conversation

nimau
Copy link
Contributor

@nimau nimau commented Jul 31, 2023

With this PR:

  • is_user_mention_enabled() now returns the value of the new push rule .m.rule.is_user_mention if the rule exists. Previously, the value of the new push rule was only returned if it was enabled.
  • is_room_mention_enabled() now returns the value of the new push rule .m.rule.is_room_mention if the rule exists. Previously, the value of the new push rule was only returned if it was enabled.

The previous implementation was problematic because setting is_room_mention_enabled() to false actually makes 2 API calls:

  • One disabling the new push rule .m.rule.is_room_mention
  • One disabling the deprecated rule .m.rule.roomnotif

And the client app was seeing the room mention enabled after the first call because the previous implementation was returning the state of the deprecated rule if the new one was disabled.
In this way, the new push rule is the one used if it exists.

This PR also fixes some UnitTests for notification_settings crate.

@nimau nimau requested a review from a team as a code owner July 31, 2023 14:30
@nimau nimau requested review from jplatte and removed request for a team July 31, 2023 14:30
@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Patch coverage: 85.34% and project coverage change: +0.03% 🎉

Comparison is base (4c7e10c) 77.57% compared to head (6071a39) 77.61%.
Report is 15 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2357      +/-   ##
==========================================
+ Coverage   77.57%   77.61%   +0.03%     
==========================================
  Files         175      175              
  Lines       18471    18536      +65     
==========================================
+ Hits        14329    14386      +57     
- Misses       4142     4150       +8     
Files Changed Coverage Δ
crates/matrix-sdk-crypto/src/gossiping/machine.rs 63.58% <ø> (ø)
crates/matrix-sdk-crypto/src/lib.rs 100.00% <ø> (ø)
crates/matrix-sdk-crypto/src/requests.rs 78.66% <ø> (ø)
crates/matrix-sdk-crypto/src/store/mod.rs 78.30% <33.33%> (-1.10%) ⬇️
crates/matrix-sdk-sqlite/src/crypto_store.rs 93.33% <33.33%> (-0.94%) ⬇️
crates/matrix-sdk/src/room/mod.rs 76.12% <75.00%> (+0.22%) ⬆️
crates/matrix-sdk-ui/src/timeline/inner/state.rs 89.40% <81.81%> (+2.87%) ⬆️
crates/matrix-sdk-base/src/client.rs 88.46% <87.50%> (-0.07%) ⬇️
crates/matrix-sdk-base/src/sliding_sync.rs 86.54% <87.50%> (+0.24%) ⬆️
crates/matrix-sdk-ui/src/notification_client.rs 68.88% <92.30%> (+2.41%) ⬆️
... and 10 more

... and 3 files with indirect coverage changes

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

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Sweet! We could use a test helper, and then this would be good to go 👍

@bnjbvr bnjbvr removed the request for review from jplatte August 1, 2023 08:44
@bnjbvr bnjbvr changed the title Fix is_user_mention_enabled, is_room_mention_enabled Fix is_user_mention_enabled and is_room_mention_enabled Aug 1, 2023
@nimau nimau requested a review from bnjbvr August 1, 2023 12:12
@bnjbvr bnjbvr merged commit 6961a7f into main Aug 1, 2023
@bnjbvr bnjbvr deleted the nicolas/notification_settings_fix_mentions branch August 1, 2023 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants