From 2f96e8b8a6583e6149707e61fdd025c2ab763849 Mon Sep 17 00:00:00 2001 From: Kiel Gillard Date: Mon, 25 Nov 2019 18:19:33 +1100 Subject: [PATCH] Feedly operations can optionally report their progress. #1328 --- .../Feedly/FeedlyOperationTests.swift | 73 ++++++++++++++++++- .../Feedly/FeedlySyncAllOperationTests.swift | 20 ++++- .../Account/Feedly/FeedlyAPICaller.swift | 15 +++- .../Feedly/FeedlyAccountDelegate.swift | 7 +- .../Feedly/FeedlyAccountDelegateError.swift | 9 +++ .../Feedly/Operations/FeedlyOperation.swift | 21 ++++++ .../Operations/FeedlySyncAllOperation.swift | 15 +++- 7 files changed, 145 insertions(+), 15 deletions(-) diff --git a/Frameworks/Account/AccountTests/Feedly/FeedlyOperationTests.swift b/Frameworks/Account/AccountTests/Feedly/FeedlyOperationTests.swift index 12992d748..640693916 100644 --- a/Frameworks/Account/AccountTests/Feedly/FeedlyOperationTests.swift +++ b/Frameworks/Account/AccountTests/Feedly/FeedlyOperationTests.swift @@ -8,6 +8,7 @@ import XCTest @testable import Account +import RSWeb class FeedlyOperationTests: XCTestCase { @@ -143,5 +144,75 @@ 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) + } } diff --git a/Frameworks/Account/AccountTests/Feedly/FeedlySyncAllOperationTests.swift b/Frameworks/Account/AccountTests/Feedly/FeedlySyncAllOperationTests.swift index 5c7869439..4ca8c1bd3 100644 --- a/Frameworks/Account/AccountTests/Feedly/FeedlySyncAllOperationTests.swift +++ b/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() { @@ -225,8 +236,9 @@ class FeedlySyncAllOperationTests: XCTestCase { let caller = FeedlyAPICaller(transport: URLSession.webserviceTransport(), api: .sandbox) 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() diff --git a/Frameworks/Account/Feedly/FeedlyAPICaller.swift b/Frameworks/Account/Feedly/FeedlyAPICaller.swift index 29cfa8ff1..10f58b59a 100644 --- a/Frameworks/Account/Feedly/FeedlyAPICaller.swift +++ b/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)" diff --git a/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift b/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift index 8f8a0e588..bc4495d2e 100644 --- a/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift +++ b/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift @@ -117,11 +117,9 @@ final class FeedlyAccountDelegate: AccountDelegate { } let log = self.log - let progress = refreshProgress - progress.addToNumberOfTasksAndRemaining(1) - - let operation = FeedlySyncAllOperation(account: account, credentials: credentials, caller: caller, database: database, lastSuccessfulFetchStartDate: accountMetadata?.lastArticleFetch, log: log) + let operation = FeedlySyncAllOperation(account: account, credentials: credentials, caller: caller, database: database, lastSuccessfulFetchStartDate: accountMetadata?.lastArticleFetch, downloadProgress: refreshProgress, 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) } diff --git a/Frameworks/Account/Feedly/FeedlyAccountDelegateError.swift b/Frameworks/Account/Feedly/FeedlyAccountDelegateError.swift index 578de4322..8a8c1e1d8 100644 --- a/Frameworks/Account/Feedly/FeedlyAccountDelegateError.swift +++ b/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 diff --git a/Frameworks/Account/Feedly/Operations/FeedlyOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyOperation.swift index 5c6b16afc..2b62ad752 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyOperation.swift +++ b/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 } diff --git a/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift index ff6a1df96..17ac26887 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift +++ b/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) -> ())? - 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() {