From 883b59257c869d9231f2eb87f269f6068ec96a5b Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Tue, 28 Jun 2022 14:53:24 -0700 Subject: [PATCH 1/9] Change mailing list subscription to when the user validates their email This change also adds support for subscribing the user to an onboarding group. Users in this group will get custom onboarding message(s), to help them along on their usage of the platform. --- .../migrations/0012_add_newsletter_setting.py | 71 +++++++++++++++++++ readthedocs/core/models.py | 35 +++++---- readthedocs/core/signals.py | 49 +++++++++++++ readthedocs/forms.py | 35 ++------- readthedocs/settings/base.py | 1 + 5 files changed, 142 insertions(+), 49 deletions(-) create mode 100644 readthedocs/core/migrations/0012_add_newsletter_setting.py diff --git a/readthedocs/core/migrations/0012_add_newsletter_setting.py b/readthedocs/core/migrations/0012_add_newsletter_setting.py new file mode 100644 index 00000000000..16819cbe83e --- /dev/null +++ b/readthedocs/core/migrations/0012_add_newsletter_setting.py @@ -0,0 +1,71 @@ +# Generated by Django 3.2.13 on 2022-06-28 21:52 + +import django.utils.timezone +import django_extensions.db.fields +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("core", "0011_alter_historicaluser_first_name"), + ] + + operations = [ + migrations.AddField( + model_name="historicaluserprofile", + name="mailing_list", + field=models.BooleanField( + default=False, + help_text="Subscribe to our mailing list, and get helpful onboarding suggestions.", + ), + ), + migrations.AddField( + model_name="userprofile", + name="mailing_list", + field=models.BooleanField( + default=False, + help_text="Subscribe to our mailing list, and get helpful onboarding suggestions.", + ), + ), + migrations.AlterField( + model_name="historicaluserprofile", + name="created", + field=django_extensions.db.fields.CreationDateTimeField( + auto_now_add=True, + default=django.utils.timezone.now, + verbose_name="created", + ), + preserve_default=False, + ), + migrations.AlterField( + model_name="historicaluserprofile", + name="modified", + field=django_extensions.db.fields.ModificationDateTimeField( + auto_now=True, + default=django.utils.timezone.now, + verbose_name="modified", + ), + preserve_default=False, + ), + migrations.AlterField( + model_name="userprofile", + name="created", + field=django_extensions.db.fields.CreationDateTimeField( + auto_now_add=True, + default=django.utils.timezone.now, + verbose_name="created", + ), + preserve_default=False, + ), + migrations.AlterField( + model_name="userprofile", + name="modified", + field=django_extensions.db.fields.ModificationDateTimeField( + auto_now=True, + default=django.utils.timezone.now, + verbose_name="modified", + ), + preserve_default=False, + ), + ] diff --git a/readthedocs/core/models.py b/readthedocs/core/models.py index 364023ef97a..42132529671 100644 --- a/readthedocs/core/models.py +++ b/readthedocs/core/models.py @@ -6,10 +6,6 @@ from django.urls import reverse from django.utils.translation import gettext from django.utils.translation import gettext_lazy as _ -from django_extensions.db.fields import ( - CreationDateTimeField, - ModificationDateTimeField, -) from django_extensions.db.models import TimeStampedModel from simple_history import register @@ -20,33 +16,34 @@ class UserProfile(TimeStampedModel): """Additional information about a User.""" - # TODO: Overridden from TimeStampedModel just to allow null values, - # remove after deploy. - created = CreationDateTimeField( - _('created'), - null=True, - blank=True, - ) - modified = ModificationDateTimeField( - _('modified'), - null=True, - blank=True, - ) - user = AutoOneToOneField( User, verbose_name=_('User'), related_name='profile', on_delete=models.CASCADE, ) - whitelisted = models.BooleanField(_('Whitelisted'), default=False) - banned = models.BooleanField(_('Banned'), default=False) + # Shown on the users profile homepage = models.CharField(_('Homepage'), max_length=100, blank=True) + + # User configuration options allow_ads = models.BooleanField( _('See paid advertising'), help_text=_('If unchecked, you will still see community ads.'), default=True, ) + + mailing_list = models.BooleanField( + default=False, + help_text=_( + "Subscribe to our mailing list, and get helpful onboarding suggestions." + ), + ) + + # Internal tracking + whitelisted = models.BooleanField(_("Whitelisted"), default=False) + banned = models.BooleanField(_("Banned"), default=False) + + # Model history history = ExtraHistoricalRecords() def __str__(self): diff --git a/readthedocs/core/signals.py b/readthedocs/core/signals.py index caa3b24b1dc..d1c73f5e0b3 100644 --- a/readthedocs/core/signals.py +++ b/readthedocs/core/signals.py @@ -1,6 +1,7 @@ """Signal handling for core app.""" import structlog +from allauth.account.signals import email_confirmed from corsheaders import signals from django.conf import settings from django.db.models.signals import pre_delete @@ -33,6 +34,54 @@ post_collectstatic = Signal() +@receiver(email_confirmed) +def process_email_confirmed(request, email_address, **kwargs): + """ + Steps to take after a users email address is confirmed. + + We currently: + + * Subscribe the user to the mailing list if they set this in their signup. + * Add them to a drip campaign for new users in the new user group. + """ + + # email_address is an object + # https://github.com/pennersr/django-allauth/blob/6315e25/allauth/account/models.py#L15 + user = email_address.user + profile = UserProfile.objects.filter(user=user).first() + if profile and profile.mailing_list: + log.bind( + email=email_address.email, + username=user.username, + ) + log.info("Subscribing user to newsletter and onboarding group.") + + # Try subscribing used to a group, then fallback to normal subscription API + url = settings.MAILERLITE_API_ONBOARDING_GROUP_URL + if not url: + url = settings.MAILERLITE_API_SUBSCRIBERS_URL + + payload = { + "email": email_address.email, + "resubscribe": True, + } + headers = { + "X-MailerLite-ApiKey": settings.MAILERLITE_API_KEY, + } + try: + resp = requests.post( + url, + json=payload, + headers=headers, + timeout=3, # seconds + ) + resp.raise_for_status() + except requests.Timeout: + log.warning("Timeout subscribing user to newsletter.") + except Exception: # noqa + log.exception("Unknown error subscribing user to newsletter.") + + def _has_donate_app(): """ Check if the current app has the sustainability API. diff --git a/readthedocs/forms.py b/readthedocs/forms.py index 499886aeed6..3751593f20c 100644 --- a/readthedocs/forms.py +++ b/readthedocs/forms.py @@ -1,11 +1,8 @@ """Community site-wide form overrides.""" import structlog - -import requests - from allauth.account.forms import SignupForm -from django.conf import settings +from core.models import UserProfile from django import forms log = structlog.get_logger(__name__) # noqa @@ -33,32 +30,10 @@ class SignupFormWithNewsletter(SignupForm): def save(self, request): user = super().save(request) - if self.cleaned_data.get("receive_newsletter"): - log.bind( - user_email=self.cleaned_data["email"], - user_username=user.username, - ) - log.info('Subscribing user to newsletter.') + receive_newsletter = self.cleaned_data.get("receive_newsletter") + profile = UserProfile.objects.get_or_create(user=user) + profile.mailing_list = receive_newsletter + profile.save() - url = settings.MAILERLITE_API_SUBSCRIBERS_URL - payload = { - 'email': self.cleaned_data["email"], - 'resubscribe': True, - } - headers = { - 'X-MailerLite-ApiKey': settings.MAILERLITE_API_KEY, - } - try: - resp = requests.post( - url, - json=payload, - headers=headers, - timeout=3, # seconds - ) - resp.raise_for_status() - except requests.Timeout: - log.warning('Timeout subscribing user to newsletter.') - except Exception: # noqa - log.exception('Unknown error subscribing user to newsletter.') return user diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 20356edaaf4..962c218ddd1 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -906,6 +906,7 @@ def DOCKER_LIMITS(self): # MailerLite API for newsletter signups MAILERLITE_API_SUBSCRIBERS_URL = 'https://api.mailerlite.com/api/v2/subscribers' + MAILERLITE_API_ONBOARDING_GROUP_URL = None MAILERLITE_API_KEY = None RTD_EMBED_API_EXTERNAL_DOMAINS = [ From 05581f9d44eaf0d049921f8ebb5eb1fd1f0ff0ab Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Tue, 28 Jun 2022 16:21:26 -0700 Subject: [PATCH 2/9] Update tests --- readthedocs/core/signals.py | 2 + readthedocs/core/tests/test_signup.py | 68 +++++++++++---------------- readthedocs/forms.py | 6 +-- 3 files changed, 32 insertions(+), 44 deletions(-) diff --git a/readthedocs/core/signals.py b/readthedocs/core/signals.py index d1c73f5e0b3..b672924f5c6 100644 --- a/readthedocs/core/signals.py +++ b/readthedocs/core/signals.py @@ -1,5 +1,6 @@ """Signal handling for core app.""" +import requests import structlog from allauth.account.signals import email_confirmed from corsheaders import signals @@ -12,6 +13,7 @@ from readthedocs.analytics.utils import get_client_ip from readthedocs.builds.models import Version +from readthedocs.core.models import UserProfile from readthedocs.core.unresolver import unresolve from readthedocs.organizations.models import Organization from readthedocs.projects.models import Project diff --git a/readthedocs/core/tests/test_signup.py b/readthedocs/core/tests/test_signup.py index cadb809f4a3..6849c3c87df 100644 --- a/readthedocs/core/tests/test_signup.py +++ b/readthedocs/core/tests/test_signup.py @@ -1,62 +1,48 @@ -from http import HTTPStatus from unittest.mock import patch -import requests -import requests_mock - -from django.test import TestCase +from allauth.account.models import EmailAddress +from allauth.account.signals import email_confirmed from django.conf import settings - -from readthedocs.forms import SignupFormWithNewsletter +from django.test import TestCase class TestNewsletterSignup(TestCase): - form_data = { - "email": "test123@gmail.com", - "username": "test123", - "password1": "123456", - "password2": "123456", - } - form_data_plus_checkbox = form_data.copy() - form_data_plus_checkbox["receive_newsletter"] = True - - @patch("readthedocs.forms.requests.post") + def setUp(self): + self.form_data = { + "email": "test123@gmail.com", + "username": "test123", + "password1": "123456", + "password2": "123456", + } + self.form_data_plus_checkbox = self.form_data.copy() + self.form_data_plus_checkbox["receive_newsletter"] = True + + @patch("readthedocs.core.signals.requests.post") def test_signup_without_checkbox_does_not_subscribe(self, mock_requests_post): response = self.client.post("/accounts/signup/", data=self.form_data) - - assert response.status_code == HTTPStatus.FOUND + email_confirmed.send( + sender=None, + request=None, + email_address=EmailAddress.objects.get(email=self.form_data["email"]), + ) mock_requests_post.assert_not_called() - - @patch("readthedocs.forms.requests.post") - def test_signup_with_checkbox_calls_subscribe_api(self, mock_requests_post): + @patch("readthedocs.core.signals.requests.post") + def test_signup_calls_subscribe_api(self, mock_requests_post): response = self.client.post("/accounts/signup/", data=self.form_data_plus_checkbox) - - assert response.status_code == HTTPStatus.FOUND, response + email_confirmed.send( + sender=None, + request=None, + email_address=EmailAddress.objects.get(email=self.form_data["email"]), + ) mock_requests_post.assert_called_with( settings.MAILERLITE_API_SUBSCRIBERS_URL, json={ - "email": self.form_data_plus_checkbox["email"], + "email": self.form_data["email"], "resubscribe": True, }, headers={"X-MailerLite-ApiKey": settings.MAILERLITE_API_KEY}, timeout=3, ) - - @requests_mock.Mocker(kw='mock_request') - def test_signup_with_checkbox_succeeds_if_timeout(self, mock_request): - mock_request.post(settings.MAILERLITE_API_SUBSCRIBERS_URL, exc=requests.Timeout) - - response = self.client.post("/accounts/signup/", data=self.form_data_plus_checkbox) - - assert response.status_code == HTTPStatus.FOUND, response - - @requests_mock.Mocker(kw='mock_request') - def test_signup_with_checkbox_succeeds_if_bad_response(self, mock_request): - mock_request.post(settings.MAILERLITE_API_SUBSCRIBERS_URL, status_code=400) - - response = self.client.post("/accounts/signup/", data=self.form_data_plus_checkbox) - - assert response.status_code == HTTPStatus.FOUND, response diff --git a/readthedocs/forms.py b/readthedocs/forms.py index 3751593f20c..2f4363164c5 100644 --- a/readthedocs/forms.py +++ b/readthedocs/forms.py @@ -2,9 +2,10 @@ import structlog from allauth.account.forms import SignupForm -from core.models import UserProfile from django import forms +from readthedocs.core.models import UserProfile + log = structlog.get_logger(__name__) # noqa @@ -31,9 +32,8 @@ def save(self, request): user = super().save(request) receive_newsletter = self.cleaned_data.get("receive_newsletter") - profile = UserProfile.objects.get_or_create(user=user) + profile, _ = UserProfile.objects.get_or_create(user=user) profile.mailing_list = receive_newsletter profile.save() - return user From d53e0caadf23c6ed9d4e42f2b7aab9992f4f1efe Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 10 Nov 2022 11:52:45 +0100 Subject: [PATCH 3/9] Update readthedocs/core/signals.py Co-authored-by: Santos Gallegos --- readthedocs/core/signals.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/core/signals.py b/readthedocs/core/signals.py index b672924f5c6..139054729f5 100644 --- a/readthedocs/core/signals.py +++ b/readthedocs/core/signals.py @@ -58,7 +58,7 @@ def process_email_confirmed(request, email_address, **kwargs): ) log.info("Subscribing user to newsletter and onboarding group.") - # Try subscribing used to a group, then fallback to normal subscription API + # Try subscribing user to a group, then fallback to normal subscription API url = settings.MAILERLITE_API_ONBOARDING_GROUP_URL if not url: url = settings.MAILERLITE_API_SUBSCRIBERS_URL From 687d17bd1150f66f9d6d63ba120641ba9d953afa Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 10 Nov 2022 11:59:24 +0100 Subject: [PATCH 4/9] Migrate date NULL values to `timezone.now()` --- .../migrations/0012_add_newsletter_setting.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/readthedocs/core/migrations/0012_add_newsletter_setting.py b/readthedocs/core/migrations/0012_add_newsletter_setting.py index 16819cbe83e..2d737843eeb 100644 --- a/readthedocs/core/migrations/0012_add_newsletter_setting.py +++ b/readthedocs/core/migrations/0012_add_newsletter_setting.py @@ -1,8 +1,19 @@ # Generated by Django 3.2.13 on 2022-06-28 21:52 -import django.utils.timezone +from django.utils import timezone import django_extensions.db.fields from django.db import migrations, models +from django.db.models import Q + + +def migrate_null_values(apps, schema_editor): + UserProfile = apps.get_model('core', 'UserProfile') + query = Q(created__isnull=True) | Q(modified__isnull=True) + queryset = UserProfile.objects.filter(query) + now = timezone.now() + for profile in queryset.iterator(): + profile.created = profile.modified = now + profile.save() class Migration(migrations.Migration): @@ -28,6 +39,10 @@ class Migration(migrations.Migration): help_text="Subscribe to our mailing list, and get helpful onboarding suggestions.", ), ), + + # Make NULL values for modified/created to be ``timezone.now()`` + migrations.RunPython(migrate_null_values), + migrations.AlterField( model_name="historicaluserprofile", name="created", From b8231a94933c0bbb0e2c0acb758acfa56c812575 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 10 Nov 2022 12:02:20 +0100 Subject: [PATCH 5/9] TODO note to migrate the signal into a Celery task --- readthedocs/core/signals.py | 1 + 1 file changed, 1 insertion(+) diff --git a/readthedocs/core/signals.py b/readthedocs/core/signals.py index 36df28a8587..8d5a3517963 100644 --- a/readthedocs/core/signals.py +++ b/readthedocs/core/signals.py @@ -71,6 +71,7 @@ def process_email_confirmed(request, email_address, **kwargs): "X-MailerLite-ApiKey": settings.MAILERLITE_API_KEY, } try: + # TODO: migrate this signal to a Celery task since it has a `requests.post` on it. resp = requests.post( url, json=payload, From eeb62b71afde2d865cbb59145ebfd7b0500c799c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 10 Nov 2022 12:34:21 +0100 Subject: [PATCH 6/9] Typo --- readthedocs/core/migrations/0012_add_newsletter_setting.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/core/migrations/0012_add_newsletter_setting.py b/readthedocs/core/migrations/0012_add_newsletter_setting.py index 2d737843eeb..d6742f835bb 100644 --- a/readthedocs/core/migrations/0012_add_newsletter_setting.py +++ b/readthedocs/core/migrations/0012_add_newsletter_setting.py @@ -1,6 +1,6 @@ # Generated by Django 3.2.13 on 2022-06-28 21:52 -from django.utils import timezone +import django.utils.timezone import django_extensions.db.fields from django.db import migrations, models from django.db.models import Q @@ -10,7 +10,7 @@ def migrate_null_values(apps, schema_editor): UserProfile = apps.get_model('core', 'UserProfile') query = Q(created__isnull=True) | Q(modified__isnull=True) queryset = UserProfile.objects.filter(query) - now = timezone.now() + now = django.utils.timezone.now() for profile in queryset.iterator(): profile.created = profile.modified = now profile.save() From fc3eb7099939db4aae404ab9f67bbdc751f5e6a4 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 10 Nov 2022 18:24:24 +0100 Subject: [PATCH 7/9] Darker files --- readthedocs/core/migrations/0012_add_newsletter_setting.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/readthedocs/core/migrations/0012_add_newsletter_setting.py b/readthedocs/core/migrations/0012_add_newsletter_setting.py index d6742f835bb..a38246f622a 100644 --- a/readthedocs/core/migrations/0012_add_newsletter_setting.py +++ b/readthedocs/core/migrations/0012_add_newsletter_setting.py @@ -7,7 +7,7 @@ def migrate_null_values(apps, schema_editor): - UserProfile = apps.get_model('core', 'UserProfile') + UserProfile = apps.get_model("core", "UserProfile") query = Q(created__isnull=True) | Q(modified__isnull=True) queryset = UserProfile.objects.filter(query) now = django.utils.timezone.now() @@ -39,10 +39,8 @@ class Migration(migrations.Migration): help_text="Subscribe to our mailing list, and get helpful onboarding suggestions.", ), ), - # Make NULL values for modified/created to be ``timezone.now()`` migrations.RunPython(migrate_null_values), - migrations.AlterField( model_name="historicaluserprofile", name="created", From 5073d44e10f66bf93c934c7cf4dd2159c4fed0dc Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 14 Nov 2022 10:51:57 +0100 Subject: [PATCH 8/9] Update readthedocs/core/signals.py Co-authored-by: Eric Holscher <25510+ericholscher@users.noreply.github.com> --- readthedocs/core/signals.py | 1 + 1 file changed, 1 insertion(+) diff --git a/readthedocs/core/signals.py b/readthedocs/core/signals.py index 8d5a3517963..99d8aaffc80 100644 --- a/readthedocs/core/signals.py +++ b/readthedocs/core/signals.py @@ -52,6 +52,7 @@ def process_email_confirmed(request, email_address, **kwargs): user = email_address.user profile = UserProfile.objects.filter(user=user).first() if profile and profile.mailing_list: + # TODO: Unsubscribe users if they unset `mailing_list`. log.bind( email=email_address.email, username=user.username, From d63bd5baf1fb2e66ca79097396efbfedf44a2385 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 14 Nov 2022 11:02:00 +0100 Subject: [PATCH 9/9] Darker --- readthedocs/core/signals.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/core/signals.py b/readthedocs/core/signals.py index 99d8aaffc80..bf31ea957b7 100644 --- a/readthedocs/core/signals.py +++ b/readthedocs/core/signals.py @@ -52,7 +52,7 @@ def process_email_confirmed(request, email_address, **kwargs): user = email_address.user profile = UserProfile.objects.filter(user=user).first() if profile and profile.mailing_list: - # TODO: Unsubscribe users if they unset `mailing_list`. + # TODO: Unsubscribe users if they unset `mailing_list`. log.bind( email=email_address.email, username=user.username,