Merge lp:~abentley/launchpad/recipe-index into lp:launchpad

Screenshot:
http://people.canonical.com/~abentley/recipe-index-review.png

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...

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

Guilherme Salgado wrote:
>> + @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?

That's Soyuz code, and I don't want to change it unnecessarily. I don't
know how the existing titles are used, and whether these strings would
make sense in that context.

>> === 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
>> +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?

Done.

>
>> +
>> + 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...

Done.

>> + 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 reco...

I don't have any comments because we've been iterating over this for a week, making changes here and there. This is exactly as we have discussed over the last week.

On Mon, 2010-03-22 at 14:09 +0000, Aaron Bentley wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Guilherme Salgado wrote:
> >> + @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?
>
> That's Soyuz code, and I don't want to change it unnecessarily. I don't
> know how the existing titles are used, and whether these strings would
> make sense in that context.
>

Being able to use the enum's titles both here and in soyuz code will
make for more consistent UI and avoid this extra code, so we all win.
Have you checked what the current title is for them? Maybe you could
use what's there already?

Julian, what do you think?

> >> === 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?
>
> It has become part of the public API, no longer an implementation
> detail, because it manages data that is useful to address directly.
>

Ok.

> >> 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?
>
> There was. Gone now.
>
> >> + <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.
>
> Yes, it is meant to be in the plural. There is a database patch that is
> just waiting for Bjorn's review to change the relationship to a
> many-to-many:
> https://code.edge.launchpad.net/~abentley/launchpad/multiple-series-recipe/+merge/21257
>

Ok, cool.

> > Also, we shouldn't
> > be using 'Distros' in the UI, should we?
>
> Okay, so what should we be using?

Distributions, perha...

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

Guilherme Salgado wrote:
> Being able to use the enum's titles both here and in soyuz code will
> make for more consistent UI and avoid this extra code, so we all win.
> Have you checked what the current title is for them? Maybe you could
> use what's there already?

I have looked at what's there, and it's not appropriate for us. That
suggests our text won't be appropriate in Soyuz contexts.

Also, we are attempting to land this without any impact on end-users.
Changing the enum titles would have an impact.

> Distributions, perhaps? I know we shouldn't use abbreviations in the
> UI, but maybe 'Distros' is an exception? The UI reviewer ought to know.

Okay.

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

iEYEARECAAYFAkunuO0ACgkQ0F+nu1YWqI11fACfXQRXTz9oMdqEZReXFbW/7T4L
iUMAnjYRuLh1ZQQ4O0+gI1uGSYvoeLnc
=6O54
-----END PGP SIGNATURE-----

Hi Aaron and Paul,

This is looking great! First, sorry if I'm re-asking questions that may have already been discussed by you both. But as I don't know the history of the discussions, I'll ask them anyway.

To test this out in the UI, I've put a breakpoint at the end of test_index, and run a devserver using the testrunner config (it'd be great to include instructions like this when requesting a ui review).

The layout looks great - nice and clean. There are a few small things that popped into mind:

The first thing that stands out is that I need to include a %20 in the url to be able to traverse to the recipe... is that intentional? So the url I'm using is:

https://code.launchpad.dev/~chef/+recipe/Cake%20Recipe

Normally we have a human readable displayname, and name is normally cake_recipe, but perhaps you guys decided against this for good reason (we should slugify the displayname for the initial name right?). The mockup for recipe creation assumed it would be a slug: http://people.canonical.com/~michaeln/bfb/create_recipe_v2-collapsed.png

It's unfortunate that branch names are so long - it's the one thing that looks very squashed in your screenshot (looks fine locally with a wider browser of course), but I guess you always have to deal with that :) Is it possible to use a short name here?

Now I'm sure this *is* something you've discussed - but why isn't the traversal for a recipe (or recipes) off the base branch? (is it in case the base-branch is modified?). I'm just looking at the bread-crumbs and wondering whether it will be: "person >> branches >> recipes >> recipe_display_name", which seems a bit weird to me (ie. as opposed to "person >> branches >> branch_short_name >> recipes...").

Similar to Guilherme, I think "Distros" should be "Distribution series" (as it's the series to which you're linking). This is consistent with the product-packages.pt template. There are some cases where we simply use "Series" but that's only when the context is already a distribution.

