From 9c14098b737397095420c4bd68cc39ebe1ce8778 Mon Sep 17 00:00:00 2001 From: Povilas Staskus Date: Thu, 26 Oct 2023 18:24:23 +0900 Subject: [PATCH 1/7] Extract guessXMLRPCURL error handling into a separate method --- .../SiteAddressViewController.swift | 64 ++++++++----------- 1 file changed, 27 insertions(+), 37 deletions(-) diff --git a/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift b/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift index 86d6d9d85..a0402a0af 100644 --- a/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift +++ b/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift @@ -511,46 +511,36 @@ private extension SiteAddressViewController { } let err = self.originalErrorOrError(error: error as NSError) + self.handleGuessXMLRPCURLError(error: err) + }) + } - let errorMessage: String? = { - if let xmlrpcValidatorError = err as? WordPressOrgXMLRPCValidatorError { - return xmlrpcValidatorError.localizedDescription - } else if (err.domain == NSURLErrorDomain && err.code == NSURLErrorCannotFindHost) || - (err.domain == NSURLErrorDomain && err.code == NSURLErrorNetworkConnectionLost) { - // NSURLErrorNetworkConnectionLost can be returned when an invalid URL is entered. - let msg = NSLocalizedString( - "The site at this address is not a WordPress site. For us to connect to it, the site must use WordPress.", - comment: "Error message shown a URL does not point to an existing site.") - return msg - } else { - return nil - } - }() - - /// Check if the host app wants to provide custom UI to handle the error. - /// If it does, insert the custom UI provided by the host app and exit early - if self.authenticationDelegate.shouldHandleError(err) { - - // Send the error to the host app - self.authenticationDelegate.handleError(err) { customUI in - self.pushCustomUI(customUI) - } - - // Track error message as failure - if let message = errorMessage { - self.tracker.track(failure: message) - } + private func handleGuessXMLRPCURLError(error: NSError) { + let errorMessage: String? + if let xmlrpcValidatorError = error as? WordPressOrgXMLRPCValidatorError { + errorMessage = xmlrpcValidatorError.localizedDescription + } else if (error.domain == NSURLErrorDomain && error.code == NSURLErrorCannotFindHost) || + (error.domain == NSURLErrorDomain && error.code == NSURLErrorNetworkConnectionLost) { + errorMessage = NSLocalizedString("The site at this address is not a WordPress site. For us to connect to it, the site must use WordPress.", comment: "Error message shown when a URL does not point to an existing site.") + } else { + errorMessage = nil + } - // Return as the error has been handled by the host app. - return - } + if self.authenticationDelegate.shouldHandleError(error) { + self.authenticationDelegate.handleError(error) { customUI in + self.pushCustomUI(customUI) + } + if let message = errorMessage { + self.tracker.track(failure: message) + } + return + } - if let message = errorMessage { - self.displayError(message: message, moveVoiceOverFocus: true) - } else { - self.displayError(error as NSError, sourceTag: self.sourceTag) - } - }) + if let message = errorMessage { + self.displayError(message: message, moveVoiceOverFocus: true) + } else { + self.displayError(error as NSError, sourceTag: self.sourceTag) + } } func fetchSiteInfo() { From b62db739cd3f6d6b20e50947ccc6821c8309e32b Mon Sep 17 00:00:00 2001 From: Povilas Staskus Date: Fri, 27 Oct 2023 18:11:54 +0900 Subject: [PATCH 2/7] Move guessXMLRPCURL method to SiteAddressViewModel Split presentation and business logic to allow for easier testing before making any changes --- .../project.pbxproj | 8 ++ .../SiteAddressViewController.swift | 81 +++++--------- .../Site Address/SiteAddressViewModel.swift | 102 ++++++++++++++++++ 3 files changed, 135 insertions(+), 56 deletions(-) create mode 100644 WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift diff --git a/WordPressAuthenticator.xcodeproj/project.pbxproj b/WordPressAuthenticator.xcodeproj/project.pbxproj index ddd3aaa38..6c4d1495f 100644 --- a/WordPressAuthenticator.xcodeproj/project.pbxproj +++ b/WordPressAuthenticator.xcodeproj/project.pbxproj @@ -7,6 +7,8 @@ objects = { /* Begin PBXBuildFile section */ + 01720C132AEB5013006713DF /* SiteAddressViewModelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 01720C122AEB5013006713DF /* SiteAddressViewModelTests.swift */; }; + 01720C152AEB5101006713DF /* SiteAddressViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 01720C142AEB5101006713DF /* SiteAddressViewModel.swift */; }; 0193F7752A615521004D7C16 /* MemoryManagementTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0193F7742A615521004D7C16 /* MemoryManagementTests.swift */; }; 020BE74A23B0BD2E007FE54C /* WordPressAuthenticatorDisplayImages.swift in Sources */ = {isa = PBXBuildFile; fileRef = 020BE74923B0BD2E007FE54C /* WordPressAuthenticatorDisplayImages.swift */; }; 020DEF6428AA091100C85D51 /* MagicLinkRequester.swift in Sources */ = {isa = PBXBuildFile; fileRef = 020DEF6328AA091100C85D51 /* MagicLinkRequester.swift */; }; @@ -264,6 +266,8 @@ /* End PBXCopyFilesBuildPhase section */ /* Begin PBXFileReference section */ + 01720C122AEB5013006713DF /* SiteAddressViewModelTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SiteAddressViewModelTests.swift; sourceTree = ""; }; + 01720C142AEB5101006713DF /* SiteAddressViewModel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SiteAddressViewModel.swift; sourceTree = ""; }; 0193F7742A615521004D7C16 /* MemoryManagementTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MemoryManagementTests.swift; sourceTree = ""; }; 020BE74923B0BD2E007FE54C /* WordPressAuthenticatorDisplayImages.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WordPressAuthenticatorDisplayImages.swift; sourceTree = ""; }; 020DEF6328AA091100C85D51 /* MagicLinkRequester.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MagicLinkRequester.swift; sourceTree = ""; }; @@ -567,6 +571,7 @@ 3F86A83F29D280DC005D20C0 /* SingIn */ = { isa = PBXGroup; children = ( + 01720C122AEB5013006713DF /* SiteAddressViewModelTests.swift */, 3F86A83D29D280D7005D20C0 /* AppleAuthenticatorTests.swift */, 3F86A84929D2A982005D20C0 /* LoginViewControllerTests.swift */, ); @@ -1082,6 +1087,7 @@ CEC77C6524854F2E00FB9050 /* SiteAddressViewController.swift */, CEC77C6724854F3E00FB9050 /* SiteAddress.storyboard */, CE73475524B77A3800A22660 /* SiteCredentialsViewController.swift */, + 01720C142AEB5101006713DF /* SiteAddressViewModel.swift */, ); path = "Site Address"; sourceTree = ""; @@ -1536,6 +1542,7 @@ 3F86A84229D28473005D20C0 /* SocialUserCreating.swift in Sources */, 3F9439BE27D6F9B60067183A /* LoginPrologueViewController.swift in Sources */, B560913B208A563800399AE4 /* LoginSelfHostedViewController.swift in Sources */, + 01720C152AEB5101006713DF /* SiteAddressViewModel.swift in Sources */, B5609136208A563800399AE4 /* Login2FAViewController.swift in Sources */, B56090E1208A4F9D00399AE4 /* WPWalkthroughTextField.m in Sources */, B56090EF208A527000399AE4 /* WPStyleGuide+Login.swift in Sources */, @@ -1564,6 +1571,7 @@ BA53D64824DFDF97001F1ABF /* WordPressSourceTagTests.swift in Sources */, 3F30A6BA299F12E30004452F /* Character+URLSafeTests.swift in Sources */, 4A1DEF4B29341B1F00322608 /* LoggingTests.swift in Sources */, + 01720C132AEB5013006713DF /* SiteAddressViewModelTests.swift in Sources */, 3F86A83E29D280D7005D20C0 /* AppleAuthenticatorTests.swift in Sources */, 3F3694022991E244006E923E /* JSONWebToken+Fixtures.swift in Sources */, 3F879FDF293A501D005C2B48 /* URLRequest+GoogleSignInTests.swift in Sources */, diff --git a/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift b/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift index a0402a0af..4d423a90b 100644 --- a/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift +++ b/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift @@ -32,6 +32,14 @@ final class SiteAddressViewController: LoginViewController { /// private let isSiteDiscovery: Bool private let configuration = WordPressAuthenticator.shared.configuration + private lazy var viewModel: SiteAddressViewModel = { + return SiteAddressViewModel( + isSiteDiscovery: isSiteDiscovery, + xmlrpcFacade: WordPressXMLRPCAPIFacade(), + authenticationDelegate: authenticationDelegate, + loginFields: loginFields + ) + }() init?(isSiteDiscovery: Bool, coder: NSCoder) { self.isSiteDiscovery = isSiteDiscovery @@ -482,64 +490,25 @@ private extension SiteAddressViewController { } func guessXMLRPCURL(for siteAddress: String) { - let facade = WordPressXMLRPCAPIFacade() - facade.guessXMLRPCURL(forSite: siteAddress, success: { [weak self] (url) in - // Success! We now know that we have a valid XML-RPC endpoint. - // At this point, we do NOT know if this is a WP.com site or a self-hosted site. - if let url = url { - self?.loginFields.meta.xmlrpcURL = url as NSURL - } - // Let's try to grab site info in preparation for the next screen. - self?.fetchSiteInfo() - - }, failure: { [weak self] (error) in - guard let error = error, let self = self else { - return - } - // Intentionally log the attempted address on failures. - // It's not guaranteed to be included in the error object depending on the error. - WPAuthenticatorLogInfo("Error attempting to connect to site address: \(self.loginFields.siteAddress)") - WPAuthenticatorLogError(error.localizedDescription) - - self.tracker.track(failure: .loginFailedToGuessXMLRPC) - - self.configureViewLoading(false) - - guard self.isSiteDiscovery == false else { - WordPressAuthenticator.shared.delegate?.troubleshootSite(nil, in: self.navigationController) - return + viewModel.guessXMLRPCURL(for: siteAddress) { [weak self] result -> Void in + guard let self else { return } + switch result { + case .success: + // Let's try to grab site info in preparation for the next screen. + self.fetchSiteInfo() + case .error(let error, let errorMessage): + if let message = errorMessage { + self.displayError(message: message, moveVoiceOverFocus: true) + } else { + self.displayError(error as NSError, sourceTag: self.sourceTag) } - - let err = self.originalErrorOrError(error: error as NSError) - self.handleGuessXMLRPCURLError(error: err) - }) - } - - private func handleGuessXMLRPCURLError(error: NSError) { - let errorMessage: String? - if let xmlrpcValidatorError = error as? WordPressOrgXMLRPCValidatorError { - errorMessage = xmlrpcValidatorError.localizedDescription - } else if (error.domain == NSURLErrorDomain && error.code == NSURLErrorCannotFindHost) || - (error.domain == NSURLErrorDomain && error.code == NSURLErrorNetworkConnectionLost) { - errorMessage = NSLocalizedString("The site at this address is not a WordPress site. For us to connect to it, the site must use WordPress.", comment: "Error message shown when a URL does not point to an existing site.") - } else { - errorMessage = nil - } - - if self.authenticationDelegate.shouldHandleError(error) { - self.authenticationDelegate.handleError(error) { customUI in - self.pushCustomUI(customUI) - } - if let message = errorMessage { - self.tracker.track(failure: message) + case .loading(let loading): + self.configureViewLoading(loading) + case .troubleshootSite: + WordPressAuthenticator.shared.delegate?.troubleshootSite(nil, in: self.navigationController) + case .customUI(let viewController): + self.pushCustomUI(viewController) } - return - } - - if let message = errorMessage { - self.displayError(message: message, moveVoiceOverFocus: true) - } else { - self.displayError(error as NSError, sourceTag: self.sourceTag) } } diff --git a/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift b/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift new file mode 100644 index 000000000..cb45213c6 --- /dev/null +++ b/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift @@ -0,0 +1,102 @@ +import Foundation +import WordPressKit + +struct SiteAddressViewModel { + private let isSiteDiscovery: Bool + private let xmlrpcFacade: WordPressXMLRPCAPIFacade + private let tracker = AuthenticatorAnalyticsTracker.shared + private let authenticationDelegate: WordPressAuthenticatorDelegate + private var loginFields: LoginFields + + init(isSiteDiscovery: Bool, + xmlrpcFacade: WordPressXMLRPCAPIFacade, + authenticationDelegate: WordPressAuthenticatorDelegate, + loginFields: LoginFields + ) { + self.isSiteDiscovery = isSiteDiscovery + self.xmlrpcFacade = xmlrpcFacade + self.authenticationDelegate = authenticationDelegate + self.loginFields = loginFields + } + + enum GuessXMLRPCURLResult { + case success + case error(NSError, String?) + case troubleshootSite + case loading(Bool) + case customUI(UIViewController) + } + + func guessXMLRPCURL( + for siteAddress: String, + completion: @escaping (GuessXMLRPCURLResult) -> () + ) { + let facade = WordPressXMLRPCAPIFacade() + facade.guessXMLRPCURL(forSite: siteAddress, success: { url in + // Success! We now know that we have a valid XML-RPC endpoint. + // At this point, we do NOT know if this is a WP.com site or a self-hosted site. + if let url = url { + self.loginFields.meta.xmlrpcURL = url as NSURL + } + + completion(.success) + + }, failure: { error in + guard let error = error else { + return + } + // Intentionally log the attempted address on failures. + // It's not guaranteed to be included in the error object depending on the error. + WPAuthenticatorLogInfo("Error attempting to connect to site address: \(self.loginFields.siteAddress)") + WPAuthenticatorLogError(error.localizedDescription) + + self.tracker.track(failure: .loginFailedToGuessXMLRPC) + + + completion(.loading(false)) + + guard self.isSiteDiscovery == false else { + completion(.troubleshootSite) + return + } + + let err = self.originalErrorOrError(error: error as NSError) + self.handleGuessXMLRPCURLError(error: err, completion: completion) + }) + } + + private func handleGuessXMLRPCURLError( + error: NSError, + completion: @escaping (GuessXMLRPCURLResult) -> () + ) { + let errorMessage: String? + if let xmlrpcValidatorError = error as? WordPressOrgXMLRPCValidatorError { + errorMessage = xmlrpcValidatorError.localizedDescription + } else if (error.domain == NSURLErrorDomain && error.code == NSURLErrorCannotFindHost) || + (error.domain == NSURLErrorDomain && error.code == NSURLErrorNetworkConnectionLost) { + errorMessage = NSLocalizedString("The site at this address is not a WordPress site. For us to connect to it, the site must use WordPress.", comment: "Error message shown when a URL does not point to an existing site.") + } else { + errorMessage = nil + } + + if self.authenticationDelegate.shouldHandleError(error) { + self.authenticationDelegate.handleError(error) { customUI in + completion(.customUI(customUI)) + } + if let message = errorMessage { + self.tracker.track(failure: message) + } + return + } + + completion(.error(error, errorMessage)) + } + + private func originalErrorOrError(error: NSError) -> NSError { + guard let err = error.userInfo[XMLRPCOriginalErrorKey] as? NSError else { + return error + } + + return err + } +} From 76925ba83e7a6712b7412fc7d849bbb6de457ab8 Mon Sep 17 00:00:00 2001 From: Povilas Staskus Date: Fri, 27 Oct 2023 18:12:09 +0900 Subject: [PATCH 3/7] Added SiteAddressViewModelTests --- .../WordPressAuthenticatorDelegateSpy.swift | 3 +- .../SingIn/SiteAddressViewModelTests.swift | 70 +++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 WordPressAuthenticatorTests/SingIn/SiteAddressViewModelTests.swift diff --git a/WordPressAuthenticatorTests/Mocks/WordPressAuthenticatorDelegateSpy.swift b/WordPressAuthenticatorTests/Mocks/WordPressAuthenticatorDelegateSpy.swift index 6cce72e36..def97c2de 100644 --- a/WordPressAuthenticatorTests/Mocks/WordPressAuthenticatorDelegateSpy.swift +++ b/WordPressAuthenticatorTests/Mocks/WordPressAuthenticatorDelegateSpy.swift @@ -8,6 +8,7 @@ class WordPressAuthenticatorDelegateSpy: WordPressAuthenticatorDelegate { var showSupportNotificationIndicator: Bool = true var supportEnabled: Bool = true var allowWPComLogin: Bool = true + var shouldHandleError: Bool = false private(set) var presentSignupEpilogueCalled = false private(set) var socialUser: SocialUser? @@ -50,7 +51,7 @@ class WordPressAuthenticatorDelegateSpy: WordPressAuthenticatorDelegate { } func shouldHandleError(_ error: Error) -> Bool { - true + shouldHandleError } func handleError(_ error: Error, onCompletion: @escaping (UIViewController) -> Void) { diff --git a/WordPressAuthenticatorTests/SingIn/SiteAddressViewModelTests.swift b/WordPressAuthenticatorTests/SingIn/SiteAddressViewModelTests.swift new file mode 100644 index 000000000..51118b2fc --- /dev/null +++ b/WordPressAuthenticatorTests/SingIn/SiteAddressViewModelTests.swift @@ -0,0 +1,70 @@ +import XCTest +@testable import WordPressAuthenticator + +final class SiteAddressViewModelTests: XCTestCase { + override func setUpWithError() throws { + try super.setUpWithError() + + WordPressAuthenticator.initializeForTesting() + } + + func testGuessXMLRPCURLSuccess() { + let mockFacade = MockWordPressXMLRPCAPIFacade() + mockFacade.success = true + let viewModel = SiteAddressViewModel(isSiteDiscovery: false, xmlrpcFacade: mockFacade, authenticationDelegate: WordPressAuthenticatorDelegateSpy(), loginFields: LoginFields()) + viewModel.guessXMLRPCURL(for: "testsite.com") { result in + switch result { + case .success: + XCTAssertTrue(true) + default: + XCTFail("Unexpected result") + } + } + } + + func testGuessXMLRPCURLError() { + let mockFacade = MockWordPressXMLRPCAPIFacade() + mockFacade.success = false + mockFacade.error = NSError(domain: "Test", code: 999, userInfo: nil) + let viewModel = SiteAddressViewModel(isSiteDiscovery: false, xmlrpcFacade: mockFacade, authenticationDelegate: WordPressAuthenticatorDelegateSpy(), loginFields: LoginFields()) + viewModel.guessXMLRPCURL(for: "testsite.com") { result in + switch result { + case .error(let error, _): + XCTAssertEqual(error.code, 999) + default: + XCTFail("Unexpected result") + } + } + } + + func testGuessXMLRPCURLErrorHandledByDelegate() { + let mockFacade = MockWordPressXMLRPCAPIFacade() + mockFacade.success = false + mockFacade.error = NSError(domain: "Test", code: 999, userInfo: nil) + let mockDelegate = WordPressAuthenticatorDelegateSpy() + mockDelegate.shouldHandleError = true + let viewModel = SiteAddressViewModel(isSiteDiscovery: false, xmlrpcFacade: mockFacade, authenticationDelegate: mockDelegate, loginFields: LoginFields()) + viewModel.guessXMLRPCURL(for: "testsite.com") { result in + switch result { + case .customUI: + XCTAssertTrue(true) + default: + XCTFail("Unexpected result") + } + } + } +} + + +private class MockWordPressXMLRPCAPIFacade: WordPressXMLRPCAPIFacade { + var success: Bool = false + var error: NSError? + + override func guessXMLRPCURL(forSite siteAddress: String, success: @escaping (URL?) -> (), failure: @escaping (Error?) -> ()) { + if self.success { + success(URL(string: "https://successful.site")) + } else { + failure(self.error) + } + } +} From a27ac06167677a7303a4d84d633c0daa6b82fb45 Mon Sep 17 00:00:00 2001 From: Povilas Staskus Date: Fri, 27 Oct 2023 19:18:19 +0900 Subject: [PATCH 4/7] Check if site is WP after failing XMLRPC validation In rare cases, XMLRPC validation can fail for unexpected reasons. Make a fallback check to determine if site is WP site before concluding to the user that the site is not WordPress site --- .../SiteAddressViewController.swift | 42 +++--- .../Site Address/SiteAddressViewModel.swift | 86 ++++++++---- .../WordPressAuthenticatorDelegateSpy.swift | 4 +- .../SingIn/SiteAddressViewModelTests.swift | 129 +++++++++++++----- 4 files changed, 178 insertions(+), 83 deletions(-) diff --git a/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift b/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift index 4d423a90b..042267f89 100644 --- a/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift +++ b/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift @@ -37,6 +37,7 @@ final class SiteAddressViewController: LoginViewController { isSiteDiscovery: isSiteDiscovery, xmlrpcFacade: WordPressXMLRPCAPIFacade(), authenticationDelegate: authenticationDelegate, + blogService: WordPressComBlogService(), loginFields: loginFields ) }() @@ -490,26 +491,29 @@ private extension SiteAddressViewController { } func guessXMLRPCURL(for siteAddress: String) { - viewModel.guessXMLRPCURL(for: siteAddress) { [weak self] result -> Void in - guard let self else { return } - switch result { - case .success: - // Let's try to grab site info in preparation for the next screen. - self.fetchSiteInfo() - case .error(let error, let errorMessage): - if let message = errorMessage { - self.displayError(message: message, moveVoiceOverFocus: true) - } else { - self.displayError(error as NSError, sourceTag: self.sourceTag) + viewModel.guessXMLRPCURL( + for: siteAddress, + loading: { [weak self] isLoading in + self?.configureViewLoading(isLoading) + }, + completion: { [weak self] result -> Void in + guard let self else { return } + switch result { + case .success: + // Let's try to grab site info in preparation for the next screen. + self.fetchSiteInfo() + case .error(let error, let errorMessage): + if let message = errorMessage { + self.displayError(message: message, moveVoiceOverFocus: true) + } else { + self.displayError(error as NSError, sourceTag: self.sourceTag) + } + case .troubleshootSite: + WordPressAuthenticator.shared.delegate?.troubleshootSite(nil, in: self.navigationController) + case .customUI(let viewController): + self.pushCustomUI(viewController) } - case .loading(let loading): - self.configureViewLoading(loading) - case .troubleshootSite: - WordPressAuthenticator.shared.delegate?.troubleshootSite(nil, in: self.navigationController) - case .customUI(let viewController): - self.pushCustomUI(viewController) - } - } + }) } func fetchSiteInfo() { diff --git a/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift b/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift index cb45213c6..74b1001fe 100644 --- a/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift +++ b/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift @@ -4,35 +4,38 @@ import WordPressKit struct SiteAddressViewModel { private let isSiteDiscovery: Bool private let xmlrpcFacade: WordPressXMLRPCAPIFacade - private let tracker = AuthenticatorAnalyticsTracker.shared - private let authenticationDelegate: WordPressAuthenticatorDelegate + private unowned let authenticationDelegate: WordPressAuthenticatorDelegate + private let blogService: WordPressComBlogService private var loginFields: LoginFields + private let tracker = AuthenticatorAnalyticsTracker.shared + init(isSiteDiscovery: Bool, xmlrpcFacade: WordPressXMLRPCAPIFacade, authenticationDelegate: WordPressAuthenticatorDelegate, + blogService: WordPressComBlogService, loginFields: LoginFields ) { self.isSiteDiscovery = isSiteDiscovery self.xmlrpcFacade = xmlrpcFacade self.authenticationDelegate = authenticationDelegate + self.blogService = blogService self.loginFields = loginFields } - enum GuessXMLRPCURLResult { + enum GuessXMLRPCURLResult: Equatable { case success case error(NSError, String?) case troubleshootSite - case loading(Bool) case customUI(UIViewController) } func guessXMLRPCURL( for siteAddress: String, + loading: @escaping ((Bool) -> ()), completion: @escaping (GuessXMLRPCURLResult) -> () ) { - let facade = WordPressXMLRPCAPIFacade() - facade.guessXMLRPCURL(forSite: siteAddress, success: { url in + xmlrpcFacade.guessXMLRPCURL(forSite: siteAddress, success: { url in // Success! We now know that we have a valid XML-RPC endpoint. // At this point, we do NOT know if this is a WP.com site or a self-hosted site. if let url = url { @@ -52,8 +55,7 @@ struct SiteAddressViewModel { self.tracker.track(failure: .loginFailedToGuessXMLRPC) - - completion(.loading(false)) + loading(false) guard self.isSiteDiscovery == false else { completion(.troubleshootSite) @@ -61,35 +63,47 @@ struct SiteAddressViewModel { } let err = self.originalErrorOrError(error: error as NSError) - self.handleGuessXMLRPCURLError(error: err, completion: completion) + self.handleGuessXMLRPCURLError(error: err, loading: loading, completion: completion) }) } private func handleGuessXMLRPCURLError( error: NSError, + loading: @escaping ((Bool) -> ()), completion: @escaping (GuessXMLRPCURLResult) -> () ) { - let errorMessage: String? - if let xmlrpcValidatorError = error as? WordPressOrgXMLRPCValidatorError { - errorMessage = xmlrpcValidatorError.localizedDescription - } else if (error.domain == NSURLErrorDomain && error.code == NSURLErrorCannotFindHost) || - (error.domain == NSURLErrorDomain && error.code == NSURLErrorNetworkConnectionLost) { - errorMessage = NSLocalizedString("The site at this address is not a WordPress site. For us to connect to it, the site must use WordPress.", comment: "Error message shown when a URL does not point to an existing site.") - } else { - errorMessage = nil + var completion: (NSError, String?) -> Void = { error, errorMessage in + if self.authenticationDelegate.shouldHandleError(error) { + self.authenticationDelegate.handleError(error) { customUI in + completion(.customUI(customUI)) + } + if let message = errorMessage { + self.tracker.track(failure: message) + } + return + } + + completion(.error(error, errorMessage)) } - if self.authenticationDelegate.shouldHandleError(error) { - self.authenticationDelegate.handleError(error) { customUI in - completion(.customUI(customUI)) - } - if let message = errorMessage { - self.tracker.track(failure: message) + /// Confirm the site is not a WordPress site before describing it as an invalid WP site + if let xmlrpcValidatorError = error as? WordPressOrgXMLRPCValidatorError, xmlrpcValidatorError == .invalid { + loading(true) + isWPSite { isWP in + loading(false) + if isWP { + let error = WordPressOrgXMLRPCValidatorError.xmlrpc_missing + completion(error as NSError, error.localizedDescription) + } else { + completion(error, Strings.notWPSiteErrorMessage) + } } - return + } else if (error.domain == NSURLErrorDomain && error.code == NSURLErrorCannotFindHost) || + (error.domain == NSURLErrorDomain && error.code == NSURLErrorNetworkConnectionLost) { + completion(error, Strings.notWPSiteErrorMessage) + } else { + completion(error, (error as? WordPressOrgXMLRPCValidatorError)?.localizedDescription) } - - completion(.error(error, errorMessage)) } private func originalErrorOrError(error: NSError) -> NSError { @@ -100,3 +114,23 @@ struct SiteAddressViewModel { return err } } + +extension SiteAddressViewModel { + private func isWPSite(_ completion: @escaping (Bool) -> ()) { + let baseSiteUrl = WordPressAuthenticator.baseSiteURL(string: loginFields.siteAddress) + blogService.fetchUnauthenticatedSiteInfoForAddress( + for: baseSiteUrl, + success: { siteInfo in + completion(siteInfo.isWP) + }, + failure: { _ in + completion(false) + }) + } +} + +private extension SiteAddressViewModel { + struct Strings { + static let notWPSiteErrorMessage = NSLocalizedString("The site at this address is not a WordPress site. For us to connect to it, the site must use WordPress.", comment: "Error message shown when a URL does not point to an existing site.") + } +} diff --git a/WordPressAuthenticatorTests/Mocks/WordPressAuthenticatorDelegateSpy.swift b/WordPressAuthenticatorTests/Mocks/WordPressAuthenticatorDelegateSpy.swift index def97c2de..ff9ab9d51 100644 --- a/WordPressAuthenticatorTests/Mocks/WordPressAuthenticatorDelegateSpy.swift +++ b/WordPressAuthenticatorTests/Mocks/WordPressAuthenticatorDelegateSpy.swift @@ -55,7 +55,9 @@ class WordPressAuthenticatorDelegateSpy: WordPressAuthenticatorDelegate { } func handleError(_ error: Error, onCompletion: @escaping (UIViewController) -> Void) { - // no-op + if shouldHandleError { + onCompletion(UIViewController()) + } } func shouldPresentSignupEpilogue() -> Bool { diff --git a/WordPressAuthenticatorTests/SingIn/SiteAddressViewModelTests.swift b/WordPressAuthenticatorTests/SingIn/SiteAddressViewModelTests.swift index 51118b2fc..d74ba5017 100644 --- a/WordPressAuthenticatorTests/SingIn/SiteAddressViewModelTests.swift +++ b/WordPressAuthenticatorTests/SingIn/SiteAddressViewModelTests.swift @@ -1,61 +1,107 @@ import XCTest +import WordPressKit @testable import WordPressAuthenticator final class SiteAddressViewModelTests: XCTestCase { - override func setUpWithError() throws { - try super.setUpWithError() + private var isSiteDiscovery: Bool! + private var xmlrpcFacade: MockWordPressXMLRPCAPIFacade! + private var authenticationDelegateSpy: WordPressAuthenticatorDelegateSpy! + private var blogService: MockWordPressComBlogService! + private var loginFields: LoginFields! + private var viewModel: SiteAddressViewModel! + + override func setUp() { + super.setUp() + isSiteDiscovery = false + xmlrpcFacade = MockWordPressXMLRPCAPIFacade() + authenticationDelegateSpy = WordPressAuthenticatorDelegateSpy() + blogService = MockWordPressComBlogService() + loginFields = LoginFields() WordPressAuthenticator.initializeForTesting() + + viewModel = SiteAddressViewModel(isSiteDiscovery: isSiteDiscovery, xmlrpcFacade: xmlrpcFacade, authenticationDelegate: authenticationDelegateSpy, blogService: blogService, loginFields: loginFields) } func testGuessXMLRPCURLSuccess() { - let mockFacade = MockWordPressXMLRPCAPIFacade() - mockFacade.success = true - let viewModel = SiteAddressViewModel(isSiteDiscovery: false, xmlrpcFacade: mockFacade, authenticationDelegate: WordPressAuthenticatorDelegateSpy(), loginFields: LoginFields()) - viewModel.guessXMLRPCURL(for: "testsite.com") { result in - switch result { - case .success: - XCTAssertTrue(true) - default: - XCTFail("Unexpected result") - } + xmlrpcFacade.success = true + var result: SiteAddressViewModel.GuessXMLRPCURLResult? + viewModel.guessXMLRPCURL(for: "https://wordpress.com", loading: { _ in }) { res in + result = res } + + XCTAssertEqual(result, .success) } func testGuessXMLRPCURLError() { - let mockFacade = MockWordPressXMLRPCAPIFacade() - mockFacade.success = false - mockFacade.error = NSError(domain: "Test", code: 999, userInfo: nil) - let viewModel = SiteAddressViewModel(isSiteDiscovery: false, xmlrpcFacade: mockFacade, authenticationDelegate: WordPressAuthenticatorDelegateSpy(), loginFields: LoginFields()) - viewModel.guessXMLRPCURL(for: "testsite.com") { result in - switch result { - case .error(let error, _): - XCTAssertEqual(error.code, 999) - default: - XCTFail("Unexpected result") - } + xmlrpcFacade.error = NSError(domain: "SomeDomain", code: 1, userInfo: nil) + var result: SiteAddressViewModel.GuessXMLRPCURLResult? + viewModel.guessXMLRPCURL(for: "https://error.com", loading: { _ in }) { res in + result = res + } + if case .error(let error, _) = result { + XCTAssertEqual(error.code, 1) + } else { + XCTFail("Unexpected result: \(String(describing: result))") + } + } + + func testGuessXMLRPCURLErrorInvalidNotWP() { + xmlrpcFacade.error = WordPressOrgXMLRPCValidatorError.invalid as NSError + blogService.isWP = false + var result: SiteAddressViewModel.GuessXMLRPCURLResult? + viewModel.guessXMLRPCURL(for: "https://invalid.com", loading: { _ in }) { res in + result = res + } + + if case .error(let error, _) = result { + XCTAssertEqual(error.code, WordPressOrgXMLRPCValidatorError.invalid.rawValue) + } else { + XCTFail("Unexpected result: \(String(describing: result))") + } + } + + func testGuessXMLRPCURLErrorInvalidIsWP() { + xmlrpcFacade.error = WordPressOrgXMLRPCValidatorError.invalid as NSError + blogService.isWP = true + var result: SiteAddressViewModel.GuessXMLRPCURLResult? + viewModel.guessXMLRPCURL(for: "https://invalidwp.com", loading: { _ in }) { res in + result = res + } + if case .error(let error, _) = result { + XCTAssertEqual(error.code, WordPressOrgXMLRPCValidatorError.xmlrpc_missing.rawValue) + } else { + XCTFail("Unexpected result: \(String(describing: result))") } } + func testGuessXMLRPCTroubleshootSite() { + viewModel = SiteAddressViewModel(isSiteDiscovery: true, xmlrpcFacade: xmlrpcFacade, authenticationDelegate: authenticationDelegateSpy, blogService: blogService, loginFields: loginFields) + xmlrpcFacade.error = NSError(domain: "SomeDomain", code: 1, userInfo: nil) + var result: SiteAddressViewModel.GuessXMLRPCURLResult? + viewModel.guessXMLRPCURL(for: "https://troubleshoot.com", loading: { _ in }) { res in + result = res + } + XCTAssertEqual(result, .troubleshootSite) + } + func testGuessXMLRPCURLErrorHandledByDelegate() { - let mockFacade = MockWordPressXMLRPCAPIFacade() - mockFacade.success = false - mockFacade.error = NSError(domain: "Test", code: 999, userInfo: nil) - let mockDelegate = WordPressAuthenticatorDelegateSpy() - mockDelegate.shouldHandleError = true - let viewModel = SiteAddressViewModel(isSiteDiscovery: false, xmlrpcFacade: mockFacade, authenticationDelegate: mockDelegate, loginFields: LoginFields()) - viewModel.guessXMLRPCURL(for: "testsite.com") { result in - switch result { - case .customUI: - XCTAssertTrue(true) - default: - XCTFail("Unexpected result") - } + xmlrpcFacade.error = NSError(domain: "SomeDomain", code: 1, userInfo: nil) + authenticationDelegateSpy.shouldHandleError = true + + var result: SiteAddressViewModel.GuessXMLRPCURLResult? + viewModel.guessXMLRPCURL(for: "https://delegatehandles.com", loading: { _ in }) { res in + result = res + } + + if case .customUI = result { + XCTAssertTrue(true) + } else { + XCTFail("Unexpected result: \(String(describing: result))") } } } - private class MockWordPressXMLRPCAPIFacade: WordPressXMLRPCAPIFacade { var success: Bool = false var error: NSError? @@ -68,3 +114,12 @@ private class MockWordPressXMLRPCAPIFacade: WordPressXMLRPCAPIFacade { } } } + +private class MockWordPressComBlogService: WordPressComBlogService { + var isWP = false + + override func fetchUnauthenticatedSiteInfoForAddress(for address: String, success: @escaping (WordPressComSiteInfo) -> Void, failure: @escaping (Error) -> Void) { + let siteInfo = WordPressComSiteInfo(remote: ["isWordPress": isWP]) + success(siteInfo) + } +} From ce507ae8e1c10840acfd43cb86b9099b88c77fd3 Mon Sep 17 00:00:00 2001 From: Povilas Staskus Date: Mon, 13 Nov 2023 14:58:50 +0200 Subject: [PATCH 5/7] Update CHANGELOG.md --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 182c0ec8c..6bbeb8b76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,11 @@ _None._ _None._ +## 7.3.1 + +### Bug Fixes +- Fix an issue where self-hosted sites are incorrectly flagged as non WordPress sites. [#796] + ## 7.3.0 ### New Features From bd01cccfef90cf5448c81210f351747c1c1f4839 Mon Sep 17 00:00:00 2001 From: Povilas Staskus <4062343+staskus@users.noreply.github.com> Date: Wed, 20 Dec 2023 10:05:54 +0200 Subject: [PATCH 6/7] Use let instead of var --- .../View Related/Site Address/SiteAddressViewModel.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift b/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift index 74b1001fe..97e921e3d 100644 --- a/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift +++ b/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift @@ -72,7 +72,7 @@ struct SiteAddressViewModel { loading: @escaping ((Bool) -> ()), completion: @escaping (GuessXMLRPCURLResult) -> () ) { - var completion: (NSError, String?) -> Void = { error, errorMessage in + let completion: (NSError, String?) -> Void = { error, errorMessage in if self.authenticationDelegate.shouldHandleError(error) { self.authenticationDelegate.handleError(error) { customUI in completion(.customUI(customUI)) From fb783d46f5f9b6b875e21713a099d8e9b181a47b Mon Sep 17 00:00:00 2001 From: Povilas Staskus <4062343+staskus@users.noreply.github.com> Date: Wed, 20 Dec 2023 10:17:10 +0200 Subject: [PATCH 7/7] Fix identation --- .../Site Address/SiteAddressViewModel.swift | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift b/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift index 97e921e3d..59c2f46be 100644 --- a/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift +++ b/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewModel.swift @@ -44,26 +44,26 @@ struct SiteAddressViewModel { completion(.success) - }, failure: { error in - guard let error = error else { - return - } - // Intentionally log the attempted address on failures. - // It's not guaranteed to be included in the error object depending on the error. - WPAuthenticatorLogInfo("Error attempting to connect to site address: \(self.loginFields.siteAddress)") - WPAuthenticatorLogError(error.localizedDescription) + }, failure: { error in + guard let error = error else { + return + } + // Intentionally log the attempted address on failures. + // It's not guaranteed to be included in the error object depending on the error. + WPAuthenticatorLogInfo("Error attempting to connect to site address: \(self.loginFields.siteAddress)") + WPAuthenticatorLogError(error.localizedDescription) - self.tracker.track(failure: .loginFailedToGuessXMLRPC) + self.tracker.track(failure: .loginFailedToGuessXMLRPC) - loading(false) + loading(false) - guard self.isSiteDiscovery == false else { - completion(.troubleshootSite) - return - } + guard self.isSiteDiscovery == false else { + completion(.troubleshootSite) + return + } - let err = self.originalErrorOrError(error: error as NSError) - self.handleGuessXMLRPCURLError(error: err, loading: loading, completion: completion) + let err = self.originalErrorOrError(error: error as NSError) + self.handleGuessXMLRPCURLError(error: err, loading: loading, completion: completion) }) } @@ -88,7 +88,7 @@ struct SiteAddressViewModel { /// Confirm the site is not a WordPress site before describing it as an invalid WP site if let xmlrpcValidatorError = error as? WordPressOrgXMLRPCValidatorError, xmlrpcValidatorError == .invalid { - loading(true) + loading(true) isWPSite { isWP in loading(false) if isWP { @@ -125,7 +125,7 @@ extension SiteAddressViewModel { }, failure: { _ in completion(false) - }) + }) } }