Launchpad itself

Merge lp://staging/~bryce/launchpad/lp-617698-forwarding into lp://staging/launchpad/db-devel

lp-617698-forwarding
Merge into db-devel

Proposed by Bryce Harrington on 2010-11-16

Status:

Rejected

Rejected by:

Curtis Hovey on 2011-06-14

Proposed branch:

lp://staging/~bryce/launchpad/lp-617698-forwarding

Merge into:

lp://staging/launchpad/db-devel

Prerequisite:

lp://staging/~bryce/launchpad/lp-617695-linkui

Diff against target:

1109 lines (+705/-45)

17 files modified

lib/canonical/launchpad/scripts/bzremotecomponentfinder.py (+6/-2)
lib/lp/bugs/browser/bugalsoaffects.py (+23/-7)
lib/lp/bugs/browser/bugtracker.py (+106/-4)
lib/lp/bugs/browser/configure.zcml (+9/-0)
lib/lp/bugs/browser/tests/bugtask-adding-views.txt (+1/-1)
lib/lp/bugs/browser/tests/test_bugtracker_component.py (+189/-0)
lib/lp/bugs/browser/widgets/bugtask.py (+20/-0)
lib/lp/bugs/doc/bugtracker.txt (+13/-20)
lib/lp/bugs/interfaces/bugtracker.py (+8/-2)
lib/lp/bugs/model/bugtracker.py (+29/-9)
lib/lp/bugs/templates/bugtracker-edit-component.pt (+16/-0)
lib/lp/bugs/templates/bugtracker-index.pt (+2/-0)
lib/lp/bugs/templates/bugtracker-portlet-components.pt (+36/-0)
lib/lp/registry/configure.zcml (+1/-0)
lib/lp/registry/interfaces/sourcepackage.py (+2/-0)
lib/lp/registry/model/sourcepackage.py (+11/-0)
lib/lp/services/features/scopes.py (+233/-0)

To merge this branch:

bzr merge lp://staging/~bryce/launchpad/lp-617698-forwarding

High

Triaged

Link a bug report

Reviewer	Review Type	Date Requested	Status
Jeroen T. Vermeulen (community)	code	2010-11-16	Approve on 2011-04-27
Bryce Harrington (community)			Needs Resubmitting on 2011-04-25
Review via email: mp+41003@code.staging.launchpad.net

Commit message

Implement remote_component support when forwarding bugs.

Description of the change

Implement remote_component support when forwarding bugs.

This branch is dependent on the lp:~bryce/launchpad/lp-617695-linkui so that branch should be landed first.

This causes the 'component' field to be properly filled in for the bugzilla submission page when forwarding a bug report to a remote project.

lint has been checked. ec2 test has been run and passed successfully.

== Testing ==
For manual testing of this feature locally, I first ran cronscripts/update-bugzilla-remote-components.py to populate the mozilla and gnome bugzillas instances in the sample data.

