Fixes #4316 - Ticket templates don't allow editing if obsolete attribtues are selected.

app/assets/javascripts/app/controllers/_ui_element/_application_action.coffee

@@ -36,6 +36,10 @@ UI Element options:
 
 - Renders a simpler attribute without operator support (default: false)
 
+**attribute.skip_unknown_attributes**
+
+- Skips rendering of unknown attributes (default: false)
+
 ###
 
 class App.UiElement.ApplicationAction
@@ -187,6 +191,8 @@ class App.UiElement.ApplicationAction
     else
 
       for groupAndAttribute, meta of params[attribute.name]
+        # Skip unknown attributes.
+        continue if attribute.skip_unknown_attributes and !_.includes(_.keys(elements), groupAndAttribute)
 
         # build and append
         element = @placeholder(item, attribute, params, groups, elements)
@@ -415,7 +421,7 @@ class App.UiElement.ApplicationAction
       __('relative'),
     ]
 
-    upcoming_operator = meta.operator
+    upcoming_operator = meta?.operator
 
     if !_.include(config?.operator, upcoming_operator)
       if Array.isArray(config?.operator)

app/assets/javascripts/app/controllers/agent_ticket_create.coffee

@@ -385,7 +385,15 @@ class App.TicketCreate extends App.Controller
               fieldArray = templateField.split('.')
               field = fieldArray[1] || fieldArray[0]
 
-              value = templateValue?['value'] || templateValue
+              if _.isObject(templateValue) and templateValue['value'] != undefined
+                value = templateValue['value']
+
+                # Move the completion value into its own parameter.
+                if templateValue['value_completion']
+                  params["#{field}_completion"] = templateValue['value_completion']
+
+              else
+                value = templateValue
 
               [field, value]
           )

app/assets/javascripts/app/models/template.coffee

@@ -4,7 +4,7 @@ class App.Template extends App.Model
   @url: @apiPath + '/templates'
   @configure_attributes = [
     { name: 'name',        display: __('Name'),     tag: 'input', type: 'text', limit: 100, null: false },
-    { name: 'options',     display: __('Actions'),  tag: 'ticket_perform_action', article_body_only: true, no_dates: true, no_richtext_uploads: true, sender_type: true, simple_attribute_selector: true, null: true },
+    { name: 'options',     display: __('Actions'),  tag: 'ticket_perform_action', article_body_only: true, no_dates: true, no_richtext_uploads: true, sender_type: true, simple_attribute_selector: true, skip_unknown_attributes: true, null: true },
     { name: 'updated_at',  display: __('Updated'),  tag: 'datetime', readonly: 1 },
     { name: 'active',      display: __('Active'),   tag: 'active', default: true },
   ]

app/controllers/templates_controller.rb

@@ -10,14 +10,22 @@ JSON
 
 Example:
 {
-  "id":1,
-  "name":"some template",
+  "id": 1,
+  "name": "some template",
   "user_id": null,
-  "options":{"a":1,"b":2},
-  "updated_at":"2012-09-14T17:51:53Z",
-  "created_at":"2012-09-14T17:51:53Z",
-  "updated_by_id":2.
-  "created_by_id":2,
+  "options": {
+    "ticket.title": {
+      "value": "some title"
+    },
+    "ticket.customer_id": {
+      "value": "2",
+      "value_completion": "Nicole Braun <nicole.braun@zammad.org>"
+    }
+  },
+  "updated_at": "2012-09-14T17:51:53Z",
+  "created_at": "2012-09-14T17:51:53Z",
+  "updated_by_id": 2,
+  "created_by_id": 2
 }
 
 =end
@@ -79,7 +87,15 @@ POST /api/v1/templates.json
 Payload:
 {
   "name": "some name",
-  "options":{"a":1,"b":2},
+  "options": {
+    "ticket.title": {
+      "value": "some title"
+    },
+    "ticket.customer_id": {
+      "value": "2",
+      "value_completion": "Nicole Braun <nicole.braun@zammad.org>"
+    }
+  }
 }
 
 Response:
@@ -90,12 +106,12 @@ Response:
 }
 
 Test:
-curl http://localhost/api/v1/templates.json -v -u #{login}:#{password} -H "Content-Type: application/json" -X POST -d '{"name": "some_name","active": true, "note": "some note"}'
+curl http://localhost/api/v1/templates.json -v -u #{login}:#{password} -H "Content-Type: application/json" -X POST -d '{"name": "some_name", "options": {"ticket.title": {"value": "some title"},"ticket.customer_id": {"value": "2", "value_completion": "Nicole Braun <nicole.braun@zammad.org>"}}}'
 
 =end
 
   def create
-    model_create_render(Template, params)
+    template_create_render(params)
   end
 
 =begin
@@ -106,7 +122,15 @@ PUT /api/v1/templates/{id}.json
 Payload:
 {
   "name": "some name",
-  "options":{"a":1,"b":2},
+  "options": {
+    "ticket.title": {
+      "value": "some title"
+    },
+    "ticket.customer_id": {
+      "value": "2",
+      "value_completion": "Nicole Braun <nicole.braun@zammad.org>"
+    }
+  }
 }
 
 Response:
