Merge pull request #3430 from cisagov/za/3145-use-transaction-atomic

#3145: Change usage of transaction.atomic() to be inside try/catch instead of outside of it [litterbox]

src/registrar/fixtures/fixtures_domains.py

@@ -3,7 +3,6 @@ from django.utils import timezone
 import logging
 import random
 from faker import Faker
-from django.db import transaction
 
 from registrar.fixtures.fixtures_requests import DomainRequestFixture
 from registrar.fixtures.fixtures_users import UserFixture
@@ -29,19 +28,18 @@ class DomainFixture(DomainRequestFixture):
     def load(cls):
         # Lumped under .atomic to ensure we don't make redundant DB calls.
         # This bundles them all together, and then saves it in a single call.
-        with transaction.atomic():
-            try:
-                # Get the usernames of users created in the UserFixture
-                created_usernames = [user_data["username"] for user_data in UserFixture.ADMINS + UserFixture.STAFF]
+        try:
+            # Get the usernames of users created in the UserFixture
+            created_usernames = [user_data["username"] for user_data in UserFixture.ADMINS + UserFixture.STAFF]
 
-                # Filter users to only include those created by the fixture
-                users = list(User.objects.filter(username__in=created_usernames))
-            except Exception as e:
-                logger.warning(e)
-                return
+            # Filter users to only include those created by the fixture
+            users = list(User.objects.filter(username__in=created_usernames))
+        except Exception as e:
+            logger.warning(e)
+            return
 
-            # Approve each user associated with `in review` status domains
-            cls._approve_domain_requests(users)
+        # Approve each user associated with `in review` status domains
+        cls._approve_domain_requests(users)
 
     @staticmethod
     def _generate_fake_expiration_date(days_in_future=365):

src/registrar/fixtures/fixtures_portfolios.py

@@ -1,7 +1,6 @@
 import logging
 import random
 from faker import Faker
-from django.db import transaction
 
 from registrar.models import User, DomainRequest, FederalAgency
 from registrar.models.portfolio import Portfolio
@@ -84,42 +83,38 @@ class PortfolioFixture:
     def load(cls):
         """Creates portfolios."""
         logger.info("Going to load %s portfolios" % len(cls.PORTFOLIOS))
+        try:
+            user = User.objects.all().last()
+        except Exception as e:
+            logger.warning(e)
+            return
 
-        # Lumped under .atomic to ensure we don't make redundant DB calls.
-        # This bundles them all together, and then saves it in a single call.
-        with transaction.atomic():
+        portfolios_to_create = []
+        for portfolio_data in cls.PORTFOLIOS:
+            organization_name = portfolio_data["organization_name"]
+
+            # Check if portfolio with the organization name already exists
+            if Portfolio.objects.filter(organization_name=organization_name).exists():
+                logger.info(
+                    f"Portfolio with organization name '{organization_name}' already exists, skipping creation."
+                )
+                continue
+
+        try:
+            portfolio = Portfolio(
+                creator=user,
+                organization_name=portfolio_data["organization_name"],
+            )
+            cls._set_non_foreign_key_fields(portfolio, portfolio_data)
+            cls._set_foreign_key_fields(portfolio, portfolio_data, user)
+            portfolios_to_create.append(portfolio)
+        except Exception as e:
+            logger.warning(e)
+
+        # Bulk create portfolios
+        if len(portfolios_to_create) > 0:
             try:
-                user = User.objects.all().last()
+                Portfolio.objects.bulk_create(portfolios_to_create)
+                logger.info(f"Successfully created {len(portfolios_to_create)} portfolios")
             except Exception as e:
-                logger.warning(e)
-                return
-
-            portfolios_to_create = []
-            for portfolio_data in cls.PORTFOLIOS:
-                organization_name = portfolio_data["organization_name"]
-
-                # Check if portfolio with the organization name already exists
-                if Portfolio.objects.filter(organization_name=organization_name).exists():
-                    logger.info(
-                        f"Portfolio with organization name '{organization_name}' already exists, skipping creation."
-                    )
-                    continue
-
-                try:
-                    portfolio = Portfolio(
-                        creator=user,
-                        organization_name=portfolio_data["organization_name"],
-                    )
-                    cls._set_non_foreign_key_fields(portfolio, portfolio_data)
-                    cls._set_foreign_key_fields(portfolio, portfolio_data, user)
-                    portfolios_to_create.append(portfolio)
-                except Exception as e:
-                    logger.warning(e)
-
-            # Bulk create domain requests
-            if len(portfolios_to_create) > 0:
-                try:
-                    Portfolio.objects.bulk_create(portfolios_to_create)
-                    logger.info(f"Successfully created {len(portfolios_to_create)} portfolios")
-                except Exception as e:
-                    logger.warning(f"Error bulk creating portfolios: {e}")
+                logger.warning(f"Error bulk creating portfolios: {e}")

