Merge ~pappacena/launchpad:pkg-upload-log-api into launchpad:master

[Thiago F. Pappacena]

API for Package Upload logs

Reopening MP (https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/377897) with proper prerequisite repo and branch.

29165ff... by Thiago F. Pappacena on 2020-01-27

fixing import formatting

[Colin Watson (cjwatson)]

review: Approve

16720db... by Thiago F. Pappacena on 2020-02-03

Merge branch 'master' into pkg-upload-log-api

09ab18f... by Thiago F. Pappacena on 2020-02-03

minor style adjustments

[Thiago F. Pappacena (pappacena)]

I'm pushing the changes. There is still one open topic about having the "ID" attributes at interface level that I would like your feedback, cjwatson.

3306b08... by Thiago F. Pappacena on 2020-02-03

removing from interface ID attributes

[Thiago F. Pappacena (pappacena)]

Pushing changes

diff --git a/lib/lp/_schema_circular_imports.py b/lib/lp/_schema_circular_imports.py
index 20db323..372744f 100644
--- a/lib/lp/_schema_circular_imports.py
+++ b/lib/lp/_schema_circular_imports.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Update the interface schema values due to circular imports.
diff --git a/lib/lp/registry/interfaces/persondistributionsourcepackage.py b/lib/lp/registry/interfaces/persondistributionsourcepackage.py
index 33ead04..d8d72bc 100644
--- a/lib/lp/registry/interfaces/persondistributionsourcepackage.py
+++ b/lib/lp/registry/interfaces/persondistributionsourcepackage.py
@@ -16,10 +16,10 @@ from zope.interface import (
     )
 from zope.schema import TextLine
 
-from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.distributionsourcepackage import (
     IDistributionSourcePackage,
     )
+from lp.registry.interfaces.person import IPerson
 
 
 class IPersonDistributionSourcePackage(Interface):
diff --git a/lib/lp/soyuz/browser/configure.zcml b/lib/lp/soyuz/browser/configure.zcml
index 1ce6b53..4ab2e41 100644
--- a/lib/lp/soyuz/browser/configure.zcml
+++ b/lib/lp/soyuz/browser/configure.zcml
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
+<!-- Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
      GNU Affero General Public License version 3 (see the file LICENSE).
 -->
 
@@ -661,6 +661,11 @@
         path_expression="string:+upload/${id}"
         attribute_to_parent="distroseries"
         />
+    <browser:url
+        for="lp.soyuz.interfaces.queue.IPackageUploadLog"
+        path_expression="string:+log/${id}"
+        attribute_to_parent="package_upload"
+        />
     <browser:navigation
         module="lp.soyuz.browser.queue"
         classes="PackageUploadNavigation"
diff --git a/lib/lp/soyuz/browser/queue.py b/lib/lp/soyuz/browser/queue.py
index 39177bd..520fd68 100644
--- a/lib/lp/soyuz/browser/queue.py
+++ b/lib/lp/soyuz/browser/queue.py
@@ -69,7 +69,6 @@ from lp.soyuz.model.files import (
 from lp.soyuz.model.packagecopyjob import PackageCopyJob
 from lp.soyuz.model.queue import (
     PackageUploadBuild,
-    PackageUploadLog,
     PackageUploadSource,
     )
 from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
@@ -211,26 +210,6 @@ class QueueItemsView(LaunchpadView):
         list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
             person_ids, need_validity=True))
 
