diff --git a/TWLight/emails/tests.py b/TWLight/emails/tests.py index e22b89d0d9..0f0acec093 100644 --- a/TWLight/emails/tests.py +++ b/TWLight/emails/tests.py @@ -647,6 +647,70 @@ def test_user_renewal_notice_user_already_filed_2_renewals(self): call_command("user_renewal_notice") self.assertEqual(len(mail.outbox), 1) + def test_user_renewal_notice_unrelated_renewals_do_not_suppress(self): + """ + Per T407250, renewal applications for authorizations that are NOT in + the two-week window must not stop the emails for everybody else. + + Two or more such applications used to build SQL of the form + "pk NOT IN ((SELECT ...), (SELECT ...))". An empty sub-query gives + NULL, so the test was never true and no user got an email. + """ + # Two other users expire far in the future, so they are outside the + # window, and both have a renewal application. This gives two empty + # sub-queries. + for i in range(2): + other_editor = EditorFactory(user__email="other{}@example.com".format(i)) + other_partner = PartnerFactory(renewals_available=True) + + other_authorization = Authorization() + other_authorization.user = other_editor.user + other_authorization.authorizer = self.coordinator + other_authorization.date_expires = datetime.today() + timedelta(weeks=30) + other_authorization.save() + other_authorization.partners.add(other_partner) + + other_application = ApplicationFactory( + editor=other_editor, + sent_by=self.coordinator, + partner=other_partner, + status=Application.SENT, + requested_access_duration=1, + ) + other_application.save() + renewed_application = other_application.renew() + renewed_application.status = Application.PENDING + renewed_application.save() + + call_command("user_renewal_notice") + + # The in-window user must still get an email. + self.assertEqual([message.to for message in mail.outbox], [[self.user.email]]) + self.authorization.refresh_from_db() + self.assertTrue(self.authorization.reminder_email_sent) + + def test_user_renewal_notice_multi_partner_sends_one_email(self): + """ + Per T407250, the partners join makes one row for each partner. An + authorization with many partners (a Bundle authorization that has an + expiry date) must get one email, not one email for each partner. + """ + bundle_editor = EditorFactory(user__email="bundle@example.com") + bundle_authorization = Authorization() + bundle_authorization.user = bundle_editor.user + bundle_authorization.authorizer = self.coordinator + bundle_authorization.date_expires = datetime.today() + timedelta(weeks=1) + bundle_authorization.save() + for _ in range(5): + bundle_authorization.partners.add( + PartnerFactory(authorization_method=Partner.BUNDLE) + ) + + call_command("user_renewal_notice") + + recipients = [message.to[0] for message in mail.outbox] + self.assertEqual(recipients.count("bundle@example.com"), 1) + @patch("TWLight.emails.tasks.UserRenewalNotice.send", return_value=0) def test_user_renewal_notice_not_marked_when_send_fails(self, mock_send): """ diff --git a/TWLight/users/management/commands/user_renewal_notice.py b/TWLight/users/management/commands/user_renewal_notice.py index 683c30d27b..255d16dc05 100644 --- a/TWLight/users/management/commands/user_renewal_notice.py +++ b/TWLight/users/management/commands/user_renewal_notice.py @@ -17,7 +17,19 @@ class Command(BaseCommand): help = "Sends advance notice to users with expiring authorizations, prompting them to apply for renewal." + def add_arguments(self, parser): + parser.add_argument( + "--dry-run", + action="store_true", + help=( + "Show the users that would get an email, then stop. " + "This sends no email and changes no data." + ), + ) + def handle(self, *args, **options): + dry_run = options["dry_run"] + # Get all authorization objects with an expiry date in the next # two weeks, for which we haven't yet sent a reminder email, and # exclude users who disabled these emails and who have already filed @@ -45,23 +57,52 @@ def handle(self, *args, **options): partners__isnull=False, ) .exclude(user__userprofile__send_renewal_notices=False) + # The partners join makes one row for each partner. Use distinct() + # so that an authorization with many partners (a Bundle + # authorization that has an expiry date) gets only one email. + .distinct() ) - # Create a list of authorizations that already have a renewal application - no_email_list = [] + # Create a set of the primary keys of the authorizations that already + # have a renewal application. + # + # Collect the primary keys. Do not collect the querysets. A list of + # querysets becomes a list of scalar sub-queries in SQL. An empty + # sub-query gives NULL, and "pk NOT IN (NULL, ...)" is never true. This + # removed every authorization from the result and stopped all of the + # emails (T407250). + no_email_list = set() for application in applications_for_renewal: - no_email_list.append( - expiring_authorizations.values_list("pk").filter( + no_email_list.update( + expiring_authorizations.filter( partners=application["partner__pk"], user=application["editor__user__pk"], - ) + ).values_list("pk", flat=True) ) # Iterate through all expiring authorizations except the ones that have # a renewal + would_email_count = 0 + sent_count = 0 + failed_count = 0 + for authorization_object in expiring_authorizations.exclude( pk__in=no_email_list ): + would_email_count += 1 + + if dry_run: + self.stdout.write( + "[dry-run] would email {email} about {partner} " + "(authorization {pk}, expires {expires})".format( + email=authorization_object.user.email or "(no email address)", + partner=get_company_name(authorization_object), + pk=authorization_object.pk, + expires=authorization_object.date_expires, + ) + ) + continue + try: responses = Notice.user_renewal_notice.send( sender=self.__class__, @@ -87,9 +128,24 @@ def handle(self, *args, **options): if email_sent: authorization_object.reminder_email_sent = True authorization_object.save() + sent_count += 1 else: + failed_count += 1 logger.warning( "Renewal notice was not sent for authorization %s. " "reminder_email_sent stays False for a retry.", authorization_object.pk, ) + + if dry_run: + self.stdout.write( + "[dry-run] {count} email(s) would be sent. No data was changed.".format( + count=would_email_count + ) + ) + else: + self.stdout.write( + "{sent} sent, {failed} failed, out of {total} due for a notice.".format( + sent=sent_count, failed=failed_count, total=would_email_count + ) + ) diff --git a/TWLight/users/management/commands/user_renewal_notice_diagnostics.py b/TWLight/users/management/commands/user_renewal_notice_diagnostics.py index 623bd57901..c48c0a657d 100644 --- a/TWLight/users/management/commands/user_renewal_notice_diagnostics.py +++ b/TWLight/users/management/commands/user_renewal_notice_diagnostics.py @@ -81,19 +81,39 @@ def handle(self, *args, **options): partners__isnull=False, ) .exclude(user__userprofile__send_renewal_notices=False) + # The partners join makes one row for each partner. Use distinct() + # so that an authorization with many partners (a Bundle + # authorization that has an expiry date) gets only one email. + .distinct() ) - no_email_list = [] + # Collect the primary keys, not the querysets. See the note in + # user_renewal_notice.py: a list of querysets makes SQL that removes + # every authorization (T407250). + no_email_list = set() for application in applications_for_renewal: - no_email_list.append( - expiring_authorizations.values_list("pk").filter( + no_email_list.update( + expiring_authorizations.filter( partners=application["partner__pk"], user=application["editor__user__pk"], - ) + ).values_list("pk", flat=True) ) would_email = expiring_authorizations.exclude(pk__in=no_email_list).count() + # Show the result of the old, defective exclusion too. A big difference + # between the two numbers shows that the defect is present. + legacy_no_email_list = [ + expiring_authorizations.values_list("pk").filter( + partners=application["partner__pk"], + user=application["editor__user__pk"], + ) + for application in applications_for_renewal + ] + legacy_would_email = expiring_authorizations.exclude( + pk__in=legacy_no_email_list + ).count() + w("") w( "[Query] authorizations the command WOULD email today: {}".format( @@ -110,6 +130,12 @@ def handle(self, *args, **options): len(no_email_list) ) ) + w( + " same count with the old (defective) exclusion: {}{}".format( + legacy_would_email, + " <-- DEFECT PRESENT" if legacy_would_email != would_email else "", + ) + ) # --- 3. Sticky-flag suppression (Authorization-side, locale-neutral) - marked = Authorization.objects.filter(reminder_email_sent=True) @@ -212,5 +238,9 @@ def status_breakdown(qs): " * would-email=0 AND no in-window auths => genuinely nobody to notify" " (not a bug)." ) + w( + " * 'DEFECT PRESENT' above => the old exclusion removed everybody." + " Deploy the T407250 fix in user_renewal_notice.py." + ) w("") w("Done. No data was modified.")