From 50d1abf253bd872a7fae2f6568abcbabc1ef8cd6 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 16 Aug 2024 10:03:40 -0500 Subject: [PATCH 01/59] initial work for migrating domainrequest model --- .../commands/populate_domain_request_dates.py | 21 ++++++++++++ src/registrar/models/domain_request.py | 33 ++++++++++++++++--- 2 files changed, 49 insertions(+), 5 deletions(-) create mode 100644 src/registrar/management/commands/populate_domain_request_dates.py diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py new file mode 100644 index 000000000..7ea898c4c --- /dev/null +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -0,0 +1,21 @@ +import logging +from django.core.management import BaseCommand +from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors +from registrar.models import DomainRequest + +logger = logging.getLogger(__name__) + + +class Command(BaseCommand, PopulateScriptTemplate): + help = "Loops through each valid domain request object and populates the last_status_update and first_submitted_date" + + def handle(self, **kwargs): + """Loops through each valid DomainRequest object and populates its last_status_update and first_submitted_date values""" + self.mass_update_records(DomainRequest, ["last_status_update", "last_submitted_date"]) + + def update_record(self, record: DomainRequest): + """Defines how we update the first_submitted_date and last_status_update fields""" + record.set_dates() + logger.info( + f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}{TerminalColors.OKCYAN}, last status update:" f"{record.last_status_update}{TerminalColors.OKCYAN}" + ) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 363de213b..d2c90243c 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -563,15 +563,32 @@ class DomainRequest(TimeStampedModel): help_text="Acknowledged .gov acceptable use policy", ) - # submission date records when domain request is submitted - submission_date = models.DateField( + # initial submission date records when domain request was first submitted + first_submitted_date = models.DateField( null=True, blank=True, default=None, verbose_name="submitted at", - help_text="Date submitted", + help_text="Date initially submitted", ) + # last submission date records when domain request was last submitted + last_submitted_date = models.DateField( + null=True, + blank=True, + default=None, + verbose_name="submitted at", + help_text="Date last submitted", + ) + + # last status update records when domain request status was last updated by an admin or analyst + last_status_update = models.DateField( + null=True, + blank=True, + default=None, + verbose_name="last updated at", + help_text="Date of last status updated", + ) notes = models.TextField( null=True, blank=True, @@ -621,6 +638,9 @@ class DomainRequest(TimeStampedModel): self.sync_organization_type() self.sync_yes_no_form_fields() + if self._cached_status != self.status: + self.last_status_update = timezone.now().date() + super().save(*args, **kwargs) # Handle the action needed email. @@ -796,9 +816,12 @@ class DomainRequest(TimeStampedModel): DraftDomain = apps.get_model("registrar.DraftDomain") if not DraftDomain.string_could_be_domain(self.requested_domain.name): raise ValueError("Requested domain is not a valid domain name.") + + if not self.first_submitted_date: + self.first_submitted_date = timezone.now().date() - # Update submission_date to today - self.submission_date = timezone.now().date() + # Update last_submission_date to today + self.last_submitted_date = timezone.now().date() self.save() # Limit email notifications to transitions from Started and Withdrawn From f054cfa5cf3144fdaf8c73573177a2a78afca82c Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Fri, 16 Aug 2024 15:35:58 -0500 Subject: [PATCH 02/59] First complete pass --- src/registrar/admin.py | 6 ++- .../commands/populate_domain_request_dates.py | 29 +++++++++++-- .../commands/utility/terminal_helper.py | 2 +- ...0118_add_domainrequest_submission_dates.py | 43 +++++++++++++++++++ src/registrar/models/domain_request.py | 9 ++-- 5 files changed, 78 insertions(+), 11 deletions(-) create mode 100644 src/registrar/migrations/0118_add_domainrequest_submission_dates.py diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 18c1052fc..3f3547341 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1649,7 +1649,9 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): # Columns list_display = [ "requested_domain", - "submission_date", + "first_submitted_date", + "last_submitted_date", + "last_status_update", "status", "generic_org_type", "federal_type", @@ -1852,7 +1854,7 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): # Table ordering # NOTE: This impacts the select2 dropdowns (combobox) # Currentl, there's only one for requests on DomainInfo - ordering = ["-submission_date", "requested_domain__name"] + ordering = ["-last_submitted_date", "requested_domain__name"] change_form_template = "django/admin/domain_request_change_form.html" diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index 7ea898c4c..90fc06dcf 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -2,20 +2,41 @@ import logging from django.core.management import BaseCommand from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors from registrar.models import DomainRequest +from auditlog.models import LogEntry +from django.core.exceptions import ObjectDoesNotExist logger = logging.getLogger(__name__) class Command(BaseCommand, PopulateScriptTemplate): - help = "Loops through each valid domain request object and populates the last_status_update and first_submitted_date" + help = "Loops through each domain request object and populates the last_status_update and first_submitted_date" def handle(self, **kwargs): - """Loops through each valid DomainRequest object and populates its last_status_update and first_submitted_date values""" - self.mass_update_records(DomainRequest, ["last_status_update", "last_submitted_date"]) + """Loops through each DomainRequest object and populates its last_status_update and first_submitted_date values""" + self.mass_update_records(DomainRequest, None, ["last_status_update", "first_submitted_date"]) def update_record(self, record: DomainRequest): """Defines how we update the first_submitted_date and last_status_update fields""" - record.set_dates() + try: + # Retrieve and order audit log entries by timestamp in descending order + audit_log_entries = LogEntry.objects.filter(object_pk=record.pk).order_by("-timestamp") + + # Loop through logs in descending order to find most recent status change + for log_entry in audit_log_entries: + if "status" in log_entry.changes: + record.last_status_update = log_entry.timestamp.date() + break + + # Loop through logs in ascending order to find first submission + for log_entry in audit_log_entries.reverse(): + if log_entry.changes_dict['status'](1) == 'Submitted': + record.first_submitted_date = log_entry.timestamp.date() + + except ObjectDoesNotExist as e: + logger.error(f"Object with object_pk {record.pk} does not exist: {e}") + except Exception as e: + logger.error(f"An error occurred during update_record: {e}") + logger.info( f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}{TerminalColors.OKCYAN}, last status update:" f"{record.last_status_update}{TerminalColors.OKCYAN}" ) diff --git a/src/registrar/management/commands/utility/terminal_helper.py b/src/registrar/management/commands/utility/terminal_helper.py index 2c69e1080..b9e11be5d 100644 --- a/src/registrar/management/commands/utility/terminal_helper.py +++ b/src/registrar/management/commands/utility/terminal_helper.py @@ -86,7 +86,7 @@ class PopulateScriptTemplate(ABC): You must define update_record before you can use this function. """ - records = object_class.objects.filter(**filter_conditions) + records = object_class.objects.filter(**filter_conditions) if filter_conditions else object_class.objects.all() readable_class_name = self.get_class_name(object_class) # Code execution will stop here if the user prompts "N" diff --git a/src/registrar/migrations/0118_add_domainrequest_submission_dates.py b/src/registrar/migrations/0118_add_domainrequest_submission_dates.py new file mode 100644 index 000000000..c971b2f9a --- /dev/null +++ b/src/registrar/migrations/0118_add_domainrequest_submission_dates.py @@ -0,0 +1,43 @@ +# Generated by Django 4.2.10 on 2024-08-16 15:28 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0117_alter_portfolioinvitation_portfolio_additional_permissions_and_more"), + ] + + operations = [ + migrations.RenameField( + model_name="domainrequest", + old_name="submission_date", + new_name="last_submitted_date", + ), + migrations.AlterField( + model_name="domainrequest", + name="last_submitted_date", + field=models.DateField( + blank=True, default=None, help_text="Date last submitted", null=True, verbose_name="last submitted on" + ), + ), + migrations.AddField( + model_name="domainrequest", + name="first_submitted_date", + field=models.DateField( + blank=True, default=None, help_text="Date initially submitted", null=True, verbose_name="first submitted on" + ), + ), + migrations.AddField( + model_name="domainrequest", + name="last_status_update", + field=models.DateField( + blank=True, + default=None, + help_text="Date of last status updated", + null=True, + verbose_name="last updated on", + ), + ), + ] diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index d2c90243c..e9711f0ae 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -568,7 +568,7 @@ class DomainRequest(TimeStampedModel): null=True, blank=True, default=None, - verbose_name="submitted at", + verbose_name="first submitted on", help_text="Date initially submitted", ) @@ -577,7 +577,7 @@ class DomainRequest(TimeStampedModel): null=True, blank=True, default=None, - verbose_name="submitted at", + verbose_name="last submitted on", help_text="Date last submitted", ) @@ -586,7 +586,7 @@ class DomainRequest(TimeStampedModel): null=True, blank=True, default=None, - verbose_name="last updated at", + verbose_name="last updated on", help_text="Date of last status updated", ) notes = models.TextField( @@ -816,7 +816,8 @@ class DomainRequest(TimeStampedModel): DraftDomain = apps.get_model("registrar.DraftDomain") if not DraftDomain.string_could_be_domain(self.requested_domain.name): raise ValueError("Requested domain is not a valid domain name.") - + + # if the domain has not been submitted before this must be the first time if not self.first_submitted_date: self.first_submitted_date = timezone.now().date() From e03e6f7d35bdd985e6fa0ae4af6fe9bdb9df28ba Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 16 Aug 2024 14:57:51 -0600 Subject: [PATCH 03/59] Add permission table --- src/djangooidc/views.py | 4 + src/registrar/admin.py | 61 ++++++--- .../0119_remove_user_portfolio_and_more.py | 108 +++++++++++++++ src/registrar/models/__init__.py | 3 + src/registrar/models/user.py | 128 +++++++----------- .../models/user_portfolio_permission.py | 95 +++++++++++++ 6 files changed, 303 insertions(+), 96 deletions(-) create mode 100644 src/registrar/migrations/0119_remove_user_portfolio_and_more.py create mode 100644 src/registrar/models/user_portfolio_permission.py diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 815df4ecf..1b8e7944d 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -111,6 +111,10 @@ def login_callback(request): if not user.verification_type or is_fixture_user: user.set_user_verification_type() user.save() + + if not user.last_selected_portfolio: + user.set_default_last_selected_portfolio() + user.save() login(request, user) logger.info("Successfully logged in user %s" % user) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 423c0a01b..85adeb663 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -118,6 +118,23 @@ class FilteredSelectMultipleArrayWidget(FilteredSelectMultiple): return context +class UserPortfolioPermissionsForm(forms.ModelForm): + class Meta: + model = models.UserPortfolioPermission + fields = "__all__" + field_classes = {"username": UsernameField} + widgets = { + "portfolio_roles": FilteredSelectMultipleArrayWidget( + "portfolio_roles", is_stacked=False, choices=UserPortfolioRoleChoices.choices + ), + "portfolio_additional_permissions": FilteredSelectMultipleArrayWidget( + "portfolio_additional_permissions", + is_stacked=False, + choices=UserPortfolioPermissionChoices.choices, + ), + } + + class MyUserAdminForm(UserChangeForm): """This form utilizes the custom widget for its class's ManyToMany UIs. @@ -130,14 +147,6 @@ class MyUserAdminForm(UserChangeForm): widgets = { "groups": NoAutocompleteFilteredSelectMultiple("groups", False), "user_permissions": NoAutocompleteFilteredSelectMultiple("user_permissions", False), - "portfolio_roles": FilteredSelectMultipleArrayWidget( - "portfolio_roles", is_stacked=False, choices=UserPortfolioRoleChoices.choices - ), - "portfolio_additional_permissions": FilteredSelectMultipleArrayWidget( - "portfolio_additional_permissions", - is_stacked=False, - choices=UserPortfolioPermissionChoices.choices, - ), } def __init__(self, *args, **kwargs): @@ -709,9 +718,7 @@ class MyUserAdmin(BaseUserAdmin, ImportExportModelAdmin): "is_superuser", "groups", "user_permissions", - "portfolio", - "portfolio_roles", - "portfolio_additional_permissions", + "last_selected_portfolio", ) }, ), @@ -719,7 +726,7 @@ class MyUserAdmin(BaseUserAdmin, ImportExportModelAdmin): ) autocomplete_fields = [ - "portfolio", + "last_selected_portfolio", ] readonly_fields = ("verification_type",) @@ -741,9 +748,7 @@ class MyUserAdmin(BaseUserAdmin, ImportExportModelAdmin): "fields": ( "is_active", "groups", - "portfolio", - "portfolio_roles", - "portfolio_additional_permissions", + "last_selected_portfolio", ) }, ), @@ -798,9 +803,7 @@ class MyUserAdmin(BaseUserAdmin, ImportExportModelAdmin): "Important dates", "last_login", "date_joined", - "portfolio", - "portfolio_roles", - "portfolio_additional_permissions", + "last_selected_portfolio", ] # TODO: delete after we merge organization feature @@ -1208,6 +1211,27 @@ class UserDomainRoleResource(resources.ModelResource): class Meta: model = models.UserDomainRole +class UserPortfolioPermissionAdmin(ListHeaderAdmin): + form = UserPortfolioPermissionsForm + class Meta: + """Contains meta information about this class""" + + model = models.UserPortfolioPermission + fields = "__all__" + + _meta = Meta() + + # Columns + list_display = [ + "user", + "portfolio", + ] + + autocomplete_fields = [ + "user", + "portfolio" + ] + class UserDomainRoleAdmin(ListHeaderAdmin, ImportExportModelAdmin): """Custom user domain role admin class.""" @@ -3176,6 +3200,7 @@ admin.site.register(models.Portfolio, PortfolioAdmin) admin.site.register(models.DomainGroup, DomainGroupAdmin) admin.site.register(models.Suborganization, SuborganizationAdmin) admin.site.register(models.SeniorOfficial, SeniorOfficialAdmin) +admin.site.register(models.UserPortfolioPermission, UserPortfolioPermissionAdmin) # Register our custom waffle implementations admin.site.register(models.WaffleFlag, WaffleFlagAdmin) diff --git a/src/registrar/migrations/0119_remove_user_portfolio_and_more.py b/src/registrar/migrations/0119_remove_user_portfolio_and_more.py new file mode 100644 index 000000000..ee65a91b7 --- /dev/null +++ b/src/registrar/migrations/0119_remove_user_portfolio_and_more.py @@ -0,0 +1,108 @@ +# Generated by Django 4.2.10 on 2024-08-16 20:41 + +from django.conf import settings +import django.contrib.postgres.fields +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0118_alter_portfolio_options_alter_portfolio_creator_and_more"), + ] + + operations = [ + migrations.RemoveField( + model_name="user", + name="portfolio", + ), + migrations.RemoveField( + model_name="user", + name="portfolio_additional_permissions", + ), + migrations.RemoveField( + model_name="user", + name="portfolio_roles", + ), + migrations.AddField( + model_name="user", + name="last_selected_portfolio", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="portfolio_selected_by_users", + to="registrar.portfolio", + ), + ), + migrations.CreateModel( + name="UserPortfolioPermission", + fields=[ + ("id", models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), + ("created_at", models.DateTimeField(auto_now_add=True)), + ("updated_at", models.DateTimeField(auto_now=True)), + ( + "portfolio_roles", + django.contrib.postgres.fields.ArrayField( + base_field=models.CharField( + choices=[ + ("organization_admin", "Admin"), + ("organization_admin_read_only", "Admin read only"), + ("organization_member", "Member"), + ], + max_length=50, + ), + blank=True, + help_text="Select one or more roles.", + null=True, + size=None, + ), + ), + ( + "portfolio_additional_permissions", + django.contrib.postgres.fields.ArrayField( + base_field=models.CharField( + choices=[ + ("view_all_domains", "View all domains and domain reports"), + ("view_managed_domains", "View managed domains"), + ("view_member", "View members"), + ("edit_member", "Create and edit members"), + ("view_all_requests", "View all requests"), + ("view_created_requests", "View created requests"), + ("edit_requests", "Create and edit requests"), + ("view_portfolio", "View organization"), + ("edit_portfolio", "Edit organization"), + ("view_suborganization", "View suborganization"), + ("edit_suborganization", "Edit suborganization"), + ], + max_length=50, + ), + blank=True, + help_text="Select one or more additional permissions.", + null=True, + size=None, + ), + ), + ( + "portfolio", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="portfolio_users", + to="registrar.portfolio", + ), + ), + ( + "user", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="portfolio_permissions", + to=settings.AUTH_USER_MODEL, + ), + ), + ], + options={ + "unique_together": {("user", "portfolio")}, + }, + ), + ] diff --git a/src/registrar/models/__init__.py b/src/registrar/models/__init__.py index 1e0aad0b1..944eeafdb 100644 --- a/src/registrar/models/__init__.py +++ b/src/registrar/models/__init__.py @@ -21,6 +21,7 @@ from .portfolio import Portfolio from .domain_group import DomainGroup from .suborganization import Suborganization from .senior_official import SeniorOfficial +from .user_portfolio_permission import UserPortfolioPermission __all__ = [ @@ -46,6 +47,7 @@ __all__ = [ "DomainGroup", "Suborganization", "SeniorOfficial", + "UserPortfolioPermission", ] auditlog.register(Contact) @@ -70,3 +72,4 @@ auditlog.register(Portfolio) auditlog.register(DomainGroup) auditlog.register(Suborganization) auditlog.register(SeniorOfficial) +auditlog.register(UserPortfolioPermission) \ No newline at end of file diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 81d3b9b61..46094367b 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -5,8 +5,7 @@ from django.db import models from django.db.models import Q from django.forms import ValidationError -from registrar.models.domain_information import DomainInformation -from registrar.models.user_domain_role import UserDomainRole +from registrar.models import DomainInformation, UserDomainRole from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from .domain_invitation import DomainInvitation @@ -112,34 +111,14 @@ class User(AbstractUser): related_name="users", ) - portfolio = models.ForeignKey( + last_selected_portfolio = models.ForeignKey( "registrar.Portfolio", null=True, blank=True, - related_name="user", + related_name="portfolio_selected_by_users", on_delete=models.SET_NULL, ) - portfolio_roles = ArrayField( - models.CharField( - max_length=50, - choices=UserPortfolioRoleChoices.choices, - ), - null=True, - blank=True, - help_text="Select one or more roles.", - ) - - portfolio_additional_permissions = ArrayField( - models.CharField( - max_length=50, - choices=UserPortfolioPermissionChoices.choices, - ), - null=True, - blank=True, - help_text="Select one or more additional permissions.", - ) - phone = PhoneNumberField( null=True, blank=True, @@ -234,64 +213,17 @@ class User(AbstractUser): """Extends clean method to perform additional validation, which can raise errors in django admin.""" super().clean() - if self.portfolio is None and self._get_portfolio_permissions(): + portfolio_perms = self.portfolio_permissions.filter(portfolio=self.last_selected_portfolio).first() + if self.last_selected_portfolio is None and portfolio_perms._get_portfolio_permissions(): raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.") - if self.portfolio is not None and not self._get_portfolio_permissions(): + if self.last_selected_portfolio is not None and not portfolio_perms._get_portfolio_permissions(): raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.") - def _get_portfolio_permissions(self): - """ - Retrieve the permissions for the user's portfolio roles. - """ - portfolio_permissions = set() # Use a set to avoid duplicate permissions - - if self.portfolio_roles: - for role in self.portfolio_roles: - if role in self.PORTFOLIO_ROLE_PERMISSIONS: - portfolio_permissions.update(self.PORTFOLIO_ROLE_PERMISSIONS[role]) - if self.portfolio_additional_permissions: - portfolio_permissions.update(self.portfolio_additional_permissions) - return list(portfolio_permissions) # Convert back to list if necessary - - def _has_portfolio_permission(self, portfolio_permission): - """The views should only call this function when testing for perms and not rely on roles.""" - - if not self.portfolio: - return False - - portfolio_permissions = self._get_portfolio_permissions() - - return portfolio_permission in portfolio_permissions - - # the methods below are checks for individual portfolio permissions. They are defined here - # to make them easier to call elsewhere throughout the application - def has_base_portfolio_permission(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_PORTFOLIO) - - def has_edit_org_portfolio_permission(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_PORTFOLIO) - - def has_domains_portfolio_permission(self): - return self._has_portfolio_permission( - UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS - ) or self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) - - def has_domain_requests_portfolio_permission(self): - return self._has_portfolio_permission( - UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS - ) or self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) - - def has_view_all_domains_permission(self): - """Determines if the current user can view all available domains in a given portfolio""" - return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS) - - # Field specific permission checks - def has_view_suborganization(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION) - - def has_edit_suborganization(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION) + def set_default_last_selected_portfolio(self): + permission = self.portfolio_permissions.first() + if permission: + self.last_selected_portfolio = permission.portfolio @classmethod def needs_identity_verification(cls, email, uuid): @@ -434,6 +366,46 @@ class User(AbstractUser): def is_org_user(self, request): has_organization_feature_flag = flag_is_active(request, "organization_feature") return has_organization_feature_flag and self.has_base_portfolio_permission() + + def _has_portfolio_permission(self, portfolio_permission): + """The views should only call this function when testing for perms and not rely on roles.""" + + if not self.last_selected_portfolio: + return False + + portfolio_perms = self.portfolio_permissions.filter(portfolio=self.last_selected_portfolio).first() + if not portfolio_perms: + return False + + portfolio_permissions = portfolio_perms._get_portfolio_permissions() + return portfolio_permission in portfolio_permissions + + def has_base_portfolio_permission(self): + return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_PORTFOLIO) + + def has_edit_org_portfolio_permission(self): + return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_PORTFOLIO) + + def has_domains_portfolio_permission(self): + return self._has_portfolio_permission( + UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS + ) or self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) + + def has_domain_requests_portfolio_permission(self): + return self._has_portfolio_permission( + UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS + ) or self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) + + def has_view_all_domains_permission(self): + """Determines if the current user can view all available domains in a given portfolio""" + return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS) + + # Field specific permission checks + def has_view_suborganization(self): + return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION) + + def has_edit_suborganization(self): + return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION) 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/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py new file mode 100644 index 000000000..d88c2b4f9 --- /dev/null +++ b/src/registrar/models/user_portfolio_permission.py @@ -0,0 +1,95 @@ +from django.db import models +from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices +from .utility.time_stamped_model import TimeStampedModel +from django.contrib.postgres.fields import ArrayField + + +class UserPortfolioPermission(TimeStampedModel): + """This is a linking table that connects a user with a role on a portfolio.""" + + class Meta: + unique_together = ["user", "portfolio"] + + PORTFOLIO_ROLE_PERMISSIONS = { + UserPortfolioRoleChoices.ORGANIZATION_ADMIN: [ + UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, + UserPortfolioPermissionChoices.VIEW_MEMBER, + UserPortfolioPermissionChoices.EDIT_MEMBER, + UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, + UserPortfolioPermissionChoices.EDIT_REQUESTS, + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.EDIT_PORTFOLIO, + # Domain: field specific permissions + UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION, + UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION, + ], + UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY: [ + UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, + UserPortfolioPermissionChoices.VIEW_MEMBER, + UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + # Domain: field specific permissions + UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION, + ], + UserPortfolioRoleChoices.ORGANIZATION_MEMBER: [ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + ], + } + + user = models.ForeignKey( + "registrar.User", + null=False, + # when a portfolio is deleted, permissions are too + on_delete=models.CASCADE, + related_name="portfolio_permissions", + ) + + portfolio = models.ForeignKey( + "registrar.Portfolio", + null=False, + # when a portfolio is deleted, permissions are too + on_delete=models.CASCADE, + related_name="portfolio_users", + ) + + portfolio_roles = ArrayField( + models.CharField( + max_length=50, + choices=UserPortfolioRoleChoices.choices, + ), + null=True, + blank=True, + help_text="Select one or more roles.", + ) + + portfolio_additional_permissions = ArrayField( + models.CharField( + max_length=50, + choices=UserPortfolioPermissionChoices.choices, + ), + null=True, + blank=True, + help_text="Select one or more additional permissions.", + ) + + def __str__(self): + return ( + f"User '{self.user}' on Portfolio '{self.portfolio}' " + f"" + ) + + def _get_portfolio_permissions(self): + """ + Retrieve the permissions for the user's portfolio roles. + """ + # Use a set to avoid duplicate permissions + portfolio_permissions = set() + + if self.portfolio_roles: + for role in self.portfolio_roles: + portfolio_permissions.update(self.PORTFOLIO_ROLE_PERMISSIONS.get(role, [])) + + if self.portfolio_additional_permissions: + portfolio_permissions.update(self.portfolio_additional_permissions) + + return list(portfolio_permissions) From 1090d75f756d151714f47d3063f4ca2b5775df43 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 16 Aug 2024 15:05:30 -0600 Subject: [PATCH 04/59] Refactor fields using portfolio --- src/registrar/context_processors.py | 2 +- src/registrar/models/portfolio_invitation.py | 9 +++++---- src/registrar/models/user.py | 4 ++-- src/registrar/registrar_middleware.py | 4 ++-- src/registrar/views/domain.py | 8 ++++---- src/registrar/views/portfolios.py | 4 ++-- 6 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index ee5f8aee1..ca7c92892 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -73,7 +73,7 @@ def portfolio_permissions(request): "has_base_portfolio_permission": request.user.has_base_portfolio_permission(), "has_domains_portfolio_permission": request.user.has_domains_portfolio_permission(), "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission(), - "portfolio": request.user.portfolio, + "portfolio": request.user.last_selected_portfolio, "has_organization_feature_flag": True, } except AttributeError: diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index 2ad780429..c9666f6cb 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -87,9 +87,10 @@ class PortfolioInvitation(TimeStampedModel): raise RuntimeError("Cannot find the user to retrieve this portfolio invitation.") # and create a role for that user on this portfolio - user.portfolio = self.portfolio + user_portfolio = user.last_selected_portfolio + user_portfolio = self.portfolio if self.portfolio_roles and len(self.portfolio_roles) > 0: - user.portfolio_roles = self.portfolio_roles + user_portfolio.portfolio_roles = self.portfolio_roles if self.portfolio_additional_permissions and len(self.portfolio_additional_permissions) > 0: - user.portfolio_additional_permissions = self.portfolio_additional_permissions - user.save() + user_portfolio.portfolio_additional_permissions = self.portfolio_additional_permissions + user_portfolio.save() diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 46094367b..5ed8a7844 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -338,7 +338,7 @@ class User(AbstractUser): for invitation in PortfolioInvitation.objects.filter( email__iexact=self.email, status=PortfolioInvitation.PortfolioInvitationStatus.INVITED ): - if self.portfolio is None: + if self.last_selected_portfolio is None: try: invitation.retrieve() invitation.save() @@ -410,6 +410,6 @@ class User(AbstractUser): def get_user_domain_ids(self, request): """Returns either the domains ids associated with this user on UserDomainRole or Portfolio""" if self.is_org_user(request) and self.has_view_all_domains_permission(): - return DomainInformation.objects.filter(portfolio=self.portfolio).values_list("domain_id", flat=True) + return DomainInformation.objects.filter(portfolio=self.last_selected_portfolio).values_list("domain_id", flat=True) else: return UserDomainRole.objects.filter(user=self).values_list("domain_id", flat=True) diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index 2af331bc9..6e7d110fb 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -143,10 +143,10 @@ class CheckPortfolioMiddleware: if current_path == self.home and request.user.is_authenticated and request.user.is_org_user(request): if request.user.has_base_portfolio_permission(): - portfolio = request.user.portfolio + portfolio = request.user.last_selected_portfolio # Add the portfolio to the request object - request.portfolio = portfolio + request.last_selected_portfolio = portfolio if request.user.has_domains_portfolio_permission(): portfolio_redirect = reverse("domains") diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 72f2fd27e..bd3392c69 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -177,7 +177,7 @@ class DomainView(DomainBaseView): if self.request.user.has_domains_portfolio_permission(): if Domain.objects.filter(id=pk).exists(): domain = Domain.objects.get(id=pk) - if domain.domain_info.portfolio == self.request.user.portfolio: + if domain.domain_info.portfolio == self.request.user.last_selected_portfolio: return True return False @@ -236,7 +236,7 @@ class DomainOrgNameAddressView(DomainFormBaseView): # Org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - if self.request.user.portfolio and is_org_user: + if self.request.user.last_selected_portfolio and is_org_user: return False else: return super().has_permission() @@ -255,7 +255,7 @@ class DomainSubOrganizationView(DomainFormBaseView): # non-org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - if self.request.user.portfolio and is_org_user: + if self.request.user.last_selected_portfolio and is_org_user: return super().has_permission() else: return False @@ -335,7 +335,7 @@ class DomainSeniorOfficialView(DomainFormBaseView): # Org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - if self.request.user.portfolio and is_org_user: + if self.request.user.last_selected_portfolio and is_org_user: return False else: return super().has_permission() diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 8a5321cc9..81c104790 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -56,7 +56,7 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): def get_object(self, queryset=None): """Get the portfolio object based on the request user.""" - portfolio = self.request.user.portfolio + portfolio = self.request.user.last_selected_portfolio if portfolio is None: raise Http404("No organization found for this user") return portfolio @@ -112,7 +112,7 @@ class PortfolioSeniorOfficialView(PortfolioBasePermissionView, FormMixin): def get_object(self, queryset=None): """Get the portfolio object based on the request user.""" - portfolio = self.request.user.portfolio + portfolio = self.request.user.last_selected_portfolio if portfolio is None: raise Http404("No organization found for this user") return portfolio From 3e024f4ed08030d1447b71ec5778dbb5ff262cd4 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 16 Aug 2024 15:17:42 -0600 Subject: [PATCH 05/59] Update user.py --- src/registrar/models/user.py | 80 ++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 40 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 5ed8a7844..2f173548b 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -220,6 +220,46 @@ class User(AbstractUser): if self.last_selected_portfolio is not None and not portfolio_perms._get_portfolio_permissions(): raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.") + def _has_portfolio_permission(self, portfolio_permission): + """The views should only call this function when testing for perms and not rely on roles.""" + + if not self.last_selected_portfolio: + return False + + portfolio_perms = self.portfolio_permissions.filter(portfolio=self.last_selected_portfolio).first() + if not portfolio_perms: + return False + + portfolio_permissions = portfolio_perms._get_portfolio_permissions() + return portfolio_permission in portfolio_permissions + + def has_base_portfolio_permission(self): + return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_PORTFOLIO) + + def has_edit_org_portfolio_permission(self): + return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_PORTFOLIO) + + def has_domains_portfolio_permission(self): + return self._has_portfolio_permission( + UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS + ) or self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) + + def has_domain_requests_portfolio_permission(self): + return self._has_portfolio_permission( + UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS + ) or self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) + + def has_view_all_domains_permission(self): + """Determines if the current user can view all available domains in a given portfolio""" + return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS) + + # Field specific permission checks + def has_view_suborganization(self): + return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION) + + def has_edit_suborganization(self): + return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION) + def set_default_last_selected_portfolio(self): permission = self.portfolio_permissions.first() if permission: @@ -366,46 +406,6 @@ class User(AbstractUser): def is_org_user(self, request): has_organization_feature_flag = flag_is_active(request, "organization_feature") return has_organization_feature_flag and self.has_base_portfolio_permission() - - def _has_portfolio_permission(self, portfolio_permission): - """The views should only call this function when testing for perms and not rely on roles.""" - - if not self.last_selected_portfolio: - return False - - portfolio_perms = self.portfolio_permissions.filter(portfolio=self.last_selected_portfolio).first() - if not portfolio_perms: - return False - - portfolio_permissions = portfolio_perms._get_portfolio_permissions() - return portfolio_permission in portfolio_permissions - - def has_base_portfolio_permission(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_PORTFOLIO) - - def has_edit_org_portfolio_permission(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_PORTFOLIO) - - def has_domains_portfolio_permission(self): - return self._has_portfolio_permission( - UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS - ) or self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) - - def has_domain_requests_portfolio_permission(self): - return self._has_portfolio_permission( - UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS - ) or self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) - - def has_view_all_domains_permission(self): - """Determines if the current user can view all available domains in a given portfolio""" - return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS) - - # Field specific permission checks - def has_view_suborganization(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION) - - def has_edit_suborganization(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION) def get_user_domain_ids(self, request): """Returns either the domains ids associated with this user on UserDomainRole or Portfolio""" From 78a5774af2cc14f67df8bdf826a2d2c3f4fdece7 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 19 Aug 2024 09:24:11 -0600 Subject: [PATCH 06/59] Change invitation logic --- src/registrar/models/portfolio_invitation.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index c9666f6cb..a76f0f9f9 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -6,6 +6,8 @@ from django.contrib.auth import get_user_model from django.db import models from django_fsm import FSMField, transition + +from registrar.models.user_portfolio_permission import UserPortfolioPermission from .utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices # type: ignore from .utility.time_stamped_model import TimeStampedModel @@ -87,10 +89,9 @@ class PortfolioInvitation(TimeStampedModel): raise RuntimeError("Cannot find the user to retrieve this portfolio invitation.") # and create a role for that user on this portfolio - user_portfolio = user.last_selected_portfolio - user_portfolio = self.portfolio + user_portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=self.portfolio, user=user) if self.portfolio_roles and len(self.portfolio_roles) > 0: - user_portfolio.portfolio_roles = self.portfolio_roles + user_portfolio_permission.portfolio_roles = self.portfolio_roles if self.portfolio_additional_permissions and len(self.portfolio_additional_permissions) > 0: - user_portfolio.portfolio_additional_permissions = self.portfolio_additional_permissions - user_portfolio.save() + user_portfolio_permission.portfolio_additional_permissions = self.portfolio_additional_permissions + user_portfolio_permission.save() From 28a7d7bee213e7eba808edc39a64df513cb7d254 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 19 Aug 2024 10:28:13 -0600 Subject: [PATCH 07/59] invitation logic pt 2 --- src/registrar/models/portfolio_invitation.py | 10 ++++++++++ src/registrar/models/user.py | 4 ++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index a76f0f9f9..68ce69fe3 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -6,6 +6,7 @@ from django.contrib.auth import get_user_model from django.db import models from django_fsm import FSMField, transition +from waffle import flag_is_active from registrar.models.user_portfolio_permission import UserPortfolioPermission from .utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices # type: ignore @@ -95,3 +96,12 @@ class PortfolioInvitation(TimeStampedModel): if self.portfolio_additional_permissions and len(self.portfolio_additional_permissions) > 0: user_portfolio_permission.portfolio_additional_permissions = self.portfolio_additional_permissions user_portfolio_permission.save() + + # The multiple_portfolios flag ensures that many portfolios can exist per user. + # This is (or will be) exposed as a multiselect, which they can choose from. + # Without it, we should just add the user on invitation as they'd have no way to select it. + # For now, the caller should ensure that this only occurs when no portfolio is selected. + # NOTE: Model enforcement of this occurs elsewhere. + if not flag_is_active(None, "multiple_portfolios"): + user.last_selected_portfolio = user_portfolio_permission.portfolio + user.save() diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 2f173548b..29e0bc90d 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -14,7 +14,6 @@ from .transition_domain import TransitionDomain from .verified_by_staff import VerifiedByStaff from .domain import Domain from .domain_request import DomainRequest -from django.contrib.postgres.fields import ArrayField from waffle.decorators import flag_is_active from phonenumber_field.modelfields import PhoneNumberField # type: ignore @@ -378,7 +377,8 @@ class User(AbstractUser): for invitation in PortfolioInvitation.objects.filter( email__iexact=self.email, status=PortfolioInvitation.PortfolioInvitationStatus.INVITED ): - if self.last_selected_portfolio is None: + only_single_portfolio = not flag_is_active(None, "multiple_portfolios") and self.last_selected_portfolio is None + if only_single_portfolio or flag_is_active(None, "multiple_portfolios"): try: invitation.retrieve() invitation.save() From 289099dd92cf4b1d8937d98bcffd2eddcfadc5ce Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 19 Aug 2024 10:47:13 -0600 Subject: [PATCH 08/59] Clean logic --- src/registrar/models/user.py | 8 ++++---- src/registrar/models/user_portfolio_permission.py | 11 +++++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 29e0bc90d..21e7e196f 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -212,11 +212,11 @@ class User(AbstractUser): """Extends clean method to perform additional validation, which can raise errors in django admin.""" super().clean() - portfolio_perms = self.portfolio_permissions.filter(portfolio=self.last_selected_portfolio).first() - if self.last_selected_portfolio is None and portfolio_perms._get_portfolio_permissions(): + portfolio_perm = self.portfolio_permissions.filter(portfolio=self.last_selected_portfolio, user=self).first() + if self.last_selected_portfolio is None and portfolio_perm._get_portfolio_permissions(): raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.") - if self.last_selected_portfolio is not None and not portfolio_perms._get_portfolio_permissions(): + if self.last_selected_portfolio is not None and not portfolio_perm._get_portfolio_permissions(): raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.") def _has_portfolio_permission(self, portfolio_permission): @@ -225,7 +225,7 @@ class User(AbstractUser): if not self.last_selected_portfolio: return False - portfolio_perms = self.portfolio_permissions.filter(portfolio=self.last_selected_portfolio).first() + portfolio_perms = self.portfolio_permissions.filter(portfolio=self.last_selected_portfolio, user=self).first() if not portfolio_perms: return False diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index d88c2b4f9..44d1199fc 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -1,4 +1,6 @@ from django.db import models +from django.forms import ValidationError +from waffle import flag_is_active from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from .utility.time_stamped_model import TimeStampedModel from django.contrib.postgres.fields import ArrayField @@ -93,3 +95,12 @@ class UserPortfolioPermission(TimeStampedModel): portfolio_permissions.update(self.portfolio_additional_permissions) return list(portfolio_permissions) + + def clean(self): + """Extends clean method to perform additional validation, which can raise errors in django admin.""" + super().clean() + + if not flag_is_active(None, "multiple_portfolios") and self.pk is None: + existing_permissions = UserPortfolioPermission.objects.filter(user=self.user) + if existing_permissions.exists(): + raise ValidationError("Only one portfolio permission is allowed per user when multiple portfolios are disabled.") From 565a9299f4712cd4c9a976eef8474e8af7107d72 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 19 Aug 2024 11:20:14 -0600 Subject: [PATCH 09/59] Update views.py --- src/djangooidc/views.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index 1b8e7944d..c4d4cec81 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -9,6 +9,8 @@ from django.http import HttpResponseRedirect from django.shortcuts import redirect from urllib.parse import parse_qs, urlencode +from waffle import flag_is_active + from djangooidc.oidc import Client from djangooidc import exceptions as o_e from registrar.models import User @@ -112,9 +114,12 @@ def login_callback(request): user.set_user_verification_type() user.save() - if not user.last_selected_portfolio: + if not flag_is_active(request, "multiple_portfolios"): user.set_default_last_selected_portfolio() user.save() + else: + user.last_selected_portfolio = None + user.save() login(request, user) logger.info("Successfully logged in user %s" % user) From 733eee6fcce85b874f7cd0f049efef7eac97dad4 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 19 Aug 2024 11:25:47 -0600 Subject: [PATCH 10/59] Update portfolio_invitation.py --- src/registrar/models/portfolio_invitation.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index 68ce69fe3..04c571298 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -96,12 +96,3 @@ class PortfolioInvitation(TimeStampedModel): if self.portfolio_additional_permissions and len(self.portfolio_additional_permissions) > 0: user_portfolio_permission.portfolio_additional_permissions = self.portfolio_additional_permissions user_portfolio_permission.save() - - # The multiple_portfolios flag ensures that many portfolios can exist per user. - # This is (or will be) exposed as a multiselect, which they can choose from. - # Without it, we should just add the user on invitation as they'd have no way to select it. - # For now, the caller should ensure that this only occurs when no portfolio is selected. - # NOTE: Model enforcement of this occurs elsewhere. - if not flag_is_active(None, "multiple_portfolios"): - user.last_selected_portfolio = user_portfolio_permission.portfolio - user.save() From f587a994f232c2e7fbafc9fe932fd06535923ec5 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 19 Aug 2024 12:26:14 -0600 Subject: [PATCH 11/59] cleanup --- src/registrar/admin.py | 40 +++++++++---------- src/registrar/models/__init__.py | 2 +- src/registrar/models/portfolio_invitation.py | 9 ++--- src/registrar/models/user.py | 8 +++- .../models/user_portfolio_permission.py | 11 +++-- 5 files changed, 36 insertions(+), 34 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 85adeb663..c91bbde48 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -118,23 +118,6 @@ class FilteredSelectMultipleArrayWidget(FilteredSelectMultiple): return context -class UserPortfolioPermissionsForm(forms.ModelForm): - class Meta: - model = models.UserPortfolioPermission - fields = "__all__" - field_classes = {"username": UsernameField} - widgets = { - "portfolio_roles": FilteredSelectMultipleArrayWidget( - "portfolio_roles", is_stacked=False, choices=UserPortfolioRoleChoices.choices - ), - "portfolio_additional_permissions": FilteredSelectMultipleArrayWidget( - "portfolio_additional_permissions", - is_stacked=False, - choices=UserPortfolioPermissionChoices.choices, - ), - } - - class MyUserAdminForm(UserChangeForm): """This form utilizes the custom widget for its class's ManyToMany UIs. @@ -178,6 +161,22 @@ class MyUserAdminForm(UserChangeForm): ) +class UserPortfolioPermissionsForm(forms.ModelForm): + class Meta: + model = models.UserPortfolioPermission + fields = "__all__" + widgets = { + "portfolio_roles": FilteredSelectMultipleArrayWidget( + "portfolio_roles", is_stacked=False, choices=UserPortfolioRoleChoices.choices + ), + "portfolio_additional_permissions": FilteredSelectMultipleArrayWidget( + "portfolio_additional_permissions", + is_stacked=False, + choices=UserPortfolioPermissionChoices.choices, + ), + } + + class PortfolioInvitationAdminForm(UserChangeForm): """This form utilizes the custom widget for its class's ManyToMany UIs.""" @@ -1211,8 +1210,10 @@ class UserDomainRoleResource(resources.ModelResource): class Meta: model = models.UserDomainRole + class UserPortfolioPermissionAdmin(ListHeaderAdmin): form = UserPortfolioPermissionsForm + class Meta: """Contains meta information about this class""" @@ -1227,10 +1228,7 @@ class UserPortfolioPermissionAdmin(ListHeaderAdmin): "portfolio", ] - autocomplete_fields = [ - "user", - "portfolio" - ] + autocomplete_fields = ["user", "portfolio"] class UserDomainRoleAdmin(ListHeaderAdmin, ImportExportModelAdmin): diff --git a/src/registrar/models/__init__.py b/src/registrar/models/__init__.py index 944eeafdb..c1023cafe 100644 --- a/src/registrar/models/__init__.py +++ b/src/registrar/models/__init__.py @@ -72,4 +72,4 @@ auditlog.register(Portfolio) auditlog.register(DomainGroup) auditlog.register(Suborganization) auditlog.register(SeniorOfficial) -auditlog.register(UserPortfolioPermission) \ No newline at end of file +auditlog.register(UserPortfolioPermission) diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index 04c571298..de7bda893 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -1,16 +1,11 @@ """People are invited by email to administer domains.""" import logging - from django.contrib.auth import get_user_model from django.db import models - from django_fsm import FSMField, transition -from waffle import flag_is_active - from registrar.models.user_portfolio_permission import UserPortfolioPermission from .utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices # type: ignore - from .utility.time_stamped_model import TimeStampedModel from django.contrib.postgres.fields import ArrayField @@ -90,7 +85,9 @@ class PortfolioInvitation(TimeStampedModel): raise RuntimeError("Cannot find the user to retrieve this portfolio invitation.") # and create a role for that user on this portfolio - user_portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=self.portfolio, user=user) + user_portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=self.portfolio, user=user + ) if self.portfolio_roles and len(self.portfolio_roles) > 0: user_portfolio_permission.portfolio_roles = self.portfolio_roles if self.portfolio_additional_permissions and len(self.portfolio_additional_permissions) > 0: diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 21e7e196f..084b1926a 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -377,7 +377,9 @@ class User(AbstractUser): for invitation in PortfolioInvitation.objects.filter( email__iexact=self.email, status=PortfolioInvitation.PortfolioInvitationStatus.INVITED ): - only_single_portfolio = not flag_is_active(None, "multiple_portfolios") and self.last_selected_portfolio is None + only_single_portfolio = ( + not flag_is_active(None, "multiple_portfolios") and self.last_selected_portfolio is None + ) if only_single_portfolio or flag_is_active(None, "multiple_portfolios"): try: invitation.retrieve() @@ -410,6 +412,8 @@ class User(AbstractUser): def get_user_domain_ids(self, request): """Returns either the domains ids associated with this user on UserDomainRole or Portfolio""" if self.is_org_user(request) and self.has_view_all_domains_permission(): - return DomainInformation.objects.filter(portfolio=self.last_selected_portfolio).values_list("domain_id", flat=True) + return DomainInformation.objects.filter(portfolio=self.last_selected_portfolio).values_list( + "domain_id", flat=True + ) else: return UserDomainRole.objects.filter(user=self).values_list("domain_id", flat=True) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index 44d1199fc..77639f569 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -76,8 +76,9 @@ class UserPortfolioPermission(TimeStampedModel): def __str__(self): return ( - f"User '{self.user}' on Portfolio '{self.portfolio}' " - f"" + f"User '{self.user}' on Portfolio '{self.portfolio}' " f"" + if self.portfolio_roles + else "" ) def _get_portfolio_permissions(self): @@ -95,7 +96,7 @@ class UserPortfolioPermission(TimeStampedModel): portfolio_permissions.update(self.portfolio_additional_permissions) return list(portfolio_permissions) - + def clean(self): """Extends clean method to perform additional validation, which can raise errors in django admin.""" super().clean() @@ -103,4 +104,6 @@ class UserPortfolioPermission(TimeStampedModel): if not flag_is_active(None, "multiple_portfolios") and self.pk is None: existing_permissions = UserPortfolioPermission.objects.filter(user=self.user) if existing_permissions.exists(): - raise ValidationError("Only one portfolio permission is allowed per user when multiple portfolios are disabled.") + raise ValidationError( + "Only one portfolio permission is allowed per user when multiple portfolios are disabled." + ) From 432ee9c860f1348cbc53c35309b1516ee0b8d077 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 19 Aug 2024 15:18:54 -0600 Subject: [PATCH 12/59] pt. 1 of test refactors --- src/registrar/admin.py | 8 +- .../0119_remove_user_portfolio_and_more.py | 6 +- src/registrar/models/portfolio_invitation.py | 4 +- src/registrar/models/user.py | 11 --- .../models/user_portfolio_permission.py | 22 +++-- src/registrar/tests/common.py | 6 +- src/registrar/tests/test_admin.py | 1 + src/registrar/tests/test_models.py | 48 ++++++---- src/registrar/tests/test_reports.py | 17 ++-- src/registrar/tests/test_views_domain.py | 20 ++--- src/registrar/tests/test_views_portfolio.py | 89 ++++++------------- 11 files changed, 100 insertions(+), 132 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index c91bbde48..fd555520d 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -166,11 +166,11 @@ class UserPortfolioPermissionsForm(forms.ModelForm): model = models.UserPortfolioPermission fields = "__all__" widgets = { - "portfolio_roles": FilteredSelectMultipleArrayWidget( - "portfolio_roles", is_stacked=False, choices=UserPortfolioRoleChoices.choices + "roles": FilteredSelectMultipleArrayWidget( + "roles", is_stacked=False, choices=UserPortfolioRoleChoices.choices ), - "portfolio_additional_permissions": FilteredSelectMultipleArrayWidget( - "portfolio_additional_permissions", + "additional_permissions": FilteredSelectMultipleArrayWidget( + "additional_permissions", is_stacked=False, choices=UserPortfolioPermissionChoices.choices, ), diff --git a/src/registrar/migrations/0119_remove_user_portfolio_and_more.py b/src/registrar/migrations/0119_remove_user_portfolio_and_more.py index ee65a91b7..7c9d2defc 100644 --- a/src/registrar/migrations/0119_remove_user_portfolio_and_more.py +++ b/src/registrar/migrations/0119_remove_user_portfolio_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.10 on 2024-08-16 20:41 +# Generated by Django 4.2.10 on 2024-08-19 20:24 from django.conf import settings import django.contrib.postgres.fields @@ -43,7 +43,7 @@ class Migration(migrations.Migration): ("created_at", models.DateTimeField(auto_now_add=True)), ("updated_at", models.DateTimeField(auto_now=True)), ( - "portfolio_roles", + "roles", django.contrib.postgres.fields.ArrayField( base_field=models.CharField( choices=[ @@ -60,7 +60,7 @@ class Migration(migrations.Migration): ), ), ( - "portfolio_additional_permissions", + "additional_permissions", django.contrib.postgres.fields.ArrayField( base_field=models.CharField( choices=[ diff --git a/src/registrar/models/portfolio_invitation.py b/src/registrar/models/portfolio_invitation.py index de7bda893..46d7bf124 100644 --- a/src/registrar/models/portfolio_invitation.py +++ b/src/registrar/models/portfolio_invitation.py @@ -89,7 +89,7 @@ class PortfolioInvitation(TimeStampedModel): portfolio=self.portfolio, user=user ) if self.portfolio_roles and len(self.portfolio_roles) > 0: - user_portfolio_permission.portfolio_roles = self.portfolio_roles + user_portfolio_permission.roles = self.portfolio_roles if self.portfolio_additional_permissions and len(self.portfolio_additional_permissions) > 0: - user_portfolio_permission.portfolio_additional_permissions = self.portfolio_additional_permissions + user_portfolio_permission.additional_permissions = self.portfolio_additional_permissions user_portfolio_permission.save() diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 084b1926a..34f2f8bcf 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -208,17 +208,6 @@ class User(AbstractUser): def has_contact_info(self): return bool(self.title or self.email or self.phone) - def clean(self): - """Extends clean method to perform additional validation, which can raise errors in django admin.""" - super().clean() - - portfolio_perm = self.portfolio_permissions.filter(portfolio=self.last_selected_portfolio, user=self).first() - if self.last_selected_portfolio is None and portfolio_perm._get_portfolio_permissions(): - raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.") - - if self.last_selected_portfolio is not None and not portfolio_perm._get_portfolio_permissions(): - raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.") - def _has_portfolio_permission(self, portfolio_permission): """The views should only call this function when testing for perms and not rely on roles.""" diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index 77639f569..e00bf37f9 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -54,7 +54,7 @@ class UserPortfolioPermission(TimeStampedModel): related_name="portfolio_users", ) - portfolio_roles = ArrayField( + roles = ArrayField( models.CharField( max_length=50, choices=UserPortfolioRoleChoices.choices, @@ -64,7 +64,7 @@ class UserPortfolioPermission(TimeStampedModel): help_text="Select one or more roles.", ) - portfolio_additional_permissions = ArrayField( + additional_permissions = ArrayField( models.CharField( max_length=50, choices=UserPortfolioPermissionChoices.choices, @@ -76,8 +76,8 @@ class UserPortfolioPermission(TimeStampedModel): def __str__(self): return ( - f"User '{self.user}' on Portfolio '{self.portfolio}' " f"" - if self.portfolio_roles + f"User '{self.user}' on Portfolio '{self.portfolio}' " f"" + if self.roles else "" ) @@ -88,12 +88,12 @@ class UserPortfolioPermission(TimeStampedModel): # Use a set to avoid duplicate permissions portfolio_permissions = set() - if self.portfolio_roles: - for role in self.portfolio_roles: + if self.roles: + for role in self.roles: portfolio_permissions.update(self.PORTFOLIO_ROLE_PERMISSIONS.get(role, [])) - if self.portfolio_additional_permissions: - portfolio_permissions.update(self.portfolio_additional_permissions) + if self.additional_permissions: + portfolio_permissions.update(self.additional_permissions) return list(portfolio_permissions) @@ -107,3 +107,9 @@ class UserPortfolioPermission(TimeStampedModel): raise ValidationError( "Only one portfolio permission is allowed per user when multiple portfolios are disabled." ) + + if self.portfolio is None and self._get_portfolio_permissions(): + raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.") + + if self.portfolio is not None and not self._get_portfolio_permissions(): + raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.") diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index ceb3b6e92..f3fd6709f 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -906,7 +906,7 @@ def completed_domain_request( # noqa federal_agency=None, federal_type=None, action_needed_reason=None, - portfolio=None, + last_selected_portfolio=None, ): """A completed domain request.""" if not user: @@ -977,8 +977,8 @@ def completed_domain_request( # noqa if action_needed_reason: domain_request_kwargs["action_needed_reason"] = action_needed_reason - if portfolio: - domain_request_kwargs["portfolio"] = portfolio + if last_selected_portfolio: + domain_request_kwargs["last_selected_portfolio"] = last_selected_portfolio domain_request, _ = DomainRequest.objects.get_or_create(**domain_request_kwargs) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 827742ef1..49d2b4802 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1209,6 +1209,7 @@ class TestMyUserAdmin(MockDbForSharedTests): domain_deleted.delete() role.delete() + # TODO - should refactor def test_analyst_cannot_see_selects_for_portfolio_role_and_permissions_in_user_form(self): """Can only test for the presence of a base element. The multiselects and the h2->h3 conversion are all dynamically generated.""" diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index f4e998fff..1c95257d3 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -17,6 +17,7 @@ from registrar.models import ( DomainInvitation, UserDomainRole, FederalAgency, + UserPortfolioPermission, ) import boto3_mocking @@ -1151,10 +1152,11 @@ class TestPortfolioInvitations(TestCase): self.assertFalse(self.user.portfolio) self.invitation.retrieve() self.user.refresh_from_db() - self.assertEqual(self.user.portfolio.organization_name, "Hotel California") - self.assertEqual(self.user.portfolio_roles, [self.portfolio_role_base, self.portfolio_role_admin]) + self.assertEqual(self.user.last_selected_portfolio.organization_name, "Hotel California") + portfolio_role, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio) + self.assertEqual(portfolio_role.roles, [self.portfolio_role_base, self.portfolio_role_admin]) self.assertEqual( - self.user.portfolio_additional_permissions, [self.portfolio_permission_1, self.portfolio_permission_2] + portfolio_role.additional_permissions, [self.portfolio_permission_1, self.portfolio_permission_2] ) self.assertEqual(self.invitation.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) @@ -1167,10 +1169,11 @@ class TestPortfolioInvitations(TestCase): @less_console_noise_decorator def test_retrieve_user_already_member_error(self): - self.assertFalse(self.user.portfolio) + self.assertFalse(self.user.last_selected_portfolio) portfolio2, _ = Portfolio.objects.get_or_create(creator=self.user2, organization_name="Tokyo Hotel") - self.user.portfolio = portfolio2 - self.assertEqual(self.user.portfolio.organization_name, "Tokyo Hotel") + portfolio_role, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio2) + self.assertEqual(self.user.last_selected_portfolio.organization_name, "Tokyo Hotel") + self.assertEqual(portfolio_role.portfolio, portfolio2) self.user.save() self.user.check_portfolio_invitations_on_login() self.user.refresh_from_db() @@ -1197,6 +1200,7 @@ class TestUser(TestCase): DomainRequest.objects.all().delete() DraftDomain.objects.all().delete() TransitionDomain.objects.all().delete() + UserPortfolioPermission.objects.all().delete() Portfolio.objects.all().delete() User.objects.all().delete() UserDomainRole.objects.all().delete() @@ -1360,8 +1364,7 @@ class TestUser(TestCase): Note: This tests _get_portfolio_permissions as a side effect """ portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") - - self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS] + self.user.last_selected_portfolio = portfolio self.user.save() self.user.refresh_from_db() @@ -1371,8 +1374,10 @@ class TestUser(TestCase): self.assertFalse(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) - self.user.portfolio = portfolio - self.user.save() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) + portfolio_permission.additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS] + portfolio_permission.save() + portfolio_permission.refresh_from_db() self.user.refresh_from_db() user_can_view_all_domains = self.user.has_domains_portfolio_permission() @@ -1381,8 +1386,10 @@ class TestUser(TestCase): self.assertTrue(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) - self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - self.user.save() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) + portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + portfolio_permission.save() + portfolio_permission.refresh_from_db() self.user.refresh_from_db() user_can_view_all_domains = self.user.has_domains_portfolio_permission() @@ -1407,13 +1414,15 @@ class TestUser(TestCase): def test_user_with_portfolio_but_no_roles(self): # Create an instance of User with a portfolio but no roles or additional permissions portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) - self.user.portfolio = portfolio - self.user.portfolio_roles = [] + # Try to remove the role + portfolio_permission.portfolio = portfolio + portfolio_permission.roles = [] # Test if the ValidationError is raised with the correct message with self.assertRaises(ValidationError) as cm: - self.user.clean() + portfolio_permission.clean() self.assertEqual( cm.exception.message, "When portfolio is assigned, portfolio roles or additional permissions are required." @@ -1426,9 +1435,16 @@ class TestUser(TestCase): self.user.portfolio = None self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) + + # Try to remove the portfolio + portfolio_permission.portfolio = None + portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + # Test if the ValidationError is raised with the correct message with self.assertRaises(ValidationError) as cm: - self.user.clean() + portfolio_permission.clean() self.assertEqual( cm.exception.message, "When portfolio roles or additional permissions are assigned, portfolio is required." diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index 52aaa8c38..8440e54e1 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -7,6 +7,7 @@ from registrar.models import ( UserDomainRole, ) from registrar.models import Portfolio +from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.utility.csv_export import ( DomainDataFull, @@ -321,8 +322,7 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): # Create a portfolio and assign it to the user portfolio = Portfolio.objects.create(creator=self.user, organization_name="Test Portfolio") - self.user.portfolio = portfolio - self.user.save() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) UserDomainRole.objects.create(user=self.user, domain=self.domain_2) UserDomainRole.objects.filter(user=self.user, domain=self.domain_1).delete() @@ -336,8 +336,9 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): self.domain_3.domain_info.save() # Set up user permissions - self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - self.user.save() + portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + portfolio_permission.save() + portfolio_permission.refresh_from_db() self.user.refresh_from_db() # Create a request object @@ -354,16 +355,16 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): self.assertNotIn(self.domain_2.name, csv_content) # Test the output for readonly admin - self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY] - self.user.save() + portfolio_permission.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY] + portfolio_permission.save() self.assertIn(self.domain_1.name, csv_content) self.assertIn(self.domain_3.name, csv_content) self.assertNotIn(self.domain_2.name, csv_content) # Get the csv content - self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] - self.user.save() + portfolio_permission.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + portfolio_permission.save() csv_content = self._run_domain_data_type_user_export(request) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 3a90543a2..f3f4d1051 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -36,6 +36,7 @@ from registrar.models import ( FederalAgency, Portfolio, Suborganization, + UserPortfolioPermission, ) from datetime import date, datetime, timedelta from django.utils import timezone @@ -328,9 +329,9 @@ class TestDomainDetail(TestDomainOverview): email="bogus@example.gov", phone="8003111234", title="test title", - portfolio=portfolio, - portfolio_roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + last_selected_portfolio=portfolio, ) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) domain, _ = Domain.objects.get_or_create(name="bogusdomain.gov") DomainInformation.objects.get_or_create(creator=user, domain=domain, portfolio=portfolio) self.client.force_login(user) @@ -1477,10 +1478,7 @@ class TestDomainSuborganization(TestDomainOverview): self.domain_information.refresh_from_db() # Add portfolio perms to the user object - self.user.portfolio = portfolio - self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - self.user.save() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) self.assertEqual(self.domain_information.sub_organization, suborg) @@ -1536,10 +1534,7 @@ class TestDomainSuborganization(TestDomainOverview): self.domain_information.refresh_from_db() # Add portfolio perms to the user object - self.user.portfolio = portfolio - self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY] - self.user.save() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY]) self.assertEqual(self.domain_information.sub_organization, suborg) @@ -1577,10 +1572,7 @@ class TestDomainSuborganization(TestDomainOverview): self.domain_information.refresh_from_db() # Add portfolio perms to the user object - self.user.portfolio = portfolio - self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] - self.user.save() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.VIEW_PORTFOLIO]) # Navigate to the domain overview page page = self.app.get(reverse("domain", kwargs={"pk": self.domain.id})) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 60764cf1c..cb72f29fe 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -10,6 +10,7 @@ from registrar.models import ( UserDomainRole, User, ) +from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from .common import create_test_user from waffle.testutils import override_flag @@ -52,10 +53,7 @@ class TestPortfolio(WebTest): self.portfolio.save() self.portfolio.refresh_from_db() - self.user.portfolio = self.portfolio - self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] - self.user.save() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) so_portfolio_page = self.app.get(reverse("senior-official")) # Assert that we're on the right page @@ -72,6 +70,7 @@ class TestPortfolio(WebTest): def test_middleware_does_not_redirect_if_no_permission(self): """Test that user with no portfolio permission is not redirected when attempting to access home""" self.app.set_user(self.user.username) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) self.user.portfolio = self.portfolio self.user.save() self.user.refresh_from_db() @@ -86,9 +85,7 @@ class TestPortfolio(WebTest): def test_middleware_does_not_redirect_if_no_portfolio(self): """Test that user with no assigned portfolio is not redirected when attempting to access home""" self.app.set_user(self.user.username) - self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] - self.user.save() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -100,10 +97,7 @@ class TestPortfolio(WebTest): def test_middleware_redirects_to_portfolio_organization_page(self): """Test that user with a portfolio and VIEW_PORTFOLIO is redirected to portfolio organization page""" self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] - self.user.save() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -117,13 +111,7 @@ class TestPortfolio(WebTest): """Test that user with a portfolio, VIEW_PORTFOLIO, VIEW_ALL_DOMAINS is redirected to portfolio domains page""" self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.portfolio_additional_permissions = [ - UserPortfolioPermissionChoices.VIEW_PORTFOLIO, - UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, - ] - self.user.save() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -137,9 +125,7 @@ class TestPortfolio(WebTest): def test_portfolio_domains_page_403_when_user_not_have_permission(self): """Test that user without proper permission is denied access to portfolio domain view""" self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.save() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -151,9 +137,7 @@ class TestPortfolio(WebTest): def test_portfolio_domain_requests_page_403_when_user_not_have_permission(self): """Test that user without proper permission is denied access to portfolio domain view""" self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.save() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -165,9 +149,7 @@ class TestPortfolio(WebTest): def test_portfolio_organization_page_403_when_user_not_have_permission(self): """Test that user without proper permission is not allowed access to portfolio organization page""" self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.save() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -179,12 +161,9 @@ class TestPortfolio(WebTest): def test_portfolio_organization_page_read_only(self): """Test that user with a portfolio can access the portfolio organization page, read only""" self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) self.portfolio.city = "Los Angeles" - self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] self.portfolio.save() - self.user.save() - self.user.refresh_from_db() with override_flag("organization_feature", active=True): response = self.app.get(reverse("organization")) # Assert the response is a 200 @@ -200,15 +179,9 @@ class TestPortfolio(WebTest): def test_portfolio_organization_page_edit_access(self): """Test that user with a portfolio can access the portfolio organization page, read only""" self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.portfolio_additional_permissions = [ - UserPortfolioPermissionChoices.VIEW_PORTFOLIO, - UserPortfolioPermissionChoices.EDIT_PORTFOLIO, - ] + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO]) self.portfolio.city = "Los Angeles" self.portfolio.save() - self.user.save() - self.user.refresh_from_db() with override_flag("organization_feature", active=True): response = self.app.get(reverse("organization")) # Assert the response is a 200 @@ -224,14 +197,12 @@ class TestPortfolio(WebTest): def test_navigation_links_hidden_when_user_not_have_permission(self): """Test that navigation links are hidden when user does not have portfolio permissions""" self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.portfolio_additional_permissions = [ + portfolio_additional_permissions = [ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, ] - self.user.save() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -245,7 +216,8 @@ class TestPortfolio(WebTest): # removing non-basic portfolio perms, which should remove domains # and domain requests from nav - self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] + portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) self.user.save() self.user.refresh_from_db() @@ -261,10 +233,8 @@ class TestPortfolio(WebTest): def test_navigation_links_hidden_when_user_not_have_role(self): """Test that admin / memmber roles are associated with the right access""" self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - self.user.save() - self.user.refresh_from_db() + portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, roles=portfolio_roles) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -278,9 +248,9 @@ class TestPortfolio(WebTest): # removing non-basic portfolio role, which should remove domains # and domain requests from nav - self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] - self.user.save() - self.user.refresh_from_db() + portfolio_permission.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + portfolio_permission.save() + portfolio_permission.refresh_from_db() portfolio_page = self.app.get(reverse("home")).follow() @@ -295,14 +265,11 @@ class TestPortfolio(WebTest): """Can load portfolio's org name page.""" with override_flag("organization_feature", active=True): self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.portfolio_additional_permissions = [ + portfolio_additional_permissions = [ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] - self.user.save() - self.user.refresh_from_db() - + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) page = self.app.get(reverse("organization")) self.assertContains( page, "The name of your federal agency will be publicly listed as the domain registrant." @@ -313,13 +280,11 @@ class TestPortfolio(WebTest): """Org name and address information appears on the page.""" with override_flag("organization_feature", active=True): self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.portfolio_additional_permissions = [ + portfolio_additional_permissions = [ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] - self.user.save() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) self.portfolio.organization_name = "Hotel California" self.portfolio.save() @@ -333,13 +298,11 @@ class TestPortfolio(WebTest): """Submitting changes works on the org name address page.""" with override_flag("organization_feature", active=True): self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.portfolio_additional_permissions = [ + portfolio_additional_permissions = [ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] - self.user.save() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) self.portfolio.address_line1 = "1600 Penn Ave" self.portfolio.save() From df9f3dcce57032153aea29906249170c1c40d70a Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 20 Aug 2024 13:48:16 -0500 Subject: [PATCH 13/59] fix some tests --- src/registrar/fixtures_domain_requests.py | 2 +- src/registrar/models/domain_request.py | 2 +- .../already_has_domains.txt | 2 +- .../emails/action_needed_reasons/bad_name.txt | 2 +- .../eligibility_unclear.txt | 2 +- .../questionable_senior_official.txt | 2 +- .../emails/domain_request_withdrawn.txt | 2 +- .../emails/status_change_approved.txt | 2 +- .../emails/status_change_rejected.txt | 2 +- .../emails/submission_confirmation.txt | 2 +- .../includes/domain_requests_table.html | 2 +- src/registrar/tests/common.py | 6 +-- src/registrar/tests/test_admin_request.py | 26 +++++----- src/registrar/tests/test_reports.py | 2 +- .../tests/test_views_requests_json.py | 48 +++++++++---------- src/registrar/utility/csv_export.py | 6 +-- src/registrar/views/domain_requests_json.py | 2 +- src/registrar/views/report_views.py | 6 +-- 18 files changed, 60 insertions(+), 58 deletions(-) diff --git a/src/registrar/fixtures_domain_requests.py b/src/registrar/fixtures_domain_requests.py index 50f611474..a5ec3fc74 100644 --- a/src/registrar/fixtures_domain_requests.py +++ b/src/registrar/fixtures_domain_requests.py @@ -95,7 +95,7 @@ class DomainRequestFixture: # TODO for a future ticket: Allow for more than just "federal" here da.generic_org_type = app["generic_org_type"] if "generic_org_type" in app else "federal" - da.submission_date = fake.date() + da.last_submitted_date = fake.date() da.federal_type = ( app["federal_type"] if "federal_type" in app diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index e9711f0ae..77ebf88f8 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -821,7 +821,7 @@ class DomainRequest(TimeStampedModel): if not self.first_submitted_date: self.first_submitted_date = timezone.now().date() - # Update last_submission_date to today + # Update last_submitted_date to today self.last_submitted_date = timezone.now().date() self.save() diff --git a/src/registrar/templates/emails/action_needed_reasons/already_has_domains.txt b/src/registrar/templates/emails/action_needed_reasons/already_has_domains.txt index b1b3b0a1c..2e3012c91 100644 --- a/src/registrar/templates/emails/action_needed_reasons/already_has_domains.txt +++ b/src/registrar/templates/emails/action_needed_reasons/already_has_domains.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. We've identified an action that you’ll need to complete before we continue reviewing your .gov domain request. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Action needed ---------------------------------------------------------------- diff --git a/src/registrar/templates/emails/action_needed_reasons/bad_name.txt b/src/registrar/templates/emails/action_needed_reasons/bad_name.txt index 7d088aa4e..9481a1e63 100644 --- a/src/registrar/templates/emails/action_needed_reasons/bad_name.txt +++ b/src/registrar/templates/emails/action_needed_reasons/bad_name.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. We've identified an action that you’ll need to complete before we continue reviewing your .gov domain request. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Action needed ---------------------------------------------------------------- diff --git a/src/registrar/templates/emails/action_needed_reasons/eligibility_unclear.txt b/src/registrar/templates/emails/action_needed_reasons/eligibility_unclear.txt index d3a986183..705805998 100644 --- a/src/registrar/templates/emails/action_needed_reasons/eligibility_unclear.txt +++ b/src/registrar/templates/emails/action_needed_reasons/eligibility_unclear.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. We've identified an action that you’ll need to complete before we continue reviewing your .gov domain request. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Action needed ---------------------------------------------------------------- diff --git a/src/registrar/templates/emails/action_needed_reasons/questionable_senior_official.txt b/src/registrar/templates/emails/action_needed_reasons/questionable_senior_official.txt index e20e4cb60..5967d7089 100644 --- a/src/registrar/templates/emails/action_needed_reasons/questionable_senior_official.txt +++ b/src/registrar/templates/emails/action_needed_reasons/questionable_senior_official.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. We've identified an action that you’ll need to complete before we continue reviewing your .gov domain request. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Action needed ---------------------------------------------------------------- diff --git a/src/registrar/templates/emails/domain_request_withdrawn.txt b/src/registrar/templates/emails/domain_request_withdrawn.txt index 6efa92d64..0db00feea 100644 --- a/src/registrar/templates/emails/domain_request_withdrawn.txt +++ b/src/registrar/templates/emails/domain_request_withdrawn.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. Your .gov domain request has been withdrawn and will not be reviewed by our team. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Withdrawn ---------------------------------------------------------------- diff --git a/src/registrar/templates/emails/status_change_approved.txt b/src/registrar/templates/emails/status_change_approved.txt index 70f813599..66f8f8b6c 100644 --- a/src/registrar/templates/emails/status_change_approved.txt +++ b/src/registrar/templates/emails/status_change_approved.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. Congratulations! Your .gov domain request has been approved. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Approved You can manage your approved domain on the .gov registrar . diff --git a/src/registrar/templates/emails/status_change_rejected.txt b/src/registrar/templates/emails/status_change_rejected.txt index 2fcbb1d83..4e5250162 100644 --- a/src/registrar/templates/emails/status_change_rejected.txt +++ b/src/registrar/templates/emails/status_change_rejected.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. Your .gov domain request has been rejected. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Rejected ---------------------------------------------------------------- diff --git a/src/registrar/templates/emails/submission_confirmation.txt b/src/registrar/templates/emails/submission_confirmation.txt index 740e6f393..c8ff4c7eb 100644 --- a/src/registrar/templates/emails/submission_confirmation.txt +++ b/src/registrar/templates/emails/submission_confirmation.txt @@ -4,7 +4,7 @@ Hi, {{ recipient.first_name }}. We received your .gov domain request. DOMAIN REQUESTED: {{ domain_request.requested_domain.name }} -REQUEST RECEIVED ON: {{ domain_request.submission_date|date }} +REQUEST RECEIVED ON: {{ domain_request.last_submitted_date|date }} STATUS: Submitted ---------------------------------------------------------------- diff --git a/src/registrar/templates/includes/domain_requests_table.html b/src/registrar/templates/includes/domain_requests_table.html index 487c1cee5..bd909350c 100644 --- a/src/registrar/templates/includes/domain_requests_table.html +++ b/src/registrar/templates/includes/domain_requests_table.html @@ -45,7 +45,7 @@ Domain name - Date submitted + Date submitted Status Action diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index a4c3e2ef4..85d1c56e7 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -775,13 +775,13 @@ class MockDb(TestCase): cls.domain_request_3.alternative_domains.add(website, website_2) cls.domain_request_3.current_websites.add(website_3, website_4) cls.domain_request_3.cisa_representative_email = "test@igorville.com" - cls.domain_request_3.submission_date = get_time_aware_date(datetime(2024, 4, 2)) + cls.domain_request_3.last_submitted_date = get_time_aware_date(datetime(2024, 4, 2)) cls.domain_request_3.save() - cls.domain_request_4.submission_date = get_time_aware_date(datetime(2024, 4, 2)) + cls.domain_request_4.last_submitted_date = get_time_aware_date(datetime(2024, 4, 2)) cls.domain_request_4.save() - cls.domain_request_6.submission_date = get_time_aware_date(datetime(2024, 4, 2)) + cls.domain_request_6.last_submitted_date = get_time_aware_date(datetime(2024, 4, 2)) cls.domain_request_6.save() @classmethod diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index c4fc8bcee..d81f73156 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -422,7 +422,7 @@ class TestDomainRequestAdmin(MockEppLib): # Assert that our sort works correctly self.test_helper.assert_table_sorted( - "11", + "13", ( "submitter__first_name", "submitter__last_name", @@ -431,7 +431,7 @@ class TestDomainRequestAdmin(MockEppLib): # Assert that sorting in reverse works correctly self.test_helper.assert_table_sorted( - "-11", + "-13", ( "-submitter__first_name", "-submitter__last_name", @@ -454,7 +454,7 @@ class TestDomainRequestAdmin(MockEppLib): # Assert that our sort works correctly self.test_helper.assert_table_sorted( - "12", + "14", ( "investigator__first_name", "investigator__last_name", @@ -463,7 +463,7 @@ class TestDomainRequestAdmin(MockEppLib): # Assert that sorting in reverse works correctly self.test_helper.assert_table_sorted( - "-12", + "-14", ( "-investigator__first_name", "-investigator__last_name", @@ -476,7 +476,7 @@ class TestDomainRequestAdmin(MockEppLib): @less_console_noise_decorator def test_default_sorting_in_domain_requests_list(self): """ - Make sure the default sortin in on the domain requests list page is reverse submission_date + Make sure the default sortin in on the domain requests list page is reverse last_submitted_date then alphabetical requested_domain """ @@ -486,12 +486,12 @@ class TestDomainRequestAdmin(MockEppLib): for name in ["ccc.gov", "bbb.gov", "eee.gov", "aaa.gov", "zzz.gov", "ddd.gov"] ] - domain_requests[0].submission_date = timezone.make_aware(datetime(2024, 10, 16)) - domain_requests[1].submission_date = timezone.make_aware(datetime(2001, 10, 16)) - domain_requests[2].submission_date = timezone.make_aware(datetime(1980, 10, 16)) - domain_requests[3].submission_date = timezone.make_aware(datetime(1998, 10, 16)) - domain_requests[4].submission_date = timezone.make_aware(datetime(2013, 10, 16)) - domain_requests[5].submission_date = timezone.make_aware(datetime(1980, 10, 16)) + domain_requests[0].last_submitted_date = timezone.make_aware(datetime(2024, 10, 16)) + domain_requests[1].last_submitted_date = timezone.make_aware(datetime(2001, 10, 16)) + domain_requests[2].last_submitted_date = timezone.make_aware(datetime(1980, 10, 16)) + domain_requests[3].last_submitted_date = timezone.make_aware(datetime(1998, 10, 16)) + domain_requests[4].last_submitted_date = timezone.make_aware(datetime(2013, 10, 16)) + domain_requests[5].last_submitted_date = timezone.make_aware(datetime(1980, 10, 16)) # Save the modified domain requests to update their attributes in the database for domain_request in domain_requests: @@ -1556,7 +1556,9 @@ class TestDomainRequestAdmin(MockEppLib): "cisa_representative_last_name", "has_cisa_representative", "is_policy_acknowledged", - "submission_date", + "first_submitted_date", + "last_submitted_date", + "last_status_update", "notes", "alternative_domains", ] diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index 52aaa8c38..4b58d6d22 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -762,7 +762,7 @@ class HelperFunctions(MockDbForSharedTests): with less_console_noise(): filter_condition = { "status": DomainRequest.DomainRequestStatus.SUBMITTED, - "submission_date__lte": self.end_date, + "last_submitted_date__lte": self.end_date, } submitted_requests_sliced_at_end_date = DomainRequestExport.get_sliced_requests(filter_condition) expected_content = [3, 2, 0, 0, 0, 0, 1, 0, 0, 1] diff --git a/src/registrar/tests/test_views_requests_json.py b/src/registrar/tests/test_views_requests_json.py index 7bdc922cf..c140b7e44 100644 --- a/src/registrar/tests/test_views_requests_json.py +++ b/src/registrar/tests/test_views_requests_json.py @@ -25,91 +25,91 @@ class GetRequestsJsonTest(TestWithUser, WebTest): DomainRequest.objects.create( creator=cls.user, requested_domain=lamb_chops, - submission_date="2024-01-01", + last_submitted_date="2024-01-01", status=DomainRequest.DomainRequestStatus.STARTED, created_at="2024-01-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=short_ribs, - submission_date="2024-02-01", + last_submitted_date="2024-02-01", status=DomainRequest.DomainRequestStatus.WITHDRAWN, created_at="2024-02-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=beef_chuck, - submission_date="2024-03-01", + last_submitted_date="2024-03-01", status=DomainRequest.DomainRequestStatus.REJECTED, created_at="2024-03-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=stew_beef, - submission_date="2024-04-01", + last_submitted_date="2024-04-01", status=DomainRequest.DomainRequestStatus.STARTED, created_at="2024-04-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-05-01", + last_submitted_date="2024-05-01", status=DomainRequest.DomainRequestStatus.STARTED, created_at="2024-05-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-06-01", + last_submitted_date="2024-06-01", status=DomainRequest.DomainRequestStatus.WITHDRAWN, created_at="2024-06-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-07-01", + last_submitted_date="2024-07-01", status=DomainRequest.DomainRequestStatus.REJECTED, created_at="2024-07-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-08-01", + last_submitted_date="2024-08-01", status=DomainRequest.DomainRequestStatus.STARTED, created_at="2024-08-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-09-01", + last_submitted_date="2024-09-01", status=DomainRequest.DomainRequestStatus.STARTED, created_at="2024-09-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-10-01", + last_submitted_date="2024-10-01", status=DomainRequest.DomainRequestStatus.WITHDRAWN, created_at="2024-10-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-11-01", + last_submitted_date="2024-11-01", status=DomainRequest.DomainRequestStatus.REJECTED, created_at="2024-11-01", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-11-02", + last_submitted_date="2024-11-02", status=DomainRequest.DomainRequestStatus.WITHDRAWN, created_at="2024-11-02", ), DomainRequest.objects.create( creator=cls.user, requested_domain=None, - submission_date="2024-12-01", + last_submitted_date="2024-12-01", status=DomainRequest.DomainRequestStatus.APPROVED, created_at="2024-12-01", ), @@ -138,7 +138,7 @@ class GetRequestsJsonTest(TestWithUser, WebTest): # Extract fields from response requested_domains = [request["requested_domain"] for request in data["domain_requests"]] - submission_dates = [request["submission_date"] for request in data["domain_requests"]] + last_submitted_dates = [request["last_submitted_date"] for request in data["domain_requests"]] statuses = [request["status"] for request in data["domain_requests"]] created_ats = [request["created_at"] for request in data["domain_requests"]] ids = [request["id"] for request in data["domain_requests"]] @@ -154,7 +154,7 @@ class GetRequestsJsonTest(TestWithUser, WebTest): self.domain_requests[i].requested_domain.name if self.domain_requests[i].requested_domain else None, requested_domains[i], ) - self.assertEqual(self.domain_requests[i].submission_date, submission_dates[i]) + self.assertEqual(self.domain_requests[i].last_submitted_date, last_submitted_dates[i]) self.assertEqual(self.domain_requests[i].get_status_display(), statuses[i]) self.assertEqual( parse_datetime(self.domain_requests[i].created_at.isoformat()), parse_datetime(created_ats[i]) @@ -287,26 +287,26 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_sorting(self): """test that sorting works properly on the result set""" - response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "submission_date", "order": "desc"}) + response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json - # Check if sorted by submission_date in descending order - submission_dates = [req["submission_date"] for req in data["domain_requests"]] - self.assertEqual(submission_dates, sorted(submission_dates, reverse=True)) + # Check if sorted by last_submitted_date in descending order + last_submitted_dates = [req["last_submitted_date"] for req in data["domain_requests"]] + self.assertEqual(last_submitted_dates, sorted(last_submitted_dates, reverse=True)) - response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "submission_date", "order": "asc"}) + response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "asc"}) self.assertEqual(response.status_code, 200) data = response.json - # Check if sorted by submission_date in ascending order - submission_dates = [req["submission_date"] for req in data["domain_requests"]] - self.assertEqual(submission_dates, sorted(submission_dates)) + # Check if sorted by last_submitted_date in ascending order + last_submitted_dates = [req["last_submitted_date"] for req in data["domain_requests"]] + self.assertEqual(last_submitted_dates, sorted(last_submitted_dates)) def test_filter_approved_excluded(self): """test that approved requests are excluded from result set.""" # sort in reverse chronological order of submission date, since most recent request is approved - response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "submission_date", "order": "desc"}) + response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index db961a36d..6f8510418 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -1235,7 +1235,7 @@ class DomainRequestExport(BaseExport): "State/territory": model.get("state_territory"), "Request purpose": model.get("purpose"), "CISA regional representative": model.get("cisa_representative_email"), - "Submitted at": model.get("submission_date"), + "Submitted at": model.get("last_submitted_date"), } row = [FIELDS.get(column, "") for column in columns] @@ -1279,8 +1279,8 @@ class DomainRequestGrowth(DomainRequestExport): end_date_formatted = format_end_date(end_date) return Q( status=DomainRequest.DomainRequestStatus.SUBMITTED, - submission_date__lte=end_date_formatted, - submission_date__gte=start_date_formatted, + last_submitted_date__lte=end_date_formatted, + last_submitted_date__gte=start_date_formatted, ) @classmethod diff --git a/src/registrar/views/domain_requests_json.py b/src/registrar/views/domain_requests_json.py index 2e58c8e48..6b0b346f8 100644 --- a/src/registrar/views/domain_requests_json.py +++ b/src/registrar/views/domain_requests_json.py @@ -46,7 +46,7 @@ def get_domain_requests_json(request): domain_requests_data = [ { "requested_domain": domain_request.requested_domain.name if domain_request.requested_domain else None, - "submission_date": domain_request.submission_date, + "last_submitted_date": domain_request.last_submitted_date, "status": domain_request.get_status_display(), "created_at": format(domain_request.created_at, "c"), # Serialize to ISO 8601 "id": domain_request.id, diff --git a/src/registrar/views/report_views.py b/src/registrar/views/report_views.py index 428298b52..abdbd37c9 100644 --- a/src/registrar/views/report_views.py +++ b/src/registrar/views/report_views.py @@ -26,7 +26,7 @@ class AnalyticsView(View): created_at__gt=thirty_days_ago, status=models.DomainRequest.DomainRequestStatus.APPROVED ) avg_approval_time = last_30_days_approved_applications.annotate( - approval_time=F("approved_domain__created_at") - F("submission_date") + approval_time=F("approved_domain__created_at") - F("last_submitted_date") ).aggregate(Avg("approval_time"))["approval_time__avg"] # Format the timedelta to display only days if avg_approval_time is not None: @@ -104,11 +104,11 @@ class AnalyticsView(View): filter_submitted_requests_start_date = { "status": models.DomainRequest.DomainRequestStatus.SUBMITTED, - "submission_date__lte": start_date_formatted, + "last_submitted_date__lte": start_date_formatted, } filter_submitted_requests_end_date = { "status": models.DomainRequest.DomainRequestStatus.SUBMITTED, - "submission_date__lte": end_date_formatted, + "last_submitted_date__lte": end_date_formatted, } submitted_requests_sliced_at_start_date = csv_export.DomainRequestExport.get_sliced_requests( filter_submitted_requests_start_date From 3c89557976742e4faa6c434f9b4db8d18470a8c4 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 20 Aug 2024 15:29:28 -0400 Subject: [PATCH 14/59] wip --- src/djangooidc/views.py | 9 --- src/registrar/admin.py | 7 --- src/registrar/context_processors.py | 29 +++++---- .../0119_remove_user_portfolio_and_more.py | 11 ---- src/registrar/models/user.py | 60 ++++++++----------- src/registrar/registrar_middleware.py | 31 ++++++---- src/registrar/templates/domain_detail.html | 2 +- src/registrar/templates/domain_sidebar.html | 2 +- .../templates/includes/domains_table.html | 2 +- src/registrar/views/domain.py | 8 +-- src/registrar/views/portfolios.py | 16 +++-- src/registrar/views/utility/mixins.py | 6 +- 12 files changed, 80 insertions(+), 103 deletions(-) diff --git a/src/djangooidc/views.py b/src/djangooidc/views.py index c4d4cec81..815df4ecf 100644 --- a/src/djangooidc/views.py +++ b/src/djangooidc/views.py @@ -9,8 +9,6 @@ from django.http import HttpResponseRedirect from django.shortcuts import redirect from urllib.parse import parse_qs, urlencode -from waffle import flag_is_active - from djangooidc.oidc import Client from djangooidc import exceptions as o_e from registrar.models import User @@ -113,13 +111,6 @@ def login_callback(request): if not user.verification_type or is_fixture_user: user.set_user_verification_type() user.save() - - if not flag_is_active(request, "multiple_portfolios"): - user.set_default_last_selected_portfolio() - user.save() - else: - user.last_selected_portfolio = None - user.save() login(request, user) logger.info("Successfully logged in user %s" % user) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index fd555520d..a41035f71 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -717,17 +717,12 @@ class MyUserAdmin(BaseUserAdmin, ImportExportModelAdmin): "is_superuser", "groups", "user_permissions", - "last_selected_portfolio", ) }, ), ("Important dates", {"fields": ("last_login", "date_joined")}), ) - autocomplete_fields = [ - "last_selected_portfolio", - ] - readonly_fields = ("verification_type",) analyst_fieldsets = ( @@ -747,7 +742,6 @@ class MyUserAdmin(BaseUserAdmin, ImportExportModelAdmin): "fields": ( "is_active", "groups", - "last_selected_portfolio", ) }, ), @@ -802,7 +796,6 @@ class MyUserAdmin(BaseUserAdmin, ImportExportModelAdmin): "Important dates", "last_login", "date_joined", - "last_selected_portfolio", ] # TODO: delete after we merge organization feature diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index ca7c92892..7d28260ed 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -61,27 +61,34 @@ def add_has_profile_feature_flag_to_context(request): def portfolio_permissions(request): """Make portfolio permissions for the request user available in global context""" try: - if not request.user or not request.user.is_authenticated or not flag_is_active(request, "organization_feature"): + if request.session["portfolio"] is not None: return { - "has_base_portfolio_permission": False, - "has_domains_portfolio_permission": False, - "has_domain_requests_portfolio_permission": False, - "portfolio": None, - "has_organization_feature_flag": False, + "has_base_portfolio_permission": request.user.has_base_portfolio_permission(request.session["portfolio"]), + "has_domains_portfolio_permission": request.user.has_domains_portfolio_permission(request.session["portfolio"]), + "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission(request.session["portfolio"]), + "has_view_suborganization": request.user.has_view_suborganization(request.session["portfolio"]), + "has_edit_suborganization": request.user.has_edit_suborganization(request.session["portfolio"]), + "portfolio": request.session["portfolio"], + "has_organization_feature_flag": True, } return { - "has_base_portfolio_permission": request.user.has_base_portfolio_permission(), - "has_domains_portfolio_permission": request.user.has_domains_portfolio_permission(), - "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission(), - "portfolio": request.user.last_selected_portfolio, - "has_organization_feature_flag": True, + "has_base_portfolio_permission": False, + "has_domains_portfolio_permission": False, + "has_domain_requests_portfolio_permission": False, + "has_view_suborganization": False, + "has_edit_suborganization": False, + "portfolio": None, + "has_organization_feature_flag": False, } + except AttributeError: # Handles cases where request.user might not exist return { "has_base_portfolio_permission": False, "has_domains_portfolio_permission": False, "has_domain_requests_portfolio_permission": False, + "has_view_suborganization": False, + "has_edit_suborganization": False, "portfolio": None, "has_organization_feature_flag": False, } diff --git a/src/registrar/migrations/0119_remove_user_portfolio_and_more.py b/src/registrar/migrations/0119_remove_user_portfolio_and_more.py index 7c9d2defc..84ed45cd1 100644 --- a/src/registrar/migrations/0119_remove_user_portfolio_and_more.py +++ b/src/registrar/migrations/0119_remove_user_portfolio_and_more.py @@ -25,17 +25,6 @@ class Migration(migrations.Migration): model_name="user", name="portfolio_roles", ), - migrations.AddField( - model_name="user", - name="last_selected_portfolio", - field=models.ForeignKey( - blank=True, - null=True, - on_delete=django.db.models.deletion.SET_NULL, - related_name="portfolio_selected_by_users", - to="registrar.portfolio", - ), - ), migrations.CreateModel( name="UserPortfolioPermission", fields=[ diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 34f2f8bcf..b151f3800 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -110,14 +110,6 @@ class User(AbstractUser): related_name="users", ) - last_selected_portfolio = models.ForeignKey( - "registrar.Portfolio", - null=True, - blank=True, - related_name="portfolio_selected_by_users", - on_delete=models.SET_NULL, - ) - phone = PhoneNumberField( null=True, blank=True, @@ -208,50 +200,48 @@ class User(AbstractUser): def has_contact_info(self): return bool(self.title or self.email or self.phone) - def _has_portfolio_permission(self, portfolio_permission): + def _has_portfolio_permission(self, portfolio, portfolio_permission): """The views should only call this function when testing for perms and not rely on roles.""" - if not self.last_selected_portfolio: - return False - - portfolio_perms = self.portfolio_permissions.filter(portfolio=self.last_selected_portfolio, user=self).first() + portfolio_perms = self.portfolio_permissions.filter(portfolio=portfolio, user=self).first() if not portfolio_perms: return False portfolio_permissions = portfolio_perms._get_portfolio_permissions() return portfolio_permission in portfolio_permissions - def has_base_portfolio_permission(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_PORTFOLIO) + def has_base_portfolio_permission(self, portfolio): + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_PORTFOLIO) - def has_edit_org_portfolio_permission(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_PORTFOLIO) + def has_edit_org_portfolio_permission(self, portfolio): + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_PORTFOLIO) - def has_domains_portfolio_permission(self): - return self._has_portfolio_permission( + def has_domains_portfolio_permission(self, portfolio): + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS - ) or self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) + ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) - def has_domain_requests_portfolio_permission(self): - return self._has_portfolio_permission( + def has_domain_requests_portfolio_permission(self, portfolio): + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS - ) or self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) + ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) - def has_view_all_domains_permission(self): + def has_view_all_domains_permission(self, portfolio): """Determines if the current user can view all available domains in a given portfolio""" - return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS) + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS) # Field specific permission checks - def has_view_suborganization(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION) + def has_view_suborganization(self, portfolio): + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_SUBORGANIZATION) - def has_edit_suborganization(self): - return self._has_portfolio_permission(UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION) + def has_edit_suborganization(self, portfolio): + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_SUBORGANIZATION) - def set_default_last_selected_portfolio(self): + def get_first_portfolio(self): permission = self.portfolio_permissions.first() if permission: - self.last_selected_portfolio = permission.portfolio + return permission.portfolio + return None @classmethod def needs_identity_verification(cls, email, uuid): @@ -367,7 +357,7 @@ class User(AbstractUser): email__iexact=self.email, status=PortfolioInvitation.PortfolioInvitationStatus.INVITED ): only_single_portfolio = ( - not flag_is_active(None, "multiple_portfolios") and self.last_selected_portfolio is None + not flag_is_active(None, "multiple_portfolios") and self.get_first_portfolio() is None ) if only_single_portfolio or flag_is_active(None, "multiple_portfolios"): try: @@ -396,12 +386,12 @@ class User(AbstractUser): def is_org_user(self, request): has_organization_feature_flag = flag_is_active(request, "organization_feature") - return has_organization_feature_flag and self.has_base_portfolio_permission() + return has_organization_feature_flag and self.has_base_portfolio_permission(request.session["portfolio"]) def get_user_domain_ids(self, request): """Returns either the domains ids associated with this user on UserDomainRole or Portfolio""" - if self.is_org_user(request) and self.has_view_all_domains_permission(): - return DomainInformation.objects.filter(portfolio=self.last_selected_portfolio).values_list( + if self.is_org_user(request) and self.has_view_all_domains_permission(request.session["portfolio"]): + return DomainInformation.objects.filter(portfolio=request.session["portfolio"]).values_list( "domain_id", flat=True ) else: diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index 6e7d110fb..1da74a527 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -125,8 +125,9 @@ class CheckUserProfileMiddleware: class CheckPortfolioMiddleware: """ - Checks if the current user has a portfolio - If they do, redirect them to the portfolio homepage when they navigate to home. + this middleware should serve two purposes: + 1 - set the portfolio in session if appropriate # views will need the session portfolio + 2 - if path is home and session portfolio is set, redirect based on permissions of user """ def __init__(self, get_response): @@ -140,20 +141,28 @@ class CheckPortfolioMiddleware: def process_view(self, request, view_func, view_args, view_kwargs): current_path = request.path - if current_path == self.home and request.user.is_authenticated and request.user.is_org_user(request): + if not request.user.is_authenticated: + return None + + # set the portfolio in the session if it is not set + if not "portfolio" in request.session or request.session["portfolio"] is None: + # if user is a multiple portfolio + if flag_is_active(request, "multiple_portfolios"): + # NOTE: we will want to change later to have a workflow for selecting + # portfolio and another for switching portfolio; for now, select first + request.session["portfolio"] = request.user.get_first_portfolio() + elif flag_is_active(request, "organization_feature"): + request.session["portfolio"] = request.user.get_first_portfolio() + else: + request.session["portfolio"] = None - if request.user.has_base_portfolio_permission(): - portfolio = request.user.last_selected_portfolio - - # Add the portfolio to the request object - request.last_selected_portfolio = portfolio - - if request.user.has_domains_portfolio_permission(): + if request.session["portfolio"] is not None and current_path == self.home: + if request.user.has_base_portfolio_permission(request.session["portfolio"]): + if request.user.has_domains_portfolio_permission(request.session["portfolio"]): portfolio_redirect = reverse("domains") else: # View organization is the lowest access portfolio_redirect = reverse("organization") - return HttpResponseRedirect(portfolio_redirect) return None diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index 19f196e40..cf55f7070 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -72,7 +72,7 @@ {% include "includes/summary_item.html" with title='DNSSEC' value='Not Enabled' edit_link=url editable=is_editable %} {% endif %} - {% if portfolio and has_domains_portfolio_permission and request.user.has_view_suborganization %} + {% if portfolio and has_domains_portfolio_permission and has_view_suborganization %} {% 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:request.user.has_edit_suborganization %} {% else %} diff --git a/src/registrar/templates/domain_sidebar.html b/src/registrar/templates/domain_sidebar.html index f2e9f190c..24f92bf16 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_domains_portfolio_permission and request.user.has_view_suborganization %} + {% if has_domains_portfolio_permission and has_view_suborganization %} {% with url_name="domain-suborganization" %} {% include "includes/domain_sidenav_item.html" with item_text="Suborganization" %} {% endwith %} diff --git a/src/registrar/templates/includes/domains_table.html b/src/registrar/templates/includes/domains_table.html index 73331c3f0..f790da4d5 100644 --- a/src/registrar/templates/includes/domains_table.html +++ b/src/registrar/templates/includes/domains_table.html @@ -157,7 +157,7 @@ Domain name Expires Status - {% if has_domains_portfolio_permission and request.user.has_view_suborganization %} + {% if has_domains_portfolio_permission and has_view_suborganization %} Suborganization {% endif %} Date: Tue, 20 Aug 2024 14:17:14 -0600 Subject: [PATCH 15/59] Fix edge cases --- src/registrar/context_processors.py | 15 ++++++++------- src/registrar/templates/domain_detail.html | 2 +- .../templates/domain_suborganization.html | 2 +- src/registrar/views/domain.py | 2 +- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index 7d28260ed..16d592982 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -61,14 +61,15 @@ def add_has_profile_feature_flag_to_context(request): def portfolio_permissions(request): """Make portfolio permissions for the request user available in global context""" try: - if request.session["portfolio"] is not None: + portfolio = request.session["portfolio"] if "portfolio" in request.session else None + if portfolio: return { - "has_base_portfolio_permission": request.user.has_base_portfolio_permission(request.session["portfolio"]), - "has_domains_portfolio_permission": request.user.has_domains_portfolio_permission(request.session["portfolio"]), - "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission(request.session["portfolio"]), - "has_view_suborganization": request.user.has_view_suborganization(request.session["portfolio"]), - "has_edit_suborganization": request.user.has_edit_suborganization(request.session["portfolio"]), - "portfolio": request.session["portfolio"], + "has_base_portfolio_permission": request.user.has_base_portfolio_permission(portfolio), + "has_domains_portfolio_permission": request.user.has_domains_portfolio_permission(portfolio), + "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission(portfolio), + "has_view_suborganization": request.user.has_view_suborganization(portfolio), + "has_edit_suborganization": request.user.has_edit_suborganization(portfolio), + "portfolio": portfolio, "has_organization_feature_flag": True, } return { diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index cf55f7070..d7bc277b3 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -74,7 +74,7 @@ {% if portfolio and has_domains_portfolio_permission and has_view_suborganization %} {% 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:request.user.has_edit_suborganization %} + {% include "includes/summary_item.html" with title='Suborganization' value=domain.domain_info.sub_organization edit_link=url editable=is_editable|and:has_edit_suborganization %} {% else %} {% url 'domain-org-name-address' pk=domain.id as url %} {% include "includes/summary_item.html" with title='Organization name and mailing address' value=domain.domain_info address='true' edit_link=url editable=is_editable %} diff --git a/src/registrar/templates/domain_suborganization.html b/src/registrar/templates/domain_suborganization.html index 42bb766a3..ad96f1d65 100644 --- a/src/registrar/templates/domain_suborganization.html +++ b/src/registrar/templates/domain_suborganization.html @@ -15,7 +15,7 @@ If you believe there is an error please contact help@get.gov.

