Children of excluded items should also be excluded in snapshot preview.

Fixes #1998

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.

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

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)