Skip to content
This repository was archived by the owner on Sep 20, 2023. It is now read-only.

Commit cf09107

Browse files
rnystromrnystrom
authored
Fortify local notifications code (#2191)
* move path/defaults to be injected * remove unused defaults * add tests for notification db, fix settings, avoid dupe inbox requests * better tests, fix dupe insert bug
1 parent 3eb648a commit cf09107

File tree

7 files changed

+145
-31
lines changed

7 files changed

+145
-31
lines changed

‎Classes/Notifications/NotificationModelController.swift‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,9 @@ final class NotificationModelController {
5151
width: CGFloat,
5252
completion: @escaping (Result<([NotificationViewModel], Int?)>) -> Void
5353
) {
54+
// hack to prevent double-fetching notifications when awaking from bg fetch
55+
guard UIApplication.shared.applicationState != .background else { return }
56+
5457
let badge = githubClient.badge
5558
let contentSizeCategory = UIContentSizeCategory.preferred
5659
// TODO move handling + parsing to a single method?

‎Classes/Systems/AppDelegate.swift‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -95,13 +95,8 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
9595
return false
9696
}
9797

98-
private var backgroundFetchClient: GithubClient? = nil
9998
func application(_ application: UIApplication, performFetchWithCompletionHandler completionHandler: @escaping (UIBackgroundFetchResult) -> Void) {
100-
guard let userSession = sessionManager.focusedUserSession else {
101-
return
102-
}
103-
backgroundFetchClient = newGithubClient(userSession: userSession)
104-
backgroundFetchClient?.badge.fetch(application: application, handler: completionHandler)
99+
rootNavigationManager.client?.badge.fetch(application: application, handler: completionHandler)
105100
}
106101

107102
}

