From: radhika <radhika@curoverse.com>
Date: Tue, 20 Jun 2017 23:00:32 +0000 (-0400)
Subject: 9426: test Tags tab
X-Git-Tag: 1.1.0~176^2
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/b6244435a4470fbddc9f13edb4329d563e579e50

9426: test Tags tab

Arvados-DCO-1.1-Signed-off-by: Radhika Chippada <radhika@curoverse.com>
---

diff --git a/apps/workbench/app/assets/javascripts/edit_collection_tags.js b/apps/workbench/app/assets/javascripts/edit_collection_tags.js
index 06cd64b00a..dc8c0330cc 100644
--- a/apps/workbench/app/assets/javascripts/edit_collection_tags.js
+++ b/apps/workbench/app/assets/javascripts/edit_collection_tags.js
@@ -1,6 +1,5 @@
-// On loading of a collection, enable the "lock" button and
-// disable all file modification controls (upload, rename, delete)
-$(document).
+jQuery(function($){
+  $(document).
     on('click', '.collection-tag-save, .collection-tag-cancel', function(event) {
         $('.edit-collection-tags').removeClass('disabled');
         $('#edit-collection-tags').attr("title", "Edit tags");
@@ -20,46 +19,50 @@ $(document).
         $('.collection-tag-field').prop("contenteditable", true);
         $('div').remove('.collection-tags-status-label');
     }).
-    on('click', '.collection-tag-save', function(e){
+    on('click', '.collection-tag-save', function(event) {
       var tag_data = {};
+      var has_tags = false;
+
       var $tags = $(".collection-tags-table");
       $tags.find('tr').each(function (i, el) {
         var $tds = $(this).find('td');
         var $key = $tds.eq(1).text();
         if ($key && $key.trim().length > 0) {
+          has_tags = true;
           tag_data[$key.trim()] = $tds.eq(2).text().trim();
         }
       });
 
-      if(jQuery.isEmptyObject(tag_data)){
-        tag_data["empty"]=true
+      var to_send;
+      if (has_tags == false) {
+        to_send = {tag_data: "empty"}
       } else {
-        tag_data = {tag_data}
+        to_send = {tag_data: tag_data}
       }
 
       $.ajax($(location).attr('pathname')+'/save_tags', {
           type: 'POST',
-          data: tag_data
+          data: to_send
       }).success(function(data, status, jqxhr) {
         $('.collection-tags-status').append('<div class="collection-tags-status-label alert alert-success"><p class="contain-align-left">Saved successfully.</p></div>');
       }).fail(function(jqxhr, status, error) {
         $('.collection-tags-status').append('<div class="collection-tags-status-label alert alert-danger"><p class="contain-align-left">We are sorry. There was an error saving tags. Please try again.</p></div>');
       });
     }).
-    on('click', '.collection-tag-cancel', function(e){
+    on('click', '.collection-tag-cancel', function(event) {
       $.ajax($(location).attr('pathname')+'/tags', {
           type: 'GET'
       });
+    }).
+    on('click', '.collection-tag-remove', function(event) {
+      $(this).parents('tr').detach();
+    }).
+    on('click', '.collection-tag-add', function(event) {
+      var $collection_tags = $(this).closest('.collection-tags-container');
+      var $clone = $collection_tags.find('tr.hide').clone(true).removeClass('hide');
+      $collection_tags.find('table').append($clone);
+    }).
+    on('keypress', '.collection-tag-field', function(event){
+      return event.which != 13;
     });
-
-jQuery(function($){
-  $(document).on('click', '.collection-tag-remove', function(e) {
-    $(this).parents('tr').detach();
-  });
-
-  $(document).on('click', '.collection-tag-add', function(e) {
-    var $collection_tags = $(this).closest('.collection-tags-container');
-    var $clone = $collection_tags.find('tr.hide').clone(true).removeClass('hide');
-    $collection_tags.find('table').append($clone);
-  });
 });
diff --git a/apps/workbench/app/assets/stylesheets/collections.css.scss b/apps/workbench/app/assets/stylesheets/collections.css.scss
index 2d2d0f25d1..dadf4ea957 100644
--- a/apps/workbench/app/assets/stylesheets/collections.css.scss
+++ b/apps/workbench/app/assets/stylesheets/collections.css.scss
@@ -70,3 +70,7 @@ $active-bg: #39b3d7;
     padding: .5em 2em;
     margin: 0 1em;
 }
+
+.collection-tag-field * {
+  display: inline-block;
+}
diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index bfba2f57f8..99399bc9c2 100644
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -350,11 +350,13 @@ class CollectionsController < ApplicationController
   end
 
   def save_tags
-    tags = nil
-    if params['tag_data']
-      tags = params['tag_data']
-    elsif params['empty']
-      tags = {}
+    tags_param = params['tag_data']
+    if tags_param
+      if tags_param.is_a?(String) && tags_param == "empty"
+        tags = {}
+      else
+        tags = tags_param
+      end
     end
 
     if tags
@@ -362,6 +364,8 @@ class CollectionsController < ApplicationController
       props[:tags] = tags
 
       if @object.update_attributes properties: props
+        @saved_tags = true
+        render
       else
         self.render_error status: 422
       end
diff --git a/apps/workbench/app/views/collections/_show_tag_rows.html.erb b/apps/workbench/app/views/collections/_show_tag_rows.html.erb
index da699256e9..eceec1057b 100644
--- a/apps/workbench/app/views/collections/_show_tag_rows.html.erb
+++ b/apps/workbench/app/views/collections/_show_tag_rows.html.erb
@@ -1,26 +1,31 @@
 <%
   tags = object.properties[:tags]
 %>
-
-        <% tags.andand.each do |k, v| %>
+      <% if tags.andand.is_a?(Hash) %>
+        <% tags.each do |k, v| %>
           <tr class="collection-tag-<%=k%>">
             <td>
-              <i class="glyphicon glyphicon-remove collection-tag-remove hide" style="cursor: pointer;"></i>
+              <% if object.editable? %>
+                <i class="glyphicon glyphicon-remove collection-tag-remove hide" style="cursor: pointer;"></i>
+              <% end %>
             </td>
-            <td class="collection-tag-field">
+            <td class="collection-tag-field collection-tag-field-key">
               <%= k %>
             </td>
-            <td class="collection-tag-field">
+            <td class="collection-tag-field collection-tag-field-value">
               <%= v %>
             </td>
           </tr>
         <% end %>
+      <% end %>
 
+      <% if @object.editable? %>
         <!-- A hidden row to add new tag -->
         <tr class="collection-tag-hidden hide">
           <td>
             <i class="glyphicon glyphicon-remove collection-tag-remove hide" style="cursor: pointer"></i>
           </td>
-          <td class="collection-tag-field"></td>
-          <td class="collection-tag-field"></td>
+          <td class="collection-tag-field collection-tag-field-key"></td>
+          <td class="collection-tag-field collection-tag-field-value"></td>
         </tr>
+      <% end %>
diff --git a/apps/workbench/app/views/collections/_show_tags.html.erb b/apps/workbench/app/views/collections/_show_tags.html.erb
index 4ffe7ff5f9..a1f58ae7b0 100644
--- a/apps/workbench/app/views/collections/_show_tags.html.erb
+++ b/apps/workbench/app/views/collections/_show_tags.html.erb
@@ -5,7 +5,7 @@
   <div class="collection-tags-container" style="padding-left:2em;padding-right:2em;">
     <% if object.editable? %>
       <p title="Edit tags" id="edit-collection-tags">
-        <button type="button" class="btn btn-primary edit-collection-tags">Edit</button>
+        <a type="button" class="btn btn-primary edit-collection-tags">Edit</a>
       </p>
     <% end %>
 
@@ -31,7 +31,7 @@
     <div>
       <% if object.editable? %>
         <div class="pull-left">
-          <button class="btn btn-primary btn-sm collection-tag-add hide"><i class="glyphicon glyphicon-plus"></i> Add new tag </button>
+          <a class="btn btn-primary btn-sm collection-tag-add hide"><i class="glyphicon glyphicon-plus"></i> Add new tag </a>
         </div>
         <div class="pull-right">
           <%= link_to(save_tags_collection_path, {class: 'btn btn-sm btn-primary collection-tag-save hide', :remote => true, method: 'post', return_to: request.url}) do %>
diff --git a/apps/workbench/app/views/collections/save_tags.js.erb b/apps/workbench/app/views/collections/save_tags.js.erb
index e2faff169a..5fc84d727d 100644
--- a/apps/workbench/app/views/collections/save_tags.js.erb
+++ b/apps/workbench/app/views/collections/save_tags.js.erb
@@ -1 +1,3 @@
+<% if @saved_tags %>
 $(".collection-tag-rows").html("<%= escape_javascript(render partial: 'show_tag_rows', locals: {object: @object}) %>");
+<% end %>
diff --git a/apps/workbench/test/controllers/collections_controller_test.rb b/apps/workbench/test/controllers/collections_controller_test.rb
index 7ff123cd45..63e1ae3972 100644
--- a/apps/workbench/test/controllers/collections_controller_test.rb
+++ b/apps/workbench/test/controllers/collections_controller_test.rb
@@ -776,4 +776,67 @@ class CollectionsControllerTest < ActionController::TestCase
     assert_response 422
     assert_includes json_response['errors'], 'Duplicate file path'
   end
+
+  [
+    [:active, true],
+    [:spectator, false],
+  ].each do |user, editable|
+    test "tags tab #{editable ? 'shows' : 'does not show'} edit button to #{user}" do
+      use_token user
+
+      get :tags, {
+        id: api_fixture('collections')['collection_with_tags_owned_by_active']['uuid'],
+        format: :js,
+      }, session_for(user)
+
+      assert_response :success
+
+      found = 0
+      response.body.scan /<i[^>]+>/ do |remove_icon|
+        remove_icon.scan(/\ collection-tag-remove(.*?)\"/).each do |i,|
+          found += 1
+        end
+      end
+
+      if editable
+        assert_equal(3, found)  # two from the tags + 1 from the hidden "add tag" row
+      else
+        assert_equal(0, found)
+      end
+    end
+  end
+
+  test "save_tags and verify that 'other' properties are retained" do
+    use_token :active
+
+    collection = api_fixture('collections')['collection_with_tags_owned_by_active']
+
+    new_tags = {"new_tag1" => "new_tag1_value",
+                "new_tag2" => "new_tag2_value"}
+
+    post :save_tags, {
+      id: collection['uuid'],
+      tag_data: new_tags,
+      format: :js,
+    }, session_for(:active)
+
+    assert_response :success
+    assert_equal true, response.body.include?("new_tag1")
+    assert_equal true, response.body.include?("new_tag1_value")
+    assert_equal true, response.body.include?("new_tag2")
+    assert_equal true, response.body.include?("new_tag2_value")
+    assert_equal false, response.body.include?("existing tag 1")
+    assert_equal false, response.body.include?("value for existing tag 1")
+
+    updated_properties = Collection.find(collection['uuid']).properties
+    updated_tags = updated_properties[:tags]
+    assert_equal true, updated_tags.keys.include?(:'new_tag1')
+    assert_equal new_tags['new_tag1'], updated_tags[:'new_tag1']
+    assert_equal true, updated_tags.keys.include?(:'new_tag2')
+    assert_equal new_tags['new_tag2'], updated_tags[:'new_tag2']
+    assert_equal false, updated_tags.keys.include?(:'existing tag 1')
+    assert_equal false, updated_tags.keys.include?(:'existing tag 2')
+    assert_equal true, updated_properties.keys.include?(:'some other property')
+    assert_equal 'value for the other property', updated_properties[:'some other property']
+  end
 end
diff --git a/apps/workbench/test/integration/collections_test.rb b/apps/workbench/test/integration/collections_test.rb
index 8b43e5dbe3..dbdcef8fbe 100644
--- a/apps/workbench/test/integration/collections_test.rb
+++ b/apps/workbench/test/integration/collections_test.rb
@@ -420,4 +420,92 @@ class CollectionsTest < ActionDispatch::IntegrationTest
     first('.lock-collection-btn').click
     accept_alert
   end
+
+  test "collection tags tab" do
+    need_selenium
+
+    visit page_with_token('active', '/collections/zzzzz-4zz18-bv31uwvy3neko21')
+
+    click_link 'Tags'
+    wait_for_ajax
+
+    # verify initial state
+    assert_selector 'a', text: 'Edit'
+    assert_no_selector 'a', text: 'Add new tag'
+    assert_no_selector 'a', text: 'Save'
+    assert_no_selector 'a', text: 'Cancel'
+
+    # Verify controls in edit mode
+    first('.edit-collection-tags').click
+    assert_selector 'a.disabled', text: 'Edit'
+    assert_selector 'a', text: 'Add new tag'
+    assert_selector 'a', text: 'Save'
+    assert_selector 'a', text: 'Cancel'
+
+    # add two tags
+    first('.edit-collection-tags').click
+
+    first('.glyphicon-plus').click
+    first('.collection-tag-field-key').click
+    first('.collection-tag-field-key').set('key 1')
+    first('.collection-tag-field-value').click
+    first('.collection-tag-field-value').set('value 1')
+
+    first('.glyphicon-plus').click
+    editable_key_fields = page.all('.collection-tag-field-key')
+    editable_key_fields[1].click
+    editable_key_fields[1].set('key 2')
+    editable_val_fields = page.all('.collection-tag-field-value')
+    editable_val_fields[1].click
+    editable_val_fields[1].set('value 2')
+
+    click_on 'Save'
+    wait_for_ajax
+
+    # added tags; verify
+    assert_text 'key 1'
+    assert_text 'value 1'
+    assert_text 'key 2'
+    assert_text 'value 2'
+    assert_selector 'a', text: 'Edit'
+    assert_no_selector 'a', text: 'Save'
+
+    # remove first tag
+    first('.edit-collection-tags').click
+    assert_not_nil first('.glyphicon-remove')
+    first('.glyphicon-remove').click
+    click_on 'Save'
+    wait_for_ajax
+
+    assert_text 'key 2'
+    assert_text 'value 2'
+    assert_no_text 'key 1'
+    assert_no_text 'value 1'
+    assert_selector 'a', text: 'Edit'
+
+    # Click on cancel and verify
+    first('.edit-collection-tags').click
+    first('.collection-tag-field-key').click
+    first('.collection-tag-field-key').set('this key wont stick')
+    first('.collection-tag-field-value').click
+    first('.collection-tag-field-value').set('this value wont stick')
+
+    click_on 'Cancel'
+    wait_for_ajax
+
+    assert_text 'key 2'
+    assert_text 'value 2'
+    assert_no_text 'this key wont stick'
+    assert_no_text 'this value wont stick'
+
+    # remove all tags
+    first('.edit-collection-tags').click
+    first('.glyphicon-remove').click
+    click_on 'Save'
+    wait_for_ajax
+
+    assert_selector 'a', text: 'Edit'
+    assert_no_text 'key 2'
+    assert_no_text 'value 2'
+  end
 end
diff --git a/services/api/test/fixtures/collections.yml b/services/api/test/fixtures/collections.yml
index 8aedbdc705..6543f5422a 100644
--- a/services/api/test/fixtures/collections.yml
+++ b/services/api/test/fixtures/collections.yml
@@ -657,6 +657,23 @@ collection_to_remove_and_rename_files:
   manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:file1 0:0:file2\n"
   name: collection to remove and rename files
 
+collection_with_tags_owned_by_active:
+  uuid: zzzzz-4zz18-taggedcolletion
+  portable_data_hash: fa7aeb5140e2848d39b416daeef4ffc5+45
+  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  created_at: 2014-02-03T17:22:54Z
+  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+  modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
+  modified_at: 2014-02-03T17:22:54Z
+  updated_at: 2014-02-03T17:22:54Z
+  manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:bar\n"
+  name: collection with tags
+  properties:
+    tags:
+      existing tag 1: value for existing tag 1
+      existing tag 2: value for existing tag 2
+    some other property: value for the other property
+
 
 # Test Helper trims the rest of the file
 
diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml
index ac579f7a97..2c65483609 100644
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@ -1056,3 +1056,17 @@ star_shared_project_for_project_viewer:
   name: zzzzz-j7d0g-starredshared01
   head_uuid: zzzzz-j7d0g-starredshared01
   properties: {}
+
+tagged_collection_readable_by_spectator:
+  uuid: zzzzz-o0j2j-readacl4tagcoll
+  owner_uuid: zzzzz-tpzed-000000000000000
+  created_at: 2014-01-24 20:42:26 -0800
+  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+  modified_by_user_uuid: zzzzz-tpzed-000000000000000
+  modified_at: 2014-01-24 20:42:26 -0800
+  updated_at: 2014-01-24 20:42:26 -0800
+  tail_uuid: zzzzz-tpzed-l1s2piq4t4mps8r
+  link_class: permission
+  name: can_read
+  head_uuid: zzzzz-4zz18-taggedcolletion
+  properties: {}
diff --git a/services/api/test/functional/arvados/v1/groups_controller_test.rb b/services/api/test/functional/arvados/v1/groups_controller_test.rb
index 3beec35958..7e3cad7d8c 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -116,6 +116,7 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
     authorize_with :active
     get :contents, {
       format: :json,
+      limit: 200,
       id: users(:active).uuid
     }
     assert_response :success