Merge lp:~jcsackett/launchpad/unknown-blueprints-service-597738 into lp:launchpad

Sorry about the bad update. Just in case of divergences between the prereq and this post update from devel, here's the diff:

=== modified file 'lib/lp/blueprints/browser/configure.zcml'
--- lib/lp/blueprints/browser/configure.zcml 2010-07-27 17:17:59 +0000
+++ lib/lp/blueprints/browser/configure.zcml 2010-09-08 20:11:11 +0000
@@ -53,8 +53,7 @@
             name="+mdz-specs.csv"
             attribute="mdzCsv"/>
         <browser:page
- name="+specs"
- template="../templates/hasspecifications-specs.pt"/>
+ name="+specs"/>
         <browser:page
             name="+portlet-latestspecs"
             template="../templates/specificationtarget-portlet-latestspecs.pt"/>
@@ -500,8 +499,7 @@
             class="lp.blueprints.browser.specificationtarget.HasSpecificationsView"
             permission="zope.Public">
             <browser:page
- name="+specs"
- template="../templates/hasspecifications-specs.pt"/>
+ name="+specs"/>
         </browser:pages>
         <browser:page
             for="lp.blueprints.interfaces.specificationtarget.ISpecificationGoal"
@@ -532,8 +530,7 @@
             class="lp.blueprints.browser.specificationtarget.HasSpecificationsView"
             permission="zope.Public">
             <browser:page
- name="+specs"
- template="../templates/hasspecifications-specs.pt"/>
+ name="+specs"/>
             <browser:page
                 name="+portlet-latestspecs"
                 template="../templates/specificationtarget-portlet-latestspecs.pt"/>
@@ -583,8 +580,7 @@
         facet="specifications"
         permission="zope.Public">
         <browser:page
- name="+specs"
- template="../templates/hasspecifications-specs.pt"/>
+ name="+specs"/>
         <browser:page
             name="+portlet-latestspecs"
             template="../templates/specificationtarget-portlet-latestspecs.pt"/>

=== modified file 'lib/lp/blueprints/browser/specificationtarget.py'
--- lib/lp/blueprints/browser/specificationtarget.py 2010-08-24 10:45:57 +0000
+++ lib/lp/blueprints/browser/specificationtarget.py 2010-09-09 18:06:03 +0000
@@ -15,6 +15,7 @@

 from operator import itemgetter

+from z3c.ptcompat import ViewPageTemplateFile
 from zope.component import queryMultiAdapter

 from canonical.config import config
@@ -32,6 +33,8 @@
     Link,
     )
 from canonical.lazr.utils import smartquote
