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

= 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)

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.
>

<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.

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(

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.

<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.