]> git.openstreetmap.org Git - rails.git/commitdiff
Merge remote-tracking branch 'upstream/pull/2451'
authorTom Hughes <tom@compton.nu>
Wed, 4 Dec 2019 16:08:26 +0000 (16:08 +0000)
committerTom Hughes <tom@compton.nu>
Wed, 4 Dec 2019 16:08:26 +0000 (16:08 +0000)
app/abilities/ability.rb
app/abilities/api_ability.rb
app/abilities/api_capability.rb
app/controllers/api/changesets_controller.rb
app/controllers/traces_controller.rb
app/views/traces/show.html.erb
config/routes.rb
test/controllers/api/changesets_controller_test.rb
test/controllers/traces_controller_test.rb

index c34f357a97a41b2febfbdb7fa1b0a505ded8e3aa..f0cebb380a46ba2ee2572fc54884e0eb8bd93353 100644 (file)
@@ -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?
index 217fe9713c8745f0c666e261389175e883425834..62cd2b17ebbd41ad9408ebcfc5730134991b3506 100644 (file)
@@ -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
index 64861f1d679cad8b962618909e86c31cfc9d2742..beb4d39bf3fa62ba20641899a89220cb91594b71 100644 (file)
@@ -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)
index 5f87324e0a63ddaa112e323005b94e49b306aca5..31601522877dce62e24ee81bbd91a385eededa8a 100644 (file)
@@ -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.
     #
index a0852d2ce2665959714c02c35c5cdc4c03504425..b800d305e03fc495f9bc9a7bdf5752a2ebd83bcf 100644 (file)
@@ -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?
index 0ebbd827fc53c75166f64a107b612fdc3eaf8856..b25ecad5376345b0475d2e4fee1efc80c2b730a3 100644 (file)
@@ -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") } } %>
   </div>
 <% end %>
index d936072d78e7cee4b6d2e12841671c8c14bff41a..992197814ee15b2dad1c7a8bb07e9f14216f4ac7 100644 (file)
@@ -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
index a4583a9289e4c95ec90fc476afc163c94bfed890..be6b84b47aa76e52b3fcbd3c0ff327d628797d14 100644 (file)
@@ -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 => "<osm><changeset/></osm>"
-      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 = "<osm><changeset/></osm>"
-      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 = "<osm><node lon='#{lon}' lat='#{lat}'/></osm>"
-      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 = "<osm><node lon='#{lon}' lat='#{lat}'/></osm>"
-      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 = "<osm><node lon='#{lon}' lat='#{lat}'/></osm>"
-      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 = "<osm><node lon='#{lon}' lat='#{lat}'/></osm>"
-      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)
index 45b0358f5f957538ff2bd0b77ade2cf0a088810a..059242af9957357f460078dd9981a9d6765d4c4d 100644 (file)
@@ -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)