From ee18be87c7ed8e64277046713cf88447e7003f55 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 30 Jul 2024 11:39:48 -0400 Subject: [PATCH 01/16] use css for read only styles --- src/registrar/assets/sass/_theme/_forms.scss | 10 +++ ...rtfolio_additional_permissions_and_more.py | 52 ++++++++++++ src/registrar/models/user.py | 17 ++-- .../includes/finish_profile_form.html | 8 +- .../templates/includes/input_read_only.html | 7 ++ .../templates/includes/input_with_errors.html | 8 +- ...donly_input.html => toggleable_input.html} | 0 ...edit_button.html => toggleable_label.html} | 0 .../templates/portfolio_organization.html | 78 ++++++++++-------- src/registrar/templatetags/field_helpers.py | 4 +- src/registrar/tests/test_views_portfolio.py | 80 ++++++++++--------- src/registrar/views/portfolios.py | 3 + 12 files changed, 179 insertions(+), 88 deletions(-) create mode 100644 src/registrar/migrations/0114_alter_user_portfolio_additional_permissions_and_more.py create mode 100644 src/registrar/templates/includes/input_read_only.html rename src/registrar/templates/includes/{readonly_input.html => toggleable_input.html} (100%) rename src/registrar/templates/includes/{label_with_edit_button.html => toggleable_label.html} (100%) diff --git a/src/registrar/assets/sass/_theme/_forms.scss b/src/registrar/assets/sass/_theme/_forms.scss index c025bdb29..0aedfcdba 100644 --- a/src/registrar/assets/sass/_theme/_forms.scss +++ b/src/registrar/assets/sass/_theme/_forms.scss @@ -82,3 +82,13 @@ legend.float-left-tablet + button.float-right-tablet { color: var(--close-button-hover-bg); } } + +.read-only-label { + font-size: size('body', 'sm'); + color: color('primary'); + margin-bottom: units(0.5); +} + +.read-only-value { + margin-top: units(0); +} diff --git a/src/registrar/migrations/0114_alter_user_portfolio_additional_permissions_and_more.py b/src/registrar/migrations/0114_alter_user_portfolio_additional_permissions_and_more.py new file mode 100644 index 000000000..f70c5388c --- /dev/null +++ b/src/registrar/migrations/0114_alter_user_portfolio_additional_permissions_and_more.py @@ -0,0 +1,52 @@ +# Generated by Django 4.2.10 on 2024-07-30 02:51 + +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"), + ("edit_domains", "User is a manager on a domain"), + ("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"), + ("edit_portfolio", "Edit organization"), + ], + max_length=50, + ), + blank=True, + help_text="Select one or more additional permissions.", + null=True, + size=None, + ), + ), + migrations.AlterField( + model_name="user", + name="portfolio_roles", + field=django.contrib.postgres.fields.ArrayField( + base_field=models.CharField( + choices=[("organization_admin", "Admin"), ("organization_admin_read_only", "Admin read only")], + max_length=50, + ), + blank=True, + help_text="Select one or more roles.", + null=True, + size=None, + ), + ), + ] diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index b135e30c7..dd826bc11 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -69,7 +69,7 @@ class UserPortfolioRoleChoices(models.TextChoices): ORGANIZATION_ADMIN = "organization_admin", "Admin" ORGANIZATION_ADMIN_READ_ONLY = "organization_admin_read_only", "Admin read only" - ORGANIZATION_MEMBER = "organization_member", "Member" + # ORGANIZATION_MEMBER is an abstract role where user.portfolio is true class UserPortfolioPermissionChoices(models.TextChoices): """ """ @@ -89,7 +89,7 @@ class UserPortfolioPermissionChoices(models.TextChoices): VIEW_CREATED_REQUESTS = "view_created_requests", "View created requests" EDIT_REQUESTS = "edit_requests", "Create and edit requests" - VIEW_PORTFOLIO = "view_portfolio", "View organization" + # VIEW_PORTFOLIO is an abstract permission that returns true when user.portfolio is true EDIT_PORTFOLIO = "edit_portfolio", "Edit organization" PORTFOLIO_ROLE_PERMISSIONS = { @@ -99,17 +99,12 @@ class UserPortfolioPermissionChoices(models.TextChoices): UserPortfolioPermissionChoices.EDIT_MEMBER, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, UserPortfolioPermissionChoices.EDIT_REQUESTS, - UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ], UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY: [ UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, UserPortfolioPermissionChoices.VIEW_MEMBER, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, - UserPortfolioPermissionChoices.VIEW_PORTFOLIO, - ], - UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ - UserPortfolioPermissionChoices.VIEW_PORTFOLIO, ], } @@ -280,10 +275,14 @@ def _has_portfolio_permission(self, portfolio_permission): return portfolio_permission in portfolio_permissions - # the methods below are checks for individual portfolio permissions. they are defined here + # the methods below are checks for individual portfolio permissions. They are defined here # to make them easier to call elsewhere throughout the application def has_base_portfolio_permission(self): - return self._has_portfolio_permission(User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO) + """Base role/permission, the user is simply linked to a portfolio""" + return self.portfolio is not None + + def has_edit_org_portfolio_permission(self): + return self._has_portfolio_permission(User.UserPortfolioPermissionChoices.EDIT_PORTFOLIO) def has_domains_portfolio_permission(self): return ( diff --git a/src/registrar/templates/includes/finish_profile_form.html b/src/registrar/templates/includes/finish_profile_form.html index 88f7a73af..8369511a5 100644 --- a/src/registrar/templates/includes/finish_profile_form.html +++ b/src/registrar/templates/includes/finish_profile_form.html @@ -35,7 +35,7 @@

What contact information should we use to reach you?

- {% with show_edit_button=True show_readonly=True group_classes="usa-form-editable usa-form-editable--no-border padding-top-2" %} + {% with toggleable_input=True toggleable_label=True group_classes="usa-form-editable usa-form-editable--no-border padding-top-2" %} {% input_with_errors form.full_name %} {% endwith %} @@ -54,7 +54,7 @@