+from lp.app.enums import service_uses_launchpad
+from lp.app.interfaces.launchpad import IServiceUsage
 from lp.blueprints.interfaces.specification import (
     SpecificationFilter,
     SpecificationSort,
@@ -133,6 +136,50 @@
     is_sprint = False
     has_drivers = False

+ # Templates for the various conditions of blueprints:
+ # * On Launchpad
+ # * External
+ # * Disabled
+ # * Unknown
+ default_template = ViewPageTemplateFile(
+ '../templates/hasspecifications-specs.pt')
+ not_launchpad_template = ViewPageTemplateFile(
+ '../templates/unknown-specs.pt')
+
+ @property
+ def template(self):
+ # If the template has been defined in the zcml, use that, as t...

Hi Jon,

Thanks for working on this branch. I was unaware of the added
complexity of the whole template/page/portlet mess.

I like the work you've done here modulo the on-going discussion with
Gary about a better approach to the template swapping. I'm marking
the MP 'needs information' so I can track the change you come up with,
but the branch is very close.

You mentioned lint being confused by the dependency on another
branch. Even so, please ensure these are not for real:

./lib/lp/blueprints/browser/specificationtarget.py
     251: E301 expected 1 blank line, found 0
     254: E301 expected 1 blank line, found 0
./lib/lp/blueprints/templates/unknown-specs.pt
      38: Line has trailing whitespace.
./lib/lp/registry/doc/distribution.txt
     582: source exceeds 78 characters.
     584: source exceeds 78 characters.

> === modified file 'lib/lp/blueprints/browser/specificationtarget.py'
> --- lib/lp/blueprints/browser/specificationtarget.py 2010-08-24 10:45:57 +0000
> +++ lib/lp/blueprints/browser/specificationtarget.py 2010-09-10 14:48:00 +0000
> @@ -15,6 +15,7 @@
>
> from operator import itemgetter
>
> +from z3c.ptcompat import ViewPageTemplateFile
> from zope.component import queryMultiAdapter
>
> from canonical.config import config
> @@ -32,6 +33,8 @@
> Link,
> )
> from canonical.lazr.utils import smartquote
> +from lp.app.enums import service_uses_launchpad
> +from lp.app.interfaces.launchpad import IServiceUsage
> from lp.blueprints.interfaces.specification import (
> SpecificationFilter,
> SpecificationSort,
> @@ -133,6 +136,50 @@
> is_sprint = False
> has_drivers = False
>
> + # Templates for the various conditions of blueprints:
> + # * On Launchpad
> + # * External
> + # * Disabled
> + # * Unknown
> + default_template = ViewPageTemplateFile(
> + '../templates/hasspecifications-specs.pt')
> + not_launchpad_template = ViewPageTemplateFile(
> + '../templates/unknown-specs.pt')
> +
> + @property
> + def template(self):
> + # If the template has been defined in the zcml, use that, as this
> + # isn't the base service page for blueprints.
> + if hasattr(self, 'index'):

As we discussed on IRC, you should use 'safe_hasattr' from
canonical.lazr.utils. Also, as I suggested please check around and
see if there is a more obvious technique for this. Sadly I cannot
offer any other solution.

> + return super(HasSpecificationsView, self).template
> +
> + # Sprints and Persons don't have a usage enum for blueprints, so we
> + # have to fallback to the default.
> + if (ISprint.providedBy(self.context)
> + or IPerson.providedBy(self.context)):
> + return self.default_template
> +
> + # ProjectGroups are a special case, as their products may be a
> + # combination of usage settings. Use the default, since it handles
> + # fetching anything that's set for ProjectGroups, and it's not correct
> + # to say anything about the ProjectGroup's usage.
> + if IProjectGroup.providedBy(self.context):
> + return self.default_template
> +
> + # If specific...

On Fri, 2010-09-10 at 15:59 +0000, Brad Crittenden wrote:
> > + @property
> > + def template(self):
> > + # If the template has been defined in the zcml, use that,
> as this
> > + # isn't the base service page for blueprints.
> > + if hasattr(self, 'index'):
>
> As we discussed on IRC, you should use 'safe_hasattr' from
> canonical.lazr.utils. Also, as I suggested please check around and
> see if there is a more obvious technique for this. Sadly I cannot
> offer any other solution.

We can abandon safe_hasattr when lpnet is on lucid. This issue was fixed
several years ago in pythong 2.6

Good changes Jon. Please alphabetize the imports where you added safe_hasattr.

The UI changes look good to me, although it's confusing that the new template (lib/lp/blueprints/templates/unknown-specs.pt) uses the term "specification[s]" when the rest of the page uses "blueprint[s]". I think we should use only one of them, and most likely the preferred would be "blueprint[s]" as that's what is used in the top heading.

Gentleman, I think the reviewers and developers need to consider why we are making this change, and what we are really changing.

Our goal is to communicate to the user where certain kinds of activities are performed--which site does the user need to visit to complete his goal. Launchpad blueprints is a branded service that provides specifications and documentation. The page needs to be talking about "specifications and documentation" because other sites, most often wikis, do not use the term blueprints. We want to allow the users to provide a URL when they choose external (maybe repurpose the wiki field). The page is not really stating that Launchpad Blueprints allows the user to track specifications and documentation...I see no reason why I would turn the feature on since it does not have a sentence stating the basics of what it is for. Specification itself may be too technical. This app is essentially about planning and documenting features.

My other concern is about what has specifications--a SpecificationTarget. I reviewed the bugs branch a few days ago and saw only IProduct was supports well. A BugTarget may be a product, projectgroup, distribution, and distributionsourcepackage. I think these are also SpediciationTargets, so everyone needs to visit an example of each to ensure the message is correct and the actions that can be takes are presented. eg, a project group's use of an app is implicit in the settings of its products. I took a brief look at this branch in launchpad.dev and am pretty sure thunderbird is somewhat right, kubuntu, firefox and mozilla are wrong.

jcsackett, Salgado, take a look at the branch again from the perspective of kinds of users who are interested in feature planning.

I just discovered I missed comments within the diff; sorry for not replying on Friday.

> If the enum is wrong, how will the user get here? The tab will be
> disabled, so we assume they have a direct URL?

I'm not sure the tab will be disabled--if no blueprints existed, and the enum said "EXTERNAL" or something, you could still get to this view so the "${Pillar} does not use blueprints" could be displayed. In this instance if it's set to EXTERNAL but people have been using the blueprints, we continue to display the blueprints information when you get here.

The involvement menu link is disabled, but the tab along the top of the page is still there, and of course nothing stops someone from going to blueprints.launchpad.net/$PILLAR.

Now, I'm open to debate on whether we should just turn off the feature regardless of whether or not blueprints are already being used--I erred on the side of not allowing someone to just delete a bunch of work, but that might not be the decision we want to make here.

>> self.verify_involvment(context)
>
> You didn't introduce it, but please fix the typo of s/verify_involment/verify_involvement/

Uhm, not sure what you mean--looks like it already is verify_involvement?

>> def test_adaptable_to_specificationtarget(self):
>> @@ -87,6 +92,63 @@
>> '<div id="involvement" class="portlet involvement">' in view())
>>
>>
>> +class TestHasSpecificationsTemplates(TestCaseWithFactory):
>> + """Tests the selection of templates based on blueprints usage."""
>> +
>> + layer = DatabaseFunctionalLayer
>> +
>> + def setUp(self):
>> + super(TestHasSpecificationsTemplates, self).setUp()
>> + self.user = self.factory.makePerson()
>> + self.product = self.factory.makeProduct()
>> + self.naked_product = removeSecurityProxy(self.product)
>> + login_person(self.user)
>
> While I'm not a prude, I must ask why all of the nakedness here?
> Couldn't you just make self.user be the owner of the product and
> proceed fully clothed?

