From a90dcc6e86203a4abaad1631bbe2b8cc80959485 Mon Sep 17 00:00:00 2001 From: sashei Date: Tue, 4 Jun 2019 14:05:25 -0700 Subject: [PATCH 1/5] implement sync timeout --- Shared/Store/BaseDataStore.swift | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Shared/Store/BaseDataStore.swift b/Shared/Store/BaseDataStore.swift index 70b06003b..d47086b8c 100644 --- a/Shared/Store/BaseDataStore.swift +++ b/Shared/Store/BaseDataStore.swift @@ -236,6 +236,11 @@ extension BaseDataStore { } queue.async { + self.queue.asyncAfter(deadline: .now() + 20, execute: { + // this block serves to "cancel" the sync if the operation is running slowly + self.syncSubject.onNext(.Synced) + }) + do { try self.loginsStorage?.sync(unlockInfo: syncInfo) } catch let error as LoginStoreError { From f6aba6d3c2f7d4f524eb8cdc617ca52031fc10f2 Mon Sep 17 00:00:00 2001 From: sashei Date: Tue, 4 Jun 2019 14:24:06 -0700 Subject: [PATCH 2/5] add sentry + checking around timeout --- Shared/Store/BaseDataStore.swift | 13 ++++++++----- lockbox-ios/Action/SentryAction.swift | 2 +- lockbox-ios/Store/SentryStore.swift | 2 +- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/Shared/Store/BaseDataStore.swift b/Shared/Store/BaseDataStore.swift index d47086b8c..438511b99 100644 --- a/Shared/Store/BaseDataStore.swift +++ b/Shared/Store/BaseDataStore.swift @@ -71,7 +71,7 @@ class BaseDataStore { internal var disposeBag = DisposeBag() private var listSubject = BehaviorRelay<[LoginRecord]>(value: []) - private var syncSubject = ReplaySubject.create(bufferSize: 1) + private var syncSubject = BehaviorRelay(value: .Synced) private var storageStateSubject = ReplaySubject.create(bufferSize: 1) private let dispatcher: Dispatcher @@ -229,16 +229,19 @@ extension BaseDataStore { else { return } if (networkStore.isConnectedToNetwork) { - self.syncSubject.onNext(SyncState.Syncing) + self.syncSubject.accept(SyncState.Syncing) } else { - self.syncSubject.onNext(SyncState.Synced) + self.syncSubject.accept(SyncState.Synced) return } queue.async { self.queue.asyncAfter(deadline: .now() + 20, execute: { // this block serves to "cancel" the sync if the operation is running slowly - self.syncSubject.onNext(.Synced) + if (self.syncSubject.value != .Synced) { + self.syncSubject.accept(.Synced) + self.dispatcher.dispatch(action: SentryAction(title: "Sync timeout without error", error: nil, function: "", line: "")) + } }) do { @@ -248,7 +251,7 @@ extension BaseDataStore { } catch let error { NSLog("Unknown error syncing: \(error)") } - self.syncSubject.onNext(SyncState.Synced) + self.syncSubject.accept(SyncState.Synced) } } diff --git a/lockbox-ios/Action/SentryAction.swift b/lockbox-ios/Action/SentryAction.swift index aeb3d021d..588ec4089 100644 --- a/lockbox-ios/Action/SentryAction.swift +++ b/lockbox-ios/Action/SentryAction.swift @@ -6,7 +6,7 @@ import Foundation struct SentryAction: Action { let title: String - let error: Error + let error: Error? let function: String let line: String } diff --git a/lockbox-ios/Store/SentryStore.swift b/lockbox-ios/Store/SentryStore.swift index ceb384300..235d0cae6 100644 --- a/lockbox-ios/Store/SentryStore.swift +++ b/lockbox-ios/Store/SentryStore.swift @@ -18,7 +18,7 @@ class SentryStore { .subscribe(onNext: { (action) in Client.shared?.reportUserException( action.title, - reason: action.error.localizedDescription, + reason: action.error?.localizedDescription ?? "", language: NSLocale.preferredLanguages.first ?? "", lineOfCode: action.line, stackTrace: Thread.callStackSymbols, From 8667dcc6243135b97dd785d85ea38265d35bd245 Mon Sep 17 00:00:00 2001 From: sashei Date: Wed, 5 Jun 2019 11:38:56 -0700 Subject: [PATCH 3/5] include sync timed out message --- Shared/Common/Resources/BaseConstants.swift | 1 + Shared/Store/BaseDataStore.swift | 6 ++++-- lockbox-ios/Presenter/ItemListPresenter.swift | 13 +++++++++++-- lockbox-iosTests/ItemListPresenterSpec.swift | 18 ++++++++++++++++-- 4 files changed, 32 insertions(+), 6 deletions(-) diff --git a/Shared/Common/Resources/BaseConstants.swift b/Shared/Common/Resources/BaseConstants.swift index 6fec3d7a5..50853b7af 100644 --- a/Shared/Common/Resources/BaseConstants.swift +++ b/Shared/Common/Resources/BaseConstants.swift @@ -72,6 +72,7 @@ class Constant { static let usernamePlaceholder = NSLocalizedString("username_placeholder", value: "(no username)", comment: "Placeholder text when there is no username. String should include appropriate open/close parenthetical or similar symbols to indicate this is a placeholder, not a real username.") static let searchYourEntries = NSLocalizedString("search.placeholder", value: "Search logins", comment: "Placeholder text for search field") static let emptyListPlaceholder = NSLocalizedString("list.empty", value: "%@ lets you access passwords you’ve already saved to Firefox. To view your logins here, you’ll need to sign in and sync with Firefox.", comment: "Label shown when there are no logins to list. %@ will be replaced with the application name") + static let syncTimedOut = NSLocalizedString("sync.timeout", value: "Sync timed out", comment: "This is the message displayed when syncing entries from the server times out") } } diff --git a/Shared/Store/BaseDataStore.swift b/Shared/Store/BaseDataStore.swift index 438511b99..3566e911f 100644 --- a/Shared/Store/BaseDataStore.swift +++ b/Shared/Store/BaseDataStore.swift @@ -24,7 +24,7 @@ enum SyncError: Error { } enum SyncState: Equatable { - case Syncing, Synced + case Syncing, Synced, TimedOut public static func ==(lhs: SyncState, rhs: SyncState) -> Bool { switch (lhs, rhs) { @@ -32,6 +32,8 @@ enum SyncState: Equatable { return true case (Synced, Synced): return true + case (TimedOut, TimedOut): + return true default: return false } @@ -239,7 +241,7 @@ extension BaseDataStore { self.queue.asyncAfter(deadline: .now() + 20, execute: { // this block serves to "cancel" the sync if the operation is running slowly if (self.syncSubject.value != .Synced) { - self.syncSubject.accept(.Synced) + self.syncSubject.accept(.TimedOut) self.dispatcher.dispatch(action: SentryAction(title: "Sync timeout without error", error: nil, function: "", line: "")) } }) diff --git a/lockbox-ios/Presenter/ItemListPresenter.swift b/lockbox-ios/Presenter/ItemListPresenter.swift index 88fec4cb6..085626730 100644 --- a/lockbox-ios/Presenter/ItemListPresenter.swift +++ b/lockbox-ios/Presenter/ItemListPresenter.swift @@ -7,7 +7,7 @@ import RxSwift import RxCocoa import RxDataSources -protocol ItemListViewProtocol: AlertControllerView, SpinnerAlertView, BaseItemListViewProtocol { +protocol ItemListViewProtocol: AlertControllerView, StatusAlertView, SpinnerAlertView, BaseItemListViewProtocol { func bind(sortingButtonTitle: Driver) func bind(scrollAction: Driver) var sortingButtonEnabled: AnyObserver? { get } @@ -174,7 +174,7 @@ extension ItemListPresenter { fileprivate func setupSpinnerDisplay() { // when this observable emits an event, the spinner gets dismissed let hideSpinnerObservable = self.dataStore.syncState - .filter { $0 == SyncState.Synced } + .filter { $0 == SyncState.Synced || $0 == SyncState.TimedOut } .map { _ in return () } .asDriver(onErrorJustReturn: ()) @@ -194,6 +194,15 @@ extension ItemListPresenter { } }) .disposed(by: self.disposeBag) + + self.dataStore.syncState + .filter { $0 == SyncState.TimedOut } + .map { _ in () } + .asDriver(onErrorJustReturn: () ) + .drive(onNext: { _ in + self.view?.displayTemporaryAlert(Constant.string.syncTimedOut, timeout: 5) + }) + .disposed(by: self.disposeBag) } } diff --git a/lockbox-iosTests/ItemListPresenterSpec.swift b/lockbox-iosTests/ItemListPresenterSpec.swift index c95a2da20..650cba0d7 100644 --- a/lockbox-iosTests/ItemListPresenterSpec.swift +++ b/lockbox-iosTests/ItemListPresenterSpec.swift @@ -31,10 +31,10 @@ class ItemListPresenterSpec: QuickSpec { var fakeOnSettingsPressed = PublishSubject() var fakeOnSortingButtonPressed = PublishSubject() var setFilterEnabledValue: Bool? - var displayOptionSheetButtons: [AlertActionButtonConfiguration]? - var displayOptionSheetTitle: String? + var temporaryAlertArgument: String? + var displayOptionSheetTitle: String? func bind(items: Driver<[ItemSectionModel]>) { items.drive(itemsObserver).disposed(by: self.disposeBag) } @@ -53,6 +53,10 @@ class ItemListPresenterSpec: QuickSpec { self.displayOptionSheetTitle = title } + func displayTemporaryAlert(_ message: String, timeout: TimeInterval) { + self.temporaryAlertArgument = message + } + func dismissKeyboard() { self.dismissKeyboardCalled = true } @@ -284,6 +288,16 @@ class ItemListPresenterSpec: QuickSpec { } } } + + describe("if the sync times out") { + beforeEach { + self.dataStore.syncStateStub.onNext(SyncState.TimedOut) + } + + it("displays a temporary alert for the user") { + expect(self.view.temporaryAlertArgument).to(equal(Constant.string.syncTimedOut)) + } + } } describe("manual sync") { From 9f2e5cd99380ad89c1717e0af9ac205ee9f43ade Mon Sep 17 00:00:00 2001 From: sashei Date: Thu, 6 Jun 2019 11:43:14 -0700 Subject: [PATCH 4/5] use constant for timeout --- Shared/Common/Resources/BaseConstants.swift | 1 + Shared/Store/BaseDataStore.swift | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Shared/Common/Resources/BaseConstants.swift b/Shared/Common/Resources/BaseConstants.swift index 50853b7af..313e9d483 100644 --- a/Shared/Common/Resources/BaseConstants.swift +++ b/Shared/Common/Resources/BaseConstants.swift @@ -10,6 +10,7 @@ public let isRunningTest = NSClassFromString("XCTestCase") != nil class Constant { class app { static let group = "group.org.mozilla.ios.Lockbox" + static let syncTimeout: Double = 20 } struct fxa { diff --git a/Shared/Store/BaseDataStore.swift b/Shared/Store/BaseDataStore.swift index 3566e911f..45381e454 100644 --- a/Shared/Store/BaseDataStore.swift +++ b/Shared/Store/BaseDataStore.swift @@ -236,9 +236,9 @@ extension BaseDataStore { self.syncSubject.accept(SyncState.Synced) return } - + queue.async { - self.queue.asyncAfter(deadline: .now() + 20, execute: { + self.queue.asyncAfter(deadline: .now() + Constant.app.syncTimeout, execute: { // this block serves to "cancel" the sync if the operation is running slowly if (self.syncSubject.value != .Synced) { self.syncSubject.accept(.TimedOut) From 8f892b7e0bc66554c2d6ad04bbc3e87ea2881499 Mon Sep 17 00:00:00 2001 From: sashei Date: Thu, 6 Jun 2019 12:49:54 -0700 Subject: [PATCH 5/5] fix spec --- lockbox-iosTests/BaseDataStoreSpec.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lockbox-iosTests/BaseDataStoreSpec.swift b/lockbox-iosTests/BaseDataStoreSpec.swift index bb1afdd91..adb6187d8 100644 --- a/lockbox-iosTests/BaseDataStoreSpec.swift +++ b/lockbox-iosTests/BaseDataStoreSpec.swift @@ -388,7 +388,7 @@ class BaseDataStoreSpec: QuickSpec { let syncStates: [SyncState] = self.syncObserver.events.map { $0.value.element! } - expect(syncStates).to(equal([SyncState.Synced, SyncState.Syncing, SyncState.Synced])) + expect(syncStates).to(equal([SyncState.Synced, SyncState.Synced, SyncState.Syncing, SyncState.Synced])) } }