From 928238fb6e40a90ea1d8cadb5818aa2d23992ea1 Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Wed, 13 Sep 2023 16:59:35 +0100 Subject: [PATCH] NPL-1704 remove send_task flag to simplify the flow. Update tests to check tasks are sent when creating datasources --- controlpanel/api/models/s3bucket.py | 17 +++--- controlpanel/api/models/users3bucket.py | 9 +--- controlpanel/frontend/views/datasource.py | 42 ++++----------- tests/frontend/views/test_datasource.py | 63 +++++++++++++++-------- 4 files changed, 58 insertions(+), 73 deletions(-) diff --git a/controlpanel/api/models/s3bucket.py b/controlpanel/api/models/s3bucket.py index d41c33cc4..90de336f3 100644 --- a/controlpanel/api/models/s3bucket.py +++ b/controlpanel/api/models/s3bucket.py @@ -66,7 +66,6 @@ class Meta: def __init__(self, *args, **kwargs): """Overwrite this constructor to pass some non-field parameter""" self.bucket_owner = kwargs.pop("bucket_owner", cluster.AWSRoleCategory.user) - self.send_task = kwargs.pop("send_task", True) super().__init__(*args, **kwargs) def __repr__(self): @@ -133,14 +132,13 @@ def save(self, *args, **kwargs): if not is_create: return self - if self.send_task: - tasks.S3BucketCreate( - entity=self, - user=self.created_by, - extra_data={ - "bucket_owner": kwargs.pop("bucket_owner", self.bucket_owner), - } - ).create_task() + tasks.S3BucketCreate( + entity=self, + user=self.created_by, + extra_data={ + "bucket_owner": kwargs.pop("bucket_owner", self.bucket_owner), + } + ).create_task() # created_by should always be set, but this is a failsafe if self.created_by: @@ -150,7 +148,6 @@ def save(self, *args, **kwargs): s3bucket=self, is_admin=True, access_level=UserS3Bucket.READWRITE, - send_task=self.send_task ) return self diff --git a/controlpanel/api/models/users3bucket.py b/controlpanel/api/models/users3bucket.py index 78ee79fe8..2832058d8 100644 --- a/controlpanel/api/models/users3bucket.py +++ b/controlpanel/api/models/users3bucket.py @@ -32,7 +32,6 @@ class Meta: def __init__(self, *args, **kwargs): """Overwrite this constructor to pass some non-field parameter""" self.current_user = kwargs.pop("current_user", None) - self.send_task = kwargs.pop("send_task", True) super().__init__(*args, **kwargs) @property @@ -46,13 +45,7 @@ def __repr__(self): ) def grant_bucket_access(self): - """ - If send_task is False, granting permission must be handled elsewhere. This may - be because we are using a database transaction and want all objects to be - created before sending the task to grant access. - """ - if self.send_task: - tasks.S3BucketGrantToUser(self, self.current_user).create_task() + tasks.S3BucketGrantToUser(self, self.current_user).create_task() def revoke_bucket_access(self): tasks.S3BucketRevokeUserAccess(self).create_task() diff --git a/controlpanel/frontend/views/datasource.py b/controlpanel/frontend/views/datasource.py index 5f605a380..77e552cf0 100644 --- a/controlpanel/frontend/views/datasource.py +++ b/controlpanel/frontend/views/datasource.py @@ -174,44 +174,20 @@ def form_valid(self, form): datasource_type = self.request.GET.get("type") try: - with transaction.atomic(): - # create the object but dont send task yet - self.object = S3Bucket.objects.create( - name=name, - created_by=self.request.user, - is_data_warehouse=datasource_type == "warehouse", - send_task=False - ) - messages.success( - self.request, - f"Successfully created {name} {datasource_type} data source", - ) - # instead wait for all objects to be committed to the database before - # sending task to avoid a race condition resulting in task error - transaction.on_commit(self.send_create_tasks) + self.object = S3Bucket.objects.create( + name=name, + created_by=self.request.user, + is_data_warehouse=datasource_type == "warehouse", + ) + messages.success( + self.request, + f"Successfully created {name} {datasource_type} data source", + ) except Exception as ex: form.add_error("name", str(ex)) return FormMixin.form_invalid(self, form) return FormMixin.form_valid(self, form) - def send_create_tasks(self): - """ - Sends tasks to create the S3 bucket and grant access to the user - """ - tasks.S3BucketCreate( - entity=self.object, - user=self.request.user, - extra_data={ - "bucket_owner": self.object.bucket_owner, - } - ).create_task() - tasks.S3BucketGrantToUser( - entity=UserS3Bucket.objects.get( - s3bucket=self.object, user=self.request.user - ), - user=self.request.user, - ).create_task() - class DeleteDatasource( OIDCLoginRequiredMixin, diff --git a/tests/frontend/views/test_datasource.py b/tests/frontend/views/test_datasource.py index 4851663a5..1f43f02a0 100644 --- a/tests/frontend/views/test_datasource.py +++ b/tests/frontend/views/test_datasource.py @@ -274,7 +274,11 @@ def test_list_other_datasources_admins(client, buckets, users): assert other_datasources_admins[buckets["other"].id] == [] -def test_bucket_creator_has_readwrite_and_admin_access(client, users): +@patch("controlpanel.api.models.users3bucket.tasks.S3BucketGrantToUser") +@patch("controlpanel.api.models.s3bucket.tasks.S3BucketCreate") +def test_bucket_creator_has_readwrite_and_admin_access( + create_bucket_task, grant_user_task, client, users +): user = users["normal_user"] client.force_login(user) create(client) @@ -282,6 +286,8 @@ def test_bucket_creator_has_readwrite_and_admin_access(client, users): ub = user.users3buckets.all()[0] assert ub.access_level == UserS3Bucket.READWRITE assert ub.is_admin + create_bucket_task.assert_called_once() + grant_user_task.assert_called_once() @pytest.mark.parametrize( @@ -303,31 +309,44 @@ def test_create_get_form_class(rf, folders_enabled, datasource_type, form_class) assert view.get_form_class() == form_class +@patch("controlpanel.api.models.users3bucket.tasks.S3BucketGrantToUser") +@patch("controlpanel.api.models.s3bucket.tasks.S3BucketCreate") @patch("django.conf.settings.features.s3_folders.enabled", True) -def test_create_folders(client, users, root_folder_bucket): +@pytest.mark.parametrize("user", ["superuser", "normal_user", "other_user"]) +def test_create_folders( + create_bucket_task, grant_user_task, user, client, users, root_folder_bucket +): """ Check that all users can create a folder datasource """ - for _, user in users.items(): - client.force_login(user) - folder_name = f"test-{user.username}-folder" - response = create(client, name=folder_name) - - # redirect expected on success - assert response.status_code == 302 - assert user.users3buckets.filter( - s3bucket__name=f"{root_folder_bucket.name}/{folder_name}" - ).exists() - - # create another folder to catch any errors updating IAM policy - folder_name = f"test-{user.username}-folder-2" - response = create(client, name=folder_name) - - # redirect expected on success - assert response.status_code == 302 - assert user.users3buckets.filter( - s3bucket__name=f"{root_folder_bucket.name}/{folder_name}" - ).exists() + user = users[user] + client.force_login(user) + folder_name = f"test-{user.username}-folder" + response = create(client, name=folder_name) + + # redirect expected on success + assert response.status_code == 302 + assert user.users3buckets.filter( + s3bucket__name=f"{root_folder_bucket.name}/{folder_name}" + ).exists() + # make sure tasks are sent + create_bucket_task.assert_called_once() + grant_user_task.assert_called_once() + + # create another folder to catch any errors updating IAM policy + create_bucket_task.reset_mock() + grant_user_task.reset_mock() + folder_name = f"test-{user.username}-folder-2" + response = create(client, name=folder_name) + + # redirect expected on success + assert response.status_code == 302 + assert user.users3buckets.filter( + s3bucket__name=f"{root_folder_bucket.name}/{folder_name}" + ).exists() + # tasks to create are sent again for the second folder + create_bucket_task.assert_called_once() + grant_user_task.assert_called_once() @patch("django.conf.settings.features.s3_folders.enabled", False)