From 990f3eba4069f98a11d98f18b18d0e35bcf295f4 Mon Sep 17 00:00:00 2001 From: Matt Amos Date: Wed, 28 Mar 2012 18:01:21 +0100 Subject: [PATCH 1/1] Altered old_way stuff to be Railsy like old_node is now --- app/controllers/old_node_controller.rb | 2 +- app/controllers/old_way_controller.rb | 73 +++++++++++++-------- app/models/old_way.rb | 5 +- config/routes.rb | 2 +- test/fixtures/ways.yml | 4 +- test/functional/old_node_controller_test.rb | 14 ++-- test/functional/old_way_controller_test.rb | 29 ++++---- 7 files changed, 79 insertions(+), 50 deletions(-) diff --git a/app/controllers/old_node_controller.rb b/app/controllers/old_node_controller.rb index 13f8b00be..02290ffa2 100644 --- a/app/controllers/old_node_controller.rb +++ b/app/controllers/old_node_controller.rb @@ -31,7 +31,7 @@ class OldNodeController < ApplicationController end def version - if @old_node.redacted? and (@user.nil? or not @user.moderator?) and not params[:show_redactions] == "true" + if @old_node.redacted? and not (@user and @user.moderator? and params[:show_redactions] == "true") render :nothing => true, :status => :forbidden else diff --git a/app/controllers/old_way_controller.rb b/app/controllers/old_way_controller.rb index fc7366bb6..e2ee5f0c5 100644 --- a/app/controllers/old_way_controller.rb +++ b/app/controllers/old_way_controller.rb @@ -2,57 +2,74 @@ class OldWayController < ApplicationController require 'xml/libxml' skip_before_filter :verify_authenticity_token + before_filter :setup_user_auth, :only => [ :history, :version ] before_filter :authorize, :only => [ :redact ] + before_filter :authorize_moderator, :only => [ :redact ] before_filter :require_allow_write_api, :only => [ :redact ] before_filter :check_api_readable + before_filter :check_api_writable, :only => [ :redact ] + before_filter :lookup_old_way, :except => [ :history ] after_filter :compress_output around_filter :api_call_handle_error, :api_call_timeout def history - way = Way.find(params[:id]) - - # TODO - maybe a bit heavyweight to do this on every - # call, perhaps try lazy auth. - setup_user_auth + way = Way.find(params[:id].to_i) doc = OSM::API.new.get_xml_doc - way.old_ways.each do |old_way| - unless old_way.redacted? and (@user.nil? or not @user.moderator?) and not params[:show_redactions] == "true" - doc.root << old_way.to_xml_node - end + visible_ways = if @user and @user.moderator? and params[:show_redactions] == "true" + way.old_ways + else + way.old_ways.unredacted + end + + visible_ways.each do |old_way| + doc.root << old_way.to_xml_node end render :text => doc.to_s, :content_type => "text/xml" end def version - if old_way = OldWay.where(:way_id => params[:id], :version => params[:version]).first - # TODO - maybe a bit heavyweight to do this on every - # call, perhaps try lazy auth. - setup_user_auth + if @old_way.redacted? and not (@user and @user.moderator? and params[:show_redactions] == "true") + render :nothing => true, :status => :forbidden + else - if old_way.redacted? and (@user.nil? or not @user.moderator?) and not params[:show_redactions] == "true" - render :nothing => true, :status => :forbidden - else - response.last_modified = old_way.timestamp + response.last_modified = @old_way.timestamp + + doc = OSM::API.new.get_xml_doc + doc.root << @old_way.to_xml_node - doc = OSM::API.new.get_xml_doc - doc.root << old_way.to_xml_node - - render :text => doc.to_s, :content_type => "text/xml" - end - else - render :nothing => true, :status => :not_found + render :text => doc.to_s, :content_type => "text/xml" end end def redact - if @user && @user.moderator? - render :nothing => true - + redaction_id = params['redaction'] + unless redaction_id.nil? + # if a redaction ID was specified, then set this way to + # be redacted in that redaction. (TODO: check that the + # user doing the redaction owns the redaction object too) + redaction = Redaction.find(redaction_id.to_i) + @old_way.redact!(redaction) + else - render :nothing => true, :status => :forbidden + # if no redaction ID was provided, then this is an unredact + # operation. + @old_way.redact!(nil) + end + + # just return an empty 200 OK for success + render :nothing => true + end + + private + + def lookup_old_way + @old_way = OldWay.where(:way_id => params[:id], :version => params[:version]).first + if @old_way.nil? + render :nothing => true, :status => :not_found + return false end end end diff --git a/app/models/old_way.rb b/app/models/old_way.rb index 474c3e02b..30bc12cc2 100644 --- a/app/models/old_way.rb +++ b/app/models/old_way.rb @@ -1,10 +1,13 @@ class OldWay < ActiveRecord::Base include ConsistencyValidations - include Redactable self.table_name = "ways" self.primary_keys = "way_id", "version" + # note this needs to be included after the table name changes, or + # the queries generated by Redactable will use the wrong table name. + include Redactable + belongs_to :changeset belongs_to :redaction belongs_to :current_way, :class_name => "Way", :foreign_key => "way_id" diff --git a/config/routes.rb b/config/routes.rb index c0a31b074..3cfba03b9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -27,8 +27,8 @@ OpenStreetMap::Application.routes.draw do match 'api/0.6/way/:id/history' => 'old_way#history', :via => :get, :id => /\d+/ match 'api/0.6/way/:id/full' => 'way#full', :via => :get, :id => /\d+/ match 'api/0.6/way/:id/relations' => 'relation#relations_for_way', :via => :get, :id => /\d+/ + match 'api/0.6/way/:id/:version/redact' => 'old_way#redact', :via => :post, :version => /\d+/, :id => /\d+/ match 'api/0.6/way/:id/:version' => 'old_way#version', :via => :get, :id => /\d+/, :version => /\d+/ - match 'api/0.6/way/:id/:version/redact' => 'old_way#redact', :via => :put, :version => /\d+/, :id => /\d+/ match 'api/0.6/way/:id' => 'way#read', :via => :get, :id => /\d+/ match 'api/0.6/way/:id' => 'way#update', :via => :put, :id => /\d+/ match 'api/0.6/way/:id' => 'way#delete', :via => :delete, :id => /\d+/ diff --git a/test/fixtures/ways.yml b/test/fixtures/ways.yml index 56f382f88..9a79b977b 100644 --- a/test/fixtures/ways.yml +++ b/test/fixtures/ways.yml @@ -33,7 +33,7 @@ way_with_versions_v2: visible: true version: 2 -way_with_versions: +way_with_versions_v3: way_id: 4 changeset_id: 4 timestamp: 2008-01-01 00:03:00 @@ -89,4 +89,4 @@ way_with_redacted_versions_v4: changeset_id: 4 timestamp: 2008-01-01 00:04:13 visible: true - version: 4 \ No newline at end of file + version: 4 diff --git a/test/functional/old_node_controller_test.rb b/test/functional/old_node_controller_test.rb index ebc387751..899e101f9 100644 --- a/test/functional/old_node_controller_test.rb +++ b/test/functional/old_node_controller_test.rb @@ -222,7 +222,6 @@ class OldNodeControllerTest < ActionController::TestCase # not even to a logged-in user basic_authorization(users(:public_user).email, "test") - get :version, :id => node.node_id, :version => node.version get :history, :id => node.node_id assert_response :success, "Redaction shouldn't have stopped history working." assert_select "osm node[id=#{node.node_id}][version=#{node.version}]", 0, "redacted node #{node.node_id} version #{node.version} shouldn't be present in the history, even when logged in." @@ -238,14 +237,20 @@ class OldNodeControllerTest < ActionController::TestCase do_redact_node(node, redactions(:example)) assert_response :success, "should be OK to redact old version as moderator." - # check moderator can still see the redacted data + # check moderator can still see the redacted data, when passing + # the appropriate flag get :version, :id => node.node_id, :version => node.version - assert_response :success, "After redaction, node should not be gone for moderator." + assert_response :forbidden, "After redaction, node should be gone for moderator, when flag not passed." + get :version, :id => node.node_id, :version => node.version, :show_redactions => 'true' + assert_response :success, "After redaction, node should not be gone for moderator, when flag passed." # and when accessed via history get :history, :id => node.node_id assert_response :success, "Redaction shouldn't have stopped history working." - assert_select "osm node[id=#{node.node_id}][version=#{node.version}]", 1, "node #{node.node_id} version #{node.version} should still be present in the history for moderators." + assert_select "osm node[id=#{node.node_id}][version=#{node.version}]", 0, "node #{node.node_id} version #{node.version} should not be present in the history for moderators when not passing flag." + get :history, :id => node.node_id, :show_redactions => 'true' + assert_response :success, "Redaction shouldn't have stopped history working." + assert_select "osm node[id=#{node.node_id}][version=#{node.version}]", 1, "node #{node.node_id} version #{node.version} should still be present in the history for moderators when passing flag." end # testing that if the moderator drops auth, he can't see the @@ -265,7 +270,6 @@ class OldNodeControllerTest < ActionController::TestCase assert_response :forbidden, "Redacted node shouldn't be visible via the version API." # and when accessed via history - get :version, :id => node.node_id, :version => node.version get :history, :id => node.node_id assert_response :success, "Redaction shouldn't have stopped history working." assert_select "osm node[id=#{node.node_id}][version=#{node.version}]", 0, "redacted node #{node.node_id} version #{node.version} shouldn't be present in the history." diff --git a/test/functional/old_way_controller_test.rb b/test/functional/old_way_controller_test.rb index e79d7b480..84f0e04b0 100644 --- a/test/functional/old_way_controller_test.rb +++ b/test/functional/old_way_controller_test.rb @@ -60,8 +60,8 @@ class OldWayControllerTest < ActionController::TestCase # test the redaction of an old version of a way, while not being # authorised. def test_redact_way_unauthorised - do_redact_way(ways(:way_with_versions), - redactions(:example)) + do_redact_way(ways(:way_with_versions_v3), + redactions(:example)) assert_response :unauthorized, "should need to be authenticated to redact." end @@ -71,8 +71,8 @@ class OldWayControllerTest < ActionController::TestCase def test_redact_way_normal_user basic_authorization(users(:public_user).email, "test") - do_redact_way(ways(:way_with_versions), - redactions(:example)) + do_redact_way(ways(:way_with_versions_v3), + redactions(:example)) assert_response :forbidden, "should need to be moderator to redact." end @@ -84,7 +84,7 @@ class OldWayControllerTest < ActionController::TestCase do_redact_way(ways(:way_with_versions_v4), redactions(:example)) - assert_response :forbidden, "shouldn't be OK to redact current version as moderator." + assert_response :bad_request, "shouldn't be OK to redact current version as moderator." end ## @@ -123,26 +123,32 @@ class OldWayControllerTest < ActionController::TestCase # test the redaction of an old version of a way, while being # authorised as a moderator. def test_redact_way_moderator - way = ways(:way_with_versions) + way = ways(:way_with_versions_v3) basic_authorization(users(:moderator_user).email, "test") do_redact_way(way, redactions(:example)) assert_response :success, "should be OK to redact old version as moderator." - # check moderator can still see the redacted data + # check moderator can still see the redacted data, when passing + # the appropriate flag get :version, :id => way.way_id, :version => way.version - assert_response :success, "After redaction, node should not be gone for moderator." + assert_response :forbidden, "After redaction, node should be gone for moderator, when flag not passed." + get :version, :id => way.way_id, :version => way.version, :show_redactions => 'true' + assert_response :success, "After redaction, node should not be gone for moderator, when flag passed." # and when accessed via history get :history, :id => way.way_id assert_response :success, "Redaction shouldn't have stopped history working." - assert_select "osm way[id=#{way.way_id}][version=#{way.version}]", 1, "way #{way.way_id} version #{way.version} should still be present in the history for moderators." + assert_select "osm way[id=#{way.way_id}][version=#{way.version}]", 0, "way #{way.way_id} version #{way.version} should not be present in the history for moderators when not passing flag." + get :history, :id => way.way_id, :show_redactions => 'true' + assert_response :success, "Redaction shouldn't have stopped history working." + assert_select "osm way[id=#{way.way_id}][version=#{way.version}]", 1, "way #{way.way_id} version #{way.version} should still be present in the history for moderators when passing flag." end # testing that if the moderator drops auth, he can't see the # redacted stuff any more. def test_redact_way_is_redacted - way = ways(:way_with_versions) + way = ways(:way_with_versions_v3) basic_authorization(users(:moderator_user).email, "test") do_redact_way(way, redactions(:example)) @@ -156,8 +162,7 @@ class OldWayControllerTest < ActionController::TestCase assert_response :forbidden, "Redacted node shouldn't be visible via the version API." # and when accessed via history - get :version, :id => way.node_id, :version => way.version - get :history, :id => way.node_id + get :history, :id => way.way_id assert_response :success, "Redaction shouldn't have stopped history working." assert_select "osm way[id=#{way.way_id}][version=#{way.version}]", 0, "redacted way #{way.way_id} version #{way.version} shouldn't be present in the history." end -- 2.43.2