From 7e954bf38ef2b668080c956a999bb5cd79463162 Mon Sep 17 00:00:00 2001
From: David Butler <dwbutler@ucla.edu>
Date: Tue, 14 Jun 2016 20:41:37 -0700
Subject: [PATCH 1/2] Fix member association filtering

When defining an association through a join table to a model using single table inheritance, ActiveRecord will always store the base class name in the join table. This allows the base class to always be correctly queried, but makes it challenging to correctly define an association that only returns a subclass.

Groupify groups define member associations using the `source_type` set correctly to the base class, but this returns all members of the base class instead of filtering to only members of the subclass. This fixes the issue by adding a filtering condition to the association on the `type` column, but only if STI is detected.
---
 lib/groupify/adapter/active_record/group.rb | 49 ++++++++++++++-------
 spec/active_record_spec.rb                  |  9 ++++
 2 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/lib/groupify/adapter/active_record/group.rb b/lib/groupify/adapter/active_record/group.rb
index b891029..d39a777 100644
--- a/lib/groupify/adapter/active_record/group.rb
+++ b/lib/groupify/adapter/active_record/group.rb
@@ -145,23 +145,7 @@ def associate_member_class(member_klass)
 
         def define_member_association(member_klass, association_name = nil)
           association_name ||= member_klass.model_name.plural.to_sym
-          source_type = member_klass.base_class
-
-          if ActiveSupport::VERSION::MAJOR > 3
-            has_many association_name,
-                     ->{ uniq },
-                     through: :group_memberships_as_group,
-                     source: :member,
-                     source_type: source_type,
-                     extend: MemberAssociationExtensions
-          else
-            has_many association_name,
-                     uniq: true,
-                     through: :group_memberships_as_group,
-                     source: :member,
-                     source_type: source_type,
-                     extend: MemberAssociationExtensions
-          end
+          has_many association_name, *association_options(member_klass)
 
           define_method(association_name) do |*args|
             opts = args.extract_options!
@@ -173,6 +157,37 @@ def define_member_association(member_klass, association_name = nil)
             end
           end
         end
+
+        def association_options(member_klass)
+          source_type = member_klass.base_class
+          using_sti = (member_klass.base_class != member_klass)
+
+          if ActiveSupport::VERSION::MAJOR > 3
+            filter = using_sti ? -> { uniq.where(type: member_klass.name.to_s) } : -> { uniq }
+
+            [
+              filter,
+              {
+                through: :group_memberships_as_group,
+                source: :member,
+                source_type: source_type,
+                extend: MemberAssociationExtensions
+              }
+            ]
+          else
+            options = {
+              uniq: true,
+              through: :group_memberships_as_group,
+              source: :member,
+              source_type: source_type,
+              extend: MemberAssociationExtensions
+            }
+            if using_sti
+              options.merge(conditions: { type: member_klass.name.to_s })
+            end
+            [options]
+          end
+        end
       end
     end
   end
diff --git a/spec/active_record_spec.rb b/spec/active_record_spec.rb
index 97e1652..df78d60 100644
--- a/spec/active_record_spec.rb
+++ b/spec/active_record_spec.rb
@@ -186,6 +186,15 @@ class ProjectMember < ActiveRecord::Base
         expect(group.namespaced_members).to include(namespaced_member)
       end
 
+      it "adds a model using STI to a group" do
+        manager = Manager.create!
+        user = User.create!
+        organization = Organization.create!
+        organization.add(manager, user)
+        expect(organization.users).to match_array [user, manager]
+        expect(organization.managers).to match_array [manager]
+      end
+
       it "adds multiple members to a group" do
         group.add(user, widget)
         expect(group.users).to include(user)

From 5e769a9961feb61b42f7785eba2242272afc304b Mon Sep 17 00:00:00 2001
From: David Butler <dwbutler@ucla.edu>
Date: Tue, 14 Jun 2016 20:48:17 -0700
Subject: [PATCH 2/2] Refactoring

---
 lib/groupify/adapter/active_record/group.rb | 26 +++++++++------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/lib/groupify/adapter/active_record/group.rb b/lib/groupify/adapter/active_record/group.rb
index d39a777..581e3e5 100644
--- a/lib/groupify/adapter/active_record/group.rb
+++ b/lib/groupify/adapter/active_record/group.rb
@@ -162,28 +162,24 @@ def association_options(member_klass)
           source_type = member_klass.base_class
           using_sti = (member_klass.base_class != member_klass)
 
+          base_options = {
+            through: :group_memberships_as_group,
+            source: :member,
+            source_type: source_type,
+            extend: MemberAssociationExtensions
+          }
+
           if ActiveSupport::VERSION::MAJOR > 3
-            filter = using_sti ? -> { uniq.where(type: member_klass.name.to_s) } : -> { uniq }
+            filter = using_sti ? -> { uniq.where("#{member_klass.table_name}.type = ?", member_klass.name.to_s) } : -> { uniq }
 
             [
               filter,
-              {
-                through: :group_memberships_as_group,
-                source: :member,
-                source_type: source_type,
-                extend: MemberAssociationExtensions
-              }
+              base_options
             ]
           else
-            options = {
-              uniq: true,
-              through: :group_memberships_as_group,
-              source: :member,
-              source_type: source_type,
-              extend: MemberAssociationExtensions
-            }
+            options = base_options.merge(uniq: true)
             if using_sti
-              options.merge(conditions: { type: member_klass.name.to_s })
+              options.merge!(conditions: ["#{member_klass.table_name}.type = ?", member_klass.name.to_s])
             end
             [options]
           end