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

[Thiago F. Pappacena]

API for Package Upload logs

I'm adding some changes here the API part to fetch package upload logs, including pre-fetching related objects.

This branch should only be merged after https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/377717 , since it includes it's changes.

[Thiago F. Pappacena (pappacena)]

This is still a work in progress, since I want to check if it would be possible to refactor some of the pre-fetching to happen only on PackageUploadSet, instead of beign kind of duplicated in the browser.queue.QueueItemsView.

[Thiago F. Pappacena (pappacena)]

QueueItemsView is caching some extra attributes that are not beign cached on PackageUploadSet, but there was room for a small improvement anyway.

This should be ok to be reviewed, but should only be merged once the other MP is approved.

[Colin Watson (cjwatson)]

It would be best to resubmit this MP specifying "Prerequisite repository: ~pappacena/launchpad/+git/launchpad" and "Prerequisite branch: archive-queue-audit-trail". That way the changes introduced by the archive-queue-audit-trail branch won't show up in the review diff.

review: Needs Resubmitting

[Colin Watson (cjwatson)]

Just a few things I noticed while trying to filter mentally for just what's added by this branch. It'll be easier to review this once it's resubmitted with the right prerequisite, so I've probably missed some things.

4c4c95c... by Thiago F. Pappacena on 2020-01-27

exporting the package upload reference on pkg upload log api

3858ddd... by Thiago F. Pappacena on 2020-01-27

do not snapshot pkg upload logs

3aa046c... by Thiago F. Pappacena on 2020-01-27

fixing typo and writing better comment

[Thiago F. Pappacena (pappacena)]

I did some of the changes, but I'm going to open a new MP removing from the diff the changes not related to the API code.

[Thiago F. Pappacena (pappacena)]

Reopening the MP here: https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/378124

Unmerged commits

3aa046c... by Thiago F. Pappacena on 2020-01-27

fixing typo and writing better comment

3858ddd... by Thiago F. Pappacena on 2020-01-27

do not snapshot pkg upload logs

4c4c95c... by Thiago F. Pappacena on 2020-01-27

exporting the package upload reference on pkg upload log api

88e3582... by Thiago F. Pappacena on 2020-01-22

Merge branch 'archive-queue-audit-trail' into pkg-upload-log-api

6aed200... by Thiago F. Pappacena on 2020-01-22

avoiding to pre-fetch same thing twice when building CompletePackageUpload

59b6065... by Thiago F. Pappacena on 2020-01-22

code formatting and comment

176ce89... by Thiago F. Pappacena on 2020-01-22

formatting import

f557d63... by Thiago F. Pappacena on 2020-01-22

fix typo and remove duplicated entry in __all__

b9845f2... by Thiago F. Pappacena on 2020-01-21

import style

e85aaed... by Thiago F. Pappacena on 2020-01-21

moving log preloading PackageUploadSet, to reuse it both on browser and api

diff --git a/lib/lp/_schema_circular_imports.py b/lib/lp/_schema_circular_imports.py
index 20db323..49b37ec 100644
--- a/lib/lp/_schema_circular_imports.py
+++ b/lib/lp/_schema_circular_imports.py
@@ -206,7 +206,7 @@ from lp.soyuz.interfaces.publishing import (
     ISourcePackagePublishingHistoryEdit,
     ISourcePackagePublishingHistoryPublic,
     )
-from lp.soyuz.interfaces.queue import IPackageUpload
+from lp.soyuz.interfaces.queue import IPackageUpload, IPackageUploadLog
 from lp.soyuz.interfaces.sourcepackagerelease import ISourcePackageRelease
 from lp.translations.interfaces.hastranslationimports import (
     IHasTranslationImports,
@@ -363,6 +363,8 @@ patch_reference_property(
     ISourcePackagePublishingHistory)
 patch_reference_property(
     ISourcePackagePublishingHistory, 'packageupload', IPackageUpload)
+patch_reference_property(
+    IPackageUploadLog, 'package_upload', IPackageUpload)
 patch_entry_return_type(
     ISourcePackagePublishingHistoryEdit, 'changeOverride',
     ISourcePackagePublishingHistory)
diff --git a/lib/lp/security.py b/lib/lp/security.py
index 66c7f73..c2d45ac 100644
--- a/lib/lp/security.py
+++ b/lib/lp/security.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).
 
 """Security policies for using content objects."""
