Merge into db-devel : lp-617679-code : Code : Launchpad itself

Status:

Merged

Approved by:

Graham Binns on 2010-09-24

Approved revision:

no longer in the source branch.

Merged at revision:

9852

Proposed branch:

lp://staging/~bryce/launchpad/lp-617679-code

Merge into:

lp://staging/launchpad/db-devel

Diff against target:

659 lines (+482/-8)

7 files modified

database/schema/patch-2208-09-0.sql (+1/-1)
database/schema/security.cfg (+5/-0)
lib/lp/bugs/configure.zcml (+46/-0)
lib/lp/bugs/interfaces/bugtracker.py (+89/-0)
lib/lp/bugs/model/bugtracker.py (+140/-7)
lib/lp/bugs/tests/test_bugtracker_components.py (+176/-0)
lib/lp/testing/factory.py (+25/-0)

To merge this branch:

bzr merge lp://staging/~bryce/launchpad/lp-617679-code

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Graham Binns (community)	code	2010-09-18	Approve on 2010-09-24
Stuart Bishop (community)	db	2010-09-20	Approve on 2010-09-21
Review via email: mp+35905@code.staging.launchpad.net

Commit message

Provide interface and model layer for tracking Bugzilla components in launchpad.

Description of the change

This implements the interface and model layer for adding tracking of Bugzilla components in launchpad, building on branch lp-617679-db.

This adds two classes BugTrackerComponent and BugTrackerComponentGroup to wrap the corresponding tables. It also adds a routine to the BugTracker class to allow adding component groups there.

For testing, I've been using this command line:
./bin/test -t bugtracker_components

A pre-implementation design meeting was done with Deryck at the Launchpad Epic and I've had several follow up mutter discussions with him about particulars.

One missing part is this does not implement the actual linking of components to source packages, but that bit of functionality was proving difficult to implement so I'm going to leave that to a follow up branch.

Revision history for this message

Bryce Harrington (bryce) wrote on 2010-09-20:

#

Note, the database changes were done on a separate branch [1], which has already landed in db-devel. This branch does contain changes to database/schema/security.cfg to enable permissions for the two tables added in in that branch, so I am subscribing stub to db review that portion of it.

1: https://code.edge.launchpad.net/~bryceharrington/launchpad/lp-617679-db/

Revision history for this message

Stuart Bishop (stub) on 2010-09-21:

#

review: Approve (db)

Revision history for this message

Graham Binns (gmb) wrote on 2010-09-21:

#

Download full text (21.0 KiB)

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,
> + de...

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."""

review: Needs Fixing (code)

Revision history for this message

Bryce Harrington (bryce) wrote on 2010-09-24:

#

Download full text (22.6 KiB)

Hi Graham,

Thanks for the review, I've pushed updates to the branch that should
cover all the issues, mind taking another look when you get a chance?

Thanks,
Bryce

On Tue, Sep 21, 2010 at 10:24:39AM -0000, Graham Binns wrote:
> Review: Needs Fixing code
> 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?'),
> > + ...

Hi Graham,

Thanks for the review, I've pushed updates to the branch that should
cover all the issues, mind taking another look when you get a chance?

Thanks,
Bryce

On Tue, Sep 21, 2010 at 10:24:39AM -0000, Graham Binns wrote:
> Review: Needs Fixing code
> 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."""
> 
> -- 
> https://code.launchpad.net/~bryceharrington/launchpad/lp-617679-code/+merge/35905
> You are the owner of lp:~bryceharrington/launchpad/lp-617679-code.

Revision history for this message

Graham Binns (gmb) on 2010-09-24:

#

review: Approve (code)

Launchpad itself

Merge lp://staging/~bryce/launchpad/lp-617679-code into lp://staging/launchpad/db-devel

Commit message

Description of the change

Preview Diff

Subscribers