Launchpad itself

Merge lp:~thumper/launchpad/branch-email-subject-oops into lp:launchpad

  1. branch-email-subject-oops
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: no longer in the source branch.
Merged at revision: 9872
Proposed branch: lp:~thumper/launchpad/branch-email-subject-oops
Merge into: lp:launchpad
Diff against target: 135 lines (+34/-24)
3 files modified
lib/lp/code/doc/branch-notifications.txt (+2/-2)
lib/lp/code/mail/branch.py (+14/-22)
lib/lp/code/mail/tests/test_branch.py (+18/-0)
To merge this branch: bzr merge lp:~thumper/launchpad/branch-email-subject-oops
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) code Approve
Review via email: mp+14780@code.launchpad.net

Commit message

Stop the interpolation of the revisions commit message string.

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

Commit messages sometimes have '%' characters in them.

The subject for revision emails have the first line of the commit message.

The default behaviour of the BaseMailer is to use dictionary based string interpolation. For the BranchMailer we don't want this. For both revision mail and modification mail we have fully specified subjects.

Tests:
  TestBranchMailerSubject

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

We changed the approach during review, from overriding _getSubject to avoid interpolation to making the caller use interpolation to compose the subject line.

The new code looks good.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/doc/branch-notifications.txt'
2--- lib/lp/code/doc/branch-notifications.txt 2009-08-13 19:03:36 +0000
3+++ lib/lp/code/doc/branch-notifications.txt 2009-11-13 01:33:12 +0000
4@@ -42,7 +42,7 @@
5 ... BranchSubscriptionDiffSize.WHOLEDIFF,
6 ... CodeReviewNotificationLevel.NOEMAIL)
7 >>> BranchMailer.forRevision(branch, 1, 'foo@canonical.com',
8- ... 'The contents.', None, None).sendAll()
9+ ... 'The contents.', None, 'Subject line').sendAll()
10
11 >>> notifications = pop_notifications()
12 >>> len(notifications)
13@@ -53,7 +53,7 @@
14 >>> print branch_notification['From']
15 foo@canonical.com
16 >>> print branch_notification['Subject']
17- [Branch ~name12/firefox/main]
18+ Subject line
19 >>> print branch_notification['X-Launchpad-Project']
20 firefox
21 >>> print branch_notification['X-Launchpad-Branch']
22
23=== modified file 'lib/lp/code/mail/branch.py'
24--- lib/lp/code/mail/branch.py 2009-08-21 02:56:35 +0000
25+++ lib/lp/code/mail/branch.py 2009-11-13 01:33:12 +0000
26@@ -168,7 +168,7 @@
27
28 def __init__(self, subject, template_name, recipients, from_address,
29 delta=None, contents=None, diff=None, message_id=None,
30- revno=None, notification_type=None):
31+ revno=None, notification_type=None, **kwargs):
32 BaseMailer.__init__(self, subject, template_name, recipients,
33 from_address, delta, message_id,
34 notification_type)
35@@ -179,6 +179,7 @@
36 else:
37 self.diff_size = self.diff.count('\n') + 1
38 self.revno = revno
39+ self.extra_template_params = kwargs
40
41 @classmethod
42 def forBranchModified(cls, branch, user, delta):
43@@ -213,10 +214,10 @@
44 subscription, recipient, rationale)
45 from_address = format_address(
46 user.displayname, user.preferredemail.email)
47- subject = cls._branchSubject(branch)
48 return cls(
49- subject, 'branch-modified.txt', actual_recipients, from_address,
50- delta=delta, notification_type='branch-updated')
51+ '[Branch %(unique_name)s]', 'branch-modified.txt',
52+ actual_recipients, from_address, delta=delta,
53+ notification_type='branch-updated')
54
55 @classmethod
56 def forRevision(cls, db_branch, revno, from_address, contents, diff,
57@@ -242,21 +243,9 @@
58 subscriber_reason = RecipientReason.forBranchSubscriber(
59 subscription, recipient, rationale)
60 recipient_dict[recipient] = subscriber_reason
61- subject = cls._branchSubject(db_branch, subject)
62- return cls(subject, 'branch-modified.txt', recipient_dict,
63+ return cls('%(full_subject)s', 'branch-modified.txt', recipient_dict,
64 from_address, contents=contents, diff=diff, revno=revno,
65- notification_type='branch-revision')
66-
67- @staticmethod
68- def _branchSubject(db_branch, subject=None):
69- """Determine a subject to use for this email.
70-
71- :param db_branch: The db branch to use.
72- :param subject: Any subject supplied as a parameter.
73- """
74- if subject is not None:
75- return subject
76- return '[Branch %s]' % (db_branch.unique_name)
77+ notification_type='branch-revision', full_subject=subject)
78
79 def _getHeaders(self, email):
80 headers = BaseMailer._getHeaders(self, email)
81@@ -271,19 +260,22 @@
82 def _getTemplateParams(self, email):
83 params = BaseMailer._getTemplateParams(self, email)
84 reason, rationale = self._recipients.getReason(email)
85- params['branch_identity'] = reason.branch.bzr_identity
86- params['branch_url'] = canonical_url(reason.branch)
87- if reason.recipient in reason.branch.subscribers:
88+ branch = reason.branch
89+ params['unique_name'] = branch.unique_name
90+ params['branch_identity'] = branch.bzr_identity
91+ params['branch_url'] = canonical_url(branch)
92+ if reason.recipient in branch.subscribers:
93 # Give subscribers a link to unsubscribe.
94 params['unsubscribe'] = (
95 "\nTo unsubscribe from this branch go to "
96- "%s/+edit-subscription." % canonical_url(reason.branch))
97+ "%s/+edit-subscription." % canonical_url(branch))
98 else:
99 params['unsubscribe'] = ''
100 params['diff'] = self.contents or ''
101 if not self._includeDiff(email):
102 params['diff'] += self._explainNotPresentDiff(email)
103 params.setdefault('delta', '')
104+ params.update(self.extra_template_params)
105 return params
106
107 def _includeDiff(self, email):
108
109=== modified file 'lib/lp/code/mail/tests/test_branch.py'
110--- lib/lp/code/mail/tests/test_branch.py 2009-07-22 18:37:32 +0000
111+++ lib/lp/code/mail/tests/test_branch.py 2009-11-13 01:33:12 +0000
112@@ -210,5 +210,23 @@
113 self.assertNotIn('larger than your specified limit', ctrl.body)
114
115
116+class TestBranchMailerSubject(TestCaseWithFactory):
117+ """The subject for a BranchMailer is returned verbatim."""
118+
119+ layer = DatabaseFunctionalLayer
120+
121+ def test_subject(self):
122+ # No string interpolation should occur on the subject.
123+ branch = self.factory.makeAnyBranch()
124+ # Subscribe the owner to get revision email.
125+ branch.getSubscription(branch.owner).notification_level = (
126+ BranchSubscriptionNotificationLevel.FULL)
127+ mailer = BranchMailer.forRevision(
128+ branch, 1, 'test@example.com', 'content', 'diff',
129+ 'Testing %j foo')
130+ self.assertEqual('Testing %j foo', mailer._getSubject(
131+ branch.owner.preferredemail.email))
132+
133+
134 def test_suite():
135 return TestLoader().loadTestsFromName(__name__)