Merge lp:~michael.nelson/launchpad/510331-syncsources-latest-pub into lp:launchpad

[Michael Nelson]

Ensures that syncSources only chooses the latest *PUBLISHED* sources, rather than simply the latest sources.

[Michael Nelson (michael.nelson)]

= Summary =
Fixes 510331 by ensuring that syncSources only finds pub records that are currently PUBLISHED.

== Proposed fix ==

== Pre-implementation notes ==

See note on the bug, as well as:
<noodles> ok, sounds good. Also with bug 510331
<bigjools> noodles: yeah that one's easy
<noodles> IArchive.syncSource() already allows you to specify the version... but with the solution you've suggested for syncSources()
<bigjools> ?
<noodles> it would mean that we'd not copy deleted sources even when there is no published one... is that expected?
<bigjools> yes, I think so
<noodles> OK. easy then.
<bigjools> well, would you expect it to?
<noodles> I'm imagining using the web ui, looking at a bunch of deleted sources and selecting them...
<bigjools> that's syncSource behaviour really
<bigjools> syncSources is more automatic
<bigjools> but we can make it explicit in the docstring
<noodles> OK.

== Implementation details ==

I wasn't sure of the best way to test the private helper method. I ended up going with removeSecurityProxy() for the test, but would be happy to hear about a neater solution?

== Tests ==
bin/test -vvt doc/archive.txt -t TestCollectLatestPublishedSources

== Demo and Q/A ==

We can Q/A this using the API on dogfood (or edge).

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/soyuz/doc/archive.txt
  lib/lp/soyuz/tests/test_archive.py
  lib/lp/soyuz/model/archive.py

== Pylint notices ==

lib/lp/soyuz/model/archive.py
    1099: [C0301] Line too long (81/78) -- fixed.
    14: [F0401] Unable to import 'lazr.lifecycle.event' (No module named lifecycle)

[Graham Binns (gmb)]

Hi Michael,

This looks good to me. One minor nitpick, but otherwise it's fine to
land as-is.

> === modified file 'lib/lp/soyuz/model/archive.py'
> --- lib/lp/soyuz/model/archive.py 2010-01-12 08:31:42 +0000
> +++ lib/lp/soyuz/model/archive.py 2010-01-26 11:54:18 +0000
> @@ -1121,6 +1112,23 @@
>
> self._copySources([source], to_pocket, to_series, include_binaries)
>
> + def _collectLatestPublishedSources(self, from_archive, source_names):
> + """Private helper to collect the latest published sources for an
> + archive.
> + """
> + sources = []
> + for name in source_names:
> + # Check to see if the source package exists, and raise a useful
> + # error if it doesn't.

What qualifies as "A useful error" here? It'd be nice to say in the
comment - or better still in the docstring - "will raise a FooBarError
if the source package doesn't exist."

