Hi Bryce,

There's lots of stuff that I'm unfamiliar with here, so please forgive me for holding you up with a "Needs Information" vote.  I have some big questions about the UI and test coverage.

Otherwise it would be Approved, with some smaller notes and superficial issues.


> === modified file 'lib/canonical/launchpad/scripts/bzremotecomponentfinder.py'
> --- lib/canonical/launchpad/scripts/bzremotecomponentfinder.py	2010-10-20 00:24:50 +0000
> +++ lib/canonical/launchpad/scripts/bzremotecomponentfinder.py	2010-11-16 19:54:52 +0000
> @@ -117,7 +117,6 @@
>          self.static_bugzilla_text = static_bugzilla_text
>  
>      def getRemoteProductsAndComponents(self, bugtracker_name=None):
> -        """"""

There's supposed to be a docstring.  It looks as if this one was put in
as a placeholder: "I'll write one later, and if I forget, the reviewer
will spot it."  Can you think of what they mant to write?

The fact that this got into the codebase may be a clue that we're
being too sloppy in our reviews.


> === modified file 'lib/canonical/widgets/bugtask.py'
> --- lib/canonical/widgets/bugtask.py	2010-11-08 12:52:43 +0000
> +++ lib/canonical/widgets/bugtask.py	2010-11-16 19:54:52 +0000
> @@ -495,6 +495,25 @@
>          return distribution
>  
>  
> +class UbuntuSourcePackageNameWidget(
> +    BugTaskSourcePackageNameWidget):

Why is this line broken?  There's plenty of space.  Did you rename this
from a longer original name?


> +    """Package widget where the distribution can be assumed as Ubuntu

There should be a full stop at the end of the sentence.


> +    distribution_name = "ubuntu"
> +
> +    def getDistribution(self):
> +        """See `BugTaskSourcePackageNameWidget`"""
> +        distribution = getUtility(IDistributionSet).getByName(
> +            self.distribution_name)
> +        if distribution is None:
> +            raise UnexpectedFormData(
> +                "No such distribution: %s" % self.distribution_name)
> +        return distribution

Would it be worth having a comment here to say why you have to override
this method?

Also, doesn't this method amount to a difficult spelling for

    return getUtility(ILaunchpadCelebrities).ubuntu

?


> === modified file 'lib/lp/bugs/browser/bugalsoaffects.py'
> --- lib/lp/bugs/browser/bugalsoaffects.py	2010-09-03 03:12:39 +0000
> +++ lib/lp/bugs/browser/bugalsoaffects.py	2010-11-16 19:54:52 +0000
> @@ -664,8 +664,20 @@
>              title = bug.title
>              description = u"Originally reported at:\n  %s\n\n%s" % (
>                  canonical_url(bug), bug.description)
> +            remote_component = ""
> +            comp_group = target.bugtracker.getRemoteComponentGroup(
> +                target.remote_product)
> +
> +            # Look up the remote component if we have the necessary info
> +            if comp_group is not None and data.get('add_packaging', False):
> +                package_name = self.context.target.sourcepackagename
> +                for component in comp_group.components:
> +                    if (component.distro_source_package is not None and
> +                        component.distro_source_package.name == package_name):
> +                        remote_component = component.name
> +
>              return target.bugtracker.getBugFilingAndSearchLinks(
> -                target.remote_product, title, description)
> +                target.remote_product, remote_component, title, description)

It may be just because I'm seeing this as a diff without Python syntax
highlighting, but I find this a bit hard to follow.

Some suggestions:

 * Almost all of the method is now an "else" clause after an early
   return.  It may be worth removing the "else:" and making the code
   stand on its own.

 * Would the part you added here make sense as a separate, private
   method?

 * Line-breaking "if" conditions is nasty in that there's no visual
   separation between the continuation of the condition and the
   code in the body.  If all else fails, insert a comment to mark the
   transition.  But in this case, you can fix it with a variable:

                for component in comp_group.components:
                    package = component.distro_source_package
                    if package is not None and package.name == package_name:
                        remote_component = component.name

That doesn't cost you any lines yet makes the "if" easier to read.


> === modified file 'lib/lp/bugs/browser/bugtracker.py'
> --- lib/lp/bugs/browser/bugtracker.py	2010-10-15 08:23:19 +0000
> +++ lib/lp/bugs/browser/bugtracker.py	2010-11-16 19:54:52 +0000

> @@ -67,13 +69,19 @@
>      DelimitedListWidget,
>      LaunchpadRadioWidget,
>      )
> +from canonical.widgets.bugtask import (
> +    UbuntuSourcePackageNameWidget,
> +    )

Not that it matters much, but this again would fit on a single line.


> @@ -229,6 +237,14 @@
>          return shortlist(chain(self.context.projects,
>                                 self.context.products), 100)
>  
> +    @property
> +    def related_component_groups(self):
> +        """Return all component groups and components

