Merge lp:~jtv/launchpad/bug-834388 into lp:launchpad

[Jeroen T. Vermeulen]

[r=danilo][bug=834388] Don't bypass ORM in IPublishingSet.setMultipleDeleted.

= Summary =

IPublishingSet.setMultipleDeleted updates the database through raw SQL. This necessitates commits in tests, a separate ORM-based implementation in [SB]PPH for individual records, and so on.

== Proposed fix ==

Use storm.ResultSet.set() instead. Implement [SB]PPH.setDeleted in terms of the updated method. Remove the now unnecessary commits from tests.

== Pre-implementation notes ==

Suggested by wgrant; I wasn't aware of storm.ResultSet.set.

== Implementation details ==

A lot of the diff is in ftpmaster.py. It goes through some contortions to delete [SB]PPHs one by one, when the normal case is actually ideally suited for mass deletions. I changed that, but in a roundabout way: I now need to filter the BPPHs out of the list of removables after first gathering them for no better reason than to maintain logging output.

Still, I hope it's a first step towards moving even more of this into IPublishingSet.requestDeletion in the future. We may decide that we don't need to log binaries that are deleted as a consequence of source deletion, or we may add a source-only option to requestDeletion, or we may decide that we don't need source-only and/or binary-only deletions at all. Adding an informational return value to requestDeletion may obviate some of the contortions. I wish I could go into it and do a better job right now, but we're on a feature death march. The only reason I made this change, really, is that I just added DSDJ creation to requestDeletion, and thus slowed this inefficient code down even further.

== Tests ==

{{{
./bin/test lp.soyuz.tests.test_publishing
./bin/test lp.soyuz -t ftpmaster
}}}

== Demo and Q/A ==

Archive deletion must still work, and mark publication records as deleted.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/tests/test_publishing.py
  lib/lp/soyuz/model/publishing.py
  lib/lp/soyuz/scripts/ftpmaster.py

[Данило Шеган (danilo)]

Assuming all is well tested and you watch for performance degradation, all is fine.

review: Approve

=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2011-08-26 16:44:05 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2011-09-05 03:15:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0211,E0213
@@ -1149,6 +1149,13 @@
         :return: a `LibraryFileAlias` instance or None
         """
 
+    def setMultipleDeleted(publication_class, ds, removed_by,
+                           removal_comment=None):
+        """Mark publications as deleted.
+
+        This is a supporting operation for a deletion request.
+        """
+
     def requestDeletion(sources, removed_by, removal_comment=None):
         """Delete the source and binary publications specified.
 

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2011-08-30 16:48:26 +0000
+++ lib/lp/soyuz/model/publishing.py	2011-09-05 03:15:31 +0000
@@ -334,10 +334,8 @@
 
     def setDeleted(self, removed_by, removal_comment=None):
         """Set to DELETED status."""
-        self.status = PackagePublishingStatus.DELETED
-        self.datesuperseded = UTC_NOW
-        self.removed_by = removed_by
-        self.removal_comment = removal_comment
+        getUtility(IPublishingSet).setMultipleDeleted(
+            self.__class__, [self.id], removed_by, removal_comment)
 
     def requestObsolescence(self):
         """See `IArchivePublisher`."""
@@ -1966,28 +1964,19 @@
         if len(ids) == 0:
             return
 
