launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:stormify-signedcodeofconduct into launchpad:master.

Commit message:
Convert SignedCodeOfConduct to Storm

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/386940

Several sites constructed a SignedCodeOfConduct with a signing_key_owner argument, which doesn't exist in this model but was apparently ignored by `storm.sqlobject`.  Remove these arguments rather than perpetuating this confusion.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:stormify-signedcodeofconduct into launchpad:master.

diff --git a/lib/lp/registry/browser/person.py b/lib/lp/registry/browser/person.py
index 8ece6c2..83e5757 100644
--- a/lib/lp/registry/browser/person.py
+++ b/lib/lp/registry/browser/person.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).
 
 """Person-related view classes."""
@@ -2204,7 +2204,7 @@ class PersonCodeOfConductEditView(LaunchpadView):
             for sig_id in sig_ids:
                 sig_id = int(sig_id)
                 # Deactivating signature.
-                comment = 'Deactivated by Owner'
+                comment = u'Deactivated by Owner'
                 sCoC_util.modifySignature(sig_id, self.user, comment, False)
 
 
diff --git a/lib/lp/registry/browser/tests/test_codeofconduct.py b/lib/lp/registry/browser/tests/test_codeofconduct.py
index 846a439..650ade3 100644
--- a/lib/lp/registry/browser/tests/test_codeofconduct.py
+++ b/lib/lp/registry/browser/tests/test_codeofconduct.py
@@ -1,8 +1,10 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for Code of Conduct views."""
 
+from __future__ import absolute_import, print_function, unicode_literals
+
 __metaclass__ = type
 
 from zope.component import getUtility
@@ -50,7 +52,7 @@ class TestSignedCodeOfConductAckView(TestCaseWithFactory):
             self.signed_coc_set, name="+new", form=form,
             principal=self.admin)
         self.assertEqual([], view.errors)
-        results = self.signed_coc_set.searchByUser(self.owner.id)
+        results = self.signed_coc_set.searchByUser(self.owner)
         self.assertEqual(1, results.count())
         signed_coc = results[0]
         self.assertEqual(self.admin, signed_coc.recipient)
@@ -71,7 +73,6 @@ class SignCodeOfConductTestCase(TestCaseWithFactory):
         """Return a SignedCodeOfConduct using dummy text."""
         signed_coc = SignedCodeOfConduct(
             owner=user, signing_key_fingerprint=gpg_key.fingerprint,
-            signing_key_owner=gpg_key.owner,
             signedcode="Dummy CoC signed text.", active=True)
         return signed_coc
 
diff --git a/lib/lp/registry/interfaces/codeofconduct.py b/lib/lp/registry/interfaces/codeofconduct.py
index b7248e3..bd8df54 100644
--- a/lib/lp/registry/interfaces/codeofconduct.py
+++ b/lib/lp/registry/interfaces/codeofconduct.py
@@ -1,4 +1,4 @@
-# Copyright 2009 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).
 
 """Interfaces for CodeOfConduct (CoC) and related classes.
@@ -130,8 +130,8 @@ class ISignedCodeOfConductSet(Interface):
     def searchByDisplayname(displayname, searchfor=None):
         """Search SignedCoC by Owner.displayname"""
 
-    def searchByUser(user_id, active=True):
-        """Search SignedCoC by Owner.id, return only the active ones by
+    def searchByUser(user, active=True):
+        """Search SignedCoC by owner, return only the active ones by
         default.
         """
 
diff --git a/lib/lp/registry/model/codeofconduct.py b/lib/lp/registry/model/codeofconduct.py
index a8dac09..1cb1034 100644
--- a/lib/lp/registry/model/codeofconduct.py
+++ b/lib/lp/registry/model/codeofconduct.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).
 
 """A module for CodeOfConduct (CoC) related classes.
@@ -15,12 +15,15 @@ import os
 
 import pytz
 import scandir
