Fixed migration trigger and spec regressions - fixes #986

Author: philayres

lib/active_record/migration/app_generator.rb

@@ -669,9 +669,9 @@ def update_fields
           end
         end
 
-        # Recreate the trigger if any field types changed, since the trigger function
-        # needs to reflect the correct types for the fields being copied to history
-        if changed.present? || changed_history.present?
+        # Recreate the trigger whenever tracked fields change so the history
+        # function matches the current table shape.
+        if field_changes || added_history.present? || removed_history.present? || changed.present? || changed_history.present?
           case resource_type
           when :dynamic_model
             create_dynamic_model_trigger

spec/models/dynamic_model_spec.rb

@@ -122,7 +122,7 @@
 
       expect(dm).to be_a DynamicModel
       # The field list has been set up
-      expect(dm.field_list).to eq 'test1 test2 created_by_user_id'
+      expect(dm.field_list.split).to include('test1', 'test2', 'created_by_user_id')
       # The keys have been set up automatically
       expect(dm.foreign_key_name).to eq 'master_id'
       expect(dm.primary_key_name).to eq 'id'
@@ -168,7 +168,7 @@
 
       expect(dm).to be_a DynamicModel
       # The field list has been set up
-      expect(dm.field_list).to eq 'test1 test2 created_by_user_id'
+      expect(dm.field_list.split).to include('test1', 'test2', 'created_by_user_id')
       # The keys have been set up automatically
       expect(dm.foreign_key_name).to eq 'master_id'
       expect(dm.primary_key_name).to eq 'id'

spec/models/option_configs/dynamic_model_options_spec.rb

@@ -849,4 +849,106 @@
       expect(history_records.first['input_b'].to_i).to eq(456)
     end
   end
+
+  # Test for renamed dynamic-model fields keeping history tables and triggers in sync.
+  # This protects create/update paths after field names change in app configuration.
+  context 'field renames' do
+    before :all do
+      create_admin
+      create_user
+
+      @original_allow_migrations = Settings::AllowDynamicMigrations
+      change_setting('AllowDynamicMigrations', true)
+
+      @schema_name = 'dynamic_test'
+      @table_name = 'test_field_renames'
+      @history_table_name = 'test_field_rename_history'
+
+      DynamicModel.active.where(table_name: @table_name).each { |dm| dm.disable!(@admin) }
+      DynamicModel.send(:remove_const, :TestFieldRename) if defined?(DynamicModel::TestFieldRename)
+
+      @conn = ActiveRecord::Base.connection
+      @conn.execute("DROP TABLE IF EXISTS #{@schema_name}.#{@history_table_name} CASCADE")
+      @conn.execute("DROP TABLE IF EXISTS #{@schema_name}.#{@table_name} CASCADE")
+
+      @dm = DynamicModel.create!(
+        current_admin: @admin,
+        name: 'Test Field Renames',
+        table_name: @table_name,
+        schema_name: @schema_name,
+        primary_key_name: :id,
+        foreign_key_name: :master_id,
+        category: :test,
+        field_list: 'select_still_interested select_continue_now notes',
+        options: <<~YAML
+          _db_columns:
+            select_still_interested:
+              type: string
+            select_continue_now:
+              type: string
+            notes:
+              type: string
+        YAML
+      )
+
+      @dm.update_tracker_events
+    end
+
+    after :all do
+      @dm&.disable!(@admin)
+      change_setting('AllowDynamicMigrations', @original_allow_migrations)
+    end
+
+    it 'rebuilds the history trigger when fields are renamed' do
+      master = Master.create! current_user: @user
+      master.current_user = @user
+
+      setup_access :dynamic_model__test_field_renames
+
+      @dm.instance_variable_set(:@ran_migration, nil)
+      @dm.update!(
+        current_admin: @admin,
+        field_list: 'still_interested continue_now notes',
+        options: <<~YAML
+          _db_columns:
+            still_interested:
+              type: string
+            continue_now:
+              type: string
+            notes:
+              type: string
+        YAML
+      )
+
+      @dm.instance_variable_set(:@migration_generator, nil)
+      @dm.send(:generate_migration)
+      @dm.send(:run_migration) if @dm.instance_variable_get(:@do_migration)
+      @dm.update_tracker_events
+
+      @conn.schema_cache.clear!
+      DynamicModel.reset_active_model_configurations!
+
+      history_columns = @conn.columns("#{@schema_name}.#{@history_table_name}").map(&:name)
+      expect(history_columns).to include('still_interested', 'continue_now')
+      expect(history_columns).not_to include('select_still_interested', 'select_continue_now')
+
+      model_class = @dm.implementation_class
+      model_class.reset_column_information
+
+      record = master.dynamic_model__test_field_renames.create!(
+        current_user: @user,
+        still_interested: 'yes',
+        continue_now: 'no',
+        notes: 'renamed fields'
+      )
+
+      history_record = @conn.exec_query(
+        "SELECT still_interested, continue_now FROM #{@schema_name}.#{@history_table_name} " \
+        "WHERE #{@table_name.singularize}_id = #{record.id} ORDER BY id DESC LIMIT 1"
+      ).first
+
+      expect(history_record['still_interested']).to eq('yes')
+      expect(history_record['continue_now']).to eq('no')
+    end
+  end
 end

