Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#2711: Domain manager view - remove manager when only one manager [BOB] #3388

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
13 changes: 0 additions & 13 deletions src/registrar/templates/domain_users.html
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ <h2 class> Domain managers </h2>
</th>
{% if not portfolio %}<td data-label="Role">{{ item.permission.role|title }}</td>{% endif %}
<td>
{% if can_delete_users %}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conditional now obsolete

<a
id="button-toggle-user-alert-{{ forloop.counter }}"
href="#toggle-user-alert-{{ forloop.counter }}"
Expand Down Expand Up @@ -112,18 +111,6 @@ <h2 class> Domain managers </h2>
{% csrf_token %}
</form>
{% endif %}
{% else %}
<input
type="submit"
class="usa-button--unstyled disabled-button usa-tooltip usa-tooltip--registrar"
value="Remove"
data-position="bottom"
title="Domains must have at least one domain manager"
data-tooltip="true"
aria-disabled="true"
role="button"
>
{% endif %}
Comment on lines -115 to -126
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer need to present disabled button under any conditions

</td>
</tr>
{% endfor %}
Expand Down
56 changes: 54 additions & 2 deletions src/registrar/tests/test_views_domain.py
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,7 @@ def tearDown(self):
"""Ensure that the user has its original permissions"""
PortfolioInvitation.objects.all().delete()
UserPortfolioPermission.objects.all().delete()
UserDomainRole.objects.all().delete()
User.objects.exclude(id=self.user.id).delete()
super().tearDown()

Expand Down Expand Up @@ -1258,8 +1259,8 @@ def test_domain_invitation_cancel_retrieved_invitation(self):
response = self.client.post(reverse("invitation-cancel", kwargs={"pk": invitation.id}), follow=True)
# Assert that an error message is displayed to the user
self.assertContains(response, f"Invitation to {email_address} has already been retrieved.")
# Assert that the Cancel link is not displayed
self.assertNotContains(response, "Cancel")
# Assert that the Cancel link (form) is not displayed
self.assertNotContains(response, f"/invitation/{invitation.id}/cancel")
Comment on lines +1262 to +1263
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior test was not specific enough

# Assert that the DomainInvitation is not deleted
self.assertTrue(DomainInvitation.objects.filter(id=invitation.id).exists())
DomainInvitation.objects.filter(email=email_address).delete()
Expand Down Expand Up @@ -1317,6 +1318,57 @@ def test_domain_invitation_flow(self):
home_page = self.app.get(reverse("home"))
self.assertContains(home_page, self.domain.name)

