be sure to always look up media objects correctly to support migrated content

Change-Id: I69d18407e08b38c6f3d320908485eb65c209cac6
Reviewed-on: https://gerrit.instructure.com/5811
Tested-by: Hudson <hudson@instructure.com>
Reviewed-by: Brian Palmer <brianp@instructure.com>

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

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

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

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

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

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)

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 <http://www.gnu.org/licenses/>.
+#
+
+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