Maintenance: Create a real migration for old template options.

- Move live migration code to a regular migration to reduce controller complexity.
- No functional changes.

app/controllers/templates_controller.rb

@@ -111,7 +111,7 @@ curl http://localhost/api/v1/templates.json -v -u #{login}:#{password} -H "Conte
 =end
 
   def create
-    template_create_render(params)
+    model_create_render(policy_scope(Template), params)
   end
 
 =begin
@@ -146,7 +146,7 @@ curl http://localhost/api/v1/templates/1.json -v -u #{login}:#{password} -H "Con
 =end
 
   def update
-    template_update_render(params)
+    model_update_render(policy_scope(Template), params)
   end
 
 =begin
@@ -163,109 +163,6 @@ curl http://localhost/api/v1/templates.json -v -u #{login}:#{password} -H "Conte
 =end
 
   def destroy
-    model_destroy_render(Template, params)
-  end
-
-  private
-
-  def old_options?(options)
-    has_new_options = false
-
-    options.each_key do |key|
-      if key.starts_with?(%r{(ticket|article)\.})
-        has_new_options = true
-      end
-      break if has_new_options
-    end
-
-    !has_new_options
-  end
-
-  def migrate_options(options)
-    old_options = options.clone
-    new_options = {}
-
-    article_attribute_list = %w[body form_id]
-
-    # Implements a compatibility layer for templates, by converting `options` to a newer format:
-    #   options: {
-    #     'ticket.field_1': { value: 'value_1' },
-    #     'ticket.field_2': { value: 'value_2', value_completion: 'value_completion_2' },
-    #   }
-    old_options.each do |key, value|
-      new_key = "ticket.#{key}"
-
-      if article_attribute_list.include?(key)
-        new_key = "article.#{key}"
-      end
-
-      new_options[new_key] = { value: value }
-
-      if old_options.key?("#{key}_completion")
-        new_options[new_key]['value_completion'] = old_options["#{key}_completion"]
-        old_options.delete("#{key}_completion")
-      end
-    end
-
-    new_options
-  end
-
-  def template_prepare_params(params)
-    clean_params = Template.association_name_to_id_convert(params)
-    clean_params = Template.param_cleanup(clean_params, true)
-
-    if Template.included_modules.include?(ChecksCoreWorkflow)
-      clean_params[:screen] = 'create'
-    end
-
-    if old_options?(clean_params[:options])
-      clean_params[:options] = migrate_options(clean_params[:options])
-      ActiveSupport::Deprecation.warn 'Usage of the old format for template options with unprefixed keys and simple values is deprecated.'
-    end
-
-    clean_params
-  end
-
-  def template_create_render(params)
-    clean_params = template_prepare_params(params)
-
-    template = Template.new(clean_params)
-    template.associations_from_param(params)
-    template.save!
-
-    if response_expand?
-      render json: template.attributes_with_association_names, status: :created
-      return
-    end
-
-    if response_full?
-      render json: template.class.full(template.id), status: :created
-      return
-    end
-
-    model_create_render_item(template)
-  end
-
-  def template_update_render(params)
-    template = Template.find(params[:id])
-
-    clean_params = template_prepare_params(params)
-
-    template.with_lock do
-      template.associations_from_param(params)
-      template.update!(clean_params)
-    end
-
-    if response_expand?
-      render json: template.attributes_with_association_names, status: :ok
-      return
-    end
-
-    if response_full?
-      render json: template.class.full(template.id), status: :ok
-      return
-    end
-
-    model_update_render_item(template)
+    model_destroy_render(policy_scope(Template), params)
   end
 end

db/migrate/20240618081710_migrate_template_options.rb

@@ -0,0 +1,52 @@
+# Copyright (C) 2012-2024 Zammad Foundation, https://zammad-foundation.org/
+
+class MigrateTemplateOptions < ActiveRecord::Migration[7.0]
+  def change
+    # return if it's a new setup
+    return if !Setting.exists?(name: 'system_init_done')
+
+    Template.all.each do |template|
+      if old_options?(template.options)
+        template.options = migrate_options(template.options)
+        template.save!
+      end
+    end
+  end
+
+  def old_options?(options)
+    options.each_key do |key|
+      return false if key.starts_with?(%r{(ticket|article)\.})
+    end
+
+    true
+  end
+
+  # Implements a compatibility layer for templates, by converting `options` to a newer format:
+  #   options: {
+  #     'ticket.field_1': { value: 'value_1' },
+  #     'ticket.field_2': { value: 'value_2', value_completion: 'value_completion_2' },
+  #   }
+  def migrate_options(options)
+    old_options = options.clone
+    new_options = {}
+
+    article_attribute_list = %w[body form_id]
+
+    old_options.each do |key, value|
+      new_key = "ticket.#{key}"
+
+      if article_attribute_list.include?(key)
+        new_key = "article.#{key}"
+      end
+
+      new_options[new_key] = { value: value }
+
+      if old_options.key?("#{key}_completion")
+        new_options[new_key]['value_completion'] = old_options["#{key}_completion"]
+        old_options.delete("#{key}_completion")
+      end
+    end
+
+    new_options
+  end
+end

