Code cleanup

src/registrar/models/__init__.py

@@ -10,16 +10,17 @@ from .host import Host
 from .domain_invitation import DomainInvitation
 from .user_domain_role import UserDomainRole
 from .public_contact import PublicContact
+
+# IMPORTANT: UserPortfolioPermission must be before PortfolioInvitation.
+# PortfolioInvitation imports from UserPortfolioPermission, so you will get a circular import otherwise.
+from .user_portfolio_permission import UserPortfolioPermission
+from .portfolio_invitation import PortfolioInvitation
 from .user import User
 from .user_group import UserGroup
 from .website import Website
 from .transition_domain import TransitionDomain
 from .verified_by_staff import VerifiedByStaff
 from .waffle_flag import WaffleFlag
-# IMPORTANT: UserPortfolioPermission must be before PortfolioInvitation.
-# PortfolioInvitation imports from UserPortfolioPermission, so you will get a circular import otherwise.
-from .user_portfolio_permission import UserPortfolioPermission
-from .portfolio_invitation import PortfolioInvitation
 from .portfolio import Portfolio
 from .domain_group import DomainGroup
 from .suborganization import Suborganization

src/registrar/models/portfolio_invitation.py

@@ -3,13 +3,11 @@
 import logging
 from django.db import models
 from django_fsm import FSMField, transition
-from registrar.models import DomainInvitation, UserPortfolioPermission, User
+from django.contrib.auth import get_user_model
-from registrar.utility.waffle import flag_is_active_for_user
+from registrar.models import DomainInvitation, UserPortfolioPermission
-from .utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices  # type: ignore
+from .utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices, validate_portfolio_invitation  # type: ignore
 from .utility.time_stamped_model import TimeStampedModel
 from django.contrib.postgres.fields import ArrayField
-from django.forms import ValidationError
-
 
 logger = logging.getLogger(__name__)
 
@@ -80,7 +78,6 @@ class PortfolioInvitation(TimeStampedModel):
         """
         Retrieve the permissions for the user's portfolio roles from the invite.
         """
-        # UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission")
         return UserPortfolioPermission.get_portfolio_permissions(self.roles, self.additional_permissions)
 
     @transition(field="status", source=PortfolioInvitationStatus.INVITED, target=PortfolioInvitationStatus.RETRIEVED)
