Split out updating a trace into an update action

app/controllers/traces_controller.rb

@@ -175,13 +175,24 @@ class TracesController < ApplicationController
       head :forbidden
     else
       @title = t ".title", :name => @trace.name
-
-      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
+  rescue ActiveRecord::RecordNotFound
+    head :not_found
+  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

app/views/traces/edit.html.erb

@@ -4,7 +4,7 @@
 
 <img src="<%= url_for :controller => 'traces', :action => 'picture', :id => @trace.id, :display_name => @trace.user.display_name %>">
 
-<%= form_for @trace, :method => :post, :url => { :action => "edit" } do |f| %>
+<%= form_for @trace do |f| %>
 
 <div id='edit-trace-form' class='standard-form'>
   <fieldset>

app/views/traces/view.html.erb

@@ -57,9 +57,7 @@
 <% if current_user && (current_user==@trace.user || current_user.administrator? || current_user.moderator?)%>
   <div class="buttons">
     <% if current_user == @trace.user %>
-      <div>
-        <%= button_to t('.edit_track'), trace_edit_path(@trace) %>
-      </div>
+      <%= 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') } %>
   </div>

config/locales/en.yml

@@ -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:

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

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)