launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04758
[Merge] lp:~julian-edwards/launchpad/changelogs-bug-827576 into lp:launchpad
Julian Edwards has proposed merging lp:~julian-edwards/launchpad/changelogs-bug-827576 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #827576 in Launchpad itself: "copyPackage only includes changes from upload being copied"
https://bugs.launchpad.net/launchpad/+bug/827576
For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/changelogs-bug-827576/+merge/72756
When a package is synced from an upstream distro, it may not always be the next version after the one we already have. For example, we synced version 1.0, missed 1.1 and then synced 1.2.
When sending an email notification for this synced package, we also need to include the changelog_entry from version 1.1 as it was not previously included in the distro.
I've added an aggregate_changelog method on sourcepackagerelease so that the combined changelog can be retrieved from various places (this will be required for other bug fixes) and made the package copying notification code use this method to work out the changelog that needs to go in the email.
--
https://code.launchpad.net/~julian-edwards/launchpad/changelogs-bug-827576/+merge/72756
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/changelogs-bug-827576 into lp:launchpad.
=== modified file 'lib/lp/soyuz/adapters/notification.py'
--- lib/lp/soyuz/adapters/notification.py 2011-08-18 14:40:24 +0000
+++ lib/lp/soyuz/adapters/notification.py 2011-08-24 17:10:28 +0000
@@ -127,7 +127,7 @@
def notify(blamer, spr, bprs, customfiles, archive, distroseries, pocket,
summary_text=None, changes=None, changesfile_content=None,
changesfile_object=None, action=None, dry_run=False,
- logger=None, announce_from_person=None):
+ logger=None, announce_from_person=None, previous_version=None):
"""Notify about
:param blamer: The `IPerson` who is to blame for this notification.
@@ -149,6 +149,9 @@
:param announce_from_person: If passed, use this `IPerson` as the From: in
announcement emails. If the person has no preferred email address,
the person is ignored and the default From: is used instead.
+ :param previous_version: If specified, the change log on the email will
+ include all of the source package's change logs after that version
+ up to and including the passed spr's version.
"""
# If this is a binary or mixed upload, we don't send *any* emails
# provided it's not a rejection or a security upload:
@@ -213,12 +216,13 @@
attach_changes = not archive.is_ppa
- def build_and_send_mail(action, recipients, from_addr=None, bcc=None):
+ def build_and_send_mail(action, recipients, from_addr=None, bcc=None,
+ previous_version=None):
subject = calculate_subject(
spr, bprs, customfiles, archive, distroseries, pocket, action)
body = assemble_body(
blamer, spr, bprs, archive, distroseries, summarystring, changes,
- action)
+ action, previous_version=previous_version)
body = body.encode("utf8")
send_mail(
spr, archive, recipients, subject, body, dry_run,
@@ -226,7 +230,8 @@
attach_changes=attach_changes, from_addr=from_addr, bcc=bcc,
logger=logger)
- build_and_send_mail(action, recipients)
+ build_and_send_mail(
+ action, recipients, previous_version=previous_version)
info = fetch_information(spr, bprs, changes)
from_addr = info['changedby']
@@ -254,15 +259,16 @@
build_and_send_mail(
'announcement', [str(distroseries.changeslist)], from_addr,
- bcc_addr)
+ bcc_addr, previous_version=previous_version)
def assemble_body(blamer, spr, bprs, archive, distroseries, summary, changes,
- action):
+ action, previous_version=None):
"""Assemble the e-mail notification body."""
if changes is None:
changes = {}
- info = fetch_information(spr, bprs, changes)
+ info = fetch_information(
+ spr, bprs, changes, previous_version=previous_version)
information = {
'STATUS': ACTION_DESCRIPTIONS[action],
'SUMMARY': summary,
@@ -596,7 +602,7 @@
pocket != PackagePublishingPocket.SECURITY)
-def fetch_information(spr, bprs, changes):
+def fetch_information(spr, bprs, changes, previous_version=None):
changedby = None
changedby_displayname = None
maintainer = None
@@ -613,7 +619,7 @@
elif spr or bprs:
if not spr and bprs:
spr = bprs[0].build.source_package_release
- changesfile = spr.changelog_entry
+ changesfile = spr.aggregate_changelog(previous_version)
date = spr.dateuploaded
changedby = person_to_email(spr.creator)
maintainer = person_to_email(spr.maintainer)
=== modified file 'lib/lp/soyuz/adapters/tests/test_notification.py'
--- lib/lp/soyuz/adapters/tests/test_notification.py 2011-08-18 15:07:03 +0000
+++ lib/lp/soyuz/adapters/tests/test_notification.py 2011-08-24 17:10:28 +0000
@@ -176,6 +176,23 @@
u"bær <%s>" % spr.maintainer.preferredemail.email,
info['maintainer_displayname'])
+ def test_fetch_information_spr_multiple_changelogs(self):
+ # If previous_version is passed the "changesfile" entry in the
+ # returned dict should contain the changelogs for all SPRs *since*
+ # that version and up to and including the passed SPR.
+ creator = self.factory.makePerson()
+ maintainer = self.factory.makePerson()
+ old_spr = self.factory.makeSourcePackageRelease(
+ creator=creator, maintainer=maintainer, sourcepackagename="foo",
+ version="1.0", changelog_entry="foo 1.0")
+ spr = self.factory.makeSourcePackageRelease(
+ creator=creator, maintainer=maintainer, sourcepackagename="foo",
+ version="1.1", changelog_entry="foo 1.1")
+ info = fetch_information(spr, None, None, previous_version="0.9")
+
+ self.assertIn("foo 1.0", info['changesfile'])
+ self.assertIn("foo 1.1", info['changesfile'])
+
def test_fetch_information_bprs(self):
bpr = self.factory.makeBinaryPackageRelease()
info = fetch_information(None, [bpr], None)
=== modified file 'lib/lp/soyuz/interfaces/sourcepackagerelease.py'
--- lib/lp/soyuz/interfaces/sourcepackagerelease.py 2011-03-06 06:26:38 +0000
+++ lib/lp/soyuz/interfaces/sourcepackagerelease.py 2011-08-24 17:10:28 +0000
@@ -240,6 +240,16 @@
:return: total size (in KB) of this package
"""
+ def aggregate_changelog(since_version=None):
+ """Get all the changelogs since the version passed.
+
+ :param since_version: Return changelogs of all versions
+ after since_version up to and including the version of this
+ sourcepackagerelease.
+ :return: A concatenated set of changelogs of all the required
+ versions, with a blank line between each.
+ """
+
class PackageDiffAlreadyRequestedError(Exception):
"""Raised when an `IPackageDiff` request already exists."""
=== modified file 'lib/lp/soyuz/model/sourcepackagerelease.py'
--- lib/lp/soyuz/model/sourcepackagerelease.py 2011-07-25 11:00:53 +0000
+++ lib/lp/soyuz/model/sourcepackagerelease.py 2011-08-24 17:10:28 +0000
@@ -603,3 +603,18 @@
return PackageDiff(
from_source=self, to_source=to_sourcepackagerelease,
requester=requester, status=status)
+
+ def aggregate_changelog(self, since_version=None):
+ """See `ISourcePackageRelease`."""
+ if since_version is None:
+ return self.changelog_entry
+
+ store = Store.of(self)
+ sprs = store.find(
+ SourcePackageRelease,
+ SourcePackageRelease.sourcepackagename == self.sourcepackagename,
+ SourcePackageRelease.version > since_version,
+ SourcePackageRelease.version <= self.version)
+
+ return "\n\n".join(
+ [spr.changelog_entry for spr in sprs])
=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py 2011-08-18 09:24:57 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py 2011-08-24 17:10:28 +0000
@@ -628,6 +628,16 @@
override = None
if overrides:
override = overrides[overrides_index]
+ if send_email:
+ old_version = None
+ # Make a note of the destination source's version for use
+ # in sending the email notification.
+ existing = archive.getPublishedSources(
+ name=source.sourcepackagerelease.name, exact_match=True,
+ status=active_publishing_status,
+ distroseries=series, pocket=pocket).first()
+ if existing:
+ old_version = existing.sourcepackagerelease.version
sub_copies = _do_direct_copy(
source, archive, destination_series, pocket,
include_binaries, override, close_bugs=close_bugs,
@@ -637,7 +647,8 @@
person, source.sourcepackagerelease, [], [], archive,
destination_series, pocket, changes=None,
action='accepted',
- announce_from_person=announce_from_person)
+ announce_from_person=announce_from_person,
+ previous_version=old_version)
overrides_index += 1
copies.extend(sub_copies)
=== modified file 'lib/lp/soyuz/scripts/tests/test_copypackage.py'
--- lib/lp/soyuz/scripts/tests/test_copypackage.py 2011-08-19 11:10:36 +0000
+++ lib/lp/soyuz/scripts/tests/test_copypackage.py 2011-08-24 17:10:28 +0000
@@ -1465,6 +1465,48 @@
self.assertIn(expected_text, notification.as_string())
self.assertIn(expected_text, announcement.as_string())
+ def test_copy_notification_contains_aggregate_change_log(self):
+ # When copying a package that generates a notification,
+ # the changelog should contain all of the changelog_entry texts for
+ # all the sourcepackagereleases between the last published version
+ # and the new version.
+ archive = self.test_publisher.ubuntutest.main_archive
+ source3 = self.test_publisher.getPubSource(
+ sourcename="foo", archive=archive, version='1.0-3',
+ architecturehintlist='any')
+ source3.sourcepackagerelease.changelog_entry = '* Foo3'
+ # Copying to a primary archive reads the changes to close bugs.
+ transaction.commit()
+
+ # Now make a new series, nobby, and publish foo 1.0-1 in it.
+ nobby = self.createNobby(('i386', 'hppa'))
+ getUtility(ISourcePackageFormatSelectionSet).add(
+ nobby, SourcePackageFormat.FORMAT_1_0)
+ nobby.changeslist = 'nobby-changes@xxxxxxxxxxx'
+ source1 = self.factory.makeSourcePackageRelease(
+ sourcepackagename="foo", version="1.0-1",
+ changelog_entry="* Foo1")
+ self.factory.makeSourcePackagePublishingHistory(
+ sourcepackagerelease=source1, distroseries=nobby,
+ status=PackagePublishingStatus.PUBLISHED,
+ pocket=source3.pocket)
+
+ # Make foo 1.0-2, it doesn't need to be published.
+ self.factory.makeSourcePackageRelease(
+ sourcepackagename="foo", version="1.0-2",
+ changelog_entry="* Foo2")
+
+ # Now copy foo 1.0-3 from ubuntutest.
+ [copied_source] = do_copy(
+ [source3], nobby.main_archive, nobby, source3.pocket, False,
+ person=source3.sourcepackagerelease.creator,
+ check_permissions=False, send_email=True)
+ [notification, announcement] = pop_notifications()
+ for mail in (notification, announcement):
+ mailtext = mail.as_string()
+ self.assertIn("* Foo2", mailtext)
+ self.assertIn("* Foo3", mailtext)
+
def test_copy_generates_rejection_email(self):
# When a copy into a primary archive fails, we expect a rejection
# email if the send_email parameter is True.
=== modified file 'lib/lp/soyuz/tests/test_sourcepackagerelease.py'
--- lib/lp/soyuz/tests/test_sourcepackagerelease.py 2011-06-15 17:03:28 +0000
+++ lib/lp/soyuz/tests/test_sourcepackagerelease.py 2011-08-24 17:10:28 +0000
@@ -81,6 +81,26 @@
spr = self.factory.makeSourcePackageRelease(homepage="<invalid<url")
self.assertEquals("<invalid<url", spr.homepage)
+ def test_aggregate_changelog(self):
+ # If since_version is passed the "changelog" entry returned
+ # should contain the changelogs for all SPRs *since*
+ # that version and up to and including the context SPR.
+ creator = self.factory.makePerson(displayname=u"foo")
+ maintainer = self.factory.makePerson(displayname=u"bar")
+ self.factory.makeSourcePackageRelease(
+ creator=creator, maintainer=maintainer, sourcepackagename="foo",
+ version="1.0", changelog_entry="this is version 1.0")
+ spr11 = self.factory.makeSourcePackageRelease(
+ creator=creator, maintainer=maintainer, sourcepackagename="foo",
+ version="1.1", changelog_entry="this is version 1.1")
+ spr12 = self.factory.makeSourcePackageRelease(
+ creator=creator, maintainer=maintainer, sourcepackagename="foo",
+ version="1.2", changelog_entry="this is version 1.2")
+ observed = spr12.aggregate_changelog(since_version="1.0")
+ expected = "\n\n".join(
+ [spr11.changelog_entry, spr12.changelog_entry])
+ self.assertEqual(expected, observed)
+
class TestSourcePackageReleaseGetBuildByArch(TestCaseWithFactory):
"""Tests for SourcePackageRelease.getBuildByArch()."""