There's supposed to be a full stop at the end of the sentence.


> @@ -447,16 +463,113 @@
>              return RemoteBug(self.context, remotebug, bugs)
>  
>      @stepthrough("+components")
> -    def component_groups(self, name):
> -        return self.context.getRemoteComponentGroup(name)
> +    def component_groups(self, id):
> +        # Navigate by id (component group name should work too)
> +        return self.context.getRemoteComponentGroup(id)

That comment really ought to be a docstring, since it documents
interface rather than implementation.  Also, we idiomatically use "id"
for an object's numeric identifier (as required by SQLObject back when
that was our ORM).  If this can be a name or numeric identifier, perhaps
it should be called name_or_id to reflect that?


> +class BugTrackerEditComponentView(LaunchpadEditFormView):
> +    """Provides editing form for setting source packages for components.
> +
> +    In this class we assume that bug tracker components are always
> +    linked to source packages in the Ubuntu distribution.
> +    """

Suggestion: "this class assumes" would be more direct.


> +
> +    schema = IBugTrackerComponent
> +    custom_widget('sourcepackagename',
> +                  UbuntuSourcePackageNameWidget)

This again would fit on one line.  Also, our coding standards do not
allow this indentation style.  When line-breaking an arguments list in
a function or method call, end the line right after the opening
parenthesis and indent by exactly 4 columns:

    custom_widget(
        'sourcepackagename', UbuntuSourcePackageNameWidget)

or

    custom_widget(
        'sourcepackagename',
        UbuntuSourcePackageNameWidget)

or in this case even

    custom_widget('sourcepackagename', UbuntuSourcePackageNameWidget)


> +    @property
> +    def field_names(self):
> +        field_names = [
> +            'sourcepackagename',
> +            ]
> +        return field_names

Wouldn't it be more convenient to use the single-line list format here?

        return ['sourcepackagename']

Of course if you're going to add more field names then that won't help.


> +    def setUpWidgets(self, context=None):
> +        for field in self.form_fields:
> +            if (field.custom_widget is None and
> +                field.__name__ in self.custom_widgets):
> +                field.custom_widget = self.custom_widgets[field.__name__]

Can you find a way to maintain the visual separation between the if's
condition and its body?


> +    @property
> +    def initial_values(self):
> +        """See `LaunchpadFormView.`"""
> +        field_values = {}
> +        for name in self.field_names:
> +            if name == 'sourcepackagename':
> +                pkg = self.context.distro_source_package
> +                if pkg is not None:
> +                    field_values['sourcepackagename'] = pkg.name
> +                else:
> +                    field_values['sourcepackagename'] = ""
> +            else:
> +                field_values[name] = getattr(self.context, name)
> +
> +        return field_values

Just a suggestion, and maybe not even a good one, but would this be clearer?

        field_to_attr_names = {
            'sourcepackagename': 'distro_source_package',
        }
        field_values = {}
        for field_name in self.field_names:
            attr_name = field_to_attr_names.get(field_name, field_name)
            field_values[field_name] = getattr(self.context, attr_name, "")

Of course that won't raise an exception if an attribute other than
distro_source_package is None.  So maybe that's not what you want.


