Merge lp:~sinzui/launchpad/bug-tags-edit-0 into lp:launchpad

[Curtis Hovey]

Change classes, not styles so that action-icons do not show text.

Graphical browsers show the edit icon next to the bug tags, after an
edit is performed, the edit action is redisplayed, except the text
"Edit" is now shown with the icon.

This is caused by the bug tag edit widget (well widget is a bit
presumptuous, more like a collective of behaviour embedded in a page)
which directly manipulates the anchors style instead of changing
classes. The code changes the anchors display from non to inline
(wrong). Instead the code should add or remove the hidden class.

--------------------------------------------------------------------

RULES

    Pre-implementation: no one
    * Update the code to add/remove the hidden class instead of changing
      styles.
    * setStyle() is called on several nodes. Maybe all calls to setStyle()
      should instead be add/removeClass().

QA

    * Visit https://bugs.qastaging.launchpad.net/gdp/+bug/459328
    * Edit the bug tags
    * Verify the edit icon is restored...there is no green "Edit" text
      after it.

LINT

    lib/lp/bugs/javascript/bug_tags_entry.js
    lib/lp/bugs/javascript/tests/test_bug_tags_entry.html
    lib/lp/bugs/javascript/tests/test_bug_tags_entry.js

TEST

    ./bin/test -vvc -t test_bug_tags_entry lp.bugs.tests.test_yui

IMPLEMENTATION

I first removed lint from the module, and changed save_tag() so that it
could be instrumented by mockio. I added a YUI test for the setStyle()
behaviour. I then changed the module to add and remove the .hidden class.
I discovered that when there is a failure, the ok/cancel buttons are not
visible so you cannot resubmit. I fix this issue.
    lib/lp/bugs/javascript/bug_tags_entry.js
    lib/lp/bugs/javascript/tests/test_bug_tags_entry.html
    lib/lp/bugs/javascript/tests/test_bug_tags_entry.

[j.c.sackett (jcsackett)]

Thanks, this looks good.

review: Approve

=== modified file 'lib/lp/bugs/javascript/bug_tags_entry.js'
--- lib/lp/bugs/javascript/bug_tags_entry.js	2012-03-15 05:40:30 +0000
+++ lib/lp/bugs/javascript/bug_tags_entry.js	2012-06-21 14:22:19 +0000
@@ -33,6 +33,7 @@
     ESCAPE = 27,
     TAGS_SHOW = 'tags-show',
     TAGS_HIDE = 'tags-hide';
+    HIDDEN = 'hidden';
 
 /**
  * Grab all existing tags and insert them into the input
@@ -50,7 +51,7 @@
     var tag_list = tags.join(' ');
     /* If there are tags then add a space to the end of the string so the user
        doesn't have to type one. */
-    if (tag_list != "") {
+    if (tag_list !== "") {
         tag_list += ' ';
     }
     tag_input.set(VALUE, tag_list);
@@ -68,21 +69,25 @@
     return tags;
 };
 
