diff --git a/.rubocop_todo.rspec.yml b/.rubocop_todo.rspec.yml index 7a38bdc4e..fe218b788 100644 --- a/.rubocop_todo.rspec.yml +++ b/.rubocop_todo.rspec.yml @@ -87,6 +87,8 @@ RSpec/FilePath: - 'spec/db/migrate/issue_2541_fix_notification_email_without_body_spec.rb' - 'spec/db/migrate/issue_2608_missing_trigger_permission_spec.rb' - 'spec/db/migrate/issue_2460_fix_corrupted_twitter_ids_spec.rb' + - 'spec/db/migrate/issue_2715_fix_broken_twitter_urls_spec.rb' + - 'spec/jobs/issue_2715_fix_broken_twitter_urls_job_spec.rb' - 'spec/lib/import/base_factory_spec.rb' # Offense count: 60 diff --git a/app/jobs/issue_2715_fix_broken_twitter_urls_job.rb b/app/jobs/issue_2715_fix_broken_twitter_urls_job.rb new file mode 100644 index 000000000..56a9757f1 --- /dev/null +++ b/app/jobs/issue_2715_fix_broken_twitter_urls_job.rb @@ -0,0 +1,15 @@ +class Issue2715FixBrokenTwitterUrlsJob < ApplicationJob + def perform + Ticket::Article.joins(:type) + .where(ticket_article_types: { name: 'twitter direct-message' }) + .order(created_at: :desc) + .limit(10_000) + .find_each do |dm| + dm.preferences[:links]&.each do |link| + link[:url] = "https://twitter.com/messages/#{dm.preferences[:twitter][:recipient_id]}-#{dm.preferences[:twitter][:sender_id]}" + end + + dm.save! + end + end +end diff --git a/app/models/observer/ticket/article/communicate_twitter/background_job.rb b/app/models/observer/ticket/article/communicate_twitter/background_job.rb index 256bfd118..9c14fb155 100644 --- a/app/models/observer/ticket/article/communicate_twitter/background_job.rb +++ b/app/models/observer/ticket/article/communicate_twitter/background_job.rb @@ -51,10 +51,9 @@ class Observer::Ticket::Article::CommunicateTwitter::BackgroundJob # fill article with tweet info # direct message - tweet_id = nil if tweet.is_a?(Hash) tweet_type = 'DirectMessage' - tweet_id = tweet[:event][:id].to_s + article.message_id = tweet[:event][:id].to_s if tweet[:event] && tweet[:event][:type] == 'message_create' #article.from = "@#{tweet.sender.screen_name}" #article.to = "@#{tweet.recipient.screen_name}" @@ -63,12 +62,19 @@ class Observer::Ticket::Article::CommunicateTwitter::BackgroundJob recipient_id: tweet[:event][:message_create][:target][:recipient_id], sender_id: tweet[:event][:message_create][:sender_id], } + + article.preferences['links'] = [ + { + url: "https://twitter.com/messages/#{article.preferences[:twitter][:recipient_id]}-#{article.preferences[:twitter][:sender_id]}", + target: '_blank', + name: 'on Twitter', + }, + ] end # regular tweet elsif tweet.class == Twitter::Tweet tweet_type = 'Tweet' - tweet_id = tweet.id.to_s article.from = "@#{tweet.user.screen_name}" if tweet.user_mentions to = '' @@ -95,6 +101,15 @@ class Observer::Ticket::Article::CommunicateTwitter::BackgroundJob created_at: tweet.created_at, ) end + + article.message_id = tweet.id.to_s + article.preferences['links'] = [ + { + url: "https://twitter.com/statuses/#{tweet.id}", + target: '_blank', + name: 'on Twitter', + }, + ] else raise "Unknown tweet type '#{tweet.class}'" end @@ -104,15 +119,6 @@ class Observer::Ticket::Article::CommunicateTwitter::BackgroundJob article.preferences['delivery_status'] = 'success' article.preferences['delivery_status_date'] = Time.zone.now - article.message_id = tweet_id - article.preferences['links'] = [ - { - url: "https://twitter.com/statuses/#{tweet_id}", - target: '_blank', - name: 'on Twitter', - }, - ] - article.save! Rails.logger.info "Send twitter (#{tweet_type}) to: '#{article.to}' (from #{article.from})" diff --git a/db/migrate/20191107181428_issue_2715_fix_broken_twitter_urls.rb b/db/migrate/20191107181428_issue_2715_fix_broken_twitter_urls.rb new file mode 100644 index 000000000..124b5b199 --- /dev/null +++ b/db/migrate/20191107181428_issue_2715_fix_broken_twitter_urls.rb @@ -0,0 +1,9 @@ +require_dependency 'issue_2715_fix_broken_twitter_urls_job' # Rails autoloading expects `issue2715_fix...` + +class Issue2715FixBrokenTwitterUrls < ActiveRecord::Migration[5.2] + def up + return if !Setting.find_by(name: 'system_init_done') + + Issue2715FixBrokenTwitterUrlsJob.perform_later + end +end diff --git a/lib/twitter_sync.rb b/lib/twitter_sync.rb index b351bf818..d49fa2fa1 100644 --- a/lib/twitter_sync.rb +++ b/lib/twitter_sync.rb @@ -190,7 +190,6 @@ class TwitterSync message_id = nil article_type = nil in_reply_to = nil - twitter_preferences = {} attachments = [] if item['type'] == 'message_create' @@ -213,6 +212,7 @@ class TwitterSync sender_screen_name = to_user_webhook_data(item['message_create']['sender_id'])['screen_name'] to = "@#{recipient_screen_name}" from = "@#{sender_screen_name}" + twitter_preferences = { created_at: item['created_timestamp'], recipient_id: item['message_create']['target']['recipient_id'], @@ -222,6 +222,18 @@ class TwitterSync app_id: app['app_id'], app_name: app['app_name'], } + + article_preferences = { + twitter: self.class.preferences_cleanup(twitter_preferences), + links: [ + { + url: "https://twitter.com/messages/#{twitter_preferences[:recipient_id]}-#{twitter_preferences[:sender_id]}", + target: '_blank', + name: 'on Twitter', + }, + ], + } + elsif item['text'].present? message_id = item['id'] text = item['text'] @@ -287,6 +299,17 @@ class TwitterSync truncated: item['truncated'], } + article_preferences = { + twitter: self.class.preferences_cleanup(twitter_preferences), + links: [ + { + url: "https://twitter.com/statuses/#{item['id']}", + target: '_blank', + name: 'on Twitter', + }, + ], + } + else raise "Unknown tweet type '#{item.class}'" end @@ -300,17 +323,6 @@ class TwitterSync ticket.save! end - article_preferences = { - twitter: self.class.preferences_cleanup(twitter_preferences), - links: [ - { - url: "https://twitter.com/statuses/#{item['id']}", - target: '_blank', - name: 'on Twitter', - }, - ], - } - article = Ticket::Article.create!( from: from, to: to, diff --git a/spec/db/migrate/issue_2715_fix_broken_twitter_urls_spec.rb b/spec/db/migrate/issue_2715_fix_broken_twitter_urls_spec.rb new file mode 100644 index 000000000..06e352eb8 --- /dev/null +++ b/spec/db/migrate/issue_2715_fix_broken_twitter_urls_spec.rb @@ -0,0 +1,9 @@ +require 'rails_helper' +require_dependency 'issue_2715_fix_broken_twitter_urls_job' # Rails autoloading expects `issue2715_fix...` + +RSpec.describe Issue2715FixBrokenTwitterUrls, type: :db_migration do + it 'invokes the corresponding job', :performs_jobs do + expect { migrate } + .to have_enqueued_job(Issue2715FixBrokenTwitterUrlsJob) + end +end diff --git a/spec/factories/ticket/article.rb b/spec/factories/ticket/article.rb index 3f6ea34b9..4cb9cb00f 100644 --- a/spec/factories/ticket/article.rb +++ b/spec/factories/ticket/article.rb @@ -25,5 +25,14 @@ FactoryBot.define do association :ticket, factory: :twitter_ticket body { Faker::Lorem.sentence } end + + factory :twitter_dm_article do + transient do + type_name { 'twitter direct-message' } + end + + association :ticket, factory: :twitter_ticket + body { Faker::Lorem.sentence } + end end end diff --git a/spec/jobs/issue_2715_fix_broken_twitter_urls_job_spec.rb b/spec/jobs/issue_2715_fix_broken_twitter_urls_job_spec.rb new file mode 100644 index 000000000..08625455b --- /dev/null +++ b/spec/jobs/issue_2715_fix_broken_twitter_urls_job_spec.rb @@ -0,0 +1,33 @@ +require 'rails_helper' +require_dependency 'issue_2715_fix_broken_twitter_urls_job' # Rails autoloading expects `issue2715_fix...` + +RSpec.describe Issue2715FixBrokenTwitterUrlsJob, type: :job do + context 'with existing Twitter articles' do + let!(:dm) { create(:twitter_dm_article, preferences: dm_preferences) } + + let(:dm_preferences) do + { + links: Array.new(5, &link_hash), + twitter: { + recipient_id: recipient_id, + sender_id: sender_id, + }, + } + end + + # NOTE: Faker 2.0+ has deprecated the `#number(20)` syntax in favor of `#number(digits: 20)`. + let(:link_hash) { ->(_) { { url: "https://twitter.com/statuses/#{Faker::Number.number(20)}" } } } + let(:recipient_id) { '1234567890' } + let(:sender_id) { '0987654321' } + + it 'reformats all Twitter DM URLs' do + expect { described_class.perform_now } + .to change { urls_of(dm) } + .to all(match(%r{^https://twitter.com/messages/#{recipient_id}-#{sender_id}$})) + end + + def urls_of(article) + article.reload.preferences[:links].map { |link| link[:url] } + end + end +end