This looks good but I have a few questions/suggestions.

 review needsfixing

On Fri, 2010-03-19 at 14:53 +0000, Aaron Bentley wrote:
> 
> = Summary =
> Provide an index page for recipes
> 
> == Proposed fix ==
> 
> == Pre-implementation notes ==
> Mostly discussed with rockstar, some with thumper
> 
> == Implementation details ==
> Because we do not want to provide an unfinished UI to Edge users, this
> page
> is only enabled for the test runner and devel.
> 
> As well as a view for recipes, a view for builds was provided, so that
> we can
> get user-facing information the view class.
> 
> It was determined, in discussion with Muharem, that the default of
> creating
> builders with manual=True is not a sane default for test purposes.
> This has an
> impact on ETA calculations, because manual builders are ignored in
> such
> calculations.  Therefore, I added the ability to control the manual
> flag in
> IBuilderSet.new, and forced it to False in the factory method.
> 
> In consultation with Michael Hudson, I determined that
> _SourcePackageRecipeData had several attributes that it would make
> sense to
> view and manipulate directly.  However, it needs to remain a separate
> class
> because it is common to recipes and manifests.  So I had
> SourcePackageRecipe
> delegate to it.
> 
> There were miscellaneous lint fixes, like calling Storm.__init__ in
> subclasses.
> 
> I adjusted LaunchpadObjectFactory.makeDistroSeries to permit
> specifying a
> displayname for distroseries.  I adjusted
> makeSourcePackageRecipeBuildJob to
> allow specifying a build.  These changes helped me provide a
> consistent
> homepage for tests.
> 
> == Tests ==
> bin/test -v test_sourcepackagerecipe
> 
> == Demo and Q/A ==
> Cannot be Q/A'ed until more pieces are in place.
> 

> === modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
> --- lib/lp/code/browser/sourcepackagerecipe.py  2010-03-12 18:53:55 +0000
> +++ lib/lp/code/browser/sourcepackagerecipe.py  2010-03-19 14:53:16 +0000
> @@ -6,3 +6,79 @@
>  __metaclass__ = type
>  
>  __all__ = []
> +
> +
> +from canonical.launchpad.webapp import (
> +    LaunchpadView)
> +from lp.buildmaster.interfaces.buildbase import BuildStatus
> +
> +
> +class SourcePackageRecipeView(LaunchpadView):
> +    """Default view of a SourcePackageRecipe."""
> +
> +    @property
> +    def title(self):
> +        return self.context.name
> +
> +    label = title
> +
> +    @property
> +    def builds(self):
> +        """A list of interesting builds.
> +
> +        All pending builds are shown, as well as 1-5 recent builds.
> +        Recent builds are ordered by date completed.
> +        """
> +        builds = list(self.context.getBuilds(pending=True))
> +        for build in self.context.getBuilds():
> +            builds.append(build)
> +            if len(builds) >= 5:
> +                break
> +        builds.reverse()
> +        return builds
> +
> +
> +class SourcePackageRecipeBuildView(LaunchpadView):
> +    """Default view of a SourcePackageRecipeBuild."""
> +
> +    @property
> +    def status(self):
> +        """A human-friendly status string."""
> +        description = {
> +            BuildStatus.NEEDSBUILD: 'Pending build',
> +            BuildStatus.FULLYBUILT: 'Successful build',
> +            BuildStatus.FAILEDTOBUILD: 'Failed to build',
> +            BuildStatus.MANUALDEPWAIT:
> +                'Could not build because of missing dependencies',
> +            BuildStatus.CHROOTWAIT:
> +                'Could not build because of chroot issues',
> +            BuildStatus.SUPERSEDED:
> +                'Could not build because source package was superseded',
> +            BuildStatus.BUILDING:
> +                'Currently building',
> +            BuildStatus.FAILEDTOUPLOAD:
> +                'Could not be uploaded correctly',

Would it not make sense to have these strings as the title of the items
in the BuildStatus enum?

> +        }
> +        if self.context.buildstate == BuildStatus.NEEDSBUILD:
> +            if self.eta is None:
> +                return 'No suitable builders'
> +        return description[self.context.buildstate]
> +
> +    @property
> +    def eta(self):
> +        """The datetime when the build job is estimated to begin."""
> +        if self.context.buildqueue_record is None:
> +            return None
> +        return self.context.buildqueue_record.getEstimatedJobStartTime()
> +
> +    @property
> +    def date(self):
> +        """The date when the build complete or will begin."""
> +        if self.estimate:
> +            return self.eta
> +        return self.context.datebuilt
> +
> +    @property
> +    def estimate(self):
> +        """If true, the date value is an estimate."""
> +        return (self.context.datebuilt is None and self.eta is not None)
> 
> === added file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
> --- lib/lp/code/browser/tests/test_sourcepackagerecipe.py       1970-01-01 00:00:00 +0000
> +++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py       2010-03-19 14:53:16 +0000
> @@ -0,0 +1,125 @@
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Tests for the product view classes and templates."""
> +
> +__metaclass__ = type
> +
> +
> +from datetime import datetime
> +from textwrap import dedent
> +import re
> +
> +from pytz import utc
> +from canonical.testing import DatabaseFunctionalLayer
> +from zope.security.proxy import removeSecurityProxy
> +
> +from canonical.launchpad.webapp import canonical_url
> +from canonical.launchpad.webapp.servers import LaunchpadTestRequest
> +from canonical.launchpad.testing.pages import extract_text, find_main_content
> +from lp.buildmaster.interfaces.buildbase import BuildStatus
> +from lp.code.browser.sourcepackagerecipe import SourcePackageRecipeView
> +from lp.testing import (TestCaseWithFactory)
> +
> +
> +class TestSourcePackageRecipe(TestCaseWithFactory):

Judging by the test class' name, I was expecting to see a test for
SourcePackageRecipe, but in fact it's testing SPR's default view. How
about renaming the test class to TestSourcePackageRecipeView?

> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def makeRecipe(self):
> +        chef = self.factory.makePersonNoCommit(displayname='Master Chef',
> +                name='chef')
> +        chocolate = self.factory.makeProduct(name='chocolate')
> +        cake_branch = self.factory.makeProductBranch(owner=chef, name='cake',
> +            product=chocolate)

Mind moving all arguments into the same line, in the calls to
makeProductBranch and makePerson above?  They should either be on a line
by themselves or be aligned...

> +        distroseries = self.factory.makeDistroSeries(
> +            displayname='Secret Squirrel')
> +        return self.factory.makeSourcePackageRecipe(
> +            None, chef, distroseries, None, u'Cake Recipe', cake_branch)

... like above. :)

> +
> +    def getMainText(self, recipe):
> +        browser = self.getUserBrowser(canonical_url(recipe))
> +        return extract_text(find_main_content(browser.contents))
> +
> +    def test_index(self):
> +        recipe = self.makeRecipe()
> +        build = removeSecurityProxy(self.factory.makeSourcePackageRecipeBuild(
> +            recipe=recipe))
> +        build.buildstate = BuildStatus.FULLYBUILT
> +        build.datebuilt = datetime(2010, 03, 16, tzinfo=utc)
> +        pattern = re.compile(dedent("""\
> +            Master Chef
> +            Branches
> +            Description
> +            This recipe .*changes.
> +            Recipe Information
> +            Owner:
> +            Master Chef
> +            Base branch:
> +            lp://dev/~chef/chocolate/cake
> +            Debian version:
> +            1.0
> +            Distros:
> +            Secret Squirrel
> +            Build records
> +            Successful build.on 2010-03-16
> +            Recipe contents
> +            # bzr-builder format 0.2 deb-version 1.0
> +            lp://dev/~chef/chocolate/cake"""), re.S)
> +        main_text = self.getMainText(recipe)
> +        self.assertTrue(pattern.search(main_text), main_text)
> +
> +    def test_index_no_suitable_builders(self):
> +        recipe = self.makeRecipe()
> +        build = removeSecurityProxy(self.factory.makeSourcePackageRecipeBuild(
> +            recipe=recipe))
> +        pattern = re.compile(dedent("""\
> +            Build records
> +            No suitable builders
> +            Recipe contents"""), re.S)
> +        main_text = self.getMainText(recipe)
> +        self.assertTrue(pattern.search(main_text), main_text)
> +
> +    def makeBuildJob(self, recipe):
> +        build = self.factory.makeSourcePackageRecipeBuild(recipe=recipe)
> +        buildjob = self.factory.makeSourcePackageRecipeBuildJob(
> +            recipe_build=build)
> +        return build
> +
> +    def test_index_pending(self):
> +        recipe = self.makeRecipe()
> +        buildjob = self.makeBuildJob(recipe)
> +        builder = self.factory.makeBuilder()
> +        pattern = re.compile(dedent("""\
> +            Build records
> +            Pending build.in .*\(estimated\)
> +            Recipe contents"""), re.S)
> +        main_text = self.getMainText(recipe)
> +        self.assertTrue(pattern.search(main_text), main_text)
> +
> +    def test_builds(self):
> +        """Ensure SourcePackageRecipe.builds is as described."""

s/SourcePackageRecipe/SourcePackageRecipeView/

> +        recipe = self.makeRecipe()
> +        build1 = self.makeBuildJob(recipe=recipe)
> +        build2 = self.makeBuildJob(recipe=recipe)
> +        build3 = self.makeBuildJob(recipe=recipe)
> +        build4 = self.makeBuildJob(recipe=recipe)
> +        build5 = self.makeBuildJob(recipe=recipe)
> +        build6 = self.makeBuildJob(recipe=recipe)
> +        view = SourcePackageRecipeView(recipe, LaunchpadTestRequest)

