Merge lp://staging/~edwin-grubbs/launchpad/bug-553384-deactivated-project-oops into lp://staging/launchpad

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.

> === 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-21 11:03:32 +0000
> @@ -351,6 +351,10 @@
> supported projects.
>
> >>> login('<email address hidden>')
> +
> + # A product cannot be deactivated if it is linked to source packages.
> + >>> from lp.testing import unlink_source_packages
> + >>> unlink_source_packages(firefox)
> >>> firefox.active = False
> >>> sorted(target.name
> ... for target in no_priv.getDirectAnswerQuestionTargets())
>
> === modified file 'lib/lp/blueprints/doc/specification.txt'
> --- lib/lp/blueprints/doc/specification.txt 2009-08-13 19:03:36 +0000
> +++ lib/lp/blueprints/doc/specification.txt 2010-04-21 11:03:32 +0000
> @@ -205,14 +205,18 @@
>
> Specs from inactive products are filtered out.
>
> - >>> from canonical.database.sqlbase import flush_database_updates
> - >>> login('<email address hidden>')
> - >>> upstream_firefox.active = False
> - >>> flush_database_updates()
> - >>> for spec in specset.specifications(filter=['install']):
> - ... print spec.name, spec.target.name
> - cluster-installation kubuntu
> - media-integrity-check ubuntu
> + >>> from canonical.database.sqlbase import flush_database_updates
> + >>> login('<email address hidden>')
> +
> + # A product cannot be deactivated if it is linked to source packages.
> + >>> from lp.testing import unlink_source_packages
> + >>> unlink_source_packages(upstream_firefox)
> + >>> upstream_firefox.active = False
> + >>> flush_database_updates()
> + >>> for spec in specset.specifications(filter=['install']):
> + ... print spec.name, spec.target.name
> + cluster-installation kubuntu
> + media-integrity-check ubuntu
>
>
> Reset firefox so we don't mess up later tests.
>
> === modified file 'lib/lp/blueprints/doc/sprint.txt'
> --- lib/lp/blueprints/doc/sprint.txt 2010-02-17 11:13:06 +0000
> +++ lib/lp/blueprints/doc/sprint.txt 2010-04-21 11:03:32 +0000
> @@ -151,14 +151,18 @@
>
> Inactive products are excluded from the listings.
>
> - >>> from canonical.launchpad.interfaces import IProductSet
> - >>> from canonical.launchpad.ftests import login
> - >>> firefox = getUtility(IProductSet).getByName('firefox')
> - >>> login("<email address hidden>")
> - >>> firefox.active = False
> - >>> flush_database_updates()
> - >>> ubz.specifications().count()
> - 0
> + >>> from canonical.launchpad.interfaces import IProductSet
> + >>> from canonical.launchpad.ftests import login
> + >>> firefox = getUtility(IProductSet).getByName('firefox')
> + >>> login("<email address hidden>")
> +
> + # A product cannot be deactivated if it is linked to source packages.
> + >>> from lp.testing import unlink_source_packages
> + >>> unlink_source_packages(firefox)
> + >>> firefox.active = False
> + >>> flush_database_updates()
> + >>> ubz.specifications().count()
> + 0
>
> ...

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.

> + >>> from lp.testing import unlink_source_packages
> + >>> unlink_source_packages(firefox)
> >>> firefox.active = False
> >>> sorted(target.name
> ... for target in no_priv.getDirectAnswerQuestionTargets())

> === modified file 'lib/lp/bugs/browser/bugalsoaffects.py'
> --- lib/lp/bugs/browser/bugalsoaffects.py 2010-01-15 03:32:46 +0000
> +++ lib/lp/bugs/browser/bugalsoaffects.py 2010-04-20 20:36:22 +0000
> @@ -103,26 +103,6 @@
> bugtask = self.context
> upstream = bugtask.target.upstream_product
> if upstream is not None:
> - if not upstream.active:
> - # XXX: Guilherme Salgado 2007-09-18 bug=140526: This is only
> - # possible because of bug 140526, which allows packages to
> - # be linked to inactive products.
> - series = bugtask.distribution.currentseries
> - assert series is not None, (
> - "This package is linked to a product series so this "
> - "package's distribution must have at least one distro "
> - "series.")
> - sourcepackage = series.getSourcePackage(
> - bugtask.sourcepackagename)
> - self.request.response.addWarningNotification(
> - structured(
> - _("""
> - This package is linked to an inactive upstream. You
> - can <a href="%(package_url)s/+edit-packaging">fix it</a>
> - to avoid this step in the future."""),
> - package_url=canonical_url(sourcepackage)))
> - return
> -

