Merge ~pappacena/launchpad:ociproject-project-pillar into launchpad:master

[Thiago F. Pappacena]

Model changes to allow OCIProject pillar to be either a Distribution or a Product.

[Tom Wardill (twom)]

To me, the approach looks correct, but I've spotted a couple of things to make it more robust to accidental problems.

review: Needs Fixing

7b77fbb... by Thiago F. Pappacena on 2020-05-13

Renaming OCIProject.product and adding checks when setting pillar

[Thiago F. Pappacena (pappacena)]

Thanks for the review, twom! I'm pushing the requested changes, but I could use some clarification on one of the comments bellow.

[Tom Wardill (twom)]

LGTM!

review: Approve

8442d00... by Thiago F. Pappacena on 2020-05-20

Merge branch 'master' into ociproject-project-pillar

bc86804... by Thiago F. Pappacena on 2020-05-21

Merge branch 'master' into ociproject-project-pillar

2d5c6b3... by Thiago F. Pappacena on 2020-05-21

Merge branch 'master' into ociproject-project-pillar

[Colin Watson (cjwatson)]

review: Approve

1ca81b6... by Thiago F. Pappacena on 2020-05-27

Merge branch 'master' into ociproject-project-pillar

930d512... by Thiago F. Pappacena on 2020-05-27

Allowing pillar change for OCIProject

[Thiago F. Pappacena (pappacena)]

Pushed the requested change. I'll land this once the corresponding db changes are applied in production.

- https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/383819
- https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/384670

diff --git a/lib/lp/registry/browser/ociproject.py b/lib/lp/registry/browser/ociproject.py
index eedec0c..df7ac85 100644
--- a/lib/lp/registry/browser/ociproject.py
+++ b/lib/lp/registry/browser/ociproject.py
@@ -82,7 +82,7 @@ class OCIProjectAddView(LaunchpadFormView):
         oci_project_name = getUtility(
             IOCIProjectNameSet).getOrCreateByName(name)
 