> +    def updateContextFromData(self, data, context=None):
> +        """Link component to specified distro source package.
> +
> +        Get the user-provided source package name from the form widget,
> +        look it up in Ubuntu to retrieve the distro_source_package
> +        object, and link it to this component.
> +        """
> +        if context is None:
> +            context = self.context
> +        component = context
> +
> +        sourcepackagename = self.request.form.get(
> +            self.widgets['sourcepackagename'].name)
> +
> +        distro_name = self.widgets['sourcepackagename'].distribution_name
> +        distribution = getUtility(IDistributionSet).getByName(distro_name)
> +        pkg = distribution.getSourcePackage(sourcepackagename)
> +
> +        # Has this source package already been assigned to a component?
> +        pkgs = Store.of(component).find(
> +            BugTrackerComponent,
> +            BugTrackerComponent.distribution == distribution.id,
> +            BugTrackerComponent.source_package_name ==
> +            pkg.sourcepackagename.id)

That last line needs an extra indent.  It continues the "==" rather
than the arguments list.


> +        if pkgs.count()>0:

There should be whitespace around the ">" operator.

Running "make lint" should warn you about this, but it no longer seems
to detect changed files once you've committed.  :-(


> +            self.request.response.addNotification(
> +                "The %s source package is already linked to %s:%s in %s" %(
> +                    sourcepackagename, component.component_group.name,
> +                    component.name, distro_name))

There's a space missing in "%(" at the end of the line.

Why doesn't Component have a method or property to construct its full
name?  Even without it, constructing the component's description in a
variable would probably make this paragraph easier to follow.


> +            return
> +
> +        component.distro_source_package = pkg
> +        self.request.response.addNotification(
> +            "%s:%s is now linked to the %s source package in %s" %(
> +                component.component_group.name, component.name,
> +                sourcepackagename, distro_name))

This uses the same component description again.


> === modified file 'lib/lp/bugs/browser/tests/bugtask-adding-views.txt'
> --- lib/lp/bugs/browser/tests/bugtask-adding-views.txt	2010-11-01 14:58:34 +0000
> +++ lib/lp/bugs/browser/tests/bugtask-adding-views.txt	2010-11-16 19:54:52 +0000
> @@ -615,7 +615,7 @@
>  
>      >>> print_links(add_task_view.upstream_bugtracker_links)
>      bug_filing_url:
> -    ...?product=foo&short_desc=Reflow%20...&long_desc=Originally%20...
> +    ...?product=foo...&short_desc=Reflow%20...&long_desc=Originally%20...
>      bug_search_url: ...query.cgi?product=foo&short_desc=Reflow%20problems...
>  
>  If the product's `bugtracker` isn't specified its
> 

Ah, the joy of doctest maintenance!


> === added file 'lib/lp/bugs/browser/tests/test_bugtracker_component.py'
> --- lib/lp/bugs/browser/tests/test_bugtracker_component.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/bugs/browser/tests/test_bugtracker_component.py	2010-11-16 19:54:52 +0000
> @@ -0,0 +1,116 @@
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version (see the file LICENSE).
> +
> +"""Unit tests for linking bug tracker components to source packages."""
> +
> +__metaclass__ = type
> +
> +import unittest
> +from zope.component import getUtility
> +
> +from canonical.testing.layers import DatabaseFunctionalLayer
> +from lp.registry.interfaces.distribution import IDistributionSet
> +from lp.testing import (
> +    login_person,
> +    TestCaseWithFactory,
> +    )
> +from lp.testing.views import create_initialized_view
> +
> +
> +class TestBugTrackerEditComponentView(TestCaseWithFactory):
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        super(TestBugTrackerEditComponentView, self).setUp()
> +        regular_user = self.factory.makePerson()
> +        login_person(regular_user)
> +
> +        self.bug_tracker = self.factory.makeBugTracker()
> +        self.comp_group = self.factory.makeBugTrackerComponentGroup(
> +            u'alpha', self.bug_tracker)
> +
> +    def _makeForm(self, sourcepackage):
> +        if sourcepackage is None:
> +            name = ''
> +        else:
> +            name = sourcepackage.name
> +        return {
> +            'field.sourcepackagename': name,
> +            'field.actions.save': 'Save',
> +            }
> +
> +    def test_view_attributes(self):
> +        component = self.factory.makeBugTrackerComponent(
> +            u'Example', self.comp_group)
> +        distro = getUtility(IDistributionSet).getByName('ubuntu')
> +        package = self.factory.makeDistributionSourcePackage(
> +            sourcepackagename='example', distribution=distro)
> +        form = self._makeForm(package)
> +        view = create_initialized_view(
> +            component, name='+edit', form=form)
> +        label = 'Link a distribution source package to the Example component'
> +        self.assertEqual(label, view.page_title)
> +        fields = ['sourcepackagename']
> +        self.assertEqual(fields, view.field_names)

