Launchpad itself

Merge lp:~deryck/launchpad/not-notified-someone-else-subscribed-494257 into lp:launchpad

  1. not-notified-someone-else-subscribed-494257
  2. Merge into devel
Proposed by Deryck Hodge
Status: Merged
Approved by: Deryck Hodge
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~deryck/launchpad/not-notified-someone-else-subscribed-494257
Merge into: lp:launchpad
Diff against target: 177 lines (+31/-13)
6 files modified
lib/canonical/launchpad/javascript/bugs/bugtask-index.js (+4/-2)
lib/lp/bugs/browser/bugsubscription.py (+1/-4)
lib/lp/bugs/doc/bugsubscription.txt (+15/-3)
lib/lp/bugs/interfaces/bug.py (+2/-1)
lib/lp/bugs/model/bug.py (+5/-3)
lib/lp/bugs/stories/webservice/xx-bug.txt (+4/-0)
To merge this branch: bzr merge lp:~deryck/launchpad/not-notified-someone-else-subscribed-494257
Reviewer Review Type Date Requested Status
Abel Deuring (community) Approve
Review via email: mp+23044@code.launchpad.net

Commit message

Notify users when subscribed by someone else, even when subscribed via the JavaScript subscription widget.

Description of the change

This branch fixes bug 494257. Users are not being notified when subscribed by someone else. This is because a notify call is required, which should be passed an ObjectCreatedEvent. This has been happening in browser code before now. The JavaScript inline subscription code calls Bug.subscribe via the API, which hits the model code not the browser code. So no notify calls were made.

I discussed various approached to fixing this with BjornT. I settled on the approach taken here, which is to add a parameter to the model method. There are certain instances were a subscription is added but we don't want to create an additional notification. This happens when bugs are created, for example, when subscriptions are created for structural subscribers.

The doc test is updated to show how to use this additional parameter to create notifications.

./bin/test -cvvt doc/bugsubscription.txt

The webservice doc test is updated to include the new parameter as an example. The functionality is tested in the previous test, though.

./bin/test -cvvt xx-bug.txt

Finally, the JavaScript is updated to send notifications now.

I want to get this portion of the change reviewed, but I won't land it until I fix Bug #516781, which notes that the inline subscriber test is currently disabled. This shouldn't affect that code at all, but I want to take the chance to re-enable the test and be sure as part of work around this fix.

There are also some minor lint cleanups here, too.

To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote :

(12:49:34) adeuring: deryck: overall, you branch looks good. I just wondered if it makes sense to explicitly test that bug.subscribe(..., send_notifications=False) does indeed not send notifications.
(12:50:01) deryck: adeuring, ah, good point.
(12:50:22) adeuring: deryck: OK, so r=me
(12:50:38) deryck: adeuring, thanks! I'll add the test here in a moment.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/javascript/bugs/bugtask-index.js'
2--- lib/canonical/launchpad/javascript/bugs/bugtask-index.js 2010-03-29 13:31:23 +0000
3+++ lib/canonical/launchpad/javascript/bugs/bugtask-index.js 2010-04-12 14:56:40 +0000
4@@ -131,7 +131,8 @@
5 },
6 parameters: {
7 person: LP.client.get_absolute_uri(
8- subscription.get('person').get('escaped_uri'))
9+ subscription.get('person').get('escaped_uri')),
10+ send_notifications: true
11 }
12 };
13 lp_client.named_post(bug_repr.self_link, 'subscribe', config);
14@@ -1194,7 +1195,8 @@
15 },
16
17 parameters: {
18- person: LP.client.get_absolute_uri(subscriber.get('escaped_uri'))
19+ person: LP.client.get_absolute_uri(subscriber.get('escaped_uri')),
20+ send_notifications: true
21 }
22 };
23 lp_client.named_post(bug_repr.self_link, 'subscribe', config);
24
25=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
26--- lib/lp/bugs/browser/bugsubscription.py 2009-11-18 15:20:12 +0000
27+++ lib/lp/bugs/browser/bugsubscription.py 2010-04-12 14:56:40 +0000
28@@ -11,10 +11,8 @@
29 ]
30
31 from simplejson import dumps
32-from zope.event import notify
33
34 from lazr.delegates import delegates
35-from lazr.lifecycle.event import ObjectCreatedEvent
36
37 from lp.bugs.browser.bug import BugViewMixin
38 from lp.bugs.interfaces.bugsubscription import IBugSubscription
39@@ -38,8 +36,7 @@
40 @action('Subscribe user', name='add')
41 def add_action(self, action, data):
42 person = data['person']
43- subscription = self.context.bug.subscribe(person, self.user)
44- notify(ObjectCreatedEvent(subscription, user=self.user))
45+ self.context.bug.subscribe(person, self.user, send_notifications=True)
46 if person.isTeam():
47 message = '%s team has been subscribed to this bug.'
48 else:
49
50=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
51--- lib/lp/bugs/doc/bugsubscription.txt 2009-12-05 18:37:28 +0000
52+++ lib/lp/bugs/doc/bugsubscription.txt 2010-04-12 14:56:40 +0000
53@@ -887,11 +887,13 @@
54 an ObjectCreatedEvent is issued, since subscribe() is used in places
55 where no notification should be sent.
56
57- >>> from lazr.lifecycle.event import ObjectCreatedEvent
58+An ObjectCreatedEvent is created using the send_notifications parameter
59+for Bug.subscribe.
60+
61 >>> transaction.commit()
62 >>> stub.test_emails = []
63- >>> bug_subscription = bug_six.subscribe(mark, lifeless)
64- >>> notify(ObjectCreatedEvent(bug_subscription))
65+ >>> bug_subscription = bug_six.subscribe(
66+ ... mark, lifeless, send_notifications=True)
67 >>> transaction.commit()
68 >>> len(stub.test_emails)
69 1
70@@ -911,3 +913,13 @@
71 ** Affects: firefox
72 ...
73 http://bugs.launchpad.dev/bugs/...
74+
75+Notifications are not created if send_notifications is False.
76+
77+ >>> len(stub.test_emails)
78+ 1
79+ >>> not_notified = bug_six.subscribe(
80+ ... keybuk, lifeless, send_notifications=False)
81+ >>> transaction.commit()
82+ >>> len(stub.test_emails)
83+ 1
84
85=== modified file 'lib/lp/bugs/interfaces/bug.py'
86--- lib/lp/bugs/interfaces/bug.py 2010-04-09 07:28:26 +0000
87+++ lib/lp/bugs/interfaces/bug.py 2010-04-12 14:56:40 +0000
88@@ -374,11 +374,12 @@
89 person=Reference(IPerson, title=_('Person'), required=True))
90 @call_with(subscribed_by=REQUEST_USER)
91 @export_write_operation()
92- def subscribe(person, subscribed_by):
93+ def subscribe(person, subscribed_by, send_notifications=False):
94 """Subscribe `person` to the bug.
95
96 :param person: the subscriber.
97 :param subscribed_by: the person who created the subscription.
98+ :param send_notifications: send "suscribed by" notifications.
99 :return: an `IBugSubscription`.
100 """
101
102
103=== modified file 'lib/lp/bugs/model/bug.py'
104--- lib/lp/bugs/model/bug.py 2010-04-08 08:55:10 +0000
105+++ lib/lp/bugs/model/bug.py 2010-04-12 14:56:40 +0000
106@@ -103,7 +103,6 @@
107 from lp.registry.interfaces.person import validate_public_person
108 from lp.registry.interfaces.product import IProduct
109 from lp.registry.interfaces.productseries import IProductSeries
110-from lp.registry.interfaces.projectgroup import IProjectGroup
111 from lp.registry.interfaces.sourcepackage import ISourcePackage
112 from lp.registry.model.mentoringoffer import MentoringOffer
113 from lp.registry.model.person import Person, ValidPersonCache
114@@ -480,7 +479,7 @@
115 """See `IBug`."""
116 return self.latest_patch_uploaded is not None
117
118- def subscribe(self, person, subscribed_by):
119+ def subscribe(self, person, subscribed_by, send_notifications=False):
120 """See `IBug`."""
121 # first look for an existing subscription
122 for sub in self.subscriptions:
123@@ -490,6 +489,9 @@
124 sub = BugSubscription(
125 bug=self, person=person, subscribed_by=subscribed_by)
126
127+ if send_notifications is True:
128+ notify(ObjectCreatedEvent(sub, user=subscribed_by))
129+
130 getUtility(ICalculateBugHeatJobSource).create(self)
131
132 # Ensure that the subscription has been flushed.
133@@ -780,7 +782,7 @@
134 # data.
135 activity_data = change.getBugActivity()
136 if activity_data is not None:
137- bug_activity = getUtility(IBugActivitySet).new(
138+ getUtility(IBugActivitySet).new(
139 self, when, change.person,
140 activity_data['whatchanged'],
141 activity_data.get('oldvalue'),
142
143=== modified file 'lib/lp/bugs/stories/webservice/xx-bug.txt'
144--- lib/lp/bugs/stories/webservice/xx-bug.txt 2010-04-05 20:33:06 +0000
145+++ lib/lp/bugs/stories/webservice/xx-bug.txt 2010-04-12 14:56:40 +0000
146@@ -852,6 +852,7 @@
147
148 >>> new_subscription = webservice.named_post(
149 ... bug_one['self_link'], 'subscribe',
150+ ... send_notifications=True,
151 ... person=webservice.getAbsoluteUrl('/~cprov')).jsonBody()
152 >>> pprint_entry(new_subscription)
153 bug_link: ...
154@@ -904,6 +905,7 @@
155
156 >>> print webservice.named_post(
157 ... bug_one['self_link'], 'subscribe',
158+ ... send_notifications=True,
159 ... person=webservice.getAbsoluteUrl('/~ubuntu-team'))
160 HTTP/1.1 200 Ok...
161
162@@ -947,6 +949,7 @@
163
164 >>> print webservice.named_post(
165 ... bug_six['self_link'], 'subscribe',
166+ ... send_notifications=True,
167 ... person=webservice.getAbsoluteUrl('/~salgado'))
168 HTTP/1.1 200 Ok...
169
170@@ -977,6 +980,7 @@
171
172 >>> print webservice.named_post(
173 ... bug_six['self_link'], 'subscribe',
174+ ... send_notifications=True,
175 ... person=webservice.getAbsoluteUrl('/~ubuntu-team'))
176 HTTP/1.1 200 Ok...
177 >>> bug_six_subscriptions = webservice.get(