[Jetpack Remote Install] New Blog service API #124
Conversation
aerych
left a comment
aerych
left a comment
There was a problem hiding this comment.
Heya @danielebogo!
Looking good! All tests pass. I left a couple of nitpicks in comments, but the one about the locale query param is the only one I'd consider a blocker.
Back to you sir!
| case unknown | ||
|
|
||
| init?(error code: String?) { | ||
| guard let code = code else { |
There was a problem hiding this comment.
If you wanted to get rid of the guard you could add a default value to code like so:
init?(error code: String = JetpackInstallError.unknown.rawValue)
There was a problem hiding this comment.
At the end I removed the guard without using the default value. Since the init(rawValue:) returns an optional I can use the nil-coalescing to use .unknown as fallback.
| completion(false, .unknown) | ||
| return | ||
| } | ||
| let path = String(format: "jetpack-install/%@/?locale=en_US", escapedURL) |
There was a problem hiding this comment.
?locale=en_US I think this is unnecessary (and we probably don't want it hard-coded to en_US regardless). @stevebaranski recently updated things so the locale is automatically added and set to the users preferred device language (iirc).
There was a problem hiding this comment.
Thanks for the ping @aerych!
Yes @danielebogo, if appendsPreferredLanguageLocale is true on the underlying WordPressComRestApi instance, this is indeed "free" behavior. Most of the changes I made to this method were focused on preventing duplicate parameter entries that were possible with the logic in-place.
Please let me know if you have questions or if you need additional information.
There was a problem hiding this comment.
Thanks @aerych and @stevebaranski for the hint 😊
| let jetpackRemoteErrorActivationResponseMockFilename = "blog-service-jetpack-remote-error-activation-response.json" | ||
| let jetpackRemoteErrorActivationFailureMockFilename = "blog-service-jetpack-remote-error-activation-failure.json" | ||
|
|
||
| var endpoint: String { return "jetpack-install/\(encodedURL)/?locale=en_US" } |
There was a problem hiding this comment.
?locale=en_US
Just a note to remove locale here as well if its indeed unnecessary.
|
Thanks @aerych ! I fixed the locale and modified a bit the enum init. It's ready for another look! |
aerych
left a comment
aerych
left a comment
There was a problem hiding this comment.
Looks good! I re-ran tests and everything passed.
sir!
|
Thanks @aerych ! |
Description
Refs. #10277
This PR introduces the new API used to download/install/activate the Jetpack plugin in a self-hosted site.
I created a new BlogServiceRemoteREST extension adding the new API and the errors enum JetpackInstallError.
The expected API success response is:
The expected failure response is:
Testing Details
The new API is fully covered by Unit Tests. Simply run
BlogServiceRemoteRESTTests+Jetpack. It contains the Unit Tests to cover all these cases:Success ->
status: trueFailure ->
status: falseError -> Every
JetpackInstallErrorhas been coveredUnknown Error -> If an error is not a
JetpackInstallErrorcase.Please check here if your pull request includes additional test coverage.