Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track Load WordPress screen events and content export events #19719

Merged
merged 3 commits into from
Dec 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,32 @@ class ContentMigrationCoordinator {
private let dataMigrator: ContentDataMigrating
private let userPersistentRepository: UserPersistentRepository
private let eligibilityProvider: ContentMigrationEligibilityProvider
private let tracker: MigrationAnalyticsTracker

init(coreDataStack: CoreDataStack = ContextManager.shared,
dataMigrator: ContentDataMigrating = DataMigrator(),
userPersistentRepository: UserPersistentRepository = UserDefaults.standard,
eligibilityProvider: ContentMigrationEligibilityProvider = AppConfiguration()) {
eligibilityProvider: ContentMigrationEligibilityProvider = AppConfiguration(),
tracker: MigrationAnalyticsTracker = .init()) {
self.coreDataStack = coreDataStack
self.dataMigrator = dataMigrator
self.userPersistentRepository = userPersistentRepository
self.eligibilityProvider = eligibilityProvider
self.tracker = tracker
}

enum ContentMigrationCoordinatorError: Error {
enum ContentMigrationCoordinatorError: LocalizedError {
case ineligible
case exportFailure
case localDraftsNotSynced

var errorDescription: String? {
switch self {
case .ineligible: return "Content export is ineligible"
case .exportFailure: return "Content export failed"
case .localDraftsNotSynced: return "Local drafts not synced"
}
Comment on lines +36 to +39
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that it matters but I think general style if it exists is to put them on new lines 😀

}
}

// MARK: Methods
Expand All @@ -41,22 +52,27 @@ class ContentMigrationCoordinator {
/// - Parameter completion: Closure called after the export process completes.
func startAndDo(completion: ((Result<Void, ContentMigrationCoordinatorError>) -> Void)? = nil) {
guard eligibilityProvider.isEligibleForMigration else {
tracker.trackContentExportEligibility(eligible: false)
completion?(.failure(.ineligible))
return
}

guard isLocalPostsSynced() else {
completion?(.failure(.localDraftsNotSynced))
let error = ContentMigrationCoordinatorError.localDraftsNotSynced
tracker.trackContentExportFailed(reason: error.localizedDescription)
completion?(.failure(error))
return
}

dataMigrator.exportData { result in
dataMigrator.exportData { [weak self] result in
switch result {
case .success:
self?.tracker.trackContentExportSucceeded()
completion?(.success(()))

case .failure(let error):
DDLogError("[Jetpack Migration] Error exporting data: \(error)")
self?.tracker.trackContentExportFailed(reason: error.localizedDescription)
completion?(.failure(.exportFailure))
}
}
Expand Down
13 changes: 9 additions & 4 deletions WordPress/Jetpack/Classes/System/JetpackWindowManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,17 @@ private extension JetpackWindowManager {
func showLoadWordPressUI(schemeUrl: URL) {
let actions = MigrationLoadWordPressViewModel.Actions()
let loadWordPressViewModel = MigrationLoadWordPressViewModel(actions: actions)
let loadWordPressViewController = MigrationLoadWordPressViewController(viewModel: loadWordPressViewModel)
actions.primary = {
let loadWordPressViewController = MigrationLoadWordPressViewController(
viewModel: loadWordPressViewModel,
tracker: migrationTracker
)
actions.primary = { [weak self] in
self?.migrationTracker.track(.loadWordPressScreenOpenTapped)
UIApplication.shared.open(schemeUrl)
}
actions.secondary = { [weak self] in
loadWordPressViewController.dismiss(animated: true) {
actions.secondary = { [weak self, weak loadWordPressViewController] in
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding [weak loadWordPressViewController] to break a reference cycle between the secondary action handler and loadWordPressViewController.

Without the weak reference, the memory would look like this:

loadWordPressViewController -> loadWordPressViewModel -> actions.secondary -> loadWordPressViewController

a -> b means a owns b

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting this! I'd had a hunch something in this area would cause a reference cycle but didn't go back to investigate.

self?.migrationTracker.track(.loadWordPressScreenNoThanksTapped)
loadWordPressViewController?.dismiss(animated: true) {
self?.showSignInUI()
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,22 @@ struct MigrationAnalyticsTracker {
WPAnalytics.track(event)
}

// MARK: - Content Export

func trackContentExportEligibility(eligible: Bool) {
let properties = ["eligible": String(eligible)]
self.track(.contentExportEligibility, properties: properties)
}

func trackContentExportSucceeded() {
self.track(.contentExportSucceeded)
}

func trackContentExportFailed(reason: String) {
let properties = ["error_type": reason]
self.track(.contentExportFailed, properties: properties)
}

// MARK: - Content Import

func trackContentImportEligibility(eligible: Bool) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import Foundation

enum MigrationEvent: String {
// Content Export
case contentExportEligibility = "migration_content_export_eligibility"
case contentExportSucceeded = "migration_content_export_succeeded"
case contentExportFailed = "migration_content_export_failed"

// Content Import
case contentImportEligibility = "migration_content_import_eligibility"
case contentImportSucceeded = "migration_content_import_succeeded"
Expand Down Expand Up @@ -37,6 +42,11 @@ enum MigrationEvent: String {
case pleaseDeleteWordPressScreenHelpTapped = "migration_please_delete_wordpress_screen_help_tapped"
case pleaseDeleteWordPressScreenCloseTapped = "migration_please_delete_wordpress_screen_close_tapped"

// WordPress Migratable Stat
// Load WordPress
case loadWordPressScreenShown = "migration_load_wordpress_screen_shown"
case loadWordPressScreenOpenTapped = "migration_load_wordpress_screen_open_tapped"
case loadWordPressScreenNoThanksTapped = "migration_load_wordpress_screen_no_thanks_tapped"

// WordPress Migratable State
case wordPressDetected = "migration_wordpressapp_detected"
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ class MigrationLoadWordPressViewController: UIViewController {
// MARK: - Dependencies

private let viewModel: MigrationLoadWordPressViewModel
private let tracker: MigrationAnalyticsTracker

// MARK: - Init

init(viewModel: MigrationLoadWordPressViewModel) {
init(viewModel: MigrationLoadWordPressViewModel, tracker: MigrationAnalyticsTracker = .init()) {
self.viewModel = viewModel
self.tracker = tracker
super.init(nibName: nil, bundle: nil)
}

Expand All @@ -32,4 +34,9 @@ class MigrationLoadWordPressViewController: UIViewController {
super.viewDidLoad()
self.view.backgroundColor = MigrationAppearance.backgroundColor
}

override func viewDidAppear(_ animated: Bool) {
super.viewDidAppear(animated)
self.tracker.track(.loadWordPressScreenShown)
}
}