launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03474
[Merge] lp:~danilo/launchpad/subscribers-list-js into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/subscribers-list-js into lp:launchpad with lp:~danilo/launchpad/bug-770241 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~danilo/launchpad/subscribers-list-js/+merge/59387
= Bug subscribers list: user link removal =
Move user link removal to the newly started subscribers_list module and add tests for it.
== Proposed fix ==
remove_user_name_link() in bugtask_index_portlets.js was:
- used every time as anim.on('end') handler together with reset()
- simply doing node.remove() which requires no separate method
So, I've joined it all together, added tests and moved it to bugs.subscribers_list JS module as remove_user_link() method.
== Implementation details ==
- bugtask_index_portlets.js: other than the change to replace repeated code with remove_user_link(), the only change is unindenting the big block that was unnecessarily in anim.on('end') in one occurrence
- tests are relatively comprehensive, and because of the green_flash animation taking a full second, a bit slow :/
- I am still working on more clean-ups in bugtask_index_portlets.js because it's basically untestable because of the hacky nature
== Tests ==
lib/lp/bugs/javascript/tests/test_subscribers_list.html
== Demo and Q/A ==
1. Mark https://bugs.launchpad.dev/bugs/2 as duplicate of https://bugs.launchpad.dev/bugs/1
2. "Subscribe someone else" on bug 2 (eg. ubuntu-team)
3. "Subscribe (the same) someone else" on bug 1.
4. Subscribe yourself
4. Reload bug 1 page.
5. Play with unsubscribe icons
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/javascript/tests/test_subscribers_list.html
lib/lp/bugs/javascript/bugtask_index_portlets.js
lib/lp/bugs/javascript/subscribers_list.js
lib/lp/bugs/javascript/tests/test_subscribers_list.js
--
https://code.launchpad.net/~danilo/launchpad/subscribers-list-js/+merge/59387
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/subscribers-list-js into lp:launchpad.
=== modified file 'lib/lp/bugs/javascript/bugtask_index_portlets.js'
--- lib/lp/bugs/javascript/bugtask_index_portlets.js 2011-04-28 13:46:24 +0000
+++ lib/lp/bugs/javascript/bugtask_index_portlets.js 2011-04-28 15:09:35 +0000
@@ -608,38 +608,26 @@
var config = {
on: {
success: function(client) {
- icon_parent.removeChild(icon);
- var anim = Y.lazr.anim.green_flash({ node: icon_parent_div });
- anim.on('end', function(e) {
- remove_user_name_link(icon_parent_div);
- Y.lp.bugs.subscribers_list.reset();
- var person_link = Y.one('.' + person.get('css_name'));
- if (Y.Lang.isNull(person_link) &&
- subscription.is_current_user_subscribing()) {
- // Current user has been completely unsubscribed.
- subscription.disable_spinner(
- subscription_labels.SUBSCRIBE);
- set_subscription_link_parent_class(
- subscription_link, false, false);
- subscription.set('is_direct', false);
- subscription.set('has_dupes', false);
- } else {
- if (is_dupe) {
- // A direct subscription remains.
- set_subscription_link_parent_class(
- subscription_link, true, false);
- subscription.set('is_direct', true);
- subscription.set('has_dupes', false);
- } else {
- // A dupe subscription remains.
- set_subscription_link_parent_class(
- subscription_link, false, true);
- subscription.set('is_direct', false);
- subscription.set('has_dupes', true);
- }
- }
- });
- anim.run();
+ Y.lp.bugs.subscribers_list.remove_user_link(person, is_dupe);
+ var person_link = Y.one('.' + person.get('css_name'));
+ var has_direct, has_dupes;
+ if (Y.Lang.isNull(person_link) &&
+ subscription.is_current_user_subscribing()) {
+ // Current user has been completely unsubscribed.
+ subscription.disable_spinner(
+ subscription_labels.SUBSCRIBE);
+ has_direct = false;
+ has_dupes = false;
+ } else {
+ // If we removed the duplicate subscription,
+ // we are left with the direct one, and vice versa.
+ has_direct = is_dupe;
+ has_dupes = !is_dupe;
+ }
+ subscription.set('is_direct', has_direct);
+ subscription.set('has_dupes', has_dupes);
+ set_subscription_link_parent_class(
+ subscription_link, is_direct, has_dupes);
},
failure: error_handler.getFailureHandler()
@@ -832,13 +820,7 @@
subscription.set('has_dupes', false);
}
- var flash_node = Y.one('.' + subscriber.get('css_name'));
- var anim = Y.lazr.anim.green_flash({ node: flash_node });
- anim.on('end', function(e) {
- remove_user_name_link(flash_node);
- Y.lp.bugs.subscribers_list.reset();
- });
- anim.run();
+ Y.lp.bugs.subscribers_list.remove_user_link(subscriber);
},
failure: error_handler.getFailureHandler()
@@ -904,17 +886,7 @@
// Remove the subscriber's name from the subscriber
// list, if it's there.
- var subscriber_node = Y.one('.' + subscriber.get('css_name'));
- if (Y.Lang.isValue(subscriber_node)) {
- var subscriber_anim = Y.lazr.anim.green_flash({
- node: subscriber_node
- });
- subscriber_anim.on('end', function(e) {
- remove_user_name_link(subscriber_node);
- Y.lp.bugs.subscribers_list.reset();
- });
- subscriber_anim.run();
- }
+ Y.lp.bugs.subscribers_list.remove_user_link(subscriber);
subscription.set('is_muted', true);
update_mute_after_subscription_change(subscription);
update_subscription_after_mute_or_unmute(subscription);
@@ -1179,17 +1151,6 @@
}
/*
- * Used to remove the user's name from the subscriber's list.
- *
- * @method remove_user_name_link
- * @param user_node {Node} Node representing the user name link.
- */
-function remove_user_name_link(user_node) {
- var parent = user_node.get('parentNode');
- parent.removeChild(user_node);
-}
-
-/*
* Set the class on subscription link's parentNode.
*
* This is used to reset the class used by the
=== modified file 'lib/lp/bugs/javascript/subscribers_list.js'
--- lib/lp/bugs/javascript/subscribers_list.js 2011-04-28 09:26:07 +0000
+++ lib/lp/bugs/javascript/subscribers_list.js 2011-04-28 15:09:35 +0000
@@ -38,6 +38,41 @@
dup_list.remove();
}
}
-namespace.reset = reset;
-
-}, "0.1", {"requires": ["node"]});
+namespace._reset = reset;
+
+/**
+ * Remove the user's name from the subscribers list.
+ * It uses the green-flash animation to indicate successful removal.
+ *
+ * @method remove_user_link
+ * @param subscriber {Subscriber} Subscriber that you want to remove.
+ * @param is_dupe {Boolean} Uses subscription link from the duplicates
+ * instead.
+ */
+function remove_user_link(subscriber, is_dupe) {
+ var user_node_id;
+ var user_name = subscriber.get('css_name');
+ if (is_dupe === true) {
+ user_node_id = '#dupe-' + user_name;
+ } else {
+ user_node_id = '#direct-' + user_name;
+ }
+ var user_node = Y.one(user_node_id);
+ if (Y.Lang.isValue(user_node)) {
+ // If there's an icon, we remove it prior to animation
+ // so animation looks better.
+ var unsub_icon = user_node.one('#unsubscribe-icon-' + user_name);
+ if (Y.Lang.isValue(unsub_icon)) {
+ unsub_icon.remove();
+ }
+ var anim = Y.lazr.anim.green_flash({ node: user_node });
+ anim.on('end', function() {
+ user_node.remove();
+ reset();
+ });
+ anim.run();
+ }
+}
+namespace.remove_user_link = remove_user_link;
+
+}, "0.1", {"requires": ["node", "lazr.anim"]});
=== modified file 'lib/lp/bugs/javascript/tests/test_subscribers_list.html'
--- lib/lp/bugs/javascript/tests/test_subscribers_list.html 2011-04-28 09:26:07 +0000
+++ lib/lp/bugs/javascript/tests/test_subscribers_list.html 2011-04-28 15:09:35 +0000
@@ -21,6 +21,10 @@
<script type="text/javascript"
src="../../../app/javascript/errors.js"></script>
+ <!-- Pre-requisite -->
+ <script type="text/javascript"
+ src="../subscriber.js"></script>
+
<!-- The module under test -->
<script type="text/javascript"
src="../subscribers_list.js"></script>
=== modified file 'lib/lp/bugs/javascript/tests/test_subscribers_list.js'
--- lib/lp/bugs/javascript/tests/test_subscribers_list.js 2011-04-28 09:26:07 +0000
+++ lib/lp/bugs/javascript/tests/test_subscribers_list.js 2011-04-28 15:09:35 +0000
@@ -1,13 +1,37 @@
YUI({
base: '../../../../canonical/launchpad/icing/yui/',
filter: 'raw', combine: false, fetchCSS: false
- }).use('test', 'console', 'lp.bugs.subscribers_list',
- 'node-event-simulate',
+ }).use('test', 'console', 'lp.bugs.subscriber',
+ 'lp.bugs.subscribers_list', 'node-event-simulate',
function(Y) {
var suite = new Y.Test.Suite("lp.bugs.subscribers_list Tests");
var module = Y.lp.bugs.subscribers_list;
+
+/**
+ * Set-up all the nodes required for subscribers list testing.
+ */
+function setUpSubscribersList(root_node, with_dupes) {
+ // Set-up subscribers list.
+ var direct_links = Y.Node.create('<div></div>')
+ .set('id', 'subscribers-links');
+ var direct_list = Y.Node.create('<div></div>')
+ .set('id', 'subscribers-direct');
+ direct_list.appendChild(direct_links);
+ root_node.appendChild(direct_list);
+
+ if (with_dupes === true) {
+ var dupe_links = Y.Node.create('<div></div>')
+ .set('id', 'subscribers-from-duplicates');
+ var dupe_list = Y.Node.create('<div></div>')
+ .set('id', 'subscribers-from-duplicates-container');
+ dupe_list.appendChild(dupe_links);
+ root_node.appendChild(dupe_list);
+ }
+ return direct_list;
+}
+
/**
* Test resetting of the subscribers list.
*/
@@ -26,15 +50,11 @@
test_no_subscribers: function() {
// There are no subscribers left in the subscribers_list
// (iow, subscribers_links is empty).
- var subscribers_links = Y.Node.create('<div></div>')
- .set('id', 'subscribers-links');
- var subscribers_list = Y.Node.create('<div></div>');
- subscribers_list.appendChild(subscribers_links);
- this.root.appendChild(subscribers_list);
+ var subscribers_list = setUpSubscribersList(this.root);
// Resetting the list adds a 'None' div to the
// subscribers_list (and not to the subscriber_links).
- module.reset();
+ module._reset();
var none_node = subscribers_list.one('#none-subscribers');
Y.Assert.isNotNull(none_node);
Y.Assert.areEqual('None', none_node.get('innerHTML'));
@@ -46,16 +66,13 @@
test_subscribers: function() {
// When there is at least one subscriber, nothing
// happens when reset() is called.
- var subscribers_links = Y.Node.create('<div></div>')
- .set('id', 'subscribers-links');
+ var subscribers_list = setUpSubscribersList(this.root);
+ var subscribers_links = subscribers_list.one('#subscribers-links');
subscribers_links.appendChild(
Y.Node.create('<div>Subscriber</div>'));
- var subscribers_list = Y.Node.create('<div></div>');
- subscribers_list.appendChild(subscribers_links);
- this.root.appendChild(subscribers_list);
// Resetting the list is a no-op.
- module.reset();
+ module._reset();
var none_node = subscribers_list.one('#none-subscribers');
Y.Assert.isNull(none_node);
},
@@ -63,38 +80,24 @@
test_empty_duplicates: function() {
// There are no subscribers among the duplicate subscribers.
- var dupe_subscribers = Y.Node.create('<div></div>')
- .set('id', 'subscribers-from-duplicates');
- this.root.appendChild(dupe_subscribers);
-
- // There must always be subscribers-links node for reset() to work.
- var subscribers_links = Y.Node.create('<div></div>')
- .set('id', 'subscribers-links');
- this.root.appendChild(subscribers_links);
+ var subscribers_list = setUpSubscribersList(this.root, true);
+ var dupe_subscribers = this.root.one('#subscribers-from-duplicates');
// Resetting the list removes the entire duplicate subscribers node.
- module.reset();
- var dupes_node = Y.one('#subscribers-from-duplicates');
- Y.Assert.isNull(dupes_node);
+ module._reset();
+ Y.Assert.isNull(Y.one('#subscribers-from-duplicates'));
},
test_duplicates: function() {
// There are subscribers among the duplicate subscribers,
// and nothing changes.
- var dupe_subscribers = Y.Node.create('<div></div>')
- .set('id', 'subscribers-from-duplicates');
- dupe_subscribers.appendChild(
- Y.Node.create('<div>Subscriber</div>'));
- this.root.appendChild(dupe_subscribers);
-
- // There must always be subscribers-links node for reset() to work.
- var subscribers_links = Y.Node.create('<div></div>')
- .set('id', 'subscribers-links');
- this.root.appendChild(subscribers_links);
+ var subscribers_list = setUpSubscribersList(this.root, true);
+ var dupe_subscribers = this.root.one('#subscribers-from-duplicates');
+ dupe_subscribers.appendChild(Y.Node.create('<div>Subscriber</div>'));
// Resetting the list does nothing.
- module.reset();
+ module._reset();
// The list is still there.
var dupes_node = this.root.one('#subscribers-from-duplicates');
@@ -104,6 +107,227 @@
}));
+/**
+ * Test removal of a single person link from the subscribers list.
+ */
+suite.add(new Y.Test.Case({
+ name: 'Removal of a subscriber link',
+
+ addSubscriber: function(root_node, subscriber, through_dupe) {
+ var css_class = subscriber.get('css_name');
+ var id;
+ if (through_dupe === true) {
+ links = root_node.one('#subscribers-from-duplicates');
+ id = 'dupe-' + css_class;
+ } else {
+ links = root_node.one('#subscribers-links');
+ id = 'direct-' + css_class;
+ }
+ return links.appendChild(
+ Y.Node.create('<div></div>')
+ .addClass(css_class)
+ .set('id', id)
+ .set('text', subscriber.get('uri')));
+ },
+
+ setUp: function() {
+ this.root = Y.Node.create('<div></div>');
+ Y.one('body').appendChild(this.root);
+ this.subscriber_ids = {};
+ },
+
+ tearDown: function() {
+ this.root.remove();
+ delete this.subscriber_ids;
+ },
+
+ test_no_matching_subscriber: function() {
+ // If there is no matching subscriber, removal silently passes.
+
+ // Set-up subscribers list.
+ setUpSubscribersList(this.root);
+
+ var person = new Y.lp.bugs.subscriber.Subscriber({
+ uri: 'myself',
+ subscriber_ids: this.subscriber_ids
+ });
+ var other_person = new Y.lp.bugs.subscriber.Subscriber({
+ uri: 'someone',
+ subscriber_ids: this.subscriber_ids
+ });
+ this.addSubscriber(this.root, other_person);
+
+ module.remove_user_link(person);
+
+ // `other_person` is not removed.
+ Y.Assert.isNotNull(
+ this.root.one('.' + other_person.get('css_name')));
+ },
+
+ test_unsubscribe_icon_removal: function() {
+ // If there is an unsubscribe icon, it gets removed
+ // before animation starts.
+
+ // Set-up subscribers list.
+ setUpSubscribersList(this.root);
+
+ var person = new Y.lp.bugs.subscriber.Subscriber({
+ uri: 'myself',
+ subscriber_ids: this.subscriber_ids
+ });
+ this.addSubscriber(this.root, person);
+ var css_name = person.get('css_name');
+ this.root.one('.' + css_name)
+ .appendChild('<div></div>')
+ .appendChild('<img></img>')
+ .set('id', 'unsubscribe-icon-' + css_name);
+
+ module.remove_user_link(person);
+
+ // Unsubscribe icon is removed immediatelly.
+ Y.Assert.isNull(this.root.one('#unsubscribe-icon-' + css_name));
+ },
+
+ test_direct_subscriber: function() {
+ // If there is a direct subscriber, removal works fine.
+
+ // Set-up subscribers list.
+ setUpSubscribersList(this.root);
+
+ var person = new Y.lp.bugs.subscriber.Subscriber({
+ uri: 'myself',
+ subscriber_ids: this.subscriber_ids
+ });
+ this.addSubscriber(this.root, person);
+
+ module.remove_user_link(person);
+
+ this.wait(function() {
+ // There is no subscriber link anymore.
+ Y.Assert.isNull(this.root.one('.' + person.get('css_name')));
+ // And the reset() call adds the "None" node.
+ Y.Assert.isNotNull(this.root.one('#none-subscribers'));
+ }, 1100);
+ },
+
+ test_direct_subscriber_remove_dupe: function() {
+ // If there is only a direct subscriber, attempting removal of
+ // a duplicate subscription link does nothing.
+
+ // Set-up subscribers list.
+ setUpSubscribersList(this.root);
+
+ var person = new Y.lp.bugs.subscriber.Subscriber({
+ uri: 'myself',
+ subscriber_ids: this.subscriber_ids
+ });
+ this.addSubscriber(this.root, person);
+
+ module.remove_user_link(person, true);
+
+ this.wait(function() {
+ // There is no subscriber link anymore.
+ Y.Assert.isNotNull(this.root.one('.' + person.get('css_name')));
+ }, 1100);
+ },
+
+ test_dupe_subscriber: function() {
+ // If there is a duplicate subscriber, removal works fine.
+
+ // Set-up subscribers list.
+ setUpSubscribersList(this.root, true);
+
+ var person = new Y.lp.bugs.subscriber.Subscriber({
+ uri: 'myself',
+ subscriber_ids: this.subscriber_ids
+ });
+ this.addSubscriber(this.root, person, true);
+
+ module.remove_user_link(person, true);
+
+ this.wait(function() {
+ // There is no subscriber link anymore.
+ Y.Assert.isNull(this.root.one('.' + person.get('css_name')));
+ // And the reset() call cleans up the entire duplicate section.
+ Y.Assert.isNull(this.root.one('#subscribers-from-duplicates'));
+ }, 1100);
+ },
+
+ test_dupe_subscriber_remove_direct: function() {
+ // If there is a duplicate subscriber, trying to remove the
+ // direct subscription user link doesn't do anything.
+
+ // Set-up subscribers list.
+ setUpSubscribersList(this.root, true);
+
+ var person = new Y.lp.bugs.subscriber.Subscriber({
+ uri: 'myself',
+ subscriber_ids: this.subscriber_ids
+ });
+ this.addSubscriber(this.root, person, true);
+
+ module.remove_user_link(person);
+
+ this.wait(function() {
+ // There is no subscriber link anymore.
+ Y.Assert.isNotNull(this.root.one('.' + person.get('css_name')));
+ }, 1100);
+ },
+
+ test_direct_and_dupe_subscriber_remove_dupe: function() {
+ // If there a subscriber is both directly subscribed and
+ // subscribed through duplicate, removal removes only one link.
+
+ // Set-up subscribers list.
+ setUpSubscribersList(this.root, true);
+
+ var person = new Y.lp.bugs.subscriber.Subscriber({
+ uri: 'myself',
+ subscriber_ids: this.subscriber_ids
+ });
+ this.addSubscriber(this.root, person);
+ this.addSubscriber(this.root, person, true);
+
+ // Remove the duplicate subscription link.
+ module.remove_user_link(person, true);
+
+ this.wait(function() {
+ // Remaining entry is the direct subscription one.
+ var nodes = this.root.all('.' + person.get('css_name'));
+ Y.Assert.areEqual(1, nodes.size());
+ Y.Assert.areEqual('direct-' + person.get('css_name'),
+ nodes.pop().get('id'));
+ }, 1100);
+ },
+
+ test_direct_and_dupe_subscriber_remove_direct: function() {
+ // If there a subscriber is both directly subscribed and
+ // subscribed through duplicate, removal removes only one link.
+
+ // Set-up subscribers list.
+ setUpSubscribersList(this.root, true);
+
+ var person = new Y.lp.bugs.subscriber.Subscriber({
+ uri: 'myself',
+ subscriber_ids: this.subscriber_ids
+ });
+ this.addSubscriber(this.root, person);
+ this.addSubscriber(this.root, person, true);
+
+ // Remove the direct subscription link.
+ module.remove_user_link(person);
+
+ this.wait(function() {
+ // Remaining entry is the duplicate subscription one.
+ var nodes = this.root.all('.' + person.get('css_name'));
+ Y.Assert.areEqual(1, nodes.size());
+ Y.Assert.areEqual('dupe-' + person.get('css_name'),
+ nodes.pop().get('id'));
+ }, 1100);
+ }
+}));
+
+
var handle_complete = function(data) {
status_node = Y.Node.create(
'<p id="complete">Test status: complete</p>');
