Code review comment for lp:~edwin-grubbs/launchpad/bug-553384-deactivated-project-oops

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 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?

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

> The reviewer can deactivate a project if he concludes it is bogus.
>
> - >>> firefox.active
> + >>> product = factory.makeProduct(name='tomato', title='Tomato')
> + >>> product.active
> True
>
> >>> form = {
> @@ -166,12 +184,12 @@
> ... 'field.actions.change': 'Change',
> ... }
> >>> view = create_initialized_view(
> - ... firefox, name='+review-license', form=form)
> + ... product, name='+review-license', form=form)
> >>> view.errors
> []
> - >>> firefox.active
> + >>> product.active
> False
> - >>> print firefox.reviewer_whiteboard
> + >>> print product.reviewer_whiteboard
> Looks bogus
>
> The reviewer can enable privileged features like private bugs. He can
> @@ -439,8 +457,8 @@
> officially supports.
>
> >>> from canonical.launchpad.testing.pages import find_tag_by_id
> + >>> product = factory.makeProduct(name='tomato', title='Tomato')
>
> - >>> product = factory.makeProduct(name='tomato', title='Tomato')
> >>> owner = product.owner
> >>> login_person(owner)
> >>> question = factory.makeQuestion(target=product)

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

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