launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/db-bug-790084 into lp:launchpad/db-devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #790084 in Launchpad itself: "PlainPackageCopyJob.findMatchingDSDs fails to filter by package"
  https://bugs.launchpad.net/launchpad/+bug/790084

For more details, see:
https://code.launchpad.net/~jtv/launchpad/db-bug-790084/+merge/62845

= Summary =

When an asynchronous package copy requested from a DistroSeriesDifference fails with a CannotCopy exception, it leaves a note in the form of a DistroSeriesDifferenceComment.  But it's currently doing that on all DSDs for the same distroseries.


== Proposed fix ==

I neglected to filter the DSDs by source package name.  Easy fix.


== Pre-implementation notes ==

Failed to have one.  I'm bad.


== Implementation details ==

I had to add IDistroSeriesDifference.source_package_name_id, in order to give the model code access to the attribute on objects as returned by the ISourcePackageNameSet utility.  Matching on id is the right thing to do here because checking source_package_name attributes will cause individual SourcePackageName attributes to be fetched for lots of irrelevant DSDs.


== Tests ==

{{{
./bin/test -vvc lp.soyuz.tests.test_packagecopyjob -t findMatchingDSDs
}}}


== Demo and Q/A ==

Request an asynchronous sync that will fail with a CannotCopy exception.  Presently amsn is a usable example.  Make sure that cronscripts/process-job-source-groups.py is run:

{{{
./cronscripts/process-job-source-groups.py MAIN -vvv -e IMembershipNotificationJobSource -e IPersonMergeJobSource -e IQuestionEmailJobSource
}}}

The failure should leave a note on its DSD entry on +localpackagediffs, but not on the entries for other packages.


= Launchpad lint =

Oops, I missed a spot which I shall fix forthwith:

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/model/packagecopyjob.py
  lib/lp/registry/interfaces/distroseriesdifference.py
  lib/lp/soyuz/tests/test_packagecopyjob.py

./lib/lp/soyuz/tests/test_packagecopyjob.py
     423: local variable 'other_dsd' is assigned to but never used
-- 
https://code.launchpad.net/~jtv/launchpad/db-bug-790084/+merge/62845
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/db-bug-790084 into lp:launchpad/db-devel.

=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py	2011-05-26 09:07:28 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py	2011-05-30 07:56:02 +0000
@@ -68,6 +68,8 @@
             "The distribution series which identifies the parent series "
             "with the difference.")))
 
+    source_package_name_id = Int(
+        title=u"Source package name id", required=True, readonly=True)
     source_package_name = Reference(
         ISourcePackageName,
         title=_("Source package name"), required=True, readonly=True,
@@ -234,6 +236,7 @@
             cannot be requested.
         """
 
+
 class IDistroSeriesDifferenceAdmin(Interface):
     """Difference attributes requiring launchpad.Admin."""
 

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2011-05-29 21:18:09 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2011-05-30 07:56:02 +0000
@@ -38,6 +38,7 @@
     IDistroSeriesDifferenceSource,
     )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
 from lp.registry.model.distroseries import DistroSeries
 from lp.registry.interfaces.distroseriesdifferencecomment import (
     IDistroSeriesDifferenceCommentSource,
@@ -291,10 +292,14 @@
         # changed.  We can however filter out DSDs that are from
         # different distributions, based on the job's target archive.
         source_distro_id = self.source_archive.distributionID
+        package_ids = set(
+            getUtility(ISourcePackageNameSet).queryByName(name).id
+            for name, version in self.metadata["source_packages"])
         return [
             dsd
             for dsd in candidates
-                if dsd.parent_series.distributionID == source_distro_id]
+                if dsd.parent_series.distributionID == source_distro_id and
+                    dsd.source_package_name_id in package_ids]
 
     def reportFailure(self, cannotcopy_exception):
         """Attempt to report failure to the user."""

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2011-05-27 02:48:33 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2011-05-30 07:56:02 +0000
@@ -418,6 +418,14 @@
 
         self.assertContentEqual([], naked_job.findMatchingDSDs())
 
+    def test_findMatchingDSDs_ignores_other_packages(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        other_dsd = self.factory.makeDistroSeriesDifference(
+            derived_series=dsd.derived_series,
+            parent_series=dsd.parent_series)
+        naked_job = removeSecurityProxy(self.makeJob(dsd))
+        self.assertContentEqual([dsd], naked_job.findMatchingDSDs())
+
 
 class TestPlainPackageCopyJobPrivileges(TestCaseWithFactory, LocalTestHelper):
     """Test that `PlainPackageCopyJob` has the privileges it needs.