@@ -90,9 +87,9 @@ class PortfolioInvitation(TimeStampedModel):
         Raises:
             RuntimeError if no matching user can be found.
         """
+        User = get_user_model()
 
         # get a user with this email address
-        # UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission")
         try:
             user = User.objects.get(email=self.email)
         except User.DoesNotExist:
@@ -113,47 +110,4 @@ class PortfolioInvitation(TimeStampedModel):
     def clean(self):
         """Extends clean method to perform additional validation, which can raise errors in django admin."""
         super().clean()
-
+        validate_portfolio_invitation(self)
-        is_admin = UserPortfolioRoleChoices.ORGANIZATION_ADMIN in self.roles
-        all_permissions = self.get_portfolio_permissions()
-        if not is_admin:
-            # Question: should we also check the edit perms?
-            # TODO - need to display multiple validation errors at once
-            if UserPortfolioPermissionChoices.VIEW_MEMBERS in all_permissions:
-                raise ValidationError("View members cannot be assigned to non-admin roles.")
-            
-            if UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS:
-                raise ValidationError("View all domains cannot be assigned to non-admin roles.")
-
-        # UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission")
-        roles = self.roles if self.roles is not None else []
-        bad_perms = UserPortfolioPermission.get_forbidden_permissions(self.roles, self.additional_permissions)
-        if bad_perms:
-            readable_perms = [
-                UserPortfolioPermissionChoices.get_user_portfolio_permission_label(perm) 
-                for perm in bad_perms
-            ]
-            readable_roles = [
-                UserPortfolioRoleChoices.get_user_portfolio_role_label(role)
-                for role in roles
-            ]
-            raise ValidationError(
-                f"These permissions cannot be assigned to {', '.join(readable_roles)}: <{', '.join(readable_perms)}>"
-            )
-
-        user = User.objects.filter(email=self.email).first()
-        if not flag_is_active_for_user(user, "multiple_portfolios"):
-            # TODO - need to display multiple validation errors
-            existing_permissions = UserPortfolioPermission.objects.filter(user=user)
-            existing_invitations = PortfolioInvitation.objects.exclude(id=self.id).filter(email=user.email)
-            if existing_permissions.exists():
-                raise ValidationError(
-                    "This user is already assigned to a portfolio. "
-                    "Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
-                )
-            
-            if existing_invitations.exists():
-                raise ValidationError(
-                    "This user is already assigned to a portfolio invitation. "
-                    "Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
-                )

src/registrar/models/user.py

@@ -1,11 +1,10 @@
 import logging
 
-from django.apps import apps
 from django.contrib.auth.models import AbstractUser
 from django.db import models
 from django.db.models import Q
 
-from registrar.models import DomainInformation, UserDomainRole
+from registrar.models import DomainInformation, UserDomainRole, PortfolioInvitation, UserPortfolioPermission
 from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices
 
 from .domain_invitation import DomainInvitation
@@ -419,7 +418,6 @@ class User(AbstractUser):
     def check_portfolio_invitations_on_login(self):
         """When a user first arrives on the site, we need to retrieve any portfolio
         invitations that match their email address."""
-        PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation")
         for invitation in PortfolioInvitation.objects.filter(
             email__iexact=self.email, status=PortfolioInvitation.PortfolioInvitationStatus.INVITED
         ):
@@ -498,8 +496,6 @@ class User(AbstractUser):
     def is_only_admin_of_portfolio(self, portfolio):
         """Check if the user is the only admin of the given portfolio."""
 
-        UserPortfolioPermission = apps.get_model("registrar", "UserPortfolioPermission")
-
         admin_permission = UserPortfolioRoleChoices.ORGANIZATION_ADMIN
 
         admins = UserPortfolioPermission.objects.filter(portfolio=portfolio, roles__contains=[admin_permission])

src/registrar/models/user_portfolio_permission.py

@@ -1,11 +1,14 @@
 from django.db import models
 from django.forms import ValidationError
-from registrar.models import UserDomainRole
 from registrar.utility.waffle import flag_is_active_for_user
-from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices
+from registrar.models import UserDomainRole
+from registrar.models.utility.portfolio_helper import (
+    UserPortfolioPermissionChoices,
+    UserPortfolioRoleChoices,
+    validate_user_portfolio_permission,
+)
 from .utility.time_stamped_model import TimeStampedModel
 from django.contrib.postgres.fields import ArrayField
-from django.apps import apps
 
 
 class UserPortfolioPermission(TimeStampedModel):
@@ -129,10 +132,9 @@ class UserPortfolioPermission(TimeStampedModel):
         # so called "forbidden" ones. But just member on their own cannot.
         # The solution to this is to only grab what is only COMMONLY "forbidden".
         # This will scale if we add more roles in the future.
-        common_forbidden_perms = set.intersection(*(
+        common_forbidden_perms = set.intersection(
-            set(cls.FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS.get(role, []))
+            *(set(cls.FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS.get(role, [])) for role in roles)
-            for role in roles
+        )
-        ))
 
         # Check if the users current permissions overlap with any forbidden permissions
         bad_perms = portfolio_permissions & common_forbidden_perms
@@ -141,60 +143,4 @@ class UserPortfolioPermission(TimeStampedModel):
     def clean(self):
         """Extends clean method to perform additional validation, which can raise errors in django admin."""
         super().clean()
-
+        validate_user_portfolio_permission(self)
-        # Check if portfolio is set without accessing the related object.
-        has_portfolio = bool(self.portfolio_id)
-        portfolio_permissions = set(self._get_portfolio_permissions())
-
-        # == Validate required fields == #
-        if not has_portfolio and portfolio_permissions:
-            raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.")
-
-        if has_portfolio and not portfolio_permissions:
-            raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.")
-
-        # == Validate role permissions. Compares existing permissions to forbidden ones. == #
-        roles = self.roles if self.roles is not None else []
-        bad_perms = self.get_forbidden_permissions(self.roles, self.additional_permissions)
-        if bad_perms:
-            readable_perms = [
-                UserPortfolioPermissionChoices.get_user_portfolio_permission_label(perm) 
-                for perm in bad_perms
-            ]
-            readable_roles = [
-                UserPortfolioRoleChoices.get_user_portfolio_role_label(role)
-                for role in roles
-            ]
-            raise ValidationError(
-                f"These permissions cannot be assigned to {', '.join(readable_roles)}: <{', '.join(readable_perms)}>"
-            )
-        # NOTE: if the user has two conflicting roles, like ["admin", "member"],
-        # then this validation will still fail if a role is found that conflicts with member.
-        # Question for reviewers: thoughts? 
-        # for role in roles:
-        #     bad_perms = portfolio_permissions & set(self.FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS.get(role, []))
-        #     if bad_perms:
-        #         readable_role = UserPortfolioRoleChoices.get_user_portfolio_role_label(role)
-        #         invalid = [UserPortfolioPermissionChoices.get_user_portfolio_permission_label(perm) for perm in bad_perms]
-        #         raise ValidationError(
-        #             f"These permissions cannot be assigned to {readable_role}: {', '.join(invalid)}"
-        #         )
-
-        # == Validate that only one permission exists when multiple_portfolios is disabled == #
-        # Check if a user is set without accessing the related object.
-        PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation")
-        existing_permissions = UserPortfolioPermission.objects.exclude(id=self.id).filter(user=self.user)
-        existing_invitations = PortfolioInvitation.objects.filter(email=self.user.email)
-        if not flag_is_active_for_user(self.user, "multiple_portfolios"):
-            # TODO - need to display multiple validation errors
-            if existing_permissions.exists():
-                raise ValidationError(
-                    "This user is already assigned to a portfolio. "
-                    "Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
-                )
-            
-            if existing_invitations.exists():
-                raise ValidationError(
-                    "This user is already assigned to a portfolio invitation. "
-                    "Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
-                )

src/registrar/models/utility/portfolio_helper.py

@@ -1,4 +1,8 @@
 from django.db import models
+from django.apps import apps
+from django.forms import ValidationError
+from registrar.utility.waffle import flag_is_active_for_user
+from django.contrib.auth import get_user_model
 
 
 class UserPortfolioRoleChoices(models.TextChoices):
@@ -40,3 +44,130 @@ class UserPortfolioPermissionChoices(models.TextChoices):
     @classmethod
     def to_dict(cls):
         return {key: value.value for key, value in cls.__members__.items()}
+
+
+def validate_user_portfolio_permission(user_portfolio_permission):
+    """
+    Validates a UserPortfolioPermission instance. Located in portfolio_helper to avoid circular imports
+    between PortfolioInvitation and UserPortfolioPermission models.
+
+    Used in UserPortfolioPermission.clean() for model validation.
+
+    Validates:
+    1. A portfolio must be assigned if roles or additional permissions are specified, and vice versa.
+    2. Assigned roles do not include any forbidden permissions.
+    3. If the 'multiple_portfolios' flag is inactive for the user,
+    they must not have existing portfolio permissions or invitations.
+
+    Raises:
+        ValidationError: If any of the validation rules are violated.
+    """
+    PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation")
+    UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission")
+
+    has_portfolio = bool(user_portfolio_permission.portfolio_id)
+    portfolio_permissions = set(user_portfolio_permission._get_portfolio_permissions())
+
+    # == Validate required fields == #
+    if not has_portfolio and portfolio_permissions:
+        raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.")
+
+    if has_portfolio and not portfolio_permissions:
+        raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.")
+
+    # == Validate role permissions. Compares existing permissions to forbidden ones. == #
+    roles = user_portfolio_permission.roles if user_portfolio_permission.roles is not None else []
+    bad_perms = user_portfolio_permission.get_forbidden_permissions(
+        user_portfolio_permission.roles, user_portfolio_permission.additional_permissions
+    )
+    if bad_perms:
+        readable_perms = [
+            UserPortfolioPermissionChoices.get_user_portfolio_permission_label(perm) for perm in bad_perms
+        ]
+        readable_roles = [UserPortfolioRoleChoices.get_user_portfolio_role_label(role) for role in roles]
+        raise ValidationError(
+            f"These permissions cannot be assigned to {', '.join(readable_roles)}: <{', '.join(readable_perms)}>"
+        )
+
+    if not flag_is_active_for_user(user_portfolio_permission.user, "multiple_portfolios"):
+        existing_permissions = UserPortfolioPermission.objects.exclude(id=user_portfolio_permission.id).filter(
+            user=user_portfolio_permission.user
+        )
+
+        existing_invitations = PortfolioInvitation.objects.filter(email=user_portfolio_permission.user.email)
+
+        if existing_permissions.exists():
+            raise ValidationError(
+                "This user is already assigned to a portfolio. "
+                "Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
+            )
+
+        if existing_invitations.exists():
+            raise ValidationError(
+                "This user is already assigned to a portfolio invitation. "
+                "Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
+            )
+
+
+def validate_portfolio_invitation(portfolio_invitation):
+    """
+    Validates a PortfolioInvitation instance. Located in portfolio_helper to avoid circular imports
+    between PortfolioInvitation and UserPortfolioPermission models.
+
+    Used in PortfolioInvitation.clean() for model validation.
+
+    Validates:
+    1. A portfolio must be assigned if roles or additional permissions are specified, and vice versa.
+    2. Assigned roles do not include any forbidden permissions.
+    3. If the 'multiple_portfolios' flag is inactive for the user,
+    they must not have existing portfolio permissions or invitations.
+
+    Raises:
+        ValidationError: If any of the validation rules are violated.
+    """
+    PortfolioInvitation = apps.get_model("registrar.PortfolioInvitation")
+    UserPortfolioPermission = apps.get_model("registrar.UserPortfolioPermission")
+    User = get_user_model()
+
+    has_portfolio = bool(portfolio_invitation.portfolio_id)
+    portfolio_permissions = set(portfolio_invitation.get_portfolio_permissions())
+
+    # == Validate required fields == #
+    if not has_portfolio and portfolio_permissions:
+        raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.")
+
+    if has_portfolio and not portfolio_permissions:
+        raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.")
+
+    roles = portfolio_invitation.roles if portfolio_invitation.roles is not None else []
+    bad_perms = UserPortfolioPermission.get_forbidden_permissions(
+        portfolio_invitation.roles, portfolio_invitation.additional_permissions
+    )
+    if bad_perms:
+        readable_perms = [
+            UserPortfolioPermissionChoices.get_user_portfolio_permission_label(perm) for perm in bad_perms
+        ]
+        readable_roles = [UserPortfolioRoleChoices.get_user_portfolio_role_label(role) for role in roles]
+        raise ValidationError(
+            f"These permissions cannot be assigned to {', '.join(readable_roles)}: <{', '.join(readable_perms)}>"
+        )
+
+    user = User.objects.filter(email=portfolio_invitation.email).first()
+    if not flag_is_active_for_user(user, "multiple_portfolios"):
+        existing_permissions = UserPortfolioPermission.objects.filter(user=user)
+
+        existing_invitations = PortfolioInvitation.objects.exclude(id=portfolio_invitation.id).filter(
+            email=portfolio_invitation.email
+        )
+
+        if existing_permissions.exists():
+            raise ValidationError(
+                "This user is already assigned to a portfolio. "
+                "Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
+            )
+
+        if existing_invitations.exists():
+            raise ValidationError(
+                "This user is already assigned to a portfolio invitation. "
+                "Based on current waffle flag settings, users cannot be assigned to multiple portfolios."
+            )