From 8f6dd708a10eb92ac92b372bbf4b5bad9f7e2357 Mon Sep 17 00:00:00 2001 From: Kirk Wang Date: Wed, 18 Sep 2024 11:45:05 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fix=20importers=20sorting=20for?= =?UTF-8?q?=20last=20run=20and=20next=20run=20(#977)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🐛 Fix importers sorting for last run and next run This commit will fix the sorting error that happens when the user is on the importers index and attempts to sort by the last run or next run. We add some migrations to add fields that the datatables can use so it can sort properly. Previously, this was not working because the last_imported_at and next_import_at fields were actually methods on the importer_run object and not the importer object. We are adding a few callbacks to the importer and importer_run models to ensure that the fields are properly set when they are called from either the web or the worker. Ref: - https://github.com/samvera/bulkrax/issues/956 * 🤖 Update upload-artifact and download-artifact CI was getting errors because the versions we were previously using were deprecated. This commit updates the actions to the latest versions. * 🐛 Remove Downloadable Files sorting The downloadable files sorting was broken plus, it's not clear now a downloadable file should be sorted. * ⚙️ Add guard for new migrations This commit will add a guard to the new migrations to ensure that they do not run if the columns already exist in the database. * 🤖 Add rake task to re save importers This rake task will allow users to re save all their importers. It accounts for tenants if it is a Hyku application. ```sh bundle exec rake bulkrax:resave_importers ``` --- .github/workflows/test.yml | 7 +++--- app/assets/javascripts/bulkrax/datatables.js | 2 +- app/models/bulkrax/importer.rb | 25 +++++++++++++++++++ app/models/bulkrax/importer_run.rb | 11 ++++++++ ...d_last_imported_at_to_bulkrax_importers.rb | 5 ++++ ...add_next_import_at_to_bulkrax_importers.rb | 5 ++++ lib/tasks/bulkrax_tasks.rake | 21 ++++++++++++++++ 7 files changed, 72 insertions(+), 4 deletions(-) create mode 100644 db/migrate/20240916182737_add_last_imported_at_to_bulkrax_importers.rb create mode 100644 db/migrate/20240916182823_add_next_import_at_to_bulkrax_importers.rb diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1eacd7b52..2e8124f54 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -43,10 +43,11 @@ jobs: run: bundle exec rake spec - name: Upload coverage results - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v4.4.0 with: name: coverage-report-${{ matrix.ruby }} - path: coverage + path: coverage/** + include-hidden-files: true coverage: runs-on: ubuntu-latest @@ -56,7 +57,7 @@ jobs: steps: - name: Download coverage report - uses: actions/download-artifact@v2 + uses: actions/download-artifact@v4.1.8 with: name: coverage-report-2.7 path: coverage diff --git a/app/assets/javascripts/bulkrax/datatables.js b/app/assets/javascripts/bulkrax/datatables.js index 9e80d348e..e5d2af1a3 100644 --- a/app/assets/javascripts/bulkrax/datatables.js +++ b/app/assets/javascripts/bulkrax/datatables.js @@ -67,7 +67,7 @@ Blacklight.onLoad(function() { { "data": "name" }, { "data": "status_message" }, { "data": "created_at" }, - { "data": "download" }, + { "data": "download", "orderable": false }, { "data": "actions", "orderable": false } ], initComplete: function () { diff --git a/app/models/bulkrax/importer.rb b/app/models/bulkrax/importer.rb index 4ef79f2e4..3e5d01967 100644 --- a/app/models/bulkrax/importer.rb +++ b/app/models/bulkrax/importer.rb @@ -18,6 +18,9 @@ class Importer < ApplicationRecord # rubocop:disable Metrics/ClassLength delegate :create_parent_child_relationships, :valid_import?, :write_errored_entries_file, :visibility, to: :parser + after_save :set_last_imported_at_from_importer_run + after_save :set_next_import_at_from_importer_run + attr_accessor :only_updates, :file_style, :file attr_writer :current_run @@ -247,5 +250,27 @@ def path_string rescue "#{self.id}_#{self.created_at.strftime('%Y%m%d%H%M%S')}" end + + private + + # Adding this here since we can update the importer without running the importer. + # When we simply save the importer (as in just updating the importer from the options), + # it does not trigger the after_save callback in the importer_run. + def set_last_imported_at_from_importer_run + return if @skip_set_last_imported_at # Prevent infinite loop + + @skip_set_last_imported_at = true + importer_runs.last&.set_last_imported_at + @skip_set_last_imported_at = false + end + + # @see #set_last_imported_at_from_importer_run + def set_next_import_at_from_importer_run + return if @skip_set_next_import_at # Prevent infinite loop + + @skip_set_next_import_at = true + importer_runs.last&.set_next_import_at + @skip_set_next_import_at = false + end end end diff --git a/app/models/bulkrax/importer_run.rb b/app/models/bulkrax/importer_run.rb index 132cdd1d7..e9e139b62 100644 --- a/app/models/bulkrax/importer_run.rb +++ b/app/models/bulkrax/importer_run.rb @@ -6,6 +6,9 @@ class ImporterRun < ApplicationRecord has_many :statuses, as: :runnable, dependent: :destroy has_many :pending_relationships, dependent: :destroy + after_save :set_last_imported_at + after_save :set_next_import_at + def parents pending_relationships.pluck(:parent_id).uniq end @@ -15,5 +18,13 @@ def user # fallback to the configured user. importer.user || Bulkrax.fallback_user_for_importer_exporter_processing end + + def set_last_imported_at + importer.update(last_imported_at: importer.last_imported_at) + end + + def set_next_import_at + importer.update(next_import_at: importer.next_import_at) + end end end diff --git a/db/migrate/20240916182737_add_last_imported_at_to_bulkrax_importers.rb b/db/migrate/20240916182737_add_last_imported_at_to_bulkrax_importers.rb new file mode 100644 index 000000000..24374800e --- /dev/null +++ b/db/migrate/20240916182737_add_last_imported_at_to_bulkrax_importers.rb @@ -0,0 +1,5 @@ +class AddLastImportedAtToBulkraxImporters < ActiveRecord::Migration[5.1] + def change + add_column :bulkrax_importers, :last_imported_at, :datetime unless column_exists?(:bulkrax_importers, :last_imported_at) + end +end diff --git a/db/migrate/20240916182823_add_next_import_at_to_bulkrax_importers.rb b/db/migrate/20240916182823_add_next_import_at_to_bulkrax_importers.rb new file mode 100644 index 000000000..8d62c2bcc --- /dev/null +++ b/db/migrate/20240916182823_add_next_import_at_to_bulkrax_importers.rb @@ -0,0 +1,5 @@ +class AddNextImportAtToBulkraxImporters < ActiveRecord::Migration[5.1] + def change + add_column :bulkrax_importers, :next_import_at, :datetime unless column_exists?(:bulkrax_importers, :next_import_at) + end +end diff --git a/lib/tasks/bulkrax_tasks.rake b/lib/tasks/bulkrax_tasks.rake index e32d4b89c..18b5b8e16 100644 --- a/lib/tasks/bulkrax_tasks.rake +++ b/lib/tasks/bulkrax_tasks.rake @@ -141,4 +141,25 @@ namespace :bulkrax do rescue => e puts "(#{e.message})" end + + desc "Resave importers" + task resave_importers: :environment do + if defined?(::Hyku) + Account.find_each do |account| + next if account.name == "search" + switch!(account) + puts "=============== updating #{account.name} ============" + + resave_importers + + puts "=============== finished updating #{account.name} ============" + end + else + resave_importers + end + end + + def resave_importers + Bulkrax::Importer.find_each(&:save!) + end end