From 0d28b3a8600d3294a9e703b101343ee4dee776e8 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Fri, 17 Feb 2023 09:59:17 +0100 Subject: [PATCH 1/8] Handle errors when updating push rules --- .../src/main/res/values/strings.xml | 2 + .../preference/VectorCheckboxPreference.kt | 7 ++ .../ConfigureAndStartSessionUseCase.kt | 2 +- .../notifications/PushRuleDefinitions.kt | 3 +- ...torSettingsPushRuleNotificationFragment.kt | 44 +++++--- ...orSettingsPushRuleNotificationViewEvent.kt | 2 +- ...orSettingsPushRuleNotificationViewModel.kt | 48 +++++++-- ...orSettingsPushRuleNotificationViewState.kt | 1 + .../GetPushRulesOnInvalidStateUseCase.kt | 44 ++++++++ .../usecase/UpdatePushRulesIfNeededUseCase.kt | 18 ++-- .../vector_settings_notification_default.xml | 2 +- ...ttingsPushRuleNotificationViewModelTest.kt | 29 ++--- .../GetPushRulesOnInvalidStateUseCaseTest.kt | 100 ++++++++++++++++++ .../UpdatePushRulesIfNeededUseCaseTest.kt | 54 +++++----- .../app/test/fixtures/PushRuleFixture.kt | 49 +++++++++ 15 files changed, 325 insertions(+), 80 deletions(-) create mode 100644 vector/src/main/java/im/vector/app/features/settings/notifications/usecase/GetPushRulesOnInvalidStateUseCase.kt create mode 100644 vector/src/test/java/im/vector/app/features/settings/notifications/usecase/GetPushRulesOnInvalidStateUseCaseTest.kt create mode 100644 vector/src/test/java/im/vector/app/test/fixtures/PushRuleFixture.kt diff --git a/library/ui-strings/src/main/res/values/strings.xml b/library/ui-strings/src/main/res/values/strings.xml index e06355a173..2ee623cf88 100644 --- a/library/ui-strings/src/main/res/values/strings.xml +++ b/library/ui-strings/src/main/res/values/strings.xml @@ -862,6 +862,8 @@ Keywords cannot start with \'.\' Keywords cannot contain \'%s\' + An error occurred when updating your notification preferences. Please try again. + Troubleshoot Notifications Troubleshooting diagnostics Run Tests diff --git a/vector/src/main/java/im/vector/app/core/preference/VectorCheckboxPreference.kt b/vector/src/main/java/im/vector/app/core/preference/VectorCheckboxPreference.kt index 13b65e11b4..355bab8da8 100644 --- a/vector/src/main/java/im/vector/app/core/preference/VectorCheckboxPreference.kt +++ b/vector/src/main/java/im/vector/app/core/preference/VectorCheckboxPreference.kt @@ -32,6 +32,8 @@ class VectorCheckboxPreference : CheckBoxPreference { constructor(context: Context) : super(context) + var summaryTextColor: Int? = null + init { // Set to false to remove the space when there is no icon isIconSpaceReserved = true @@ -42,4 +44,9 @@ class VectorCheckboxPreference : CheckBoxPreference { (holder.findViewById(android.R.id.title) as? TextView)?.isSingleLine = false super.onBindViewHolder(holder) } + + override fun syncSummaryView(holder: PreferenceViewHolder) { + super.syncSummaryView(holder) + summaryTextColor?.let { (holder.findViewById(android.R.id.summary) as? TextView)?.setTextColor(it) } + } } diff --git a/vector/src/main/java/im/vector/app/core/session/ConfigureAndStartSessionUseCase.kt b/vector/src/main/java/im/vector/app/core/session/ConfigureAndStartSessionUseCase.kt index b9573e9292..8a9fc9729d 100644 --- a/vector/src/main/java/im/vector/app/core/session/ConfigureAndStartSessionUseCase.kt +++ b/vector/src/main/java/im/vector/app/core/session/ConfigureAndStartSessionUseCase.kt @@ -52,7 +52,7 @@ class ConfigureAndStartSessionUseCase @Inject constructor( updateMatrixClientInfoIfNeeded(session) createNotificationSettingsAccountDataIfNeeded(session) notificationsSettingUpdater.onSessionStarted(session) - pushRulesUpdater.onSessionStarted(session) +// pushRulesUpdater.onSessionStarted(session) } private fun updateMatrixClientInfoIfNeeded(session: Session) { diff --git a/vector/src/main/java/im/vector/app/features/settings/notifications/PushRuleDefinitions.kt b/vector/src/main/java/im/vector/app/features/settings/notifications/PushRuleDefinitions.kt index fff6ffe933..9e19e2afb4 100644 --- a/vector/src/main/java/im/vector/app/features/settings/notifications/PushRuleDefinitions.kt +++ b/vector/src/main/java/im/vector/app/features/settings/notifications/PushRuleDefinitions.kt @@ -42,8 +42,7 @@ fun getStandardAction(ruleId: String, index: NotificationIndex): StandardActions RuleIds.RULE_ID_POLL_START_ONE_TO_ONE, RuleIds.RULE_ID_POLL_START_ONE_TO_ONE_UNSTABLE, RuleIds.RULE_ID_POLL_END_ONE_TO_ONE, - RuleIds.RULE_ID_POLL_END_ONE_TO_ONE_UNSTABLE, - -> + RuleIds.RULE_ID_POLL_END_ONE_TO_ONE_UNSTABLE -> when (index) { NotificationIndex.OFF -> StandardActions.DontNotify NotificationIndex.SILENT -> StandardActions.Notify diff --git a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationFragment.kt b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationFragment.kt index 0f991867fe..8a2099ecad 100644 --- a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationFragment.kt +++ b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationFragment.kt @@ -21,33 +21,35 @@ import android.view.View import androidx.preference.Preference import com.airbnb.mvrx.fragmentViewModel import com.airbnb.mvrx.withState +import im.vector.app.R import im.vector.app.core.preference.VectorCheckboxPreference import im.vector.app.features.settings.VectorSettingsBaseFragment +import im.vector.app.features.themes.ThemeUtils abstract class VectorSettingsPushRuleNotificationFragment : VectorSettingsBaseFragment() { - private val viewModel: VectorSettingsPushRuleNotificationViewModel by fragmentViewModel() + protected val viewModel: VectorSettingsPushRuleNotificationViewModel by fragmentViewModel() abstract val prefKeyToPushRuleId: Map override fun onViewCreated(view: View, savedInstanceState: Bundle?) { super.onViewCreated(view, savedInstanceState) observeViewEvents() - viewModel.onEach(VectorSettingsPushRuleNotificationViewState::isLoading) { isLoading -> - if (isLoading) { - displayLoadingView() - } else { - hideLoadingView() - } - } + viewModel.onEach(VectorSettingsPushRuleNotificationViewState::isLoading) { updateLoadingView(it) } + viewModel.onEach(VectorSettingsPushRuleNotificationViewState::rulesOnError) { refreshErrors(it) } } private fun observeViewEvents() { viewModel.observeViewEvents { when (it) { - is VectorSettingsPushRuleNotificationViewEvent.Failure -> refreshDisplay() - is VectorSettingsPushRuleNotificationViewEvent.PushRuleUpdated -> updatePreference(it.ruleId, it.checked) + is VectorSettingsPushRuleNotificationViewEvent.Failure -> onFailure(it.ruleId) + is VectorSettingsPushRuleNotificationViewEvent.PushRuleUpdated -> { + updatePreference(it.ruleId, it.checked) + if (it.failure != null) { + onFailure(it.ruleId) + } + } } } } @@ -71,14 +73,28 @@ abstract class VectorSettingsPushRuleNotificationFragment : } } - override fun invalidate() = withState(viewModel) { state -> - if (state.isLoading) { + private fun updateLoadingView(isLoading: Boolean) { + if (isLoading) { displayLoadingView() } else { hideLoadingView() } } + private fun refreshErrors(rulesWithError: Set) { + if (withState(viewModel, VectorSettingsPushRuleNotificationViewState::isLoading)) return + prefKeyToPushRuleId.forEach { (preferenceKey, ruleId) -> + val preference = findPreference(preferenceKey)!! + if (ruleId in rulesWithError) { + preference.summaryTextColor = context?.let { ThemeUtils.getColor(it, R.attr.colorError) } + preference.setSummary(R.string.settings_notification_error_on_update) + } else { + preference.summaryTextColor = null + preference.summary = null + } + } + } + protected fun refreshDisplay() { listView?.adapter?.notifyDataSetChanged() } @@ -88,4 +104,8 @@ abstract class VectorSettingsPushRuleNotificationFragment : val preference = findPreference(preferenceKey) ?: return preference.isChecked = checked } + + protected open fun onFailure(ruleId: String) { + refreshDisplay() + } } diff --git a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewEvent.kt b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewEvent.kt index adfc17f827..490a28c0b0 100644 --- a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewEvent.kt +++ b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewEvent.kt @@ -33,5 +33,5 @@ sealed interface VectorSettingsPushRuleNotificationViewEvent : VectorViewEvents * * @property throwable the related exception, if any. */ - data class Failure(val throwable: Throwable?) : VectorSettingsPushRuleNotificationViewEvent + data class Failure(val ruleId: String, val throwable: Throwable?) : VectorSettingsPushRuleNotificationViewEvent } diff --git a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModel.kt b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModel.kt index 39969ec13e..eee199d550 100644 --- a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModel.kt +++ b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModel.kt @@ -26,13 +26,17 @@ import im.vector.app.core.di.hiltMavericksViewModelFactory import im.vector.app.core.platform.VectorViewModel import im.vector.app.features.settings.notifications.VectorSettingsPushRuleNotificationViewEvent.Failure import im.vector.app.features.settings.notifications.VectorSettingsPushRuleNotificationViewEvent.PushRuleUpdated +import im.vector.app.features.settings.notifications.usecase.GetPushRulesOnInvalidStateUseCase import kotlinx.coroutines.launch import org.matrix.android.sdk.api.failure.Failure.ServerError import org.matrix.android.sdk.api.failure.MatrixError +import org.matrix.android.sdk.api.session.accountdata.UserAccountDataTypes import org.matrix.android.sdk.api.session.pushrules.Action import org.matrix.android.sdk.api.session.pushrules.RuleIds import org.matrix.android.sdk.api.session.pushrules.RuleKind import org.matrix.android.sdk.api.session.pushrules.rest.PushRuleAndKind +import org.matrix.android.sdk.flow.flow +import org.matrix.android.sdk.flow.unwrap private typealias ViewModel = VectorSettingsPushRuleNotificationViewModel private typealias ViewState = VectorSettingsPushRuleNotificationViewState @@ -40,6 +44,7 @@ private typealias ViewState = VectorSettingsPushRuleNotificationViewState class VectorSettingsPushRuleNotificationViewModel @AssistedInject constructor( @Assisted initialState: ViewState, private val activeSessionHolder: ActiveSessionHolder, + private val getPushRulesOnInvalidStateUseCase: GetPushRulesOnInvalidStateUseCase, ) : VectorViewModel(initialState) { @@ -51,6 +56,17 @@ class VectorSettingsPushRuleNotificationViewModel @AssistedInject constructor( companion object : MavericksViewModelFactory by hiltMavericksViewModelFactory() + init { + val session = activeSessionHolder.getSafeActiveSession() + session?.flow() + ?.liveUserAccountData(UserAccountDataTypes.TYPE_PUSH_RULES) + ?.unwrap() + ?.setOnEach { + val rulesOnError = getPushRulesOnInvalidStateUseCase.execute(session).map { it.ruleId }.toSet() + copy(rulesOnError = rulesOnError) + } + } + override fun handle(action: VectorSettingsPushRuleNotificationViewAction) { when (action) { is VectorSettingsPushRuleNotificationViewAction.UpdatePushRule -> handleUpdatePushRule(action.pushRuleAndKind, action.checked) @@ -73,6 +89,7 @@ class VectorSettingsPushRuleNotificationViewModel @AssistedInject constructor( val standardAction = getStandardAction(ruleId, newIndex) ?: return val enabled = standardAction != StandardActions.Disabled val newActions = standardAction.actions + setState { copy(isLoading = true) } viewModelScope.launch { @@ -82,22 +99,31 @@ class VectorSettingsPushRuleNotificationViewModel @AssistedInject constructor( updatePushRule(kind, ruleId, enabled, newActions) } } - setState { copy(isLoading = false) } - val failure = results.firstNotNullOfOrNull { result -> + + val failures = results.mapNotNull { result -> // If the failure is a rule not found error, do not consider it result.exceptionOrNull()?.takeUnless { it is ServerError && it.error.code == MatrixError.M_NOT_FOUND } } - val newChecked = if (checked) { - // If any rule is checked, the global rule is checked - results.any { it.isSuccess } + val hasSuccess = results.any { it.isSuccess } + val hasFailures = failures.isNotEmpty() + + // Any rule has been checked or some rules have not been unchecked + val newChecked = (checked && hasSuccess) || (!checked && hasFailures) + if (hasSuccess) { + _viewEvents.post(PushRuleUpdated(ruleId, newChecked, failures.firstOrNull())) } else { - // If any rule has not been unchecked, the global rule remains checked - failure != null + _viewEvents.post(Failure(ruleId, failures.firstOrNull())) } - if (results.any { it.isSuccess }) { - _viewEvents.post(PushRuleUpdated(ruleId, newChecked, failure)) - } else { - _viewEvents.post(Failure(failure)) + + setState { + copy( + isLoading = false, + rulesOnError = when { + hasSuccess && hasFailures -> rulesOnError.plus(ruleId) // some failed + hasSuccess -> rulesOnError.minus(ruleId) // all succeed + else -> rulesOnError // all failed + } + ) } } } diff --git a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewState.kt b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewState.kt index 477727aee6..392a8f3bb5 100644 --- a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewState.kt +++ b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewState.kt @@ -20,4 +20,5 @@ import com.airbnb.mvrx.MavericksState data class VectorSettingsPushRuleNotificationViewState( val isLoading: Boolean = false, + val rulesOnError: Set = emptySet() ) : MavericksState diff --git a/vector/src/main/java/im/vector/app/features/settings/notifications/usecase/GetPushRulesOnInvalidStateUseCase.kt b/vector/src/main/java/im/vector/app/features/settings/notifications/usecase/GetPushRulesOnInvalidStateUseCase.kt new file mode 100644 index 0000000000..225be1ee62 --- /dev/null +++ b/vector/src/main/java/im/vector/app/features/settings/notifications/usecase/GetPushRulesOnInvalidStateUseCase.kt @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package im.vector.app.features.settings.notifications.usecase + +import org.matrix.android.sdk.api.extensions.orTrue +import org.matrix.android.sdk.api.session.Session +import org.matrix.android.sdk.api.session.pushrules.RuleIds +import org.matrix.android.sdk.api.session.pushrules.getActions +import org.matrix.android.sdk.api.session.pushrules.getParentRule +import org.matrix.android.sdk.api.session.pushrules.getSyncedRules +import org.matrix.android.sdk.api.session.pushrules.rest.PushRule +import javax.inject.Inject + +class GetPushRulesOnInvalidStateUseCase @Inject constructor() { + + fun execute(session: Session): List { + val allRules = session.pushRuleService().getPushRules().getAllRules() + return allRules.filter { it.isOnInvalidState(allRules) } + } + + private fun PushRule.isOnInvalidState(allRules: List): Boolean { + val parent = RuleIds.getParentRule(ruleId)?.let { parentId -> allRules.find { it.ruleId == parentId } } + val children = RuleIds.getSyncedRules(ruleId).mapNotNull { childId -> allRules.find { it.ruleId == childId } } + val isAlignedWithParent = parent?.let { isAlignedWithParentRule(it) }.orTrue() + return !isAlignedWithParent || !isAlignedWithChildrenRules(children) + } + + private fun PushRule.isAlignedWithParentRule(parent: PushRule) = this.getActions() == parent.getActions() && this.enabled == parent.enabled + private fun PushRule.isAlignedWithChildrenRules(children: List) = children.all { it.isAlignedWithParentRule(this) } +} diff --git a/vector/src/main/java/im/vector/app/features/settings/notifications/usecase/UpdatePushRulesIfNeededUseCase.kt b/vector/src/main/java/im/vector/app/features/settings/notifications/usecase/UpdatePushRulesIfNeededUseCase.kt index 6998065f01..12fc3d480d 100644 --- a/vector/src/main/java/im/vector/app/features/settings/notifications/usecase/UpdatePushRulesIfNeededUseCase.kt +++ b/vector/src/main/java/im/vector/app/features/settings/notifications/usecase/UpdatePushRulesIfNeededUseCase.kt @@ -24,18 +24,18 @@ import org.matrix.android.sdk.api.session.pushrules.rest.PushRule import org.matrix.android.sdk.api.session.pushrules.rest.PushRuleAndKind import javax.inject.Inject -class UpdatePushRulesIfNeededUseCase @Inject constructor() { +class UpdatePushRulesIfNeededUseCase @Inject constructor( + private val getPushRulesOnInvalidStateUseCase: GetPushRulesOnInvalidStateUseCase, +) { suspend fun execute(session: Session) { + val rulesOnError = getPushRulesOnInvalidStateUseCase.execute(session).takeUnless { it.isEmpty() } ?: return + val ruleSet = session.pushRuleService().getPushRules() - val pushRules = ruleSet.getAllRules() - val rulesToUpdate = pushRules.mapNotNull { rule -> - val parent = RuleIds.getParentRule(rule.ruleId)?.let { ruleId -> ruleSet.findDefaultRule(ruleId) } - if (parent != null && (rule.enabled != parent.pushRule.enabled || rule.actions != parent.pushRule.actions)) { - PushRuleWithParent(rule, parent) - } else { - null - } + val rulesToUpdate = rulesOnError.mapNotNull { rule -> + RuleIds.getParentRule(rule.ruleId) + ?.let { ruleId -> ruleSet.findDefaultRule(ruleId) } + ?.let { PushRuleWithParent(rule, it) } } rulesToUpdate.forEach { diff --git a/vector/src/main/res/xml/vector_settings_notification_default.xml b/vector/src/main/res/xml/vector_settings_notification_default.xml index fb565d2230..9319ef2b39 100644 --- a/vector/src/main/res/xml/vector_settings_notification_default.xml +++ b/vector/src/main/res/xml/vector_settings_notification_default.xml @@ -25,4 +25,4 @@ android:title="@string/settings_encrypted_group_messages" /> - \ No newline at end of file + diff --git a/vector/src/test/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModelTest.kt b/vector/src/test/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModelTest.kt index 04a22bc21f..7994d173b6 100644 --- a/vector/src/test/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModelTest.kt +++ b/vector/src/test/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModelTest.kt @@ -17,7 +17,9 @@ package im.vector.app.features.settings.notifications import com.airbnb.mvrx.test.MavericksTestRule +import im.vector.app.features.settings.notifications.usecase.GetPushRulesOnInvalidStateUseCase import im.vector.app.test.fakes.FakeActiveSessionHolder +import im.vector.app.test.fixtures.PushRuleFixture import im.vector.app.test.test import im.vector.app.test.testDispatcher import io.mockk.coVerifyOrder @@ -41,16 +43,19 @@ internal class VectorSettingsPushRuleNotificationViewModelTest { private val fakeActiveSessionHolder = FakeActiveSessionHolder() private val fakePushRuleService = fakeActiveSessionHolder.fakeSession.fakePushRuleService + private val fakeGetPushRulesOnInvalidStateUseCase = mockk() private val initialState = VectorSettingsPushRuleNotificationViewState() private fun createViewModel() = VectorSettingsPushRuleNotificationViewModel( initialState = initialState, activeSessionHolder = fakeActiveSessionHolder.instance, + fakeGetPushRulesOnInvalidStateUseCase, ) @Before fun setup() { mockkStatic("im.vector.app.features.settings.notifications.NotificationIndexKt") + every { fakeGetPushRulesOnInvalidStateUseCase.execute(any()) } returns emptyList() } @After @@ -98,8 +103,8 @@ internal class VectorSettingsPushRuleNotificationViewModelTest { { copy(isLoading = false) }, ) .assertEvents( - VectorSettingsPushRuleNotificationViewEvent.PushRuleUpdated(RuleIds.RULE_ID_ONE_TO_ONE_ROOM, true), - VectorSettingsPushRuleNotificationViewEvent.PushRuleUpdated(RuleIds.RULE_ID_ALL_OTHER_MESSAGES_ROOMS, false), + VectorSettingsPushRuleNotificationViewEvent.PushRuleUpdated(firstRuleId, true), + VectorSettingsPushRuleNotificationViewEvent.PushRuleUpdated(secondRuleId, false), ) .finish() } @@ -149,13 +154,13 @@ internal class VectorSettingsPushRuleNotificationViewModelTest { .assertStatesChanges( initialState, { copy(isLoading = true) }, - { copy(isLoading = false) }, + { copy(isLoading = false, setOf(firstRuleId)) }, { copy(isLoading = true) }, { copy(isLoading = false) }, ) .assertEvents( - VectorSettingsPushRuleNotificationViewEvent.PushRuleUpdated(RuleIds.RULE_ID_ONE_TO_ONE_ROOM, true, failure), - VectorSettingsPushRuleNotificationViewEvent.Failure(failure), + VectorSettingsPushRuleNotificationViewEvent.PushRuleUpdated(firstRuleId, true, failure), + VectorSettingsPushRuleNotificationViewEvent.Failure(secondRuleId, failure), ) .finish() } @@ -205,14 +210,14 @@ internal class VectorSettingsPushRuleNotificationViewModelTest { .assertStatesChanges( initialState, { copy(isLoading = true) }, - { copy(isLoading = false) }, + { copy(isLoading = false, rulesOnError = setOf(firstRuleId)) }, { copy(isLoading = true) }, { copy(isLoading = false) }, ) .assertEvents( // The global rule remains checked if all the rules are not unchecked - VectorSettingsPushRuleNotificationViewEvent.PushRuleUpdated(RuleIds.RULE_ID_ONE_TO_ONE_ROOM, true, failure), - VectorSettingsPushRuleNotificationViewEvent.Failure(failure), + VectorSettingsPushRuleNotificationViewEvent.PushRuleUpdated(firstRuleId, true, failure), + VectorSettingsPushRuleNotificationViewEvent.Failure(secondRuleId, failure), ) .finish() } @@ -245,11 +250,9 @@ internal class VectorSettingsPushRuleNotificationViewModelTest { } private fun givenARuleId(ruleId: String, notificationIndex: NotificationIndex = NotificationIndex.NOISY): PushRuleAndKind { - val ruleAndKind = mockk { - every { pushRule.ruleId } returns ruleId - every { pushRule.notificationIndex } returns notificationIndex - every { kind } returns mockk() - } + val pushRule = PushRuleFixture.aPushRule(ruleId) + every { pushRule.notificationIndex } returns notificationIndex + val ruleAndKind = PushRuleFixture.aPushRuleAndKind(pushRule) every { fakePushRuleService.getPushRules().findDefaultRule(ruleId) } returns ruleAndKind diff --git a/vector/src/test/java/im/vector/app/features/settings/notifications/usecase/GetPushRulesOnInvalidStateUseCaseTest.kt b/vector/src/test/java/im/vector/app/features/settings/notifications/usecase/GetPushRulesOnInvalidStateUseCaseTest.kt new file mode 100644 index 0000000000..a434ac93d3 --- /dev/null +++ b/vector/src/test/java/im/vector/app/features/settings/notifications/usecase/GetPushRulesOnInvalidStateUseCaseTest.kt @@ -0,0 +1,100 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package im.vector.app.features.settings.notifications.usecase + +import im.vector.app.test.fakes.FakeSession +import im.vector.app.test.fixtures.PushRuleFixture +import io.mockk.every +import io.mockk.mockkStatic +import io.mockk.unmockkAll +import org.amshove.kluent.shouldBeEqualTo +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.matrix.android.sdk.api.session.pushrules.Action +import org.matrix.android.sdk.api.session.pushrules.RuleIds +import org.matrix.android.sdk.api.session.pushrules.rest.PushRule + +internal class GetPushRulesOnInvalidStateUseCaseTest { + + private val fakeSession = FakeSession() + private val getPushRulesOnInvalidStateUseCase = GetPushRulesOnInvalidStateUseCase() + + @Before + fun setup() { + mockkStatic("org.matrix.android.sdk.api.session.pushrules.ActionKt") + } + + @After + fun tearDown() { + unmockkAll() + } + + @Test + fun `given a list of push rules with children not matching their parent when execute then returns the list of not matching rules`() { + // Given + val firstActions = listOf(Action.Notify) + val secondActions = listOf(Action.DoNotNotify) + givenARuleList( + listOf( + // first set of related rules + givenARuleId(RuleIds.RULE_ID_ONE_TO_ONE_ROOM, true, firstActions), + givenARuleId(RuleIds.RULE_ID_POLL_START_ONE_TO_ONE, true, listOf(Action.DoNotNotify)), // diff + givenARuleId(RuleIds.RULE_ID_POLL_START_ONE_TO_ONE_UNSTABLE, true, emptyList()), // diff + givenARuleId(RuleIds.RULE_ID_POLL_END_ONE_TO_ONE, false, listOf(Action.Notify)), // diff + givenARuleId(RuleIds.RULE_ID_POLL_END_ONE_TO_ONE_UNSTABLE, true, listOf(Action.Notify)), + // second set of related rules + givenARuleId(RuleIds.RULE_ID_ALL_OTHER_MESSAGES_ROOMS, false, secondActions), + givenARuleId(RuleIds.RULE_ID_POLL_START, true, listOf(Action.Notify)), // diff + givenARuleId(RuleIds.RULE_ID_POLL_START_UNSTABLE, false, listOf(Action.DoNotNotify)), + givenARuleId(RuleIds.RULE_ID_POLL_END, false, listOf(Action.Notify)), // diff + givenARuleId(RuleIds.RULE_ID_POLL_END_UNSTABLE, true, listOf()), // diff + // Another rule + givenARuleId(RuleIds.RULE_ID_CONTAIN_USER_NAME, true, listOf(Action.Notify)), + ) + ) + + // When + val result = getPushRulesOnInvalidStateUseCase.execute(fakeSession).map { it.ruleId } + + // Then + result shouldBeEqualTo listOf( + RuleIds.RULE_ID_ONE_TO_ONE_ROOM, // parent rule + RuleIds.RULE_ID_POLL_START_ONE_TO_ONE, + RuleIds.RULE_ID_POLL_START_ONE_TO_ONE_UNSTABLE, + RuleIds.RULE_ID_POLL_END_ONE_TO_ONE, + RuleIds.RULE_ID_ALL_OTHER_MESSAGES_ROOMS, // parent rule + RuleIds.RULE_ID_POLL_START, + RuleIds.RULE_ID_POLL_END, + RuleIds.RULE_ID_POLL_END_UNSTABLE, + ) + } + + private fun givenARuleList(rules: List) { + every { fakeSession.fakePushRuleService.getPushRules().getAllRules() } returns rules + } + + private fun givenARuleId(ruleId: String, enabled: Boolean, actions: List): PushRule { + val ruleAndKind = PushRuleFixture.aPushRuleAndKind( + PushRuleFixture.aPushRule(ruleId, enabled, actions), + ) + + every { fakeSession.fakePushRuleService.getPushRules().findDefaultRule(ruleId) } returns ruleAndKind + + return ruleAndKind.pushRule + } +} diff --git a/vector/src/test/java/im/vector/app/features/settings/notifications/usecase/UpdatePushRulesIfNeededUseCaseTest.kt b/vector/src/test/java/im/vector/app/features/settings/notifications/usecase/UpdatePushRulesIfNeededUseCaseTest.kt index 7dcbe68d4b..1f76a7f9a5 100644 --- a/vector/src/test/java/im/vector/app/features/settings/notifications/usecase/UpdatePushRulesIfNeededUseCaseTest.kt +++ b/vector/src/test/java/im/vector/app/features/settings/notifications/usecase/UpdatePushRulesIfNeededUseCaseTest.kt @@ -17,6 +17,7 @@ package im.vector.app.features.settings.notifications.usecase import im.vector.app.test.fakes.FakeSession +import im.vector.app.test.fixtures.PushRuleFixture import io.mockk.coVerifySequence import io.mockk.every import io.mockk.mockk @@ -28,14 +29,13 @@ import org.junit.Before import org.junit.Test import org.matrix.android.sdk.api.session.pushrules.Action import org.matrix.android.sdk.api.session.pushrules.RuleIds -import org.matrix.android.sdk.api.session.pushrules.getActions import org.matrix.android.sdk.api.session.pushrules.rest.PushRule -import org.matrix.android.sdk.api.session.pushrules.rest.PushRuleAndKind internal class UpdatePushRulesIfNeededUseCaseTest { private val fakeSession = FakeSession() - private val updatePushRulesIfNeededUseCase = UpdatePushRulesIfNeededUseCase() + private val fakeGetPushRulesOnInvalidStateUseCase = mockk() + private val updatePushRulesIfNeededUseCase = UpdatePushRulesIfNeededUseCase(fakeGetPushRulesOnInvalidStateUseCase) @Before fun setup() { @@ -50,25 +50,26 @@ internal class UpdatePushRulesIfNeededUseCaseTest { @Test fun test() = runTest { // Given - val firstActions = listOf(Action.Notify) - val secondActions = listOf(Action.DoNotNotify) - val rules = listOf( + val firstParentEnabled = true + val firstParentActions = listOf(Action.Notify) + val firstParent = givenARuleId(RuleIds.RULE_ID_ONE_TO_ONE_ROOM, firstParentEnabled, firstParentActions) + val secondParentEnabled = false + val secondParentActions = listOf(Action.DoNotNotify) + val secondParent = givenARuleId(RuleIds.RULE_ID_ALL_OTHER_MESSAGES_ROOMS, secondParentEnabled, secondParentActions) + val rulesOnError = listOf( // first set of related rules - givenARuleId(RuleIds.RULE_ID_ONE_TO_ONE_ROOM, true, firstActions), + firstParent, givenARuleId(RuleIds.RULE_ID_POLL_START_ONE_TO_ONE, true, listOf(Action.DoNotNotify)), // diff givenARuleId(RuleIds.RULE_ID_POLL_START_ONE_TO_ONE_UNSTABLE, true, emptyList()), // diff givenARuleId(RuleIds.RULE_ID_POLL_END_ONE_TO_ONE, false, listOf(Action.Notify)), // diff - givenARuleId(RuleIds.RULE_ID_POLL_END_ONE_TO_ONE_UNSTABLE, true, listOf(Action.Notify)), // second set of related rules - givenARuleId(RuleIds.RULE_ID_ALL_OTHER_MESSAGES_ROOMS, false, secondActions), + secondParent, givenARuleId(RuleIds.RULE_ID_POLL_START, true, listOf(Action.Notify)), // diff - givenARuleId(RuleIds.RULE_ID_POLL_START_UNSTABLE, false, listOf(Action.DoNotNotify)), givenARuleId(RuleIds.RULE_ID_POLL_END, false, listOf(Action.Notify)), // diff givenARuleId(RuleIds.RULE_ID_POLL_END_UNSTABLE, true, listOf()), // diff - // Another rule - givenARuleId(RuleIds.RULE_ID_CONTAIN_USER_NAME, true, listOf(Action.Notify)), ) - every { fakeSession.fakePushRuleService.getPushRules().getAllRules() } returns rules + every { fakeGetPushRulesOnInvalidStateUseCase.execute(fakeSession) } returns rulesOnError + every { fakeSession.fakePushRuleService.getPushRules().getAllRules() } returns rulesOnError // When updatePushRulesIfNeededUseCase.execute(fakeSession) @@ -77,30 +78,23 @@ internal class UpdatePushRulesIfNeededUseCaseTest { coVerifySequence { fakeSession.fakePushRuleService.getPushRules() // first set - fakeSession.fakePushRuleService.updatePushRuleActions(any(), RuleIds.RULE_ID_POLL_START_ONE_TO_ONE, true, firstActions) - fakeSession.fakePushRuleService.updatePushRuleActions(any(), RuleIds.RULE_ID_POLL_START_ONE_TO_ONE_UNSTABLE, true, firstActions) - fakeSession.fakePushRuleService.updatePushRuleActions(any(), RuleIds.RULE_ID_POLL_END_ONE_TO_ONE, true, firstActions) + fakeSession.fakePushRuleService.updatePushRuleActions(any(), RuleIds.RULE_ID_POLL_START_ONE_TO_ONE, firstParentEnabled, firstParentActions) + fakeSession.fakePushRuleService.updatePushRuleActions(any(), RuleIds.RULE_ID_POLL_START_ONE_TO_ONE_UNSTABLE, firstParentEnabled, firstParentActions) + fakeSession.fakePushRuleService.updatePushRuleActions(any(), RuleIds.RULE_ID_POLL_END_ONE_TO_ONE, firstParentEnabled, firstParentActions) // second set - fakeSession.fakePushRuleService.updatePushRuleActions(any(), RuleIds.RULE_ID_POLL_START, false, secondActions) - fakeSession.fakePushRuleService.updatePushRuleActions(any(), RuleIds.RULE_ID_POLL_END, false, secondActions) - fakeSession.fakePushRuleService.updatePushRuleActions(any(), RuleIds.RULE_ID_POLL_END_UNSTABLE, false, secondActions) + fakeSession.fakePushRuleService.updatePushRuleActions(any(), RuleIds.RULE_ID_POLL_START, secondParentEnabled, secondParentActions) + fakeSession.fakePushRuleService.updatePushRuleActions(any(), RuleIds.RULE_ID_POLL_END, secondParentEnabled, secondParentActions) + fakeSession.fakePushRuleService.updatePushRuleActions(any(), RuleIds.RULE_ID_POLL_END_UNSTABLE, secondParentEnabled, secondParentActions) } } private fun givenARuleId(ruleId: String, enabled: Boolean, actions: List): PushRule { - val pushRule = mockk { - every { this@mockk.ruleId } returns ruleId - every { this@mockk.enabled } returns enabled - every { this@mockk.actions } returns actions - every { getActions() } returns actions - } - val ruleAndKind = mockk { - every { this@mockk.pushRule } returns pushRule - every { kind } returns mockk() - } + val ruleAndKind = PushRuleFixture.aPushRuleAndKind( + pushRule = PushRuleFixture.aPushRule(ruleId, enabled, actions), + ) every { fakeSession.fakePushRuleService.getPushRules().findDefaultRule(ruleId) } returns ruleAndKind - return pushRule + return ruleAndKind.pushRule } } diff --git a/vector/src/test/java/im/vector/app/test/fixtures/PushRuleFixture.kt b/vector/src/test/java/im/vector/app/test/fixtures/PushRuleFixture.kt new file mode 100644 index 0000000000..2170d60228 --- /dev/null +++ b/vector/src/test/java/im/vector/app/test/fixtures/PushRuleFixture.kt @@ -0,0 +1,49 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package im.vector.app.test.fixtures + +import io.mockk.every +import io.mockk.mockk +import org.matrix.android.sdk.api.session.pushrules.Action +import org.matrix.android.sdk.api.session.pushrules.RuleSetKey +import org.matrix.android.sdk.api.session.pushrules.getActions +import org.matrix.android.sdk.api.session.pushrules.rest.PushRule +import org.matrix.android.sdk.api.session.pushrules.rest.PushRuleAndKind + +object PushRuleFixture { + + private const val A_RULE_ID = "a-rule-id" + + // Needs: mockkStatic("org.matrix.android.sdk.api.session.pushrules.ActionKt") + fun aPushRule( + ruleId: String = A_RULE_ID, + enabled: Boolean = true, + actions: List = listOf(Action.Notify), + ): PushRule = mockk { + every { this@mockk.ruleId } returns ruleId + every { this@mockk.enabled } returns enabled + every { getActions() } returns actions + } + + fun aPushRuleAndKind( + pushRule: PushRule = aPushRule(), + kind: RuleSetKey = RuleSetKey.UNDERRIDE, + ): PushRuleAndKind = mockk { + every { this@mockk.pushRule } returns pushRule + every { this@mockk.kind } returns kind + } +} From 552c6fbbd35ca9a4042c892e5645fecb744e57b4 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Fri, 17 Feb 2023 10:34:18 +0100 Subject: [PATCH 2/8] Changelog --- changelog.d/8141.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8141.feature diff --git a/changelog.d/8141.feature b/changelog.d/8141.feature new file mode 100644 index 0000000000..8c03af1150 --- /dev/null +++ b/changelog.d/8141.feature @@ -0,0 +1 @@ +[Poll] Error handling for push rules synchronization From 84ccd30ab753f767bc3b230ed226b6043141d585 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Fri, 17 Feb 2023 10:57:27 +0100 Subject: [PATCH 3/8] Fix wrong import --- .../usecase/GetPushRulesOnInvalidStateUseCase.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vector/src/main/java/im/vector/app/features/settings/notifications/usecase/GetPushRulesOnInvalidStateUseCase.kt b/vector/src/main/java/im/vector/app/features/settings/notifications/usecase/GetPushRulesOnInvalidStateUseCase.kt index 225be1ee62..9b131f9a40 100644 --- a/vector/src/main/java/im/vector/app/features/settings/notifications/usecase/GetPushRulesOnInvalidStateUseCase.kt +++ b/vector/src/main/java/im/vector/app/features/settings/notifications/usecase/GetPushRulesOnInvalidStateUseCase.kt @@ -16,12 +16,12 @@ package im.vector.app.features.settings.notifications.usecase +import im.vector.app.features.settings.notifications.getParentRule +import im.vector.app.features.settings.notifications.getSyncedRules import org.matrix.android.sdk.api.extensions.orTrue import org.matrix.android.sdk.api.session.Session import org.matrix.android.sdk.api.session.pushrules.RuleIds import org.matrix.android.sdk.api.session.pushrules.getActions -import org.matrix.android.sdk.api.session.pushrules.getParentRule -import org.matrix.android.sdk.api.session.pushrules.getSyncedRules import org.matrix.android.sdk.api.session.pushrules.rest.PushRule import javax.inject.Inject From 8f56f9de469884fb29fb866dc8c1d0a542017d86 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Fri, 17 Feb 2023 12:22:02 +0100 Subject: [PATCH 4/8] update doc --- .../notifications/VectorSettingsPushRuleNotificationViewEvent.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewEvent.kt b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewEvent.kt index 490a28c0b0..42cb923a5e 100644 --- a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewEvent.kt +++ b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewEvent.kt @@ -31,6 +31,7 @@ sealed interface VectorSettingsPushRuleNotificationViewEvent : VectorViewEvents /** * A failure has occurred. * + * @property ruleId the global rule id related to the failure. * @property throwable the related exception, if any. */ data class Failure(val ruleId: String, val throwable: Throwable?) : VectorSettingsPushRuleNotificationViewEvent From 13866c62bf48386ef40ebf3acb7e7fb399540223 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Fri, 17 Feb 2023 17:38:47 +0100 Subject: [PATCH 5/8] Refresh push rules settings on account data changes --- ...torSettingsPushRuleNotificationFragment.kt | 39 ++++++++++--------- ...rSettingsPushRuleNotificationViewAction.kt | 3 +- ...orSettingsPushRuleNotificationViewModel.kt | 30 +++++++------- ...orSettingsPushRuleNotificationViewState.kt | 4 +- 4 files changed, 40 insertions(+), 36 deletions(-) diff --git a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationFragment.kt b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationFragment.kt index 8a2099ecad..b7264f6cb3 100644 --- a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationFragment.kt +++ b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationFragment.kt @@ -36,6 +36,7 @@ abstract class VectorSettingsPushRuleNotificationFragment : override fun onViewCreated(view: View, savedInstanceState: Bundle?) { super.onViewCreated(view, savedInstanceState) observeViewEvents() + viewModel.onEach(VectorSettingsPushRuleNotificationViewState::allRules) { refreshPreferences() } viewModel.onEach(VectorSettingsPushRuleNotificationViewState::isLoading) { updateLoadingView(it) } viewModel.onEach(VectorSettingsPushRuleNotificationViewState::rulesOnError) { refreshErrors(it) } } @@ -55,18 +56,11 @@ abstract class VectorSettingsPushRuleNotificationFragment : } override fun bindPref() { - for (preferenceKey in prefKeyToPushRuleId.keys) { - val preference = findPreference(preferenceKey)!! - preference.isIconSpaceReserved = false - val ruleAndKind = prefKeyToPushRuleId[preferenceKey]?.let { viewModel.getPushRuleAndKind(it) } - if (ruleAndKind == null) { - // The rule is not defined, hide the preference - preference.isVisible = false - } else { - preference.isVisible = true - updatePreference(ruleAndKind.pushRule.ruleId, viewModel.isPushRuleChecked(ruleAndKind.pushRule.ruleId)) - preference.onPreferenceChangeListener = Preference.OnPreferenceChangeListener { _, newValue -> - viewModel.handle(VectorSettingsPushRuleNotificationViewAction.UpdatePushRule(ruleAndKind, newValue as Boolean)) + prefKeyToPushRuleId.forEach { (preferenceKey, ruleId) -> + findPreference(preferenceKey)?.apply { + isIconSpaceReserved = false + onPreferenceChangeListener = Preference.OnPreferenceChangeListener { _, newValue -> + viewModel.handle(VectorSettingsPushRuleNotificationViewAction.UpdatePushRule(ruleId, newValue as Boolean)) false } } @@ -81,16 +75,21 @@ abstract class VectorSettingsPushRuleNotificationFragment : } } + private fun refreshPreferences() { + prefKeyToPushRuleId.values.forEach { ruleId -> updatePreference(ruleId, viewModel.isPushRuleChecked(ruleId)) } + } + private fun refreshErrors(rulesWithError: Set) { if (withState(viewModel, VectorSettingsPushRuleNotificationViewState::isLoading)) return prefKeyToPushRuleId.forEach { (preferenceKey, ruleId) -> - val preference = findPreference(preferenceKey)!! - if (ruleId in rulesWithError) { - preference.summaryTextColor = context?.let { ThemeUtils.getColor(it, R.attr.colorError) } - preference.setSummary(R.string.settings_notification_error_on_update) - } else { - preference.summaryTextColor = null - preference.summary = null + findPreference(preferenceKey)?.apply { + if (ruleId in rulesWithError) { + summaryTextColor = ThemeUtils.getColor(context, R.attr.colorError) + setSummary(R.string.settings_notification_error_on_update) + } else { + summaryTextColor = null + summary = null + } } } } @@ -102,6 +101,8 @@ abstract class VectorSettingsPushRuleNotificationFragment : private fun updatePreference(ruleId: String, checked: Boolean) { val preferenceKey = prefKeyToPushRuleId.entries.find { it.value == ruleId }?.key ?: return val preference = findPreference(preferenceKey) ?: return + val ruleIds = withState(viewModel) { state -> state.allRules.map { it.ruleId } } + preference.isVisible = ruleId in ruleIds preference.isChecked = checked } diff --git a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewAction.kt b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewAction.kt index 61bf7c5b15..94b7666122 100644 --- a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewAction.kt +++ b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewAction.kt @@ -17,8 +17,7 @@ package im.vector.app.features.settings.notifications import im.vector.app.core.platform.VectorViewModelAction -import org.matrix.android.sdk.api.session.pushrules.rest.PushRuleAndKind sealed interface VectorSettingsPushRuleNotificationViewAction : VectorViewModelAction { - data class UpdatePushRule(val pushRuleAndKind: PushRuleAndKind, val checked: Boolean) : VectorSettingsPushRuleNotificationViewAction + data class UpdatePushRule(val ruleId: String, val checked: Boolean) : VectorSettingsPushRuleNotificationViewAction } diff --git a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModel.kt b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModel.kt index eee199d550..31f7f9c531 100644 --- a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModel.kt +++ b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModel.kt @@ -20,7 +20,6 @@ import com.airbnb.mvrx.MavericksViewModelFactory import dagger.assisted.Assisted import dagger.assisted.AssistedFactory import dagger.assisted.AssistedInject -import im.vector.app.core.di.ActiveSessionHolder import im.vector.app.core.di.MavericksAssistedViewModelFactory import im.vector.app.core.di.hiltMavericksViewModelFactory import im.vector.app.core.platform.VectorViewModel @@ -30,6 +29,7 @@ import im.vector.app.features.settings.notifications.usecase.GetPushRulesOnInval import kotlinx.coroutines.launch import org.matrix.android.sdk.api.failure.Failure.ServerError import org.matrix.android.sdk.api.failure.MatrixError +import org.matrix.android.sdk.api.session.Session import org.matrix.android.sdk.api.session.accountdata.UserAccountDataTypes import org.matrix.android.sdk.api.session.pushrules.Action import org.matrix.android.sdk.api.session.pushrules.RuleIds @@ -43,7 +43,7 @@ private typealias ViewState = VectorSettingsPushRuleNotificationViewState class VectorSettingsPushRuleNotificationViewModel @AssistedInject constructor( @Assisted initialState: ViewState, - private val activeSessionHolder: ActiveSessionHolder, + private val session: Session, private val getPushRulesOnInvalidStateUseCase: GetPushRulesOnInvalidStateUseCase, ) : VectorViewModel by hiltMavericksViewModelFactory() init { - val session = activeSessionHolder.getSafeActiveSession() - session?.flow() - ?.liveUserAccountData(UserAccountDataTypes.TYPE_PUSH_RULES) - ?.unwrap() - ?.setOnEach { + session.flow() + .liveUserAccountData(UserAccountDataTypes.TYPE_PUSH_RULES) + .unwrap() + .setOnEach { + val allRules = session.pushRuleService().getPushRules().getAllRules() val rulesOnError = getPushRulesOnInvalidStateUseCase.execute(session).map { it.ruleId }.toSet() - copy(rulesOnError = rulesOnError) + copy( + allRules = allRules, + rulesOnError = rulesOnError + ) } } override fun handle(action: VectorSettingsPushRuleNotificationViewAction) { when (action) { - is VectorSettingsPushRuleNotificationViewAction.UpdatePushRule -> handleUpdatePushRule(action.pushRuleAndKind, action.checked) + is VectorSettingsPushRuleNotificationViewAction.UpdatePushRule -> handleUpdatePushRule(action.ruleId, action.checked) } } fun getPushRuleAndKind(ruleId: String): PushRuleAndKind? { - return activeSessionHolder.getSafeActiveSession()?.pushRuleService()?.getPushRules()?.findDefaultRule(ruleId) + return session.pushRuleService().getPushRules().findDefaultRule(ruleId) } fun isPushRuleChecked(ruleId: String): Boolean { @@ -82,9 +85,8 @@ class VectorSettingsPushRuleNotificationViewModel @AssistedInject constructor( return rulesGroup.mapNotNull { getPushRuleAndKind(it) }.any { it.pushRule.notificationIndex != NotificationIndex.OFF } } - private fun handleUpdatePushRule(pushRuleAndKind: PushRuleAndKind, checked: Boolean) { - val ruleId = pushRuleAndKind.pushRule.ruleId - val kind = pushRuleAndKind.kind + private fun handleUpdatePushRule(ruleId: String, checked: Boolean) { + val kind = getPushRuleAndKind(ruleId)?.kind ?: return val newIndex = if (checked) NotificationIndex.NOISY else NotificationIndex.OFF val standardAction = getStandardAction(ruleId, newIndex) ?: return val enabled = standardAction != StandardActions.Disabled @@ -129,7 +131,7 @@ class VectorSettingsPushRuleNotificationViewModel @AssistedInject constructor( } private suspend fun updatePushRule(kind: RuleKind, ruleId: String, enable: Boolean, newActions: List?) { - activeSessionHolder.getSafeActiveSession()?.pushRuleService()?.updatePushRuleActions( + session.pushRuleService().updatePushRuleActions( kind = kind, ruleId = ruleId, enable = enable, diff --git a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewState.kt b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewState.kt index 392a8f3bb5..091c183d49 100644 --- a/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewState.kt +++ b/vector/src/main/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewState.kt @@ -17,8 +17,10 @@ package im.vector.app.features.settings.notifications import com.airbnb.mvrx.MavericksState +import org.matrix.android.sdk.api.session.pushrules.rest.PushRule data class VectorSettingsPushRuleNotificationViewState( val isLoading: Boolean = false, - val rulesOnError: Set = emptySet() + val allRules: List = emptyList(), + val rulesOnError: Set = emptySet(), ) : MavericksState From cd648a0b50d472c7705530a7724581b4e0fa63d1 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Mon, 20 Feb 2023 14:02:28 +0100 Subject: [PATCH 6/8] Fix unit tests --- ...ttingsPushRuleNotificationViewModelTest.kt | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/vector/src/test/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModelTest.kt b/vector/src/test/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModelTest.kt index 7994d173b6..8a45f922e7 100644 --- a/vector/src/test/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModelTest.kt +++ b/vector/src/test/java/im/vector/app/features/settings/notifications/VectorSettingsPushRuleNotificationViewModelTest.kt @@ -18,7 +18,7 @@ package im.vector.app.features.settings.notifications import com.airbnb.mvrx.test.MavericksTestRule import im.vector.app.features.settings.notifications.usecase.GetPushRulesOnInvalidStateUseCase -import im.vector.app.test.fakes.FakeActiveSessionHolder +import im.vector.app.test.fakes.FakeSession import im.vector.app.test.fixtures.PushRuleFixture import im.vector.app.test.test import im.vector.app.test.testDispatcher @@ -34,21 +34,20 @@ import org.junit.Before import org.junit.Rule import org.junit.Test import org.matrix.android.sdk.api.session.pushrules.RuleIds -import org.matrix.android.sdk.api.session.pushrules.rest.PushRuleAndKind internal class VectorSettingsPushRuleNotificationViewModelTest { @get:Rule val mavericksTestRule = MavericksTestRule(testDispatcher = testDispatcher) - private val fakeActiveSessionHolder = FakeActiveSessionHolder() - private val fakePushRuleService = fakeActiveSessionHolder.fakeSession.fakePushRuleService + private val fakeSession = FakeSession() + private val fakePushRuleService = fakeSession.fakePushRuleService private val fakeGetPushRulesOnInvalidStateUseCase = mockk() private val initialState = VectorSettingsPushRuleNotificationViewState() private fun createViewModel() = VectorSettingsPushRuleNotificationViewModel( initialState = initialState, - activeSessionHolder = fakeActiveSessionHolder.instance, + session = fakeSession, fakeGetPushRulesOnInvalidStateUseCase, ) @@ -70,12 +69,14 @@ internal class VectorSettingsPushRuleNotificationViewModelTest { val firstRuleId = RuleIds.RULE_ID_ONE_TO_ONE_ROOM val secondRuleId = RuleIds.RULE_ID_ALL_OTHER_MESSAGES_ROOMS + givenARuleId(firstRuleId) + givenARuleId(secondRuleId) fakePushRuleService.givenUpdatePushRuleActionsSucceed() // When val viewModelTest = viewModel.test() - viewModel.handle(VectorSettingsPushRuleNotificationViewAction.UpdatePushRule(givenARuleId(firstRuleId), true)) - viewModel.handle(VectorSettingsPushRuleNotificationViewAction.UpdatePushRule(givenARuleId(secondRuleId), false)) + viewModel.handle(VectorSettingsPushRuleNotificationViewAction.UpdatePushRule(firstRuleId, true)) + viewModel.handle(VectorSettingsPushRuleNotificationViewAction.UpdatePushRule(secondRuleId, false)) // Then coVerifyOrder { @@ -116,10 +117,12 @@ internal class VectorSettingsPushRuleNotificationViewModelTest { val failure = mockk() val firstRuleId = RuleIds.RULE_ID_ONE_TO_ONE_ROOM + givenARuleId(firstRuleId) fakePushRuleService.givenUpdatePushRuleActionsSucceed() fakePushRuleService.givenUpdatePushRuleActionsFail(RuleIds.RULE_ID_POLL_START_ONE_TO_ONE_UNSTABLE, failure) val secondRuleId = RuleIds.RULE_ID_ALL_OTHER_MESSAGES_ROOMS + givenARuleId(secondRuleId) fakePushRuleService.givenUpdatePushRuleActionsFail(secondRuleId, failure) fakePushRuleService.givenUpdatePushRuleActionsFail(RuleIds.RULE_ID_POLL_START, failure) fakePushRuleService.givenUpdatePushRuleActionsFail(RuleIds.RULE_ID_POLL_START_UNSTABLE, failure) @@ -129,9 +132,9 @@ internal class VectorSettingsPushRuleNotificationViewModelTest { // When val viewModelTest = viewModel.test() // One rule failed to update - viewModel.handle(VectorSettingsPushRuleNotificationViewAction.UpdatePushRule(givenARuleId(firstRuleId), true)) + viewModel.handle(VectorSettingsPushRuleNotificationViewAction.UpdatePushRule(firstRuleId, true)) // All the rules failed to update - viewModel.handle(VectorSettingsPushRuleNotificationViewAction.UpdatePushRule(givenARuleId(secondRuleId), true)) + viewModel.handle(VectorSettingsPushRuleNotificationViewAction.UpdatePushRule(secondRuleId, true)) // Then coVerifyOrder { @@ -154,7 +157,7 @@ internal class VectorSettingsPushRuleNotificationViewModelTest { .assertStatesChanges( initialState, { copy(isLoading = true) }, - { copy(isLoading = false, setOf(firstRuleId)) }, + { copy(isLoading = false, rulesOnError = setOf(firstRuleId)) }, { copy(isLoading = true) }, { copy(isLoading = false) }, ) @@ -172,10 +175,12 @@ internal class VectorSettingsPushRuleNotificationViewModelTest { val failure = mockk() val firstRuleId = RuleIds.RULE_ID_ONE_TO_ONE_ROOM + givenARuleId(firstRuleId) fakePushRuleService.givenUpdatePushRuleActionsSucceed() fakePushRuleService.givenUpdatePushRuleActionsFail(RuleIds.RULE_ID_POLL_START_ONE_TO_ONE_UNSTABLE, failure) val secondRuleId = RuleIds.RULE_ID_ALL_OTHER_MESSAGES_ROOMS + givenARuleId(secondRuleId) fakePushRuleService.givenUpdatePushRuleActionsFail(secondRuleId, failure) fakePushRuleService.givenUpdatePushRuleActionsFail(RuleIds.RULE_ID_POLL_START, failure) fakePushRuleService.givenUpdatePushRuleActionsFail(RuleIds.RULE_ID_POLL_START_UNSTABLE, failure) @@ -185,9 +190,9 @@ internal class VectorSettingsPushRuleNotificationViewModelTest { // When val viewModelTest = viewModel.test() // One rule failed to update - viewModel.handle(VectorSettingsPushRuleNotificationViewAction.UpdatePushRule(givenARuleId(firstRuleId), false)) + viewModel.handle(VectorSettingsPushRuleNotificationViewAction.UpdatePushRule(firstRuleId, false)) // All the rules failed to update - viewModel.handle(VectorSettingsPushRuleNotificationViewAction.UpdatePushRule(givenARuleId(secondRuleId), false)) + viewModel.handle(VectorSettingsPushRuleNotificationViewAction.UpdatePushRule(secondRuleId, false)) // Then coVerifyOrder { @@ -249,13 +254,11 @@ internal class VectorSettingsPushRuleNotificationViewModelTest { secondResult shouldBe false } - private fun givenARuleId(ruleId: String, notificationIndex: NotificationIndex = NotificationIndex.NOISY): PushRuleAndKind { + private fun givenARuleId(ruleId: String, notificationIndex: NotificationIndex = NotificationIndex.NOISY) { val pushRule = PushRuleFixture.aPushRule(ruleId) every { pushRule.notificationIndex } returns notificationIndex val ruleAndKind = PushRuleFixture.aPushRuleAndKind(pushRule) every { fakePushRuleService.getPushRules().findDefaultRule(ruleId) } returns ruleAndKind - - return ruleAndKind } } From 021babc9b10345192ff1b7a653c1a137e3d90496 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Thu, 23 Feb 2023 09:23:30 +0100 Subject: [PATCH 7/8] Fix notification in encrypted room for poll end event --- .../org/matrix/android/sdk/api/session/events/model/Event.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/events/model/Event.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/events/model/Event.kt index 40c69ceb66..ae3e3a63c8 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/events/model/Event.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/events/model/Event.kt @@ -488,7 +488,7 @@ fun Event.getPollContent(): MessagePollContent? { } fun Event.supportsNotification() = - this.getClearType() in EventType.MESSAGE + EventType.POLL_START.values + EventType.STATE_ROOM_BEACON_INFO.values + this.getClearType() in EventType.MESSAGE + EventType.POLL_START.values + EventType.POLL_END.values + EventType.STATE_ROOM_BEACON_INFO.values fun Event.isContentReportable() = this.getClearType() in EventType.MESSAGE + EventType.STATE_ROOM_BEACON_INFO.values From 6649297ea0e9ce46f83339eebd205bcda0d5c190 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Thu, 23 Feb 2023 14:01:49 +0100 Subject: [PATCH 8/8] restore onSessionStarted method call --- .../vector/app/core/session/ConfigureAndStartSessionUseCase.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vector/src/main/java/im/vector/app/core/session/ConfigureAndStartSessionUseCase.kt b/vector/src/main/java/im/vector/app/core/session/ConfigureAndStartSessionUseCase.kt index 8a9fc9729d..b9573e9292 100644 --- a/vector/src/main/java/im/vector/app/core/session/ConfigureAndStartSessionUseCase.kt +++ b/vector/src/main/java/im/vector/app/core/session/ConfigureAndStartSessionUseCase.kt @@ -52,7 +52,7 @@ class ConfigureAndStartSessionUseCase @Inject constructor( updateMatrixClientInfoIfNeeded(session) createNotificationSettingsAccountDataIfNeeded(session) notificationsSettingUpdater.onSessionStarted(session) -// pushRulesUpdater.onSessionStarted(session) + pushRulesUpdater.onSessionStarted(session) } private fun updateMatrixClientInfoIfNeeded(session: Session) {