Merge pull request #1065 from MITLibraries/transfer_processing_perfor…

…mance

Transfer processing performance

README.md

@@ -352,17 +352,28 @@ unaffected by this altenative.
 
 ## Resetting counter cache
 
-We use counter cache to improve the application's performance. Currently, all counters are associated with the Thesis
-model. You can run the following rake task to update the counters:
+We use counter caches to improve the application's performance. These commands should only need to be run at
+most once in a given app to update counter columns on existing records. After that, the counters will update
+automatically. If you suspect something has gone wrong with the counters, you can re-run them to ensure they are
+accurate with no side effects other than the counters definitely being wrong for a few minutes as they recalculate.
+
+These rake tasks should be updated as we add new counter cache columns.
+
+### Thesis counter_cache
+
+You can run the following rake task to update the counters:
 
 ```shell
 heroku run rails cache:reset_thesis_counters --app TARGET-HEROKU-APP
 ```
 
-This command should only be run once in a given app to update existing the counter columns on existing records. After
-that, the counters will update automatically.
+### Transfer counter_cache
 
-The rake task should be updated as we add new counter cache columns.
+This is a non-standard counter_cache that needs to calculate counters based on Theses and not just Transfers.
+
+```shell
+heroku run rails cache:reset_transfer_counters --app TARGET-HEROKU-APP
+```
 
 # Local deployment
 

app/controllers/thesis_controller.rb

@@ -122,6 +122,9 @@ def process_theses_update
         flash[:success] += '<p>The following files were removed from this thesis. They can still be found attached to their original transfer, via the following links:</p><ul>'.html_safe
         removed.each do |r|
           flash[:success] += "<li><a href='/transfer/#{r['transfer_id']}'>#{r['filename']}</a></li>".html_safe
+
+          # update the original transfer so unassigned_files_count will be accurate
+          Transfer.find(r['transfer_id']).save
         end
         flash[:success] += '</ul>'.html_safe
       end
@@ -144,6 +147,7 @@ def deleted_file_list
     list = []
     return list unless thesis_params['files_attachments_attributes']
 
