From 03818e642a1ae871bffdc0c39c10f05f0b8b0398 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 5 May 2020 09:07:50 +0100 Subject: [PATCH] FEATURE: Include optimized thumbnails for topics (#9215) This introduces new APIs for obtaining optimized thumbnails for topics. There are a few building blocks required for this: - Introduces new `image_upload_id` columns on the `posts` and `topics` table. This replaces the old `image_url` column, which means that thumbnails are now restricted to uploads. Hotlinked thumbnails are no longer possible. In normal use (with pull_hotlinked_images enabled), this has no noticeable impact - A migration attempts to match existing urls to upload records. If a match cannot be found then the posts will be queued for rebake - Optimized thumbnails are generated during post_process_cooked. If thumbnails are missing when serializing a topic list, then a sidekiq job is queued - Topic lists and topics now include a `thumbnails` key, which includes all the available images: ``` "thumbnails": [ { "max_width": null, "max_height": null, "url": "//example.com/original-image.png", "width": 1380, "height": 1840 }, { "max_width": 1024, "max_height": 1024, "url": "//example.com/optimized-image.png", "width": 768, "height": 1024 } ] ``` - Themes can request additional thumbnail sizes by using a modifier in their `about.json` file: ``` "modifiers": { "topic_thumbnail_sizes": [ [200, 200], [800, 800] ], ... ``` Remember that these are generated asynchronously, so your theme should include logic to fallback to other available thumbnails if your requested size has not yet been generated - Two new raw plugin outlets are introduced, to improve the customisability of the topic list. `topic-list-before-columns` and `topic-list-before-link` --- .../app/templates/list/topic-list-item.hbr | 3 + .../templates/mobile/list/topic-list-item.hbr | 2 + app/jobs/regular/generate_topic_thumbnails.rb | 18 ++++ app/jobs/scheduled/ensure_db_consistency.rb | 3 +- app/models/post.rb | 8 ++ app/models/theme_modifier_set.rb | 35 +++++-- app/models/topic.rb | 82 ++++++++++++++++ app/models/topic_thumbnail.rb | 60 ++++++++++++ app/models/upload.rb | 1 + app/serializers/listable_topic_serializer.rb | 12 ++- app/serializers/topic_view_serializer.rb | 6 ++ ...9095034_add_topic_thumbnail_information.rb | 19 ++++ ...35_migrate_image_url_to_image_upload_id.rb | 95 +++++++++++++++++++ lib/cooked_post_processor.rb | 26 +++-- lib/search.rb | 2 +- lib/topic_query.rb | 6 ++ spec/components/cooked_post_processor_spec.rb | 43 ++++++--- spec/components/search_spec.rb | 4 +- spec/components/topic_view_spec.rb | 21 ++-- spec/fabricators/post_fabricator.rb | 2 +- spec/integration/topic_thumbnail_spec.rb | 88 +++++++++++++++++ spec/lib/db_helper_spec.rb | 9 -- spec/models/topic_thumbnail_spec.rb | 46 +++++++++ .../serializers/topic_view_serializer_spec.rb | 28 +++++- .../web_hook_topic_view_serializer_spec.rb | 1 + 25 files changed, 563 insertions(+), 57 deletions(-) create mode 100644 app/jobs/regular/generate_topic_thumbnails.rb create mode 100644 app/models/topic_thumbnail.rb create mode 100644 db/migrate/20200429095034_add_topic_thumbnail_information.rb create mode 100644 db/migrate/20200429095035_migrate_image_url_to_image_upload_id.rb create mode 100644 spec/integration/topic_thumbnail_spec.rb create mode 100644 spec/models/topic_thumbnail_spec.rb diff --git a/app/assets/javascripts/discourse/app/templates/list/topic-list-item.hbr b/app/assets/javascripts/discourse/app/templates/list/topic-list-item.hbr index ab05872ece0..03ea153bb09 100644 --- a/app/assets/javascripts/discourse/app/templates/list/topic-list-item.hbr +++ b/app/assets/javascripts/discourse/app/templates/list/topic-list-item.hbr @@ -1,3 +1,5 @@ +{{~raw-plugin-outlet name="topic-list-before-columns"}} + {{#if bulkSelectEnabled}} @@ -12,6 +14,7 @@ at the end of the link, preventing it from line wrapping onto its own line. --}} + {{~raw-plugin-outlet name="topic-list-before-link"}} {{~raw-plugin-outlet name="topic-list-before-status"}} {{~raw "topic-status" topic=topic}} diff --git a/app/assets/javascripts/discourse/app/templates/mobile/list/topic-list-item.hbr b/app/assets/javascripts/discourse/app/templates/mobile/list/topic-list-item.hbr index c126de09242..6c4663b83db 100644 --- a/app/assets/javascripts/discourse/app/templates/mobile/list/topic-list-item.hbr +++ b/app/assets/javascripts/discourse/app/templates/mobile/list/topic-list-item.hbr @@ -1,4 +1,5 @@ + {{~raw-plugin-outlet name="topic-list-before-columns"}} {{~#unless expandPinned}}
{{avatar topic.lastPosterUser imageSize="large"}} @@ -14,6 +15,7 @@ This causes the topic-post-badge to be considered the same word as "text" at the end of the link, preventing it from line wrapping onto its own line. --}} + {{~raw-plugin-outlet name="topic-list-before-link"}}
{{~raw-plugin-outlet name="topic-list-before-status"}} {{~raw "topic-status" topic=topic~}} diff --git a/app/jobs/regular/generate_topic_thumbnails.rb b/app/jobs/regular/generate_topic_thumbnails.rb new file mode 100644 index 00000000000..a01452670c7 --- /dev/null +++ b/app/jobs/regular/generate_topic_thumbnails.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Jobs + class GenerateTopicThumbnails < ::Jobs::Base + sidekiq_options queue: 'ultra_low' + + def execute(args) + topic_id = args[:topic_id] + extra_sizes = args[:extra_sizes] + + raise Discourse::InvalidParameters.new(:topic_id) if topic_id.blank? + + topic = Topic.find(topic_id) + topic.generate_thumbnails!(extra_sizes: extra_sizes) + end + + end +end diff --git a/app/jobs/scheduled/ensure_db_consistency.rb b/app/jobs/scheduled/ensure_db_consistency.rb index 6d7f3fbbb8a..6c21814fa01 100644 --- a/app/jobs/scheduled/ensure_db_consistency.rb +++ b/app/jobs/scheduled/ensure_db_consistency.rb @@ -22,7 +22,8 @@ module Jobs CategoryTagStat, User, UserAvatar, - Category + Category, + TopicThumbnail ].each do |klass| klass.ensure_consistency! measure(klass) diff --git a/app/models/post.rb b/app/models/post.rb index f2ee10543f3..c2f80c8c6fc 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -51,6 +51,8 @@ class Post < ActiveRecord::Base has_many :user_actions, foreign_key: :target_post_id + belongs_to :image_upload, class_name: "Upload" + validates_with PostValidator, unless: :skip_validation after_save :index_search @@ -1062,6 +1064,10 @@ class Post < ActiveRecord::Base Upload.where(access_control_post_id: self.id) end + def image_url + image_upload&.url + end + private def parse_quote_into_arguments(quote) @@ -1144,6 +1150,7 @@ end # action_code :string # image_url :string # locked_by_id :integer +# image_upload_id :bigint # # Indexes # @@ -1152,6 +1159,7 @@ end # idx_posts_user_id_deleted_at (user_id) WHERE (deleted_at IS NULL) # index_for_rebake_old (id) WHERE (((baked_version IS NULL) OR (baked_version < 2)) AND (deleted_at IS NULL)) # index_posts_on_id_and_baked_version (id DESC,baked_version) WHERE (deleted_at IS NULL) +# index_posts_on_image_upload_id (image_upload_id) # index_posts_on_reply_to_post_number (reply_to_post_number) # index_posts_on_topic_id_and_percent_rank (topic_id,percent_rank) # index_posts_on_topic_id_and_post_number (topic_id,post_number) UNIQUE diff --git a/app/models/theme_modifier_set.rb b/app/models/theme_modifier_set.rb index 99a8818f58c..9fae7faf54a 100644 --- a/app/models/theme_modifier_set.rb +++ b/app/models/theme_modifier_set.rb @@ -12,7 +12,7 @@ class ThemeModifierSet < ActiveRecord::Base def type_validator ThemeModifierSet.modifiers.each do |k, config| - value = public_send(k) + value = read_attribute(k) next if value.nil? case config[:type] @@ -39,7 +39,7 @@ class ThemeModifierSet < ActiveRecord::Base def self.resolve_modifier_for_themes(theme_ids, modifier_name) return nil if !(config = self.modifiers[modifier_name]) - all_values = self.where(theme_id: theme_ids).where.not(modifier_name => nil).pluck(modifier_name) + all_values = self.where(theme_id: theme_ids).where.not(modifier_name => nil).map { |s| s.public_send(modifier_name) } case config[:type] when :boolean all_values.any? @@ -50,6 +50,26 @@ class ThemeModifierSet < ActiveRecord::Base end end + def topic_thumbnail_sizes + array = read_attribute(:topic_thumbnail_sizes) + + return if array.nil? + + array.map do |dimension| + parts = dimension.split("x") + next if parts.length != 2 + [parts[0].to_i, parts[1].to_i] + end.filter(&:present?) + end + + def topic_thumbnail_sizes=(val) + return write_attribute(:topic_thumbnail_sizes, val) if val.nil? + return write_attribute(:topic_thumbnail_sizes, val) if !val.is_a?(Array) + return write_attribute(:topic_thumbnail_sizes, val) if !val.all? { |v| v.is_a?(Array) && v.length == 2 } + + super(val.map { |dim| "#{dim[0]}x#{dim[1]}" }) + end + private # Build the list of modifiers from the DB schema. @@ -78,11 +98,12 @@ end # # Table name: theme_modifier_sets # -# id :bigint not null, primary key -# theme_id :bigint not null -# serialize_topic_excerpts :boolean -# csp_extensions :string is an Array -# svg_icons :string is an Array +# id :bigint not null, primary key +# theme_id :bigint not null +# serialize_topic_excerpts :boolean +# csp_extensions :string is an Array +# svg_icons :string is an Array +# topic_thumbnail_sizes :string is an Array # # Indexes # diff --git a/app/models/topic.rb b/app/models/topic.rb index aaeca3f1453..371f32d5702 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -28,6 +28,83 @@ class Topic < ActiveRecord::Base 400 end + def self.share_thumbnail_size + [1024, 1024] + end + + def self.thumbnail_sizes + [ self.share_thumbnail_size ] + end + + def thumbnail_job_redis_key(extra_sizes) + "generate_topic_thumbnail_enqueue_#{id}_#{extra_sizes.inspect}" + end + + def filtered_topic_thumbnails(extra_sizes: []) + return nil unless original = image_upload + return nil unless original.width && original.height + + thumbnail_sizes = Topic.thumbnail_sizes + extra_sizes + topic_thumbnails.filter { |record| thumbnail_sizes.include?([record.max_width, record.max_height]) } + end + + def thumbnail_info(enqueue_if_missing: false, extra_sizes: []) + return nil unless original = image_upload + return nil unless original.width && original.height + + infos = [] + infos << { # Always add original + max_width: nil, + max_height: nil, + width: original.width, + height: original.height, + url: original.url + } + + records = filtered_topic_thumbnails(extra_sizes: extra_sizes) + + records.each do |record| + next unless record.optimized_image # Only serialize successful thumbnails + + infos << { + max_width: record.max_width, + max_height: record.max_height, + width: record.optimized_image&.width, + height: record.optimized_image&.height, + url: record.optimized_image&.url + } + end + + thumbnail_sizes = Topic.thumbnail_sizes + extra_sizes + if SiteSetting.create_thumbnails && + enqueue_if_missing && + records.length < thumbnail_sizes.length && + Discourse.redis.set(thumbnail_job_redis_key(extra_sizes), 1, nx: true, ex: 1.minute) + + Jobs.enqueue(:generate_topic_thumbnails, { topic_id: id, extra_sizes: extra_sizes }) + end + + infos.sort_by! { |i| -i[:width] * i[:height] } + end + + def generate_thumbnails!(extra_sizes: []) + return nil unless SiteSetting.create_thumbnails + return nil unless original = image_upload + return nil unless original.width && original.height + + (Topic.thumbnail_sizes + extra_sizes).each do |dim| + TopicThumbnail.find_or_create_for!(original, max_width: dim[0], max_height: dim[1]) + end + end + + def image_url + thumbnail = topic_thumbnails.detect do |record| + record.max_width == Topic.share_thumbnail_size[0] && + record.max_height == Topic.share_thumbnail_size[1] + end + thumbnail&.optimized_image&.url || image_upload&.url + end + def featured_users @featured_users ||= TopicFeaturedUsers.new(self) end @@ -139,6 +216,9 @@ class Topic < ActiveRecord::Base has_one :topic_search_data has_one :topic_embed, dependent: :destroy + belongs_to :image_upload, class_name: 'Upload' + has_many :topic_thumbnails, through: :image_upload + # When we want to temporarily attach some data to a forum topic (usually before serialization) attr_accessor :user_data attr_accessor :category_user_data @@ -1601,6 +1681,7 @@ end # highest_staff_post_number :integer default(0), not null # featured_link :string # reviewable_score :float default(0.0), not null +# image_upload_id :bigint # # Indexes # @@ -1611,6 +1692,7 @@ end # index_topics_on_created_at_and_visible (created_at,visible) WHERE ((deleted_at IS NULL) AND ((archetype)::text <> 'private_message'::text)) # index_topics_on_id_and_deleted_at (id,deleted_at) # index_topics_on_id_filtered_banner (id) UNIQUE WHERE (((archetype)::text = 'banner'::text) AND (deleted_at IS NULL)) +# index_topics_on_image_upload_id (image_upload_id) # index_topics_on_lower_title (lower((title)::text)) # index_topics_on_pinned_at (pinned_at) WHERE (pinned_at IS NOT NULL) # index_topics_on_pinned_globally (pinned_globally) WHERE pinned_globally diff --git a/app/models/topic_thumbnail.rb b/app/models/topic_thumbnail.rb new file mode 100644 index 00000000000..139ddde2321 --- /dev/null +++ b/app/models/topic_thumbnail.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +# This model indicates an 'attempt' to create a topic thumbnail +# for an upload. This means we don't keep trying to create optimized +# images for small/invalid original images. +# +# Foreign keys with ON DELETE CASCADE are used to ensure unneeded data +# is deleted automatically +class TopicThumbnail < ActiveRecord::Base + belongs_to :upload + belongs_to :optimized_image + + def self.find_or_create_for!(original, max_width: , max_height:) + existing = TopicThumbnail.find_by(upload: original, max_width: max_width, max_height: max_height) + return existing if existing + return nil if !SiteSetting.create_thumbnails? + + target_width, target_height = ImageSizer.resize(original.width, original.height, { max_width: max_width, max_height: max_height }) + + if target_width < original.width && target_height < original.height + optimized = OptimizedImage.create_for(original, target_width, target_height) + end + + create!(upload: original, max_width: max_width, max_height: max_height, optimized_image: optimized) + end + + def self.ensure_consistency! + # Clean up records for broken upload links or broken optimized image links + TopicThumbnail + .joins("LEFT JOIN uploads on upload_id = uploads.id") + .joins("LEFT JOIN optimized_images on optimized_image_id = optimized_images.id") + .where(<<~SQL) + (optimized_image_id IS NOT NULL AND optimized_images IS NULL) + OR uploads IS NULL + SQL + .delete_all + + # Delete records for sizes which are no longer needed + sizes = Topic.thumbnail_sizes + ThemeModifierHelper.new(theme_ids: Theme.pluck(:id)).topic_thumbnail_sizes + sizes_sql = sizes.map { |s| "(max_width = #{s[0].to_i} AND max_height = #{s[1].to_i})" }.join(" OR ") + TopicThumbnail.where.not(sizes_sql).delete_all + end +end + +# == Schema Information +# +# Table name: topic_thumbnails +# +# id :bigint not null, primary key +# upload_id :bigint not null +# optimized_image_id :bigint +# max_width :integer not null +# max_height :integer not null +# +# Indexes +# +# index_topic_thumbnails_on_optimized_image_id (optimized_image_id) +# index_topic_thumbnails_on_upload_id (upload_id) +# unique_topic_thumbnails (upload_id,max_width,max_height) UNIQUE +# diff --git a/app/models/upload.rb b/app/models/upload.rb index cb4cf458afc..bb4e7976f5e 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -25,6 +25,7 @@ class Upload < ActiveRecord::Base has_many :optimized_images, dependent: :destroy has_many :user_uploads, dependent: :destroy + has_many :topic_thumbnails attr_accessor :for_group_message attr_accessor :for_theme diff --git a/app/serializers/listable_topic_serializer.rb b/app/serializers/listable_topic_serializer.rb index a00cd2f619d..52d7fb27157 100644 --- a/app/serializers/listable_topic_serializer.rb +++ b/app/serializers/listable_topic_serializer.rb @@ -25,10 +25,20 @@ class ListableTopicSerializer < BasicTopicSerializer :bookmarked, :liked, :unicode_title, - :unread_by_group_member + :unread_by_group_member, + :thumbnails has_one :last_poster, serializer: BasicUserSerializer, embed: :objects + def image_url + object.image_url + end + + def thumbnails + extra_sizes = ThemeModifierHelper.new(request: scope.request).topic_thumbnail_sizes + object.thumbnail_info(enqueue_if_missing: true, extra_sizes: extra_sizes) + end + def include_unicode_title? object.title.match?(/:[\w\-+]+:/) end diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb index e40dca1d9fb..27cc97523ed 100644 --- a/app/serializers/topic_view_serializer.rb +++ b/app/serializers/topic_view_serializer.rb @@ -73,6 +73,7 @@ class TopicViewSerializer < ApplicationSerializer :queued_posts_count, :show_read_indicator, :requested_group_name, + :thumbnails ) has_one :details, serializer: TopicViewDetailsSerializer, root: false, embed: :objects @@ -284,4 +285,9 @@ class TopicViewSerializer < ApplicationSerializer def include_published_page? SiteSetting.enable_page_publishing? && scope.is_staff? && object.published_page.present? end + + def thumbnails + extra_sizes = ThemeModifierHelper.new(request: scope.request).topic_thumbnail_sizes + object.topic.thumbnail_info(enqueue_if_missing: true, extra_sizes: extra_sizes) + end end diff --git a/db/migrate/20200429095034_add_topic_thumbnail_information.rb b/db/migrate/20200429095034_add_topic_thumbnail_information.rb new file mode 100644 index 00000000000..be34f93c32e --- /dev/null +++ b/db/migrate/20200429095034_add_topic_thumbnail_information.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddTopicThumbnailInformation < ActiveRecord::Migration[6.0] + def change + add_reference :posts, :image_upload + add_reference :topics, :image_upload + + add_column :theme_modifier_sets, :topic_thumbnail_sizes, :string, array: true + + create_table :topic_thumbnails do |t| + t.references :upload, null: false + t.references :optimized_image, null: true + t.integer :max_width, null: false + t.integer :max_height, null: false + end + + add_index :topic_thumbnails, [:upload_id, :max_width, :max_height], name: :unique_topic_thumbnails, unique: true + end +end diff --git a/db/migrate/20200429095035_migrate_image_url_to_image_upload_id.rb b/db/migrate/20200429095035_migrate_image_url_to_image_upload_id.rb new file mode 100644 index 00000000000..5f4747b2c3c --- /dev/null +++ b/db/migrate/20200429095035_migrate_image_url_to_image_upload_id.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +class MigrateImageUrlToImageUploadId < ActiveRecord::Migration[6.0] + disable_ddl_transaction! # Avoid holding update locks on posts for the whole migration + + BATCH_SIZE = 1000 + + def up + # Defining regex here to avoid needing to double-escape the \ characters + regex = '\/(original|optimized)\/\dX[\/\.\w]*\/([a-zA-Z0-9]+)[\.\w]*' + + execute <<~SQL + CREATE TEMPORARY TABLE tmp_post_image_uploads( + post_id int primary key, + upload_id int + ) + SQL + + # Look for an SHA1 in the existing image_url, and match to the uploads table + execute <<~SQL + INSERT INTO tmp_post_image_uploads(post_id, upload_id) + SELECT + posts.id as post_id, + uploads.id as upload_id + FROM posts + LEFT JOIN LATERAL regexp_matches(posts.image_url, '#{regex}') matched_sha1 ON TRUE + LEFT JOIN uploads on uploads.sha1 = matched_sha1[2] + WHERE posts.image_url IS NOT NULL + AND uploads.id IS NOT NULL + ORDER BY posts.id ASC + SQL + + # Update the posts table to match the temp table data + last_update_id = -1 + begin + result = DB.query <<~SQL + WITH to_update AS ( + SELECT post_id, upload_id + FROM tmp_post_image_uploads + JOIN posts ON posts.id = post_id + WHERE posts.id > #{last_update_id} + ORDER BY post_id ASC + LIMIT #{BATCH_SIZE} + ) + UPDATE posts SET image_upload_id = to_update.upload_id + FROM to_update + WHERE to_update.post_id = posts.id + RETURNING posts.id + SQL + last_update_id = result.last&.id + end while last_update_id + + # Update the topic image based on the first post image + last_update_id = -1 + begin + result = DB.query <<~SQL + WITH to_update AS ( + SELECT topic_id, posts.image_upload_id as upload_id + FROM topics + JOIN posts ON post_number = 1 AND posts.topic_id = topics.id + WHERE posts.image_upload_id IS NOT NULL + AND topics.id > #{last_update_id} + ORDER BY topics.id ASC + LIMIT #{BATCH_SIZE} + ) + UPDATE topics SET image_upload_id = to_update.upload_id + FROM to_update + WHERE topics.id = to_update.topic_id + RETURNING topics.id + SQL + last_update_id = result.last&.id + end while last_update_id + + # For posts we couldn't figure out, mark them for background rebake + last_update_id = -1 + begin + updated_count = DB.query <<~SQL + WITH to_update AS ( + SELECT id as post_id + FROM posts + WHERE posts.image_url IS NOT NULL + AND posts.image_upload_id IS NULL + AND posts.id > #{last_update_id} + ORDER BY posts.id ASC + LIMIT #{BATCH_SIZE} + ) + UPDATE posts SET baked_version = NULL + FROM to_update + WHERE posts.id = to_update.post_id + RETURNING posts.id + SQL + last_update_id = result.last&.id + end while last_update_id + end +end diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index 725258cc10d..d2e8f4d2d50 100644 --- a/lib/cooked_post_processor.rb +++ b/lib/cooked_post_processor.rb @@ -490,16 +490,26 @@ class CookedPostProcessor end def update_post_image - img = extract_images_for_post.first - if img.blank? - @post.update_column(:image_url, nil) if @post.image_url - @post.topic.update_column(:image_url, nil) if @post.topic.image_url && @post.is_first_post? - return + upload = nil + eligible_image_fragments = extract_images_for_post + + # Loop through those fragments until we find one with an upload record + @post.each_upload_url(fragments: eligible_image_fragments) do |src, path, sha1| + upload = Upload.find_by(sha1: sha1) + break if upload end - if img["src"].present? - @post.update_column(:image_url, img["src"][0...255]) # post - @post.topic.update_column(:image_url, img["src"][0...255]) if @post.is_first_post? # topic + if upload.present? + @post.update_column(:image_upload_id, upload.id) # post + if @post.is_first_post? # topic + @post.topic.update_column(:image_upload_id, upload.id) + extra_sizes = ThemeModifierHelper.new(theme_ids: Theme.user_selectable.pluck(:id)).topic_thumbnail_sizes + @post.topic.generate_thumbnails!(extra_sizes: extra_sizes) + end + else + @post.update_column(:image_upload_id, nil) if @post.image_upload_id + @post.topic.update_column(:image_upload_id, nil) if @post.topic.image_upload_id && @post.is_first_post? + nil end end diff --git a/lib/search.rb b/lib/search.rb index e1c6509c423..ffe08884eaa 100644 --- a/lib/search.rb +++ b/lib/search.rb @@ -434,7 +434,7 @@ class Search end advanced_filter(/^with:images$/) do |posts| - posts.where("posts.image_url IS NOT NULL") + posts.where("posts.image_upload_id IS NOT NULL") end advanced_filter(/^category:(.+)$/) do |posts, match| diff --git a/lib/topic_query.rb b/lib/topic_query.rb index 35bc7347feb..772ea5af28b 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -828,6 +828,8 @@ class TopicQuery result = result.where('topics.posts_count <= ?', options[:max_posts]) if options[:max_posts].present? result = result.where('topics.posts_count >= ?', options[:min_posts]) if options[:min_posts].present? + result = preload_thumbnails(result) + result = TopicQuery.apply_custom_filters(result, self) @guardian.filter_allowed_categories(result) @@ -1050,6 +1052,10 @@ class TopicQuery result.order('topics.bumped_at DESC') end + def preload_thumbnails(result) + result.preload(:image_upload, topic_thumbnails: :optimized_image) + end + private def sanitize_sql_array(input) diff --git a/spec/components/cooked_post_processor_spec.rb b/spec/components/cooked_post_processor_spec.rb index 51e9eadf69f..a429d64bee7 100644 --- a/spec/components/cooked_post_processor_spec.rb +++ b/spec/components/cooked_post_processor_spec.rb @@ -751,16 +751,16 @@ describe CookedPostProcessor do end context "topic image" do - let(:post) { Fabricate(:post_with_uploaded_image) } + fab!(:post) { Fabricate(:post_with_uploaded_image) } let(:cpp) { CookedPostProcessor.new(post) } it "adds a topic image if there's one in the first post" do FastImage.stubs(:size) - expect(post.topic.image_url).to eq(nil) + expect(post.topic.image_upload_id).to eq(nil) cpp.post_process post.topic.reload - expect(post.topic.image_url).to be_present + expect(post.topic.image_upload_id).to be_present end it "removes image if post is edited and no longer has an image" do @@ -768,14 +768,14 @@ describe CookedPostProcessor do cpp.post_process post.topic.reload - expect(post.topic.image_url).to be_present - expect(post.image_url).to be_present + expect(post.topic.image_upload_id).to be_present + expect(post.image_upload_id).to be_present post.update!(raw: "This post no longer has an image.") CookedPostProcessor.new(post).post_process post.topic.reload - expect(post.topic.image_url).not_to be_present - expect(post.image_url).not_to be_present + expect(post.topic.image_upload_id).not_to be_present + expect(post.image_upload_id).not_to be_present end it "won't remove the original image if another post doesn't have an image" do @@ -784,15 +784,32 @@ describe CookedPostProcessor do cpp.post_process topic.reload - expect(topic.image_url).to be_present - expect(post.image_url).to be_present + expect(topic.image_upload_id).to be_present + expect(post.image_upload_id).to be_present post = Fabricate(:post, topic: topic, raw: "this post doesn't have an image") CookedPostProcessor.new(post).post_process topic.reload - expect(post.topic.image_url).to be_present - expect(post.image_url).to be_blank + expect(post.topic.image_upload_id).to be_present + expect(post.image_upload_id).to be_blank + end + + it "generates thumbnails correctly" do + FastImage.expects(:size).returns([1750, 2000]) + + topic = post.topic + cpp.post_process + topic.reload + expect(topic.image_upload_id).to be_present + expect(post.image_upload_id).to be_present + + post = Fabricate(:post, topic: topic, raw: "this post doesn't have an image") + CookedPostProcessor.new(post).post_process + topic.reload + + expect(post.topic.image_upload_id).to be_present + expect(post.image_upload_id).to be_blank end end @@ -802,10 +819,10 @@ describe CookedPostProcessor do it "adds a post image if there's one in the post" do FastImage.stubs(:size) - expect(reply.image_url).to eq(nil) + expect(reply.image_upload_id).to eq(nil) cpp.post_process reply.reload - expect(reply.image_url).to be_present + expect(reply.image_upload_id).to be_present end end end diff --git a/spec/components/search_spec.rb b/spec/components/search_spec.rb index 64e0efb726c..a455d81ec13 100644 --- a/spec/components/search_spec.rb +++ b/spec/components/search_spec.rb @@ -1134,13 +1134,11 @@ describe Search do it 'can find posts with images' do post_uploaded = Fabricate(:post_with_uploaded_image) - post_with_image_urls = Fabricate(:post_with_image_urls) Fabricate(:post) CookedPostProcessor.new(post_uploaded).update_post_image - CookedPostProcessor.new(post_with_image_urls).update_post_image - expect(Search.execute('with:images').posts.map(&:id)).to contain_exactly(post_uploaded.id, post_with_image_urls.id) + expect(Search.execute('with:images').posts.map(&:id)).to contain_exactly(post_uploaded.id) end it 'can find by latest' do diff --git a/spec/components/topic_view_spec.rb b/spec/components/topic_view_spec.rb index 16ee55dab0e..73bb74ac256 100644 --- a/spec/components/topic_view_spec.rb +++ b/spec/components/topic_view_spec.rb @@ -708,9 +708,12 @@ describe TopicView do end describe '#image_url' do - let!(:post1) { Fabricate(:post, topic: topic) } - let!(:post2) { Fabricate(:post, topic: topic) } - let!(:post3) { Fabricate(:post, topic: topic).tap { |p| p.update_column(:image_url, "post3_image.png") }.reload } + fab!(:op_upload) { Fabricate(:image_upload) } + fab!(:post3_upload) { Fabricate(:image_upload) } + + fab!(:post1) { Fabricate(:post, topic: topic) } + fab!(:post2) { Fabricate(:post, topic: topic) } + fab!(:post3) { Fabricate(:post, topic: topic).tap { |p| p.update_column(:image_upload_id, post3_upload.id) }.reload } def topic_view_for_post(post_number) TopicView.new(topic.id, evil_trout, post_number: post_number) @@ -718,14 +721,14 @@ describe TopicView do context "when op has an image" do before do - topic.update_column(:image_url, "op_image.png") - post1.update_column(:image_url, "op_image.png") + topic.update_column(:image_upload_id, op_upload.id) + post1.update_column(:image_upload_id, op_upload.id) end it "uses the topic image as a fallback when posts have no image" do - expect(topic_view_for_post(1).image_url).to eq("op_image.png") - expect(topic_view_for_post(2).image_url).to eq("op_image.png") - expect(topic_view_for_post(3).image_url).to eq("post3_image.png") + expect(topic_view_for_post(1).image_url).to eq(op_upload.url) + expect(topic_view_for_post(2).image_url).to eq(op_upload.url) + expect(topic_view_for_post(3).image_url).to eq(post3_upload.url) end end @@ -733,7 +736,7 @@ describe TopicView do it "returns nil when posts have no image" do expect(topic_view_for_post(1).image_url).to eq(nil) expect(topic_view_for_post(2).image_url).to eq(nil) - expect(topic_view_for_post(3).image_url).to eq("post3_image.png") + expect(topic_view_for_post(3).image_url).to eq(post3_upload.url) end end end diff --git a/spec/fabricators/post_fabricator.rb b/spec/fabricators/post_fabricator.rb index 412504c7c4e..42235cb94fc 100644 --- a/spec/fabricators/post_fabricator.rb +++ b/spec/fabricators/post_fabricator.rb @@ -56,7 +56,7 @@ HTML end Fabricator(:post_with_uploaded_image, from: :post) do - raw "" + raw { "" } end Fabricator(:post_with_an_attachment, from: :post) do diff --git a/spec/integration/topic_thumbnail_spec.rb b/spec/integration/topic_thumbnail_spec.rb new file mode 100644 index 00000000000..b288fb6c488 --- /dev/null +++ b/spec/integration/topic_thumbnail_spec.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true +require 'rails_helper' + +describe "Topic Thumbnails" do + before { SiteSetting.create_thumbnails = true } + + fab!(:image) { Fabricate(:image_upload, width: 5000, height: 5000) } + fab!(:topic) { Fabricate(:topic, image_upload_id: image.id) } + fab!(:user) { Fabricate(:user) } + + context 'latest' do + def get_topic + Discourse.redis.del(topic.thumbnail_job_redis_key([])) + get '/latest.json' + response.parsed_body["topic_list"]["topics"][0] + end + + it "includes thumbnails" do + topic_json = nil + expect do + topic_json = get_topic + end.to change { Jobs::GenerateTopicThumbnails.jobs.size }.by(1) + + thumbnails = topic_json["thumbnails"] + + # Original only. Optimized not yet generated + expect(thumbnails.length).to eq(1) + + # Original + expect(thumbnails[0]["max_width"]).to eq(nil) + expect(thumbnails[0]["max_height"]).to eq(nil) + expect(thumbnails[0]["width"]).to eq(image.width) + expect(thumbnails[0]["height"]).to eq(image.height) + expect(thumbnails[0]["url"]).to eq(image.url) + + # Run the job + args = Jobs::GenerateTopicThumbnails.jobs.last["args"].first + Jobs::GenerateTopicThumbnails.new.execute(args.with_indifferent_access) + + # Re-request + expect do + topic_json = get_topic + end.to change { Jobs::GenerateTopicThumbnails.jobs.size }.by(0) + thumbnails = topic_json["thumbnails"] + + expect(thumbnails[1]["max_width"]).to eq(Topic.share_thumbnail_size[0]) + expect(thumbnails[1]["max_height"]).to eq(Topic.share_thumbnail_size[1]) + expect(thumbnails[1]["width"]).to eq(1024) + expect(thumbnails[1]["height"]).to eq(1024) + expect(thumbnails[1]["url"]).to include("/optimized/") + end + + context "with a theme" do + before do + theme = Fabricate(:theme) + theme.theme_modifier_set.topic_thumbnail_sizes = [ + [100, 100], + [200, 200], + [300, 300] + ] + theme.theme_modifier_set.save! + theme.set_default! + end + + it "includes the theme specified resolutions" do + topic_json = nil + + expect do + topic_json = get_topic + end.to change { Jobs::GenerateTopicThumbnails.jobs.size }.by(1) + + # Run the job + args = Jobs::GenerateTopicThumbnails.jobs.last["args"].first + Jobs::GenerateTopicThumbnails.new.execute(args.with_indifferent_access) + + # Request again + expect do + topic_json = get_topic + end.to change { Jobs::GenerateTopicThumbnails.jobs.size }.by(0) + + thumbnails = topic_json["thumbnails"] + + # Original + Optimized + 3 theme requests + expect(thumbnails.length).to eq(5) + end + end + end +end diff --git a/spec/lib/db_helper_spec.rb b/spec/lib/db_helper_spec.rb index b178cd1e193..c2074a0f80b 100644 --- a/spec/lib/db_helper_spec.rb +++ b/spec/lib/db_helper_spec.rb @@ -7,8 +7,6 @@ RSpec.describe DbHelper do it 'should remap columns properly' do post = Fabricate(:post, cooked: "this is a specialcode that I included") post_attributes = post.reload.attributes - post2 = Fabricate(:post, image_url: "/testing/specialcode") - post2_attributes = post2.reload.attributes badge = Fabricate(:badge, query: "specialcode") badge_attributes = badge.reload.attributes @@ -19,13 +17,6 @@ RSpec.describe DbHelper do expect(post.cooked).to include("codespecial") - post2.reload - - expect(post2.image_url).to eq("/testing/codespecial") - - expect(post2_attributes.except("image_url")) - .to eq(post2.attributes.except("image_url")) - badge.reload expect(badge.query).to eq("codespecial") diff --git a/spec/models/topic_thumbnail_spec.rb b/spec/models/topic_thumbnail_spec.rb new file mode 100644 index 00000000000..b9e1df75fd1 --- /dev/null +++ b/spec/models/topic_thumbnail_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true +require 'rails_helper' + +describe "TopicThumbnail" do + let(:upload1) { Fabricate(:image_upload, width: 5000, height: 5000) } + let(:topic) { Fabricate(:topic, image_upload: upload1) } + + before do + SiteSetting.create_thumbnails = true + topic.generate_thumbnails! + + TopicThumbnail.ensure_consistency! + topic.reload + + expect(topic.topic_thumbnails.length).to eq(1) + end + + it "cleans up deleted uploads" do + upload1.delete + + TopicThumbnail.ensure_consistency! + topic.reload + + expect(topic.topic_thumbnails.length).to eq(0) + end + + it "cleans up deleted optimized images" do + upload1.optimized_images.reload.delete_all + + TopicThumbnail.ensure_consistency! + topic.reload + + expect(topic.topic_thumbnails.length).to eq(0) + end + + it "cleans up unneeded sizes" do + expect(topic.topic_thumbnails.length).to eq(1) + topic.topic_thumbnails[0].update_column(:max_width, 999999) + + TopicThumbnail.ensure_consistency! + topic.reload + + expect(topic.topic_thumbnails.length).to eq(0) + end + +end diff --git a/spec/serializers/topic_view_serializer_spec.rb b/spec/serializers/topic_view_serializer_spec.rb index a80f884812f..0adfbfe08ae 100644 --- a/spec/serializers/topic_view_serializer_spec.rb +++ b/spec/serializers/topic_view_serializer_spec.rb @@ -46,15 +46,35 @@ describe TopicViewSerializer do end describe '#image_url' do - let(:image_url) { 'http://meta.discourse.org/images/welcome/discourse-edit-post-animated.gif' } + let(:image_upload) { Fabricate(:image_upload, width: 5000, height: 5000) } describe 'when a topic has an image' do - it 'should return the image url' do - topic.update!(image_url: image_url) + before { topic.update!(image_upload_id: image_upload.id) } + it 'should return the image url' do json = serialize_topic(topic, user) - expect(json[:image_url]).to eq(image_url) + expect(json[:image_url]).to eq(image_upload.url) + end + + it 'should have thumbnails' do + SiteSetting.create_thumbnails = true + + Discourse.redis.del(topic.thumbnail_job_redis_key([])) + json = nil + + expect do + json = serialize_topic(topic, user) + end.to change { Jobs::GenerateTopicThumbnails.jobs.size }.by(1) + + topic.generate_thumbnails! + + expect do + json = serialize_topic(topic, user) + end.to change { Jobs::GenerateTopicThumbnails.jobs.size }.by(0) + + # Original + Optimized + expect(json[:thumbnails].length).to eq(2) end end diff --git a/spec/serializers/web_hook_topic_view_serializer_spec.rb b/spec/serializers/web_hook_topic_view_serializer_spec.rb index f0499f985e0..df878fb7cba 100644 --- a/spec/serializers/web_hook_topic_view_serializer_spec.rb +++ b/spec/serializers/web_hook_topic_view_serializer_spec.rb @@ -50,6 +50,7 @@ RSpec.describe WebHookTopicViewSerializer do created_by last_poster tags + thumbnails } keys = serializer.as_json.keys