From d17fb7013116767fc5c7d5eef63218bd8c45b023 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 9 Mar 2022 09:06:17 +0100 Subject: [PATCH] Change how changes to media attachments are stored for edits (#17696) * Change how changes to media attachments are stored for edits Fix not being able to re-order media attachments * Fix not broadcasting updates when polls/media is changed through ActivityPub * Various fixes and improvements * Update app/models/report.rb Co-authored-by: Claire * Add tracking of media attachment description changes * Change poll in status edit to have a structure closer to the real one Co-authored-by: Claire --- app/chewy/statuses_index.rb | 2 +- app/helpers/statuses_helper.rb | 12 ++-- app/lib/feed_manager.rb | 2 +- app/lib/rss/serializer.rb | 2 +- app/models/report.rb | 16 ++++- app/models/status.rb | 63 +++++++++++-------- app/models/status_edit.rb | 41 +++++++++--- .../activitypub/note_serializer.rb | 6 +- .../rest/status_edit_serializer.rb | 10 ++- app/serializers/rest/status_serializer.rb | 2 +- .../process_status_update_service.rb | 19 +++--- app/services/fan_out_on_write_service.rb | 2 +- app/services/post_status_service.rb | 6 +- app/services/remove_status_service.rb | 2 +- app/services/update_status_service.rb | 21 ++----- app/views/admin/reports/_status.html.haml | 12 ++-- app/views/admin/reports/index.html.haml | 4 +- .../admin/trends/statuses/_status.html.haml | 2 +- app/views/disputes/strikes/show.html.haml | 2 +- .../notification_mailer/_status.html.haml | 4 +- app/views/statuses/_detailed_status.html.haml | 6 +- app/views/statuses/_og_image.html.haml | 2 +- app/views/statuses/_simple_status.html.haml | 6 +- ...rdered_media_attachment_ids_to_statuses.rb | 5 ++ ...ed_media_attachment_ids_to_status_edits.rb | 8 +++ ...a_attachments_changed_from_status_edits.rb | 7 +++ db/schema.rb | 7 ++- spec/models/report_spec.rb | 13 ++-- .../process_status_update_service_spec.rb | 17 ++--- spec/services/update_status_service_spec.rb | 14 ++--- 30 files changed, 190 insertions(+), 125 deletions(-) create mode 100644 db/migrate/20220302232632_add_ordered_media_attachment_ids_to_statuses.rb create mode 100644 db/migrate/20220303000827_add_ordered_media_attachment_ids_to_status_edits.rb create mode 100644 db/post_migrate/20220303203437_remove_media_attachments_changed_from_status_edits.rb diff --git a/app/chewy/statuses_index.rb b/app/chewy/statuses_index.rb index 1903c2ea30..65cbb6fcd0 100644 --- a/app/chewy/statuses_index.rb +++ b/app/chewy/statuses_index.rb @@ -57,7 +57,7 @@ class StatusesIndex < Chewy::Index field :id, type: 'long' field :account_id, type: 'long' - field :text, type: 'text', value: ->(status) { [status.spoiler_text, Formatter.instance.plaintext(status)].concat(status.media_attachments.map(&:description)).concat(status.preloadable_poll ? status.preloadable_poll.options : []).join("\n\n") } do + field :text, type: 'text', value: ->(status) { [status.spoiler_text, Formatter.instance.plaintext(status)].concat(status.ordered_media_attachments.map(&:description)).concat(status.preloadable_poll ? status.preloadable_poll.options : []).join("\n\n") } do field :stemmed, type: 'text', analyzer: 'content' end diff --git a/app/helpers/statuses_helper.rb b/app/helpers/statuses_helper.rb index 25f079e9df..d328f89b7a 100644 --- a/app/helpers/statuses_helper.rb +++ b/app/helpers/statuses_helper.rb @@ -132,7 +132,7 @@ module StatusesHelper end def render_video_component(status, **options) - video = status.media_attachments.first + video = status.ordered_media_attachments.first meta = video.file.meta || {} @@ -150,12 +150,12 @@ module StatusesHelper }.merge(**options) react_component :video, component_params do - render partial: 'statuses/attachment_list', locals: { attachments: status.media_attachments } + render partial: 'statuses/attachment_list', locals: { attachments: status.ordered_media_attachments } end end def render_audio_component(status, **options) - audio = status.media_attachments.first + audio = status.ordered_media_attachments.first meta = audio.file.meta || {} @@ -170,7 +170,7 @@ module StatusesHelper }.merge(**options) react_component :audio, component_params do - render partial: 'statuses/attachment_list', locals: { attachments: status.media_attachments } + render partial: 'statuses/attachment_list', locals: { attachments: status.ordered_media_attachments } end end @@ -178,11 +178,11 @@ module StatusesHelper component_params = { sensitive: sensitized?(status, current_account), autoplay: prefers_autoplay?, - media: status.media_attachments.map { |a| ActiveModelSerializers::SerializableResource.new(a, serializer: REST::MediaAttachmentSerializer).as_json }, + media: status.ordered_media_attachments.map { |a| ActiveModelSerializers::SerializableResource.new(a, serializer: REST::MediaAttachmentSerializer).as_json }, }.merge(**options) react_component :media_gallery, component_params do - render partial: 'statuses/attachment_list', locals: { attachments: status.media_attachments } + render partial: 'statuses/attachment_list', locals: { attachments: status.ordered_media_attachments } end end diff --git a/app/lib/feed_manager.rb b/app/lib/feed_manager.rb index fc35292f2a..46a55c7a43 100644 --- a/app/lib/feed_manager.rb +++ b/app/lib/feed_manager.rb @@ -448,7 +448,7 @@ class FeedManager Formatter.instance.plaintext(status), status.spoiler_text, status.preloadable_poll ? status.preloadable_poll.options.join("\n\n") : nil, - status.media_attachments.map(&:description).join("\n\n"), + status.ordered_media_attachments.map(&:description).join("\n\n"), ].compact.join("\n\n") combined_regex.match?(combined_text) diff --git a/app/lib/rss/serializer.rb b/app/lib/rss/serializer.rb index fd56c568c7..7e3ed1f178 100644 --- a/app/lib/rss/serializer.rb +++ b/app/lib/rss/serializer.rb @@ -11,7 +11,7 @@ class RSS::Serializer .pub_date(status.created_at) .description(status.spoiler_text.presence || Formatter.instance.format(status, inline_poll_options: true).to_str) - status.media_attachments.each do |media| + status.ordered_media_attachments.each do |media| item.enclosure(full_asset_url(media.file.url(:original, false)), media.file.content_type, media.file.size) end end diff --git a/app/models/report.rb b/app/models/report.rb index 8ba2dd8fd2..6d41665401 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -63,8 +63,20 @@ class Report < ApplicationRecord Status.with_discarded.where(id: status_ids) end - def media_attachments - MediaAttachment.where(status_id: status_ids) + def media_attachments_count + statuses_to_query = [] + count = 0 + + statuses.pluck(:id, :ordered_media_attachment_ids).each do |id, ordered_ids| + if ordered_ids.nil? + statuses_to_query << id + else + count += ordered_ids.size + end + end + + count += MediaAttachment.where(status_id: statuses_to_query).count unless statuses_to_query.empty? + count end def rules diff --git a/app/models/status.rb b/app/models/status.rb index af3e645dc3..db10eedc23 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -3,28 +3,29 @@ # # Table name: statuses # -# id :bigint(8) not null, primary key -# uri :string -# text :text default(""), not null -# created_at :datetime not null -# updated_at :datetime not null -# in_reply_to_id :bigint(8) -# reblog_of_id :bigint(8) -# url :string -# sensitive :boolean default(FALSE), not null -# visibility :integer default("public"), not null -# spoiler_text :text default(""), not null -# reply :boolean default(FALSE), not null -# language :string -# conversation_id :bigint(8) -# local :boolean -# account_id :bigint(8) not null -# application_id :bigint(8) -# in_reply_to_account_id :bigint(8) -# poll_id :bigint(8) -# deleted_at :datetime -# edited_at :datetime -# trendable :boolean +# id :bigint(8) not null, primary key +# uri :string +# text :text default(""), not null +# created_at :datetime not null +# updated_at :datetime not null +# in_reply_to_id :bigint(8) +# reblog_of_id :bigint(8) +# url :string +# sensitive :boolean default(FALSE), not null +# visibility :integer default("public"), not null +# spoiler_text :text default(""), not null +# reply :boolean default(FALSE), not null +# language :string +# conversation_id :bigint(8) +# local :boolean +# account_id :bigint(8) not null +# application_id :bigint(8) +# in_reply_to_account_id :bigint(8) +# poll_id :bigint(8) +# deleted_at :datetime +# edited_at :datetime +# trendable :boolean +# ordered_media_attachment_ids :bigint(8) is an Array # class Status < ApplicationRecord @@ -211,11 +212,14 @@ class Status < ApplicationRecord public_visibility? || unlisted_visibility? end - def snapshot!(media_attachments_changed: false, account_id: nil, at_time: nil) + def snapshot!(account_id: nil, at_time: nil) edits.create!( text: text, spoiler_text: spoiler_text, - media_attachments_changed: media_attachments_changed, + sensitive: sensitive, + ordered_media_attachment_ids: ordered_media_attachment_ids || media_attachments.pluck(:id), + media_descriptions: ordered_media_attachments.map(&:description), + poll_options: preloadable_poll&.options, account_id: account_id || self.account_id, created_at: at_time || edited_at ) @@ -228,7 +232,7 @@ class Status < ApplicationRecord alias sign? distributable? def with_media? - media_attachments.any? + ordered_media_attachments.any? end def with_preview_card? @@ -252,6 +256,15 @@ class Status < ApplicationRecord @emojis = CustomEmoji.from_text(fields.join(' '), account.domain) end + def ordered_media_attachments + if ordered_media_attachment_ids.nil? + media_attachments + else + map = media_attachments.index_by(&:id) + ordered_media_attachment_ids.map { |media_attachment_id| map[media_attachment_id] } + end + end + def replies_count status_stat&.replies_count || 0 end diff --git a/app/models/status_edit.rb b/app/models/status_edit.rb index 6e88864e8c..94a387c362 100644 --- a/app/models/status_edit.rb +++ b/app/models/status_edit.rb @@ -3,17 +3,29 @@ # # Table name: status_edits # -# id :bigint(8) not null, primary key -# status_id :bigint(8) not null -# account_id :bigint(8) -# text :text default(""), not null -# spoiler_text :text default(""), not null -# media_attachments_changed :boolean default(FALSE), not null -# created_at :datetime not null -# updated_at :datetime not null +# id :bigint(8) not null, primary key +# status_id :bigint(8) not null +# account_id :bigint(8) +# text :text default(""), not null +# spoiler_text :text default(""), not null +# created_at :datetime not null +# updated_at :datetime not null +# ordered_media_attachment_ids :bigint(8) is an Array +# media_descriptions :text is an Array +# poll_options :string is an Array +# sensitive :boolean # class StatusEdit < ApplicationRecord + self.ignored_columns = %w( + media_attachments_changed + ) + + class PreservedMediaAttachment < ActiveModelSerializers::Model + attributes :media_attachment, :description + delegate :id, :type, :url, :preview_url, :remote_url, :preview_remote_url, :text_url, :meta, :blurhash, to: :media_attachment + end + belongs_to :status belongs_to :account, optional: true @@ -25,4 +37,17 @@ class StatusEdit < ApplicationRecord return @emojis if defined?(@emojis) @emojis = CustomEmoji.from_text([spoiler_text, text].join(' '), status.account.domain) end + + def ordered_media_attachments + return @ordered_media_attachments if defined?(@ordered_media_attachments) + + @ordered_media_attachments = begin + if ordered_media_attachment_ids.nil? + [] + else + map = status.media_attachments.index_by(&:id) + ordered_media_attachment_ids.map.with_index { |media_attachment_id, index| PreservedMediaAttachment.new(media_attachment: map[media_attachment_id], description: media_descriptions[index]) } + end + end + end end diff --git a/app/serializers/activitypub/note_serializer.rb b/app/serializers/activitypub/note_serializer.rb index 12dabc65a0..7be2e26476 100644 --- a/app/serializers/activitypub/note_serializer.rb +++ b/app/serializers/activitypub/note_serializer.rb @@ -13,7 +13,7 @@ class ActivityPub::NoteSerializer < ActivityPub::Serializer attribute :content_map, if: :language? attribute :updated, if: :edited? - has_many :media_attachments, key: :attachment + has_many :virtual_attachments, key: :attachment has_many :virtual_tags, key: :tag has_one :replies, serializer: ActivityPub::CollectionSerializer, if: :local? @@ -106,6 +106,10 @@ class ActivityPub::NoteSerializer < ActivityPub::Serializer object.account.sensitized? || object.sensitive end + def virtual_attachments + object.ordered_media_attachments + end + def virtual_tags object.active_mentions.to_a.sort_by(&:id) + object.tags + object.emojis end diff --git a/app/serializers/rest/status_edit_serializer.rb b/app/serializers/rest/status_edit_serializer.rb index a1f9e824e2..05ccd5e942 100644 --- a/app/serializers/rest/status_edit_serializer.rb +++ b/app/serializers/rest/status_edit_serializer.rb @@ -3,12 +3,18 @@ class REST::StatusEditSerializer < ActiveModel::Serializer has_one :account, serializer: REST::AccountSerializer - attributes :content, :spoiler_text, - :media_attachments_changed, :created_at + attributes :content, :spoiler_text, :sensitive, :created_at + has_many :ordered_media_attachments, key: :media_attachments, serializer: REST::MediaAttachmentSerializer has_many :emojis, serializer: REST::CustomEmojiSerializer + attribute :poll, if: -> { object.poll_options.present? } + def content Formatter.instance.format(object) end + + def poll + { options: object.poll_options.map { |title| { title: title } } } + end end diff --git a/app/serializers/rest/status_serializer.rb b/app/serializers/rest/status_serializer.rb index aef51e0f76..7c3dd673e3 100644 --- a/app/serializers/rest/status_serializer.rb +++ b/app/serializers/rest/status_serializer.rb @@ -19,7 +19,7 @@ class REST::StatusSerializer < ActiveModel::Serializer belongs_to :application, if: :show_application? belongs_to :account, serializer: REST::AccountSerializer - has_many :media_attachments, serializer: REST::MediaAttachmentSerializer + has_many :ordered_media_attachments, key: :media_attachments, serializer: REST::MediaAttachmentSerializer has_many :ordered_mentions, key: :mentions has_many :tags has_many :emojis, serializer: REST::CustomEmojiSerializer diff --git a/app/services/activitypub/process_status_update_service.rb b/app/services/activitypub/process_status_update_service.rb index 7438a7c536..11afa894f7 100644 --- a/app/services/activitypub/process_status_update_service.rb +++ b/app/services/activitypub/process_status_update_service.rb @@ -76,13 +76,14 @@ class ActivityPub::ProcessStatusUpdateService < BaseService end end - removed_media_attachments = previous_media_attachments - next_media_attachments - added_media_attachments = next_media_attachments - previous_media_attachments + added_media_attachments = next_media_attachments - previous_media_attachments - MediaAttachment.where(id: removed_media_attachments.map(&:id)).update_all(status_id: nil) MediaAttachment.where(id: added_media_attachments.map(&:id)).update_all(status_id: @status.id) - @media_attachments_changed = true if removed_media_attachments.any? || added_media_attachments.any? + @status.ordered_media_attachment_ids = next_media_attachments.map(&:id) + @status.media_attachments.reload + + @media_attachments_changed = true if @status.ordered_media_attachment_ids_changed? end def update_poll! @@ -215,19 +216,13 @@ class ActivityPub::ProcessStatusUpdateService < BaseService return if @status.edits.any? - @status.snapshot!( - media_attachments_changed: false, - at_time: @status.created_at - ) + @status.snapshot!(at_time: @status.created_at) end def create_edit! return unless significant_changes? - @status.snapshot!( - media_attachments_changed: @media_attachments_changed || @poll_changed, - account_id: @account.id - ) + @status.snapshot!(account_id: @account.id) end def skip_download? diff --git a/app/services/fan_out_on_write_service.rb b/app/services/fan_out_on_write_service.rb index 2bab91116a..76404c6b8c 100644 --- a/app/services/fan_out_on_write_service.rb +++ b/app/services/fan_out_on_write_service.rb @@ -110,7 +110,7 @@ class FanOutOnWriteService < BaseService Redis.current.publish('timeline:public', anonymous_payload) Redis.current.publish(@status.local? ? 'timeline:public:local' : 'timeline:public:remote', anonymous_payload) - if @status.media_attachments.any? + if @status.with_media? Redis.current.publish('timeline:public:media', anonymous_payload) Redis.current.publish(@status.local? ? 'timeline:public:local:media' : 'timeline:public:remote:media', anonymous_payload) end diff --git a/app/services/post_status_service.rb b/app/services/post_status_service.rb index 7508c3b646..c132930a98 100644 --- a/app/services/post_status_service.rb +++ b/app/services/post_status_service.rb @@ -100,7 +100,10 @@ class PostStatusService < BaseService end def validate_media! - return if @options[:media_ids].blank? || !@options[:media_ids].is_a?(Enumerable) + if @options[:media_ids].blank? || !@options[:media_ids].is_a?(Enumerable) + @media = [] + return + end raise Mastodon::ValidationError, I18n.t('media_attachments.validations.too_many') if @options[:media_ids].size > 4 || @options[:poll].present? @@ -157,6 +160,7 @@ class PostStatusService < BaseService { text: @text, media_attachments: @media || [], + ordered_media_attachment_ids: (@options[:media_ids] || []).map(&:to_i) & @media.map(&:id), thread: @in_reply_to, poll_attributes: poll_attributes, sensitive: @sensitive, diff --git a/app/services/remove_status_service.rb b/app/services/remove_status_service.rb index 7fb9b6301a..159aec1f23 100644 --- a/app/services/remove_status_service.rb +++ b/app/services/remove_status_service.rb @@ -40,7 +40,7 @@ class RemoveStatusService < BaseService remove_reblogs remove_from_hashtags remove_from_public - remove_from_media if @status.media_attachments.any? + remove_from_media if @status.with_media? remove_media end diff --git a/app/services/update_status_service.rb b/app/services/update_status_service.rb index 973e6ddee9..1c63ab6566 100644 --- a/app/services/update_status_service.rb +++ b/app/services/update_status_service.rb @@ -17,8 +17,6 @@ class UpdateStatusService < BaseService @status = status @options = options @account_id = account_id - @media_attachments_changed = false - @poll_changed = false Status.transaction do create_previous_edit! @@ -41,14 +39,12 @@ class UpdateStatusService < BaseService def update_media_attachments! previous_media_attachments = @status.media_attachments.to_a next_media_attachments = validate_media! - removed_media_attachments = previous_media_attachments - next_media_attachments added_media_attachments = next_media_attachments - previous_media_attachments - MediaAttachment.where(id: removed_media_attachments.map(&:id)).update_all(status_id: nil) MediaAttachment.where(id: added_media_attachments.map(&:id)).update_all(status_id: @status.id) + @status.ordered_media_attachment_ids = (@options[:media_ids] || []).map(&:to_i) & next_media_attachments.map(&:id) @status.media_attachments.reload - @media_attachments_changed = true if removed_media_attachments.any? || added_media_attachments.any? end def validate_media! @@ -73,19 +69,18 @@ class UpdateStatusService < BaseService # If for some reasons the options were changed, it invalidates all previous # votes, so we need to remove them - @poll_changed = true if @options[:poll][:options] != poll.options || ActiveModel::Type::Boolean.new.cast(@options[:poll][:multiple]) != poll.multiple + poll_changed = true if @options[:poll][:options] != poll.options || ActiveModel::Type::Boolean.new.cast(@options[:poll][:multiple]) != poll.multiple poll.options = @options[:poll][:options] poll.hide_totals = @options[:poll][:hide_totals] || false poll.multiple = @options[:poll][:multiple] || false poll.expires_in = @options[:poll][:expires_in] - poll.reset_votes! if @poll_changed + poll.reset_votes! if poll_changed poll.save! @status.poll_id = poll.id elsif previous_poll.present? previous_poll.destroy - @poll_changed = true @status.poll_id = nil end end @@ -136,16 +131,10 @@ class UpdateStatusService < BaseService return if @status.edits.any? - @status.snapshot!( - media_attachments_changed: false, - at_time: @status.created_at - ) + @status.snapshot!(at_time: @status.created_at) end def create_edit! - @status.snapshot!( - media_attachments_changed: @media_attachments_changed || @poll_changed, - account_id: @account_id - ) + @status.snapshot!(account_id: @account_id) end end diff --git a/app/views/admin/reports/_status.html.haml b/app/views/admin/reports/_status.html.haml index 4e06d4bbf0..1c033c4c34 100644 --- a/app/views/admin/reports/_status.html.haml +++ b/app/views/admin/reports/_status.html.haml @@ -11,15 +11,15 @@ %strong> Content warning: #{Formatter.instance.format_spoiler(status.proper)} = Formatter.instance.format(status.proper, custom_emojify: true) - - unless status.proper.media_attachments.empty? - - if status.proper.media_attachments.first.video? - - video = status.proper.media_attachments.first + - unless status.proper.ordered_media_attachments.empty? + - if status.proper.ordered_media_attachments.first.video? + - video = status.proper.ordered_media_attachments.first = react_component :video, src: video.file.url(:original), preview: video.file.url(:small), frameRate: video.file.meta.dig('original', 'frame_rate'), blurhash: video.blurhash, sensitive: status.proper.sensitive?, visible: false, width: 610, height: 343, inline: true, alt: video.description, media: [ActiveModelSerializers::SerializableResource.new(video, serializer: REST::MediaAttachmentSerializer)].as_json - - elsif status.proper.media_attachments.first.audio? - - audio = status.proper.media_attachments.first + - elsif status.proper.ordered_media_attachments.first.audio? + - audio = status.proper.ordered_media_attachments.first = react_component :audio, src: audio.file.url(:original), height: 110, alt: audio.description, duration: audio.file.meta.dig(:original, :duration) - else - = react_component :media_gallery, height: 343, sensitive: status.proper.sensitive?, visible: false, media: status.proper.media_attachments.map { |a| ActiveModelSerializers::SerializableResource.new(a, serializer: REST::MediaAttachmentSerializer).as_json } + = react_component :media_gallery, height: 343, sensitive: status.proper.sensitive?, visible: false, media: status.proper.ordered_media_attachments.map { |a| ActiveModelSerializers::SerializableResource.new(a, serializer: REST::MediaAttachmentSerializer).as_json } .detailed-status__meta - if status.application diff --git a/app/views/admin/reports/index.html.haml b/app/views/admin/reports/index.html.haml index 6191733733..248718a73d 100644 --- a/app/views/admin/reports/index.html.haml +++ b/app/views/admin/reports/index.html.haml @@ -59,11 +59,11 @@ %span.report-card__summary__item__content__icon{ title: t('admin.accounts.statuses') } = fa_icon('comment') - = report.statuses.count + = report.status_ids.size %span.report-card__summary__item__content__icon{ title: t('admin.accounts.media_attachments') } = fa_icon('camera') - = report.media_attachments.count + = report.media_attachments_count - if report.forwarded? ยท diff --git a/app/views/admin/trends/statuses/_status.html.haml b/app/views/admin/trends/statuses/_status.html.haml index edb27b9ff5..50a8553492 100644 --- a/app/views/admin/trends/statuses/_status.html.haml +++ b/app/views/admin/trends/statuses/_status.html.haml @@ -9,7 +9,7 @@ = link_to ActivityPub::TagManager.instance.url_for(status), target: '_blank', class: 'emojify', rel: 'noopener noreferrer' do = one_line_preview(status) - - status.media_attachments.each do |media_attachment| + - status.ordered_media_attachments.each do |media_attachment| %abbr{ title: media_attachment.description } = fa_icon 'link' = media_attachment.file_file_name diff --git a/app/views/disputes/strikes/show.html.haml b/app/views/disputes/strikes/show.html.haml index 7248b25741..0fc32b9186 100644 --- a/app/views/disputes/strikes/show.html.haml +++ b/app/views/disputes/strikes/show.html.haml @@ -53,7 +53,7 @@ = link_to short_account_status_url(@strike.target_account, status_id), class: 'emojify' do = one_line_preview(status) - - status.media_attachments.each do |media_attachment| + - status.ordered_media_attachments.each do |media_attachment| %abbr{ title: media_attachment.description } = fa_icon 'link' = media_attachment.file_file_name diff --git a/app/views/notification_mailer/_status.html.haml b/app/views/notification_mailer/_status.html.haml index 31460a76e1..f520208e11 100644 --- a/app/views/notification_mailer/_status.html.haml +++ b/app/views/notification_mailer/_status.html.haml @@ -33,9 +33,9 @@ %div.auto-dir = Formatter.instance.format(status) - - if status.media_attachments.size > 0 + - if status.ordered_media_attachments.size > 0 %p - - status.media_attachments.each do |a| + - status.ordered_media_attachments.each do |a| - if status.local? = link_to full_asset_url(a.file.url(:original)), full_asset_url(a.file.url(:original)) - else diff --git a/app/views/statuses/_detailed_status.html.haml b/app/views/statuses/_detailed_status.html.haml index 1922f53ce3..6ccf3725a6 100644 --- a/app/views/statuses/_detailed_status.html.haml +++ b/app/views/statuses/_detailed_status.html.haml @@ -25,10 +25,10 @@ - if status.preloadable_poll = render_poll_component(status) - - if !status.media_attachments.empty? - - if status.media_attachments.first.video? + - if !status.ordered_media_attachments.empty? + - if status.ordered_media_attachments.first.video? = render_video_component(status, width: 670, height: 380, detailed: true) - - elsif status.media_attachments.first.audio? + - elsif status.ordered_media_attachments.first.audio? = render_audio_component(status, width: 670, height: 380) - else = render_media_gallery_component(status, height: 380, standalone: true) diff --git a/app/views/statuses/_og_image.html.haml b/app/views/statuses/_og_image.html.haml index c8b6147ef7..5a647531ab 100644 --- a/app/views/statuses/_og_image.html.haml +++ b/app/views/statuses/_og_image.html.haml @@ -1,6 +1,6 @@ - if activity.is_a?(Status) && (activity.non_sensitive_with_media? || (activity.with_media? && Setting.preview_sensitive_media)) - player_card = false - - activity.media_attachments.each do |media| + - activity.ordered_media_attachments.each do |media| - if media.image? = opengraph 'og:image', full_asset_url(media.file.url(:original)) = opengraph 'og:image:type', media.file_content_type diff --git a/app/views/statuses/_simple_status.html.haml b/app/views/statuses/_simple_status.html.haml index 0139d2016c..5dd265b59e 100644 --- a/app/views/statuses/_simple_status.html.haml +++ b/app/views/statuses/_simple_status.html.haml @@ -37,10 +37,10 @@ - if status.preloadable_poll = render_poll_component(status) - - if !status.media_attachments.empty? - - if status.media_attachments.first.video? + - if !status.ordered_media_attachments.empty? + - if status.ordered_media_attachments.first.video? = render_video_component(status, width: 610, height: 343) - - elsif status.media_attachments.first.audio? + - elsif status.ordered_media_attachments.first.audio? = render_audio_component(status, width: 610, height: 343) - else = render_media_gallery_component(status, height: 343) diff --git a/db/migrate/20220302232632_add_ordered_media_attachment_ids_to_statuses.rb b/db/migrate/20220302232632_add_ordered_media_attachment_ids_to_statuses.rb new file mode 100644 index 0000000000..5443f32a2e --- /dev/null +++ b/db/migrate/20220302232632_add_ordered_media_attachment_ids_to_statuses.rb @@ -0,0 +1,5 @@ +class AddOrderedMediaAttachmentIdsToStatuses < ActiveRecord::Migration[6.1] + def change + add_column :statuses, :ordered_media_attachment_ids, :bigint, array: true + end +end diff --git a/db/migrate/20220303000827_add_ordered_media_attachment_ids_to_status_edits.rb b/db/migrate/20220303000827_add_ordered_media_attachment_ids_to_status_edits.rb new file mode 100644 index 0000000000..b1071f3593 --- /dev/null +++ b/db/migrate/20220303000827_add_ordered_media_attachment_ids_to_status_edits.rb @@ -0,0 +1,8 @@ +class AddOrderedMediaAttachmentIdsToStatusEdits < ActiveRecord::Migration[6.1] + def change + add_column :status_edits, :ordered_media_attachment_ids, :bigint, array: true + add_column :status_edits, :media_descriptions, :text, array: true + add_column :status_edits, :poll_options, :string, array: true + add_column :status_edits, :sensitive, :boolean + end +end diff --git a/db/post_migrate/20220303203437_remove_media_attachments_changed_from_status_edits.rb b/db/post_migrate/20220303203437_remove_media_attachments_changed_from_status_edits.rb new file mode 100644 index 0000000000..09725c74ec --- /dev/null +++ b/db/post_migrate/20220303203437_remove_media_attachments_changed_from_status_edits.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class RemoveMediaAttachmentsChangedFromStatusEdits < ActiveRecord::Migration[5.2] + def change + safety_assured { remove_column :status_edits, :media_attachments_changed, :boolean, default: false, null: false } + end +end diff --git a/db/schema.rb b/db/schema.rb index ef1620113c..6251fa28c5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -391,7 +391,6 @@ ActiveRecord::Schema.define(version: 2022_03_07_094650) do t.bigint "parent_id" t.inet "ips", array: true t.datetime "last_refresh_at" - t.index ["domain"], name: "index_email_domain_blocks_on_domain", unique: true end @@ -845,9 +844,12 @@ ActiveRecord::Schema.define(version: 2022_03_07_094650) do t.bigint "account_id" t.text "text", default: "", null: false t.text "spoiler_text", default: "", null: false - t.boolean "media_attachments_changed", default: false, null: false t.datetime "created_at", precision: 6, null: false t.datetime "updated_at", precision: 6, null: false + t.bigint "ordered_media_attachment_ids", array: true + t.text "media_descriptions", array: true + t.string "poll_options", array: true + t.boolean "sensitive" t.index ["account_id"], name: "index_status_edits_on_account_id" t.index ["status_id"], name: "index_status_edits_on_status_id" end @@ -892,6 +894,7 @@ ActiveRecord::Schema.define(version: 2022_03_07_094650) do t.datetime "deleted_at" t.datetime "edited_at" t.boolean "trendable" + t.bigint "ordered_media_attachment_ids", array: true t.index ["account_id", "id", "visibility", "updated_at"], name: "index_statuses_20190820", order: { id: :desc }, where: "(deleted_at IS NULL)" t.index ["deleted_at"], name: "index_statuses_on_deleted_at", where: "(deleted_at IS NOT NULL)" t.index ["id", "account_id"], name: "index_statuses_local_20190824", order: { id: :desc }, where: "((local OR (uri IS NULL)) AND (deleted_at IS NULL) AND (visibility = 0) AND (reblog_of_id IS NULL) AND ((NOT reply) OR (in_reply_to_account_id = account_id)))" diff --git a/spec/models/report_spec.rb b/spec/models/report_spec.rb index df32a7c9d5..874be41328 100644 --- a/spec/models/report_spec.rb +++ b/spec/models/report_spec.rb @@ -11,14 +11,13 @@ describe Report do end end - describe 'media_attachments' do - it 'returns media attachments from statuses' do - status = Fabricate(:status) - media_attachment = Fabricate(:media_attachment, status: status) - _other_media_attachment = Fabricate(:media_attachment) - report = Fabricate(:report, status_ids: [status.id]) + describe 'media_attachments_count' do + it 'returns count of media attachments in statuses' do + status1 = Fabricate(:status, ordered_media_attachment_ids: [1, 2]) + status2 = Fabricate(:status, ordered_media_attachment_ids: [5]) + report = Fabricate(:report, status_ids: [status1.id, status2.id]) - expect(report.media_attachments).to eq [media_attachment] + expect(report.media_attachments_count).to eq 3 end end diff --git a/spec/services/activitypub/process_status_update_service_spec.rb b/spec/services/activitypub/process_status_update_service_spec.rb index 6ee1dcb43a..40b405217d 100644 --- a/spec/services/activitypub/process_status_update_service_spec.rb +++ b/spec/services/activitypub/process_status_update_service_spec.rb @@ -124,7 +124,7 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do end it 'updates media attachments' do - media_attachment = status.media_attachments.reload.first + media_attachment = status.reload.ordered_media_attachments.first expect(media_attachment).to_not be_nil expect(media_attachment.remote_url).to eq 'https://example.com/foo.png' @@ -135,7 +135,7 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do end it 'records media change in edit' do - expect(status.edits.reload.last.media_attachments_changed).to be true + expect(status.edits.reload.last.ordered_media_attachment_ids).to_not be_empty end end @@ -173,11 +173,11 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do end it 'updates media attachments' do - expect(status.media_attachments.reload.map(&:remote_url)).to eq %w(https://example.com/foo.png) + expect(status.ordered_media_attachments.map(&:remote_url)).to eq %w(https://example.com/foo.png) end it 'records media change in edit' do - expect(status.edits.reload.last.media_attachments_changed).to be true + expect(status.edits.reload.last.ordered_media_attachment_ids).to_not be_empty end end @@ -193,7 +193,7 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do end it 'records media change in edit' do - expect(status.edits.reload.last.media_attachments_changed).to be true + expect(status.edits.reload.last.poll_options).to be_nil end end @@ -226,7 +226,7 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do end it 'records media change in edit' do - expect(status.edits.reload.last.media_attachments_changed).to be true + expect(status.edits.reload.last.poll_options).to eq %w(Foo Bar Baz) end end @@ -239,10 +239,5 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do subject.call(status, json) expect(status.reload.edited_at.to_s).to eq '2021-09-08 22:39:25 UTC' end - - it 'records that no media has been changed in edit' do - subject.call(status, json) - expect(status.edits.reload.last.media_attachments_changed).to be false - end end end diff --git a/spec/services/update_status_service_spec.rb b/spec/services/update_status_service_spec.rb index 4fd4837c62..78cc89cd46 100644 --- a/spec/services/update_status_service_spec.rb +++ b/spec/services/update_status_service_spec.rb @@ -21,7 +21,7 @@ RSpec.describe UpdateStatusService, type: :service do end it 'saves edit history' do - expect(status.edits.pluck(:text, :media_attachments_changed)).to eq [['Foo', false], ['Bar', false]] + expect(status.edits.pluck(:text)).to eq %w(Foo Bar) end end @@ -39,7 +39,7 @@ RSpec.describe UpdateStatusService, type: :service do end it 'saves edit history' do - expect(status.edits.pluck(:text, :spoiler_text, :media_attachments_changed)).to eq [['Foo', '', false], ['Foo', 'Bar', false]] + expect(status.edits.pluck(:text, :spoiler_text)).to eq [['Foo', ''], ['Foo', 'Bar']] end end @@ -54,11 +54,11 @@ RSpec.describe UpdateStatusService, type: :service do end it 'updates media attachments' do - expect(status.media_attachments.to_a).to eq [attached_media_attachment] + expect(status.ordered_media_attachments).to eq [attached_media_attachment] end - it 'detaches detached media attachments' do - expect(detached_media_attachment.reload.status_id).to be_nil + it 'does not detach detached media attachments' do + expect(detached_media_attachment.reload.status_id).to eq status.id end it 'attaches attached media attachments' do @@ -66,7 +66,7 @@ RSpec.describe UpdateStatusService, type: :service do end it 'saves edit history' do - expect(status.edits.pluck(:text, :media_attachments_changed)).to eq [['Foo', false], ['Foo', true]] + expect(status.edits.pluck(:ordered_media_attachment_ids)).to eq [[detached_media_attachment.id], [attached_media_attachment.id]] end end @@ -95,7 +95,7 @@ RSpec.describe UpdateStatusService, type: :service do end it 'saves edit history' do - expect(status.edits.pluck(:text, :media_attachments_changed)).to eq [['Foo', false], ['Foo', true]] + expect(status.edits.pluck(:poll_options)).to eq [%w(Foo Bar), %w(Bar Baz Foo)] end end