From 4ccde1ba8f80ccf385187ec6b38759454883e320 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 24 Mar 2025 12:11:36 -0600 Subject: [PATCH 01/12] add modal logic --- src/registrar/assets/src/js/getgov/main.js | 6 +++-- .../src/js/getgov/portfolio-member-page.js | 26 +++++++++++++++++-- .../portfolio_member_permissions.html | 22 ++++++++++++++++ src/registrar/views/portfolios.py | 7 +++++ 4 files changed, 57 insertions(+), 4 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/main.js b/src/registrar/assets/src/js/getgov/main.js index f077448aa..21efd492e 100644 --- a/src/registrar/assets/src/js/getgov/main.js +++ b/src/registrar/assets/src/js/getgov/main.js @@ -10,7 +10,7 @@ import { initDomainRequestsTable } from './table-domain-requests.js'; import { initMembersTable } from './table-members.js'; import { initMemberDomainsTable } from './table-member-domains.js'; import { initEditMemberDomainsTable } from './table-edit-member-domains.js'; -import { initPortfolioNewMemberPageToggle, initAddNewMemberPageListeners, initPortfolioMemberPageRadio } from './portfolio-member-page.js'; +import { initPortfolioNewMemberPageToggle, initAddNewMemberPageListeners, initPortfolioMemberPage } from './portfolio-member-page.js'; import { initDomainRequestForm } from './domain-request-form.js'; import { initDomainManagersPage } from './domain-managers.js'; import { initDomainDSData } from './domain-dsdata.js'; @@ -56,8 +56,10 @@ initDomainDNSSEC(); initFormErrorHandling(); +// Init the portfolio member page +initPortfolioMemberPage(); + // Init the portfolio new member page -initPortfolioMemberPageRadio(); initPortfolioNewMemberPageToggle(); initAddNewMemberPageListeners(); diff --git a/src/registrar/assets/src/js/getgov/portfolio-member-page.js b/src/registrar/assets/src/js/getgov/portfolio-member-page.js index 96961e5dc..6dfb4c041 100644 --- a/src/registrar/assets/src/js/getgov/portfolio-member-page.js +++ b/src/registrar/assets/src/js/getgov/portfolio-member-page.js @@ -193,10 +193,14 @@ export function initAddNewMemberPageListeners() { } // Initalize the radio for the member pages -export function initPortfolioMemberPageRadio() { +export function initPortfolioMemberPage() { document.addEventListener("DOMContentLoaded", () => { let memberForm = document.getElementById("member_form"); - let newMemberForm = document.getElementById("add_member_form") + let newMemberForm = document.getElementById("add_member_form"); + let editSelfWarningModal = document.getElementById("toggle-member-permissions-edit-self"); + let editSelfWarningModalConfirm = document.getElementById("member-permissions-edit-self"); + + // Init the radio if (memberForm || newMemberForm) { hookupRadioTogglerListener( 'role', @@ -206,5 +210,23 @@ export function initPortfolioMemberPageRadio() { } ); } + + // Init the "edit self" warning modal + if (memberForm && editSelfWarningModal) { + var canSubmit = false; + memberForm.addEventListener("submit", function(e) { + if (!canSubmit) { + e.preventDefault(); + } + editSelfWarningModal.click(); + }); + + if (editSelfWarningModalConfirm) { + editSelfWarningModalConfirm.addEventListener("click", function() { + canSubmit = true; + memberForm.submit(); + }); + } + } }); } diff --git a/src/registrar/templates/portfolio_member_permissions.html b/src/registrar/templates/portfolio_member_permissions.html index e5ae5864e..0ecf47d10 100644 --- a/src/registrar/templates/portfolio_member_permissions.html +++ b/src/registrar/templates/portfolio_member_permissions.html @@ -109,4 +109,26 @@ + +{% comment %} If an admin is trying to edit themselves, show a modal {% endcomment %} + {% if member and member.id == request.user.id %} + + + {% endif %} {% endblock portfolio_content%} diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 952793084..8654050a0 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -259,6 +259,12 @@ class PortfolioMemberEditView(DetailView, View): user = portfolio_permission.user form = self.form_class(instance=portfolio_permission) + admin_count = UserPortfolioPermission.objects.filter( + portfolio=request.session["portfolio"], + roles__overlap=[ + UserPortfolioRoleChoices.ORGANIZATION_ADMIN + ] + ).count() return render( request, @@ -267,6 +273,7 @@ class PortfolioMemberEditView(DetailView, View): "form": form, "member": user, "portfolio_permission": portfolio_permission, + "is_only_admin": admin_count < 2 }, ) From f5503f8935c67e56e7e0da74bcff82224c7cb106 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 24 Mar 2025 12:23:48 -0600 Subject: [PATCH 02/12] Update portfolio_member_permissions.html --- src/registrar/templates/portfolio_member_permissions.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/portfolio_member_permissions.html b/src/registrar/templates/portfolio_member_permissions.html index 0ecf47d10..5558786c3 100644 --- a/src/registrar/templates/portfolio_member_permissions.html +++ b/src/registrar/templates/portfolio_member_permissions.html @@ -125,7 +125,7 @@ data-force-action > {% if is_only_admin %} - {% include 'includes/modal.html' with modal_heading="You cannot modify yourself at this time." modal_description="You are the only admin on this organization. Add another administrator to remove yourself from this organization." modal_button_id="member-permissions-edit-self" cancel_button_only=True %} + {% include 'includes/modal.html' with modal_heading="You are the only admin on this organization." modal_description="Add another administrator to remove yourself from this organization." modal_button_id="member-permissions-edit-self" cancel_button_only=True %} {% else %} {% include 'includes/modal.html' with modal_heading="Are you sure you want to change your own permissions?" modal_description="This could permanently impact your access. This cannot be undone." modal_button_id="member-permissions-edit-self" modal_button_text="Yes, I know what I'm doing!" %} {% endif %} From ae7a3a97edb48369b479b9a930f62004e5490108 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 25 Mar 2025 11:56:15 -0600 Subject: [PATCH 03/12] change message --- .../src/js/getgov/portfolio-member-page.js | 16 +++++++++++++--- .../templates/portfolio_member_permissions.html | 2 +- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/portfolio-member-page.js b/src/registrar/assets/src/js/getgov/portfolio-member-page.js index 6dfb4c041..a3e75d8d0 100644 --- a/src/registrar/assets/src/js/getgov/portfolio-member-page.js +++ b/src/registrar/assets/src/js/getgov/portfolio-member-page.js @@ -213,20 +213,30 @@ export function initPortfolioMemberPage() { // Init the "edit self" warning modal if (memberForm && editSelfWarningModal) { - var canSubmit = false; + var canSubmit = document.querySelector(`input[name="role"]:checked`)?.value != "organization_member"; + let radioButtons = document.querySelectorAll(`input[name="role"]`); + radioButtons.forEach(function (radioButton) { + radioButton.addEventListener("change", function() { + selectedValue = radioButton.checked ? radioButton.value : null; + canSubmit = selectedValue != "organization_member"; + }); + }); + memberForm.addEventListener("submit", function(e) { if (!canSubmit) { e.preventDefault(); + editSelfWarningModal.click(); } - editSelfWarningModal.click(); }); - + if (editSelfWarningModalConfirm) { editSelfWarningModalConfirm.addEventListener("click", function() { canSubmit = true; memberForm.submit(); }); } + } }); + } diff --git a/src/registrar/templates/portfolio_member_permissions.html b/src/registrar/templates/portfolio_member_permissions.html index 5558786c3..d8c34f5f1 100644 --- a/src/registrar/templates/portfolio_member_permissions.html +++ b/src/registrar/templates/portfolio_member_permissions.html @@ -127,7 +127,7 @@ {% if is_only_admin %} {% include 'includes/modal.html' with modal_heading="You are the only admin on this organization." modal_description="Add another administrator to remove yourself from this organization." modal_button_id="member-permissions-edit-self" cancel_button_only=True %} {% else %} - {% include 'includes/modal.html' with modal_heading="Are you sure you want to change your own permissions?" modal_description="This could permanently impact your access. This cannot be undone." modal_button_id="member-permissions-edit-self" modal_button_text="Yes, I know what I'm doing!" %} + {% include 'includes/modal.html' with modal_heading="Are you sure you want to change your member access?" modal_description="You’ve selected basic access, which means you’ll no longer have the ability to manage user permissions. This action cannot be undone." modal_button_id="member-permissions-edit-self" modal_button_text="Yes, change my access" %} {% endif %} {% endif %} From 91db8d8d50cc4d8658af8c227616487c9807d393 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 26 Mar 2025 09:26:37 -0600 Subject: [PATCH 04/12] alert logic --- src/registrar/forms/portfolio.py | 16 ++++++++++++++ src/registrar/templates/portfolio_member.html | 12 ++++++++++ .../portfolio_member_permissions.html | 22 ++++++++++++++----- src/registrar/views/portfolios.py | 14 ++++-------- 4 files changed, 48 insertions(+), 16 deletions(-) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index db1f58d88..fcc261236 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -411,6 +411,22 @@ class PortfolioMemberForm(BasePortfolioMemberForm): model = UserPortfolioPermission fields = ["roles", "additional_permissions"] + def clean(self): + """ + Override of clean to ensure that the user isn't removing themselves + if they're the only portfolio admin + """ + 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." + )}) + class PortfolioInvitedMemberForm(BasePortfolioMemberForm): """ diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 1b9ffd653..577ccada0 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -98,6 +98,18 @@ Organization member {% if portfolio_permission %} + {% if is_only_admin %} +
+
+