@@ -246,6 +246,7 @@ from lp.soyuz.interfaces.publishing import (
     )
 from lp.soyuz.interfaces.queue import (
     IPackageUpload,
+    IPackageUploadLog,
     IPackageUploadQueue,
     )
 from lp.soyuz.interfaces.sourcepackagerelease import ISourcePackageRelease
@@ -1918,6 +1919,15 @@ class ViewPackageUpload(AuthorizationBase):
             methodcaller("checkUnauthenticated"), self.iter_adapters()))
 
 
+class ViewPackageUploadLog(DelegatedAuthorization):
+    """Anyone who can view a package upload can view its logs."""
+    permission = 'launchpad.View'
+    usedfor = IPackageUploadLog
+
+    def __init__(self, obj):
+        super(ViewPackageUploadLog, self).__init__(obj, obj.package_upload)
+
+
 class EditPackageUpload(AdminByAdminsTeam):
     permission = 'launchpad.Edit'
     usedfor = IPackageUpload
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 a476e1d..02d5f54 100644
--- a/lib/lp/soyuz/browser/queue.py
+++ b/lib/lp/soyuz/browser/queue.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 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).
 
 """Browser views for package queue."""
@@ -10,6 +10,7 @@ __all__ = [
     'QueueItemsView',
     ]
 
+from collections import defaultdict
 from operator import attrgetter
 
 from lazr.delegates import delegate_to
@@ -207,7 +208,7 @@ class QueueItemsView(LaunchpadView):
         jobs = load_related(Job, package_copy_jobs, ['job_id'])
         person_ids.extend(map(attrgetter('requester_id'), jobs))
         list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
