Merge lp:~abentley/launchpad/select-owner into lp:launchpad

[Aaron Bentley]

Suggest branch owner as recipe owner

= Summary =
Fix bug #670431: Make it easy to select branch owner as recipe owner when
creating recipe

== Proposed fix ==
Provide a list of suggested owners that includes the logged-in user and, if the
logged-in-user is a member of the branch owner, the branch owner. The normal
list of potential owners is provided as a secondary choice.

See: https://code.launchpad.net/~abentley/launchpad/select-owner

== Pre-implementation notes ==
None

== Implementation details ==
Extracted a SuggestionWidget from TargetBranchWidget, and implemented
RecipeOwnerWidget in terms of SuggestionWidget.

== Tests ==
bin/test -t test_create_new_recipe

== Demo and Q/A ==
Create a branch owned by a team you are a member of. Choose "Create a
packaging recipe". Both you and the owning team should be suggested as
owners.

Leave the team. Now only you should be suggested as an owner.

Create a recipe using a suggested owner.
Create a recipe using "Other".

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/branch.py
  lib/lp/code/browser/sourcepackagerecipe.py
  lib/lp/code/browser/tests/test_sourcepackagerecipe.py
  lib/canonical/widgets/suggestion.py

[Guilherme Salgado (salgado)]

Looks good to me. The only improvement I can think of would be to place the dropdown beside the 'Other' label rather than below it. It may not be worth the effort if it's too difficult to do so, though.

review: Approve (ui*)

[Aaron Bentley (abentley)]

Screenshot is here: salgado: http://people.canonical.com/~abentley/suggest-owner.png

[Curtis Hovey (sinzui)]

We discussed making the suggestion menu be subordinate to the (*) other radio item, but our attempt broke other things. I think this is a bigger form CSS issue that needs a separate fix. So this UI is good to land.

review: Approve (ui)

[Aaron Bentley (abentley)]

Actually, I was able to make the change such that only RecipeOwnerWidget was affected.

[Gavin Panella (allenap)]

Looks good! Some quite trivial comments.

[1]

+ @classmethod
+ def _generateSuggestionVocab(cls, context, full_vocabulary):

Any reason this is a class method? Not complaining, just wondering.

[2]

+ terms = [term for term in full_vocabulary
+ if term.value in suggestions]

Indentation is a bit off.

[3]

+ def _renderSuggestions(self):

This seems like an imperative name, "go and render the
suggestions". Perhaps _shouldRenderSuggestions()? Feel free to ignore
me too :)

[4]

In http://people.canonical.com/~abentley/suggest-owner.png the
drop-down box has "Foo Bar", but that's already a radio item. I think
it's confusing, or perhaps ugly, to repeat choices. That's more of a
UI thing though, and this is meant to be a code review :)

[5]

+ text = '%s (<a href="%s">branch details</a>)' % (
+ branch.displayname, canonical_url(branch))

Even if branch.displayname cannot be invalid, please cgi.escape() it,
same for the branch URL. If the implementation of displayname ever
changes it will continue to be safe.

[6]

+ return u'<label for="%s" style="font-weight: normal">%s</label>' % (
+ option_id, text)

Same as [6] for option_id.

[7]

+ def _autoselect_other(self):
...
+ def _get_suggestions(branch):

Other methods are camel-case, so these probably should be too.

review: Approve

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/11/2010 12:32 PM, Gavin Panella wrote:
> Review: Approve
> Looks good! Some quite trivial comments.
>
>
> [1]
>
> + @classmethod
> + def _generateSuggestionVocab(cls, context, full_vocabulary):
>
> Any reason this is a class method? Not complaining, just wondering.

It needs to be a method to support polymorphism. I think it doesn't
make sense for something to be an instance method when it doesn't use
the instance. It does use the class, so it can't be a staticmethod.

> [2]
>
> + terms = [term for term in full_vocabulary
> + if term.value in suggestions]
>
> Indentation is a bit off.