I'm really glad you found and removed these.

> try:
> valid_upstreamtask(bugtask.bug, upstream)
> except WidgetsError:

> === 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 dea...

=== modified file 'lib/lp/answers/doc/person.txt'
--- lib/lp/answers/doc/person.txt 2010-04-20 20:27:09 +0000
+++ lib/lp/answers/doc/person.txt 2010-04-21 16:05:41 +0000
@@ -352,7 +352,7 @@

     >>> login('<email address hidden>')

- # A product cannot be deactivated if it is linked to source packages.
+ # Unlink the source packages so the project can be deactivated.
     >>> from lp.testing import unlink_source_packages
     >>> unlink_source_packages(firefox)
     >>> firefox.active = False

=== modified file 'lib/lp/blueprints/doc/specification.txt'
--- lib/lp/blueprints/doc/specification.txt 2010-04-20 20:27:09 +0000
+++ lib/lp/blueprints/doc/specification.txt 2010-04-21 16:05:41 +0000
@@ -208,7 +208,7 @@
     >>> from canonical.database.sqlbase import flush_database_updates
     >>> login('<email address hidden>')

- # A product cannot be deactivated if it is linked to source packages.
+ # Unlink the source packages so the project can be deactivated.
     >>> from lp.testing import unlink_source_packages
     >>> unlink_source_packages(upstream_firefox)
     >>> upstream_firefox.active = False

=== modified file 'lib/lp/blueprints/doc/sprint.txt'
--- lib/lp/blueprints/doc/sprint.txt 2010-04-20 20:27:09 +0000
+++ lib/lp/blueprints/doc/sprint.txt 2010-04-21 16:05:41 +0000
@@ -156,7 +156,7 @@
     >>> firefox = getUtility(IProductSet).getByName('firefox')
     >>> login("<email address hidden>")

- # A product cannot be deactivated if it is linked to source packages.
+ # Unlink the source packages so the project can be deactivated.
     >>> from lp.testing import unlink_source_packages
     >>> unlink_source_packages(firefox)
     >>> firefox.active = False

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py 2010-04-19 20:03:36 +0000
+++ lib/lp/registry/browser/product.py 2010-04-21 14:34:21 +0000
@@ -1376,7 +1376,7 @@
         return self.next_url

-class EditPrivateBugsMixin:
+class ProductValidationMixin:

     def validate_private_bugs(self, data):
         """Perform validation for the private bugs setting."""
@@ -1387,8 +1387,20 @@
                     'for this project first.',
                     canonical_url(self.context, rootsite="bugs")))

-
-class ProductAdminView(ProductEditView, EditPrivateBugsMixin):
+ def validate_deactivation(self, data):
+ """Verify whether a product can be safely deactivated."""
+ 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 one or more '
+ '<a href="%s">source packages</a>.',
+ canonical_url(self.context, view_name='+packages')))
+
+
+class ProductAdminView(ProductEditView, ProductValidationMixin):
+ """View for $project/+admin"""
     label = "Administer project details"
     field_names = ["name", "owner", "active", "autoupdate", "private_bugs"]

@@ -1443,15 +1455,7 @@
     def validate(self, data):
         """See `La...

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

Thanks Edwin, looks good :)

Hello Edwin, et al.

Regarding who and how projects are deactivated...

Users cannot deactivate a project, reviewers and admins can. The rule of not deactivating a linked project seemed obvious 6 months ago and I have always deleted bad packaging links /before/ deactivating a project. It is clear from the oops this was not obvious to everyone so I reported a bug to stop other reviewers from doing the wrong thing.

I do not deactivate projects with legitimate links to Ubuntu. This has frustrated two users who think their desires outweigh the community. We *will not* deactivate any linked project because someone else will just have it reactivated.

This issue relates to series deletion. Users cannot delete a series that is linked to a package. The user can delete the packaing links. In the case of the sugar project, the user recreated links for the correct series.