> === modified file 'lib/canonical/widgets/bugtask.py' Hi Bryce. I have some questions about some of the methods and properties. I have some suggestions that I think you can make in less than an hour to improve this branch. > --- lib/canonical/widgets/bugtask.py 2010-11-08 12:52:43 +0000 > +++ lib/canonical/widgets/bugtask.py 2010-12-03 23:48:54 +0000 I think you need to remerge. This module was moved about 4 months ago. I expect this to be lp/bugs/browser/widgets/bugtask.py. I see conflicts too. > @@ -495,6 +495,25 @@ > return distribution > > > +class UbuntuSourcePackageNameWidget( > + BugTaskSourcePackageNameWidget): > + """Package widget where the distribution can be assumed as Ubuntu > + > + This widgets works the same as `BugTaskSourcePackageNameWidget`, > + except that it assumes the distribution is 'ubuntu'. > + """ > + 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 I think this could be expressed differently. We are not /assuming/ that the distro is Ubuntu, it is. To ensure it is this, the whole class would look like: class UbuntuSourcePackageNameWidget(BugTaskSourcePackageNameWidget): """A widget to select Ubuntu packages.""" def getDistribution(self): """See `BugTaskSourcePackageNameWidget`""" return getUtility(ILaunchpadCelebrities).ubuntu > === modified file 'lib/lp/bugs/browser/bugtracker.py' > --- lib/lp/bugs/browser/bugtracker.py 2010-11-23 23:22:27 +0000 > +++ lib/lp/bugs/browser/bugtracker.py 2010-12-03 23:48:54 +0000 > ... > @@ -231,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 > + > + This property was created for the Related components portlet in > + the bug tracker's page. > + """ > + return self.context.getAllRemoteComponentGroups() Properties docstring do avoid "return" because they are not intended to be methods. The call site is not interesting. The docstring could be. """All component groups and components.""" > @@ -449,16 +463,111 @@ ... > +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. > + """ > + > + schema = IBugTrackerComponent > + custom_widget('sourcepackagename', > + UbuntuSourcePackageNameWidget) > + > + @property > + def page_title(self): > + return smartquote( > + u'Link a distribution source package to the %s component' > + % self.context.name) This is an odd breadcrumb/page_title. Labels are meant to be verbose, while this attribute is meant to be tearse so that it fits in the breadcrumbs: Bug trackers > AbiSource bug tracker > Link component > + @property > + def field_names(self): > + field_names = [ > + 'sourcepackagename', > + ] > + return field_names This is complex for a simple list. Most Lp forms just do: field_names = ['sourcepackagename'] > + 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__] > + self.widgets = form.setUpWidgets( > + self.form_fields, self.prefix, self.context, self.request, > + data=self.initial_values, adapters=self.adapters, > + ignore_request=False) I do not understand why this method is needed. I have written something like this when I had another method that mutated the view's field names, but this view does not. This method looks like it was taken from LaunchpadFormView. I really think this could be deleted. ... > + 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) Why is the vetted and converted data in "data" ignored. The form contains raw, possibly dangerous values. This gets the actual SourcePackageName sourcepackagename = data['sourcepackagename'] Which is what you want, Lp is doing extra work by getting the string. > + distro_name = self.widgets['sourcepackagename'].distribution_name > + distribution = getUtility(IDistributionSet).getByName(distro_name) The widget already knows the distribution, we do not need to do duplicate work. distribution = self.widgets['sourcepackagename'].getDistribution() > + pkg = distribution.getSourcePackage(sourcepackagename) pkg here is actually a dsp, and getSourcePackage wants the object, and will do an extra db call to convert the string pkg = distribution.getSourcePackage(sourcepackagename) ... Think this method could be condensed to look like: 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. """ component = context or self.context sourcepackagename = data['sourcepackagename'] distribution = self.widgets['sourcepackagename'].getDistribution() dsp = distribution.getSourcePackage(sourcepackagename) bug_tracker = self.context.component_group.bug_tracker # Has this source package already been assigned to a component? link_comp = bug_tracker.getRemoteComponentForDistroSourcePackage( distribution.name, sourcepackagename.name) if link_comp is not None: self.request.response.addNotification( "The %s source package is already linked to %s:%s in %s" %( sourcepackagename.anme, link_comp.component_group.name, link_comp.name, distribution.name)) return component.distro_source_package = dsp self.request.response.addNotification( "%s:%s is now linked to the %s source package in %s" %( component.component_group.name, component.name, sourcepackagename.name, distribution.name)) I have some reservations about getRemoteComponentForDistroSourcePackage(). I think it should take a distro and spn, not there names > === modified file 'lib/lp/bugs/browser/configure.zcml' > --- lib/lp/bugs/browser/configure.zcml 2010-10-28 09:11:36 +0000 > +++ lib/lp/bugs/browser/configure.zcml 2010-12-03 23:48:54 +0000 > @@ -811,6 +811,12 @@ > path_expression="name" > attribute_to_parent="component_group" > rootsite="bugs"/> > + + name="+edit" > + for="lp.bugs.interfaces.bugtracker.IBugTrackerComponent" > + class="lp.bugs.browser.bugtracker.BugTrackerEditComponentView" > + permission="launchpad.AnyPerson" > + template="../templates/bugtracker-edit-component.pt"/> use the generic template instead. template="../../app/templates/generic-edit.pt" > === 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-12-03 23:48:54 +0000 ... > + 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) This test asserts that the user see the label phrase as a

, then under it as a breadcrumb; page_title should be terse. ... > === 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-12-03 23:48:54 +0000 ... > + def getRemoteComponentForDistroSourcePackage( > + self, distro_name, source_package_name): > + """See `IBugTracker`.""" > + distribution = getUtility(IDistributionSet).getByName(distro_name) > + if distribution is None: > + return None > + pkg = distribution.getSourcePackage(source_package_name) > + > + pkgs = Store.of(self).find( > + BugTrackerComponent, > + BugTrackerComponent.distribution == distribution.id, > + BugTrackerComponent.source_package_name == > + pkg.sourcepackagename.id) > + if pkgs.count() == 0: > + return None > + assert(pkgs.count() == 1) > + return pkgs[0] The assert here is scary. It implies we do not trust this code. Calling store.find().one() guarantees None or 1, and I think that is what should be used. The schema should have a constraint to ensure that BugTrackerComponent(distribution, source_package_name) is unique. > === added file 'lib/lp/bugs/templates/bugtracker-edit-component.pt' > --- lib/lp/bugs/templates/bugtracker-edit-component.pt 1970-01-01 00:00:00 +0000 > +++ lib/lp/bugs/templates/bugtracker-edit-component.pt 2010-12-03 23:48:54 +0000 > @@ -0,0 +1,16 @@ > + + xmlns="http://www.w3.org/1999/xhtml" > + xmlns:tal="http://xml.zope.org/namespaces/tal" > + xmlns:metal="http://xml.zope.org/namespaces/metal" > + xmlns:i18n="http://xml.zope.org/namespaces/i18n" > + metal:use-macro="view/macro:page/main_only" > + i18n:domain="malone"> > + > +
> +

Link component ''

> +
> +

Configure component

> +
> +
> + > + Forms get their

from the Schema or view's label. This template is not needed. The ZCML should use generic-edit.pt