Simplify media attachment lookup in show/player actions (#38565)

app/controllers/media_controller.rb

@@ -24,12 +24,7 @@ class MediaController < ApplicationController
   private
 
   def set_media_attachment
-    id = params[:id] || params[:medium_id]
+    @media_attachment = MediaAttachment.local.attached.identified(params[:id])
-    return if id.nil?
-
-    scope = MediaAttachment.local.attached
-    # If id is 19 characters long, it's a shortcode, otherwise it's an identifier
-    @media_attachment = id.size == 19 ? scope.find_by!(shortcode: id) : scope.find(id)
   end
 
   def verify_permitted_status!

app/models/media_attachment.rb

@@ -38,6 +38,8 @@ class MediaAttachment < ApplicationRecord
   enum :type, { image: 0, gifv: 1, video: 2, unknown: 3, audio: 4 }
   enum :processing, { queued: 0, in_progress: 1, complete: 2, failed: 3 }, prefix: true
 
+  SHORTCODE_LENGTH = 19
+
   MAX_DESCRIPTION_LENGTH = 1_500
   MAX_DESCRIPTION_HARD_LENGTH_LIMIT = 10_000
 
@@ -300,6 +302,10 @@ class MediaAttachment < ApplicationRecord
   after_post_process :set_meta
 
   class << self
+    def identified(identifier)
+      identifier.size == SHORTCODE_LENGTH ? find_by!(shortcode: identifier) : find(identifier)
+    end
+
     def supported_mime_types
       IMAGE_MIME_TYPES + VIDEO_MIME_TYPES + AUDIO_MIME_TYPES
     end

app/serializers/seo/social_media_posting_serializer.rb

@@ -97,7 +97,7 @@ class SEO::SocialMediaPostingSerializer < ActiveModel::Serializer
         upload_date: attachment.created_at.iso8601,
         content_url: full_asset_url(attachment.file.url(:original, false)),
         thumbnail_url: attachment.thumbnail.present? ? full_asset_url(attachment.thumbnail.url(:original)) : full_asset_url(attachment.file.url(:small)),
-        embed_url: medium_player_url(attachment),
+        embed_url: player_medium_url(attachment),
         description: attachment.description,
       }
     end
@@ -112,7 +112,7 @@ class SEO::SocialMediaPostingSerializer < ActiveModel::Serializer
         upload_date: attachment.created_at.iso8601,
         content_url: full_asset_url(attachment.file.url(:original, false)),
         thumbnail_url: attachment.thumbnail.present? ? full_asset_url(attachment.thumbnail.url(:original)) : full_asset_url(attachment.file.url(:small)),
-        embed_url: medium_player_url(attachment),
+        embed_url: player_medium_url(attachment),
         description: attachment.description,
       }
     end

app/views/statuses/_og_image.html.haml

@@ -19,7 +19,7 @@
       = opengraph 'og:video', full_asset_url(media.file.url(:original))
       = opengraph 'og:video:secure_url', full_asset_url(media.file.url(:original))
       = opengraph 'og:video:type', media.file_content_type
-      = opengraph 'twitter:player', medium_player_url(media)
+      = opengraph 'twitter:player', player_medium_url(media)
       = opengraph 'twitter:player:stream', full_asset_url(media.file.url(:original))
       = opengraph 'twitter:player:stream:content_type', media.file_content_type
       - unless media.file.meta.nil?
@@ -35,7 +35,7 @@
       = opengraph 'og:audio', full_asset_url(media.file.url(:original))
       = opengraph 'og:audio:secure_url', full_asset_url(media.file.url(:original))
       = opengraph 'og:audio:type', media.file_content_type
-      = opengraph 'twitter:player', medium_player_url(media)
+      = opengraph 'twitter:player', player_medium_url(media)
       = opengraph 'twitter:player:stream', full_asset_url(media.file.url(:original))
       = opengraph 'twitter:player:stream:content_type', media.file_content_type
       = opengraph 'twitter:player:width', '670'

config/routes.rb

@@ -193,7 +193,7 @@ Rails.application.routes.draw do
   end
 
   resources :media, only: [:show] do
-    get :player
+    member { get :player }
   end
 
   resources :tags,   only: [:show]

spec/requests/media_spec.rb

@@ -93,7 +93,7 @@ RSpec.describe 'Media' do
       let(:media) { Fabricate :media_attachment }
 
       it 'responds with not found' do
-        get medium_player_path(media)
+        get player_medium_path(media)
 
         expect(response)
           .to have_http_status(404)

spec/system/media_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe 'Media' do
         let(:media) { Fabricate :media_attachment, type: :video }
 
         it 'visits the player page and renders media' do
-          visit medium_player_path(media)
+          visit player_medium_path(media)
 
           expect(page)
             .to have_css('body', class: 'player')
@@ -27,7 +27,7 @@ RSpec.describe 'Media' do
         let(:media) { Fabricate :media_attachment, type: :gifv }
 
         it 'visits the player page and renders media' do
-          visit medium_player_path(media)
+          visit player_medium_path(media)
 
           expect(page)
             .to have_css('body', class: 'player')
@@ -39,7 +39,7 @@ RSpec.describe 'Media' do
         let(:media) { Fabricate :media_attachment, type: :audio }
 
         it 'visits the player page and renders media' do
-          visit medium_player_path(media)
+          visit player_medium_path(media)
 
           expect(page)
             .to have_css('body', class: 'player')