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.

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

Rewrite WordPressOrgRestApi #724

Merged
merged 17 commits into from
Feb 16, 2024
Merged

Rewrite WordPressOrgRestApi #724

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
f545297
Add a new appendURLString function to HTTPRequestBuilder
crazytonyli Feb 12, 2024
b01de44
Use the new appendURLString function in WordPressComRestApi
crazytonyli Feb 12, 2024
6f10cf4
Add a new .org REST API client
crazytonyli Dec 18, 2023
535f3d4
WordPressOrgRestApi now works with URL that ends with `wp-json`
crazytonyli Feb 7, 2024
26e7117
Refactor plugin management implementation
crazytonyli Jan 23, 2024
913a32e
Add unit tests for plugin management
crazytonyli Feb 7, 2024
52fb701
Remove WordPressRestApi
crazytonyli Feb 8, 2024
660f8b2
Use WordPressOrgRestApi in BlockEditorSettingsServiceRemote
crazytonyli Feb 8, 2024
a7867c5
Update test cases for WordPressOrgRestApi.GET
crazytonyli Feb 8, 2024
5d1f6ba
Move unit tests for wp/v2 path in WP.com
crazytonyli Feb 8, 2024
4777462
Remove WordPressRestApi conformance from WordPressComRestApi
crazytonyli Feb 12, 2024
614daf8
Add a changelog
crazytonyli Feb 15, 2024
5e77bcf
Remove unused Authenticator types
crazytonyli Feb 16, 2024
4f4b6b9
Format code comments
crazytonyli Feb 16, 2024
49f177f
Use let instead of var
crazytonyli Feb 16, 2024
a65b40b
Remove unnecessary parentheses
crazytonyli Feb 16, 2024
c44bb13
Remove all stubs upon tearing down test case
crazytonyli Feb 16, 2024
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
155 changes: 76 additions & 79 deletions WordPressKit/Plugin Management/SelfHostedPluginManagementClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,89 +7,74 @@ public class SelfHostedPluginManagementClient: PluginManagementClient {

// MARK: - Get
public func getPlugins(success: @escaping (SitePlugins) -> Void, failure: @escaping (Error) -> Void) {
remote.GET(path(), parameters: nil) { (result, _) in
switch result {
case .success(let responseObject):
guard let response = responseObject as? [[String: AnyObject]] else {
failure(PluginServiceRemote.ResponseError.decodingFailure)
return
Task { @MainActor in
await remote.get(path: path(), type: [PluginStateResponse].self)
.mapError { error -> Error in
if case let .unparsableResponse(_, _, underlyingError) = error, underlyingError is DecodingError {
return PluginServiceRemote.ResponseError.decodingFailure
}
return error
}
.map {
SitePlugins(
plugins: $0.compactMap { self.pluginState(with: $0) },
capabilities: SitePluginCapabilities(modify: true, autoupdate: false)
)
}
.execute(onSuccess: success, onFailure: failure)

let plugins = response.compactMap { (obj) -> PluginState? in
self.pluginState(with: obj)
}

let result = SitePlugins(plugins: plugins,
capabilities: SitePluginCapabilities(modify: true, autoupdate: false))
success(result)

case .failure(let error):
failure(error)
}
}
}

// MARK: - Activate / Deactivate
public func activatePlugin(pluginID: String, success: @escaping () -> Void, failure: @escaping (Error) -> Void) {
let parameters = ["status": "active"] as [String: AnyObject]
let parameters = ["status": "active"]
let path = self.path(with: pluginID)
remote.request(method: .put, path: path, parameters: parameters) { (result, _) in
switch result {
case .success:
success()

case .failure(let error):
failure(error)
}
Task { @MainActor in
await remote.perform(.put, path: path, parameters: parameters, type: AnyResponse.self)
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered adding methods for PUT and other HTTP verbs, for consistency with the POST and GET implementations?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the AnyResponse type and related .map { _ in } took me a bit to understand.

I read them as a way to deal with an API which returns no data, or for which we don't care about the data.

What do you think of pushing this knowledge down at the WordPressOrgRestApi level?

We could have a perform implementation that returns WordPressAPIResult<Void, WordPressOrgRestApiError>, allowing us to segregate the AnyResponse + .map { _ in } "trick" lower into the implementation layer.

If instead it was implemented at this level because it's a one-off kind of thing, I'd still suggest centralizing the implementation into a fileprivate extension WordPressOrgRestApi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have you considered adding methods for PUT and other HTTP verbs, for consistency with the POST and GET implementations?

The post and get functions are convenience functions because the majority of HTTP requests uses these two methods. I didn't add convenience functions for other methods simply because they are not used by many places (maybe only one or two places?).

What do you think of pushing this knowledge down at the WordPressOrgRestApi level?

IMO, It's kind of strange to ignore the response. Adding such support to WordPressOrgRestApi feels like we are encouraging it, but I don't think we should... Also, I believe this is the only place that ignores HTTP API response. I don't want to complicate WordPressOrgRestApi too much for this one case.

.map { _ in }
.execute(onSuccess: success, onFailure: failure)

}
}

public func deactivatePlugin(pluginID: String, success: @escaping () -> Void, failure: @escaping (Error) -> Void) {
let parameters = ["status": "inactive"] as [String: AnyObject]
let parameters = ["status": "inactive"]
let path = self.path(with: pluginID)
remote.request(method: .put, path: path, parameters: parameters) { (result, _) in
switch result {
case .success:
success()

case .failure(let error):
failure(error)
}
Task { @MainActor in
await remote.perform(.put, path: path, parameters: parameters, type: AnyResponse.self)
.map { _ in }
.execute(onSuccess: success, onFailure: failure)
}

}

// MARK: - Install / Uninstall
public func install(pluginSlug: String, success: @escaping (PluginState) -> Void, failure: @escaping (Error) -> Void) {
let parameters = ["slug": pluginSlug] as [String: AnyObject]

remote.request(method: .post, path: path(), parameters: parameters) { (result, _) in
switch result {
case .success(let responseObject):
guard let response = responseObject as? [String: AnyObject],
let plugin = self.pluginState(with: response) else {
failure(PluginServiceRemote.ResponseError.decodingFailure)
return
let parameters = ["slug": pluginSlug]
Task { @MainActor in
await remote.post(path: path(), parameters: parameters, type: PluginStateResponse.self)
.mapError { error -> Error in
if case let .unparsableResponse(_, _, underlyingError) = error, underlyingError is DecodingError {
return PluginServiceRemote.ResponseError.decodingFailure
}

success(plugin)
case .failure(let error):
failure(error)
}
return error
}
.flatMap {
guard let state = self.pluginState(with: $0) else {
return .failure(PluginServiceRemote.ResponseError.decodingFailure)
}
return .success(state)
}
.execute(onSuccess: success, onFailure: failure)
}
}

public func remove(pluginID: String, success: @escaping () -> Void, failure: @escaping (Error) -> Void) {
let path = self.path(with: pluginID)

remote.request(method: .delete, path: path, parameters: nil) { (result, _) in
switch result {
case .success:
success()

case .failure(let error):
failure(error)
}
Task { @MainActor in
await remote.perform(.delete, path: path, type: AnyResponse.self)
.map { _ in }
.execute(onSuccess: success, onFailure: failure)
}
}

Expand All @@ -104,40 +89,29 @@ public class SelfHostedPluginManagementClient: PluginManagementClient {
return returnPath
}

/// Converts an incoming dictionary response to a PluginState struct
/// - Returns: Returns nil if the dictionary does not pass validation
private func pluginState(with obj: [String: AnyObject]) -> PluginState? {
private func pluginState(with response: PluginStateResponse) -> PluginState? {
guard
let id = obj["plugin"] as? String,

// The slugs returned are in the form of XXX/YYY
// The PluginStore uses slugs that are just XXX
// Extract that information out
let slug = id.components(separatedBy: "/").first,

let active = obj["status"] as? String,
let name = obj["name"] as? String,
let author = obj["author"] as? String,
let version = obj["version"] as? String
let slug = response.plugin.components(separatedBy: "/").first
else {
return nil
}

let isActive = active == "active"

// Find the URL
let url = URL(string: (obj["plugin_uri"] as? String) ?? "")
let isActive = response.status == "active"

return PluginState(id: id,
return PluginState(id: response.plugin,
slug: slug,
active: isActive,
name: name,
author: author,
version: version,
name: response.name,
author: response.author,
version: response.version,
updateState: .updated, // API Doesn't support this yet
autoupdate: false, // API Doesn't support this yet
automanaged: false, // API Doesn't support this yet
url: url,
// TODO: Return nil instead of an empty URL when 'plugin_uri' is nil?
url: URL(string: response.pluginURI ?? ""),
settingsURL: nil)
}

Expand All @@ -163,3 +137,26 @@ public class SelfHostedPluginManagementClient: PluginManagementClient {
activatePlugin(pluginID: pluginID, success: success, failure: failure)
}
}

private struct PluginStateResponse: Decodable {
enum CodingKeys: String, CodingKey {
case plugin = "plugin"
case status = "status"
case name = "name"
case author = "author"
case version = "version"
case pluginURI = "plugin_uri"
Copy link
Contributor

Choose a reason for hiding this comment

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

There's always one that gets in the way of the automatic key generation 😅

}
var plugin: String
var status: String
var name: String
var author: String
var version: String
var pluginURI: String?
}

private struct AnyResponse: Decodable {
init(from decoder: Decoder) throws {
// Do nothing
}
}