It's a shame to see setup start to crowd out the verification.  There
may be some small ways to fight that, such as a helper for "create an
Ubuntu distro source package of a given name."

> +
> +    def test_linking(self):
> +        component = self.factory.makeBugTrackerComponent(
> +            u'Example', self.comp_group)
> +        distro = getUtility(IDistributionSet).getByName('ubuntu')
> +        package = self.factory.makeDistributionSourcePackage(
> +            sourcepackagename='example', distribution=distro)
> +
> +        self.assertIs(None, component.distro_source_package)
> +        form = self._makeForm(package)
> +        view = create_initialized_view(
> +            component, name='+edit', form=form)
> +        self.assertEqual([], view.errors)
> +
> +        notifications = view.request.response.notifications
> +        #self.assertEqual(1, len(notifications))

Dead code!


> +        self.assertEqual(component.distro_source_package, package)

I think "package" is the expected value here, in which case it should be
the first argument.

Also, I wonder if this test isn't trying to do too much.  On the one
hand it's a story test: it goes through the linking process and checks
for output.  On the other it verifies the result itself.

Would it make sense to separate those aspects?  Or maybe the absence of
errors should be taken as read when the operation succeeds?


> +        expected = (
> +            u"alpha:Example is now linked to the example "
> +            "source package in ubuntu")
> +        self.assertEqual(expected, notifications.pop().message)
> +
> +    def test_cannot_doublelink_sourcepackages(self):
> +        # Two components try linking to same package

That is what users do.  It would be useful to state how the view should
behave in that situation.

Also, the system would describe the situation differently from the user:
someone tries to link a component to a package that is already linked to
another package.  How users got themselves into that situation is
a different matter.


> +        component_a = self.factory.makeBugTrackerComponent(
> +            u'a', self.comp_group)
> +        component_b = self.factory.makeBugTrackerComponent(
> +            u'b', self.comp_group)
> +        distro = getUtility(IDistributionSet).getByName('ubuntu')
> +        package = self.factory.makeDistributionSourcePackage(
> +            sourcepackagename='example', distribution=distro)
> +
> +        form = self._makeForm(package)
> +        view = create_initialized_view(
> +            component_a, name='+edit', form=form)
> +        notifications = view.request.response.notifications
> +        self.assertEqual([], view.errors)
> +        self.assertEqual(1, len(notifications))
> +        self.assertEqual(package, component_a.distro_source_package)

This is part of the "two components try linking to the same package"
user scenario.  But is it really relevant to the test how users got
themselves into this situation?

Moreover, is there any need to repeat the verification of output from
the first attempt?  That just repeats something you already tested.

I'd treat this part as setup and make the change directly in the
database.  Test a single thing--how the view reacts to someone trying to
link a package that's already linked.


> +
> +        form = self._makeForm(package)
> +        view = create_initialized_view(
> +            component_b, name='+edit', form=form)
> +        self.assertIs(None, component_b.distro_source_package)
> +        self.assertEqual([], view.errors)
> +        notifications = view.request.response.notifications
> +        self.assertEqual(1, len(notifications))
> +        expected = (
> +            u"""The example source package is already linked to """
> +            """alpha:b in ubuntu""")
> +        self.assertEqual(expected, notifications.pop().message)

