Merge ~robru/britney/+git/britney2-ubuntu:send-emails-daily into ~ubuntu-release/britney/+git/britney2-ubuntu:master

[Robert Bruce Park]

Resend emails periodically, such that 2^daysold days pass in between each mail sent, with a maximum of 30, so that emails are never sent less often than monthly (see tests for explicit list of emailing schedule).

[Iain Lane (laney)]

Hmm.

Well, I'm just one reviewer, so take this with appropriate weight (if someone else wants to merge, they can).

I think that the initial schedule is too aggressive. I get that you want to psychologically burden people so that they do the work, but I think that we should be giving people some time to actually do it. As an initial proposal, we can remove the '2', and '4' levels? And an idea to ramp up the pressure if you want to do that is to put in the email subject and/or body the amount of times you've reminded about that upload so far (I guess that it should be possible to calculate this so you don't have to store it).

  Subject: [proposed-migration] glib2.0 is stuck in zesty-proposed (2nd reminder)
  Body: You are bad, this is the 2nd time we've emailed you about this.

Also, some comments/questions inline.

review: Needs Information

[Robert Bruce Park (robru)]

Not sure I understand your review laney, are you saying you want emails sent after 1 day, then 8 days after that, then 16 days after that, then 30 days thereafter?

[Robert Bruce Park (robru)]

Ok, aside from the mail frequency, I think I've addressed all your concerns, please merge. possibly the frequency issue should be decided by vote? Steve wants it to be quite frequent to maximize nagginess.

[Iain Lane (laney)]

Righto - the code looks good to me as it stands so on that basis I'm happy, thanks. I've only eyeballed and ran the testsuite though so I could be missing something.

Still not convinced by the aggressiveness of the frequency. I think the initial emails sent at 1, 2, then 4 days is too much. If there's a problem then I think people should be given some time to sort it out. Sending an email the very day after the first one, which itself is only maybe 1 day after the initial upload, is not giving people a reasonable chance to sort out the problem IMO. 2^n might be nice because it's a neat formula but I think it produces bad results. To strawman, I personally think that there should be at least 3 clear days between nags. Backing off as we get further away from the upload date is a nice feature though - I think that aspect should be retained.

Like I said initially, if the consensus of the rest of the team is to disagree with me then somebody else could merge.

(And I still think it would be cute to say in the subject that it's the nth (where n > 1) reminder, but that's not a requirement.)

[Iain Lane (laney)]

Ok, it now has a 3 day threshold for everything (valid or not valid).

I think that *might* be acceptable, so let's go with it and see if it turns out to be.

If we need to go back to the 1 and 5 cases, then I propose to set a lower bound on the frequency of 3 days but we'll see when we come to it.

review: Approve

diff --git a/britney2/policies/email.py b/britney2/policies/email.py
index 60947c8..c488e58 100644
--- a/britney2/policies/email.py
+++ b/britney2/policies/email.py
@@ -11,6 +11,9 @@ from britney2.policies.rest import Rest
 from britney2.policies.policy import BasePolicy, PolicyVerdict
 
 
+# Recurring emails should never be more than this many days apart
+MAX_FREQUENCY = 30
+
 API_PREFIX = 'https://api.launchpad.net/1.0/'
 USER = API_PREFIX + '~'
 