You might want to pass None as the request here. If you'd rather pass a
proper request, then you should pass an instance of LTR.

> +        self.assertEqual(
> +            set([build1, build2, build3, build4, build5, build6]),
> +            set(view.builds))
> +        def set_day(build, day):
> +            removeSecurityProxy(build).datebuilt = datetime(
> +                2010, 03, day, tzinfo=utc)
> +        set_day(build1, 16)
> +        set_day(build2, 15)
> +        self.assertEqual(
> +            set([build1, build3, build4, build5, build6]),
> +            set(view.builds))

The purpose of this assertion is not clear, I think.  Care to add a
comment?

> +        set_day(build3, 14)
> +        set_day(build4, 13)
> +        set_day(build5, 12)
> +        set_day(build6, 11)
> +        self.assertEqual([build5, build4, build3, build2, build1], view.builds)

> === modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py'
> --- lib/lp/code/interfaces/sourcepackagerecipe.py       2010-02-05 14:22:03 +0000
> +++ lib/lp/code/interfaces/sourcepackagerecipe.py       2010-03-19 14:53:16 +0000
> @@ -42,7 +46,20 @@
>          self.newest_supported = newest_supported
>  
>  
> -class ISourcePackageRecipe(IHasOwner):
> +class ISourcePackageRecipeData(Interface):
> +    """A recipe as database data, not text."""
> +
> +    base_branch = Object(
> +        schema=IBranch, title=_("Base branch"), description=_(
> +            "The base branch to use when building the recipe."))
> +
> +    deb_version_template = TextLine(
> +        title=_('deb-version template'),
> +        description = _(
> +            'The template that will be used to generate a deb version.'),)
> +
> +
> +class ISourcePackageRecipe(IHasOwner, ISourcePackageRecipeData):
>      """An ISourcePackageRecipe describes how to build a source package.
>  
>      More precisely, it describes how to combine a number of branches into a
> @@ -87,6 +104,9 @@
>              able to upload to the archive.
>          """
>  
> +    def getBuilds(pending=False):
> +        """Return a ResultSet of all the specified builds."""

"... of all the builds in the given state.", maybe?

> +
>  
>  class ISourcePackageRecipeSource(Interface):
>      """A utility of this interface can be used to create and access recipes.
> 
> === modified file 'lib/lp/code/model/sourcepackagerecipe.py'
> --- lib/lp/code/model/sourcepackagerecipe.py    2010-02-05 15:38:19 +0000
> +++ lib/lp/code/model/sourcepackagerecipe.py    2010-03-19 14:53:16 +0000
> @@ -111,3 +118,14 @@
>              self, requester, archive)
>          build.queueBuild()
>          return build
> +
> +    def getBuilds(self, pending=False):

"""See `...`"""

> +        if pending:
> +            clauses = [SourcePackageRecipeBuild.datebuilt == None]
> +        else:
> +            clauses = [SourcePackageRecipeBuild.datebuilt != None]
> +        result = Store.of(self).find(
> +            SourcePackageRecipeBuild, SourcePackageRecipeBuild.recipe==self,
> +            *clauses)
> +        result.order_by(Desc(SourcePackageRecipeBuild.datebuilt))
> +        return result
> 
> === modified file 'lib/lp/code/model/sourcepackagerecipedata.py'
> --- lib/lp/code/model/sourcepackagerecipedata.py        2010-02-05 15:07:44 +0000
> +++ lib/lp/code/model/sourcepackagerecipedata.py        2010-03-19 14:53:16 +0000
> @@ -9,7 +9,7 @@
>  """
>  
>  __metaclass__ = type
> -__all__ = ['_SourcePackageRecipeData']
> +__all__ = ['SourcePackageRecipeData']

Why did you rename it?

>  
>  from bzrlib.plugins.builder.recipe import (
>      BaseRecipeBranch, MergeInstruction, NestInstruction, RecipeBranch)
> @@ -50,6 +50,7 @@
>  
>      def __init__(self, name, type, comment, line_number, branch, revspec,
>                   directory, recipe_data, parent_instruction):
> +        super( _SourcePackageRecipeDataInstruction, self).__init__()

Is there a whitespace preceding the class name above?