> + getUtility(ISourcePackageNameSet)[name]
> + # Grabbing the item at index 0 ensures it's the most recent
> + # publication.
> + sources.append(
> + from_archive.getPublishedSources(
> + name=name, exact_match=True,
> + status=PackagePublishingStatus.PUBLISHED)[0])
> + return sources
> +
> def _copySources(self, sources, to_pocket, to_series=None,
> include_binaries=False):
> """Private helper function to copy sources to this archive.
>

review: Approve (code)

[Michael Nelson (michael.nelson)]

<noodles775> gmb: great. Did you have any thoughts about a better way to test the private method (as mentioned in the MP)?
<gmb> noodles775: Argh, must've skipped that bit of the mp. Sorry. Hang on...
<gmb> noodles775: So, no, I can't think of a better way (which is why I never balked at the use of rSP()). The only way I could think of is to test it indirectly, but I'd rather have the direct test myself.
<noodles775> Great, thanks for that.

[Michael Nelson (michael.nelson)]

Hi gmb,

I just got back to this branch, tested it, and had errors or the webservice branch that tested syncSource(s) through the API. The error was simply because the sources used in the test weren't in the published status, but it was 500ing, rather than a nice API error, so I've updated the code to avoid that also.

Here's the diff:

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py 2010-01-28 16:54:43 +0000
+++ lib/lp/soyuz/model/archive.py 2010-02-02 11:28:11 +0000
@@ -1127,10 +1127,11 @@
             getUtility(ISourcePackageNameSet)[name]
             # Grabbing the item at index 0 ensures it's the most recent
             # publication.
- sources.append(
- from_archive.getPublishedSources(
- name=name, exact_match=True,
- status=PackagePublishingStatus.PUBLISHED)[0])
+ published_sources = from_archive.getPublishedSources(
+ name=name, exact_match=True,
+ status=PackagePublishingStatus.PUBLISHED)
+ if published_sources.count() > 0:
+ sources.append(published_sources[0])
         return sources

     def _copySources(self, sources, to_pocket, to_series=None,

=== modified file 'lib/lp/soyuz/stories/webservice/xx-archive.txt'
--- lib/lp/soyuz/stories/webservice/xx-archive.txt 2009-10-07 08:28:03 +0000
+++ lib/lp/soyuz/stories/webservice/xx-archive.txt 2010-02-02 11:57:30 +0000
@@ -576,9 +576,12 @@
     ... sourcename="package1", version="1.0",
     ... archive=ubuntu_db.main_archive)

+ >>> from lp.soyuz.interfaces.publishing import (
+ ... PackagePublishingStatus)
     >>> ignore = test_publisher.getPubSource(
     ... sourcename="package1", version="1.1",
- ... archive=ubuntu_db.main_archive)
+ ... archive=ubuntu_db.main_archive,
+ ... status=PackagePublishingStatus.PUBLISHED)

     >>> ignore = test_publisher.getPubSource(
     ... sourcename="package2", version="1.0",
@@ -742,6 +745,8 @@

     >>> private_publication = test_publisher.createSource(
     ... cprov.archive, 'foocomm', '1.0-1')
+ >>> private_publication.secure_record.status = (
+ ... PackagePublishingStatus.PUBLISHED)

     >>> logout()

@@ -908,8 +913,10 @@
 version.

     >>> login('<email address hidden>')
- >>> unused = test_publisher.createSource(
+ >>> subsequent_version = test_publisher.createSource(
     ... cprov.archive, 'foocomm', '1.0-2')
+ >>> subsequent_version.secure_record.status = (
+ ... PackagePublishingStatus.PUBLISHED)
     >>> logout()

     >>> print cprov_webservice.named_post(

[Graham Binns (gmb)]

As discussed on IRC, you should test for `not foo.is_empty()` rather than `foo.count() > 0` as it's more efficient. Other than that, this change looks fine.

review: Approve (code)

[Michael Nelson (michael.nelson)]

<noodles775> gmb, jtv: http://pastebin.ubuntu.com/367524/
<noodles775> Looks like is_empty isn't included on the ISQLObjectResultSet in storm.zope.interfaces :/
<gmb> ?!
<gmb> Oh, ah.
<gmb> I see.
<gmb> That's suboptimal.
<gmb> noodles775: In that case, just go back to using .count(). You could maybe file a tech-debt bug about changing getPublishedSources() to use Storm, but I don't know how worthwhile that would be.
<noodles775> Yep, thanks.

=== modified file 'lib/lp/soyuz/doc/archive.txt'
--- lib/lp/soyuz/doc/archive.txt	2010-01-12 08:59:23 +0000
+++ lib/lp/soyuz/doc/archive.txt	2010-02-02 12:06:20 +0000
@@ -2349,13 +2349,17 @@
     >>> test_publisher.addFakeChroots(hoary)
     >>> unused = test_publisher.setUpDefaultDistroSeries(hoary)
     >>> discard = test_publisher.getPubSource(
-    ...     sourcename="package1", version="1.0", archive=cprov.archive)
-    >>> discard = test_publisher.getPubSource(
-    ...     sourcename="package1", version="1.1", archive=cprov.archive)
-    >>> discard = test_publisher.getPubSource(
-    ...     sourcename="package2", version="1.0", archive=cprov.archive)
-    >>> discard = test_publisher.getPubSource(
-    ...     sourcename="pack", version="1.0", archive=cprov.archive)
+    ...     sourcename="package1", version="1.0", archive=cprov.archive,
+    ...     status=PackagePublishingStatus.PUBLISHED) 
+    >>> discard = test_publisher.getPubSource( 
+    ...     sourcename="package1", version="1.1", archive=cprov.archive,
+    ...     status=PackagePublishingStatus.PUBLISHED)
+    >>> discard = test_publisher.getPubSource(
+    ...     sourcename="package2", version="1.0", archive=cprov.archive,
+    ...     status=PackagePublishingStatus.PUBLISHED)
+    >>> discard = test_publisher.getPubSource(
+    ...     sourcename="pack", version="1.0", archive=cprov.archive,
+    ...     status=PackagePublishingStatus.PUBLISHED)
 
 Now we have package1 1.0 and 1.1, and package2 1.0 in cprov's PPA.  We
 can ask syncSources to synchronise these packages into mark's PPA in the

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2010-01-12 03:45:02 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2010-02-02 12:06:20 +0000
@@ -896,8 +896,8 @@
                     to_series=None, include_binaries=False):
         """Synchronise (copy) named sources into this archive from another.
 
