Moved favs/voting out into managers to consolidate logic

While the logic was not particularly complex or spread out it was
made more complicated by the models being dependant on one another.

This creates a central entry point for voting and favorites and
coordinates all transactions.

TODO: upvote/downvote/fav metatags in tag section result in an error
because they attempt to change the transaction isolation mode during
an outer transaction.

app/controllers/favorites_controller.rb

@@ -25,7 +25,7 @@ class FavoritesController < ApplicationController
 
   def create
     @post = Post.find(params[:post_id])
-    @post.add_favorite!(CurrentUser.user)
+    FavoriteManager.add!(user: CurrentUser.user, post: @post)
     flash.now[:notice] = "You have favorited this post"
 
     respond_with(@post)
@@ -33,12 +33,7 @@ class FavoritesController < ApplicationController
 
   def destroy
     @post = Post.find_by_id(params[:id])
-
-    if @post
-      @post.remove_favorite!(CurrentUser.user)
-    else
-      Favorite.remove(post_id: params[:id], user: CurrentUser.user)
-    end
+    FavoriteManager.remove!(user: CurrentUser.user, post: @post, post_id: params[:id])
 
     flash.now[:notice] = "You have unfavorited this post"
     respond_with(@post)

app/controllers/post_votes_controller.rb

@@ -4,7 +4,7 @@ class PostVotesController < ApplicationController
 
   def create
     @post = Post.find(params[:post_id])
-    @post.vote!(params[:score])
+    VoteManager.vote!(post: @post, user: CurrentUser.user, score: params[:score])
   rescue PostVote::Error, ActiveRecord::RecordInvalid => x
     @error = x
     render status: 500
@@ -12,7 +12,7 @@ class PostVotesController < ApplicationController
 
   def destroy
     @post = Post.find(params[:post_id])
-    @post.unvote!
+    VoteManager.unvote!(post: @post, user: CurrentUser.user)
   rescue PostVote::Error => x
     @error = x
     render status: 500

app/jobs/index_update_job.rb

@@ -2,7 +2,7 @@
 
 class IndexUpdateJob
   include Sidekiq::Worker
-  sidekiq_options queue: 'high_prio', lock: :until_executed, unique_args: ->(args) { args[1] }
+  sidekiq_options queue: 'high_prio', lock: :until_executing, unique_args: ->(args) { args[1] }
 
   def perform(klass, id)
     obj = klass.constantize.find(id)

app/jobs/transfer_favorites_job.rb

@@ -0,0 +1,14 @@
+class TransferFavoritesJob < ApplicationJob
+  queue_as :low_prio
+
+  def perform(*args)
+    without_mod_action = args[2]
+    @post = Post.find(args[0])
+    @user = User.find(args[1])
+
+    CurrentUser.as(@user) do
+      @post.give_favorites_to_parent!(without_mod_action: without_mod_action)
+    end
+  end
+
+end

app/jobs/user_deletion_job.rb

@@ -13,7 +13,7 @@ class UserDeletionJob < ApplicationJob
   def remove_favorites(user)
     Favorite.without_timeout do
       Favorite.for_user(user.id).includes(:post).find_each do |fav|
-        Favorite.remove!(post: fav.post, user: user)
+        FavoriteManager.remove!(user: user, post: post)
       end
     end
   end

app/logical/favorite_manager.rb

@@ -0,0 +1,44 @@
+class FavoriteManager
+  def self.add!(user:, post:, force: false)
+    begin
+      Favorite.transaction(isolation: :serializable) do
+        unless force
+          if user.favorite_count >= user.favorite_limit
+            raise Favorite::Error, "You can only keep up to #{user.favorite_limit} favorites. Upgrade your account to save more."
+          end
+        end
+
+        Favorite.create!(:user_id => user.id, :post_id => post.id)
+        post.append_user_to_fav_string(user.id)
+        post.save
+      end
+    rescue ActiveRecord::RecordNotUnique
+      raise Favorite::Error, "You have already favorited this post" unless force
+    end
+  end
+
+  def self.remove!(user:, post:, post_id: nil)
+    post_id = post ? post.id : post_id
+    raise Favorite::Error, "Must specify a post or post_id to remove favorite" unless post_id
+    Favorite.transaction(isolation: :serializable) do
+      unless Favorite.for_user(user.id).where(:user_id => user.id, :post_id => post_id).exists?
+        return
+      end
+
+      Favorite.for_user(user.id).where(post_id: post_id).destroy_all
+      post.delete_user_from_fav_string(user.id) if post
+      post.save if post
+    end
+  end
+
+  def self.give_to_parent!(post)
+    # TODO Much better and more intelligent logic can exist for this
+    parent = post.parent
+    return false unless parent
+    post.favorites.each do |fav|
+      FavoriteManager.remove!(user: fav.user, post: post)
+      FavoriteManager.add!(user: fav.user, post: parent, force: true)
+    end
+    true
+  end
+end