Fixed.

> [3]
>
> + def _renderSuggestions(self):
>
> This seems like an imperative name, "go and render the
> suggestions". Perhaps _shouldRenderSuggestions()? Feel free to ignore
> me too :)

Done.

> [4]
>
> In http://people.canonical.com/~abentley/suggest-owner.png the
> drop-down box has "Foo Bar", but that's already a radio item. I think
> it's confusing, or perhaps ugly, to repeat choices. That's more of a
> UI thing though, and this is meant to be a code review :)

OTOH, if you were looking for one of your teams in the drop-down, I
think it would be confusing if it wasn't there because it was a radio item.

> [5]
>
> + text = '%s (<a href="%s">branch details</a>)' % (
> + branch.displayname, canonical_url(branch))
>
> Even if branch.displayname cannot be invalid, please cgi.escape() it,
> same for the branch URL. If the implementation of displayname ever
> changes it will continue to be safe.

Done.

> [6]
>
> + return u'<label for="%s" style="font-weight: normal">%s</label>' % (
> + option_id, text)
>
> Same as [6] for option_id.

Done, by creating a new _optionID method for the whole thing.

> [7]
>
> + def _autoselect_other(self):
> ...
> + def _get_suggestions(branch):
>
> Other methods are camel-case, so these probably should be too.

Done. Also, _other_id => _otherId.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkzcMkgACgkQ0F+nu1YWqI34kgCgiA+4dDTTGg3Cl92uYx4C32gH
aeEAoIHKcHt4gbNMM2W+OGUuH/b2/OXN
=2JzN
-----END PGP SIGNATURE-----

=== renamed file 'lib/canonical/widgets/branch.py' => 'lib/canonical/widgets/suggestion.py'
--- lib/canonical/widgets/branch.py	2010-11-08 12:52:43 +0000
+++ lib/canonical/widgets/suggestion.py	2010-11-19 14:13:57 +0000
@@ -5,10 +5,13 @@
 
 __metaclass__ = type
 __all__ = [
+    'RecipeOwnerWidget',
     'TargetBranchWidget',
     ]
 
 
+import cgi
+
 from zope.app.form.browser.widget import renderElement
 from zope.app.form.interfaces import IInputWidget, InputErrors
 from zope.app.form.utility import setUpWidget
@@ -20,109 +23,91 @@
 from canonical.widgets.itemswidgets import LaunchpadRadioWidget
 
 
-class TargetBranchWidget(LaunchpadRadioWidget):
-    """Widget for selecting a target branch.
-
-    The default branch for a new branch merge proposal should be
-    the branch associated with the development focus series if there
-    is one (that isn't an import branch).
-
-    Also in the initial radio button selector are other branches for
-    the product that the branch owner has specified as target branches
-    for other merge proposals.
-
-    Finally there is an "other" button that gets the user to use the
-    normal branch selector.
+class SuggestionWidget(LaunchpadRadioWidget):
+    """Base class for widgets that suggest values.
+
+    Users can pick a suggested value from a radio button list, or use the
+    normal widget to pick any value they could normally pick.
     """
 
     def __init__(self, field, vocabulary, request):
         # Create the vocabulary and pass that to the radio button
         # constructor.
-        branch = field.context
-        self.branch_selector_vocab = self._generateTargetVocab(branch)
+        self.suggestion_vocab = self._generateSuggestionVocab(
+            field.context, vocabulary)
 
         LaunchpadRadioWidget.__init__(
-            self, field, self.branch_selector_vocab, request)
+            self, field, self.suggestion_vocab, request)
 
-        self.other_branch_widget = getMultiAdapter(
+        self.other_selection_widget = getMultiAdapter(
             (field, request), IInputWidget)
         setUpWidget(
-            self, 'other_branch', field, IInputWidget,
-            prefix=self.name, context=branch)
-
-        # If there are branches to show explicitly, then we want to be
-        # able to select the 'Other' selection item when someone types
-        # in any values.
-        branch_count = len(self.branch_selector_vocab)
-        if branch_count > 0:
-            on_key_press = "selectWidget('%s.%d', event);" % (
-                self.name, branch_count)
-            self.other_branch_widget.onKeyPress = on_key_press
-
-    def _generateTargetVocab(self, branch):
-        """Generate the vocabulary for the radio buttons.
-
-        The generated vocabulary contains the branch associated with the
-        development series of the product if there is one, and also any other
-        branches that the user has specified before as a target for a proposed
-        merge.
+            self, 'other_selection', field, IInputWidget,
+            prefix=self.name, context=field.context)
+
+        # If there are suggestions to show explicitly, then we want to select
+        # the 'Other' selection item when the user chooses a non-suggested
+        # value.
+        if self._shouldRenderSuggestions():
+            self._autoselectOther()
+
+    @classmethod
+    def _generateSuggestionVocab(cls, context, full_vocabulary):
+        """Generate a vocabulary for the suggestions.
+
+        :param context: The context object to generate suggestions for.
+        :param full_vocabulary: The vocabulary suggestions may be drawn from.
+            suggestions not present in this vocabulary are ignored.
         """
