From 4cf12d12d13f1a70c6858ec91a831473178e4be5 Mon Sep 17 00:00:00 2001
From: Urban Rotnik <urban.rotnik@gmail.com>
Date: Tue, 15 Jan 2019 13:45:05 +0100
Subject: [PATCH] Refactor 'move_experiment' method into service object

Jira Ticket: SCI-2926
---
 .rubocop.yml                                  |  3 +
 app/controllers/experiments_controller.rb     | 56 ++++-------
 app/models/experiment.rb                      | 18 ----
 app/models/tag.rb                             |  7 +-
 .../experiments/move_to_project_service.rb    | 89 +++++++++++++++++
 spec/controllers/projects_controller_spec.rb  |  6 +-
 spec/factories/experiments.rb                 |  2 +-
 spec/factories/my_modules.rb                  |  3 +
 spec/factories/tags.rb                        |  9 ++
 spec/factories/teams.rb                       |  3 +
 spec/factories/user_project.rb                |  5 -
 spec/factories/user_projects.rb               | 21 ++++
 spec/models/experiment_spec.rb                |  2 +
 spec/models/tag_spec.rb                       | 49 ++++++++--
 spec/models/user_spec.rb                      |  4 +-
 spec/requests/api/v1/users_controller_spec.rb |  8 +-
 .../move_to_project_service_spec.rb           | 98 +++++++++++++++++++
 17 files changed, 306 insertions(+), 77 deletions(-)
 create mode 100644 app/services/experiments/move_to_project_service.rb
 create mode 100644 spec/factories/tags.rb
 delete mode 100644 spec/factories/user_project.rb
 create mode 100644 spec/factories/user_projects.rb
 create mode 100644 spec/services/experiments/move_to_project_service_spec.rb

diff --git a/.rubocop.yml b/.rubocop.yml
index 543b1c577..538599c28 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -350,6 +350,9 @@ Style/WhenThen:
 Metrics/AbcSize:
   Enabled: false
 
+Metrics/BlockLength:
+  ExcludedMethods: ['describe', 'context']
+
 Metrics/ClassLength:
   Enabled: false
 
diff --git a/app/controllers/experiments_controller.rb b/app/controllers/experiments_controller.rb
index 7c5a51381..1259c893f 100644
--- a/app/controllers/experiments_controller.rb
+++ b/app/controllers/experiments_controller.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class ExperimentsController < ApplicationController
   include SampleActions
   include TeamsHelper
@@ -237,49 +239,23 @@ class ExperimentsController < ApplicationController
 
   # POST: move_experiment(id)
   def move
-    project = Project.find_by_id(params[:experiment].try(:[], :project_id))
-    old_project = @experiment.project
-
-    # Try to move the experiment
-    success = true
-    if @experiment.moveable_projects(current_user).include?(project)
-      success = @experiment.move_to_project(project)
-    else
-      success = false
-    end
-
-    if success
-      Activity.create(
-        type_of: :move_experiment,
-        project: project,
-        experiment: @experiment,
-        user: current_user,
-        message: I18n.t(
-          'activities.move_experiment',
-          user: current_user.full_name,
-          experiment: @experiment.name,
-          project_new: project.name,
-          project_original: old_project.name
-        )
-      )
+    service = Experiments::MoveToProjectService
+              .call(experiment_id: @experiment.id,
+                    project_id: move_experiment_param,
+                    user_id: current_user.id)
 
+    if service.succeed?
       flash[:success] = t('experiments.move.success_flash',
                           experiment: @experiment.name)
-      respond_to do |format|
-        format.json do
-          render json: { path: canvas_experiment_url(@experiment) }, status: :ok
-        end
-      end
+      path = canvas_experiment_url(@experiment)
+      status = :ok
     else
