Merge ~pappacena/launchpad:change-override-log into launchpad:master

[Thiago F. Pappacena]

Saving and showing the user that requested to override a package publishing.

[Thiago F. Pappacena (pappacena)]

cjwatson, this is still a WIP (I'm still missing tests, for example), but I would like to get a "pre-review", just to make sure we are aligned on what should be done here.

Basically, I checked lp:ubuntu-archive-tools repo and it seems that the `change-override` scripts call the changeOverride on the API, with an authenticated user.

So, on LP side, I'm getting the logged in user and setting it on the "creator" field both for Source and Binary PackagePublishingHistory. Then, showing it on the details of the +publishinghistory page.

[Colin Watson (cjwatson)]

This looks like broadly the right sort of direction. The main thing that I think needs to change is to explicitly pass the principal down from the caller (using @call_with(user=REQUEST_USER) or similar in the interface declaration) rather than picking out the principal in model code.

[Colin Watson (cjwatson)]

review: Needs Information

[Thiago F. Pappacena (pappacena)]

Added more info about why it's showing only for PENDING, cjwatson. Let me know what do you think about showing in every other situation.

I'm doing the other requests right now.

[Thiago F. Pappacena (pappacena)]

cjwatson, pushed the requested change. It should now show the "Created by ..." always, apart from deleted and copied packages, which are special cases.

[Colin Watson (cjwatson)]

review: Approve

[Thiago F. Pappacena (pappacena)]

Changes pushed. It should good to go as soon as we deploy the database change to production.

[Colin Watson (cjwatson)]

This can land now - the database patch is in place.

[Thiago F. Pappacena (pappacena)]

Thanks! I'll top-approve it now.

diff --git a/lib/lp/soyuz/interfaces/publishing.py b/lib/lp/soyuz/interfaces/publishing.py
index 02c3615..4697209 100644
--- a/lib/lp/soyuz/interfaces/publishing.py
+++ b/lib/lp/soyuz/interfaces/publishing.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).
 
 """Publishing interfaces."""
@@ -561,8 +561,9 @@ class ISourcePackagePublishingHistoryEdit(IPublishingEdit):
         new_component=TextLine(title=u"The new component name."),
         new_section=TextLine(title=u"The new section name."))
     @export_write_operation()
+    @call_with(creator=REQUEST_USER)
     @operation_for_version("devel")
-    def changeOverride(new_component=None, new_section=None):
+    def changeOverride(new_component=None, new_section=None, creator=None):
         """Change the component and/or section of this publication.
 
         It is changed only if the argument is not None.
@@ -663,6 +664,13 @@ class IBinaryPackagePublishingHistoryPublic(IPublishingView):
             title=_('The build which superseded this one'),
             required=False, readonly=False,
             )
+    creator = exported(
+        Reference(
+            IPerson,
+            title=_('Publication Creator'),
+            description=_('The IPerson who created this publication.'),
+            required=False, readonly=True
+        ))
     datecreated = exported(
         Datetime(
             title=_('Date Created'),
@@ -840,9 +848,11 @@ class IBinaryPackagePublishingHistoryEdit(IPublishingEdit):
         new_phased_update_percentage=Int(
             title=u"The new phased update percentage."))
     @export_write_operation()
+    @call_with(creator=REQUEST_USER)
     @operation_for_version("devel")
     def changeOverride(new_component=None, new_section=None,
-                       new_priority=None, new_phased_update_percentage=None):
+                       new_priority=None, new_phased_update_percentage=None,
+                       creator=None):
         """Change the component/section/priority/phase of this publication.
 
         It is changed only if the argument is not None.
diff --git a/lib/lp/soyuz/model/publishing.py b/lib/lp/soyuz/model/publishing.py
index 3e73e98..ae7b414 100644
--- a/lib/lp/soyuz/model/publishing.py
+++ b/lib/lp/soyuz/model/publishing.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).
 
 __metaclass__ = type
