From 89c5a5d9998855d8c39f39485b5f1eb9cabab264 Mon Sep 17 00:00:00 2001 From: jack Date: Sun, 26 Jul 2026 18:16:45 +0200 Subject: [PATCH] Inject the previews preference instead of reading shared defaults MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught a real problem this introduced. `NotificationServiceTests` already asserted the full-preview title and body, and both it and the new redaction tests read `NotificationPrivacySettings` from `UserDefaults.standard` in the same process. Whichever ran second depended on the other's cleanup, so it passed locally and failed in CI: XCTAssertEqual failed: ("🔒 new dm") is not equal to ("🔒 DM from Alice") Fixed at the source rather than by ordering or serialization. `NotificationService` now takes a `hidePreviewsProvider`, defaulting to the real preference, so each test states which behavior it asserts. The pre-existing test asks for previews shown and gains a redacted counterpart; the redaction tests no longer touch the shared store. `NotificationPrivacySettings` also gained store-injecting accessors so the default-value and round-trip assertions can use an isolated suite rather than mutating preferences other tests read. Co-Authored-By: Claude Opus 5 --- .../NotificationPrivacySettings.swift | 20 ++- bitchat/Services/NotificationService.swift | 13 +- .../Services/NotificationRedactionTests.swift | 145 ++++++++++-------- .../Services/NotificationServiceTests.swift | 26 +++- 4 files changed, 128 insertions(+), 76 deletions(-) diff --git a/bitchat/Services/NotificationPrivacySettings.swift b/bitchat/Services/NotificationPrivacySettings.swift index 4fb4863f..da67c30b 100644 --- a/bitchat/Services/NotificationPrivacySettings.swift +++ b/bitchat/Services/NotificationPrivacySettings.swift @@ -22,13 +22,23 @@ enum NotificationPrivacySettings { private static let hidePreviewsKey = "notifications.hideMessagePreviews" static var hideMessagePreviews: Bool { - get { UserDefaults.standard.object(forKey: hidePreviewsKey) as? Bool ?? true } - set { UserDefaults.standard.set(newValue, forKey: hidePreviewsKey) } + get { hideMessagePreviews(in: .standard) } + set { setHideMessagePreviews(newValue, in: .standard) } + } + + /// Store-injecting forms, so tests can assert the default and both settings + /// without touching the shared preferences other tests read. + static func hideMessagePreviews(in defaults: UserDefaults) -> Bool { + defaults.object(forKey: hidePreviewsKey) as? Bool ?? true + } + + static func setHideMessagePreviews(_ hide: Bool, in defaults: UserDefaults) { + defaults.set(hide, forKey: hidePreviewsKey) } /// Panic-wipe hook. Removing the key restores the hidden default, so a - /// wiped device cannot come back quieter than a fresh install. - static func reset() { - UserDefaults.standard.removeObject(forKey: hidePreviewsKey) + /// wiped device cannot come back louder than a fresh install. + static func reset(in defaults: UserDefaults = .standard) { + defaults.removeObject(forKey: hidePreviewsKey) } } diff --git a/bitchat/Services/NotificationService.swift b/bitchat/Services/NotificationService.swift index 4ad85f09..dc8ef193 100644 --- a/bitchat/Services/NotificationService.swift +++ b/bitchat/Services/NotificationService.swift @@ -114,8 +114,14 @@ final class NotificationService { } /// Whether delivered alerts must withhold sender, content, and geohash. + /// + /// Injected rather than read from the preference directly so tests state + /// which behavior they are asserting instead of inheriting whatever the + /// shared preference happens to hold when they run. + private let hidePreviewsProvider: () -> Bool + private var hidePreviews: Bool { - NotificationPrivacySettings.hideMessagePreviews + hidePreviewsProvider() } private let isRunningTestsProvider: () -> Bool @@ -129,6 +135,7 @@ final class NotificationService { } private init() { + self.hidePreviewsProvider = { NotificationPrivacySettings.hideMessagePreviews } self.isRunningTestsProvider = { let env = ProcessInfo.processInfo.environment return NSClassFromString("XCTestCase") != nil || @@ -153,12 +160,14 @@ final class NotificationService { isRunningTestsProvider: @escaping () -> Bool, authorizer: NotificationAuthorizing, requestDeliverer: NotificationRequestDelivering, - categoryRegistrar: NotificationCategoryRegistering = NoopNotificationCategoryRegistrar() + categoryRegistrar: NotificationCategoryRegistering = NoopNotificationCategoryRegistrar(), + hidePreviewsProvider: @escaping () -> Bool = { NotificationPrivacySettings.hideMessagePreviews } ) { self.isRunningTestsProvider = isRunningTestsProvider self.authorizer = authorizer self.requestDeliverer = requestDeliverer self.categoryRegistrar = categoryRegistrar + self.hidePreviewsProvider = hidePreviewsProvider } func requestAuthorization() { diff --git a/bitchatTests/Services/NotificationRedactionTests.swift b/bitchatTests/Services/NotificationRedactionTests.swift index 165b2c5d..be5c8109 100644 --- a/bitchatTests/Services/NotificationRedactionTests.swift +++ b/bitchatTests/Services/NotificationRedactionTests.swift @@ -9,9 +9,10 @@ import UserNotifications /// a table narrated conversations to anyone looking at it. These cover the /// redaction that is now the default. /// -/// Serialized because the preference lives in `UserDefaults.standard`; parallel -/// cases would race each other's saves. -@Suite(.serialized) +/// The preference is injected rather than written to shared preferences. These +/// run in the same process as `NotificationServiceTests`, and mutating the +/// global store made whichever ran second depend on the other's cleanup — which +/// passed locally and failed in CI. struct NotificationRedactionTests { private final class RecordingDeliverer: NotificationRequestDelivering { var requests: [UNNotificationRequest] = [] @@ -30,92 +31,100 @@ struct NotificationRedactionTests { } } - /// Runs `body` with the preference forced to `hidePreviews`, restoring - /// whatever the environment had afterwards. - private func withHidePreviews( - _ hidePreviews: Bool, - _ body: (NotificationService, RecordingDeliverer) throws -> Void - ) rethrows { - let original = NotificationPrivacySettings.hideMessagePreviews - defer { NotificationPrivacySettings.hideMessagePreviews = original } - NotificationPrivacySettings.hideMessagePreviews = hidePreviews - + private func makeService( + hidePreviews: Bool + ) -> (NotificationService, RecordingDeliverer) { let deliverer = RecordingDeliverer() let service = NotificationService( isRunningTestsProvider: { false }, authorizer: StubAuthorizer(), - requestDeliverer: deliverer + requestDeliverer: deliverer, + hidePreviewsProvider: { hidePreviews } ) - try body(service, deliverer) + return (service, deliverer) + } + + private func isolatedDefaults() -> UserDefaults { + UserDefaults(suiteName: "bitchat.tests.notifications.\(UUID().uuidString)")! } @Test func previewsAreHiddenByDefault() { // A fresh install must start quiet rather than opt-in quiet. - UserDefaults.standard.removeObject(forKey: "notifications.hideMessagePreviews") - #expect(NotificationPrivacySettings.hideMessagePreviews) + #expect(NotificationPrivacySettings.hideMessagePreviews(in: isolatedDefaults())) } - @Test func redactedDirectMessageWithholdsSenderAndBody() { - withHidePreviews(true) { service, deliverer in - service.sendPrivateMessageNotification( - from: "alice", - message: "meet at the north gate", - peerID: PeerID(str: "00112233445566ff") - ) + @Test func theSettingRoundTrips() { + let defaults = isolatedDefaults() - let content = try! #require(deliverer.requests.first).content - #expect(!content.title.contains("alice")) - #expect(!content.body.contains("north gate")) - #expect(!content.title.isEmpty) - // Still routable: userInfo is never rendered on the lock screen. - #expect(content.userInfo["peerID"] as? String == "00112233445566ff") - } + NotificationPrivacySettings.setHideMessagePreviews(false, in: defaults) + #expect(!NotificationPrivacySettings.hideMessagePreviews(in: defaults)) + + // Panic wipe restores the safe default rather than the last choice. + NotificationPrivacySettings.reset(in: defaults) + #expect(NotificationPrivacySettings.hideMessagePreviews(in: defaults)) } - @Test func redactedMentionWithholdsSenderAndBody() { - withHidePreviews(true) { service, deliverer in - service.sendMentionNotification(from: "bob", message: "regroup now") + @Test func redactedDirectMessageWithholdsSenderAndBody() throws { + let (service, deliverer) = makeService(hidePreviews: true) - let content = try! #require(deliverer.requests.first).content - #expect(!content.title.contains("bob")) - #expect(!content.body.contains("regroup")) - } + service.sendPrivateMessageNotification( + from: "alice", + message: "meet at the north gate", + peerID: PeerID(str: "00112233445566ff") + ) + + let content = try #require(deliverer.requests.first).content + #expect(!content.title.contains("alice")) + #expect(!content.body.contains("north gate")) + #expect(!content.title.isEmpty) + // Still routable: userInfo is never rendered on the lock screen. + #expect(content.userInfo["peerID"] as? String == "00112233445566ff") } - @Test func redactedGeohashActivityWithholdsTheGeohash() { - withHidePreviews(true) { service, deliverer in - service.sendGeohashActivityNotification( - geohash: "u4pruyd", - bodyPreview: "someone said something" - ) + @Test func redactedMentionWithholdsSenderAndBody() throws { + let (service, deliverer) = makeService(hidePreviews: true) - let content = try! #require(deliverer.requests.first).content - #expect(!content.title.contains("u4pruyd")) - #expect(!content.body.contains("someone said")) - // The deep link still carries it: tapping must land in the channel. - #expect(content.userInfo["deeplink"] as? String == "bitchat://geohash/u4pruyd") - } + service.sendMentionNotification(from: "bob", message: "regroup now") + + let content = try #require(deliverer.requests.first).content + #expect(!content.title.contains("bob")) + #expect(!content.body.contains("regroup")) + } + + @Test func redactedGeohashActivityWithholdsTheGeohash() throws { + let (service, deliverer) = makeService(hidePreviews: true) + + service.sendGeohashActivityNotification( + geohash: "u4pruyd", + bodyPreview: "someone said something" + ) + + let content = try #require(deliverer.requests.first).content + #expect(!content.title.contains("u4pruyd")) + #expect(!content.body.contains("someone said")) + // The deep link still carries it: tapping must land in the channel. + #expect(content.userInfo["deeplink"] as? String == "bitchat://geohash/u4pruyd") } @Test func previewsShownWhenTheSettingIsOff() { - withHidePreviews(false) { service, deliverer in - service.sendPrivateMessageNotification( - from: "alice", - message: "meet at the north gate", - peerID: PeerID(str: "00112233445566ff") - ) - service.sendGeohashActivityNotification( - geohash: "u4pruyd", - bodyPreview: "someone said something" - ) + let (service, deliverer) = makeService(hidePreviews: false) - #expect(deliverer.requests.count == 2) - let dm = deliverer.requests[0].content - #expect(dm.title.contains("alice")) - #expect(dm.body == "meet at the north gate") - let geo = deliverer.requests[1].content - #expect(geo.title.contains("u4pruyd")) - #expect(geo.body == "someone said something") - } + service.sendPrivateMessageNotification( + from: "alice", + message: "meet at the north gate", + peerID: PeerID(str: "00112233445566ff") + ) + service.sendGeohashActivityNotification( + geohash: "u4pruyd", + bodyPreview: "someone said something" + ) + + #expect(deliverer.requests.count == 2) + let dm = deliverer.requests[0].content + #expect(dm.title.contains("alice")) + #expect(dm.body == "meet at the north gate") + let geo = deliverer.requests[1].content + #expect(geo.title.contains("u4pruyd")) + #expect(geo.body == "someone said something") } } diff --git a/bitchatTests/Services/NotificationServiceTests.swift b/bitchatTests/Services/NotificationServiceTests.swift index 9ffad1f4..384f0075 100644 --- a/bitchatTests/Services/NotificationServiceTests.swift +++ b/bitchatTests/Services/NotificationServiceTests.swift @@ -56,12 +56,15 @@ final class NotificationServiceTests: XCTestCase { XCTAssertNil(request?.trigger) } + /// Previews shown: the opt-in behavior. Stated explicitly rather than + /// inherited from the shared preference, which now defaults to hidden. func test_sendPrivateMessageNotification_populatesPeerMetadata() { let deliverer = RecordingNotificationRequestDeliverer() let service = NotificationService( isRunningTestsProvider: { false }, authorizer: RecordingNotificationAuthorizer(), - requestDeliverer: deliverer + requestDeliverer: deliverer, + hidePreviewsProvider: { false } ) let peerID = PeerID(str: "deadbeefdeadbeef") @@ -74,6 +77,27 @@ final class NotificationServiceTests: XCTestCase { XCTAssertEqual(request?.content.userInfo["senderName"] as? String, "Alice") } + /// Previews hidden: the default. The routing payload has to survive + /// redaction, or tapping the alert would not open the conversation. + func test_sendPrivateMessageNotification_withPreviewsHidden_keepsRoutingButDropsContent() { + let deliverer = RecordingNotificationRequestDeliverer() + let service = NotificationService( + isRunningTestsProvider: { false }, + authorizer: RecordingNotificationAuthorizer(), + requestDeliverer: deliverer, + hidePreviewsProvider: { true } + ) + let peerID = PeerID(str: "deadbeefdeadbeef") + + service.sendPrivateMessageNotification(from: "Alice", message: "hi", peerID: peerID) + + let request = deliverer.requests.singleValue + XCTAssertFalse(request?.content.title.contains("Alice") ?? true) + XCTAssertFalse(request?.content.body.contains("hi") ?? true) + XCTAssertFalse(request?.content.title.isEmpty ?? true) + XCTAssertEqual(request?.content.userInfo["peerID"] as? String, peerID.id) + } + func test_wrapperNotifications_setExpectedIdentifiersAndDeepLinks() { let deliverer = RecordingNotificationRequestDeliverer() let service = NotificationService(