launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02699
[Merge] lp:~thumper/launchpad/choosing-recipe-name into lp:launchpad
Tim Penhey has proposed merging lp:~thumper/launchpad/choosing-recipe-name into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#671080 Hard to pick good title for recipe
https://bugs.launchpad.net/bugs/671080
#714534 Branch:+new-recipe refers to "product series"
https://bugs.launchpad.net/bugs/714534
For more details, see:
https://code.launchpad.net/~thumper/launchpad/choosing-recipe-name/+merge/50530
The primary purpose of this branch is to make the choice of
a recipe name easier. As a drive by fix, I changed product
series to project series in a place where the users can see
it.
The main changes here are:
- the main title of a recipe is now simply the name
- when creating a new recipe, the name is pre-generated
although the user is able to change it
- new recipes default to being daily builds
- in order to have daily builds, we need some distro series
selected by default, so I decided that all development
and current distroseries should be selected by default
- I changed most of the source package recipe browser tests
that were checking all the text on the page. It was often
very hard to work out what had changed.
- I deleted a few tests that seemed to add no value at all
--
https://code.launchpad.net/~thumper/launchpad/choosing-recipe-name/+merge/50530
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/choosing-recipe-name into lp:launchpad.
=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py 2011-02-18 02:39:47 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py 2011-02-21 03:33:17 +0000
@@ -14,6 +14,8 @@
'SourcePackageRecipeView',
]
+import itertools
+
from bzrlib.plugins.builder.recipe import (
ForbiddenInstructionError,
RecipeParseError,
@@ -93,7 +95,9 @@
MINIMAL_RECIPE_TEXT,
RECIPE_BETA_FLAG,
)
+from lp.code.model.sourcepackagerecipe import get_buildable_distroseries_set
from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.registry.interfaces.series import SeriesStatus
from lp.services.features import getFeatureFlag
from lp.services.propertycache import cachedproperty
from lp.soyuz.model.archive import Archive
@@ -546,12 +550,32 @@
"""The branch on which the recipe is built."""
return self.context
+ def _recipe_names(self):
+ """A generator of recipe names."""
+ branch_target_name = self.context.target.name.split('/')[-1]
+ yield "%s-daily" % branch_target_name
+ counter = itertools.count(1)
+ while True:
+ yield "%s-daily-%s" % (branch_target_name, counter.next())
+
+ def _find_unused_name(self, owner):
+ # Grab the last path element of the branch target path.
+ source = getUtility(ISourcePackageRecipeSource)
+ for recipe_name in self._recipe_names():
+ if not source.exists(owner, recipe_name):
+ return recipe_name
+
@property
def initial_values(self):
+ distros = get_buildable_distroseries_set(self.user)
+ series = [series for series in distros if series.status in (
+ SeriesStatus.CURRENT, SeriesStatus.DEVELOPMENT)]
return {
+ 'name' : self._find_unused_name(self.user),
'recipe_text': MINIMAL_RECIPE_TEXT % self.context.bzr_identity,
'owner': self.user,
- 'build_daily': False,
+ 'distros': series,
+ 'build_daily': True,
'use_ppa': EXISTING_PPA,
}
=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2011-02-18 05:40:41 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2011-02-21 03:33:17 +0000
@@ -12,10 +12,17 @@
datetime,
timedelta,
)
+import doctest
from textwrap import dedent
from mechanize import LinkNotFoundError
from pytz import utc
+from testtools.matchers import (
+ DocTestMatches,
+ Equals,
+ Matcher,
+ Mismatch,
+ )
import transaction
from zope.component import getUtility
from zope.security.interfaces import Unauthorized
@@ -50,10 +57,10 @@
)
from lp.code.tests.helpers import recipe_parser_newest_version
from lp.registry.interfaces.person import (
- IPersonSet,
TeamSubscriptionPolicy,
)
from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.registry.interfaces.series import SeriesStatus
from lp.services.features.testing import FeatureFixture
from lp.services.propertycache import clear_property_cache
from lp.soyuz.model.processor import ProcessorFamily
@@ -195,6 +202,142 @@
return extract_text(tags)
+class SoupMismatch(Mismatch):
+
+ def __init__(self, widget_id, soup_content):
+ self.widget_id = widget_id
+ self.soup_content = soup_content
+
+ def get_details(self):
+ return {'content': self.soup_content}
+
+
+class MissingElement(SoupMismatch):
+
+ def describe(self):
+ return 'No HTML element found with id %r' % self.widget_id
+
+
+class MultipleElements(SoupMismatch):
+
+ def describe(self):
+ return 'HTML id %r found multiple times in document' % self.widget_id
+
+
+class MatchesTagText(Matcher):
+ """Match against the extracted text of the tag."""
+
+ def __init__(self, soup_content, tag_id):
+ """Construct the matcher with the soup content."""
+ self.soup_content = soup_content
+ self.tag_id = tag_id
+
+ def __str__(self):
+ return "matches widget %r text" % self.tag_id
+
+ def match(self, matchee):
+ widgets = self.soup_content.findAll(id=self.tag_id)
+ if len(widgets) == 0:
+ return MissingElement(self.tag_id, self.soup_content)
+ elif len(widgets) > 1:
+ return MultipleElements(self.tag_id, self.soup_content)
+ widget = widgets[0]
+ text_matcher = DocTestMatches(
+ extract_text(widget), doctest.NORMALIZE_WHITESPACE)
+ return text_matcher.match(matchee)
+
+
+class MatchesPickerText(Matcher):
+ """Match against the text in a widget."""
+
+ def __init__(self, soup_content, widget_id):
+ """Construct the matcher with the soup content."""
+ self.soup_content = soup_content
+ self.widget_id = widget_id
+
+ def __str__(self):
+ return "matches widget %r text" % self.widget_id
+
+ def match(self, matchee):
+ widgets = self.soup_content.findAll(id=self.widget_id)
+ if len(widgets) == 0:
+ return MissingElement(self.widget_id, self.soup_content)
+ elif len(widgets) > 1:
+ return MultipleElements(self.widget_id, self.soup_content)
+ widget = widgets[0]
+ text = widget.findAll(attrs={'class': 'yui3-activator-data-box'})[0]
+ text_matcher = DocTestMatches(
+ extract_text(text), doctest.NORMALIZE_WHITESPACE)
+ return text_matcher.match(matchee)
+
+
+class TestSourcePackageRecipeAddViewInitalValues(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_project_branch_initial_name(self):
+ # When a project branch is used, the initial name is the name of the
+ # project followed by "-daily"
+ widget = self.factory.makeProduct(name='widget')
+ branch = self.factory.makeProductBranch(widget)
+ with person_logged_in(branch.owner):
+ view = create_initialized_view(branch, '+new-recipe')
+ self.assertThat('widget-daily', Equals(view.initial_values['name']))
+
+ def test_package_branch_initial_name(self):
+ # When a package branch is used, the initial name is the name of the
+ # source package followed by "-daily"
+ branch = self.factory.makePackageBranch(sourcepackagename='widget')
+ with person_logged_in(branch.owner):
+ view = create_initialized_view(branch, '+new-recipe')
+ self.assertThat('widget-daily', Equals(view.initial_values['name']))
+
+ def test_initial_name_exists(self):
+ # If the initial name exists, a generator is used to find an unused
+ # name by appending a numbered suffix on the end.
+ owner = self.factory.makePerson()
+ self.factory.makeSourcePackageRecipe(owner=owner, name=u'widget-daily')
+ widget = self.factory.makeProduct(name='widget')
+ branch = self.factory.makeProductBranch(widget)
+ with person_logged_in(owner):
+ view = create_initialized_view(branch, '+new-recipe')
+ self.assertThat('widget-daily-1', Equals(view.initial_values['name']))
+
+ def test_initial_series(self):
+ # The initial series are those that are current or in development.
+ archive = self.factory.makeArchive()
+ experimental = self.factory.makeDistroSeries(
+ distribution=archive.distribution,
+ status=SeriesStatus.EXPERIMENTAL)
+ development = self.factory.makeDistroSeries(
+ distribution=archive.distribution,
+ status=SeriesStatus.DEVELOPMENT)
+ frozen = self.factory.makeDistroSeries(
+ distribution=archive.distribution,
+ status=SeriesStatus.FROZEN)
+ current = self.factory.makeDistroSeries(
+ distribution=archive.distribution,
+ status=SeriesStatus.CURRENT)
+ supported = self.factory.makeDistroSeries(
+ distribution=archive.distribution,
+ status=SeriesStatus.SUPPORTED)
+ obsolete = self.factory.makeDistroSeries(
+ distribution=archive.distribution,
+ status=SeriesStatus.OBSOLETE)
+ future = self.factory.makeDistroSeries(
+ distribution=archive.distribution,
+ status=SeriesStatus.FUTURE)
+ branch = self.factory.makeAnyBranch()
+ with person_logged_in(archive.owner):
+ view = create_initialized_view(branch, '+new-recipe')
+ series = set(view.initial_values['distros'])
+ initial_series = set([development, current])
+ self.assertEqual(initial_series, series.intersection(initial_series))
+ other_series = set(
+ [experimental, frozen, supported, obsolete, future])
+ self.assertEqual(set(), series.intersection(other_series))
+
+
class TestSourcePackageRecipeAddView(TestCaseForRecipe):
layer = DatabaseFunctionalLayer
@@ -232,32 +375,17 @@
browser.getControl(name='field.name').value = 'daily'
browser.getControl('Description').value = 'Make some food!'
- browser.getControl('Secret Squirrel').click()
- browser.getControl('Built daily').click()
browser.getControl('Create Recipe').click()
- pattern = """\
- Master Chef's daily recipe
- .*
-
- Description
- Make some food!
-
- Recipe information
- Build schedule: Tag help Built daily
- Owner: Master Chef Edit
- Base branch: lp://dev/~chef/ratatouille/veggies
- Debian version: {debupstream}-0~{revno}
- Daily build archive: Secret PPA Edit
- Distribution series: Secret Squirrel
- .*
-
- Recipe contents
- # bzr-builder format 0.3 deb-version {debupstream}-0~{revno}
- lp://dev/~chef/ratatouille/veggies"""
- main_text = extract_text(find_main_content(browser.contents))
- self.assertTextMatchesExpressionIgnoreWhitespace(
- pattern, main_text)
+ content = find_main_content(browser.contents)
+ self.assertEqual('daily', content.h1.string)
+ self.assertThat(
+ 'Make some food!', MatchesTagText(content, 'edit-description'))
+ self.assertThat(
+ 'Master Chef', MatchesPickerText(content, 'edit-owner'))
+ self.assertThat(
+ 'Secret PPA',
+ MatchesPickerText(content, 'edit-daily_build_archive'))
def test_create_new_recipe_private_branch(self):
# Recipes can't be created on private branches.
@@ -275,10 +403,8 @@
# Teams that the user is in are options for the recipe owner.
self.factory.makeTeam(
name='good-chefs', displayname='Good Chefs', members=[self.chef])
- branch = self.makeBranch()
- browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
- browser.getLink('Create packaging recipe').click()
-
+ browser = self.getViewBrowser(
+ self.makeBranch(), '+new-recipe', user=self.chef)
# The options for the owner include the Good Chefs team.
options = browser.getControl(name='field.owner.owner').displayOptions
self.assertEquals(
@@ -289,14 +415,10 @@
# New recipes can be owned by teams that the user is a member of.
team = self.factory.makeTeam(
name='good-chefs', displayname='Good Chefs', members=[self.chef])
- branch = self.makeBranch()
- browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
- browser.getLink('Create packaging recipe').click()
-
+ browser = self.getViewBrowser(
+ self.makeBranch(), '+new-recipe', user=self.chef)
browser.getControl(name='field.name').value = 'daily'
browser.getControl('Description').value = 'Make some food!'
- browser.getControl('Secret Squirrel').click()
- browser.getControl('Built daily').click()
browser.getControl('Other').click()
browser.getControl(name='field.owner.owner').displayValue = [
'Good Chefs']
@@ -341,81 +463,28 @@
name='ratatouille', displayname='Ratatouille')
branch = self.factory.makeBranch(
owner=self.chef, product=product, name='veggies')
-
- # A new recipe can be created from the branch page.
- browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
- browser.getLink('Create packaging recipe').click()
-
- browser.getControl(name='field.name').value = 'daily'
+ browser = self.getViewBrowser(branch, '+new-recipe', user=self.chef)
browser.getControl('Description').value = 'Make some food!'
- browser.getControl('Secret Squirrel').click()
-
browser.getControl('Recipe text').value = (
browser.getControl('Recipe text').value + 'run cat /etc/passwd')
-
browser.getControl('Create Recipe').click()
-
self.assertEqual(
get_message_text(browser, 2),
'The bzr-builder instruction "run" is not permitted here.')
- def test_create_new_recipe_empty_name(self):
- # Leave off the name and make sure that the widgets validate before
- # the content validates.
- product = self.factory.makeProduct(
- name='ratatouille', displayname='Ratatouille')
- branch = self.factory.makeBranch(
- owner=self.chef, product=product, name='veggies')
-
- # A new recipe can be created from the branch page.
- browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
- browser.getLink('Create packaging recipe').click()
-
- browser.getControl('Description').value = 'Make some food!'
- browser.getControl('Secret Squirrel').click()
- browser.getControl('Create Recipe').click()
-
- self.assertEqual(
- get_message_text(browser, 2), 'Required input is missing.')
-
def createRecipe(self, recipe_text, branch=None):
if branch is None:
product = self.factory.makeProduct(
name='ratatouille', displayname='Ratatouille')
branch = self.factory.makeBranch(
owner=self.chef, product=product, name='veggies')
-
- # A new recipe can be created from the branch page.
- browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
- browser.getLink('Create packaging recipe').click()
-
+ browser = self.getViewBrowser(branch, '+new-recipe', user=self.chef)
browser.getControl(name='field.name').value = 'daily'
browser.getControl('Description').value = 'Make some food!'
browser.getControl('Recipe text').value = recipe_text
browser.getControl('Create Recipe').click()
return browser
- def test_create_recipe_bad_text(self):
- # If a user tries to create source package recipe with bad text, they
- # should get an error.
- branch = self.factory.makeBranch(name='veggies')
- package_branch = self.factory.makeBranch(name='packaging')
-
- browser = self.createRecipe(
- dedent('''
- # bzr-builder format 0.3 deb-version {debupstream}-0~{revno}
- %(branch)s
- merge %(package_branch)s
- ''' % {
- 'branch': branch.bzr_identity,
- 'package_branch': package_branch.bzr_identity,
- }),
- branch=branch)
- self.assertEqual(
- get_message_text(browser, 2),
- "Error parsing recipe:1:1:"
- " End of line while looking for '#'.")
-
def test_create_recipe_usage(self):
# The error for a recipe with invalid instruction parameters should
# include instruction usage.
@@ -441,8 +510,7 @@
browser = self.getViewBrowser(self.makeBranch(), '+new-recipe')
browser.getControl(name='field.name').value = 'daily'
browser.getControl('Description').value = 'Make some food!'
-
- browser.getControl('Built daily').click()
+ browser.getControl(name='field.distros').value = []
browser.getControl('Create Recipe').click()
self.assertEqual(
'You must specify at least one series for daily builds.',
@@ -762,29 +830,18 @@
browser.getControl('PPA 2').click()
browser.getControl('Update Recipe').click()
- pattern = """\
- Master Chef's fings recipe
- .*
-
- Description
- This is stuff
-
- Recipe information
- Build schedule: Tag help Built on request
- Owner: Master Chef Edit
- Base branch: lp://dev/~chef/ratatouille/meat
- Debian version: {debupstream}-0~{revno}
- Daily build archive:
- PPA 2 Edit
- Distribution series: Mumbly Midget
- .*
-
- Recipe contents
- # bzr-builder format 0.3 deb-version {debupstream}-0~{revno}
- lp://dev/~chef/ratatouille/meat"""
- main_text = extract_text(find_main_content(browser.contents))
- self.assertTextMatchesExpressionIgnoreWhitespace(
- pattern, main_text)
+ content = find_main_content(browser.contents)
+ self.assertThat(
+ 'This is stuff', MatchesTagText(content, 'edit-description'))
+ self.assertThat(
+ '# bzr-builder format 0.3 deb-version {debupstream}-0~{revno}\n'
+ 'lp://dev/~chef/ratatouille/meat',
+ MatchesTagText(content, 'edit-recipe_text'))
+ self.assertThat(
+ 'Distribution series: Mumbly Midget',
+ MatchesTagText(content, 'distros'))
+ self.assertThat(
+ 'PPA 2', MatchesPickerText(content, 'edit-daily_build_archive'))
def test_admin_edit(self):
self.factory.makeDistroSeries(
@@ -822,29 +879,17 @@
browser.getControl('Mumbly Midget').click()
browser.getControl('Update Recipe').click()
- pattern = """\
- Master Chef's fings recipe
- .*
-
- Description
- This is stuff
-
- Recipe information
- Build schedule: Tag help Built on request
- Owner: Master Chef Edit
- Base branch: lp://dev/~chef/ratatouille/meat
- Debian version: {debupstream}-0~{revno}
- Daily build archive:
- Secret PPA Edit
- Distribution series: Mumbly Midget
- .*
-
- Recipe contents
- # bzr-builder format 0.3 deb-version {debupstream}-0~{revno}
- lp://dev/~chef/ratatouille/meat"""
- main_text = extract_text(find_main_content(browser.contents))
- self.assertTextMatchesExpressionIgnoreWhitespace(
- pattern, main_text)
+ content = find_main_content(browser.contents)
+ self.assertEqual('fings', content.h1.string)
+ self.assertThat(
+ 'This is stuff', MatchesTagText(content, 'edit-description'))
+ self.assertThat(
+ '# bzr-builder format 0.3 deb-version {debupstream}-0~{revno}\n'
+ 'lp://dev/~chef/ratatouille/meat',
+ MatchesTagText(content, 'edit-recipe_text'))
+ self.assertThat(
+ 'Distribution series: Mumbly Midget',
+ MatchesTagText(content, 'distros'))
def test_edit_recipe_forbidden_instruction(self):
self.factory.makeDistroSeries(
@@ -934,58 +979,6 @@
extract_text(find_tags_by_class(browser.contents, 'message')[1]),
'There is already a recipe owned by Master Chef with this name.')
- def test_edit_recipe_but_not_name(self):
- self.factory.makeDistroSeries(
- displayname='Mumbly Midget', name='mumbly',
- distribution=self.ppa.distribution)
- product = self.factory.makeProduct(
- name='ratatouille', displayname='Ratatouille')
- veggie_branch = self.factory.makeBranch(
- owner=self.chef, product=product, name='veggies')
- meat_branch = self.factory.makeBranch(
- owner=self.chef, product=product, name='meat')
- ppa = self.factory.makeArchive(name='ppa')
- recipe = self.factory.makeSourcePackageRecipe(
- owner=self.chef, registrant=self.chef,
- name=u'things', description=u'This is a recipe',
- distroseries=self.squirrel, branches=[veggie_branch],
- daily_build_archive=ppa)
-
- meat_path = meat_branch.bzr_identity
-
- browser = self.getUserBrowser(canonical_url(recipe), user=self.chef)
- browser.getLink('Edit recipe').click()
- browser.getControl('Description').value = 'This is stuff'
- browser.getControl('Recipe text').value = (
- MINIMAL_RECIPE_TEXT % meat_path)
- browser.getControl('Secret Squirrel').click()
- browser.getControl('Mumbly Midget').click()
- browser.getControl('Update Recipe').click()
-
- pattern = """\
- Master Chef's things recipe
- .*
-
- Description
- This is stuff
-
- Recipe information
- Build schedule: Tag help Built on request
- Owner: Master Chef Edit
- Base branch: lp://dev/~chef/ratatouille/meat
- Debian version: {debupstream}-0~{revno}
- Daily build archive:
- Secret PPA Edit
- Distribution series: Mumbly Midget
- .*
-
- Recipe contents
- # bzr-builder format 0.3 deb-version {debupstream}-0~{revno}
- lp://dev/~chef/ratatouille/meat"""
- main_text = extract_text(find_main_content(browser.contents))
- self.assertTextMatchesExpressionIgnoreWhitespace(
- pattern, main_text)
-
def test_edit_recipe_private_branch(self):
# If a user tries to set source package recipe to use a private
# branch, they should get an error.
=== modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py'
--- lib/lp/code/interfaces/sourcepackagerecipe.py 2011-02-18 02:39:47 +0000
+++ lib/lp/code/interfaces/sourcepackagerecipe.py 2011-02-21 03:33:17 +0000
@@ -29,7 +29,6 @@
mutator_for,
operation_for_version,
operation_parameters,
- operation_removed_in_version,
REQUEST_USER,
)
from lazr.restful.fields import (
@@ -51,7 +50,6 @@
TextLine,
)
-from canonical.config import config
from canonical.launchpad import _
from canonical.launchpad.validators.name import name_validator
from lp.code.interfaces.branch import IBranch
@@ -188,7 +186,9 @@
name = exported(TextLine(
title=_("Name"), required=True,
constraint=name_validator,
- description=_("The name of this recipe.")))
+ description=_(
+ "The name of the recipe is part of the URL and needs to "
+ "be unique for the given owner.")))
description = exported(Description(
title=_('Description'), required=True,
=== modified file 'lib/lp/code/templates/sourcepackagerecipe-index.pt'
--- lib/lp/code/templates/sourcepackagerecipe-index.pt 2011-02-15 02:33:13 +0000
+++ lib/lp/code/templates/sourcepackagerecipe-index.pt 2011-02-21 03:33:17 +0000
@@ -35,6 +35,12 @@
<div tal:replace="structure context/@@+global-actions" />
</metal:side>
+<metal:heading fill-slot="heading">
+ <h1 tal:content="context/name">
+ recipe name
+ </h1>
+</metal:heading>
+
<div metal:fill-slot="main">
<div class="yui-g first">
<div class="yui-u first">
=== modified file 'lib/lp/code/templates/sourcepackagerecipe-related-branches.pt'
--- lib/lp/code/templates/sourcepackagerecipe-related-branches.pt 2011-02-03 12:50:03 +0000
+++ lib/lp/code/templates/sourcepackagerecipe-related-branches.pt 2011-02-21 03:33:17 +0000
@@ -40,7 +40,7 @@
</div>
<div tal:condition="seriesBranches" id="related-series-branches">
- <h2 style="margin-top: 1.5em;">Product series branches</h2>
+ <h2 style="margin-top: 1.5em;">Project series branches</h2>
<table class="listing" id="related-series-branches-listing">
<thead>
<tr>
