From d990bb9c7a36954c2e5c56bedfadef52c4f74c84 Mon Sep 17 00:00:00 2001 From: fherreazcue Date: Wed, 7 Dec 2022 10:16:10 +0000 Subject: [PATCH 01/13] Added custom title and description to snapshots. --- app/controllers/snapshots_controller.rb | 5 +++ app/helpers/snapshots_helper.rb | 8 +++- app/views/snapshots/_form.html.erb | 10 +++++ app/views/snapshots/_snapshots.html.erb | 2 +- app/views/snapshots/new.html.erb | 12 +++-- app/views/snapshots/show.html.erb | 10 +++-- ...6114653_add_snapshot_title_to_snapshots.rb | 6 +++ db/schema.rb | 2 + test/functional/snapshots_controller_test.rb | 45 +++++++++++++++++-- test/unit/snapshot_test.rb | 8 ++++ 10 files changed, 96 insertions(+), 12 deletions(-) create mode 100644 app/views/snapshots/_form.html.erb create mode 100644 db/migrate/20221206114653_add_snapshot_title_to_snapshots.rb diff --git a/app/controllers/snapshots_controller.rb b/app/controllers/snapshots_controller.rb index ebeb52f528..67391e37e0 100644 --- a/app/controllers/snapshots_controller.rb +++ b/app/controllers/snapshots_controller.rb @@ -16,6 +16,7 @@ class SnapshotsController < ApplicationController def create @snapshot = @resource.create_snapshot if @snapshot + @snapshot.update(snapshot_params) flash[:notice] = "Snapshot created" redirect_to polymorphic_path([@resource, @snapshot]) else @@ -128,4 +129,8 @@ def doi_minting_enabled? def metadata_params params.require(:metadata).permit(:access_right, :license, :embargo_date, :access_conditions, creators: [:name]).delete_if { |k,v| v.blank? } end + + def snapshot_params + params.require(:snapshot).permit(:snapshot_title, :snapshot_description) + end end diff --git a/app/helpers/snapshots_helper.rb b/app/helpers/snapshots_helper.rb index 39f6026d91..49dc25934b 100644 --- a/app/helpers/snapshots_helper.rb +++ b/app/helpers/snapshots_helper.rb @@ -15,6 +15,12 @@ def list_item_snapshot_list(resource) end def snapshot_link(resource, snapshot) - link_to "Snapshot #{snapshot.snapshot_number}", polymorphic_path([resource, snapshot]) + link_to snapshot_display_name(snapshot), polymorphic_path([resource, snapshot]), 'data-tooltip' => tooltip(snapshot.snapshot_description) + end + + def snapshot_display_name(snapshot) + display_name = "Snapshot #{snapshot.snapshot_number}" + display_name = snapshot.snapshot_title unless snapshot.snapshot_title.nil? || snapshot.snapshot_title == '' + display_name end end diff --git a/app/views/snapshots/_form.html.erb b/app/views/snapshots/_form.html.erb new file mode 100644 index 0000000000..d3cf9fc19b --- /dev/null +++ b/app/views/snapshots/_form.html.erb @@ -0,0 +1,10 @@ +<%= error_messages_for :snapshot %> + +
+ + <%= f.text_field :snapshot_title,:class=>"form-control"%> +
+
+ <%= f.label :snapshot_description -%>
+ <%= f.text_area :snapshot_description, rows: 3, :class=>"form-control"-%> +
diff --git a/app/views/snapshots/_snapshots.html.erb b/app/views/snapshots/_snapshots.html.erb index a38b34f034..9bd48b862c 100644 --- a/app/views/snapshots/_snapshots.html.erb +++ b/app/views/snapshots/_snapshots.html.erb @@ -3,7 +3,7 @@ <% snapshots.order("created_at DESC").each do |snapshot| %>
<% if snapshot == selected %> - <%= content_tag :strong, "Snapshot #{snapshot.snapshot_number}" %> + <%= content_tag :strong, snapshot_display_name(snapshot), 'data-tooltip' => tooltip(snapshot.snapshot_description) %> <% else %> <%= snapshot_link(resource,snapshot) %> <% end %> diff --git a/app/views/snapshots/new.html.erb b/app/views/snapshots/new.html.erb index d834dc68cb..7e01af23bf 100644 --- a/app/views/snapshots/new.html.erb +++ b/app/views/snapshots/new.html.erb @@ -38,10 +38,14 @@
<% end %> +
+ <%= form_for [@resource, @resource.snapshots.build] do |f| -%> + <%= render :partial => "form", :locals => { :f => f, :action=>:new } -%> -<%= link_to('Create Snapshot', polymorphic_path([@resource, :snapshots]), - :method => :post, :class => 'btn btn-primary') %> + <%= f.submit(:class => 'btn btn-primary') %> -<%= image_tag_for_key('publish', polymorphic_path(@resource, :action => :check_related_items), nil, {:method=>:post,:class => 'btn btn-default'}, "Publish full #{t(@resource.class.name.downcase)}") if excluded_items.any? %> + <%= image_tag_for_key('publish', polymorphic_path(@resource, :action => :check_related_items), nil, {:method=>:post,:class => 'btn btn-default'}, "Publish full #{t(@resource.class.name.downcase)}") if excluded_items.any? %> -or <%= cancel_button polymorphic_path(@resource) -%> \ No newline at end of file + or <%= cancel_button polymorphic_path(@resource) -%> + <% end -%> +
diff --git a/app/views/snapshots/show.html.erb b/app/views/snapshots/show.html.erb index 10c45aafd3..b9bb46a49d 100644 --- a/app/views/snapshots/show.html.erb +++ b/app/views/snapshots/show.html.erb @@ -2,11 +2,10 @@ <%= @snapshot.title -%> (snapshot <%= @snapshot.snapshot_number -%>) <% end %> -<%= render :partial => "general/item_title", :locals => {:item=>@snapshot, :buttons_partial => 'snapshots/buttons'} %> - +<%= render :partial => "general/item_title", :locals => {:item=>@snapshot, :title=>snapshot_display_name(@snapshot), :buttons_partial => 'snapshots/buttons'} %>
- <%= item_description @snapshot.description -%> + <%= item_description @snapshot.snapshot_description -%> <% if Seek::Config.doi_minting_enabled %>

@@ -34,6 +33,11 @@

Created at: <%= date_as_string(@snapshot.created_at, true) %>

