Merge pull request #842 from ElixirTeSS/issue/collection-index-bug

Fix issue reindexing collection with content. Fixes #841

Author: fbacall

Gemfile

@@ -189,7 +189,7 @@ group :development do
 end
 
 group :test do
-  gem 'fakeredis', git: 'https://github.com/artygus/fakeredis/', ref: 'f68bd4f'
+  gem 'fakeredis', git: 'https://github.com/guilleiguaran/fakeredis/', ref: 'f68bd4f'
   gem 'minitest', '5.14.4'
   gem 'rails-controller-testing'
   gem 'vcr', '~> 6.1.0'

Gemfile.lock

@@ -6,7 +6,7 @@ GIT
       linkeddata (~> 3.2.0)
 
 GIT
-  remote: https://github.com/artygus/fakeredis/
+  remote: https://github.com/guilleiguaran/fakeredis/
   revision: f68bd4f8d2b87c6445b9166447c1752c6236d63b
   ref: f68bd4f
   specs:

app/models/collection.rb

@@ -16,12 +16,8 @@ class Collection < ApplicationRecord
   # Remove trailing and squeezes (:squish option) white spaces inside the string (before_validation):
   # e.g. "James     Bond  " => "James Bond"
   auto_strip_attributes :title, :description, :image_url, :squish => false
-  after_save do
-    index_items if saved_change_to_title?
-  end
-  after_destroy do
-    index_items
-  end
+
+  after_commit :index_items, if: :title_previously_changed?
 
   validates :title, presence: true
 
@@ -97,6 +93,6 @@ def scientific_topics
   def index_items
     return unless TeSS::Config.solr_enabled
 
-    items.each(&:solr_index)
+    items.each(&:reindex_resource)
   end
 end

app/models/collection_item.rb

@@ -8,8 +8,7 @@ class CollectionItem < ApplicationRecord
 
   before_create :set_order
   after_save :log_activity
-  after_create :solr_index
-  after_destroy :solr_index
+  after_commit :reindex_resource, on: [:create, :destroy]
 
   def log_activity
     self.collection.create_activity(:add_item, owner: User.current_user,
@@ -21,15 +20,15 @@ def log_activity
                                                 collection_title: self.collection.title })
   end
 
+  def reindex_resource
+    # we should consider doing this in a background job if it turns out to be slow
+    # when curating large collections
+    resource.solr_index if TeSS::Config.solr_enabled
+  end
+
   private
 
   def set_order
     self.order ||= (collection.items.maximum(:order) || 0) + 1
   end
-
-  def solr_index
-    # we should consider doing this in a background job if it turns out to be slow
-    # when curating large collections
-    resource.solr_index if TeSS::Config.solr_enabled
-  end
 end

test/models/collection_test.rb

@@ -1,6 +1,28 @@
 require 'test_helper'
 
 class CollectionTest < ActiveSupport::TestCase
+  teardown do
+    DummyMaterial.clear_index!
+  end
+
+  class DummyMaterial < ::Material
+    def self.index
+      (@index ||= Hash.new).values.flatten.uniq
+    end
+
+    def self.add_to_index(m)
+      index
+      @index[m.id] = m.reload.collections.to_a
+    end
+
+    def self.clear_index!
+      @index = Hash.new
+    end
+
+    def solr_index
+      self.class.add_to_index(self)
+    end
+  end
 
   test 'visibility scope' do
     assert_not_includes Collection.visible_by(nil), collections(:secret_collection)
@@ -92,4 +114,48 @@ class CollectionTest < ActiveSupport::TestCase
       assert collection.destroy
     end
   end
+
+  test 'index collection resources when collection created or destroyed' do
+    user = users(:regular_user)
+    material = materials(:good_material).becomes(DummyMaterial)
+    with_settings(solr_enabled: true) do
+      collection = user.collections.create!(title: 'test 123', materials: [material])
+
+      assert collection
+      assert_includes DummyMaterial.index, collection
+      assert_equal 1, DummyMaterial.index.length
+
+      assert collection.destroy
+      assert_equal 0, DummyMaterial.index.length
+    end
+  end
+
+  test 'index collection resources when collection renamed' do
+    user = users(:regular_user)
+    material = materials(:good_material).becomes(DummyMaterial)
+    collection = user.collections.create!(title: 'test 123', materials: [material])
+    assert collection
+
+    with_settings(solr_enabled: true) do
+      assert_equal 0, DummyMaterial.index.length
+
+      collection.update!(title: 'Hello world')
+      assert_includes DummyMaterial.index, collection
+      assert_equal 1, DummyMaterial.index.length
+    end
+  end
+
+  test 'do not index collection resources when collection updated without renaming' do
+    user = users(:regular_user)
+    material = materials(:good_material).becomes(DummyMaterial)
+    collection = user.collections.create!(title: 'test 123', materials: [material])
+    assert collection
+
+    with_settings(solr_enabled: true) do
+      assert_equal 0, DummyMaterial.index.length
+
+      collection.update!(description: 'hello')
+      assert_equal 0, DummyMaterial.index.length
+    end
+  end
 end