I wanted to test what a non-owner user has happen here; that may be the same thing as self.user, but it may not--I've been bitten by using owners/admins in the past.

> Why use 'specifications' just the once? The page says 'blueprints'
> six times and it is unclear that specifications and blueprints are the
> same from the context.
>
> Should we be more frank:
>
> <project> tracks specifications somewhere besides Launchpad, but we
> don't know where. You should check with the project maintainers for
> the location.
>
> I'm just throwing that out for thought. I'm not sure I even like the idea.

I'm playing with this right now related to the UI review; neither blueprints nor specifications are totally right for the copy, per Curtis's comments.

But consider your comments thrown into the mix.

> Thanks for the additional drive-by fixes.

I do what I can. :-P

On Mon, 2010-09-13 at 19:54 +0000, j.c.sackett wrote:
>
> The involvement menu link is disabled, but the tab along the top of
> the page is still there, and of course nothing stops someone from
> going to blueprints.launchpad.net/$PILLAR.
>
> Now, I'm open to debate on whether we should just turn off the feature
> regardless of whether or not blueprints are already being used--I
> erred on the side of not allowing someone to just delete a bunch of
> work, but that might not be the decision we want to make here.

The complaint from project owners is that users created blueprints on
Launchpad instead of the upstream wiki. The project owner can enable
blueprints if he chooses to. Blueprints should be off until enabled.

I went to firefox in lp.dev and saw blueprints on. As the owner of the
project I decided to turn blueprints off, so I choses the configure
link, and was surprised that Blueprints was off. More over, Saving the
state of unknown did not disable blueprints. The state of the app does
not work like answers. Remember, we are working on other applications to
ensure all apps have the same kind of behaviours.

--
__Curtis C. Hovey_________
http://launchpad.net/

> The complaint from project owners is that users created blueprints on
> Launchpad instead of the upstream wiki. The project owner can enable
> blueprints if he chooses to. Blueprints should be off until enabled.
>
> I went to firefox in lp.dev and saw blueprints on. As the owner of the
> project I decided to turn blueprints off, so I choses the configure
> link, and was surprised that Blueprints was off. More over, Saving the
> state of unknown did not disable blueprints. The state of the app does
> not work like answers. Remember, we are working on other applications to
> ensure all apps have the same kind of behaviours.

I can buy this argument, totally. I've removed the conditional that prioritizes the existence of blueprints over the status of the enum.

> Our goal is to communicate to the user where certain kinds of activities are
> performed--which site does the user need to visit to complete his goal.
> Launchpad blueprints is a branded service that provides specifications and
> documentation. The page needs to be talking about "specifications and
> documentation" because other sites, most often wikis, do not use the term
> blueprints.

Given this, I've updated the template to refer to the notion of documentation, and feature planning. In the instance where blueprints is set active, I've left the template alone, as the process of setting it up tells the user that Blueprints is a branded way that Launchpad does feature planning and documentation.

