From 89d52d984c86efd1374cf0e00065e485ab3115fe Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 27 Jan 2025 17:31:45 -0600 Subject: [PATCH 01/24] initial updated script --- .../commands/create_federal_portfolio.py | 260 +++++++++++++++++- 1 file changed, 249 insertions(+), 11 deletions(-) diff --git a/src/registrar/management/commands/create_federal_portfolio.py b/src/registrar/management/commands/create_federal_portfolio.py index 389662ead..c3e78874e 100644 --- a/src/registrar/management/commands/create_federal_portfolio.py +++ b/src/registrar/management/commands/create_federal_portfolio.py @@ -5,7 +5,12 @@ import logging from django.core.management import BaseCommand, CommandError from registrar.management.commands.utility.terminal_helper import TerminalColors, TerminalHelper from registrar.models import DomainInformation, DomainRequest, FederalAgency, Suborganization, Portfolio, User +from registrar.models.domain import Domain +from registrar.models.portfolio_invitation import PortfolioInvitation +from registrar.models.user_domain_role import UserDomainRole +from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.generic_helper import normalize_string +from django.db.models import F, Q logger = logging.getLogger(__name__) @@ -20,6 +25,8 @@ class Command(BaseCommand): self.updated_portfolios = set() self.skipped_portfolios = set() self.failed_portfolios = set() + self.added_managers = set() + self.added_invitations = set() def add_arguments(self, parser): """Add command line arguments to create federal portfolios. @@ -37,6 +44,9 @@ class Command(BaseCommand): Optional (mutually exclusive with parse options): --both: Shorthand for using both --parse_requests and --parse_domains Cannot be used with --parse_requests or --parse_domains + + Optional: + --add_managers: Add all domain managers of the portfolio's domains to the organization. """ group = parser.add_mutually_exclusive_group(required=True) group.add_argument( @@ -63,6 +73,11 @@ class Command(BaseCommand): action=argparse.BooleanOptionalAction, help="Adds portfolio to both requests and domains", ) + parser.add_argument( + "--add_managers", + action=argparse.BooleanOptionalAction, + help="Add all domain managers of the portfolio's domains to the organization.", + ) def handle(self, **options): agency_name = options.get("agency_name") @@ -70,7 +85,7 @@ class Command(BaseCommand): parse_requests = options.get("parse_requests") parse_domains = options.get("parse_domains") both = options.get("both") - + add_managers = options.get("add_managers") if not both: if not parse_requests and not parse_domains: raise CommandError("You must specify at least one of --parse_requests or --parse_domains.") @@ -98,18 +113,26 @@ class Command(BaseCommand): # C901 'Command.handle' is too complex (12) portfolio = self.handle_populate_portfolio(federal_agency, parse_domains, parse_requests, both) portfolios.append(portfolio) + + if add_managers: + self.add_managers_to_portfolio(portfolio) except Exception as exec: self.failed_portfolios.add(federal_agency) logger.error(exec) message = f"Failed to create portfolio '{federal_agency.agency}'" TerminalHelper.colorful_logger(logger.info, TerminalColors.FAIL, message) + # POST PROCESS STEP: Add additional suborg info where applicable. + updated_suborg_count = self.post_process_all_suborganization_fields(agencies) + message = f"Added city and state_territory information to {updated_suborg_count} suborgs." + TerminalHelper.colorful_logger(logger.info, TerminalColors.MAGENTA, message) + TerminalHelper.log_script_run_summary( self.updated_portfolios, self.failed_portfolios, self.skipped_portfolios, debug=False, - skipped_header="----- SOME PORTFOLIOS WERE SKIPPED -----", + skipped_header="----- SOME PORTFOLIOS WERENT CREATED -----", display_as_str=True, ) @@ -126,6 +149,87 @@ class Command(BaseCommand): ) self.post_process_started_domain_requests(agencies, portfolios) + def add_managers_to_portfolio(self, portfolio: Portfolio): + """ + Add all domain managers of the portfolio's domains to the organization. + This includes adding them to the correct group and creating portfolio invitations. + """ + logger.info("Adding managers for portfolio '{portfolio}'") + + # Fetch all domains associated with the portfolio + domains = portfolio.get_domains() + + domain_managers = set() + + # Fetch all users with manager roles for the domains + managers = UserDomainRole.objects.filter( + domain__in=domains, + role=UserDomainRole.Roles.MANAGER + ).values_list('user', flat=True) + + domain_managers.update(managers) + + # Fetch PortfolioInvitations with manager roles for the portfolio + manager_invitations = PortfolioInvitation.objects.filter( + portfolio=portfolio, + roles__contains=[UserPortfolioPermission.RoleChoices.MANAGER] + ).values_list('email', flat=True) + + # Fetch user IDs for existing users with emails in manager_invitations + invited_users = User.objects.filter(email__in=manager_invitations) + invited_user_ids = invited_users.values_list('id', flat=True) + domain_managers.update(invited_user_ids) + + for user_id in domain_managers: + try: + user = User.objects.get(id=user_id) + _, created = UserPortfolioPermission.objects.get_or_create( + portfolio=portfolio, + user=user, + defaults={"role": UserPortfolioPermission.RoleChoices.MANAGER}, + ) + if created: + logger.info("Added manager '{user}' to portfolio '{portfolio}'") + else: + logger.info("Manager '{user}' already exists in portfolio '{portfolio}'") + except User.DoesNotExist: + self.create_portfolio_invitation(portfolio, user) + + def create_portfolio_invitation(self, portfolio: Portfolio, user: User): + """ + Create a portfolio invitation for the given user. + If the user already has a portfolio invitation, retreive their invitation and create a portfolio permission. + """ + try: + user = User.objects.get(email=user.email) + _, created = PortfolioInvitation.objects.get_or_create( + portfolio=portfolio, + user=user, + defaults={"status": PortfolioInvitation.PortfolioInvitationStatus.PENDING}, + ) + if created: + logger.info("Created portfolio invitation for '{user}' to portfolio '{portfolio}'") + else: + logger.info("Retrieved existing portfolio invitation for '{user}' to portfolio '{portfolio}'") + + # Assign portfolio permissions + _, created = UserPortfolioPermission.objects.get_or_create( + portfolio=portfolio, + user=user, + defaults={"role": UserPortfolioPermission.RoleChoices.MANAGER}, + ) + if created: + logger.info("Created portfolio permission for '{user}' to portfolio '{portfolio}'") + else: + logger.info("Retrieved existing portfolio permission for '{user}' to portfolio '{portfolio}'") + except User.DoesNotExist: + PortfolioInvitation.objects.create( + portfolio=portfolio, + user=user, + status=PortfolioInvitation.PortfolioInvitationStatus.PENDING, + ) + logger.info("Created portfolio invitation for '{user}' to portfolio '{portfolio}'") + def post_process_started_domain_requests(self, agencies, portfolios): """ Removes duplicate organization data by clearing federal_agency when it matches the portfolio name. @@ -169,14 +273,11 @@ class Command(BaseCommand): def handle_populate_portfolio(self, federal_agency, parse_domains, parse_requests, both): """Attempts to create a portfolio. If successful, this function will - also create new suborganizations. - Returns the portfolio for the given federal_agency. - """ - portfolio, created = self.create_portfolio(federal_agency) - if created: - self.create_suborganizations(portfolio, federal_agency) - if parse_domains or both: - self.handle_portfolio_domains(portfolio, federal_agency) + also create new suborganizations""" + portfolio, _ = self.create_portfolio(federal_agency) + self.create_suborganizations(portfolio, federal_agency) + if parse_domains or both: + self.handle_portfolio_domains(portfolio, federal_agency) if parse_requests or both: self.handle_portfolio_requests(portfolio, federal_agency) @@ -233,7 +334,6 @@ class Command(BaseCommand): federal_agency=federal_agency, organization_name__isnull=False ) org_names = set(valid_agencies.values_list("organization_name", flat=True)) - if not org_names: message = ( "Could not add any suborganizations." @@ -352,3 +452,141 @@ class Command(BaseCommand): DomainInformation.objects.bulk_update(domain_infos, ["portfolio", "sub_organization"]) message = f"Added portfolio '{portfolio}' to {len(domain_infos)} domains." TerminalHelper.colorful_logger(logger.info, TerminalColors.OKGREEN, message) + + def post_process_all_suborganization_fields(self, agencies): + """Batch updates suborganization locations from domain and request data. + + Args: + agencies: List of FederalAgency objects to process + + Returns: + int: Number of suborganizations updated + + Priority for location data: + 1. Domain information + 2. Domain request suborganization fields + 3. Domain request standard fields + """ + # Common filter between domaininformation / domain request. + # Filter by only the agencies we've updated thus far. + # Then, only process records without null portfolio, org name, or suborg name. + base_filter = Q( + federal_agency__in=agencies, + portfolio__isnull=False, + organization_name__isnull=False, + sub_organization__isnull=False, + ) & ~Q(organization_name__iexact=F("portfolio__organization_name")) + + # First: Remove null city / state_territory values on domain info / domain requests. + # We want to add city data if there is data to add to begin with! + domains = DomainInformation.objects.filter( + base_filter, + Q(city__isnull=False, state_territory__isnull=False), + ) + requests = DomainRequest.objects.filter( + base_filter, + ( + Q(city__isnull=False, state_territory__isnull=False) + | Q(suborganization_city__isnull=False, suborganization_state_territory__isnull=False) + ), + ) + + # Second: Group domains and requests by normalized organization name. + # This means that later down the line we have to account for "duplicate" org names. + domains_dict = {} + requests_dict = {} + for domain in domains: + normalized_name = normalize_string(domain.organization_name) + domains_dict.setdefault(normalized_name, []).append(domain) + + for request in requests: + normalized_name = normalize_string(request.organization_name) + requests_dict.setdefault(normalized_name, []).append(request) + + # Third: Get suborganizations to update + suborgs_to_edit = Suborganization.objects.filter( + Q(id__in=domains.values_list("sub_organization", flat=True)) + | Q(id__in=requests.values_list("sub_organization", flat=True)) + ) + + # Fourth: Process each suborg to add city / state territory info + for suborg in suborgs_to_edit: + self.post_process_suborganization_fields(suborg, domains_dict, requests_dict) + + # Fifth: Perform a bulk update + return Suborganization.objects.bulk_update(suborgs_to_edit, ["city", "state_territory"]) + + def post_process_suborganization_fields(self, suborg, domains_dict, requests_dict): + """Updates a single suborganization's location data if valid. + + Args: + suborg: Suborganization to update + domains_dict: Dict of domain info records grouped by org name + requests_dict: Dict of domain requests grouped by org name + + Priority matches parent method. Updates are skipped if location data conflicts + between multiple records of the same type. + """ + normalized_suborg_name = normalize_string(suborg.name) + domains = domains_dict.get(normalized_suborg_name, []) + requests = requests_dict.get(normalized_suborg_name, []) + + # Try to get matching domain info + domain = None + if domains: + reference = domains[0] + use_location_for_domain = all( + d.city == reference.city and d.state_territory == reference.state_territory for d in domains + ) + if use_location_for_domain: + domain = reference + + # Try to get matching request info + # Uses consensus: if all city / state_territory info matches, then we can assume the data is "good". + # If not, take the safe route and just skip updating this particular record. + request = None + use_suborg_location_for_request = True + use_location_for_request = True + if requests: + reference = requests[0] + use_suborg_location_for_request = all( + r.suborganization_city + and r.suborganization_state_territory + and r.suborganization_city == reference.suborganization_city + and r.suborganization_state_territory == reference.suborganization_state_territory + for r in requests + ) + use_location_for_request = all( + r.city + and r.state_territory + and r.city == reference.city + and r.state_territory == reference.state_territory + for r in requests + ) + if use_suborg_location_for_request or use_location_for_request: + request = reference + + if not domain and not request: + message = f"Skipping adding city / state_territory information to suborg: {suborg}. Bad data." + TerminalHelper.colorful_logger(logger.warning, TerminalColors.YELLOW, message) + return + + # PRIORITY: + # 1. Domain info + # 2. Domain request requested suborg fields + # 3. Domain request normal fields + if domain: + suborg.city = normalize_string(domain.city, lowercase=False) + suborg.state_territory = domain.state_territory + elif request and use_suborg_location_for_request: + suborg.city = normalize_string(request.suborganization_city, lowercase=False) + suborg.state_territory = request.suborganization_state_territory + elif request and use_location_for_request: + suborg.city = normalize_string(request.city, lowercase=False) + suborg.state_territory = request.state_territory + + message = ( + f"Added city/state_territory to suborg: {suborg}. " + f"city - {suborg.city}, state - {suborg.state_territory}" + ) + TerminalHelper.colorful_logger(logger.info, TerminalColors.MAGENTA, message) From 4bac837dbcc36292a65c7c536ce74b4587e738b8 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 28 Jan 2025 15:40:32 -0600 Subject: [PATCH 02/24] fix tests for portfolio script --- .../commands/create_federal_portfolio.py | 67 +++++++------- .../tests/test_management_scripts.py | 90 ++++++++++++++++++- 2 files changed, 126 insertions(+), 31 deletions(-) diff --git a/src/registrar/management/commands/create_federal_portfolio.py b/src/registrar/management/commands/create_federal_portfolio.py index c3e78874e..095d6d9f5 100644 --- a/src/registrar/management/commands/create_federal_portfolio.py +++ b/src/registrar/management/commands/create_federal_portfolio.py @@ -6,12 +6,15 @@ from django.core.management import BaseCommand, CommandError from registrar.management.commands.utility.terminal_helper import TerminalColors, TerminalHelper from registrar.models import DomainInformation, DomainRequest, FederalAgency, Suborganization, Portfolio, User from registrar.models.domain import Domain +from registrar.models.domain_invitation import DomainInvitation from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.user_domain_role import UserDomainRole from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.generic_helper import normalize_string from django.db.models import F, Q +from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices + logger = logging.getLogger(__name__) @@ -114,7 +117,9 @@ class Command(BaseCommand): portfolio = self.handle_populate_portfolio(federal_agency, parse_domains, parse_requests, both) portfolios.append(portfolio) + logger.debug(f"add_managers: {add_managers}") if add_managers: + logger.debug("Adding managers to portfolio") self.add_managers_to_portfolio(portfolio) except Exception as exec: self.failed_portfolios.add(federal_agency) @@ -154,11 +159,11 @@ class Command(BaseCommand): Add all domain managers of the portfolio's domains to the organization. This includes adding them to the correct group and creating portfolio invitations. """ - logger.info("Adding managers for portfolio '{portfolio}'") + logger.info(f"Adding managers for portfolio {portfolio}") # Fetch all domains associated with the portfolio - domains = portfolio.get_domains() - + domains = Domain.objects.filter(domain_info__portfolio=portfolio) + logger.debug(f"domains: {domains}") domain_managers = set() # Fetch all users with manager roles for the domains @@ -166,51 +171,53 @@ class Command(BaseCommand): domain__in=domains, role=UserDomainRole.Roles.MANAGER ).values_list('user', flat=True) - domain_managers.update(managers) - # Fetch PortfolioInvitations with manager roles for the portfolio - manager_invitations = PortfolioInvitation.objects.filter( - portfolio=portfolio, - roles__contains=[UserPortfolioPermission.RoleChoices.MANAGER] - ).values_list('email', flat=True) + invited_managers = set() - # Fetch user IDs for existing users with emails in manager_invitations - invited_users = User.objects.filter(email__in=manager_invitations) - invited_user_ids = invited_users.values_list('id', flat=True) - domain_managers.update(invited_user_ids) + # Get the emails of invited managers + for domain in domains: + domain_invitations = DomainInvitation.objects.filter(domain=domain, status=DomainInvitation.DomainInvitationStatus.INVITED).values_list('email', flat=True) + invited_managers.update(domain_invitations) - for user_id in domain_managers: + logger.debug(f"invited_managers: {invited_managers}") + for manager in domain_managers: try: - user = User.objects.get(id=user_id) + # manager is a user id + user = User.objects.get(id=manager) _, created = UserPortfolioPermission.objects.get_or_create( portfolio=portfolio, user=user, - defaults={"role": UserPortfolioPermission.RoleChoices.MANAGER}, + defaults={"roles": [UserPortfolioRoleChoices.ORGANIZATION_MEMBER]}, ) if created: - logger.info("Added manager '{user}' to portfolio '{portfolio}'") + logger.info(f"Added manager '{user}' to portfolio '{portfolio}'") else: - logger.info("Manager '{user}' already exists in portfolio '{portfolio}'") + logger.info(f"Manager '{user}' already exists in portfolio '{portfolio}'") except User.DoesNotExist: - self.create_portfolio_invitation(portfolio, user) + logger.debug(f"User '{user}' does not exist") + + for manager in invited_managers: + self.create_portfolio_invitation(portfolio, manager) - def create_portfolio_invitation(self, portfolio: Portfolio, user: User): + def create_portfolio_invitation(self, portfolio: Portfolio, email: str): """ Create a portfolio invitation for the given user. If the user already has a portfolio invitation, retreive their invitation and create a portfolio permission. """ try: - user = User.objects.get(email=user.email) + logger.debug(f"Creating portfolio invitation for user '{email}'") + user = User.objects.get(email=email) + logger.debug(f"user: {user}") _, created = PortfolioInvitation.objects.get_or_create( portfolio=portfolio, user=user, - defaults={"status": PortfolioInvitation.PortfolioInvitationStatus.PENDING}, + defaults={"status": PortfolioInvitation.PortfolioInvitationStatus.INVITED}, ) if created: - logger.info("Created portfolio invitation for '{user}' to portfolio '{portfolio}'") + logger.info(f"Created portfolio invitation for '{user}' to portfolio '{portfolio}'") else: - logger.info("Retrieved existing portfolio invitation for '{user}' to portfolio '{portfolio}'") + logger.info(f"Retrieved existing portfolio invitation for '{user}' to portfolio '{portfolio}'") # Assign portfolio permissions _, created = UserPortfolioPermission.objects.get_or_create( @@ -219,16 +226,16 @@ class Command(BaseCommand): defaults={"role": UserPortfolioPermission.RoleChoices.MANAGER}, ) if created: - logger.info("Created portfolio permission for '{user}' to portfolio '{portfolio}'") + logger.info(f"Created portfolio permission for '{user}' to portfolio '{portfolio}'") else: - logger.info("Retrieved existing portfolio permission for '{user}' to portfolio '{portfolio}'") + logger.info(f"Retrieved existing portfolio permission for '{user}' to portfolio '{portfolio}'") except User.DoesNotExist: - PortfolioInvitation.objects.create( + PortfolioInvitation.objects.get_or_create( portfolio=portfolio, - user=user, - status=PortfolioInvitation.PortfolioInvitationStatus.PENDING, + email=email, + defaults={"status": PortfolioInvitation.PortfolioInvitationStatus.INVITED}, ) - logger.info("Created portfolio invitation for '{user}' to portfolio '{portfolio}'") + logger.info(f"Created portfolio invitation for '{email}' to portfolio '{portfolio}'") def post_process_started_domain_requests(self, agencies, portfolios): """ diff --git a/src/registrar/tests/test_management_scripts.py b/src/registrar/tests/test_management_scripts.py index 8ecb7cbea..b2b217044 100644 --- a/src/registrar/tests/test_management_scripts.py +++ b/src/registrar/tests/test_management_scripts.py @@ -3,7 +3,10 @@ import boto3_mocking # type: ignore from datetime import date, datetime, time from django.core.management import call_command from django.test import TestCase, override_settings +from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.senior_official import SeniorOfficial +from registrar.models.user_portfolio_permission import UserPortfolioPermission +from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.utility.constants import BranchChoices from django.utils import timezone from django.utils.module_loading import import_string @@ -1456,37 +1459,46 @@ class TestCreateFederalPortfolio(TestCase): self.executive_so_2 = SeniorOfficial.objects.create( first_name="first", last_name="last", email="mango@igorville.gov", federal_agency=self.executive_agency_2 ) + + self.portfolio = Portfolio.objects.create(organization_name=self.federal_agency.agency, federal_agency=self.federal_agency, creator=self.user) + with boto3_mocking.clients.handler_for("sesv2", self.mock_client): self.domain_request = completed_domain_request( status=DomainRequest.DomainRequestStatus.IN_REVIEW, generic_org_type=DomainRequest.OrganizationChoices.CITY, federal_agency=self.federal_agency, + portfolio=self.portfolio, user=self.user, ) self.domain_request.approve() self.domain_info = DomainInformation.objects.filter(domain_request=self.domain_request).get() + self.domain = Domain.objects.get(name="city.gov") self.domain_request_2 = completed_domain_request( name="icecreamforigorville.gov", status=DomainRequest.DomainRequestStatus.IN_REVIEW, generic_org_type=DomainRequest.OrganizationChoices.CITY, federal_agency=self.federal_agency, + portfolio=self.portfolio, user=self.user, organization_name="Test Federal Agency", ) self.domain_request_2.approve() self.domain_info_2 = DomainInformation.objects.filter(domain_request=self.domain_request_2).get() + self.domain_2 = Domain.objects.get(name="icecreamforigorville.gov") self.domain_request_3 = completed_domain_request( name="exec_1.gov", status=DomainRequest.DomainRequestStatus.IN_REVIEW, generic_org_type=DomainRequest.OrganizationChoices.FEDERAL, federal_agency=self.executive_agency_1, + portfolio=self.portfolio, user=self.user, organization_name="Executive Agency 1", ) self.domain_request_3.approve() self.domain_info_3 = self.domain_request_3.DomainRequest_info + self.domain = Domain.objects.get(name="exec_1.gov") self.domain_request_4 = completed_domain_request( name="exec_2.gov", @@ -1494,10 +1506,12 @@ class TestCreateFederalPortfolio(TestCase): generic_org_type=DomainRequest.OrganizationChoices.FEDERAL, federal_agency=self.executive_agency_2, user=self.user, + portfolio=self.portfolio, organization_name="Executive Agency 2", ) self.domain_request_4.approve() self.domain_info_4 = self.domain_request_4.DomainRequest_info + self.domain_4 = Domain.objects.get(name="exec_2.gov") def tearDown(self): DomainInformation.objects.all().delete() @@ -1508,7 +1522,6 @@ class TestCreateFederalPortfolio(TestCase): FederalAgency.objects.all().delete() User.objects.all().delete() - @less_console_noise_decorator def run_create_federal_portfolio(self, **kwargs): with patch( "registrar.management.commands.utility.terminal_helper.TerminalHelper.query_yes_no_exit", @@ -1844,3 +1857,78 @@ class TestCreateFederalPortfolio(TestCase): self.assertEqual(existing_portfolio.organization_name, self.federal_agency.agency) self.assertEqual(existing_portfolio.notes, "Old notes") self.assertEqual(existing_portfolio.creator, self.user) + + @less_console_noise_decorator + def test_add_managers_from_domains(self): + """Test that all domain managers are added as portfolio managers.""" + + # Create users and assign them as domain managers + manager1 = User.objects.create(username="manager1", email="manager1@example.com") + manager2 = User.objects.create(username="manager2", email="manager2@example.com") + UserDomainRole.objects.create(user=manager1, domain=self.domain, role=UserDomainRole.Roles.MANAGER) + UserDomainRole.objects.create(user=manager2, domain=self.domain, role=UserDomainRole.Roles.MANAGER) + + # Run the management command + self.run_create_federal_portfolio(agency_name=self.portfolio.organization_name, parse_requests=True, add_managers=True) + + # Check that the users have been added as portfolio managers + permissions = UserPortfolioPermission.objects.filter(portfolio=self.portfolio, user__in=[manager1, manager2]) + print(UserPortfolioPermission.objects.all()) + + # Check that the users have been added as portfolio managers + self.assertEqual(permissions.count(), 2) + for perm in permissions: + self.assertIn(UserPortfolioRoleChoices.ORGANIZATION_MEMBER, perm.roles) + + @less_console_noise_decorator + def test_add_invited_managers(self): + """Test that invited domain managers receive portfolio invitations.""" + + # create a domain invitation for the manager + _ = DomainInvitation.objects.create( + domain=self.domain, + email="manager1@example.com", + status=DomainInvitation.DomainInvitationStatus.INVITED + ) + + # Ensure no existing PortfolioInvitation for the invited email + self.assertFalse(PortfolioInvitation.objects.filter(email="manager1@example.com", portfolio=self.portfolio).exists()) + + # Run the management command + self.run_create_federal_portfolio(agency_name=self.federal_agency.agency, parse_requests=True, add_managers=True) + + # Check that a PortfolioInvitation has been created for the invited email + invitation = PortfolioInvitation.objects.get(email="manager1@example.com", portfolio=self.portfolio) + + # Verify the status of the invitation remains INVITED + self.assertEqual( + invitation.status, + PortfolioInvitation.PortfolioInvitationStatus.INVITED, + "PortfolioInvitation status should remain INVITED for non-existent users." + ) + + # Verify that no duplicate invitations are created + self.run_create_federal_portfolio(agency_name=self.federal_agency.agency, parse_requests=True, add_managers=True) + duplicated_invitations = PortfolioInvitation.objects.filter(email="manager1@example.com", portfolio=self.portfolio) + self.assertEqual( + duplicated_invitations.count(), + 1, + "Duplicate PortfolioInvitation should not be created for the same email and portfolio." + ) + + @less_console_noise_decorator + def test_no_duplicate_managers_added(self): + """Test that duplicate managers are not added multiple times.""" + # Create a manager + manager = User.objects.create(username="manager", email="manager@example.com") + UserDomainRole.objects.create(user=manager, domain=self.domain, role=UserDomainRole.Roles.MANAGER) + + # Manually add the manager to the portfolio + UserPortfolioPermission.objects.create(portfolio=self.portfolio, user=manager, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER]) + + # Run the management command + self.run_create_federal_portfolio(agency_name=self.federal_agency.agency, parse_requests=True, add_managers=True) + + # Ensure that the manager is not duplicated + permissions = UserPortfolioPermission.objects.filter(portfolio=self.portfolio, user=manager) + self.assertEqual(permissions.count(), 1) \ No newline at end of file From b28ddf296ecf6985e3145d8dfc529d12bd12a4e9 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Sun, 2 Feb 2025 17:54:53 -0600 Subject: [PATCH 03/24] fix tests --- .../commands/create_federal_portfolio.py | 54 +++++++++++++++++- .../tests/test_management_scripts.py | 55 +++++++++++++------ 2 files changed, 88 insertions(+), 21 deletions(-) diff --git a/src/registrar/management/commands/create_federal_portfolio.py b/src/registrar/management/commands/create_federal_portfolio.py index 095d6d9f5..7a90deed1 100644 --- a/src/registrar/management/commands/create_federal_portfolio.py +++ b/src/registrar/management/commands/create_federal_portfolio.py @@ -30,6 +30,8 @@ class Command(BaseCommand): self.failed_portfolios = set() self.added_managers = set() self.added_invitations = set() + self.failed_managers = set() + self.failed_invitations = set() def add_arguments(self, parser): """Add command line arguments to create federal portfolios. @@ -114,9 +116,15 @@ class Command(BaseCommand): TerminalHelper.colorful_logger(logger.info, TerminalColors.MAGENTA, message) try: # C901 'Command.handle' is too complex (12) - portfolio = self.handle_populate_portfolio(federal_agency, parse_domains, parse_requests, both) - portfolios.append(portfolio) - + # if the portfolio is already created, we don't want to create it again + portfolio = Portfolio.objects.filter(organization_name=federal_agency.agency) + if portfolio.exists(): + portfolio = portfolio.first() + message = f"Portfolio '{federal_agency.agency}' already exists. Skipping create." + TerminalHelper.colorful_logger(logger.info, TerminalColors.YELLOW, message) + else: + portfolio = self.handle_populate_portfolio(federal_agency, parse_domains, parse_requests, both) + portfolios.append(portfolio) logger.debug(f"add_managers: {add_managers}") if add_managers: logger.debug("Adding managers to portfolio") @@ -141,6 +149,25 @@ class Command(BaseCommand): display_as_str=True, ) + if add_managers: + TerminalHelper.log_script_run_summary( + self.added_managers, + self.failed_managers, + [], # can't skip managers, can only add or fail + log_header="----- MANAGERS ADDED -----", + debug=False, + display_as_str=True, + ) + + TerminalHelper.log_script_run_summary( + self.added_invitations, + self.failed_invitations, + [], # can't skip invitations, can only add or fail + log_header="----- INVITATIONS ADDED -----", + debug=False, + display_as_str=True, + ) + # POST PROCESSING STEP: Remove the federal agency if it matches the portfolio name. # We only do this for started domain requests. if parse_requests or both: @@ -190,11 +217,13 @@ class Command(BaseCommand): user=user, defaults={"roles": [UserPortfolioRoleChoices.ORGANIZATION_MEMBER]}, ) + self.added_managers.add(user) if created: logger.info(f"Added manager '{user}' to portfolio '{portfolio}'") else: logger.info(f"Manager '{user}' already exists in portfolio '{portfolio}'") except User.DoesNotExist: + self.failed_managers.add(user) logger.debug(f"User '{user}' does not exist") for manager in invited_managers: @@ -229,13 +258,20 @@ class Command(BaseCommand): logger.info(f"Created portfolio permission for '{user}' to portfolio '{portfolio}'") else: logger.info(f"Retrieved existing portfolio permission for '{user}' to portfolio '{portfolio}'") + + self.added_invitations.add(user) except User.DoesNotExist: PortfolioInvitation.objects.get_or_create( portfolio=portfolio, email=email, defaults={"status": PortfolioInvitation.PortfolioInvitationStatus.INVITED}, ) + self.added_invitations.add(email) logger.info(f"Created portfolio invitation for '{email}' to portfolio '{portfolio}'") + except Exception as exc: + self.failed_invitations.add(email) + logger.error(exc, exc_info=True) + logger.error(f"Failed to create portfolio invitation for '{email}' to portfolio '{portfolio}'") def post_process_started_domain_requests(self, agencies, portfolios): """ @@ -250,13 +286,20 @@ class Command(BaseCommand): # 2. Said portfolio (or portfolios) are only the ones specified at the start of the script. # 3. The domain request is in status "started". # Note: Both names are normalized so excess spaces are stripped and the string is lowercased. + message = f"agencies: {agencies}" + TerminalHelper.colorful_logger(logger.info, TerminalColors.MAGENTA, message) + domain_requests_to_update = DomainRequest.objects.filter( federal_agency__in=agencies, federal_agency__agency__isnull=False, status=DomainRequest.DomainRequestStatus.STARTED, organization_name__isnull=False, ) + message = (f"domain_requests_to_update: {domain_requests_to_update}") + TerminalHelper.colorful_logger(logger.info, TerminalColors.MAGENTA, message) portfolio_set = {normalize_string(portfolio.organization_name) for portfolio in portfolios if portfolio} + message = f"portfolio_set: {portfolio_set}" + TerminalHelper.colorful_logger(logger.info, TerminalColors.MAGENTA, message) # Update the request, assuming the given agency name matches the portfolio name updated_requests = [] @@ -265,6 +308,9 @@ class Command(BaseCommand): if agency_name in portfolio_set: req.federal_agency = None updated_requests.append(req) + + message = f"updated_requests: {updated_requests}" + TerminalHelper.colorful_logger(logger.info, TerminalColors.MAGENTA, message) # Execute the update and Log the results if TerminalHelper.prompt_for_execution( @@ -275,6 +321,8 @@ class Command(BaseCommand): ), prompt_title="Do you wish to commit this update to the database?", ): + message = f"prompted for execution" + TerminalHelper.colorful_logger(logger.info, TerminalColors.MAGENTA, message) DomainRequest.objects.bulk_update(updated_requests, ["federal_agency"]) TerminalHelper.colorful_logger(logger.info, TerminalColors.OKBLUE, "Action completed successfully.") diff --git a/src/registrar/tests/test_management_scripts.py b/src/registrar/tests/test_management_scripts.py index b2b217044..515c32782 100644 --- a/src/registrar/tests/test_management_scripts.py +++ b/src/registrar/tests/test_management_scripts.py @@ -1460,14 +1460,11 @@ class TestCreateFederalPortfolio(TestCase): first_name="first", last_name="last", email="mango@igorville.gov", federal_agency=self.executive_agency_2 ) - self.portfolio = Portfolio.objects.create(organization_name=self.federal_agency.agency, federal_agency=self.federal_agency, creator=self.user) - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): self.domain_request = completed_domain_request( status=DomainRequest.DomainRequestStatus.IN_REVIEW, generic_org_type=DomainRequest.OrganizationChoices.CITY, federal_agency=self.federal_agency, - portfolio=self.portfolio, user=self.user, ) self.domain_request.approve() @@ -1479,26 +1476,22 @@ class TestCreateFederalPortfolio(TestCase): status=DomainRequest.DomainRequestStatus.IN_REVIEW, generic_org_type=DomainRequest.OrganizationChoices.CITY, federal_agency=self.federal_agency, - portfolio=self.portfolio, user=self.user, organization_name="Test Federal Agency", ) self.domain_request_2.approve() self.domain_info_2 = DomainInformation.objects.filter(domain_request=self.domain_request_2).get() - self.domain_2 = Domain.objects.get(name="icecreamforigorville.gov") self.domain_request_3 = completed_domain_request( name="exec_1.gov", status=DomainRequest.DomainRequestStatus.IN_REVIEW, generic_org_type=DomainRequest.OrganizationChoices.FEDERAL, federal_agency=self.executive_agency_1, - portfolio=self.portfolio, user=self.user, organization_name="Executive Agency 1", ) self.domain_request_3.approve() self.domain_info_3 = self.domain_request_3.DomainRequest_info - self.domain = Domain.objects.get(name="exec_1.gov") self.domain_request_4 = completed_domain_request( name="exec_2.gov", @@ -1506,12 +1499,10 @@ class TestCreateFederalPortfolio(TestCase): generic_org_type=DomainRequest.OrganizationChoices.FEDERAL, federal_agency=self.executive_agency_2, user=self.user, - portfolio=self.portfolio, organization_name="Executive Agency 2", ) self.domain_request_4.approve() self.domain_info_4 = self.domain_request_4.DomainRequest_info - self.domain_4 = Domain.objects.get(name="exec_2.gov") def tearDown(self): DomainInformation.objects.all().delete() @@ -1529,7 +1520,7 @@ class TestCreateFederalPortfolio(TestCase): ): call_command("create_federal_portfolio", **kwargs) - @less_console_noise_decorator + # @less_console_noise_decorator def test_post_process_started_domain_requests_existing_portfolio(self): """Ensures that federal agency is cleared when agency name matches portfolio name. As the name implies, this implicitly tests the "post_process_started_domain_requests" function. @@ -1869,7 +1860,10 @@ class TestCreateFederalPortfolio(TestCase): UserDomainRole.objects.create(user=manager2, domain=self.domain, role=UserDomainRole.Roles.MANAGER) # Run the management command - self.run_create_federal_portfolio(agency_name=self.portfolio.organization_name, parse_requests=True, add_managers=True) + self.run_create_federal_portfolio(agency_name=self.federal_agency.agency, parse_requests=True, add_managers=True) + + # Check that the portfolio was created + self.portfolio = Portfolio.objects.get(federal_agency=self.federal_agency) # Check that the users have been added as portfolio managers permissions = UserPortfolioPermission.objects.filter(portfolio=self.portfolio, user__in=[manager1, manager2]) @@ -1890,13 +1884,13 @@ class TestCreateFederalPortfolio(TestCase): email="manager1@example.com", status=DomainInvitation.DomainInvitationStatus.INVITED ) - - # Ensure no existing PortfolioInvitation for the invited email - self.assertFalse(PortfolioInvitation.objects.filter(email="manager1@example.com", portfolio=self.portfolio).exists()) - + # Run the management command self.run_create_federal_portfolio(agency_name=self.federal_agency.agency, parse_requests=True, add_managers=True) + # Check that the portfolio was created + self.portfolio = Portfolio.objects.get(federal_agency=self.federal_agency) + # Check that a PortfolioInvitation has been created for the invited email invitation = PortfolioInvitation.objects.get(email="manager1@example.com", portfolio=self.portfolio) @@ -1909,9 +1903,9 @@ class TestCreateFederalPortfolio(TestCase): # Verify that no duplicate invitations are created self.run_create_federal_portfolio(agency_name=self.federal_agency.agency, parse_requests=True, add_managers=True) - duplicated_invitations = PortfolioInvitation.objects.filter(email="manager1@example.com", portfolio=self.portfolio) + invitations = PortfolioInvitation.objects.filter(email="manager1@example.com", portfolio=self.portfolio) self.assertEqual( - duplicated_invitations.count(), + invitations.count(), 1, "Duplicate PortfolioInvitation should not be created for the same email and portfolio." ) @@ -1922,6 +1916,9 @@ class TestCreateFederalPortfolio(TestCase): # Create a manager manager = User.objects.create(username="manager", email="manager@example.com") UserDomainRole.objects.create(user=manager, domain=self.domain, role=UserDomainRole.Roles.MANAGER) + + # Create a pre-existing portfolio + self.portfolio = Portfolio.objects.create(organization_name=self.federal_agency.agency, federal_agency=self.federal_agency, creator=self.user) # Manually add the manager to the portfolio UserPortfolioPermission.objects.create(portfolio=self.portfolio, user=manager, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER]) @@ -1931,4 +1928,26 @@ class TestCreateFederalPortfolio(TestCase): # Ensure that the manager is not duplicated permissions = UserPortfolioPermission.objects.filter(portfolio=self.portfolio, user=manager) - self.assertEqual(permissions.count(), 1) \ No newline at end of file + self.assertEqual(permissions.count(), 1) + + @less_console_noise_decorator + def test_add_managers_portfolio_already_exists(self): + """Test that managers are skipped when the portfolio already exists.""" + + # Create a pre-existing portfolio + self.portfolio = Portfolio.objects.create(organization_name=self.federal_agency.agency, federal_agency=self.federal_agency, creator=self.user) + + # Create users and assign them as domain managers + manager1 = User.objects.create(username="manager1", email="manager1@example.com") + manager2 = User.objects.create(username="manager2", email="manager2@example.com") + UserDomainRole.objects.create(user=manager1, domain=self.domain, role=UserDomainRole.Roles.MANAGER) + UserDomainRole.objects.create(user=manager2, domain=self.domain, role=UserDomainRole.Roles.MANAGER) + + # Run the management command + self.run_create_federal_portfolio(agency_name=self.federal_agency.agency, parse_requests=True, add_managers=True) + + # Check that managers were added to the portfolio + permissions = UserPortfolioPermission.objects.filter(portfolio=self.portfolio, user__in=[manager1, manager2]) + self.assertEqual(permissions.count(), 2) + for perm in permissions: + self.assertIn(UserPortfolioRoleChoices.ORGANIZATION_MEMBER, perm.roles) From bdd57c8cfda0abbaa7f3e3a0e8e1b9b8460ceb93 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 3 Feb 2025 11:20:09 -0600 Subject: [PATCH 04/24] linter and final test fixes --- .../commands/create_federal_portfolio.py | 42 ++++--- .../tests/test_management_scripts.py | 118 +++++++++++------- 2 files changed, 94 insertions(+), 66 deletions(-) diff --git a/src/registrar/management/commands/create_federal_portfolio.py b/src/registrar/management/commands/create_federal_portfolio.py index 2f2cc2625..71004dc49 100644 --- a/src/registrar/management/commands/create_federal_portfolio.py +++ b/src/registrar/management/commands/create_federal_portfolio.py @@ -89,7 +89,7 @@ class Command(BaseCommand): help="Only add suborganizations to newly created portfolios, skip existing ones.", ) - def handle(self, **options): + def handle(self, **options): # noqa: C901 agency_name = options.get("agency_name") branch = options.get("branch") parse_requests = options.get("parse_requests") @@ -116,7 +116,6 @@ class Command(BaseCommand): ) else: raise CommandError(f"Cannot find '{branch}' federal agencies in our database.") - portfolios = [] for federal_agency in agencies: message = f"Processing federal agency '{federal_agency.agency}'..." @@ -127,6 +126,8 @@ class Command(BaseCommand): federal_agency, parse_domains, parse_requests, both, skip_existing_portfolios ) portfolios.append(portfolio) + if add_managers: + self.add_managers_to_portfolio(portfolio) except Exception as exec: self.failed_portfolios.add(federal_agency) logger.error(exec) @@ -192,31 +193,32 @@ class Command(BaseCommand): This includes adding them to the correct group and creating portfolio invitations. """ logger.info(f"Adding managers for portfolio {portfolio}") - + # Fetch all domains associated with the portfolio domains = Domain.objects.filter(domain_info__portfolio=portfolio) logger.debug(f"domains: {domains}") - domain_managers = set() + domain_managers: set[int] = set() # Fetch all users with manager roles for the domains - managers = UserDomainRole.objects.filter( - domain__in=domains, - role=UserDomainRole.Roles.MANAGER - ).values_list('user', flat=True) + managers = UserDomainRole.objects.filter(domain__in=domains, role=UserDomainRole.Roles.MANAGER).values_list( + "user", flat=True + ) domain_managers.update(managers) - invited_managers = set() + invited_managers: set[str] = set() # Get the emails of invited managers for domain in domains: - domain_invitations = DomainInvitation.objects.filter(domain=domain, status=DomainInvitation.DomainInvitationStatus.INVITED).values_list('email', flat=True) + domain_invitations = DomainInvitation.objects.filter( + domain=domain, status=DomainInvitation.DomainInvitationStatus.INVITED + ).values_list("email", flat=True) invited_managers.update(domain_invitations) logger.debug(f"invited_managers: {invited_managers}") - for manager in domain_managers: + for id in domain_managers: try: # manager is a user id - user = User.objects.get(id=manager) + user = User.objects.get(id=id) _, created = UserPortfolioPermission.objects.get_or_create( portfolio=portfolio, user=user, @@ -230,9 +232,9 @@ class Command(BaseCommand): except User.DoesNotExist: self.failed_managers.add(user) logger.debug(f"User '{user}' does not exist") - - for manager in invited_managers: - self.create_portfolio_invitation(portfolio, manager) + + for email in invited_managers: + self.create_portfolio_invitation(portfolio, email) def create_portfolio_invitation(self, portfolio: Portfolio, email: str): """ @@ -252,18 +254,18 @@ class Command(BaseCommand): logger.info(f"Created portfolio invitation for '{user}' to portfolio '{portfolio}'") else: logger.info(f"Retrieved existing portfolio invitation for '{user}' to portfolio '{portfolio}'") - + # Assign portfolio permissions _, created = UserPortfolioPermission.objects.get_or_create( portfolio=portfolio, user=user, - defaults={"role": UserPortfolioPermission.RoleChoices.MANAGER}, + defaults={"role": UserPortfolioRoleChoices.ORGANIZATION_MEMBER}, ) if created: logger.info(f"Created portfolio permission for '{user}' to portfolio '{portfolio}'") else: logger.info(f"Retrieved existing portfolio permission for '{user}' to portfolio '{portfolio}'") - + self.added_invitations.add(user) except User.DoesNotExist: PortfolioInvitation.objects.get_or_create( @@ -316,7 +318,7 @@ class Command(BaseCommand): if agency_name in portfolio_set: req.federal_agency = None updated_requests.append(req) - + message = f"updated_requests: {updated_requests}" TerminalHelper.colorful_logger(logger.info, TerminalColors.MAGENTA, message) @@ -329,7 +331,7 @@ class Command(BaseCommand): ), prompt_title="Do you wish to commit this update to the database?", ): - message = f"prompted for execution" + message = "prompted for execution" TerminalHelper.colorful_logger(logger.info, TerminalColors.MAGENTA, message) DomainRequest.objects.bulk_update(updated_requests, ["federal_agency"]) TerminalHelper.colorful_logger(logger.info, TerminalColors.OKBLUE, "Action completed successfully.") diff --git a/src/registrar/tests/test_management_scripts.py b/src/registrar/tests/test_management_scripts.py index 2ca4e4ffe..ac507cdf2 100644 --- a/src/registrar/tests/test_management_scripts.py +++ b/src/registrar/tests/test_management_scripts.py @@ -3,17 +3,11 @@ import boto3_mocking # type: ignore from datetime import date, datetime, time from django.core.management import call_command from django.test import TestCase, override_settings -<<<<<<< HEAD -from registrar.models.portfolio_invitation import PortfolioInvitation -from registrar.models.senior_official import SeniorOfficial -from registrar.models.user_portfolio_permission import UserPortfolioPermission -from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices -======= from registrar.models.domain_group import DomainGroup from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.senior_official import SeniorOfficial from registrar.models.user_portfolio_permission import UserPortfolioPermission ->>>>>>> origin/main +from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.utility.constants import BranchChoices from django.utils import timezone from django.utils.module_loading import import_string @@ -1472,7 +1466,7 @@ class TestCreateFederalPortfolio(TestCase): self.executive_so_2 = SeniorOfficial.objects.create( first_name="first", last_name="last", email="mango@igorville.gov", federal_agency=self.executive_agency_2 ) - + with boto3_mocking.clients.handler_for("sesv2", self.mock_client): self.domain_request = completed_domain_request( status=DomainRequest.DomainRequestStatus.IN_REVIEW, @@ -1533,7 +1527,7 @@ class TestCreateFederalPortfolio(TestCase): ): call_command("create_federal_portfolio", **kwargs) - # @less_console_noise_decorator + @less_console_noise_decorator def test_post_process_started_domain_requests_existing_portfolio(self): """Ensures that federal agency is cleared when agency name matches portfolio name. As the name implies, this implicitly tests the "post_process_started_domain_requests" function. @@ -1862,7 +1856,6 @@ class TestCreateFederalPortfolio(TestCase): self.assertEqual(existing_portfolio.notes, "Old notes") self.assertEqual(existing_portfolio.creator, self.user) -<<<<<<< HEAD @less_console_noise_decorator def test_add_managers_from_domains(self): """Test that all domain managers are added as portfolio managers.""" @@ -1872,56 +1865,55 @@ class TestCreateFederalPortfolio(TestCase): manager2 = User.objects.create(username="manager2", email="manager2@example.com") UserDomainRole.objects.create(user=manager1, domain=self.domain, role=UserDomainRole.Roles.MANAGER) UserDomainRole.objects.create(user=manager2, domain=self.domain, role=UserDomainRole.Roles.MANAGER) - + # Run the management command - self.run_create_federal_portfolio(agency_name=self.federal_agency.agency, parse_requests=True, add_managers=True) + self.run_create_federal_portfolio(agency_name=self.federal_agency.agency, parse_domains=True, add_managers=True) # Check that the portfolio was created self.portfolio = Portfolio.objects.get(federal_agency=self.federal_agency) - + # Check that the users have been added as portfolio managers permissions = UserPortfolioPermission.objects.filter(portfolio=self.portfolio, user__in=[manager1, manager2]) - print(UserPortfolioPermission.objects.all()) - # Check that the users have been added as portfolio managers + # Check that the users have been added as portfolio managers self.assertEqual(permissions.count(), 2) for perm in permissions: self.assertIn(UserPortfolioRoleChoices.ORGANIZATION_MEMBER, perm.roles) - + @less_console_noise_decorator def test_add_invited_managers(self): """Test that invited domain managers receive portfolio invitations.""" # create a domain invitation for the manager _ = DomainInvitation.objects.create( - domain=self.domain, - email="manager1@example.com", - status=DomainInvitation.DomainInvitationStatus.INVITED - ) - + domain=self.domain, email="manager1@example.com", status=DomainInvitation.DomainInvitationStatus.INVITED + ) + # Run the management command - self.run_create_federal_portfolio(agency_name=self.federal_agency.agency, parse_requests=True, add_managers=True) - + self.run_create_federal_portfolio(agency_name=self.federal_agency.agency, parse_domains=True, add_managers=True) + # Check that the portfolio was created self.portfolio = Portfolio.objects.get(federal_agency=self.federal_agency) # Check that a PortfolioInvitation has been created for the invited email invitation = PortfolioInvitation.objects.get(email="manager1@example.com", portfolio=self.portfolio) - + # Verify the status of the invitation remains INVITED self.assertEqual( invitation.status, PortfolioInvitation.PortfolioInvitationStatus.INVITED, - "PortfolioInvitation status should remain INVITED for non-existent users." + "PortfolioInvitation status should remain INVITED for non-existent users.", ) - + # Verify that no duplicate invitations are created - self.run_create_federal_portfolio(agency_name=self.federal_agency.agency, parse_requests=True, add_managers=True) + self.run_create_federal_portfolio( + agency_name=self.federal_agency.agency, parse_requests=True, add_managers=True + ) invitations = PortfolioInvitation.objects.filter(email="manager1@example.com", portfolio=self.portfolio) self.assertEqual( invitations.count(), 1, - "Duplicate PortfolioInvitation should not be created for the same email and portfolio." + "Duplicate PortfolioInvitation should not be created for the same email and portfolio.", ) @less_console_noise_decorator @@ -1931,41 +1923,76 @@ class TestCreateFederalPortfolio(TestCase): manager = User.objects.create(username="manager", email="manager@example.com") UserDomainRole.objects.create(user=manager, domain=self.domain, role=UserDomainRole.Roles.MANAGER) - # Create a pre-existing portfolio - self.portfolio = Portfolio.objects.create(organization_name=self.federal_agency.agency, federal_agency=self.federal_agency, creator=self.user) - + # Create a pre-existing portfolio + self.portfolio = Portfolio.objects.create( + organization_name=self.federal_agency.agency, federal_agency=self.federal_agency, creator=self.user + ) + # Manually add the manager to the portfolio - UserPortfolioPermission.objects.create(portfolio=self.portfolio, user=manager, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER]) - + UserPortfolioPermission.objects.create( + portfolio=self.portfolio, user=manager, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + ) + # Run the management command - self.run_create_federal_portfolio(agency_name=self.federal_agency.agency, parse_requests=True, add_managers=True) - + self.run_create_federal_portfolio( + agency_name=self.federal_agency.agency, parse_requests=True, add_managers=True + ) + # Ensure that the manager is not duplicated permissions = UserPortfolioPermission.objects.filter(portfolio=self.portfolio, user=manager) self.assertEqual(permissions.count(), 1) @less_console_noise_decorator - def test_add_managers_portfolio_already_exists(self): + def test_add_managers_skip_existing_portfolios(self): """Test that managers are skipped when the portfolio already exists.""" - - # Create a pre-existing portfolio - self.portfolio = Portfolio.objects.create(organization_name=self.federal_agency.agency, federal_agency=self.federal_agency, creator=self.user) - + + # Create a pre-existing portfolio + self.portfolio = Portfolio.objects.create( + organization_name=self.federal_agency.agency, federal_agency=self.federal_agency, creator=self.user + ) + + domain_request_1 = completed_domain_request( + name="domain1.gov", + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + generic_org_type=DomainRequest.OrganizationChoices.CITY, + federal_agency=self.federal_agency, + user=self.user, + portfolio=self.portfolio, + ) + domain_request_1.approve() + domain1 = Domain.objects.get(name="domain1.gov") + + domain_request_2 = completed_domain_request( + name="domain2.gov", + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + generic_org_type=DomainRequest.OrganizationChoices.CITY, + federal_agency=self.federal_agency, + user=self.user, + portfolio=self.portfolio, + ) + domain_request_2.approve() + domain2 = Domain.objects.get(name="domain2.gov") + # Create users and assign them as domain managers manager1 = User.objects.create(username="manager1", email="manager1@example.com") manager2 = User.objects.create(username="manager2", email="manager2@example.com") - UserDomainRole.objects.create(user=manager1, domain=self.domain, role=UserDomainRole.Roles.MANAGER) - UserDomainRole.objects.create(user=manager2, domain=self.domain, role=UserDomainRole.Roles.MANAGER) + UserDomainRole.objects.create(user=manager1, domain=domain1, role=UserDomainRole.Roles.MANAGER) + UserDomainRole.objects.create(user=manager2, domain=domain2, role=UserDomainRole.Roles.MANAGER) # Run the management command - self.run_create_federal_portfolio(agency_name=self.federal_agency.agency, parse_requests=True, add_managers=True) - + self.run_create_federal_portfolio( + agency_name=self.federal_agency.agency, + parse_requests=True, + add_managers=True, + skip_existing_portfolios=True, + ) + # Check that managers were added to the portfolio permissions = UserPortfolioPermission.objects.filter(portfolio=self.portfolio, user__in=[manager1, manager2]) self.assertEqual(permissions.count(), 2) for perm in permissions: self.assertIn(UserPortfolioRoleChoices.ORGANIZATION_MEMBER, perm.roles) -======= + def test_skip_existing_portfolios(self): """Tests the skip_existing_portfolios to ensure that it doesn't add suborgs, domain requests, and domain info.""" @@ -2466,4 +2493,3 @@ class TestRemovePortfolios(TestCase): # Check that the portfolio was deleted self.assertFalse(Portfolio.objects.filter(organization_name="Test with suborg").exists()) ->>>>>>> origin/main From dca2f635a287fb845ea1becfdf9c263f0d2908e4 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Mon, 3 Feb 2025 13:07:36 -0700 Subject: [PATCH 05/24] Shrink max-width of tables in widescreen. Constrict Action column to 80px. Add Action column updates to Members table. --- src/docker-compose.yml | 2 ++ .../src/js/getgov/table-domain-requests.js | 4 +-- .../assets/src/js/getgov/table-domains.js | 2 +- .../assets/src/js/getgov/table-members.js | 30 +++++++------------ .../assets/src/sass/_theme/_base.scss | 6 +++- .../templates/includes/members_table.html | 2 +- 6 files changed, 21 insertions(+), 25 deletions(-) diff --git a/src/docker-compose.yml b/src/docker-compose.yml index 5ad6d0ce6..09bf8243e 100644 --- a/src/docker-compose.yml +++ b/src/docker-compose.yml @@ -79,6 +79,8 @@ services: - POSTGRES_DB=app - POSTGRES_USER=user - POSTGRES_PASSWORD=feedabee + ports: + - "5432:5432" node: build: diff --git a/src/registrar/assets/src/js/getgov/table-domain-requests.js b/src/registrar/assets/src/js/getgov/table-domain-requests.js index f667a96b5..9a78a4551 100644 --- a/src/registrar/assets/src/js/getgov/table-domain-requests.js +++ b/src/registrar/assets/src/js/getgov/table-domain-requests.js @@ -116,8 +116,8 @@ export class DomainRequestsTable extends BaseTable { ${request.status} - -
+ +
${markupForSuborganizationRow} - + Extra Actions`; - let tableHeaderRow = this.tableWrapper.querySelector('thead tr'); - tableHeaderRow.appendChild(extraActionsHeader); - } return { 'hasAdditionalActions': hasEditPermission, 'UserPortfolioPermissionChoices' : data.UserPortfolioPermissionChoices @@ -121,15 +109,17 @@ export class MembersTable extends BaseTable { ${last_active.display_value} - - - - ${member.action_label} ${member.name} - + +
+ + + ${member.action_label} ${member.name} + + ${customTableOptions.hasAdditionalActions ? kebabHTML : ''} +
- ${customTableOptions.hasAdditionalActions ? ''+kebabHTML+'' : ''} `; tbody.appendChild(row); if (domainsHTML || permissionsHTML) { diff --git a/src/registrar/assets/src/sass/_theme/_base.scss b/src/registrar/assets/src/sass/_theme/_base.scss index be3b89baf..8ab914e6c 100644 --- a/src/registrar/assets/src/sass/_theme/_base.scss +++ b/src/registrar/assets/src/sass/_theme/_base.scss @@ -1,7 +1,7 @@ @use "uswds-core" as *; @use "cisa_colors" as *; -$widescreen-max-width: 1920px; +$widescreen-max-width: 1536px; //1920px; $widescreen-x-padding: 4.5rem; $hot-pink: #FFC3F9; @@ -275,3 +275,7 @@ abbr[title] { .width-quarter { width: 25%; } + +.width--action-column { + max-width: 80px; +} diff --git a/src/registrar/templates/includes/members_table.html b/src/registrar/templates/includes/members_table.html index cc308619a..18f6b3546 100644 --- a/src/registrar/templates/includes/members_table.html +++ b/src/registrar/templates/includes/members_table.html @@ -59,7 +59,7 @@ role="columnheader" id="header-action" > - Action + Action From d89f324e6f42f8110e2030ad18bd3b9ebf5dac09 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 3 Feb 2025 14:45:47 -0600 Subject: [PATCH 06/24] remove log statements --- .../management/commands/create_federal_portfolio.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/registrar/management/commands/create_federal_portfolio.py b/src/registrar/management/commands/create_federal_portfolio.py index 71004dc49..7ed7316be 100644 --- a/src/registrar/management/commands/create_federal_portfolio.py +++ b/src/registrar/management/commands/create_federal_portfolio.py @@ -293,8 +293,6 @@ class Command(BaseCommand): # 2. Said portfolio (or portfolios) are only the ones specified at the start of the script. # 3. The domain request is in status "started". # Note: Both names are normalized so excess spaces are stripped and the string is lowercased. - message = f"agencies: {agencies}" - TerminalHelper.colorful_logger(logger.info, TerminalColors.MAGENTA, message) domain_requests_to_update = DomainRequest.objects.filter( federal_agency__in=agencies, @@ -308,8 +306,6 @@ class Command(BaseCommand): return portfolio_set = {normalize_string(portfolio.organization_name) for portfolio in portfolios if portfolio} - message = f"portfolio_set: {portfolio_set}" - TerminalHelper.colorful_logger(logger.info, TerminalColors.MAGENTA, message) # Update the request, assuming the given agency name matches the portfolio name updated_requests = [] @@ -319,9 +315,6 @@ class Command(BaseCommand): req.federal_agency = None updated_requests.append(req) - message = f"updated_requests: {updated_requests}" - TerminalHelper.colorful_logger(logger.info, TerminalColors.MAGENTA, message) - # Execute the update and Log the results if TerminalHelper.prompt_for_execution( system_exit_on_terminate=False, @@ -331,8 +324,6 @@ class Command(BaseCommand): ), prompt_title="Do you wish to commit this update to the database?", ): - message = "prompted for execution" - TerminalHelper.colorful_logger(logger.info, TerminalColors.MAGENTA, message) DomainRequest.objects.bulk_update(updated_requests, ["federal_agency"]) TerminalHelper.colorful_logger(logger.info, TerminalColors.OKBLUE, "Action completed successfully.") From 72da75773efb8ece3d54f3b8beddfc089b89a487 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Mon, 3 Feb 2025 13:55:00 -0700 Subject: [PATCH 07/24] Icon alignments --- src/registrar/assets/src/js/getgov/table-domain-requests.js | 2 +- src/registrar/assets/src/js/getgov/table-domains.js | 2 +- src/registrar/assets/src/js/getgov/table-members.js | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/table-domain-requests.js b/src/registrar/assets/src/js/getgov/table-domain-requests.js index 9a78a4551..8556b714f 100644 --- a/src/registrar/assets/src/js/getgov/table-domain-requests.js +++ b/src/registrar/assets/src/js/getgov/table-domain-requests.js @@ -119,7 +119,7 @@ export class DomainRequestsTable extends BaseTable {
- ${request.requested_domain ? request.requested_domain : 'New domain request'} diff --git a/src/registrar/assets/src/js/getgov/table-domains.js b/src/registrar/assets/src/js/getgov/table-domains.js index cb0372584..7eeaebab4 100644 --- a/src/registrar/assets/src/js/getgov/table-domains.js +++ b/src/registrar/assets/src/js/getgov/table-domains.js @@ -58,7 +58,7 @@ export class DomainsTable extends BaseTable { ${markupForSuborganizationRow} - ${domain.name} diff --git a/src/registrar/assets/src/js/getgov/table-members.js b/src/registrar/assets/src/js/getgov/table-members.js index d676e7801..29d140185 100644 --- a/src/registrar/assets/src/js/getgov/table-members.js +++ b/src/registrar/assets/src/js/getgov/table-members.js @@ -110,9 +110,9 @@ export class MembersTable extends BaseTable { ${last_active.display_value} -
+
- ${member.name} From 78f447fad09fa38296ac5391ee5b182b3396f70a Mon Sep 17 00:00:00 2001 From: CocoByte Date: Mon, 3 Feb 2025 14:00:49 -0700 Subject: [PATCH 08/24] cleanup --- src/registrar/assets/src/js/getgov/table-domains.js | 2 +- src/registrar/assets/src/sass/_theme/_base.scss | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/table-domains.js b/src/registrar/assets/src/js/getgov/table-domains.js index 7eeaebab4..6536e7f6e 100644 --- a/src/registrar/assets/src/js/getgov/table-domains.js +++ b/src/registrar/assets/src/js/getgov/table-domains.js @@ -56,7 +56,7 @@ export class DomainsTable extends BaseTable { ${markupForSuborganizationRow} - +
-
-
- -
-
+ {% with label_text="Search all domains" item_name="edit-member-domains" aria_label_text="Member domains search component" %} + {% include "includes/search.html" %} + {% endwith %}
@@ -85,7 +47,7 @@ member domains - Assigned domains + Assigned domains Domains diff --git a/src/registrar/templates/includes/member_domains_table.html b/src/registrar/templates/includes/member_domains_table.html index d7839e485..4e63fdbc3 100644 --- a/src/registrar/templates/includes/member_domains_table.html +++ b/src/registrar/templates/includes/member_domains_table.html @@ -1,5 +1,3 @@ -{% load static %} - {% if member %} -
+
-
-
- -
-
+ {% with label_text="Domains assigned to " %} + {% if member %} + {% with label_text=label_text|add:member.email item_name="member-domains" aria_label_text="Member domains search component" %} + {% include "includes/search.html" %} + {% endwith %} + {% else %} + {% with label_text=label_text|add:portfolio_invitation.email item_name="member-domains" aria_label_text="Member domains search component" %} + {% include "includes/search.html" %} + {% endwith %} + {% endif %} + {% endwith %}
diff --git a/src/registrar/templates/includes/search.html b/src/registrar/templates/includes/search.html new file mode 100644 index 000000000..a276f27f1 --- /dev/null +++ b/src/registrar/templates/includes/search.html @@ -0,0 +1,34 @@ +{% load static %} + +
+
+ +
+
From f1b76ceb03a68afe6408cb54a9f71b54e2bd65c1 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 5 Feb 2025 16:12:02 -0500 Subject: [PATCH 11/24] cleanp --- src/registrar/templates/includes/member_domains_edit_table.html | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/registrar/templates/includes/member_domains_edit_table.html b/src/registrar/templates/includes/member_domains_edit_table.html index cefc30c15..fd63e5aa7 100644 --- a/src/registrar/templates/includes/member_domains_edit_table.html +++ b/src/registrar/templates/includes/member_domains_edit_table.html @@ -1,5 +1,3 @@ -{% load static %} - {% if member %} Date: Thu, 6 Feb 2025 13:09:27 -0500 Subject: [PATCH 12/24] layout tweak --- src/registrar/templates/includes/search.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/templates/includes/search.html b/src/registrar/templates/includes/search.html index a276f27f1..8c9f60cdb 100644 --- a/src/registrar/templates/includes/search.html +++ b/src/registrar/templates/includes/search.html @@ -22,7 +22,7 @@ alt="Search" /> -
  • -
  • -