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

Abstract ContextManager usages behind CoreDataStack #24191

Draft
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Draft
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
2 changes: 2 additions & 0 deletions Modules/Sources/WordPressDataObjC/include/CoreDataStack.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ NS_ASSUME_NONNULL_BEGIN

@property (nonatomic, readonly, strong) NSManagedObjectContext *mainContext;

+ (id<CoreDataStack>)sharedInstance;

- (NSManagedObjectContext *const)newDerivedContext DEPRECATED_MSG_ATTRIBUTE("Use `performAndSave` instead");

- (void)saveContextAndWait:(NSManagedObjectContext *)context;
Expand Down
3 changes: 2 additions & 1 deletion WordPress/Classes/Models/Blog/Blog+SelfHosted.swift
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Foundation
import CryptoKit
import WordPressAPI
import WordPressData

extension Blog {

Expand All @@ -17,7 +18,7 @@ extension Blog {

static func createRestApiBlog(
with details: WpApiApplicationPasswordDetails,
in contextManager: ContextManager,
in contextManager: CoreDataStackSwift,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this PR, I decided not to rename the contextManager variable that now have CoreDataStackSwift to keep the diff smaller.

We might want to do it later on, but it does not seem urgent.

using keychainImplementation: KeychainAccessible = KeychainUtils()
) async throws -> String {
try await contextManager.performAndSave { context in
Expand Down
7 changes: 5 additions & 2 deletions WordPress/Classes/Services/JetpackSocialService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ import WordPressKit

/// Init method for Objective-C.
///
@objc init(contextManager: ContextManager) {
self.coreDataStack = contextManager
@objc init(contextManager: CoreDataStack) {
guard let typeCastStack = contextManager as? CoreDataStackSwift else {
fatalError("Expected a CoreDataStackSwift-conforming type even though this initializer is marked @objc.")
}
self.coreDataStack = typeCastStack
Comment on lines -22 to +26
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not proud of this one. I don't know if there's a neater way to go about it because if CoreDataStackSwift could be defined in Objective-C, then we wouldn't have needed it in the first place.

Luckily, this hack has to be used only twice, so I think we can live with it...?

Of course, one possible solution would be to convert the Objective-C consumers to Swift... But I think we ought to focus on moving forward with the Core Data migration at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, it'd be okay to remove the argument and use the shared instance internally.

}

init(coreDataStack: CoreDataStackSwift = ContextManager.shared) {
Expand Down
9 changes: 5 additions & 4 deletions WordPress/Classes/Services/SharingService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@ import WordPressKit
private let coreDataStack: CoreDataStackSwift

/// The initialiser for Objective-C code.
///
/// Using `ContextManager` as the argument becuase `CoreDataStackSwift` is not accessible from Objective-C code.
@objc
init(contextManager: ContextManager) {
self.coreDataStack = contextManager
init(contextManager: CoreDataStack) {
guard let typeCastStack = contextManager as? CoreDataStackSwift else {
fatalError("Expected a CoreDataStackSwift-conforming type even though this initializer is marked @objc.")
}
self.coreDataStack = typeCastStack
}

init(coreDataStack: CoreDataStackSwift) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Foundation
import UIKit
import SwiftUI
import WordPressData

class WelcomeSplitViewContent: SplitViewDisplayable {
let supplementary: UINavigationController
Expand All @@ -9,7 +10,7 @@ class WelcomeSplitViewContent: SplitViewDisplayable {
init(addSite: @escaping (AddSiteMenuViewModel.Selection) -> Void) {
supplementary = UINavigationController(rootViewController: UnifiedPrologueViewController())

let addSiteViewModel = AddSiteMenuViewModel(context: .shared, onSelection: addSite)
let addSiteViewModel = AddSiteMenuViewModel(context: ContextManager.shared, onSelection: addSite)
let noSitesViewModel = NoSitesViewModel(appUIType: JetpackFeaturesRemovalCoordinator.currentAppUIType, account: nil)
let noSiteView = NoSitesView(addSiteViewModel: addSiteViewModel, viewModel: noSitesViewModel)
let noSitesVC = UIHostingController(rootView: noSiteView)
Expand Down
4 changes: 3 additions & 1 deletion WordPress/Classes/System/UITesting/UITestConfigurator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ struct UITestConfigurator {

private static func resetEverything() {
// Remove CoreData DB
ContextManager.shared.resetEverything()
//
// We can afford to force cast here because we are in a test-specific code
(ContextManager.shared as! ContextManager).resetEverything()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In particular, this code will run only in the UI tests and only if a certain launch argument is given

https://github.com/wordpress-mobile/WordPress-iOS/pull/24191/files#diff-666427fc460fbbf821022ce337766cb3f1f3e56a2900c16e041d0fb28c51ae19L26-L28


// Clear user defaults.
for key in UserDefaults.standard.dictionaryRepresentation().keys {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -330,14 +330,14 @@ class WeeklyRoundupBackgroundTask: BackgroundTask {
private let eventTracker: NotificationEventTracker
let runDateComponents: DateComponents
let notificationScheduler: WeeklyRoundupNotificationScheduler
let coreDataStack: ContextManager
let coreDataStack: CoreDataStackSwift

init(
eventTracker: NotificationEventTracker = NotificationEventTracker(),
runDateComponents: DateComponents? = nil,
staticNotificationDateComponents: DateComponents? = nil,
store: Store = Store(),
coreDataStack: ContextManager = .shared
coreDataStack: CoreDataStackSwift = ContextManager.shared
) {
self.coreDataStack = coreDataStack
self.eventTracker = eventTracker
Expand Down
6 changes: 3 additions & 3 deletions WordPress/Classes/Utility/CoreData/ContextManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -282,16 +282,16 @@ extension ContextManager {
/// Tests purpose only
static var overrideInstance: ContextManager?

@objc class func sharedInstance() -> ContextManager {
@objc public class func sharedInstance() -> CoreDataStack {
if let overrideInstance {
return overrideInstance
}

return ContextManager.internalSharedInstance
}

static var shared: ContextManager {
return sharedInstance()
static var shared: CoreDataStackSwift {
return sharedInstance() as! CoreDataStackSwift
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import UIKit
import Combine
import AutomatticTracks
import AutomatticEncryptedLogs
import WordPressData

/// A wrapper around the logging stack – provides shared initialization and configuration for Tracks Crash and Event Logging
struct WPLoggingStack {
Expand Down Expand Up @@ -38,9 +39,9 @@ struct WPLoggingStack {
}

struct WPCrashLoggingDataProvider: CrashLoggingDataProvider {
private let contextManager: ContextManager
private let contextManager: CoreDataStackSwift

init(contextManager: ContextManager = .shared) {
init(contextManager: CoreDataStackSwift = ContextManager.shared) {
self.contextManager = contextManager
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import UIKit
import SwiftUI
import WordPressAuthenticator
import WordPressData

/// Manages the site creation flows.
struct AddSiteController {
Expand Down Expand Up @@ -80,7 +81,7 @@ struct AddSiteMenuViewModel {
}
}

init(context: ContextManager = .shared, onSelection: @escaping (Selection) -> Void) {
init(context: CoreDataStackSwift = ContextManager.shared, onSelection: @escaping (Selection) -> Void) {
let defaultAccount = try? WPAccount.lookupDefaultWordPressComAccount(in: context.mainContext)
let canAddSelfHostedSite = AppConfiguration.showAddSelfHostedSiteButton

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import CoreData
import SwiftUI
import WordPressData
import WordPressShared

final class BlogListViewModel: NSObject, ObservableObject {
Expand All @@ -15,7 +16,7 @@ final class BlogListViewModel: NSObject, ObservableObject {
private let configuration: BlogListConfiguration
private var rawSites: [Blog] = []
private let fetchedResultsController: NSFetchedResultsController<Blog>
private let contextManager: ContextManager
private let contextManager: CoreDataStackSwift
private let blogService: BlogService
private let eventTracker: EventTracker
private let recentSitesService: RecentSitesService
Expand All @@ -24,7 +25,7 @@ final class BlogListViewModel: NSObject, ObservableObject {
var onAddSiteTapped: (AddSiteMenuViewModel.Selection) -> Void = { _ in }

init(configuration: BlogListConfiguration = .defaultConfig,
contextManager: ContextManager = ContextManager.shared,
contextManager: CoreDataStackSwift = ContextManager.shared,
recentSitesService: RecentSitesService = RecentSitesService(),
eventTracker: EventTracker = DefaultEventTracker()) {
self.configuration = configuration
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Foundation
import UIKit
import WordPressData
import WordPressUI

class CompliancePopoverViewModel: ObservableObject {
Expand All @@ -13,12 +14,12 @@ class CompliancePopoverViewModel: ObservableObject {

private let analyticsTracker: PrivacySettingsAnalyticsTracking
private let defaults: UserDefaults
private let contextManager: ContextManager
private let contextManager: CoreDataStackSwift

// MARK: - Init

init(defaults: UserDefaults,
contextManager: ContextManager,
contextManager: CoreDataStackSwift,
analyticsTracker: PrivacySettingsAnalyticsTracking = PrivacySettingsAnalyticsTracker()) {
self.defaults = defaults
self.analyticsTracker = analyticsTracker
Expand Down
21 changes: 0 additions & 21 deletions WordPress/WordPressTest/DataMigratorTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -209,27 +209,6 @@ class DataMigratorTests: XCTestCase {
}
}

// MARK: - CoreDataStackMock

private final class CoreDataStackMock: CoreDataStack {
var mainContext: NSManagedObjectContext

init(mainContext: NSManagedObjectContext) {
self.mainContext = mainContext
}

func newDerivedContext() -> NSManagedObjectContext {
return mainContext
}

func saveContextAndWait(_ context: NSManagedObjectContext) {}
func save(_ context: NSManagedObjectContext) {}
func save(_ context: NSManagedObjectContext, completion completionBlock: (() -> Void)?, on queue: DispatchQueue) {}

func performAndSave(_ aBlock: @escaping (NSManagedObjectContext) -> Void) {}
func performAndSave(_ aBlock: @escaping (NSManagedObjectContext) -> Void, completion: (() -> Void)?, on queue: DispatchQueue) {}
}

// MARK: - KeychainUtilsMock

private final class KeychainUtilsMock: KeychainUtils {
Expand Down
9 changes: 7 additions & 2 deletions WordPress/WordPressTest/SharedDataIssueSolverTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,18 @@ private extension SharedDataIssueSolverTests {

// MARK: - CoreDataStackMock

private final class CoreDataStackMock: CoreDataStack {
var mainContext: NSManagedObjectContext
final class CoreDataStackMock: CoreDataStack {
private(set) var mainContext: NSManagedObjectContext

private static var shared: CoreDataStack?

init(mainContext: NSManagedObjectContext) {
self.mainContext = mainContext
CoreDataStackMock.shared = self
}

static func sharedInstance() -> any CoreDataStack { CoreDataStackMock.shared! }

func newDerivedContext() -> NSManagedObjectContext {
return mainContext
}
Expand Down