From 3f090100397f2e245e93095a3034c8a929f549ef Mon Sep 17 00:00:00 2001 From: Stefan Oderbolz Date: Wed, 11 Nov 2015 04:49:25 +0100 Subject: [PATCH 01/13] Add harvest_source_patch to API --- ckanext/harvest/logic/action/patch.py | 58 +++++++++++++++++++++++++++ ckanext/harvest/logic/auth/patch.py | 3 ++ ckanext/harvest/plugin.py | 2 +- 3 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 ckanext/harvest/logic/action/patch.py create mode 100644 ckanext/harvest/logic/auth/patch.py diff --git a/ckanext/harvest/logic/action/patch.py b/ckanext/harvest/logic/action/patch.py new file mode 100644 index 0000000..9e6d79b --- /dev/null +++ b/ckanext/harvest/logic/action/patch.py @@ -0,0 +1,58 @@ +'''API functions for partial updates of existing data in CKAN''' + +import logging +from ckan.logic import get_action +from ckanext.harvest.plugin import DATASET_TYPE_NAME + +log = logging.getLogger(__name__) + + +def harvest_source_patch(context, data_dict): + ''' + Patch an existing harvest source + + This method just proxies the request to package_patch, + which will update a harvest_source dataset type and the + HarvestSource object. All auth checks and validation will + be done there. We only make sure to set the dataset type. + + Note that the harvest source type (ckan, waf, csw, etc) + is now set via the source_type field. + + All fields that are not provided, will be stay as they were before. + + :param id: the name or id of the harvest source to update + :type id: string + :param url: the URL for the harvest source + :type url: string + :param name: the name of the new harvest source, must be between 2 and 100 + characters long and contain only lowercase alphanumeric characters + :type name: string + :param title: the title of the dataset (optional, default: same as + ``name``) + :type title: string + :param notes: a description of the harvest source (optional) + :type notes: string + :param source_type: the harvester type for this source. This must be one + of the registerd harvesters, eg 'ckan', 'csw', etc. + :type source_type: string + :param frequency: the frequency in wich this harvester should run. See + ``ckanext.harvest.model`` source for possible values. Default is + 'MANUAL' + :type frequency: string + :param config: extra configuration options for the particular harvester + type. Should be a serialized as JSON. (optional) + :type config: string + + :returns: the updated harvest source + :rtype: dictionary + ''' + log.info('Patch harvest source: %r', data_dict) + + data_dict['type'] = DATASET_TYPE_NAME + + context['extras_as_string'] = True + source = get_action('package_patch')(context, data_dict) + + return source + diff --git a/ckanext/harvest/logic/auth/patch.py b/ckanext/harvest/logic/auth/patch.py new file mode 100644 index 0000000..17e297f --- /dev/null +++ b/ckanext/harvest/logic/auth/patch.py @@ -0,0 +1,3 @@ +import ckanext.harvest.logic.auth.update as _update + +harvest_source_patch = _update.harvest_source_update diff --git a/ckanext/harvest/plugin.py b/ckanext/harvest/plugin.py index 30bd814..890a6ce 100644 --- a/ckanext/harvest/plugin.py +++ b/ckanext/harvest/plugin.py @@ -287,7 +287,7 @@ def _add_extra(data_dict, key, value): def _get_logic_functions(module_root, logic_functions = {}): - for module_name in ['get', 'create', 'update','delete']: + for module_name in ['get', 'create', 'update', 'patch', 'delete']: module_path = '%s.%s' % (module_root, module_name,) module = __import__(module_path) From 359da2eb690d80361bc1d03bc2136e325ce2eb30 Mon Sep 17 00:00:00 2001 From: Stefan Oderbolz Date: Wed, 11 Nov 2015 04:59:22 +0100 Subject: [PATCH 02/13] Add test class for harvest_source_patch --- ckanext/harvest/tests/test_action.py | 52 ++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/ckanext/harvest/tests/test_action.py b/ckanext/harvest/tests/test_action.py index 24127b0..0e9b894 100644 --- a/ckanext/harvest/tests/test_action.py +++ b/ckanext/harvest/tests/test_action.py @@ -307,6 +307,58 @@ class TestHarvestSourceActionUpdate(HarvestSourceActionBase): assert source.type == source_dict['source_type'] +class TestHarvestSourceActionPatch(HarvestSourceActionBase): + + @classmethod + def setup_class(cls): + + cls.action = 'harvest_source_patch' + + super(TestHarvestSourceActionPatch, cls).setup_class() + + # Create a source to udpate + source_dict = cls.default_source_dict + result = call_action_api('harvest_source_create', + apikey=cls.sysadmin['apikey'], **source_dict) + + cls.default_source_dict['id'] = result['id'] + + def test_invalid_missing_values(self): + # patch should *NOT* return an error about missing values + + # source_dict = {} + # source_dict['id'] = self.default_source_dict['id'] + + # result = call_action_api(self.action, + # apikey=self.sysadmin['apikey'], **source_dict) + + # for key in ('name', 'title', 'url', 'source_type'): + # assert result[key] == self.default_source_dict[key] + pass + + def test_patch(self): + patch_dict = { + "id": self.default_source_dict['id'], + "name": "test-source-action-patched", + "url": "http://test.action.patched.com", + "config": json.dumps({"custom_option": ["pat", "ched"]}) + } + + result = call_action_api('harvest_source_patch', + apikey=self.sysadmin['apikey'], **patch_dict) + + source_dict = self.default_source_dict + source_dict.update(patch_dict) + + for key in source_dict.keys(): + assert source_dict[key] == result[key] + + # Check that source was actually updated + source = harvest_model.HarvestSource.get(result['id']) + assert source.url == source_dict['url'] + assert source.type == source_dict['source_type'] + + class TestActions(ActionBase): def test_harvest_source_clear(self): source = factories.HarvestSourceObj(**SOURCE_DICT) From 136fcb87d568e67fdcb3906729192fa31db86506 Mon Sep 17 00:00:00 2001 From: Stefan Oderbolz Date: Wed, 11 Nov 2015 11:07:54 +0100 Subject: [PATCH 03/13] Make sure package_patch has a fallback for package_update on CKAN < 2.3 --- ckanext/harvest/logic/action/patch.py | 6 +++++- ckanext/harvest/tests/test_action.py | 19 +++++++++---------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/ckanext/harvest/logic/action/patch.py b/ckanext/harvest/logic/action/patch.py index 9e6d79b..66988c6 100644 --- a/ckanext/harvest/logic/action/patch.py +++ b/ckanext/harvest/logic/action/patch.py @@ -52,7 +52,11 @@ def harvest_source_patch(context, data_dict): data_dict['type'] = DATASET_TYPE_NAME context['extras_as_string'] = True - source = get_action('package_patch')(context, data_dict) + try: + source = get_action('package_patch')(context, data_dict) + except KeyError: + log.warn('This CKAN instance does not support package_patch, using package_update instead') + source = get_action('package_update')(context, data_dict) return source diff --git a/ckanext/harvest/tests/test_action.py b/ckanext/harvest/tests/test_action.py index 0e9b894..add9c93 100644 --- a/ckanext/harvest/tests/test_action.py +++ b/ckanext/harvest/tests/test_action.py @@ -3,6 +3,7 @@ import copy import factories import unittest from nose.tools import assert_equal, assert_raises +from nose.plugins.skip import SkipTest try: from ckan.tests import factories as ckan_factories @@ -324,19 +325,17 @@ class TestHarvestSourceActionPatch(HarvestSourceActionBase): cls.default_source_dict['id'] = result['id'] def test_invalid_missing_values(self): - # patch should *NOT* return an error about missing values - - # source_dict = {} - # source_dict['id'] = self.default_source_dict['id'] - - # result = call_action_api(self.action, - # apikey=self.sysadmin['apikey'], **source_dict) - - # for key in ('name', 'title', 'url', 'source_type'): - # assert result[key] == self.default_source_dict[key] + # patch should *NOT* return an error about missing values, so skip this test pass def test_patch(self): + # check if current version of CKAN supports package_patch + try: + toolkit.get_action('package_patch') + except KeyError: + # package_patch is not supported + raise SkipTest() + patch_dict = { "id": self.default_source_dict['id'], "name": "test-source-action-patched", From ffca5cc3da9486122e105b82e2daf2f8c2ce0779 Mon Sep 17 00:00:00 2001 From: Stefan Oderbolz Date: Wed, 11 Nov 2015 11:33:03 +0100 Subject: [PATCH 04/13] Use new factory style for fixtures - Remove default_source_dict from tests - Replace setup_class with constructor - Create mixin for harvest source fixture - Replace assert with assert_equal where appropriate - Replace assert with ok_ - Remove dependency to global SOURCE_DICT - General refactoring of tests --- ckanext/harvest/tests/test_action.py | 142 +++++++++++++-------------- 1 file changed, 69 insertions(+), 73 deletions(-) diff --git a/ckanext/harvest/tests/test_action.py b/ckanext/harvest/tests/test_action.py index add9c93..d9b4292 100644 --- a/ckanext/harvest/tests/test_action.py +++ b/ckanext/harvest/tests/test_action.py @@ -2,7 +2,7 @@ import json import copy import factories import unittest -from nose.tools import assert_equal, assert_raises +from nose.tools import assert_equal, assert_raises, ok_ from nose.plugins.skip import SkipTest try: @@ -72,10 +72,10 @@ def call_action_api(action, apikey=None, status=200, **kwargs): status=status) if status in (200,): - assert response.json['success'] is True + ok_(response.json['success'] is True) return response.json['result'] else: - assert response.json['success'] is False + ok_(response.json['success'] is False) return response.json['error'] @@ -165,8 +165,6 @@ class HarvestSourceActionBase(FunctionalTestBaseWithoutClearBetweenTests): cls.sysadmin = ckan_factories.Sysadmin() - cls.default_source_dict = SOURCE_DICT - if not p.plugin_loaded('test_action_harvester'): p.load('test_action_harvester') @@ -176,54 +174,63 @@ class HarvestSourceActionBase(FunctionalTestBaseWithoutClearBetweenTests): p.unload('test_action_harvester') - def test_invalid_missing_values(self): + def _get_source_dict(self): + return { + "url": "http://test.action.com", + "name": "test-source-action", + "title": "Test source action", + "notes": "Test source action desc", + "source_type": "test-for-action", + "frequency": "MANUAL", + "config": json.dumps({"custom_option": ["a", "b"]}) + } + def test_invalid_missing_values(self): source_dict = {} - if 'id' in self.default_source_dict: - source_dict['id'] = self.default_source_dict['id'] + test_data = self._get_source_dict() + if 'id' in test_data: + source_dict['id'] = test_data['id'] result = call_action_api(self.action, apikey=self.sysadmin['apikey'], status=409, **source_dict) for key in ('name', 'title', 'url', 'source_type'): - assert result[key] == [u'Missing value'] + assert_equal(result[key], [u'Missing value']) def test_invalid_unknown_type(self): - - source_dict = copy.deepcopy(self.default_source_dict) + source_dict = self._get_source_dict() source_dict['source_type'] = 'unknown' result = call_action_api(self.action, apikey=self.sysadmin['apikey'], status=409, **source_dict) - assert 'source_type' in result - assert u'Unknown harvester type' in result['source_type'][0] + ok_('source_type' in result) + ok_(u'Unknown harvester type' in result['source_type'][0]) def test_invalid_unknown_frequency(self): wrong_frequency = 'ANNUALLY' - source_dict = copy.deepcopy(self.default_source_dict) + source_dict = self._get_source_dict() source_dict['frequency'] = wrong_frequency result = call_action_api(self.action, apikey=self.sysadmin['apikey'], status=409, **source_dict) - assert 'frequency' in result - assert u'Frequency {0} not recognised'.format(wrong_frequency) in result['frequency'][0] + ok_('frequency' in result) + ok_(u'Frequency {0} not recognised'.format(wrong_frequency) in result['frequency'][0]) def test_invalid_wrong_configuration(self): - - source_dict = copy.deepcopy(self.default_source_dict) + source_dict = self._get_source_dict() source_dict['config'] = 'not_json' result = call_action_api(self.action, apikey=self.sysadmin['apikey'], status=409, **source_dict) - assert 'config' in result - assert u'Error parsing the configuration options: No JSON object could be decoded' in result['config'][0] + ok_('config' in result) + ok_(u'Error parsing the configuration options: No JSON object could be decoded' in result['config'][0]) source_dict['config'] = json.dumps({'custom_option': 'not_a_list'}) @@ -231,8 +238,8 @@ class HarvestSourceActionBase(FunctionalTestBaseWithoutClearBetweenTests): apikey=self.sysadmin['apikey'], status=409, **source_dict) - assert 'config' in result - assert u'Error parsing the configuration options: custom_option must be a list' in result['config'][0] + ok_('config' in result) + ok_(u'Error parsing the configuration options: custom_option must be a list' in result['config'][0]) class TestHarvestSourceActionCreate(HarvestSourceActionBase): @@ -242,50 +249,52 @@ class TestHarvestSourceActionCreate(HarvestSourceActionBase): def test_create(self): - source_dict = self.default_source_dict + source_dict = self._get_source_dict() result = call_action_api('harvest_source_create', apikey=self.sysadmin['apikey'], **source_dict) for key in source_dict.keys(): - assert source_dict[key] == result[key] + assert_equal(source_dict[key], result[key]) # Check that source was actually created source = harvest_model.HarvestSource.get(result['id']) - assert source.url == source_dict['url'] - assert source.type == source_dict['source_type'] + assert_equal(source.url, source_dict['url']) + assert_equal(source.type, source_dict['source_type']) # Trying to create a source with the same URL fails - source_dict = copy.deepcopy(self.default_source_dict) + source_dict = self._get_source_dict() source_dict['name'] = 'test-source-action-new' result = call_action_api('harvest_source_create', apikey=self.sysadmin['apikey'], status=409, **source_dict) - assert 'url' in result - assert u'There already is a Harvest Source for this URL' in result['url'][0] + ok_('url' in result) + ok_(u'There already is a Harvest Source for this URL' in result['url'][0]) +class HarvestSourceFixtureMixin(object): + def _get_source_dict(self): + template = { + "url": "http://test.action.com", + "name": "test-source-action", + "title": "Test source action", + "notes": "Test source action desc", + "source_type": "test-for-action", + "frequency": "MANUAL", + "config": json.dumps({"custom_option": ["a", "b"]}) + } + result = call_action_api('harvest_source_create', + apikey=self.sysadmin['apikey'], **template) + template['id'] = result['id'] + return template -class TestHarvestSourceActionUpdate(HarvestSourceActionBase): - - @classmethod - def setup_class(cls): - - cls.action = 'harvest_source_update' - - super(TestHarvestSourceActionUpdate, cls).setup_class() - - # Create a source to udpate - source_dict = cls.default_source_dict - result = call_action_api('harvest_source_create', - apikey=cls.sysadmin['apikey'], **source_dict) - - cls.default_source_dict['id'] = result['id'] +class TestHarvestSourceActionUpdate(HarvestSourceFixtureMixin, HarvestSourceActionBase): + def __init__(self): + self.action = 'harvest_source_update' def test_update(self): - - source_dict = self.default_source_dict + source_dict = self._get_source_dict() source_dict.update({ "url": "http://test.action.updated.com", "name": "test-source-action-updated", @@ -300,32 +309,19 @@ class TestHarvestSourceActionUpdate(HarvestSourceActionBase): apikey=self.sysadmin['apikey'], **source_dict) for key in source_dict.keys(): - assert source_dict[key] == result[key] + assert_equal(source_dict[key], result[key], "Key: %s" % key) # Check that source was actually updated source = harvest_model.HarvestSource.get(result['id']) - assert source.url == source_dict['url'] - assert source.type == source_dict['source_type'] + assert_equal(source.url, source_dict['url']) + assert_equal(source.type, source_dict['source_type']) -class TestHarvestSourceActionPatch(HarvestSourceActionBase): - - @classmethod - def setup_class(cls): - - cls.action = 'harvest_source_patch' - - super(TestHarvestSourceActionPatch, cls).setup_class() - - # Create a source to udpate - source_dict = cls.default_source_dict - result = call_action_api('harvest_source_create', - apikey=cls.sysadmin['apikey'], **source_dict) - - cls.default_source_dict['id'] = result['id'] +class TestHarvestSourceActionPatch(HarvestSourceFixtureMixin, HarvestSourceActionBase): + def __init__(self): + self.action = 'harvest_source_patch' def test_invalid_missing_values(self): - # patch should *NOT* return an error about missing values, so skip this test pass def test_patch(self): @@ -336,8 +332,10 @@ class TestHarvestSourceActionPatch(HarvestSourceActionBase): # package_patch is not supported raise SkipTest() + source_dict = self._get_source_dict() + patch_dict = { - "id": self.default_source_dict['id'], + "id": source_dict['id'], "name": "test-source-action-patched", "url": "http://test.action.patched.com", "config": json.dumps({"custom_option": ["pat", "ched"]}) @@ -346,16 +344,14 @@ class TestHarvestSourceActionPatch(HarvestSourceActionBase): result = call_action_api('harvest_source_patch', apikey=self.sysadmin['apikey'], **patch_dict) - source_dict = self.default_source_dict source_dict.update(patch_dict) - for key in source_dict.keys(): - assert source_dict[key] == result[key] + assert_equal(source_dict[key], result[key], "Key: %s" % key) # Check that source was actually updated source = harvest_model.HarvestSource.get(result['id']) - assert source.url == source_dict['url'] - assert source.type == source_dict['source_type'] + assert_equal(source.url, source_dict['url']) + assert_equal(source.type, source_dict['source_type']) class TestActions(ActionBase): @@ -373,7 +369,7 @@ class TestActions(ActionBase): assert_equal(result, {'id': source.id}) source = harvest_model.HarvestSource.get(source.id) - assert source + ok_(source) assert_equal(harvest_model.HarvestJob.get(job.id), None) assert_equal(harvest_model.HarvestObject.get(object_.id), None) assert_equal(model.Package.get(dataset['id']), None) @@ -446,7 +442,7 @@ class TestHarvestObject(unittest.TestCase): # fetch the object from database to check it was created created_object = harvest_model.HarvestObject.get(harvest_object['id']) - assert created_object.guid == harvest_object['guid'] == data_dict['guid'] + ok_(created_object.guid == harvest_object['guid'] == data_dict['guid']) def test_create_bad_parameters(self): source_a = factories.HarvestSourceObj() From 644fa49dd4444cff670a2f0470999b4381ff6103 Mon Sep 17 00:00:00 2001 From: Stefan Oderbolz Date: Wed, 11 Nov 2015 19:22:58 +0100 Subject: [PATCH 05/13] Make tests independent from cls.sysadmin Generate unique harvest sources --- ckanext/harvest/tests/test_action.py | 36 +++++++++++++++++----------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/ckanext/harvest/tests/test_action.py b/ckanext/harvest/tests/test_action.py index d9b4292..3bb68d5 100644 --- a/ckanext/harvest/tests/test_action.py +++ b/ckanext/harvest/tests/test_action.py @@ -1,5 +1,6 @@ import json import copy +import uuid import factories import unittest from nose.tools import assert_equal, assert_raises, ok_ @@ -163,8 +164,6 @@ class HarvestSourceActionBase(FunctionalTestBaseWithoutClearBetweenTests): super(HarvestSourceActionBase, cls).setup_class() harvest_model.setup() - cls.sysadmin = ckan_factories.Sysadmin() - if not p.plugin_loaded('test_action_harvester'): p.load('test_action_harvester') @@ -191,8 +190,9 @@ class HarvestSourceActionBase(FunctionalTestBaseWithoutClearBetweenTests): if 'id' in test_data: source_dict['id'] = test_data['id'] + sysadmin = ckan_factories.Sysadmin() result = call_action_api(self.action, - apikey=self.sysadmin['apikey'], status=409, + apikey=sysadmin['apikey'], status=409, **source_dict) for key in ('name', 'title', 'url', 'source_type'): @@ -202,8 +202,9 @@ class HarvestSourceActionBase(FunctionalTestBaseWithoutClearBetweenTests): source_dict = self._get_source_dict() source_dict['source_type'] = 'unknown' + sysadmin = ckan_factories.Sysadmin() result = call_action_api(self.action, - apikey=self.sysadmin['apikey'], status=409, + apikey=sysadmin['apikey'], status=409, **source_dict) ok_('source_type' in result) @@ -214,8 +215,9 @@ class HarvestSourceActionBase(FunctionalTestBaseWithoutClearBetweenTests): source_dict = self._get_source_dict() source_dict['frequency'] = wrong_frequency + sysadmin = ckan_factories.Sysadmin() result = call_action_api(self.action, - apikey=self.sysadmin['apikey'], status=409, + apikey=sysadmin['apikey'], status=409, **source_dict) ok_('frequency' in result) @@ -225,8 +227,9 @@ class HarvestSourceActionBase(FunctionalTestBaseWithoutClearBetweenTests): source_dict = self._get_source_dict() source_dict['config'] = 'not_json' + sysadmin = ckan_factories.Sysadmin() result = call_action_api(self.action, - apikey=self.sysadmin['apikey'], status=409, + apikey=sysadmin['apikey'], status=409, **source_dict) ok_('config' in result) @@ -235,7 +238,7 @@ class HarvestSourceActionBase(FunctionalTestBaseWithoutClearBetweenTests): source_dict['config'] = json.dumps({'custom_option': 'not_a_list'}) result = call_action_api(self.action, - apikey=self.sysadmin['apikey'], status=409, + apikey=sysadmin['apikey'], status=409, **source_dict) ok_('config' in result) @@ -251,8 +254,9 @@ class TestHarvestSourceActionCreate(HarvestSourceActionBase): source_dict = self._get_source_dict() + sysadmin = ckan_factories.Sysadmin() result = call_action_api('harvest_source_create', - apikey=self.sysadmin['apikey'], **source_dict) + apikey=sysadmin['apikey'], **source_dict) for key in source_dict.keys(): assert_equal(source_dict[key], result[key]) @@ -267,7 +271,7 @@ class TestHarvestSourceActionCreate(HarvestSourceActionBase): source_dict['name'] = 'test-source-action-new' result = call_action_api('harvest_source_create', - apikey=self.sysadmin['apikey'], status=409, + apikey=sysadmin['apikey'], status=409, **source_dict) ok_('url' in result) @@ -275,17 +279,19 @@ class TestHarvestSourceActionCreate(HarvestSourceActionBase): class HarvestSourceFixtureMixin(object): def _get_source_dict(self): + id = uuid.uuid4() template = { - "url": "http://test.action.com", - "name": "test-source-action", + "url": "http://test.action.com/%s" % id, + "name": "test-source-action%s" % id, "title": "Test source action", "notes": "Test source action desc", "source_type": "test-for-action", "frequency": "MANUAL", "config": json.dumps({"custom_option": ["a", "b"]}) } + sysadmin = ckan_factories.Sysadmin() result = call_action_api('harvest_source_create', - apikey=self.sysadmin['apikey'], **template) + apikey=sysadmin['apikey'], **template) template['id'] = result['id'] return template @@ -305,8 +311,9 @@ class TestHarvestSourceActionUpdate(HarvestSourceFixtureMixin, HarvestSourceActi "config": json.dumps({"custom_option": ["c", "d"]}) }) + sysadmin = ckan_factories.Sysadmin() result = call_action_api('harvest_source_update', - apikey=self.sysadmin['apikey'], **source_dict) + apikey=sysadmin['apikey'], **source_dict) for key in source_dict.keys(): assert_equal(source_dict[key], result[key], "Key: %s" % key) @@ -341,8 +348,9 @@ class TestHarvestSourceActionPatch(HarvestSourceFixtureMixin, HarvestSourceActio "config": json.dumps({"custom_option": ["pat", "ched"]}) } + sysadmin = ckan_factories.Sysadmin() result = call_action_api('harvest_source_patch', - apikey=self.sysadmin['apikey'], **patch_dict) + apikey=sysadmin['apikey'], **patch_dict) source_dict.update(patch_dict) for key in source_dict.keys(): From 73aecb7f126109efd7f741279c899c17a30f1a7a Mon Sep 17 00:00:00 2001 From: Stefan Oderbolz Date: Wed, 11 Nov 2015 19:59:25 +0100 Subject: [PATCH 06/13] Add datastore_default user due to PR #2234 from ckan --- bin/travis-build.bash | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bin/travis-build.bash b/bin/travis-build.bash index aae36f3..09e97e5 100644 --- a/bin/travis-build.bash +++ b/bin/travis-build.bash @@ -33,7 +33,9 @@ sudo service jetty restart echo "Creating the PostgreSQL user and database..." sudo -u postgres psql -c "CREATE USER ckan_default WITH PASSWORD 'pass';" +sudo -u postgres psql -c "CREATE USER datastore_default WITH PASSWORD 'pass';" sudo -u postgres psql -c 'CREATE DATABASE ckan_test WITH OWNER ckan_default;' +sudo -u postgres psql -c 'CREATE DATABASE datastore_test WITH OWNER ckan_default;' echo "Initialising the database..." cd ckan From c33c6e8c133cdc0a08acc5c7c04b9cf963a73dd4 Mon Sep 17 00:00:00 2001 From: Stefan Oderbolz Date: Thu, 12 Nov 2015 11:38:11 +0100 Subject: [PATCH 07/13] Raise an error instead of falling back to harvest_source_update As the behaviour of *_patch is clearly different from the *_update we should raise an error if this action is called on a CKAN instance, where the action is not available. --- ckanext/harvest/logic/action/patch.py | 6 +++--- ckanext/harvest/tests/test_action.py | 18 ++++++++---------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/ckanext/harvest/logic/action/patch.py b/ckanext/harvest/logic/action/patch.py index 66988c6..24f0fc5 100644 --- a/ckanext/harvest/logic/action/patch.py +++ b/ckanext/harvest/logic/action/patch.py @@ -1,7 +1,7 @@ '''API functions for partial updates of existing data in CKAN''' import logging -from ckan.logic import get_action +from ckan.logic import ActionError, get_action from ckanext.harvest.plugin import DATASET_TYPE_NAME log = logging.getLogger(__name__) @@ -55,8 +55,8 @@ def harvest_source_patch(context, data_dict): try: source = get_action('package_patch')(context, data_dict) except KeyError: - log.warn('This CKAN instance does not support package_patch, using package_update instead') - source = get_action('package_update')(context, data_dict) + log.warn('This CKAN instance does not support package_patch, consider upgrading to CKAN >= 2.3 if needed.') + raise ActionError('The harvest_source_patch action is not available on this version of CKAN') return source diff --git a/ckanext/harvest/tests/test_action.py b/ckanext/harvest/tests/test_action.py index 3bb68d5..967fedc 100644 --- a/ckanext/harvest/tests/test_action.py +++ b/ckanext/harvest/tests/test_action.py @@ -68,9 +68,14 @@ def call_action_api(action, apikey=None, status=200, **kwargs): ''' params = json.dumps(kwargs) app = _get_test_app() - response = app.post('/api/action/{0}'.format(action), params=params, - extra_environ={'Authorization': str(apikey)}, - status=status) + try: + response = app.post('/api/action/{0}'.format(action), params=params, + extra_environ={'Authorization': str(apikey)}, + status=status) + except ActionError, e: + if str(e) == 'The harvest_source_patch action is not available on this version of CKAN': + raise SkipTest() + raise e if status in (200,): ok_(response.json['success'] is True) @@ -332,13 +337,6 @@ class TestHarvestSourceActionPatch(HarvestSourceFixtureMixin, HarvestSourceActio pass def test_patch(self): - # check if current version of CKAN supports package_patch - try: - toolkit.get_action('package_patch') - except KeyError: - # package_patch is not supported - raise SkipTest() - source_dict = self._get_source_dict() patch_dict = { From 0ce3748153b3c72739bd51bc17b5a6c679090f08 Mon Sep 17 00:00:00 2001 From: Stefan Oderbolz Date: Fri, 13 Nov 2015 12:01:19 +0100 Subject: [PATCH 08/13] Do not use ActionError as this does not yet exist in CKAN 2.2 --- ckanext/harvest/logic/action/patch.py | 4 ++-- ckanext/harvest/tests/test_action.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ckanext/harvest/logic/action/patch.py b/ckanext/harvest/logic/action/patch.py index 24f0fc5..30b87e3 100644 --- a/ckanext/harvest/logic/action/patch.py +++ b/ckanext/harvest/logic/action/patch.py @@ -1,7 +1,7 @@ '''API functions for partial updates of existing data in CKAN''' import logging -from ckan.logic import ActionError, get_action +from ckan.logic import get_action from ckanext.harvest.plugin import DATASET_TYPE_NAME log = logging.getLogger(__name__) @@ -56,7 +56,7 @@ def harvest_source_patch(context, data_dict): source = get_action('package_patch')(context, data_dict) except KeyError: log.warn('This CKAN instance does not support package_patch, consider upgrading to CKAN >= 2.3 if needed.') - raise ActionError('The harvest_source_patch action is not available on this version of CKAN') + raise Exception('The harvest_source_patch action is not available on this version of CKAN') return source diff --git a/ckanext/harvest/tests/test_action.py b/ckanext/harvest/tests/test_action.py index 967fedc..c1ebc18 100644 --- a/ckanext/harvest/tests/test_action.py +++ b/ckanext/harvest/tests/test_action.py @@ -72,7 +72,7 @@ def call_action_api(action, apikey=None, status=200, **kwargs): response = app.post('/api/action/{0}'.format(action), params=params, extra_environ={'Authorization': str(apikey)}, status=status) - except ActionError, e: + except Exception, e: if str(e) == 'The harvest_source_patch action is not available on this version of CKAN': raise SkipTest() raise e From 1288a4d9e7781e776a5b7510a7ea16022edf6551 Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 13 Nov 2015 12:32:13 +0000 Subject: [PATCH 09/13] Reflow text to 79 char width. Warning not necessary with an exception I think. --- ckanext/harvest/logic/action/patch.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/ckanext/harvest/logic/action/patch.py b/ckanext/harvest/logic/action/patch.py index 30b87e3..cd21d97 100644 --- a/ckanext/harvest/logic/action/patch.py +++ b/ckanext/harvest/logic/action/patch.py @@ -11,13 +11,13 @@ def harvest_source_patch(context, data_dict): ''' Patch an existing harvest source - This method just proxies the request to package_patch, - which will update a harvest_source dataset type and the - HarvestSource object. All auth checks and validation will - be done there. We only make sure to set the dataset type. + This method just proxies the request to package_patch, which will update a + harvest_source dataset type and the HarvestSource object. All auth checks + and validation will be done there. We only make sure to set the dataset + type. - Note that the harvest source type (ckan, waf, csw, etc) - is now set via the source_type field. + Note that the harvest source type (ckan, waf, csw, etc) is now set via the + source_type field. All fields that are not provided, will be stay as they were before. @@ -55,8 +55,7 @@ def harvest_source_patch(context, data_dict): try: source = get_action('package_patch')(context, data_dict) except KeyError: - log.warn('This CKAN instance does not support package_patch, consider upgrading to CKAN >= 2.3 if needed.') - raise Exception('The harvest_source_patch action is not available on this version of CKAN') + raise Exception('The harvest_source_patch action is not available on ' + 'this version of CKAN') return source - From 01a4bfd31438e59325b84874dc69c989cb597c30 Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 13 Nov 2015 12:33:18 +0000 Subject: [PATCH 10/13] Patch test should skip if ckan version is wrong, rather than ignore all exceptions from posts of all tests. Remove FunctionalTestBaseWithoutClearBetweenTests now the tests are modernized. --- ckanext/harvest/tests/test_action.py | 42 ++++++++-------------------- 1 file changed, 11 insertions(+), 31 deletions(-) diff --git a/ckanext/harvest/tests/test_action.py b/ckanext/harvest/tests/test_action.py index c1ebc18..e8a8e3e 100644 --- a/ckanext/harvest/tests/test_action.py +++ b/ckanext/harvest/tests/test_action.py @@ -1,5 +1,4 @@ import json -import copy import uuid import factories import unittest @@ -8,14 +7,14 @@ from nose.plugins.skip import SkipTest try: from ckan.tests import factories as ckan_factories - from ckan.tests.helpers import _get_test_app, reset_db + from ckan.tests.helpers import _get_test_app, reset_db, FunctionalTestBase except ImportError: from ckan.new_tests import factories as ckan_factories - from ckan.new_tests.helpers import _get_test_app, reset_db + from ckan.new_tests.helpers import (_get_test_app, reset_db, + FunctionalTestBase) from ckan import plugins as p from ckan.plugins import toolkit from ckan import model -import ckan.lib.search as search from ckanext.harvest.interfaces import IHarvester import ckanext.harvest.model as harvest_model @@ -68,14 +67,9 @@ def call_action_api(action, apikey=None, status=200, **kwargs): ''' params = json.dumps(kwargs) app = _get_test_app() - try: - response = app.post('/api/action/{0}'.format(action), params=params, - extra_environ={'Authorization': str(apikey)}, - status=status) - except Exception, e: - if str(e) == 'The harvest_source_patch action is not available on this version of CKAN': - raise SkipTest() - raise e + response = app.post('/api/action/{0}'.format(action), params=params, + extra_environ={'Authorization': str(apikey)}, + status=status) if status in (200,): ok_(response.json['success'] is True) @@ -119,23 +113,6 @@ class MockHarvesterForActionTests(p.SingletonPlugin): return True -class FunctionalTestBaseWithoutClearBetweenTests(object): - ''' Functional tests should normally derive from - ckan.lib.helpers.FunctionalTestBase, but these are legacy tests so this - class is a compromise. This version doesn't call reset_db before every - test, because these tests are designed with fixtures created in - setup_class.''' - - @classmethod - def setup_class(cls): - reset_db() - harvest_model.setup() - - @classmethod - def teardown_class(cls): - pass - - SOURCE_DICT = { "url": "http://test.action.com", "name": "test-source-action", @@ -162,7 +139,7 @@ class ActionBase(object): p.unload('test_action_harvester') -class HarvestSourceActionBase(FunctionalTestBaseWithoutClearBetweenTests): +class HarvestSourceActionBase(FunctionalTestBase): @classmethod def setup_class(cls): @@ -295,7 +272,7 @@ class HarvestSourceFixtureMixin(object): "config": json.dumps({"custom_option": ["a", "b"]}) } sysadmin = ckan_factories.Sysadmin() - result = call_action_api('harvest_source_create', + result = call_action_api('harvest_source_create', apikey=sysadmin['apikey'], **template) template['id'] = result['id'] return template @@ -337,6 +314,9 @@ class TestHarvestSourceActionPatch(HarvestSourceFixtureMixin, HarvestSourceActio pass def test_patch(self): + if toolkit.check_ckan_version(max_version='2.2.99'): + # harvest_source_patch only came in with ckan 2.3 + raise SkipTest() source_dict = self._get_source_dict() patch_dict = { From b150b50887fdb379feff2cdc3a6179b065b06974 Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 13 Nov 2015 12:44:27 +0000 Subject: [PATCH 11/13] Move the SkipTest to include inherited tests too. --- ckanext/harvest/tests/test_action.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ckanext/harvest/tests/test_action.py b/ckanext/harvest/tests/test_action.py index e8a8e3e..c1b0aa3 100644 --- a/ckanext/harvest/tests/test_action.py +++ b/ckanext/harvest/tests/test_action.py @@ -309,14 +309,14 @@ class TestHarvestSourceActionUpdate(HarvestSourceFixtureMixin, HarvestSourceActi class TestHarvestSourceActionPatch(HarvestSourceFixtureMixin, HarvestSourceActionBase): def __init__(self): self.action = 'harvest_source_patch' + if toolkit.check_ckan_version(max_version='2.2.99'): + # harvest_source_patch only came in with ckan 2.3 + raise SkipTest() def test_invalid_missing_values(self): pass def test_patch(self): - if toolkit.check_ckan_version(max_version='2.2.99'): - # harvest_source_patch only came in with ckan 2.3 - raise SkipTest() source_dict = self._get_source_dict() patch_dict = { From 42ab55cb6d13259e6320806e75ac730de6cc9397 Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 13 Nov 2015 13:32:55 +0000 Subject: [PATCH 12/13] No longer need uuid since we clear db between tests now. Added ignore_missing because of occasional failures. --- ckanext/harvest/logic/schema.py | 4 +-- ckanext/harvest/tests/test_action.py | 39 ++++++++++++++-------------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/ckanext/harvest/logic/schema.py b/ckanext/harvest/logic/schema.py index c92c806..c698540 100644 --- a/ckanext/harvest/logic/schema.py +++ b/ckanext/harvest/logic/schema.py @@ -89,8 +89,8 @@ def harvest_source_show_package_schema(): 'organization': [], 'notes': [], 'revision_id': [], - 'revision_timestamp': [], - 'tracking_summary': [], + 'revision_timestamp': [ignore_missing], + 'tracking_summary': [ignore_missing], }) schema['__extras'] = [ignore] diff --git a/ckanext/harvest/tests/test_action.py b/ckanext/harvest/tests/test_action.py index c1b0aa3..de290a9 100644 --- a/ckanext/harvest/tests/test_action.py +++ b/ckanext/harvest/tests/test_action.py @@ -259,25 +259,23 @@ class TestHarvestSourceActionCreate(HarvestSourceActionBase): ok_('url' in result) ok_(u'There already is a Harvest Source for this URL' in result['url'][0]) + class HarvestSourceFixtureMixin(object): def _get_source_dict(self): - id = uuid.uuid4() - template = { - "url": "http://test.action.com/%s" % id, - "name": "test-source-action%s" % id, - "title": "Test source action", - "notes": "Test source action desc", - "source_type": "test-for-action", - "frequency": "MANUAL", - "config": json.dumps({"custom_option": ["a", "b"]}) - } - sysadmin = ckan_factories.Sysadmin() - result = call_action_api('harvest_source_create', - apikey=sysadmin['apikey'], **template) - template['id'] = result['id'] - return template + '''Not only returns a source_dict, but creates the HarvestSource object + as well - suitable for testing update actions. + ''' + source = HarvestSourceActionBase._get_source_dict(self) + source = factories.HarvestSource(**source) + # delete status because it gets in the way of the status supplied to + # call_action_api later on. It is only a generated value, not affecting + # the update/patch anyway. + del source['status'] + return source -class TestHarvestSourceActionUpdate(HarvestSourceFixtureMixin, HarvestSourceActionBase): + +class TestHarvestSourceActionUpdate(HarvestSourceFixtureMixin, + HarvestSourceActionBase): def __init__(self): self.action = 'harvest_source_update' @@ -297,7 +295,8 @@ class TestHarvestSourceActionUpdate(HarvestSourceFixtureMixin, HarvestSourceActi result = call_action_api('harvest_source_update', apikey=sysadmin['apikey'], **source_dict) - for key in source_dict.keys(): + for key in set(('url', 'name', 'title', 'notes', 'source_type', + 'frequency', 'config')): assert_equal(source_dict[key], result[key], "Key: %s" % key) # Check that source was actually updated @@ -306,7 +305,8 @@ class TestHarvestSourceActionUpdate(HarvestSourceFixtureMixin, HarvestSourceActi assert_equal(source.type, source_dict['source_type']) -class TestHarvestSourceActionPatch(HarvestSourceFixtureMixin, HarvestSourceActionBase): +class TestHarvestSourceActionPatch(HarvestSourceFixtureMixin, + HarvestSourceActionBase): def __init__(self): self.action = 'harvest_source_patch' if toolkit.check_ckan_version(max_version='2.2.99'): @@ -331,7 +331,8 @@ class TestHarvestSourceActionPatch(HarvestSourceFixtureMixin, HarvestSourceActio apikey=sysadmin['apikey'], **patch_dict) source_dict.update(patch_dict) - for key in source_dict.keys(): + for key in set(('url', 'name', 'title', 'notes', 'source_type', + 'frequency', 'config')): assert_equal(source_dict[key], result[key], "Key: %s" % key) # Check that source was actually updated From c0a865e64e2ded83007dd6ebee9e99d1929e5356 Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 13 Nov 2015 13:45:56 +0000 Subject: [PATCH 13/13] Revert ok_ - makes it slightly less readable for little benefit. --- ckanext/harvest/logic/action/update.py | 12 +++++------ ckanext/harvest/tests/test_action.py | 30 +++++++++++++------------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/ckanext/harvest/logic/action/update.py b/ckanext/harvest/logic/action/update.py index bd06d2c..18c32af 100644 --- a/ckanext/harvest/logic/action/update.py +++ b/ckanext/harvest/logic/action/update.py @@ -39,13 +39,13 @@ def harvest_source_update(context, data_dict): ''' Updates an existing harvest source - This method just proxies the request to package_update, - which will create a harvest_source dataset type and the - HarvestSource object. All auth checks and validation will - be done there .We only make sure to set the dataset type + This method just proxies the request to package_update, which will create a + harvest_source dataset type and the HarvestSource object. All auth checks + and validation will be done there. We only make sure to set the dataset + type - Note that the harvest source type (ckan, waf, csw, etc) - is now set via the source_type field. + Note that the harvest source type (ckan, waf, csw, etc) is now set via the + source_type field. :param id: the name or id of the harvest source to update :type id: string diff --git a/ckanext/harvest/tests/test_action.py b/ckanext/harvest/tests/test_action.py index de290a9..9cbfc36 100644 --- a/ckanext/harvest/tests/test_action.py +++ b/ckanext/harvest/tests/test_action.py @@ -2,7 +2,7 @@ import json import uuid import factories import unittest -from nose.tools import assert_equal, assert_raises, ok_ +from nose.tools import assert_equal, assert_raises from nose.plugins.skip import SkipTest try: @@ -72,10 +72,10 @@ def call_action_api(action, apikey=None, status=200, **kwargs): status=status) if status in (200,): - ok_(response.json['success'] is True) + assert response.json['success'] is True return response.json['result'] else: - ok_(response.json['success'] is False) + assert response.json['success'] is False return response.json['error'] @@ -189,8 +189,8 @@ class HarvestSourceActionBase(FunctionalTestBase): apikey=sysadmin['apikey'], status=409, **source_dict) - ok_('source_type' in result) - ok_(u'Unknown harvester type' in result['source_type'][0]) + assert 'source_type' in result + assert u'Unknown harvester type' in result['source_type'][0] def test_invalid_unknown_frequency(self): wrong_frequency = 'ANNUALLY' @@ -202,8 +202,8 @@ class HarvestSourceActionBase(FunctionalTestBase): apikey=sysadmin['apikey'], status=409, **source_dict) - ok_('frequency' in result) - ok_(u'Frequency {0} not recognised'.format(wrong_frequency) in result['frequency'][0]) + assert 'frequency' in result + assert u'Frequency {0} not recognised'.format(wrong_frequency) in result['frequency'][0] def test_invalid_wrong_configuration(self): source_dict = self._get_source_dict() @@ -214,8 +214,8 @@ class HarvestSourceActionBase(FunctionalTestBase): apikey=sysadmin['apikey'], status=409, **source_dict) - ok_('config' in result) - ok_(u'Error parsing the configuration options: No JSON object could be decoded' in result['config'][0]) + assert 'config' in result + assert u'Error parsing the configuration options: No JSON object could be decoded' in result['config'][0] source_dict['config'] = json.dumps({'custom_option': 'not_a_list'}) @@ -223,8 +223,8 @@ class HarvestSourceActionBase(FunctionalTestBase): apikey=sysadmin['apikey'], status=409, **source_dict) - ok_('config' in result) - ok_(u'Error parsing the configuration options: custom_option must be a list' in result['config'][0]) + assert 'config' in result + assert u'Error parsing the configuration options: custom_option must be a list' in result['config'][0] class TestHarvestSourceActionCreate(HarvestSourceActionBase): @@ -256,8 +256,8 @@ class TestHarvestSourceActionCreate(HarvestSourceActionBase): apikey=sysadmin['apikey'], status=409, **source_dict) - ok_('url' in result) - ok_(u'There already is a Harvest Source for this URL' in result['url'][0]) + assert 'url' in result + assert u'There already is a Harvest Source for this URL' in result['url'][0] class HarvestSourceFixtureMixin(object): @@ -356,7 +356,7 @@ class TestActions(ActionBase): assert_equal(result, {'id': source.id}) source = harvest_model.HarvestSource.get(source.id) - ok_(source) + assert source assert_equal(harvest_model.HarvestJob.get(job.id), None) assert_equal(harvest_model.HarvestObject.get(object_.id), None) assert_equal(model.Package.get(dataset['id']), None) @@ -429,7 +429,7 @@ class TestHarvestObject(unittest.TestCase): # fetch the object from database to check it was created created_object = harvest_model.HarvestObject.get(harvest_object['id']) - ok_(created_object.guid == harvest_object['guid'] == data_dict['guid']) + assert created_object.guid == harvest_object['guid'] == data_dict['guid'] def test_create_bad_parameters(self): source_a = factories.HarvestSourceObj()