-    def _getPreloadedLogs(self, uploads):
-        """Returns a dict of preloaded PackageUploadLog
-
-        The keys from the returning dict are the package_upload_id, and the
-        values are lists of log entries
-        """
-        logs = load_referencing(
-            PackageUploadLog, uploads, ['package_upload_id'])
-
-        # Preload users from log entries
-        # Not using `need_icon` since the log's reviewers are always persons,
-        # and fetching icons should be only needed for teams
-        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
-            [log.reviewer_id for log in logs],
-            need_validity=True))
-        logs_dict = defaultdict(list)
-        for log in logs:
-            logs_dict[log.package_upload_id].append(log)
-        return logs_dict
-
     def decoratedQueueBatch(self):
         """Return the current batch, converted to decorated objects.
 
@@ -244,12 +223,12 @@ class QueueItemsView(LaunchpadView):
             return None
 
         upload_ids = [upload.id for upload in uploads]
-        puses = load_referencing(
-            PackageUploadSource, uploads, ['packageuploadID'])
-        pubs = load_referencing(
-            PackageUploadBuild, uploads, ['packageuploadID'])
 
-        logs_dict = self._getPreloadedLogs(uploads)
+        # Both "u.sources" and "u.builds" below are preloaded by
+        # self.context.getPackageUploads (which uses PackageUploadSet.getAll)
+        # when building self.batchnav.
+        puses = sum([removeSecurityProxy(u.sources) for u in uploads], [])
+        pubs = sum([removeSecurityProxy(u.builds) for u in uploads], [])
 
         source_sprs = load_related(
             SourcePackageRelease, puses, ['sourcepackagereleaseID'])
@@ -288,9 +267,7 @@ class QueueItemsView(LaunchpadView):
 
         return [
             CompletePackageUpload(
-                item, build_upload_files, source_upload_files, package_sets,
-                sorted(logs_dict[item.id], key=attrgetter("date_created"),
-                       reverse=True))
+                item, build_upload_files, source_upload_files, package_sets)
             for item in uploads]
 
     def is_new(self, binarypackagerelease):
@@ -517,24 +494,18 @@ class CompletePackageUpload:
     # (i.e. no proxying of __set__).
     pocket = None
     date_created = None
-    sources = None
-    builds = None
-    logs = None
     customfiles = None
     contains_source = None
     contains_build = None
     sourcepackagerelease = None
 
     def __init__(self, packageupload, build_upload_files,
-                 source_upload_files, package_sets, logs=None):
+                 source_upload_files, package_sets):
         self.pocket = packageupload.pocket
         self.date_created = packageupload.date_created
         self.context = packageupload
-        self.sources = list(packageupload.sources)
         self.contains_source = len(self.sources) > 0
-        self.builds = list(packageupload.builds)
         self.contains_build = len(self.builds) > 0
-        self.logs = list(logs) if logs is not None else []
         self.customfiles = list(packageupload.customfiles)
 
         # Create a dictionary of binary files keyed by
diff --git a/lib/lp/soyuz/browser/tests/test_queue.py b/lib/lp/soyuz/browser/tests/test_queue.py
index 242e920..0e08739 100644
--- a/lib/lp/soyuz/browser/tests/test_queue.py
+++ b/lib/lp/soyuz/browser/tests/test_queue.py
@@ -438,7 +438,7 @@ class TestQueueItemsView(TestCaseWithFactory):
             with StormStatementRecorder() as recorder:
                 view = self.makeView(distroseries, queue_admin)
                 view()
-        self.assertThat(recorder, HasQueryCount(Equals(57)))
+        self.assertThat(recorder, HasQueryCount(Equals(55)))
 
     def test_package_upload_with_logs_query_count(self):
         login(ADMIN_EMAIL)
diff --git a/lib/lp/soyuz/interfaces/queue.py b/lib/lp/soyuz/interfaces/queue.py
index 929dc45..7bda98a 100644
--- a/lib/lp/soyuz/interfaces/queue.py
+++ b/lib/lp/soyuz/interfaces/queue.py
@@ -26,6 +26,7 @@ __all__ = [
 
 import httplib
 
+from lazr.lifecycle.snapshot import doNotSnapshot
 from lazr.restful.declarations import (
     call_with,
     error_status,
@@ -37,7 +38,10 @@ from lazr.restful.declarations import (
     operation_parameters,
     REQUEST_USER,
     )
-from lazr.restful.fields import Reference
+from lazr.restful.fields import (
+    CollectionField,
+    Reference,
+    )
 from zope.interface import (
     Attribute,
     Interface,
@@ -56,6 +60,7 @@ from zope.security.interfaces import Unauthorized
 from lp import _
 from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.webservice.apihelpers import patch_reference_property
 from lp.soyuz.enums import PackageUploadStatus
 from lp.soyuz.interfaces.packagecopyjob import IPackageCopyJob
 
@@ -113,6 +118,44 @@ class IPackageUploadQueue(Interface):
     """
 
 