+
+namespace.lp_config = {};
+
+
 /**
  * Save the currently entered tags and switch inline editing off.
  *
  * @method save_tags
  */
 var save_tags = function() {
-    var lp_client = new Y.lp.client.Launchpad();
+    var lp_client = new Y.lp.client.Launchpad(namespace.lp_config);
     var tags = namespace.parse_tags(tag_input.get(VALUE));
     var bug = new Y.lp.client.Entry(
         lp_client, LP.cache[BUG], LP.cache[BUG].self_link);
     bug.removeAttr('http_etag');
     bug.set('tags', tags);
-    tags_edit_spinner.setStyle(DISPLAY, INLINE);
-    ok_button.setStyle(DISPLAY, NONE);
-    cancel_button.setStyle(DISPLAY, NONE);
+    tags_edit_spinner.removeClass(HIDDEN);
+    ok_button.addClass(HIDDEN);
+    cancel_button.addClass(HIDDEN);
     bug.lp_save({on : {
         success: function(updated_entry) {
             var official_tags = [];
@@ -106,12 +111,12 @@
                     {tag_url: base_url + tag, tag: tag});
             }).join(' ');
             tag_list_span.set(INNER_HTML, tags_html);
-            tag_input.setStyle(DISPLAY, NONE);
-            tag_list_span.setStyle(DISPLAY, INLINE);
-            ok_button.setStyle(DISPLAY, NONE);
-            cancel_button.setStyle(DISPLAY, NONE);
-            edit_tags_trigger.setStyle(DISPLAY, INLINE);
-            tags_edit_spinner.setStyle(DISPLAY, NONE);
+            tag_input.addClass(HIDDEN);
+            tag_list_span.removeClass(HIDDEN);
+            ok_button.addClass(HIDDEN);
+            cancel_button.addClass(HIDDEN);
+            edit_tags_trigger.removeClass(HIDDEN);
+            tags_edit_spinner.addClass(HIDDEN);
             Y.lp.anim.green_flash({ node: tag_list_span }).run();
             if (Y.Lang.trim(tags_html) === '') {
                 Y.one('#bug-tags').removeClass(TAGS_SHOW);
@@ -121,7 +126,9 @@
             }
         },
         failure: function(id, request) {
-            tags_edit_spinner.setStyle(DISPLAY, NONE);
+            tags_edit_spinner.addClass(HIDDEN);
+            ok_button.removeClass(HIDDEN);
+            cancel_button.removeClass(HIDDEN);
             Y.lp.anim.green_flash({ node: tag_list_span }).run();
         }
     }});
