Merge lp:~rvb/launchpad/init-multiple-parents-packagesetpicker into lp:launchpad

[Raphaël Badin]

[r=jtv][bug=793603] Fix the packagesets widget on initseries page.

This branch fixes the packagesets widget on the +initseries page. The packagesets from a parent where not removed properly when the parent was removed from the parent picker widget.

= Q/A =

On DF:
- Create a new series
- Go to the initialisation page series/+initseries
- Add a parent with packagesets
- Make sure the parent's packagesets are added to the packagesets widget
- Remove the parent
- Make sure the packagesets from this parent are properly removed from the packagesets widget

= Tests =

lib/lp/registry/javascript/tests/test_distroseries.html
(testRemoveDistroSeriesUpdatesPackageSets)

[Jeroen T. Vermeulen (jtv)]

Hi Raphaël,

I really need some help with this. Without comments, I don't know what anything does and where it's tested! That makes it very hard for me to say anything more useful about the code than that it needs comments. I *think* there's a couple of bugs, but I'm not sure.

Let's have a look at the diff:

 === modified file 'lib/lp/registry/javascript/distroseries.js'
2 --- lib/lp/registry/javascript/distroseries.js 2011-06-01 10:31:05 +0000
3 +++ lib/lp/registry/javascript/distroseries.js 2011-06-07 21:48:32 +0000

15 @@ -1073,6 +1073,7 @@
16 * @method add_packagesets
17 */
18 add_packagesets: function(packagesets, distroseries) {
19 + this._packagesets[distroseries.value] = packagesets.entries;

What is distroseries exactly, and more to the point, what is distroseries.value? It would be helpful to have some documentation of the parameters. Is distroseries.value a distroseries id?

23 @@ -1095,9 +1096,12 @@
24 * @method remove_distroseries
25 */
26 remove_distroseries: function(distroseries_id) {
27 - this.fieldNode.all(
28 - 'option[value$="-' + distroseries_id + '"]').remove();
29 - Y.lazr.anim.green_flash({node: this.fieldNode}).run();
30 + this._packagesets[distroseries_id].forEach(
31 + function(packageset) {
32 + this.fieldNode.all(
33 + 'option[value$="' + packageset.get("id") + '"]').remove();
34 + }, this);
35 + Y.lazr.anim.green_flash({node: this.fieldNode}).run();

The new code looks like it may be slower. Could that be an issue?

I had a hard time understanding the this.fieldNode.all(). The old code looked for:

    'option[value$="-' + distroseries_id + '"]'

As far as I can tell, that's a CSS3 selector for "value ends with a dash followed by distroseries_id." Is it safe for us to use CSS3 selectors? AFAICT CSS2 does not have $=.

The new code looks for:

    'option[value$="' + packageset.get("id") + '"]'

That means "value ends with the packageset id." I may be misunderstanding it, but it looks wrong to me in two ways:

1. So what does that value end in — distroseries id or packageset id!?

2. What about spurious substring matches, e.g. won't this also remove "321" when you really want to remove only "21"?

39 @@ -1109,6 +1113,7 @@
40 this.error_handler.clearProgressUI = Y.bind(this.hideSpinner, this);
41 this.error_handler.showError = Y.bind(this.showError, this);
42 this.clean_display();
43 + this._packagesets = {};

It would be helpful to say here what structure _packagesets will have, e.g. that it maps distroseries ids to arrays of packageset ids. We minify our JS, so comments won't slow the code down but they can save oodles of time in maintenance.

48 === modified file 'lib/lp/registry/javascript/tests/test_distroseries.js'
49 --- lib/lp/registry/javascript/tests/test_distroseries.js 2011-06-01 09:17:43 +0000
50 +++ lib/lp/registry/javascript/tests/test_distroseries.js 2011-06-07 21:48:32 +0000
51 @@ -854,6 +854,48 @@
52 attrselect("value")(this.widget.get("choices")));
53 },
54
55 + testRemoveDistroSer...

review: Needs Fixing (js)

[Raphaël Badin (rvb)]

Hi Jeroen,

Sorry about the fact that this code is not really readable. I've tried to improve it quite a bit.

> What is distroseries exactly, and more to the point, what is
> distroseries.value? It would be helpful to have some documentation of the
> parameters. Is distroseries.value a distroseries id?

Added documentation.

> The new code looks like it may be slower. Could that be an issue?

I don't think so, it'll just iterate over the packagesets of a distroseries.

> As far as I can tell, that's a CSS3 selector for "value ends with a dash
> followed by distroseries_id." Is it safe for us to use CSS3 selectors?
> AFAICT CSS2 does not have $=.
>
> The new code looks for:
>
> 'option[value$="' + packageset.get("id") + '"]'
>
> That means "value ends with the packageset id." I may be misunderstanding it,
> but it looks wrong to me in two ways:
>
> 1. So what does that value end in — distroseries id or packageset id!?
>
> 2. What about spurious substring matches, e.g. won't this also remove "321"
> when you really want to remove only "21"?