+class IPackageUploadLog(Interface):
+    """A log entry recording a change in a package upload's status."""
+
+    export_as_webservice_entry(publish_web_link=True, as_of="devel")
+
+    id = Int(title=_('ID'), required=True, readonly=True)
+
+    package_upload = exported(
+        Reference(
+            Interface, title=_("The package upload that generated this log"),
+            required=True, readonly=True))
+
+    date_created = exported(
+        Datetime(
+            title=_("When this action happened."), required=True,
+            readonly=True))
+
+    reviewer = exported(
+        Reference(
+            IPerson, title=_("Who did this action."),
+            required=True, readonly=True))
+
+    old_status = exported(
+        Choice(
+            vocabulary=PackageUploadStatus, description=_("Old status."),
+            required=True, readonly=True))
+
+    new_status = exported(
+        Choice(
+            vocabulary=PackageUploadStatus, description=_("New status."),
+            required=True, readonly=True))
+
+    comment = exported(
+        TextLine(
+            title=_("User's comment about this change."),
+            required=False, readonly=True))
+
+
 class IPackageUpload(Interface):
     """A Queue item for the archive uploader."""
 
@@ -151,8 +194,6 @@ class IPackageUpload(Interface):
             title=_('Date created'),
             description=_("The date this package upload was done.")))
 
-    logs = Attribute(_("The change log of this PackageUpload."))
-
     changesfile = Attribute("The librarian alias for the changes file "
                             "associated with this upload")
     changes_file_url = exported(
@@ -185,6 +226,14 @@ class IPackageUpload(Interface):
     sources = Attribute("The queue sources associated with this queue item")
     builds = Attribute("The queue builds associated with the queue item")
 
+    logs = exported(
+        doNotSnapshot(
+            CollectionField(
+                title=_("The package upload logs"),
+                value_type=Reference(schema=IPackageUploadLog),
+                readonly=True)),
+        as_of="devel")
+
     customfiles = Attribute("Custom upload files associated with this "
                             "queue item")
     custom_file_urls = exported(
@@ -495,6 +544,9 @@ class IPackageUpload(Interface):
         """
 
 
+patch_reference_property(IPackageUploadLog, 'package_upload', IPackageUpload)
+
+
 class IPackageUploadBuild(Interface):
     """A Queue item's related builds."""
 
@@ -507,9 +559,7 @@ class IPackageUploadBuild(Interface):
             readonly=False,
             )
 
-    build = Int(
-            title=_("The related build"), required=True, readonly=False,
-            )
+    build = Int(title=_("The related build"), required=True, readonly=False)
 
     def binaries():
         """Returns the properties of the binaries in this build.
@@ -552,8 +602,7 @@ class IPackageUploadSource(Interface):
 
     sourcepackagerelease = Int(
             title=_("The related source package release"), required=True,
-            readonly=False,
-            )
+            readonly=False)
 
     def getSourceAncestryForDiffs():
         """Return a suitable ancestry publication for this context.
@@ -715,35 +764,6 @@ class IPackageUploadCustom(Interface):
         """
 
 
-class IPackageUploadLog(Interface):
-    """Entries of package upload status change"""
-
-    id = Int(title=_('ID'), required=True, readonly=True)
-
-    package_upload = Reference(
-        IPackageUpload,
-        title=_("Original package upload."), required=True, readonly=True)
-
-    date_created = Datetime(
-        title=_("When this action happened."), required=True, readonly=True)
-
-    reviewer = Reference(
-        IPerson, title=_("Who did this action."),
-        required=True, readonly=True)
-
-    old_status = Choice(
-        vocabulary=PackageUploadStatus, description=_("Old status."),
-        required=True, readonly=True)
-
-    new_status = Choice(
-        vocabulary=PackageUploadStatus, description=_("New status."),
-        required=True, readonly=True)
-
-    comment = TextLine(
-        title=_("User's comment about this change."),
-        required=False, readonly=True)
-
-
 class IPackageUploadSet(Interface):
     """Represents a set of IPackageUploads"""
 
diff --git a/lib/lp/soyuz/interfaces/webservice.py b/lib/lp/soyuz/interfaces/webservice.py
index 3620e81..90ef0e1 100644
--- a/lib/lp/soyuz/interfaces/webservice.py
+++ b/lib/lp/soyuz/interfaces/webservice.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """All the interfaces that are exposed through the webservice.
@@ -35,6 +35,7 @@ __all__ = [
     'ILiveFSBuild',
     'ILiveFSSet',
     'IPackageUpload',
+    'IPackageUploadLog',
     'IPackageset',
     'IPackagesetSet',
     'ISourcePackagePublishingHistory',
@@ -104,7 +105,10 @@ from lp.soyuz.interfaces.publishing import (
     IBinaryPackagePublishingHistory,
     ISourcePackagePublishingHistory,
     )
-from lp.soyuz.interfaces.queue import IPackageUpload
+from lp.soyuz.interfaces.queue import (
+    IPackageUpload,
+    IPackageUploadLog,
+    )
 
 
 _schema_circular_imports
diff --git a/lib/lp/soyuz/model/queue.py b/lib/lp/soyuz/model/queue.py
index 6a55fa7..91e92cd 100644
--- a/lib/lp/soyuz/model/queue.py
+++ b/lib/lp/soyuz/model/queue.py
@@ -12,7 +12,9 @@ __all__ = [
     'PackageUploadSource',
     ]
 
+from collections import defaultdict
 from itertools import chain
+from operator import attrgetter
 
 import pytz
 from sqlobject import (
@@ -42,6 +44,7 @@ from zope.interface import implementer
 from lp.app.errors import NotFoundError
 from lp.archiveuploader.tagfiles import parse_tagfile_content
 from lp.registry.interfaces.gpg import IGPGKeySet
+from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.services.auditor.client import AuditorClient
@@ -1600,8 +1603,10 @@ class PackageUploadSet:
                 PackageUploadBuild, rows, ["packageuploadID"])
             pucs = load_referencing(
                 PackageUploadCustom, rows, ["packageuploadID"])
+            logs = load_referencing(
+                PackageUploadLog, rows, ["package_upload_id"])
 
-            prefill_packageupload_caches(rows, puses, pubs, pucs)
+            prefill_packageupload_caches(rows, puses, pubs, pucs, logs)
 
         return DecoratedResultSet(query, pre_iter_hook=preload_hook)
 
@@ -1622,18 +1627,32 @@ class PackageUploadSet:
             PackageUpload.package_copy_job_id.is_in(pcj_ids))
 
 
-def prefill_packageupload_caches(uploads, puses, pubs, pucs):
+def prefill_packageupload_caches(uploads, puses, pubs, pucs, logs):
     # Circular imports.
     from lp.soyuz.model.archive import Archive
     from lp.soyuz.model.binarypackagebuild import BinaryPackageBuild
     from lp.soyuz.model.publishing import SourcePackagePublishingHistory
     from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 
+    logs_per_pu = defaultdict(list)
+    reviewer_ids = set()
+    for log in logs:
+        reviewer_ids.add(log.reviewer_id)
+        logs_per_pu[log.package_upload_id].append(log)
+
+    # Preload reviewers of the logs.
+    # We are not using `need_icon` here because reviewers are persons,
+    # and icons are only available for teams.
+    list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+        reviewer_ids, need_validity=True))
+
     for pu in uploads:
         cache = get_property_cache(pu)
         cache.sources = []
         cache.builds = []
         cache.customfiles = []