@@ -133,12 +140,12 @@
  * @method cancel
  */
 var cancel = function() {
-    tag_input.setStyle(DISPLAY, NONE);
-    tag_list_span.setStyle(DISPLAY, INLINE);
-    ok_button.setStyle(DISPLAY, NONE);
-    cancel_button.setStyle(DISPLAY, NONE);
-    edit_tags_trigger.setStyle(DISPLAY, INLINE);
-    tags_edit_spinner.setStyle(DISPLAY, NONE);
+    tag_input.addClass(HIDDEN);
+    tag_list_span.removeClass(HIDDEN);
+    ok_button.addClass(HIDDEN);
+    cancel_button.addClass(HIDDEN);
+    edit_tags_trigger.removeClass(HIDDEN);
+    tags_edit_spinner.addClass(HIDDEN);
     autocomplete.hide();
     Y.lp.anim.red_flash({ node: tag_list_span }).run();
     if (Y.Lang.trim(tag_list_span.get('innerHTML')) === '') {
@@ -156,12 +163,12 @@
  */
 var edit = function() {
     populate_tags_input();
-    tag_list_span.setStyle(DISPLAY, NONE);
-    tag_input.setStyle(DISPLAY, INLINE);
+    tag_list_span.addClass(HIDDEN);
+    tag_input.removeClass(HIDDEN);
     tag_input.focus();
-    edit_tags_trigger.setStyle(DISPLAY, NONE);
-    ok_button.setStyle(DISPLAY, INLINE);
-    cancel_button.setStyle(DISPLAY, INLINE);
+    edit_tags_trigger.addClass(HIDDEN);
+    ok_button.removeClass(HIDDEN);
+    cancel_button.removeClass(HIDDEN);
     autocomplete.render();
 };
 
@@ -214,7 +221,7 @@
     });
 
     tag_input.on('keydown', function(e) {
-        if (e.keyCode == ESCAPE) {
+        if (e.keyCode === ESCAPE) {
             e.halt();
             cancel();
         }

=== modified file 'lib/lp/bugs/javascript/tests/test_bug_tags_entry.html'
--- lib/lp/bugs/javascript/tests/test_bug_tags_entry.html	2012-03-14 04:41:36 +0000
+++ lib/lp/bugs/javascript/tests/test_bug_tags_entry.html	2012-06-21 14:22:19 +0000
@@ -31,21 +31,61 @@
           src="../../../../../build/js/lp/app/client.js"></script>
       <script type="text/javascript"
           src="../../../../../build/js/lp/app/testing/mockio.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/anim/anim.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/extras/extras.js"></script>
+      <script type="text/javascript"
+          src="../../../../../build/js/lp/app/autocomplete/autocomplete.js"></script>
 
       <!-- The module under test. -->
       <script type="text/javascript" src="../bug_tags_entry.js"></script>
 
-      <!-- Any css assert for this module. -->
-      <!-- <link rel="stylesheet" href="../assets/bug_tags_entry-core.css" /> -->
-
       <!-- The test suite. -->
       <script type="text/javascript" src="test_bug_tags_entry.js"></script>
 
     </head>
     <body class="yui3-skin-sam">
         <ul id="suites">
-            <!-- <li>lp.large_indicator.test</li> -->
             <li>lp.bug_tags_entry.test</li>
         </ul>
+
+        <div id="fixture"></div>
+
+        <script type="text/x-template" id="bug-tag-form">
+        <!-- Has tags -->
+         <div id="bug-tags" class="tags-show">
+            Tags:
+            <span id="tag-list">
+              <a class="official-tag"
+                  href="/fnord/+bugs?field.tag=project-tag">project-tag</a>
+              <a class="unofficial-tag"
+                  href="/fnord/+bugs?field.tag=user-tag">user-tag</a>
+            </span>
+
+            <form id="tags-form" style="display: inline">
+                <input type="text" id="tag-input" class="hidden" />
+                <img src="/@@/spinner" id="tags-edit-spinner" class="hidden"/>
+                <a href="+edit" title="Edit tags" id="edit-tags-trigger"
+                    class="sprite edit action-icon">Edit</a>
+
+              <button class="lazr-pos lazr-btn yui-ieditor-submit_button hidden"
+                  id="edit-tags-ok" type="button">Ok</button>
+              <button class="lazr-neg lazr-btn yui-ieditor-cancel_button hidden"
+                  id="edit-tags-cancel" type="button">Cancel</button>
+           </form>
+
+           <a href="/+help-bugs/tag-help.html" target="help"
+               class="sprite maybe action-icon">Tag help</a>
+          </div>
+
+          <!-- No tags -->
+          <div id="add-bug-tags" class="tags-hide">
+            <a href="+edit" title="Add tags" id="add-tags-trigger"
+                class="sprite add">Add tags</a>
+            <a href="/+help-bugs/tag-help.html" target="help"
+                class="sprite maybe action-icon">Tag help</a>
+          </div>
+        </script>
     </body>
 </html>

=== modified file 'lib/lp/bugs/javascript/tests/test_bug_tags_entry.js'
--- lib/lp/bugs/javascript/tests/test_bug_tags_entry.js	2012-01-05 18:25:04 +0000
+++ lib/lp/bugs/javascript/tests/test_bug_tags_entry.js	2012-06-21 14:22:19 +0000
@@ -40,5 +40,101 @@
 
       }));
 