-      respond_to do |format|
-        format.json do
-          render json: { message: t('experiments.move.error_flash',
-                                    experiment:
-                                      escape_input(@experiment.name)) },
-                                    status: :unprocessable_entity
-        end
-      end
+      message = t('experiments.move.error_flash',
+                  experiment: escape_input(@experiment.name))
+      status = :unprocessable_entity
     end
+
+    render json: { message: message, path: path }, status: status
   end
 
   def module_archive
@@ -348,6 +324,10 @@ class ExperimentsController < ApplicationController
     params.require(:experiment).permit(:name, :description, :archived)
   end
 
+  def move_experiment_param
+    params.require(:experiment).require(:project_id)
+  end
+
   def load_projects_tree
     # Switch to correct team
     current_team_switch(@experiment.project.team) unless @experiment.project.nil?
diff --git a/app/models/experiment.rb b/app/models/experiment.rb
index 851372781..ee99a7bb5 100644
--- a/app/models/experiment.rb
+++ b/app/models/experiment.rb
@@ -255,24 +255,6 @@ class Experiment < ApplicationRecord
     clone
   end
 
-  def move_to_project(project)
-    self.project = project
-
-    my_modules.each do |m|
-      new_tags = []
-      m.tags.each do |t|
-        new_tags << t.deep_clone_to_project(project)
-      end
-      m.my_module_tags.destroy_all
-
-      project.tags << new_tags
-      m.tags << new_tags
-    end
-    result = save
-    touch(:workflowimg_updated_at) if result
-    result
-  end
-
   # Get projects where user is either owner or user in the same team
   # as this experiment
   def projects_with_role_above_user(current_user)
diff --git a/app/models/tag.rb b/app/models/tag.rb
index a1b1089ee..4818622de 100644
--- a/app/models/tag.rb
+++ b/app/models/tag.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class Tag < ApplicationRecord
   include SearchableModel
 
@@ -48,7 +50,10 @@ class Tag < ApplicationRecord
     end
   end
 