@@ -471,7 +471,8 @@ class SourcePackagePublishingHistory(SQLBase, ArchivePublisherBase):
 
             self.supersededby = dominant.sourcepackagerelease
 
-    def changeOverride(self, new_component=None, new_section=None):
+    def changeOverride(self, new_component=None, new_section=None,
+                       creator=None):
         """See `ISourcePackagePublishingHistory`."""
         # Check we have been asked to do something
         if (new_component is None and
@@ -516,6 +517,7 @@ class SourcePackagePublishingHistory(SQLBase, ArchivePublisherBase):
             pocket=self.pocket,
             component=new_component,
             section=new_section,
+            creator=creator,
             archive=self.archive)
 
     def copyTo(self, distroseries, pocket, archive, override=None,
@@ -629,6 +631,9 @@ class BinaryPackagePublishingHistory(SQLBase, ArchivePublisherBase):
     phased_update_percentage = IntCol(
         dbName='phased_update_percentage', notNull=False, default=None)
     scheduleddeletiondate = UtcDateTimeCol(default=None)
+    creator = ForeignKey(
+        dbName='creator', foreignKey='Person',
+        storm_validator=validate_public_person, notNull=False, default=None)
     datepublished = UtcDateTimeCol(default=None)
     datecreated = UtcDateTimeCol(default=UTC_NOW)
     datesuperseded = UtcDateTimeCol(default=None)
@@ -794,7 +799,8 @@ class BinaryPackagePublishingHistory(SQLBase, ArchivePublisherBase):
             dominated.supersede(dominant, logger)
 
     def changeOverride(self, new_component=None, new_section=None,
-                       new_priority=None, new_phased_update_percentage=None):
+                       new_priority=None, new_phased_update_percentage=None,
+                       creator=None):
         """See `IBinaryPackagePublishingHistory`."""
 
         # Check we have been asked to do something
@@ -873,6 +879,7 @@ class BinaryPackagePublishingHistory(SQLBase, ArchivePublisherBase):
                 component=new_component,
                 section=new_section,
                 priority=new_priority,
+                creator=creator,
                 archive=debug.archive,
                 phased_update_percentage=new_phased_update_percentage)
 
@@ -888,6 +895,7 @@ class BinaryPackagePublishingHistory(SQLBase, ArchivePublisherBase):
             section=new_section,
             priority=new_priority,
             archive=self.archive,
+            creator=creator,
             phased_update_percentage=new_phased_update_percentage)
 
     def copyTo(self, distroseries, pocket, archive):
diff --git a/lib/lp/soyuz/stories/soyuz/xx-packagepublishinghistory.txt b/lib/lp/soyuz/stories/soyuz/xx-packagepublishinghistory.txt
index 31e98b7..51a629b 100644
--- a/lib/lp/soyuz/stories/soyuz/xx-packagepublishinghistory.txt
+++ b/lib/lp/soyuz/stories/soyuz/xx-packagepublishinghistory.txt
@@ -23,10 +23,35 @@ shows the complete history of a package in all series.
     >>> print(extract_text(table))
     Date    Status    Target     Pocket   Component Section Version
     ... UTC Published Breezy ... release  main      base    666
+    Created ... ago by Foo Bar
     Published ... ago
     >>> print(table.findAll("tr")[2].td["colspan"])
     8
 
+A change-override request should show who made the request
+
+    >>> from lp.registry.interfaces.person import IPersonSet
+    >>> from zope.component import getUtility
+
+    >>> login('[e-mail]')
+    >>> person = getUtility(IPersonSet).getByEmail('[e-mail]')
+    >>> new_pub = source_pub.changeOverride(
+    ...     new_component='universe', creator=person)
+    >>> logout()
+
+    >>> anon_browser.open(
+    ...     'http://launchpad.test/ubuntutest/+source/test-history/'
+    ...     '+publishinghistory')
+    >>> table = find_tag_by_id(anon_browser.contents, 'publishing-summary')
+    >>> print(extract_text(table))
+    Date    Status    Target     Pocket   Component Section Version
+    ... UTC Pending   Breezy ... release  universe  base    666
+    Created ... ago by Foo Bar
+    ... UTC Published Breezy ... release  main      base    666
+    Created ... ago by Foo Bar
+    Published ... ago
+
+
 A publishing record will be shown as deleted in the publishing history after a
 request for deletion by a user.
 
