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

@@ -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

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()

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,

tests/frontend/views/test_datasource.py

@@ -274,14 +274,20 @@ 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)
     assert user.users3buckets.count() == 1
     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)