-  def deep_clone_to_project(project)
+  def clone_to_project_or_return_existing(project)
+    tag = Tag.find_by(project: project, name: name, color: color)
+    return tag if tag
+
     Tag.create(
       name: name,
       color: color,
diff --git a/app/services/experiments/move_to_project_service.rb b/app/services/experiments/move_to_project_service.rb
new file mode 100644
index 000000000..088bbc7b0
--- /dev/null
+++ b/app/services/experiments/move_to_project_service.rb
@@ -0,0 +1,89 @@
+# frozen_string_literal: true
+
+module Experiments
+  class MoveToProjectService
+    extend Service
+
+    attr_reader :errors
+
+    def initialize(experiment_id:, project_id:, user_id:)
+      @exp = Experiment.find experiment_id
+      @project = Project.find project_id
+      @user = User.find user_id
+      @original_project = @exp&.project
+      @errors = {}
+    end
+
+    def call
+      return self unless valid?
+
+      ActiveRecord::Base.transaction do
+        @exp.project = @project
+
+        @exp.my_modules.each do |m|
+          new_tags = m.tags.map do |t|
+            t.clone_to_project_or_return_existing(@project)
+          end
+          m.my_module_tags.delete_all
+          m.tags = new_tags
+        end
+
+        raise ActiveRecord::Rollback unless @exp.save
+      end
+
+      @errors.merge!(@exp.errors.to_hash) unless @exp.valid?
+
+      track_activity if succeed?
+      self
+    end
+
+    def succeed?
+      @errors.none?
+    end
+
+    private
+
+    def valid?
+      unless @exp && @project && @user
+        @errors[:invalid_arguments] =
+          { 'experiment': @exp,
+            'project': @project,
+            'user': @user }
+          .map do |key, value|
+            "Can't find #{key.capitalize}" if value.nil?
+          end.compact
+        return false
+      end
+
+      e = Experiment.find_by(name: @exp.name, project: @project)
+
+      if e
+        @errors[:project_with_exp] =
+          ['Project already contains experiment with this name']
+        false
+      elsif !@exp.moveable_projects(@user).include?(@project)
+        @errors[:target_project_not_valid] =
+          ['Experiment cannot be moved to this project']
+        false
+      else
+        true
+      end
+    end
+
+    def track_activity
+      Activity.create(
+        type_of: :move_experiment,
+        project: @project,
+        experiment: @exp,
+        user: @user,
+        message: I18n.t(
+          'activities.move_experiment',
+          user: @user,
+          experiment: @exp.name,
+          project_new: @project.name,
+          project_original: @original_project.name
+        )
+      )
+    end
+  end
+end
diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb
index 511fce723..1aa7f6690 100644
--- a/spec/controllers/projects_controller_spec.rb
+++ b/spec/controllers/projects_controller_spec.rb
@@ -54,11 +54,15 @@ describe ProjectsController, type: :controller do
     end
   end
 
+  # rubocop:disable Security/Eval
+  # rubocop:disable Style/EvalWithLocation
   (1..PROJECTS_CNT).each do |i|
     let!("user_projects_#{i}") do
-      create :user_project, project: eval("project_#{i}"), user: user
+      create :user_project, :owner, project: eval("project_#{i}"), user: user
     end
   end
+  # rubocop:enable Security/Eval
+  # rubocop:enable Style/EvalWithLocation
 
   describe '#index' do
     context 'in JSON format' do
diff --git a/spec/factories/experiments.rb b/spec/factories/experiments.rb
index 610c12b20..8743772e8 100644
--- a/spec/factories/experiments.rb
+++ b/spec/factories/experiments.rb
@@ -12,7 +12,7 @@ FactoryBot.define do
     project { create :project, created_by: user }
     factory :experiment_with_tasks do
       after(:create) do |e|
-        create_list :my_module, 3, experiment: e
+        create_list :my_module, 3, :with_tag, experiment: e
       end
     end
   end
diff --git a/spec/factories/my_modules.rb b/spec/factories/my_modules.rb
index 3101a427d..c383d794f 100644
--- a/spec/factories/my_modules.rb
+++ b/spec/factories/my_modules.rb
@@ -8,5 +8,8 @@ FactoryBot.define do
     workflow_order { MyModule.where(experiment_id: experiment.id).count + 1 }
     experiment
     my_module_group { create :my_module_group, experiment: experiment }
+    trait :with_tag do
+      tags { create_list :tag, 3, project: experiment.project }
+    end
   end
 end
diff --git a/spec/factories/tags.rb b/spec/factories/tags.rb
new file mode 100644
index 000000000..5e5a09f41
--- /dev/null
+++ b/spec/factories/tags.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :tag do
+    sequence(:name) { |n| "My tag-#{n}" }
+    project
+    color { Faker::Color.hex_color }
+  end
+end
diff --git a/spec/factories/teams.rb b/spec/factories/teams.rb
index 130f67fb7..a94666a66 100644
--- a/spec/factories/teams.rb
+++ b/spec/factories/teams.rb
@@ -6,5 +6,8 @@ FactoryBot.define do
     sequence(:name) { |n| "My team-#{n}" }
     description { Faker::Lorem.sentence }
     space_taken { 1048576 }
+    trait :with_members do
+      users { create_list :user, 3 }
+    end
   end
 end
diff --git a/spec/factories/user_project.rb b/spec/factories/user_project.rb
deleted file mode 100644
index 18b5d163c..000000000
--- a/spec/factories/user_project.rb
+++ /dev/null
@@ -1,5 +0,0 @@
-FactoryBot.define do
-  factory :user_project do
-    role 'owner'
-  end
-end
diff --git a/spec/factories/user_projects.rb b/spec/factories/user_projects.rb
new file mode 100644
index 000000000..6ab1aa8e2
--- /dev/null
+++ b/spec/factories/user_projects.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :user_project do
+    user
+    project
+    assigned_by { user }
+    trait :owner do
+      role { UserProject.roles[:owner] }
+    end
+    trait :normal_user do
+      role { UserProject.roles[:normal_user] }
+    end
+    trait :technician do
+      role { UserProject.roles[:technician] }
+    end
+    trait :viewer do
+      role { UserProject.roles[:viewer] }
+    end
+  end
+end
diff --git a/spec/models/experiment_spec.rb b/spec/models/experiment_spec.rb
index d30877fa4..1f5bcbe14 100644
--- a/spec/models/experiment_spec.rb
+++ b/spec/models/experiment_spec.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'rails_helper'
 
 describe Experiment, type: :model do
diff --git a/spec/models/tag_spec.rb b/spec/models/tag_spec.rb
index e95c7abef..ded47e60c 100644
--- a/spec/models/tag_spec.rb
+++ b/spec/models/tag_spec.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'rails_helper'
 
 describe Tag, type: :model do
@@ -23,15 +25,46 @@ describe Tag, type: :model do
     it { should have_many :my_modules }
   end
 
-  describe 'Should be a valid object' do
-    it { should validate_presence_of :name }
-    it { should validate_presence_of :color }
-    it { should validate_presence_of :project }
-    it do
-      should validate_length_of(:name).is_at_most(Constants::NAME_MAX_LENGTH)
+  describe 'Validations' do
+    describe '#name' do
+      it { should validate_presence_of :name }
+      it do
+        should validate_length_of(:name)
+          .is_at_most(Constants::NAME_MAX_LENGTH)
+      end
     end
-    it do
-      should validate_length_of(:color).is_at_most(Constants::COLOR_MAX_LENGTH)
+
+    describe '#color' do
+      it { should validate_presence_of :color }
+      it do
+        should validate_length_of(:color)
+          .is_at_most(Constants::COLOR_MAX_LENGTH)
+      end
+    end
+    describe '#projects' do
+      it { should validate_presence_of :project }
+    end
+  end
+  describe '.clone_to_project_or_return_existing' do
+    let(:project) { create :project }
+    let(:tag) { create :tag }
+
+    context 'when tag does not exits' do
+      it 'does create new tag for project' do
+        expect do
+          tag.clone_to_project_or_return_existing(project)
+        end.to(change { Tag.where(project_id: project.id).count })
+      end
+    end
+
+    context 'when tag already exists' do
+      it 'return existing tag for project' do
+        Tag.create(name: tag.name, color: tag.color, project: project)
+
+        expect do
+          tag.clone_to_project_or_return_existing(project)
+        end.to_not(change { Tag.where(project_id: project.id).count })
+      end
     end
   end
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 15ab3be5e..a41f965b3 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -187,7 +187,9 @@ describe User, type: :model do
   describe '#last_activities' do
     let!(:user) { create :user }
     let!(:project) { create :project }
-    let!(:user_projects) { create :user_project, project: project, user: user }
+    let!(:user_projects) do
+      create :user_project, :viewer, project: project, user: user
+    end
     let!(:activity_one) { create :activity, user: user, project: project }
     let!(:activity_two) { create :activity, user: user, project: project }
 
diff --git a/spec/requests/api/v1/users_controller_spec.rb b/spec/requests/api/v1/users_controller_spec.rb
index ba705391d..508b471ec 100644
--- a/spec/requests/api/v1/users_controller_spec.rb
+++ b/spec/requests/api/v1/users_controller_spec.rb
@@ -4,9 +4,9 @@ require 'rails_helper'
 
 RSpec.describe 'Api::V1::UsersController', type: :request do
   before :all do
-    @user1 = create(:user, email: Faker::Internet.unique.email)
-    @user2 = create(:user, email: Faker::Internet.unique.email)
-    @user3 = create(:user, email: Faker::Internet.unique.email)
+    @user1 = create(:user)
+    @user2 = create(:user)
+    @user3 = create(:user)
     @team1 = create(:team, created_by: @user1)
     @team2 = create(:team, created_by: @user2)
     @team3 = create(:team, created_by: @user3)
@@ -40,7 +40,7 @@ RSpec.describe 'Api::V1::UsersController', type: :request do
 
     it 'When invalid request, non existing user' do
       hash_body = nil
-      get api_v1_user_path(id: 123), headers: @valid_headers
+      get api_v1_user_path(id: -1), headers: @valid_headers
       expect(response).to have_http_status(403)
       expect { hash_body = json }.not_to raise_exception
       expect(hash_body['errors'][0]).to include('status': 403)
diff --git a/spec/services/experiments/move_to_project_service_spec.rb b/spec/services/experiments/move_to_project_service_spec.rb
new file mode 100644
index 000000000..67d8e33a9
--- /dev/null
+++ b/spec/services/experiments/move_to_project_service_spec.rb
@@ -0,0 +1,98 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe Experiments::MoveToProjectService do
+  let(:team) { create :team, :with_members }
+  let(:project) do
+    create :project, team: team, user_projects: []
+  end
+  let(:new_project) do
+    create :project, team: team, user_projects: [user_project2]
+  end
+  let(:experiment) do
+    create :experiment_with_tasks, name: 'MyExp', project: project
+  end
+  let(:user) { create :user }
+  let(:user_project2) { create :user_project, :normal_user, user: user }
+  let(:service_call) do
+    Experiments::MoveToProjectService.call(experiment_id: experiment.id,
+                                    project_id: new_project.id,
+                                    user_id: user.id)
+  end
+
+  context 'when call with valid params' do
+    it 'unnasigns experiment from project' do
+      service_call
+
+      expect(project.experiments.pluck(:id)).to_not include(experiment.id)
+    end
+
+    it 'assigns experiment to new_project' do
+      service_call
+
+      expect(new_project.experiments.pluck(:id)).to include(experiment.id)
+    end
+
+    it 'copies tags to new project' do
+      expect { service_call }.to(change { new_project.tags.count })
+    end
+
+    it 'leaves tags on an old project' do
+      experiment # explicit call to create tags
+      expect { service_call }.not_to(change { project.tags.count })
+    end
+
+    it 'sets new project tags to modules' do
+      service_call
+      new_tags = experiment.my_modules.map { |m| m.tags.map { |t| t } }.flatten
+      tags_project_id = new_tags.map(&:project_id).uniq.first
+
+      expect(tags_project_id).to be == new_project.id
+    end
+
+    it 'adds Activity record' do
+      expect { service_call }.to(change { Activity.all.count })
+    end
+  end
+
+  context 'when call with invalid params' do
+    it 'returns an error when can\'t find user and project' do
+      allow(Project).to receive(:find).and_return(nil)
+      allow(User).to receive(:find).and_return(nil)
+
+      expect(service_call.errors).to have_key(:invalid_arguments)
+    end
+
+    it 'returns an error on validate' do
+      FactoryBot.create :experiment, project: new_project, name: 'MyExp'
+      expect(service_call.succeed?).to be_falsey
+    end
+
+    it 'returns an error on saving' do
+      expect_any_instance_of(Experiments::MoveToProjectService)
+        .to receive(:valid?)
+        .and_return(true)
+      FactoryBot.create :experiment, project: new_project, name: 'MyExp'
+
+      expect(service_call.succeed?).to be_falsey
+    end
+
+    it 'rollbacks cloned tags after unsucessful save' do
+      expect_any_instance_of(Experiments::MoveToProjectService)
+        .to receive(:valid?)
+        .and_return(true)
+      FactoryBot.create :experiment, project: new_project, name: 'MyExp'
+      experiment
+
+      expect { service_call }.not_to(change { Tag.count })
+    end
+
+    it 'returns error if teams is not the same' do
+      t = create :team, :with_members
+      project.update(team: t)
+
+      expect(service_call.errors).to have_key(:target_project_not_valid)
+    end
+  end
+end