cleanup and comments

src/registrar/admin.py

@@ -1486,7 +1486,11 @@ class PortfolioInvitationAdmin(ListHeaderAdmin):
 
     def save_model(self, request, obj, form, change):
         """
-        Override the save_model method to send an email only on creation of the PortfolioInvitation object.
+        Override the save_model method.
+
+        Only send email on creation of the PortfolioInvitation object. Not on updates.
+        Emails sent to requested user / email.
+        When exceptions are raised, return without saving model.
         """
         if not change:  # Only send email if this is a new PortfolioInvitation(creation)
             portfolio = obj.portfolio
@@ -1499,19 +1503,23 @@ class PortfolioInvitationAdmin(ListHeaderAdmin):
             ).exists()
             try:
                 if not requested_user or not permission_exists:
+                    # if requested user does not exist or permission does not exist, send email
                     send_portfolio_invitation_email(email=requested_email, requestor=requestor, portfolio=portfolio)
                     messages.success(request, f"{requested_email} has been invited.")
-                else:
+                elif permission_exists:
-                    if permission_exists:
+                    messages.warning(request, "User is already a member of this portfolio.")
-                        messages.warning(request, "User is already a member of this portfolio.")
             except Exception as e:
+                # when exception is raised, handle and do not save the model
                 self._handle_exceptions(e, request, obj)
                 return
         # Call the parent save method to save the object
         super().save_model(request, obj, form, change)
 
     def _handle_exceptions(self, exception, request, obj):
-        """Handle exceptions raised during the process."""
+        """Handle exceptions raised during the process.
+
+        Log warnings / errors, and message errors to the user.
+        """
         if isinstance(exception, EmailSendingError):
             logger.warning(
                 "Could not sent email invitation to %s for portfolio %s (EmailSendingError)",
@@ -1534,7 +1542,10 @@ class PortfolioInvitationAdmin(ListHeaderAdmin):
 
     def response_add(self, request, obj, post_url_continue=None):
         """
-        Override response_add to handle redirection when exceptions are raised.
+        Override response_add to handle rendering when exceptions are raised during add model.
+
+        Normal flow on successful save_model on add is to redirect to changelist_view.
+        If there are errors, flow is modified to instead render change form.
         """
         # Check if there are any error or warning messages in the `messages` framework
         storage = get_messages(request)
@@ -1576,7 +1587,7 @@ class PortfolioInvitationAdmin(ListHeaderAdmin):
                 "obj": obj,
                 "adminform": admin_form,  # Pass the AdminForm instance
                 "media": media,
-                "errors": None,  # You can use this to pass custom form errors
+                "errors": None,
             }
             return self.render_change_form(
                 request,

src/registrar/forms/portfolio.py

@@ -113,6 +113,7 @@ class PortfolioSeniorOfficialForm(forms.ModelForm):
 class BasePortfolioMemberForm(forms.ModelForm):
     """Base form for the PortfolioMemberForm and PortfolioInvitedMemberForm"""
 
+    # The label for each of these has a red "required" star. We can just embed that here for simplicity.
     required_star = '<abbr class="usa-hint usa-hint--required" title="required">*</abbr>'
     role = forms.ChoiceField(
         choices=[
@@ -167,6 +168,9 @@ class BasePortfolioMemberForm(forms.ModelForm):
         },
     )
 
+    # Tracks what form elements are required for a given role choice.
+    # All of the fields included here have "required=False" by default as they are conditionally required.
+    # see def clean() for more details.
     ROLE_REQUIRED_FIELDS = {
         UserPortfolioRoleChoices.ORGANIZATION_ADMIN: [
             "domain_request_permission_admin",
@@ -183,10 +187,13 @@ class BasePortfolioMemberForm(forms.ModelForm):
 
     def __init__(self, *args, **kwargs):
         """
-        Override the form's initialization to map existing model values
+        Override the form's initialization.
-        to custom form fields.
+
+        Map existing model values to custom form fields.
+        Update field descriptions.
         """
         super().__init__(*args, **kwargs)
+        # Adds a <p> description beneath each role option
         self.fields["role"].descriptions = {
             "organization_admin": UserPortfolioRoleChoices.get_role_description(
                 UserPortfolioRoleChoices.ORGANIZATION_ADMIN
@@ -196,14 +203,14 @@ class BasePortfolioMemberForm(forms.ModelForm):
             ),
         }
         # Map model instance values to custom form fields
-        logger.info(self.instance)
-        logger.info(self.initial)
         if self.instance:
             self.map_instance_to_initial()
 
     def clean(self):
-        """Validates form data based on selected role and its required fields."""
+        """Validates form data based on selected role and its required fields.
-        logger.info("clean")
+        Updates roles and additional_permissions in cleaned_data so they can be properly
+        mapped to the model.
+        """
         cleaned_data = super().clean()
         role = cleaned_data.get("role")
 
@@ -239,13 +246,12 @@ class BasePortfolioMemberForm(forms.ModelForm):
         role_permissions = UserPortfolioPermission.get_portfolio_permissions(cleaned_data["roles"], [], get_list=False)
         cleaned_data["additional_permissions"] = list(additional_permissions - role_permissions)
 
-        logger.info(cleaned_data)
         return cleaned_data
 
     def map_instance_to_initial(self):
         """
         Maps self.instance to self.initial, handling roles and permissions.
-        Returns form data dictionary with appropriate permission levels based on user role:
+        Updates self.initial dictionary with appropriate permission levels based on user role:
         {
             "role": "organization_admin" or "organization_member",
             "member_permission_admin": permission level if admin,
@@ -253,10 +259,9 @@ class BasePortfolioMemberForm(forms.ModelForm):
             "domain_request_permission_member": permission level if member
         }
         """
-        logger.info(self.instance)
-        # Function variables
         if self.initial is None:
             self.initial = {}
+        # Function variables
         perms = UserPortfolioPermission.get_portfolio_permissions(
             self.instance.roles, self.instance.additional_permissions, get_list=False
         )
@@ -290,7 +295,6 @@ class BasePortfolioMemberForm(forms.ModelForm):
             # Edgecase: Member uses a special form value for None called "no_access". This ensures a form selection.
             selected_domain_permission = next((perm for perm in domain_perms if perm in perms), "no_access")
             self.initial["domain_request_permission_member"] = selected_domain_permission
-        logger.info(self.initial)
 
 
 class PortfolioMemberForm(BasePortfolioMemberForm):
@@ -314,6 +318,9 @@ class PortfolioInvitedMemberForm(BasePortfolioMemberForm):
 
 
 class PortfolioNewMemberForm(BasePortfolioMemberForm):
+    """
+    Form for adding a portfolio invited member.
+    """
 
     email = forms.EmailField(
         label="Enter the email of the member you'd like to invite",

src/registrar/models/portfolio.py

@@ -6,9 +6,6 @@ from registrar.models.user import User
 from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices
 
 from .utility.time_stamped_model import TimeStampedModel
-import logging
-
-logger = logging.getLogger(__name__)
 
 
 class Portfolio(TimeStampedModel):
@@ -166,7 +163,3 @@ class Portfolio(TimeStampedModel):
     def get_suborganizations(self):
         """Returns all suborganizations associated with this portfolio"""
         return self.portfolio_suborganizations.all()
-
-    def full_clean(self, exclude=None, validate_unique=True):
-        logger.info("portfolio full clean")
-        super().full_clean(exclude, validate_unique)