> Hi Edwin.
> 
> I really appreciate your effort to solve this usability issue.
> 
> UI/Usability
> ------------
> 
> ! As the owner of thunderbird, I saw that the "Report a bug" was disabled. I
>   used "Configure Launchpad bugs" and choose The Mozilla.org bug tracker and
>   provided the remove project.
>   * I still see "Report a bug" (This service needs configuration). This is
>     wrong. I provided the real information that should satisfy other
>     contributors. I expect to see the warning disappear.

Hi Curtis,

Thanks for the review.

As we discussed on Skype, I will focus on code hosting now and fix the logic for displaying the bug tracker warning in another branch.

I have not fixed any of the unit tests for these recent changes, since I want to get your input on the UI changes first. You will notice that "Configure code hosting" switches between a plus-icon and an edit-icon, since I'm re-using the original menu item for +linkbranch. The plus-icon doesn't really make sense with the "configure" verb, but I think it is worth contemplating the use of a different icon to indicate unconfigured services.


> ! As the owner of Firefox and thunderbird, I expect to see a warning that
>   "Development focus branch has not been specified" which I can see in the
>   Project information portlet.
>   * I think this is a case where official_codehosting is a bogus attribute.
>     We know if the project officially uses code if the branch is neither a
>     mirror or import. I expect the Involvement portlet to tell me that this
>     is a problem.
>   * As an anonymous user I should see a warning that the development focus is
>     missing a branch in the Involvement portlet.
>   * May be call for action should also be repeated on the series page since
>     that is the object that is being edited.


I'm confused here, since a little further down you comment that the warning could be removed from the Project information portlet. Do you want a more noticeable warning message in the involvement portlet than a tooltip?



> ! As the user of evolution, a project that has a link to a package with
>   I expect to see a warning that translation syncing should be setup.
>   The owner certainly can set official/unofficial usage, but any user
>   can enable series sync, and should if Ubuntu has translations for it.
>   * This is a unique case were any user can set this. This information
>     must be shown on the series overview for any user to act on.
>   * We may want alternate configuration link or tool tips about the
>     problem.
>   * This is case is harder than the others because the official_translations
>     attribute is legitimate.


We agreed to handle translations later.



> ! I think the message for all these services is wrong. We want the user
>   to tell Launchpad where the service is and maybe configure it if the
>   service is Launchpad's. In the bug tracker example. we need to know the
>   bug tracker, and the owner wants to tell every one about the official
>   tracker; no one is asking users to enable Launchpad Bugs.


I tried improving the tooltip for the warnings.


> * I noticed that as an anonymous user or no-priv that the Involvement portlet
>   has about 1em extra padding in the bottom. Oh. The markup has an empty
>   <ul>; this is invalid HTML.


Fixed.


> * I Think the "Uses Launchpad for" item in the Project information portlet
>   may be redundant. We can discuss removing it after we are certain that this
>   change is all the information needed. I suspect we need to disable answers
>   and blueprints first since they are effectively on and it is not clear
>   that the involvement portlet is 100% related to official uses. Of course,
>   the involvement portlet is not just about official uses anymore either.
> 
> * Think the "Development focus branch has not been specified" link in
>   the Project information portlet may be removable if we solve the Involvement
>   portlet issues.


Removed.


> * Note to self, we may want to remove official_translations from distros.
>   Only Ubuntu can use translations. I recall there was a complaint from a
>   user that we cannot set a distro bug tracker.


Deferred to another branch.


> I see your configuration for the bug tracker is similar to my branch and I
> can drop some of my proposed changes. I think the sprint was very helpful
> in getting us in sync.
> 
> 
> Lint
> ----
> 
> lib/lp/registry/browser/product.py
>     9: undefined name 'ProductInvolvementView' in __all__
> 
> I have a few thoughts about the implementation. I think these are minor
> compared to the usability concerns.
> 
> ...
> 
> > === modified file 'lib/lp/registry/browser/pillar.py'
> > --- lib/lp/registry/browser/pillar.py 2010-02-17 11:19:42 +0000
> > +++ lib/lp/registry/browser/pillar.py 2010-03-17 16:31:13 +0000
> 
> ...
> 
> > @@ -131,3 +134,43 @@
> >
> >  provideAdapter(
> >      InvolvedMenu, [IInvolved], INavigationMenu, name="overview")
> > +
> > +
> > +# This class can't be moved into the browser/product.py file, since
> > +# the pillar-views.txt test will fail due to the MenuAPI adapter
> > +# for PillarView.enabled_links not working.
> > +class ProductInvolvementView(PillarView):
> > +    """Encourage configuration of involvement links for projects."""
> 
> I also think this view should be supporting ProductSeries or we need
> another subclass to provide that.


