diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index a52d20a12..4d99ebd80 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -418,18 +418,15 @@ class PortfolioMemberForm(BasePortfolioMemberForm): """ super().clean() role = self.cleaned_data.get("role") - if role and self.instance.user.is_only_admin_of_portfolio(self.instance.portfolio): - # This is how you associate a validation error to a particular field. - # The alternative is to do this in clean_role, but execution order matters. - raise forms.ValidationError( - { - "role": forms.ValidationError( - "You can't change your member access because you're " - "the only admin for this organization. " - "To change your access, you'll need to add another admin." - ) - } - ) + if self.instance and hasattr(self.instance, "user") and hasattr(self.instance, "portfolio"): + if role and self.instance.user.is_only_admin_of_portfolio(self.instance.portfolio): + # This is how you associate a validation error to a particular field. + # The alternative is to do this in clean_role, but execution order matters. + raise forms.ValidationError({"role": forms.ValidationError( + "You can't change your member access because you're " + "the only admin for this organization. " + "To change your access, you'll need to add another admin." + )}) class PortfolioInvitedMemberForm(BasePortfolioMemberForm): diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index d5476ab9a..c59ab376b 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -474,7 +474,7 @@ class User(AbstractUser): admin_count = admins.count() # Check if the current user is in the list of admins - if admin_count == 1 and admins.first().user == self: + if admin_count == 1 and admins.first() and admins.first().user == self: return True # The user is the only admin # If there are other admins or the user is not the only one diff --git a/src/registrar/tests/test_forms.py b/src/registrar/tests/test_forms.py index 1c8b127ae..6e4b6adae 100644 --- a/src/registrar/tests/test_forms.py +++ b/src/registrar/tests/test_forms.py @@ -477,8 +477,8 @@ class TestBasePortfolioMemberForms(TestCase): self.assertTrue(form.is_valid(), f"Form {form_class.__name__} failed validation with data: {data}") return form - def _assert_form_has_error(self, form_class, data, field_name): - form = form_class(data=data) + def _assert_form_has_error(self, form_class, data, field_name, instance=None): + form = form_class(data=data, instance=instance) self.assertFalse(form.is_valid()) self.assertIn(field_name, form.errors) @@ -504,17 +504,23 @@ class TestBasePortfolioMemberForms(TestCase): "domain_permissions": "", # Simulate missing field "member_permissions": "", # Simulate missing field } + user_portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=self.portfolio, user=self.user + ) + portfolio_invitation, _ = PortfolioInvitation.objects.get_or_create(portfolio=self.portfolio, email="hi@ho") # Check required fields for all forms - self._assert_form_has_error(PortfolioMemberForm, data, "domain_request_permissions") - self._assert_form_has_error(PortfolioMemberForm, data, "domain_permissions") - self._assert_form_has_error(PortfolioMemberForm, data, "member_permissions") - self._assert_form_has_error(PortfolioInvitedMemberForm, data, "domain_request_permissions") - self._assert_form_has_error(PortfolioInvitedMemberForm, data, "domain_permissions") - self._assert_form_has_error(PortfolioInvitedMemberForm, data, "member_permissions") - self._assert_form_has_error(PortfolioNewMemberForm, data, "domain_request_permissions") - self._assert_form_has_error(PortfolioNewMemberForm, data, "domain_permissions") - self._assert_form_has_error(PortfolioNewMemberForm, data, "member_permissions") + self._assert_form_has_error(PortfolioMemberForm, data, "domain_request_permissions", user_portfolio_permission) + self._assert_form_has_error(PortfolioMemberForm, data, "domain_permissions", user_portfolio_permission) + self._assert_form_has_error(PortfolioMemberForm, data, "member_permissions", user_portfolio_permission) + self._assert_form_has_error( + PortfolioInvitedMemberForm, data, "domain_request_permissions", portfolio_invitation + ) + self._assert_form_has_error(PortfolioInvitedMemberForm, data, "domain_permissions", portfolio_invitation) + self._assert_form_has_error(PortfolioInvitedMemberForm, data, "member_permissions", portfolio_invitation) + self._assert_form_has_error(PortfolioNewMemberForm, data, "domain_request_permissions", portfolio_invitation) + self._assert_form_has_error(PortfolioNewMemberForm, data, "domain_permissions", portfolio_invitation) + self._assert_form_has_error(PortfolioNewMemberForm, data, "member_permissions", portfolio_invitation) @less_console_noise_decorator def test_clean_validates_required_fields_for_admin_role(self): @@ -529,7 +535,6 @@ class TestBasePortfolioMemberForms(TestCase): portfolio=self.portfolio, user=self.user ) portfolio_invitation, _ = PortfolioInvitation.objects.get_or_create(portfolio=self.portfolio, email="hi@ho") - data = { "role": UserPortfolioRoleChoices.ORGANIZATION_ADMIN.value, } @@ -677,6 +682,7 @@ class TestBasePortfolioMemberForms(TestCase): @less_console_noise_decorator def test_invalid_data_for_member(self): """Test invalid form submission for a member role with missing permissions.""" + portfolio_invitation, _ = PortfolioInvitation.objects.get_or_create(portfolio=self.portfolio, email="hi@ho") data = { "email": "hi@ho.com", "portfolio": self.portfolio.id, @@ -685,5 +691,5 @@ class TestBasePortfolioMemberForms(TestCase): "member_permissions": "", # Missing field "domain_permissions": "", # Missing field } - self._assert_form_has_error(PortfolioMemberForm, data, "domain_request_permissions") - self._assert_form_has_error(PortfolioInvitedMemberForm, data, "member_permissions") + self._assert_form_has_error(PortfolioMemberForm, data, "domain_request_permissions", portfolio_invitation) + self._assert_form_has_error(PortfolioInvitedMemberForm, data, "member_permissions", portfolio_invitation) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 98010c60f..8b92fb8fe 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -1826,10 +1826,7 @@ class TestPortfolioMemberDeleteView(WebTest): ) self.assertEqual(response.status_code, 400) - expected_error_message = ( - "There must be at least one admin in your organization. Give another member admin " - "permissions, make sure they log into the registrar, and then remove this member." - ) + expected_error_message = "the only admin for this organization" self.assertContains(response, expected_error_message, status_code=400) # assert that send_portfolio_admin_removal_emails is not called @@ -2155,10 +2152,7 @@ class TestPortfolioMemberDeleteView(WebTest): self.assertEqual(response.status_code, 302) - expected_error_message = ( - "There must be at least one admin in your organization. Give another member admin " - "permissions, make sure they log into the registrar, and then remove this member." - ) + expected_error_message = "the only admin for this organization." args, kwargs = mock_error.call_args # Check if first arg is a WSGIRequest, confirms request object passed correctly