diff --git a/Zotero/Assets/en.lproj/Localizable.strings b/Zotero/Assets/en.lproj/Localizable.strings index 49dffaa8d..6f9348eee 100644 --- a/Zotero/Assets/en.lproj/Localizable.strings +++ b/Zotero/Assets/en.lproj/Localizable.strings @@ -418,6 +418,7 @@ "errors.sync_toolbar.conflict_retry_limit" = "Remote sync in progress. Please try again in a few minutes."; "errors.sync_toolbar.group_permissions" = "You don’t have permission to edit groups."; "errors.sync_toolbar.internet_connection" = "Unable to connect to the network. Please try again."; +"errors.sync_toolbar.unexpected_my_library_last_read_deletions" = "Sync received unexpected last read settings deletions for My Library."; "errors.sync_toolbar.personal_quota_reached" = "You have reached your Zotero Storage quota. Some files were not uploaded. Other Zotero data will continue to sync.\n\nSee your zotero.org account settings for additional storage options."; "errors.sync_toolbar.group_quota_reached" = "The group “%@” has reached its Zotero Storage quota. Some files were not uploaded. Other Zotero data will continue to sync.\n\nThe group owner can increase the group’s storage capacity from their storage settings on zotero.org."; "errors.sync_toolbar.insufficient_space" = "You have insufficient space on your WebDAV server. Some files were not uploaded. Other Zotero data will continue to sync."; diff --git a/Zotero/Controllers/Database/Requests/MarkObjectsAsSyncedDbRequest.swift b/Zotero/Controllers/Database/Requests/MarkObjectsAsSyncedDbRequest.swift index 329a8eb56..611e5f223 100644 --- a/Zotero/Controllers/Database/Requests/MarkObjectsAsSyncedDbRequest.swift +++ b/Zotero/Controllers/Database/Requests/MarkObjectsAsSyncedDbRequest.swift @@ -51,9 +51,9 @@ struct MarkSettingsAsSyncedDbRequest: DbRequest { func process(in database: Realm) throws { for setting in settings { let object: UpdatableObject&Syncable - if setting.uid.starts(with: "lastRead"), let lastRead = database.objects(RLastReadDate.self).uniqueObject(key: setting.key, libraryId: setting.libraryId) { + if setting.uid.starts(with: "lastRead_"), let lastRead = database.objects(RLastReadDate.self).uniqueObject(key: setting.key, libraryId: setting.libraryId) { object = lastRead - } else if setting.uid.starts(with: "lastPageIndex"), let pageIndex = database.objects(RPageIndex.self).uniqueObject(key: setting.key, libraryId: setting.libraryId) { + } else if setting.uid.starts(with: "lastPageIndex_"), let pageIndex = database.objects(RPageIndex.self).uniqueObject(key: setting.key, libraryId: setting.libraryId) { object = pageIndex } else { DDLogError("MarkSettingsAsSyncedDbRequest: could not find setting for \(setting.uid)") diff --git a/Zotero/Controllers/Database/Requests/PerformDeletionsDbRequest.swift b/Zotero/Controllers/Database/Requests/PerformDeletionsDbRequest.swift index 2dba2a3bc..86edd5710 100644 --- a/Zotero/Controllers/Database/Requests/PerformDeletionsDbRequest.swift +++ b/Zotero/Controllers/Database/Requests/PerformDeletionsDbRequest.swift @@ -143,22 +143,32 @@ struct PerformPageIndexDeletionsDbRequest: DbRequest { } struct PerformLastReadDeletionsDbRequest: DbRequest { + enum Error: Swift.Error { + case myLibraryNotSupported + } + let libraryId: LibraryIdentifier let keys: [String] var needsWrite: Bool { return true } func process(in database: Realm) throws { - let objects = database.objects(RLastReadDate.self).filter(.keys(keys, in: libraryId)) - for object in objects { - guard !object.isInvalidated else { continue } - if object.isChanged { - // If remotely deleted lastRead is changed locally, we want to keep the lastRead, so we mark that - // this lastRead is new and it will be reinserted by sync - object.markAsChanged(in: database) - } else { - object.willRemove(in: database) - database.delete(object) + switch libraryId { + case .custom(.myLibrary): + throw Error.myLibraryNotSupported + + case .group: + let objects = database.objects(RLastReadDate.self).filter(.keys(keys, in: libraryId)) + for object in objects { + guard !object.isInvalidated else { continue } + if object.isChanged { + // If remotely deleted lastRead is changed locally, we want to keep the lastRead, so we mark that + // this lastRead is new and it will be reinserted by sync + object.markAsChanged(in: database) + } else { + object.willRemove(in: database) + database.delete(object) + } } } } diff --git a/Zotero/Controllers/Sync/SyncActions/PerformDeletionsSyncAction.swift b/Zotero/Controllers/Sync/SyncActions/PerformDeletionsSyncAction.swift index 5a7cfca0e..032575ff2 100644 --- a/Zotero/Controllers/Sync/SyncActions/PerformDeletionsSyncAction.swift +++ b/Zotero/Controllers/Sync/SyncActions/PerformDeletionsSyncAction.swift @@ -8,10 +8,11 @@ import Foundation +import CocoaLumberjackSwift import RxSwift struct PerformDeletionsSyncAction: SyncAction { - typealias Result = [(String, String)] + typealias Result = (conflicts: [(String, String)], unexpectedMyLibraryLastReadDeletions: [String]) private static let batchSize = 500 let libraryId: LibraryIdentifier @@ -24,7 +25,7 @@ struct PerformDeletionsSyncAction: SyncAction { unowned let dbStorage: DbStorage let queue: DispatchQueue - var result: Single<[(String, String)]> { + var result: Single { return Single.create { subscriber -> Disposable in do { let hasCollections = try dbStorage.perform(request: CountObjectsDbRequest(), on: queue) > 0 @@ -57,7 +58,7 @@ struct PerformDeletionsSyncAction: SyncAction { } } - let pageIndices = settings.filter({ $0.hasPrefix("lastPageIndex") }) + let pageIndices = settings.filter({ $0.hasPrefix("lastPageIndex_") }) let hasPageIndices = try dbStorage.perform(request: CountObjectsDbRequest(), on: queue) > 0 if hasPageIndices { try batch(values: pageIndices, batchSize: Self.batchSize) { uids in @@ -72,7 +73,8 @@ struct PerformDeletionsSyncAction: SyncAction { } } - let lastRead = settings.filter({ $0.hasPrefix("lastRead") }) + var unexpectedMyLibraryLastReadDeletions: [String] = [] + let lastRead = settings.filter({ $0.hasPrefix("lastRead_") }) let hasLastRead = try dbStorage.perform(request: CountObjectsDbRequest(), on: queue) > 0 if hasLastRead { try batch(values: lastRead, batchSize: Self.batchSize) { uids in @@ -82,12 +84,25 @@ struct PerformDeletionsSyncAction: SyncAction { groupedIndices[libraryId, default: []].append(key) } for (libraryId, keys) in groupedIndices { - try dbStorage.perform(request: PerformLastReadDeletionsDbRequest(libraryId: libraryId, keys: keys), on: queue) + do { + try dbStorage.perform(request: PerformLastReadDeletionsDbRequest(libraryId: libraryId, keys: keys), on: queue) + } catch let error { + switch error { + case PerformLastReadDeletionsDbRequest.Error.myLibraryNotSupported: + unexpectedMyLibraryLastReadDeletions.append(contentsOf: keys) + + default: + throw error + } + } } } } + if !unexpectedMyLibraryLastReadDeletions.isEmpty { + DDLogWarn("PerformDeletionsSyncAction: Received unexpected My Library lastRead deletions - \(unexpectedMyLibraryLastReadDeletions)") + } - subscriber(.success(conflicts)) + subscriber(.success((conflicts: conflicts, unexpectedMyLibraryLastReadDeletions: unexpectedMyLibraryLastReadDeletions))) } catch let error { subscriber(.failure(error)) } diff --git a/Zotero/Controllers/Sync/SyncActions/SubmitDeletionSyncAction.swift b/Zotero/Controllers/Sync/SyncActions/SubmitDeletionSyncAction.swift index f35a443ac..d5d4f6dc3 100644 --- a/Zotero/Controllers/Sync/SyncActions/SubmitDeletionSyncAction.swift +++ b/Zotero/Controllers/Sync/SyncActions/SubmitDeletionSyncAction.swift @@ -66,7 +66,7 @@ struct SubmitDeletionSyncAction: SyncAction { var groupedSettings: [LibraryIdentifier: [String]] = [:] for uid in keys { // only lastRead is deletable - guard uid.starts(with: "lastRead"), let (key, libraryId) = try? SettingKeyParser.parse(key: uid) else { continue } + guard uid.starts(with: "lastRead_"), let (key, libraryId) = try? SettingKeyParser.parse(key: uid) else { continue } groupedSettings[libraryId, default: []].append(key) } for (libraryId, keys) in groupedSettings { diff --git a/Zotero/Controllers/Sync/SyncController.swift b/Zotero/Controllers/Sync/SyncController.swift index 04cb9a46d..0c3877124 100644 --- a/Zotero/Controllers/Sync/SyncController.swift +++ b/Zotero/Controllers/Sync/SyncController.swift @@ -470,7 +470,8 @@ final class SyncController: SynchronizationController { .webDavDeletionFailed, .webDavVerification, .webDavDownload, - .webDavUpload: + .webDavUpload, + .unexpectedMyLibraryLastReadDeletions: reportErrors.append(error) } } @@ -1394,9 +1395,9 @@ final class SyncController: SynchronizationController { queue: self.workQueue ) action.result.subscribe(on: self.workScheduler) - .subscribe(onSuccess: { [weak self] conflicts in + .subscribe(onSuccess: { [weak self] result in self?.accessQueue.async(flags: .barrier) { [weak self] in - self?.finishDeletionsSync(result: .success(conflicts), items: items, libraryId: libraryId) + self?.finishDeletionsSync(result: .success(result), items: items, libraryId: libraryId) } }, onFailure: { [weak self] error in self?.accessQueue.async(flags: .barrier) { [weak self] in @@ -1406,9 +1407,14 @@ final class SyncController: SynchronizationController { .disposed(by: self.disposeBag) } - private func finishDeletionsSync(result: Result<[(String, String)], Error>, items: [String]?, libraryId: LibraryIdentifier, version: Int? = nil) { + private func finishDeletionsSync(result: Result, items: [String]?, libraryId: LibraryIdentifier, version: Int? = nil) { switch result { - case .success(let conflicts): + case .success(let result): + if !result.unexpectedMyLibraryLastReadDeletions.isEmpty { + nonFatalErrors.append(.unexpectedMyLibraryLastReadDeletions(keys: result.unexpectedMyLibraryLastReadDeletions)) + } + + let conflicts = result.conflicts if !conflicts.isEmpty { self.resolve(conflict: .removedItemsHaveLocalChanges(keys: conflicts, libraryId: libraryId)) } else { diff --git a/Zotero/Models/Sync/SyncErrors.swift b/Zotero/Models/Sync/SyncErrors.swift index cc0c19a68..9830f1b35 100644 --- a/Zotero/Models/Sync/SyncErrors.swift +++ b/Zotero/Models/Sync/SyncErrors.swift @@ -79,6 +79,7 @@ enum SyncError { case webDavDownload(WebDavError.Download) case webDavUpload(WebDavError.Upload) case preconditionFailed(LibraryIdentifier) + case unexpectedMyLibraryLastReadDeletions(keys: [String]) var isVersionMismatch: Bool { switch self { diff --git a/Zotero/Scenes/General/Views/SyncToolbarController.swift b/Zotero/Scenes/General/Views/SyncToolbarController.swift index 842e69d62..c93495466 100644 --- a/Zotero/Scenes/General/Views/SyncToolbarController.swift +++ b/Zotero/Scenes/General/Views/SyncToolbarController.swift @@ -266,6 +266,9 @@ final class SyncToolbarController { case .preconditionFailed(let libraryId): return (L10n.Errors.SyncToolbar.conflictRetryLimit, SyncError.ErrorData(itemKeys: nil, libraryId: libraryId)) + + case .unexpectedMyLibraryLastReadDeletions(let keys): + return (L10n.Errors.SyncToolbar.unexpectedMyLibraryLastReadDeletions, SyncError.ErrorData(itemKeys: keys, libraryId: .custom(.myLibrary))) } } diff --git a/ZoteroTests/SyncActionsSpec.swift b/ZoteroTests/SyncActionsSpec.swift index a50f60f09..2cc367106 100644 --- a/ZoteroTests/SyncActionsSpec.swift +++ b/ZoteroTests/SyncActionsSpec.swift @@ -740,6 +740,48 @@ final class SyncActionsSpec: QuickSpec { }) } } + + context("remote settings deletions") { + it("doesn't treat lastReadAloudPosition deletions as lastRead deletions") { + try! realm.write { + let lastRead = RLastReadDate() + lastRead.key = "EXISTINGKEY" + lastRead.date = Date(timeIntervalSince1970: 1234) + lastRead.groupKey = 1 + realm.add(lastRead) + } + + waitUntil(timeout: .seconds(60), action: { doneAction in + PerformDeletionsSyncAction( + libraryId: .custom(.myLibrary), + collections: [], + items: [], + searches: [], + tags: [], + settings: ["lastReadAloudPosition_u_R2NCC4YU"], + conflictMode: .resolveConflicts, + dbStorage: dbStorage, + queue: .main + ) + .result + .subscribe(onSuccess: { result in + expect(result.conflicts).to(beEmpty()) + expect(result.unexpectedMyLibraryLastReadDeletions).to(beEmpty()) + + realm.refresh() + let lastReadDates = realm.objects(RLastReadDate.self) + expect(lastReadDates.count).to(equal(1)) + expect(lastReadDates.first?.key).to(equal("EXISTINGKEY")) + + doneAction() + }, onFailure: { error in + fail("PerformDeletionsSyncAction failed with \(error)") + doneAction() + }) + .disposed(by: disposeBag) + }) + } + } } } } diff --git a/ZoteroTests/UpdatableObjectSpec.swift b/ZoteroTests/UpdatableObjectSpec.swift index 98c788a18..4b2cb7bf4 100644 --- a/ZoteroTests/UpdatableObjectSpec.swift +++ b/ZoteroTests/UpdatableObjectSpec.swift @@ -114,6 +114,35 @@ final class UpdatableObjectSpec: QuickSpec { expect(refinedBy["end"] as? Int).to(equal(1705)) } } + + context("when applying My Library last-read deletions") { + it("throws and leaves the item's last-read value unchanged") { + let key = "AAAAAAAA" + let date = Date(timeIntervalSince1970: 1234) + + try! realm.write { + let item = RItem() + item.key = key + item.rawType = ItemTypes.attachment + item.customLibraryKey = .myLibrary + item.dateAdded = Date() + item.dateModified = item.dateAdded + item.lastRead = date + item.updateEffectiveLastRead() + realm.add(item) + } + + expect { + try realm.write { + try PerformLastReadDeletionsDbRequest(libraryId: .custom(.myLibrary), keys: [key]).process(in: realm) + } + }.to(throwError()) + + let item = realm.objects(RItem.self).first! + expect(item.lastRead).to(equal(date)) + expect(item.effectiveLastRead).to(equal(date)) + } + } } } }