Rewrite AnalyticsTrackerAutomatticTracks in Swift by kean · Pull Request #24399 · wordpress-mobile/WordPress-iOS

kean · 2025-04-02T23:18:40Z

This class had a lot of Swift APIs that it depended on. Prerequisite for Keystone. I carefully translated the code and compared it side-by-side multiple times. The risk of introducing user-facing regression is none because it's tracking.

To test:

Add a self-hosted site and verify that session starts as anonymous (step with a debugger)
Add a WP.com account and verify that session starts with the username/token

Regression Notes

Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

dangermattic · 2025-04-02T23:19:19Z

	2 Warnings
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	This PR is assigned to the milestone 25.9. The due date for this milestone has already passed. Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

kean · 2025-04-02T23:26:01Z

+        let name: String
+        var properties: [String: Any]?
+
+        switch stat {


There was no AI used – only search&replace, so there is no need to verify it.

wpmobilebot · 2025-04-02T23:32:16Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Configuration	Release-Alpha
	Build Number	`27202`
	Version	`PR #24399`
	Bundle ID	`com.jetpack.alpha`
	Commit	`84ffe73`
	Installation URL	553blrmomqeo8

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

wpmobilebot · 2025-04-02T23:34:24Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Configuration	Release-Alpha
	Build Number	`27202`
	Version	`PR #24399`
	Bundle ID	`org.wordpress.alpha`
	Commit	`84ffe73`
	Installation URL	6l24ajbfe7uh0

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

crazytonyli · 2025-04-03T09:32:39Z

+        var mergedProperties: [AnyHashable: Any] = event.properties ?? [:]
+        for (key, value) in properties ?? [:] {
+            mergedProperties[key] = value
+        }


Nitpick: The above could be implemented in one line.

let mergedProperties = (event.properties ?? [:]).merging(properties ?? [:]) { (_, new) in new }

Updated. I'm not sure why I keep avoiding merging. It is more clear and less code this way.

crazytonyli · 2025-04-03T09:34:22Z

+        track(stat, withProperties: nil)
+    }
+
+    public func track(_ stat: WPAnalyticsStat, withProperties properties: [AnyHashable: Any]?) {


Should the key of properties be String?

It's how it's defined in the protocol. I would ideally update it to [String: EnumWithSupportedTypesOnly] to avoid any confusion.

crazytonyli · 2025-04-03T09:46:34Z

+            return
+        }
+
+        if currentUserID?.isEmpty == true {


The condition should be currentUserID?.isEmpty != false because currentUserID = nil should execute the if branch.

The previous condition was the following:

if (self.loggedInID.length == 0) {

The logic is that if you don't have any currentUserID saved, then call switchToAuthenticatedUser with skipAliasEventCreation: false. A != nil check should probably be enough, but I used isEmpty just for to be safe.

Yeah, I added the comment because the condition check has changed. The previous condition is true if loggedInID is nil, but the new condition is false if currentUserID is nil.

The condition should be currentUserID?.isEmpty != false

It's the same as currentUserID?.isEmpty == true. It's true if currentUserID is empty.

I don't think they are the same. currentUserID?.isEmpty has three possible values: Optional.some(true), .some(false), and Optional.none. If currentUserID is nil, currentUserID?.isEmpty would be nil and the if condition is false, which is the opssite of the original Objective-C code if (self.loggedInID.length == 0).

Oh yeah you are totally right. I don't know who I missed that 🤦

I didn't want to move the conditions around, so I replaced this condition with the following (not great, but should be clear enough):

if (currentUserID ?? "").isEmpty

In the future, I feel like I'll just stop relying on these optional tricks.

kean · 2025-04-08T14:44:48Z

Hey, let's get this one merged folks, cc @mokagio , @crazytonyli

kean added the Analytics label Apr 2, 2025

kean added this to the 25.9 milestone Apr 2, 2025

kean requested review from crazytonyli and mokagio and removed request for crazytonyli April 2, 2025 23:21

kean commented Apr 2, 2025

View reviewed changes

crazytonyli reviewed Apr 3, 2025

View reviewed changes

kean requested a review from crazytonyli April 3, 2025 12:13

kean force-pushed the task/rewrite-automattic-tracks branch from 479119b to 4181f81 Compare April 3, 2025 13:37

kean mentioned this pull request Apr 3, 2025

Add remaining public APIs for Keystone #24406

Merged

14 tasks

Rewrite WPAnalyticsTrackerAutomatticTracks in Swift

4d2b2ff

kean force-pushed the task/rewrite-automattic-tracks branch from ba6c0ef to 4d2b2ff Compare April 7, 2025 12:28

Fix condition

84ffe73

kean mentioned this pull request Apr 8, 2025

Update access of types required by Keystone for WordPressData link #24420

Closed

crazytonyli approved these changes Apr 8, 2025

View reviewed changes

kean added this pull request to the merge queue Apr 8, 2025

Merged via the queue into trunk with commit b246225 Apr 8, 2025

kean deleted the task/rewrite-automattic-tracks branch April 8, 2025 22:08

Uh oh!

Conversation

kean commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Regression Notes

Uh oh!

dangermattic commented Apr 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wpmobilebot commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crazytonyli Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kean commented Apr 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kean commented Apr 2, 2025 •

edited

Loading

wpmobilebot commented Apr 2, 2025 •

edited

Loading

wpmobilebot commented Apr 2, 2025 •

edited

Loading

crazytonyli Apr 3, 2025 •

edited

Loading