src/registrar/fixtures/fixtures_requests.py

@@ -3,7 +3,6 @@ from django.utils import timezone
 import logging
 import random
 from faker import Faker
-from django.db import transaction
 
 from registrar.fixtures.fixtures_portfolios import PortfolioFixture
 from registrar.fixtures.fixtures_suborganizations import SuborganizationFixture
@@ -303,24 +302,17 @@ class DomainRequestFixture:
     def load(cls):
         """Creates domain requests for each user in the database."""
         logger.info("Going to load %s domain requests" % len(cls.DOMAINREQUESTS))
+        try:
+            # Get the usernames of users created in the UserFixture
+            created_usernames = [user_data["username"] for user_data in UserFixture.ADMINS + UserFixture.STAFF]
 
-        # Lumped under .atomic to ensure we don't make redundant DB calls.
-        # This bundles them all together, and then saves it in a single call.
-        # The atomic block will cause the code to stop executing if one instance in the
-        # nested iteration fails, which will cause an early exit and make it hard to debug.
-        # Comment out with transaction.atomic() when debugging.
-        with transaction.atomic():
-            try:
-                # Get the usernames of users created in the UserFixture
-                created_usernames = [user_data["username"] for user_data in UserFixture.ADMINS + UserFixture.STAFF]
+            # Filter users to only include those created by the fixture
+            users = list(User.objects.filter(username__in=created_usernames))
+        except Exception as e:
+            logger.warning(e)
+            return
 
-                # Filter users to only include those created by the fixture
-                users = list(User.objects.filter(username__in=created_usernames))
-            except Exception as e:
-                logger.warning(e)
-                return
-
-            cls._create_domain_requests(users)
+        cls._create_domain_requests(users)
 
     @classmethod
     def _create_domain_requests(cls, users):  # noqa: C901

src/registrar/fixtures/fixtures_suborganizations.py

@@ -1,6 +1,5 @@
 import logging
 from faker import Faker
-from django.db import transaction
 
 from registrar.models.portfolio import Portfolio
 from registrar.models.suborganization import Suborganization
@@ -34,14 +33,12 @@ class SuborganizationFixture:
     def load(cls):
         """Creates suborganizations."""
         logger.info(f"Going to load {len(cls.SUBORGS)} suborgs")
+        portfolios = cls._get_portfolios()
+        if not portfolios:
+            return
 
-        with transaction.atomic():
-            portfolios = cls._get_portfolios()
-            if not portfolios:
-                return
-
-            suborgs_to_create = cls._prepare_suborgs_to_create(portfolios)
-            cls._bulk_create_suborgs(suborgs_to_create)
+        suborgs_to_create = cls._prepare_suborgs_to_create(portfolios)
+        cls._bulk_create_suborgs(suborgs_to_create)
 
     @classmethod
     def _get_portfolios(cls):

src/registrar/fixtures/fixtures_user_portfolio_permissions.py

@@ -1,7 +1,6 @@
 import logging
 import random
 from faker import Faker
-from django.db import transaction
 
 from registrar.fixtures.fixtures_portfolios import PortfolioFixture
 from registrar.fixtures.fixtures_users import UserFixture
@@ -26,56 +25,55 @@ class UserPortfolioPermissionFixture:
 
         # Lumped under .atomic to ensure we don't make redundant DB calls.
         # This bundles them all together, and then saves it in a single call.
-        with transaction.atomic():
-            try:
-                # Get the usernames of users created in the UserFixture
-                created_usernames = [user_data["username"] for user_data in UserFixture.ADMINS + UserFixture.STAFF]
+        try:
+            # Get the usernames of users created in the UserFixture
+            created_usernames = [user_data["username"] for user_data in UserFixture.ADMINS + UserFixture.STAFF]
 
-                # Filter users to only include those created by the fixture
-                users = list(User.objects.filter(username__in=created_usernames))
+            # Filter users to only include those created by the fixture
+            users = list(User.objects.filter(username__in=created_usernames))
 
-                organization_names = [portfolio["organization_name"] for portfolio in PortfolioFixture.PORTFOLIOS]
+            organization_names = [portfolio["organization_name"] for portfolio in PortfolioFixture.PORTFOLIOS]
 
-                portfolios = list(Portfolio.objects.filter(organization_name__in=organization_names))
+            portfolios = list(Portfolio.objects.filter(organization_name__in=organization_names))
 
-                if not users:
-                    logger.warning("User fixtures missing.")
-                    return
-
-                if not portfolios:
-                    logger.warning("Portfolio fixtures missing.")
-                    return
-
-            except Exception as e:
-                logger.warning(e)
+            if not users:
+                logger.warning("User fixtures missing.")
                 return
 
