launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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

Commit message:
Convert BranchSubscription to Storm

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/381293
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:stormify-branchsubscription into launchpad:master.

diff --git a/lib/lp/code/browser/branchsubscription.py b/lib/lp/code/browser/branchsubscription.py
index ca3833f..fbd98bc 100644
--- a/lib/lp/code/browser/branchsubscription.py
+++ b/lib/lp/code/browser/branchsubscription.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).
 
 __metaclass__ = type
@@ -43,7 +43,7 @@ class BranchPortletSubscribersContent(LaunchpadView):
         # The security adaptor will do the job also but we don't want or need
         # the expense of running several complex SQL queries.
         subscriptions = list(self.context.subscriptions)
-        person_ids = [sub.personID for sub in subscriptions]
+        person_ids = [sub.person_id for sub in subscriptions]
         list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
             person_ids, need_validity=True))
         if self.user is not None:
diff --git a/lib/lp/code/browser/decorations.py b/lib/lp/code/browser/decorations.py
index a7ac2d6..c4b0ae7 100644
--- a/lib/lp/code/browser/decorations.py
+++ b/lib/lp/code/browser/decorations.py
@@ -1,4 +1,4 @@
-# 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).
 
 """Decorated model objects used in the browser code."""
@@ -95,7 +95,7 @@ class DecoratedBranch(BzrIdentityMixin):
         """
         if user is None:
             return False
-        return user.id in [sub.personID for sub in self.subscriptions]
+        return user.id in [sub.person_id for sub in self.subscriptions]
 
     @cachedproperty
     def latest_revisions(self):
diff --git a/lib/lp/code/interfaces/branchsubscription.py b/lib/lp/code/interfaces/branchsubscription.py
index c885c78..e5a8861 100644
--- a/lib/lp/code/interfaces/branchsubscription.py
+++ b/lib/lp/code/interfaces/branchsubscription.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 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).
 
 """Bazaar branch subscription interfaces."""
@@ -38,7 +38,7 @@ class IBranchSubscription(Interface):
     export_as_webservice_entry()
 
     id = Int(title=_('ID'), readonly=True, required=True)
-    personID = Int(title=_('Person ID'), required=True, readonly=True)
+    person_id = Int(title=_('Person ID'), required=True, readonly=True)
     person = exported(
         PersonChoice(
             title=_('Person'), required=True, vocabulary='ValidPersonOrTeam',
diff --git a/lib/lp/code/model/branch.py b/lib/lp/code/model/branch.py
index 7811d45..b214c86 100644
--- a/lib/lp/code/model/branch.py
+++ b/lib/lp/code/model/branch.py
@@ -24,8 +24,6 @@ from six.moves.urllib_parse import urlsplit
 from sqlobject import (
     ForeignKey,
     IntCol,
-    SQLMultipleJoin,
-    SQLRelatedJoin,
     StringCol,
     )
 from storm.expr import (
@@ -299,7 +297,8 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
         # this branch.
         self.information_type = information_type
         self._reconcileAccess()
-        if information_type in PRIVATE_INFORMATION_TYPES and self.subscribers:
+        if (information_type in PRIVATE_INFORMATION_TYPES and
+                not self.subscribers.is_empty()):
             # Grant the subscriber access if they can't see the branch.
             service = getUtility(IService, 'sharing')
             blind_subscribers = service.getPeopleWithoutAccess(
@@ -438,11 +437,11 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
         result = result.order_by(Desc(BranchRevision.sequence))
         return DecoratedResultSet(result, operator.itemgetter(0))
 
-    subscriptions = SQLMultipleJoin(
-        'BranchSubscription', joinColumn='branch', orderBy='id')
-    subscribers = SQLRelatedJoin(
-        'Person', joinColumn='branch', otherColumn='person',
-        intermediateTable='BranchSubscription', orderBy='name')
+    subscriptions = ReferenceSet(
+        'id', 'BranchSubscription.branch_id', order_by='BranchSubscription.id')
+    subscribers = ReferenceSet(
+        'id', 'BranchSubscription.branch_id',
+        'BranchSubscription.person_id', 'Person.id', order_by='Person.name')
 
     bug_branches = ReferenceSet(
         'id', 'BugBranch.branch_id', order_by='BugBranch.id')
@@ -1071,9 +1070,10 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
         """See `IBranch`."""
         if person is None:
             return None
-        subscription = BranchSubscription.selectOneBy(
-            person=person, branch=self)
-        return subscription
+        return Store.of(self).find(
+            BranchSubscription,
+            BranchSubscription.person == person,
+            BranchSubscription.branch == self).one()
 
     def getSubscriptionsByLevel(self, notification_levels):
         """See `IBranch`."""
diff --git a/lib/lp/code/model/branchsubscription.py b/lib/lp/code/model/branchsubscription.py
index 49df1c9..74ac06f 100644
--- a/lib/lp/code/model/branchsubscription.py
+++ b/lib/lp/code/model/branchsubscription.py
@@ -1,10 +1,15 @@
-# 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).
 
+from __future__ import absolute_import, print_function, unicode_literals
+
 __metaclass__ = type
 __all__ = ['BranchSubscription']
 
-from sqlobject import ForeignKey
+from storm.locals import (
+    Int,
+    Reference,
+    )
 from zope.interface import implementer
 
 from lp.code.enums import (
@@ -18,29 +23,41 @@ from lp.code.security import BranchSubscriptionEdit
 from lp.registry.interfaces.person import validate_person
 from lp.registry.interfaces.role import IPersonRoles
 from lp.services.database.constants import DEFAULT
-from lp.services.database.enumcol import EnumCol
-from lp.services.database.sqlbase import SQLBase
+from lp.services.database.enumcol import DBEnum
+from lp.services.database.stormbase import StormBase
 
 
 @implementer(IBranchSubscription, IHasBranchTarget)
-class BranchSubscription(SQLBase):
+class BranchSubscription(StormBase):
     """A relationship between a person and a branch."""
 
-    _table = 'BranchSubscription'
-
-    person = ForeignKey(
-        dbName='person', foreignKey='Person',
-        storm_validator=validate_person, notNull=True)
-    branch = ForeignKey(dbName='branch', foreignKey='Branch', notNull=True)
-    notification_level = EnumCol(enum=BranchSubscriptionNotificationLevel,
-                                 notNull=True, default=DEFAULT)
-    max_diff_lines = EnumCol(enum=BranchSubscriptionDiffSize,
-                             notNull=False, default=DEFAULT)
-    review_level = EnumCol(enum=CodeReviewNotificationLevel,
-                                 notNull=True, default=DEFAULT)
-    subscribed_by = ForeignKey(
-        dbName='subscribed_by', foreignKey='Person',
-        storm_validator=validate_person, notNull=True)
+    __storm_table__ = 'BranchSubscription'
+
+    id = Int(primary=True)
+
+    person_id = Int(name='person', allow_none=False, validator=validate_person)
+    person = Reference(person_id, 'Person.id')
+    branch_id = Int(name='branch', allow_none=False)
+    branch = Reference(branch_id, 'Branch.id')
+    notification_level = DBEnum(enum=BranchSubscriptionNotificationLevel,
+                                allow_none=False, default=DEFAULT)
+    max_diff_lines = DBEnum(enum=BranchSubscriptionDiffSize,
+                            allow_none=True, default=DEFAULT)
+    review_level = DBEnum(enum=CodeReviewNotificationLevel,
+                          allow_none=False, default=DEFAULT)
+    subscribed_by_id = Int(
+        name='subscribed_by', allow_none=False, validator=validate_person)
+    subscribed_by = Reference(subscribed_by_id, 'Person.id')
+
+    def __init__(self, person, branch, notification_level, max_diff_lines,
+                 review_level, subscribed_by):
+        super(BranchSubscription, self).__init__()
+        self.person = person
+        self.branch = branch
+        self.notification_level = notification_level
+        self.max_diff_lines = max_diff_lines
+        self.review_level = review_level
+        self.subscribed_by = subscribed_by
 
     @property
     def target(self):
diff --git a/lib/lp/registry/model/sharingjob.py b/lib/lp/registry/model/sharingjob.py
index a3dd532..c069213 100644
--- a/lib/lp/registry/model/sharingjob.py
+++ b/lib/lp/registry/model/sharingjob.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Job classes related to the sharing feature are in here."""
@@ -418,7 +418,7 @@ class RemoveArtifactSubscriptionsJob(SharingJobDerived):
                         TeamParticipation.personID,
                         where=TeamParticipation.team == self.grantee)))
             branch_filters.append(
-                In(BranchSubscription.personID,
+                In(BranchSubscription.person_id,
                     Select(
                         TeamParticipation.personID,
                         where=TeamParticipation.team == self.grantee)))
@@ -447,10 +447,10 @@ class RemoveArtifactSubscriptionsJob(SharingJobDerived):
                     sub.person, self.requestor, ignore_permissions=True)
         if branch_filters:
             branch_filters.append(Not(
-                Or(*get_branch_privacy_filter(BranchSubscription.personID))))
+                Or(*get_branch_privacy_filter(BranchSubscription.person_id))))
             branch_subscriptions = IStore(BranchSubscription).using(
                 BranchSubscription,
-                Join(Branch, Branch.id == BranchSubscription.branchID)
+                Join(Branch, Branch.id == BranchSubscription.branch_id)
                 ).find(BranchSubscription, *branch_filters).config(
                     distinct=True)
             for sub in branch_subscriptions:
