use default update service for wps

app/services/attachments/replace_attachments.rb

@@ -34,13 +34,11 @@ module Attachments
       private
 
       def set_attributes(attributes)
-        call = super
-
-        if call.success? && call.result.attachments_replacements
-          call.result.attachments = call.result.attachments_replacements
+        super.tap do |call|
+          if call.success? && call.result.attachments_replacements
+            call.result.attachments = call.result.attachments_replacements
+          end
         end
-
-        call
       end
     end
   end

app/services/work_packages/set_attributes_service.rb

@@ -90,7 +90,7 @@ class WorkPackages::SetAttributesService < ::BaseServices::SetAttributes
                                 elsif parent_start_earlier_than_due?
                                   work_package.parent.start_date
                                 elsif Setting.work_package_startdate_is_adddate?
-                                  Date.today
+                                  Time.zone.today
                                 end
   end
 
@@ -111,7 +111,7 @@ class WorkPackages::SetAttributesService < ::BaseServices::SetAttributes
 
     # And the new type has a default text
     default_description = work_package.type&.description
-    return unless default_description.present?
+    return if default_description.blank?
 
     # And the current description matches ANY current default text
     return unless work_package.description.blank? || default_description?
@@ -155,6 +155,7 @@ class WorkPackages::SetAttributesService < ::BaseServices::SetAttributes
     model.change_by_system do
       set_version_to_nil
       reassign_category
+      set_parent_to_nil
 
       reassign_type unless work_package.type_id_changed?
     end
@@ -194,6 +195,14 @@ class WorkPackages::SetAttributesService < ::BaseServices::SetAttributes
     end
   end
 
+  def set_parent_to_nil
+    if !Setting.cross_project_work_package_relations? &&
+      !work_package.parent_changed?
+
+      work_package.parent = nil
+    end
+  end
+
   def reassign_category
     # work_package is moved to another project
     # reassign to the category with same name if any

app/services/work_packages/set_schedule_service.rb

@@ -176,9 +176,9 @@ class WorkPackages::SetScheduleService
 
   def date_rescheduling_delta(predecessor)
     if predecessor.due_date.present?
-      predecessor.due_date - (predecessor.due_date_was || predecessor.due_date)
+      predecessor.due_date - (predecessor.due_date_before_last_save || predecessor.due_date_was || predecessor.due_date)
     elsif predecessor.start_date.present?
-      predecessor.start_date - (predecessor.start_date_was || predecessor.start_date)
+      predecessor.start_date - (predecessor.start_date_before_last_save || predecessor.start_date_was || predecessor.start_date)
     else
       0
     end

app/services/work_packages/update_service.rb

@@ -26,97 +26,63 @@
 # See COPYRIGHT and LICENSE files for more details.
 #++
 
-# TODO: use default update base class
-class WorkPackages::UpdateService < ::BaseServices::BaseCallable
+class WorkPackages::UpdateService < ::BaseServices::Update
   include ::WorkPackages::Shared::UpdateAncestors
-  include ::Shared::ServiceContext
+  include Attachments::ReplaceAttachments
 
-  attr_accessor :user,
-                :model,
-                :contract_class
+  private
 
-  def initialize(user:, model:, contract_class: WorkPackages::UpdateContract)
-    self.user = user
-    self.model = model
-    self.contract_class = contract_class
-  end
+  def after_perform(service_call)
+    update_related_work_packages(service_call)
+    cleanup(service_call.result)
 
-  def perform(send_notifications: true, **attributes)
-    in_context(model, send_notifications) do
-      update(attributes)
-    end
+    service_call
   end
 
-  private
-
-  def update(attributes)
-    result = set_attributes(attributes)
-
-    if result.success?
-      work_package.attachments = work_package.attachments_replacements if work_package.attachments_replacements
-      result.merge!(update_dependent)
+  def update_related_work_packages(service_call)
+    update_ancestors([service_call.result]).each do |ancestor_service_call|
+      service_call.merge!(ancestor_service_call)
     end
 
