Merge lp:~rockstar/launchpad/merge-queues-api into lp:launchpad/db-devel

[Paul Hummer]

This branch exposes the new branch merge queue models through the API. Since there's no UI yet, the URL resolving code also needed to be added. While doing this, I also added some more stuff to the factory method for making merge queues.

There's a lot of lint, but most of it isn't something I introduced. I could fix the lint, but it would make the diff hard to read. If you'd like me to, I'll fix the lint, but I think you'd rather me do that after the review. :)

[Jelmer Vernooij (jelmer)]

Hi Paul,

Just a few minor (stylistic) comments. I'm looking forward to being able
to use the merge queues.

On Wed, 2010-10-20 at 15:37 +0000, Paul Hummer wrote:
> There's a lot of lint, but most of it isn't something I introduced. I
> could fix the lint, but it would make the diff hard to read. If you'd
> like me to, I'll fix the lint, but I think you'd rather me do that
> after the review. :)
Thanks for waiting with those fixes. Some lint cleanup would be nice
though. :-)

> differences between files attachment (review-diff.txt)

> === modified file 'lib/lp/code/interfaces/branch.py'
> --- lib/lp/code/interfaces/branch.py 2010-10-20 15:36:54 +0000
> +++ lib/lp/code/interfaces/branch.py 2010-10-20 15:36:56 +0000
> @@ -990,18 +990,6 @@
> required=False, readonly=True,
> vocabulary=ControlFormat))
>
> - merge_queue = Reference(
> - title=_('Branch Merge Queue'),
> - schema=IBranchMergeQueue, required=False, readonly=True,
> - description=_(
> - "The branch merge queue that manages merges for this branch."))
> -
> - merge_queue_config = TextLine(
> - title=_('Name'), required=True, constraint=branch_name_validator,
> - description=_(
> - "A JSON string of configuration values to send to a branch"
> - "merge robot."))
This indentation seems odd - is there a particular reason for not having
the title= and description= lines start at the same column for
merge_queue_config as for merge_queue ?

> === modified file 'lib/lp/registry/model/person.py'
> --- lib/lp/registry/model/person.py 2010-10-18 21:18:03 +0000
> +++ lib/lp/registry/model/person.py 2010-10-20 15:36:56 +0000
> @@ -2661,6 +2661,13 @@
> SourcePackageRecipe, SourcePackageRecipe.owner == self,
> SourcePackageRecipe.name == name).one()
>
> + def getMergeQueue(self, name):
> + from lp.code.model.branchmergequeue import BranchMergeQueue
> + return Store.of(self).find(
> + BranchMergeQueue,
> + BranchMergeQueue.owner == self,
> + BranchMergeQueue.name == unicode(name)).one()
> +
^^ It would be nice to have a "See IPerson" comment here, if only for
consistency with the rest of the file.

  review approve

Cheers,

Jelmer

review: Approve

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2010-10-06 18:53:53 +0000
+++ lib/lp/code/browser/configure.zcml	2010-10-20 17:56:59 +0000
@@ -1326,4 +1326,10 @@
             permission="zope.Public"/>
     </facet>
 
+    <browser:url
+        for="lp.code.interfaces.branchmergequeue.IBranchMergeQueue"
+        attribute_to_parent="owner"
+        path_expression="string:+merge-queues/${name}"
+        rootsite="code" />
+
 </configure>

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2010-10-18 21:18:03 +0000
+++ lib/lp/code/configure.zcml	2010-10-20 17:56:59 +0000
@@ -22,6 +22,11 @@
       provides="zope.publisher.interfaces.browser.IDefaultBrowserLayer"
       name="code" />
 
+  <!-- Branch Merge Queues -->
+  <class class="lp.code.model.branchmergequeue.BranchMergeQueue">
+    <allow interface="lp.code.interfaces.branchmergequeue.IBranchMergeQueue" />
+  </class>
+
   <class class="lp.code.model.codereviewvote.CodeReviewVoteReference">
     <allow interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReferencePublic"/>
     <require
@@ -433,18 +438,18 @@
                    lp.code.interfaces.branch.IBranchAnyone
                    lp.code.interfaces.branch.IBranchEditableAttributes
                    lp.code.interfaces.branch.IBranchPublic