What contact information should we use to reach you?

{% public_site_url "help/account-management/#get-help-with-login.gov" as login_help_url %} - {% with show_readonly=True add_class="display-none" group_classes="usa-form-editable usa-form-editable padding-top-2 bold-usa-label" %} + {% with toggleable_input=True add_class="display-none" group_classes="usa-form-editable usa-form-editable padding-top-2 bold-usa-label" %} {% with link_href=login_help_url %} {% with sublabel_text="We recommend using your work email for your .gov account. If the wrong email is displayed below, you’ll need to update your Login.gov account and log back in. Get help with your Login.gov account." %} {% with link_text="Get help with your Login.gov account" target_blank=True do_not_show_max_chars=True %} @@ -64,11 +64,11 @@

What contact information should we use to reach you?

{% endwith %} {% endwith %} - {% with show_edit_button=True show_readonly=True group_classes="usa-form-editable padding-top-2" %} + {% with toggleable_input=True toggleable_label=True group_classes="usa-form-editable padding-top-2" %} {% input_with_errors form.title %} {% endwith %} - {% with show_edit_button=True show_readonly=True group_classes="usa-form-editable padding-top-2" %} + {% with toggleable_input=True toggleable_label=True group_classes="usa-form-editable padding-top-2" %} {% with add_class="usa-input--medium" %} {% input_with_errors form.phone %} {% endwith %} diff --git a/src/registrar/templates/includes/input_read_only.html b/src/registrar/templates/includes/input_read_only.html new file mode 100644 index 000000000..b76f82e8b --- /dev/null +++ b/src/registrar/templates/includes/input_read_only.html @@ -0,0 +1,7 @@ +{% comment %} +Template include for read-only form fields +{% endcomment %} + + +

{{ field.label }}

+

{{ field.value }}

