Code review comment for lp:~edwin-grubbs/launchpad/bug-490593-configure-involvement-portlet

Hi Edwin.

I have a couple suggestion and questions that I think will take a few minutes
to resolve.

Using the UI:

    * "bug tracker" is two words.
    * Maybe "Configure support tracking" for answers.
    * We are asking users to Configure project development branch? I guess
      length is the issue here.

Code:

I see these warnings:

    == Pyflakes notices ==

    lib/lp/registry/browser/pillar.py
        28: 'IProductSeries' imported but unused

    lib/lp/registry/browser/product.py
        9: undefined name 'ProductInvolvementView' in __all__

> === modified file 'lib/lp/bugs/templates/bugtarget-bugs.pt'
> --- lib/lp/bugs/templates/bugtarget-bugs.pt 2010-03-04 15:09:10 +0000
> +++ lib/lp/bugs/templates/bugtarget-bugs.pt 2010-03-19 16:20:16 +0000
> @@ -169,9 +169,14 @@
> bug tracking.</strong></p>
>
> <p tal:condition="context/required:launchpad.Edit"
> - id="no-malone-edit">
> - <a tal:attributes="href string:${context/fmt:url/+edit}">Enable
> - bug tracking.</a>
> + id="no-malone-edit"
> + tal:define ="configure_bugtracker context/menu:overview/configure_bugtracker | nothing"
> + >
> + <a tal:condition="configure_bugtracker"
> + tal:replace="structure configure_bugtracker/fmt:link"/>
> + <a tal:condition="not: configure_bugtracker"
> + tal:attributes="href string:${context/fmt:url/+edit}">
> + Enable bug tracking.</a>

Why isn't this using the context's overview menu edit link that has
permission checking for rendering.

    context/menu:overview/edit:fmt:url

The language looks wrong. We are interested in Configuring, not enabling.
I think this is for the bugs root page which may be fine in this case.

> === modified file 'lib/lp/code/stories/branches/xx-product-branches.txt'
> --- lib/lp/code/stories/branches/xx-product-branches.txt 2010-01-08 14:22:42 +0000
> +++ lib/lp/code/stories/branches/xx-product-branches.txt 2010-03-19 16:20:16 +0000
> @@ -170,7 +170,7 @@
> the 'Import your project' button is not shown.
>
> >>> admin_browser.open('http://launchpad.dev/gnome-terminal')
> - >>> admin_browser.getLink('Change details').click()
> + >>> admin_browser.getLink('Configure Launchpad Branches').click()
> >>> admin_browser.getControl(
> ... 'Code for this project is published in Bazaar branches '
> ... 'on Launchpad').click()

I do not like the use of "Launchpad" in these links. They imply the project
is setting up hosting when we are really interested in gathering project info.

I have dealt with several angry, confused users who see Launchpad masquerading
or is forking project. I think taking "Launchpad" out of the link will make
it clear we want the know where the upstream branch is.

> === 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-19 16:20:16 +0000

> @@ -128,6 +130,78 @@
> 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'))
+

Are the "[]" required? sorted() accepts a generator which is what the first
argument provides.

> +# 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."""
> +
> + 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_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'))

Are the brackets required in this sorted()?

> === 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-19 16:20:16 +0000

> @@ -331,6 +337,30 @@
> return Link('+edit', text, icon='edit')
>
> @enabled_with_permission('launchpad.Edit')
> + def configure_bugtracker(self):
> + text = 'Configure project bugtracker'
> + summary = 'Allow users to report bugs on this project'
> + return Link('+configure-bugtracker', text, summary, icon='edit')

Our intent it to know where bugs are tracked, and possibly use launchpad.
The problem we have at this moment is that it is not possible to set the
upstream contact without also enabling launchpad bug tracking...making it
impossible for Ubuntu to who to contact when the project does not use
launchpad bugs.

> + @enabled_with_permission('launchpad.Edit')
> + def configure_answers(self):
> + text = 'Configure project answers'
> + summary = 'Allow users to ask questions on this project'
> + return Link('+configure-answers', text, summary, icon='edit')

Maybe we want to use the generic term "support tracker". Answers was called
Support and questions "tickets" in Launchpad beta.

> + @enabled_with_permission('launchpad.Edit')
> + def configure_blueprints(self):
> + text = 'Configure project blueprints'
> + summary = 'Allow users to submit blueprints on this project'
> + return Link('+configure-blueprints', text, summary, icon='edit')

Specifications and meetings tracking is what this enables.

> === modified file 'lib/lp/registry/stories/product/xx-product-launchpad-usage.txt'
> --- lib/lp/registry/stories/product/xx-product-launchpad-usage.txt 2010-01-20 23:10:13 +0000
> +++ lib/lp/registry/stories/product/xx-product-launchpad-usage.txt 2010-03-19 16:20:16 +0000
> @@ -28,20 +28,14 @@
> >>> registrant_browser = setupBrowser(
> ... auth='Basic <email address hidden>:test')
> >>> registrant_browser.open('http://launchpad.dev/firefox')
> - >>> registrant_browser.getLink('Change details').click()
> + >>> registrant_browser.getLink('Configure Launchpad Bugs').click()

I think users will provide the information is we use common terms:

    Configure bug tracking