Skip to content
This repository has been archived by the owner on Dec 14, 2021. It is now read-only.

implement sync timeout #1045

Merged
merged 7 commits into from
Jun 6, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Shared/Common/Resources/BaseConstants.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anyone we need to inform of new/changed localized strings?

}
}

Expand Down
20 changes: 15 additions & 5 deletions Shared/Store/BaseDataStore.swift
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,16 @@ enum SyncError: Error {
}

enum SyncState: Equatable {
case Syncing, Synced
case Syncing, Synced, TimedOut

public static func ==(lhs: SyncState, rhs: SyncState) -> Bool {
switch (lhs, rhs) {
case (Syncing, Syncing):
return true
case (Synced, Synced):
return true
case (TimedOut, TimedOut):
return true
default:
return false
}
Expand Down Expand Up @@ -71,7 +73,7 @@ class BaseDataStore {

internal var disposeBag = DisposeBag()
private var listSubject = BehaviorRelay<[LoginRecord]>(value: [])
private var syncSubject = ReplaySubject<SyncState>.create(bufferSize: 1)
private var syncSubject = BehaviorRelay<SyncState>(value: .Synced)
private var storageStateSubject = ReplaySubject<LoginStoreState>.create(bufferSize: 1)

private let dispatcher: Dispatcher
Expand Down Expand Up @@ -229,21 +231,29 @@ 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: {
Copy link
Contributor

Choose a reason for hiding this comment

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

this queues for 20 seconds from .now(), correct? I ask because I am not super-familiar with the interface, and most other systems seem to use milliseconds.

Also, is it worth making 20 a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it queues for 20 seconds from now. Constant is a good idea!

// this block serves to "cancel" the sync if the operation is running slowly
if (self.syncSubject.value != .Synced) {
self.syncSubject.accept(.TimedOut)
self.dispatcher.dispatch(action: SentryAction(title: "Sync timeout without error", error: nil, function: "", line: ""))
}
})

do {
try self.loginsStorage?.sync(unlockInfo: syncInfo)
} catch let error as LoginStoreError {
self.storageStateSubject.onNext(.Errored(cause: error))
} catch let error {
NSLog("Unknown error syncing: \(error)")
}
self.syncSubject.onNext(SyncState.Synced)
self.syncSubject.accept(SyncState.Synced)
}
}

Expand Down
2 changes: 1 addition & 1 deletion lockbox-ios/Action/SentryAction.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import Foundation

struct SentryAction: Action {
let title: String
let error: Error
let error: Error?
let function: String
let line: String
}
13 changes: 11 additions & 2 deletions lockbox-ios/Presenter/ItemListPresenter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>)
func bind(scrollAction: Driver<ScrollAction>)
var sortingButtonEnabled: AnyObserver<Bool>? { get }
Expand Down Expand Up @@ -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: ())

Expand All @@ -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)
}
}

Expand Down
2 changes: 1 addition & 1 deletion lockbox-ios/Store/SentryStore.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
18 changes: 16 additions & 2 deletions lockbox-iosTests/ItemListPresenterSpec.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ class ItemListPresenterSpec: QuickSpec {
var fakeOnSettingsPressed = PublishSubject<Void>()
var fakeOnSortingButtonPressed = PublishSubject<Void>()
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)
}
Expand All @@ -53,6 +53,10 @@ class ItemListPresenterSpec: QuickSpec {
self.displayOptionSheetTitle = title
}

func displayTemporaryAlert(_ message: String, timeout: TimeInterval) {
self.temporaryAlertArgument = message
}

func dismissKeyboard() {
self.dismissKeyboardCalled = true
}
Expand Down Expand Up @@ -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") {
Expand Down