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

Proposed by Bryce Harrington
Status: Merged
Approved by: Graham Binns
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
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Stuart Bishop (community) db Approve
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.

To post a comment you must log in.
Revision history for this message
Bryce Harrington (bryce) wrote :

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) :
review: Approve (db)
Revision history for this message
Graham Binns (gmb) wrote :
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...

review: Needs Fixing (code)
Revision history for this message
Bryce Harrington (bryce) wrote :
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?'),
> > + ...

Revision history for this message
Graham Binns (gmb) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to status/vote changes: