launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:oci-recipe-model-bugs into launchpad:master.

Commit message:
Fix various bugs in OCIRecipe interfaces and models

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

Update OCIRecipe.date_last_modified on changes.

The IOCIRecipe interfaces had a number of small bugs.

makeOCIRecipeBuild needs to flush the store before returning in order for the build ID to be set.

I discovered all these problems while writing views.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:oci-recipe-model-bugs into launchpad:master.

diff --git a/lib/lp/oci/configure.zcml b/lib/lp/oci/configure.zcml
index 30212b9..4a67bbb 100644
--- a/lib/lp/oci/configure.zcml
+++ b/lib/lp/oci/configure.zcml
@@ -23,6 +23,11 @@
             permission="launchpad.Admin"
             set_schema="lp.oci.interfaces.ocirecipe.IOCIRecipeAdminAttributes" />
     </class>
+    <subscriber
+        for="lp.oci.interfaces.ocirecipe.IOCIRecipe
+             zope.lifecycleevent.interfaces.IObjectModifiedEvent"
+        handler="lp.oci.model.ocirecipe.oci_recipe_modified" />
+
     <securedutility
         class="lp.oci.model.ocirecipe.OCIRecipeSet"
         provides="lp.oci.interfaces.ocirecipe.IOCIRecipeSet">
diff --git a/lib/lp/oci/interfaces/ocirecipe.py b/lib/lp/oci/interfaces/ocirecipe.py
index c6ce359..f9ed77a 100644
--- a/lib/lp/oci/interfaces/ocirecipe.py
+++ b/lib/lp/oci/interfaces/ocirecipe.py
@@ -44,6 +44,7 @@ from lp.code.interfaces.gitref import IGitRef
 from lp.code.interfaces.gitrepository import IGitRepository
 from lp.registry.interfaces.ociproject import IOCIProject
 from lp.registry.interfaces.role import IHasOwner