-                   lp.code.interfaces.branch.IBranchView
-                   "/>
+                   lp.code.interfaces.branch.IBranchView"
+        attributes="merge_queue merge_queue_config"/>
     <require
         permission="launchpad.Edit"
         interface="lp.code.interfaces.branch.IBranchEdit"
         set_schema="lp.code.interfaces.branch.IBranchEditableAttributes"
-        attributes="setPrivate"
+        attributes="setPrivate addToQueue setMergeQueueConfig"
         set_attributes="branch_format control_format repository_format
                         branch_type
                         last_scanned last_scanned_id
                         last_mirrored last_mirrored_id next_mirror_time
-                        revision_count merge_queue mirror_failures
+                        revision_count mirror_failures
                         stacked_on mirror_status_message"/>
     <require
         permission="launchpad.AnyPerson"

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2010-10-20 17:56:58 +0000
+++ lib/lp/code/interfaces/branch.py	2010-10-20 17:56:59 +0000
@@ -990,18 +990,6 @@
             required=False, readonly=True,
             vocabulary=ControlFormat))
 
-    merge_queue = Reference(
-        title=_('Branch Merge Queue'),
-        schema=IBranchMergeQueue, required=False, readonly=True,
-        description=_(
-            "The branch merge queue that manages merges for this branch."))
-
-    merge_queue_config = TextLine(
-            title=_('Name'), required=True, constraint=branch_name_validator,
-            description=_(
-                "A JSON string of configuration values to send to a branch"
-                "merge robot."))
-
 
 class IBranchEdit(Interface):
     """IBranch attributes that require launchpad.Edit permission."""
@@ -1031,9 +1019,9 @@
         is set, the branch gets moved into the junk namespace of the branch
         owner.
 
-        :raise: `BranchTargetError` if both project and source_package are set,
-          or if either the project or source_package fail to be adapted to an
-          IBranchTarget.
+        :raise: `BranchTargetError` if both project and source_package are
+          set, or if either the project or source_package fail to be
+          adapted to an IBranchTarget.
         """
 
     def requestUpgrade():
@@ -1075,6 +1063,30 @@
         :raise: CannotDeleteBranch if the branch cannot be deleted.
         """
 
+
+class IMergeQueueable(Interface):
+    """An interface for branches that can be queued."""
+
+    merge_queue = exported(
+        Reference(
+            title=_('Branch Merge Queue'),
+            schema=IBranchMergeQueue, required=False, readonly=True,
+            description=_(
+                "The branch merge queue that manages merges for this "
+                "branch.")))
+
+    merge_queue_config = exported(
+        TextLine(
+            title=_('Name'), required=True, readonly=True,
+            description=_(
+                "A JSON string of configuration values to send to a "
+                "branch merge robot.")))
+
+    @mutator_for(merge_queue)
+    @operation_parameters(
+        queue=Reference(title=_('Branch Merge Queue'),
+              schema=IBranchMergeQueue))
+    @export_write_operation()
     def addToQueue(queue):
         """Add this branch to a specified queue.
 
@@ -1083,6 +1095,10 @@
         :param queue: The branch merge queue that will manage the branch.
         """
 