diff --git a/lib/lp/services/database/bulk.py b/lib/lp/services/database/bulk.py
index f73bbf8..67dff02 100644
--- a/lib/lp/services/database/bulk.py
+++ b/lib/lp/services/database/bulk.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2018 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).
 
 """Optimized bulk operations against the database."""
@@ -158,7 +158,7 @@ def load_referencing(object_type, owning_objects, reference_keys,
         At this point, all the objects should be of the same type, but that
         constraint could be lifted in future.
     :param reference_keys: A list of attributes that should be used to select
-        object_type keys. e.g. ['branchID']
+        object_type keys. e.g. ['branch_id']
     :param extra_conditions: A list of Storm clauses that will be used in the
         final query.
     :return: A list of object_type where any of reference_keys refered to the
diff --git a/lib/lp/services/database/tests/test_bulk.py b/lib/lp/services/database/tests/test_bulk.py
index 43f903f..d1efe56 100644
--- a/lib/lp/services/database/tests/test_bulk.py
+++ b/lib/lp/services/database/tests/test_bulk.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2018 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).
 
 """Test the bulk database functions."""
@@ -264,7 +264,7 @@ class TestLoaders(TestCaseWithFactory):
         self.assertNotEqual(0, len(expected))
         self.assertEqual(expected,
             set(bulk.load_referencing(BranchSubscription, owned_objects,
-                ['branchID'])))
+                ['branch_id'])))
 
 
 class TestCreate(TestCaseWithFactory):