Added view for product series.


> 
> > === modified file 'lib/lp/registry/browser/product.py'
> > --- lib/lp/registry/browser/product.py        2010-03-09 00:46:37 +0000
> > +++ lib/lp/registry/browser/product.py        2010-03-17 16:31:13 +0000
> 
> ...
> 
> > @@ -331,6 +337,31 @@
> >          return Link('+edit', text, icon='edit')
> >
> >      @enabled_with_permission('launchpad.Edit')
> > +    def configure_bugtracker(self):
> > +        text = 'Configure Launchpad Bugs'
> > +        return Link('+configure-bugtracker', text, icon='edit')
> > +
> > +    @enabled_with_permission('launchpad.Edit')
> > +    def configure_branches(self):
> > +        text = 'Configure Launchpad Branches'
> > +        return Link('+configure-branches', text, icon='edit')
> > +
> > +    @enabled_with_permission('launchpad.Edit')
> > +    def configure_translations(self):
> > +        text = 'Configure Launchpad Translations'
> > +        return Link('+configure-translations', text, icon='edit')
> > +
> > +    @enabled_with_permission('launchpad.Edit')
> > +    def configure_answers(self):
> > +        text = 'Configure Launchpad Answers'
> > +        return Link('+configure-answers', text, icon='edit')
> > +
> > +    @enabled_with_permission('launchpad.Edit')
> > +    def configure_blueprints(self):
> > +        text = 'Configure Launchpad Blueprints'
> > +        return Link('+configure-blueprints', text, icon='edit')
> 
> I do not think "Launchpad" is relevant to the action. I think the summary
> attribute (tooltip) is need to elaborate on what the user can do.


I included my attempt to rename these. The most difficult one is Answers, since "Configure project answers" sounds like a FAQ and not the ability to ask questions. Maybe, it would be better as "Enable asking questions".



