launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19007
[Merge] lp:~cjwatson/launchpad/mail-footer-pref into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/mail-footer-pref into lp:launchpad.
Commit message:
Include filtering information in e-mail footers if PersonSettings.expanded_notification_footers is set.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1474071 in Launchpad itself: "Gmail filtering improvements"
https://bugs.launchpad.net/launchpad/+bug/1474071
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/mail-footer-pref/+merge/264720
Include filtering information in e-mail footers if PersonSettings.expanded_notification_footers is set.
Following https://code.launchpad.net/~cjwatson/launchpad/db-mail-footer-pref/+merge/264622, this models the new column, backfills it with False in a garbo job, and adds the relevant code to BaseMailer.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/mail-footer-pref into lp:launchpad.
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2015-07-08 15:13:48 +0000
+++ database/schema/security.cfg 2015-07-14 14:44:02 +0000
@@ -2279,6 +2279,7 @@
public.openidconsumerassociation = SELECT, DELETE
public.openidconsumernonce = SELECT, DELETE
public.person = SELECT, DELETE
+public.personsettings = SELECT, UPDATE
public.product = SELECT, UPDATE
public.pofiletranslator = SELECT, INSERT, UPDATE, DELETE
public.potranslation = SELECT, DELETE
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2015-07-09 20:06:17 +0000
+++ lib/lp/registry/browser/person.py 2015-07-14 14:44:02 +0000
@@ -2702,7 +2702,8 @@
field_names = ['displayname', 'name', 'mugshot', 'description',
'hide_email_addresses', 'verbose_bugnotifications',
- 'selfgenerated_bugnotifications']
+ 'selfgenerated_bugnotifications',
+ 'expanded_notification_footers']
custom_widget('mugshot', ImageChangeWidget, ImageChangeWidget.EDIT_STYLE)
label = 'Change your personal details'
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2015-05-11 13:17:41 +0000
+++ lib/lp/registry/interfaces/person.py 2015-07-14 14:44:02 +0000
@@ -552,6 +552,14 @@
title=_("Send me bug notifications for changes I make"),
required=False, default=False)
+ expanded_notification_footers = Bool(
+ title=_("Include filtering information in e-mail footers"),
+ description=_(
+ "Some e-mail clients do not allow filtering on arbitrary message "
+ "headers. If you use one of these, you can set this option to "
+ "add more information to the end of message bodies."),
+ required=False, default=False)
+
class IPersonPublic(IPrivacy):
"""Public attributes for a Person.
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2015-07-09 20:06:17 +0000
+++ lib/lp/registry/model/person.py 2015-07-14 14:44:02 +0000
@@ -20,6 +20,7 @@
'person_sort_key',
'PersonLanguage',
'PersonSet',
+ 'PersonSettings',
'SSHKey',
'SSHKeySet',
'TeamInvitationEvent',
@@ -105,10 +106,7 @@
from lp import _
from lp.answers.model.questionsperson import QuestionsPersonMixin
-from lp.app.enums import (
- InformationType,
- PRIVATE_INFORMATION_TYPES,
- )
+from lp.app.enums import PRIVATE_INFORMATION_TYPES
from lp.app.interfaces.launchpad import (
IHasIcon,
IHasLogo,
@@ -423,6 +421,8 @@
selfgenerated_bugnotifications = BoolCol(notNull=True, default=False)
+ expanded_notification_footers = BoolCol(notNull=False, default=False)
+
def readonly_settings(message, interface):
"""Make an object that disallows writes to values on the interface.
=== modified file 'lib/lp/registry/stories/person/xx-person-edit.txt'
--- lib/lp/registry/stories/person/xx-person-edit.txt 2012-01-15 13:32:27 +0000
+++ lib/lp/registry/stories/person/xx-person-edit.txt 2015-07-14 14:44:02 +0000
@@ -78,12 +78,19 @@
False
>>> self_generated_control.click()
+He will enable expanded mail notification footers.
+
+ >>> expanded_footer_control = browser.getControl(
+ ... "Include filtering information in e-mail footers")
+ >>> expanded_footer_control.selected
+ False
+ >>> expanded_footer_control.click()
+
>>> browser.getControl('Save').click()
>>> browser.url
'http://launchpad.dev/~rayjay'
-We now check to ensure that the verbose bug notifications option and
-self-generated bug notification option were changed:
+We now check to ensure that the various notifications options were changed:
>>> browser.open('http://launchpad.dev/~rayjay/+edit')
>>> browser.getControl("Include bug descriptions when sending me "
@@ -92,3 +99,6 @@
>>> browser.getControl(
... "Send me bug notifications for changes I make").selected
True
+ >>> browser.getControl(
+ ... "Include filtering information in e-mail footers").selected
+ True
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2015-06-26 09:59:30 +0000
+++ lib/lp/registry/tests/test_person.py 2015-07-14 14:44:02 +0000
@@ -515,6 +515,11 @@
user = self.factory.makePerson()
self.assertFalse(user.selfgenerated_bugnotifications)
+ def test_expanded_notification_footers_false_by_default(self):
+ # Default for new accounts is to disable expanded notification footers.
+ user = self.factory.makePerson()
+ self.assertFalse(user.expanded_notification_footers)
+
def test_canAccess__anonymous(self):
# Anonymous users cannot call Person.canAccess()
person = self.factory.makePerson()
=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py 2015-07-13 00:52:36 +0000
+++ lib/lp/scripts/garbo.py 2015-07-14 14:44:02 +0000
@@ -70,7 +70,10 @@
)
from lp.hardwaredb.model.hwdb import HWSubmission
from lp.registry.model.commercialsubscription import CommercialSubscription
-from lp.registry.model.person import Person
+from lp.registry.model.person import (
+ Person,
+ PersonSettings,
+ )
from lp.registry.model.product import Product
from lp.registry.model.teammembership import TeamMembership
from lp.services.config import config
@@ -1411,6 +1414,29 @@
"""
+class PersonSettingsENFPopulator(BulkPruner):
+ """Populates PersonSettings.expanded_notification_footers."""
+
+ target_table_class = PersonSettings
+ ids_to_prune_query = """
+ SELECT person
+ FROM PersonSettings
+ WHERE expanded_notification_footers IS NULL
+ """
+
+ def __call__(self, chunk_size):
+ """See `ITunableLoop`."""
+ result = self.store.execute("""
+ UPDATE PersonSettings
+ SET expanded_notification_footers = FALSE
+ WHERE person IN (
+ SELECT * FROM
+ cursor_fetch('%s', %d) AS f(person integer))
+ """ % (self.cursor_name, chunk_size))
+ self._num_removed = result.rowcount
+ transaction.commit()
+
+
class BaseDatabaseGarbageCollector(LaunchpadCronScript):
"""Abstract base class to run a collection of TunableLoops."""
script_name = None # Script name for locking and database user. Override.
@@ -1694,6 +1720,7 @@
LoginTokenPruner,
ObsoleteBugAttachmentPruner,
OldTimeLimitedTokenDeleter,
+ PersonSettingsENFPopulator,
RevisionAuthorEmailLinker,
ScrubPOFileTranslator,
SuggestiveTemplatesCacheUpdater,
=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py 2015-07-13 00:55:56 +0000
+++ lib/lp/scripts/tests/test_garbo.py 2015-07-14 14:44:02 +0000
@@ -16,6 +16,7 @@
import time
from pytz import UTC
+from storm.exceptions import NoneError
from storm.expr import (
In,
Like,
@@ -70,6 +71,7 @@
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.teammembership import TeamMembershipStatus
from lp.registry.model.commercialsubscription import CommercialSubscription
+from lp.registry.model.person import PersonSettings
from lp.registry.model.teammembership import TeamMembership
from lp.scripts.garbo import (
AntiqueSessionPruner,
@@ -1369,6 +1371,49 @@
self._test_LiveFSFilePruner(
'application/octet-stream', 0, expected_count=1)
+ def test_PersonSettingsENFPopulator(self):
+ switch_dbuser('testadmin')
+ store = IMasterStore(PersonSettings)
+ people_enf_none = []
+ people_enf_false = []
+ people_enf_true = []
+ for _ in range(2):
+ person = self.factory.makePerson()
+ try:
+ person.expanded_notification_footers = None
+ except NoneError:
+ # Now enforced by DB NOT NULL constraint; backfilling is no
+ # longer necessary.
+ return
+ people_enf_none.append(person)
+ person = self.factory.makePerson()
+ person.expanded_notification_footers = False
+ people_enf_false.append(person)
+ person = self.factory.makePerson()
+ person.expanded_notification_footers = True
+ people_enf_true.append(person)
+ settings_count = store.find(PersonSettings).count()
+ self.runDaily()
+ switch_dbuser('testadmin')
+
+ # No rows have been deleted.
+ self.assertEqual(settings_count, store.find(PersonSettings).count())
+
+ def _assert_enf_by_person(person, expected):
+ record = store.find(
+ PersonSettings, PersonSettings.person == person.id).one()
+ self.assertEqual(expected, record.expanded_notification_footers)
+
+ # Rows with expanded_notification_footers=None have been backfilled.
+ for person in people_enf_none:
+ _assert_enf_by_person(person, False)
+
+ # Other rows have been left alone.
+ for person in people_enf_false:
+ _assert_enf_by_person(person, False)
+ for person in people_enf_true:
+ _assert_enf_by_person(person, True)
+
class TestGarboTasks(TestCaseWithFactory):
layer = LaunchpadZopelessLayer
=== modified file 'lib/lp/services/mail/basemailer.py'
--- lib/lp/services/mail/basemailer.py 2012-06-29 08:40:05 +0000
+++ lib/lp/services/mail/basemailer.py 2015-07-14 14:44:02 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Base class for sending out emails."""
@@ -7,6 +7,7 @@
__all__ = ['BaseMailer', 'RecipientReason']
+from collections import OrderedDict
import logging
from smtplib import SMTPException
@@ -77,6 +78,9 @@
headers = self._getHeaders(email)
subject = self._getSubject(email, recipient)
body = self._getBody(email, recipient)
+ expanded_footer = self._getExpandedFooter(headers, recipient)
+ if expanded_footer:
+ body = append_footer(body, expanded_footer)
ctrl = self._mail_controller_class(
self.from_address, to_addresses, subject, body, headers,
envelope_to=[email])
@@ -100,7 +104,8 @@
def _getHeaders(self, email):
"""Return the mail headers to use."""
reason, rationale = self._recipients.getReason(email)
- headers = {'X-Launchpad-Message-Rationale': reason.mail_header}
+ headers = OrderedDict()
+ headers['X-Launchpad-Message-Rationale'] = reason.mail_header
if self.notification_type is not None:
headers['X-Launchpad-Notification-Type'] = self.notification_type
reply_to = self._getReplyToAddress()
@@ -146,6 +151,16 @@
"""Provide a footer to attach to the body, or None."""
return None
+ def _getExpandedFooter(self, headers, recipient):
+ """Provide an expanded footer for recipients who have requested it."""
+ if not recipient.expanded_notification_footers:
+ return None
+ lines = []
+ for key, value in headers.items():
+ if key.startswith('X-Launchpad-'):
+ lines.append('%s: %s\n' % (key[2:], value))
+ return ''.join(lines)
+
def sendAll(self):
"""Send notifications to all recipients."""
# We never want SMTP errors to propagate from this function.
=== modified file 'lib/lp/services/mail/tests/test_basemailer.py'
--- lib/lp/services/mail/tests/test_basemailer.py 2012-01-01 02:58:52 +0000
+++ lib/lp/services/mail/tests/test_basemailer.py 2015-07-14 14:44:02 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for the BaseMailer class."""
@@ -7,6 +7,8 @@
from smtplib import SMTPException
+from testtools.matchers import EndsWith
+
from lp.services.mail.basemailer import BaseMailer
from lp.services.mail.sendmail import MailController
from lp.testing import TestCaseWithFactory
@@ -132,6 +134,33 @@
self.assertEqual('text/plain', attachment['Content-Type'])
self.assertEqual('inline', attachment['Content-Disposition'])
+ def test_generateEmail_append_no_expanded_footer(self):
+ # Recipients without expanded_notification_footers do not receive an
+ # expanded footer on messages.
+ fake_to = self.factory.makePerson(email='to@xxxxxxxxxxx')
+ recipients = {fake_to: FakeSubscription()}
+ mailer = BaseMailerSubclass(
+ 'subject', None, recipients, 'from@xxxxxxxxxxx',
+ notification_type='test')
+ ctrl = mailer.generateEmail('to@xxxxxxxxxxx', fake_to)
+ self.assertNotIn('Launchpad-Message-Rationale', ctrl.body)
+
+ def test_generateEmail_append_expanded_footer(self):
+ # Recipients with expanded_notification_footers receive an expanded
+ # footer on messages.
+ fake_to = self.factory.makePerson(email='to@xxxxxxxxxxx')
+ fake_to.expanded_notification_footers = True
+ recipients = {fake_to: FakeSubscription()}
+ mailer = BaseMailerSubclass(
+ 'subject', None, recipients, 'from@xxxxxxxxxxx',
+ notification_type='test')
+ ctrl = mailer.generateEmail('to@xxxxxxxxxxx', fake_to)
+ self.assertThat(
+ ctrl.body, EndsWith(
+ '\n-- \n'
+ 'Launchpad-Message-Rationale: pete\n'
+ 'Launchpad-Notification-Type: test\n'))
+
def test_sendall_single_failure_doesnt_kill_all(self):
# A failure to send to a particular email address doesn't stop sending
# to others.
Follow ups