-            person_ids, need_validity=True, need_icon=True))
+            person_ids, need_validity=True))
 
     def decoratedQueueBatch(self):
         """Return the current batch, converted to decorated objects.
@@ -222,10 +223,8 @@ 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'])
+        puses = sum([u.sources for u in uploads], [])
+        pubs = sum([u.builds for u in uploads], [])
 
         source_sprs = load_related(
             SourcePackageRelease, puses, ['sourcepackagereleaseID'])
@@ -491,8 +490,6 @@ class CompletePackageUpload:
     # (i.e. no proxying of __set__).
     pocket = None
     date_created = None
-    sources = None
-    builds = None
     customfiles = None
     contains_source = None
     contains_build = None
@@ -503,9 +500,7 @@ class CompletePackageUpload:
         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.customfiles = list(packageupload.customfiles)
 
diff --git a/lib/lp/soyuz/browser/tests/test_queue.py b/lib/lp/soyuz/browser/tests/test_queue.py
index c2d15f2..0e08739 100644
--- a/lib/lp/soyuz/browser/tests/test_queue.py
+++ b/lib/lp/soyuz/browser/tests/test_queue.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2018 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).
 
 """Unit tests for QueueItemsView."""
@@ -33,6 +33,7 @@ from lp.testing import (
     login_person,
     logout,
     person_logged_in,
+    record_two_runs,
     StormStatementRecorder,
     TestCaseWithFactory,
     )
@@ -342,10 +343,10 @@ class TestQueueItemsView(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
-    def makeView(self, distroseries, user):
+    def makeView(self, distroseries, user, form=None):
         """Create a queue view."""
         return create_initialized_view(
-            distroseries, name='+queue', principal=user)
+            distroseries, name='+queue', principal=user, form=form)
 
     def test_view_renders_source_upload(self):
         login(ADMIN_EMAIL)
@@ -437,7 +438,34 @@ class TestQueueItemsView(TestCaseWithFactory):
             with StormStatementRecorder() as recorder:
                 view = self.makeView(distroseries, queue_admin)
                 view()
-        self.assertThat(recorder, HasQueryCount(Equals(56)))
+        self.assertThat(recorder, HasQueryCount(Equals(55)))
+
+    def test_package_upload_with_logs_query_count(self):
+        login(ADMIN_EMAIL)
+        uploads = []
+        distroseries = self.factory.makeDistroSeries()
+
+        for i in range(11):
+            uploads.append(self.factory.makeSourcePackageUpload(distroseries))
+        queue_admin = self.factory.makeArchiveAdmin(distroseries.main_archive)
+
+        def reject_some_package():
+            for upload in uploads:
+                if len(upload.logs) == 0:
+                    person = self.factory.makePerson()
+                    upload.rejectFromQueue(person)
+                    break
+
+        def run_view():
+            with person_logged_in(queue_admin):
+                url = ("%s/+queue?queue_state=%s" % (
+                    canonical_url(distroseries),
+                    PackageUploadStatus.REJECTED.value))
+                self.getUserBrowser(url, queue_admin)
+
+        recorder1, recorder2 = record_two_runs(
+            run_view, reject_some_package, 1, 10)
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
 
 
 class TestCompletePackageUpload(TestCaseWithFactory):
diff --git a/lib/lp/soyuz/configure.zcml b/lib/lp/soyuz/configure.zcml
index 0e35ccb..9b50239 100644
--- a/lib/lp/soyuz/configure.zcml
+++ b/lib/lp/soyuz/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).
 -->
 
@@ -151,6 +151,7 @@
                 displayarchs
                 displayversion
                 isPPA
+                logs
                 package_copy_job
                 package_copy_job_id
                 package_name
@@ -183,6 +184,12 @@
             set_attributes="status distroseries pocket changesfile archive"/>
     </class>
     <class
+        class="lp.soyuz.model.queue.PackageUploadLog">
+        <require
+            permission="launchpad.View"
+            interface="lp.soyuz.interfaces.queue.IPackageUploadLog"/>
+    </class>
+    <class
         class="lp.soyuz.model.queue.PackageUploadSource">
         <allow
             interface="lp.soyuz.interfaces.queue.IPackageUploadSource"/>
diff --git a/lib/lp/soyuz/interfaces/queue.py b/lib/lp/soyuz/interfaces/queue.py
index c57da73..3a02689 100644
--- a/lib/lp/soyuz/interfaces/queue.py
+++ b/lib/lp/soyuz/interfaces/queue.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 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).
 
 """Queue interfaces."""
@@ -9,22 +9,24 @@ __all__ = [
     'CustomUploadError',
     'ICustomUploadHandler',
     'IHasQueueItems',
-    'IPackageUploadQueue',
     'IPackageUpload',
     'IPackageUploadBuild',
-    'IPackageUploadSource',
     'IPackageUploadCustom',
+    'IPackageUploadLog',
+    'IPackageUploadQueue',
     'IPackageUploadSet',
+    'IPackageUploadSource',
     'NonBuildableSourceUploadError',
     'QueueAdminUnauthorizedError',
     'QueueBuildAcceptError',
     'QueueInconsistentStateError',
     'QueueSourceAcceptError',
-    'QueueStateWriteProtectedError',
+    'QueueStateWriteProtectedError'
     ]
 
 import httplib
 
+from lazr.lifecycle.snapshot import doNotSnapshot
 from lazr.restful.declarations import (
     call_with,
     error_status,
@@ -36,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,
@@ -53,6 +58,7 @@ from zope.schema import (
 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.soyuz.enums import PackageUploadStatus
 from lp.soyuz.interfaces.packagecopyjob import IPackageCopyJob
@@ -111,6 +117,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."""
 
@@ -181,6 +225,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(
@@ -503,9 +555,11 @@ 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)
+
+    buildID = Int(
+        title=_("The Related build ID"), required=True,
+        readonly=True)
 
     def binaries():
         """Returns the properties of the binaries in this build.
@@ -548,8 +602,11 @@ class IPackageUploadSource(Interface):
 
     sourcepackagerelease = Int(
             title=_("The related source package release"), required=True,
-            readonly=False,
-            )
+            readonly=False)
+
+    sourcepackagereleaseID = Int(
+            title=_("The related source package release ID"), required=True,
+            readonly=True)
 
     def getSourceAncestryForDiffs():
         """Return a suitable ancestry publication for this context.
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 0fd1799..91e92cd 100644
--- a/lib/lp/soyuz/model/queue.py
+++ b/lib/lp/soyuz/model/queue.py
@@ -1,18 +1,22 @@
-# Copyright 2009-2018 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).
 
 __metaclass__ = type
 __all__ = [
-    'PackageUploadQueue',
     'PackageUpload',
     'PackageUploadBuild',
-    'PackageUploadSource',
     'PackageUploadCustom',
+    'PackageUploadLog',
+    'PackageUploadQueue',
     'PackageUploadSet',
+    'PackageUploadSource',
     ]
 