@less_console_noise_decorator
def test_domain_user_role_delete(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No previous tests existed for this view

"""Posting to the delete view deletes a user domain role."""
# add two managers to the domain so that one can be successfully deleted
email_address = "[email protected]"
new_user = User.objects.create(email=email_address, username="mayor")
email_address_2 = "[email protected]"
new_user_2 = User.objects.create(email=email_address_2, username="secondmayor")
user_domain_role = UserDomainRole.objects.create(
user=new_user, domain=self.domain, role=UserDomainRole.Roles.MANAGER
)
UserDomainRole.objects.create(user=new_user_2, domain=self.domain, role=UserDomainRole.Roles.MANAGER)
response = self.client.post(
reverse("domain-user-delete", kwargs={"pk": self.domain.id, "user_pk": new_user.id}), follow=True
)
# Assert that a success message is displayed to the user
self.assertContains(response, f"Removed {email_address} as a manager for this domain.")
# Assert that the second user is displayed
self.assertContains(response, f"{email_address_2}")
# Assert that the UserDomainRole is deleted
self.assertFalse(UserDomainRole.objects.filter(id=user_domain_role.id).exists())

@less_console_noise_decorator
def test_domain_user_role_delete_only_manager(self):
"""Posting to the delete view attempts to delete a user domain role when there is only one manager."""
# self.user is the only domain manager, so attempt to delete it
response = self.client.post(
reverse("domain-user-delete", kwargs={"pk": self.domain.id, "user_pk": self.user.id}), follow=True
)
# Assert that an error message is displayed to the user
self.assertContains(response, "Domains must have at least one domain manager.")
# Assert that the user is still displayed
self.assertContains(response, f"{self.user.email}")
# Assert that the UserDomainRole still exists
self.assertTrue(UserDomainRole.objects.filter(user=self.user, domain=self.domain).exists())

@less_console_noise_decorator
def test_domain_user_role_delete_self_delete(self):
"""Posting to the delete view attempts to delete a user domain role when there is only one manager."""
# add one manager, so there are two and the logged in user, self.user, can be deleted
email_address = "[email protected]"
new_user = User.objects.create(email=email_address, username="mayor")
UserDomainRole.objects.create(user=new_user, domain=self.domain, role=UserDomainRole.Roles.MANAGER)
response = self.client.post(
reverse("domain-user-delete", kwargs={"pk": self.domain.id, "user_pk": self.user.id}), follow=True
)
# Assert that a success message is displayed to the user
self.assertContains(response, f"You are no longer managing the domain {self.domain}.")
# Assert that the UserDomainRole no longer exists
self.assertFalse(UserDomainRole.objects.filter(user=self.user, domain=self.domain).exists())


class TestDomainNameservers(TestDomainOverview, MockEppLib):
@less_console_noise_decorator
Expand Down
28 changes: 10 additions & 18 deletions src/registrar/views/domain.py
Original file line number Diff line number Diff line change
Expand Up @@ -1074,9 +1074,6 @@ def get_context_data(self, **kwargs):
"""The initial value for the form (which is a formset here)."""
context = super().get_context_data(**kwargs)

# Add conditionals to the context (such as "can_delete_users")
context = self._add_booleans_to_context(context)

Comment on lines -1077 to -1079
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer needed in context since template is no longer disabling button based on context values this method was adding

# Get portfolio from session (if set)
portfolio = self.request.session.get("portfolio")

Expand Down Expand Up @@ -1162,20 +1159,6 @@ def _add_invitations_to_context(self, context, portfolio):

return context

def _add_booleans_to_context(self, context):
# Determine if the current user can delete managers
domain_pk = None
can_delete_users = False

if self.kwargs is not None and "pk" in self.kwargs:
domain_pk = self.kwargs["pk"]
# Prevent the end user from deleting themselves as a manager if they are the
# only manager that exists on a domain.
can_delete_users = UserDomainRole.objects.filter(domain__id=domain_pk).count() > 1

context["can_delete_users"] = can_delete_users
return context


class DomainAddUserView(DomainFormBaseView):
"""Inside of a domain's user management, a form for adding users.
Expand Down Expand Up @@ -1341,7 +1324,16 @@ def form_valid(self, form):
return redirect(self.get_success_url())

def post(self, request, *args, **kwargs):
"""Custom post implementation to redirect to home in the event that the user deletes themselves"""
"""Custom post implementation to ensure last userdomainrole is not removed and to
redirect to home in the event that the user deletes themselves"""
self.object = self.get_object() # Retrieve the UserDomainRole to delete

# Check if this is the only UserDomainRole for the domain
if not len(UserDomainRole.objects.filter(domain=self.object.domain)) > 1:
messages.error(request, "Domains must have at least one domain manager.")
return redirect(self.get_success_url())

# normal delete processing in the event that the above condition not reached
response = super().post(request, *args, **kwargs)

# If the user is deleting themselves, redirect to home
Expand Down
6 changes: 0 additions & 6 deletions src/registrar/views/utility/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -343,12 +343,6 @@ def has_permission(self):
if not (has_delete_permission or user_is_analyst_or_superuser):
return False

# Check if more than one manager exists on the domain.
# If only one exists, prevent this from happening
has_multiple_managers = len(UserDomainRole.objects.filter(domain=domain_pk)) > 1
if not has_multiple_managers:
return False

Comment on lines -346 to -351
Copy link
Contributor Author

@dave-kennedy-ecs dave-kennedy-ecs Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition no longer returns a 403 permissions problem, and instead the view handles this and returns a specific error message.

return True


Expand Down
Loading