diff --git a/dataworkspace/dataworkspace/apps/datasets/admin.py b/dataworkspace/dataworkspace/apps/datasets/admin.py index 41c830c3aa..44b2446e6f 100644 --- a/dataworkspace/dataworkspace/apps/datasets/admin.py +++ b/dataworkspace/dataworkspace/apps/datasets/admin.py @@ -323,6 +323,23 @@ def save_model(self, request, obj, form, change): form.cleaned_data.get("authorized_users", get_user_model().objects.none()) ) + if form.cleaned_data["request_approvers"]: + request_approvers_emails = list(form.cleaned_data["request_approvers"]) + + existing_data_catalogue_editors_emails = ( + [item.email for item in form.cleaned_data["data_catalogue_editors"]] + if form.cleaned_data["data_catalogue_editors"] + else [] + ) + combined_emails = set().union( + request_approvers_emails, existing_data_catalogue_editors_emails + ) + + valid_request_approver_users = get_user_model().objects.filter( + email__in=combined_emails + ) + form.cleaned_data["data_catalogue_editors"] = valid_request_approver_users + added_users = ( form.cleaned_data["data_catalogue_editors"].difference( obj.data_catalogue_editors.all() diff --git a/dataworkspace/dataworkspace/apps/datasets/views.py b/dataworkspace/dataworkspace/apps/datasets/views.py index 93a08d3012..e25b6ce0fd 100644 --- a/dataworkspace/dataworkspace/apps/datasets/views.py +++ b/dataworkspace/dataworkspace/apps/datasets/views.py @@ -1576,6 +1576,10 @@ def remove_authorised_editor(request, pk, user_id): user = get_user_model().objects.get(id=user_id) dataset.data_catalogue_editors.remove(user) + if dataset.request_approvers and user.email in dataset.request_approvers: + dataset.request_approvers.remove(user.email) + dataset.save() + log_event( request.user, EventLog.TYPE_DATA_CATALOGUE_EDITOR_REMOVED, diff --git a/dataworkspace/dataworkspace/tests/datasets/test_views.py b/dataworkspace/dataworkspace/tests/datasets/test_views.py index a890e44a63..364439f3be 100644 --- a/dataworkspace/dataworkspace/tests/datasets/test_views.py +++ b/dataworkspace/dataworkspace/tests/datasets/test_views.py @@ -41,7 +41,7 @@ from dataworkspace.apps.eventlog.models import EventLog from dataworkspace.apps.your_files.models import UploadedTable from dataworkspace.tests import factories -from dataworkspace.tests.common import get_http_sso_data, MatchUnorderedMembers +from dataworkspace.tests.common import BaseTestCase, get_http_sso_data, MatchUnorderedMembers from dataworkspace.tests.factories import ( VisualisationCatalogueItemFactory, UserFactory, @@ -4879,3 +4879,85 @@ def test_master_dataset_detail_page_shows_pipeline_failures(client, metadata_db) len([x for x in response.context["master_datasets_info"] if x.pipeline_last_run_succeeded]) == 1 ) + + +class TestRemoveAuthorisedEditor(BaseTestCase): + @mock.patch("dataworkspace.apps.datasets.views.log_event") + def test_user_is_removed_from_data_catalogue_editors_when_request_approvers_missing( + self, mock_log_event + ): + ds = factories.DataSetFactory(published=True) + remaining_user = UserFactory() + + ds.data_catalogue_editors.set([self.user.id, remaining_user.id]) + + self._authenticated_get( + reverse( + "datasets:remove_authorised_editor", + args=( + ds.id, + self.user.id, + ), + ) + ) + + mock_log_event.assert_called() + updated_ds = DataSet.objects.filter(pk=ds.id).first() + + assert updated_ds.data_catalogue_editors.count() == 1 + assert updated_ds.data_catalogue_editors.filter(pk=self.user.id).exists() is False + + @mock.patch("dataworkspace.apps.datasets.views.log_event") + def test_user_is_removed_from_data_catalogue_editors_only_when_not_present_in_request_approvers( + self, mock_log_event + ): + remaining_user = UserFactory() + ds = factories.DataSetFactory(published=True, request_approvers=[remaining_user.email]) + + ds.data_catalogue_editors.set([self.user.id, remaining_user.id]) + + self._authenticated_get( + reverse( + "datasets:remove_authorised_editor", + args=( + ds.id, + self.user.id, + ), + ) + ) + + updated_ds = DataSet.objects.filter(pk=ds.id).first() + + assert updated_ds.data_catalogue_editors.count() == 1 + assert updated_ds.data_catalogue_editors.filter(pk=self.user.id).exists() is False + + assert updated_ds.request_approvers == [remaining_user.email] + + @mock.patch("dataworkspace.apps.datasets.views.log_event") + def test_user_is_removed_from_data_catalogue_editors_and_request_approvers_when_present( + self, mock_log_event + ): + remaining_user = UserFactory() + ds = factories.DataSetFactory( + published=True, request_approvers=[self.user.email, remaining_user.email] + ) + + ds.data_catalogue_editors.set([self.user.id, remaining_user.id]) + + self._authenticated_get( + reverse( + "datasets:remove_authorised_editor", + args=( + ds.id, + self.user.id, + ), + ) + ) + + mock_log_event.assert_called() + updated_ds = DataSet.objects.filter(pk=ds.id).first() + + assert updated_ds.data_catalogue_editors.count() == 1 + assert updated_ds.data_catalogue_editors.filter(pk=self.user.id).exists() is False + + assert updated_ds.request_approvers == [remaining_user.email] diff --git a/dataworkspace/dataworkspace/tests/test_admin.py b/dataworkspace/dataworkspace/tests/test_admin.py index bdade08692..26fea69a55 100644 --- a/dataworkspace/dataworkspace/tests/test_admin.py +++ b/dataworkspace/dataworkspace/tests/test_admin.py @@ -2,6 +2,7 @@ import inspect import sys import mock +import factory from botocore.exceptions import ClientError @@ -2552,6 +2553,37 @@ def test_source_link_upload(self, mock_client): class TestDatasetAdmin(BaseAdminTestCase): + def _valid_dataset(self, dataset, user): + return { + "published": dataset.published, + "name": dataset.name, + "slug": dataset.slug, + "short_description": "test short description", + "description": LONG_DATASET_DESCRIPTION, + "information_asset_owner": str(user.id), + "information_asset_manager": str(user.id), + "enquiries_contact": str(user.id), + "type": 2, + "user_access_type": UserAccessType.OPEN, + "sourcelink_set-TOTAL_FORMS": "0", + "sourcelink_set-INITIAL_FORMS": "0", + "sourcelink_set-MIN_NUM_FORMS": "0", + "sourcelink_set-MAX_NUM_FORMS": "1000", + "sourceview_set-TOTAL_FORMS": "0", + "sourceview_set-INITIAL_FORMS": "0", + "sourceview_set-MIN_NUM_FORMS": "0", + "sourceview_set-MAX_NUM_FORMS": "1000", + "customdatasetquery_set-TOTAL_FORMS": "0", + "customdatasetquery_set-INITIAL_FORMS": "0", + "customdatasetquery_set-MIN_NUM_FORMS": "0", + "customdatasetquery_set-MAX_NUM_FORMS": "1000", + "_continue": "Save and continue editing", + "charts-TOTAL_FORMS": "0", + "charts-INITIAL_FORMS": "0", + "charts-MIN_NUM_FORMS": "0", + "charts-MAX_NUM_FORMS": "1000", + } + def test_update_dataset_with_description_not_long_enough(self): dataset = factories.DataSetFactory.create() response = self._authenticated_post( @@ -2911,6 +2943,116 @@ def test_delete_local_source_link(self, mock_client): Bucket=settings.AWS_UPLOADS_BUCKET, Key="s3://sourcelink/a-file.txt" ) + @mock.patch("dataworkspace.apps.core.boto3_client.boto3.client") + def test_request_approvers_with_unknown_email_address_not_added_to_data_catalogue_editors( + self, mock_client + ): + dataset = factories.DataSetFactory() + user = factories.UserFactory() + data = self._valid_dataset(dataset, user) + data["request_approvers"] = ["not_real"] + + self._authenticated_post( + reverse("admin:datasets_datacutdataset_change", args=(dataset.id,)), + data, + ) + + reloaded_dataset = DataSet.objects.filter(pk=dataset.id).first() + assert reloaded_dataset.data_catalogue_editors.count() == 0 + + @mock.patch("dataworkspace.apps.core.boto3_client.boto3.client") + def test_request_approvers_with_new_email_address_added_to_data_catalogue_editors( + self, mock_client + ): + dataset = factories.DataSetFactory() + user = factories.UserFactory() + data = self._valid_dataset(dataset, user) + # data['data_catalogue_editors'] = [user.id] + + self._authenticated_post( + reverse("admin:datasets_datacutdataset_change", args=(dataset.id,)), + data, + ) + + data["request_approvers"] = [user.email] + + self._authenticated_post( + reverse("admin:datasets_datacutdataset_change", args=(dataset.id,)), + data, + ) + + reloaded_dataset = DataSet.objects.filter(pk=dataset.id).first() + assert reloaded_dataset.data_catalogue_editors.count() == 1 + + @mock.patch("dataworkspace.apps.core.boto3_client.boto3.client") + def test_request_approvers_with_existing_email_address_in_data_catalogue_editors_not_added_again_to_data_catalogue_editors( # pylint: disable=line-too-long + self, mock_client + ): + dataset = factories.DataSetFactory() + user = factories.UserFactory() + data = self._valid_dataset(dataset, user) + data["data_catalogue_editors"] = [user.id] + + self._authenticated_post( + reverse("admin:datasets_datacutdataset_change", args=(dataset.id,)), + data, + ) + + data["request_approvers"] = [user.email] + + self._authenticated_post( + reverse("admin:datasets_datacutdataset_change", args=(dataset.id,)), + data, + ) + + reloaded_dataset = DataSet.objects.filter(pk=dataset.id).first() + assert reloaded_dataset.data_catalogue_editors.count() == 1 + + @mock.patch("dataworkspace.apps.core.boto3_client.boto3.client") + def test_request_approvers_and_data_catalogue_editors_containing_same_email_only_added_once_to_data_catalogue_editors( + self, mock_client + ): + dataset = factories.DataSetFactory() + user = factories.UserFactory() + data = self._valid_dataset(dataset, user) + data["data_catalogue_editors"] = [user.id] + data["request_approvers"] = [user.email] + + self._authenticated_post( + reverse("admin:datasets_datacutdataset_change", args=(dataset.id,)), + data, + ) + + reloaded_dataset = DataSet.objects.filter(pk=dataset.id).first() + assert reloaded_dataset.data_catalogue_editors.count() == 1 + + @mock.patch("dataworkspace.apps.core.boto3_client.boto3.client") + def test_request_approvers_with_new_known_email_addresses_added_to_existing_data_catalogue_editors( + self, mock_client + ): + dataset = factories.DataSetFactory() + user = factories.UserFactory() + matching_request_approvers = ["new_user1@example.com", "new_user2@example.com"] + factories.UserFactory.create_batch( + len(matching_request_approvers), email=factory.Iterator(matching_request_approvers) + ) + data = self._valid_dataset(dataset, user) + data["data_catalogue_editors"] = [user.id] + self._authenticated_post( + reverse("admin:datasets_datacutdataset_change", args=(dataset.id,)), + data, + ) + + data["request_approvers"] = matching_request_approvers + ["not_real"] + + self._authenticated_post( + reverse("admin:datasets_datacutdataset_change", args=(dataset.id,)), + data, + ) + + reloaded_dataset = DataSet.objects.filter(pk=dataset.id).first() + assert reloaded_dataset.data_catalogue_editors.count() == 3 + class TestDatasetAdminPytest: def test_sql_queries_must_be_reviewed_before_publishing(self, staff_client, user):