From 4080a8ec33671a37858cb70898ea0f6af58b5f5a Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 13 Sep 2021 18:59:37 +0200 Subject: [PATCH] Stop setting a shortcode to newly-created media attachments (#16730) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Stop setting a shortcode to newly-created media attachments The WebUI has stopped using the “short media URL” in ages. This isn't used anywhere except for mail notifications. Deprecating it would allow us to eventually get rid of at least a database column and corruption-prone index, as well as a controller. * Fix tests --- app/models/media_attachment.rb | 11 ++--------- app/serializers/rest/media_attachment_serializer.rb | 2 +- app/views/notification_mailer/_status.html.haml | 2 +- spec/controllers/media_controller_spec.rb | 6 +++--- 4 files changed, 7 insertions(+), 14 deletions(-) diff --git a/app/models/media_attachment.rb b/app/models/media_attachment.rb index 3515f68950..66d800b7b4 100644 --- a/app/models/media_attachment.rb +++ b/app/models/media_attachment.rb @@ -255,7 +255,7 @@ class MediaAttachment < ApplicationRecord after_commit :reset_parent_cache, on: :update before_create :prepare_description, unless: :local? - before_create :set_shortcode + before_create :set_unknown_type before_create :set_processing after_post_process :set_meta @@ -298,15 +298,8 @@ class MediaAttachment < ApplicationRecord private - def set_shortcode + def set_unknown_type self.type = :unknown if file.blank? && !type_changed? - - return unless local? - - loop do - self.shortcode = SecureRandom.urlsafe_base64(14) - break if MediaAttachment.find_by(shortcode: shortcode).nil? - end end def prepare_description diff --git a/app/serializers/rest/media_attachment_serializer.rb b/app/serializers/rest/media_attachment_serializer.rb index a24f953152..f27dda832a 100644 --- a/app/serializers/rest/media_attachment_serializer.rb +++ b/app/serializers/rest/media_attachment_serializer.rb @@ -40,7 +40,7 @@ class REST::MediaAttachmentSerializer < ActiveModel::Serializer end def text_url - object.local? ? medium_url(object) : nil + object.local? && object.shortcode.present? ? medium_url(object) : nil end def meta diff --git a/app/views/notification_mailer/_status.html.haml b/app/views/notification_mailer/_status.html.haml index 9b7e1b65c6..31460a76e1 100644 --- a/app/views/notification_mailer/_status.html.haml +++ b/app/views/notification_mailer/_status.html.haml @@ -37,7 +37,7 @@ %p - status.media_attachments.each do |a| - if status.local? - = link_to medium_url(a), medium_url(a) + = link_to full_asset_url(a.file.url(:original)), full_asset_url(a.file.url(:original)) - else = link_to a.remote_url, a.remote_url diff --git a/spec/controllers/media_controller_spec.rb b/spec/controllers/media_controller_spec.rb index 2925aed599..6651e9d840 100644 --- a/spec/controllers/media_controller_spec.rb +++ b/spec/controllers/media_controller_spec.rb @@ -8,14 +8,14 @@ describe MediaController do describe '#show' do it 'redirects to the file url when attached to a status' do status = Fabricate(:status) - media_attachment = Fabricate(:media_attachment, status: status) + media_attachment = Fabricate(:media_attachment, status: status, shortcode: 'foo') get :show, params: { id: media_attachment.to_param } expect(response).to redirect_to(media_attachment.file.url(:original)) end it 'responds with missing when there is not an attached status' do - media_attachment = Fabricate(:media_attachment, status: nil) + media_attachment = Fabricate(:media_attachment, status: nil, shortcode: 'foo') get :show, params: { id: media_attachment.to_param } expect(response).to have_http_status(404) @@ -29,7 +29,7 @@ describe MediaController do it 'raises when not permitted to view' do status = Fabricate(:status, visibility: :direct) - media_attachment = Fabricate(:media_attachment, status: status) + media_attachment = Fabricate(:media_attachment, status: status, shortcode: 'foo') get :show, params: { id: media_attachment.to_param } expect(response).to have_http_status(404)