refactor Spotlight::Resource reindexing stats tracking and the Spotli…

…ght::ReindexProgress that used to use it

* refactor the way Spotlight::Resource tracks indexing stats
  * add items_reindexed_estimate field to create_spotlight_reindexing_log_entries migration
  * resource.rb: refactor Spotlight::Resource#reindex so that it takes an optional ReindexingLogEntry; use the log entry to track the stats that Resource used to keep on itself.  remove unused methods.  update tests.
  * reindex_job.rb: use before_perform hook to update items_reindexed_estimate on the reindexing log entry, if there is one.  pass the log entry to resource.reindex.  add tests.
* refactor Spotlight::ReindexProgress to get a ReindexingLogEntry from an Exhibit, instead of dealing with a Resource list
  * refactor Spotlight::ReindexProgress so that it takes a Spotlight::Exhibit (instead of a Resource list), and uses the current ReindexingLogEntry from the Exhibit (if there is one).  fix Exhibit's call to ReindexProgress and have it initialize the log entries it creates to 0. update exhibit_spec.rb.
    * rewrite reindex_progress_spec.rb
  * reindex_monitor_spec.rb: fix the simulation of a recent reindexing job, slightly relax expectations for reindexing progress text to accommodate multi-digit numbers

app/jobs/spotlight/reindex_job.rb

@@ -4,8 +4,14 @@ module Spotlight
   class ReindexJob < ActiveJob::Base
     queue_as :default
 
-    before_enqueue do |job|
-      resource_list(job.arguments.first).each(&:waiting!)
+    before_perform do |job|
+      job_log_entry = log_entry(job)
+      next unless job_log_entry
+
+      items_reindexed_estimate = resource_list(job.arguments.first).sum do |resource|
+        resource.document_builder.documents_to_index.size
+      end
+      job_log_entry.update(items_reindexed_estimate: items_reindexed_estimate)
     end
 
     around_perform do |job, block|
@@ -22,8 +28,10 @@ class ReindexJob < ActiveJob::Base
       job_log_entry.succeeded! if job_log_entry
     end
 
-    def perform(exhibit_or_resources, _log_entry = nil)
-      resource_list(exhibit_or_resources).each(&:reindex)
+    def perform(exhibit_or_resources, log_entry = nil)
+      resource_list(exhibit_or_resources).each do |resource|
+        resource.reindex(log_entry)
+      end
     end
 
     private

app/models/spotlight/exhibit.rb

@@ -101,7 +101,7 @@ def requested_by
     end
 
     def reindex_progress
-      @reindex_progress ||= ReindexProgress.new(resources) if resources
+      @reindex_progress ||= ReindexProgress.new(self)
     end
 
     protected
@@ -111,7 +111,7 @@ def sanitize_description
     end
 
     def new_reindexing_log_entry(user = nil)
-      Spotlight::ReindexingLogEntry.create(exhibit: self, user: user, items_reindexed_count: resources.size, job_status: 'unstarted')
+      Spotlight::ReindexingLogEntry.create(exhibit: self, user: user, items_reindexed_count: 0, job_status: 'unstarted')
     end
   end
 end

app/models/spotlight/reindex_progress.rb

@@ -2,51 +2,47 @@ module Spotlight
   ##
   # ReindexProgress is a class that models the progress of reindexing a list of resources
   class ReindexProgress
-    def initialize(resource_list)
-      @resources = if resource_list.present?
-                     resource_list.order('updated_at')
-                   else
-                     Spotlight::Resource.none
-                   end
+    attr_reader :exhibit
+
+    def initialize(exhibit)
+      @exhibit = exhibit
     end
 
     def recently_in_progress?
-      any_waiting? || (!!finished_at && finished_at > Spotlight::Engine.config.reindex_progress_window.minutes.ago)
+      return false if current_log_entry.blank?
+      return true if current_log_entry.in_progress?
+
+      current_log_entry.end_time.present? && (current_log_entry.end_time > Spotlight::Engine.config.reindex_progress_window.minutes.ago)
     end
 
     def started_at
-      return unless resources.present?
-
-      enqueued_resources = resources.select(&:enqueued_at?)
-
-      return unless enqueued_resources.any?
-
-      @started ||= enqueued_resources.min_by(&:enqueued_at).enqueued_at
+      current_log_entry.try(:start_time)
     end
 
     def updated_at
-      @updated ||= resources.maximum(:updated_at) || started_at
+      current_log_entry.try(:updated_at)
     end
 
     def finished?
