Merge pull request #15236 from opf/fix/transaction_for_update_progres…

…s_job

Fix/transaction for update progress job

app/services/journals/create_service.rb

@@ -48,7 +48,7 @@ def initialize(journable, user)
       self.journable = journable
     end
 
-    def call(notes: '', cause: {})
+    def call(notes: "", cause: {})
       # JSON columns read from the database always have string keys. As we do not know what is passed in here,
       # and we want to compare it to values read from the DB, we need to stringify the keys here as well
       normalized_cause = cause.deep_stringify_keys

app/workers/work_packages/apply_statuses_p_complete_job.rb

@@ -88,10 +88,12 @@ def journal_cause_from(cause_type, status_name:, status_id:, change:)
   end
 
   def with_temporary_progress_table
-    create_temporary_progress_table
-    yield
-  ensure
-    drop_temporary_progress_table
+    WorkPackage.transaction do
+      create_temporary_progress_table
+      yield
+    ensure
+      drop_temporary_progress_table
+    end
   end
 
   def create_temporary_progress_table

app/workers/work_packages/update_progress_job.rb

@@ -60,10 +60,12 @@ def perform(current_mode:, previous_mode:)
   private
 
   def with_temporary_progress_table
-    create_temporary_progress_table
-    yield
-  ensure
-    drop_temporary_progress_table
+    WorkPackage.transaction do
+      create_temporary_progress_table
+      yield
+    ensure
+      drop_temporary_progress_table
+    end
   end
 
   def create_temporary_progress_table
@@ -242,7 +244,8 @@ def copy_progress_values_to_work_packages
 
   def create_journals_for_updated_work_packages(updated_work_package_ids)
     WorkPackage.where(id: updated_work_package_ids).find_each do |work_package|
-      Journals::CreateService.new(work_package, system_user)
+      Journals::CreateService
+        .new(work_package, system_user)
         .call(cause: { type: "system_update", feature: "progress_calculation_changed" })
     end
   end

db/migrate/20240402072213_update_progress_calculation.rb

@@ -8,7 +8,7 @@ def up
       current_mode = "field"
     end
 
-    perform_method = Rails.env.production? ? :perform_later : :perform_now
+    perform_method = Rails.env.development? ? :perform_now : :perform_later
     WorkPackages::UpdateProgressJob.public_send(perform_method, current_mode:, previous_mode:)
   end
 

spec/migrations/update_progress_calculation_spec.rb

@@ -31,7 +31,11 @@
 
 RSpec.describe UpdateProgressCalculation, type: :model do
   # Silencing migration logs, since we are not interested in that during testing
-  subject(:run_migration) { ActiveRecord::Migration.suppress_messages { described_class.new.up } }
+  subject(:run_migration) do
+    perform_enqueued_jobs do
+      ActiveRecord::Migration.suppress_messages { described_class.new.up }
+    end
+  end
 
   shared_let(:author) { create(:user) }
   shared_let(:priority) { create(:priority, name: "Normal") }
@@ -686,4 +690,47 @@ def expect_migrates(from:, to:)
       end
     end
   end
+
+  describe "error during job execution" do
+    let_work_packages(<<~TABLE)
+      subject     | work | remaining work | % complete | ∑ work | ∑ remaining work | ∑ % complete
+      wp working  |      |             4h |        60% |    10h |               4h |          60%
+      wp breaking |      |             4h |        60% |    10h |               4h |          60%
+    TABLE
+
+    before do
+      Setting.work_package_done_ratio = "field"
+
+      allow(Journals::CreateService)
+        .to receive(:new)
+              .with(wp_working, User.system)
+              .and_call_original
+
+      allow(Journals::CreateService)
+              .to receive(:new)
+                    .with(wp_breaking, User.system)
+                    .and_return(nil)
+
+      ActiveRecord::Migration.suppress_messages { described_class.new.up }
+
+      begin
+        perform_enqueued_jobs
+      rescue StandardError
+      end
+    end
+
+    it "does not create a journal entry" do
+      table_work_packages.each do |wp|
+        expect(wp.journals.count).to eq(1)
+      end
+    end
+
+    it "does not update the work packages" do
+      expect_work_packages(WorkPackage.all, <<~TABLE)
+        subject     | work | remaining work | % complete | ∑ work | ∑ remaining work | ∑ % complete
+        wp working  |      |             4h |        60% |    10h |               4h |          60%
+        wp breaking |      |             4h |        60% |    10h |               4h |          60%
+      TABLE
+    end
+  end
 end

spec/workers/work_packages/apply_statuses_p_complete_job_spec.rb

@@ -219,4 +219,44 @@ def expect_performing_job_changes(from:, to:,
       end
     end
   end
+
+  context "with errors during job execution",
+          with_settings: { work_package_done_ratio: "status" } do
+    let_work_packages(<<~TABLE)
+      subject     | status      | % complete
+      wp          | Doing (40%) |
+      wp 0%       | To do (0%)  |        50%
+      wp 40%      | Doing (40%) |        50%
+      wp 100%     | Done (100%) |        50%
+    TABLE
+
+    before do
+      allow(Journals::CreateService)
+        .to receive(:new)
+              .and_call_original
+
+      allow(Journals::CreateService)
+        .to receive(:new)
+              .with(WorkPackage.last, User.system)
+              .and_return(nil)
+
+      begin
+        job.perform_now(cause_type: "status_p_complete_changed",
+                        status_name: "New",
+                        status_id: 99,
+                        change: [33, 66])
+      rescue StandardError
+      end
+    end
+
+    it "does not update any work package" do
+      expect_work_packages(WorkPackage.all, <<~TABLE)
+        subject     | status      | % complete
+        wp          | Doing (40%) |
+        wp 0%       | To do (0%)  |        50%
+        wp 40%      | Doing (40%) |        50%
+        wp 100%     | Done (100%) |        50%
+      TABLE
+    end
+  end
 end