Hi Bryce,

Really nice branch, good work!. There are some issues with it, mostly
stylistic, that need fixing before it lands. Also, I think that we can
do away with the show(), hide() and setCustom() mutators and just have
attributes that are read-only for non-admins. Finally, you've got a
#TODO in the code that needs to go, but it seems to mark a problem that
hasn't been solved (or has been, but you've forgotten about the #TODO.

Cheers,

Graham

> === modified file 'lib/lp/bugs/interfaces/bugtracker.py'
> --- lib/lp/bugs/interfaces/bugtracker.py	2010-08-26 20:08:43 +0000
> +++ lib/lp/bugs/interfaces/bugtracker.py	2010-09-18 01:18:53 +0000
> @@ -353,6 +356,30 @@
>              point between now and 24 hours hence.
>          """
>  
> +    @operation_parameters(
> +        component_group_name=TextLine(
> +            title=u"The name of the remote component group", required=True))
> +    @export_write_operation()
> +    def addRemoteComponentGroup(component_group_name):
> +        """Adds a new component group to the bug tracker"""
> +
> +    @export_read_operation()
> +    def getAllRemoteComponentGroups():
> +        """Retrieve all of the registered component groups for bug tracker.
> +        """
> +
> +    @operation_parameters(
> +        component_group_name=TextLine(
> +            title=u"The name of the remote component group", required=True))
> +# TODO: Why can't I specify this as the return type?
> +#    @operation_returns_entry(IBugTrackerComponentGroup)

I don't know. Did you figure it out? If not, ping me abou this this
afternoon if you like and we'll try to work it out together. Obviously
the branch can't land with the TODO in it :).

> @@ -457,6 +484,80 @@
>          """Query IBugTrackerAliases by BugTracker."""
>  
>  
> +class IBugTrackerComponent(Interface):
> +    """The software component in the remote bug tracker.
> +
> +    Most bug trackers organize bug reports by the software 'component'
> +    they affect.  This class provides a mapping of this upstream component
> +    to the corresponding source package in the distro.
> +    """
> +    export_as_webservice_entry()
> +
> +    id = Int(title=_('ID'), required=True, readonly=True)
> +    is_visible = Bool(title=_('Is Visible?'),
> +                      description=_("Should the component be shown in "
> +                                    "the Launchpad web interface?"),
> +                      readonly=True)

We wrap arguments on the following line with a 4-space indent, thus:

    is_visible = Bool(
        title=_('Is Visible?'),
        description=_(
            "Should the component be shown in "
            "the Launchpad web interface?"),
        readonly=True)

Please fix the wrapping for all the below.

> +    is_custom = Bool(title=_('Is Custom?'),
> +                     description=_("Was the component added locally in "
> +                                   "Launchpad?  If it was, we must retain "
> +                                   "it across updates of bugtracker data."),
> +                     readonly=True)
> +
> +    name = exported(
> +        Text(
> +            title=_('Name'),
> +            constraint=name_validator,
> +            description=_('The name of a software component'
> +                          'in a remote bug tracker')))
> +
> +    distro_source_package = exported(
> +        Reference(
> +            title=_('Distribution Source Package'),
> +            schema=Interface,
> +            description=_('The distribution source package for this '
> +                          'component, if one has been defined.')))
> +
> +    @export_write_operation()
> +    def show():
> +        """Cause this component to be shown in the Launchpad web interface"""
> +
> +    @export_write_operation()
> +    def hide():
> +        """Do not show this component in the Launchpad web interface"""
> +

I think it would be simpler to just have self.is_visible an attribute
that's settable by launchpad.Admin.

> +
>[...]
> === modified file 'lib/lp/bugs/model/bugtracker.py'
> --- lib/lp/bugs/model/bugtracker.py	2010-08-26 20:08:43 +0000
> +++ lib/lp/bugs/model/bugtracker.py	2010-09-18 01:18:53 +0000
> @@ -6,10 +6,12 @@
>  __metaclass__ = type
>  __all__ = [
>      'BugTracker',
> +    'BugTrackerSet',
>      'BugTrackerAlias',
>      'BugTrackerAliasSet',
> -    'BugTrackerSet']
> -
> +    'BugTrackerComponent',
> +    'BugTrackerComponentGroup',
> +    ]
>  
>  from datetime import datetime
>  from itertools import chain
> @@ -21,9 +23,15 @@
>      splittype,
>      )
>  
> +from storm.base import Storm
> +from storm.locals import (
> +        Int,
> +        Reference,
> +        ReferenceSet,
> +        Unicode,
> +        )
>  from zope.component import getUtility
>  from zope.interface import implements
> -from zope.security.interfaces import Unauthorized
>  
>  from lazr.uri import URI
>  from pytz import timezone
> @@ -44,8 +52,6 @@
>      )
>  from storm.locals import Bool
>  from storm.store import Store
> -from zope.component import getUtility
> -from zope.interface import implements
>  
>  from canonical.database.enumcol import EnumCol
>  from canonical.database.sqlbase import (
> @@ -63,6 +69,8 @@
>      IBugTracker,
>      IBugTrackerAlias,
>      IBugTrackerAliasSet,
> +    IBugTrackerComponent,
> +    IBugTrackerComponentGroup,
>      IBugTrackerSet,
>      SINGLE_PRODUCT_BUGTRACKERTYPES,
>      )
> @@ -198,6 +206,9 @@
>          'BugWatch', joinColumn='bugtracker', orderBy='-datecreated',
>          prejoins=['bug'])
>  
> +    component_groups = SQLMultipleJoin(
> +        'BugTrackerComponentGroup', joinColumn='bug_tracker', orderBy='name')

You've used a SQLMultipleJoin here and then you don't use it in
getAllRemoteComponentGroups() below, which seems odd to me. Aren't
component_groups and the results of getAllRemoteComponentGroups() always
going to be the same? If so, we should do away with one of them. I'd
prefer to do away with getAllRemoteComponentGroups() and make
component_groups a property, possibly a cached one. What do you think?

>      _filing_url_patterns = {
>          BugTrackerType.BUGZILLA: (
>              "%(base_url)s/enter_bug.cgi?product=%(remote_product)s"
> @@ -515,6 +526,41 @@
>              next_check=new_next_check, lastchecked=None,
>              last_error_type=None)
>  
> +    def addRemoteComponentGroup(self, component_group_name):
> +        """See `IBugTracker`."""
> +
> +        if component_group_name is None:
> +            component_group_name = "default"
> +        component_group = BugTrackerComponentGroup()
> +        component_group.name = component_group_name
> +        component_group.bug_tracker = self
> +
> +        store = IStore(BugTrackerComponentGroup)
> +        store.add(component_group)
> +        store.commit()
> +
> +        return component_group
> +
> +    def getAllRemoteComponentGroups(self):
> +        """See `IBugTracker`."""
> +        component_groups = []
> +
> +        component_groups = Store.of(self).find(
> +            BugTrackerComponentGroup,
> +            BugTrackerComponentGroup.bug_tracker == self.id)
> +        component_groups = component_groups.order_by(
> +            BugTrackerComponentGroup.name)
> +        return component_groups
> +
> +    def getRemoteComponentGroup(self, component_group_name):
> +        """See `IBugTracker`."""
> +        component_group = None
> +        store = IStore(BugTrackerComponentGroup)
> +        component_group = store.find(
> +            BugTrackerComponentGroup,
> +            name = component_group_name).one()
> +        return component_group
> +
>  
>  class BugTrackerSet:
>      """Implements IBugTrackerSet for a container or set of BugTrackers,
> @@ -614,7 +660,9 @@
>      def getMostActiveBugTrackers(self, limit=None):
>          """See `IBugTrackerSet`."""
>          store = IStore(self.table)
> -        result = store.find(self.table, self.table.id == BugWatch.bugtrackerID)
> +        result = store.find(
> +            self.table,
> +            self.table.id == BugWatch.bugtrackerID)
>          result = result.group_by(self.table)
>          result = result.order_by(Desc(Count(BugWatch)))
>          if limit is not None:

