launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03557
[Merge] lp:~danilo/launchpad/bug-771204 into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/bug-771204 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #771204 in Launchpad itself: "mute link does not appear when a new direct subscription is made on a bug"
https://bugs.launchpad.net/launchpad/+bug/771204
For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-771204/+merge/60493
= Bug 771204 =
When someone subscribes, mute link doesn't show up for them to mute the subscription. Similar happens when someone subscribes a team they are a member of.
== Proposed fix ==
Always render the 'mute link', but default to adding the 'hidden' class so it doesn't show up. For graceful degradation (there's already a mute form on a separate page that the link links to), we do not set the 'hidden' flag if it needs to show up by default.
In JS, we simply unhide it when it's first needed using update_mute_after_subscription_change() method.
== Implementation details ==
Side-fix:
- the flash_node animation in the diff was dropping a warning in the console about the node not being defined (because it's not yet in the DOM), so I've wrapped the animation in a 'contentready' event handler.
== Tests ==
No new tests for JS: this still needs a lot of work and refactoring to be easily testable. We are planning to have at least good integration test.
== Demo and Q/A ==
https://bugs.launchpad.dev/debian/+source/mozilla-firefox/+bug/3
With feature flag: "malone.advanced-subscriptions.enabled default 1 on".
Log-in as admin@xxxxxxxxxxxxx:test.
Subscribe yourself, a "mute link" should show up. Unsubscribe yourself and refresh (that'd be a separate bug that mute link never disappears).
Subscribe someone else (eg. "mark"), note that mute link doesn't show up.
Subscribe someone else (a team you are a member of, eg. "ubuntu-team"), note that the mute link shows up.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/browser/tests/test_bug_views.py
lib/lp/bugs/browser/bug.py
lib/lp/bugs/templates/bug-portlet-subscribers.pt
lib/lp/bugs/javascript/bugtask_index_portlets.js
--
https://code.launchpad.net/~danilo/launchpad/bug-771204/+merge/60493
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-771204 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py 2011-05-10 02:23:38 +0000
+++ lib/lp/bugs/browser/bug.py 2011-05-10 11:41:08 +0000
@@ -523,10 +523,15 @@
def current_user_mute_class(self):
bug = self.context
subscription_class = self.current_user_subscription_class
+ if self.user_should_see_mute_link:
+ visibility_class = ''
+ else:
+ visibility_class = 'hidden'
if bug.isMuted(self.user):
- return 'muted-true %s' % subscription_class
+ return 'muted-true %s %s' % (subscription_class, visibility_class)
else:
- return 'muted-false %s' % subscription_class
+ return 'muted-false %s %s' % (
+ subscription_class, visibility_class)
@cachedproperty
def user_should_see_mute_link(self):
=== modified file 'lib/lp/bugs/browser/tests/test_bug_views.py'
--- lib/lp/bugs/browser/tests/test_bug_views.py 2011-05-10 02:23:38 +0000
+++ lib/lp/bugs/browser/tests/test_bug_views.py 2011-05-10 11:41:08 +0000
@@ -7,6 +7,8 @@
from zope.component import getUtility
+from BeautifulSoup import BeautifulSoup
+
from canonical.launchpad.webapp.publisher import canonical_url
from canonical.launchpad.webapp.interfaces import IOpenLaunchBag
from canonical.launchpad.webapp.servers import LaunchpadTestRequest
@@ -64,26 +66,6 @@
launchbag.add(self.bug)
launchbag.add(self.bug.default_bugtask)
- def test_mute_subscription_link_not_shown_for_non_subscribers(self):
- # If a person is not already subscribed to a bug in some way,
- # the mute link will not be displayed to them.
- person = self.factory.makePerson()
- with person_logged_in(person):
- with FeatureFixture({self.feature_flag_2: None}):
- # The user isn't subscribed or muted already.
- self.assertFalse(self.bug.isSubscribed(person))
- self.assertFalse(self.bug.isMuted(person))
- self.assertFalse(
- self.bug.personIsAlsoNotifiedSubscriber(
- person))
- view = create_initialized_view(
- self.bug, name="+portlet-subscribers")
- self.assertFalse(view.user_should_see_mute_link)
- # The template uses user_should_see_mute_link to decide
- # whether or not to display the mute link.
- html = view.render()
- self.assertFalse('mute_subscription' in html)
-
def test_edit_subscriptions_link_shown_when_feature_enabled(self):
with FeatureFixture({self.feature_flag_2: 'on'}):
request = LaunchpadTestRequest()
@@ -101,6 +83,24 @@
self.assertTrue('menu-link-editsubscriptions' not in html)
self.assertTrue('/+subscriptions' not in html)
+ def test_mute_subscription_link_not_shown_with_no_feature_flag(self):
+ # Mute link is not shown when the feature flag is off.
+ person = self.factory.makePerson()
+ with person_logged_in(person):
+ with FeatureFixture({self.feature_flag_1: None}):
+ view = create_initialized_view(
+ self.bug, name="+portlet-subscribers")
+ self.assertFalse(view.user_should_see_mute_link)
+ html = view.render()
+ self.assertFalse('mute_subscription' in html)
+
+ def _hasCSSClass(self, html, element_id, css_class):
+ # Return True if element with ID `element_id` in `html` has
+ # a CSS class `css_class`.
+ soup = BeautifulSoup(html)
+ element = soup.find(attrs={'id': element_id})
+ return css_class in element['class'].split(' ')
+
def test_bug_mute_for_individual_structural_subscription(self):
# If the person has a structural subscription to the pillar,
# then the mute link will be displayed to them.
@@ -116,6 +116,9 @@
contents = view.render()
self.assertTrue('mute_subscription' in contents,
"'mute_subscription' not in contents.")
+ self.assertFalse(
+ self._hasCSSClass(
+ contents, 'mute-link-container', 'hidden'))
create_initialized_view(
self.bug.default_bugtask, name="+mute",
form={'field.actions.mute': 'Mute bug mail'})
@@ -143,7 +146,34 @@
contents = view.render()
self.assertTrue('mute_subscription' in contents,
"'mute_subscription' not in contents.")
+ self.assertFalse(
+ self._hasCSSClass(
+ contents, 'mute-link-container', 'hidden'))
create_initialized_view(
self.bug.default_bugtask, name="+mute",
form={'field.actions.mute': 'Mute bug mail'})
self.assertTrue(self.bug.isMuted(person))
+
+ def test_mute_subscription_link_hidden_for_non_subscribers(self):
+ # If a person is not already subscribed to a bug in some way,
+ # the mute link will not be displayed to them.
+ person = self.factory.makePerson()
+ with person_logged_in(person):
+ with FeatureFixture({self.feature_flag_1: 'on'}):
+ # The user isn't subscribed or muted already.
+ self.assertFalse(self.bug.isSubscribed(person))
+ self.assertFalse(self.bug.isMuted(person))
+ self.assertFalse(
+ self.bug.personIsAlsoNotifiedSubscriber(
+ person))
+ view = create_initialized_view(
+ self.bug, name="+portlet-subscribers")
+ self.assertFalse(view.user_should_see_mute_link)
+ html = view.render()
+ self.assertTrue('mute_subscription' in html)
+ # The template uses user_should_see_mute_link to decide
+ # whether or not to display the mute link.
+ soup = BeautifulSoup(html)
+ self.assertTrue(
+ self._hasCSSClass(html, 'mute-link-container', 'hidden'),
+ 'No "hidden" CSS class in mute-link-container.')
=== modified file 'lib/lp/bugs/javascript/bugtask_index_portlets.js'
--- lib/lp/bugs/javascript/bugtask_index_portlets.js 2011-04-29 14:01:34 +0000
+++ lib/lp/bugs/javascript/bugtask_index_portlets.js 2011-05-10 11:41:08 +0000
@@ -337,6 +337,7 @@
function update_mute_after_subscription_change(mute_subscription) {
var mute_link = mute_subscription.get('link');
var parent_node = mute_link.get('parentNode');
+ parent_node.removeClass('hidden');
if (mute_subscription.get('is_muted')) {
parent_node.removeClass('muted-false');
parent_node.addClass('muted-true');
@@ -763,10 +764,15 @@
}
add_user_name_link(subscription);
+ var mute_subscription = get_mute_subscription();
+ update_mute_after_subscription_change(mute_subscription);
- var flash_node = Y.one('.' + subscriber.get('css_name'));
- var anim = Y.lazr.anim.green_flash({ node: flash_node });
- anim.run();
+ // Only when the link is added to the page, indicate success.
+ Y.on('contentready', function() {
+ var flash_node = Y.one('.' + subscriber.get('css_name'));
+ var anim = Y.lazr.anim.green_flash({ node: flash_node });
+ anim.run();
+ }, '.' + subscriber.get('css_name'));
},
failure: error_handler.getFailureHandler()
@@ -1376,6 +1382,10 @@
if (team_member) {
subscription.set('can_be_unsubscribed', true);
add_temp_user_name(subscription);
+ var mute_subscription;
+ mute_subscription = get_mute_subscription();
+ update_mute_after_subscription_change(
+ mute_subscription);
} else {
subscription.set(
'can_be_unsubscribed', false);
=== modified file 'lib/lp/bugs/templates/bug-portlet-subscribers.pt'
--- lib/lp/bugs/templates/bug-portlet-subscribers.pt 2011-04-13 18:38:54 +0000
+++ lib/lp/bugs/templates/bug-portlet-subscribers.pt 2011-05-10 11:41:08 +0000
@@ -15,8 +15,10 @@
<div tal:content="structure context_menu/addsubscriber/render" />
<div tal:condition="request/features/malone.advanced-structural-subscriptions.enabled"
tal:content="structure context_menu/editsubscriptions/render" />
- <tal:show-mute condition="view/user_should_see_mute_link">
- <div tal:attributes="class view/current_user_mute_class">
+ <tal:show-mute condition="
+ request/features/malone.advanced-subscriptions.enabled">
+ <div tal:attributes="class view/current_user_mute_class"
+ id="mute-link-container">
<span tal:replace="structure context_menu/mute_subscription/render"/>
<a target="help" class="sprite maybe mute-help"
href="/+help/subscription-mute.html"