-        oci_project = getUtility(IOCIProjectSet).getByDistributionAndName(
+        oci_project = getUtility(IOCIProjectSet).getByPillarAndName(
             self.context, oci_project_name.name)
         if oci_project:
             self.setFieldError(
@@ -194,7 +194,7 @@ class OCIProjectEditView(LaunchpadEditFormView):
         distribution = data.get('distribution')
         name = data.get('name')
         if distribution and name:
-            oci_project = getUtility(IOCIProjectSet).getByDistributionAndName(
+            oci_project = getUtility(IOCIProjectSet).getByPillarAndName(
                 distribution, name)
             if oci_project is not None and oci_project != self.context:
                 self.setFieldError(
diff --git a/lib/lp/registry/interfaces/ociproject.py b/lib/lp/registry/interfaces/ociproject.py
index 8e66f5d..d6f8468 100644
--- a/lib/lp/registry/interfaces/ociproject.py
+++ b/lib/lp/registry/interfaces/ociproject.py
@@ -47,6 +47,7 @@ from lp.code.interfaces.gitref import IGitRef
 from lp.code.interfaces.hasgitrepositories import IHasGitRepositories
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.ociprojectname import IOCIProjectName
+from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.database.constants import DEFAULT
 from lp.services.fields import (
@@ -107,7 +108,11 @@ class IOCIProjectEditableAttributes(IBugTarget):
     distribution = exported(ReferenceChoice(
         title=_("The distribution that this OCI project is associated with."),
         schema=IDistribution, vocabulary="Distribution",
-        required=True, readonly=False))
+        required=False, readonly=False))
+    project = exported(ReferenceChoice(
+        title=_('The project that this OCI project is associated with.'),
+        schema=IProduct, vocabulary='Product',
+        required=False, readonly=False))
     name = exported(TextLine(
         title=_("Name"), required=True, readonly=False,
         constraint=name_validator,
@@ -120,9 +125,10 @@ class IOCIProjectEditableAttributes(IBugTarget):
     description = exported(Text(
         title=_("The description for this OCI project."),
         required=True, readonly=False))
-    pillar = exported(Reference(
-        IDistribution,
-        title=_("The pillar containing this target."), readonly=True))
+    pillar = Reference(
+        Interface,
+        title=_("The pillar containing this target."),
+        required=True, readonly=False)
 
 
 class IOCIProjectEdit(Interface):
@@ -189,8 +195,13 @@ class IOCIProjectSet(Interface):
             bugfiling_duplicate_search=False):
         """Create an `IOCIProject`."""
 
-    def getByDistributionAndName(distribution, name):
-        """Get the OCIProjects for a given distribution."""
+    def getByPillarAndName(pillar, name):
+        """Get the OCIProjects for a given distribution or project.
+
+        :param pillar: An instance of Distribution or Product.
+        :param name: The OCIProject name to find.
+        :return: The OCIProject found.
+        """
 
     def findByDistributionAndName(distribution, name_substring):
         """Find OCIProjects for a given distribution that contains the
diff --git a/lib/lp/registry/model/distribution.py b/lib/lp/registry/model/distribution.py
index 9c4c96e..50efe68 100644
--- a/lib/lp/registry/model/distribution.py
+++ b/lib/lp/registry/model/distribution.py
@@ -841,7 +841,7 @@ class Distribution(SQLBase, BugTargetBase, MakesAnnouncements,
             """ % sqlvalues(self.id, name))
 
     def getOCIProject(self, name):
-        oci_project = getUtility(IOCIProjectSet).getByDistributionAndName(
+        oci_project = getUtility(IOCIProjectSet).getByPillarAndName(
             self, name)
         return oci_project
 
diff --git a/lib/lp/registry/model/ociproject.py b/lib/lp/registry/model/ociproject.py
index 0d94e38..b991c19 100644
--- a/lib/lp/registry/model/ociproject.py
+++ b/lib/lp/registry/model/ociproject.py
@@ -33,6 +33,7 @@ from lp.registry.interfaces.ociproject import (
     )
 from lp.registry.interfaces.ociprojectname import IOCIProjectNameSet
 from lp.registry.interfaces.person import IPersonSet
+from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.model.ociprojectname import OCIProjectName
 from lp.registry.model.ociprojectseries import OCIProjectSeries
@@ -78,6 +79,9 @@ class OCIProject(BugTargetBase, StormBase):
     distribution_id = Int(name="distribution", allow_none=True)
     distribution = Reference(distribution_id, "Distribution.id")
 
+    project_id = Int(name='project', allow_none=True)
+    project = Reference(project_id, 'Product.id')
+
     ociprojectname_id = Int(name="ociprojectname", allow_none=False)
     ociprojectname = Reference(ociprojectname_id, "OCIProjectName.id")
 
@@ -100,7 +104,20 @@ class OCIProject(BugTargetBase, StormBase):
     @property
     def pillar(self):
         """See `IBugTarget`."""
-        return self.distribution
+        return self.project if self.project_id else self.distribution
+
+    @pillar.setter
+    def pillar(self, pillar):
+        if IDistribution.providedBy(pillar):
+            self.distribution = pillar
+            self.project = None
+        elif IProduct.providedBy(pillar):
+            self.project = pillar
+            self.distribution = None
+        else:
+            raise ValueError(
+                'The target of an OCIProject must be either an IDistribution '
+                'or IProduct instance.')
 
     @property
     def display_name(self):
@@ -214,16 +231,7 @@ class OCIProjectSet:
         target = OCIProject()
         target.date_created = date_created
         target.date_last_modified = date_created
-
-        # XXX twom 2019-10-10 This needs to have IProduct support
-        # when the model supports it
-        if IDistribution.providedBy(pillar):
-            target.distribution = pillar
-        else:
-            raise ValueError(
-                'The target of an OCIProject must be an '
-                'IDistribution instance.')
-
+        target.pillar = pillar
         target.registrant = registrant
         target.ociprojectname = name
         target.description = description
@@ -233,11 +241,30 @@ class OCIProjectSet:
         store.add(target)
         return target
 
-    def getByDistributionAndName(self, distribution, name):
+    def _get_pillar_attribute(self, pillar):
+        """Checks if the provided pillar is a valid one for OCIProject,
+        returning the model attribute where this pillar would be stored.
+
+        If pillar is not valid, raises ValueError.
+
+        :param pillar: A Distribution or Product.
+        :return: Storm attribute where the pillar would be stored.
+                 If pillar is not valid, raises ValueError.
+        """
+        if IDistribution.providedBy(pillar):
+            return OCIProject.distribution
+        elif IProduct.providedBy(pillar):
+            return OCIProject.project
+        else:
+            raise ValueError(
+                'The target of an OCIProject must be either an '
+                'IDistribution or an IProduct instance.')
+
+    def getByPillarAndName(self, pillar, name):
         """See `IOCIProjectSet`."""
         target = IStore(OCIProject).find(
             OCIProject,
-            OCIProject.distribution == distribution,
+            self._get_pillar_attribute(pillar) == pillar,
             OCIProject.ociprojectname == OCIProjectName.id,
             OCIProjectName.name == name).one()
         return target
diff --git a/lib/lp/registry/tests/test_ociproject.py b/lib/lp/registry/tests/test_ociproject.py
index 6ac98bb..c01f939 100644
--- a/lib/lp/registry/tests/test_ociproject.py
+++ b/lib/lp/registry/tests/test_ociproject.py
@@ -46,6 +46,30 @@ class TestOCIProject(TestCaseWithFactory):
         with admin_logged_in():
             self.assertProvides(oci_project, IOCIProject)
 
+    def test_product_pillar(self):
+        product = self.factory.makeProduct(name="some-project")
+        oci_project = self.factory.makeOCIProject(pillar=product)
+        self.assertEqual(product, oci_project.pillar)
+
+    def test_prevents_moving_pillar_to_invalid_type(self):
+        project = self.factory.makeProduct()
+        distro = self.factory.makeDistribution()
+
+        project_oci_project = self.factory.makeOCIProject(pillar=project)
+        distro_oci_project = self.factory.makeOCIProject(pillar=distro)
+
+        with admin_logged_in():
+            project_oci_project.pillar = distro
+            self.assertEqual(project_oci_project.distribution, distro)
+            self.assertIsNone(project_oci_project.project)
+
+            distro_oci_project.pillar = project
+            self.assertIsNone(distro_oci_project.distribution)
+            self.assertEqual(distro_oci_project.project, project)
+
+            self.assertRaises(
+                ValueError, setattr, distro_oci_project, 'pillar', 'Invalid')
+
     def test_newSeries(self):
         driver = self.factory.makePerson()
         distribution = self.factory.makeDistribution(driver=driver)
@@ -144,7 +168,7 @@ class TestOCIProjectSet(TestCaseWithFactory):
 
         with person_logged_in(registrant):
             fetched_result = getUtility(
-                IOCIProjectSet).getByDistributionAndName(
+                IOCIProjectSet).getByPillarAndName(
                     distribution, oci_project.ociprojectname.name)
             self.assertEqual(oci_project, fetched_result)
 
@@ -287,6 +311,5 @@ class TestOCIProjectWebservice(TestCaseWithFactory):
             other_user = self.factory.makePerson()
             distro = removeSecurityProxy(self.factory.makeDistribution(
                 owner=other_user))
-            url = api_url(distro)
 
         self.assertCanCreateOCIProject(distro, self.person)