launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02515
[Merge] lp:~gary/launchpad/bug548-db-3 into lp:launchpad/db-devel
Gary Poster has proposed merging lp:~gary/launchpad/bug548-db-3 into lp:launchpad/db-devel with lp:~wgrant/launchpad/bug548-db-2-tests as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~gary/launchpad/bug548-db-3/+merge/48651
This branch fixes the remaining test failures from wgrant's branch (fixing most of the failures) of my previous branches. It also reinstates the use of a test helper for one file (bugnotification-sending.txt) that wgrant reverted because I believe it is an improvement and I don't see a reason to lose it. He probably reverted it because it was unnecessary to get the tests to pass, which is absolutely true. These changes have already been reviewed and approved (https://code.launchpad.net/~gary/launchpad/bug548-db-2-tests/+merge/48570).
This branch had to fix three sorts of test failures. First, I had hoped that we would be able to make the person-specific settings simply not implemented for teams. This proved to be untenable: at least two automated interface-based functions (verifyObject from zope.interface and lazr.lifecycle's snapshot functionality) crashed and burned by this behavior. Therefore, I opted to make the attributes readonly for teams, using the defaults from the interfaces. I had a mid-implementation call with Danilo about how to tackle that. I want to do what you see here, rather than a __getattr__/__setattr__ approach or a manual approach. I don't think that the __getattr__/__setattr__ approach is potentially better than this, but I was somehwhat tempted by the manual approach. My arguments against it were these.
- This way keeps you from having to repeat yourself (again) by explicitly listing the attribute names.
- We expect to add many more attributes to this settings bag, and setting up each one manually in a readonly version would be a drag.
- We expect to add a team-based settings bag, and repeating things there would be even more of a drag.
Danilo thought that my preferred approach was acceptable, so I proceeded.
The second test failure was in garbo, cleaning up people. This was simply addressed by teaching it about the new person settings table (that it is safe to ignore it when trying to determine whether a person is still linked).
The last test failure was that ImmutableVisibilityError was being raised because Person.visibilityConsistencyWarning needed to be taught to ignore person settings too.
My goal is to get this landed asap to try and make it into PQM. I'd prefer having to make "I'll fix that next" promises if possible, rather than significantly holding up the branch, but I will understand if you are not comfortable with that.
Thank you
Gary
--
https://code.launchpad.net/~gary/launchpad/bug548-db-3/+merge/48651
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug548-db-3 into lp:launchpad/db-devel.
=== removed directory 'lib/canonical/widgets'
=== removed file 'lib/canonical/widgets/__init__.py'
--- lib/canonical/widgets/__init__.py 2011-02-02 21:39:04 +0000
+++ lib/canonical/widgets/__init__.py 1970-01-01 00:00:00 +0000
@@ -1,12 +0,0 @@
-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-# This is a stub to keep canonical.shipit operational. this module
-# can delete when shipit is independent.
-
-from lp.app.widgets.itemswidgets import (
- CheckBoxMatrixWidget,
- LabeledMultiCheckBoxWidget,
- )
-
-
=== added file 'lib/lp/app/widgets/__init__.py'
--- lib/lp/app/widgets/__init__.py 1970-01-01 00:00:00 +0000
+++ lib/lp/app/widgets/__init__.py 2011-02-02 15:37:35 +0000
@@ -0,0 +1,2 @@
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
--- lib/lp/bugs/doc/bugnotification-sending.txt 2011-02-04 19:25:28 +0000
+++ lib/lp/bugs/doc/bugnotification-sending.txt 2011-02-04 19:25:31 +0000
@@ -30,20 +30,9 @@
... print email_notification.get_payload(decode=True)
... print "-" * 70
-We'll also define some helper functions to help us with database users.
-
- >>> from canonical.config import config
- >>> from canonical.database.sqlbase import commit
- >>> from canonical.testing.layers import LaunchpadZopelessLayer
-
- >>> def switch_db_to_launchpad():
- ... commit()
- ... LaunchpadZopelessLayer.switchDbUser('launchpad')
-
- >>> def switch_db_to_bugnotification():
- ... commit()
- ... LaunchpadZopelessLayer.switchDbUser(
- ... config.malone.bugnotification_dbuser)
+We'll also import a helper function to help us with database users.
+
+ >>> from lp.testing.dbuser import lp_dbuser
You'll note that we are printing out an X-Launchpad-Message-Rationale
header. This header is a simple string that allows people to filter
@@ -356,16 +345,13 @@
... print member.preferredemail.email
marilize@xxxxxxx
- >>> switch_db_to_launchpad()
- >>> bug_one.subscribe(shipit_admins, shipit_admins)
- <...>
-
- >>> comment = getUtility(IMessageSet).fromText(
- ... 'subject', 'a comment.', sample_person,
- ... datecreated=ten_minutes_ago)
- >>> bug_one.addCommentNotification(comment)
-
- >>> switch_db_to_bugnotification()
+ >>> with lp_dbuser():
+ ... ignored = bug_one.subscribe(shipit_admins, shipit_admins)
+ ... comment = getUtility(IMessageSet).fromText(
+ ... 'subject', 'a comment.', sample_person,
+ ... datecreated=ten_minutes_ago)
+ ... bug_one.addCommentNotification(comment)
+
>>> pending_notifications = getUtility(
... IBugNotificationSet).getNotificationsToSend()
>>> len(pending_notifications)
@@ -398,17 +384,15 @@
>>> params = CreateBugParams(
... msg=description, owner=sample_person, title='new bug')
- >>> switch_db_to_launchpad()
- >>> new_bug = ubuntu.createBug(params)
- >>> switch_db_to_bugnotification()
- >>> flush_notifications()
+ >>> with lp_dbuser():
+ ... new_bug = ubuntu.createBug(params)
If a bug is a duplicate of another bug, a marker gets inserted at the
top of the email:
- >>> switch_db_to_launchpad()
- >>> new_bug.markAsDuplicate(bug_one)
- >>> switch_db_to_bugnotification()
+ >>> flush_notifications()
+ >>> with lp_dbuser():
+ ... new_bug.markAsDuplicate(bug_one)
>>> comment = getUtility(IMessageSet).fromText(
... 'subject', 'a comment.', sample_person,
... datecreated=ten_minutes_ago)
@@ -474,12 +458,11 @@
... 'Zero-day on Frobulator', 'Woah.', sample_person,
... datecreated=ten_minutes_ago)
- >>> switch_db_to_launchpad()
- >>> sec_vuln_bug = ubuntu.createBug(CreateBugParams(
+ >>> with lp_dbuser():
+ ... sec_vuln_bug = ubuntu.createBug(CreateBugParams(
... msg=sec_vuln_description, owner=sample_person,
... title='Zero-day on Frobulator',
... security_related=True, private=True))
- >>> switch_db_to_bugnotification()
>>> sec_vuln_bug.security_related
True
@@ -730,10 +713,8 @@
The tags will be space-separated to allow the list to be wrapped if it
gets over-long.
- >>> switch_db_to_launchpad()
- >>> bug_three.tags = [u'layout-test', u'another-tag', u'yet-another']
-
- >>> switch_db_to_bugnotification()
+ >>> with lp_dbuser():
+ ... bug_three.tags = [u'layout-test', u'another-tag', u'yet-another']
>>> bug_three = getUtility(IBugSet).get(3)
>>> for message in trigger_and_get_email_messages(bug_three):
@@ -743,15 +724,13 @@
If we remove the tags from the bug, the X-Launchpad-Bug-Tags header
won't be included.
- >>> switch_db_to_launchpad()
- >>> bug_three.tags = []
- >>> switch_db_to_bugnotification()
+ >>> with lp_dbuser():
+ ... bug_three.tags = []
>>> bug_three = getUtility(IBugSet).get(3)
>>> for message in trigger_and_get_email_messages(bug_three):
... message.get_all('X-Launchpad-Bug-Tags')
- >>> switch_db_to_launchpad()
>>> #bug_three.unsubscribe(sample_person, sample_person)
@@ -771,7 +750,8 @@
Predictably, private bugs are sent with a slightly different header:
- >>> bug_three.setPrivate(True, sample_person)
+ >>> with lp_dbuser():
+ ... bug_three.setPrivate(True, sample_person)
True
>>> bug_three.private
True
@@ -799,7 +779,8 @@
The presence of the security flag on a bug is, surprise, denoted by a
simple "yes":
- >>> bug_three.setSecurityRelated(True)
+ >>> with lp_dbuser():
+ ... bug_three.setSecurityRelated(True)
True
>>> bug_three.security_related
True
@@ -824,9 +805,9 @@
>>> foo_bar = getUtility(IPersonSet).getByEmail('foo.bar@xxxxxxxxxxxxx')
>>> from lp.bugs.interfaces.bugmessage import IBugMessageSet
- >>> getUtility(IBugMessageSet).createMessage(
- ... 'Hungry', bug_three, foo_bar, "Make me a sandwich.")
- <BugMessage ...>
+ >>> with lp_dbuser():
+ ... ignored = getUtility(IBugMessageSet).createMessage(
+ ... 'Hungry', bug_three, foo_bar, "Make me a sandwich.")
>>> for message in trigger_and_get_email_messages(bug_three):
... print message.get('X-Launchpad-Bug-Commenters')
@@ -835,9 +816,9 @@
It only lists each user once, no matter how many comments they've
made.
- >>> getUtility(IBugMessageSet).createMessage(
- ... 'Hungry', bug_three, foo_bar, "Make me a sandwich.")
- <BugMessage ...>
+ >>> with lp_dbuser():
+ ... ignored = getUtility(IBugMessageSet).createMessage(
+ ... 'Hungry', bug_three, foo_bar, "Make me a sandwich.")
>>> for message in trigger_and_get_email_messages(bug_three):
... print message.get('X-Launchpad-Bug-Commenters')
@@ -868,63 +849,58 @@
Concise Person does not. We'll also create teams and give them members
with different verbose_bugnotifications settings.
- >>> switch_db_to_launchpad()
- >>> bug = factory.makeBug(
- ... product=factory.makeProduct(title='Foo'),
- ... title='In the beginning, the universe was created. This '
- ... 'has made a lot of people very angry and has been '
- ... 'widely regarded as a bad move',
- ... description="This is a long description of the bug, which "
- ... "will be automatically wrapped by the BugNotification "
- ... "machinery. Ain't technology great?")
-
- >>> verbose_person = factory.makePerson(
- ... displayname='Verbose Person', email='verbose@xxxxxxxxxxx')
- >>> verbose_person.verbose_bugnotifications = True
- >>> bug.subscribe(verbose_person, verbose_person)
- <lp.bugs.model.bugsubscription.BugSubscription ...>
-
- >>> concise_person = factory.makePerson(
- ... displayname='Concise Person', email='concise@xxxxxxxxxxx')
- >>> concise_person.verbose_bugnotifications = False
- >>> bug.subscribe(concise_person, concise_person)
- <lp.bugs.model.bugsubscription.BugSubscription ...>
+ >>> with lp_dbuser():
+ ... bug = factory.makeBug(
+ ... product=factory.makeProduct(title='Foo'),
+ ... title='In the beginning, the universe was created. This '
+ ... 'has made a lot of people very angry and has been '
+ ... 'widely regarded as a bad move',
+ ... description="This is a long description of the bug, which "
+ ... "will be automatically wrapped by the BugNotification "
+ ... "machinery. Ain't technology great?")
+ ... verbose_person = factory.makePerson(
+ ... displayname='Verbose Person', email='verbose@xxxxxxxxxxx')
+ ... verbose_person.verbose_bugnotifications = True
+ ... ignored = bug.subscribe(verbose_person, verbose_person)
+ ... concise_person = factory.makePerson(
+ ... displayname='Concise Person', email='concise@xxxxxxxxxxx')
+ ... concise_person.verbose_bugnotifications = False
+ ... ignored = bug.subscribe(concise_person, concise_person)
Concise Team doesn't want verbose notifications, while Concise Team
Person, a member, does.
- >>> concise_team = factory.makeTeam(
- ... name='conciseteam', displayname='Concise Team')
- >>> concise_team.verbose_bugnotifications = False
- >>> concise_team_person = factory.makePerson(
- ... displayname='Concise Team Person',
- ... email='conciseteam@xxxxxxxxxxx')
- >>> concise_team_person.verbose_bugnotifications = True
- >>> ignored = concise_team.addMember(
- ... concise_team_person, concise_team_person)
- >>> bug.subscribe(concise_team, concise_team_person)
- <lp.bugs.model.bugsubscription.BugSubscription ...>
+ >>> with lp_dbuser():
+ ... concise_team = factory.makeTeam(
+ ... name='conciseteam', displayname='Concise Team')
+ ... concise_team.verbose_bugnotifications = False
+ ... concise_team_person = factory.makePerson(
+ ... displayname='Concise Team Person',
+ ... email='conciseteam@xxxxxxxxxxx')
+ ... concise_team_person.verbose_bugnotifications = True
+ ... ignored = concise_team.addMember(
+ ... concise_team_person, concise_team_person)
+ ... ignored = bug.subscribe(concise_team, concise_team_person)
Verbose Team wants verbose notifications, while Verbose Team Person, a
member, does not.
- >>> verbose_team = factory.makeTeam(
- ... name='verboseteam', displayname='Verbose Team')
- >>> verbose_team.verbose_bugnotifications = True
- >>> verbose_team_person = factory.makePerson(
- ... displayname='Verbose Team Person',
- ... email='verboseteam@xxxxxxxxxxx')
- >>> verbose_team_person.verbose_bugnotifications = False
- >>> ignored = verbose_team.addMember(
- ... verbose_team_person, verbose_team_person)
- >>> bug.subscribe(verbose_team, verbose_team_person)
- <lp.bugs.model.bugsubscription.BugSubscription ...>
+ >>> with lp_dbuser():
+ ... verbose_team = factory.makeTeam(
+ ... name='verboseteam', displayname='Verbose Team')
+ ... verbose_team.verbose_bugnotifications = True
+ ... verbose_team_person = factory.makePerson(
+ ... displayname='Verbose Team Person',
+ ... email='verboseteam@xxxxxxxxxxx')
+ ... verbose_team_person.verbose_bugnotifications = False
+ ... ignored = verbose_team.addMember(
+ ... verbose_team_person, verbose_team_person)
+ ... ignored = bug.subscribe(verbose_team, verbose_team_person)
We'll expire all existing notifications since we're not interested in
them:
- >>> switch_db_to_bugnotification()
>>> notifications = getUtility(
... IBugNotificationSet).getNotificationsToSend()
>>> len(notifications)
@@ -1103,19 +1079,22 @@
People (not teams) will have the choice to receive notifications from actions
they generated. For now, everyone receives these notifications whether they
-want them or not. However, we can show that the attribute exists for now.
+want them or not.
- >>> flush_notifications()
- >>> switch_db_to_launchpad()
>>> verbose_person.selfgenerated_bugnotifications
True
+ >>> with lp_dbuser():
+ ... verbose_person.selfgenerated_bugnotifications = False
-Teams do not implement this attribute.
+Teams provide this attribute read-only.
>>> verbose_team.selfgenerated_bugnotifications
+ True
+ >>> with lp_dbuser():
+ ... verbose_team.selfgenerated_bugnotifications = False
Traceback (most recent call last):
...
- NotImplementedError: Teams do not support this attribute.
+ NotImplementedError: Teams do not support changing this attribute.
Notification Recipients
-----------------------
@@ -1125,16 +1104,14 @@
notification level of the subscription.
>>> flush_notifications()
- >>> switch_db_to_launchpad()
>>> from lp.bugs.enum import BugNotificationLevel
>>> from lp.registry.interfaces.product import IProductSet
>>> firefox = getUtility(IProductSet).getByName('firefox')
>>> mr_no_privs = getUtility(IPersonSet).getByName('no-priv')
- >>> subscription_no_priv = firefox.addBugSubscription(
- ... mr_no_privs, mr_no_privs)
-
- >>> switch_db_to_bugnotification()
+ >>> with lp_dbuser():
+ ... subscription_no_priv = firefox.addBugSubscription(
+ ... mr_no_privs, mr_no_privs)
The notifications generated by addCommentNotification() are sent only to
structural subscribers with no filters, or with the notification level
@@ -1200,10 +1177,9 @@
>>> flush_notifications()
- >>> switch_db_to_launchpad()
- >>> filter = subscription_no_priv.newBugFilter()
- >>> filter.bug_notification_level = BugNotificationLevel.COMMENTS
- >>> switch_db_to_bugnotification()
+ >>> with lp_dbuser():
+ ... filter = subscription_no_priv.newBugFilter()
+ ... filter.bug_notification_level = BugNotificationLevel.COMMENTS
>>> comment = getUtility(IMessageSet).fromText(
... 'subject', 'another comment.', sample_person,
@@ -1261,9 +1237,8 @@
no comment notifications.
>>> flush_notifications()
- >>> switch_db_to_launchpad()
- >>> filter.bug_notification_level = BugNotificationLevel.METADATA
- >>> switch_db_to_bugnotification()
+ >>> with lp_dbuser():
+ ... filter.bug_notification_level = BugNotificationLevel.METADATA
>>> comment = getUtility(IMessageSet).fromText(
... 'subject', 'no comment for no-priv.', sample_person,
@@ -1373,9 +1348,8 @@
no notifications created by addChangeNotification().
>>> flush_notifications()
- >>> switch_db_to_launchpad()
- >>> filter.bug_notification_level = BugNotificationLevel.LIFECYCLE
- >>> switch_db_to_bugnotification()
+ >>> with lp_dbuser():
+ ... filter.bug_notification_level = BugNotificationLevel.LIFECYCLE
>>> bug_one.addChangeNotification('** Summary changed to: something.',
... sample_person, when=ten_minutes_ago)
@@ -1433,10 +1407,9 @@
after all.
>>> flush_notifications()
- >>> switch_db_to_launchpad()
- >>> filter2 = subscription_no_priv.newBugFilter()
- >>> filter2.bug_notification_level = BugNotificationLevel.METADATA
- >>> switch_db_to_bugnotification()
+ >>> with lp_dbuser():
+ ... filter2 = subscription_no_priv.newBugFilter()
+ ... filter2.bug_notification_level = BugNotificationLevel.METADATA
>>> bug_one.addChangeNotification('** Summary changed to: whatever.',
... sample_person, when=ten_minutes_ago)
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2011-02-04 19:25:28 +0000
+++ lib/lp/registry/model/person.py 2011-02-04 19:25:31 +0000
@@ -84,6 +84,7 @@
from zope.event import notify
from zope.interface import (
alsoProvides,
+ classImplements,
implementer,
implements,
)
@@ -361,6 +362,7 @@
class PersonSettings(Storm):
+ "The relatively rarely used settings for person (not a team)."
implements(IPersonSettings)
@@ -371,6 +373,24 @@
selfgenerated_bugnotifications = BoolCol(notNull=True, default=True)
+def readonly_settings(message, interface):
+ """Make an object that disallows writes to values on the interface.
+
+ When you write, the message is raised in a NotImplementedError.
+ """
+ def unwritable(self, value):
+ raise NotImplementedError(message)
+ data = {}
+ for name in interface.names(True):
+ closure_f = lambda result: lambda self: result
+ data[name] = property(closure_f(interface[name].default), unwritable)
+ cls = type('Unwritable' + interface.__name__, (), data)
+ classImplements(cls, interface)
+ return cls()
+
+_readonly_person_settings = readonly_settings(
+ 'Teams do not support changing this attribute.', IPersonSettings)
+
class Person(
SQLBase, HasBugsBase, HasSpecificationsMixin, HasTranslationImportsMixin,
@@ -391,13 +411,11 @@
@cachedproperty
def _person_settings(self):
if self.is_team:
- # Hopefully no-one ever encounters this. If someone does,
- # that means that the code is trying to look at
- # person-specific attributes on a team, and we should warn
- # about that explicitly to give a hint about what is wrong
- # (rather than merely returning None).
- raise NotImplementedError(
- 'Teams do not support this attribute.')
+ # Teams need to provide these attributes for reading in order for
+ # things like snapshots to work, but they are not actually
+ # pertinent to teams, so they are not actually implemented for
+ # writes.
+ return _readonly_person_settings
else:
# This is a person.
return IStore(PersonSettings).find(
@@ -2112,6 +2130,7 @@
('logintoken', 'requester'),
('personlanguage', 'person'),
('personlocation', 'person'),
+ ('personsettings', 'person'),
('persontransferjob', 'minor_person'),
('persontransferjob', 'major_person'),
('signedcodeofconduct', 'owner'),
=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py 2011-01-31 09:24:13 +0000
+++ lib/lp/scripts/garbo.py 2011-02-04 19:25:31 +0000
@@ -431,10 +431,13 @@
for (from_table, from_column, to_table, to_column, uflag, dflag) in (
postgresql.listReferences(cursor(), 'person', 'id')):
# Skip things that don't link to Person.id or that link to it from
- # TeamParticipation or EmailAddress, as all Person entries will be
- # linked to from these tables.
+ # TeamParticipation, EmailAddress, as all Person entries will be
+ # linked to from these tables. Similarly, PersonSettings can
+ # simply be deleted if it exists, because it has a 1 (or 0) to 1
+ # relationship with Person.
if (to_table != 'person' or to_column != 'id'
- or from_table in ('teamparticipation', 'emailaddress')):
+ or from_table in ('teamparticipation', 'emailaddress',
+ 'personsettings')):
continue
self.log.debug(
"Populating LinkedPeople from %s.%s"
@@ -510,6 +513,7 @@
UPDATE EmailAddress SET person=NULL
WHERE person IN (%s)
""" % people_ids)
+ # This cascade deletes any PersonSettings records.
self.store.execute("""
DELETE FROM Person
WHERE id IN (%s)
Follow ups