> ...
> 
> > @@ -473,11 +509,12 @@
> >          return Link('+securitycontact', text, icon='edit')
> >
> >
> > -class ProductSpecificationsMenu(NavigationMenu,
> > +class ProductSpecificationsMenu(NavigationMenu, ProductEditLinksMixin,
> >                                  HasSpecificationsMenuMixin):
> >      usedfor = IProduct
> >      facet = 'specifications'
> > -    links = ['listall', 'doc', 'assignments', 'new', 'register_sprint']
> > +    links = ['configure_blueprints', 'listall', 'doc', 'assignments',
> 'new',
> > +             'register_sprint']
> >
> >
> >  def _cmp_distros(a, b):
> > @@ -1128,6 +1165,79 @@
> >          return canonical_url(self.context)
> >
> >
> > +class ProductConfigureBase(ReturnToReferrerMixin, LaunchpadEditFormView):
> > +    implements(IProductEditMenu)
> > +    schema = IProduct
> > +
> > +    @property
> > +    def page_title(self):
> > +        return self.label
> > +
> > +    @action("Change", name='change')
> > +    def change_action(self, action, data):
> > +        self.updateContextFromData(data)
> > +
> > +
> > +class ProductConfigureBugTrackerView(ProductConfigureBase):
> > +    """View class to configure the bug tracker for a project."""
> > +
> > +    label = "Configure bug tracker"
> > +    field_names = [
> > +        "bugtracker",
> > +        "enable_bug_expiration",
> > +        "remote_product",
> > +        "bug_reporting_guidelines",
> > +        ]
> > +    custom_widget('bugtracker', ProductBugTrackerWidget)
> > +
> > +    def validate(self, data):
> > +        """Constrain bug expiration to Launchpad Bugs tracker."""
> > +        # enable_bug_expiration is disabled by JavaScript when bugtracker
> > +        # is not 'In Launchpad'. The contraint is enforced here in case the
> > +        # JavaScript fails to activate or run. Note that the bugtracker
> > +        # name : values are {'In Launchpad' : object, 'Somewhere else' :
> None
> > +        # 'In a registered bug tracker' : IBugTracker}.
> > +        bugtracker = data.get('bugtracker', None)
> > +        if bugtracker is None or IBugTracker.providedBy(bugtracker):
> > +            data['enable_bug_expiration'] = False
> 
> grammar: s/contraint/constraint/


Fixed.


> /me thinks he misspelled the word 2.5 years ago.
> 
> > +class ProductConfigureBranchesView(ProductConfigureBase):
> > +    """View class to configure the Launchpad Branches for a project."""
> > +
> > +    label = "Configure Launchpad Branches"
> > +    field_names = [
> > +        "official_codehosting",
> > +        ]
> 
> This is a problem. We want this to go to series/+setbranch. Brad's branch
> will provide an alternate name to configure code. I think we need to
> reconcile this link and what we actually need users to do.



I won't land this branch until Brad's branch is ready. For now, I have pointed "Configure code hosting" at $product/$devfocus/+linkbranch.


> ...
> 
> > +class ProductConfigureTranslationsView(ProductConfigureBase):
> > +    """View class to configure the Launchpad Translations for a project."""
> > +
> > +    label = "Configure Launchpad Translations"
> > +    field_names = [
> > +        "official_rosetta",
> > +        ]
> 
> This does not actually configure translations, but it improves the existing
> behaviour. I think we need to work with the translations team to understand
> how we can get the required information into a single form.
>
> ...
> 
> > === modified file 'lib/lp/registry/templates/pillar-involvement-portlet.pt'
> > --- lib/lp/registry/templates/pillar-involvement-portlet.pt   2009-08-15
> 05:35:13 +0000
> > +++ lib/lp/registry/templates/pillar-involvement-portlet.pt   2010-03-17
> 16:31:13 +0000
> > @@ -1,13 +1,40 @@
> >  <div
> >    xmlns:tal="http://xml.zope.org/namespaces/tal"
> >    xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> > -  id="involvement" class="portlet involvement"
> > -  tal:condition="view/has_involvement">
> > +  id="involvement" class="portlet"
> > +  tal:condition="view/has_involvement"
> > +  >
> >    <h2>Get Involved</h2>
> >
> > -  <ul>
> > +  <ul class="involvement">
> >      <li tal:repeat="link view/enabled_links">
> >        <a tal:replace="structure link/fmt:link" />
> >      </li>
> > +
> > +    <tal:disabled repeat="link view/visible_disabled_links">
> > +      <li tal:condition="not: link/enabled"
> > +          title="This service needs to be configured."
> > +          class="disabled">
> > +        <div style="padding: 0.3em 0 0.3em 0.3em">
> > +          <div style="display: inline-block"
> > +               class="sprite icon-only warning-icon"></div>
> > +
> > +          <div style="height: 1.2em; float: right">
> > +            <img src="/@@/gray-arrow-right.png"
> > +                style="top: 50%; bottom: 50%; margin-top: -5px;
> > +                       position: relative;
> > +                       height: 9px; width: 14px; padding: 0"/>
> > +          </div>
> > +
> > +          <span tal:content="link/text"/>
> > +        </div>
> > +      </li>
> > +    </tal:disabled>
> > +  </ul>
> > +
> > +  <ul style="padding-top: 1em">
> > +    <li tal:repeat="link view/configuration_links">
> > +      <a tal:replace="structure link/fmt:link" />
> > +    </li>
> >    </ul>
> >  </div>
> 
> I think this last url needs something like:
>     tal:condition="view/configuration_links"

Fixed.


Incremental diff:
=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2010-03-16 22:07:45 +0000
+++ lib/lp/registry/browser/configure.zcml	2010-03-19 15:25:16 +0000
@@ -502,6 +502,13 @@
         facet="overview"
         permission="zope.Public"
         template="../templates/pillar-involvement-portlet.pt"/>
+    <browser:page
+        name="+get-involved"
+        for="lp.registry.interfaces.product.IProductSeries"
+        class="lp.registry.browser.pillar.ProductSeriesInvolvementView"
+        facet="overview"
+        permission="zope.Public"
+        template="../templates/pillar-involvement-portlet.pt"/>
     <facet
         facet="overview">
         <browser:url

=== modified file 'lib/lp/registry/browser/pillar.py'
--- lib/lp/registry/browser/pillar.py	2010-03-17 04:01:37 +0000
+++ lib/lp/registry/browser/pillar.py	2010-03-19 15:44:33 +0000
@@ -15,6 +15,7 @@
 from zope.component.globalregistry import provideAdapter
 from zope.interface import implements, Interface
 
+from canonical.cachedproperty import cachedproperty
 from canonical.launchpad.webapp.interfaces import INavigationMenu
 from canonical.launchpad.webapp.menu import Link, NavigationMenu
 from canonical.launchpad.webapp.publisher import LaunchpadView, nearest
@@ -70,7 +71,7 @@
     implements(IInvolved)
 
     configuration_links = []
-    visible_disabled_links = []
+    visible_disabled_link_names = []
 
     def __init__(self, context, request):
         super(PillarView, self).__init__(context, request)
@@ -88,9 +89,7 @@
             self.official_codehosting = False
         else:
             self._set_official_launchpad(pillar)
-            if IProductSeries.providedBy(self.context):
-                self.official_answers = False
-            elif IDistroSeries.providedBy(self.context):
+            if IDistroSeries.providedBy(self.context):
                 self.official_answers = False
                 self.official_codehosting = False
             elif IDistributionSourcePackage.providedBy(self.context):
@@ -131,6 +130,24 @@
             link for link in menuapi.navigation.values() if link.enabled],
             key=attrgetter('sort_key'))
 
