Fix error when rendering public pages with media attachments (#16763)

* Add tests

* Fix error when rendering public pages with media attachments

* Add tests

* Fix tests

* Please CodeClimate

app/controllers/media_controller.rb

@@ -27,7 +27,12 @@ class MediaController < ApplicationController
   private
 
   def set_media_attachment
-    @media_attachment = MediaAttachment.attached.find_by!(shortcode: params[:id] || params[:medium_id])
+    id = params[:id] || params[:medium_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_by!(id: id)
   end
 
   def verify_permitted_status!

app/models/media_attachment.rb

@@ -217,7 +217,7 @@ class MediaAttachment < ApplicationRecord
   end
 
   def to_param
-    shortcode
+    shortcode.presence || id&.to_s
   end
 
   def focus=(point)

spec/controllers/media_controller_spec.rb

@@ -6,33 +6,60 @@ describe MediaController do
   render_views
 
   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, 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, shortcode: 'foo')
-      get :show, params: { id: media_attachment.to_param }
-
-      expect(response).to have_http_status(404)
-    end
-
     it 'raises when shortcode cant be found' do
       get :show, params: { id: 'missing' }
 
       expect(response).to have_http_status(404)
     end
 
+    context 'when the media attachment has a shortcode' do
+      it 'redirects to the file url when attached to a status' do
+        status = Fabricate(:status)
+        media_attachment = Fabricate(:media_attachment, status: status, shortcode: 'OI6IgDzG-nYTqvDQ994')
+        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, shortcode: 'OI6IgDzG-nYTqvDQ994')
+        get :show, params: { id: media_attachment.to_param }
+
+        expect(response).to have_http_status(404)
+      end
+
       it 'raises when not permitted to view' do
         status = Fabricate(:status, visibility: :direct)
-      media_attachment = Fabricate(:media_attachment, status: status, shortcode: 'foo')
+        media_attachment = Fabricate(:media_attachment, status: status, shortcode: 'OI6IgDzG-nYTqvDQ994')
+        get :show, params: { id: media_attachment.to_param }
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    context 'when the media attachment has no shortcode' do
+      it 'redirects to the file url when attached to a status' do
+        status = Fabricate(:status)
+        media_attachment = Fabricate(:media_attachment, status: status)
+        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)
+        get :show, params: { id: media_attachment.to_param }
+
+        expect(response).to have_http_status(404)
+      end
+
+      it 'raises when not permitted to view' do
+        status = Fabricate(:status, visibility: :direct)
+        media_attachment = Fabricate(:media_attachment, status: status)
         get :show, params: { id: media_attachment.to_param }
 
         expect(response).to have_http_status(404)
       end
     end
   end
+end

spec/models/media_attachment_spec.rb

@@ -62,14 +62,26 @@ RSpec.describe MediaAttachment, type: :model do
   end
 
   describe '#to_param' do
-    let(:media_attachment) { Fabricate(:media_attachment) }
+    let(:media_attachment) { Fabricate(:media_attachment, shortcode: shortcode) }
-    let(:shortcode)        { media_attachment.shortcode }
+    let(:shortcode)        { nil }
+
+    context 'when media attachment has a shortcode' do
+      let(:shortcode) { 'foo' }
 
       it 'returns shortcode' do
         expect(media_attachment.to_param).to eq shortcode
       end
     end
 
+    context 'when media attachment does not have a shortcode' do
+      let(:shortcode) { nil }
+
+      it 'returns string representation of id' do
+        expect(media_attachment.to_param).to eq media_attachment.id.to_s
+      end
+    end
+  end
+
   describe 'animated gif conversion' do
     let(:media) { MediaAttachment.create(account: Fabricate(:account), file: attachment_fixture('avatar.gif')) }
 

spec/views/statuses/show.html.haml_spec.rb

@@ -19,6 +19,7 @@ describe 'statuses/show.html.haml', without_verify_partial_doubles: true do
     alice  = Fabricate(:account, username: 'alice', display_name: 'Alice')
     bob    = Fabricate(:account, username: 'bob', display_name: 'Bob')
     status = Fabricate(:status, account: alice, text: 'Hello World')
+    media  = Fabricate(:media_attachment, account: alice, status: status, type: :video)
     reply  = Fabricate(:status, account: bob, thread: status, text: 'Hello Alice')
 
     assign(:status, status)
@@ -39,6 +40,7 @@ describe 'statuses/show.html.haml', without_verify_partial_doubles: true do
     bob     = Fabricate(:account, username: 'bob', display_name: 'Bob')
     carl    = Fabricate(:account, username: 'carl', display_name: 'Carl')
     status  = Fabricate(:status, account: alice, text: 'Hello World')
+    media   = Fabricate(:media_attachment, account: alice, status: status, type: :video)
     reply   = Fabricate(:status, account: bob, thread: status, text: 'Hello Alice')
     comment = Fabricate(:status, account: carl, thread: reply, text: 'Hello Bob')
 
@@ -64,6 +66,7 @@ describe 'statuses/show.html.haml', without_verify_partial_doubles: true do
   it 'has valid opengraph tags' do
     alice  = Fabricate(:account, username: 'alice', display_name: 'Alice')
     status = Fabricate(:status, account: alice, text: 'Hello World')
+    media  = Fabricate(:media_attachment, account: alice, status: status, type: :video)
 
     assign(:status, status)
     assign(:account, alice)
@@ -78,4 +81,21 @@ describe 'statuses/show.html.haml', without_verify_partial_doubles: true do
     expect(header_tags).to match(%r{<meta content=".+" property="og:image" />})
     expect(header_tags).to match(%r{<meta content="http://.+" property="og:url" />})
   end
+
+  it 'has twitter player tag' do
+    alice  = Fabricate(:account, username: 'alice', display_name: 'Alice')
+    status = Fabricate(:status, account: alice, text: 'Hello World')
+    media  = Fabricate(:media_attachment, account: alice, status: status, type: :video)
+
+    assign(:status, status)
+    assign(:account, alice)
+    assign(:descendant_threads, [])
+
+    render
+
+    header_tags = view.content_for(:header_tags)
+
+    expect(header_tags).to match(%r{<meta content="http://.+/media/.+/player" property="twitter:player" />})
+    expect(header_tags).to match(%r{<meta content="player" property="twitter:card" />})
+  end
 end