Merge ~cjwatson/launchpad:ci-build-files into launchpad:master

[Colin Watson]

Show built files on CIBuild:+index

Most build types show links to their output files on their index page, and it was odd that CI builds didn't; it also made certain kinds of debugging more difficult.

[Colin Watson (cjwatson)]

UI screenshot: https://people.canonical.com/~cjwatson/launchpad/screenshots/ci-build-files.png

[Guruprasad (lgp171188)]

LGTM 👍 with a question.

review: Approve

diff --git a/lib/lp/code/browser/cibuild.py b/lib/lp/code/browser/cibuild.py
index 187f5ef..a1018e1 100644
--- a/lib/lp/code/browser/cibuild.py
+++ b/lib/lp/code/browser/cibuild.py
@@ -13,7 +13,11 @@ from zope.interface import Interface
 
 from lp.app.browser.launchpadform import LaunchpadFormView, action
 from lp.code.interfaces.cibuild import ICIBuild
-from lp.services.librarian.browser import FileNavigationMixin
+from lp.services.librarian.browser import (
+    FileNavigationMixin,
+    ProxiedLibraryFileAlias,
+)
+from lp.services.propertycache import cachedproperty
 from lp.services.webapp import (
     ContextMenu,
     Link,
@@ -77,6 +81,22 @@ class CIBuildView(LaunchpadFormView):
 
     page_title = label
 
+    @cachedproperty
+    def files(self):
+        """Return `LibraryFileAlias`es for files produced by this build."""
+        if not self.context.was_built:
+            return None
+
+        return [
+            ProxiedLibraryFileAlias(artifact.library_file, artifact)
+            for artifact in self.context.getArtifacts()
+            if not artifact.library_file.deleted
+        ]
+
+    @cachedproperty
+    def has_files(self):
+        return bool(self.files)
+
 
 class CIBuildRetryView(LaunchpadFormView):
     """View for retrying a CI build."""
diff --git a/lib/lp/code/browser/tests/test_cibuild.py b/lib/lp/code/browser/tests/test_cibuild.py
index 2390997..3c5d7bf 100644
--- a/lib/lp/code/browser/tests/test_cibuild.py
+++ b/lib/lp/code/browser/tests/test_cibuild.py
@@ -11,6 +11,7 @@ from fixtures import FakeLogger
 from storm.locals import Store
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
+from zope.security.proxy import removeSecurityProxy
 from zope.testbrowser.browser import LinkNotFoundError
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
@@ -23,12 +24,13 @@ from lp.testing import (
     login,
     person_logged_in,
 )
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import DatabaseFunctionalLayer, LaunchpadFunctionalLayer
 from lp.testing.pages import (
     extract_text,
     find_main_content,
     find_tags_by_class,
 )
+from lp.testing.views import create_initialized_view
 
 
 class TestCanonicalUrlForCIBuild(TestCaseWithFactory):
@@ -224,3 +226,50 @@ class TestCIBuildOperations(BrowserTestCase):
         build = self.makeBuildingRecipe()
         browser = self.getViewBrowser(build.builder, no_login=True)
         self.assertIn("tail of the log", browser.contents)
+
+
+class TestCIBuildView(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_files(self):
+        # CIBuildView.files returns all the associated files.
+        build = self.factory.makeCIBuild(status=BuildStatus.FULLYBUILT)
+        reports = [
+            build.getOrCreateRevisionStatusReport(job_id)
+            for job_id in ("build:0", "build:1")
+        ]
+        for report in reports:
+            # Deliberately use identical filenames for each artifact, since
+            # that's the hardest case.
+            removeSecurityProxy(report).attach(
+                "package.tar.gz", report.title.encode()
+            )
+        artifacts = build.getArtifacts()
+        self.assertContentEqual(
+            reports, {artifact.report for artifact in artifacts}
+        )
+        build_view = create_initialized_view(build, "+index")
+        self.assertEqual(
+            [
+                "%s/+files/%s"
+                % (canonical_url(artifact), artifact.library_file.filename)
+                for artifact in artifacts
+            ],
+            [lfa.http_url for lfa in build_view.files],
+        )
+        # Deleted files won't be included.
+        self.assertFalse(artifacts[1].library_file.deleted)
+        removeSecurityProxy(artifacts[1].library_file).content = None
+        self.assertTrue(artifacts[1].library_file.deleted)
+        build_view = create_initialized_view(build, "+index")
+        self.assertEqual(
+            [
+                "%s/+files/%s"
+                % (
+                    canonical_url(artifacts[0]),
+                    artifacts[0].library_file.filename,
+                )
+            ],
+            [lfa.http_url for lfa in build_view.files],
+        )
diff --git a/lib/lp/code/interfaces/cibuild.py b/lib/lp/code/interfaces/cibuild.py
index 5c7725f..9599b75 100644
--- a/lib/lp/code/interfaces/cibuild.py
+++ b/lib/lp/code/interfaces/cibuild.py
@@ -201,6 +201,14 @@ class ICIBuildView(IPackageBuildView, IPrivacy):
         :return: The corresponding `ILibraryFileAlias`.
         """
 
+    def getArtifacts():
+        """Return `IRevisionStatusArtifact`s produced by this build.
+
+        :return: A result set of `IRevisionStatusArtifact`s, ordered by
+            filename then artifact ID, with their associated
+            `ILibraryFileAlias`es preloaded.
+        """
+
     @export_read_operation()
     @operation_for_version("devel")
     def getFileUrls():
diff --git a/lib/lp/code/model/cibuild.py b/lib/lp/code/model/cibuild.py
index 0bfc773..c06e7a6 100644
--- a/lib/lp/code/model/cibuild.py
+++ b/lib/lp/code/model/cibuild.py
@@ -9,7 +9,7 @@ __all__ = [
 
 from copy import copy
 from datetime import timedelta
-from operator import attrgetter, itemgetter
+from operator import itemgetter
 
 import pytz
 from lazr.lifecycle.event import ObjectCreatedEvent
@@ -49,12 +49,13 @@ from lp.code.interfaces.cibuild import (
 )
 from lp.code.interfaces.githosting import IGitHostingClient
 from lp.code.interfaces.gitrepository import IGitRepository
-from lp.code.interfaces.revisionstatus import (
-    IRevisionStatusArtifactSet,
-    IRevisionStatusReportSet,
-)
+from lp.code.interfaces.revisionstatus import IRevisionStatusReportSet
 from lp.code.model.gitref import GitRef
 from lp.code.model.lpcraft import load_configuration
+from lp.code.model.revisionstatus import (
+    RevisionStatusArtifact,
+    RevisionStatusReport,
+)
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.sourcepackage import SourcePackageType
@@ -478,15 +479,25 @@ class CIBuild(PackageBuildMixin, StormBase):
 
         raise NotFoundError(filename)
 
-    def getFileUrls(self):
-        artifacts = getUtility(IRevisionStatusArtifactSet).findByCIBuild(self)
-        load_related(LibraryFileAlias, artifacts, ["library_file_id"])
-        artifacts = sorted(
-            artifacts, key=attrgetter("library_file.filename", "id")
+    def getArtifacts(self):
+        """See `ICIBuild`."""
+        artifacts = (
+            IStore(self)
+            .find(
+                (RevisionStatusArtifact, LibraryFileAlias),
+                RevisionStatusReport.ci_build == self,
+                RevisionStatusArtifact.report == RevisionStatusReport.id,
+                RevisionStatusArtifact.library_file == LibraryFileAlias.id,
+            )
+            .order_by(LibraryFileAlias.filename, RevisionStatusArtifact.id)
         )
+        return DecoratedResultSet(artifacts, result_decorator=itemgetter(0))
+
+    def getFileUrls(self):
+        """See `ICIBuild`."""
         return [
             ProxiedLibraryFileAlias(artifact.library_file, artifact).http_url
-            for artifact in artifacts
+            for artifact in self.getArtifacts()
         ]
 
     def verifySuccessfulUpload(self) -> bool:
diff --git a/lib/lp/code/templates/cibuild-index.pt b/lib/lp/code/templates/cibuild-index.pt
index 35ef0cf..790b651 100644
--- a/lib/lp/code/templates/cibuild-index.pt
+++ b/lib/lp/code/templates/cibuild-index.pt
@@ -33,6 +33,10 @@
 
       </div> <!-- yui-g -->
 
+      <div id="files" class="portlet" tal:condition="view/has_files">
+        <div metal:use-macro="template/macros/files"/>
+      </div>
+
       <div id="buildlog" class="portlet"
            tal:condition="context/status/enumvalue:BUILDING">
         <div metal:use-macro="template/macros/buildlog"/>
@@ -162,6 +166,22 @@
     </div>
   </metal:macro>
 
+  <metal:macro define-macro="files">
+    <tal:comment replace="nothing">
+      Files section.
+    </tal:comment>
+    <h2>Built files</h2>
+    <p>Files resulting from this build:</p>
+    <ul>
+      <li tal:repeat="file view/files">
+        <a class="sprite download"
+           tal:content="file/filename"
+           tal:attributes="href file/http_url"/>
+        (<span tal:replace="file/content/filesize/fmt:bytes"/>)
+      </li>
+    </ul>
+  </metal:macro>
+
   <metal:macro define-macro="buildlog">
     <tal:comment replace="nothing">
       Buildlog section.
diff --git a/lib/lp/services/librarian/browser.py b/lib/lp/services/librarian/browser.py
index bd42c26..d364b68 100644
--- a/lib/lp/services/librarian/browser.py
+++ b/lib/lp/services/librarian/browser.py
@@ -149,3 +149,10 @@ class ProxiedLibraryFileAlias:
             url_path_quote(self.context.filename.encode("utf-8")),
         )
         return url
+
+    # XXX cjwatson 2023-02-23: For plain `LibraryFileAlias`, callers prefer
+    # to use `getURL` over `http_url`, because that returns an HTTPS URL
+    # where appropriate.  However, if you try to do that with a
+    # `ProxiedLibraryFileAlias`, it returns a URL pointing directly to the
+    # librarian rather than proxying via the webapp.  To avoid this gotcha,
+    # perhaps we should implement `getURL` here as well.