simplify flag-cache

app/models/ability.rb

@@ -100,11 +100,8 @@ def initialize(user)
   end
 
   def flagged?(flaggable)
-    if flag_cache && flag_cache.user == @user && flag_cache.knows?(flaggable)
-      flag_cache.flagged?(flaggable)
-    else
-      Flag.flagged?(@user, flaggable)
-    end
+    is_flagged = flag_cache && flag_cache.flagged?(flaggable)
+    is_flagged.nil? ? Flag.flagged?(@user, flaggable) : is_flagged
   end
 
 end

app/models/flag.rb

@@ -10,7 +10,9 @@ class Flag < ActiveRecord::Base
   end)
 
   scope(:by_user_and_flaggables, lambda do |user, flaggables|
-    where(build_query_for_user_and_flaggables(user, flaggables))
+    return where('FALSE') if flaggables.empty? || user.nil? || user.id.nil?
+    where([ "flags.user_id = ? and flags.flaggable_type = ? and flags.flaggable_id IN (?)",
+            user.id, flaggables.first.class.name, flaggables.collect(&:id) ])
   end)
 
   def self.flag!(user, flaggable)
@@ -35,27 +37,12 @@ def initialize(user, flaggables)
       @user = user
       @cache = {}
       flags = Flag.by_user_and_flaggables(user, flaggables)
-
-      # Fill up the "trues"
-      flags.each do |flag|
-        @cache[flag.flaggable_type] ||= {}
-        @cache[flag.flaggable_type][flag.flaggable_id] = true
-      end
-
-      # Fill up the "falses"
-      flaggables.each do |flaggable|
-        type = flaggable.class.name
-        @cache[type] ||= {}
-        @cache[type][flaggable.id] = !! @cache[type][flaggable.id]
-      end
+      flags.each{ |flag| @cache[flag.flaggable_id] = true }
+      flaggables.each{ |flaggable| @cache[flaggable.id] = !! @cache[flaggable.id] }
     end
 
     def flagged?(flaggable)
-      @cache[flaggable.class.name].to_h[flaggable.id]
-    end
-
-    def knows?(flaggable)
-      flagged?(flaggable) != nil
+      @cache[flaggable.id]
     end
   end
 
@@ -70,25 +57,4 @@ def initialize
       super "The flaggable was not flagged by this user"
     end
   end
-
-  private
-    def self.build_query_for_user_and_flaggables(user, flaggables)
-      return ['FALSE'] if flaggables.empty?
-
-      ids_by_type = {}
-      flaggables.each do |flaggable|
-        type = flaggable.class.name
-        ids_by_type[type] ||= []
-        ids_by_type[type] << flaggable.id
-      end
-
-      query_strings = []
-      query_values = []
-      ids_by_type.each do |type, ids|
-        query_strings << "(flags.flaggable_type = ? AND flags.flaggable_id IN (?))"
-        query_values << type << ids
-      end
-      query_string = "(flags.user_id = ?) AND (#{query_strings.join(' OR ')})"
-      [query_string, user.try(:id)] + query_values
-    end
 end

spec/models/ability_spec.rb

@@ -205,30 +205,24 @@
   describe '#flagged?' do
     let(:user) { create(:user) }
     let(:comment) { create(:comment) }
-    it 'calls Flag.flagged? when the flag_cache is not set' do
-      expect(Flag).to receive(:flagged?).with(user, comment).and_return(false)
-      expect(ability.flagged?(comment)).to_not be
-    end
 
-    it 'calls Flag.flagged? when the flag_cache is set but its user is different' do
-      ability.flag_cache = double(:cache, user: create(:user))
+    it 'calls Flag.flagged? when the flag_cache is not set' do
       expect(Flag).to receive(:flagged?).with(user, comment).and_return(false)
-      expect(ability.flagged?(comment)).to_not be
+      ability.flagged?(comment)
     end
 
     it 'calls Flag.flagged? when the flag_cache is set, but it does not know the flaggable' do
       ability.flag_cache = double(:cache, user: user)
-      expect(ability.flag_cache).to receive(:knows?).with(comment).and_return(false)
+      expect(ability.flag_cache).to receive(:flagged?).with(comment).and_return(nil)
       expect(Flag).to receive(:flagged?).with(user, comment).and_return(false)
