launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01897
[Merge] lp:~abentley/launchpad/select-owner into lp:launchpad/devel
Aaron Bentley has proposed merging lp:~abentley/launchpad/select-owner into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#670431 Make it easy to select branch owner as recipe owner when creating recipe
https://bugs.launchpad.net/bugs/670431
= 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
--
https://code.launchpad.net/~abentley/launchpad/select-owner/+merge/40556
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/select-owner into lp:launchpad/devel.
=== 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-10 17:12:46 +0000
@@ -5,6 +5,7 @@
__metaclass__ = type
__all__ = [
+ 'RecipeOwnerWidget',
'TargetBranchWidget',
]
@@ -20,109 +21,86 @@
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._renderSuggestions():
+ self._autoselect_other()
+
+ @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._get_suggestions(context)
+ terms = [term for term in full_vocabulary
+ if term.value in suggestions]
return SimpleVocabulary(terms)
+ def _renderSuggestions(self):
+ """Return True if suggestions should be rendered."""
+ return len(self.suggestion_vocab) > 0
+
+ def _other_id(self):
+ """Return the id of the "Other" option."""
+ return '%s.%d' % (self.name, 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._renderSuggestions() 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._renderSuggestions():
+ 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):
@@ -139,52 +117,43 @@
return u'<label for="%s" style="font-weight: normal">%s</label>' % (
option_id, 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 + "– <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=suggestion.name,
+ 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 +161,9 @@
id='%s.%s' % (self.name, index),
cssClass=self.cssClass,
type='radio',
- onClick=other_branch_onclick)
+ onClick=other_selection_onclick)
- other_radio_button = '%s %s' % (elem, other_branch_text)
+ other_radio_button = '%s %s' % (elem, other_selection_text)
items.append(other_radio_button)
@@ -202,7 +171,100 @@
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._renderSuggestions():
+ 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."""
+ option_id = '%s.%s' % (self.name, 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.context.context.target.default_merge_target:
+ text = text + "– <em>development focus</em>"
+ return u'<label for="%s" style="font-weight: normal">%s</label>' % (
+ option_id, text)
+
+ def _autoselect_other(self):
+ """Select "other" on keypress."""
+ on_key_press = "selectWidget('%s', event);" % self._other_id()
+ 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 _get_suggestions(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 _autoselect_other(self):
+ """Select "other" on click."""
+ on_click = "onClick=\"selectWidget('%s', event);\"" % self._other_id()
+ self.other_selection_widget.extra = on_click
=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py 2010-11-09 18:42:38 +0000
+++ lib/lp/code/browser/branch.py 2010-11-10 17:12:46 +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-10 17:12:46 +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-05 18:30:21 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-11-10 17:12:46 +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),
Follow ups
