Skip to content

Refactor PluginsService to use plugin file path for better stability #15871

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jul 10, 2025
Merged

Refactor PluginsService to use plugin file path for better stability #15871

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
98b4d7b
Fetch storage system plugin by plugin file path instead of plugin nam…
jaclync Jul 4, 2025
a0e9cdd
Merge branch 'trunk' into feat/WOOMOB-760-fetch-plugin-by-slug
jaclync Jul 4, 2025
26cb6ec
Update `pluginsService.waitForPluginInStorage` in legacy eligibility …
jaclync Jul 4, 2025
6542f82
Update unit tests for `LegacyPOSTabEligibilityCheckerTests`.
jaclync Jul 4, 2025
5f83b82
Update comment to match what it does better.
jaclync Jul 10, 2025
c6488a1
Rename `plugin` to `pluginPath` parameter in `PluginsServiceProtocol.…
jaclync Jul 10, 2025
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
17 changes: 11 additions & 6 deletions Modules/Sources/Yosemite/Tools/Plugins/PluginsService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@ import protocol Storage.StorageManagerType
/// A service for system plugins.
public protocol PluginsServiceProtocol {
/// Waits for a specific plugin to be available in storage.
func waitForPluginInStorage(siteID: Int64, pluginName: String, isActive: Bool) async -> SystemPlugin
/// - Parameters:
/// - siteID: The site ID to search for the plugin.
/// - plugin: The plugin's file path (e.g., "woocommerce/woocommerce.php" for WooCommerce).
/// - isActive: Whether to wait for the plugin to be active or inactive.
Copy link
Contributor

@iamgabrielma iamgabrielma Jul 7, 2025

Choose a reason for hiding this comment

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

nit: I think the usage of "wait for" bit here could be miss leading, since we're not waiting but matching the current plugin state in storage. wdyt?

Suggested change
/// - isActive: Whether to wait for the plugin to be active or inactive.
/// - isActive: Whether to match a plugin that is currently active or inactive.

Copy link
Contributor Author

@jaclync jaclync Jul 10, 2025

Choose a reason for hiding this comment

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

Thanks for catching this inaccurate comment, updated the comment in 5f83b82.

/// - Returns: The SystemPlugin when found in storage.
func waitForPluginInStorage(siteID: Int64, plugin: String, isActive: Bool) async -> SystemPlugin
}

public class PluginsService: PluginsServiceProtocol {
Expand All @@ -16,20 +21,20 @@ public class PluginsService: PluginsServiceProtocol {
}

@MainActor
public func waitForPluginInStorage(siteID: Int64, pluginName: String, isActive: Bool) async -> SystemPlugin {
let predicate = \StorageSystemPlugin.siteID == siteID && \StorageSystemPlugin.name == pluginName && \StorageSystemPlugin.active == isActive
let nameDescriptor = NSSortDescriptor(keyPath: \StorageSystemPlugin.name, ascending: true)
public func waitForPluginInStorage(siteID: Int64, plugin: String, isActive: Bool) async -> SystemPlugin {
Copy link
Contributor

@iamgabrielma iamgabrielma Jul 7, 2025

Choose a reason for hiding this comment

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

Nit: How about updating the signature to pluginPath, or similar? Just to be more explicit about what we need to pass in.

Copy link
Contributor Author

@jaclync jaclync Jul 10, 2025

Choose a reason for hiding this comment

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

Good idea, renamed in c6488a1.

let predicate = \StorageSystemPlugin.siteID == siteID && \StorageSystemPlugin.plugin == plugin && \StorageSystemPlugin.active == isActive
let pluginDescriptor = NSSortDescriptor(keyPath: \StorageSystemPlugin.plugin, ascending: true)
let resultsController = ResultsController<StorageSystemPlugin>(storageManager: storageManager,
matching: predicate,
fetchLimit: 1,
sortedBy: [nameDescriptor])
sortedBy: [pluginDescriptor])
do {
try resultsController.performFetch()
if let plugin = resultsController.fetchedObjects.first {
return plugin
}
} catch {
DDLogError("Error loading plugin \(pluginName) for site \(siteID) initially: \(error.localizedDescription)")
DDLogError("Error loading plugin \(plugin) for site \(siteID) initially: \(error.localizedDescription)")
}

return await withCheckedContinuation { continuation in
Expand Down
14 changes: 10 additions & 4 deletions Modules/Tests/YosemiteTests/Tools/Plugins/PluginsServiceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ struct PluginsServiceTests {
storageManager.insertWCPlugin(siteID: siteID, isActive: true, version: "1.0.0")

// When
let result = await sut.waitForPluginInStorage(siteID: siteID, pluginName: PluginConstants.pluginName, isActive: true)
let result = await sut.waitForPluginInStorage(siteID: siteID, plugin: PluginConstants.plugin, isActive: true)

// Then
#expect(result.siteID == siteID)
#expect(result.name == PluginConstants.pluginName)
#expect(result.plugin == PluginConstants.plugin)
#expect(result.active == true)
#expect(result.version == "1.0.0")
}
Expand All @@ -33,14 +33,15 @@ struct PluginsServiceTests {
await storageManager.reset()

// When
async let plugin = sut.waitForPluginInStorage(siteID: siteID, pluginName: PluginConstants.pluginName, isActive: true)
async let plugin = sut.waitForPluginInStorage(siteID: siteID, plugin: PluginConstants.plugin, isActive: true)
#expect(storageManager.viewStorage.loadSystemPlugins(siteID: siteID).count == 0)
storageManager.insertWCPlugin(siteID: siteID, isActive: true, version: "2.0.0")
#expect(storageManager.viewStorage.loadSystemPlugins(siteID: siteID).count == 1)

// Then
let result = await plugin
#expect(result.siteID == siteID)
#expect(result.plugin == PluginConstants.plugin)
#expect(result.name == PluginConstants.pluginName)
#expect(result.active == true)
#expect(result.version == "2.0.0")
Expand All @@ -50,7 +51,11 @@ struct PluginsServiceTests {
private extension MockStorageManager {
func insertWCPlugin(siteID: Int64, isActive: Bool, version: String? = nil) {
performAndSave({ storage in
let plugin = SystemPlugin.fake().copy(siteID: siteID, name: PluginConstants.pluginName, version: version, active: isActive)
let plugin = SystemPlugin.fake().copy(siteID: siteID,
plugin: PluginConstants.plugin,
name: PluginConstants.pluginName,
version: version,
active: isActive)
let newPlugin = storage.insertNewObject(ofType: StorageSystemPlugin.self)
newPlugin.update(with: plugin)
}, completion: nil, on: .main)
Expand All @@ -67,5 +72,6 @@ private extension MockStorageManager {

// MARK: - Constants
private enum PluginConstants {
static let plugin = "example-plugin/example-plugin.php"
static let pluginName = "Example Plugin"
}
4 changes: 2 additions & 2 deletions WooCommerce/Classes/ViewRelated/Dashboard/Settings/POS/LegacyPOSTabEligibilityChecker.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ private extension LegacyPOSTabEligibilityChecker {

@MainActor
func fetchWooCommercePlugin(siteID: Int64) async -> SystemPlugin {
await pluginsService.waitForPluginInStorage(siteID: siteID, pluginName: Constants.wcPluginName, isActive: true)
await pluginsService.waitForPluginInStorage(siteID: siteID, plugin: Constants.wcPlugin, isActive: true)
}

@MainActor
Expand Down Expand Up @@ -254,7 +254,7 @@ private extension LegacyPOSTabEligibilityChecker {

private extension LegacyPOSTabEligibilityChecker {
enum Constants {
static let wcPluginName = "WooCommerce"
static let wcPlugin = "woocommerce/woocommerce.php"
static let wcPluginMinimumVersion = "9.6.0-beta"
static let wcPluginMinimumVersionWithFeatureSwitch = "10.0.0"
}
Expand Down
4 changes: 2 additions & 2 deletions WooCommerce/Classes/ViewRelated/Dashboard/Settings/POS/POSTabEligibilityChecker.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ private extension POSTabEligibilityChecker {

@MainActor
func fetchWooCommercePlugin(siteID: Int64) async -> SystemPlugin {
await pluginsService.waitForPluginInStorage(siteID: siteID, pluginName: Constants.wcPluginName, isActive: true)
await pluginsService.waitForPluginInStorage(siteID: siteID, plugin: Constants.wcPlugin, isActive: true)
}

@MainActor
Expand Down Expand Up @@ -279,7 +279,7 @@ private extension POSTabEligibilityChecker {

private extension POSTabEligibilityChecker {
enum Constants {
static let wcPluginName = "WooCommerce"
static let wcPlugin = "woocommerce/woocommerce.php"
static let wcPluginMinimumVersion = "9.6.0-beta"
static let wcPluginMinimumVersionWithFeatureSwitch = "10.0.0"
}
Expand Down
4 changes: 2 additions & 2 deletions ...merce/WooCommerceTests/ViewRelated/Settings/POS/LegacyPOSTabEligibilityCheckerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ private extension LegacyPOSTabEligibilityCheckerTests {
func setupWooCommerceVersion(_ version: String = "9.6.0-beta") {
pluginsService.pluginToReturn = .fake().copy(
siteID: siteID,
plugin: "WooCommerce",
plugin: "woocommerce/woocommerce.php",
version: version,
active: true
)
Expand Down Expand Up @@ -426,7 +426,7 @@ private extension LegacyPOSTabEligibilityCheckerTests {
private final class MockPluginsService: PluginsServiceProtocol {
var pluginToReturn: SystemPlugin = .fake()

func waitForPluginInStorage(siteID: Int64, pluginName: String, isActive: Bool) async -> SystemPlugin {
func waitForPluginInStorage(siteID: Int64, plugin: String, isActive: Bool) async -> SystemPlugin {
pluginToReturn
}
}
Expand Down
4 changes: 2 additions & 2 deletions WooCommerce/WooCommerceTests/ViewRelated/Settings/POS/POSTabEligibilityCheckerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ private extension POSTabEligibilityCheckerTests {
func setupWooCommerceVersion(_ version: String = "9.6.0-beta") {
pluginsService.pluginToReturn = .fake().copy(
siteID: siteID,
plugin: "WooCommerce",
plugin: "woocommerce/woocommerce.php",
version: version,
active: true
)
Expand Down Expand Up @@ -648,7 +648,7 @@ private extension POSTabEligibilityCheckerTests {
private final class MockPluginsService: PluginsServiceProtocol {
var pluginToReturn: SystemPlugin = .fake()

func waitForPluginInStorage(siteID: Int64, pluginName: String, isActive: Bool) async -> SystemPlugin {
func waitForPluginInStorage(siteID: Int64, plugin: String, isActive: Bool) async -> SystemPlugin {
pluginToReturn
}
}
Expand Down