Code review comment for lp:~bac/launchpad/bug-490518-link-suggestion

..

=== modified file 'lib/lp/registry/browser/sourcepackage.py'
--- lib/lp/registry/browser/sourcepackage.py 2009-12-18 13:25:19 +0000
+++ lib/lp/registry/browser/sourcepackage.py 2010-01-25 23:25:27 +0000

...

> @@ -294,3 +301,49 @@
> """A View to show Answers help."""
>
> page_title = 'Help and support options'
> +
> +
> +class SourcePackageAssociationPortletView(LaunchpadEditFormView):

LaunchpadEditFormView is used to update the context object, which is not
the case here; it is creating Packaging object though an indirect method.
Why was this base class used instead of LaunchpadFormView? It is because
it can be used to update the Packaging object after it is set? If so,
the view will have to be registered on IPackaging, which does not exist in
this scenario.

> + """A view for linking to an upstream package."""
> +
> + schema = Interface
> + custom_widget(
> + 'upstream', LaunchpadRadioWidget, orientation='vertical')
> + product_suggestions = None
> +
> + def setUpFields(self):
> + """See `LaunchpadFormView`."""
> + super(SourcePackageAssociationPortletView, self).setUpFields()
> + # Find registered products that are similarly named to the source
> + # package.
> + product_vocab = getVocabularyRegistry().get(None, 'Product')
> + matches = product_vocab.searchForTerms(self.context.name)
> + # Based upon the matching products, create a new vocabulary with
> + # term descriptions that include a link to the product.
> + self.product_suggestions = []
> + vocab_terms = []
> + for item in matches:
> + product = item.value
> + self.product_suggestions.append(product)
> + item_url = canonical_url(product)
> + description = """<a href="%s">%s</a>""" % (
> + item_url, product.displayname)

I think this needs escaping. product.displayname may contain [?><].

> + vocab_terms.append(SimpleTerm(product, product.name, description))
> + upstream_vocabulary = SimpleVocabulary(vocab_terms)
> +
> + self.form_fields = Fields(
> + Choice(__name__='upstream',
> + title=_('Upstream project'),

I have been pondering cases where we need to be clear that we are showing
*registered* launchpad projects. Many of the confusing-ui issues is that users
do not understand that someone must register the project for it to be used.

Does the use of *registered* provide a clue that someone may need to register
the upstream project to complete the task?

> + default=None,
> + vocabulary=upstream_vocabulary,
> + #description=_("Select the upstream project"),

I suppose this is not needed.

> + required=True))
> +
> + @action('Link to Upstream Project', name='link')
> + def link(self, action, data):
> + upstream = data.get('upstream')
> + self.context.setPackaging(upstream.development_focus, self.user)
> + self.request.response.addInfoNotification(
> + 'The project %s was linked to this source package.' %
> + upstream.displayname)
> + self.next_url = self.request.getURL()

> === modified file 'lib/lp/registry/browser/tests/sourcepackage-views.txt'
> --- lib/lp/registry/browser/tests/sourcepackage-views.txt 2009-10-27 04:04:55 +0000
> +++ lib/lp/registry/browser/tests/sourcepackage-views.txt 2010-01-25 23:25:27 +0000
> @@ -1,6 +1,8 @@
> SourcePackage views
> ===================
>
> +Edit packaging view
> +-----------------------

I would have made the underline the same width as the heading.

...

@@ -96,3 +98,65 @@

     >>> print view.request.response.notifications
     []
