Merge lp:~jtv/launchpad/bug-798521 into lp:launchpad

[Jeroen T. Vermeulen]

[r=danilo][bug=798521] Clean up +queue page generation.

= Summary =

This cleans up some technical debt related to the +queue page, in penance for my first adding to it in a hurry.

== Implementation details ==

Here's a quick run-through of what I changed (a bit of a grab-bag, but it's all connected):

Added a Storm class for the PackagesetSources linking table, which was previously hacked into the python ad-hoc.

Batch-fetched any SourcePackageReleases associated with the PackageUploads on the page.

Batch-fetched any Packagesets associated with the SourcePackages related to SourcePackageReleases for source uploads.

Extended CompletePackageUpload to be less like a "PackageUpload with some data pre-fetched" and more like a "view class for PackageUploads."

Moved some small-scale TAL logic into CompletePackageUpload (but not so much as to split the <table> structure across TAL and python).

Unit-tested CompletePackageUpload.

Modernized the PackagesetSet unit test, replacing its setUp with specific factory calls.

== Tests ==

{{{
./bin/test -vvc soyuz.browser.tests.test_queue
./bin/test -vvc soyuz.tests.test_packageset
./bin/test -vvc soyuz -t stories
}}}

== Demo and Q/A ==

The +queue page should still work, and be no less efficient. (Hopefully it will be slightly more efficient).

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/browser/tests/test_queue.py
  lib/lp/soyuz/templates/distroseries-queue.pt
  lib/lp/soyuz/model/packagesetsources.py
  lib/lp/soyuz/browser/queue.py
  lib/lp/soyuz/interfaces/packageset.py
  lib/lp/soyuz/model/packageset.py
  lib/lp/soyuz/tests/test_packageset.py
  lib/lp/registry/model/distroseriesdifference.py

[Данило Шеган (danilo)]

Hi Jeroen,

Thanks for doing this. As with any refactoring and cleanup, it always
invites more of the same. Please ignore any of such requests below if
you don't feel like doing it. The only thing I care about is XSS attack
vectors, so please ensure that no trouble can come out of it. As long
as that's fine, feel free to land.

  review approve

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

> + packageuploadsources = load_referencing(
> + source_sprs = load_related(

Didn't know about these, cool stuff :)

> @@ -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)

> @@ -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?

> @@ -459,6 +486,12 @@
> else:
> self.sourcepackagerelease = None
>
> + if self.contains_source:
> + self.package_sets = package_sets.get(
> + self.sourcepackagerelease.sourcepackagenameID, [])
> + else:
> + self.package_sets = []
> +
> @property
> def pending_delayed_copy(self):
> """Whether the context is a delayed-copy pending
processing."""
> @@ -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 equival...

review: Approve

[Jeroen T. Vermeulen (jtv)]

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

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2011-06-14 13:47:51 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2011-06-22 13:09:13 +0000
@@ -97,6 +97,7 @@
     DistroSeriesSourcePackageRelease,
     )
 from lp.soyuz.model.packageset import Packageset
+from lp.soyuz.model.packagesetsources import PackagesetSources
 from lp.soyuz.model.publishing import SourcePackagePublishingHistory
 from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 
@@ -205,7 +206,6 @@
     if len(dsds) == 0:
         return {}
 
-    PackagesetSources = Table("PackageSetSources")
     FlatPackagesetInclusion = Table("FlatPackagesetInclusion")
 
     tables = IStore(Packageset).using(
@@ -213,18 +213,17 @@
         PackagesetSources, FlatPackagesetInclusion)
     results = tables.find(
         (DistroSeriesDifference.id, Packageset),
-        Column("packageset", PackagesetSources) == (
-            Column("child", FlatPackagesetInclusion)),
+        PackagesetSources.packageset_id == Column(
+            "child", FlatPackagesetInclusion),
         Packageset.distroseries_id == (
             DistroSeriesDifference.parent_series_id if in_parent else
             DistroSeriesDifference.derived_series_id),
         Column("parent", FlatPackagesetInclusion) == Packageset.id,
-        Column("sourcepackagename", PackagesetSources) == (
+        PackagesetSources.sourcepackagename_id == (
             DistroSeriesDifference.source_package_name_id),
         DistroSeriesDifference.id.is_in(dsd.id for dsd in dsds))
     results = results.order_by(
-        Column("sourcepackagename", PackagesetSources),
-        Packageset.name)
+        PackagesetSources.sourcepackagename_id, Packageset.name)
 
     grouped = defaultdict(list)
     for dsd_id, packageset in results:

=== modified file 'lib/lp/soyuz/browser/queue.py'
--- lib/lp/soyuz/browser/queue.py	2011-06-16 10:44:00 +0000
+++ lib/lp/soyuz/browser/queue.py	2011-06-22 13:09:13 +0000
@@ -9,6 +9,7 @@
     'QueueItemsView',
     ]
 
+import cgi
 import operator
 
 from lazr.delegates import delegates
@@ -22,6 +23,10 @@
     NotFoundError,
     UnexpectedFormData,
     )
