From 647511030800d228feb6955dfab9cb0a26cbfcfb Mon Sep 17 00:00:00 2001 From: Lucas Di Pentima Date: Mon, 1 Oct 2018 14:32:12 -0300 Subject: [PATCH] 13561: Old collection version's modified_at reflects version's create time. Don't update this field when syncing past versions' fields with selected current updates. Also, keep created_at the same as the current version when snapshotting. Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima --- services/api/app/models/arvados_model.rb | 4 +-- services/api/app/models/collection.rb | 7 ++-- services/api/test/unit/collection_test.rb | 39 +++++++++++++++++++++-- 3 files changed, 43 insertions(+), 7 deletions(-) diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index c67a3961d9..4ba4ac7710 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -537,12 +537,12 @@ class ArvadosModel < ActiveRecord::Base def update_modified_by_fields current_time = db_current_time - self.created_at = created_at_was || current_time + self.created_at ||= created_at_was || current_time self.updated_at = current_time self.owner_uuid ||= current_default_owner if self.respond_to? :owner_uuid= - self.modified_at = current_time if !anonymous_updater self.modified_by_user_uuid = current_user ? current_user.uuid : nil + self.modified_at = current_time end self.modified_by_client_uuid = current_api_client ? current_api_client.uuid : nil true diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb index f051fb3445..025811cf8a 100644 --- a/services/api/app/models/collection.rb +++ b/services/api/app/models/collection.rb @@ -220,7 +220,7 @@ class Collection < ArvadosModel self.current_version_uuid ||= self.uuid end - def save! + def save! *args if !Rails.configuration.collection_versioning || new_record? || (!self.changes.include?('uuid') && current_version_uuid != uuid) return super end @@ -256,7 +256,9 @@ class Collection < ArvadosModel c.attributes = updates # Use a different validation context to skip the 'old_versions_cannot_be_updated' # validator, as on this case it is legal to update some fields. - c.save(context: :update_old_versions) + leave_modified_by_user_alone do + c.save(context: :update_old_versions) + end end # Also update current object just in case a new version will be created, # as it has to receive the same values for the synced attributes. @@ -268,6 +270,7 @@ class Collection < ArvadosModel # Create a snapshot of the original collection snapshot = self.dup snapshot.uuid = nil # Reset UUID so it's created as a new record + snapshot.created_at = created_at # Update current version number self.version += 1 end diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb index 31c76efa3e..569e589b29 100644 --- a/services/api/test/unit/collection_test.rb +++ b/services/api/test/unit/collection_test.rb @@ -128,9 +128,42 @@ class CollectionTest < ActiveSupport::TestCase end end + test "older versions' modified_at indicate when they're created" do + Rails.configuration.collection_versioning = true + act_as_user users(:active) do + # Set up initial collection + c = create_collection 'foo', Encoding::US_ASCII + assert c.valid? + # Make changes so that a new version is created + c.update_attributes!({'name' => 'bar'}) + c.reload + assert_equal 2, c.version + # Get the old version + c_old = Collection.where(current_version_uuid: c.uuid, version: 1).first + assert_not_nil c_old + + version_creation_datetime = c_old.modified_at.to_f + assert_equal c.created_at.to_f, c_old.created_at.to_f + # Current version is updated just a few milliseconds before the version is + # saved on the database. + assert_operator c.modified_at.to_f, :<, version_creation_datetime + + # Make update on current version so old version get the attribute synced; + # its modified_at should not change. + new_replication = 3 + c.update_attributes!({'replication_desired' => new_replication}) + c.reload + assert_equal new_replication, c.replication_desired + c_old.reload + assert_equal new_replication, c_old.replication_desired + assert_equal version_creation_datetime, c_old.modified_at.to_f + assert_operator c.modified_at.to_f, :>, c_old.modified_at.to_f + end + end + test "older versions should no be directly updatable" do Rails.configuration.collection_versioning = true - act_as_system_user do + act_as_user users(:active) do # Set up initial collection c = create_collection 'foo', Encoding::US_ASCII assert c.valid? @@ -213,7 +246,7 @@ class CollectionTest < ActiveSupport::TestCase ].each do |versioning, attr, val, new_version_expected| test "update #{attr} with versioning #{versioning ? '' : 'not '}enabled should #{new_version_expected ? '' : 'not '}create a new version" do Rails.configuration.collection_versioning = versioning - act_as_system_user do + act_as_user users(:active) do # Create initial collection c = create_collection 'foo', Encoding::US_ASCII assert c.valid? @@ -247,7 +280,7 @@ class CollectionTest < ActiveSupport::TestCase test 'with versioning enabled, simultaneous updates increment version correctly' do Rails.configuration.collection_versioning = true - act_as_system_user do + act_as_user users(:active) do # Create initial collection col = create_collection 'foo', Encoding::US_ASCII assert col.valid? -- 2.30.2