launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxxxxxx>]

Jonathan Lange has proposed merging lp:~jml/launchpad/remove-archive-commercial into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1006295 in Launchpad itself: "Archive's 'suppress_subscription_notifications' attribute is called 'commercial' in the database"
  https://bugs.launchpad.net/launchpad/+bug/1006295

For more details, see:
https://code.launchpad.net/~jml/launchpad/remove-archive-commercial/+merge/109332

This branch removes the code we added to support IArchive.suppress_subscription_notifications being available as 'suppress_subscription_notifications' and as 'commercial' in the database.  Henceforth, it is now just suppress_subscription_notifications. commercial is gone.

This branch may only land after the related database patch (https://code.launchpad.net/~jml/launchpad/db-rename-archive-commercial-to-suppress/+merge/109179) has landed and been applied to all database servers.
-- 
https://code.launchpad.net/~jml/launchpad/remove-archive-commercial/+merge/109332
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jml/launchpad/remove-archive-commercial into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-05-29 15:54:57 +0000
+++ lib/lp/soyuz/model/archive.py	2012-06-08 11:19:19 +0000
@@ -323,6 +323,10 @@
         dbName='external_dependencies', notNull=False, default=None,
         storm_validator=storm_validate_external_dependencies)
 
+    suppress_subscription_notifications = BoolCol(
+        dbName='suppress_subscription_notifications',
+        notNull=True, default=False)
+
     def _init(self, *args, **kw):
         """Provide the right interface for URL traversal."""
         SQLBase._init(self, *args, **kw)
@@ -335,57 +339,6 @@
         else:
             alsoProvides(self, IDistributionArchive)
 
-    # Here we provide manual properties instead of declaring the column in
-    # storm.  This is to handle a transition period where we rename the
-    # 'commercial' column to 'suppress_subscription_notifications'.  During
-    # the transition period, the code needs to work with both column names.
-    #
-    # The approach taken here only works because we never use 'commercial' in
-    # a WHERE clause or anything like that.
-    #
-    # Once the database change has taken place, these properties should be
-    # deleted, and replaced with a class variable declaration that looks
-    # something like:
-    #
-    #   suppress_subscription_notifications = BoolCol(
-    #       dbName='suppress_subscription_notifications',
-    #       notNull=True, default=False)
-
-    def _get_suppress_column_name(self):
-        """Get the name of the column for suppressing notifications.
-
-        Older versions of the database call it 'commercial', newer ones call
-        it 'suppress_subscription_notifications'.
-
-        Works by interrogating PostgreSQL's own records.
-        """
-        # Chose this look-before-you-leap implementation so as to avoid
-        # invalidating the query by forcing a ProgrammingError.
-        cur = cursor()
-        has_old_column = table_has_column(cur, 'archive', 'commercial')
-        if has_old_column:
-            return 'commercial'
-        else:
-            return 'suppress_subscription_notifications'
-
-    @property
-    def suppress_subscription_notifications(self):
-        """See `IArchive`."""
-        store = Store.of(self)
-        store.flush()
-        suppress_column = self._get_suppress_column_name()
-        query = "SELECT %s FROM archive WHERE id=%%s" % (suppress_column,)
-        return store.execute(query, (self.id,)).get_one()[0]
-
-    @suppress_subscription_notifications.setter
-    def suppress_subscription_notifications(self, suppress):
-        """See `IArchive`."""
-        store = Store.of(self)
-        store.flush()
-        suppress_column = self._get_suppress_column_name()
-        query = "UPDATE archive SET %s=%%s WHERE id=%%s" % (suppress_column,)
-        store.execute(query, (bool(suppress), self.id))
-
     # Note: You may safely ignore lint when it complains about this
     # declaration.  As of Python 2.6, this is a perfectly valid way
     # of adding a setter