spec/db/migrate/migrate_template_options_spec.rb

@@ -0,0 +1,40 @@
+# Copyright (C) 2012-2024 Zammad Foundation, https://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe MigrateTemplateOptions, type: :db_migration do
+  let!(:template) { create(:template) }
+
+  context 'with new options' do
+    it 'keeps new options unchanged' do
+      expect { migrate }.not_to change(template, :options)
+    end
+  end
+
+  context 'with old options' do
+    let!(:template)   { create(:template, options: old_options) }
+    let(:customer)    { create(:customer) }
+    let(:old_options) do
+      {
+        title:                  'Bar',
+        customer_id:            customer.id.to_s,
+        customer_id_completion: "#{customer.firstname} #{customer.lastname} <#{customer.email}>",
+        body:                   'abc'
+      }
+    end
+    let(:new_options) do
+      {
+        'ticket.title'       => { 'value' => 'Bar' },
+        'ticket.customer_id' => {
+          'value'            => customer.id.to_s,
+          'value_completion' => "#{customer.firstname} #{customer.lastname} <#{customer.email}>",
+        },
+        'article.body'       => { 'value' => 'abc' }
+      }
+    end
+
+    it 'migrates them' do
+      expect { migrate }.to change { template.reload.options }.from(old_options).to(new_options)
+    end
+  end
+end

spec/requests/template_spec.rb

@@ -97,20 +97,6 @@ RSpec.describe Template, type: :request do
         expect(response).to have_http_status(:created)
       end
 
-      it 'supports template options in an older format' do
-        params = { name: 'Foo', options: { title: 'Bar', customer_id: customer.id.to_s, customer_id_completion: "#{customer.firstname} #{customer.lastname} <#{customer.email}>" } }
-
-        post '/api/v1/templates.json', params: params
-
-        expect(json_response['options']).to eq({ 'ticket.title': { value: 'Bar' }, 'ticket.customer_id': { value: customer.id.to_s, value_completion: "#{customer.firstname} #{customer.lastname} <#{customer.email}>" } }.deep_stringify_keys)
-      end
-
-      it 'throws deprecation warning' do
-        post '/api/v1/templates.json', params: { name: 'Foo', options: { title: 'Bar', customer_id: customer.id.to_s, customer_id_completion: "#{customer.firstname} #{customer.lastname} <#{customer.email}>" } }
-
-        expect(ActiveSupport::Deprecation).to have_received(:warn)
-      end
-
       context 'with agent permissions', authenticated_as: :agent do
         it 'request is forbidden' do
           post '/api/v1/templates.json', params: { name: 'Foo', options: { 'ticket.title': { value: 'Bar' } } }
@@ -129,20 +115,6 @@ RSpec.describe Template, type: :request do
         expect(response).to have_http_status(:ok)
       end
 
-      it 'supports template options in an older format' do
-        params = { name: 'Foo', options: { title: 'Bar', customer_id: customer.id.to_s, customer_id_completion: "#{customer.firstname} #{customer.lastname} <#{customer.email}>" } }
-
-        put "/api/v1/templates/#{template.id}.json", params: params
-
-        expect(json_response['options']).to eq({ 'ticket.title': { value: 'Bar' }, 'ticket.customer_id': { value: customer.id.to_s, value_completion: "#{customer.firstname} #{customer.lastname} <#{customer.email}>" } }.deep_stringify_keys)
-      end
-
-      it 'throws deprecation warning' do
-        put "/api/v1/templates/#{template.id}.json", params: { name: 'Foo', options: { title: 'Bar', customer_id: customer.id.to_s, customer_id_completion: "#{customer.firstname} #{customer.lastname} <#{customer.email}>" } }
-
-        expect(ActiveSupport::Deprecation).to have_received(:warn)
-      end
-
       context 'with agent permissions', authenticated_as: :agent do
         it 'request is forbidden' do
           put "/api/v1/templates/#{template.id}.json", params: { options: { 'ticket.title': { value: 'Foo' } } }