Skip to content

[ENG-11997] Exclude Unconfirmed Accounts from Email Campaigns #11878

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
antkryt wants to merge 1 commit into
base: feature/post-enter-project
Choose a base branch
from
Open

[ENG-11997] Exclude Unconfirmed Accounts from Email Campaigns #11878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions admin/notifications/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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(
Expand Down
18 changes: 6 additions & 12 deletions admin/templates/notifications/notification_campaigns_detail.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,25 +267,19 @@ <h4>General</h4>
<div class="col-md-12">
<h4>Recipient Filters</h4>

{% 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 %}
<p class="text-muted">No filters configured.</p>
{% endif %}
{% endif %}

{% elif "predefined" in metadata.filters %}

{% if "predefined" in metadata.filters %}
<table class="table table-bordered">
<tr>
<th style="width:250px;">Predefined Filter</th>
<td>{{ metadata.filters.predefined }}</td>
</tr>
</table>
{% 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 %}
<p class="text-muted">No filters configured.</p>
{% endif %}

<a
Expand Down
42 changes: 39 additions & 3 deletions admin/templates/notifications/notification_campaing_create.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,20 @@ <h4>Recipient Filters</h4>
</select>
</div>

<div class="form-group" style="margin-top:15px;">
<label>
<input
type="checkbox"
id="exclude-unconfirmed"
>
Exclude unconfirmed accounts
</label>
<p class="help-block">
When enabled, users who have not confirmed their accounts
are excluded from the recipient list.
</p>
</div>

<input
type="hidden"
id="filters-input"
Expand Down Expand Up @@ -406,15 +420,37 @@ <h4>Execution</h4>
}

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",
Comment on lines +436 to +437

@cslzchen cslzchen Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

date_confirmed = NonNaiveDateTimeField(db_index=True, null=True, blank=True)

...

@property
def is_confirmed(self):
    return bool(self.date_confirmed)

Is it possible that date_confirmed is blank instead of null?

@antkryt antkryt Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

date_confirmed is datetime field, postgres timestamptz field cannot be blank ('')

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() {
Expand Down
24 changes: 18 additions & 6 deletions osf/email/notification_campaign.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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()
Expand Down
93 changes: 93 additions & 0 deletions osf_tests/test_notification_campaign.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down Expand Up @@ -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()
Expand Down
Loading