Launchpad itself

Merge lp:~sinzui/launchpad/licenses-modified into lp:launchpad

  1. licenses-modified
  2. Merge into devel
Proposed by Curtis Hovey
Status: Merged
Approved by: j.c.sackett
Approved revision: not available
Merge reported by: Curtis Hovey
Merged at revision: not available
Proposed branch: lp:~sinzui/launchpad/licenses-modified
Merge into: lp:launchpad
To merge this branch: bzr merge lp:~sinzui/launchpad/licenses-modified
Reviewer Review Type Date Requested Status
j.c.sackett (community) Approve
Review via email: mp+107229@code.launchpad.net

Commit message

Prevent Launchpad from notifying maintainers multiple times when their project was given a complimentary commercial subscription.

Description of the change

Pre-implementation: flacoste

When an existing project uses Change details to add a proprietary
license, The maintainer is notified once because the licenses change,
and then again because the form was used to change the project. The
ObjectModifiedEvent event is created twice, which in turn calls the single
subscriber to IObjectModifiedEvent event twice.

--------------------------------------------------------------------

RULES

    * Create a specific kind of event that is only created by the
      _setLicenses() method and subscribe the notify function to it.
    * Subclass the ObjectModifiedEvent and implemented a unique interface.
      * Do not use edited_fields because few callsites populate it
        properly. Francis advises not supporting it.
    * Subscribe the notify function to the new interface.
    * Update the model (which is used by both UI and API) to notify
      the new event. It will be the only callsite for the event in
      any request.

QA

    * Visit https://qastaging.launchpad.net/projects/+new
    * Register a new project and choose the MIT license.
    * Choose Change details
    * Add the proprietary license.
    * Verify only one notification is shown.

LINT

    lib/lp/registry/configure.zcml
    lib/lp/registry/subscribers.py
    lib/lp/registry/interfaces/product.py
    lib/lp/registry/model/product.py
    lib/lp/registry/tests/test_subscribers.py

TEST

    ./bin/test -vvc lp.registry.tests.test_subscribers

IMPLEMENTATION

Created LicensesModifiedEvent which has a unique interface for license
events. The class ignores the edited_fields and object_before_modification,
but they are provided for the parent class. Updated _setLicenses() to
use the new event.
    lib/lp/registry/interfaces/product.py
    lib/lp/registry/model/product.py
    lib/lp/registry/tests/test_subscribers.py

Subscribed product_licenses_modified to the ILicensesModifiedEvent. Removed
the codes that was needed to work with the generic event.
    lib/lp/registry/configure.zcml
    lib/lp/registry/subscribers.py
    lib/lp/registry/tests/test_subscribers.py

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

