launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02374
[Merge] lp:~danilo/launchpad/ss-storm-rewrite-methods into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/ss-storm-rewrite-methods into lp:launchpad with lp:~danilo/launchpad/ss-storm-rewrite as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~danilo/launchpad/ss-storm-rewrite-methods/+merge/46843
= Convert StructuralSubscription to Storm =
This branch completes the migration of StructuralSubscription to Storm. The first step in lp:~danilo/launchpad/bugs-subscriptions was to replace column definitions with Storm versions.
This branch continues on that and moves all .select() and .selectBy() calls on StructuralSubscription to .find calls, uses store.remove() instead of object.destroySelf(), provides a constructor similar to one provided by SQLBase and then finally switches the class to inherit from Storm instead of SQLBase.
Followed https://dev.launchpad.net/StormMigrationGuide (constructor switch was not documented there, so I'll be adding it shortly).
== Implementation details ==
There are not enough unit tests for the functionality I am changing, but the full test run over all the layers of implementation (doc tests, unit tests, API tests and browser tests) did catch a few bugs (or incompatible changes that I did). While I'd love to add new tests for all the methods in StructuralSubscriptionTargetMixin, with the existing test coverage and our plan (where we'd allow more than one StructuralSubscription per target and person), most of the code I'd write tests for would go away.
It also includes slight optimizations in two places where we go over all the subscriptions to find one for a particular subscriber. It now executes a query which returns just one result instead.
To achieve this, subscriber parameter is added to getSubscriptions(), but it's not exposed in the interface because we are only using it from inside the module. That looks dirty even to me, so I'd be happy to change it if a reviewer thinks I should do it.
I am ignoring doctest lint errors since it might make diff harder to read. I'd be happy to update it after the review is done.
== Tests ==
bin/test -cvvt bugsubscription.txt -t bugnotification-sending.txt -t structural-subscriptions.txt -t test_structuralsubscription
== Demo and Q/A ==
no-qa
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/doc/bugsubscription.txt
lib/lp/registry/model/structuralsubscription.py
lib/lp/registry/model/person.py
./lib/lp/bugs/doc/bugsubscription.txt
1: narrative uses a moin header.
6: narrative uses a moin header.
25: narrative uses a moin header.
438: source has bad indentation.
442: source has bad indentation.
459: source has bad indentation.
474: source has bad indentation.
487: source has bad indentation.
494: want exceeds 78 characters.
528: narrative uses a moin header.
589: narrative uses a moin header.
644: narrative uses a moin header.
852: narrative uses a moin header.
--
https://code.launchpad.net/~danilo/launchpad/ss-storm-rewrite-methods/+merge/46843
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/ss-storm-rewrite-methods into lp:launchpad.
=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt 2011-01-18 22:16:03 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt 2011-01-19 22:18:42 +0000
@@ -147,6 +147,7 @@
>>> subscription_no_priv = linux_source.addBugSubscription(
... mr_no_privs, mr_no_privs)
+ >>> transaction.commit()
>>> print_displayname(
... sub.subscriber for sub in linux_source.bug_subscriptions)
No Privileges Person
@@ -804,7 +805,7 @@
So, if the Ubuntu team is added as a bug contact to evolution:
>>> evolution.addBugSubscription(ubuntu_team, ubuntu_team)
- <StructuralSubscription at ...>
+ <...StructuralSubscription object at ...>
The team will be implicitly subscribed to the previous bug we
created. (Remember that Sample Person is also implicitly subscribed
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2011-01-18 04:40:41 +0000
+++ lib/lp/registry/model/person.py 2011-01-19 22:18:42 +0000
@@ -2713,8 +2713,10 @@
@property
def structural_subscriptions(self):
"""See `IPerson`."""
- return StructuralSubscription.selectBy(
- subscriber=self, orderBy=['-date_created'])
+ return IStore(self).find(
+ StructuralSubscription,
+ StructuralSubscription.subscriberID==self.id).order_by(
+ Desc(StructuralSubscription.date_created))
def autoSubscribeToMailingList(self, mailinglist, requester=None):
"""See `IPerson`."""
=== modified file 'lib/lp/registry/model/structuralsubscription.py'
--- lib/lp/registry/model/structuralsubscription.py 2011-01-19 18:12:07 +0000
+++ lib/lp/registry/model/structuralsubscription.py 2011-01-19 22:18:42 +0000
@@ -15,6 +15,7 @@
Reference,
)
+from storm.base import Storm
from storm.expr import (
Alias,
And,
@@ -39,10 +40,7 @@
from canonical.database.constants import UTC_NOW
from canonical.database.enumcol import DBEnum
-from canonical.database.sqlbase import (
- quote,
- SQLBase,
- )
+from canonical.database.sqlbase import quote
from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
from canonical.launchpad.interfaces.lpstorm import IStore
from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilter
@@ -80,13 +78,11 @@
from lp.services.propertycache import cachedproperty
-class StructuralSubscription(SQLBase):
+class StructuralSubscription(Storm):
"""A subscription to a Launchpad structure."""
implements(IStructuralSubscription)
- _table = 'StructuralSubscription'
-
__storm_table__ = 'StructuralSubscription'
id = Int(primary=True)
@@ -135,6 +131,12 @@
"date_last_updated", allow_none=False, default=UTC_NOW,
tzinfo=pytz.UTC)
+ def __init__(self, subscriber, subscribed_by, **kwargs):
+ self.subscriber = subscriber
+ self.subscribed_by = subscribed_by
+ for arg, value in kwargs.iteritems():
+ setattr(self, arg, value)
+
@property
def target(self):
"""See `IStructuralSubscription`."""
@@ -427,13 +429,9 @@
'%s does not have permission to unsubscribe %s.' % (
unsubscribed_by.name, subscriber.name))
- subscription_to_remove = None
- for subscription in self.getSubscriptions(
- min_bug_notification_level=BugNotificationLevel.METADATA):
- # Only search for bug subscriptions
- if subscription.subscriber == subscriber:
- subscription_to_remove = subscription
- break
+ subscription_to_remove = self.getSubscriptions(
+ min_bug_notification_level=BugNotificationLevel.METADATA,
+ subscriber=subscriber).one()
if subscription_to_remove is None:
raise DeleteSubscriptionError(
@@ -447,39 +445,44 @@
subscription_to_remove.bug_notification_level = (
BugNotificationLevel.NOTHING)
else:
- subscription_to_remove.destroySelf()
+ store = Store.of(subscription_to_remove)
+ store.remove(subscription_to_remove)
def getSubscription(self, person):
"""See `IStructuralSubscriptionTarget`."""
- all_subscriptions = self.getSubscriptions()
- for subscription in all_subscriptions:
- if subscription.subscriber == person:
- return subscription
- return None
+ # getSubscriptions returns all subscriptions regardless of
+ # the person for person==None, so we special-case that.
+ if person is None:
+ return None
+ all_subscriptions = self.getSubscriptions(subscriber=person)
+ return all_subscriptions.one()
def getSubscriptions(self,
min_bug_notification_level=
BugNotificationLevel.NOTHING,
min_blueprint_notification_level=
- BlueprintNotificationLevel.NOTHING):
+ BlueprintNotificationLevel.NOTHING,
+ subscriber=None):
"""See `IStructuralSubscriptionTarget`."""
+ from lp.registry.model.person import Person
clauses = [
- "StructuralSubscription.subscriber = Person.id",
- "StructuralSubscription.bug_notification_level "
- ">= %s" % quote(min_bug_notification_level),
- "StructuralSubscription.blueprint_notification_level "
- ">= %s" % quote(min_blueprint_notification_level),
+ StructuralSubscription.subscriberID==Person.id,
+ (StructuralSubscription.bug_notification_level >=
+ min_bug_notification_level),
+ (StructuralSubscription.blueprint_notification_level >=
+ min_blueprint_notification_level),
]
for key, value in self._target_args.iteritems():
- if value is None:
- clauses.append(
- "StructuralSubscription.%s IS NULL" % (key,))
- else:
- clauses.append(
- "StructuralSubscription.%s = %s" % (key, quote(value)))
- query = " AND ".join(clauses)
- return StructuralSubscription.select(
- query, orderBy='Person.displayname', clauseTables=['Person'])
+ clauses.append(
+ getattr(StructuralSubscription, key)==value)
+
+ if subscriber is not None:
+ clauses.append(
+ StructuralSubscription.subscriberID==subscriber.id)
+
+ store = Store.of(self.__helper.pillar)
+ return store.find(
+ StructuralSubscription, *clauses).order_by('Person.displayname')
@property
def bug_subscriptions(self):
