Fixes #2715 - Twitter DM URLs are broken

When importing Twitter DMs (articles of type "twitter direct-message"),
Zammad generates a source link URL of an incorrect format:

    https://twitter.com/statuses/:id

This format is for statuses (or "tweets").
The appropriate URL scheme for direct messages is:

    https://twitter.com/messages/:recipient_id-:current_user_id

This commit adds conditional logic for setting Twitter link URLs
based on message type.
It also provides a DB migration to rectify existing, broken DM URLs.
For performance purposes, this migration is performed in the background
and limited to the latest 10,000 DMs.

GitHub: https://github.com/zammad/zammad/issues/2715

.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

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

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})"

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

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,

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

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

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