+from lp.services.database.bulk import (
+    load_referencing,
+    load_related,
+    )
 from lp.soyuz.enums import (
     PackagePublishingPriority,
     PackageUploadStatus,
@@ -41,6 +46,7 @@
     QueueInconsistentStateError,
     )
 from lp.soyuz.interfaces.section import ISectionSet
+from lp.soyuz.model.queue import PackageUploadSource
 
 
 QUEUE_SIZE = 30
@@ -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))
+
     def decoratedQueueBatch(self):
         """Return the current batch, converted to decorated objects.
 
@@ -183,6 +198,9 @@
         CompletePackageUpload.  This avoids many additional SQL queries
         in the +queue template.
         """
+        # Avoid circular imports.
+        from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
+
         uploads = list(self.batchnav.currentBatch())
 
         if len(uploads) == 0:
@@ -196,9 +214,15 @@
                     upload.status != PackageUploadStatus.DONE)]
         binary_file_set = getUtility(IBinaryPackageFileSet)
         binary_files = binary_file_set.getByPackageUploadIDs(upload_ids)
+        packageuploadsources = load_referencing(
+            PackageUploadSource, uploads, ['packageuploadID'])
         source_file_set = getUtility(ISourcePackageReleaseFileSet)
         source_files = source_file_set.getByPackageUploadIDs(upload_ids)
 
+        source_sprs = load_related(
+            SourcePackageRelease, packageuploadsources,
+            ['sourcepackagereleaseID'])
+
         # Get a dictionary of lists of binary files keyed by upload ID.
         package_upload_builds_dict = self.builds_dict(
             upload_ids, binary_files)
@@ -217,9 +241,11 @@
         self.old_binary_packages = self.calculateOldBinaries(
             binary_package_names)
 
+        package_sets = self.getPackagesetsFor(source_sprs)
+
         return [
             CompletePackageUpload(
-                item, build_upload_files, source_upload_files)
+                item, build_upload_files, source_upload_files, package_sets)
             for item in uploads]
 
     def is_new(self, binarypackagerelease):
