Merge branch '39-count-unique-users-for-more-accurate-smau-reporting' into 'master'

Allow UsageData.count to use count_by:

See merge request gitlab-org/gitlab-ce!30770

5 files changed, 36 insertions, 16 deletions

diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index f6b19317c50..3d6f397e599 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -15,8 +15,8 @@ class GroupMember < Member
   default_scope { where(source_type: SOURCE_TYPE) }
 
   scope :of_groups, ->(groups) { where(source_id: groups.select(:id)) }
-
   scope :count_users_by_group_id, -> { joins(:user).group(:source_id).count }
+  scope :of_ldap_type, -> { where(ldap: true) }
 
   after_create :update_two_factor_requirement, unless: :invite?
   after_destroy :update_two_factor_requirement, unless: :invite?
diff --git a/lib/gitlab/usage_data.rb b/lib/gitlab/usage_data.rb
index 1542905d2ce..353298e67b3 100644
--- a/lib/gitlab/usage_data.rb
+++ b/lib/gitlab/usage_data.rb
@@ -188,8 +188,8 @@ module Gitlab
         {} # augmented in EE
       end
 
-      def count(relation, fallback: -1)
-        relation.count
+      def count(relation, count_by: nil, fallback: -1)
+        count_by ? relation.count(count_by) : relation.count
       rescue ActiveRecord::StatementInvalid
         fallback
       end
diff --git a/spec/factories/group_members.rb b/spec/factories/group_members.rb
index 8dab6c71b06..4c875935d82 100644
--- a/spec/factories/group_members.rb
+++ b/spec/factories/group_members.rb
@@ -20,5 +20,9 @@ FactoryBot.define do
         "email#{n}@email.com"
       end
     end
+
+    trait(:ldap) do
+      ldap true
+    end
   end
 end
diff --git a/spec/lib/gitlab/usage_data_spec.rb b/spec/lib/gitlab/usage_data_spec.rb
index 9bbd9394d57..64254674157 100644
--- a/spec/lib/gitlab/usage_data_spec.rb
+++ b/spec/lib/gitlab/usage_data_spec.rb
@@ -266,6 +266,12 @@ describe Gitlab::UsageData do
       expect(described_class.count(relation)).to eq(1)
     end
 
+    it 'returns the count for count_by when provided' do
+      allow(relation).to receive(:count).with(:creator_id).and_return(2)
+
+      expect(described_class.count(relation, count_by: :creator_id)).to eq(2)
+    end
+
     it 'returns the fallback value when counting fails' do
       allow(relation).to receive(:count).and_raise(ActiveRecord::StatementInvalid.new(''))
 
diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb
index ebb0bfca369..ad7dfac87af 100644
--- a/spec/models/members/group_member_spec.rb
+++ b/spec/models/members/group_member_spec.rb
@@ -3,19 +3,29 @@
 require 'spec_helper'
 
 describe GroupMember do
-  describe '.count_users_by_group_id' do
-    it 'counts users by group ID' do
-      user_1 = create(:user)
-      user_2 = create(:user)
-      group_1 = create(:group)
-      group_2 = create(:group)
-
-      group_1.add_owner(user_1)
-      group_1.add_owner(user_2)
-      group_2.add_owner(user_1)
-
-      expect(described_class.count_users_by_group_id).to eq(group_1.id => 2,
-                                                            group_2.id => 1)
+  context 'scopes' do
+    describe '.count_users_by_group_id' do
+      it 'counts users by group ID' do
+        user_1 = create(:user)
+        user_2 = create(:user)
+        group_1 = create(:group)
+        group_2 = create(:group)
+
+        group_1.add_owner(user_1)
+        group_1.add_owner(user_2)
+        group_2.add_owner(user_1)
+
+        expect(described_class.count_users_by_group_id).to eq(group_1.id => 2,
+                                                              group_2.id => 1)
+      end
+    end
+
+    describe '.of_ldap_type' do
+      it 'returns ldap type users' do
+        group_member = create(:group_member, :ldap)
+
+        expect(described_class.of_ldap_type).to eq([group_member])
+      end
     end
   end