app/logical/vote_manager.rb

@@ -0,0 +1,41 @@
+class VoteManager
+  def self.vote!(user:, post:, score: "locked")
+    begin
+      PostVote.transaction(isolation: :serializable) do
+        unless user.is_voter?
+          raise PostVote::Error.new("You do not have permission to vote")
+        end
+
+        vote = post.votes.create!(user: user, vote: score)
+        vote_cols = "score = score + #{vote.score}"
+        if vote.score > 0
+          vote_cols += ", up_score = up_score + #{vote.score}"
+        else
+          vote_cols += ", down_score = down_score + #{vote.score}"
+        end
+        Post.where(id: post.id).update_all(vote_cols)
+        post.reload
+      end
+      post.update_index
+    rescue ActiveRecord::RecordNotUnique
+      raise PostVote::Error.new("You have already voted for this post")
+    end
+  end
+
+  def self.unvote!(user:, post:)
+    PostVote.transaction(isolation: :serializable) do
+      vote = post.votes.where(user: user).first
+      return unless vote
+      post.votes.where(user: user).delete_all
+      vote_cols = "score = score - #{vote.score}"
+      if vote.score > 0
+        vote_cols += ", up_score = up_score - #{vote.score}"
+      else
+        vote_cols += ", down_score = down_score - #{vote.score}"
+      end
+      Post.where(id: post.id).update_all(vote_cols)
+      post.reload
+    end
+    post.update_index
+  end
+end

app/models/favorite.rb

@@ -1,73 +1,8 @@
 class Favorite < ApplicationRecord
-  class Error < Exception;
+  class Error < Exception
   end
 
   belongs_to :post
-  belongs_to :user
+  belongs_to :user, counter_cache: 'favorite_count'
   scope :for_user, ->(user_id) {where("user_id = #{user_id.to_i}")}
-
-  def self.add(post:, user:)
-    ckey = "fav:#{user.id}:#{post.id}"
-    raise Error.new("You are already favoriting") if !Cache.get(ckey).nil?
-    begin
-      Cache.put(ckey, true, 30)
-      Favorite.add!(user: user, post: post)
-    ensure
-      Cache.delete(ckey)
-    end
-  end
-
-  def self.add!(post:, user:)
-    Favorite.transaction do
-      User.where(:id => user.id).select("id").lock("FOR UPDATE NOWAIT").first
-
-      if user.favorite_count >= user.favorite_limit
-        raise Error, "You can only keep up to #{user.favorite_limit} favorites. Upgrade your account to save more."
-      elsif Favorite.for_user(user.id).where(:user_id => user.id, :post_id => post.id).exists?
-        raise Error, "You have already favorited this post"
-      end
-
-      post.with_lock do
-        Favorite.create!(:user_id => user.id, :post_id => post.id)
-        post.append_user_to_fav_string(user.id)
-        post.save
-      end
-      User.where(:id => user.id).update_all("favorite_count = favorite_count + 1")
-      user.favorite_count += 1
-    end
-  end
-
-  def self.remove(user:, post: nil, post_id: nil)
-    ckey = "fav:#{user.id}:#{post.id}"
-    raise Error.new("You are already favoriting") if !Cache.get(ckey).nil?
-    begin
-      Cache.put(ckey, true, 30)
-      Favorite.remove!(user: user, post: post, post_id: post_id)
-    ensure
-      Cache.delete(ckey)
-    end
-  end
-
-  def self.remove!(user:, post: nil, post_id: nil)
-    Favorite.transaction do
-      if post && post_id.nil?
-        post_id = post.id
-      end
-      post = Post.find(post_id) unless post
-
-      User.where(:id => user.id).select("id").lock("FOR UPDATE NOWAIT").first
-
-      unless Favorite.for_user(user.id).where(:user_id => user.id, :post_id => post_id).exists?
-        return
-      end
-
-      post.with_lock do
-        Favorite.for_user(user.id).where(post_id: post.id).delete_all
-        post.delete_user_from_fav_string(user.id)
-        post.save
-      end
-      User.where(:id => user.id).update_all("favorite_count = favorite_count - 1")
-      user.favorite_count -= 1
-    end
-  end
 end

app/models/post.rb

@@ -861,13 +861,13 @@ class Post < ApplicationRecord
           add_pool!(pool) if pool
 
         when /^fav:(.+)$/i
-          add_favorite!(CurrentUser.user)
+          FavoriteManager.add!(user: CurrentUser.user, post: self)
 
         when /^-fav:(.+)$/i
-          remove_favorite!(CurrentUser.user)
+          FavoriteManager.remove!(user: CurrentUser.user, post: self)
 
         when /^(up|down)vote:(.+)$/i
