Merge branch 'offload-onboarding-progress-git-read-action-to-worker' into 'master'

Offload tracking onboarding progress to worker

See merge request gitlab-org/gitlab!54573

app/controllers/repositories/git_http_controller.rb

@@ -78,11 +78,11 @@ module Repositories
     def update_fetch_statistics
       return unless project
       return if Gitlab::Database.read_only?
-      return if Feature.enabled?(:disable_git_http_fetch_writes)
-
       return unless repo_type.project?
 
-      OnboardingProgressService.new(project.namespace).execute(action: :git_read)
+      OnboardingProgressService.async(project.namespace_id).execute(action: :git_pull)
+
+      return if Feature.enabled?(:disable_git_http_fetch_writes)
 
       if Feature.enabled?(:project_statistics_sync, project, default_enabled: true)
         Projects::FetchStatisticsIncrementService.new(project).execute

app/models/onboarding_progress.rb

@@ -66,6 +66,13 @@ class OnboardingProgress < ApplicationRecord
       where(namespace: namespace).where.not(action_column => nil).exists?
     end
 
+    def not_completed?(namespace_id, action)
+      return unless ACTIONS.include?(action)
+
+      action_column = column_name(action)
+      where(namespace_id: namespace_id).where(action_column => nil).exists?
+    end
+
     def column_name(action)
       :"#{action}_at"
     end

app/services/onboarding_progress_service.rb

 # frozen_string_literal: true
 
 class OnboardingProgressService
+  class Async
+    attr_reader :namespace_id
+
+    def initialize(namespace_id)
+      @namespace_id = namespace_id
+    end
+
+    def execute(action:)
+      return unless OnboardingProgress.not_completed?(namespace_id, action)
+
+      Namespaces::OnboardingProgressWorker.perform_async(namespace_id, action)
+    end
+  end
+
+  def self.async(namespace_id)
+    Async.new(namespace_id)
+  end
+
   def initialize(namespace)
     @namespace = namespace&.root_ancestor
   end

app/workers/all_queues.yml

@@ -1848,6 +1848,14 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: namespaces_onboarding_progress
+  :feature_category: :product_analytics
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: namespaces_onboarding_user_added
   :feature_category: :users
   :has_external_dependencies:

app/workers/namespaces/onboarding_progress_worker.rb

+# frozen_string_literal: true
+
+module Namespaces
+  class OnboardingProgressWorker
+    include ApplicationWorker
+
+    feature_category :product_analytics
+    urgency :low
+
+    deduplicate :until_executed
+    idempotent!
+
+    def perform(namespace_id, action)
+      namespace = Namespace.find_by_id(namespace_id)
+      return unless namespace && action
+
+      OnboardingProgressService.new(namespace).execute(action: action.to_sym)
+    end
+  end
+end

config/sidekiq_queues.yml

@@ -214,6 +214,8 @@
   - 1
 - - namespaces_onboarding_pipeline_created
   - 1
+- - namespaces_onboarding_progress
+  - 1
 - - namespaces_onboarding_user_added
   - 1
 - - new_epic

spec/controllers/repositories/git_http_controller_spec.rb

@@ -54,14 +54,17 @@ RSpec.describe Repositories::GitHttpController do
           }.from(0).to(1)
         end
 
-        it_behaves_like 'records an onboarding progress action', :git_read do
-          let(:namespace) { project.namespace }
-
-          subject { send_request }
+        describe 'recording the onboarding progress', :sidekiq_inline do
+          let_it_be(:namespace) { project.namespace }
 
           before do
-            stub_feature_flags(disable_git_http_fetch_writes: false)
+            OnboardingProgress.onboard(namespace)
+            send_request
           end
+
+          subject { OnboardingProgress.completed?(namespace, :git_pull) }
+
+          it { is_expected.to be(true) }
         end
 
         context 'when disable_git_http_fetch_writes is enabled' do
@@ -75,12 +78,6 @@ RSpec.describe Repositories::GitHttpController do
 
             send_request
           end
-
-          it 'does not record onboarding progress' do
-            expect(OnboardingProgressService).not_to receive(:new)
-
-            send_request
-          end
         end
       end
     end

spec/models/onboarding_progress_spec.rb

@@ -182,6 +182,30 @@ RSpec.describe OnboardingProgress do
     end
   end
 
+  describe '.not_completed?' do
+    subject { described_class.not_completed?(namespace.id, action) }
+
+    context 'when the namespace has not yet been onboarded' do
+      it { is_expected.to be(false) }
+    end
+
+    context 'when the namespace has been onboarded but not registered the action yet' do
+      before do
+        described_class.onboard(namespace)
+      end
+
+      it { is_expected.to be(true) }
+
+      context 'when the action has been registered' do
+        before do
+          described_class.register(namespace, action)
+        end
+
+        it { is_expected.to be(false) }
+      end
+    end
+  end
+
   describe '.column_name' do
     subject { described_class.column_name(action) }
 

spec/services/onboarding_progress_service_spec.rb

@@ -3,6 +3,47 @@
 require 'spec_helper'
 
 RSpec.describe OnboardingProgressService do
+  describe '.async' do
+    let_it_be(:namespace) { create(:namespace) }
+    let_it_be(:action) { :git_pull }
+
+    subject(:execute_service) { described_class.async(namespace.id).execute(action: action) }
+
+    context 'when not onboarded' do
+      it 'does not schedule a worker' do
+        expect(Namespaces::OnboardingProgressWorker).not_to receive(:perform_async)
+
+        execute_service
+      end
+    end
+
+    context 'when onboarded' do
+      before do
+        OnboardingProgress.onboard(namespace)
+      end
+
+      context 'when action is already completed' do
+        before do
+          OnboardingProgress.register(namespace, action)
+        end
+
+        it 'does not schedule a worker' do
+          expect(Namespaces::OnboardingProgressWorker).not_to receive(:perform_async)
+
+          execute_service
+        end
+      end
+
+      context 'when action is not yet completed' do
+        it 'schedules a worker' do
+          expect(Namespaces::OnboardingProgressWorker).to receive(:perform_async)
+
+          execute_service
+        end
+      end
+    end
+  end
+
   describe '#execute' do
     let(:namespace) { create(:namespace, parent: root_namespace) }
     let(:root_namespace) { nil }

spec/workers/namespaces/onboarding_progress_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Namespaces::OnboardingProgressWorker, '#perform' do
+  let_it_be(:namespace) { create(:namespace) }
+  let_it_be(:action) { 'git_pull' }
+
+  it_behaves_like 'records an onboarding progress action', :git_pull do
+    include_examples 'an idempotent worker' do
+      subject { described_class.new.perform(namespace.id, action) }
+    end
+  end
+
+  it_behaves_like 'does not record an onboarding progress action' do
+    subject { described_class.new.perform(namespace.id, nil) }
+  end
+
+  it_behaves_like 'does not record an onboarding progress action' do
+    subject { described_class.new.perform(nil, action) }
+  end
+end