326/scaffold/sudo users not stored

app/controllers/requests_controller.rb

@@ -43,7 +43,7 @@ def notify_users(title, message)
   def create
     prepare_params
     @request = Request.new(request_params.merge(user: current_user))
-    @request.assign_sudo_users(request_params[:sudo_user_ids][1..-1])
+    @request.assign_sudo_users(request_params[:sudo_user_ids])
 
     respond_to do |format|
       if @request.save
@@ -59,17 +59,11 @@ def create
   def update
     respond_to do |format|
       prepare_params
+      @request.assign_sudo_users(request_params[:sudo_user_ids])
+      @request.accept!
       if @request.update(request_params)
-        @request.accept!
-        @request.save
         notify_request_update
-        vm = @request.create_vm
-        if vm
-          format.html { redirect_to({ controller: :vms, action: 'edit_config', id: vm.name }, method: :get, notice: I18n.t('request.successfully_updated_and_vm_created')) }
-          format.json { render status: :ok }
-        else
-          format.html { redirect_to requests_path, alert: 'VM could not be created, please create it manually in vSphere!' }
-        end
+        safe_create_vm_for format, @request
       else
         unsuccessful_action format, :edit
       end
@@ -79,9 +73,8 @@ def update
   def reject
     @request = Request.find params[:id]
     respond_to do |format|
+      @request.reject!
       if @request&.update(rejection_params)
-        @request.reject!
-        @request.save
         notify_request_update
         format.html { redirect_to requests_path, notice: "Request '#{@request.name}' rejected!" }
         format.json { render status: :ok, action: :index }
@@ -109,6 +102,18 @@ def push_to_git
 
   private
 
+  def safe_create_vm_for(format, request)
+    vm = request.create_vm
+    if vm
+      format.html { redirect_to({ controller: :vms, action: 'edit_config', id: vm.name }, method: :get, notice: I18n.t('request.successfully_updated_and_vm_created')) }
+      format.json { render status: :ok }
+    else
+      format.html { redirect_to requests_path, alert: 'VM could not be created, please create it manually in vSphere!' }
+    end
+  rescue RbVmomi::Fault => fault
+    format.html { redirect_to requests_path, alert: "VM could not be created, error: \"#{fault.message}\"" }
+  end
+
   def host_url
     request.base_url
   end
@@ -149,12 +154,15 @@ def unsuccessful_action(format, method)
   end
 
   # Storage and RAM are displayed in GB but internally stored in MB.
+  # sudo_user_ids always contain one empty element which must be removed
   def prepare_params
-    return unless params[:request]
+    request_parameters = params[:request]
+    return unless request_parameters
 
-    params[:request][:name] = replace_whitespaces(params[:request][:name]) if params[:request][:name]
-    params[:request][:ram_mb] = gb_to_mb(params[:request][:ram_mb].to_i) if params[:request][:ram_mb]
-    params[:request][:storage_mb] = gb_to_mb(params[:request][:storage_mb].to_i) if params[:request][:storage_mb]
+    request_parameters[:name] &&= replace_whitespaces(request_parameters[:name])
+    request_parameters[:ram_mb] &&= gb_to_mb(request_parameters[:ram_mb].to_i)
+    request_parameters[:storage_mb] &&= gb_to_mb(request_parameters[:storage_mb].to_i)
+    request_parameters[:sudo_user_ids] &&= request_parameters[:sudo_user_ids][1..-1]
   end
 
   # Never trust parameters from the scary internet, only allow the white list through.

app/models/request.rb

@@ -5,6 +5,10 @@ class Request < ApplicationRecord
   has_many :users, through: :users_assigned_to_requests
   belongs_to :user
 
+  before_save do
+    users_assigned_to_requests.each(&:save)
+  end
+
   attr_accessor :sudo_user_ids
 
   MAX_NAME_LENGTH = 20
@@ -39,12 +43,13 @@ def reject!
   end
 
   def assign_sudo_users(sudo_user_ids)
+    assign_attributes(users_assigned_to_requests: users_assigned_to_requests - sudo_user_assignments)
     sudo_user_ids&.each do |id|
       assignment = users_assigned_to_requests.find { |an_assignment| an_assignment.user_id == id.to_i }
-      if !assignment.nil?
-        assignment.update_attribute(:sudo, true)
-      else
+      if assignment.nil?
         users_assigned_to_requests.new(sudo: true, user_id: id)
+      else
+        assignment.assign_attributes(sudo: true)
       end
     end
   end

app/views/requests/_form.html.erb

@@ -38,7 +38,7 @@
                                                      :email,
                                                      @request.sudo_user_assignments.collect(&:user_id)),
                     {},
-                    { multiple: true, class: "selecttwo", style: 'width: 50%' } %>
+                    { multiple: true, class: "selecttwo", style: 'width: 100%' } %>
   </div>
 
   <div class="field form-group">
