diff --git a/app/controllers/context_controller.rb b/app/controllers/context_controller.rb index d2a13778d0a..600c484afba 100644 --- a/app/controllers/context_controller.rb +++ b/app/controllers/context_controller.rb @@ -40,13 +40,12 @@ class ContextController < ApplicationController def media_object_inline @show_left_side = false @show_right_side = false - @media_object = MediaObject.find_by_media_id(params[:id]) + @media_object = MediaObject.by_media_id(params[:id]).first render end def media_object_redirect - mo = MediaObject.find_by_media_id(params[:id]) - mo ||= MediaObject.find_by_old_media_id(params[:id]) + mo = MediaObject.by_media_id(params[:id]).first mo.viewed! if mo config = Kaltura::ClientV3.config if config @@ -102,7 +101,7 @@ class ContextController < ApplicationController end elsif notification[:notification_type] == 'entry_delete' entry_id = notification[:entry_id] - mo = MediaObject.find_by_media_id(entry_id) + mo = MediaObject.by_media_id(entry_id).first mo.destroy_without_destroying_attachment end end diff --git a/app/controllers/conversations_controller.rb b/app/controllers/conversations_controller.rb index ec19a91ee36..9ccabc7f786 100644 --- a/app/controllers/conversations_controller.rb +++ b/app/controllers/conversations_controller.rb @@ -659,7 +659,7 @@ class ConversationsController < ApplicationController media_id = params[:media_comment_id] media_type = params[:media_comment_type] if media_id.present? && media_type.present? - media_comment = MediaObject.find_by_media_id_and_media_type(media_id, media_type) + media_comment = MediaObject.by_media_id.by_media_type(media_id, media_type).first if media_comment media_comment.context = @current_user media_comment.save diff --git a/app/models/conversation_message.rb b/app/models/conversation_message.rb index 47638329b28..657eb42f41f 100644 --- a/app/models/conversation_message.rb +++ b/app/models/conversation_message.rb @@ -61,7 +61,7 @@ class ConversationMessage < ActiveRecord::Base def infer_values self.media_comment_id = nil if self.media_comment_id && self.media_comment_id.strip.empty? if self.media_comment_id && self.media_comment_id_changed? - @media_comment = MediaObject.find_by_media_id(self.media_comment_id) + @media_comment = MediaObject.by_media_id(self.media_comment_id).first self.media_comment_id = nil unless @media_comment self.media_comment_type = @media_comment.media_type if @media_comment end @@ -74,7 +74,7 @@ class ConversationMessage < ActiveRecord::Base def media_comment if !@media_comment && self.media_comment_id - @media_comment = MediaObject.find_by_media_id(self.media_comment_id) + @media_comment = MediaObject.by_media_id(self.media_comment_id).first end @media_comment end diff --git a/app/models/media_object.rb b/app/models/media_object.rb index 095a865501c..5f1ab56ab2b 100644 --- a/app/models/media_object.rb +++ b/app/models/media_object.rb @@ -41,6 +41,13 @@ class MediaObject < ActiveRecord::Base @push_user_title = nil end + def self.find_by_media_id(media_id) + unless Rails.env.production? + raise "Do not look up MediaObjects by media_id - use the scope by_media_id instead to support migrated content." + end + super + end + # if wait_for_completion is true, this will wait SYNCHRONOUSLY for the bulk # upload to complete. Wrap it in a timeout if you ever want it to give up # waiting. @@ -261,6 +268,14 @@ class MediaObject < ActiveRecord::Base {:conditions => ['media_objects.workflow_state != ?', 'deleted'] } } + named_scope :by_media_id, lambda { |media_id| + { :conditions => [ 'media_objects.media_id = ? OR media_objects.old_media_id = ?', media_id, media_id ] } + } + + named_scope :by_media_type, lambda { |media_type| + { :conditions => [ 'media_objects.media_type = ?', media_type ]} + } + workflow do state :active state :deleted diff --git a/app/models/submission.rb b/app/models/submission.rb index 203f4977ee1..24cff803ab1 100644 --- a/app/models/submission.rb +++ b/app/models/submission.rb @@ -368,7 +368,7 @@ class Submission < ActiveRecord::Base end self.media_comment_id = nil if self.media_comment_id && self.media_comment_id.strip.empty? if self.media_comment_id && (self.media_comment_id_changed? || !self.media_object_id) - mo = MediaObject.find_by_media_id(self.media_comment_id) + mo = MediaObject.by_media_id(self.media_comment_id).first self.media_object_id = mo && mo.id end self.media_comment_type = nil unless self.media_comment_id diff --git a/lib/cc/cc_helper.rb b/lib/cc/cc_helper.rb index e8f687900a9..19bd28e696f 100644 --- a/lib/cc/cc_helper.rb +++ b/lib/cc/cc_helper.rb @@ -196,7 +196,7 @@ module CCHelper doc = Nokogiri::HTML::DocumentFragment.parse(html) doc.css('a.instructure_inline_media_comment').each do |anchor| media_id = anchor['id'].gsub(/^media_comment_/, '') - obj = course.media_objects.find_by_media_id(media_id) + obj = course.media_objects.by_media_id(media_id).first if obj && obj.context == course && migration_id = CCHelper.create_key(obj) @used_media_objects << obj info = CCHelper.media_object_info(obj, nil, media_object_flavor) diff --git a/spec/models/media_object_spec.rb b/spec/models/media_object_spec.rb new file mode 100644 index 00000000000..4de86d0d83d --- /dev/null +++ b/spec/models/media_object_spec.rb @@ -0,0 +1,35 @@ +# +# Copyright (C) 2011 Instructure, Inc. +# +# This file is part of Canvas. +# +# Canvas is free software: you can redistribute it and/or modify it under +# the terms of the GNU Affero General Public License as published by the Free +# Software Foundation, version 3 of the License. +# +# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +# details. +# +# You should have received a copy of the GNU Affero General Public License along +# with this program. If not, see . +# + +require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb') + +describe MediaObject do + context "loading with legacy support" do + it "should load by either media_id or old_media_id" do + course + mo = factory_with_protected_attributes(MediaObject, :media_id => '0_abcdefgh', :old_media_id => '1_01234567', :context => @course) + + MediaObject.by_media_id('0_abcdefgh').first.should == mo + MediaObject.by_media_id('1_01234567').first.should == mo + end + + it "should raise an error if someone tries to use find_by_media_id" do + lambda { MediaObject.find_by_media_id('fjdksl') }.should raise_error + end + end +end