Skip to content

[REST API] Merge WordPressOrgRequest into RESTRequest #8553

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 7 commits into from
Jan 6, 2023
Merged

[REST API] Merge WordPressOrgRequest into RESTRequest #8553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0925eea
Make `RESTRequest` confirm to `Request`
selanthiraiyan Jan 5, 2023
e4a91e9
Add unit tests for `RESTRequest`
selanthiraiyan Jan 5, 2023
43d6afc
Use `RESTRequest` in application password use case.
selanthiraiyan Jan 5, 2023
b587839
Use `RESTRequest` in Jetpack connection remote.
selanthiraiyan Jan 5, 2023
02c0278
Test `RESTRequest` in `RemoteTests`
selanthiraiyan Jan 5, 2023
5915f46
Delete `WordPressOrgRequest`.
selanthiraiyan Jan 5, 2023
5b9705b
Remove `WordPressOrgRequest` from docs.
selanthiraiyan Jan 5, 2023
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
8 changes: 0 additions & 8 deletions Networking/Networking.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,14 +682,12 @@
DE2E8EB1295464C5002E4B14 /* URLRequest+Request.swift in Sources */ = {isa = PBXBuildFile; fileRef = DE2E8EB0295464C5002E4B14 /* URLRequest+Request.swift */; };
DE2E8EB329546501002E4B14 /* PlaceholderDataValidator.swift in Sources */ = {isa = PBXBuildFile; fileRef = DE2E8EB229546501002E4B14 /* PlaceholderDataValidator.swift */; };
DE2E8EB529546648002E4B14 /* WooConstants.swift in Sources */ = {isa = PBXBuildFile; fileRef = DE2E8EB429546648002E4B14 /* WooConstants.swift */; };
DE34051328BDCA5100CF0D97 /* WordPressOrgRequest.swift in Sources */ = {isa = PBXBuildFile; fileRef = DE34051228BDCA5100CF0D97 /* WordPressOrgRequest.swift */; };
DE34051528BDEB1900CF0D97 /* WordPressOrgNetwork.swift in Sources */ = {isa = PBXBuildFile; fileRef = DE34051428BDEB1900CF0D97 /* WordPressOrgNetwork.swift */; };
DE34051728BDEB6D00CF0D97 /* JetpackConnectionRemote.swift in Sources */ = {isa = PBXBuildFile; fileRef = DE34051628BDEB6D00CF0D97 /* JetpackConnectionRemote.swift */; };
DE34051928BDEE6A00CF0D97 /* JetpackConnectionURLMapper.swift in Sources */ = {isa = PBXBuildFile; fileRef = DE34051828BDEE6A00CF0D97 /* JetpackConnectionURLMapper.swift */; };
DE34051B28BDF12C00CF0D97 /* jetpack-connection-url.json in Resources */ = {isa = PBXBuildFile; fileRef = DE34051A28BDF12C00CF0D97 /* jetpack-connection-url.json */; };
DE34051D28BDF1C900CF0D97 /* JetpackConnectionURLMapperTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = DE34051C28BDF1C900CF0D97 /* JetpackConnectionURLMapperTests.swift */; };
DE34051F28BDFB0B00CF0D97 /* JetpackConnectionRemoteTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = DE34051E28BDFB0B00CF0D97 /* JetpackConnectionRemoteTests.swift */; };
DE34052128BDFE3500CF0D97 /* WordPressOrgRequestTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = DE34052028BDFE3500CF0D97 /* WordPressOrgRequestTests.swift */; };
DE50295928C5BD0200551736 /* JetpackUser.swift in Sources */ = {isa = PBXBuildFile; fileRef = DE50295828C5BD0200551736 /* JetpackUser.swift */; };
DE50295B28C5F99700551736 /* DotcomUser.swift in Sources */ = {isa = PBXBuildFile; fileRef = DE50295A28C5F99700551736 /* DotcomUser.swift */; };
DE50295D28C6068B00551736 /* JetpackUserMapper.swift in Sources */ = {isa = PBXBuildFile; fileRef = DE50295C28C6068B00551736 /* JetpackUserMapper.swift */; };
Expand Down Expand Up @@ -1475,14 +1473,12 @@
DE2E8EB0295464C5002E4B14 /* URLRequest+Request.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "URLRequest+Request.swift"; sourceTree = "<group>"; };
DE2E8EB229546501002E4B14 /* PlaceholderDataValidator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PlaceholderDataValidator.swift; sourceTree = "<group>"; };
DE2E8EB429546648002E4B14 /* WooConstants.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WooConstants.swift; sourceTree = "<group>"; };
DE34051228BDCA5100CF0D97 /* WordPressOrgRequest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WordPressOrgRequest.swift; sourceTree = "<group>"; };
DE34051428BDEB1900CF0D97 /* WordPressOrgNetwork.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = WordPressOrgNetwork.swift; sourceTree = "<group>"; };
DE34051628BDEB6D00CF0D97 /* JetpackConnectionRemote.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = JetpackConnectionRemote.swift; sourceTree = "<group>"; };
DE34051828BDEE6A00CF0D97 /* JetpackConnectionURLMapper.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = JetpackConnectionURLMapper.swift; sourceTree = "<group>"; };
DE34051A28BDF12C00CF0D97 /* jetpack-connection-url.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = "jetpack-connection-url.json"; sourceTree = "<group>"; };
DE34051C28BDF1C900CF0D97 /* JetpackConnectionURLMapperTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = JetpackConnectionURLMapperTests.swift; sourceTree = "<group>"; };
DE34051E28BDFB0B00CF0D97 /* JetpackConnectionRemoteTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = JetpackConnectionRemoteTests.swift; sourceTree = "<group>"; };
DE34052028BDFE3500CF0D97 /* WordPressOrgRequestTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WordPressOrgRequestTests.swift; sourceTree = "<group>"; };
DE50295828C5BD0200551736 /* JetpackUser.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = JetpackUser.swift; sourceTree = "<group>"; };
DE50295A28C5F99700551736 /* DotcomUser.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DotcomUser.swift; sourceTree = "<group>"; };
DE50295C28C6068B00551736 /* JetpackUserMapper.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = JetpackUserMapper.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1837,7 +1833,6 @@
B519A3E82097A91800E2603A /* Requests */ = {
isa = PBXGroup;
children = (
DE34052028BDFE3500CF0D97 /* WordPressOrgRequestTests.swift */,
B567AF2D20A0FB8F00AB6C62 /* DotcomRequestTests.swift */,
B567AF2E20A0FB8F00AB6C62 /* JetpackRequestTests.swift */,
020D0C02291504DE00BB3DCE /* UnauthenticatedRequestTests.swift */,
Expand Down Expand Up @@ -1979,7 +1974,6 @@
E1BAB2C02913F99500C3982B /* Request.swift */,
B557DA0E20975E07005962F4 /* DotcomRequest.swift */,
B557D9FF209754FF005962F4 /* JetpackRequest.swift */,
DE34051228BDCA5100CF0D97 /* WordPressOrgRequest.swift */,
029C9E5B291507A40013E5EE /* UnauthenticatedRequest.swift */,
DEFBA74D29485A7600C35BA9 /* RESTRequest.swift */,
EEFAA57A295D7793003583BE /* AuthenticatedDotcomRequest.swift */,
Expand Down Expand Up @@ -3181,7 +3175,6 @@
7426CA0D21AF27B9004E9FFC /* SiteAPIRemote.swift in Sources */,
023930632918FF5400B2632F /* DomainRemote.swift in Sources */,
451A97D1260A03900059D135 /* ShippingLabelCustomPackage.swift in Sources */,
DE34051328BDCA5100CF0D97 /* WordPressOrgRequest.swift in Sources */,
D88D5A45230BC6F9007B6E01 /* ProductReviewsRemote.swift in Sources */,
B59325D4217E4206000B0E8E /* NoteBlock.swift in Sources */,
DEC51AF92769A212009F3DF4 /* SystemStatus+Settings.swift in Sources */,
Expand Down Expand Up @@ -3600,7 +3593,6 @@
7412A8EE21B6E335005D182A /* ReportOrderMapperTests.swift in Sources */,
AED8AEBC272A997500663FCC /* IgnoringResponseMapperTests.swift in Sources */,
74CF0A8C22414D7800DB993F /* ProductMapperTests.swift in Sources */,
DE34052128BDFE3500CF0D97 /* WordPressOrgRequestTests.swift in Sources */,
45152815257A83DD0076B03C /* ProductAttributesRemoteTests.swift in Sources */,
B505F6D720BEE58800BB1B69 /* AccountRemoteTests.swift in Sources */,
453305EB2459E01A00264E50 /* PostMapperTests.swift in Sources */,
Expand Down
4 changes: 2 additions & 2 deletions Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ private extension DefaultApplicationPasswordUseCase {
let passwordName = await applicationPasswordName

let parameters = [ParameterKey.name: passwordName]
let request = WordPressOrgRequest(baseURL: siteAddress, method: .post, path: Path.applicationPasswords, parameters: parameters)
let request = RESTRequest(siteURL: siteAddress, method: .post, path: Path.applicationPasswords, parameters: parameters)
return try await withCheckedThrowingContinuation { continuation in
network.responseData(for: request) { result in
switch result {
Expand Down Expand Up @@ -179,7 +179,7 @@ private extension DefaultApplicationPasswordUseCase {

let passwordName = await applicationPasswordName
let parameters = [ParameterKey.name: passwordName]
let request = WordPressOrgRequest(baseURL: siteAddress, method: .delete, path: Path.applicationPasswords, parameters: parameters)
let request = RESTRequest(siteURL: siteAddress, method: .delete, path: Path.applicationPasswords, parameters: parameters)

try await withCheckedThrowingContinuation { continuation in
network.responseData(for: request) { result in
Expand Down
10 changes: 5 additions & 5 deletions Networking/Networking/Remote/JetpackConnectionRemote.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public final class JetpackConnectionRemote: Remote {
///
public func retrieveJetpackPluginDetails(completion: @escaping (Result<SitePlugin, Error>) -> Void) {
let path = "\(Path.plugins)/\(Constants.jetpackPluginName)"
let request = WordPressOrgRequest(baseURL: siteURL, method: .get, path: path)
let request = RESTRequest(siteURL: siteURL, method: .get, path: path)
let mapper = SitePluginMapper()
enqueue(request, mapper: mapper, completion: completion)
}
Expand All @@ -26,7 +26,7 @@ public final class JetpackConnectionRemote: Remote {
///
public func installJetpackPlugin(completion: @escaping (Result<SitePlugin, Error>) -> Void) {
let parameters: [String: Any] = [Field.slug.rawValue: Constants.jetpackPluginSlug]
let request = WordPressOrgRequest(baseURL: siteURL, method: .post, path: Path.plugins, parameters: parameters)
let request = RESTRequest(siteURL: siteURL, method: .post, path: Path.plugins, parameters: parameters)
let mapper = SitePluginMapper()
enqueue(request, mapper: mapper, completion: completion)
}
Expand All @@ -36,15 +36,15 @@ public final class JetpackConnectionRemote: Remote {
public func activateJetpackPlugin(completion: @escaping (Result<SitePlugin, Error>) -> Void) {
let path = "\(Path.plugins)/\(Constants.jetpackPluginName)"
let parameters: [String: Any] = [Field.status.rawValue: Constants.activeStatus]
let request = WordPressOrgRequest(baseURL: siteURL, method: .put, path: path, parameters: parameters)
let request = RESTRequest(siteURL: siteURL, method: .put, path: path, parameters: parameters)
let mapper = SitePluginMapper()
enqueue(request, mapper: mapper, completion: completion)
}

/// Fetches the URL for setting up Jetpack connection.
///
public func fetchJetpackConnectionURL(completion: @escaping (Result<URL, Error>) -> Void) {
let request = WordPressOrgRequest(baseURL: siteURL, method: .get, path: Path.jetpackConnectionURL)
let request = RESTRequest(siteURL: siteURL, method: .get, path: Path.jetpackConnectionURL)
let mapper = JetpackConnectionURLMapper()

enqueue(request, mapper: mapper, completion: completion)
Expand Down Expand Up @@ -88,7 +88,7 @@ public final class JetpackConnectionRemote: Remote {
/// Fetches the user connection state with the site's Jetpack.
///
public func fetchJetpackUser(completion: @escaping (Result<JetpackUser, Error>) -> Void) {
let request = WordPressOrgRequest(baseURL: siteURL, method: .get, path: Path.jetpackConnectionUser)
let request = RESTRequest(siteURL: siteURL, method: .get, path: Path.jetpackConnectionUser)
let mapper = JetpackUserMapper()
enqueue(request, mapper: mapper, completion: completion)
}
Expand Down
10 changes: 7 additions & 3 deletions Networking/Networking/Requests/RESTRequest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import Foundation
import Alamofire

/// Wraps up a URLRequestConvertible Instance, and injects the Authorization + User Agent whenever the actual Request is required.
/// Represents a WordPress.org REST API request
///
struct RESTRequest: URLRequestConvertible {
struct RESTRequest: Request {
/// URL of the site to make the request with
///
let siteURL: String
Expand Down Expand Up @@ -33,7 +33,7 @@ struct RESTRequest: URLRequestConvertible {
/// - parameters: Collection of String parameters to be passed over to our target endpoint.
///
init(siteURL: String,
wooApiVersion: WooAPIVersion,
wooApiVersion: WooAPIVersion = .none,
method: HTTPMethod,
path: String,
parameters: [String: Any] = [:]) {
Expand All @@ -59,6 +59,10 @@ struct RESTRequest: URLRequestConvertible {
return try URLEncoding.default.encode(request, with: parameters)
}
}

func responseDataValidator() -> ResponseDataValidator {
PlaceholderDataValidator()
}
}

extension RESTRequest {
Expand Down
43 changes: 0 additions & 43 deletions Networking/Networking/Requests/WordPressOrgRequest.swift
View file
Open in desktop

This file was deleted.

18 changes: 11 additions & 7 deletions Networking/NetworkingTests/Remote/RemoteTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,28 +423,32 @@ final class RemoteTests: XCTestCase {
XCTAssertNotNil(result)
}

/// Verifies that WordPressOrg request parses WordPressApiError
/// Verifies that RESTRequest request doesn't parse WordPressApiError
///
func test_wordpress_org_request_parses_wordpress_api_error() async throws {
func test_wordpress_org_request_does_not_parse_wordpress_api_error() async throws {
Copy link
Contributor

@itsmeichigo itsmeichigo Jan 6, 2023

Choose a reason for hiding this comment

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

I don't understand this test case, should the request parse the error though?

Copy link
Contributor Author

@selanthiraiyan selanthiraiyan Jan 6, 2023

Choose a reason for hiding this comment

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

The original test case validated that WordPressApiValidator is used in WordPressOrgRequest.

In RESTRequest we decided to use PlaceholderDataValidator which will not validate the request. So, I updated the test case to check that no errors are parsed.
i.e. test_wordpress_org_request_does_not_parse_wordpress_api_error and test_wordpress_org_request_does_not_parse_dotcom_error will ensure that PlaceholderDataValidator is used.

// Given
let network = MockNetwork()
let mapper = DummyMapper()
let remote = Remote(network: network)
let request = WordPressOrgRequest(baseURL: "https://example.com", method: .get, path: "mock")
let request = RESTRequest(siteURL: "https://example.com", method: .get, path: "mock")

network.simulateResponse(requestUrlSuffix: "mock", filename: "error-wp-rest-forbidden")
network.simulateResponse(requestUrlSuffix: "mock", filename: "timeout_error")

await assertThrowsError({ _ = try await remote.enqueue(request, mapper: mapper)}, errorAssert: { $0 is WordPressApiError })
// When
let result = try await remote.enqueue(request, mapper: mapper)

// Then
XCTAssertNotNil(result)
Copy link
Contributor

@itsmeichigo itsmeichigo Jan 6, 2023

Choose a reason for hiding this comment

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

Should we check that result is failure because we are mocking a timeout error?

Copy link
Contributor Author

@selanthiraiyan selanthiraiyan Jan 6, 2023

Choose a reason for hiding this comment

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

I followed this pattern from the test_wordpress_org_request_does_not_parse_dotcom_error method.

The idea here is ensure try await remote.enqueue(request, mapper: mapper) doesn't throw any error.

Please note that the result is of type Data from DummyMapper, and not of Swift.Result<Data, Error> type.

}

/// Verifies that WordPressOrg request doesn't parse DotcomError
/// Verifies that RESTRequest request doesn't parse DotcomError
///
func test_wordpress_org_request_does_not_parse_dotcom_error() async throws {
// Given
let network = MockNetwork()
let mapper = DummyMapper()
let remote = Remote(network: network)
let request = WordPressOrgRequest(baseURL: "https://example.com", method: .get, path: "mock")
let request = RESTRequest(siteURL: "https://example.com", method: .get, path: "mock")

network.simulateResponse(requestUrlSuffix: "mock", filename: "timeout_error")

Expand Down
35 changes: 35 additions & 0 deletions Networking/NetworkingTests/Requests/RESTRequestTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,41 @@ final class RESTRequestTests: XCTestCase {
///
private let sampleParameters = ["some": "thing", "yo": "semite"]

func test_request_url_is_correct() throws {
// Given
let request = RESTRequest(siteURL: sampleSiteAddress, method: .get, path: sampleRPC)

// When
let url = try XCTUnwrap(request.asURLRequest().url)

// Then
let expectedURL = "https://wordpress.com/wp-json/sample"
assertEqual(url.absoluteString, expectedURL)
}

func test_request_method_is_correct() throws {
// Given
let request = RESTRequest(siteURL: sampleSiteAddress, method: .get, path: sampleRPC)

// When
let urlRequest = try request.asURLRequest()

// Then
assertEqual(urlRequest.httpMethod, "GET")
}

func test_request_wooApiVersion_is_correct() throws {
// Given
let request = RESTRequest(siteURL: sampleSiteAddress, wooApiVersion: sampleWooApiVersion, method: .get, path: sampleRPC)

// When
let url = try XCTUnwrap(request.asURLRequest().url)

// Then
let expectedURL = "https://wordpress.com/wp-json/wc/v3/sample"
assertEqual(url.absoluteString, expectedURL)
}

func test_it_uses_JSON_encoding_for_post_method() throws {
// Given
let request = RESTRequest(siteURL: sampleSiteAddress, wooApiVersion: sampleWooApiVersion, method: .post, path: sampleRPC, parameters: sampleParameters)
Expand Down
31 changes: 0 additions & 31 deletions Networking/NetworkingTests/Requests/WordPressOrgRequestTests.swift
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion docs/NETWORKING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ At the moment, we provide four implementations of `URLRequestConvertible`:
* [`RESTRequest`](../Networking/Networking/Requests/RESTRequest.swift) represents a REST API request sent to the site (instead of through the Jetpack tunnel) directly.
* [`AuthenticatedDotcomRequest`](../Networking/Networking/Requests/AuthenticatedDotcomRequest.swift) Wraps up a `URLRequestConvertible` instance, and injects WordPress.com authentication token.
* [`AuthenticatedRESTRequest`](../Networking/Networking/Requests/AuthenticatedRESTRequest.swift) Wraps up a `URLRequestConvertible` instance, and injects application password.
* [`WordPressOrgRequest`](../Networking/Networking/Requests/WordPressOrgRequest.swift) model requests to the WordPress.org REST API.
* [`UnauthenticatedRequest`](../Networking/Networking/Requests/UnauthenticatedRequest.swift) Wraps up a `URLRequestConvertible` instance, and injects a custom user-agent header

## [`Mapper`](../Networking/Networking/Mapper/Mapper.swift)
Expand Down