Merge lp:~bac/launchpad/bug-618148 into lp:launchpad

[Brad Crittenden]

Make +configure-{answers,blueprints,translations} use the ServiceUsage enums rather than a Boolean.

= Summary =

The project configuration progress bar and configuration indicators are
currently based on the official_* booleans and usage of Launchpad apps
is controlled by a boolean. This branch changes the
+configure{answers|blueprints|translations} pages to use the
ServiceUsage enums for multi-state selection.

== Proposed fix ==

As above.

== Pre-implementation notes ==

Talks with Curtis and Jon.

== Implementation details ==

As above.

== Tests ==

bin/test -vvt xx-product-launchpad-usage.txt

== Demo and Q/A ==

Go to https://launchpad.dev/firefox and click on the four configure
links. For blueprints you must go to the app and then click on the
configure link.

= Launchpad lint =

I'll fix these now.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/stories/standalone/xx-product-translations.txt
  lib/lp/testing/sampledata.py
  lib/lp/registry/browser/product.py
  lib/lp/app/interfaces/launchpad.py
  lib/lp/translations/stories/translations/55-rosetta-potemplates.txt
  lib/lp/registry/stories/product/xx-product-launchpad-usage.txt
  lib/canonical/launchpad/pagetests/standalone/xx-form-layout.txt

lib/lp/translations/stories/translationgroups/46-test-distro-structured-permissions.txt

lib/lp/translations/stories/translationgroups/15-product-translation-group.txt

./lib/lp/translations/stories/standalone/xx-product-translations.txt
       1: narrative uses a moin header.
     169: narrative uses a moin header.
./lib/lp/app/interfaces/launchpad.py
      80: E231 missing whitespace after ','
./lib/lp/translations/stories/translations/55-rosetta-potemplates.txt
       1: narrative uses a moin header.
      70: narrative uses a moin header.
      98: narrative uses a moin header.
     109: source exceeds 78 characters.
     128: want exceeds 78 characters.
     140: want exceeds 78 characters.
     153: want exceeds 78 characters.
     166: want exceeds 78 characters.
./lib/lp/registry/stories/product/xx-product-launchpad-usage.txt
      63: source exceeds 78 characters.
     100: source exceeds 78 characters.

[Steve Kowalik (stevenk)]

Hi,

This looks like a well-thought out change, however, I have a few questions:

* You have lots of use of sampledata in the doctests changed, would you consider not using it?
* You have three classes that look exceedingly similar (ProductConfigureBlueprintsView and so on), can field_names and custom_widget be moved to the base class?

review: Needs Information (code*)

[Brad Crittenden (bac)]

Hi Steven,

Thanks for the review.

Your idea about refactoring the classes was a good one. It was a little more complicated due to the use of class variables and how custom_widget works but I did get it working and it'll be better long term. Oh, there is some extra funny business there too since the bug tracker set extends this class but then does some funniness.

Incremental diff at http://pastebin.ubuntu.com/483495/

As to sample data, you'll note all of the tests were just changes to existing tests that rely on sample data. I assiduously avoid sample data when constructing new tests but when only modifying old ones it often doesn't make sense to invest the time to rewrite the hole thing. This approach seems to be the one adopted by most teams.

[Brad Crittenden (bac)]

Thanks for your comments on IRC, Edwin.

Here is a diff to copy the field before modifying and to make 'field_names' a property. Thanks for the improvements.
http://pastebin.ubuntu.com/483575/

[Edwin Grubbs (edwin-grubbs)]

Brad, You changes look good.

Steve, I feel I should give you some feedback as a reviewer mentee. I think your review's comment about moving more of the common elements into the base class was very good. You should definitely take a look at Brad's latest changes using copy_field(), since the description modifications would have propagated to other forms if the field isn't copied. Of course, that's a hard one to catch. Eliminating sample data is also a tough call. If a pre-existing test only requires a few lines to be changed, and the branch isn't urgent, then it would be reasonable to force the drive-by changes to made immediately. So, I think your review was good, and you got to learn about some of the esoteric behavior of LaunchpadFormView.

review: Approve (code)

=== modified file 'lib/canonical/launchpad/pagetests/standalone/xx-form-layout.txt'
--- lib/canonical/launchpad/pagetests/standalone/xx-form-layout.txt	2010-06-24 02:21:13 +0000
+++ lib/canonical/launchpad/pagetests/standalone/xx-form-layout.txt	2010-08-25 18:34:45 +0000
@@ -96,25 +96,21 @@
 Checkbox widgets
 ----------------
 