+
+Upstream associations portlet
+-----------------------------
+
+The upstreams associations portlet either displays the upstream
+information if it is already set or gives the user the opportunity to
+suggest the association. The suggestion is based on a
+ProductVocabulary query using the source package name.
+
+Since the bonkers source project was associated previously with the
+bonkers project, the portlet will display that information.
+
+ >>> view = create_initialized_view(package, name='+portlet-associations')
+ >>> for product in view.product_suggestions:
+ ... print product.name
+ bonkers
+
+ >>> from canonical.launchpad.testing.pages import (
+ ... extract_text, find_tag_by_id)
+ >>> content = extract_text(find_tag_by_id(view.render(), 'upstreams'))
+ >>> print content
+ Project:...Bonkers...
+ Series:...crazy...
+
+A new source project that is not linked to an upstream will result in
+the portlet showing the suggested project.
+
+ >>> product = factory.makeProduct(name='lernid', displayname='Lernid')
+ >>> sourcepackagename = factory.makeSourcePackageName(name='lernid')
+ >>> package = factory.makeSourcePackage(
+ ... sourcepackagename=sourcepackagename, distroseries=distroseries)
+
+ >>> view = create_initialized_view(package, name='+portlet-associations')
+ >>> for product in view.product_suggestions:
+ ... print product.name
+ lernid
+
+ >>> content = extract_text(find_tag_by_id(view.render(), 'no-upstreams'))
+ >>> print content
+ Launchpad doesn&#8217;t know which project and series this
+ package belongs to. ...
+ Is the following project the upstream for this source package?
+ Upstream project:
+ Lernid...
+
+If there are multiple potential matches they are all shown.
+
+ >>> product = factory.makeProduct(name='lernid-dev', displayname='Lernid Dev')
+ >>> view = create_initialized_view(package, name='+portlet-associations')
+ >>> for product in view.product_suggestions:
+ ... print product.name
+ lernid
+ lernid-dev
+
+ >>> content = extract_text(find_tag_by_id(view.render(), 'no-upstreams'))
+ >>> print content
+ Launchpad doesn&#8217;t know which project and series this
+ package belongs to. ...
+ Is one of these projects the upstream for this source package?
+ Upstream project:
+ Lernid...
+ Lernid Dev...

...

> === modified file 'lib/lp/registry/templates/sourcepackage-index.pt'
> --- lib/lp/registry/templates/sourcepackage-index.pt 2009-12-03 16:54:29 +0000
> +++ lib/lp/registry/templates/sourcepackage-index.pt 2010-01-25 23:25:27 +0000

...

> + <tal:porlet replace="structure context/@@+portlet-associations" />

mis-spelling? See my next remark. I would use:

    <div class="portlet" content="structure context/@@+portlet-associations" />

> === added file 'lib/lp/registry/templates/sourcepackage-portlet-associations.pt'
> --- lib/lp/registry/templates/sourcepackage-portlet-associations.pt 1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/templates/sourcepackage-portlet-associations.pt 2010-01-25 23:25:27 +0000
> @@ -0,0 +1,86 @@
> +<tal:root
> + xmlns:tal="http://xml.zope.org/namespaces/tal"
> + xmlns:metal="http://xml.zope.org/namespaces/metal"
> + xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> + omit-tag="">
> +
> + <div class="portlet" id="portlet-associations">

While I do not think there is a issue with using the portlet class here, it
can present portability issues when if it is reused on other pages, perhaps
the DistributionSourcePackage. I think it is safer to define the class in the
callsite.

...

+ <tal:has_no_series condition="not: series">
+ <div id="no-upstreams" tal:condition="view/product_suggestions">
+ <p>
+ Launchpad doesn&#8217;t know which project and series this
+ package belongs to. Links from distribution packages to
+ upstream project let distribution and upstream
+ maintainers share bugs, patches, and translations
+ efficiently.
+ </p>
+ <div tal:condition="python: len(view.product_suggestions) > 1">
+ <b>Is one of these projects the upstream for this source
+ package?</b>
+ </div>
+ <div tal:condition="python: len(view.product_suggestions) == 1">
+ <b>Is the following project the upstream for this source package?</b>
+ </div>

I see my favourite plural problem has mutated. There must be some ironic god
at work hear because I have an unlanded branch that introduces a plural macro
that adds a suffix if the count is not 1. My solution will not work in this
case. I just want to keep these python hacks out of the code. I can certainly
modify my macro to support singular and plural phrases instead of suffixes.

We can collaborate on this. This is what I have queued to land in
base-layout-macros.pt.

<metal:plural define-macro="plural"><tal:comment condition="nothing">
  This macro requires the call site to define `count` in a containing
  tag or as a global. The plural `suffix` can also be defined by the
  containing tag or as a global, the default is 's'.
  </tal:comment><tal:if
    define="
      l_default string:s;
      l_suffix suffix | l_default;"
    condition="python: count != 1"
    replace="l_suffix" />
</metal:plural>
</macros>

...