-      completed_resources.present? && !any_waiting?
+      return false if current_log_entry.blank?
+      current_log_entry.succeeded? || current_log_entry.failed?
     end
 
     def finished_at
-      return unless finished?
-      @finished ||= completed_resources.max_by(&:last_indexed_finished).last_indexed_finished
+      current_log_entry.try(:end_time)
     end
 
     def total
-      @total ||= resources.map(&:last_indexed_estimate).compact.sum
+      current_log_entry.try(:items_reindexed_estimate)
     end
 
     def completed
-      @completed ||= resources.map(&:last_indexed_count).compact.sum
+      current_log_entry.try(:items_reindexed_count)
     end
 
     def errored?
-      resources.any?(&:errored?)
+      return false if current_log_entry.blank?
+      current_log_entry.failed?
     end
 
     def as_json(*)
@@ -63,10 +59,8 @@ def as_json(*)
 
     private
 
-    attr_reader :resources
-
-    def any_waiting?
-      resources.any?(&:waiting?)
+    def current_log_entry
+      exhibit.reindexing_log_entries.where.not(job_status: 'unstarted').first
     end
 
     def localized_start_time
@@ -83,9 +77,5 @@ def localized_updated_time
       return unless updated_at
       I18n.l(updated_at, format: :short)
     end
-
-    def completed_resources
-      resources.completed
-    end
   end
 end

app/models/spotlight/resource.rb

@@ -16,19 +16,8 @@ class Resource < ActiveRecord::Base
     has_many :solr_document_sidecars
 
     serialize :data, Hash
-    store :metadata, accessors: [
-      :enqueued_at,
-      :last_indexed_estimate,
-      :last_indexed_count,
-      :last_index_elapsed_time,
-      :last_indexed_finished
-    ], coder: JSON
 
-    enum index_status: [:waiting, :completed, :errored]
-
-    around_index :reindex_with_logging
     after_index :commit
-    after_index :completed!
     after_index :touch_exhibit!
 
     ##
@@ -42,27 +31,9 @@ def save_and_index(*args)
     ##
     # Enqueue an asynchronous reindexing job for this resource
     def reindex_later
-      waiting!
       Spotlight::ReindexJob.perform_later(self)
     end
 
-    def waiting!
-      update(enqueued_at: Time.zone.now)
-      super
-    end
-
-    def enqueued_at
-      cast_to_date_time(super)
-    end
-
-    def enqueued_at?
-      enqueued_at.present?
-    end
-
-    def last_indexed_finished
-      cast_to_date_time(super)
-    end
-
     def document_model
       exhibit.blacklight_config.document_model if exhibit
     end
@@ -72,43 +43,28 @@ def document_model
       # Index the result of {#to_solr} into the index in batches of {#batch_size}
       #
       # @return [Integer] number of records indexed
-      def reindex
+      # rubocop:disable Metrics/MethodLength
+      def reindex(reindexing_log_entry = nil)
         benchmark "Reindexing #{self} (batch size: #{batch_size})" do
           count = 0
 
           run_callbacks :index do
             document_builder.documents_to_index.each_slice(batch_size) do |batch|
               write_to_index(batch)
-              update(last_indexed_count: (count += batch.length))
+              count += batch.length
+              reindexing_log_entry.update(items_reindexed_count: count) if reindexing_log_entry
             end
 
             count
           end
         end
       end
+      # rubocop:enable Metrics/MethodLength
 
       def document_builder
         @document_builder ||= document_builder_class.new(self)
       end
 
-      protected
-
-      def reindex_with_logging
-        time_start = Time.zone.now
-
-        update(indexed_at: time_start,
-               last_indexed_estimate: document_builder.documents_to_index.size,
-               last_indexed_finished: nil,
-               last_index_elapsed_time: nil)
-
-        count = yield
-
-        time_end = Time.zone.now
-        update(last_indexed_count: count,
-               last_indexed_finished: time_end,
-               last_index_elapsed_time: time_end - time_start)
-      end
-
       private
 
       def blacklight_solr
@@ -144,16 +100,6 @@ def touch_exhibit!
       def write?
         Spotlight::Engine.config.writable_index
       end
-
-      def cast_to_date_time(value)
-        return unless value
-
-        if defined? ActiveModel::Type::DateTime
-          ActiveModel::Type::DateTime.new.cast(value)
-        else
-          ActiveRecord::Type::DateTime.new.type_cast_from_database(value)
-        end
-      end
     end
   end
 end