-    if save_if_valid(result)
-      update_ancestors([work_package]).each do |ancestor_result|
-        result.merge!(ancestor_result)
-      end
+    update_related(service_call.result).each do |related_service_call|
+      service_call.merge!(related_service_call)
     end
-
-    result
   end
 
-  def save_if_valid(result)
-    if result.success?
-      result.success = consolidated_results(result)
-                         .all? { |wp| wp.save(validate: false) }
-    end
-
-    result.success?
+  def update_related(work_package)
+    consolidated_calls(update_descendants(work_package) + reschedule_related(work_package))
+      .reject { |dependent_call| dependent_call.result.id == work_package.id }
+      .each { |dependent_call| dependent_call.result.save(validate: false) }
   end
 
-  def update_dependent
-    result = ServiceResult.new(success: true, result: work_package)
-
-    result.merge!(update_descendants)
-
-    cleanup if result.success?
-
-    result.merge!(reschedule_related)
+  def update_descendants(work_package)
+    if work_package.saved_change_to_project_id?
+      attributes = { project: work_package.project }
 
-    result
+      work_package.descendants.map do |descendant|
+        set_descendant_attributes(attributes, descendant)
+      end
+    else
+      []
+    end
   end
 
-  def set_attributes(attributes, wp = work_package)
+  def set_descendant_attributes(attributes, descendant)
     WorkPackages::SetAttributesService
       .new(user:,
-           model: wp,
-           contract_class:)
+           model: descendant,
+           contract_class: EmptyContract)
       .call(attributes)
   end
 
-  def update_descendants
-    result = ServiceResult.new(success: true, result: work_package)
-
-    if work_package.project_id_changed?
-      attributes = { project: work_package.project }
-
-      work_package.descendants.each do |descendant|
-        result.add_dependent!(set_attributes(attributes, descendant))
-      end
-    end
-
-    result
-  end
-
-  def cleanup
-    if work_package.project_id_changed?
+  def cleanup(work_package)
+    if work_package.saved_change_to_project_id?
       moved_work_packages = [work_package] + work_package.descendants
       delete_relations(moved_work_packages)
       move_time_entries(moved_work_packages, work_package.project_id)
     end
-    if work_package.type_id_changed?
-      reset_custom_values
+    if work_package.saved_change_to_type_id?
+      reset_custom_values(work_package)
     end
   end
 
@@ -134,60 +100,29 @@ class WorkPackages::UpdateService < ::BaseServices::BaseCallable
       .update_all(project_id:)
   end
 
-  def reset_custom_values
+  def reset_custom_values(work_package)
     work_package.reset_custom_values!
   end
 
-  def reschedule_related
-    result = ServiceResult.new(success: true, result: work_package)
-
-    with_temporarily_persisted_parent_changes do
-      if work_package.parent_id_changed? && work_package.parent_id_was
-        result.merge!(reschedule_former_siblings)
-      end
-
-      result.merge!(reschedule(work_package))
-    end
-
-    result
-  end
-
-  def with_temporarily_persisted_parent_changes
-    # Explicitly using requires_new: true since we are already within a transaction.
-    # Because of that, raising ActiveRecord::Rollback would have no effect:
-    # https://www.bigbinary.com/learn-rubyonrails-book/activerecord-transactions-in-depth#nested-transactions
-    WorkPackage.transaction(requires_new: true) do
-      if work_package.parent_id_changed?
-        # HACK: we need to persist the parent relation before rescheduling the parent
-        # and the former parent since we rely on the database for scheduling.
-        # The following will update the parent_id of the work package without that being noticed by the
-        # work package instance (work_package) that is already instantiated. That way, the change can be rolled
-        # back without any side effects to the instance (e.g. dirty tracking).
-        WorkPackage.where(id: work_package.id).update_all(parent_id: work_package.parent_id)
-        work_package.rebuild! # using the ClosureTree#rebuild! method to update the transitive hierarchy information
-      end
-
-      yield
+  def reschedule_related(work_package)
+    rescheduled = if work_package.saved_change_to_parent_id? && work_package.parent_id_before_last_save
+                    reschedule_former_siblings(work_package).dependent_results
+                  else
+                    []
+                  end
 
