-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add POST requests helper in WordPressOrgRestApi
#589
Conversation
Apply swiftlint corrections Update `testSuccessfulPostCall` test case
WordPressOrgRestApi
2051109
to
c44cda9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving based on my review outlined in wordpress-mobile/WordPress-iOS#20428 (review), but will defer to Tanner's native expertise in case there's anything I may be missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will defer to Tanner's native expertise in case there's anything I may be missing.
I'm going to step in to merge this as wordpress-mobile/WordPress-iOS#20428 depends on it and that PR is already approved and ought to go into Jetpack and WordPress 22.2
I'm doing this a bit earlier than usual (around 12 hours) because I'm trying to get ahead on the code freeze as I have another one to run tomorrow. I don't think there's any point in me waiting till tomorrow because, given we're in the weekend, it's unlikely Tanner will get to it.
} | ||
let expect = self.expectation(description: "One callback should be invoked") | ||
let api = WordPressOrgRestApi(apiBase: apiBase) | ||
let blockContent = "<!-- wp:paragraph -->\n<p>Some text</p>\n<!-- /wp:paragraph -->\n\n<!-- wp:list -->\n<ul><li>Item 1</li><li>Item 2</li><li>Item 3</li></ul>\n<!-- /wp:list -->" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a long value to use in the test given that the response is stubbed. Is there a particular reason for use it? Having an example with real-world-like input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make it shorter, the only condition is to match the content from the stub:
"raw": "<!-- wp:paragraph -->\n<p>Some text<\/p>\n<!-- \/wp:paragraph -->\n\n<!-- wp:list -->\n<ul><li>Item 1<\/li><li>Item 2<\/li><li>Item 3<\/li><\/ul>\n<!-- \/wp:list -->", |
Thanks @mokagio for taking a look at the PR and merging it 🙇. Although this PR is related to wordpress-mobile/WordPress-iOS#20428, we can safely merge it as it only introduces a new function. |
Description
Add a helper function to make POST requests in
WordPressOrgRestApi
. Note that this is just a helper to make POST requests similar toGET
function:WordPressKit-iOS/WordPressKit/WordPressOrgRestApi.swift
Lines 28 to 32 in 27aacb2
Most of the POST requests to WP.org Rest API made from WP-iOS use directly the
request
function passing thePOST
method.Testing Details
WP-iOS is not making use of this helper yet, hence this change can only be tested with a test PR: wordpress-mobile/WordPress-iOS#20430. Specifically, we should test the case POST request to WPORG.
version
in the.podspec
file.CHANGELOG.md
if necessary.