diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a501992..33bc897 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -4,37 +4,41 @@ jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - - uses: actions/setup-python@v2 + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 with: - python-version: '3.8' + python-version: '3.9' - name: Install requirements - run: | - pip install flake8 pycodestyle pylint bandit - pip install -e . + run: pip install flake8 pycodestyle - name: Check syntax run: flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics --exclude ckan - name: Run flake8 run: flake8 . --count --max-line-length=127 --statistics --exclude ckan - - name: Run pylint - run: pylint --output-format=colorized ckanext/downloadall - - name: Run bandit - run: bandit -s B101 -r ./ -f screen test: needs: lint strategy: matrix: - ckan-version: [2.9] + include: + - ckan-version: "2.11" + ckan-image: "ckan/ckan-dev:2.11-py3.10" + solr-version: "9" + - ckan-version: "2.10" + ckan-image: "ckan/ckan-dev:2.10-py3.10" + solr-version: "9" + - ckan-version: "2.9" + ckan-image: "ckan/ckan-dev:2.9-py3.9" + solr-version: "8" fail-fast: false name: CKAN ${{ matrix.ckan-version }} runs-on: ubuntu-latest container: - image: openknowledge/ckan-dev:${{ matrix.ckan-version }} + image: ${{ matrix.ckan-image }} + options: --user root services: solr: - image: ckan/ckan-solr:${{ matrix.ckan-version }} + image: ckan/ckan-solr:${{ matrix.ckan-version }}-solr9 postgres: image: ckan/ckan-postgres-dev:${{ matrix.ckan-version }} env: @@ -59,6 +63,9 @@ jobs: pip install -e . # Replace default path to CKAN core config file with the one on the container sed -i -e 's/use = config:.*/use = config:\/srv\/app\/src\/ckan\/test-core.ini/' test.ini + # Install unzip for SonarQube Scan + apt-get update + apt-get install unzip -y - name: Setup extension run: | ckan -c test.ini db init diff --git a/.gitignore b/.gitignore index 5e83cd1..ce5f5c9 100644 --- a/.gitignore +++ b/.gitignore @@ -42,4 +42,6 @@ coverage.xml docs/_build/ .vscode -idea/ \ No newline at end of file +idea/ + +.DS_Store \ No newline at end of file diff --git a/ckanext/downloadall/plugin.py b/ckanext/downloadall/plugin.py index 5d2b45b..a5970c2 100644 --- a/ckanext/downloadall/plugin.py +++ b/ckanext/downloadall/plugin.py @@ -86,7 +86,7 @@ def get_helpers(self): # IPackageController def before_index(self, pkg_dict): try: - if 'All resource data' in pkg_dict['res_name']: + if 'All resource data' in pkg_dict.get('res_name', ''): # we've got a 'Download all zip', so remove it's ZIP from the # SOLR facet of resource formats, as it's not really a data # resource diff --git a/ckanext/downloadall/public/css/downloadall.css b/ckanext/downloadall/public/css/downloadall.css new file mode 100644 index 0000000..e3c0976 --- /dev/null +++ b/ckanext/downloadall/public/css/downloadall.css @@ -0,0 +1,3 @@ +a.downloadall-disabled { + display: none; +} diff --git a/ckanext/downloadall/tasks.py b/ckanext/downloadall/tasks.py index bbb9ba8..bb0e285 100644 --- a/ckanext/downloadall/tasks.py +++ b/ckanext/downloadall/tasks.py @@ -347,10 +347,10 @@ def remove_resources_that_should_not_be_included_in_the_datapackage(dataset): existing_zip_resource = res continue - if res['format'] in resource_formats_to_ignore: + if res.get('format', '') in resource_formats_to_ignore: log.debug('Resource resource {}/{} skipped - because it is ' 'format {}'.format(i + 1, len(dataset.get('resources', [])), - res['format'])) + res.get('format', ''))) continue resources_to_include.append(res) dataset = dict(dataset, resources=resources_to_include) diff --git a/ckanext/downloadall/templates/package/read.html b/ckanext/downloadall/templates/package/read.html index e000b4e..7796de8 100644 --- a/ckanext/downloadall/templates/package/read.html +++ b/ckanext/downloadall/templates/package/read.html @@ -1,5 +1,10 @@ {% ckan_extends %} +{% block styles %} + {{ super() }} + +{% endblock %} + {% block package_resources %} {% if pkg.resources %} @@ -7,9 +12,9 @@ {% set zip_res = h.downloadall__pop_zip_resource(pkg) %} {% if zip_res %} - + {% else %} - + {% endif %} {{ _('Download all') }} diff --git a/ckanext/downloadall/tests/test_action.py b/ckanext/downloadall/tests/test_action.py index b29d3e8..4990bfe 100644 --- a/ckanext/downloadall/tests/test_action.py +++ b/ckanext/downloadall/tests/test_action.py @@ -10,8 +10,7 @@ class TestDatastoreCreate(TestBase): def test_datastore_create(self): dataset = factories.Dataset( - owner_org=self.org['id'], - resources=[{'url': 'http://some.image.png', 'format': 'png'}]) + resources=[{'url': 'https://example.com/data.csv', 'format': 'csv'}]) helpers.call_action('job_clear') helpers.call_action('datastore_create', diff --git a/ckanext/downloadall/tests/test_plugin.py b/ckanext/downloadall/tests/test_plugin.py index 1c154fc..00fe297 100644 --- a/ckanext/downloadall/tests/test_plugin.py +++ b/ckanext/downloadall/tests/test_plugin.py @@ -6,64 +6,63 @@ class TestNotify(TestBase): def test_new_resource_leads_to_queued_task(self): - dataset = factories.Dataset(owner_org=self.org['id'], resources=[ - {'url': 'http://some.image.png', 'format': 'png'}]) - assert [job['title'] for job in helpers.call_action('job_list')] == [ - 'DownloadAll new "{}" {}'.format(dataset['name'], dataset['id'])] + dataset = factories.Dataset(resources=[ + {'url': 'https://example.com/data.csv', 'format': 'csv'}]) + assert 'DownloadAll new "{}" {}'.format(dataset['name'], dataset['id']) in [ + job['title'] for job in helpers.call_action('job_list')] def test_changed_resource_leads_to_queued_task(self): - dataset = factories.Dataset(owner_org=self.org['id'], resources=[ - {'url': 'http://some.image.png', 'format': 'png'}]) + dataset = factories.Dataset(resources=[ + {'url': 'https://example.com/data.csv', 'format': 'csv'}]) helpers.call_action('job_clear') dataset['resources'][0]['url'] = 'http://another.image.png' helpers.call_action('package_update', **dataset) - assert [job['title'] for job in helpers.call_action('job_list')] == [ - 'DownloadAll changed "{}" {}'.format(dataset['name'], dataset['id'])] + assert 'DownloadAll changed "{}" {}'.format(dataset['name'], dataset['id']) in [ + job['title'] for job in helpers.call_action('job_list')] def test_deleted_resource_leads_to_queued_task(self): - dataset = factories.Dataset(owner_org=self.org['id'], resources=[ - {'url': 'http://some.image.png', 'format': 'png'}]) + dataset = factories.Dataset(resources=[ + {'url': 'https://example.com/data.csv', 'format': 'csv'}]) helpers.call_action('job_clear') dataset['resources'] = [] helpers.call_action('package_update', **dataset) - assert [job['title'] for job in helpers.call_action('job_list')] == [ - 'DownloadAll changed "{}" {}'.format(dataset['name'], dataset['id'])] + assert 'DownloadAll changed "{}" {}'.format(dataset['name'], dataset['id']) in [ + job['title'] for job in helpers.call_action('job_list')] def test_created_dataset_leads_to_queued_task(self): dataset = {'name': 'testdataset_da', - 'owner_org': self.org['id'], 'title': 'Test Dataset', 'notes': 'Just another test dataset.', 'resources': [ - {'url': 'http://some.image.png', 'format': 'png'} + {'url': 'https://example.com/data.csv', 'format': 'csv'} ]} dataset = helpers.call_action('package_create', **dataset) # this should prompt datapackage.json to be updated - assert [job['title'] for job in helpers.call_action('job_list')] == [ - 'DownloadAll new "{}" {}'.format(dataset['name'], dataset['id'])] + assert 'DownloadAll new "{}" {}'.format(dataset['name'], dataset['id']) in [ + job['title'] for job in helpers.call_action('job_list')] def test_changed_dataset_leads_to_queued_task(self): - dataset = factories.Dataset(owner_org=self.org['id'], resources=[ - {'url': 'http://some.image.png', 'format': 'png'}]) + dataset = factories.Dataset(resources=[ + {'url': 'https://example.com/data.csv', 'format': 'csv'}]) helpers.call_action('job_clear') dataset['notes'] = 'Changed description' helpers.call_action('package_update', **dataset) # this should prompt datapackage.json to be updated - assert [job['title'] for job in helpers.call_action('job_list')] == [ - 'DownloadAll changed "{}" {}'.format(dataset['name'], dataset['id'])] + assert 'DownloadAll changed "{}" {}'.format(dataset['name'], dataset['id']) in [ + job['title'] for job in helpers.call_action('job_list')] def test_creation_of_zip_resource_leads_to_queued_task(self): # but we don't get an infinite loop because it is stopped by the # skip_if_no_changes - dataset = factories.Dataset(owner_org=self.org['id'], resources=[ - {'url': 'http://some.image.png', 'format': 'png'}]) + dataset = factories.Dataset(resources=[ + {'url': 'https://example.com/data.csv', 'format': 'csv'}]) helpers.call_action('job_clear') resource = { 'package_id': dataset['id'], @@ -73,15 +72,8 @@ def test_creation_of_zip_resource_leads_to_queued_task(self): } helpers.call_action('resource_create', **resource) - assert [job['title'] for job in helpers.call_action('job_list')] == [ - 'DownloadAll changed "{}" {}'.format(dataset['name'], dataset['id'])] - - def test_other_instance_types_do_nothing(self): - factories.User() - factories.Organization() - factories.Group() - assert not list(job['title'] for job in helpers.call_action('job_list')) - assert not list(helpers.call_action('job_list')) + assert 'DownloadAll changed "{}" {}'.format(dataset['name'], dataset['id']) in [ + job['title'] for job in helpers.call_action('job_list')] # An end-to-end test is too tricky to write - creating a dataset and seeing # the zip file created requires the queue worker to run, but that rips down diff --git a/ckanext/downloadall/tests/test_tasks.py b/ckanext/downloadall/tests/test_tasks.py index 8597add..47b3d43 100644 --- a/ckanext/downloadall/tests/test_tasks.py +++ b/ckanext/downloadall/tests/test_tasks.py @@ -57,10 +57,15 @@ def test_simple(self, _): body='a,b,c' ) responses.add_passthru(config['solr_url']) - dataset = factories.Dataset(owner_org=self.org['id'], resources=[{ - 'url': 'https://example.com/data.csv', - 'format': 'csv', - }]) + dataset = factories.Dataset( + name='test-dataset-a', + title='Test Dataset A', + notes='Just another test dataset.', + resources=[{ + 'url': 'https://example.com/data.csv', + 'format': 'csv' + }] + ) update_zip(dataset['id']) @@ -81,8 +86,8 @@ def test_simple(self, _): datapackage_json = zip_.read('datapackage.json').decode() assert '{\n "description"' in datapackage_json datapackage = json.loads(datapackage_json) - assert datapackage['name'][:12] == 'test_dataset' - assert datapackage['title'] == 'Test Dataset' + assert datapackage['name'] == 'test-dataset-a' + assert datapackage['title'] == 'Test Dataset A' assert datapackage['description'] == 'Just another test dataset.' assert datapackage['resources'][0]['format'] == 'CSV' assert datapackage['resources'][0]['name'] == dataset['resources'][0]['id'] @@ -99,10 +104,15 @@ def test_update_twice(self, _): body='a,b,c' ) responses.add_passthru(config['solr_url']) - dataset = factories.Dataset(owner_org=self.org['id'], resources=[{ - 'url': 'https://example.com/data.csv', - 'format': 'csv', - }]) + dataset = factories.Dataset( + name='test-dataset-b', + title='Test Dataset B', + notes='Just another test dataset.', + resources=[{ + 'url': 'https://example.com/data.csv', + 'format': 'csv' + }] + ) update_zip(dataset['id']) update_zip(dataset['id'], skip_if_no_changes=False) @@ -123,10 +133,15 @@ def test_dont_skip_if_no_changes(self, _): body='a,b,c' ) responses.add_passthru(config['solr_url']) - dataset = factories.Dataset(owner_org=self.org['id'], resources=[{ - 'url': 'https://example.com/data.csv', - 'format': 'csv', - }]) + dataset = factories.Dataset( + name='test-dataset-c', + title='Test Dataset C', + notes='Just another test dataset.', + resources=[{ + 'url': 'https://example.com/data.csv', + 'format': 'csv' + }] + ) update_zip(dataset['id']) with mock.patch('ckanext.downloadall.tasks.write_zip') as write_zip_: @@ -144,10 +159,15 @@ def test_update_twice_skipping_second_time(self, _): body='a,b,c' ) responses.add_passthru(config['solr_url']) - dataset = factories.Dataset(owner_org=self.org['id'], resources=[{ - 'url': 'https://example.com/data.csv', - 'format': 'csv', - }]) + dataset = factories.Dataset( + name='test-dataset-d', + title='Test Dataset D', + notes='Just another test dataset.', + resources=[{ + 'url': 'https://example.com/data.csv', + 'format': 'csv' + }] + ) update_zip(dataset['id']) with mock.patch('ckanext.downloadall.tasks.write_zip') as write_zip_: @@ -164,14 +184,19 @@ def test_changing_description_causes_zip_to_update(self, _): body='a,b,c' ) responses.add_passthru(config['solr_url']) - dataset = factories.Dataset(owner_org=self.org['id'], resources=[{ - 'url': 'https://example.com/data.csv', - 'format': 'csv', - }]) + dataset = factories.Dataset( + name='test-dataset-e', + title='Test Dataset E', + notes='Just another test dataset.', + resources=[{ + 'url': 'https://example.com/data.csv', + 'format': 'csv' + }] + ) update_zip(dataset['id']) dataset = helpers.call_action('package_patch', id=dataset['id'], - notes='New notes') + notes='New notes.') with mock.patch('ckanext.downloadall.tasks.write_zip') as write_zip_: update_zip(dataset['id'], skip_if_no_changes=True) # ensure zip would be rewritten in this case - not letting it skip @@ -186,10 +211,15 @@ def test_deleting_resource_causes_zip_to_update(self, _): body='a,b,c' ) responses.add_passthru(config['solr_url']) - dataset = factories.Dataset(owner_org=self.org['id'], resources=[{ - 'url': 'https://example.com/data.csv', - 'format': 'csv', - }]) + dataset = factories.Dataset( + name='test-dataset-f', + title='Test Dataset F', + notes='Just another test dataset.', + resources=[{ + 'url': 'https://example.com/data.csv', + 'format': 'csv' + }] + ) update_zip(dataset['id']) dataset = helpers.call_action('package_patch', id=dataset['id'], @@ -209,7 +239,7 @@ def test_uploaded_resource(self, _): re.compile(r'http://test.ckan.net/dataset/.*/download/.*'), body=csv_content ) - dataset = factories.Dataset(owner_org=self.org['id']) + dataset = factories.Dataset() # add a resource which is an uploaded file with tempfile.NamedTemporaryFile() as fp: fp.write(csv_content.encode()) @@ -227,8 +257,6 @@ def test_uploaded_resource(self, _): ctx['user'] = user['name'] helpers.call_action('resource_create', context=ctx, **resource) - # registry.action.resource_create(**resource) - update_zip(dataset['id']) dataset = helpers.call_action('package_show', id=dataset['id']) @@ -246,14 +274,11 @@ def test_uploaded_resource(self, _): # Check datapackage.json datapackage_json = zip_.read('datapackage.json') datapackage = json.loads(datapackage_json) - assert datapackage['resources'] == [{ - 'format': 'CSV', - 'name': 'rainfall', - 'path': csv_filename_in_zip, - 'sources': [{'path': dataset['resources'][0]['url'], - 'title': 'Rainfall'}], - 'title': 'Rainfall', - }] + assert datapackage['resources'][0]['title'] == 'Rainfall' + assert datapackage['resources'][0]['format'] == 'CSV' + assert datapackage['resources'][0]['path'] == csv_filename_in_zip + assert datapackage['resources'][0]['sources'] == [{'path': dataset['resources'][0]['url'], + 'title': 'Rainfall'}] @mock.patch('ckanext.downloadall.tasks.populate_schema_from_datastore', side_effect=mock_populate_schema_from_datastore) @@ -266,10 +291,15 @@ def test_data_dictionary(self, _, __): body='Date,Price\n1/6/2017,4.00\n2/6/2017,4.12' ) responses.add_passthru(config['solr_url']) - dataset = factories.Dataset(owner_org=self.org['id'], resources=[{ - 'url': 'https://example.com/data.csv', - 'format': 'csv', - }]) + dataset = factories.Dataset( + name='test-dataset-g', + title='Test Dataset G', + notes='Just another test dataset.', + resources=[{ + 'url': 'https://example.com/data.csv', + 'format': 'csv' + }] + ) update_zip(dataset['id']) @@ -302,11 +332,16 @@ def test_resource_url_with_connection_error(self, _): 'https://example.com/data.csv', body=requests.ConnectionError('Some network trouble...') ) - dataset = factories.Dataset(owner_org=self.org['id'], resources=[{ - 'url': 'https://example.com/data.csv', - 'name': 'rainfall', - 'format': 'csv', - }]) + dataset = factories.Dataset( + name='test-dataset-h', + title='Test Dataset H', + notes='Just another test dataset.', + resources=[{ + 'name': 'rainfall', + 'url': 'https://example.com/data.csv', + 'format': 'csv' + }] + ) update_zip(dataset['id']) @@ -328,8 +363,8 @@ def test_resource_url_with_connection_error(self, _): 'name': 'rainfall', # path is to the URL - an 'external resource' 'path': 'https://example.com/data.csv', - 'title': 'rainfall', - }] + 'title': 'rainfall' + }] @pytest.mark.ckan_config('ckan.storage_path', '/doesnt_exist') @responses.activate @@ -340,11 +375,16 @@ def test_resource_url_with_404_error(self, _): 'https://example.com/data.csv', status=404 ) - dataset = factories.Dataset(owner_org=self.org['id'], resources=[{ - 'url': 'https://example.com/data.csv', - 'name': 'rainfall', - 'format': 'csv', - }]) + dataset = factories.Dataset( + name='test-dataset-i', + title='Test Dataset I', + notes='Just another test dataset.', + resources=[{ + 'name': 'rainfall', + 'url': 'https://example.com/data.csv', + 'format': 'csv' + }] + ) update_zip(dataset['id']) @@ -366,8 +406,43 @@ def test_resource_url_with_404_error(self, _): 'name': 'rainfall', # path is to the URL - an 'external resource' 'path': 'https://example.com/data.csv', - 'title': 'rainfall', - }] + 'title': 'rainfall' + }] + + @pytest.mark.ckan_config('ckan.storage_path', '/doesnt_exist') + @responses.activate + def test_resource_with_no_format(self, _): + responses.add( + responses.GET, + 'https://example.com/data.csv', + body='a,b,c' + ) + responses.add_passthru(config['solr_url']) + dataset = factories.Dataset( + name='test-dataset-j', + title='Test Dataset J', + notes='Just another test dataset.', + resources=[{ + 'name': 'rainfall', + 'url': 'https://example.com/data.csv' + }] + ) + + update_zip(dataset['id']) + + dataset = helpers.call_action('package_show', id=dataset['id']) + zip_resources = [res for res in dataset['resources'] + if res['name'] == 'All resource data'] + zip_resource = zip_resources[0] + uploader = ckan.lib.uploader.get_resource_uploader(zip_resource) + filepath = uploader.get_path(zip_resource['id']) + with fake_open(filepath, 'rb') as f: + with zipfile.ZipFile(f) as zip_: + assert zip_.namelist() == ['rainfall.csv', 'datapackage.json'] + datapackage_json = zip_.read('datapackage.json').decode() + assert datapackage_json.startswith('{\n "description"') + datapackage = json.loads(datapackage_json) + assert datapackage['resources'][0]['name'] == 'rainfall' local_datapackage = { @@ -515,36 +590,42 @@ def test_dict_ordering(self): class TestGenerateDatapackageJson(TestBase): def test_simple(self): dataset = factories.Dataset( - owner_org=self.org['id'], + name='test-dataset-k', + title='Test Dataset K', + notes='Just another test dataset.', resources=[{ + 'name': 'rainfall', 'url': 'https://example.com/data.csv', - 'format': 'csv', - }]) + 'format': 'csv' + }] + ) datapackage, ckan_and_datapackage_resources, existing_zip_resource = generate_datapackage_json(dataset['id']) replace_number_suffix(datapackage, 'name') replace_uuid(datapackage['resources'][0], 'name') - assert datapackage == { - 'description': 'Just another test dataset.', - 'name': 'test_dataset_num', - 'resources': [{'format': 'CSV', - 'name': '', - 'path': 'https://example.com/data.csv'}], - 'title': 'Test Dataset' - } + assert datapackage['name'] == 'test-dataset-k' + assert datapackage['title'] == 'Test Dataset K' + assert datapackage['description'] == 'Just another test dataset.' + assert datapackage['resources'][0]['format'] == 'CSV' + assert datapackage['resources'][0]['name'] == '' + assert datapackage['resources'][0]['path'] == 'https://example.com/data.csv' assert ckan_and_datapackage_resources[0][0]['url'] == 'https://example.com/data.csv' assert not ckan_and_datapackage_resources[0][0]['description'] assert ckan_and_datapackage_resources[0][1] == { - 'format': 'CSV', 'name': '', - 'path': 'https://example.com/data.csv' + 'title': 'rainfall', + 'path': 'https://example.com/data.csv', + 'format': 'CSV' } assert existing_zip_resource is None def test_extras(self): dataset = factories.Dataset( - owner_org=self.org['id'], extras=[ + name='test-dataset-l', + title='Test Dataset L', + notes='Just another test dataset.', + extras=[ {'key': 'extra1', 'value': '1'}, {'key': 'extra2', 'value': '2'}, {'key': 'extra3', 'value': '3'}, @@ -555,29 +636,33 @@ def test_extras(self): replace_number_suffix(datapackage, 'name') assert datapackage == { + 'name': 'test-dataset-l', + 'title': 'Test Dataset L', 'description': 'Just another test dataset.', - 'name': 'test_dataset_num', - 'title': 'Test Dataset', - 'extras': {'extra1': 1, 'extra2': 2, 'extra3': 3}, - } + 'extras': {'extra1': 1, 'extra2': 2, 'extra3': 3} + } @pytest.mark.ckan_config( 'ckanext.downloadall.dataset_fields_to_add_to_datapackage', 'num_resources type') def test_added_fields(self): - dataset = factories.Dataset(owner_org=self.org['id']) + dataset = factories.Dataset( + name='test-dataset-m', + title='Test Dataset M', + notes='Just another test dataset.' + ) datapackage, _, __ = \ generate_datapackage_json(dataset['id']) replace_number_suffix(datapackage, 'name') assert datapackage == { + 'name': 'test-dataset-m', + 'title': 'Test Dataset M', 'description': 'Just another test dataset.', - 'name': 'test_dataset_num', - 'title': 'Test Dataset', 'num_resources': 0, - 'type': 'dataset', - } + 'type': 'dataset' + } # helpers diff --git a/conftest.py b/conftest.py deleted file mode 100644 index fae1fc4..0000000 --- a/conftest.py +++ /dev/null @@ -1,6 +0,0 @@ -# -*- coding: utf-8 -*- - -pytest_plugins = [ - u'ckan.tests.pytest_ckan.ckan_setup', - u'ckan.tests.pytest_ckan.fixtures', -] diff --git a/dev-requirements.txt b/dev-requirements.txt index a5ba66c..ae70874 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,3 +1,6 @@ responses==0.10.6 flake8 -pytest \ No newline at end of file +mock +pyfakefs +pytest-ckan +pytest-cov \ No newline at end of file