>          self.name = unicode(name)
>          self.type = type
>          self.comment = comment
> @@ -78,7 +79,7 @@
>      directory = Unicode(allow_none=True)
>  
>      recipe_data_id = Int(name='recipe_data', allow_none=False)
> -    recipe_data = Reference(recipe_data_id, '_SourcePackageRecipeData.id')
> +    recipe_data = Reference(recipe_data_id, 'SourcePackageRecipeData.id')
>  
>      parent_instruction_id = Int(name='parent_instruction', allow_none=True)
>      parent_instruction = Reference(
> @@ -97,7 +98,7 @@
>          return branch
>  
>  
> -class _SourcePackageRecipeData(Storm):
> +class SourcePackageRecipeData(Storm):
>      """The database representation of a BaseRecipeBranch from bzr-builder.
>  
>      This is referenced from the SourcePackageRecipe table as the 'recipe_data'
> @@ -215,6 +216,7 @@
>      def __init__(self, recipe, sourcepackage_recipe):
>          """Initialize from the bzr-builder recipe and link it to a db recipe.
>          """
> +        super(SourcePackageRecipeData, self).__init__()
>          self.setRecipe(recipe)
>          self.sourcepackage_recipe = sourcepackage_recipe
>  
> 
> === added file 'lib/lp/code/templates/sourcepackagerecipe-index.pt'
> --- lib/lp/code/templates/sourcepackagerecipe-index.pt  1970-01-01 00:00:00 +0000
> +++ lib/lp/code/templates/sourcepackagerecipe-index.pt  2010-03-19 14:53:16 +0000
> @@ -0,0 +1,80 @@
> +<html
> +  xmlns="http://www.w3.org/1999/xhtml"
> +  xmlns:tal="http://xml.zope.org/namespaces/tal"
> +  xmlns:metal="http://xml.zope.org/namespaces/metal"
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> +  metal:use-macro="view/macro:page/main_side"
> +  i18n:domain="launchpad"
> +>
> +
> +
> +<body>
> +
> +<tal:registering metal:fill-slot="registering">
> +  Created by
> +    <tal:registrant replace="structure context/registrant/fmt:link" />
> +  on
> +    <tal:created-on replace="structure context/date_created/fmt:date" />
> +  and last modified on
> +    <tal:last-modified replace="structure context/date_last_modified/fmt:date" />
> +</tal:registering>
> +
> +
> +<div metal:fill-slot="main">
> +  <div class="yui-g first">
> +    <div class="portlet">
> +      <h2>Description</h2>
> +        This recipe builds a foo for disto bar, with my Secret Squirrel
> +        changes.
> +      </div>
> +    </div>
> +    <div class="yui-g">
> +      <div class="yui-u first">
> +        <div class="portlet">
> +          <h2>Recipe Information</h2>
> +          <div class="two-column-list">
> +            <dl id="owner">
> +              <dt>Owner:</dt>
> +              <dd tal:content="structure context/owner/fmt:link" />
> +            </dl>
> +            <dl id="base-branch">
> +              <dt>Base branch:</dt>
> +              <dd tal:content="structure context/base_branch/fmt:link" />
> +            </dl>
> +            <dl id="debian-version">
> +              <dt>Debian version:</dt>
> +              <dd tal:content="context/deb_version_template" />
> +            </dl>
> +            <dl id="distros">
> +              <dt>Distros:</dt>

Is this really meant to be in the plural?  Judging by the code below,
I'd expect to see a single distribution series here.  Also, we shouldn't
be using 'Distros' in the UI, should we?

> +              <dd>
> +                <ul>
> +                  <li tal:content="structure context/distroseries/fmt:link" />
> +                </ul>
> +              </dd>
> +            </dl>
> +          </div>
> +        </div>
> +      </div>
> +      <div class="yui-u">
> +        <div class="portlet">
> +          <h2>Build records</h2>
> +          <ul>
> +            <li tal:repeat="build view/builds">
> +              <tal:status replace="build/@@+index/status" />
> +              <tal:date replace="build/@@+index/date/fmt:displaydate" />
> +              <tal:estimate condition="build/@@+index/estimate">
> +              (estimated)
> +              </tal:estimate>
> +            </li>
> +          </ul>
> +        </div>
> +      </div>
> +    </div>
> +    <div class='portlet'>
> +      <h2>Recipe contents</h2>
> +      <pre tal:content="context/builder_recipe" />
> +    </div>
> +  </div>
> +</body>
> +</html>

> === modified file 'lib/lp/registry/interfaces/person.py'
> --- lib/lp/registry/interfaces/person.py        2010-03-11 20:59:17 +0000
> +++ lib/lp/registry/interfaces/person.py        2010-03-19 14:53:16 +0000
> @@ -874,6 +874,9 @@
>          not teams can be converted into teams.
>          """
>  
> +    def getRecipe(name):

I wonder if we shouldn't call this getSourcePackageRecipe(name)?
Although it's a lot longer it doesn't leave one wondering what sort of
recipe could a person have.

> +        """Return the person's recipe with the given name."""
> +
>      def getInvitedMemberships():
>          """Return all TeamMemberships of this team with the INVITED status.
>  
> 



-- 
Guilherme Salgado <email address hidden>