Refs #534 - Control Panels: dexterity-types #907
Conversation
|
@avoinea 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! |
1 similar comment
|
@avoinea thank you! I will try to find time in the next days to review this PR. |
There was a problem hiding this comment.
Overall this PR looks good to me. Great work @avoinea!!! Most of my comments are nitpicking and recommendations more than requests for changes. Though, we definitely need a few unit/integration tests for this new endpoint to keep our high standards. :)
Sidenote: I just realized that coveralls is broken (I will look into this), otherwise it would complain about the drop in test coverage this PR would introduce.
docs/source/controlpanels.rst
Outdated
| Dexterity Types | ||
| ^^^^^^^^^^^^^^^ | ||
|
|
||
| ``@controlpanels/dexterity-types`` is a custom folderish control panel endpoint, that will allow you to add, remove and configure available :ref:`types` |
There was a problem hiding this comment.
Just a note to self. The old control panel is also named "dexterity-types".
There was a problem hiding this comment.
;) I tried to follow the same pattern as for control panels based on plone.app.registry
| if "IDisableCSRFProtection" in dir(plone.protect.interfaces): | ||
| alsoProvides(self.request, plone.protect.interfaces.IDisableCSRFProtection) | ||
|
|
||
| if IPloneRestapiLayer.providedBy(self.request): |
There was a problem hiding this comment.
Just out of curiosity. Why do we do this here?
There was a problem hiding this comment.
Well I don't think CSRF protection make sense with RestAPI calls. Anyway, without it calls will fail with invalid CSRF.
I inspired also from plone.restapi https://github.com/plone/plone.restapi/search?q=IDisableCSRFProtection&unscoped_q=IDisableCSRFProtection
There was a problem hiding this comment.
CSRF is clear. I commented on this line "if IPloneRestapiLayer.providedBy(self.request):"
| @@ -0,0 +1,37 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
I am wondering if we should prepare the file structure for another non-plone.app.registry-based controlpanel. Then we need another directory where we put delete.py into.
| @@ -1345,7 +1345,7 @@ def test_querystringsearch_post(self): | |||
|
|
|||
| class TestDocumentationMessageTranslations(TestDocumentationBase): | |||
|
|
|||
| layer = layer = PLONE_RESTAPI_DX_FUNCTIONAL_TESTING | |||
There was a problem hiding this comment.
Good catch. How the hell did that end up here? :)
| ) | ||
| save_request_and_response_for_docs("controlpanels_post_dexterity_item", response) | ||
|
|
||
| # GET |
There was a problem hiding this comment.
Those should go into individual tests IMHO to indicate what's wrong to the developers in case we have a test failure.
There was a problem hiding this comment.
Oh. Those are just documentation generation tests. Didn't realize that at first. I'd still prefer individual test functions here I guess.
There was a problem hiding this comment.
Inspired from https://github.com/plone/plone.restapi/blob/master/src/plone/restapi/tests/test_documentation.py#L234
But I suppose I can split it 😉
|
FYI: coverage does not work because the branch lives on a different repo. @avoinea would it be possible for future PRs (both Volto and REST API) to create branches on the original repo? It seems forking introduces some problems with our current CI infrastructure. |
|
@avoinea sounds good. Could you close this PR then pls? |
See #534