On a related note: I also agree that we should not need to re-define the buildstate titles/descriptions (what's wrong with "Successfully built 7 minutes ago" in the UI?). I don't see why we can't define a good set of common titles and descriptions (that's why we moved BuildBase and BuildStatus to lp.buildmaster). The main issue seems to be that the current ones (BuildStatus) are still soyuz-specific and I guess changing them will blow this branch out (due to test failures). Can you create a bug and XXX here so that we don't forget. Another option you might consider (still with an XXX) is to augment the current ones here, rather than create a new set with some duplication? (eg. code_specific_descriptions.get(self.context.buildstate, self.context.buildstate.title) or similar). See what you think.

Again, something you've probably already discussed, but in the mockups we were planning on presenting the "Packaging branch" (just a merge statement with the name 'packaging') if one was defined for the recipe (optional, as an advanced recipe might specify multiple packaging related branches). Is it worth adding it to the "Recipe information" portlet? (which should actually be ...

On Monday 22 March 2010 18:12:40 Guilherme Salgado wrote:
> On Mon, 2010-03-22 at 14:09 +0000, Aaron Bentley wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > Guilherme Salgado wrote:
> > >> + @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?
> >
> > That's Soyuz code, and I don't want to change it unnecessarily. I don't
> > know how the existing titles are used, and whether these strings would
> > make sense in that context.
>
> Being able to use the enum's titles both here and in soyuz code will
> make for more consistent UI and avoid this extra code, so we all win.
> Have you checked what the current title is for them? Maybe you could
> use what's there already?
>
> Julian, what do you think?

I think that using the enum titles is absolutely the right thing to do.

There are some templates that use the existing titles (grep for
"buildstate/title"), so any changes should get a UI review to make sure they
still make sense, but I fully support anything that makes the UI more
readable!

> > >> + 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.
> >
> > IMO, SourcePackageRecipe is way too long, and the fewer places we use
> > it, the better. I think the confusion will be short-lived, and less
> > typing is a win.
>
> Ok

FWIW I think getSourcePackageRecipe is better. What if we introduce other
kinds of recipes in the future? And all editors I've used have auto-complete
:)

J

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

Michael Nelson wrote:
> Normally we have a human readable displayname, and name is normally
> cake_recipe, but perhaps you guys decided against this for good
> reason (we should slugify the displayname for the initial name
> right?). The mockup for recipe creation assumed it would be a slug:
> http://people.canonical.com/~michaeln/bfb/create_recipe_v2-collapsed.png

The table does not provide a displayname, so I used the name. It was an
error to use "Cake Recipe" as the name. I should have used
"cake_recipe" or similar.

> It's unfortunate that branch names are so long - it's the one thing
> that looks very squashed in your screenshot (looks fine locally with
> a wider browser of course), but I guess you always have to deal with
> that :) Is it possible to use a short name here?

We use short names automatically when they are available, but most
branches don't have short names.

> Now I'm sure this *is* something you've discussed - but why isn't the
> traversal for a recipe (or recipes) off the base branch?

The canonical URL is already defined in stable/dbstable; it was not
introduced by this branch. Tim proposed the current URL and no one
disagreed, so it was documented in
https://dev.launchpad.net/BuildBranchToArchiveUI/InitialCut

As you note, branch names are long, which is a disadvantage.
Sourcepackagebranches have even longer names. It's also trivial to
change the base branch in a recipe, so varying the name according to the
base branch would make URLs volatile.

> Similar to Guilherme, I think "Distros" should be "Distribution
> series" (as it's the series to which you're linking). This is
> consistent with the product-packages.pt template. There are some
> cases where we simply use "Series" but that's only when the context
> is already a distribution.

Done

