From 093c9e7621d35bf2586f21ad5d6dcdc4c920cc5a Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Thu, 26 Jul 2018 16:24:31 +0200 Subject: [PATCH] Fixed #2132 by putting the I-doit objects in ticket preferences by creating the ticket. --- .gitlab-ci.yml | 15 +++ .../controllers/agent_ticket_create.coffee | 4 + .../agent_ticket_create/sidebar.coffee | 5 + .../app/controllers/ticket_zoom.coffee | 4 + .../controllers/ticket_zoom/sidebar.coffee | 5 + .../ticket_zoom/sidebar_idoit.coffee | 11 +- .../javascripts/app/models/ticket.coffee | 2 +- .../integration/idoit_controller.rb | 10 +- app/controllers/ticket_articles_controller.rb | 3 + app/controllers/tickets_controller.rb | 3 + .../application_model/can_cleanup_param.rb | 30 ++++- .../ticket_articles_controller_test.rb | 43 ++++++- test/controllers/tickets_controller_test.rb | 9 ++ test/integration/idoit_browser_test.rb | 111 ++++++++++++++++++ test/unit/model_test.rb | 47 ++++++++ 15 files changed, 285 insertions(+), 17 deletions(-) create mode 100644 test/integration/idoit_browser_test.rb diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index bfe5b71b6..2a12ef724 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1055,3 +1055,18 @@ test:browser:integration:zendesk_chrome: - script/build/test_startup.sh $RAILS_ENV $BROWSER_PORT $WS_PORT 1 - ruby -I test/ test/integration/zendesk_import_browser_test.rb || script/build/test_shutdown.sh $RAILS_ENV $BROWSER_PORT $WS_PORT 1 1 - script/build/test_shutdown.sh $RAILS_ENV $BROWSER_PORT $WS_PORT 0 1 + +test:browser:integration:idoit_chrome: + stage: browser-integration + dependencies: + - browser:build + tags: + - browser + script: + - export BROWSER=chrome + - export BROWSER_URL=http://$IP:$BROWSER_PORT + - RAILS_ENV=test rake db:create + - cp contrib/auto_wizard_test.json auto_wizard.json + - script/build/test_startup.sh $RAILS_ENV $BROWSER_PORT $WS_PORT 1 + - ruby -I test/ test/integration/idoit_browser_test.rb || script/build/test_shutdown.sh $RAILS_ENV $BROWSER_PORT $WS_PORT 1 1 + - script/build/test_shutdown.sh $RAILS_ENV $BROWSER_PORT $WS_PORT 0 1 diff --git a/app/assets/javascripts/app/controllers/agent_ticket_create.coffee b/app/assets/javascripts/app/controllers/agent_ticket_create.coffee index 5c02b5bb0..d16d113d1 100644 --- a/app/assets/javascripts/app/controllers/agent_ticket_create.coffee +++ b/app/assets/javascripts/app/controllers/agent_ticket_create.coffee @@ -508,6 +508,10 @@ class App.TicketCreate extends App.Controller if !confirm(App.i18n.translateContent('You use %s in text but no attachment is attached. Do you want to continue?', matchingWord)) return + # add sidebar params + if @sidebarWidget && @sidebarWidget.postParams + @sidebarWidget.postParams(ticket: ticket) + # disable form @submitDisable(e) ui = @ diff --git a/app/assets/javascripts/app/controllers/agent_ticket_create/sidebar.coffee b/app/assets/javascripts/app/controllers/agent_ticket_create/sidebar.coffee index 66f811ee3..550467ebe 100644 --- a/app/assets/javascripts/app/controllers/agent_ticket_create/sidebar.coffee +++ b/app/assets/javascripts/app/controllers/agent_ticket_create/sidebar.coffee @@ -13,6 +13,11 @@ class App.TicketCreateSidebar extends App.Controller if backend && backend.commit backend.commit(args) + postParams: (args) => + for key, backend of @sidebarBackends + if backend && backend.postParams + backend.postParams(args) + render: (params) => if params @params = params diff --git a/app/assets/javascripts/app/controllers/ticket_zoom.coffee b/app/assets/javascripts/app/controllers/ticket_zoom.coffee index 141e06537..a7f3057be 100644 --- a/app/assets/javascripts/app/controllers/ticket_zoom.coffee +++ b/app/assets/javascripts/app/controllers/ticket_zoom.coffee @@ -839,6 +839,10 @@ class App.TicketZoom extends App.Controller ticket.article = article + # add sidebar params + if @sidebarWidget && @sidebarWidget.postParams + @sidebarWidget.postParams(ticket: ticket) + if !ticket.article @submitPost(e, ticket, macro) return diff --git a/app/assets/javascripts/app/controllers/ticket_zoom/sidebar.coffee b/app/assets/javascripts/app/controllers/ticket_zoom/sidebar.coffee index 78491dbdb..4ebb1ab8d 100644 --- a/app/assets/javascripts/app/controllers/ticket_zoom/sidebar.coffee +++ b/app/assets/javascripts/app/controllers/ticket_zoom/sidebar.coffee @@ -14,6 +14,11 @@ class App.TicketZoomSidebar extends App.ObserverController if backend && backend.commit backend.commit(args) + postParams: (args) => + for key, backend of @sidebarBackends + if backend && backend.postParams + backend.postParams(args) + render: (ticket) => @sidebarBackends ||= {} @sidebarItems = [] diff --git a/app/assets/javascripts/app/controllers/ticket_zoom/sidebar_idoit.coffee b/app/assets/javascripts/app/controllers/ticket_zoom/sidebar_idoit.coffee index cdeb42e46..c23e9cc34 100644 --- a/app/assets/javascripts/app/controllers/ticket_zoom/sidebar_idoit.coffee +++ b/app/assets/javascripts/app/controllers/ticket_zoom/sidebar_idoit.coffee @@ -102,13 +102,14 @@ class SidebarIdoit extends App.Controller @updateTicket(@ticket.id, @objectIds) @showObjectsContent() - commit: (args) => - return if @ticket && @ticket.id + postParams: (args) => + return if !args.ticket + return if args.ticket.created_at return if !@objectIds return if _.isEmpty(@objectIds) - return if !args - return if !args.ticket_id - @updateTicket(args.ticket_id, @objectIds) + args.ticket.preferences ||= {} + args.ticket.preferences.idoit ||= {} + args.ticket.preferences.idoit.object_ids = @objectIds updateTicket: (ticket_id, objectIds, callback) => App.Ajax.request( diff --git a/app/assets/javascripts/app/models/ticket.coffee b/app/assets/javascripts/app/models/ticket.coffee index 90e56db84..3e979d694 100644 --- a/app/assets/javascripts/app/models/ticket.coffee +++ b/app/assets/javascripts/app/models/ticket.coffee @@ -1,5 +1,5 @@ class App.Ticket extends App.Model - @configure 'Ticket', 'number', 'title', 'group_id', 'owner_id', 'customer_id', 'state_id', 'priority_id', 'article', 'tags', 'links', 'updated_at' + @configure 'Ticket', 'number', 'title', 'group_id', 'owner_id', 'customer_id', 'state_id', 'priority_id', 'article', 'tags', 'links', 'updated_at', 'preferences' @extend Spine.Model.Ajax @url: @apiPath + '/tickets' @configure_attributes = [ diff --git a/app/controllers/integration/idoit_controller.rb b/app/controllers/integration/idoit_controller.rb index 7bfe3067a..6ca159113 100644 --- a/app/controllers/integration/idoit_controller.rb +++ b/app/controllers/integration/idoit_controller.rb @@ -38,10 +38,12 @@ class Integration::IdoitController < ApplicationController def update params[:object_ids] ||= [] ticket = Ticket.find(params[:ticket_id]) - access!(ticket, 'read') - ticket.preferences[:idoit] ||= {} - ticket.preferences[:idoit][:object_ids] = Array(params[:object_ids]).uniq - ticket.save! + ticket.with_lock do + access!(ticket, 'read') + ticket.preferences[:idoit] ||= {} + ticket.preferences[:idoit][:object_ids] = Array(params[:object_ids]).uniq + ticket.save! + end render json: { result: 'ok', diff --git a/app/controllers/ticket_articles_controller.rb b/app/controllers/ticket_articles_controller.rb index 2e1084023..110321e63 100644 --- a/app/controllers/ticket_articles_controller.rb +++ b/app/controllers/ticket_articles_controller.rb @@ -112,6 +112,9 @@ class TicketArticlesController < ApplicationController clean_params = Ticket::Article.association_name_to_id_convert(params) clean_params = Ticket::Article.param_cleanup(clean_params, true) + # only apply preferences changes (keep not updated keys/values) + clean_params = article.param_preferences_merge(clean_params) + article.update!(clean_params) if response_expand? diff --git a/app/controllers/tickets_controller.rb b/app/controllers/tickets_controller.rb index 354da67e9..5d10c945c 100644 --- a/app/controllers/tickets_controller.rb +++ b/app/controllers/tickets_controller.rb @@ -222,6 +222,9 @@ class TicketsController < ApplicationController clean_params = Ticket.association_name_to_id_convert(params) clean_params = Ticket.param_cleanup(clean_params, true) + # only apply preferences changes (keep not updated keys/values) + clean_params = ticket.param_preferences_merge(clean_params) + # overwrite params if !current_user.permissions?('ticket.agent') %i[owner owner_id customer customer_id organization organization_id preferences].each do |key| diff --git a/app/models/application_model/can_cleanup_param.rb b/app/models/application_model/can_cleanup_param.rb index f31efd4b9..e9ded1977 100644 --- a/app/models/application_model/can_cleanup_param.rb +++ b/app/models/application_model/can_cleanup_param.rb @@ -33,30 +33,30 @@ returns data = {} params.each do |key, value| - data[key.to_sym] = value + data[key.to_s] = value end # ignore id for new objects if new_object && params[:id] - data.delete(:id) + data.delete('id') end # only use object attributes - clean_params = {} + clean_params = ActiveSupport::HashWithIndifferentAccess.new new.attributes.each_key do |attribute| - next if !data.key?(attribute.to_sym) + next if !data.key?(attribute) # check reference records, referenced by _id attributes reflect_on_all_associations.map do |assoc| class_name = assoc.options[:class_name] next if !class_name - name = "#{assoc.name}_id".to_sym + name = "#{assoc.name}_id" next if !data.key?(name) next if data[name].blank? next if assoc.klass.lookup(id: data[name]) raise ArgumentError, "Invalid value for param '#{name}': #{data[name].inspect}" end - clean_params[attribute.to_sym] = data[attribute.to_sym] + clean_params[attribute] = data[attribute] end # we do want to set this via database @@ -89,5 +89,23 @@ returns end data end + + end + +=begin + +merge preferences param + + record = Model.find(123) + + new_preferences = record.param_preferences_merge(param_preferences) + +=end + + def param_preferences_merge(new_params) + return new_params if new_params.blank? + return new_params if preferences.blank? + new_params[:preferences] = preferences.merge(new_params[:preferences] || {}) + new_params end end diff --git a/test/controllers/ticket_articles_controller_test.rb b/test/controllers/ticket_articles_controller_test.rb index 73b355032..62b5ba792 100644 --- a/test/controllers/ticket_articles_controller_test.rb +++ b/test/controllers/ticket_articles_controller_test.rb @@ -152,6 +152,46 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO assert_equal('some_file.txt', result['attachments'][0]['filename']) assert_equal('8', result['attachments'][0]['size']) assert_equal('text/plain', result['attachments'][0]['preferences']['Mime-Type']) + + params = { + ticket_id: result['ticket_id'], + content_type: 'text/plain', + body: 'some body', + type: 'note', + preferences: { + some_key1: 123, + }, + } + post '/api/v1/ticket_articles', params: params.to_json, headers: @headers.merge('Authorization' => credentials) + assert_response(201) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_nil(result['subject']) + assert_equal('some body', result['body']) + assert_equal('text/plain', result['content_type']) + assert_equal(@agent.id, result['updated_by_id']) + assert_equal(@agent.id, result['created_by_id']) + assert_equal(123, result['preferences']['some_key1']) + assert_equal(5, ticket.articles.count) + + params = { + body: 'some body 2', + preferences: { + some_key2: 'abc', + }, + } + put "/api/v1/ticket_articles/#{result['id']}", params: params.to_json, headers: @headers.merge('Authorization' => credentials) + assert_response(200) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_nil(result['subject']) + assert_equal('some body 2', result['body']) + assert_equal('text/plain', result['content_type']) + assert_equal(@agent.id, result['updated_by_id']) + assert_equal(@agent.id, result['created_by_id']) + assert_equal(123, result['preferences']['some_key1']) + assert_equal('abc', result['preferences']['some_key2']) + end test '02.01 ticket create with customer and articles' do @@ -243,7 +283,7 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO assert_equal(0, ticket.articles[3].attachments.count) # add internal article - article = Ticket::Article.create( + article = Ticket::Article.create!( ticket_id: ticket.id, from: 'some_sender@example.com', to: 'some_recipient@example.com', @@ -297,6 +337,7 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO assert_response(201) result = JSON.parse(@response.body) assert_equal(Hash, result.class) + assert_equal('a new ticket #1', result['title']) article = Ticket::Article.find_by(ticket_id: result['id']) assert_equal(@customer_without_org.id, article.origin_by_id) diff --git a/test/controllers/tickets_controller_test.rb b/test/controllers/tickets_controller_test.rb index bbd13cd42..3e2f59689 100644 --- a/test/controllers/tickets_controller_test.rb +++ b/test/controllers/tickets_controller_test.rb @@ -767,6 +767,9 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO priority: Ticket::Priority.lookup(name: '2 normal'), updated_by_id: 1, created_by_id: 1, + preferences: { + some_key1: 123, + }, ) credentials = ActionController::HttpAuthentication::Basic.encode_credentials('tickets-agent@example.com', 'agentpw') get "/api/v1/tickets/#{ticket.id}", params: {}, headers: @headers.merge('Authorization' => credentials) @@ -778,10 +781,14 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO assert_equal(ticket.customer_id, result['customer_id']) assert_equal(1, result['updated_by_id']) assert_equal(1, result['created_by_id']) + assert_equal(123, result['preferences']['some_key1']) params = { title: "#{title} - 2", customer_id: @agent.id, + preferences: { + some_key2: 'abc', + }, } put "/api/v1/tickets/#{ticket.id}", params: params.to_json, headers: @headers.merge('Authorization' => credentials) assert_response(200) @@ -792,6 +799,8 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO assert_equal(@agent.id, result['customer_id']) assert_equal(@agent.id, result['updated_by_id']) assert_equal(1, result['created_by_id']) + assert_equal(123, result['preferences']['some_key1']) + assert_equal('abc', result['preferences']['some_key2']) params = { ticket_id: ticket.id, diff --git a/test/integration/idoit_browser_test.rb b/test/integration/idoit_browser_test.rb new file mode 100644 index 000000000..93a35a3bc --- /dev/null +++ b/test/integration/idoit_browser_test.rb @@ -0,0 +1,111 @@ + +require 'browser_test_helper' + +class IntegrationIdoitTest < TestCase + + def test_idoit_objects_corrects_saves_on_ticket_creation + # Read I-doit credentials from ENV + if !ENV['IDOIT_API_TOKEN'] + raise "ERROR: Need IDOIT_API_TOKEN - hint IDOIT_API_TOKEN='1234'" + end + api_token = ENV['IDOIT_API_TOKEN'] + if !ENV['IDOIT_API_ENDPOINT'] + raise "ERROR: Need IDOIT_API_ENDPOINT - hint IDOIT_API_ENDPOINT='1234'" + end + api_endpoint = ENV['IDOIT_API_ENDPOINT'] + if !ENV['IDOIT_API_CATEGORY'] + raise "ERROR: Need IDOIT_API_CATEGORY - hint IDOIT_API_CATEGORY='Building'" + end + api_category = ENV['IDOIT_API_CATEGORY'] + + id = rand(99_999_999) + @browser = instance = browser_instance + login( + username: 'master@example.com', + password: 'test', + url: browser_url, + ) + + # turn on I-doit integration + click(css: 'a[href="#manage"]') + click(css: 'a[href="#system/integration"]') + click(css: 'a[href="#system/integration/idoit"]') + switch( + css: '.content.active .js-switch', + type: 'on' + ) + + # fill in I-doit login details + set( + css: '.content.active .main input[name="api_token"]', + value: api_token, + ) + set( + css: '.content.active .main input[name="endpoint"]', + value: api_endpoint, + ) + click(css: '.content.active .main .js-submit') + + watch_for( + css: '#notify', + value: 'update successful', + ) + + # new create a new ticket with an I-doit object + ticket = ticket_create( + data: { + customer: 'nico', + group: 'Users', + title: 'subject - I-doit integration', + body: 'body - I-doit integration', + }, + do_not_submit: true, + ) + + # open the I-doit selection modal + click(css: '.content.active .tabsSidebar svg.icon-printer') + click(css: '.content.active .sidebar[data-tab="idoit"] .js-headline') + click(css: '.content.active .sidebar[data-tab="idoit"] .dropdown-menu') + + # wait for the API call to populate the dropdown menu + watch_for(css: '.content.active .modal form input.js-input') + # open the dropdown menu and choose the Building option + click(css: '.content.active .modal form input.js-input') + click(css: ".content.active .modal form li.js-option[title='#{api_category}']") + # wait for the building results to populate from the API + watch_for(css: '.content.active .modal form.js-result table.table') + + # click the check box from the first row and note its entry ID + checkbox = instance.find_elements(css: '.content.active .modal form.js-result tbody :first-child input')[0] + entry_id = checkbox.attribute('value') + checkbox.click() + + # submit the I-doit object selections + click(css: '.content.active .modal form button.js-submit') + + # confirm that the entry have been successfully recorded + watch_for( + css: ".content.active .sidebar[data-tab='idoit'] a[href='#{api_endpoint}/?objID=#{entry_id}']", + ) + + # now submit the ticket + click(css: '.content.active .newTicket button.js-submit') + sleep 5 + + # open the I-doit sidebar again and verify that the entry is still there + click(css: '.content.active .tabsSidebar svg.icon-printer') + watch_for( + css: ".content.active .sidebar[data-tab='idoit'] a[href='#{api_endpoint}/?objID=#{entry_id}']", + ) + + # finally turn off I-doit integration + click(css: 'a[href="#manage"]') + click(css: 'a[href="#system/integration"]') + click(css: 'a[href="#system/integration/idoit"]') + + switch( + css: '.content.active .js-switch', + type: 'off' + ) + end +end diff --git a/test/unit/model_test.rb b/test/unit/model_test.rb index b8483213d..6090e1a7d 100644 --- a/test/unit/model_test.rb +++ b/test/unit/model_test.rb @@ -337,4 +337,51 @@ class ModelTest < ActiveSupport::TestCase assert_not(result.key?(:controller)) end + test 'param_preferences_merge test' do + params = { + id: 123, + firstname: '123', + created_by_id: 1, + created_at: Time.zone.now, + updated_by_id: 1, + updated_at: Time.zone.now, + preferences: {}, + } + user = User.new(params) + assert(user.preferences.blank?) + + user.preferences = { A: 1, B: 2 } + assert(user.preferences.present?) + + params = { + firstname: '123 ABC', + preferences: { 'B' => 3, 'C': 4 }, + } + clean_params = User.param_cleanup(params) + clean_user_params = user.param_preferences_merge(clean_params) + assert_equal(clean_user_params[:firstname], '123 ABC') + assert(clean_user_params[:preferences].present?) + assert_equal(clean_user_params[:preferences]['A'], 1) + assert_equal(clean_user_params[:preferences]['B'], 3) + assert_equal(clean_user_params[:preferences]['C'], 4) + assert_equal(clean_user_params[:preferences][:A], 1) + assert_equal(clean_user_params[:preferences][:B], 3) + assert_equal(clean_user_params[:preferences][:C], 4) + + params = { + firstname: '123 ABCD', + preferences: {}, + } + clean_params = User.param_cleanup(params) + clean_user_params = user.param_preferences_merge(clean_params) + assert_equal(clean_user_params[:firstname], '123 ABCD') + assert(clean_user_params[:preferences].present?) + assert_equal(clean_user_params[:preferences]['A'], 1) + assert_equal(clean_user_params[:preferences]['B'], 2) + assert_nil(clean_user_params[:preferences]['C']) + assert_equal(clean_user_params[:preferences][:A], 1) + assert_equal(clean_user_params[:preferences][:B], 2) + assert_nil(clean_user_params[:preferences][:C]) + end + end