@@ -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.
     """
     # These need to be predeclared to avoid delegates taking them over.
     # Would be nice if there was a way of allowing writes to just work
@@ -433,7 +460,7 @@
     delegates(IPackageUpload)
 
     def __init__(self, packageupload, build_upload_files,
-                 source_upload_files):
+                 source_upload_files, package_sets):
         self.pocket = packageupload.pocket
         self.date_created = packageupload.date_created
         self.context = packageupload
@@ -459,6 +486,12 @@
         else:
             self.sourcepackagerelease = None
 
+        if self.contains_source:
+            self.package_sets = package_sets.get(
+                self.sourcepackagerelease.sourcepackagenameID, [])
+        else:
+            self.package_sets = []
+
     @property
     def pending_delayed_copy(self):
         """Whether the context is a delayed-copy pending processing."""
@@ -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)))
+    def display_package_sets(self):
+        """Package sets, if any, for display on the +queue page."""
+        if self.contains_source:
+            return ' '.join(sorted(
+                packageset.name for packageset in self.package_sets))
+        else:
+            return ""
+
+    @property
+    def display_component(self):
+        """Component name, if any, for display on the +queue page."""
+        if self.contains_source:
+            return self.component_name.lower()
+        else:
+            return ""
+
+    @property
+    def display_section(self):
+        """Section name, if any, for display on the +queue page."""
+        if self.contains_source:
+            return self.section_name.lower()
+        else:
+            return ""
+
+    @property
+    def display_priority(self):
+        """Priority name, if any, for display on the +queue page."""
+        if self.contains_source:
+            return self.sourcepackagerelease.urgency.name.lower()
+        else:
+            return ""
+
+    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),
+            )
+
+    def composeIconList(self):
+        """List icons that should be shown for this upload."""
+        ddtp = "Debian Description Translation Project Indexes"
+        potential_icons = [
+            (self.contains_source, ("Source", 'package-source')),
+            (self.contains_build, ("Build", 'package-binary', "Binary")),
+            (self.contains_translation, ("Translation", 'translation-file')),
+            (self.contains_installer, ("Installer", 'ubuntu-icon')),
+            (self.contains_upgrader, ("Upgrader", 'ubuntu-icon')),
+            (self.contains_ddtp, (ddtp, 'ubuntu-icon')),
+            ]
+        return [
+            self.composeIcon(*details)
+            for condition, details in potential_icons
+                if condition]
+
+    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)
+
+    @property
+    def icons_and_name(self):
+        """Icon list and name, linked to changes file if appropriate."""
+        iconlist_id = "queue%d-iconlist" % self.id
+        icons = self.composeIconList()
+        link = self.composeNameAndChangesLink()
+        return """
+            <div id="%s">
+              %s
+              %s
+              (%s)
+            </div>
+            """ % (iconlist_id, '\n'.join(icons), link, self.displayarchs)

=== modified file 'lib/lp/soyuz/browser/tests/test_queue.py'
--- lib/lp/soyuz/browser/tests/test_queue.py	2011-06-16 13:21:14 +0000
+++ lib/lp/soyuz/browser/tests/test_queue.py	2011-06-22 13:09:13 +0000
@@ -5,6 +5,8 @@
 
 __metaclass__ = type
 
+import cgi
+from lxml import html
 import transaction
 from zope.component import (
     getUtility,
@@ -12,8 +14,12 @@
     )
 
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
-from canonical.testing.layers import LaunchpadFunctionalLayer
+from canonical.testing.layers import (
+    LaunchpadFunctionalLayer,
+    LaunchpadZopelessLayer,
+    )
 from lp.archiveuploader.tests import datadir
+from lp.soyuz.browser.queue import CompletePackageUpload
 from lp.soyuz.enums import PackageUploadStatus
 from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
 from lp.soyuz.interfaces.queue import IPackageUploadSet
@@ -208,8 +214,8 @@
             upload.distroseries.main_archive)
         with person_logged_in(queue_admin):
             view = self.makeView(upload.distroseries, queue_admin)
-            html = view()
-        self.assertIn(upload.package_name, html)
+            html_text = view()
+        self.assertIn(upload.package_name, html_text)
 
     def test_view_renders_build_upload(self):
         login(ADMIN_EMAIL)
@@ -218,8 +224,8 @@
             upload.distroseries.main_archive)
         with person_logged_in(queue_admin):
             view = self.makeView(upload.distroseries, queue_admin)
-            html = view()
-        self.assertIn(upload.package_name, html)
+            html_text = view()
+        self.assertIn(upload.package_name, html_text)
 
     def test_view_renders_copy_upload(self):
         login(ADMIN_EMAIL)
@@ -228,5 +234,166 @@
             upload.distroseries.main_archive)
         with person_logged_in(queue_admin):
             view = self.makeView(upload.distroseries, queue_admin)
-            html = view()
-        self.assertIn(upload.package_name, html)
+            html_text = view()
+        self.assertIn(upload.package_name, html_text)
+
+
+class TestCompletePackageUpload(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def makeCompletePackageUpload(self, upload=None, build_upload_files=None,
+                                  source_upload_files=None,
+                                  package_sets=None):
+        if upload is None:
+            upload = self.factory.makeSourcePackageUpload()
+        if build_upload_files is None:
+            build_upload_files = {}
+        if source_upload_files is None:
+            source_upload_files = {}
+        if package_sets is None:
+            package_sets = {}
+        return CompletePackageUpload(
+            upload, build_upload_files, source_upload_files, package_sets)
+
+    def mapPackageSets(self, upload, package_sets=None):
+        if package_sets is None:
+            package_sets = [self.factory.makePackageset(
+                distroseries=upload.distroseries)]
+        spn = upload.sourcepackagerelease.sourcepackagename
+        return {spn.id: package_sets}
+
+    def test_display_package_sets_returns_source_upload_packagesets(self):
+        upload = self.factory.makeSourcePackageUpload()
+        package_sets = self.mapPackageSets(upload)
+        complete_upload = self.makeCompletePackageUpload(
+            upload, package_sets=package_sets)
+        self.assertEqual(
+            package_sets.values()[0][0].name,
+            complete_upload.display_package_sets)
+
+    def test_display_package_sets_returns_empty_for_non_source_upload(self):
+        upload = self.factory.makeBuildPackageUpload()
+        complete_upload = self.makeCompletePackageUpload(
+            upload, package_sets=self.mapPackageSets(upload))
+        self.assertEqual("", complete_upload.display_package_sets)
+
+    def test_display_package_sets_sorts_by_name(self):
+        complete_upload = self.makeCompletePackageUpload()
+        distroseries = complete_upload.distroseries
+        complete_upload.package_sets = [
+            self.factory.makePackageset(distroseries=distroseries, name=name)
+            for name in [u'ccc', u'aaa', u'bbb']]
+        self.assertEqual("aaa bbb ccc", complete_upload.display_package_sets)
+
+    def test_display_component_returns_source_upload_component_name(self):
+        complete_upload = self.makeCompletePackageUpload()
+        self.assertEqual(
+            complete_upload.sourcepackagerelease.component.name.lower(),
+            complete_upload.display_component)
+
+    def test_display_component_returns_empty_for_non_source_upload(self):
+        complete_upload = self.makeCompletePackageUpload(
+            self.factory.makeBuildPackageUpload())
+        self.assertEqual('', complete_upload.display_component)
+
+    def test_display_section_returns_source_upload_section_name(self):
+        complete_upload = self.makeCompletePackageUpload()
+        self.assertEqual(
+            complete_upload.sourcepackagerelease.section.name.lower(),
+            complete_upload.display_section)
+
+    def test_display_section_returns_empty_for_non_source_upload(self):
+        complete_upload = self.makeCompletePackageUpload(
+            self.factory.makeBuildPackageUpload())
+        self.assertEqual('', complete_upload.display_section)
+
+    def test_display_priority_returns_source_upload_priority(self):
+        complete_upload = self.makeCompletePackageUpload()
+        self.assertEqual(
+            complete_upload.sourcepackagerelease.urgency.name.lower(),
+            complete_upload.display_priority)
+
+    def test_display_priority_returns_empty_for_non_source_upload(self):
+        complete_upload = self.makeCompletePackageUpload(
+            self.factory.makeBuildPackageUpload())
+        self.assertEqual('', complete_upload.display_priority)
+
+    def test_composeIcon_produces_image_tag(self):
+        alt = self.factory.getUniqueString()
+        icon = self.factory.getUniqueString() + ".png"
+        title = self.factory.getUniqueString()
+        html_text = self.makeCompletePackageUpload().composeIcon(
+            alt, icon, title)
+        img = html.fromstring(html_text)
+        self.assertEqual("img", img.tag)
+        self.assertEqual("[%s]" % alt, img.get("alt"))
+        self.assertEqual("/@@/" + icon, img.get("src"))
+        self.assertEqual(title, img.get("title"))
+
+    def test_composeIcon_title_defaults_to_alt_text(self):
+        alt = self.factory.getUniqueString()
+        icon = self.factory.getUniqueString() + ".png"
+        html_text = self.makeCompletePackageUpload().composeIcon(alt, icon)
+        img = html.fromstring(html_text)
+        self.assertEqual(alt, img.get("title"))
+
+    def test_composeIcon_escapes_alt_and_title(self):
+        alt = 'alt"&'
+        icon = self.factory.getUniqueString() + ".png"
+        title = 'title"&'
+        html_text = self.makeCompletePackageUpload().composeIcon(
+            alt, icon, title)
+        img = html.fromstring(html_text)
+        self.assertEqual("[%s]" % alt, img.get("alt"))
+        self.assertEqual(title, img.get("title"))
+
+    def test_composeIconList_produces_icons(self):
+        icons = self.makeCompletePackageUpload().composeIconList()
+        self.assertNotEqual([], icons)
+        self.assertEqual('img', html.fromstring(icons[0]).tag)
+
+    def test_composeIconList_produces_icons_conditionally(self):
+        complete_upload = self.makeCompletePackageUpload()
+        base_count = len(complete_upload.composeIconList())
+        complete_upload.contains_build = True
+        new_count = len(complete_upload.composeIconList())
+        self.assertEqual(base_count + 1, new_count)
+
+    def test_composeNameAndChangesLink_does_not_link_if_no_changes_file(self):
+        upload = self.factory.makeCopyJobPackageUpload()
+        complete_upload = self.makeCompletePackageUpload(upload)
+        self.assertEqual(
+            complete_upload.displayname,
+            complete_upload.composeNameAndChangesLink())
+
+    def test_composeNameAndChangesLink_links_to_changes_file(self):
+        complete_upload = self.makeCompletePackageUpload()
+        link = html.fromstring(complete_upload.composeNameAndChangesLink())
+        self.assertEqual(
+            complete_upload.changesfile.http_url, link.get("href"))
+
+    def test_composeNameAndChangesLink_escapes_nonlinked_display_name(self):
+        filename = 'name"&name'
+        upload = self.factory.makeCustomPackageUpload(filename=filename)
+        # Stop nameAndChangesLink from producing a link.
+        upload.changesfile = None
+        complete_upload = self.makeCompletePackageUpload(upload)
+        self.assertIn(
+            cgi.escape(filename), complete_upload.composeNameAndChangesLink())
+
+    def test_composeNameAndChangesLink_escapes_name_in_link(self):
+        filename = 'name"&name'
+        upload = self.factory.makeCustomPackageUpload(filename=filename)
+        complete_upload = self.makeCompletePackageUpload(upload)
+        link = html.fromstring(complete_upload.composeNameAndChangesLink())
+        self.assertIn(filename, link.get("title"))
+        self.assertEqual(filename, link.text)
+
+    def test_icons_and_name_composes_icons_and_link_and_archs(self):
+        complete_upload = self.makeCompletePackageUpload()
+        icons_and_name = html.fromstring(complete_upload.icons_and_name)
+        self.assertNotEqual(None, icons_and_name.find("img"))
+        self.assertNotEqual(None, icons_and_name.find("a"))
+        self.assertIn(
+            complete_upload.displayarchs, ' '.join(icons_and_name.itertext()))

=== modified file 'lib/lp/soyuz/interfaces/files.py'
--- lib/lp/soyuz/interfaces/files.py	2010-08-20 20:31:18 +0000
+++ lib/lp/soyuz/interfaces/files.py	2011-06-22 13:09:13 +0000
@@ -14,6 +14,7 @@
     'ISourcePackageReleaseFileSet',
     ]
 
+from lazr.restful.fields import Reference
 from zope.interface import Interface
 from zope.schema import (
     Bool,
@@ -21,6 +22,7 @@
     )
 
 from canonical.launchpad import _
+from lp.soyuz.interfaces.sourcepackagerelease import ISourcePackageRelease
 
 
 class IBinaryPackageFile(Interface):
@@ -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(
+            title=_('ID of the source package release being published'),
             required=True,
-            readonly=False,
-            )
+            readonly=False)
 
     libraryfile = Int(
             title=_('The library file alias for this file'), required=True,

=== modified file 'lib/lp/soyuz/interfaces/packageset.py'
--- lib/lp/soyuz/interfaces/packageset.py	2011-06-16 18:51:45 +0000
+++ lib/lp/soyuz/interfaces/packageset.py	2011-06-22 13:09:13 +0000
@@ -445,6 +445,17 @@
         :return: An iterable collection of `IPackageset` instances.
         """
 
