launchpad-reviewers team mailing list archive

[Colin Watson <mp+429506@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:remove-dsp-is-upstream-link-allowed into launchpad:master.

Commit message:
Remove DistributionSourcePackage.is_upstream_link_allowed

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/429506

The logic for computing its value had several problems:

 * It didn't match the docstring, since it was set to True only for packages not published in the relevant distribution or for packages in the "misc" section, rather than for packages _not_ in the "misc" section;
 * Lots of non-metapackages are in the "misc" section;
 * Metapackages are normally in the "metapackages" section nowadays anyway;
 * It crashed on SPPHs without a section, such as those created from CI builds;
 * It was unused.

The database column has a default, so it's safe to just remove it entirely from the ORM.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:remove-dsp-is-upstream-link-allowed into launchpad:master.

diff --git a/lib/lp/registry/model/distributionsourcepackage.py b/lib/lp/registry/model/distributionsourcepackage.py
index fd9da20..76d68db 100644
--- a/lib/lp/registry/model/distributionsourcepackage.py
+++ b/lib/lp/registry/model/distributionsourcepackage.py
@@ -61,16 +61,6 @@ from lp.translations.model.customlanguagecode import (
 )
 
 
-def is_upstream_link_allowed(spph):
-    """Metapackages shouldn't have upstream links.
-
-    Metapackages normally are in the 'misc' section.
-    """
-    if spph is None:
-        return True
-    return spph.section.name == "misc"
-
-
 class DistributionSourcePackageProperty:
     def __init__(self, attrname, default=None):
         self.attrname = attrname
@@ -81,24 +71,7 @@ class DistributionSourcePackageProperty:
 
     def __set__(self, obj, value):
         if obj._self_in_database is None:
-            spph = (
-                Store.of(obj.distribution)
-                .find(
-                    SourcePackagePublishingHistory,
-                    SourcePackagePublishingHistory.distroseriesID
-                    == DistroSeries.id,
-                    DistroSeries.distributionID == obj.distribution.id,
-                    SourcePackagePublishingHistory.sourcepackagenameID
-                    == obj.sourcepackagename.id,
-                )
-                .order_by(Desc(SourcePackagePublishingHistory.id))
-                .first()
-            )
-            obj._new(
-                obj.distribution,
-                obj.sourcepackagename,
-                is_upstream_link_allowed(spph),
-            )
+            obj._new(obj.distribution, obj.sourcepackagename)
         setattr(obj._self_in_database, self.attrname, value)
 
 
@@ -130,9 +103,6 @@ class DistributionSourcePackage(
     )
     bug_count = DistributionSourcePackageProperty("bug_count")
     po_message_count = DistributionSourcePackageProperty("po_message_count")
-    is_upstream_link_allowed = DistributionSourcePackageProperty(
-        "is_upstream_link_allowed"
-    )
     enable_bugfiling_duplicate_search = DistributionSourcePackageProperty(
         "enable_bugfiling_duplicate_search", default=True
     )
@@ -555,11 +525,9 @@ class DistributionSourcePackage(
         )
 
     @classmethod
-    def _new(
-        cls, distribution, sourcepackagename, is_upstream_link_allowed=False
-    ):
+    def _new(cls, distribution, sourcepackagename):
         return DistributionSourcePackageInDatabase.new(
-            distribution, sourcepackagename, is_upstream_link_allowed
+            distribution, sourcepackagename
         )
 
     @classmethod
@@ -590,8 +558,7 @@ class DistributionSourcePackage(
             sourcepackagename = sourcepackage.sourcepackagename
         dsp = cls._get(distribution, sourcepackagename)
         if dsp is None:
-            upstream_link_allowed = is_upstream_link_allowed(spph)
-            cls._new(distribution, sourcepackagename, upstream_link_allowed)
+            cls._new(distribution, sourcepackagename)
 
 
 @implementer(transaction.interfaces.ISynchronizer)
@@ -632,7 +599,6 @@ class DistributionSourcePackageInDatabase(Storm):
 
     bug_count = Int()
     po_message_count = Int()
-    is_upstream_link_allowed = Bool()
     enable_bugfiling_duplicate_search = Bool()
 
     @property
@@ -706,9 +672,7 @@ class DistributionSourcePackageInDatabase(Storm):
         return dsp
 
     @classmethod
-    def new(
-        cls, distribution, sourcepackagename, is_upstream_link_allowed=False
-    ):
+    def new(cls, distribution, sourcepackagename):
         """Create a new DSP with the given parameters.
 
         Caches the `(distro_id, spn_id) --> dsp_id` mapping.
@@ -716,7 +680,6 @@ class DistributionSourcePackageInDatabase(Storm):
         dsp = DistributionSourcePackageInDatabase()
         dsp.distribution = distribution
         dsp.sourcepackagename = sourcepackagename
-        dsp.is_upstream_link_allowed = is_upstream_link_allowed
         Store.of(distribution).add(dsp)
         Store.of(distribution).flush()
         dsp_cache_key = distribution.id, sourcepackagename.id
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 1667cfd..3fd560c 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -5471,7 +5471,7 @@ class LaunchpadObjectFactory(ObjectFactory):
             # done by secondary process.
             naked_package = removeSecurityProxy(package)
             if naked_package._get(distribution, sourcepackagename) is None:
-                naked_package._new(distribution, sourcepackagename, False)
+                naked_package._new(distribution, sourcepackagename)
         return package
 
     def makeDSPCache(