= Approving =

This looks like Pretty Good Stuff now.  The commit logs show lots of changes (and actually explain them, which is a nice touch).

I see just 2 things that need fixing, but I'm sure that won't be helped by any further nannying from me.  See "Important" below.  Some other notes that you may want to consider or improve at your option: nice to have but not worth further delay.


== Design ==

I see a few documented assumptions (very thoughtful, by the way!) about Ubuntu being involved.  Does that fit with the ongoing work for supporting derived distributions?  Probably depends on the requirements for derived distros.

If not, it may present some new development tasks for derived distros.  I wouldn't hold up your branch for it unless it poses some kind of serious problem.


== Important ==

=== Jack-in-the-box file ===

According to the MP diff, you're _adding_ lib/lp/services/features/scopes.py.  Which already exists, with contents apparently similar or identical to what's in the diff.  How can that happen!?


=== Model/view boundaries ===

In lib/lp/bugs/browser/bugtracker.py, you query the database directly:

188	+    def updateContextFromData(self, data, context=None):
[...]
208	+        # Has this source package already been assigned to a component?
209	+        pkgs = Store.of(component).find(
210	+            BugTrackerComponent,
211	+            BugTrackerComponent.distribution == distribution.id,
212	+            BugTrackerComponent.source_package_name ==
213	+                pkg.sourcepackagename.id)
214	+        if pkgs.count() > 0:
215	+            self.request.response.addNotification(
216	+                "The %s source package is already linked to %s in %s" % (
217	+                    sourcepackagename, component_full_name, distro_name))
218	+            return

Querying the database from browser code is frowned upon: it's a job for the model, not for the view.

This particular query does a job that can be concisely defined, so that's good.  (In fact some say that a block of code like this with a defining comment on top should almost certainly be a method of its own).  I didn't find any method that does exactly this, but there's no reason why there shouldn't be.  I think SourcePackage would be a good place.

(Also note that a count() on the query can be relatively expensive and deliver more information than you need.  If all you need to know is whether there are any at all, use is_empty() instead.)


== Style ==

In lib/lp/bugs/browser/bugalsoaffects.py:

62	+        # Look up the remote component if we have the necessary info
63	+        #if data.get('add_packaging', False):
64	+        comp_group = target.bugtracker.getRemoteComponentGroup(
65	+            target.remote_product)

There's still a commented-out line in there.

In lib/lp/bugs/browser/tests/test_bugtracker_component.py: setUp() still creates a few objects that most of the tests don't actually care about.

Also in lib/lp/bugs/browser/tests/test_bugtracker_component.py, test_no_anonymous_edits:

353	+        expected_html = """
354	+  <h2>Components</h2>
355	+  <ul>
356	+    <li>
357	+      <strong>alpha</strong>
358	+      <ul>
359	+        <li>
360	+          Example
361	+        </li>
362	+      </ul>
363	+    </li>
364	+"""

Breaking indentation like this is really ugly.  If the assertTextMatchesExpressionIgnoreWhitespace can't deal with the indentation for whatever reason, try writing it like this:

    expected_html = dedent("""\
        <h2>Components</h2>

Same file, in test_generated_links:

395	+        #self.assertIsNot(None, find_tag_by_id(actual_html, 'portlet-components'))

Commented-out code is a bad thing.  But there seems to be more going on: there's a similar line (though not commented out) in test_no_anonymous_edits.  It seems to be testing for something in its own right.  This could be a holdover from when our tests would go through actions and then prove things about the effects, rather than an individual test proving one fact about the code's behaviour as we like to do now.

I find it very helpful to name tests after those facts rather than after the scenario: "test_edit_form_links_to_source_package" instead of "test_linking."  That also helps me identify what's important and what is circumstantial, what changes should break the test and which ones it should overlook, or whether I should fix the test or the code when the test fails.

In lib/lp/bugs/interfaces/bugtracker.py, getBugFilingAndSearchLinks:

618	+        :param remote_component: The name of the component on which the bug
619	+            is to be filed or search for.

Typo for "searched"?

Finally, a question: in lib/lp/bugs/templates/bugtracker-edit-component.pt I notice that the root tag is bug-tracker-edit-component.  Can we do that!?  My TAL knowledge is mostly cribbed off existing templates, but I thought that for whatever reason we only make up our own tags in the tal/metal namespaces.


Jeroen