diff --git a/Modules/Sources/WordPressData/Swift/Blog+Analytics.swift b/Modules/Sources/WordPressData/Swift/Blog+Analytics.swift new file mode 100644 index 000000000000..9e7645658751 --- /dev/null +++ b/Modules/Sources/WordPressData/Swift/Blog+Analytics.swift @@ -0,0 +1,20 @@ +import Foundation +import CoreData +import WordPressShared + +extension Blog: BlogAnalyticsRepresentable { + /// A `Sendable` snapshot of the properties analytics attaches to an event. + /// + /// The read runs on the blog's own context queue, so the snapshot is safe to + /// hand to the analytics layer from any thread. This matters because + /// `dotComID`'s getter can mutate the object, and `isWPForTeams` faults the + /// `options` relationship — neither is safe to touch off-queue. + public var analyticsProperties: BlogAnalyticsProperties { + guard let managedObjectContext else { + return BlogAnalyticsProperties(dotComID: dotComID.map { Int($0.int64Value) }, isWPForTeams: isWPForTeams) + } + return managedObjectContext.performAndWait { + BlogAnalyticsProperties(dotComID: dotComID.map { Int($0.int64Value) }, isWPForTeams: isWPForTeams) + } + } +} diff --git a/Modules/Sources/WordPressShared/Analytics/BlogAnalyticsProperties.swift b/Modules/Sources/WordPressShared/Analytics/BlogAnalyticsProperties.swift new file mode 100644 index 000000000000..d0b6c0c6fbcc --- /dev/null +++ b/Modules/Sources/WordPressShared/Analytics/BlogAnalyticsProperties.swift @@ -0,0 +1,48 @@ +import Foundation + +/// A `Sendable`, value-type snapshot of the site facts that analytics attaches +/// to an event. +/// +/// Analytics only needs a couple of scalars about a site — its WordPress.com ID +/// and whether it's a P2 — but historically those were read straight off a Core +/// Data `Blog` at the point of tracking, on whatever thread the caller happened +/// to be on. Reading a managed object off its context's queue is a Core Data +/// threading violation, and `Blog.dotComID`'s getter can *write* back to the +/// object. Capturing the facts in this value type moves the read to a single, +/// controlled place and lets only a `Sendable` snapshot cross into the analytics +/// layer. +/// +/// - SeeAlso: ``BlogAnalyticsRepresentable`` +public struct BlogAnalyticsProperties: Equatable, Sendable { + /// The site's WordPress.com ID (the `blog_id` property). `nil` for + /// self-hosted sites. + /// + /// `Int` rather than `Int64` so it satisfies a consumer's `as? Int` after the + /// analytics dictionary round-trips through Objective-C; `Int` is 64-bit on + /// every supported platform, so no blog ID is truncated. + public let dotComID: Int? + + /// Whether the site is a WordPress for Teams (P2) site. Drives the + /// `site_type` property. + public let isWPForTeams: Bool + + public init(dotComID: Int?, isWPForTeams: Bool) { + self.dotComID = dotComID + self.isWPForTeams = isWPForTeams + } +} + +/// A type that can describe itself to the analytics layer as a value. +/// +/// Conform model types (e.g. `Blog`) to this protocol so callers can hand +/// analytics a `Sendable` ``BlogAnalyticsProperties`` snapshot instead of a live +/// Core Data object. Only the snapshot crosses the model boundary, which keeps +/// the analytics layer free of Core Data — and free of the threading hazards +/// that come with it. +public protocol BlogAnalyticsRepresentable { + /// A value-type snapshot of the site facts analytics needs. + /// + /// A conformance that wraps a managed object must produce this snapshot on + /// the object's context queue. + var analyticsProperties: BlogAnalyticsProperties { get } +} diff --git a/Modules/Tests/WordPressDataTests/BlogAnalyticsPropertiesTests.swift b/Modules/Tests/WordPressDataTests/BlogAnalyticsPropertiesTests.swift new file mode 100644 index 000000000000..2917f3d950ce --- /dev/null +++ b/Modules/Tests/WordPressDataTests/BlogAnalyticsPropertiesTests.swift @@ -0,0 +1,35 @@ +import CoreData +import Testing +import WordPressShared +@testable import WordPressData + +@MainActor +struct BlogAnalyticsPropertiesTests { + private let contextManager = ContextManager.forTesting() + private var mainContext: NSManagedObjectContext { contextManager.mainContext } + + @Test func snapshotCapturesDotComIDAndBlogSiteType() { + let blog = BlogBuilder(mainContext, dotComID: NSNumber(value: 42)) + .with(isWPForTeamsSite: false) + .build() + + let properties = blog.analyticsProperties + + #expect(properties.dotComID == 42) + #expect(!properties.isWPForTeams) + } + + @Test func snapshotCapturesP2SiteType() { + let blog = BlogBuilder(mainContext, dotComID: NSNumber(value: 7)) + .with(isWPForTeamsSite: true) + .build() + + #expect(blog.analyticsProperties.isWPForTeams) + } + + @Test func selfHostedSiteHasNoDotComID() { + let blog = BlogBuilder(mainContext, dotComID: nil).build() + + #expect(blog.analyticsProperties.dotComID == nil) + } +} diff --git a/Tests/KeystoneTests/Tests/Misc/BlogAnalyticsTrackingTests.swift b/Tests/KeystoneTests/Tests/Misc/BlogAnalyticsTrackingTests.swift new file mode 100644 index 000000000000..316b0718568d --- /dev/null +++ b/Tests/KeystoneTests/Tests/Misc/BlogAnalyticsTrackingTests.swift @@ -0,0 +1,62 @@ +import CoreData +import XCTest +import WordPressData +import WordPressShared + +@testable import WordPress + +/// End-to-end coverage for attaching a site to an event via `WPAnalytics.track(_:properties:blog:)`. +/// +/// Confirms the `Blog` snapshot reaches the emitted event as `blog_id` / `site_type`, +/// and that `blog_id` arrives as a number (a Swift `Int` bridges to `NSNumber` at the +/// Objective-C tracker boundary). +final class BlogAnalyticsTrackingTests: CoreDataTestCase { + + override func setUp() { + super.setUp() + TestAnalyticsTracker.setup() + } + + override func tearDown() { + TestAnalyticsTracker.tearDown() + super.tearDown() + } + + func testTrackAttachesBlogIDAndP2SiteType() throws { + let blog = BlogBuilder(mainContext, dotComID: 100) + .with(isWPForTeamsSite: true) + .build() + + WPAnalytics.track(.dashboardCardShown, properties: ["type": "post"], blog: blog) + + let tracked = try XCTUnwrap(TestAnalyticsTracker.tracked.last) + let blogID: Int? = tracked.value(for: "blog_id") + XCTAssertEqual(blogID, 100) + XCTAssertEqual(tracked.value(for: "site_type"), "p2") + XCTAssertEqual(tracked.value(for: "type"), "post") + } + + func testTrackMarksNonP2SiteAsBlog() throws { + let blog = BlogBuilder(mainContext, dotComID: 200) + .with(isWPForTeamsSite: false) + .build() + + WPAnalytics.track(.dashboardCardShown, blog: blog) + + let tracked = try XCTUnwrap(TestAnalyticsTracker.tracked.last) + let blogID: Int? = tracked.value(for: "blog_id") + XCTAssertEqual(blogID, 200) + XCTAssertEqual(tracked.value(for: "site_type"), "blog") + } + + func testSelfHostedSiteTracksWithoutBlogID() throws { + let blog = BlogBuilder(mainContext, dotComID: nil).build() + + WPAnalytics.track(.dashboardCardShown, blog: blog) + + let tracked = try XCTUnwrap(TestAnalyticsTracker.tracked.last) + let blogID: Int? = tracked.value(for: "blog_id") + XCTAssertNil(blogID) + XCTAssertEqual(tracked.value(for: "site_type"), "blog") + } +} diff --git a/WordPress/Classes/Utility/Analytics/WPAnalyticsEvent.swift b/WordPress/Classes/Utility/Analytics/WPAnalyticsEvent.swift index 32b02d55756b..c61d51e0ff77 100644 --- a/WordPress/Classes/Utility/Analytics/WPAnalyticsEvent.swift +++ b/WordPress/Classes/Utility/Analytics/WPAnalyticsEvent.swift @@ -1978,19 +1978,41 @@ extension WPAnalytics { WPAnalytics.trackString(event.value, withProperties: mergedProperties) } - /// This will call each registered tracker and fire the given event. + /// This will call each registered tracker and fire the given event, attaching the site. /// - Parameters: /// - event: a `String` that represents the event name /// - properties: a `Hash` that represents the properties - /// - blog: a `Blog` asssociated with the event - static func track(_ event: WPAnalyticsEvent, properties: [AnyHashable: Any], blog: Blog) { + /// - blogProperties: a value-type snapshot of the site associated with the event + static func track( + _ event: WPAnalyticsEvent, + properties: [AnyHashable: Any] = [:], + blogProperties: BlogAnalyticsProperties + ) { var props = properties - props[WPAppAnalyticsKeyBlogID] = blog.dotComID + if let dotComID = blogProperties.dotComID { + props[WPAppAnalyticsKeyBlogID] = dotComID + } props[WPAppAnalyticsKeySiteType] = - blog.isWPForTeams ? WPAppAnalyticsValueSiteTypeP2 : WPAppAnalyticsValueSiteTypeBlog + blogProperties.isWPForTeams ? WPAppAnalyticsValueSiteTypeP2 : WPAppAnalyticsValueSiteTypeBlog WPAnalytics.track(event, properties: props) } + /// This will call each registered tracker and fire the given event, attaching the site. + /// + /// Only a `Sendable` ``BlogAnalyticsProperties`` snapshot crosses into the analytics + /// layer; the Core Data read happens once, in the model's `analyticsProperties`. + /// - Parameters: + /// - event: a `String` that represents the event name + /// - properties: a `Hash` that represents the properties + /// - blog: the site associated with the event + static func track( + _ event: WPAnalyticsEvent, + properties: [AnyHashable: Any] = [:], + blog: some BlogAnalyticsRepresentable + ) { + track(event, properties: properties, blogProperties: blog.analyticsProperties) + } + /// Track a Reader event /// /// This will call each registered tracker and fire the given event diff --git a/WordPress/Classes/Utility/Analytics/WPAppAnalytics+Extensions.swift b/WordPress/Classes/Utility/Analytics/WPAppAnalytics+Extensions.swift index b7783a217eb5..8e105246005f 100644 --- a/WordPress/Classes/Utility/Analytics/WPAppAnalytics+Extensions.swift +++ b/WordPress/Classes/Utility/Analytics/WPAppAnalytics+Extensions.swift @@ -23,10 +23,15 @@ extension WPAppAnalytics { public class func track(_ stat: WPAnalyticsStat, properties: [String: Any]?, blog: Blog?) { var properties = properties ?? [:] if let blog { - if let blogID = blog.dotComID { + // Snapshot on the blog's context queue so the Core Data reads + // (`dotComID`'s getter can mutate the object) never run off-queue. + let blogProperties = blog.analyticsProperties + if let blogID = blogProperties.dotComID { properties[WPAppAnalyticsKeyBlogID] = blogID } - properties[WPAppAnalyticsKeySiteType] = siteType(for: blog) + properties[WPAppAnalyticsKeySiteType] = + blogProperties.isWPForTeams + ? WPAppAnalyticsValueSiteTypeP2 : WPAppAnalyticsValueSiteTypeBlog } WPAppAnalytics.track(stat, withProperties: properties) } diff --git a/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Blaze/DashboardBlazeCardCell.swift b/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Blaze/DashboardBlazeCardCell.swift index 083adbbbbb81..b2c1b4419761 100644 --- a/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Blaze/DashboardBlazeCardCell.swift +++ b/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Blaze/DashboardBlazeCardCell.swift @@ -31,25 +31,29 @@ final class DashboardBlazeCardCell: DashboardCollectionViewCell { switch viewModel.state { case .promo: let cardView = DashboardBlazePromoCardView(.make(with: blog, viewController: viewController)) - self.setCardView(cardView, subtype: .promo) + self.setCardView(cardView, subtype: .promo, blog: blog) case .campaign(let campaign): let cardView = DashboardBlazeCampaignsCardView() cardView.configure(blog: blog, viewController: viewController, campaign: campaign) - self.setCardView(cardView, subtype: .campaigns) + self.setCardView(cardView, subtype: .campaigns, blog: blog) } } - private func setCardView(_ cardView: UIView, subtype: DashboardBlazeCardSubtype) { + private func setCardView(_ cardView: UIView, subtype: DashboardBlazeCardSubtype, blog: Blog) { contentView.subviews.forEach { $0.removeFromSuperview() } cardView.translatesAutoresizingMaskIntoConstraints = false contentView.addSubview(cardView) contentView.pinSubviewToAllEdges(cardView, priority: UILayoutPriority(999)) - BlogDashboardAnalytics.shared.track(.dashboardCardShown, properties: [ - "type": DashboardCard.blaze.rawValue, - "sub_type": subtype.rawValue - ]) + BlogDashboardAnalytics.shared.track( + .dashboardCardShown, + properties: [ + "type": DashboardCard.blaze.rawValue, + "sub_type": subtype.rawValue + ], + blog: blog + ) } } diff --git a/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Posts/DashboardPostsListCardCell.swift b/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Posts/DashboardPostsListCardCell.swift index 9319c38e3d00..286c93fb7c77 100644 --- a/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Posts/DashboardPostsListCardCell.swift +++ b/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Posts/DashboardPostsListCardCell.swift @@ -74,10 +74,6 @@ class DashboardPostsListCardCell: UICollectionViewCell, Reusable { contentView.addSubview(frameView) contentView.pinSubviewToAllEdges(frameView, priority: UILayoutPriority(999)) } - - func trackPostsDisplayed() { - BlogDashboardAnalytics.shared.track(.dashboardCardShown, properties: ["type": "post", "sub_type": status.rawValue]) - } } // MARK: BlogDashboardCardConfigurable diff --git a/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Posts/PostsCardViewModel.swift b/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Posts/PostsCardViewModel.swift index 1f0ba85d29db..e1b6c64610ed 100644 --- a/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Posts/PostsCardViewModel.swift +++ b/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Posts/PostsCardViewModel.swift @@ -262,9 +262,9 @@ private extension PostsCardViewModel { func trackCardDisplayedIfNeeded() { switch currentState { case .posts: - BlogDashboardAnalytics.shared.track(.dashboardCardShown, properties: ["type": "post", "sub_type": status.rawValue]) + BlogDashboardAnalytics.shared.track(.dashboardCardShown, properties: ["type": "post", "sub_type": status.rawValue], blog: blog) case .error: - BlogDashboardAnalytics.shared.track(.dashboardCardShown, properties: ["type": "post", "sub_type": "error"]) + BlogDashboardAnalytics.shared.track(.dashboardCardShown, properties: ["type": "post", "sub_type": "error"], blog: blog) case .loading: return } diff --git a/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Helpers/BlogDashboardAnalytics.swift b/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Helpers/BlogDashboardAnalytics.swift index ff74e743de9c..bf1d8dd2d6c0 100644 --- a/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Helpers/BlogDashboardAnalytics.swift +++ b/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Helpers/BlogDashboardAnalytics.swift @@ -17,19 +17,18 @@ class BlogDashboardAnalytics { /// This will track the given event and properties given they haven't been /// triggered before. /// + /// The My Site dashboard always shows exactly one site, so every card-shown + /// event carries that site. `blog` is required so the site identifier is + /// always attached and no future card can regress by omitting it. + /// /// - Parameters: /// - event: a `String` that represents the event name /// - properties: a `Hash` that represents the properties - /// - blog: a `Blog` asssociated with the event - func track(_ event: WPAnalyticsEvent, properties: [AnyHashable: String] = [:], blog: Blog? = nil) { + /// - blog: the `Blog` whose dashboard is being shown + func track(_ event: WPAnalyticsEvent, properties: [AnyHashable: String] = [:], blog: Blog) { if !fired.contains(where: { $0 == (event, properties) }) { fired.append((event, properties)) - - if let blog { - WPAnalytics.track(event, properties: properties, blog: blog) - } else { - WPAnalytics.track(event, properties: properties) - } + WPAnalytics.track(event, properties: properties, blog: blog) } }