+from collections import defaultdict
 from itertools import chain
+from operator import attrgetter
 
+import pytz
 from sqlobject import (
     ForeignKey,
     SQLMultipleJoin,
@@ -29,6 +33,7 @@ from storm.locals import (
     SQL,
     Unicode,
     )
+from storm.properties import DateTime
 from storm.store import (
     EmptyResultSet,
     Store,
@@ -39,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
@@ -46,10 +52,16 @@ from lp.services.database.bulk import (
     load_referencing,
     load_related,
     )
-from lp.services.database.constants import UTC_NOW
+from lp.services.database.constants import (
+    DEFAULT,
+    UTC_NOW,
+    )
 from lp.services.database.datetimecol import UtcDateTimeCol
 from lp.services.database.decoratedresultset import DecoratedResultSet
-from lp.services.database.enumcol import EnumCol
+from lp.services.database.enumcol import (
+    DBEnum,
+    EnumCol,
+    )
 from lp.services.database.interfaces import (
     IMasterStore,
     IStore,
@@ -58,6 +70,7 @@ from lp.services.database.sqlbase import (
     SQLBase,
     sqlvalues,
     )
+from lp.services.database.stormbase import StormBase
 from lp.services.database.stormexpr import (
     Array,
     ArrayContains,
@@ -97,6 +110,7 @@ from lp.soyuz.interfaces.queue import (
     IPackageUpload,
     IPackageUploadBuild,
     IPackageUploadCustom,
+    IPackageUploadLog,
     IPackageUploadQueue,
     IPackageUploadSet,
     IPackageUploadSource,
@@ -205,6 +219,23 @@ class PackageUpload(SQLBase):
             self.addSearchableVersions([self.package_copy_job.package_version])
 
     @cachedproperty
+    def logs(self):
+        logs = Store.of(self).find(
+            PackageUploadLog,
+            PackageUploadLog.package_upload == self)
+        return list(logs.order_by(Desc('date_created')))
+
+    def _addLog(self, reviewer, new_status, comment=None):
+        del get_property_cache(self).logs  # clean cache
+        return Store.of(self).add(PackageUploadLog(
+            package_upload=self,
+            reviewer=reviewer,
+            old_status=self.status,
+            new_status=new_status,
+            comment=comment
+        ))
+
+    @cachedproperty
     def sources(self):
         return list(self._sources)
 
@@ -571,6 +602,10 @@ class PackageUpload(SQLBase):
 
     def acceptFromQueue(self, user=None):
         """See `IPackageUpload`."""
+        # XXX: Only tests are not passing user here. We should adjust the
+        # tests and always create the log entries after
+        if user is not None:
+            self._addLog(user, PackageUploadStatus.ACCEPTED, None)
         if self.package_copy_job is None:
             self._acceptNonSyncFromQueue()
         else:
@@ -581,6 +616,7 @@ class PackageUpload(SQLBase):
 
     def rejectFromQueue(self, user, comment=None):
         """See `IPackageUpload`."""
+        self._addLog(user, PackageUploadStatus.REJECTED, comment)
         self.setRejected()
         if self.package_copy_job is not None:
             # Circular imports :(
@@ -1153,6 +1189,45 @@ def get_properties_for_binary(bpr):
         }
 
 
+@implementer(IPackageUploadLog)
+class PackageUploadLog(StormBase):
+    """Tracking of status changes for a given package upload"""
+
+    __storm_table__ = "PackageUploadLog"
+
+    id = Int(primary=True)
+
+    package_upload_id = Int(name='package_upload')
+    package_upload = Reference(package_upload_id, PackageUpload.id)
+
+    date_created = DateTime(tzinfo=pytz.UTC, allow_none=False,
+                            default=UTC_NOW)
+
+    reviewer_id = Int(name='reviewer', allow_none=False)
+    reviewer = Reference(reviewer_id, 'Person.id')
+
+    old_status = DBEnum(enum=PackageUploadStatus, allow_none=False)
+
+    new_status = DBEnum(enum=PackageUploadStatus, allow_none=False)
+
+    comment = Unicode(allow_none=True)
+
+    def __init__(self, package_upload, reviewer, old_status, new_status,
+                 comment=None, date_created=DEFAULT):
+        self.package_upload = package_upload
+        self.reviewer = reviewer
+        self.old_status = old_status
+        self.new_status = new_status
+        self.comment = comment
+        self.date_created = date_created
+
+    def __repr__(self):
+        return (
+            "<{self.__class__.__name__} ~{self.reviewer.name} "
+            "changed {self.package_upload} to {self.new_status} "
+            "on {self.date_created}>").format(self=self)
+
+
 @implementer(IPackageUploadBuild)
 class PackageUploadBuild(SQLBase):
     """A Queue item's related builds."""
@@ -1528,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)
 
@@ -1550,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/stories/soyuz/xx-queue-pages.txt b/lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt
index c5f4741..b2bc537 100644
--- a/lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt
+++ b/lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt
@@ -512,6 +512,7 @@ values:
     >>> for row in filelist:
     ...     print(extract_text(row))
     pmount_1.0-1_all.deb (18 bytes) NEW 0.1-1 restricted admin extra
+    Accepted a moment ago by Sample Person
 
 'netapplet' has gone straight to the 'done' queue because it's a single
 source upload, and we can see its overridden values there:
@@ -547,6 +548,15 @@ Rejecting 'alsa-utils' source:
     netapplet...ddtp... -                                           Release 2006-...
     netapplet...dist... -                                           Release 2006-...
 
+    >>> upload_manager_browser.getControl(
+    ...     name="queue_state", index=0).displayValue=['Rejected']
+    >>> upload_manager_browser.getControl("Update").click()
+    >>> logs = find_tags_by_class(
+    ...     upload_manager_browser.contents, "log-content")
+    >>> for log in logs:
+    ...     print(extract_text(log))
+    Rejected...a moment ago...by Sample Person...Foo
+
 One rejection email is generated:
 
     >>> run_package_upload_notification_jobs()
@@ -600,6 +610,21 @@ button will be disabled.
     >>> upload_manager_browser.getControl(name="Reject").disabled
     True
 
+Accepting alsa again, and check that the package upload log has more rows
+
+    >>> upload_manager_browser.getControl(name="QUEUE_ID").value = ['4']
+    >>> upload_manager_browser.getControl(name="Accept").click()
+    >>> upload_manager_browser.getControl(
+    ...     name="queue_state", index=0).displayValue=['Accepted']
+    >>> upload_manager_browser.getControl("Update").click()
+    >>> pkg_content = first_tag_by_class(upload_manager_browser.contents,
+    ...                                  "queue-4")
+    >>> logs = find_tags_by_class(str(pkg_content), "log-content")
+    >>> for log in logs:
+    ...     print(extract_text(log))
+    Accepted...a moment ago...by Sample Person...
+    Rejected...a moment ago...by Sample Person...Foo
+
 
 Clean up
 ========
diff --git a/lib/lp/soyuz/templates/distroseries-queue.pt b/lib/lp/soyuz/templates/distroseries-queue.pt
index 59b77d5..39629ce 100644
--- a/lib/lp/soyuz/templates/distroseries-queue.pt
+++ b/lib/lp/soyuz/templates/distroseries-queue.pt
@@ -149,6 +149,16 @@
            </tbody>
            <tbody tal:attributes="class string:${filelist_class}">
              <metal:filelist use-macro="template/macros/package-filelist"/>
+             <tr class="log-content" tal:repeat="log packageupload/logs">
+               <td colspan="2" style="border: 0"></td>
+               <td colspan="8" style="border: 0">
+                 <span tal:content="log/new_status"></span>
+                 <span tal:attributes="title log/date_created/fmt:datetime"
+                       tal:content="log/date_created/fmt:displaydate" />
+                 by <span tal:content="structure log/reviewer/fmt:link" />
+                 <p tal:condition="log/comment" tal:content="log/comment" />
+               </td>
+             </tr>
            </tbody>
          </tal:block>
        </tal:batch>
diff --git a/lib/lp/soyuz/tests/test_packageupload.py b/lib/lp/soyuz/tests/test_packageupload.py
index cb4d038..1e7e655 100644
--- a/lib/lp/soyuz/tests/test_packageupload.py
+++ b/lib/lp/soyuz/tests/test_packageupload.py
@@ -16,7 +16,10 @@ from lazr.restfulclient.errors import (
     BadRequest,
     Unauthorized,
     )
-from testtools.matchers import Equals
+from testtools.matchers import (
+    Equals,
+    MatchesStructure,
+    )
 import transaction
 from zope.component import (
     getUtility,
@@ -91,6 +94,27 @@ class PackageUploadTestCase(TestCaseWithFactory):
         if os.path.exists(config.personalpackagearchive.root):
             shutil.rmtree(config.personalpackagearchive.root)
 
+    def test_add_log_entry(self):
+        upload = self.factory.makePackageUpload(
+            status=PackageUploadStatus.UNAPPROVED)
+        upload = removeSecurityProxy(upload)
+        self.assertEqual(0, len(upload.logs))
+
+        person = self.factory.makePerson(name='lpusername')
+
+        upload._addLog(person, PackageUploadStatus.REJECTED, 'just because')
+
+        log = upload.logs[0]
+        self.assertThat(log, MatchesStructure.byEquality(
+            reviewer=person, old_status=PackageUploadStatus.UNAPPROVED,
+            new_status=PackageUploadStatus.REJECTED, comment='just because'))
+
+        expected_repr = (
+            "<PackageUploadLog ~lpusername "
+            "changed {self.package_upload} to Rejected "
+            "on {self.date_created}>").format(self=log)
+        self.assertEqual(str(expected_repr), repr(log))
+
     def test_realiseUpload_for_overridden_component_archive(self):
         # If the component of an upload is overridden to 'Partner' for
         # example, then the new publishing record should be for the
@@ -358,8 +382,14 @@ class PackageUploadTestCase(TestCaseWithFactory):
 
         # Accepting one of them works.  (Since it's a single source upload,
         # it goes straight to DONE.)
-        upload_one.acceptFromQueue()
+        person = self.factory.makePerson()
+        upload_one.acceptFromQueue(person)
         self.assertEqual("DONE", upload_one.status.name)
+
+        log = upload_one.logs[0]
+        self.assertThat(log, MatchesStructure.byEquality(
+            reviewer=person, old_status=PackageUploadStatus.UNAPPROVED,
+            new_status=PackageUploadStatus.ACCEPTED, comment=None))
         transaction.commit()
 
         # Trying to accept the second fails.
@@ -368,9 +398,15 @@ class PackageUploadTestCase(TestCaseWithFactory):
         self.assertEqual("UNAPPROVED", upload_two.status.name)
 
         # Rejecting the second upload works.
-        upload_two.rejectFromQueue(self.factory.makePerson())
+        upload_two.rejectFromQueue(person, 'Because yes')
         self.assertEqual("REJECTED", upload_two.status.name)
 
+        self.assertEqual(1, len(upload_two.logs))
+        log = upload_two.logs[0]
+        self.assertThat(log, MatchesStructure.byEquality(
+            reviewer=person, old_status=PackageUploadStatus.UNAPPROVED,
+            new_status=PackageUploadStatus.REJECTED, comment='Because yes'))
+
     def test_rejectFromQueue_source_sends_email(self):
         # Rejecting a source package sends an email to the uploader.
         self.test_publisher.prepareBreezyAutotest()
@@ -1447,3 +1483,23 @@ 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))
+        for log, ws_log in zip(logs, ws_logs):
+            self.assertEqual(log.comment, ws_log.comment)
+            self.assertEqual(log.date_created, ws_log.date_created)
+            self.assertEqual(log.new_status.title, ws_log.new_status)
+            self.assertEqual(log.old_status.title, ws_log.old_status)
+            self.assertEqual(log.reviewer.name, ws_log.reviewer.name)
+            self.assertEqual(ws_log.package_upload.id, ws_upload.id)