From 1600d214954d4cc61df0cab768d1086c8436e124 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 18 Jan 2016 16:39:27 +0100 Subject: [PATCH 01/36] Make settings service/network reactive Converted the settings refresh to use observables. This allows us to do a few interesting things relatively easily: - Cancel refresh when observer unsubscribes - Show a message when the request is taking longer than usual - Share a refresh request by many view controllers (not implemented yet, but `.share()` should make it possible) --- .../Networking/AccountSettingsRemote.swift | 30 ++++++++++++- .../Services/AccountSettingsService.swift | 43 +++++++++++++++++++ .../Utility/ImmuTableViewController.swift | 8 ++++ .../Me/MyProfileViewController.swift | 34 ++++++++++++--- 4 files changed, 107 insertions(+), 8 deletions(-) diff --git a/WordPress/Classes/Networking/AccountSettingsRemote.swift b/WordPress/Classes/Networking/AccountSettingsRemote.swift index 3e18e2aec014..8ebf435f0cd2 100644 --- a/WordPress/Classes/Networking/AccountSettingsRemote.swift +++ b/WordPress/Classes/Networking/AccountSettingsRemote.swift @@ -1,12 +1,38 @@ +import AFNetworking import Foundation +import RxSwift class AccountSettingsRemote: ServiceRemoteREST { - func getSettings(success success: AccountSettings -> Void, failure: ErrorType -> Void) { + func settings() -> Observable { + let api = self.api + + return Observable.create { observer in + let remote = AccountSettingsRemote(api: api) + let operation = remote.getSettings( + success: { settings in + observer.onNext(settings) + observer.onCompleted() + }, failure: { error in + DDLogSwift.logError("Error refreshing settings: \(error)") + observer.onError(error) + }) + return AnonymousDisposable() { + if let operation = operation { + if !operation.finished { + DDLogSwift.logError("Canceled refreshing settings") + operation.cancel() + } + } + } + } + } + + func getSettings(success success: AccountSettings -> Void, failure: ErrorType -> Void) -> AFHTTPRequestOperation? { let endpoint = "me/settings" let parameters = ["context": "edit"] let path = pathForEndpoint(endpoint, withVersion: ServiceRemoteRESTApiVersion_1_1) - api.GET(path, + return api.GET(path, parameters: parameters, success: { operation, responseObject in diff --git a/WordPress/Classes/Services/AccountSettingsService.swift b/WordPress/Classes/Services/AccountSettingsService.swift index 9161aa76881a..10791e53351b 100644 --- a/WordPress/Classes/Services/AccountSettingsService.swift +++ b/WordPress/Classes/Services/AccountSettingsService.swift @@ -15,6 +15,28 @@ struct AccountSettingsService { self.userID = userID } + var refresh: Observable { + let remote = self.remote + let stalledTimeout = 4.0 + + let refresh: Observable = remote.settings() + .map { settings in + self.updateSettings(settings) + return .Idle + } + .share() + + let stalled: Observable = Observable + .timer(stalledTimeout, scheduler: MainScheduler.instance) + .map({ _ in .Stalled }) + + return Observable.of(refresh, stalled) + .merge() + .startWith(.Refreshing) + .distinctUntilChanged() + .takeUntil(refresh) + } + func refreshSettings(completion: (Bool) -> Void) { remote.getSettings( success: { @@ -108,4 +130,25 @@ struct AccountSettingsService { enum Errors: ErrorType { case NotFound } + + enum RefreshStatus { + case Idle + case Refreshing + case Stalled + case Failed + case Offline + + var errorMessage: String? { + switch self { + case Stalled: + return NSLocalizedString("We are having trouble loading data", comment: "Error message displayed when a refresh is taking longer than usual. The refresh hasn't failed and it might still succeed") + case Failed: + return NSLocalizedString("We had trouble loading data", comment: "Error message displayed when a refresh failed") + case Offline: + return NSLocalizedString("You are currently offline", comment: "Error message displayed when the app can't connect to the API servers") + case Idle, Refreshing: + return nil + } + } + } } diff --git a/WordPress/Classes/Utility/ImmuTableViewController.swift b/WordPress/Classes/Utility/ImmuTableViewController.swift index 1424d5411186..b9e5efb11251 100644 --- a/WordPress/Classes/Utility/ImmuTableViewController.swift +++ b/WordPress/Classes/Utility/ImmuTableViewController.swift @@ -67,6 +67,14 @@ final class ImmuTableViewController: UITableViewController, ImmuTablePresenter { ImmuTable.registerRows(rows, tableView: tableView) } + var errorMessage: String? = nil { + didSet { + // TODO: write the actuall error message UI + // @koke 2016-01-18 + print("Error message changed: \(errorMessage)") + } + } + // MARK: - Outputs /// Emits a value every time viewWillAppear is called diff --git a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift index 8cd9439e1522..fa0eda6bc427 100644 --- a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift +++ b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift @@ -7,6 +7,8 @@ class MyProfileController: NSObject { let service: AccountSettingsService let viewController = ImmuTableViewController() + private let bag = DisposeBag() + init(service: AccountSettingsService) { self.service = service super.init() @@ -14,14 +16,34 @@ class MyProfileController: NSObject { viewController.title = title viewController.registerRows(immutableRows) - _ = viewModel + viewModel .observeOn(MainScheduler.instance) - .takeUntil(viewController.rx_deallocated) .subscribeNext(viewController.bindViewModel) - // Only refresh on first appearance - _ = viewController.willAppear.take(1).subscribeNext { - service.refreshSettings({ _ in }) - } + .addDisposableTo(bag) + + viewController.willAppear + // On first appearance + .take(1) + // request a refresh of account settings + .flatMap({ service.refresh }) + // replace errors with .Failed status + .catchErrorJustReturn(.Failed) + // convert status to string + .map({ $0.errorMessage }) + // and set the view controller error message + .subscribe { event in + switch event { + case .Next(let status): + self.viewController.errorMessage = status + case .Completed: + self.viewController.errorMessage = nil + case .Error(_): + // We're replacing errors with .Failed, but let's handle it + // just in case. + self.viewController.errorMessage = nil + } + } + .addDisposableTo(bag) } convenience init(account: WPAccount) { From 8a2666625616ceb8e99474cfb579b55e0a00f0c1 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Tue, 19 Jan 2016 11:51:55 +0100 Subject: [PATCH 02/36] Added notices UI for the connection errors --- WordPress/Classes/Utility/Animator.swift | 63 +++++++++++ WordPress/Classes/Utility/ErrorAnimator.swift | 105 ++++++++++++++++++ .../Utility/ImmuTableViewController.swift | 13 ++- .../ViewRelated/Views/PaddedLabel.swift | 67 +++++++++++ WordPress/WordPress.xcodeproj/project.pbxproj | 14 ++- 5 files changed, 258 insertions(+), 4 deletions(-) create mode 100644 WordPress/Classes/Utility/Animator.swift create mode 100644 WordPress/Classes/Utility/ErrorAnimator.swift create mode 100644 WordPress/Classes/ViewRelated/Views/PaddedLabel.swift diff --git a/WordPress/Classes/Utility/Animator.swift b/WordPress/Classes/Utility/Animator.swift new file mode 100644 index 000000000000..31e8e128f97b --- /dev/null +++ b/WordPress/Classes/Utility/Animator.swift @@ -0,0 +1,63 @@ +import UIKit + +/// Animator is a helper to build responsive animations. +/// +/// The main benefit of this class are the preamble and cleanup blocks, which +/// are only called before/after all the animations. +/// +/// You should keep a reference to the animator object, and use the same +/// animator for related animations. +/// +/// A very simple example of preamble and cleanup: +/// +/// class MyViewController: UIViewController { +/// lazy var animator = Animator() +/// var showError: Bool = false { +/// didSet { +/// animator.animateWithDuration(0.3, +/// preamble: { [unowned self] in +/// let view = self.createErrorView() +/// self.view.addSubview(view) +/// self.errorView = view +/// self.errorView?.alpha = 0 +/// }, animations: { [unowned self] in +/// self.errorView?.alpha = 1 +/// }, cleanup: { [unowned self] in +/// self.errorView?.removeFromSuperview() +/// self.errorView = nil +/// }) +/// } +/// } +/// +/// func createErrorView() -> UIView { +/// // Create the error view +/// } +/// var errorView: UIView? = nil +/// } +/// +class Animator: NSObject { + private var animationsInProgress = 0 + + /// Animates changes to one or more views using the specified duration. + /// + /// - parameter preamble: A block called before the animations start. It will only be called if there were no previous animations. + /// - parameter animations: A block object containing the changes to commit to the views. + /// - parameter cleanup: A block called after the animations complete if there are no more pending animations. + func animateWithDuration(duration: NSTimeInterval, preamble: (() -> Void)? = nil, animations: () -> Void, cleanup: (() -> Void)? = nil) { + precondition(NSThread.isMainThread(), "Animator only works on the main (UI) thread") + + if animationsInProgress == 0 { + preamble?() + } + + UIView.animateWithDuration(duration, delay: 0, options: .CurveEaseOut, animations: animations) { [unowned self] _ in + self.animationsInProgress -= 1 + + if self.animationsInProgress == 0 { + cleanup?() + } + } + + animationsInProgress += 1 + } +} diff --git a/WordPress/Classes/Utility/ErrorAnimator.swift b/WordPress/Classes/Utility/ErrorAnimator.swift new file mode 100644 index 000000000000..d3e9b9b2e2d3 --- /dev/null +++ b/WordPress/Classes/Utility/ErrorAnimator.swift @@ -0,0 +1,105 @@ +import UIKit +import WordPressShared + +/// ErrorAnimator is a helper class to animate error messages. +/// +/// The error messages show at the top of the target view, and are meant to +/// appear to be attached to a navigation bar. The expected usage is to display +/// offline status or requests taking longer than usual. +/// +/// To use an ErrorAnimator, you need to keep a reference to it, and call two +/// methods: +/// +/// - `layout()` from your `UIView.layoutSubviews()` or +/// `UIViewController.viewDidLayoutSubviews()`. Failure to do this won't render +/// the animation correctly. +/// +/// - `animateErrorMessage(_)` when you want to change the error displayed. Pass +/// nil if you want to hide the error view. +/// +class ErrorAnimator: Animator { + let animationDuration = 0.3 + let targetHeight: CGFloat = 40 + + private var errorLabel: PaddedLabel? = nil + private var message: String? = nil + private var showingError: Bool { + return (message != nil) + } + let targetView: UIView + var targetTableView: UITableView? { + return targetView as? UITableView + } + + init(target: UIView) { + targetView = target + super.init() + } + + func layout() { + if let errorLabel = errorLabel { + let errorFrame = errorLabel.frame + var frame = targetView.bounds + frame.size.height = errorFrame.height + errorLabel.frame = frame + } + } + + func animateErrorMessage(message: String?) { + let previouslyShowing = showingError + // Are we showing or hiding the message + self.message = message + + if previouslyShowing != showingError { + animateWithDuration(animationDuration, preamble: preamble, animations: animations, cleanup: cleanup) + } + if showingError { + errorLabel?.text = message + } + } + + private func preamble() { + errorLabel = createErrorLabel() + targetView.addSubview(errorLabel!) + errorLabel?.frame.size.height = 0 + errorLabel?.textAlpha = 0 + + UIView.performWithoutAnimation { [unowned self] in + self.targetView.layoutIfNeeded() + } + } + + private func animations() { + if showingError { + errorLabel?.frame.size.height = targetHeight + errorLabel?.textAlpha = 1 + + targetTableView?.contentInset.top += targetHeight + if targetTableView?.contentOffset.y == 0 { + targetTableView?.contentOffset.y = -targetHeight + } + } else { + errorLabel?.frame.size.height = 0 + errorLabel?.textAlpha = 0 + + targetTableView?.contentInset.top -= targetHeight + } + targetView.layoutIfNeeded() + } + + private func cleanup() { + if !showingError { + errorLabel?.removeFromSuperview() + errorLabel = nil + } + } + + private func createErrorLabel() -> PaddedLabel { + let label = PaddedLabel() + label.padding.horizontal = 15 + label.textColor = UIColor.whiteColor() + label.backgroundColor = WPStyleGuide.mediumBlue() + label.font = WPStyleGuide.regularTextFont() + return label + } +} diff --git a/WordPress/Classes/Utility/ImmuTableViewController.swift b/WordPress/Classes/Utility/ImmuTableViewController.swift index b9e5efb11251..f1ed3ed89301 100644 --- a/WordPress/Classes/Utility/ImmuTableViewController.swift +++ b/WordPress/Classes/Utility/ImmuTableViewController.swift @@ -32,6 +32,8 @@ final class ImmuTableViewController: UITableViewController, ImmuTablePresenter { return willAppear as! PublishSubject } + private var errorAnimator: ErrorAnimator! + // MARK: - Table View Controller init() { @@ -45,10 +47,17 @@ final class ImmuTableViewController: UITableViewController, ImmuTablePresenter { override func viewDidLoad() { super.viewDidLoad() + errorAnimator = ErrorAnimator(target: view) + WPStyleGuide.resetReadableMarginsForTableView(tableView) WPStyleGuide.configureColorsForView(view, andTableView: tableView) } + override func viewDidLayoutSubviews() { + super.viewDidLayoutSubviews() + errorAnimator.layout() + } + override func viewWillAppear(animated: Bool) { super.viewWillAppear(animated) willAppearSubject.onNext() @@ -69,9 +78,7 @@ final class ImmuTableViewController: UITableViewController, ImmuTablePresenter { var errorMessage: String? = nil { didSet { - // TODO: write the actuall error message UI - // @koke 2016-01-18 - print("Error message changed: \(errorMessage)") + errorAnimator.animateErrorMessage(errorMessage) } } diff --git a/WordPress/Classes/ViewRelated/Views/PaddedLabel.swift b/WordPress/Classes/ViewRelated/Views/PaddedLabel.swift new file mode 100644 index 000000000000..a3d1a6d54303 --- /dev/null +++ b/WordPress/Classes/ViewRelated/Views/PaddedLabel.swift @@ -0,0 +1,67 @@ +import UIKit + +class PaddedLabel: UIView { + var text: String? { + get { + return label.text + } + + set { + label.text = newValue + } + } + + var textColor: UIColor { + get { + return label.textColor + } + + set { + label.textColor = newValue + } + } + + var font: UIFont { + get { + return label.font + } + + set { + label.font = newValue + } + } + + var textAlpha: CGFloat { + get { + return label.alpha + } + + set { + label.alpha = newValue + } + } + + var padding: (horizontal: CGFloat, vertical: CGFloat) = (0,0) { + didSet { + setNeedsLayout() + } + } + + private let label = UILabel() + + override init(frame: CGRect) { + super.init(frame: frame) + addSubview(label) + } + + required init?(coder aDecoder: NSCoder) { + super.init(coder: aDecoder) + addSubview(label) + } + + override func layoutSubviews() { + super.layoutSubviews() + + label.frame = CGRectInset(bounds, padding.horizontal, padding.vertical) + } +} diff --git a/WordPress/WordPress.xcodeproj/project.pbxproj b/WordPress/WordPress.xcodeproj/project.pbxproj index 1edc2dc69879..54b5a9102d2d 100644 --- a/WordPress/WordPress.xcodeproj/project.pbxproj +++ b/WordPress/WordPress.xcodeproj/project.pbxproj @@ -442,6 +442,7 @@ E10B3655158F2D7800419A93 /* CoreGraphics.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 834CE7371256D0F60046A4A3 /* CoreGraphics.framework */; }; E10B5ACF1C4518E100F6A390 /* AccountService+Rx.swift in Sources */ = {isa = PBXBuildFile; fileRef = E10B5ACE1C4518E100F6A390 /* AccountService+Rx.swift */; }; E11330511A13BAA300D36D84 /* me-sites-with-jetpack.json in Resources */ = {isa = PBXBuildFile; fileRef = E11330501A13BAA300D36D84 /* me-sites-with-jetpack.json */; }; + E11450DF1C4E47E600A6BD0F /* ErrorAnimator.swift in Sources */ = {isa = PBXBuildFile; fileRef = E11450DE1C4E47E600A6BD0F /* ErrorAnimator.swift */; }; E114D79A153D85A800984182 /* WPError.m in Sources */ = {isa = PBXBuildFile; fileRef = E114D799153D85A800984182 /* WPError.m */; }; E1209FA41BB4978B00D69778 /* PeopleService.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1209FA31BB4978B00D69778 /* PeopleService.swift */; }; E120D90E1B09D8C300FB9A6E /* JetpackState.m in Sources */ = {isa = PBXBuildFile; fileRef = E120D90D1B09D8C300FB9A6E /* JetpackState.m */; }; @@ -532,6 +533,8 @@ E1B9128B1BB0129C003C25B9 /* WPStyleGuide+People.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1B9128A1BB0129C003C25B9 /* WPStyleGuide+People.swift */; }; E1B9128F1BB05B1D003C25B9 /* PeopleCell.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1B912841BB01266003C25B9 /* PeopleCell.swift */; }; E1B921BC1C0ED5A3003EA3CB /* MediaSizeSliderCellTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1B921BB1C0ED5A3003EA3CB /* MediaSizeSliderCellTest.swift */; }; + E1BEEC631C4E35A8000B4FA0 /* Animator.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1BEEC621C4E35A8000B4FA0 /* Animator.swift */; }; + E1BEEC651C4E3978000B4FA0 /* PaddedLabel.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1BEEC641C4E3978000B4FA0 /* PaddedLabel.swift */; }; E1C265C91BECFCDD00DC4C6B /* WPCrashlyticsLogger.m in Sources */ = {isa = PBXBuildFile; fileRef = E1C265C81BECFCDD00DC4C6B /* WPCrashlyticsLogger.m */; }; E1C9AA511C10419200732665 /* Math.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1C9AA501C10419200732665 /* Math.swift */; }; E1C9AA561C10427100732665 /* MathTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1C9AA551C10427100732665 /* MathTest.swift */; }; @@ -1429,6 +1432,7 @@ E10B3653158F2D4500419A93 /* UIKit.framework */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = wrapper.framework; name = UIKit.framework; path = System/Library/Frameworks/UIKit.framework; sourceTree = SDKROOT; }; E10B5ACE1C4518E100F6A390 /* AccountService+Rx.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "AccountService+Rx.swift"; sourceTree = ""; }; E11330501A13BAA300D36D84 /* me-sites-with-jetpack.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = "me-sites-with-jetpack.json"; sourceTree = ""; }; + E11450DE1C4E47E600A6BD0F /* ErrorAnimator.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ErrorAnimator.swift; sourceTree = ""; }; E114D798153D85A800984182 /* WPError.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WPError.h; sourceTree = ""; }; E114D799153D85A800984182 /* WPError.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = WPError.m; sourceTree = ""; }; E115F2D116776A2900CCF00D /* WordPress 8.xcdatamodel */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcdatamodel; path = "WordPress 8.xcdatamodel"; sourceTree = ""; }; @@ -1575,6 +1579,8 @@ E1B9128A1BB0129C003C25B9 /* WPStyleGuide+People.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "WPStyleGuide+People.swift"; sourceTree = ""; }; E1B921BB1C0ED5A3003EA3CB /* MediaSizeSliderCellTest.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = MediaSizeSliderCellTest.swift; sourceTree = ""; }; E1BCFBC51C0626C5004BDADF /* WordPress 43.xcdatamodel */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcdatamodel; path = "WordPress 43.xcdatamodel"; sourceTree = ""; }; + E1BEEC621C4E35A8000B4FA0 /* Animator.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = Animator.swift; sourceTree = ""; }; + E1BEEC641C4E3978000B4FA0 /* PaddedLabel.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = PaddedLabel.swift; sourceTree = ""; }; E1C265C71BECFCDD00DC4C6B /* WPCrashlyticsLogger.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WPCrashlyticsLogger.h; sourceTree = ""; }; E1C265C81BECFCDD00DC4C6B /* WPCrashlyticsLogger.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = WPCrashlyticsLogger.m; sourceTree = ""; }; E1C807471696F72E00E545A6 /* WordPress 9.xcdatamodel */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcdatamodel; path = "WordPress 9.xcdatamodel"; sourceTree = ""; }; @@ -1877,6 +1883,7 @@ C58349C31806F95100B64089 /* IOS7CorrectedTextView.h */, C58349C41806F95100B64089 /* IOS7CorrectedTextView.m */, B5B410B51B1772B000CFCF8D /* NavigationTitleView.swift */, + E1BEEC641C4E3978000B4FA0 /* PaddedLabel.swift */, B57B92BC1B73B08100DFF00B /* SeparatorsView.swift */, 37022D8F1981BF9200F322B7 /* VerticallyStackedButton.h */, 37022D901981BF9200F322B7 /* VerticallyStackedButton.m */, @@ -2621,6 +2628,7 @@ 852416CC1A12EAF70030700C /* Ratings */, E1523EB216D3B2EE002C5A36 /* Sharing */, B526DC241B1E473B002A8C5F /* WebViewController */, + E1BEEC621C4E35A8000B4FA0 /* Animator.swift */, C545E0A01811B9880020844C /* ContextManager.h */, 93EF094B19ED4F1100C89770 /* ContextManager-Internals.h */, C545E0A11811B9880020844C /* ContextManager.m */, @@ -2628,8 +2636,9 @@ FD9A948B12FAEA2300438F94 /* DateUtils.m */, 93A379D919FE6D3000415023 /* DDLogSwift.h */, 93A379DA19FE6D3000415023 /* DDLogSwift.m */, - 313692771A5D6F7900EBE645 /* HelpshiftUtils.h */, + E11450DE1C4E47E600A6BD0F /* ErrorAnimator.swift */, E1266D2C1BBE8B9A00FCB6B6 /* Gravatar.swift */, + 313692771A5D6F7900EBE645 /* HelpshiftUtils.h */, 313692781A5D6F7900EBE645 /* HelpshiftUtils.m */, E1EBC36E1C118EA500F638E0 /* ImmuTable.swift */, E1E49CE31C4902EE002393A4 /* ImmuTableViewController.swift */, @@ -4426,6 +4435,7 @@ F1A0C49C1AF65B02001B544C /* MFMessageComposeViewController+StatusBarStyle.m in Sources */, ACBAB6860E1247F700F38795 /* PostPreviewViewController.m in Sources */, 5D2FB2861AE98C6600F1D4ED /* RestorePostTableViewCell.m in Sources */, + E1BEEC651C4E3978000B4FA0 /* PaddedLabel.swift in Sources */, E1A6DBE519DC7D230071AC1E /* PostService.m in Sources */, C58349C51806F95100B64089 /* IOS7CorrectedTextView.m in Sources */, 594DB2951AB891A200E2E456 /* WPUserAgent.m in Sources */, @@ -4607,6 +4617,7 @@ B532D4EE199D4418006E4DF6 /* NoteBlockImageTableViewCell.swift in Sources */, E10B5ACF1C4518E100F6A390 /* AccountService+Rx.swift in Sources */, 93FA59DD18D88C1C001446BC /* PostCategoryService.m in Sources */, + E1BEEC631C4E35A8000B4FA0 /* Animator.swift in Sources */, 5DCC4CD819A50CC0003E548C /* ReaderSite.m in Sources */, 93C4864F181043D700A24725 /* ActivityLogDetailViewController.m in Sources */, 859F761D18F2159800EF8D5D /* WPAnalyticsTrackerMixpanelInstructionsForStat.m in Sources */, @@ -4750,6 +4761,7 @@ 74D5FFD619ACDF6700389E8F /* WPLegacyEditPostViewController.m in Sources */, B54E1DF41A0A7BBF00807537 /* NotificationMediaDownloader.swift in Sources */, E174F6E6172A73960004F23A /* WPAccount.m in Sources */, + E11450DF1C4E47E600A6BD0F /* ErrorAnimator.swift in Sources */, E100C6BB1741473000AE48D8 /* WordPress-11-12.xcmappingmodel in Sources */, E1A03EE217422DCF0085D192 /* BlogToAccount.m in Sources */, 5D44EB381986D8BA008B7175 /* ReaderSiteService.m in Sources */, From d50c2078264007beb62f52663ebb5e77b8e8dfc3 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Tue, 19 Jan 2016 12:08:06 +0100 Subject: [PATCH 03/36] Added link to WWDC video in the documentation --- WordPress/Classes/Utility/Animator.swift | 2 ++ 1 file changed, 2 insertions(+) diff --git a/WordPress/Classes/Utility/Animator.swift b/WordPress/Classes/Utility/Animator.swift index 31e8e128f97b..65d10acbd5ea 100644 --- a/WordPress/Classes/Utility/Animator.swift +++ b/WordPress/Classes/Utility/Animator.swift @@ -35,6 +35,8 @@ import UIKit /// var errorView: UIView? = nil /// } /// +/// Animator is heavily inspired by the final demo on WWDC 2014 Session 236 +/// [Building Interruptible and Responsive Interactions](https://developer.apple.com/videos/play/wwdc2014-236/). class Animator: NSObject { private var animationsInProgress = 0 From 9336aedd2743c99408c28325f3aa77ef42cbbbcd Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Tue, 19 Jan 2016 12:38:19 +0100 Subject: [PATCH 04/36] Remove PaddedLabel accessors Instead, expose the underlying label and modify its attributes directly --- WordPress/Classes/Utility/ErrorAnimator.swift | 20 ++++----- .../ViewRelated/Views/PaddedLabel.swift | 42 +------------------ 2 files changed, 11 insertions(+), 51 deletions(-) diff --git a/WordPress/Classes/Utility/ErrorAnimator.swift b/WordPress/Classes/Utility/ErrorAnimator.swift index d3e9b9b2e2d3..de98330893cb 100644 --- a/WordPress/Classes/Utility/ErrorAnimator.swift +++ b/WordPress/Classes/Utility/ErrorAnimator.swift @@ -54,7 +54,7 @@ class ErrorAnimator: Animator { animateWithDuration(animationDuration, preamble: preamble, animations: animations, cleanup: cleanup) } if showingError { - errorLabel?.text = message + errorLabel?.label.text = message } } @@ -62,7 +62,7 @@ class ErrorAnimator: Animator { errorLabel = createErrorLabel() targetView.addSubview(errorLabel!) errorLabel?.frame.size.height = 0 - errorLabel?.textAlpha = 0 + errorLabel?.label.alpha = 0 UIView.performWithoutAnimation { [unowned self] in self.targetView.layoutIfNeeded() @@ -72,7 +72,7 @@ class ErrorAnimator: Animator { private func animations() { if showingError { errorLabel?.frame.size.height = targetHeight - errorLabel?.textAlpha = 1 + errorLabel?.label.alpha = 1 targetTableView?.contentInset.top += targetHeight if targetTableView?.contentOffset.y == 0 { @@ -80,7 +80,7 @@ class ErrorAnimator: Animator { } } else { errorLabel?.frame.size.height = 0 - errorLabel?.textAlpha = 0 + errorLabel?.label.alpha = 0 targetTableView?.contentInset.top -= targetHeight } @@ -95,11 +95,11 @@ class ErrorAnimator: Animator { } private func createErrorLabel() -> PaddedLabel { - let label = PaddedLabel() - label.padding.horizontal = 15 - label.textColor = UIColor.whiteColor() - label.backgroundColor = WPStyleGuide.mediumBlue() - label.font = WPStyleGuide.regularTextFont() - return label + let paddedLabel = PaddedLabel() + paddedLabel.padding.horizontal = 15 + paddedLabel.label.textColor = UIColor.whiteColor() + paddedLabel.backgroundColor = WPStyleGuide.mediumBlue() + paddedLabel.label.font = WPStyleGuide.regularTextFont() + return paddedLabel } } diff --git a/WordPress/Classes/ViewRelated/Views/PaddedLabel.swift b/WordPress/Classes/ViewRelated/Views/PaddedLabel.swift index a3d1a6d54303..636bd3325f0e 100644 --- a/WordPress/Classes/ViewRelated/Views/PaddedLabel.swift +++ b/WordPress/Classes/ViewRelated/Views/PaddedLabel.swift @@ -1,53 +1,13 @@ import UIKit class PaddedLabel: UIView { - var text: String? { - get { - return label.text - } - - set { - label.text = newValue - } - } - - var textColor: UIColor { - get { - return label.textColor - } - - set { - label.textColor = newValue - } - } - - var font: UIFont { - get { - return label.font - } - - set { - label.font = newValue - } - } - - var textAlpha: CGFloat { - get { - return label.alpha - } - - set { - label.alpha = newValue - } - } - var padding: (horizontal: CGFloat, vertical: CGFloat) = (0,0) { didSet { setNeedsLayout() } } - private let label = UILabel() + let label = UILabel() override init(frame: CGRect) { super.init(frame: frame) From 2cbf62c3209b0637ea32875003ef287c204dca36 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Tue, 19 Jan 2016 15:53:32 +0100 Subject: [PATCH 05/36] Adds reachability to refresh logic --- .../Services/AccountSettingsService.swift | 13 +++++++++++- .../Classes/Utility/Reachability+Rx.swift | 20 +++++++++++++++++++ .../Me/MyProfileViewController.swift | 16 ++++----------- WordPress/WordPress.xcodeproj/project.pbxproj | 4 ++++ 4 files changed, 40 insertions(+), 13 deletions(-) create mode 100644 WordPress/Classes/Utility/Reachability+Rx.swift diff --git a/WordPress/Classes/Services/AccountSettingsService.swift b/WordPress/Classes/Services/AccountSettingsService.swift index bea68e37fbc6..6b2b1425f675 100644 --- a/WordPress/Classes/Services/AccountSettingsService.swift +++ b/WordPress/Classes/Services/AccountSettingsService.swift @@ -1,4 +1,5 @@ import Foundation +import Reachability import RxCocoa import RxSwift @@ -19,6 +20,8 @@ struct AccountSettingsService { let remote = self.remote let stalledTimeout = 4.0 + let reachability = Reachability.internetConnection + let refresh: Observable = remote.settings() .map { settings in self.updateSettings(settings) @@ -30,11 +33,19 @@ struct AccountSettingsService { .timer(stalledTimeout, scheduler: MainScheduler.instance) .map({ _ in .Stalled }) - return Observable.of(refresh, stalled) + let request = Observable.of(refresh, stalled) .merge() .startWith(.Refreshing) .distinctUntilChanged() .takeUntil(refresh) + + return reachability.flatMapLatest({ reachable -> Observable in + if reachable { + return request + } else { + return Observable.just(.Offline) + } + }) } func refreshSettings(completion: (Bool) -> Void) { diff --git a/WordPress/Classes/Utility/Reachability+Rx.swift b/WordPress/Classes/Utility/Reachability+Rx.swift new file mode 100644 index 000000000000..f53f32709f1c --- /dev/null +++ b/WordPress/Classes/Utility/Reachability+Rx.swift @@ -0,0 +1,20 @@ +import Foundation +import Reachability +import RxSwift + +extension Reachability { + static let internetConnection = Observable.create { observer in + let reach = Reachability.reachabilityForInternetConnection() + reach.reachableBlock = { _ in + observer.onNext(true) + } + reach.unreachableBlock = { _ in + observer.onNext(false) + } + observer.onNext(reach.isReachable()) + reach.startNotifier() + return AnonymousDisposable() { + reach.stopNotifier() + } + }.shareReplayLatestWhileConnected() +} diff --git a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift index fa0eda6bc427..1e5faf0573a9 100644 --- a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift +++ b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift @@ -25,23 +25,15 @@ class MyProfileController: NSObject { // On first appearance .take(1) // request a refresh of account settings - .flatMap({ service.refresh }) + .flatMapLatest({ service.refresh }) // replace errors with .Failed status .catchErrorJustReturn(.Failed) // convert status to string .map({ $0.errorMessage }) // and set the view controller error message - .subscribe { event in - switch event { - case .Next(let status): - self.viewController.errorMessage = status - case .Completed: - self.viewController.errorMessage = nil - case .Error(_): - // We're replacing errors with .Failed, but let's handle it - // just in case. - self.viewController.errorMessage = nil - } + .observeOn(MainScheduler.instance) + .subscribeNext { [weak self] message in + self?.viewController.errorMessage = message } .addDisposableTo(bag) } diff --git a/WordPress/WordPress.xcodeproj/project.pbxproj b/WordPress/WordPress.xcodeproj/project.pbxproj index 006e7f9217a0..8a3744ef3f81 100644 --- a/WordPress/WordPress.xcodeproj/project.pbxproj +++ b/WordPress/WordPress.xcodeproj/project.pbxproj @@ -476,6 +476,7 @@ E149D64E19349E69006A843D /* AccountServiceRemoteREST.m in Sources */ = {isa = PBXBuildFile; fileRef = E149D64619349E69006A843D /* AccountServiceRemoteREST.m */; }; E149D65019349E69006A843D /* MediaServiceRemoteREST.m in Sources */ = {isa = PBXBuildFile; fileRef = E149D64B19349E69006A843D /* MediaServiceRemoteREST.m */; }; E149D65119349E69006A843D /* MediaServiceRemoteXMLRPC.m in Sources */ = {isa = PBXBuildFile; fileRef = E149D64D19349E69006A843D /* MediaServiceRemoteXMLRPC.m */; }; + E14B13C31C4E7675009DD68F /* Reachability+Rx.swift in Sources */ = {isa = PBXBuildFile; fileRef = E14B13C21C4E7675009DD68F /* Reachability+Rx.swift */; }; E1556CF2193F6FE900FC52EA /* CommentService.m in Sources */ = {isa = PBXBuildFile; fileRef = E1556CF1193F6FE900FC52EA /* CommentService.m */; }; E15618FD16DB8677006532C4 /* UIKitTestHelper.m in Sources */ = {isa = PBXBuildFile; fileRef = E15618FC16DB8677006532C4 /* UIKitTestHelper.m */; }; E15618FF16DBA983006532C4 /* xmlrpc-response-newpost.xml in Resources */ = {isa = PBXBuildFile; fileRef = E15618FE16DBA983006532C4 /* xmlrpc-response-newpost.xml */; }; @@ -1503,6 +1504,7 @@ E149D64B19349E69006A843D /* MediaServiceRemoteREST.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MediaServiceRemoteREST.m; sourceTree = ""; }; E149D64C19349E69006A843D /* MediaServiceRemoteXMLRPC.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MediaServiceRemoteXMLRPC.h; sourceTree = ""; }; E149D64D19349E69006A843D /* MediaServiceRemoteXMLRPC.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MediaServiceRemoteXMLRPC.m; sourceTree = ""; }; + E14B13C21C4E7675009DD68F /* Reachability+Rx.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "Reachability+Rx.swift"; sourceTree = ""; }; E14D65C717E09663007E3EA4 /* Social.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Social.framework; path = System/Library/Frameworks/Social.framework; sourceTree = SDKROOT; }; E150520B16CAC5C400D3DDDC /* BlogJetpackTest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = BlogJetpackTest.m; sourceTree = ""; }; E150520D16CAC75A00D3DDDC /* CoreDataTestHelper.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CoreDataTestHelper.h; sourceTree = ""; }; @@ -2664,6 +2666,7 @@ E13A8C9A1C3E6EF2005BB1C1 /* ImmuTable+WordPress.swift */, 5DB4683918A2E718004A89A9 /* LocationService.h */, 5DB4683A18A2E718004A89A9 /* LocationService.m */, + E14B13C21C4E7675009DD68F /* Reachability+Rx.swift */, 5D3E334C15EEBB6B005FC6F2 /* ReachabilityUtils.h */, 5D3E334D15EEBB6B005FC6F2 /* ReachabilityUtils.m */, 85D239B41AE5A6170074768D /* ReachabilityFacade.h */, @@ -4782,6 +4785,7 @@ 5D44EB381986D8BA008B7175 /* ReaderSiteService.m in Sources */, E1A03F48174283E10085D192 /* BlogToJetpackAccount.m in Sources */, B522C4F81B3DA79B00E47B59 /* NotificationSettingsViewController.swift in Sources */, + E14B13C31C4E7675009DD68F /* Reachability+Rx.swift in Sources */, B587797C19B799D800E57C5A /* NSParagraphStyle+Helpers.swift in Sources */, 5D6C4B121B604190005E3C43 /* RichTextView.swift in Sources */, 5D119DA3176FBE040073D83A /* UIImageView+AFNetworkingExtra.m in Sources */, From 07e5e602623c863dbf44586cfb881a93b5d2d34f Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Thu, 21 Jan 2016 10:03:19 +0100 Subject: [PATCH 06/36] Add Rx forwardIf operator --- WordPress/Classes/Extensions/Rx.swift | 61 +++++++++++++++++++ WordPress/WordPress.xcodeproj/project.pbxproj | 4 ++ 2 files changed, 65 insertions(+) create mode 100644 WordPress/Classes/Extensions/Rx.swift diff --git a/WordPress/Classes/Extensions/Rx.swift b/WordPress/Classes/Extensions/Rx.swift new file mode 100644 index 000000000000..b5b9e9a6ab67 --- /dev/null +++ b/WordPress/Classes/Extensions/Rx.swift @@ -0,0 +1,61 @@ +import RxSwift + +// MARK: - forwardIf +// This has been proposed for inclusion in RxSwift +// https://github.com/ReactiveX/RxSwift/pull/422 +// I can't copy the exact implementation here since it relies on internal classes +// I added an alternative implementation instead +// @koke 2016-01-21 + +extension ObservableType { + + /** + Propagates the source observable sequence while the condition observable sequence last value is true. + + - parameter source: Source observable sequence to propagate + - parameter condition: Boolean observable sequence that dictates if the source propagates. + - returns: An observable sequence that subscribes and emits the values of the source observable as long as the last emitted value of the condition observable is true. + */ + public func forwardIf(condition: ConditionO) -> Observable { + return ForwardIf(source: self, condition: condition.asObservable()).asObservable() + } +} + +class ForwardIf: ObservableType { + typealias E = S.E + typealias DisposeKey = CompositeDisposable.DisposeKey + + private let _source: S + private let _condition: Observable + private let _controller = PublishSubject() + private let _group = CompositeDisposable() + + private var _connectionKey: DisposeKey? = nil + + init(source: S, condition: Observable) { + _source = source + _condition = condition + } + + func subscribe(observer: O) -> Disposable { + let conn = _source.publish() + let connection = conn.subscribe(observer) + _group.addDisposable(connection) + + let subscription = _condition + .distinctUntilChanged() + .subscribeNext { active in + if active { + self._connectionKey = self._group.addDisposable(conn.connect()) + } else { + if let connectionKey = self._connectionKey { + self._group.removeDisposable(connectionKey) + self._connectionKey = nil + } + } + } + _group.addDisposable(subscription) + + return _group + } +} diff --git a/WordPress/WordPress.xcodeproj/project.pbxproj b/WordPress/WordPress.xcodeproj/project.pbxproj index f75c0457a19d..0b2e70d5760d 100644 --- a/WordPress/WordPress.xcodeproj/project.pbxproj +++ b/WordPress/WordPress.xcodeproj/project.pbxproj @@ -450,6 +450,7 @@ E11330511A13BAA300D36D84 /* me-sites-with-jetpack.json in Resources */ = {isa = PBXBuildFile; fileRef = E11330501A13BAA300D36D84 /* me-sites-with-jetpack.json */; }; E11450DF1C4E47E600A6BD0F /* ErrorAnimator.swift in Sources */ = {isa = PBXBuildFile; fileRef = E11450DE1C4E47E600A6BD0F /* ErrorAnimator.swift */; }; E114D79A153D85A800984182 /* WPError.m in Sources */ = {isa = PBXBuildFile; fileRef = E114D799153D85A800984182 /* WPError.m */; }; + E116D4411C50D5F400DC5593 /* Rx.swift in Sources */ = {isa = PBXBuildFile; fileRef = E116D4401C50D5F400DC5593 /* Rx.swift */; }; E1209FA41BB4978B00D69778 /* PeopleService.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1209FA31BB4978B00D69778 /* PeopleService.swift */; }; E120D90E1B09D8C300FB9A6E /* JetpackState.m in Sources */ = {isa = PBXBuildFile; fileRef = E120D90D1B09D8C300FB9A6E /* JetpackState.m */; }; E1249B4319408C910035E895 /* RemoteComment.m in Sources */ = {isa = PBXBuildFile; fileRef = E1249B4219408C910035E895 /* RemoteComment.m */; }; @@ -1457,6 +1458,7 @@ E114D798153D85A800984182 /* WPError.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WPError.h; sourceTree = ""; }; E114D799153D85A800984182 /* WPError.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = WPError.m; sourceTree = ""; }; E115F2D116776A2900CCF00D /* WordPress 8.xcdatamodel */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcdatamodel; path = "WordPress 8.xcdatamodel"; sourceTree = ""; }; + E116D4401C50D5F400DC5593 /* Rx.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = Rx.swift; sourceTree = ""; }; E1209FA31BB4978B00D69778 /* PeopleService.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = PeopleService.swift; sourceTree = ""; }; E120D90C1B09D8C300FB9A6E /* JetpackState.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JetpackState.h; sourceTree = ""; }; E120D90D1B09D8C300FB9A6E /* JetpackState.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = JetpackState.m; sourceTree = ""; }; @@ -3121,6 +3123,7 @@ FFB1FA9F1BF0EC4E0090C761 /* PHAsset+Exporters.swift */, E1C9AA501C10419200732665 /* Math.swift */, E131F5341C2930FC00D2D975 /* String+Helpers.swift */, + E116D4401C50D5F400DC5593 /* Rx.swift */, ); path = Extensions; sourceTree = ""; @@ -4659,6 +4662,7 @@ E1E49CE41C4902EE002393A4 /* ImmuTableViewController.swift in Sources */, E616E4B31C480896002C024E /* SharingService.swift in Sources */, E1FD45E01C030B3800750F4C /* AccountSettingsService.swift in Sources */, + E116D4411C50D5F400DC5593 /* Rx.swift in Sources */, E1D0D81616D3B86800E33F4C /* SafariActivity.m in Sources */, E603C7701BC94AED00AD49D7 /* WordPress-37-38.xcmappingmodel in Sources */, FF0AAE0D1A16550D0089841D /* WPMediaProgressTableViewController.m in Sources */, From 3abc7149bedf7dbb96ec19402bd29d7e564b59da Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Thu, 21 Jan 2016 18:09:06 +0100 Subject: [PATCH 07/36] Don't emit an error for canceled requests --- .../Classes/Networking/AccountSettingsRemote.swift | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/WordPress/Classes/Networking/AccountSettingsRemote.swift b/WordPress/Classes/Networking/AccountSettingsRemote.swift index 8ebf435f0cd2..ce964af99642 100644 --- a/WordPress/Classes/Networking/AccountSettingsRemote.swift +++ b/WordPress/Classes/Networking/AccountSettingsRemote.swift @@ -13,13 +13,19 @@ class AccountSettingsRemote: ServiceRemoteREST { observer.onNext(settings) observer.onCompleted() }, failure: { error in - DDLogSwift.logError("Error refreshing settings: \(error)") - observer.onError(error) + let nserror = error as NSError + if nserror.domain == NSURLErrorDomain && nserror.code == NSURLErrorCancelled { + // If we canceled the operation, don't propagate the error + // This probably means the observable is being disposed + DDLogSwift.logError("Canceled refreshing settings") + } else { + DDLogSwift.logError("Error refreshing settings: \(error)") + observer.onError(error) + } }) return AnonymousDisposable() { if let operation = operation { if !operation.finished { - DDLogSwift.logError("Canceled refreshing settings") operation.cancel() } } From f65397341d700d2a9a1da37a387b626b53b09f6d Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Thu, 21 Jan 2016 18:09:45 +0100 Subject: [PATCH 08/36] Stop refreshing when VC disappears --- .../Utility/ImmuTableViewController.swift | 17 +++++++++++------ .../Me/MyProfileViewController.swift | 7 ++----- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/WordPress/Classes/Utility/ImmuTableViewController.swift b/WordPress/Classes/Utility/ImmuTableViewController.swift index f1ed3ed89301..cd0e1cfc6cd5 100644 --- a/WordPress/Classes/Utility/ImmuTableViewController.swift +++ b/WordPress/Classes/Utility/ImmuTableViewController.swift @@ -28,9 +28,7 @@ final class ImmuTableViewController: UITableViewController, ImmuTablePresenter { return ImmuTableViewHandler(takeOver: self) }() - private var willAppearSubject: PublishSubject { - return willAppear as! PublishSubject - } + private var visibleSubject = PublishSubject() private var errorAnimator: ErrorAnimator! @@ -60,7 +58,12 @@ final class ImmuTableViewController: UITableViewController, ImmuTablePresenter { override func viewWillAppear(animated: Bool) { super.viewWillAppear(animated) - willAppearSubject.onNext() + visibleSubject.on(.Next(true)) + } + + override func viewDidDisappear(animated: Bool) { + super.viewDidDisappear(animated) + visibleSubject.on(.Next(false)) } // MARK: - Inputs @@ -84,6 +87,8 @@ final class ImmuTableViewController: UITableViewController, ImmuTablePresenter { // MARK: - Outputs - /// Emits a value every time viewWillAppear is called - let willAppear: Observable = PublishSubject() + /// Emits a value when the view controller appears or disappears + var visible: Observable { + return visibleSubject + } } diff --git a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift index 1e5faf0573a9..1d08926ae15f 100644 --- a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift +++ b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift @@ -21,11 +21,8 @@ class MyProfileController: NSObject { .subscribeNext(viewController.bindViewModel) .addDisposableTo(bag) - viewController.willAppear - // On first appearance - .take(1) - // request a refresh of account settings - .flatMapLatest({ service.refresh }) + service.refresh + .forwardIf(viewController.visible) // replace errors with .Failed status .catchErrorJustReturn(.Failed) // convert status to string From ea137d840854299217bc946ebc08d804963cba90 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Thu, 21 Jan 2016 18:10:48 +0100 Subject: [PATCH 09/36] Improved refreshing logic - Split multiple observables into their own properties. - Adds retry logic for recoverable errors. - Reuse existing remotes to avoid duplicating network observables. - Improved logging - Adds polling - Improves "Stalled" logic so it doesn't duplicate/cancel requests --- .../Networking/AccountSettingsRemote.swift | 15 ++++ .../Services/AccountSettingsService.swift | 71 ++++++++++++++----- 2 files changed, 69 insertions(+), 17 deletions(-) diff --git a/WordPress/Classes/Networking/AccountSettingsRemote.swift b/WordPress/Classes/Networking/AccountSettingsRemote.swift index ce964af99642..24cb436830ee 100644 --- a/WordPress/Classes/Networking/AccountSettingsRemote.swift +++ b/WordPress/Classes/Networking/AccountSettingsRemote.swift @@ -3,6 +3,21 @@ import Foundation import RxSwift class AccountSettingsRemote: ServiceRemoteREST { + static let remotes = NSMapTable(keyOptions: .StrongMemory, valueOptions: .WeakMemory) + + static func remoteWithApi(api: WordPressComApi) -> AccountSettingsRemote { + let key = api.authToken.hashValue + // FIXME: not thread safe + // @koke 2016-01-21 + if let remote = remotes.objectForKey(key) { + return remote as! AccountSettingsRemote + } else { + let remote = AccountSettingsRemote(api: api) + remotes.setObject(remote, forKey: key) + return remote + } + } + func settings() -> Observable { let api = self.api diff --git a/WordPress/Classes/Services/AccountSettingsService.swift b/WordPress/Classes/Services/AccountSettingsService.swift index 6b2b1425f675..11efdc507d1e 100644 --- a/WordPress/Classes/Services/AccountSettingsService.swift +++ b/WordPress/Classes/Services/AccountSettingsService.swift @@ -6,42 +6,79 @@ import RxSwift let AccountSettingsServiceChangeSaveFailedNotification = "AccountSettingsServiceChangeSaveFailed" struct AccountSettingsService { + let stallTimeout = 4.0 + let maxRetries = 3 + let pollingInterval = 60.0 + let remote: AccountSettingsRemote let userID: Int private let context = ContextManager.sharedInstance().mainContext init(userID: Int, api: WordPressComApi) { - self.remote = AccountSettingsRemote(api: api) + self.remote = AccountSettingsRemote.remoteWithApi(api) self.userID = userID } - var refresh: Observable { - let remote = self.remote - let stalledTimeout = 4.0 - - let reachability = Reachability.internetConnection + var reachable: Observable { + return Reachability.internetConnection + } - let refresh: Observable = remote.settings() - .map { settings in + var remoteSettings: Observable { + return remote.settings() + .map({ settings -> RefreshStatus in self.updateSettings(settings) return .Idle - } + }) + .catchError({ error in + let error = error as NSError + // We want to retry only for networking errors, so we convert errors that aren't + // on NSURLErrorDomain to a .Failed status and log them. + if error.domain == NSURLErrorDomain { + DDLogSwift.logError("Error refreshing settings (will retry): \(error)") + throw error + } else { + DDLogSwift.logError("Error refreshing settings (unrecoverable): \(error)") + return Observable.just(.Failed) + } + }) + .retry(maxRetries) + .doOn(onError: { (error) -> Void in + DDLogSwift.logError("Error refreshing settings (maxRetries reached): \(error)") + }) .share() + } - let stalled: Observable = Observable - .timer(stalledTimeout, scheduler: MainScheduler.instance) - .map({ _ in .Stalled }) + var stalled: Observable { + return Observable + .just(.Stalled) + .delaySubscription(stallTimeout, scheduler: MainScheduler.instance) + } - let request = Observable.of(refresh, stalled) + var request: Observable { + let remoteSettings = self.remoteSettings + let stalledSettings = Observable.of(stalled, remoteSettings) .merge() + + return remoteSettings + .amb(stalledSettings) .startWith(.Refreshing) - .distinctUntilChanged() - .takeUntil(refresh) + } + + var refresh: Observable { + // Copy request to avoid capture of self in closure + let request = self.request + + // Convert to a polling request + let polling = Observable + .interval(pollingInterval, scheduler: MainScheduler.instance) + .startWith(0) + .flatMapLatest({ _ in request }) - return reachability.flatMapLatest({ reachable -> Observable in + // Enable only when reachable, otherwise emit .Offline + return reachable.flatMapLatest({ reachable -> Observable in if reachable { - return request + return polling } else { return Observable.just(.Offline) } From 2c31f909395737355dd853e5d65111f09c5abe65 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Thu, 21 Jan 2016 19:26:11 +0100 Subject: [PATCH 10/36] Refactor settings remote to multicast Instead of creating an observable for each call to settings, store it as a property and add `.share()` so multiple subscriptions don't cause multiple netwokr requests. --- .../Classes/Networking/AccountSettingsRemote.swift | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/WordPress/Classes/Networking/AccountSettingsRemote.swift b/WordPress/Classes/Networking/AccountSettingsRemote.swift index 24cb436830ee..01d561eba7d2 100644 --- a/WordPress/Classes/Networking/AccountSettingsRemote.swift +++ b/WordPress/Classes/Networking/AccountSettingsRemote.swift @@ -18,10 +18,15 @@ class AccountSettingsRemote: ServiceRemoteREST { } } - func settings() -> Observable { - let api = self.api + let settings: Observable - return Observable.create { observer in + override init(api: WordPressComApi) { + settings = AccountSettingsRemote.settingsWithApi(api) + super.init(api: api) + } + + private static func settingsWithApi(api: WordPressComApi) -> Observable { + let settings = Observable.create { observer in let remote = AccountSettingsRemote(api: api) let operation = remote.getSettings( success: { settings in @@ -46,6 +51,9 @@ class AccountSettingsRemote: ServiceRemoteREST { } } } + + return settings + .share() } func getSettings(success success: AccountSettings -> Void, failure: ErrorType -> Void) -> AFHTTPRequestOperation? { From e5b7a41af6f8aba7ae72367d338b4d8e5b052b53 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Thu, 21 Jan 2016 19:28:19 +0100 Subject: [PATCH 11/36] Refactor service to favor stored properties When possible, avoid the overhead (and possible duplication of side effects) of creating observables by storing them instead of using computed properties. Also, add documentation for observable properties. --- .../Services/AccountSettingsService.swift | 65 ++++++++++++------- 1 file changed, 41 insertions(+), 24 deletions(-) diff --git a/WordPress/Classes/Services/AccountSettingsService.swift b/WordPress/Classes/Services/AccountSettingsService.swift index 11efdc507d1e..5b0014fd2d76 100644 --- a/WordPress/Classes/Services/AccountSettingsService.swift +++ b/WordPress/Classes/Services/AccountSettingsService.swift @@ -5,10 +5,12 @@ import RxSwift let AccountSettingsServiceChangeSaveFailedNotification = "AccountSettingsServiceChangeSaveFailed" -struct AccountSettingsService { - let stallTimeout = 4.0 - let maxRetries = 3 - let pollingInterval = 60.0 +class AccountSettingsService { + struct Defaults { + static let stallTimeout = 4.0 + static let maxRetries = 3 + static let pollingInterval = 60.0 + } let remote: AccountSettingsRemote let userID: Int @@ -20,12 +22,18 @@ struct AccountSettingsService { self.userID = userID } - var reachable: Observable { - return Reachability.internetConnection - } + /// Emits a boolean value each time reachability changes for the internet connection. + private let reachable = Reachability.internetConnection - var remoteSettings: Observable { - return remote.settings() + /// Performs a network refresh of settings and emits values with the refresh status. + /// + /// - When it's subscribed, it requests a refresh from the server and immediately emits a `.Refreshing` value. + /// - If a networking error happens it doesn't emit a new value and will retry the request. + /// - If it reaches the maximum permitted number of retries it will emit an Error. + /// - If an error not related to networking happens, it will emit an Error. + /// - When the data is refreshed, it will emit an `.Idle` value and complete. + lazy private var remoteSettings: Observable = { + return self.remote.settings .map({ settings -> RefreshStatus in self.updateSettings(settings) return .Idle @@ -42,48 +50,57 @@ struct AccountSettingsService { return Observable.just(.Failed) } }) - .retry(maxRetries) + .retry(Defaults.maxRetries) .doOn(onError: { (error) -> Void in DDLogSwift.logError("Error refreshing settings (maxRetries reached): \(error)") }) - .share() - } + }() - var stalled: Observable { - return Observable + /// Emits one `.Stalled` value after a timeout and then completes + let stalled = Observable .just(.Stalled) - .delaySubscription(stallTimeout, scheduler: MainScheduler.instance) - } + .delaySubscription(Defaults.stallTimeout, scheduler: MainScheduler.instance) - var request: Observable { + /// Performs a network refresh, emitting a `.Stalled` value if it's taking too long + /// - seealso: remoteSettings + lazy private var request: Observable = { let remoteSettings = self.remoteSettings - let stalledSettings = Observable.of(stalled, remoteSettings) + let stalledSettings = Observable.of(self.stalled, remoteSettings) .merge() return remoteSettings .amb(stalledSettings) .startWith(.Refreshing) - } - - var refresh: Observable { + }() + + /// Emits values when the refresh status changes. + /// + /// On subscription, this will start refreshing settings, polling each minute, while there's an internet connection. + /// Possible values: + /// - `.Refreshing` when it starts getting remote data. + /// - `.Stalled` when it's getting remote data and hasn't succeeded before `stallTimeout`. + /// - `.Failed` when the request couldn't complete. It will retry after the polling interval. + /// - `.Offline` when there is no internet connection. + /// - `.Idle` when the request was successful and it's waiting for the polling interval. + lazy var refresh: Observable = { // Copy request to avoid capture of self in closure let request = self.request // Convert to a polling request let polling = Observable - .interval(pollingInterval, scheduler: MainScheduler.instance) + .interval(Defaults.pollingInterval, scheduler: MainScheduler.instance) .startWith(0) .flatMapLatest({ _ in request }) // Enable only when reachable, otherwise emit .Offline - return reachable.flatMapLatest({ reachable -> Observable in + return self.reachable.flatMapLatest({ reachable -> Observable in if reachable { return polling } else { return Observable.just(.Offline) } }) - } + }() func refreshSettings(completion: (Bool) -> Void) { remote.getSettings( From 74d293786e75fd9ad9416a034bcf945e640daee6 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Fri, 22 Jan 2016 12:42:13 +0100 Subject: [PATCH 12/36] Documentation and comments for settings remote --- .../Networking/AccountSettingsRemote.swift | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/WordPress/Classes/Networking/AccountSettingsRemote.swift b/WordPress/Classes/Networking/AccountSettingsRemote.swift index 01d561eba7d2..9fcb6bd92659 100644 --- a/WordPress/Classes/Networking/AccountSettingsRemote.swift +++ b/WordPress/Classes/Networking/AccountSettingsRemote.swift @@ -5,7 +5,18 @@ import RxSwift class AccountSettingsRemote: ServiceRemoteREST { static let remotes = NSMapTable(keyOptions: .StrongMemory, valueOptions: .WeakMemory) + /// Returns an AccountSettingsRemote with the given api, reusing a previous + /// remote if it exists. static func remoteWithApi(api: WordPressComApi) -> AccountSettingsRemote { + // We're hashing on the authToken because we don't want duplicate api + // objects for the same account. + // + // In theory this would be taken care of by the fact that the api comes + // from a WPAccount, and since WPAccount is a managed object Core Data + // guarantees there's only one of it. + // + // However it might be possible that the account gets deallocated and + // when it's fetched again it would create a different api object. let key = api.authToken.hashValue // FIXME: not thread safe // @koke 2016-01-21 @@ -20,6 +31,8 @@ class AccountSettingsRemote: ServiceRemoteREST { let settings: Observable + /// Creates a new AccountSettingsRemote. It is recommended that you use AccountSettingsRemote.remoteWithApi(_) + /// instead. override init(api: WordPressComApi) { settings = AccountSettingsRemote.settingsWithApi(api) super.init(api: api) @@ -140,4 +153,4 @@ class AccountSettingsRemote: ServiceRemoteREST { enum Error: ErrorType { case DecodeError } -} \ No newline at end of file +} From eba03b9aae52d75b902aa4452d4afd339c38346e Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Fri, 22 Jan 2016 12:42:54 +0100 Subject: [PATCH 13/36] Unit Tests for settings remote --- WordPress/WordPress.xcodeproj/project.pbxproj | 8 ++ .../AccountSettingsServiceRemoteTests.swift | 128 ++++++++++++++++++ .../Test Data/get-me-settings-v1.1.json | 33 +++++ 3 files changed, 169 insertions(+) create mode 100644 WordPress/WordPressTest/AccountSettingsServiceRemoteTests.swift create mode 100644 WordPress/WordPressTest/Test Data/get-me-settings-v1.1.json diff --git a/WordPress/WordPress.xcodeproj/project.pbxproj b/WordPress/WordPress.xcodeproj/project.pbxproj index 79db8f9879d9..612ea333bb4a 100644 --- a/WordPress/WordPress.xcodeproj/project.pbxproj +++ b/WordPress/WordPress.xcodeproj/project.pbxproj @@ -462,6 +462,7 @@ E1266D2D1BBE8B9A00FCB6B6 /* Gravatar.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1266D2C1BBE8B9A00FCB6B6 /* Gravatar.swift */; }; E1266D2F1BBEC37B00FCB6B6 /* GravatarTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1266D2E1BBEC37B00FCB6B6 /* GravatarTest.swift */; }; E127A0F11C43B7CB00085129 /* SiteServiceRemoteREST.m in Sources */ = {isa = PBXBuildFile; fileRef = E127A0F01C43B7CB00085129 /* SiteServiceRemoteREST.m */; }; + E12BE5EE1C5235DB000FD5CA /* get-me-settings-v1.1.json in Resources */ = {isa = PBXBuildFile; fileRef = E12BE5ED1C5235DB000FD5CA /* get-me-settings-v1.1.json */; }; E12DB07B1C48D1C200A6C1D4 /* WPAccount+AccountSettings.swift in Sources */ = {isa = PBXBuildFile; fileRef = E12DB07A1C48D1C200A6C1D4 /* WPAccount+AccountSettings.swift */; }; E12E6E331C21BA170033C5D0 /* FeatureFlag.swift in Sources */ = {isa = PBXBuildFile; fileRef = E12E6E321C21BA170033C5D0 /* FeatureFlag.swift */; }; E12E6E381C21E75F0033C5D0 /* FeatureFlagTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = E12E6E371C21E75F0033C5D0 /* FeatureFlagTest.swift */; }; @@ -471,6 +472,7 @@ E131CB5816CACFB4004B0314 /* get-user-blogs_doesnt-have-blog.json in Resources */ = {isa = PBXBuildFile; fileRef = E131CB5716CACFB4004B0314 /* get-user-blogs_doesnt-have-blog.json */; }; E131F5351C2930FC00D2D975 /* String+Helpers.swift in Sources */ = {isa = PBXBuildFile; fileRef = E131F5341C2930FC00D2D975 /* String+Helpers.swift */; }; E13A8C9B1C3E6EF2005BB1C1 /* ImmuTable+WordPress.swift in Sources */ = {isa = PBXBuildFile; fileRef = E13A8C9A1C3E6EF2005BB1C1 /* ImmuTable+WordPress.swift */; }; + E13BF2CA1C522A1300275BE9 /* AccountSettingsServiceRemoteTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E13BF2C91C522A1300275BE9 /* AccountSettingsServiceRemoteTests.swift */; }; E13EB7A5157D230000885780 /* WordPressComApi.m in Sources */ = {isa = PBXBuildFile; fileRef = E13EB7A4157D230000885780 /* WordPressComApi.m */; }; E13F23C314FE84600081D9CC /* NSMutableDictionary+Helpers.m in Sources */ = {isa = PBXBuildFile; fileRef = E13F23C214FE84600081D9CC /* NSMutableDictionary+Helpers.m */; }; E14200781C117A2E00B3B115 /* ManagedAccountSettings.swift in Sources */ = {isa = PBXBuildFile; fileRef = E14200771C117A2E00B3B115 /* ManagedAccountSettings.swift */; }; @@ -1483,6 +1485,7 @@ E127A0EF1C43B7CB00085129 /* SiteServiceRemoteREST.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SiteServiceRemoteREST.h; sourceTree = ""; }; E127A0F01C43B7CB00085129 /* SiteServiceRemoteREST.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = SiteServiceRemoteREST.m; sourceTree = ""; }; E12963A8174654B2002E7744 /* ru */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = ru; path = ru.lproj/Localizable.strings; sourceTree = ""; }; + E12BE5ED1C5235DB000FD5CA /* get-me-settings-v1.1.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = "get-me-settings-v1.1.json"; sourceTree = ""; }; E12DB07A1C48D1C200A6C1D4 /* WPAccount+AccountSettings.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "WPAccount+AccountSettings.swift"; sourceTree = ""; }; E12E6E321C21BA170033C5D0 /* FeatureFlag.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = FeatureFlag.swift; sourceTree = ""; }; E12E6E371C21E75F0033C5D0 /* FeatureFlagTest.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = FeatureFlagTest.swift; sourceTree = ""; }; @@ -1496,6 +1499,7 @@ E131F5341C2930FC00D2D975 /* String+Helpers.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "String+Helpers.swift"; sourceTree = ""; }; E133DB40137AE180003C0AF9 /* he */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = he; path = he.lproj/Localizable.strings; sourceTree = ""; }; E13A8C9A1C3E6EF2005BB1C1 /* ImmuTable+WordPress.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "ImmuTable+WordPress.swift"; sourceTree = ""; }; + E13BF2C91C522A1300275BE9 /* AccountSettingsServiceRemoteTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = AccountSettingsServiceRemoteTests.swift; sourceTree = ""; }; E13EB7A3157D230000885780 /* WordPressComApi.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WordPressComApi.h; sourceTree = ""; }; E13EB7A4157D230000885780 /* WordPressComApi.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = WordPressComApi.m; sourceTree = ""; }; E13F23C114FE84600081D9CC /* NSMutableDictionary+Helpers.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = "NSMutableDictionary+Helpers.h"; sourceTree = ""; }; @@ -2203,6 +2207,7 @@ isa = PBXGroup; children = ( 591CFB051B28A960009E61B3 /* AccountServiceRemoteRESTTests.m */, + E13BF2C91C522A1300275BE9 /* AccountSettingsServiceRemoteTests.swift */, 591CFB081B28AC8C009E61B3 /* BlogServiceRemoteRESTTests.m */, 59E2AAEB1B20E5CE0051DC06 /* PostServiceRemoteRESTTests.m */, 59E2AAE71B20E3EA0051DC06 /* ServiceRemoteRESTTests.m */, @@ -3655,6 +3660,7 @@ B5AEEC781ACACFDA008BF2A4 /* notifications-replied-comment.json */, B5EFB1D01B33630C007608A3 /* notifications-settings.json */, 93CD939219099BE70049096E /* authtoken.json */, + E12BE5ED1C5235DB000FD5CA /* get-me-settings-v1.1.json */, FAFB84051BBF3638000BBA8E /* get-multiple-themes-v1.2.json */, 59F9C1581B9DD3D600885CC1 /* get-purchased-themes-v1.1.json */, 59F9C1561B9DCE4E00885CC1 /* get-single-theme-v1.1.json */, @@ -4091,6 +4097,7 @@ buildActionMask = 2147483647; files = ( E1EBC3751C118EDE00F638E0 /* ImmuTableTestViewCellWithNib.xib in Resources */, + E12BE5EE1C5235DB000FD5CA /* get-me-settings-v1.1.json in Resources */, B5A6BB8C1BF4DF38002F6A96 /* rest-site-settings.json in Resources */, E16AB93414D978240047A2E5 /* InfoPlist.strings in Resources */, 93594BD5191D2F5A0079E6B2 /* stats-batch.json in Resources */, @@ -4905,6 +4912,7 @@ B5D689FD1A5EBC900063D9E5 /* NotificationsManager+TestHelper.m in Sources */, BEA0E4851BD83565000AEE81 /* WP3DTouchShortcutCreatorTests.swift in Sources */, 85F8E19B1B017AA6000859BB /* PushAuthenticationServiceRemoteTests.swift in Sources */, + E13BF2CA1C522A1300275BE9 /* AccountSettingsServiceRemoteTests.swift in Sources */, 59FBD5621B5684F300734466 /* ThemeServiceTests.m in Sources */, 85F8E19D1B018698000859BB /* PushAuthenticationServiceTests.swift in Sources */, 931D270019EDAE8600114F17 /* CoreDataMigrationTests.m in Sources */, diff --git a/WordPress/WordPressTest/AccountSettingsServiceRemoteTests.swift b/WordPress/WordPressTest/AccountSettingsServiceRemoteTests.swift new file mode 100644 index 000000000000..27d3d4d032fa --- /dev/null +++ b/WordPress/WordPressTest/AccountSettingsServiceRemoteTests.swift @@ -0,0 +1,128 @@ +import XCTest +import Nimble +import OHHTTPStubs +import RxSwift +@testable import WordPress + +class AccountSettingsServiceRemoteTests: XCTestCase { + + override func setUp() { + super.setUp() + } + + override func tearDown() { + // It should be already empty if we did memory management right + // But let's be safe + AccountSettingsRemote.remotes.removeAllObjects() + OHHTTPStubs.removeAllStubs() + + super.tearDown() + } + + func testRemoteWithApiDoesntDuplicateRemotes() { + let api = WordPressComApi(OAuthToken: "authtoken1") + let remote1 = AccountSettingsRemote.remoteWithApi(api) + let remote2 = AccountSettingsRemote.remoteWithApi(api) + expect(remote1).to(beIdenticalTo(remote2)) + expect(remote1.settings).to(beIdenticalTo(remote2.settings)) + + let duplicatedApi = WordPressComApi(OAuthToken: "authtoken1") + let remote3 = AccountSettingsRemote.remoteWithApi(duplicatedApi) + expect(remote1).to(beIdenticalTo(remote3)) + } + + func testSettingsSuccessful() { + stub(isGetSettings()) { request in + let stubPath = OHPathForFile("get-me-settings-v1.1.json", self.dynamicType) + return fixture(stubPath!, headers: ["Content-Type": "application/json"]) + } + + let events = subscribeToSettingsAndWait() + + expect(events.count).to(equal(2)) + expect(events[0].element).toNot(beNil()) + expect(events[1].isCompleted).to(beTrue()) + guard let settings = events[0].element else { + XCTFail("First emitted value should be settings") + return + } + expect(settings.firstName).to(equal("Jorge")) + expect(settings.lastName).to(equal("Bernal")) + expect(settings.displayName).to(equal("Jorge Bernal")) + expect(settings.aboutMe).to(equal("A description of me")) + } + + func testSettingsFail() { + stub(isGetSettings()) { request in + let error = NSError(domain: NSURLErrorDomain, code: NSURLErrorTimedOut, userInfo: nil) + return OHHTTPStubsResponse(error: error) + } + + let events = subscribeToSettingsAndWait() + + expect(events.count).to(equal(1)) + expect(events[0].isError).to(beTrue()) + } + + // MARK: - Helpers + + func settingsObservable() -> Observable { + let api = WordPressComApi(OAuthToken: "authtoken") + let remote = AccountSettingsRemote(api: api) + return remote.settings + } + + func subscribeToSettingsAndWait() -> [Event] { + var events = [Event]() + let expectation = expectationWithDescription("settings completed or errored") + let subscription = settingsObservable().subscribe { (event) -> Void in + events.append(event) + + switch event { + case .Next(_): + break + case .Completed, .Error(_): + expectation.fulfill() + } + } + defer { + subscription.dispose() + } + waitForExpectationsWithTimeout(5, handler: nil) + return events + } + + func isGetSettings() -> OHHTTPStubsTestBlock { + return isMethodGET() && isMeSettingsEndpoint() + } + + func isUpdateSettings() -> OHHTTPStubsTestBlock { + return isMethodPOST() && isMeSettingsEndpoint() + } + + func isMeSettingsEndpoint() -> OHHTTPStubsTestBlock { + return { request in + return request.URL?.path?.hasSuffix("me/settings") ?? false + } + } + + +} + +extension Event { + private var isCompleted: Bool { + if case .Completed = self { + return true + } else { + return false + } + } + + private var isError: Bool { + if case .Error = self { + return true + } else { + return false + } + } +} diff --git a/WordPress/WordPressTest/Test Data/get-me-settings-v1.1.json b/WordPress/WordPressTest/Test Data/get-me-settings-v1.1.json new file mode 100644 index 000000000000..fb4137598ad4 --- /dev/null +++ b/WordPress/WordPressTest/Test Data/get-me-settings-v1.1.json @@ -0,0 +1,33 @@ +{ + "enable_translator": true, + "surprise_me": true, + "post_post_flag": true, + "holidaysnow": true, + "user_login": "koketest", + "password": "", + "display_name": "Jorge Bernal", + "first_name": "Jorge", + "last_name": "Bernal", + "description": "A description of me", + "user_email": "koke@example.com", + "user_email_change_pending": false, + "new_user_email": "", + "user_URL": "http:\/\/koke.me", + "language": "es", + "avatar_URL": "https:\/\/2.gravatar.com\/avatar\/e0ebc5cd3f08c8f7cf3e4a0e703fedee?s=200&d=mm", + "primary_site_ID": 16764956, + "comment_like_notification": true, + "mentions_notification": true, + "subscription_delivery_email_default": "never", + "subscription_delivery_jabber_default": false, + "subscription_delivery_mail_option": "html", + "subscription_delivery_day": 1, + "subscription_delivery_hour": 6, + "subscription_delivery_email_blocked": false, + "two_step_enabled": false, + "two_step_sms_enabled": false, + "two_step_backup_codes_printed": false, + "two_step_sms_country": "ES", + "two_step_sms_phone_number": "600123456", + "user_login_can_be_changed": true, +} \ No newline at end of file From bbaf0e9ef114e9bed706ac293b000b4776df8b24 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Fri, 22 Jan 2016 15:57:53 +0100 Subject: [PATCH 14/36] Remove unused method --- .../Classes/Services/AccountSettingsService.swift | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/WordPress/Classes/Services/AccountSettingsService.swift b/WordPress/Classes/Services/AccountSettingsService.swift index 5b0014fd2d76..1cda727e984e 100644 --- a/WordPress/Classes/Services/AccountSettingsService.swift +++ b/WordPress/Classes/Services/AccountSettingsService.swift @@ -102,21 +102,6 @@ class AccountSettingsService { }) }() - func refreshSettings(completion: (Bool) -> Void) { - remote.getSettings( - success: { - (settings) -> Void in - - self.updateSettings(settings) - completion(true) - }, failure: { - (error) -> Void in - - DDLogSwift.logError(String(error)) - completion(false) - }) - } - func saveChange(change: AccountSettingsChange) { guard let reverse = try? applyChange(change) else { return From e81a3412adc186a342267638800f539fa3852a39 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Fri, 22 Jan 2016 18:43:04 +0100 Subject: [PATCH 15/36] Unit tests for AccountSettingsService --- Podfile | 1 + Podfile.lock | 4 + .../Services/AccountSettingsService.swift | 21 ++- WordPress/WordPress.xcodeproj/project.pbxproj | 4 + .../AccountSettingsServiceTests.swift | 150 ++++++++++++++++++ 5 files changed, 176 insertions(+), 4 deletions(-) create mode 100644 WordPress/WordPressTest/AccountSettingsServiceTests.swift diff --git a/Podfile b/Podfile index 024fa03f5a71..71be6daffd65 100644 --- a/Podfile +++ b/Podfile @@ -56,6 +56,7 @@ target :WordPressTest, :exclusive => true do pod 'Expecta', '0.3.2' pod 'Nimble', '~> 3.0.0' pod 'RxSwift', '~> 2.1.0' + pod 'RxTests', '~> 2.1.0' end target 'UITests', :exclusive => true do diff --git a/Podfile.lock b/Podfile.lock index 79e91ac927a5..73dd2c1e79c2 100644 --- a/Podfile.lock +++ b/Podfile.lock @@ -138,6 +138,8 @@ PODS: - RxCocoa (2.1.0): - RxSwift (~> 2.0) - RxSwift (2.1.0) + - RxTests (2.1.0): + - RxSwift (~> 2.0) - Simperium (0.8.10): - Simperium/DiffMatchPach (= 0.8.10) - Simperium/JRSwizzle (= 0.8.10) @@ -210,6 +212,7 @@ DEPENDENCIES: - ReactiveCocoa (~> 2.4.7) - RxCocoa (~> 2.1.0) - RxSwift (~> 2.1.0) + - RxTests (~> 2.1.0) - Simperium (= 0.8.10) - Specta (= 1.0.5) - SVProgressHUD (~> 1.1.3) @@ -282,6 +285,7 @@ SPEC CHECKSUMS: ReactiveCocoa: eb38dee0a0e698f73a9b25e5c1faea2bb4c79240 RxCocoa: 79b5feb8378545336e756a0a33fcf5e95050b71c RxSwift: 110fb07f81c17c2c3b3254d168363057b1880d18 + RxTests: 94c67ffc37c36bd8c7aec90a84601a3db142be94 Simperium: f507d9b400c499048a98fe728a0b2b9956fd14c1 Specta: ac94d110b865115fe60ff2c6d7281053c6f8e8a2 SVProgressHUD: 748080e4f36e603f6c02aec292664239df5279c1 diff --git a/WordPress/Classes/Services/AccountSettingsService.swift b/WordPress/Classes/Services/AccountSettingsService.swift index 1cda727e984e..6f0869b15799 100644 --- a/WordPress/Classes/Services/AccountSettingsService.swift +++ b/WordPress/Classes/Services/AccountSettingsService.swift @@ -5,6 +5,13 @@ import RxSwift let AccountSettingsServiceChangeSaveFailedNotification = "AccountSettingsServiceChangeSaveFailed" +protocol AccountSettingsRemoteInterface { + var settings: Observable { get } + func updateSetting(change: AccountSettingsChange, success: () -> Void, failure: ErrorType -> Void) +} + +extension AccountSettingsRemote: AccountSettingsRemoteInterface {} + class AccountSettingsService { struct Defaults { static let stallTimeout = 4.0 @@ -12,7 +19,7 @@ class AccountSettingsService { static let pollingInterval = 60.0 } - let remote: AccountSettingsRemote + let remote: AccountSettingsRemoteInterface let userID: Int private let context = ContextManager.sharedInstance().mainContext @@ -22,17 +29,22 @@ class AccountSettingsService { self.userID = userID } + init(userID: Int, remote: AccountSettingsRemoteInterface) { + self.userID = userID + self.remote = remote + } + /// Emits a boolean value each time reachability changes for the internet connection. private let reachable = Reachability.internetConnection /// Performs a network refresh of settings and emits values with the refresh status. /// - /// - When it's subscribed, it requests a refresh from the server and immediately emits a `.Refreshing` value. + /// - When it's subscribed, it requests a refresh from the server /// - If a networking error happens it doesn't emit a new value and will retry the request. /// - If it reaches the maximum permitted number of retries it will emit an Error. /// - If an error not related to networking happens, it will emit an Error. /// - When the data is refreshed, it will emit an `.Idle` value and complete. - lazy private var remoteSettings: Observable = { + lazy var remoteSettings: Observable = { return self.remote.settings .map({ settings -> RefreshStatus in self.updateSettings(settings) @@ -54,6 +66,7 @@ class AccountSettingsService { .doOn(onError: { (error) -> Void in DDLogSwift.logError("Error refreshing settings (maxRetries reached): \(error)") }) + .catchErrorJustReturn(.Failed) }() /// Emits one `.Stalled` value after a timeout and then completes @@ -61,7 +74,7 @@ class AccountSettingsService { .just(.Stalled) .delaySubscription(Defaults.stallTimeout, scheduler: MainScheduler.instance) - /// Performs a network refresh, emitting a `.Stalled` value if it's taking too long + /// Performs a network refresh, emitting a `.Stalled` value if it's taking too long. It initially emits a `.Refreshing` value. /// - seealso: remoteSettings lazy private var request: Observable = { let remoteSettings = self.remoteSettings diff --git a/WordPress/WordPress.xcodeproj/project.pbxproj b/WordPress/WordPress.xcodeproj/project.pbxproj index 612ea333bb4a..398304c085a6 100644 --- a/WordPress/WordPress.xcodeproj/project.pbxproj +++ b/WordPress/WordPress.xcodeproj/project.pbxproj @@ -463,6 +463,7 @@ E1266D2F1BBEC37B00FCB6B6 /* GravatarTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1266D2E1BBEC37B00FCB6B6 /* GravatarTest.swift */; }; E127A0F11C43B7CB00085129 /* SiteServiceRemoteREST.m in Sources */ = {isa = PBXBuildFile; fileRef = E127A0F01C43B7CB00085129 /* SiteServiceRemoteREST.m */; }; E12BE5EE1C5235DB000FD5CA /* get-me-settings-v1.1.json in Resources */ = {isa = PBXBuildFile; fileRef = E12BE5ED1C5235DB000FD5CA /* get-me-settings-v1.1.json */; }; + E12BE5F01C524FC9000FD5CA /* AccountSettingsServiceTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E12BE5EF1C524FC9000FD5CA /* AccountSettingsServiceTests.swift */; }; E12DB07B1C48D1C200A6C1D4 /* WPAccount+AccountSettings.swift in Sources */ = {isa = PBXBuildFile; fileRef = E12DB07A1C48D1C200A6C1D4 /* WPAccount+AccountSettings.swift */; }; E12E6E331C21BA170033C5D0 /* FeatureFlag.swift in Sources */ = {isa = PBXBuildFile; fileRef = E12E6E321C21BA170033C5D0 /* FeatureFlag.swift */; }; E12E6E381C21E75F0033C5D0 /* FeatureFlagTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = E12E6E371C21E75F0033C5D0 /* FeatureFlagTest.swift */; }; @@ -1486,6 +1487,7 @@ E127A0F01C43B7CB00085129 /* SiteServiceRemoteREST.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = SiteServiceRemoteREST.m; sourceTree = ""; }; E12963A8174654B2002E7744 /* ru */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = ru; path = ru.lproj/Localizable.strings; sourceTree = ""; }; E12BE5ED1C5235DB000FD5CA /* get-me-settings-v1.1.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = "get-me-settings-v1.1.json"; sourceTree = ""; }; + E12BE5EF1C524FC9000FD5CA /* AccountSettingsServiceTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = AccountSettingsServiceTests.swift; sourceTree = ""; }; E12DB07A1C48D1C200A6C1D4 /* WPAccount+AccountSettings.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "WPAccount+AccountSettings.swift"; sourceTree = ""; }; E12E6E321C21BA170033C5D0 /* FeatureFlag.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = FeatureFlag.swift; sourceTree = ""; }; E12E6E371C21E75F0033C5D0 /* FeatureFlagTest.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = FeatureFlagTest.swift; sourceTree = ""; }; @@ -3163,6 +3165,7 @@ 5DE8A0401912D95B00B2FF59 /* ReaderPostServiceTest.m */, E66969C71B9E0A6800EC9C00 /* ReaderTopicServiceTest.swift */, 59FBD5611B5684F300734466 /* ThemeServiceTests.m */, + E12BE5EF1C524FC9000FD5CA /* AccountSettingsServiceTests.swift */, ); name = Services; sourceTree = ""; @@ -4878,6 +4881,7 @@ 59E2AAE81B20E3EA0051DC06 /* ServiceRemoteRESTTests.m in Sources */, E61084C41B9DC09C008050C5 /* ReaderPostServiceRemoteTests.m in Sources */, BEC8A3FF1B4BAA2C001CB8C3 /* BlogListViewControllerTests.m in Sources */, + E12BE5F01C524FC9000FD5CA /* AccountSettingsServiceTests.swift in Sources */, E6B9B8AD1B94EACA0001B92F /* ReaderHelperTests.swift in Sources */, E66969CD1B9E2EBF00EC9C00 /* SafeReaderTopicToReaderTopic.m in Sources */, E66969C81B9E0A6800EC9C00 /* ReaderTopicServiceTest.swift in Sources */, diff --git a/WordPress/WordPressTest/AccountSettingsServiceTests.swift b/WordPress/WordPressTest/AccountSettingsServiceTests.swift new file mode 100644 index 000000000000..90fb96f9c052 --- /dev/null +++ b/WordPress/WordPressTest/AccountSettingsServiceTests.swift @@ -0,0 +1,150 @@ +import XCTest +import RxSwift +import RxTests +@testable import WordPress + +class AccountSettingsServiceTests: XCTestCase { + struct TestData { + static let sampleSettings = AccountSettings( + firstName: "Jorge", + lastName: "Bernal", + displayName: "Jorge Bernal", + aboutMe: "A description about me", + username: "koketest", + email: "koke@example.com", + primarySiteID: 16764956, + webAddress: "http://koke.me", + language: "es" + ) + } + + override func setUp() { + super.setUp() + // Put setup code here. This method is called before the invocation of each test method in the class. + } + + override func tearDown() { + // Put teardown code here. This method is called after the invocation of each test method in the class. + super.tearDown() + } + + func testRemoteSettingsSuccessful() { + let scheduler = TestScheduler(initialClock: 0) + let mockRemote = MockAccountSettingsRemote() + var requestCount = 0 + mockRemote.settings = Observable.create { observer in + requestCount += 1 + return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in + observer.on(.Next(TestData.sampleSettings)) + observer.on(.Completed) + return NopDisposable.instance + }) + } + + let service = AccountSettingsService(userID: 123, remote: mockRemote) + + let res = scheduler.start { + service.remoteSettings + } + + XCTAssertEqual(requestCount, 1) + XCTAssertEqual(res.events, [ + next(210, .Idle), + completed(210) + ]) + } + + func testRemoteSettingsOneNetworkErrorShouldRetry() { + let scheduler = TestScheduler(initialClock: 0) + let mockRemote = MockAccountSettingsRemote() + var requestCount = 0 + mockRemote.settings = Observable.create { observer in + requestCount += 1 + return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in + if requestCount == 1 { + let error = NSError(domain: NSURLErrorDomain, code: NSURLErrorNetworkConnectionLost, userInfo: nil) + observer.on(.Error(error)) + } else { + observer.on(.Next(TestData.sampleSettings)) + observer.on(.Completed) + } + return NopDisposable.instance + }) + } + + let service = AccountSettingsService(userID: 123, remote: mockRemote) + + let res = scheduler.start { + service.remoteSettings + } + + XCTAssertEqual(requestCount, 2) + XCTAssertEqual(res.events, [ + next(220, .Idle), + completed(220) + ]) + } + + func testRemoteSettingsFourNetworkErrorsShouldFail() { + let scheduler = TestScheduler(initialClock: 0) + let mockRemote = MockAccountSettingsRemote() + var requestCount = 0 + let connectionLost = NSError(domain: NSURLErrorDomain, code: NSURLErrorNetworkConnectionLost, userInfo: nil) + mockRemote.settings = Observable.create { observer in + requestCount += 1 + return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in + observer.on(.Error(connectionLost)) + return NopDisposable.instance + }) + } + + let service = AccountSettingsService(userID: 123, remote: mockRemote) + + let res = scheduler.start { + service.remoteSettings + } + + XCTAssertEqual(requestCount, 3) + XCTAssertEqual(res.events, [ + next(230, .Failed), + completed(230) + ]) + } + + func testRemoteSettingsUnrecoverableErrorsShouldFailImmediately() { + let scheduler = TestScheduler(initialClock: 0) + let mockRemote = MockAccountSettingsRemote() + var requestCount = 0 + let unexpected = NSError(domain: "Unexpected", code: -999, userInfo: nil) + mockRemote.settings = Observable.create { observer in + requestCount += 1 + return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in + observer.on(.Error(unexpected)) + return NopDisposable.instance + }) + } + + let service = AccountSettingsService(userID: 123, remote: mockRemote) + + let res = scheduler.start { + service.remoteSettings + } + + XCTAssertEqual(requestCount, 1) + XCTAssertEqual(res.events, [ + next(210, .Failed), + completed(210) + ]) + } + +} + +class MockAccountSettingsRemote: AccountSettingsRemoteInterface { + var settings: Observable = Observable.never() + + var mockUpdateSetting: (AccountSettingsChange, () -> Void, ErrorType -> Void) -> Void = { _, _, _ in } + + func updateSetting(change: AccountSettingsChange, success: () -> Void, failure: ErrorType -> Void) { + mockUpdateSetting(change, success, failure) + } +} From 391d36dc0412d33ec200e8081f6e70a52ee0ebac Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 25 Jan 2016 08:09:03 +0100 Subject: [PATCH 16/36] Rename forwardIf as pausable Also add locking as suggested in https://github.com/ReactiveX/RxSwift/pull/422 --- WordPress/Classes/Extensions/Rx.swift | 45 +++++++++---------- .../Me/MyProfileViewController.swift | 2 +- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/WordPress/Classes/Extensions/Rx.swift b/WordPress/Classes/Extensions/Rx.swift index b5b9e9a6ab67..31b92c9e35d5 100644 --- a/WordPress/Classes/Extensions/Rx.swift +++ b/WordPress/Classes/Extensions/Rx.swift @@ -1,40 +1,35 @@ import RxSwift -// MARK: - forwardIf -// This has been proposed for inclusion in RxSwift -// https://github.com/ReactiveX/RxSwift/pull/422 -// I can't copy the exact implementation here since it relies on internal classes -// I added an alternative implementation instead -// @koke 2016-01-21 +// MARK: - pausable extension ObservableType { /** - Propagates the source observable sequence while the condition observable sequence last value is true. + Pauses the underlying observable sequence based upon the observable sequence which yields true/false. - - parameter source: Source observable sequence to propagate - - parameter condition: Boolean observable sequence that dictates if the source propagates. + - parameter pauser: The observable sequence used to pause the underlying sequence. - returns: An observable sequence that subscribes and emits the values of the source observable as long as the last emitted value of the condition observable is true. */ - public func forwardIf(condition: ConditionO) -> Observable { - return ForwardIf(source: self, condition: condition.asObservable()).asObservable() + public func pausable(pauser: ConditionO) -> Observable { + return Pausable(source: self, pauser: pauser.asObservable()).asObservable() } } -class ForwardIf: ObservableType { +class Pausable: ObservableType { typealias E = S.E typealias DisposeKey = CompositeDisposable.DisposeKey + private let _lock = NSRecursiveLock() + private let _source: S - private let _condition: Observable - private let _controller = PublishSubject() + private let _pauser: Observable private let _group = CompositeDisposable() private var _connectionKey: DisposeKey? = nil - init(source: S, condition: Observable) { + init(source: S, pauser: Observable) { _source = source - _condition = condition + _pauser = pauser } func subscribe(observer: O) -> Disposable { @@ -42,17 +37,19 @@ class ForwardIf: ObservableType { let connection = conn.subscribe(observer) _group.addDisposable(connection) - let subscription = _condition + let subscription = _pauser .distinctUntilChanged() .subscribeNext { active in - if active { - self._connectionKey = self._group.addDisposable(conn.connect()) - } else { - if let connectionKey = self._connectionKey { - self._group.removeDisposable(connectionKey) - self._connectionKey = nil + self._lock.lock(); defer { self._lock.unlock() } // lock { + if active { + self._connectionKey = self._group.addDisposable(conn.connect()) + } else { + if let connectionKey = self._connectionKey { + self._group.removeDisposable(connectionKey) + self._connectionKey = nil + } } - } + // } } _group.addDisposable(subscription) diff --git a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift index 1d08926ae15f..49f00338ddcb 100644 --- a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift +++ b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift @@ -22,7 +22,7 @@ class MyProfileController: NSObject { .addDisposableTo(bag) service.refresh - .forwardIf(viewController.visible) + .pausable(viewController.visible) // replace errors with .Failed status .catchErrorJustReturn(.Failed) // convert status to string From 5f5a7af3b10e5adb9538cdb940a3db08b933b102 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 25 Jan 2016 09:04:17 +0100 Subject: [PATCH 17/36] Make remoteSettings emit .Refreshing initially --- WordPress/Classes/Services/AccountSettingsService.swift | 2 +- WordPress/WordPressTest/AccountSettingsServiceTests.swift | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/WordPress/Classes/Services/AccountSettingsService.swift b/WordPress/Classes/Services/AccountSettingsService.swift index 6f0869b15799..a9e8c0aeee85 100644 --- a/WordPress/Classes/Services/AccountSettingsService.swift +++ b/WordPress/Classes/Services/AccountSettingsService.swift @@ -63,6 +63,7 @@ class AccountSettingsService { } }) .retry(Defaults.maxRetries) + .startWith(.Refreshing) .doOn(onError: { (error) -> Void in DDLogSwift.logError("Error refreshing settings (maxRetries reached): \(error)") }) @@ -83,7 +84,6 @@ class AccountSettingsService { return remoteSettings .amb(stalledSettings) - .startWith(.Refreshing) }() /// Emits values when the refresh status changes. diff --git a/WordPress/WordPressTest/AccountSettingsServiceTests.swift b/WordPress/WordPressTest/AccountSettingsServiceTests.swift index 90fb96f9c052..34e723290efa 100644 --- a/WordPress/WordPressTest/AccountSettingsServiceTests.swift +++ b/WordPress/WordPressTest/AccountSettingsServiceTests.swift @@ -49,6 +49,7 @@ class AccountSettingsServiceTests: XCTestCase { XCTAssertEqual(requestCount, 1) XCTAssertEqual(res.events, [ + next(200, .Refreshing), next(210, .Idle), completed(210) ]) @@ -80,6 +81,7 @@ class AccountSettingsServiceTests: XCTestCase { XCTAssertEqual(requestCount, 2) XCTAssertEqual(res.events, [ + next(200, .Refreshing), next(220, .Idle), completed(220) ]) @@ -106,6 +108,7 @@ class AccountSettingsServiceTests: XCTestCase { XCTAssertEqual(requestCount, 3) XCTAssertEqual(res.events, [ + next(200, .Refreshing), next(230, .Failed), completed(230) ]) @@ -132,6 +135,7 @@ class AccountSettingsServiceTests: XCTestCase { XCTAssertEqual(requestCount, 1) XCTAssertEqual(res.events, [ + next(200, .Refreshing), next(210, .Failed), completed(210) ]) From c408f950d290fb47473df80c8afafa0e07457ff1 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 25 Jan 2016 09:24:11 +0100 Subject: [PATCH 18/36] Unit Tests for Rx pausable operator --- WordPress/WordPress.xcodeproj/project.pbxproj | 4 + .../WordPressTest/Extensions/RxTests.swift | 136 ++++++++++++++++++ 2 files changed, 140 insertions(+) create mode 100644 WordPress/WordPressTest/Extensions/RxTests.swift diff --git a/WordPress/WordPress.xcodeproj/project.pbxproj b/WordPress/WordPress.xcodeproj/project.pbxproj index 398304c085a6..ad2cc5a41d21 100644 --- a/WordPress/WordPress.xcodeproj/project.pbxproj +++ b/WordPress/WordPress.xcodeproj/project.pbxproj @@ -523,6 +523,7 @@ E19DF741141F7BDD000002F3 /* libz.dylib in Frameworks */ = {isa = PBXBuildFile; fileRef = E19DF740141F7BDD000002F3 /* libz.dylib */; }; E1A03EE217422DCF0085D192 /* BlogToAccount.m in Sources */ = {isa = PBXBuildFile; fileRef = E1A03EE117422DCE0085D192 /* BlogToAccount.m */; }; E1A03F48174283E10085D192 /* BlogToJetpackAccount.m in Sources */ = {isa = PBXBuildFile; fileRef = E1A03F47174283E00085D192 /* BlogToJetpackAccount.m */; }; + E1A0AC821C560F3A00070E2B /* RxTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E1A0AC811C560F3A00070E2B /* RxTests.swift */; }; E1A0FAE7162F11CF0063B098 /* UIDevice+Helpers.m in Sources */ = {isa = PBXBuildFile; fileRef = E1A0FAE6162F11CE0063B098 /* UIDevice+Helpers.m */; }; E1A386C814DB05C300954CF8 /* AVFoundation.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = E1A386C714DB05C300954CF8 /* AVFoundation.framework */; }; E1A386CA14DB05F700954CF8 /* CoreMedia.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = E1A386C914DB05F700954CF8 /* CoreMedia.framework */; }; @@ -1578,6 +1579,7 @@ E1A03EE117422DCE0085D192 /* BlogToAccount.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = BlogToAccount.m; sourceTree = ""; }; E1A03F46174283DF0085D192 /* BlogToJetpackAccount.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = BlogToJetpackAccount.h; sourceTree = ""; }; E1A03F47174283E00085D192 /* BlogToJetpackAccount.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = BlogToJetpackAccount.m; sourceTree = ""; }; + E1A0AC811C560F3A00070E2B /* RxTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = RxTests.swift; sourceTree = ""; }; E1A0FAE5162F11CE0063B098 /* UIDevice+Helpers.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; lineEnding = 0; path = "UIDevice+Helpers.h"; sourceTree = ""; xcLanguageSpecificationIdentifier = xcode.lang.objcpp; }; E1A0FAE6162F11CE0063B098 /* UIDevice+Helpers.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; lineEnding = 0; path = "UIDevice+Helpers.m"; sourceTree = ""; xcLanguageSpecificationIdentifier = xcode.lang.objc; }; E1A386C714DB05C300954CF8 /* AVFoundation.framework */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = wrapper.framework; name = AVFoundation.framework; path = System/Library/Frameworks/AVFoundation.framework; sourceTree = SDKROOT; }; @@ -3739,6 +3741,7 @@ isa = PBXGroup; children = ( E1C9AA551C10427100732665 /* MathTest.swift */, + E1A0AC811C560F3A00070E2B /* RxTests.swift */, ); path = Extensions; sourceTree = ""; @@ -4923,6 +4926,7 @@ E6B9B8AA1B94E1FE0001B92F /* ReaderPostTest.m in Sources */, 85D239C11AE5A7020074768D /* LoginViewModelTests.m in Sources */, 85D790AC1AE5D95E0033AE83 /* MixpanelProxyTests.m in Sources */, + E1A0AC821C560F3A00070E2B /* RxTests.swift in Sources */, 85F8E19F1B0186D0000859BB /* MockWordPressComApi.swift in Sources */, 931D26F619ED7F7000114F17 /* BlogServiceTest.m in Sources */, 852416D21A12ED690030700C /* AppRatingUtilityTests.m in Sources */, diff --git a/WordPress/WordPressTest/Extensions/RxTests.swift b/WordPress/WordPressTest/Extensions/RxTests.swift new file mode 100644 index 000000000000..c51c28024987 --- /dev/null +++ b/WordPress/WordPressTest/Extensions/RxTests.swift @@ -0,0 +1,136 @@ +import XCTest +import RxSwift +import RxTests +@testable import WordPress + +class RxTests: XCTestCase { + + override func setUp() { + super.setUp() + // Put setup code here. This method is called before the invocation of each test method in the class. + } + + override func tearDown() { + // Put teardown code here. This method is called after the invocation of each test method in the class. + super.tearDown() + } + + func testPausable_simple1() { + let scheduler = TestScheduler(initialClock: 0) + + let xs = scheduler.createHotObservable([ + next(90, 1), + next(180, 2), + next(250, 3), + next(260, 4), + next(310, 5), + next(360, 6), + completed(390) + ]) + + let ys = scheduler.createHotObservable([ + next(210, true), + next(300, false), + next(350, true), + completed(400) + ]) + + let res = scheduler.start { + xs.pausable(ys) + } + + XCTAssertEqual(res.events, [ + next(250, 3), + next(260, 4), + next(360, 6), + completed(390) + ]) + + XCTAssertEqual(xs.subscriptions, [ + Subscription(210, 300), + Subscription(350, 390) + ]) + + XCTAssertEqual(ys.subscriptions, [ + Subscription(200, 390) + ]) + + } + + func testPausable_PauserCompleteContinuesEmittingIfLastValueTrue() { + let scheduler = TestScheduler(initialClock: 0) + + let xs = scheduler.createHotObservable([ + next(90, 1), + next(180, 2), + next(250, 3), + next(260, 4), + next(310, 5), + next(360, 6), + completed(390) + ]) + + let ys = scheduler.createHotObservable([ + next(290, true), + completed(300) + ]) + + let res = scheduler.start { + xs.pausable(ys) + } + + XCTAssertEqual(res.events, [ + next(310, 5), + next(360, 6), + completed(300) + ]) + + XCTAssertEqual(xs.subscriptions, [ + Subscription(290, 390) + ]) + + XCTAssertEqual(ys.subscriptions, [ + Subscription(200, 300) + ]) + + } + + func testPausable_PauserCompleteDoesntEmitIfLastValueFalse() { + let scheduler = TestScheduler(initialClock: 0) + + let xs = scheduler.createHotObservable([ + next(90, 1), + next(180, 2), + next(250, 3), + next(260, 4), + next(310, 5), + next(360, 6), + completed(390) + ]) + + let ys = scheduler.createHotObservable([ + next(240, true), + next(290, false), + completed(320) + ]) + + let res = scheduler.start { + xs.pausable(ys) + } + + XCTAssertEqual(res.events, [ + next(250, 3), + next(260, 4), + ]) + + XCTAssertEqual(xs.subscriptions, [ + Subscription(240, 290) + ]) + + XCTAssertEqual(ys.subscriptions, [ + Subscription(200, 320) + ]) + + } + +} From 8be6b2805f0732615fc334bf9f341737a6a4f765 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 25 Jan 2016 10:24:24 +0100 Subject: [PATCH 19/36] Adds retryIf Rx operator Mostly a helper to retryWhen but improves readability --- WordPress/Classes/Extensions/Rx.swift | 25 +++++++++++++ .../WordPressTest/Extensions/RxTests.swift | 35 +++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/WordPress/Classes/Extensions/Rx.swift b/WordPress/Classes/Extensions/Rx.swift index 31b92c9e35d5..13babef769af 100644 --- a/WordPress/Classes/Extensions/Rx.swift +++ b/WordPress/Classes/Extensions/Rx.swift @@ -56,3 +56,28 @@ class Pausable: ObservableType { return _group } } + +// MARK: - retryIf + +extension ObservableType { + /** + Repeats the source observable sequence on error if the given condition evaluates true. + + - parameter condition: A closure to be evaluated on error to decide if the source sequence should be retried. It takes two parameters: an incrementing `count` integer, and a `lastError` containing the latest error emitted. + - returns: An observable sequence producing the elements of the given sequence repeatedly until it terminates successfully or the condition evaluates false. + */ + public func retryIf(condition: (count: Int, lastError: NSError) -> Bool) -> Observable { + return retryWhen { (errors: Observable) in + errors.scan((0, nil)) { (accumulator: (Int, NSError!), error) in + (accumulator.0 + 1, error) + } + .flatMap { (count, lastError) -> Observable in + if condition(count: count, lastError: lastError) { + return Observable.just(count) + } else { + return Observable.error(lastError) + } + } + } + } +} \ No newline at end of file diff --git a/WordPress/WordPressTest/Extensions/RxTests.swift b/WordPress/WordPressTest/Extensions/RxTests.swift index c51c28024987..6cbb830427c5 100644 --- a/WordPress/WordPressTest/Extensions/RxTests.swift +++ b/WordPress/WordPressTest/Extensions/RxTests.swift @@ -133,4 +133,39 @@ class RxTests: XCTestCase { } + func testRetryIf() { + let scheduler = TestScheduler(initialClock: 0) + + let xs = scheduler.createColdObservable([ + next(10, 1), + next(20, 2), + error(30, testError) + ]) + + let res = scheduler.start { + xs.retryIf({ (count, lastError) -> Bool in + return count < 3 + }) + } + + let correct = [ + next(210, 1), + next(220, 2), + next(240, 1), + next(250, 2), + next(270, 1), + next(280, 2), + error(290, testError) + ] + + XCTAssertEqual(res.events, correct) + + XCTAssertEqual(xs.subscriptions, [ + Subscription(200, 230), + Subscription(230, 260), + Subscription(260, 290) + ]) + } + + let testError = NSError(domain: "dummyError", code: -232, userInfo: nil) } From c150eac0440b6d09d29073a40bd358a140131769 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 25 Jan 2016 10:27:56 +0100 Subject: [PATCH 20/36] Fix Rx pausable test --- WordPress/WordPressTest/Extensions/RxTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/WordPressTest/Extensions/RxTests.swift b/WordPress/WordPressTest/Extensions/RxTests.swift index 6cbb830427c5..a3927a04aa7d 100644 --- a/WordPress/WordPressTest/Extensions/RxTests.swift +++ b/WordPress/WordPressTest/Extensions/RxTests.swift @@ -82,7 +82,7 @@ class RxTests: XCTestCase { XCTAssertEqual(res.events, [ next(310, 5), next(360, 6), - completed(300) + completed(390) ]) XCTAssertEqual(xs.subscriptions, [ From f5e028aef1d2b5bcef7f30afa7bcfb38e98020c0 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 25 Jan 2016 10:28:37 +0100 Subject: [PATCH 21/36] Make remoteSettings emit errors instead of .Failing --- .../Services/AccountSettingsService.swift | 16 ++++------------ .../AccountSettingsServiceTests.swift | 6 ++---- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/WordPress/Classes/Services/AccountSettingsService.swift b/WordPress/Classes/Services/AccountSettingsService.swift index a9e8c0aeee85..bd553a44eee8 100644 --- a/WordPress/Classes/Services/AccountSettingsService.swift +++ b/WordPress/Classes/Services/AccountSettingsService.swift @@ -50,24 +50,16 @@ class AccountSettingsService { self.updateSettings(settings) return .Idle }) - .catchError({ error in - let error = error as NSError - // We want to retry only for networking errors, so we convert errors that aren't - // on NSURLErrorDomain to a .Failed status and log them. + .retryIf({ (count, error) in if error.domain == NSURLErrorDomain { - DDLogSwift.logError("Error refreshing settings (will retry): \(error)") - throw error + DDLogSwift.logError("Error refreshing settings (attempt \(count)): \(error)") } else { DDLogSwift.logError("Error refreshing settings (unrecoverable): \(error)") - return Observable.just(.Failed) } + + return error.domain == NSURLErrorDomain && count < Defaults.maxRetries }) - .retry(Defaults.maxRetries) .startWith(.Refreshing) - .doOn(onError: { (error) -> Void in - DDLogSwift.logError("Error refreshing settings (maxRetries reached): \(error)") - }) - .catchErrorJustReturn(.Failed) }() /// Emits one `.Stalled` value after a timeout and then completes diff --git a/WordPress/WordPressTest/AccountSettingsServiceTests.swift b/WordPress/WordPressTest/AccountSettingsServiceTests.swift index 34e723290efa..0d5ba6dd73a9 100644 --- a/WordPress/WordPressTest/AccountSettingsServiceTests.swift +++ b/WordPress/WordPressTest/AccountSettingsServiceTests.swift @@ -109,8 +109,7 @@ class AccountSettingsServiceTests: XCTestCase { XCTAssertEqual(requestCount, 3) XCTAssertEqual(res.events, [ next(200, .Refreshing), - next(230, .Failed), - completed(230) + error(230, connectionLost) ]) } @@ -136,8 +135,7 @@ class AccountSettingsServiceTests: XCTestCase { XCTAssertEqual(requestCount, 1) XCTAssertEqual(res.events, [ next(200, .Refreshing), - next(210, .Failed), - completed(210) + error(210, unexpected) ]) } From fe4658750b2dc21a3827134c3d2179c4972d5747 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 25 Jan 2016 11:02:12 +0100 Subject: [PATCH 22/36] Unit test for .Stalled Refactored AccountSettingsService a bit so it only exposes `request`. Reverted the change where `remoteSettings` would initially emit a `.Refreshing` value as it breaks the `amb` in `request` --- .../Services/AccountSettingsService.swift | 19 +++++-- .../AccountSettingsServiceTests.swift | 57 +++++++++++++++---- 2 files changed, 58 insertions(+), 18 deletions(-) diff --git a/WordPress/Classes/Services/AccountSettingsService.swift b/WordPress/Classes/Services/AccountSettingsService.swift index bd553a44eee8..7fad63c15661 100644 --- a/WordPress/Classes/Services/AccountSettingsService.swift +++ b/WordPress/Classes/Services/AccountSettingsService.swift @@ -24,6 +24,11 @@ class AccountSettingsService { private let context = ContextManager.sharedInstance().mainContext + var testScheduler: SchedulerType? = nil + private var scheduler: SchedulerType { + return testScheduler ?? MainScheduler.instance + } + init(userID: Int, api: WordPressComApi) { self.remote = AccountSettingsRemote.remoteWithApi(api) self.userID = userID @@ -44,7 +49,7 @@ class AccountSettingsService { /// - If it reaches the maximum permitted number of retries it will emit an Error. /// - If an error not related to networking happens, it will emit an Error. /// - When the data is refreshed, it will emit an `.Idle` value and complete. - lazy var remoteSettings: Observable = { + private lazy var remoteSettings: Observable = { return self.remote.settings .map({ settings -> RefreshStatus in self.updateSettings(settings) @@ -59,23 +64,25 @@ class AccountSettingsService { return error.domain == NSURLErrorDomain && count < Defaults.maxRetries }) - .startWith(.Refreshing) }() /// Emits one `.Stalled` value after a timeout and then completes - let stalled = Observable + private lazy var stalled: Observable = { + return Observable .just(.Stalled) - .delaySubscription(Defaults.stallTimeout, scheduler: MainScheduler.instance) + .delaySubscription(Defaults.stallTimeout, scheduler: self.scheduler) + }() /// Performs a network refresh, emitting a `.Stalled` value if it's taking too long. It initially emits a `.Refreshing` value. /// - seealso: remoteSettings - lazy private var request: Observable = { - let remoteSettings = self.remoteSettings + lazy private(set) var request: Observable = { + let remoteSettings = self.remoteSettings.share() let stalledSettings = Observable.of(self.stalled, remoteSettings) .merge() return remoteSettings .amb(stalledSettings) + .startWith(.Refreshing) }() /// Emits values when the refresh status changes. diff --git a/WordPress/WordPressTest/AccountSettingsServiceTests.swift b/WordPress/WordPressTest/AccountSettingsServiceTests.swift index 0d5ba6dd73a9..e9cc55db5111 100644 --- a/WordPress/WordPressTest/AccountSettingsServiceTests.swift +++ b/WordPress/WordPressTest/AccountSettingsServiceTests.swift @@ -28,8 +28,8 @@ class AccountSettingsServiceTests: XCTestCase { super.tearDown() } - func testRemoteSettingsSuccessful() { - let scheduler = TestScheduler(initialClock: 0) + func testRequestSuccessful() { + let scheduler = TestScheduler(initialClock: 0, resolution: 0.01) let mockRemote = MockAccountSettingsRemote() var requestCount = 0 mockRemote.settings = Observable.create { observer in @@ -42,9 +42,10 @@ class AccountSettingsServiceTests: XCTestCase { } let service = AccountSettingsService(userID: 123, remote: mockRemote) + service.testScheduler = scheduler let res = scheduler.start { - service.remoteSettings + service.request } XCTAssertEqual(requestCount, 1) @@ -55,8 +56,8 @@ class AccountSettingsServiceTests: XCTestCase { ]) } - func testRemoteSettingsOneNetworkErrorShouldRetry() { - let scheduler = TestScheduler(initialClock: 0) + func testRequestOneNetworkErrorShouldRetry() { + let scheduler = TestScheduler(initialClock: 0, resolution: 0.01) let mockRemote = MockAccountSettingsRemote() var requestCount = 0 mockRemote.settings = Observable.create { observer in @@ -74,9 +75,10 @@ class AccountSettingsServiceTests: XCTestCase { } let service = AccountSettingsService(userID: 123, remote: mockRemote) + service.testScheduler = scheduler let res = scheduler.start { - service.remoteSettings + service.request } XCTAssertEqual(requestCount, 2) @@ -87,8 +89,8 @@ class AccountSettingsServiceTests: XCTestCase { ]) } - func testRemoteSettingsFourNetworkErrorsShouldFail() { - let scheduler = TestScheduler(initialClock: 0) + func testRequestFourNetworkErrorsShouldFail() { + let scheduler = TestScheduler(initialClock: 0, resolution: 0.01) let mockRemote = MockAccountSettingsRemote() var requestCount = 0 let connectionLost = NSError(domain: NSURLErrorDomain, code: NSURLErrorNetworkConnectionLost, userInfo: nil) @@ -101,9 +103,10 @@ class AccountSettingsServiceTests: XCTestCase { } let service = AccountSettingsService(userID: 123, remote: mockRemote) + service.testScheduler = scheduler let res = scheduler.start { - service.remoteSettings + service.request } XCTAssertEqual(requestCount, 3) @@ -113,8 +116,8 @@ class AccountSettingsServiceTests: XCTestCase { ]) } - func testRemoteSettingsUnrecoverableErrorsShouldFailImmediately() { - let scheduler = TestScheduler(initialClock: 0) + func testRequestUnrecoverableErrorsShouldFailImmediately() { + let scheduler = TestScheduler(initialClock: 0, resolution: 0.01) let mockRemote = MockAccountSettingsRemote() var requestCount = 0 let unexpected = NSError(domain: "Unexpected", code: -999, userInfo: nil) @@ -127,9 +130,10 @@ class AccountSettingsServiceTests: XCTestCase { } let service = AccountSettingsService(userID: 123, remote: mockRemote) + service.testScheduler = scheduler let res = scheduler.start { - service.remoteSettings + service.request } XCTAssertEqual(requestCount, 1) @@ -139,6 +143,35 @@ class AccountSettingsServiceTests: XCTestCase { ]) } + func testRequestEmitsStalledValue() { + let scheduler = TestScheduler(initialClock: 0, resolution: 0.01) + let mockRemote = MockAccountSettingsRemote() + var requestCount = 0 + mockRemote.settings = Observable.create { observer in + requestCount += 1 + return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 500, action: { _ in + observer.on(.Next(TestData.sampleSettings)) + observer.on(.Completed) + return NopDisposable.instance + }) + } + + let service = AccountSettingsService(userID: 123, remote: mockRemote) + service.testScheduler = scheduler + + let res = scheduler.start { + service.request + } + + XCTAssertEqual(requestCount, 1) + XCTAssertEqual(res.events, [ + next(200, .Refreshing), + next(600, .Stalled), + next(700, .Idle), + completed(700) + ]) + } + } class MockAccountSettingsRemote: AccountSettingsRemoteInterface { From 4626660e2509df25d938b287e46736f78e962bbf Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 25 Jan 2016 11:06:00 +0100 Subject: [PATCH 23/36] Updated documentation for request --- .../Classes/Services/AccountSettingsService.swift | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/WordPress/Classes/Services/AccountSettingsService.swift b/WordPress/Classes/Services/AccountSettingsService.swift index 7fad63c15661..8a9d01daf4fd 100644 --- a/WordPress/Classes/Services/AccountSettingsService.swift +++ b/WordPress/Classes/Services/AccountSettingsService.swift @@ -73,8 +73,14 @@ class AccountSettingsService { .delaySubscription(Defaults.stallTimeout, scheduler: self.scheduler) }() - /// Performs a network refresh, emitting a `.Stalled` value if it's taking too long. It initially emits a `.Refreshing` value. - /// - seealso: remoteSettings + /// Performs a network refresh of settings and emits values with the refresh status. + /// + /// - When it's subscribed, it requests a refresh from the server + /// - If it takes more than `stallTimeout` to complete, it will emit a `.Stalled` value and continue waiting for the request to finish. + /// - If a networking error happens it doesn't emit a new value and will retry the request. + /// - If it reaches the maximum permitted number of retries it will emit an Error. + /// - If an error not related to networking happens, it will emit an Error. + /// - When the data is refreshed, it will emit an `.Idle` value and complete. lazy private(set) var request: Observable = { let remoteSettings = self.remoteSettings.share() let stalledSettings = Observable.of(self.stalled, remoteSettings) From 5eae3de4d19eca6277369336ccb01d7e09892bad Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 25 Jan 2016 12:17:42 +0100 Subject: [PATCH 24/36] Unit tests for settings reachability --- .../Services/AccountSettingsService.swift | 11 +- .../AccountSettingsServiceTests.swift | 180 ++++++++++++++++++ 2 files changed, 187 insertions(+), 4 deletions(-) diff --git a/WordPress/Classes/Services/AccountSettingsService.swift b/WordPress/Classes/Services/AccountSettingsService.swift index 8a9d01daf4fd..8bdaed95e1f5 100644 --- a/WordPress/Classes/Services/AccountSettingsService.swift +++ b/WordPress/Classes/Services/AccountSettingsService.swift @@ -39,8 +39,11 @@ class AccountSettingsService { self.remote = remote } + var testReachability: Observable? = nil /// Emits a boolean value each time reachability changes for the internet connection. - private let reachable = Reachability.internetConnection + private lazy var reachable: Observable = { + return self.testReachability ?? Reachability.internetConnection + }() /// Performs a network refresh of settings and emits values with the refresh status. /// @@ -82,7 +85,7 @@ class AccountSettingsService { /// - If an error not related to networking happens, it will emit an Error. /// - When the data is refreshed, it will emit an `.Idle` value and complete. lazy private(set) var request: Observable = { - let remoteSettings = self.remoteSettings.share() + let remoteSettings = self.remoteSettings.shareReplayLatestWhileConnected() let stalledSettings = Observable.of(self.stalled, remoteSettings) .merge() @@ -97,16 +100,16 @@ class AccountSettingsService { /// Possible values: /// - `.Refreshing` when it starts getting remote data. /// - `.Stalled` when it's getting remote data and hasn't succeeded before `stallTimeout`. - /// - `.Failed` when the request couldn't complete. It will retry after the polling interval. /// - `.Offline` when there is no internet connection. /// - `.Idle` when the request was successful and it's waiting for the polling interval. + /// - An error when the request couldn't complete. It will stop retrying. lazy var refresh: Observable = { // Copy request to avoid capture of self in closure let request = self.request // Convert to a polling request let polling = Observable - .interval(Defaults.pollingInterval, scheduler: MainScheduler.instance) + .interval(Defaults.pollingInterval, scheduler: self.scheduler) .startWith(0) .flatMapLatest({ _ in request }) diff --git a/WordPress/WordPressTest/AccountSettingsServiceTests.swift b/WordPress/WordPressTest/AccountSettingsServiceTests.swift index e9cc55db5111..178b5e997666 100644 --- a/WordPress/WordPressTest/AccountSettingsServiceTests.swift +++ b/WordPress/WordPressTest/AccountSettingsServiceTests.swift @@ -28,6 +28,8 @@ class AccountSettingsServiceTests: XCTestCase { super.tearDown() } + // MARK: - request + func testRequestSuccessful() { let scheduler = TestScheduler(initialClock: 0, resolution: 0.01) let mockRemote = MockAccountSettingsRemote() @@ -172,6 +174,184 @@ class AccountSettingsServiceTests: XCTestCase { ]) } + // MARK: - refresh + + func testRefreshRepeatsSuccessfulRequest() { + let scheduler = TestScheduler(initialClock: 0, resolution: 0.1) + let mockRemote = MockAccountSettingsRemote() + var requestCount = 0 + mockRemote.settings = Observable.create { observer in + requestCount += 1 + return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in + observer.on(.Next(TestData.sampleSettings)) + observer.on(.Completed) + return NopDisposable.instance + }) + } + + let service = AccountSettingsService(userID: 123, remote: mockRemote) + service.testScheduler = scheduler + + let res = scheduler.start { + service.refresh + } + + XCTAssertEqual(requestCount, 2) + XCTAssertEqual(res.events, [ + next(200, .Refreshing), + next(210, .Idle), + next(800, .Refreshing), + next(810, .Idle) + ]) + } + + func testRefreshDoesntRepeatFailedRequest() { + let scheduler = TestScheduler(initialClock: 0, resolution: 0.1) + let mockRemote = MockAccountSettingsRemote() + var requestCount = 0 + let unexpected = NSError(domain: "Unexpected", code: -999, userInfo: nil) + mockRemote.settings = Observable.create { observer in + requestCount += 1 + return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in + observer.on(.Error(unexpected)) + return NopDisposable.instance + }) + } + + let service = AccountSettingsService(userID: 123, remote: mockRemote) + service.testScheduler = scheduler + + let res = scheduler.start { + service.refresh + } + + XCTAssertEqual(requestCount, 1) + XCTAssertEqual(res.events, [ + next(200, .Refreshing), + error(210, unexpected) + ]) + } + + func testRefreshDoesntRequestIfUnreachable() { + let scheduler = TestScheduler(initialClock: 0, resolution: 0.1) + let mockRemote = MockAccountSettingsRemote() + var requestCount = 0 + mockRemote.settings = Observable.create { observer in + requestCount += 1 + return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in + observer.on(.Next(TestData.sampleSettings)) + observer.on(.Completed) + return NopDisposable.instance + }) + } + + let service = AccountSettingsService(userID: 123, remote: mockRemote) + service.testScheduler = scheduler + service.testReachability = Observable.create { observer in + return scheduler.scheduleAbsoluteVirtual((), time: 200, action: { _ in + observer.on(.Next(false)) + return NopDisposable.instance + }) + } + + let res = scheduler.start { + service.refresh + } + + XCTAssertEqual(requestCount, 0) + XCTAssertEqual(res.events, [ + next(200, .Offline), + ]) + + } + + func testRefreshRetriesWhenReachable() { + let scheduler = TestScheduler(initialClock: 0, resolution: 0.1) + let mockRemote = MockAccountSettingsRemote() + var requestCount = 0 + mockRemote.settings = Observable.create { observer in + requestCount += 1 + return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in + observer.on(.Next(TestData.sampleSettings)) + observer.on(.Completed) + return NopDisposable.instance + }) + } + + let service = AccountSettingsService(userID: 123, remote: mockRemote) + service.testScheduler = scheduler + service.testReachability = Observable.create { observer in + scheduler.scheduleAbsoluteVirtual((), time: 200, action: { _ in + observer.on(.Next(false)) + return NopDisposable.instance + }) + scheduler.scheduleAbsoluteVirtual((), time: 500, action: { _ in + observer.on(.Next(true)) + return NopDisposable.instance + }) + scheduler.scheduleAbsoluteVirtual((), time: 800, action: { _ in + observer.on(.Next(false)) + return NopDisposable.instance + }) + return NopDisposable.instance + } + + let res = scheduler.start { + service.refresh + } + + XCTAssertEqual(requestCount, 1) + XCTAssertEqual(res.events, [ + next(200, .Offline), + next(500, .Refreshing), + next(510, .Idle), + next(800, .Offline) + ]) + } + + func testRefreshDoesntRepeatFailedRequestAfterReachable() { + let scheduler = TestScheduler(initialClock: 0, resolution: 0.1) + let mockRemote = MockAccountSettingsRemote() + var requestCount = 0 + let unexpected = NSError(domain: "Unexpected", code: -999, userInfo: nil) + mockRemote.settings = Observable.create { observer in + requestCount += 1 + return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in + observer.on(.Error(unexpected)) + return NopDisposable.instance + }) + } + + let service = AccountSettingsService(userID: 123, remote: mockRemote) + service.testScheduler = scheduler + service.testReachability = Observable.create { observer in + scheduler.scheduleAbsoluteVirtual((), time: 200, action: { _ in + observer.on(.Next(false)) + return NopDisposable.instance + }) + scheduler.scheduleAbsoluteVirtual((), time: 500, action: { _ in + observer.on(.Next(true)) + return NopDisposable.instance + }) + scheduler.scheduleAbsoluteVirtual((), time: 800, action: { _ in + observer.on(.Next(false)) + return NopDisposable.instance + }) + return NopDisposable.instance + } + + let res = scheduler.start { + service.refresh + } + + XCTAssertEqual(requestCount, 1) + XCTAssertEqual(res.events, [ + next(200, .Offline), + next(500, .Refreshing), + error(510, unexpected) + ]) + } + } class MockAccountSettingsRemote: AccountSettingsRemoteInterface { From b9af55c87e713186b64f7885fcb2697e21ddeb1e Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 25 Jan 2016 12:35:02 +0100 Subject: [PATCH 25/36] Refactor unit tests for readability Use createColdObservable when possible as it's way more readable. The only reason to avoid it is in `testRequestOneNetworkErrorShouldRetry` because we want to return something different the second time it's subscribed. --- .../AccountSettingsServiceTests.swift | 184 ++++++------------ 1 file changed, 64 insertions(+), 120 deletions(-) diff --git a/WordPress/WordPressTest/AccountSettingsServiceTests.swift b/WordPress/WordPressTest/AccountSettingsServiceTests.swift index 178b5e997666..0c08c0e22414 100644 --- a/WordPress/WordPressTest/AccountSettingsServiceTests.swift +++ b/WordPress/WordPressTest/AccountSettingsServiceTests.swift @@ -32,16 +32,12 @@ class AccountSettingsServiceTests: XCTestCase { func testRequestSuccessful() { let scheduler = TestScheduler(initialClock: 0, resolution: 0.01) + let remoteSettings: TestableObservable = scheduler.createColdObservable([ + next(10, TestData.sampleSettings), + completed(10) + ]) let mockRemote = MockAccountSettingsRemote() - var requestCount = 0 - mockRemote.settings = Observable.create { observer in - requestCount += 1 - return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in - observer.on(.Next(TestData.sampleSettings)) - observer.on(.Completed) - return NopDisposable.instance - }) - } + mockRemote.settings = remoteSettings.asObservable() let service = AccountSettingsService(userID: 123, remote: mockRemote) service.testScheduler = scheduler @@ -50,7 +46,7 @@ class AccountSettingsServiceTests: XCTestCase { service.request } - XCTAssertEqual(requestCount, 1) + XCTAssertEqual(remoteSettings.subscriptions.count, 1) XCTAssertEqual(res.events, [ next(200, .Refreshing), next(210, .Idle), @@ -93,16 +89,12 @@ class AccountSettingsServiceTests: XCTestCase { func testRequestFourNetworkErrorsShouldFail() { let scheduler = TestScheduler(initialClock: 0, resolution: 0.01) - let mockRemote = MockAccountSettingsRemote() - var requestCount = 0 let connectionLost = NSError(domain: NSURLErrorDomain, code: NSURLErrorNetworkConnectionLost, userInfo: nil) - mockRemote.settings = Observable.create { observer in - requestCount += 1 - return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in - observer.on(.Error(connectionLost)) - return NopDisposable.instance - }) - } + let remoteSettings: TestableObservable = scheduler.createColdObservable([ + error(10, connectionLost) + ]) + let mockRemote = MockAccountSettingsRemote() + mockRemote.settings = remoteSettings.asObservable() let service = AccountSettingsService(userID: 123, remote: mockRemote) service.testScheduler = scheduler @@ -111,7 +103,7 @@ class AccountSettingsServiceTests: XCTestCase { service.request } - XCTAssertEqual(requestCount, 3) + XCTAssertEqual(remoteSettings.subscriptions.count, 3) XCTAssertEqual(res.events, [ next(200, .Refreshing), error(230, connectionLost) @@ -120,16 +112,12 @@ class AccountSettingsServiceTests: XCTestCase { func testRequestUnrecoverableErrorsShouldFailImmediately() { let scheduler = TestScheduler(initialClock: 0, resolution: 0.01) - let mockRemote = MockAccountSettingsRemote() - var requestCount = 0 let unexpected = NSError(domain: "Unexpected", code: -999, userInfo: nil) - mockRemote.settings = Observable.create { observer in - requestCount += 1 - return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in - observer.on(.Error(unexpected)) - return NopDisposable.instance - }) - } + let remoteSettings: TestableObservable = scheduler.createColdObservable([ + error(10, unexpected) + ]) + let mockRemote = MockAccountSettingsRemote() + mockRemote.settings = remoteSettings.asObservable() let service = AccountSettingsService(userID: 123, remote: mockRemote) service.testScheduler = scheduler @@ -138,7 +126,7 @@ class AccountSettingsServiceTests: XCTestCase { service.request } - XCTAssertEqual(requestCount, 1) + XCTAssertEqual(remoteSettings.subscriptions.count, 1) XCTAssertEqual(res.events, [ next(200, .Refreshing), error(210, unexpected) @@ -147,16 +135,12 @@ class AccountSettingsServiceTests: XCTestCase { func testRequestEmitsStalledValue() { let scheduler = TestScheduler(initialClock: 0, resolution: 0.01) + let remoteSettings: TestableObservable = scheduler.createColdObservable([ + next(500, TestData.sampleSettings), + completed(500) + ]) let mockRemote = MockAccountSettingsRemote() - var requestCount = 0 - mockRemote.settings = Observable.create { observer in - requestCount += 1 - return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 500, action: { _ in - observer.on(.Next(TestData.sampleSettings)) - observer.on(.Completed) - return NopDisposable.instance - }) - } + mockRemote.settings = remoteSettings.asObservable() let service = AccountSettingsService(userID: 123, remote: mockRemote) service.testScheduler = scheduler @@ -165,7 +149,7 @@ class AccountSettingsServiceTests: XCTestCase { service.request } - XCTAssertEqual(requestCount, 1) + XCTAssertEqual(remoteSettings.subscriptions.count, 1) XCTAssertEqual(res.events, [ next(200, .Refreshing), next(600, .Stalled), @@ -178,16 +162,12 @@ class AccountSettingsServiceTests: XCTestCase { func testRefreshRepeatsSuccessfulRequest() { let scheduler = TestScheduler(initialClock: 0, resolution: 0.1) + let remoteSettings: TestableObservable = scheduler.createColdObservable([ + next(10, TestData.sampleSettings), + completed(10) + ]) let mockRemote = MockAccountSettingsRemote() - var requestCount = 0 - mockRemote.settings = Observable.create { observer in - requestCount += 1 - return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in - observer.on(.Next(TestData.sampleSettings)) - observer.on(.Completed) - return NopDisposable.instance - }) - } + mockRemote.settings = remoteSettings.asObservable() let service = AccountSettingsService(userID: 123, remote: mockRemote) service.testScheduler = scheduler @@ -196,7 +176,7 @@ class AccountSettingsServiceTests: XCTestCase { service.refresh } - XCTAssertEqual(requestCount, 2) + XCTAssertEqual(remoteSettings.subscriptions.count, 2) XCTAssertEqual(res.events, [ next(200, .Refreshing), next(210, .Idle), @@ -207,16 +187,12 @@ class AccountSettingsServiceTests: XCTestCase { func testRefreshDoesntRepeatFailedRequest() { let scheduler = TestScheduler(initialClock: 0, resolution: 0.1) - let mockRemote = MockAccountSettingsRemote() - var requestCount = 0 let unexpected = NSError(domain: "Unexpected", code: -999, userInfo: nil) - mockRemote.settings = Observable.create { observer in - requestCount += 1 - return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in - observer.on(.Error(unexpected)) - return NopDisposable.instance - }) - } + let remoteSettings: TestableObservable = scheduler.createColdObservable([ + error(10, unexpected) + ]) + let mockRemote = MockAccountSettingsRemote() + mockRemote.settings = remoteSettings.asObservable() let service = AccountSettingsService(userID: 123, remote: mockRemote) service.testScheduler = scheduler @@ -225,7 +201,7 @@ class AccountSettingsServiceTests: XCTestCase { service.refresh } - XCTAssertEqual(requestCount, 1) + XCTAssertEqual(remoteSettings.subscriptions.count, 1) XCTAssertEqual(res.events, [ next(200, .Refreshing), error(210, unexpected) @@ -234,16 +210,12 @@ class AccountSettingsServiceTests: XCTestCase { func testRefreshDoesntRequestIfUnreachable() { let scheduler = TestScheduler(initialClock: 0, resolution: 0.1) + let remoteSettings: TestableObservable = scheduler.createColdObservable([ + next(10, TestData.sampleSettings), + completed(10) + ]) let mockRemote = MockAccountSettingsRemote() - var requestCount = 0 - mockRemote.settings = Observable.create { observer in - requestCount += 1 - return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in - observer.on(.Next(TestData.sampleSettings)) - observer.on(.Completed) - return NopDisposable.instance - }) - } + mockRemote.settings = remoteSettings.asObservable() let service = AccountSettingsService(userID: 123, remote: mockRemote) service.testScheduler = scheduler @@ -258,7 +230,7 @@ class AccountSettingsServiceTests: XCTestCase { service.refresh } - XCTAssertEqual(requestCount, 0) + XCTAssertEqual(remoteSettings.subscriptions.count, 0) XCTAssertEqual(res.events, [ next(200, .Offline), ]) @@ -267,40 +239,26 @@ class AccountSettingsServiceTests: XCTestCase { func testRefreshRetriesWhenReachable() { let scheduler = TestScheduler(initialClock: 0, resolution: 0.1) + let remoteSettings: TestableObservable = scheduler.createColdObservable([ + next(10, TestData.sampleSettings), + completed(10) + ]) let mockRemote = MockAccountSettingsRemote() - var requestCount = 0 - mockRemote.settings = Observable.create { observer in - requestCount += 1 - return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in - observer.on(.Next(TestData.sampleSettings)) - observer.on(.Completed) - return NopDisposable.instance - }) - } + mockRemote.settings = remoteSettings.asObservable() let service = AccountSettingsService(userID: 123, remote: mockRemote) service.testScheduler = scheduler - service.testReachability = Observable.create { observer in - scheduler.scheduleAbsoluteVirtual((), time: 200, action: { _ in - observer.on(.Next(false)) - return NopDisposable.instance - }) - scheduler.scheduleAbsoluteVirtual((), time: 500, action: { _ in - observer.on(.Next(true)) - return NopDisposable.instance - }) - scheduler.scheduleAbsoluteVirtual((), time: 800, action: { _ in - observer.on(.Next(false)) - return NopDisposable.instance - }) - return NopDisposable.instance - } + service.testReachability = scheduler.createColdObservable([ + next(0, false), + next(300, true), + next(600, false) + ]).asObservable() let res = scheduler.start { service.refresh } - XCTAssertEqual(requestCount, 1) + XCTAssertEqual(remoteSettings.subscriptions.count, 1) XCTAssertEqual(res.events, [ next(200, .Offline), next(500, .Refreshing), @@ -311,40 +269,26 @@ class AccountSettingsServiceTests: XCTestCase { func testRefreshDoesntRepeatFailedRequestAfterReachable() { let scheduler = TestScheduler(initialClock: 0, resolution: 0.1) - let mockRemote = MockAccountSettingsRemote() - var requestCount = 0 let unexpected = NSError(domain: "Unexpected", code: -999, userInfo: nil) - mockRemote.settings = Observable.create { observer in - requestCount += 1 - return scheduler.scheduleRelativeVirtual(requestCount, dueTime: 10, action: { _ in - observer.on(.Error(unexpected)) - return NopDisposable.instance - }) - } + let remoteSettings: TestableObservable = scheduler.createColdObservable([ + error(10, unexpected) + ]) + let mockRemote = MockAccountSettingsRemote() + mockRemote.settings = remoteSettings.asObservable() let service = AccountSettingsService(userID: 123, remote: mockRemote) service.testScheduler = scheduler - service.testReachability = Observable.create { observer in - scheduler.scheduleAbsoluteVirtual((), time: 200, action: { _ in - observer.on(.Next(false)) - return NopDisposable.instance - }) - scheduler.scheduleAbsoluteVirtual((), time: 500, action: { _ in - observer.on(.Next(true)) - return NopDisposable.instance - }) - scheduler.scheduleAbsoluteVirtual((), time: 800, action: { _ in - observer.on(.Next(false)) - return NopDisposable.instance - }) - return NopDisposable.instance - } + service.testReachability = scheduler.createColdObservable([ + next(0, false), + next(300, true), + next(600, false) + ]).asObservable() let res = scheduler.start { service.refresh } - XCTAssertEqual(requestCount, 1) + XCTAssertEqual(remoteSettings.subscriptions.count, 1) XCTAssertEqual(res.events, [ next(200, .Offline), next(500, .Refreshing), From 4f10c4abb9eeed42023da0c4557a4a762934fc7f Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 25 Jan 2016 18:30:10 +0100 Subject: [PATCH 26/36] Renamed AccountSettingsRemoteTests --- WordPress/WordPress.xcodeproj/project.pbxproj | 8 ++++---- ...RemoteTests.swift => AccountSettingsRemoteTests.swift} | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) rename WordPress/WordPressTest/{AccountSettingsServiceRemoteTests.swift => AccountSettingsRemoteTests.swift} (98%) diff --git a/WordPress/WordPress.xcodeproj/project.pbxproj b/WordPress/WordPress.xcodeproj/project.pbxproj index ad2cc5a41d21..be9c8e4c421b 100644 --- a/WordPress/WordPress.xcodeproj/project.pbxproj +++ b/WordPress/WordPress.xcodeproj/project.pbxproj @@ -473,7 +473,7 @@ E131CB5816CACFB4004B0314 /* get-user-blogs_doesnt-have-blog.json in Resources */ = {isa = PBXBuildFile; fileRef = E131CB5716CACFB4004B0314 /* get-user-blogs_doesnt-have-blog.json */; }; E131F5351C2930FC00D2D975 /* String+Helpers.swift in Sources */ = {isa = PBXBuildFile; fileRef = E131F5341C2930FC00D2D975 /* String+Helpers.swift */; }; E13A8C9B1C3E6EF2005BB1C1 /* ImmuTable+WordPress.swift in Sources */ = {isa = PBXBuildFile; fileRef = E13A8C9A1C3E6EF2005BB1C1 /* ImmuTable+WordPress.swift */; }; - E13BF2CA1C522A1300275BE9 /* AccountSettingsServiceRemoteTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E13BF2C91C522A1300275BE9 /* AccountSettingsServiceRemoteTests.swift */; }; + E13BF2CA1C522A1300275BE9 /* AccountSettingsRemoteTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E13BF2C91C522A1300275BE9 /* AccountSettingsRemoteTests.swift */; }; E13EB7A5157D230000885780 /* WordPressComApi.m in Sources */ = {isa = PBXBuildFile; fileRef = E13EB7A4157D230000885780 /* WordPressComApi.m */; }; E13F23C314FE84600081D9CC /* NSMutableDictionary+Helpers.m in Sources */ = {isa = PBXBuildFile; fileRef = E13F23C214FE84600081D9CC /* NSMutableDictionary+Helpers.m */; }; E14200781C117A2E00B3B115 /* ManagedAccountSettings.swift in Sources */ = {isa = PBXBuildFile; fileRef = E14200771C117A2E00B3B115 /* ManagedAccountSettings.swift */; }; @@ -1502,7 +1502,7 @@ E131F5341C2930FC00D2D975 /* String+Helpers.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "String+Helpers.swift"; sourceTree = ""; }; E133DB40137AE180003C0AF9 /* he */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = he; path = he.lproj/Localizable.strings; sourceTree = ""; }; E13A8C9A1C3E6EF2005BB1C1 /* ImmuTable+WordPress.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "ImmuTable+WordPress.swift"; sourceTree = ""; }; - E13BF2C91C522A1300275BE9 /* AccountSettingsServiceRemoteTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = AccountSettingsServiceRemoteTests.swift; sourceTree = ""; }; + E13BF2C91C522A1300275BE9 /* AccountSettingsRemoteTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = AccountSettingsRemoteTests.swift; sourceTree = ""; }; E13EB7A3157D230000885780 /* WordPressComApi.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WordPressComApi.h; sourceTree = ""; }; E13EB7A4157D230000885780 /* WordPressComApi.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = WordPressComApi.m; sourceTree = ""; }; E13F23C114FE84600081D9CC /* NSMutableDictionary+Helpers.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = "NSMutableDictionary+Helpers.h"; sourceTree = ""; }; @@ -2211,7 +2211,7 @@ isa = PBXGroup; children = ( 591CFB051B28A960009E61B3 /* AccountServiceRemoteRESTTests.m */, - E13BF2C91C522A1300275BE9 /* AccountSettingsServiceRemoteTests.swift */, + E13BF2C91C522A1300275BE9 /* AccountSettingsRemoteTests.swift */, 591CFB081B28AC8C009E61B3 /* BlogServiceRemoteRESTTests.m */, 59E2AAEB1B20E5CE0051DC06 /* PostServiceRemoteRESTTests.m */, 59E2AAE71B20E3EA0051DC06 /* ServiceRemoteRESTTests.m */, @@ -4919,7 +4919,7 @@ B5D689FD1A5EBC900063D9E5 /* NotificationsManager+TestHelper.m in Sources */, BEA0E4851BD83565000AEE81 /* WP3DTouchShortcutCreatorTests.swift in Sources */, 85F8E19B1B017AA6000859BB /* PushAuthenticationServiceRemoteTests.swift in Sources */, - E13BF2CA1C522A1300275BE9 /* AccountSettingsServiceRemoteTests.swift in Sources */, + E13BF2CA1C522A1300275BE9 /* AccountSettingsRemoteTests.swift in Sources */, 59FBD5621B5684F300734466 /* ThemeServiceTests.m in Sources */, 85F8E19D1B018698000859BB /* PushAuthenticationServiceTests.swift in Sources */, 931D270019EDAE8600114F17 /* CoreDataMigrationTests.m in Sources */, diff --git a/WordPress/WordPressTest/AccountSettingsServiceRemoteTests.swift b/WordPress/WordPressTest/AccountSettingsRemoteTests.swift similarity index 98% rename from WordPress/WordPressTest/AccountSettingsServiceRemoteTests.swift rename to WordPress/WordPressTest/AccountSettingsRemoteTests.swift index 27d3d4d032fa..5e2c579913bd 100644 --- a/WordPress/WordPressTest/AccountSettingsServiceRemoteTests.swift +++ b/WordPress/WordPressTest/AccountSettingsRemoteTests.swift @@ -4,7 +4,7 @@ import OHHTTPStubs import RxSwift @testable import WordPress -class AccountSettingsServiceRemoteTests: XCTestCase { +class AccountSettingsRemoteTests: XCTestCase { override func setUp() { super.setUp() From 63b76c12dca577b9f2d95c8ff4e0d00cdd02c54d Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 25 Jan 2016 18:30:22 +0100 Subject: [PATCH 27/36] Don't share remote's settings observable We're already combining subscription in the service, and this was preventing a second settings request when My Profile was dismissed and presented again --- WordPress/Classes/Networking/AccountSettingsRemote.swift | 1 - 1 file changed, 1 deletion(-) diff --git a/WordPress/Classes/Networking/AccountSettingsRemote.swift b/WordPress/Classes/Networking/AccountSettingsRemote.swift index 9fcb6bd92659..c16e99d330a0 100644 --- a/WordPress/Classes/Networking/AccountSettingsRemote.swift +++ b/WordPress/Classes/Networking/AccountSettingsRemote.swift @@ -66,7 +66,6 @@ class AccountSettingsRemote: ServiceRemoteREST { } return settings - .share() } func getSettings(success success: AccountSettings -> Void, failure: ErrorType -> Void) -> AFHTTPRequestOperation? { From 15861320852f436db757bc83627569eb270f3116 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Mon, 25 Jan 2016 18:53:19 +0100 Subject: [PATCH 28/36] Fixed stuck error message I was creating and adding the label even if the animation was to hide it, so there was still an error message hiding behind the navbar, visible when you scrolled. --- WordPress/Classes/Utility/ErrorAnimator.swift | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/WordPress/Classes/Utility/ErrorAnimator.swift b/WordPress/Classes/Utility/ErrorAnimator.swift index de98330893cb..4bfdb5290548 100644 --- a/WordPress/Classes/Utility/ErrorAnimator.swift +++ b/WordPress/Classes/Utility/ErrorAnimator.swift @@ -59,10 +59,12 @@ class ErrorAnimator: Animator { } private func preamble() { - errorLabel = createErrorLabel() - targetView.addSubview(errorLabel!) - errorLabel?.frame.size.height = 0 - errorLabel?.label.alpha = 0 + if showingError { + errorLabel = createErrorLabel() + targetView.addSubview(errorLabel!) + errorLabel?.frame.size.height = 0 + errorLabel?.label.alpha = 0 + } UIView.performWithoutAnimation { [unowned self] in self.targetView.layoutIfNeeded() From f918d9355c60a8435d5ee0367b16a518c119d9ba Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Tue, 26 Jan 2016 10:22:37 +0100 Subject: [PATCH 29/36] Don't log settings errors twice The service is already logging any errors other than cancelation. --- WordPress/Classes/Networking/AccountSettingsRemote.swift | 1 - 1 file changed, 1 deletion(-) diff --git a/WordPress/Classes/Networking/AccountSettingsRemote.swift b/WordPress/Classes/Networking/AccountSettingsRemote.swift index c16e99d330a0..68cd310605a2 100644 --- a/WordPress/Classes/Networking/AccountSettingsRemote.swift +++ b/WordPress/Classes/Networking/AccountSettingsRemote.swift @@ -52,7 +52,6 @@ class AccountSettingsRemote: ServiceRemoteREST { // This probably means the observable is being disposed DDLogSwift.logError("Canceled refreshing settings") } else { - DDLogSwift.logError("Error refreshing settings: \(error)") observer.onError(error) } }) From 0a69fdfae02ec4585e26810ee82221c9e84f8c5e Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Tue, 26 Jan 2016 10:26:30 +0100 Subject: [PATCH 30/36] End file in new line --- WordPress/Classes/Extensions/Rx.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Classes/Extensions/Rx.swift b/WordPress/Classes/Extensions/Rx.swift index 13babef769af..591f1be87f0d 100644 --- a/WordPress/Classes/Extensions/Rx.swift +++ b/WordPress/Classes/Extensions/Rx.swift @@ -80,4 +80,4 @@ extension ObservableType { } } } -} \ No newline at end of file +} From ee7162903593f482612485575a1ee8807b959252 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Tue, 26 Jan 2016 15:45:03 +0100 Subject: [PATCH 31/36] Turn second initializer into a convenience initializer --- WordPress/Classes/Services/AccountSettingsService.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/WordPress/Classes/Services/AccountSettingsService.swift b/WordPress/Classes/Services/AccountSettingsService.swift index c90329c303f2..bbf1d5253453 100644 --- a/WordPress/Classes/Services/AccountSettingsService.swift +++ b/WordPress/Classes/Services/AccountSettingsService.swift @@ -29,9 +29,9 @@ class AccountSettingsService { return testScheduler ?? MainScheduler.instance } - init(userID: Int, api: WordPressComApi) { - self.remote = AccountSettingsRemote.remoteWithApi(api) - self.userID = userID + convenience init(userID: Int, api: WordPressComApi) { + let remote = AccountSettingsRemote.remoteWithApi(api) + self.init(userID: userID, remote: remote) } init(userID: Int, remote: AccountSettingsRemoteInterface) { From 5d2073638bef210b7b366c4609131226ab2b44e7 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Tue, 26 Jan 2016 15:45:30 +0100 Subject: [PATCH 32/36] Fixed Animator example --- WordPress/Classes/Utility/Animator.swift | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/WordPress/Classes/Utility/Animator.swift b/WordPress/Classes/Utility/Animator.swift index 65d10acbd5ea..b9ef34af4b44 100644 --- a/WordPress/Classes/Utility/Animator.swift +++ b/WordPress/Classes/Utility/Animator.swift @@ -16,15 +16,19 @@ import UIKit /// didSet { /// animator.animateWithDuration(0.3, /// preamble: { [unowned self] in -/// let view = self.createErrorView() -/// self.view.addSubview(view) -/// self.errorView = view -/// self.errorView?.alpha = 0 +/// if self.showError { +/// let view = self.createErrorView() +/// self.view.addSubview(view) +/// self.errorView = view +/// self.errorView?.alpha = 0 +/// } /// }, animations: { [unowned self] in /// self.errorView?.alpha = 1 /// }, cleanup: { [unowned self] in -/// self.errorView?.removeFromSuperview() -/// self.errorView = nil +/// if !self.showError { +/// self.errorView?.removeFromSuperview() +/// self.errorView = nil +/// } /// }) /// } /// } From e59301ddabb12f7719ae49b85eb1d33db4feeb72 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Wed, 27 Jan 2016 07:51:29 +0100 Subject: [PATCH 33/36] Add ImmuTable.Empty helper --- WordPress/Classes/Utility/ImmuTable.swift | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/WordPress/Classes/Utility/ImmuTable.swift b/WordPress/Classes/Utility/ImmuTable.swift index 6a4fbd96e6a8..8392fd3bf893 100644 --- a/WordPress/Classes/Utility/ImmuTable.swift +++ b/WordPress/Classes/Utility/ImmuTable.swift @@ -72,6 +72,13 @@ public struct ImmuTable { } } +extension ImmuTable { + /// Alias for an ImmuTable with no sections + static var Empty: ImmuTable { + return ImmuTable(sections: []) + } +} + // MARK: - @@ -242,7 +249,7 @@ public class ImmuTableViewHandler: NSObject, UITableViewDataSource, UITableViewD } /// An ImmuTable object representing the table structure. - public var viewModel = ImmuTable(sections: []) { + public var viewModel = ImmuTable.Empty { didSet { if target.isViewLoaded() { target.tableView.reloadData() From 4ac01479d096891106fc4117ab3ed32172233060 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Wed, 27 Jan 2016 08:15:27 +0100 Subject: [PATCH 34/36] Refactor My Profile to improve memory management Prior to this, MyProfileController was a class and owned the view controller. This had a couple problems: 1. We want to release the whole thing when UIKit decides it should deallocate the view controller, and for that we need the VC to own the controller. 2. Having the controller being a class led to a lot of retain cycles since the code relies on closures a lot. With the new code, the VC and controller are deallocated when the VC is popped, and the Observable subscriptions are properly disposed. --- .../Utility/ImmuTableViewController.swift | 40 +++++++-- .../Me/MyProfileViewController.swift | 84 +++++++++++-------- .../ViewRelated/MeViewController.swift | 4 +- 3 files changed, 83 insertions(+), 45 deletions(-) diff --git a/WordPress/Classes/Utility/ImmuTableViewController.swift b/WordPress/Classes/Utility/ImmuTableViewController.swift index cd0e1cfc6cd5..1e3f6b970f22 100644 --- a/WordPress/Classes/Utility/ImmuTableViewController.swift +++ b/WordPress/Classes/Utility/ImmuTableViewController.swift @@ -4,7 +4,8 @@ import WordPressShared typealias ImmuTableRowControllerGenerator = ImmuTableRow -> UIViewController -protocol ImmuTablePresenter: AnyObject { +protocol ImmuTablePresenter: class { + var visible: Observable { get } func push(controllerGenerator: ImmuTableRowControllerGenerator) -> ImmuTableAction } @@ -18,6 +19,14 @@ extension ImmuTablePresenter where Self: UIViewController { } } +protocol ImmuTableController { + var presenter: ImmuTablePresenter? { get set } + var title: String { get } + var immuTableRows: [ImmuTableRow.Type] { get } + var immuTable: Observable { get } + var errorMessage: Observable { get } +} + /// Generic view controller to present ImmuTable-based tables /// /// Instead of subclassing the view controller, this is designed to be used from @@ -32,10 +41,32 @@ final class ImmuTableViewController: UITableViewController, ImmuTablePresenter { private var errorAnimator: ErrorAnimator! + private let controller: ImmuTableController? + + private let bag = DisposeBag() + // MARK: - Table View Controller - init() { + init(controller: ImmuTableController? = nil) { + self.controller = controller super.init(style: .Grouped) + self.controller?.presenter = self + if let controller = self.controller { + title = controller.title + registerRows(controller.immuTableRows) + controller.immuTable + .observeOn(MainScheduler.instance) + .subscribeNext({ [weak self] in + self?.handler.viewModel = $0 + }) + .addDisposableTo(bag) + controller.errorMessage + .observeOn(MainScheduler.instance) + .subscribeNext({ [weak self] in + self?.errorMessage = $0 + }) + .addDisposableTo(bag) + } } required init?(coder aDecoder: NSCoder) { @@ -68,11 +99,6 @@ final class ImmuTableViewController: UITableViewController, ImmuTablePresenter { // MARK: - Inputs - /// Sets the view model for the view controller - func bindViewModel(viewModel: ImmuTable) { - handler.viewModel = viewModel - } - /// Registers custom rows /// - seealso: ImmuTable.registerRows(_:tableView) func registerRows(rows: [ImmuTableRow.Type]) { diff --git a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift index 8d17b633c353..fb178b188f24 100644 --- a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift +++ b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift @@ -2,71 +2,82 @@ import UIKit import RxSwift import WordPressShared -class MyProfileController: NSObject { - let title = NSLocalizedString("My Profile", comment: "My Profile view title") - let service: AccountSettingsService - let viewController = ImmuTableViewController() +func MyProfileViewController(account account: WPAccount) -> ImmuTableViewController { + let service = AccountSettingsService(userID: account.userID.integerValue, api: account.restApi) + return MyProfileViewController(service: service) +} - private let bag = DisposeBag() +func MyProfileViewController(service service: AccountSettingsService) -> ImmuTableViewController { + let controller = MyProfileController(service: service) + return ImmuTableViewController(controller: controller) +} - init(service: AccountSettingsService) { - self.service = service - super.init() +private struct MyProfileController: ImmuTableController { + // MARK: - ImmuTableController - viewController.title = title - viewController.registerRows(immutableRows) + weak var presenter: ImmuTablePresenter? = nil - viewModel - .observeOn(MainScheduler.instance) - .subscribeNext(viewController.bindViewModel) - .addDisposableTo(bag) + let title = NSLocalizedString("My Profile", comment: "My Profile view title") + + var immuTableRows: [ImmuTableRow.Type] { + return [EditableTextRow.self] + } + + var immuTable: Observable { + return service.settings.map(mapViewModel) + } - service.refresh - .pausable(viewController.visible) + var errorMessage: Observable { + precondition(presenter != nil) + guard let presenter = presenter else { + // This shouldn't happen, but if it does, disabling the error feels + // safer than having it running when the VC is not visible. + return Observable.just(nil) + } + return service.refresh + .pausable(presenter.visible) // replace errors with .Failed status .catchErrorJustReturn(.Failed) // convert status to string .map({ $0.errorMessage }) - // and set the view controller error message - .observeOn(MainScheduler.instance) - .subscribeNext { [weak self] message in - self?.viewController.errorMessage = message - } - .addDisposableTo(bag) } - convenience init(account: WPAccount) { - self.init(service: AccountSettingsService(userID: account.userID.integerValue, api: account.restApi)) - } + // MARK: - Initialization - var immutableRows: [ImmuTableRow.Type] { - return [EditableTextRow.self] - } + let service: AccountSettingsService - var viewModel: Observable { - return service.settings.map(mapViewModel) + init(service: AccountSettingsService) { + self.service = service } + // MARK: - Model mapping + func mapViewModel(settings: AccountSettings?) -> ImmuTable { + precondition(presenter != nil) + guard let presenter = presenter else { + // This shouldn't happen. If there's no presenter we can't push the + // editText controllers. + return ImmuTable.Empty + } let firstNameRow = EditableTextRow( title: NSLocalizedString("First Name", comment: "My Profile first name label"), value: settings?.firstName ?? "", - action: viewController.push(editText(AccountSettingsChange.FirstName))) + action: presenter.push(editText(AccountSettingsChange.FirstName))) let lastNameRow = EditableTextRow( title: NSLocalizedString("Last Name", comment: "My Profile last name label"), value: settings?.lastName ?? "", - action: viewController.push(editText(AccountSettingsChange.LastName))) + action: presenter.push(editText(AccountSettingsChange.LastName))) let displayNameRow = EditableTextRow( title: NSLocalizedString("Display Name", comment: "My Profile display name label"), value: settings?.displayName ?? "", - action: viewController.push(editText(AccountSettingsChange.DisplayName))) + action: presenter.push(editText(AccountSettingsChange.DisplayName))) let aboutMeRow = EditableTextRow( title: NSLocalizedString("About Me", comment: "My Profile 'About me' label"), value: settings?.aboutMe ?? "", - action: viewController.push(editText(AccountSettingsChange.AboutMe))) + action: presenter.push(editText(AccountSettingsChange.AboutMe))) return ImmuTable(sections: [ ImmuTableSection(rows: [ @@ -78,8 +89,10 @@ class MyProfileController: NSObject { ]) } + // MARK: - Actions + func editText(changeType: (AccountSettingsChangeWithString), hint: String? = nil) -> ImmuTableRowControllerGenerator { - return { [unowned self] row in + return { row in let row = row as! EditableTextRow return self.controllerForEditableText(row, changeType: changeType, hint: hint) } @@ -97,7 +110,6 @@ class MyProfileController: NSObject { controller.title = title controller.onValueChanged = { - [unowned self] value in let change = changeType(value) diff --git a/WordPress/Classes/ViewRelated/MeViewController.swift b/WordPress/Classes/ViewRelated/MeViewController.swift index 0ca3424bc759..6ffae8eb9671 100644 --- a/WordPress/Classes/ViewRelated/MeViewController.swift +++ b/WordPress/Classes/ViewRelated/MeViewController.swift @@ -187,8 +187,8 @@ class MeViewController: UITableViewController, UIViewControllerRestoration { } WPAppAnalytics.track(.OpenedMyProfile) - let controller = MyProfileController(account: account) - self.navigationController?.pushViewController(controller.viewController, animated: true) + let controller = MyProfileViewController(account: account) + self.navigationController?.pushViewController(controller, animated: true) } } From 47d92dbf09dce9e882353ebca4971b4525c6eb94 Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Wed, 27 Jan 2016 08:28:16 +0100 Subject: [PATCH 35/36] Add some more safeguards against a nil presenter --- .../ViewRelated/Me/MyProfileViewController.swift | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift index fb178b188f24..33d077233522 100644 --- a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift +++ b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift @@ -9,9 +9,14 @@ func MyProfileViewController(account account: WPAccount) -> ImmuTableViewControl func MyProfileViewController(service service: AccountSettingsService) -> ImmuTableViewController { let controller = MyProfileController(service: service) - return ImmuTableViewController(controller: controller) + let viewController = ImmuTableViewController(controller: controller) + assert(controller.presenter != nil, "ImmuTableViewController should have set the presenter for MyProfileController") + return viewController } +/// MyProfileController requires the `presenter` to be set before using. +/// To avoid problems, it's marked private and should only be initialized using the +/// `MyProfileViewController` factory functions. private struct MyProfileController: ImmuTableController { // MARK: - ImmuTableController @@ -24,11 +29,12 @@ private struct MyProfileController: ImmuTableController { } var immuTable: Observable { + precondition(presenter != nil, "presenter must be set before using") return service.settings.map(mapViewModel) } var errorMessage: Observable { - precondition(presenter != nil) + precondition(presenter != nil, "presenter must be set before using") guard let presenter = presenter else { // This shouldn't happen, but if it does, disabling the error feels // safer than having it running when the VC is not visible. @@ -53,7 +59,7 @@ private struct MyProfileController: ImmuTableController { // MARK: - Model mapping func mapViewModel(settings: AccountSettings?) -> ImmuTable { - precondition(presenter != nil) + precondition(presenter != nil, "presenter must be set before using") guard let presenter = presenter else { // This shouldn't happen. If there's no presenter we can't push the // editText controllers. From 3f20e45b8434c49f396689a03e18cb6c10a047cd Mon Sep 17 00:00:00 2001 From: Jorge Bernal Date: Wed, 27 Jan 2016 09:58:06 +0100 Subject: [PATCH 36/36] Fix presenter assertion Since controller has value semantics, our local `controller` wasn't changed so its presenter was nil. the viewController.controller is a copy of the local one, and it's the one that has (and needs to have) a presenter. --- WordPress/Classes/Utility/ImmuTableViewController.swift | 2 +- WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/Classes/Utility/ImmuTableViewController.swift b/WordPress/Classes/Utility/ImmuTableViewController.swift index 1e3f6b970f22..d66b345a2226 100644 --- a/WordPress/Classes/Utility/ImmuTableViewController.swift +++ b/WordPress/Classes/Utility/ImmuTableViewController.swift @@ -41,7 +41,7 @@ final class ImmuTableViewController: UITableViewController, ImmuTablePresenter { private var errorAnimator: ErrorAnimator! - private let controller: ImmuTableController? + let controller: ImmuTableController? private let bag = DisposeBag() diff --git a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift index 33d077233522..0dcfd7adad7d 100644 --- a/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift +++ b/WordPress/Classes/ViewRelated/Me/MyProfileViewController.swift @@ -10,7 +10,7 @@ func MyProfileViewController(account account: WPAccount) -> ImmuTableViewControl func MyProfileViewController(service service: AccountSettingsService) -> ImmuTableViewController { let controller = MyProfileController(service: service) let viewController = ImmuTableViewController(controller: controller) - assert(controller.presenter != nil, "ImmuTableViewController should have set the presenter for MyProfileController") + assert(viewController.controller?.presenter != nil, "ImmuTableViewController should have set the presenter for MyProfileController") return viewController }