From b745126b6ecf679fb43de65d571e44a149dfc7f6 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 29 Aug 2018 17:31:12 +0800 Subject: [PATCH] Split out updating a trace into an update action --- app/controllers/traces_controller.rb | 27 ++++++++++--- app/views/traces/edit.html.erb | 2 +- app/views/traces/view.html.erb | 4 +- config/locales/en.yml | 4 +- config/routes.rb | 5 ++- test/controllers/traces_controller_test.rb | 46 +++++----------------- 6 files changed, 39 insertions(+), 49 deletions(-) diff --git a/app/controllers/traces_controller.rb b/app/controllers/traces_controller.rb index ab4c8f3b8..31ce88ec3 100644 --- a/app/controllers/traces_controller.rb +++ b/app/controllers/traces_controller.rb @@ -175,13 +175,24 @@ class TracesController < ApplicationController head :forbidden else @title = t ".title", :name => @trace.name + end + rescue ActiveRecord::RecordNotFound + head :not_found + end - if request.post? && params[:trace] - @trace.description = params[:trace][:description] - @trace.tagstring = params[:trace][:tagstring] - @trace.visibility = params[:trace][:visibility] - redirect_to :action => "view", :display_name => current_user.display_name if @trace.save - end + def update + @trace = Trace.find(params[:id]) + + if !@trace.visible? + head :not_found + elsif current_user.nil? || @trace.user != current_user + head :forbidden + elsif @trace.update(trace_params) + flash[:notice] = t ".updated" + redirect_to :action => "view", :display_name => current_user.display_name + else + @title = t ".title", :name => @trace.name + render :action => "edit" end rescue ActiveRecord::RecordNotFound head :not_found @@ -413,4 +424,8 @@ class TracesController < ApplicationController "public" end end + + def trace_params + params.require(:trace).permit(:description, :tagstring, :visibility) + end end diff --git a/app/views/traces/edit.html.erb b/app/views/traces/edit.html.erb index 6657a33ad..b5b4a84d6 100644 --- a/app/views/traces/edit.html.erb +++ b/app/views/traces/edit.html.erb @@ -4,7 +4,7 @@ -<%= form_for @trace, :method => :post, :url => { :action => "edit" } do |f| %> +<%= form_for @trace do |f| %>
diff --git a/app/views/traces/view.html.erb b/app/views/traces/view.html.erb index 648160c2f..59c8a0a1f 100644 --- a/app/views/traces/view.html.erb +++ b/app/views/traces/view.html.erb @@ -57,9 +57,7 @@ <% if current_user && (current_user==@trace.user || current_user.administrator? || current_user.moderator?)%>
<% if current_user == @trace.user %> -
- <%= button_to t('.edit_track'), trace_edit_path(@trace) %> -
+ <%= link_to t('.edit_track'), edit_trace_path(@trace), :class => "button" %> <% end %> <%= button_to t('.delete_track'), { :controller => 'traces', :action => 'delete', :id => @trace.id }, :data => { :confirm => t('.confirm_delete') } %>
diff --git a/config/locales/en.yml b/config/locales/en.yml index 5bc88547a..5854df0d9 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1024,7 +1024,7 @@ en: other_label: Other create: successful_report: Your report has been registered sucessfully - provide_details: Please provide the required details + provide_details: Please provide the required details layouts: project_name: # in @@ -1722,6 +1722,8 @@ en: visibility: "Visibility:" visibility_help: "what does this mean?" visibility_help_url: "https://wiki.openstreetmap.org/wiki/Visibility_of_GPS_traces" + update: + updated: Trace updated trace_optionals: tags: "Tags" view: diff --git a/config/routes.rb b/config/routes.rb index 6a3efe3b0..4b126c752 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -208,11 +208,12 @@ OpenStreetMap::Application.routes.draw do get "/traces/mine/tag/:tag" => "traces#mine" get "/traces/mine/page/:page" => "traces#mine" get "/traces/mine" => "traces#mine" - resources :traces, :only => [:new, :create] + resources :traces, :only => [:new, :create, :edit, :update] post "/trace/create" => "traces#create" # remove after deployment get "/trace/create", :to => redirect(:path => "/traces/new") get "/trace/:id/data" => "traces#data", :id => /\d+/, :as => "trace_data" - match "/trace/:id/edit" => "traces#edit", :via => [:get, :post], :id => /\d+/, :as => "trace_edit" + post "trace/:id/edit" => "traces#update" # remove after deployment + get "/trace/:id/edit", :to => redirect(:path => "/traces/%{id}/edit") post "/trace/:id/delete" => "traces#delete", :id => /\d+/ # diary pages diff --git a/test/controllers/traces_controller_test.rb b/test/controllers/traces_controller_test.rb index fbbbe4adc..5a5b8631b 100644 --- a/test/controllers/traces_controller_test.rb +++ b/test/controllers/traces_controller_test.rb @@ -151,12 +151,12 @@ class TracesControllerTest < ActionController::TestCase { :controller => "traces", :action => "data", :id => "1", :format => "xml" } ) assert_routing( - { :path => "/trace/1/edit", :method => :get }, + { :path => "/traces/1/edit", :method => :get }, { :controller => "traces", :action => "edit", :id => "1" } ) assert_routing( - { :path => "/trace/1/edit", :method => :post }, - { :controller => "traces", :action => "edit", :id => "1" } + { :path => "/traces/1", :method => :put }, + { :controller => "traces", :action => "update", :id => "1" } ) assert_routing( { :path => "/trace/1/delete", :method => :post }, @@ -594,7 +594,7 @@ class TracesControllerTest < ActionController::TestCase # First with no auth get :edit, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id } assert_response :redirect - assert_redirected_to :controller => :user, :action => :login, :referer => trace_edit_path(:display_name => public_trace_file.user.display_name, :id => public_trace_file.id) + assert_redirected_to :controller => :user, :action => :login, :referer => edit_trace_path(:display_name => public_trace_file.user.display_name, :id => public_trace_file.id) # Now with some other user, which should fail get :edit, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id }, :session => { :user => create(:user) } @@ -613,34 +613,8 @@ class TracesControllerTest < ActionController::TestCase assert_response :success end - # Test fetching the edit page for a trace using POST - def test_edit_post_no_details - public_trace_file = create(:trace, :visibility => "public") - deleted_trace_file = create(:trace, :deleted) - - # First with no auth - post :edit, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id } - assert_response :forbidden - - # Now with some other user, which should fail - post :edit, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id }, :session => { :user => create(:user) } - assert_response :forbidden - - # Now with a trace which doesn't exist - post :edit, :params => { :display_name => create(:user).display_name, :id => 0 }, :session => { :user => create(:user) } - assert_response :not_found - - # Now with a trace which has been deleted - post :edit, :params => { :display_name => deleted_trace_file.user.display_name, :id => deleted_trace_file.id }, :session => { :user => deleted_trace_file.user } - assert_response :not_found - - # Finally with a trace that we are allowed to edit - post :edit, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id }, :session => { :user => public_trace_file.user } - assert_response :success - end - # Test saving edits to a trace - def test_edit_post_with_details + def test_update public_trace_file = create(:trace, :visibility => "public") deleted_trace_file = create(:trace, :deleted) @@ -648,23 +622,23 @@ class TracesControllerTest < ActionController::TestCase new_details = { :description => "Changed description", :tagstring => "new_tag", :visibility => "private" } # First with no auth - post :edit, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id, :trace => new_details } + put :update, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id, :trace => new_details } assert_response :forbidden # Now with some other user, which should fail - post :edit, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id, :trace => new_details }, :session => { :user => create(:user) } + put :update, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id, :trace => new_details }, :session => { :user => create(:user) } assert_response :forbidden # Now with a trace which doesn't exist - post :edit, :params => { :display_name => create(:user).display_name, :id => 0 }, :session => { :user => create(:user), :trace => new_details } + put :update, :params => { :display_name => create(:user).display_name, :id => 0 }, :session => { :user => create(:user), :trace => new_details } assert_response :not_found # Now with a trace which has been deleted - post :edit, :params => { :display_name => deleted_trace_file.user.display_name, :id => deleted_trace_file.id, :trace => new_details }, :session => { :user => deleted_trace_file.user } + put :update, :params => { :display_name => deleted_trace_file.user.display_name, :id => deleted_trace_file.id, :trace => new_details }, :session => { :user => deleted_trace_file.user } assert_response :not_found # Finally with a trace that we are allowed to edit - post :edit, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id, :trace => new_details }, :session => { :user => public_trace_file.user } + put :update, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id, :trace => new_details }, :session => { :user => public_trace_file.user } assert_response :redirect assert_redirected_to :action => :view, :display_name => public_trace_file.user.display_name trace = Trace.find(public_trace_file.id) -- 2.43.2