Merge pull request #1362 from kielgillard/master

Feedly operations can optionally report their progress. #1328

Frameworks/Account/AccountTests/Feedly/FeedlyOperationTests.swift

@@ -8,6 +8,7 @@
 
 import XCTest
 @testable import Account
+import RSWeb
 
 class FeedlyOperationTests: XCTestCase {
 	
@@ -144,4 +145,74 @@ class FeedlyOperationTests: XCTestCase {
 		waitForExpectations(timeout: 2)
     }
 	
+	func testProgressReporting() {
+		let progress = DownloadProgress(numberOfTasks: 0)
+        let testOperation = TestOperation()
+		
+		testOperation.downloadProgress = progress
+		XCTAssertTrue(progress.numberRemaining == 1)
+		
+		testOperation.downloadProgress = nil
+		XCTAssertTrue(progress.numberRemaining == 0)
+	}
+	
+	func testProgressReportingOnCancel() {
+		let progress = DownloadProgress(numberOfTasks: 0)
+		let testOperation = TestOperation()
+		testOperation.downloadProgress = progress
+		
+		let completionExpectation = expectation(description: "Operation Completed")
+		testOperation.completionBlock = {
+			completionExpectation.fulfill()
+		}
+		
+		OperationQueue.main.addOperation(testOperation)
+		
+		XCTAssertTrue(progress.numberRemaining == 1)
+		testOperation.cancel()
+		XCTAssertTrue(progress.numberRemaining == 1)
+		
+		waitForExpectations(timeout: 2)
+		
+		XCTAssertTrue(progress.numberRemaining == 0)
+    }
+	
+	func testDoesProgressReportingOnSuccess() {
+		let progress = DownloadProgress(numberOfTasks: 0)
+		let testOperation = TestOperation()
+		testOperation.downloadProgress = progress
+		
+		let completionExpectation = expectation(description: "Operation Completed")
+		testOperation.completionBlock = {
+			completionExpectation.fulfill()
+		}
+		
+		OperationQueue.main.addOperation(testOperation)
+		
+		XCTAssertTrue(progress.numberRemaining == 1)
+		
+		waitForExpectations(timeout: 2)
+		
+		XCTAssertTrue(progress.numberRemaining == 0)
+    }
+	
+	func testProgressReportingOnFailure() {
+		let progress = DownloadProgress(numberOfTasks: 0)
+		let testOperation = TestOperation()
+		testOperation.mockError = TestOperationError.mockError
+		testOperation.downloadProgress = progress
+		
+		let completionExpectation = expectation(description: "Operation Completed")
+		testOperation.completionBlock = {
+			completionExpectation.fulfill()
+		}
+		
+		OperationQueue.main.addOperation(testOperation)
+		
+		XCTAssertTrue(progress.numberRemaining == 1)
+		
+		waitForExpectations(timeout: 2)
+		
+		XCTAssertTrue(progress.numberRemaining == 0)
+    }
 }

Frameworks/Account/AccountTests/Feedly/FeedlySyncAllOperationTests.swift

