diff --git a/src/registrar/admin.py b/src/registrar/admin.py
index 2707a1dff..168c27c01 100644
--- a/src/registrar/admin.py
+++ b/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:
- if permission_exists:
- messages.warning(request, "User is already a member of this portfolio.")
+ elif permission_exists:
+ 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,
diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py
index b055985d1..0a8c4d623 100644
--- a/src/registrar/forms/portfolio.py
+++ b/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 = '*'
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
- to custom form fields.
+ Override the form's initialization.
+
+ Map existing model values to custom form fields.
+ Update field descriptions.
"""
super().__init__(*args, **kwargs)
+ # Adds a 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."""
- logger.info("clean")
+ """Validates form data based on selected role and its required fields.
+ 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",
diff --git a/src/registrar/models/portfolio.py b/src/registrar/models/portfolio.py
index e7730230e..82afcd4d6 100644
--- a/src/registrar/models/portfolio.py
+++ b/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)