Fix update_canvas

app/models/experiment.rb

@@ -132,14 +132,25 @@ class Experiment < ApplicationRecord
     positions,
     current_user
   )
-    cloned_modules = []
     begin
       with_lock do
-        # First, add new modules
+        # Start with archiving to release positions for new tasks
+        archive_modules(to_archive, current_user) if to_archive.any?
+
+        # Update only existing tasks positions to release positions for new tasks
+        existing_positions = positions
+                             .slice(*positions.keys.map { |k| k unless k.to_s.start_with?('n') }.compact)
+        update_module_positions(existing_positions) if existing_positions.any?
+
+        # Move only existing tasks to release positions for new tasks
+        existing_to_move = to_move
+                            .slice(*to_move.keys.map { |k| k unless k.to_s.start_with?('n') }.compact)
+        move_modules(existing_to_move, current_user) if existing_to_move.any?
+
+        # add new modules
         new_ids, cloned_pairs, originals = add_modules(
           to_add, to_clone, current_user
         )
-        cloned_modules = cloned_pairs.collect { |mn, _| mn }
 
         # Rename modules
         rename_modules(to_rename, current_user)
@@ -161,9 +172,6 @@ class Experiment < ApplicationRecord
                                    my_module_new: mn.id })
         end
 
-        # Then, archive modules that need to be archived
-        archive_modules(to_archive, current_user) if to_archive.any?
-
         # Update connections, positions & module group variables
         # with actual IDs retrieved from the new modules creation
         updated_to_move = {}
@@ -325,9 +333,9 @@ class Experiment < ApplicationRecord
     to_move.each do |id, experiment_id|
       my_module = my_modules.find_by_id(id)
       experiment = project.experiments.find_by_id(experiment_id)
-      experiment_org = my_module.experiment
       next unless my_module.present? && experiment.present?
 
+      experiment_original = my_module.experiment
       my_module.experiment = experiment
 
       # Calculate new module position
@@ -349,7 +357,7 @@ class Experiment < ApplicationRecord
                                              message_items: {
                                                my_module: my_module.id,
                                                experiment_original:
-                                                 experiment_org.id,
+                                                 experiment_original.id,
                                                experiment_new: experiment.id
                                              })
     end

spec/factories/my_modules.rb

@@ -3,8 +3,8 @@
 FactoryBot.define do
   factory :my_module do
     sequence(:name) { |n| "Task-#{n}" }
-    x { Faker::Number.between(from: 1, to: 100) }
     sequence(:y) { |n| n }
+    sequence(:x) { |n| n }
     workflow_order { MyModule.where(experiment_id: experiment.id).count + 1 }
     experiment
     my_module_group { create :my_module_group, experiment: experiment }

spec/models/experiment_spec.rb

@@ -92,12 +92,8 @@ describe Experiment, type: :model do
     let(:user) { experiment.created_by }
 
     context 'when creating tasks' do
-      let(:to_add) do
-        [{ id: 'n0', name: 'new task name', x: 50, y: 50 }]
-      end
-      let(:function_call) do
-        experiment.update_canvas([], to_add, [], [], [], [], [], [], user)
-      end
+      let(:to_add) { [{ id: 'n0', name: 'new task name', x: 50, y: 50 }] }
+      let(:function_call) { experiment.update_canvas([], to_add, [], {}, [], [], [], {}, user) }
 
       it 'calls create activity for creating tasks' do
         expect(Activities::CreateActivityService)
@@ -112,6 +108,57 @@ describe Experiment, type: :model do
       it 'adds activity in DB' do
         expect { function_call }.to(change { Activity.all.count })
       end
