launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jml/launchpad/archive-commercial-rename-support/+merge/107819

This branch updates the Launchpad code base in preparation for a database patch that will rename Archive.commercial to Archive.suppress_subscription_notifications.

It deletes the commercial field from the class (unused), and changes the suppress_subscription_notifications property to first check with the db whether the 'archive' table has a 'commercial' field.  If so, it uses that, if not, it uses the new name.

This approach works because Archive.commercial is never queried on, never used in a WHERE clause.

Other approaches discussed and dismissed were:
  * hack Storm to support this somehow (too hard)
  * add a new column and have that update with triggers; deploy that; change code to always use new column; deploy that; remove old column and triggers (quite cumbersome)

This branch adds 30 lines of code.  This are mostly temporary, as they can be deleted after the rename.  Further, we are currently 140 lines in credit on this arc of work anyway.  Happy to count those toward that total, leaving us at 110. 
-- 
https://code.launchpad.net/~jml/launchpad/archive-commercial-rename-support/+merge/107819
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jml/launchpad/archive-commercial-rename-support into lp:launchpad.

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2012-05-22 10:50:19 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2012-05-29 16:01:24 +0000
@@ -319,14 +319,6 @@
     is_main = Bool(
         title=_("True if archive is a main archive type"), required=False)
 
-    commercial = Bool(
-        title=_("Commercial"),
-        required=True,
-        description=_(
-            "True if the archive is for commercial applications in the "
-            "Ubuntu Software Centre.  Governs whether subscribers or "
-            "uploaders get mail from Launchpad about archive events."))
-
     suppress_subscription_notifications = exported(
         Bool(
             title=_("Suppress subscription notifications"),

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-05-14 01:29:38 +0000
+++ lib/lp/soyuz/model/archive.py	2012-05-29 16:01:24 +0000
@@ -78,10 +78,8 @@
 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.lpstorm import (
-    ISlaveStore,
-    IStore,
-    )
+from lp.services.database.lpstorm import ISlaveStore
+from lp.services.database.postgresql import table_has_column
 from lp.services.database.sqlbase import (
     cursor,
     quote,
@@ -325,9 +323,6 @@
         dbName='external_dependencies', notNull=False, default=None,
         storm_validator=storm_validate_external_dependencies)
 
-    commercial = BoolCol(
-        dbName='commercial', notNull=True, default=False)
-
     def _init(self, *args, **kw):
         """Provide the right interface for URL traversal."""
         SQLBase._init(self, *args, **kw)
@@ -340,13 +335,56 @@
         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):
-        return self.commercial
+        """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):
-        self.commercial = 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