-from sqlobject import (
-    BoolCol,
-    ForeignKey,
-    StringCol,
+from storm.locals import (
+    Bool,
+    DateTime,
+    Int,
+    Not,
+    Reference,
+    SQL,
+    Unicode,
     )
-from storm.properties import Unicode
 from zope.component import getUtility
 from zope.interface import implementer
 
@@ -35,12 +38,12 @@ from lp.registry.interfaces.codeofconduct import (
 from lp.registry.interfaces.gpg import IGPGKeySet
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
-from lp.services.database.datetimecol import UtcDateTimeCol
+from lp.services.database.interfaces import IStore
 from lp.services.database.sqlbase import (
     flush_database_updates,
     quote,
-    SQLBase,
     )
+from lp.services.database.stormbase import StormBase
 from lp.services.gpg.interfaces import (
     GPGKeyExpired,
     GPGKeyNotFoundError,
@@ -175,27 +178,38 @@ class CodeOfConductSet:
 
 
 @implementer(ISignedCodeOfConduct)
-class SignedCodeOfConduct(SQLBase):
+class SignedCodeOfConduct(StormBase):
     """Code of Conduct."""
 
-    _table = 'SignedCodeOfConduct'
+    __storm_table__ = 'SignedCodeOfConduct'
 
-    owner = ForeignKey(foreignKey="Person", dbName="owner", notNull=True)
+    id = Int(primary=True)
 
-    signedcode = StringCol(dbName='signedcode', notNull=False, default=None)
+    owner_id = Int(name='owner', allow_none=False)
+    owner = Reference(owner_id, 'Person.id')
+
+    signedcode = Unicode(name='signedcode', allow_none=True, default=None)
 
     signing_key_fingerprint = Unicode()
 
-    datecreated = UtcDateTimeCol(dbName='datecreated', notNull=True,
-                                 default=UTC_NOW)
+    datecreated = DateTime(
+        tzinfo=pytz.UTC, name='datecreated', allow_none=False,
+        default=UTC_NOW)
+
+    recipient_id = Int(name='recipient', allow_none=True, default=None)
+    recipient = Reference(recipient_id, 'Person.id')
 
-    recipient = ForeignKey(foreignKey="Person", dbName="recipient",
-                           notNull=False, default=None)
+    admincomment = Unicode(name='admincomment', allow_none=True, default=None)
 
-    admincomment = StringCol(dbName='admincomment', notNull=False,
-                             default=None)
+    active = Bool(name='active', allow_none=False, default=False)
 
-    active = BoolCol(dbName='active', notNull=True, default=False)
+    def __init__(self, owner, signedcode=None, signing_key_fingerprint=None,
+            recipient=None, active=False):
+        self.owner = owner
+        self.signedcode = signedcode
+        self.signing_key_fingerprint = signing_key_fingerprint
+        self.recipient = recipient
+        self.active = active
 
     @cachedproperty
     def signingkey(self):
@@ -242,11 +256,11 @@ class SignedCodeOfConductSet:
 
     def __getitem__(self, id):
         """Get a Signed CoC Entry."""
-        return SignedCodeOfConduct.get(id)
+        return IStore(SignedCodeOfConduct).get(SignedCodeOfConduct, id)
 
     def __iter__(self):
         """Iterate through the Signed CoC."""
-        return iter(SignedCodeOfConduct.select())
+        return iter(IStore(SignedCodeOfConduct).find(SignedCodeOfConduct))
 
     def verifyAndStore(self, user, signedcode):
         """See ISignedCodeOfConductSet."""
@@ -327,15 +341,15 @@ class SignedCodeOfConductSet:
 
     def searchByDisplayname(self, displayname, searchfor=None):
         """See ISignedCodeOfConductSet."""
-        clauseTables = ['Person']
+        # Circular import.
+        from lp.registry.model.person import Person
 
         # XXX: cprov 2005-02-27:
         # FTI presents problems when query by incomplete names
         # and I'm not sure if the best solution here is to use
         # trivial ILIKE query. Oppinion required on Review.
 
-        # glue Person and SignedCoC table
-        query = 'SignedCodeOfConduct.owner = Person.id'
+        clauses = [SignedCodeOfConduct.owner == Person.id]
 
         # XXX cprov 2005-03-02:
         # I'm not sure if the it is correct way to query ALL
@@ -345,29 +359,25 @@ class SignedCodeOfConductSet:
         # the name shoudl work like a filter, if you don't enter anything
         # you get everything.
         if displayname:
-            query += ' AND Person.fti @@ ftq(%s)' % quote(displayname)
+            clauses.append(SQL('Person.fti @@ ftq(%s)' % quote(displayname)))
 
         # Attempt to search for directive
         if searchfor == 'activeonly':
-            query += ' AND SignedCodeOfConduct.active = true'
-
+            clauses.append(SignedCodeOfConduct.active)
         elif searchfor == 'inactiveonly':
-            query += ' AND SignedCodeOfConduct.active = false'
+            clauses.append(Not(SignedCodeOfConduct.active))
 
-        return SignedCodeOfConduct.select(
-            query, clauseTables=clauseTables,
-            orderBy='SignedCodeOfConduct.active')
+        return IStore(SignedCodeOfConduct).find(
+            SignedCodeOfConduct, *clauses).order_by(SignedCodeOfConduct.active)
 
-    def searchByUser(self, user_id, active=True):
+    def searchByUser(self, user, active=True):
         """See ISignedCodeOfConductSet."""
-        # XXX kiko 2006-08-14:
-        # What is this user_id nonsense? Use objects!
-        return SignedCodeOfConduct.selectBy(ownerID=user_id,
-                                            active=active)
+        return IStore(SignedCodeOfConduct).find(
+            SignedCodeOfConduct, owner=user, active=active)
 
     def modifySignature(self, sign_id, recipient, admincomment, state):
         """See ISignedCodeOfConductSet."""
-        sign = SignedCodeOfConduct.get(sign_id)
+        sign = IStore(SignedCodeOfConduct).get(SignedCodeOfConduct, sign_id)
         sign.active = state
         sign.admincomment = admincomment
         sign.recipient = recipient.id
diff --git a/lib/lp/registry/model/person.py b/lib/lp/registry/model/person.py
index ad5885b..2c422e9 100644
--- a/lib/lp/registry/model/person.py
+++ b/lib/lp/registry/model/person.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).
 
 """Implementation classes for a Person."""
@@ -76,6 +76,7 @@ from storm.info import ClassAlias
 from storm.locals import (
     Int,
     Reference,
+    ReferenceSet,
     )
 from storm.store import (
     EmptyResultSet,
@@ -611,7 +612,7 @@ class Person(
     hide_email_addresses = BoolCol(notNull=True, default=False)
     verbose_bugnotifications = BoolCol(notNull=True, default=True)
 
-    signedcocs = SQLMultipleJoin('SignedCodeOfConduct', joinColumn='owner')
+    signedcocs = ReferenceSet('<primary key>', 'SignedCodeOfConduct.owner_id')
     _ircnicknames = SQLMultipleJoin('IrcID', joinColumn='person')
     jabberids = SQLMultipleJoin('JabberID', joinColumn='person')
 
@@ -2951,7 +2952,8 @@ class Person(
         """See `IPerson`."""
         # Also assigned to by self._members.
         store = Store.of(self)
-        query = And(SignedCodeOfConduct.ownerID == self.id,
+        query = And(
+            SignedCodeOfConduct.owner_id == self.id,
             Person._is_ubuntu_coc_signer_condition())
         return not store.find(SignedCodeOfConduct, query).is_empty()
 
@@ -2967,13 +2969,13 @@ class Person(
     def activesignatures(self):
         """See `IPerson`."""
         sCoC_util = getUtility(ISignedCodeOfConductSet)
-        return sCoC_util.searchByUser(self.id)
+        return sCoC_util.searchByUser(self)
 
     @property
     def inactivesignatures(self):
         """See `IPerson`."""
         sCoC_util = getUtility(ISignedCodeOfConductSet)
-        return sCoC_util.searchByUser(self.id, active=False)
+        return sCoC_util.searchByUser(self, active=False)
 
     @cachedproperty
     def archive(self):
@@ -3944,7 +3946,7 @@ class PersonSet:
                         tables=[SignedCodeOfConduct],
                         where=And(
                             Person._is_ubuntu_coc_signer_condition(),
-                            SignedCodeOfConduct.ownerID == Person.id))),
+                            SignedCodeOfConduct.owner_id == Person.id))),
                     name='is_ubuntu_coc_signer'))
         if need_location or need_api:
             # New people have no location rows
diff --git a/utilities/soyuz-sampledata-setup.py b/utilities/soyuz-sampledata-setup.py
index 2c679f7..9a1edba 100755
--- a/utilities/soyuz-sampledata-setup.py
+++ b/utilities/soyuz-sampledata-setup.py
@@ -1,6 +1,6 @@
 #!/usr/bin/python2 -S
 
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 #
 # This code is based on William Grant's make-ubuntu-sane.py script, but
@@ -322,13 +322,11 @@ def sign_code_of_conduct(person, log):
 
     log.info("Signing Ubuntu code of conduct.")
     signedcocset = getUtility(ISignedCodeOfConductSet)
-    person_id = person.id
-    if signedcocset.searchByUser(person_id).count() == 0:
+    if signedcocset.searchByUser(person).count() == 0:
         fake_gpg_key = LaunchpadObjectFactory().makeGPGKey(person)
         Store.of(person).add(SignedCodeOfConduct(
             owner=person, signing_key_fingerprint=fake_gpg_key.fingerprint,
-            signing_key_owner=fake_gpg_key.owner,
-            signedcode="Normally a signed CoC would go here.", active=True))
+            signedcode=u"Normally a signed CoC would go here.", active=True))
 
 
 def create_ppa_user(username, options, approver, log):