Better snapshot validation & error handling

app/controllers/snapshots_controller.rb

@@ -7,21 +7,21 @@ 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]
 
   include Zenodo::Oauth2::SessionHelper
   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

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

app/views/snapshots/new.html.erb

@@ -38,9 +38,9 @@
       </div>
   <% end %>
 </div>
-<div class="asset_form">
-  <%= form_for [@resource, @resource.snapshots.build] do |f| -%>
-    <%= render :partial => "form", :locals => { :f => f, :action=>:new } -%>
+<div class="asset_form" id="snapshot-metadata">
+  <%= 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? %>
 

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)

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

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)