Thanks for the review!


> > === modified file 'lib/lp/soyuz/browser/queue.py'
> > @@ -176,6 +182,15 @@
> >          # Listify to avoid repeated queries.
> >          return list(old_binary_packages)
> >
> > +    def getPackagesetsFor(self, source_package_releases):
> > +        """Find associated `Packagesets`.
> > +
> > +        :param source_package_releases: A sequence of
> `SourcePackageRelease`s.
> > +        """
> > +        sprs = [spr for spr in source_package_releases if spr is not
> None]
> > +        return getUtility(IPackagesetSet).getForPackages(
> > +            self.context, set(spr.sourcepackagenameID for spr in
> sprs))
> 
> Any reason you are not constructing a set directly in the line above?

I spelled this in various ways over time, but found that breaking it up in this way keeps both individual lines nicely legible.


> > @@ -217,9 +241,11 @@
> >          self.old_binary_packages = self.calculateOldBinaries(
> >              binary_package_names)
> >
> > +        package_sets = self.getPackagesetsFor(source_sprs)
> 
> I tried to restrain myself, but I really hate "Packageset" spelling.  I
> am assuming this was a decision made some time ago, but I always expect
> it to be PackageSet, even though they might imply some other things in
> Launchpad.
> 
> (The fact that you are spelling it as package_sets when lowercase seems
> to imply that you agree)

I do.  I got it wrong many times while working on this branch.


> > @@ -415,8 +441,9 @@
> >  class CompletePackageUpload:
> >      """A decorated `PackageUpload` including sources, builds and
> packages.
> >
> > -    Some properties of PackageUpload are cached here to reduce the
> number
> > -    of queries that the +queue template has to make.
> > +    This acts effectively as a view for package uploads.  Some
> properties of
> > +    the class are cached here to reduce the number of queries that
> the +queue
> > +    template has to make.  Others are added here exclusively.
> 
> What I don't understand is why is this not a view in the first place?

With all the logic in the TAL, this class started out purely as a decorator for bulk-fetching some attributes on PackageUpload.


> > @@ -478,11 +511,87 @@
> >          return self.context.changesfile
> >
> >      @property
> > -    def package_sets(self):
> > -        assert self.sourcepackagerelease, \
> > -            "Can only be used on a source upload."
> > -        return ' '.join(sorted(ps.name for ps in
> > -            getUtility(IPackagesetSet).setsIncludingSource(
> > -                self.sourcepackagerelease.sourcepackagename,
> > -                distroseries=self.distroseries,
> > -                direct_inclusion=True)))
> 
> Is self.contains_source equivalent to self.sourcepackagerelease?  Any
> reason you are dropping the assertion and replacing it with empty
> package_sets?

It's not equivalent, no.  The reason I removed the assertion is that what it says is no longer true.  We wish to display this information for sync uploads as well, though there it will need to be retrieved in different ways.


> > +    def composeIcon(self, alt, icon, title=None):
> > +        """Compose an icon for the package's icon list."""
> > +        # These should really be sprites!
> > +        if title is None:
> > +            title = alt
> > +        return '<img alt="[%s]" src="/@@/%s" title="%s" />' % (
> > +            cgi.escape(alt, quote=True),
> > +            icon,
> > +            cgi.escape(title, quote=True),
> > +            )
> 
> Any reason you are not using "structured"[*] in here instead?  I am
> guessing it's for quote=True parameter (not present on 'structured'),
> but perhaps we can always set it there as well?

That's a thought.  Frankly I wasn't even sure whether quote escaping would be valid outside of attributes, but it seems to turn " into &quot;  I would expect it to break some tests though!


> [*] canonical.launchpad.webapp.menu.structured
> 
> > +    def composeNameAndChangesLink(self):
> > +        """Compose HTML: upload name and link to changes file."""
> > +        raw_displayname = self.displayname
> > +        displayname = cgi.escape(raw_displayname)
> > +        if self.pending_delayed_copy or self.changesfile is None:
> > +            return displayname
> > +        else:
> > +            return '<a href="%s" title="Changes file for %s">%s</a>'
> % (
> > +                self.changesfile.http_url,
> > +                cgi.escape(self.displayname, quote=True),
> > +                displayname)
> 
> Definitely use structured because of the displayname (or at least escape
> it directly).

I did escape it, at the top of the method!


> I see you have tests for basic escaping, but I wonder what happens when
> displayname is something like "<script>alert("!");</script>"?

cgi.escape turns that into "&lt;script&gt;alert(&quot;!&quot;);&lt;/script&gt;".  A little dangerous to the eyes, but little else.  :-)


> > === modified file 'lib/lp/soyuz/interfaces/files.py'
> ...
> > @@ -57,11 +59,16 @@
> >      id = Int(
> >              title=_('ID'), required=True, readonly=True,
> >              )
> > -    sourcepackagerelease = Int(
> > -            title=_('The sourcepackagerelease being published'),
> > +
> > +    sourcepackagerelease = Reference(
> > +        ISourcePackageRelease,
> > +        title=_("The source package release being published"),
> > +        required=True, readonly=False)
> > +
> > +    sourcepackagereleaseID = Int(
> 
> Most people seem to use *_id spelling when explicitely defining it
> (as opposed to the default storm-assigned *ID).  Is the storm spelling
> used in enough queries that it'd be hard to follow our naming guidelines
> here?

No, I just figured it'd serve as a reminder to port the class to Storm!


> > === modified file 'lib/lp/soyuz/model/packageset.py'

> >      def setsIncludingSource(self, sourcepackagename,
> distroseries=None,
> >                              direct_inclusion=False):
> >          """See `IPackagesetSet`."""
> >          sourcepackagename =
> self._nameToSourcePackageName(sourcepackagename)
> >
> > -        if direct_inclusion == False:
> > +        if direct_inclusion:
> > +            query = '''
> > +                SELECT pss.packageset FROM packagesetsources pss
> > +                WHERE pss.sourcepackagename = ?
> > +            '''
> > +        else:
> >              query = '''
> >                  SELECT fpsi.parent
> >                  FROM packagesetsources pss, flatpackagesetinclusion
> fpsi
> >                  WHERE pss.sourcepackagename = ?
> >                  AND pss.packageset = fpsi.child
> >              '''
> > -        else:
> > -            query = '''
> > -                SELECT pss.packageset FROM packagesetsources pss
> > -                WHERE pss.sourcepackagename = ?
> > -            '''
> >          store = IStore(Packageset)
> >          psets = SQL(query, (sourcepackagename.id,))
> >          clauses = [Packageset.id.is_in(psets)]
> 
> Any reason you haven't migrated this to Storm as well?

Laziness!  The branch was oversized already.  I'll probably get the urge at some point in the future if nobody else does.


> > === added file 'lib/lp/soyuz/model/packagesetsources.py'
> ...
> > +class PackagesetSources(Storm):
> > +    """Linking table: which packages are in a package set?"""
> > +    # This table is largely managed from Packageset, but also
> directly
> > +    # accessed from other places.
> > +
> > +    __storm_table__ = 'PackagesetSources'
> > +
> > +    # There's a vestigial id as well, a holdover from the SQLObject
> > +    # days.  Nobody seems to use it.  The only key that matters is
> > +    # (packageset, sourcepackagename).
> 
> How about an XXX with a bug to drop the column? :)

Good idea!  Consider it done.


Jeroen