Ah! Thanks for spotting this! I've changed it to:

{{{
this.fieldNode.one(
    'option[value="' + packageset.get("id") + '"]').remove();
}}}

Just so you know, before this change, the packageset was identified by packagesetname-distro_id. Hence left-over/buggy the substring matches ... I guess staring at javascript too much made me dizzy.

> It would be helpful to say here what structure _packagesets will have, e.g.
> that it maps distroseries ids to arrays of packageset ids. We minify our JS,
> so comments won't slow the code down but they can save oodles of time in
> maintenance.

Good point, I've added a small explanation for this._packagesets.

> At 40 lines this test is far too long for me to see what it does without any
> comments or helpful identifiers. Is there anything you can do to express what
> this test tests?

I've refactored the test to make it more clear ... I hope. But I'm afraid js is kinda verbose :(.

> The substr('-5') is highly opaque. If there is no clearer way to write this
> (I can't guess because I have no indication what it's for) then can you
> isolate it in a function with a helpful name, or comment it?
> [...]
> Except that "name" may not actually be the distroseries name, because it's
> just the last 5 characters… in which case, why not use the full "ubuntu/hoary"
> and "ubuntu/breezy" here and drop the confusing "substr('-5')" trick?

Again, very good point, I refactored the code.

> Are the ids 4 and 5 here in any way related to the ids 4 and 5 you use above?
> If you're looking to check for possible confusion between the two, then that's
> a good thing to put in a separate test. Don't lob multiple aspects into a
> single test because they can grow, grow, grow without bound.

I've changed the ids to make it more clean. It was not my intention to test for that (and I don't think it's worth adding a test for this).

> I guess these are packageset ids. But the only thing that really tells me
> that is the "6"! It's hard to come up with anything that would make this
> clearer. Maybe you could wrap the widget.get("choices") in a function
> get_packageset_cho...

[Jeroen T. Vermeulen (jtv)]

Thanks for making these changes. Much better now!

One thing that still needs fixing, as mentioned on IRC: in the test, the two distroseries have the same id. That happens to produce the right results here, but it does upset the bookkeeping a little bit.

The ordering of the ids in the test comparison seems a little arbitrary (5, 7, 6, 4 IIRC). If possible, use ordering-insensitive comparisons where ordering is not important and test ordering separately. Less brittle, easier to comprehend.

But now I'm nit-picking. Well done!

Jeroen

review: Approve (js)

[Raphaël Badin (rvb)]

> One thing that still needs fixing, as mentioned on IRC: in the test, the two
> distroseries have the same id. That happens to produce the right results
> here, but it does upset the bookkeeping a little bit.

Done.

> The ordering of the ids in the test comparison seems a little arbitrary (5, 7,
> 6, 4 IIRC). If possible, use ordering-insensitive comparisons where ordering
> is not important and test ordering separately. Less brittle, easier to
> comprehend.

Done.

> But now I'm nit-picking. Well done!

Thanks again for the review, I reckon the code has improved quite a bit thanks to you remarks!

[Jeroen T. Vermeulen (jtv)]

JS, or code, or whatever. It's all good.

review: Approve

=== modified file 'lib/lp/registry/javascript/distroseries.js'
--- lib/lp/registry/javascript/distroseries.js	2011-06-06 08:51:09 +0000
+++ lib/lp/registry/javascript/distroseries.js	2011-06-09 09:14:43 +0000
@@ -702,6 +702,8 @@
      * distroseries to the possible choices.
      *
      * @method add_distroseries
+     * @param {Object} The distroseries to add ({value:distroseries_id),
+     *     api_uri:distroseries_uri}).
      */
     add_distroseries: function(distroseries) {
         var path = distroseries.api_uri + "/architectures";
@@ -757,6 +759,8 @@
      * Add a list of distroarchseries.
      *
      * @method add_distroarchseries
+     * @param {String} distroseries_id The distroarchseries id.
+     * @param {Object} distroarchseries The distroarchseries object.
      */
     add_distroarchseries: function(distroseries_id, distroarchseries) {
         this._distroseries[distroseries_id] = distroarchseries;
@@ -990,6 +994,7 @@
      * Add a distroseries: add its packagesets to the packageset picker.
      *
      * @method add_distroseries
+     * @param {Object} distroseries The distroseries object.
      */
     add_distroseries: function(distroseries) {
         var distro_series_uri = Y.lp.client.get_absolute_uri(
@@ -1045,13 +1050,15 @@
      * Add a choice to the picker.
      *
      * @method add_choice
+     * @param {Object} choice The choice object to be added
+     *     ({text: choice_text, value: choice_value, data: choice_data}).
      */
     add_choice: function(choice) {
         var select = this.init_select();
         var option = Y.Node.create("<option />");
         option.set("value", choice.value)
-                .set("text", choice.text)
-                .setData("data", choice.data);
+            .set("text", choice.text)
+            .setData("data", choice.data);
         var options = select.all('option');
         if (options.isEmpty()) {
             select.append(option);
@@ -1071,8 +1078,13 @@
      * Add choices (a set of packagesets) to the picker.
      *
      * @method add_packagesets
+     * @param {Y.lp.client.Collection} packagesets The collection of
+     *     packagesets to add.
+     * @param {Object} distroseries The distroseries object
+     *     ({value:distroseries_id), api_uri:distroseries_uri}).
      */
     add_packagesets: function(packagesets, distroseries) {
+        this._packagesets[distroseries.value] = packagesets.entries;
         packagesets.entries.forEach(
             function(packageset) {
                 var value = packageset.get("id");
@@ -1093,11 +1105,16 @@
      * Remove a distroseries: remove its packagesets from the picker.
      *
      * @method remove_distroseries
+     * @param {String} distroseries_id The id of the distroseries to be
+     *     removed.
      */
     remove_distroseries: function(distroseries_id) {
-        this.fieldNode.all(
-            'option[value$="-' + distroseries_id + '"]').remove();
-        Y.lazr.anim.green_flash({node: this.fieldNode}).run();
+        this._packagesets[distroseries_id].forEach(
+            function(packageset) {
+                this.fieldNode.one(
+                    'option[value="' + packageset.get("id") + '"]').remove();
+            }, this);
+       Y.lazr.anim.green_flash({node: this.fieldNode}).run();
         if (this.fieldNode.all('option').isEmpty()) {
             this.clean_display();
         }
@@ -1109,6 +1126,12 @@
         this.error_handler.clearProgressUI = Y.bind(this.hideSpinner, this);
         this.error_handler.showError = Y.bind(this.showError, this);
         this.clean_display();
+        // _packagesets maps each distroseries' id to a collection of ids 
+        // of its packagesets.
+        // It's populated each time a new distroseries is added as a parent
+        // and used when a distroseries is removed to get all the
+        // corresponding packagesets to be removed from the widget.
+        this._packagesets = {};
     }
 
 });

=== modified file 'lib/lp/registry/javascript/tests/test_distroseries.js'
--- lib/lp/registry/javascript/tests/test_distroseries.js	2011-06-01 09:17:43 +0000
+++ lib/lp/registry/javascript/tests/test_distroseries.js	2011-06-09 09:14:43 +0000
@@ -854,6 +854,65 @@
                 attrselect("value")(this.widget.get("choices")));
         },
 
+        testRemoveDistroSeriesUpdatesPackageSets: function() {
+            // Calling remove_distroseries removes the packagesets
+            // related to the distroseries from the widget.
+
+            // Setup a first distroseries with a bunch of packagesets.
+            var distroseries1 = {
+                value: "1", title: "series1",
+                api_uri: "ubuntu/hoary"};
+            var package_sets1 = [
+                {id: "4", name: "aa", description: "Aa"},
+                {id: "5", name: "bb", description: "Bb"}
+            ];
+            var package_sets_collection1 =
+                new Y.lp.client.Collection(
+                    null, {entries: package_sets1}, null);
+
+            // Setup a second distroseries with other packagesets.
+            var distroseries2 = {
+                value: "2", title: "series2",
+                api_uri: "ubuntu/breezy"};
+            var package_sets2 = [
+                {id: "6", name: "cc", description: "Cc"},
+                {id: "7", name: "dd", description: "Dd"}
+            ];
+            var package_sets_collection2 =
+                new Y.lp.client.Collection(
+                    null, {entries: package_sets2}, null);
+
+            // Setup the client so that the proper packagesets are returned
+            // for each distroseries.
+            var package_set_collections = {
+                'hoary': package_sets_collection1,
+                'breezy': package_sets_collection2
+            };
+            this.widget.client = {
+                named_get: function(path, operation, config) {
+                    var series_name = config
+                        .parameters.distroseries.split('/')[6];
+                    config.on.success(package_set_collections[series_name]);
+                }
+            };
+
+            // Add the two series.
+            this.widget.add_distroseries(distroseries1);
+            this.widget.add_distroseries(distroseries2);
+            // The packagesets widget has been populated with the
+            // packagesets from the series.
+            ArrayAssert.itemsAreEqual(
+                ["4", "5", "6", "7"],
+                attrselect("value")(this.widget.get("choices")));
+
+            // Remove a series.
+            this.widget.remove_distroseries("2");
+            // The remaining packagesets are those from the remaining series.
+            ArrayAssert.itemsAreEqual(
+                ["4", "5"],
+                attrselect("value")(this.widget.get("choices")));
+        },
+
         testSetDistroSeriesSpinner: function() {
             var widget = this.widget;
             widget.client = {