> Hi Edwin,
>
> This is nice, and it's good to see some XXXs go in the process. The
> view code with the Storm validator as a backstop is elegant.
>
> I've got a few questions and comments, so Needs Information for now.
>
> Cheers, Gavin.
>


Hi Gavin and Brad,

Thanks for the reviews. The incremental diff is in the previous comment.



> > === modified file 'lib/lp/registry/browser/product.py'
> > --- lib/lp/registry/browser/product.py        2010-04-14 17:16:56 +0000
> > +++ lib/lp/registry/browser/product.py        2010-04-21 11:03:32 +0000
> > @@ -1444,6 +1444,15 @@
> >          """See `LaunchpadFormView`."""
> >          self.validate_private_bugs(data)
> >
> > +        if data['active'] == False and self.context.active == True:
> > +            if len(self.context.sourcepackages) > 0:
> > +                self.setFieldError('active',
> > +                    structured(
> > +                        'This project cannot be deactivated since it is '
> > +                        'linked to '
> > +                        '<a href="%s/+packages">source packages</a>.',
> > +                        canonical_url(self.context)))
>
> You could change this to instead pass view_name='+packages' to
> canonical_url().


Fixed.


> It is worth showing this message before the user tries to deactivate
> the project? Perhaps a message next to the control, and the control
> disabled.


That's a nice idea, however, the validation would still be necessary for
race conditions when the form is displayed before the source package is
linked. Since deactivation will be done much more often by reviewers
than regular users, I don't know if it is worth it. I believe Curtis
still reviews most of the projects, so I'll ask him.


> Also, the text "This project cannot be deactivated since it is linked
> to source packages" doesn't sound quite right. How about "... linked
> to one or more source packages"?

Fixed.

> Perhaps it's worth getting a UI review?

Since I'm already talking to Curtis, and he's a UI reviewer, I'll ask him.


> > +
> >      @property
> >      def cancel_url(self):
> >          """See `LaunchpadFormView`."""
> > @@ -1491,6 +1500,15 @@
> >          # supervisor.
> >          self.validate_private_bugs(data)
> >
> > +        if data['active'] == False and self.context.active == True:
> > +            if len(self.context.sourcepackages) > 0:
> > +                self.setFieldError('active',
> > +                    structured(
> > +                        'This project cannot be deactivated since it is '
> > +                        'linked to '
> > +                        '<a href="%s/+packages">source packages</a>.',
> > +                        canonical_url(self.context)))
>
> Same here, re. view_name.
>
> ProductReviewLicenseView.validate() uses the same validation code as
> in ProductAdminView.validate(). Consider putting this in a mixin class
> that both ProductAdminView and ProductReviewLicenseView can use.


Done.


> > +
> >
> >  class ProductAddSeriesView(LaunchpadFormView):
> >      """A form to add new product series"""
> >
> > === modified file 'lib/lp/registry/tests/test_product.py'
> > --- lib/lp/registry/tests/test_product.py     2009-10-23 13:48:28 +0000
> > +++ lib/lp/registry/tests/test_product.py     2010-04-21 11:03:32 +0000
> > @@ -19,13 +19,39 @@
> >  from canonical.launchpad.ftests import syncUpdate
> >
> >  from lp.registry.interfaces.person import IPersonSet
> > -from lp.registry.interfaces.product import License
> > +from lp.registry.interfaces.product import IProductSet, License
> >  from lp.registry.model.product import Product
> >  from lp.registry.model.productlicense import ProductLicense
> >  from lp.registry.model.commercialsubscription import (
> >      CommercialSubscription)
> >  from lp.testing import TestCaseWithFactory
> >
> > +class TestProduct(TestCaseWithFactory):
> > +    """Tests product object."""
> > +
> > +    layer = LaunchpadFunctionalLayer
> > +
> > +    def test_deactivation_failure(self):
> > +        # Ensure that a product cannot be deactivated if
> > +        # it is linked to source packages.
> > +        login('<email address hidden>')
> > +        firefox = getUtility(IProductSet).getByName('firefox')
>
> For new tests we shouldn't really use sample data, or at least that's
> how I understand it. Is it reasonable to create new data here?


Fixed.


> > +        self.assertEqual(True, firefox.active)
> > +        self.failUnless(len(firefox.sourcepackages) > 0)
> > +        self.assertRaises(
> > +            AssertionError,
> > +            setattr, firefox, 'active', False)
> > +
> > +    def test_deactivation_success(self):
> > +        # Ensure that a product can be deactivated if
> > +        # it is not linked to source packages.
> > +        login('<email address hidden>')
> > +        product = self.factory.makeProduct()
> > +        self.assertEqual(True, product.active)
> > +        product.active = False
> > +        self.assertEqual(False, product.active)
> > +
> > +
> >  class TestProductFiles(unittest.TestCase):
> >      """Tests for downloadable product files."""
> >
> >
> > === modified file 'lib/lp/testing/__init__.py'
> > --- lib/lp/testing/__init__.py        2010-04-18 22:31:40 +0000
> > +++ lib/lp/testing/__init__.py        2010-04-21 11:03:32 +0000
> > @@ -31,6 +31,7 @@
> >      'TestCaseWithFactory',
> >      'test_tales',
> >      'time_counter',
> > +    'unlink_source_packages',
> >      # XXX: This really shouldn't be exported from here. People should
> import
> >      # it from Zope.
> >      'verifyObject',
> > @@ -83,6 +84,7 @@
> >  from canonical.launchpad.webapp.interfaces import ILaunchBag
> >  from canonical.launchpad.windmill.testing import constants
> >  from lp.codehosting.vfs import branch_id_to_path, get_multi_server
> > +from lp.registry.interfaces.packaging import IPackagingUtil
> >  # Import the login and logout functions here as it is a much better
> >  # place to import them from in tests.
> >  from lp.testing._login import (
> > @@ -930,3 +932,15 @@
> >                              name, mock_args, real_args))
> >                      else:
> >                          break
> > +
> > +def unlink_source_packages(product):
> > +    """Remove all links between the product and source packages.
> > +
> > +    A product cannot be deactivated if it is linked to source packages.
> > +    """
> > +    packaging_util = getUtility(IPackagingUtil)
> > +    for source_package in product.sourcepackages:
> > +        packaging_util.deletePackaging(
> > +            source_package.productseries,
> > +            source_package.sourcepackagename,
> > +            source_package.distroseries)
>
> Is this an appropriate place for this helper function?
> lp/testing/__init__.py is already a hodgepodge from all corners of
> Launchpad. Can this go somewhere more specific?