‎Classes/Systems/BadgeNotifications.swift‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,9 @@ final class BadgeNotifications {
4343
UNUserNotificationCenter.current().getNotificationSettings { settings in
4444
DispatchQueue.main.async {
4545
switch settings.authorizationStatus {
46-
case .notDetermined, .denied:
46+
case .denied:
4747
callback(.error(nil))
48-
case .authorized, .provisional:
48+
case .authorized, .provisional, .notDetermined:
4949
callback(.success((isBadgeEnabled, isLocalNotificationEnabled)))
5050
}
5151
}
@@ -115,17 +115,15 @@ final class BadgeNotifications {
115115
completion: ((Bool) -> Void)? = nil
116116
) {
117117
localNotificationsCache.update(notifications: notifications) { [weak self] filtered in
118-
let changed = notifications.count != filtered.count
119-
if showAlert && changed {
118+
if showAlert {
120119
self?.sendLocalPush(for: filtered)
121120
}
122-
completion?(changed)
121+
completion?(filtered.count > 0)
123122
}
124123
}
125124

126125
private func sendLocalPush(for notifications: [V3Notification]) {
127126
let center = UNUserNotificationCenter.current()
128-
129127
notifications.forEach {
130128
guard let type = NotificationType(rawValue: $0.subject.type.rawValue),
131129
let identifier = $0.subject.identifier

‎Classes/Systems/LocalNotificationsCache.swift‎

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,18 +12,20 @@ import GitHubAPI
1212

1313
final class LocalNotificationsCache {
1414

15-
private let path: String = {
15+
private static let sqlitePath: String = {
1616
let path = NSSearchPathForDirectoriesInDomains(.documentDirectory, .userDomainMask, true)[0]
1717
return "\(path)/read-notifications.sqlite"
1818
}()
19+
20+
private let path: String
1921
private lazy var queue: FMDatabaseQueue = {
2022
return FMDatabaseQueue(path: self.path)
2123
}()
22-
private let defaults = UserDefaults.standard
23-
private let setupKey = "com.whoisryannystrom.freetime.local-notifications.setup"
2424

25-
private var isFirstSetup: Bool {
26-
return defaults.bool(forKey: setupKey)
25+
init(
26+
path: String = LocalNotificationsCache.sqlitePath
27+
) {
28+
self.path = path
2729
}
2830

2931
func update(
@@ -63,18 +65,21 @@ final class LocalNotificationsCache {
6365
}
6466
}
6567

66-
// add latest notification ids in the db
67-
let insertSanitized = map.keys.map { _ in "(?)" }.joined(separator: ", ")
68-
try db.executeUpdate(
69-
"insert into \(table) (\(apiCol)) values \(insertSanitized)",
70-
values: apiIDs
71-
)
68+
// only perform updates if there are new notifications
69+
if map.count > 0 {
70+
// add latest notification ids in the db
71+
let insertSanitized = map.keys.map { _ in "(?)" }.joined(separator: ", ")
72+
try db.executeUpdate(
73+
"insert into \(table) (\(apiCol)) values \(insertSanitized)",
74+
values: map.keys.map { $0 }
75+
)
7276

73-
// cap the local database to latest 1000 notifications
74-
try db.executeUpdate(
75-
"delete from \(table) where \(idCol) not in (select \(idCol) from \(table) order by \(idCol) desc limit 1000)",
76-
values: nil
77-
)
77+
// cap the local database to latest 1000 notifications
78+
try db.executeUpdate(
79+
"delete from \(table) where \(idCol) not in (select \(idCol) from \(table) order by \(idCol) desc limit 1000)",
80+
values: nil
81+
)
82+
}
7883
} catch {
7984
print("failed: \(error.localizedDescription)")
8085
}

‎Classes/Systems/RootNavigationManager.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ final class RootNavigationManager: GitHubSessionListener {
2222
// keep alive between switching accounts
2323
private var settingsRootViewController: UIViewController?
2424

25-
private var lastClient: GithubClient?
25+
private(set) var client: GithubClient?
2626

2727
init(
2828
sessionManager: GitHubSessionManager,
@@ -62,7 +62,7 @@ final class RootNavigationManager: GitHubSessionListener {
6262
guard let userSession = userSession else { return }
6363

6464
let client = newGithubClient(userSession: userSession)
65-
lastClient = client
65+
self.client = client
6666

6767
fetchUsernameForMigrationIfNecessary(client: client, userSession: userSession, sessionManager: sessionManager)
6868

‎Freetime.xcodeproj/project.pbxproj‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,7 @@
228228
29764C141FDC4DB60095FF95 /* SettingsLabel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 29764C131FDC4DB60095FF95 /* SettingsLabel.swift */; };
229229
2977788820B0DAD500F2AFC2 /* ViewMarkdownViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2977788720B0DAD500F2AFC2 /* ViewMarkdownViewController.swift */; };
230230
2977788A20B306F200F2AFC2 /* LabelLayoutManager.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2977788920B306F200F2AFC2 /* LabelLayoutManager.swift */; };
231+
2977D8BF215AE12D0073F737 /* LocalNotificationCacheTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2977D8BE215AE12D0073F737 /* LocalNotificationCacheTests.swift */; };
231232
29792B191FFB10A3007A0C57 /* UIViewController+MessageAutocompleteControllerLayoutDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 29792B181FFB10A3007A0C57 /* UIViewController+MessageAutocompleteControllerLayoutDelegate.swift */; };
232233
29792B1B1FFB21AD007A0C57 /* AutocompleteController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 29792B1A1FFB21AD007A0C57 /* AutocompleteController.swift */; };
233234
29792B1D1FFB2FC6007A0C57 /* IssueManagingNavSectionController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 29792B1C1FFB2FC6007A0C57 /* IssueManagingNavSectionController.swift */; };
@@ -744,6 +745,7 @@
744745
29764C131FDC4DB60095FF95 /* SettingsLabel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SettingsLabel.swift; sourceTree = "<group>"; };
745746
2977788720B0DAD500F2AFC2 /* ViewMarkdownViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ViewMarkdownViewController.swift; sourceTree = "<group>"; };
746747
2977788920B306F200F2AFC2 /* LabelLayoutManager.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LabelLayoutManager.swift; sourceTree = "<group>"; };
748+
2977D8BE215AE12D0073F737 /* LocalNotificationCacheTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LocalNotificationCacheTests.swift; sourceTree = "<group>"; };
747749
29792B181FFB10A3007A0C57 /* UIViewController+MessageAutocompleteControllerLayoutDelegate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "UIViewController+MessageAutocompleteControllerLayoutDelegate.swift"; sourceTree = "<group>"; };
748750
29792B1A1FFB21AD007A0C57 /* AutocompleteController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AutocompleteController.swift; sourceTree = "<group>"; };
749751
29792B1C1FFB2FC6007A0C57 /* IssueManagingNavSectionController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = IssueManagingNavSectionController.swift; sourceTree = "<group>"; };
@@ -1547,9 +1549,10 @@
15471549
29A476B11ED24D99005D0953 /* IssueTests.swift */,
15481550
29C2950D1EC7B43B00D46CD2 /* ListKitTestCase.swift */,
15491551
29C295091EC7AFA500D46CD2 /* ListTestKit.swift */,
1550-
BD89007D20B8844B0026013F /* NetworkingURLPathTests.swift */,
1552+
2977D8BE215AE12D0073F737 /* LocalNotificationCacheTests.swift */,
15511553
49AF91B0204B416500DFF325 /* MergeTests.swift */,
15521554
DC60C6CC1F98344B00241271 /* Mocks */,
1555+
BD89007D20B8844B0026013F /* NetworkingURLPathTests.swift */,
15531556
49D028FF1F91D90C00E39094 /* ReactionTests.swift */,
15541557
DC60C6CF1F9836C500241271 /* Search Tests */,
15551558
9870B9071FC74E300009719C /* SecretsTests.swift */,
@@ -3094,6 +3097,7 @@
30943097
293A45771F296B7E00DD1006 /* ListKitTestCase.swift in Sources */,
30953098
BD89007E20B8844B0026013F /* NetworkingURLPathTests.swift in Sources */,
30963099
BD3761B0209E032500401DFB /* BookmarkNavigationItemTests.swift in Sources */,
3100+
2977D8BF215AE12D0073F737 /* LocalNotificationCacheTests.swift in Sources */,
30973101
DC5C02C51F9C6E3500E80B9F /* SearchQueryTests.swift in Sources */,
30983102
293A457E1F296BD500DD1006 /* API.swift in Sources */,
30993103
49AF91B1204B416500DFF325 /* MergeTests.swift in Sources */,

‎FreetimeTests/LocalNotificationCacheTests.swift‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
//
2+
// LocalNotificationCacheTests.swift
3+
// FreetimeTests
4+
//
5+
// Created by Ryan Nystrom on 9/25/18.
6+
// Copyright © 2018 Ryan Nystrom. All rights reserved.
7+
//
8+
9+
import XCTest
10+
11+
@testable import Freetime
12+
@testable import GitHubAPI
13+
14+
class LocalNotificationCacheTests: XCTestCase {
15+
16+
func path(for test: String) -> String {
17+
let clean = test.replacingOccurrences(of: "()", with: "")
18+
let path = NSSearchPathForDirectoriesInDomains(.documentDirectory, .userDomainMask, true)[0]
19+
return "\(path)/\(clean).sqlite"
20+
}
21+
22+
func clear(for test: String) {
23+
let path = self.path(for: test)
24+
do {
25+
try FileManager.default.removeItem(atPath: path)
26+
} catch {
27+
print("\(error.localizedDescription)\npath: \(path)")
28+
}
29+
}
30+
31+
func test_whenNoNotifications_thatNothingReturned() {
32+
clear(for: #function)
33+
34+
let cache = LocalNotificationsCache(path: path(for: #function))
35+
var executed = false
36+
cache.update(notifications: []) { results in
37+
executed = true
38+
XCTAssertEqual(results.count, 0)
39+
}
40+
XCTAssertTrue(executed)
41+
}
42+
43+
func makeRepo() -> V3Repository {
44+
return V3Repository(
45+
description: nil,
46+
fork: false,
47+
fullName: "org/name",
48+
id: 42,
49+
name: "name",
50+
owner: V3User(
51+
avatarUrl: URL(string: "github.com")!,
52+
id: 1,
53+
login: "github",
54+
siteAdmin: false,
55+
type: .user
56+
),
57+
isPrivate: false
58+
)
59+
}
60+
61+
func makeNotificaction(id: String, title: String) -> V3Notification {
62+
return V3Notification(
63+
id: id,
64+
lastReadAt: nil,
65+
reason: .assign,
66+
repository: makeRepo(),
67+
subject: V3NotificationSubject(title: title, type: .issue, url: nil),
68+
unread: true,
69+
updatedAt: Date()
70+
)
71+
}
72+
73+
func test_whenUpdating_thatReadFirstTime_thenSecondCallEmpty() {
74+
clear(for: #function)
75+
76+
let cache = LocalNotificationsCache(path: path(for: #function))
77+
let n1 = [
78+
makeNotificaction(id: "123", title: "foo")
79+
]
80+
var executions = 0
81+
82+
cache.update(notifications: n1) { results in
83+
executions += 1
84+
XCTAssertEqual(results.count, 1)
85+
}
86+
87+
let n2 = [
88+
makeNotificaction(id: "123", title: "foo"),
89+
makeNotificaction(id: "456", title: "bar")
90+
]
91+
cache.update(notifications: n2) { results in
92+
executions += 1
93+
XCTAssertEqual(results.count, 1)
94+
}
95+
96+
let n3 = [
97+
makeNotificaction(id: "123", title: "foo"),
98+
makeNotificaction(id: "456", title: "bar")
99+
]
100+
101+
cache.update(notifications: n3) { results in
102+
executions += 1
103+
XCTAssertEqual(results.count, 0)
104+
}
105+
106+
XCTAssertEqual(executions, 3)
107+
}
108+
109+
}

0 commit comments

Comments
 (0)