+    def getForPackages(distroseries, sourcepackagename_ids):
+        """Get `Packagesets` that directly contain the given packages.
+
+        :param distroseries: `DistroSeries` to look in.  Only packagesets for
+            this series will be returned.
+        :param sourcepackagename_ids: A sequence of `SourcePackageName` ids.
+            Only packagesets for these package names will be returned.
+        :return: A dict mapping `SourcePackageName` ids to lists of their
+            respective packagesets, in no particular order.
+        """
+
     @operation_parameters(
         sourcepackagename=TextLine(
             title=_('Source package name'), required=True),

=== modified file 'lib/lp/soyuz/model/packageset.py'
--- lib/lp/soyuz/model/packageset.py	2011-03-22 11:30:13 +0000
+++ lib/lp/soyuz/model/packageset.py	2011-06-22 13:09:13 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -36,6 +36,7 @@
     NoSuchPackageSet,
     )
 from lp.soyuz.model.packagesetgroup import PackagesetGroup
+from lp.soyuz.model.packagesetsources import PackagesetSources
 
 
 def _order_result_set(result_set):
@@ -427,23 +428,36 @@
             source_name = getUtility(ISourcePackageNameSet)[source_name]
         return source_name
 
+    def getForPackages(self, distroseries, sourcepackagename_ids):
+        """See `IPackagesetSet`."""
+        tuples = IStore(Packageset).find(
+            (PackagesetSources.sourcepackagename_id, Packageset),
+            Packageset.id == PackagesetSources.packageset_id,
+            Packageset.distroseries == distroseries,
+            PackagesetSources.sourcepackagename_id.is_in(
+                sourcepackagename_ids))
+        packagesets_by_package = {}
+        for package, packageset in tuples:
+            packagesets_by_package.setdefault(package, []).append(packageset)
+        return packagesets_by_package
+
     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)]