You could use self.assertTextMatchesExpressionIgnoreWhitespace for the
verification, and use a convenient multi-line string for "expected."

It's great to see unit tests replace doctests for testing the UI.  But
does this test any of the links that you generate in the TAL?


> +
> +
> +def test_suite():
> +    suite = unittest.TestSuite()
> +    suite.addTest(unittest.TestLoader().loadTestsFromName(__name__))
> +
> +    return suite
> +
> +if __name__ == '__main__':
> +    unittest.TextTestRunner().run(test_suite())

The test_suite boilerplate isn't normally needed any more.  Why do we
need this test to be able to run stand-alone?


> === modified file 'lib/lp/bugs/doc/bugtracker.txt'
> --- lib/lp/bugs/doc/bugtracker.txt	2010-10-27 20:00:54 +0000
> +++ lib/lp/bugs/doc/bugtracker.txt	2010-11-16 19:54:52 +0000
> @@ -356,26 +356,26 @@
>  Filing a bug on the remote tracker
>  ----------------------------------
>  
> -The IBugTracker interface defines a method, getBugFilingAndSearchLinks(),
> -which returns the URLs of the bug filing form and the bug search on
> -the remote bug tracker as a dict. It accepts three parameters:
> -remote_product, which is the name of the product on the remote
> -tracker; summary, which is the bug summary to be passed to the remote
> -bug tracker for searching or filing and description, which is the full
> -description of the bug. This is only passed to the bug filing form as
> -it is too specific for the search form.
> +The IBugTracker interface defines a method,
> +getBugFilingAndSearchLinks(), which returns the URLs of the bug filing
> +form and the bug search on the remote bug tracker as a dict. It accepts
> +fource parameters: remote_product, which is the name of the product on

Typo: s/fource/four/

But is there really any point to having this much detail in the doctest?
Much of it is obvious and belongs in the docstring.  Repeating it here
may only drive the reader away.


> +the remote tracker; remote_component, which corresponds to the component
> +field; summary, which is the bug summary to be passed to the remote bug
> +tracker for searching or filing and description, which is the full
> +description of the bug. This is only passed to the bug filing form as it
> +is too specific for the search form.
>  
>      >>> def print_links(links_dict):
>      ...     for key in sorted(links_dict):
>      ...         print "%s: %s" % (key, links_dict[key])
>  
>      >>> links = mozilla_bugzilla.getBugFilingAndSearchLinks(
> -    ...     remote_product='testproduct', summary="Foo", description="Bar")
> +    ...     remote_product='testproduct', remote_component='testcomponent',
> +    ...     summary="Foo", description="Bar")
>      >>> print_links(links)
> -    bug_filing_url:
> -    https://.../enter_bug.cgi?product=testproduct&short_desc=Foo&long_desc=Bar
> -    bug_search_url:
> -    https://.../query.cgi?product=testproduct&short_desc=Foo
> +    bug_filing_url: https://.../enter_bug.cgi?product=testproduct&component=testcomponent&short_desc=Foo&long_desc=Bar
> +    bug_search_url: https://.../query.cgi?product=testproduct&short_desc=Foo

Please keep the lines under 78 columns; "make lint" will probably
complain if you don't.


