Code review feedback

- Add unit test for sending an email
- Use ApplicationRecord instead of ActiveRecord::Base
- Add a test that email is not sent with 0 consumption
- Fix name of new worker to be added in comment

config/puma_actioncable.rb

-# frozen_string_literal: true
-
-# -----------------------------------------------------------------------
-# This file is used by the GDK to generate a default config/puma_actioncable.rb file
-# Note that `/Users/allisonbrowne/gitlab/gdk` will be substituted for the actual GDK root
-# directory when this file is generated
-# -----------------------------------------------------------------------
-
-# Load "path" as a rackup file.
-#
-# The default is "cable/config.ru".
-#
-rackup 'cable/config.ru'
-pidfile '/Users/allisonbrowne/gitlab/gdk/gitlab/tmp/pids/puma_actioncable.pid'
-state_path '/Users/allisonbrowne/gitlab/gdk/gitlab/tmp/pids/puma_actioncable.state'
-
-## Uncomment the lines if you would like to write puma stdout & stderr streams
-## to a different location than rails logs.
-## When using GitLab Development Kit, by default, these logs will be consumed
-## by runit and can be accessed using `gdk tail rails-actioncable`
-# stdout_redirect '/Users/allisonbrowne/gitlab/gdk/gitlab/log/puma_actioncable.stdout.log',
-#  '/Users/allisonbrowne/gitlab/gdk/gitlab/log/puma_actioncable.stderr.log',
-#  true
-
-# Configure "min" to be the minimum number of threads to use to answer
-# requests and "max" the maximum.
-#
-# The default is "0, 16".
-#
-threads 1, 4
-
-# By default, workers accept all requests and queue them to pass to handlers.
-# When false, workers accept the number of simultaneous requests configured.
-#
-# Queueing requests generally improves performance, but can cause deadlocks if
-# the app is waiting on a request to itself. See https://github.com/puma/puma/issues/612
-#
-# When set to false this may require a reverse proxy to handle slow clients and
-# queue requests before they reach puma. This is due to disabling HTTP keepalive
-queue_requests false
-
-# Bind the server to "url". "tcp://", "unix://" and "ssl://" are the only
-# accepted protocols.
-bind 'unix:///Users/allisonbrowne/gitlab/gdk/gitlab_actioncable.socket'
-
-workers 1
-
-require_relative "/Users/allisonbrowne/gitlab/gdk/gitlab/lib/gitlab/cluster/lifecycle_events"
-
-on_restart do
-  # Signal application hooks that we're about to restart
-  Gitlab::Cluster::LifecycleEvents.do_before_master_restart
-end
-
-before_fork do
-  # Signal to the puma killer
-  Gitlab::Cluster::PumaWorkerKillerInitializer.start @config.options unless ENV['DISABLE_PUMA_WORKER_KILLER']
-
-  # Signal application hooks that we're about to fork
-  Gitlab::Cluster::LifecycleEvents.do_before_fork
-end
-
-Gitlab::Cluster::LifecycleEvents.set_puma_options @config.options
-on_worker_boot do
-  # Signal application hooks of worker start
-  Gitlab::Cluster::LifecycleEvents.do_worker_start
-end
-
-# Preload the application before starting the workers; this conflicts with
-# phased restart feature. (off by default)
-
-preload_app!
-
-tag 'gitlab-actioncable-puma-worker'
-
-# Verifies that all workers have checked in to the master process within
-# the given timeout. If not the worker process will be restarted. Default
-# value is 60 seconds.
-#
-worker_timeout 60
-
-# https://github.com/puma/puma/blob/master/5.0-Upgrade.md#lower-latency-better-throughput
-wait_for_less_busy_worker ENV.fetch('PUMA_WAIT_FOR_LESS_BUSY_WORKER', 0.001).to_f
-
-# https://github.com/puma/puma/blob/master/5.0-Upgrade.md#nakayoshi_fork
-nakayoshi_fork unless ENV['DISABLE_PUMA_NAKAYOSHI_FORK'] == 'true'
-
-# Use json formatter
-require_relative "/Users/allisonbrowne/gitlab/gdk/gitlab/lib/gitlab/puma_logging/json_formatter"
-
-json_formatter = Gitlab::PumaLogging::JSONFormatter.new
-log_formatter do |str|
-  json_formatter.call(str)
-end

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