-        self.default_target = branch.target.default_merge_target
-        logged_in_user = getUtility(ILaunchBag).user
-        collection = branch.target.collection.targetedBy(logged_in_user)
-        collection = collection.visibleByUser(logged_in_user)
-        branches = collection.getBranches().config(distinct=True)
-        target_branches = list(branches.config(limit=5))
-        # If there is a development focus branch, make sure it is always
-        # shown, and as the first item.
-        if self.default_target is not None:
-            if self.default_target in target_branches:
-                target_branches.remove(self.default_target)
-            target_branches.insert(0, self.default_target)
-
-        # Make sure the source branch isn't in the target_branches.
-        if branch in target_branches:
-            target_branches.remove(branch)
-
-        terms = []
-        for branch in target_branches:
-            terms.append(SimpleTerm(
-                    branch, branch.unique_name))
-
+        suggestions = cls._getSuggestions(context)
+        terms = [
+            term for term in full_vocabulary
+            if term.value in suggestions
+            ]
         return SimpleVocabulary(terms)
 
+    def _shouldRenderSuggestions(self):
+        """Return True if suggestions should be rendered."""
+        return len(self.suggestion_vocab) > 0
+
+    def _optionId(self, index):
+        return '%s.%d' % (cgi.escape(self.name), index)
+
+    def _otherId(self):
+        """Return the id of the "Other" option."""
+        return self._optionId(len(self.suggestion_vocab))
+
     def _toFieldValue(self, form_value):
-        """Convert the form value into a branch.
+        """Convert the form value into the target value.
 
         If there were no radio button options, or 'other' was selected, then
-        get the value from the branch widget, otherwise get the branch
-        reference from the built up vocabulary.
+        get the value from the other_selection widget, otherwise get the
+        object reference from the built up vocabulary.
         """
-        if (len(self.branch_selector_vocab) == 0 or
-            form_value == "other"):
-            # Get the value from the branch selector widget.
+        if not self._shouldRenderSuggestions() or form_value == "other":
+            # Get the value from the other selector widget.
             try:
-                return self.other_branch_widget.getInputValue()
+                return self.other_selection_widget.getInputValue()
             except InputErrors:
-                self._error = self.other_branch_widget._error
+                self._error = self.other_selection_widget._error
                 raise
         else:
-            term = self.branch_selector_vocab.getTermByToken(form_value)
+            term = self.suggestion_vocab.getTermByToken(form_value)
             return term.value
 
     def hasInput(self):
         """Is there any input for the widget.
 
