Fixes #2634 - Merge not possible with not set, required attributes

app/assets/javascripts/app/views/object_manager/screens.jst.eco

@@ -23,7 +23,11 @@
               <%- @T(key) %>: <input class="js-boolean" name="screens::<%= screen %>::<%= role %>::<%= key %>" type="checkbox" <% if @params && @params.screens && @params.screens[screen] && @params.screens[screen][role] && @params.screens[screen][role][key] is true: %>checked<% end %> value="true">
             <% end %>
         <% end %>
+        <tr>
+          <td class="settings-list-control-cell">
+          <td class="settings-list-control-cell small" colspan=2>
+            <%- @T('Not applicable to: merging, emails, form, Facebook, Telegram, Twitter, SMS') %>
       <% end %>
     </tbody>
   </table>
-</div>
+</div>

app/controllers/channels_sms_controller.rb

@@ -55,22 +55,24 @@ class ChannelsSmsController < ApplicationController
   def webhook
     raise Exceptions::UnprocessableEntity, 'token param missing' if params['token'].blank?
 
-    channel = nil
-    Channel.where(active: true, area: 'Sms::Account').each do |local_channel|
-      next if local_channel.options[:webhook_token] != params['token']
+    ApplicationHandleInfo.in_context('sms') do
+      channel = nil
+      Channel.where(active: true, area: 'Sms::Account').each do |local_channel|
+        next if local_channel.options[:webhook_token] != params['token']
 
-      channel = local_channel
-    end
-    if !channel
-      render(
-        json:   { message: 'channel not found' },
-        status: :not_found
-      )
-      return
-    end
+        channel = local_channel
+      end
+      if !channel
+        render(
+          json:   { message: 'channel not found' },
+          status: :not_found
+        )
+        return
+      end
 
-    conten_type, content = channel.process(params.permit!.to_h)
-    send_data content, type: conten_type
+      content_type, content = channel.process(params.permit!.to_h)
+      send_data content, type: content_type
+    end
   end
 
   private

app/controllers/form_controller.rb

@@ -89,46 +89,49 @@ class FormController < ApplicationController
       )
     end
 
+    ticket = nil
+
     # set current user
     UserInfo.current_user_id = customer.id
-
-    group = Group.find_by(id: Setting.get('form_ticket_create_group_id'))
-    if !group
-      group = Group.where(active: true).first
+    ApplicationHandleInfo.in_context('form') do # rubocop:disable Metrics/BlockLength
+      group = Group.find_by(id: Setting.get('form_ticket_create_group_id'))
       if !group
-        group = Group.first
+        group = Group.where(active: true).first
+        if !group
+          group = Group.first
+        end
       end
