launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #22561
[Merge] lp:~cjwatson/launchpad/better-vcs-import-emails into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/better-vcs-import-emails into lp:launchpad.
Commit message:
Make updated code import emails more informative.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/better-vcs-import-emails/+merge/346733
They now have an X-Launchpad-Notification-Type header, and they show the imported-from details even if they aren't being changed since those often make it clear at a glance why an import might be failing.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/better-vcs-import-emails into lp:launchpad.
=== modified file 'lib/lp/code/doc/codeimport.txt'
--- lib/lp/code/doc/codeimport.txt 2018-05-13 09:59:59 +0000
+++ lib/lp/code/doc/codeimport.txt 2018-05-23 11:53:21 +0000
@@ -546,6 +546,9 @@
The branch whiteboard was changed to:
<BLANKLINE>
stuff
+ <BLANKLINE>
+ This code import is from:
+ hello from :pserver:anoncvs@xxxxxxxxxxxxxxx:/var/cvsroot
>>> print(cvs_import.branch.whiteboard)
stuff
@@ -556,5 +559,8 @@
>>> print(make_email_body_for_code_import_update(
... cvs_import, modify_event, ''))
The branch whiteboard was deleted.
+ <BLANKLINE>
+ This code import is from:
+ hello from :pserver:anoncvs@xxxxxxxxxxxxxxx:/var/cvsroot
>>> print(cvs_import.branch.whiteboard)
None
=== modified file 'lib/lp/code/mail/codeimport.py'
--- lib/lp/code/mail/codeimport.py 2016-10-03 17:00:56 +0000
+++ lib/lp/code/mail/codeimport.py 2018-05-23 11:53:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Email notifications related to code imports."""
@@ -102,42 +102,46 @@
else:
raise AssertionError('Unexpected review status for code import.')
- details_change_prefix = '\n'.join(textwrap.wrap(
- "%s is now being imported from:" % code_import.target.unique_name))
if code_import.rcs_type == RevisionControlSystems.CVS:
+ old_details = new_details = "%s from %s" % (
+ code_import.cvs_module, code_import.cvs_root)
if (CodeImportEventDataType.OLD_CVS_ROOT in event_data or
- CodeImportEventDataType.OLD_CVS_MODULE in event_data):
- new_details = ' %s from %s' % (
- code_import.cvs_module, code_import.cvs_root)
+ CodeImportEventDataType.OLD_CVS_MODULE in event_data):
old_root = event_data.get(
CodeImportEventDataType.OLD_CVS_ROOT,
code_import.cvs_root)
old_module = event_data.get(
CodeImportEventDataType.OLD_CVS_MODULE,
code_import.cvs_module)
- old_details = ' %s from %s' % (old_module, old_root)
- body.append(
- details_change_prefix + '\n' + new_details +
- "\ninstead of:\n" + old_details)
+ old_details = "%s from %s" % (old_module, old_root)
elif code_import.rcs_type in (RevisionControlSystems.BZR_SVN,
RevisionControlSystems.GIT,
RevisionControlSystems.BZR):
+ old_details = new_details = code_import.url
if CodeImportEventDataType.OLD_URL in event_data:
- old_url = event_data[CodeImportEventDataType.OLD_URL]
- body.append(
- details_change_prefix + '\n ' + code_import.url +
- "\ninstead of:\n " + old_url)
+ old_details = event_data[CodeImportEventDataType.OLD_URL]
else:
raise AssertionError(
'Unexpected rcs_type %r for code import.' % code_import.rcs_type)
+ if new_details != old_details:
+ body.append(
+ textwrap.fill(
+ "%s is now being imported from:" %
+ code_import.target.unique_name) +
+ "\n " + new_details +
+ "\ninstead of:\n " + old_details)
+
if new_whiteboard is not None:
if new_whiteboard != '':
body.append("The branch whiteboard was changed to:")
- body.append("\n".join(textwrap.wrap(new_whiteboard)))
+ body.append(textwrap.fill(new_whiteboard))
else:
body.append("The branch whiteboard was deleted.")
+ if new_details == old_details:
+ body.append("This code import is from:\n " + new_details)
+
return '\n\n'.join(body)
@@ -151,7 +155,10 @@
herder_rationale = 'Operator @%s' % vcs_imports.name
recipients.add(vcs_imports, None, herder_rationale)
- headers = {'X-Launchpad-Branch': target.unique_name}
+ headers = {
+ 'X-Launchpad-Notification-Type': 'code-import-updated',
+ 'X-Launchpad-Branch': target.unique_name,
+ }
subject = 'Code import %s status: %s' % (
code_import.target.unique_name, code_import.review_status.title)
=== modified file 'lib/lp/code/mail/tests/test_codeimport.py'
--- lib/lp/code/mail/tests/test_codeimport.py 2016-10-11 14:26:31 +0000
+++ lib/lp/code/mail/tests/test_codeimport.py 2018-05-23 11:53:21 +0000
@@ -1,19 +1,22 @@
-# Copyright 2010-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for code import related mailings"""
from email import message_from_string
+import textwrap
import transaction
from lp.code.enums import (
+ CodeImportReviewStatus,
RevisionControlSystems,
TargetRevisionControlSystems,
)
from lp.code.tests.helpers import GitHostingFixture
from lp.services.mail import stub
from lp.testing import (
+ login_celebrity,
login_person,
TestCaseWithFactory,
)
@@ -146,3 +149,150 @@
'\n'
'-- \nYou are getting this email because you are a member of the '
'vcs-imports team.\n', msg.get_payload(decode=True))
+
+
+class TestUpdatedCodeImports(TestCaseWithFactory):
+ """Test the emails sent out for updated code imports."""
+
+ layer = DatabaseFunctionalLayer
+
+ def assertSameDetailsEmail(self, details, unique_name):
+ msg = message_from_string(stub.test_emails[0][2])
+ self.assertEqual(
+ 'code-import-updated', msg['X-Launchpad-Notification-Type'])
+ self.assertEqual(unique_name, msg['X-Launchpad-Branch'])
+ self.assertEqual(
+ 'Hello,\n\n'
+ 'The import has been marked as failing.\n\n'
+ 'This code import is from:\n'
+ ' %(details)s\n\n'
+ '-- \nhttp://code.launchpad.dev/%(unique_name)s\n'
+ 'You are getting this email because you are a member of the '
+ 'vcs-imports team.\n' % {
+ 'details': details,
+ 'unique_name': unique_name,
+ },
+ msg.get_payload(decode=True))
+
+ def assertDifferentDetailsEmail(self, old_details, new_details,
+ unique_name):
+ msg = message_from_string(stub.test_emails[0][2])
+ self.assertEqual(
+ 'code-import-updated', msg['X-Launchpad-Notification-Type'])
+ self.assertEqual(unique_name, msg['X-Launchpad-Branch'])
+ self.assertEqual(
+ 'Hello,\n\n'
+ '%(details_change_message)s\n'
+ ' %(new_details)s\n'
+ 'instead of:\n'
+ ' %(old_details)s\n'
+ '\n'
+ '-- \nhttp://code.launchpad.dev/%(unique_name)s\n'
+ 'You are getting this email because you are a member of the '
+ 'vcs-imports team.\n' % {
+ 'details_change_message': textwrap.fill(
+ '%s is now being imported from:' % unique_name),
+ 'old_details': old_details,
+ 'new_details': new_details,
+ 'unique_name': unique_name,
+ },
+ msg.get_payload(decode=True))
+
+ def test_cvs_to_bzr_import_same_details(self):
+ code_import = self.factory.makeProductCodeImport(
+ cvs_root=':pserver:anonymouse@xxxxxxxxxxxxxxx:/cvsroot',
+ cvs_module='a_module')
+ unique_name = code_import.target.unique_name
+ user = login_celebrity('vcs_imports')
+ code_import.updateFromData(
+ {'review_status': CodeImportReviewStatus.FAILING}, user)
+ transaction.commit()
+ self.assertSameDetailsEmail(
+ 'a_module from :pserver:anonymouse@xxxxxxxxxxxxxxx:/cvsroot',
+ unique_name)
+
+ def test_cvs_to_bzr_import_different_details(self):
+ code_import = self.factory.makeProductCodeImport(
+ cvs_root=':pserver:anonymouse@xxxxxxxxxxxxxxx:/cvsroot',
+ cvs_module='a_module')
+ unique_name = code_import.target.unique_name
+ user = login_celebrity('vcs_imports')
+ code_import.updateFromData({'cvs_module': 'another_module'}, user)
+ transaction.commit()
+ self.assertDifferentDetailsEmail(
+ 'a_module from :pserver:anonymouse@xxxxxxxxxxxxxxx:/cvsroot',
+ 'another_module from :pserver:anonymouse@xxxxxxxxxxxxxxx:/cvsroot',
+ unique_name)
+
+ def test_svn_to_bzr_import_same_details(self):
+ code_import = self.factory.makeProductCodeImport(
+ svn_branch_url='svn://svn.example.com/fooix/trunk')
+ unique_name = code_import.target.unique_name
+ user = login_celebrity('vcs_imports')
+ code_import.updateFromData(
+ {'review_status': CodeImportReviewStatus.FAILING}, user)
+ transaction.commit()
+ self.assertSameDetailsEmail(
+ 'svn://svn.example.com/fooix/trunk', unique_name)
+
+ def test_svn_to_bzr_import_different_details(self):
+ code_import = self.factory.makeProductCodeImport(
+ svn_branch_url='svn://svn.example.com/fooix/trunk')
+ unique_name = code_import.target.unique_name
+ user = login_celebrity('vcs_imports')
+ code_import.updateFromData(
+ {'url': 'https://svn.example.com/fooix/trunk'}, user)
+ transaction.commit()
+ self.assertDifferentDetailsEmail(
+ 'svn://svn.example.com/fooix/trunk',
+ 'https://svn.example.com/fooix/trunk', unique_name)
+
+ def test_git_to_bzr_import_same_details(self):
+ code_import = self.factory.makeProductCodeImport(
+ git_repo_url='git://git.example.com/fooix.git')
+ unique_name = code_import.target.unique_name
+ user = login_celebrity('vcs_imports')
+ code_import.updateFromData(
+ {'review_status': CodeImportReviewStatus.FAILING}, user)
+ transaction.commit()
+ self.assertSameDetailsEmail(
+ 'git://git.example.com/fooix.git', unique_name)
+
+ def test_git_to_bzr_import_different_details(self):
+ code_import = self.factory.makeProductCodeImport(
+ git_repo_url='git://git.example.com/fooix.git')
+ unique_name = code_import.target.unique_name
+ user = login_celebrity('vcs_imports')
+ code_import.updateFromData(
+ {'url': 'https://git.example.com/fooix.git'}, user)
+ transaction.commit()
+ self.assertDifferentDetailsEmail(
+ 'git://git.example.com/fooix.git',
+ 'https://git.example.com/fooix.git', unique_name)
+
+ def test_git_to_git_import_same_details(self):
+ self.useFixture(GitHostingFixture())
+ code_import = self.factory.makeProductCodeImport(
+ git_repo_url='git://git.example.com/fooix.git',
+ target_rcs_type=TargetRevisionControlSystems.GIT)
+ unique_name = code_import.target.unique_name
+ user = login_celebrity('vcs_imports')
+ code_import.updateFromData(
+ {'review_status': CodeImportReviewStatus.FAILING}, user)
+ transaction.commit()
+ self.assertSameDetailsEmail(
+ 'git://git.example.com/fooix.git', unique_name)
+
+ def test_git_to_git_import_different_details(self):
+ self.useFixture(GitHostingFixture())
+ code_import = self.factory.makeProductCodeImport(
+ git_repo_url='git://git.example.com/fooix.git',
+ target_rcs_type=TargetRevisionControlSystems.GIT)
+ unique_name = code_import.target.unique_name
+ user = login_celebrity('vcs_imports')
+ code_import.updateFromData(
+ {'url': 'https://git.example.com/fooix.git'}, user)
+ transaction.commit()
+ self.assertDifferentDetailsEmail(
+ 'git://git.example.com/fooix.git',
+ 'https://git.example.com/fooix.git', unique_name)
Follow ups
