fix a few cross-shard collection item stuffs

* don't use set_shard_override, just set shard= directly
 * when creating follows, run the transaction, the search, and
   the create all on the same shard
 * when searching for upvotes, root on the user, not the
   collection item

Change-Id: I5ae3f29e5f48f92c02d3a0c0e04765956d8eb892
Reviewed-on: https://gerrit.instructure.com/14648
Reviewed-by: Cody Cutrer <cody@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>

app/controllers/collection_items_controller.rb

@@ -379,6 +379,6 @@ class CollectionItemsController < ApplicationController
   end
 
   def find_upvote
-    @item.collection_item_data.collection_item_upvotes.find_by_user_id(@current_user.id)
+    @current_user.collection_item_upvotes.find_by_collection_item_data_id(@item.collection_item_data_id)
   end
 end

app/models/collection_item_data.rb

@@ -21,7 +21,8 @@ class CollectionItemData < ActiveRecord::Base
 
   belongs_to :root_item, :class_name => "CollectionItem"
   belongs_to :image_attachment, :class_name => "Attachment"
-  has_many :collection_item_upvotes
+  # belongs with the user, so avoid accessing from here
+  # has_many :collection_item_upvotes
 
   VALID_ITEM_TYPES = %w(url image audio video)
   THUMBNAIL_SIZE = "640x>"

app/models/collection_item_upvote.rb

@@ -30,9 +30,11 @@ class CollectionItemUpvote < ActiveRecord::Base
 
   # upvotes get saved to the user's shard, and then increment the counter
   # stored on the collection_item_data, wherever it may live
-  set_shard_override do |record|
-    record.user.shard
+  def user_with_sharding=(user)
+    self.shard = user.shard
+    self.user_without_sharding = user
   end
+  alias_method_chain :user=, :sharding
 
   def update_upvote_count
     increment = 0

app/models/user.rb

@@ -162,6 +162,7 @@ class User < ActiveRecord::Base
 
   has_many :collections, :as => :context
   has_many :collection_items, :through => :collections
+  has_many :collection_item_upvotes
 
   has_one :profile, :class_name => 'UserProfile'
   alias :orig_profile :profile

app/models/user_follow.rb

@@ -34,9 +34,11 @@ class UserFollow < ActiveRecord::Base
   # while creating the complementary record on the other shard.
   def self.create_follow(following_user, followed_item, complementary_record = false)
     search_shard = (complementary_record ? followed_item : following_user).shard
-    UserFollow.unique_constraint_retry do
-      user_follow = search_shard.activate { UserFollow.first(:conditions => { :following_user_id => following_user.id, :followed_item_id => followed_item.id, :followed_item_type => followed_item.class.name }) }
-      user_follow ||= UserFollow.create(:following_user => following_user, :followed_item => followed_item) { |uf| uf.complementary_record = complementary_record }
+    search_shard.activate do
+      UserFollow.unique_constraint_retry do
+        user_follow = UserFollow.first(:conditions => { :following_user_id => following_user.id, :followed_item_id => followed_item.id, :followed_item_type => followed_item.class.name })
+        user_follow ||= UserFollow.create(:following_user => following_user, :followed_item => followed_item)
+      end
     end
   end
 
@@ -62,15 +64,6 @@ class UserFollow < ActiveRecord::Base
     return true
   end
 
-  # we force the record to be created on the same shard as the following_user
-  # then the after_create below creates a duplicate, complementary record on
-  # the shard of the followed_item, if it's on a different shard
-  #
-  # this way both associations work as expected
-  set_shard_override do |record|
-    record.following_user.shard unless record.complementary_record?
-  end
-
   after_create :create_complementary_record
   attr_writer :complementary_record
 
@@ -78,18 +71,12 @@ class UserFollow < ActiveRecord::Base
   # item, and this UserFollow is the secondary copy that's on the followed
   # item's shard
   def complementary_record?
-    if new_record?
-      @complementary_record
-    else
-      self.shard != following_user.shard
-    end
+    self.shard != following_user.shard
   end
 
   def create_complementary_record
     if !complementary_record? && followed_item.shard != following_user.shard
-      followed_item.shard.activate do
-        UserFollow.create_follow(following_user, followed_item, true)
-      end
+      UserFollow.create_follow(following_user, followed_item, true)
     end
     true
   end

lib/stubs/shard.rb

@@ -75,7 +75,7 @@ ActiveRecord::Base.class_eval do
   end
 
   def shard=(new_shard)
-    raise ReadOnlyRecord unless new_record?
+    raise ReadOnlyRecord if new_record? && self.shard != new_shard
     new_shard
   end
 
@@ -86,8 +86,4 @@ ActiveRecord::Base.class_eval do
   def local_id
     id
   end
-
-  def self.set_shard_override(&block)
-    # pass
-  end
 end

spec/apis/v1/collections_spec.rb

@@ -463,7 +463,7 @@ describe "Collections API", :type => :integration do
         end
 
         it "should allow removing an upvote" do
-          @i3.collection_item_data.collection_item_upvotes.create!(:user => @user)
+          @user.collection_item_upvotes.create!(:collection_item_data => @i3.collection_item_data)
           @i3.reload.collection_item_data.upvote_count.should == 1
           json = api_call(:delete, "#{@unscoped_items_path}/#{@i3.id}/upvotes/self", @unscoped_items_path_options.merge(:action => "remove_upvote", :item_id => @i3.to_param))
           @i3.reload.collection_item_data.upvote_count.should == 0

spec/models/collection_item_spec.rb

@@ -47,6 +47,7 @@ describe 'CollectionItem' do
       @shard2.activate { @item = collection_item_model(:collection => group_model.collections.create!, :user => user_model) }
       @upvote = CollectionItemUpvote.create!(:user => @user1, :collection_item_data => @item.data)
       @upvote.shard.should == @user1.shard
+      @upvote.reload.user.should == @user1
       @item.data.reload.upvote_count.should == 1
 
       [ Shard.default, @shard1, @shard2 ].each do |shard|

spec/models/user_follow_spec.rb

@@ -112,7 +112,7 @@ describe "UserFollow" do
       it_should_behave_like "sharded user following"
 
       before do
-        @uf = @user1.user_follows.create!(:followed_item => @collection)
+        @uf = UserFollow.create_follow(@user1, @collection)
         @uf.shard.should == @user1.shard
       end
     end
@@ -121,7 +121,7 @@ describe "UserFollow" do
       it_should_behave_like "sharded user following"
 
       before do
-        @uf = @collection.shard.activate { UserFollow.create!(:following_user => @user1, :followed_item => @collection) }
+        @uf = @collection.shard.activate { UserFollow.create_follow(@user1, @collection) }
         @uf.shard.should == @user1.shard
       end
     end
@@ -130,7 +130,7 @@ describe "UserFollow" do
       it_should_behave_like "sharded user following"
 
       before do
-        @uf = @shard2.activate { UserFollow.create!(:following_user => @user1, :followed_item => @collection) }
+        @uf = @shard2.activate { UserFollow.create_follow(@user1, @collection) }
         @uf.shard.should == @user1.shard
       end
     end