From 5852dc6d8cbbb471961da3585cc6417f76657ca5 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Sun, 13 Dec 2015 13:08:58 +0100 Subject: [PATCH] Code cleanup. Reworked for current twitter result structure. Improved timing. --- Gemfile | 2 - app/models/application_model.rb | 118 ++++++++++++------------ app/models/channel/driver/twitter.rb | 20 ++--- lib/tweet.rb | 129 +++++++++++++-------------- test/integration/twitter_test.rb | 41 ++++++--- 5 files changed, 160 insertions(+), 150 deletions(-) diff --git a/Gemfile b/Gemfile index d877dac1f..798f85deb 100644 --- a/Gemfile +++ b/Gemfile @@ -14,7 +14,6 @@ gem 'json' # Gems used only for assets and not required # in production environments by default. group :assets do - gem 'sqlite3' gem 'sass-rails' #, github: 'rails/sass-rails' gem 'coffee-rails' gem 'coffee-script-source' @@ -53,7 +52,6 @@ gem 'therubyracer' # e. g. for mysql you need to load mysql gem 'mysql2', '~> 0.3.20' -#gem 'sqlite3' gem 'net-ldap' diff --git a/app/models/application_model.rb b/app/models/application_model.rb index 0dfaf675a..682566de2 100644 --- a/app/models/application_model.rb +++ b/app/models/application_model.rb @@ -109,7 +109,7 @@ returns self.class.reflect_on_all_associations.map { |assoc| real_key = assoc.name.to_s[0, assoc.name.to_s.length - 1] + '_ids' - next if !params.key?( real_key.to_sym ) + next if !params.key?(real_key.to_sym) list_of_items = params[ real_key.to_sym ] if params[ real_key.to_sym ].class != Array @@ -117,9 +117,9 @@ returns end list = [] list_of_items.each {|item| - list.push( assoc.klass.find(item) ) + list.push(assoc.klass.find(item)) } - send( assoc.name.to_s + '=', list ) + send(assoc.name.to_s + '=', list) } end @@ -142,8 +142,8 @@ returns attributes = self.attributes self.class.reflect_on_all_associations.map { |assoc| real_key = assoc.name.to_s[0, assoc.name.to_s.length - 1] + '_ids' - if self.respond_to?( real_key ) - attributes[ real_key ] = send( real_key ) + if self.respond_to?(real_key) + attributes[ real_key ] = send(real_key) end } attributes @@ -164,10 +164,10 @@ returns def self.param_validation(data) # we do want to set this via database - data.delete( :updated_at ) - data.delete( :created_at ) - data.delete( :updated_by_id ) - data.delete( :created_by_id ) + data.delete(:updated_at) + data.delete(:created_at) + data.delete(:updated_by_id) + data.delete(:created_by_id) if data.respond_to?('permit!') data.permit! end @@ -245,7 +245,7 @@ returns if changes.key?('name') name = changes['name'][0] key = "#{self.class}::#{name}" - Cache.delete( key.to_s ) + Cache.delete(key.to_s) end if changes.key?('login') name = changes['login'][0] @@ -317,34 +317,34 @@ returns def self.lookup(data) if data[:id] - cache = cache_get( data[:id] ) + cache = cache_get(data[:id]) return cache if cache - record = find_by( id: data[:id] ) - cache_set( data[:id], record ) + record = find_by(id: data[:id]) + cache_set(data[:id], record) return record elsif data[:name] - cache = cache_get( data[:name] ) + cache = cache_get(data[:name]) return cache if cache # do lookup with == to handle case insensitive databases - records = where( name: data[:name] ) + records = where(name: data[:name]) records.each {|loop_record| if loop_record.name == data[:name] - cache_set( data[:name], loop_record ) + cache_set(data[:name], loop_record) return loop_record end } return elsif data[:login] - cache = cache_get( data[:login] ) + cache = cache_get(data[:login]) return cache if cache # do lookup with == to handle case insensitive databases - records = where( login: data[:login] ) + records = where(login: data[:login]) records.each {|loop_record| if loop_record.login == data[:login] - cache_set( data[:login], loop_record ) + cache_set( data[:login], loop_record) return loop_record end } @@ -358,7 +358,7 @@ returns create model if not exists (check exists based on id, name, login, email or locale) - result = Model.create_if_not_exists( attributes ) + result = Model.create_if_not_exists(attributes) returns @@ -368,33 +368,33 @@ returns def self.create_if_not_exists(data) if data[:id] - record = find_by( id: data[:id] ) + record = find_by(id: data[:id]) return record if record elsif data[:name] # do lookup with == to handle case insensitive databases - records = where( name: data[:name] ) + records = where(name: data[:name]) records.each {|loop_record| return loop_record if loop_record.name == data[:name] } elsif data[:login] # do lookup with == to handle case insensitive databases - records = where( login: data[:login] ) + records = where(login: data[:login]) records.each {|loop_record| return loop_record if loop_record.login == data[:login] } elsif data[:email] # do lookup with == to handle case insensitive databases - records = where( email: data[:email] ) + records = where(email: data[:email]) records.each {|loop_record| return loop_record if loop_record.email == data[:email] } elsif data[:locale] && data[:source] # do lookup with == to handle case insensitive databases - records = where( locale: data[:locale], source: data[:source] ) + records = where(locale: data[:locale], source: data[:source]) records.each {|loop_record| return loop_record if loop_record.source == data[:source] } @@ -406,7 +406,7 @@ returns create or update model (check exists based on id, name, login, email or locale) - result = Model.create_or_update( attributes ) + result = Model.create_or_update(attributes) returns @@ -416,64 +416,64 @@ returns def self.create_or_update(data) if data[:id] - record = find_by( id: data[:id] ) + record = find_by(id: data[:id]) if record - record.update_attributes( data ) + record.update_attributes(data) return record end - record = new( data ) + record = new(data) record.save return record elsif data[:name] # do lookup with == to handle case insensitive databases - records = where( name: data[:name] ) + records = where(name: data[:name]) records.each {|loop_record| if loop_record.name == data[:name] - loop_record.update_attributes( data ) + loop_record.update_attributes(data) return loop_record end } - record = new( data ) + record = new(data) record.save return record elsif data[:login] # do lookup with == to handle case insensitive databases - records = where( login: data[:login] ) + records = where(login: data[:login]) records.each {|loop_record| if loop_record.login.downcase == data[:login].downcase - loop_record.update_attributes( data ) + loop_record.update_attributes(data) return loop_record end } - record = new( data ) + record = new(data) record.save return record elsif data[:email] # do lookup with == to handle case insensitive databases - records = where( email: data[:email] ) + records = where(email: data[:email]) records.each {|loop_record| if loop_record.email.downcase == data[:email].downcase - loop_record.update_attributes( data ) + loop_record.update_attributes(data) return loop_record end } - record = new( data ) + record = new(data) record.save return record elsif data[:locale] # do lookup with == to handle case insensitive databases - records = where( locale: data[:locale] ) + records = where(locale: data[:locale]) records.each {|loop_record| if loop_record.locale.downcase == data[:locale].downcase - loop_record.update_attributes( data ) + loop_record.update_attributes(data) return loop_record end } - record = new( data ) + record = new(data) record.save return record else @@ -511,9 +511,9 @@ end expires_in = 31_536_000 # 1 year if updated_at.nil? - Cache.delete( key ) + Cache.delete(key) else - Cache.write( key, updated_at, { expires_in: expires_in } ) + Cache.write(key, updated_at, { expires_in: expires_in }) end end @@ -555,10 +555,10 @@ end =end def self.notify_clients_support - after_create :notify_clients_after_create - after_update :notify_clients_after_update - after_touch :notify_clients_after_touch - after_destroy :notify_clients_after_destroy + after_create :notify_clients_after_create + after_update :notify_clients_after_update + after_touch :notify_clients_after_touch + after_destroy :notify_clients_after_destroy end =begin @@ -723,7 +723,7 @@ delete search index object, will be executed automatically def search_index_destroy return if !self.class.search_index_support_config - SearchIndexBackend.remove( self.class.to_s, id ) + SearchIndexBackend.remove(self.class.to_s, id) end =begin @@ -768,7 +768,7 @@ log object create activity stream, if configured - will be executed automaticall def activity_stream_create return if !self.class.activity_stream_support_config - activity_stream_log( 'created', self['created_by_id'] ) + activity_stream_log('created', self['created_by_id']) end =begin @@ -809,7 +809,7 @@ log object update activity stream, if configured - will be executed automaticall return if !log - activity_stream_log( 'updated', self['updated_by_id'] ) + activity_stream_log('updated', self['updated_by_id']) end =begin @@ -823,7 +823,7 @@ delete object activity stream, will be executed automatically def activity_stream_destroy return if !self.class.activity_stream_support_config - ActivityStream.remove( self.class.to_s, id ) + ActivityStream.remove(self.class.to_s, id) end =begin @@ -856,7 +856,7 @@ log object create history, if configured - will be executed automatically def history_create return if !self.class.history_support_config #logger.debug 'create ' + self.changes.inspect - history_log( 'created', created_by_id ) + history_log('created', created_by_id) end @@ -953,7 +953,7 @@ log object update history with all updated attributes, if configured - will be e id_to: value_id[1], } #logger.info "HIST NEW #{self.class.to_s}.find(#{self.id}) #{data.inspect}" - history_log( 'updated', updated_by_id, data ) + history_log('updated', updated_by_id, data) } end @@ -968,7 +968,7 @@ delete object history, will be executed automatically def history_destroy return if !self.class.history_support_config - History.remove( self.class.to_s, id ) + History.remove(self.class.to_s, id) end =begin @@ -985,7 +985,7 @@ returns =end def attachments - Store.list( object: self.class.to_s, o_id: id ) + Store.list(object: self.class.to_s, o_id: id) end =begin @@ -1049,14 +1049,14 @@ get assets of object list def self.assets_of_object_list(list, assets = {}) list.each {|item| require item['object'].to_filename - record = Kernel.const_get( item['object'] ).find( item['o_id'] ) + record = Kernel.const_get(item['object']).find(item['o_id']) assets = record.assets(assets) if item['created_by_id'] - user = User.find( item['created_by_id'] ) + user = User.find(item['created_by_id']) assets = user.assets(assets) end if item['updated_by_id'] - user = User.find( item['updated_by_id'] ) + user = User.find(item['updated_by_id']) assets = user.assets(assets) end } @@ -1103,7 +1103,7 @@ delete object recent viewed list, will be executed automatically =end def recent_view_destroy - RecentView.log_destroy( self.class.to_s, id ) + RecentView.log_destroy(self.class.to_s, id) end =begin diff --git a/app/models/channel/driver/twitter.rb b/app/models/channel/driver/twitter.rb index 4e268fa40..3d5135d36 100644 --- a/app/models/channel/driver/twitter.rb +++ b/app/models/channel/driver/twitter.rb @@ -5,7 +5,7 @@ class Channel::Driver::Twitter def fetch (_adapter_options, channel) @channel = channel - @tweet = Tweet.new( @channel[:options][:auth] ) + @tweet = Tweet.new(@channel[:options][:auth]) @sync = @channel[:options][:sync] Rails.logger.debug 'twitter fetch started' @@ -21,8 +21,8 @@ class Channel::Driver::Twitter def send(article, _notification = false) - @channel = Channel.find_by( area: 'Twitter::Account', active: true ) - @tweet = Tweet.new( @channel[:options][:auth] ) + @channel = Channel.find_by(area: 'Twitter::Account', active: true) + @tweet = Tweet.new(@channel[:options][:auth]) tweet = @tweet.from_article(article) disconnect @@ -49,12 +49,12 @@ class Channel::Driver::Twitter Rails.logger.debug " - searching for '#{search[:term]}'" counter = 0 - @tweet.client.search( search[:term], result_type: result_type ).collect { |tweet| + @tweet.client.search(search[:term], result_type: result_type).collect { |tweet| break if search[:limit] && search[:limit] <= counter - break if Ticket::Article.find_by( message_id: tweet.id.to_s ) + break if Ticket::Article.find_by(message_id: tweet.id) - @tweet.to_group( tweet, search[:group_id] ) + @tweet.to_group(tweet, search[:group_id]) counter += 1 } @@ -72,9 +72,9 @@ class Channel::Driver::Twitter @tweet.client.mentions_timeline.each { |tweet| break if @sync[:mentions][:limit] && @sync[:mentions][:limit] <= counter - break if Ticket::Article.find_by( message_id: tweet.id.to_s ) + break if Ticket::Article.find_by(message_id: tweet.id) - @tweet.to_group( tweet, @sync[:mentions][:group_id] ) + @tweet.to_group(tweet, @sync[:mentions][:group_id]) counter += 1 } @@ -91,9 +91,9 @@ class Channel::Driver::Twitter @tweet.client.direct_messages.each { |tweet| break if @sync[:direct_messages][:limit] && @sync[:direct_messages][:limit] <= counter - break if Ticket::Article.find_by( message_id: tweet.id.to_s ) + break if Ticket::Article.find_by(message_id: tweet.id) - @tweet.to_group( tweet, @sync[:direct_messages][:group_id] ) + @tweet.to_group(tweet, @sync[:direct_messages][:group_id]) counter += 1 } diff --git a/lib/tweet.rb b/lib/tweet.rb index 2a710d217..53b109249 100644 --- a/lib/tweet.rb +++ b/lib/tweet.rb @@ -14,32 +14,29 @@ class Tweet config.access_token = auth[:oauth_token] config.access_token_secret = auth[:oauth_token_secret] end + end def disconnect return if !@client - @client = nil end def user(tweet) - # status (full user data is included) - return tweet.user if tweet.respond_to?('user') - - # direct message (full user data is included) - return tweet.sender if tweet.respond_to?('sender') - - # search (no user data is included, do extra lookup) - begin - return @client.user(tweet.from_user_id) if tweet.respond_to?('from_user_id') - rescue => e - Rails.logger.error "Twitter (#{tweet.id}): 'from_user_id' lookup error '#{e.inspect}'" + if tweet.class == Twitter::DirectMessage + Rails.logger.error "Twitter sender for dm (#{tweet.id}): found" + Rails.logger.debug tweet.sender.inspect + return tweet.sender + elsif tweet.class == Twitter::Tweet + Rails.logger.error "Twitter sender for tweet (#{tweet.id}): found" + Rails.logger.debug tweet.user.inspect + return tweet.user + else + fail "Unknown tweet type '#{tweet.class}'" end - Rails.logger.error "Twitter (#{tweet.id}): unknown user source" - end def to_user(tweet) @@ -50,26 +47,26 @@ class Tweet # do tweet_user lookup tweet_user = user(tweet) - return if !tweet_user - - auth = Authorization.find_by( uid: tweet_user.id, provider: 'twitter' ) + auth = Authorization.find_by(uid: tweet_user.id, provider: 'twitter') # create or update user user_data = { login: tweet_user.screen_name, - firstname: tweet_user.name, - lastname: '', - email: '', - password: '', image_source: tweet_user.profile_image_url.to_s, - note: tweet_user.description, - active: true, - roles: Role.where( name: 'Customer' ), } if auth - user_data[:id] = auth.user_id + user = User.find(auth.user_id) + if (!user_data[:note] || user_data[:note].empty?) && tweet_user.description + user_data[:note] = tweet_user.description + end + user.update_attributes(user_data) + else + user_data[:firstname] = tweet_user.name + user_data[:note] = tweet_user.description + user_data[:active] = true + user_data[:roles] = Role.where(name: 'Customer') + user = User.create(user_data) end - user = User.create_or_update( user_data ) # create or update authorization auth_data = { @@ -79,9 +76,9 @@ class Tweet provider: 'twitter' } if auth - auth.update_attributes( auth_data ) + auth.update_attributes(auth_data) else - Authorization.new( auth_data ) + Authorization.create(auth_data) end UserInfo.current_user_id = user.id @@ -96,19 +93,18 @@ class Tweet Rails.logger.debug user.inspect Rails.logger.debug group_id.inspect - if tweet.class.to_s == 'Twitter::DirectMessage' + if tweet.class == Twitter::DirectMessage article = Ticket::Article.find_by( - from: tweet.in_reply_to_screen_name, - type_id: Ticket::Article::Type.find_by( name: 'twitter direct-message' ).id, + from: tweet.sender.screen_name, + type_id: Ticket::Article::Type.find_by(name: 'twitter direct-message').id, ) - if article ticket = Ticket.find_by( id: article.ticket_id, customer_id: user.id, state: Ticket::State.where.not( state_type_id: Ticket::StateType.where( - name: 'closed', + name: %w(closed merged removed), ) ) ) @@ -120,8 +116,8 @@ class Tweet customer_id: user.id, title: "#{tweet.text[0, 37]}...", group_id: group_id, - state_id: Ticket::State.find_by( name: 'new' ).id, - priority_id: Ticket::Priority.find_by( name: '2 normal' ).id, + state_id: Ticket::State.find_by(name: 'new').id, + priority_id: Ticket::Priority.find_by(name: '2 normal').id, ) end @@ -134,35 +130,36 @@ class Tweet # set ticket state to open if not new if ticket.state.name != 'new' - ticket.state = Ticket::State.find_by( name: 'open' ) + ticket.state = Ticket::State.find_by(name: 'open') ticket.save end # import tweet to = nil - if tweet.respond_to?('recipient') - to = tweet.recipient.name - end - - article_type = 'twitter status' - if tweet.class.to_s == 'Twitter::DirectMessage' - article_type = 'twitter direct-message' - end - + from = nil + article_type = nil in_reply_to = nil - if tweet.respond_to?('in_reply_to_status_id') && tweet.in_reply_to_status_id && tweet.in_reply_to_status_id.to_s != '' + if tweet.class == Twitter::DirectMessage + article_type = 'twitter direct-message' + to = tweet.recipient.screen_name + from = tweet.sender.screen_name + elsif tweet.class == Twitter::Tweet + article_type = 'twitter status' + from = tweet.in_reply_to_screen_name in_reply_to = tweet.in_reply_to_status_id + else + fail "Unknown tweet type '#{tweet.class}'" end Ticket::Article.create( - from: tweet.in_reply_to_screen_name, + from: from, to: to, body: tweet.text, message_id: tweet.id, ticket_id: ticket.id, in_reply_to: in_reply_to, - type_id: Ticket::Article::Type.find_by( name: article_type ).id, - sender_id: Ticket::Article::Sender.find_by( name: 'Customer' ).id, + type_id: Ticket::Article::Type.find_by(name: article_type).id, + sender_id: Ticket::Article::Sender.find_by(name: 'Customer').id, internal: false, ) end @@ -179,26 +176,26 @@ class Tweet # check if parent exists user = to_user(tweet) - - return if !user - - if tweet.respond_to?('in_reply_to_status_id') && tweet.in_reply_to_status_id && tweet.in_reply_to_status_id.to_s != '' - - existing_article = Ticket::Article.find_by( message_id: tweet.in_reply_to_status_id.to_s ) - if existing_article - ticket = existing_article.ticket - else - Rails.logger.debug 'import in_reply_tweet ' + tweet.in_reply_to_status_id.to_s - - parent_tweet = @client.status( tweet.in_reply_to_status_id ) - ticket = to_group( parent_tweet, group_id ) - end - else + if tweet.class == Twitter::DirectMessage ticket = to_ticket(tweet, user, group_id) + to_article(tweet, user, ticket) + elsif tweet.class == Twitter::Tweet + if tweet.in_reply_to_status_id + existing_article = Ticket::Article.find_by(message_id: tweet.in_reply_to_status_id) + if existing_article + ticket = existing_article.ticket + else + parent_tweet = @client.status(tweet.in_reply_to_status_id) + ticket = to_group(parent_tweet, group_id) + end + else + ticket = to_ticket(tweet, user, group_id) + end + to_article(tweet, user, ticket) + else + fail "Unknown tweet type '#{tweet.class}'" end - to_article(tweet, user, ticket) - # execute ticket events Observer::Ticket::Notification.transaction end diff --git a/test/integration/twitter_test.rb b/test/integration/twitter_test.rb index 1e34e0719..f0f496ed5 100644 --- a/test/integration/twitter_test.rb +++ b/test/integration/twitter_test.rb @@ -91,8 +91,8 @@ class TwitterTest < ActiveSupport::TestCase title: text[0, 40], customer_id: user.id, group_id: 2, - state: Ticket::State.find_by( name: 'new' ), - priority: Ticket::Priority.find_by( name: '2 normal' ), + state: Ticket::State.find_by(name: 'new'), + priority: Ticket::Priority.find_by(name: '2 normal'), updated_by_id: 1, created_by_id: 1, ) @@ -135,13 +135,25 @@ class TwitterTest < ActiveSupport::TestCase ) # fetch check system account - Channel.fetch + sleep 10 - # check if follow up article has been created - article = Ticket::Article.find_by( message_id: tweet.id ) + # fetch check system account + article = nil + (1..2).each { + Channel.fetch + + # check if follow up article has been created + article = Ticket::Article.find_by(message_id: tweet.id) + + break if article + + sleep 10 + } assert(article, "article tweet '#{tweet.id}' imported") assert_equal('armin_theo', article.from, 'ticket article inbound from') + assert_equal(nil, article.to, 'ticket article inbound to') + assert_equal(tweet.id.to_s, article.message_id, 'ticket article inbound message_id') assert_equal(2, article.ticket.articles.count, 'ticket article inbound count') assert_equal(reply_text.utf8_to_3bytesutf8, ticket.articles.last.body, 'ticket article inbound body') end @@ -161,11 +173,11 @@ class TwitterTest < ActiveSupport::TestCase tweet = client.update( text, ) - sleep 15 + sleep 10 # fetch check system account article = nil - (1..3).each { + (1..2).each { Channel.fetch # check if ticket and article has been created @@ -190,7 +202,8 @@ class TwitterTest < ActiveSupport::TestCase created_by_id: 1, ) assert(article, "outbound article created, text: #{reply_text}") - sleep 2 + assert_equal(nil, article.to, 'ticket article outbound to') + sleep 5 tweet_found = false client.user_timeline('armin_theo').each { |local_tweet| next if local_tweet.id.to_s != article.message_id.to_s @@ -209,7 +222,7 @@ class TwitterTest < ActiveSupport::TestCase config.access_token = armin_theo_token config.access_token_secret = armin_theo_token_secret end - dms = client.direct_messages(count: 200) + dms = client.direct_messages(count: 40) dms.each {|dm| client.destroy_direct_message(dm.id) } @@ -219,11 +232,10 @@ class TwitterTest < ActiveSupport::TestCase access_token: me_bauer_token, access_token_secret: me_bauer_token_secret ) - dms = client.direct_messages(count: 200) + dms = client.direct_messages(count: 40) dms.each {|dm| client.destroy_direct_message(dm.id) } - hash = '#citheo44' + rand(9999).to_s text = 'How about the details? ' + hash dm = client.create_direct_message( @@ -231,6 +243,7 @@ class TwitterTest < ActiveSupport::TestCase text, ) assert(dm, "dm with ##{hash} created") + sleep 10 # fetch check system account article = nil @@ -242,7 +255,7 @@ class TwitterTest < ActiveSupport::TestCase break if article - sleep 15 + sleep 10 } assert(article, "inbound article '#{text}' created") @@ -275,6 +288,7 @@ class TwitterTest < ActiveSupport::TestCase text, ) assert(dm, "second dm with ##{hash} created") + sleep 10 # fetch check system account article = nil @@ -282,7 +296,7 @@ class TwitterTest < ActiveSupport::TestCase Channel.fetch # check if ticket and article has been created - article = Ticket::Article.find_by( message_id: dm.id ) + article = Ticket::Article.find_by(message_id: dm.id) break if article @@ -290,6 +304,7 @@ class TwitterTest < ActiveSupport::TestCase } assert(article, "inbound article '#{text}' created") + assert_equal(article.ticket.id, ticket.id, 'still the same ticket') ticket = article.ticket assert(ticket, 'ticket of inbound article exists') assert(ticket.articles, 'ticket.articles exists')