From 658e7c98a77bb3e8f2b6dd054eb317dc2433db51 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 24 Jul 2024 22:28:34 -0400 Subject: [PATCH 1/5] initial working code --- src/registrar/assets/js/get-gov.js | 11 ++++-- src/registrar/models/user.py | 29 ++++----------- .../templates/includes/domains_table.html | 3 ++ src/registrar/tests/test_models.py | 9 ----- src/registrar/views/domains_json.py | 35 +++++++++++++++---- 5 files changed, 47 insertions(+), 40 deletions(-) diff --git a/src/registrar/assets/js/get-gov.js b/src/registrar/assets/js/get-gov.js index f83966756..bec7e861e 100644 --- a/src/registrar/assets/js/get-gov.js +++ b/src/registrar/assets/js/get-gov.js @@ -1141,6 +1141,8 @@ document.addEventListener('DOMContentLoaded', function() { const statusIndicator = document.querySelector('.domain__filter-indicator'); const statusToggle = document.querySelector('.usa-button--filter'); const noPortfolioFlag = document.getElementById('no-portfolio-js-flag'); + const portfolioElement = document.getElementById('portfolio-js-value'); + const portfolioValue = portfolioElement ? portfolioElement.getAttribute('data-portfolio') : null; /** * Loads rows in the domains list, as well as updates pagination around the domains list @@ -1150,10 +1152,15 @@ document.addEventListener('DOMContentLoaded', function() { * @param {*} order - the sort order {asc, desc} * @param {*} scroll - control for the scrollToElement functionality * @param {*} searchTerm - the search term + * @param {*} portfolio - the portfolio id */ - function loadDomains(page, sortBy = currentSortBy, order = currentOrder, scroll = scrollToTable, status = currentStatus, searchTerm = currentSearchTerm) { + function loadDomains(page, sortBy = currentSortBy, order = currentOrder, scroll = scrollToTable, status = currentStatus, searchTerm = currentSearchTerm, portfolio = portfolioValue) { // fetch json of page of domains, given params - fetch(`/get-domains-json/?page=${page}&sort_by=${sortBy}&order=${order}&status=${status}&search_term=${searchTerm}`) + let url = `/get-domains-json/?page=${page}&sort_by=${sortBy}&order=${order}&status=${status}&search_term=${searchTerm}` + if (portfolio) + url += `&portfolio=${portfolio}` + + fetch(url) .then(response => response.json()) .then(data => { if (data.error) { diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index b135e30c7..b1c9473db 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -76,11 +76,6 @@ class UserPortfolioPermissionChoices(models.TextChoices): VIEW_ALL_DOMAINS = "view_all_domains", "View all domains and domain reports" VIEW_MANAGED_DOMAINS = "view_managed_domains", "View managed domains" - # EDIT_DOMAINS is really self.domains. We add is hear and leverage it in has_permission - # so we have one way to test for portfolio and domain edit permissions - # Do we need to check for portfolio domains specifically? - # NOTE: A user on an org can currently invite a user outside the org - EDIT_DOMAINS = "edit_domains", "User is a manager on a domain" VIEW_MEMBER = "view_member", "View members" EDIT_MEMBER = "edit_member", "Create and edit members" @@ -268,11 +263,6 @@ def _get_portfolio_permissions(self): def _has_portfolio_permission(self, portfolio_permission): """The views should only call this function when testing for perms and not rely on roles.""" - # EDIT_DOMAINS === user is a manager on a domain (has UserDomainRole) - # NOTE: Should we check whether the domain is in the portfolio? - if portfolio_permission == self.UserPortfolioPermissionChoices.EDIT_DOMAINS and self.domains.exists(): - return True - if not self.portfolio: return False @@ -286,21 +276,14 @@ def has_base_portfolio_permission(self): return self._has_portfolio_permission(User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO) def has_domains_portfolio_permission(self): - return ( - self._has_portfolio_permission(User.UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS) - or self._has_portfolio_permission(User.UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) - # or self._has_portfolio_permission(User.UserPortfolioPermissionChoices.EDIT_DOMAINS) - ) - - def has_edit_domains_portfolio_permission(self): - return self._has_portfolio_permission(User.UserPortfolioPermissionChoices.EDIT_DOMAINS) + return self._has_portfolio_permission( + User.UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS + ) or self._has_portfolio_permission(User.UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) def has_domain_requests_portfolio_permission(self): - return ( - self._has_portfolio_permission(User.UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS) - or self._has_portfolio_permission(User.UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) - # or self._has_portfolio_permission(User.UserPortfolioPermissionChoices.EDIT_REQUESTS) - ) + return self._has_portfolio_permission( + User.UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS + ) or self._has_portfolio_permission(User.UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) @classmethod def needs_identity_verification(cls, email, uuid): diff --git a/src/registrar/templates/includes/domains_table.html b/src/registrar/templates/includes/domains_table.html index 3a7aee80b..b7ec4d3f3 100644 --- a/src/registrar/templates/includes/domains_table.html +++ b/src/registrar/templates/includes/domains_table.html @@ -8,6 +8,9 @@

Domains

+ {% else %} + + {% endif %}
diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 8daf15933..9f2872f5d 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1221,7 +1221,6 @@ def test_has_portfolio_permission(self): 1. Returns False when a user does not have a portfolio 2. Returns True when user has direct permission 3. Returns True when user has permission through a role - 4. Returns True EDIT_DOMAINS when user does not have the perm but has UserDomainRole Note: This tests _get_portfolio_permissions as a side effect """ @@ -1233,11 +1232,9 @@ def test_has_portfolio_permission(self): user_can_view_all_domains = self.user.has_domains_portfolio_permission() user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission() - user_can_edit_domains = self.user.has_edit_domains_portfolio_permission() self.assertFalse(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) - self.assertFalse(user_can_edit_domains) self.user.portfolio = portfolio self.user.save() @@ -1245,11 +1242,9 @@ def test_has_portfolio_permission(self): user_can_view_all_domains = self.user.has_domains_portfolio_permission() user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission() - user_can_edit_domains = self.user.has_edit_domains_portfolio_permission() self.assertTrue(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) - self.assertFalse(user_can_edit_domains) self.user.portfolio_roles = [User.UserPortfolioRoleChoices.ORGANIZATION_ADMIN] self.user.save() @@ -1257,11 +1252,9 @@ def test_has_portfolio_permission(self): user_can_view_all_domains = self.user.has_domains_portfolio_permission() user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission() - user_can_edit_domains = self.user.has_edit_domains_portfolio_permission() self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) - self.assertFalse(user_can_edit_domains) UserDomainRole.objects.all().get_or_create( user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER @@ -1269,11 +1262,9 @@ def test_has_portfolio_permission(self): user_can_view_all_domains = self.user.has_domains_portfolio_permission() user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission() - user_can_edit_domains = self.user.has_edit_domains_portfolio_permission() self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) - self.assertTrue(user_can_edit_domains) Portfolio.objects.all().delete() diff --git a/src/registrar/views/domains_json.py b/src/registrar/views/domains_json.py index 3b3cae2c7..d4c09d808 100644 --- a/src/registrar/views/domains_json.py +++ b/src/registrar/views/domains_json.py @@ -6,6 +6,8 @@ from django.urls import reverse from django.db.models import Q +from registrar.models.domain_information import DomainInformation + logger = logging.getLogger(__name__) @@ -14,10 +16,9 @@ def get_domains_json(request): """Given the current request, get all domains that are associated with the UserDomainRole object""" - user_domain_roles = UserDomainRole.objects.filter(user=request.user).select_related("domain_info__sub_organization") - domain_ids = user_domain_roles.values_list("domain_id", flat=True) + domain_ids = get_domain_ids_from_request(request) - objects = Domain.objects.filter(id__in=domain_ids) + objects = Domain.objects.filter(id__in=domain_ids).select_related("domain_info__sub_organization") unfiltered_total = objects.count() objects = apply_search(objects, request) @@ -28,7 +29,7 @@ def get_domains_json(request): page_number = request.GET.get("page") page_obj = paginator.get_page(page_number) - domains = [serialize_domain(domain) for domain in page_obj.object_list] + domains = [serialize_domain(domain, request.user) for domain in page_obj.object_list] return JsonResponse( { @@ -43,6 +44,21 @@ def get_domains_json(request): ) +def get_domain_ids_from_request(request): + """Get domain ids from request. + + If portfolio specified, return domain ids associated with portfolio. + Otherwise, return domain ids associated with request.user. + """ + portfolio = request.GET.get("portfolio") + if portfolio: + domain_infos = DomainInformation.objects.filter(portfolio=portfolio) + return domain_infos.values_list("domain_id", flat=True) + else: + user_domain_roles = UserDomainRole.objects.filter(user=request.user) + return user_domain_roles.values_list("domain_id", flat=True) + + def apply_search(queryset, request): search_term = request.GET.get("search_term") if search_term: @@ -94,7 +110,7 @@ def apply_sorting(queryset, request): return queryset.order_by(sort_by) -def serialize_domain(domain): +def serialize_domain(domain, user): suborganization_name = None try: domain_info = domain.domain_info @@ -106,6 +122,9 @@ def serialize_domain(domain): domain_info = None logger.debug(f"Issue in domains_json: We could not find domain_info for {domain}") + # Check if there is a UserDomainRole for this domain and user + user_domain_role_exists = UserDomainRole.objects.filter(domain_id=domain.id, user=user).exists() + return { "id": domain.id, "name": domain.name, @@ -114,7 +133,11 @@ def serialize_domain(domain): "state_display": domain.state_display(), "get_state_help_text": domain.get_state_help_text(), "action_url": reverse("domain", kwargs={"pk": domain.id}), - "action_label": ("View" if domain.state in [Domain.State.DELETED, Domain.State.ON_HOLD] else "Manage"), + "action_label": ( + "View" + if not user_domain_role_exists or domain.state in [Domain.State.DELETED, Domain.State.ON_HOLD] + else "Manage" + ), "svg_icon": ("visibility" if domain.state in [Domain.State.DELETED, Domain.State.ON_HOLD] else "settings"), "suborganization": suborganization_name, } From ed669cc4c6715702f8794dd884ea5ca7a6f8f788 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 25 Jul 2024 08:25:57 -0400 Subject: [PATCH 2/5] updated test code --- .../tests/test_views_domains_json.py | 91 ++++++++++++++++++- 1 file changed, 88 insertions(+), 3 deletions(-) diff --git a/src/registrar/tests/test_views_domains_json.py b/src/registrar/tests/test_views_domains_json.py index 28a7308f5..09a233768 100644 --- a/src/registrar/tests/test_views_domains_json.py +++ b/src/registrar/tests/test_views_domains_json.py @@ -1,4 +1,4 @@ -from registrar.models import UserDomainRole, Domain +from registrar.models import UserDomainRole, Domain, DomainInformation, Portfolio from django.urls import reverse from .test_views import TestWithUser from django_webtest import WebTest # type: ignore @@ -15,16 +15,25 @@ def setUp(self): self.domain1 = Domain.objects.create(name="example1.com", expiration_date="2024-01-01", state="unknown") self.domain2 = Domain.objects.create(name="example2.com", expiration_date="2024-02-01", state="dns needed") self.domain3 = Domain.objects.create(name="example3.com", expiration_date="2024-03-01", state="ready") + self.domain4 = Domain.objects.create(name="example4.com", expiration_date="2024-03-01", state="ready") # Create UserDomainRoles UserDomainRole.objects.create(user=self.user, domain=self.domain1) UserDomainRole.objects.create(user=self.user, domain=self.domain2) UserDomainRole.objects.create(user=self.user, domain=self.domain3) + # Create Portfolio + self.portfolio = Portfolio.objects.create(creator=self.user, organization_name="Example org") + + # Add domain3 and domain4 to portfolio + DomainInformation.objects.create(creator=self.user, domain=self.domain3, portfolio=self.portfolio) + DomainInformation.objects.create(creator=self.user, domain=self.domain4, portfolio=self.portfolio) + def tearDown(self): - super().tearDown() - UserDomainRole.objects.all().delete() UserDomainRole.objects.all().delete() + DomainInformation.objects.all().delete() + Portfolio.objects.all().delete() + super().tearDown() @less_console_noise_decorator def test_get_domains_json_unauthenticated(self): @@ -105,6 +114,82 @@ def test_get_domains_json_authenticated(self): ) self.assertEqual(svg_icon_expected, svg_icons[i]) + @less_console_noise_decorator + def test_get_domains_json_with_portfolio(self): + """Test that an authenticated user gets the list of 2 domains for portfolio.""" + + response = self.app.get(reverse("get_domains_json"), {"portfolio": self.portfolio.id}) + self.assertEqual(response.status_code, 200) + data = response.json + + # Check pagination info + self.assertEqual(data["page"], 1) + self.assertFalse(data["has_next"]) + self.assertFalse(data["has_previous"]) + self.assertEqual(data["num_pages"], 1) + + # Check the number of domains + self.assertEqual(len(data["domains"]), 2) + + # Expected domains + expected_domains = [self.domain3, self.domain4] + + # Extract fields from response + domain_ids = [domain["id"] for domain in data["domains"]] + names = [domain["name"] for domain in data["domains"]] + expiration_dates = [domain["expiration_date"] for domain in data["domains"]] + states = [domain["state"] for domain in data["domains"]] + state_displays = [domain["state_display"] for domain in data["domains"]] + get_state_help_texts = [domain["get_state_help_text"] for domain in data["domains"]] + action_urls = [domain["action_url"] for domain in data["domains"]] + action_labels = [domain["action_label"] for domain in data["domains"]] + svg_icons = [domain["svg_icon"] for domain in data["domains"]] + + # Check fields for each domain + for i, expected_domain in enumerate(expected_domains): + self.assertEqual(expected_domain.id, domain_ids[i]) + self.assertEqual(expected_domain.name, names[i]) + self.assertEqual(expected_domain.expiration_date, expiration_dates[i]) + self.assertEqual(expected_domain.state, states[i]) + + # Parsing the expiration date from string to date + parsed_expiration_date = parse_date(expiration_dates[i]) + expected_domain.expiration_date = parsed_expiration_date + + # Check state_display and get_state_help_text + self.assertEqual(expected_domain.state_display(), state_displays[i]) + self.assertEqual(expected_domain.get_state_help_text(), get_state_help_texts[i]) + + self.assertEqual(reverse("domain", kwargs={"pk": expected_domain.id}), action_urls[i]) + + # Check action_label + user_domain_role_exists = UserDomainRole.objects.filter( + domain_id=expected_domains[i].id, user=self.user + ).exists() + action_label_expected = ( + "View" + if not user_domain_role_exists + or expected_domains[i].state + in [ + Domain.State.DELETED, + Domain.State.ON_HOLD, + ] + else "Manage" + ) + self.assertEqual(action_label_expected, action_labels[i]) + + # Check svg_icon + svg_icon_expected = ( + "visibility" + if expected_domains[i].state + in [ + Domain.State.DELETED, + Domain.State.ON_HOLD, + ] + else "settings" + ) + self.assertEqual(svg_icon_expected, svg_icons[i]) + @less_console_noise_decorator def test_get_domains_json_search(self): """Test search.""" From edb33bba6979f54c2dd8cd01f0ff140570e07d65 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 25 Jul 2024 08:35:52 -0400 Subject: [PATCH 3/5] cleaned up some comments --- src/registrar/context_processors.py | 1 - src/registrar/templates/includes/domains_table.html | 3 --- 2 files changed, 4 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index 06ef07050..9854cf404 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -61,7 +61,6 @@ def add_path_to_context(request): def add_has_profile_feature_flag_to_context(request): return {"has_profile_feature_flag": flag_is_active(request, "profile_feature")} - def portfolio_permissions(request): """Make portfolio permissions for the request user available in global context""" try: diff --git a/src/registrar/templates/includes/domains_table.html b/src/registrar/templates/includes/domains_table.html index b7ec4d3f3..cd9ea372f 100644 --- a/src/registrar/templates/includes/domains_table.html +++ b/src/registrar/templates/includes/domains_table.html @@ -2,7 +2,6 @@
- {% if portfolio is None %}

Domains

@@ -41,7 +40,6 @@

Domains

- {% if portfolio %}
Filter by @@ -145,7 +143,6 @@

Status

Domain name Expires Status - {% if portfolio %} Suborganization {% endif %} From 784b005968ec9ced1406b689911b21dc6018bc14 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 25 Jul 2024 08:36:14 -0400 Subject: [PATCH 4/5] cleaned up some comments --- src/registrar/templates/includes/domain_requests_table.html | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/templates/includes/domain_requests_table.html b/src/registrar/templates/includes/domain_requests_table.html index efebd1e28..ad91699ef 100644 --- a/src/registrar/templates/includes/domain_requests_table.html +++ b/src/registrar/templates/includes/domain_requests_table.html @@ -2,7 +2,6 @@
- {% if portfolio is None %}

Domain requests

From 06a5803bba480d26fbc7dc2f79e2666c53f3e98e Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 25 Jul 2024 08:47:52 -0400 Subject: [PATCH 5/5] lint plus migrations --- src/registrar/context_processors.py | 1 + ...r_user_portfolio_additional_permissions.py | 38 +++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 src/registrar/migrations/0114_alter_user_portfolio_additional_permissions.py diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index 9854cf404..06ef07050 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -61,6 +61,7 @@ def add_path_to_context(request): def add_has_profile_feature_flag_to_context(request): return {"has_profile_feature_flag": flag_is_active(request, "profile_feature")} + def portfolio_permissions(request): """Make portfolio permissions for the request user available in global context""" try: diff --git a/src/registrar/migrations/0114_alter_user_portfolio_additional_permissions.py b/src/registrar/migrations/0114_alter_user_portfolio_additional_permissions.py new file mode 100644 index 000000000..55645298f --- /dev/null +++ b/src/registrar/migrations/0114_alter_user_portfolio_additional_permissions.py @@ -0,0 +1,38 @@ +# Generated by Django 4.2.10 on 2024-07-25 12:45 + +import django.contrib.postgres.fields +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0113_user_portfolio_user_portfolio_additional_permissions_and_more"), + ] + + operations = [ + migrations.AlterField( + model_name="user", + name="portfolio_additional_permissions", + field=django.contrib.postgres.fields.ArrayField( + base_field=models.CharField( + choices=[ + ("view_all_domains", "View all domains and domain reports"), + ("view_managed_domains", "View managed domains"), + ("view_member", "View members"), + ("edit_member", "Create and edit members"), + ("view_all_requests", "View all requests"), + ("view_created_requests", "View created requests"), + ("edit_requests", "Create and edit requests"), + ("view_portfolio", "View organization"), + ("edit_portfolio", "Edit organization"), + ], + max_length=50, + ), + blank=True, + help_text="Select one or more additional permissions.", + null=True, + size=None, + ), + ), + ]