Fix T407250: build renewal exclusion from pks

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):
         """

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
+                )
+            )

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.")