-      expect(ability.flagged?(comment)).to_not be
+      ability.flagged?(comment)
     end
 
-    it 'calls flag_cache.flagged? when the flag_cache is set, the user is correct, and it knows the flaggable' do
+    it 'calls flag_cache.flagged? when the flag_cache is set and it knows the flaggable' do
       ability.flag_cache = double(:cache, user: user)
-      expect(ability.flag_cache).to receive(:knows?).with(comment).and_return(true)
-      expect(ability.flag_cache).to receive(:flagged?).with(comment).and_return(false)
+      expect(ability.flag_cache).to receive(:flagged?).with(comment).and_return(true)
       expect(Flag).to_not receive(:flagged?)
-      expect(ability.flagged?(comment)).to_not be
+      ability.flagged?(comment)
     end
   end
 end

spec/models/flag_spec.rb

@@ -53,73 +53,32 @@
     end
   end
 
-  describe '.by_user_and_flaggables' do
-    let(:user) { create(:user) }
-    let(:debate) { create(:debate) }
-    let(:comment) { create(:comment) }
-    it 'returns an empty scope when no flaggables are given' do
-      Flag.flag!(user, create(:debate))
-      expect(Flag.by_user_and_flaggables(user, [])).to be_empty
-    end
-    it 'returns an empty list of flags if there are no flags' do
-      expect(Flag.by_user_and_flaggables(user, [debate, comment])).to be_empty
-    end
-
-    it 'builds a single query to retrieve all the flags' do
-      Flag.flag!(user, debate)
-      Flag.flag!(user, comment)
-      flags = Flag.by_user_and_flaggables(user, [debate, comment])
-      expect(flags.count).to eq(2)
-      expect(flags.pluck(:flaggable_type).sort).to eq(['Comment', 'Debate'])
-      expect(flags.pluck(:flaggable_id)).to include(debate.id, comment.id)
-    end
-  end
-
-
   describe Flag::Cache do
     let(:user) { create(:user) }
-    let(:debate) { create(:debate) }
-    let(:comment) { create(:comment) }
+    let(:debate1) { create(:debate) }
+    let(:debate2) { create(:debate) }
 
     it 'accepts a user and a collection of flaggables' do
-      expect{ Flag::Cache.new(user, [comment, debate]) }.to_not raise_error
+      expect{ Flag::Cache.new(user, [debate2, debate1]) }.to_not raise_error
     end
 
     describe '#flagged?' do
       it 'returns false if the item was not flagged by the user' do
-        cache = Flag::Cache.new(user, [debate, comment])
-        expect(cache.flagged?(debate)).to eq(false)
+        cache = Flag::Cache.new(user, [debate1, debate2])
+        expect(cache.flagged?(debate1)).to eq(false)
       end
 
       it 'returns true if the item was flagged by the user' do
-        Flag.flag!(user, debate)
-        cache = Flag::Cache.new(user, [debate, comment])
-        expect(cache.flagged?(debate)).to eq(true)
-        expect(cache.flagged?(comment)).to eq(false)
+        Flag.flag!(user, debate1)
+        cache = Flag::Cache.new(user, [debate1, debate2])
+        expect(cache.flagged?(debate1)).to eq(true)
+        expect(cache.flagged?(debate2)).to eq(false)
       end
 
       it 'returns nil if the item was not used for building the cache' do
-        cache = Flag::Cache.new(user, [comment])
-        expect(cache.flagged?(debate)).to eq(nil)
+        cache = Flag::Cache.new(user, [debate2])
+        expect(cache.flagged?(debate1)).to eq(nil)
       end
     end
-
-    describe '#knows?' do
-      it 'returns false if the flaggable was not used for building the cache' do
-        cache = Flag::Cache.new(user, [debate])
-        expect(cache.knows?(comment)).to eq(false)
-      end
-
-      it 'returns true if the flaggable was used for building the cache, independently of wether it was flagged or not' do
-        cache = Flag::Cache.new(user, [debate])
-        expect(cache.knows?(debate)).to eq(true)
-
-        Flag.flag!(user, debate)
-        cache = Flag::Cache.new(user, [debate])
-        expect(cache.knows?(debate)).to eq(true)
-      end
-    end
-
   end
-
 end