@@ -3,9 +3,8 @@
 module Ci
   module Minutes
     class UpdateBuildMinutesService < BaseService
-      # Calculates consumption and updates the project and namespace minutes based on the passed build
-      # Calculating the consumption syncronously before triggering an async job to update
-      # ensures the pipeline data still exists in the case where minutes are updated prior to pipeline deletion
+      # Calculates consumption and updates the project and namespace statistics(legacy)
+      # or ProjectMonthlyUsage and NamespaceMonthlyUsage(not legacy) based on the passed build.
       def execute(build)
         return unless build.shared_runners_minutes_limit_enabled?
         return unless build.complete?
@@ -13,13 +12,10 @@ module Ci
 
         consumption = ::Gitlab::Ci::Minutes::BuildConsumption.new(build, build.duration).amount
 
-        Ci::Minutes::UpdateMinutesByConsumptionService.new(project, namespace).execute(consumption)
-        # TODO(Issue #335338): Introduce async worker UpdateMinutesByConsumptionWorker, example:
-        # if ::Feature.enabled?(:cancel_pipelines_prior_to_destroy, default_enabled: :yaml)
-        #   Ci::Minutes::UpdateMinutesByConsumptionService.new(project, namespace).execute_async(consumption)
-        # else
-        #   Ci::Minutes::UpdateMinutesByConsumptionService.new(project, namespace).execute(consumption)
-        # end
+        return unless consumption > 0
+
+        # TODO(Issue #335338): Introduce async worker UpdateProjectAndNamespaceUsageWorker
+        Ci::Minutes::UpdateProjectAndNamespaceUsageService.new(project, namespace).execute(consumption)
 
         compare_with_live_consumption(build, consumption)
       end

ee/app/services/ci/minutes/update_minutes_by_consumption_service.rb → ee/app/services/ci/minutes/update_project_and_namespace_usage_service.rb

@@ -2,26 +2,24 @@
 
 module Ci
   module Minutes
-    class UpdateMinutesByConsumptionService
+    class UpdateProjectAndNamespaceUsageService
       def initialize(project, namespace)
         @project = project
         @namespace = namespace
       end
 
-      # Updates the project and namespace minutes based on the passed consumption amount
+      # Updates the project and namespace usage based on the passed consumption amount
       def execute(consumption)
-        return unless consumption > 0
-
         consumption_in_seconds = consumption.minutes.to_i
         legacy_track_usage_of_monthly_minutes(consumption_in_seconds)
 
         track_usage_of_monthly_minutes(consumption)
-        send_email_notification
+        send_minutes_email_notification
       end
 
       private
 
-      def send_email_notification
+      def send_minutes_email_notification
         # `perform reset` on `project` because `Namespace#namespace_statistics` will otherwise return stale data.
         ::Ci::Minutes::EmailNotificationService.new(@project.reset).execute if ::Gitlab.com?
       end
@@ -40,7 +38,7 @@ module Ci
         namespace_usage = ::Ci::Minutes::NamespaceMonthlyUsage.find_or_create_current(@namespace)
         project_usage = ::Ci::Minutes::ProjectMonthlyUsage.find_or_create_current(@project)
 
-        ActiveRecord::Base.transaction do
+        ApplicationRecord.transaction do
           ::Ci::Minutes::NamespaceMonthlyUsage.increase_usage(namespace_usage, consumption)
           ::Ci::Minutes::ProjectMonthlyUsage.increase_usage(project_usage, consumption)
         end

ee/lib/gitlab/ci/minutes/build_consumption.rb

@@ -20,7 +20,7 @@ module Gitlab
         private
 
         def cost_factor
-          @build&.runner&.minutes_cost_factor(project_visibility_level)
+          @build.runner.minutes_cost_factor(project_visibility_level)
         end
 
         def project_visibility_level

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

@@ -31,6 +31,38 @@ RSpec.describe Ci::Minutes::UpdateBuildMinutesService do
       end
     end
 