+ You're the only admin for this organization. + Organizations must have at least one admin. + To remove yourself or change your member access, + you'll need to add another admin. +

+
+
+ {% endif %} {% include "includes/summary_item.html" with title='Member access and permissions' permissions=True value=portfolio_permission edit_link=edit_url editable=has_edit_members_portfolio_permission %} {% elif portfolio_invitation %} {% include "includes/summary_item.html" with title='Member access and permissions' permissions=True value=portfolio_invitation edit_link=edit_url editable=has_edit_members_portfolio_permission %} diff --git a/src/registrar/templates/portfolio_member_permissions.html b/src/registrar/templates/portfolio_member_permissions.html index d8c34f5f1..5470978b9 100644 --- a/src/registrar/templates/portfolio_member_permissions.html +++ b/src/registrar/templates/portfolio_member_permissions.html @@ -75,11 +75,25 @@ +

Member access

+ + {% if is_only_admin %} +
+
+

+ You're the only admin for this organization. + Organizations must have at least one admin. + To remove yourself or change your member access, + you'll need to add another admin. +

+
+
+ {% endif %} Select the level of access for this member. * @@ -111,7 +125,7 @@ {% comment %} If an admin is trying to edit themselves, show a modal {% endcomment %} - {% if member and member.id == request.user.id %} + {% if member and member.id == request.user.id and not is_only_admin %} - {% if is_only_admin %} - {% include 'includes/modal.html' with modal_heading="You are the only admin on this organization." modal_description="Add another administrator to remove yourself from this organization." modal_button_id="member-permissions-edit-self" cancel_button_only=True %} - {% else %} - {% include 'includes/modal.html' with modal_heading="Are you sure you want to change your member access?" modal_description="You’ve selected basic access, which means you’ll no longer have the ability to manage user permissions. This action cannot be undone." modal_button_id="member-permissions-edit-self" modal_button_text="Yes, change my access" %} - {% endif %} + {% include 'includes/modal.html' with modal_heading="Are you sure you want to change your member access?" modal_description="You’ve selected basic access, which means you’ll no longer have the ability to manage user permissions. This action cannot be undone." modal_button_id="member-permissions-edit-self" modal_button_text="Yes, change my access" %} {% endif %} {% endblock portfolio_content%} diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 8654050a0..61f770849 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -114,6 +114,7 @@ class PortfolioMemberView(DetailView, View): "member_has_view_members_portfolio_permission": member_has_view_members_portfolio_permission, "member_has_edit_members_portfolio_permission": member_has_edit_members_portfolio_permission, "member_has_view_all_domains_portfolio_permission": member_has_view_all_domains_portfolio_permission, + "is_only_admin": request.user.is_only_admin_of_portfolio(portfolio_permission.portfolio), }, ) @@ -160,8 +161,8 @@ class PortfolioMemberDeleteView(View): ) if member.is_only_admin_of_portfolio(portfolio): return ( - "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." + "You can't remove yourself because you're the only admin for this organization. " + "To remove yourself, you'll need to add another admin." ) return None @@ -259,13 +260,6 @@ class PortfolioMemberEditView(DetailView, View): user = portfolio_permission.user form = self.form_class(instance=portfolio_permission) - admin_count = UserPortfolioPermission.objects.filter( - portfolio=request.session["portfolio"], - roles__overlap=[ - UserPortfolioRoleChoices.ORGANIZATION_ADMIN - ] - ).count() - return render( request, self.template_name, @@ -273,7 +267,7 @@ class PortfolioMemberEditView(DetailView, View): "form": form, "member": user, "portfolio_permission": portfolio_permission, - "is_only_admin": admin_count < 2 + "is_only_admin": request.user.is_only_admin_of_portfolio(portfolio_permission.portfolio) }, ) From 85666d2e7dfdf5c76f33f414892408529f2b1400 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 26 Mar 2025 10:54:56 -0600 Subject: [PATCH 05/12] lint and unit tests --- .../src/js/getgov/portfolio-member-page.js | 25 ++++++----- src/registrar/forms/portfolio.py | 16 ++++--- src/registrar/tests/test_views_portfolio.py | 44 +++++++++++++++++++ 3 files changed, 68 insertions(+), 17 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/portfolio-member-page.js b/src/registrar/assets/src/js/getgov/portfolio-member-page.js index a3e75d8d0..f1e64b744 100644 --- a/src/registrar/assets/src/js/getgov/portfolio-member-page.js +++ b/src/registrar/assets/src/js/getgov/portfolio-member-page.js @@ -211,8 +211,12 @@ export function initPortfolioMemberPage() { ); } - // Init the "edit self" warning modal - if (memberForm && editSelfWarningModal) { + // Init the "edit self" warning modal, which triggers when the user is trying to edit themselves. + // The dom will include these elements when this occurs. + // NOTE: This logic does not trigger when the user is the ONLY admin in the portfolio. + // This is because info alerts are used rather than modals in this case. + if (memberForm && editSelfWarningModal && editSelfWarningModalConfirm) { + // Only show the warning modal when the user is changing their ROLE. var canSubmit = document.querySelector(`input[name="role"]:checked`)?.value != "organization_member"; let radioButtons = document.querySelectorAll(`input[name="role"]`); radioButtons.forEach(function (radioButton) { @@ -221,21 +225,20 @@ export function initPortfolioMemberPage() { canSubmit = selectedValue != "organization_member"; }); }); - + + // Prevent form submission assuming org member is selected for role, and open the modal. memberForm.addEventListener("submit", function(e) { if (!canSubmit) { e.preventDefault(); editSelfWarningModal.click(); } }); - - if (editSelfWarningModalConfirm) { - editSelfWarningModalConfirm.addEventListener("click", function() { - canSubmit = true; - memberForm.submit(); - }); - } - + + // Hook the confirm button on the modal to form submission. + editSelfWarningModalConfirm.addEventListener("click", function() { + canSubmit = true; + memberForm.submit(); + }); } }); diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index fcc261236..a52d20a12 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -413,7 +413,7 @@ class PortfolioMemberForm(BasePortfolioMemberForm): def clean(self): """ - Override of clean to ensure that the user isn't removing themselves + Override of clean to ensure that the user isn't removing themselves if they're the only portfolio admin """ super().clean() @@ -421,11 +421,15 @@ class PortfolioMemberForm(BasePortfolioMemberForm): 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." - )}) + 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/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f490f51ad..98010c60f 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -4639,6 +4639,17 @@ class TestPortfolioMemberEditView(WebTest): # Get the user's admin permission admin_permission = UserPortfolioPermission.objects.get(user=self.user, portfolio=self.portfolio) + # Create a second permission so the user isn't just deleting themselves + member = create_test_user() + UserPortfolioPermission.objects.create( + user=member, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + # First, verify that the change modal is on the page + response = self.client.get(reverse("member-permissions", kwargs={"member_pk": admin_permission.id})) + self.assertEqual(response.status_code, 200) + self.assertContains(response, "Yes, change my access") + response = self.client.post( reverse("member-permissions", kwargs={"member_pk": admin_permission.id}), { @@ -4652,6 +4663,39 @@ class TestPortfolioMemberEditView(WebTest): self.assertEqual(response.status_code, 302) self.assertEqual(response["Location"], reverse("home")) + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_admin_removing_own_admin_role_only_admin(self): + """Tests that admin removing their own admin role when they are the only admin + throws a validation error. + """ + + self.client.force_login(self.user) + + # Get the user's admin permission + admin_permission = UserPortfolioPermission.objects.get(user=self.user, portfolio=self.portfolio) + + # First, verify that the info alert is present on the page + response = self.client.get(reverse("member-permissions", kwargs={"member_pk": admin_permission.id})) + self.assertEqual(response.status_code, 200) + self.assertContains(response, "To remove yourself or change your member access") + + # Then, verify that the right form error is shown + response = self.client.post( + reverse("member-permissions", kwargs={"member_pk": admin_permission.id}), + { + "role": UserPortfolioRoleChoices.ORGANIZATION_MEMBER, + "domain_permissions": UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS, + "member_permissions": "no_access", + "domain_request_permissions": "no_access", + }, + ) + + self.assertEqual(response.status_code, 200) + error_message = "the only admin for this organization" + self.assertIn(error_message, str(response.context["form"].errors)) + class TestPortfolioInvitedMemberEditView(WebTest): """Tests for the edit invited member page on portfolios""" From 8ee3ba6190eee01bb98383418a9a6037e626fd04 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 26 Mar 2025 12:14:09 -0600 Subject: [PATCH 06/12] unit tests and lint --- src/registrar/forms/portfolio.py | 17 ++++++----- src/registrar/models/user.py | 2 +- src/registrar/tests/test_forms.py | 34 ++++++++++++--------- src/registrar/tests/test_views_portfolio.py | 10 ++---- 4 files changed, 32 insertions(+), 31 deletions(-) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index fcc261236..40d42ab4d 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -418,14 +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 f490f51ad..6303a2135 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 From 7a42b913ed73bdb6ae08ab8c5917a9ef004a6e48 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 26 Mar 2025 13:11:04 -0600 Subject: [PATCH 07/12] lint --- src/registrar/forms/portfolio.py | 14 +++++++++----- src/registrar/tests/test_views_portfolio.py | 2 +- src/registrar/views/portfolios.py | 2 +- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 4d99ebd80..43a391ae0 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -422,11 +422,15 @@ class PortfolioMemberForm(BasePortfolioMemberForm): 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." - )}) + 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/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 8b92fb8fe..81be0d3ad 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -2159,7 +2159,7 @@ class TestPortfolioMemberDeleteView(WebTest): # WSGIRequest protocol is basically the HTTPRequest but in Django form (ie POST '/member/1/delete') self.assertIsInstance(args[0], WSGIRequest) # Check that the error message matches the expected error message - self.assertEqual(args[1], expected_error_message) + self.assertIn(expected_error_message, args[1]) # Location is used for a 3xx HTTP status code to indicate that the URL was redirected # and then confirm that we're still on the Manage Members page diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 61f770849..6233b9e89 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -267,7 +267,7 @@ class PortfolioMemberEditView(DetailView, View): "form": form, "member": user, "portfolio_permission": portfolio_permission, - "is_only_admin": request.user.is_only_admin_of_portfolio(portfolio_permission.portfolio) + "is_only_admin": request.user.is_only_admin_of_portfolio(portfolio_permission.portfolio), }, ) From c0b68eebe84b4b7bfc164d54682eaad4ec5f0cb1 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 27 Mar 2025 11:07:28 -0600 Subject: [PATCH 08/12] Update portfolio-member-page.js --- .../assets/src/js/getgov/portfolio-member-page.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/registrar/assets/src/js/getgov/portfolio-member-page.js b/src/registrar/assets/src/js/getgov/portfolio-member-page.js index f1e64b744..bbf91f250 100644 --- a/src/registrar/assets/src/js/getgov/portfolio-member-page.js +++ b/src/registrar/assets/src/js/getgov/portfolio-member-page.js @@ -215,14 +215,19 @@ export function initPortfolioMemberPage() { // The dom will include these elements when this occurs. // NOTE: This logic does not trigger when the user is the ONLY admin in the portfolio. // This is because info alerts are used rather than modals in this case. - if (memberForm && editSelfWarningModal && editSelfWarningModalConfirm) { + if (memberForm && editSelfWarningModal) { + console.log("Test") // Only show the warning modal when the user is changing their ROLE. var canSubmit = document.querySelector(`input[name="role"]:checked`)?.value != "organization_member"; + console.log("can submit") + console.log(canSubmit) let radioButtons = document.querySelectorAll(`input[name="role"]`); radioButtons.forEach(function (radioButton) { radioButton.addEventListener("change", function() { selectedValue = radioButton.checked ? radioButton.value : null; canSubmit = selectedValue != "organization_member"; + console.log("selected value") + console.log(selectedValue) }); }); From 58af877ca190e94d3c9a98efb22f7a453e65bfe1 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 27 Mar 2025 11:19:28 -0600 Subject: [PATCH 09/12] Update portfolio-member-page.js --- src/registrar/assets/src/js/getgov/portfolio-member-page.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/assets/src/js/getgov/portfolio-member-page.js b/src/registrar/assets/src/js/getgov/portfolio-member-page.js index bbf91f250..338149e86 100644 --- a/src/registrar/assets/src/js/getgov/portfolio-member-page.js +++ b/src/registrar/assets/src/js/getgov/portfolio-member-page.js @@ -224,7 +224,7 @@ export function initPortfolioMemberPage() { let radioButtons = document.querySelectorAll(`input[name="role"]`); radioButtons.forEach(function (radioButton) { radioButton.addEventListener("change", function() { - selectedValue = radioButton.checked ? radioButton.value : null; + let selectedValue = radioButton.checked ? radioButton.value : null; canSubmit = selectedValue != "organization_member"; console.log("selected value") console.log(selectedValue) From 7e8aad82f03f179850d8defbe2753bee76d0f579 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 27 Mar 2025 12:10:18 -0600 Subject: [PATCH 10/12] Fix bug with info dialog appearing on the wrong page --- .../src/js/getgov/portfolio-member-page.js | 5 --- src/registrar/templates/portfolio_member.html | 2 +- .../portfolio_member_permissions.html | 32 +++++++++---------- 3 files changed, 17 insertions(+), 22 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/portfolio-member-page.js b/src/registrar/assets/src/js/getgov/portfolio-member-page.js index 338149e86..faf2c7b18 100644 --- a/src/registrar/assets/src/js/getgov/portfolio-member-page.js +++ b/src/registrar/assets/src/js/getgov/portfolio-member-page.js @@ -216,18 +216,13 @@ export function initPortfolioMemberPage() { // NOTE: This logic does not trigger when the user is the ONLY admin in the portfolio. // This is because info alerts are used rather than modals in this case. if (memberForm && editSelfWarningModal) { - console.log("Test") // Only show the warning modal when the user is changing their ROLE. var canSubmit = document.querySelector(`input[name="role"]:checked`)?.value != "organization_member"; - console.log("can submit") - console.log(canSubmit) let radioButtons = document.querySelectorAll(`input[name="role"]`); radioButtons.forEach(function (radioButton) { radioButton.addEventListener("change", function() { let selectedValue = radioButton.checked ? radioButton.value : null; canSubmit = selectedValue != "organization_member"; - console.log("selected value") - console.log(selectedValue) }); }); diff --git a/src/registrar/templates/portfolio_member.html b/src/registrar/templates/portfolio_member.html index 577ccada0..1afe96161 100644 --- a/src/registrar/templates/portfolio_member.html +++ b/src/registrar/templates/portfolio_member.html @@ -98,7 +98,7 @@ Organization member {% if portfolio_permission %} - {% if is_only_admin %} + {% if member and member.id == request.user.id and is_only_admin %}