+        cache.logs = sorted(
+            logs_per_pu[pu.id], key=attrgetter("date_created"), reverse=True)
 
     for pus in puses:
         get_property_cache(pus.packageupload).sources.append(pus)
diff --git a/lib/lp/soyuz/tests/test_packageupload.py b/lib/lp/soyuz/tests/test_packageupload.py
index 0c83f33..6e78cdf 100644
--- a/lib/lp/soyuz/tests/test_packageupload.py
+++ b/lib/lp/soyuz/tests/test_packageupload.py
@@ -18,6 +18,7 @@ from lazr.restfulclient.errors import (
     )
 from testtools.matchers import (
     Equals,
+    MatchesListwise,
     MatchesStructure,
     )
 import transaction
@@ -1483,3 +1484,26 @@ class TestPackageUploadWebservice(TestCaseWithFactory):
                 person, component=self.universe),
             5)
         self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
+    def test_api_package_upload_log(self):
+        # API clients can see upload logs of a source uploads.
+        admin = self.makeQueueAdmin([self.universe])
+        upload, ws_upload = self.makeSourcePackageUpload(
+            admin, sourcepackagename="hello", component=self.universe)
+        with person_logged_in(admin):
+            upload.rejectFromQueue(admin, 'not a good change')
+            upload.acceptFromQueue(admin)
+
+        logs = removeSecurityProxy(upload).logs
+        ws_logs = ws_upload.logs
+        self.assertEqual(len(ws_logs), len(logs))
+        self.assertThat(ws_upload.logs, MatchesListwise([
+            MatchesStructure(
+                comment=Equals(log.comment),
+                date_created=Equals(log.date_created),
+                new_status=Equals(log.new_status.title),
+                old_status=Equals(log.old_status.title),
+                reviewer=MatchesStructure.byEquality(
+                    name=log.reviewer.name),
+                package_upload=MatchesStructure.byEquality(id=ws_upload.id))
+            for log in removeSecurityProxy(upload).logs]))