Merge branch '217120-resolve-duplicate-ci-minutes-percentage-definitions' into 'master'

Resolve duplicate ci minutes percentage definitions

Closes #217120

See merge request gitlab-org/gitlab!33064

ee/app/models/ci/minutes/notification.rb

@@ -29,6 +29,18 @@ module Ci
         contextual_map.dig(stage, :style)
       end
 
+      def no_remaining_minutes?
+        stage == :exceeded
+      end
+
+      def running_out?
+        [:danger, :warning].include?(stage)
+      end
+
+      def stage_percentage
+        PERCENTAGES[stage]
+      end
+
       private
 
       attr_reader :context, :stage

ee/app/models/ee/group.rb

@@ -370,6 +370,10 @@ module EE
       end
     end
 
+    def owners_emails
+      owners.pluck(:email)
+    end
+
     private
 
     def custom_project_templates_group_allowed

ee/app/models/ee/namespace.rb

@@ -20,8 +20,6 @@ module EE
     LICENSE_PLANS_TO_NAMESPACE_PLANS = NAMESPACE_PLANS_TO_LICENSE_PLANS.invert.freeze
     PLANS = (NAMESPACE_PLANS_TO_LICENSE_PLANS.keys + [Plan::FREE]).freeze
 
-    CI_USAGE_ALERT_LEVELS = [30, 5].freeze
-
     prepended do
       include EachBatch
 
@@ -55,6 +53,8 @@ module EE
       delegate :shared_runners_minutes, :shared_runners_seconds, :shared_runners_seconds_last_reset,
         :extra_shared_runners_minutes, to: :namespace_statistics, allow_nil: true
 
+      delegate :email, to: :owner, allow_nil: true, prefix: true
+
       # Opportunistically clear the +file_template_project_id+ if invalid
       before_validation :clear_file_template_project_id
 

ee/app/services/ci/minutes/email_notification_service.rb

@@ -4,49 +4,55 @@ module Ci
   module Minutes
     class EmailNotificationService < ::BaseService
       def execute
-        return unless ::Gitlab.com?
         return unless namespace.shared_runners_minutes_limit_enabled?
 
-        notify_on_total_usage
-        notify_on_partial_usage
+        notify
       end
 
       private
 
-      def recipients
-        namespace.user? ? [namespace.owner.email] : namespace.owners.pluck(:email) # rubocop:disable CodeReuse/ActiveRecord
+      attr_reader :notification
+
+      def notify
+        @notification = ::Ci::Minutes::Notification.new(project, nil)
+
+        if notification.no_remaining_minutes?
+          notify_total_usage
+        elsif notification.running_out?
+          notify_partial_usage
+        end
       end
 
-      def notify_on_total_usage
-        return unless namespace.shared_runners_minutes_used? && namespace.last_ci_minutes_notification_at.nil?
+      def notify_total_usage
+        return if namespace.last_ci_minutes_notification_at
 
         namespace.update_columns(last_ci_minutes_notification_at: Time.current)
 
         CiMinutesUsageMailer.notify(namespace, recipients).deliver_later
       end
 
-      def notify_on_partial_usage
-        return if namespace.shared_runners_minutes_used?
-        return if namespace.last_ci_minutes_usage_notification_level == current_alert_level
-        return if alert_levels.max < namespace.shared_runners_remaining_minutes_percent
+      def notify_partial_usage
+        return if already_notified_running_out
 
-        namespace.update_columns(last_ci_minutes_usage_notification_level: current_alert_level)
+        namespace.update_columns(last_ci_minutes_usage_notification_level: current_alert_percentage)
 
-        CiMinutesUsageMailer.notify_limit(namespace, recipients, current_alert_level).deliver_later
+        CiMinutesUsageMailer.notify_limit(namespace, recipients, current_alert_percentage).deliver_later
       end
 
