From d3a4d2ee7431da7d089f2777aae255d64c8db441 Mon Sep 17 00:00:00 2001 From: Markus Kahl Date: Thu, 28 May 2026 13:26:01 +0100 Subject: [PATCH] fix: delay adding role to make migration not crash due to schema errors (#23426) * fix: delay adding role to make migration not crash due to schema errors * update spec to execute part of migration now done in background --- app/workers/call_service_job.rb | 71 +++++++++++++++++++ ...principals_permission_to_existing_roles.rb | 10 +-- ...ipals_permission_to_existing_roles_spec.rb | 16 +++-- 3 files changed, 88 insertions(+), 9 deletions(-) create mode 100644 app/workers/call_service_job.rb diff --git a/app/workers/call_service_job.rb b/app/workers/call_service_job.rb new file mode 100644 index 00000000000..7766722959a --- /dev/null +++ b/app/workers/call_service_job.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +## +# Calls a service in a background job. +# +# Example: +# +# CallServiceJob.perform_later( +# "Members::AddRoleService", +# { user_id: 42, role_id: 7, project_id: nil, send_notifications: false }, +# check_pending_migrations: true +# ) +class CallServiceJob < ApplicationJob + class ServiceCallFailed < StandardError; end + + # Transient errors are retried + retry_on StandardError, wait: :polynomially_longer, attempts: 3 + + # If there are pending migrations wait for up to an hour (well 78 minutes altogether) + # for them to finish before giving up + retry_on ActiveRecord::PendingMigrationError, wait: :polynomially_longer, attempts: 7 + + # Service result failures are permanent — fail the job immediately without retry. + # Declared after retry_on StandardError so it takes precedence for ServiceCallFailed. + retry_on ServiceCallFailed, attempts: 1 + + queue_with_priority :low + + # @param service_class_name [String] Fully-qualified service class name + # @param kwargs [Hash] Keyword arguments forwarded to service.call(...) + # @param current_user_id [Integer, nil] Current user ID; nil uses the system user + # @param check_pending_migrations [Boolean] Ensure this job is not executed while there are migrations pending. + def perform(service_class_name, kwargs, current_user_id: User.system.id, check_pending_migrations: false) + ActiveRecord::Migration.check_pending_migrations if check_pending_migrations + + service = service_class_name.constantize.new current_user: User.find(current_user_id) + result = service.call **kwargs.symbolize_keys + + result.on_failure do |r| + raise ServiceCallFailed, "#{service_class_name}#call failed: #{r.message}" + end + end +end diff --git a/db/migrate/20250929070310_add_view_all_principals_permission_to_existing_roles.rb b/db/migrate/20250929070310_add_view_all_principals_permission_to_existing_roles.rb index 60d8cf7b09f..814df2723f0 100644 --- a/db/migrate/20250929070310_add_view_all_principals_permission_to_existing_roles.rb +++ b/db/migrate/20250929070310_add_view_all_principals_permission_to_existing_roles.rb @@ -63,12 +63,14 @@ class AddViewAllPrincipalsPermissionToExistingRoles < ActiveRecord::Migration[8. private def add_global_role_for_manage_members - service = Members::AddRoleService.new(current_user: User.system) + role_id = global_role_view_all_users.id find_user_ids_with_manage_members.each do |user_id| - service - .call(user_id:, role_id: global_role_view_all_users.id, project_id: nil, send_notifications: false) - .on_failure { |result| Rails.logger.error("Failed to assign global role to user #{user_id}: #{result.message}") } + CallServiceJob.perform_later( + "Members::AddRoleService", + { user_id:, role_id:, project_id: nil, send_notifications: false }, + check_pending_migrations: true + ) end end diff --git a/spec/migrations/add_view_all_principals_permission_to_existing_roles_spec.rb b/spec/migrations/add_view_all_principals_permission_to_existing_roles_spec.rb index 69e456d2f0e..be23f46fdb7 100644 --- a/spec/migrations/add_view_all_principals_permission_to_existing_roles_spec.rb +++ b/spec/migrations/add_view_all_principals_permission_to_existing_roles_spec.rb @@ -36,6 +36,12 @@ RSpec.describe AddViewAllPrincipalsPermissionToExistingRoles, type: :model do let(:regular_user) { create(:user) } let(:project) { create(:project) } + def migrate + perform_enqueued_jobs do + ActiveRecord::Migration.suppress_messages { described_class.migrate(:up) } + end + end + describe "up migration" do context "when global roles have manage_user permission" do let(:global_role) { create(:global_role, name: "Staff Manager") } @@ -67,7 +73,7 @@ RSpec.describe AddViewAllPrincipalsPermissionToExistingRoles, type: :model do expect(GlobalRole.find_by(name: "View all users (migration)")).to be_nil expect(user_with_manage_members.members.where(project: nil)).to be_empty - ActiveRecord::Migration.suppress_messages { described_class.migrate(:up) } + migrate migration_role = GlobalRole.find_by(name: "View all users (migration)") expect(migration_role).to be_present @@ -79,10 +85,10 @@ RSpec.describe AddViewAllPrincipalsPermissionToExistingRoles, type: :model do end it "does not duplicate assignments for users already having the global role" do - ActiveRecord::Migration.suppress_messages { described_class.migrate(:up) } + migrate # Run migration again - ActiveRecord::Migration.suppress_messages { described_class.migrate(:up) } + migrate migration_role = GlobalRole.find_by(name: "View all users (migration)") user_with_manage_members.reload @@ -106,7 +112,7 @@ RSpec.describe AddViewAllPrincipalsPermissionToExistingRoles, type: :model do end it "assigns the global role only once per user" do - ActiveRecord::Migration.suppress_messages { described_class.migrate(:up) } + migrate migration_role = GlobalRole.find_by(name: "View all users (migration)") user_with_multiple_projects.reload @@ -196,7 +202,7 @@ RSpec.describe AddViewAllPrincipalsPermissionToExistingRoles, type: :model do end it "assigns the migration global role even if user already has view_all_users via global role" do - ActiveRecord::Migration.suppress_messages { described_class.migrate(:up) } + migrate migration_role = GlobalRole.find_by(name: "View all users (migration)") user_with_both.reload