-
Notifications
You must be signed in to change notification settings - Fork 19
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
330a350
e3b667a
7078198
f27a55d
a470ee7
4f970fa
abbb793
e984853
aec5404
13b8d1a
c2dafae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 %} | ||
<a | ||
id="button-toggle-user-alert-{{ forloop.counter }}" | ||
href="#toggle-user-alert-{{ forloop.counter }}" | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 %} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
|
@@ -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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||
|
||
|
@@ -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. | ||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conditional now obsolete