@@ -42,6 +67,8 @@ request for deletion by a user.
     >>> table = find_tag_by_id(anon_browser.contents, 'publishing-summary')
     >>> print(extract_text(table))
     Date    Status    Target     Pocket   Component Section Version
+    ... UTC Pending   Breezy ... release  universe  base    666
+    Created ... ago by Foo Bar
             Deleted   Breezy ... release  main      base    666
     Deleted ... ago by ... fix bug 1
     Published ... ago
diff --git a/lib/lp/soyuz/stories/webservice/xx-binary-package-publishing.txt b/lib/lp/soyuz/stories/webservice/xx-binary-package-publishing.txt
index 81482f2..722c4c0 100644
--- a/lib/lp/soyuz/stories/webservice/xx-binary-package-publishing.txt
+++ b/lib/lp/soyuz/stories/webservice/xx-binary-package-publishing.txt
@@ -71,6 +71,7 @@ Each binary publication exposes a number of properties:
     binary_package_version: u'1.0'
     build_link: u'http://.../~cprov/+archive/ubuntu/ppa/+build/28'
     component_name: u'main'
+    creator_link: None
     date_created: u'2007-08-10T13:00:00+00:00'
     date_made_pending: None
     date_published: u'2007-08-10T13:00:01+00:00'
diff --git a/lib/lp/soyuz/templates/packagepublishing-details.pt b/lib/lp/soyuz/templates/packagepublishing-details.pt
index 0b0102c..4c65323 100644
--- a/lib/lp/soyuz/templates/packagepublishing-details.pt
+++ b/lib/lp/soyuz/templates/packagepublishing-details.pt
@@ -4,6 +4,14 @@
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
   omit-tag="">
   <ul>
+    <li tal:condition="python: context.creator
+                               and not view.wasCopied()
+                               and not view.wasDeleted()">
+      Created
+      <span tal:attributes="title context/datecreated/fmt:datetime"
+            tal:content="context/datecreated/fmt:displaydate" />
+      by <a tal:replace="structure context/creator/fmt:link"/>
+    </li>
     <li tal:condition="view/isRemoved">
       Removed from disk
       <span tal:attributes="title context/dateremoved/fmt:datetime"
diff --git a/lib/lp/soyuz/tests/test_publishing.py b/lib/lp/soyuz/tests/test_publishing.py
index c29028e..e65003d 100644
--- a/lib/lp/soyuz/tests/test_publishing.py
+++ b/lib/lp/soyuz/tests/test_publishing.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).
 
 """Test native publication workflow for Soyuz. """
@@ -62,6 +62,9 @@ from lp.soyuz.model.publishing import (
     SourcePackagePublishingHistory,
     )
 from lp.testing import (
+    login_admin,
+    person_logged_in,
+    record_two_runs,
     StormStatementRecorder,
     TestCaseWithFactory,
     )
@@ -72,6 +75,7 @@ from lp.testing.dbuser import (
 from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
     LaunchpadZopelessLayer,
     ZopelessDatabaseLayer,
     )
@@ -997,13 +1001,15 @@ class TestPublishingSetLite(TestCaseWithFactory):
             debug_non_match_bpph.status, PackagePublishingStatus.PENDING)
 
     def test_changeOverride_also_overrides_debug_package(self):
+        user = self.factory.makePerson()
         bpph, debug_bpph = self.factory.makeBinaryPackagePublishingHistory(
             pocket=PackagePublishingPocket.RELEASE, with_debug=True)
         new_section = self.factory.makeSection()
-        new_bpph = bpph.changeOverride(new_section=new_section)
+        new_bpph = bpph.changeOverride(new_section=new_section, creator=user)
         publishing_set = getUtility(IPublishingSet)
         [new_debug_bpph] = publishing_set.findCorrespondingDDEBPublications(
             [new_bpph])