> On a related note: I also agree that we should not need to re-define
> the buildstate titles/descriptions (what's wrong with "Successfully
> built 7 minutes ago" in the UI?)

It's not the UI that I was tasked to implement:
http://people.canonical.com/~rockstar/RecipeView.png

>. I don't see why we can't define a
> good set of common titles and descriptions (that's why we moved
> BuildBase and BuildStatus to lp.buildmaster). The main issue seems to
> be that the current ones (BuildStatus) are still soyuz-specific and I
> guess changing them will blow this branch out (due to test failures).

I find the existing ones hard to understand. That's the biggest issue
from my point of view. But they also didn't match the grammar in the UI
mockup.

> Can you create a bug and XXX here so that we don't forget.

Per Julian, I have changed all the enums to match what Code needs.

> Again, something you've probably already discussed, but in the
> mockups we were planning on presenting the "Packaging branch"

We believe that's not required for the initial cut. We are going for
the simplest thing that could possibly work.

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

iEYEARECAAYFAkuo1TAACgkQ0F+nu1YWqI24+QCfUT8AQewwkMkqOV8rbYaLIMch
ncoAnj4u3smwkzElVHGC3Jfa...

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

Julian Edwards wrote:
> I think that using the enum titles is absolutely the right thing to do.

Okay, I have changed the enum titles.

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

iEYEARECAAYFAkuo1bAACgkQ0F+nu1YWqI3aZwCdESwa/wgUV5x9T71HW8fNIIV2
hmwAni08pSXCzY8bhy1fyXabAA7CLmik
=w0Jz
-----END PGP SIGNATURE-----

Thanks for filling me in on all the info Aaron.

On Tue, Mar 23, 2010 at 3:51 PM, Aaron Bentley <email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Michael Nelson wrote:
>> Normally we have a human readable displayname, and name is normally
>> cake_recipe, but perhaps you guys decided against this for good
>> reason (we should slugify the displayname for the initial name
>> right?). The mockup for recipe creation assumed it would be a slug:
>> http://people.canonical.com/~michaeln/bfb/create_recipe_v2-collapsed.png
>
> The table does not provide a displayname, so I used the name.  It was an
> error to use "Cake Recipe" as the name.  I should have used
> "cake_recipe" or similar.

OK, so the create validation will need to enforce this I guess. Great.

>> Now I'm sure this *is* something you've discussed - but why isn't the
>> traversal for a recipe (or recipes) off the base branch?
>
> The canonical URL is already defined in stable/dbstable; it was not
> introduced by this branch.  Tim proposed the current URL and no one
> disagreed, so it was documented in
> https://dev.launchpad.net/BuildBranchToArchiveUI/InitialCut
>
> As you note, branch names are long, which is a disadvantage.
> Sourcepackagebranches have even longer names.  It's also trivial to
> change the base branch in a recipe, so varying the name according to the
> base branch would make URLs volatile.

I got to chat briefly with Tim and he had a few other points too, so
just for the record in case anyone else asks the same question (I'll
try to document this on the wiki as a FAQ or something):
{{{
08:43 < noodles775> The only reason I could think of (which Aaron also
highlighted) is that it would be fragile if the base branch changes
etc., but the original mockups didn't allow you to change the base
branch of a recipe.
08:43 < thumper> noodles775: and I picked something
08:44 < noodles775> thumper: no problem, as long as you guys have
thought about it.
08:44 < thumper> noodles775: but what about changing the name of a branch?
08:44 < thumper> noodles775: or changing the owner
08:44 < thumper> noodles775: both of those change the unique name of the branch
08:44 < thumper> noodles775: and since the branch is accessed through
the id, the recipe url would chnage
08:45 < thumper> noodles775: also ~user/+recipe/name is short
08:45 < noodles775> thumper: I see, whereas both of those things
wouldn't change the recipe url where it is currently? Great.
08:45 * thumper nods
}}}

>
>> On a related note: I also agree that we should not need to re-define
>> the buildstate titles/descriptions (what's wrong with "Successfully
>> built 7 minutes ago" in the UI?)
>
> It's not the UI that I was tasked to implement:
> http://people.canonical.com/~rockstar/RecipeView.png

Right, if Paul specifically chose "Successful build" because he
thought "Successfully built" was incorrect, then I think it's worth
updating the BuildStatus as you've done, to improve it.

>
>>. I don't see why we can't define a
>> good set of common titles and descriptions (that's why we moved
>> BuildBase and BuildStatus to lp.buildmaster). The main issue seems to
>> be that the current ones (BuildSta...