From 4536859663af3095b2b818633619c9a153e7fb88 Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Tue, 17 Dec 2024 15:51:35 +0100 Subject: [PATCH 1/2] Refactor ReplaceReferencesService Instead of expecting modules to selectively overwrite methods of this class, we offer an explicit extension point where replacements can be registered explicitly. Thus patching the implementation is not necessary anymore, instead it can be configured from initializers. --- .../principals/replace_references_service.rb | 105 +++--------------- .../replace_references_service.rb | 54 +++++++++ .../lib/open_project/storages/engine.rb | 10 +- .../replace_references_service_patch.rb | 19 ---- 4 files changed, 80 insertions(+), 108 deletions(-) create mode 100644 config/initializers/replace_references_service.rb delete mode 100644 modules/storages/lib/open_project/storages/patches/replace_references_service_patch.rb diff --git a/app/services/principals/replace_references_service.rb b/app/services/principals/replace_references_service.rb index 14d46adac922..faef6f921ce7 100644 --- a/app/services/principals/replace_references_service.rb +++ b/app/services/principals/replace_references_service.rb @@ -30,6 +30,19 @@ # No data is to be removed. module Principals class ReplaceReferencesService + class << self + attr_reader :replacements, :foreign_keys + + def add_replacement(class_name, attribute) + @replacements ||= {} + @replacements[class_name] ||= Set.new + @replacements[class_name] << attribute + + @foreign_keys ||= Set.new + @foreign_keys << attribute.to_s + end + end + def call(from:, to:) rewrite_active_models(from, to) rewrite_custom_value(from, to) @@ -42,15 +55,10 @@ def call(from:, to:) private def rewrite_active_models(from, to) - rewrite_author(from, to) - rewrite_creator(from, to) - rewrite_user(from, to) - rewrite_assigned_to(from, to) - rewrite_responsible(from, to) - rewrite_actor(from, to) - rewrite_owner(from, to) - rewrite_logged_by(from, to) - rewrite_presenter(from, to) + self.class.replacements.each do |class_name, attributes| + klass = class_name.constantize + attributes.each { |attribute| rewrite(klass, attribute, from, to) } + end end def rewrite_custom_value(from, to) @@ -62,7 +70,7 @@ def rewrite_custom_value(from, to) def rewrite_default_journals(from, to) journal_classes.each do |klass| - foreign_keys.each do |foreign_key| + self.class.foreign_keys.each do |foreign_key| if klass.column_names.include? foreign_key rewrite(klass, foreign_key, from, to) end @@ -78,87 +86,10 @@ def rewrite_customizable_journals(from, to) .update_all(value: to.id.to_s) end - def rewrite_author(from, to) - [WorkPackage, - Attachment, - WikiPage, - News, - Comment, - Message, - Budget, - MeetingAgenda, - MeetingMinutes, - MeetingAgendaItem].each do |klass| - rewrite(klass, :author_id, from, to) - end - end - - def rewrite_creator(from, to) - [AuthProvider].each do |klass| - rewrite(klass, :creator_id, from, to) - end - end - - def rewrite_user(from, to) - [TimeEntry, - CostEntry, - ::Query, - Changeset, - CostQuery, - MeetingParticipant].each do |klass| - rewrite(klass, :user_id, from, to) - end - end - - def rewrite_actor(from, to) - [::Notification].each do |klass| - rewrite(klass, :actor_id, from, to) - end - end - - def rewrite_owner(from, to) - [::Doorkeeper::Application].each do |klass| - rewrite(klass, :owner_id, from, to) - end - end - - def rewrite_assigned_to(from, to) - [WorkPackage].each do |klass| - rewrite(klass, :assigned_to_id, from, to) - end - end - - def rewrite_responsible(from, to) - [WorkPackage].each do |klass| - rewrite(klass, :responsible_id, from, to) - end - end - - def rewrite_logged_by(from, to) - [ - TimeEntry, - CostEntry - ].each do |klass| - rewrite(klass, :logged_by_id, from, to) - end - end - - def rewrite_presenter(from, to) - [ - MeetingAgendaItem - ].each do |klass| - rewrite(klass, :presenter_id, from, to) - end - end - def journal_classes [Journal] + Journal::BaseJournal.subclasses end - def foreign_keys - %w[author_id creator_id user_id assigned_to_id responsible_id logged_by_id presenter_id] - end - def rewrite(klass, attribute, from, to) klass.where(attribute => from.id).update_all(attribute => to.id) end diff --git a/config/initializers/replace_references_service.rb b/config/initializers/replace_references_service.rb new file mode 100644 index 000000000000..39684a6c2708 --- /dev/null +++ b/config/initializers/replace_references_service.rb @@ -0,0 +1,54 @@ +#-- 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. +#++ + +Rails.application.reloader.to_prepare do + Principals::ReplaceReferencesService.add_replacement("AuthProvider", :creator_id) + Principals::ReplaceReferencesService.add_replacement("Attachment", :author_id) + Principals::ReplaceReferencesService.add_replacement("Budget", :author_id) + Principals::ReplaceReferencesService.add_replacement("Changeset", :user_id) + Principals::ReplaceReferencesService.add_replacement("Comment", :author_id) + Principals::ReplaceReferencesService.add_replacement("CostEntry", :logged_by_id) + Principals::ReplaceReferencesService.add_replacement("CostEntry", :user_id) + Principals::ReplaceReferencesService.add_replacement("CostQuery", :user_id) + Principals::ReplaceReferencesService.add_replacement("::Doorkeeper::Application", :owner_id) + Principals::ReplaceReferencesService.add_replacement("MeetingAgenda", :author_id) + Principals::ReplaceReferencesService.add_replacement("MeetingAgendaItem", :author_id) + Principals::ReplaceReferencesService.add_replacement("MeetingAgendaItem", :presenter_id) + Principals::ReplaceReferencesService.add_replacement("MeetingMinutes", :author_id) + Principals::ReplaceReferencesService.add_replacement("MeetingParticipant", :user_id) + Principals::ReplaceReferencesService.add_replacement("Message", :author_id) + Principals::ReplaceReferencesService.add_replacement("News", :author_id) + Principals::ReplaceReferencesService.add_replacement("::Notification", :actor_id) + Principals::ReplaceReferencesService.add_replacement("::Query", :user_id) + Principals::ReplaceReferencesService.add_replacement("TimeEntry", :logged_by_id) + Principals::ReplaceReferencesService.add_replacement("TimeEntry", :user_id) + Principals::ReplaceReferencesService.add_replacement("WikiPage", :author_id) + Principals::ReplaceReferencesService.add_replacement("WorkPackage", :author_id) + Principals::ReplaceReferencesService.add_replacement("WorkPackage", :assigned_to_id) + Principals::ReplaceReferencesService.add_replacement("WorkPackage", :responsible_id) +end diff --git a/modules/storages/lib/open_project/storages/engine.rb b/modules/storages/lib/open_project/storages/engine.rb index d60979349731..2e38777f9a5d 100644 --- a/modules/storages/lib/open_project/storages/engine.rb +++ b/modules/storages/lib/open_project/storages/engine.rb @@ -50,6 +50,14 @@ def self.external_file_permissions # please see comments inside ActsAsOpEngine class include OpenProject::Plugins::ActsAsOpEngine + initializer "openproject_storages.replace_references" do + Rails.application.reloader.to_prepare do + Principals::ReplaceReferencesService.add_replacement("::Storages::Storage", :creator_id) + Principals::ReplaceReferencesService.add_replacement("::Storages::ProjectStorage", :creator_id) + Principals::ReplaceReferencesService.add_replacement("::Storages::FileLink", :creator_id) + end + end + initializer "openproject_storages.feature_decisions" do OpenProject::FeatureDecisions.add :storage_file_picking_select_all end @@ -240,8 +248,6 @@ def self.external_file_permissions end end - patch_with_namespace :Principals, :ReplaceReferencesService - # This hook is executed when the module is loaded. config.to_prepare do # Allow the browser to connect to external servers for direct file uploads. diff --git a/modules/storages/lib/open_project/storages/patches/replace_references_service_patch.rb b/modules/storages/lib/open_project/storages/patches/replace_references_service_patch.rb deleted file mode 100644 index 4e4889495100..000000000000 --- a/modules/storages/lib/open_project/storages/patches/replace_references_service_patch.rb +++ /dev/null @@ -1,19 +0,0 @@ -module OpenProject::Storages::Patches::ReplaceReferencesServicePatch - def self.included(base) # :nodoc: - base.prepend InstanceMethods - end - - module InstanceMethods - private - - def rewrite_creator(from, to) - super - - [::Storages::Storage, - ::Storages::ProjectStorage, - ::Storages::FileLink].each do |klass| - klass.where(creator_id: from.id).update_all(creator_id: to.id) - end - end - end -end From 07a2b146ae73a1a7512d63f91e4e93d35769dc88 Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Thu, 19 Dec 2024 10:52:07 +0100 Subject: [PATCH 2/2] Add short-hand syntaxes to add reference replacements There is now add_replacements which accepts a hash, allowing to add multiple replacements in one call. This should make mass-adding replacements look less bulky. Also including helpers to ActsAsOpEngine to make adding replacements from a module nicer. --- .../principals/replace_references_service.rb | 6 +++ .../replace_references_service.rb | 47 +++++++++---------- lib/open_project/plugins/acts_as_op_engine.rb | 17 +++++++ .../lib/open_project/storages/engine.rb | 12 ++--- 4 files changed, 50 insertions(+), 32 deletions(-) diff --git a/app/services/principals/replace_references_service.rb b/app/services/principals/replace_references_service.rb index faef6f921ce7..a3b7af255f81 100644 --- a/app/services/principals/replace_references_service.rb +++ b/app/services/principals/replace_references_service.rb @@ -33,6 +33,12 @@ class ReplaceReferencesService class << self attr_reader :replacements, :foreign_keys + def add_replacements(attributes_by_class_name) + attributes_by_class_name.each do |class_name, attributes| + Array(attributes).each { |attribute| add_replacement(class_name, attribute) } + end + end + def add_replacement(class_name, attribute) @replacements ||= {} @replacements[class_name] ||= Set.new diff --git a/config/initializers/replace_references_service.rb b/config/initializers/replace_references_service.rb index 39684a6c2708..e04b0ca16cb2 100644 --- a/config/initializers/replace_references_service.rb +++ b/config/initializers/replace_references_service.rb @@ -27,28 +27,27 @@ #++ Rails.application.reloader.to_prepare do - Principals::ReplaceReferencesService.add_replacement("AuthProvider", :creator_id) - Principals::ReplaceReferencesService.add_replacement("Attachment", :author_id) - Principals::ReplaceReferencesService.add_replacement("Budget", :author_id) - Principals::ReplaceReferencesService.add_replacement("Changeset", :user_id) - Principals::ReplaceReferencesService.add_replacement("Comment", :author_id) - Principals::ReplaceReferencesService.add_replacement("CostEntry", :logged_by_id) - Principals::ReplaceReferencesService.add_replacement("CostEntry", :user_id) - Principals::ReplaceReferencesService.add_replacement("CostQuery", :user_id) - Principals::ReplaceReferencesService.add_replacement("::Doorkeeper::Application", :owner_id) - Principals::ReplaceReferencesService.add_replacement("MeetingAgenda", :author_id) - Principals::ReplaceReferencesService.add_replacement("MeetingAgendaItem", :author_id) - Principals::ReplaceReferencesService.add_replacement("MeetingAgendaItem", :presenter_id) - Principals::ReplaceReferencesService.add_replacement("MeetingMinutes", :author_id) - Principals::ReplaceReferencesService.add_replacement("MeetingParticipant", :user_id) - Principals::ReplaceReferencesService.add_replacement("Message", :author_id) - Principals::ReplaceReferencesService.add_replacement("News", :author_id) - Principals::ReplaceReferencesService.add_replacement("::Notification", :actor_id) - Principals::ReplaceReferencesService.add_replacement("::Query", :user_id) - Principals::ReplaceReferencesService.add_replacement("TimeEntry", :logged_by_id) - Principals::ReplaceReferencesService.add_replacement("TimeEntry", :user_id) - Principals::ReplaceReferencesService.add_replacement("WikiPage", :author_id) - Principals::ReplaceReferencesService.add_replacement("WorkPackage", :author_id) - Principals::ReplaceReferencesService.add_replacement("WorkPackage", :assigned_to_id) - Principals::ReplaceReferencesService.add_replacement("WorkPackage", :responsible_id) + Principals::ReplaceReferencesService.add_replacements( + { + "AuthProvider" => :creator_id, + "Attachment" => :author_id, + "Budget" => :author_id, + "Changeset" => :user_id, + "Comment" => :author_id, + "CostEntry" => %i[logged_by_id user_id], + "CostQuery" => :user_id, + "::Doorkeeper::Application" => :owner_id, + "MeetingAgenda" => :author_id, + "MeetingAgendaItem" => %i[author_id presenter_id], + "MeetingMinutes" => :author_id, + "MeetingParticipant" => :user_id, + "Message" => :author_id, + "News" => :author_id, + "::Notification" => :actor_id, + "::Query" => :user_id, + "TimeEntry" => %i[logged_by_id user_id], + "WikiPage" => :author_id, + "WorkPackage" => %i[author_id assigned_to_id responsible_id] + } + ) end diff --git a/lib/open_project/plugins/acts_as_op_engine.rb b/lib/open_project/plugins/acts_as_op_engine.rb index 5e2213a6634b..8cd78d0f83c5 100644 --- a/lib/open_project/plugins/acts_as_op_engine.rb +++ b/lib/open_project/plugins/acts_as_op_engine.rb @@ -313,6 +313,23 @@ def class_inflection_override(overrides) OpenProject::Inflector.inflection(overrides) end end + + # Adds a replacement rule for a principal reference. When the module allows adding a + # principal (e.g. a user) reference to a model, a replacement rule should be added so + # that the user can be deleted, by making sure references to that user are replaced + # on that model as well. + def replace_principal_reference(class_name, attribute) + config.to_prepare do + Principals::ReplaceReferencesService.add_replacement(class_name, attribute) + end + end + + # Like #replace_principal_reference, but allows to add multiple classes and attributes at once. + def replace_principal_references(attributes_by_class_name) + config.to_prepare do + Principals::ReplaceReferencesService.add_replacements(attributes_by_class_name) + end + end end end end diff --git a/modules/storages/lib/open_project/storages/engine.rb b/modules/storages/lib/open_project/storages/engine.rb index 2e38777f9a5d..c98483d74a86 100644 --- a/modules/storages/lib/open_project/storages/engine.rb +++ b/modules/storages/lib/open_project/storages/engine.rb @@ -50,14 +50,6 @@ def self.external_file_permissions # please see comments inside ActsAsOpEngine class include OpenProject::Plugins::ActsAsOpEngine - initializer "openproject_storages.replace_references" do - Rails.application.reloader.to_prepare do - Principals::ReplaceReferencesService.add_replacement("::Storages::Storage", :creator_id) - Principals::ReplaceReferencesService.add_replacement("::Storages::ProjectStorage", :creator_id) - Principals::ReplaceReferencesService.add_replacement("::Storages::FileLink", :creator_id) - end - end - initializer "openproject_storages.feature_decisions" do OpenProject::FeatureDecisions.add :storage_file_picking_select_all end @@ -370,5 +362,9 @@ def self.external_file_permissions } } end + + replace_principal_reference("::Storages::Storage", :creator_id) + replace_principal_reference("::Storages::ProjectStorage", :creator_id) + replace_principal_reference("::Storages::FileLink", :creator_id) end end