launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/recipe-pending-build-testfix into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #728789 in Launchpad itself: ""Request build" dialog indicates builds are pending when they are not and doesn't allow new builds"
  https://bugs.launchpad.net/launchpad/+bug/728789

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/recipe-pending-build-testfix/+merge/53033

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.

-- 
https://code.launchpad.net/~wallyworld/launchpad/recipe-pending-build-testfix/+merge/53033
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/recipe-pending-build-testfix into lp:launchpad.

=== 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 15:53:25 +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()
+    def getPendingBuildInfo():
+        """Find distroseries and archive data for pending builds.
+
+        Return a list of {
+            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."""

=== 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 15:53:25 +0000
@@ -410,15 +410,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 +440,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 +461,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 +480,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 15:53:25 +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`."""