From a5c728609da553a713cee28b5fc97ad4e40c284c Mon Sep 17 00:00:00 2001 From: Mantas Masalskis Date: Mon, 20 Dec 2021 14:02:30 +0100 Subject: [PATCH] Fixes #3817 - Ticket object does not get a rollback after failing bulk action --- .../app/controllers/ticket_overview.coffee | 78 +++------ .../widget/ticket_bulk_form.coffee | 94 ++++------ .../javascripts/app/lib/app_post/ajax.coffee | 3 + .../lib/mixins/ticket_mass_updatable.coffee | 46 +++++ .../javascripts/app/lib/spine/ajax.coffee | 72 +++----- app/controllers/macros_controller.rb | 5 +- app/controllers/tickets_mass_controller.rb | 85 +++++++++ app/models/macro.rb | 19 ++ .../controllers/macros_controller_policy.rb | 7 + app/policies/macro_policy.rb | 4 + app/policies/macro_policy/scope.rb | 36 ++++ config/routes/ticket.rb | 2 + i18n/zammad.pot | 18 +- spec/factories/core_workflow.rb | 34 ++++ spec/models/macro_spec.rb | 60 +++++++ spec/policies/macro_policy.rb | 4 + spec/policies/macro_policy/scope_spec.rb | 58 +++++++ spec/requests/macro_spec.rb | 164 ++++++++++++++++++ spec/requests/tickets_mass_spec.rb | 90 ++++++++++ spec/system/ticket/view_spec.rb | 74 ++++++++ 20 files changed, 783 insertions(+), 170 deletions(-) create mode 100644 app/assets/javascripts/app/lib/mixins/ticket_mass_updatable.coffee create mode 100644 app/controllers/tickets_mass_controller.rb create mode 100644 app/policies/controllers/macros_controller_policy.rb create mode 100644 app/policies/macro_policy.rb create mode 100644 app/policies/macro_policy/scope.rb create mode 100644 spec/policies/macro_policy.rb create mode 100644 spec/policies/macro_policy/scope_spec.rb create mode 100644 spec/requests/macro_spec.rb create mode 100644 spec/requests/tickets_mass_spec.rb diff --git a/app/assets/javascripts/app/controllers/ticket_overview.coffee b/app/assets/javascripts/app/controllers/ticket_overview.coffee index 5abf27e09..a5d1beada 100644 --- a/app/assets/javascripts/app/controllers/ticket_overview.coffee +++ b/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++ + ticket_ids = items.toArray().map (item) -> $(item).val() - # refresh view after all tickets are proceeded - if @batchCountIndex == @batchCount - App.Event.trigger('overview:fetch') - ) - return + 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++ + data.attributes.group_id = groupId - # refresh view after all tickets are proceeded - if @batchCountIndex == @batchCount - App.Event.trigger('overview:fetch') - ) - return + when 'group_assign' + path = 'update' - 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++ + data = + ticket_ids: ticket_ids + attributes: + group_id: id - # refresh view after all tickets are proceeded - if @batchCountIndex == @batchCount - App.Event.trigger('overview:fetch') - ) - return + @ajax_mass(path, data) showBatchOverlay: -> @batchOverlay.addClass('is-visible') diff --git a/app/assets/javascripts/app/controllers/widget/ticket_bulk_form.coffee b/app/assets/javascripts/app/controllers/widget/ticket_bulk_form.coffee index d4ce81346..c4d628631 100644 --- a/app/assets/javascripts/app/controllers/widget/ticket_bulk_form.coffee +++ b/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) + if params['body'] + article = new App.TicketArticle + params.from = @Session.get().displayName() + params.form_id = @form_id - # update ticket - ticketUpdate = @ticketMergeParams(params) + sender = App.TicketArticleSender.findByAttribute('name', 'Agent') + type = App.TicketArticleType.find(params['type_id']) + params.sender_id = sender.id - # 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['internal'] + params['internal'] = false - sender = App.TicketArticleSender.findByAttribute('name', 'Agent') - type = App.TicketArticleType.find(params['type_id']) - params.sender_id = sender.id + @log 'notice', 'update article', params, sender + article.load(params) + errors = article.validate() + if errors + @log 'error', 'update article', errors + @formEnable(e) + return - 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 + data = + ticket_ids: ticket_ids + attributes: params + article: article?.attributes() - ticket.load(ticketUpdate) - - # if title is empty - ticket can't processed, set ? - if _.isEmpty(ticket.title) - ticket.title = '-' - - @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() ) diff --git a/app/assets/javascripts/app/lib/app_post/ajax.coffee b/app/assets/javascripts/app/lib/app_post/ajax.coffee index d79e0d39d..db117d066 100644 --- a/app/assets/javascripts/app/lib/app_post/ajax.coffee +++ b/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 diff --git a/app/assets/javascripts/app/lib/mixins/ticket_mass_updatable.coffee b/app/assets/javascripts/app/lib/mixins/ticket_mass_updatable.coffee new file mode 100644 index 000000000..0560d05f8 --- /dev/null +++ b/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 diff --git a/app/assets/javascripts/app/lib/spine/ajax.coffee b/app/assets/javascripts/app/lib/spine/ajax.coffee index 491bea7ea..6af970725 100644 --- a/app/assets/javascripts/app/lib/spine/ajax.coffee +++ b/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 diff --git a/app/controllers/macros_controller.rb b/app/controllers/macros_controller.rb index 12b7b318a..f6363eca7 100644 --- a/app/controllers/macros_controller.rb +++ b/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 diff --git a/app/controllers/tickets_mass_controller.rb b/app/controllers/tickets_mass_controller.rb new file mode 100644 index 000000000..555c2cd2e --- /dev/null +++ b/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 diff --git a/app/models/macro.rb b/app/models/macro.rb index b3801223b..be0ff8d52 100644 --- a/app/models/macro.rb +++ b/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 diff --git a/app/policies/controllers/macros_controller_policy.rb b/app/policies/controllers/macros_controller_policy.rb new file mode 100644 index 000000000..71ff45382 --- /dev/null +++ b/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 diff --git a/app/policies/macro_policy.rb b/app/policies/macro_policy.rb new file mode 100644 index 000000000..aab20ad4e --- /dev/null +++ b/app/policies/macro_policy.rb @@ -0,0 +1,4 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +class MacroPolicy < ApplicationPolicy +end diff --git a/app/policies/macro_policy/scope.rb b/app/policies/macro_policy/scope.rb new file mode 100644 index 000000000..6cf2fecb0 --- /dev/null +++ b/app/policies/macro_policy/scope.rb @@ -0,0 +1,36 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +class MacroPolicy < ApplicationPolicy + class Scope < ApplicationPolicy::Scope + + def resolve + if user.permissions?('admin.macro') + scope.all + elsif user.permissions?('ticket.agent') + scope + .left_joins(:groups) + .group('macros.id') + .having(agent_having_groups) + else + scope.none + end + end + + private + + def agent_having_groups + base_query = 'SELECT Count(*) FROM groups_macros WHERE groups_macros.macro_id = macros.id' + + having = "((#{base_query}) = 0)" + + groups = user.groups.access(:change, :create) + + if groups.any? + groups_matcher = groups.map(&:id).join(',') + having += " OR ((#{base_query} AND groups_macros.group_id IN (#{groups_matcher})) > 0)" + end + + having + end + end +end diff --git a/config/routes/ticket.rb b/config/routes/ticket.rb index 2c9fa0bbb..8274da43b 100644 --- a/config/routes/ticket.rb +++ b/config/routes/ticket.rb @@ -11,6 +11,8 @@ Zammad::Application.routes.draw do match api_path + '/tickets', to: 'tickets#create', via: :post match api_path + '/tickets/:id', to: 'tickets#update', via: :put match api_path + '/tickets/:id', to: 'tickets#destroy', via: :delete + match api_path + '/tickets/mass_macro', to: 'tickets_mass#macro', via: :post + match api_path + '/tickets/mass_update', to: 'tickets_mass#update', via: :post match api_path + '/ticket_create', to: 'tickets#ticket_create', via: :get match api_path + '/ticket_split', to: 'tickets#ticket_split', via: :get match api_path + '/ticket_history/:id', to: 'tickets#ticket_history', via: :get diff --git a/i18n/zammad.pot b/i18n/zammad.pot index 2715e702d..252d5bd9d 100644 --- a/i18n/zammad.pot +++ b/i18n/zammad.pot @@ -1221,10 +1221,14 @@ msgstr "" msgid "Browser too old!" msgstr "" -#: app/assets/javascripts/app/controllers/widget/ticket_bulk_form.coffee +#: app/assets/javascripts/app/lib/mixins/ticket_mass_updatable.coffee msgid "Bulk action executed!" msgstr "" +#: app/assets/javascripts/app/lib/mixins/ticket_mass_updatable.coffee +msgid "Bulk action failed" +msgstr "" + #: app/assets/javascripts/app/controllers/widget/ticket_bulk_form.coffee msgid "Bulk action stopped %s!" msgstr "" @@ -1361,10 +1365,6 @@ msgstr "" msgid "Can't send spool, session not authenticated" msgstr "" -#: app/assets/javascripts/app/controllers/widget/ticket_bulk_form.coffee -msgid "Can't update Ticket %s!" -msgstr "" - #: app/assets/javascripts/app/controllers/_profile/password.coffee #: app/assets/javascripts/app/controllers/password_reset_verify.coffee msgid "Can't update password, your entered passwords do not match. Please try again!" @@ -5522,6 +5522,10 @@ msgstr "" msgid "Macro" msgstr "" +#: app/controllers/tickets_mass_controller.rb +msgid "Macro group restrictions do not cover some tickets" +msgstr "" + #: app/assets/javascripts/app/controllers/macro.coffee #: db/seeds/permissions.rb msgid "Macros" @@ -9112,6 +9116,10 @@ msgstr "" msgid "Ticket escalation" msgstr "" +#: app/assets/javascripts/app/lib/mixins/ticket_mass_updatable.coffee +msgid "Ticket failed to save: %s" +msgstr "" + #: app/assets/javascripts/app/controllers/_profile/notification.coffee msgid "Ticket reminder reached" msgstr "" diff --git a/spec/factories/core_workflow.rb b/spec/factories/core_workflow.rb index d4755d542..ceadf862a 100644 --- a/spec/factories/core_workflow.rb +++ b/spec/factories/core_workflow.rb @@ -6,5 +6,39 @@ FactoryBot.define do changeable { false } created_by_id { 1 } updated_by_id { 1 } + + trait :active_and_screen do + transient do + screen { 'edit' } + end + + preferences { { screen: screen } } + active { true } + end + + trait :condition_group do + transient do + group { nil } + end + + condition_saved do + { 'ticket.group_id': { operator: 'is', value: group.id.to_s } } + end + end + + trait :perform_action do + transient do + object_name { 'Ticket' } + key { 'ticket.priority_id' } + operator { 'remove_option' } + value { '3' } + end + + perform do + { key => { operator: operator, operator => value } } + end + + object { object_name } + end end end diff --git a/spec/models/macro_spec.rb b/spec/models/macro_spec.rb index 90939614a..a42eec914 100644 --- a/spec/models/macro_spec.rb +++ b/spec/models/macro_spec.rb @@ -7,4 +7,64 @@ require 'models/concerns/has_xss_sanitized_note_examples' RSpec.describe Macro, type: :model do it_behaves_like 'HasCollectionUpdate', collection_factory: :macro it_behaves_like 'HasXssSanitizedNote', model_factory: :macro + + describe 'Instance methods:' do + describe '#applicable_on?' do + let(:ticket) { create(:ticket) } + let(:ticket_a) { create(:ticket, group: group_a) } + let(:ticket_b) { create(:ticket, group: group_b) } + let(:ticket_c) { create(:ticket, group: group_c) } + let(:group_a) { create(:group) } + let(:group_b) { create(:group) } + let(:group_c) { create(:group) } + + context 'when macro has no groups' do + subject(:macro) { create(:macro, groups: []) } + + it 'return true for a single group' do + expect(macro).to be_applicable_on(ticket) + end + + it 'return true for multiple tickets' do + expect(macro).to be_applicable_on([ticket, ticket_a, ticket_b]) + end + end + + context 'when macro has a single group' do + subject(:macro) { create(:macro, groups: [group_a]) } + + it 'returns true if macro group matches ticket' do + expect(macro).to be_applicable_on(ticket_a) + end + + it 'returns false if macro group does not match ticket' do + expect(macro).not_to be_applicable_on(ticket_b) + end + + it 'returns false if macro group match a ticket and not the other' do + expect(macro).not_to be_applicable_on([ticket_a, ticket_b]) + end + end + + context 'when macro has multiple groups' do + subject(:macro) { create(:macro, groups: [group_a, group_c]) } + + it 'returns true if macro groups include ticket group' do + expect(macro).to be_applicable_on(ticket_a) + end + + it 'returns false if macro groups do not include ticket group' do + expect(macro).not_to be_applicable_on(ticket_b) + end + + it 'returns true if macro groups match tickets groups' do + expect(macro).to be_applicable_on([ticket_a, ticket_c]) + end + + it 'returns false if macro groups does not match one of tickets group' do + expect(macro).not_to be_applicable_on([ticket_a, ticket_b]) + end + end + end + end end diff --git a/spec/policies/macro_policy.rb b/spec/policies/macro_policy.rb new file mode 100644 index 000000000..aab20ad4e --- /dev/null +++ b/spec/policies/macro_policy.rb @@ -0,0 +1,4 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +class MacroPolicy < ApplicationPolicy +end diff --git a/spec/policies/macro_policy/scope_spec.rb b/spec/policies/macro_policy/scope_spec.rb new file mode 100644 index 000000000..914ca8aa2 --- /dev/null +++ b/spec/policies/macro_policy/scope_spec.rb @@ -0,0 +1,58 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +require 'rails_helper' + +RSpec.describe MacroPolicy::Scope do + subject(:scope) { described_class.new(user, original_collection) } + + let(:original_collection) { Macro } + + let(:group_a) { create(:group) } + let(:macro_a) { create(:macro, groups: [group_a]) } + let(:group_b) { create(:group) } + let(:macro_b) { create(:macro, groups: [group_b]) } + let(:macro_c) { create(:macro, groups: []) } + + before do + Macro.destroy_all + macro_a && macro_b && macro_c + end + + describe '#resolve' do + context 'without user' do + let(:user) { nil } + + it 'throws exception' do + expect { scope.resolve }.to raise_error %r{Authentication required} + end + end + + context 'with customer' do + let(:user) { create(:customer) } + + it 'returns empty' do + expect(scope.resolve).to be_empty + end + end + + context 'with agent' do + let(:user) { create(:agent) } + + before { user.groups << group_a } + + it 'returns global and group macro' do + expect(scope.resolve).to match_array [macro_a, macro_c] + end + end + + context 'with admin' do + let(:user) { create(:admin) } + + before { user.groups << group_b } + + it 'returns all macros' do + expect(scope.resolve).to match_array [macro_a, macro_b, macro_c] + end + end + end +end diff --git a/spec/requests/macro_spec.rb b/spec/requests/macro_spec.rb new file mode 100644 index 000000000..1c68f9690 --- /dev/null +++ b/spec/requests/macro_spec.rb @@ -0,0 +1,164 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +require 'rails_helper' + +RSpec.describe 'Macro', type: :request, authenticated_as: :user do + let(:successful_params) do + { + name: 'asd', + perform: { + 'ticket.state_id': { + value: '2' + } + }, + ux_flow_next_up: 'none', + note: '', + group_ids: nil, + active: true + } + end + + describe '#create' do + before do + post '/api/v1/macros', params: successful_params, as: :json + end + + context 'when user is not allowed to create macro' do + let(:user) { create(:agent) } + + it 'does not create macro' do + expect(response).to have_http_status(:forbidden) + end + end + + context 'when user is allowed to create macros' do + let(:user) { create(:admin) } + + it 'creates macro' do + expect(response).to have_http_status(:created) + end + end + end + + describe '#update' do + let(:macro) { create(:macro, name: 'test') } + + before do + put "/api/v1/macros/#{macro.id}", params: successful_params, as: :json + end + + context 'when user is not allowed to update macro' do + let(:user) { create(:agent) } + + it 'does not update macro' do + expect(response).to have_http_status(:forbidden) + end + + it 'macro is not changed' do + expect(macro.reload.name).to eq 'test' + end + end + + context 'when user is allowed to update macros' do + let(:user) { create(:admin) } + + it 'request is successful' do + expect(response).to have_http_status(:ok) + end + + it 'macro is changed' do + expect(macro.reload.name).to eq 'asd' + end + end + end + + describe '#destroy' do + let(:macro) { create(:macro) } + + before do + delete "/api/v1/macros/#{macro.id}", as: :json + end + + context 'when user is not allowed to destroy macro' do + let(:user) { create(:agent) } + + it 'does not destroy macro' do + expect(response).to have_http_status(:forbidden) + end + + it 'macro is not destroyed' do + expect(macro).not_to be_destroyed + end + end + + context 'when user is allowed to create macros' do + let(:user) { create(:admin) } + + it 'request is successful' do + expect(response).to have_http_status(:ok) + end + + it 'macro is destroyed' do + expect(Macro).not_to be_exist(macro.id) + end + end + end + + describe '#index' do + before do + get '/api/v1/macros', as: :json + end + + context 'when user is not allowed to use macros' do + let(:user) { create(:customer) } + + it 'returns exception' do + expect(response).to have_http_status(:forbidden) + end + end + + context 'when user is allowed to use macros' do + let(:user) { create(:agent) } + + it 'request is successful' do + expect(response).to have_http_status(:ok) + end + + it 'returns array of macros' do + expect(json_response.map { |elem| elem['id'] }).to eq [Macro.first.id] + end + end + end + + describe '#show' do + let(:macro) { create(:macro, groups: [create(:group)]) } + + before do + get "/api/v1/macros/#{macro.id}", as: :json + end + + context 'when user is not allowed to use macros' do + let(:user) { create(:customer) } + + it 'returns exception' do + expect(response).to have_http_status(:forbidden) + end + end + + context 'when user is allowed to use macros' do + let(:user) { create(:agent) } + + it 'returns exception when user has no access to related group' do + expect(response).to have_http_status(:not_found) + end + + context 'when user has acess to this group' do + let(:user) { create(:agent, groups: macro.groups) } + + it 'returns macro when user has access to related group' do + expect(response).to have_http_status(:ok) + end + end + end + end +end diff --git a/spec/requests/tickets_mass_spec.rb b/spec/requests/tickets_mass_spec.rb new file mode 100644 index 000000000..04495e708 --- /dev/null +++ b/spec/requests/tickets_mass_spec.rb @@ -0,0 +1,90 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +require 'rails_helper' + +RSpec.describe 'TicketsMass', type: :request, authenticated_as: :user do + let(:user) { create(:agent, groups: [group_a, group_b]) } + let(:owner) { create(:agent) } + + let(:group_a) { create(:group) } + let(:group_b) { create(:group) } + let(:group_c) { create(:group) } + + let(:ticket_a) { create(:ticket, group: group_a, owner: owner) } + let(:ticket_b) { create(:ticket, group: group_b, owner: owner) } + let(:ticket_c) { create(:ticket, group: group_c, owner: owner) } + + let(:core_workflow) do + create(:core_workflow, :active_and_screen, :condition_group, :perform_action, group: group_b) + end + + describe 'POST /tickets/mass_macro' do + let(:macro_perform) do + { + 'ticket.priority_id': { pre_condition: 'specific', value: 3.to_s } + } + end + let(:macro) { create :macro, perform: macro_perform } + let(:macro_groups) { create :macro, groups: [group_a], perform: macro_perform } + + it 'applies macro' do + post '/api/v1/tickets/mass_macro', params: { macro_id: macro.id, ticket_ids: [ticket_a.id] } + + expect(ticket_a.reload.priority_id).to eq 3 + end + + it 'does not apply changes if one of ticket updates fail' do + core_workflow + + post '/api/v1/tickets/mass_macro', params: { macro_id: macro.id, ticket_ids: [ticket_a.id, ticket_b.id] }, as: :json + + expect(ticket_a.reload.articles).not_to eq 3 + end + + it 'returns error if macro not applicable to at least one ticket' do + post '/api/v1/tickets/mass_macro', params: { macro_id: macro_groups.id, ticket_ids: [ticket_a.id, ticket_b.id] } + + expect(response).to have_http_status(:unprocessable_entity) + end + + it 'checks if user has write access to tickets' do + post '/api/v1/tickets/mass_macro', params: { macro_id: macro_groups.id, ticket_ids: [ticket_a.id, ticket_c.id] } + + expect(response).to have_http_status(:unprocessable_entity) + end + end + + describe 'POST /tickets/mass_update' do + it 'applies changes' do + post '/api/v1/tickets/mass_update', params: { attributes: { priority_id: 3 }, ticket_ids: [ticket_a.id] } + + expect(ticket_a.reload.priority_id).to eq 3 + end + + it 'does not apply changes' do + post '/api/v1/tickets/mass_update', params: { attributes: { priority_id: 3 }, ticket_ids: [ticket_c.id] } + + expect(ticket_c.reload.priority_id).not_to eq 3 + end + + it 'adds note' do + post '/api/v1/tickets/mass_update', params: { attributes: {}, article: { body: 'test mass update body' }, ticket_ids: [ticket_a.id] } + + expect(ticket_a.reload.articles.last).to have_attributes(body: 'test mass update body') + end + + it 'does not apply changes if one of ticket updates fail' do + core_workflow + + post '/api/v1/tickets/mass_update', params: { attributes: { priority_id: 3 }, ticket_ids: [ticket_a.id, ticket_b.id] } + + expect(ticket_a.reload.priority_id).not_to eq 3 + end + + it 'checks if user has write access to tickets' do + post '/api/v1/tickets/mass_update', params: { attributes: { priority_id: 3 }, ticket_ids: [ticket_a.id, ticket_c.id] } + + expect(response).to have_http_status(:unprocessable_entity) + end + end +end diff --git a/spec/system/ticket/view_spec.rb b/spec/system/ticket/view_spec.rb index 7a61cd2d4..164991da0 100644 --- a/spec/system/ticket/view_spec.rb +++ b/spec/system/ticket/view_spec.rb @@ -118,6 +118,54 @@ RSpec.describe 'Ticket views', type: :system do end end + context 'when saving is blocked by one of selected tickets', authenticated_as: :pre_authentication do + let(:core_workflow_action) { { 'ticket.priority_id': { operator: 'remove_option', remove_option: '3' } } } + let(:core_workflow) { create(:core_workflow, :active_and_screen, :perform_action) } + + let(:macro_perform) do + { + 'ticket.priority_id': { pre_condition: 'specific', value: 3.to_s } + } + end + + let(:macro_priority) { create :macro, perform: macro_perform } + let(:ticket1) { create :ticket, group: Group.first } + + def pre_authentication + core_workflow && macro_priority && ticket1 + + true + end + + it 'shows modal with blocking ticket title' do + visit '#ticket/view/all_open' + + within(:active_content) do + ticket_dom = page.find(:table_row, ticket1.id).native + + # click and hold first ticket in table + click_and_hold(ticket_dom) + + # move ticket to y -ticket.location.y + move_mouse_by(0, -ticket_dom.location.y + 5) + + # move a bit to the left to display macro batches + move_mouse_by(-250, 0) + + expect(page).to have_selector(:macro_batch, macro_priority.id, wait: 10) + + macro_dom = find(:macro_batch, macro_priority.id) + move_mouse_to(macro_dom) + + release_mouse + + in_modal disappears: false do + expect(page).to have_text(ticket1.title) + end + end + end + end + context 'with macro batch overlay' do shared_examples "adding 'small' class to macro element" do it 'adds a "small" class to the macro element' do @@ -298,6 +346,32 @@ RSpec.describe 'Ticket views', type: :system do end end end + + context 'when saving is blocked by one of selected tickets', authenticated_as: :pre_authentication do + let(:core_workflow) { create(:core_workflow, :active_and_screen, :perform_action) } + let(:ticket1) { create :ticket, group: Group.first } + + def pre_authentication + core_workflow && ticket1 + + true + end + + it 'shows modal with blocking ticket title' do + visit '#ticket/view/all_open' + + within(:active_content) do + find("tr[data-id='#{ticket1.id}']").check('bulk', allow_label_click: true) + select '3 high', from: 'priority_id' + click '.js-confirm' + click '.js-submit' + + in_modal disappears: false do + expect(page).to have_text(ticket1.title) + end + end + end + end end context 'Setting "ui_table_group_by_show_count"', authenticated_as: :authenticate, db_strategy: :reset do