From 25cd6449516e8f9303e4928c03e93996b9e449b0 Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Thu, 14 Nov 2019 14:03:34 +0800 Subject: [PATCH] Fixes #2715 - Twitter status URLs are broken This commit is a complement of its parent: the parent fixes broken links to Twitter DMs, while this one fixes broken links to Twitter statuses. When importing tweets (articles of type "twitter status"), Zammad generates a source link URL using the following format: https://twitter.com/statuses/:id and then stores that URL under `article.preferences[:links]`. This URL template worked until as recently as 2017[0], but currently fails for users actively logged in to Twitter. Now, the correct URL template appears to be https://twitter.com/:user_id/status/:id where `:user_id` is not strict, and may be any word (\w+) <= 20 chars. Try it yourself: $ curl https://twitter.com/elonmusk/status/1069382411899817990 You are being redirected. In this commit, we replace `:user_id` with a single underscore (`_`). This behavior is not officially documented anywhere (as far as I know), but it works (for now). This commit also extends the previous commit's DB migration/bg job to rectify existing, broken tweet URLs stored in the database. For performance purposes, this migration is performed in the background and limited to the latest 10,000 Twitter articles. [0]: https://stackoverflow.com/questions/41786123 GitHub: https://github.com/zammad/zammad/issues/2715 --- .../issue_2715_fix_broken_twitter_urls_job.rb | 26 ++++++++++++++----- .../communicate_twitter/background_job.rb | 2 +- lib/twitter_sync.rb | 10 ++++--- spec/factories/ticket/article.rb | 1 + ...e_2715_fix_broken_twitter_urls_job_spec.rb | 17 +++++++++--- spec/models/ticket/article_spec.rb | 4 +-- ...tweet_id_https_twitter_com_status_id_.yml} | 0 7 files changed, 44 insertions(+), 16 deletions(-) rename test/data/vcr_cassettes/models/ticket/article/{auto-setting_of_outgoing_twitter_article_attributes_via_bg_jobs_sets_message_id_to_tweet_id_https_twitter_com_statuses_id_.yml => auto-setting_of_outgoing_twitter_article_attributes_via_bg_jobs_sets_message_id_to_tweet_id_https_twitter_com_status_id_.yml} (100%) diff --git a/app/jobs/issue_2715_fix_broken_twitter_urls_job.rb b/app/jobs/issue_2715_fix_broken_twitter_urls_job.rb index 56a9757f1..792d0a965 100644 --- a/app/jobs/issue_2715_fix_broken_twitter_urls_job.rb +++ b/app/jobs/issue_2715_fix_broken_twitter_urls_job.rb @@ -1,15 +1,29 @@ class Issue2715FixBrokenTwitterUrlsJob < ApplicationJob + STATUS_TEMPLATE = 'https://twitter.com/_/status/%s'.freeze + DM_TEMPLATE = 'https://twitter.com/messages/%s-%s'.freeze + def perform Ticket::Article.joins(:type) - .where(ticket_article_types: { name: 'twitter direct-message' }) + .where(ticket_article_types: { name: ['twitter status', '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 + .find_each(&method(:fix_broken_links)) + end - dm.save! + private + + def fix_broken_links(article) + type = Ticket::Article::Type.lookup(id: article.type_id).name + + article.preferences[:links]&.each do |link| + link[:url] = case type + when 'twitter status' + STATUS_TEMPLATE % article.attributes.symbolize_keys + when 'twitter direct-message' + DM_TEMPLATE % article.preferences[:twitter].symbolize_keys + end end + + article.save! 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 9c14fb155..041ef4e99 100644 --- a/app/models/observer/ticket/article/communicate_twitter/background_job.rb +++ b/app/models/observer/ticket/article/communicate_twitter/background_job.rb @@ -105,7 +105,7 @@ class Observer::Ticket::Article::CommunicateTwitter::BackgroundJob article.message_id = tweet.id.to_s article.preferences['links'] = [ { - url: "https://twitter.com/statuses/#{tweet.id}", + url: TwitterSync::STATUS_URL_TEMPLATE % tweet.id, target: '_blank', name: 'on Twitter', }, diff --git a/lib/twitter_sync.rb b/lib/twitter_sync.rb index d49fa2fa1..c48b006b1 100644 --- a/lib/twitter_sync.rb +++ b/lib/twitter_sync.rb @@ -4,6 +4,8 @@ require 'http/uri' class TwitterSync + STATUS_URL_TEMPLATE = 'https://twitter.com/_/status/%s'.freeze + attr_accessor :client def initialize(auth, payload = nil) @@ -303,7 +305,7 @@ class TwitterSync twitter: self.class.preferences_cleanup(twitter_preferences), links: [ { - url: "https://twitter.com/statuses/#{item['id']}", + url: STATUS_URL_TEMPLATE % item['id'], target: '_blank', name: 'on Twitter', }, @@ -399,7 +401,7 @@ class TwitterSync twitter: self.class.preferences_cleanup(twitter_preferences), links: [ { - url: "https://twitter.com/statuses/#{tweet.id}", + url: STATUS_URL_TEMPLATE % tweet.id, target: '_blank', name: 'on Twitter', }, @@ -571,7 +573,7 @@ create a tweet ot direct message from an article twitter: twitter_preferences, links: [ { - url: 'https://twitter.com/statuses/123', + url: 'https://twitter.com/_/status/123', target: '_blank', name: 'on Twitter', }, @@ -584,7 +586,7 @@ or twitter: TwitterSync.preferences_cleanup(twitter_preferences), links: [ { - url: 'https://twitter.com/statuses/123', + url: 'https://twitter.com/_/status/123', target: '_blank', name: 'on Twitter', }, diff --git a/spec/factories/ticket/article.rb b/spec/factories/ticket/article.rb index 4cb9cb00f..9f151e3d5 100644 --- a/spec/factories/ticket/article.rb +++ b/spec/factories/ticket/article.rb @@ -23,6 +23,7 @@ FactoryBot.define do end association :ticket, factory: :twitter_ticket + message_id { '775410014383026176' } body { Faker::Lorem.sentence } 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 index 08625455b..e8a7d0dbe 100644 --- a/spec/jobs/issue_2715_fix_broken_twitter_urls_job_spec.rb +++ b/spec/jobs/issue_2715_fix_broken_twitter_urls_job_spec.rb @@ -3,11 +3,18 @@ require_dependency 'issue_2715_fix_broken_twitter_urls_job' # Rails autoloading RSpec.describe Issue2715FixBrokenTwitterUrlsJob, type: :job do context 'with existing Twitter articles' do + let!(:tweet) { create(:twitter_article, preferences: tweet_preferences) } let!(:dm) { create(:twitter_dm_article, preferences: dm_preferences) } + let(:tweet_preferences) do + # NOTE: Faker 2.0+ has deprecated the `#number(20)` syntax in favor of `#number(digits: 20)`. + { links: [{ url: "https://twitter.com/statuses/#{Faker::Number.number(20)}" }] } + end + let(:dm_preferences) do { - links: Array.new(5, &link_hash), + # NOTE: Faker 2.0+ has deprecated the `#number(20)` syntax in favor of `#number(digits: 20)`. + links: [{ url: "https://twitter.com/statuses/#{Faker::Number.number(20)}" }], twitter: { recipient_id: recipient_id, sender_id: sender_id, @@ -15,11 +22,15 @@ RSpec.describe Issue2715FixBrokenTwitterUrlsJob, type: :job do } 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 status URLs' do + expect { described_class.perform_now } + .to change { urls_of(tweet) } + .to all(match(%r{^https://twitter.com/_/status/#{tweet.message_id}$})) + end + it 'reformats all Twitter DM URLs' do expect { described_class.perform_now } .to change { urls_of(dm) } diff --git a/spec/models/ticket/article_spec.rb b/spec/models/ticket/article_spec.rb index 392e27cb5..7a91fd291 100644 --- a/spec/models/ticket/article_spec.rb +++ b/spec/models/ticket/article_spec.rb @@ -317,7 +317,7 @@ RSpec.describe Ticket::Article, type: :model do .to('') # Tweet in VCR cassette is addressed to no one end - it 'sets #message_id to tweet ID (https://twitter.com/statuses/)' do + it 'sets #message_id to tweet ID (https://twitter.com/_/status/)' do expect(&run_bg_jobs) .to change { twitter_article.reload.message_id } .to('1069382411899817990') @@ -332,7 +332,7 @@ RSpec.describe Ticket::Article, type: :model do .to include( 'name' => 'on Twitter', 'target' => '_blank', - 'url' => "https://twitter.com/statuses/#{twitter_article.message_id}" + 'url' => "https://twitter.com/_/status/#{twitter_article.message_id}" ) end diff --git a/test/data/vcr_cassettes/models/ticket/article/auto-setting_of_outgoing_twitter_article_attributes_via_bg_jobs_sets_message_id_to_tweet_id_https_twitter_com_statuses_id_.yml b/test/data/vcr_cassettes/models/ticket/article/auto-setting_of_outgoing_twitter_article_attributes_via_bg_jobs_sets_message_id_to_tweet_id_https_twitter_com_status_id_.yml similarity index 100% rename from test/data/vcr_cassettes/models/ticket/article/auto-setting_of_outgoing_twitter_article_attributes_via_bg_jobs_sets_message_id_to_tweet_id_https_twitter_com_statuses_id_.yml rename to test/data/vcr_cassettes/models/ticket/article/auto-setting_of_outgoing_twitter_article_attributes_via_bg_jobs_sets_message_id_to_tweet_id_https_twitter_com_status_id_.yml