Merge branch 'al-273258-auto-approve-users-when-require-admin-approval-disabled' into 'master'

Auto approve users if Admin approval after sign up setting is disabled

See merge request gitlab-org/gitlab!49068

app/services/application_settings/update_service.rb

@@ -9,6 +9,16 @@ module ApplicationSettings
     MARKDOWN_CACHE_INVALIDATING_PARAMS = %w(asset_proxy_enabled asset_proxy_url asset_proxy_secret_key asset_proxy_whitelist).freeze
 
     def execute
+      result = update_settings
+
+      auto_approve_blocked_users if result
+
+      result
+    end
+
+    private
+
+    def update_settings
       validate_classification_label(application_setting, :external_authorization_service_default_label) unless bypass_external_auth?
 
       if application_setting.errors.any?
@@ -40,8 +50,6 @@ module ApplicationSettings
       @application_setting.save
     end
 
-    private
-
     def usage_stats_updated?
       params.key?(:usage_ping_enabled) || params.key?(:version_check_enabled)
     end
@@ -95,6 +103,20 @@ module ApplicationSettings
     def bypass_external_auth?
       params.key?(:external_authorization_service_enabled) && !Gitlab::Utils.to_boolean(params[:external_authorization_service_enabled])
     end
+
+    def auto_approve_blocked_users
+      return unless should_auto_approve_blocked_users?
+
+      ApproveBlockedPendingApprovalUsersWorker.perform_async(current_user.id)
+    end
+
+    def should_auto_approve_blocked_users?
+      return false unless application_setting.previous_changes.key?(:require_admin_approval_after_user_signup)
+
+      enabled_previous, enabled_current = application_setting.previous_changes[:require_admin_approval_after_user_signup]
+
+      enabled_previous && !enabled_current
+    end
   end
 end
 

app/workers/all_queues.yml

@@ -1384,6 +1384,14 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: approve_blocked_pending_approval_users
+  :feature_category: :users
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: authorized_keys
   :feature_category: :source_code_management
   :has_external_dependencies:

ee/app/workers/approve_blocked_users_worker.rb → app/workers/approve_blocked_pending_approval_users_worker.rb

 # frozen_string_literal: true
 
-class ApproveBlockedUsersWorker
+class ApproveBlockedPendingApprovalUsersWorker
   include ApplicationWorker
 
   idempotent!

changelogs/unreleased/al-273258-auto-approve-users-when-require-admin-approval-disabled.yml

+---
+title: Auto approve users if Admin approval after sign up setting is disabled
+merge_request: 49068
+author:
+type: changed

config/sidekiq_queues.yml

@@ -34,7 +34,7 @@
   - 1
 - - analytics_instance_statistics_counter_job
   - 1
-- - approve_blocked_users
+- - approve_blocked_pending_approval_users
   - 1
 - - authorized_keys
   - 2

ee/app/services/ee/application_settings/update_service.rb

@@ -15,13 +15,10 @@ module EE
         elasticsearch_namespace_ids = params.delete(:elasticsearch_namespace_ids)
         elasticsearch_project_ids = params.delete(:elasticsearch_project_ids)
 
-        previous_user_cap = application_setting.new_user_signups_cap
-
         if result = super
           find_or_create_index
           update_elasticsearch_containers(ElasticsearchIndexedNamespace, elasticsearch_namespace_ids)
           update_elasticsearch_containers(ElasticsearchIndexedProject, elasticsearch_project_ids)
-          auto_approve_blocked_users(previous_user_cap)
         end
 
         result
@@ -43,18 +40,22 @@ module EE
         new_container_ids.each { |id| klass.create!(klass.target_attr_name => id) }
       end
 
-      def auto_approve_blocked_users(previous_user_cap)
-        return if ::Feature.disabled?(:admin_new_user_signups_cap)
-        return if previous_user_cap.nil?
-
-        current_user_cap = application_setting.new_user_signups_cap
+      private
 