+from lp.services.database.constants import DEFAULT
 from lp.services.fields import (
     PersonChoice,
     PublicPersonChoice,
@@ -145,7 +146,8 @@ class IOCIRecipeEditableAttributes(IHasOwner):
     """
 
     name = TextLine(
-        title=_("The name of this recipe."),
+        title=_("Name"),
+        description=_("The name of this recipe."),
         constraint=name_validator,
         required=True,
         readonly=False)
@@ -159,7 +161,8 @@ class IOCIRecipeEditableAttributes(IHasOwner):
 
     oci_project = Reference(
         IOCIProject,
-        title=_("The OCI project that this recipe is for."),
+        title=_("OCI project"),
+        description=_("The OCI project that this recipe is for."),
         required=True,
         readonly=True)
 
@@ -171,7 +174,7 @@ class IOCIRecipeEditableAttributes(IHasOwner):
         readonly=False)
 
     git_ref = Reference(
-        IGitRef, title=_("Git branch"), required=False, readonly=False,
+        IGitRef, title=_("Git branch"), required=True, readonly=False,
         description=_(
             "The Git branch containing a Dockerfile at the location "
             "defined by the build_file attribute."))
@@ -179,24 +182,27 @@ class IOCIRecipeEditableAttributes(IHasOwner):
     git_repository = ReferenceChoice(
         title=_("Git repository"),
         schema=IGitRepository, vocabulary="GitRepository",
-        required=False, readonly=True,
+        required=True, readonly=False,
         description=_(
             "A Git repository with a branch containing a Dockerfile "
             "at the location defined by the build_file attribute."))
 
     git_path = TextLine(
-        title=_("Git branch path"), required=False, readonly=False,
+        title=_("Git branch path"), required=True, readonly=False,
         description=_(
             "The path of the Git branch containing a Dockerfile "
             "at the location defined by the build_file attribute."))
 
     description = Text(
-        title=_("A short description of this recipe."),
+        title=_("Description"),
+        description=_("A short description of this recipe."),
+        required=False,
         readonly=False)
 
     build_file = TextLine(
-        title=_("The relative path to the file within this recipe's "
-                "branch that defines how to build the recipe."),
+        title=_("Build file path"),
+        description=_("The relative path to the file within this recipe's "
+                      "branch that defines how to build the recipe."),
         constraint=path_does_not_escape,
         required=True,
         readonly=False)
@@ -228,8 +234,9 @@ class IOCIRecipe(IOCIRecipeView, IOCIRecipeEdit, IOCIRecipeEditableAttributes,
 class IOCIRecipeSet(Interface):
     """A utility to create and access OCI Recipes."""
 
-    def new(name, registrant, owner, oci_project, git_ref, description,
-            official, require_virtualized, build_file, date_created):
+    def new(name, registrant, owner, oci_project, git_ref, build_file,
+            description=None, official=False, require_virtualized=True,
+            date_created=DEFAULT):
         """Create an IOCIRecipe."""
 
     def exists(owner, oci_project, name):
diff --git a/lib/lp/oci/model/ocirecipe.py b/lib/lp/oci/model/ocirecipe.py
index 8a01b2d..4555a38 100644
--- a/lib/lp/oci/model/ocirecipe.py
+++ b/lib/lp/oci/model/ocirecipe.py
@@ -36,8 +36,6 @@ from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
 from lp.buildmaster.model.buildfarmjob import BuildFarmJob
 from lp.buildmaster.model.buildqueue import BuildQueue
-from lp.code.model.gitcollection import GenericGitCollection
-from lp.code.model.gitrepository import GitRepository
 from lp.oci.interfaces.ocirecipe import (
     DuplicateOCIRecipeName,
     IOCIRecipe,
@@ -50,8 +48,10 @@ from lp.oci.interfaces.ocirecipe import (
 from lp.oci.interfaces.ocirecipebuild import IOCIRecipeBuildSet
 from lp.oci.model.ocirecipebuild import OCIRecipeBuild
 from lp.registry.interfaces.person import IPersonSet
-from lp.services.database.bulk import load_related
-from lp.services.database.constants import DEFAULT
+from lp.services.database.constants import (
+    DEFAULT,
+    UTC_NOW,
+    )
 from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.interfaces import (
     IMasterStore,
@@ -63,6 +63,15 @@ from lp.services.database.stormexpr import (
     )
 
 
+def oci_recipe_modified(recipe, event):
+    """Update the date_last_modified property when an OCIRecipe is modified.
+
+    This method is registered as a subscriber to `IObjectModifiedEvent`
+    events on OCI recipes.
+    """
+    removeSecurityProxy(recipe).date_last_modified = UTC_NOW
+
+
 @implementer(IOCIRecipe)
 class OCIRecipe(Storm):
 
@@ -84,7 +93,7 @@ class OCIRecipe(Storm):
     oci_project = Reference(oci_project_id, "OCIProject.id")
 
     name = Unicode(name="name", allow_none=False)
-    description = Unicode(name="description")
+    description = Unicode(name="description", allow_none=True)
 
     official = Bool(name="official", default=False)
 
@@ -111,6 +120,7 @@ class OCIRecipe(Storm):
         self.official = official
         self.require_virtualized = require_virtualized
         self.date_created = date_created
+        self.date_last_modified = date_created
         self.git_ref = git_ref
 
     def destroySelf(self):
diff --git a/lib/lp/oci/tests/test_ocirecipe.py b/lib/lp/oci/tests/test_ocirecipe.py
index db71400..5b614f0 100644
--- a/lib/lp/oci/tests/test_ocirecipe.py
+++ b/lib/lp/oci/tests/test_ocirecipe.py
@@ -19,6 +19,12 @@ from lp.oci.interfaces.ocirecipe import (
     OCIRecipeNotOwner,
     )
 from lp.oci.interfaces.ocirecipebuild import IOCIRecipeBuildSet
+from lp.services.database.constants import (
+    ONE_DAY_AGO,
+    UTC_NOW,
+    )
+from lp.services.database.sqlbase import flush_database_caches
+from lp.services.webapp.snapshot import notify_modified
 from lp.testing import (
     admin_logged_in,
     person_logged_in,
@@ -36,6 +42,20 @@ class TestOCIRecipe(TestCaseWithFactory):
         with admin_logged_in():
             self.assertProvides(target, IOCIRecipe)
 
+    def test_initial_date_last_modified(self):
+        # The initial value of date_last_modified is date_created.
+        recipe = self.factory.makeOCIRecipe(date_created=ONE_DAY_AGO)
+        self.assertEqual(recipe.date_created, recipe.date_last_modified)
+
+    def test_modifiedevent_sets_date_last_modified(self):
+        # When an OCIRecipe receives an object modified event, the last
+        # modified date is set to UTC_NOW.
+        recipe = self.factory.makeOCIRecipe(date_created=ONE_DAY_AGO)
+        with notify_modified(removeSecurityProxy(recipe), ["name"]):
+            pass
+        self.assertSqlAttributeEqualsDate(
+            recipe, "date_last_modified", UTC_NOW)
+
     def test_checkRequestBuild(self):
         ocirecipe = removeSecurityProxy(self.factory.makeOCIRecipe())
         unrelated_person = self.factory.makePerson()
@@ -69,6 +89,7 @@ class TestOCIRecipe(TestCaseWithFactory):
 
         with person_logged_in(oci_recipe.owner):
             oci_recipe.destroySelf()
+        flush_database_caches()
 
         for build_id in build_ids:
             self.assertIsNone(getUtility(IOCIRecipeBuildSet).getByID(build_id))
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index f0788d8..bac9b17 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -5002,6 +5002,7 @@ class BareLaunchpadObjectFactory(ObjectFactory):
         else:
             removeSecurityProxy(oci_build).updateStatus(
                 status, builder=builder)
+        IStore(oci_build).flush()
         return oci_build
 
     def makeOCIFile(self, build=None, library_file=None,