From 1a4268dedd72b5cfedefd235af1d756497883a77 Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Thu, 22 Aug 2024 09:46:07 -0500 Subject: [PATCH 01/32] add new script to update first ready values --- .../commands/update_first_ready_values.py | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 src/registrar/management/commands/update_first_ready_values.py diff --git a/src/registrar/management/commands/update_first_ready_values.py b/src/registrar/management/commands/update_first_ready_values.py new file mode 100644 index 000000000..9ee02109d --- /dev/null +++ b/src/registrar/management/commands/update_first_ready_values.py @@ -0,0 +1,28 @@ +import logging +from django.core.management import BaseCommand +from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors +from registrar.models import Domain, TransitionDomain + +logger = logging.getLogger(__name__) + +class Command(BaseCommand, PopulateScriptTemplate): + help = "Loops through eachdomain object and populates the last_status_update and first_submitted_date" + + def handle(self, **kwargs): + """Loops through each valid Domain object and updates it's first_ready value if they are out of sync""" + filter_conditions="state__in=[Domain.State.READY, Domain.State.ON_HOLD, Domain.State.DELETED]" + self.mass_update_records(Domain, filter_conditions, ["first_ready"]) + + def update_record(self, record: Domain): + """Defines how we update the first_read field""" + # if these are out of sync, update the + if self.is_transition_domain(record) and record.first_ready != record.created_at: + record.first_ready = record.created_at + + logger.info( + f"{TerminalColors.OKCYAN}Updating {record} => first_ready: " f"{record.first_ready}{TerminalColors.OKCYAN}" + ) + + # check if a transition domain object for this domain name exists + def is_transition_domain(record: Domain): + return TransitionDomain.objects.filter(domain_name=record.name).exists() \ No newline at end of file From 48901a716d80f5f3bc5507d72a20100806bfc45c Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 22 Aug 2024 14:30:14 -0500 Subject: [PATCH 02/32] rewrite script and extend terminal helper for more context --- ..._ready_values.py => update_first_ready.py} | 21 +++-- .../commands/utility/terminal_helper.py | 77 ++++++++++++++++--- 2 files changed, 75 insertions(+), 23 deletions(-) rename src/registrar/management/commands/{update_first_ready_values.py => update_first_ready.py} (56%) diff --git a/src/registrar/management/commands/update_first_ready_values.py b/src/registrar/management/commands/update_first_ready.py similarity index 56% rename from src/registrar/management/commands/update_first_ready_values.py rename to src/registrar/management/commands/update_first_ready.py index 9ee02109d..4803b70f8 100644 --- a/src/registrar/management/commands/update_first_ready_values.py +++ b/src/registrar/management/commands/update_first_ready.py @@ -6,23 +6,22 @@ from registrar.models import Domain, TransitionDomain logger = logging.getLogger(__name__) class Command(BaseCommand, PopulateScriptTemplate): - help = "Loops through eachdomain object and populates the last_status_update and first_submitted_date" + help = "Loops through each domain object and populates the last_status_update and first_submitted_date" def handle(self, **kwargs): - """Loops through each valid Domain object and updates it's first_ready value if they are out of sync""" - filter_conditions="state__in=[Domain.State.READY, Domain.State.ON_HOLD, Domain.State.DELETED]" - self.mass_update_records(Domain, filter_conditions, ["first_ready"]) + """Loops through each valid Domain object and updates it's first_ready value if it is out of sync""" + filter_conditions={"state__in":[Domain.State.READY, Domain.State.ON_HOLD, Domain.State.DELETED]} + self.mass_update_records(Domain, filter_conditions, ["first_ready"], verbose=True, custom_filter=self.should_update) def update_record(self, record: Domain): - """Defines how we update the first_read field""" - # if these are out of sync, update the - if self.is_transition_domain(record) and record.first_ready != record.created_at: - record.first_ready = record.created_at + """Defines how we update the first_ready field""" + # update the first_ready value based on the creation date. + record.first_ready = record.created_at logger.info( f"{TerminalColors.OKCYAN}Updating {record} => first_ready: " f"{record.first_ready}{TerminalColors.OKCYAN}" ) - # check if a transition domain object for this domain name exists - def is_transition_domain(record: Domain): - return TransitionDomain.objects.filter(domain_name=record.name).exists() \ No newline at end of file + # check if a transition domain object for this domain name exists, and if so whether + def should_update(self, record: Domain) -> bool: + return TransitionDomain.objects.filter(domain_name=record.name).exists() and record.first_ready != record.created_at \ No newline at end of file diff --git a/src/registrar/management/commands/utility/terminal_helper.py b/src/registrar/management/commands/utility/terminal_helper.py index 2c69e1080..13d58191a 100644 --- a/src/registrar/management/commands/utility/terminal_helper.py +++ b/src/registrar/management/commands/utility/terminal_helper.py @@ -2,6 +2,7 @@ import logging import sys from abc import ABC, abstractmethod from django.core.paginator import Paginator +from django.db.models import Model from typing import List from registrar.utility.enums import LogCode @@ -75,28 +76,74 @@ class PopulateScriptTemplate(ABC): run_summary_header = None @abstractmethod - def update_record(self, record): - """Defines how we update each field. Must be defined before using mass_update_records.""" + def update_record(self, record: Model): + """Defines how we update each field. + + raises: + NotImplementedError: If not defined before calling mass_update_records. + """ raise NotImplementedError - def mass_update_records(self, object_class, filter_conditions, fields_to_update, debug=True): + def mass_update_records(self, object_class: Model, filter_conditions, fields_to_update, debug=True, verbose=False, custom_filter=None): """Loops through each valid "object_class" object - specified by filter_conditions - and updates fields defined by fields_to_update using update_record. - You must define update_record before you can use this function. + Parameters: + object_class: The Django model class that you want to perform the bulk update on. + This should be the actual class, not a string of the class name. + + filter_conditions: dictionary of valid Django Queryset filter conditions + (e.g. {'verification_type__isnull'=True}). + + fields_to_update: List of strings specifying which fields to update. + (e.g. ["first_ready_date", "last_submitted_date"]) + + debug: Whether to log script run summary in debug mode. + Default: True. + + verbose: Whether to print a detailed run summary *before* run confirmation. + Default: False. + + custom_filter: function taking in a single record and returning a boolean representing whether + the record should be included of the final record set. Used to allow for filters that can't be + represented by django queryset field lookups. Applied *after* filter_conditions. + + Raises: + NotImplementedError: If you do not define update_record before using this function. + TypeError: If custom_filter is not Callable. """ records = object_class.objects.filter(**filter_conditions) + + # apply custom filter + if custom_filter: + to_exclude_pks = [] + for record in records: + try: + if not custom_filter(record): + to_exclude_pks.append(record.pk) + except TypeError as e: + logger.error(f"Error applying custom filter: {e}") + sys.exit() + records=records.exclude(pk__in=to_exclude_pks) + readable_class_name = self.get_class_name(object_class) + # for use in the execution prompt. + proposed_changes=f"""==Proposed Changes== + Number of {readable_class_name} objects to change: {len(records)} + These fields will be updated on each record: {fields_to_update} + """ + + if verbose: + proposed_changes=f"""{proposed_changes} + These records will be updated: {list(records.all())} + """ + # Code execution will stop here if the user prompts "N" TerminalHelper.prompt_for_execution( system_exit_on_terminate=True, - info_to_inspect=f""" - ==Proposed Changes== - Number of {readable_class_name} objects to change: {len(records)} - These fields will be updated on each record: {fields_to_update} - """, + prompt_message=proposed_changes, prompt_title=self.prompt_title, ) logger.info("Updating...") @@ -220,6 +267,9 @@ class TerminalHelper: an answer is required of the user). The "answer" return value is True for "yes" or False for "no". + + Raises: + ValueError: When "default" is not "yes", "no", or None. """ valid = {"yes": True, "y": True, "ye": True, "no": False, "n": False} if default is None: @@ -244,6 +294,7 @@ class TerminalHelper: @staticmethod def query_yes_no_exit(question: str, default="yes"): """Ask a yes/no question via raw_input() and return their answer. + Allows for answer "e" to exit. "question" is a string that is presented to the user. "default" is the presumed answer if the user just hits . @@ -251,6 +302,9 @@ class TerminalHelper: an answer is required of the user). The "answer" return value is True for "yes" or False for "no". + + Raises: + ValueError: When "default" is not "yes", "no", or None. """ valid = { "yes": True, @@ -317,9 +371,8 @@ class TerminalHelper: case _: logger.info(print_statement) - # TODO - "info_to_inspect" should be refactored to "prompt_message" @staticmethod - def prompt_for_execution(system_exit_on_terminate: bool, info_to_inspect: str, prompt_title: str) -> bool: + def prompt_for_execution(system_exit_on_terminate: bool, prompt_message: str, prompt_title: str) -> bool: """Create to reduce code complexity. Prompts the user to inspect the given string and asks if they wish to proceed. @@ -340,7 +393,7 @@ class TerminalHelper: ===================================================== *** IMPORTANT: VERIFY THE FOLLOWING LOOKS CORRECT *** - {info_to_inspect} + {prompt_message} {TerminalColors.FAIL} Proceed? (Y = proceed, N = {action_description_for_selecting_no}) {TerminalColors.ENDC}""" From 4026e9be8601ec63f9d1aa29a001ef6c3c3cd31d Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Thu, 22 Aug 2024 15:47:21 -0500 Subject: [PATCH 03/32] minor changes --- src/registrar/management/commands/update_first_ready.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/management/commands/update_first_ready.py b/src/registrar/management/commands/update_first_ready.py index 4803b70f8..aaf04e2a7 100644 --- a/src/registrar/management/commands/update_first_ready.py +++ b/src/registrar/management/commands/update_first_ready.py @@ -16,7 +16,7 @@ class Command(BaseCommand, PopulateScriptTemplate): def update_record(self, record: Domain): """Defines how we update the first_ready field""" # update the first_ready value based on the creation date. - record.first_ready = record.created_at + record.first_ready = record.created_at.date() logger.info( f"{TerminalColors.OKCYAN}Updating {record} => first_ready: " f"{record.first_ready}{TerminalColors.OKCYAN}" @@ -24,4 +24,4 @@ class Command(BaseCommand, PopulateScriptTemplate): # check if a transition domain object for this domain name exists, and if so whether def should_update(self, record: Domain) -> bool: - return TransitionDomain.objects.filter(domain_name=record.name).exists() and record.first_ready != record.created_at \ No newline at end of file + return TransitionDomain.objects.filter(domain_name=record.name).exists() and record.first_ready != record.created_at.date() \ No newline at end of file From a7c801739d7ff19c379a6d5306e127eaa6725db1 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 16:33:10 -0400 Subject: [PATCH 04/32] new view, transfer and delete logic, modal, combobox --- src/registrar/admin.py | 2 + src/registrar/assets/js/get-gov-admin.js | 62 +++-- src/registrar/assets/sass/_theme/_admin.scss | 56 ++++- src/registrar/config/settings.py | 3 +- src/registrar/config/urls.py | 2 + src/registrar/templates/admin/analytics.html | 20 +- .../templates/admin/transfer_user.html | 229 ++++++++++++++++++ .../django/admin/user_change_form.html | 15 ++ src/registrar/views/transfer_user.py | 158 ++++++++++++ 9 files changed, 492 insertions(+), 55 deletions(-) create mode 100644 src/registrar/templates/admin/transfer_user.html create mode 100644 src/registrar/views/transfer_user.py diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 3ad5e3ea0..4a34ac0f8 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -41,6 +41,8 @@ from django.core.exceptions import ObjectDoesNotExist from django.contrib.admin.widgets import FilteredSelectMultiple from django.utils.html import format_html from django.utils.translation import gettext_lazy as _ +from django.shortcuts import get_object_or_404, render +from django.urls import path logger = logging.getLogger(__name__) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 01c93abf6..6c302836f 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -172,40 +172,39 @@ function addOrRemoveSessionBoolean(name, add){ ** To perform data operations on this - we need to use jQuery rather than vanilla js. */ (function (){ - let selector = django.jQuery("#id_investigator") - let assignSelfButton = document.querySelector("#investigator__assign_self"); - if (!selector || !assignSelfButton) { - return; - } - - let currentUserId = assignSelfButton.getAttribute("data-user-id"); - let currentUserName = assignSelfButton.getAttribute("data-user-name"); - if (!currentUserId || !currentUserName){ - console.error("Could not assign current user: no values found.") - return; - } - - // Hook a click listener to the "Assign to me" button. - // Logic borrowed from here: https://select2.org/programmatic-control/add-select-clear-items#create-if-not-exists - assignSelfButton.addEventListener("click", function() { - if (selector.find(`option[value='${currentUserId}']`).length) { - // Select the value that is associated with the current user. - selector.val(currentUserId).trigger("change"); - } else { - // Create a DOM Option that matches the desired user. Then append it and select it. - let userOption = new Option(currentUserName, currentUserId, true, true); - selector.append(userOption).trigger("change"); + if (document.getElementById("id_investigator") && django && django.jQuery) { + let selector = django.jQuery("#id_investigator") + let assignSelfButton = document.querySelector("#investigator__assign_self"); + if (!selector || !assignSelfButton) { + return; } - }); - // Listen to any change events, and hide the parent container if investigator has a value. - selector.on('change', function() { - // The parent container has display type flex. - assignSelfButton.parentElement.style.display = this.value === currentUserId ? "none" : "flex"; - }); - - + let currentUserId = assignSelfButton.getAttribute("data-user-id"); + let currentUserName = assignSelfButton.getAttribute("data-user-name"); + if (!currentUserId || !currentUserName){ + console.error("Could not assign current user: no values found.") + return; + } + // Hook a click listener to the "Assign to me" button. + // Logic borrowed from here: https://select2.org/programmatic-control/add-select-clear-items#create-if-not-exists + assignSelfButton.addEventListener("click", function() { + if (selector.find(`option[value='${currentUserId}']`).length) { + // Select the value that is associated with the current user. + selector.val(currentUserId).trigger("change"); + } else { + // Create a DOM Option that matches the desired user. Then append it and select it. + let userOption = new Option(currentUserName, currentUserId, true, true); + selector.append(userOption).trigger("change"); + } + }); + + // Listen to any change events, and hide the parent container if investigator has a value. + selector.on('change', function() { + // The parent container has display type flex. + assignSelfButton.parentElement.style.display = this.value === currentUserId ? "none" : "flex"; + }); + } })(); /** An IIFE for pages in DjangoAdmin that use a clipboard button @@ -215,7 +214,6 @@ function addOrRemoveSessionBoolean(name, add){ function copyToClipboardAndChangeIcon(button) { // Assuming the input is the previous sibling of the button let input = button.previousElementSibling; - let userId = input.getAttribute("user-id") // Copy input value to clipboard if (input) { navigator.clipboard.writeText(input.value).then(function() { diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index 8ca6b5465..40aef1121 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -120,7 +120,7 @@ html[data-theme="light"] { body.dashboard, body.change-list, body.change-form, - .analytics { + .custom-admin-template, dt { color: var(--body-fg); } .usa-table td { @@ -149,7 +149,7 @@ html[data-theme="dark"] { body.dashboard, body.change-list, body.change-form, - .analytics { + .custom-admin-template, dt { color: var(--body-fg); } .usa-table td { @@ -160,7 +160,7 @@ html[data-theme="dark"] { // Remove when dark mode successfully applies to Django delete page. .delete-confirmation .content a:not(.button) { color: color('primary'); - } + } } @@ -364,14 +364,40 @@ input.admin-confirm-button { list-style-type: none; line-height: normal; } - .button { - display: inline-block; - padding: 10px 8px; - line-height: normal; - } - a.button:active, a.button:focus { - text-decoration: none; - } +} + +// This block resolves some of the issues we're seeing on buttons due to css +// conflicts between DJ and USWDS +a.button, +.usa-button { + display: inline-block; + padding: 10px 15px; + font-size: 14px; + line-height: 16.1px; + font-kerning: auto; + font-family: inherit; + font-weight: normal; +} +.button svg, +.button span, +.usa-button svg, +.usa-button span { + vertical-align: middle; +} +.usa-button:not(.usa-button--unstyled, .usa-button--outline, .usa-modal__close, .usa-button--secondary) { + background: var(--button-bg); +} +.usa-button span { + font-size: 14px; +} +.usa-button:not(.usa-button--unstyled, .usa-button--outline, .usa-modal__close, .usa-button--secondary):hover { + background: var(--button-hover-bg); +} +a.button:active, a.button:focus { + text-decoration: none; +} +.usa-modal { + font-family: inherit; } .module--custom { @@ -732,7 +758,7 @@ div.dja__model-description{ li { list-style-type: disc; - font-family: Source Sans Pro Web,Helvetica Neue,Helvetica,Roboto,Arial,sans-serif; + font-family: family('sans'); } a, a:link, a:visited { @@ -852,3 +878,9 @@ ul.add-list-reset { padding: 0 !important; margin: 0 !important; } + +// Fix the combobox when deployed outside admin (eg user transfer) +.submit-row .select2, +.submit-row .select2 span { + margin-top: 0; +} \ No newline at end of file diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index 73aecad7a..d1f8d2384 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -357,13 +357,14 @@ CSP_FORM_ACTION = allowed_sources # and inline with a nonce, as well as allowing connections back to their domain. # Note: If needed, we can embed chart.js instead of using the CDN CSP_DEFAULT_SRC = ("'self'",) -CSP_STYLE_SRC = ["'self'", "https://www.ssa.gov/accessibility/andi/andi.css"] +CSP_STYLE_SRC = ["'self'", "https://www.ssa.gov/accessibility/andi/andi.css", "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/css/select2.min.css"] CSP_SCRIPT_SRC_ELEM = [ "'self'", "https://www.googletagmanager.com/", "https://cdn.jsdelivr.net/npm/chart.js", "https://www.ssa.gov", "https://ajax.googleapis.com", + "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/js/select2.min.js" ] CSP_CONNECT_SRC = ["'self'", "https://www.google-analytics.com/", "https://www.ssa.gov/accessibility/andi/andi.js"] CSP_INCLUDE_NONCE_IN = ["script-src-elem", "style-src"] diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index 59f52cd95..ee32930f1 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -24,6 +24,7 @@ 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.transfer_user import TransferUserView from registrar.views.utility.api_views import get_senior_official_from_federal_agency_json from registrar.views.domains_json import get_domains_json from registrar.views.utility import always_404 @@ -129,6 +130,7 @@ urlpatterns = [ AnalyticsView.as_view(), name="analytics", ), + path('admin/registrar/user//transfer/', TransferUserView.as_view(), name='transfer_user'), path( "admin/api/get-senior-official-from-federal-agency-json/", get_senior_official_from_federal_agency_json, diff --git a/src/registrar/templates/admin/analytics.html b/src/registrar/templates/admin/analytics.html index 13db3b60a..2729d883d 100644 --- a/src/registrar/templates/admin/analytics.html +++ b/src/registrar/templates/admin/analytics.html @@ -5,7 +5,7 @@ {% block content %} -
+
@@ -29,28 +29,28 @@ +
+
+ +
+
+{% endblock %} \ No newline at end of file diff --git a/src/registrar/templates/django/admin/user_change_form.html b/src/registrar/templates/django/admin/user_change_form.html index 005d67aec..c0ddd8caf 100644 --- a/src/registrar/templates/django/admin/user_change_form.html +++ b/src/registrar/templates/django/admin/user_change_form.html @@ -1,6 +1,21 @@ {% extends 'django/admin/email_clipboard_change_form.html' %} {% load i18n static %} + +{% block field_sets %} + + + {% for fieldset in adminform %} + {% include "django/admin/includes/domain_fieldset.html" with state_help_message=state_help_message %} + {% endfor %} +{% endblock %} + {% block after_related_objects %}