-        We need to defer the call to the other branch widget when either there
-        are no terms in the vocabulary or the other radio button was selected.
+        We need to defer the call to the other widget when either there are no
+        terms in the vocabulary or the other radio button was selected.
         """
-        if len(self.branch_selector_vocab) == 0:
-            return self.other_branch_widget.hasInput()
+        if not self._shouldRenderSuggestions():
+            return self.other_selection_widget.hasInput()
         else:
             has_input = LaunchpadRadioWidget.hasInput(self)
             if has_input:
                 if self._getFormInput() == "other":
-                    return self.other_branch_widget.hasInput()
+                    return self.other_selection_widget.hasInput()
             return has_input
 
     def getInputValue(self):
@@ -135,56 +120,46 @@
 
     def _renderLabel(self, text, index):
         """Render a label for the option with the specified index."""
-        option_id = '%s.%s' % (self.name, index)
         return u'<label for="%s" style="font-weight: normal">%s</label>' % (
-            option_id, text)
+            self._optionId(index), text)
 
-    def _renderBranchLabel(self, branch, index):
+    def _renderSuggestionLabel(self, value, index):
         """Render a label for the option based on a branch."""
-        option_id = '%s.%s' % (self.name, index)
+        return self._renderLabel(self._valueDisplayname(value), index)
 
-        # To aid usability there needs to be some text connected with the
-        # radio buttons that is not a hyperlink in order to select the radio
-        # button.  It was decided not to have the entire text as a link, but
-        # instead to have a separate link to the branch details.
-        text = '%s (<a href="%s">branch details</a>)' % (
-            branch.displayname, canonical_url(branch))
-        # If the branch is the development focus, say so.
-        if branch == self.default_target:
-            text = text + "&ndash; <em>development focus</em>"
-        return u'<label for="%s" style="font-weight: normal">%s</label>' % (
-            option_id, text)
+    @staticmethod
+    def _valueDisplayname(value):
+        """Return the displayname for a value."""
+        return value.displayname
 
     def renderItems(self, value):
         """Render the items for the selector."""
         field = self.context
-        product = field.context
         if value == self._missing:
             value = field.missing_value
 
         items = []
         index = 0
-        # Render each of the branches with the first selected.
-        for index, term in enumerate(self.branch_selector_vocab):
-            branch = term.value
+        # Render each of the suggestions with the first selected.
+        for index, term in enumerate(self.suggestion_vocab):
+            suggestion = term.value
             if index == 0:
                 renderfunc = self.renderSelectedItem
             else:
                 renderfunc = self.renderItem
-
+            text = self._renderSuggestionLabel(suggestion, index)
             render_args = dict(
-                index=index, text=self._renderBranchLabel(branch, index),
-                value=branch.unique_name, name=self.name,
-                cssClass=self.cssClass)
+                index=index, text=text, value=term.token,
+                name=self.name, cssClass=self.cssClass)
             items.append(renderfunc(**render_args))
 
         # Lastly render the other option.
         index = len(items)
-        other_branch_text = "%s %s" % (
+        other_selection_text = "%s %s" % (
             self._renderLabel("Other:", index),
-            self.other_branch_widget())
-        other_branch_onclick = (
-            "this.form['%s.target_branch'].focus()" % self.name)
+            self.other_selection_widget())
+        other_selection_onclick = (
+            "this.form['%s'].focus()" % self.other_selection_widget.name)
 
         elem = renderElement(u'input',
                              value="other",
@@ -192,9 +167,9 @@
                              id='%s.%s' % (self.name, index),
                              cssClass=self.cssClass,
                              type='radio',
-                             onClick=other_branch_onclick)
+                             onClick=other_selection_onclick)
 
-        other_radio_button = '%s&nbsp;%s' % (elem, other_branch_text)
+        other_radio_button = '%s&nbsp;%s' % (elem, other_selection_text)
 
         items.append(other_radio_button)
 
@@ -202,7 +177,98 @@
 
     def __call__(self):
         """Don't render the radio buttons if only one choice."""