-Checkboxes have their label to the right. One example is the checkbox for
-specifying whether your project uses Launchpad Translations.
+Checkboxes have their label to the right. Let's look at one example.
 
     >>> admin_browser.open(
-    ...     'http://launchpad.dev/evolution/+configure-translations')
-    >>> content = find_main_content(admin_browser.contents)
-
-    >>> print content
+    ...     'http://launchpad.dev/firefox/+review-license')
+    >>> print find_tag_by_id(admin_browser.contents, 'launchpad-form-widgets')
     <...
     <tr>
       <td colspan="2">
-        ...
-        <input ... name="field.official_rosetta" type="checkbox" ... />
-        <label for="field.official_rosetta">Translations...</label>...
+       <div>
+          <input ... name="field.license_reviewed" type="checkbox" ...  />
+          <label for="field.license_reviewed">Project reviewed</label>...
       </td>
     </tr>
     ...
 
-
 Rendering just the form content
 -------------------------------
 

=== modified file 'lib/lp/app/interfaces/launchpad.py'
--- lib/lp/app/interfaces/launchpad.py	2010-08-20 20:31:18 +0000
+++ lib/lp/app/interfaces/launchpad.py	2010-08-25 18:34:45 +0000
@@ -31,27 +31,27 @@
     # implies an actual location not an answer of "Launchpad, externally, or
     # neither."
     answers_usage = Choice(
-        title=_('Type of service for answers application.'),
+        title=_('Type of service for answers application'),
         description=_("Where does this pillar have an Answers forum?"),
         default=ServiceUsage.UNKNOWN,
         vocabulary=ServiceUsage)
     blueprints_usage = Choice(
-        title=_('Type of service for blueprints application.'),
+        title=_('Type of service for blueprints application'),
         description=_("Where does this pillar host blueprints?"),
         default=ServiceUsage.UNKNOWN,
         vocabulary=ServiceUsage)
     codehosting_usage = Choice(
-        title=_('Type of service for hosting code.'),
+        title=_('Type of service for hosting code'),
         description=_("Where does this pillar host code?"),
         default=ServiceUsage.UNKNOWN,
         vocabulary=ServiceUsage)
     translations_usage = Choice(
-        title=_('Type of service for translations application.'),
+        title=_('Type of service for translations application'),
         description=_("Where does this pillar do translations?"),
         default=ServiceUsage.UNKNOWN,
         vocabulary=ServiceUsage)
     bug_tracking_usage = Choice(
-        title=_('Type of service for tracking bugs.'),
+        title=_('Type of service for tracking bugs'),
         description=_("Where does this pillar track bugs?"),
         default=ServiceUsage.UNKNOWN,
         vocabulary=ServiceUsage)
@@ -77,7 +77,7 @@
         title=_('Translations for this project are done in Launchpad'),
         required=True)
     official_anything = Bool(
-        title=_('Uses Launchpad for something'),)
+        title=_('Uses Launchpad for something'))
     enable_bug_expiration = Bool(
         title=_('Expire "Incomplete" bug reports when they become inactive'),
         required=True)

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2010-08-24 10:45:23 +0000
+++ lib/lp/registry/browser/product.py	2010-08-25 18:34:45 +0000
@@ -52,6 +52,7 @@
 
 import pytz
 from z3c.ptcompat import ViewPageTemplateFile