@@ -48,6 +48,7 @@ class FeedlySyncAllOperationTests: XCTestCase {
 		getStarredContents.getStreamContentsExpectation = expectation(description: "Get Contents of global.saved")
 		getStarredContents.getStreamContentsExpectation?.isInverted = true
 		
+		let progress = DownloadProgress(numberOfTasks: 0)
 		let container = support.makeTestDatabaseContainer()
 		let syncAll = FeedlySyncAllOperation(account: account,
 											 credentials: support.accessToken,
@@ -58,6 +59,7 @@ class FeedlySyncAllOperationTests: XCTestCase {
 											 getStreamContentsService: getGlobalStreamContents,
 											 getStarredArticlesService: getStarredContents,
 											 database: container.database,
+											 downloadProgress: progress,
 											 log: support.log)
 		
 		// If this expectation is not fulfilled, the operation is not calling `didFinish`.
@@ -81,6 +83,9 @@ class FeedlySyncAllOperationTests: XCTestCase {
 		
 		OperationQueue.main.addOperation(syncAll)
 		
+		XCTAssertTrue(progress.numberOfTasks > 1)
+		
+		// This operation cancels asynchronously, so it is hard to test the number of tasks == 0
 		syncAll.cancel()
 		
 		waitForExpectations(timeout: 2)
@@ -115,6 +120,7 @@ class FeedlySyncAllOperationTests: XCTestCase {
 		let provider = FeedlyMockResponseProvider(findingMocksIn: subdirectory)
 		transport.mockResponseFileUrlProvider = provider
 		
+		let progress = DownloadProgress(numberOfTasks: 0)
 		// lastSuccessfulFetchStartDate does not matter for the test, content will always be the same.
 		// It is tested in `FeedlyGetStreamContentsOperationTests`.
 		let syncAll = FeedlySyncAllOperation(account: account,
@@ -122,6 +128,7 @@ class FeedlySyncAllOperationTests: XCTestCase {
 											 caller: caller,
 											 database: databaseContainer.database,
 											 lastSuccessfulFetchStartDate: nil,
+											 downloadProgress: progress,
 											 log: support.log)
 		
 		// If this expectation is not fulfilled, the operation is not calling `didFinish`.
@@ -132,7 +139,11 @@ class FeedlySyncAllOperationTests: XCTestCase {
 				
 		OperationQueue.main.addOperation(syncAll)
 		
+		XCTAssertTrue(progress.numberOfTasks > 1)
+		
 		waitForExpectations(timeout: 5)
+		
+		XCTAssertTrue(progress.numberOfTasks == 0)
 	}
 	
 	func performInitialSync() {
@@ -226,7 +237,8 @@ class FeedlySyncAllOperationTests: XCTestCase {
 		let credentials = Credentials(type: .oauthAccessToken, username: "<#USERNAME#>", secret: "<#SECRET#>")
 		caller.credentials = credentials
 		
-		let syncAll = FeedlySyncAllOperation(account: account, credentials: credentials, caller: caller, database: databaseContainer.database, lastSuccessfulFetchStartDate: lastSuccessfulFetchStartDate, log: support.log)
+		let progress = DownloadProgress(numberOfTasks: 0)
+		let syncAll = FeedlySyncAllOperation(account: account, credentials: credentials, caller: caller, database: databaseContainer.database, lastSuccessfulFetchStartDate: lastSuccessfulFetchStartDate, downloadProgress: progress, log: support.log)
 		
 		// If this expectation is not fulfilled, the operation is not calling `didFinish`.
 		let completionExpectation = expectation(description: "Did Finish")
@@ -238,7 +250,11 @@ class FeedlySyncAllOperationTests: XCTestCase {
 		
 		OperationQueue.main.addOperation(syncAll)
 		
+		XCTAssertTrue(progress.numberOfTasks > 1)
+		
 		waitForExpectations(timeout: 60)
+		
+		XCTAssertTrue(progress.numberOfTasks == 0)
 	}
 	
 	// Prefix with "test" to manually run this particular function, e.g.: func test_getTestData()

Frameworks/Account/Feedly/FeedlyAPICaller.swift

@@ -196,7 +196,7 @@ final class FeedlyAPICaller {
 		}
 		guard let encodedId = encodeForURLPath(id) else {
 			return DispatchQueue.main.async {
-				completionHandler(.failure(FeedbinAccountDelegateError.invalidParameter))
+				completionHandler(.failure(FeedlyAccountDelegateError.unexpectedResourceId(id)))
 			}
 		}
 		var components = baseUrlComponents
@@ -235,7 +235,7 @@ final class FeedlyAPICaller {
 
 		guard let encodedId = encodeForURLPath(collectionId) else {
 			return DispatchQueue.main.async {
-				completionHandler(.failure(FeedbinAccountDelegateError.invalidParameter))
+				completionHandler(.failure(FeedlyAccountDelegateError.unexpectedResourceId(collectionId)))
 			}
 		}
 		var components = baseUrlComponents
@@ -286,11 +286,18 @@ final class FeedlyAPICaller {
 			}
 		}
 
-		guard let encodedCollectionId = encodeForURLPath(collectionId), let encodedFeedId = encodeForURLPath(feedId) else {
+		guard let encodedCollectionId = encodeForURLPath(collectionId) else {
 			return DispatchQueue.main.async {
-				completionHandler(.failure(FeedbinAccountDelegateError.invalidParameter))
+				completionHandler(.failure(FeedlyAccountDelegateError.unexpectedResourceId(collectionId)))
 			}
 		}
+		
+		guard let encodedFeedId = encodeForURLPath(feedId) else {
+			return DispatchQueue.main.async {
+				completionHandler(.failure(FeedlyAccountDelegateError.unexpectedResourceId(feedId)))
+			}
+		}
+		
 		var components = baseUrlComponents
 		components.percentEncodedPath = "/v3/collections/\(encodedCollectionId)/feeds/\(encodedFeedId)"
 		

Frameworks/Account/Feedly/FeedlyAccountDelegate.swift

@@ -117,11 +117,9 @@ final class FeedlyAccountDelegate: AccountDelegate {
 		}
 		
 		let log = self.log
-		let progress = refreshProgress
+		let operation = FeedlySyncAllOperation(account: account, credentials: credentials, caller: caller, database: database, lastSuccessfulFetchStartDate: accountMetadata?.lastArticleFetch, downloadProgress: refreshProgress, log: log)
-		progress.addToNumberOfTasksAndRemaining(1)
-		
-		let operation = FeedlySyncAllOperation(account: account, credentials: credentials, caller: caller, database: database, lastSuccessfulFetchStartDate: accountMetadata?.lastArticleFetch, log: log)
 		
+		operation.downloadProgress = refreshProgress
 		let date = Date()
 		operation.syncCompletionHandler = { [weak self] result in
 			if case .success = result {
@@ -129,7 +127,6 @@ final class FeedlyAccountDelegate: AccountDelegate {
 			}
 			
 			os_log(.debug, log: log, "Sync took %{public}.3f seconds", -date.timeIntervalSinceNow)
-			progress.completeTask()
 			completion(result)
 		}
 		

Frameworks/Account/Feedly/FeedlyAccountDelegateError.swift

@@ -10,6 +10,7 @@ import Foundation
 
 enum FeedlyAccountDelegateError: LocalizedError {
 	case notLoggedIn
+	case unexpectedResourceId(String)
 	case unableToAddFolder(String)
 	case unableToRenameFolder(String, String)
 	case unableToRemoveFolder(String)
@@ -24,6 +25,10 @@ enum FeedlyAccountDelegateError: LocalizedError {
 		case .notLoggedIn:
 			return NSLocalizedString("Please add the Feedly account again.", comment: "Feedly – Credentials not found.")
 			
+		case .unexpectedResourceId(let resourceId):
+			let template = NSLocalizedString("Could not encode the identifier “%@”.", comment: "Feedly – Could not encode resource id to send to Feedly.")
+			return String(format: template, resourceId)
+			
 		case .unableToAddFolder(let name):
 			let template = NSLocalizedString("Could not create a folder named “%@”.", comment: "Feedly – Could not create a folder/collection.")
 			return String(format: template, name)
@@ -62,6 +67,10 @@ enum FeedlyAccountDelegateError: LocalizedError {
 		case .notLoggedIn:
 			return nil
 			
+		case .unexpectedResourceId:
+			let template = NSLocalizedString("Please contact NetNewsWire support.", comment: "Feedly – Recovery suggestion for not being able to encode a resource id to send to Feedly..")
+			return String(format: template)
+			
 		case .unableToAddFolder:
 			return nil
 			

Frameworks/Account/Feedly/Operations/FeedlyOperation.swift

@@ -7,6 +7,7 @@
 //
 
 import Foundation
+import RSWeb
 
 protocol FeedlyOperationDelegate: class {
 	func feedlyOperation(_ operation: FeedlyOperation, didFailWith error: Error)
@@ -18,9 +19,22 @@ class FeedlyOperation: Operation {
 	
 	weak var delegate: FeedlyOperationDelegate?
 	
+	var downloadProgress: DownloadProgress? {
+		didSet {
+			guard downloadProgress == nil || !isExecuting else {
+				fatalError("\(\FeedlyOperation.downloadProgress) was set to late. Set before operation starts executing.")
+			}
+			oldValue?.completeTask()
+			downloadProgress?.addToNumberOfTasksAndRemaining(1)
+		}
+	}
+	
 	func didFinish() {
 		assert(Thread.isMainThread)
 		assert(!isFinished, "Finished operation is attempting to finish again.")
+		
+		downloadProgress = nil
+		
 		isExecutingOperation = false
 		isFinishedOperation = true
 	}
@@ -36,6 +50,13 @@ class FeedlyOperation: Operation {
 		guard !isCancelled else {
 			isExecutingOperation = false
 			isFinishedOperation = true
+			
+			if downloadProgress != nil {
+				DispatchQueue.main.async {
+					self.downloadProgress = nil
+				}
+			}
+			
 			return
 		}
 		

Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift

@@ -9,6 +9,7 @@
 import Foundation
 import os.log
 import SyncDatabase
+import RSWeb
 
 /// Single responsibility is to compose the operations necessary to get the entire set of articles, feeds and folders with the statuses the user expects between now and a certain date in the past.
 final class FeedlySyncAllOperation: FeedlyOperation {
@@ -18,7 +19,7 @@ final class FeedlySyncAllOperation: FeedlyOperation {
 	
 	var syncCompletionHandler: ((Result<Void, Error>) -> ())?
 	
-	init(account: Account, credentials: Credentials, lastSuccessfulFetchStartDate: Date?, markArticlesService: FeedlyMarkArticlesService, getUnreadService: FeedlyGetStreamIdsService, getCollectionsService: FeedlyGetCollectionsService, getStreamContentsService: FeedlyGetStreamContentsService, getStarredArticlesService: FeedlyGetStreamContentsService, database: SyncDatabase, log: OSLog) {
+	init(account: Account, credentials: Credentials, lastSuccessfulFetchStartDate: Date?, markArticlesService: FeedlyMarkArticlesService, getUnreadService: FeedlyGetStreamIdsService, getCollectionsService: FeedlyGetCollectionsService, getStreamContentsService: FeedlyGetStreamContentsService, getStarredArticlesService: FeedlyGetStreamContentsService, database: SyncDatabase, downloadProgress: DownloadProgress, log: OSLog) {
 		self.syncUUID = UUID()
 		self.log = log
 		self.operationQueue = OperationQueue()
@@ -26,20 +27,25 @@ final class FeedlySyncAllOperation: FeedlyOperation {
 		
 		super.init()
 		
+		self.downloadProgress = downloadProgress
+		
 		// Send any read/unread/starred article statuses to Feedly before anything else.
 		let sendArticleStatuses = FeedlySendArticleStatusesOperation(database: database, service: markArticlesService, log: log)
 		sendArticleStatuses.delegate = self
+		sendArticleStatuses.downloadProgress = downloadProgress
 		self.operationQueue.addOperation(sendArticleStatuses)
 		
 		// Get each page of unread article ids in the global.all stream for the last 31 days (nil = Feedly API default).
 		let getUnread = FeedlySyncUnreadStatusesOperation(account: account, credentials: credentials, service: getUnreadService, newerThan: nil, log: log)
 		getUnread.delegate = self
 		getUnread.addDependency(sendArticleStatuses)
+		getUnread.downloadProgress = downloadProgress
 		self.operationQueue.addOperation(getUnread)
 		
 		// Get all the Collections the user has.
 		let getCollections = FeedlyGetCollectionsOperation(service: getCollectionsService, log: log)
 		getCollections.delegate = self
+		getCollections.downloadProgress = downloadProgress
 		getCollections.addDependency(sendArticleStatuses)
 		self.operationQueue.addOperation(getCollections)
 		
@@ -58,6 +64,7 @@ final class FeedlySyncAllOperation: FeedlyOperation {
 		// Get each page of the global.all stream until we get either the content from the last sync or the last 31 days.
 		let getStreamContents = FeedlySyncStreamContentsOperation(account: account, credentials: credentials, service: getStreamContentsService, newerThan: lastSuccessfulFetchStartDate, log: log)
 		getStreamContents.delegate = self
+		getStreamContents.downloadProgress = downloadProgress
 		getStreamContents.addDependency(getCollections)
 		getStreamContents.addDependency(getUnread)
 		getStreamContents.addDependency(createFeedsOperation)
@@ -65,19 +72,21 @@ final class FeedlySyncAllOperation: FeedlyOperation {
 		
 		// Get each and every starred article.
 		let syncStarred = FeedlySyncStarredArticlesOperation(account: account, credentials: credentials, service: getStarredArticlesService, log: log)
+		syncStarred.downloadProgress = downloadProgress
 		syncStarred.addDependency(createFeedsOperation)
 		self.operationQueue.addOperation(syncStarred)
 		
 		// Once this operation's dependencies, their dependencies etc finish, we can finish.
 		let finishOperation = FeedlyCheckpointOperation()
 		finishOperation.checkpointDelegate = self
+		finishOperation.downloadProgress = downloadProgress
 		finishOperation.addDependency(getStreamContents)
 		finishOperation.addDependency(syncStarred)
 
 		self.operationQueue.addOperation(finishOperation)
 	}
 	
-	convenience init(account: Account, credentials: Credentials, caller: FeedlyAPICaller, database: SyncDatabase, lastSuccessfulFetchStartDate: Date?, log: OSLog) {
+	convenience init(account: Account, credentials: Credentials, caller: FeedlyAPICaller, database: SyncDatabase, lastSuccessfulFetchStartDate: Date?, downloadProgress: DownloadProgress, log: OSLog) {
 		
 		let newerThan: Date? = {
 			if let date = lastSuccessfulFetchStartDate {
@@ -87,7 +96,7 @@ final class FeedlySyncAllOperation: FeedlyOperation {
 			}
 		}()
 		
-		self.init(account: account, credentials: credentials, lastSuccessfulFetchStartDate: newerThan, markArticlesService: caller, getUnreadService: caller, getCollectionsService: caller, getStreamContentsService: caller, getStarredArticlesService: caller, database: database, log: log)
+		self.init(account: account, credentials: credentials, lastSuccessfulFetchStartDate: newerThan, markArticlesService: caller, getUnreadService: caller, getCollectionsService: caller, getStreamContentsService: caller, getStarredArticlesService: caller, database: database, downloadProgress: downloadProgress, log: log)
 	}
 	
 	override func cancel() {