Improve Plone Site Root object API #672
Conversation
|
@sneridagh thanks for creating this Pull Request and help improve Plone! To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass. Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating: With this simple comment all the jobs will be started automatically. Happy hacking! |
|
@jenkins-plone-org please run jobs |
There was a problem hiding this comment.
LGTM. Maybe @lukasgraf or @buchi want to have a look as well...
| hiding the implementation details (the Plone Site Root has no workflow for | ||
| historical reasons) and equalizing it to the other content types, making the | ||
| API more consistent. | ||
| [sneridagh] |
There was a problem hiding this comment.
Uhm, I'm not a fan of mixing together two such totally unrelated changes, let alone in a single commit. The fact that the changelog entry needs 6 lines should be an indication that this should be split up some more.
There was a problem hiding this comment.
I tried to improve the wording and I split up the entry. @sneridagh please double check if you are happy with my amendment.
There was a problem hiding this comment.
@tisto thank you! IMHO that summarizes the changes perfectly. From what I understand, the "opt-in" part just referred to the fact that that the tiles-specific fields aren't required in the deserializer, which is the case in most places in the API, so that can be omitted.
There was a problem hiding this comment.
@tisto Absolutelly fine with me!!
| # API we fake the response to the endpoint by providing an empty | ||
| # response instead of a 404. | ||
| if IPloneSiteRoot.providedBy(self.context): | ||
| result['workflow'].update({'history': [], 'transitions': []}) |
There was a problem hiding this comment.
Immediately return result here? No point in even trying all the rest, right? That would make it clearer what happens here.
| @@ -0,0 +1,46 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
There already is an existing src/plone/restapi/tests/test_site_deserializer.py - can you please integrate your testing changes into that one instead? (And also rename it to something like TestSiteRootDeserializer - the Plone site is not DX content).
There was a problem hiding this comment.
I moved the tests into test_site_deserializer.py and renamed the class name. Thx for the pointer!
| @@ -15,6 +15,7 @@ Content-Type: application/json | |||
| }, | |||
There was a problem hiding this comment.
While the http examples have been updated, the narrative documentation seems to be out of date. https://plonerestapi.readthedocs.io/en/latest/tiles.html needs a major update I think. I'm honestly quite confused at this point when trying to determine which tiles related functionality is implemented, and where, and where the documentation is just stubbed out, talks about future ideas that may or may not happen, or is outdated.
Please, please try to keep the documentation in sync with API changes - it's so very hard to even to attempt to do it after the fact, after things have changed over time.
There was a problem hiding this comment.
@lukasgraf you are absolutely correct. Thanks for pointing that out. I created a separate issue for that since it is not directly related to the PR. #674
|
@lukasgraf thanks for your detailed and thoughtful review (as always)! I will look into the tiles endpoint update asap. |
|
@jenkins-plone-org please run jobs |
|
@lukasgraf Thanks for the review!! Agreed that we have to improve the documentation in both plone.restapi and Volto itself (almost not existent) that's a must that we should address in the next major sprint. |
Addresses #671
/cc @tisto