Fixes #3817 - Ticket object does not get a rollback after failing bulk action

app/assets/javascripts/app/controllers/ticket_overview.coffee

@@ -1,4 +1,6 @@
 class App.TicketOverview extends App.Controller
+  @extend App.TicketMassUpdatable
+
   className: 'overviews'
   activeFocus: 'nav'
   mouse:
@@ -192,65 +194,35 @@ class App.TicketOverview extends App.Controller
         duration: 300
 
   performBatchAction: (items, action, id, groupId) ->
-    if action is 'macro'
-      @batchCount = items.length
-      @batchCountIndex = 0
-      macro = App.Macro.find(id)
-      for item in items
-        #console.log "perform action #{action} with id #{id} on ", $(item).val()
-        ticket = App.Ticket.find($(item).val())
-        article = {}
-        App.Ticket.macro(
-          macro: macro.perform
-          ticket: ticket
-          article: article
-        )
-        ticket.article = article
-        ticket.save(
-          done: (r) =>
-            @batchCountIndex++
-
-            # refresh view after all tickets are proceeded
-            if @batchCountIndex == @batchCount
-              App.Event.trigger('overview:fetch')
-        )
-      return
+    ticket_ids = items.toArray().map (item) -> $(item).val()
+
+    switch action
+      when 'macro'
+        path = 'macro'
+        data =
+          ticket_ids: ticket_ids
+          macro_id:   id
+
+      when 'user_assign'
+        path = 'update'
+
+        data =
+          ticket_ids: ticket_ids
+          attributes:
+            owner_id: id
 
-    if action is 'user_assign'
-      @batchCount = items.length
-      @batchCountIndex = 0
-      for item in items
-        #console.log "perform action #{action} with id #{id} on ", $(item).val()
-        ticket = App.Ticket.find($(item).val())
-        ticket.owner_id = id
         if !_.isEmpty(groupId)
-          ticket.group_id = groupId
-        ticket.save(
-          done: (r) =>
-            @batchCountIndex++
-
-            # refresh view after all tickets are proceeded
-            if @batchCountIndex == @batchCount
-              App.Event.trigger('overview:fetch')
-        )
-      return
+          data.attributes.group_id = groupId
 
-    if action is 'group_assign'
-      @batchCount = items.length
-      @batchCountIndex = 0
-      for item in items
-        #console.log "perform action #{action} with id #{id} on ", $(item).val()
-        ticket = App.Ticket.find($(item).val())
-        ticket.group_id = id
-        ticket.save(
-          done: (r) =>
-            @batchCountIndex++
-
-            # refresh view after all tickets are proceeded
-            if @batchCountIndex == @batchCount
-              App.Event.trigger('overview:fetch')
-        )
-      return
+      when 'group_assign'
+        path = 'update'
+
+        data =
+          ticket_ids: ticket_ids
+          attributes:
+            group_id: id
+
+    @ajax_mass(path, data)
 
   showBatchOverlay: ->
     @batchOverlay.addClass('is-visible')

app/assets/javascripts/app/controllers/widget/ticket_bulk_form.coffee

@@ -1,4 +1,6 @@
 class App.TicketBulkForm extends App.Controller
+  @extend App.TicketMassUpdatable
+
   className: 'bulkAction hide'
 
   events:
@@ -171,6 +173,10 @@ class App.TicketBulkForm extends App.Controller
 
     params = @formParam(e.target)
 
+    for key, value of params
+      if value == '' || value == null
+        delete params[key]
+
     for ticket_id in ticket_ids
       ticket = App.Ticket.find(ticket_id)
 
@@ -204,74 +210,34 @@ class App.TicketBulkForm extends App.Controller
         @cancel()
         return
 
-    @bulkCountIndex = 0
-    for ticket_id in ticket_ids
-      ticket = App.Ticket.find(ticket_id)
-
-      # update ticket
-      ticketUpdate = @ticketMergeParams(params)
-
-      # validate article
-      if params['body']
-        article = new App.TicketArticle
-        params.from      = @Session.get().displayName()
-        params.ticket_id = ticket.id
-        params.form_id   = @form_id
+    if params['body']
+      article = new App.TicketArticle
+      params.from      = @Session.get().displayName()
+      params.form_id   = @form_id
 