+from zope.app.form import CustomWidgetFactory
 from zope.app.form.browser import (
     CheckBoxWidget,
     TextAreaWidget,
@@ -1399,6 +1400,28 @@
 class ProductConfigureBase(ReturnToReferrerMixin, LaunchpadEditFormView):
     implements(IProductEditMenu)
     schema = IProduct
+    usage_fieldname = None
+
+    def setUpFields(self):
+        super(ProductConfigureBase, self).setUpFields()
+        if self.usage_fieldname is not None:
+            # The usage fields are shared among pillars.  But when referring to
+            # an individual object in Launchpad it is better to call it by its
+            # real name, i.e. 'project' instead of 'pillar'.
+            usage_field = self.form_fields.get(self.usage_fieldname)
+            if usage_field:
+                usage_field.custom_widget = CustomWidgetFactory(
+                    LaunchpadRadioWidget, orientation='vertical')
+                # Copy the field or else the description in the interface will
+                # be modified in-place.
+                field = copy_field(usage_field.field)
+                field.description = (
+                    field.description.replace('pillar', 'project'))
+                usage_field.field = field
+
+    @property
+    def field_names(self):
+        return [self.usage_fieldname]
 
     @property
     def page_title(self):
@@ -1413,27 +1436,21 @@
     """View class to configure the Launchpad Blueprints for a project."""
 
     label = "Configure Blueprints"
-    field_names = [
-        "official_blueprints",
-        ]
+    usage_fieldname = 'blueprints_usage'
 
 
 class ProductConfigureTranslationsView(ProductConfigureBase):
     """View class to configure the Launchpad Translations for a project."""
 
     label = "Configure Translations"
-    field_names = [
-        "official_rosetta",
-        ]
+    usage_fieldname = 'translations_usage'
 
 
 class ProductConfigureAnswersView(ProductConfigureBase):
     """View class to configure the Launchpad Answers for a project."""
 
     label = "Configure Answers"
-    field_names = [
-        "official_answers",
-        ]
+    usage_fieldname = 'answers_usage'
 
 
 class ProductEditView(ProductLicenseMixin, LaunchpadEditFormView):

=== modified file 'lib/lp/registry/stories/product/xx-product-launchpad-usage.txt'
--- lib/lp/registry/stories/product/xx-product-launchpad-usage.txt	2010-06-14 20:12:43 +0000
+++ lib/lp/registry/stories/product/xx-product-launchpad-usage.txt	2010-08-25 18:34:45 +0000
@@ -23,10 +23,12 @@
       ...
     Unauthorized...
 
-so let's use Sample Person, the registrant of Firefox..
+Let's use Sample Person, the registrant of Firefox..
 
+    >>> from lp.testing.sampledata import SAMPLE_PERSON_EMAIL
     >>> registrant_browser = setupBrowser(
-    ...     auth='Basic [e-mail]:test')
+    ...     auth='Basic %(email)s:%(passwd)s' % dict(
+    ...         email=SAMPLE_PERSON_EMAIL, passwd='test'))
     >>> registrant_browser.open('http://launchpad.dev/firefox')
     >>> registrant_browser.getLink('Configure bug tracker').click()
     >>> registrant_browser.url
@@ -58,11 +60,14 @@
     >>> registrant_browser.getLink('Configure translations').click()
     >>> registrant_browser.url
     'http://launchpad.dev/firefox/+configure-translations'
-    >>> control = registrant_browser.getControl(
-    ...     'Translations for this project are done in Launchpad')
-    >>> control.selected
-    False
-    >>> control.selected = True
+    >>> print_radio_button_field(registrant_browser.contents,
+    ...                          "translations_usage")
+    (*) Unknown
+    ( ) Launchpad
+    ( ) External
+    ( ) Not Applicable
+
+    >>> registrant_browser.getControl('Launchpad').click()
     >>> registrant_browser.getControl('Change').click()
 
 Answers can be disabled.
@@ -71,11 +76,12 @@
     >>> registrant_browser.getLink('Configure support tracker').click()
     >>> registrant_browser.url
     'http://launchpad.dev/firefox/+configure-answers'
-    >>> control = registrant_browser.getControl(
-    ...     'People can ask questions in Launchpad Answers')
-    >>> control.selected
-    True
-    >>> control.selected = False
+    >>> print_radio_button_field(registrant_browser.contents, "answers_usage")
+    ( ) Unknown
+    (*) Launchpad
+    ( ) External
+    ( ) Not Applicable
+    >>> registrant_browser.getControl('Unknown').click()
     >>> registrant_browser.getControl('Change').click()
 
 On the product page, we can see that the product doesn't use any bug
@@ -85,6 +91,44 @@
     >>> print extract_text(uses)
     Uses Launchpad for: Branches and Translations.
 
