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 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('noreply@canonical.com', 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 = ['example@email.com'] 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 = ['email@address.com'] + 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__':