=== added file 'lib/lp/soyuz/model/packagesetsources.py'
--- lib/lp/soyuz/model/packagesetsources.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/model/packagesetsources.py	2011-06-22 13:09:13 +0000
@@ -0,0 +1,41 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""The `PackagesetSources` linking table.
+
+This table associates `Packageset`s with `SourcePackageName`s.
+"""
+
+__metaclass__ = type
+__all__ = [
+    'PackagesetSources',
+    ]
+
+from storm.locals import (
+    Int,
+    Reference,
+    Storm,
+    )
+
+
+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).
+    # XXX JeroenVermeulen 2011-06-22, bug=800677: Drop the id column.
+    __storm_primary__ = (
+        'packageset_id',
+        'sourcepackagename_id',
+        )
+
+    packageset_id = Int(name='packageset')
+    packageset = Reference(packageset_id, 'Packageset.id')
+    sourcepackagename_id = Int(name='sourcepackagename')
+    sourcepackagename = Reference(
+        sourcepackagename_id, 'SourcePackageName.id')

=== modified file 'lib/lp/soyuz/templates/distroseries-queue.pt'
--- lib/lp/soyuz/templates/distroseries-queue.pt	2011-06-16 13:21:14 +0000
+++ lib/lp/soyuz/templates/distroseries-queue.pt	2011-06-22 13:09:13 +0000
@@ -89,33 +89,30 @@
                      <input type="checkbox" name="QUEUE_ID"
                             tal:attributes="value packageupload/id"/>
                 </td>
-                <td style="padding-top: 5px">
-                  <metal:iconlist use-macro="template/macros/package-iconlist"/>
+                <td
+                  style="padding-top: 5px"
+                  tal:content="structure packageupload/icons_and_name">
                 </td>
 
                 <td style="padding-top: 5px"
                     tal:content="packageupload/displayversion">2.0.17-6
                 </td>
                 <tal:is_source define="is_source packageupload/contains_source">
-                  <td style="padding-top: 5px">
-                    <tal:component condition="is_source"
-                         content="packageupload/sourcepackagerelease/component/name/lower">
-                    </tal:component>
-                  </td>
-                  <td style="padding-top: 5px">
-                    <tal:section condition="is_source"
-                         content="packageupload/sourcepackagerelease/section/name/lower">
-                    </tal:section>
-                  </td>
-                  <td style="padding-top: 5px">
-                    <tal:priority condition="is_source"
-                         content="packageupload/sourcepackagerelease/urgency/name/lower">
-                    </tal:priority>
-                  </td>
-                  <td style="padding-top: 5px">
-                    <tal:packagesets condition="is_source"
-                         content="packageupload/package_sets">
-                    </tal:packagesets>
+                  <td
+                    style="padding-top: 5px"
+                    tal:content="packageupload/display_component">
+                  </td>
+                  <td
+                    style="padding-top: 5px"
+                    tal:content="packageupload/display_section">
+                  </td>
+                  <td
+                    style="padding-top: 5px"
+                    tal:content="packageupload/display_priority">
+                  </td>
+                  <td
+                    style="padding-top: 5px"
+                    tal:content="packageupload/display_package_sets">
                   </td>
                 </tal:is_source>
                 <td style="padding-top: 5px"
@@ -232,7 +229,9 @@
           style="display:none">
         <td/>
         <td tal:condition="view/availableActions"/>
-        <td tal:define="libraryfilealias file/libraryfile">
+        <td
+          tal:define="libraryfilealias file/libraryfile"
+          tal:condition="libraryfilealias">
           <metal:file use-macro="template/macros/package-file"/>
         </td>
         <td colspan="6"/>
@@ -298,59 +297,15 @@
     :libraryfilealias: A LibraryFileAlias to link to. If it is expired,
       no link will be created.
   </tal:comment>
-  <tal:not-none condition="libraryfilealias">
-    <tal:unexpired tal:condition="libraryfilealias/content">
-      <a tal:attributes="href libraryfilealias/http_url">
-        <tal:filename replace="libraryfilealias/filename"/>
-      </a>
-      (<span tal:replace="libraryfilealias/content/filesize/fmt:bytes" />)
-    </tal:unexpired>
-    <tal:expired tal:condition="not:libraryfilealias/content">
-      <span tal:content="libraryfilealias/filename"/>
-    </tal:expired>
-  </tal:not-none>
-</metal:macro>
-
-<metal:macro define-macro="package-iconlist">
-  <tal:comment replace="nothing">
-    This macro expects the following variables defined:
-    :packageupload: A PackageUpload record for which we display icons.
-  </tal:comment>
-
-  <div tal:attributes="id string:queue${packageupload/id}-iconlist">
-    <img tal:condition="packageupload/contains_source"
-         alt="[Source]" src="/@@/package-source" title="Source"/>
-    <img tal:condition="packageupload/contains_build"
-         alt="[Build]" src="/@@/package-binary" title="Binary"/>
-    <img tal:condition="packageupload/contains_translation"
-         alt="[Translation]" src="/@@/translation-file"
-         title="Translation"/>
-    <img tal:condition="packageupload/contains_installer"
-         alt="[Installer]" src="/@@/ubuntu-icon"
-         title="Installer"/>
-    <img tal:condition="packageupload/contains_upgrader"
-         alt="[Upgrader]" src="/@@/ubuntu-icon"
-         title="Upgrader"/>
-    <img tal:condition="packageupload/contains_ddtp"
-         alt="[Debian Description Translation Project Indexes]"
-         src="/@@/ubuntu-icon"
-         title="Debian Description Translation Project Indexes"/>
-    <tal:not-delayed condition="not: packageupload/pending_delayed_copy">
-      <a tal:condition="packageupload/changesfile"
-         tal:content="packageupload/displayname"
-         tal:attributes="
-           href packageupload/changesfile/http_url;
-           title string:Changes file for ${packageupload/displayname};">
-      </a>
-      <tal:no-changes-file
-        condition="not: packageupload/changesfile"
-        replace="packageupload/displayname"/>
-    </tal:not-delayed>
-    <tal:pending_delayed_copy_title
-      condition="packageupload/pending_delayed_copy"
-      replace="packageupload/displayname" />
-    <tal:arches replace="string: (${packageupload/displayarchs})"/>
-  </div>
+  <tal:unexpired tal:condition="libraryfilealias/content">
+    <a tal:attributes="href libraryfilealias/http_url">
+      <tal:filename replace="libraryfilealias/filename"/>
+    </a>
+    (<span tal:replace="libraryfilealias/content/filesize/fmt:bytes" />)
+  </tal:unexpired>
+  <tal:expired tal:condition="not:libraryfilealias/content">
+    <span tal:content="libraryfilealias/filename"/>
+  </tal:expired>
 </metal:macro>
 
 </metal:macros>

=== modified file 'lib/lp/soyuz/tests/test_packageset.py'
--- lib/lp/soyuz/tests/test_packageset.py	2011-03-22 11:30:13 +0000
+++ lib/lp/soyuz/tests/test_packageset.py	2011-06-22 13:09:13 +0000
@@ -1,11 +1,11 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test Packageset features."""
 
 from zope.component import getUtility
 