-        if current_user_cap.nil? || current_user_cap > previous_user_cap
-          ApproveBlockedUsersWorker.perform_async(current_user.id)
-        end
+      def should_auto_approve_blocked_users?
+        super || user_cap_increased?
       end
 
-      private
+      def user_cap_increased?
+        return false unless application_setting.previous_changes.key?(:new_user_signups_cap)
+        return false unless ::Feature.enabled?(:admin_new_user_signups_cap)
+
+        previous_user_cap, current_user_cap = application_setting.previous_changes[:new_user_signups_cap]
+
+        return false if previous_user_cap.nil?
+
+        current_user_cap.nil? || current_user_cap > previous_user_cap
+      end
 
       def find_or_create_index
         # The order of checks is important. We should not attempt to create a new index

ee/app/workers/all_queues.yml

@@ -653,14 +653,6 @@
   :weight: 1
   :idempotent: true
   :tags: []
-- :name: approve_blocked_users
-  :feature_category: :users
-  :has_external_dependencies:
-  :urgency: :low
-  :resource_boundary: :unknown
-  :weight: 1
-  :idempotent: true
-  :tags: []
 - :name: ci_batch_reset_minutes
   :feature_category: :continuous_integration
   :has_external_dependencies:

ee/spec/services/application_settings/update_service_spec.rb

@@ -167,16 +167,16 @@ RSpec.describe ApplicationSettings::UpdateService do
 
     context 'user cap setting' do
       shared_examples 'worker is not called' do
-        it 'does not call ApproveBlockedUsersWorker' do
-          expect(ApproveBlockedUsersWorker).not_to receive(:perform_async)
+        it 'does not call ApproveBlockedPendingApprovalUsersWorker' do
+          expect(ApproveBlockedPendingApprovalUsersWorker).not_to receive(:perform_async)
 
           service.execute
         end
       end
 
       shared_examples 'worker is called' do
-        it 'calls ApproveBlockedUsersWorker' do
-          expect(ApproveBlockedUsersWorker).to receive(:perform_async)
+        it 'calls ApproveBlockedPendingApprovalUsersWorker' do
+          expect(ApproveBlockedPendingApprovalUsersWorker).to receive(:perform_async)
 
           service.execute
         end
@@ -198,7 +198,7 @@ RSpec.describe ApplicationSettings::UpdateService do
 
       context 'when new user cap is set to a number' do
         let(:setting) do
-          build(:application_setting, new_user_signups_cap: 10)
+          create(:application_setting, new_user_signups_cap: 10)
         end
 
         context 'when decreasing new user cap' do

spec/services/application_settings/update_service_spec.rb

@@ -350,4 +350,28 @@ RSpec.describe ApplicationSettings::UpdateService do
       expect(application_settings.issues_create_limit).to eq(600)
     end
   end
+
+  context 'when require_admin_approval_after_user_signup changes' do
+    context 'when it goes from enabled to disabled' do
+      let(:params) { { require_admin_approval_after_user_signup: false } }
+
+      it 'calls ApproveBlockedPendingApprovalUsersWorker' do
+        expect(ApproveBlockedPendingApprovalUsersWorker).to receive(:perform_async)
+
+        subject.execute
+      end
+    end
+
+    context 'when it goes from disabled to enabled' do
+      let(:params) { { require_admin_approval_after_user_signup: true } }
+
+      it 'does not call ApproveBlockedPendingApprovalUsersWorker' do
+        application_settings.update!(require_admin_approval_after_user_signup: false)
+
+        expect(ApproveBlockedPendingApprovalUsersWorker).not_to receive(:perform_async)
+
+        subject.execute
+      end
+    end
+  end
 end

ee/spec/workers/approve_blocked_users_worker_spec.rb → spec/workers/approve_blocked_pending_approval_users_worker_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe ApproveBlockedUsersWorker, type: :worker do
+RSpec.describe ApproveBlockedPendingApprovalUsersWorker, type: :worker do
   let_it_be(:admin) { create(:admin) }
   let_it_be(:active_user) { create(:user) }
   let_it_be(:blocked_user) { create(:user, state: 'blocked_pending_approval') }