+
+      context 'when moving existing one and creating new one on the same position' do
+        let(:first_task) { experiment.my_modules.first }
+        let(:to_add) { [{ id: 'n0', name: 'new task name', x: 0, y: 0 }] }
+        let(:function_call) { experiment.update_canvas([], to_add, [], {}, [], [], [], positions, user) }
+        let(:positions) do
+          Hash[first_task.id.to_s,
+               { x: first_task.x + 1, y: first_task.y + 1 }, 'n0', { x: first_task.x, y: first_task.y }]
+        end
+
+        before do
+          first_task.update(x: 0, y: 0)
+        end
+
+        it 'returns true' do
+          expect(function_call).to be_truthy
+        end
+      end
+
+      context 'when creating new one on position of "toBeArchived" task' do
+        let(:first_task) { experiment.my_modules.first }
+        let(:to_add) { [{ id: 'n0', name: 'new task name', x: 0, y: 0 }] }
+        let(:positions) { Hash['n0', { x: first_task.x, y: first_task.y }] }
+        let(:to_archive) { [first_task.id] }
+        let(:function_call) { experiment.update_canvas(to_archive, to_add, [], {}, [], [], [], positions, user) }
+
+        before do
+          first_task.update(x: 0, y: 0)
+        end
+
+        it 'returns true' do
+          expect(function_call).to be_truthy
+        end
+      end
+
+      context 'when creating new one on position of "toBeMoved" task' do
+        let(:first_task) { experiment.my_modules.first }
+        let(:to_add) { [{ id: 'n0', name: 'new task name', x: 0, y: 0 }] }
+        let(:positions) { Hash['n0', { x: first_task.x, y: first_task.y }] }
+        let(:to_move) { Hash[first_task.id, second_experiment.id] }
+        let(:second_experiment) { create :experiment, project: experiment.project }
+        let(:function_call) { experiment.update_canvas([], to_add, [], to_move, [], [], [], positions, user) }
+
+        before do
+          first_task.update(x: 0, y: 0)
+        end
+
+        it 'returns true' do
+          expect(function_call).to be_truthy
+        end
+      end
     end
 
     context 'when cloning tasks' do
@@ -131,7 +178,7 @@ describe Experiment, type: :model do
           .map.with_index { |t, i| { 'n' + i.to_s => t.id } }.reduce({}, :merge)
       end
       let(:function_call) do
-        experiment.update_canvas([], to_add, [], [], [], to_clone, [], [], user)
+        experiment.update_canvas([], to_add, [], {}, [], to_clone, [], {}, user)
       end
 
       it 'calls create activity for cloning tasks' do
@@ -159,7 +206,7 @@ describe Experiment, type: :model do
       end
 
       let(:function_call) do
-        experiment.update_canvas([], [], to_rename, [], [], [], [], [], user)
+        experiment.update_canvas([], [], to_rename, {}, [], [], [], {}, user)
       end
 
       it 'calls create activity for renaming my_moudles' do
@@ -180,7 +227,7 @@ describe Experiment, type: :model do
       let(:to_archive) { experiment.my_modules.pluck(:id) }
 
       let(:function_call) do
-        experiment.update_canvas(to_archive, [], [], [], [], [], [], [], user)
+        experiment.update_canvas(to_archive, [], [], {}, [], [], [], {}, user)
       end
 
       it 'calls create activity for archiving tasks' do
@@ -205,7 +252,7 @@ describe Experiment, type: :model do
           .map { |t| { t.id => new_experiment.id } }.reduce({}, :merge)
       end
       let(:function_call) do
-        experiment.update_canvas([], [], [], to_move, [], [], [], [], user)
+        experiment.update_canvas([], [], [], to_move, [], [], [], {}, user)
       end
 
       it 'calls create activity for moving tasks to another experiment' do
@@ -221,5 +268,21 @@ describe Experiment, type: :model do
         expect { function_call }.to change { Activity.all.count }.by(3)
       end
     end
+
+    context 'when moving new task (not saved to DB yet)' do
+      let(:first_task) { experiment.my_modules.first }
+      let(:second_experiment) { create :experiment, project: experiment.project }
+      let(:to_add) { [{ id: 'n0', name: 'new task name', x: 0, y: 0 }] }
+      let(:to_move) { Hash['n0', second_experiment.id] }
+      let(:function_call) { experiment.update_canvas([], to_add, [], to_move, [], [], [], {}, user) }
+
+      it 'returns true' do
+        expect(function_call).to be_truthy
+      end
+
+      it 'assigns task to new experiment' do
+        expect { function_call }.to change { second_experiment.my_modules.count }.by(1)
+      end
+    end
   end
 end