-      # Always rolling back the changes we made in here
-      raise ActiveRecord::Rollback
-    end
+    rescheduled + reschedule(work_package, [work_package]).dependent_results
   end
 
-  # Rescheduling the former siblings will lead to the whole former tree being rescheduled.
-  def reschedule_former_siblings
-    reschedule(WorkPackage.where(parent_id: work_package.parent_id_was))
+  def reschedule_former_siblings(work_package)
+    reschedule(work_package, WorkPackage.where(parent_id: work_package.parent_id_before_last_save))
   end
 
-  def reschedule(work_packages)
+  def reschedule(work_package, work_packages)
     WorkPackages::SetScheduleService
       .new(user:,
            work_package: work_packages)
-      .call(changed_attributes)
-  end
-
-  def changed_attributes
-    work_package.changed.map(&:to_sym)
+      .call(work_package.saved_changes.keys.map(&:to_sym))
   end
 
   # When multiple services change a work package, we still only want one update to the database due to:
@@ -195,19 +130,15 @@ class WorkPackages::UpdateService < ::BaseServices::BaseCallable
   # * having only one journal entry
   # * stale object errors
   # we thus consolidate the results so that one instance contains the changes made by all the services.
-  def consolidated_results(result)
-    result.all_results.group_by(&:id).inject([]) do |a, (_, instances)|
-      master = instances.pop
-
-      instances.each do |instance|
-        master.attributes = instance.changes.transform_values(&:last)
+  def consolidated_calls(service_calls)
+    service_calls
+      .group_by { |sc| sc.result.id }
+      .map do |(_, same_work_package_calls)|
+      same_work_package_calls.pop.tap do |master|
+        same_work_package_calls.each do |sc|
+          master.result.attributes = sc.result.changes.transform_values(&:last)
+        end
       end
-
-      a + [master]
     end
   end
-
-  def work_package
-    model
-  end
 end

modules/backlogs/lib/open_project/backlogs/list.rb

@@ -81,8 +81,10 @@ module OpenProject::Backlogs::List
 
     protected
 
-    def assume_bottom_position
-      update_columns(position: bottom_position_in_list(self).to_i + 1)
+    # Overwriting acts/list to avoid it calling save.
+    # Calling save will remove the changes/saved_changes information.
+    def set_list_position(new_position, _raise_exception_if_save_fails = false)
+      update_columns(position: new_position)
     end
 
     def fix_other_work_package_positions

modules/backlogs/lib/open_project/backlogs/patches/update_service_patch.rb

@@ -32,36 +32,44 @@ module OpenProject::Backlogs::Patches::UpdateServicePatch
   end
 
   module InstanceMethods
-    def update_descendants
+    def update_descendants(work_package)
       super_result = super
 
-      if work_package.in_backlogs_type? && work_package.version_id_changed?
-        inherit_version_to_descendants(super_result)
+      if work_package.in_backlogs_type? && work_package.saved_change_to_version_id?
+        super_result += inherit_version_to_descendants(work_package)
       end
 
       super_result
     end
 
-    def inherit_version_to_descendants(result)
-      all_descendants = work_package
-                          .descendants
-                          .includes(project: :enabled_modules)
-                          .order_by_ancestors('asc')
-                          .select('work_packages.*')
-      stop_descendants_ids = []
-
-      descendant_tasks = all_descendants.reject do |t|
-        if stop_descendants_ids.include?(t.parent_id) || !t.is_task?
-          stop_descendants_ids << t.id
-        end
-      end
+    def inherit_version_to_descendants(work_package)
+      all_descendants = sorted_descendants(work_package)
+      descendant_tasks = descendant_tasks_of(all_descendants)
 
       attributes = { version_id: work_package.version_id }
 
-      descendant_tasks.each do |task|
+      descendant_tasks.map do |task|
         # Ensure the parent is already moved to new version so that validation errors are avoided.
         task.parent = ([work_package] + all_descendants).detect { |d| d.id == task.parent_id }