+    @mutator_for(merge_queue_config)
+    @operation_parameters(
+        config=TextLine(title=_("A JSON string of config values.")))
+    @export_write_operation()
     def setMergeQueueConfig(config):
         """Set the merge_queue_config property.
 
@@ -1094,7 +1110,7 @@
 
 
 class IBranch(IBranchPublic, IBranchView, IBranchEdit,
-              IBranchEditableAttributes, IBranchAnyone):
+              IBranchEditableAttributes, IBranchAnyone, IMergeQueueable):
     """A Bazaar branch."""
 
     # Mark branches as exported entries for the Launchpad API.

=== modified file 'lib/lp/code/interfaces/branchmergequeue.py'
--- lib/lp/code/interfaces/branchmergequeue.py	2010-10-20 17:56:58 +0000
+++ lib/lp/code/interfaces/branchmergequeue.py	2010-10-20 17:56:59 +0000
@@ -10,6 +10,13 @@
     'IBranchMergeQueueSource',
     ]
 
+from lazr.restful.declarations import (
+    export_as_webservice_entry,
+    export_write_operation,
+    exported,
+    mutator_for,
+    operation_parameters,
+    )
 from lazr.restful.fields import (
     CollectionField,
     Reference,
@@ -32,47 +39,61 @@
 class IBranchMergeQueue(Interface):
     """An interface for managing branch merges."""
 
+    export_as_webservice_entry()
+
     id = Int(title=_('ID'), readonly=True, required=True)
 
-    registrant = PublicPersonChoice(
-        title=_("The user that registered the branch."),
-        required=True, readonly=True,
-        vocabulary='ValidPersonOrTeam')
-
-    owner = PersonChoice(
-        title=_('Owner'),
-        required=True, readonly=True,
-        vocabulary='UserTeamsParticipationPlusSelf',
-        description=_("The owner of the merge queue."))
-
-    name = TextLine(
-        title=_('Name'), required=True,
-        description=_(
-            "Keep very short, unique, and descriptive, because it will "
-            "be used in URLs.  "
-            "Examples: main, devel, release-1.0, gnome-vfs."))
-
-    description = Text(
-        title=_('Description'), required=False,
-        description=_(
-            'A short description of the purpose of this merge queue.'))
-
-    configuration = TextLine(
-        title=_('Configuration'), required=False,
-        description=_(
-            "A JSON string of configuration values."))
-
-    date_created = Datetime(
-        title=_('Date Created'),
-        required=True,
-        readonly=True)
-
-    branches = CollectionField(
-        title=_('Dependent Branches'),
-        description=_('A collection of branches that this queue manages.'),
-        readonly=True,
-        value_type=Reference(Interface))
-
+    registrant = exported(
+        PublicPersonChoice(
+            title=_("The user that registered the branch."),
+            required=True, readonly=True,
+            vocabulary='ValidPersonOrTeam'))
+
+    owner = exported(
+        PersonChoice(
+            title=_('Owner'),
+            required=True, readonly=True,
+            vocabulary='UserTeamsParticipationPlusSelf',
+            description=_("The owner of the merge queue.")))
+
+    name = exported(
+        TextLine(
+            title=_('Name'), required=True,
+            description=_(
+                "Keep very short, unique, and descriptive, because it will "
+                "be used in URLs.  "
+                "Examples: main, devel, release-1.0, gnome-vfs.")))
+
+    description = exported(
+        Text(
+            title=_('Description'), required=False,
+            description=_(
+                'A short description of the purpose of this merge queue.')))
+
+    configuration = exported(
+        TextLine(
+            title=_('Configuration'), required=False, readonly=True,
+            description=_(
+                "A JSON string of configuration values.")))
+
+    date_created = exported(
+        Datetime(
+            title=_('Date Created'),
+            required=True,
+            readonly=True))
+
+    branches = exported(
+        CollectionField(
+            title=_('Dependent Branches'),
+            description=_(
+                'A collection of branches that this queue manages.'),
+            readonly=True,
+            value_type=Reference(Interface)))
+
+    @mutator_for(configuration)
+    @operation_parameters(
+        config=TextLine(title=_("A JSON string of configuration values.")))
+    @export_write_operation()
     def setMergeQueueConfig(config):
         """Set the JSON string configuration of the merge queue.
 

=== modified file 'lib/lp/code/interfaces/webservice.py'
--- lib/lp/code/interfaces/webservice.py	2010-08-20 20:31:18 +0000
+++ lib/lp/code/interfaces/webservice.py	2010-10-20 17:56:59 +0000
@@ -20,6 +20,7 @@
     IBranchSet,
     )
 from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
+from lp.code.interfaces.branchmergequeue import IBranchMergeQueue
 from lp.code.interfaces.branchsubscription import IBranchSubscription
 from lp.code.interfaces.codeimport import ICodeImport
 from lp.code.interfaces.codereviewcomment import ICodeReviewComment
@@ -35,3 +36,4 @@
     )
 
 
+IBranchMergeQueue['branches'].value_type.schema = IBranch

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2010-10-20 17:56:58 +0000
+++ lib/lp/code/model/tests/test_branch.py	2010-10-20 17:56:59 +0000
@@ -33,6 +33,7 @@
 from canonical.launchpad.interfaces.lpstorm import IStore
 from canonical.launchpad.webapp.interfaces import IOpenLaunchBag
 from canonical.testing.layers import (
+    AppServerLayer,
     DatabaseFunctionalLayer,
     LaunchpadZopelessLayer,
     )
@@ -118,6 +119,7 @@
 from lp.testing import (
     ANONYMOUS,
     celebrity_logged_in,
+    launchpadlib_for,
     login,
     login_person,
     logout,
@@ -126,6 +128,7 @@
     TestCase,
     TestCaseWithFactory,
     time_counter,
+    ws_object,
     )
 from lp.testing.factory import LaunchpadObjectFactory
 from lp.translations.model.translationtemplatesbuildjob import (
@@ -2744,5 +2747,45 @@
                 config)
 
 