diff --git a/src/registrar/templates/portfolio_member_permissions.html b/src/registrar/templates/portfolio_member_permissions.html index 5470978b9..65e5541f8 100644 --- a/src/registrar/templates/portfolio_member_permissions.html +++ b/src/registrar/templates/portfolio_member_permissions.html @@ -82,7 +82,7 @@

Member access

- {% if is_only_admin %} + {% if member and member.id == request.user.id and is_only_admin %}

@@ -124,21 +124,21 @@

-{% comment %} If an admin is trying to edit themselves, show a modal {% endcomment %} + {% comment %} If an admin is trying to edit themselves, show a modal {% endcomment %} {% if member and member.id == request.user.id and not is_only_admin %} -
- + + {% endif %} {% endblock portfolio_content%} From accda363cbb008dc8465e937a19164fd7f830d83 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 27 Mar 2025 12:40:00 -0600 Subject: [PATCH 11/12] fix bad redirect url bug --- src/registrar/views/portfolios.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 6233b9e89..2a9b9684d 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -258,7 +258,6 @@ class PortfolioMemberEditView(DetailView, View): def get(self, request, member_pk): portfolio_permission = get_object_or_404(UserPortfolioPermission, pk=member_pk) user = portfolio_permission.user - form = self.form_class(instance=portfolio_permission) return render( request, @@ -308,15 +307,17 @@ class PortfolioMemberEditView(DetailView, View): form.save() messages.success(self.request, "The member access and permission changes have been saved.") return redirect("member", member_pk=member_pk) if not removing_admin_role_on_self else redirect("home") - - return render( - request, - self.template_name, - { - "form": form, - "member": user, # Pass the user object again to the template - }, - ) + else: + return render( + request, + self.template_name, + { + "form": form, + "member": user, + "portfolio_permission": portfolio_permission, + "is_only_admin": request.user.is_only_admin_of_portfolio(portfolio_permission.portfolio), + }, + ) def _handle_exceptions(self, exception): """Handle exceptions raised during the process.""" From a2cba301a562d9b8946dfd3d3c84975fa2d20698 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 27 Mar 2025 13:43:50 -0600 Subject: [PATCH 12/12] Update portfolio_member_permissions.html --- src/registrar/templates/portfolio_member_permissions.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/portfolio_member_permissions.html b/src/registrar/templates/portfolio_member_permissions.html index 65e5541f8..59a8b1fd8 100644 --- a/src/registrar/templates/portfolio_member_permissions.html +++ b/src/registrar/templates/portfolio_member_permissions.html @@ -138,7 +138,7 @@ id="modal-member-permissions-edit-self" data-force-action > - {% include 'includes/modal.html' with modal_heading="Are you sure you want to change your member access?" modal_description="You’ve selected basic access, which means you’ll no longer have the ability to manage user permissions. This action cannot be undone." modal_button_id="member-permissions-edit-self" modal_button_text="Yes, change my access" %} + {% include 'includes/modal.html' with modal_heading="Are you sure you want to change your member access?" modal_description="You’ve selected basic access, which means you’ll no longer be able to manage member permissions. This action cannot be undone." modal_button_id="member-permissions-edit-self" modal_button_text="Yes, change my access" %}
{% endif %} {% endblock portfolio_content%}