-        result.add_dependent!(set_attributes(attributes, task))
+        set_descendant_attributes(attributes, task)
+      end
+    end
+
+    def sorted_descendants(work_package)
+      work_package
+        .descendants
+        .includes(project: :enabled_modules)
+        .order_by_ancestors('asc')
+        .select('work_packages.*')
+    end
+
+    def descendant_tasks_of(descendants)
+      stop_descendants_ids = []
+
+      descendants.reject do |t|
+        if stop_descendants_ids.include?(t.parent_id) || !t.is_task?
+          stop_descendants_ids << t.id
+        end
       end
     end
   end

spec/services/work_packages/set_attributes_service_spec.rb

@@ -1096,6 +1096,33 @@ describe WorkPackages::SetAttributesService, type: :model do
           end
         end
       end
+
+      context 'for parent' do
+        let(:parent_work_package) { build_stubbed(:work_package, project:) }
+        let(:work_package) do
+          build_stubbed(:work_package, project:, type: initial_type, parent: parent_work_package)
+        end
+
+        context 'with cross project relations allowed', with_settings: { cross_project_work_package_relations: true } do
+          it 'keeps the parent' do
+            expect(subject)
+              .to be_success
+
+            expect(work_package.parent)
+              .to eql(parent_work_package)
+          end
+        end
+
+        context 'with cross project relations disabled', with_settings: { cross_project_work_package_relations: false } do
+          it 'deletes the parent' do
+            expect(subject)
+              .to be_success
+
+            expect(work_package.parent)
+              .to be_nil
+          end
+        end
+      end
     end
 
     context 'when updating project before calling the service' do

spec/services/work_packages/update_service_spec.rb

@@ -27,238 +27,15 @@
 #++
 
 require 'spec_helper'
+require 'services/base_services/behaves_like_update_service'
 
 describe WorkPackages::UpdateService, type: :model do
-  let(:user) { build_stubbed(:user) }
-  let(:project) do
-    p = build_stubbed(:project)
-    allow(p).to receive(:shared_versions).and_return([])
-
-    p
-  end
-  let(:work_package) do
-    wp = build_stubbed(:work_package, project:)
-    wp.type = build_stubbed(:type)
-    wp.send(:clear_changes_information)
-
-    wp
-  end
-  let(:instance) do
-    described_class.new(user:,
-                        model: work_package)
-  end
-
-  before do
-    # Stub update_ancestors because it messes with the jouralizing expectations
-    allow(instance).to receive(:update_ancestors).and_return []
-  end
-
-  describe 'call' do
-    let(:set_attributes_service) do
-      service = double("WorkPackages::SetAttributesService",
-                       new: set_attributes_service_instance)
-
-      stub_const('WorkPackages::SetAttributesService', service)
-
-      service
-    end
-    let(:send_notifications) { true }
-    let(:set_attributes_service_instance) do
-      instance = double("WorkPackages::SetAttributesServiceInstance")
-
-      allow(instance)
-        .to receive(:call) do |attributes|
-        work_package.attributes = attributes
-
-        set_service_results
-      end
-
-      instance
-    end
-    let(:errors) { [] }
-    let(:set_service_results) { ServiceResult.new success: true, result: work_package }
-    let(:work_package_save_result) { true }
-
+  # This is now only a very basic test testing the structure of the service.
+  # The domain tests are in the update_service_integration_spec.rb
+  it_behaves_like 'BaseServices update service' do
     before do