+class TestWebservice(TestCaseWithFactory):
+    """Tests for the webservice."""
+
+    layer = AppServerLayer
+
+    def test_set_merge_queue(self):
+        """Test that the merge queue can be set properly."""
+        with person_logged_in(ANONYMOUS):
+            db_queue = self.factory.makeBranchMergeQueue()
+            db_branch = self.factory.makeBranch()
+            launchpad = launchpadlib_for('test', db_branch.owner,
+                service_root="http://api.launchpad.dev:8085")
+
+        configuration = simplejson.dumps({'test': 'make check'})
+
+        branch = ws_object(launchpad, db_branch)
+        queue = ws_object(launchpad, db_queue)
+        branch.merge_queue = queue
+        branch.lp_save()
+
+        branch2 = ws_object(launchpad, db_branch)
+        self.assertEqual(branch2.merge_queue, queue)
+
+    def test_set_configuration(self):
+        """Test the mutator for setting configuration."""
+        with person_logged_in(ANONYMOUS):
+            db_branch = self.factory.makeBranch()
+            launchpad = launchpadlib_for('test', db_branch.owner,
+                service_root="http://api.launchpad.dev:8085")
+
+        configuration = simplejson.dumps({'test': 'make check'})
+
+        branch = ws_object(launchpad, db_branch)
+        branch.merge_queue_config = configuration
+        branch.lp_save()
+
+        branch2 = ws_object(launchpad, db_branch)
+        self.assertEqual(branch2.merge_queue_config, configuration)
+
+
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/code/model/tests/test_branchmergequeue.py'
--- lib/lp/code/model/tests/test_branchmergequeue.py	2010-10-20 17:56:58 +0000
+++ lib/lp/code/model/tests/test_branchmergequeue.py	2010-10-20 17:56:59 +0000
@@ -7,15 +7,23 @@
 
 import simplejson
 
+import transaction
+
 from canonical.launchpad.interfaces.lpstorm import IStore
 from canonical.launchpad.webapp.testing import verifyObject
-from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.testing.layers import (
+    AppServerLayer,
+    DatabaseFunctionalLayer,
+    )
 from lp.code.errors import InvalidMergeQueueConfig
 from lp.code.interfaces.branchmergequeue import IBranchMergeQueue
 from lp.code.model.branchmergequeue import BranchMergeQueue
 from lp.testing import (
+    ANONYMOUS,
     person_logged_in,
+    launchpadlib_for,
     TestCaseWithFactory,
+    ws_object,
     )
 
 
@@ -89,3 +97,59 @@
             InvalidMergeQueueConfig,
             queue.setMergeQueueConfig,
             'abc')
+
+
+class TestWebservice(TestCaseWithFactory):
+
+    layer = AppServerLayer
+
+    def test_properties(self):
+        """Test that the correct properties are exposed."""
+        with person_logged_in(ANONYMOUS):
+            name = u'teh-queue'
+            description = u'Oh hai! I are a queues'
+            configuration = unicode(simplejson.dumps({'test': 'make check'}))
+
+            queuer = self.factory.makePerson()
+            db_queue = self.factory.makeBranchMergeQueue(
+                registrant=queuer, owner=queuer, name=name,
+                description=description,
+                configuration=configuration)
+            branch1 = self.factory.makeBranch()
+            with person_logged_in(branch1.owner):
+                branch1.addToQueue(db_queue)
+            branch2 = self.factory.makeBranch()
+            with person_logged_in(branch2.owner):
+                branch2.addToQueue(db_queue)
+            launchpad = launchpadlib_for('test', db_queue.owner,
+                service_root="http://api.launchpad.dev:8085")
+            transaction.commit()
+
+        queuer = ws_object(launchpad, queuer)
+        queue = ws_object(launchpad, db_queue)
+        branch1 = ws_object(launchpad, branch1)
+        branch2 = ws_object(launchpad, branch2)
+
+        self.assertEqual(queue.registrant, queuer)
+        self.assertEqual(queue.owner, queuer)
+        self.assertEqual(queue.name, name)
+        self.assertEqual(queue.description, description)
+        self.assertEqual(queue.configuration, configuration)
+        self.assertEqual(queue.date_created, db_queue.date_created)
+        self.assertEqual(len(queue.branches), 2)
+
+    def test_set_configuration(self):
+        """Test the mutator for setting configuration."""
+        with person_logged_in(ANONYMOUS):
+            db_queue = self.factory.makeBranchMergeQueue()
+            launchpad = launchpadlib_for('test', db_queue.owner,
+                service_root="http://api.launchpad.dev:8085")
+
+        configuration = simplejson.dumps({'test': 'make check'})
+
+        queue = ws_object(launchpad, db_queue)
+        queue.configuration = configuration
+        queue.lp_save()
+
+        queue2 = ws_object(launchpad, db_queue)
+        self.assertEqual(queue2.configuration, configuration)

