Change link previews to keep original URL from the status (#27312)

This commit is contained in:
Eugen Rochko 2023-11-13 10:58:28 +01:00 committed by GitHub
parent 9dc3ce878b
commit 0d14fcebae
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
19 changed files with 70 additions and 30 deletions

View file

@ -53,7 +53,7 @@ class PublicStatusesIndex < Chewy::Index
index_scope ::Status.unscoped index_scope ::Status.unscoped
.kept .kept
.indexable .indexable
.includes(:media_attachments, :preloadable_poll, :preview_cards, :tags) .includes(:media_attachments, :preloadable_poll, :tags, preview_cards_status: :preview_card)
root date_detection: false do root date_detection: false do
field(:id, type: 'long') field(:id, type: 'long')

View file

@ -50,7 +50,7 @@ class StatusesIndex < Chewy::Index
}, },
} }
index_scope ::Status.unscoped.kept.without_reblogs.includes(:media_attachments, :preview_cards, :local_mentioned, :local_favorited, :local_reblogged, :local_bookmarked, :tags, preloadable_poll: :local_voters), delete_if: ->(status) { status.searchable_by.empty? } index_scope ::Status.unscoped.kept.without_reblogs.includes(:media_attachments, :local_mentioned, :local_favorited, :local_reblogged, :local_bookmarked, :tags, preview_cards_status: :preview_card, preloadable_poll: :local_voters), delete_if: ->(status) { status.searchable_by.empty? }
root date_detection: false do root date_detection: false do
field(:id, type: 'long') field(:id, type: 'long')

View file

