Fix and test for include_total not working #4448
Conversation
…validator was broken.
| assert res_dict['success'] is True | ||
| result = res_dict['result'] | ||
| assert 'total' not in result | ||
|
|
There was a problem hiding this comment.
Would you add this to the class at the top instead of with the legacy tests and like those tests use helpers.call_action to skip the request processing bits.
ckanext/datastore/controller.py
Outdated
| @@ -143,7 +143,7 @@ def result_page(offs, lim): | |||
| 'offset': offs, | |||
| 'sort': '_id', | |||
| 'records_format': records_format, | |||
| 'include_total': 'false', # XXX: default() is broken | |||
| 'include_total': 'false', | |||
There was a problem hiding this comment.
we can change this to False now 🙌
|
@wardi thanks for the review. I've made those two improvements now :) |
|
@amercader mentioned that the change to |
|
I've added these things to the changelog. I've also removed an unnecessary total/ This PR now gets rid of 2 of the 4 |
CHANGELOG.rst
Outdated
| Minor changes: | ||
|
|
||
| * For navl schemas, the 'default' validator no longer applies the default when | ||
| the value is False, (), [] or {} (#4448) |
There was a problem hiding this comment.
please remove () (shouldn't appear as part of a json payload) and add 0 to this list.
ckan/lib/navl/validators.py
Outdated
| @@ -74,6 +74,8 @@ def ignore(key, data, errors, context): | |||
| raise StopOnError | |||
|
|
|||
| def default(default_value): | |||
| '''When key is missing or value is 'empty', set it with a default value. | |||
| See the code for how 'empty' is defined.''' | |||
There was a problem hiding this comment.
I don't think it hurts to repeat ourselves here.
When key is missing or value is an empty string or None, replace it with a default value
One day we could include the validator docstrings in the proper docs.
| @@ -2280,7 +2280,8 @@ def resource_view_get_fields(resource): | |||
|
|
|||
| data = { | |||
| 'resource_id': resource['id'], | |||
| 'limit': 0 | |||
| 'limit': 0, | |||
| 'include_total': False, | |||
|
Comments addressed! |
include_total wasn't working because of the default() validator was broken, so I fixed that.
Fixes #4446
Proposed fixes:
Features:
Please [X] all the boxes above that apply