=== modified file 'lib/lp/code/stories/webservice/xx-branch.txt'
--- lib/lp/code/stories/webservice/xx-branch.txt	2010-04-09 01:30:22 +0000
+++ lib/lp/code/stories/webservice/xx-branch.txt	2010-10-20 17:56:59 +0000
@@ -123,6 +123,8 @@
     last_scanned_id: None
     lifecycle_status: u'Development'
     linked_bugs_collection_link: u'http://.../~eric/fooix/trunk/linked_bugs'
+    merge_queue_config: None
+    merge_queue_link: None
     mirror_status_message: None
     name: u'trunk'
     owner_link: u'.../~eric'

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-10-14 20:20:47 +0000
+++ lib/lp/registry/browser/person.py	2010-10-20 17:56:59 +0000
@@ -543,6 +543,11 @@
         """Traverse to this person's recipes."""
         return self.context.getRecipe(name)
 
+    @stepthrough('+merge-queues')
+    def traverse_merge_queue(self, name):
+        """Traverse to this person's merge queues."""
+        return self.context.getMergeQueue(name)
+
 
 class TeamNavigation(PersonNavigation):
 

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2010-10-07 14:03:32 +0000
+++ lib/lp/registry/interfaces/person.py	2010-10-20 17:56:59 +0000
@@ -896,6 +896,9 @@
     def getRecipe(name):
         """Return the person's recipe with the given name."""
 
+    def getMergeQueue(name):
+        """Return the person's merge queue with the given name."""
+
     @call_with(requester=REQUEST_USER)
     @export_read_operation()
     def getArchiveSubscriptionURLs(requester):

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-10-18 21:18:03 +0000
+++ lib/lp/registry/model/person.py	2010-10-20 17:56:59 +0000
@@ -2661,6 +2661,13 @@
             SourcePackageRecipe, SourcePackageRecipe.owner == self,
             SourcePackageRecipe.name == name).one()
 
+    def getMergeQueue(self, name):
+        from lp.code.model.branchmergequeue import BranchMergeQueue
+        return Store.of(self).find(
+            BranchMergeQueue,
+            BranchMergeQueue.owner == self,
+            BranchMergeQueue.name == unicode(name)).one()
+
     def isUploader(self, distribution):
         """See `IPerson`."""
         permissions = getUtility(IArchivePermissionSet).componentsForUploader(

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-10-20 17:56:58 +0000
+++ lib/lp/testing/factory.py	2010-10-20 17:56:59 +0000
@@ -1098,16 +1098,23 @@
         namespace = target.getNamespace(owner)
         return namespace.createBranch(branch_type, name, creator)
 
-    def makeBranchMergeQueue(self):
+    def makeBranchMergeQueue(self, registrant=None, owner=None, name=None,
+                             description=None, configuration=None):
         """Create a BranchMergeQueue."""
-        name = unicode(self.getUniqueString('queue'))
-        owner = self.makePerson()
-        description = unicode(self.getUniqueString('queue-description'))
-        configuration = unicode(simplejson.dumps({
-            self.getUniqueString('key'): self.getUniqueString('value')}))
+        if name is None:
+            name = unicode(self.getUniqueString('queue'))
+        if owner is None:
+            owner = self.makePerson()
+        if registrant is None:
+            registrant = self.makePerson()
+        if description is None:
+            description = unicode(self.getUniqueString('queue-description'))
+        if configuration is None:
+            configuration = unicode(simplejson.dumps({
+                self.getUniqueString('key'): self.getUniqueString('value')}))
 
         queue = BranchMergeQueue.new(
-            name, owner, owner, description, configuration)
+            name, registrant, owner, description, configuration)
         return queue
 
     def enableDefaultStackingForProduct(self, product, branch=None):