From 325f22018182aeed9bd86c4f6ddc02add84c3e4f Mon Sep 17 00:00:00 2001 From: Kiel Gillard Date: Mon, 13 Jan 2020 17:51:16 +1100 Subject: [PATCH] Ignore the remote article status if the sync database contains a pending (and therefore more recent) status. Fixes #1516. --- .../FeedlyAddNewFeedOperationTests.swift | 6 +++ .../Feedly/FeedlyAccountDelegate.swift | 5 ++- .../FeedlyAddNewFeedOperation.swift | 7 +++- ...edlyIngestStarredArticleIdsOperation.swift | 38 +++++++++++++++---- ...eedlyIngestUnreadArticleIdsOperation.swift | 38 +++++++++++++++---- .../Operations/FeedlySyncAllOperation.swift | 4 +- 6 files changed, 76 insertions(+), 22 deletions(-) diff --git a/Frameworks/Account/AccountTests/Feedly/FeedlyAddNewFeedOperationTests.swift b/Frameworks/Account/AccountTests/Feedly/FeedlyAddNewFeedOperationTests.swift index 396cc8783..775b52a67 100644 --- a/Frameworks/Account/AccountTests/Feedly/FeedlyAddNewFeedOperationTests.swift +++ b/Frameworks/Account/AccountTests/Feedly/FeedlyAddNewFeedOperationTests.swift @@ -89,6 +89,7 @@ class FeedlyAddNewFeedOperationTests: XCTestCase { } let progress = DownloadProgress(numberOfTasks: 0) + let container = support.makeTestDatabaseContainer() let _ = expectationForCompletion(of: progress) let addNewFeed = try! FeedlyAddNewFeedOperation(account: account, @@ -99,6 +100,7 @@ class FeedlyAddNewFeedOperationTests: XCTestCase { addToCollectionService: caller, syncUnreadIdsService: caller, getStreamContentsService: caller, + database: container.database, container: folder, progress: progress, log: support.log) @@ -126,6 +128,7 @@ class FeedlyAddNewFeedOperationTests: XCTestCase { } let progress = DownloadProgress(numberOfTasks: 0) + let container = support.makeTestDatabaseContainer() let _ = expectationForCompletion(of: progress) let subdirectory = "feedly-add-new-feed" @@ -145,6 +148,7 @@ class FeedlyAddNewFeedOperationTests: XCTestCase { addToCollectionService: caller, syncUnreadIdsService: caller, getStreamContentsService: caller, + database: container.database, container: folder, progress: progress, log: support.log) @@ -191,6 +195,7 @@ class FeedlyAddNewFeedOperationTests: XCTestCase { } let progress = DownloadProgress(numberOfTasks: 0) + let container = support.makeTestDatabaseContainer() let _ = expectationForCompletion(of: progress) let subdirectory = "feedly-add-new-feed" @@ -220,6 +225,7 @@ class FeedlyAddNewFeedOperationTests: XCTestCase { addToCollectionService: service, syncUnreadIdsService: caller, getStreamContentsService: caller, + database: container.database, container: folder, progress: progress, log: support.log) diff --git a/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift b/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift index 4e24cde7d..caf462074 100644 --- a/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift +++ b/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift @@ -156,7 +156,7 @@ final class FeedlyAccountDelegate: AccountDelegate { let group = DispatchGroup() - let ingestUnread = FeedlyIngestUnreadArticleIdsOperation(account: account, credentials: credentials, service: caller, newerThan: nil, log: log) + let ingestUnread = FeedlyIngestUnreadArticleIdsOperation(account: account, credentials: credentials, service: caller, database: database, newerThan: nil, log: log) group.enter() ingestUnread.completionBlock = { @@ -164,7 +164,7 @@ final class FeedlyAccountDelegate: AccountDelegate { } - let ingestStarred = FeedlyIngestStarredArticleIdsOperation(account: account, credentials: credentials, service: caller, newerThan: nil, log: log) + let ingestStarred = FeedlyIngestStarredArticleIdsOperation(account: account, credentials: credentials, service: caller, database: database, newerThan: nil, log: log) group.enter() ingestStarred.completionBlock = { @@ -297,6 +297,7 @@ final class FeedlyAccountDelegate: AccountDelegate { addToCollectionService: caller, syncUnreadIdsService: caller, getStreamContentsService: caller, + database: database, container: container, progress: refreshProgress, log: log) diff --git a/Frameworks/Account/Feedly/Operations/FeedlyAddNewFeedOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyAddNewFeedOperation.swift index 4d83e41ad..65221a9bd 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyAddNewFeedOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlyAddNewFeedOperation.swift @@ -8,6 +8,7 @@ import Foundation import os.log +import SyncDatabase import RSWeb class FeedlyAddNewFeedOperation: FeedlyOperation, FeedlyOperationDelegate, FeedlySearchOperationDelegate, FeedlyCheckpointOperationDelegate { @@ -17,6 +18,7 @@ class FeedlyAddNewFeedOperation: FeedlyOperation, FeedlyOperationDelegate, Feedl private let url: String private let account: Account private let credentials: Credentials + private let database: SyncDatabase private let feedName: String? private let addToCollectionService: FeedlyAddFeedToCollectionService private let syncUnreadIdsService: FeedlyGetStreamIdsService @@ -25,7 +27,7 @@ class FeedlyAddNewFeedOperation: FeedlyOperation, FeedlyOperationDelegate, Feedl var addCompletionHandler: ((Result) -> ())? - init(account: Account, credentials: Credentials, url: String, feedName: String?, searchService: FeedlySearchService, addToCollectionService: FeedlyAddFeedToCollectionService, syncUnreadIdsService: FeedlyGetStreamIdsService, getStreamContentsService: FeedlyGetStreamContentsService, container: Container, progress: DownloadProgress, log: OSLog) throws { + init(account: Account, credentials: Credentials, url: String, feedName: String?, searchService: FeedlySearchService, addToCollectionService: FeedlyAddFeedToCollectionService, syncUnreadIdsService: FeedlyGetStreamIdsService, getStreamContentsService: FeedlyGetStreamContentsService, database: SyncDatabase, container: Container, progress: DownloadProgress, log: OSLog) throws { let validator = FeedlyFeedContainerValidator(container: container, userId: credentials.username) (self.folder, self.collectionId) = try validator.getValidContainer() @@ -35,6 +37,7 @@ class FeedlyAddNewFeedOperation: FeedlyOperation, FeedlyOperationDelegate, Feedl self.operationQueue.isSuspended = true self.account = account self.credentials = credentials + self.database = database self.feedName = feedName self.addToCollectionService = addToCollectionService self.syncUnreadIdsService = syncUnreadIdsService @@ -92,7 +95,7 @@ class FeedlyAddNewFeedOperation: FeedlyOperation, FeedlyOperationDelegate, Feedl createFeeds.downloadProgress = downloadProgress self.operationQueue.addOperation(createFeeds) - let syncUnread = FeedlyIngestUnreadArticleIdsOperation(account: account, credentials: credentials, service: syncUnreadIdsService, newerThan: nil, log: log) + let syncUnread = FeedlyIngestUnreadArticleIdsOperation(account: account, credentials: credentials, service: syncUnreadIdsService, database: database, newerThan: nil, log: log) syncUnread.addDependency(createFeeds) syncUnread.downloadProgress = downloadProgress self.operationQueue.addOperation(syncUnread) diff --git a/Frameworks/Account/Feedly/Operations/FeedlyIngestStarredArticleIdsOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyIngestStarredArticleIdsOperation.swift index 87b9d73b9..21e483d39 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyIngestStarredArticleIdsOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlyIngestStarredArticleIdsOperation.swift @@ -8,6 +8,7 @@ import Foundation import os.log +import SyncDatabase /// Single responsibility is to clone locally the remote starred article state. /// @@ -19,19 +20,20 @@ final class FeedlyIngestStarredArticleIdsOperation: FeedlyOperation { private let account: Account private let resource: FeedlyResourceId private let service: FeedlyGetStreamIdsService - private let entryIdsProvider: FeedlyEntryIdentifierProvider + private let database: SyncDatabase + private var remoteEntryIds = Set() private let log: OSLog - convenience init(account: Account, credentials: Credentials, service: FeedlyGetStreamIdsService, newerThan: Date?, log: OSLog) { + convenience init(account: Account, credentials: Credentials, service: FeedlyGetStreamIdsService, database: SyncDatabase, newerThan: Date?, log: OSLog) { let resource = FeedlyTagResourceId.Global.saved(for: credentials.username) - self.init(account: account, resource: resource, service: service, newerThan: newerThan, log: log) + self.init(account: account, resource: resource, service: service, database: database, newerThan: newerThan, log: log) } - init(account: Account, resource: FeedlyResourceId, service: FeedlyGetStreamIdsService, newerThan: Date?, log: OSLog) { + init(account: Account, resource: FeedlyResourceId, service: FeedlyGetStreamIdsService, database: SyncDatabase, newerThan: Date?, log: OSLog) { self.account = account self.resource = resource self.service = service - self.entryIdsProvider = FeedlyEntryIdentifierProvider() + self.database = database self.log = log } @@ -57,10 +59,10 @@ final class FeedlyIngestStarredArticleIdsOperation: FeedlyOperation { switch result { case .success(let streamIds): - entryIdsProvider.addEntryIds(in: streamIds.ids) + remoteEntryIds.formUnion(streamIds.ids) guard let continuation = streamIds.continuation else { - updateStarredStatuses() + removeEntryIdsWithPendingStatus() return } @@ -71,6 +73,26 @@ final class FeedlyIngestStarredArticleIdsOperation: FeedlyOperation { } } + /// Do not override pending statuses with the remote statuses of the same articles, otherwise an article will temporarily re-acquire the remote status before the pending status is pushed and subseqently pulled. + private func removeEntryIdsWithPendingStatus() { + guard !isCancelled else { + didFinish() + return + } + + database.selectPendingStarredStatusArticleIDs { result in + switch result { + case .success(let pendingArticleIds): + self.remoteEntryIds.subtract(pendingArticleIds) + + self.updateStarredStatuses() + + case .failure(let error): + self.didFinish(error) + } + } + } + private func updateStarredStatuses() { guard !isCancelled else { didFinish() @@ -94,7 +116,7 @@ final class FeedlyIngestStarredArticleIdsOperation: FeedlyOperation { return } - let remoteStarredArticleIDs = entryIdsProvider.entryIds + let remoteStarredArticleIDs = remoteEntryIds let group = DispatchGroup() diff --git a/Frameworks/Account/Feedly/Operations/FeedlyIngestUnreadArticleIdsOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyIngestUnreadArticleIdsOperation.swift index 3f5036138..ce3b0bb9e 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyIngestUnreadArticleIdsOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlyIngestUnreadArticleIdsOperation.swift @@ -9,6 +9,7 @@ import Foundation import os.log import RSParser +import SyncDatabase /// Single responsibility is to clone locally the remote unread article state. /// @@ -20,19 +21,20 @@ final class FeedlyIngestUnreadArticleIdsOperation: FeedlyOperation { private let account: Account private let resource: FeedlyResourceId private let service: FeedlyGetStreamIdsService - private let entryIdsProvider: FeedlyEntryIdentifierProvider + private let database: SyncDatabase + private var remoteEntryIds = Set() private let log: OSLog - convenience init(account: Account, credentials: Credentials, service: FeedlyGetStreamIdsService, newerThan: Date?, log: OSLog) { + convenience init(account: Account, credentials: Credentials, service: FeedlyGetStreamIdsService, database: SyncDatabase, newerThan: Date?, log: OSLog) { let resource = FeedlyCategoryResourceId.Global.all(for: credentials.username) - self.init(account: account, resource: resource, service: service, newerThan: newerThan, log: log) + self.init(account: account, resource: resource, service: service, database: database, newerThan: newerThan, log: log) } - init(account: Account, resource: FeedlyResourceId, service: FeedlyGetStreamIdsService, newerThan: Date?, log: OSLog) { + init(account: Account, resource: FeedlyResourceId, service: FeedlyGetStreamIdsService, database: SyncDatabase, newerThan: Date?, log: OSLog) { self.account = account self.resource = resource self.service = service - self.entryIdsProvider = FeedlyEntryIdentifierProvider() + self.database = database self.log = log } @@ -58,10 +60,10 @@ final class FeedlyIngestUnreadArticleIdsOperation: FeedlyOperation { switch result { case .success(let streamIds): - entryIdsProvider.addEntryIds(in: streamIds.ids) + remoteEntryIds.formUnion(streamIds.ids) guard let continuation = streamIds.continuation else { - updateUnreadStatuses() + removeEntryIdsWithPendingStatus() return } @@ -72,6 +74,26 @@ final class FeedlyIngestUnreadArticleIdsOperation: FeedlyOperation { } } + /// Do not override pending statuses with the remote statuses of the same articles, otherwise an article will temporarily re-acquire the remote status before the pending status is pushed and subseqently pulled. + private func removeEntryIdsWithPendingStatus() { + guard !isCancelled else { + didFinish() + return + } + + database.selectPendingReadStatusArticleIDs { result in + switch result { + case .success(let pendingArticleIds): + self.remoteEntryIds.subtract(pendingArticleIds) + + self.updateUnreadStatuses() + + case .failure(let error): + self.didFinish(error) + } + } + } + private func updateUnreadStatuses() { guard !isCancelled else { didFinish() @@ -95,7 +117,7 @@ final class FeedlyIngestUnreadArticleIdsOperation: FeedlyOperation { return } - let remoteUnreadArticleIDs = entryIdsProvider.entryIds + let remoteUnreadArticleIDs = remoteEntryIds let group = DispatchGroup() final class ReadStatusResults { diff --git a/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift index 2218eb501..90ad7268e 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift @@ -73,7 +73,7 @@ final class FeedlySyncAllOperation: FeedlyOperation { self.operationQueue.addOperation(getAllArticleIds) // Get each page of unread article ids in the global.all stream for the last 31 days (nil = Feedly API default). - let getUnread = FeedlyIngestUnreadArticleIdsOperation(account: account, credentials: credentials, service: getUnreadService, newerThan: nil, log: log) + let getUnread = FeedlyIngestUnreadArticleIdsOperation(account: account, credentials: credentials, service: getUnreadService, database: database, newerThan: nil, log: log) getUnread.delegate = self getUnread.addDependency(getAllArticleIds) getUnread.downloadProgress = downloadProgress @@ -88,7 +88,7 @@ final class FeedlySyncAllOperation: FeedlyOperation { self.operationQueue.addOperation(getUpdated) // Get each page of the article ids for starred articles. - let getStarred = FeedlyIngestStarredArticleIdsOperation(account: account, credentials: credentials, service: getStarredService, newerThan: nil, log: log) + let getStarred = FeedlyIngestStarredArticleIdsOperation(account: account, credentials: credentials, service: getStarredService, database: database, newerThan: nil, log: log) getStarred.delegate = self getStarred.downloadProgress = downloadProgress getStarred.addDependency(createFeedsOperation)