Merge lp:~wgrant/launchpad/tiny-inline-subscription-fixes into lp:launchpad

Proposed by William Grant
Status: Merged
Merged at revision: not available
Proposed branch: lp:~wgrant/launchpad/tiny-inline-subscription-fixes
Merge into: lp:launchpad
Diff against target: None lines
To merge this branch: bzr merge lp:~wgrant/launchpad/tiny-inline-subscription-fixes
Reviewer Review Type Date Requested Status
Eleanor Berger (community) code Approve
Review via email: mp+10454@code.launchpad.net
To post a comment you must log in.
Revision history for this message
William Grant (wgrant) wrote :

= Summary =

I picked up a couple of bugs in the inline subscription stuff while testing fixes for the other bugs earlier this week. I thought I might as well fix them now.

See the two linked bugs for details.

== Proposed fix ==

The team icon was showing up as the icon for self-subscription because the self-subscription handler doesn't set is_team -- only check_can_be_unsubscribed does. I've forced is_team to false when setting the current person on the subscription.

The unsubscription handler was unconditionally setting the 'add' class on the Unsubscribe link. I initially moved the manual class changes into the block that sets the text to "Subscribe", but later discovered subscription.disable_spinner(). I now instead use that.

== Tests ==

I've added Windmill tests for the two bugs.

 $ ./bin/lp-windmill -e test=lib/lp/bugs/windmill/tests/test_bugs/test_bug_inline_subscriber.py firefox http://bugs.launchpad.dev:8085

== Demo and Q/A ==

To Q/A:

Bug #415220:
 1. Subscribe yourself to a bug.
 2. Subscribe a team of which you are a member to the bug.
 4. Click the link to unsubscribe the team.
 5. Verify that the "(-) Unsubscribe" link retains the correct icon.

Bug #415219:
 1. Subscribe a team to a bug.
 2. Subscribe yourself to a bug (using "Subscribe", not "Subscribe someone else").
 3. Verify that the icon next to your name is the person, not team, icon.

Revision history for this message
Eleanor Berger (intellectronica) wrote :

Good fix. Thanks for picking this up.

review: Approve (code)

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 2009-08-20 04:52:29 +0000
+++ lib/canonical/launchpad/javascript/bugs/bugtask-index.js 2009-08-20 11:38:10 +0000
@@ -294,6 +294,7 @@
294 e.halt();294 e.halt();
295 subscription.set('can_be_unsubscribed', true);295 subscription.set('can_be_unsubscribed', true);
296 subscription.set('person', subscription.get('subscriber'));296 subscription.set('person', subscription.get('subscriber'));
297 subscription.set('is_team', false);
297 var parent = e.target.get('parentNode');298 var parent = e.target.get('parentNode');
298 // Look for the false conditions of subscription, which299 // Look for the false conditions of subscription, which
299 // is_direct_subscription, etc. don't report correctly,300 // is_direct_subscription, etc. don't report correctly,
@@ -1030,7 +1031,7 @@
1030 if (Y.Lang.isNull(person_link) &&1031 if (Y.Lang.isNull(person_link) &&
1031 subscription.is_current_user_subscribing()) {1032 subscription.is_current_user_subscribing()) {
1032 // Current user has been completely unsubscribed.1033 // Current user has been completely unsubscribed.
1033 subscription_link.set('innerHTML', 'Subscribe');1034 subscription.disable_spinner('Subscribe');
1034 set_subscription_link_parent_class(1035 set_subscription_link_parent_class(
1035 subscription_link, false, false);1036 subscription_link, false, false);
1036 subscription.set('is_direct', false);1037 subscription.set('is_direct', false);
@@ -1050,8 +1051,6 @@
1050 subscription.set('has_dupes', true);1051 subscription.set('has_dupes', true);
1051 }1052 }
1052 }1053 }
1053 subscription_link.removeClass('modify remove');
1054 subscription_link.addClass('add');
1055 });1054 });
1056 anim.run();1055 anim.run();
1057 },1056 },
10581057
=== modified file 'lib/lp/bugs/windmill/tests/test_bugs/test_bug_inline_subscriber.py'
--- lib/lp/bugs/windmill/tests/test_bugs/test_bug_inline_subscriber.py 2009-07-30 12:37:18 +0000
+++ lib/lp/bugs/windmill/tests/test_bugs/test_bug_inline_subscriber.py 2009-08-20 10:28:37 +0000
@@ -118,6 +118,14 @@
118 client.click(118 client.click(
119 xpath=u'//div[@class="yui-lazr-formoverlay-actions"]/button[2]')119 xpath=u'//div[@class="yui-lazr-formoverlay-actions"]/button[2]')
120120
121 # If we subscribe the user again, the icon should still be the person icon.
122 client.click(xpath=SUBSCRIPTION_LINK)
123 client.waits.sleep(milliseconds=SLEEP)
124 client.asserts.assertProperty(
125 xpath=(PERSON_LINK % u'Sample Person') + '/span',
126 validator=u'className|person')
127
128
121 # Sample Person is logged in currently. She is not a129 # Sample Person is logged in currently. She is not a
122 # member of Ubuntu Team, and so, does not have permission130 # member of Ubuntu Team, and so, does not have permission
123 # to unsubscribe the team.131 # to unsubscribe the team.
@@ -135,6 +143,11 @@
135 client.click(id=u'unsubscribe-icon-subscriber-17')143 client.click(id=u'unsubscribe-icon-subscriber-17')
136 client.waits.sleep(milliseconds=SLEEP)144 client.waits.sleep(milliseconds=SLEEP)
137 client.asserts.assertNotNode(xpath=PERSON_LINK % u'Ubuntu Team')145 client.asserts.assertNotNode(xpath=PERSON_LINK % u'Ubuntu Team')
146 client.asserts.assertText(
147 xpath=SUBSCRIPTION_LINK, validator=u'Unsubscribe')
148 client.asserts.assertProperty(
149 xpath=SUBSCRIPTION_LINK,
150 validator=u'className|remove')
138151
139 # Test unsubscribing via the remove icon for duplicates.152 # Test unsubscribing via the remove icon for duplicates.
140 # First, go to bug 6 and subscribe.153 # First, go to bug 6 and subscribe.