From a6de5293c2fdb51971e87f6ac7bc89b36061dbaf Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 23 Jan 2025 15:15:24 -0700 Subject: [PATCH 01/22] Core logic --- src/registrar/forms/domain_request_wizard.py | 20 ++++++++++++++++++-- src/registrar/utility/waffle.py | 16 ++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 7c9dcb180..c1ea2dda1 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -2,7 +2,8 @@ from __future__ import annotations # allows forward references in annotations import logging from api.views import DOMAIN_API_MESSAGES from phonenumber_field.formfields import PhoneNumberField # type: ignore - +from registrar.models.portfolio import Portfolio +from registrar.utility.waffle import flag_is_active_anywhere from django import forms from django.core.validators import RegexValidator, MaxLengthValidator from django.utils.safestring import mark_safe @@ -321,7 +322,7 @@ class OrganizationContactForm(RegistrarForm): # if it has been filled in when required. # uncomment to see if modelChoiceField can be an arg later required=False, - queryset=FederalAgency.objects.exclude(agency__in=excluded_agencies), + queryset=FederalAgency.objects.none(), widget=ComboboxWidget, ) organization_name = forms.CharField( @@ -363,6 +364,21 @@ class OrganizationContactForm(RegistrarForm): label="Urbanization (required for Puerto Rico only)", ) + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + # Set the queryset for federal agency. + # If the organization_requests flag is active, we hide data that exists in portfolios. + # NOTE: This function assumes that the federal_agency field was first set to None if a portfolio exists. + federal_agency_queryset = FederalAgency.objects.exclude(agency__in=self.excluded_agencies) + if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): + # Exclude both predefined agencies and those matching portfolio names in one query + federal_agency_queryset = federal_agency_queryset.exclude( + agency__in=Portfolio.objects.values_list("organization_name", flat=True) + ) + + self.fields["federal_agency"].queryset = federal_agency_queryset + def clean_federal_agency(self): """Require something to be selected when this is a federal agency.""" federal_agency = self.cleaned_data.get("federal_agency", None) diff --git a/src/registrar/utility/waffle.py b/src/registrar/utility/waffle.py index a78799e4c..f97a18874 100644 --- a/src/registrar/utility/waffle.py +++ b/src/registrar/utility/waffle.py @@ -1,5 +1,6 @@ from django.http import HttpRequest from waffle.decorators import flag_is_active +from waffle.models import get_waffle_flag_model def flag_is_active_for_user(user, flag_name): @@ -10,3 +11,18 @@ def flag_is_active_for_user(user, flag_name): request = HttpRequest() request.user = user return flag_is_active(request, flag_name) + + +def flag_is_active_anywhere(flag_name): + """Checks if the given flag name is active for anyone, anywhere. + More specifically, it checks on flag.everyone or flag.users.exists(). + Does not check self.superuser, self.staff or self.group. + + This function effectively behaves like a switch: + If said flag is enabled for someone, somewhere - return true. + Otherwise - return false. + """ + flag = get_waffle_flag_model().get(flag_name) + if flag.everyone is None: + return flag.users.exists() + return flag.everyone From a4f1abc0011556e8557a17909edeb58e1e3cd693 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 24 Jan 2025 08:12:10 -0700 Subject: [PATCH 02/22] add check --- src/registrar/models/domain_request.py | 26 ++++++++++++++++++++++++-- src/registrar/views/domain_request.py | 10 +--------- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 3d3aac769..49fddbe18 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -14,8 +14,7 @@ from registrar.utility.constants import BranchChoices from auditlog.models import LogEntry from django.core.exceptions import ValidationError -from registrar.utility.waffle import flag_is_active_for_user - +from registrar.utility.waffle import flag_is_active_for_user, flag_is_active_anywhere from .utility.time_stamped_model import TimeStampedModel from ..utility.email import send_templated_email, EmailSendingError from itertools import chain @@ -1289,6 +1288,29 @@ class DomainRequest(TimeStampedModel): return True return False + def unlock_organization_contact(self) -> bool: + """Unlocks the organization_contact step.""" + + # NOTE: This check may struggle with a high number of portfolios, consider something else then. + if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): + # Check if the current federal agency is an outlawed one + Portfolio = apps.get_model("registrar.Portfolio") + return FederalAgency.objects.exclude( + agency__in=Portfolio.objects.values_list("organization_name", flat=True), + ).filter(agency=self.federal_agency).exists() + + # NOTE: Shouldn't this be an AND on all required fields? + return ( + self.domain_request.federal_agency is not None + or self.domain_request.organization_name is not None + or self.domain_request.address_line1 is not None + or self.domain_request.city is not None + or self.domain_request.state_territory is not None + or self.domain_request.zipcode is not None + or self.domain_request.urbanization is not None + ) + + # ## Form policies ## # # # These methods control what questions need to be answered by applicants diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index 9754b0d0c..b530dc7ae 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -107,15 +107,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): Step.TRIBAL_GOVERNMENT: lambda self: self.domain_request.tribe_name is not None, Step.ORGANIZATION_FEDERAL: lambda self: self.domain_request.federal_type is not None, Step.ORGANIZATION_ELECTION: lambda self: self.domain_request.is_election_board is not None, - Step.ORGANIZATION_CONTACT: lambda self: ( - self.domain_request.federal_agency is not None - or self.domain_request.organization_name is not None - or self.domain_request.address_line1 is not None - or self.domain_request.city is not None - or self.domain_request.state_territory is not None - or self.domain_request.zipcode is not None - or self.domain_request.urbanization is not None - ), + Step.ORGANIZATION_CONTACT: lambda self: self.from_model("unlock_organization_contact", False), Step.ABOUT_YOUR_ORGANIZATION: lambda self: self.domain_request.about_your_organization is not None, Step.SENIOR_OFFICIAL: lambda self: self.domain_request.senior_official is not None, Step.CURRENT_SITES: lambda self: ( From 297029a447504749af194fab2065240634f21a6a Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 27 Jan 2025 11:14:37 -0700 Subject: [PATCH 03/22] Add UI logic --- src/registrar/models/domain.py | 1 + src/registrar/models/domain_request.py | 151 +----------------- .../includes/request_review_steps.html | 2 +- src/registrar/views/domain_request.py | 9 +- 4 files changed, 12 insertions(+), 151 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index cb481db7a..c869dfa67 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -244,6 +244,7 @@ class Domain(TimeStampedModel, DomainHelper): is called in the validate function on the request/domain page throws- RegistryError or InvalidDomainError""" + return True if not cls.string_could_be_domain(domain): logger.warning("Not a valid domain: %s" % str(domain)) # throw invalid domain error so that it can be caught in diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 49fddbe18..1456742dd 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -1290,8 +1290,6 @@ class DomainRequest(TimeStampedModel): def unlock_organization_contact(self) -> bool: """Unlocks the organization_contact step.""" - - # NOTE: This check may struggle with a high number of portfolios, consider something else then. if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): # Check if the current federal agency is an outlawed one Portfolio = apps.get_model("registrar.Portfolio") @@ -1301,16 +1299,15 @@ class DomainRequest(TimeStampedModel): # NOTE: Shouldn't this be an AND on all required fields? return ( - self.domain_request.federal_agency is not None - or self.domain_request.organization_name is not None - or self.domain_request.address_line1 is not None - or self.domain_request.city is not None - or self.domain_request.state_territory is not None - or self.domain_request.zipcode is not None - or self.domain_request.urbanization is not None + self.federal_agency is not None + or self.organization_name is not None + or self.address_line1 is not None + or self.city is not None + or self.state_territory is not None + or self.zipcode is not None + or self.urbanization is not None ) - # ## Form policies ## # # # These methods control what questions need to be answered by applicants @@ -1408,140 +1405,6 @@ class DomainRequest(TimeStampedModel): names = [n for n in [self.cisa_representative_first_name, self.cisa_representative_last_name] if n] return " ".join(names) if names else "Unknown" - def _is_federal_complete(self): - # Federal -> "Federal government branch" page can't be empty + Federal Agency selection can't be None - return not (self.federal_type is None or self.federal_agency is None) - - def _is_interstate_complete(self): - # Interstate -> "About your organization" page can't be empty - return self.about_your_organization is not None - - def _is_state_or_territory_complete(self): - # State -> ""Election office" page can't be empty - return self.is_election_board is not None - - def _is_tribal_complete(self): - # Tribal -> "Tribal name" and "Election office" page can't be empty - return self.tribe_name is not None and self.is_election_board is not None - - def _is_county_complete(self): - # County -> "Election office" page can't be empty - return self.is_election_board is not None - - def _is_city_complete(self): - # City -> "Election office" page can't be empty - return self.is_election_board is not None - - def _is_special_district_complete(self): - # Special District -> "Election office" and "About your organization" page can't be empty - return self.is_election_board is not None and self.about_your_organization is not None - - # Do we still want to test this after creator is autogenerated? Currently it went back to being selectable - def _is_creator_complete(self): - return self.creator is not None - - def _is_organization_name_and_address_complete(self): - return not ( - self.organization_name is None - and self.address_line1 is None - and self.city is None - and self.state_territory is None - and self.zipcode is None - ) - - def _is_senior_official_complete(self): - return self.senior_official is not None - - def _is_requested_domain_complete(self): - return self.requested_domain is not None - - def _is_purpose_complete(self): - return self.purpose is not None - - def _has_other_contacts_and_filled(self): - # Other Contacts Radio button is Yes and if all required fields are filled - return ( - self.has_other_contacts() - and self.other_contacts.filter( - first_name__isnull=False, - last_name__isnull=False, - title__isnull=False, - email__isnull=False, - phone__isnull=False, - ).exists() - ) - - def _has_no_other_contacts_gives_rationale(self): - # Other Contacts Radio button is No and a rationale is provided - return self.has_other_contacts() is False and self.no_other_contacts_rationale is not None - - def _is_other_contacts_complete(self): - if self._has_other_contacts_and_filled() or self._has_no_other_contacts_gives_rationale(): - return True - return False - - def _cisa_rep_check(self): - # Either does not have a CISA rep, OR has a CISA rep + both first name - # and last name are NOT empty and are NOT an empty string - to_return = ( - self.has_cisa_representative is True - and self.cisa_representative_first_name is not None - and self.cisa_representative_first_name != "" - and self.cisa_representative_last_name is not None - and self.cisa_representative_last_name != "" - ) or self.has_cisa_representative is False - - return to_return - - def _anything_else_radio_button_and_text_field_check(self): - # Anything else boolean is True + filled text field and it's not an empty string OR the boolean is No - return ( - self.has_anything_else_text is True and self.anything_else is not None and self.anything_else != "" - ) or self.has_anything_else_text is False - - def _is_additional_details_complete(self): - return self._cisa_rep_check() and self._anything_else_radio_button_and_text_field_check() - - def _is_policy_acknowledgement_complete(self): - return self.is_policy_acknowledged is not None - - def _is_general_form_complete(self, request): - return ( - self._is_creator_complete() - and self._is_organization_name_and_address_complete() - and self._is_senior_official_complete() - and self._is_requested_domain_complete() - and self._is_purpose_complete() - and self._is_other_contacts_complete() - and self._is_additional_details_complete() - and self._is_policy_acknowledgement_complete() - ) - - def _form_complete(self, request): - match self.generic_org_type: - case DomainRequest.OrganizationChoices.FEDERAL: - is_complete = self._is_federal_complete() - case DomainRequest.OrganizationChoices.INTERSTATE: - is_complete = self._is_interstate_complete() - case DomainRequest.OrganizationChoices.STATE_OR_TERRITORY: - is_complete = self._is_state_or_territory_complete() - case DomainRequest.OrganizationChoices.TRIBAL: - is_complete = self._is_tribal_complete() - case DomainRequest.OrganizationChoices.COUNTY: - is_complete = self._is_county_complete() - case DomainRequest.OrganizationChoices.CITY: - is_complete = self._is_city_complete() - case DomainRequest.OrganizationChoices.SPECIAL_DISTRICT: - is_complete = self._is_special_district_complete() - case DomainRequest.OrganizationChoices.SCHOOL_DISTRICT: - is_complete = True - case _: - # NOTE: Shouldn't happen, this is only if somehow they didn't choose an org type - is_complete = False - if not is_complete or not self._is_general_form_complete(request): - return False - return True - """The following converted_ property methods get field data from this domain request's portfolio, if there is an associated portfolio. If not, they return data from the domain request model.""" diff --git a/src/registrar/templates/includes/request_review_steps.html b/src/registrar/templates/includes/request_review_steps.html index 6151d01a8..96ba7f151 100644 --- a/src/registrar/templates/includes/request_review_steps.html +++ b/src/registrar/templates/includes/request_review_steps.html @@ -41,7 +41,7 @@ {% endif %} {% if step == Step.ORGANIZATION_CONTACT %} - {% if domain_request.organization_name %} + {% if domain_request.unlock_organization_contact %} {% with title=form_titles|get_item:step value=domain_request %} {% include "includes/summary_item.html" with title=title value=value heading_level=heading_level editable=is_editable edit_link=domain_request_url address='true' %} {% endwith %} diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index b530dc7ae..f200cc3ae 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -434,12 +434,8 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): requested_domain_name = self.domain_request.requested_domain.name context = {} - - # Note: we will want to consolidate the non_org_steps_complete check into the same check that - # org_steps_complete is using at some point. - non_org_steps_complete = DomainRequest._form_complete(self.domain_request, self.request) org_steps_complete = len(self.db_check_for_unlocking_steps()) == len(self.steps) - if (not self.is_portfolio and non_org_steps_complete) or (self.is_portfolio and org_steps_complete): + if org_steps_complete: context = { "form_titles": self.titles, "steps": self.steps, @@ -774,7 +770,8 @@ class Review(DomainRequestWizard): forms = [] # type: ignore def get_context_data(self): - if DomainRequest._form_complete(self.domain_request, self.request) is False: + form_complete = len(self.db_check_for_unlocking_steps()) == len(self.steps) + if form_complete is False: logger.warning("User arrived at review page with an incomplete form.") context = super().get_context_data() context["Step"] = self.get_step_enum().__members__ From 1f9efb7fc0baebd0c1c0d34bf4f835ae4e992a1e Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 27 Jan 2025 12:56:08 -0700 Subject: [PATCH 04/22] Add unit test for waffle utility --- src/registrar/forms/domain_request_wizard.py | 2 +- src/registrar/tests/test_utilities.py | 40 +++++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 5668b3757..d27ccde89 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -322,6 +322,7 @@ class OrganizationContactForm(RegistrarForm): # if it has been filled in when required. # uncomment to see if modelChoiceField can be an arg later required=False, + # We populate this queryset in init. We want to exclude agencies with a portfolio. queryset=FederalAgency.objects.none(), widget=ComboboxWidget, ) @@ -369,7 +370,6 @@ class OrganizationContactForm(RegistrarForm): # Set the queryset for federal agency. # If the organization_requests flag is active, we hide data that exists in portfolios. - # NOTE: This function assumes that the federal_agency field was first set to None if a portfolio exists. federal_agency_queryset = FederalAgency.objects.exclude(agency__in=self.excluded_agencies) if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): # Exclude both predefined agencies and those matching portfolio names in one query diff --git a/src/registrar/tests/test_utilities.py b/src/registrar/tests/test_utilities.py index 5a2234d66..60b74cd60 100644 --- a/src/registrar/tests/test_utilities.py +++ b/src/registrar/tests/test_utilities.py @@ -1,7 +1,8 @@ from django.test import TestCase from registrar.models import User from waffle.testutils import override_flag -from registrar.utility.waffle import flag_is_active_for_user +from waffle.models import get_waffle_flag_model +from registrar.utility.waffle import flag_is_active_for_user, flag_is_active_anywhere class FlagIsActiveForUserTest(TestCase): @@ -21,3 +22,40 @@ class FlagIsActiveForUserTest(TestCase): # Test that the flag is inactive for the user is_active = flag_is_active_for_user(self.user, "test_flag") self.assertFalse(is_active) + + +class TestFlagIsActiveAnywhere(TestCase): + def setUp(self): + self.user = User.objects.create_user(username="testuser") + self.flag_name = "test_flag" + + @override_flag("test_flag", active=True) + def test_flag_active_for_everyone(self): + """Test when flag is active for everyone""" + is_active = flag_is_active_anywhere("test_flag") + self.assertTrue(is_active) + + @override_flag("test_flag", active=False) + def test_flag_inactive_for_everyone(self): + """Test when flag is inactive for everyone""" + is_active = flag_is_active_anywhere("test_flag") + self.assertFalse(is_active) + + def test_flag_active_for_some_users(self): + """Test when flag is active for specific users""" + flag, _ = get_waffle_flag_model().objects.get_or_create(name="test_flag") + flag.everyone = None + flag.save() + flag.users.add(self.user) + + is_active = flag_is_active_anywhere("test_flag") + self.assertTrue(is_active) + + def test_flag_inactive_with_no_users(self): + """Test when flag has no users and everyone is None""" + flag, _ = get_waffle_flag_model().objects.get_or_create(name="test_flag") + flag.everyone = None + flag.save() + + is_active = flag_is_active_anywhere("test_flag") + self.assertFalse(is_active) From bdb3875f1210671b9379ecccfdf0249b645ab382 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 27 Jan 2025 14:07:16 -0700 Subject: [PATCH 05/22] Add tests for unlock_organization_contact --- src/registrar/tests/test_views_request.py | 41 +++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/registrar/tests/test_views_request.py b/src/registrar/tests/test_views_request.py index 81beba604..8e4871f79 100644 --- a/src/registrar/tests/test_views_request.py +++ b/src/registrar/tests/test_views_request.py @@ -3221,6 +3221,47 @@ class TestDomainRequestWizard(TestWithUser, WebTest): federal_agency.delete() domain_request.delete() + @override_flag("organization_feature", active=True) + @override_flag("organization_requests", active=True) + @less_console_noise_decorator + def test_unlock_organization_contact_flags_enabled(self): + """Tests unlock_organization_contact when agency exists in a portfolio""" + # Create a federal agency + federal_agency = FederalAgency.objects.create(agency="Portfolio Agency") + + # Create a portfolio with matching organization name + Portfolio.objects.create( + creator=self.user, + organization_name=federal_agency.agency + ) + + # Create domain request with the portfolio agency + domain_request = completed_domain_request( + federal_agency=federal_agency, + user=self.user + ) + self.assertFalse(domain_request.unlock_organization_contact()) + + @override_flag("organization_feature", active=False) + @override_flag("organization_requests", active=False) + @less_console_noise_decorator + def test_unlock_organization_contact_flags_disabled(self): + """Tests unlock_organization_contact when organization flags are disabled""" + # Create a federal agency + federal_agency = FederalAgency.objects.create(agency="Portfolio Agency") + + # Create a portfolio with matching organization name + Portfolio.objects.create( + creator=self.user, + organization_name=federal_agency.agency + ) + + domain_request = completed_domain_request( + federal_agency=federal_agency, + user=self.user + ) + self.assertTrue(domain_request.unlock_organization_contact()) + class TestPortfolioDomainRequestViewonly(TestWithUser, WebTest): From ad79557a55598cc4f67c33d11b01acf76a7024be Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 28 Jan 2025 09:58:41 -0700 Subject: [PATCH 06/22] lint and fix test --- src/registrar/forms/domain_request_wizard.py | 4 +-- src/registrar/models/domain.py | 1 - src/registrar/models/domain_request.py | 10 +++++-- src/registrar/tests/test_utilities.py | 10 +++---- src/registrar/tests/test_views_request.py | 30 ++++++-------------- 5 files changed, 23 insertions(+), 32 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index d27ccde89..0cf82558b 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -367,7 +367,7 @@ class OrganizationContactForm(RegistrarForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - + # Set the queryset for federal agency. # If the organization_requests flag is active, we hide data that exists in portfolios. federal_agency_queryset = FederalAgency.objects.exclude(agency__in=self.excluded_agencies) @@ -376,7 +376,7 @@ class OrganizationContactForm(RegistrarForm): federal_agency_queryset = federal_agency_queryset.exclude( agency__in=Portfolio.objects.values_list("organization_name", flat=True) ) - + self.fields["federal_agency"].queryset = federal_agency_queryset def clean_federal_agency(self): diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index c869dfa67..cb481db7a 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -244,7 +244,6 @@ class Domain(TimeStampedModel, DomainHelper): is called in the validate function on the request/domain page throws- RegistryError or InvalidDomainError""" - return True if not cls.string_could_be_domain(domain): logger.warning("Not a valid domain: %s" % str(domain)) # throw invalid domain error so that it can be caught in diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 1456742dd..2d0dd50e5 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -1293,9 +1293,13 @@ class DomainRequest(TimeStampedModel): if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): # Check if the current federal agency is an outlawed one Portfolio = apps.get_model("registrar.Portfolio") - return FederalAgency.objects.exclude( - agency__in=Portfolio.objects.values_list("organization_name", flat=True), - ).filter(agency=self.federal_agency).exists() + return ( + FederalAgency.objects.exclude( + agency__in=Portfolio.objects.values_list("organization_name", flat=True), + ) + .filter(agency=self.federal_agency) + .exists() + ) # NOTE: Shouldn't this be an AND on all required fields? return ( diff --git a/src/registrar/tests/test_utilities.py b/src/registrar/tests/test_utilities.py index 60b74cd60..d882fdedd 100644 --- a/src/registrar/tests/test_utilities.py +++ b/src/registrar/tests/test_utilities.py @@ -28,29 +28,29 @@ class TestFlagIsActiveAnywhere(TestCase): def setUp(self): self.user = User.objects.create_user(username="testuser") self.flag_name = "test_flag" - + @override_flag("test_flag", active=True) def test_flag_active_for_everyone(self): """Test when flag is active for everyone""" is_active = flag_is_active_anywhere("test_flag") self.assertTrue(is_active) - + @override_flag("test_flag", active=False) def test_flag_inactive_for_everyone(self): """Test when flag is inactive for everyone""" is_active = flag_is_active_anywhere("test_flag") self.assertFalse(is_active) - + def test_flag_active_for_some_users(self): """Test when flag is active for specific users""" flag, _ = get_waffle_flag_model().objects.get_or_create(name="test_flag") flag.everyone = None flag.save() flag.users.add(self.user) - + is_active = flag_is_active_anywhere("test_flag") self.assertTrue(is_active) - + def test_flag_inactive_with_no_users(self): """Test when flag has no users and everyone is None""" flag, _ = get_waffle_flag_model().objects.get_or_create(name="test_flag") diff --git a/src/registrar/tests/test_views_request.py b/src/registrar/tests/test_views_request.py index 8e4871f79..c559a73c7 100644 --- a/src/registrar/tests/test_views_request.py +++ b/src/registrar/tests/test_views_request.py @@ -3228,18 +3228,12 @@ class TestDomainRequestWizard(TestWithUser, WebTest): """Tests unlock_organization_contact when agency exists in a portfolio""" # Create a federal agency federal_agency = FederalAgency.objects.create(agency="Portfolio Agency") - + # Create a portfolio with matching organization name - Portfolio.objects.create( - creator=self.user, - organization_name=federal_agency.agency - ) - + Portfolio.objects.create(creator=self.user, organization_name=federal_agency.agency) + # Create domain request with the portfolio agency - domain_request = completed_domain_request( - federal_agency=federal_agency, - user=self.user - ) + domain_request = completed_domain_request(federal_agency=federal_agency, user=self.user) self.assertFalse(domain_request.unlock_organization_contact()) @override_flag("organization_feature", active=False) @@ -3247,19 +3241,13 @@ class TestDomainRequestWizard(TestWithUser, WebTest): @less_console_noise_decorator def test_unlock_organization_contact_flags_disabled(self): """Tests unlock_organization_contact when organization flags are disabled""" - # Create a federal agency + # Create a federal agency federal_agency = FederalAgency.objects.create(agency="Portfolio Agency") - - # Create a portfolio with matching organization name - Portfolio.objects.create( - creator=self.user, - organization_name=federal_agency.agency - ) - domain_request = completed_domain_request( - federal_agency=federal_agency, - user=self.user - ) + # Create a portfolio with matching organization name + Portfolio.objects.create(creator=self.user, organization_name=federal_agency.agency) + + domain_request = completed_domain_request(federal_agency=federal_agency, user=self.user) self.assertTrue(domain_request.unlock_organization_contact()) From 0478d2bee6078859d5b96e1076de0608fe2459f4 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 28 Jan 2025 14:36:40 -0700 Subject: [PATCH 07/22] Fix unit tests --- src/registrar/models/domain.py | 1 + src/registrar/models/domain_request.py | 13 ++- .../includes/request_review_steps.html | 2 +- src/registrar/tests/test_models.py | 91 +++++++++++-------- src/registrar/utility/waffle.py | 11 ++- src/registrar/views/domain_request.py | 32 +++++-- 6 files changed, 97 insertions(+), 53 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index cb481db7a..c869dfa67 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -244,6 +244,7 @@ class Domain(TimeStampedModel, DomainHelper): is called in the validate function on the request/domain page throws- RegistryError or InvalidDomainError""" + return True if not cls.string_could_be_domain(domain): logger.warning("Not a valid domain: %s" % str(domain)) # throw invalid domain error so that it can be caught in diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 2d0dd50e5..c56d42c1e 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -1300,8 +1300,6 @@ class DomainRequest(TimeStampedModel): .filter(agency=self.federal_agency) .exists() ) - - # NOTE: Shouldn't this be an AND on all required fields? return ( self.federal_agency is not None or self.organization_name is not None @@ -1312,6 +1310,17 @@ class DomainRequest(TimeStampedModel): or self.urbanization is not None ) + def unlock_other_contacts(self) -> bool: + """Unlocks the other contacts step""" + other_contacts_filled_out = self.other_contacts.filter( + first_name__isnull=False, + last_name__isnull=False, + title__isnull=False, + email__isnull=False, + phone__isnull=False, + ).exists() + return (self.has_other_contacts() and other_contacts_filled_out) or self.no_other_contacts_rationale is not None + # ## Form policies ## # # # These methods control what questions need to be answered by applicants diff --git a/src/registrar/templates/includes/request_review_steps.html b/src/registrar/templates/includes/request_review_steps.html index 96ba7f151..f1b13f890 100644 --- a/src/registrar/templates/includes/request_review_steps.html +++ b/src/registrar/templates/includes/request_review_steps.html @@ -116,7 +116,7 @@ {% endif %} {% if step == Step.OTHER_CONTACTS %} - {% if domain_request.other_contacts.all %} + {% if domain_request.unlock_other_contacts %} {% with title=form_titles|get_item:step value=domain_request.other_contacts.all %} {% include "includes/summary_item.html" with title=title value=value heading_level=heading_level editable=is_editable edit_link=domain_request_url contact='true' list='true' %} {% endwith %} diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index d8db0f043..45f8a15f0 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1,9 +1,10 @@ from django.forms import ValidationError from django.test import TestCase from unittest.mock import patch - +from unittest.mock import Mock from django.test import RequestFactory - +from waffle.models import get_waffle_flag_model +from registrar.views.domain_request import DomainRequestWizard from registrar.models import ( Contact, DomainRequest, @@ -1692,11 +1693,20 @@ class TestDomainRequestIncomplete(TestCase): anything_else="Anything else", is_policy_acknowledged=True, creator=self.user, + city="fake", ) - self.domain_request.other_contacts.add(other) self.domain_request.current_websites.add(current) self.domain_request.alternative_domains.add(alt) + self.wizard = DomainRequestWizard() + self.wizard._domain_request = self.domain_request + self.wizard.request = Mock(user=self.user, session={}) + self.wizard.kwargs = {"id": self.domain_request.id} + + # We use both of these flags in the test. In the normal app these are generated normally. + # The alternative syntax is adding the decorator to each test. + get_waffle_flag_model().objects.get_or_create(name="organization_feature") + get_waffle_flag_model().objects.get_or_create(name="organization_requests") def tearDown(self): super().tearDown() @@ -1709,66 +1719,67 @@ class TestDomainRequestIncomplete(TestCase): super().tearDownClass() cls.user.delete() - @less_console_noise_decorator + # @less_console_noise_decorator def test_is_federal_complete(self): - self.assertTrue(self.domain_request._is_federal_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.federal_type = None self.domain_request.save() - self.assertFalse(self.domain_request._is_federal_complete()) + self.domain_request.refresh_from_db() + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_interstate_complete(self): self.domain_request.generic_org_type = DomainRequest.OrganizationChoices.INTERSTATE self.domain_request.about_your_organization = "Something something about your organization" self.domain_request.save() - self.assertTrue(self.domain_request._is_interstate_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.about_your_organization = None self.domain_request.save() - self.assertFalse(self.domain_request._is_interstate_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_state_or_territory_complete(self): self.domain_request.generic_org_type = DomainRequest.OrganizationChoices.STATE_OR_TERRITORY self.domain_request.is_election_board = True self.domain_request.save() - self.assertTrue(self.domain_request._is_state_or_territory_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.is_election_board = None self.domain_request.save() - self.assertFalse(self.domain_request._is_state_or_territory_complete()) + self.assertFalse(self.wizard.form_is_complete()) - @less_console_noise_decorator + # @less_console_noise_decorator def test_is_tribal_complete(self): self.domain_request.generic_org_type = DomainRequest.OrganizationChoices.TRIBAL self.domain_request.tribe_name = "Tribe Name" self.domain_request.is_election_board = False self.domain_request.save() - self.assertTrue(self.domain_request._is_tribal_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.is_election_board = None self.domain_request.save() - self.assertFalse(self.domain_request._is_tribal_complete()) + self.assertFalse(self.wizard.form_is_complete()) self.domain_request.tribe_name = None self.domain_request.save() - self.assertFalse(self.domain_request._is_tribal_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_county_complete(self): self.domain_request.generic_org_type = DomainRequest.OrganizationChoices.COUNTY self.domain_request.is_election_board = False self.domain_request.save() - self.assertTrue(self.domain_request._is_county_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.is_election_board = None self.domain_request.save() - self.assertFalse(self.domain_request._is_county_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_city_complete(self): self.domain_request.generic_org_type = DomainRequest.OrganizationChoices.CITY self.domain_request.is_election_board = False self.domain_request.save() - self.assertTrue(self.domain_request._is_city_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.is_election_board = None self.domain_request.save() - self.assertFalse(self.domain_request._is_city_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_special_district_complete(self): @@ -1776,55 +1787,55 @@ class TestDomainRequestIncomplete(TestCase): self.domain_request.about_your_organization = "Something something about your organization" self.domain_request.is_election_board = False self.domain_request.save() - self.assertTrue(self.domain_request._is_special_district_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.is_election_board = None self.domain_request.save() - self.assertFalse(self.domain_request._is_special_district_complete()) + self.assertFalse(self.wizard.form_is_complete()) self.domain_request.about_your_organization = None self.domain_request.save() - self.assertFalse(self.domain_request._is_special_district_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_organization_name_and_address_complete(self): - self.assertTrue(self.domain_request._is_organization_name_and_address_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.organization_name = None self.domain_request.address_line1 = None self.domain_request.save() - self.assertTrue(self.domain_request._is_organization_name_and_address_complete()) + self.assertTrue(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_senior_official_complete(self): - self.assertTrue(self.domain_request._is_senior_official_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.senior_official = None self.domain_request.save() - self.assertFalse(self.domain_request._is_senior_official_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_requested_domain_complete(self): - self.assertTrue(self.domain_request._is_requested_domain_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.requested_domain = None self.domain_request.save() - self.assertFalse(self.domain_request._is_requested_domain_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_purpose_complete(self): - self.assertTrue(self.domain_request._is_purpose_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.purpose = None self.domain_request.save() - self.assertFalse(self.domain_request._is_purpose_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_other_contacts_complete_missing_one_field(self): - self.assertTrue(self.domain_request._is_other_contacts_complete()) + self.assertTrue(self.wizard.form_is_complete()) contact = self.domain_request.other_contacts.first() contact.first_name = None contact.save() - self.assertFalse(self.domain_request._is_other_contacts_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_other_contacts_complete_all_none(self): self.domain_request.other_contacts.clear() - self.assertFalse(self.domain_request._is_other_contacts_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_other_contacts_False_and_has_rationale(self): @@ -1832,7 +1843,7 @@ class TestDomainRequestIncomplete(TestCase): self.domain_request.other_contacts.clear() self.domain_request.other_contacts.exists = False self.domain_request.no_other_contacts_rationale = "Some rationale" - self.assertTrue(self.domain_request._is_other_contacts_complete()) + self.assertTrue(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_other_contacts_False_and_NO_rationale(self): @@ -1840,7 +1851,7 @@ class TestDomainRequestIncomplete(TestCase): self.domain_request.other_contacts.clear() self.domain_request.other_contacts.exists = False self.domain_request.no_other_contacts_rationale = None - self.assertFalse(self.domain_request._is_other_contacts_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_is_additional_details_complete(self): @@ -2044,28 +2055,28 @@ class TestDomainRequestIncomplete(TestCase): self.domain_request.save() self.domain_request.refresh_from_db() self.assertEqual( - self.domain_request._is_additional_details_complete(), + self.wizard.form_is_complete(), case["expected"], msg=f"Failed for case: {case}", ) @less_console_noise_decorator def test_is_policy_acknowledgement_complete(self): - self.assertTrue(self.domain_request._is_policy_acknowledgement_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.is_policy_acknowledged = False - self.assertTrue(self.domain_request._is_policy_acknowledgement_complete()) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.is_policy_acknowledged = None - self.assertFalse(self.domain_request._is_policy_acknowledgement_complete()) + self.assertFalse(self.wizard.form_is_complete()) @less_console_noise_decorator def test_form_complete(self): request = self.factory.get("/") request.user = self.user - self.assertTrue(self.domain_request._form_complete(request)) + self.assertTrue(self.wizard.form_is_complete()) self.domain_request.generic_org_type = None self.domain_request.save() - self.assertFalse(self.domain_request._form_complete(request)) + self.assertFalse(self.wizard.form_is_complete()) class TestPortfolio(TestCase): diff --git a/src/registrar/utility/waffle.py b/src/registrar/utility/waffle.py index f97a18874..3071fbed9 100644 --- a/src/registrar/utility/waffle.py +++ b/src/registrar/utility/waffle.py @@ -22,7 +22,10 @@ def flag_is_active_anywhere(flag_name): If said flag is enabled for someone, somewhere - return true. Otherwise - return false. """ - flag = get_waffle_flag_model().get(flag_name) - if flag.everyone is None: - return flag.users.exists() - return flag.everyone + try: + flag = get_waffle_flag_model().get(flag_name) + if flag.everyone is None: + return flag.users.exists() + return flag.everyone + except get_waffle_flag_model().DoesNotExist: + return False diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index f200cc3ae..45d802764 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -115,9 +115,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): ), Step.DOTGOV_DOMAIN: lambda self: self.domain_request.requested_domain is not None, Step.PURPOSE: lambda self: self.domain_request.purpose is not None, - Step.OTHER_CONTACTS: lambda self: ( - self.domain_request.other_contacts.exists() or self.domain_request.no_other_contacts_rationale is not None - ), + Step.OTHER_CONTACTS: lambda self: self.from_model("unlock_other_contacts", False), Step.ADDITIONAL_DETAILS: lambda self: ( # Additional details is complete as long as "has anything else" and "has cisa rep" are not None ( @@ -425,16 +423,38 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): """Helper for get_context_data. Queries the DB for a domain request and returns a list of unlocked steps.""" return [key for key, is_unlocked_checker in self.unlocking_steps.items() if is_unlocked_checker(self)] + + def form_is_complete(self): + """ + Determines if all required steps in the domain request form are complete. + + This method: + 1. Gets a list of all steps that have been completed (unlocked_steps) + 2. Filters the full step list to only include steps that should be shown based on + the wizard conditions. For example, federal-specific steps are only required + if the organization type is federal. + 3. Compares the number of completed steps to required steps to determine if + the form is complete. + + Returns: + bool: True if all required steps are complete, False otherwise + """ + unlockable_steps = {step.value for step in self.db_check_for_unlocking_steps()} + required_steps = set(self.steps.all) + unlocked_steps = set() + for step in required_steps: + if step in unlockable_steps: + unlocked_steps.add(step) + return required_steps == unlocked_steps def get_context_data(self): """Define context for access on all wizard pages.""" - requested_domain_name = None if self.domain_request.requested_domain is not None: requested_domain_name = self.domain_request.requested_domain.name context = {} - org_steps_complete = len(self.db_check_for_unlocking_steps()) == len(self.steps) + org_steps_complete = self.form_is_complete() if org_steps_complete: context = { "form_titles": self.titles, @@ -770,7 +790,7 @@ class Review(DomainRequestWizard): forms = [] # type: ignore def get_context_data(self): - form_complete = len(self.db_check_for_unlocking_steps()) == len(self.steps) + form_complete = self.form_is_complete() if form_complete is False: logger.warning("User arrived at review page with an incomplete form.") context = super().get_context_data() From 4dba0a31c35faf1eab2fdc19c34ca1b372728666 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 28 Jan 2025 15:15:56 -0700 Subject: [PATCH 08/22] remove test code and lint --- src/registrar/models/domain.py | 1 - src/registrar/views/domain_request.py | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index c869dfa67..cb481db7a 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -244,7 +244,6 @@ class Domain(TimeStampedModel, DomainHelper): is called in the validate function on the request/domain page throws- RegistryError or InvalidDomainError""" - return True if not cls.string_could_be_domain(domain): logger.warning("Not a valid domain: %s" % str(domain)) # throw invalid domain error so that it can be caught in diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index 45d802764..e1f94391e 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -423,11 +423,11 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): """Helper for get_context_data. Queries the DB for a domain request and returns a list of unlocked steps.""" return [key for key, is_unlocked_checker in self.unlocking_steps.items() if is_unlocked_checker(self)] - + def form_is_complete(self): """ Determines if all required steps in the domain request form are complete. - + This method: 1. Gets a list of all steps that have been completed (unlocked_steps) 2. Filters the full step list to only include steps that should be shown based on From b5935a36d690bfda88f7ec277f6e2313b28ca309 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 29 Jan 2025 11:10:12 -0700 Subject: [PATCH 09/22] lint --- src/registrar/tests/test_models.py | 4 ++-- src/registrar/views/domain_request.py | 22 ++++++---------------- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 45f8a15f0..2a44f7765 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1719,7 +1719,7 @@ class TestDomainRequestIncomplete(TestCase): super().tearDownClass() cls.user.delete() - # @less_console_noise_decorator + @less_console_noise_decorator def test_is_federal_complete(self): self.assertTrue(self.wizard.form_is_complete()) self.domain_request.federal_type = None @@ -1747,7 +1747,7 @@ class TestDomainRequestIncomplete(TestCase): self.domain_request.save() self.assertFalse(self.wizard.form_is_complete()) - # @less_console_noise_decorator + @less_console_noise_decorator def test_is_tribal_complete(self): self.domain_request.generic_org_type = DomainRequest.OrganizationChoices.TRIBAL self.domain_request.tribe_name = "Tribe Name" diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index e1f94391e..3248c1368 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -425,26 +425,16 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): return [key for key, is_unlocked_checker in self.unlocking_steps.items() if is_unlocked_checker(self)] def form_is_complete(self): - """ - Determines if all required steps in the domain request form are complete. - - This method: - 1. Gets a list of all steps that have been completed (unlocked_steps) - 2. Filters the full step list to only include steps that should be shown based on - the wizard conditions. For example, federal-specific steps are only required - if the organization type is federal. - 3. Compares the number of completed steps to required steps to determine if - the form is complete. - + """Determines if all required steps in the domain request form are complete. Returns: bool: True if all required steps are complete, False otherwise """ - unlockable_steps = {step.value for step in self.db_check_for_unlocking_steps()} + # 1. Get all steps visibly present to the user (required steps) + # 2. Return every possible step that is "unlocked" (even hidden, conditional ones) + # 3. Narrows down the list to remove hidden conditional steps required_steps = set(self.steps.all) - unlocked_steps = set() - for step in required_steps: - if step in unlockable_steps: - unlocked_steps.add(step) + unlockable_steps = {step.value for step in self.db_check_for_unlocking_steps()} + unlocked_steps = {step for step in required_steps if step in unlockable_steps} return required_steps == unlocked_steps def get_context_data(self): From cc0c53006c6696a119d310bb76bbc762ba2b2e9f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 29 Jan 2025 15:43:53 -0700 Subject: [PATCH 10/22] Update test_forms.py --- src/registrar/tests/test_forms.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/registrar/tests/test_forms.py b/src/registrar/tests/test_forms.py index a2960deac..0589d8dec 100644 --- a/src/registrar/tests/test_forms.py +++ b/src/registrar/tests/test_forms.py @@ -23,6 +23,7 @@ from registrar.forms.portfolio import ( PortfolioMemberForm, PortfolioNewMemberForm, ) +from waffle.models import get_waffle_flag_model from registrar.models.portfolio import Portfolio from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user import User @@ -38,6 +39,10 @@ class TestFormValidation(MockEppLib): self.API_BASE_PATH = "/api/v1/available/?domain=" self.user = get_user_model().objects.create(username="username") self.factory = RequestFactory() + # We use both of these flags in the test. In the normal app these are generated normally. + # The alternative syntax is adding the decorator to each test. + get_waffle_flag_model().objects.get_or_create(name="organization_feature") + get_waffle_flag_model().objects.get_or_create(name="organization_requests") def test_org_contact_zip_invalid(self): form = OrganizationContactForm(data={"zipcode": "nah"}) From 59d1301fd72be651e49b0a39ad7d08b133783ee5 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 30 Jan 2025 08:40:24 -0700 Subject: [PATCH 11/22] Update test_views_request.py --- src/registrar/tests/test_views_request.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/registrar/tests/test_views_request.py b/src/registrar/tests/test_views_request.py index c559a73c7..9638ff669 100644 --- a/src/registrar/tests/test_views_request.py +++ b/src/registrar/tests/test_views_request.py @@ -3081,17 +3081,26 @@ class TestDomainRequestWizard(TestWithUser, WebTest): contact = Contact.objects.create( first_name="Henry", last_name="Mcfakerson", + title="test", + email="moar@igorville.gov", + phone="1234567890" ) # Create two non-orphaned contacts contact_2 = Contact.objects.create( first_name="Saturn", last_name="Mars", + title="test", + email="moar@igorville.gov", + phone="1234567890" ) # Attach a user object to a contact (should not be deleted) contact_user, _ = Contact.objects.get_or_create( first_name="Hank", last_name="McFakey", + title="test", + email="moar@igorville.gov", + phone="1234567890" ) site = DraftDomain.objects.create(name="igorville.gov") From 714a7232311be3659eb2c79819bf476c3386decc Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 30 Jan 2025 08:58:37 -0700 Subject: [PATCH 12/22] lint --- src/registrar/tests/test_views_request.py | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/src/registrar/tests/test_views_request.py b/src/registrar/tests/test_views_request.py index 9638ff669..d33d7e6ad 100644 --- a/src/registrar/tests/test_views_request.py +++ b/src/registrar/tests/test_views_request.py @@ -3079,28 +3079,16 @@ class TestDomainRequestWizard(TestWithUser, WebTest): # Create the site and contacts to delete (orphaned) contact = Contact.objects.create( - first_name="Henry", - last_name="Mcfakerson", - title="test", - email="moar@igorville.gov", - phone="1234567890" + first_name="Henry", last_name="Mcfakerson", title="test", email="moar@igorville.gov", phone="1234567890" ) # Create two non-orphaned contacts contact_2 = Contact.objects.create( - first_name="Saturn", - last_name="Mars", - title="test", - email="moar@igorville.gov", - phone="1234567890" + first_name="Saturn", last_name="Mars", title="test", email="moar@igorville.gov", phone="1234567890" ) # Attach a user object to a contact (should not be deleted) contact_user, _ = Contact.objects.get_or_create( - first_name="Hank", - last_name="McFakey", - title="test", - email="moar@igorville.gov", - phone="1234567890" + first_name="Hank", last_name="McFakey", title="test", email="moar@igorville.gov", phone="1234567890" ) site = DraftDomain.objects.create(name="igorville.gov") From 3a473ccb505caf2ab224d538786aedbed9987536 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Thu, 30 Jan 2025 15:32:40 -0800 Subject: [PATCH 13/22] remove the domain request export --- src/registrar/config/urls.py | 6 ------ .../templates/includes/domain_requests_table.html | 15 +-------------- src/registrar/views/report_views.py | 10 ---------- 3 files changed, 1 insertion(+), 30 deletions(-) diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index beb38e104..eb095c5ca 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -20,7 +20,6 @@ from registrar.views.report_views import ( AnalyticsView, ExportDomainRequestDataFull, ExportDataTypeUser, - ExportDataTypeRequests, ExportMembersPortfolio, ) @@ -260,11 +259,6 @@ urlpatterns = [ ExportDataTypeUser.as_view(), name="export_data_type_user", ), - path( - "reports/export_data_type_requests/", - ExportDataTypeRequests.as_view(), - name="export_data_type_requests", - ), path( "domain-request//edit/", views.DomainRequestWizard.as_view(), diff --git a/src/registrar/templates/includes/domain_requests_table.html b/src/registrar/templates/includes/domain_requests_table.html index b026a7a6b..ca64e1bff 100644 --- a/src/registrar/templates/includes/domain_requests_table.html +++ b/src/registrar/templates/includes/domain_requests_table.html @@ -51,20 +51,7 @@ - {% if portfolio %} -
-
- - -
-
- {% endif %} + {% if portfolio %} diff --git a/src/registrar/views/report_views.py b/src/registrar/views/report_views.py index ee2c079f3..e1a4a7d81 100644 --- a/src/registrar/views/report_views.py +++ b/src/registrar/views/report_views.py @@ -201,16 +201,6 @@ class ExportMembersPortfolio(PortfolioReportsPermission, View): return response -class ExportDataTypeRequests(DomainAndRequestsReportsPermission, View): - """Returns a domain requests report for a given user on the request""" - - def get(self, request, *args, **kwargs): - response = HttpResponse(content_type="text/csv") - response["Content-Disposition"] = 'attachment; filename="domain-requests.csv"' - csv_export.DomainRequestDataType.export_data_to_csv(response, request=request) - - return response - @method_decorator(staff_member_required, name="dispatch") class ExportDataFull(View): From cbf23d80e7960646277efd2b40a755b2950c7174 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Thu, 30 Jan 2025 15:49:47 -0800 Subject: [PATCH 14/22] added lint --- src/registrar/views/report_views.py | 1 - src/registrar/views/utility/mixins.py | 4 +--- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/registrar/views/report_views.py b/src/registrar/views/report_views.py index e1a4a7d81..1b9198c79 100644 --- a/src/registrar/views/report_views.py +++ b/src/registrar/views/report_views.py @@ -201,7 +201,6 @@ class ExportMembersPortfolio(PortfolioReportsPermission, View): return response - @method_decorator(staff_member_required, name="dispatch") class ExportDataFull(View): def get(self, request, *args, **kwargs): diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 2d121849e..a05334169 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -189,9 +189,7 @@ class PortfolioReportsPermission(PermissionsLoginMixin): return False portfolio = self.request.session.get("portfolio") - if not self.request.user.has_view_members_portfolio_permission( - portfolio - ): + if not self.request.user.has_view_members_portfolio_permission(portfolio): return False return self.request.user.is_org_user(self.request) From d14a23621e7f76891bde5bdff5f8246dc342c288 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 3 Feb 2025 15:36:45 -0700 Subject: [PATCH 15/22] fix bug --- src/registrar/models/domain_request.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index f58c6c6a2..fdeebaad6 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -1302,15 +1302,16 @@ class DomainRequest(TimeStampedModel): """Unlocks the organization_contact step.""" if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): # Check if the current federal agency is an outlawed one - Portfolio = apps.get_model("registrar.Portfolio") - return ( - FederalAgency.objects.exclude( - agency__in=Portfolio.objects.values_list("organization_name", flat=True), + if self.organization_type == self.OrganizationChoices.FEDERAL and self.federal_agency: + Portfolio = apps.get_model("registrar.Portfolio") + return ( + FederalAgency.objects.exclude( + agency__in=Portfolio.objects.values_list("organization_name", flat=True), + ) + .filter(agency=self.federal_agency) + .exists() ) - .filter(agency=self.federal_agency) - .exists() - ) - return ( + return bool( self.federal_agency is not None or self.organization_name is not None or self.address_line1 is not None From 8a82256206d7259d1a22441a32fa5b49218a7e1d Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 4 Feb 2025 05:13:58 -0500 Subject: [PATCH 16/22] update header and test --- .../templates/includes/header_extended.html | 2 ++ src/registrar/tests/test_views_portfolio.py | 22 ++++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/registrar/templates/includes/header_extended.html b/src/registrar/templates/includes/header_extended.html index 1e40a508d..83b71c3ab 100644 --- a/src/registrar/templates/includes/header_extended.html +++ b/src/registrar/templates/includes/header_extended.html @@ -92,11 +92,13 @@ {% endif %} {% if has_organization_members_flag %} + {% if has_view_members_portfolio_permission %}
  • Members
    • + {% endif %} {% endif %}
  • diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 33f334f7f..b50c9a36f 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -1097,8 +1097,10 @@ class TestPortfolio(WebTest): @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_requests", active=True) + @override_flag("organization_members", active=True) def test_main_nav_when_user_has_no_permissions(self): - """Test the nav contains a link to the no requests page""" + """Test the nav contains a link to the no requests page + Also test that members link not present""" UserPortfolioPermission.objects.get_or_create( user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] ) @@ -1118,20 +1120,23 @@ class TestPortfolio(WebTest): self.assertNotContains(portfolio_landing_page, "basic-nav-section-two") # link to requests self.assertNotContains(portfolio_landing_page, 'href="/requests/') - # link to create + # link to create request self.assertNotContains(portfolio_landing_page, 'href="/request/') + # link to members + self.assertNotContains(portfolio_landing_page, 'href="/members/') @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_requests", active=True) + @override_flag("organization_members", 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""" + Also test for the existence of the Create a new request btn on the requests page + Also test for the existence of the members link""" UserPortfolioPermission.objects.get_or_create( user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], - additional_permissions=[UserPortfolioPermissionChoices.EDIT_REQUESTS], ) self.client.force_login(self.user) # create and submit a domain request @@ -1151,6 +1156,8 @@ class TestPortfolio(WebTest): self.assertContains(portfolio_landing_page, 'href="/requests/') # link to create self.assertContains(portfolio_landing_page, 'href="/request/') + # link to members + self.assertContains(portfolio_landing_page, 'href="/members/') requests_page = self.client.get(reverse("domain-requests")) @@ -1160,15 +1167,18 @@ class TestPortfolio(WebTest): @less_console_noise_decorator @override_flag("organization_feature", active=True) @override_flag("organization_requests", active=True) + @override_flag("organization_members", 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""" + Also test for the existence of the Create a new request btn on the requests page + Also test for the existence of members link""" UserPortfolioPermission.objects.get_or_create( user=self.user, portfolio=self.portfolio, additional_permissions=[ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, + UserPortfolioPermissionChoices.VIEW_MEMBERS, ], ) self.client.force_login(self.user) @@ -1189,6 +1199,8 @@ class TestPortfolio(WebTest): self.assertContains(portfolio_landing_page, 'href="/requests/') # link to create self.assertNotContains(portfolio_landing_page, 'href="/request/') + # link to members + self.assertContains(portfolio_landing_page, 'href="/members/') requests_page = self.client.get(reverse("domain-requests")) From a8fa08acb2afeb0aa719badcd2163899714f1c60 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 4 Feb 2025 06:14:15 -0500 Subject: [PATCH 17/22] combined suborg and portfolio permissions --- src/registrar/context_processors.py | 9 ++------- src/registrar/models/user.py | 9 +-------- .../models/user_portfolio_permission.py | 4 ---- .../models/utility/portfolio_helper.py | 4 ---- src/registrar/templates/domain_detail.html | 8 ++++---- src/registrar/templates/domain_sidebar.html | 2 +- .../templates/domain_suborganization.html | 2 +- .../templates/includes/domains_table.html | 2 +- src/registrar/tests/test_models.py | 18 ++---------------- src/registrar/tests/test_reports.py | 2 +- src/registrar/tests/test_views_domain.py | 2 +- 11 files changed, 14 insertions(+), 48 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index b3d9c3727..b22729563 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -57,11 +57,10 @@ def portfolio_permissions(request): """Make portfolio permissions for the request user available in global context""" portfolio_context = { "has_base_portfolio_permission": False, + "has_edit_org_portfolio_permission": False, "has_any_domains_portfolio_permission": False, "has_any_requests_portfolio_permission": False, "has_edit_request_portfolio_permission": False, - "has_view_suborganization_portfolio_permission": False, - "has_edit_suborganization_portfolio_permission": False, "has_view_members_portfolio_permission": False, "has_edit_members_portfolio_permission": False, "portfolio": None, @@ -82,15 +81,11 @@ def portfolio_permissions(request): } ) - # Linting: line too long - view_suborg = request.user.has_view_suborganization_portfolio_permission(portfolio) - edit_suborg = request.user.has_edit_suborganization_portfolio_permission(portfolio) if portfolio: return { "has_base_portfolio_permission": request.user.has_base_portfolio_permission(portfolio), + "has_edit_org_portfolio_permission": request.user.has_edit_org_portfolio_permission(portfolio), "has_edit_request_portfolio_permission": request.user.has_edit_request_portfolio_permission(portfolio), - "has_view_suborganization_portfolio_permission": view_suborg, - "has_edit_suborganization_portfolio_permission": edit_suborg, "has_any_domains_portfolio_permission": request.user.has_any_domains_portfolio_permission(portfolio), "has_any_requests_portfolio_permission": request.user.has_any_requests_portfolio_permission(portfolio), "has_view_members_portfolio_permission": request.user.has_view_members_portfolio_permission(portfolio), diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 1d508f88f..7e0790c5b 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -268,13 +268,6 @@ class User(AbstractUser): def has_edit_request_portfolio_permission(self, portfolio): return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_REQUESTS) - # Field specific permission checks - def has_view_suborganization_portfolio_permission(self, portfolio): - return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION) - - def has_edit_suborganization_portfolio_permission(self, portfolio): - return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION) - def is_portfolio_admin(self, portfolio): return "Admin" in self.portfolio_role_summary(portfolio) @@ -293,7 +286,7 @@ class User(AbstractUser): # Define the conditions and their corresponding roles conditions_roles = [ - (self.has_edit_suborganization_portfolio_permission(portfolio), ["Admin"]), + (self.has_edit_org_portfolio_permission(portfolio), ["Admin"]), ( self.has_view_all_domains_portfolio_permission(portfolio) and self.has_any_requests_portfolio_permission(portfolio) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index 11d9c56e3..5378dc185 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -27,13 +27,10 @@ class UserPortfolioPermission(TimeStampedModel): UserPortfolioPermissionChoices.EDIT_MEMBERS, UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO, - UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION, - UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION, ], # NOTE: Check FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS before adding roles here. UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, - UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION, ], } @@ -43,7 +40,6 @@ class UserPortfolioPermission(TimeStampedModel): UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ UserPortfolioPermissionChoices.EDIT_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_MEMBERS, - UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION, ], } diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index 8c42b80c7..2c7b733d5 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -41,10 +41,6 @@ class UserPortfolioPermissionChoices(models.TextChoices): VIEW_PORTFOLIO = "view_portfolio", "View organization" EDIT_PORTFOLIO = "edit_portfolio", "Edit organization" - # Domain: field specific permissions - VIEW_SUBORGANIZATION = "view_suborganization", "View suborganization" - EDIT_SUBORGANIZATION = "edit_suborganization", "Edit suborganization" - @classmethod def get_user_portfolio_permission_label(cls, user_portfolio_permission): return cls(user_portfolio_permission).label if user_portfolio_permission else None diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index 03df2d59c..489d6fdf9 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -103,12 +103,12 @@ {% endif %} {% if portfolio %} - {% if has_any_domains_portfolio_permission and has_edit_suborganization_portfolio_permission %} + {% if has_any_domains_portfolio_permission and has_edit_org_portfolio_permission %} {% url 'domain-suborganization' pk=domain.id as url %} - {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_edit_suborganization_portfolio_permission %} - {% elif has_any_domains_portfolio_permission and has_view_suborganization_portfolio_permission %} + {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_edit_org_portfolio_permission %} + {% elif has_any_domains_portfolio_permission and has_base_portfolio_permission %} {% url 'domain-suborganization' pk=domain.id as url %} - {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_view_suborganization_portfolio_permission view_button=True %} + {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_base_portfolio_permission view_button=True %} {% endif %} {% else %} {% url 'domain-org-name-address' pk=domain.id as url %} diff --git a/src/registrar/templates/domain_sidebar.html b/src/registrar/templates/domain_sidebar.html index ca3802720..a87a611cd 100644 --- a/src/registrar/templates/domain_sidebar.html +++ b/src/registrar/templates/domain_sidebar.html @@ -61,7 +61,7 @@ {% if portfolio %} {% comment %} Only show this menu option if the user has the perms to do so {% endcomment %} - {% if has_any_domains_portfolio_permission and has_view_suborganization_portfolio_permission %} + {% if has_any_domains_portfolio_permission and has_base_portfolio_permission %} {% with url_name="domain-suborganization" %} {% include "includes/domain_sidenav_item.html" with item_text="Suborganization" %} {% endwith %} diff --git a/src/registrar/templates/domain_suborganization.html b/src/registrar/templates/domain_suborganization.html index e050690c8..89ce4e79d 100644 --- a/src/registrar/templates/domain_suborganization.html +++ b/src/registrar/templates/domain_suborganization.html @@ -39,7 +39,7 @@ please contact help@get.gov.

    - {% if has_any_domains_portfolio_permission and has_edit_suborganization_portfolio_permission %} + {% if has_any_domains_portfolio_permission and has_edit_org_portfolio_permission %}
    {% csrf_token %} {% input_with_errors form.sub_organization %} diff --git a/src/registrar/templates/includes/domains_table.html b/src/registrar/templates/includes/domains_table.html index de3d15eb0..9a49e46f9 100644 --- a/src/registrar/templates/includes/domains_table.html +++ b/src/registrar/templates/includes/domains_table.html @@ -208,7 +208,7 @@ Domain name Expires Status - {% if portfolio and has_view_suborganization_portfolio_permission %} + {% if portfolio and has_base_portfolio_permission %} Suborganization {% endif %} Date: Tue, 4 Feb 2025 06:19:19 -0500 Subject: [PATCH 18/22] added migration --- ...itation_additional_permissions_and_more.py | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 src/registrar/migrations/0140_alter_portfolioinvitation_additional_permissions_and_more.py diff --git a/src/registrar/migrations/0140_alter_portfolioinvitation_additional_permissions_and_more.py b/src/registrar/migrations/0140_alter_portfolioinvitation_additional_permissions_and_more.py new file mode 100644 index 000000000..8360d7a72 --- /dev/null +++ b/src/registrar/migrations/0140_alter_portfolioinvitation_additional_permissions_and_more.py @@ -0,0 +1,60 @@ +# Generated by Django 4.2.10 on 2025-02-04 11:18 + +import django.contrib.postgres.fields +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0139_alter_domainrequest_action_needed_reason"), + ] + + operations = [ + migrations.AlterField( + model_name="portfolioinvitation", + name="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_members", "View members"), + ("edit_members", "Create and edit members"), + ("view_all_requests", "View all 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, + ), + ), + migrations.AlterField( + model_name="userportfoliopermission", + name="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_members", "View members"), + ("edit_members", "Create and edit members"), + ("view_all_requests", "View all 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, + ), + ), + ] From 367b2b804316237b6d1dffa7d8263e92211a0e45 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 4 Feb 2025 09:58:17 -0700 Subject: [PATCH 19/22] Requested changes --- src/registrar/forms/domain_request_wizard.py | 8 ++++---- src/registrar/models/domain_request.py | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 0cf82558b..d7a02b124 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -322,7 +322,7 @@ class OrganizationContactForm(RegistrarForm): # if it has been filled in when required. # uncomment to see if modelChoiceField can be an arg later required=False, - # We populate this queryset in init. We want to exclude agencies with a portfolio. + # We populate this queryset in init. queryset=FederalAgency.objects.none(), widget=ComboboxWidget, ) @@ -369,12 +369,12 @@ class OrganizationContactForm(RegistrarForm): super().__init__(*args, **kwargs) # Set the queryset for federal agency. - # If the organization_requests flag is active, we hide data that exists in portfolios. + # If the organization_requests flag is active, We want to exclude agencies with a portfolio. federal_agency_queryset = FederalAgency.objects.exclude(agency__in=self.excluded_agencies) if flag_is_active_anywhere("organization_feature") and flag_is_active_anywhere("organization_requests"): - # Exclude both predefined agencies and those matching portfolio names in one query + # Exclude both predefined agencies and those matching portfolio records in one query federal_agency_queryset = federal_agency_queryset.exclude( - agency__in=Portfolio.objects.values_list("organization_name", flat=True) + id__in=Portfolio.objects.values_list("federal_agency__id", flat=True) ) self.fields["federal_agency"].queryset = federal_agency_queryset diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index fdeebaad6..3071d40d8 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -1306,9 +1306,9 @@ class DomainRequest(TimeStampedModel): Portfolio = apps.get_model("registrar.Portfolio") return ( FederalAgency.objects.exclude( - agency__in=Portfolio.objects.values_list("organization_name", flat=True), + id__in=Portfolio.objects.values_list("federal_agency__id", flat=True), ) - .filter(agency=self.federal_agency) + .filter(id=self.federal_agency.id) .exists() ) return bool( From 9bf152a8fc9abd5b1c6e07314daaa6bb08a06571 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 4 Feb 2025 13:18:24 -0700 Subject: [PATCH 20/22] unit test --- src/registrar/tests/test_views_request.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/registrar/tests/test_views_request.py b/src/registrar/tests/test_views_request.py index d33d7e6ad..f6eba2a56 100644 --- a/src/registrar/tests/test_views_request.py +++ b/src/registrar/tests/test_views_request.py @@ -3227,7 +3227,9 @@ class TestDomainRequestWizard(TestWithUser, WebTest): federal_agency = FederalAgency.objects.create(agency="Portfolio Agency") # Create a portfolio with matching organization name - Portfolio.objects.create(creator=self.user, organization_name=federal_agency.agency) + Portfolio.objects.create( + creator=self.user, organization_name=federal_agency.agency, federal_agency=federal_agency + ) # Create domain request with the portfolio agency domain_request = completed_domain_request(federal_agency=federal_agency, user=self.user) From 2bdd1cf71e12758b95e436cecbaeb556ec8997d9 Mon Sep 17 00:00:00 2001 From: Cameron Dixon Date: Wed, 5 Feb 2025 14:34:34 -0500 Subject: [PATCH 21/22] Delete story.yml --- .github/ISSUE_TEMPLATE/story.yml | 61 -------------------------------- 1 file changed, 61 deletions(-) delete mode 100644 .github/ISSUE_TEMPLATE/story.yml diff --git a/.github/ISSUE_TEMPLATE/story.yml b/.github/ISSUE_TEMPLATE/story.yml deleted file mode 100644 index e7d81ad3a..000000000 --- a/.github/ISSUE_TEMPLATE/story.yml +++ /dev/null @@ -1,61 +0,0 @@ -name: Story -description: Capture actionable sprint work -labels: ["story"] - -body: - - type: markdown - id: help - attributes: - value: | - > **Note** - > GitHub Issues use [GitHub Flavored Markdown](https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax) for formatting. - - type: textarea - id: story - attributes: - label: Story - description: | - Please add the "as a, I want, so that" details that describe the story. - If more than one "as a, I want, so that" describes the story, add multiple. - - Example: - As an analyst - I want the ability to approve a domain request - so that a request can be fulfilled and a new .gov domain can be provisioned - value: | - As a - I want - so that - validations: - required: true - - type: textarea - id: acceptance-criteria - attributes: - label: Acceptance Criteria - description: | - Please add the acceptance criteria that best describe the desired outcomes when this work is completed - - Example: - - Application sends an email when analysts approve domain requests - - Domain request status is "approved" - - Example ("given, when, then" format): - Given that I am an analyst who has finished reviewing a domain request - When I click to approve a domain request - Then the domain provisioning process should be initiated, and the applicant should receive an email update. - validations: - required: true - - type: textarea - id: additional-context - attributes: - label: Additional Context - description: "Please include additional references (screenshots, design links, documentation, etc.) that are relevant" - - type: textarea - id: issue-links - attributes: - label: Issue Links - description: | - What other issues does this story relate to and how? - - Example: - - ๐Ÿšง Blocked by: #123 - - ๐Ÿ”„ Relates to: #234 From 120650895a5bdf8b8507f80945e9178602a94657 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 5 Feb 2025 16:42:56 -0500 Subject: [PATCH 22/22] normalized more portfolio permission names --- src/registrar/context_processors.py | 8 ++++---- src/registrar/models/domain_request.py | 2 +- src/registrar/models/user.py | 16 ++++++++-------- src/registrar/templates/domain_detail.html | 8 ++++---- src/registrar/templates/domain_sidebar.html | 2 +- .../templates/domain_suborganization.html | 2 +- .../templates/includes/domains_table.html | 2 +- .../templates/portfolio_organization.html | 2 +- src/registrar/tests/test_models.py | 18 +++++++++--------- src/registrar/views/portfolios.py | 2 +- 10 files changed, 31 insertions(+), 31 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index b22729563..a078c81ac 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -56,8 +56,8 @@ def add_path_to_context(request): def portfolio_permissions(request): """Make portfolio permissions for the request user available in global context""" portfolio_context = { - "has_base_portfolio_permission": False, - "has_edit_org_portfolio_permission": False, + "has_view_portfolio_permission": False, + "has_edit_portfolio_permission": False, "has_any_domains_portfolio_permission": False, "has_any_requests_portfolio_permission": False, "has_edit_request_portfolio_permission": False, @@ -83,8 +83,8 @@ def portfolio_permissions(request): if portfolio: return { - "has_base_portfolio_permission": request.user.has_base_portfolio_permission(portfolio), - "has_edit_org_portfolio_permission": request.user.has_edit_org_portfolio_permission(portfolio), + "has_view_portfolio_permission": request.user.has_view_portfolio_permission(portfolio), + "has_edit_portfolio_permission": request.user.has_edit_portfolio_permission(portfolio), "has_edit_request_portfolio_permission": request.user.has_edit_request_portfolio_permission(portfolio), "has_any_domains_portfolio_permission": request.user.has_any_domains_portfolio_permission(portfolio), "has_any_requests_portfolio_permission": request.user.has_any_requests_portfolio_permission(portfolio), diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index c5a0926ad..598fe7a3d 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -947,7 +947,7 @@ class DomainRequest(TimeStampedModel): try: if not context: has_organization_feature_flag = flag_is_active_for_user(recipient, "organization_feature") - is_org_user = has_organization_feature_flag and recipient.has_base_portfolio_permission(self.portfolio) + is_org_user = has_organization_feature_flag and recipient.has_view_portfolio_permission(self.portfolio) context = { "domain_request": self, # This is the user that we refer to in the email diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 7e0790c5b..6f8ee499b 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -210,10 +210,10 @@ class User(AbstractUser): return portfolio_permission in user_portfolio_perms._get_portfolio_permissions() - def has_base_portfolio_permission(self, portfolio): + def has_view_portfolio_permission(self, portfolio): return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_PORTFOLIO) - def has_edit_org_portfolio_permission(self, portfolio): + def has_edit_portfolio_permission(self, portfolio): return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_PORTFOLIO) def has_any_domains_portfolio_permission(self, portfolio): @@ -286,7 +286,7 @@ class User(AbstractUser): # Define the conditions and their corresponding roles conditions_roles = [ - (self.has_edit_org_portfolio_permission(portfolio), ["Admin"]), + (self.has_edit_portfolio_permission(portfolio), ["Admin"]), ( self.has_view_all_domains_portfolio_permission(portfolio) and self.has_any_requests_portfolio_permission(portfolio) @@ -299,20 +299,20 @@ class User(AbstractUser): ["View-only admin"], ), ( - self.has_base_portfolio_permission(portfolio) + self.has_view_portfolio_permission(portfolio) and self.has_edit_request_portfolio_permission(portfolio) and self.has_any_domains_portfolio_permission(portfolio), ["Domain requestor", "Domain manager"], ), ( - self.has_base_portfolio_permission(portfolio) and self.has_edit_request_portfolio_permission(portfolio), + self.has_view_portfolio_permission(portfolio) and self.has_edit_request_portfolio_permission(portfolio), ["Domain requestor"], ), ( - self.has_base_portfolio_permission(portfolio) and self.has_any_domains_portfolio_permission(portfolio), + self.has_view_portfolio_permission(portfolio) and self.has_any_domains_portfolio_permission(portfolio), ["Domain manager"], ), - (self.has_base_portfolio_permission(portfolio), ["Member"]), + (self.has_view_portfolio_permission(portfolio), ["Member"]), ] # Evaluate conditions and add roles @@ -470,7 +470,7 @@ class User(AbstractUser): def is_org_user(self, request): has_organization_feature_flag = flag_is_active(request, "organization_feature") portfolio = request.session.get("portfolio") - return has_organization_feature_flag and self.has_base_portfolio_permission(portfolio) + return has_organization_feature_flag and self.has_view_portfolio_permission(portfolio) def get_user_domain_ids(self, request): """Returns either the domains ids associated with this user on UserDomainRole or Portfolio""" diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index 489d6fdf9..758c43366 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -103,12 +103,12 @@ {% endif %} {% if portfolio %} - {% if has_any_domains_portfolio_permission and has_edit_org_portfolio_permission %} + {% if has_any_domains_portfolio_permission and has_edit_portfolio_permission %} {% url 'domain-suborganization' pk=domain.id as url %} - {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_edit_org_portfolio_permission %} - {% elif has_any_domains_portfolio_permission and has_base_portfolio_permission %} + {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_edit_portfolio_permission %} + {% elif has_any_domains_portfolio_permission and has_view_portfolio_permission %} {% url 'domain-suborganization' pk=domain.id as url %} - {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_base_portfolio_permission view_button=True %} + {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_view_portfolio_permission view_button=True %} {% endif %} {% else %} {% url 'domain-org-name-address' pk=domain.id as url %} diff --git a/src/registrar/templates/domain_sidebar.html b/src/registrar/templates/domain_sidebar.html index a87a611cd..5946b6859 100644 --- a/src/registrar/templates/domain_sidebar.html +++ b/src/registrar/templates/domain_sidebar.html @@ -61,7 +61,7 @@ {% if portfolio %} {% comment %} Only show this menu option if the user has the perms to do so {% endcomment %} - {% if has_any_domains_portfolio_permission and has_base_portfolio_permission %} + {% if has_any_domains_portfolio_permission and has_view_portfolio_permission %} {% with url_name="domain-suborganization" %} {% include "includes/domain_sidenav_item.html" with item_text="Suborganization" %} {% endwith %} diff --git a/src/registrar/templates/domain_suborganization.html b/src/registrar/templates/domain_suborganization.html index 89ce4e79d..1c3b8e588 100644 --- a/src/registrar/templates/domain_suborganization.html +++ b/src/registrar/templates/domain_suborganization.html @@ -39,7 +39,7 @@ please contact help@get.gov.

    - {% if has_any_domains_portfolio_permission and has_edit_org_portfolio_permission %} + {% if has_any_domains_portfolio_permission and has_edit_portfolio_permission %} {% csrf_token %} {% input_with_errors form.sub_organization %} diff --git a/src/registrar/templates/includes/domains_table.html b/src/registrar/templates/includes/domains_table.html index 9a49e46f9..94cb4ea6d 100644 --- a/src/registrar/templates/includes/domains_table.html +++ b/src/registrar/templates/includes/domains_table.html @@ -208,7 +208,7 @@ Domain name Expires Status - {% if portfolio and has_base_portfolio_permission %} + {% if portfolio and has_view_portfolio_permission %} Suborganization {% endif %} The name of your organization will be publicly listed as the domain registrant.

    - {% if has_edit_org_portfolio_permission %} + {% if has_edit_portfolio_permission %}

    Your organization name canโ€™t be updated here. To suggest an update, email help@get.gov. diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 4cd353d36..0d708671e 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1190,7 +1190,7 @@ class TestUser(TestCase): User.objects.all().delete() UserDomainRole.objects.all().delete() - @patch.object(User, "has_edit_org_portfolio_permission", return_value=True) + @patch.object(User, "has_edit_portfolio_permission", return_value=True) def test_portfolio_role_summary_admin(self, mock_edit_org): # Test if the user is recognized as an Admin self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Admin"]) @@ -1216,7 +1216,7 @@ class TestUser(TestCase): @patch.multiple( User, - has_base_portfolio_permission=lambda self, portfolio: True, + has_view_portfolio_permission=lambda self, portfolio: True, has_edit_request_portfolio_permission=lambda self, portfolio: True, has_any_domains_portfolio_permission=lambda self, portfolio: True, ) @@ -1226,7 +1226,7 @@ class TestUser(TestCase): @patch.multiple( User, - has_base_portfolio_permission=lambda self, portfolio: True, + has_view_portfolio_permission=lambda self, portfolio: True, has_edit_request_portfolio_permission=lambda self, portfolio: True, ) def test_portfolio_role_summary_member_domain_requestor(self): @@ -1235,14 +1235,14 @@ class TestUser(TestCase): @patch.multiple( User, - has_base_portfolio_permission=lambda self, portfolio: True, + has_view_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 self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Domain manager"]) - @patch.multiple(User, has_base_portfolio_permission=lambda self, portfolio: True) + @patch.multiple(User, has_view_portfolio_permission=lambda self, portfolio: True) def test_portfolio_role_summary_member(self): # Test if the user is recognized as a Member self.assertEqual(self.user.portfolio_role_summary(self.portfolio), ["Member"]) @@ -1252,17 +1252,17 @@ class TestUser(TestCase): 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): + def test_has_view_portfolio_permission(self, mock_has_permission): mock_has_permission.return_value = True - self.assertTrue(self.user.has_base_portfolio_permission(self.portfolio)) + self.assertTrue(self.user.has_view_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): + def test_has_edit_portfolio_permission(self, mock_has_permission): mock_has_permission.return_value = True - self.assertTrue(self.user.has_edit_org_portfolio_permission(self.portfolio)) + self.assertTrue(self.user.has_edit_portfolio_permission(self.portfolio)) mock_has_permission.assert_called_once_with(self.portfolio, UserPortfolioPermissionChoices.EDIT_PORTFOLIO) @patch("registrar.models.User._has_portfolio_permission") diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 212ce089d..beb04d2c7 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -641,7 +641,7 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): """Add additional context data to the template.""" context = super().get_context_data(**kwargs) portfolio = self.request.session.get("portfolio") - context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission(portfolio) + context["has_edit_portfolio_permission"] = self.request.user.has_edit_portfolio_permission(portfolio) return context def get_object(self, queryset=None):