+ <%= panel('Resource') do %> + <%= render :partial => "general/item_title", :locals => {:item=>@resource} %> + <%= item_description @snapshot.description -%> + <% end %> + <%= panel('Contents') do %> <%= render :partial => 'tree', locals: { resource: @snapshot.metadata, top: true } %> <% end %> diff --git a/db/migrate/20221206114653_add_snapshot_title_to_snapshots.rb b/db/migrate/20221206114653_add_snapshot_title_to_snapshots.rb new file mode 100644 index 0000000000..78f1903bb8 --- /dev/null +++ b/db/migrate/20221206114653_add_snapshot_title_to_snapshots.rb @@ -0,0 +1,6 @@ +class AddSnapshotTitleToSnapshots < ActiveRecord::Migration[6.1] + def change + add_column :snapshots, :snapshot_title, :string + add_column :snapshots, :snapshot_description, :text + end +end diff --git a/db/schema.rb b/db/schema.rb index 058f8c95a2..586a2f6d2e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1921,6 +1921,8 @@ t.datetime "updated_at", null: false t.integer "zenodo_deposition_id" t.string "zenodo_record_url" + t.string "snapshot_title" + t.text "snapshot_description" end create_table "sop_auth_lookup", force: :cascade do |t| diff --git a/test/functional/snapshots_controller_test.rb b/test/functional/snapshots_controller_test.rb index 000b40a499..8d7cc68815 100644 --- a/test/functional/snapshots_controller_test.rb +++ b/test/functional/snapshots_controller_test.rb @@ -62,7 +62,7 @@ class SnapshotsControllerTest < ActionController::TestCase login_as(user) assert_difference('Snapshot.count') do - post :create, params: { investigation_id: investigation } + post :create, params: { investigation_id: investigation, snapshot: empty_snapshot } end assert investigation.can_manage?(user) @@ -76,7 +76,7 @@ class SnapshotsControllerTest < ActionController::TestCase login_as(user) assert_difference('Snapshot.count') do - post :create, params: { study_id: study } + post :create, params: { study_id: study, snapshot: empty_snapshot } end assert study.can_manage?(user) @@ -90,13 +90,48 @@ class SnapshotsControllerTest < ActionController::TestCase login_as(user) assert_difference('Snapshot.count') do - post :create, params: { assay_id: assay } + post :create, params: { assay_id: assay, snapshot: empty_snapshot } end assert assay.can_manage?(user) assert_redirected_to assay_snapshot_path(assay, assigns(:snapshot).snapshot_number) end + test 'snapshot title saved correctly' do + user = Factory(:user) + investigation = Factory(:investigation, policy: Factory(:publicly_viewable_policy), contributor: user.person) + login_as(user) + assert_difference('Snapshot.count') do + post :create, params: { investigation_id: investigation, + snapshot: { snapshot_title: 'MyTitle', snapshot_description: 'Some info' } } + end + assert_equal 'MyTitle', investigation.snapshots.last.snapshot_title + assert_equal 'Some info', investigation.snapshots.last.snapshot_description + end + + test 'snapshot title and description correctly displayed' do + user = Factory(:user) + investigation = Factory(:investigation, policy: Factory(:publicly_viewable_policy), contributor: user.person, + description: 'Not a snapshot', title: 'My Investigation') + login_as(user) + post :create, params: { investigation_id: investigation, + snapshot: { snapshot_title: 'My first snapshot', snapshot_description: 'Some info' } } + post :create, params: { investigation_id: investigation, + snapshot: { snapshot_title: 'My second snapshot', snapshot_description: 'Other info' } } + post :create, params: { investigation_id: investigation, snapshot: empty_snapshot } + + get :show, params: { investigation_id: investigation, id: 1 } + assert_response :success + assert_select 'h1', 'My first snapshot' + assert_select 'div#description', 'Some info' + assert_select 'div#snapshots' do + assert_select 'strong', 'My first snapshot' + assert_select 'a[href=?]', investigation_snapshot_path(investigation, 2), text: 'My second snapshot' + assert_select 'a[data-tooltip=?]', 'Other info' + assert_select 'a[href=?]', investigation_snapshot_path(investigation, 3), text: 'Snapshot 3' + end + end + test "can't create snapshot if no manage permissions" do user = FactoryBot.create(:user) investigation = FactoryBot.create(:investigation, policy: FactoryBot.create(:publicly_viewable_policy), @@ -577,6 +612,10 @@ class SnapshotsControllerTest < ActionController::TestCase private + def empty_snapshot + { snapshot_title: '', snapshot_description: '' } + end + def create_investigation_snapshot @user = FactoryBot.create(:user) @investigation = FactoryBot.create(:investigation, description: 'not blank', policy: FactoryBot.create(:publicly_viewable_policy), diff --git a/test/unit/snapshot_test.rb b/test/unit/snapshot_test.rb index 363e224fa7..c4677e08ce 100644 --- a/test/unit/snapshot_test.rb +++ b/test/unit/snapshot_test.rb @@ -37,6 +37,14 @@ class SnapshotTest < ActiveSupport::TestCase assert_equal 2, s2.snapshot_number end + test 'snapshot title and description correctly set' do + s1 = @investigation.create_snapshot + s1.update_attribute(:snapshot_title, 'My snapshot title') + s1.update_attribute(:snapshot_description, 'My description') + assert_equal 'My snapshot title', s1.snapshot_title + assert_equal 'My description', s1.snapshot_description + end + test 'sha1 and md5 checksum' do s1 = @investigation.create_snapshot refute_nil s1.md5sum From f18e63386b76506e5536d4227cf52550d830cc5e Mon Sep 17 00:00:00 2001 From: fherreazcue Date: Wed, 7 Dec 2022 16:56:00 +0000 Subject: [PATCH 02/13] Removed "snapshot_" from "snapshot_title" and "snapshot_description" --- app/controllers/snapshots_controller.rb | 2 +- app/helpers/snapshots_helper.rb | 4 +-- app/models/snapshot.rb | 12 ++++++-- app/views/snapshots/_form.html.erb | 8 ++--- app/views/snapshots/_snapshots.html.erb | 2 +- app/views/snapshots/show.html.erb | 6 ++-- ...6114653_add_snapshot_title_to_snapshots.rb | 4 +-- db/schema.rb | 4 +-- lib/zenodo/acts_as_zenodo_depositable.rb | 1 + test/functional/snapshots_controller_test.rb | 30 +++++++++++++++---- test/unit/snapshot_test.rb | 14 ++++----- 11 files changed, 56 insertions(+), 31 deletions(-) diff --git a/app/controllers/snapshots_controller.rb b/app/controllers/snapshots_controller.rb index 67391e37e0..aff98280b6 100644 --- a/app/controllers/snapshots_controller.rb +++ b/app/controllers/snapshots_controller.rb @@ -131,6 +131,6 @@ def metadata_params end def snapshot_params - params.require(:snapshot).permit(:snapshot_title, :snapshot_description) + params.require(:snapshot).permit(:title, :description) end end diff --git a/app/helpers/snapshots_helper.rb b/app/helpers/snapshots_helper.rb index 49dc25934b..36e997eb94 100644 --- a/app/helpers/snapshots_helper.rb +++ b/app/helpers/snapshots_helper.rb @@ -15,12 +15,12 @@ def list_item_snapshot_list(resource) end def snapshot_link(resource, snapshot) - link_to snapshot_display_name(snapshot), polymorphic_path([resource, snapshot]), 'data-tooltip' => tooltip(snapshot.snapshot_description) + link_to snapshot_display_name(snapshot), polymorphic_path([resource, snapshot]), 'data-tooltip' => tooltip(snapshot.description) end def snapshot_display_name(snapshot) display_name = "Snapshot #{snapshot.snapshot_number}" - display_name = snapshot.snapshot_title unless snapshot.snapshot_title.nil? || snapshot.snapshot_title == '' + display_name = snapshot.title unless snapshot.title.nil? || snapshot.title == '' display_name end end diff --git a/app/models/snapshot.rb b/app/models/snapshot.rb index 0eb03145f7..0c256d0c3e 100644 --- a/app/models/snapshot.rb +++ b/app/models/snapshot.rb @@ -35,7 +35,7 @@ def metadata def title if content_blob.present? - metadata['title'] + super else 'incomplete snapshot' end @@ -43,12 +43,20 @@ def title def description if content_blob.present? - metadata['description'] + super else 'The snapshot currently has no content, and could still be being generated.' end end + def m_title + metadata['title'] + end + + def m_description + metadata['description'] + end + def contributor Person.find(metadata['contributor']['uri'].match(/people\/([1-9][0-9]*)/)[1]) end diff --git a/app/views/snapshots/_form.html.erb b/app/views/snapshots/_form.html.erb index d3cf9fc19b..78afbcb94f 100644 --- a/app/views/snapshots/_form.html.erb +++ b/app/views/snapshots/_form.html.erb @@ -1,10 +1,10 @@ <%= error_messages_for :snapshot %>
- - <%= f.text_field :snapshot_title,:class=>"form-control"%> + <%= f.label :title -%>
+ <%= f.text_field :title,:class=>"form-control"%>
- <%= f.label :snapshot_description -%>
- <%= f.text_area :snapshot_description, rows: 3, :class=>"form-control"-%> + <%= f.label :description -%>
+ <%= f.text_area :description, rows: 3, :class=>"form-control"-%>
diff --git a/app/views/snapshots/_snapshots.html.erb b/app/views/snapshots/_snapshots.html.erb index 9bd48b862c..ae99e60ccf 100644 --- a/app/views/snapshots/_snapshots.html.erb +++ b/app/views/snapshots/_snapshots.html.erb @@ -3,7 +3,7 @@ <% snapshots.order("created_at DESC").each do |snapshot| %>
<% if snapshot == selected %> - <%= content_tag :strong, snapshot_display_name(snapshot), 'data-tooltip' => tooltip(snapshot.snapshot_description) %> + <%= content_tag :strong, snapshot_display_name(snapshot), 'data-tooltip' => tooltip(snapshot.description) %> <% else %> <%= snapshot_link(resource,snapshot) %> <% end %> diff --git a/app/views/snapshots/show.html.erb b/app/views/snapshots/show.html.erb index b9bb46a49d..901235f9ec 100644 --- a/app/views/snapshots/show.html.erb +++ b/app/views/snapshots/show.html.erb @@ -5,7 +5,7 @@ <%= render :partial => "general/item_title", :locals => {:item=>@snapshot, :title=>snapshot_display_name(@snapshot), :buttons_partial => 'snapshots/buttons'} %>
- <%= item_description @snapshot.snapshot_description -%> + <%= item_description @snapshot.description -%> <% if Seek::Config.doi_minting_enabled %>

@@ -34,8 +34,8 @@

Created at: <%= date_as_string(@snapshot.created_at, true) %>

<%= panel('Resource') do %> - <%= render :partial => "general/item_title", :locals => {:item=>@resource} %> - <%= item_description @snapshot.description -%> + <%= render :partial => "general/item_title", :locals => {:item=>@resource, :title=>@snapshot.m_title} %> + <%= item_description @snapshot.m_description -%> <% end %> <%= panel('Contents') do %> diff --git a/db/migrate/20221206114653_add_snapshot_title_to_snapshots.rb b/db/migrate/20221206114653_add_snapshot_title_to_snapshots.rb index 78f1903bb8..e4816e6cec 100644 --- a/db/migrate/20221206114653_add_snapshot_title_to_snapshots.rb +++ b/db/migrate/20221206114653_add_snapshot_title_to_snapshots.rb @@ -1,6 +1,6 @@ class AddSnapshotTitleToSnapshots < ActiveRecord::Migration[6.1] def change - add_column :snapshots, :snapshot_title, :string - add_column :snapshots, :snapshot_description, :text + add_column :snapshots, :title, :string + add_column :snapshots, :description, :text end end diff --git a/db/schema.rb b/db/schema.rb index 586a2f6d2e..4db13aee94 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1921,8 +1921,8 @@ t.datetime "updated_at", null: false t.integer "zenodo_deposition_id" t.string "zenodo_record_url" - t.string "snapshot_title" - t.text "snapshot_description" + t.string "title" + t.text "description" end create_table "sop_auth_lookup", force: :cascade do |t| diff --git a/lib/zenodo/acts_as_zenodo_depositable.rb b/lib/zenodo/acts_as_zenodo_depositable.rb index 5dba8151ba..c712dfcfa6 100644 --- a/lib/zenodo/acts_as_zenodo_depositable.rb +++ b/lib/zenodo/acts_as_zenodo_depositable.rb @@ -43,6 +43,7 @@ def export_to_zenodo(access_token, extra_metadata = {}) metadata.merge!(extra_metadata.to_h.deep_symbolize_keys) #FIXME: this is a quick hack + metadata[:title] = 'not set' if metadata[:title].blank? metadata[:description] = 'not set' if metadata[:description].blank? client = Zenodo::Client.new(access_token, Seek::Config.zenodo_api_url) diff --git a/test/functional/snapshots_controller_test.rb b/test/functional/snapshots_controller_test.rb index 8d7cc68815..292c99bfd1 100644 --- a/test/functional/snapshots_controller_test.rb +++ b/test/functional/snapshots_controller_test.rb @@ -103,10 +103,10 @@ class SnapshotsControllerTest < ActionController::TestCase login_as(user) assert_difference('Snapshot.count') do post :create, params: { investigation_id: investigation, - snapshot: { snapshot_title: 'MyTitle', snapshot_description: 'Some info' } } + snapshot: { title: 'MyTitle', description: 'Some info' } } end - assert_equal 'MyTitle', investigation.snapshots.last.snapshot_title - assert_equal 'Some info', investigation.snapshots.last.snapshot_description + assert_equal 'MyTitle', investigation.snapshots.last.title + assert_equal 'Some info', investigation.snapshots.last.description end test 'snapshot title and description correctly displayed' do @@ -115,9 +115,9 @@ class SnapshotsControllerTest < ActionController::TestCase description: 'Not a snapshot', title: 'My Investigation') login_as(user) post :create, params: { investigation_id: investigation, - snapshot: { snapshot_title: 'My first snapshot', snapshot_description: 'Some info' } } + snapshot: { title: 'My first snapshot', description: 'Some info' } } post :create, params: { investigation_id: investigation, - snapshot: { snapshot_title: 'My second snapshot', snapshot_description: 'Other info' } } + snapshot: { title: 'My second snapshot', description: 'Other info' } } post :create, params: { investigation_id: investigation, snapshot: empty_snapshot } get :show, params: { investigation_id: investigation, id: 1 } @@ -132,6 +132,24 @@ class SnapshotsControllerTest < ActionController::TestCase end end + test 'snapshot shows captured state' do + user = Factory(:user) + investigation = Factory(:investigation, policy: Factory(:publicly_viewable_policy), contributor: user.person, + title: 'Old Title', description: 'Old description') + login_as(user) + post :create, params: { investigation_id: investigation, + snapshot: { title: 'My snapshot', description: 'Snapshot info' } } + investigation.update(title: 'New title', description: 'New description') + get :show, params: { investigation_id: investigation, id: 1 } + assert_response :success + assert_select 'h1', 'My snapshot' + assert_select 'div#description', 'Snapshot info' + assert_select 'div.panel-body' do + assert_select 'h1', 'Old Title' + assert_select 'div#description', 'Old description' + end + end + test "can't create snapshot if no manage permissions" do user = FactoryBot.create(:user) investigation = FactoryBot.create(:investigation, policy: FactoryBot.create(:publicly_viewable_policy), @@ -613,7 +631,7 @@ class SnapshotsControllerTest < ActionController::TestCase private def empty_snapshot - { snapshot_title: '', snapshot_description: '' } + { title: '', description: '' } end def create_investigation_snapshot diff --git a/test/unit/snapshot_test.rb b/test/unit/snapshot_test.rb index c4677e08ce..35674f0776 100644 --- a/test/unit/snapshot_test.rb +++ b/test/unit/snapshot_test.rb @@ -39,10 +39,10 @@ class SnapshotTest < ActiveSupport::TestCase test 'snapshot title and description correctly set' do s1 = @investigation.create_snapshot - s1.update_attribute(:snapshot_title, 'My snapshot title') - s1.update_attribute(:snapshot_description, 'My description') - assert_equal 'My snapshot title', s1.snapshot_title - assert_equal 'My description', s1.snapshot_description + s1.update_attribute(:title, 'My snapshot title') + s1.update_attribute(:description, 'My description') + assert_equal 'My snapshot title', s1.title + assert_equal 'My description', s1.description end test 'sha1 and md5 checksum' do @@ -82,8 +82,8 @@ class SnapshotTest < ActiveSupport::TestCase assert_equal 'New title', @investigation.title assert_equal 'New description', @investigation.description - assert_equal old_title, snapshot.title - assert_equal old_description, snapshot.description + assert_equal old_title, snapshot.m_title + assert_equal old_description, snapshot.m_description end test 'generates sensible DOI' do @@ -256,7 +256,6 @@ class SnapshotTest < ActiveSupport::TestCase test 'study snapshot' do snapshot = @study.create_snapshot - assert_equal 's1', snapshot.title assert_equal 2, snapshot.metadata['assays'].count assert_equal 2, snapshot.metadata['assays'].find { |a| a['title'] == 'a1' }['assets'].count assert_equal 1, snapshot.metadata['assays'].find { |a| a['title'] == 'a2' }['assets'].count @@ -274,7 +273,6 @@ class SnapshotTest < ActiveSupport::TestCase snapshot = @assay.create_snapshot - assert_equal 'a1', snapshot.title assert_equal 2, snapshot.metadata['assets'].count titles = extract_keys(snapshot.metadata, 'title') From 4158a784631ab619016cf2cfc3e920ddf5ca4307 Mon Sep 17 00:00:00 2001 From: fherreazcue Date: Thu, 8 Dec 2022 09:56:52 +0000 Subject: [PATCH 03/13] Updated tests expectation of snapshot.title --- test/functional/homes_controller_test.rb | 9 ++++++--- test/unit/helpers/homes_helper_test.rb | 8 ++++++-- test/unit/investigation_test.rb | 2 +- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/test/functional/homes_controller_test.rb b/test/functional/homes_controller_test.rb index a8bbc1c54f..2005afc65e 100644 --- a/test/functional/homes_controller_test.rb +++ b/test/functional/homes_controller_test.rb @@ -261,8 +261,10 @@ class HomesControllerTest < ActionController::TestCase person = FactoryBot.create(:person) snapshot1 = FactoryBot.create(:investigation, policy: FactoryBot.create(:publicly_viewable_policy), title: 'inv with snap', contributor: person, creators: [person]).create_snapshot + snapshot1.update_attribute(:title, 'My snapshot') snapshot2 = FactoryBot.create(:assay, policy: FactoryBot.create(:publicly_viewable_policy), title: 'assay with snap', contributor: person, creators: [person]).create_snapshot + snapshot2.update_attribute(:title, 'My other snapshot') assert_difference 'ActivityLog.count', 2 do FactoryBot.create(:activity_log, action: 'create', activity_loggable: snapshot1, created_at: 1.day.ago, culprit: person.user) FactoryBot.create(:activity_log, action: 'download', activity_loggable: snapshot2, created_at: 1.day.ago, culprit: person.user) @@ -270,7 +272,7 @@ class HomesControllerTest < ActionController::TestCase get :index assert_response :success - assert_select 'div#recently_added ul>li>a[href=?]', investigation_snapshot_path(snapshot1.resource, snapshot1), text: /inv with snap/ + assert_select 'div#recently_added ul>li>a[href=?]', investigation_snapshot_path(snapshot1.resource, snapshot1), text: /My snapshot/ end test 'should show headline announcement' do @@ -379,6 +381,7 @@ class HomesControllerTest < ActionController::TestCase assay = FactoryBot.create :assay, title: 'A new assay', contributor: person, policy: FactoryBot.create(:public_policy), creators: [person] snapshot = assay.create_snapshot + snapshot.update_attribute(:title, 'My snapshot') FactoryBot.create :activity_log, activity_loggable: df, controller_name: 'data_files', culprit: person.user FactoryBot.create :activity_log, activity_loggable: sop, controller_name: 'sops', culprit: person.user @@ -392,7 +395,7 @@ class HomesControllerTest < ActionController::TestCase assert_select 'div#my-recent-contributions ul>li a[href=?]', data_file_path(df), text: /A new data file/ assert_select 'div#my-recent-contributions ul li a[href=?]', sop_path(sop), text: /A new sop/ assert_select 'div#my-recent-contributions ul li a[href=?]', assay_path(assay), text: /A new assay/ - assert_select 'div#my-recent-contributions ul li a[href=?]', assay_snapshot_path(assay, snapshot), text: /A new assay/ + assert_select 'div#my-recent-contributions ul li a[href=?]', assay_snapshot_path(assay, snapshot), text: /My snapshot/ sop.update(title: 'An old sop') FactoryBot.create :activity_log, activity_loggable: sop, controller_name: 'assays', culprit: person.user, action: 'update' @@ -403,7 +406,7 @@ class HomesControllerTest < ActionController::TestCase assert_select 'div#my-recent-contributions ul>li a[href=?]', sop_path(sop), text: /An old sop/ assert_select 'div#my-recent-contributions ul li a[href=?]', data_file_path(df), text: /A new data file/ assert_select 'div#my-recent-contributions ul li a[href=?]', assay_path(assay), text: /A new assay/ - assert_select 'div#my-recent-contributions ul li a[href=?]', assay_snapshot_path(assay, snapshot), text: /A new assay/ + assert_select 'div#my-recent-contributions ul li a[href=?]', assay_snapshot_path(assay, snapshot), text: /My snapshot/ assert_select 'div#my-recent-contributions ul li a[href=?]', sop_path(sop), text: /A new sop/, count: 0 end diff --git a/test/unit/helpers/homes_helper_test.rb b/test/unit/helpers/homes_helper_test.rb index 90613f5530..984982b04c 100644 --- a/test/unit/helpers/homes_helper_test.rb +++ b/test/unit/helpers/homes_helper_test.rb @@ -62,8 +62,10 @@ def setup person = FactoryBot.create(:person) snapshot1 = FactoryBot.create(:investigation, policy: FactoryBot.create(:publicly_viewable_policy), contributor: person, creators: [person]).create_snapshot + snapshot1.update_attribute(:title, 'My snapshot') snapshot2 = FactoryBot.create(:assay, policy: FactoryBot.create(:publicly_viewable_policy), contributor: person, creators: [person]).create_snapshot + snapshot2.update_attribute(:title, 'My other snapshot') FactoryBot.create(:activity_log, action: 'create', activity_loggable: snapshot1, created_at: 1.day.ago, culprit: person.user) FactoryBot.create(:activity_log, action: 'download', activity_loggable: snapshot2, created_at: 1.day.ago, culprit: person.user) @@ -72,14 +74,16 @@ def setup assert_equal 1, added_hash.count added = added_hash.first + pp added_hash + pp added assert_equal 'Snapshot', added[:type] - assert_equal snapshot1.resource.title.to_s, added[:title] + assert_equal snapshot1.title.to_s, added[:title] assert_equal investigation_snapshot_path(snapshot1.resource, snapshot1), added[:url] assert_equal 1, downloaded_hash.count downloaded = downloaded_hash.first assert_equal 'Snapshot', downloaded[:type] - assert_equal snapshot2.resource.title.to_s, downloaded[:title] + assert_equal snapshot2.title.to_s, downloaded[:title] assert_equal assay_snapshot_path(snapshot2.resource, snapshot2), downloaded[:url] end diff --git a/test/unit/investigation_test.rb b/test/unit/investigation_test.rb index f7093a60fa..f17cfa50cc 100644 --- a/test/unit/investigation_test.rb +++ b/test/unit/investigation_test.rb @@ -155,7 +155,7 @@ class InvestigationTest < ActiveSupport::TestCase end assert_equal 1, investigation.snapshots.count - assert_equal investigation.title, snapshot.title + assert_equal investigation.title, snapshot.m_title end test 'clone with associations' do From c1ea18bfcb4f948a9b10a25a18dbd5d40dd3b111 Mon Sep 17 00:00:00 2001 From: fherreazcue Date: Fri, 9 Dec 2022 13:33:35 +0000 Subject: [PATCH 04/13] Added edit option for snapshot title and description. --- app/controllers/snapshots_controller.rb | 21 +++++- app/views/snapshots/_buttons.html.erb | 2 + app/views/snapshots/_form.html.erb | 2 + app/views/snapshots/edit.html.erb | 7 ++ app/views/snapshots/new.html.erb | 2 - app/views/snapshots/show.html.erb | 2 +- config/routes.rb | 2 +- test/functional/snapshots_controller_test.rb | 77 ++++++++++++++++++++ 8 files changed, 109 insertions(+), 6 deletions(-) create mode 100644 app/views/snapshots/edit.html.erb diff --git a/app/controllers/snapshots_controller.rb b/app/controllers/snapshots_controller.rb index aff98280b6..f23d1744b8 100644 --- a/app/controllers/snapshots_controller.rb +++ b/app/controllers/snapshots_controller.rb @@ -3,9 +3,9 @@ class SnapshotsController < ApplicationController before_action :find_resource - before_action :auth_resource, only: [:mint_doi_confirm, :mint_doi, :new, :create, :export_preview, :export_submit, :destroy] + before_action :auth_resource, only: [:mint_doi_confirm, :mint_doi, :new, :create, :edit, :update, :export_preview, :export_submit, :destroy] before_action :check_resource_permitted_for_ro, only: [:new, :create] - before_action :find_snapshot, only: [:show, :mint_doi_confirm, :mint_doi, :download, :export_preview, :export_submit, :destroy] + before_action :find_snapshot, only: [:show, :edit, :update, :mint_doi_confirm, :mint_doi, :download, :export_preview, :export_submit, :destroy] before_action :doi_minting_enabled?, only: [:mint_doi_confirm, :mint_doi] before_action :zenodo_oauth_client before_action :zenodo_oauth_session, only: [:export_submit] @@ -40,6 +40,23 @@ def show def new end + def edit + if @snapshot.has_doi? + flash[:error] = "You cannot modify a snapshot that has a DOI." + redirect_to polymorphic_path([@resource, @snapshot]) + end + end + + def update + if @snapshot.has_doi? + flash[:error] = "You cannot modify a snapshot that has a DOI." + else + @snapshot.update(snapshot_params) + flash[:notice] = "Snapshot updated" + end + redirect_to polymorphic_path([@resource, @snapshot]) + end + def download @content_blob = @snapshot.content_blob send_file @content_blob.filepath, diff --git a/app/views/snapshots/_buttons.html.erb b/app/views/snapshots/_buttons.html.erb index 9a932bf48b..62aba7d695 100644 --- a/app/views/snapshots/_buttons.html.erb +++ b/app/views/snapshots/_buttons.html.erb @@ -6,6 +6,8 @@ <% if @snapshot.can_mint_doi? %> <%= button_link_to("Generate a DOI", 'doi', polymorphic_path([@resource, @snapshot], action: 'mint_doi_confirm')) %> <% end %> + <%= button_link_to("Edit",'edit', polymorphic_path([@resource, @snapshot], action: 'edit'), + 'data-tooltip' => 'Modify the title and/or description of your snapshot') %> <%= button_link_to("Delete", 'destroy', polymorphic_path([@resource, @snapshot]), { confirm: "Are you sure you wish to delete this snapshot?", method: :delete }) %> <% end %> diff --git a/app/views/snapshots/_form.html.erb b/app/views/snapshots/_form.html.erb index 78afbcb94f..e3fdc66fff 100644 --- a/app/views/snapshots/_form.html.erb +++ b/app/views/snapshots/_form.html.erb @@ -8,3 +8,5 @@ <%= f.label :description -%>
<%= f.text_area :description, rows: 3, :class=>"form-control"-%>
+ +<%= f.submit(:class => 'btn btn-primary') %> \ No newline at end of file diff --git a/app/views/snapshots/edit.html.erb b/app/views/snapshots/edit.html.erb new file mode 100644 index 0000000000..36d10e5239 --- /dev/null +++ b/app/views/snapshots/edit.html.erb @@ -0,0 +1,7 @@ +

Edit <%= t('snapshot') %>

+ +<%= form_for [@resource, @snapshot] do |f| %> + <%= render :partial => "form", :locals => { :f => f, :action=>:edit } -%> + + or <%= cancel_button polymorphic_path([@resource, @snapshot]) -%> +<% end -%> diff --git a/app/views/snapshots/new.html.erb b/app/views/snapshots/new.html.erb index 7e01af23bf..17674ad440 100644 --- a/app/views/snapshots/new.html.erb +++ b/app/views/snapshots/new.html.erb @@ -42,8 +42,6 @@ <%= form_for [@resource, @resource.snapshots.build] do |f| -%> <%= render :partial => "form", :locals => { :f => f, :action=>:new } -%> - <%= f.submit(:class => 'btn btn-primary') %> - <%= image_tag_for_key('publish', polymorphic_path(@resource, :action => :check_related_items), nil, {:method=>:post,:class => 'btn btn-default'}, "Publish full #{t(@resource.class.name.downcase)}") if excluded_items.any? %> or <%= cancel_button polymorphic_path(@resource) -%> diff --git a/app/views/snapshots/show.html.erb b/app/views/snapshots/show.html.erb index 901235f9ec..99e0d4235b 100644 --- a/app/views/snapshots/show.html.erb +++ b/app/views/snapshots/show.html.erb @@ -2,7 +2,7 @@ <%= @snapshot.title -%> (snapshot <%= @snapshot.snapshot_number -%>) <% end %> -<%= render :partial => "general/item_title", :locals => {:item=>@snapshot, :title=>snapshot_display_name(@snapshot), :buttons_partial => 'snapshots/buttons'} %> +<%= render :partial => "general/item_title", :locals => {:item=>@snapshot, :buttons_partial => 'snapshots/buttons'} %>
<%= item_description @snapshot.description -%> diff --git a/config/routes.rb b/config/routes.rb index 8d9cf068e8..25fbaa9caf 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -81,7 +81,7 @@ end concern :has_snapshots do - resources :snapshots, only: [:show, :new, :create, :destroy], concerns: [:has_doi] do + resources :snapshots, concerns: [:has_doi] do member do get :download get :export, action: :export_preview diff --git a/test/functional/snapshots_controller_test.rb b/test/functional/snapshots_controller_test.rb index 292c99bfd1..11358a5d90 100644 --- a/test/functional/snapshots_controller_test.rb +++ b/test/functional/snapshots_controller_test.rb @@ -248,7 +248,84 @@ class SnapshotsControllerTest < ActionController::TestCase assert flash[:error].include?('exist') end + test 'edit button is shown for authorized users' do + create_investigation_snapshot + login_as(@user) + get :show, params: { investigation_id: @investigation, id: @snapshot.snapshot_number } + assert_select 'a', text: 'Edit' + end + test 'edit button not shown for unauthorized users' do + create_investigation_snapshot + login_as(Factory(:user)) + get :show, params: { investigation_id: @investigation, id: @snapshot.snapshot_number } + assert_select 'a', text: 'Edit', count: 0 + end + + test 'edit button not shown for snapshots with DOI' do + create_investigation_snapshot + login_as(@user) + @snapshot.doi = '10.5072/123' + @snapshot.save + get :show, params: { investigation_id: @investigation, id: @snapshot.snapshot_number } + assert_select 'a', text: 'Edit', count: 0 + end + + test 'authorized users can edit snapshot title and description' do + create_investigation_snapshot + login_as(@user) + get :edit, params: { investigation_id: @investigation, id: @snapshot.snapshot_number } + assert_response :success + end + + test "unauthorized user can't edit snapshot" do + create_investigation_snapshot + login_as(Factory(:user)) + get :edit, params: { investigation_id: @investigation, id: @snapshot.snapshot_number } + assert_redirected_to investigation_path(@investigation) + assert flash[:error] + end + + test "can't edit snapshot with DOI" do + create_investigation_snapshot + login_as(@user) + @snapshot.doi = '10.5072/123' + @snapshot.save + get :edit, params: { investigation_id: @investigation, id: @snapshot.snapshot_number } + assert_redirected_to investigation_snapshot_path(@investigation, @snapshot) + assert flash[:error].include?('DOI') + end + + test 'authorized users can update snapshot' do + create_investigation_snapshot + login_as(@user) + put :update, params: { investigation_id: @investigation, id: @snapshot.snapshot_number, + snapshot: { title: 'My mod snapshot', description: 'Snapshot mod info' } } + assert_redirected_to investigation_snapshot_path(@investigation, @snapshot) + @snapshot.reload + assert_equal 'My mod snapshot', @snapshot.title + assert_equal 'Snapshot mod info', @snapshot.description + end + + test "unauthorized users can't update snapshot" do + create_investigation_snapshot + login_as(Factory(:user)) + put :update, params: { investigation_id: @investigation, id: @snapshot.snapshot_number, + snapshot: { title: 'My mod snapshot', description: 'Snapshot mod info' } } + assert_redirected_to investigation_path(@investigation) + assert flash[:error] + end + + test "can't update snapshot with doi" do + create_investigation_snapshot + login_as(@user) + @snapshot.doi = '10.5072/123' + @snapshot.save + put :update, params: { investigation_id: @investigation, id: @snapshot.snapshot_number, + snapshot: { title: 'My mod snapshot', description: 'Snapshot mod info' } } + assert_redirected_to investigation_snapshot_path(@investigation, @snapshot) + assert flash[:error].include?('DOI') + end test 'can get confirmation when minting DOI for snapshot' do datacite_mock From 5d0c7544506f05c51a4ec911aaf4fff715219d11 Mon Sep 17 00:00:00 2001 From: fherreazcue Date: Fri, 9 Dec 2022 15:51:31 +0000 Subject: [PATCH 05/13] Removed m_title and m_description from snapshots --- app/models/snapshot.rb | 8 -------- app/views/snapshots/show.html.erb | 4 ++-- test/unit/investigation_test.rb | 2 +- test/unit/snapshot_test.rb | 4 ++-- 4 files changed, 5 insertions(+), 13 deletions(-) diff --git a/app/models/snapshot.rb b/app/models/snapshot.rb index 0c256d0c3e..5605bf1af7 100644 --- a/app/models/snapshot.rb +++ b/app/models/snapshot.rb @@ -49,14 +49,6 @@ def description end end - def m_title - metadata['title'] - end - - def m_description - metadata['description'] - end - def contributor Person.find(metadata['contributor']['uri'].match(/people\/([1-9][0-9]*)/)[1]) end diff --git a/app/views/snapshots/show.html.erb b/app/views/snapshots/show.html.erb index 99e0d4235b..27ef30fe31 100644 --- a/app/views/snapshots/show.html.erb +++ b/app/views/snapshots/show.html.erb @@ -34,8 +34,8 @@

Created at: <%= date_as_string(@snapshot.created_at, true) %>

<%= panel('Resource') do %> - <%= render :partial => "general/item_title", :locals => {:item=>@resource, :title=>@snapshot.m_title} %> - <%= item_description @snapshot.m_description -%> + <%= render :partial => "general/item_title", :locals => {:item=>@resource, :title=>@snapshot.metadata['title']} %> + <%= item_description @snapshot.metadata['description'] -%> <% end %> <%= panel('Contents') do %> diff --git a/test/unit/investigation_test.rb b/test/unit/investigation_test.rb index f17cfa50cc..1c74096a23 100644 --- a/test/unit/investigation_test.rb +++ b/test/unit/investigation_test.rb @@ -155,7 +155,7 @@ class InvestigationTest < ActiveSupport::TestCase end assert_equal 1, investigation.snapshots.count - assert_equal investigation.title, snapshot.m_title + assert_equal investigation.title, snapshot.metadata['title'] end test 'clone with associations' do diff --git a/test/unit/snapshot_test.rb b/test/unit/snapshot_test.rb index 35674f0776..556e8d43b1 100644 --- a/test/unit/snapshot_test.rb +++ b/test/unit/snapshot_test.rb @@ -82,8 +82,8 @@ class SnapshotTest < ActiveSupport::TestCase assert_equal 'New title', @investigation.title assert_equal 'New description', @investigation.description - assert_equal old_title, snapshot.m_title - assert_equal old_description, snapshot.m_description + assert_equal old_title, snapshot.metadata['title'] + assert_equal old_description, snapshot.metadata['description'] end test 'generates sensible DOI' do From ad22ffb96d4639a2a253832c8f428612437b81f2 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 26 Sep 2024 13:23:14 +0100 Subject: [PATCH 06/13] Use resource title/description for snapshot's Zenodo entry --- app/models/snapshot.rb | 29 ++++++++++-------------- lib/zenodo/acts_as_zenodo_depositable.rb | 4 ---- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/app/models/snapshot.rb b/app/models/snapshot.rb index 5605bf1af7..c92246baf9 100644 --- a/app/models/snapshot.rb +++ b/app/models/snapshot.rb @@ -33,22 +33,6 @@ def metadata @ro_metadata ||= parse_metadata end - def title - if content_blob.present? - super - else - 'incomplete snapshot' - end - end - - def description - if content_blob.present? - super - else - 'The snapshot currently has no content, and could still be being generated.' - end - end - def contributor Person.find(metadata['contributor']['uri'].match(/people\/([1-9][0-9]*)/)[1]) end @@ -97,10 +81,21 @@ def can_mint_doi? (resource.created_at + (Seek::Config.time_lock_doi_for || 0).to_i.days) <= Time.now end + def zenodo_metadata + super.tap do |zm| + zm[:title] = metadata['title'] + zm[:description] = metadata['description'] + end + end + + def potential_snapshot_number + (resource.snapshots.maximum(:snapshot_number) || 0) + 1 + end + private def set_snapshot_number - self.snapshot_number ||= (resource.snapshots.maximum(:snapshot_number) || 0) + 1 + self.snapshot_number ||= potential_snapshot_number end def doi_target_url diff --git a/lib/zenodo/acts_as_zenodo_depositable.rb b/lib/zenodo/acts_as_zenodo_depositable.rb index c712dfcfa6..c5beac0acb 100644 --- a/lib/zenodo/acts_as_zenodo_depositable.rb +++ b/lib/zenodo/acts_as_zenodo_depositable.rb @@ -42,10 +42,6 @@ def export_to_zenodo(access_token, extra_metadata = {}) metadata = zenodo_metadata metadata.merge!(extra_metadata.to_h.deep_symbolize_keys) - #FIXME: this is a quick hack - metadata[:title] = 'not set' if metadata[:title].blank? - metadata[:description] = 'not set' if metadata[:description].blank? - client = Zenodo::Client.new(access_token, Seek::Config.zenodo_api_url) deposition = client.create_deposition({ metadata: metadata.build }) deposition_file = deposition.create_file(zenodo_depositable_file) From 349d1cce7b9acb02e2d7693e8c50ffda0de85210 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 26 Sep 2024 13:51:12 +0100 Subject: [PATCH 07/13] Display default snapshot title in page name --- app/views/snapshots/show.html.erb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/app/views/snapshots/show.html.erb b/app/views/snapshots/show.html.erb index 27ef30fe31..84105ea257 100644 --- a/app/views/snapshots/show.html.erb +++ b/app/views/snapshots/show.html.erb @@ -1,8 +1,6 @@ -<% title = capture do %> - <%= @snapshot.title -%> - (snapshot <%= @snapshot.snapshot_number -%>) -<% end %> -<%= render :partial => "general/item_title", :locals => {:item=>@snapshot, :buttons_partial => 'snapshots/buttons'} %> +<%= render partial: 'general/item_title', locals: { item: @snapshot, title: snapshot_display_name(@snapshot), + buttons_partial: 'snapshots/buttons' } %> +
<%= item_description @snapshot.description -%> From 69cf89b064e139a0f839b52ecf6a90282ec86ca0 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 26 Sep 2024 13:52:29 +0100 Subject: [PATCH 08/13] Show default snapshot title as placeholder in form --- app/helpers/snapshots_helper.rb | 6 +++--- app/views/snapshots/_form.html.erb | 7 ++++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/app/helpers/snapshots_helper.rb b/app/helpers/snapshots_helper.rb index 36e997eb94..6edb78c51b 100644 --- a/app/helpers/snapshots_helper.rb +++ b/app/helpers/snapshots_helper.rb @@ -19,8 +19,8 @@ def snapshot_link(resource, snapshot) end def snapshot_display_name(snapshot) - display_name = "Snapshot #{snapshot.snapshot_number}" - display_name = snapshot.title unless snapshot.title.nil? || snapshot.title == '' - display_name + return snapshot.title if snapshot.title.present? + return "Snapshot #{snapshot.snapshot_number}" if snapshot.persisted? + "Snapshot #{snapshot.potential_snapshot_number}" end end diff --git a/app/views/snapshots/_form.html.erb b/app/views/snapshots/_form.html.erb index e3fdc66fff..17c4d8c3f9 100644 --- a/app/views/snapshots/_form.html.erb +++ b/app/views/snapshots/_form.html.erb @@ -2,11 +2,12 @@
<%= f.label :title -%>
- <%= f.text_field :title,:class=>"form-control"%> + <%= f.text_field :title, class: 'form-control', placeholder: snapshot_display_name(f.object) %>
+
<%= f.label :description -%>
- <%= f.text_area :description, rows: 3, :class=>"form-control"-%> + <%= f.text_area :description, rows: 3, class: 'form-control' %>
-<%= f.submit(:class => 'btn btn-primary') %> \ No newline at end of file +<%= f.submit class: 'btn btn-primary' %> From b84d78433580ce614190ae488ea628b8c55d855a Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 26 Sep 2024 14:56:59 +0100 Subject: [PATCH 09/13] Better snapshot validation & error handling --- app/controllers/snapshots_controller.rb | 41 +++++++------ app/models/snapshot.rb | 7 +++ app/views/snapshots/new.html.erb | 6 +- .../research_objects/acts_as_snapshottable.rb | 9 +-- test/functional/snapshots_controller_test.rb | 61 +++++++++++-------- test/unit/snapshot_test.rb | 24 +++++++- 6 files changed, 90 insertions(+), 58 deletions(-) diff --git a/app/controllers/snapshots_controller.rb b/app/controllers/snapshots_controller.rb index f23d1744b8..e21575b194 100644 --- a/app/controllers/snapshots_controller.rb +++ b/app/controllers/snapshots_controller.rb @@ -7,6 +7,8 @@ class SnapshotsController < ApplicationController before_action :check_resource_permitted_for_ro, only: [:new, :create] before_action :find_snapshot, only: [:show, :edit, :update, :mint_doi_confirm, :mint_doi, :download, :export_preview, :export_submit, :destroy] before_action :doi_minting_enabled?, only: [:mint_doi_confirm, :mint_doi] + before_action :check_doi, only: [:edit, :update, :destroy] + before_action :zenodo_oauth_client before_action :zenodo_oauth_session, only: [:export_submit] @@ -14,14 +16,12 @@ class SnapshotsController < ApplicationController include Seek::ExternalServiceWrapper def create - @snapshot = @resource.create_snapshot - if @snapshot - @snapshot.update(snapshot_params) + @snapshot = @resource.create_snapshot(snapshot_params) + if @snapshot&.valid? flash[:notice] = "Snapshot created" redirect_to polymorphic_path([@resource, @snapshot]) else - flash[:error] = @resource.errors.full_messages.join(', ') - redirect_to polymorphic_path(@resource) + render :new, status: :unprocessable_entity end end @@ -41,20 +41,15 @@ def new end def edit - if @snapshot.has_doi? - flash[:error] = "You cannot modify a snapshot that has a DOI." - redirect_to polymorphic_path([@resource, @snapshot]) - end end def update - if @snapshot.has_doi? - flash[:error] = "You cannot modify a snapshot that has a DOI." - else - @snapshot.update(snapshot_params) + if @snapshot.update(snapshot_params) flash[:notice] = "Snapshot updated" + redirect_to polymorphic_path([@resource, @snapshot]) + else + render :edit, status: :unprocessable_entity end - redirect_to polymorphic_path([@resource, @snapshot]) end def download @@ -97,13 +92,12 @@ def export_submit # Export AND publish end def destroy - if @snapshot.has_doi? - flash[:error] = "You cannot delete a snapshot that has a DOI." - redirect_to polymorphic_path([@resource, @snapshot]) - else - @snapshot.destroy + if @snapshot.destroy flash[:notice] = "Snapshot successfully deleted" redirect_to polymorphic_path(@resource) + else + flash[:error] = @snapshot.errors.full_messages + redirect_to polymorphic_path([@resource, @snapshot]) end end @@ -143,11 +137,18 @@ def doi_minting_enabled? end end + def check_doi + if @snapshot.has_doi? + flash[:error] = "You cannot #{action_name} a snapshot that has a DOI." + redirect_to polymorphic_path([@resource, @snapshot]) + end + end + def metadata_params params.require(:metadata).permit(:access_right, :license, :embargo_date, :access_conditions, creators: [:name]).delete_if { |k,v| v.blank? } end def snapshot_params - params.require(:snapshot).permit(:title, :description) + params.fetch(:snapshot, {}).permit(:title, :description) end end diff --git a/app/models/snapshot.rb b/app/models/snapshot.rb index c92246baf9..cea87b5d8c 100644 --- a/app/models/snapshot.rb +++ b/app/models/snapshot.rb @@ -15,6 +15,7 @@ class Snapshot < ApplicationRecord delegate :md5sum, :sha1sum, to: :content_blob + validate :resource_creators_present?, on: :create validates :snapshot_number, uniqueness: { scope: %i[resource_type resource_id] } acts_as_doi_mintable(proxy: :resource, general_type: 'Collection') @@ -110,4 +111,10 @@ def parse_metadata def reindex_parent_resource ReindexingQueue.enqueue(resource) if saved_change_to_doi? end + + def resource_creators_present? + if resource.creators.empty? + errors.add(:base, "At least one creator is required. To add, go to Actions -> Manage #{resource.class.model_name.human}.") + end + end end diff --git a/app/views/snapshots/new.html.erb b/app/views/snapshots/new.html.erb index 17674ad440..5055a632f0 100644 --- a/app/views/snapshots/new.html.erb +++ b/app/views/snapshots/new.html.erb @@ -38,9 +38,9 @@
<% end %>
-
- <%= form_for [@resource, @resource.snapshots.build] do |f| -%> - <%= render :partial => "form", :locals => { :f => f, :action=>:new } -%> +
+ <%= form_for([@resource, @resource.snapshots.build], url: polymorphic_path([@resource, :snapshots], anchor: 'snapshot-metadata')) do |f| -%> + <%= render partial: 'form', locals: { f: f, action: :new } -%> <%= image_tag_for_key('publish', polymorphic_path(@resource, :action => :check_related_items), nil, {:method=>:post,:class => 'btn btn-default'}, "Publish full #{t(@resource.class.name.downcase)}") if excluded_items.any? %> diff --git a/lib/seek/research_objects/acts_as_snapshottable.rb b/lib/seek/research_objects/acts_as_snapshottable.rb index 30fc276266..26b17732c3 100644 --- a/lib/seek/research_objects/acts_as_snapshottable.rb +++ b/lib/seek/research_objects/acts_as_snapshottable.rb @@ -27,13 +27,10 @@ def is_snapshottable? end module InstanceMethods - def create_snapshot - if self.creators.empty? - errors.add(:base, "At least one creator is required. To add, go to Actions -> Manage #{self.class.model_name.human}.") - return nil - end + def create_snapshot(snapshot_metadata = {}) Rails.logger.debug("Creating snapshot for: #{self.class.name} #{id}") - snapshot = snapshots.create + snapshot = snapshots.build(snapshot_metadata) + return snapshot unless snapshot.save filename = "#{self.class.name.underscore}-#{id}-#{snapshot.snapshot_number}.ro.zip" blob = snapshot.build_content_blob(content_type: Mime::Type.lookup_by_extension('ro').to_s, original_filename: filename) diff --git a/test/functional/snapshots_controller_test.rb b/test/functional/snapshots_controller_test.rb index 11358a5d90..2c51b6815d 100644 --- a/test/functional/snapshots_controller_test.rb +++ b/test/functional/snapshots_controller_test.rb @@ -27,8 +27,6 @@ class SnapshotsControllerTest < ActionController::TestCase assert_response :success end - - test "can't get snapshot preview if no manage permissions" do user = FactoryBot.create(:user) investigation = FactoryBot.create(:investigation, policy: FactoryBot.create(:publicly_viewable_policy), @@ -98,8 +96,9 @@ class SnapshotsControllerTest < ActionController::TestCase end test 'snapshot title saved correctly' do - user = Factory(:user) - investigation = Factory(:investigation, policy: Factory(:publicly_viewable_policy), contributor: user.person) + user = FactoryBot.create(:user) + investigation = FactoryBot.create(:investigation, policy: FactoryBot.create(:publicly_viewable_policy), + contributor: user.person, creators: [FactoryBot.create(:user).person]) login_as(user) assert_difference('Snapshot.count') do post :create, params: { investigation_id: investigation, @@ -110,18 +109,21 @@ class SnapshotsControllerTest < ActionController::TestCase end test 'snapshot title and description correctly displayed' do - user = Factory(:user) - investigation = Factory(:investigation, policy: Factory(:publicly_viewable_policy), contributor: user.person, - description: 'Not a snapshot', title: 'My Investigation') + user = FactoryBot.create(:user) + investigation = FactoryBot.create(:investigation, policy: FactoryBot.create(:publicly_viewable_policy), + contributor: user.person, creators: [FactoryBot.create(:user).person], + description: 'Not a snapshot', title: 'My Investigation') login_as(user) - post :create, params: { investigation_id: investigation, - snapshot: { title: 'My first snapshot', description: 'Some info' } } - post :create, params: { investigation_id: investigation, - snapshot: { title: 'My second snapshot', description: 'Other info' } } - post :create, params: { investigation_id: investigation, snapshot: empty_snapshot } + + assert_difference('Snapshot.count', 3) do + investigation.create_snapshot(title: 'My first snapshot', description: 'Some info') + investigation.create_snapshot(title: 'My second snapshot', description: 'Other info') + investigation.create_snapshot + end get :show, params: { investigation_id: investigation, id: 1 } assert_response :success + assert_select 'h1', 'My first snapshot' assert_select 'div#description', 'Some info' assert_select 'div#snapshots' do @@ -130,16 +132,23 @@ class SnapshotsControllerTest < ActionController::TestCase assert_select 'a[data-tooltip=?]', 'Other info' assert_select 'a[href=?]', investigation_snapshot_path(investigation, 3), text: 'Snapshot 3' end + + get :show, params: { investigation_id: investigation, id: 3 } + assert_response :success + assert_select 'h1', 'Snapshot 3' end test 'snapshot shows captured state' do - user = Factory(:user) - investigation = Factory(:investigation, policy: Factory(:publicly_viewable_policy), contributor: user.person, - title: 'Old Title', description: 'Old description') + user = FactoryBot.create(:user) + investigation = FactoryBot.create(:investigation, policy: FactoryBot.create(:publicly_viewable_policy), + contributor: user.person, creators: [FactoryBot.create(:user).person], + title: 'Old Title', description: 'Old description') login_as(user) - post :create, params: { investigation_id: investigation, - snapshot: { title: 'My snapshot', description: 'Snapshot info' } } - investigation.update(title: 'New title', description: 'New description') + assert_difference('Snapshot.count') do + post :create, params: { investigation_id: investigation, + snapshot: { title: 'My snapshot', description: 'Snapshot info' } } + end + investigation.update!(title: 'New title', description: 'New description') get :show, params: { investigation_id: investigation, id: 1 } assert_response :success assert_select 'h1', 'My snapshot' @@ -203,8 +212,8 @@ class SnapshotsControllerTest < ActionController::TestCase assert_no_difference('Snapshot.count') do post :create, params: { "#{type}_id": snap.id } end - assert_redirected_to Seek::Util.routes.polymorphic_path(snap) - assert flash[:error].include?('creator is required') + assert_response :unprocessable_entity + assert assigns(:snapshot).errors.full_messages.any? { |e| e.include?('creator is required') } end end @@ -257,7 +266,7 @@ class SnapshotsControllerTest < ActionController::TestCase test 'edit button not shown for unauthorized users' do create_investigation_snapshot - login_as(Factory(:user)) + login_as(FactoryBot.create(:user)) get :show, params: { investigation_id: @investigation, id: @snapshot.snapshot_number } assert_select 'a', text: 'Edit', count: 0 end @@ -280,7 +289,7 @@ class SnapshotsControllerTest < ActionController::TestCase test "unauthorized user can't edit snapshot" do create_investigation_snapshot - login_as(Factory(:user)) + login_as(FactoryBot.create(:user)) get :edit, params: { investigation_id: @investigation, id: @snapshot.snapshot_number } assert_redirected_to investigation_path(@investigation) assert flash[:error] @@ -293,7 +302,7 @@ class SnapshotsControllerTest < ActionController::TestCase @snapshot.save get :edit, params: { investigation_id: @investigation, id: @snapshot.snapshot_number } assert_redirected_to investigation_snapshot_path(@investigation, @snapshot) - assert flash[:error].include?('DOI') + assert flash[:error].include?('edit a snapshot that has a DOI') end test 'authorized users can update snapshot' do @@ -309,7 +318,7 @@ class SnapshotsControllerTest < ActionController::TestCase test "unauthorized users can't update snapshot" do create_investigation_snapshot - login_as(Factory(:user)) + login_as(FactoryBot.create(:user)) put :update, params: { investigation_id: @investigation, id: @snapshot.snapshot_number, snapshot: { title: 'My mod snapshot', description: 'Snapshot mod info' } } assert_redirected_to investigation_path(@investigation) @@ -324,7 +333,7 @@ class SnapshotsControllerTest < ActionController::TestCase put :update, params: { investigation_id: @investigation, id: @snapshot.snapshot_number, snapshot: { title: 'My mod snapshot', description: 'Snapshot mod info' } } assert_redirected_to investigation_snapshot_path(@investigation, @snapshot) - assert flash[:error].include?('DOI') + assert flash[:error].include?('update a snapshot that has a DOI') end test 'can get confirmation when minting DOI for snapshot' do @@ -640,7 +649,7 @@ class SnapshotsControllerTest < ActionController::TestCase end assert_redirected_to investigation_snapshot_path(@investigation, @snapshot) - assert flash[:error].include?('DOI') + assert flash[:error].include?('destroy a snapshot that has a DOI') end test "can't delete snapshot without permission" do diff --git a/test/unit/snapshot_test.rb b/test/unit/snapshot_test.rb index 556e8d43b1..12162b250c 100644 --- a/test/unit/snapshot_test.rb +++ b/test/unit/snapshot_test.rb @@ -38,9 +38,7 @@ class SnapshotTest < ActiveSupport::TestCase end test 'snapshot title and description correctly set' do - s1 = @investigation.create_snapshot - s1.update_attribute(:title, 'My snapshot title') - s1.update_attribute(:description, 'My description') + s1 = @investigation.create_snapshot(title: 'My snapshot title', description: 'My description') assert_equal 'My snapshot title', s1.title assert_equal 'My description', s1.description end @@ -334,6 +332,26 @@ class SpecialSnapshotTestException < StandardError; end end end + test 'validates resource has creators on snapshot creation' do + i = FactoryBot.create(:investigation) + assert i.creators.empty? + snap = i.create_snapshot + refute snap.valid? + assert snap.errors.added?(:base, 'At least one creator is required. To add, go to Actions -> Manage Investigation.') + end + + test 'does not validate resource has creators on snapshot update' do + snap = @investigation.create_snapshot + assert snap.valid? + + disable_authorization_checks do + @investigation.creators = [] + end + + assert @investigation.reload.creators.empty? + assert snap.valid? + end + private def extract_keys(o, key) From 51691323ed5b2c7105173d248a8bf4b7c4ef9283 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 26 Sep 2024 15:39:51 +0100 Subject: [PATCH 10/13] Test snapshot zenodo metadata uses resource title & desc --- test/unit/snapshot_test.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/unit/snapshot_test.rb b/test/unit/snapshot_test.rb index 12162b250c..e19e135fc8 100644 --- a/test/unit/snapshot_test.rb +++ b/test/unit/snapshot_test.rb @@ -43,6 +43,13 @@ class SnapshotTest < ActiveSupport::TestCase assert_equal 'My description', s1.description end + test 'zenodo metadata uses resource title and description' do + s1 = @investigation.create_snapshot(title: 'My snapshot title', description: 'My description') + zm = s1.zenodo_metadata + assert_equal 'i1', zm[:title] + assert_equal 'not blank', zm[:description] + end + test 'sha1 and md5 checksum' do s1 = @investigation.create_snapshot refute_nil s1.md5sum From fa05ab7c2563f5365d96844c10f8bf34a860a0a3 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 26 Sep 2024 15:46:21 +0100 Subject: [PATCH 11/13] Tidy up ERB --- app/views/snapshots/edit.html.erb | 2 +- app/views/snapshots/show.html.erb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/snapshots/edit.html.erb b/app/views/snapshots/edit.html.erb index 36d10e5239..58a5178da9 100644 --- a/app/views/snapshots/edit.html.erb +++ b/app/views/snapshots/edit.html.erb @@ -1,7 +1,7 @@

Edit <%= t('snapshot') %>

<%= form_for [@resource, @snapshot] do |f| %> - <%= render :partial => "form", :locals => { :f => f, :action=>:edit } -%> + <%= render partial: 'form', locals: { f: f, action: :edit } -%> or <%= cancel_button polymorphic_path([@resource, @snapshot]) -%> <% end -%> diff --git a/app/views/snapshots/show.html.erb b/app/views/snapshots/show.html.erb index 84105ea257..071b1b5c6e 100644 --- a/app/views/snapshots/show.html.erb +++ b/app/views/snapshots/show.html.erb @@ -32,12 +32,12 @@

Created at: <%= date_as_string(@snapshot.created_at, true) %>

<%= panel('Resource') do %> - <%= render :partial => "general/item_title", :locals => {:item=>@resource, :title=>@snapshot.metadata['title']} %> + <%= render partial: 'general/item_title', locals: { item: @resource, title: @snapshot.metadata['title'] } %> <%= item_description @snapshot.metadata['description'] -%> <% end %> <%= panel('Contents') do %> - <%= render :partial => 'tree', locals: { resource: @snapshot.metadata, top: true } %> + <%= render partial: 'tree', locals: { resource: @snapshot.metadata, top: true } %> <% end %> <%= panel('Fingerprints') do %> From 7a8c37353fac8aaf11d8e0a95c5befdf8e35dfb5 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 26 Sep 2024 16:32:55 +0100 Subject: [PATCH 12/13] Children of excluded items should also be excluded in snapshot preview. Fixes #1998 --- app/views/snapshots/new.html.erb | 4 +- lib/seek/research_objects/generator.rb | 66 +++++++++----------- test/unit/research_objects/generator_test.rb | 45 +++++++++++++ 3 files changed, 75 insertions(+), 40 deletions(-) diff --git a/app/views/snapshots/new.html.erb b/app/views/snapshots/new.html.erb index 5055a632f0..366122022d 100644 --- a/app/views/snapshots/new.html.erb +++ b/app/views/snapshots/new.html.erb @@ -1,7 +1,5 @@ <%= render :partial => "general/page_title", :locals => { :title => 'Snapshot Preview' } %> -<% items = Seek::ResearchObjects::Generator.new(@resource).gather_entries(true).group_by(&:permitted_for_research_object?) %> -<% included_items = items[true] || [] %> -<% excluded_items = items[false] || [] %> +<% included_items, excluded_items = Seek::ResearchObjects::Generator.new(@resource).included_items %> <% included_text = capture do %> The following public resources will be included in the snapshot. diff --git a/lib/seek/research_objects/generator.rb b/lib/seek/research_objects/generator.rb index 3c90169fda..bcc3db5e76 100644 --- a/lib/seek/research_objects/generator.rb +++ b/lib/seek/research_objects/generator.rb @@ -8,7 +8,6 @@ class Generator def initialize(resource) @resource = resource - @bundled_resources = [] end # :call-seq @@ -32,44 +31,44 @@ def generate(file = nil) end # Recursively store metadata/files of this resource and its children. - def bundle(resource, parents = []) + def bundle(resource, bundled = Set.new, parents = []) + return if bundled.include?(resource) + return unless resource.permitted_for_research_object? + bundled << resource store_reference(resource, parents) store_metadata(resource, parents) store_files(resource, parents) if resource.is_asset? - subentries(resource).select(&:permitted_for_research_object?).each do |child| - bundle(child, (parents + [resource])) + children(resource).each do |child| + bundle(child, bundled, (parents + [resource])) end end - # Gather child resources of the given resource - def subentries(resource) - s = case resource - when Investigation - resource.studies + resource.assets - when Study - resource.assays + resource.assets - when Assay - resource.assets - else - [] - end - remove_duplicates(s) - end - - def all_subentries(resource) - subentries(resource).map do |sub| - all_subentries(sub) - end + [resource] + def children(resource) + case resource + when Investigation + resource.studies + resource.assets + when Study + resource.assays + resource.assets + when Assay + resource.assets + else + [] + end end - # collects the entries contained by the resource for inclusion in - # the research object - def gather_entries(show_all = false) - # This will break when used for non-ISA things: - entries = all_subentries(@resource).flatten - entries = entries.select(&:permitted_for_research_object?) unless show_all + def included_items(resource = @resource, included = Set.new, excluded = Set.new, parent_excluded = false) + return if included.include?(resource) || excluded.include?(resource) + permitted = !parent_excluded && resource.permitted_for_research_object? + if permitted + included << resource + else + excluded << resource + end + children(resource).each do |child| + included_items(child, included, excluded, !permitted) + end - entries + [included, excluded] end private @@ -127,13 +126,6 @@ def temp_file(filename, prefix = '') dir = Dir.mktmpdir(prefix) open(File.join(dir, filename), 'w+') end - - def remove_duplicates(resources) - unique_resources = resources - @bundled_resources - @bundled_resources += unique_resources - - unique_resources - end end end end diff --git a/test/unit/research_objects/generator_test.rb b/test/unit/research_objects/generator_test.rb index de2347051a..37be46322b 100644 --- a/test/unit/research_objects/generator_test.rb +++ b/test/unit/research_objects/generator_test.rb @@ -53,6 +53,51 @@ class GeneratorTest < ActiveSupport::TestCase file = Seek::ResearchObjects::Generator.new(assay).generate end + test 'check inclusion' do + inv = investigation + + included, excluded = Seek::ResearchObjects::Generator.new(inv).included_items + assert_empty excluded + inc = included.group_by { |a| a.class.name } + assert_equal 1, inc.delete('Investigation').count + assert_equal 1, inc.delete('Study').count + assert_equal 2, inc.delete('Assay').count + assert_equal 1, inc.delete('Publication').count + assert_equal 2, inc.delete('Sop').count + assert_equal 1, inc.delete('DataFile').count + assert_equal 3, inc.delete('Model').count + assert_empty inc + + # Make 1 model private + disable_authorization_checks do + @assay_asset5.asset.policy.update!(access_type: Policy::NO_ACCESS) + end + included, excluded = Seek::ResearchObjects::Generator.new(inv.reload).included_items + inc = included.group_by { |a| a.class.name } + exc = excluded.group_by { |a| a.class.name } + assert_equal 2, inc['Model'].count + assert_equal 1, exc['Model'].count + + # Make study private, which should excluded all descendents + disable_authorization_checks do + inv.studies.first.policy.update!(access_type: Policy::NO_ACCESS) + end + + included, excluded = Seek::ResearchObjects::Generator.new(inv.reload).included_items + inc = included.group_by { |a| a.class.name } + assert_equal 1, inc.delete('Investigation').count + assert_empty inc + + exc = excluded.group_by { |a| a.class.name } + assert_equal 1, exc.delete('Study').count + assert_equal 2, exc.delete('Assay').count + assert_equal 1, exc.delete('Publication').count + assert_equal 2, exc.delete('Sop').count + assert_equal 1, exc.delete('DataFile').count + assert_equal 3, exc.delete('Model').count + assert_empty exc + end + private def check_contents(file, inv) From f97633f938f9a710e3fef76ad4c58695c359ab58 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 26 Sep 2024 16:38:56 +0100 Subject: [PATCH 13/13] Test cleanup --- test/functional/homes_controller_test.rb | 9 +++------ test/unit/helpers/homes_helper_test.rb | 8 ++------ 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/test/functional/homes_controller_test.rb b/test/functional/homes_controller_test.rb index 2005afc65e..6cd96cf2c5 100644 --- a/test/functional/homes_controller_test.rb +++ b/test/functional/homes_controller_test.rb @@ -260,11 +260,9 @@ class HomesControllerTest < ActionController::TestCase test 'recently added and download should include snapshot' do person = FactoryBot.create(:person) snapshot1 = FactoryBot.create(:investigation, policy: FactoryBot.create(:publicly_viewable_policy), - title: 'inv with snap', contributor: person, creators: [person]).create_snapshot - snapshot1.update_attribute(:title, 'My snapshot') + title: 'inv with snap', contributor: person, creators: [person]).create_snapshot(title: 'My snapshot') snapshot2 = FactoryBot.create(:assay, policy: FactoryBot.create(:publicly_viewable_policy), - title: 'assay with snap', contributor: person, creators: [person]).create_snapshot - snapshot2.update_attribute(:title, 'My other snapshot') + title: 'assay with snap', contributor: person, creators: [person]).create_snapshot(title: 'My other snapshot') assert_difference 'ActivityLog.count', 2 do FactoryBot.create(:activity_log, action: 'create', activity_loggable: snapshot1, created_at: 1.day.ago, culprit: person.user) FactoryBot.create(:activity_log, action: 'download', activity_loggable: snapshot2, created_at: 1.day.ago, culprit: person.user) @@ -380,8 +378,7 @@ class HomesControllerTest < ActionController::TestCase sop = FactoryBot.create :sop, title: 'A new sop', contributor: person, policy: FactoryBot.create(:public_policy) assay = FactoryBot.create :assay, title: 'A new assay', contributor: person, policy: FactoryBot.create(:public_policy), creators: [person] - snapshot = assay.create_snapshot - snapshot.update_attribute(:title, 'My snapshot') + snapshot = assay.create_snapshot(title: 'My snapshot') FactoryBot.create :activity_log, activity_loggable: df, controller_name: 'data_files', culprit: person.user FactoryBot.create :activity_log, activity_loggable: sop, controller_name: 'sops', culprit: person.user diff --git a/test/unit/helpers/homes_helper_test.rb b/test/unit/helpers/homes_helper_test.rb index 984982b04c..c6a01cc8a7 100644 --- a/test/unit/helpers/homes_helper_test.rb +++ b/test/unit/helpers/homes_helper_test.rb @@ -61,11 +61,9 @@ def setup test 'should handle snapshots for download and recently added' do person = FactoryBot.create(:person) snapshot1 = FactoryBot.create(:investigation, policy: FactoryBot.create(:publicly_viewable_policy), - contributor: person, creators: [person]).create_snapshot - snapshot1.update_attribute(:title, 'My snapshot') + contributor: person, creators: [person]).create_snapshot(title: 'My snapshot') snapshot2 = FactoryBot.create(:assay, policy: FactoryBot.create(:publicly_viewable_policy), - contributor: person, creators: [person]).create_snapshot - snapshot2.update_attribute(:title, 'My other snapshot') + contributor: person, creators: [person]).create_snapshot(title: 'My other snapshot') FactoryBot.create(:activity_log, action: 'create', activity_loggable: snapshot1, created_at: 1.day.ago, culprit: person.user) FactoryBot.create(:activity_log, action: 'download', activity_loggable: snapshot2, created_at: 1.day.ago, culprit: person.user) @@ -74,8 +72,6 @@ def setup assert_equal 1, added_hash.count added = added_hash.first - pp added_hash - pp added assert_equal 'Snapshot', added[:type] assert_equal snapshot1.title.to_s, added[:title] assert_equal investigation_snapshot_path(snapshot1.resource, snapshot1), added[:url]