@@ -117,12 +141,12 @@ Response:
 }
 
 Test:
-curl http://localhost/api/v1/templates.json -v -u #{login}:#{password} -H "Content-Type: application/json" -X PUT -d '{"name": "some_name","active": true, "note": "some note"}'
+curl http://localhost/api/v1/templates/1.json -v -u #{login}:#{password} -H "Content-Type: application/json" -X PUT -d '{"name": "some_name", "options": {"ticket.title": {"value": "some title"},"ticket.customer_id": {"value": "2", "value_completion": "Nicole Braun <nicole.braun@zammad.org>"}}}'
 
 =end
 
   def update
-    model_update_render(Template, params)
+    template_update_render(params)
   end
 
 =begin
@@ -141,4 +165,111 @@ curl http://localhost/api/v1/templates.json -v -u #{login}:#{password} -H "Conte
   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_create_render(params) # rubocop:disable Metrics/AbcSize
+    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
+
+    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) # rubocop:disable Metrics/AbcSize
+    template = Template.find(params[:id])
+
+    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] = 'update'
+    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
+
+    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)
+  end
 end

db/migrate/20221031105713_issue_4316_template_options_migration.rb

@@ -0,0 +1,36 @@
+# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/
+
+class Issue4316TemplateOptionsMigration < ActiveRecord::Migration[6.1]
+  def change
+    # return if it's a new setup
+    return if !Setting.exists?(name: 'system_init_done')
+
+    Template.all.each do |template|
+      template.options = migrate_template_options(template.options)
+      template.save!
+    end
+  end
+
+  private
+
+  def migrate_template_options(options)
+    new_options = {}
+
+    options.each do |key, value|
+      next if key.ends_with?('_completion')
+
+      if value.is_a?(Hash)
+        new_options[key] = value
+        next
+      end
+
+      new_options[key] = { value: value }
+
+      if options.key?("#{key}_completion")
+        new_options[key]['value_completion'] = options["#{key}_completion"]
+      end
+    end
+
+    new_options
+  end
+end

public/assets/tests/qunit/form_ticket_perform_action.js

@@ -721,6 +721,9 @@ QUnit.test( "ticket_perform_action check template attributes", assert => {
       'article.body': {
         value: 'foobar',
       },
+      'ticket.foo': {
+        value: 'bar',
+      },
     },
   }
 
@@ -737,6 +740,7 @@ QUnit.test( "ticket_perform_action check template attributes", assert => {
           no_dates:                  true,
           no_richtext_uploads:       true,
           simple_attribute_selector: true,
+          skip_unknown_attributes:   true,
           null:                      true,
         },
       ],

spec/db/migrate/issue_4316_template_options_migration_spec.rb