+    Rails.logger.debug('TRANSFER_COUNTS: Files count changed on thesis, expect updated Transfer count logs')
     thesis_params['files_attachments_attributes'].values.select { |item| item['_destroy'] == '1' }.each do |file|
       needle = ActiveStorage::Attachment.find_by(id: file['id']).blob
       list.append({

app/controllers/transfer_controller.rb

@@ -46,11 +46,12 @@ def files
       thesis.files.attach(file.blob)
       flash[:success] += "#{file.filename}<br>".html_safe
     end
+    transfer.save # triggers update to files_counts
     redirect_to transfer_path(transfer.id, view_all: params[:view_all] || 'false')
   end
 
   def select
-    @transfer = Transfer.all
+    @transfers = Transfer.all.with_attached_files.includes(:user).includes(:department)
   end
 
   def show

app/models/transfer.rb

@@ -13,6 +13,7 @@
 class Transfer < ApplicationRecord
   belongs_to :user
   belongs_to :department
+  before_save :update_files_count
 
   has_many_attached :files
 
@@ -74,9 +75,29 @@ def split_graduation_date
 
   def unassigned_files
     count = 0
-    files.blobs.all.each do |blob|
-      count += 1 if blob.attachment_ids.count == 1
+    if files.blobs.is_a?(Array)
+      files.blobs.each do |blob|
+        count += 1 if blob.attachment_ids.count == 1
+      end
+    else
+      files.blobs.all.each do |blob|
+        count += 1 if blob.attachment_ids.count == 1
+      end
     end
     count
   end
+
+  # This is triggered on before_save, but we must also remember to call Transfer#save to trigger this
+  # any time we are changing the files attached in a manner that wouldn't update Transfer directly.
+  # This can happen during the Transfer processing workflow (attaching files to Theses) and Thesis
+  # processing workflows (removing files from Theses)
+  def update_files_count
+    Rails.logger.debug("TRANSFER_COUNTS: Initial files_count for thesis #{id} is #{files_count}")
+    self.files_count = files.count
+    Rails.logger.debug("TRANSFER_COUNTS: Updated files_count for thesis #{id} is #{files_count}")
+
+    Rails.logger.debug("TRANSFER_COUNTS: Initial unassigned_files_count for thesis #{id} is #{unassigned_files_count}")
+    self.unassigned_files_count = unassigned_files
+    Rails.logger.debug("TRANSFER_COUNTS: Updated unassigned_files_count for thesis #{id} is #{unassigned_files_count}")
+  end
 end

app/views/thesis/process_theses.html.erb

@@ -199,7 +199,8 @@
                                aria: { describedby: 'thesis_date_ids-hint' }
                                } %>
           <%= f.input :graduation_year, label: 'Year *',
-                                        collection: [Date.today.last_year.strftime('%Y'),
+                                        collection: [Date.today.last_year.last_year.strftime('%Y'),
+                                                     Date.today.last_year.strftime('%Y'),
                                                      Date.today.strftime('%Y'),
                                                      Date.today.next_year.strftime('%Y')],
                               wrapper_html: { class: 'field-wrap' },

app/views/transfer/_transfer.html.erb

@@ -3,6 +3,6 @@
   <td data-sort="<%= transfer.grad_date %>"><%= transfer.graduation_month[...3] %> <%= transfer.graduation_year %></td>
   <td><%= transfer.department.name_dw %></td>
   <td><%= transfer.user.display_name %></td>
-  <td data-sort="<%= transfer.unassigned_files %>"><%= transfer.unassigned_files %> / <%= transfer.files.count %></td>
+  <td data-sort="<%= transfer.unassigned_files_count %>"><%= transfer.unassigned_files_count %> / <%= transfer.files_count %></td>
   <td><%= transfer.note.present? ? "<span title='#{transfer.note}'>#{transfer.note[..10]}...</span>".html_safe : "" %></td>
 </tr>

db/migrate/20221018145756_add_missing_indexes.rb

@@ -0,0 +1,7 @@
+class AddMissingIndexes < ActiveRecord::Migration[6.1]
+  def change
+    add_index :active_storage_attachments, [:record_id, :record_type]
+    add_index :active_storage_variant_records, :blob_id
+    add_index :degrees, :degree_type_id
+  end
+end

db/migrate/20221018182433_add_files_count_to_transfers.rb

@@ -0,0 +1,10 @@
+class AddFilesCountToTransfers < ActiveRecord::Migration[6.1]
+  def self.up
+    add_column :transfers, :files_count, :integer, null: false, default: 0
+    add_column :transfers, :unassigned_files_count, :integer, null: false, default: 0
+  end
+
+  def self.down
+    remove_column :transfers, :files_count
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2022_10_17_163130) do
+ActiveRecord::Schema.define(version: 2022_10_18_182433) do
 
   create_table "active_storage_attachments", force: :cascade do |t|
     t.string "name", null: false
@@ -21,6 +21,7 @@
     t.text "description"
     t.integer "purpose"
     t.index ["blob_id"], name: "index_active_storage_attachments_on_blob_id"
+    t.index ["record_id", "record_type"], name: "index_active_storage_attachments_on_record_id_and_record_type"
     t.index ["record_type", "record_id", "name", "blob_id"], name: "index_active_storage_attachments_uniqueness", unique: true
   end
 
@@ -40,6 +41,7 @@
     t.integer "blob_id", null: false
     t.string "variation_digest", null: false
     t.index ["blob_id", "variation_digest"], name: "index_active_storage_variant_records_uniqueness", unique: true
+    t.index ["blob_id"], name: "index_active_storage_variant_records_on_blob_id"
   end
 
   create_table "advisor_theses", id: false, force: :cascade do |t|
@@ -99,6 +101,7 @@
     t.string "name_dspace"
     t.integer "degree_type_id"
     t.index ["code_dw"], name: "index_degrees_on_code_dw", unique: true
+    t.index ["degree_type_id"], name: "index_degrees_on_degree_type_id"
   end
 
   create_table "delayed_jobs", force: :cascade do |t|
@@ -222,6 +225,8 @@
     t.datetime "created_at", precision: 6, null: false
     t.datetime "updated_at", precision: 6, null: false
     t.text "note"
+    t.integer "files_count", default: 0, null: false
+    t.integer "unassigned_files_count", default: 0, null: false
     t.index ["department_id"], name: "index_transfers_on_department_id"
     t.index ["user_id"], name: "index_transfers_on_user_id"
   end

lib/tasks/cache.rake

@@ -6,4 +6,13 @@ namespace :cache do
     end
     Rails.logger.info('Thesis model counters have been updated.')
   end
+
+  desc 'Reset counter cache associated with the transfer model'
+  task reset_transfer_counters: :environment do
+    Rails.logger.info('Transfer counters update is starting.')
+    # The counter caches are non-standard in this model and are updated via a before_save hook so we update
+    # them by calling save.
+    Transfer.find_each(&:save)
+    Rails.logger.info('Transfer counters have been updated.')
+  end
 end

test/models/transfer_test.rb

@@ -138,14 +138,31 @@ class TransferTest < ActiveSupport::TestCase
   end
 
   test 'unassigned_files updates as needed' do
-    assert_equal 0, @transfer.unassigned_files
-
-    @newfile = Rails.root.join('test', 'fixtures', 'files', 'a_pdf.pdf')
-    @transfer.files.attach(io: File.open(@newfile), filename: 'a_pdf.pdf')
-    assert_equal 1, @transfer.unassigned_files
-
+    # confirm an initial state on a transfer
+    @transfer.save # required to update initial counts from fixtured data
+    assert_equal 0, @transfer.unassigned_files_count
+    assert_equal 1, @transfer.files_count
+
+    # add a file to a transfer and confirm it is not assigned to a thesis
+    @newfile = Rails.root.join('test', 'fixtures', 'files', 'b_pdf.pdf')
+    @transfer.files.attach(io: File.open(@newfile), filename: 'b_pdf.pdf')
+    @transfer.save
+    assert_equal 1, @transfer.unassigned_files_count
+    assert_equal 2, @transfer.files_count
+
+    # assign that file to a thesis to confirm the unassigned changes appropriately
+    file = @transfer.files.last
     @thesis = theses(:one)
-    @thesis.files.attach(@transfer.files.blobs.last)
-    assert_equal 0, @transfer.unassigned_files
+    @thesis.files.attach(file.blob)
+    @thesis.save
+    @transfer.save
+    assert_equal 0, @transfer.unassigned_files_count
+
+    # unassign that file from a thesis to confirm the unassigned count changes appropriately
+    @thesis.files = []
+    @thesis.save
+    @transfer.save
+    assert_equal 1, @transfer.unassigned_files_count
+    assert_equal 2, @transfer.files_count
   end
 end