From faf8756fb73e1a195e32c7d1bc4850352c7fc973 Mon Sep 17 00:00:00 2001 From: pdelboca Date: Wed, 14 Dec 2022 17:38:00 +0100 Subject: [PATCH 1/7] Add workflow for 2.9 and 2.10 --- .github/workflows/test.yml | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6377871..1344978 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -6,8 +6,6 @@ jobs: steps: - uses: actions/checkout@v2 - uses: actions/setup-python@v2 - with: - python-version: '3.6' - name: Install requirements run: pip install flake8 pycodestyle - name: Check syntax @@ -17,13 +15,23 @@ jobs: needs: lint name: CKAN 2.9 runs-on: ubuntu-latest + strategy: + matrix: + include: + - ckan-version: "2.10" + solr-image: "2.10" + - ckan-version: 2.9 + solr-image: 2.9 + - ckan-version: 2.9 + solr-image: 2.9-solr8 + fail-fast: false container: - image: openknowledge/ckan-dev:2.9 + image: openknowledge/ckan-dev:${{ matrix.ckan-version }} services: solr: - image: ckan/ckan-solr-dev:2.9 + image: ckan/ckan-solr:${{ matrix.solr-image }} postgres: - image: ckan/ckan-postgres-dev:2.9 + image: ckan/ckan-postgres-dev:${{ matrix.ckan-version }} env: POSTGRES_USER: postgres POSTGRES_PASSWORD: postgres @@ -47,6 +55,10 @@ 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 + - name: Remove activity plugin (CKAN 2.9) + if: {{ matrix.ckan-version == '2.9'}} + run: | + ckan config-tool test.ini "ckan.plugins=versions image_view" - name: Setup extension run: | ckan -c test.ini db init From 9340e83e00562c5ccd15f51d14ab380dbfefba88 Mon Sep 17 00:00:00 2001 From: pdelboca Date: Wed, 14 Dec 2022 17:39:14 +0100 Subject: [PATCH 2/7] Add support for CKAN 2.10 CKAN 2.10 separates Activity logic into its own extension. Therefore we need to change were we import from. --- ckanext/versions/logic/action.py | 10 ++- ckanext/versions/tests/test_actions.py | 12 +-- ckanext/versions/tests/test_auth.py | 105 +++++++++++++------------ test.ini | 2 +- 4 files changed, 68 insertions(+), 61 deletions(-) diff --git a/ckanext/versions/logic/action.py b/ckanext/versions/logic/action.py index 450a5f6..0f25907 100644 --- a/ckanext/versions/logic/action.py +++ b/ckanext/versions/logic/action.py @@ -10,6 +10,12 @@ from ckan.plugins import toolkit from sqlalchemy.exc import IntegrityError +try: + from ckanext.activity.model import Activity +except ImportError: + # Remove when dropping support for 2.9.x + from ckan.model import Activity + from ckanext.versions.model import Version log = logging.getLogger(__name__) @@ -186,9 +192,9 @@ def resource_version_create(context, data_dict): creator_user_id = _get_creator_user_id(data_dict, model, context) - activity = model.Session.query(model.Activity). \ + activity = model.Session.query(Activity). \ filter_by(object_id=resource.package_id). \ - order_by(model.Activity.timestamp.desc()). \ + order_by(Activity.timestamp.desc()). \ first() if not activity: diff --git a/ckanext/versions/tests/test_actions.py b/ckanext/versions/tests/test_actions.py index 9a0d5e5..ade49a2 100644 --- a/ckanext/versions/tests/test_actions.py +++ b/ckanext/versions/tests/test_actions.py @@ -297,14 +297,14 @@ def test_name_must_be_unique(self): def test_version_id_is_mandatory_for_update(self): with pytest.raises(toolkit.ValidationError) as e: helpers.call_action('resource_version_update', {}, name='2.0') - assert 'Missing value' in str(e) - assert 'version_id' in str(e) + assert 'Missing value' in str(e.value) + assert 'version_id' in str(e.value) def test_version_name_is_mandatory_for_update(self): with pytest.raises(toolkit.ValidationError) as e: helpers.call_action('resource_version_update', {}, version_id='fake-id') - assert 'Missing value' in str(e) - assert 'name' in str(e) + assert 'Missing value' in str(e.value) + assert 'name' in str(e.value) @pytest.mark.usefixtures('clean_db', 'versions_setup') @@ -372,8 +372,8 @@ def test_version_id_is_mandatory_for_patch(self): with pytest.raises(toolkit.ValidationError) as e: helpers.call_action('resource_version_patch', {}, name='2.0') - assert "Missing value" in str(e) - assert "version_id" in str(e) + assert "Missing value" in str(e.value) + assert "version_id" in str(e.value) @pytest.mark.usefixtures('clean_db', 'versions_setup') diff --git a/ckanext/versions/tests/test_auth.py b/ckanext/versions/tests/test_auth.py index e5b0456..b7064e6 100644 --- a/ckanext/versions/tests/test_auth.py +++ b/ckanext/versions/tests/test_auth.py @@ -4,7 +4,35 @@ from ckan.tests import factories, helpers -@pytest.mark.usefixtures("clean_db", "versions_setup") +@pytest.fixture(scope="class") +def auth_fixture(): + org_admin = factories.User() + org_editor = factories.User() + org_member = factories.User() + other_org_admin = factories.User() + admin_user = factories.Sysadmin() + + org = factories.Organization( + users=[ + {'name': org_admin['name'], 'capacity': 'admin'}, + {'name': org_editor['name'], 'capacity': 'editor'}, + {'name': org_member['name'], 'capacity': 'member'}, + ] + ) + + other_org = factories.Organization( + users=[ + {'name': other_org_admin['name'], 'capacity': 'admin'}, + ] + ) + + private_dataset = factories.Dataset(owner_org=org['id'], private=True) + public_dataset = factories.Dataset(owner_org=org['id'], private=False) + yield locals() + helpers.reset_db() + + +@pytest.mark.usefixtures("versions_setup") class TestVersionsAuth(object): def _get_context(self, user): @@ -13,33 +41,6 @@ def _get_context(self, user): 'user': user if isinstance(user, str) else user['name'] } - def setup(self): - # TODO: Refactor to a new pytest approach - self.org_admin = factories.User() - self.org_editor = factories.User() - self.org_member = factories.User() - self.other_org_admin = factories.User() - self.admin_user = factories.Sysadmin() - - self.org = factories.Organization( - users=[ - {'name': self.org_admin['name'], 'capacity': 'admin'}, - {'name': self.org_editor['name'], 'capacity': 'editor'}, - {'name': self.org_member['name'], 'capacity': 'member'}, - ] - ) - - self.other_org = factories.Organization( - users=[ - {'name': self.other_org_admin['name'], 'capacity': 'admin'}, - ] - ) - - self.private_dataset = factories.Dataset(owner_org=self.org['id'], - private=True) - self.public_dataset = factories.Dataset(owner_org=self.org['id'], - private=False) - @pytest.mark.parametrize("user_type, dataset_type", [ ('org_admin', 'private_dataset'), ('org_admin', 'public_dataset'), @@ -48,11 +49,11 @@ def setup(self): ('admin_user', 'private_dataset'), ('admin_user', 'public_dataset'), ]) - def test_create_is_authorized(self, user_type, dataset_type): + def test_create_is_authorized(self, user_type, dataset_type, auth_fixture): """Test that authorized users can create versions on a given dataset """ - user = getattr(self, user_type) - dataset = getattr(self, dataset_type) + user = auth_fixture[user_type] + dataset = auth_fixture[dataset_type] context = self._get_context(user) assert helpers.call_auth('version_create', context=context, @@ -64,12 +65,12 @@ def test_create_is_authorized(self, user_type, dataset_type): ('other_org_admin', 'private_dataset'), ('other_org_admin', 'public_dataset'), ]) - def test_create_is_unauthorized(self, user_type, dataset_type): + def test_create_is_unauthorized(self, user_type, dataset_type, auth_fixture): """Test that unauthorized users cannot create versions on a given dataset """ - user = getattr(self, user_type) - dataset = getattr(self, dataset_type) + user = auth_fixture[user_type] + dataset = auth_fixture[dataset_type] context = self._get_context(user) with pytest.raises(toolkit.NotAuthorized): helpers.call_auth( @@ -85,11 +86,11 @@ def test_create_is_unauthorized(self, user_type, dataset_type): ('admin_user', 'private_dataset'), ('admin_user', 'public_dataset'), ]) - def test_delete_is_authorized(self, user_type, dataset_type): + def test_delete_is_authorized(self, user_type, dataset_type, auth_fixture): """Test that authorized users can delete versions on a given dataset """ - user = getattr(self, user_type) - dataset = getattr(self, dataset_type) + user = auth_fixture[user_type] + dataset = auth_fixture[dataset_type] context = self._get_context(user) assert helpers.call_auth('version_delete', context=context, @@ -101,12 +102,12 @@ def test_delete_is_authorized(self, user_type, dataset_type): ('other_org_admin', 'private_dataset'), ('other_org_admin', 'public_dataset'), ]) - def test_delete_is_unauthorized(self, user_type, dataset_type): + def test_delete_is_unauthorized(self, user_type, dataset_type, auth_fixture): """Test that unauthorized users cannot delete versions on a given dataset """ - user = getattr(self, user_type) - dataset = getattr(self, dataset_type) + user = auth_fixture[user_type] + dataset = auth_fixture[dataset_type] context = self._get_context(user) with pytest.raises(toolkit.NotAuthorized): helpers.call_auth( @@ -125,11 +126,11 @@ def test_delete_is_unauthorized(self, user_type, dataset_type): ('admin_user', 'public_dataset'), ('other_org_admin', 'public_dataset'), ]) - def test_list_is_authorized(self, user_type, dataset_type): + def test_list_is_authorized(self, user_type, dataset_type, auth_fixture): """Test that authorized users can list versions of a given dataset """ - user = getattr(self, user_type) - dataset = getattr(self, dataset_type) + user = auth_fixture[user_type] + dataset = auth_fixture[dataset_type] context = self._get_context(user) assert helpers.call_auth('version_list', context=context, @@ -138,12 +139,12 @@ def test_list_is_authorized(self, user_type, dataset_type): @pytest.mark.parametrize("user_type, dataset_type", [ ('other_org_admin', 'private_dataset'), ]) - def test_list_is_unauthorized(self, user_type, dataset_type): + def test_list_is_unauthorized(self, user_type, dataset_type, auth_fixture): """Test that unauthorized users cannot list versions on a given dataset """ - user = getattr(self, user_type) - dataset = getattr(self, dataset_type) + user = auth_fixture[user_type] + dataset = auth_fixture[dataset_type] context = self._get_context(user) with pytest.raises(toolkit.NotAuthorized): helpers.call_auth( @@ -162,11 +163,11 @@ def test_list_is_unauthorized(self, user_type, dataset_type): ('admin_user', 'public_dataset'), ('other_org_admin', 'public_dataset'), ]) - def test_show_is_authorized(self, user_type, dataset_type): + def test_show_is_authorized(self, user_type, dataset_type, auth_fixture): """Test that authorized users can view versions of a given dataset """ - user = getattr(self, user_type) - dataset = getattr(self, dataset_type) + user = auth_fixture[user_type] + dataset = auth_fixture[dataset_type] context = self._get_context(user) assert helpers.call_auth('version_show', context=context, @@ -175,12 +176,12 @@ def test_show_is_authorized(self, user_type, dataset_type): @pytest.mark.parametrize("user_type, dataset_type", [ ('other_org_admin', 'private_dataset'), ]) - def test_show_is_unauthorized(self, user_type, dataset_type): + def test_show_is_unauthorized(self, user_type, dataset_type, auth_fixture): """Test that unauthorized users cannot view versions on a given dataset """ - user = getattr(self, user_type) - dataset = getattr(self, dataset_type) + user = auth_fixture[user_type] + dataset = auth_fixture[dataset_type] context = self._get_context(user) with pytest.raises(toolkit.NotAuthorized): helpers.call_auth( diff --git a/test.ini b/test.ini index ad35da7..39b74f2 100644 --- a/test.ini +++ b/test.ini @@ -14,7 +14,7 @@ use = config:../ckan/test-core.ini # Insert any custom config settings to be used when running your extension's # tests here. -ckan.plugins = versions image_view +ckan.plugins=activity versions # Logging configuration [loggers] From b8ef84c272ead33af4418ac0477f0926c4feb1c9 Mon Sep 17 00:00:00 2001 From: pdelboca Date: Wed, 14 Dec 2022 17:44:31 +0100 Subject: [PATCH 3/7] Fix typo --- .github/workflows/test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1344978..7c25dca 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -56,7 +56,7 @@ jobs: # 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 - name: Remove activity plugin (CKAN 2.9) - if: {{ matrix.ckan-version == '2.9'}} + if: ${{ matrix.ckan-version == '2.9' }} run: | ckan config-tool test.ini "ckan.plugins=versions image_view" - name: Setup extension From e4a3913ff332d912e7f3b8f8449230d0263d262d Mon Sep 17 00:00:00 2001 From: pdelboca Date: Wed, 14 Dec 2022 17:49:18 +0100 Subject: [PATCH 4/7] Restore image_view plugin --- test.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test.ini b/test.ini index 39b74f2..c700d4a 100644 --- a/test.ini +++ b/test.ini @@ -14,7 +14,7 @@ use = config:../ckan/test-core.ini # Insert any custom config settings to be used when running your extension's # tests here. -ckan.plugins=activity versions +ckan.plugins=activity versions image_view # Logging configuration [loggers] From a0efa157a89f8092c3f87c48882ad97f1ccbf7a8 Mon Sep 17 00:00:00 2001 From: pdelboca Date: Wed, 14 Dec 2022 17:51:32 +0100 Subject: [PATCH 5/7] Update actions versions --- .github/workflows/test.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7c25dca..91a847e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -4,8 +4,8 @@ jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - - uses: actions/setup-python@v2 + - uses: actions/checkout@v3 + - uses: actions/setup-python@v4 - name: Install requirements run: pip install flake8 pycodestyle - name: Check syntax @@ -13,7 +13,7 @@ jobs: test: needs: lint - name: CKAN 2.9 + name: ckanext-versions tests runs-on: ubuntu-latest strategy: matrix: @@ -47,7 +47,7 @@ jobs: CKAN_REDIS_URL: redis://redis:6379/1 steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Install requirements run: | pip install -r requirements.txt From fb7a97dfff5b92264bcb40088cf07fc001fc1703 Mon Sep 17 00:00:00 2001 From: pdelboca Date: Wed, 14 Dec 2022 17:54:02 +0100 Subject: [PATCH 6/7] Fix pep8 --- ckanext/versions/logic/action.py | 22 +++++++++++---------- ckanext/versions/tests/test_actions.py | 27 ++++++++++++++------------ 2 files changed, 27 insertions(+), 22 deletions(-) diff --git a/ckanext/versions/logic/action.py b/ckanext/versions/logic/action.py index 0f25907..4457828 100644 --- a/ckanext/versions/logic/action.py +++ b/ckanext/versions/logic/action.py @@ -363,15 +363,15 @@ def resource_history(context, data_dict): result = [] for version in versions_list: - resource = activity_resource_show( - {'user': context['user']}, - { - 'activity_id': version['activity_id'], - 'resource_id': version['resource_id'] - } - ) - resource['version'] = version - result.append(resource) + resource = activity_resource_show( + {'user': context['user']}, + { + 'activity_id': version['activity_id'], + 'resource_id': version['resource_id'] + } + ) + resource['version'] = version + result.append(resource) return result @@ -432,7 +432,9 @@ def resource_in_activity(context, data_dict): ''' user = context.get('user') if not user: - site_user = toolkit.get_action('get_site_user')({'ignore_auth': True},{}) + site_user = toolkit.get_action('get_site_user')( + {'ignore_auth': True}, {} + ) user = site_user['name'] activity_show_context = { diff --git a/ckanext/versions/tests/test_actions.py b/ckanext/versions/tests/test_actions.py index ade49a2..697e545 100644 --- a/ckanext/versions/tests/test_actions.py +++ b/ckanext/versions/tests/test_actions.py @@ -170,9 +170,9 @@ def test_resource_has_version(self): user = factories.Sysadmin() context = get_context(user) - assert False == resource_has_versions( + assert resource_has_versions( context, {'resource_id': resource['id']} - ) + ) is False resource_version_create( context, { @@ -182,9 +182,9 @@ def test_resource_has_version(self): } ) - assert True == resource_has_versions( + assert resource_has_versions( context, {'resource_id': resource['id']} - ) + ) is True def test_resource_version_create_creator_user_id_parameter(self): user = factories.User() @@ -270,7 +270,7 @@ def test_resource_version_update_overrides_if_not_provided(self): ) assert version['name'] == '2.0' - assert version['notes'] == None + assert version['notes'] is None assert version['creator_user_id'] == new_creator['id'] def test_name_must_be_unique(self): @@ -292,7 +292,7 @@ def test_name_must_be_unique(self): resource_id=resource['id'], name='1.0', notes='Another version 1.0' - ) + ) def test_version_id_is_mandatory_for_update(self): with pytest.raises(toolkit.ValidationError) as e: @@ -302,7 +302,9 @@ def test_version_id_is_mandatory_for_update(self): def test_version_name_is_mandatory_for_update(self): with pytest.raises(toolkit.ValidationError) as e: - helpers.call_action('resource_version_update', {}, version_id='fake-id') + helpers.call_action( + 'resource_version_update', {}, version_id='fake-id' + ) assert 'Missing value' in str(e.value) assert 'name' in str(e.value) @@ -366,7 +368,7 @@ def test_name_must_be_unique(self): context, resource_id=resource['id'], name='1.0' - ) + ) def test_version_id_is_mandatory_for_patch(self): with pytest.raises(toolkit.ValidationError) as e: @@ -544,6 +546,7 @@ def test_resource_version_clear(self): assert len(resource_version_list(context, {'resource_id': resource['id']})) == 0 + @pytest.mark.usefixtures('clean_db', 'versions_setup') class TestActivityActions(object): @@ -684,20 +687,20 @@ def test_resource_in_activity(self): ) expected_activity_id = version['activity_id'] - assert True == resource_in_activity(context, { + assert resource_in_activity(context, { 'activity_id': expected_activity_id, 'resource_id': resource['id']} - ) + ) is True resource_2 = factories.Resource( package_id=dataset['id'], name='Resource 2' ) - assert False == resource_in_activity(context, { + assert resource_in_activity(context, { 'activity_id': expected_activity_id, 'resource_id': resource_2['id']} - ) + ) is False @pytest.mark.usefixtures('clean_db', 'versions_setup') From 0d7cf62a19c7cc546096b05231187e9f5cc688f7 Mon Sep 17 00:00:00 2001 From: pdelboca Date: Mon, 19 Jun 2023 10:07:57 +0200 Subject: [PATCH 7/7] Add new helper to get resource from the activity --- ckanext/versions/helpers.py | 13 +++++++++++++ ckanext/versions/plugin.py | 2 ++ 2 files changed, 15 insertions(+) diff --git a/ckanext/versions/helpers.py b/ckanext/versions/helpers.py index 29c442b..bff0bd3 100644 --- a/ckanext/versions/helpers.py +++ b/ckanext/versions/helpers.py @@ -73,3 +73,16 @@ def download_url(resource_url, version_id): ) return url + + +@toolkit.side_effect_free +def get_resource_from_activity(activity_id, resource_id): + """Get the resource dict from the activity object. + """ + context = {"user": toolkit.g.user} + resource = toolkit.get_action("activity_resource_show")( + context, { + "activity_id": activity_id, + "resource_id": resource_id + }) + return resource diff --git a/ckanext/versions/plugin.py b/ckanext/versions/plugin.py index 95c8f27..ec5230c 100644 --- a/ckanext/versions/plugin.py +++ b/ckanext/versions/plugin.py @@ -55,6 +55,7 @@ def get_actions(self): 'version_show': action.version_show, 'version_delete': action.version_delete, 'resource_view_list': action.resource_view_list, + 'activity_resource_show': action.activity_resource_show } # IAuthFunctions @@ -77,6 +78,7 @@ def get_helpers(self): 'versions_resource_version_from_activity_id': helpers.resource_version_from_activity_id, 'versions_resource_version_current': helpers.resource_version_current, 'versions_download_url': helpers.download_url, + 'versions_get_resource_from_activity': helpers.get_resource_from_activity, } return helper_functions