> We want to allow the users to provide a URL when they choose
> external (maybe repurpose the wiki field).

Based on our prior conversation I thought we were going to simply default to "There is a wiki for this project, which may be used for this purpose," with a link to the wiki if it exists, rather than introducing an extra db field to track an external specification tracking url.

I do think it may be worthwhile to circle back and add that field and get that information (for blueprints, answers, and translations) so we can provide a specific external place, but I think those changes are out of scope for this branch.

> The page is not really stating that
> Launchpad Blueprints allows the user to track specifications and
> documentation...I see no reason why I would turn the feature on since it does
> not have a sentence stating the basics of what it is for.

Agreed; I've updated what the page says to address this.

An incremental diff with these changes is attached.

I'll be addressing your other concerns re: specificationtarget next.

> My other concern is about what has specifications--a SpecificationTarget. I
> reviewed the bugs branch a few days ago and saw only IProduct was supports
> well.

Valid; I've done some digging and have some explanations below.

> A BugTarget may be a product, projectgroup, distribution, and
> distributionsourcepackage. I think these are also SpediciationTargets, so
> everyone needs to visit an example of each to ensure the message is correct
> and the actions that can be takes are presented. eg, a project group's use of
> an app is implicit in the settings of its products.

So, I think products are now handled right.

Project groups are tricky--right now I have it defaulting to the old style, so that any blueprints made for any project will be shown. Given how the earlier question of whether or not to continue showing blueprints if has_any_specifications was True, I think it is appropriate to only turn this on if at least one product has LAUNCHPAD as its usage setting.

Distributions were mostly handled right; the issue with say, Kubuntu was the configure_blueprints link, which I fixed.

DSPs aren't valid for blueprints; it's always NOT_APPLICABLE and the links are all disabled. You can't even get to it by going to blueprints.launchpad.net for the DSP in question; it takes you to the overview. (see https://blueprints.edge.launchpad.net/ubuntu/+source/firefox or https://blueprints.launchpad.dev/ubuntu/+source/mozilla-firefox)

Attached is a diff of this round of changes.

On Tue, 2010-09-14 at 19:16 +0000, j.c.sackett wrote:
> Project groups are tricky--right now I have it defaulting to the old
> style, so that any blueprints made for any project will be shown.
> Given how the earlier question of whether or not to continue showing
> blueprints if has_any_specifications was True, I think it is
> appropriate to only turn this on if at least one product has LAUNCHPAD
> as its usage setting.

I do not this this rule is right. PillarView that renders the
Involvement Portlet and the "Register a Blueprint" link only shows the
link on a project group if a sub project has enabled it. The involvement
portlet is controlled by the pillar owners and its links are enabled to
attack contributors to the applications the pillar uses.

We do not want the Involvement portlet to contradict the app page, which
is the reason we need to disable apps for unknown, not applicable, and
external (most of the time). Answers uses the involvement portlet to
decide the state to ensure it never contradicts. I expect all the apps
do do something similar so that we have a DRY implementation.

        involvement = getMultiAdapter(
            (self.context, self.request), name='+get-involved')
        if service_uses_launchpad(involvement.answers_usage):
            # enable the listing.

--
__Curtis C. Hovey_________
http://launchpad.net/

I like the multiAdapter strategy you showed and have put it in place of the for loop.

Hi Jon,

This looks good, and I only have a couple suggestions.

s/use// @ "[...] does not track use feature planning or documentation."

And for bonus points you could change the 'Configure blueprints' text according to the state the context is in. For example, in the UNKNOWN state, it could be changed to "Tell us how <project> tracks feature planning and documentation". Similarly, for the EXTERNAL (and maybe NOT_APPLICABLE as well?) state, it could be "Use Launchpad to track feature planning and documentation", although that's a bit of a lie as there are other things you can do by following that link (but then it should be a worthy tradeoff if our goal is to get people to use blueprints).

I think this is good to land, with the fixing of some link test. I suspect there are some other issues we will see when this feature is used with real data.

Thanks Salgado for seeing the problem in the enums. I see a similar issue with this link test: 'Enable tracking of specifications and meetings'. meetings, AKA sprints do not belong to projects or teams. They are always available because they are a cross-project/cross-team feature. The link is enabling the project to use Launcpads feature planning and documentation tracking features.

Doh. My comment was intended to be UI approve.