3411: code review

Feedback from code review:

* Dropped unnecessary lambda from Collection.default_scope.
* Set owner_uuid of new collection test fixtures to user 'active',
  removed unnecessary permission link test fixtures.
* Removed permit_unsigned_manifests from unit tests
* Added tests for :show and :update to expired and unexpired
  collections.
* Added rake config:check test to make sure default_trash_lifetime is at
  least 24 hours.

diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index d7cd9a2802f2d5dec60585ec084e70088ac08953..accd2cc62c7bc049518481efdcbf49db592f325a 100644 (file)
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -11,7 +11,7 @@ class Collection < ArvadosModel
   validate :ensure_hash_matches_manifest_text
 
   # Query only undeleted collections by default.
-  default_scope { where("expires_at IS NULL or expires_at > CURRENT_TIMESTAMP") }
+  default_scope where("expires_at IS NULL or expires_at > CURRENT_TIMESTAMP")
 
   api_accessible :user, extend: :common do |t|
     t.add :name

diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 8521454e7a85a72d572373afc4d84cacbcdfeeec..bd69102992a44e30d904283c0195baaf39228289 100644 (file)
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -681,9 +681,9 @@ CREATE TABLE pipeline_instances (
     properties text,
     state character varying(255),
     components_summary text,
-    description text,
     started_at timestamp without time zone,
-    finished_at timestamp without time zone
+    finished_at timestamp without time zone,
+    description text
 );
 
 
@@ -2018,4 +2018,4 @@ INSERT INTO schema_migrations (version) VALUES ('20140828141043');
 
 INSERT INTO schema_migrations (version) VALUES ('20140909183946');
 
-INSERT INTO schema_migrations (version) VALUES ('20140911221252');
+INSERT INTO schema_migrations (version) VALUES ('20140911221252');
\ No newline at end of file

diff --git a/services/api/lib/tasks/config_check.rake b/services/api/lib/tasks/config_check.rake
index ec1ae7bdc478a08cc172eca7c30ffd7eecda1cba..c42110144261dc23b6a6dbaa6c2a65a9aa5a68e2 100644 (file)
--- a/services/api/lib/tasks/config_check.rake
+++ b/services/api/lib/tasks/config_check.rake
@@ -15,5 +15,9 @@ namespace :config do
         end
       end
     end
+    # default_trash_lifetime cannot be less than 24 hours
+    if $application_config['default_trash_lifetime'] < 86400 then
+      raise "default_trash_lifetime is %d, must be at least 86400" % $application_config['default_trash_lifetime']
+    end
   end
 end

diff --git a/services/api/test/fixtures/collections.yml b/services/api/test/fixtures/collections.yml
index 1a4a9ab924172a7032f0cdb33a52da916582c20d..18531174e0937991f17abbbc8bc43fe98dc90480 100644 (file)
--- a/services/api/test/fixtures/collections.yml
+++ b/services/api/test/fixtures/collections.yml
@@ -177,7 +177,7 @@ collection_to_move_around_in_aproject:
 expired_collection:
   uuid: zzzzz-4zz18-mto52zx1s7sn3ih
   portable_data_hash: 0b21a217243bfce5617fb9224b95bcb9+49
-  owner_uuid: zzzzz-tpzed-000000000000000
+  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
@@ -190,7 +190,7 @@ expired_collection:
 collection_expires_in_future:
   uuid: zzzzz-4zz18-padkqo7yb8d9i3j
   portable_data_hash: 0b21a217243bfce5617fb9224b95bcb9+49
-  owner_uuid: zzzzz-tpzed-000000000000000
+  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

diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml
index bd69b36c7bdfb97ff661c7c5937d22b21a97a962..28dbf014d1f496ec628e6194f8d3b3909eed6c96 100644 (file)
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@ -222,34 +222,6 @@ baz_file_publicly_readable:
   head_uuid: zzzzz-4zz18-y9vne9npefyxh8g
   properties: {}
 
-expired_collection_readable_by_active:
-  uuid: zzzzz-o0j2j-e3cq2eg21va67ud
-  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-xurymjxw79nv3jz
-  link_class: permission
-  name: can_read
-  head_uuid: zzzzz-4zz18-mto52zx1s7sn3ih
-  properties: {}
-
-unexpired_collection_readable_by_active:
-  uuid: zzzzz-o0j2j-4undx2cussodkkg
-  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-xurymjxw79nv3jz
-  link_class: permission
-  name: can_read
-  head_uuid: zzzzz-4zz18-padkqo7yb8d9i3j
-  properties: {}
-
 barbaz_job_readable_by_spectator:
   uuid: zzzzz-o0j2j-cpy7p41hpk531e1
   owner_uuid: zzzzz-tpzed-000000000000000

diff --git a/services/api/test/functional/arvados/v1/collections_controller_test.rb b/services/api/test/functional/arvados/v1/collections_controller_test.rb
index 18cbafc6898bd90d49925fd17fe047fda7034793..81e77cd9f5f68d50921b4137383d63ae0552bee1 100644 (file)
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -560,7 +560,6 @@ EOS
   end
 
   test 'Expired collections are not returned' do
-    permit_unsigned_manifests
     authorize_with :active
     get :index, {
       where: {name: 'expired_collection'},
@@ -568,15 +567,40 @@ EOS
     assert_response :success
     found = assigns(:objects)
     assert_equal 0, found.count
+
+    get :show, {
+      id: 'zzzzz-4zz18-mto52zx1s7sn3ih',
+    }
+    assert_response 404
+
+    post :update, {
+      id: 'zzzzz-4zz18-mto52zx1s7sn3ih',
+      collection: {
+        name: "still expired"
+      }
+    }
+    assert_response 404
   end
 
   test 'Collection with future expiration time is returned' do
-    permit_unsigned_manifests
     authorize_with :active
     get :index, {
       where: {name: 'collection_expires_in_future'},
     }
     found = assigns(:objects)
     assert_equal 1, found.count
+
+    get :show, {
+      id: 'zzzzz-4zz18-padkqo7yb8d9i3j',
+    }
+    assert_success
+
+    post :update, {
+      id: 'zzzzz-4zz18-padkqo7yb8d9i3j',
+      collection: {
+        name: "still not expired"
+      }
+    }
+    assert_success
   end
 end