diff --git a/coldfront/core/allocation/management/commands/send_allowance_renewal_available_emails.py b/coldfront/core/allocation/management/commands/send_allowance_renewal_available_emails.py index 4cbe03c8a3..50ab3bfa63 100644 --- a/coldfront/core/allocation/management/commands/send_allowance_renewal_available_emails.py +++ b/coldfront/core/allocation/management/commands/send_allowance_renewal_available_emails.py @@ -25,19 +25,29 @@ class Command(BaseCommand): help = ( "Send emails to active projects with FCAs (BRC) or PCAs (LRC), " "notifying them that they may renew their computing allowance for the " - "upcoming allowance year." + "upcoming allowance year. By default, emails are sent to all eligible " + "active projects. Pass --not-yet-renewed to send only to those that " + "have not yet submitted a renewal request." ) logger = logging.getLogger("coldfront.commands") - # TODO: Consider only sending the email to projects that have not yet - # renewed, so that this command can be safely re-run for subsequent emails. - def add_arguments(self, parser): add_argparse_dry_run_argument(parser) + parser.add_argument( + "--not-renewed-only", + action="store_true", + dest="not_renewed_only", + default=False, + help=( + "Only send to active projects that have not yet submitted a " + "renewal request for the upcoming allowance year." + ), + ) def handle(self, *args, **options): dry_run = options["dry_run"] + not_renewed_only = options["not_renewed_only"] current_allowance_year = get_current_allowance_year_period() next_allowance_year = get_next_allowance_year_period() @@ -60,6 +70,7 @@ def handle(self, *args, **options): current_allowance_year, next_allowance_year, computing_allowance, + not_renewed_only=not_renewed_only, email_strategy=email_strategy, ) email_sender.run() diff --git a/coldfront/core/allocation/tests/test_commands/test_send_allowance_renewal_available_emails.py b/coldfront/core/allocation/tests/test_commands/test_send_allowance_renewal_available_emails.py new file mode 100644 index 0000000000..25aa5ea3e0 --- /dev/null +++ b/coldfront/core/allocation/tests/test_commands/test_send_allowance_renewal_available_emails.py @@ -0,0 +1,203 @@ +from io import StringIO +import logging +from unittest.mock import patch + +from django.contrib.auth.models import User +from django.core.management import call_command + +from coldfront.core.allocation.models import ( + AllocationRenewalRequest, + AllocationRenewalRequestStatusChoice, +) +from coldfront.core.project.models import ( + Project, + ProjectStatusChoice, + ProjectUser, + ProjectUserRoleChoice, + ProjectUserStatusChoice, +) +from coldfront.core.project.utils_.renewal_utils import ( + AllowanceRenewalAvailableEmailSender, + get_current_allowance_year_period, + get_next_allowance_year_period, +) +from coldfront.core.resource.utils_.allowance_utils.interface import ( + ComputingAllowanceInterface, +) +from coldfront.core.user.models import UserProfile +from coldfront.core.utils.common import utc_now_offset_aware +from coldfront.core.utils.tests.test_base import ( + LRCTestBase, + TestBase, + enable_deployment, +) + + +class SendAllowanceRenewalAvailableEmailsTestMixin: + """Shared tests for the send_allowance_renewal_available_emails command, + exercised under both BRC and LRC deployments via concrete subclasses.""" + + def setUp(self): + super().setUp() + self._deployer = enable_deployment(self._deployment_name) + self._deployer.enable() + + self.current_period = get_current_allowance_year_period() + self.next_period = get_next_allowance_year_period() + + self.pi0 = User.objects.create( + email="pi0@example.com", + first_name="PI0", + last_name="User", + username="pi0", + ) + self.pi1 = User.objects.create( + email="pi1@example.com", + first_name="PI1", + last_name="User", + username="pi1", + ) + for pi in [self.pi0, self.pi1]: + profile = UserProfile.objects.get(user=pi) + profile.is_pi = True + profile.save() + + active_status = ProjectStatusChoice.objects.get(name="Active") + pi_role = ProjectUserRoleChoice.objects.get(name="Principal Investigator") + active_pu_status = ProjectUserStatusChoice.objects.get(name="Active") + + allowance_resource = self.get_predominant_computing_allowance() + self.allowance_resource = allowance_resource + prefix = ComputingAllowanceInterface().code_from_name(allowance_resource.name) + + self.project0 = Project.objects.create( + name=f"{prefix}project0", status=active_status + ) + self.project1 = Project.objects.create( + name=f"{prefix}project1", status=active_status + ) + + for project, pi in [ + (self.project0, self.pi0), + (self.project1, self.pi1), + ]: + ProjectUser.objects.create( + project=project, + user=pi, + role=pi_role, + status=active_pu_status, + ) + + def tearDown(self): + self._deployer.disable() + super().tearDown() + + def _call_command_dry_run(self, *extra_args): + """Call the command with --dry_run and _assert_allocation_period_ready + mocked out. Returns the stdout string.""" + out = StringIO() + with patch.object( + AllowanceRenewalAvailableEmailSender, "_assert_allocation_period_ready" + ): + call_command( + "send_allowance_renewal_available_emails", + "--dry_run", + *extra_args, + stdout=out, + ) + return out.getvalue() + + def _make_renewal_request(self, project, pi, status_name): + """Create an AllocationRenewalRequest for the given project/PI + targeting the next allocation period.""" + status = AllocationRenewalRequestStatusChoice.objects.get(name=status_name) + return AllocationRenewalRequest.objects.create( + requester=pi, + pi=pi, + computing_allowance=self.allowance_resource, + allocation_period=self.next_period, + status=status, + pre_project=project, + post_project=project, + request_time=utc_now_offset_aware(), + ) + + # --- default (all active) --- + + def test_dry_run_logs_all_active_projects(self): + """--dry_run logs a count of all active allowance projects.""" + with self.assertLogs("coldfront.commands", level=logging.INFO) as cm: + self._call_command_dry_run() + self.assertTrue( + any("Would send emails to 2 projects." in msg for msg in cm.output) + ) + + # --- --not-renewed-only --- + + def test_dry_run_not_renewed_only_excludes_renewed(self): + """--not-renewed-only reduces the count when a project has already + submitted a non-denied renewal request.""" + self._make_renewal_request(self.project0, self.pi0, "Under Review") + with self.assertLogs("coldfront.commands", level=logging.INFO) as cm: + self._call_command_dry_run("--not-renewed-only") + self.assertTrue( + any("Would send emails to 1 projects." in msg for msg in cm.output) + ) + + def test_dry_run_not_renewed_only_all_renewed(self): + """--not-renewed-only logs 0 projects when all active projects have + non-denied renewal requests.""" + self._make_renewal_request(self.project0, self.pi0, "Approved") + self._make_renewal_request(self.project1, self.pi1, "Complete") + with self.assertLogs("coldfront.commands", level=logging.INFO) as cm: + self._call_command_dry_run("--not-renewed-only") + self.assertTrue( + any("Would send emails to 0 projects." in msg for msg in cm.output) + ) + + def test_dry_run_not_renewed_only_denied_counts_as_not_renewed(self): + """--not-renewed-only treats a project with only a Denied request as + not yet renewed, so it is still included in the count.""" + self._make_renewal_request(self.project0, self.pi0, "Denied") + with self.assertLogs("coldfront.commands", level=logging.INFO) as cm: + self._call_command_dry_run("--not-renewed-only") + self.assertTrue( + any("Would send emails to 2 projects." in msg for msg in cm.output) + ) + + # --- wet run (confirmed) --- + + def test_confirmed_by_user_logs_sent_count(self): + """Entering 'y' at the confirmation prompt logs the sent count.""" + with self.assertLogs("coldfront.commands", level=logging.INFO) as cm: + with patch.object( + AllowanceRenewalAvailableEmailSender, "_assert_allocation_period_ready" + ): + with patch("builtins.input", return_value="y"): + call_command("send_allowance_renewal_available_emails") + self.assertTrue(any("Sent emails to 2 projects." in msg for msg in cm.output)) + + # --- cancellation --- + + def test_cancelled_by_user_prints_warning(self): + """Entering 'n' at the confirmation prompt prints 'Operation + cancelled.' and returns without sending emails.""" + out = StringIO() + with patch.object( + AllowanceRenewalAvailableEmailSender, "_assert_allocation_period_ready" + ): + with patch("builtins.input", return_value="n"): + call_command("send_allowance_renewal_available_emails", stdout=out) + self.assertIn("Operation cancelled.", out.getvalue()) + + +class TestSendAllowanceRenewalAvailableEmailsBRC( + SendAllowanceRenewalAvailableEmailsTestMixin, TestBase +): + """Run send_allowance_renewal_available_emails tests under BRC (FCA).""" + + +class TestSendAllowanceRenewalAvailableEmailsLRC( + SendAllowanceRenewalAvailableEmailsTestMixin, LRCTestBase +): + """Run send_allowance_renewal_available_emails tests under LRC (PCA).""" diff --git a/coldfront/core/project/tests/test_utils/test_renewal_utils/test_allowance_renewal_available_email_sender.py b/coldfront/core/project/tests/test_utils/test_renewal_utils/test_allowance_renewal_available_email_sender.py new file mode 100644 index 0000000000..e0f522cf05 --- /dev/null +++ b/coldfront/core/project/tests/test_utils/test_renewal_utils/test_allowance_renewal_available_email_sender.py @@ -0,0 +1,252 @@ +from unittest.mock import patch + +from django.contrib.auth.models import User + +from coldfront.core.allocation.models import ( + AllocationRenewalRequest, + AllocationRenewalRequestStatusChoice, +) +from coldfront.core.project.models import ( + Project, + ProjectStatusChoice, + ProjectUser, + ProjectUserRoleChoice, + ProjectUserStatusChoice, +) +from coldfront.core.project.utils_.renewal_utils import ( + AllowanceRenewalAvailableEmailSender, + get_current_allowance_year_period, + get_next_allowance_year_period, +) +from coldfront.core.resource.utils_.allowance_utils.computing_allowance import ( + ComputingAllowance, +) +from coldfront.core.resource.utils_.allowance_utils.interface import ( + ComputingAllowanceInterface, +) +from coldfront.core.user.models import UserProfile +from coldfront.core.utils.common import utc_now_offset_aware +from coldfront.core.utils.email.email_strategy import EnqueueEmailStrategy +from coldfront.core.utils.tests.test_base import ( + LRCTestBase, + TestBase, + enable_deployment, +) + + +class AllowanceRenewalAvailableEmailSenderTestMixin: + """Shared tests for AllowanceRenewalAvailableEmailSender, exercised + under both BRC and LRC deployments via concrete subclasses.""" + + def setUp(self): + super().setUp() + self._deployer = enable_deployment(self._deployment_name) + self._deployer.enable() + + self.current_period = get_current_allowance_year_period() + self.next_period = get_next_allowance_year_period() + + self.pi0 = User.objects.create( + email="pi0@example.com", + first_name="PI0", + last_name="User", + username="pi0", + ) + self.pi1 = User.objects.create( + email="pi1@example.com", + first_name="PI1", + last_name="User", + username="pi1", + ) + self.pi2 = User.objects.create( + email="pi2@example.com", + first_name="PI2", + last_name="User", + username="pi2", + ) + for pi in [self.pi0, self.pi1, self.pi2]: + profile = UserProfile.objects.get(user=pi) + profile.is_pi = True + profile.save() + + active_status = ProjectStatusChoice.objects.get(name="Active") + inactive_status = ProjectStatusChoice.objects.get(name="Inactive") + pi_role = ProjectUserRoleChoice.objects.get(name="Principal Investigator") + active_pu_status = ProjectUserStatusChoice.objects.get(name="Active") + + allowance_resource = self.get_predominant_computing_allowance() + self.allowance_resource = allowance_resource + self.computing_allowance = ComputingAllowance(allowance_resource) + prefix = ComputingAllowanceInterface().code_from_name(allowance_resource.name) + + # Three active projects, one inactive, all with the deployment prefix. + self.project0 = Project.objects.create( + name=f"{prefix}project0", status=active_status + ) + self.project1 = Project.objects.create( + name=f"{prefix}project1", status=active_status + ) + self.project2 = Project.objects.create( + name=f"{prefix}project2", status=active_status + ) + self.project_inactive = Project.objects.create( + name=f"{prefix}project_inactive", status=inactive_status + ) + + for project, pi in [ + (self.project0, self.pi0), + (self.project1, self.pi1), + (self.project2, self.pi2), + (self.project_inactive, self.pi0), + ]: + ProjectUser.objects.create( + project=project, + user=pi, + role=pi_role, + status=active_pu_status, + ) + + def tearDown(self): + self._deployer.disable() + super().tearDown() + + def _make_sender(self, not_renewed_only=False): + """Return an AllowanceRenewalAvailableEmailSender with + _assert_allocation_period_ready mocked out (audit is tested + separately; here we focus on project filtering and email queueing).""" + self.email_strategy = EnqueueEmailStrategy() + with patch.object( + AllowanceRenewalAvailableEmailSender, "_assert_allocation_period_ready" + ): + sender = AllowanceRenewalAvailableEmailSender( + self.current_period, + self.next_period, + self.computing_allowance, + not_renewed_only=not_renewed_only, + email_strategy=self.email_strategy, + ) + return sender + + def _make_renewal_request(self, project, pi, status_name): + """Create and return an AllocationRenewalRequest for the given + project and PI targeting the next allocation period.""" + status = AllocationRenewalRequestStatusChoice.objects.get(name=status_name) + return AllocationRenewalRequest.objects.create( + requester=pi, + pi=pi, + computing_allowance=self.allowance_resource, + allocation_period=self.next_period, + status=status, + pre_project=project, + post_project=project, + request_time=utc_now_offset_aware(), + ) + + def _queued_project_pks(self): + """Return the set of project PKs from the current email queue.""" + return {args[0].pk for _, args, _ in self.email_strategy.get_queue()} + + # --- _get_eligible_projects --- + + def test_eligible_projects_includes_all_active(self): + """Without not_renewed_only, all active allowance projects are + returned.""" + sender = self._make_sender() + pks = {p.pk for p in sender._get_eligible_projects()} + self.assertIn(self.project0.pk, pks) + self.assertIn(self.project1.pk, pks) + self.assertIn(self.project2.pk, pks) + + def test_eligible_projects_excludes_inactive(self): + """Without not_renewed_only, inactive projects are not returned.""" + sender = self._make_sender() + pks = {p.pk for p in sender._get_eligible_projects()} + self.assertNotIn(self.project_inactive.pk, pks) + + def test_not_renewed_only_excludes_under_review(self): + """With not_renewed_only, a project with an Under Review request + for the next period is excluded.""" + self._make_renewal_request(self.project0, self.pi0, "Under Review") + sender = self._make_sender(not_renewed_only=True) + pks = {p.pk for p in sender._get_eligible_projects()} + self.assertNotIn(self.project0.pk, pks) + + def test_not_renewed_only_excludes_approved(self): + """With not_renewed_only, a project with an Approved request for the + next period is excluded.""" + self._make_renewal_request(self.project0, self.pi0, "Approved") + sender = self._make_sender(not_renewed_only=True) + pks = {p.pk for p in sender._get_eligible_projects()} + self.assertNotIn(self.project0.pk, pks) + + def test_not_renewed_only_excludes_complete(self): + """With not_renewed_only, a project with a Complete request for the + next period is excluded.""" + self._make_renewal_request(self.project0, self.pi0, "Complete") + sender = self._make_sender(not_renewed_only=True) + pks = {p.pk for p in sender._get_eligible_projects()} + self.assertNotIn(self.project0.pk, pks) + + def test_not_renewed_only_keeps_denied_only(self): + """With not_renewed_only, a project whose only renewal request for + the next period is Denied is still included.""" + self._make_renewal_request(self.project0, self.pi0, "Denied") + sender = self._make_sender(not_renewed_only=True) + pks = {p.pk for p in sender._get_eligible_projects()} + self.assertIn(self.project0.pk, pks) + + def test_not_renewed_only_keeps_no_request(self): + """With not_renewed_only, projects with no renewal request for the + next period are included.""" + self._make_renewal_request(self.project0, self.pi0, "Under Review") + sender = self._make_sender(not_renewed_only=True) + pks = {p.pk for p in sender._get_eligible_projects()} + self.assertIn(self.project1.pk, pks) + self.assertIn(self.project2.pk, pks) + + # --- run() --- + + def test_run_queues_one_email_per_active_project(self): + """run() without not_renewed_only queues exactly one email per + active project and excludes the inactive project.""" + sender = self._make_sender() + sender.run() + queued_pks = self._queued_project_pks() + self.assertEqual(len(self.email_strategy.get_queue()), 3) + self.assertIn(self.project0.pk, queued_pks) + self.assertIn(self.project1.pk, queued_pks) + self.assertIn(self.project2.pk, queued_pks) + self.assertNotIn(self.project_inactive.pk, queued_pks) + + def test_run_not_renewed_only_skips_renewed_projects(self): + """run() with not_renewed_only only queues emails for projects that + have not yet submitted a non-denied renewal request.""" + self._make_renewal_request(self.project0, self.pi0, "Under Review") + sender = self._make_sender(not_renewed_only=True) + sender.run() + queued_pks = self._queued_project_pks() + self.assertEqual(len(self.email_strategy.get_queue()), 2) + self.assertNotIn(self.project0.pk, queued_pks) + self.assertIn(self.project1.pk, queued_pks) + self.assertIn(self.project2.pk, queued_pks) + + def test_run_not_renewed_only_includes_denied_project(self): + """run() with not_renewed_only queues an email for a project that + only has a Denied renewal request.""" + self._make_renewal_request(self.project0, self.pi0, "Denied") + sender = self._make_sender(not_renewed_only=True) + sender.run() + queued_pks = self._queued_project_pks() + self.assertIn(self.project0.pk, queued_pks) + + +class TestAllowanceRenewalAvailableEmailSenderBRC( + AllowanceRenewalAvailableEmailSenderTestMixin, TestBase +): + """Run AllowanceRenewalAvailableEmailSender tests under BRC (FCA).""" + + +class TestAllowanceRenewalAvailableEmailSenderLRC( + AllowanceRenewalAvailableEmailSenderTestMixin, LRCTestBase +): + """Run AllowanceRenewalAvailableEmailSender tests under LRC (PCA).""" diff --git a/coldfront/core/project/utils_/renewal_utils.py b/coldfront/core/project/utils_/renewal_utils.py index 69b4382576..3c18b74d85 100644 --- a/coldfront/core/project/utils_/renewal_utils.py +++ b/coldfront/core/project/utils_/renewal_utils.py @@ -252,7 +252,7 @@ def non_denied_renewal_request_statuses(): def pis_with_renewal_requests_pks( - allocation_period, computing_allowance=None, request_status_names=[] + allocation_period, computing_allowance=None, request_status_names=None ): """Return a list of primary keys of PIs of allocation renewal requests for the given AllocationPeriod that match the given filters. @@ -274,6 +274,8 @@ def pis_with_renewal_requests_pks( cannot be retrieved. """ assert isinstance(allocation_period, AllocationPeriod) + if request_status_names is None: + request_status_names = [] f = Q(allocation_period=allocation_period) if computing_allowance is not None: assert isinstance(computing_allowance, Resource) @@ -680,6 +682,7 @@ def __init__( current_allocation_period, next_allocation_period, computing_allowance, + not_renewed_only=False, email_strategy=None, ): assert isinstance(current_allocation_period, AllocationPeriod) @@ -710,6 +713,8 @@ def __init__( ) ) + self._not_renewed_only = not_renewed_only + self._email_strategy = validate_email_strategy_or_get_default( email_strategy=email_strategy ) @@ -752,15 +757,30 @@ def _assert_allocation_period_ready(self): raise e def _get_eligible_projects(self): - """Return a list of Projects that are eligible to receive a - reminder email: currently "Active" ones that have the - computing allowance.""" + """Return a queryset of Projects that are eligible to receive a + reminder email: currently "Active" ones that have the computing + allowance. If not_renewed_only is set, further excludes projects + that already have a non-denied renewal request for the next + allocation period. + + Note: this does not exclude projects whose PI has submitted a + non-denied new project request for the next period instead of + renewing. That case is rare enough in practice that it is not + worth the added complexity.""" project_name_prefix = self._computing_allowance_interface.code_from_name( self._computing_allowance.get_name() ) active_projects_with_allowance = Project.objects.filter( name__startswith=project_name_prefix, status__name="Active" ) + if self._not_renewed_only: + already_renewed_project_ids = AllocationRenewalRequest.objects.filter( + allocation_period=self._next_allocation_period, + status__in=non_denied_renewal_request_statuses(), + ).values_list("pre_project_id", flat=True) + active_projects_with_allowance = active_projects_with_allowance.exclude( + pk__in=already_renewed_project_ids + ) return active_projects_with_allowance def _process_email(