Skip to content

Refactor reminders for draft challenge requests #4729

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
chrisvanrun wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Refactor reminders for draft challenge requests #4729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4b78f3c
Fix frequency of draft reminders
chrisvanrun May 22, 2026
2145197
Simplify test
chrisvanrun May 22, 2026
e37b725
Small comment
chrisvanrun May 22, 2026
9d8b6a7
Add the counter to the settings
chrisvanrun May 22, 2026
99397c1
Merge branch 'main' into 4721-draft-request-reminders-not-send-often-…
chrisvanrun May 22, 2026
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
4 changes: 2 additions & 2 deletions app/config/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,8 +1087,8 @@ def get_private_ip():

CHALLENGE_INVOICE_OVERDUE_CUTOFF = timedelta(weeks=4)

CHALLENGE_REQUEST_AGE_START_REMINDER_CUTOFF = timedelta(days=14)
CHALLENGE_REQUEST_AGE_END_REMINDER_CUTOFF = timedelta(days=31)
CHALLENGE_REQUEST_AGE_START_DRAFT_REMINDER_CUTOFF = timedelta(days=7)
CHALLENGE_REQUEST_MAX_DRAFT_REMINDERS = 2

FORGE_DISABLE_GPUS = False

Expand Down
21 changes: 21 additions & 0 deletions app/grandchallenge/challenges/migrations/0072_challengerequest_draft_reminders_count.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Generated by Django 5.2.14 on 2026-05-22 09:01

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("challenges", "0071_alter_challengerequest_status_and_more"),
]