I don't see an existing file that makes sense for it, and I don't think
creating a new file is worth it unless I'm completely refactoring all
of __init__.py.

    factory.py
    fakemethod.py
    faketransaction.py
    __init__.py
    _login.py
    mail_helpers.py
    mail.py
    menu.py
    publication.py
    _tales.py
    views.py
    _webservice.py


> Hi Edwin,
>
> Thanks for making this fix...it sure had lots of tentacles.
>
>
> > === modified file 'lib/lp/answers/doc/person.txt'
> > --- lib/lp/answers/doc/person.txt     2010-02-05 21:25:23 +0000
> +++ lib/lp/answers/doc/person.txt       2010-04-20 20:36:22 +0000
> > @@ -351,6 +351,10 @@
> >  supported projects.
> >
> >      >>> login('<email address hidden>')
> > +
> > +    # A product cannot be deactivated if it is linked to source packages.
> 
> I think this comment would be easier to read in context if it were
> phrased positively, i.e.
> 
> # Unlink the source packages so the firefox project can be deactivated.
> 
> I see you've repeated the phrase many, many times in your changes.
> I'll leave it up to you to decide whether you want to make the wording
> change everywhere as it is only a minor improvement.


I just did a search and replace.


> > === modified file 'lib/lp/registry/browser/product.py'
> > --- lib/lp/registry/browser/product.py        2010-04-14 17:16:56 +0000
> > +++ lib/lp/registry/browser/product.py        2010-04-20 20:36:22 +0000
> > @@ -1444,6 +1444,15 @@
> >          """See `LaunchpadFormView`."""
> >          self.validate_private_bugs(data)
> >
> > +        if data['active'] == False and self.context.active == True:
> > +            if len(self.context.sourcepackages) > 0:
> > +                self.setFieldError('active',
> > +                    structured(
> > +                        'This project cannot be deactivated since it is '
> > +                        'linked to '
> > +                        '<a href="%s/+packages">source packages</a>.',
> > +                        canonical_url(self.context)))
> > +
> >      @property
> >      def cancel_url(self):
> >          """See `LaunchpadFormView`."""
> > @@ -1491,6 +1500,15 @@
> >          # supervisor.
> >          self.validate_private_bugs(data)
> >
> > +        if data['active'] == False and self.context.active == True:
> > +            if len(self.context.sourcepackages) > 0:
> > +                self.setFieldError('active',
> > +                    structured(
> > +                        'This project cannot be deactivated since it is '
> > +                        'linked to '
> > +                        '<a href="%s/+packages">source packages</a>.',
> > +                        canonical_url(self.context)))
>
> Couldn't those be refactored into a method?


Done.


> >
> >  class ProductAddSeriesView(LaunchpadFormView):
> >      """A form to add new product series"""
>
> > === modified file 'lib/lp/registry/browser/tests/product-views.txt'
> > --- lib/lp/registry/browser/tests/product-views.txt   2010-04-14 20:54:40
> +0000
> > +++ lib/lp/registry/browser/tests/product-views.txt   2010-04-20 20:36:22
> +0000
> > @@ -155,9 +155,27 @@
> >      ['license_reviewed', 'license_approved', 'active', 'private_bugs',
> >       'reviewer_whiteboard']
> >
> > +The reviewer cannot deactivate a project if it is linked
> > +to a source package.
> > +
> > +    >>> firefox.active
> > +    True
> > +
> > +    >>> form = {
> > +    ...     'field.active.used': '', # unchecked
> > +    ...     'field.reviewer_whiteboard': 'Looks bogus',
> > +    ...     'field.actions.change': 'Change',
> > +    ...     }
> > +    >>> view = create_initialized_view(
> > +    ...     firefox, name='+review-license', form=form)
> > +    >>> view.errors
> > +    [...This project cannot be deactivated since it is linked to
> > +    ...source packages</a>.']
> 
> So if I create a spam project all I have to do is link it to an Ubuntu
> package and I'll be invincible?  Perhaps the reviewer needs some UI
> magic that unlinks the source package and then deactivates the
> project.


Apparently, anyone can remove source package links.


> > === modified file 'lib/lp/registry/doc/product.txt'
> > --- lib/lp/registry/doc/product.txt   2010-04-09 16:19:25 +0000
> > +++ lib/lp/registry/doc/product.txt   2010-04-20 20:36:22 +0000
> > @@ -54,7 +54,7 @@
> >      >>> evo = p
> >
> >  To fetch a product we use IProductSet.getByName() or
> IProductSet.__getitem__.
> > -The former will, by default, reeturn active and inactive products, while
> the
> > +The former will, by default, return active and inactive products, while the
> 
> 'Reeturn' occurs one other place in the tree in doc/project.txt.  Maybe you
> could kill it
> too as a drive-by.


Done.

> >  later returns only active ones. Both can be used to look up products by
> their
> >  aliases, though.
> >