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
=== modified file 'lib/canonical/launchpad/javascript/bugs/bugtask-index.js'
--- lib/canonical/launchpad/javascript/bugs/bugtask-index.js 2010-03-29 13:31:23 +0000
+++ lib/canonical/launchpad/javascript/bugs/bugtask-index.js 2010-04-12 14:56:40 +0000
@@ -131,7 +131,8 @@
131 },131 },
132 parameters: {132 parameters: {
133 person: LP.client.get_absolute_uri(133 person: LP.client.get_absolute_uri(
134 subscription.get('person').get('escaped_uri'))134 subscription.get('person').get('escaped_uri')),
135 send_notifications: true
135 }136 }
136 };137 };
137 lp_client.named_post(bug_repr.self_link, 'subscribe', config);138 lp_client.named_post(bug_repr.self_link, 'subscribe', config);
@@ -1194,7 +1195,8 @@
1194 },1195 },
11951196
1196 parameters: {1197 parameters: {
1197 person: LP.client.get_absolute_uri(subscriber.get('escaped_uri'))1198 person: LP.client.get_absolute_uri(subscriber.get('escaped_uri')),
1199 send_notifications: true
1198 }1200 }
1199 };1201 };
1200 lp_client.named_post(bug_repr.self_link, 'subscribe', config);1202 lp_client.named_post(bug_repr.self_link, 'subscribe', config);
12011203
=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py 2009-11-18 15:20:12 +0000
+++ lib/lp/bugs/browser/bugsubscription.py 2010-04-12 14:56:40 +0000
@@ -11,10 +11,8 @@
11 ]11 ]
1212
13from simplejson import dumps13from simplejson import dumps
14from zope.event import notify
1514
16from lazr.delegates import delegates15from lazr.delegates import delegates
17from lazr.lifecycle.event import ObjectCreatedEvent
1816
19from lp.bugs.browser.bug import BugViewMixin17from lp.bugs.browser.bug import BugViewMixin
20from lp.bugs.interfaces.bugsubscription import IBugSubscription18from lp.bugs.interfaces.bugsubscription import IBugSubscription
@@ -38,8 +36,7 @@
38 @action('Subscribe user', name='add')36 @action('Subscribe user', name='add')
39 def add_action(self, action, data):37 def add_action(self, action, data):
40 person = data['person']38 person = data['person']
41 subscription = self.context.bug.subscribe(person, self.user)39 self.context.bug.subscribe(person, self.user, send_notifications=True)
42 notify(ObjectCreatedEvent(subscription, user=self.user))
43 if person.isTeam():40 if person.isTeam():
44 message = '%s team has been subscribed to this bug.'41 message = '%s team has been subscribed to this bug.'
45 else:42 else:
4643
=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt 2009-12-05 18:37:28 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt 2010-04-12 14:56:40 +0000
@@ -887,11 +887,13 @@
887an ObjectCreatedEvent is issued, since subscribe() is used in places887an ObjectCreatedEvent is issued, since subscribe() is used in places
888where no notification should be sent.888where no notification should be sent.
889889
890 >>> from lazr.lifecycle.event import ObjectCreatedEvent890An ObjectCreatedEvent is created using the send_notifications parameter
891for Bug.subscribe.
892
891 >>> transaction.commit()893 >>> transaction.commit()
892 >>> stub.test_emails = []894 >>> stub.test_emails = []
893 >>> bug_subscription = bug_six.subscribe(mark, lifeless)895 >>> bug_subscription = bug_six.subscribe(
894 >>> notify(ObjectCreatedEvent(bug_subscription))896 ... mark, lifeless, send_notifications=True)
895 >>> transaction.commit()897 >>> transaction.commit()
896 >>> len(stub.test_emails)898 >>> len(stub.test_emails)
897 1899 1
@@ -911,3 +913,13 @@
911 ** Affects: firefox913 ** Affects: firefox
912 ...914 ...
913 http://bugs.launchpad.dev/bugs/...915 http://bugs.launchpad.dev/bugs/...
916
917Notifications are not created if send_notifications is False.
918
919 >>> len(stub.test_emails)
920 1
921 >>> not_notified = bug_six.subscribe(
922 ... keybuk, lifeless, send_notifications=False)
923 >>> transaction.commit()
924 >>> len(stub.test_emails)
925 1
914926
=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py 2010-04-09 07:28:26 +0000
+++ lib/lp/bugs/interfaces/bug.py 2010-04-12 14:56:40 +0000
@@ -374,11 +374,12 @@
374 person=Reference(IPerson, title=_('Person'), required=True))374 person=Reference(IPerson, title=_('Person'), required=True))
375 @call_with(subscribed_by=REQUEST_USER)375 @call_with(subscribed_by=REQUEST_USER)
376 @export_write_operation()376 @export_write_operation()
377 def subscribe(person, subscribed_by):377 def subscribe(person, subscribed_by, send_notifications=False):
378 """Subscribe `person` to the bug.378 """Subscribe `person` to the bug.
379379
380 :param person: the subscriber.380 :param person: the subscriber.
381 :param subscribed_by: the person who created the subscription.381 :param subscribed_by: the person who created the subscription.
382 :param send_notifications: send "suscribed by" notifications.
382 :return: an `IBugSubscription`.383 :return: an `IBugSubscription`.
383 """384 """
384385
385386
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2010-04-08 08:55:10 +0000
+++ lib/lp/bugs/model/bug.py 2010-04-12 14:56:40 +0000
@@ -103,7 +103,6 @@
103from lp.registry.interfaces.person import validate_public_person103from lp.registry.interfaces.person import validate_public_person
104from lp.registry.interfaces.product import IProduct104from lp.registry.interfaces.product import IProduct
105from lp.registry.interfaces.productseries import IProductSeries105from lp.registry.interfaces.productseries import IProductSeries
106from lp.registry.interfaces.projectgroup import IProjectGroup
107from lp.registry.interfaces.sourcepackage import ISourcePackage106from lp.registry.interfaces.sourcepackage import ISourcePackage
108from lp.registry.model.mentoringoffer import MentoringOffer107from lp.registry.model.mentoringoffer import MentoringOffer
109from lp.registry.model.person import Person, ValidPersonCache108from lp.registry.model.person import Person, ValidPersonCache
@@ -480,7 +479,7 @@
480 """See `IBug`."""479 """See `IBug`."""
481 return self.latest_patch_uploaded is not None480 return self.latest_patch_uploaded is not None
482481
483 def subscribe(self, person, subscribed_by):482 def subscribe(self, person, subscribed_by, send_notifications=False):
484 """See `IBug`."""483 """See `IBug`."""
485 # first look for an existing subscription484 # first look for an existing subscription
486 for sub in self.subscriptions:485 for sub in self.subscriptions:
@@ -490,6 +489,9 @@
490 sub = BugSubscription(489 sub = BugSubscription(
491 bug=self, person=person, subscribed_by=subscribed_by)490 bug=self, person=person, subscribed_by=subscribed_by)
492491
492 if send_notifications is True:
493 notify(ObjectCreatedEvent(sub, user=subscribed_by))
494
493 getUtility(ICalculateBugHeatJobSource).create(self)495 getUtility(ICalculateBugHeatJobSource).create(self)
494496
495 # Ensure that the subscription has been flushed.497 # Ensure that the subscription has been flushed.
@@ -780,7 +782,7 @@
780 # data.782 # data.
781 activity_data = change.getBugActivity()783 activity_data = change.getBugActivity()
782 if activity_data is not None:784 if activity_data is not None:
783 bug_activity = getUtility(IBugActivitySet).new(785 getUtility(IBugActivitySet).new(
784 self, when, change.person,786 self, when, change.person,
785 activity_data['whatchanged'],787 activity_data['whatchanged'],
786 activity_data.get('oldvalue'),788 activity_data.get('oldvalue'),
787789
=== modified file 'lib/lp/bugs/stories/webservice/xx-bug.txt'
--- lib/lp/bugs/stories/webservice/xx-bug.txt 2010-04-05 20:33:06 +0000
+++ lib/lp/bugs/stories/webservice/xx-bug.txt 2010-04-12 14:56:40 +0000
@@ -852,6 +852,7 @@
852852
853 >>> new_subscription = webservice.named_post(853 >>> new_subscription = webservice.named_post(
854 ... bug_one['self_link'], 'subscribe',854 ... bug_one['self_link'], 'subscribe',
855 ... send_notifications=True,
855 ... person=webservice.getAbsoluteUrl('/~cprov')).jsonBody()856 ... person=webservice.getAbsoluteUrl('/~cprov')).jsonBody()
856 >>> pprint_entry(new_subscription)857 >>> pprint_entry(new_subscription)
857 bug_link: ...858 bug_link: ...
@@ -904,6 +905,7 @@
904905
905 >>> print webservice.named_post(906 >>> print webservice.named_post(
906 ... bug_one['self_link'], 'subscribe',907 ... bug_one['self_link'], 'subscribe',
908 ... send_notifications=True,
907 ... person=webservice.getAbsoluteUrl('/~ubuntu-team'))909 ... person=webservice.getAbsoluteUrl('/~ubuntu-team'))
908 HTTP/1.1 200 Ok...910 HTTP/1.1 200 Ok...
909911
@@ -947,6 +949,7 @@
947949
948 >>> print webservice.named_post(950 >>> print webservice.named_post(
949 ... bug_six['self_link'], 'subscribe',951 ... bug_six['self_link'], 'subscribe',
952 ... send_notifications=True,
950 ... person=webservice.getAbsoluteUrl('/~salgado'))953 ... person=webservice.getAbsoluteUrl('/~salgado'))
951 HTTP/1.1 200 Ok...954 HTTP/1.1 200 Ok...
952955
@@ -977,6 +980,7 @@
977980
978 >>> print webservice.named_post(981 >>> print webservice.named_post(
979 ... bug_six['self_link'], 'subscribe',982 ... bug_six['self_link'], 'subscribe',
983 ... send_notifications=True,
980 ... person=webservice.getAbsoluteUrl('/~ubuntu-team'))984 ... person=webservice.getAbsoluteUrl('/~ubuntu-team'))
981 HTTP/1.1 200 Ok...985 HTTP/1.1 200 Ok...
982 >>> bug_six_subscriptions = webservice.get(986 >>> bug_six_subscriptions = webservice.get(