Whilst you're here, you could s/self\.table/BugTracker/ in this method
so that I don't have to go hunting around to find out what it is.

> @@ -657,3 +705,104 @@
>      def queryByBugTracker(self, bugtracker):
>          """See IBugTrackerSet."""
>          return self.table.selectBy(bugtracker=bugtracker.id)
> +
> +
> +class BugTrackerComponent(Storm):
> +    """The software component in the remote bug tracker.
> +
> +    Most bug trackers organize bug reports by the software 'component'
> +    they affect.  This class provides a mapping of this upstream component
> +    to the corresponding source package in the distro.
> +    """
> +    implements(IBugTrackerComponent)
> +    __storm_table__ = 'BugTrackerComponent'
> +
> +    id = Int(primary=True)
> +    name = Unicode(allow_none=False)
> +
> +    component_group_id = Int('component_group')
> +    component_group = Reference(
> +        component_group_id,
> +        'BugTrackerComponentGroup.id')
> +
> +    is_visible = Bool(allow_none=False)
> +    is_custom = Bool(allow_none=False)
> +
> +    distro_source_package_id = Int('distro_source_package')
> +    distro_source_package = Reference(
> +        distro_source_package_id,
> +        'DistributionSourcePackageInDatabase.id')
> +
> +    def show(self):
> +        if not self.is_visible:
> +            self.is_visible = True
> +
> +    def hide(self):
> +        if self.is_visible:
> +            self.is_visible = False
> +
> +    def setCustom(self):
> +        if not self.is_custom:
> +            self.is_custom = True