Associated requests and domains

diff --git a/src/registrar/views/transfer_user.py b/src/registrar/views/transfer_user.py new file mode 100644 index 000000000..5e367f04d --- /dev/null +++ b/src/registrar/views/transfer_user.py @@ -0,0 +1,158 @@ +import logging + +from django.shortcuts import render, get_object_or_404, redirect +from django.views import View +from registrar.models.domain import Domain +from registrar.models.domain_information import DomainInformation +from registrar.models.domain_request import DomainRequest +from registrar.models.portfolio import Portfolio +from registrar.models.user import User +from django.contrib.admin import site +from django.contrib import messages + +from registrar.models.user_domain_role import UserDomainRole +from registrar.models.verified_by_staff import VerifiedByStaff + +logger = logging.getLogger(__name__) + +class TransferUserView(View): + """Transfer user methods that set up the transfer_user template and handle the forms on it.""" + + JOINS = [ + (DomainRequest, 'creator'), + (DomainInformation, 'creator'), + (Portfolio, 'creator'), + (DomainRequest, 'investigator'), + (UserDomainRole, 'user'), + (VerifiedByStaff, 'requestor'), + ] + + USER_FIELDS = ['portfolio', 'portfolio_roles', 'portfolio_additional_permissions'] + + def get(self, request, user_id): + """current_user referes to the 'source' user where the button that redirects to this view was clicked. + other_users exclude current_user and populate a dropdown, selected_user is the selection in the dropdown. + + This also querries the relevant domains and domain requests, and the admin context needed for the sidenav.""" + + current_user = get_object_or_404(User, pk=user_id) + other_users = User.objects.exclude(pk=user_id).order_by('first_name', 'last_name') # Exclude the current user from the dropdown + + # Get the default admin site context, needed for the sidenav + admin_context = site.each_context(request) + + context = { + 'current_user': current_user, + 'other_users': other_users, + 'logged_in_user': request.user, + **admin_context, # Include the admin context + 'current_user_domains': self.get_domains(current_user), + 'current_user_domain_requests': self.get_domain_requests(current_user) + } + + selected_user_id = request.GET.get('selected_user') + if selected_user_id: + selected_user = get_object_or_404(User, pk=selected_user_id) + context['selected_user'] = selected_user + context['selected_user_domains'] = self.get_domains(selected_user) + context['selected_user_domain_requests'] = self.get_domain_requests(selected_user) + + return render(request, 'admin/transfer_user.html', context) + + def post(self, request, user_id): + """This handles the transfer from selected_user to current_user then deletes selected_user. + + NOTE: We have a ticket to refactor this into a more solid lookup for related fields in #2645""" + + current_user = get_object_or_404(User, pk=user_id) + selected_user_id = request.POST.get('selected_user') + selected_user = get_object_or_404(User, pk=selected_user_id) + + try: + change_logs = [] + + # Transfer specific fields + self.transfer_user_fields_and_log(selected_user, current_user, change_logs) + + # Perform the updates and log the changes + for model_class, field_name in self.JOINS: + self.update_joins_and_log(model_class, field_name, selected_user, current_user, change_logs) + + # Success message if any related objects were updated + if change_logs: + success_message = f"Data transferred successfully for the following objects: {change_logs}" + messages.success(request, success_message) + + selected_user.delete() + messages.success(request, f"Deleted {selected_user} {selected_user.username}") + + except Exception as e: + messages.error(request, f"An error occurred during the transfer: {e}") + + return redirect('admin:registrar_user_change', object_id=user_id) + + @classmethod + def update_joins_and_log(cls, model_class, field_name, old_user, new_user, change_logs): + """ + Helper function to update the user join fields for a given model and log the changes. + """ + + filter_kwargs = {field_name: old_user} + updated_objects = model_class.objects.filter(**filter_kwargs) + + for obj in updated_objects: + # Check for duplicate UserDomainRole before updating + if model_class == UserDomainRole: + if model_class.objects.filter(user=new_user, domain=obj.domain).exists(): + continue # Skip the update to avoid a duplicate + + # Update the field on the object and save it + setattr(obj, field_name, new_user) + obj.save() + + # Log the change + cls.log_change(obj, field_name, old_user, new_user, change_logs) + + @classmethod + def transfer_user_fields_and_log(cls, old_user, new_user, change_logs): + """ + Transfers portfolio fields from the old_user to the new_user. + Logs the changes for each transferred field. + + NOTE: This will be refactored in #2644 + """ + + for field in cls.USER_FIELDS: + old_value = getattr(old_user, field, None) + + if old_value: + setattr(new_user, field, old_value) + cls.log_change(new_user, field, old_value, old_value, change_logs) + + new_user.save() + + @classmethod + def log_change(cls, obj, field_name, old_value, new_value, change_logs): + """Logs the change for a specific field on an object""" + log_entry = f'Changed {field_name} from "{old_value}" to "{new_value}" on {obj}' + + logger.info(log_entry) + + # Collect the related object for the success message + change_logs.append(log_entry) + + @classmethod + def get_domains(cls, user): + """A simplified version of domains_json""" + user_domain_roles = UserDomainRole.objects.filter(user=user) + domain_ids = user_domain_roles.values_list("domain_id", flat=True) + domains = Domain.objects.filter(id__in=domain_ids) + + return domains + + @classmethod + def get_domain_requests(cls, user): + """A simplified version of domain_requests_json""" + domain_requests = DomainRequest.objects.filter(creator=user) + + return domain_requests \ No newline at end of file From c1df03d831c48c1d4cbb25afe2fa806ee687da5c Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 21:15:42 -0400 Subject: [PATCH 05/32] test and lint --- src/registrar/admin.py | 2 - src/registrar/config/settings.py | 8 +- src/registrar/config/urls.py | 2 +- src/registrar/tests/test_admin.py | 225 ++++++++++++++++++++++++++- src/registrar/views/__init__.py | 1 + src/registrar/views/transfer_user.py | 82 +++++----- 6 files changed, 274 insertions(+), 46 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 4a34ac0f8..3ad5e3ea0 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -41,8 +41,6 @@ from django.core.exceptions import ObjectDoesNotExist from django.contrib.admin.widgets import FilteredSelectMultiple from django.utils.html import format_html from django.utils.translation import gettext_lazy as _ -from django.shortcuts import get_object_or_404, render -from django.urls import path logger = logging.getLogger(__name__) diff --git a/src/registrar/config/settings.py b/src/registrar/config/settings.py index d1f8d2384..7965424bc 100644 --- a/src/registrar/config/settings.py +++ b/src/registrar/config/settings.py @@ -357,14 +357,18 @@ CSP_FORM_ACTION = allowed_sources # and inline with a nonce, as well as allowing connections back to their domain. # Note: If needed, we can embed chart.js instead of using the CDN CSP_DEFAULT_SRC = ("'self'",) -CSP_STYLE_SRC = ["'self'", "https://www.ssa.gov/accessibility/andi/andi.css", "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/css/select2.min.css"] +CSP_STYLE_SRC = [ + "'self'", + "https://www.ssa.gov/accessibility/andi/andi.css", + "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/css/select2.min.css", +] CSP_SCRIPT_SRC_ELEM = [ "'self'", "https://www.googletagmanager.com/", "https://cdn.jsdelivr.net/npm/chart.js", "https://www.ssa.gov", "https://ajax.googleapis.com", - "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/js/select2.min.js" + "https://cdn.jsdelivr.net/npm/select2@4.1.0-rc.0/dist/js/select2.min.js", ] CSP_CONNECT_SRC = ["'self'", "https://www.google-analytics.com/", "https://www.ssa.gov/accessibility/andi/andi.js"] CSP_INCLUDE_NONCE_IN = ["script-src-elem", "style-src"] diff --git a/src/registrar/config/urls.py b/src/registrar/config/urls.py index ee32930f1..0a8e00350 100644 --- a/src/registrar/config/urls.py +++ b/src/registrar/config/urls.py @@ -130,7 +130,7 @@ urlpatterns = [ AnalyticsView.as_view(), name="analytics", ), - path('admin/registrar/user//transfer/', TransferUserView.as_view(), name='transfer_user'), + path("admin/registrar/user//transfer/", TransferUserView.as_view(), name="transfer_user"), path( "admin/api/get-senior-official-from-federal-agency-json/", get_senior_official_from_federal_agency_json, diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 827742ef1..f051325a6 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -45,6 +45,7 @@ from registrar.models import ( from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.senior_official import SeniorOfficial from registrar.models.user_domain_role import UserDomainRole +from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from registrar.models.verified_by_staff import VerifiedByStaff from .common import ( MockDbForSharedTests, @@ -60,7 +61,8 @@ from .common import ( ) from django.contrib.sessions.backends.db import SessionStore from django.contrib.auth import get_user_model -from unittest.mock import patch, Mock +from unittest.mock import ANY, patch, Mock +from django_webtest import WebTest # type: ignore import logging @@ -2121,3 +2123,224 @@ class TestPortfolioAdmin(TestCase): self.assertIn("request1.gov", domain_requests) self.assertIn("request2.gov", domain_requests) self.assertIn('
    ', domain_requests) + + +class TestTransferUser(WebTest): + """User transfer custom admin page""" + + # csrf checks do not work well with WebTest. + # We disable them here. + csrf_checks = False + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.site = AdminSite() + cls.superuser = create_superuser() + cls.admin = PortfolioAdmin(model=Portfolio, admin_site=cls.site) + cls.factory = RequestFactory() + + def setUp(self): + self.app.set_user(self.superuser) + self.user1, _ = User.objects.get_or_create( + username="madmax", first_name="Max", last_name="Rokatanski", title="Road warrior" + ) + self.user2, _ = User.objects.get_or_create( + username="furiosa", first_name="Furiosa", last_name="Jabassa", title="Imperator" + ) + + def tearDown(self): + Suborganization.objects.all().delete() + DomainInformation.objects.all().delete() + DomainRequest.objects.all().delete() + Domain.objects.all().delete() + Portfolio.objects.all().delete() + UserDomainRole.objects.all().delete() + + @less_console_noise_decorator + def test_transfer_user_shows_current_and_selected_user_information(self): + """Assert we pull the current user info and display it on the transfer page""" + completed_domain_request(user=self.user1, name="wasteland.gov") + domain_request = completed_domain_request( + user=self.user1, name="citadel.gov", status=DomainRequest.DomainRequestStatus.SUBMITTED + ) + domain_request.status = DomainRequest.DomainRequestStatus.APPROVED + domain_request.save() + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + + self.assertContains(user_transfer_page, "madmax") + self.assertContains(user_transfer_page, "Max") + self.assertContains(user_transfer_page, "Rokatanski") + self.assertContains(user_transfer_page, "Road warrior") + self.assertContains(user_transfer_page, "wasteland.gov") + self.assertContains(user_transfer_page, "citadel.gov") + + select_form = user_transfer_page.forms[0] + select_form["selected_user"] = str(self.user2.id) + preview_result = select_form.submit() + + self.assertContains(preview_result, "furiosa") + self.assertContains(preview_result, "Furiosa") + self.assertContains(preview_result, "Jabassa") + self.assertContains(preview_result, "Imperator") + + @less_console_noise_decorator + def test_transfer_user_transfers_portfolio_roles_and_permissions_and_portfolio_creator(self): + """Assert that a portfolio gets copied over + NOTE: Should be revised for #2644""" + portfolio = Portfolio.objects.create(organization_name="Citadel", creator=self.user2) + self.user2.portfolio = portfolio + self.user2.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + self.user2.portfolio_additional_permissions = [ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.EDIT_PORTFOLIO, + ] + self.user2.save() + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + + self.user1.refresh_from_db() + portfolio.refresh_from_db() + + self.assertEquals(portfolio.creator, self.user1) + self.assertEquals(self.user1.portfolio, portfolio) + self.assertEquals(self.user1.portfolio_roles, [UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) + self.assertEquals( + self.user1.portfolio_additional_permissions, + [UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO], + ) + + @less_console_noise_decorator + def test_transfer_user_transfers_domain_request_creator_and_investigator(self): + """Assert that domain request fields get transferred""" + domain_request = completed_domain_request(user=self.user2, name="wasteland.gov", investigator=self.user2) + + self.assertEquals(domain_request.creator, self.user2) + self.assertEquals(domain_request.investigator, self.user2) + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + domain_request.refresh_from_db() + + self.assertEquals(domain_request.creator, self.user1) + self.assertEquals(domain_request.investigator, self.user1) + + @less_console_noise_decorator + def test_transfer_user_transfers_domain_information_creator(self): + """Assert that domain fields get transferred""" + domain_information, _ = DomainInformation.objects.get_or_create(creator=self.user2) + + self.assertEquals(domain_information.creator, self.user2) + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + domain_information.refresh_from_db() + + self.assertEquals(domain_information.creator, self.user1) + + @less_console_noise_decorator + def test_transfer_user_transfers_domain_role(self): + """Assert that user domain role get transferred""" + domain_1, _ = Domain.objects.get_or_create(name="chrome.gov", state=Domain.State.READY) + domain_2, _ = Domain.objects.get_or_create(name="v8.gov", state=Domain.State.READY) + user_domain_role1, _ = UserDomainRole.objects.get_or_create( + user=self.user2, domain=domain_1, role=UserDomainRole.Roles.MANAGER + ) + user_domain_role2, _ = UserDomainRole.objects.get_or_create( + user=self.user2, domain=domain_2, role=UserDomainRole.Roles.MANAGER + ) + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + user_domain_role1.refresh_from_db() + user_domain_role2.refresh_from_db() + + self.assertEquals(user_domain_role1.user, self.user1) + self.assertEquals(user_domain_role2.user, self.user1) + + @less_console_noise_decorator + def test_transfer_user_transfers_verified_by_staff_requestor(self): + """Assert that verified by staff creator gets transferred""" + vip, _ = VerifiedByStaff.objects.get_or_create(requestor=self.user2, email="immortan.joe@citadel.com") + + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + vip.refresh_from_db() + + self.assertEquals(vip.requestor, self.user1) + + @less_console_noise_decorator + def test_transfer_user_deletes_old_user(self): + """Assert that the slected user gets deleted""" + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit() + # Refresh user2 from the database and check if it still exists + with self.assertRaises(User.DoesNotExist): + self.user2.refresh_from_db() + + @less_console_noise_decorator + def test_transfer_user_throws_transfer_and_delete_success_messages(self): + """Test that success messages for data transfer and user deletion are displayed.""" + # Ensure the setup for VerifiedByStaff + VerifiedByStaff.objects.get_or_create(requestor=self.user2, email="immortan.joe@citadel.com") + + # Access the transfer user page + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + + with patch("django.contrib.messages.success") as mock_success_message: + + # Fill the form with the selected user and submit + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + after_submit = submit_form.submit().follow() + + self.assertContains(after_submit, "

    Change user

    ") + + mock_success_message.assert_any_call( + ANY, + ( + "Data transferred successfully for the following objects: ['Changed requestor " + + 'from "Furiosa Jabassa " to "Max Rokatanski " on immortan.joe@citadel.com\']' + ), + ) + + mock_success_message.assert_any_call(ANY, f"Deleted {self.user2} {self.user2.username}") + + @less_console_noise_decorator + def test_transfer_user_throws_error_message(self): + """Test that an error message is thrown if the transfer fails.""" + with patch( + "registrar.views.TransferUserView.transfer_user_fields_and_log", side_effect=Exception("Simulated Error") + ): + with patch("django.contrib.messages.error") as mock_error: + # Access the transfer user page + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + + # Fill the form with the selected user and submit + submit_form = user_transfer_page.forms[1] + submit_form["selected_user"] = self.user2.pk + submit_form.submit().follow() + + # Assert that the error message was called with the correct argument + mock_error.assert_called_once_with(ANY, "An error occurred during the transfer: Simulated Error") + + @less_console_noise_decorator + def test_transfer_user_modal(self): + """Assert modal on page""" + user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) + self.assertContains(user_transfer_page, "But you know what you're doing, right?") diff --git a/src/registrar/views/__init__.py b/src/registrar/views/__init__.py index f6e87dd07..2b830d958 100644 --- a/src/registrar/views/__init__.py +++ b/src/registrar/views/__init__.py @@ -19,3 +19,4 @@ from .user_profile import UserProfileView, FinishProfileSetupView from .health import * from .index import * from .portfolios import * +from .transfer_user import TransferUserView diff --git a/src/registrar/views/transfer_user.py b/src/registrar/views/transfer_user.py index 5e367f04d..cc901d5ab 100644 --- a/src/registrar/views/transfer_user.py +++ b/src/registrar/views/transfer_user.py @@ -15,57 +15,60 @@ from registrar.models.verified_by_staff import VerifiedByStaff logger = logging.getLogger(__name__) + class TransferUserView(View): """Transfer user methods that set up the transfer_user template and handle the forms on it.""" JOINS = [ - (DomainRequest, 'creator'), - (DomainInformation, 'creator'), - (Portfolio, 'creator'), - (DomainRequest, 'investigator'), - (UserDomainRole, 'user'), - (VerifiedByStaff, 'requestor'), + (DomainRequest, "creator"), + (DomainInformation, "creator"), + (Portfolio, "creator"), + (DomainRequest, "investigator"), + (UserDomainRole, "user"), + (VerifiedByStaff, "requestor"), ] - USER_FIELDS = ['portfolio', 'portfolio_roles', 'portfolio_additional_permissions'] + USER_FIELDS = ["portfolio", "portfolio_roles", "portfolio_additional_permissions"] def get(self, request, user_id): """current_user referes to the 'source' user where the button that redirects to this view was clicked. other_users exclude current_user and populate a dropdown, selected_user is the selection in the dropdown. - + This also querries the relevant domains and domain requests, and the admin context needed for the sidenav.""" current_user = get_object_or_404(User, pk=user_id) - other_users = User.objects.exclude(pk=user_id).order_by('first_name', 'last_name') # Exclude the current user from the dropdown + other_users = User.objects.exclude(pk=user_id).order_by( + "first_name", "last_name" + ) # Exclude the current user from the dropdown # Get the default admin site context, needed for the sidenav admin_context = site.each_context(request) context = { - 'current_user': current_user, - 'other_users': other_users, - 'logged_in_user': request.user, + "current_user": current_user, + "other_users": other_users, + "logged_in_user": request.user, **admin_context, # Include the admin context - 'current_user_domains': self.get_domains(current_user), - 'current_user_domain_requests': self.get_domain_requests(current_user) + "current_user_domains": self.get_domains(current_user), + "current_user_domain_requests": self.get_domain_requests(current_user), } - selected_user_id = request.GET.get('selected_user') + selected_user_id = request.GET.get("selected_user") if selected_user_id: selected_user = get_object_or_404(User, pk=selected_user_id) - context['selected_user'] = selected_user - context['selected_user_domains'] = self.get_domains(selected_user) - context['selected_user_domain_requests'] = self.get_domain_requests(selected_user) + context["selected_user"] = selected_user + context["selected_user_domains"] = self.get_domains(selected_user) + context["selected_user_domain_requests"] = self.get_domain_requests(selected_user) - return render(request, 'admin/transfer_user.html', context) + return render(request, "admin/transfer_user.html", context) def post(self, request, user_id): """This handles the transfer from selected_user to current_user then deletes selected_user. - + NOTE: We have a ticket to refactor this into a more solid lookup for related fields in #2645""" current_user = get_object_or_404(User, pk=user_id) - selected_user_id = request.POST.get('selected_user') + selected_user_id = request.POST.get("selected_user") selected_user = get_object_or_404(User, pk=selected_user_id) try: @@ -89,53 +92,52 @@ class TransferUserView(View): except Exception as e: messages.error(request, f"An error occurred during the transfer: {e}") - return redirect('admin:registrar_user_change', object_id=user_id) + return redirect("admin:registrar_user_change", object_id=user_id) @classmethod - def update_joins_and_log(cls, model_class, field_name, old_user, new_user, change_logs): + def update_joins_and_log(cls, model_class, field_name, selected_user, current_user, change_logs): """ Helper function to update the user join fields for a given model and log the changes. """ - filter_kwargs = {field_name: old_user} + filter_kwargs = {field_name: selected_user} updated_objects = model_class.objects.filter(**filter_kwargs) for obj in updated_objects: # Check for duplicate UserDomainRole before updating if model_class == UserDomainRole: - if model_class.objects.filter(user=new_user, domain=obj.domain).exists(): + if model_class.objects.filter(user=current_user, domain=obj.domain).exists(): continue # Skip the update to avoid a duplicate # Update the field on the object and save it - setattr(obj, field_name, new_user) + setattr(obj, field_name, current_user) obj.save() # Log the change - cls.log_change(obj, field_name, old_user, new_user, change_logs) + cls.log_change(obj, field_name, selected_user, current_user, change_logs) @classmethod - def transfer_user_fields_and_log(cls, old_user, new_user, change_logs): + def transfer_user_fields_and_log(cls, selected_user, current_user, change_logs): """ - Transfers portfolio fields from the old_user to the new_user. + Transfers portfolio fields from the selected_user to the current_user. Logs the changes for each transferred field. NOTE: This will be refactored in #2644 """ - for field in cls.USER_FIELDS: - old_value = getattr(old_user, field, None) + field_value = getattr(selected_user, field, None) - if old_value: - setattr(new_user, field, old_value) - cls.log_change(new_user, field, old_value, old_value, change_logs) + if field_value: + setattr(current_user, field, field_value) + cls.log_change(current_user, field, field_value, field_value, change_logs) - new_user.save() + current_user.save() @classmethod - def log_change(cls, obj, field_name, old_value, new_value, change_logs): + def log_change(cls, obj, field_name, field_value, new_value, change_logs): """Logs the change for a specific field on an object""" - log_entry = f'Changed {field_name} from "{old_value}" to "{new_value}" on {obj}' - + log_entry = f'Changed {field_name} from "{field_value}" to "{new_value}" on {obj}' + logger.info(log_entry) # Collect the related object for the success message @@ -149,10 +151,10 @@ class TransferUserView(View): domains = Domain.objects.filter(id__in=domain_ids) return domains - + @classmethod def get_domain_requests(cls, user): """A simplified version of domain_requests_json""" domain_requests = DomainRequest.objects.filter(creator=user) - return domain_requests \ No newline at end of file + return domain_requests From 778ad4de6f331ef919d42c30ac1c38a740353f99 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 21:39:36 -0400 Subject: [PATCH 06/32] accessibility fix --- src/registrar/assets/sass/_theme/_admin.scss | 5 ++++- src/registrar/templates/admin/transfer_user.html | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index 40aef1121..b948e2934 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -883,4 +883,7 @@ ul.add-list-reset { .submit-row .select2, .submit-row .select2 span { margin-top: 0; -} \ No newline at end of file +} +.transfer-user-selector .select2-selection__placeholder { + color: #3d4551!important; +} diff --git a/src/registrar/templates/admin/transfer_user.html b/src/registrar/templates/admin/transfer_user.html index a73106979..eee1d1140 100644 --- a/src/registrar/templates/admin/transfer_user.html +++ b/src/registrar/templates/admin/transfer_user.html @@ -38,7 +38,7 @@
    -
    + - +
    {% if selected_user %} - + Transfer and delete old user {% endif %} @@ -197,7 +197,7 @@
    {% csrf_token %} - +
    {% endif %} From 7ba93d05a2bd7a3b76d571d96c2383b8a0238c4e Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 23:50:40 -0400 Subject: [PATCH 08/32] add sample kwarg for url test --- src/registrar/tests/test_url_auth.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/tests/test_url_auth.py b/src/registrar/tests/test_url_auth.py index 3a045498a..284ec7638 100644 --- a/src/registrar/tests/test_url_auth.py +++ b/src/registrar/tests/test_url_auth.py @@ -25,6 +25,7 @@ SAMPLE_KWARGS = { "domain": "whitehouse.gov", "user_pk": "1", "portfolio_id": "1", + "user_id": "1", } # Our test suite will ignore some namespaces. From ec6e5873a4ace4b5e6da09b6f848d13911bf37b8 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 23 Aug 2024 23:56:55 -0400 Subject: [PATCH 09/32] zap exclusion --- src/zap.conf | 1 + 1 file changed, 1 insertion(+) diff --git a/src/zap.conf b/src/zap.conf index c97897aeb..c25406796 100644 --- a/src/zap.conf +++ b/src/zap.conf @@ -72,6 +72,7 @@ 10038 OUTOFSCOPE http://app:8080/domains/ 10038 OUTOFSCOPE http://app:8080/organization/ 10038 OUTOFSCOPE http://app:8080/suborganization/ +10038 OUTOFSCOPE http://app:8080/transfer/ # This URL always returns 404, so include it as well. 10038 OUTOFSCOPE http://app:8080/todo # OIDC isn't configured in the test environment and DEBUG=True so this gives a 500 without CSP headers From 04157ce404e8087608fdf509e51176b38abca980 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Sat, 24 Aug 2024 00:02:52 -0400 Subject: [PATCH 10/32] zap exclusion --- src/zap.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zap.conf b/src/zap.conf index c25406796..dd9ae1565 100644 --- a/src/zap.conf +++ b/src/zap.conf @@ -72,7 +72,7 @@ 10038 OUTOFSCOPE http://app:8080/domains/ 10038 OUTOFSCOPE http://app:8080/organization/ 10038 OUTOFSCOPE http://app:8080/suborganization/ -10038 OUTOFSCOPE http://app:8080/transfer/ +10038 OUTOFSCOPE http://app:8080/transfer/ # This URL always returns 404, so include it as well. 10038 OUTOFSCOPE http://app:8080/todo # OIDC isn't configured in the test environment and DEBUG=True so this gives a 500 without CSP headers From 7dafc863c0f347edb6f2dda353834e53845eb0c8 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 26 Aug 2024 16:14:17 -0500 Subject: [PATCH 11/32] build on the terminal helper a bit --- src/registrar/management/commands/update_first_ready.py | 5 +++-- src/registrar/management/commands/utility/terminal_helper.py | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/registrar/management/commands/update_first_ready.py b/src/registrar/management/commands/update_first_ready.py index aaf04e2a7..4808471ea 100644 --- a/src/registrar/management/commands/update_first_ready.py +++ b/src/registrar/management/commands/update_first_ready.py @@ -19,9 +19,10 @@ class Command(BaseCommand, PopulateScriptTemplate): record.first_ready = record.created_at.date() logger.info( - f"{TerminalColors.OKCYAN}Updating {record} => first_ready: " f"{record.first_ready}{TerminalColors.OKCYAN}" + f"{TerminalColors.OKCYAN}Updating {record} => first_ready: " f"{record.first_ready}{TerminalColors.ENDC}" ) - # check if a transition domain object for this domain name exists, and if so whether + # check if a transition domain object for this domain name exists, + # and if so whether its first_ready value matches its created_at date def should_update(self, record: Domain) -> bool: return TransitionDomain.objects.filter(domain_name=record.name).exists() and record.first_ready != record.created_at.date() \ No newline at end of file diff --git a/src/registrar/management/commands/utility/terminal_helper.py b/src/registrar/management/commands/utility/terminal_helper.py index 13d58191a..c045a2c25 100644 --- a/src/registrar/management/commands/utility/terminal_helper.py +++ b/src/registrar/management/commands/utility/terminal_helper.py @@ -188,7 +188,9 @@ class PopulateScriptTemplate(ABC): return f"{TerminalColors.FAIL}" f"Failed to update {record}" f"{TerminalColors.ENDC}" def should_skip_record(self, record) -> bool: # noqa - """Defines the condition in which we should skip updating a record. Override as needed.""" + """Defines the condition in which we should skip updating a record. Override as needed. + The difference between this and Custom_filter is that records matching these conditions + *will* be included in the run but will be skipped (and logged as such).""" # By default - don't skip return False From f6e10b658522ffa00f3cf4fb86f5b75b47c2afa8 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 27 Aug 2024 14:58:04 -0500 Subject: [PATCH 12/32] Add documentation and refactor custom filtering --- docs/developer/management_script_helpers.md | 3 ++- docs/operations/data_migration.md | 27 +++++++++++++++++++ .../management/commands/clean_tables.py | 2 +- .../commands/extend_expiration_dates.py | 2 +- .../commands/load_organization_data.py | 4 +-- .../commands/load_senior_official_table.py | 2 +- .../commands/load_transition_domain.py | 2 +- .../commands/patch_federal_agency_info.py | 4 +-- .../commands/populate_first_ready.py | 2 +- .../commands/populate_organization_type.py | 4 +-- .../management/commands/update_first_ready.py | 16 +++++++---- .../commands/utility/terminal_helper.py | 26 +++++++----------- 12 files changed, 61 insertions(+), 33 deletions(-) diff --git a/docs/developer/management_script_helpers.md b/docs/developer/management_script_helpers.md index 104e4dc13..a43bb16aa 100644 --- a/docs/developer/management_script_helpers.md +++ b/docs/developer/management_script_helpers.md @@ -62,4 +62,5 @@ The class provides the following optional configuration variables: The class also provides helper methods: - `get_class_name`: Returns a display-friendly class name for the terminal prompt - `get_failure_message`: Returns the message to display if a record fails to update -- `should_skip_record`: Defines the condition for skipping a record (by default, no records are skipped) \ No newline at end of file +- `should_skip_record`: Defines the condition for skipping a record (by default, no records are skipped) +- `custom_filter`: Allows for additional filters that cannot be expressed using django queryset field lookups \ No newline at end of file diff --git a/docs/operations/data_migration.md b/docs/operations/data_migration.md index cd748b22d..75164bf9e 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 | + +## Update First Ready Values +This section outlines how to run the populate_first_ready 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 update_first_ready --debug``` + +### Running locally +```docker-compose exec app ./manage.py update_first_ready --debug``` + +##### Optional parameters +| | Parameter | Description | +|:-:|:-------------------------- |:----------------------------------------------------------------------------| +| 1 | **debug** | Increases logging detail. Defaults to False. | diff --git a/src/registrar/management/commands/clean_tables.py b/src/registrar/management/commands/clean_tables.py index 5d4439d95..66b3e772f 100644 --- a/src/registrar/management/commands/clean_tables.py +++ b/src/registrar/management/commands/clean_tables.py @@ -21,7 +21,7 @@ class Command(BaseCommand): TerminalHelper.prompt_for_execution( system_exit_on_terminate=True, - info_to_inspect=""" + prompt_message=""" This script will delete all rows from the following tables: * Contact * Domain diff --git a/src/registrar/management/commands/extend_expiration_dates.py b/src/registrar/management/commands/extend_expiration_dates.py index cefc38b9e..ac083da1d 100644 --- a/src/registrar/management/commands/extend_expiration_dates.py +++ b/src/registrar/management/commands/extend_expiration_dates.py @@ -130,7 +130,7 @@ class Command(BaseCommand): """Asks if the user wants to proceed with this action""" TerminalHelper.prompt_for_execution( system_exit_on_terminate=True, - info_to_inspect=f""" + prompt_message=f""" ==Extension Amount== Period: {extension_amount} year(s) diff --git a/src/registrar/management/commands/load_organization_data.py b/src/registrar/management/commands/load_organization_data.py index 122795400..35cc248ee 100644 --- a/src/registrar/management/commands/load_organization_data.py +++ b/src/registrar/management/commands/load_organization_data.py @@ -64,7 +64,7 @@ class Command(BaseCommand): # Will sys.exit() when prompt is "n" TerminalHelper.prompt_for_execution( system_exit_on_terminate=True, - info_to_inspect=f""" + prompt_message=f""" ==Master data file== domain_additional_filename: {org_args.domain_additional_filename} @@ -84,7 +84,7 @@ class Command(BaseCommand): # Will sys.exit() when prompt is "n" TerminalHelper.prompt_for_execution( system_exit_on_terminate=True, - info_to_inspect=f""" + prompt_message=f""" ==Master data file== domain_additional_filename: {org_args.domain_additional_filename} diff --git a/src/registrar/management/commands/load_senior_official_table.py b/src/registrar/management/commands/load_senior_official_table.py index 43f61d57a..cdbc607bf 100644 --- a/src/registrar/management/commands/load_senior_official_table.py +++ b/src/registrar/management/commands/load_senior_official_table.py @@ -27,7 +27,7 @@ class Command(BaseCommand): TerminalHelper.prompt_for_execution( system_exit_on_terminate=True, - info_to_inspect=f""" + prompt_message=f""" ==Proposed Changes== CSV: {federal_cio_csv_path} diff --git a/src/registrar/management/commands/load_transition_domain.py b/src/registrar/management/commands/load_transition_domain.py index 4132096c8..c2dd66f55 100644 --- a/src/registrar/management/commands/load_transition_domain.py +++ b/src/registrar/management/commands/load_transition_domain.py @@ -651,7 +651,7 @@ class Command(BaseCommand): title = "Do you wish to load additional data for TransitionDomains?" proceed = TerminalHelper.prompt_for_execution( system_exit_on_terminate=True, - info_to_inspect=f""" + prompt_message=f""" !!! ENSURE THAT ALL FILENAMES ARE CORRECT BEFORE PROCEEDING ==Master data file== domain_additional_filename: {domain_additional_filename} diff --git a/src/registrar/management/commands/patch_federal_agency_info.py b/src/registrar/management/commands/patch_federal_agency_info.py index b286f1516..51a98ffaa 100644 --- a/src/registrar/management/commands/patch_federal_agency_info.py +++ b/src/registrar/management/commands/patch_federal_agency_info.py @@ -91,7 +91,7 @@ class Command(BaseCommand): # Code execution will stop here if the user prompts "N" TerminalHelper.prompt_for_execution( system_exit_on_terminate=True, - info_to_inspect=f""" + prompt_message=f""" ==Proposed Changes== Number of DomainInformation objects to change: {len(human_readable_domain_names)} The following DomainInformation objects will be modified: {human_readable_domain_names} @@ -148,7 +148,7 @@ class Command(BaseCommand): # Code execution will stop here if the user prompts "N" TerminalHelper.prompt_for_execution( system_exit_on_terminate=True, - info_to_inspect=f""" + prompt_message=f""" ==File location== current-full.csv filepath: {file_path} diff --git a/src/registrar/management/commands/populate_first_ready.py b/src/registrar/management/commands/populate_first_ready.py index 9636476c2..04468029a 100644 --- a/src/registrar/management/commands/populate_first_ready.py +++ b/src/registrar/management/commands/populate_first_ready.py @@ -31,7 +31,7 @@ class Command(BaseCommand): # Code execution will stop here if the user prompts "N" TerminalHelper.prompt_for_execution( system_exit_on_terminate=True, - info_to_inspect=f""" + prompt_message=f""" ==Proposed Changes== Number of Domain objects to change: {len(domains)} """, diff --git a/src/registrar/management/commands/populate_organization_type.py b/src/registrar/management/commands/populate_organization_type.py index a7dd98b24..60d179cb8 100644 --- a/src/registrar/management/commands/populate_organization_type.py +++ b/src/registrar/management/commands/populate_organization_type.py @@ -54,7 +54,7 @@ class Command(BaseCommand): # Code execution will stop here if the user prompts "N" TerminalHelper.prompt_for_execution( system_exit_on_terminate=True, - info_to_inspect=f""" + prompt_message=f""" ==Proposed Changes== Number of DomainRequest objects to change: {len(domain_requests)} @@ -72,7 +72,7 @@ class Command(BaseCommand): # Code execution will stop here if the user prompts "N" TerminalHelper.prompt_for_execution( system_exit_on_terminate=True, - info_to_inspect=f""" + prompt_message=f""" ==Proposed Changes== Number of DomainInformation objects to change: {len(domain_infos)} diff --git a/src/registrar/management/commands/update_first_ready.py b/src/registrar/management/commands/update_first_ready.py index 4808471ea..3c7a12928 100644 --- a/src/registrar/management/commands/update_first_ready.py +++ b/src/registrar/management/commands/update_first_ready.py @@ -1,5 +1,6 @@ import logging from django.core.management import BaseCommand +from django.db.models.manager import BaseManager from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors from registrar.models import Domain, TransitionDomain @@ -10,8 +11,8 @@ class Command(BaseCommand, PopulateScriptTemplate): def handle(self, **kwargs): """Loops through each valid Domain object and updates it's first_ready value if it is out of sync""" - filter_conditions={"state__in":[Domain.State.READY, Domain.State.ON_HOLD, Domain.State.DELETED]} - self.mass_update_records(Domain, filter_conditions, ["first_ready"], verbose=True, custom_filter=self.should_update) + filter_conditions={"state__in":[Domain.State.READY, Domain.State.ON_HOLD, Domain.State.DELETED, Domain.State.UNKNOWN]} + self.mass_update_records(Domain, filter_conditions, ["first_ready"], verbose=True) def update_record(self, record: Domain): """Defines how we update the first_ready field""" @@ -23,6 +24,11 @@ class Command(BaseCommand, PopulateScriptTemplate): ) # check if a transition domain object for this domain name exists, - # and if so whether its first_ready value matches its created_at date - def should_update(self, record: Domain) -> bool: - return TransitionDomain.objects.filter(domain_name=record.name).exists() and record.first_ready != record.created_at.date() \ No newline at end of file + # or if so whether its first_ready value matches its created_at date + def custom_filter(self, records: BaseManager[Domain]) -> BaseManager[Domain]: + to_include_pks = [] + for record in records: + if TransitionDomain.objects.filter(domain_name=record.name).exists() and record.first_ready != record.created_at.date(): + to_include_pks.append(record.pk) + + return records.filter(pk__in=to_include_pks) \ No newline at end of file diff --git a/src/registrar/management/commands/utility/terminal_helper.py b/src/registrar/management/commands/utility/terminal_helper.py index c045a2c25..8bb8a5723 100644 --- a/src/registrar/management/commands/utility/terminal_helper.py +++ b/src/registrar/management/commands/utility/terminal_helper.py @@ -3,6 +3,7 @@ import sys from abc import ABC, abstractmethod from django.core.paginator import Paginator from django.db.models import Model +from django.db.models.manager import BaseManager from typing import List from registrar.utility.enums import LogCode @@ -84,7 +85,7 @@ class PopulateScriptTemplate(ABC): """ raise NotImplementedError - def mass_update_records(self, object_class: Model, filter_conditions, fields_to_update, debug=True, verbose=False, custom_filter=None): + def mass_update_records(self, object_class: Model, filter_conditions, fields_to_update, debug=True, verbose=False): """Loops through each valid "object_class" object - specified by filter_conditions - and updates fields defined by fields_to_update using update_record. @@ -104,10 +105,6 @@ class PopulateScriptTemplate(ABC): verbose: Whether to print a detailed run summary *before* run confirmation. Default: False. - custom_filter: function taking in a single record and returning a boolean representing whether - the record should be included of the final record set. Used to allow for filters that can't be - represented by django queryset field lookups. Applied *after* filter_conditions. - Raises: NotImplementedError: If you do not define update_record before using this function. TypeError: If custom_filter is not Callable. @@ -116,16 +113,7 @@ class PopulateScriptTemplate(ABC): records = object_class.objects.filter(**filter_conditions) # apply custom filter - if custom_filter: - to_exclude_pks = [] - for record in records: - try: - if not custom_filter(record): - to_exclude_pks.append(record.pk) - except TypeError as e: - logger.error(f"Error applying custom filter: {e}") - sys.exit() - records=records.exclude(pk__in=to_exclude_pks) + records=self.custom_filter(records) readable_class_name = self.get_class_name(object_class) @@ -189,10 +177,16 @@ class PopulateScriptTemplate(ABC): def should_skip_record(self, record) -> bool: # noqa """Defines the condition in which we should skip updating a record. Override as needed. - The difference between this and Custom_filter is that records matching these conditions + The difference between this and custom_filter is that records matching these conditions *will* be included in the run but will be skipped (and logged as such).""" # By default - don't skip return False + + def custom_filter(self, records: BaseManager[Model]) -> BaseManager[Model]: + """Override to define filters that can't be represented by django queryset field lookups. + Applied to individual records *after* filter_conditions. True means """ + return records + class TerminalHelper: From febd9566cee185af682fb9aec871b8e246218d52 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 27 Aug 2024 15:25:44 -0500 Subject: [PATCH 13/32] linter fixes --- .../management/commands/update_first_ready.py | 14 ++++++++------ .../management/commands/utility/terminal_helper.py | 14 ++++++-------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/registrar/management/commands/update_first_ready.py b/src/registrar/management/commands/update_first_ready.py index 3c7a12928..632635180 100644 --- a/src/registrar/management/commands/update_first_ready.py +++ b/src/registrar/management/commands/update_first_ready.py @@ -4,14 +4,16 @@ from django.db.models.manager import BaseManager from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors from registrar.models import Domain, TransitionDomain + logger = logging.getLogger(__name__) + class Command(BaseCommand, PopulateScriptTemplate): help = "Loops through each domain object and populates the last_status_update and first_submitted_date" def handle(self, **kwargs): """Loops through each valid Domain object and updates it's first_ready value if it is out of sync""" - filter_conditions={"state__in":[Domain.State.READY, Domain.State.ON_HOLD, Domain.State.DELETED, Domain.State.UNKNOWN]} + filter_conditions = {"state__in": [Domain.State.READY, Domain.State.ON_HOLD, Domain.State.DELETED]} self.mass_update_records(Domain, filter_conditions, ["first_ready"], verbose=True) def update_record(self, record: Domain): @@ -22,13 +24,13 @@ class Command(BaseCommand, PopulateScriptTemplate): logger.info( f"{TerminalColors.OKCYAN}Updating {record} => first_ready: " f"{record.first_ready}{TerminalColors.ENDC}" ) - - # check if a transition domain object for this domain name exists, + + # check if a transition domain object for this domain name exists, # or if so whether its first_ready value matches its created_at date def custom_filter(self, records: BaseManager[Domain]) -> BaseManager[Domain]: to_include_pks = [] for record in records: - if TransitionDomain.objects.filter(domain_name=record.name).exists() and record.first_ready != record.created_at.date(): + if TransitionDomain.objects.filter(domain_name=record.name).exists() and record.first_ready != record.created_at.date(): #noqa to_include_pks.append(record.pk) - - return records.filter(pk__in=to_include_pks) \ No newline at end of file + + return records.filter(pk__in=to_include_pks) diff --git a/src/registrar/management/commands/utility/terminal_helper.py b/src/registrar/management/commands/utility/terminal_helper.py index 8bb8a5723..450fbd27a 100644 --- a/src/registrar/management/commands/utility/terminal_helper.py +++ b/src/registrar/management/commands/utility/terminal_helper.py @@ -99,7 +99,7 @@ class PopulateScriptTemplate(ABC): fields_to_update: List of strings specifying which fields to update. (e.g. ["first_ready_date", "last_submitted_date"]) - debug: Whether to log script run summary in debug mode. + debug: Whether to log script run summary in debug mode. Default: True. verbose: Whether to print a detailed run summary *before* run confirmation. @@ -118,13 +118,13 @@ class PopulateScriptTemplate(ABC): readable_class_name = self.get_class_name(object_class) # for use in the execution prompt. - proposed_changes=f"""==Proposed Changes== + proposed_changes = f"""==Proposed Changes== Number of {readable_class_name} objects to change: {len(records)} These fields will be updated on each record: {fields_to_update} """ - + if verbose: - proposed_changes=f"""{proposed_changes} + proposed_changes = f"""{proposed_changes} These records will be updated: {list(records.all())} """ @@ -181,14 +181,12 @@ class PopulateScriptTemplate(ABC): *will* be included in the run but will be skipped (and logged as such).""" # By default - don't skip return False - + def custom_filter(self, records: BaseManager[Model]) -> BaseManager[Model]: - """Override to define filters that can't be represented by django queryset field lookups. + """Override to define filters that can't be represented by django queryset field lookups. Applied to individual records *after* filter_conditions. True means """ return records - - class TerminalHelper: @staticmethod def log_script_run_summary( From 6aa5986441cd491c2eef1dff43a71ee78ffe85e4 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Wed, 28 Aug 2024 09:31:01 -0500 Subject: [PATCH 14/32] Linter fixes --- src/registrar/management/commands/update_first_ready.py | 5 ++++- .../management/commands/utility/terminal_helper.py | 9 +++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/registrar/management/commands/update_first_ready.py b/src/registrar/management/commands/update_first_ready.py index 632635180..f1ebdd555 100644 --- a/src/registrar/management/commands/update_first_ready.py +++ b/src/registrar/management/commands/update_first_ready.py @@ -30,7 +30,10 @@ class Command(BaseCommand, PopulateScriptTemplate): def custom_filter(self, records: BaseManager[Domain]) -> BaseManager[Domain]: to_include_pks = [] for record in records: - if TransitionDomain.objects.filter(domain_name=record.name).exists() and record.first_ready != record.created_at.date(): #noqa + if ( + TransitionDomain.objects.filter(domain_name=record.name).exists() + and record.first_ready != record.created_at.date() + ): # noqa to_include_pks.append(record.pk) return records.filter(pk__in=to_include_pks) diff --git a/src/registrar/management/commands/utility/terminal_helper.py b/src/registrar/management/commands/utility/terminal_helper.py index 450fbd27a..0c7f8ebed 100644 --- a/src/registrar/management/commands/utility/terminal_helper.py +++ b/src/registrar/management/commands/utility/terminal_helper.py @@ -93,7 +93,7 @@ class PopulateScriptTemplate(ABC): object_class: The Django model class that you want to perform the bulk update on. This should be the actual class, not a string of the class name. - filter_conditions: dictionary of valid Django Queryset filter conditions + filter_conditions: dictionary of valid Django Queryset filter conditions (e.g. {'verification_type__isnull'=True}). fields_to_update: List of strings specifying which fields to update. @@ -102,7 +102,7 @@ class PopulateScriptTemplate(ABC): debug: Whether to log script run summary in debug mode. Default: True. - verbose: Whether to print a detailed run summary *before* run confirmation. + verbose: Whether to print a detailed run summary *before* run confirmation. Default: False. Raises: @@ -113,7 +113,7 @@ class PopulateScriptTemplate(ABC): records = object_class.objects.filter(**filter_conditions) # apply custom filter - records=self.custom_filter(records) + records = self.custom_filter(records) readable_class_name = self.get_class_name(object_class) @@ -184,9 +184,10 @@ class PopulateScriptTemplate(ABC): def custom_filter(self, records: BaseManager[Model]) -> BaseManager[Model]: """Override to define filters that can't be represented by django queryset field lookups. - Applied to individual records *after* filter_conditions. True means """ + Applied to individual records *after* filter_conditions. True means""" return records + class TerminalHelper: @staticmethod def log_script_run_summary( From 5f21cd9ab89921939c14035d67b4500d1e890747 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 28 Aug 2024 16:20:54 -0400 Subject: [PATCH 15/32] design review tweaks --- src/registrar/assets/sass/_theme/_admin.scss | 4 + .../templates/admin/transfer_user.html | 114 +++++++++--------- src/registrar/tests/test_admin.py | 2 +- 3 files changed, 64 insertions(+), 56 deletions(-) diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index e6d3cf6fc..a91cdef03 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -886,3 +886,7 @@ ul.add-list-reset { .transfer-user-selector .select2-selection__placeholder { color: #3d4551!important; } + +.dl-dja dt { + font-size: 14px; +} diff --git a/src/registrar/templates/admin/transfer_user.html b/src/registrar/templates/admin/transfer_user.html index ad643b2f4..4e5b4f535 100644 --- a/src/registrar/templates/admin/transfer_user.html +++ b/src/registrar/templates/admin/transfer_user.html @@ -48,7 +48,7 @@ {% endfor %} - +
    @@ -61,64 +61,13 @@
    -
    -
    -

    User to receive data

    -
    -
    -
    Username:
    -
    {{ current_user.username }}
    -
    Created at:
    -
    {{ current_user.created_at }}
    -
    Last login:
    -
    {{ current_user.last_login }}
    -
    First name:
    -
    {{ current_user.first_name }}
    -
    Middle name:
    -
    {{ current_user.middle_name }}
    -
    Last name:
    -
    {{ current_user.last_name }}
    -
    Title:
    -
    {{ current_user.title }}
    -
    Email:
    -
    {{ current_user.email }}
    -
    Phone:
    -
    {{ current_user.phone }}
    -
    Domains:
    -
    - {% if current_user_domains %} -
      - {% for domain in current_user_domains %} -
    • {{ domain }}
    • - {% endfor %} -
    - {% else %} - None - {% endif %} -
    -
    Domain requests:
    -
    - {% if current_user_domain_requests %} -
      - {% for request in current_user_domain_requests %} -
    • {{ request }}
    • - {% endfor %} -
    - {% else %} - None - {% endif %} -
    -
    -
    -
    -
    -

    User to lose data and be deleted

    +

    User to transfer data from

    {% if selected_user %} -
    +
    Username:
    {{ selected_user.username }}
    Created at:
    @@ -137,6 +86,7 @@
    {{ selected_user.email }}
    Phone:
    {{ selected_user.phone }}
    +

    Data that will get transferred:

    Domains:
    {% if selected_user_domains %} @@ -168,6 +118,60 @@
    + +
    +
    +

    User to receive data

    +
    +
    +
    Username:
    +
    {{ current_user.username }}
    +
    Created at:
    +
    {{ current_user.created_at }}
    +
    Last login:
    +
    {{ current_user.last_login }}
    +
    First name:
    +
    {{ current_user.first_name }}
    +
    Middle name:
    +
    {{ current_user.middle_name }}
    +
    Last name:
    +
    {{ current_user.last_name }}
    +
    Title:
    +
    {{ current_user.title }}
    +
    Email:
    +
    {{ current_user.email }}
    +
    Phone:
    +
    {{ current_user.phone }}
    +

     

    +
    Domains:
    +
    + {% if current_user_domains %} +
      + {% for domain in current_user_domains %} +
    • {{ domain }}
    • + {% endfor %} +
    + {% else %} + None + {% endif %} +
    +
    Domain requests:
    +
    + {% if current_user_domain_requests %} +
      + {% for request in current_user_domain_requests %} +
    • {{ request }}
    • + {% endfor %} +
    + {% else %} + None + {% endif %} +
    +
    +
    +
    +
    +
    @@ -184,7 +188,7 @@
    {% if selected_user != logged_in_user %} -

    But you know what you're doing, right?

    +

    This action cannot be undone.

    {% else %}

    Don't do it!

    {% endif %} diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index f051325a6..56fc91c5a 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -2343,4 +2343,4 @@ class TestTransferUser(WebTest): def test_transfer_user_modal(self): """Assert modal on page""" user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) - self.assertContains(user_transfer_page, "But you know what you're doing, right?") + self.assertContains(user_transfer_page, "This action cannot be undone.") From 0076f252fea821ead22f7032a5a1821485545610 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 28 Aug 2024 16:48:49 -0400 Subject: [PATCH 16/32] fix unit test --- src/registrar/tests/test_admin.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 22a2ccea6..d34e81271 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -2119,9 +2119,6 @@ class TestPortfolioAdmin(TestCase): domain_requests = self.admin.domain_requests(self.portfolio) self.assertIn("2 domain requests", domain_requests) - self.assertIn("request1.gov", domain_requests) - self.assertIn("request2.gov", domain_requests) - self.assertIn('
      ', domain_requests) class TestTransferUser(WebTest): From dfdf5ae5550eb12f2f58c828210ccb0323541f15 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 28 Aug 2024 17:24:40 -0400 Subject: [PATCH 17/32] combobox init js --- src/registrar/assets/js/get-gov-admin-extra.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 src/registrar/assets/js/get-gov-admin-extra.js diff --git a/src/registrar/assets/js/get-gov-admin-extra.js b/src/registrar/assets/js/get-gov-admin-extra.js new file mode 100644 index 000000000..da0288056 --- /dev/null +++ b/src/registrar/assets/js/get-gov-admin-extra.js @@ -0,0 +1,14 @@ +// Use Django's jQuery with Select2 to make the user select on the user transfer view a combobox +(function($) { + $(document).ready(function() { + if ($) { + $("#selected_user").select2({ + width: 'resolve', + placeholder: 'Select a user', + allowClear: true + }); + } else { + console.error('jQuery is not available'); + } + }); +})(window.jQuery); \ No newline at end of file From ed8541e1744a15af365803d459f57fb35eec54c3 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 28 Aug 2024 18:17:44 -0400 Subject: [PATCH 18/32] design tweaks --- src/registrar/assets/sass/_theme/_admin.scss | 14 ++++++++++++++ src/registrar/templates/admin/transfer_user.html | 11 +++++++---- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index 88358f704..efb4f1763 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -405,6 +405,20 @@ a.button:active, a.button:focus { .usa-modal { font-family: inherit; } +input[type=submit].button--dja-toolbar { + border: 1px solid var(--border-color); + font-size: 0.8125rem; + padding: 4px 8px; + margin: 0; + vertical-align: middle; + background: var(--body-bg); + box-shadow: 0 -15px 20px -10px rgba(0, 0, 0, 0.15) inset; + cursor: pointer; + color: var(--body-fg); +} +input[type=submit].button--dja-toolbar:focus, input[type=submit].button--dja-toolbar:hover { + border-color: var(--body-quiet-color); +} // Targets the DJA buttom with a nested icon button .usa-icon, .button .usa-icon, diff --git a/src/registrar/templates/admin/transfer_user.html b/src/registrar/templates/admin/transfer_user.html index 4e5b4f535..e49565afc 100644 --- a/src/registrar/templates/admin/transfer_user.html +++ b/src/registrar/templates/admin/transfer_user.html @@ -48,13 +48,13 @@ {% endfor %} - +
    @@ -184,10 +184,13 @@

    - This action will delete {{ selected_user }} + Are you sure you want to transfer data and delete this user?

    {% if selected_user != logged_in_user %} +

    Username: {{ selected_user.username }}
    + Name: {{ selected_user.first_name }} {{ selected_user.last_name }}
    + Email: {{ selected_user.email }}

    This action cannot be undone.

    {% else %}

    Don't do it!

    @@ -201,7 +204,7 @@
    {% csrf_token %} - +
    {% endif %} From e1e812c5c71c7e814acc2b51744d89eae8d2b47f Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 30 Aug 2024 11:51:24 -0400 Subject: [PATCH 19/32] Fix portfolio transfer --- src/registrar/models/user.py | 28 +------------ .../templates/admin/transfer_user.html | 24 +++++++++++ src/registrar/tests/test_admin.py | 41 +++++++++---------- src/registrar/views/transfer_user.py | 18 ++++++-- 4 files changed, 60 insertions(+), 51 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index a7ea1e14a..9cc1492d1 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -6,7 +6,7 @@ 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 +from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices from .domain_invitation import DomainInvitation from .portfolio_invitation import PortfolioInvitation @@ -64,32 +64,6 @@ class User(AbstractUser): # after they login. FIXTURE_USER = "fixture_user", "Created by fixtures" - 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, - ], - } - # #### Constants for choice fields #### RESTRICTED = "restricted" STATUS_CHOICES = ((RESTRICTED, RESTRICTED),) diff --git a/src/registrar/templates/admin/transfer_user.html b/src/registrar/templates/admin/transfer_user.html index e49565afc..9e55346e7 100644 --- a/src/registrar/templates/admin/transfer_user.html +++ b/src/registrar/templates/admin/transfer_user.html @@ -111,6 +111,18 @@ None {% endif %} +
    Portfolios:
    +
    + {% if selected_user_portfolios %} +
      + {% for portfolio in selected_user_portfolios %} +
    • {{ portfolio.portfolio }}
    • + {% endfor %} +
    + {% else %} + None + {% endif %} +
    {% else %}

    No user selected yet.

    @@ -167,6 +179,18 @@ None {% endif %} +
    Portfolios:
    +
    + {% if current_user_portfolios %} +
      + {% for portfolio in current_user_portfolios %} +
    • {{ portfolio.portfolio }}
    • + {% endfor %} +
    + {% else %} + None + {% endif %} +
    diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index d34e81271..0293816fd 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -45,7 +45,8 @@ from registrar.models import ( from registrar.models.portfolio_invitation import PortfolioInvitation from registrar.models.senior_official import SeniorOfficial from registrar.models.user_domain_role import UserDomainRole -from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices +from registrar.models.user_portfolio_permission import UserPortfolioPermission +from registrar.models.utility.portfolio_helper import UserPortfolioRoleChoices from registrar.models.verified_by_staff import VerifiedByStaff from .common import ( MockDbForSharedTests, @@ -2162,6 +2163,14 @@ class TestTransferUser(WebTest): ) domain_request.status = DomainRequest.DomainRequestStatus.APPROVED domain_request.save() + portfolio1 = Portfolio.objects.create(organization_name="Hotel California", creator=self.user2) + UserPortfolioPermission.objects.create( + user=self.user1, portfolio=portfolio1, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + portfolio2 = Portfolio.objects.create(organization_name="Tokyo Hotel", creator=self.user2) + UserPortfolioPermission.objects.create( + user=self.user2, portfolio=portfolio2, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) @@ -2171,6 +2180,7 @@ class TestTransferUser(WebTest): self.assertContains(user_transfer_page, "Road warrior") self.assertContains(user_transfer_page, "wasteland.gov") self.assertContains(user_transfer_page, "citadel.gov") + self.assertContains(user_transfer_page, "Hotel California") select_form = user_transfer_page.forms[0] select_form["selected_user"] = str(self.user2.id) @@ -2180,19 +2190,15 @@ class TestTransferUser(WebTest): self.assertContains(preview_result, "Furiosa") self.assertContains(preview_result, "Jabassa") self.assertContains(preview_result, "Imperator") + self.assertContains(preview_result, "Tokyo Hotel") @less_console_noise_decorator - def test_transfer_user_transfers_portfolio_roles_and_permissions_and_portfolio_creator(self): - """Assert that a portfolio gets copied over - NOTE: Should be revised for #2644""" - portfolio = Portfolio.objects.create(organization_name="Citadel", creator=self.user2) - self.user2.portfolio = portfolio - self.user2.portfolio_roles = [UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - self.user2.portfolio_additional_permissions = [ - UserPortfolioPermissionChoices.VIEW_PORTFOLIO, - UserPortfolioPermissionChoices.EDIT_PORTFOLIO, - ] - self.user2.save() + def test_transfer_user_transfers_user_portfolio_roles(self): + """Assert that a portfolio user role gets transferred""" + portfolio = Portfolio.objects.create(organization_name="Hotel California", creator=self.user2) + user_portfolio_permission = UserPortfolioPermission.objects.create( + user=self.user2, portfolio=portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) @@ -2200,16 +2206,9 @@ class TestTransferUser(WebTest): submit_form["selected_user"] = self.user2.pk submit_form.submit() - self.user1.refresh_from_db() - portfolio.refresh_from_db() + user_portfolio_permission.refresh_from_db() - self.assertEquals(portfolio.creator, self.user1) - self.assertEquals(self.user1.portfolio, portfolio) - self.assertEquals(self.user1.portfolio_roles, [UserPortfolioRoleChoices.ORGANIZATION_ADMIN]) - self.assertEquals( - self.user1.portfolio_additional_permissions, - [UserPortfolioPermissionChoices.VIEW_PORTFOLIO, UserPortfolioPermissionChoices.EDIT_PORTFOLIO], - ) + self.assertEquals(user_portfolio_permission.user, self.user1) @less_console_noise_decorator def test_transfer_user_transfers_domain_request_creator_and_investigator(self): diff --git a/src/registrar/views/transfer_user.py b/src/registrar/views/transfer_user.py index cc901d5ab..ac51cd20b 100644 --- a/src/registrar/views/transfer_user.py +++ b/src/registrar/views/transfer_user.py @@ -11,7 +11,9 @@ from django.contrib.admin import site from django.contrib import messages from registrar.models.user_domain_role import UserDomainRole +from registrar.models.user_portfolio_permission import UserPortfolioPermission from registrar.models.verified_by_staff import VerifiedByStaff +from typing import Any, List logger = logging.getLogger(__name__) @@ -26,9 +28,12 @@ class TransferUserView(View): (DomainRequest, "investigator"), (UserDomainRole, "user"), (VerifiedByStaff, "requestor"), + (UserPortfolioPermission, "user"), ] - USER_FIELDS = ["portfolio", "portfolio_roles", "portfolio_additional_permissions"] + # Future-proofing in case joined fields get added on the user model side + # This was tested in the first portfolio model iteration and works + USER_FIELDS: List[Any] = [] def get(self, request, user_id): """current_user referes to the 'source' user where the button that redirects to this view was clicked. @@ -51,6 +56,7 @@ class TransferUserView(View): **admin_context, # Include the admin context "current_user_domains": self.get_domains(current_user), "current_user_domain_requests": self.get_domain_requests(current_user), + "current_user_portfolios": self.get_portfolios(current_user), } selected_user_id = request.GET.get("selected_user") @@ -59,6 +65,7 @@ class TransferUserView(View): context["selected_user"] = selected_user context["selected_user_domains"] = self.get_domains(selected_user) context["selected_user_domain_requests"] = self.get_domain_requests(selected_user) + context["selected_user_portfolios"] = self.get_portfolios(selected_user) return render(request, "admin/transfer_user.html", context) @@ -121,8 +128,6 @@ class TransferUserView(View): """ Transfers portfolio fields from the selected_user to the current_user. Logs the changes for each transferred field. - - NOTE: This will be refactored in #2644 """ for field in cls.USER_FIELDS: field_value = getattr(selected_user, field, None) @@ -158,3 +163,10 @@ class TransferUserView(View): domain_requests = DomainRequest.objects.filter(creator=user) return domain_requests + + @classmethod + def get_portfolios(cls, user): + """Get portfolios""" + portfolios = UserPortfolioPermission.objects.filter(user=user) + + return portfolios From c4dc3802a8a7ce6fe3ee0194d9875c24706a0652 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 30 Aug 2024 11:56:24 -0400 Subject: [PATCH 20/32] cleanup --- src/registrar/assets/js/get-gov-admin-extra.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/assets/js/get-gov-admin-extra.js b/src/registrar/assets/js/get-gov-admin-extra.js index da0288056..14059267b 100644 --- a/src/registrar/assets/js/get-gov-admin-extra.js +++ b/src/registrar/assets/js/get-gov-admin-extra.js @@ -11,4 +11,4 @@ console.error('jQuery is not available'); } }); -})(window.jQuery); \ No newline at end of file +})(window.jQuery); From dd1b92340401a462d22dbd9df2cdf383e3bd3029 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 4 Sep 2024 13:41:26 -0400 Subject: [PATCH 21/32] wip --- src/registrar/context_processors.py | 6 ++ src/registrar/models/user.py | 56 ++++++++++--------- .../models/user_portfolio_permission.py | 6 +- .../models/utility/portfolio_helper.py | 4 +- .../templates/includes/header_extended.html | 6 +- 5 files changed, 45 insertions(+), 33 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index ea04dca80..beae164c3 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -69,6 +69,8 @@ def portfolio_permissions(request): "has_domain_requests_portfolio_permission": request.user.has_domain_requests_portfolio_permission( portfolio ), + "has_view_members_portfolio_permission": request.user.has_view_members_portfolio_permission(portfolio), + "has_edit_members_portfolio_permission": request.user.has_edit_members_portfolio_permission(portfolio), "has_view_suborganization": request.user.has_view_suborganization(portfolio), "has_edit_suborganization": request.user.has_edit_suborganization(portfolio), "portfolio": portfolio, @@ -78,6 +80,8 @@ def portfolio_permissions(request): "has_base_portfolio_permission": False, "has_domains_portfolio_permission": False, "has_domain_requests_portfolio_permission": False, + "has_view_members_portfolio_permission": False, + "has_edit_members_portfolio_permission": False, "has_view_suborganization": False, "has_edit_suborganization": False, "portfolio": None, @@ -90,6 +94,8 @@ def portfolio_permissions(request): "has_base_portfolio_permission": False, "has_domains_portfolio_permission": False, "has_domain_requests_portfolio_permission": False, + "has_view_members_portfolio_permission": False, + "has_edit_members_portfolio_permission": False, "has_view_suborganization": False, "has_edit_suborganization": False, "portfolio": None, diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index a7ea1e14a..d68104566 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -64,32 +64,6 @@ class User(AbstractUser): # after they login. FIXTURE_USER = "fixture_user", "Created by fixtures" - 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, - ], - } - # #### Constants for choice fields #### RESTRICTED = "restricted" STATUS_CHOICES = ((RESTRICTED, RESTRICTED),) @@ -224,10 +198,40 @@ class User(AbstractUser): ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) def has_domain_requests_portfolio_permission(self, portfolio): + ## BEGIN + ## Note code below is to add organization_request feature + request = HttpRequest() + request.user = self + has_organization_requests_flag = flag_is_active(request, "organization_requests") + if not has_organization_requests_flag: + return False + ## END return self._has_portfolio_permission( portfolio, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) + def has_view_members_portfolio_permission(self, portfolio): + ## BEGIN + ## Note code below is to add organization_request feature + request = HttpRequest() + request.user = self + has_organization_members_flag = flag_is_active(request, "organization_members") + if not has_organization_members_flag: + return False + ## END + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_MEMBERS) + + def has_edit_members_portfolio_permission(self, portfolio): + ## BEGIN + ## Note code below is to add organization_request feature + request = HttpRequest() + request.user = self + has_organization_members_flag = flag_is_active(request, "organization_members") + if not has_organization_members_flag: + return False + ## END + return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_MEMBERS) + 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(portfolio, UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS) diff --git a/src/registrar/models/user_portfolio_permission.py b/src/registrar/models/user_portfolio_permission.py index bf1c3e566..0c2487df3 100644 --- a/src/registrar/models/user_portfolio_permission.py +++ b/src/registrar/models/user_portfolio_permission.py @@ -16,8 +16,8 @@ class UserPortfolioPermission(TimeStampedModel): PORTFOLIO_ROLE_PERMISSIONS = { UserPortfolioRoleChoices.ORGANIZATION_ADMIN: [ UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, - UserPortfolioPermissionChoices.VIEW_MEMBER, - UserPortfolioPermissionChoices.EDIT_MEMBER, + UserPortfolioPermissionChoices.VIEW_MEMBERS, + UserPortfolioPermissionChoices.EDIT_MEMBERS, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, UserPortfolioPermissionChoices.EDIT_REQUESTS, UserPortfolioPermissionChoices.VIEW_PORTFOLIO, @@ -28,7 +28,7 @@ class UserPortfolioPermission(TimeStampedModel): ], UserPortfolioRoleChoices.ORGANIZATION_ADMIN_READ_ONLY: [ UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, - UserPortfolioPermissionChoices.VIEW_MEMBER, + UserPortfolioPermissionChoices.VIEW_MEMBERS, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, UserPortfolioPermissionChoices.VIEW_PORTFOLIO, # Domain: field specific permissions diff --git a/src/registrar/models/utility/portfolio_helper.py b/src/registrar/models/utility/portfolio_helper.py index 86aaa5e16..7afd32603 100644 --- a/src/registrar/models/utility/portfolio_helper.py +++ b/src/registrar/models/utility/portfolio_helper.py @@ -17,8 +17,8 @@ class UserPortfolioPermissionChoices(models.TextChoices): VIEW_ALL_DOMAINS = "view_all_domains", "View all domains and domain reports" VIEW_MANAGED_DOMAINS = "view_managed_domains", "View managed domains" - VIEW_MEMBER = "view_member", "View members" - EDIT_MEMBER = "edit_member", "Create and edit members" + VIEW_MEMBERS = "view_members", "View members" + EDIT_MEMBERS = "edit_members", "Create and edit members" VIEW_ALL_REQUESTS = "view_all_requests", "View all requests" VIEW_CREATED_REQUESTS = "view_created_requests", "View created requests" diff --git a/src/registrar/templates/includes/header_extended.html b/src/registrar/templates/includes/header_extended.html index 54ceb3a67..3bda2c326 100644 --- a/src/registrar/templates/includes/header_extended.html +++ b/src/registrar/templates/includes/header_extended.html @@ -23,11 +23,11 @@ Domains -
  • + {% if has_domain_requests_portfolio_permission %}
  • @@ -37,11 +37,13 @@
  • {% endif %} + {% if has_view_members_portfolio_permission %}
  • Members
  • + {% endif %}
  • {% url 'organization' as url %} From 39e6cb89330ed2a3e43c7c4ed9cdf797ef9179bd Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 4 Sep 2024 11:50:44 -0600 Subject: [PATCH 22/32] Add members --- src/registrar/views/utility/__init__.py | 1 + src/registrar/views/utility/mixins.py | 17 +++++++++++++++++ src/registrar/views/utility/permission_views.py | 9 +++++++++ 3 files changed, 27 insertions(+) diff --git a/src/registrar/views/utility/__init__.py b/src/registrar/views/utility/__init__.py index 7219f4358..7e4e19085 100644 --- a/src/registrar/views/utility/__init__.py +++ b/src/registrar/views/utility/__init__.py @@ -7,5 +7,6 @@ from .permission_views import ( DomainRequestPermissionWithdrawView, DomainInvitationPermissionDeleteView, DomainRequestWizardPermissionView, + PortfolioMembersPermission, ) from .api_views import get_senior_official_from_federal_agency_json diff --git a/src/registrar/views/utility/mixins.py b/src/registrar/views/utility/mixins.py index 6f0745f41..190d80981 100644 --- a/src/registrar/views/utility/mixins.py +++ b/src/registrar/views/utility/mixins.py @@ -454,3 +454,20 @@ class PortfolioDomainRequestsPermission(PortfolioBasePermission): return False return super().has_permission() + + +class PortfolioMembersPermission(PortfolioBasePermission): + """Permission mixin that allows access to portfolio members pages if user + has access, otherwise 403""" + + def has_permission(self): + """Check if this user has access to members for this portfolio. + + 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.session.get("portfolio") + if not self.request.user.has_view_members(portfolio): + return False + + return super().has_permission() diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index 0ff7d1676..734a6dbfe 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -18,6 +18,7 @@ from .mixins import ( UserDeleteDomainRolePermission, UserProfilePermission, PortfolioBasePermission, + PortfolioMembersPermission, ) import logging @@ -229,3 +230,11 @@ class PortfolioDomainRequestsPermissionView(PortfolioDomainRequestsPermission, P This abstract view cannot be instantiated. Actual views must specify `template_name`. """ + + +class PortfolioMembersPermissionView(PortfolioMembersPermission, PortfolioBasePermissionView, abc.ABC): + """Abstract base view for portfolio domain request views that enforces permissions. + + This abstract view cannot be instantiated. Actual views must specify + `template_name`. + """ \ No newline at end of file From dbe438b6dcf4c0208a71da57e2060e85b6c2072e Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 4 Sep 2024 11:52:02 -0600 Subject: [PATCH 23/32] Update context_processors.py --- src/registrar/context_processors.py | 35 +++++++++++------------------ 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/src/registrar/context_processors.py b/src/registrar/context_processors.py index beae164c3..2ac22b2e0 100644 --- a/src/registrar/context_processors.py +++ b/src/registrar/context_processors.py @@ -60,6 +60,17 @@ def add_has_profile_feature_flag_to_context(request): def portfolio_permissions(request): """Make portfolio permissions for the request user available in global context""" + context = { + "has_base_portfolio_permission": False, + "has_domains_portfolio_permission": False, + "has_domain_requests_portfolio_permission": False, + "has_view_members_portfolio_permission": False, + "has_edit_members_portfolio_permission": False, + "has_view_suborganization": False, + "has_edit_suborganization": False, + "portfolio": None, + "has_organization_feature_flag": False, + } try: portfolio = request.session.get("portfolio") if portfolio: @@ -76,28 +87,8 @@ def portfolio_permissions(request): "portfolio": portfolio, "has_organization_feature_flag": True, } - return { - "has_base_portfolio_permission": False, - "has_domains_portfolio_permission": False, - "has_domain_requests_portfolio_permission": False, - "has_view_members_portfolio_permission": False, - "has_edit_members_portfolio_permission": False, - "has_view_suborganization": False, - "has_edit_suborganization": False, - "portfolio": None, - "has_organization_feature_flag": False, - } + return context 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_members_portfolio_permission": False, - "has_edit_members_portfolio_permission": False, - "has_view_suborganization": False, - "has_edit_suborganization": False, - "portfolio": None, - "has_organization_feature_flag": False, - } + return context From d802b51dc47d5abeb854dd111bb70af3e28798b2 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 4 Sep 2024 14:38:58 -0400 Subject: [PATCH 24/32] unit tests and lint --- src/registrar/models/user.py | 20 ++-- src/registrar/tests/test_views_portfolio.py | 98 +++++++++++++++++++ .../views/utility/permission_views.py | 2 +- 3 files changed, 109 insertions(+), 11 deletions(-) diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index d68104566..4b4659094 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -6,7 +6,7 @@ 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 +from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices from .domain_invitation import DomainInvitation from .portfolio_invitation import PortfolioInvitation @@ -198,38 +198,38 @@ class User(AbstractUser): ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS) def has_domain_requests_portfolio_permission(self, portfolio): - ## BEGIN - ## Note code below is to add organization_request feature + # BEGIN + # Note code below is to add organization_request feature request = HttpRequest() request.user = self has_organization_requests_flag = flag_is_active(request, "organization_requests") if not has_organization_requests_flag: return False - ## END + # END return self._has_portfolio_permission( portfolio, UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS ) or self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_CREATED_REQUESTS) def has_view_members_portfolio_permission(self, portfolio): - ## BEGIN - ## Note code below is to add organization_request feature + # BEGIN + # Note code below is to add organization_request feature request = HttpRequest() request.user = self has_organization_members_flag = flag_is_active(request, "organization_members") if not has_organization_members_flag: return False - ## END + # END return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.VIEW_MEMBERS) def has_edit_members_portfolio_permission(self, portfolio): - ## BEGIN - ## Note code below is to add organization_request feature + # BEGIN + # Note code below is to add organization_request feature request = HttpRequest() request.user = self has_organization_members_flag = flag_is_active(request, "organization_members") if not has_organization_members_flag: return False - ## END + # END return self._has_portfolio_permission(portfolio, UserPortfolioPermissionChoices.EDIT_MEMBERS) def has_view_all_domains_permission(self, portfolio): diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index c5d1a9830..807c66cf7 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -230,6 +230,7 @@ class TestPortfolio(WebTest): self.assertContains(response, 'for="id_city"') @less_console_noise_decorator + @override_flag("organization_requests", active=True) def test_accessible_pages_when_user_does_not_have_permission(self): """Tests which pages are accessible when user does not have portfolio permissions""" self.app.set_user(self.user.username) @@ -280,6 +281,7 @@ class TestPortfolio(WebTest): self.assertEquals(domain_request_page.status_code, 403) @less_console_noise_decorator + @override_flag("organization_requests", active=True) def test_accessible_pages_when_user_does_not_have_role(self): """Test that admin / memmber roles are associated with the right access""" self.app.set_user(self.user.username) @@ -532,3 +534,99 @@ class TestPortfolio(WebTest): self.assertEqual(response.status_code, 200) self.assertContains(response, "Domain name") permission.delete() + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_requests", active=False) + def test_organization_requests_waffle_flag_off_hides_nav_link_and_restricts_permission(self): + """Setting the organization_requests waffle off hides the nav link and restricts access to the requests 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_CREATED_REQUESTS, + UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, + UserPortfolioPermissionChoices.EDIT_REQUESTS, + ], + ) + + home = self.app.get(reverse("home")).follow() + + self.assertContains(home, "Hotel California") + self.assertNotContains(home, "Domain requests") + + domain_requests = self.app.get(reverse("domain-requests"), expect_errors=True) + self.assertEqual(domain_requests.status_code, 403) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_requests", active=True) + def test_organization_requests_waffle_flag_on_shows_nav_link_and_allows_permission(self): + """Setting the organization_requests waffle on shows the nav link and allows access to the requests 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_CREATED_REQUESTS, + UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, + UserPortfolioPermissionChoices.EDIT_REQUESTS, + ], + ) + + home = self.app.get(reverse("home")).follow() + + self.assertContains(home, "Hotel California") + self.assertContains(home, "Domain requests") + + domain_requests = self.app.get(reverse("domain-requests")) + self.assertEqual(domain_requests.status_code, 200) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=False) + def test_organization_members_waffle_flag_off_hides_nav_link(self): + """Setting the organization_members waffle off hides the nav link""" + 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_CREATED_REQUESTS, + UserPortfolioPermissionChoices.VIEW_ALL_REQUESTS, + UserPortfolioPermissionChoices.EDIT_REQUESTS, + ], + ) + + home = self.app.get(reverse("home")).follow() + + self.assertContains(home, "Hotel California") + self.assertNotContains(home, "Members") + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_members", active=True) + def test_organization_members_waffle_flag_on_shows_nav_link(self): + """Setting the organization_members waffle on shows the nav link""" + 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_MEMBERS, + ], + ) + + home = self.app.get(reverse("home")).follow() + + self.assertContains(home, "Hotel California") + self.assertContains(home, "Members") diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index 734a6dbfe..e7031cf0d 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -237,4 +237,4 @@ class PortfolioMembersPermissionView(PortfolioMembersPermission, PortfolioBasePe This abstract view cannot be instantiated. Actual views must specify `template_name`. - """ \ No newline at end of file + """ From e5f2b83f6a34b200dcb9c6f6545bf620be31f6e9 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 4 Sep 2024 15:34:54 -0600 Subject: [PATCH 25/32] Fix failing tests --- ...rtfolio_additional_permissions_and_more.py | 66 +++++++++++++++++++ src/registrar/tests/test_models.py | 6 +- 2 files changed, 71 insertions(+), 1 deletion(-) create mode 100644 src/registrar/migrations/0123_alter_portfolioinvitation_portfolio_additional_permissions_and_more.py diff --git a/src/registrar/migrations/0123_alter_portfolioinvitation_portfolio_additional_permissions_and_more.py b/src/registrar/migrations/0123_alter_portfolioinvitation_portfolio_additional_permissions_and_more.py new file mode 100644 index 000000000..c14a70ab0 --- /dev/null +++ b/src/registrar/migrations/0123_alter_portfolioinvitation_portfolio_additional_permissions_and_more.py @@ -0,0 +1,66 @@ +# Generated by Django 4.2.10 on 2024-09-04 21:29 + +import django.contrib.postgres.fields +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0122_create_groups_v16"), + ] + + operations = [ + migrations.AlterField( + model_name="portfolioinvitation", + name="portfolio_additional_permissions", + field=django.contrib.postgres.fields.ArrayField( + base_field=models.CharField( + choices=[ + ("view_all_domains", "View all domains and domain reports"), + ("view_managed_domains", "View managed domains"), + ("view_members", "View members"), + ("edit_members", "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, + ), + ), + migrations.AlterField( + model_name="userportfoliopermission", + name="additional_permissions", + field=django.contrib.postgres.fields.ArrayField( + base_field=models.CharField( + choices=[ + ("view_all_domains", "View all domains and domain reports"), + ("view_managed_domains", "View managed domains"), + ("view_members", "View members"), + ("edit_members", "Create and edit members"), + ("view_all_requests", "View all requests"), + ("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, + ), + ), + ] diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 3a1b21ee2..d0a3e0462 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1474,6 +1474,7 @@ class TestUser(TestCase): self.assertFalse(self.user.has_contact_info()) @less_console_noise_decorator + @override_flag("organization_requests", active=True) def test_has_portfolio_permission(self): """ 0. Returns False when user does not have a permission @@ -1495,7 +1496,10 @@ class TestUser(TestCase): portfolio_permission, _ = UserPortfolioPermission.objects.get_or_create( portfolio=portfolio, user=self.user, - additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS], + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS + ], ) user_can_view_all_domains = self.user.has_domains_portfolio_permission(portfolio) From 49110461ef542f3a230f28cb3e11f32597f73cdc Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 5 Sep 2024 12:10:49 -0400 Subject: [PATCH 26/32] lint --- src/registrar/tests/test_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index d0a3e0462..827f349b9 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1498,7 +1498,7 @@ class TestUser(TestCase): user=self.user, additional_permissions=[ UserPortfolioPermissionChoices.VIEW_PORTFOLIO, - UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS + UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS, ], ) From 467536600efe075c20d7c16288b74eb15b5ec970 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 6 Sep 2024 08:56:27 -0400 Subject: [PATCH 27/32] limit domains_json view to domains based on user's portfolio permissions --- .../tests/test_views_domains_json.py | 106 +++++++++++++++++- src/registrar/views/domains_json.py | 14 ++- 2 files changed, 113 insertions(+), 7 deletions(-) diff --git a/src/registrar/tests/test_views_domains_json.py b/src/registrar/tests/test_views_domains_json.py index 70ae23b43..bf8fe4c0a 100644 --- a/src/registrar/tests/test_views_domains_json.py +++ b/src/registrar/tests/test_views_domains_json.py @@ -1,9 +1,13 @@ from registrar.models import UserDomainRole, Domain, DomainInformation, Portfolio from django.urls import reverse + +from registrar.models.user_portfolio_permission import UserPortfolioPermission +from registrar.models.utility.portfolio_helper import UserPortfolioPermissionChoices, UserPortfolioRoleChoices from .test_views import TestWithUser from django_webtest import WebTest # type: ignore from django.utils.dateparse import parse_date from api.tests.common import less_console_noise_decorator +from waffle.testutils import override_flag class GetDomainsJsonTest(TestWithUser, WebTest): @@ -31,6 +35,7 @@ class GetDomainsJsonTest(TestWithUser, WebTest): def tearDown(self): UserDomainRole.objects.all().delete() + UserPortfolioPermission.objects.all().delete() DomainInformation.objects.all().delete() Portfolio.objects.all().delete() super().tearDown() @@ -115,8 +120,105 @@ class GetDomainsJsonTest(TestWithUser, WebTest): self.assertEqual(svg_icon_expected, svg_icons[i]) @less_console_noise_decorator - def test_get_domains_json_with_portfolio(self): - """Test that an authenticated user gets the list of 2 domains for portfolio.""" + @override_flag("organization_feature", active=True) + def test_get_domains_json_with_portfolio_view_managed_domains(self): + """Test that an authenticated user gets the list of 1 domain for portfolio. The 1 domain + is the domain that they manage within the portfolio.""" + + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + additional_permissions=[UserPortfolioPermissionChoices.VIEW_MANAGED_DOMAINS], + ) + + response = self.app.get(reverse("get_domains_json"), {"portfolio": self.portfolio.id}) + self.assertEqual(response.status_code, 200) + data = response.json + + # Check pagination info + self.assertEqual(data["page"], 1) + self.assertFalse(data["has_next"]) + self.assertFalse(data["has_previous"]) + self.assertEqual(data["num_pages"], 1) + + # Check the number of domains + self.assertEqual(len(data["domains"]), 1) + + # Expected domains + expected_domains = [self.domain3] + + # Extract fields from response + domain_ids = [domain["id"] for domain in data["domains"]] + names = [domain["name"] for domain in data["domains"]] + expiration_dates = [domain["expiration_date"] for domain in data["domains"]] + states = [domain["state"] for domain in data["domains"]] + state_displays = [domain["state_display"] for domain in data["domains"]] + get_state_help_texts = [domain["get_state_help_text"] for domain in data["domains"]] + action_urls = [domain["action_url"] for domain in data["domains"]] + action_labels = [domain["action_label"] for domain in data["domains"]] + svg_icons = [domain["svg_icon"] for domain in data["domains"]] + + # Check fields for each domain + for i, expected_domain in enumerate(expected_domains): + self.assertEqual(expected_domain.id, domain_ids[i]) + self.assertEqual(expected_domain.name, names[i]) + self.assertEqual(expected_domain.expiration_date, expiration_dates[i]) + self.assertEqual(expected_domain.state, states[i]) + + # Parsing the expiration date from string to date + parsed_expiration_date = parse_date(expiration_dates[i]) + expected_domain.expiration_date = parsed_expiration_date + + # Check state_display and get_state_help_text + self.assertEqual(expected_domain.state_display(), state_displays[i]) + self.assertEqual(expected_domain.get_state_help_text(), get_state_help_texts[i]) + + self.assertEqual(reverse("domain", kwargs={"pk": expected_domain.id}), action_urls[i]) + + # Check action_label + user_domain_role_exists = UserDomainRole.objects.filter( + domain_id=expected_domains[i].id, user=self.user + ).exists() + action_label_expected = ( + "View" + if not user_domain_role_exists + or expected_domains[i].state + in [ + Domain.State.DELETED, + Domain.State.ON_HOLD, + ] + else "Manage" + ) + self.assertEqual(action_label_expected, action_labels[i]) + + # Check svg_icon + svg_icon_expected = ( + "visibility" + if not user_domain_role_exists + or expected_domains[i].state + in [ + Domain.State.DELETED, + Domain.State.ON_HOLD, + ] + else "settings" + ) + self.assertEqual(svg_icon_expected, svg_icons[i]) + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + def test_get_domains_json_with_portfolio_view_all_domains(self): + """Test that an authenticated user gets the list of 2 domains for portfolio. One is a domain which + they manage within the portfolio. The other is a domain which they don't manage within the + portfolio.""" + + UserPortfolioPermission.objects.get_or_create( + user=self.user, + portfolio=self.portfolio, + roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], + additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS], + ) + # self.app.set_user(self.user.username) response = self.app.get(reverse("get_domains_json"), {"portfolio": self.portfolio.id}) self.assertEqual(response.status_code, 200) diff --git a/src/registrar/views/domains_json.py b/src/registrar/views/domains_json.py index 06c211227..aaecd33b7 100644 --- a/src/registrar/views/domains_json.py +++ b/src/registrar/views/domains_json.py @@ -50,11 +50,15 @@ def get_domain_ids_from_request(request): """ portfolio = request.GET.get("portfolio") if portfolio: - domain_infos = DomainInformation.objects.filter(portfolio=portfolio) - return domain_infos.values_list("domain_id", flat=True) - else: - user_domain_roles = UserDomainRole.objects.filter(user=request.user) - return user_domain_roles.values_list("domain_id", flat=True) + if request.user.is_org_user(request) and request.user.has_view_all_domains_permission(portfolio): + domain_infos = DomainInformation.objects.filter(portfolio=portfolio) + return domain_infos.values_list("domain_id", flat=True) + else: + domain_info_ids = DomainInformation.objects.filter(portfolio=portfolio).values_list("domain_id", flat=True) + user_domain_roles = UserDomainRole.objects.filter(user=request.user).values_list("domain_id", flat=True) + return domain_info_ids.intersection(user_domain_roles) + user_domain_roles = UserDomainRole.objects.filter(user=request.user) + return user_domain_roles.values_list("domain_id", flat=True) def apply_search(queryset, request): From 008b9d57a596f2f5e74dba743b69f6abb90fbac7 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 6 Sep 2024 08:58:28 -0400 Subject: [PATCH 28/32] cleanup --- src/registrar/tests/test_views_domains_json.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/tests/test_views_domains_json.py b/src/registrar/tests/test_views_domains_json.py index bf8fe4c0a..07799104b 100644 --- a/src/registrar/tests/test_views_domains_json.py +++ b/src/registrar/tests/test_views_domains_json.py @@ -218,7 +218,6 @@ class GetDomainsJsonTest(TestWithUser, WebTest): roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER], additional_permissions=[UserPortfolioPermissionChoices.VIEW_ALL_DOMAINS], ) - # self.app.set_user(self.user.username) response = self.app.get(reverse("get_domains_json"), {"portfolio": self.portfolio.id}) self.assertEqual(response.status_code, 200) From a2291d75ed149b6836cd1335da7fe4c3bdc3336a Mon Sep 17 00:00:00 2001 From: Matthew Spence Date: Fri, 6 Sep 2024 10:27:52 -0500 Subject: [PATCH 29/32] doc fixes --- docs/operations/data_migration.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/operations/data_migration.md b/docs/operations/data_migration.md index 90080520b..4301ca878 100644 --- a/docs/operations/data_migration.md +++ b/docs/operations/data_migration.md @@ -834,10 +834,10 @@ Example: `cf ssh getgov-za` ```/tmp/lifecycle/shell``` #### Step 4: Running the script -```./manage.py populate_domain_request_dates``` +```./manage.py update_first_ready``` ### Running locally -```docker-compose exec app ./manage.py populate_domain_request_dates``` +```docker-compose exec app ./manage.py update_first_ready``` ## Populate Domain Request Dates This section outlines how to run the populate_domain_request_dates script From db593fcb6e7a9022ed73802b529f2a8050fcee5a Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 6 Sep 2024 13:57:28 -0400 Subject: [PATCH 30/32] merge cleanup --- src/registrar/tests/test_admin.py | 175 +++++++++++++++--------------- 1 file changed, 88 insertions(+), 87 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 93942e438..25d7e5fd2 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -2122,6 +2122,94 @@ class TestPortfolioAdmin(TestCase): domain_requests = self.admin.domain_requests(self.portfolio) self.assertIn("2 domain requests", domain_requests) + @less_console_noise_decorator + def test_portfolio_members_display(self): + """Tests the custom portfolio members field, admin and member sections""" + admin_user_1 = User.objects.create( + username="testuser1", + first_name="Gerald", + last_name="Meoward", + title="Captain", + email="meaoward@gov.gov", + ) + + UserPortfolioPermission.objects.all().create( + user=admin_user_1, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + admin_user_2 = User.objects.create( + username="testuser2", + first_name="Arnold", + last_name="Poopy", + title="Major", + email="poopy@gov.gov", + ) + + UserPortfolioPermission.objects.all().create( + user=admin_user_2, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] + ) + + admin_user_3 = User.objects.create( + username="testuser3", + first_name="Mad", + last_name="Max", + title="Road warrior", + email="madmax@gov.gov", + ) + + UserPortfolioPermission.objects.all().create( + user=admin_user_3, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] + ) + + admin_user_4 = User.objects.create( + username="testuser4", + first_name="Agent", + last_name="Smith", + title="Program", + email="thematrix@gov.gov", + ) + + UserPortfolioPermission.objects.all().create( + user=admin_user_4, + portfolio=self.portfolio, + additional_permissions=[ + UserPortfolioPermissionChoices.VIEW_PORTFOLIO, + UserPortfolioPermissionChoices.EDIT_REQUESTS, + ], + ) + + display_admins = self.admin.display_admins(self.portfolio) + + self.assertIn( + f'Gerald Meoward meaoward@gov.gov', + display_admins, + ) + self.assertIn("Captain", display_admins) + self.assertIn( + f'Arnold Poopy poopy@gov.gov', display_admins + ) + self.assertIn("Major", display_admins) + + display_members_summary = self.admin.display_members_summary(self.portfolio) + + self.assertIn( + f'Mad Max madmax@gov.gov', + display_members_summary, + ) + self.assertIn( + f'Agent Smith thematrix@gov.gov', + display_members_summary, + ) + + display_members = self.admin.display_members(self.portfolio) + + self.assertIn("Mad Max", display_members) + self.assertIn("Member", display_members) + self.assertIn("Road warrior", display_members) + self.assertIn("Agent Smith", display_members) + self.assertIn("Domain requestor", display_members) + self.assertIn("Program", display_members) + class TestTransferUser(WebTest): """User transfer custom admin page""" @@ -2340,90 +2428,3 @@ class TestTransferUser(WebTest): """Assert modal on page""" user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk])) self.assertContains(user_transfer_page, "This action cannot be undone.") - @less_console_noise_decorator - def test_portfolio_members_display(self): - """Tests the custom portfolio members field, admin and member sections""" - admin_user_1 = User.objects.create( - username="testuser1", - first_name="Gerald", - last_name="Meoward", - title="Captain", - email="meaoward@gov.gov", - ) - - UserPortfolioPermission.objects.all().create( - user=admin_user_1, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - ) - - admin_user_2 = User.objects.create( - username="testuser2", - first_name="Arnold", - last_name="Poopy", - title="Major", - email="poopy@gov.gov", - ) - - UserPortfolioPermission.objects.all().create( - user=admin_user_2, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] - ) - - admin_user_3 = User.objects.create( - username="testuser3", - first_name="Mad", - last_name="Max", - title="Road warrior", - email="madmax@gov.gov", - ) - - UserPortfolioPermission.objects.all().create( - user=admin_user_3, portfolio=self.portfolio, roles=[UserPortfolioRoleChoices.ORGANIZATION_MEMBER] - ) - - admin_user_4 = User.objects.create( - username="testuser4", - first_name="Agent", - last_name="Smith", - title="Program", - email="thematrix@gov.gov", - ) - - UserPortfolioPermission.objects.all().create( - user=admin_user_4, - portfolio=self.portfolio, - additional_permissions=[ - UserPortfolioPermissionChoices.VIEW_PORTFOLIO, - UserPortfolioPermissionChoices.EDIT_REQUESTS, - ], - ) - - display_admins = self.admin.display_admins(self.portfolio) - - self.assertIn( - f'Gerald Meoward meaoward@gov.gov', - display_admins, - ) - self.assertIn("Captain", display_admins) - self.assertIn( - f'Arnold Poopy poopy@gov.gov', display_admins - ) - self.assertIn("Major", display_admins) - - display_members_summary = self.admin.display_members_summary(self.portfolio) - - self.assertIn( - f'Mad Max madmax@gov.gov', - display_members_summary, - ) - self.assertIn( - f'Agent Smith thematrix@gov.gov', - display_members_summary, - ) - - display_members = self.admin.display_members(self.portfolio) - - self.assertIn("Mad Max", display_members) - self.assertIn("Member", display_members) - self.assertIn("Road warrior", display_members) - self.assertIn("Agent Smith", display_members) - self.assertIn("Domain requestor", display_members) - self.assertIn("Program", display_members) From ffaf3b5a375364fc6e78977bc8600d5b00747ede Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 9 Sep 2024 10:00:34 -0500 Subject: [PATCH 31/32] fix linter errors --- src/registrar/management/commands/update_first_ready.py | 2 +- src/registrar/management/commands/utility/terminal_helper.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/management/commands/update_first_ready.py b/src/registrar/management/commands/update_first_ready.py index f1ebdd555..22320a3d3 100644 --- a/src/registrar/management/commands/update_first_ready.py +++ b/src/registrar/management/commands/update_first_ready.py @@ -27,7 +27,7 @@ class Command(BaseCommand, PopulateScriptTemplate): # check if a transition domain object for this domain name exists, # or if so whether its first_ready value matches its created_at date - def custom_filter(self, records: BaseManager[Domain]) -> BaseManager[Domain]: + def custom_filter(self, records): to_include_pks = [] for record in records: if ( diff --git a/src/registrar/management/commands/utility/terminal_helper.py b/src/registrar/management/commands/utility/terminal_helper.py index e69d54c07..fa7cde683 100644 --- a/src/registrar/management/commands/utility/terminal_helper.py +++ b/src/registrar/management/commands/utility/terminal_helper.py @@ -78,7 +78,7 @@ class PopulateScriptTemplate(ABC): run_summary_header = None @abstractmethod - def update_record(self, record: Model): + def update_record(self, record): """Defines how we update each field. raises: @@ -86,7 +86,7 @@ class PopulateScriptTemplate(ABC): """ raise NotImplementedError - def mass_update_records(self, object_class: Model, filter_conditions, fields_to_update, debug=True, verbose=False): + def mass_update_records(self, object_class, filter_conditions, fields_to_update, debug=True, verbose=False): """Loops through each valid "object_class" object - specified by filter_conditions - and updates fields defined by fields_to_update using update_record. From 872f943220e43274a02cd202801de5b0bb5c825e Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Mon, 9 Sep 2024 10:10:04 -0500 Subject: [PATCH 32/32] unused import --- src/registrar/management/commands/update_first_ready.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/management/commands/update_first_ready.py b/src/registrar/management/commands/update_first_ready.py index 22320a3d3..0a4ea10a7 100644 --- a/src/registrar/management/commands/update_first_ready.py +++ b/src/registrar/management/commands/update_first_ready.py @@ -1,6 +1,5 @@ import logging from django.core.management import BaseCommand -from django.db.models.manager import BaseManager from registrar.management.commands.utility.terminal_helper import PopulateScriptTemplate, TerminalColors from registrar.models import Domain, TransitionDomain