@@ -49,12 +49,12 @@
                                                        :email,
                                                        @request.non_sudo_user_assignments.collect(&:user_id)),
                     {},
-                    { multiple: true, class: "selecttwo", style: 'width: 50%' } %>
+                    { multiple: true, class: "selecttwo", style: 'width: 100%' } %>
   </div>
 
   <div class="field form-group">
     <%= form.label :operating_system, "Operating System" %> <br>
-    <%= form.select :operating_system, operating_system_options, {}, id: 'operating_system' %>
+    <%= form.select :operating_system, operating_system_options, {}, id: 'operating_system', style: 'width: 100%' %>
   </div>
 
     <div class="field form-group">

db/schema.rb

@@ -67,6 +67,12 @@
     t.index ["user_id"], name: "index_requests_on_user_id"
   end
 
+  create_table "requests_responsible_users", id: false, force: :cascade do |t|
+    t.integer "request_id", null: false
+    t.integer "user_id", null: false
+    t.index ["request_id", "user_id"], name: "index_requests_responsible_users_on_request_id_and_user_id"
+  end
+
   create_table "responsible_users", force: :cascade do |t|
     t.integer "user_id", null: false
     t.integer "project_id", null: false
@@ -129,7 +135,7 @@
     t.string "first_name"
     t.string "last_name"
     t.integer "user_id"
-    t.boolean "email_notifications", default: true
+    t.boolean "email_notifications", default: false
     t.index ["email"], name: "index_users_on_email", unique: true
     t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true
   end
@@ -140,6 +146,8 @@
     t.integer "user_id"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.index ["request_id"], name: "index_users_assigned_to_requests_on_request_id"
+    t.index ["user_id"], name: "index_users_assigned_to_requests_on_user_id"
   end
 
   create_table "virtual_machine_configs", force: :cascade do |t|

spec/controllers/requests_controller_spec.rb

@@ -27,8 +27,8 @@
 
 RSpec.describe RequestsController, type: :controller do
   let(:user) { FactoryBot.create :employee }
-
   let(:sudo_user) { FactoryBot.create :user }
+  let(:sudo_user2) { FactoryBot.create :admin }
 
   # This should return the minimal set of attributes required to create a valid
   # Request. As you add validations to Request, be sure to
@@ -44,7 +44,7 @@
       comment: 'Comment',
       status: 'pending',
       user: user,
-      sudo_user_ids: ['']
+      sudo_user_ids: ['', sudo_user.id.to_s, sudo_user2.id.to_s]
     }
   end
 
@@ -133,9 +133,7 @@
 
       # regression test for #320
       it 'does not persist the sudo users' do
-        assigns(:request).users_assigned_to_requests.each do |each|
-          expect(each).not_to be_persisted
-        end
+        expect(assigns(:request).users_assigned_to_requests).to all(be_changed)
       end
     end
   end
@@ -182,16 +180,20 @@
           operating_system: 'MyNewOS',
           comment: 'newComment',
           status: 'pending',
-          user: user
+          user: user,
+          sudo_user_ids: ['', sudo_user.id.to_s]
         }
       end
 
       # this variable may not be called request, because it would then override an internal RSpec variable
       let(:the_request) do
-        Request.create! valid_attributes
+        request = Request.create! valid_attributes
+        request.assign_sudo_users valid_attributes[:sudo_user_ids][1..-1]
+        request.save!
+        request
       end
 
-      it 'updates the requested request' do
+      it 'updates the request' do
         patch :update, params: { id: the_request.to_param, request: new_attributes }
         the_request.reload
         expect(the_request.name).to eq('mynewvm')
@@ -207,6 +209,12 @@
         the_request.reload
         expect(the_request).to be_accepted
       end
+
+      it 'correctly updates the sudo users' do
+        patch :update, params: { id: the_request.to_param, request: new_attributes }
+        the_request.reload
+        expect(the_request.sudo_users).to match_array([sudo_user])
+      end
     end
 
     context 'with invalid params' do

spec/features/dashboard/dashboard_spec.rb

@@ -9,9 +9,9 @@
   end
 
   it 'does render a list of all available vms for a signed in user' do
+    skip('user is not yet connected to his vms')
     visit(:dashboard)
     expect(page).to(have_text('VMs'))
-    skip('user is not yet connected to his vms')
   end
 
   it 'does render an empty list if a user does not have access to a vm' do

spec/models/request_spec.rb

@@ -93,76 +93,122 @@
   end
 
   describe 'method tests' do
-    before do
-      @request = FactoryBot.create :request
-      @user = FactoryBot.build :user
-    end
+    let(:request) { FactoryBot.create :request }
+    let(:user) { FactoryBot.create :user }
 
     context 'when accepting a request' do
       it 'changes status to accepted' do
-        @request.accept!
-        expect(@request.status).to eq('accepted')
+        request.accept!
+        expect(request.status).to eq('accepted')
       end
     end
 
-    context 'given a request and an user' do
+    describe 'assign_sudo_users' do
       it 'creates sudo user assignments correctly' do
