Merge branch '210559-follow-up-from-optimize-zoommeeting-batch-query' into 'master'

Resolve "Follow-up from "Optimize ZoomMeeting batch query""

Closes #210559

See merge request gitlab-org/gitlab!27236

app/models/concerns/usage_statistics.rb

+# frozen_string_literal: true
+
+module UsageStatistics
+  extend ActiveSupport::Concern
+
+  class_methods do
+    def distinct_count_by(column = nil, fallback = -1)
+      distinct.count(column)
+    rescue ActiveRecord::StatementInvalid
+      fallback
+    end
+  end
+end

app/models/zoom_meeting.rb

 # frozen_string_literal: true
 
 class ZoomMeeting < ApplicationRecord
+  include UsageStatistics
+
   belongs_to :project, optional: false
   belongs_to :issue, optional: false
 
@@ -23,10 +25,4 @@ class ZoomMeeting < ApplicationRecord
   def self.canonical_meeting_url(issue)
     canonical_meeting(issue)&.url
   end
-
-  def self.distinct_count_by(column = nil, fallback = -1)
-    distinct.count(column)
-  rescue ActiveRecord::StatementInvalid
-    fallback
-  end
 end

ee/app/models/approval_merge_request_rule.rb

@@ -3,7 +3,7 @@
 class ApprovalMergeRequestRule < ApplicationRecord
   include Gitlab::Utils::StrongMemoize
   include ApprovalRuleLike
-  include EE::UsageStatistics # rubocop: disable Cop/InjectEnterpriseEditionModule
+  include UsageStatistics
 
   scope :not_matching_pattern, -> (pattern) { code_owner.where.not(name: pattern) }
   scope :matching_pattern, -> (pattern) { code_owner.where(name: pattern) }

ee/app/models/concerns/ee/usage_statistics.rb

-# frozen_string_literal: true
-
-module EE
-  module UsageStatistics
-    extend ActiveSupport::Concern
-
-    class_methods do
-      def distinct_count_by(column = nil, fallback = -1)
-        distinct.count(column)
-      rescue ActiveRecord::StatementInvalid
-        fallback
-      end
-    end
-  end
-end

ee/app/models/ldap_key.rb

 # frozen_string_literal: true
 
 class LDAPKey < Key
-  include EE::UsageStatistics # rubocop: disable Cop/InjectEnterpriseEditionModule
+  include UsageStatistics
 
   def can_delete?
     false

spec/lib/gitlab/usage_data_spec.rb

@@ -387,6 +387,22 @@ describe Gitlab::UsageData do
           expect(described_class.count(relation, fallback: 15, batch: false)).to eq(15)
         end
       end
+
+      describe '#distinct_count' do
+        let(:relation) { double(:relation) }
+
+        it 'returns the count when counting succeeds' do
+          allow(relation).to receive(:distinct_count_by).and_return(1)
+
+          expect(described_class.distinct_count(relation, batch: false)).to eq(1)
+        end
+
+        it 'returns the fallback value when counting fails' do
+          allow(relation).to receive(:distinct_count_by).and_raise(ActiveRecord::StatementInvalid.new(''))
+
+          expect(described_class.distinct_count(relation, fallback: 15, batch: false)).to eq(15)
+        end
+      end
     end
   end
 end

ee/spec/models/concerns/ee/usage_statistics_spec.rb → spec/models/concerns/usage_statistics_spec.rb

@@ -2,31 +2,34 @@
 
 require 'spec_helper'
 
-describe EE::UsageStatistics do
+describe UsageStatistics do
   describe '.distinct_count_by' do
-    let(:user_1) { create(:user) }
-    let(:user_2) { create(:user) }
+    let_it_be(:issue_1) { create(:issue) }
+    let_it_be(:issue_2) { create(:issue) }
 
-    context 'two records created by the same user' do
-      let!(:models_created_by_user_1) { create_list(:group_member, 2, user: user_1) }
+    context 'two records created by the same issue' do
+      let!(:models_created_by_issue) do
+        create(:zoom_meeting, :added_to_issue, issue: issue_1)
+        create(:zoom_meeting, :removed_from_issue, issue: issue_1)
+      end
 
       it 'returns a count of 1' do
-        expect(::GroupMember.distinct_count_by(:user_id)).to eq(1)
+        expect(::ZoomMeeting.distinct_count_by(:issue_id)).to eq(1)
       end
 
-      context 'when given no colum to count' do
+      context 'when given no column to count' do
         it 'counts by :id and returns a count of 2' do
-          expect(::GroupMember.distinct_count_by).to eq(2)
+          expect(::ZoomMeeting.distinct_count_by).to eq(2)
         end
       end
     end
 
-    context 'one record created by each user' do
-      let!(:model_created_by_user_1) { create(:group_member, user: user_1) }
-      let!(:model_created_by_user_2) { create(:group_member, user: user_2) }
+    context 'one record created by each issue' do
+      let!(:model_created_by_issue_1) { create(:zoom_meeting, issue: issue_1) }
+      let!(:model_created_by_issue_2) { create(:zoom_meeting, issue: issue_2) }
 
       it 'returns a count of 2' do
-        expect(::GroupMember.distinct_count_by(:user_id)).to eq(2)
+        expect(::ZoomMeeting.distinct_count_by(:issue_id)).to eq(2)
       end
     end
 
@@ -38,11 +41,11 @@ describe EE::UsageStatistics do
       end
 
       it 'does not raise an error' do
-        expect { ::GroupMember.distinct_count_by(:user_id) }.not_to raise_error
+        expect { ::ZoomMeeting.distinct_count_by(:issue_id) }.not_to raise_error
       end
 
       it 'returns -1' do
-        expect(::GroupMember.distinct_count_by(:user_id)).to eq(-1)
+        expect(::ZoomMeeting.distinct_count_by(:issue_id)).to eq(-1)
       end
     end
   end

spec/models/zoom_meeting_spec.rb

@@ -151,51 +151,4 @@ describe ZoomMeeting do
       it_behaves_like 'can remove meetings'
     end
   end
-
-  describe '.distinct_count_by' do
-    let(:issue_1) { create(:issue) }
-    let(:issue_2) { create(:issue) }
-
-    context 'two meetings for the same issue' do
-      before do
-        create(:zoom_meeting, issue: issue_1)
-        create(:zoom_meeting, :removed_from_issue, issue: issue_1)
-      end
-
-      it 'returns a count of 1' do
-        expect(described_class.distinct_count_by(:issue_id)).to eq(1)
-      end
-
-      context 'when given no colum to count' do
-        it 'counts by :id and returns a count of 2' do
-          expect(described_class.distinct_count_by).to eq(2)
-        end
-      end
-    end
-
-    context 'one meeting for each issue' do
-      it 'returns a count of 2' do
-        create(:zoom_meeting, issue: issue_1)
-        create(:zoom_meeting, issue: issue_2)
-
-        expect(described_class.distinct_count_by(:issue_id)).to eq(2)
-      end
-    end
-
-    context 'the count query times out' do
-      before do
-        allow_next_instance_of(ActiveRecord::Relation) do |instance|
-          allow(instance).to receive(:count).and_raise(ActiveRecord::StatementInvalid.new(''))
-        end
-      end
-
-      it 'does not raise an error' do
-        expect { described_class.distinct_count_by(:issue_id) }.not_to raise_error
-      end
-
-      it 'returns -1' do
-        expect(described_class.distinct_count_by(:issue_id)).to eq(-1)
-      end
-    end
-  end
 end