@@ -25,13 +28,13 @@ BOTS = {
 MESSAGE = """From: Ubuntu Release Team <[e-mail]>
 To: {recipients}
 X-Proposed-Migration: notice
-Subject: [proposed-migration] {source_name} {version} stuck in {series}-proposed
+Subject: [proposed-migration] {source_name} {version} stuck in {series}-proposed for {rounded_age} days.
 
 Hi,
 
 {source_name} {version} needs attention.
 
-It has been stuck in {series}-proposed for {age} day{plural}.
+It has been stuck in {series}-proposed for {rounded_age} days.
 
 You either sponsored or uploaded this package, please investigate why it hasn't been approved for migration.
 
@@ -82,6 +85,8 @@ class EmailPolicy(BasePolicy, Rest):
     def __init__(self, options, suite_info, dry_run=False):
         super().__init__('email', options, suite_info, {'unstable'})
         self.filename = os.path.join(options.unstable, 'EmailCache')
+        # Maps lp username -> email address
+        self.addresses = {}
         # Dict of dicts; maps pkg name -> pkg version -> boolean
         self.emails_by_pkg = defaultdict(dict)
         # self.cache contains self.emails_by_pkg from previous run
@@ -99,6 +104,8 @@ class EmailPolicy(BasePolicy, Rest):
 
     def _scrape_gpg_emails(self, person):
         """Find email addresses from one person's GPG keys."""
+        if person in self.addresses:
+            return self.addresses[person]
         addresses = []
         gpg = self.query_lp_rest_api(person + '/gpg_keys', {})
         for key in gpg['entries']:
@@ -121,15 +128,12 @@ class EmailPolicy(BasePolicy, Rest):
                     match = re.match(r'^.*<(.+@.+)>$', uid)
                     if match:
                         addresses.append(match.group(1))
-        return addresses
+        address = self.addresses[person] = address_chooser(addresses)
+        return address
 
     def scrape_gpg_emails(self, people):
         """Find email addresses from GPG keys."""
-        addresses = []
-        for person in people or []:
-            address = address_chooser(self._scrape_gpg_emails(person))
-            addresses.append(address)
-        return addresses
+        return [self._scrape_gpg_emails(person) for person in (people or [])]
 
     def lp_get_emails(self, pkg, version):
         """Ask LP who uploaded this package."""
@@ -153,15 +157,24 @@ class EmailPolicy(BasePolicy, Rest):
 
     def apply_policy_impl(self, email_info, suite, source_name, source_data_tdist, source_data_srcdist, excuse):
         """Send email if package is rejected."""
-        # Have more patience for things blocked in update_output.txt
-        max_age = 5 if excuse.is_valid else 1
         series = self.options.series
         version = source_data_srcdist.version
-        sent = self.cache.get(source_name, {}).get(version, False)
         age = excuse.daysold or 0
-        stuck = age >= max_age
-        age = int(age)
-        plural = 's' if age != 1 else ''
+        rounded_age = int(age)
+        stuck = age >= 3
+        cached = self.cache.get(source_name, {}).get(version)
+        try:
+            emails, sent_age = cached
+            sent = (age - sent_age) < min(MAX_FREQUENCY, age / 2.0)
+        except TypeError:
+            # This exception happens when source_name, version never seen before
+            emails = []
+            sent_age = 0
+            sent = False
+            # TODO: This transitional code can only trigger on the first
+            # production run, feel free to drop this shortly after merging.
+            if cached is True:
+                sent = True
         if self.dry_run:
             self.log("[email dry run] Considering: %s/%s: %s" %
                      (source_name, version, "stuck" if stuck else "not stuck"))
@@ -171,7 +184,8 @@ class EmailPolicy(BasePolicy, Rest):
             # don't update the cache file in dry run mode; we'll see all output each time
             return PolicyVerdict.PASS
         if stuck and not sent:
-            emails = self.lp_get_emails(source_name, version)
+            if not emails:
+                emails = self.lp_get_emails(source_name, version)
             if emails:
                 recipients = ', '.join(emails)
                 msg = MESSAGE.format(**locals())
@@ -181,11 +195,11 @@ class EmailPolicy(BasePolicy, Rest):
                     server = smtplib.SMTP('localhost')
                     server.sendmail('[e-mail]', emails, msg)
                     server.quit()
-                    sent = True
+                    sent_age = age
                 except socket.error as err:
                     self.log("Failed to send mail! Is SMTP server running?")
                     self.log(err)
-        self.emails_by_pkg[source_name][version] = sent
+        self.emails_by_pkg[source_name][version] = (emails, sent_age)
         self.save_state()
         return PolicyVerdict.PASS
 
diff --git a/tests/test_email.py b/tests/test_email.py
index a456141..fde6405 100755
--- a/tests/test_email.py
+++ b/tests/test_email.py
@@ -193,11 +193,9 @@ class T(unittest.TestCase):
         """Know when not to send any emails."""
         lp.return_value = ['[e-mail]']
         e = EmailPolicy(FakeOptions, None)
-        FakeExcuse.is_valid = False
         FakeExcuse.daysold = 0.002
         e.apply_policy_impl(None, None, 'chromium-browser', None, FakeSourceData, FakeExcuse)
-        FakeExcuse.is_valid = True
-        FakeExcuse.daysold = 4.28
+        FakeExcuse.daysold = 2.98
         e.apply_policy_impl(None, None, 'chromium-browser', None, FakeSourceData, FakeExcuse)
         # Would email but no address found
         FakeExcuse.daysold = 10.12
@@ -218,33 +216,25 @@ class T(unittest.TestCase):
 
     @patch('britney2.policies.email.EmailPolicy.lp_get_emails')
     @patch('britney2.policies.email.smtplib', autospec=True)
-    def test_smtp_days(self, smtp, lp):
-        """Pluralize correctly."""
-        sendmail = smtp.SMTP().sendmail
+    def test_smtp_repetition(self, smtp, lp):
+        """Resend mails periodically, with decreasing frequency."""
         lp.return_value = ['[e-mail]']
+        sendmail = smtp.SMTP().sendmail
         e = EmailPolicy(FakeOptions, None)
-        FakeExcuse.is_valid = False
-        # day
-        FakeExcuse.daysold = 1.01
-        e.apply_policy_impl(None, None, 'chromium-browser', None, FakeSourceData, FakeExcuse)
-        _, args, _ = sendmail.mock_calls[-1]
-        text = args[2]
-        self.assertEquals(sendmail.call_count, 1)
-        self.assertIn(' 1 day.', text)
-        # days
-        FakeExcuse.daysold = 4.9
-        e.apply_policy_impl(None, None, 'chromium-browser', None, FakeSourceData, FakeExcuse)
-        _, args, _ = sendmail.mock_calls[-1]
-        text = args[2]
-        self.assertEquals(sendmail.call_count, 2)
-        self.assertIn(' 4 days.', text)
-        # day, exactly 1
-        FakeExcuse.daysold = 1
-        e.apply_policy_impl(None, None, 'chromium-browser', None, FakeSourceData, FakeExcuse)
-        _, args, _ = sendmail.mock_calls[-1]
-        text = args[2]
-        self.assertEquals(sendmail.call_count, 3)
-        self.assertIn(' 1 day.', text)
+        called = []
+        e.cache = {}
+        for hours in range(0, 5000):
+            previous = sendmail.call_count
+            age = hours / 24
+            FakeExcuse.daysold = age
+            e.apply_policy_impl(None, None, 'unity8', None, FakeSourceData, FakeExcuse)
+            if sendmail.call_count > previous:
+                e.initialise(None)  # Refill e.cache from disk
+                called.append(age)
+        # Emails were sent when daysold reached these values:
+        self.assertSequenceEqual(called, [
+            3.0, 6.0, 12.0, 24.0, 48.0, 78.0, 108.0, 138.0, 168.0, 198.0
+        ])
 
 
 if __name__ == '__main__':