+    suite.add(new Y.Test.Case({
+        name: 'Actions',
+
+        setUp: function() {
+            this.fixture = Y.one("#fixture");
+            var template = Y.one('#bug-tag-form').getContent();
+            this.fixture.append(template);
+            this.edit_tags_trigger = Y.one('#edit-tags-trigger');
+            this.add_tags_trigger = Y.one('#add-tags-trigger');
+            this.tag_list_span = Y.one('#tag-list');
+            this.tag_input = Y.one('#tag-input');
+            this.ok_button = Y.one('#edit-tags-ok');
+            this.cancel_button = Y.one('#edit-tags-cancel');
+            this.tags_edit_spinner = Y.one('#tags-edit-spinner');
+            window.LP = {
+                links: {me : "/~user"},
+                cache: {
+                    bug: {
+                        resource_type_link: 'Bug',
+                        self_link: '/bug/1',
+                        tags: ['project-tag']}
+                    }
+                };
+        },
+
+        tearDown: function () {
+            if (this.fixture !== null) {
+                this.fixture.empty();
+            }
+            delete this.fixture;
+            delete window.LP;
+        },
+
+        test_edit: function () {
+            module.setup_tag_entry(['project-tag']);
+            this.edit_tags_trigger.simulate('click');
+            Y.Assert.isTrue(this.tag_list_span.hasClass('hidden'));
+            Y.Assert.isTrue(this.edit_tags_trigger.hasClass('hidden'));
+            Y.Assert.isFalse(this.tag_input.hasClass('hidden'));
+            Y.Assert.isFalse(this.ok_button.hasClass('hidden'));
+            Y.Assert.isFalse(this.cancel_button.hasClass('hidden'));
+        },
+
+        test_cancel: function () {
+            module.setup_tag_entry(['project-tag']);
+            this.edit_tags_trigger.simulate('click');
+            this.cancel_button.simulate('click');
+            Y.Assert.isFalse(this.tag_list_span.hasClass('hidden'));
+            Y.Assert.isFalse(this.edit_tags_trigger.hasClass('hidden'));
+            Y.Assert.isTrue(this.tag_input.hasClass('hidden'));
+            Y.Assert.isTrue(this.ok_button.hasClass('hidden'));
+            Y.Assert.isTrue(this.cancel_button.hasClass('hidden'));
+            Y.Assert.isTrue(this.tags_edit_spinner.hasClass('hidden'));
+        },
+
+        test_save_tags_success: function () {
+            module.setup_tag_entry(['project-tag']);
+            this.edit_tags_trigger.simulate('click');
+            var mockio = new Y.lp.testing.mockio.MockIo();
+            module.lp_config = {io_provider: mockio};
+            this.ok_button.simulate('click');
+            Y.Assert.isFalse(this.tags_edit_spinner.hasClass('hidden'));
+            Y.Assert.areEqual(
+                '/api/devel/bug/1', mockio.last_request.url);
+            mockio.success({
+                responseText: Y.JSON.stringify({
+                    resource_type_link: 'Bug',
+                    self_link: '/bug/1',
+                    tags: ['project-tag']}),
+                responseHeaders: {'Content-Type': 'application/json'}});
+            Y.Assert.isFalse(this.tag_list_span.hasClass('hidden'));
+            Y.Assert.isFalse(this.edit_tags_trigger.hasClass('hidden'));
+            Y.Assert.isTrue(this.tag_input.hasClass('hidden'));
+            Y.Assert.isTrue(this.ok_button.hasClass('hidden'));
+            Y.Assert.isTrue(this.cancel_button.hasClass('hidden'));
+            Y.Assert.isTrue(this.tags_edit_spinner.hasClass('hidden'));
+        },
+
+        test_save_tags_failure: function () {
+            module.setup_tag_entry(['project-tag']);
+            this.edit_tags_trigger.simulate('click');
+            var mockio = new Y.lp.testing.mockio.MockIo();
+            module.lp_config = {io_provider: mockio};
+            this.ok_button.simulate('click');
+            mockio.failure({
+                responseText: Y.JSON.stringify({
+                    resource_type_link: 'Bug',
+                    self_link: '/bug/1',
+                    tags: ['project-tag']}),
+                responseHeaders: {'Content-Type': 'application/json'}});
+            Y.Assert.isFalse(this.ok_button.hasClass('hidden'));
+            Y.Assert.isFalse(this.cancel_button.hasClass('hidden'));
+            Y.Assert.isTrue(this.tags_edit_spinner.hasClass('hidden'));
+        }
+    }));
+
     Y.lp.testing.Runner.run(suite);
 });