Merge lp:~wallyworld/launchpad/recipe-pending-build-testfix into lp:launchpad

[Ian Booth]

[r=bac][bug=728789] Ensure pending builds are always correctly disabled on the recipe request build popup form.

This change is a more complex but necessary way of fixing the correct rendering of existing pending builds on the Request Builds popup. The original way worked with the sample data but failed on some production data.

== Implementation ==

Is is necessary for the popup form, when displayed, to perform a named_get call to the new getPendingBuildInfo() method on the recipe. This call returns the required data (distroseries displayname and archive token) for each pending build, allowing the corresponding checkboxes to be disabled on the popup form. Previously a simple pending_builds_collection_link retrieval was performed but this only returns links to the distroseries and archive items and not the required data.

== Tests ==

A windmill test:
bin/test -vvt test_recipe_build_request_already_pending

== Lint ==

Linting changed files:
  lib/lp/code/interfaces/sourcepackagerecipe.py
  lib/lp/code/javascript/requestbuild_overlay.js
  lib/lp/code/model/sourcepackagerecipe.py

./lib/lp/code/interfaces/sourcepackagerecipe.py
     228: E501 line too long (83 characters)
     228: Line exceeds 78 characters.
./lib/lp/code/javascript/requestbuild_overlay.js
     110: Line exceeds 78 characters.
     148: Line exceeds 78 characters.
     180: Line exceeds 78 characters.

[Brad Crittenden (bac)]

Hi Ian,

Thanks for the branch. It all seems to make sense.

I would like to see a unit test for your new method getPendingBuildInfo.
And please clean up the issues pointed out by lint if they are real.

review: Needs Fixing (code)

[Ian Booth (wallyworld)]

> Hi Ian,
>
> Thanks for the branch. It all seems to make sense.
>
> I would like to see a unit test for your new method getPendingBuildInfo.
> And please clean up the issues pointed out by lint if they are real.

Thanks for the review. I've added a unit test for the new method and also for the web service invocation. I've also cleaned up up the pre-existing lint issues.

[Brad Crittenden (bac)]

Thanks for adding the tests Ian -- it looks good.

review: Approve (code)

=== modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py'
--- lib/lp/code/interfaces/sourcepackagerecipe.py	2011-03-01 18:02:21 +0000
+++ lib/lp/code/interfaces/sourcepackagerecipe.py	2011-03-11 23:58:19 +0000
@@ -24,6 +24,7 @@
 from lazr.restful.declarations import (
     call_with,
     export_as_webservice_entry,
+    export_read_operation,
     export_write_operation,
     exported,
     mutator_for,
@@ -166,6 +167,19 @@
     def performDailyBuild():
         """Perform a build into the daily build archive."""
 
+    @export_read_operation()
+    @operation_for_version("devel")
+    def getPendingBuildInfo():
+        """Find distroseries and archive data for pending builds.
+
+        Return a list of dict(
+        distroseries:distroseries.displayname
+        archive:archive.token)
+        The archive token is the same as that defined by the archive vocab:
+        archive.owner.name/archive.name
+        This information is used to construct the request builds popup form.
+        """
+
 
 class ISourcePackageRecipeEdit(Interface):
     """ISourcePackageRecipe methods that require launchpad.Edit permission."""
@@ -211,7 +225,8 @@
         readonly=False)
     build_daily = exported(Bool(
         title=_("Built daily"),
-        description=_("Automatically build each day, if the source has changed.")))
+        description=_(
+            "Automatically build each day, if the source has changed.")))
 
     name = exported(TextLine(
             title=_("Name"), required=True,

=== modified file 'lib/lp/code/javascript/requestbuild_overlay.js'
--- lib/lp/code/javascript/requestbuild_overlay.js	2011-03-07 07:13:52 +0000
+++ lib/lp/code/javascript/requestbuild_overlay.js	2011-03-11 23:58:19 +0000
@@ -107,7 +107,8 @@
     request_build_response_handler.showError = function(header, error_msgs) {
         if (header == null) {
             if (error_msgs == null)
-                header = "An error occurred, please contact an administrator.";
+                header = "An error occurred, please contact an " +
+                         "administrator.";
             else
                 header = "The following errors occurred:";
         }
@@ -145,7 +146,8 @@
         var submit_url = base_url+"/+request-daily-build";
         var current_builds = harvest_current_build_records();
 
-        var qs = Y.lp.client.append_qs('', 'field.actions.build', 'Build now');
+        var qs = Y.lp.client.append_qs(
+                            '', 'field.actions.build', 'Build now');
         var y_config = {
             method: "POST",
             headers: {'Accept': 'application/xhtml'},
@@ -177,7 +179,8 @@
                                                 {nr_new: nr_new});
                         }
                         var build_message_node = Y.Node.create([
-                            '<div id="new-builds-info" class="build-informational">',
+                            '<div id="new-builds-info" ' +
+                                    'class="build-informational">',
                             new_builds_message,
                             '</div>'].join(''));
                         request_daily_build_handle.insert(
@@ -410,15 +413,13 @@
 
     var nr_matches = 0;
     Y.Array.each(last_known_pending_builds, function(distroarchive) {
-        var archive_parts = ppa_value.split("/");
-        var archive = archive_parts.pop();
-        if( archive.toLowerCase() != distroarchive[1].toLowerCase() )
+        if (ppa_value != distroarchive[1])
             return;
 
         for (var i=0; i<distroseries_nodes.size(); i++) {
             var distroseries_node = distroseries_nodes.item(i);
             var distro = distroseries_node.get("text").trim();
-            if (distro.toLowerCase() == distroarchive[0].toLowerCase() ) {
+            if (distro == distroarchive[0]) {
                 nr_matches += 1;
                 var disabled_checkbox_html = Y.Lang.substitute(
                     DISABLED_DISTROSERIES_CHECKBOX_HTML, {distro: distro});
@@ -442,6 +443,12 @@
     // or be initiated after this initial lookup, but we do handle that
     // situation in the error handling.
 
+    // The ui may not be ready yet.
+    var ppa_selector = request_build_overlay.form_node.one(
+            "[name='field.archive']");
+    if (ppa_selector == null) {
+        return;
+    }
     distroseries_node_html = new Array();
     last_known_pending_builds = new Array();
     var y_config = {
@@ -457,28 +464,16 @@
                                 distro_node.get("innerHTML"));
                     });
 
-                    //We have a json collection of the pending build records.
-                    var size = build_info.total_size;
+                    // We have a collection of the pending build info.
+                    // The info is the distroseries displayname and archive
+                    // token.
+                    var size = build_info.length;
                     for( var i=0; i<size; i++ ) {
-                        var build_record = build_info.entries[i];
-                        var distro_link = build_record.get(
-                                                "distro_series_link");
-                        var archive_link = build_record.get("archive_link");
-
-                        // The very last segment of the distro series and
-                        // archive links will be the bits we want to match on.
-                        // eg for distro series, the link will be like:
-                        // https://code.launchpad.dev/api/devel/ubuntu/warty
-                        // and "warty" is what we match on.
-                        var distro_parts = distro_link.split("/");
-                        var archive_parts = archive_link.split("/");
+                        var build_record = build_info[i];
+                        var distro_name = build_record["distroseries"];
+                        var archive_token = build_record["archive"];
                         last_known_pending_builds.push(
-                                [distro_parts.pop(), archive_parts.pop()])
-                    }
-                    var ppa_selector = request_build_overlay.form_node.one(
-                            "[name='field.archive']");
-                    if (ppa_selector == null) {
-                        return;
+                                [distro_name, archive_token]);
                     }
                     ppa_selector.on("change", function(e) {
                         ppa_changed(ppa_selector.get("value"), submit_button);
@@ -488,7 +483,8 @@
         }
     };
     set_up_lp_client();
-    lp_client.get(LP.cache.context.pending_builds_collection_link, y_config);
+    lp_client.named_get(
+            LP.cache.context.self_link, 'getPendingBuildInfo', y_config);
 }
 }, "0.1", {"requires": [
     "dom", "node", "io-base", "lazr.anim", "lazr.formoverlay", "lp.client"

=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2011-02-25 08:27:08 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2011-03-11 23:58:19 +0000
@@ -338,6 +338,17 @@
         result.order_by(order_by)
         return result
 
+    def getPendingBuildInfo(self):
+        """See `ISourcePackageRecipe`."""
+        builds = self.pending_builds
+        result = []
+        for build in builds:
+            result.append(
+                {"distroseries": build.distroseries.displayname,
+                 "archive": '%s/%s' %
+                           (build.archive.owner.name, build.archive.name)})
+        return result
+
     @property
     def last_build(self):
         """See `ISourcePackageRecipeBuild`."""

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2011-03-09 20:12:10 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2011-03-11 23:58:19 +0000
@@ -599,6 +599,22 @@
         self.assertEqual(builds[1:], list(recipe.pending_builds))
         self.assertEqual(builds, list(recipe.builds))
 
+    def test_getPendingBuildInfo(self):
+        """SourcePackageRecipe.getPendingBuildInfo() is as expected."""
+        person = self.factory.makePerson()
+        archives = [self.factory.makeArchive(owner=person) for x in range(4)]
+        distroseries= self.factory.makeSourcePackageRecipeDistroseries()
+        recipe = self.factory.makeSourcePackageRecipe()
+
+        build_info = []
+        for archive in archives:
+            build = recipe.requestBuild(archive, person, distroseries)
+            build_info.insert(0, {
+                "distroseries": distroseries.displayname,
+                "archive": '%s/%s' %
+                           (archive.owner.name, archive.name)})
+        self.assertEqual(build_info, list(recipe.getPendingBuildInfo()))
+
     def test_getBuilds_cancelled(self):
         # Cancelled builds are not considered pending.
         recipe = self.factory.makeSourcePackageRecipe()
@@ -847,7 +863,8 @@
         branch = self.factory.makeBranch()
         return MINIMAL_RECIPE_TEXT % branch.bzr_identity
 
-    def makeRecipe(self, user=None, owner=None, recipe_text=None, version='devel'):
+    def makeRecipe(self, user=None, owner=None, recipe_text=None,
+                   version='devel'):
         # rockstar 21 Jul 2010 - This function does more commits than I'd
         # like, but it's the result of the fact that the webservice runs in a
         # separate thread so doesn't get the database updates without those
@@ -1006,3 +1023,24 @@
         self.assertEqual(builds, list(recipe.pending_builds))
         self.assertEqual(builds, list(recipe.builds))
         self.assertEqual([], list(recipe.completed_builds))
+
+    def test_getPendingBuildInfo(self):
+        """SourcePackageRecipe.getPendingBuildInfo() is as expected."""
+        person = self.factory.makePerson()
+        archives = [self.factory.makeArchive(owner=person) for x in range(4)]
+        distroseries= self.factory.makeSourcePackageRecipeDistroseries()
+
+        recipe, user, launchpad = self.makeRecipe(person)
+        ws_distroseries = ws_object(launchpad, distroseries)
+
+        build_info = []
+        for archive in archives:
+            ws_archive = ws_object(launchpad, archive)
+            build = recipe.requestBuild(
+                archive=ws_archive, distroseries=ws_distroseries,
+                pocket=PackagePublishingPocket.RELEASE.title)
+            build_info.insert(0, {
+                "distroseries": distroseries.displayname,
+                "archive": '%s/%s' %
+                           (archive.owner.name, archive.name)})
+        self.assertEqual(build_info, list(recipe.getPendingBuildInfo()))