-        if len(self.branch_selector_vocab) == 0:
-            return self.other_branch_widget()
+        if not self._shouldRenderSuggestions():
+            return self.other_selection_widget()
         else:
             return LaunchpadRadioWidget.__call__(self)
+
+
+class TargetBranchWidget(SuggestionWidget):
+    """Widget for selecting a target branch.
+
+    The default branch for a new branch merge proposal should be
+    the branch associated with the development focus series if there
+    is one (that isn't an import branch).
+
+    Also in the initial radio button selector are other branches for
+    the product that the branch owner has specified as target branches
+    for other merge proposals.
+
+    Finally there is an "other" button that gets the user to use the
+    normal branch selector.
+    """
+
+    @staticmethod
+    def _generateSuggestionVocab(branch, full_vocabulary):
+        """Generate the vocabulary for the radio buttons.
+
+        The generated vocabulary contains the branch associated with the
+        development series of the product if there is one, and also any other
+        branches that the user has specified before as a target for a proposed
+        merge.
+        """
+        default_target = branch.target.default_merge_target
+        logged_in_user = getUtility(ILaunchBag).user
+        collection = branch.target.collection.targetedBy(logged_in_user)
+        collection = collection.visibleByUser(logged_in_user)
+        branches = collection.getBranches().config(distinct=True)
+        target_branches = list(branches.config(limit=5))
+        # If there is a development focus branch, make sure it is always
+        # shown, and as the first item.
+        if default_target is not None:
+            if default_target in target_branches:
+                target_branches.remove(default_target)
+            target_branches.insert(0, default_target)
+
+        # Make sure the source branch isn't in the target_branches.
+        if branch in target_branches:
+            target_branches.remove(branch)
+
+        terms = []
+        for branch in target_branches:
+            terms.append(SimpleTerm(
+                    branch, branch.unique_name))
+
+        return SimpleVocabulary(terms)
+
+    def _renderSuggestionLabel(self, branch, index):
+        """Render a label for the option based on a branch."""
+        # To aid usability there needs to be some text connected with the
+        # radio buttons that is not a hyperlink in order to select the radio
+        # button.  It was decided not to have the entire text as a link, but
+        # instead to have a separate link to the branch details.
+        text = '%s (<a href="%s">branch details</a>)' % (
+            cgi.escape(branch.displayname), cgi.escape(canonical_url(branch)))
+        # If the branch is the development focus, say so.
+        if branch == self.context.context.target.default_merge_target:
+            text = text + "&ndash; <em>development focus</em>"
+        return u'<label for="%s" style="font-weight: normal">%s</label>' % (
+            self._optionId(index), text)
+
+    def _autoselectOther(self):
+        """Select "other" on keypress."""
+        on_key_press = "selectWidget('%s', event);" % self._otherId()
+        self.other_selection_widget.onKeyPress = on_key_press
+
+
+class RecipeOwnerWidget(SuggestionWidget):
+    """Widget for selecting a recipe owner.
+
+    The current user and the base branch owner are suggested.
+    """
+
+    @staticmethod
+    def _getSuggestions(branch):
+        """Suggest the branch owner and current user."""
+        logged_in_user = getUtility(ILaunchBag).user
+        return set([branch.owner, logged_in_user])
+
+    @staticmethod
+    def _valueDisplayname(value):
+        """Provide a specialized displayname for Persons"""
+        return value.unique_displayname
+
+    def _autoselectOther(self):
+        """Select "other" on click."""
+        on_click = "onClick=\"selectWidget('%s', event);\"" % self._otherId()
+        self.other_selection_widget.extra = on_click

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2010-11-11 11:55:53 +0000
+++ lib/lp/code/browser/branch.py	2010-11-19 14:13:57 +0000
@@ -102,7 +102,7 @@
 from canonical.launchpad.webapp.interfaces import ICanonicalUrlData
 from canonical.launchpad.webapp.menu import structured
 from canonical.lazr.utils import smartquote