db/migrate/20170105222939_create_spotlight_reindexing_log_entries.rb

@@ -2,6 +2,7 @@ class CreateSpotlightReindexingLogEntries < ActiveRecord::Migration
   def change
     create_table :spotlight_reindexing_log_entries do |t|
       t.integer :items_reindexed_count
+      t.integer :items_reindexed_estimate
       t.datetime :start_time
       t.datetime :end_time
       t.integer :job_status

spec/features/javascript/reindex_monitor_spec.rb

@@ -6,16 +6,16 @@
   let(:exhibit_curator) { FactoryGirl.create(:exhibit_curator, exhibit: exhibit) }
 
   before do
-    resources.each(&:waiting!)
+    FactoryGirl.create(:in_progress_reindexing_log_entry, exhibit: exhibit, items_reindexed_estimate: 5)
     login_as exhibit_curator
     visit spotlight.admin_exhibit_catalog_path(exhibit)
   end
 
   it 'is rendered on the item admin page' do
     expect(page).to have_css('.panel.index-status', visible: true)
     within('.panel.index-status') do
-      expect(page).to have_css('p', text: /Began reindexing a total of \d items/)
-      expect(page).to have_css('p', text: /Reindexed \d of \d items/)
+      expect(page).to have_css('p', text: /Began reindexing a total of \d+ items/)
+      expect(page).to have_css('p', text: /Reindexed \d+ of \d+ items/)
     end
   end
 end

spec/jobs/spotlight/reindex_job_spec.rb

@@ -46,6 +46,18 @@
         expect(log_entry).to receive(:failed!)
         expect { subject.perform_now }.to raise_error unexpected_error
       end
+
+      it 'updates the items_reindexed_estimate field on the log entry' do
+        expect(log_entry).to receive(:update).with(items_reindexed_estimate: 1)
+        subject.perform_now
+      end
+
+      it 'passes log_entry to the resource.reindex call' do
+        # ActiveJob will reload the collection, so we go through a little trouble:
+        expect_any_instance_of(Spotlight::Resource).to receive(:reindex).with(log_entry).exactly(:once)
+        # expect(resource).to receive(:reindex).with(log_entry)
+        subject.perform_now
+      end
     end
   end
 

spec/models/spotlight/exhibit_spec.rb

@@ -190,7 +190,7 @@
 
   describe '#reindex_later' do
     subject { FactoryGirl.create(:exhibit) }
-    let(:log_entry) { Spotlight::ReindexingLogEntry.new(exhibit: subject, user: user, items_reindexed_count: subject.resources.size) }
+    let(:log_entry) { Spotlight::ReindexingLogEntry.new(exhibit: subject, user: user, items_reindexed_count: 0) }
 
     context 'user is omitted' do
       let(:user) { nil }
@@ -215,6 +215,25 @@
     end
   end
 
+  describe '#new_reindexing_log_entry' do
+    let(:user) { FactoryGirl.build(:user) }
+    it 'returns a properly configured Spotlight::ReindexingLogEntry instance' do
+      reindexing_log_entry = subject.send(:new_reindexing_log_entry, user)
+      expect(reindexing_log_entry.exhibit).to eq subject
+      expect(reindexing_log_entry.user).to eq user
+      expect(reindexing_log_entry.items_reindexed_count).to eq 0
+      expect(reindexing_log_entry.unstarted?).to be true
+    end
+
+    it 'does not require user the user parameter' do
+      reindexing_log_entry = subject.send(:new_reindexing_log_entry)
+      expect(reindexing_log_entry.exhibit).to eq subject
+      expect(reindexing_log_entry.user).to be nil
+      expect(reindexing_log_entry.items_reindexed_count).to eq 0
+      expect(reindexing_log_entry.unstarted?).to be true
+    end
+  end
+
   describe '#solr_documents' do
     let(:blacklight_config) { Blacklight::Configuration.new }
     let(:slug) { 'some_slug' }
@@ -284,7 +303,9 @@
 
   describe '#reindex_progress' do
     it 'returns a Spotlight::ReindexProgress' do
-      expect(subject.reindex_progress).to be_a Spotlight::ReindexProgress
+      reindex_progress = subject.reindex_progress
+      expect(reindex_progress).to be_a Spotlight::ReindexProgress
+      expect(reindex_progress.exhibit).to eq subject
     end
   end
 end