-          vote!($1)
+          VoteManager.vote!(user: CurrentUser.user, post: self, score: $1)
 
         when /^child:none$/i
           children.each do |post|
@@ -978,30 +978,11 @@ class Post < ApplicationRecord
       clean_fav_string!
     end
 
-    def add_favorite(user)
-      add_favorite!(user)
-      true
-    rescue Favorite::Error
-      false
-    end
-
-    def add_favorite!(user)
-      Favorite.add(post: self, user: user)
-      reload
-    rescue PostVote::Error
-    end
-
     def delete_user_from_fav_string(user_id)
       self.fav_string = fav_string.gsub(/(?:\A| )fav:#{user_id}(?:\Z| )/, " ").strip
       clean_fav_string!
     end
 
-    def remove_favorite!(user)
-      Favorite.remove(post: self, user: user)
-      reload
-    rescue PostVote::Error
-    end
-
     # users who favorited this post, ordered by users who favorited it first
     def favorited_users
       favorited_user_ids = fav_string.scan(/\d+/).map(&:to_i)
@@ -1139,47 +1120,6 @@ class Post < ApplicationRecord
     def can_be_voted_by?(user)
       !PostVote.exists?(:user_id => user.id, :post_id => id)
     end
-
-    def vote!(vote, voter = CurrentUser.user)
-      ckey = "vote:#{voter.id}:#{id}"
-      raise PostVote::Error.new('You are already voting') if !Cache.get(ckey).nil?
-      begin
-        Cache.put(ckey, true, 30)
-        PostVote.transaction do
-          unless voter.is_voter?
-            raise PostVote::Error.new("You do not have permission to vote")
-          end
-
-          unless can_be_voted_by?(voter)
-            raise PostVote::Error.new("You have already voted for this post")
-          end
-
-          votes.create!(user: voter, vote: vote)
-          reload # PostVote.create modifies our score. Reload to get the new score.
-        end
-      ensure
-        Cache.delete(ckey)
-      end
-    end
-
-    def unvote!(voter = CurrentUser.user)
-      ckey = "vote:#{voter.id}:#{id}"
-      raise PostVote::Error.new('You are already voting') if !Cache.get(ckey).nil?
-      begin
-        Cache.put(ckey, true, 30)
-        PostVote.transaction do
-          if can_be_voted_by?(voter)
-            Cache.delete(ckey)
-            raise PostVote::Error.new("You have not voted for this post")
-          else
-            votes.where(user: voter).destroy_all
-            reload
-          end
-        end
-      ensure
-        Cache.delete(ckey)
-      end
-    end
   end
 
   module CountMethods
@@ -1327,14 +1267,13 @@ class Post < ApplicationRecord
     end
 
     def give_favorites_to_parent(options = {})
+      TransferFavoritesJob.perform_later(id, CurrentUser.id, options[:without_mod_action])
+    end
+
+    def give_favorites_to_parent!(options = {})
       return if parent.nil?
 
-      transaction do
-        favorites.each do |fav|
-          remove_favorite!(fav.user)
-          parent.add_favorite(fav.user)
-        end
-      end
+      FavoriteManager.give_to_parent!(self)
 
       unless options[:without_mod_action]
         ModAction.log("moved favorites from post ##{id} to post ##{parent.id}", :post_move_favorites)

app/models/post_vote.rb

@@ -7,13 +7,7 @@ class PostVote < ApplicationRecord
 
   after_initialize :initialize_attributes, if: :new_record?
   validates_presence_of :post_id, :user_id, :score
-  validates_inclusion_of :score, :in => [SuperVoter::MAGNITUDE, 1, -1, -SuperVoter::MAGNITUDE]
-  after_create :update_post_on_create
-  after_destroy :update_post_on_destroy
-
-  def self.prune!
-    where("created_at < ?", 90.days.ago).delete_all
-  end
+  validates_inclusion_of :score, :in => [SuperVoter::MAGNITUDE, 1, 0, -1, -SuperVoter::MAGNITUDE]
 
   def self.positive_user_ids
     select_values_sql("select user_id from post_votes where score > 0 group by user_id having count(*) > 100")
@@ -34,34 +28,8 @@ class PostVote < ApplicationRecord
       self.score = magnitude
     elsif vote == "down"
       self.score = -magnitude
-    end
-  end
-
-  def update_post_on_create
-    post = Post.find(post_id)
-    return unless post
-    post.with_lock do
-      post.score += score
-      if score > 0
-        post.up_score += score
-      else
-        post.down_score += score
-      end
-      post.save
-    end
-  end
-
-  def update_post_on_destroy
-    post = Post.find(post_id)
-    return unless post
-    post.with_lock do
-      post.score -= score
-      if score > 0
-        post.up_score -= score
-      else
-        post.down_score -= score
-      end
-      post.save
+    elsif vote == "locked"
+      self.score = 0
     end
   end
 

db/migrate/20190331193644_better_fav_votes_indexing.rb

@@ -0,0 +1,10 @@
+class BetterFavVotesIndexing < ActiveRecord::Migration[5.2]
+  def change
+    remove_index :favorites, :user_id
+    remove_index :favorites, :post_id
+    remove_index :post_votes, :user_id
+    remove_index :post_votes, :post_id
+    add_index :favorites, [:user_id, :post_id], unique: true
+    add_index :post_votes, [:user_id, :post_id], unique: true
+  end
+end

db/structure.sql

@@ -1055,6 +1055,40 @@ CREATE SEQUENCE public.forum_topics_id_seq
 ALTER SEQUENCE public.forum_topics_id_seq OWNED BY public.forum_topics.id;
 
 
+--
+-- Name: help_pages; Type: TABLE; Schema: public; Owner: -
+--
+
+CREATE TABLE public.help_pages (
+    id bigint NOT NULL,
+    created_at timestamp without time zone NOT NULL,
+    updated_at timestamp without time zone NOT NULL,
+    name character varying NOT NULL,
+    wiki_page character varying NOT NULL,
+    related character varying,
+    title character varying
+);
+
+
+--
+-- Name: help_pages_id_seq; Type: SEQUENCE; Schema: public; Owner: -
+--
+
+CREATE SEQUENCE public.help_pages_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+
+--
+-- Name: help_pages_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: -
+--
+
+ALTER SEQUENCE public.help_pages_id_seq OWNED BY public.help_pages.id;
+
+
 --
 -- Name: ip_bans; Type: TABLE; Schema: public; Owner: -
 --
@@ -2565,6 +2599,13 @@ ALTER TABLE ONLY public.forum_topic_visits ALTER COLUMN id SET DEFAULT nextval('
 ALTER TABLE ONLY public.forum_topics ALTER COLUMN id SET DEFAULT nextval('public.forum_topics_id_seq'::regclass);
 
 
+--
+-- Name: help_pages id; Type: DEFAULT; Schema: public; Owner: -
+--
+
+ALTER TABLE ONLY public.help_pages ALTER COLUMN id SET DEFAULT nextval('public.help_pages_id_seq'::regclass);
+
+
 --
 -- Name: ip_bans id; Type: DEFAULT; Schema: public; Owner: -
 --
@@ -3012,6 +3053,14 @@ ALTER TABLE ONLY public.forum_topics
     ADD CONSTRAINT forum_topics_pkey PRIMARY KEY (id);
 
 
+--
+-- Name: help_pages help_pages_pkey; Type: CONSTRAINT; Schema: public; Owner: -
+--
+
+ALTER TABLE ONLY public.help_pages
+    ADD CONSTRAINT help_pages_pkey PRIMARY KEY (id);
+
+
 --
 -- Name: ip_bans ip_bans_pkey; Type: CONSTRAINT; Schema: public; Owner: -
 --
@@ -3593,17 +3642,10 @@ CREATE INDEX index_favorite_groups_on_lower_name ON public.favorite_groups USING
 
 
 --
--- Name: index_favorites_on_post_id; Type: INDEX; Schema: public; Owner: -
+-- Name: index_favorites_on_user_id_and_post_id; Type: INDEX; Schema: public; Owner: -
 --
 
-CREATE INDEX index_favorites_on_post_id ON public.favorites USING btree (post_id);
-
-
---
--- Name: index_favorites_on_user_id; Type: INDEX; Schema: public; Owner: -
---
-
-CREATE INDEX index_favorites_on_user_id ON public.favorites USING btree (user_id);
+CREATE UNIQUE INDEX index_favorites_on_user_id_and_post_id ON public.favorites USING btree (user_id, post_id);
 
 
 --
@@ -3915,17 +3957,10 @@ CREATE INDEX index_post_replacements_on_post_id ON public.post_replacements USIN
 
 
 --
--- Name: index_post_votes_on_post_id; Type: INDEX; Schema: public; Owner: -
+-- Name: index_post_votes_on_user_id_and_post_id; Type: INDEX; Schema: public; Owner: -
 --
 
-CREATE INDEX index_post_votes_on_post_id ON public.post_votes USING btree (post_id);
-
-
---
--- Name: index_post_votes_on_user_id; Type: INDEX; Schema: public; Owner: -
---
-
-CREATE INDEX index_post_votes_on_user_id ON public.post_votes USING btree (user_id);
+CREATE UNIQUE INDEX index_post_votes_on_user_id_and_post_id ON public.post_votes USING btree (user_id, post_id);
 
 
 --
@@ -4589,6 +4624,8 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20190228144206'),
 ('20190305165101'),
 ('20190313221440'),
-('20190317024446');
+('20190317024446'),
+('20190324111703'),
+('20190331193644');