Launchpad is struggling to show the diff, yet loggerhead can show it :(.

Attached is the diff of this branch against devel.

--
Curtis Hovey
http://launchpad.net/~sinzui

_diff.diff
1=== modified file 'lib/lp/registry/configure.zcml'
2--- lib/lp/registry/configure.zcml 2012-04-12 04:39:58 +0000
3+++ lib/lp/registry/configure.zcml 2012-05-23 20:51:01 +0000
4@@ -1070,7 +1070,7 @@
5 interface="lp.registry.interfaces.milestonetag.IProjectGroupMilestoneTag"/>
6 </class>
7 <subscriber
8- for="lp.registry.interfaces.product.IProduct zope.lifecycleevent.interfaces.IObjectModifiedEvent"
9+ for="lp.registry.interfaces.product.IProduct lp.registry.interfaces.product.ILicensesModifiedEvent"
10 handler="lp.registry.subscribers.product_licenses_modified"/>
11 <class
12 class="lp.registry.model.mailinglist.MailingList">
13
14=== modified file 'lib/lp/registry/interfaces/product.py'
15--- lib/lp/registry/interfaces/product.py 2012-03-06 20:43:07 +0000
16+++ lib/lp/registry/interfaces/product.py 2012-05-24 16:05:36 +0000
17@@ -1,4 +1,4 @@
18-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
19+# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
20 # GNU Affero General Public License version 3 (see the file LICENSE).
21
22 # pylint: disable-msg=E0211,E0213
23@@ -8,6 +8,7 @@
24 __metaclass__ = type
25
26 __all__ = [
27+ 'ILicensesModifiedEvent',
28 'InvalidProductName',
29 'IProduct',
30 'IProductModerateRestricted',
31@@ -341,6 +342,18 @@
32 OTHER_OPEN_SOURCE = DBItem(1010, "Other/Open Source")
33
34
35+class ILicensesModifiedEvent(Interface):
36+ """A Product's licenses were changed."""
37+
38+ def __init__(self, product, user=None):
39+ """Create an an event about a license change to a product.
40+
41+ :param product: The product that was modified.
42+ :param user: The user who modified the object. The user comes from
43+ the current interaction if the user is not provided.
44+ """
45+
46+
47 class IProductDriverRestricted(Interface):
48 """`IProduct` properties which require launchpad.Driver permission."""
49
50
51=== modified file 'lib/lp/registry/model/product.py'
52--- lib/lp/registry/model/product.py 2012-04-11 08:49:12 +0000
53+++ lib/lp/registry/model/product.py 2012-05-23 22:02:31 +0000
54@@ -6,6 +6,7 @@
55
56 __metaclass__ = type
57 __all__ = [
58+ 'LicensesModifiedEvent',
59 'Product',
60 'ProductSet',
61 'ProductWithLicenses',
62@@ -127,6 +128,7 @@
63 )
64 from lp.registry.interfaces.pillar import IPillarNameSet
65 from lp.registry.interfaces.product import (
66+ ILicensesModifiedEvent,
67 IProduct,
68 IProductSet,
69 License,
70@@ -188,6 +190,15 @@
71 from lp.translations.model.translationpolicy import TranslationPolicyMixin
72
73
74+class LicensesModifiedEvent(ObjectModifiedEvent):
75+ """See `ILicensesModifiedEvent`."""
76+ implements(ILicensesModifiedEvent)
77+
78+ def __init__(self, product, user=None):
79+ super(LicensesModifiedEvent, self).__init__(
80+ product, product, [], user)
81+
82+
83 def get_license_status(license_approved, project_reviewed, licenses):
84 """Decide the license status for an `IProduct`.
85
86@@ -788,8 +799,7 @@
87 registrant=lp_janitor, purchaser=lp_janitor,
88 sales_system_id=sales_system_id, whiteboard=whiteboard)
89 get_property_cache(self).commercial_subscription = subscription
90- # Do not use a snapshot because the past is unintersting.
91- notify(ObjectModifiedEvent(self, self, edited_fields=['licenses']))
92+ notify(LicensesModifiedEvent(self))
93
94 licenses = property(_getLicenses, _setLicenses)
95
96
97=== modified file 'lib/lp/registry/subscribers.py'
98--- lib/lp/registry/subscribers.py 2012-04-12 04:39:58 +0000
99+++ lib/lp/registry/subscribers.py 2012-05-24 16:05:34 +0000
100@@ -1,4 +1,4 @@
101-# Copyright 2009 Canonical Ltd. This software is licensed under the
102+# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
103 # GNU Affero General Public License version 3 (see the file LICENSE).
104 """Functions and classes that are subscribed to registry events."""
105
106@@ -16,7 +16,6 @@
107
108 from lp.registry.interfaces.person import (
109 IPerson,
110- IPersonViewRestricted,
111 )
112 from lp.registry.interfaces.product import License
113 from lp.services.config import config
114@@ -34,11 +33,7 @@
115
116 def product_licenses_modified(product, event):
117 """Send a notification if licenses changed and a license is special."""
118- if not event.edited_fields:
119- return
120- licenses_changed = 'licenses' in event.edited_fields
121- needs_notification = LicenseNotification.needs_notification(product)
122- if licenses_changed and needs_notification:
123+ if LicenseNotification.needs_notification(product):
124 user = IPerson(event.user)
125 notification = LicenseNotification(product, user)
126 notification.send()
127
128=== modified file 'lib/lp/registry/tests/test_subscribers.py'
129--- lib/lp/registry/tests/test_subscribers.py 2012-04-12 04:39:58 +0000
130+++ lib/lp/registry/tests/test_subscribers.py 2012-05-24 15:30:39 +0000
131@@ -7,11 +7,11 @@
132
133 from datetime import datetime
134
135-from lazr.lifecycle.event import ObjectModifiedEvent
136 import pytz
137 from zope.security.proxy import removeSecurityProxy
138
139 from lp.registry.interfaces.product import License
140+from lp.registry.model.product import LicensesModifiedEvent
141 from lp.registry.subscribers import (
142 LicenseNotification,
143 product_licenses_modified,
144@@ -26,25 +26,37 @@
145 from lp.testing.mail_helpers import pop_notifications
146
147
148+class LicensesModifiedEventTestCase(TestCaseWithFactory):
149+
150+ layer = DatabaseFunctionalLayer
151+
152+ def test_init(self):
153+ product = self.factory.makeProduct()
154+ login_person(product.owner)
155+ event = LicensesModifiedEvent(product)
156+ self.assertEqual(product.owner, event.user.person)
157+ self.assertEqual(product, event.object)
158+ self.assertEqual(product, event.object_before_modification)
159+ self.assertEqual([], event.edited_fields)
160+
161+ def test_init_with_user(self):
162+ product = self.factory.makeProduct()
163+ login_person(product.owner)
164+ event = LicensesModifiedEvent(product, user=product.owner)
165+ self.assertEqual(product.owner, event.user)
166+
167+
168 class ProductLicensesModifiedTestCase(TestCaseWithFactory):
169
170 layer = DatabaseFunctionalLayer
171
172- def make_product_event(self, licenses, edited_fields='licenses'):
173+ def make_product_event(self, licenses):
174 product = self.factory.makeProduct(licenses=licenses)
175 pop_notifications()
176 login_person(product.owner)
177- event = ObjectModifiedEvent(
178- product, product, edited_fields, user=product.owner)
179+ event = LicensesModifiedEvent(product, user=product.owner)
180 return product, event
181
182- def test_product_licenses_modified_licenses_not_edited(self):
183- product, event = self.make_product_event(
184- [License.OTHER_PROPRIETARY], edited_fields='_owner')
185- product_licenses_modified(product, event)
186- notifications = pop_notifications()
187- self.assertEqual(0, len(notifications))
188-
189 def test_product_licenses_modified_licenses_common_license(self):
190 product, event = self.make_product_event([License.MIT])
191 product_licenses_modified(product, event)
Revision history for this message
j.c.sackett (jcsackett) wrote :

Curtis--

Thanks for providing the diff, and thanks for this update. Looks good!

review: Approve