+Blueprints can be enabled too, though it isn't easy.
+
+    >>> # First go to the Blueprints page.
+    >>> registrant_browser.open('http://launchpad.dev/firefox')
+    >>> registrant_browser.getLink('Blueprints').click()
+    >>> registrant_browser.getLink('Configure blueprints').click()
+    >>> registrant_browser.url
+    'http://blueprints.launchpad.dev/firefox/+configure-blueprints'
+    >>> print_radio_button_field(registrant_browser.contents,
+    ...                          "blueprints_usage")
+    (*) Unknown
+    ( ) Launchpad
+    ( ) External
+    ( ) Not Applicable
+    >>> registrant_browser.getControl('Launchpad').click()
+    >>> registrant_browser.getControl('Change').click()
+
+
+On the product page, we can see that the product doesn't use any bug
+tracker, not even Launchpad, but does use other apps.
+
+    >>> registrant_browser.open('http://launchpad.dev/firefox')
+    >>> uses = find_tag_by_id(registrant_browser.contents, id='uses')
+    >>> print extract_text(uses)
+    Uses Launchpad for: Blueprints, Branches, and Translations.
+
+Setting a usage to external is reflected in the "Uses" message.
+Change translations to external.
+
+    >>> registrant_browser.open('http://launchpad.dev/firefox')
+    >>> registrant_browser.getLink('Configure translations').click()
+    >>> registrant_browser.getControl('External').click()
+    >>> registrant_browser.getControl('Change').click()
+
+    >>> uses = find_tag_by_id(registrant_browser.contents, id='uses')
+    >>> print extract_text(uses)
+    Uses Launchpad for: Blueprints and Branches.
+
 
 Tracking bugs by email
 ----------------------

=== modified file 'lib/lp/testing/sampledata.py'
--- lib/lp/testing/sampledata.py	2010-08-13 21:30:24 +0000
+++ lib/lp/testing/sampledata.py	2010-08-25 18:34:45 +0000
@@ -13,6 +13,7 @@
     'CHROOT_LIBRARYFILEALIAS',
     'ADMIN_EMAIL',
     'COMMERCIAL_ADMIN_EMAIL',
+    'SAMPLE_PERSON_EMAIL',
     'HOARY_DISTROSERIES_NAME',
     'I386_ARCHITECTURE_NAME',
     'LAUNCHPAD_DBUSER_NAME',
@@ -43,7 +44,7 @@
 NO_PRIVILEGE_EMAIL = '[e-mail]'
 COMMERCIAL_ADMIN_EMAIL = '[e-mail]'
 ADMIN_EMAIL = '[e-mail]'
-
+SAMPLE_PERSON_EMAIL = '[e-mail]'
 # A user that is an admin of ubuntu-team, which has upload rights
 # to Ubuntu.
 UBUNTU_DEVELOPER_ADMIN_NAME = 'name16'

=== modified file 'lib/lp/translations/stories/standalone/xx-product-translations.txt'
--- lib/lp/translations/stories/standalone/xx-product-translations.txt	2010-03-31 20:25:33 +0000
+++ lib/lp/translations/stories/standalone/xx-product-translations.txt	2010-08-25 18:34:45 +0000
@@ -1,4 +1,6 @@
-= Product Translations =
+====================
+Product Translations
+====================
 
 Each product in Launchpad has a Translations page.
 
@@ -137,20 +139,17 @@
 
 If the netapplet project is updated to use Launchpad for translations...
 
-    >>> admin_browser.open('http://translations.launchpad.dev/netapplet')
-    >>> admin_browser.getLink('Overview').click()
-    >>> print admin_browser.title
-    Network Applet in Launchpad
+    >>> admin_browser.open('http://launchpad.dev/netapplet')
     >>> admin_browser.getLink('Configure translations').click()
-    >>> admin_browser.getControl(
-    ...     'Translations for this project are done in Launchpad').selected
-    False
-    >>> admin_browser.getControl(
-    ...     'Translations for this project are done in Launchpad'
-    ...     ).selected = True
+    >>> print_radio_button_field(admin_browser.contents, "translations_usage")
+    (*) Unknown
+    ( ) Launchpad
+    ( ) External
+    ( ) Not Applicable
+    >>> admin_browser.getControl('Launchpad').click()
     >>> admin_browser.getControl('Change').click()
 
-...there are no longer any obsolete entires.
+...there are no longer any obsolete entries.
 
     >>> admin_browser.getLink('Translations').click()
     >>> print admin_browser.title
@@ -169,7 +168,8 @@
     Language                   Untranslated    Unreviewed       Changed
 
 
-== Translation recommendation ==
+Translation recommendation
+==========================
 
 The page mentions which product series should be translated.
 

