From db20bb5fc9530ee61fbdcfeba168cb59b39fad81 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Wed, 8 Jun 2016 13:43:07 +0200 Subject: [PATCH] Improved tag management + added admin tests. --- app/controllers/tags_controller.rb | 33 +--- app/models/observer/tag/ticket_history.rb | 2 + app/models/tag.rb | 218 +++++++++++++++------- test/unit/tag_test.rb | 198 ++++++++++++++++++++ 4 files changed, 356 insertions(+), 95 deletions(-) diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 3e62ac7e9..666c8b70c 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -35,7 +35,7 @@ class TagsController < ApplicationController success = Tag.tag_add( object: params[:object], o_id: params[:o_id], - item: params[:item].strip, + item: params[:item], ) if success render json: success, status: :created @@ -49,7 +49,7 @@ class TagsController < ApplicationController success = Tag.tag_remove( object: params[:object], o_id: params[:o_id], - item: params[:item].strip, + item: params[:item], ) if success render json: success, status: :created @@ -76,41 +76,24 @@ class TagsController < ApplicationController # POST /api/v1/tag_list def admin_create return if deny_if_not_role('Admin') - name = params[:name].strip - if !Tag.tag_item_lookup(name) - Tag::Item.create(name: name) - end + Tag::Item.lookup_by_name_and_create(params[:name]) render json: {} end # PUT /api/v1/tag_list/:id def admin_rename return if deny_if_not_role('Admin') - name = params[:name].strip - tag_item = Tag::Item.find(params[:id]) - existing_tag_id = Tag.tag_item_lookup(name) - if existing_tag_id - - # assign to already existing tag - Tag.where(tag_item_id: tag_item.id).update_all(tag_item_id: existing_tag_id) - - # delete not longer used tag - tag_item.destroy - - # update new tag name - else - tag_item.name = name - tag_item.save - end + Tag::Item.rename( + id: params[:id], + name: params[:name], + ) render json: {} end # DELETE /api/v1/tag_list/:id def admin_delete return if deny_if_not_role('Admin') - tag_item = Tag::Item.find(params[:id]) - Tag.where(tag_item_id: tag_item.id).destroy_all - tag_item.destroy + Tag::Item.remove(params[:id]) render json: {} end diff --git a/app/models/observer/tag/ticket_history.rb b/app/models/observer/tag/ticket_history.rb index b9ba9470f..20be28751 100644 --- a/app/models/observer/tag/ticket_history.rb +++ b/app/models/observer/tag/ticket_history.rb @@ -17,6 +17,7 @@ class Observer::Tag::TicketHistory < ActiveRecord::Observer history_object: 'Ticket', history_attribute: 'tag', value_to: record.tag_item.name, + created_by_id: record.created_by_id, ) end @@ -32,6 +33,7 @@ class Observer::Tag::TicketHistory < ActiveRecord::Observer history_object: 'Ticket', history_attribute: 'tag', value_to: record.tag_item.name, + created_by_id: record.created_by_id, ) end end diff --git a/app/models/tag.rb b/app/models/tag.rb index 6f5d682ca..e398c1a90 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -4,11 +4,6 @@ class Tag < ApplicationModel belongs_to :tag_object, class_name: 'Tag::Object' belongs_to :tag_item, class_name: 'Tag::Item' - # rubocop:disable Style/ClassVars - @@cache_item = {} - @@cache_object = {} -# rubocop:enable Style/ClassVars - =begin add tags for certain object @@ -23,18 +18,19 @@ add tags for certain object =end def self.tag_add(data) + data[:item].strip! # lookups if data[:object] - tag_object_id = tag_object_lookup(data[:object]) + tag_object_id = Tag::Object.lookup_by_name_and_create(data[:object]).id end if data[:item] - tag_item_id = tag_item_lookup(data[:item].strip) + tag_item_id = Tag::Item.lookup_by_name_and_create(data[:item]).id end - # return in duplicate + # return if duplicate current_tags = tag_list(data) - return true if current_tags.include?(data[:item].strip) + return true if current_tags.include?(data[:item]) # create history Tag.create( @@ -53,29 +49,41 @@ add tags for certain object remove tags of certain object - Tag.tag_add( + Tag.tag_remove( object: 'Ticket', o_id: ticket.id, item: 'some tag', created_by_id: current_user.id, ) +or by ids + + Tag.tag_remove( + tag_object_id: 123, + o_id: ticket.id, + tag_item_id: 123, + created_by_id: current_user.id, + ) + =end def self.tag_remove(data) # lookups if data[:object] - tag_object_id = tag_object_lookup(data[:object]) + data[:tag_object_id] = Tag::Object.lookup_by_name_and_create(data[:object]).id + else + data[:object] = Tag::Object.lookup(id: data[:tag_object_id]).name end if data[:item] - tag_item_id = tag_item_lookup(data[:item].strip) + data[:item].strip! + data[:tag_item_id] = Tag::Item.lookup_by_name_and_create(data[:item]).id end # create history result = Tag.where( - tag_object_id: tag_object_id, - tag_item_id: tag_item_id, + tag_object_id: data[:tag_object_id], + tag_item_id: data[:tag_item_id], o_id: data[:o_id], ) result.each(&:destroy) @@ -92,8 +100,6 @@ tag list for certain object tags = Tag.tag_list( object: 'Ticket', o_id: ticket.id, - item: 'some tag', - created_by_id: current_user.id, ) returns @@ -103,83 +109,155 @@ returns =end def self.tag_list(data) - tag_object_id_requested = tag_object_lookup(data[:object]) + tag_object_id_requested = Tag::Object.lookup(name: data[:object]) + return [] if !tag_object_id_requested tag_search = Tag.where( tag_object_id: tag_object_id_requested, o_id: data[:o_id], ) tags = [] tag_search.each {|tag| - tags.push tag_item_lookup_id(tag.tag_item_id) + tag_item = Tag::Item.lookup(id: tag.tag_item_id) + next if !tag_item + tags.push tag_item.name } tags end - def self.tag_item_lookup_id(id) + class Object < ApplicationModel + validates :name, presence: true - # use cache - return @@cache_item[id] if @@cache_item[id] +=begin - # lookup - tag_item = Tag::Item.find(id) - @@cache_item[id] = tag_item.name - tag_item.name - end +lookup by name and create tag item - def self.tag_item_lookup(name) + tag_object = Tag::Object.lookup_by_name_and_create('some tag') - # use cache - return @@cache_item[name] if @@cache_item[name] +=end - # lookup - tag_items = Tag::Item.where(name: name) - tag_items.each {|tag_item| - next if tag_item.name != name - @@cache_item[name] = tag_item.id - return tag_item.id - } + def self.lookup_by_name_and_create(name) + name.strip! - # create - tag_item = Tag::Item.create(name: name) - @@cache_item[name] = tag_item.id - tag_item.id - end + tag_object = Tag::Object.lookup(name: name) + return tag_object if tag_object - def self.tag_object_lookup_id(id) - - # use cache - return @@cache_object[id] if @@cache_object[id] - - # lookup - tag_object = Tag::Object.find(id) - @@cache_object[id] = tag_object.name - tag_object.name - end - - def self.tag_object_lookup(name) - - # use cache - return @@cache_object[name] if @@cache_object[name] - - # lookup - tag_object = Tag::Object.find_by(name: name) - if tag_object - @@cache_object[name] = tag_object.id - return tag_object.id + Tag::Object.create(name: name) end - # create - tag_object = Tag::Object.create(name: name) - @@cache_object[name] = tag_object.id - tag_object.id end - class Object < ActiveRecord::Base - end - - class Item < ActiveRecord::Base + class Item < ApplicationModel + validates :name, presence: true before_save :fill_namedowncase +=begin + +lookup by name and create tag item + + tag_item = Tag::Item.lookup_by_name_and_create('some tag') + +=end + + def self.lookup_by_name_and_create(name) + name.strip! + + tag_item = Tag::Item.lookup(name: name) + return tag_item if tag_item + + Tag::Item.create(name: name) + end + +=begin + +rename tag items + + Tag::Item.rename( + id: existing_tag_item_to_rename, + name: 'new tag item name', + updated_by_id: current_user.id, + ) + +=end + + def self.rename(data) + + new_tag_name = data[:name].strip + old_tag_item = Tag::Item.find(data[:id]) + already_existing_tag = Tag::Item.lookup(name: new_tag_name) + + # merge old with new tag if already existing + if already_existing_tag + + # re-assign old tag to already existing tag + Tag.where(tag_item_id: old_tag_item.id).each {|tag| + + # check if tag already exists on object + if Tag.find_by(tag_object_id: tag.tag_object_id, o_id: tag.o_id, tag_item_id: already_existing_tag.id) + Tag.tag_remove( + tag_object_id: tag.tag_object_id, + o_id: tag.o_id, + tag_item_id: old_tag_item.id, + ) + next + end + + # re-assign + tag_object = Tag::Object.lookup(id: tag.tag_object_id) + tag.tag_item_id = already_existing_tag.id + tag.save + + # touch reference objects + Tag.touch_reference_by_params( + object: tag_object.name, + o_id: tag.o_id, + ) + } + + # delete not longer used tag + old_tag_item.destroy + + # update new tag name + else + + old_tag_item.name = new_tag_name + old_tag_item.save + + # touch reference objects + Tag.where(tag_item_id: old_tag_item.id).each {|tag| + tag_object = Tag::Object.lookup(id: tag.tag_object_id) + Tag.touch_reference_by_params( + object: tag_object.name, + o_id: tag.o_id, + ) + } + end + + true + end + +=begin + +remove tag item (destroy with reverences) + + Tag::Item.remove(id) + +=end + + def self.remove(id) + + # search for references, destroy and touch + Tag.where(tag_item_id: id).each {|tag| + tag_object = Tag::Object.lookup(id: tag.tag_object_id) + tag.destroy + Tag.touch_reference_by_params( + object: tag_object.name, + o_id: tag.o_id, + ) + } + Tag::Item.find(id).destroy + true + end + def fill_namedowncase self.name_downcase = name.downcase end diff --git a/test/unit/tag_test.rb b/test/unit/tag_test.rb index dcb4b2e11..07e72197d 100644 --- a/test/unit/tag_test.rb +++ b/test/unit/tag_test.rb @@ -163,4 +163,202 @@ class TagTest < ActiveSupport::TestCase assert(!list.include?(tags[:item]), 'Tag entry destroyed') } end + + test 'tags - real live' do + + ticket1 = Ticket.create( + title: 'some title tag1', + group: Group.lookup(name: 'Users'), + customer_id: 2, + state: Ticket::State.lookup(name: 'new'), + priority: Ticket::Priority.lookup(name: '2 normal'), + updated_by_id: 1, + created_by_id: 1, + ) + ticket2 = Ticket.create( + title: 'some title tag2', + group: Group.lookup(name: 'Users'), + customer_id: 2, + state: Ticket::State.lookup(name: 'new'), + priority: Ticket::Priority.lookup(name: '2 normal'), + updated_by_id: 1, + created_by_id: 1, + ) + sleep 2 + Tag.tag_add( + object: 'Ticket', + o_id: ticket1.id, + item: 'some tag1', + created_by_id: 1, + ) + Tag.tag_add( + object: 'Ticket', + o_id: ticket1.id, + item: 'some tag2 ', + created_by_id: 1, + ) + Tag.tag_add( + object: 'Ticket', + o_id: ticket1.id, + item: ' some tag3', + created_by_id: 1, + ) + Tag.tag_add( + object: 'Ticket', + o_id: ticket1.id, + item: 'some TAG4', + created_by_id: 1, + ) + Tag.tag_add( + object: 'Ticket', + o_id: ticket1.id, + item: ' some tag4', + created_by_id: 1, + ) + Tag.tag_add( + object: 'Ticket', + o_id: ticket2.id, + item: 'some tag3', + created_by_id: 1, + ) + Tag.tag_add( + object: 'Ticket', + o_id: ticket2.id, + item: 'some TAG4', + created_by_id: 1, + ) + Tag.tag_add( + object: 'Ticket', + o_id: ticket2.id, + item: 'some tag4 ', + created_by_id: 1, + ) + + Tag.tag_remove( + object: 'Ticket', + o_id: ticket1.id, + item: 'some tag1', + created_by_id: 1, + ) + + ticket1_lookup1 = Ticket.lookup(id: ticket1.id) + assert_not_equal(ticket1.updated_at.to_s, ticket1_lookup1.updated_at.to_s) + ticket2_lookup1 = Ticket.lookup(id: ticket2.id) + assert_not_equal(ticket2.updated_at.to_s, ticket2_lookup1.updated_at.to_s) + + tags_ticket1 = Tag.tag_list( + object: 'Ticket', + o_id: ticket1.id, + ) + assert_equal(4, tags_ticket1.count) + assert(tags_ticket1.include?('some tag2')) + assert(tags_ticket1.include?('some tag3')) + assert(tags_ticket1.include?('some TAG4')) + assert(tags_ticket1.include?('some tag4')) + + tags_ticket2 = Tag.tag_list( + object: 'Ticket', + o_id: ticket2.id, + ) + assert_equal(3, tags_ticket2.count) + assert(tags_ticket2.include?('some tag3')) + assert(tags_ticket2.include?('some TAG4')) + assert(tags_ticket2.include?('some tag4')) + + # rename tag + sleep 2 + tag_item3 = Tag::Item.find_by(name: 'some tag3') + Tag::Item.rename( + id: tag_item3.id, + name: ' some tag33', + created_by_id: 1, + ) + + ticket1_lookup2 = Ticket.lookup(id: ticket1.id) + assert_not_equal(ticket1_lookup2.updated_at.to_s, ticket1_lookup1.updated_at.to_s) + ticket2_lookup2 = Ticket.lookup(id: ticket2.id) + assert_not_equal(ticket2_lookup2.updated_at.to_s, ticket2_lookup1.updated_at.to_s) + + tags_ticket1 = Tag.tag_list( + object: 'Ticket', + o_id: ticket1.id, + ) + + assert_equal(4, tags_ticket1.count) + assert(tags_ticket1.include?('some tag2')) + assert(tags_ticket1.include?('some tag33')) + assert(tags_ticket1.include?('some TAG4')) + assert(tags_ticket1.include?('some tag4')) + + tags_ticket2 = Tag.tag_list( + object: 'Ticket', + o_id: ticket2.id, + ) + assert_equal(3, tags_ticket2.count) + assert(tags_ticket2.include?('some tag33')) + assert(tags_ticket2.include?('some TAG4')) + assert(tags_ticket2.include?('some tag4')) + + # merge tags + sleep 2 + Tag::Item.rename( + id: tag_item3.id, + name: 'some tag2', + created_by_id: 1, + ) + + ticket1_lookup3 = Ticket.lookup(id: ticket1.id) + assert_not_equal(ticket1_lookup3.updated_at.to_s, ticket1_lookup2.updated_at.to_s) + ticket2_lookup3 = Ticket.lookup(id: ticket2.id) + assert_not_equal(ticket2_lookup3.updated_at.to_s, ticket2_lookup2.updated_at.to_s) + + tags_ticket1 = Tag.tag_list( + object: 'Ticket', + o_id: ticket1.id, + ) + assert_equal(3, tags_ticket1.count) + assert(tags_ticket1.include?('some tag2')) + assert(tags_ticket1.include?('some TAG4')) + assert(tags_ticket1.include?('some tag4')) + + tags_ticket2 = Tag.tag_list( + object: 'Ticket', + o_id: ticket2.id, + ) + assert_equal(3, tags_ticket2.count) + assert(tags_ticket2.include?('some tag2')) + assert(tags_ticket2.include?('some TAG4')) + assert(tags_ticket2.include?('some tag4')) + + assert_not(Tag::Item.find_by(id: tag_item3.id)) + + # remove tag item + sleep 2 + tag_item4 = Tag::Item.find_by(name: 'some TAG4') + Tag::Item.remove(tag_item4.id) + + tags_ticket1 = Tag.tag_list( + object: 'Ticket', + o_id: ticket1.id, + ) + assert_equal(2, tags_ticket1.count) + assert(tags_ticket1.include?('some tag2')) + assert(tags_ticket1.include?('some tag4')) + + tags_ticket2 = Tag.tag_list( + object: 'Ticket', + o_id: ticket2.id, + ) + assert_equal(2, tags_ticket2.count) + assert(tags_ticket2.include?('some tag2')) + assert(tags_ticket2.include?('some tag4')) + + assert_not(Tag::Item.find_by(id: tag_item4.id)) + + ticket1_lookup4 = Ticket.lookup(id: ticket1.id) + assert_not_equal(ticket1_lookup4.updated_at.to_s, ticket1_lookup3.updated_at.to_s) + ticket2_lookup4 = Ticket.lookup(id: ticket2.id) + assert_not_equal(ticket2_lookup4.updated_at.to_s, ticket2_lookup3.updated_at.to_s) + + end end