From f29867ff17f2b149efb7c8ad192e462c299b23fd Mon Sep 17 00:00:00 2001 From: Alfonso Grillo Date: Tue, 31 Jan 2023 12:24:55 +0100 Subject: [PATCH 1/4] Add error handling for push sync --- Riot/Assets/en.lproj/Vector.strings | 1 + Riot/Generated/Strings.swift | 4 +++ .../View/NotificationSettings.swift | 17 ++++++++--- .../NotificationSettingsViewModel.swift | 30 +++++++++++++++++-- .../NotificationSettingsViewState.swift | 1 + 5 files changed, 47 insertions(+), 6 deletions(-) diff --git a/Riot/Assets/en.lproj/Vector.strings b/Riot/Assets/en.lproj/Vector.strings index 1e584740b3..73d395fafc 100644 --- a/Riot/Assets/en.lproj/Vector.strings +++ b/Riot/Assets/en.lproj/Vector.strings @@ -759,6 +759,7 @@ Tap the + to start adding people."; "settings_your_keywords" = "Your Keywords"; "settings_new_keyword" = "Add new Keyword"; "settings_mentions_and_keywords_encryption_notice" = "You won’t get notifications for mentions & keywords in encrypted rooms on mobile."; +"settings_push_rules_error" = "An error occurred when updating your notification preferences. Please try to toggle your option again."; "settings_enable_callkit" = "Integrated calling"; "settings_callkit_info" = "Receive incoming calls on your lock screen. See your %@ calls in the system's call history. If iCloud is enabled, this call history will be shared with Apple."; diff --git a/Riot/Generated/Strings.swift b/Riot/Generated/Strings.swift index d722d24f90..fd8eee3b64 100644 --- a/Riot/Generated/Strings.swift +++ b/Riot/Generated/Strings.swift @@ -7767,6 +7767,10 @@ public class VectorL10n: NSObject { public static var settingsProfilePicture: String { return VectorL10n.tr("Vector", "settings_profile_picture") } + /// An error occurred when updating your notification preferences. Please try to toggle your option again. + public static var settingsPushRulesError: String { + return VectorL10n.tr("Vector", "settings_push_rules_error") + } /// Are you sure you want to remove the email address %@? public static func settingsRemoveEmailPromptMsg(_ p1: String) -> String { return VectorL10n.tr("Vector", "settings_remove_email_prompt_msg", p1) diff --git a/RiotSwiftUI/Modules/Settings/Notifications/View/NotificationSettings.swift b/RiotSwiftUI/Modules/Settings/Notifications/View/NotificationSettings.swift index 4d1d3ee04a..4a8c7df2f2 100644 --- a/RiotSwiftUI/Modules/Settings/Notifications/View/NotificationSettings.swift +++ b/RiotSwiftUI/Modules/Settings/Notifications/View/NotificationSettings.swift @@ -21,6 +21,7 @@ import SwiftUI /// Also renders an optional bottom section. /// Used in the case of keywords, for the keyword chips and input. struct NotificationSettings: View { + @Environment(\.theme) var theme: ThemeSwiftUI @ObservedObject var viewModel: NotificationSettingsViewModel var bottomSection: BottomSection? @@ -31,10 +32,18 @@ struct NotificationSettings: View { header: FormSectionHeader(text: VectorL10n.settingsNotifyMeFor) ) { ForEach(viewModel.viewState.ruleIds) { ruleId in - let checked = viewModel.viewState.selectionState[ruleId] ?? false - FormPickerItem(title: ruleId.title, selected: checked) { - Task { - await viewModel.update(ruleID: ruleId, isChecked: !checked) + VStack(alignment: .leading, spacing: 4) { + let checked = viewModel.viewState.selectionState[ruleId] ?? false + FormPickerItem(title: ruleId.title, selected: checked) { + viewModel.update(ruleID: ruleId, isChecked: !checked) + } + + if viewModel.isRuleOutOfSync(ruleId) { + Text(VectorL10n.settingsPushRulesError) + .font(theme.fonts.caption1) + .foregroundColor(theme.colors.alert) + .padding(.horizontal) + .padding(.bottom, 16) } } } diff --git a/RiotSwiftUI/Modules/Settings/Notifications/ViewModel/NotificationSettingsViewModel.swift b/RiotSwiftUI/Modules/Settings/Notifications/ViewModel/NotificationSettingsViewModel.swift index 7a00940af5..f782901b1d 100644 --- a/RiotSwiftUI/Modules/Settings/Notifications/ViewModel/NotificationSettingsViewModel.swift +++ b/RiotSwiftUI/Modules/Settings/Notifications/ViewModel/NotificationSettingsViewModel.swift @@ -147,6 +147,10 @@ final class NotificationSettingsViewModel: NotificationSettingsViewModelType, Ob keywordsOrdered = keywordsOrdered.filter { $0 != keyword } notificationSettingsService.remove(keyword: keyword) } + + func isRuleOutOfSync(_ ruleId: NotificationPushRuleId) -> Bool { + viewState.outOfSyncRules.contains(ruleId) && viewState.saving == false + } } // MARK: - Private @@ -206,11 +210,12 @@ private extension NotificationSettingsViewModel { @MainActor func completeUpdate() { - #warning("Handle error here in the next ticket") viewState.saving = false } func rulesUpdated(newRules: [NotificationPushRuleType]) { + var outOfSyncRules: Set = .init() + for rule in newRules { guard let ruleId = NotificationPushRuleId(rawValue: rule.ruleId), @@ -219,8 +224,15 @@ private extension NotificationSettingsViewModel { continue } - viewState.selectionState[ruleId] = isChecked(rule: rule, syncedRules: ruleId.syncedRules(in: newRules)) + let relatedSyncedRules = ruleId.syncedRules(in: newRules) + viewState.selectionState[ruleId] = isChecked(rule: rule, syncedRules: relatedSyncedRules) + + if isOutOfSync(rule: rule, syncedRules: relatedSyncedRules) { + outOfSyncRules.insert(ruleId) + } } + + viewState.outOfSyncRules = outOfSyncRules } func keywordRuleUpdated(anyEnabled: Bool) { @@ -266,6 +278,20 @@ private extension NotificationSettingsViewModel { return defaultIsChecked(rule: rule) } } + + func isOutOfSync(rule: NotificationPushRuleType, syncedRules: [NotificationPushRuleType]) -> Bool { + guard let ruleId = NotificationPushRuleId(rawValue: rule.ruleId) else { + return false + } + + switch ruleId { + case .oneToOneRoom, .allOtherMessages: + let ruleIsChecked = defaultIsChecked(rule: rule) + return syncedRules.contains(where: { defaultIsChecked(rule: $0) != ruleIsChecked }) + default: + return false + } + } } private extension NotificationPushRuleId { diff --git a/RiotSwiftUI/Modules/Settings/Notifications/ViewModel/NotificationSettingsViewState.swift b/RiotSwiftUI/Modules/Settings/Notifications/ViewModel/NotificationSettingsViewState.swift index 22bb4fed83..a732f56b1b 100644 --- a/RiotSwiftUI/Modules/Settings/Notifications/ViewModel/NotificationSettingsViewState.swift +++ b/RiotSwiftUI/Modules/Settings/Notifications/ViewModel/NotificationSettingsViewState.swift @@ -22,5 +22,6 @@ struct NotificationSettingsViewState { var saving: Bool var ruleIds: [NotificationPushRuleId] var selectionState: [NotificationPushRuleId: Bool] + var outOfSyncRules: Set = .init() var keywords = [String]() } From e9bbc57a6e75a2d2073838d838ef00d89902f514 Mon Sep 17 00:00:00 2001 From: Alfonso Grillo Date: Tue, 31 Jan 2023 15:25:45 +0100 Subject: [PATCH 2/4] Improve UT --- .../Test/Unit/NotificationSettingsViewModelTests.swift | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/RiotSwiftUI/Modules/Settings/Notifications/Test/Unit/NotificationSettingsViewModelTests.swift b/RiotSwiftUI/Modules/Settings/Notifications/Test/Unit/NotificationSettingsViewModelTests.swift index a24e287330..1593b47ac0 100644 --- a/RiotSwiftUI/Modules/Settings/Notifications/Test/Unit/NotificationSettingsViewModelTests.swift +++ b/RiotSwiftUI/Modules/Settings/Notifications/Test/Unit/NotificationSettingsViewModelTests.swift @@ -97,6 +97,10 @@ final class NotificationSettingsViewModelTests: XCTestCase { // The one to one room rule ui flag should match the loudest related poll rule XCTAssertEqual(viewModel.viewState.selectionState[.oneToOneRoom], true) + + // the oneToOneRoom rule should be flagged as "out of sync" + XCTAssertTrue(self.viewModel.isRuleOutOfSync(.oneToOneRoom)) + XCTAssertFalse(self.viewModel.isRuleOutOfSync(.allOtherMessages)) } } From 832a8bab78eeae818ceb59d0f4dc6e216ff2b5e3 Mon Sep 17 00:00:00 2001 From: Alfonso Grillo Date: Tue, 31 Jan 2023 15:31:06 +0100 Subject: [PATCH 3/4] Add changelog.d file --- changelog.d/pr-7324.change | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/pr-7324.change diff --git a/changelog.d/pr-7324.change b/changelog.d/pr-7324.change new file mode 100644 index 0000000000..1d7133eb8c --- /dev/null +++ b/changelog.d/pr-7324.change @@ -0,0 +1 @@ +Polls: add error handling when syncing push rules with the ones of normal messages. From 241a1fcb7f599c0de6edd26dbae86fb20646feaf Mon Sep 17 00:00:00 2001 From: Alfonso Grillo Date: Wed, 1 Feb 2023 11:41:59 +0100 Subject: [PATCH 4/4] Fix rebase issues --- .../Test/Unit/NotificationSettingsViewModelTests.swift | 4 ++-- .../Settings/Notifications/View/NotificationSettings.swift | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/RiotSwiftUI/Modules/Settings/Notifications/Test/Unit/NotificationSettingsViewModelTests.swift b/RiotSwiftUI/Modules/Settings/Notifications/Test/Unit/NotificationSettingsViewModelTests.swift index 1593b47ac0..95b5e08fad 100644 --- a/RiotSwiftUI/Modules/Settings/Notifications/Test/Unit/NotificationSettingsViewModelTests.swift +++ b/RiotSwiftUI/Modules/Settings/Notifications/Test/Unit/NotificationSettingsViewModelTests.swift @@ -99,8 +99,8 @@ final class NotificationSettingsViewModelTests: XCTestCase { XCTAssertEqual(viewModel.viewState.selectionState[.oneToOneRoom], true) // the oneToOneRoom rule should be flagged as "out of sync" - XCTAssertTrue(self.viewModel.isRuleOutOfSync(.oneToOneRoom)) - XCTAssertFalse(self.viewModel.isRuleOutOfSync(.allOtherMessages)) + XCTAssertTrue(viewModel.isRuleOutOfSync(.oneToOneRoom)) + XCTAssertFalse(viewModel.isRuleOutOfSync(.allOtherMessages)) } } diff --git a/RiotSwiftUI/Modules/Settings/Notifications/View/NotificationSettings.swift b/RiotSwiftUI/Modules/Settings/Notifications/View/NotificationSettings.swift index 4a8c7df2f2..62cdf247a4 100644 --- a/RiotSwiftUI/Modules/Settings/Notifications/View/NotificationSettings.swift +++ b/RiotSwiftUI/Modules/Settings/Notifications/View/NotificationSettings.swift @@ -35,7 +35,9 @@ struct NotificationSettings: View { VStack(alignment: .leading, spacing: 4) { let checked = viewModel.viewState.selectionState[ruleId] ?? false FormPickerItem(title: ruleId.title, selected: checked) { - viewModel.update(ruleID: ruleId, isChecked: !checked) + Task { + await viewModel.update(ruleID: ruleId, isChecked: !checked) + } } if viewModel.isRuleOutOfSync(ruleId) {