=== modified file 'lib/lp/translations/stories/translationgroups/15-product-translation-group.txt'
--- lib/lp/translations/stories/translationgroups/15-product-translation-group.txt	2010-03-31 20:25:33 +0000
+++ lib/lp/translations/stories/translationgroups/15-product-translation-group.txt	2010-08-25 18:34:45 +0000
@@ -15,9 +15,7 @@
     >>> print netapplet_owner_browser.title
     Configure Translations : NetApplet
 
-    >>> netapplet_owner_browser.getControl(
-    ...     'Translations for this project are done in Launchpad'
-    ...     ).selected = True
+    >>> netapplet_owner_browser.getControl('Launchpad').click()
     >>> netapplet_owner_browser.getControl('Change').click()
     >>> print netapplet_owner_browser.title
     Network Applet in Launchpad

=== modified file 'lib/lp/translations/stories/translationgroups/46-test-distro-structured-permissions.txt'
--- lib/lp/translations/stories/translationgroups/46-test-distro-structured-permissions.txt	2010-03-31 20:25:33 +0000
+++ lib/lp/translations/stories/translationgroups/46-test-distro-structured-permissions.txt	2010-08-25 18:34:45 +0000
@@ -1,11 +1,13 @@
-First, we state that netapplet is using Launchpad Translations.
+First, we verify that netapplet is using Launchpad Translations.
 
     >>> admin_browser.open('http://launchpad.dev/netapplet')
     >>> admin_browser.getLink('Configure translations').click()
-    >>> admin_browser.getControl(
-    ...     'Translations for this project are done in Launchpad'
-    ...     ).selected = True
-    >>> admin_browser.getControl('Change').click()
+    >>> print_radio_button_field(admin_browser.contents, "translations_usage")
+    ( ) Unknown
+    (*) Launchpad
+    ( ) External
+    ( ) Not Applicable
+    >>> admin_browser.getLink('Cancel').click()
     >>> print admin_browser.title
     Network Applet in Launchpad
 

=== modified file 'lib/lp/translations/stories/translations/55-rosetta-potemplates.txt'
--- lib/lp/translations/stories/translations/55-rosetta-potemplates.txt	2010-03-16 22:07:45 +0000
+++ lib/lp/translations/stories/translations/55-rosetta-potemplates.txt	2010-08-25 18:34:45 +0000
@@ -1,4 +1,6 @@
-== Legend and headers display ==
+==========================
+Legend and headers display
+==========================
 
 On template overview page, when user is not logged in (and there is no
 sufficient GeoIP data), or user has not set his preferred languages,
@@ -49,9 +51,7 @@
     # translations.
     >>> admin_browser.open(
     ...   'http://launchpad.dev/netapplet/+configure-translations')
-    >>> admin_browser.getControl(
-    ...     'Translations for this project are done in Launchpad'
-    ...     ).selected = True
+    >>> admin_browser.getControl('Launchpad').click()
     >>> admin_browser.getControl('Change').click()
     >>> anon_browser.open(
     ...     'http://translations.launchpad.dev/netapplet/trunk/+pots/'
@@ -69,7 +69,8 @@
     True
 
 
-= The PO Template Views =
+The PO Template Views
+=====================
 
 Carlos is a translator who translates things into Spanish and Catalan.
 When he looks at available translations for Evolution in Hoary, he
@@ -97,7 +98,8 @@
 
 So, everything is fine, and Carlos can sleep calmly.
 
-== Links to filtered pages ==
+Links to filtered pages
+=======================
 
 The POTemplate overview page shows different statistics: a count of
 untranslated messages, messages with new, unreviewed suggestions, and
@@ -108,8 +110,8 @@
 15 untranslated, 1 unreviewed and 1 changed in LP Spanish translations
 (all numbers repeated as hidden 'sortkey' values).
 
-    >>> browser.open('http://translations.launchpad.dev/ubuntu/hoary/+source/'+
-    ...              'evolution/+pots/evolution-2.2')
+    >>> browser.open('http://translations.launchpad.dev/ubuntu/hoary/'+
+    ...              '+source/evolution/+pots/evolution-2.2')
     >>> spanish_line = find_tag_by_id(browser.contents, 'evolution-2.2_es')
     >>> print extract_text(spanish_line)
     Spanish