Snapshots

app/controllers/snapshots_controller.rb

@@ -3,24 +3,25 @@
 
 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 :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 = @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
 
@@ -39,6 +40,18 @@ def show
   def new
   end
 
+  def edit
+  end
+
+  def update
+    if @snapshot.update(snapshot_params)
+      flash[:notice] = "Snapshot updated"
+      redirect_to polymorphic_path([@resource, @snapshot])
+    else
+      render :edit, status: :unprocessable_entity
+    end
+  end
+
   def download
     @content_blob = @snapshot.content_blob
     send_file @content_blob.filepath,
@@ -79,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
 
@@ -125,7 +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.fetch(:snapshot, {}).permit(:title, :description)
+  end
 end

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.description)
+  end
+
+  def snapshot_display_name(snapshot)
+    return snapshot.title if snapshot.title.present?
+    return "Snapshot #{snapshot.snapshot_number}" if snapshot.persisted?
+    "Snapshot #{snapshot.potential_snapshot_number}"
   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')
@@ -33,22 +34,6 @@ def metadata
     @ro_metadata ||= parse_metadata
   end
 
-  def title
-    if content_blob.present?
-      metadata['title']
-    else
-      'incomplete snapshot'
-    end
-  end
-
-  def description
-    if content_blob.present?
-      metadata['description']
-    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 +82,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
@@ -115,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/_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 %>

app/views/snapshots/_form.html.erb

@@ -0,0 +1,13 @@
+<%= error_messages_for :snapshot %>
+
+<div class="form-group">
+  <%= f.label :title -%><br/>
+  <%= f.text_field :title, class: 'form-control', placeholder: snapshot_display_name(f.object) %>
+</div>
+
+<div class="form-group">
+  <%= f.label :description -%><br/>
+  <%= f.text_area :description, rows: 3, class: 'form-control' %>
+</div>
+
+<%= f.submit class: 'btn btn-primary' %>

app/views/snapshots/_snapshots.html.erb

@@ -3,7 +3,7 @@
     <% snapshots.order("created_at DESC").each do |snapshot| %>
         <div>
           <% if snapshot == selected %>
-              <%= content_tag :strong, "Snapshot #{snapshot.snapshot_number}" %>
+              <%= content_tag :strong, snapshot_display_name(snapshot), 'data-tooltip' => tooltip(snapshot.description) %>
           <% else %>
               <%= snapshot_link(resource,snapshot) %>
           <% end %>

app/views/snapshots/edit.html.erb

@@ -0,0 +1,7 @@
+<h1>Edit <%= t('snapshot') %></h1>
+
+<%= form_for [@resource, @snapshot] do |f| %>
+    <%= render partial: 'form', locals: { f: f, action: :edit } -%>
+
+    or <%= cancel_button polymorphic_path([@resource, @snapshot]) -%>
+<% end -%>

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.
@@ -38,10 +36,12 @@
       </div>
   <% end %>
 </div>
+<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 } -%>
 
-<%= link_to('Create Snapshot', polymorphic_path([@resource, :snapshots]),
-            :method => :post, :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) -%>
+    or <%= cancel_button polymorphic_path(@resource) -%>
+  <% end -%>
+</div>

app/views/snapshots/show.html.erb

@@ -1,8 +1,5 @@
-<% title = capture do %>
-    <%= @snapshot.title -%>
-    <small>(snapshot <%= @snapshot.snapshot_number -%>)</small>
-<% 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' } %>
 
 <div class="row">
   <div class="col-md-9 col-sm-8">
@@ -34,8 +31,13 @@
 
     <p><strong>Created at:</strong> <%= date_as_string(@snapshot.created_at, true) %></p>
 
+    <%= panel('Resource') do %>
+      <%= 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 %>

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

db/migrate/20221206114653_add_snapshot_title_to_snapshots.rb

@@ -0,0 +1,6 @@
+class AddSnapshotTitleToSnapshots < ActiveRecord::Migration[6.1]
+  def change
+    add_column :snapshots, :title, :string
+    add_column :snapshots, :description, :text
+  end
+end

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 "title"
+    t.text "description"
   end
 
   create_table "sop_auth_lookup", force: :cascade do |t|

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)

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