From 5159ba26e485daaeded2288c1b02bd1e516e1ca6 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 13 Oct 2021 15:27:19 +0200 Subject: [PATCH] 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 | 7 ++- app/models/media_attachment.rb | 2 +- spec/controllers/media_controller_spec.rb | 67 +++++++++++++++------- spec/models/media_attachment_spec.rb | 20 +++++-- spec/views/statuses/show.html.haml_spec.rb | 44 ++++++++++---- 5 files changed, 102 insertions(+), 38 deletions(-) diff --git a/app/controllers/media_controller.rb b/app/controllers/media_controller.rb index ce015dd1b2..ee82625a03 100644 --- a/app/controllers/media_controller.rb +++ b/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! diff --git a/app/models/media_attachment.rb b/app/models/media_attachment.rb index 97d619f35f..1741d20e96 100644 --- a/app/models/media_attachment.rb +++ b/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) diff --git a/spec/controllers/media_controller_spec.rb b/spec/controllers/media_controller_spec.rb index 6651e9d840..efd15b5b40 100644 --- a/spec/controllers/media_controller_spec.rb +++ b/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 - it 'raises when not permitted to view' do - status = Fabricate(:status, visibility: :direct) - media_attachment = Fabricate(:media_attachment, status: status, shortcode: 'foo') - get :show, params: { id: media_attachment.to_param } + 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 have_http_status(404) + 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: '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 diff --git a/spec/models/media_attachment_spec.rb b/spec/models/media_attachment_spec.rb index 5dbb3d2060..7360b23cf0 100644 --- a/spec/models/media_attachment_spec.rb +++ b/spec/models/media_attachment_spec.rb @@ -62,11 +62,23 @@ RSpec.describe MediaAttachment, type: :model do end describe '#to_param' do - let(:media_attachment) { Fabricate(:media_attachment) } - let(:shortcode) { media_attachment.shortcode } + let(:media_attachment) { Fabricate(:media_attachment, shortcode: shortcode) } + let(:shortcode) { nil } - it 'returns shortcode' do - expect(media_attachment.to_param).to eq shortcode + 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 diff --git a/spec/views/statuses/show.html.haml_spec.rb b/spec/views/statuses/show.html.haml_spec.rb index dbda3b6655..879a26959e 100644 --- a/spec/views/statuses/show.html.haml_spec.rb +++ b/spec/views/statuses/show.html.haml_spec.rb @@ -16,10 +16,11 @@ describe 'statuses/show.html.haml', without_verify_partial_doubles: true do end it 'has valid author h-card and basic data for a detailed_status' 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') - reply = Fabricate(:status, account: bob, thread: status, text: 'Hello Alice') + 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) assign(:account, alice) @@ -35,12 +36,13 @@ describe 'statuses/show.html.haml', without_verify_partial_doubles: true do end it 'has valid h-cites for p-in-reply-to and p-comment' do - alice = Fabricate(:account, username: 'alice', display_name: 'Alice') - bob = Fabricate(:account, username: 'bob', display_name: 'Bob') - carl = Fabricate(:account, username: 'carl', display_name: 'Carl') - status = Fabricate(:status, account: alice, text: 'Hello World') - reply = Fabricate(:status, account: bob, thread: status, text: 'Hello Alice') - comment = Fabricate(:status, account: carl, thread: reply, text: 'Hello Bob') + alice = Fabricate(:account, username: 'alice', display_name: 'Alice') + 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') assign(:status, reply) assign(:account, alice) @@ -62,8 +64,9 @@ describe 'statuses/show.html.haml', without_verify_partial_doubles: true do end it 'has valid opengraph tags' do - alice = Fabricate(:account, username: 'alice', display_name: 'Alice') - status = Fabricate(:status, account: alice, text: 'Hello World') + 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{}) expect(header_tags).to match(%r{}) 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{}) + expect(header_tags).to match(%r{}) + end end