From a17028cb6fc4423ae406a4d0d0c20fba75895378 Mon Sep 17 00:00:00 2001 From: Ryan Nystrom Date: Tue, 25 Sep 2018 17:33:08 -0400 Subject: [PATCH 1/4] move path/defaults to be injected --- Classes/Systems/LocalNotificationsCache.swift | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/Classes/Systems/LocalNotificationsCache.swift b/Classes/Systems/LocalNotificationsCache.swift index 1aafed927..4bd98e58b 100644 --- a/Classes/Systems/LocalNotificationsCache.swift +++ b/Classes/Systems/LocalNotificationsCache.swift @@ -12,20 +12,30 @@ import GitHubAPI final class LocalNotificationsCache { - private let path: String = { + private static let sqlitePath: String = { let path = NSSearchPathForDirectoriesInDomains(.documentDirectory, .userDomainMask, true)[0] return "\(path)/read-notifications.sqlite" }() + + private let path: String + private let defaults: UserDefaults private lazy var queue: FMDatabaseQueue = { return FMDatabaseQueue(path: self.path) }() - private let defaults = UserDefaults.standard private let setupKey = "com.whoisryannystrom.freetime.local-notifications.setup" private var isFirstSetup: Bool { return defaults.bool(forKey: setupKey) } + init( + path: String = LocalNotificationsCache.sqlitePath, + defaults: UserDefaults = UserDefaults.standard + ) { + self.path = path + self.defaults = defaults + } + func update( notifications: [V3Notification], completion: @escaping ([V3Notification]) -> Void From dcf0641df03d9d7d06e2e8f33d33af2cfa310289 Mon Sep 17 00:00:00 2001 From: Ryan Nystrom Date: Tue, 25 Sep 2018 17:36:05 -0400 Subject: [PATCH 2/4] remove unused defaults --- Classes/Systems/LocalNotificationsCache.swift | 10 +----- .../LocalNotificationCacheTests.swift | 33 +++++++++++++++++++ 2 files changed, 34 insertions(+), 9 deletions(-) create mode 100644 FreetimeTests/LocalNotificationCacheTests.swift diff --git a/Classes/Systems/LocalNotificationsCache.swift b/Classes/Systems/LocalNotificationsCache.swift index 4bd98e58b..2a26539ca 100644 --- a/Classes/Systems/LocalNotificationsCache.swift +++ b/Classes/Systems/LocalNotificationsCache.swift @@ -18,22 +18,14 @@ final class LocalNotificationsCache { }() private let path: String - private let defaults: UserDefaults private lazy var queue: FMDatabaseQueue = { return FMDatabaseQueue(path: self.path) }() - private let setupKey = "com.whoisryannystrom.freetime.local-notifications.setup" - - private var isFirstSetup: Bool { - return defaults.bool(forKey: setupKey) - } init( - path: String = LocalNotificationsCache.sqlitePath, - defaults: UserDefaults = UserDefaults.standard + path: String = LocalNotificationsCache.sqlitePath ) { self.path = path - self.defaults = defaults } func update( diff --git a/FreetimeTests/LocalNotificationCacheTests.swift b/FreetimeTests/LocalNotificationCacheTests.swift new file mode 100644 index 000000000..6497b1961 --- /dev/null +++ b/FreetimeTests/LocalNotificationCacheTests.swift @@ -0,0 +1,33 @@ +// +// LocalNotificationCacheTests.swift +// FreetimeTests +// +// Created by Ryan Nystrom on 9/25/18. +// Copyright © 2018 Ryan Nystrom. All rights reserved. +// + +import XCTest + +class LocalNotificationCacheTests: XCTestCase { + + override func setUp() { + // Put setup code here. This method is called before the invocation of each test method in the class. + } + + override func tearDown() { + // Put teardown code here. This method is called after the invocation of each test method in the class. + } + + func testExample() { + // This is an example of a functional test case. + // Use XCTAssert and related functions to verify your tests produce the correct results. + } + + func testPerformanceExample() { + // This is an example of a performance test case. + self.measure { + // Put the code you want to measure the time of here. + } + } + +} From f874c2fdccc8434ddf90df8c1e15a470c70cd67c Mon Sep 17 00:00:00 2001 From: Ryan Nystrom Date: Wed, 26 Sep 2018 19:35:49 -0400 Subject: [PATCH 3/4] add tests for notification db, fix settings, avoid dupe inbox requests --- .../NotificationModelController.swift | 3 + Classes/Systems/AppDelegate.swift | 7 +- Classes/Systems/BadgeNotifications.swift | 10 +-- Classes/Systems/LocalNotificationsCache.swift | 25 +++--- Classes/Systems/RootNavigationManager.swift | 4 +- Freetime.xcodeproj/project.pbxproj | 6 +- .../LocalNotificationCacheTests.swift | 86 +++++++++++++++++-- 7 files changed, 107 insertions(+), 34 deletions(-) diff --git a/Classes/Notifications/NotificationModelController.swift b/Classes/Notifications/NotificationModelController.swift index d21143525..b581b951e 100644 --- a/Classes/Notifications/NotificationModelController.swift +++ b/Classes/Notifications/NotificationModelController.swift @@ -51,6 +51,9 @@ final class NotificationModelController { width: CGFloat, completion: @escaping (Result<([NotificationViewModel], Int?)>) -> Void ) { + // hack to prevent double-fetching notifications when awaking from bg fetch + guard UIApplication.shared.applicationState != .background else { return } + let badge = githubClient.badge let contentSizeCategory = UIContentSizeCategory.preferred // TODO move handling + parsing to a single method? diff --git a/Classes/Systems/AppDelegate.swift b/Classes/Systems/AppDelegate.swift index 9ef8b65c9..e5166e4e6 100644 --- a/Classes/Systems/AppDelegate.swift +++ b/Classes/Systems/AppDelegate.swift @@ -95,13 +95,8 @@ class AppDelegate: UIResponder, UIApplicationDelegate { return false } - private var backgroundFetchClient: GithubClient? = nil func application(_ application: UIApplication, performFetchWithCompletionHandler completionHandler: @escaping (UIBackgroundFetchResult) -> Void) { - guard let userSession = sessionManager.focusedUserSession else { - return - } - backgroundFetchClient = newGithubClient(userSession: userSession) - backgroundFetchClient?.badge.fetch(application: application, handler: completionHandler) + rootNavigationManager.client?.badge.fetch(application: application, handler: completionHandler) } } diff --git a/Classes/Systems/BadgeNotifications.swift b/Classes/Systems/BadgeNotifications.swift index b221a7747..b6cb00e74 100644 --- a/Classes/Systems/BadgeNotifications.swift +++ b/Classes/Systems/BadgeNotifications.swift @@ -43,9 +43,9 @@ final class BadgeNotifications { UNUserNotificationCenter.current().getNotificationSettings { settings in DispatchQueue.main.async { switch settings.authorizationStatus { - case .notDetermined, .denied: + case .denied: callback(.error(nil)) - case .authorized, .provisional: + case .authorized, .provisional, .notDetermined: callback(.success((isBadgeEnabled, isLocalNotificationEnabled))) } } @@ -115,17 +115,15 @@ final class BadgeNotifications { completion: ((Bool) -> Void)? = nil ) { localNotificationsCache.update(notifications: notifications) { [weak self] filtered in - let changed = notifications.count != filtered.count - if showAlert && changed { + if showAlert { self?.sendLocalPush(for: filtered) } - completion?(changed) + completion?(filtered.count > 0) } } private func sendLocalPush(for notifications: [V3Notification]) { let center = UNUserNotificationCenter.current() - notifications.forEach { guard let type = NotificationType(rawValue: $0.subject.type.rawValue), let identifier = $0.subject.identifier diff --git a/Classes/Systems/LocalNotificationsCache.swift b/Classes/Systems/LocalNotificationsCache.swift index 2a26539ca..91f287bc8 100644 --- a/Classes/Systems/LocalNotificationsCache.swift +++ b/Classes/Systems/LocalNotificationsCache.swift @@ -65,18 +65,21 @@ final class LocalNotificationsCache { } } - // add latest notification ids in the db - let insertSanitized = map.keys.map { _ in "(?)" }.joined(separator: ", ") - try db.executeUpdate( - "insert into \(table) (\(apiCol)) values \(insertSanitized)", - values: apiIDs - ) + // only perform updates if there are new notifications + if map.count > 0 { + // add latest notification ids in the db + let insertSanitized = map.keys.map { _ in "(?)" }.joined(separator: ", ") + try db.executeUpdate( + "insert into \(table) (\(apiCol)) values \(insertSanitized)", + values: apiIDs + ) - // cap the local database to latest 1000 notifications - try db.executeUpdate( - "delete from \(table) where \(idCol) not in (select \(idCol) from \(table) order by \(idCol) desc limit 1000)", - values: nil - ) + // cap the local database to latest 1000 notifications + try db.executeUpdate( + "delete from \(table) where \(idCol) not in (select \(idCol) from \(table) order by \(idCol) desc limit 1000)", + values: nil + ) + } } catch { print("failed: \(error.localizedDescription)") } diff --git a/Classes/Systems/RootNavigationManager.swift b/Classes/Systems/RootNavigationManager.swift index c668afa21..4f761d3ef 100644 --- a/Classes/Systems/RootNavigationManager.swift +++ b/Classes/Systems/RootNavigationManager.swift @@ -22,7 +22,7 @@ final class RootNavigationManager: GitHubSessionListener { // keep alive between switching accounts private var settingsRootViewController: UIViewController? - private var lastClient: GithubClient? + private(set) var client: GithubClient? init( sessionManager: GitHubSessionManager, @@ -62,7 +62,7 @@ final class RootNavigationManager: GitHubSessionListener { guard let userSession = userSession else { return } let client = newGithubClient(userSession: userSession) - lastClient = client + self.client = client fetchUsernameForMigrationIfNecessary(client: client, userSession: userSession, sessionManager: sessionManager) diff --git a/Freetime.xcodeproj/project.pbxproj b/Freetime.xcodeproj/project.pbxproj index 71b636583..888c83a14 100644 --- a/Freetime.xcodeproj/project.pbxproj +++ b/Freetime.xcodeproj/project.pbxproj @@ -228,6 +228,7 @@ 29764C141FDC4DB60095FF95 /* SettingsLabel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 29764C131FDC4DB60095FF95 /* SettingsLabel.swift */; }; 2977788820B0DAD500F2AFC2 /* ViewMarkdownViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2977788720B0DAD500F2AFC2 /* ViewMarkdownViewController.swift */; }; 2977788A20B306F200F2AFC2 /* LabelLayoutManager.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2977788920B306F200F2AFC2 /* LabelLayoutManager.swift */; }; + 2977D8BF215AE12D0073F737 /* LocalNotificationCacheTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2977D8BE215AE12D0073F737 /* LocalNotificationCacheTests.swift */; }; 29792B191FFB10A3007A0C57 /* UIViewController+MessageAutocompleteControllerLayoutDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 29792B181FFB10A3007A0C57 /* UIViewController+MessageAutocompleteControllerLayoutDelegate.swift */; }; 29792B1B1FFB21AD007A0C57 /* AutocompleteController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 29792B1A1FFB21AD007A0C57 /* AutocompleteController.swift */; }; 29792B1D1FFB2FC6007A0C57 /* IssueManagingNavSectionController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 29792B1C1FFB2FC6007A0C57 /* IssueManagingNavSectionController.swift */; }; @@ -744,6 +745,7 @@ 29764C131FDC4DB60095FF95 /* SettingsLabel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SettingsLabel.swift; sourceTree = ""; }; 2977788720B0DAD500F2AFC2 /* ViewMarkdownViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ViewMarkdownViewController.swift; sourceTree = ""; }; 2977788920B306F200F2AFC2 /* LabelLayoutManager.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LabelLayoutManager.swift; sourceTree = ""; }; + 2977D8BE215AE12D0073F737 /* LocalNotificationCacheTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LocalNotificationCacheTests.swift; sourceTree = ""; }; 29792B181FFB10A3007A0C57 /* UIViewController+MessageAutocompleteControllerLayoutDelegate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "UIViewController+MessageAutocompleteControllerLayoutDelegate.swift"; sourceTree = ""; }; 29792B1A1FFB21AD007A0C57 /* AutocompleteController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AutocompleteController.swift; sourceTree = ""; }; 29792B1C1FFB2FC6007A0C57 /* IssueManagingNavSectionController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = IssueManagingNavSectionController.swift; sourceTree = ""; }; @@ -1547,9 +1549,10 @@ 29A476B11ED24D99005D0953 /* IssueTests.swift */, 29C2950D1EC7B43B00D46CD2 /* ListKitTestCase.swift */, 29C295091EC7AFA500D46CD2 /* ListTestKit.swift */, - BD89007D20B8844B0026013F /* NetworkingURLPathTests.swift */, + 2977D8BE215AE12D0073F737 /* LocalNotificationCacheTests.swift */, 49AF91B0204B416500DFF325 /* MergeTests.swift */, DC60C6CC1F98344B00241271 /* Mocks */, + BD89007D20B8844B0026013F /* NetworkingURLPathTests.swift */, 49D028FF1F91D90C00E39094 /* ReactionTests.swift */, DC60C6CF1F9836C500241271 /* Search Tests */, 9870B9071FC74E300009719C /* SecretsTests.swift */, @@ -3090,6 +3093,7 @@ 293A45771F296B7E00DD1006 /* ListKitTestCase.swift in Sources */, BD89007E20B8844B0026013F /* NetworkingURLPathTests.swift in Sources */, BD3761B0209E032500401DFB /* BookmarkNavigationItemTests.swift in Sources */, + 2977D8BF215AE12D0073F737 /* LocalNotificationCacheTests.swift in Sources */, DC5C02C51F9C6E3500E80B9F /* SearchQueryTests.swift in Sources */, 293A457E1F296BD500DD1006 /* API.swift in Sources */, 49AF91B1204B416500DFF325 /* MergeTests.swift in Sources */, diff --git a/FreetimeTests/LocalNotificationCacheTests.swift b/FreetimeTests/LocalNotificationCacheTests.swift index 6497b1961..24ad023a2 100644 --- a/FreetimeTests/LocalNotificationCacheTests.swift +++ b/FreetimeTests/LocalNotificationCacheTests.swift @@ -8,26 +8,96 @@ import XCTest +@testable import Freetime +@testable import GitHubAPI + class LocalNotificationCacheTests: XCTestCase { + var path: String = { + let path = NSSearchPathForDirectoriesInDomains(.documentDirectory, .userDomainMask, true)[0] + return "\(path)/read-notifications.sqlite" + }() + override func setUp() { // Put setup code here. This method is called before the invocation of each test method in the class. } override func tearDown() { - // Put teardown code here. This method is called after the invocation of each test method in the class. + try? FileManager.default.removeItem(atPath: path) + } + + func test_whenNoNotifications_thatNothingReturned() { + let cache = LocalNotificationsCache(path: path) + var executed = false + cache.update(notifications: []) { results in + executed = true + XCTAssertEqual(results.count, 0) + } + XCTAssertTrue(executed) + } + + func makeRepo() -> V3Repository { + return V3Repository( + description: nil, + fork: false, + fullName: "org/name", + id: 42, + name: "name", + owner: V3User( + avatarUrl: URL(string: "github.com")!, + id: 1, + login: "github", + siteAdmin: false, + type: .user + ), + isPrivate: false + ) } - func testExample() { - // This is an example of a functional test case. - // Use XCTAssert and related functions to verify your tests produce the correct results. + func makeNotificaction(id: String, title: String) -> V3Notification { + return V3Notification( + id: id, + lastReadAt: nil, + reason: .assign, + repository: makeRepo(), + subject: V3NotificationSubject(title: title, type: .issue, url: nil), + unread: true, + updatedAt: Date() + ) } - func testPerformanceExample() { - // This is an example of a performance test case. - self.measure { - // Put the code you want to measure the time of here. + func test_whenUpdating_thatReadFirstTime_thenSecondCallEmpty() { + let cache = LocalNotificationsCache(path: path) + let n1 = [ + makeNotificaction(id: "123", title: "foo") + ] + var executions = 0 + + cache.update(notifications: n1) { results in + executions += 1 + XCTAssertEqual(results.count, 1) + } + + let n2 = [ + makeNotificaction(id: "123", title: "foo"), + makeNotificaction(id: "456", title: "bar") + ] + cache.update(notifications: n2) { results in + executions += 1 + XCTAssertEqual(results.count, 1) } + + let n3 = [ + makeNotificaction(id: "123", title: "foo"), + makeNotificaction(id: "456", title: "bar") + ] + + cache.update(notifications: n3) { results in + executions += 1 + XCTAssertEqual(results.count, 0) + } + + XCTAssertEqual(executions, 3) } } From 11ddb70aa8c8723079e24759864e1449fcfb84ce Mon Sep 17 00:00:00 2001 From: Ryan Nystrom Date: Fri, 28 Sep 2018 12:52:54 -0400 Subject: [PATCH 4/4] better tests, fix dupe insert bug --- Classes/Systems/LocalNotificationsCache.swift | 2 +- .../LocalNotificationCacheTests.swift | 26 ++++++++++++------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/Classes/Systems/LocalNotificationsCache.swift b/Classes/Systems/LocalNotificationsCache.swift index 91f287bc8..83124ed31 100644 --- a/Classes/Systems/LocalNotificationsCache.swift +++ b/Classes/Systems/LocalNotificationsCache.swift @@ -71,7 +71,7 @@ final class LocalNotificationsCache { let insertSanitized = map.keys.map { _ in "(?)" }.joined(separator: ", ") try db.executeUpdate( "insert into \(table) (\(apiCol)) values \(insertSanitized)", - values: apiIDs + values: map.keys.map { $0 } ) // cap the local database to latest 1000 notifications diff --git a/FreetimeTests/LocalNotificationCacheTests.swift b/FreetimeTests/LocalNotificationCacheTests.swift index 24ad023a2..a20e51d84 100644 --- a/FreetimeTests/LocalNotificationCacheTests.swift +++ b/FreetimeTests/LocalNotificationCacheTests.swift @@ -13,21 +13,25 @@ import XCTest class LocalNotificationCacheTests: XCTestCase { - var path: String = { + func path(for test: String) -> String { + let clean = test.replacingOccurrences(of: "()", with: "") let path = NSSearchPathForDirectoriesInDomains(.documentDirectory, .userDomainMask, true)[0] - return "\(path)/read-notifications.sqlite" - }() - - override func setUp() { - // Put setup code here. This method is called before the invocation of each test method in the class. + return "\(path)/\(clean).sqlite" } - override func tearDown() { - try? FileManager.default.removeItem(atPath: path) + func clear(for test: String) { + let path = self.path(for: test) + do { + try FileManager.default.removeItem(atPath: path) + } catch { + print("\(error.localizedDescription)\npath: \(path)") + } } func test_whenNoNotifications_thatNothingReturned() { - let cache = LocalNotificationsCache(path: path) + clear(for: #function) + + let cache = LocalNotificationsCache(path: path(for: #function)) var executed = false cache.update(notifications: []) { results in executed = true @@ -67,7 +71,9 @@ class LocalNotificationCacheTests: XCTestCase { } func test_whenUpdating_thatReadFirstTime_thenSecondCallEmpty() { - let cache = LocalNotificationsCache(path: path) + clear(for: #function) + + let cache = LocalNotificationsCache(path: path(for: #function)) let n1 = [ makeNotificaction(id: "123", title: "foo") ]