allow including tokens in querystring vocabs #1403
Conversation
|
@jackahl thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
✅ Deploy Preview for plone-restapi canceled.
|
| self.assertTrue( | ||
| all(elem in idx["values"].items() for elem in expected_vocab_values.items()) |
There was a problem hiding this comment.
@ksuess I removed this check as it only was breaking with the new inclusion of tokens inside of vocabularies returned in plone/plone.app.querystring#107.
Also I was not able to determine what is tested here anyways in a glimbs, as this nesting of loops, although short and elegant, is not really easy to read.
There was a problem hiding this comment.
I am OK with your refactoring, especially because this test 'test_endpoint_inlines_vocabularies' is not about testing the existence of review states, but about testing the format of the response of the 'querystring' endpoint.
| self.assertTrue( | ||
| all(elem in idx["values"].items() for elem in expected_vocab_values.items()) |
There was a problem hiding this comment.
I am OK with your refactoring, especially because this test 'test_endpoint_inlines_vocabularies' is not about testing the existence of review states, but about testing the format of the response of the 'querystring' endpoint.
| @@ -99,10 +99,26 @@ def test_endpoint_inlines_vocabularies(self): | |||
| "published": {"title": "Published with accent \xe9 [published]"}, | |||
| "visible": {"title": "Public draft [visible]"}, | |||
| } | |||
|
|
|||
| self.assertTrue("external" in (idx["values"].keys())) | |||
| @@ -99,10 +99,26 @@ def test_endpoint_inlines_vocabularies(self): | |||
| "published": {"title": "Published with accent \xe9 [published]"}, | |||
| "visible": {"title": "Public draft [visible]"}, | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
With your refactoring of the tests, the 'expected_vocab_values' can already be updated with the additional token.
|
@jenkins-plone-org please run jobs |
|
Thanks for requesting a review. See my review above. |
|
@ksuess could you pls confirm that all requested changes have been addressed? |
|
@jackahl let's discuss this today or next week... |
fix for #1402. Required to be able to merge plone/plone.app.querystring#107