-      def namespace
-        @namespace ||= project.shared_runners_limit_namespace
+      def already_notified_running_out
+        namespace.last_ci_minutes_usage_notification_level == current_alert_percentage
       end
 
-      def alert_levels
-        @alert_levels ||= EE::Namespace::CI_USAGE_ALERT_LEVELS.sort
+      def recipients
+        namespace.user? ? [namespace.owner_email] : namespace.owners_emails
       end
 
-      def current_alert_level
-        remaining_percent = namespace.shared_runners_remaining_minutes_percent
+      def namespace
+        @namespace ||= project.shared_runners_limit_namespace
+      end
 
-        @current_alert_level ||= alert_levels.find { |level| level >= remaining_percent }
+      def current_alert_percentage
+        notification.stage_percentage
       end
     end
   end

ee/app/workers/ee/build_finished_worker.rb

@@ -6,7 +6,7 @@ module EE
       UpdateBuildMinutesService.new(build.project, nil).execute(build)
       # We need to use `reset` on `project` because their AR associations have been cached
       # and `Namespace#namespace_statistics` will return stale data.
-      ::Ci::Minutes::EmailNotificationService.new(build.project.reset).execute
+      ::Ci::Minutes::EmailNotificationService.new(build.project.reset).execute if ::Gitlab.com?
 
       StoreSecurityScansWorker.perform_async(build.id)
 

ee/spec/models/ci/minutes/notification_spec.rb

@@ -41,10 +41,30 @@ RSpec.describe Ci::Minutes::Notification do
           allow(group).to receive(:shared_runners_remaining_minutes).and_return(4)
         end
 
-        it 'has warning notification' do
-          expect(subject.show?(user)).to be_truthy
-          expect(subject.text).to match(/.*\shas 30% or less Shared Runner Pipeline minutes remaining/)
-          expect(subject.style).to eq :warning
+        describe '#show?' do
+          it 'has warning notification' do
+            expect(subject.show?(user)).to be_truthy
+            expect(subject.text).to match(/.*\shas 30% or less Shared Runner Pipeline minutes remaining/)
+            expect(subject.style).to eq :warning
+          end
+        end
+
+        describe '#running_out?' do
+          it 'is running out of minutes' do
+            expect(subject.running_out?).to be_truthy
+          end
+        end
+
+        describe '#no_remaining_minutes?' do
+          it 'has not ran out of minutes' do
+            expect(subject.no_remaining_minutes?).to be_falsey
+          end
+        end
+
+        describe '#stage_percentage' do
+          it 'provides percentage for current alert level' do
+            expect(subject.stage_percentage).to eq 30
+          end
         end
       end
 
@@ -53,10 +73,30 @@ RSpec.describe Ci::Minutes::Notification do
           allow(group).to receive(:shared_runners_remaining_minutes).and_return(1)
         end
 
-        it 'has danger notification' do
-          expect(subject.show?(user)).to be_truthy
-          expect(subject.text).to match(/.*\shas 5% or less Shared Runner Pipeline minutes remaining/)
-          expect(subject.style).to eq :danger
+        describe '#show?' do
+          it 'has danger notification' do
+            expect(subject.show?(user)).to be_truthy
+            expect(subject.text).to match(/.*\shas 5% or less Shared Runner Pipeline minutes remaining/)
+            expect(subject.style).to eq :danger
+          end
+        end
+
+        describe '#running_out?' do
+          it 'is running out of minutes' do
+            expect(subject.running_out?).to be_truthy
+          end
+        end
+
+        describe '#no_remaining_minutes?' do
+          it 'has not ran out of minutes' do
+            expect(subject.no_remaining_minutes?).to be_falsey
+          end
+        end
+
+        describe '#stage_percentage' do
+          it 'provides percentage for current alert level' do
+            expect(subject.stage_percentage).to eq 5
+          end
         end
       end
 
@@ -65,20 +105,60 @@ RSpec.describe Ci::Minutes::Notification do
           allow(group).to receive(:shared_runners_remaining_minutes).and_return(6)
         end
 