diff --git a/src/registrar/templates/includes/input_with_errors.html b/src/registrar/templates/includes/input_with_errors.html index 623ec0a33..d1e53968e 100644 --- a/src/registrar/templates/includes/input_with_errors.html +++ b/src/registrar/templates/includes/input_with_errors.html @@ -27,8 +27,8 @@ {% endif %} {% if not field.widget_type == "checkbox" %} - {% if show_edit_button %} - {% include "includes/label_with_edit_button.html" with bold_label=True %} + {% if toggleable_label %} + {% include "includes/toggleable_label.html" with bold_label=True %} {% else %} {% include "django/forms/label.html" %} {% endif %} @@ -63,8 +63,8 @@
{% endif %} - {% if show_readonly %} - {% include "includes/readonly_input.html" %} + {% if toggleable_input %} + {% include "includes/toggleable_input.html" %} {% endif %} {# this is the input field, itself #} diff --git a/src/registrar/templates/includes/readonly_input.html b/src/registrar/templates/includes/toggleable_input.html similarity index 100% rename from src/registrar/templates/includes/readonly_input.html rename to src/registrar/templates/includes/toggleable_input.html diff --git a/src/registrar/templates/includes/label_with_edit_button.html b/src/registrar/templates/includes/toggleable_label.html similarity index 100% rename from src/registrar/templates/includes/label_with_edit_button.html rename to src/registrar/templates/includes/toggleable_label.html diff --git a/src/registrar/templates/portfolio_organization.html b/src/registrar/templates/portfolio_organization.html index 0dede3c32..cc9cf5b6a 100644 --- a/src/registrar/templates/portfolio_organization.html +++ b/src/registrar/templates/portfolio_organization.html @@ -23,42 +23,56 @@

Organization

The name of your federal agency will be publicly listed as the domain registrant.

-

- The federal agency for your organization can’t be updated here. - To suggest an update, email help@get.gov. -

- - {% include "includes/form_errors.html" with form=form %} - - {% include "includes/required_fields.html" %} - -
- {% csrf_token %} - + {% if has_edit_org_portfolio_permission %}

- Federal agency - {{ portfolio }} + The federal agency for your organization can’t be updated here. + To suggest an update, email help@get.gov.

- {% input_with_errors form.address_line1 %} - - {% input_with_errors form.address_line2 %} - - {% input_with_errors form.city %} - - {% input_with_errors form.state_territory %} - - {% with add_class="usa-input--small" %} - {% input_with_errors form.zipcode %} - {% endwith %} + {% include "includes/form_errors.html" with form=form %} + {% include "includes/required_fields.html" %} + + {% csrf_token %} +

Federal agency

+

+ {{ portfolio }} +

+ {% input_with_errors form.address_line1 %} + {% input_with_errors form.address_line2 %} + {% input_with_errors form.city %} + {% input_with_errors form.state_territory %} + {% with add_class="usa-input--small" %} + {% input_with_errors form.zipcode %} + {% endwith %} + +
+ {% else %} +

Federal agency

+

+ {{ portfolio }} +

+ {% if form.address_line1.value is not None %} + {% include "includes/input_read_only.html" with field=form.address_line1 %} + {% endif %} + {% if form.address_line2.value is not None %} + {% include "includes/input_read_only.html" with field=form.address_line2 %} + {% endif %} + {% if form.city.value is not None %} + {% include "includes/input_read_only.html" with field=form.city %} + {% endif %} + {% if form.state_territory.value is not None %} + {% include "includes/input_read_only.html" with field=form.state_territory %} + {% endif %} + {% if form.zipcode.value is not None %} + {% include "includes/input_read_only.html" with field=form.zipcode %} + {% endif %} + {% endif %} - -
{% endblock %} diff --git a/src/registrar/templatetags/field_helpers.py b/src/registrar/templatetags/field_helpers.py index b72f77e21..68a803711 100644 --- a/src/registrar/templatetags/field_helpers.py +++ b/src/registrar/templatetags/field_helpers.py @@ -26,7 +26,7 @@ def input_with_errors(context, field=None): # noqa: C901 add_group_class: append to input element's surrounding tag's `class` attribute attr_* - adds or replaces any single html attribute for the input add_error_attr_* - like `attr_*` but only if field.errors is not empty - show_edit_button: shows a simple edit button, and adds display-none to the input field. + toggleable_input: shows a simple edit button, and adds display-none to the input field. Example usage: ``` @@ -92,7 +92,7 @@ def input_with_errors(context, field=None): # noqa: C901 elif key == "add_group_class": group_classes.append(value) - elif key == "show_edit_button": + elif key == "toggleable_input": # Hide the primary input field. # Used such that we can toggle it with JS if "display-none" not in classes: diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 3596bf567..c4cdcc2b2 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -37,25 +37,10 @@ def tearDown(self): User.objects.all().delete() super().tearDown() - @less_console_noise_decorator - def test_middleware_does_not_redirect_if_no_permission(self): - """Test that user with no portfolio permission is not redirected when attempting to access home""" - self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.save() - self.user.refresh_from_db() - with override_flag("organization_feature", active=True): - # This will redirect the user to the portfolio page. - # Follow implicity checks if our redirect is working. - portfolio_page = self.app.get(reverse("home")) - # Assert that we're on the right page - self.assertNotContains(portfolio_page, self.portfolio.organization_name) - @less_console_noise_decorator def test_middleware_does_not_redirect_if_no_portfolio(self): """Test that user with no assigned portfolio is not redirected when attempting to access home""" self.app.set_user(self.user.username) - self.user.portfolio_additional_permissions = [User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO] self.user.save() self.user.refresh_from_db() with override_flag("organization_feature", active=True): @@ -67,10 +52,9 @@ def test_middleware_does_not_redirect_if_no_portfolio(self): @less_console_noise_decorator def test_middleware_redirects_to_portfolio_organization_page(self): - """Test that user with VIEW_PORTFOLIO is redirected to portfolio organization page""" + """Test that user with a portfolio is redirected to portfolio organization page""" self.app.set_user(self.user.username) self.user.portfolio = self.portfolio - self.user.portfolio_additional_permissions = [User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO] self.user.save() self.user.refresh_from_db() with override_flag("organization_feature", active=True): @@ -83,11 +67,10 @@ def test_middleware_redirects_to_portfolio_organization_page(self): @less_console_noise_decorator def test_middleware_redirects_to_portfolio_domains_page(self): - """Test that user with VIEW_PORTFOLIO and VIEW_ALL_DOMAINS is redirected to portfolio domains page""" + """Test that user with a portfolio and VIEW_ALL_DOMAINS is redirected to portfolio domains page""" self.app.set_user(self.user.username) self.user.portfolio = self.portfolio self.user.portfolio_additional_permissions = [ - User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO, User.UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, ] self.user.save() @@ -134,20 +117,47 @@ def test_portfolio_domain_requests_page_403_when_user_not_have_permission(self): self.assertEqual(response.status_code, 403) @less_console_noise_decorator - def test_portfolio_organization_page_403_when_user_not_have_permission(self): - """Test that user without proper permission is not allowed access to portfolio organization page""" + def test_portfolio_organization_page_read_only(self): + """Test that user with a portfolio can access the portfolio organization page, read only""" self.app.set_user(self.user.username) self.user.portfolio = self.portfolio + self.portfolio.city = "Los Angeles" + self.portfolio.save() self.user.save() self.user.refresh_from_db() with override_flag("organization_feature", active=True): - # This will redirect the user to the portfolio page. - # Follow implicity checks if our redirect is working. - response = self.app.get( - reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk}), status=403 - ) - # Assert the response is a 403 Forbidden - self.assertEqual(response.status_code, 403) + response = self.app.get(reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk})) + # Assert the response is a 200 + self.assertEqual(response.status_code, 200) + # The label for Federal agency will always be a h4 + self.assertContains(response, '

Federal agency

') + # The read only label for city will be a h4 + self.assertContains(response, '

City

') + self.assertNotContains(response, 'for="id_city"') + self.assertContains(response, '

Los Angeles

') + + @less_console_noise_decorator + def test_portfolio_organization_page_edit_access(self): + """Test that user with a portfolio can access the portfolio organization page, read only""" + self.app.set_user(self.user.username) + self.user.portfolio = self.portfolio + self.user.portfolio_additional_permissions = [ + User.UserPortfolioPermissionChoices.EDIT_PORTFOLIO, + ] + self.portfolio.city = "Los Angeles" + self.portfolio.save() + self.user.save() + self.user.refresh_from_db() + with override_flag("organization_feature", active=True): + response = self.app.get(reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk})) + # Assert the response is a 200 + self.assertEqual(response.status_code, 200) + # The label for Federal agency will always be a h4 + self.assertContains(response, '

Federal agency

') + # The read only label for city will be a h4 + self.assertNotContains(response, '

City

') + self.assertNotContains(response, '

Los Angeles

>') + self.assertContains(response, 'for="id_city"') @less_console_noise_decorator def test_navigation_links_hidden_when_user_not_have_permission(self): @@ -155,7 +165,6 @@ def test_navigation_links_hidden_when_user_not_have_permission(self): self.app.set_user(self.user.username) self.user.portfolio = self.portfolio self.user.portfolio_additional_permissions = [ - User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO, User.UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, User.UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, ] @@ -176,9 +185,9 @@ def test_navigation_links_hidden_when_user_not_have_permission(self): portfolio_page, reverse("portfolio-domain-requests", kwargs={"portfolio_id": self.portfolio.pk}) ) - # reducing portfolio permissions to just VIEW_PORTFOLIO, which should remove domains + # removing non-basic portfolio perms, which should remove domains # and domain requests from nav - self.user.portfolio_additional_permissions = [User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO] + self.user.portfolio_additional_permissions = [] self.user.save() self.user.refresh_from_db() @@ -194,16 +203,13 @@ def test_navigation_links_hidden_when_user_not_have_permission(self): portfolio_page, reverse("portfolio-domain-requests", kwargs={"portfolio_id": self.portfolio.pk}) ) - -class TestPortfolioOrganization(TestPortfolio): - + @less_console_noise_decorator def test_portfolio_org_name(self): """Can load portfolio's org name page.""" with override_flag("organization_feature", active=True): self.app.set_user(self.user.username) self.user.portfolio = self.portfolio self.user.portfolio_additional_permissions = [ - User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO, User.UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] self.user.save() @@ -214,13 +220,13 @@ def test_portfolio_org_name(self): page, "The name of your federal agency will be publicly listed as the domain registrant." ) + @less_console_noise_decorator def test_domain_org_name_address_content(self): """Org name and address information appears on the page.""" with override_flag("organization_feature", active=True): self.app.set_user(self.user.username) self.user.portfolio = self.portfolio self.user.portfolio_additional_permissions = [ - User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO, User.UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] self.user.save() @@ -232,13 +238,13 @@ def test_domain_org_name_address_content(self): # Once in the sidenav, once in the main nav, once in the form self.assertContains(page, "Hotel California", count=3) + @less_console_noise_decorator def test_domain_org_name_address_form(self): """Submitting changes works on the org name address page.""" with override_flag("organization_feature", active=True): self.app.set_user(self.user.username) self.user.portfolio = self.portfolio self.user.portfolio_additional_permissions = [ - User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO, User.UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] self.user.save() diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index abd9648ba..c04044664 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -64,6 +64,9 @@ def get_context_data(self, **kwargs): """Add additional context data to the template.""" context = super().get_context_data(**kwargs) # no need to add portfolio to request context here + + context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission() + context["has_profile_feature_flag"] = flag_is_active(self.request, "profile_feature") context["has_organization_feature_flag"] = flag_is_active(self.request, "organization_feature") return context From f183090e24bd05461fb7c2b1b084914df26bd0e7 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 30 Jul 2024 12:04:25 -0400 Subject: [PATCH 02/16] fix unit tests --- src/registrar/tests/test_views_portfolio.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index c4cdcc2b2..f28b10516 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -130,9 +130,9 @@ def test_portfolio_organization_page_read_only(self): # Assert the response is a 200 self.assertEqual(response.status_code, 200) # The label for Federal agency will always be a h4 - self.assertContains(response, '

Federal agency

') + self.assertContains(response, '

Federal agency

') # The read only label for city will be a h4 - self.assertContains(response, '

City

') + self.assertContains(response, '

City

') self.assertNotContains(response, 'for="id_city"') self.assertContains(response, '

Los Angeles

') @@ -153,9 +153,9 @@ def test_portfolio_organization_page_edit_access(self): # Assert the response is a 200 self.assertEqual(response.status_code, 200) # The label for Federal agency will always be a h4 - self.assertContains(response, '

Federal agency

') + self.assertContains(response, '

Federal agency

') # The read only label for city will be a h4 - self.assertNotContains(response, '

City

') + self.assertNotContains(response, '

City

') self.assertNotContains(response, '

Los Angeles

>') self.assertContains(response, 'for="id_city"') From 6e900bc501cf00238b006fff9719f54de287bc13 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 30 Jul 2024 12:35:17 -0400 Subject: [PATCH 03/16] fix unit tests --- src/registrar/tests/test_views_portfolio.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f28b10516..089b710aa 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -134,7 +134,7 @@ def test_portfolio_organization_page_read_only(self): # The read only label for city will be a h4 self.assertContains(response, '

City

') self.assertNotContains(response, 'for="id_city"') - self.assertContains(response, '

Los Angeles

') + self.assertContains(response, '

Los Angeles

') @less_console_noise_decorator def test_portfolio_organization_page_edit_access(self): @@ -156,7 +156,7 @@ def test_portfolio_organization_page_edit_access(self): self.assertContains(response, '

Federal agency

') # The read only label for city will be a h4 self.assertNotContains(response, '

City

') - self.assertNotContains(response, '

Los Angeles

>') + self.assertNotContains(response, '

Los Angeles

>') self.assertContains(response, 'for="id_city"') @less_console_noise_decorator From 7f212edda5d61ad0b82ead1df71818369fb4959b Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 30 Jul 2024 19:58:39 -0400 Subject: [PATCH 04/16] revert some of the tweaks --- ..._user_portfolio_additional_permissions.py} | 17 +------- src/registrar/models/user.py | 14 +++--- src/registrar/tests/test_views_portfolio.py | 43 ++++++++++++++++++- 3 files changed, 51 insertions(+), 23 deletions(-) rename src/registrar/migrations/{0114_alter_user_portfolio_additional_permissions_and_more.py => 0114_alter_user_portfolio_additional_permissions.py} (70%) diff --git a/src/registrar/migrations/0114_alter_user_portfolio_additional_permissions_and_more.py b/src/registrar/migrations/0114_alter_user_portfolio_additional_permissions.py similarity index 70% rename from src/registrar/migrations/0114_alter_user_portfolio_additional_permissions_and_more.py rename to src/registrar/migrations/0114_alter_user_portfolio_additional_permissions.py index f70c5388c..27f6e699f 100644 --- a/src/registrar/migrations/0114_alter_user_portfolio_additional_permissions_and_more.py +++ b/src/registrar/migrations/0114_alter_user_portfolio_additional_permissions.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.10 on 2024-07-30 02:51 +# Generated by Django 4.2.10 on 2024-07-30 23:58 import django.contrib.postgres.fields from django.db import migrations, models @@ -26,6 +26,7 @@ class Migration(migrations.Migration): ("view_created_requests", "View created requests"), ("edit_requests", "Create and edit requests"), ("edit_portfolio", "Edit organization"), + ("view_portfolio", "View organization"), ], max_length=50, ), @@ -35,18 +36,4 @@ class Migration(migrations.Migration): size=None, ), ), - migrations.AlterField( - model_name="user", - name="portfolio_roles", - field=django.contrib.postgres.fields.ArrayField( - base_field=models.CharField( - choices=[("organization_admin", "Admin"), ("organization_admin_read_only", "Admin read only")], - max_length=50, - ), - blank=True, - help_text="Select one or more roles.", - null=True, - size=None, - ), - ), ] diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index dd826bc11..48e6dc420 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -64,15 +64,15 @@ class VerificationTypeChoices(models.TextChoices): class UserPortfolioRoleChoices(models.TextChoices): """ - Roles make it easier for admins to look at + Roles make it easier for admins to look at groups of users """ ORGANIZATION_ADMIN = "organization_admin", "Admin" ORGANIZATION_ADMIN_READ_ONLY = "organization_admin_read_only", "Admin read only" - # ORGANIZATION_MEMBER is an abstract role where user.portfolio is true + ORGANIZATION_MEMBER = "organization_member", "Member" class UserPortfolioPermissionChoices(models.TextChoices): - """ """ + """We test against permissions to manage access""" VIEW_ALL_DOMAINS = "view_all_domains", "View all domains and domain reports" VIEW_MANAGED_DOMAINS = "view_managed_domains", "View managed domains" @@ -89,8 +89,8 @@ class UserPortfolioPermissionChoices(models.TextChoices): VIEW_CREATED_REQUESTS = "view_created_requests", "View created requests" EDIT_REQUESTS = "edit_requests", "Create and edit requests" - # VIEW_PORTFOLIO is an abstract permission that returns true when user.portfolio is true EDIT_PORTFOLIO = "edit_portfolio", "Edit organization" + VIEW_PORTFOLIO = "view_portfolio", "View organization" PORTFOLIO_ROLE_PERMISSIONS = { UserPortfolioRoleChoices.ORGANIZATION_ADMIN: [ @@ -106,6 +106,9 @@ class UserPortfolioPermissionChoices(models.TextChoices): UserPortfolioPermissionChoices.VIEW_MEMBER, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, ], + UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + ], } # #### Constants for choice fields #### @@ -278,8 +281,7 @@ def _has_portfolio_permission(self, portfolio_permission): # the methods below are checks for individual portfolio permissions. They are defined here # to make them easier to call elsewhere throughout the application def has_base_portfolio_permission(self): - """Base role/permission, the user is simply linked to a portfolio""" - return self.portfolio is not None + return self._has_portfolio_permission(User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO) def has_edit_org_portfolio_permission(self): return self._has_portfolio_permission(User.UserPortfolioPermissionChoices.EDIT_PORTFOLIO) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 089b710aa..21b794cf7 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -37,10 +37,25 @@ def tearDown(self): User.objects.all().delete() super().tearDown() + @less_console_noise_decorator + def test_middleware_does_not_redirect_if_no_permission(self): + """Test that user with no portfolio permission is not redirected when attempting to access home""" + self.app.set_user(self.user.username) + self.user.portfolio = self.portfolio + self.user.save() + self.user.refresh_from_db() + with override_flag("organization_feature", active=True): + # This will redirect the user to the portfolio page. + # Follow implicity checks if our redirect is working. + portfolio_page = self.app.get(reverse("home")) + # Assert that we're on the right page + self.assertNotContains(portfolio_page, self.portfolio.organization_name) + @less_console_noise_decorator def test_middleware_does_not_redirect_if_no_portfolio(self): """Test that user with no assigned portfolio is not redirected when attempting to access home""" self.app.set_user(self.user.username) + self.user.portfolio_additional_permissions = [User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO] self.user.save() self.user.refresh_from_db() with override_flag("organization_feature", active=True): @@ -52,9 +67,10 @@ def test_middleware_does_not_redirect_if_no_portfolio(self): @less_console_noise_decorator def test_middleware_redirects_to_portfolio_organization_page(self): - """Test that user with a portfolio is redirected to portfolio organization page""" + """Test that user with a portfolio and VIEW_PORTFOLIO is redirected to portfolio organization page""" self.app.set_user(self.user.username) self.user.portfolio = self.portfolio + self.user.portfolio_additional_permissions = [User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO] self.user.save() self.user.refresh_from_db() with override_flag("organization_feature", active=True): @@ -67,10 +83,12 @@ def test_middleware_redirects_to_portfolio_organization_page(self): @less_console_noise_decorator def test_middleware_redirects_to_portfolio_domains_page(self): - """Test that user with a portfolio and VIEW_ALL_DOMAINS is redirected to portfolio domains page""" + """Test that user with a portfolio, VIEW_PORTFOLIO, VIEW_ALL_DOMAINS + is redirected to portfolio domains page""" self.app.set_user(self.user.username) self.user.portfolio = self.portfolio self.user.portfolio_additional_permissions = [ + User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO, User.UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, ] self.user.save() @@ -116,12 +134,29 @@ def test_portfolio_domain_requests_page_403_when_user_not_have_permission(self): # Assert the response is a 403 Forbidden self.assertEqual(response.status_code, 403) + @less_console_noise_decorator + def test_portfolio_organization_page_403_when_user_not_have_permission(self): + """Test that user without proper permission is not allowed access to portfolio organization page""" + self.app.set_user(self.user.username) + self.user.portfolio = self.portfolio + self.user.save() + self.user.refresh_from_db() + with override_flag("organization_feature", active=True): + # This will redirect the user to the portfolio page. + # Follow implicity checks if our redirect is working. + response = self.app.get( + reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk}), status=403 + ) + # Assert the response is a 403 Forbidden + self.assertEqual(response.status_code, 403) + @less_console_noise_decorator def test_portfolio_organization_page_read_only(self): """Test that user with a portfolio can access the portfolio organization page, read only""" self.app.set_user(self.user.username) self.user.portfolio = self.portfolio self.portfolio.city = "Los Angeles" + self.user.portfolio_additional_permissions = [User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO] self.portfolio.save() self.user.save() self.user.refresh_from_db() @@ -142,6 +177,7 @@ def test_portfolio_organization_page_edit_access(self): self.app.set_user(self.user.username) self.user.portfolio = self.portfolio self.user.portfolio_additional_permissions = [ + User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO, User.UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] self.portfolio.city = "Los Angeles" @@ -210,6 +246,7 @@ def test_portfolio_org_name(self): self.app.set_user(self.user.username) self.user.portfolio = self.portfolio self.user.portfolio_additional_permissions = [ + User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO, User.UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] self.user.save() @@ -227,6 +264,7 @@ def test_domain_org_name_address_content(self): self.app.set_user(self.user.username) self.user.portfolio = self.portfolio self.user.portfolio_additional_permissions = [ + User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO, User.UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] self.user.save() @@ -245,6 +283,7 @@ def test_domain_org_name_address_form(self): self.app.set_user(self.user.username) self.user.portfolio = self.portfolio self.user.portfolio_additional_permissions = [ + User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO, User.UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] self.user.save() From 867ccd097e32c29493da3e75241bbe4053b262b9 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 30 Jul 2024 20:21:41 -0400 Subject: [PATCH 05/16] remove migration --- ...r_user_portfolio_additional_permissions.py | 39 ------------------- src/registrar/models/user.py | 2 +- 2 files changed, 1 insertion(+), 40 deletions(-) delete mode 100644 src/registrar/migrations/0114_alter_user_portfolio_additional_permissions.py diff --git a/src/registrar/migrations/0114_alter_user_portfolio_additional_permissions.py b/src/registrar/migrations/0114_alter_user_portfolio_additional_permissions.py deleted file mode 100644 index 27f6e699f..000000000 --- a/src/registrar/migrations/0114_alter_user_portfolio_additional_permissions.py +++ /dev/null @@ -1,39 +0,0 @@ -# Generated by Django 4.2.10 on 2024-07-30 23:58 - -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"), - ("edit_domains", "User is a manager on a domain"), - ("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"), - ("edit_portfolio", "Edit organization"), - ("view_portfolio", "View organization"), - ], - max_length=50, - ), - blank=True, - help_text="Select one or more additional permissions.", - null=True, - size=None, - ), - ), - ] diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 48e6dc420..f4d7635cd 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -89,8 +89,8 @@ class UserPortfolioPermissionChoices(models.TextChoices): VIEW_CREATED_REQUESTS = "view_created_requests", "View created requests" EDIT_REQUESTS = "edit_requests", "Create and edit requests" - EDIT_PORTFOLIO = "edit_portfolio", "Edit organization" VIEW_PORTFOLIO = "view_portfolio", "View organization" + EDIT_PORTFOLIO = "edit_portfolio", "Edit organization" PORTFOLIO_ROLE_PERMISSIONS = { UserPortfolioRoleChoices.ORGANIZATION_ADMIN: [ From 6a94da8a5d96cc0788cb375525ec763bfc3e3373 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 30 Jul 2024 20:22:53 -0400 Subject: [PATCH 06/16] cleanup --- src/registrar/models/user.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index f4d7635cd..19097a96e 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -99,12 +99,14 @@ class UserPortfolioPermissionChoices(models.TextChoices): UserPortfolioPermissionChoices.EDIT_MEMBER, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, UserPortfolioPermissionChoices.EDIT_REQUESTS, + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ], UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY: [ UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, UserPortfolioPermissionChoices.VIEW_MEMBER, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, ], UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, From 3fef7664e43c4341088038829f8718c9eb8e1b5f Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 31 Jul 2024 13:59:00 -0400 Subject: [PATCH 07/16] cleanup --- src/registrar/templates/includes/finish_profile_form.html | 4 ---- src/registrar/tests/test_views_portfolio.py | 1 + 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/registrar/templates/includes/finish_profile_form.html b/src/registrar/templates/includes/finish_profile_form.html index 58bc604ae..1806c2603 100644 --- a/src/registrar/templates/includes/finish_profile_form.html +++ b/src/registrar/templates/includes/finish_profile_form.html @@ -68,10 +68,6 @@

What contact information should we use to reach you?

{% endwith %} {% endwith %} - {% with toggleable_input=True toggleable_label=True group_classes="usa-form-editable padding-top-2" %} - {% input_with_errors form.title %} - {% endwith %} - {% with toggleable_input=True toggleable_label=True group_classes="usa-form-editable padding-top-2" %} {% with add_class="usa-input--medium" %} {% input_with_errors form.phone %} diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 21b794cf7..0554d9908 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -201,6 +201,7 @@ def test_navigation_links_hidden_when_user_not_have_permission(self): self.app.set_user(self.user.username) self.user.portfolio = self.portfolio self.user.portfolio_additional_permissions = [ + User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO, User.UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, User.UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, ] From 6523fca640b88ecac5de5befbd4403832663c80f Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 31 Jul 2024 18:16:51 -0400 Subject: [PATCH 08/16] test clean up --- src/registrar/tests/test_views_portfolio.py | 43 ++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 0554d9908..a870f771a 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -224,7 +224,48 @@ def test_navigation_links_hidden_when_user_not_have_permission(self): # removing non-basic portfolio perms, which should remove domains # and domain requests from nav - self.user.portfolio_additional_permissions = [] + self.user.portfolio_additional_permissions = [User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO] + self.user.save() + self.user.refresh_from_db() + + portfolio_page = self.app.get(reverse("home")).follow() + + self.assertContains(portfolio_page, self.portfolio.organization_name) + self.assertContains(portfolio_page, "

Organization

") + self.assertNotContains(portfolio_page, '

Domains

') + self.assertNotContains( + portfolio_page, reverse("portfolio-domains", kwargs={"portfolio_id": self.portfolio.pk}) + ) + self.assertNotContains( + portfolio_page, reverse("portfolio-domain-requests", kwargs={"portfolio_id": self.portfolio.pk}) + ) + + @less_console_noise_decorator + def test_navigation_links_hidden_when_user_not_have_role(self): + """Test that admin / memmber roles are associated with the right access""" + self.app.set_user(self.user.username) + self.user.portfolio = self.portfolio + self.user.portfolio_roles = [User.UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + self.user.save() + self.user.refresh_from_db() + with override_flag("organization_feature", active=True): + # This will redirect the user to the portfolio page. + # Follow implicity checks if our redirect is working. + portfolio_page = self.app.get(reverse("home")).follow() + # Assert that we're on the right page + self.assertContains(portfolio_page, self.portfolio.organization_name) + self.assertNotContains(portfolio_page, "

Organization

") + self.assertContains(portfolio_page, '

Domains

') + self.assertContains( + portfolio_page, reverse("portfolio-domains", kwargs={"portfolio_id": self.portfolio.pk}) + ) + self.assertContains( + portfolio_page, reverse("portfolio-domain-requests", kwargs={"portfolio_id": self.portfolio.pk}) + ) + + # removing non-basic portfolio role, which should remove domains + # and domain requests from nav + self.user.portfolio_roles = [User.UserPortfolioRoleChoices.ORGANIZATION_MEMBER] self.user.save() self.user.refresh_from_db() From 165e27307bd0fb79cff17d1d9abed05cc2aaad3b Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 1 Aug 2024 14:10:20 -0400 Subject: [PATCH 09/16] fix federal agency --- src/registrar/templates/portfolio_organization.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/portfolio_organization.html b/src/registrar/templates/portfolio_organization.html index cc9cf5b6a..b17d18b62 100644 --- a/src/registrar/templates/portfolio_organization.html +++ b/src/registrar/templates/portfolio_organization.html @@ -35,7 +35,7 @@

Organization

{% csrf_token %}

Federal agency

- {{ portfolio }} + {{ portfolio.federal_agency }}

{% input_with_errors form.address_line1 %} {% input_with_errors form.address_line2 %} @@ -54,7 +54,7 @@

Federal agency

{% else %}

Federal agency

- {{ portfolio }} + {{ portfolio.federal_agency }}

{% if form.address_line1.value is not None %} {% include "includes/input_read_only.html" with field=form.address_line1 %} From aedb05dd34852358504c7fe7ff17117f14c2342e Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 1 Aug 2024 14:16:45 -0400 Subject: [PATCH 10/16] fix unit test --- src/registrar/tests/test_views_portfolio.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index a870f771a..3ec8efc62 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -315,8 +315,9 @@ def test_domain_org_name_address_content(self): self.portfolio.organization_name = "Hotel California" self.portfolio.save() page = self.app.get(reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk})) - # Once in the sidenav, once in the main nav, once in the form - self.assertContains(page, "Hotel California", count=3) + # Once in the sidenav, once in the main nav + self.assertContains(page, "Hotel California", count=2) + self.assertContains(page, "Non-Federal Agency") @less_console_noise_decorator def test_domain_org_name_address_form(self): From 3e70b344e3db77848f0486e64089c6430e915be9 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 1 Aug 2024 14:26:13 -0400 Subject: [PATCH 11/16] cleanup --- src/registrar/models/user.py | 2 +- src/registrar/templates/includes/finish_profile_form.html | 2 +- src/registrar/templates/portfolio_organization.html | 7 ++----- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 19097a96e..c5895cdf0 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -64,7 +64,7 @@ class VerificationTypeChoices(models.TextChoices): class UserPortfolioRoleChoices(models.TextChoices): """ - Roles make it easier for admins to look at groups of users + Roles are containers of UserPortfolioPermissionChoices """ ORGANIZATION_ADMIN = "organization_admin", "Admin" diff --git a/src/registrar/templates/includes/finish_profile_form.html b/src/registrar/templates/includes/finish_profile_form.html index 1806c2603..42a2de1a5 100644 --- a/src/registrar/templates/includes/finish_profile_form.html +++ b/src/registrar/templates/includes/finish_profile_form.html @@ -53,7 +53,7 @@

What contact information should we use to reach you?

{% endwith %} - {% with show_edit_button=True show_readonly=True group_classes="usa-form-editable padding-top-2" %} + {% with toggleable_input=True toggleable_label=True group_classes="usa-form-editable padding-top-2" %} {% input_with_errors form.title %} {% endwith %} diff --git a/src/registrar/templates/portfolio_organization.html b/src/registrar/templates/portfolio_organization.html index b17d18b62..51adba3d9 100644 --- a/src/registrar/templates/portfolio_organization.html +++ b/src/registrar/templates/portfolio_organization.html @@ -31,7 +31,7 @@

Organization

{% include "includes/form_errors.html" with form=form %} {% include "includes/required_fields.html" %} -
+ {% csrf_token %}

Federal agency

@@ -44,10 +44,7 @@

Federal agency

{% with add_class="usa-input--small" %} {% input_with_errors form.zipcode %} {% endwith %} -
From d8fd49fa10264c5193b82da0a0c48162e647b32d Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 2 Aug 2024 13:19:09 -0400 Subject: [PATCH 12/16] cleanup --- src/registrar/tests/test_views_portfolio.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 49a5679c1..f21321e96 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -300,7 +300,7 @@ def test_domain_org_name_address_content(self): self.portfolio.organization_name = "Hotel California" self.portfolio.save() - page = self.app.get(reverse("portfolio-organization")) + page = self.app.get(reverse("organization")) # Once in the sidenav, once in the main nav self.assertContains(page, "Hotel California", count=2) self.assertContains(page, "Non-Federal Agency") From 53d9f69420837f1d6cb06dbc26d5f332dd343e75 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 2 Aug 2024 13:46:00 -0400 Subject: [PATCH 13/16] clean up merge errors --- src/registrar/tests/test_views_portfolio.py | 12 ++++++------ src/registrar/views/portfolios.py | 1 + 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f21321e96..05d0b61ef 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -155,7 +155,7 @@ def test_portfolio_organization_page_read_only(self): self.user.save() self.user.refresh_from_db() with override_flag("organization_feature", active=True): - response = self.app.get(reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk})) + response = self.app.get(reverse("organization")) # Assert the response is a 200 self.assertEqual(response.status_code, 200) # The label for Federal agency will always be a h4 @@ -179,7 +179,7 @@ def test_portfolio_organization_page_edit_access(self): self.user.save() self.user.refresh_from_db() with override_flag("organization_feature", active=True): - response = self.app.get(reverse("portfolio-organization", kwargs={"portfolio_id": self.portfolio.pk})) + response = self.app.get(reverse("organization")) # Assert the response is a 200 self.assertEqual(response.status_code, 200) # The label for Federal agency will always be a h4 @@ -243,10 +243,10 @@ def test_navigation_links_hidden_when_user_not_have_role(self): self.assertNotContains(portfolio_page, "

Organization

") self.assertContains(portfolio_page, '

Domains

') self.assertContains( - portfolio_page, reverse("portfolio-domains", kwargs={"portfolio_id": self.portfolio.pk}) + portfolio_page, reverse("domains") ) self.assertContains( - portfolio_page, reverse("portfolio-domain-requests", kwargs={"portfolio_id": self.portfolio.pk}) + portfolio_page, reverse("domain-requests") ) # removing non-basic portfolio role, which should remove domains @@ -261,10 +261,10 @@ def test_navigation_links_hidden_when_user_not_have_role(self): self.assertContains(portfolio_page, "

Organization

") self.assertNotContains(portfolio_page, '

Domains

') self.assertNotContains( - portfolio_page, reverse("portfolio-domains", kwargs={"portfolio_id": self.portfolio.pk}) + portfolio_page, reverse("domains") ) self.assertNotContains( - portfolio_page, reverse("portfolio-domain-requests", kwargs={"portfolio_id": self.portfolio.pk}) + portfolio_page, reverse("domain-requests") ) @less_console_noise_decorator diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 4b7374d81..63ebbaa01 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -48,6 +48,7 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): def get_context_data(self, **kwargs): """Add additional context data to the template.""" context = super().get_context_data(**kwargs) + context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission() return context def get_object(self, queryset=None): From 94cda46baf174ab91aa982395b04d1836df81bc9 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 2 Aug 2024 13:53:17 -0400 Subject: [PATCH 14/16] solidify domains permissions --- .../templates/includes/domains_table.html | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/registrar/templates/includes/domains_table.html b/src/registrar/templates/includes/domains_table.html index 348ab21d7..64eddec41 100644 --- a/src/registrar/templates/includes/domains_table.html +++ b/src/registrar/templates/includes/domains_table.html @@ -1,15 +1,15 @@ {% load static %} -
-
- {% if portfolio is None %} +
+
+ {% if not has_domains_portfolio_permission %}

Domains

{% else %} {% endif %} -
+
-
+

Domains

- {% if portfolio %} + {% if has_domains_portfolio_permission %}
Filter by
@@ -150,7 +150,7 @@

Status

Domain name Expires Status - {% if portfolio %} + {% if has_domains_portfolio_permission %} Suborganization {% endif %} Date: Fri, 2 Aug 2024 13:55:14 -0400 Subject: [PATCH 15/16] solidify requests table perms --- src/registrar/templates/includes/domain_requests_table.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/includes/domain_requests_table.html b/src/registrar/templates/includes/domain_requests_table.html index ad91699ef..30c206741 100644 --- a/src/registrar/templates/includes/domain_requests_table.html +++ b/src/registrar/templates/includes/domain_requests_table.html @@ -2,7 +2,7 @@
- {% if portfolio is None %} + {% if not has_domain_requests_portfolio_permission %}

Domain requests

From 85f51ef142178e5d35ce4b96f24c41a07521a5f3 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 2 Aug 2024 14:11:21 -0400 Subject: [PATCH 16/16] Fix unit tests --- src/registrar/tests/test_views_domain.py | 3 ++- src/registrar/tests/test_views_portfolio.py | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index e24085b30..00fcc8fe0 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -7,6 +7,7 @@ from api.tests.common import less_console_noise_decorator from registrar.models.portfolio import Portfolio +from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from .common import MockEppLib, MockSESClient, create_user # type: ignore from django_webtest import WebTest # type: ignore import boto3_mocking # type: ignore @@ -326,7 +327,7 @@ def test_domain_readonly_on_detail_page(self): phone="8003111234", title="test title", portfolio=portfolio, - portfolio_roles=[User.UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + portfolio_roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], ) domain, _ = Domain.objects.get_or_create(name="bogusdomain.gov") DomainInformation.objects.get_or_create(creator=user, domain=domain, portfolio=portfolio) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 49b4db271..f72130d94 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -151,7 +151,7 @@ def test_portfolio_organization_page_read_only(self): self.app.set_user(self.user.username) self.user.portfolio = self.portfolio self.portfolio.city = "Los Angeles" - self.user.portfolio_additional_permissions = [User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO] + self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] self.portfolio.save() self.user.save() self.user.refresh_from_db() @@ -172,8 +172,8 @@ def test_portfolio_organization_page_edit_access(self): self.app.set_user(self.user.username) self.user.portfolio = self.portfolio self.user.portfolio_additional_permissions = [ - User.UserPortfolioPermissionChoices.VIEW_PORTFOLIO, - User.UserPortfolioPermissionChoices.EDIT_PORTFOLIO, + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] self.portfolio.city = "Los Angeles" self.portfolio.save()