-        table = publication_class.__name__
-        permitted_tables = [
-            'BinaryPackagePublishingHistory',
-            'SourcePackagePublishingHistory',
+        permitted_classes = [
+            BinaryPackagePublishingHistory,
+            SourcePackagePublishingHistory,
             ]
-        assert table in permitted_tables, "Deleting wrong type."
-
-        params = sqlvalues(
-            deleted=PackagePublishingStatus.DELETED, now=UTC_NOW,
-            removal_comment=removal_comment, removed_by=removed_by)
-
-        IMasterStore(publication_class).execute("\n".join([
-            "UPDATE %s" % table,
-            """
-            SET
-                status = %(deleted)s,
-                datesuperseded = %(now)s,
-                removed_by = %(removed_by)s,
-                removal_comment = %(removal_comment)s
-            """ % params,
-            "WHERE id IN %s" % sqlvalues(ids),
-            ]))
+        assert publication_class in permitted_classes, "Deleting wrong type."
+
+        affected_pubs = IMasterStore(publication_class).find(
+            publication_class, publication_class.id.is_in(ids))
+        affected_pubs.set(
+            status=PackagePublishingStatus.DELETED,
+            datesuperseded=UTC_NOW,
+            removed_byID=removed_by.id,
+            removal_comment=removal_comment)
 
     def requestDeletion(self, sources, removed_by, removal_comment=None):
         """See `IPublishingSet`."""

=== modified file 'lib/lp/soyuz/scripts/ftpmaster.py'
--- lib/lp/soyuz/scripts/ftpmaster.py	2011-08-25 08:29:37 +0000
+++ lib/lp/soyuz/scripts/ftpmaster.py	2011-09-05 03:15:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """FTPMaster utilities."""
@@ -52,6 +52,7 @@
     )
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.sourcepackage import SourcePackageFileType
+from lp.services.browser_helpers import get_plural_text
 from lp.services.scripts.base import (
     LaunchpadScript,
     LaunchpadScriptFailure,
@@ -1256,10 +1257,10 @@
 
         self.logger.info("Removing candidates:")
         for removable in removables:
-            self.logger.info('\t%s' % removable.displayname)
+            self.logger.info('\t%s', removable.displayname)
 
-        self.logger.info("Removed-by: %s" % removed_by.displayname)
-        self.logger.info("Comment: %s" % self.options.removal_comment)
+        self.logger.info("Removed-by: %s", removed_by.displayname)
+        self.logger.info("Comment: %s", self.options.removal_comment)
 
         removals = []
         for removable in removables:
@@ -1268,14 +1269,12 @@
                 removal_comment=self.options.removal_comment)
             removals.append(removable)
 
-        if len(removals) == 1:
-            self.logger.info(
-                "%s package successfully removed." % len(removals))
-        elif len(removals) > 1:
-            self.logger.info(
-                "%s packages successfully removed." % len(removals))
-        else:
+        if len(removals) == 0:
             self.logger.info("No package removed (bug ?!?).")
+        else:
+            self.logger.info(
+                "%d %s successfully removed.", len(removals),
+                get_plural_text(len(removals), "package", "packages"))
 
         # Information returned mainly for the benefit of the test harness.
         return removals

=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py	2011-08-26 09:17:08 +0000
+++ lib/lp/soyuz/tests/test_publishing.py	2011-09-05 03:15:31 +0000
@@ -1179,8 +1179,6 @@
         spph = self.factory.makeSourcePackagePublishingHistory()
         getUtility(IPublishingSet).requestDeletion(
             [spph], self.factory.makePerson())
-        # XXX JeroenVermeulen 2011-08-25, bug=834388: obviate commit.
-        transaction.commit()
         self.assertEqual(PackagePublishingStatus.DELETED, spph.status)
 
     def test_requestDeletion_leaves_other_SPPHs_alone(self):
@@ -1188,8 +1186,6 @@
         other_spph = self.factory.makeSourcePackagePublishingHistory()
         getUtility(IPublishingSet).requestDeletion(
             [other_spph], self.factory.makePerson())
-        # XXX JeroenVermeulen 2011-08-25, bug=834388: obviate commit.
-        transaction.commit()
         self.assertEqual(PackagePublishingStatus.PENDING, spph.status)
 
     def test_requestDeletion_marks_attached_BPPHs_deleted(self):
@@ -1197,8 +1193,6 @@
         spph = self.factory.makeSPPHForBPPH(bpph)
         getUtility(IPublishingSet).requestDeletion(
             [spph], self.factory.makePerson())
-        # XXX JeroenVermeulen 2011-08-25, bug=834388: obviate commit.
-        transaction.commit()
         self.assertEqual(PackagePublishingStatus.DELETED, spph.status)
 
     def test_requestDeletion_leaves_other_BPPHs_alone(self):
@@ -1206,8 +1200,6 @@
         unrelated_spph = self.factory.makeSourcePackagePublishingHistory()
         getUtility(IPublishingSet).requestDeletion(
             [unrelated_spph], self.factory.makePerson())
-        # XXX JeroenVermeulen 2011-08-25, bug=834388: obviate commit.
-        transaction.commit()
         self.assertEqual(PackagePublishingStatus.PENDING, bpph.status)
 
     def test_requestDeletion_accepts_empty_sources_list(self):