-        it 'has warning notification' do
-          expect(subject.show?(user)).to be_truthy
-          expect(subject.text).to match(/.*\shas 30% or less Shared Runner Pipeline minutes remaining/)
-          expect(subject.style).to eq :warning
+        describe '#show?' do
+          it 'has warning notification' do
+            expect(subject.show?(user)).to be_truthy
+            expect(subject.text).to match(/.*\shas 30% or less Shared Runner Pipeline minutes remaining/)
+            expect(subject.style).to eq :warning
+          end
+        end
+
+        describe '#running_out?' do
+          it 'is running out of minutes' do
+            expect(subject.running_out?).to be_truthy
+          end
+        end
+
+        describe '#no_remaining_minutes?' do
+          it 'has not ran out of minutes' do
+            expect(subject.no_remaining_minutes?).to be_falsey
+          end
+        end
+
+        describe '#stage_percentage' do
+          it 'provides percentage for current alert level' do
+            expect(subject.stage_percentage).to eq 30
+          end
         end
       end
 
       context 'when usage has exceeded the limit' do
         let(:group) { create(:group, :with_used_build_minutes_limit) }
 
-        it 'has exceeded notification' do
-          expect(subject.show?(user)).to be_truthy
-          expect(subject.text).to match(/.*\shas exceeded its pipeline minutes quota/)
-          expect(subject.style).to eq :danger
+        describe '#show?' do
+          it 'has exceeded notification' do
+            expect(subject.show?(user)).to be_truthy
+            expect(subject.text).to match(/.*\shas exceeded its pipeline minutes quota/)
+            expect(subject.style).to eq :danger
+          end
+        end
+
+        describe '#running_out?' do
+          it 'does not have any minutes left' do
+            expect(subject.running_out?).to be_falsey
+          end
+        end
+
+        describe '#no_remaining_minutes?' do
+          it 'has run out of minutes out of minutes' do
+            expect(subject.no_remaining_minutes?).to be_truthy
+          end
+        end
+
+        describe '#stage_percentage' do
+          it 'provides percentage for current alert level' do
+            expect(subject.stage_percentage).to eq 0
+          end
         end
       end
     end
@@ -91,83 +171,85 @@ RSpec.describe Ci::Minutes::Notification do
     end
 
     context 'when not permitted to see notifications' do
-      it 'has no notifications set' do
-        expect(subject.show?(user)).to be_falsey
+      describe '#show?' do
+        it 'has no notifications set' do
+          expect(subject.show?(user)).to be_falsey
+        end
       end
     end
   end
 
   context 'when at project level' do
-    describe '#show?' do
-      context 'when eligible to see notifications' do
-        before do
-          group.add_developer(user)
-        end
+    context 'when eligible to see notifications' do
+      before do
+        group.add_developer(user)
+      end
 
+      describe '#show?' do
         it_behaves_like 'queries for notifications' do
           subject do
             threshold = described_class.new(injected_project, nil)
             threshold.show?(user)
           end
         end
-
-        it_behaves_like 'has notifications' do
-          subject { described_class.new(injected_project, nil) }
-        end
       end
 
-      it_behaves_like 'not eligible to see notifications' do
+      it_behaves_like 'has notifications' do
         subject { described_class.new(injected_project, nil) }
       end
+    end
 
-      context 'when user is not authenticated' do
-        let(:user) { nil }
+    it_behaves_like 'not eligible to see notifications' do
+      subject { described_class.new(injected_project, nil) }
+    end
 
-        it_behaves_like 'not eligible to see notifications' do
-          subject { described_class.new(injected_project, nil) }
-        end
+    context 'when user is not authenticated' do
+      let(:user) { nil }
+
+      it_behaves_like 'not eligible to see notifications' do
+        subject { described_class.new(injected_project, nil) }
       end
     end
   end
 
   context 'when at namespace level' do
