Code review comment for lp:~rvb/launchpad/init-multiple-parents-packagesetpicker

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 + testRemoveDistroSeriesUpdatesPackageSets: function() {
56 + var package_sets1 = [
57 + {id: "4", name: "foo", description: "Foo"},
58 + {id: "5", name: "bar", description: "Bar"}
59 + ];
60 + var package_sets2 = [
61 + {id: "6", name: "baz", description: "Baz"}
62 + ];
63 + var package_sets_collection1 =
64 + new Y.lp.client.Collection(
65 + null, {entries: package_sets1}, null);
66 + var package_sets_collection2 =
67 + new Y.lp.client.Collection(
68 + null, {entries: package_sets2}, null);

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?

69 + this.widget.client = {
70 + named_get: function(path, operation, config) {
71 + var name = config.parameters.distroseries.substr('-5');

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?

72 + if (name === 'hoary') {
73 + config.on.success(package_sets_collection1);
74 + }
75 + else {
76 + config.on.success(package_sets_collection2);
77 + }

What makes hoary special here? I think you're setting up two distroseries later one of which is hoary, but that's not clear here. If there is meant to be a tie between the two package set collections and the two distroseries, then maybe it's worth using "hoary" and "breezy" in the variable names rather than 1 and 2.

If I'm right in my guesses, I think this bit of code would be written more clearly as:

    var package_set_collections = {
        'hoary': package_sets_collection1,
        'breezy': package_sets_collection2
    };
    config.on.success(package_set_collections[name]);

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?

80 + var distroseries1 = {
81 + id:'4', value: "4", title: "series1",
82 + api_uri: "ubuntu/hoary"};
83 + var distroseries2 = {
84 + id:'5', value: "5", title: "series2",
85 + api_uri: "ubuntu/breezy"};

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.

86 + this.widget.add_distroseries(distroseries1);
87 + this.widget.add_distroseries(distroseries2);
88 + ArrayAssert.itemsAreEqual(
89 + ["5", "6", "4"],
90 + attrselect("value")(this.widget.get("choices")));

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_choices(widget)?

91 + this.widget.remove_distroseries("5");
92 + ArrayAssert.itemsAreEqual(
93 + ["5", "4"],
94 + attrselect("value")(this.widget.get("choices")));
95 + },

Jeroen