From 857444bfff7ff46efbe1bc14f1061c87f24221ed Mon Sep 17 00:00:00 2001 From: Tony Li Date: Mon, 8 Jun 2026 22:49:33 +1200 Subject: [PATCH] Wire the app-side uploader lifecycle teardown Tears the Blog-keyed, process-wide uploader down on logout, site removal, and Jetpack disconnect, and adds the registry and tracker-adapter tests. --- ...MediaTrackerAdapterUploadEventsTests.swift | 173 ++++++++++++++++++ .../Media/MediaUploaderRegistryTests.swift | 58 ++++++ .../Services/SiteManagementService.swift | 7 + WordPress/Classes/Utility/AccountHelper.swift | 3 + .../BlogDetailsViewController+Swift.swift | 6 + .../JetpackConnectionViewController.swift | 6 + 6 files changed, 253 insertions(+) create mode 100644 Tests/KeystoneTests/Tests/Features/Media/MediaTrackerAdapterUploadEventsTests.swift create mode 100644 Tests/KeystoneTests/Tests/Features/Media/MediaUploaderRegistryTests.swift diff --git a/Tests/KeystoneTests/Tests/Features/Media/MediaTrackerAdapterUploadEventsTests.swift b/Tests/KeystoneTests/Tests/Features/Media/MediaTrackerAdapterUploadEventsTests.swift new file mode 100644 index 000000000000..976dcb331d9d --- /dev/null +++ b/Tests/KeystoneTests/Tests/Features/Media/MediaTrackerAdapterUploadEventsTests.swift @@ -0,0 +1,173 @@ +import Foundation +import CoreData +import Testing +import WordPressData +@testable import WordPress +@testable import WordPressMediaLibrary + +@Suite("MediaTrackerAdapter upload events", .serialized) +@MainActor +struct MediaTrackerAdapterUploadEventsTests { + let contextManager = ContextManager.forTesting() + var mainContext: NSManagedObjectContext { contextManager.mainContext } + + private func makeAdapter() -> MediaTrackerAdapter { + let blog = ModelTestHelper.insertDotComBlog(context: mainContext) + contextManager.saveContextAndWait(mainContext) + return MediaTrackerAdapter(blog: blog, baseProperties: ["is_v2": "1"]) + } + + @Test("Photo from PHPicker maps to AddedPhotoViaDeviceLibrary") + func photoLibraryImage() { + TestAnalyticsTracker.setup() + defer { TestAnalyticsTracker.tearDown() } + + makeAdapter().track(.mediaLibraryAdded(source: .photoLibrary, kind: .image)) + + #expect(TestAnalyticsTracker.tracked.last?.stat == .mediaLibraryAddedPhotoViaDeviceLibrary) + } + + @Test("Video from PHPicker maps to AddedVideoViaDeviceLibrary") + func photoLibraryVideo() { + TestAnalyticsTracker.setup() + defer { TestAnalyticsTracker.tearDown() } + + makeAdapter().track(.mediaLibraryAdded(source: .photoLibrary, kind: .video)) + + #expect(TestAnalyticsTracker.tracked.last?.stat == .mediaLibraryAddedVideoViaDeviceLibrary) + } + + @Test("Photo from camera maps to AddedPhotoViaCamera") + func cameraImage() { + TestAnalyticsTracker.setup() + defer { TestAnalyticsTracker.tearDown() } + + makeAdapter().track(.mediaLibraryAdded(source: .camera, kind: .image)) + + #expect(TestAnalyticsTracker.tracked.last?.stat == .mediaLibraryAddedPhotoViaCamera) + } + + @Test("Video from camera maps to AddedVideoViaCamera") + func cameraVideo() { + TestAnalyticsTracker.setup() + defer { TestAnalyticsTracker.tearDown() } + + makeAdapter().track(.mediaLibraryAdded(source: .camera, kind: .video)) + + #expect(TestAnalyticsTracker.tracked.last?.stat == .mediaLibraryAddedVideoViaCamera) + } + + @Test("Photo from file picker maps to AddedPhotoViaOtherApps") + func otherAppsImage() { + TestAnalyticsTracker.setup() + defer { TestAnalyticsTracker.tearDown() } + + makeAdapter().track(.mediaLibraryAdded(source: .otherApps, kind: .image)) + + #expect(TestAnalyticsTracker.tracked.last?.stat == .mediaLibraryAddedPhotoViaOtherApps) + } + + @Test("Video from file picker maps to AddedVideoViaOtherApps") + func otherAppsVideo() { + TestAnalyticsTracker.setup() + defer { TestAnalyticsTracker.tearDown() } + + makeAdapter().track(.mediaLibraryAdded(source: .otherApps, kind: .video)) + + #expect(TestAnalyticsTracker.tracked.last?.stat == .mediaLibraryAddedVideoViaOtherApps) + } + + @Test("Document is silently dropped") + func documentDropped() { + TestAnalyticsTracker.setup() + defer { TestAnalyticsTracker.tearDown() } + + makeAdapter().track(.mediaLibraryAdded(source: .otherApps, kind: .document)) + + #expect(TestAnalyticsTracker.tracked.isEmpty) + } + + @Test("Audio is silently dropped") + func audioDropped() { + TestAnalyticsTracker.setup() + defer { TestAnalyticsTracker.tearDown() } + + makeAdapter().track(.mediaLibraryAdded(source: .photoLibrary, kind: .audio)) + + #expect(TestAnalyticsTracker.tracked.isEmpty) + } + + @Test("Retry maps to mediaLibraryUploadMediaRetried") + func retryEvent() { + TestAnalyticsTracker.setup() + defer { TestAnalyticsTracker.tearDown() } + + makeAdapter().track(.mediaLibraryUploadRetried) + + #expect(TestAnalyticsTracker.tracked.last?.stat == .mediaLibraryUploadMediaRetried) + } + + @Test("Stock Photos image fires PhotoViaStockPhotos + StockMediaUploaded") + func stockPhotos_imageAdded_firesPhotoViaStockPhotos_andStockMediaUploaded() { + TestAnalyticsTracker.setup() + defer { TestAnalyticsTracker.tearDown() } + + makeAdapter().track(.mediaLibraryAdded(source: .stockPhotos, kind: .image)) + + let events = TestAnalyticsTracker.tracked + #expect(events.contains { $0.stat == .mediaLibraryAddedPhotoViaStockPhotos }) + #expect(events.contains { $0.stat == .stockMediaUploaded }) + let photoViaEvent = events.first { $0.stat == .mediaLibraryAddedPhotoViaStockPhotos } + #expect(photoViaEvent?.properties["is_v2"] as? String == "1") + #expect(photoViaEvent?.properties["media_origin"] as? String == "full_screen_picker") + } + + @Test("Tenor image fires PhotoViaTenor + TenorUploaded through event rail") + func tenor_imageAdded_firesPhotoViaTenor_andTenorUploaded_throughEventRail() { + TestAnalyticsTracker.setup() + defer { TestAnalyticsTracker.tearDown() } + + makeAdapter().track(.mediaLibraryAdded(source: .tenor, kind: .image)) + + // WPAnalyticsEvent emissions land via the `trackString` path on the test + // tracker with `stat == .noStat` and the raw event name in `event`. + let trackedEventNames = TestAnalyticsTracker.tracked.map(\.event) + #expect(trackedEventNames.contains("media_library_photo_added")) + #expect(trackedEventNames.contains("tenor_uploaded")) + let mediaAdded = TestAnalyticsTracker.tracked.first { $0.event == "media_library_photo_added" } + #expect(mediaAdded?.properties["via"] as? String == "tenor") + #expect(mediaAdded?.properties["media_origin"] as? String == "full_screen_picker") + #expect(mediaAdded?.properties["is_v2"] as? String == "1") + } + + @Test("Tenor non-image kinds fire no external-source events") + func tenor_videoOrDocumentAdded_doesNotFireExternalSourceEvents() { + TestAnalyticsTracker.setup() + defer { TestAnalyticsTracker.tearDown() } + + let blog = ModelTestHelper.insertDotComBlog(context: mainContext) + contextManager.saveContextAndWait(mainContext) + let adapter = MediaTrackerAdapter(blog: blog, baseProperties: [:]) + adapter.track(.mediaLibraryAdded(source: .tenor, kind: .video)) + + let trackedEventNames = TestAnalyticsTracker.tracked.map(\.event) + #expect(!trackedEventNames.contains("tenor_uploaded")) + #expect(!trackedEventNames.contains("media_library_photo_added")) + } + + @Test("Image Playground image fires no added-photo event") + func imagePlayground_imageAdded_firesNoAddedPhotoEvent() { + TestAnalyticsTracker.setup() + defer { TestAnalyticsTracker.tearDown() } + + let blog = ModelTestHelper.insertDotComBlog(context: mainContext) + contextManager.saveContextAndWait(mainContext) + let adapter = MediaTrackerAdapter(blog: blog, baseProperties: [:]) + adapter.track(.mediaLibraryAdded(source: .imagePlayground, kind: .image)) + + let trackedEventNames = TestAnalyticsTracker.tracked.map(\.event) + let trackedStats = TestAnalyticsTracker.tracked.map(\.stat) + #expect(!trackedEventNames.contains("media_library_photo_added")) + #expect(!trackedStats.contains(.mediaLibraryAddedPhotoViaStockPhotos)) + } +} diff --git a/Tests/KeystoneTests/Tests/Features/Media/MediaUploaderRegistryTests.swift b/Tests/KeystoneTests/Tests/Features/Media/MediaUploaderRegistryTests.swift new file mode 100644 index 000000000000..b277db213a7c --- /dev/null +++ b/Tests/KeystoneTests/Tests/Features/Media/MediaUploaderRegistryTests.swift @@ -0,0 +1,58 @@ +import Foundation +import CoreData +import Testing +import WordPressData +@testable import WordPress + +@Suite("MediaUploaderRegistry", .serialized) +@MainActor +struct MediaUploaderRegistryTests { + let contextManager = ContextManager.forTesting() + var mainContext: NSManagedObjectContext { contextManager.mainContext } + + // WordPressSite(blog:) for a dotCom blog requires a non-nil dotComID and + // account.authToken so it can construct the .dotCom case without hitting + // the keychain. + private func makeBlog(siteId: Int) -> Blog { + let blog = ModelTestHelper.insertDotComBlog(context: mainContext) + blog.dotComID = siteId as NSNumber + blog.account?.authToken = "test-token" + // TaggedManagedObjectID(blog) requires a permanent ID; save the + // context so Core Data assigns one before the blog is keyed. + contextManager.saveContextAndWait(mainContext) + return blog + } + + @Test("Returns the same uploader for the same blog") + func sameBlogReturnsSameUploader() throws { + let blog = makeBlog(siteId: 1) + let registry = MediaUploaderRegistry() + let first = try registry.uploader(for: blog) + let second = try registry.uploader(for: blog) + #expect(first === second) + } + + @Test("tearDown removes only the targeted blog's uploader") + func tearDownIsTargeted() async throws { + let blogA = makeBlog(siteId: 1) + let blogB = makeBlog(siteId: 2) + let registry = MediaUploaderRegistry() + _ = try registry.uploader(for: blogA) + let bUploader = try registry.uploader(for: blogB) + await registry.tearDown(blogID: TaggedManagedObjectID(blogA)) + let bAgain = try registry.uploader(for: blogB) + #expect(bUploader === bAgain) + } + + @Test("tearDownAll clears every uploader") + func tearDownAllClears() async throws { + let blogA = makeBlog(siteId: 1) + let blogB = makeBlog(siteId: 2) + let registry = MediaUploaderRegistry() + let aFirst = try registry.uploader(for: blogA) + _ = try registry.uploader(for: blogB) + await registry.tearDownAll() + let aSecond = try registry.uploader(for: blogA) + #expect(aFirst !== aSecond) + } +} diff --git a/WordPress/Classes/Services/SiteManagementService.swift b/WordPress/Classes/Services/SiteManagementService.swift index d86ef2f76c81..7c41a7c9643b 100644 --- a/WordPress/Classes/Services/SiteManagementService.swift +++ b/WordPress/Classes/Services/SiteManagementService.swift @@ -30,11 +30,18 @@ open class SiteManagementService: NSObject { guard let remote = siteManagementServiceRemoteForBlog(blog) else { return } + // Capture the blog id synchronously, before `remove(blog)` deletes + // the managed object. The teardown Task can then run safely without + // touching the deleted Core Data instance. + let blogID = TaggedManagedObjectID(blog) remote.deleteSite( blog.dotComID!, success: { let blogService = BlogService(coreDataStack: self.coreDataStack) blogService.remove(blog) + Task { @MainActor in + await MediaUploaderRegistry.shared.tearDown(blogID: blogID) + } DispatchQueue.main.async { NotificationCenter.default.post(name: .WPSiteDeleted, object: nil) success?() diff --git a/WordPress/Classes/Utility/AccountHelper.swift b/WordPress/Classes/Utility/AccountHelper.swift index 8d1083a65186..ac4fa8e1656c 100644 --- a/WordPress/Classes/Utility/AccountHelper.swift +++ b/WordPress/Classes/Utility/AccountHelper.swift @@ -99,6 +99,9 @@ import WordPressData WordPressClientFactory.shared.reset() JetpackSocialFactory.shared.reset() + Task { @MainActor in + await MediaUploaderRegistry.shared.tearDownAll() + } } static func deleteAccountData() { diff --git a/WordPress/Classes/ViewRelated/Blog/Blog Details/BlogDetailsViewController+Swift.swift b/WordPress/Classes/ViewRelated/Blog/Blog Details/BlogDetailsViewController+Swift.swift index de8e10e129c6..7fed969be897 100644 --- a/WordPress/Classes/ViewRelated/Blog/Blog Details/BlogDetailsViewController+Swift.swift +++ b/WordPress/Classes/ViewRelated/Blog/Blog Details/BlogDetailsViewController+Swift.swift @@ -20,6 +20,12 @@ extension BlogDetailsViewController { public func confirmRemoveSite() { let blogService = BlogService(coreDataStack: ContextManager.shared) + // Compute the id synchronously before `remove(blog)` deletes the + // managed object, so the teardown Task doesn't touch a deleted Blog. + let blogID = TaggedManagedObjectID(blog) + Task { @MainActor in + await MediaUploaderRegistry.shared.tearDown(blogID: blogID) + } blogService.remove(blog) WordPressAppDelegate.shared?.trackLogoutIfNeeded() diff --git a/WordPress/Classes/ViewRelated/Jetpack/Jetpack Settings/JetpackConnectionViewController.swift b/WordPress/Classes/ViewRelated/Jetpack/Jetpack Settings/JetpackConnectionViewController.swift index 6978dffb03cd..a13868bdbfaa 100644 --- a/WordPress/Classes/ViewRelated/Jetpack/Jetpack Settings/JetpackConnectionViewController.swift +++ b/WordPress/Classes/ViewRelated/Jetpack/Jetpack Settings/JetpackConnectionViewController.swift @@ -127,6 +127,12 @@ open class JetpackConnectionViewController: UITableViewController { self?.stopLoading() if let blog = self?.blog { let service = BlogService(coreDataStack: ContextManager.shared) + // Compute the id synchronously before `remove(blog)` so + // the teardown Task doesn't touch a deleted Blog. + let blogID = TaggedManagedObjectID(blog) + Task { @MainActor in + await MediaUploaderRegistry.shared.tearDown(blogID: blogID) + } service.remove(blog) self?.delegate?.jetpackDisconnectedForBlog(blog) } else {