From: Tom Hughes Date: Wed, 20 Nov 2019 19:17:29 +0000 (+0000) Subject: Merge remote-tracking branch 'upstream/pull/2434' X-Git-Tag: live~3766 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/81fb26d840767e04d364a0a24d186e1a35ab4b5f?hp=b5d4c262ddf559e8f0053d40514c8d33a1e959ab Merge remote-tracking branch 'upstream/pull/2434' --- diff --git a/app/abilities/api_ability.rb b/app/abilities/api_ability.rb index 6d62ece51..217fe9713 100644 --- a/app/abilities/api_ability.rb +++ b/app/abilities/api_ability.rb @@ -34,7 +34,7 @@ class ApiAbility 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 diff --git a/app/abilities/api_capability.rb b/app/abilities/api_capability.rb index 7d8a13364..64861f1d6 100644 --- a/app/abilities/api_capability.rb +++ b/app/abilities/api_capability.rb @@ -10,8 +10,8 @@ class ApiCapability 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) diff --git a/app/controllers/api/user_preferences_controller.rb b/app/controllers/api/user_preferences_controller.rb index 39e0dff30..83e28bc4e 100644 --- a/app/controllers/api/user_preferences_controller.rb +++ b/app/controllers/api/user_preferences_controller.rb @@ -9,31 +9,22 @@ module Api ## # return all the preferences as an XML document - def read - doc = OSM::API.new.get_xml_doc + def index + @user_preferences = current_user.preferences - prefs = current_user.preferences - - el1 = XML::Node.new "preferences" - - prefs.each do |pref| - el1 << pref.to_xml_node - end - - doc.root << el1 - render :xml => doc.to_s + render :formats => [:xml] end ## # return the value for a single preference - def read_one + def show pref = UserPreference.find([current_user.id, params[:preference_key]]) render :plain => pref.v.to_s end # update the entire set of preferences - def update + def update_all old_preferences = current_user.preferences.each_with_object({}) do |preference, preferences| preferences[preference.k] = preference end @@ -63,7 +54,7 @@ module Api ## # update the value of a single preference - def update_one + def update begin pref = UserPreference.find([current_user.id, params[:preference_key]]) rescue ActiveRecord::RecordNotFound @@ -80,7 +71,7 @@ module Api ## # delete a single preference - def delete_one + def destroy UserPreference.find([current_user.id, params[:preference_key]]).delete render :plain => "" diff --git a/app/models/relation.rb b/app/models/relation.rb index dc2b71ac4..a73d63183 100644 --- a/app/models/relation.rb +++ b/app/models/relation.rb @@ -121,31 +121,6 @@ class Relation < ActiveRecord::Base relation end - def to_xml - doc = OSM::API.new.get_xml_doc - doc.root << to_xml_node - doc - end - - def to_xml_node(changeset_cache = {}, user_display_name_cache = {}) - el = XML::Node.new "relation" - el["id"] = id.to_s - - add_metadata_to_xml_node(el, self, changeset_cache, user_display_name_cache) - - relation_members.each do |member| - member_el = XML::Node.new "member" - member_el["type"] = member.member_type.downcase - member_el["ref"] = member.member_id.to_s - member_el["role"] = member.member_role - el << member_el - end - - add_tags_to_xml_node(el, relation_tags) - - el - end - # FIXME: is this really needed? def members @members ||= relation_members.map do |member| diff --git a/app/models/user_preference.rb b/app/models/user_preference.rb index 583ced3c5..37bab38b0 100644 --- a/app/models/user_preference.rb +++ b/app/models/user_preference.rb @@ -18,13 +18,4 @@ class UserPreference < ActiveRecord::Base validates :user, :presence => true, :associated => true validates :k, :v, :length => 1..255, :characters => true - - # Turn this Node in to an XML Node without the wrapper. - def to_xml_node - el1 = XML::Node.new "preference" - el1["k"] = k - el1["v"] = v - - el1 - end end diff --git a/app/models/way.rb b/app/models/way.rb index b3466213c..3284c47f3 100644 --- a/app/models/way.rb +++ b/app/models/way.rb @@ -106,44 +106,6 @@ class Way < ActiveRecord::Base way end - # Find a way given it's ID, and in a single SQL call also grab its nodes and tags - def to_xml - doc = OSM::API.new.get_xml_doc - doc.root << to_xml_node - doc - end - - def to_xml_node(visible_nodes = nil, changeset_cache = {}, user_display_name_cache = {}) - el = XML::Node.new "way" - el["id"] = id.to_s - - add_metadata_to_xml_node(el, self, changeset_cache, user_display_name_cache) - - # make sure nodes are output in sequence_id order - ordered_nodes = [] - way_nodes.each do |nd| - if visible_nodes - # if there is a list of visible nodes then use that to weed out deleted nodes - ordered_nodes[nd.sequence_id] = nd.node_id.to_s if visible_nodes[nd.node_id] - else - # otherwise, manually go to the db to check things - ordered_nodes[nd.sequence_id] = nd.node_id.to_s if nd.node&.visible? - end - end - - ordered_nodes.each do |nd_id| - next unless nd_id && nd_id != "0" - - node_el = XML::Node.new "nd" - node_el["ref"] = nd_id - el << node_el - end - - add_tags_to_xml_node(el, way_tags) - - el - end - def nds @nds ||= way_nodes.collect(&:node_id) end diff --git a/app/views/api/user_preferences/_user_preference.xml.builder b/app/views/api/user_preferences/_user_preference.xml.builder new file mode 100644 index 000000000..ae830bed3 --- /dev/null +++ b/app/views/api/user_preferences/_user_preference.xml.builder @@ -0,0 +1,6 @@ +attrs = { + "k" => user_preference.k, + "v" => user_preference.v +} + +xml.preference(attrs) diff --git a/app/views/api/user_preferences/index.xml.builder b/app/views/api/user_preferences/index.xml.builder new file mode 100644 index 000000000..0a852b269 --- /dev/null +++ b/app/views/api/user_preferences/index.xml.builder @@ -0,0 +1,7 @@ +xml.instruct! + +xml.osm(OSM::API.new.xml_root_attributes) do |osm| + osm.preferences do |preferences| + preferences << (render(@user_preferences) || "") + end +end diff --git a/config/routes.rb b/config/routes.rb index 58c05da17..d936072d7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -67,11 +67,11 @@ OpenStreetMap::Application.routes.draw do 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+/ diff --git a/db/migrate/20191120140058_remove_nearby_from_users.rb b/db/migrate/20191120140058_remove_nearby_from_users.rb new file mode 100644 index 000000000..7d9383dc6 --- /dev/null +++ b/db/migrate/20191120140058_remove_nearby_from_users.rb @@ -0,0 +1,6 @@ +class RemoveNearbyFromUsers < ActiveRecord::Migration[5.2] + def change + # We've already ignored this column in the model, so it is safe to remove + safety_assured { remove_column :users, :nearby, :integer, :default => 50 } + end +end diff --git a/db/structure.sql b/db/structure.sql index 6370eb95a..78e58d4be 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -1394,7 +1394,6 @@ CREATE TABLE public.users ( home_lat double precision, home_lon double precision, home_zoom smallint DEFAULT 3, - nearby integer DEFAULT 50, pass_salt character varying, email_valid boolean DEFAULT false NOT NULL, new_email character varying, @@ -3137,6 +3136,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20190623093642'), ('20190702193519'), ('20190716173946'), +('20191120140058'), ('21'), ('22'), ('23'), diff --git a/test/abilities/api_capability_test.rb b/test/abilities/api_capability_test.rb index 8d0e682f6..8a98f29e0 100644 --- a/test/abilities/api_capability_test.rb +++ b/test/abilities/api_capability_test.rb @@ -100,33 +100,33 @@ class UserApiCapabilityTest < ApiCapabilityTest test "user preferences" do # a user with no tokens capability = ApiCapability.new nil - [:read, :read_one, :update, :update_one, :delete_one].each do |act| + [:index, :show, :update_all, :update, :destroy].each do |act| assert capability.cannot? act, UserPreference end # A user with empty tokens capability = ApiCapability.new tokens - [:read, :read_one, :update, :update_one, :delete_one].each do |act| + [:index, :show, :update_all, :update, :destroy].each do |act| assert capability.cannot? act, UserPreference end capability = ApiCapability.new tokens(:allow_read_prefs) - [:update, :update_one, :delete_one].each do |act| + [:update_all, :update, :destroy].each do |act| assert capability.cannot? act, UserPreference end - [:read, :read_one].each do |act| + [:index, :show].each do |act| assert capability.can? act, UserPreference end capability = ApiCapability.new tokens(:allow_write_prefs) - [:read, :read_one].each do |act| + [:index, :show].each do |act| assert capability.cannot? act, UserPreference end - [:update, :update_one, :delete_one].each do |act| + [:update_all, :update, :destroy].each do |act| assert capability.can? act, UserPreference end end diff --git a/test/controllers/api/changesets_controller_test.rb b/test/controllers/api/changesets_controller_test.rb index c3c9c5058..a4583a928 100644 --- a/test/controllers/api/changesets_controller_test.rb +++ b/test/controllers/api/changesets_controller_test.rb @@ -458,9 +458,9 @@ CHANGESET 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 << 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 @@ -590,8 +590,8 @@ CHANGESET 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 << 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 @@ -633,8 +633,8 @@ CHANGESET delete = XML::Node.new "delete" diff.root << delete delete["if-unused"] = "" - delete << used_relation.to_xml_node - delete << used_way.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 @@ -1175,7 +1175,7 @@ CHANGESET 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 @@ -1487,7 +1487,7 @@ CHANGESET # 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 diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb index 4d2969026..1b54e4863 100644 --- a/test/controllers/api/relations_controller_test.rb +++ b/test/controllers/api/relations_controller_test.rb @@ -525,28 +525,28 @@ module Api assert_response :forbidden # try to delete with an invalid (closed) changeset - xml = update_changeset(relation.to_xml, + xml = update_changeset(xml_for_relation(relation), private_user_closed_changeset.id) delete :delete, :params => { :id => relation.id }, :body => xml.to_s assert_response :forbidden # try to delete with an invalid (non-existent) changeset - xml = update_changeset(relation.to_xml, 0) + xml = update_changeset(xml_for_relation(relation), 0) delete :delete, :params => { :id => relation.id }, :body => xml.to_s assert_response :forbidden # this won't work because the relation is in-use by another relation - xml = used_relation.to_xml + xml = xml_for_relation(used_relation) delete :delete, :params => { :id => used_relation.id }, :body => xml.to_s assert_response :forbidden # this should work when we provide the appropriate payload... - xml = relation.to_xml + xml = xml_for_relation(relation) delete :delete, :params => { :id => relation.id }, :body => xml.to_s assert_response :forbidden # this won't work since the relation is already deleted - xml = deleted_relation.to_xml + xml = xml_for_relation(deleted_relation) delete :delete, :params => { :id => deleted_relation.id }, :body => xml.to_s assert_response :forbidden @@ -568,36 +568,36 @@ module Api assert_match(/Changeset id is missing/, @response.body) # try to delete with an invalid (closed) changeset - xml = update_changeset(relation.to_xml, + xml = update_changeset(xml_for_relation(relation), closed_changeset.id) delete :delete, :params => { :id => relation.id }, :body => xml.to_s assert_response :conflict # try to delete with an invalid (non-existent) changeset - xml = update_changeset(relation.to_xml, 0) + xml = update_changeset(xml_for_relation(relation), 0) delete :delete, :params => { :id => relation.id }, :body => xml.to_s assert_response :conflict # this won't work because the relation is in a changeset owned by someone else - xml = update_changeset(relation.to_xml, create(:changeset).id) + xml = update_changeset(xml_for_relation(relation), create(:changeset).id) delete :delete, :params => { :id => relation.id }, :body => xml.to_s assert_response :conflict, "shouldn't be able to delete a relation in a changeset owned by someone else (#{@response.body})" # this won't work because the relation in the payload is different to that passed - xml = update_changeset(relation.to_xml, changeset.id) + xml = update_changeset(xml_for_relation(relation), changeset.id) delete :delete, :params => { :id => create(:relation).id }, :body => xml.to_s assert_response :bad_request, "shouldn't be able to delete a relation when payload is different to the url" # this won't work because the relation is in-use by another relation - xml = update_changeset(used_relation.to_xml, changeset.id) + xml = update_changeset(xml_for_relation(used_relation), changeset.id) delete :delete, :params => { :id => used_relation.id }, :body => xml.to_s assert_response :precondition_failed, "shouldn't be able to delete a relation used in a relation (#{@response.body})" assert_equal "Precondition failed: The relation #{used_relation.id} is used in relation #{super_relation.id}.", @response.body # this should work when we provide the appropriate payload... - xml = update_changeset(multi_tag_relation.to_xml, changeset.id) + xml = update_changeset(xml_for_relation(multi_tag_relation), changeset.id) delete :delete, :params => { :id => multi_tag_relation.id }, :body => xml.to_s assert_response :success @@ -607,18 +607,18 @@ module Api "delete request should return a new version number for relation" # this won't work since the relation is already deleted - xml = update_changeset(deleted_relation.to_xml, changeset.id) + xml = update_changeset(xml_for_relation(deleted_relation), changeset.id) delete :delete, :params => { :id => deleted_relation.id }, :body => xml.to_s assert_response :gone # Public visible relation needs to be deleted - xml = update_changeset(super_relation.to_xml, changeset.id) + xml = update_changeset(xml_for_relation(super_relation), changeset.id) delete :delete, :params => { :id => super_relation.id }, :body => xml.to_s assert_response :success # this works now because the relation which was using this one # has been deleted. - xml = update_changeset(used_relation.to_xml, changeset.id) + xml = update_changeset(xml_for_relation(used_relation), changeset.id) delete :delete, :params => { :id => used_relation.id }, :body => xml.to_s assert_response :success, "should be able to delete a relation used in an old relation (#{@response.body})" @@ -643,7 +643,7 @@ module Api # indirectly via the way), so the bbox should be [3,3,5,5]. check_changeset_modify(BoundingBox.new(3, 3, 5, 5)) do |changeset_id| # add a tag to an existing relation - relation_xml = relation.to_xml + relation_xml = xml_for_relation(relation) relation_element = relation_xml.find("//osm/relation").first new_tag = XML::Node.new("tag") new_tag["k"] = "some_new_tag" @@ -675,7 +675,7 @@ module Api [node1, node2, way1, way2].each do |element| bbox = element.bbox.to_unscaled check_changeset_modify(bbox) do |changeset_id| - relation_xml = Relation.find(relation.id).to_xml + relation_xml = xml_for_relation(Relation.find(relation.id)) relation_element = relation_xml.find("//osm/relation").first new_member = XML::Node.new("member") new_member["ref"] = element.id.to_s @@ -710,7 +710,7 @@ module Api check_changeset_modify(BoundingBox.new(5, 5, 5, 5)) do |changeset_id| # remove node 5 (5,5) from an existing relation - relation_xml = relation.to_xml + relation_xml = xml_for_relation(relation) relation_xml .find("//osm/relation/member[@type='node'][@ref='#{node2.id}']") .first.remove! @@ -879,7 +879,7 @@ OSM create(:relation_member, :relation => relation, :member => node2) check_changeset_modify(BoundingBox.new(3, 3, 5, 5)) do |changeset_id| - relation_xml = relation.to_xml + relation_xml = xml_for_relation(relation) relation_xml .find("//osm/relation/member") .each(&:remove!) diff --git a/test/controllers/api/user_preferences_controller_test.rb b/test/controllers/api/user_preferences_controller_test.rb index 99dad9597..ac377848c 100644 --- a/test/controllers/api/user_preferences_controller_test.rb +++ b/test/controllers/api/user_preferences_controller_test.rb @@ -7,38 +7,38 @@ module Api def test_routes assert_routing( { :path => "/api/0.6/user/preferences", :method => :get }, - { :controller => "api/user_preferences", :action => "read" } + { :controller => "api/user_preferences", :action => "index" } ) assert_routing( { :path => "/api/0.6/user/preferences", :method => :put }, - { :controller => "api/user_preferences", :action => "update" } + { :controller => "api/user_preferences", :action => "update_all" } ) assert_routing( { :path => "/api/0.6/user/preferences/key", :method => :get }, - { :controller => "api/user_preferences", :action => "read_one", :preference_key => "key" } + { :controller => "api/user_preferences", :action => "show", :preference_key => "key" } ) assert_routing( { :path => "/api/0.6/user/preferences/key", :method => :put }, - { :controller => "api/user_preferences", :action => "update_one", :preference_key => "key" } + { :controller => "api/user_preferences", :action => "update", :preference_key => "key" } ) assert_routing( { :path => "/api/0.6/user/preferences/key", :method => :delete }, - { :controller => "api/user_preferences", :action => "delete_one", :preference_key => "key" } + { :controller => "api/user_preferences", :action => "destroy", :preference_key => "key" } ) end ## - # test read action - def test_read + # test showing all preferences + def test_index # first try without auth - get :read + get :index assert_response :unauthorized, "should be authenticated" # authenticate as a user with no preferences basic_authorization create(:user).email, "test" # try the read again - get :read + get :index assert_select "osm" do assert_select "preferences", :count => 1 do assert_select "preference", :count => 0 @@ -52,7 +52,7 @@ module Api basic_authorization user.email, "test" # try the read again - get :read + get :index assert_response :success assert_equal "application/xml", @response.content_type assert_select "osm" do @@ -65,39 +65,39 @@ module Api end ## - # test read_one action - def test_read_one + # test showing one preference + def test_show user = create(:user) create(:user_preference, :user => user, :k => "key", :v => "value") # try a read without auth - get :read_one, :params => { :preference_key => "key" } + get :show, :params => { :preference_key => "key" } assert_response :unauthorized, "should be authenticated" # authenticate as a user with preferences basic_authorization user.email, "test" # try the read again - get :read_one, :params => { :preference_key => "key" } + get :show, :params => { :preference_key => "key" } assert_response :success assert_equal "text/plain", @response.content_type assert_equal "value", @response.body # try the read again for a non-existent key - get :read_one, :params => { :preference_key => "unknown_key" } + get :show, :params => { :preference_key => "unknown_key" } assert_response :not_found end ## - # test update action - def test_update + # test bulk update action + def test_update_all user = create(:user) create(:user_preference, :user => user, :k => "key", :v => "value") create(:user_preference, :user => user, :k => "some_key", :v => "some_value") # try a put without auth assert_no_difference "UserPreference.count" do - put :update, :body => "" + put :update_all, :body => "" end assert_response :unauthorized, "should be authenticated" assert_equal "value", UserPreference.find([user.id, "key"]).v @@ -111,7 +111,7 @@ module Api # try the put again assert_no_difference "UserPreference.count" do - put :update, :body => "" + put :update_all, :body => "" end assert_response :success assert_equal "text/plain", @response.content_type @@ -124,7 +124,7 @@ module Api # try a put with duplicate keys assert_no_difference "UserPreference.count" do - put :update, :body => "" + put :update_all, :body => "" end assert_response :bad_request assert_equal "text/plain", @response.content_type @@ -133,20 +133,20 @@ module Api # try a put with invalid content assert_no_difference "UserPreference.count" do - put :update, :body => "nonsense" + put :update_all, :body => "nonsense" end assert_response :bad_request end ## - # test update_one action - def test_update_one + # test update action + def test_update user = create(:user) create(:user_preference, :user => user) # try a put without auth assert_no_difference "UserPreference.count" do - put :update_one, :params => { :preference_key => "new_key" }, :body => "new_value" + put :update, :params => { :preference_key => "new_key" }, :body => "new_value" end assert_response :unauthorized, "should be authenticated" assert_raises ActiveRecord::RecordNotFound do @@ -158,7 +158,7 @@ module Api # try adding a new preference assert_difference "UserPreference.count", 1 do - put :update_one, :params => { :preference_key => "new_key" }, :body => "new_value" + put :update, :params => { :preference_key => "new_key" }, :body => "new_value" end assert_response :success assert_equal "text/plain", @response.content_type @@ -167,7 +167,7 @@ module Api # try changing the value of a preference assert_no_difference "UserPreference.count" do - put :update_one, :params => { :preference_key => "new_key" }, :body => "newer_value" + put :update, :params => { :preference_key => "new_key" }, :body => "newer_value" end assert_response :success assert_equal "text/plain", @response.content_type @@ -176,14 +176,14 @@ module Api end ## - # test delete_one action - def test_delete_one + # test destroy action + def test_destroy user = create(:user) create(:user_preference, :user => user, :k => "key", :v => "value") # try a delete without auth assert_no_difference "UserPreference.count" do - delete :delete_one, :params => { :preference_key => "key" } + delete :destroy, :params => { :preference_key => "key" } end assert_response :unauthorized, "should be authenticated" assert_equal "value", UserPreference.find([user.id, "key"]).v @@ -193,7 +193,7 @@ module Api # try the delete again assert_difference "UserPreference.count", -1 do - get :delete_one, :params => { :preference_key => "key" } + get :destroy, :params => { :preference_key => "key" } end assert_response :success assert_equal "text/plain", @response.content_type @@ -204,7 +204,7 @@ module Api # try the delete again for the same key assert_no_difference "UserPreference.count" do - get :delete_one, :params => { :preference_key => "key" } + get :destroy, :params => { :preference_key => "key" } end assert_response :not_found assert_raises ActiveRecord::RecordNotFound do @@ -214,7 +214,7 @@ module Api # Ensure that a valid access token with correct capabilities can be used to # read preferences - def test_read_one_using_token + def test_show_using_token user = create(:user) token = create(:access_token, :user => user, :allow_read_prefs => true) create(:user_preference, :user => user, :k => "key", :v => "value") @@ -224,14 +224,14 @@ module Api @request.env["oauth.strategies"] = [:token] @request.env["oauth.token"] = token - get :read_one, :params => { :preference_key => "key" } + get :show, :params => { :preference_key => "key" } assert_response :success end # Ensure that a valid access token with incorrect capabilities can't be used # to read preferences even, though the owner of that token could read them # by other methods. - def test_read_one_using_token_fail + def test_show_using_token_fail user = create(:user) token = create(:access_token, :user => user, :allow_read_prefs => false) create(:user_preference, :user => user, :k => "key", :v => "value") @@ -239,7 +239,7 @@ module Api @request.env["oauth.strategies"] = [:token] @request.env["oauth.token"] = token - get :read_one, :params => { :preference_key => "key" } + get :show, :params => { :preference_key => "key" } assert_response :forbidden end end diff --git a/test/controllers/api/ways_controller_test.rb b/test/controllers/api/ways_controller_test.rb index c390e8bc2..433b43f27 100644 --- a/test/controllers/api/ways_controller_test.rb +++ b/test/controllers/api/ways_controller_test.rb @@ -290,17 +290,17 @@ module Api assert_response :forbidden # try to delete with an invalid (closed) changeset - xml = update_changeset(private_way.to_xml, private_closed_changeset.id) + xml = update_changeset(xml_for_way(private_way), private_closed_changeset.id) delete :delete, :params => { :id => private_way.id }, :body => xml.to_s assert_response :forbidden # try to delete with an invalid (non-existent) changeset - xml = update_changeset(private_way.to_xml, 0) + xml = update_changeset(xml_for_way(private_way), 0) delete :delete, :params => { :id => private_way.id }, :body => xml.to_s assert_response :forbidden # Now try with a valid changeset - xml = private_way.to_xml + xml = xml_for_way(private_way) delete :delete, :params => { :id => private_way.id }, :body => xml.to_s assert_response :forbidden @@ -311,12 +311,12 @@ module Api # "delete request should return a new version number for way" # this won't work since the way is already deleted - xml = private_deleted_way.to_xml + xml = xml_for_way(private_deleted_way) delete :delete, :params => { :id => private_deleted_way.id }, :body => xml.to_s assert_response :forbidden # this shouldn't work as the way is used in a relation - xml = private_used_way.to_xml + xml = xml_for_way(private_used_way) delete :delete, :params => { :id => private_used_way.id }, :body => xml.to_s assert_response :forbidden, "shouldn't be able to delete a way used in a relation (#{@response.body}), when done by a private user" @@ -339,17 +339,17 @@ module Api assert_response :bad_request # try to delete with an invalid (closed) changeset - xml = update_changeset(way.to_xml, closed_changeset.id) + xml = update_changeset(xml_for_way(way), closed_changeset.id) delete :delete, :params => { :id => way.id }, :body => xml.to_s assert_response :conflict # try to delete with an invalid (non-existent) changeset - xml = update_changeset(way.to_xml, 0) + xml = update_changeset(xml_for_way(way), 0) delete :delete, :params => { :id => way.id }, :body => xml.to_s assert_response :conflict # Now try with a valid changeset - xml = way.to_xml + xml = xml_for_way(way) delete :delete, :params => { :id => way.id }, :body => xml.to_s assert_response :success @@ -360,12 +360,12 @@ module Api "delete request should return a new version number for way" # this won't work since the way is already deleted - xml = deleted_way.to_xml + xml = xml_for_way(deleted_way) delete :delete, :params => { :id => deleted_way.id }, :body => xml.to_s assert_response :gone # this shouldn't work as the way is used in a relation - xml = used_way.to_xml + xml = xml_for_way(used_way) delete :delete, :params => { :id => used_way.id }, :body => xml.to_s assert_response :precondition_failed, "shouldn't be able to delete a way used in a relation (#{@response.body})" @@ -390,7 +390,7 @@ module Api ## First test with no user credentials # try and update a way without authorisation - xml = way.to_xml + xml = xml_for_way(way) put :update, :params => { :id => way.id }, :body => xml.to_s assert_response :unauthorized @@ -402,33 +402,33 @@ module Api ## trying to break changesets # try and update in someone else's changeset - xml = update_changeset(private_way.to_xml, + xml = update_changeset(xml_for_way(private_way), create(:changeset).id) put :update, :params => { :id => private_way.id }, :body => xml.to_s assert_require_public_data "update with other user's changeset should be forbidden when date isn't public" # try and update in a closed changeset - xml = update_changeset(private_way.to_xml, + xml = update_changeset(xml_for_way(private_way), create(:changeset, :closed, :user => private_user).id) put :update, :params => { :id => private_way.id }, :body => xml.to_s assert_require_public_data "update with closed changeset should be forbidden, when data isn't public" # try and update in a non-existant changeset - xml = update_changeset(private_way.to_xml, 0) + xml = update_changeset(xml_for_way(private_way), 0) put :update, :params => { :id => private_way.id }, :body => xml.to_s assert_require_public_data("update with changeset=0 should be forbidden, when data isn't public") ## try and submit invalid updates - xml = xml_replace_node(private_way.to_xml, node.id, 9999) + xml = xml_replace_node(xml_for_way(private_way), node.id, 9999) put :update, :params => { :id => private_way.id }, :body => xml.to_s assert_require_public_data "way with non-existent node should be forbidden, when data isn't public" - xml = xml_replace_node(private_way.to_xml, node.id, create(:node, :deleted).id) + xml = xml_replace_node(xml_for_way(private_way), node.id, create(:node, :deleted).id) put :update, :params => { :id => private_way.id }, :body => xml.to_s assert_require_public_data "way with deleted node should be forbidden, when data isn't public" ## finally, produce a good request which will still not work - xml = private_way.to_xml + xml = xml_for_way(private_way) put :update, :params => { :id => private_way.id }, :body => xml.to_s assert_require_public_data "should have failed with a forbidden when data isn't public" @@ -440,28 +440,28 @@ module Api ## trying to break changesets # try and update in someone else's changeset - xml = update_changeset(way.to_xml, + xml = update_changeset(xml_for_way(way), create(:changeset).id) put :update, :params => { :id => way.id }, :body => xml.to_s assert_response :conflict, "update with other user's changeset should be rejected" # try and update in a closed changeset - xml = update_changeset(way.to_xml, + xml = update_changeset(xml_for_way(way), create(:changeset, :closed, :user => user).id) put :update, :params => { :id => way.id }, :body => xml.to_s assert_response :conflict, "update with closed changeset should be rejected" # try and update in a non-existant changeset - xml = update_changeset(way.to_xml, 0) + xml = update_changeset(xml_for_way(way), 0) put :update, :params => { :id => way.id }, :body => xml.to_s assert_response :conflict, "update with changeset=0 should be rejected" ## try and submit invalid updates - xml = xml_replace_node(way.to_xml, node.id, 9999) + xml = xml_replace_node(xml_for_way(way), node.id, 9999) put :update, :params => { :id => way.id }, :body => xml.to_s assert_response :precondition_failed, "way with non-existent node should be rejected" - xml = xml_replace_node(way.to_xml, node.id, create(:node, :deleted).id) + xml = xml_replace_node(xml_for_way(way), node.id, create(:node, :deleted).id) put :update, :params => { :id => way.id }, :body => xml.to_s assert_response :precondition_failed, "way with deleted node should be rejected" @@ -469,26 +469,26 @@ module Api current_way_version = way.version # try and submit a version behind - xml = xml_attr_rewrite(way.to_xml, + xml = xml_attr_rewrite(xml_for_way(way), "version", current_way_version - 1) put :update, :params => { :id => way.id }, :body => xml.to_s assert_response :conflict, "should have failed on old version number" # try and submit a version ahead - xml = xml_attr_rewrite(way.to_xml, + xml = xml_attr_rewrite(xml_for_way(way), "version", current_way_version + 1) put :update, :params => { :id => way.id }, :body => xml.to_s assert_response :conflict, "should have failed on skipped version number" # try and submit total crap in the version field - xml = xml_attr_rewrite(way.to_xml, + xml = xml_attr_rewrite(xml_for_way(way), "version", "p1r4t3s!") put :update, :params => { :id => way.id }, :body => xml.to_s assert_response :conflict, "should not be able to put 'p1r4at3s!' in the version field" ## try an update with the wrong ID - xml = create(:way).to_xml + xml = xml_for_way(create(:way)) put :update, :params => { :id => way.id }, :body => xml.to_s assert_response :bad_request, "should not be able to update a way with a different ID from the XML" @@ -500,7 +500,7 @@ module Api "should not be able to update a way with non-OSM XML doc." ## finally, produce a good request which should work - xml = way.to_xml + xml = xml_for_way(way) put :update, :params => { :id => way.id }, :body => xml.to_s assert_response :success, "a valid update request failed" end @@ -527,7 +527,7 @@ module Api tag_xml["v"] = "yes" # add the tag into the existing xml - way_xml = private_way.to_xml + way_xml = xml_for_way(private_way) way_xml.find("//osm/way").first << tag_xml # try and upload it @@ -545,7 +545,7 @@ module Api tag_xml["v"] = "yes" # add the tag into the existing xml - way_xml = way.to_xml + way_xml = xml_for_way(way) way_xml.find("//osm/way").first << tag_xml # try and upload it @@ -575,7 +575,7 @@ module Api tag_xml["v"] = private_existing_tag.v # add the tag into the existing xml - way_xml = private_way.to_xml + way_xml = xml_for_way(private_way) way_xml.find("//osm/way").first << tag_xml # try and upload it @@ -593,7 +593,7 @@ module Api tag_xml["v"] = existing_tag.v # add the tag into the existing xml - way_xml = way.to_xml + way_xml = xml_for_way(way) way_xml.find("//osm/way").first << tag_xml # try and upload it @@ -621,7 +621,7 @@ module Api tag_xml["v"] = "foobar" # add the tag into the existing xml - way_xml = private_way.to_xml + way_xml = xml_for_way(private_way) # add two copies of the tag way_xml.find("//osm/way").first << tag_xml.copy(true) << tag_xml @@ -641,7 +641,7 @@ module Api tag_xml["v"] = "foobar" # add the tag into the existing xml - way_xml = way.to_xml + way_xml = xml_for_way(way) # add two copies of the tag way_xml.find("//osm/way").first << tag_xml.copy(true) << tag_xml diff --git a/test/test_helper.rb b/test/test_helper.rb index e4db27d31..05385b4f9 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -193,6 +193,62 @@ module ActiveSupport el end + def xml_for_way(way) + doc = OSM::API.new.get_xml_doc + doc.root << xml_node_for_way(way) + doc + end + + def xml_node_for_way(way) + el = XML::Node.new "way" + el["id"] = way.id.to_s + + OMHelper.add_metadata_to_xml_node(el, way, {}, {}) + + # make sure nodes are output in sequence_id order + ordered_nodes = [] + way.way_nodes.each do |nd| + ordered_nodes[nd.sequence_id] = nd.node_id.to_s if nd.node&.visible? + end + + ordered_nodes.each do |nd_id| + next unless nd_id && nd_id != "0" + + node_el = XML::Node.new "nd" + node_el["ref"] = nd_id + el << node_el + end + + OMHelper.add_tags_to_xml_node(el, way.way_tags) + + el + end + + def xml_for_relation(relation) + doc = OSM::API.new.get_xml_doc + doc.root << xml_node_for_relation(relation) + doc + end + + def xml_node_for_relation(relation) + el = XML::Node.new "relation" + el["id"] = relation.id.to_s + + OMHelper.add_metadata_to_xml_node(el, relation, {}, {}) + + relation.relation_members.each do |member| + member_el = XML::Node.new "member" + member_el["type"] = member.member_type.downcase + member_el["ref"] = member.member_id.to_s + member_el["role"] = member.member_role + el << member_el + end + + OMHelper.add_tags_to_xml_node(el, relation.relation_tags) + + el + end + class OMHelper extend ObjectMetadata end