From 44494128c4607d76c8c5b426900f0abbad7961a3 Mon Sep 17 00:00:00 2001 From: Christoph Matthies Date: Tue, 26 Mar 2019 17:40:44 +0100 Subject: [PATCH 1/6] Fix(Servers Controller): Refactor --- app/controllers/servers_controller.rb | 105 ++++---------------------- 1 file changed, 16 insertions(+), 89 deletions(-) diff --git a/app/controllers/servers_controller.rb b/app/controllers/servers_controller.rb index 4cfd4bd5..c70a495b 100644 --- a/app/controllers/servers_controller.rb +++ b/app/controllers/servers_controller.rb @@ -4,14 +4,13 @@ class ServersController < ApplicationController before_action :set_server, only: %i[show edit update destroy] before_action :authenticate_employee, only: %i[show index] before_action :authenticate_admin, only: %i[new create edit update destroy] + # GET /servers - # GET /servers.json def index @servers = Server.all end # GET /servers/1 - # GET /servers/1.json def show; end # GET /servers/new @@ -23,103 +22,28 @@ def new def edit; end # POST /servers - # POST /servers.json def create - # parse software fields from the post parameters into the server_params - software = [] - - params.each do |key, value| - key_matcher = /software\d+/ - software << value if key_matcher.match?(key) + @server = Server.new(server_params) + if @server.save + redirect_to @server, notice: 'Server was successfully created.' + else + render :new end - - # merge server_params [and software information - server_params[:installed_software] = software - - server_params.permit( - :name, - :cpu_cores, - :ram_gb, - :storage_gb, - :ipv4_address, - :ipv6_address, - :mac_address, - :fqdn, - :installed_software, - :vendor, - :model, - :responsible_id, - :description - ) - - # create new Server object - setNewServerObject - - respond_to do |format| - if @server.save - format.html { redirect_to @server, notice: 'Server was successfully created.' } - format.json { render :show, status: :created, location: @server } - else - format.html { render :new } - format.json { render json: @server.errors, status: :unprocessable_entity } - end - end - end - - def setNewServerObject - @server = Server.new( - name: server_params[:name], - cpu_cores: server_params[:cpu_cores], - ram_gb: server_params[:ram_gb], - storage_gb: server_params[:storage_gb], - mac_address: server_params[:mac_address], - fqdn: server_params[:fqdn], - ipv4_address: server_params[:ipv4_address], - ipv6_address: server_params[:ipv6_address], - installed_software: server_params[:installed_software], - vendor: server_params[:vendor], - model: server_params[:model], - description: server_params[:description], - responsible_id: server_params[:responsible] - ) end - + # PATCH/PUT /servers/1 - # PATCH/PUT /servers/1.json def update - respond_to do |format| - if @server.update(server_params.permit( - :name, - :cpu_cores, - :ram_gb, - :storage_gb, - :ipv4_address, - :ipv6_address, - :mac_address, - :fqdn, - :installed_software, - :model, - :vendor, - :description, - :responsible_id - )) - format.html { redirect_to @server, notice: 'Server was successfully updated.' } - format.json { render :show, status: :ok, location: @server } - else - format.html { render :edit } - format.json { render json: @server.errors, status: :unprocessable_entity } - end + if @server.update(server_params) + redirect_to @server, notice: 'Server was successfully updated.' + else + render :edit end end # DELETE /servers/1 - # DELETE /servers/1.json def destroy @server.destroy - respond_to do |format| - format.html { redirect_to servers_url, notice: 'Server was successfully destroyed.' } - format.json { head :no_content } - end + redirect_to servers_url, notice: 'Server was successfully destroyed.' end private @@ -131,6 +55,9 @@ def set_server # Never trust parameters from the scary internet, only allow the white list through. def server_params - params.fetch(:server, {}) + # `installed_software: []` permits arrays + params.require(:server).permit(:name, :cpu_cores, :ram_gb, :storage_gb, :ipv4_address, + :ipv6_address, :mac_address, :fqdn, {installed_software: []}, + :model, :vendor, :description, :responsible_id) end end From 5aafc00794926cfce78c4f4c98aedeab5e3fa924 Mon Sep 17 00:00:00 2001 From: Christoph Matthies Date: Tue, 26 Mar 2019 17:41:00 +0100 Subject: [PATCH 2/6] Fix(Servers form): Allow editing software --- app/views/servers/_form.html.erb | 57 +++++++++++++++----------------- 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/app/views/servers/_form.html.erb b/app/views/servers/_form.html.erb index 9677aeea..0d054909 100644 --- a/app/views/servers/_form.html.erb +++ b/app/views/servers/_form.html.erb @@ -21,13 +21,10 @@
- <%= form.label :responsible, "Responsible" %>
- <%= form.select 'responsible', - options_from_collection_for_select(User.all, - :id, - :human_readable_identifier), - {}, - { multiple: false, class: "selecttwo", style: 'width: 50%' } %> + <%= form.label :responsible, "Responsible" %> + <%# form.collection_select(method, collection, value_method, text_method, options = {}, html_options = {}) %> + <%= form.collection_select(:responsible_id, User.all, :id, :human_readable_identifier, + options={prompt: true}, html_options={class: "selecttwo"}) %>
@@ -85,32 +82,30 @@
Software Information
- - -function addInput() -{ - var boxName="software" + countBox; - var input = document.createElement("div"); - input.setAttribute("class", "field"); - - var inputKind = document.createElement("INPUT"); - inputKind.setAttribute("type", "text"); - inputKind.setAttribute("placeholder", "Software " + countBox); - inputKind.setAttribute("class", "form-control"); - inputKind.setAttribute("name", boxName); - inputKind.setAttribute("id", boxName); - - input.appendChild(inputKind); - document.getElementById('response').appendChild(input); - - countBox += 1; -} - +
+ <%= form.label :installed_software, "Installed Software" %> + <% server.installed_software.each do |software| %> + <%= form.text_field :installed_software, multiple: true, value: software, class: 'form-control' %> + <% end %> -
+
From a8f779dbc0055d4624976f6a5ed61e046cc0263b Mon Sep 17 00:00:00 2001 From: Christoph Matthies Date: Tue, 26 Mar 2019 18:00:52 +0100 Subject: [PATCH 3/6] Fix(Server tests): Use ids --- spec/controllers/servers_controller_spec.rb | 5 ++--- spec/views/servers/new.html.erb_spec.rb | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/spec/controllers/servers_controller_spec.rb b/spec/controllers/servers_controller_spec.rb index 57f8b5e8..1ed09b32 100644 --- a/spec/controllers/servers_controller_spec.rb +++ b/spec/controllers/servers_controller_spec.rb @@ -14,7 +14,7 @@ ipv4_address: '8.8.8.8', ipv6_address: '::1', installed_software: ['SpeckTester'], - responsible: FactoryBot.create(:user) + responsible_id: FactoryBot.create(:user).id } end @@ -28,8 +28,7 @@ fqdn: 'arrrr.speck.de', ipv4_address: 'c8.a8.d8.b8', ipv6_address: 42, - installed_software: ['SpeckTester'], - responsible: 'Hans Würtschen' + installed_software: ['SpeckTester'] } end diff --git a/spec/views/servers/new.html.erb_spec.rb b/spec/views/servers/new.html.erb_spec.rb index 91cc6d01..1a948402 100644 --- a/spec/views/servers/new.html.erb_spec.rb +++ b/spec/views/servers/new.html.erb_spec.rb @@ -38,7 +38,7 @@ assert_select 'input[name=?]', 'server[ipv6_address]' - assert_select 'select[name=?]', 'server[responsible]' + assert_select 'select[name=?]', 'server[responsible_id]' expect(rendered).to have_button 'Add Software' end From c03a348eb033caa122efe2158822cffd3f80901e Mon Sep 17 00:00:00 2001 From: Christoph Matthies Date: Tue, 26 Mar 2019 18:25:19 +0100 Subject: [PATCH 4/6] Refactor(Server Controller sPec): Use factories --- spec/controllers/servers_controller_spec.rb | 45 ++++----------------- 1 file changed, 7 insertions(+), 38 deletions(-) diff --git a/spec/controllers/servers_controller_spec.rb b/spec/controllers/servers_controller_spec.rb index 1ed09b32..13308e48 100644 --- a/spec/controllers/servers_controller_spec.rb +++ b/spec/controllers/servers_controller_spec.rb @@ -4,32 +4,12 @@ RSpec.describe ServersController, type: :controller do let(:valid_attributes) do - { - name: 'SpecServer', - cpu_cores: 4, - ram_gb: 1024, - storage_gb: 4096, - mac_address: 'C0:FF:EE:C4:11:42', - fqdn: 'arrrr.speck.de', - ipv4_address: '8.8.8.8', - ipv6_address: '::1', - installed_software: ['SpeckTester'], - responsible_id: FactoryBot.create(:user).id - } + employee = FactoryBot.create(:employee) + FactoryBot.attributes_for(:server, responsible_id: employee.id) end - + let(:invalid_attributes) do - { - name: 'SpecServer', - cpu_cores: '', - ram_gb: 1024, - storage_gb: 4096, - mac_address: 'EE:C4:11:42', - fqdn: 'arrrr.speck.de', - ipv4_address: 'c8.a8.d8.b8', - ipv6_address: 42, - installed_software: ['SpeckTester'] - } + { cpu_cores: 'twelve-hundred', mac_address: 1234 } end let(:valid_session) { {} } @@ -98,25 +78,14 @@ describe 'PUT #update' do context 'with valid params' do let(:new_attributes) do - { - name: 'SpeckServer', - cpu_cores: 2, - ram_gb: 1024, - storage_gb: 4096, - mac_address: 'C0:FF:EE:C4:11:42', - fqdn: 'arrrr.speck.de', - ipv4_address: '8.8.8.8', - ipv6_address: '::1', - installed_software: ['SpeckTester'], - responsible: FactoryBot.create(:admin) - } + FactoryBot.attributes_for(:server, name: 'Changed') end it 'updates the requested server' do - server = Server.create! valid_attributes + server = Server.create! valid_attributes.update(name: 'Original') put :update, params: { id: server.to_param, server: new_attributes }, session: valid_session server.reload - expect(server.name).to eq('SpeckServer') + expect(server.name).to eq(new_attributes[:name]) end it 'redirects to the server' do From 39b4fc699cfc92b7dbfbacdca7b627472c9cadd8 Mon Sep 17 00:00:00 2001 From: Christoph Matthies Date: Wed, 27 Mar 2019 11:05:33 +0100 Subject: [PATCH 5/6] Fix(Server controller): No save of empty fields, tests --- app/controllers/servers_controller.rb | 6 ++-- app/views/servers/_form.html.erb | 21 +++++--------- spec/controllers/servers_controller_spec.rb | 31 +++++++++++++++------ 3 files changed, 33 insertions(+), 25 deletions(-) diff --git a/app/controllers/servers_controller.rb b/app/controllers/servers_controller.rb index c70a495b..36c1b28c 100644 --- a/app/controllers/servers_controller.rb +++ b/app/controllers/servers_controller.rb @@ -39,7 +39,7 @@ def update render :edit end end - + # DELETE /servers/1 def destroy @server.destroy @@ -47,7 +47,7 @@ def destroy end private - + # Use callbacks to share common setup or constraints between actions. def set_server @server = Server.find(params[:id]) @@ -55,6 +55,8 @@ def set_server # Never trust parameters from the scary internet, only allow the white list through. def server_params + # Remove empty software form fields + params[:server][:installed_software].reject!(&:blank?) if params[:server][:installed_software] # `installed_software: []` permits arrays params.require(:server).permit(:name, :cpu_cores, :ram_gb, :storage_gb, :ipv4_address, :ipv6_address, :mac_address, :fqdn, {installed_software: []}, diff --git a/app/views/servers/_form.html.erb b/app/views/servers/_form.html.erb index 0d054909..53cf0af3 100644 --- a/app/views/servers/_form.html.erb +++ b/app/views/servers/_form.html.erb @@ -83,26 +83,19 @@
<%= form.label :installed_software, "Installed Software" %> <% server.installed_software.each do |software| %> - <%= form.text_field :installed_software, multiple: true, value: software, class: 'form-control' %> + <%= form.text_field :installed_software, multiple: true, value: software, class: 'form-control mb-1' %> <% end %> diff --git a/spec/controllers/servers_controller_spec.rb b/spec/controllers/servers_controller_spec.rb index 13308e48..fe718ff0 100644 --- a/spec/controllers/servers_controller_spec.rb +++ b/spec/controllers/servers_controller_spec.rb @@ -58,13 +58,21 @@ describe 'POST #create' do context 'with valid params' do it 'creates a new Server' do - expect { post :create, params: { server: valid_attributes }, session: valid_session }.to change(Server, :count).by(1) + expect { + post :create, params: { server: valid_attributes }, session: valid_session + }.to change(Server, :count).by(1) end it 'redirects to the created server' do post :create, params: { server: valid_attributes }, session: valid_session expect(response).to redirect_to(Server.last) end + + it 'creates a new Server without saving empty software fields' do + valid_attributes.update(installed_software: ['software','','']) + post :create, params: { server: valid_attributes }, session: valid_session + expect(Server.last.installed_software).to eq(['software']) + end end context 'with invalid params' do @@ -77,15 +85,12 @@ describe 'PUT #update' do context 'with valid params' do - let(:new_attributes) do - FactoryBot.attributes_for(:server, name: 'Changed') - end - it 'updates the requested server' do - server = Server.create! valid_attributes.update(name: 'Original') - put :update, params: { id: server.to_param, server: new_attributes }, session: valid_session - server.reload - expect(server.name).to eq(new_attributes[:name]) + server = FactoryBot.create(:server, name: 'Original') + valid_attributes.update(name: 'Changed') + expect{ + put :update, params: { id: server.to_param, server: valid_attributes }, session: valid_session + }.to change{ server.reload.name }.from(server.name).to(valid_attributes[:name]) end it 'redirects to the server' do @@ -93,6 +98,14 @@ put :update, params: { id: server.to_param, server: valid_attributes }, session: valid_session expect(response).to redirect_to(server) end + + it 'updates the requested server without saving empty software fields' do + server = FactoryBot.create(:server, installed_software: []) + valid_attributes.update(installed_software: ['software','','']) + expect{ + put :update, params: { id: server.to_param, server: valid_attributes }, session: valid_session + }.to change{ server.reload.installed_software }.from(server.installed_software).to(['software']) + end end context 'with invalid params' do From 11ebec6c052276d067cc18530458d5d046f031e7 Mon Sep 17 00:00:00 2001 From: Christoph Matthies Date: Wed, 27 Mar 2019 11:20:36 +0100 Subject: [PATCH 6/6] Chore(Server Controller): Whitespace --- app/controllers/servers_controller.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/servers_controller.rb b/app/controllers/servers_controller.rb index 4f865ef3..2d8357c2 100644 --- a/app/controllers/servers_controller.rb +++ b/app/controllers/servers_controller.rb @@ -30,7 +30,7 @@ def create render :new end end - + # PATCH/PUT /servers/1 def update if @server.update(server_params) @@ -39,7 +39,7 @@ def update render :edit end end - + # DELETE /servers/1 def destroy @server.destroy @@ -47,7 +47,7 @@ def destroy end private - + # Use callbacks to share common setup or constraints between actions. def set_server @server = Server.find(params[:id])