> === modified file 'lib/lp/bugs/interfaces/bugtracker.py'
> --- lib/lp/bugs/interfaces/bugtracker.py	2010-11-04 02:32:16 +0000
> +++ lib/lp/bugs/interfaces/bugtracker.py	2010-11-16 19:54:52 +0000
> @@ -306,12 +306,14 @@
>      watches_needing_update = Attribute(
>          "The set of bug watches that need updating.")
>  
> -    def getBugFilingAndSearchLinks(remote_product, summary=None,
> -                                   description=None):
> +    def getBugFilingAndSearchLinks(remote_product, remote_component=None,
> +                                   summary=None, description=None):
>          """Return the bug filing and search links for the tracker.
>  
>          :param remote_product: The name of the product on which the bug
>              is to be filed or search for.
> +        :param remote_product: The name of the component on which the bug
> +            is to be filed or search for.

I think it's meant to be :param remote_component: that second time!


> === modified file 'lib/lp/bugs/model/bugtracker.py'
> --- lib/lp/bugs/model/bugtracker.py	2010-11-04 02:32:16 +0000
> +++ lib/lp/bugs/model/bugtracker.py	2010-11-16 19:54:52 +0000
> @@ -266,18 +265,23 @@
>          None is returned if there is no component by that name in the
>          group.
>          """
> -
>          if component_name is None:
>              return None
> +        elif component_name.isdigit():
> +            component_id = int(component_name)

Is this really the right thing to do in model code?  If you want to
accept "component name or numerical id" from the user in the web UI,
isn't the browser code the right place to turn that into either a name
or an integer depending?

Also, I'm surprised to see this interface change not affect the
docstring.


> @@ -673,9 +685,17 @@
>          """See `IBugTracker`."""
>          component_group = None
>          store = IStore(BugTrackerComponentGroup)
> -        component_group = store.find(
> -            BugTrackerComponentGroup,
> -            name = component_group_name).one()
> +        if component_group_name is None:
> +            return None
> +        elif component_group_name.isdigit():
> +            component_group_id = int(component_group_name)
> +            component_group = store.find(
> +                BugTrackerComponentGroup,
> +                id = component_group_id).one()

I'm not familiar with this way of calling find.  I always use
Foo.id == value.  This looks more convenient.

One thing though: there should be no whitespace around the "=" for a
parameter initializer.


> +        else:
> +            component_group = store.find(
> +                BugTrackerComponentGroup,
> +                name = component_group_name).one()

(Same here, of course.)


> === added file 'lib/lp/bugs/templates/bugtracker-portlet-components.pt'
> --- lib/lp/bugs/templates/bugtracker-portlet-components.pt	1970-01-01 00:00:00 +0000
> +++ lib/lp/bugs/templates/bugtracker-portlet-components.pt	2010-11-16 19:54:52 +0000
> @@ -0,0 +1,37 @@
> +<div
> +    xmlns:tal="http://xml.zope.org/namespaces/tal"
> +    xmlns:metal="http://xml.zope.org/namespaces/metal"
> +    xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> +    class="portlet" id="portlet-components">
> +  <h2>Components</h2>
> +  <p>
> +    You can link components from this bug tracker to their corresponding
> +    distribution source packages in the project's &ldquo;Change
> +    components&rdquo; page.
> +  </p>
> +  <ul tal:define="related_component_groups view/related_component_groups">
> +    <li tal:repeat="component_group related_component_groups">
> +      <strong><span tal:replace="component_group/name" /></strong>
> +      <ul tal:define="components component_group/components">
> +        <li tal:repeat="component components">
> +          <span tal:replace="component/name" />
> +          &nbsp;
> +          <span tal:condition="component/distro_source_package">
> +            <a class="sprite edit"
> +               tal:attributes="href string:${context/fmt:url}/+components/${component_group/id}/${component/id}/+edit">

This looks as if we want a URL formatter for Component.  Don't we have one?

Also, what happens if the user does not have permission to edit?  Won't
they see a broken edit or add link?  Is there a framework behaviour or a
test that guarantees the right behaviour?


> +               <span tal:replace="structure component/distro_source_package/name"/></a>
> +          </span>
> +          <a class="sprite add"
> +             tal:condition="not: component/distro_source_package"
> +             tal:attributes="href string:${context/fmt:url}/+components/${component_group/id}/${component/id}/+edit"></a>
> +        </li>
> +        <li tal:condition="not: components">

Performance check: do we know that the "not: components" won't trigger
an extra database query?


> +          <i>This bug tracker has no components for this group</i>

This looks as if <em> would express your intent better than <i> does.


Jeroen