-            user_portfolio_permissions_to_create = []
-            for user in users:
-                # Assign a random portfolio to a user
-                portfolio = random.choice(portfolios)  # nosec
-                try:
-                    if not UserPortfolioPermission.objects.filter(user=user, portfolio=portfolio).exists():
-                        user_portfolio_permission = UserPortfolioPermission(
-                            user=user,
-                            portfolio=portfolio,
-                            roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
-                            additional_permissions=[
-                                UserPortfolioPermissionChoices.EDIT_MEMBERS,
-                                UserPortfolioPermissionChoices.EDIT_REQUESTS,
-                            ],
-                        )
-                        user_portfolio_permissions_to_create.append(user_portfolio_permission)
-                    else:
-                        logger.info(
-                            f"Permission exists for user '{user.username}' "
-                            f"on portfolio '{portfolio.organization_name}'."
-                        )
-                except Exception as e:
-                    logger.warning(e)
+            if not portfolios:
+                logger.warning("Portfolio fixtures missing.")
+                return
 
-            # Bulk create permissions
-            cls._bulk_create_permissions(user_portfolio_permissions_to_create)
+        except Exception as e:
+            logger.warning(e)
+            return
+
+        user_portfolio_permissions_to_create = []
+        for user in users:
+            # Assign a random portfolio to a user
+            portfolio = random.choice(portfolios)  # nosec
+            try:
+                if not UserPortfolioPermission.objects.filter(user=user, portfolio=portfolio).exists():
+                    user_portfolio_permission = UserPortfolioPermission(
+                        user=user,
+                        portfolio=portfolio,
+                        roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN],
+                        additional_permissions=[
+                            UserPortfolioPermissionChoices.EDIT_MEMBERS,
+                            UserPortfolioPermissionChoices.EDIT_REQUESTS,
+                        ],
+                    )
+                    user_portfolio_permissions_to_create.append(user_portfolio_permission)
+                else:
+                    logger.info(
+                        f"Permission exists for user '{user.username}' "
+                        f"on portfolio '{portfolio.organization_name}'."
+                    )
+            except Exception as e:
+                logger.warning(e)
+
+        # Bulk create permissions
+        cls._bulk_create_permissions(user_portfolio_permissions_to_create)
 
     @classmethod
     def _bulk_create_permissions(cls, user_portfolio_permissions_to_create):

src/registrar/fixtures/fixtures_users.py

@@ -1,6 +1,5 @@
 import logging
 from faker import Faker
-from django.db import transaction
 
 from registrar.models import (
     User,
@@ -455,10 +454,9 @@ class UserFixture:
 
     @classmethod
     def load(cls):
-        with transaction.atomic():
-            cls.load_users(cls.ADMINS, "full_access_group", are_superusers=True)
-            cls.load_users(cls.STAFF, "cisa_analysts_group")
+        cls.load_users(cls.ADMINS, "full_access_group", are_superusers=True)
+        cls.load_users(cls.STAFF, "cisa_analysts_group")
 
-            # Combine ADMINS and STAFF lists
-            all_users = cls.ADMINS + cls.STAFF
-            cls.load_allowed_emails(cls, all_users, additional_emails=cls.ADDITIONAL_ALLOWED_EMAILS)
+        # Combine ADMINS and STAFF lists
+        all_users = cls.ADMINS + cls.STAFF
+        cls.load_allowed_emails(cls, all_users, additional_emails=cls.ADDITIONAL_ALLOWED_EMAILS)

src/registrar/management/commands/remove_unused_portfolios.py

@@ -149,9 +149,9 @@ class Command(BaseCommand):
                 )
                 return
 
-        with transaction.atomic():
-            # Try to delete the portfolios
-            try:
+        # Try to delete the portfolios
+        try:
+            with transaction.atomic():
                 summary = []
                 for portfolio in portfolios_to_delete:
                     portfolio_summary = [f"---- CASCADE SUMMARY for {portfolio.organization_name} -----"]
@@ -222,14 +222,14 @@ class Command(BaseCommand):
                     """
                 )
 
-            except IntegrityError as e:
-                logger.info(
-                    f"""{TerminalColors.FAIL}
-                    Could not delete some portfolios due to integrity constraints:
-                    {e}
-                    {TerminalColors.ENDC}
-                    """
-                )
+        except IntegrityError as e:
+            logger.info(
+                f"""{TerminalColors.FAIL}
+                Could not delete some portfolios due to integrity constraints:
+                {e}
+                {TerminalColors.ENDC}
+                """
+            )
 
     def handle(self, *args, **options):
         # Get all Portfolio entries not in the allowed portfolios list