launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #24236
[Merge] ~pappacena/launchpad:archive-queue-audit-trail into launchpad:master
Thiago F. Pappacena has proposed merging ~pappacena/launchpad:archive-queue-audit-trail into launchpad:master.
Commit message:
Creating audit logs for package approval and rejection
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #885739 in Launchpad itself: "queue and override manipulations should have an audit trail"
https://bugs.launchpad.net/launchpad/+bug/885739
For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/377717
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:archive-queue-audit-trail into launchpad:master.
diff --git a/database/schema/security.cfg b/database/schema/security.cfg
index 06da0d4..2844a30 100644
--- a/database/schema/security.cfg
+++ b/database/schema/security.cfg
@@ -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).
#
# Possible permissions: SELECT, INSERT, UPDATE, EXECUTE
@@ -1080,6 +1080,7 @@ public.packagesetgroup = SELECT, INSERT
public.packagesetinclusion = SELECT, INSERT
public.packagesetsources = SELECT, INSERT
public.packageupload = SELECT, INSERT, UPDATE
+public.packageuploadlog = SELECT
public.packageuploadsource = SELECT
public.packageuploadbuild = SELECT
public.packageuploadcustom = SELECT, INSERT
@@ -1242,6 +1243,7 @@ public.ociprojectname = SELECT, INSERT, UPDATE
public.ociprojectseries = SELECT, INSERT, UPDATE, DELETE
public.openididentifier = SELECT
public.packageupload = SELECT, INSERT, UPDATE
+public.packageuploadlog = SELECT, INSERT
public.packageuploadbuild = SELECT, INSERT, UPDATE
public.packageuploadcustom = SELECT, INSERT, UPDATE
public.packageuploadsource = SELECT, INSERT, UPDATE
@@ -2293,6 +2295,7 @@ public.packagediff = SELECT, UPDATE
public.packageset = SELECT, UPDATE
public.packagesetgroup = SELECT, UPDATE
public.packageupload = SELECT, UPDATE
+public.packageuploadlog = SELECT, UPDATE
public.packaging = SELECT, UPDATE
public.person = SELECT, UPDATE
public.personlanguage = SELECT, UPDATE
diff --git a/lib/lp/soyuz/configure.zcml b/lib/lp/soyuz/configure.zcml
index 0e35ccb..f159d27 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,11 @@
set_attributes="status distroseries pocket changesfile archive"/>
</class>
<class
+ class="lp.soyuz.model.queue.PackageUploadLog">
+ <allow
+ 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..3b44a13 100644
--- a/lib/lp/soyuz/interfaces/queue.py
+++ b/lib/lp/soyuz/interfaces/queue.py
@@ -11,6 +11,7 @@ __all__ = [
'IHasQueueItems',
'IPackageUploadQueue',
'IPackageUpload',
+ 'IPackageUploadLog',
'IPackageUploadBuild',
'IPackageUploadSource',
'IPackageUploadCustom',
@@ -149,6 +150,8 @@ 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(
@@ -711,6 +714,20 @@ class IPackageUploadCustom(Interface):
"""
+class IPackageUploadLog(Interface):
+ package_upload = Attribute("Original package upload")
+
+ date_created = Attribute("When this action happened")
+
+ person = Attribute("Who did this action")
+
+ old_status = Attribute("Old status")
+
+ new_status = Attribute("New status")
+
+ comment = Attribute("User's comment about this change")
+
+
class IPackageUploadSet(Interface):
"""Represents a set of IPackageUploads"""
diff --git a/lib/lp/soyuz/model/queue.py b/lib/lp/soyuz/model/queue.py
index 0fd1799..fcfdc2c 100644
--- a/lib/lp/soyuz/model/queue.py
+++ b/lib/lp/soyuz/model/queue.py
@@ -1,10 +1,11 @@
-# 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',
+ 'PackageUploadLog',
'PackageUploadBuild',
'PackageUploadSource',
'PackageUploadCustom',
@@ -13,6 +14,7 @@ __all__ = [
from itertools import chain
+import pytz
from sqlobject import (
ForeignKey,
SQLMultipleJoin,
@@ -29,6 +31,8 @@ from storm.locals import (
SQL,
Unicode,
)
+from storm.properties import DateTime
+from storm.references import ReferenceSet
from storm.store import (
EmptyResultSet,
Store,
@@ -106,7 +110,7 @@ from lp.soyuz.interfaces.queue import (
QueueInconsistentStateError,
QueueSourceAcceptError,
QueueStateWriteProtectedError,
- )
+ IPackageUploadLog)
from lp.soyuz.interfaces.section import ISectionSet
from lp.soyuz.mail.packageupload import PackageUploadMailer
from lp.soyuz.model.binarypackagename import BinaryPackageName
@@ -204,6 +208,20 @@ class PackageUpload(SQLBase):
self.addSearchableNames([self.package_copy_job.package_name])
self.addSearchableVersions([self.package_copy_job.package_version])
+ @property
+ def logs(self):
+ return Store.of(self).find(PackageUploadLog,
+ PackageUploadLog.package_upload == self)
+
+ def _addLog(self, person, new_status, comment=None):
+ return Store.of(self).add(PackageUploadLog(
+ package_upload=self,
+ person=person,
+ old_status=self.status,
+ new_status=new_status,
+ comment=comment
+ ))
+
@cachedproperty
def sources(self):
return list(self._sources)
@@ -571,6 +589,7 @@ class PackageUpload(SQLBase):
def acceptFromQueue(self, user=None):
"""See `IPackageUpload`."""
+ self._addLog(user, PackageUploadStatus.ACCEPTED, None)
if self.package_copy_job is None:
self._acceptNonSyncFromQueue()
else:
@@ -581,6 +600,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 +1173,24 @@ def get_properties_for_binary(bpr):
}
+@implementer(IPackageUploadLog)
+class PackageUploadLog(SQLBase):
+ 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)
+
+ person_id = Int(name='person', allow_none=True)
+ person = Reference(person_id, 'Person.id')
+
+ old_status = EnumCol(schema=PackageUploadStatus)
+
+ new_status = EnumCol(schema=PackageUploadStatus)
+
+ comment = Unicode(allow_none=True)
+
+
@implementer(IPackageUploadBuild)
class PackageUploadBuild(SQLBase):
"""A Queue item's related builds."""
diff --git a/lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt b/lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt
index 7417a7b..1a5f2cc 100644
--- a/lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt
+++ b/lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt
@@ -510,7 +510,7 @@ values:
>>> filelist = find_tags_by_class(anon_browser.contents, 'queue-2')
>>> for row in filelist:
... print(extract_text(row))
- pmount_1.0-1_all.deb (18 bytes) NEW 0.1-1 restricted admin extra
+ 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:
@@ -546,6 +546,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()
@@ -599,6 +608,20 @@ 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()
+ >>> 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
+ Accepted...a moment ago...by Sample Person...
+
Clean up
========
diff --git a/lib/lp/soyuz/templates/distroseries-queue.pt b/lib/lp/soyuz/templates/distroseries-queue.pt
index 59b77d5..c210a0b 100644
--- a/lib/lp/soyuz/templates/distroseries-queue.pt
+++ b/lib/lp/soyuz/templates/distroseries-queue.pt
@@ -149,6 +149,18 @@
</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" />
+ <span tal:condition="log/person">
+ by <span tal:content="structure log/person/fmt:link" />
+ </span>
+ <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 5f78fe2..c74ad9b 100644
--- a/lib/lp/soyuz/tests/test_packageupload.py
+++ b/lib/lp/soyuz/tests/test_packageupload.py
@@ -1,4 +1,4 @@
-# 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).
"""Test Build features."""
@@ -14,7 +14,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,
@@ -84,6 +87,20 @@ class PackageUploadTestCase(TestCaseWithFactory):
super(PackageUploadTestCase, self).setUp()
self.test_publisher = SoyuzTestPublisher()
+ def test_add_log_entry(self):
+ upload = self.factory.makePackageUpload(
+ status=PackageUploadStatus.UNAPPROVED)
+ upload = removeSecurityProxy(upload)
+ self.assertEqual(upload.logs.count(), 0)
+
+ person = self.factory.makePerson()
+
+ upload._addLog(person, PackageUploadStatus.REJECTED, 'just because')
+
+ self.assertThat(upload.logs.one(), MatchesStructure.byEquality(
+ person=person, old_status=PackageUploadStatus.UNAPPROVED,
+ new_status=PackageUploadStatus.REJECTED, comment='just because'))
+
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
@@ -353,6 +370,12 @@ class PackageUploadTestCase(TestCaseWithFactory):
# it goes straight to DONE.)
upload_one.acceptFromQueue()
self.assertEqual("DONE", upload_one.status.name)
+
+ log = upload_one.logs.one()
+ self.assertThat(log, MatchesStructure.byEquality(
+ person=None, old_status=PackageUploadStatus.UNAPPROVED,
+ new_status=PackageUploadStatus.ACCEPTED, comment=None
+ ))
transaction.commit()
# Trying to accept the second fails.
@@ -361,9 +384,17 @@ class PackageUploadTestCase(TestCaseWithFactory):
self.assertEqual("UNAPPROVED", upload_two.status.name)
# Rejecting the second upload works.
- upload_two.rejectFromQueue(self.factory.makePerson())
+ person = self.factory.makePerson()
+ upload_two.rejectFromQueue(person, 'Because yes')
self.assertEqual("REJECTED", upload_two.status.name)
+ self.assertEqual(upload_two.logs.count(), 2)
+ log = upload_two.logs.order_by('id')[1]
+ self.assertThat(log, MatchesStructure.byEquality(
+ person=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()
