Fix/missing sudo users on create (#374)

* Fix the bug

* Refactoring

* Template selection now also works with keyboard input

* Don't show sudo users in the user field

* Fix error when creating a VM without correct git setup
includes refactorings

* Fix test

* Make CodeFactor happy

app/assets/javascripts/request.js

@@ -1,4 +1,4 @@
-$('#request_template_id').click(function (){
+$('#request_template_id').change(function (){
     cpus = document.getElementById('cpu');
     ram = document.getElementById('ram');
     storage = document.getElementById('storage');

app/controllers/requests_controller.rb

@@ -47,13 +47,8 @@ def create
     @request = Request.new(request_params.merge(user: current_user))
     @request.assign_sudo_users(request_params[:sudo_user_ids])
     respond_to do |format|
-      if enough_resources
-        if @request.save
-          @request.assign_sudo_users(request_params[:sudo_user_ids][1..-1])
-          successful_save(format)
-        else
-          unsuccessful_action(format, :new)
-        end
+      if enough_resources && @request.save
+        successful_save(format)
       else
         unsuccessful_action(format, :new)
       end
@@ -66,12 +61,10 @@ def update
     respond_to do |format|
       prepare_params
       @request.assign_sudo_users(request_params[:sudo_user_ids])
+      @request.accept!
       if @request.update(request_params)
-        redirect_params = @request.accept!
-        redirect_params = redirect_params.nil? ? {} : redirect_params
-        @request.save!
         notify_request_update
-        safe_create_vm_for format, @request, redirect_params
+        safe_create_vm_for format, @request
       else
         unsuccessful_action format, :edit
       end
@@ -110,18 +103,14 @@ def push_to_git
 
   private
 
-  def add_notices(redirect_params)
-    redirect_params[:notice] = I18n.t('request.successfully_updated_and_vm_created') unless redirect_params[:alert]
-    redirect_params
-  end
-
-  def safe_create_vm_for(format, request, redirect_params)
-    vm = request.create_vm
+  def safe_create_vm_for(format, request)
+    vm, warning = request.create_vm
     if vm
-      format.html { redirect_to({ controller: :vms, action: 'edit_config', id: vm.name }, { method: :get }.merge(add_notices(redirect_params))) }
+      notices = warning.nil? ? { notice: 'VM was successfully created!' } : { alert: warning }
+      format.html { redirect_to({ controller: :vms, action: 'edit_config', id: vm.name }, { method: :get }.merge(notices)) }
       format.json { render status: :ok }
     else
-      format.html { redirect_to requests_path, alert: 'VM could not be created, please create it manually in vSphere!' }
+      format.html { redirect_to requests_path, alert: 'VM could not be created, there are no hosts available in vSphere!' }
     end
   rescue RbVmomi::Fault => fault
     format.html { redirect_to requests_path, alert: "VM could not be created, error: \"#{fault.message}\"" }

app/models/request.rb

@@ -42,7 +42,6 @@ def description_text(host_name)
 
   def accept!
     self.status = 'accepted'
-    push_to_git
   end
 
   def reject!
@@ -75,9 +74,16 @@ def non_sudo_user_assignments
 
   def create_vm
     clusters = VSphere::Cluster.all
-    return nil unless clusters.first
-
-    create_vm_in_cluster(clusters.first)
+    return nil, nil unless clusters.first
+
+    warning = nil
+    begin
+      push_to_git
+    rescue Git::GitExecuteError => e
+      logger.error(e)
+      warning = "Your VM was created, but users could not be associated with the VM! Push to git failed, error: \"#{e.message}\""
+    end
+    [create_vm_in_cluster(clusters.first), warning]
   end
 
   def create_vm_in_cluster(cluster)
@@ -89,14 +95,13 @@ def create_vm_in_cluster(cluster)
     vm
   end
 
+  # Error handling has been moved into create_vm to provide easier feedback for the user
   def push_to_git
     GitHelper.open_repository(Puppetscript.puppet_script_path) do |git_writer|
       git_writer.write_file(File.join('Node', "node_#{name}.pp"), generate_puppet_node_script)
       git_writer.write_file(File.join('Name', "#{name}.pp"), generate_puppet_name_script)
       git_writer.save(commit_message(git_writer))
     end
-  rescue Git::GitExecuteError => e
-    logger.error(e)
   end
 
   def commit_message(git_writer)

app/views/requests/_form.html.erb

@@ -104,7 +104,7 @@
                           options_from_collection_for_select(User.all,
                                                               :id,
                                                               :email,
-                                                              @request.users.collect(&:id)),
+                                                              (@request.users - @request.sudo_users).collect(&:id)),
                           { },
                           { class: ' selecttwo', multiple: true} %>
         </div>

db/schema.rb

@@ -131,8 +131,12 @@
     t.datetime "last_sign_in_at"
     t.string "current_sign_in_ip"
     t.string "last_sign_in_ip"
+    t.integer "role"
     t.string "provider"
     t.string "uid"
+    t.string "ssh_key"
+    t.string "first_name"
+    t.string "last_name"
     t.integer "user_id"
     t.boolean "email_notifications", default: false
     t.index ["email"], name: "index_users_on_email", unique: true

spec/controllers/requests_controller_spec.rb

@@ -143,6 +143,11 @@
         post :create, params: { request: valid_attributes }
         expect(response).to redirect_to(requests_path)
       end
+
+      it 'correctly assigns the sudo users' do
+        post :create, params: { request: valid_attributes }
+        expect(assigns(:request).sudo_users).to match_array([sudo_user, sudo_user2])
+      end
     end
 
     context 'with invalid params' do

spec/models/request_spec.rb

@@ -219,7 +219,7 @@
 
     it 'saves the responsible users in the VM' do
       request.responsible_users = [FactoryBot.create(:admin)]
-      vm = request.create_vm
+      vm, _warning = request.create_vm
       expect(vm.responsible_users).to match_array(request.responsible_users)
     end
   end
@@ -284,21 +284,16 @@ class node_$myvm {
         allow(@git_stub.status).to receive(:added).and_return(['added_file'])
       end
 
-      it 'pushes to git when a request is accepted' do
+      it 'pushes to git when a vm is created' do
         expect(@git_stub.git).to receive(:commit_all)
         expect(@git_stub.git).to receive(:push)
-        request.accept!
+        request.create_vm
       end
 
       it 'correctly calls git' do
         request.push_to_git
         expect(@git_stub.git).to have_received(:commit_all).with('Add myvm')
       end
-
-      it 'returns an empty error message' do
-        skip('Why would it show this behavior?')
-        expect(request.push_to_git).to eq({})
-      end
     end
 
     context 'with a changed puppet script' do
@@ -311,11 +306,6 @@ class node_$myvm {
         expect(@git_stub.git).to receive(:commit_all).with('Update myvm')
         request.push_to_git
       end
-
-      it 'returns an empty error message' do
-        skip('Why would it show this behavior?')
-        expect(request.push_to_git).to eq({})
-      end
     end
 
     context 'without any changes' do
@@ -328,11 +318,6 @@ class node_$myvm {
         expect(@git_stub.git).not_to receive(:commit_all)
         request.push_to_git
       end
-
-      it 'returns an emtpy error message' do
-        skip('Why would it show this behavior?')
-        expect(request.push_to_git).to eq({})
-      end
     end
   end
 end