-from canonical.testing.layers import LaunchpadZopelessLayer
+from canonical.testing.layers import ZopelessDatabaseLayer
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.series import SeriesStatus
 from lp.soyuz.interfaces.packageset import (
@@ -17,128 +17,154 @@
 
 class TestPackagesetSet(TestCaseWithFactory):
 
-    layer = LaunchpadZopelessLayer
-
-    def setUp(self):
-        """Setup a distribution with multiple distroseries."""
-        super(TestPackagesetSet, self).setUp()
-        self.distribution = getUtility(IDistributionSet).getByName(
-            'ubuntu')
-        self.distroseries_current = self.distribution.currentseries
-        self.distroseries_experimental = self.factory.makeDistroRelease(
-            distribution = self.distribution, name="experimental",
+    layer = ZopelessDatabaseLayer
+
+    def getUbuntu(self):
+        """Get the Ubuntu `Distribution`."""
+        return getUtility(IDistributionSet).getByName('ubuntu')
+
+    def makeExperimentalSeries(self):
+        """Create an experimental Ubuntu `DistroSeries`."""
+        return self.factory.makeDistroSeries(
+            distribution=self.getUbuntu(), name="experimental",
             status=SeriesStatus.EXPERIMENTAL)
 
-        self.person1 = self.factory.makePerson(
-            name='hacker', displayname=u'Happy Hacker')
-
-        self.packageset_set = getUtility(IPackagesetSet)
-
     def test_new_defaults_to_current_distroseries(self):
         # If the distroseries is not provided, the current development
         # distroseries will be assumed.
-        packageset = self.packageset_set.new(
-            u'kernel', u'Contains all OS kernel packages', self.person1)
-
+        packageset = getUtility(IPackagesetSet).new(
+            self.factory.getUniqueUnicode(), self.factory.getUniqueUnicode(),
+            self.factory.makePerson())
         self.failUnlessEqual(
-            self.distroseries_current, packageset.distroseries)
+            self.getUbuntu().currentseries, packageset.distroseries)
 
     def test_new_with_specified_distroseries(self):
         # A distroseries can be provided when creating a package set.
-        packageset = self.packageset_set.new(
-            u'kernel', u'Contains all OS kernel packages', self.person1,
-            distroseries=self.distroseries_experimental)
-
-        self.failUnlessEqual(
-            self.distroseries_experimental, packageset.distroseries)
+        experimental_series = self.makeExperimentalSeries()
+        packageset = getUtility(IPackagesetSet).new(
+            self.factory.getUniqueUnicode(), self.factory.getUniqueUnicode(),
+            self.factory.makePerson(), distroseries=experimental_series)
+        self.failUnlessEqual(experimental_series, packageset.distroseries)
 
     def test_new_creates_new_packageset_group(self):
         # Creating a new packageset should also create a new packageset
         # group with the same owner.
-        packageset = self.packageset_set.new(
-            u'kernel', u'Contains all OS kernel packages', self.person1,
-            distroseries=self.distroseries_experimental)
-
-        self.failUnlessEqual(
-            self.person1, packageset.packagesetgroup.owner)
+        owner = self.factory.makePerson()
+        experimental_series = self.makeExperimentalSeries()
+        packageset = getUtility(IPackagesetSet).new(
+            self.factory.getUniqueUnicode(), self.factory.getUniqueUnicode(),
+            owner, distroseries=experimental_series)
+        self.failUnlessEqual(owner, packageset.packagesetgroup.owner)
 
     def test_new_duplicate_name_for_same_distroseries(self):
         # Creating a packageset with a duplicate name for the
         # given distroseries will fail.
-        packageset = self.packageset_set.new(
-            u'kernel', u'Contains all OS kernel packages', self.person1,
-            distroseries=self.distroseries_experimental)
-
-        self.failUnlessRaises(
-            DuplicatePackagesetName, self.packageset_set.new,
-            u'kernel', u'A packageset with a duplicate name', self.person1,
-            distroseries=self.distroseries_experimental)
+        distroseries = self.factory.makeDistroSeries()
+        name = self.factory.getUniqueUnicode()
+        self.factory.makePackageset(name, distroseries=distroseries)
+        self.assertRaises(
+            DuplicatePackagesetName, getUtility(IPackagesetSet).new,
+            name, self.factory.getUniqueUnicode(), self.factory.makePerson(),
+            distroseries=distroseries)
 
     def test_new_duplicate_name_for_different_distroseries(self):
         # Creating a packageset with a duplicate name but for a different
         # series is no problem.
-        packageset = self.packageset_set.new(
-            u'kernel', u'Contains all OS kernel packages', self.person1)
-
-        packageset2 = self.packageset_set.new(
-            u'kernel', u'A packageset with a duplicate name', self.person1,
-            distroseries=self.distroseries_experimental)
-        self.assertEqual(packageset.name, packageset2.name)
+        name = self.factory.getUniqueUnicode()
+        packageset1 = self.factory.makePackageset(name)
+        packageset2 = getUtility(IPackagesetSet).new(
+            name, self.factory.getUniqueUnicode(), self.factory.makePerson(),
+            distroseries=self.factory.makeDistroSeries())
+        self.assertEqual(packageset1.name, packageset2.name)
 
     def test_new_related_packageset(self):
         # Creating a new package set while specifying a `related_set` should
         # have the effect that the former ends up in the same group as the
         # latter.
-        pset1 = self.packageset_set.new(
-            u'kernel', u'Contains all OS kernel packages', self.person1)
-
-        pset2 = self.packageset_set.new(
-            u'kernel', u'A related package set.', self.person1,
-            distroseries=self.distroseries_experimental, related_set=pset1)
+        name = self.factory.getUniqueUnicode()
+        pset1 = self.factory.makePackageset(name)
+        pset2 = self.factory.makePackageset(
+            name, distroseries=self.makeExperimentalSeries(),
+            related_set=pset1)
         self.assertEqual(pset1.packagesetgroup, pset2.packagesetgroup)
 
     def test_get_by_name_in_current_distroseries(self):
         # IPackagesetSet.getByName() will return the package set in the
         # current distroseries if the optional `distroseries` parameter is
         # omitted.
-        pset1 = self.packageset_set.new(
-            u'kernel', u'Contains all OS kernel packages', self.person1)
-        pset2 = self.packageset_set.new(
-            u'kernel', u'A related package set.', self.person1,
-            distroseries=self.distroseries_experimental, related_set=pset1)
-        pset_found = getUtility(IPackagesetSet).getByName('kernel')
-        self.assertEqual(pset1, pset_found)
+        name = self.factory.getUniqueUnicode()
+        pset1 = self.factory.makePackageset(name)
+        self.factory.makePackageset(
+            name, distroseries=self.makeExperimentalSeries(),
+            related_set=pset1)
+        self.assertEqual(pset1, getUtility(IPackagesetSet).getByName(name))
 
     def test_get_by_name_in_specified_distroseries(self):
         # IPackagesetSet.getByName() will return the package set in the
         # specified distroseries.
-        pset1 = self.packageset_set.new(
-            u'kernel', u'Contains all OS kernel packages', self.person1)
-        pset2 = self.packageset_set.new(
-            u'kernel', u'A related package set.', self.person1,
-            distroseries=self.distroseries_experimental, related_set=pset1)
+        name = self.factory.getUniqueUnicode()
+        experimental_series = self.makeExperimentalSeries()
+        pset1 = self.factory.makePackageset(name)
+        pset2 = self.factory.makePackageset(
+            name, distroseries=experimental_series, related_set=pset1)
         pset_found = getUtility(IPackagesetSet).getByName(
-            'kernel', distroseries=self.distroseries_experimental)
+            name, distroseries=experimental_series)
         self.assertEqual(pset2, pset_found)
 
     def test_get_by_distroseries(self):
         # IPackagesetSet.getBySeries() will return those package sets
         # associated with the given distroseries.
