From: Andy Allan Date: Wed, 4 Dec 2019 11:48:34 +0000 (+0100) Subject: Merge pull request #2427 from mmd-osm/patch/remove_expand_bbox X-Git-Tag: live~3319 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/f1c6a87aa137c11d0aff5a4b0e563ac2c2a8f82d?hp=-c Merge pull request #2427 from mmd-osm/patch/remove_expand_bbox Remove expand_bbox endpoint --- f1c6a87aa137c11d0aff5a4b0e563ac2c2a8f82d diff --combined app/abilities/api_ability.rb index 217fe9713,7aef15f11..62cd2b17e --- a/app/abilities/api_ability.rb +++ b/app/abilities/api_ability.rb @@@ -34,10 -34,10 +34,10 @@@ class ApiAbilit can [:new, :create], Report can [:create, :show, :update, :destroy, :data], Trace can [:details, :gpx_files], User - can [:read, :read_one, :update, :update_one, :delete_one], UserPreference + 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 --combined app/abilities/api_capability.rb index 64861f1d6,a4ca25204..beb4d39bf --- a/app/abilities/api_capability.rb +++ b/app/abilities/api_capability.rb @@@ -10,11 -10,11 +10,11 @@@ class ApiCapabilit can [:create, :update, :destroy], Trace if capability?(token, :allow_write_gpx) can [:details], User if capability?(token, :allow_read_prefs) can [:gpx_files], User if capability?(token, :allow_read_gpx) - can [:read, :read_one], UserPreference if capability?(token, :allow_read_prefs) - can [:update, :update_one, :delete_one], UserPreference if capability?(token, :allow_write_prefs) + can [:index, :show], UserPreference if capability?(token, :allow_read_prefs) + 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 --combined config/routes.rb index d61da1ea9,d8d91f47e..992197814 --- a/config/routes.rb +++ b/config/routes.rb @@@ -12,7 -12,6 +12,6 @@@ OpenStreetMap::Application.routes.draw 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+/ @@@ -67,11 -66,11 +66,11 @@@ get "user/gpx_files" => "api/users#gpx_files" get "users" => "api/users#index", :as => :api_users - get "user/preferences" => "api/user_preferences#read" - get "user/preferences/:preference_key" => "api/user_preferences#read_one" - put "user/preferences" => "api/user_preferences#update" - put "user/preferences/:preference_key" => "api/user_preferences#update_one" - delete "user/preferences/:preference_key" => "api/user_preferences#delete_one" + get "user/preferences" => "api/user_preferences#index" + get "user/preferences/:preference_key" => "api/user_preferences#show" + put "user/preferences" => "api/user_preferences#update_all" + put "user/preferences/:preference_key" => "api/user_preferences#update" + delete "user/preferences/:preference_key" => "api/user_preferences#destroy" post "gpx/create" => "api/traces#create" get "gpx/:id" => "api/traces#show", :id => /\d+/ @@@ -208,6 -207,7 +207,6 @@@ 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 --combined test/controllers/api/changesets_controller_test.rb index a4583a928,e8dda0ba1..be6b84b47 --- a/test/controllers/api/changesets_controller_test.rb +++ b/test/controllers/api/changesets_controller_test.rb @@@ -17,10 -17,6 +17,6 @@@ module Ap { :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" } @@@ -458,10 -454,10 +454,10 @@@ CHANGESE diff.root = XML::Node.new "osmChange" delete = XML::Node.new "delete" diff.root << delete - delete << super_relation.to_xml_node - delete << used_relation.to_xml_node - delete << used_way.to_xml_node - delete << used_node.to_xml_node + delete << xml_node_for_relation(super_relation) + delete << xml_node_for_relation(used_relation) + delete << xml_node_for_way(used_way) + delete << xml_node_for_node(used_node) # update the changeset to one that this user owns %w[node way relation].each do |type| @@@ -590,9 -586,9 +586,9 @@@ CHANGESE diff.root = XML::Node.new "osmChange" delete = XML::Node.new "delete" diff.root << delete - delete << other_relation.to_xml_node - delete << used_way.to_xml_node - delete << used_node.to_xml_node + delete << xml_node_for_relation(other_relation) + delete << xml_node_for_way(used_way) + delete << xml_node_for_node(used_node) # update the changeset to one that this user owns %w[node way relation].each do |type| @@@ -633,9 -629,9 +629,9 @@@ delete = XML::Node.new "delete" diff.root << delete delete["if-unused"] = "" - delete << used_relation.to_xml_node - delete << used_way.to_xml_node - delete << used_node.to_xml_node + delete << xml_node_for_relation(used_relation) + delete << xml_node_for_way(used_way) + delete << xml_node_for_node(used_node) # update the changeset to one that this user owns %w[node way relation].each do |type| @@@ -1137,7 -1133,7 +1133,7 @@@ CHANGESE diff = XML::Document.new diff.root = XML::Node.new "osmChange" modify = XML::Node.new "modify" - xml_old_node = old_node.to_xml_node + xml_old_node = xml_node_for_node(old_node) xml_old_node["lat"] = 2.0.to_s xml_old_node["lon"] = 2.0.to_s xml_old_node["changeset"] = changeset_id.to_s @@@ -1175,7 -1171,7 +1171,7 @@@ diff = XML::Document.new diff.root = XML::Node.new "osmChange" modify = XML::Node.new "modify" - xml_old_way = old_way.to_xml_node + xml_old_way = xml_node_for_way(old_way) nd_ref = XML::Node.new "nd" nd_ref["ref"] = create(:node, :lat => 3, :lon => 3).id.to_s xml_old_way << nd_ref @@@ -1228,7 -1224,7 +1224,7 @@@ diff.root = XML::Node.new "osmChange" delete = XML::Node.new "delete" diff.root << delete - delete << node.to_xml_node + delete << xml_node_for_node(node) # upload it error_format "xml" @@@ -1487,7 -1483,7 +1483,7 @@@ CHANGESE # add (delete) a way to it, which contains a point at (3,3) with_controller(WaysController.new) do - xml = update_changeset(way.to_xml, changeset_id) + xml = update_changeset(xml_for_way(way), changeset_id) put :delete, :params => { :id => way.id }, :body => xml.to_s assert_response :success, "Couldn't delete a way." end @@@ -1501,57 -1497,6 +1497,6 @@@ 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 +1874,6 @@@ 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)