spec/models/study_recruitment_setup_spec.rb

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+# These tests lock down the Study Recruitment initial-call dynamic-model
+# configuration so reused setup state gets rebuilt when it still references
+# the legacy select_* field names.
+
+require 'rails_helper'
+
+describe StudyRecruitmentSetup do
+  describe '.stale_initial_call_dynamic_model_config?' do
+    let(:dynamic_model) do
+      instance_double(DynamicModel, field_list: field_list, options: options)
+    end
+
+    context 'when the initial-call config still uses legacy select-prefixed fields' do
+      let(:field_list) { 'select_still_interested select_continue_now callback_date callback_time notes' }
+      let(:options) { 'select_still_interested:\nselect_continue_now:' }
+
+      it 'treats the config as stale' do
+        expect(described_class.stale_initial_call_dynamic_model_config?(dynamic_model)).to be true
+      end
+    end
+
+    context 'when the initial-call config uses the current field names' do
+      let(:field_list) { 'still_interested continue_now callback_date callback_time notes' }
+      let(:options) { 'still_interested:\ncontinue_now:' }
+
+      it 'treats the config as current' do
+        expect(described_class.stale_initial_call_dynamic_model_config?(dynamic_model)).to be false
+      end
+    end
+  end
+end

spec/setup_helper.rb

@@ -261,10 +261,10 @@ def self.reload_configs
     Rails.logger.info 'Reload configs'
     Admin::AppType.reset_memo_associated_items!
     AppControl.define_models
+    ExternalIdentifier.enable_active_configurations
     DynamicModel.enable_active_configurations disable_on_failure: true
     ItemFlag.enable_active_configurations
     ActivityLog.enable_active_configurations
-    ExternalIdentifier.enable_active_configurations
     DynamicModel.routes_reload
   end
 

spec/support/feature_support.rb

@@ -322,7 +322,10 @@ def expand_master_record_tab(name)
       raise "Could not find panel tab #{name.id_underscore.inspect}. Available tabs: #{available_tabs.join(', ')}"
     end
 
-    tab_link = find(tab_selector, visible: :all, wait: 0)
+    tab_link = all('.master-main-panel.in a[data-panel-tab]', visible: :all, wait: 0)
+               .find { |tab| tab['data-panel-tab'] == name.id_underscore }
+    tab_link ||= all(tab_selector, visible: :all, wait: 0).find(&:visible?)
+    tab_link ||= find(tab_selector, visible: :all, wait: 0)
     expect(tab_link).not_to be nil
     scroll_into_view(tab_link)
     tab_link.click if tab_link['aria-expanded'] != 'true'

spec/system/nfs_store/secure_viewer_enhancements_spec.rb

@@ -282,7 +282,7 @@ def open_secure_viewer(filename)
     expect(@container).to be_a(NfsStore::Manage::Container)
 
     # Upload a test file to the container for secure viewer tests
-    temp_dir = Rails.root.join('tmp', 'test_files')
+    temp_dir = Rails.root.join('tmp', 'test_files_secure_viewer')
     FileUtils.mkdir_p(temp_dir)
     @test_image_path = temp_dir.join('test-image.png')
     # Create a valid 100x100 PNG that the secure viewer can convert
@@ -307,7 +307,7 @@ def open_secure_viewer(filename)
 
   after :all do
     disable_active_panel_layout('test-columns-panel')
-    temp_dir = Rails.root.join('tmp', 'test_files')
+    temp_dir = Rails.root.join('tmp', 'test_files_secure_viewer')
     FileUtils.rm_rf(temp_dir)
   end