Merge lp:~gmb/launchpad/make-+subscribe-play-nice-bug-735397 into lp:launchpad
- make-+subscribe-play-nice-bug-735397
- Merge into devel
Status: | Merged |
---|---|
Merged at revision: | 12619 |
Proposed branch: | lp:~gmb/launchpad/make-+subscribe-play-nice-bug-735397 |
Merge into: | lp:launchpad |
Diff against target: |
708 lines (+383/-58) 9 files modified
lib/lp/bugs/browser/bugsubscription.py (+33/-10) lib/lp/bugs/browser/tests/test_bugsubscription_views.py (+99/-0) lib/lp/bugs/configure.zcml (+2/-0) lib/lp/bugs/interfaces/bug.py (+17/-0) lib/lp/bugs/javascript/bug_subscription.js (+59/-0) lib/lp/bugs/javascript/bugtask_index_portlets.js (+103/-48) lib/lp/bugs/model/bug.py (+21/-0) lib/lp/bugs/templates/bug-subscription.pt (+19/-0) lib/lp/bugs/tests/test_bug.py (+30/-0) |
To merge this branch: | bzr merge lp:~gmb/launchpad/make-+subscribe-play-nice-bug-735397 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
j.c.sackett (community) | Approve | ||
Review via email: mp+53839@code.launchpad.net |
Commit message
[r=jcsackett][bug=735397] The BugTask:+subscribe page has been updated so that it plays nicely with muted subscriptions. The controls on the BugTask:+index page should now behave correctly, too.
Description of the change
This branch makes the +subscribe view for a bug, and its associated
overlay on the bug page, play nice with muted subscriptions.
== lib/lp/
- I've updated BugSubscription
muted subscriptions properly and presents options related to unmuting
to the user.
== lib/lp/
- I've added tests to cover the changes I've made to
BugSubscript
== lib/lp/
- I've updated the IBug declaration to include the mute() and unmute()
methods.
== lib/lp/
- I've added two methods, mute() and unmute() to IBug. These are needed
for two reasons:
1. I want to not have to fool around with subscriptions in the JS
unless I need to (see below).
2. We may want to replace the mute/unmute via subscriptions
functionality with something else (like a BugMute table). Having
these two methods means we can do it reasonably simply without
mucking about with View and JS too much.
== lib/lp/
- I've added this file with some JavaScript to add UI polish to the
+subscribe view. This should help make the view a little less
confusing when people hit it whilst they have a mute on a bug.
== lib/lp/
- I've made quite a lot of changes here, including several
refactorings. The main points are:
- Clicking Mute will now remove you from the subscribers list if
you're there.
- If you're muted and you click Subscribe, you'll see the updated
version of the +subscribe form, with all the JS goodness I've added
in bug_subscriptio
- Using the Subscribe link to unmute and resubscribe really just
updates your subscription, but it also makes all the UI elements on
the BugTask:+index page play nice.
== lib/lp/
- I've added implementations of IBug.mute() and unmute().
== lib/lp/
- I've updated the page to include the JS calls for
bug_
== lib/lp/
- I've added tests for Bug.mute() and unmute().
Graham Binns (gmb) wrote : | # |
> * What's the exact difference between a muted subscription and not being subscribed? Is it a difference between getting all comments and just say, getting notified when something is Fix released?
If you have a muted subscription you will get no mail about the bug
*at all*. None. Nada. Zilch. Zip. Nothing.
> * The answer to the first may answer this, but: why is unmuting the same as unsubscribing-
This is going to change in a subsequent branch. Instead of just
removing the (muted) subscription we'll display the new improved
+subscribe form when you click unmute, so your options will be:
(*) Unmute bug mail [which removes the mute]
( ) Unmute bug mail and subscribe me to this bug
> * In bugtask_
In this case there's no difference between the two at all. Passing the
callback is the equivalent of having a local-scope function or a
Lambda in Python. The only time callbacks matter is when you're doing
things asynchronously, and in this case we weren't; the callback just
meant you had to go and look for the definition of the callback
function, so it seemed simpler to make it part of a named function and
call that directly.
Hope that helps.
j.c.sackett (jcsackett) wrote : | # |
Thanks for those answers, Graham.
Preview Diff
1 | === modified file 'lib/lp/bugs/browser/bugsubscription.py' |
2 | --- lib/lp/bugs/browser/bugsubscription.py 2011-03-10 12:42:35 +0000 |
3 | +++ lib/lp/bugs/browser/bugsubscription.py 2011-03-17 15:02:39 +0000 |
4 | @@ -222,9 +222,20 @@ |
5 | |
6 | @cachedproperty |
7 | def _update_subscription_term(self): |
8 | + if self.user_is_muted: |
9 | + label = "Unmute bug mail from this bug and subscribe me to it" |
10 | + else: |
11 | + label = "Update my current subscription" |
12 | return SimpleTerm( |
13 | - 'update-subscription', 'update-subscription', |
14 | - 'Update my current subscription') |
15 | + 'update-subscription', 'update-subscription', label) |
16 | + |
17 | + @cachedproperty |
18 | + def _unsubscribe_current_user_term(self): |
19 | + if self._use_advanced_features and self.user_is_muted: |
20 | + label = "Unmute bug mail from this bug" |
21 | + else: |
22 | + label = 'Unsubscribe me from this bug' |
23 | + return SimpleTerm(self.user, self.user.name, label) |
24 | |
25 | @cachedproperty |
26 | def _subscription_field(self): |
27 | @@ -233,12 +244,12 @@ |
28 | for person in self._subscribers_for_current_user: |
29 | if person.id == self.user.id: |
30 | if (self._use_advanced_features and |
31 | - self.user_is_subscribed_directly): |
32 | - subscription_terms.append(self._update_subscription_term) |
33 | + (self.user_is_subscribed_directly or |
34 | + self.user_is_muted)): |
35 | + subscription_terms.append( |
36 | + self._update_subscription_term) |
37 | subscription_terms.insert( |
38 | - 0, SimpleTerm( |
39 | - person, person.name, |
40 | - 'Unsubscribe me from this bug')) |
41 | + 0, self._unsubscribe_current_user_term) |
42 | self_subscribed = True |
43 | else: |
44 | subscription_terms.append( |
45 | @@ -279,6 +290,8 @@ |
46 | """See `LaunchpadFormView`.""" |
47 | super(BugSubscriptionSubscribeSelfView, self).setUpWidgets() |
48 | if self._use_advanced_features: |
49 | + self.widgets['bug_notification_level'].widget_class = ( |
50 | + 'bug-notification-level-field') |
51 | if self._subscriber_count_for_current_user == 0: |
52 | # We hide the subscription widget if the user isn't |
53 | # subscribed, since we know who the subscriber is and we |
54 | @@ -298,14 +311,22 @@ |
55 | self.widgets['bug_notification_level'].visible = False |
56 | |
57 | @cachedproperty |
58 | + def user_is_muted(self): |
59 | + return self.context.bug.isMuted(self.user) |
60 | + |
61 | + @cachedproperty |
62 | def user_is_subscribed_directly(self): |
63 | """Is the user subscribed directly to this bug?""" |
64 | - return self.context.bug.isSubscribed(self.user) |
65 | + return ( |
66 | + self.context.bug.isSubscribed(self.user) and not |
67 | + self.user_is_muted) |
68 | |
69 | @cachedproperty |
70 | def user_is_subscribed_to_dupes(self): |
71 | """Is the user subscribed to dupes of this bug?""" |
72 | - return self.context.bug.isSubscribedToDupes(self.user) |
73 | + return ( |
74 | + self.context.bug.isSubscribedToDupes(self.user) and not |
75 | + self.user_is_muted) |
76 | |
77 | @property |
78 | def user_is_subscribed(self): |
79 | @@ -347,8 +368,10 @@ |
80 | bug_notification_level = None |
81 | |
82 | if (subscription_person == self._update_subscription_term.value and |
83 | - self.user_is_subscribed): |
84 | + (self.user_is_subscribed or self.user_is_muted)): |
85 | self._handleUpdateSubscription(level=bug_notification_level) |
86 | + elif self.user_is_muted and subscription_person == self.user: |
87 | + self._handleUnsubscribeCurrentUser() |
88 | elif (not self.user_is_subscribed and |
89 | (subscription_person == self.user)): |
90 | self._handleSubscribe(bug_notification_level) |
91 | |
92 | === modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py' |
93 | --- lib/lp/bugs/browser/tests/test_bugsubscription_views.py 2011-03-10 12:42:35 +0000 |
94 | +++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py 2011-03-17 15:02:39 +0000 |
95 | @@ -29,6 +29,9 @@ |
96 | |
97 | def setUp(self): |
98 | super(BugSubscriptionAdvancedFeaturesTestCase, self).setUp() |
99 | + self.bug = self.factory.makeBug() |
100 | + self.person = self.factory.makePerson() |
101 | + self.team = self.factory.makeTeam() |
102 | with feature_flags(): |
103 | set_feature_flag(u'malone.advanced-subscriptions.enabled', u'on') |
104 | |
105 | @@ -276,6 +279,102 @@ |
106 | BugNotificationLevel.COMMENTS, |
107 | default_notification_level_value) |
108 | |
109 | + def test_muted_subs_have_unmute_option(self): |
110 | + # If a user has a muted subscription, the |
111 | + # BugSubscriptionSubscribeSelfView's subscription field will |
112 | + # show an "Unmute" option. |
113 | + with person_logged_in(self.person): |
114 | + self.bug.mute(self.person, self.person) |
115 | + |
116 | + with feature_flags(): |
117 | + with person_logged_in(self.person): |
118 | + subscribe_view = create_initialized_view( |
119 | + self.bug.default_bugtask, name='+subscribe') |
120 | + subscription_widget = ( |
121 | + subscribe_view.widgets['subscription']) |
122 | + # The Unmute option is actually treated the same way as |
123 | + # the unsubscribe option. |
124 | + self.assertEqual( |
125 | + "Unmute bug mail from this bug", |
126 | + subscription_widget.vocabulary.getTerm(self.person).title) |
127 | + |
128 | + def test_muted_subs_have_unmute_and_update_option(self): |
129 | + # If a user has a muted subscription, the |
130 | + # BugSubscriptionSubscribeSelfView's subscription field will |
131 | + # show an option to unmute the subscription and update it to a |
132 | + # new BugNotificationLevel. |
133 | + with person_logged_in(self.person): |
134 | + self.bug.mute(self.person, self.person) |
135 | + |
136 | + with feature_flags(): |
137 | + with person_logged_in(self.person): |
138 | + subscribe_view = create_initialized_view( |
139 | + self.bug.default_bugtask, name='+subscribe') |
140 | + subscription_widget = ( |
141 | + subscribe_view.widgets['subscription']) |
142 | + update_term = subscription_widget.vocabulary.getTermByToken( |
143 | + 'update-subscription') |
144 | + self.assertEqual( |
145 | + "Unmute bug mail from this bug and subscribe me to it", |
146 | + update_term.title) |
147 | + |
148 | + def test_unmute_unmutes(self): |
149 | + # Using the "Unmute bug mail" option when the user has a muted |
150 | + # subscription will remove the muted subscription. |
151 | + with person_logged_in(self.person): |
152 | + self.bug.mute(self.person, self.person) |
153 | + |
154 | + with feature_flags(): |
155 | + with person_logged_in(self.person): |
156 | + level = BugNotificationLevel.METADATA |
157 | + form_data = { |
158 | + 'field.subscription': self.person.name, |
159 | + # Although this isn't used we must pass it for the |
160 | + # sake of form validation. |
161 | + 'field.bug_notification_level': level.title, |
162 | + 'field.actions.continue': 'Continue', |
163 | + } |
164 | + subscribe_view = create_initialized_view( |
165 | + self.bug.default_bugtask, form=form_data, |
166 | + name='+subscribe') |
167 | + self.assertFalse(self.bug.isMuted(self.person)) |
168 | + self.assertFalse(self.bug.isSubscribed(self.person)) |
169 | + |
170 | + def test_update_when_muted_updates(self): |
171 | + # Using the "Unmute and subscribe me" option when the user has a |
172 | + # muted subscription will update the existing subscription to a |
173 | + # new BugNotificationLevel. |
174 | + with person_logged_in(self.person): |
175 | + muted_subscription = self.bug.mute(self.person, self.person) |
176 | + |
177 | + with feature_flags(): |
178 | + with person_logged_in(self.person): |
179 | + level = BugNotificationLevel.COMMENTS |
180 | + form_data = { |
181 | + 'field.subscription': 'update-subscription', |
182 | + 'field.bug_notification_level': level.title, |
183 | + 'field.actions.continue': 'Continue', |
184 | + } |
185 | + subscribe_view = create_initialized_view( |
186 | + self.bug.default_bugtask, form=form_data, |
187 | + name='+subscribe') |
188 | + self.assertFalse(self.bug.isMuted(self.person)) |
189 | + self.assertTrue(self.bug.isSubscribed(self.person)) |
190 | + self.assertEqual( |
191 | + level, muted_subscription.bug_notification_level) |
192 | + |
193 | + def test_bug_notification_level_field_has_widget_class(self): |
194 | + # The bug_notification_level widget has a widget_class property |
195 | + # that can be used to manipulate it with JavaScript. |
196 | + with person_logged_in(self.person): |
197 | + with feature_flags(): |
198 | + subscribe_view = create_initialized_view( |
199 | + self.bug.default_bugtask, name='+subscribe') |
200 | + widget_class = ( |
201 | + subscribe_view.widgets['bug_notification_level'].widget_class) |
202 | + self.assertEqual( |
203 | + 'bug-notification-level-field', widget_class) |
204 | + |
205 | |
206 | class BugPortletSubcribersIdsTests(TestCaseWithFactory): |
207 | |
208 | |
209 | === modified file 'lib/lp/bugs/configure.zcml' |
210 | --- lib/lp/bugs/configure.zcml 2011-03-15 04:15:45 +0000 |
211 | +++ lib/lp/bugs/configure.zcml 2011-03-17 15:02:39 +0000 |
212 | @@ -782,6 +782,7 @@ |
213 | linkMessage |
214 | markAsDuplicate |
215 | markUserAffected |
216 | + mute |
217 | newMessage |
218 | removeWatch |
219 | setPrivate |
220 | @@ -791,6 +792,7 @@ |
221 | unlinkBranch |
222 | unlinkCVE |
223 | unlinkHWSubmission |
224 | + unmute |
225 | unsubscribe |
226 | unsubscribeFromDupes |
227 | updateHeat" |
228 | |
229 | === modified file 'lib/lp/bugs/interfaces/bug.py' |
230 | --- lib/lp/bugs/interfaces/bug.py 2011-03-15 04:15:45 +0000 |
231 | +++ lib/lp/bugs/interfaces/bug.py 2011-03-17 15:02:39 +0000 |
232 | @@ -32,6 +32,7 @@ |
233 | export_write_operation, |
234 | exported, |
235 | mutator_for, |
236 | + operation_for_version, |
237 | operation_parameters, |
238 | operation_returns_collection_of, |
239 | operation_returns_entry, |
240 | @@ -489,6 +490,22 @@ |
241 | with a BugNotificationLevel of NOTHING. |
242 | """ |
243 | |
244 | + @operation_parameters( |
245 | + person=Reference(IPerson, title=_('Person'), required=False)) |
246 | + @call_with(muted_by=REQUEST_USER) |
247 | + @export_write_operation() |
248 | + @operation_for_version('devel') |
249 | + def mute(person, muted_by): |
250 | + """Add a muted subscription for `person`.""" |
251 | + |
252 | + @operation_parameters( |
253 | + person=Reference(IPerson, title=_('Person'), required=False)) |
254 | + @call_with(unmuted_by=REQUEST_USER) |
255 | + @export_write_operation() |
256 | + @operation_for_version('devel') |
257 | + def unmute(person, unmuted_by): |
258 | + """Remove a muted subscription for `person`.""" |
259 | + |
260 | def getDirectSubscriptions(): |
261 | """A sequence of IBugSubscriptions directly linked to this bug.""" |
262 | |
263 | |
264 | === added file 'lib/lp/bugs/javascript/bug_subscription.js' |
265 | --- lib/lp/bugs/javascript/bug_subscription.js 1970-01-01 00:00:00 +0000 |
266 | +++ lib/lp/bugs/javascript/bug_subscription.js 2011-03-17 15:02:39 +0000 |
267 | @@ -0,0 +1,59 @@ |
268 | +/* Copyright 2011 Canonical Ltd. This software is licensed under the |
269 | + * GNU Affero General Public License version 3 (see the file LICENSE). |
270 | + * |
271 | + * A bugtracker form overlay that can create a bugtracker within any page. |
272 | + * |
273 | + * @namespace Y.lp.bugs.bugtracker_overlay |
274 | + * @requires dom, node, io-base, lazr.anim, lazr.formoverlay |
275 | + */ |
276 | +YUI.add('lp.bugs.bug_subscription', function(Y) { |
277 | +var namespace = Y.namespace('lp.bugs.bug_subscription'); |
278 | +var level_div; |
279 | + |
280 | +// XXX: gmb 2011-03-17 bug=728457 |
281 | +// This fix for resizing needs to be incorporated into |
282 | +// lazr.effects. When that is done it should be removed from here. |
283 | +/* |
284 | + * Make sure that the bug_notification_level div is hidden properly . |
285 | + * @method clean_up_level_div |
286 | + */ |
287 | +namespace.clean_up_level_div = function() { |
288 | + if (Y.Lang.isValue(level_div)) { |
289 | + level_div.setStyles({ |
290 | + height: 0, |
291 | + visibility: 'hidden' |
292 | + }); |
293 | + } |
294 | +}; |
295 | + |
296 | +namespace.set_up_bug_notification_level_field = function() { |
297 | + level_div = Y.one('.bug-notification-level-field'); |
298 | + var subscription_radio_buttons = Y.all( |
299 | + 'input[name=field.subscription]'); |
300 | + |
301 | + // Only collapse the bug_notification_level field if the buttons are |
302 | + // available to display it again. |
303 | + if (Y.Lang.isValue(level_div) && subscription_radio_buttons.size() > 1) { |
304 | + var slide_in_anim = Y.lazr.effects.slide_in(level_div); |
305 | + slide_in_anim.on('end', namespace.clean_up_level_div); |
306 | + var slide_out_anim = Y.lazr.effects.slide_out(level_div); |
307 | + slide_out_anim.on('end', function() { |
308 | + level_div.setStyles({ |
309 | + visibility: 'visible' |
310 | + }); |
311 | + }); |
312 | + slide_in_anim.run(); |
313 | + Y.each(subscription_radio_buttons, function(subscription_button) { |
314 | + subscription_button.on('click', function(e) { |
315 | + if(e.target.get('value') == 'update-subscription') { |
316 | + slide_out_anim.run(); |
317 | + } else { |
318 | + slide_in_anim.run(); |
319 | + } |
320 | + }); |
321 | + }); |
322 | + } |
323 | +}; |
324 | + |
325 | +}, "0.1", {"requires": ["dom", "node", "io-base", "lazr.anim", "lazr.effects", |
326 | + ]}); |
327 | |
328 | === modified file 'lib/lp/bugs/javascript/bugtask_index_portlets.js' |
329 | --- lib/lp/bugs/javascript/bugtask_index_portlets.js 2011-03-14 14:39:33 +0000 |
330 | +++ lib/lp/bugs/javascript/bugtask_index_portlets.js 2011-03-17 15:02:39 +0000 |
331 | @@ -247,13 +247,10 @@ |
332 | } |
333 | |
334 | /* |
335 | - * Set up the handlers for the mute / unmute link. |
336 | + * Set up and return a Subscription object for the mute link. |
337 | + * @method get_mute_subscription |
338 | */ |
339 | -function setup_mute_link_handlers() { |
340 | - if (LP.links.me === undefined) { |
341 | - return; |
342 | - } |
343 | - |
344 | +function get_mute_subscription() { |
345 | setup_client_and_bug(); |
346 | var mute_link = Y.one('.menu-link-mute_subscription'); |
347 | var mute_subscription = new Y.lp.bugs.subscriber.Subscription({ |
348 | @@ -264,41 +261,56 @@ |
349 | subscriber_ids: subscriber_ids |
350 | }) |
351 | }); |
352 | + var parent_node = mute_link.get('parentNode'); |
353 | + mute_subscription.set( |
354 | + 'is_subscribed', !parent_node.hasClass('subscribed-false')); |
355 | + mute_subscription.set( |
356 | + 'is_muted', parent_node.hasClass('muted-true')); |
357 | mute_subscription.set( |
358 | 'person', mute_subscription.get('subscriber')); |
359 | + return mute_subscription; |
360 | +} |
361 | + |
362 | +/* |
363 | + * Set up the handlers for the mute / unmute link. |
364 | + */ |
365 | +function setup_mute_link_handlers() { |
366 | + if (LP.links.me === undefined) { |
367 | + return; |
368 | + } |
369 | + |
370 | + var mute_subscription = get_mute_subscription(); |
371 | + var mute_link = mute_subscription.get('link'); |
372 | + var parent_node = mute_link.get('parentNode'); |
373 | mute_link.addClass('js-action'); |
374 | mute_link.on('click', function(e) { |
375 | e.halt(); |
376 | - var parent_node = mute_link.get('parentNode'); |
377 | - var is_subscribed = !parent_node.hasClass('subscribed-false'); |
378 | var is_muted = parent_node.hasClass('muted-true'); |
379 | mute_subscription.enable_spinner('Muting...'); |
380 | if (! is_muted) { |
381 | - mute_subscription.set( |
382 | - 'bug_notification_level', "Nothing"); |
383 | - success_callback = function() { |
384 | - parent_node.removeClass('muted-false'); |
385 | - parent_node.addClass('muted-true'); |
386 | - mute_subscription.set('is_muted', true); |
387 | - update_subscription_after_mute_or_unmute( |
388 | - mute_subscription); |
389 | - } |
390 | - mute_current_user(mute_subscription, success_callback); |
391 | + mute_current_user(mute_subscription); |
392 | } else { |
393 | - success_callback = function() { |
394 | - parent_node.removeClass('muted-true'); |
395 | - parent_node.addClass('muted-false'); |
396 | - mute_subscription.set('is_muted', false); |
397 | - update_subscription_after_mute_or_unmute( |
398 | - mute_subscription); |
399 | - } |
400 | - unmute_current_user(mute_subscription, success_callback); |
401 | + unmute_current_user(mute_subscription); |
402 | } |
403 | - mute_subscription.disable_spinner(); |
404 | }); |
405 | } |
406 | |
407 | /* |
408 | + * Update the Mute link after the user's subscriptions or mutes have |
409 | + * changed. |
410 | + */ |
411 | +function update_mute_after_subscription_change(mute_subscription) { |
412 | + var parent_node = mute_subscription.get('link').get('parentNode'); |
413 | + if (mute_subscription.get('is_muted')) { |
414 | + parent_node.removeClass('muted-false'); |
415 | + parent_node.addClass('muted-true'); |
416 | + } else { |
417 | + parent_node.removeClass('muted-true'); |
418 | + parent_node.addClass('muted-false'); |
419 | + } |
420 | +} |
421 | + |
422 | +/* |
423 | * Update the subscription links after the mute button has been clicked. |
424 | * |
425 | * @param mute_subscription {Object} A Y.lp.bugs.subscriber.Subscription |
426 | @@ -593,6 +605,13 @@ |
427 | centered: true, |
428 | visible: false |
429 | }); |
430 | + // Register a couple of handlers to clean up the overlay when it's |
431 | + // hidden. |
432 | + subscription_overlay.get('form_cancel_button').on( |
433 | + 'click', |
434 | + Y.lp.bugs.bug_subscription.clean_up_level_div); |
435 | + subscription_overlay.on( |
436 | + 'cancel', Y.lp.bugs.bug_subscription.clean_up_level_div); |
437 | subscription_overlay.render('#privacy-form-container'); |
438 | return subscription_overlay; |
439 | } |
440 | @@ -615,6 +634,7 @@ |
441 | subscription_overlay.set( |
442 | 'form_submit_callback', function(form_data) { |
443 | handle_advanced_subscription_overlay(subscription, form_data); |
444 | + Y.lp.bugs.bug_subscription.clean_up_level_div(); |
445 | subscription_overlay.hide(); |
446 | subscription_overlay.loadFormContentAndRender( |
447 | subscription_link_url); |
448 | @@ -629,6 +649,13 @@ |
449 | subscription_overlay.renderUI(); |
450 | subscription_overlay.bindUI(); |
451 | subscription.disable_spinner(); |
452 | + |
453 | + // If the user has a mute on the bug we add some UI polish to |
454 | + // the subscription overlay. |
455 | + var mute_subscription = get_mute_subscription(); |
456 | + if(mute_subscription.get('is_muted')) { |
457 | + Y.lp.bugs.bug_subscription.set_up_bug_notification_level_field(); |
458 | + } |
459 | subscription_overlay.show(); |
460 | } |
461 | function on_failure(id, response, subscription_overlay) { |
462 | @@ -784,9 +811,8 @@ |
463 | * |
464 | * @method mute_current_user |
465 | * @param subscription {Object} A Y.lp.bugs.subscribe.Subscription object. |
466 | - * @param success_callback {Object} A function to be called on success. |
467 | */ |
468 | -function mute_current_user(subscription, success_callback) { |
469 | +function mute_current_user(subscription) { |
470 | subscription.enable_spinner('Muting...'); |
471 | var subscription_link = subscription.get('link'); |
472 | var subscriber = subscription.get('subscriber'); |
473 | @@ -805,11 +831,25 @@ |
474 | success: function(client) { |
475 | subscription.disable_spinner('Unmute bug mail'); |
476 | var flash_node = subscription_link.get('parentNode'); |
477 | - var anim = Y.lazr.anim.green_flash({ node: flash_node }); |
478 | - anim.run(); |
479 | - if (Y.Lang.isValue(success_callback)) { |
480 | - success_callback(); |
481 | + var mute_anim = Y.lazr.anim.green_flash({ node: flash_node }); |
482 | + mute_anim.run(); |
483 | + |
484 | + // Remove the subscriber's name from the subscriber |
485 | + // list, if it's there. |
486 | + var subscriber_node = Y.one('.' + subscriber.get('css_name')); |
487 | + if (Y.Lang.isValue(subscriber_node)) { |
488 | + var subscriber_anim = Y.lazr.anim.green_flash({ |
489 | + node: subscriber_node |
490 | + }); |
491 | + subscriber_anim.on('end', function(e) { |
492 | + remove_user_name_link(subscriber_node); |
493 | + set_none_for_empty_subscribers(); |
494 | + }); |
495 | + subscriber_anim.run(); |
496 | } |
497 | + subscription.set('is_muted', true); |
498 | + update_mute_after_subscription_change(subscription); |
499 | + update_subscription_after_mute_or_unmute(subscription); |
500 | }, |
501 | |
502 | failure: error_handler.getFailureHandler() |
503 | @@ -817,12 +857,10 @@ |
504 | |
505 | parameters: { |
506 | person: Y.lp.client.get_absolute_uri( |
507 | - subscriber.get('escaped_uri')), |
508 | - suppress_notify: false, |
509 | - level: bug_notification_level |
510 | + subscriber.get('escaped_uri')) |
511 | } |
512 | }; |
513 | - lp_client.named_post(bug_repr.self_link, 'subscribe', config); |
514 | + lp_client.named_post(bug_repr.self_link, 'mute', config); |
515 | } |
516 | |
517 | /* |
518 | @@ -830,9 +868,8 @@ |
519 | * |
520 | * @method unmute_current_user |
521 | * @param subscription {Object} A Y.lp.bugs.subscriber.Subscription object. |
522 | - * @param success_callback {Object} A function to be called on success. |
523 | */ |
524 | -function unmute_current_user(subscription, success_callback) { |
525 | +function unmute_current_user(subscription) { |
526 | subscription.enable_spinner('Unmuting...'); |
527 | var subscription_link = subscription.get('link'); |
528 | var subscriber = subscription.get('subscriber'); |
529 | @@ -852,15 +889,20 @@ |
530 | var flash_node = subscription_link.get('parentNode'); |
531 | var anim = Y.lazr.anim.green_flash({ node: flash_node }); |
532 | anim.run(); |
533 | - if (Y.Lang.isValue(success_callback)) { |
534 | - success_callback(); |
535 | - } |
536 | + subscription.set('is_muted', false); |
537 | + update_mute_after_subscription_change(subscription); |
538 | + update_subscription_after_mute_or_unmute(subscription); |
539 | }, |
540 | |
541 | failure: error_handler.getFailureHandler() |
542 | + }, |
543 | + |
544 | + parameters: { |
545 | + person: Y.lp.client.get_absolute_uri( |
546 | + subscriber.get('escaped_uri')) |
547 | } |
548 | }; |
549 | - lp_client.named_post(bug_repr.self_link, 'unsubscribe', config); |
550 | + lp_client.named_post(bug_repr.self_link, 'unmute', config); |
551 | } |
552 | |
553 | /* |
554 | @@ -1153,17 +1195,19 @@ |
555 | function handle_advanced_subscription_overlay(subscription, form_data) { |
556 | var link = subscription.get('link'); |
557 | var link_parent = link.get('parentNode'); |
558 | + var mute_subscription = get_mute_subscription(); |
559 | if (link_parent.hasClass('subscribed-false') && |
560 | - link_parent.hasClass('dup-subscribed-false')) { |
561 | - // The user isn't subscribed, so subscribe them. |
562 | + link_parent.hasClass('dup-subscribed-false') && |
563 | + !mute_subscription.get('is_muted')) { |
564 | + // The user isn't subscribed or muted, so subscribe them. |
565 | subscription.set( |
566 | 'bug_notification_level', |
567 | form_data['field.bug_notification_level']); |
568 | subscribe_current_user(subscription); |
569 | } else if ( |
570 | form_data['field.subscription'] == 'update-subscription') { |
571 | - // The user is already subscribed and wants to update their |
572 | - // subscription. |
573 | + // The user is already subscribed or is muted and wants to |
574 | + // update their subscription. |
575 | setup_client_and_bug(); |
576 | var person_name = subscription.get('person').get('name'); |
577 | var subscription_url = |
578 | @@ -1173,6 +1217,9 @@ |
579 | on: { |
580 | success: function(lp_subscription) { |
581 | subscription.enable_spinner('Updating subscription...'); |
582 | + if (mute_subscription.get('is_muted')) { |
583 | + mute_subscription.enable_spinner('Unmuting...'); |
584 | + } |
585 | lp_subscription.set( |
586 | 'bug_notification_level', |
587 | form_data['field.bug_notification_level'][0]) |
588 | @@ -1185,6 +1232,14 @@ |
589 | node: link_parent |
590 | }); |
591 | anim.run(); |
592 | + if (mute_subscription.get('is_muted')) { |
593 | + mute_subscription.set('is_muted', false); |
594 | + mute_subscription.disable_spinner( |
595 | + "Mute bug mail"); |
596 | + update_mute_after_subscription_change( |
597 | + mute_subscription); |
598 | + add_user_name_link(subscription); |
599 | + } |
600 | }, |
601 | failure: function(e) { |
602 | subscription.disable_spinner( |
603 | @@ -1366,4 +1421,4 @@ |
604 | "lazr.overlay", "lazr.choiceedit", "lp.app.picker", |
605 | "lp.client", |
606 | "lp.client.plugins", "lp.bugs.subscriber", |
607 | - "lp.app.errors"]}); |
608 | + "lp.bugs.bug_subscription", "lp.app.errors"]}); |
609 | |
610 | === modified file 'lib/lp/bugs/model/bug.py' |
611 | --- lib/lp/bugs/model/bug.py 2011-03-15 04:15:45 +0000 |
612 | +++ lib/lp/bugs/model/bug.py 2011-03-17 15:02:39 +0000 |
613 | @@ -836,6 +836,27 @@ |
614 | BugNotificationLevel.NOTHING) |
615 | return not subscriptions.is_empty() |
616 | |
617 | + def mute(self, person, muted_by): |
618 | + """See `IBug`.""" |
619 | + # If there's an existing subscription, update it. |
620 | + store = Store.of(self) |
621 | + subscriptions = store.find( |
622 | + BugSubscription, |
623 | + BugSubscription.bug == self, |
624 | + BugSubscription.person == person) |
625 | + if subscriptions.is_empty(): |
626 | + return self.subscribe( |
627 | + person, muted_by, level=BugNotificationLevel.NOTHING) |
628 | + else: |
629 | + subscription = subscriptions.one() |
630 | + subscription.bug_notification_level = ( |
631 | + BugNotificationLevel.NOTHING) |
632 | + return subscription |
633 | + |
634 | + def unmute(self, person, unmuted_by): |
635 | + """See `IBug`.""" |
636 | + self.unsubscribe(person, unmuted_by) |
637 | + |
638 | @property |
639 | def subscriptions(self): |
640 | """The set of `BugSubscriptions` for this bug.""" |
641 | |
642 | === modified file 'lib/lp/bugs/templates/bug-subscription.pt' |
643 | --- lib/lp/bugs/templates/bug-subscription.pt 2010-10-18 10:14:37 +0000 |
644 | +++ lib/lp/bugs/templates/bug-subscription.pt 2011-03-17 15:02:39 +0000 |
645 | @@ -10,6 +10,25 @@ |
646 | i18n:domain="malone" |
647 | > |
648 | |
649 | + <metal:block fill-slot="head_epilogue"> |
650 | + <script type="text/javascript" |
651 | + tal:condition="devmode" |
652 | + tal:content="string:var yui_base='${yui}';"></script> |
653 | + <script type="text/javascript" |
654 | + tal:condition="devmode" |
655 | + tal:define="lp_js string:${icingroot}/build" |
656 | + tal:attributes="src string:${lp_js}/bugs/filebug-dupefinder.js"></script> |
657 | + |
658 | + <script type="text/javascript" tal:condition="view/user_is_muted"> |
659 | + LPS.use('base', 'node', 'oop', 'event', 'lp.bugs.bug_subscription', |
660 | + function(Y) { |
661 | + Y.on( |
662 | + 'domready', |
663 | + Y.lp.bugs.bug_subscription.set_up_bug_notification_level_field); |
664 | + }); |
665 | + </script> |
666 | + </metal:block> |
667 | + |
668 | <body> |
669 | <div metal:fill-slot="main"> |
670 | |
671 | |
672 | === modified file 'lib/lp/bugs/tests/test_bug.py' |
673 | --- lib/lp/bugs/tests/test_bug.py 2011-03-04 16:17:08 +0000 |
674 | +++ lib/lp/bugs/tests/test_bug.py 2011-03-17 15:02:39 +0000 |
675 | @@ -44,3 +44,33 @@ |
676 | # subscription. |
677 | with person_logged_in(self.person): |
678 | self.assertEqual(False, self.bug.isMuted(self.person)) |
679 | + |
680 | + def test_mute_mutes_user(self): |
681 | + # Bug.mute() adds a muted subscription for the user passed to |
682 | + # it. |
683 | + with person_logged_in(self.person): |
684 | + muted_subscription = self.bug.mute( |
685 | + self.person, self.person) |
686 | + self.assertEqual( |
687 | + BugNotificationLevel.NOTHING, |
688 | + muted_subscription.bug_notification_level) |
689 | + |
690 | + def test_mute_mutes_user_with_existing_subscription(self): |
691 | + # Bug.mute() will update an existing subscription so that it |
692 | + # becomes muted. |
693 | + with person_logged_in(self.person): |
694 | + subscription = self.bug.subscribe(self.person, self.person) |
695 | + muted_subscription = self.bug.mute(self.person, self.person) |
696 | + self.assertEqual(subscription, muted_subscription) |
697 | + self.assertEqual( |
698 | + BugNotificationLevel.NOTHING, |
699 | + subscription.bug_notification_level) |
700 | + |
701 | + def test_unmute_unmutes_user(self): |
702 | + # Bug.unmute() will remove a muted subscription for the user |
703 | + # passed to it. |
704 | + with person_logged_in(self.person): |
705 | + self.bug.mute(self.person, self.person) |
706 | + self.assertTrue(self.bug.isMuted(self.person)) |
707 | + self.bug.unmute(self.person, self.person) |
708 | + self.assertFalse(self.bug.isMuted(self.person)) |
Graham--
Code wise, this all looks good to me.
I do have a couple of questions, but they're not critiques of the code, just me being confused and seeing this as a good opportunity to figure something out.
* What's the exact difference between a muted subscription and not being subscribed? Is it a difference between getting all comments and just say, getting notified when something is Fix released?
* The answer to the first may answer this, but: why is unmuting the same as unsubscribing- -wouldn' t unmuting leave a subscription but signal I want to get notified of more events?
* In bugtask_ index_portlets, you pull out a bunch of code from some callbacks and create a function, `update_ mute_after_ subscription_ change` , that runs the same code. This is pretty clearly to keep code DRY, but you then manually call it rather than pass it in as the callback. I'm the first to say I don't know a ton about how callbacks work vs manually calling a function, so I was wondering what the reason is for not passing it an as a callback?
Again, this is all about helping me figure things out, not a problem with your branch.
Thanks for any answers, and nice work.