Merge lp:~sinzui/launchpad/project-packages-portlet-ui into lp:launchpad

[Curtis Hovey]

This is my branch to simplify the project package portlet suggestion form.

    lp:~sinzui/launchpad/project-packages-portlet-ui
    Diff size: 314
    Launchpad bug: https://bugs.launchpad.net/bugs/550642
                   https://bugs.launchpad.net/bugs/556534
    Test command: ./bin/test -vv \
        -t product-portlet-packages-view -t xx-product-index
    Pre-implementation: bac, james_w
    Target release: 10.04

Simplify the project package portlet suggestion form
--------------------------------------------------------------------

Bug #550642 [The form in +product-portlet-packages is shown to anonymous users]
    As an anonymous user, I was show the form to link /gdp to a sourcepackage.
    As an anonymous user, I should never be asked to complete a form. Note
    that the link to +ubuntupkg was not rendered because it does require
    launchpad.View, so I saw the form ending with the "or".

Bug #556534 [Suggestion to link to an Ubuntu package is repetitive]
    it says:

        [ Link to this Ubuntu Package ] or (+) Link to Ubuntu package

    which is a little repetitive and took me two readings to notice why there
    were two links.

    This problem must be solved because we need to add a third action to
    allow the user to state the project is not packaged in Ubuntu.

Rules
-----

    * Do not show the form to anonymous users.
    * Replace the link to +ubuntupkg with an option that states what will
      happen next: Choose another Ubuntu package
      * This allows us to add a "This is not packaged in Ubuntu"

UI
--

There are two presentations to accommodate when there are suggestions, and
when there are none.

    http://people.canonical.com/~curtis/project-with-package-suggestions.png
    http://people.canonical.com/~curtis/project-without-package-suggestions.png

QA
--

    * Visit https://edge.launchpad.net/gdp
    * Verify that several packages are listed, and the last option is
      "Choose another Ubuntu package"
    * Visit https://edge.launchpad.net/gedit-class-browser
    * Verify there is only one option listed
    * Select "Choose another Ubuntu package" and submit.
    * Verify you are on the +ubuntupkg form.

Lint
----

Linting changed files:
  lib/lp/registry/browser/product.py
  lib/lp/registry/browser/tests/product-portlet-packages-view.txt
  lib/lp/registry/stories/product/xx-product-index.txt
  lib/lp/registry/templates/product-portlet-packages.pt

Test
----

    * lib/lp/registry/browser/tests/product-portlet-packages-view.txt
      * Updated the test to reflect the new labels for the form.
      * Added a test to verify that anonymous users are not show the form.
      * Added a test to verify the form works.
      * Revised the test to show that selecting "Choose another Ubuntu
        package" redirects to +ubuntupkg. Removed the verification of the link
        to the form.
    * lib/lp/registry/stories/product/xx-product-index.txt
      * Updated the test to verify what the user sees.

Implementation
--------------

    * lib/lp/registry/browser/product.py
      * Updated can_show_portlet to return False when there is no user.
      * Refactored package_field_name so that the string was defined once.
        and updated the label to make it clear that the suggestions are
        from the current Ubuntu development series.
      * Added other_package to the vocabulary and made it the default value.
      * Updated the form action label to accommodate the case were the
        user has another step. This will also work for the case where the
        user needs to say the project is not packaged.
    * lib/lp/registry/templates/product-portlet-packages.pt
      * Removed the link to +ubuntupkg.
      * Moved the paragraph about no matches into the first form so that
        the second form could be removed.

[Gavin Panella (allenap)]

Nice :)

Log from IRC:

<allenap> sinzui: In http://people.canonical.com/~curtis/project-without-package-suggestions.png it might work even better if no radio buttons (or the title) appeared, just the button.
<sinzui> allenap, there will also be an option to say it is not packaged.
<sinzui> allenap, The previous UI proposal did have button...
<allenap> sinzui: Okay. Also, can_show_portlet() can return True if there are sourcepackages even when user is None. Is that right?
<sinzui> Correct, that is what you see now...https://edge.launchpad.net/gedit
<sinzui> The feature we are refining suggests packages when we do not know about them
<sinzui> There can be a list like this: https://edge.launchpad.net/gdp
<sinzui> allenap, https://edge.launchpad.net/gedit-class-browser shows just the link to choose another package, but the correct answer is Not packaged.
<sinzui> allenap, The UI to support a list or packages, choose another package, or not packaged became very complicated and abentley, james_w, bac, and I agreed that we should try a single list and button
<allenap> sinzui: Okay, that makes sense, thanks.
<allenap> sinzui: Line 43 in the diff, is it safe to use 'OTHER_PACKAGE' because package.name can never be upper-case? If so, can you add a tiny comment to explain.
<sinzui> hmm
<sinzui> You are correct, but I think I got lucky in my name. I did not think about it
<james_w> name can never have an underscore
<sinzui> allenap, I was cargo culting a style I have reviewed in the past when Object() was used to create a unique and use all-caps to define the field name
<sinzui> james_w, you meana source package name cannot contain underscores?
<james_w> correct
<james_w> for Debian/Ubuntu at least
<james_w> "Package names (both source and binary, see Package, Section 5.6.7) must consist only of lower case letters (a-z), digits (0-9), plus (+) and minus (-) signs, and periods (.). They must be at least two characters long and must start with an alphanumeric character."
<sinzui> ah yes, I recall someone wanted to create a gentoo package with an '/' in it