+    @cachedproperty
+    def visible_disabled_links(self):
+        """Important disabled links.
+
+        These are displayed to notify the user to provide configuration
+        info to enable the links.
+
+        Override the visible_disabled_link_names attribute to change
+        the results.
+        """
+        involved_menu = MenuAPI(self).navigation
+        important_links = [
+            involved_menu[name]
+            for name in self.visible_disabled_link_names]
+        return sorted([
+            link for link in important_links if not link.enabled],
+            key=attrgetter('sort_key'))
+
 
 provideAdapter(
     InvolvedMenu, [IInvolved], INavigationMenu, name="overview")
@@ -143,34 +160,48 @@
     """Encourage configuration of involvement links for projects."""
 
     has_involvement = True
+    visible_disabled_link_names = ['submit_code']
+
+    def __init__(self, context, request):
+        super(ProductInvolvementView, self).__init__(context, request)
+        self.official_codehosting = (
+            self.context.development_focus.branch is not None)
 
     @property
     def configuration_links(self):
         """The enabled involvement links."""
         overview_menu = MenuAPI(self.context).overview
+        series_menu = MenuAPI(self.context.development_focus).overview
         configuration_names = [
             'configure_answers',
-            'configure_branches',
             'configure_bugtracker',
             'configure_translations',
             ]
         configuration_links = [
             overview_menu[name] for name in configuration_names]
+        link_branch = series_menu['link_branch']
+        link_branch.text = 'Configure project branch'
+        configuration_links.append(link_branch)
         return sorted([
             link for link in configuration_links if link.enabled],
             key=attrgetter('sort_key'))
 
+
+class ProductSeriesInvolvementView(PillarView):
+    """Encourage configuration of involvement links for project series."""
+
+    has_involvement = True
+    visible_disabled_link_names = ['submit_code']
+
+    def __init__(self, context, request):
+        super(ProductSeriesInvolvementView, self).__init__(context, request)
+        self.official_codehosting = self.context.branch is not None
+        self.official_answers = False
+
     @property
-    def visible_disabled_links(self):
-        """Important disabled links.
-
-        These are displayed to notify the user to provide configuration
-        info to enable the links.
-        """
-        involved_menu = MenuAPI(self).navigation
-        important_links = [
-            involved_menu[name]
-            for name in ['report_bug']]
-        return sorted([
-            link for link in important_links if not link.enabled],
-            key=attrgetter('sort_key'))
+    def configuration_links(self):
+        """The enabled involvement links."""
+        series_menu = MenuAPI(self.context).overview
+        link_branch = series_menu['link_branch']
+        link_branch.text = 'Configure series branch'
+        return [link_branch]

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2010-03-16 22:07:45 +0000
+++ lib/lp/registry/browser/product.py	2010-03-19 15:33:53 +0000
@@ -338,28 +338,27 @@
 
     @enabled_with_permission('launchpad.Edit')
     def configure_bugtracker(self):
-        text = 'Configure Launchpad Bugs'
-        return Link('+configure-bugtracker', text, icon='edit')
-
-    @enabled_with_permission('launchpad.Edit')
-    def configure_branches(self):
-        text = 'Configure Launchpad Branches'
-        return Link('+configure-branches', text, icon='edit')
+        text = 'Configure project bugtracker'
+        summary = 'Allow users to report bugs on this project'
+        return Link('+configure-bugtracker', text, summary, icon='edit')
 
     @enabled_with_permission('launchpad.Edit')
     def configure_translations(self):
-        text = 'Configure Launchpad Translations'
-        return Link('+configure-translations', text, icon='edit')
+        text = 'Configure project translations'
+        summary = 'Allow users to submit translations for this project'
+        return Link('+configure-translations', text, summary, icon='edit')
 
     @enabled_with_permission('launchpad.Edit')
     def configure_answers(self):
-        text = 'Configure Launchpad Answers'
-        return Link('+configure-answers', text, icon='edit')
+        text = 'Configure project answers'
+        summary = 'Allow users to ask questions on this project'
+        return Link('+configure-answers', text, summary, icon='edit')
 
     @enabled_with_permission('launchpad.Edit')
     def configure_blueprints(self):
