From e3f745c073b1a59c45aa1ab637b6d59d79709656 Mon Sep 17 00:00:00 2001 From: Matt Bumgardner Date: Mon, 5 Nov 2018 14:37:48 -0600 Subject: [PATCH 1/2] Added updateReadStatus to NotificationStore --- .../Tools/StorageType+Extensions.swift | 9 ++++- .../Yosemite/Actions/NotificationAction.swift | 1 + .../Yosemite/Stores/NotificationStore.swift | 40 +++++++++++++++++-- 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/Storage/Storage/Tools/StorageType+Extensions.swift b/Storage/Storage/Tools/StorageType+Extensions.swift index b75c30abcf2..9c0d952cfde 100644 --- a/Storage/Storage/Tools/StorageType+Extensions.swift +++ b/Storage/Storage/Tools/StorageType+Extensions.swift @@ -92,7 +92,14 @@ public extension StorageType { /// Retrieves the Notification. /// - public func loadNotification(noteID: Int, noteHash: Int) -> Note? { + public func loadNotification(noteID: Int64) -> Note? { + let predicate = NSPredicate(format: "noteID = %ld", noteID) + return firstObject(ofType: Note.self, matching: predicate) + } + + /// Retrieves the Notification. + /// + public func loadNotification(noteID: Int64, noteHash: Int) -> Note? { let predicate = NSPredicate(format: "noteID = %ld AND noteHash = %ld", noteID, noteHash) return firstObject(ofType: Note.self, matching: predicate) } diff --git a/Yosemite/Yosemite/Actions/NotificationAction.swift b/Yosemite/Yosemite/Actions/NotificationAction.swift index f7a415cae36..ef306a21d4b 100644 --- a/Yosemite/Yosemite/Actions/NotificationAction.swift +++ b/Yosemite/Yosemite/Actions/NotificationAction.swift @@ -8,4 +8,5 @@ import Networking public enum NotificationAction: Action { case synchronizeNotifications(onCompletion: (Error?) -> Void) case updateLastSeen(timestamp: String, onCompletion: (Error?) -> Void) + case updateReadStatus(noteID: Int64, read: Bool, onCompletion: (Error?) -> Void) } diff --git a/Yosemite/Yosemite/Stores/NotificationStore.swift b/Yosemite/Yosemite/Stores/NotificationStore.swift index 159d1964c0f..d21fa9adf12 100644 --- a/Yosemite/Yosemite/Stores/NotificationStore.swift +++ b/Yosemite/Yosemite/Stores/NotificationStore.swift @@ -30,6 +30,8 @@ public class NotificationStore: Store { synchronizeNotifications(onCompletion: onCompletion) case .updateLastSeen(let timestamp, let onCompletion): updateLastSeen(timestamp: timestamp, onCompletion: onCompletion) + case .updateReadStatus(let noteID, let read, let onCompletion): + updateReadStatus(noteID: noteID, read: read, onCompletion: onCompletion) } } } @@ -70,12 +72,28 @@ private extension NotificationStore { onCompletion(error) } } + + /// Updates the read status for the given notification ID + /// + func updateReadStatus(noteID: Int64, read: Bool, onCompletion: @escaping (Error?) -> Void) { + let remote = NotificationsRemote(network: network) + remote.updateReadStatus(String(noteID), read: read) { [weak self] (error) in + guard let `self` = self, error == nil else { + onCompletion(error) + return + } + + self.updateReadStatus(noteID: noteID, read: read, onCompletion: { (error) in + onCompletion(error) + }) + } + } } // MARK: - Persistence // -private extension NotificationStore { +extension NotificationStore { /// Deletes the collection of local notifications that cannot be found in a given collection of /// remote hashes. @@ -111,9 +129,7 @@ private extension NotificationStore { derivedStorage.perform { for remoteNote in remoteNotes { - let predicate = NSPredicate(format: "(noteID == %ld)", remoteNote.noteId) - let localNote = derivedStorage.firstObject(ofType: Storage.Note.self, matching: predicate) ?? derivedStorage.insertNewObject(ofType: Storage.Note.self) - + let localNote = derivedStorage.loadNotification(noteID: remoteNote.noteId) ?? derivedStorage.insertNewObject(ofType: Storage.Note.self) localNote.update(with: remoteNote) } } @@ -124,6 +140,22 @@ private extension NotificationStore { } } } + + /// Updates the read status for the given noteID in the Storage layer. If the local note to update cannot be found, + /// nothing is updated/created in storage. + /// + func updateReadStatus(for noteID: Int64, read: Bool, completion: (() -> Void)? = nil) { + assert(Thread.isMainThread) + let storage = storageManager.viewStorage + guard let storageNote = storage.loadNotification(noteID: noteID) else { + completion?() + return + } + + storageNote.read = read + storage.saveIfNeeded() + completion?() + } } From 2639d2ba1db1670ed4827c8ad0c4baf734226cb2 Mon Sep 17 00:00:00 2001 From: Matt Bumgardner Date: Mon, 5 Nov 2018 16:46:15 -0600 Subject: [PATCH 2/2] Unit tests for NotificationStore --- .../Yosemite/Stores/NotificationStore.swift | 17 ++- .../Stores/NotificationStoreTests.swift | 143 +++++++++++++++++- 2 files changed, 154 insertions(+), 6 deletions(-) diff --git a/Yosemite/Yosemite/Stores/NotificationStore.swift b/Yosemite/Yosemite/Stores/NotificationStore.swift index d21fa9adf12..afa735473f7 100644 --- a/Yosemite/Yosemite/Stores/NotificationStore.swift +++ b/Yosemite/Yosemite/Stores/NotificationStore.swift @@ -58,6 +58,7 @@ private extension NotificationStore { // Step 2: Update the storage notes from the fetched notes self.updateLocalNotes(with: remoteNotes) { + type(of: self).resetSharedDerivedStorage() onCompletion(nil) } } @@ -82,10 +83,10 @@ private extension NotificationStore { onCompletion(error) return } - - self.updateReadStatus(noteID: noteID, read: read, onCompletion: { (error) in - onCompletion(error) - }) + + self.updateLocalNoteReadStatus(for: noteID, read: read) { + onCompletion(nil) + } } } } @@ -144,7 +145,7 @@ extension NotificationStore { /// Updates the read status for the given noteID in the Storage layer. If the local note to update cannot be found, /// nothing is updated/created in storage. /// - func updateReadStatus(for noteID: Int64, read: Bool, completion: (() -> Void)? = nil) { + func updateLocalNoteReadStatus(for noteID: Int64, read: Bool, completion: (() -> Void)? = nil) { assert(Thread.isMainThread) let storage = storageManager.viewStorage guard let storageNote = storage.loadNotification(noteID: noteID) else { @@ -171,6 +172,12 @@ extension NotificationStore { } return privateStorage } + + /// Nukes the private Shared Derived Storage instance. + /// + static func resetSharedDerivedStorage() { + privateStorage = nil + } } diff --git a/Yosemite/YosemiteTests/Stores/NotificationStoreTests.swift b/Yosemite/YosemiteTests/Stores/NotificationStoreTests.swift index 34c3593c508..b2ffe5228fc 100644 --- a/Yosemite/YosemiteTests/Stores/NotificationStoreTests.swift +++ b/Yosemite/YosemiteTests/Stores/NotificationStoreTests.swift @@ -30,7 +30,10 @@ class NotificationStoreTests: XCTestCase { super.setUp() dispatcher = Dispatcher() storageManager = MockupStorageManager() - network = MockupNetwork(useResponseQueue: true) + network = MockupNetwork() + + // Need to nuke this in-between tests otherwise some will randomly fail + NotificationStore.resetSharedDerivedStorage() } @@ -135,4 +138,142 @@ class NotificationStoreTests: XCTestCase { noteStore.onAction(action) wait(for: [expectation], timeout: Constants.expectationTimeout) } + + + // MARK: - NotificationAction.updateReadStatus + + /// Verifies that NotificationAction.updateReadStatus handles a success response from the backend properly + /// + func testUpdateNotificationReadStatusReturnsSuccess() { + let expectation = self.expectation(description: "Update read status success response") + let noteStore = NotificationStore(dispatcher: dispatcher, storageManager: storageManager, network: network) + let originalNote = sampleNotification() + let expectedNote = sampleNotificationMutated() + + network.simulateResponse(requestUrlSuffix: "notifications/read", filename: "generic_success") + let action = NotificationAction.updateReadStatus(noteID: originalNote.noteId, read: true) { [weak self] (error) in + XCTAssertNil(error) + let storageNote = self?.viewStorage.loadNotification(noteID: originalNote.noteId) + XCTAssertEqual(storageNote?.toReadOnly().read, expectedNote.read) + expectation.fulfill() + } + + XCTAssertEqual(viewStorage.countObjects(ofType: Storage.Note.self), 0) + noteStore.updateLocalNotes(with: [originalNote]) { [weak self] in + XCTAssertEqual(self?.viewStorage.countObjects(ofType: Storage.Note.self), 1) + noteStore.onAction(action) + } + + wait(for: [expectation], timeout: Constants.expectationTimeout) + } + + /// Verifies that NotificationAction.updateReadStatus returns an error whenever there is an error response from the backend. + /// + func testUpdateNotificationReadStatusReturnsErrorUponReponseError() { + let expectation = self.expectation(description: "Update notification read status error response") + let noteStore = NotificationStore(dispatcher: dispatcher, storageManager: storageManager, network: network) + + network.simulateResponse(requestUrlSuffix: "notifications/read", filename: "generic_error") + let action = NotificationAction.updateReadStatus(noteID: 9999, read: true) { (error) in + XCTAssertNotNil(error) + expectation.fulfill() + } + + noteStore.onAction(action) + wait(for: [expectation], timeout: Constants.expectationTimeout) + } + + /// Verifies that NotificationAction.updateReadStatus returns an error whenever there is no backend response. + /// + func testUpdateNotificationReadStatusReturnsErrorUponEmptyResponse() { + let expectation = self.expectation(description: "Update notification read status empty response") + let noteStore = NotificationStore(dispatcher: dispatcher, storageManager: storageManager, network: network) + + let action = NotificationAction.updateReadStatus(noteID: 9999, read: true) { (error) in + XCTAssertNotNil(error) + expectation.fulfill() + } + + noteStore.onAction(action) + wait(for: [expectation], timeout: Constants.expectationTimeout) + } + + /// Verifies that `updateLocalNoteReadStatus` does not produce duplicate entries. + /// + func testUpdateStoredNotificationEffectivelyUpdatesPreexistantNotification() { + let expectation = self.expectation(description: "Update read status on existing note") + let noteStore = NotificationStore(dispatcher: dispatcher, storageManager: storageManager, network: network) + let originalNote = sampleNotification() + let expectedNote = sampleNotificationMutated() + + XCTAssertEqual(viewStorage.countObjects(ofType: Storage.Note.self), 0) + noteStore.updateLocalNotes(with: [originalNote]) { [weak self] in + XCTAssertEqual(self?.viewStorage.countObjects(ofType: Storage.Note.self), 1) + noteStore.updateLocalNoteReadStatus(for: originalNote.noteId, read: true) { + XCTAssertEqual(self?.viewStorage.countObjects(ofType: Storage.Note.self), 1) + let storageNote = self?.viewStorage.loadNotification(noteID: originalNote.noteId) + XCTAssertEqual(storageNote?.toReadOnly().read, expectedNote.read) + expectation.fulfill() + } + } + + wait(for: [expectation], timeout: Constants.expectationTimeout) + } + + /// Verifies that `updateLocalNoteReadStatus` does not produce duplicate entries with an invalid notification ID. + /// + func testUpdateStoredNotificationDoesntUpdateInvalidNote() { + let expectation = self.expectation(description: "Update read status on invalid note") + let noteStore = NotificationStore(dispatcher: dispatcher, storageManager: storageManager, network: network) + + XCTAssertEqual(viewStorage.countObjects(ofType: Storage.Note.self), 0) + noteStore.updateLocalNoteReadStatus(for: 9999, read: true) { [weak self] in + XCTAssertEqual(self?.viewStorage.countObjects(ofType: Storage.Note.self), 0) + let storageNote = self?.viewStorage.loadNotification(noteID: 9999) + XCTAssertNil(storageNote) + expectation.fulfill() + } + + wait(for: [expectation], timeout: Constants.expectationTimeout) + } +} + + +// MARK: - Private Methods +// +private extension NotificationStoreTests { + + // MARK: - Notification Sample + + func sampleNotification() -> Networking.Note { + return Note(noteId: 123456, + hash: 11223344, + read: false, + icon: "https://gravatar.tld/some-hash", + noticon: "\u{f408}", + timestamp: "2018-10-22T18:51:33+00:00", + type: "comment_like", + url: "https:\\someurl.sometld", + title: "3 Likes", + subject: Data(), + header: Data(), + body: Data(), + meta: Data()) + } + + func sampleNotificationMutated() -> Networking.Note { + return Note(noteId: 123456, + hash: 11223344, + read: true, + icon: "https://gravatar.tld/some-hash", + noticon: "\u{f408}", + timestamp: "2018-10-22T18:51:33+00:00", + type: "comment_like", + url: "https:\\someurl.sometld", + title: "3 Likes", + subject: Data(), + header: Data(), + body: Data(), + meta: Data()) + } }