Again, this can be a setAttribute in the ZCML and you can do away with
the mutator.

> +class BugTrackerComponentGroup(Storm):
> +    """A collection of components in a remote bug tracker.
> +
> +    Some bug trackers organize sets of components into higher level groups,
> +    such as Bugzilla's 'product'.
> +    """
> +    implements(IBugTrackerComponentGroup)
> +    __storm_table__ = 'BugTrackerComponentGroup'
> +
> +    id = Int(primary=True)
> +    name = Unicode(allow_none=False)
> +    bug_tracker_id = Int('bug_tracker')
> +    bug_tracker = Reference(bug_tracker_id, 'BugTracker.id')
> +
> +    components = ReferenceSet(
> +        id,
> +        BugTrackerComponent.component_group_id,
> +        order_by=BugTrackerComponent.name)
> +
> +    def addComponent(self, component_name):
> +        """Adds a component that is synced from a remote bug tracker"""
> +
> +        component = BugTrackerComponent()
> +        component.name = component_name
> +        component.component_group = self
> +
> +        store = IStore(BugTrackerComponent)
> +        store.add(component)
> +        store.commit()

I think this should be store.flush().

> +
> +        return component
> +
> +    def getComponent(self, component_name):
> +        """Retrieves a component by the given name.
> +
> +        None is returned if there is no component by that name in the
> +        group.
> +        """
> +
> +        if component_name is None:
> +            return None
> +        else:
> +            return Store.of(self).find(
> +                BugTrackerComponent,
> +                (BugTrackerComponent.name == component_name)).one()
> +
> +    def addCustomComponent(self, component_name):
> +        """Adds a component locally that isn't synced from a remote tracker
> +        """
> +
> +        component = BugTrackerComponent()
> +        component.name = component_name
> +        component.component_group = self
> +        component.setCustom()
> +
> +        store = IStore(BugTrackerComponent)
> +        store.add(component)
> +        store.commit()

I think this should be store.flush().

> +
> +        return component
> === added file 'lib/lp/bugs/tests/test_bugtracker_components.py'
> --- lib/lp/bugs/tests/test_bugtracker_components.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/bugs/tests/test_bugtracker_components.py	2010-09-18 01:18:53 +0000
> @@ -0,0 +1,185 @@
> +# Copyright 2009 Canonical Ltd.  This software is licensed under the

It's 2010 :).

> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Test for components and component groups (products) in bug trackers."""
> +
> +__metaclass__ = type
> +
> +__all__ = []
> +
> +import unittest
> +
> +from canonical.launchpad.ftests import login_person
> +from canonical.testing import DatabaseFunctionalLayer
> +from lp.testing import TestCaseWithFactory
> +
> +
> +class TestBugTrackerComponent(TestCaseWithFactory):
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        super(TestBugTrackerComponent, 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 test_component_creation(self):
> +        """Verify a component can be created"""
> +

This VWS confused me. Please nuke it.

> +        component = self.factory.makeBugTrackerComponent(
> +            u'example', self.comp_group)
> +        self.assertTrue(component is not None)
> +        self.assertEqual(component.name, u'example')
> +
> +    def test_set_visibility(self):
> +        """Users can delete components
> +
> +        In case invalid components get imported from a remote bug
> +        tracker, users can hide them so they don't show up in the UI.
> +        We do this rather than delete them outright so that they won't
> +        show up again when we re-sync from the remote bug tracker.
> +        """
> +

VWS again.

> +        component = self.factory.makeBugTrackerComponent(
> +            u'example', self.comp_group)
> +        self.assertEqual(component.is_visible, True)
> +
> +        component.hide()
> +        self.assertEqual(component.is_visible, False)
> +
> +        component.show()
> +        self.assertEqual(component.is_visible, True)
> +
> +    def test_custom_component(self):
> +        """Users can also add components
> +
> +        For whatever reason, it may be that we can't import a component
> +        from the remote bug tracker.  This gives users a way to correct
> +        the omissions."""
> +

And again.

> +        custom_component = self.factory.makeBugTrackerComponent(
> +            u'example', self.comp_group, custom=True)
> +        self.assertTrue(custom_component != None)
> +        self.assertEqual(custom_component.is_custom, True)
> +
> +    def test_multiple_component_creation(self):
> +        """Verify several components can be created at once"""
> +

And again.

> +        comp_a = self.factory.makeBugTrackerComponent(
> +            u'example-a', self.comp_group)
> +        comp_b = self.factory.makeBugTrackerComponent(
> +            u'example-b', self.comp_group)
> +        comp_c = self.factory.makeBugTrackerComponent(
> +            u'example-c', self.comp_group, True)
> +
> +        self.assertTrue(comp_a is not None)
> +        self.assertTrue(comp_b is not None)
> +        self.assertTrue(comp_c is not None)
> +
> +
> +class TestBugTrackerWithComponents(TestCaseWithFactory):
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        super(TestBugTrackerWithComponents, self).setUp()
> +
> +        regular_user = self.factory.makePerson()
> +        login_person(regular_user)
> +
> +        self.bug_tracker = self.factory.makeBugTracker()
> +
> +    def test_empty_bugtracker(self):
> +        """Trivial case of bugtracker with no products or components"""
> +

And again.

> +        self.assertTrue(self.bug_tracker is not None)
> +
> +        # Empty bugtrackers shouldn't return component groups
> +        comp_group = self.bug_tracker.getRemoteComponentGroup(u'non-existant')
> +        self.assertEqual(comp_group, None)
> +
> +        # Verify it contains no component groups
> +        comp_groups = self.bug_tracker.getAllRemoteComponentGroups()
> +        self.assertEqual(len(list(comp_groups)), 0)
> +
> +    def test_single_product_bugtracker(self):
> +        """Bug tracker with a single (default) product and several components
> +        """

And again.

> +
> +        # Add a component group and fill it with some components
> +        default_comp_group = self.bug_tracker.addRemoteComponentGroup(
> +            u'alpha')
> +        default_comp_group.addComponent(u'example-a')
> +        default_comp_group.addComponent(u'example-b')
> +        default_comp_group.addComponent(u'example-c')
> +
> +        # Verify that retrieving an invalid component group returns nothing
> +        comp_group = self.bug_tracker.getRemoteComponentGroup(u'non-existant')
> +        self.assertEqual(comp_group, None)
> +
> +        # Now retrieve the component group we added
> +        comp_group = self.bug_tracker.getRemoteComponentGroup(u'alpha')
> +        self.assertEqual(comp_group, default_comp_group)
> +        self.assertEqual(comp_group.name, u'alpha')
> +
> +        # Verify there is only the one component group in the tracker
> +        comp_groups = self.bug_tracker.getAllRemoteComponentGroups()
> +        self.assertEqual(len(list(comp_groups)), 1)
> +
> +    def test_multiple_product_bugtracker(self):
> +        """Bug tracker with multiple products and components"""
> +

And again.

> +        # Create several component groups with varying numbers of components
> +        comp_group_i = self.bug_tracker.addRemoteComponentGroup(u'alpha')
> +
> +        comp_group_ii = self.bug_tracker.addRemoteComponentGroup(u'beta')
> +        comp_group_ii.addComponent(u'example-beta-1')
> +
> +        comp_group_iii = self.bug_tracker.addRemoteComponentGroup(u'gamma')
> +        comp_group_iii.addComponent(u'example-gamma-1')
> +        comp_group_iii.addComponent(u'example-gamma-2')
> +        comp_group_iii.addComponent(u'example-gamma-3')
> +
> +        # Retrieving a non-existant component group returns nothing
> +        comp_group = self.bug_tracker.getRemoteComponentGroup(u'non-existant')
> +        self.assertEqual(comp_group, None)
> +
> +        # Now retrieve one of the real component groups
> +        comp_group = self.bug_tracker.getRemoteComponentGroup(u'beta')
> +        self.assertEqual(comp_group, comp_group_ii)
> +
> +        # Check the correct number of component groups are in the bug tracker
> +        comp_groups = self.bug_tracker.getAllRemoteComponentGroups()
> +        self.assertEqual(len(list(comp_groups)), 3)
> +
> +    def test_get_components_for_component_group(self):
> +        """Retrieve a set of components from a given product"""
> +

And again.

> +        # Create a component group with some components
> +        default_comp_group = self.bug_tracker.addRemoteComponentGroup(
> +            u'alpha')
> +        default_comp_group.addComponent(u'example-a')
> +        default_comp_group.addComponent(u'example-b')
> +        default_comp_group.addComponent(u'example-c')
> +
> +        # Verify group has the correct number of components
> +        comp_group = self.bug_tracker.getRemoteComponentGroup(u'alpha')
> +        self.assertEqual(len(list(comp_group.components)), 3)
> +
> +        # Check one of the components, that it is what we expect
> +        comp = comp_group.getComponent(u'example-b')
> +        self.assertEqual(comp.name, u'example-b')
> +
> +
> +def test_suite():
> +    suite = unittest.TestSuite()
> +    suite.addTest(unittest.TestLoader().loadTestsFromName(__name__))
> +
> +    return suite
> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py	2010-09-17 10:44:05 +0000
> +++ lib/lp/testing/factory.py	2010-09-18 01:18:53 +0000
> @@ -1385,6 +1385,38 @@
>          return getUtility(IBugTrackerSet).ensureBugTracker(
>              base_url, owner, bugtrackertype, title=title, name=name)
>  
> +    def makeBugTrackerComponentGroup(self, name=None, bug_tracker=None):
> +        """Make a new bug tracker component group."""
> +
> +        if name is None:
> +            name = u'default'
> +
> +        if bug_tracker is None:
> +            bug_tracker = self.makeBugTracker()
> +
> +        component_group = bug_tracker.addRemoteComponentGroup(name)
> +        return component_group
> +
> +    def makeBugTrackerComponent(self, name=None, component_group=None,
> +                                custom=None):
> +        """Make a new bug tracker component."""
> +
> +        if name is None:
> +            name = u'default'
> +
> +        if component_group is None:
> +            component_group = self.makeBugTrackerComponentGroup()
> +
> +        if custom is None:
> +            custom = False
> +
> +        if custom:
> +            component = component_group.addCustomComponent(name)
> +        else:
> +            component = component_group.addComponent(name)
> +
> +        return component
> +

There's quite a bit of VWS in these two methods that can be removed, I
think. It doesn't add to the readability of the code since the methods
aren't particularly dense.

>      def makeBugWatch(self, remote_bug=None, bugtracker=None, bug=None,
>                       owner=None, bug_task=None):
>          """Make a new bug watch."""