-        sender           = App.TicketArticleSender.findByAttribute('name', 'Agent')
-        type             = App.TicketArticleType.find(params['type_id'])
-        params.sender_id = sender.id
+      sender           = App.TicketArticleSender.findByAttribute('name', 'Agent')
+      type             = App.TicketArticleType.find(params['type_id'])
+      params.sender_id = sender.id
 
-        if !params['internal']
-          params['internal'] = false
+      if !params['internal']
+        params['internal'] = false
 
-        @log 'notice', 'update article', params, sender
-        article.load(params)
-        errors = article.validate()
-        if errors
-          @log 'error', 'update article', errors
-          @formEnable(e)
-          return
+      @log 'notice', 'update article', params, sender
+      article.load(params)
+      errors = article.validate()
+      if errors
+        @log 'error', 'update article', errors
+        @formEnable(e)
+        return
 
-      ticket.load(ticketUpdate)
 
-      # if title is empty - ticket can't processed, set ?
-      if _.isEmpty(ticket.title)
-        ticket.title = '-'
+    data =
+      ticket_ids: ticket_ids
+      attributes: params
+      article: article?.attributes()
 
-      @saveTicketArticle(ticket, article)
-
-    @holder.find('.table').find('[name="bulk"]:checked').prop('checked', false)
-    App.Event.trigger('notify', {
-      type: 'success'
-      msg: App.i18n.translateContent('Bulk action executed!')
-    })
-
-  saveTicketArticle: (ticket, article) =>
-    ticket.save(
-      done: (r) =>
-        @bulkCountIndex++
-
-        # reset form after save
-        if article
-          article.save(
-            fail: (r) =>
-              @log 'error', 'update article', r
-          )
-
-        # refresh view after all tickets are proceeded
-        if @bulkCountIndex == @bulkCount
-          @render()
-          @hide()
-
-          # fetch overview data again
-          App.Event.trigger('overview:fetch')
-
-      fail: (r) =>
-        @bulkCountIndex++
-        @log 'error', 'update ticket', r
-        App.Event.trigger 'notify', {
-          type: 'error'
-          msg: App.i18n.translateContent('Can\'t update Ticket %s!', ticket.number)
-        }
+    @ajax_mass_update(data, =>
+      @holder.find('.table').find('[name="bulk"]:checked').prop('checked', false)
+      @render()
+      @hide()
     )

app/assets/javascripts/app/lib/app_post/ajax.coffee

@@ -82,6 +82,9 @@ class _ajaxSingleton
 
     # show error messages
     $(document).bind('ajaxError', (e, jqxhr, settings, exception) ->
+      if settings.failResponseNoTrigger
+        return
+
       status = jqxhr.status
       detail = jqxhr.responseText
       if !status && !detail

app/assets/javascripts/app/lib/mixins/ticket_mass_updatable.coffee

@@ -0,0 +1,46 @@
+InstanceMethods =
+  ajax_mass_update: (data, success) ->
+    @ajax_mass('update', data, success)
+
+  ajax_mass_macro: (data, success) ->
+    @ajax_mass('macro', data, success)
+
+  ajax_mass: (path, data, success) ->
+    @startLoading()
+
+    @ajax(
+      id: 'bulk_update'
+      type: 'POST'
+      url:   "#{@apiPath}/tickets/mass_#{path}"
+      data: JSON.stringify(data)
+      success: (data) =>
+        @stopLoading()
+        App.Collection.loadAssets(data.assets)
+        App.Event.trigger('overview:fetch')
+        App.Event.trigger('notify', {
+          type: 'success'
+          msg: App.i18n.translateContent(__('Bulk action executed!'))
+        })
+
+        success?()
+
+      error: (xhr, status, error) =>
+        @stopLoading()
+
+        return if xhr.status != 422
+
+        message = if xhr.responseJSON.error && ticket = App.Ticket.find(xhr.responseJSON.ticket_id)
+                    App.i18n.translateContent(__('Ticket failed to save: %s'), ticket.title)
+                  else
+                    error
+
+        new App.ErrorModal(
+          head: __('Bulk action failed')
+          contentInline: message
+          container: @el.closest('.content')
+        )
+    )
+
+App.TicketMassUpdatable =
+  extended: ->
+    @include InstanceMethods

app/assets/javascripts/app/lib/spine/ajax.coffee

@@ -120,6 +120,20 @@ class Base
     @queue request
     promise
 
+  ajaxQueueOptions: (options, defaultUrl = null, defaultMethod = Ajax.config.loadMethod, jsonObject = null) ->
+    hash = {
+      type:                  options.method || defaultMethod
+      url:                   options.url || defaultUrl
+      parallel:              options.parallel
+      failResponseNoTrigger: options.failResponseNoTrigger
+    }
+
+    if jsonObject
+      hash.data        = jsonObject.toJSON()
+      hash.contentType = 'application/json'
+
+    hash
+
   ajaxSettings: (params, defaults) ->
     $.extend({}, @defaults, defaults, params)
 
@@ -128,23 +142,13 @@ class Collection extends Base
 
   find: (id, params, options = {}) ->
     record = new @model(id: id)
-    @ajaxQueue(
-      params, {
-        type: options.method or Ajax.config.loadMethod
-        url: options.url or Ajax.getURL(record)
-        parallel: options.parallel
-      }
-    ).done(@recordsResponse(options))
+    @ajaxQueue(params, @ajaxQueueOptions(options, Ajax.getURL(record)))
+      .done(@recordsResponse(options))
       .fail(@failResponse(options))
 
   all: (params, options = {}) ->
-    @ajaxQueue(
-      params, {
-        type: options.method or Ajax.config.loadMethod
-        url: options.url or Ajax.getURL(@model)
-        parallel: options.parallel
-      }
-    ).done(@recordsResponse(options))
+    @ajaxQueue(params, @ajaxQueueOptions(options, Ajax.getURL(@model)))
+      .done(@recordsResponse(options))
       .fail(@failResponse(options))
 
   fetch: (params = {}, options = {}) ->
@@ -180,47 +184,23 @@ class Singleton extends Base
     @model = @record.constructor
 
   reload: (params, options = {}) ->
-    @ajaxQueue(
-      params, {
-        type: options.method or Ajax.config.loadMethod
-        url: options.url
-        parallel: options.parallel
-      }, @record
-    ).done(@recordResponse(options))
+    @ajaxQueue(params, @ajaxQueueOptions(options), @record)
+      .done(@recordResponse(options))
       .fail(@failResponse(options))
 
   create: (params, options = {}) ->
-    @ajaxQueue(
-      params, {
-        type: options.method or Ajax.config.createMethod
-        contentType: 'application/json'
-        data: @record.toJSON()
-        url: options.url or Ajax.getCollectionURL(@record)
-        parallel: options.parallel
-      }
-    ).done(@recordResponse(options))
+    @ajaxQueue(params, @ajaxQueueOptions(options, Ajax.getCollectionURL(@record), Ajax.config.createMethod, @record))
+      .done(@recordResponse(options))
       .fail(@failResponse(options))
 
   update: (params, options = {}) ->
-    @ajaxQueue(
-      params, {
-        type: options.method or Ajax.config.updateMethod
-        contentType: 'application/json'
-        data: @record.toJSON()
-        url: options.url
-        parallel: options.parallel
-      }, @record
-    ).done(@recordResponse(options))
+    @ajaxQueue(params, @ajaxQueueOptions(options, null, Ajax.config.updateMethod, @record), @record)
+      .done(@recordResponse(options))
       .fail(@failResponse(options))
 
   destroy: (params, options = {}) ->
-    @ajaxQueue(
-      params, {
-        type: options.method or Ajax.config.destroyMethod
-        url: options.url
-        parallel: options.parallel
-      }, @record
-    ).done(@recordResponse(options))
+    @ajaxQueue(params, @ajaxQueueOptions(options, null, Ajax.config.destroyMethod), @record)
+      .done(@recordResponse(options))
       .fail(@failResponse(options))
 
   # Private

app/controllers/macros_controller.rb

@@ -1,6 +1,7 @@
 # Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
 
 class MacrosController < ApplicationController
+  prepend_before_action :authorize!
   prepend_before_action :authentication_check
 
 =begin
@@ -50,7 +51,7 @@ curl http://localhost/api/v1/macros.json -v -u #{login}:#{password}
 =end
 
   def index
-    model_index_render(Macro, params)
+    model_index_render(policy_scope(Macro), params)
   end
 
 =begin
@@ -71,7 +72,7 @@ curl http://localhost/api/v1/macros/#{id}.json -v -u #{login}:#{password}
 =end
 
   def show
-    model_show_render(Macro, params)
+    model_show_render(policy_scope(Macro), params)
   end
 
 =begin

app/controllers/tickets_mass_controller.rb

@@ -0,0 +1,85 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class TicketsMassController < ApplicationController
+  include CreatesTicketArticles
+
+  prepend_before_action :authentication_check
+  before_action :fetch_tickets
+
+  def macro
+    macro = Macro.find params[:macro_id]
+
+    applicable = macro.applicable_on? @tickets
+
+    if !applicable
+      render json: {
+        error:            __('Macro group restrictions do not cover some tickets'),
+        blocking_tickets: applicable.blocking_tickets.map(&:id)
+      }, status: :unprocessable_entity
+
+      return
+    end
+
+    execute_transaction(@tickets) do |ticket|
+      ticket.screen = 'edit'
+      ticket.perform_changes macro, 'macro', ticket, current_user.id
+    end
+  end
+
+  def update
+    clean_params = clean_update_params
+
+    execute_transaction(@tickets) do |ticket|
+      ticket.update!(clean_params) if clean_params
+      if params[:article].present?
+        article_create(ticket, params[:article])
+      end
+    end
+  end
+
+  private
+
+  def fetch_tickets
+    @tickets = Ticket.find params[:ticket_ids]
+
+    @tickets.each do |elem|
+      authorize!(elem, :follow_up?)
+      authorize!(elem, :update?)
+    end
+  rescue Pundit::NotAuthorizedError => e
+    render json: { error: true, ticket_id: e.record.id }, status: :unprocessable_entity
+  end
+
+  def clean_update_params
+    return if params[:attributes].blank?
+
+    clean_params = Ticket.association_name_to_id_convert(params.require(:attributes))
+    clean_params = Ticket.param_cleanup(clean_params, true)
+    clean_params.reject! { |_k, v| v.blank? }
+
+    clean_params[:screen] = 'edit'
+    clean_params.delete('number')
+
+    clean_params
+  end
+
+  def execute_transaction(tickets, &block)
+    failed_record = nil
+
+    ActiveRecord::Base.transaction do
+      tickets.each(&block)
+
+      assets = ApplicationModel::CanAssets.reduce tickets
+
+      render json: { ticket_ids: tickets.map(&:id), assets: assets }, status: :ok
+    rescue => e
+      raise e if !e.try(:record)
+
+      failed_record = e.record
+
+      raise ActiveRecord::Rollback
+    end
+
+    render json: { error: true, ticket_id: failed_record.id }, status: :unprocessable_entity if failed_record
+  end
+end

app/models/macro.rb

@@ -16,4 +16,23 @@ class Macro < ApplicationModel
   sanitized_html :note
 
   collection_push_permission('ticket.agent')
+
+  ApplicableOn = Struct.new(:result, :blocking_tickets) do
+    delegate :==, to: :result
+    delegate :!, to: :result
+
+    def error_message
+      "Macro blocked by: #{blocking_tickets.join(', ')}"
+    end
+  end
+
+  def applicable_on?(tickets)
+    tickets = Array(tickets)
+
+    return ApplicableOn.new(true, []) if group_ids.blank?
+
+    blocking = tickets.reject { |elem| group_ids.include? elem.group_id }
+
+    ApplicableOn.new(blocking.none?, blocking)
+  end
 end

app/policies/controllers/macros_controller_policy.rb

@@ -0,0 +1,7 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class Controllers::MacrosControllerPolicy < Controllers::ApplicationControllerPolicy
+  default_permit! ['admin.macro']
+
+  permit! %i[index show], to: ['ticket.agent']
+end

app/policies/macro_policy.rb

@@ -0,0 +1,4 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class MacroPolicy < ApplicationPolicy
+end