+    shared_examples 'does nothing' do
+      it 'does not update legacy statistics' do
+        subject
+
+        expect(project.statistics.reload.shared_runners_seconds).to eq(0)
+        expect(namespace.namespace_statistics).to be_nil
+      end
+
+      it 'does not update namespace monthly usage' do
+        expect { subject }.not_to change { Ci::Minutes::NamespaceMonthlyUsage.count }
+      end
+
+      it 'does not update project monthly usage' do
+        expect { subject }.not_to change { Ci::Minutes::ProjectMonthlyUsage.count }
+      end
+
+      it 'does not observe the difference between actual vs live consumption' do
+        expect(::Gitlab::Ci::Pipeline::Metrics)
+          .not_to receive(:gitlab_ci_difference_live_vs_actual_minutes)
+
+        subject
+      end
+
+      it 'does not send an email' do
+        allow(Gitlab).to receive(:com?).and_return(true)
+
+        expect(Ci::Minutes::EmailNotificationService).not_to receive(:new)
+
+        subject
+      end
+    end
+
     context 'with shared runner' do
       let(:cost_factor) { 2.0 }
       let(:runner) { create(:ci_runner, :instance, private_projects_minutes_cost_factor: cost_factor) }
@@ -91,7 +123,17 @@ RSpec.describe Ci::Minutes::UpdateBuildMinutesService do
         end
       end
 
-      context 'when statistics are created' do
+      context 'when consumption is 0' do
+        let(:build) do
+          create(:ci_build, :success,
+            runner: runner, pipeline: pipeline,
+            started_at: Time.current, finished_at: Time.current)
+        end
+
+        it_behaves_like 'does nothing'
+      end
+
+      context 'when statistics and usage have existing amounts' do
         let(:usage_in_seconds) { 100 }
         let(:usage_in_minutes) { (100.to_f / 60).round(2) }
 
@@ -208,19 +250,7 @@ RSpec.describe Ci::Minutes::UpdateBuildMinutesService do
     context 'for specific runner' do
       let(:runner) { create(:ci_runner, :project) }
 
-      it 'does not create statistics' do
-        subject
-
-        expect(namespace.namespace_statistics).to be_nil
-      end
-
-      it 'does not track namespace monthly usage' do
-        expect { subject }.not_to change { Ci::Minutes::NamespaceMonthlyUsage.count }
-      end
-
-      it 'does not track project monthly usage' do
-        expect { subject }.not_to change { Ci::Minutes::ProjectMonthlyUsage.count }
-      end
+      it_behaves_like 'does nothing'
     end
   end
 end

ee/spec/services/ci/minutes/update_minutes_by_consumption_service_spec.rb → ee/spec/services/ci/minutes/update_project_and_namespace_usage_service_spec.rb

@@ -2,16 +2,16 @@
 
 require 'spec_helper'
 
-RSpec.describe Ci::Minutes::UpdateMinutesByConsumptionService do
-  describe '#execute' do
+RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageService do
   let_it_be(:namespace) { create(:namespace, shared_runners_minutes_limit: 100) }
   let_it_be(:project) { create(:project, :private, namespace: namespace) }
 
   let(:consumption_minutes) { 120 }
-    let(:consumption_seconds) { 120 * 60 }
+  let(:consumption_seconds) { consumption_minutes * 60 }
   let(:namespace_amount_used) { Ci::Minutes::NamespaceMonthlyUsage.find_or_create_current(namespace).amount_used }
   let(:project_amount_used) { Ci::Minutes::ProjectMonthlyUsage.find_or_create_current(project).amount_used }
 
+  describe '#execute' do
     subject { described_class.new(project, namespace).execute(consumption_minutes) }
 
     context 'with shared runner' do
@@ -45,6 +45,32 @@ RSpec.describe Ci::Minutes::UpdateMinutesByConsumptionService do
             expect(project_amount_used).to eq(0)
           end
         end
+
+        context 'when on .com' do
+          before do
+            allow(Gitlab).to receive(:com?).and_return(true)
+          end
+
+          it 'sends a minute notification email' do
+            expect_next_instance_of(Ci::Minutes::EmailNotificationService) do |service|
+              expect(service).to receive(:execute)
+            end
+
+            subject
+          end
+        end
+
+        context 'when not on .com' do
+          before do
+            allow(Gitlab).to receive(:com?).and_return(false)
+          end
+
+          it 'does not send a minute notification email' do
+            expect(Ci::Minutes::EmailNotificationService).not_to receive(:new)
+
+            subject
+          end
+        end
       end
 
       context 'when statistics and usage have existing values' do