-        @request.assign_sudo_users([@user.id])
-        expect((@request.sudo_user_assignments.select { |assignment| assignment.user_id == @user.id }).size).to eq(1)
+        request.assign_sudo_users([user.id])
+        expect((request.sudo_user_assignments.select { |assignment| assignment.user_id == user.id }).size).to eq(1)
+      end
+
+      it 'does not persist new assignments' do
+        request.assign_sudo_users([user.id])
+        expect(request.sudo_user_assignments).to all(be_changed)
+      end
+
+      it 'persists new assignments when saved' do
+        request.assign_sudo_users [user.id]
+        request.save!
+        request.sudo_user_assignments.each do |each|
+          expect(each).not_to be_changed
+        end
+      end
+
+      context 'with pre-existing sudo assignmnets' do
+        let(:user2) { FactoryBot.create :user }
+
+        before do
+          request.assign_sudo_users([user.id, user2.id])
+          request.save!
+        end
+
+        it 'can remove assignments' do
+          request.assign_sudo_users([user.id])
+          expect(request.sudo_users).to match_array([user])
+        end
+      end
+
+      context 'if a user should be upgraded' do
+        before do
+          request.update! users: [user]
+          request.assign_sudo_users [user.id]
+        end
+
+        it 'can upgrade users' do
+          expect(request.sudo_users).to match_array([user])
+        end
+
+        it 'does not save upgraded user assignments' do
+          expect(request.sudo_user_assignments).to all(be_changed)
+        end
+
+        it 'saves upgraded user assignments when saved' do
+          request.save!
+          request.sudo_user_assignments.each do |each|
+            expect(each).not_to be_changed
+          end
+        end
       end
     end
 
     context 'when having a non sudo user' do
       before do
-        @assignment = @request.users_assigned_to_requests.build(user_id: @user.id, sudo: false)
+        @assignment = request.users_assigned_to_requests.build(user_id: user.id, sudo: false)
       end
 
       it 'does not return a sudo user' do
-        expect(@request.sudo_user_assignments.include?(@assignment)).to be false
+        expect(request.sudo_user_assignments.include?(@assignment)).to be false
       end
 
       it 'does return a non sudo user' do
-        expect(@request.non_sudo_user_assignments.include?(@assignment)).to be true
+        expect(request.non_sudo_user_assignments.include?(@assignment)).to be true
       end
     end
 
     context 'when having a sudo user' do
       before do
-        @sudo_assignment = @request.users_assigned_to_requests.build(user_id: @user.id, sudo: true)
+        @sudo_assignment = request.users_assigned_to_requests.build(user_id: user.id, sudo: true)
       end
 
       it 'does return a sudo user' do
-        expect(@request.sudo_user_assignments.include?(@sudo_assignment)).to be true
+        expect(request.sudo_user_assignments.include?(@sudo_assignment)).to be true
       end
 
       it 'does not return a non sudo user' do
-        expect(@request.non_sudo_user_assignments.include?(@sudo_assignment)).to be false
+        expect(request.non_sudo_user_assignments.include?(@sudo_assignment)).to be false
       end
     end
 
     context 'when having a user being assigned as user and sudo user' do
+      let(:request) { FactoryBot.create(:request, name: 'MyVM1', user_ids: [user.id]) }
+
       before do
-        @request = FactoryBot.create(:request, name: 'MyVM1', user_ids: [@user.id])
-        @request.assign_sudo_users([@user.id])
+        request.assign_sudo_users([user.id])
       end
 
       it 'creates a sudo user assignment' do
-        expect((@request.sudo_user_assignments.select { |assignment| assignment.user_id == @user.id && assignment.sudo == true }).size).to eq(1)
+        expect((request.sudo_user_assignments.select { |assignment| assignment.user_id == user.id && assignment.sudo == true }).size).to eq(1)
       end
 
       it 'does not create a user assignment' do
-        expect(@request.users_assigned_to_requests.select { |assignment| assignment.user_id == @user.id && assignment.sudo == false }).to be_empty
+        expect(request.users_assigned_to_requests.select { |assignment| assignment.user_id == user.id && assignment.sudo == false }).to be_empty
       end
     end
   end
 
   describe 'puppet script helper methods' do
-    before do
-      @request = FactoryBot.create(:request_with_users)
-    end
+    let(:request) { FactoryBot.create(:request_with_users) }
 
     it 'returns correct declaration script for a given request' do
-      script = @request.generate_puppet_name_script
+      script = request.generate_puppet_name_script
       expected_string = <<~NAME_SCRIPT
         node \'myvm\'{
 
@@ -175,7 +221,7 @@ class { node_myvm: }
     end
 
     it 'returns correct initialization script for a given request' do
-      script = @request.generate_puppet_node_script
+      script = request.generate_puppet_node_script
       expected_string = <<~NODE_SCRIPT
         class node_myvm {
                 $admins = []