Remove unnecessary preset_scenario_users flag and improve error message handling

Author: louispt1

app/controllers/api/v3/scenario_users_controller.rb

@@ -18,7 +18,7 @@ def index
       def create
         process_scenario_users(create_new: true) do |scenario_user, _|
           unless scenario_user.valid?
-            add_error(scenario_user.email, scenario_user.errors.messages.keys)
+            add_error(scenario_user.email, scenario_user.errors.full_messages)
             next
           end
 
@@ -41,7 +41,7 @@ def update
           scenario_user.update_role(new_role)
 
           unless scenario_user.save
-            add_error(scenario_user.email, scenario_user.errors.messages.keys)
+            add_error(scenario_user.email, scenario_user.errors.full_messages)
             next
           end
 
@@ -55,7 +55,7 @@ def update
       def destroy
         process_scenario_users do |scenario_user, _|
           unless scenario_user.destroy
-            add_error(scenario_user.email, scenario_user.errors.messages.keys)
+            add_error(scenario_user.email, scenario_user.errors.full_messages)
             next
           end
 

app/models/scenario.rb

@@ -363,28 +363,14 @@ def delete_all_users
     scenario_users.delete_all
   end
 
-  def copy_preset_roles(preset_scenario_users = nil)
+  def copy_preset_roles
     return unless parent
 
-    if preset_scenario_users.present?
-      copy_roles_from_user_data(preset_scenario_users)
-    else
-      copy_roles_from_parent
-    end
+    copy_roles_from_parent
   end
 
   private
 
-  def copy_roles_from_user_data(users_data)
-    users_data.each do |user_data|
-      scenario_users.create(
-        user_id: user_data['user_id'] || user_data[:user_id],
-        user_email: user_data['user_email'] || user_data[:user_email],
-        role_id: user_data['role_id'] || user_data[:role_id]
-      )
-    end
-  end
-
   def copy_roles_from_parent
     parent.scenario_users.each do |preset_user|
       if (existing_user = scenario_users.find_by(user: preset_user.user))

app/models/scenario_updater.rb

@@ -75,8 +75,7 @@ def apply(scenario_data, (coupling_state, user_values, balanced_values))
   # Post-save
   def post_save(scenario_data, persisted)
     set_preset_roles = truthy?(scenario_data[:set_preset_roles])
-    preset_scenario_users = scenario_data[:preset_scenario_users]
-    _post_saved = yield post_save_operations(set_preset_roles, preset_scenario_users)
+    _post_saved = yield post_save_operations(set_preset_roles)
     Success(persisted)
   end
 
@@ -149,9 +148,8 @@ def persist_scenario(attributes)
     service(:PersistScenario).call(scenario, attributes, skip_validation)
   end
 
-  def post_save_operations(set_preset_roles, preset_scenario_users)
-    service(:PostSaveOperations).call(scenario, set_preset_roles, preset_scenario_users,
-      current_user)
+  def post_save_operations(set_preset_roles)
+    service(:PostSaveOperations).call(scenario, set_preset_roles, current_user)
   end
 
   # Helper to instantiate services

app/models/scenario_updater/services/post_save_operations.rb

@@ -8,18 +8,18 @@ class PostSaveOperations
 
       TRUTHY_VALUES = Set.new([true, 'true', '1']).freeze
 
-      def call(scenario, set_preset_roles, preset_scenario_users, current_user)
-        copy_preset_roles_if_requested(scenario, set_preset_roles, preset_scenario_users)
+      def call(scenario, set_preset_roles, current_user)
+        copy_preset_roles_if_requested(scenario, set_preset_roles)
         update_version_tag(scenario, current_user)
 
         Success(scenario)
       end
 
       private
 
-      def copy_preset_roles_if_requested(scenario, set_preset_roles, preset_scenario_users)
-        should_copy = TRUTHY_VALUES.include?(set_preset_roles) || preset_scenario_users.present?
-        scenario.copy_preset_roles(preset_scenario_users) if should_copy
+      def copy_preset_roles_if_requested(scenario, set_preset_roles)
+        should_copy = TRUTHY_VALUES.include?(set_preset_roles)
+        scenario.copy_preset_roles if should_copy
       end
 
       def update_version_tag(scenario, current_user)

spec/models/scenario_spec.rb

@@ -868,38 +868,5 @@
     it 'sets the collaborator of the preset' do
       expect { subject }.to(change { scenario.owner?(preset_collaborator) })
     end
-
-    context 'with preset_scenario_users data' do
-      subject do
-        scenario.copy_preset_roles(users_data)
-        scenario.reload
-      end
-
-      let(:user1) { create(:user) }
-      let(:user2) { create(:user) }
-
-      let(:users_data) do
-        [
-          { user_id: user1.id, user_email: nil, role_id: 3 },
-          { user_id: nil, user_email: '[email protected]', role_id: 2 }
-        ]
-      end
-
-      it 'creates scenario users from the provided data' do
-        expect { subject }.to change(scenario.scenario_users, :count).by(2)
-      end
-
-      it 'sets the owner from user_id' do
-        subject
-        expect(scenario).to be_owner(user1)
-      end
-
-      it 'creates pending user with email' do
-        subject
-        pending_user = scenario.scenario_users.find_by(user_email: '[email protected]')
-        expect(pending_user).to be_present
-        expect(pending_user.role_id).to eq(2)
-      end
-    end
   end
 end

spec/models/scenario_updater/services/post_save_operations_spec.rb

@@ -7,20 +7,14 @@
   let(:service) { described_class.new }
 
   it 'copies preset roles if requested' do
-    expect(scenario).to receive(:copy_preset_roles).with(nil)
-    service.call(scenario, true, nil, 'user')
-  end
-
-  it 'copies preset roles with user data' do
-    users_data = [{ user_id: 1, user_email: nil, role_id: 3 }]
-    expect(scenario).to receive(:copy_preset_roles).with(users_data)
-    service.call(scenario, false, users_data, 'user')
+    expect(scenario).to receive(:copy_preset_roles).with(no_args)
+    service.call(scenario, true, 'user')
   end
 
   it 'updates version tag' do
     version_tag = double('VersionTag')
     allow(scenario).to receive(:scenario_version_tag).and_return(version_tag)
     expect(version_tag).to receive(:update).with(user: 'user')
-    service.call(scenario, false, nil, 'user')
+    service.call(scenario, false, 'user')
   end
 end

spec/requests/api/v3/scenario_users_controller_spec.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'spec_helper'
 
 RSpec.describe 'Api::V3::ScenarioUsers', type: :request, api: true do
@@ -100,7 +102,7 @@
       end
 
       it 'returns an error' do
-        expect(json['errors']['[email protected]']).to include('role_id')
+        expect(json['errors']['[email protected]']).to include('Role unknown')
       end
     end
 
@@ -118,7 +120,7 @@
       end
 
       it 'returns an error' do
-        expect(json['errors']['viewer']).to include('user_email')
+        expect(json['errors']['viewer']).to include('User email is invalid')
       end
     end
 
@@ -222,7 +224,7 @@
 
       it 'returns an error' do
         expect(json['errors']['']).to include(
-          'base'
+          'Last owner cannot be altered'
         )
       end
     end
@@ -289,7 +291,7 @@
       end
 
       it 'returns an error' do
-        expect(json['errors']['']).to include('role_id')
+        expect(json['errors']['']).to include('Role unknown')
       end
     end
   end
@@ -362,7 +364,7 @@
 
       it 'returns an error' do
         expect(json['errors']['']).to include(
-          'base'
+          'Last owner cannot be altered'
         )
       end
     end