-    describe '#show?' do
-      context 'when eligible to see notifications' do
-        before do
-          group.add_developer(user)
-        end
+    context 'when eligible to see notifications' do
+      before do
+        group.add_developer(user)
+      end
 
-        context 'with a project that has runners enabled inside namespace' do
+      context 'with a project that has runners enabled inside namespace' do
+        describe '#show?' do
           it_behaves_like 'queries for notifications' do
             subject do
               threshold = described_class.new(nil, injected_group)
               threshold.show?(user)
             end
           end
-
-          it_behaves_like 'has notifications' do
-            subject { described_class.new(nil, injected_group) }
-          end
         end
 
-        context 'with no projects that have runners enabled inside namespace' do
-          it_behaves_like 'not eligible to see notifications' do
-            let(:shared_runners_enabled) { false }
-            subject { described_class.new(nil, injected_group) }
-          end
+        it_behaves_like 'has notifications' do
+          subject { described_class.new(nil, injected_group) }
         end
       end
 
-      it_behaves_like 'not eligible to see notifications' do
-        subject { described_class.new(nil, injected_group) }
+      context 'with no projects that have runners enabled inside namespace' do
+        it_behaves_like 'not eligible to see notifications' do
+          let(:shared_runners_enabled) { false }
+          subject { described_class.new(nil, injected_group) }
+        end
       end
+    end
 
-      context 'when user is not authenticated' do
-        let(:user) { nil }
+    it_behaves_like 'not eligible to see notifications' do
+      subject { described_class.new(nil, injected_group) }
+    end
 
-        it_behaves_like 'not eligible to see notifications' do
-          subject { described_class.new(injected_project, nil) }
-        end
+    context 'when user is not authenticated' do
+      let(:user) { nil }
+
+      it_behaves_like 'not eligible to see notifications' do
+        subject { described_class.new(injected_project, nil) }
       end
     end
   end

ee/spec/models/ee/namespace_spec.rb

@@ -23,6 +23,7 @@ RSpec.describe Namespace do
   it { is_expected.to delegate_method(:trial?).to(:gitlab_subscription) }
   it { is_expected.to delegate_method(:trial_ends_on).to(:gitlab_subscription) }
   it { is_expected.to delegate_method(:upgradable?).to(:gitlab_subscription) }
+  it { is_expected.to delegate_method(:email).to(:owner).with_prefix.allow_nil }
 
   shared_examples 'plan helper' do |namespace_plan|
     let(:namespace) { create(:namespace_with_plan, plan: "#{plan_name}_plan") }

ee/spec/models/group_spec.rb

@@ -430,7 +430,7 @@ RSpec.describe Group do
 
     context 'group with associated push_rules record' do
       context 'with its own push rule' do
-        let(:push_rule) { create(:push_rule )}
+        let(:push_rule) { create(:push_rule) }
 
         it 'returns its own push rule' do
           group.update(push_rule: push_rule)
@@ -941,4 +941,16 @@ RSpec.describe Group do
       end
     end
   end
+
+  describe '#owners_emails' do
+    let(:user) { create(:user, email: 'bob@example.com') }
+
+    before do
+      group.add_owner(user)
+    end
+
+    subject { group.owners_emails }
+
+    it { is_expected.to match([user.email]) }
+  end
 end

ee/spec/services/ci/minutes/email_notification_service_spec.rb

@@ -32,12 +32,6 @@ RSpec.describe Ci::Minutes::EmailNotificationService do
     create(:namespace_statistics, namespace: namespace, shared_runners_seconds: ci_minutes_used * 60)
   end
 
-  let(:gitlab_dot_com) { true }
-
-  before do
-    allow(Gitlab).to receive(:com?).and_return(gitlab_dot_com)
-  end
-
   describe '#execute' do
     let(:extra_ci_minutes) { 0 }
     let(:namespace) do
