diff --git a/admin/notifications/views.py b/admin/notifications/views.py index fc27b02417b..c66df11a20e 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -19,7 +19,7 @@ from mako.parsetree import ControlLine from string import Formatter from osf.email import _render_email_html -from osf.email.notification_campaign import FILTER_PRESETS, counter_subquery, build_query +from osf.email.notification_campaign import FILTER_PRESETS, counter_subquery, build_campaign_filter_query from website import settings from urllib.parse import urlencode @@ -641,10 +641,7 @@ def get_queryset(self): raw_filters = self.request.GET.get('filters', None) if raw_filters: json_filters = json.loads(raw_filters) - if predefined := json_filters.get('predefined'): - query = Q(**FILTER_PRESETS.get(predefined, {})) - else: - query = build_query(json_filters.get('manual')) + query = build_campaign_filter_query(json_filters) qs = OSFUser.objects.filter(query) qs = qs.annotate( diff --git a/admin/templates/notifications/notification_campaigns_detail.html b/admin/templates/notifications/notification_campaigns_detail.html index 6e2cec6babc..588b05a8b9d 100644 --- a/admin/templates/notifications/notification_campaigns_detail.html +++ b/admin/templates/notifications/notification_campaigns_detail.html @@ -267,25 +267,19 @@

General

Recipient Filters

- {% if not "predefined" in metadata.filters %} - - {% if not "predefined" in metadata.filters %} - {% if metadata.filters.manual %} - {% include "notifications/campaign_filter_group.html" with group=metadata.filters.manual is_root=True %} - {% else %} -

No filters configured.

- {% endif %} - {% endif %} - - {% elif "predefined" in metadata.filters %} - + {% if "predefined" in metadata.filters %}
Predefined Filter {{ metadata.filters.predefined }}
+ {% endif %} + {% if metadata.filters.manual %} + {% include "notifications/campaign_filter_group.html" with group=metadata.filters.manual is_root=True %} + {% elif not "predefined" in metadata.filters %} +

No filters configured.

{% endif %} Recipient Filters
+
+ +

+ When enabled, users who have not confirmed their accounts + are excluded from the recipient list. +

+
+ Execution } function buildFilters() { + let filters; if (filterMode.value === "predefined") { - return { + filters = { "predefined": document.getElementById("filter-row").value }; + } else { + const root = builder.querySelector(":scope > .group"); + filters = {"manual": serializeGroup(root)}; + } + + if (document.getElementById("exclude-unconfirmed").checked) { + const confirmed = { + field: "date_confirmed", + lookup: "isnull", + value: false, + }; + if (filters.manual) { + filters.manual = { + operator: "AND", + children: [filters.manual, confirmed], + }; + } else { + filters.manual = { + operator: "AND", + children: [confirmed], + }; + } } - const root = builder.querySelector(":scope > .group"); - return {"manual": serializeGroup(root)} + return filters; } function updateFiltersInput() { diff --git a/osf/email/notification_campaign.py b/osf/email/notification_campaign.py index d9363594691..e0c44f56f4a 100644 --- a/osf/email/notification_campaign.py +++ b/osf/email/notification_campaign.py @@ -3,7 +3,7 @@ from osf.models import NotificationType, NotificationTypeEnum, OSFUser, UserActivityCounter, Email from osf.models.spam import SpamStatus from django.db import transaction -from django.db.models import OuterRef, Subquery, Case, When, CharField, Count, Q +from django.db.models import OuterRef, Subquery, Case, When, CharField, Count, Q, BooleanField from django.db.models.functions import Coalesce from framework.celery_tasks import app as celery_app from celery import group, chain @@ -54,6 +54,9 @@ def build_query(node): if lookup == 'in': value = [v.strip() for v in value.split(',')] + if lookup == 'isnull': + value = BooleanField().to_python(value) + if lookup in negated_lookups: return ~Q(**{ f'{node["field"]}__{negated_lookups[lookup]}': value @@ -79,6 +82,18 @@ def build_query(node): return query + +def build_campaign_filter_query(filters): + """AND together optional predefined and manual filter clauses.""" + filters = filters or {} + query = Q() + if predefined := filters.get('predefined'): + query &= Q(**FILTER_PRESETS.get(predefined, {})) + if manual := filters.get('manual'): + query &= build_query(manual) + return query + + def create_campaign_recipients(filters, campaign_id): qs = ( OSFUser.objects @@ -260,14 +275,11 @@ def start_notification_campaign(campaign_id, restart_failed=False, restart_stuck if hasattr(NotificationTypeEnum, notification_type_name): del getattr(NotificationTypeEnum, notification_type_name).instance - if predefined_filter_name := filters.get('predefined'): - filters = Q(**FILTER_PRESETS.get(predefined_filter_name, {})) - else: - filters = build_query(filters.get('manual', [])) + recipient_filters = build_campaign_filter_query(filters) if not restart_failed and not restart_stuck: recipients_creation_started_at = timezone.now() - create_campaign_recipients(filters=filters, campaign_id=campaign_id) + create_campaign_recipients(filters=recipient_filters, campaign_id=campaign_id) campaign.recipient_count = NotificationCampaignRecipient.objects.filter(campaign_id=campaign_id).count() campaign.save() recipients_creation_finished_at = timezone.now() diff --git a/osf_tests/test_notification_campaign.py b/osf_tests/test_notification_campaign.py index a26604afe43..890d9522a3a 100644 --- a/osf_tests/test_notification_campaign.py +++ b/osf_tests/test_notification_campaign.py @@ -135,6 +135,61 @@ def test_or_combines_usernames(self): assert plain_user.id in user_ids assert other_email.id not in user_ids + def test_isnull_false_excludes_unconfirmed_accounts(self): + confirmed = UserFactory() + unconfirmed = UserFactory(date_confirmed=None, is_registered=False) + + query = build_query({ + 'operator': 'AND', + 'children': [ + { + 'field': 'id', + 'lookup': 'in', + 'value': f'{confirmed.id},{unconfirmed.id}', + }, + { + 'field': 'date_confirmed', + 'lookup': 'isnull', + 'value': False, + }, + ], + }) + user_ids = set(OSFUser.objects.filter(query).values_list('id', flat=True)) + + assert user_ids == {confirmed.id} + + def test_build_campaign_filter_query_ands_predefined_and_manual(self): + from osf.email.notification_campaign import build_campaign_filter_query + + confirmed = UserFactory() + unconfirmed = UserFactory(date_confirmed=None, is_registered=False) + inactive = UserFactory() + inactive.is_disabled = True + inactive.save() + assert inactive.is_active is False + + query = build_campaign_filter_query({ + 'predefined': 'active', + 'manual': { + 'operator': 'AND', + 'children': [ + { + 'field': 'id', + 'lookup': 'in', + 'value': f'{confirmed.id},{unconfirmed.id},{inactive.id}', + }, + { + 'field': 'date_confirmed', + 'lookup': 'isnull', + 'value': False, + }, + ], + }, + }) + user_ids = set(OSFUser.objects.filter(query).values_list('id', flat=True)) + + assert user_ids == {confirmed.id} + class TestCreateCampaignRecipients: @@ -442,6 +497,44 @@ def test_start_creates_recipients_and_schedules_workflow(self, mock_chain, campa mock_chain.assert_called_once() mock_chain.return_value.apply_async.assert_called_once() + @mock.patch('osf.email.notification_campaign.chain') + def test_start_excludes_unconfirmed_accounts_when_enabled(self, mock_chain, campaign): + confirmed = UserFactory() + unconfirmed = UserFactory(date_confirmed=None, is_registered=False) + _set_activity(confirmed, 100) + _set_activity(unconfirmed, 100) + + campaign.metadata['filters'] = { + 'predefined': 'active', + 'manual': { + 'operator': 'AND', + 'children': [ + { + 'field': 'id', + 'lookup': 'in', + 'value': f'{confirmed.id},{unconfirmed.id}', + }, + { + 'field': 'date_confirmed', + 'lookup': 'isnull', + 'value': False, + }, + ], + }, + } + campaign.run_id = uuid.uuid4() + campaign.save() + mock_chain.return_value.apply_async = mock.Mock() + + start_notification_campaign(campaign.id) + + recipient_ids = set( + NotificationCampaignRecipient.objects.filter(campaign=campaign).values_list( + 'user_id', flat=True + ) + ) + assert recipient_ids == {confirmed.id} + @mock.patch('osf.email.notification_campaign.chain') def test_start_restart_failed_does_not_recreate_recipients(self, mock_chain, campaign): user = UserFactory()