-    end
-    ticket = Ticket.create!(
-      group_id:    group.id,
-      customer_id: customer.id,
-      title:       params[:title],
-      preferences: {
-        form: {
-          remote_ip:       request.remote_ip,
-          fingerprint_md5: Digest::MD5.hexdigest(params[:fingerprint]),
-        }
-      }
-    )
-    article = Ticket::Article.create!(
-      ticket_id: ticket.id,
-      type_id:   Ticket::Article::Type.find_by(name: 'web').id,
-      sender_id: Ticket::Article::Sender.find_by(name: 'Customer').id,
-      body:      params[:body],
-      subject:   params[:title],
-      internal:  false,
-    )
-
-    params[:file]&.each do |file|
-      Store.add(
-        object:      'Ticket::Article',
-        o_id:        article.id,
-        data:        file.read,
-        filename:    file.original_filename,
+      ticket = Ticket.create!(
+        group_id:    group.id,
+        customer_id: customer.id,
+        title:       params[:title],
         preferences: {
-          'Mime-Type' => file.content_type,
+          form: {
+            remote_ip:       request.remote_ip,
+            fingerprint_md5: Digest::MD5.hexdigest(params[:fingerprint]),
+          }
         }
       )
+      article = Ticket::Article.create!(
+        ticket_id: ticket.id,
+        type_id:   Ticket::Article::Type.find_by(name: 'web').id,
+        sender_id: Ticket::Article::Sender.find_by(name: 'Customer').id,
+        body:      params[:body],
+        subject:   params[:title],
+        internal:  false,
+      )
+
+      params[:file]&.each do |file|
+        Store.add(
+          object:      'Ticket::Article',
+          o_id:        article.id,
+          data:        file.read,
+          filename:    file.original_filename,
+          preferences: {
+            'Mime-Type' => file.content_type,
+          }
+        )
+      end
     end
 
     UserInfo.current_user_id = 1

app/models/object_manager/attribute/validation.rb

@@ -4,7 +4,7 @@ class ObjectManager::Attribute::Validation < ActiveModel::Validator
   include ::Mixin::HasBackends
 
   def validate(record)
-    return if validation_unneeded?
+    return if !validation_needed?
 
     @record = record
     sanitize_memory_cache
@@ -20,10 +20,12 @@ class ObjectManager::Attribute::Validation < ActiveModel::Validator
 
   attr_reader :record
 
-  def validation_unneeded?
-    return true if Setting.get('import_mode')
+  def validation_needed?
+    return false if Setting.get('import_mode')
 
-    ApplicationHandleInfo.current != 'application_server'
+    return false if ApplicationHandleInfo.context_without_custom_attributes?
+
+    ApplicationHandleInfo.current == 'application_server'
   end
 
   def attributes_unchanged?

app/models/ticket.rb

@@ -290,7 +290,7 @@ returns
     raise Exceptions::UnprocessableEntity, 'Can\'t merge ticket with it self!' if id == target_ticket.id
 
     # update articles
-    Transaction.execute do
+    Transaction.execute context: 'merge' do
 
       Ticket::Article.where(ticket_id: id).each(&:touch)
 

app/models/transaction.rb

@@ -2,7 +2,7 @@
 
 class Transaction
   attr_reader :options
-  attr_accessor :original_user_id, :original_interface_handle
+  attr_accessor :original_user_id, :original_interface_handle, :original_interface_context
 
   def initialize(options = {})
     @options = options
@@ -32,6 +32,7 @@ class Transaction
     reset_user_id_start
     bulk_import_start
     interface_handle_start
+    interface_context_start
   end
 
   def start_transaction
@@ -45,6 +46,7 @@ class Transaction
 
   def finish_transaction
     interface_handle_finish
+    interface_context_finish
 
     TransactionDispatcher.commit(options)
     PushMessages.finish
@@ -101,4 +103,22 @@ class Transaction
 
     ApplicationHandleInfo.current = original_interface_handle
   end
+
+  def interface_context?
+    options[:context].present?
+  end
+
+  def interface_context_start
+    return if !interface_context?
+
+    self.original_interface_context = ApplicationHandleInfo.context
+
+    ApplicationHandleInfo.context = options[:context]
+  end
+
+  def interface_context_finish
+    return if !interface_context?
+
+    ApplicationHandleInfo.context = original_interface_context
+  end
 end

lib/application_handle_info.rb

@@ -1,13 +1,9 @@
 # Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
 
 module ApplicationHandleInfo
-  def self.current
-    Thread.current[:application_handle] || 'unknown'
-  end
-
-  def self.current=(name)
-    Thread.current[:application_handle] = name
-  end
+  # stores current application handler.
+  # for example application_server, scheduler, websocket, postmaster...
+  thread_mattr_accessor :current
 
   def self.postmaster?
     return false if current.blank?
@@ -24,4 +20,23 @@ module ApplicationHandleInfo
   ensure
     self.current = orig
   end
+
+  # stores action context
+  # for example merge, twitter, telegram....
+  # used to determine if custom attribute validation shall run
+  thread_mattr_accessor :context
+
+  def self.in_context(name)
+    raise ArgumentError, 'requires a block' if !block_given?
+
+    orig = context
+    self.context = name
+    yield
+  ensure
+    self.context = orig
+  end
+
+  def self.context_without_custom_attributes?
+    %w[merge twitter telegram facebook form mail sms].include? context.to_s
+  end
 end

lib/facebook.rb

@@ -305,7 +305,7 @@ result
     ticket = nil
 
     # use transaction
-    Transaction.execute(reset_user_id: true) do
+    Transaction.execute(reset_user_id: true, context: 'facebook') do
       existing_article = Ticket::Article.find_by(message_id: post['id'])
       ticket = if existing_article
                  existing_article.ticket

lib/telegram.rb

@@ -744,7 +744,7 @@ returns
     ticket = nil
 
     # use transaction
-    Transaction.execute(reset_user_id: true) do
+    Transaction.execute(reset_user_id: true, context: 'telegram') do
       user   = to_user(params)
       ticket = to_ticket(params, user, group_id, channel)
       to_article(params, user, ticket, channel)

lib/twitter_sync.rb

@@ -450,7 +450,7 @@ class TwitterSync
     Rails.logger.debug { 'import tweet' }
 
     ticket = nil
-    Transaction.execute(reset_user_id: true) do
+    Transaction.execute(reset_user_id: true, context: 'twitter') do
 
       # check if parent exists
       user = to_user(tweet)

spec/lib/application_handle_info_spec.rb

@@ -3,6 +3,24 @@
 require 'rails_helper'
 
 RSpec.describe ApplicationHandleInfo do
+  shared_context 'safe block execution' do |attribute:|
+    # This `around` block is identical to ApplicationHandleInfo.use.
+    #
+    # Q: So why don't we just use it here to DRY things up?
+    # A: Because that's the method we're trying to test, dummy!
+    #
+    # Q: Why can't we do `before { ApplicationHandleInfo.current = 'foo' }` instead?
+    # A: Because that would change `ApplicationHandleInfo.current` for all subsequent specs.
+    #    (RSpec uses database transactions to keep test environments clean,
+    #    but `ApplicationHandleInfo.current` lives outside of the database.)
+    around do |example|
+      original = described_class.send(attribute)
+      described_class.send("#{attribute}=", 'foo')
+      example.run
+      described_class.send("#{attribute}=", original)
+    end
+  end
+
   describe '.use' do
     it 'requires a block' do
       expect { described_class.use('foo') }
@@ -10,21 +28,7 @@ RSpec.describe ApplicationHandleInfo do
     end
 
     context 'for a given starting ApplicationHandleInfo' do
-      # This `around` block is identical to ApplicationHandleInfo.use.
-      #
-      # Q: So why don't we just use it here to DRY things up?
-      # A: Because that's the method we're trying to test, dummy!
-      #
-      # Q: Why can't we do `before { ApplicationHandleInfo.current = 'foo' }` instead?
-      # A: Because that would change `ApplicationHandleInfo.current` for all subsequent specs.
-      #    (RSpec uses database transactions to keep test environments clean,
-      #    but `ApplicationHandleInfo.current` lives outside of the database.)
-      around do |example|
-        original = described_class.current
-        described_class.current = 'foo'
-        example.run
-        described_class.current = original
-      end
+      include_examples 'safe block execution', attribute: :current
 
       it 'runs the block using the given ApplicationHandleInfo' do
         described_class.use('bar') do
@@ -47,4 +51,56 @@ RSpec.describe ApplicationHandleInfo do
       end
     end
   end
+
+  describe '.in_context' do
+    it 'requires a block' do
+      expect { described_class.use('foo') }
+        .to raise_error(ArgumentError)
+    end
+
+    context 'for a given starting ApplicationHandleInfo' do
+      include_examples 'safe block execution', attribute: :context
+
+      it 'runs the block using the given ApplicationHandleInfo' do
+        described_class.in_context('bar') do
+          expect(described_class.context).to eq('bar')
+        end
+      end
+
+      it 'resets ApplicationHandleInfo to its original value' do
+        described_class.in_context('bar') { nil }
+
+        expect(described_class.context).to eq('foo')
+      end
+
+      context 'when an error is raised in the given block' do
+        it 'does not rescue the error, and still resets ApplicationHandleInfo' do
+          expect { described_class.in_context('bar') { raise } }
+            .to raise_error(StandardError)
+            .and not_change(described_class, :context)
+        end
+      end
+    end
+  end
+
+  describe '.context_without_custom_attributes?' do
+    it 'returns false when set to default context' do
+      expect(described_class).not_to be_context_without_custom_attributes
+    end
+
+    context 'for a given starting ApplicationHandleInfo' do
+      include_examples 'safe block execution', attribute: :context
+
+      it 'returns true when set to context that does not use custom attributes' do
+        described_class.context = 'merge'
+        expect(described_class).to be_context_without_custom_attributes
+      end
+
+      it 'returns true when in .in_context block' do
+        described_class.in_context(:merge) do
+          expect(described_class).to be_context_without_custom_attributes
+        end
+      end
+    end
+  end
 end

spec/models/channel/driver/facebook_spec.rb

@@ -10,7 +10,21 @@ RSpec.describe Channel::Driver::Facebook, use_vcr: true, required_envs: %w[FACEB
 
   let!(:channel) { create(:facebook_channel) }
 
+  # This test requires ENV variables to run
+  # Yes, it runs off VCR cassette
+  # But it requires following ENV variables to be present:
+  #
+  # export FACEBOOK_CUSTOMER_ID=placeholder
+  # export FACEBOOK_CUSTOMER_FIRSTNAME=placeholder
+  # export FACEBOOK_CUSTOMER_LASTNAME=placeholder
+  # export FACEBOOK_PAGE_1_ACCCESS_TOKEN=placeholder
+  # export FACEBOOK_PAGE_1_ID=123
+  # export FACEBOOK_PAGE_1_NAME=placeholder
+  # export FACEBOOK_PAGE_1_POST_ID=placeholder
+  # export FACEBOOK_PAGE_1_POST_COMMENT_ID=placeholder
+  #
   it 'tests full run' do # rubocop:disable RSpec/MultipleExpectations, RSpec/ExampleLength
+    allow(ApplicationHandleInfo).to receive('context=')
     ExternalCredential.create name: :facebook, credentials: { application_id: ENV['FACEBOOK_APPLICATION_ID'], application_secret: ENV['FACEBOOK_APPLICATION_SECRET'] }
 
     channel.fetch
@@ -50,5 +64,7 @@ RSpec.describe Channel::Driver::Facebook, use_vcr: true, required_envs: %w[FACEB
     expect(outbound_article).to be_present
     expect(outbound_article.from).to eq ENV['FACEBOOK_PAGE_1_NAME']
     expect(outbound_article.ticket.articles.count).to be ticket_initial_count + 1
+
+    expect(ApplicationHandleInfo).to have_received('context=').with('facebook').at_least(1)
   end
 end

spec/models/channel/driver/twitter_spec.rb

@@ -745,6 +745,20 @@ RSpec.describe Channel::Driver::Twitter, required_envs: %w[TWITTER_CONSUMER_KEY
   end
 
   describe '#fetch', use_vcr: :time_sensitive do
+    context 'when ApplicationHandleInfo context' do
+      it 'gets switched to "twitter"' do
+        allow(ApplicationHandleInfo).to receive('context=')
+        channel.fetch
+        expect(ApplicationHandleInfo).to have_received('context=').with('twitter').at_least(1)
+      end
+
+      it 'reverts back to default' do
+        allow(ApplicationHandleInfo).to receive('context=')
+        channel.fetch
+        expect(ApplicationHandleInfo.context).not_to eq 'twitter'
+      end
+    end
+
     describe 'rate limiting' do
       before do
         allow(Rails.env).to receive(:test?).and_return(false)

spec/models/object_manager/attribute/validation_spec.rb

@@ -97,5 +97,41 @@ RSpec.describe ObjectManager::Attribute::Validation, application_handle: 'applic
         end
       end
     end
+
+    context 'when custom attribute exists' do
+      before do
+        allow(subject).to receive(:attributes_unchanged?) # rubocop:disable RSpec/SubjectStub
+      end
+
+      it 'runs validation in default context' do
+        ApplicationHandleInfo.in_context(nil) do
+          subject.validate(record)
+        end
+
+        expect(subject).to have_received(:attributes_unchanged?) # rubocop:disable RSpec/SubjectStub
+      end
+
+      it 'does not run validations in contexts that do not use custom attributes' do
+        ApplicationHandleInfo.in_context('merge') do
+          subject.validate(record)
+        end
+
+        expect(subject).not_to have_received(:attributes_unchanged?) # rubocop:disable RSpec/SubjectStub
+      end
+    end
+  end
+
+  describe '#validation_needed' do
+    it 'runs validation in default context' do
+      ApplicationHandleInfo.in_context(nil) do
+        expect(subject.send(:validation_needed?)).to be true
+      end
+    end
+
+    it 'does not run validations in contexts that do not use custom attributes' do
+      ApplicationHandleInfo.in_context('merge') do
+        expect(subject.send(:validation_needed?)).to be false
+      end
+    end
   end
 end

spec/models/ticket_spec.rb

@@ -418,6 +418,21 @@ RSpec.describe Ticket, type: :model do
           expect(log).to include(start_with("Another ticket was merged into ticket (#{target_ticket.title})"))
         end
       end
+
+      context 'ApplicationHandleInfo context' do
+        it 'gets switched to "merge"' do
+          allow(ApplicationHandleInfo).to receive('context=')
+          ticket.merge_to(ticket_id: target_ticket.id, user_id: 1)
+          expect(ApplicationHandleInfo).to have_received('context=').with('merge').at_least(1)
+        end
+
+        it 'reverts back to default' do
+          allow(ApplicationHandleInfo).to receive('context=')
+          ticket.merge_to(ticket_id: target_ticket.id, user_id: 1)
+
+          expect(ApplicationHandleInfo.context).not_to eq 'merge'
+        end
+      end
     end
 
     describe '#perform_changes' do

spec/requests/form_spec.rb

@@ -233,5 +233,27 @@ RSpec.describe 'Form', type: :request, searchindex: true do
 
       expect(response).to have_http_status(:forbidden)
     end
+
+    context 'when ApplicationHandleInfo context' do
+      let(:fingerprint) { SecureRandom.hex(40) }
+      let(:token)       { json_response['token'] }
+
+      before do
+        Setting.set('form_ticket_create', true)
+        post '/api/v1/form_config', params: { fingerprint: fingerprint }, as: :json
+      end
+
+      it 'gets switched to "form"' do
+        allow(ApplicationHandleInfo).to receive('context=')
+        post '/api/v1/form_submit', params: { fingerprint: fingerprint, token: token, name: 'Bob Smith', email: 'discard@znuny.com', title: 'test-last', body: 'hello' }, as: :json
+        expect(ApplicationHandleInfo).to have_received('context=').with('form').at_least(1)
+      end
+
+      it 'reverts back to default' do
+        allow(ApplicationHandleInfo).to receive('context=')
+        post '/api/v1/form_submit', params: { fingerprint: fingerprint, token: token, name: 'Bob Smith', email: 'discard@znuny.com', title: 'test-last', body: 'hello' }, as: :json
+        expect(ApplicationHandleInfo.context).not_to eq 'form'
+      end
+    end
   end
 end

spec/requests/integration/telegram_spec.rb

@@ -398,6 +398,20 @@ RSpec.describe 'Telegram Webhook Integration', type: :request do
         expect(ticket2.articles.first.to).to eq('@ChrispressoBot2')
       end
 
+      context 'when ApplicationHandleInfo context' do
+        it 'gets switched to "telegram"' do
+          allow(ApplicationHandleInfo).to receive('context=')
+          post callback_url, params: read_message('private', 'text'), as: :json
+          expect(ApplicationHandleInfo).to have_received('context=').with('telegram').at_least(1)
+        end
+
+        it 'reverts back to default' do
+          allow(ApplicationHandleInfo).to receive('context=')
+          post callback_url, params: read_message('private', 'text'), as: :json
+          expect(ApplicationHandleInfo.context).not_to eq 'telegram'
+        end
+      end
+
     end
 
     def read_message(type, file)

spec/requests/integration/twilio_sms_spec.rb

@@ -201,6 +201,28 @@ RSpec.describe 'Twilio SMS', type: :request do
       expect(customer.id).to eq(User.last.id)
     end
 
+    it 'does basic call when ticket has a custom attribute', db_strategy: :reset do
+      create(:object_manager_attribute_text, screens: attributes_for(:required_screen))
+      ObjectManager::Attribute.migration_execute
+
+      UserInfo.current_user_id = 1
+      create(
+        :channel,
+        area:     'Sms::Account',
+        options:  {
+          adapter:       'sms/twilio',
+          webhook_token: 'f409460e50f76d331fdac8ba7b7963b6',
+          account_id:    '111',
+          token:         '223',
+          sender:        '333',
+        },
+        group_id: Group.first.id,
+      )
+
+      post '/api/v1/sms_webhook/f409460e50f76d331fdac8ba7b7963b6', params: read_message('inbound_sms1'), as: :json
+      expect(response).to have_http_status(:ok)
+    end
+
     def read_message(file)
       JSON.parse(File.read(Rails.root.join('test', 'data', 'twilio', "#{file}.json")))
     end

spec/support/application_handle_info.rb

@@ -6,4 +6,10 @@ RSpec.configure do |config|
       example.run
     end
   end
+
+  config.around(:each, :application_handle_context) do |example|
+    ApplicationHandleInfo.in_context(example.metadata[:application_handle_context]) do
+      example.run
+    end
+  end
 end