diff --git a/CredentialProvider/Presenter/CredentialItemListPresenter.swift b/CredentialProvider/Presenter/CredentialItemListPresenter.swift index 3aa734ccd..8ffc8cd46 100644 --- a/CredentialProvider/Presenter/CredentialItemListPresenter.swift +++ b/CredentialProvider/Presenter/CredentialItemListPresenter.swift @@ -28,17 +28,17 @@ class ItemListPresenter: BaseItemListPresenter { view.dismissKeyboard() } - self.dataStore.get(id) - .take(1) - .subscribe(onNext: { login in + target.dataStore.get(id) + .map { login -> CredentialStatusAction in if let login = login { - target.dispatcher.dispatch(action: CredentialStatusAction.loginSelected(login: login, relock: false)) + return CredentialStatusAction.loginSelected(login: login, relock: false) } else { - target.dispatcher.dispatch(action: CredentialStatusAction.userCanceled) + return CredentialStatusAction.userCanceled } - }) - .disposed(by: self.disposeBag) - }.asObserver() + } + .subscribe(onNext: { target.dispatcher.dispatch(action: $0) }) + .disposed(by: target.disposeBag) + }.asObserver() } override var helpTextItems: [LoginListCellConfiguration] { return [LoginListCellConfiguration.SelectAPasswordHelpText] } diff --git a/CredentialProvider/Presenter/CredentialProviderPresenter.swift b/CredentialProvider/Presenter/CredentialProviderPresenter.swift index 3da644123..93e4bf34b 100644 --- a/CredentialProvider/Presenter/CredentialProviderPresenter.swift +++ b/CredentialProvider/Presenter/CredentialProviderPresenter.swift @@ -82,14 +82,11 @@ class CredentialProviderPresenter { }) .disposed(by: self.disposeBag) - self.dispatcher.dispatch(action: LifecycleAction.foreground) self.dispatcher.dispatch(action: DataStoreAction.unlock) self.startTelemetry() } func extensionConfigurationRequested() { - self.dispatcher.dispatch(action: LifecycleAction.foreground) - self.dataStore.locked .bind { [weak self] locked in if locked { @@ -104,8 +101,6 @@ class CredentialProviderPresenter { } func credentialProvisionRequested(for credentialIdentity: ASPasswordCredentialIdentity) { - self.dispatcher.dispatch(action: LifecycleAction.foreground) - self.dataStore.locked .take(1) .bind { [weak self] locked in @@ -123,8 +118,6 @@ class CredentialProviderPresenter { } func credentialList(for serviceIdentifiers: [ASCredentialServiceIdentifier]) { - self.dispatcher.dispatch(action: LifecycleAction.foreground) - self.dataStore.locked .asDriver(onErrorJustReturn: true) .drive(onNext: { [weak self] locked in @@ -154,7 +147,10 @@ extension CredentialProviderPresenter { return } - self.dataStore.get(id) + self.dataStore.locked + .filter { !$0 } + .take(1) + .flatMap { _ in self.dataStore.get(id) } .bind { [weak self] login in guard let login = login else { self?.cancelWith(.credentialIdentityNotFound) diff --git a/Shared/Store/BaseDataStore.swift b/Shared/Store/BaseDataStore.swift index d4741ebd8..1a335c735 100644 --- a/Shared/Store/BaseDataStore.swift +++ b/Shared/Store/BaseDataStore.swift @@ -161,7 +161,10 @@ class BaseDataStore { self.lifecycleStore.lifecycleEvents .filter { $0 == .foreground } .subscribe(onNext: { [weak self] _ in - self?.initializeLoginsStorage() + guard let _ = self?.loginsStorage else { + self?.initializeLoginsStorage() + return + } }) .disposed(by: self.disposeBag) @@ -187,12 +190,18 @@ class BaseDataStore { } public func get(_ id: String) -> Observable { - return self.listSubject - .map { items -> LoginRecord? in - return items.filter { item in - return item.id == id - }.first - } + return Observable.create({ [weak self] observer -> Disposable in + self?.queue.async { + do { + let login = try self?.loginsStorage?.get(id: id) + observer.onNext(login) + } catch let error { + observer.onError(error) + } + } + + return Disposables.create() + }) } public func initialized() { @@ -258,7 +267,6 @@ extension BaseDataStore { extension BaseDataStore { private func setupAutolock() { self.lifecycleStore.lifecycleEvents - .distinctUntilChanged() .filter { $0 == .background } .flatMap { _ in self.storageState } .take(1) @@ -270,7 +278,6 @@ extension BaseDataStore { .disposed(by: disposeBag) self.lifecycleStore.lifecycleEvents - .distinctUntilChanged() .filter { $0 == .foreground } .flatMap { _ in self.storageState } .take(1) diff --git a/Shared/Store/BaseItemDetailStore.swift b/Shared/Store/BaseItemDetailStore.swift index 2af1b99fb..072acc1a3 100644 --- a/Shared/Store/BaseItemDetailStore.swift +++ b/Shared/Store/BaseItemDetailStore.swift @@ -10,7 +10,7 @@ class BaseItemDetailStore { internal var dispatcher: Dispatcher internal var disposeBag = DisposeBag() - internal var _itemDetailId = ReplaySubject.create(bufferSize: 1) + internal var _itemDetailId = BehaviorRelay.init(value: "") lazy private(set) var itemDetailId: Observable = { return self._itemDetailId.asObservable() @@ -18,6 +18,5 @@ class BaseItemDetailStore { init(dispatcher: Dispatcher = Dispatcher.shared) { self.dispatcher = dispatcher - self._itemDetailId.onNext("") } } diff --git a/lockbox-ios/Presenter/ItemDetailPresenter.swift b/lockbox-ios/Presenter/ItemDetailPresenter.swift index 9c1b2523b..664008351 100644 --- a/lockbox-ios/Presenter/ItemDetailPresenter.swift +++ b/lockbox-ios/Presenter/ItemDetailPresenter.swift @@ -47,9 +47,6 @@ class ItemDetailPresenter { target.itemDetailStore.itemDetailId .take(1) .flatMap { target.dataStore.get($0) } - // The first `take(1)` call does not push a completion after it emits, so a second one is necessary here - // to ensure that subsequent updates to the item do not result in subsequent dispacthed actions. - .take(1) .map { item -> [Action] in var actions: [Action] = [] if copyableFields.contains(value) { @@ -64,6 +61,7 @@ class ItemDetailPresenter { } return actions } + .observeOn(MainScheduler.instance) .subscribe(onNext: { actions in for action in actions { target.dispatcher.dispatch(action: action) @@ -153,7 +151,6 @@ class ItemDetailPresenter { self.itemDetailStore.itemDetailId .take(1) .flatMap { self.dataStore.get($0) } - .take(1) .flatMap { item -> Observable<[Action]> in var actions: [Action] = [] if let item = item { diff --git a/lockbox-ios/Store/ItemDetailStore.swift b/lockbox-ios/Store/ItemDetailStore.swift index c5958f075..0b0682f76 100644 --- a/lockbox-ios/Store/ItemDetailStore.swift +++ b/lockbox-ios/Store/ItemDetailStore.swift @@ -43,7 +43,7 @@ class ItemDetailStore: BaseItemDetailStore { .subscribe(onNext: { (route) in switch route { case .detail(let itemId): - self._itemDetailId.onNext(itemId) + self._itemDetailId.accept(itemId) case .list: break } @@ -82,7 +82,7 @@ class ItemDetailStore: BaseItemDetailStore { private func showFirstLogin(_ login: LoginRecord?) { if let login = login { runOnMainThread { - self._itemDetailId.onNext(login.id) + self._itemDetailId.accept(login.id) } } } diff --git a/lockbox-iosTests/BaseDataStoreSpec.swift b/lockbox-iosTests/BaseDataStoreSpec.swift index a0b27aa76..cdb00d1d2 100644 --- a/lockbox-iosTests/BaseDataStoreSpec.swift +++ b/lockbox-iosTests/BaseDataStoreSpec.swift @@ -272,7 +272,6 @@ class BaseDataStoreSpec: QuickSpec { self.dataStoreSupport.clearInvocations() self.lifecycleStore.fakeCycle.onNext(.foreground) _ = try! self.subject.storageState.toBlocking().first() - expect(self.dataStoreSupport.createArgument).notTo(beNil()) expect(self.listObserver.events.last!.value.element!).to(beEmpty()) } } @@ -306,6 +305,7 @@ class BaseDataStoreSpec: QuickSpec { } it("locks") { + _ = try! self.subject.storageState.toBlocking().first() let state = try! self.subject.storageState.toBlocking().first() expect(self.loginsStorage.ensureLockedCalled).to(beTrue()) expect(state).to(equal(LoginStoreState.Locked)) @@ -334,6 +334,7 @@ class BaseDataStoreSpec: QuickSpec { it("backdates the next lock time and locks the db") { expect(self.autoLockSupport.backdateCalled).to(beTrue()) + _ = try! self.subject.storageState.toBlocking().first() let state = try! self.subject.storageState.toBlocking().first() expect(self.loginsStorage.ensureLockedCalled).to(beTrue()) expect(state).to(equal(LoginStoreState.Locked)) @@ -376,8 +377,8 @@ class BaseDataStoreSpec: QuickSpec { } it("syncs + pushes syncing followed by synced") { + _ = try! self.subject.syncState.toBlocking().first() expect(self.loginsStorage.syncArgument).notTo(beNil()) - let _ = try! self.subject.syncState.toBlocking().first() let syncStates: [SyncState] = self.syncObserver.events.map { $0.value.element! } @@ -435,17 +436,6 @@ class BaseDataStoreSpec: QuickSpec { expect(self.loginsStorage.closeCalled).to(beTrue()) } } - - describe("foreground events") { - beforeEach { - self.dataStoreSupport.clearInvocations() - self.lifecycleStore.fakeCycle.onNext(.foreground) - } - - it("re-inits the datastore") { - expect(self.dataStoreSupport.createArgument).notTo(beNil()) - } - } } } diff --git a/lockbox-iosTests/CredentialProviderPresenterSpec.swift b/lockbox-iosTests/CredentialProviderPresenterSpec.swift index 00ac9f458..7189df8bf 100644 --- a/lockbox-iosTests/CredentialProviderPresenterSpec.swift +++ b/lockbox-iosTests/CredentialProviderPresenterSpec.swift @@ -71,7 +71,7 @@ class CredentialProviderPresenterSpec: QuickSpec { } class FakeDataStore: DataStore { - let lockedStub = PublishSubject() + let lockedStub = ReplaySubject.create(bufferSize: 1) let getStub = PublishSubject() var getGuid: String? @@ -179,8 +179,7 @@ class CredentialProviderPresenterSpec: QuickSpec { self.subject.extensionConfigurationRequested() } - it("foregrounds the datastore and tells the view to display the welcome screen") { - expect(self.dispatcher.actionArguments.popLast() as? LifecycleAction).to(equal(LifecycleAction.foreground)) + it("tells the view to display the welcome screen") { expect(self.view.displayWelcomeCalled).to(beTrue()) } } @@ -231,32 +230,38 @@ class CredentialProviderPresenterSpec: QuickSpec { it("unlocks the datastore") { expect(self.dispatcher.actionArguments.popLast()! as? DataStoreAction).to(equal(DataStoreAction.unlock)) } - - it("requests the login from the datastore") { - expect(self.dataStore.getGuid).to(equal(passwordIdentity.recordIdentifier)) - } - - describe("when the login is nil") { + describe("after unlocking") { beforeEach { - self.dataStore.getStub.onNext(nil) + self.dataStore.lockedStub.onNext(false) } - it("cancels the request with notFound") { - expect(self.view.fakeExtensionContext.error).notTo(beNil()) - expect((self.view.fakeExtensionContext.error! as NSError).code).to(equal(ASExtensionError.credentialIdentityNotFound.rawValue)) + it("requests the login from the datastore") { + expect(self.dataStore.getGuid).to(equal(passwordIdentity.recordIdentifier)) } - } - describe("when the login is not nil and the password fill completes") { - let login = LoginRecord(fromJSONDict: ["id": guid, "hostname": "http://www.mozilla.com", "username": "dogs@dogs.com", "password": "meow"]) + describe("when the login is nil") { + beforeEach { + self.dataStore.getStub.onNext(nil) + } - beforeEach { - self.dataStore.getStub.onNext(login) + it("cancels the request with notFound") { + expect(self.view.fakeExtensionContext.error).notTo(beNil()) + expect((self.view.fakeExtensionContext.error! as NSError).code).to(equal(ASExtensionError.credentialIdentityNotFound.rawValue)) + } } - it("dispatches the selected credential action") { - expect(self.dispatcher.actionArguments.popLast()! as? CredentialStatusAction).to(equal(CredentialStatusAction.loginSelected(login: login, relock: true))) + describe("when the login is not nil and the password fill completes") { + let login = LoginRecord(fromJSONDict: ["id": guid, "hostname": "http://www.mozilla.com", "username": "dogs@dogs.com", "password": "meow"]) + + beforeEach { + self.dataStore.getStub.onNext(login) + } + + it("dispatches the selected credential action") { + expect(self.dispatcher.actionArguments.popLast()! as? CredentialStatusAction).to(equal(CredentialStatusAction.loginSelected(login: login, relock: true))) + } } + } } diff --git a/lockbox-iosTests/ItemDetailPresenterSpec.swift b/lockbox-iosTests/ItemDetailPresenterSpec.swift index e120e7f20..b33ab9fb4 100644 --- a/lockbox-iosTests/ItemDetailPresenterSpec.swift +++ b/lockbox-iosTests/ItemDetailPresenterSpec.swift @@ -307,7 +307,7 @@ class ItemDetailPresenterSpec: QuickSpec { } it("dispatches nothing new") { - expect(self.dispatcher.dispatchActionArgument.count).to(equal(2)) + expect(self.dispatcher.dispatchActionArgument.count).to(equal(3)) } } }