-      set_attributes_service
-    end
-
-    before do
-      expect(Journal::NotificationConfiguration)
-        .to receive(:with)
-        .with(send_notifications)
-        .and_yield
-
-      allow(work_package)
-        .to receive(:save)
-        .and_return work_package_save_result
-    end
-
-    shared_examples_for 'service call' do
-      subject { instance.call(**call_attributes.merge(send_notifications:).symbolize_keys) }
-
-      it 'is successful' do
-        expect(subject.success?).to be_truthy
-      end
-
-      it 'sets the value' do
-        subject
-
-        attributes.each do |attribute, key|
-          expect(work_package.send(attribute)).to eql key
-        end
-      end
-
-      it 'has no errors' do
-        expect(subject.errors.all?(&:empty?)).to be_truthy
-      end
-
-      context 'when setting the attributes is unsuccessful (invalid)' do
-        let(:errors) { ActiveModel::Errors.new(work_package) }
-        let(:set_service_results) { ServiceResult.new success: false, errors:, result: work_package }
-
-        it 'is unsuccessful' do
-          expect(subject.success?).to be_falsey
-        end
-
-        it 'does not persist the changes' do
-          subject
-
-          expect(work_package).not_to receive(:save)
-        end
-
-        it 'exposes the errors' do
-          errors.add(:base, 'This is a custom error!')
-
-          subject
-
-          expect(subject.errors).to eql errors
-          expect(subject.errors[:base]).to include 'This is a custom error!'
-        end
-      end
-
-      context 'when the saving is unsuccessful' do
-        let(:work_package_save_result) { false }
-        let(:saving_errors) { ActiveModel::Errors.new(work_package) }
-
-        before do
-          allow(work_package)
-            .to receive(:errors)
-            .and_return(saving_errors)
-        end
-
-        it 'is unsuccessful' do
-          expect(subject.success?).to be_falsey
-        end
-
-        it 'leaves the value unchanged' do
-          subject
-
-          expect(work_package.changed?).to be_truthy
-        end
-
-        it 'exposes the errors, but is a new instance' do
-          saving_errors.add(:base, 'This is a custom error!')
-
-          subject
-
-          expect(subject.errors).not_to eql saving_errors
-          expect(subject.errors[:base]).to include 'This is a custom error!'
-        end
-      end
-    end
-
-    context 'update subject before calling the service' do
-      let(:call_attributes) { {} }
-      let(:attributes) { { subject: 'blubs blubs' } }
-
-      before do
-        work_package.attributes = attributes
-      end
-
-      it_behaves_like 'service call'
-    end
-
-    context 'updating subject via attributes' do
-      let(:call_attributes) { attributes }
-      let(:attributes) { { subject: 'blubs blubs' } }
-
-      it_behaves_like 'service call'
-    end
-
-    context 'when switching the project' do
-      let(:target_project) do
-        build_stubbed(:project)
-      end
-      let(:call_attributes) { attributes }
-      let(:attributes) { { project: target_project } }
-
-      it_behaves_like 'service call'
-
-      context 'relations' do
-        let!(:scope) do
-          instance_double(ActiveRecord::Relation).tap do |relations|
-            allow(Relation)
-              .to receive(:of_work_package)
-                    .with([work_package])
-                    .and_return(relations)
-            allow(relations)
-              .to receive(:destroy_all)
-          end
-        end
-
-        it 'removes the relations if the setting does not permit cross project relations' do
-          allow(Setting)
-            .to receive(:cross_project_work_package_relations?)
-            .and_return false
-
-          instance.call(project: target_project)
-
-          expect(scope)
-            .to have_received(:destroy_all)
-        end
-
-        it 'leaves the relations unchanged if the setting allows cross project relations' do
-          allow(Setting)
-            .to receive(:cross_project_work_package_relations?)
-                  .and_return true
-
-          instance.call(project: target_project)
-
-          expect(scope)
-            .not_to have_received(:destroy_all)
-        end
-      end
-
-      context 'time_entries' do
-        it 'moves the time entries' do
-          scope = double('scope')
-
-          expect(TimeEntry)
-            .to receive(:on_work_packages)
-            .with([work_package])
-            .and_return(scope)
-
-          expect(scope)
-            .to receive(:update_all)
-            .with(project_id: target_project.id)
-
-          instance.call(project: target_project)
-        end
-      end
-    end
-
-    context 'when switching the type' do
-      let(:target_type) { build_stubbed(:type) }
-
-      context 'custom_values' do
-        it 'resets the custom values' do
-          expect(work_package)
-            .to receive(:reset_custom_values!)
-
-          instance.call(type: target_type)
-        end
-      end
+      allow(set_attributes_errors)
+        .to receive(:merge!)
     end
   end
 end