-from canonical.widgets.branch import TargetBranchWidget
+from canonical.widgets.suggestion import TargetBranchWidget
 from canonical.widgets.itemswidgets import LaunchpadRadioWidgetWithDescription
 from canonical.widgets.lazrjs import vocabulary_to_choice_edit_items
 from lp.app.errors import NotFoundError

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2010-11-04 19:52:14 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2010-11-19 14:13:57 +0000
@@ -57,6 +57,7 @@
     )
 from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
+from canonical.widgets.suggestion import RecipeOwnerWidget
 from canonical.widgets.itemswidgets import LabeledMultiCheckBoxWidget
 from lp.code.errors import (
     BuildAlreadyPending,
@@ -315,6 +316,7 @@
 
     schema = ISourcePackageAddEditSchema
     custom_widget('distros', LabeledMultiCheckBoxWidget)
+    custom_widget('owner', RecipeOwnerWidget)
 
     def initialize(self):
         # XXX: rockstar: This should be removed when source package recipes
@@ -407,7 +409,6 @@
             self.form_fields = self.form_fields.omit('owner')
             self.form_fields = any_owner_field + self.form_fields
 
-
     @property
     def initial_values(self):
         return {

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-11-10 20:53:17 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2010-11-19 14:13:57 +0000
@@ -168,7 +168,7 @@
         browser.getLink('Create packaging recipe').click()
 
         # The options for the owner include the Good Chefs team.
-        options = browser.getControl('Owner').displayOptions
+        options = browser.getControl(name='field.owner.owner').displayOptions
         self.assertEquals(
             ['Good Chefs (good-chefs)', 'Master Chef (chef)'],
             sorted([str(option) for option in options]))
@@ -185,7 +185,9 @@
         browser.getControl('Description').value = 'Make some food!'
         browser.getControl('Secret Squirrel').click()
         browser.getControl('Automatically build each day').click()
-        browser.getControl('Owner').displayValue = ['Good Chefs']
+        browser.getControl('Other').click()
+        browser.getControl(name='field.owner.owner').displayValue = [
+            'Good Chefs']
         browser.getControl('Create Recipe').click()
 
         login(ANONYMOUS)
@@ -193,6 +195,33 @@
         self.assertEqual(team, recipe.owner)
         self.assertEqual('daily', recipe.name)
 
+    def test_create_new_recipe_suggests_user(self):
+        """The current user is suggested as a recipe owner, once."""
+        branch = self.factory.makeBranch(owner=self.chef)
+        text = self.getMainText(branch, '+new-recipe')
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            r'Owner: Master Chef \(chef\) Other:', text)
+
+    def test_create_new_recipe_suggests_user_team(self):
+        """If current user is a member of branch owner, it is suggested."""
+        team = self.factory.makeTeam(
+            name='branch-team', displayname='Branch Team',
+            members=[self.chef])
+        branch = self.factory.makeBranch(owner=team)
+        text = self.getMainText(branch, '+new-recipe')
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            r'Owner: Master Chef \(chef\)'
+            r' Branch Team \(branch-team\) Other:', text)
+
+    def test_create_new_recipe_ignores_non_user_team(self):
+        """If current user isn't a member of branch owner, it is ignored."""
+        team = self.factory.makeTeam(
+            name='branch-team', displayname='Branch Team')
+        branch = self.factory.makeBranch(owner=team)
+        text = self.getMainText(branch, '+new-recipe')
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            r'Owner: Master Chef \(chef\) Other:', text)
+
     def test_create_recipe_forbidden_instruction(self):
         # We don't allow the "run" instruction in our recipes.  Make sure this
         # is communicated to the user properly.
@@ -267,7 +296,8 @@
                 merge %(package_branch)s
                 ''' % {
                     'branch': branch.bzr_identity,
-                    'package_branch': package_branch.bzr_identity,}),
+                    'package_branch': package_branch.bzr_identity,
+                }),
             branch=branch)
         self.assertEqual(
             get_message_text(browser, 2),