From 7599e8e17a5121e90f49d25a84ee8a2df02a4700 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Mon, 27 Nov 2017 11:50:57 +0100 Subject: [PATCH] Fixed issue #1681 - Unable to re-order overviews in admin interface with over 100 overviews. --- .../_application_controller_generic.coffee | 39 ++-- .../app/controllers/overview.coffee | 17 +- .../app/models/_application_model.coffee | 12 +- .../javascripts/app/models/overview.coffee | 2 +- app/controllers/overviews_controller.rb | 58 +++++- app/models/application_model/can_assets.rb | 14 +- app/models/overview.rb | 47 +++-- app/models/overview/assets.rb | 2 - config/routes/overview.rb | 1 + lib/fill_db.rb | 42 +++- test/controllers/overviews_controller_test.rb | 179 ++++++++++++++++++ test/unit/overview_test.rb | 156 ++++++++++++++- 12 files changed, 503 insertions(+), 66 deletions(-) create mode 100644 test/controllers/overviews_controller_test.rb diff --git a/app/assets/javascripts/app/controllers/_application_controller_generic.coffee b/app/assets/javascripts/app/controllers/_application_controller_generic.coffee index 20991117e..7fda56473 100644 --- a/app/assets/javascripts/app/controllers/_application_controller_generic.coffee +++ b/app/assets/javascripts/app/controllers/_application_controller_generic.coffee @@ -148,24 +148,26 @@ class App.ControllerGenericIndex extends App.Controller return item ) - # show description button, only if content exists - showDescription = false - if App[ @genericObject ].description && !_.isEmpty(objects) - showDescription = true + if !@table - @html App.view('generic/admin/index')( - head: @pageData.objects - notes: @pageData.notes - buttons: @pageData.buttons - menus: @pageData.menus - showDescription: showDescription - ) + # show description button, only if content exists + showDescription = false + if App[ @genericObject ].description && !_.isEmpty(objects) + showDescription = true - # show description in content if no no content exists - if _.isEmpty(objects) && App[ @genericObject ].description - description = marked(App[ @genericObject ].description) - @$('.table-overview').html(description) - return + @html App.view('generic/admin/index')( + head: @pageData.objects + notes: @pageData.notes + buttons: @pageData.buttons + menus: @pageData.menus + showDescription: showDescription + ) + + # show description in content if no no content exists + if _.isEmpty(objects) && App[ @genericObject ].description + description = marked(App[ @genericObject ].description) + @$('.table-overview').html(description) + return # append content table params = _.extend( @@ -184,7 +186,10 @@ class App.ControllerGenericIndex extends App.Controller }, @pageData.tableExtend ) - new App.ControllerTable(params) + if !@table + @table = new App.ControllerTable(params) + else + @table.update(objects: objects) edit: (id, e) => e.preventDefault() diff --git a/app/assets/javascripts/app/controllers/overview.coffee b/app/assets/javascripts/app/controllers/overview.coffee index 3a8fae430..377328750 100644 --- a/app/assets/javascripts/app/controllers/overview.coffee +++ b/app/assets/javascripts/app/controllers/overview.coffee @@ -23,17 +23,22 @@ class Index extends App.ControllerSubContent ] container: @el.closest('.content') large: true - dndCallback: => + dndCallback: (e, item) => items = @el.find('table > tbody > tr') - order = [] + prios = [] prio = 0 for item in items prio += 1 id = $(item).data('id') - overview = App.Overview.find(id) - if overview.prio isnt prio - overview.prio = prio - overview.save() + prios.push [id, prio] + + @ajax( + id: 'overview_prio' + type: 'POST' + url: "#{@apiPath}/overviews_prio" + processData: true + data: JSON.stringify(prios: prios) + ) ) App.Config.set('Overview', { prio: 2300, name: 'Overviews', parent: '#manage', target: '#manage/overviews', controller: Index, permission: ['admin.overview'] }, 'NavBarAdmin') diff --git a/app/assets/javascripts/app/models/_application_model.coffee b/app/assets/javascripts/app/models/_application_model.coffee index cf41a2c23..660af96df 100644 --- a/app/assets/javascripts/app/models/_application_model.coffee +++ b/app/assets/javascripts/app/models/_application_model.coffee @@ -387,15 +387,17 @@ set new attributes of model (remove already available attributes) => return if _.isEmpty(@SUBSCRIPTION_COLLECTION) App.Log.debug('Model', "server notify collection change #{@className}") - @fetchFull( - -> - clear: true - ) + callback = => + @fetchFull( + -> + clear: true + ) + App.Delay.set(callback, 200, "full-#{@className}") "Collection::Subscribe::#{@className}" ) - key = @className + '-' + Math.floor( Math.random() * 99999 ) + key = "#{@className}-#{Math.floor(Math.random() * 99999)}" @SUBSCRIPTION_COLLECTION[key] = callback # fetch init collection diff --git a/app/assets/javascripts/app/models/overview.coffee b/app/assets/javascripts/app/models/overview.coffee index 85581873c..d6ad48a3c 100644 --- a/app/assets/javascripts/app/models/overview.coffee +++ b/app/assets/javascripts/app/models/overview.coffee @@ -1,5 +1,5 @@ class App.Overview extends App.Model - @configure 'Overview', 'name', 'prio', 'condition', 'order', 'group_by', 'view', 'user_ids', 'organization_shared', 'role_ids', 'order', 'group_by', 'active', 'updated_at' + @configure 'Overview', 'name', 'prio', 'condition', 'order', 'group_by', 'view', 'user_ids', 'organization_shared', 'role_ids', 'active' @extend Spine.Model.Ajax @url: @apiPath + '/overviews' @configure_attributes = [ diff --git a/app/controllers/overviews_controller.rb b/app/controllers/overviews_controller.rb index 65deb2d0f..ee7d835ca 100644 --- a/app/controllers/overviews_controller.rb +++ b/app/controllers/overviews_controller.rb @@ -30,7 +30,7 @@ Example: =begin Resource: -GET /api/v1/overviews.json +GET /api/v1/overviews Response: [ @@ -47,7 +47,7 @@ Response: ] Test: -curl http://localhost/api/v1/overviews.json -v -u #{login}:#{password} +curl http://localhost/api/v1/overviews -v -u #{login}:#{password} =end @@ -58,7 +58,7 @@ curl http://localhost/api/v1/overviews.json -v -u #{login}:#{password} =begin Resource: -GET /api/v1/overviews/#{id}.json +GET /api/v1/overviews/#{id} Response: { @@ -68,7 +68,7 @@ Response: } Test: -curl http://localhost/api/v1/overviews/#{id}.json -v -u #{login}:#{password} +curl http://localhost/api/v1/overviews/#{id} -v -u #{login}:#{password} =end @@ -79,7 +79,7 @@ curl http://localhost/api/v1/overviews/#{id}.json -v -u #{login}:#{password} =begin Resource: -POST /api/v1/overviews.json +POST /api/v1/overviews Payload: { @@ -101,7 +101,7 @@ Response: } Test: -curl http://localhost/api/v1/overviews.json -v -u #{login}:#{password} -H "Content-Type: application/json" -X POST -d '{"name": "some_name","active": true, "note": "some note"}' +curl http://localhost/api/v1/overviews -v -u #{login}:#{password} -H "Content-Type: application/json" -X POST -d '{"name": "some_name","active": true, "note": "some note"}' =end @@ -112,7 +112,7 @@ curl http://localhost/api/v1/overviews.json -v -u #{login}:#{password} -H "Conte =begin Resource: -PUT /api/v1/overviews/{id}.json +PUT /api/v1/overviews/{id} Payload: { @@ -134,7 +134,7 @@ Response: } Test: -curl http://localhost/api/v1/overviews.json -v -u #{login}:#{password} -H "Content-Type: application/json" -X PUT -d '{"name": "some_name","active": true, "note": "some note"}' +curl http://localhost/api/v1/overviews -v -u #{login}:#{password} -H "Content-Type: application/json" -X PUT -d '{"name": "some_name","active": true, "note": "some note"}' =end @@ -145,17 +145,55 @@ curl http://localhost/api/v1/overviews.json -v -u #{login}:#{password} -H "Conte =begin Resource: -DELETE /api/v1/overviews/{id}.json +DELETE /api/v1/overviews/{id} Response: {} Test: -curl http://localhost/api/v1/overviews/#{id}.json -v -u #{login}:#{password} -H "Content-Type: application/json" -X DELETE +curl http://localhost/api/v1/overviews/#{id} -v -u #{login}:#{password} -H "Content-Type: application/json" -X DELETE =end def destroy model_destroy_render(Overview, params) end + +=begin + +Resource: +POST /api/v1/overviews_prio + +Payload: +{ + "prios": [ + [overview_id, prio], + [overview_id, prio], + [overview_id, prio], + [overview_id, prio], + [overview_id, prio] + ] +} + +Response: +{ + "success": true, +} + +Test: +curl http://localhost/api/v1/overviews_prio -v -u #{login}:#{password} -H "Content-Type: application/json" -X POST -d '{"prios": [ [1,1], [44,2] ]}' + +=end + + def prio + Overview.without_callback(:update, :before, :rearrangement) do + params[:prios].each do |overview_prio| + overview = Overview.find(overview_prio[0]) + next if overview.prio == overview_prio[1] + overview.prio = overview_prio[1] + overview.save! + end + end + render json: { success: true }, status: :ok + end end diff --git a/app/models/application_model/can_assets.rb b/app/models/application_model/can_assets.rb index 802e6afd4..42f2fee84 100644 --- a/app/models/application_model/can_assets.rb +++ b/app/models/application_model/can_assets.rb @@ -75,12 +75,12 @@ get assets and record_ids of selector attribute_ref_class = models[attribute_class][:reflections][reflection].klass if content['value'].instance_of?(Array) content['value'].each do |item_id| - attribute_object = attribute_ref_class.find_by(id: item_id) - if attribute_object - assets = attribute_object.assets(assets) - end + next if item_id.blank? + attribute_object = attribute_ref_class.lookup(id: item_id) + next if !attribute_object + assets = attribute_object.assets(assets) end - else + elsif content['value'].present? attribute_object = attribute_ref_class.find_by(id: content['value']) if attribute_object assets = attribute_object.assets(assets) @@ -138,11 +138,11 @@ get assets of object list require item['object'].to_filename record = Kernel.const_get(item['object']).find(item['o_id']) assets = record.assets(assets) - if item['created_by_id'] + if item['created_by_id'].present? user = User.find(item['created_by_id']) assets = user.assets(assets) end - if item['updated_by_id'] + if item['updated_by_id'].present? user = User.find(item['updated_by_id']) assets = user.assets(assets) end diff --git a/app/models/overview.rb b/app/models/overview.rb index c6e604323..3b7db3588 100644 --- a/app/models/overview.rb +++ b/app/models/overview.rb @@ -17,26 +17,47 @@ class Overview < ApplicationModel validates :name, presence: true before_create :fill_link_on_create, :fill_prio - before_update :fill_link_on_update + before_update :fill_link_on_update, :rearrangement private + def rearrangement + return true if !changes['prio'] + prio = 0 + Overview.all.order(prio: :asc, updated_at: :desc).pluck(:id).each do |overview_id| + prio += 1 + next if id == overview_id + Overview.without_callback(:update, :before, :rearrangement) do + overview = Overview.find(overview_id) + next if overview.prio == prio + overview.prio = prio + overview.save! + end + end + end + def fill_prio - return true if prio - self.prio = 9999 + return true if prio.present? + self.prio = Overview.count + 1 true end def fill_link_on_create - return true if link.present? - self.link = link_name(name) + self.link = if link.present? + link_name(link) + else + link_name(name) + end true end def fill_link_on_update - return true if !changes['name'] - return true if changes['link'] - self.link = link_name(name) + return true if !changes['name'] && !changes['link'] + self.link = if link.present? + link_name(link) + else + link_name(name) + end true end @@ -50,12 +71,16 @@ class Overview < ApplicationModel local_link = id || rand(999) end check = true + count = 0 + local_lookup_link = local_link while check - exists = Overview.find_by(link: local_link) - if exists&.id != id - local_link = "#{local_link}_#{rand(999)}" + count += 1 + exists = Overview.find_by(link: local_lookup_link) + if exists && exists.id != id # rubocop:disable Style/SafeNavigation + local_lookup_link = "#{local_link}_#{count}" else check = false + local_link = local_lookup_link end end local_link diff --git a/app/models/overview/assets.rb b/app/models/overview/assets.rb index 15f252a66..bccb2440e 100644 --- a/app/models/overview/assets.rb +++ b/app/models/overview/assets.rb @@ -40,9 +40,7 @@ returns next if !user data = user.assets(data) end - data = assets_of_selector('condition', data) - end %w[created_by_id updated_by_id].each do |local_user_id| next if !self[ local_user_id ] diff --git a/config/routes/overview.rb b/config/routes/overview.rb index 8b017738d..14038be5a 100644 --- a/config/routes/overview.rb +++ b/config/routes/overview.rb @@ -7,5 +7,6 @@ Zammad::Application.routes.draw do match api_path + '/overviews', to: 'overviews#create', via: :post match api_path + '/overviews/:id', to: 'overviews#update', via: :put match api_path + '/overviews/:id', to: 'overviews#destroy', via: :delete + match api_path + '/overviews_prio', to: 'overviews#prio', via: :post end diff --git a/lib/fill_db.rb b/lib/fill_db.rb index 1ceabdd4f..a02bd298f 100644 --- a/lib/fill_db.rb +++ b/lib/fill_db.rb @@ -10,7 +10,8 @@ fill your database with demo records customers: 1000, groups: 20, organizations: 40, - tickets: 100 + overviews: 5, + tickets: 100, ) or if you only want to create 100 tickets @@ -25,6 +26,7 @@ or if you only want to create 100 tickets customers = params[:customers] || 0 groups = params[:groups] || 0 organizations = params[:organizations] || 0 + overviews = params[:overviews] || 0 tickets = params[:tickets] || 0 puts 'load db with:' @@ -32,6 +34,7 @@ or if you only want to create 100 tickets puts " customers:#{customers}" puts " groups:#{groups}" puts " organizations:#{organizations}" + puts " overviews:#{overviews}" puts " tickets:#{tickets}" # set current user @@ -39,7 +42,7 @@ or if you only want to create 100 tickets # organizations organization_pool = [] - if organizations && !organizations.zero? + if !organizations.zero? (1..organizations).each do ActiveRecord::Base.transaction do organization = Organization.create!(name: "FillOrganization::#{rand(999_999)}", active: true) @@ -53,7 +56,7 @@ or if you only want to create 100 tickets # create agents agent_pool = [] - if agents && !agents.zero? + if !agents.zero? roles = Role.where(name: [ 'Agent']) groups_all = Group.all @@ -81,7 +84,7 @@ or if you only want to create 100 tickets # create customer customer_pool = [] - if customers && !customers.zero? + if !customers.zero? roles = Role.where(name: [ 'Customer']) groups_all = Group.all @@ -113,7 +116,7 @@ or if you only want to create 100 tickets # create groups group_pool = [] - if groups && !groups.zero? + if !groups.zero? (1..groups).each do ActiveRecord::Base.transaction do @@ -133,6 +136,35 @@ or if you only want to create 100 tickets puts " take #{group_pool.length} groups" end + # create overviews + if !overviews.zero? + (1..overviews).each do + ActiveRecord::Base.transaction do + overview = Overview.create!( + name: "Filloverview::#{rand(999_999)}", + role_ids: [Role.find_by(name: 'Agent').id], + condition: { + 'ticket.state_id' => { + operator: 'is', + value: Ticket::State.by_category(:work_on_all).pluck(:id), + }, + }, + order: { + by: 'created_at', + direction: 'ASC', + }, + view: { + d: %w[title customer group state owner created_at], + s: %w[title customer group state owner created_at], + m: %w[number title customer group state owner created_at], + view_mode_default: 's', + }, + active: true + ) + end + end + end + # create tickets priority_pool = Ticket::Priority.all state_pool = Ticket::State.all diff --git a/test/controllers/overviews_controller_test.rb b/test/controllers/overviews_controller_test.rb new file mode 100644 index 000000000..04cf81b1b --- /dev/null +++ b/test/controllers/overviews_controller_test.rb @@ -0,0 +1,179 @@ + +require 'test_helper' + +class OverviewsControllerTest < ActionDispatch::IntegrationTest + setup do + + # set accept header + @headers = { 'ACCEPT' => 'application/json', 'CONTENT_TYPE' => 'application/json' } + + # create agent + roles = Role.where(name: %w[Admin Agent]) + groups = Group.all + + UserInfo.current_user_id = 1 + @admin = User.create_or_update( + login: 'tickets-admin', + firstname: 'Tickets', + lastname: 'Admin', + email: 'tickets-admin@example.com', + password: 'adminpw', + active: true, + roles: roles, + groups: groups, + ) + + # create agent + roles = Role.where(name: 'Agent') + @agent = User.create_or_update( + login: 'tickets-agent@example.com', + firstname: 'Tickets', + lastname: 'Agent', + email: 'tickets-agent@example.com', + password: 'agentpw', + active: true, + roles: roles, + groups: Group.all, + ) + + end + + test 'no permissions' do + credentials = ActionController::HttpAuthentication::Basic.encode_credentials('tickets-agent', 'agentpw') + + params = { + name: 'Overview2', + link: 'my_overview', + condition: { + 'ticket.state_id' => { + operator: 'is', + value: [1, 2, 3], + }, + }, + order: { + by: 'created_at', + direction: 'DESC', + }, + view: { + d: %w[title customer state created_at], + s: %w[number title customer state created_at], + m: %w[number title customer state created_at], + view_mode_default: 's', + }, + } + + post '/api/v1/overviews', params: params.to_json, headers: @headers.merge('Authorization' => credentials) + assert_response(401) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal('authentication failed', result['error']) + end + + test 'create overviews' do + credentials = ActionController::HttpAuthentication::Basic.encode_credentials('tickets-admin', 'adminpw') + + params = { + name: 'Overview2', + link: 'my_overview', + condition: { + 'ticket.state_id' => { + operator: 'is', + value: [1, 2, 3], + }, + }, + order: { + by: 'created_at', + direction: 'DESC', + }, + view: { + d: %w[title customer state created_at], + s: %w[number title customer state created_at], + m: %w[number title customer state created_at], + view_mode_default: 's', + }, + } + + post '/api/v1/overviews', params: params.to_json, headers: @headers.merge('Authorization' => credentials) + assert_response(201) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal('Overview2', result['name']) + assert_equal('my_overview', result['link']) + + post '/api/v1/overviews', params: params.to_json, headers: @headers.merge('Authorization' => credentials) + assert_response(201) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal('Overview2', result['name']) + assert_equal('my_overview_1', result['link']) + end + + test 'set mass prio' do + overview1 = Overview.create!( + name: 'Overview1', + link: 'my_overview', + condition: { + 'ticket.state_id' => { + operator: 'is', + value: [1, 2, 3], + }, + }, + order: { + by: 'created_at', + direction: 'DESC', + }, + view: { + d: %w[title customer state created_at], + s: %w[number title customer state created_at], + m: %w[number title customer state created_at], + view_mode_default: 's', + }, + prio: 1, + updated_by_id: 1, + created_by_id: 1, + ) + overview2 = Overview.create!( + name: 'Overview2', + link: 'my_overview', + condition: { + 'ticket.state_id' => { + operator: 'is', + value: [1, 2, 3], + }, + }, + order: { + by: 'created_at', + direction: 'DESC', + }, + view: { + d: %w[title customer state created_at], + s: %w[number title customer state created_at], + m: %w[number title customer state created_at], + view_mode_default: 's', + }, + prio: 2, + updated_by_id: 1, + created_by_id: 1, + ) + + credentials = ActionController::HttpAuthentication::Basic.encode_credentials('tickets-admin', 'adminpw') + params = { + prios: [ + [overview2.id, 1], + [overview1.id, 2], + ] + } + post '/api/v1/overviews_prio', params: params.to_json, headers: @headers.merge('Authorization' => credentials) + assert_response(200) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal(true, result['success']) + + overview1.reload + overview2.reload + + assert_equal(2, overview1.prio) + assert_equal(1, overview2.prio) + end + +end diff --git a/test/unit/overview_test.rb b/test/unit/overview_test.rb index a70b2d381..01192ff56 100644 --- a/test/unit/overview_test.rb +++ b/test/unit/overview_test.rb @@ -28,7 +28,7 @@ class OverviewTest < ActiveSupport::TestCase overview.destroy! overview = Overview.create!( - name: 'My assigned Tickets', + name: 'My assigned Tickets 2', condition: { 'ticket.state_id' => { operator: 'is', @@ -46,7 +46,7 @@ class OverviewTest < ActiveSupport::TestCase view_mode_default: 's', }, ) - assert_equal(overview.link, 'my_assigned_tickets') + assert_equal(overview.link, 'my_assigned_tickets_2') overview.destroy! overview = Overview.create!( @@ -210,6 +210,158 @@ class OverviewTest < ActiveSupport::TestCase assert_equal(overview.link, 'my_overview2') overview.destroy! + end + test 'same url' do + UserInfo.current_user_id = 1 + + overview1 = Overview.create!( + name: 'My own assigned Tickets', + condition: { + 'ticket.state_id' => { + operator: 'is', + value: [1, 2, 3], + }, + }, + order: { + by: 'created_at', + direction: 'DESC', + }, + view: { + d: %w[title customer state created_at], + s: %w[number title customer state created_at], + m: %w[number title customer state created_at], + view_mode_default: 's', + }, + ) + assert_equal(overview1.link, 'my_own_assigned_tickets') + + overview2 = Overview.create!( + name: 'My own assigned Tickets', + condition: { + 'ticket.state_id' => { + operator: 'is', + value: [1, 2, 3], + }, + }, + order: { + by: 'created_at', + direction: 'DESC', + }, + view: { + d: %w[title customer state created_at], + s: %w[number title customer state created_at], + m: %w[number title customer state created_at], + view_mode_default: 's', + }, + ) + assert_equal(overview2.link, 'my_own_assigned_tickets_1') + + overview3 = Overview.create!( + name: 'My own assigned Tickets', + condition: { + 'ticket.state_id' => { + operator: 'is', + value: [1, 2, 3], + }, + }, + order: { + by: 'created_at', + direction: 'DESC', + }, + view: { + d: %w[title customer state created_at], + s: %w[number title customer state created_at], + m: %w[number title customer state created_at], + view_mode_default: 's', + }, + ) + assert_equal(overview3.link, 'my_own_assigned_tickets_2') + + overview1.destroy! + overview2.destroy! + overview3.destroy! + end + + test 'priority rearrangement' do + UserInfo.current_user_id = 1 + + overview1 = Overview.create!( + name: 'Overview1', + link: 'my_overview', + condition: { + 'ticket.state_id' => { + operator: 'is', + value: [1, 2, 3], + }, + }, + order: { + by: 'created_at', + direction: 'DESC', + }, + view: { + d: %w[title customer state created_at], + s: %w[number title customer state created_at], + m: %w[number title customer state created_at], + view_mode_default: 's', + }, + prio: 1, + ) + + overview2 = Overview.create!( + name: 'Overview2', + link: 'my_overview', + condition: { + 'ticket.state_id' => { + operator: 'is', + value: [1, 2, 3], + }, + }, + order: { + by: 'created_at', + direction: 'DESC', + }, + view: { + d: %w[title customer state created_at], + s: %w[number title customer state created_at], + m: %w[number title customer state created_at], + view_mode_default: 's', + }, + prio: 2, + ) + + overview3 = Overview.create!( + name: 'Overview3', + link: 'my_overview', + condition: { + 'ticket.state_id' => { + operator: 'is', + value: [1, 2, 3], + }, + }, + order: { + by: 'created_at', + direction: 'DESC', + }, + view: { + d: %w[title customer state created_at], + s: %w[number title customer state created_at], + m: %w[number title customer state created_at], + view_mode_default: 's', + }, + prio: 3, + ) + + overview2.prio = 3 + overview2.save! + + overviews = Overview.all.order(prio: :asc).pluck(:id) + assert_equal(overview1.id, overviews[0]) + assert_equal(overview3.id, overviews[1]) + assert_equal(overview2.id, overviews[2]) + + overview1.destroy! + overview2.destroy! + overview3.destroy! end end