[ARVADOS] updated: 1.1.3-87-gafee3c9

[Git user]

Summary of changes:
 services/api/app/models/collection.rb                | 19 ++++++++++---------
 ...80216203422_add_storage_classes_to_collections.rb |  4 ++--
 services/api/db/structure.sql                        |  4 ++--
 services/api/test/unit/collection_test.rb            | 20 +++++++++++++++++++-
 4 files changed, 33 insertions(+), 14 deletions(-)

       via  afee3c9d8851d3f337cb63bfb01174b96e039ff2 (commit)
      from  56c1d14687b53bdb6e3ca1a9d3ff50638ff9af5b (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit afee3c9d8851d3f337cb63bfb01174b96e039ff2
Author: Lucas Di Pentima <ldipentima at veritasgenetics.com>
Date:   Fri Feb 23 18:23:01 2018 -0300

    12707: Fixes and tests on storage classes columns
    
    Re-added default values on migration, removed the default setting on the model
    and added checks and tests for empty string values.
    
    Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <ldipentima at veritasgenetics.com>

diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 6018a27..221176e 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -19,13 +19,13 @@ class Collection < ArvadosModel
   serialize :storage_classes_confirmed, Array
 
   before_validation :default_empty_manifest
-  before_validation :default_storage_classes, on: :create
   before_validation :check_encoding
   before_validation :check_manifest_validity
   before_validation :check_signatures
   before_validation :strip_signatures_and_update_replication_confirmed
   validate :ensure_pdh_matches_manifest_text
   validate :ensure_storage_classes_desired_is_not_empty
+  validate :ensure_storage_classes_contain_non_empty_strings
   before_save :set_file_names
 
   api_accessible :user, extend: :common do |t|
@@ -453,13 +453,6 @@ class Collection < ArvadosModel
 
   protected
 
-  def default_storage_classes
-    if self.storage_classes_desired.nil? || self.storage_classes_desired.empty?
-      self.storage_classes_desired = ["default"]
-    end
-    self.storage_classes_confirmed ||= []
-  end
-
   def portable_manifest_text
     self.class.munge_manifest_locators(manifest_text) do |match|
       if match[2] # size
@@ -502,7 +495,15 @@ class Collection < ArvadosModel
 
   def ensure_storage_classes_desired_is_not_empty
     if self.storage_classes_desired.empty?
-      raise ArvadosModel::PermissionDeniedError.new("storage_classes_desired shouldn't be empty")
+      raise ArvadosModel::InvalidStateTransitionError.new("storage_classes_desired shouldn't be empty")
+    end
+  end
+
+  def ensure_storage_classes_contain_non_empty_strings
+    (self.storage_classes_desired + self.storage_classes_confirmed).each do |c|
+      if !c.is_a?(String) || c == ''
+        raise ArvadosModel::InvalidStateTransitionError.new("storage classes should only be non-empty strings")
+      end
     end
   end
 end
diff --git a/services/api/db/migrate/20180216203422_add_storage_classes_to_collections.rb b/services/api/db/migrate/20180216203422_add_storage_classes_to_collections.rb
index e1bda82..112c2ba 100644
--- a/services/api/db/migrate/20180216203422_add_storage_classes_to_collections.rb
+++ b/services/api/db/migrate/20180216203422_add_storage_classes_to_collections.rb
@@ -4,8 +4,8 @@
 
 class AddStorageClassesToCollections < ActiveRecord::Migration
   def up
-    add_column :collections, :storage_classes_desired, :jsonb
-    add_column :collections, :storage_classes_confirmed, :jsonb
+    add_column :collections, :storage_classes_desired, :jsonb, :default => ["default"]
+    add_column :collections, :storage_classes_confirmed, :jsonb, :default => []
     add_column :collections, :storage_classes_confirmed_at, :datetime, :default => nil, :null => true
   end
 
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 5b267e7..357e95c 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -171,8 +171,8 @@ CREATE TABLE collections (
     file_names character varying(8192),
     trash_at timestamp without time zone,
     is_trashed boolean DEFAULT false NOT NULL,
-    storage_classes_desired jsonb,
-    storage_classes_confirmed jsonb,
+    storage_classes_desired jsonb DEFAULT '["default"]'::jsonb,
+    storage_classes_confirmed jsonb DEFAULT '[]'::jsonb,
     storage_classes_confirmed_at timestamp without time zone
 );
 
diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index 9c4c758..d425bc6 100644
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -226,12 +226,30 @@ class CollectionTest < ActiveSupport::TestCase
       c = collections(:collection_owned_by_active)
       c.update_attributes storage_classes_desired: ["hot"]
       assert_equal ["hot"], c.storage_classes_desired
-      assert_raise ArvadosModel::PermissionDeniedError do
+      assert_raise ArvadosModel::InvalidStateTransitionError do
         c.update_attributes storage_classes_desired: []
       end
     end
   end
 
+  test "storage classes lists should only contain non-empty strings" do
+    c = collections(:storage_classes_desired_default_unconfirmed)
+    act_as_user users(:admin) do
+      assert c.update_attributes(storage_classes_desired: ["default", "a_string"],
+                                 storage_classes_confirmed: ["another_string"])
+      [
+        ["storage_classes_desired", ["default", 42]],
+        ["storage_classes_confirmed", [{the_answer: 42}]],
+        ["storage_classes_desired", ["default", ""]],
+        ["storage_classes_confirmed", [""]],
+      ].each do |attr, val|
+        assert_raise ArvadosModel::InvalidStateTransitionError do
+          assert c.update_attributes({attr => val})
+        end
+      end
+    end
+  end
+
   test "storage_classes_confirmed* can be set by admin user" do
     c = collections(:storage_classes_desired_default_unconfirmed)
     act_as_user users(:admin) do

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


hooks/post-receive
--