@@ -0,0 +1,67 @@
+# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe Issue4316TemplateOptionsMigration, type: :db_migration do
+  let(:expected) do
+    {
+      'article.body'          => { value: 'twet 23123' },
+      'ticket.formSenderType' => { value: 'phone-in' },
+      'ticket.title'          => { value: 'aaa' },
+      'ticket.customer_id'    => { value: '2', value_completion: 'Nicole Braun <nicole.braun@example.com>' },
+      'ticket.cc'             => { value: 'somebody2@example.com' },
+      'ticket.group_id'       => { value: '1' },
+      'ticket.owner_id'       => { value: '11' },
+      'ticket.state_id'       => { value: '2' },
+      'ticket.priority_id'    => { value: '2' },
+      'ticket.a1'             => { value: 'a' },
+      'ticket.a2'             => { value: %w[a b] },
+      'ticket.b1'             => { value: 'a::c' },
+      'ticket.b2'             => { value: ['b'], value_completion: '' },
+      'ticket.category'       => { value: 'a::aa' },
+      'ticket.tags'           => { value: 'aa, bb' },
+    }
+  end
+
+  let(:template) do
+
+    # Wrong format after initial migration from 5.2 to 5.3.
+    Template.create!(
+      name:          'new',
+      options:
+                     {
+                       'article.body'                  => 'twet 23123',
+                       'ticket.formSenderType'         => 'phone-in',
+                       'ticket.title'                  => 'aaa',
+                       'ticket.customer_id'            => '2',
+                       'ticket.customer_id_completion' => 'Nicole Braun <nicole.braun@example.com>',
+                       'ticket.cc'                     => 'somebody2@example.com',
+                       'ticket.group_id'               => '1',
+                       'ticket.owner_id'               => '11',
+                       'ticket.state_id'               => '2',
+                       'ticket.priority_id'            => '2',
+                       'ticket.a1'                     => 'a',
+                       'ticket.a2'                     => %w[a b],
+                       'ticket.b1'                     => 'a::c',
+                       'ticket.b2'                     => ['b'],
+                       'ticket.b2_completion'          => '',
+                       'ticket.category'               => 'a::aa',
+                       'ticket.tags'                   => 'aa, bb'
+                     },
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+  end
+
+  before do
+    template
+  end
+
+  context 'when migrating' do
+    it 'update options to expected hash value (#4316)' do
+      migrate
+      expect(template.reload.options).to eq(expected.deep_stringify_keys)
+    end
+  end
+
+end

spec/factories/template.rb

@@ -3,7 +3,7 @@
 FactoryBot.define do
   factory :template do
     name          { Faker::Name.unique.name }
-    options       { { 'ticket.title': 'Some dummy title' } }
+    options       { { 'ticket.title': { value: 'Some dummy title' } } }
     updated_by_id { 1 }
     created_by_id { 1 }
 
@@ -19,13 +19,12 @@ FactoryBot.define do
     trait :dummy_data do
       options do
         {
-          'ticket.formSenderType'         => sender_type,
-          'ticket.title'                  => title,
-          'article.body'                  => body,
-          'ticket.customer_id'            => customer.id,
-          'ticket.customer_id_completion' => "#{customer.firstname} #{customer.lastname} <#{customer.email}>",
-          'ticket.group_id'               => group.id,
-          'ticket.owner_id'               => owner.id,
+          'ticket.formSenderType' => { value: sender_type },
+          'ticket.title'          => { value: title },
+          'article.body'          => { value: body },
+          'ticket.customer_id'    => { value: customer.id, value_completion: "#{customer.firstname} #{customer.lastname} <#{customer.email}>" },
+          'ticket.group_id'       => { value: group.id },
+          'ticket.owner_id'       => { value: owner.id },
         }
       end
     end

spec/requests/template_spec.rb

@@ -0,0 +1,104 @@
+# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe Template, type: :request do
+  let(:agent)    { create(:agent) }
+  let(:customer) { create(:customer) }
+
+  describe 'request handling', authenticated_as: :agent do
+    before do
+      allow(ActiveSupport::Deprecation).to receive(:warn)
+    end
+
+    context 'when listing templates' do
+      let!(:templates) { create_list(:template, 10) }
+
+      it 'returns all' do
+        get '/api/v1/templates.json'
+
+        expect(json_response.length).to eq(10)
+      end
+
+      it 'returns options in a new format only' do
+        get '/api/v1/templates.json'
+
+        templates.each_with_index do |template, index|
+          expect(json_response[index]['options']).to eq(template.options)
+        end
+      end
+    end
+
+    context 'when showing templates' do
+      let!(:template) { create(:template) }
+
+      it 'returns ok' do
+        get "/api/v1/templates/#{template.id}.json"
+
+        expect(response).to have_http_status(:ok)
+      end
+
+      it 'returns options in a new format only' do
+        get "/api/v1/templates/#{template.id}.json"
+
+        expect(json_response['options']).to eq(template.options)
+      end
+    end
+
+    context 'when creating template' do
+      it 'returns created' do
+        post '/api/v1/templates.json', params: { name: 'Foo', options: { 'ticket.title': { value: 'Bar' }, 'ticket.customer_id': { value: customer.id.to_s, value_completion: "#{customer.firstname} #{customer.lastname} <#{customer.email}>" } } }
+
+        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
+    end
+
+    context 'when updating template' do
+      let!(:template) { create(:template) }
+
+      it 'returns ok' do
+        put "/api/v1/templates/#{template.id}.json", params: { options: { 'ticket.title': { value: 'Foo' } } }
+
+        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
+    end
+
+    context 'when destroying template' do
+      let!(:template) { create(:template) }
+
+      it 'returns ok' do
+        delete "/api/v1/templates/#{template.id}.json"
+
+        expect(response).to have_http_status(:ok)
+      end
+    end
+  end
+end

spec/system/manage/template_spec.rb

@@ -25,4 +25,36 @@ RSpec.describe 'Manage > Templates', type: :system do
     end
   end
 
+  context 'when editing an existing template' do
+    let!(:template) { create(:template, :dummy_data) }
+
+    before do
+      visit '#manage/templates'
+
+      within(:active_content) do
+        find(".js-tableBody tr.item[data-id='#{template.id}']").click
+      end
+    end
+
+    it 'restores stored attributes' do
+      in_modal do
+        expect(find('input[name="name"]').value).to eq(template.name)
+        expect(find('select[name="options::ticket.formSenderType::value"] option[selected]').value).to eq(template.options['ticket.formSenderType']['value'])
+        expect(find('input[name="options::ticket.title::value"]').value).to eq(template.options['ticket.title']['value'])
+        expect(find('[data-name="options::article.body::value"]').text).to eq(template.options['article.body']['value'])
+        expect(find('select[name="options::ticket.group_id::value"] option[selected]').value).to eq(template.options['ticket.group_id']['value'].to_s)
+      end
+    end
+
+    context 'with custom attributes' do
+      let(:template) { create(:template, options: { 'ticket.title': { value: 'Test' }, 'ticket.foo': { value: 'bar', 'ticket.baz': { value: nil }, 'ticket.qux': nil } }) }
+
+      it 'ignores unknown or invalid attributes (#4316)' do
+        in_modal do
+          expect(find('input[name="options::ticket.title::value"]').value).to eq('Test')
+          expect(find_all('select.form-control').length).to eq(2)
+        end
+      end
+    end
+  end
 end