Stop setting a shortcode to newly-created media attachments (#16730)

* 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

diff --git a/app/models/media_attachment.rb b/app/models/media_attachment.rb
index 3515f6895082df490241ed3a2f3e89b5869aeae1..66d800b7b4d30c1ce663ac62b89b1ab100252611 100644 (file)
--- 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 a24f953152f4a811b47e400341a6f37c9f33c4ea..f27dda832adeb8f832bb6fd8418828fe93c998e8 100644 (file)
--- 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 9b7e1b65c63937b9dc10920b221603df1c65ab8a..31460a76e186e831018bfc2979495c42c151003a 100644 (file)
--- 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 2925aed599af3d8d08a6120f8b96aa9a4da439e2..6651e9d840871df7165149ed21d27442c5f3616a 100644 (file)
--- 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)