diff --git a/app/models/preview_card.rb b/app/models/preview_card.rb index 56fe483635..8d9a6f1779 100644 --- a/app/models/preview_card.rb +++ b/app/models/preview_card.rb @@ -33,6 +33,7 @@ # published_at :datetime # image_description :string default(""), not null # author_account_id :bigint(8) +# image_remote_url :string # class PreviewCard < ApplicationRecord diff --git a/db/migrate/20240803160904_add_image_remote_url_to_preview_cards.rb b/db/migrate/20240803160904_add_image_remote_url_to_preview_cards.rb new file mode 100644 index 0000000000..d727324f2c --- /dev/null +++ b/db/migrate/20240803160904_add_image_remote_url_to_preview_cards.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class AddImageRemoteURLToPreviewCards < ActiveRecord::Migration[7.1] + disable_ddl_transaction! + + def up + safety_assured { add_column :preview_cards, :image_remote_url, :string } + end + + def down + remove_column :preview_cards, :image_remote_url + end +end diff --git a/db/schema.rb b/db/schema.rb index db1687ba99..91593ed63f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -861,6 +861,7 @@ ActiveRecord::Schema[8.0].define(version: 2025_04_28_095029) do t.datetime "published_at" t.string "image_description", default: "", null: false t.bigint "author_account_id" + t.string "image_remote_url" t.index ["author_account_id"], name: "index_preview_cards_on_author_account_id", where: "(author_account_id IS NOT NULL)" t.index ["url"], name: "index_preview_cards_on_url", unique: true end diff --git a/spec/fabricators/preview_card_fabricator.rb b/spec/fabricators/preview_card_fabricator.rb index b8f2c50972..2e0c0948ec 100644 --- a/spec/fabricators/preview_card_fabricator.rb +++ b/spec/fabricators/preview_card_fabricator.rb @@ -1,9 +1,13 @@ # frozen_string_literal: true Fabricator(:preview_card) do + transient :image_remote_url + url { Faker::Internet.url } title { Faker::Lorem.sentence } description { Faker::Lorem.paragraph } type 'link' image { attachment_fixture('attachment.jpg') } + + after_build { |preview_card, transients| preview_card[:image_remote_url] = transients[:image_remote_url] } end diff --git a/spec/services/fetch_link_card_service_spec.rb b/spec/services/fetch_link_card_service_spec.rb index 1d61e33c0b..ae276c081c 100644 --- a/spec/services/fetch_link_card_service_spec.rb +++ b/spec/services/fetch_link_card_service_spec.rb @@ -7,8 +7,16 @@ RSpec.describe FetchLinkCardService do let(:html) { 'Hello world' } let(:oembed_cache) { nil } + let(:preview_card) { nil } before do + if preview_card + allow(preview_card).to receive(:download_image!).with(anything).and_call_original + + allow(PreviewCard).to receive(:find_by).with(any_args).and_return(nil) + allow(PreviewCard).to receive(:find_by).with(url: preview_card.url).and_return(preview_card) + end + stub_request(:get, 'http://example.com/html').to_return(headers: { 'Content-Type' => 'text/html' }, body: html) stub_request(:get, 'http://example.com/not-found').to_return(status: 404, headers: { 'Content-Type' => 'text/html' }, body: html) stub_request(:get, 'http://example.com/text').to_return(status: 404, headers: { 'Content-Type' => 'text/plain' }, body: 'Hello') @@ -34,6 +42,8 @@ RSpec.describe FetchLinkCardService do stub_request(:get, 'http://example.com/long_canonical_url').to_return(request_fixture('long_canonical_url.txt')) stub_request(:get, 'http://example.com/alternative_utf8_spelling_in_header').to_return(request_fixture('alternative_utf8_spelling_in_header.txt')) + stub_request(:get, 'http://example.com/image.jpeg').to_return(request_fixture('attachment1.txt')) + Rails.cache.write('oembed_endpoint:example.com', oembed_cache) if oembed_cache subject.call(status) @@ -89,6 +99,7 @@ RSpec.describe FetchLinkCardService do it 'creates preview card' do expect(status.preview_card).to_not be_nil + expect(status.preview_card.original_url).to eq 'http://example.com/redirect' expect(status.preview_card.url).to eq 'http://example.com/html' expect(status.preview_card.title).to eq 'Hello world' end @@ -235,6 +246,18 @@ RSpec.describe FetchLinkCardService do end end + context 'with URL of a page with an og:image tag' do + let(:status) { Fabricate(:status, text: 'http://example.com/html') } + let(:html) { 'Hello world' } + + it 'creates preview card with image' do + expect(status.preview_card.image_remote_url).to eq 'http://example.com/image.jpeg' + expect(status.preview_card.image).to_not be_nil + expect(status.preview_card.width).to eq 1136 + expect(status.preview_card.height).to eq 852 + end + end + context 'with a URL of a page with oEmbed support' do let(:html) { 'Hello world' } let(:status) { Fabricate(:status, text: 'http://example.com/html') } @@ -321,4 +344,75 @@ RSpec.describe FetchLinkCardService do expect(a_request(:get, 'https://quitter.se/tag/wannacry')).to_not have_been_made end end + + context 'when preview card already exists' do + let(:updated_at) { 1.hour.ago.beginning_of_minute } + let(:image_url) { 'http://example.com/image.jpeg' } + let(:preview_card) { Fabricate(:preview_card, url: 'http://example.com/html', title: 'Old news', updated_at: updated_at, image_remote_url: image_url, image: image_url && attachment_fixture('attachment.jpg')) } + let(:status) { Fabricate(:status, text: 'http://example.com/html') } + let(:html) { 'Hello world' } + + context 'when card is recently updated' do + it 'leaves existing card unchanged' do + expect(status.preview_card).to eq preview_card + expect(status.preview_card.updated_at).to eq updated_at + expect(status.preview_card.url).to eq 'http://example.com/html' + expect(status.preview_card.title).to eq 'Old news' + expect(status.preview_card.image).to_not be_nil + expect(status.preview_card).to_not have_received(:download_image!) + end + end + + context 'when card is stale' do + let(:updated_at) { 3.weeks.ago } + + it 'updates existing card' do + expect(status.preview_card).to eq preview_card + expect(status.preview_card.updated_at).to_not eq updated_at + expect(status.preview_card.url).to eq 'http://example.com/html' + expect(status.preview_card.title).to eq 'Hello world' + expect(preview_card).to_not have_received(:download_image!) + end + + context 'when page has different image' do + let(:image_url) { 'http://example.com/old.jpeg' } + + it 'updates existing card' do + expect(status.preview_card).to eq preview_card + expect(status.preview_card.url).to eq 'http://example.com/html' + expect(status.preview_card.title).to eq 'Hello world' + expect(status.preview_card).to have_received(:download_image!) + expect(status.preview_card.image_remote_url).to eq 'http://example.com/image.jpeg' + expect(status.preview_card.width).to eq 1136 + expect(status.preview_card.height).to eq 852 + end + end + end + + context 'when card has no image' do + let(:image_url) { nil } + + it 'updates existing card' do + expect(status.preview_card).to eq preview_card + expect(status.preview_card.url).to eq 'http://example.com/html' + expect(status.preview_card.title).to eq 'Hello world' + expect(status.preview_card).to have_received(:download_image!) + expect(status.preview_card.image_remote_url).to eq 'http://example.com/image.jpeg' + expect(status.preview_card.width).to eq 1136 + expect(status.preview_card.height).to eq 852 + end + end + + context 'with a redirect URL' do + let(:status) { Fabricate(:status, text: 'http://example.com/redirect') } + + it 'updates existing card' do + expect(status.preview_card).to eq preview_card + expect(status.preview_card.original_url).to eq 'http://example.com/redirect' + expect(status.preview_card.url).to eq 'http://example.com/html' + expect(status.preview_card.title).to eq 'Hello world' + expect(preview_card).to_not have_received(:download_image!) + end + end + end end