[ Next I turned the feature flag on via the psql statement 'insert into featureflag (scope, priority, flag, value) values ('default', 1, 'bugtracker_components', 'on');'. However I had so much trouble getting feature flags to work properly that I've dropped this from the branch. ]

Next, I configured the alsa-utils project as using the gnome bugzilla, at https://bugs.launchpad.dev/alsa-utils/+configure-bugtracker by selecting "In a registered bug tracker" and typing in "gnome-bugs"; for the field "Project ID in bug tracker" I typed in "zenity" just to pick a random existing project.

Navigating to https://bugs.launchpad.dev/bugs/bugtrackers/gnome-bugs I scrolled down to the end and under the 'zenity' component group I clicked the icon next to 'general'. On the linking form I specified 'alsa-utils' as the package. I doublechecked that zenity/general now showed a link to 'alsa-utils'.

I logged out, and verified as anonymous that the edit links don't show up on https://bugs.launchpad.dev/bugs/bugtrackers/gnome-bugs.

Finally, after re-logging in, I filed a new bug report against alsa-utils, clicked "Also affects project", clicked the "bug filing form" link, and verified it filled in the component field in the bugzilla form properly.

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2010-11-17:

Download full text (25.6 KiB)

Hi Bryce,

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

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

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

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

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

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

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

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

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

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

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

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

return getUtility(ILaunchpadCelebrities).ubuntu

Hi Bryce,

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

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

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

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

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

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

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

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

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

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

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

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

return getUtility(ILaunchpadCelebrities).ubuntu

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

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

Some suggestions:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

custom_widget(
        'sourcepackagename', UbuntuSourcePackageNameWidget)

custom_widget(
        'sourcepackagename',
        UbuntuSourcePackageNameWidget)

or in this case even

custom_widget('sourcepackagename', UbuntuSourcePackageNameWidget)

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

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

return ['sourcepackagename']

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

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

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

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

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

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

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

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

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

> +        if pkgs.count()>0:

There should be whitespace around the ">" operator.

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

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

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

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

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

This uses the same component description again.

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

Ah, the joy of doctest maintenance!

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

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

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

Dead code!

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Typo: s/fource/four/

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

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

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

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

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

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

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

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

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

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

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

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

(Same here, of course.)

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

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

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

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

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

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

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

Jeroen

review: Needs Information

lp://staging/~bryce/launchpad/lp-617698-forwarding updated on 2010-12-15

9926. By Bryce Harrington on 2010-11-23: Review comments by Jeroen.
9927. By Bryce Harrington on 2010-12-02: Review by Jerone: Code can be simplified using the ubuntu celebrity
9928. By Bryce Harrington on 2010-12-02: Almost all of the method is now an "else" clause after an early
return; we can drop the else and make the code stand on its own.
9929. By Bryce Harrington on 2010-12-02: Review by Jerone: Make if statement easier to read
9930. By Bryce Harrington on 2010-12-02: Review by Jeroen: Split out lookup functionality to separate routine
9931. By Bryce Harrington on 2010-12-02: Review by Jeroen: Find way to maintain visual separation between the two
if conditions and the body
9932. By Bryce Harrington on 2010-12-03: Review by Jeroen: Note interface change in docstring
9933. By Bryce Harrington on 2010-12-03: Review by Jeroen: Paragraph is too detailed for a doctest and mostly is
just stating obvious stuff about a function's parameters that doesn't
belong here.
9934. By Bryce Harrington on 2010-12-03: Review by Jeroen: Simplify setup by using helper functions.
9935. By Bryce Harrington on 2010-12-03: Review by Jeroen: Split up linking test into two tests
9936. By Bryce Harrington on 2010-12-03: Review by Jeroen: Use dict to eliminate some if clauses
9937. By Bryce Harrington on 2010-12-03: Review by Jeroen: Put a space in "% ("
9938. By Bryce Harrington on 2010-12-03: Review by Jeroen: Construct full name in a variable to make easier to follow
9939. By Bryce Harrington on 2010-12-03: Review by Jeroen: Use more verbose but conventional way of calling find.
9940. By Bryce Harrington on 2010-12-03: Re-merge db-devel trunk
9941. By Bryce Harrington on 2010-12-14: Review by Jeroen: State how the view should behave in this situation.
9942. By Bryce Harrington on 2010-12-14: Unnecessary to re-test output from the first component since this was
tested previously and is just setup here.
9943. By Bryce Harrington on 2010-12-14: Review by Jeroen: It doesn't matter how the first component got linked
so just treat it as setup and link the package directly, so that this
test focuses just on testing one thing.
9944. By Bryce Harrington on 2010-12-14: Review by Jeroen: You could use
self.assertTextMatchesExpressionIgnoreWhitespace for the
verification.
9945. By Bryce Harrington on 2010-12-14: Avoid potential extra database call when there are no components
9946. By Bryce Harrington on 2010-12-14: Review by Jeroen: Use URL formatter for links
9947. By Bryce Harrington on 2010-12-14: Review by Jeroen: Only display editing links if there is a logged in user
9948. By Bryce Harrington on 2010-12-15: Omit the nonbreaking space for anonymous users
9949. By Bryce Harrington on 2010-12-15: Preliminary implementation of test for checking anonymous edits
9950. By Bryce Harrington on 2010-12-15: Re-merge db-devel trunk
9951. By Bryce Harrington on 2010-12-15: Fix typo in find conditional
9952. By Bryce Harrington on 2010-12-15: ILaunchpadCelebrities needs declared
9953. By Bryce Harrington on 2010-12-15: Fix up several test errors

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2011-01-12:

Hi Bryce,

I see that you've done a lot of work on this branch since my initial review. Looks a lot better (though watch that double blank line below test_no_anonymous_edits ☺).

Please let me know when it's ready for the next round and hopefully we can get it landed!

lp://staging/~bryce/launchpad/lp-617698-forwarding updated on 2011-01-25

9954. By Bryce Harrington on 2011-01-05

Use a store call to delete components.

9955. By Bryce Harrington on 2011-01-08

Enable testing generated links

Featureflags and testing view/user are interfering, so disable them
temporarily.

9956. By Bryce Harrington on 2011-01-08

Adjust create_initialized_view() call to set up proper logged in user

9957. By Bryce Harrington on 2011-01-20

Further experimentation with feature flags.

Just can't sort out how to make it work in a unittest. :-/

9958. By Bryce Harrington on 2011-01-20

Re-merge trunk

9959. By Bryce Harrington on 2011-01-24

Giving up on feature flags stuff and ripping it out.

The flags work fine in the browser when setting the flag manually, but
I just can't sort out how to get the unittests to recognize it. This
issue has been blocking progress on this branch for months now, and I've
failed to find anyone who can explain how to make it work properly.
Martin Pool suggested just dropping it and moving on, which sounds good
to me.

9960. By Bryce Harrington on 2011-01-24

Re-merge db-devel trunk

9961. By Bryce Harrington on 2011-01-24

Adjust to new widgets location

Revision history for this message

Bryce Harrington (bryce) wrote on 2011-04-25:

Hi Jeroen, yeah the extensiveness of the review made for a rather Herculean labor...

I'm afraid since returning from launchpad X.org has monopolized my attention, however I think this branch is more or less done and ready to go, but I haven't looked at it in a while; I'll check it over again.

lp://staging/~bryce/launchpad/lp-617698-forwarding updated on 2011-04-25

9962. By Bryce Harrington on 2011-04-25

Compare string against the '.name' field of DSP rather than object itself.

9963. By Bryce Harrington on 2011-04-25

Revert Jeroen's change for component form so editing components works.

In review, Jeroen noticed I had an ugly if clause to look up the source
package name field for the component edit form, and suggested replacing
it with a dict to look up the fields.

Unfortunately, it's not so simple. The SourcePackageName object is not
a string property, so has to be handled differently than other kinds of
objects (we have to explicitly reference it's .name field to get the
string out of it). So the None check and special handling of this field
is required. Otherwise, an Oops occurs when it tries to fill in the
package name field with the object rather than the string.

9964. By Bryce Harrington on 2011-04-25

Re-merge db-devel trunk

Revision history for this message

Bryce Harrington (bryce) wrote on 2011-04-25:

Alright, I exercised the code for a while, found a couple issues that were introduced due to changes from the review and got them fixed up.

I also re-merged db-devel and fixed a minor conflict; unfortunately it seems launchpad has moved from postgres 8.3 to 8.4, and the upgrade scripts seem to have borked up my system, so I can no longer make schema or run tests locally. Hopefully it's correct though.

I don't know when I'll next get some spare moments to get back to launchpad work, so if there are any remaining minor niggles like whitespace or formatting or UI tweaks or whatnot, if you don't mind please feel free to correct those. Also, if someone else could handle testing and landing this baby I would *really* appreciate getting it off my plate.

review: Needs Resubmitting

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2011-04-27:

Bryce,

Thanks for sticking with it! I'm re-reviewing it. Meanwhile, to get your Launchpad instance working again:

* Make sure your postgres config has 8.4 running on port 5432. If you install it side by side with an existing 8.3, it'll try to evade to port 5433 instead. Get rid of the 8.3.

* There's a script called launchpad-database-setup that re-creates your entire database. This is highly destructive (so better not do it if you have any other postgres databases on that system) but tends to fix these problems.

Jeroen

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2011-04-27:

Download full text (4.8 KiB)

= Approving =

This looks like Pretty Good Stuff now. The commit logs show lots of changes (and actually explain them, which is a nice touch).

I see just 2 things that need fixing, but I'm sure that won't be helped by any further nannying from me. See "Important" below. Some other notes that you may want to consider or improve at your option: nice to have but not worth further delay.

== Design ==

I see a few documented assumptions (very thoughtful, by the way!) about Ubuntu being involved. Does that fit with the ongoing work for supporting derived distributions? Probably depends on the requirements for derived distros.

If not, it may present some new development tasks for derived distros. I wouldn't hold up your branch for it unless it poses some kind of serious problem.

== Important ==

=== Jack-in-the-box file ===

According to the MP diff, you're _adding_ lib/lp/services/features/scopes.py. Which already exists, with contents apparently similar or identical to what's in the diff. How can that happen!?

=== Model/view boundaries ===

In lib/lp/bugs/browser/bugtracker.py, you query the database directly:

188 + def updateContextFromData(self, data, context=None):
[...]
208 + # Has this source package already been assigned to a component?
209 + pkgs = Store.of(component).find(
210 + BugTrackerComponent,
211 + BugTrackerComponent.distribution == distribution.id,
212 + BugTrackerComponent.source_package_name ==
213 + pkg.sourcepackagename.id)
214 + if pkgs.count() > 0:
215 + self.request.response.addNotification(
216 + "The %s source package is already linked to %s in %s" % (
217 + sourcepackagename, component_full_name, distro_name))
218 + return

Querying the database from browser code is frowned upon: it's a job for the model, not for the view.

This particular query does a job that can be concisely defined, so that's good. (In fact some say that a block of code like this with a defining comment on top should almost certainly be a method of its own). I didn't find any method that does exactly this, but there's no reason why there shouldn't be. I think SourcePackage would be a good place.

(Also note that a count() on the query can be relatively expensive and deliver more information than you need. If all you need to know is whether there are any at all, use is_empty() instead.)

== Style ==

In lib/lp/bugs/browser/bugalsoaffects.py:

62 + # Look up the remote component if we have the necessary info
63 + #if data.get('add_packaging', False):
64 + comp_group = target.bugtracker.getRemoteComponentGroup(
65 + target.remote_product)

There's still a commented-out line in there.

In lib/lp/bugs/browser/tests/test_bugtracker_component.py: setUp() still creates a few objects that most of the tests don't actually care about.

Also in lib/lp/bugs/browser/tests/test_bugtracker_component.py, test_no_anonymous_edits:

353 + expected_html = """
354 + <h2>Components</h2>
355 + <ul>
356 + <li>
357 + <strong>alpha</strong>
358 + <ul>
359 + <li>
360 + ...

Unmerged revisions

9968. By Bryce Harrington on 2011-05-19

Review from jeroen: Move component/sourcepackage check to a new
member function of SourcePackage.

9967. By Bryce Harrington on 2011-05-18

Review from jeroen: Typo for "searched"?

9966. By Bryce Harrington on 2011-05-18

Review from jeroen: "Commented-out code is a bad thing."

9965. By Bryce Harrington on 2011-05-18

Review from jeroen: "Also note that a count() on the query can be
relatively expensive and deliver more information than you need. If all
you need to know is whether there are any at all, use is_empty() instead"

9964. By Bryce Harrington on 2011-04-25

Re-merge db-devel trunk

9963. By Bryce Harrington on 2011-04-25

Revert Jeroen's change for component form so editing components works.

In review, Jeroen noticed I had an ugly if clause to look up the source
package name field for the component edit form, and suggested replacing
it with a dict to look up the fields.

9962. By Bryce Harrington on 2011-04-25