review: Approve

[Paul Hummer (rockstar)]

<sinzui> EdwinGrubbs, rockstar: can either you you comment on the UI of my branch https://code.edge.launchpad.net/~sinzui/launchpad/project-packages-portlet-ui/+merge/23360
<rockstar> sinzui, looking
<rockstar> sinzui, that's pretty straightforward. How many package suggestions can a project have?
<sinzui> That's a good question. that I do not know the answer to
<sinzui> I would say too much because I think I have seen 10 and I tuned out
<sinzui> rockstar, 20
<sinzui> rockstar, the number and test are easy to adjust
<rockstar> sinzui, I fear what it might look like with 10 different suggestions.
* sinzui think 10 is the limit
<sinzui> rockstar, no need to speculate: https://edge.launchpad.net/launchpad
<sinzui> We will alway have to an option to choose another package, or say it is not packaged. I think 8 is the most we should suggest
<rockstar> sinzui, I agree. Is there code to limit the suggestions currently?
<sinzui> yes, 20. The value and test are trivial fixes I can do now
* sinzui has them open
<rockstar> sinzui, great. With those changes, ui=me*

review: Approve (ui*)

[Michael Nelson (michael.nelson)]

Thanks Gavin and Paul for including all the info. My initial thought (similar to Gavin) that it seems strange having a single radio (http://people.canonical.com/~curtis/project-without-package-suggestions.png), but Curtis, you said:

<sinzui> allenap, there will also be an option to say it is not packaged.

Will this be part of this branch? (running locally still shows the single radio + submit button). If not, can we remove the "Ubuntu Hoary packages" title, radio and submit and replace it with the current link (just until the "Not packaged" option is present). I personally don't think it should land in this intermediate state... what do you think Curtis?

Great points Paul about the limit change.

review: Needs Information (ui)

[Curtis Hovey (sinzui)]

On Thu, 2010-04-15 at 10:28 +0000, Michael Nelson wrote:
> Review: Needs Information ui
> Thanks Gavin and Paul for including all the info. My initial thought
> (similar to Gavin) that it seems strange having a single radio
> (http://people.canonical.com/~curtis/project-without-package-suggestions.png), but Curtis, you said:
>
> <sinzui> allenap, there will also be an option to say it is not packaged.

This UI branch came from the branch that allows users to say "This is
not packaged in Ubuntu". There will always be two radio items. The
additional option will land Friday or Monday. The not-packaged branch
was blocked over the model implementation, so I decided to extract the
UI and apply the UI suggestions from abently, james_w, Edwin, and bac,

> Will this be part of this branch? (running locally still shows the
> single radio + submit button). If not, can we remove the "Ubuntu Hoary
> packages" title, radio and submit and replace it with the current link
> (just until the "Not packaged" option is present). I personally don't
> think it should land in this intermediate state... what do you think
> Curtis?

I really do not want to do this. I do not want to design something that
will exist for two days. This feature has a lock on it It will not be
released to lpnet until we land a branch that removed the !- lp_net.

The "Ubuntu Hoary packages" label was added from Edwin's UI review of
the not-packages branch. He was correct in pointing out that the
packages and the operation is about the current development series.

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

[Michael Nelson (michael.nelson)]

> On Thu, 2010-04-15 at 10:28 +0000, Michael Nelson wrote:
> > Review: Needs Information ui
> > Thanks Gavin and Paul for including all the info. My initial thought
> > (similar to Gavin) that it seems strange having a single radio
> > (http://people.canonical.com/~curtis/project-without-package-
> suggestions.png), but Curtis, you said:
> >
> > <sinzui> allenap, there will also be an option to say it is not packaged.
>
> This UI branch came from the branch that allows users to say "This is
> not packaged in Ubuntu". There will always be two radio items. The
> additional option will land Friday or Monday. The not-packaged branch
> was blocked over the model implementation, so I decided to extract the
> UI and apply the UI suggestions from abently, james_w, Edwin, and bac,

Great, that's all I needed to know :)

> The "Ubuntu Hoary packages" label was added from Edwin's UI review of
> the not-packages branch. He was correct in pointing out that the
> packages and the operation is about the current development series.

Yep, I think the label is great when there is a selection to make.

Thanks!

review: Approve (ui)

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2010-04-12 23:10:39 +0000
+++ lib/lp/registry/browser/product.py	2010-04-14 21:02:45 +0000
@@ -1041,10 +1041,12 @@
     """View class for product packaging portlet."""
 
     schema = Interface
+    package_field_name = 'distributionsourcepackage'
     custom_widget(
-        'distributionsourcepackage', LaunchpadRadioWidget,
-        orientation='vertical')
+        package_field_name, LaunchpadRadioWidget, orientation='vertical')
     suggestions = None
+    max_suggestions = 8
+    other_package = object()
 
     @cachedproperty
     def sourcepackages(self):
@@ -1058,7 +1060,17 @@
     @cachedproperty
     def can_show_portlet(self):
         """Are there packages, or can packages be suggested."""
-        return len(self.sourcepackages) > 0 or not config.launchpad.is_lpnet
+        if len(self.sourcepackages) > 0:
+            return True
+        if self.user is None or config.launchpad.is_lpnet:
+            return False
+        else:
+            return True
+
+    @property
+    def initial_values(self):
+        """See `LaunchpadFormView`."""
+        return {self.package_field_name: self.other_package}
 
     def setUpFields(self):
         """See `LaunchpadFormView`."""
@@ -1071,7 +1083,7 @@
         # term descriptions that include a link to the source package.
         self.suggestions = []
         vocab_terms = []
-        for package in distro_source_packages[:20]:
+        for package in distro_source_packages[:self.max_suggestions]:
             if package.development_version.currentrelease is not None:
                 self.suggestions.append(package)
                 item_url = canonical_url(package)
@@ -1079,20 +1091,31 @@
                     item_url, escape(package.name))
                 vocab_terms.append(
                     SimpleTerm(package, package.name, description))