@ -41,10 +41,10 @@ class Api::V1::ConversationsController < Api::BaseController
account: :account_stat, account: :account_stat,
last_status: [ last_status: [
:media_attachments, :media_attachments,
:preview_cards,
:status_stat, :status_stat,
:tags, :tags,
{ {
preview_cards_status: :preview_card,
active_mentions: [account: :account_stat], active_mentions: [account: :account_stat],
account: :account_stat, account: :account_stat,
}, },

View file

@ -74,7 +74,7 @@ class Admin::StatusBatchAction
# Can't use a transaction here because UpdateStatusService queues # Can't use a transaction here because UpdateStatusService queues
# Sidekiq jobs # Sidekiq jobs
statuses.includes(:media_attachments, :preview_cards).find_each do |status| statuses.includes(:media_attachments, preview_cards_status: :preview_card).find_each do |status|
next if status.discarded? || !(status.with_media? || status.with_preview_card?) next if status.discarded? || !(status.with_media? || status.with_preview_card?)
authorize([:admin, status], :update?) authorize([:admin, status], :update?)

View file

@ -40,7 +40,7 @@ module StatusSearchConcern
properties << 'media' if with_media? properties << 'media' if with_media?
properties << 'poll' if with_poll? properties << 'poll' if with_poll?
properties << 'link' if with_preview_card? properties << 'link' if with_preview_card?
properties << 'embed' if preview_cards.any?(&:video?) properties << 'embed' if preview_card&.video?
properties << 'sensitive' if sensitive? properties << 'sensitive' if sensitive?
properties << 'reply' if reply? properties << 'reply' if reply?
end end

View file

@ -50,7 +50,9 @@ class PreviewCard < ApplicationRecord
enum type: { link: 0, photo: 1, video: 2, rich: 3 } enum type: { link: 0, photo: 1, video: 2, rich: 3 }
enum link_type: { unknown: 0, article: 1 } enum link_type: { unknown: 0, article: 1 }
has_and_belongs_to_many :statuses has_many :preview_cards_statuses, dependent: :delete_all, inverse_of: :preview_card
has_many :statuses, through: :preview_cards_statuses
has_one :trend, class_name: 'PreviewCardTrend', inverse_of: :preview_card, dependent: :destroy has_one :trend, class_name: 'PreviewCardTrend', inverse_of: :preview_card, dependent: :destroy
has_attached_file :image, processors: [:thumbnail, :blurhash_transcoder], styles: ->(f) { image_styles(f) }, convert_options: { all: '-quality 90 +profile "!icc,*" +set date:modify +set date:create +set date:timestamp' }, validate_media_type: false has_attached_file :image, processors: [:thumbnail, :blurhash_transcoder], styles: ->(f) { image_styles(f) }, convert_options: { all: '-quality 90 +profile "!icc,*" +set date:modify +set date:create +set date:timestamp' }, validate_media_type: false
@ -64,6 +66,9 @@ class PreviewCard < ApplicationRecord
before_save :extract_dimensions, if: :link? before_save :extract_dimensions, if: :link?
# This can be set by the status when retrieving the preview card using the join model
attr_accessor :original_url
def appropriate_for_trends? def appropriate_for_trends?
link? && article? && title.present? && description.present? && image.present? && provider_name.present? link? && article? && title.present? && description.present? && image.present? && provider_name.present?
end end

View file

@ -0,0 +1,18 @@
# frozen_string_literal: true
# == Schema Information
#
# Table name: preview_cards_statuses
#
# preview_card_id :bigint(8) not null
# status_id :bigint(8) not null
# url :string
#
class PreviewCardsStatus < ApplicationRecord
# Composite primary keys are not properly supported in Rails. However,
# we shouldn't need this anyway...
self.primary_key = nil
belongs_to :preview_card
belongs_to :status
end

View file

@ -79,8 +79,8 @@ class Status < ApplicationRecord
has_many :local_bookmarked, -> { merge(Account.local) }, through: :bookmarks, source: :account has_many :local_bookmarked, -> { merge(Account.local) }, through: :bookmarks, source: :account
has_and_belongs_to_many :tags has_and_belongs_to_many :tags
has_and_belongs_to_many :preview_cards
has_one :preview_cards_status, inverse_of: :status # Because of a composite primary key, the dependent option cannot be used
has_one :notification, as: :activity, dependent: :destroy has_one :notification, as: :activity, dependent: :destroy
has_one :status_stat, inverse_of: :status has_one :status_stat, inverse_of: :status
has_one :poll, inverse_of: :status, dependent: :destroy has_one :poll, inverse_of: :status, dependent: :destroy
@ -142,24 +142,25 @@ class Status < ApplicationRecord
# The `prepend: true` option below ensures this runs before # The `prepend: true` option below ensures this runs before
# the `dependent: destroy` callbacks remove relevant records # the `dependent: destroy` callbacks remove relevant records
before_destroy :unlink_from_conversations!, prepend: true before_destroy :unlink_from_conversations!, prepend: true
before_destroy :reset_preview_card!
cache_associated :application, cache_associated :application,
:media_attachments, :media_attachments,
:conversation, :conversation,
:status_stat, :status_stat,
:tags, :tags,
:preview_cards,
:preloadable_poll, :preloadable_poll,
preview_cards_status: [:preview_card],
account: [:account_stat, user: :role], account: [:account_stat, user: :role],
active_mentions: { account: :account_stat }, active_mentions: { account: :account_stat },
reblog: [ reblog: [
:application, :application,
:tags, :tags,
:preview_cards,
:media_attachments, :media_attachments,
:conversation, :conversation,
:status_stat, :status_stat,
:preloadable_poll, :preloadable_poll,
preview_cards_status: [:preview_card],
account: [:account_stat, user: :role], account: [:account_stat, user: :role],
active_mentions: { account: :account_stat }, active_mentions: { account: :account_stat },
], ],
@ -226,7 +227,11 @@ class Status < ApplicationRecord
end end
def preview_card def preview_card
preview_cards.first preview_cards_status&.preview_card&.tap { |x| x.original_url = preview_cards_status.url }
end
def reset_preview_card!
PreviewCardsStatus.where(status_id: id).delete_all
end end
def hidden? def hidden?
@ -244,7 +249,7 @@ class Status < ApplicationRecord
end end
def with_preview_card? def with_preview_card?
preview_cards.any? preview_cards_status.present?
end end
def with_poll? def with_poll?

View file

@ -54,9 +54,7 @@ class Trends::Links < Trends::Base
!(original_status.account.silenced? || status.account.silenced?) && !(original_status.account.silenced? || status.account.silenced?) &&
!(original_status.spoiler_text? || original_status.sensitive?) !(original_status.spoiler_text? || original_status.sensitive?)
original_status.preview_cards.each do |preview_card| add(original_status.preview_card, status.account_id, at_time) if original_status.preview_card&.appropriate_for_trends?
add(preview_card, status.account_id, at_time) if preview_card.appropriate_for_trends?
end
end end
def add(preview_card, account_id, at_time = Time.now.utc) def add(preview_card, account_id, at_time = Time.now.utc)

View file

@ -8,6 +8,10 @@ class REST::PreviewCardSerializer < ActiveModel::Serializer
:provider_url, :html, :width, :height, :provider_url, :html, :width, :height,
:image, :image_description, :embed_url, :blurhash, :published_at :image, :image_description, :embed_url, :blurhash, :published_at
def url
object.original_url.presence || object.url
end
def image def image
object.image? ? full_asset_url(object.image.url(:original)) : nil object.image? ? full_asset_url(object.image.url(:original)) : nil
end end

View file

@ -280,7 +280,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
end end
def reset_preview_card! def reset_preview_card!
@status.preview_cards.clear @status.reset_preview_card!
LinkCrawlWorker.perform_in(rand(1..59).seconds, @status.id) LinkCrawlWorker.perform_in(rand(1..59).seconds, @status.id)
end end

View file

@ -19,7 +19,7 @@ class FetchLinkCardService < BaseService
@status = status @status = status
@original_url = parse_urls @original_url = parse_urls
return if @original_url.nil? || @status.preview_cards.any? return if @original_url.nil? || @status.with_preview_card?
@url = @original_url.to_s @url = @original_url.to_s
@ -62,9 +62,9 @@ class FetchLinkCardService < BaseService
def attach_card def attach_card
with_redis_lock("attach_card:#{@status.id}") do with_redis_lock("attach_card:#{@status.id}") do
return if @status.preview_cards.any? return if @status.with_preview_card?
@status.preview_cards << @card PreviewCardsStatus.create(status: @status, preview_card: @card, url: @original_url)
Rails.cache.delete(@status) Rails.cache.delete(@status)
Trends.links.register(@status) Trends.links.register(@status)
end end

View file

@ -123,7 +123,7 @@ class UpdateStatusService < BaseService
def reset_preview_card! def reset_preview_card!
return unless @status.text_previously_changed? return unless @status.text_previously_changed?
@status.preview_cards.clear @status.reset_preview_card!
LinkCrawlWorker.perform_async(@status.id) LinkCrawlWorker.perform_async(@status.id)
end end

View file

@ -0,0 +1,7 @@
# frozen_string_literal: true
class AddURLToPreviewCardsStatuses < ActiveRecord::Migration[7.0]
def change
add_column :preview_cards_statuses, :url, :string
end
end

View file

@ -10,7 +10,7 @@
# #
# It's strongly recommended that you check this file into your version control system. # It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema[7.0].define(version: 2023_09_07_150100) do ActiveRecord::Schema[7.0].define(version: 2023_10_06_183200) do
# These are extensions that must be enabled in order to support this database # These are extensions that must be enabled in order to support this database
enable_extension "plpgsql" enable_extension "plpgsql"
@ -811,6 +811,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_09_07_150100) do
create_table "preview_cards_statuses", primary_key: ["status_id", "preview_card_id"], force: :cascade do |t| create_table "preview_cards_statuses", primary_key: ["status_id", "preview_card_id"], force: :cascade do |t|
t.bigint "preview_card_id", null: false t.bigint "preview_card_id", null: false
t.bigint "status_id", null: false t.bigint "status_id", null: false
t.string "url"
end end
create_table "relays", force: :cascade do |t| create_table "relays", force: :cascade do |t|

View file

@ -69,7 +69,7 @@ namespace :tests do
exit(1) exit(1)
end end
unless Status.find(12).preview_cards.pluck(:url) == ['https://joinmastodon.org/'] unless PreviewCard.where(id: PreviewCardsStatus.where(status_id: 12).select(:preview_card_id)).pluck(:url) == ['https://joinmastodon.org/']
puts 'Preview cards not deduplicated as expected' puts 'Preview cards not deduplicated as expected'
exit(1) exit(1)
end end

View file

@ -49,10 +49,12 @@ describe MediaComponentHelper do
end end
describe 'render_card_component' do describe 'render_card_component' do
let(:status) { Fabricate(:status, preview_cards: [Fabricate(:preview_card)]) } let(:status) { Fabricate(:status) }
let(:result) { helper.render_card_component(status) } let(:result) { helper.render_card_component(status) }
before do before do
PreviewCardsStatus.create(status: status, preview_card: Fabricate(:preview_card))
without_partial_double_verification do without_partial_double_verification do
allow(helper).to receive(:current_account).and_return(status.account) allow(helper).to receive(:current_account).and_return(status.account)
end end

View file

@ -120,7 +120,7 @@ RSpec.describe FetchLinkCardService, type: :service do
let(:status) { Fabricate(:status, text: 'Check out http://example.com/sjis') } let(:status) { Fabricate(:status, text: 'Check out http://example.com/sjis') }
it 'decodes the HTML' do it 'decodes the HTML' do
expect(status.preview_cards.first.title).to eq('SJISのページ') expect(status.preview_card.title).to eq('SJISのページ')
end end
end end
@ -128,7 +128,7 @@ RSpec.describe FetchLinkCardService, type: :service do
let(:status) { Fabricate(:status, text: 'Check out http://example.com/sjis_with_wrong_charset') } let(:status) { Fabricate(:status, text: 'Check out http://example.com/sjis_with_wrong_charset') }
it 'decodes the HTML despite the wrong charset header' do it 'decodes the HTML despite the wrong charset header' do
expect(status.preview_cards.first.title).to eq('SJISのページ') expect(status.preview_card.title).to eq('SJISのページ')
end end
end end
@ -136,7 +136,7 @@ RSpec.describe FetchLinkCardService, type: :service do
let(:status) { Fabricate(:status, text: 'Check out http://example.com/koi8-r') } let(:status) { Fabricate(:status, text: 'Check out http://example.com/koi8-r') }
it 'decodes the HTML' do it 'decodes the HTML' do
expect(status.preview_cards.first.title).to eq('Московя начинаетъ только въ XVI ст. привлекать внимане иностранцевъ.') expect(status.preview_card.title).to eq('Московя начинаетъ только въ XVI ст. привлекать внимане иностранцевъ.')
end end
end end
@ -144,7 +144,7 @@ RSpec.describe FetchLinkCardService, type: :service do
let(:status) { Fabricate(:status, text: 'Check out http://example.com/windows-1251') } let(:status) { Fabricate(:status, text: 'Check out http://example.com/windows-1251') }
it 'decodes the HTML' do it 'decodes the HTML' do
expect(status.preview_cards.first.title).to eq('сэмпл текст') expect(status.preview_card.title).to eq('сэмпл текст')
end end
end end

View file

@ -23,11 +23,11 @@ RSpec.describe UpdateStatusService, type: :service do
end end
context 'when text changes' do context 'when text changes' do
let!(:status) { Fabricate(:status, text: 'Foo') } let(:status) { Fabricate(:status, text: 'Foo') }
let(:preview_card) { Fabricate(:preview_card) } let(:preview_card) { Fabricate(:preview_card) }
before do before do
status.preview_cards << preview_card PreviewCardsStatus.create(status: status, preview_card: preview_card)
subject.call(status, status.account_id, text: 'Bar') subject.call(status, status.account_id, text: 'Bar')
end end
@ -45,11 +45,11 @@ RSpec.describe UpdateStatusService, type: :service do
end end
context 'when content warning changes' do context 'when content warning changes' do
let!(:status) { Fabricate(:status, text: 'Foo', spoiler_text: '') } let(:status) { Fabricate(:status, text: 'Foo', spoiler_text: '') }
let(:preview_card) { Fabricate(:preview_card) } let(:preview_card) { Fabricate(:preview_card) }
before do before do
status.preview_cards << preview_card PreviewCardsStatus.create(status: status, preview_card: preview_card)
subject.call(status, status.account_id, text: 'Foo', spoiler_text: 'Bar') subject.call(status, status.account_id, text: 'Foo', spoiler_text: 'Bar')
end end