- {% if has_domains_portfolio_permission and request.user.has_edit_suborganization %} + {% if has_domains_portfolio_permission and has_edit_suborganization %}
{% csrf_token %} {% input_with_errors form.sub_organization %} diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index d6ffbf64a..aac9d056d 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -174,7 +174,7 @@ class DomainView(DomainBaseView): """Most views should not allow permission to portfolio users. If particular views allow permissions, they will need to override this function.""" - if self.request.user.has_domains_portfolio_permission(): + if self.request.user.has_domains_portfolio_permission(self.request.session["portfolio"]): if Domain.objects.filter(id=pk).exists(): domain = Domain.objects.get(id=pk) if domain.domain_info.portfolio == self.request.session["portfolio"]: From 5c8ad8cd34fc9cd7677edabd10d00032b6c1697d Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 20 Aug 2024 14:30:04 -0600 Subject: [PATCH 16/59] Fix incorrect param name for completed domain request --- src/registrar/tests/common.py | 6 +++--- src/registrar/tests/test_admin.py | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index f3fd6709f..ceb3b6e92 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -906,7 +906,7 @@ def completed_domain_request( # noqa federal_agency=None, federal_type=None, action_needed_reason=None, - last_selected_portfolio=None, + portfolio=None, ): """A completed domain request.""" if not user: @@ -977,8 +977,8 @@ def completed_domain_request( # noqa if action_needed_reason: domain_request_kwargs["action_needed_reason"] = action_needed_reason - if last_selected_portfolio: - domain_request_kwargs["last_selected_portfolio"] = last_selected_portfolio + if portfolio: + domain_request_kwargs["portfolio"] = portfolio domain_request, _ = DomainRequest.objects.get_or_create(**domain_request_kwargs) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 49d2b4802..144605b87 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -41,6 +41,7 @@ from registrar.models import ( TransitionDomain, Portfolio, Suborganization, + UserPortfolioPermission, ) from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.senior_official import SeniorOfficial From 9e1643edf08520383e1a6970bdce09ffb8749edd Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 20 Aug 2024 14:49:04 -0600 Subject: [PATCH 17/59] Fix (some) test errors --- src/registrar/tests/test_models.py | 53 +++++++++++++-------- src/registrar/tests/test_reports.py | 1 + src/registrar/tests/test_views_domain.py | 5 +- src/registrar/tests/test_views_portfolio.py | 10 ++-- 4 files changed, 42 insertions(+), 27 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 1c95257d3..52e950cdc 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1149,7 +1149,8 @@ class TestPortfolioInvitations(TestCase): @less_console_noise_decorator def test_retrieval(self): - self.assertFalse(self.user.portfolio) + portfolio_role_exists = UserPortfolioPermission.objects.filter(user=self.user).exists() + self.assertFalse(portfolio_role_exists) self.invitation.retrieve() self.user.refresh_from_db() self.assertEqual(self.user.last_selected_portfolio.organization_name, "Hotel California") @@ -1363,13 +1364,16 @@ class TestUser(TestCase): Note: This tests _get_portfolio_permissions as a side effect """ - portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") - self.user.last_selected_portfolio = portfolio - self.user.save() - self.user.refresh_from_db() - user_can_view_all_domains = self.user.has_domains_portfolio_permission() - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission() + portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") + + # Create a dummy request + request = self.factory.get("/") + request.user = self.user + request.session = {} + + user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) self.assertFalse(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) @@ -1380,8 +1384,13 @@ class TestUser(TestCase): portfolio_permission.refresh_from_db() self.user.refresh_from_db() - user_can_view_all_domains = self.user.has_domains_portfolio_permission() - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission() + # Create a dummy request + request = self.factory.get("/") + request.user = self.user + request.session = {} + + user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) self.assertTrue(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) @@ -1392,18 +1401,28 @@ class TestUser(TestCase): portfolio_permission.refresh_from_db() self.user.refresh_from_db() - user_can_view_all_domains = self.user.has_domains_portfolio_permission() - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission() + # Create a dummy request + request = self.factory.get("/") + request.user = self.user + request.session = {} + + user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) - UserDomainRole.objects.all().get_or_create( + UserDomainRole.objects.get_or_create( user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER ) - user_can_view_all_domains = self.user.has_domains_portfolio_permission() - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission() + # Create a dummy request + request = self.factory.get("/") + request.user = self.user + request.session = {} + + user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) @@ -1431,12 +1450,8 @@ class TestUser(TestCase): @less_console_noise_decorator def test_user_with_portfolio_roles_but_no_portfolio(self): - # Create an instance of User with a portfolio role but no portfolio - self.user.portfolio = None - self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) # Try to remove the portfolio portfolio_permission.portfolio = None diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index 8440e54e1..eca6bdb8b 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -345,6 +345,7 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): factory = RequestFactory() request = factory.get("/") request.user = self.user + request.session = {} # Get the csv content csv_content = self._run_domain_data_type_user_export(request) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index f3f4d1051..6ce842bc6 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -329,9 +329,8 @@ class TestDomainDetail(TestDomainOverview): email="bogus@example.gov", phone="8003111234", title="test title", - last_selected_portfolio=portfolio, ) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + UserPortfolioPermission.objects.get_or_create(user=user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) domain, _ = Domain.objects.get_or_create(name="bogusdomain.gov") DomainInformation.objects.get_or_create(creator=user, domain=domain, portfolio=portfolio) self.client.force_login(user) @@ -1572,7 +1571,7 @@ class TestDomainSuborganization(TestDomainOverview): self.domain_information.refresh_from_db() # Add portfolio perms to the user object - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.VIEW_PORTFOLIO]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) # Navigate to the domain overview page page = self.app.get(reverse("domain", kwargs={"pk": self.domain.id})) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index cb72f29fe..1c4cfcac9 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -31,6 +31,7 @@ class TestPortfolio(WebTest): ) def tearDown(self): + UserPortfolioPermission.objects.all().delete() Portfolio.objects.all().delete() UserDomainRole.objects.all().delete() DomainRequest.objects.all().delete() @@ -85,7 +86,6 @@ class TestPortfolio(WebTest): def test_middleware_does_not_redirect_if_no_portfolio(self): """Test that user with no assigned portfolio is not redirected when attempting to access home""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -97,7 +97,7 @@ class TestPortfolio(WebTest): def test_middleware_redirects_to_portfolio_organization_page(self): """Test that user with a portfolio and VIEW_PORTFOLIO is redirected to portfolio organization page""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -111,7 +111,7 @@ class TestPortfolio(WebTest): """Test that user with a portfolio, VIEW_PORTFOLIO, VIEW_ALL_DOMAINS is redirected to portfolio domains page""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS]) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -125,7 +125,7 @@ class TestPortfolio(WebTest): def test_portfolio_domains_page_403_when_user_not_have_permission(self): """Test that user without proper permission is denied access to portfolio domain view""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -137,7 +137,7 @@ class TestPortfolio(WebTest): def test_portfolio_domain_requests_page_403_when_user_not_have_permission(self): """Test that user without proper permission is denied access to portfolio domain view""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. From a362c2ef6c49c9dde1d58747321b1eea536c5edb Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 20 Aug 2024 14:53:16 -0600 Subject: [PATCH 18/59] Linting --- src/registrar/context_processors.py | 6 +- src/registrar/models/user.py | 8 +- .../models/user_portfolio_permission.py | 8 +- src/registrar/tests/test_models.py | 8 +- src/registrar/tests/test_views_domain.py | 16 +++- src/registrar/tests/test_views_portfolio.py | 76 +++++++++++++++---- src/registrar/views/portfolios.py | 4 +- 7 files changed, 90 insertions(+), 36 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index 16d592982..92a89ca02 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -66,7 +66,9 @@ def portfolio_permissions(request): return { "has_base_portfolio_permission": request.user.has_base_portfolio_permission(portfolio), "has_domains_portfolio_permission": request.user.has_domains_portfolio_permission(portfolio), - "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission(portfolio), + "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission( + portfolio + ), "has_view_suborganization": request.user.has_view_suborganization(portfolio), "has_edit_suborganization": request.user.has_edit_suborganization(portfolio), "portfolio": portfolio, @@ -81,7 +83,7 @@ def portfolio_permissions(request): "portfolio": None, "has_organization_feature_flag": False, } - + except AttributeError: # Handles cases where request.user might not exist return { diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index b151f3800..73b67b6ae 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -217,13 +217,13 @@ class User(AbstractUser): return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_PORTFOLIO) def has_domains_portfolio_permission(self, portfolio): - return self._has_portfolio_permission(portfolio, - UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS + return self._has_portfolio_permission( + portfolio, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) def has_domain_requests_portfolio_permission(self, portfolio): - return self._has_portfolio_permission(portfolio, - UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS + return self._has_portfolio_permission( + portfolio, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) def has_view_all_domains_permission(self, portfolio): diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index e00bf37f9..4582ffd08 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -75,11 +75,7 @@ class UserPortfolioPermission(TimeStampedModel): ) def __str__(self): - return ( - f"User '{self.user}' on Portfolio '{self.portfolio}' " f"" - if self.roles - else "" - ) + return f"User '{self.user}' on Portfolio '{self.portfolio}' " f"" if self.roles else "" def _get_portfolio_permissions(self): """ @@ -107,7 +103,7 @@ class UserPortfolioPermission(TimeStampedModel): raise ValidationError( "Only one portfolio permission is allowed per user when multiple portfolios are disabled." ) - + if self.portfolio is None and self._get_portfolio_permissions(): raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.") diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 52e950cdc..ec672c970 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1412,9 +1412,7 @@ class TestUser(TestCase): self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) - UserDomainRole.objects.get_or_create( - user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER - ) + UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER) # Create a dummy request request = self.factory.get("/") @@ -1451,7 +1449,9 @@ class TestUser(TestCase): @less_console_noise_decorator def test_user_with_portfolio_roles_but_no_portfolio(self): portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=portfolio, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) # Try to remove the portfolio portfolio_permission.portfolio = None diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 6ce842bc6..ab489c813 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -330,7 +330,9 @@ class TestDomainDetail(TestDomainOverview): phone="8003111234", title="test title", ) - UserPortfolioPermission.objects.get_or_create(user=user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + UserPortfolioPermission.objects.get_or_create( + user=user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) domain, _ = Domain.objects.get_or_create(name="bogusdomain.gov") DomainInformation.objects.get_or_create(creator=user, domain=domain, portfolio=portfolio) self.client.force_login(user) @@ -1477,7 +1479,9 @@ class TestDomainSuborganization(TestDomainOverview): self.domain_information.refresh_from_db() # Add portfolio perms to the user object - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) self.assertEqual(self.domain_information.sub_organization, suborg) @@ -1533,7 +1537,9 @@ class TestDomainSuborganization(TestDomainOverview): self.domain_information.refresh_from_db() # Add portfolio perms to the user object - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY] + ) self.assertEqual(self.domain_information.sub_organization, suborg) @@ -1571,7 +1577,9 @@ class TestDomainSuborganization(TestDomainOverview): self.domain_information.refresh_from_db() # Add portfolio perms to the user object - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO] + ) # Navigate to the domain overview page page = self.app.get(reverse("domain", kwargs={"pk": self.domain.id})) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 1c4cfcac9..f785a1551 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -54,7 +54,11 @@ class TestPortfolio(WebTest): self.portfolio.save() self.portfolio.refresh_from_db() - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO], + ) so_portfolio_page = self.app.get(reverse("senior-official")) # Assert that we're on the right page @@ -71,7 +75,9 @@ class TestPortfolio(WebTest): def test_middleware_does_not_redirect_if_no_permission(self): """Test that user with no portfolio permission is not redirected when attempting to access home""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=[] + ) self.user.portfolio = self.portfolio self.user.save() self.user.refresh_from_db() @@ -97,7 +103,11 @@ class TestPortfolio(WebTest): def test_middleware_redirects_to_portfolio_organization_page(self): """Test that user with a portfolio and VIEW_PORTFOLIO is redirected to portfolio organization page""" self.app.set_user(self.user.username) - UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO], + ) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -111,7 +121,14 @@ class TestPortfolio(WebTest): """Test that user with a portfolio, VIEW_PORTFOLIO, VIEW_ALL_DOMAINS is redirected to portfolio domains page""" self.app.set_user(self.user.username) - UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS]) + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, + ], + ) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -125,7 +142,9 @@ class TestPortfolio(WebTest): def test_portfolio_domains_page_403_when_user_not_have_permission(self): """Test that user without proper permission is denied access to portfolio domain view""" self.app.set_user(self.user.username) - UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=[] + ) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -137,7 +156,9 @@ class TestPortfolio(WebTest): def test_portfolio_domain_requests_page_403_when_user_not_have_permission(self): """Test that user without proper permission is denied access to portfolio domain view""" self.app.set_user(self.user.username) - UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=[] + ) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -149,7 +170,9 @@ class TestPortfolio(WebTest): def test_portfolio_organization_page_403_when_user_not_have_permission(self): """Test that user without proper permission is not allowed access to portfolio organization page""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=[] + ) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -161,7 +184,11 @@ class TestPortfolio(WebTest): def test_portfolio_organization_page_read_only(self): """Test that user with a portfolio can access the portfolio organization page, read only""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO], + ) self.portfolio.city = "Los Angeles" self.portfolio.save() with override_flag("organization_feature", active=True): @@ -179,7 +206,14 @@ class TestPortfolio(WebTest): def test_portfolio_organization_page_edit_access(self): """Test that user with a portfolio can access the portfolio organization page, read only""" self.app.set_user(self.user.username) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.EDIT_PORTFOLIO, + ], + ) self.portfolio.city = "Los Angeles" self.portfolio.save() with override_flag("organization_feature", active=True): @@ -202,7 +236,9 @@ class TestPortfolio(WebTest): UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, ] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions + ) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -217,7 +253,9 @@ class TestPortfolio(WebTest): # removing non-basic portfolio perms, which should remove domains # and domain requests from nav portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions + ) self.user.save() self.user.refresh_from_db() @@ -234,7 +272,9 @@ class TestPortfolio(WebTest): """Test that admin / memmber roles are associated with the right access""" self.app.set_user(self.user.username) portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, roles=portfolio_roles) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=portfolio_roles + ) with override_flag("organization_feature", active=True): # This will redirect the user to the portfolio page. # Follow implicity checks if our redirect is working. @@ -269,7 +309,9 @@ class TestPortfolio(WebTest): UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions + ) page = self.app.get(reverse("organization")) self.assertContains( page, "The name of your federal agency will be publicly listed as the domain registrant." @@ -284,7 +326,9 @@ class TestPortfolio(WebTest): UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions + ) self.portfolio.organization_name = "Hotel California" self.portfolio.save() @@ -302,7 +346,9 @@ class TestPortfolio(WebTest): UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO, ] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions + ) self.portfolio.address_line1 = "1600 Penn Ave" self.portfolio.save() diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 554576a2e..d6869c37a 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -51,7 +51,9 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): def get_context_data(self, **kwargs): """Add additional context data to the template.""" context = super().get_context_data(**kwargs) - context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission(self.request.session["portfolio"]) + context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission( + self.request.session["portfolio"] + ) return context def get_object(self, queryset=None): From 381ac494870a4427524dae1116bae5b4246cc380 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 20 Aug 2024 14:58:51 -0600 Subject: [PATCH 19/59] lint pt 2 --- src/registrar/models/user.py | 1 - src/registrar/registrar_middleware.py | 8 ++++---- src/registrar/tests/test_admin.py | 1 - 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 73b67b6ae..521093e2f 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -3,7 +3,6 @@ import logging from django.contrib.auth.models import AbstractUser from django.db import models from django.db.models import Q -from django.forms import ValidationError from registrar.models import DomainInformation, UserDomainRole from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index 1da74a527..d0f2dfa2f 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -143,20 +143,20 @@ class CheckPortfolioMiddleware: if not request.user.is_authenticated: return None - + # set the portfolio in the session if it is not set - if not "portfolio" in request.session or request.session["portfolio"] is None: + if "portfolio" not in request.session or request.session["portfolio"] is None: # if user is a multiple portfolio if flag_is_active(request, "multiple_portfolios"): # NOTE: we will want to change later to have a workflow for selecting # portfolio and another for switching portfolio; for now, select first request.session["portfolio"] = request.user.get_first_portfolio() - elif flag_is_active(request, "organization_feature"): + elif flag_is_active(request, "organization_feature"): request.session["portfolio"] = request.user.get_first_portfolio() else: request.session["portfolio"] = None - if request.session["portfolio"] is not None and current_path == self.home: + if request.session["portfolio"] is not None and current_path == self.home: if request.user.has_base_portfolio_permission(request.session["portfolio"]): if request.user.has_domains_portfolio_permission(request.session["portfolio"]): portfolio_redirect = reverse("domains") diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 144605b87..49d2b4802 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -41,7 +41,6 @@ from registrar.models import ( TransitionDomain, Portfolio, Suborganization, - UserPortfolioPermission, ) from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.senior_official import SeniorOfficial From 4f4dac272873c4cab07e84050fb63edfd7936f8b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 08:42:05 -0600 Subject: [PATCH 20/59] Add better error handling on session["portfolio"] This was causing an error on some functions where request can be None, like our csv exports --- src/registrar/models/user.py | 8 +++++--- src/registrar/tests/common.py | 4 ++++ src/registrar/tests/test_models.py | 26 ++++++++++++++------------ src/registrar/views/domain.py | 14 +++++++++----- src/registrar/views/portfolios.py | 13 ++++++++----- src/registrar/views/utility/mixins.py | 11 ++++++++--- 6 files changed, 48 insertions(+), 28 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 521093e2f..8789a628b 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -385,12 +385,14 @@ class User(AbstractUser): def is_org_user(self, request): has_organization_feature_flag = flag_is_active(request, "organization_feature") - return has_organization_feature_flag and self.has_base_portfolio_permission(request.session["portfolio"]) + portfolio = request.session["portfolio"] if "portfolio" in request.session else None + return has_organization_feature_flag and self.has_base_portfolio_permission(portfolio) def get_user_domain_ids(self, request): """Returns either the domains ids associated with this user on UserDomainRole or Portfolio""" - if self.is_org_user(request) and self.has_view_all_domains_permission(request.session["portfolio"]): - return DomainInformation.objects.filter(portfolio=request.session["portfolio"]).values_list( + portfolio = request.session["portfolio"] if "portfolio" in request.session else None + if self.is_org_user(request) and self.has_view_all_domains_permission(portfolio): + return DomainInformation.objects.filter(portfolio=portfolio).values_list( "domain_id", flat=True ) else: diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index ceb3b6e92..96da5c14d 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -27,6 +27,8 @@ from registrar.models import ( PublicContact, Domain, FederalAgency, + UserPortfolioPermission, + Portfolio, ) from epplibwrapper import ( commands, @@ -791,6 +793,8 @@ class MockDb(TestCase): DomainInformation.objects.all().delete() DomainRequest.objects.all().delete() UserDomainRole.objects.all().delete() + Portfolio.objects.all().delete() + UserPortfolioPermission.objects.all().delete() User.objects.all().delete() DomainInvitation.objects.all().delete() cls.federal_agency_1.delete() diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index ec672c970..8db11a0e7 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1143,21 +1143,22 @@ class TestPortfolioInvitations(TestCase): def tearDown(self): super().tearDown() + UserPortfolioPermission.objects.all().delete() Portfolio.objects.all().delete() PortfolioInvitation.objects.all().delete() User.objects.all().delete() @less_console_noise_decorator def test_retrieval(self): - portfolio_role_exists = UserPortfolioPermission.objects.filter(user=self.user).exists() + portfolio_role_exists = UserPortfolioPermission.objects.filter(user=self.user, portfolio=self.portfolio).exists() self.assertFalse(portfolio_role_exists) self.invitation.retrieve() self.user.refresh_from_db() - self.assertEqual(self.user.last_selected_portfolio.organization_name, "Hotel California") - portfolio_role, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio) - self.assertEqual(portfolio_role.roles, [self.portfolio_role_base, self.portfolio_role_admin]) + created_role = UserPortfolioPermission.objects.get(user=self.user, portfolio=self.portfolio) + self.assertEqual(created_role.portfolio.organization_name, "Hotel California") + self.assertEqual(created_role.roles, [self.portfolio_role_base, self.portfolio_role_admin]) self.assertEqual( - portfolio_role.additional_permissions, [self.portfolio_permission_1, self.portfolio_permission_2] + created_role.additional_permissions, [self.portfolio_permission_1, self.portfolio_permission_2] ) self.assertEqual(self.invitation.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) @@ -1170,15 +1171,15 @@ class TestPortfolioInvitations(TestCase): @less_console_noise_decorator def test_retrieve_user_already_member_error(self): - self.assertFalse(self.user.last_selected_portfolio) - portfolio2, _ = Portfolio.objects.get_or_create(creator=self.user2, organization_name="Tokyo Hotel") - portfolio_role, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=portfolio2) - self.assertEqual(self.user.last_selected_portfolio.organization_name, "Tokyo Hotel") - self.assertEqual(portfolio_role.portfolio, portfolio2) - self.user.save() + portfolio_role_exists = UserPortfolioPermission.objects.filter(user=self.user, portfolio=self.portfolio).exists() + self.assertFalse(portfolio_role_exists) + portfolio_role, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio) + self.assertEqual(portfolio_role.portfolio.organization_name, "Hotel California") self.user.check_portfolio_invitations_on_login() self.user.refresh_from_db() - self.assertEqual(self.user.portfolio.organization_name, "Tokyo Hotel") + + roles = UserPortfolioPermission.objects.filter(user=self.user) + self.assertEqual(len(roles), 1) self.assertEqual(self.invitation.status, PortfolioInvitation.PortfolioInvitationStatus.INVITED) @@ -1192,6 +1193,7 @@ class TestUser(TestCase): self.domain_name = "igorvilleInTransition.gov" self.domain, _ = Domain.objects.get_or_create(name="igorville.gov") self.user, _ = User.objects.get_or_create(email=self.email) + self.factory = RequestFactory() def tearDown(self): super().tearDown() diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index aac9d056d..7f31945f6 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -174,10 +174,11 @@ class DomainView(DomainBaseView): """Most views should not allow permission to portfolio users. If particular views allow permissions, they will need to override this function.""" - if self.request.user.has_domains_portfolio_permission(self.request.session["portfolio"]): + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + if self.request.user.has_domains_portfolio_permission(portfolio): if Domain.objects.filter(id=pk).exists(): domain = Domain.objects.get(id=pk) - if domain.domain_info.portfolio == self.request.session["portfolio"]: + if domain.domain_info.portfolio == portfolio: return True return False @@ -236,7 +237,8 @@ class DomainOrgNameAddressView(DomainFormBaseView): # Org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - if self.request.session["portfolio"] and is_org_user: + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + if portfolio and is_org_user: return False else: return super().has_permission() @@ -255,7 +257,8 @@ class DomainSubOrganizationView(DomainFormBaseView): # non-org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - if self.request.session["portfolio"] and is_org_user: + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + if portfolio and is_org_user: return super().has_permission() else: return False @@ -335,7 +338,8 @@ class DomainSeniorOfficialView(DomainFormBaseView): # Org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - if self.request.session["portfolio"] and is_org_user: + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + if portfolio and is_org_user: return False else: return super().has_permission() diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index d6869c37a..d3a6d6055 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -51,16 +51,18 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): def get_context_data(self, **kwargs): """Add additional context data to the template.""" context = super().get_context_data(**kwargs) + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission( - self.request.session["portfolio"] + portfolio ) return context def get_object(self, queryset=None): """Get the portfolio object based on the session.""" - if self.request.session["portfolio"] is None: + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + if portfolio is None: raise Http404("No organization found for this user") - return self.request.session["portfolio"] + return portfolio def get_form_kwargs(self): """Include the instance in the form kwargs.""" @@ -113,9 +115,10 @@ class PortfolioSeniorOfficialView(PortfolioBasePermissionView, FormMixin): def get_object(self, queryset=None): """Get the portfolio object based on the session.""" - if self.request.session["portfolio"] is None: + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + if portfolio is None: raise Http404("No organization found for this user") - return self.request.session["portfolio"] + return portfolio def get_form_kwargs(self): """Include the instance in the form kwargs.""" diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index c6fdd5b0a..1f255f414 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -419,7 +419,8 @@ class PortfolioBasePermission(PermissionsLoginMixin): if not self.request.user.is_authenticated: return False - return self.request.user.has_base_portfolio_permission(self.request.session["portfolio"]) + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + return self.request.user.has_base_portfolio_permission(portfolio) class PortfolioDomainsPermission(PortfolioBasePermission): @@ -434,7 +435,9 @@ class PortfolioDomainsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - return self.request.user.has_domains_portfolio_permission(self.request.session["portfolio"]) + + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + return self.request.user.has_domains_portfolio_permission(portfolio) class PortfolioDomainRequestsPermission(PortfolioBasePermission): @@ -449,4 +452,6 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - return self.request.user.has_domain_requests_portfolio_permission(self.request.session["portfolio"]) + + portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + return self.request.user.has_domain_requests_portfolio_permission(portfolio) From 729ce4c9f784089f7ce4a31cb67670d77be73210 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 08:46:47 -0600 Subject: [PATCH 21/59] Replace inline if with .get Not sure why I was doing that --- src/registrar/context_processors.py | 2 +- src/registrar/models/user.py | 4 ++-- src/registrar/views/domain.py | 8 ++++---- src/registrar/views/portfolios.py | 6 +++--- src/registrar/views/utility/mixins.py | 6 +++--- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index 92a89ca02..ea04dca80 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -61,7 +61,7 @@ def add_has_profile_feature_flag_to_context(request): def portfolio_permissions(request): """Make portfolio permissions for the request user available in global context""" try: - portfolio = request.session["portfolio"] if "portfolio" in request.session else None + portfolio = request.session.get("portfolio") if portfolio: return { "has_base_portfolio_permission": request.user.has_base_portfolio_permission(portfolio), diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 8789a628b..c19778827 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -385,12 +385,12 @@ class User(AbstractUser): def is_org_user(self, request): has_organization_feature_flag = flag_is_active(request, "organization_feature") - portfolio = request.session["portfolio"] if "portfolio" in request.session else None + portfolio = request.session.get("portfolio") return has_organization_feature_flag and self.has_base_portfolio_permission(portfolio) def get_user_domain_ids(self, request): """Returns either the domains ids associated with this user on UserDomainRole or Portfolio""" - portfolio = request.session["portfolio"] if "portfolio" in request.session else None + portfolio = request.session.get("portfolio") if self.is_org_user(request) and self.has_view_all_domains_permission(portfolio): return DomainInformation.objects.filter(portfolio=portfolio).values_list( "domain_id", flat=True diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 7f31945f6..003f8dd0d 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -174,7 +174,7 @@ class DomainView(DomainBaseView): """Most views should not allow permission to portfolio users. If particular views allow permissions, they will need to override this function.""" - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") if self.request.user.has_domains_portfolio_permission(portfolio): if Domain.objects.filter(id=pk).exists(): domain = Domain.objects.get(id=pk) @@ -237,7 +237,7 @@ class DomainOrgNameAddressView(DomainFormBaseView): # Org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") if portfolio and is_org_user: return False else: @@ -257,7 +257,7 @@ class DomainSubOrganizationView(DomainFormBaseView): # non-org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") if portfolio and is_org_user: return super().has_permission() else: @@ -338,7 +338,7 @@ class DomainSeniorOfficialView(DomainFormBaseView): # Org users shouldn't have access to this page is_org_user = self.request.user.is_org_user(self.request) - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") if portfolio and is_org_user: return False else: diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index d3a6d6055..18285774b 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -51,7 +51,7 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): def get_context_data(self, **kwargs): """Add additional context data to the template.""" context = super().get_context_data(**kwargs) - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") context["has_edit_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission( portfolio ) @@ -59,7 +59,7 @@ class PortfolioOrganizationView(PortfolioBasePermissionView, FormMixin): def get_object(self, queryset=None): """Get the portfolio object based on the session.""" - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") if portfolio is None: raise Http404("No organization found for this user") return portfolio @@ -115,7 +115,7 @@ class PortfolioSeniorOfficialView(PortfolioBasePermissionView, FormMixin): def get_object(self, queryset=None): """Get the portfolio object based on the session.""" - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") if portfolio is None: raise Http404("No organization found for this user") return portfolio diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 1f255f414..376912634 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -419,7 +419,7 @@ class PortfolioBasePermission(PermissionsLoginMixin): if not self.request.user.is_authenticated: return False - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") return self.request.user.has_base_portfolio_permission(portfolio) @@ -436,7 +436,7 @@ class PortfolioDomainsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") return self.request.user.has_domains_portfolio_permission(portfolio) @@ -453,5 +453,5 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - portfolio = self.request.session["portfolio"] if "portfolio" in self.request.session else None + portfolio = self.request.session.get("portfolio") return self.request.user.has_domain_requests_portfolio_permission(portfolio) From ba2add8bc8346c76311964989a6d870cd1f7e9cf Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 09:42:31 -0600 Subject: [PATCH 22/59] Fix redirect bug and fix permission issue --- src/registrar/models/user.py | 5 +++++ src/registrar/registrar_middleware.py | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index c19778827..38e971a3b 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -202,6 +202,9 @@ class User(AbstractUser): def _has_portfolio_permission(self, portfolio, portfolio_permission): """The views should only call this function when testing for perms and not rely on roles.""" + if not portfolio: + return False + portfolio_perms = self.portfolio_permissions.filter(portfolio=portfolio, user=self).first() if not portfolio_perms: return False @@ -383,6 +386,8 @@ class User(AbstractUser): self.check_domain_invitations_on_login() self.check_portfolio_invitations_on_login() + # NOTE TO DAVE: I'd simply suggest that we move these functions outside of the user object, + # and move them to some sort of utility file. That way we aren't calling request inside here. def is_org_user(self, request): has_organization_feature_flag = flag_is_active(request, "organization_feature") portfolio = request.session.get("portfolio") diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index d0f2dfa2f..aa92ed5cd 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -157,7 +157,7 @@ class CheckPortfolioMiddleware: request.session["portfolio"] = None if request.session["portfolio"] is not None and current_path == self.home: - if request.user.has_base_portfolio_permission(request.session["portfolio"]): + if request.user.is_org_user(request): if request.user.has_domains_portfolio_permission(request.session["portfolio"]): portfolio_redirect = reverse("domains") else: From cbec3f2ed7b5e6445568cf83d4097df047467b41 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 09:43:24 -0600 Subject: [PATCH 23/59] Update mixins.py --- src/registrar/views/utility/mixins.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 376912634..c35804243 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -419,8 +419,7 @@ class PortfolioBasePermission(PermissionsLoginMixin): if not self.request.user.is_authenticated: return False - portfolio = self.request.session.get("portfolio") - return self.request.user.has_base_portfolio_permission(portfolio) + return self.request.user.is_org_user(self.request) class PortfolioDomainsPermission(PortfolioBasePermission): @@ -436,8 +435,7 @@ class PortfolioDomainsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - portfolio = self.request.session.get("portfolio") - return self.request.user.has_domains_portfolio_permission(portfolio) + return self.request.user.is_org_user(self.request) class PortfolioDomainRequestsPermission(PortfolioBasePermission): @@ -453,5 +451,4 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - portfolio = self.request.session.get("portfolio") - return self.request.user.has_domain_requests_portfolio_permission(portfolio) + return self.request.user.is_org_user(self.request) From 5c077752770becfd67e7cf5d41d8284e1b084d9a Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 10:15:49 -0600 Subject: [PATCH 24/59] Fix two tests --- src/registrar/tests/common.py | 9 +++++++++ src/registrar/tests/test_models.py | 8 +++++++- src/registrar/tests/test_reports.py | 27 ++++++++++++--------------- 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 96da5c14d..f5f62cf3e 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -1747,3 +1747,12 @@ class MockEppLib(TestCase): def tearDown(self): self.mockSendPatch.stop() + + +def get_wsgi_request_object(client, user, url="/"): + """Returns client.get(url).wsgi_request for testing functions or classes + that need a request object directly passed to them.""" + client.force_login(user) + request = client.get(url).wsgi_request + request.user = user + return request diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 8db11a0e7..33499eb91 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1459,9 +1459,15 @@ class TestUser(TestCase): portfolio_permission.portfolio = None portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + # Create a new UserPortfolioPermission instance without a portfolio + invalid_permission = UserPortfolioPermission( + user=self.user, + roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], + portfolio=None # This should trigger the validation error + ) # Test if the ValidationError is raised with the correct message with self.assertRaises(ValidationError) as cm: - portfolio_permission.clean() + invalid_permission.clean() self.assertEqual( cm.exception.message, "When portfolio roles or additional permissions are assigned, portfolio is required." diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index eca6bdb8b..1e169b51d 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -34,7 +34,7 @@ import boto3_mocking from registrar.utility.s3_bucket import S3ClientError, S3ClientErrorCodes # type: ignore from django.utils import timezone from api.tests.common import less_console_noise_decorator -from .common import MockDbForSharedTests, MockDbForIndividualTests, MockEppLib, less_console_noise, get_time_aware_date +from .common import MockDbForSharedTests, MockDbForIndividualTests, MockEppLib, get_wsgi_request_object, less_console_noise, get_time_aware_date from waffle.testutils import override_flag @@ -282,10 +282,8 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): # Create a user and associate it with some domains UserDomainRole.objects.create(user=self.user, domain=self.domain_2) - # Create a request object - factory = RequestFactory() - request = factory.get("/") - request.user = self.user + # Make a GET request using self.client to get a request object + request = get_wsgi_request_object(client=self.client, user=self.user) # Create a CSV file in memory csv_file = StringIO() @@ -339,13 +337,9 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] portfolio_permission.save() portfolio_permission.refresh_from_db() - self.user.refresh_from_db() - # Create a request object - factory = RequestFactory() - request = factory.get("/") - request.user = self.user - request.session = {} + # Make a GET request using self.client to get a request object + request = get_wsgi_request_object(client=self.client, user=self.user) # Get the csv content csv_content = self._run_domain_data_type_user_export(request) @@ -356,19 +350,22 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): self.assertNotIn(self.domain_2.name, csv_content) # Test the output for readonly admin - portfolio_permission.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY] + portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY] portfolio_permission.save() + portfolio_permission.refresh_from_db() + # Get the csv content + csv_content = self._run_domain_data_type_user_export(request) self.assertIn(self.domain_1.name, csv_content) self.assertIn(self.domain_3.name, csv_content) self.assertNotIn(self.domain_2.name, csv_content) - # Get the csv content - portfolio_permission.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] portfolio_permission.save() + portfolio_permission.refresh_from_db() + # Get the csv content csv_content = self._run_domain_data_type_user_export(request) - self.assertNotIn(self.domain_1.name, csv_content) self.assertNotIn(self.domain_3.name, csv_content) self.assertIn(self.domain_2.name, csv_content) From 08c42a6e8d48bb117e494e4ae82095c403106521 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Wed, 21 Aug 2024 12:28:41 -0500 Subject: [PATCH 25/59] merge main and resolve migration conflicts --- ...sion_dates.py => 0119_add_domainrequest_submission_dates.py} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename src/registrar/migrations/{0118_add_domainrequest_submission_dates.py => 0119_add_domainrequest_submission_dates.py} (92%) diff --git a/src/registrar/migrations/0118_add_domainrequest_submission_dates.py b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py similarity index 92% rename from src/registrar/migrations/0118_add_domainrequest_submission_dates.py rename to src/registrar/migrations/0119_add_domainrequest_submission_dates.py index c971b2f9a..0b94e1257 100644 --- a/src/registrar/migrations/0118_add_domainrequest_submission_dates.py +++ b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py @@ -6,7 +6,7 @@ from django.db import migrations, models class Migration(migrations.Migration): dependencies = [ - ("registrar", "0117_alter_portfolioinvitation_portfolio_additional_permissions_and_more"), + ("registrar", "0118_alter_portfolio_options_alter_portfolio_creator_and_more"), ] operations = [ From 32ed84a5e15d07c820d4553fb4804ad1c1a06e71 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 11:43:40 -0600 Subject: [PATCH 26/59] Fix logic in clean --- src/registrar/models/user_portfolio_permission.py | 10 +++++++--- src/registrar/tests/test_models.py | 8 +------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index 4582ffd08..a3460a651 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -97,15 +97,19 @@ class UserPortfolioPermission(TimeStampedModel): """Extends clean method to perform additional validation, which can raise errors in django admin.""" super().clean() - if not flag_is_active(None, "multiple_portfolios") and self.pk is None: + # Check if a user is set without accessing the related object. + has_user = bool(self.user_id) + if not flag_is_active(None, "multiple_portfolios") and self.pk is None and has_user: existing_permissions = UserPortfolioPermission.objects.filter(user=self.user) if existing_permissions.exists(): raise ValidationError( "Only one portfolio permission is allowed per user when multiple portfolios are disabled." ) - if self.portfolio is None and self._get_portfolio_permissions(): + # Check if portfolio is set without accessing the related object. + has_portfolio = bool(self.portfolio_id) + if not has_portfolio and self._get_portfolio_permissions(): raise ValidationError("When portfolio roles or additional permissions are assigned, portfolio is required.") - if self.portfolio is not None and not self._get_portfolio_permissions(): + if has_portfolio and not self._get_portfolio_permissions(): raise ValidationError("When portfolio is assigned, portfolio roles or additional permissions are required.") diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 33499eb91..8db11a0e7 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1459,15 +1459,9 @@ class TestUser(TestCase): portfolio_permission.portfolio = None portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - # Create a new UserPortfolioPermission instance without a portfolio - invalid_permission = UserPortfolioPermission( - user=self.user, - roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN], - portfolio=None # This should trigger the validation error - ) # Test if the ValidationError is raised with the correct message with self.assertRaises(ValidationError) as cm: - invalid_permission.clean() + portfolio_permission.clean() self.assertEqual( cm.exception.message, "When portfolio roles or additional permissions are assigned, portfolio is required." From bfc2693a12e61f8629777a31f4612cdf02901d7d Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 12:12:50 -0600 Subject: [PATCH 27/59] Fix a few more tests --- src/registrar/tests/test_models.py | 61 +++++++++------------ src/registrar/tests/test_views_domain.py | 12 ++-- src/registrar/tests/test_views_portfolio.py | 9 +-- 3 files changed, 37 insertions(+), 45 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 8db11a0e7..995f2a89c 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -30,6 +30,7 @@ from registrar.utility.constants import BranchChoices from .common import ( MockSESClient, + get_wsgi_request_object, less_console_noise, completed_domain_request, set_domain_request_investigators, @@ -1369,60 +1370,50 @@ class TestUser(TestCase): portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") - # Create a dummy request - request = self.factory.get("/") - request.user = self.user - request.session = {} - - user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + # TODO - uncomment this when we just pass request to these functions + # request = get_wsgi_request_object(self.client, self.user) + # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) self.assertFalse(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) - portfolio_permission.additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS] - portfolio_permission.save() - portfolio_permission.refresh_from_db() - self.user.refresh_from_db() + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user, additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS]) - # Create a dummy request - request = self.factory.get("/") - request.user = self.user - request.session = {} - - user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + # TODO - uncomment this when we just pass request to these functions + # request = get_wsgi_request_object(self.client, self.user) + # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) self.assertTrue(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] portfolio_permission.save() portfolio_permission.refresh_from_db() - self.user.refresh_from_db() - # Create a dummy request - request = self.factory.get("/") - request.user = self.user - request.session = {} - - user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + # TODO - uncomment this when we just pass request to these functions + # request = get_wsgi_request_object(self.client, self.user) + # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER) - # Create a dummy request - request = self.factory.get("/") - request.user = self.user - request.session = {} - - user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + # TODO - uncomment this when we just pass request to these functions + # request = get_wsgi_request_object(self.client, self.user) + # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) + # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) + user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) + user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) self.assertTrue(user_can_view_all_domains) self.assertTrue(user_can_view_all_requests) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index ab489c813..e457f1b66 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -317,6 +317,7 @@ class TestDomainDetail(TestDomainOverview): self.assertContains(detail_page, "Domain missing domain information") @less_console_noise_decorator + @override_flag("organization_feature", active=True) def test_domain_readonly_on_detail_page(self): """Test that a domain, which is part of a portfolio, but for which the user is not a domain manager, properly displays read only""" @@ -330,11 +331,13 @@ class TestDomainDetail(TestDomainOverview): phone="8003111234", title="test title", ) + domain, _ = Domain.objects.get_or_create(name="bogusdomain.gov") + DomainInformation.objects.get_or_create(creator=user, domain=domain, portfolio=portfolio) + UserPortfolioPermission.objects.get_or_create( user=user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] ) - domain, _ = Domain.objects.get_or_create(name="bogusdomain.gov") - DomainInformation.objects.get_or_create(creator=user, domain=domain, portfolio=portfolio) + user.refresh_from_db() self.client.force_login(user) detail_page = self.client.get(f"/domain/{domain.id}") # Check that alert message displays properly @@ -1577,9 +1580,10 @@ class TestDomainSuborganization(TestDomainOverview): self.domain_information.refresh_from_db() # Add portfolio perms to the user object - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=portfolio, additional_permissions=[UserPortfolioPermissionChoices.VIEW_PORTFOLIO] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] ) + self.user.refresh_from_db() # Navigate to the domain overview page page = self.app.get(reverse("domain", kwargs={"pk": self.domain.id})) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f785a1551..039f6f13e 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -252,12 +252,9 @@ class TestPortfolio(WebTest): # removing non-basic portfolio perms, which should remove domains # and domain requests from nav - portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, additional_permissions=portfolio_additional_permissions - ) - self.user.save() - self.user.refresh_from_db() + portfolio_permission.additional_permissions = [UserPortfolioPermissionChoices.VIEW_PORTFOLIO] + portfolio_permission.save() + portfolio_permission.refresh_from_db() portfolio_page = self.app.get(reverse("home")).follow() From aa872d6e41457a73fe13f64e9eb3b38502144bed Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 12:22:03 -0600 Subject: [PATCH 28/59] Fix last test and lint --- src/registrar/models/user.py | 4 +--- src/registrar/tests/test_admin.py | 1 - src/registrar/tests/test_models.py | 15 +++++++++++---- src/registrar/tests/test_reports.py | 9 ++++++++- src/registrar/tests/test_views_portfolio.py | 2 +- src/registrar/views/portfolios.py | 4 +--- src/registrar/views/utility/mixins.py | 4 ++-- 7 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 38e971a3b..6aed8195c 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -397,8 +397,6 @@ class User(AbstractUser): """Returns either the domains ids associated with this user on UserDomainRole or Portfolio""" portfolio = request.session.get("portfolio") if self.is_org_user(request) and self.has_view_all_domains_permission(portfolio): - return DomainInformation.objects.filter(portfolio=portfolio).values_list( - "domain_id", flat=True - ) + return DomainInformation.objects.filter(portfolio=portfolio).values_list("domain_id", flat=True) else: return UserDomainRole.objects.filter(user=self).values_list("domain_id", flat=True) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 49d2b4802..827742ef1 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1209,7 +1209,6 @@ class TestMyUserAdmin(MockDbForSharedTests): domain_deleted.delete() role.delete() - # TODO - should refactor def test_analyst_cannot_see_selects_for_portfolio_role_and_permissions_in_user_form(self): """Can only test for the presence of a base element. The multiselects and the h2->h3 conversion are all dynamically generated.""" diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 995f2a89c..3a5405fe8 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -30,7 +30,6 @@ from registrar.utility.constants import BranchChoices from .common import ( MockSESClient, - get_wsgi_request_object, less_console_noise, completed_domain_request, set_domain_request_investigators, @@ -1151,7 +1150,9 @@ class TestPortfolioInvitations(TestCase): @less_console_noise_decorator def test_retrieval(self): - portfolio_role_exists = UserPortfolioPermission.objects.filter(user=self.user, portfolio=self.portfolio).exists() + portfolio_role_exists = UserPortfolioPermission.objects.filter( + user=self.user, portfolio=self.portfolio + ).exists() self.assertFalse(portfolio_role_exists) self.invitation.retrieve() self.user.refresh_from_db() @@ -1172,7 +1173,9 @@ class TestPortfolioInvitations(TestCase): @less_console_noise_decorator def test_retrieve_user_already_member_error(self): - portfolio_role_exists = UserPortfolioPermission.objects.filter(user=self.user, portfolio=self.portfolio).exists() + portfolio_role_exists = UserPortfolioPermission.objects.filter( + user=self.user, portfolio=self.portfolio + ).exists() self.assertFalse(portfolio_role_exists) portfolio_role, _ = UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio) self.assertEqual(portfolio_role.portfolio.organization_name, "Hotel California") @@ -1380,7 +1383,11 @@ class TestUser(TestCase): self.assertFalse(user_can_view_all_domains) self.assertFalse(user_can_view_all_requests) - portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user, additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS]) + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=portfolio, + user=self.user, + additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS], + ) # TODO - uncomment this when we just pass request to these functions # request = get_wsgi_request_object(self.client, self.user) diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index 1e169b51d..8bb15921d 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -34,7 +34,14 @@ import boto3_mocking from registrar.utility.s3_bucket import S3ClientError, S3ClientErrorCodes # type: ignore from django.utils import timezone from api.tests.common import less_console_noise_decorator -from .common import MockDbForSharedTests, MockDbForIndividualTests, MockEppLib, get_wsgi_request_object, less_console_noise, get_time_aware_date +from .common import ( + MockDbForSharedTests, + MockDbForIndividualTests, + MockEppLib, + get_wsgi_request_object, + less_console_noise, + get_time_aware_date, +) from waffle.testutils import override_flag diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 039f6f13e..1568391fc 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -285,7 +285,7 @@ class TestPortfolio(WebTest): # removing non-basic portfolio role, which should remove domains # and domain requests from nav - portfolio_permission.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + portfolio_permission.roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] portfolio_permission.save() portfolio_permission.refresh_from_db() diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 18285774b..1b5cabea7 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -52,9 +52,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_org_portfolio_permission"] = self.request.user.has_edit_org_portfolio_permission(portfolio) return context def get_object(self, queryset=None): diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index c35804243..d1edfc46e 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -434,7 +434,7 @@ class PortfolioDomainsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - + return self.request.user.is_org_user(self.request) @@ -450,5 +450,5 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): if not self.request.user.is_authenticated: return False - + return self.request.user.is_org_user(self.request) From f4647caded419d5ea41d0b336a5120c9cff47999 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 14:28:23 -0400 Subject: [PATCH 29/59] working on tests --- src/registrar/tests/test_views_portfolio.py | 45 +++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f785a1551..ffcc87823 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -14,6 +14,7 @@ from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from .common import create_test_user from waffle.testutils import override_flag +from django.contrib.sessions.middleware import SessionMiddleware import logging @@ -364,3 +365,47 @@ class TestPortfolio(WebTest): self.assertContains(success_result_page, "6 Downing st") self.assertContains(success_result_page, "London") + + @less_console_noise_decorator + def test_portfolio_in_session_when_multiple_portfolios_active(self): + """When multiple_portfolios flag is true and user has a portfolio, + the portfolio should be set in session.""" + self.client.force_login(self.user) + portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=portfolio_roles + ) + with override_flag("organization_feature", active=True), override_flag("multiple_portfolios", active=True): + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." + + @less_console_noise_decorator + def test_portfolio_in_session_when_organization_feature_active(self): + """When organization_feature flag is true and user has a portfolio, + the portfolio should be set in session.""" + self.client.force_login(self.user) + portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=portfolio_roles + ) + with override_flag("organization_feature", active=True): + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." \ No newline at end of file From 01f05c7fde2a9cd1b6f80a6b5e01cab476aef58c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 14:42:36 -0400 Subject: [PATCH 30/59] test registrar_middleware for multiple_portfolios --- src/registrar/tests/test_views_portfolio.py | 77 ++++++++++++++++++--- 1 file changed, 68 insertions(+), 9 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 0104eefb8..3667b1460 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -363,6 +363,69 @@ class TestPortfolio(WebTest): self.assertContains(success_result_page, "6 Downing st") self.assertContains(success_result_page, "London") + @less_console_noise_decorator + def test_portfolio_in_session_when_organization_feature_active(self): + """When organization_feature flag is true and user has a portfolio, + the portfolio should be set in session.""" + self.client.force_login(self.user) + portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=portfolio_roles + ) + with override_flag("organization_feature", active=True): + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." + + @less_console_noise_decorator + def test_portfolio_in_session_is_none_when_organization_feature_inactive(self): + """When organization_feature flag is false and user has a portfolio, + the portfolio should be set to None in session. + This test also satisfies the conidition when multiple_portfolios flag + is false and user has a portfolio, so won't add a redundant test for that.""" + self.client.force_login(self.user) + portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=portfolio_roles + ) + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + self.assertIsNone(session['portfolio']) + + @less_console_noise_decorator + def test_portfolio_in_session_is_none_when_organization_feature_active_and_no_portfolio(self): + """When organization_feature flag is true and user does not have a portfolio, + the portfolio should be set to None in session.""" + self.client.force_login(self.user) + with override_flag("organization_feature", active=True): + response = self.client.get(reverse("home")) + # Ensure that middleware processes the session + session_middleware = SessionMiddleware(lambda request: None) + session_middleware.process_request(response.wsgi_request) + response.wsgi_request.session.save() + # Access the session via the request + session = response.wsgi_request.session + # Check if the 'portfolio' session variable exists + assert 'portfolio' in session, "Portfolio session variable should exist." + # Check the value of the 'portfolio' session variable + self.assertIsNone(session['portfolio']) + @less_console_noise_decorator def test_portfolio_in_session_when_multiple_portfolios_active(self): """When multiple_portfolios flag is true and user has a portfolio, @@ -386,15 +449,11 @@ class TestPortfolio(WebTest): assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." @less_console_noise_decorator - def test_portfolio_in_session_when_organization_feature_active(self): - """When organization_feature flag is true and user has a portfolio, - the portfolio should be set in session.""" + def test_portfolio_in_session_is_none_when_multiple_portfolios_active_and_no_portfolio(self): + """When multiple_portfolios flag is true and user does not have a portfolio, + the portfolio should be set to None in session.""" self.client.force_login(self.user) - portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, roles=portfolio_roles - ) - with override_flag("organization_feature", active=True): + with override_flag("multiple_portfolios", active=True): response = self.client.get(reverse("home")) # Ensure that middleware processes the session session_middleware = SessionMiddleware(lambda request: None) @@ -405,4 +464,4 @@ class TestPortfolio(WebTest): # Check if the 'portfolio' session variable exists assert 'portfolio' in session, "Portfolio session variable should exist." # Check the value of the 'portfolio' session variable - assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." \ No newline at end of file + self.assertIsNone(session['portfolio']) From 035b4206f3f67a829287a6f707464527738f0477 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 13:07:52 -0600 Subject: [PATCH 31/59] Add test + fix perms --- src/registrar/tests/test_models.py | 65 +++++++++++++++++++++++++++ src/registrar/views/utility/mixins.py | 10 +++-- 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 3a5405fe8..22758d4f6 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1187,6 +1187,71 @@ class TestPortfolioInvitations(TestCase): self.assertEqual(self.invitation.status, PortfolioInvitation.PortfolioInvitationStatus.INVITED) +class TestUserPortfolioPermission(TestCase): + @less_console_noise_decorator + def setUp(self): + self.user, _ = User.objects.get_or_create(email="mayor@igorville.gov") + super().setUp() + + def tearDown(self): + super().tearDown() + Domain.objects.all().delete() + DomainInformation.objects.all().delete() + DomainRequest.objects.all().delete() + UserPortfolioPermission.objects.all().delete() + Portfolio.objects.all().delete() + User.objects.all().delete() + UserDomainRole.objects.all().delete() + + @less_console_noise_decorator + @override_flag("multiple_portfolios", active=True) + def test_clean_on_multiple_portfolios_when_flag_active(self): + """Ensures that a user can create multiple portfolio permission objects when the flag is enabled""" + # Create an instance of User with a portfolio but no roles or additional permissions + portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") + portfolio_2, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Motel California") + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=portfolio, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + portfolio_permission_2 = UserPortfolioPermission( + portfolio=portfolio_2, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + # Clean should pass on both of these objects + try: + portfolio_permission.clean() + portfolio_permission_2.clean() + except ValidationError as error: + self.fail(f"Raised ValidationError unexpectedly: {error}") + + @less_console_noise_decorator + @override_flag("multiple_portfolios", active=False) + def test_clean_on_creates_multiple_portfolios(self): + """Ensures that a user cannot create multiple portfolio permission objects when the flag is disabled""" + # Create an instance of User with a portfolio but no roles or additional permissions + portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") + portfolio_2, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Motel California") + portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( + portfolio=portfolio, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + portfolio_permission_2 = UserPortfolioPermission( + portfolio=portfolio_2, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + # This should work as intended + portfolio_permission.clean() + + # Test if the ValidationError is raised with the correct message + with self.assertRaises(ValidationError) as cm: + portfolio_permission_2.clean() + + portfolio_permission_2, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) + + self.assertEqual( + cm.exception.message, "Only one portfolio permission is allowed per user when multiple portfolios are disabled." + ) + + class TestUser(TestCase): """Test actions that occur on user login, test class method that controls how users get validated.""" diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index d1edfc46e..643405262 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -432,10 +432,11 @@ class PortfolioDomainsPermission(PortfolioBasePermission): The user is in self.request.user and the portfolio can be looked up from the portfolio's primary key in self.kwargs["pk"]""" - if not self.request.user.is_authenticated: + portfolio = self.request.get("portfolio") + if not self.request.user.has_domains_portfolio_permission(portfolio): return False - return self.request.user.is_org_user(self.request) + return super().has_permission() class PortfolioDomainRequestsPermission(PortfolioBasePermission): @@ -448,7 +449,8 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): The user is in self.request.user and the portfolio can be looked up from the portfolio's primary key in self.kwargs["pk"]""" - if not self.request.user.is_authenticated: + portfolio = self.request.get("portfolio") + if not self.request.user.has_domain_requests_portfolio_permission(portfolio): return False - return self.request.user.is_org_user(self.request) + return super().has_permission() From 9e04e9f5bac29b02730ab0112b562427a92ec803 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 13:08:40 -0600 Subject: [PATCH 32/59] typo --- src/registrar/views/utility/mixins.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 643405262..6f0745f41 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -432,7 +432,7 @@ class PortfolioDomainsPermission(PortfolioBasePermission): The user is in self.request.user and the portfolio can be looked up from the portfolio's primary key in self.kwargs["pk"]""" - portfolio = self.request.get("portfolio") + portfolio = self.request.session.get("portfolio") if not self.request.user.has_domains_portfolio_permission(portfolio): return False @@ -449,7 +449,7 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): The user is in self.request.user and the portfolio can be looked up from the portfolio's primary key in self.kwargs["pk"]""" - portfolio = self.request.get("portfolio") + portfolio = self.request.session.get("portfolio") if not self.request.user.has_domain_requests_portfolio_permission(portfolio): return False From 4dadb715e84a2638356eed8687969bc61f4ebf4a Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 15:03:28 -0600 Subject: [PATCH 33/59] Fix merge conflict (pt 1) --- src/registrar/tests/test_views_portfolio.py | 31 ++++++++++----------- src/registrar/views/portfolios.py | 14 ++++++---- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index c87a2f3c8..f9501aa14 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -12,7 +12,7 @@ from registrar.models import ( ) from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices -from .common import create_test_user +from .common import create_test_user, get_wsgi_request_object from waffle.testutils import override_flag from django.contrib.sessions.middleware import SessionMiddleware @@ -502,14 +502,12 @@ class TestPortfolio(WebTest): # A default organization member should not be able to see any domains self.app.set_user(self.user.username) - self.user.portfolio = self.portfolio - self.user.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_MEMBER] - self.user.save() - self.user.refresh_from_db() - - self.assertFalse(self.user.has_domains_portfolio_permission()) + permission, _ = UserPortfolioPermission.objects.get_or_create( + user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + ) response = self.app.get(reverse("no-portfolio-domains")) + self.assertFalse(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "You aren’t managing any domains.") @@ -518,25 +516,24 @@ class TestPortfolio(WebTest): self.assertEqual(response.status_code, 403) # Ensure that this user can see domains with the right permissions - self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS] - self.user.save() - self.user.refresh_from_db() - - self.assertTrue(self.user.has_domains_portfolio_permission()) + permission.additional_permissions = [UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS] + permission.save() + permission.refresh_from_db() # Test the domains page - this user should have access response = self.app.get(reverse("domains")) + self.assertTrue(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") # Test the managed domains permission - self.user.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS] - self.user.save() - self.user.refresh_from_db() - - self.assertTrue(self.user.has_domains_portfolio_permission()) + permission.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS] + permission.save() + permission.refresh_from_db() # Test the domains page - this user should have access response = self.app.get(reverse("domains")) + self.assertTrue(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") + permission.delete() diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index 436d1b913..e317d5089 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -5,6 +5,7 @@ from django.urls import reverse from django.contrib import messages from registrar.forms.portfolio import PortfolioOrgAddressForm, PortfolioSeniorOfficialForm from registrar.models import Portfolio, User +from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.views.utility.permission_views import ( PortfolioDomainRequestsPermissionView, @@ -55,14 +56,17 @@ class PortfolioNoDomainsView(NoPortfolioDomainsPermissionView, View): """Add additional context data to the template.""" # We can override the base class. This view only needs this item. context = {} - portfolio = self.request.user.portfolio if self.request and self.request.user else None + portfolio = self.request.session.get("portfolio") if portfolio: - context["portfolio_administrators"] = User.objects.filter( + admin_ids = UserPortfolioPermission.objects.filter( portfolio=portfolio, - portfolio_roles__overlap=[ + roles__overlap=[ UserPortfolioRoleChoices.ORGANIZATION_ADMIN, - ], - ) + ] + ).values_list("user__id", flat=True) + + admin_users = User.objects.filter(id__in=admin_ids) + context["portfolio_administrators"] = admin_users return context From 0858a9663f11c1f4c4f5c1dd8d71f1a160944043 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 17:08:09 -0400 Subject: [PATCH 34/59] added more tests --- src/registrar/tests/test_models.py | 42 ++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 3a5405fe8..e5ce01599 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1186,6 +1186,48 @@ class TestPortfolioInvitations(TestCase): self.assertEqual(len(roles), 1) self.assertEqual(self.invitation.status, PortfolioInvitation.PortfolioInvitationStatus.INVITED) + @less_console_noise_decorator + def test_retrieve_user_multiple_invitations(self): + """Retrieve user portfolio invitations when there are multiple and multiple_options flag true.""" + # create a 2nd portfolio and a 2nd portfolio invitation to self.user + portfolio2, _ = Portfolio.objects.get_or_create(creator=self.user2, organization_name="Take It Easy") + PortfolioInvitation.objects.get_or_create( + email=self.email, + portfolio=portfolio2, + portfolio_roles=[self.portfolio_role_base, self.portfolio_role_admin], + portfolio_additional_permissions=[self.portfolio_permission_1, self.portfolio_permission_2], + ) + with override_flag("multiple_portfolios", active=True): + self.user.check_portfolio_invitations_on_login() + self.user.refresh_from_db() + roles = UserPortfolioPermission.objects.filter(user=self.user) + self.assertEqual(len(roles), 2) + updated_invitation1, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=self.portfolio) + self.assertEqual(updated_invitation1.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) + updated_invitation2, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=portfolio2) + self.assertEqual(updated_invitation2.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) + + @less_console_noise_decorator + def test_retrieve_user_multiple_invitations_when_multiple_portfolios_inactive(self): + """Attempt to retrieve user portfolio invitations when there are multiple + but multiple_portfolios flag set to False""" + # create a 2nd portfolio and a 2nd portfolio invitation to self.user + portfolio2, _ = Portfolio.objects.get_or_create(creator=self.user2, organization_name="Take It Easy") + PortfolioInvitation.objects.get_or_create( + email=self.email, + portfolio=portfolio2, + portfolio_roles=[self.portfolio_role_base, self.portfolio_role_admin], + portfolio_additional_permissions=[self.portfolio_permission_1, self.portfolio_permission_2], + ) + self.user.check_portfolio_invitations_on_login() + self.user.refresh_from_db() + roles = UserPortfolioPermission.objects.filter(user=self.user) + self.assertEqual(len(roles), 1) + updated_invitation1, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=self.portfolio) + self.assertEqual(updated_invitation1.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) + updated_invitation2, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=portfolio2) + self.assertEqual(updated_invitation2.status, PortfolioInvitation.PortfolioInvitationStatus.INVITED) + class TestUser(TestCase): """Test actions that occur on user login, From e866bcb2fbbf6d6dcd95e2456497d203cc81079f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 21 Aug 2024 15:12:37 -0600 Subject: [PATCH 35/59] Fix test (still needs refinement) --- src/registrar/tests/test_views_portfolio.py | 23 +++++++++++---------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f9501aa14..3a973b7c7 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -500,19 +500,20 @@ class TestPortfolio(WebTest): if they do not have the right permissions. """ - # A default organization member should not be able to see any domains - self.app.set_user(self.user.username) permission, _ = UserPortfolioPermission.objects.get_or_create( user=self.user, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] ) - response = self.app.get(reverse("no-portfolio-domains")) - self.assertFalse(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) + # A default organization member should not be able to see any domains + self.client.force_login(self.user) + response = self.client.get(reverse("home"), follow=True) + + self.assertFalse(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) - self.assertContains(response, "You aren’t managing any domains.") + self.assertContains(response, "You aren't managing any domains.") # Test the domains page - this user should not have access - response = self.app.get(reverse("domains"), expect_errors=True) + response = self.client.get(reverse("domains")) self.assertEqual(response.status_code, 403) # Ensure that this user can see domains with the right permissions @@ -521,19 +522,19 @@ class TestPortfolio(WebTest): permission.refresh_from_db() # Test the domains page - this user should have access - response = self.app.get(reverse("domains")) - self.assertTrue(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) + response = self.client.get(reverse("domains")) + self.assertTrue(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") # Test the managed domains permission - permission.portfolio_additional_permissions = [UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS] + permission.additional_permissions = [UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS] permission.save() permission.refresh_from_db() # Test the domains page - this user should have access - response = self.app.get(reverse("domains")) - self.assertTrue(self.user.has_domains_portfolio_permission(response.request.get("portfolio"))) + response = self.client.get(reverse("domains")) + self.assertTrue(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") permission.delete() From ff86edbc3538c57c99b1e770dbeb5035c6f7491f Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 17:18:00 -0400 Subject: [PATCH 36/59] more updates to test and for linting --- src/registrar/tests/test_models.py | 13 +++++--- src/registrar/tests/test_views_domain.py | 2 +- src/registrar/tests/test_views_portfolio.py | 36 +++++++++------------ src/registrar/views/portfolios.py | 2 +- 4 files changed, 25 insertions(+), 28 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 6c67aa31f..227548b61 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1202,7 +1202,9 @@ class TestPortfolioInvitations(TestCase): self.user.refresh_from_db() roles = UserPortfolioPermission.objects.filter(user=self.user) self.assertEqual(len(roles), 2) - updated_invitation1, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=self.portfolio) + updated_invitation1, _ = PortfolioInvitation.objects.get_or_create( + email=self.email, portfolio=self.portfolio + ) self.assertEqual(updated_invitation1.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) updated_invitation2, _ = PortfolioInvitation.objects.get_or_create(email=self.email, portfolio=portfolio2) self.assertEqual(updated_invitation2.status, PortfolioInvitation.PortfolioInvitationStatus.RETRIEVED) @@ -1244,7 +1246,7 @@ class TestUserPortfolioPermission(TestCase): Portfolio.objects.all().delete() User.objects.all().delete() UserDomainRole.objects.all().delete() - + @less_console_noise_decorator @override_flag("multiple_portfolios", active=True) def test_clean_on_multiple_portfolios_when_flag_active(self): @@ -1279,18 +1281,19 @@ class TestUserPortfolioPermission(TestCase): portfolio_permission_2 = UserPortfolioPermission( portfolio=portfolio_2, user=self.user, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] ) - + # This should work as intended portfolio_permission.clean() # Test if the ValidationError is raised with the correct message with self.assertRaises(ValidationError) as cm: portfolio_permission_2.clean() - + portfolio_permission_2, _ = UserPortfolioPermission.objects.get_or_create(portfolio=portfolio, user=self.user) self.assertEqual( - cm.exception.message, "Only one portfolio permission is allowed per user when multiple portfolios are disabled." + cm.exception.message, + "Only one portfolio permission is allowed per user when multiple portfolios are disabled.", ) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index e457f1b66..5ac24fd69 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -6,7 +6,7 @@ from django.urls import reverse from django.contrib.auth import get_user_model from waffle.testutils import override_flag from api.tests.common import less_console_noise_decorator -from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices +from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from .common import MockEppLib, MockSESClient, create_user # type: ignore from django_webtest import WebTest # type: ignore import boto3_mocking # type: ignore diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index f9501aa14..277f0520a 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -12,7 +12,7 @@ from registrar.models import ( ) from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices -from .common import create_test_user, get_wsgi_request_object +from .common import create_test_user from waffle.testutils import override_flag from django.contrib.sessions.middleware import SessionMiddleware @@ -396,9 +396,7 @@ class TestPortfolio(WebTest): the portfolio should be set in session.""" self.client.force_login(self.user) portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, roles=portfolio_roles - ) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, roles=portfolio_roles) with override_flag("organization_feature", active=True): response = self.client.get(reverse("home")) # Ensure that middleware processes the session @@ -408,9 +406,9 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." + self.assertEqual(session["portfolio"], self.portfolio, "Portfolio session variable has the wrong value.") @less_console_noise_decorator def test_portfolio_in_session_is_none_when_organization_feature_inactive(self): @@ -420,9 +418,7 @@ class TestPortfolio(WebTest): is false and user has a portfolio, so won't add a redundant test for that.""" self.client.force_login(self.user) portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, roles=portfolio_roles - ) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, roles=portfolio_roles) response = self.client.get(reverse("home")) # Ensure that middleware processes the session session_middleware = SessionMiddleware(lambda request: None) @@ -431,9 +427,9 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - self.assertIsNone(session['portfolio']) + self.assertIsNone(session["portfolio"]) @less_console_noise_decorator def test_portfolio_in_session_is_none_when_organization_feature_active_and_no_portfolio(self): @@ -449,19 +445,17 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - self.assertIsNone(session['portfolio']) - + self.assertIsNone(session["portfolio"]) + @less_console_noise_decorator def test_portfolio_in_session_when_multiple_portfolios_active(self): """When multiple_portfolios flag is true and user has a portfolio, the portfolio should be set in session.""" self.client.force_login(self.user) portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - UserPortfolioPermission.objects.get_or_create( - user=self.user, portfolio=self.portfolio, roles=portfolio_roles - ) + UserPortfolioPermission.objects.get_or_create(user=self.user, portfolio=self.portfolio, roles=portfolio_roles) with override_flag("organization_feature", active=True), override_flag("multiple_portfolios", active=True): response = self.client.get(reverse("home")) # Ensure that middleware processes the session @@ -471,9 +465,9 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - assert session['portfolio'] == self.portfolio, "Portfolio session variable has the wrong value." + self.assertEqual(session["portfolio"], self.portfolio, "Portfolio session variable has the wrong value.") @less_console_noise_decorator def test_portfolio_in_session_is_none_when_multiple_portfolios_active_and_no_portfolio(self): @@ -489,9 +483,9 @@ class TestPortfolio(WebTest): # Access the session via the request session = response.wsgi_request.session # Check if the 'portfolio' session variable exists - assert 'portfolio' in session, "Portfolio session variable should exist." + self.assertIn("portfolio", session, "Portfolio session variable should exist.") # Check the value of the 'portfolio' session variable - self.assertIsNone(session['portfolio']) + self.assertIsNone(session["portfolio"]) @less_console_noise_decorator @override_flag("organization_feature", active=True) diff --git a/src/registrar/views/portfolios.py b/src/registrar/views/portfolios.py index e317d5089..0232b50d7 100644 --- a/src/registrar/views/portfolios.py +++ b/src/registrar/views/portfolios.py @@ -62,7 +62,7 @@ class PortfolioNoDomainsView(NoPortfolioDomainsPermissionView, View): portfolio=portfolio, roles__overlap=[ UserPortfolioRoleChoices.ORGANIZATION_ADMIN, - ] + ], ).values_list("user__id", flat=True) admin_users = User.objects.filter(id__in=admin_ids) From 833b698bf28655a71e1eaf4ad209dc1b9973eac4 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 17:52:02 -0400 Subject: [PATCH 37/59] adding request to a few flag_is_active instances --- src/registrar/models/user.py | 7 ++++++- src/registrar/models/user_portfolio_permission.py | 6 +++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 6aed8195c..03d11e5bd 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -3,6 +3,7 @@ import logging from django.contrib.auth.models import AbstractUser from django.db import models from django.db.models import Q +from django.http import HttpRequest from registrar.models import DomainInformation, UserDomainRole from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices @@ -358,8 +359,12 @@ class User(AbstractUser): for invitation in PortfolioInvitation.objects.filter( email__iexact=self.email, status=PortfolioInvitation.PortfolioInvitationStatus.INVITED ): + # need to create a bogus request and assign user to it, in order to pass request + # to flag_is_active + request = HttpRequest() + request.user = self only_single_portfolio = ( - not flag_is_active(None, "multiple_portfolios") and self.get_first_portfolio() is None + not flag_is_active(request, "multiple_portfolios") and self.get_first_portfolio() is None ) if only_single_portfolio or flag_is_active(None, "multiple_portfolios"): try: diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index a3460a651..abbd20afb 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -1,5 +1,6 @@ from django.db import models from django.forms import ValidationError +from django.http import HttpRequest from waffle import flag_is_active from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from .utility.time_stamped_model import TimeStampedModel @@ -99,7 +100,10 @@ class UserPortfolioPermission(TimeStampedModel): # Check if a user is set without accessing the related object. has_user = bool(self.user_id) - if not flag_is_active(None, "multiple_portfolios") and self.pk is None and has_user: + # Have to create a bogus request to set the user and pass to flag_is_active + request = HttpRequest() + request.user = self.user + if not flag_is_active(request, "multiple_portfolios") and self.pk is None and has_user: existing_permissions = UserPortfolioPermission.objects.filter(user=self.user) if existing_permissions.exists(): raise ValidationError( From 76fef5303b1e24460fb1a282fe7662b4c38f5df6 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 21 Aug 2024 18:41:14 -0400 Subject: [PATCH 38/59] removed pseudo apostrophe --- src/registrar/tests/test_views_portfolio.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 01cd84743..42cf82fa4 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -504,7 +504,7 @@ class TestPortfolio(WebTest): self.assertFalse(self.user.has_domains_portfolio_permission(response.wsgi_request.session.get("portfolio"))) self.assertEqual(response.status_code, 200) - self.assertContains(response, "You aren't managing any domains.") + self.assertContains(response, "You aren") # Test the domains page - this user should not have access response = self.client.get(reverse("domains")) From bef49a85c16de5fc9f2640a9c2ee3f1c37e25478 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 22 Aug 2024 08:33:03 -0600 Subject: [PATCH 39/59] Cleanup --- .../models/user_portfolio_permission.py | 10 +++++----- src/registrar/tests/test_models.py | 16 ---------------- 2 files changed, 5 insertions(+), 21 deletions(-) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index abbd20afb..c2cdc61ad 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -100,12 +100,12 @@ class UserPortfolioPermission(TimeStampedModel): # Check if a user is set without accessing the related object. has_user = bool(self.user_id) - # Have to create a bogus request to set the user and pass to flag_is_active - request = HttpRequest() - request.user = self.user - if not flag_is_active(request, "multiple_portfolios") and self.pk is None and has_user: + if self.pk is None and has_user: + # Have to create a bogus request to set the user and pass to flag_is_active + request = HttpRequest() + request.user = self.user existing_permissions = UserPortfolioPermission.objects.filter(user=self.user) - if existing_permissions.exists(): + if not flag_is_active(request, "multiple_portfolios") and existing_permissions.exists(): raise ValidationError( "Only one portfolio permission is allowed per user when multiple portfolios are disabled." ) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 227548b61..9f55fced1 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1483,10 +1483,6 @@ class TestUser(TestCase): portfolio, _ = Portfolio.objects.get_or_create(creator=self.user, organization_name="Hotel California") - # TODO - uncomment this when we just pass request to these functions - # request = get_wsgi_request_object(self.client, self.user) - # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) @@ -1499,10 +1495,6 @@ class TestUser(TestCase): additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS], ) - # TODO - uncomment this when we just pass request to these functions - # request = get_wsgi_request_object(self.client, self.user) - # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) @@ -1513,10 +1505,6 @@ class TestUser(TestCase): portfolio_permission.save() portfolio_permission.refresh_from_db() - # TODO - uncomment this when we just pass request to these functions - # request = get_wsgi_request_object(self.client, self.user) - # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) @@ -1525,10 +1513,6 @@ class TestUser(TestCase): UserDomainRole.objects.get_or_create(user=self.user, domain=self.domain, role=UserDomainRole.Roles.MANAGER) - # TODO - uncomment this when we just pass request to these functions - # request = get_wsgi_request_object(self.client, self.user) - # user_can_view_all_domains = self.user.has_domains_portfolio_permission(request) - # user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(request) user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) user_can_view_all_requests = self.user.has_domain_requests_portfolio_permission(portfolio) From 62ac4757ec1b0f41b05d87d4d5cd2e3baa503f40 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 22 Aug 2024 16:25:24 -0500 Subject: [PATCH 40/59] review changes --- src/registrar/assets/js/get-gov.js | 4 +-- .../commands/populate_domain_request_dates.py | 36 +++++++++---------- ...0119_add_domainrequest_submission_dates.py | 2 +- src/registrar/models/domain_request.py | 8 ++--- 4 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/registrar/assets/js/get-gov.js b/src/registrar/assets/js/get-gov.js index f3b41eb51..70659b009 100644 --- a/src/registrar/assets/js/get-gov.js +++ b/src/registrar/assets/js/get-gov.js @@ -1599,7 +1599,7 @@ document.addEventListener('DOMContentLoaded', function() { const domainName = request.requested_domain ? request.requested_domain : `New domain request
(${utcDateString(request.created_at)})`; const actionUrl = request.action_url; const actionLabel = request.action_label; - const submissionDate = request.submission_date ? new Date(request.submission_date).toLocaleDateString('en-US', options) : `Not submitted`; + const submissionDate = request.last_submitted_date ? new Date(request.last_submitted_date).toLocaleDateString('en-US', options) : `Not submitted`; // Even if the request is not deletable, we may need this empty string for the td if the deletable column is displayed let modalTrigger = ''; @@ -1699,7 +1699,7 @@ document.addEventListener('DOMContentLoaded', function() { ${domainName} - + ${submissionDate} diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index 90fc06dcf..19d8e4f00 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -17,26 +17,26 @@ class Command(BaseCommand, PopulateScriptTemplate): def update_record(self, record: DomainRequest): """Defines how we update the first_submitted_date and last_status_update fields""" - try: - # Retrieve and order audit log entries by timestamp in descending order - audit_log_entries = LogEntry.objects.filter(object_pk=record.pk).order_by("-timestamp") + + # Retrieve and order audit log entries by timestamp in descending order + audit_log_entries = LogEntry.objects.filter(object_pk=record.pk).order_by("-timestamp") - # Loop through logs in descending order to find most recent status change - for log_entry in audit_log_entries: - if "status" in log_entry.changes: - record.last_status_update = log_entry.timestamp.date() - break - - # Loop through logs in ascending order to find first submission - for log_entry in audit_log_entries.reverse(): - if log_entry.changes_dict['status'](1) == 'Submitted': - record.first_submitted_date = log_entry.timestamp.date() + # Loop through logs in descending order to find most recent status change + for log_entry in audit_log_entries: + if 'status' in LogEntry.changes_dict: + record.last_status_update = log_entry.timestamp.date() + break - except ObjectDoesNotExist as e: - logger.error(f"Object with object_pk {record.pk} does not exist: {e}") - except Exception as e: - logger.error(f"An error occurred during update_record: {e}") + # Loop through logs in ascending order to find first submission + for log_entry in audit_log_entries.reverse(): + if log_entry.changes_dict['status'](1) == 'Submitted': + record.first_submitted_date = log_entry.timestamp.date() + break logger.info( - f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}{TerminalColors.OKCYAN}, last status update:" f"{record.last_status_update}{TerminalColors.OKCYAN}" + f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}{TerminalColors.OKCYAN}, last status update:" f"{record.last_status_update}{TerminalColors.ENDC}" ) + + def should_skip_record(self, record) -> bool: + # make sure the record had some kind of history + return LogEntry.objects.filter(object_pk=record.pk).exists() diff --git a/src/registrar/migrations/0119_add_domainrequest_submission_dates.py b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py index 0b94e1257..ea209626e 100644 --- a/src/registrar/migrations/0119_add_domainrequest_submission_dates.py +++ b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py @@ -35,7 +35,7 @@ class Migration(migrations.Migration): field=models.DateField( blank=True, default=None, - help_text="Date of last status updated", + help_text="Date of the last status update", null=True, verbose_name="last updated on", ), diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 74d275d95..09f200793 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -563,7 +563,7 @@ class DomainRequest(TimeStampedModel): help_text="Acknowledged .gov acceptable use policy", ) - # initial submission date records when domain request was first submitted + # Records when the domain request was first submitted first_submitted_date = models.DateField( null=True, blank=True, @@ -572,7 +572,7 @@ class DomainRequest(TimeStampedModel): help_text="Date initially submitted", ) - # last submission date records when domain request was last submitted + # Records when domain request was last submitted last_submitted_date = models.DateField( null=True, blank=True, @@ -581,13 +581,13 @@ class DomainRequest(TimeStampedModel): help_text="Date last submitted", ) - # last status update records when domain request status was last updated by an admin or analyst + # Records when domain request status was last updated by an admin or analyst last_status_update = models.DateField( null=True, blank=True, default=None, verbose_name="last updated on", - help_text="Date of last status updated", + help_text="Date of the last status update", ) notes = models.TextField( null=True, From 8a9971ac79bb08a934e269674a28993d5470b608 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 23 Aug 2024 17:50:24 -0400 Subject: [PATCH 41/59] initial implementation --- src/registrar/assets/js/get-gov-admin.js | 51 +++++++++++++++++++ src/registrar/config/urls.py | 10 +++- src/registrar/models/portfolio.py | 16 ++++-- .../django/admin/portfolio_change_form.html | 2 + src/registrar/views/utility/api_views.py | 33 ++++++++++++ 5 files changed, 107 insertions(+), 5 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 01c93abf6..11e3cae69 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -837,6 +837,27 @@ function initializeWidgetOnList(list, parentId) { // If we can update the contact information, it'll be shown again. hideElement(contactList.parentElement); + let federalPortfolioApi = document.getElementById("federal_and_portfolio_types_from_agency_json_url").value; + fetch(`${federalPortfolioApi}?organization_type=${organizationType.value}&agency_name=${selectedText}`) + .then(response => { + const statusCode = response.status; + return response.json().then(data => ({ statusCode, data })); + }) + .then(({ statusCode, data }) => { + if (data.error) { + console.error("Error in AJAX call: " + data.error); + return; + } + + let federal_type = data.federal_type; + let portfolio_type = data.portfolio_type; + console.log("portfolio type: " + portfolio_type); + console.log("federal type: " + federal_type); + updateFederalType(data.federal_type); + updatePortfolioType(data.portfolio_type); + }) + .catch(error => console.error("Error fetching federal and portfolio types: ", error)); + let seniorOfficialApi = document.getElementById("senior_official_from_agency_json_url").value; fetch(`${seniorOfficialApi}?agency_name=${selectedText}`) .then(response => { @@ -879,6 +900,7 @@ function initializeWidgetOnList(list, parentId) { } }) .catch(error => console.error("Error fetching senior official: ", error)); + } function handleStateTerritoryChange(stateTerritory, urbanizationField) { @@ -890,6 +912,35 @@ function initializeWidgetOnList(list, parentId) { } } + function updatePortfolioType(portfolioType) { + console.log("attempting to update portfolioType"); + // Find the div with class 'field-portfolio_type' + const portfolioTypeDiv = document.querySelector('.field-portfolio_type'); + if (portfolioTypeDiv) { + console.log("found portfoliotype"); + // Find the nested div with class 'readonly' inside 'field-portfolio_type' + const readonlyDiv = portfolioTypeDiv.querySelector('.readonly'); + if (readonlyDiv) { + console.log("found readonly div"); + // Update the text content of the readonly div + readonlyDiv.textContent = portfolioType !== null ? portfolioType : '-'; + } + } + } + + function updateFederalType(federalType) { + // Find the div with class 'field-federal_type' + const federalTypeDiv = document.querySelector('.field-federal_type'); + if (federalTypeDiv) { + // Find the nested div with class 'readonly' inside 'field-federal_type' + const readonlyDiv = federalTypeDiv.querySelector('.readonly'); + if (readonlyDiv) { + // Update the text content of the readonly div + readonlyDiv.textContent = federalType !== null ? federalType : '-'; + } + } + } + function updateContactInfo(data) { if (!contactList) return; diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 413449896..5e58cf740 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -24,7 +24,10 @@ from registrar.views.report_views import ( from registrar.views.domain_request import Step from registrar.views.domain_requests_json import get_domain_requests_json -from registrar.views.utility.api_views import get_senior_official_from_federal_agency_json +from registrar.views.utility.api_views import ( + get_senior_official_from_federal_agency_json, + get_federal_and_portfolio_types_from_federal_agency_json +) from registrar.views.domains_json import get_domains_json from registrar.views.utility import always_404 from api.views import available, get_current_federal, get_current_full @@ -139,6 +142,11 @@ urlpatterns = [ get_senior_official_from_federal_agency_json, name="get-senior-official-from-federal-agency-json", ), + path( + "admin/api/get-federal-and-portfolio-types-from-federal-agency-json/", + get_federal_and_portfolio_types_from_federal_agency_json, + name="get-federal-and-portfolio-types-from-federal-agency-json", + ), path("admin/", admin.site.urls), path( "reports/export_data_type_user/", diff --git a/src/registrar/models/portfolio.py b/src/registrar/models/portfolio.py index 0f9904c31..b9b0d4a22 100644 --- a/src/registrar/models/portfolio.py +++ b/src/registrar/models/portfolio.py @@ -131,9 +131,13 @@ class Portfolio(TimeStampedModel): Returns a combination of organization_type / federal_type, seperated by ' - '. If no federal_type is found, we just return the org type. """ - org_type_label = self.OrganizationChoices.get_org_label(self.organization_type) - agency_type_label = BranchChoices.get_branch_label(self.federal_type) - if self.organization_type == self.OrganizationChoices.FEDERAL and agency_type_label: + return self.get_portfolio_type(self.organization_type, self.federal_type) + + @classmethod + def get_portfolio_type(cls, organization_type, federal_type): + org_type_label = cls.OrganizationChoices.get_org_label(organization_type) + agency_type_label = BranchChoices.get_branch_label(federal_type) + if organization_type == cls.OrganizationChoices.FEDERAL and agency_type_label: return " - ".join([org_type_label, agency_type_label]) else: return org_type_label @@ -141,8 +145,12 @@ class Portfolio(TimeStampedModel): @property def federal_type(self): """Returns the federal_type value on the underlying federal_agency field""" - return self.federal_agency.federal_type if self.federal_agency else None + return self.get_federal_type(self.federal_agency) + @classmethod + def get_federal_type(cls, federal_agency): + return federal_agency.federal_type if federal_agency else None + # == Getters for domains == # def get_domains(self): """Returns all DomainInformations associated with this portfolio""" diff --git a/src/registrar/templates/django/admin/portfolio_change_form.html b/src/registrar/templates/django/admin/portfolio_change_form.html index 9d59aae42..4eb941340 100644 --- a/src/registrar/templates/django/admin/portfolio_change_form.html +++ b/src/registrar/templates/django/admin/portfolio_change_form.html @@ -5,6 +5,8 @@ {% comment %} Stores the json endpoint in a url for easier access {% endcomment %} {% url 'get-senior-official-from-federal-agency-json' as url %} + {% url 'get-federal-and-portfolio-types-from-federal-agency-json' as url %} + {{ block.super }} {% endblock content %} diff --git a/src/registrar/views/utility/api_views.py b/src/registrar/views/utility/api_views.py index 2c9414d1d..e67a1974c 100644 --- a/src/registrar/views/utility/api_views.py +++ b/src/registrar/views/utility/api_views.py @@ -5,6 +5,9 @@ from registrar.models import FederalAgency, SeniorOfficial from django.contrib.admin.views.decorators import staff_member_required from django.contrib.auth.decorators import login_required +from registrar.models.portfolio import Portfolio +from registrar.utility.constants import BranchChoices + logger = logging.getLogger(__name__) @@ -34,3 +37,33 @@ def get_senior_official_from_federal_agency_json(request): return JsonResponse(so_dict) else: return JsonResponse({"error": "Senior Official not found"}, status=404) + +@login_required +@staff_member_required +def get_federal_and_portfolio_types_from_federal_agency_json(request): + """Returns specific portfolio information as a JSON. Request must have + both agency_name and organization_type.""" + + # This API is only accessible to admins and analysts + superuser_perm = request.user.has_perm("registrar.full_access_permission") + analyst_perm = request.user.has_perm("registrar.analyst_access_permission") + if not request.user.is_authenticated or not any([analyst_perm, superuser_perm]): + return JsonResponse({"error": "You do not have access to this resource"}, status=403) + + federal_type = None + portfolio_type = None + + agency_name = request.GET.get("agency_name") + organization_type = request.GET.get("organization_type") + agency = FederalAgency.objects.filter(agency=agency_name).first() + if agency: + federal_type = Portfolio.get_federal_type(agency) + portfolio_type = Portfolio.get_portfolio_type(organization_type, federal_type) + federal_type = BranchChoices.get_branch_label(federal_type) if federal_type else "-" + + response_data = { + 'portfolio_type': portfolio_type, + 'federal_type': federal_type, + } + + return JsonResponse(response_data) \ No newline at end of file From ef7739dc556b9e9c7b5c046934e4ee44a5bf733e Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 23 Aug 2024 17:52:57 -0400 Subject: [PATCH 42/59] lint --- src/registrar/config/urls.py | 2 +- src/registrar/models/portfolio.py | 2 +- src/registrar/views/utility/api_views.py | 11 ++++++----- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 5e58cf740..19fa99809 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -26,7 +26,7 @@ from registrar.views.domain_request import Step from registrar.views.domain_requests_json import get_domain_requests_json from registrar.views.utility.api_views import ( get_senior_official_from_federal_agency_json, - get_federal_and_portfolio_types_from_federal_agency_json + get_federal_and_portfolio_types_from_federal_agency_json, ) from registrar.views.domains_json import get_domains_json from registrar.views.utility import always_404 diff --git a/src/registrar/models/portfolio.py b/src/registrar/models/portfolio.py index b9b0d4a22..fadcf8cac 100644 --- a/src/registrar/models/portfolio.py +++ b/src/registrar/models/portfolio.py @@ -150,7 +150,7 @@ class Portfolio(TimeStampedModel): @classmethod def get_federal_type(cls, federal_agency): return federal_agency.federal_type if federal_agency else None - + # == Getters for domains == # def get_domains(self): """Returns all DomainInformations associated with this portfolio""" diff --git a/src/registrar/views/utility/api_views.py b/src/registrar/views/utility/api_views.py index e67a1974c..97eb7e86c 100644 --- a/src/registrar/views/utility/api_views.py +++ b/src/registrar/views/utility/api_views.py @@ -37,7 +37,8 @@ def get_senior_official_from_federal_agency_json(request): return JsonResponse(so_dict) else: return JsonResponse({"error": "Senior Official not found"}, status=404) - + + @login_required @staff_member_required def get_federal_and_portfolio_types_from_federal_agency_json(request): @@ -62,8 +63,8 @@ def get_federal_and_portfolio_types_from_federal_agency_json(request): federal_type = BranchChoices.get_branch_label(federal_type) if federal_type else "-" response_data = { - 'portfolio_type': portfolio_type, - 'federal_type': federal_type, + "portfolio_type": portfolio_type, + "federal_type": federal_type, } - - return JsonResponse(response_data) \ No newline at end of file + + return JsonResponse(response_data) From 31285da3956043a0a924ef4ea2a30d785a6a1c1a Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 23 Aug 2024 18:16:01 -0400 Subject: [PATCH 43/59] added test code --- src/registrar/tests/test_api.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/registrar/tests/test_api.py b/src/registrar/tests/test_api.py index 0025bc902..bd96f3e24 100644 --- a/src/registrar/tests/test_api.py +++ b/src/registrar/tests/test_api.py @@ -4,6 +4,8 @@ from registrar.models import FederalAgency, SeniorOfficial, User from django.contrib.auth import get_user_model from registrar.tests.common import create_superuser, create_user +from api.tests.common import less_console_noise_decorator + class GetSeniorOfficialJsonTest(TestCase): def setUp(self): @@ -65,3 +67,32 @@ class GetSeniorOfficialJsonTest(TestCase): self.assertEqual(response.status_code, 404) data = response.json() self.assertEqual(data["error"], "Senior Official not found") + + +class GetFederalPortfolioTypeJsonTest(TestCase): + def setUp(self): + self.client = Client() + p = "password" + self.user = get_user_model().objects.create_user(username="testuser", password=p) + + self.superuser = create_superuser() + self.analyst_user = create_user() + + self.agency = FederalAgency.objects.create(agency="Test Agency", federal_type="judicial") + + self.api_url = reverse("get-federal-and-portfolio-types-from-federal-agency-json") + + def tearDown(self): + User.objects.all().delete() + FederalAgency.objects.all().delete() + + @less_console_noise_decorator + def test_get_federal_and_portfolio_types_json_authenticated_superuser(self): + """Test that a superuser can fetch the federal and portfolio types.""" + p = "adminpass" + self.client.login(username="superuser", password=p) + response = self.client.get(self.api_url, {"agency_name": "Test Agency", "organization_type": "federal"}) + self.assertEqual(response.status_code, 200) + data = response.json() + self.assertEqual(data["federal_type"], "Judicial") + self.assertEqual(data["portfolio_type"], "Federal - Judicial") From 6091ff76c3ad71aaaa130842d5cdbdf0c24dd21c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 26 Aug 2024 11:42:10 -0400 Subject: [PATCH 44/59] cleanup --- src/registrar/assets/js/get-gov-admin.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 11e3cae69..98a2395db 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -837,6 +837,8 @@ function initializeWidgetOnList(list, parentId) { // If we can update the contact information, it'll be shown again. hideElement(contactList.parentElement); + // Determine if any changes are necessary to the display of portfolio type or federal type + // based on changes to the Federal Agency let federalPortfolioApi = document.getElementById("federal_and_portfolio_types_from_agency_json_url").value; fetch(`${federalPortfolioApi}?organization_type=${organizationType.value}&agency_name=${selectedText}`) .then(response => { @@ -851,8 +853,6 @@ function initializeWidgetOnList(list, parentId) { let federal_type = data.federal_type; let portfolio_type = data.portfolio_type; - console.log("portfolio type: " + portfolio_type); - console.log("federal type: " + federal_type); updateFederalType(data.federal_type); updatePortfolioType(data.portfolio_type); }) @@ -912,22 +912,25 @@ function initializeWidgetOnList(list, parentId) { } } + /** + * Dynamically update the portfolio type text in the dom to portfolioType + */ function updatePortfolioType(portfolioType) { - console.log("attempting to update portfolioType"); // Find the div with class 'field-portfolio_type' const portfolioTypeDiv = document.querySelector('.field-portfolio_type'); if (portfolioTypeDiv) { - console.log("found portfoliotype"); // Find the nested div with class 'readonly' inside 'field-portfolio_type' const readonlyDiv = portfolioTypeDiv.querySelector('.readonly'); if (readonlyDiv) { - console.log("found readonly div"); // Update the text content of the readonly div readonlyDiv.textContent = portfolioType !== null ? portfolioType : '-'; } } } + /** + * Dynamically update the federal type text in the dom to federalType + */ function updateFederalType(federalType) { // Find the div with class 'field-federal_type' const federalTypeDiv = document.querySelector('.field-federal_type'); From 497837b09882731c121adbba88110d6155253e66 Mon Sep 17 00:00:00 2001 From: Matt-Spence Date: Mon, 26 Aug 2024 11:18:43 -0500 Subject: [PATCH 45/59] Update src/registrar/management/commands/populate_domain_request_dates.py Co-authored-by: zandercymatics <141044360+zandercymatics@users.noreply.github.com> --- .../management/commands/populate_domain_request_dates.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index 19d8e4f00..fe471fc9d 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -34,7 +34,7 @@ class Command(BaseCommand, PopulateScriptTemplate): break logger.info( - f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}{TerminalColors.OKCYAN}, last status update:" f"{record.last_status_update}{TerminalColors.ENDC}" + f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}, last status update:" f"{record.last_status_update}{TerminalColors.ENDC}" ) def should_skip_record(self, record) -> bool: From f7b2e38bba21c91cc46b39da26464800cdf72ac9 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Mon, 26 Aug 2024 11:54:08 -0500 Subject: [PATCH 46/59] linter fixes --- .../commands/populate_domain_request_dates.py | 11 +++++++---- src/registrar/models/domain_request.py | 2 +- src/registrar/tests/test_views_requests_json.py | 6 ++++-- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index fe471fc9d..e20e35909 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -3,7 +3,6 @@ from django.core.management import BaseCommand from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors from registrar.models import DomainRequest from auditlog.models import LogEntry -from django.core.exceptions import ObjectDoesNotExist logger = logging.getLogger(__name__) @@ -12,12 +11,13 @@ class Command(BaseCommand, PopulateScriptTemplate): help = "Loops through each domain request object and populates the last_status_update and first_submitted_date" def handle(self, **kwargs): - """Loops through each DomainRequest object and populates its last_status_update and first_submitted_date values""" + """Loops through each DomainRequest object and populates + its last_status_update and first_submitted_date values""" self.mass_update_records(DomainRequest, None, ["last_status_update", "first_submitted_date"]) def update_record(self, record: DomainRequest): """Defines how we update the first_submitted_date and last_status_update fields""" - + # Retrieve and order audit log entries by timestamp in descending order audit_log_entries = LogEntry.objects.filter(object_pk=record.pk).order_by("-timestamp") @@ -34,7 +34,10 @@ class Command(BaseCommand, PopulateScriptTemplate): break logger.info( - f"{TerminalColors.OKCYAN}Updating {record} => first submitted date: " f"{record.first_submitted_date}, last status update:" f"{record.last_status_update}{TerminalColors.ENDC}" + f"""{TerminalColors.OKCYAN}Updating {record} => + first submitted date: {record.first_submitted_date}, + last status update: {record.last_status_update}{TerminalColors.ENDC} + """ ) def should_skip_record(self, record) -> bool: diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 09f200793..7ee80e43a 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -823,7 +823,7 @@ class DomainRequest(TimeStampedModel): if not DraftDomain.string_could_be_domain(self.requested_domain.name): raise ValueError("Requested domain is not a valid domain name.") - # if the domain has not been submitted before this must be the first time + # if the domain has not been submitted before this must be the first time if not self.first_submitted_date: self.first_submitted_date = timezone.now().date() diff --git a/src/registrar/tests/test_views_requests_json.py b/src/registrar/tests/test_views_requests_json.py index c140b7e44..fd44bdc31 100644 --- a/src/registrar/tests/test_views_requests_json.py +++ b/src/registrar/tests/test_views_requests_json.py @@ -287,7 +287,8 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_sorting(self): """test that sorting works properly on the result set""" - response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"}) + response = self.app.get(reverse("get_domain_requests_json"), + {"sort_by": "last_submitted_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json @@ -306,7 +307,8 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_filter_approved_excluded(self): """test that approved requests are excluded from result set.""" # sort in reverse chronological order of submission date, since most recent request is approved - response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"}) + response = self.app.get(reverse("get_domain_requests_json"), + {"sort_by": "last_submitted_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json From b2a464668b5c475c4a263a0f745dfe49679ceed3 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 27 Aug 2024 11:44:30 -0400 Subject: [PATCH 47/59] update from merge --- src/registrar/admin.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 57c8cdfaf..d1ffda23a 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -2982,6 +2982,7 @@ class PortfolioAdmin(ListHeaderAdmin): "domain_requests", "suborganizations", "portfolio_type", + "creator", ] def federal_type(self, obj: models.Portfolio): From 89c2e4543a3d37b64306b44829c13815e06f305f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Aug 2024 12:27:35 -0600 Subject: [PATCH 48/59] Update src/registrar/models/user.py Co-authored-by: Matt-Spence --- src/registrar/models/user.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 03d11e5bd..a7ea1e14a 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -206,12 +206,11 @@ class User(AbstractUser): if not portfolio: return False - portfolio_perms = self.portfolio_permissions.filter(portfolio=portfolio, user=self).first() - if not portfolio_perms: + user_portfolio_perms = self.portfolio_permissions.filter(portfolio=portfolio, user=self).first() + if not user_portfolio_perms: return False - portfolio_permissions = portfolio_perms._get_portfolio_permissions() - return portfolio_permission in portfolio_permissions + return portfolio_permission in user_portfolio_perms._get_portfolio_permissions() def has_base_portfolio_permission(self, portfolio): return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_PORTFOLIO) From 207a3ac3aa2801e1d188f28349d1ea71a95b328a Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 27 Aug 2024 13:31:03 -0500 Subject: [PATCH 49/59] fix populate script --- .../management/commands/populate_domain_request_dates.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index e20e35909..f7ea5ce27 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -20,16 +20,16 @@ class Command(BaseCommand, PopulateScriptTemplate): # Retrieve and order audit log entries by timestamp in descending order audit_log_entries = LogEntry.objects.filter(object_pk=record.pk).order_by("-timestamp") - # Loop through logs in descending order to find most recent status change for log_entry in audit_log_entries: - if 'status' in LogEntry.changes_dict: + if 'status' in log_entry.changes_dict: record.last_status_update = log_entry.timestamp.date() break # Loop through logs in ascending order to find first submission for log_entry in audit_log_entries.reverse(): - if log_entry.changes_dict['status'](1) == 'Submitted': + status = log_entry.changes_dict.get('status') + if status and status[1] == 'Submitted': record.first_submitted_date = log_entry.timestamp.date() break @@ -42,4 +42,4 @@ class Command(BaseCommand, PopulateScriptTemplate): def should_skip_record(self, record) -> bool: # make sure the record had some kind of history - return LogEntry.objects.filter(object_pk=record.pk).exists() + return not LogEntry.objects.filter(object_pk=record.pk).exists() \ No newline at end of file From 990e16246d49b25173d85e00a6f88927690da071 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Aug 2024 13:26:01 -0600 Subject: [PATCH 50/59] Update src/registrar/tests/test_views_portfolio.py Co-authored-by: Matt-Spence --- src/registrar/tests/test_views_portfolio.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 42cf82fa4..c5d1a9830 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -414,7 +414,7 @@ class TestPortfolio(WebTest): def test_portfolio_in_session_is_none_when_organization_feature_inactive(self): """When organization_feature flag is false and user has a portfolio, the portfolio should be set to None in session. - This test also satisfies the conidition when multiple_portfolios flag + This test also satisfies the condition when multiple_portfolios flag is false and user has a portfolio, so won't add a redundant test for that.""" self.client.force_login(self.user) portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] From 1fa80e8193c2650a1296ac8479cc036af633f569 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Aug 2024 13:26:13 -0600 Subject: [PATCH 51/59] Update src/registrar/registrar_middleware.py Co-authored-by: Matt-Spence --- src/registrar/registrar_middleware.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/registrar_middleware.py b/src/registrar/registrar_middleware.py index caefee650..4b590db1e 100644 --- a/src/registrar/registrar_middleware.py +++ b/src/registrar/registrar_middleware.py @@ -146,7 +146,7 @@ class CheckPortfolioMiddleware: # set the portfolio in the session if it is not set if "portfolio" not in request.session or request.session["portfolio"] is None: - # if user is a multiple portfolio + # if multiple portfolios are allowed for this user if flag_is_active(request, "multiple_portfolios"): # NOTE: we will want to change later to have a workflow for selecting # portfolio and another for switching portfolio; for now, select first From 9e592cbd753052c6f5df68d5120bf732f6c43e25 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 27 Aug 2024 15:11:43 -0500 Subject: [PATCH 52/59] fix dumb typo --- .../management/commands/populate_domain_request_dates.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index f7ea5ce27..cef140cdd 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -29,7 +29,7 @@ class Command(BaseCommand, PopulateScriptTemplate): # Loop through logs in ascending order to find first submission for log_entry in audit_log_entries.reverse(): status = log_entry.changes_dict.get('status') - if status and status[1] == 'Submitted': + if status and status[1] == 'submitted': record.first_submitted_date = log_entry.timestamp.date() break From 9c069a8e64e48b237caec9d41423aec264f66fb6 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 27 Aug 2024 15:12:57 -0500 Subject: [PATCH 53/59] add instructions --- docs/operations/data_migration.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/docs/operations/data_migration.md b/docs/operations/data_migration.md index cd748b22d..ed2f643d5 100644 --- a/docs/operations/data_migration.md +++ b/docs/operations/data_migration.md @@ -816,3 +816,30 @@ Example: `cf ssh getgov-za` | | Parameter | Description | |:-:|:-------------------------- |:-----------------------------------------------------------------------------------| | 1 | **federal_cio_csv_path** | Specifies where the federal CIO csv is | + +## Populate Domain Request Dates +This section outlines how to run the populate_domain_request_dates script + +### Running on sandboxes + +#### Step 1: Login to CloudFoundry +```cf login -a api.fr.cloud.gov --sso``` + +#### Step 2: SSH into your environment +```cf ssh getgov-{space}``` + +Example: `cf ssh getgov-za` + +#### Step 3: Create a shell instance +```/tmp/lifecycle/shell``` + +#### Step 4: Running the script +```./manage.py populate_domain_request_dates --debug``` + +### Running locally +```docker-compose exec app ./manage.py populate_domain_request_dates --debug``` + +##### Optional parameters +| | Parameter | Description | +|:-:|:-------------------------- |:----------------------------------------------------------------------------| +| 1 | **debug** | Increases logging detail. Defaults to False. | From 09ba7450b39d00e6cec1a3e91085ede27715db09 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 27 Aug 2024 15:19:36 -0500 Subject: [PATCH 54/59] linter fixes --- .../management/commands/populate_domain_request_dates.py | 8 ++++---- src/registrar/tests/test_views_requests_json.py | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index cef140cdd..3bbe6b306 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -11,7 +11,7 @@ class Command(BaseCommand, PopulateScriptTemplate): help = "Loops through each domain request object and populates the last_status_update and first_submitted_date" def handle(self, **kwargs): - """Loops through each DomainRequest object and populates + """Loops through each DomainRequest object and populates its last_status_update and first_submitted_date values""" self.mass_update_records(DomainRequest, None, ["last_status_update", "first_submitted_date"]) @@ -34,12 +34,12 @@ class Command(BaseCommand, PopulateScriptTemplate): break logger.info( - f"""{TerminalColors.OKCYAN}Updating {record} => - first submitted date: {record.first_submitted_date}, + f"""{TerminalColors.OKCYAN}Updating {record} => + first submitted date: {record.first_submitted_date}, last status update: {record.last_status_update}{TerminalColors.ENDC} """ ) def should_skip_record(self, record) -> bool: # make sure the record had some kind of history - return not LogEntry.objects.filter(object_pk=record.pk).exists() \ No newline at end of file + return not LogEntry.objects.filter(object_pk=record.pk).exists() diff --git a/src/registrar/tests/test_views_requests_json.py b/src/registrar/tests/test_views_requests_json.py index fd44bdc31..07a65b586 100644 --- a/src/registrar/tests/test_views_requests_json.py +++ b/src/registrar/tests/test_views_requests_json.py @@ -287,7 +287,7 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_sorting(self): """test that sorting works properly on the result set""" - response = self.app.get(reverse("get_domain_requests_json"), + response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json @@ -307,7 +307,7 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_filter_approved_excluded(self): """test that approved requests are excluded from result set.""" # sort in reverse chronological order of submission date, since most recent request is approved - response = self.app.get(reverse("get_domain_requests_json"), + response = self.app.get(reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"}) self.assertEqual(response.status_code, 200) data = response.json From 635e6e67351594a9242d6d704a94bbc63a91918d Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Wed, 28 Aug 2024 09:32:12 -0500 Subject: [PATCH 55/59] linter fixes --- .../commands/populate_domain_request_dates.py | 6 +++--- .../0119_add_domainrequest_submission_dates.py | 6 +++++- src/registrar/tests/test_views_requests_json.py | 10 ++++++---- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/registrar/management/commands/populate_domain_request_dates.py b/src/registrar/management/commands/populate_domain_request_dates.py index 3bbe6b306..d975a035d 100644 --- a/src/registrar/management/commands/populate_domain_request_dates.py +++ b/src/registrar/management/commands/populate_domain_request_dates.py @@ -22,14 +22,14 @@ class Command(BaseCommand, PopulateScriptTemplate): audit_log_entries = LogEntry.objects.filter(object_pk=record.pk).order_by("-timestamp") # Loop through logs in descending order to find most recent status change for log_entry in audit_log_entries: - if 'status' in log_entry.changes_dict: + if "status" in log_entry.changes_dict: record.last_status_update = log_entry.timestamp.date() break # Loop through logs in ascending order to find first submission for log_entry in audit_log_entries.reverse(): - status = log_entry.changes_dict.get('status') - if status and status[1] == 'submitted': + status = log_entry.changes_dict.get("status") + if status and status[1] == "submitted": record.first_submitted_date = log_entry.timestamp.date() break diff --git a/src/registrar/migrations/0119_add_domainrequest_submission_dates.py b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py index ea209626e..35897e302 100644 --- a/src/registrar/migrations/0119_add_domainrequest_submission_dates.py +++ b/src/registrar/migrations/0119_add_domainrequest_submission_dates.py @@ -26,7 +26,11 @@ class Migration(migrations.Migration): model_name="domainrequest", name="first_submitted_date", field=models.DateField( - blank=True, default=None, help_text="Date initially submitted", null=True, verbose_name="first submitted on" + blank=True, + default=None, + help_text="Date initially submitted", + null=True, + verbose_name="first submitted on", ), ), migrations.AddField( diff --git a/src/registrar/tests/test_views_requests_json.py b/src/registrar/tests/test_views_requests_json.py index 07a65b586..20a4069f7 100644 --- a/src/registrar/tests/test_views_requests_json.py +++ b/src/registrar/tests/test_views_requests_json.py @@ -287,8 +287,9 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_sorting(self): """test that sorting works properly on the result set""" - response = self.app.get(reverse("get_domain_requests_json"), - {"sort_by": "last_submitted_date", "order": "desc"}) + response = self.app.get( + reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"} + ) self.assertEqual(response.status_code, 200) data = response.json @@ -307,8 +308,9 @@ class GetRequestsJsonTest(TestWithUser, WebTest): def test_filter_approved_excluded(self): """test that approved requests are excluded from result set.""" # sort in reverse chronological order of submission date, since most recent request is approved - response = self.app.get(reverse("get_domain_requests_json"), - {"sort_by": "last_submitted_date", "order": "desc"}) + response = self.app.get( + reverse("get_domain_requests_json"), {"sort_by": "last_submitted_date", "order": "desc"} + ) self.assertEqual(response.status_code, 200) data = response.json From 6849f13e6cba86890f6c2e620d5a2c74d12d98d9 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 29 Aug 2024 08:22:16 -0600 Subject: [PATCH 56/59] Update src/registrar/models/user_portfolio_permission.py Co-authored-by: Alysia Broddrick <109625347+abroddrick@users.noreply.github.com> --- src/registrar/models/user_portfolio_permission.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index c2cdc61ad..bf1c3e566 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -42,7 +42,7 @@ class UserPortfolioPermission(TimeStampedModel): user = models.ForeignKey( "registrar.User", null=False, - # when a portfolio is deleted, permissions are too + # when a user is deleted, permissions are too on_delete=models.CASCADE, related_name="portfolio_permissions", ) From 06aacd91ce464fd808d792a04f285b34e2954a05 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 29 Aug 2024 10:58:19 -0400 Subject: [PATCH 57/59] minor improvements --- src/registrar/assets/js/get-gov-admin.js | 51 +++++++++--------------- src/registrar/tests/test_api.py | 11 ++++- 2 files changed, 28 insertions(+), 34 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 25dd91e0a..24f020b75 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -908,10 +908,6 @@ function initializeWidgetOnList(list, parentId) { return; } - // Hide the contactList initially. - // If we can update the contact information, it'll be shown again. - hideElement(contactList.parentElement); - // Determine if any changes are necessary to the display of portfolio type or federal type // based on changes to the Federal Agency let federalPortfolioApi = document.getElementById("federal_and_portfolio_types_from_agency_json_url").value; @@ -925,14 +921,15 @@ function initializeWidgetOnList(list, parentId) { console.error("Error in AJAX call: " + data.error); return; } - - let federal_type = data.federal_type; - let portfolio_type = data.portfolio_type; - updateFederalType(data.federal_type); - updatePortfolioType(data.portfolio_type); + updateReadOnly(data.federal_type, '.field-federal_type'); + updateReadOnly(data.portfolio_type, '.field-portfolio_type'); }) .catch(error => console.error("Error fetching federal and portfolio types: ", error)); + // Hide the contactList initially. + // If we can update the contact information, it'll be shown again. + hideElement(contactList.parentElement); + let seniorOfficialApi = document.getElementById("senior_official_from_agency_json_url").value; fetch(`${seniorOfficialApi}?agency_name=${selectedText}`) .then(response => { @@ -988,33 +985,21 @@ function initializeWidgetOnList(list, parentId) { } /** - * Dynamically update the portfolio type text in the dom to portfolioType + * Utility that selects a div from the DOM using selectorString, + * and updates a div within that div which has class of 'readonly' + * so that the text of the div is updated to updateText + * @param {*} updateText + * @param {*} selectorString */ - function updatePortfolioType(portfolioType) { - // Find the div with class 'field-portfolio_type' - const portfolioTypeDiv = document.querySelector('.field-portfolio_type'); - if (portfolioTypeDiv) { - // Find the nested div with class 'readonly' inside 'field-portfolio_type' - const readonlyDiv = portfolioTypeDiv.querySelector('.readonly'); + function updateReadOnly(updateText, selectorString) { + // find the div by selectorString + const selectedDiv = document.querySelector(selectorString); + if (selectedDiv) { + // find the nested div with class 'readonly' inside the selectorString div + const readonlyDiv = selectedDiv.querySelector('.readonly'); if (readonlyDiv) { // Update the text content of the readonly div - readonlyDiv.textContent = portfolioType !== null ? portfolioType : '-'; - } - } - } - - /** - * Dynamically update the federal type text in the dom to federalType - */ - function updateFederalType(federalType) { - // Find the div with class 'field-federal_type' - const federalTypeDiv = document.querySelector('.field-federal_type'); - if (federalTypeDiv) { - // Find the nested div with class 'readonly' inside 'field-federal_type' - const readonlyDiv = federalTypeDiv.querySelector('.readonly'); - if (readonlyDiv) { - // Update the text content of the readonly div - readonlyDiv.textContent = federalType !== null ? federalType : '-'; + readonlyDiv.textContent = updateText !== null ? updateText : '-'; } } } diff --git a/src/registrar/tests/test_api.py b/src/registrar/tests/test_api.py index 00bc9a1a2..2597e65c2 100644 --- a/src/registrar/tests/test_api.py +++ b/src/registrar/tests/test_api.py @@ -5,6 +5,7 @@ from django.contrib.auth import get_user_model from registrar.tests.common import create_superuser, create_user from api.tests.common import less_console_noise_decorator +from registrar.utility.constants import BranchChoices class GetSeniorOfficialJsonTest(TestCase): @@ -82,7 +83,7 @@ class GetFederalPortfolioTypeJsonTest(TestCase): self.superuser = create_superuser() self.analyst_user = create_user() - self.agency = FederalAgency.objects.create(agency="Test Agency", federal_type="judicial") + self.agency = FederalAgency.objects.create(agency="Test Agency", federal_type=BranchChoices.JUDICIAL) self.api_url = reverse("get-federal-and-portfolio-types-from-federal-agency-json") @@ -100,3 +101,11 @@ class GetFederalPortfolioTypeJsonTest(TestCase): data = response.json() self.assertEqual(data["federal_type"], "Judicial") self.assertEqual(data["portfolio_type"], "Federal - Judicial") + + @less_console_noise_decorator + def test_get_federal_and_portfolio_types_json_authenticated_regularuser(self): + """Test that a regular user receives a 403 with an error message.""" + p = "password" + self.client.login(username="testuser", password=p) + response = self.client.get(self.api_url, {"agency_name": "Test Agency", "organization_type": "federal"}) + self.assertEqual(response.status_code, 302) From ba1553a936aedf26c3d95177e1dddf73ad77a729 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Thu, 29 Aug 2024 11:49:45 -0500 Subject: [PATCH 58/59] add new dates to csv export --- src/registrar/utility/csv_export.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index 6f8510418..7ca3b7e97 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -1235,7 +1235,9 @@ class DomainRequestExport(BaseExport): "State/territory": model.get("state_territory"), "Request purpose": model.get("purpose"), "CISA regional representative": model.get("cisa_representative_email"), - "Submitted at": model.get("last_submitted_date"), + "Last submitted date": model.get("last_submitted_date"), + "First submitted date": model.get("first_submitted_date"), + "Last status update": model.get("last_status_update"), } row = [FIELDS.get(column, "") for column in columns] @@ -1304,7 +1306,9 @@ class DomainRequestDataFull(DomainRequestExport): """ return [ "Domain request", - "Submitted at", + "Last submitted date", + "First submitted date", + "Last status update", "Status", "Domain type", "Federal type", From fc17854529ecdebd544ba717a67bb979948f069f Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Thu, 29 Aug 2024 11:52:04 -0500 Subject: [PATCH 59/59] correct migration doc --- docs/operations/data_migration.md | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/docs/operations/data_migration.md b/docs/operations/data_migration.md index ed2f643d5..5914eb179 100644 --- a/docs/operations/data_migration.md +++ b/docs/operations/data_migration.md @@ -834,12 +834,7 @@ Example: `cf ssh getgov-za` ```/tmp/lifecycle/shell``` #### Step 4: Running the script -```./manage.py populate_domain_request_dates --debug``` +```./manage.py populate_domain_request_dates``` ### Running locally -```docker-compose exec app ./manage.py populate_domain_request_dates --debug``` - -##### Optional parameters -| | Parameter | Description | -|:-:|:-------------------------- |:----------------------------------------------------------------------------| -| 1 | **debug** | Increases logging detail. Defaults to False. | +```docker-compose exec app ./manage.py populate_domain_request_dates```