From 44c6fdc27340ead5e88ca60558978babf4baf2d9 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 4 Dec 2019 11:59:18 +0100 Subject: [PATCH] Use the conventional 'destroy' method name for destroying traces --- app/abilities/ability.rb | 2 +- app/controllers/traces_controller.rb | 6 +++--- app/views/traces/show.html.erb | 2 +- config/routes.rb | 1 - test/controllers/traces_controller_test.rb | 22 +++++++++++----------- 5 files changed, 16 insertions(+), 17 deletions(-) diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index c34f357a9..f0cebb380 100644 --- a/app/abilities/ability.rb +++ b/app/abilities/ability.rb @@ -40,7 +40,7 @@ class Ability can [:new, :create, :reply, :show, :inbox, :outbox, :mark, :destroy], Message can [:close, :reopen], Note can [:new, :create], Report - can [:mine, :new, :create, :edit, :update, :delete], Trace + can [:mine, :new, :create, :edit, :update, :destroy], Trace can [:account, :go_public, :make_friend, :remove_friend], User if user.moderator? diff --git a/app/controllers/traces_controller.rb b/app/controllers/traces_controller.rb index a0852d2ce..b800d305e 100644 --- a/app/controllers/traces_controller.rb +++ b/app/controllers/traces_controller.rb @@ -7,9 +7,9 @@ class TracesController < ApplicationController authorize_resource - before_action :check_database_writable, :only => [:new, :create, :edit, :delete] + before_action :check_database_writable, :only => [:new, :create, :edit, :destroy] before_action :offline_warning, :only => [:mine, :show] - before_action :offline_redirect, :only => [:new, :create, :edit, :delete, :data] + before_action :offline_redirect, :only => [:new, :create, :edit, :destroy, :data] # Counts and selects pages of GPX traces for various criteria (by user, tags, public etc.). # target_user - if set, specifies the user to fetch traces for. if not set will fetch all traces @@ -184,7 +184,7 @@ class TracesController < ApplicationController head :not_found end - def delete + def destroy trace = Trace.find(params[:id]) if !trace.visible? diff --git a/app/views/traces/show.html.erb b/app/views/traces/show.html.erb index 0ebbd827f..b25ecad53 100644 --- a/app/views/traces/show.html.erb +++ b/app/views/traces/show.html.erb @@ -59,6 +59,6 @@ <% if current_user == @trace.user %> <%= link_to t(".edit_trace"), edit_trace_path(@trace), :class => "button" %> <% end %> - <%= button_to t(".delete_trace"), { :controller => "traces", :action => "delete", :id => @trace.id }, { :data => { :confirm => t(".confirm_delete") } } %> + <%= button_to t(".delete_trace"), { :controller => "traces", :action => "destroy", :method => :delete, :id => @trace.id }, { :data => { :confirm => t(".confirm_delete") } } %> <% end %> diff --git a/config/routes.rb b/config/routes.rb index d936072d7..d61da1ea9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -208,7 +208,6 @@ OpenStreetMap::Application.routes.draw do get "/trace/create", :to => redirect(:path => "/traces/new") get "/trace/:id/data" => "traces#data", :id => /\d+/, :as => "trace_data" get "/trace/:id/edit", :to => redirect(:path => "/traces/%{id}/edit") - post "/trace/:id/delete" => "traces#delete", :id => /\d+/ # diary pages resources :diary_entries, :path => "diary", :only => [:new, :create, :index] do diff --git a/test/controllers/traces_controller_test.rb b/test/controllers/traces_controller_test.rb index 45b0358f5..059242af9 100644 --- a/test/controllers/traces_controller_test.rb +++ b/test/controllers/traces_controller_test.rb @@ -115,8 +115,8 @@ class TracesControllerTest < ActionController::TestCase { :controller => "traces", :action => "update", :id => "1" } ) assert_routing( - { :path => "/trace/1/delete", :method => :post }, - { :controller => "traces", :action => "delete", :id => "1" } + { :path => "/traces/1", :method => :delete }, + { :controller => "traces", :action => "destroy", :id => "1" } ) end @@ -637,39 +637,39 @@ class TracesControllerTest < ActionController::TestCase assert_equal new_details[:visibility], trace.visibility end - # Test deleting a trace - def test_delete + # Test destroying a trace + def test_destroy public_trace_file = create(:trace, :visibility => "public") deleted_trace_file = create(:trace, :deleted) # First with no auth - post :delete, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id } + delete :destroy, :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 :delete, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id }, :session => { :user => create(:user) } + delete :destroy, :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 :delete, :params => { :display_name => create(:user).display_name, :id => 0 }, :session => { :user => create(:user) } + delete :destroy, :params => { :display_name => create(:user).display_name, :id => 0 }, :session => { :user => create(:user) } assert_response :not_found # Now with a trace has already been deleted - post :delete, :params => { :display_name => deleted_trace_file.user.display_name, :id => deleted_trace_file.id }, :session => { :user => deleted_trace_file.user } + delete :destroy, :params => { :display_name => deleted_trace_file.user.display_name, :id => deleted_trace_file.id }, :session => { :user => deleted_trace_file.user } assert_response :not_found # Now with a trace that we are allowed to delete - post :delete, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id }, :session => { :user => public_trace_file.user } + delete :destroy, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id }, :session => { :user => public_trace_file.user } assert_response :redirect assert_redirected_to :action => :index, :display_name => public_trace_file.user.display_name trace = Trace.find(public_trace_file.id) assert_equal false, trace.visible - # Finally with a trace that is deleted by an admin + # Finally with a trace that is destroyed by an admin public_trace_file = create(:trace, :visibility => "public") admin = create(:administrator_user) - post :delete, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id }, :session => { :user => admin } + delete :destroy, :params => { :display_name => public_trace_file.user.display_name, :id => public_trace_file.id }, :session => { :user => admin } assert_response :redirect assert_redirected_to :action => :index, :display_name => public_trace_file.user.display_name trace = Trace.find(public_trace_file.id) -- 2.43.2