operations = [
migrations.AddField(
model_name="challengerequest",
name="draft_reminder_count",
field=models.PositiveSmallIntegerField(
default=0,
help_text="Number of reminders sent to the challenge organizers to submit this draft for review.",
),
),
]
6 changes: 6 additions & 0 deletions app/grandchallenge/challenges/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1029,6 +1029,12 @@ class ChallengeRequest(UUIDModel, ChallengeBase):
"This email address will be visible on the challenge website, so please provide an address that you are comfortable sharing publicly."
),
)
draft_reminder_count = models.PositiveSmallIntegerField(
default=0,
Copy link
Copy Markdown
Member

@jmsmkn jmsmkn May 27, 2026

Choose a reason for hiding this comment

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

This field should not be editable.

help_text=(
"Number of reminders sent to the challenge organizers to submit this draft for review."
),
)
start_date = models.DateField(
help_text="Estimated start date for this challenge.",
null=True,
Expand Down
7 changes: 4 additions & 3 deletions app/grandchallenge/challenges/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,9 @@ def send_challenge_request_draft_reminder_emails():
for c in ChallengeRequest.objects.filter(
status=ChallengeRequest.ChallengeRequestStatusChoices.DRAFT,
created__lte=now()
- settings.CHALLENGE_REQUEST_AGE_START_REMINDER_CUTOFF,
created__gte=now()
- settings.CHALLENGE_REQUEST_AGE_END_REMINDER_CUTOFF,
- settings.CHALLENGE_REQUEST_AGE_START_DRAFT_REMINDER_CUTOFF,
draft_reminder_count__lt=settings.CHALLENGE_REQUEST_MAX_DRAFT_REMINDERS,
Comment thread
chrisvanrun marked this conversation as resolved.
):
send_challenge_requests_draft_reminder(challenge_request=c)
c.draft_reminder_count += 1
c.save(update_fields=["draft_reminder_count"])
Copy link
Copy Markdown
Member

@jmsmkn jmsmkn May 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The task is not a singleton so it would be more robust to do this with a single update:

requests = ChallengeRequest.objects.filter(
        status=ChallengeRequest.ChallengeRequestStatusChoices.DRAFT,
        created__lte=now()
        - settings.CHALLENGE_REQUEST_AGE_START_DRAFT_REMINDER_CUTOFF,
        draft_reminder_count__lt=settings.CHALLENGE_REQUEST_MAX_DRAFT_REMINDERS,Expand commentComment on line R248Resolved
    )

for request in requests:
    send_challenge_requests_draft_reminder(challenge_request=request)

requests.update(draft_reminder_count=F("draft_reminder_count") + 1)

Copy link
Copy Markdown
Member Author

@chrisvanrun chrisvanrun May 28, 2026

Choose a reason for hiding this comment

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

It is superior in that it would prevent an additional DB query per reminder! I am not sure why you would say it is more robust? Any failure would be caught by the transaction wrapper, and doing the update in a single call wouldn't catch other spawned (same) tasks to also send the email afaik.

Copy link
Copy Markdown
Member

@jmsmkn jmsmkn May 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The task is not a singleton, so n-tasks can run at the same time. Updating the value in python takes the read value and increments it. If n tasks run at the same time, they read draft_reminder_count as 0, and then all set it to 1. But n emails have been sent. Using F(...) does the update in the database, so the counter gets updated correctly. See https://docs.djangoproject.com/en/6.0/ref/models/expressions/#f-expressions

Copy link
Copy Markdown
Member Author

@chrisvanrun chrisvanrun May 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Aaaaah! The increment is applied at the commit of a transaction if using an F() construct, and a pure increment in that it's not calculating the new value during the transaction but only at the time of commit in the DB. Yep, way robuster!

Today I learned something new! Thank you!

159 changes: 56 additions & 103 deletions app/tests/challenges_tests/test_tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,131 +316,84 @@ def test_challenge_onboarding_task_due_emails(

@pytest.mark.django_db
@pytest.mark.parametrize(
"description, created_offsets, expected_recipients_indices",
"description, created_at, reminder_count, status, expected_email",
[
(
"single valid request",
[
(
"Target",
ChallengeRequest.ChallengeRequestStatusChoices.DRAFT,
timedelta(days=15),
),
],
[0], # 1 email
),
(
"too young request",
[
(
"Too young",
ChallengeRequest.ChallengeRequestStatusChoices.DRAFT,
timedelta(hours=1),
),
],
[], # 0 emails
"first reminder due",
_fixed_now - timedelta(days=15),
0,
ChallengeRequest.ChallengeRequestStatusChoices.DRAFT,
True,
),
(
"too old request",
[
(
"Too old",
ChallengeRequest.ChallengeRequestStatusChoices.DRAFT,
timedelta(days=42),
),
],
[], # 0 emails
"too young for first reminder",
_fixed_now - timedelta(hours=1),
0,
ChallengeRequest.ChallengeRequestStatusChoices.DRAFT,
False,
),
(
"wrong status request",
[
(
"Wrong status",
ChallengeRequest.ChallengeRequestStatusChoices.PENDING,
timedelta(days=15),
),
],
[], # 0 emails
"second reminder due",
_fixed_now - timedelta(days=15),
1,
ChallengeRequest.ChallengeRequestStatusChoices.DRAFT,
True,
),
(
"multiple valid requests",
[
(
"Target 1",
ChallengeRequest.ChallengeRequestStatusChoices.DRAFT,
timedelta(days=15),
),
(
"Target 2",
ChallengeRequest.ChallengeRequestStatusChoices.DRAFT,
timedelta(days=30),
),
],
[0, 1], # 2 emails
"max reminders reached",
_fixed_now - timedelta(days=15),
3,
ChallengeRequest.ChallengeRequestStatusChoices.DRAFT,
False,
),
(
"mix of valid and invalid",
[
(
"Valid",
ChallengeRequest.ChallengeRequestStatusChoices.DRAFT,
timedelta(days=15),
),
(
"Too young",
ChallengeRequest.ChallengeRequestStatusChoices.DRAFT,
timedelta(hours=1),
),
(
"Too old",
ChallengeRequest.ChallengeRequestStatusChoices.DRAFT,
timedelta(days=40),
),
(
"Wrong status",
ChallengeRequest.ChallengeRequestStatusChoices.ACCEPTED,
timedelta(days=15),
),
],
[0], # 1 email
"wrong status",
_fixed_now - timedelta(days=15),
0,
ChallengeRequest.ChallengeRequestStatusChoices.PENDING,
False,
),
],
)
def test_challenge_request_draft_reminder_emails(
mocker, description, created_offsets, expected_recipients_indices
mocker,
settings,
description,
created_at,
reminder_count,
status,
expected_email,
):
# Overwrite settings so the test is not dependent on the actual settings values
settings.CHALLENGE_REQUEST_AGE_START_DRAFT_REMINDER_CUTOFF = timedelta(
days=7
)
settings.CHALLENGE_REQUEST_MAX_DRAFT_REMINDERS = 2

mocker.patch(
"grandchallenge.challenges.tasks.now",
return_value=_fixed_now,
)

requests = []
for title, status, offset in created_offsets:
challenge_request = ChallengeRequestFactory(
title=title,
status=status,
)
challenge_request.created = _fixed_now - offset
challenge_request.save()
requests.append(challenge_request)
challenge_request = ChallengeRequestFactory(
title="Test Request",
status=status,
draft_reminder_count=reminder_count,
)
challenge_request.created = created_at
challenge_request.save()

mail.outbox.clear()

send_challenge_request_draft_reminder_emails()

expected_recipients = [
requests[i].creator.email for i in expected_recipients_indices
]
assert len(mail.outbox) == len(expected_recipients), description

actual_recipients = {r for m in mail.outbox for r in m.recipients()}
assert actual_recipients == set(expected_recipients)

for i in expected_recipients_indices:
user_emails = [
m
for m in mail.outbox
if requests[i].creator.email in m.recipients()
]
assert len(user_emails) == 1
assert requests[i].title in user_emails[0].body
if expected_email:
assert len(mail.outbox) == 1, description
assert challenge_request.creator.email in mail.outbox[0].recipients()
assert challenge_request.title in mail.outbox[0].body
challenge_request.refresh_from_db()
assert challenge_request.draft_reminder_count == reminder_count + 1
else:
assert len(mail.outbox) == 0, description
challenge_request.refresh_from_db()
assert challenge_request.draft_reminder_count == reminder_count
Loading