-        text = 'Configure Launchpad Blueprints'
-        return Link('+configure-blueprints', text, icon='edit')
+        text = 'Configure project blueprints'
+        summary = 'Allow users to submit blueprints on this project'
+        return Link('+configure-blueprints', text, summary, icon='edit')
 
     @enabled_with_permission('launchpad.Edit')
     def branding(self):
@@ -407,7 +406,6 @@
         'edit',
         'configure_answers',
         'configure_blueprints',
-        'configure_branches',
         'configure_bugtracker',
         'configure_translations',
         'reassign',
@@ -1193,7 +1191,7 @@
     def validate(self, data):
         """Constrain bug expiration to Launchpad Bugs tracker."""
         # enable_bug_expiration is disabled by JavaScript when bugtracker
-        # is not 'In Launchpad'. The contraint is enforced here in case the
+        # is not 'In Launchpad'. The constraint is enforced here in case the
         # JavaScript fails to activate or run. Note that the bugtracker
         # name : values are {'In Launchpad' : object, 'Somewhere else' : None
         # 'In a registered bug tracker' : IBugTracker}.

=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py	2010-03-09 22:06:12 +0000
+++ lib/lp/registry/browser/productseries.py	2010-03-19 03:24:09 +0000
@@ -74,7 +74,8 @@
 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
 from canonical.launchpad.webapp.interfaces import NotFoundError
 from canonical.launchpad.webapp.launchpadform import (
-    action, custom_widget, LaunchpadEditFormView, LaunchpadFormView)
+    action, custom_widget, LaunchpadEditFormView, LaunchpadFormView,
+    ReturnToReferrerMixin)
 from canonical.launchpad.webapp.menu import structured
 from canonical.widgets.textwidgets import StrippedTextWidget
 
@@ -644,7 +645,8 @@
         self.next_url = canonical_url(product)
 
 
-class ProductSeriesLinkBranchView(LaunchpadEditFormView):
+class ProductSeriesLinkBranchView(ReturnToReferrerMixin,
+                                  LaunchpadEditFormView):
     """View to set the bazaar branch for a product series."""
 
     schema = IProductSeries
@@ -661,11 +663,6 @@
         """The page title."""
         return self.label
 
-    @property
-    def next_url(self):
-        """See `LaunchpadFormView`."""
-        return canonical_url(self.context)
-
     @action(_('Update'), name='update')
     def update_action(self, action, data):
         """Update the branch attribute."""
@@ -679,11 +676,6 @@
         self.request.response.addInfoNotification(
             'Series code location updated.')
 
-    @property
-    def cancel_url(self):
-        """See `LaunchpadFormView`."""
-        return canonical_url(self.context)
-
 
 class ProductSeriesLinkBranchFromCodeView(ProductSeriesLinkBranchView):
     """Set the branch link from the code overview page."""

=== modified file 'lib/lp/registry/templates/pillar-involvement-portlet.pt'
--- lib/lp/registry/templates/pillar-involvement-portlet.pt	2010-03-15 23:32:07 +0000
+++ lib/lp/registry/templates/pillar-involvement-portlet.pt	2010-03-19 16:04:11 +0000
@@ -13,7 +13,7 @@
 
     <tal:disabled repeat="link view/visible_disabled_links">
       <li tal:condition="not: link/enabled"
-          title="This service needs to be configured."
+          tal:attributes="title string:Launchpad needs to know where the user can ${link/text/lower}"
           class="disabled">
         <div style="padding: 0.3em 0 0.3em 0.3em">
           <div style="display: inline-block"
@@ -32,7 +32,7 @@
     </tal:disabled>
   </ul>
 
-  <ul style="padding-top: 1em">
+  <ul tal:condition="view/configuration_links" style="padding-top: 1em">
     <li tal:repeat="link view/configuration_links">
       <a tal:replace="structure link/fmt:link" />
     </li>

=== modified file 'lib/lp/registry/templates/product-index.pt'
--- lib/lp/registry/templates/product-index.pt	2009-11-18 04:51:08 +0000
+++ lib/lp/registry/templates/product-index.pt	2010-03-19 15:03:53 +0000
@@ -116,10 +116,6 @@
                     <br/>
                     <a tal:replace="structure trunk/menu:context/source/fmt:link"/>
                   </p>
-                  <p tal:condition="not: trunk_visible">
-                    Development focus branch has not been specified
-                    <a tal:replace="structure dev_focus/menu:overview/link_branch/fmt:icon" />
-                  </p>
                 </dd>
               </dl>