From 8f88217c5baf9b94671c585f98a2bc52150eddd8 Mon Sep 17 00:00:00 2001 From: David Read Date: Thu, 30 Aug 2018 19:10:58 +0100 Subject: [PATCH 01/11] Add a missing error message --- ckan/lib/cli.py | 1 + 1 file changed, 1 insertion(+) diff --git a/ckan/lib/cli.py b/ckan/lib/cli.py index 8c17a80d2c3..aecc40c2f2e 100644 --- a/ckan/lib/cli.py +++ b/ckan/lib/cli.py @@ -118,6 +118,7 @@ def user_add(args): # Required while '@' not in data_dict.get('email', ''): + print('Error: Invalid email address') data_dict['email'] = input('Email address: ').strip() if 'password' not in data_dict: From 6509ff4592d249605bc0121709b89c6bf2b52223 Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 7 Sep 2018 13:20:24 +0100 Subject: [PATCH 02/11] Fix and test for include_total not working, because of the default() validator was broken. --- ckan/lib/navl/validators.py | 2 +- ckan/tests/legacy/lib/test_navl.py | 15 ----------- ckan/tests/lib/navl/test_validators.py | 36 +++++++++++++++++++++++--- ckanext/datastore/controller.py | 2 +- ckanext/datastore/tests/test_search.py | 12 +++++++++ 5 files changed, 47 insertions(+), 20 deletions(-) diff --git a/ckan/lib/navl/validators.py b/ckan/lib/navl/validators.py index 2fa55411845..d911d5f7047 100644 --- a/ckan/lib/navl/validators.py +++ b/ckan/lib/navl/validators.py @@ -78,7 +78,7 @@ def default(default_value): def callable(key, data, errors, context): value = data.get(key) - if not value or value is missing: + if value is None or value == '' or value is missing: data[key] = default_value return callable diff --git a/ckan/tests/legacy/lib/test_navl.py b/ckan/tests/legacy/lib/test_navl.py index 71bc02ddecc..c884c60fd91 100644 --- a/ckan/tests/legacy/lib/test_navl.py +++ b/ckan/tests/legacy/lib/test_navl.py @@ -186,21 +186,6 @@ def test_basic_errors(): assert errors == {('__junk',): [u"The input field [('4', 1, '30')] was not expected."], ('1',): [u'Missing value'], ('__extras',): [u'The input field __extras was not expected.']}, errors -def test_default(): - schema = { - "__junk": [ignore], - "__extras": [ignore, default("weee")], - "__before": [ignore], - "__after": [ignore], - "0": [default("default")], - "1": [default("default")], - } - - converted_data, errors = validate_flattened(data, schema) - - assert not errors - assert converted_data == {('1',): 'default', ('0',): '0 value'}, converted_data - def test_flatten(): diff --git a/ckan/tests/lib/navl/test_validators.py b/ckan/tests/lib/navl/test_validators.py index b030e32fcca..5643a9eab29 100644 --- a/ckan/tests/lib/navl/test_validators.py +++ b/ckan/tests/lib/navl/test_validators.py @@ -8,6 +8,10 @@ import nose.tools import ckan.tests.factories as factories +import ckan.lib.navl.validators as validators + + +eq_ = nose.tools.eq_ def returns_None(function): @@ -222,7 +226,6 @@ def test_ignore_missing_with_value_missing(self): ''' import ckan.lib.navl.dictization_functions as df - import ckan.lib.navl.validators as validators for value in (None, df.missing, 'skip'): @@ -251,8 +254,6 @@ def test_ignore_missing_with_a_value(self): nothing. ''' - import ckan.lib.navl.validators as validators - key = ('key to be validated',) data = factories.validator_data_dict() data[key] = 'value to be validated' @@ -265,3 +266,32 @@ def test_ignore_missing_with_a_value(self): def call_validator(*args, **kwargs): return validators.ignore_missing(*args, **kwargs) call_validator(key=key, data=data, errors=errors, context={}) + + +class TestDefault(object): + def test_key_doesnt_exist(self): + dict_ = {} + validators.default('default_value')('key', dict_, {}, {}) + eq_(dict_, {'key': 'default_value'}) + + def test_value_is_none(self): + dict_ = {'key': None} + validators.default('default_value')('key', dict_, {}, {}) + eq_(dict_, {'key': 'default_value'}) + + def test_value_is_empty_string(self): + dict_ = {'key': ''} + validators.default('default_value')('key', dict_, {}, {}) + eq_(dict_, {'key': 'default_value'}) + + def test_value_is_false(self): + # False is a consciously set value, so should not be changed to the + # default + dict_ = {'key': False} + validators.default('default_value')('key', dict_, {}, {}) + eq_(dict_, {'key': False}) + + def test_already_has_a_value(self): + dict_ = {'key': 'original'} + validators.default('default_value')('key', dict_, {}, {}) + eq_(dict_, {'key': 'original'}) diff --git a/ckanext/datastore/controller.py b/ckanext/datastore/controller.py index e86a53ca74c..0f8d191d527 100644 --- a/ckanext/datastore/controller.py +++ b/ckanext/datastore/controller.py @@ -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', }) result = result_page(offset, limit) diff --git a/ckanext/datastore/tests/test_search.py b/ckanext/datastore/tests/test_search.py index d1ca715cc0c..e2196483c09 100644 --- a/ckanext/datastore/tests/test_search.py +++ b/ckanext/datastore/tests/test_search.py @@ -634,6 +634,18 @@ def test_search_is_unsuccessful_when_called_with_invalid_fields(self): assert res_dict['success'] is False assert res_dict['error'].get('fields') is not None, res_dict['error'] + def test_search_without_total(self): + data = {'resource_id': self.data['resource_id'], + 'include_total': False} + postparams = '%s=1' % json.dumps(data) + auth = {'Authorization': str(self.normal_user.apikey)} + res = self.app.post('/api/action/datastore_search', params=postparams, + extra_environ=auth) + res_dict = json.loads(res.body) + assert res_dict['success'] is True + result = res_dict['result'] + assert 'total' not in result + class TestDatastoreFullTextSearch(DatastoreLegacyTestBase): @classmethod From a690b4f6b7ef460835bc5f512efa7190b926b7bc Mon Sep 17 00:00:00 2001 From: Konstantin Sivakov Date: Fri, 7 Sep 2018 16:01:44 +0200 Subject: [PATCH 03/11] Fix urls in the breadcrumb --- ckan/templates/package/base.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ckan/templates/package/base.html b/ckan/templates/package/base.html index ad137bdb49b..8ea75b53711 100644 --- a/ckan/templates/package/base.html +++ b/ckan/templates/package/base.html @@ -12,8 +12,8 @@ {% if pkg.organization %} {% set organization = h.get_translated(pkg.organization, 'title') or pkg.organization.name %} {% set group_type = pkg.organization.type %} -
  • {% link_for _('Organizations'), controller='organization', action='index', named_route=group_type + '_index' %}
    • -
  • {% link_for organization|truncate(30), controller='organization', action='read', id=pkg.organization.name, named_route=group_type + '_read' %}
    • +
  • {% link_for _('Organizations'), controller='organization', action='index' %}
    • +
  • {% link_for organization|truncate(30), controller='organization', action='read', id=pkg.organization.name %}
    • {% else %}
  • {% link_for _('Datasets'), named_route='dataset.search' %}
    • {% endif %} From ffc73f6c0b5d04222248df6c076bbeec91bbd9ee Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 7 Sep 2018 15:08:35 +0000 Subject: [PATCH 04/11] Address review comments --- ckanext/datastore/controller.py | 2 +- ckanext/datastore/tests/test_search.py | 29 +++++++++++++++----------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/ckanext/datastore/controller.py b/ckanext/datastore/controller.py index 0f8d191d527..1c80ca2f4b6 100644 --- a/ckanext/datastore/controller.py +++ b/ckanext/datastore/controller.py @@ -143,7 +143,7 @@ def result_page(offs, lim): 'offset': offs, 'sort': '_id', 'records_format': records_format, - 'include_total': 'false', + 'include_total': False, }) result = result_page(offset, limit) diff --git a/ckanext/datastore/tests/test_search.py b/ckanext/datastore/tests/test_search.py index e2196483c09..994e8309c95 100644 --- a/ckanext/datastore/tests/test_search.py +++ b/ckanext/datastore/tests/test_search.py @@ -100,6 +100,23 @@ def test_all_params_work_with_fields_with_whitespaces(self): result_years = [r['the year'] for r in result['records']] assert_equals(result_years, [2013]) + def test_search_without_total(self): + resource = factories.Resource() + data = { + 'resource_id': resource['id'], + 'force': True, + 'records': [ + {'the year': 2014}, + {'the year': 2013}, + ], + } + result = helpers.call_action('datastore_create', **data) + search_data = { + 'resource_id': resource['id'], + 'include_total': False + } + result = helpers.call_action('datastore_search', **search_data) + assert 'total' not in result class TestDatastoreSearch(DatastoreLegacyTestBase): @@ -634,18 +651,6 @@ def test_search_is_unsuccessful_when_called_with_invalid_fields(self): assert res_dict['success'] is False assert res_dict['error'].get('fields') is not None, res_dict['error'] - def test_search_without_total(self): - data = {'resource_id': self.data['resource_id'], - 'include_total': False} - postparams = '%s=1' % json.dumps(data) - auth = {'Authorization': str(self.normal_user.apikey)} - res = self.app.post('/api/action/datastore_search', params=postparams, - extra_environ=auth) - res_dict = json.loads(res.body) - assert res_dict['success'] is True - result = res_dict['result'] - assert 'total' not in result - class TestDatastoreFullTextSearch(DatastoreLegacyTestBase): @classmethod From eeaafd52c985c18e2c8a66845625ec26ad3dc481 Mon Sep 17 00:00:00 2001 From: Konstantin Sivakov Date: Tue, 11 Sep 2018 09:45:16 +0200 Subject: [PATCH 05/11] Use blueprints instead of controllers --- ckan/templates/package/base.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ckan/templates/package/base.html b/ckan/templates/package/base.html index 8ea75b53711..aee47d83551 100644 --- a/ckan/templates/package/base.html +++ b/ckan/templates/package/base.html @@ -12,8 +12,8 @@ {% if pkg.organization %} {% set organization = h.get_translated(pkg.organization, 'title') or pkg.organization.name %} {% set group_type = pkg.organization.type %} -
  • {% link_for _('Organizations'), controller='organization', action='index' %}
    • -
  • {% link_for organization|truncate(30), controller='organization', action='read', id=pkg.organization.name %}
    • +
  • {% link_for _('Organizations'), named_route='organization.index' %}
    • +
  • {% link_for organization|truncate(30), named_route='organization.read', id=pkg.organization.name %}
    • {% else %}
  • {% link_for _('Datasets'), named_route='dataset.search' %}
    • {% endif %} From 1b7b95fd5b79fde97df772124681e2e81347f686 Mon Sep 17 00:00:00 2001 From: Stefan Oderbolz Date: Thu, 13 Sep 2018 13:19:50 +0200 Subject: [PATCH 06/11] Test for dataset navigation --- ckan/tests/lib/test_helpers.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/ckan/tests/lib/test_helpers.py b/ckan/tests/lib/test_helpers.py index 217fc689870..48c94c8fd52 100644 --- a/ckan/tests/lib/test_helpers.py +++ b/ckan/tests/lib/test_helpers.py @@ -563,6 +563,21 @@ def test_legacy_pylon_routes(self): '
  • Groups
    • ' '
  • About
    • ')) + def test_dataset_navigation(self): + dataset_name = 'test-dataset' + eq_( + h.build_nav_icon('dataset_read', 'Dataset', id=dataset_name), + '
  • Datasets
    • ' + ) + eq_( + h.build_nav_icon('dataset_groups', 'Groups', id=dataset_name), + '
  • Groups
    • ' + ) + eq_( + h.build_nav_icon('dataset_activity', 'Activity Stream', id=dataset_name), + '
  • Activity Stream
    • ' + ) + class TestHelperException(helpers.FunctionalTestBase): From fcbea847b371588f970e9970704098d92cd110c5 Mon Sep 17 00:00:00 2001 From: Stefan Oderbolz Date: Thu, 13 Sep 2018 14:07:34 +0200 Subject: [PATCH 07/11] Tests for group and organization navigation --- ckan/tests/lib/test_helpers.py | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/ckan/tests/lib/test_helpers.py b/ckan/tests/lib/test_helpers.py index 48c94c8fd52..6f9966237cf 100644 --- a/ckan/tests/lib/test_helpers.py +++ b/ckan/tests/lib/test_helpers.py @@ -563,10 +563,10 @@ def test_legacy_pylon_routes(self): '
  • Groups
    • ' '
  • About
    • ')) - def test_dataset_navigation(self): + def test_dataset_navigation_legacy_routes(self): dataset_name = 'test-dataset' eq_( - h.build_nav_icon('dataset_read', 'Dataset', id=dataset_name), + h.build_nav_icon('dataset_read', 'Datasets', id=dataset_name), '
  • Datasets
    • ' ) eq_( @@ -578,6 +578,36 @@ def test_dataset_navigation(self): '
  • Activity Stream
    • ' ) + def test_group_navigation_legacy_routes(self): + group_name = 'test-group' + eq_( + h.build_nav_icon('group_read', 'Datasets', id=group_name), + '
  • Datasets
    • ' + ) + eq_( + h.build_nav_icon('group_activity', 'Activity Stream', id=group_name), + '
  • Activity Stream
    • ' + ) + eq_( + h.build_nav_icon('group_about', 'About', id=group_name), + '
  • About
    • ' + ) + + def test_organization_navigation_legacy_routes(self): + org_name = 'test-org' + eq_( + h.build_nav_icon('organization_read', 'Datasets', id=org_name), + '
  • Datasets
    • ' + ) + eq_( + h.build_nav_icon('organization_activity', 'Activity Stream', id=org_name), + '
  • Activity Stream
    • ' + ) + eq_( + h.build_nav_icon('organization_about', 'About', id=org_name), + '
  • About
    • ' + ) + class TestHelperException(helpers.FunctionalTestBase): From 844988359dd6c03147075f7e28326d80802d1b60 Mon Sep 17 00:00:00 2001 From: Stefan Oderbolz Date: Thu, 13 Sep 2018 14:12:57 +0200 Subject: [PATCH 08/11] Add missing routes to LEGACY_ROUTE_NAMES --- ckan/lib/helpers.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/ckan/lib/helpers.py b/ckan/lib/helpers.py index 6152fde36a0..b5714e5dec9 100644 --- a/ckan/lib/helpers.py +++ b/ckan/lib/helpers.py @@ -69,8 +69,17 @@ 'home': 'home.index', 'about': 'home.about', 'search': 'dataset.search', + 'dataset_read': 'dataset.read', + 'dataset_activity': 'dataset.activity', + 'dataset_groups': 'dataset.groups', 'group_index': 'group.index', - 'organizations_index': 'organization.index' + 'group_about': 'group.about', + 'group_read': 'group.read', + 'group_activity': 'group.activity', + 'organizations_index': 'organization.index', + 'organization_activity': 'organization.activity', + 'organization_read': 'organization.read', + 'organization_about': 'organization.about', } From d565d8143dcb68993bb472e0fa3ba9530469cd86 Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 14 Sep 2018 10:30:36 +0100 Subject: [PATCH 09/11] We dont need the total when just getting the field names --- ckan/lib/helpers.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ckan/lib/helpers.py b/ckan/lib/helpers.py index 6152fde36a0..9a0cd6d9f80 100644 --- a/ckan/lib/helpers.py +++ b/ckan/lib/helpers.py @@ -2280,7 +2280,8 @@ def resource_view_get_fields(resource): data = { 'resource_id': resource['id'], - 'limit': 0 + 'limit': 0, + 'include_total': False, } result = logic.get_action('datastore_search')({}, data) From d446ad83ec12767677d6205588bdc9b5aaf060f8 Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 14 Sep 2018 11:00:04 +0100 Subject: [PATCH 10/11] Changelog and docstring --- CHANGELOG.rst | 10 ++++++++++ ckan/lib/navl/validators.py | 2 ++ 2 files changed, 12 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 0c7a4dc2399..cb2e14b0e59 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -10,6 +10,16 @@ Changelog v.2.9.0 TBA ================== +Minor changes: + + * For navl schemas, the 'default' validator no longer applies the default when + the value is False, (), [] or {} (#4448) + +Bugfixes: + + * Action function "datastore_search" would calculate the total, even if you + set include_total=False (#4448) + Deprecations: * ``c.action`` and ``c.controller`` variables should be avoided. ``ckan.plugins.toolkit.get_endpoint`` can be used instead. This function diff --git a/ckan/lib/navl/validators.py b/ckan/lib/navl/validators.py index d911d5f7047..08a9bc3bccf 100644 --- a/ckan/lib/navl/validators.py +++ b/ckan/lib/navl/validators.py @@ -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.''' def callable(key, data, errors, context): From 435e33a0f648a7a10bb3ffe26ad379ba13f08673 Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 14 Sep 2018 20:01:16 +0100 Subject: [PATCH 11/11] Doc improvements --- CHANGELOG.rst | 2 +- ckan/lib/navl/validators.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index cb2e14b0e59..34b50544daa 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,7 +13,7 @@ v.2.9.0 TBA Minor changes: * For navl schemas, the 'default' validator no longer applies the default when - the value is False, (), [] or {} (#4448) + the value is False, 0, [] or {} (#4448) Bugfixes: diff --git a/ckan/lib/navl/validators.py b/ckan/lib/navl/validators.py index 08a9bc3bccf..172da15316d 100644 --- a/ckan/lib/navl/validators.py +++ b/ckan/lib/navl/validators.py @@ -74,8 +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.''' + '''When key is missing or value is an empty string or None, replace it with + a default value''' def callable(key, data, errors, context):