@@ -46,17 +40,6 @@ RSpec.describe Ci::Minutes::EmailNotificationService do
 
     subject { described_class.new(project).execute }
 
-    context 'when it is not GitLab.com' do
-      let(:gitlab_dot_com) { false }
-      let(:ci_minutes_used) { 2500 }
-
-      it 'does not send the email to all the owners' do
-        expect(CiMinutesUsageMailer).not_to receive(:notify)
-
-        subject
-      end
-    end
-
     context 'with a personal namespace' do
       before do
         namespace.update(owner_id: user.id)
@@ -169,8 +152,6 @@ RSpec.describe Ci::Minutes::EmailNotificationService do
     end
 
     before do
-      stub_const("EE::Namespace::CI_USAGE_ALERT_LEVELS", [30, 5])
-
       namespace.add_owner(user)
       namespace.add_owner(user_2)
     end
@@ -182,16 +163,7 @@ RSpec.describe Ci::Minutes::EmailNotificationService do
     end
 
     context 'when available minutes have reached the first level of alert' do
-      context 'when it is not GitLab.com' do
-        let(:gitlab_dot_com) { false }
-        let(:ci_minutes_used) { 1500 }
-
-        it_behaves_like 'no notification is sent'
-      end
-
       context 'when quota is unlimited' do
-        # Gitlab.com? => true is required in order to excercise the notification
-        let(:gitlab_dot_com) { true }
         let(:ci_minutes_used) { 1500 }
 
         before do

ee/spec/workers/build_finished_worker_spec.rb

@@ -21,32 +21,46 @@ RSpec.describe BuildFinishedWorker do
   end
 
   describe '#perform' do
-    before do
-      allow(Gitlab).to receive(:com?).and_return(true)
-      allow_any_instance_of(EE::Project).to receive(:shared_runners_minutes_limit_enabled?).and_return(true)
-    end
+    context 'when on .com' do
+      before do
+        allow(Gitlab).to receive(:com?).and_return(true)
+        allow_any_instance_of(EE::Project).to receive(:shared_runners_minutes_limit_enabled?).and_return(true)
+      end
 
-    it 'updates the project stats' do
-      expect { subject }.to change { project_stats.reload.shared_runners_seconds }
-    end
+      it 'updates the project stats' do
+        expect { subject }.to change { project_stats.reload.shared_runners_seconds }
+      end
 
-    it 'updates the namespace stats' do
-      expect { subject }.to change { namespace_stats.reload.shared_runners_seconds }
-    end
+      it 'updates the namespace stats' do
+        expect { subject }.to change { namespace_stats.reload.shared_runners_seconds }
+      end
 
-    it 'notifies the owners of Groups' do
-      namespace.update_attribute(:shared_runners_minutes_limit, 2000)
-      namespace_stats.update_attribute(:shared_runners_seconds, 2100 * 60)
+      it 'notifies the owners of Groups' do
+        namespace.update_attribute(:shared_runners_minutes_limit, 2000)
+        namespace_stats.update_attribute(:shared_runners_seconds, 2100 * 60)
 
-      expect(CiMinutesUsageMailer).to receive(:notify).once.with(namespace, [namespace.owner.email]).and_return(spy)
+        expect(CiMinutesUsageMailer).to receive(:notify).once.with(namespace, [namespace.owner.email]).and_return(spy)
 
-      subject
+        subject
+      end
+
+      it 'stores security scans' do
+        expect(StoreSecurityScansWorker).to receive(:perform_async)
+
+        subject
+      end
     end
 
-    it 'stores security scans' do
-      expect(StoreSecurityScansWorker).to receive(:perform_async)
+    context 'when not on .com' do
+      before do
+        allow(Gitlab).to receive(:com?).and_return(false)
+      end
+
+      it 'does not notify the owners of Groups' do
+        expect(::Ci::Minutes::EmailNotificationService).not_to receive(:new)
 
-      subject
+        subject
+      end
     end
   end
 end