-        It will copy the most recent versions of the named sources to
-        the destination archive if necessary.
+        It will copy the most recent PUBLISHED versions of the named
+        sources to the destination archive if necessary.
 
         This operation will only succeeds when all requested packages
         are synchronised between the archives. If any of the requested

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2010-01-12 08:31:42 +0000
+++ lib/lp/soyuz/model/archive.py	2010-02-02 12:06:20 +0000
@@ -1096,17 +1096,8 @@
                     to_series=None, include_binaries=False):
         """See `IArchive`."""
         # Find and validate the source package names in source_names.
-        sources = []
-        for name in source_names:
-            # Check to see if the source package exists, and raise a useful
-            # error if it doesn't.
-            getUtility(ISourcePackageNameSet)[name]
-            # Grabbing the item at index 0 ensures it's the most recent
-            # publication.
-            sources.append(
-                from_archive.getPublishedSources(
-                    name=name, exact_match=True)[0])
-
+        sources = self._collectLatestPublishedSources(
+            from_archive, source_names)
         self._copySources(sources, to_pocket, to_series, include_binaries)
 
     def syncSource(self, source_name, version, from_archive, to_pocket,
@@ -1121,6 +1112,28 @@
 
         self._copySources([source], to_pocket, to_series, include_binaries)
 
+    def _collectLatestPublishedSources(self, from_archive, source_names):
+        """Private helper to collect the latest published sources for an 
+        archive.
+
+        :raises NoSuchSourcePackageName: If any of the source_names do not
+            exist.
+        """
+        sources = []
+        for name in source_names:
+            # Check to see if the source package exists. This will raise
+            # a NoSuchSourcePackageName exception if the source package
+            # doesn't exist.
+            getUtility(ISourcePackageNameSet)[name]
+            # Grabbing the item at index 0 ensures it's the most recent
+            # publication.
+            published_sources = from_archive.getPublishedSources(
+                name=name, exact_match=True,
+                status=PackagePublishingStatus.PUBLISHED)
+            if published_sources.count() > 0:
+                sources.append(published_sources[0])
+        return sources
+
     def _copySources(self, sources, to_pocket, to_series=None,
                      include_binaries=False):
         """Private helper function to copy sources to this archive.

=== modified file 'lib/lp/soyuz/stories/webservice/xx-archive.txt'
--- lib/lp/soyuz/stories/webservice/xx-archive.txt	2009-10-07 08:28:03 +0000
+++ lib/lp/soyuz/stories/webservice/xx-archive.txt	2010-02-02 12:06:20 +0000
@@ -576,9 +576,12 @@
     ...     sourcename="package1", version="1.0",
     ...     archive=ubuntu_db.main_archive)
 
+    >>> from lp.soyuz.interfaces.publishing import (
+    ...     PackagePublishingStatus)
     >>> ignore = test_publisher.getPubSource(
     ...     sourcename="package1", version="1.1",
-    ...     archive=ubuntu_db.main_archive)
+    ...     archive=ubuntu_db.main_archive,
+    ...     status=PackagePublishingStatus.PUBLISHED)
 
     >>> ignore = test_publisher.getPubSource(
     ...     sourcename="package2", version="1.0",
@@ -742,6 +745,8 @@
 
     >>> private_publication = test_publisher.createSource(
     ...     cprov.archive, 'foocomm', '1.0-1')
+    >>> private_publication.secure_record.status = (
+    ...     PackagePublishingStatus.PUBLISHED)
 
     >>> logout()
 
@@ -908,8 +913,10 @@
 version.
 
     >>> login('[e-mail]')
-    >>> unused = test_publisher.createSource(
+    >>> subsequent_version = test_publisher.createSource(
     ...     cprov.archive, 'foocomm', '1.0-2')
+    >>> subsequent_version.secure_record.status = (
+    ...     PackagePublishingStatus.PUBLISHED)
     >>> logout()
 
     >>> print cprov_webservice.named_post(

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2010-01-12 04:21:42 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2010-02-02 12:06:20 +0000
@@ -513,6 +513,50 @@
         self.archive.disable()
         self.assertRaises(AssertionError, self.archive.disable)
 
+class TestCollectLatestPublishedSources(TestCaseWithFactory):
+    """Ensure that the private helper method works as expected."""
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        """Setup an archive with relevant publications."""
+        super(TestCollectLatestPublishedSources, self).setUp()
+        self.publisher = SoyuzTestPublisher()
+        self.publisher.prepareBreezyAutotest()
+
+        # Create an archive with some published sources. We'll store
+        # a reference to the naked archive so that we can call
+        # the private method which is not defined on the interface.
+        self.archive = self.factory.makeArchive()
+        self.naked_archive = removeSecurityProxy(self.archive)
+        
+        self.pub_1 = self.publisher.getPubSource(
+            version='0.5.11~ppa1', archive=self.archive, sourcename="foo",
+            status=PackagePublishingStatus.PUBLISHED)
+
+        self.pub_2 = self.publisher.getPubSource(
+            version='0.5.11~ppa2', archive=self.archive, sourcename="foo",
+            status=PackagePublishingStatus.PUBLISHED)
+
+        self.pub_3 = self.publisher.getPubSource(
+            version='0.9', archive=self.archive, sourcename="bar",
+            status=PackagePublishingStatus.PUBLISHED)
+
+    def test_collectLatestPublishedSources_returns_latest(self):
+        pubs = self.naked_archive._collectLatestPublishedSources(
+            self.archive, ["foo"])
+        self.assertEqual(1, len(pubs))
+        self.assertEqual('0.5.11~ppa2', pubs[0].source_package_version)
+
+    def test_collectLatestPublishedSources_returns_published_only(self):
+        # Set the status of the latest pub to DELETED and ensure that it
+        # is not returned.
+        self.pub_2.secure_record.status = PackagePublishingStatus.DELETED
+
+        pubs = self.naked_archive._collectLatestPublishedSources(
+            self.archive, ["foo"])
+        self.assertEqual(1, len(pubs))
+        self.assertEqual('0.5.11~ppa1', pubs[0].source_package_version)
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)