From: Tom Hughes Date: Wed, 4 Dec 2019 16:08:26 +0000 (+0000) Subject: Merge remote-tracking branch 'upstream/pull/2451' X-Git-Tag: live~3009 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/6a68e7676d950661edf24e572ee6ba302b9b3cc4?hp=35370684e503ef4c51a9b6798eef12d336ed1ee9 Merge remote-tracking branch 'upstream/pull/2451' --- 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/abilities/api_ability.rb b/app/abilities/api_ability.rb index 217fe9713..62cd2b17e 100644 --- a/app/abilities/api_ability.rb +++ b/app/abilities/api_ability.rb @@ -37,7 +37,7 @@ class ApiAbility can [:index, :show, :update, :update_all, :destroy], UserPreference if user.terms_agreed? - can [:create, :update, :upload, :close, :subscribe, :unsubscribe, :expand_bbox], Changeset + can [:create, :update, :upload, :close, :subscribe, :unsubscribe], Changeset can :create, ChangesetComment can [:create, :update, :delete], Node can [:create, :update, :delete], Way diff --git a/app/abilities/api_capability.rb b/app/abilities/api_capability.rb index 64861f1d6..beb4d39bf 100644 --- a/app/abilities/api_capability.rb +++ b/app/abilities/api_capability.rb @@ -14,7 +14,7 @@ class ApiCapability can [:update, :update_all, :destroy], UserPreference if capability?(token, :allow_write_prefs) if token&.user&.terms_agreed? - can [:create, :update, :upload, :close, :subscribe, :unsubscribe, :expand_bbox], Changeset if capability?(token, :allow_write_api) + can [:create, :update, :upload, :close, :subscribe, :unsubscribe], Changeset if capability?(token, :allow_write_api) can :create, ChangesetComment if capability?(token, :allow_write_api) can [:create, :update, :delete], Node if capability?(token, :allow_write_api) can [:create, :update, :delete], Way if capability?(token, :allow_write_api) diff --git a/app/controllers/api/changesets_controller.rb b/app/controllers/api/changesets_controller.rb index 5f87324e0..316015228 100644 --- a/app/controllers/api/changesets_controller.rb +++ b/app/controllers/api/changesets_controller.rb @@ -61,50 +61,6 @@ module Api head :ok end - ## - # insert a (set of) points into a changeset bounding box. this can only - # increase the size of the bounding box. this is a hint that clients can - # set either before uploading a large number of changes, or changes that - # the client (but not the server) knows will affect areas further away. - def expand_bbox - # only allow POST requests, because although this method is - # idempotent, there is no "document" to PUT really... - assert_method :post - - cs = Changeset.find(params[:id]) - check_changeset_consistency(cs, current_user) - - # keep an array of lons and lats - lon = [] - lat = [] - - # the request is in pseudo-osm format... this is kind-of an - # abuse, maybe should change to some other format? - doc = XML::Parser.string(request.raw_post, :options => XML::Parser::Options::NOERROR).parse - doc.find("//osm/node").each do |n| - lon << n["lon"].to_f * GeoRecord::SCALE - lat << n["lat"].to_f * GeoRecord::SCALE - end - - # add the existing bounding box to the lon-lat array - lon << cs.min_lon unless cs.min_lon.nil? - lat << cs.min_lat unless cs.min_lat.nil? - lon << cs.max_lon unless cs.max_lon.nil? - lat << cs.max_lat unless cs.max_lat.nil? - - # collapse the arrays to minimum and maximum - cs.min_lon = lon.min.round - cs.min_lat = lat.min.round - cs.max_lon = lon.max.round - cs.max_lat = lat.max.round - - # save the larger bounding box and return the changeset, which - # will include the bigger bounding box. - cs.save! - @changeset = cs - render "changeset" - end - ## # Upload a diff in a single transaction. # 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..992197814 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -12,7 +12,6 @@ OpenStreetMap::Application.routes.draw do put "changeset/create" => "api/changesets#create" post "changeset/:id/upload" => "api/changesets#upload", :id => /\d+/ get "changeset/:id/download" => "api/changesets#download", :as => :changeset_download, :id => /\d+/ - post "changeset/:id/expand_bbox" => "api/changesets#expand_bbox", :id => /\d+/ get "changeset/:id" => "api/changesets#show", :as => :changeset_show, :id => /\d+/ post "changeset/:id/subscribe" => "api/changesets#subscribe", :as => :changeset_subscribe, :id => /\d+/ post "changeset/:id/unsubscribe" => "api/changesets#unsubscribe", :as => :changeset_unsubscribe, :id => /\d+/ @@ -208,7 +207,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/api/changesets_controller_test.rb b/test/controllers/api/changesets_controller_test.rb index a4583a928..be6b84b47 100644 --- a/test/controllers/api/changesets_controller_test.rb +++ b/test/controllers/api/changesets_controller_test.rb @@ -17,10 +17,6 @@ module Api { :path => "/api/0.6/changeset/1/download", :method => :get }, { :controller => "api/changesets", :action => "download", :id => "1" } ) - assert_routing( - { :path => "/api/0.6/changeset/1/expand_bbox", :method => :post }, - { :controller => "api/changesets", :action => "expand_bbox", :id => "1" } - ) assert_routing( { :path => "/api/0.6/changeset/1", :method => :get }, { :controller => "api/changesets", :action => "show", :id => "1" } @@ -1501,57 +1497,6 @@ CHANGESET assert_select "osm>changeset[max_lat='3.0000000']", 1 end - ## - # test that the changeset :include method works as it should - def test_changeset_include - basic_authorization create(:user).display_name, "test" - - # create a new changeset - put :create, :body => "" - assert_response :success, "Creating of changeset failed." - changeset_id = @response.body.to_i - - # NOTE: the include method doesn't over-expand, like inserting - # a real method does. this is because we expect the client to - # know what it is doing! - check_after_include(changeset_id, 1, 1, [1, 1, 1, 1]) - check_after_include(changeset_id, 3, 3, [1, 1, 3, 3]) - check_after_include(changeset_id, 4, 2, [1, 1, 4, 3]) - check_after_include(changeset_id, 2, 2, [1, 1, 4, 3]) - check_after_include(changeset_id, -1, -1, [-1, -1, 4, 3]) - check_after_include(changeset_id, -2, 5, [-2, -1, 4, 5]) - end - - ## - # test that a not found, wrong method with the expand bbox works as expected - def test_changeset_expand_bbox_error - basic_authorization create(:user).display_name, "test" - - # create a new changeset - xml = "" - put :create, :body => xml - assert_response :success, "Creating of changeset failed." - changeset_id = @response.body.to_i - - lon = 58.2 - lat = -0.45 - - # Try and put - xml = "" - put :expand_bbox, :params => { :id => changeset_id }, :body => xml - assert_response :method_not_allowed, "shouldn't be able to put a bbox expand" - - # Try to get the update - xml = "" - get :expand_bbox, :params => { :id => changeset_id }, :body => xml - assert_response :method_not_allowed, "shouldn't be able to get a bbox expand" - - # Try to use a hopefully missing changeset - xml = "" - post :expand_bbox, :params => { :id => changeset_id + 13245 }, :body => xml - assert_response :not_found, "shouldn't be able to do a bbox expand on a nonexistant changeset" - end - ## # test the query functionality of changesets def test_query @@ -1929,26 +1874,6 @@ CHANGESET end end - ## - # call the include method and assert properties of the bbox - def check_after_include(changeset_id, lon, lat, bbox) - xml = "" - post :expand_bbox, :params => { :id => changeset_id }, :body => xml - assert_response :success, "Setting include of changeset failed: #{@response.body}" - - # check exactly one changeset - assert_select "osm>changeset", 1 - assert_select "osm>changeset[id='#{changeset_id}']", 1 - - # check the bbox - doc = XML::Parser.string(@response.body).parse - changeset = doc.find("//osm/changeset").first - assert_equal bbox[0], changeset["min_lon"].to_f, "min lon" - assert_equal bbox[1], changeset["min_lat"].to_f, "min lat" - assert_equal bbox[2], changeset["max_lon"].to_f, "max lon" - assert_equal bbox[3], changeset["max_lat"].to_f, "max lat" - end - ## # update the changeset_id of a way element def update_changeset(xml, changeset_id) 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)