Add autosave attributes to RemotePost #192
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Autosave metadata is supported through the new `meta` option when fetching posts. Autosave metadata currently supported are: `title`, `excerpt`, and `content`.
Simplify the treatment of null values when creating a `RemotePost` from a dictionary representing its JSON data.
I'm unsure if `meta` is supported in XML-RPC, so removing this comment.
guarani
commented
Oct 27, 2019
Although a good idea, specifying `nullable` triggers warnings on the other properties of this class. To resolve the warnings we would have to either explicitly annotate each property as nullable or nonnull - or, if all properties are not nullable, mark the class with NS_ASSUME_NONNULL_BEGIN.
guarani
changed the title
Update: Autosave is not supported by the XML RPC API. |
shiki
requested changes
Oct 30, 2019
There was a problem hiding this comment.
It's looking good, @guarani. I have a couple of comments.
Do you think it's possible to add a unit test that confirms that the autoSave properties are copied to RemotePost? Probably using a mock JSON like PostServiceRemoteRESTRevisionsTest.
When the fetch posts REST endpoint is called, autosave properties are mapped to the RemotePost object.
|
@shiki, yes. I've now added a test |
1 task
`open` allowed subclassing of RemotePostAutosave and overriding of its properties, `public` disallows this. Further restiction to disallow setting the properties from outside the module using internal(set) doesn't work due to Obj-C interoperability issues.
|
Thanks again for the review, @shiki! I've updated the PR to address your comments. |
guarani
commented
Nov 6, 2019
If the fetch posts response JSON returns autosave as null, that's converted to NSNull, which is a truthy value. Updating to treat NSNull as no autosave.
|
Thank you, @shiki! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Post autosave attributes are added to
RemotePost- the type that describes a post (or page) fetched from the API. Autosave data is obtained by includingautosavein themetaparameter, but at the time of writing this is undocumented.Issue: wordpress-mobile/WordPress-iOS#12141
REST API
The title, content, excerpt and date of last modification are fetched from the following REST API endpoint:
https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/1357/files
https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/1391/files
Testing Details
These changes are best tested alongside the matching WordPress-iOS PR wordpress-mobile/WordPress-iOS#12826
Unit Tests
The new
metaparameter uses the existing mechanism for specifying options,PostServiceRemoteOptions. There is already a test that asserts that arbitrary options are correctly set on the request payload (- testThatGetPostsOfTypeWithOptionsWorks), so I think the current tests have this covered.