-        pset1 = self.packageset_set.new(
-            u'timmy', u'Timmy Mallett', self.person1)
-        pset2 = self.packageset_set.new(
-            u'savile', u'Jimmy Savile', self.person1)
-        self.packageset_set.new(
-            u'hoskins', u'Bob Hoskins', self.person1,
-            distroseries=self.distroseries_experimental)
+        package_sets_for_current_ubuntu = [
+            self.factory.makePackageset() for counter in xrange(2)]
+        self.factory.makePackageset(
+            distroseries=self.makeExperimentalSeries())
         self.assertContentEqual(
-            [pset1, pset2],
-            self.packageset_set.getBySeries(self.distroseries_current))
+            package_sets_for_current_ubuntu,
+            getUtility(IPackagesetSet).getBySeries(
+                self.getUbuntu().currentseries))
+
+    def test_getForPackages_returns_packagesets(self):
+        # getForPackages finds package sets for given source package
+        # names in a distroseries, and maps them by
+        # SourcePackageName.id.
+        series = self.factory.makeDistroSeries()
+        packageset = self.factory.makePackageset(distroseries=series)
+        package = self.factory.makeSourcePackageName()
+        packageset.addSources([package.name])
+        self.assertEqual(
+            {package.id: [packageset]},
+            getUtility(IPackagesetSet).getForPackages(series, [package.id]))
+
+    def test_getForPackages_filters_by_distroseries(self):
+        # getForPackages does not return packagesets for different
+        # distroseries.
+        series = self.factory.makeDistroSeries()
+        other_series = self.factory.makeDistroSeries()
+        packageset = self.factory.makePackageset(distroseries=series)
+        package = self.factory.makeSourcePackageName()
+        packageset.addSources([package.name])
+        self.assertEqual(
+            {},
+            getUtility(IPackagesetSet).getForPackages(
+                other_series, [package.id]))
+
+    def test_getForPackages_filters_by_sourcepackagename(self):
+        # getForPackages does not return packagesets for different
+        # source package names.
+        series = self.factory.makeDistroSeries()
+        packageset = self.factory.makePackageset(distroseries=series)
+        package = self.factory.makeSourcePackageName()
+        other_package = self.factory.makeSourcePackageName()
+        packageset.addSources([package.name])
+        self.assertEqual(
+            {},
+            getUtility(IPackagesetSet).getForPackages(
+                series, [other_package.id]))
 
 
 class TestPackageset(TestCaseWithFactory):
 
-    layer = LaunchpadZopelessLayer
+    layer = ZopelessDatabaseLayer
 
     def setUp(self):
         """Setup a distribution with multiple distroseries."""
@@ -147,10 +173,10 @@
             'ubuntu')
         self.distroseries_current = self.distribution.currentseries
         self.distroseries_experimental = self.factory.makeDistroRelease(
-            distribution = self.distribution, name="experimental",
+            distribution=self.distribution, name="experimental",
             status=SeriesStatus.EXPERIMENTAL)
         self.distroseries_experimental2 = self.factory.makeDistroRelease(
-            distribution = self.distribution, name="experimental2",
+            distribution=self.distribution, name="experimental2",
             status=SeriesStatus.EXPERIMENTAL)
 
         self.person1 = self.factory.makePerson(