From 1e9ae7befc61c76c3df93935f0b22f84737320bb Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 6 Sep 2024 13:46:52 -0400 Subject: [PATCH 1/3] cleanup --- src/registrar/assets/js/get-gov.js | 4 ++-- src/registrar/assets/sass/_theme/_base.scss | 12 +++++----- .../assets/sass/_theme/_buttons.scss | 2 ++ src/registrar/context_processors.py | 8 +++---- src/registrar/models/user.py | 12 +++++----- src/registrar/registrar_middleware.py | 2 +- src/registrar/templates/domain_detail.html | 2 +- src/registrar/templates/domain_sidebar.html | 2 +- .../templates/domain_suborganization.html | 2 +- .../templates/includes/header_extended.html | 4 ++-- src/registrar/tests/test_models.py | 24 +++++++++---------- src/registrar/tests/test_views_portfolio.py | 6 ++--- src/registrar/views/domain.py | 2 +- src/registrar/views/utility/mixins.py | 4 ++-- 14 files changed, 44 insertions(+), 42 deletions(-) diff --git a/src/registrar/assets/js/get-gov.js b/src/registrar/assets/js/get-gov.js index aa186dc31..1cb1e6828 100644 --- a/src/registrar/assets/js/get-gov.js +++ b/src/registrar/assets/js/get-gov.js @@ -1717,7 +1717,7 @@ document.addEventListener('DOMContentLoaded', function() { role="button" id="button-toggle-delete-domain-alert-${request.id}" href="#toggle-delete-domain-alert-${request.id}" - class="usa-button text-secondary usa-button--unstyled text-no-underline late-loading-modal-trigger margin-top-2 visible-mobile" + class="usa-button text-secondary usa-button--unstyled text-no-underline late-loading-modal-trigger margin-top-2 visible-mobile-flex" aria-controls="toggle-delete-domain-alert-${request.id}" data-open-modal > @@ -1726,7 +1726,7 @@ document.addEventListener('DOMContentLoaded', function() { Delete ${domainName} -
+
  • - {% if has_domains_portfolio_permission %} + {% if has_any_domains_portfolio_permission %} {% url 'domains' as url %} {% else %} {% url 'no-portfolio-domains' as url %} @@ -77,7 +77,7 @@
- {% elif has_requests_portfolio_permission %} + {% elif has_any_requests_portfolio_permission %} {% url 'domain-requests' as url %} Domain requests diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index cdf008c57..26f4c381f 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1334,7 +1334,7 @@ class TestUser(TestCase): @patch.multiple( User, has_view_all_domains_portfolio_permission=lambda self, portfolio: True, - has_requests_portfolio_permission=lambda self, portfolio: True, + has_any_requests_portfolio_permission=lambda self, portfolio: True, has_edit_request_portfolio_permission=lambda self, portfolio: True, ) def test_portfolio_role_summary_view_only_admin_and_domain_requestor(self): @@ -1344,7 +1344,7 @@ class TestUser(TestCase): @patch.multiple( User, has_view_all_domains_portfolio_permission=lambda self, portfolio: True, - has_requests_portfolio_permission=lambda self, portfolio: True, + has_any_requests_portfolio_permission=lambda self, portfolio: True, ) def test_portfolio_role_summary_view_only_admin(self): # Test if the user is recognized as a View-only admin @@ -1354,7 +1354,7 @@ class TestUser(TestCase): User, has_base_portfolio_permission=lambda self, portfolio: True, has_edit_request_portfolio_permission=lambda self, portfolio: True, - has_domains_portfolio_permission=lambda self, portfolio: True, + has_any_domains_portfolio_permission=lambda self, portfolio: True, ) def test_portfolio_role_summary_member_domain_requestor_domain_manager(self): # Test if the user has 'Member', 'Domain requestor', and 'Domain manager' roles @@ -1370,7 +1370,7 @@ class TestUser(TestCase): @patch.multiple( User, has_base_portfolio_permission=lambda self, portfolio: True, - has_domains_portfolio_permission=lambda self, portfolio: True, + has_any_domains_portfolio_permission=lambda self, portfolio: True, ) def test_portfolio_role_summary_member_domain_manager(self): # Test if the user has 'Member' and 'Domain manager' roles @@ -1546,8 +1546,8 @@ class TestUser(TestCase): portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") - user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) - user_can_view_all_requests = self.user.has_requests_portfolio_permission(portfolio) + user_can_view_all_domains = self.user.has_any_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_any_requests_portfolio_permission(portfolio) self.assertFalse(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) @@ -1558,8 +1558,8 @@ class TestUser(TestCase): additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS], ) - user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) - user_can_view_all_requests = self.user.has_requests_portfolio_permission(portfolio) + user_can_view_all_domains = self.user.has_any_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_any_requests_portfolio_permission(portfolio) self.assertTrue(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) @@ -1568,16 +1568,16 @@ class TestUser(TestCase): portfolio_permission.save() portfolio_permission.refresh_from_db() - user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) - user_can_view_all_requests = self.user.has_requests_portfolio_permission(portfolio) + user_can_view_all_domains = self.user.has_any_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_any_requests_portfolio_permission(portfolio) self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER) - user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) - user_can_view_all_requests = self.user.has_requests_portfolio_permission(portfolio) + user_can_view_all_domains = self.user.has_any_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_any_requests_portfolio_permission(portfolio) self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index c5d1a9830..2e7bb978e 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -502,7 +502,7 @@ class TestPortfolio(WebTest): self.client.force_login(self.user) response = self.client.get(reverse("home"), follow=True) - self.assertFalse(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) + self.assertFalse(self.user.has_any_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "You aren") @@ -517,7 +517,7 @@ class TestPortfolio(WebTest): # Test the domains page - this user should have access response = self.client.get(reverse("domains")) - self.assertTrue(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) + self.assertTrue(self.user.has_any_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") @@ -528,7 +528,7 @@ class TestPortfolio(WebTest): # Test the domains page - this user should have access response = self.client.get(reverse("domains")) - self.assertTrue(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) + self.assertTrue(self.user.has_any_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") permission.delete() diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 003f8dd0d..511c55228 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -175,7 +175,7 @@ class DomainView(DomainBaseView): If particular views allow permissions, they will need to override this function.""" portfolio = self.request.session.get("portfolio") - if self.request.user.has_domains_portfolio_permission(portfolio): + if self.request.user.has_any_domains_portfolio_permission(portfolio): if Domain.objects.filter(id=pk).exists(): domain = Domain.objects.get(id=pk) if domain.domain_info.portfolio == portfolio: diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 48c0ccb61..24483b6ef 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -433,7 +433,7 @@ class PortfolioDomainsPermission(PortfolioBasePermission): up from the portfolio's primary key in self.kwargs["pk"]""" portfolio = self.request.session.get("portfolio") - if not self.request.user.has_domains_portfolio_permission(portfolio): + if not self.request.user.has_any_domains_portfolio_permission(portfolio): return False return super().has_permission() @@ -450,7 +450,7 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): up from the portfolio's primary key in self.kwargs["pk"]""" portfolio = self.request.session.get("portfolio") - if not self.request.user.has_requests_portfolio_permission(portfolio): + if not self.request.user.has_any_requests_portfolio_permission(portfolio): return False return super().has_permission() From 7a67845b6daeff5562623199473148596ba717e2 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 6 Sep 2024 12:11:49 -0600 Subject: [PATCH 2/3] bug fix --- src/registrar/registrar_middleware.py | 4 ++++ src/registrar/tests/test_views_portfolio.py | 5 +++++ src/registrar/views/domain_request.py | 1 + 3 files changed, 10 insertions(+) diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index 0bedec3ca..5323d513f 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -162,6 +162,10 @@ class CheckPortfolioMiddleware: request.session["portfolio"] = request.user.get_first_portfolio() else: request.session["portfolio"] = None + else: + # Edge case: waffle flag is changed while the user is logged in + if not request.user.is_org_user(request) and request.session.get("portfolio"): + request.session["portfolio"] = None if request.session.get("portfolio"): if current_path == self.home: diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 2e7bb978e..b8ed5c074 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -2,6 +2,7 @@ from django.urls import reverse from api.tests.common import less_console_noise_decorator from registrar.config import settings from registrar.models import Portfolio, SeniorOfficial +from unittest import skip from django_webtest import WebTest # type: ignore from registrar.models import ( DomainRequest, @@ -532,3 +533,7 @@ class TestPortfolio(WebTest): self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") permission.delete() + + @skip("TODO") + def test_portfolio_cache_updates_when_modified(self): + pass diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index 7c19c3af2..5beb74e94 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -522,6 +522,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): return HttpResponseRedirect(reverse("domain-requests")) else: return HttpResponseRedirect(reverse("home")) + # otherwise, proceed as normal return self.goto_next_step() From 1af08a6b4803bd755a5588af19b181c0a3e4dd9b Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 6 Sep 2024 16:44:35 -0400 Subject: [PATCH 3/3] model and view unit tests --- src/registrar/tests/test_models.py | 67 ++++++++++++ src/registrar/tests/test_views_portfolio.py | 112 +++++++++++++++++++- 2 files changed, 178 insertions(+), 1 deletion(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 26f4c381f..936e644d0 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1385,6 +1385,73 @@ class TestUser(TestCase): # Test if the user has no roles self.assertEqual(self.user.portfolio_role_summary(self.portfolio), []) + @patch('registrar.models.User._has_portfolio_permission') + def test_has_base_portfolio_permission(self, mock_has_permission): + mock_has_permission.return_value = True + + self.assertTrue(self.user.has_base_portfolio_permission(self.portfolio)) + mock_has_permission.assert_called_once_with(self.portfolio, UserPortfolioPermissionChoices.VIEW_PORTFOLIO) + + @patch('registrar.models.User._has_portfolio_permission') + def test_has_edit_org_portfolio_permission(self, mock_has_permission): + mock_has_permission.return_value = True + + self.assertTrue(self.user.has_edit_org_portfolio_permission(self.portfolio)) + mock_has_permission.assert_called_once_with(self.portfolio, UserPortfolioPermissionChoices.EDIT_PORTFOLIO) + + @patch('registrar.models.User._has_portfolio_permission') + def test_has_any_domains_portfolio_permission(self, mock_has_permission): + mock_has_permission.side_effect = [False, True] # First permission false, second permission true + + self.assertTrue(self.user.has_any_domains_portfolio_permission(self.portfolio)) + self.assertEqual(mock_has_permission.call_count, 2) + mock_has_permission.assert_any_call(self.portfolio, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS) + mock_has_permission.assert_any_call(self.portfolio, UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) + + @patch('registrar.models.User._has_portfolio_permission') + def test_has_view_all_domains_portfolio_permission(self, mock_has_permission): + mock_has_permission.return_value = True + + self.assertTrue(self.user.has_view_all_domains_portfolio_permission(self.portfolio)) + mock_has_permission.assert_called_once_with(self.portfolio, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS) + + @patch('registrar.models.User._has_portfolio_permission') + def test_has_any_requests_portfolio_permission(self, mock_has_permission): + mock_has_permission.side_effect = [False, True] # First permission false, second permission true + + self.assertTrue(self.user.has_any_requests_portfolio_permission(self.portfolio)) + self.assertEqual(mock_has_permission.call_count, 2) + mock_has_permission.assert_any_call(self.portfolio, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS) + mock_has_permission.assert_any_call(self.portfolio, UserPortfolioPermissionChoices.EDIT_REQUESTS) + + @patch('registrar.models.User._has_portfolio_permission') + def test_has_view_all_requests_portfolio_permission(self, mock_has_permission): + mock_has_permission.return_value = True + + self.assertTrue(self.user.has_view_all_requests_portfolio_permission(self.portfolio)) + mock_has_permission.assert_called_once_with(self.portfolio, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS) + + @patch('registrar.models.User._has_portfolio_permission') + def test_has_edit_request_portfolio_permission(self, mock_has_permission): + mock_has_permission.return_value = True + + self.assertTrue(self.user.has_edit_request_portfolio_permission(self.portfolio)) + mock_has_permission.assert_called_once_with(self.portfolio, UserPortfolioPermissionChoices.EDIT_REQUESTS) + + @patch('registrar.models.User._has_portfolio_permission') + def test_has_view_suborganization_portfolio_permission(self, mock_has_permission): + mock_has_permission.return_value = True + + self.assertTrue(self.user.has_view_suborganization_portfolio_permission(self.portfolio)) + mock_has_permission.assert_called_once_with(self.portfolio, UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION) + + @patch('registrar.models.User._has_portfolio_permission') + def test_has_edit_suborganization_portfolio_permission(self, mock_has_permission): + mock_has_permission.return_value = True + + self.assertTrue(self.user.has_edit_suborganization_portfolio_permission(self.portfolio)) + mock_has_permission.assert_called_once_with(self.portfolio, UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION) + @less_console_noise_decorator def test_check_transition_domains_without_domains_on_login(self): """A user's on_each_login callback does not check transition domains. diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index b8ed5c074..7d3c6f3be 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -13,9 +13,10 @@ from registrar.models import ( ) from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices -from .common import create_test_user +from .common import MockSESClient, completed_domain_request, create_test_user from waffle.testutils import override_flag from django.contrib.sessions.middleware import SessionMiddleware +import boto3_mocking # type: ignore import logging @@ -534,6 +535,115 @@ class TestPortfolio(WebTest): self.assertContains(response, "Domain name") permission.delete() + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + def test_portfolio_domain_requests_page_when_user_has_no_permissions(self): + """Test the no requests page""" + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + ) + self.client.force_login(self.user) + # create and submit a domain request + domain_request = completed_domain_request(user=self.user) + mock_client = MockSESClient() + with boto3_mocking.clients.handler_for("sesv2", mock_client): + domain_request.submit() + domain_request.save() + + requests_page = self.client.get(reverse("no-portfolio-requests"), follow=True) + + self.assertContains(requests_page, "You don’t have access to domain requests.") + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + def test_main_nav_when_user_has_no_permissions(self): + """Test the nav contains a link to the no requests page""" + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + ) + self.client.force_login(self.user) + # create and submit a domain request + domain_request = completed_domain_request(user=self.user) + mock_client = MockSESClient() + with boto3_mocking.clients.handler_for("sesv2", mock_client): + domain_request.submit() + domain_request.save() + + portfolio_landing_page = self.client.get(reverse("home"), follow=True) + + # link to no requests + self.assertContains(portfolio_landing_page, "no-organization-requests/") + # dropdown + self.assertNotContains(portfolio_landing_page, "basic-nav-section-two") + # link to requests + self.assertNotContains(portfolio_landing_page, 'href="/requests/') + # link to create + self.assertNotContains(portfolio_landing_page, 'href="/request/') + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + def test_main_nav_when_user_has_all_permissions(self): + """Test the nav contains a dropdown with a link to create and another link to view requests + Also test for the existence of the Create a new request btn on the requests page""" + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + self.client.force_login(self.user) + # create and submit a domain request + domain_request = completed_domain_request(user=self.user) + mock_client = MockSESClient() + with boto3_mocking.clients.handler_for("sesv2", mock_client): + domain_request.submit() + domain_request.save() + + portfolio_landing_page = self.client.get(reverse("home"), follow=True) + + # link to no requests + self.assertNotContains(portfolio_landing_page, "no-organization-requests/") + # dropdown + self.assertContains(portfolio_landing_page, "basic-nav-section-two") + # link to requests + self.assertContains(portfolio_landing_page, 'href="/requests/') + # link to create + self.assertContains(portfolio_landing_page, 'href="/request/') + + requests_page = self.client.get(reverse("domain-requests")) + + # create new request btn + self.assertContains(requests_page, 'Start a new domain request') + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + def test_main_nav_when_user_has_view_but_not_edit_permissions(self): + """Test the nav contains a simple link to view requests + Also test for the existence of the Create a new request btn on the requests page""" + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS] + ) + self.client.force_login(self.user) + # create and submit a domain request + domain_request = completed_domain_request(user=self.user) + mock_client = MockSESClient() + with boto3_mocking.clients.handler_for("sesv2", mock_client): + domain_request.submit() + domain_request.save() + + portfolio_landing_page = self.client.get(reverse("home"), follow=True) + + # link to no requests + self.assertNotContains(portfolio_landing_page, "no-organization-requests/") + # dropdown + self.assertNotContains(portfolio_landing_page, "basic-nav-section-two") + # link to requests + self.assertContains(portfolio_landing_page, 'href="/requests/') + # link to create + self.assertNotContains(portfolio_landing_page, 'href="/request/') + + requests_page = self.client.get(reverse("domain-requests")) + + # create new request btn + self.assertNotContains(requests_page, 'Start a new domain request') + @skip("TODO") def test_portfolio_cache_updates_when_modified(self): pass