Fixes Changing Account Settings > Notifications > Advanced Notifications on android causes discrepancies with web #3681
Conversation
|
This fix also needs to be incorporated into the V2 logic in #3673 |
| @@ -130,10 +130,11 @@ class PushRulePreference : VectorPreference { | |||
| } | |||
| } else { | |||
| if (NOTIFICATION_OFF_INDEX == index) { | |||
| if (safeKind == RuleSetKey.UNDERRIDE || safeRule.ruleId == RuleIds.RULE_ID_SUPPRESS_BOTS_NOTIFICATIONS) { | |||
| if (safeKind == RuleSetKey.UNDERRIDE && safeRule.ruleId != RuleIds.RULE_ID_CALL) { | |||
There was a problem hiding this comment.
I understand that it was not possible for safeRule.ruleId == RuleIds.RULE_ID_SUPPRESS_BOTS_NOTIFICATIONS to be true here. But why checking RULE_ID_CALL now?
To be honest, this code is really hard to follow, it misses some comment... :/
| @@ -142,7 +143,7 @@ class PushRulePreference : VectorPreference { | |||
| && safeRule.ruleId != RuleIds.RULE_ID_INVITE_ME | |||
| && NOTIFICATION_NOISY_INDEX == index) | |||
|
|
|||
| if (NOTIFICATION_NOISY_INDEX == index) { | |||
| if (NOTIFICATION_NOISY_INDEX == index && RuleIds.RULE_ID_ROOM_NOTIF != safeRule.ruleId) { | |||
There was a problem hiding this comment.
Same remark, should be nice to have some comment to explain why this code is necessary.
…eature/dla/fix_account_notifications_discrepancies
matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/pushrules/PushRuleService.kt
Show resolved
Hide resolved
| * Get the highlight status. As spec mentions assume false if no tweak present. | ||
| */ | ||
| fun getHighlight(): Boolean { | ||
| return (getActions().firstOrNull { it is Action.Highlight } as? Action.Highlight)?.highlight ?: false |
There was a problem hiding this comment.
You can rewrite to
return getActions().filterIsInstance<Action.Highlight>().firstOrNull()?.highlight.orFalse()
| enum class NotificationIndex(val index: Int) { | ||
| OFF(0), | ||
| SILENT(1), | ||
| NOISEY(2); |
|
|
||
| import org.matrix.android.sdk.api.pushrules.Action | ||
|
|
||
| sealed class StandardAction( |
| @@ -104,7 +105,7 @@ data class PushRule( | |||
| * Get the highlight status. As spec mentions assume false if no tweak present. | |||
| */ | |||
| fun getHighlight(): Boolean { | |||
| return (getActions().firstOrNull { it is Action.Highlight } as? Action.Highlight)?.highlight ?: false | |||
| return (getActions().firstOrNull { it is Action.Highlight } as? Action.Highlight)?.highlight.orFalse() | |||
There was a problem hiding this comment.
Why you did not take my whole suggestion?
There was a problem hiding this comment.
As sorry, read it too quickly
…eature/dla/fix_account_notifications_discrepancies
Fixes #3672
This PR changes the existing logic, which continuously mutated the push rule definitions, updating them by trying to generalise the behaviour based on the
PushRuleKindand handling exceptions to the definitions, overriding using theRuleId. The new implementation matches that on web, by having a static definition for each notification type.The new StandardAction matches that on the web. As do the PushRuleDefinitions match those on the web.
I also updated the SDK to match the behaviour of the web based on the static rule definitions, whereby
enabledrequest is always sent andactionsrequest is sent if we are updating the actions(i.e.enabled==true).I tested selecting each radio option, for each setting from android and they all match the requests(/reflect correctly in the ui) as on web.