+        self.assertEqual(new_debug_bpph.creator, user)
         self.assertEqual(new_debug_bpph.section, new_section)
 
     def test_requestDeletion_forbids_debug_package(self):
@@ -1524,6 +1530,21 @@ class TestChangeOverride(TestNativePublishingBase):
             binary=True, new_component="universe", new_section="misc",
             new_priority="extra", new_phased_update_percentage=90)
 
+    def test_change_binary_logged_in_user(self):
+        person = self.factory.makePerson()
+        new_pub = self.assertCanOverride(
+            binary=True, new_component="universe", new_section="misc",
+            new_priority="extra", new_phased_update_percentage=90,
+            creator=person)
+        self.assertEqual(person, new_pub.creator)
+
+    def test_change_source_logged_in_user(self):
+        person = self.factory.makePerson()
+        new_pub = self.assertCanOverride(
+            binary=False, new_component="universe", new_section="misc",
+            creator=person)
+        self.assertEqual(person, new_pub.creator)
+
     def test_set_and_clear_phased_update_percentage(self):
         # new_phased_update_percentage=<integer> sets a phased update
         # percentage; new_phased_update_percentage=100 clears it.
@@ -1572,3 +1593,47 @@ class TestChangeOverride(TestNativePublishingBase):
         # archive.
         self.assertCannotOverride(new_component="partner")
         self.assertCannotOverride(binary=True, new_component="partner")
+
+
+class TestPublishingHistoryView(TestCaseWithFactory):
+    layer = LaunchpadFunctionalLayer
+
+    def test_constant_query_counts_on_publishing_history_change_override(self):
+        admin = self.factory.makeAdministrator()
+        normal_user = self.factory.makePerson()
+
+        with person_logged_in(admin):
+            test_publisher = SoyuzTestPublisher()
+            test_publisher.prepareBreezyAutotest()
+
+            source_pub = test_publisher.getPubSource(
+                "test-history", status=PackagePublishingStatus.PUBLISHED)
+        url = ("http://launchpad.test/ubuntutest/+source/test-history"
+               "/+publishinghistory")
+
+        def insert_more_publish_history():
+            person1 = self.factory.makePerson()
+            new_component = (
+                'universe' if source_pub.component.name == 'main'
+                else 'main')
+            source_pub.changeOverride(
+                new_component=new_component, creator=person1)
+
+            person2 = self.factory.makePerson()
+            new_section = ('web' if source_pub.section.name == 'base'
+                           else 'base')
+            source_pub.changeOverride(
+                new_section=new_section, creator=person2)
+
+        def show_page():
+            self.getUserBrowser(url, normal_user)
+
+        # Make sure to have all the history fitting in one page.
+        self.pushConfig("launchpad", default_batch_size=50)
+
+        recorder1, recorder2 = record_two_runs(
+            show_page, insert_more_publish_history,
+            1, 10, login_method=login_admin)
+
+        self.assertThat(recorder1, HasQueryCount(Equals(26)))
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 8248fa8..2bdac13 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -3989,7 +3989,8 @@ class BareLaunchpadObjectFactory(ObjectFactory):
                                            sourcepackagename=None,
                                            version=None,
                                            architecturespecific=False,
-                                           with_debug=False, with_file=False):
+                                           with_debug=False, with_file=False,
+                                           creator=None):
         """Make a `BinaryPackagePublishingHistory`."""
         if distroarchseries is None:
             if archive is None:
@@ -4082,7 +4083,8 @@ class BareLaunchpadObjectFactory(ObjectFactory):
                 binpackageformat=BinaryPackageFormat.DDEB,
                 sourcepackagename=sourcepackagename,
                 architecturespecific=architecturespecific,
-                with_file=with_file)
+                with_file=with_file,
+                creator=creator)
             removeSecurityProxy(bpph.binarypackagerelease).debug_package = (
                 debug_bpph.binarypackagerelease)
             return bpphs[0], debug_bpph