launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/kill-bpb-is_new into lp:launchpad.

Commit message:
Move the BinaryPackageRelease NEWness check to PackageUploadBuild, to get some of the DAS- and archive-specific bits out of the agnostic BinaryPackageRelease.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/kill-bpb-is_new/+merge/215069

This branch moves some context-specific properties from BinaryPackageRelease to PackageUploadBuild. They're still evil and context-specific, but they're at least not on the archive- and DAS-agnostic BinaryPackageRelease object.
-- 
https://code.launchpad.net/~wgrant/launchpad/kill-bpb-is_new/+merge/215069
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/kill-bpb-is_new into lp:launchpad.

=== modified file 'lib/lp/soyuz/doc/binarypackagerelease.txt'
--- lib/lp/soyuz/doc/binarypackagerelease.txt	2012-12-26 01:32:19 +0000
+++ lib/lp/soyuz/doc/binarypackagerelease.txt	2014-04-10 02:37:07 +0000
@@ -35,20 +35,6 @@
    ...    firefox_bin_release.distributionsourcepackagerelease)
    True
 
-The 'is_new' property tells us whether or not this
-BinaryPackageRelease has ever been published for the DistroArchSeries
-it was built for. If not, then 'is_new' will be True, otherwise False.
-
-   >>> firefox_bin_release.is_new
-   False
-
-   >>> pmount_bin_release = BinaryPackageRelease.get(20)
-   >>> pmount_bin_release.name, pmount_bin_release.version
-   (u'pmount', u'0.1-1')
-
-   >>> pmount_bin_release.is_new
-   True
-
 The IBinaryPackageNameSet.getNotNewByNames() returns all the
 BinaryPackageName records for BinaryPackageReleases that are published
 in the supplied distroseries in the archives with the supplied
@@ -61,6 +47,7 @@
     >>> from lp.soyuz.interfaces.binarypackagename import (
     ...     IBinaryPackageNameSet)
     >>> foobar_name = getUtility(IBinaryPackageNameSet)['foobar']
+    >>> pmount_bin_release = BinaryPackageRelease.get(20)
     >>> name_ids = (
     ...     foobar_name.id,
     ...     pmount_bin_release.binarypackagename.id,

=== modified file 'lib/lp/soyuz/interfaces/binarypackagerelease.py'
--- lib/lp/soyuz/interfaces/binarypackagerelease.py	2013-01-07 02:40:55 +0000
+++ lib/lp/soyuz/interfaces/binarypackagerelease.py	2014-04-10 02:37:07 +0000
@@ -26,7 +26,6 @@
     Bool,
     Date,
     Datetime,
-    Dict,
     Int,
     List,
     Object,
@@ -92,14 +91,6 @@
         "The sourcepackage release in this distribution from which this "
         "binary was built.")
 
-    is_new = Bool(
-        title=_("New Binary."),
-        description=_("True if there binary version was never published for "
-                      "the architeture it was built for. False otherwise."))
-
-    # This is a dictionary for fast retrieval over the webservice.
-    properties = Dict(title=_("The properties of this binary."))
-
     def addFile(file):
         """Create a BinaryPackageFile record referencing this build
         and attach the provided library file alias (file).

=== modified file 'lib/lp/soyuz/model/binarypackagerelease.py'
--- lib/lp/soyuz/model/binarypackagerelease.py	2013-01-07 02:40:55 +0000
+++ lib/lp/soyuz/model/binarypackagerelease.py	2014-04-10 02:37:07 +0000
@@ -121,27 +121,6 @@
         """See `IBinaryPackageRelease`."""
         return self.build.source_package_release.sourcepackagename.name
 
-    @property
-    def is_new(self):
-        """See `IBinaryPackageRelease`."""
-        distroarchseries = self.build.distro_arch_series
-        distroarchseries_binary_package = distroarchseries.getBinaryPackage(
-            self.binarypackagename)
-        return distroarchseries_binary_package.currentrelease is None
-
-    @property
-    def properties(self):
-        """See `IBinaryPackageRelease`."""
-        return {
-            "name": self.name,
-            "version": self.version,
-            "is_new": self.is_new,
-            "architecture": self.build.arch_tag,
-            "component": self.component.name,
-            "section": self.section.name,
-            "priority": self.priority.name,
-            }
-
     @cachedproperty
     def files(self):
         return list(

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2013-12-20 05:42:43 +0000
+++ lib/lp/soyuz/model/queue.py	2014-04-10 02:37:07 +0000
@@ -1150,6 +1150,27 @@
         return made_changes
 
 
+def get_properties_for_binary(bpr):
+    # XXX wgrant 2014-04-08: This is so, so wrong, as it assumes that
+    # the BPR is only ever published where it was built. But that holds
+    # whenever this code is called for now, as copies don't use
+    # PackageUploadBuild.
+    das = bpr.build.distro_arch_series
+    distroarchseries_binary_package = das.getBinaryPackage(
+        bpr.binarypackagename)
+    is_new = distroarchseries_binary_package.currentrelease is None
+
+    return {
+        "name": bpr.name,
+        "version": bpr.version,
+        "is_new": is_new,
+        "architecture": bpr.build.arch_tag,
+        "component": bpr.component.name,
+        "section": bpr.section.name,
+        "priority": bpr.priority.name,
+        }
+
+
 class PackageUploadBuild(SQLBase):
     """A Queue item's related builds."""
     implements(IPackageUploadBuild)
@@ -1166,7 +1187,7 @@
     def binaries(self):
         """See `IPackageUploadBuild`."""
         for binary in self.build.binarypackages:
-            yield binary.properties
+            yield get_properties_for_binary(binary)
 
     def checkComponentAndSection(self):
         """See `IPackageUploadBuild`."""

=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py	2013-06-20 05:50:00 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py	2014-04-10 02:37:07 +0000
@@ -34,6 +34,7 @@
 from lp.services.mail.sendmail import format_address_for_person
 from lp.soyuz.adapters.overrides import SourceOverride
 from lp.soyuz.enums import (
+    PackagePublishingStatus,
     PackageUploadCustomFormat,
     PackageUploadStatus,
     )
@@ -1135,6 +1136,21 @@
             self.assertCanOpenRedirectedUrl(browser, ws_binary_file_url)
         self.assertCanOpenRedirectedUrl(browser, ws_upload.changes_file_url)
 
+    def test_binary_is_new(self):
+        # The is_new property is False if a binary with this name has
+        # already been published in the same DistroArchSeries.
+        person = self.makeQueueAdmin([self.universe])
+        upload, ws_upload = self.makeBinaryPackageUpload(
+            person, binarypackagename="hello")
+        self.assertTrue(ws_upload.getBinaryProperties()[1]['is_new'])
+        with person_logged_in(person):
+            das = upload.builds[0].build.distro_arch_series
+            self.factory.makeBinaryPackagePublishingHistory(
+                binarypackagename="hello", archive=das.main_archive,
+                distroarchseries=das, status=PackagePublishingStatus.PUBLISHED)
+            transaction.commit()
+        self.assertFalse(ws_upload.getBinaryProperties()[1]['is_new'])
+
     def test_overrideBinaries_limited_component_permissions(self):
         # Overriding between two components requires queue admin of both.
         person = self.makeQueueAdmin([self.universe])