+        # Add an option to represent the user's decision to choose a
+        # different package. Note that source packages cannot have uppercase
+        # names with underscores, so the name is safe to use.
+        description = 'Choose another Ubuntu package'
+        vocab_terms.append(
+            SimpleTerm(self.other_package, 'OTHER_PACKAGE', description))
         vocabulary = SimpleVocabulary(vocab_terms)
         self.form_fields = form.Fields(
-            Choice(__name__='distributionsourcepackage',
-                   title=_('Ubuntu packages'),
+            Choice(__name__=self.package_field_name,
+                   title=_('Ubuntu %s packages' %
+                           ubuntu.currentseries.displayname),
                    default=None,
                    vocabulary=vocabulary,
                    required=True))
 
-    @action(_('Link to this Ubuntu Package'), name='link')
+    @action(_('Set Ubuntu package information'), name='link')
     def link(self, action, data):
         product = self.context
-        dsp = data.get('distributionsourcepackage')
-        assert dsp is not None, "distributionsourcepackage was not specified"
+        dsp = data.get(self.package_field_name)
         product_series = product.development_focus
+        if dsp is self.other_package:
+            # The user wants to link an alternate package to this project.
+            self.next_url = canonical_url(
+                product_series, view_name="+ubuntupkg")
+            return
         ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
         product_series.setPackaging(ubuntu.currentseries,
                                     dsp.sourcepackagename,

=== modified file 'lib/lp/registry/browser/tests/product-portlet-packages-view.txt'
--- lib/lp/registry/browser/tests/product-portlet-packages-view.txt	2010-04-06 20:25:12 +0000
+++ lib/lp/registry/browser/tests/product-portlet-packages-view.txt	2010-04-14 21:02:45 +0000
@@ -27,7 +27,7 @@
     ...     transaction.commit()
     ...     reconnect_stores(config.statistician.dbuser)
     ...     ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
-    ...     updated = ubuntu.updateCompleteSourcePackageCache(
+    ...     ignore = ubuntu.updateCompleteSourcePackageCache(
     ...         archive=ubuntu.main_archive, log=logger, ztm=transaction)
     ...     transaction.commit()
     ...     reconnect_stores('launchpad')
@@ -39,7 +39,6 @@
     ...     login(ANONYMOUS)
     ...     return (ubuntu, product, spn)
 
-
 Let's create a test project.
 
     >>> product = factory.makeProduct(name="bingo")
@@ -63,8 +62,8 @@
     translations efficiently.
     There are no unlinked source packages that are a good match. Can you
     suggest one?
-    Link to Ubuntu package
-
+    Ubuntu Hoary packages:
+    Choose another Ubuntu package
 
 A distribution source package in a distribution other than ubuntu will
 not be suggested.
@@ -93,8 +92,9 @@
     >>> view = create_initialized_view(
     ...     product, name="+portlet-packages",
     ...     principal=product.owner)
-    >>> for dsp in view.suggestions:
-    ...     print dsp.name
+    >>> view.suggestions
+    []
+
 
 A source package that does NOT have a publishing history in the PENDING
 or PUBLISHED statuses will NOT be suggested.
@@ -111,7 +111,8 @@
     ...     print dsp.name
 
 A source package that does have a publishing history for the current
-Ubuntu series in the PUBLISHED status will be suggested.
+Ubuntu series will be suggested. The view as a distributionsourcepackage
+field.
 
     >>> spph = factory.makeSourcePackagePublishingHistory(
     ...     sourcepackagename=spn, distroseries=ubuntu.currentseries,
@@ -123,9 +124,14 @@
     ...     print dsp.name
     bingo
 
+    >>> for field in view.form_fields:
+    ...     print field.__name__
+    distributionsourcepackage
+
 And the user is presented with a form to select the distribution
 source package.
 
+    >>> login_person(product.owner)
     >>> content = find_tag_by_id(view.render(), 'portlet-packages')
 
     >>> print extract_text(content)
@@ -135,7 +141,7 @@
     provides. Links from distribution packages to upstream projects
     let distribution and upstream maintainers share bugs, patches, and
     translations efficiently.
-    Ubuntu packages:
+    Ubuntu Hoary packages:
     bingo...
 
 A distribution series with a matching name in the Ubuntu current
@@ -161,6 +167,7 @@
     >>> spph = factory.makeSourcePackagePublishingHistory(
     ...     sourcepackagename=spn, distroseries=ubuntu.currentseries)
     >>> (ubuntu, product, spn) = updateCache()
+    >>> login_person(product.owner)
     >>> view = create_initialized_view(
     ...     product, name="+portlet-packages",
     ...     principal=product.owner)
@@ -177,10 +184,15 @@
     provides. Links from distribution packages to upstream projects
     let distribution and upstream maintainers share bugs, patches, and
     translations efficiently.
-    Ubuntu packages:
+    Ubuntu Hoary packages:
     bingo
     ba-bingo...
 
+But the limit is 8 packages.
+
+    >>> view.max_suggestions
+    8
+
 If a package matches by name but is already linked to an Ubuntu
 package then it will not be shown as one of the suggestions.
 
@@ -198,6 +210,17 @@
     ...     print dsp.name
     bingo
 
+The view is not rendered when there are no source packages and the user is
+anonymous.
+
+    >>> login(ANONYMOUS)
+    >>> view = create_initialized_view(
+    ...     product, name="+portlet-packages", principal=product.owner)
+    >>> view.sourcepackages
+    []
+    >>> view.can_show_portlet
+    False
+
 The can_show_portlet property indicates that the portlet can be rendered. The
 portlet is not rendered if there are no source packages and the environment
 is lpnet.
@@ -205,6 +228,9 @@
     >>> config.launchpad.is_lpnet
     False
 
+    >>> login_person(product.owner)
+    >>> view = create_initialized_view(
+    ...     product, name="+portlet-packages", principal=product.owner)
     >>> view.sourcepackages
     []
 
@@ -233,15 +259,58 @@
 
     >>> ignore = config.pop('test_data')
 
+The +portlet-packages view can create a Packaging link.
+
+    >>> product.sourcepackages
+    []
+
+    >>> form = {
+    ...     'field.distributionsourcepackage': 'bingo',
+    ...     'field.actions.link': 'Set Ubuntu package information',
+    ...     }
+    >>> view = create_initialized_view(
+    ...     product, name="+portlet-packages", form=form)
+    >>> view.errors
+    []
+
+    >>> for sp in product.sourcepackages:
+    ...     print sp.name
+    bingo
+
+    >>> from lp.registry.interfaces.packaging import (
+    ...     IPackagingUtil, PackagingType)
+
+    >>> packaging_util = getUtility(IPackagingUtil)
+    >>> packaging_util.deletePackaging(
+    ...     product.development_focus,
+    ...     getUtility(ISourcePackageNameSet)['bingo'],
+    ...     ubuntu.currentseries)
+
+Choosing the other_package option issues a redirect to the +ubuntupkg. The
+option is the default value.
+
+    >>> view.initial_values['distributionsourcepackage'] is view.other_package
+    True
+
+    >>> form = {
+    ...     'field.distributionsourcepackage': 'OTHER_PACKAGE',
+    ...     'field.actions.link': 'Set Ubuntu package information',
+    ...     }
+    >>> view = create_initialized_view(
+    ...     product, name="+portlet-packages", form=form)
+    >>> view.errors
+    []
+
+    >>> product.sourcepackages
+    []
+
+    >>> print view.next_url
+    http://launchpad.dev/bingo/trunk/+ubuntupkg
+
 The view's sourcepackages property filters out obsolete packages and
 reverses the order so that the latest packages for the current ubuntu
 series are shown first.
 
-    >>> from lp.registry.interfaces.packaging import (
-    ...     IPackagingUtil, PackagingType)
-
-    >>> packaging_util = getUtility(IPackagingUtil)
-    >>> series = ubuntu.currentseries
     >>> spn = factory.makeSourcePackageName(name="a-obsolete-package")
     >>> packaging_util.createPackaging(
     ...     product.development_focus, spn, ubuntu.currentseries,

=== modified file 'lib/lp/registry/browser/tests/product-views.txt'
--- lib/lp/registry/browser/tests/product-views.txt	2010-04-12 17:16:10 +0000
+++ lib/lp/registry/browser/tests/product-views.txt	2010-04-14 21:02:45 +0000
@@ -506,7 +506,8 @@
     translations efficiently.
     There are no unlinked source packages that are a good match.
     Can you suggest one?
-    Link to Ubuntu package
+    Ubuntu Hoary packages:
+    Choose another Ubuntu package
 
     >>> spn = factory.makeSourcePackageName(name="bingo")
     >>> from canonical.launchpad.interfaces.launchpad import (

=== modified file 'lib/lp/registry/stories/product/xx-product-index.txt'
--- lib/lp/registry/stories/product/xx-product-index.txt	2010-03-26 16:12:55 +0000
+++ lib/lp/registry/stories/product/xx-product-index.txt	2010-04-14 21:02:45 +0000
@@ -373,10 +373,11 @@
     ...     find_tag_by_id(user_browser.contents, 'portlet-packages'))
     All packages...
     Packages in Ubuntu...
-    Ubuntu packages:
+    Ubuntu Hoary packages:
     pmount...
 
-    >>> user_browser.getControl(name='field.distributionsourcepackage').value = ['pmount']
-    >>> user_browser.getControl('Link to this Ubuntu Package').click()
+    >>> user_browser.getControl(
+    ...     name='field.distributionsourcepackage').value = ['pmount']
+    >>> user_browser.getControl('Set Ubuntu package information').click()
     >>> print_feedback_messages(user_browser.contents)
     This project was linked to the source package "pmount in ubuntu"

=== modified file 'lib/lp/registry/templates/product-portlet-packages.pt'
--- lib/lp/registry/templates/product-portlet-packages.pt	2010-03-26 16:12:55 +0000
+++ lib/lp/registry/templates/product-portlet-packages.pt	2010-04-14 21:02:45 +0000
@@ -43,26 +43,19 @@
       let distribution and upstream maintainers share bugs, patches, and
       translations efficiently.
     </p>
-    <div id="suggestions" tal:condition="view/suggestions">
+    <div id="suggestions">
+      <p tal:condition="not:view/suggestions">
+        There are no unlinked source packages that are a good match.
+        Can you suggest one?
+      </p>
+
       <div metal:use-macro="context/@@launchpad_form/form">
         <div class="actions" metal:fill-slot="buttons">
           <input tal:replace="structure view/link/render"/>
-              &nbsp;or&nbsp;
-              <a tal:replace="structure
-                              context/development_focus/menu:overview/ubuntupkg/fmt:link" />
         </div>
       </div>
     </div>
 
-    <div id="suggestions" tal:condition="not:view/suggestions">
-      <p>
-        There are no unlinked source packages that are a good match.
-        Can you suggest one?
-      </p>
-      <a tal:replace="structure
-                      context/development_focus/menu:overview/ubuntupkg/fmt:link" />
-    </div>
-
   </tal:has_no_packages>
 </div>
 </tal:root>