From 947a41edee95df9e75cce0452277e2a00a8b5fa5 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Sun, 24 Feb 2019 12:38:09 +0100 Subject: [PATCH] Move the api methods from changeset_comments_controller into the api namespaced controller --- .../api/changeset_comments_controller.rb | 85 ++++++ .../changeset_comments_controller.rb | 84 +----- config/routes.rb | 6 +- .../api/changeset_comments_controller_test.rb | 254 ++++++++++++++++++ .../changeset_comments_controller_test.rb | 244 ----------------- 5 files changed, 345 insertions(+), 328 deletions(-) create mode 100644 app/controllers/api/changeset_comments_controller.rb create mode 100644 test/controllers/api/changeset_comments_controller_test.rb diff --git a/app/controllers/api/changeset_comments_controller.rb b/app/controllers/api/changeset_comments_controller.rb new file mode 100644 index 000000000..6093f529e --- /dev/null +++ b/app/controllers/api/changeset_comments_controller.rb @@ -0,0 +1,85 @@ +module Api + class ChangesetCommentsController < ApplicationController + skip_before_action :verify_authenticity_token + before_action :authorize + before_action :api_deny_access_handler + + authorize_resource + + before_action :require_public_data, :only => [:create] + before_action :check_api_writable + before_action :check_api_readable, :except => [:create] + around_action :api_call_handle_error + around_action :api_call_timeout + + ## + # Add a comment to a changeset + def create + # Check the arguments are sane + raise OSM::APIBadUserInput, "No id was given" unless params[:id] + raise OSM::APIBadUserInput, "No text was given" if params[:text].blank? + + # Extract the arguments + id = params[:id].to_i + body = params[:text] + + # Find the changeset and check it is valid + changeset = Changeset.find(id) + raise OSM::APIChangesetNotYetClosedError, changeset if changeset.is_open? + + # Add a comment to the changeset + comment = changeset.comments.create(:changeset => changeset, + :body => body, + :author => current_user) + + # Notify current subscribers of the new comment + changeset.subscribers.visible.each do |user| + Notifier.changeset_comment_notification(comment, user).deliver_later if current_user != user + end + + # Add the commenter to the subscribers if necessary + changeset.subscribers << current_user unless changeset.subscribers.exists?(current_user.id) + + # Return a copy of the updated changeset + render :xml => changeset.to_xml.to_s + end + + ## + # Sets visible flag on comment to false + def destroy + # Check the arguments are sane + raise OSM::APIBadUserInput, "No id was given" unless params[:id] + + # Extract the arguments + id = params[:id].to_i + + # Find the changeset + comment = ChangesetComment.find(id) + + # Hide the comment + comment.update(:visible => false) + + # Return a copy of the updated changeset + render :xml => comment.changeset.to_xml.to_s + end + + ## + # Sets visible flag on comment to true + def restore + # Check the arguments are sane + raise OSM::APIBadUserInput, "No id was given" unless params[:id] + + # Extract the arguments + id = params[:id].to_i + + # Find the changeset + comment = ChangesetComment.find(id) + + # Unhide the comment + comment.update(:visible => true) + + # Return a copy of the updated changeset + render :xml => comment.changeset.to_xml.to_s + end + end +end diff --git a/app/controllers/changeset_comments_controller.rb b/app/controllers/changeset_comments_controller.rb index a3023af3e..05b28eacf 100644 --- a/app/controllers/changeset_comments_controller.rb +++ b/app/controllers/changeset_comments_controller.rb @@ -1,89 +1,11 @@ class ChangesetCommentsController < ApplicationController - skip_before_action :verify_authenticity_token, :except => [:index] - before_action :authorize_web, :only => [:index] - before_action :set_locale, :only => [:index] - before_action :authorize, :only => [:create, :destroy, :restore] - before_action :api_deny_access_handler, :only => [:create, :destroy, :restore] + before_action :authorize_web + before_action :set_locale authorize_resource - before_action :require_public_data, :only => [:create] - before_action :check_api_writable, :only => [:create, :destroy, :restore] - before_action :check_api_readable, :except => [:create, :index] before_action(:only => [:index]) { |c| c.check_database_readable(true) } - around_action :api_call_handle_error, :except => [:index] - around_action :api_call_timeout, :except => [:index] - around_action :web_timeout, :only => [:index] - - ## - # Add a comment to a changeset - def create - # Check the arguments are sane - raise OSM::APIBadUserInput, "No id was given" unless params[:id] - raise OSM::APIBadUserInput, "No text was given" if params[:text].blank? - - # Extract the arguments - id = params[:id].to_i - body = params[:text] - - # Find the changeset and check it is valid - changeset = Changeset.find(id) - raise OSM::APIChangesetNotYetClosedError, changeset if changeset.is_open? - - # Add a comment to the changeset - comment = changeset.comments.create(:changeset => changeset, - :body => body, - :author => current_user) - - # Notify current subscribers of the new comment - changeset.subscribers.visible.each do |user| - Notifier.changeset_comment_notification(comment, user).deliver_later if current_user != user - end - - # Add the commenter to the subscribers if necessary - changeset.subscribers << current_user unless changeset.subscribers.exists?(current_user.id) - - # Return a copy of the updated changeset - render :xml => changeset.to_xml.to_s - end - - ## - # Sets visible flag on comment to false - def destroy - # Check the arguments are sane - raise OSM::APIBadUserInput, "No id was given" unless params[:id] - - # Extract the arguments - id = params[:id].to_i - - # Find the changeset - comment = ChangesetComment.find(id) - - # Hide the comment - comment.update(:visible => false) - - # Return a copy of the updated changeset - render :xml => comment.changeset.to_xml.to_s - end - - ## - # Sets visible flag on comment to true - def restore - # Check the arguments are sane - raise OSM::APIBadUserInput, "No id was given" unless params[:id] - - # Extract the arguments - id = params[:id].to_i - - # Find the changeset - comment = ChangesetComment.find(id) - - # Unhide the comment - comment.update(:visible => true) - - # Return a copy of the updated changeset - render :xml => comment.changeset.to_xml.to_s - end + around_action :web_timeout ## # Get a feed of recent changeset comments diff --git a/config/routes.rb b/config/routes.rb index 95142a49f..314cefe9e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -18,9 +18,9 @@ OpenStreetMap::Application.routes.draw do put "changeset/:id" => "api/changesets#update", :id => /\d+/ put "changeset/:id/close" => "api/changesets#close", :id => /\d+/ get "changesets" => "api/changesets#query" - post "changeset/:id/comment" => "changeset_comments#create", :as => :changeset_comment, :id => /\d+/ - post "changeset/comment/:id/hide" => "changeset_comments#destroy", :as => :changeset_comment_hide, :id => /\d+/ - post "changeset/comment/:id/unhide" => "changeset_comments#restore", :as => :changeset_comment_unhide, :id => /\d+/ + post "changeset/:id/comment" => "api/changeset_comments#create", :as => :changeset_comment, :id => /\d+/ + post "changeset/comment/:id/hide" => "api/changeset_comments#destroy", :as => :changeset_comment_hide, :id => /\d+/ + post "changeset/comment/:id/unhide" => "api/changeset_comments#restore", :as => :changeset_comment_unhide, :id => /\d+/ put "node/create" => "nodes#create" get "node/:id/ways" => "ways#ways_for_node", :id => /\d+/ diff --git a/test/controllers/api/changeset_comments_controller_test.rb b/test/controllers/api/changeset_comments_controller_test.rb new file mode 100644 index 000000000..24228c28c --- /dev/null +++ b/test/controllers/api/changeset_comments_controller_test.rb @@ -0,0 +1,254 @@ +require "test_helper" + +module Api + class ChangesetCommentsControllerTest < ActionController::TestCase + ## + # test all routes which lead to this controller + def test_routes + assert_routing( + { :path => "/api/0.6/changeset/1/comment", :method => :post }, + { :controller => "api/changeset_comments", :action => "create", :id => "1" } + ) + assert_routing( + { :path => "/api/0.6/changeset/comment/1/hide", :method => :post }, + { :controller => "api/changeset_comments", :action => "destroy", :id => "1" } + ) + assert_routing( + { :path => "/api/0.6/changeset/comment/1/unhide", :method => :post }, + { :controller => "api/changeset_comments", :action => "restore", :id => "1" } + ) + end + + ## + # create comment success + def test_create_comment_success + user = create(:user) + user2 = create(:user) + private_user = create(:user, :data_public => false) + suspended_user = create(:user, :suspended) + deleted_user = create(:user, :deleted) + private_user_closed_changeset = create(:changeset, :closed, :user => private_user) + + basic_authorization user.email, "test" + + assert_difference "ChangesetComment.count", 1 do + assert_no_difference "ActionMailer::Base.deliveries.size" do + perform_enqueued_jobs do + post :create, :params => { :id => private_user_closed_changeset.id, :text => "This is a comment" } + end + end + end + assert_response :success + + changeset = create(:changeset, :closed, :user => private_user) + changeset.subscribers.push(private_user) + changeset.subscribers.push(user) + changeset.subscribers.push(suspended_user) + changeset.subscribers.push(deleted_user) + + assert_difference "ChangesetComment.count", 1 do + assert_difference "ActionMailer::Base.deliveries.size", 1 do + perform_enqueued_jobs do + post :create, :params => { :id => changeset.id, :text => "This is a comment" } + end + end + end + assert_response :success + + email = ActionMailer::Base.deliveries.first + assert_equal 1, email.to.length + assert_equal "[OpenStreetMap] #{user.display_name} has commented on one of your changesets", email.subject + assert_equal private_user.email, email.to.first + + ActionMailer::Base.deliveries.clear + + basic_authorization user2.email, "test" + + assert_difference "ChangesetComment.count", 1 do + assert_difference "ActionMailer::Base.deliveries.size", 2 do + perform_enqueued_jobs do + post :create, :params => { :id => changeset.id, :text => "This is a comment" } + end + end + end + assert_response :success + + email = ActionMailer::Base.deliveries.find { |e| e.to.first == private_user.email } + assert_not_nil email + assert_equal 1, email.to.length + assert_equal "[OpenStreetMap] #{user2.display_name} has commented on one of your changesets", email.subject + + email = ActionMailer::Base.deliveries.find { |e| e.to.first == user.email } + assert_not_nil email + assert_equal 1, email.to.length + assert_equal "[OpenStreetMap] #{user2.display_name} has commented on a changeset you are interested in", email.subject + + ActionMailer::Base.deliveries.clear + end + + ## + # create comment fail + def test_create_comment_fail + # unauthorized + post :create, :params => { :id => create(:changeset, :closed).id, :text => "This is a comment" } + assert_response :unauthorized + + basic_authorization create(:user).email, "test" + + # bad changeset id + assert_no_difference "ChangesetComment.count" do + post :create, :params => { :id => 999111, :text => "This is a comment" } + end + assert_response :not_found + + # not closed changeset + assert_no_difference "ChangesetComment.count" do + post :create, :params => { :id => create(:changeset).id, :text => "This is a comment" } + end + assert_response :conflict + + # no text + assert_no_difference "ChangesetComment.count" do + post :create, :params => { :id => create(:changeset, :closed).id } + end + assert_response :bad_request + + # empty text + assert_no_difference "ChangesetComment.count" do + post :create, :params => { :id => create(:changeset, :closed).id, :text => "" } + end + assert_response :bad_request + end + + ## + # test hide comment fail + def test_destroy_comment_fail + # unauthorized + comment = create(:changeset_comment) + assert_equal true, comment.visible + + post :destroy, :params => { :id => comment.id } + assert_response :unauthorized + assert_equal true, comment.reload.visible + + basic_authorization create(:user).email, "test" + + # not a moderator + post :destroy, :params => { :id => comment.id } + assert_response :forbidden + assert_equal true, comment.reload.visible + + basic_authorization create(:moderator_user).email, "test" + + # bad comment id + post :destroy, :params => { :id => 999111 } + assert_response :not_found + assert_equal true, comment.reload.visible + end + + ## + # test hide comment succes + def test_hide_comment_success + comment = create(:changeset_comment) + assert_equal true, comment.visible + + basic_authorization create(:moderator_user).email, "test" + + post :destroy, :params => { :id => comment.id } + assert_response :success + assert_equal false, comment.reload.visible + end + + ## + # test unhide comment fail + def test_restore_comment_fail + # unauthorized + comment = create(:changeset_comment, :visible => false) + assert_equal false, comment.visible + + post :restore, :params => { :id => comment.id } + assert_response :unauthorized + assert_equal false, comment.reload.visible + + basic_authorization create(:user).email, "test" + + # not a moderator + post :restore, :params => { :id => comment.id } + assert_response :forbidden + assert_equal false, comment.reload.visible + + basic_authorization create(:moderator_user).email, "test" + + # bad comment id + post :restore, :params => { :id => 999111 } + assert_response :not_found + assert_equal false, comment.reload.visible + end + + ## + # test unhide comment succes + def test_unhide_comment_success + comment = create(:changeset_comment, :visible => false) + assert_equal false, comment.visible + + basic_authorization create(:moderator_user).email, "test" + + post :restore, :params => { :id => comment.id } + assert_response :success + assert_equal true, comment.reload.visible + end + + # This test ensures that token capabilities behave correctly for a method that + # requires the terms to have been agreed. + # (This would be better as an integration or system testcase, since the changeset_comment + # create method is simply a stand-in for any method that requires terms agreement. + # But writing oauth tests is hard, and so it's easier to put in a controller test.) + def test_api_write_and_terms_agreed_via_token + user = create(:user, :terms_agreed => nil) + token = create(:access_token, :user => user, :allow_write_api => true) + changeset = create(:changeset, :closed) + + # Hack together an oauth request - an alternative would be to sign the request properly + @request.env["oauth.version"] = 1 + @request.env["oauth.strategies"] = [:token] + @request.env["oauth.token"] = token + + assert_difference "ChangesetComment.count", 0 do + post :create, :params => { :id => changeset.id, :text => "This is a comment" } + end + assert_response :forbidden + + # Try again, after agreement with the terms + user.terms_agreed = Time.now + user.save! + + assert_difference "ChangesetComment.count", 1 do + post :create, :params => { :id => changeset.id, :text => "This is a comment" } + end + assert_response :success + end + + # This test does the same as above, but with basic auth, to similarly test that the + # abilities take into account terms agreement too. + def test_api_write_and_terms_agreed_via_basic_auth + user = create(:user, :terms_agreed => nil) + changeset = create(:changeset, :closed) + + basic_authorization user.email, "test" + + assert_difference "ChangesetComment.count", 0 do + post :create, :params => { :id => changeset.id, :text => "This is a comment" } + end + assert_response :forbidden + + # Try again, after agreement with the terms + user.terms_agreed = Time.now + user.save! + + assert_difference "ChangesetComment.count", 1 do + post :create, :params => { :id => changeset.id, :text => "This is a comment" } + end + assert_response :success + end + end +end diff --git a/test/controllers/changeset_comments_controller_test.rb b/test/controllers/changeset_comments_controller_test.rb index 33ee1deb5..2d13bd92b 100644 --- a/test/controllers/changeset_comments_controller_test.rb +++ b/test/controllers/changeset_comments_controller_test.rb @@ -4,18 +4,6 @@ class ChangesetCommentsControllerTest < ActionController::TestCase ## # test all routes which lead to this controller def test_routes - assert_routing( - { :path => "/api/0.6/changeset/1/comment", :method => :post }, - { :controller => "changeset_comments", :action => "create", :id => "1" } - ) - assert_routing( - { :path => "/api/0.6/changeset/comment/1/hide", :method => :post }, - { :controller => "changeset_comments", :action => "destroy", :id => "1" } - ) - assert_routing( - { :path => "/api/0.6/changeset/comment/1/unhide", :method => :post }, - { :controller => "changeset_comments", :action => "restore", :id => "1" } - ) assert_routing( { :path => "/changeset/1/comments/feed", :method => :get }, { :controller => "changeset_comments", :action => "index", :id => "1", :format => "rss" } @@ -26,185 +14,6 @@ class ChangesetCommentsControllerTest < ActionController::TestCase ) end - ## - # create comment success - def test_create_comment_success - user = create(:user) - user2 = create(:user) - private_user = create(:user, :data_public => false) - suspended_user = create(:user, :suspended) - deleted_user = create(:user, :deleted) - private_user_closed_changeset = create(:changeset, :closed, :user => private_user) - - basic_authorization user.email, "test" - - assert_difference "ChangesetComment.count", 1 do - assert_no_difference "ActionMailer::Base.deliveries.size" do - perform_enqueued_jobs do - post :create, :params => { :id => private_user_closed_changeset.id, :text => "This is a comment" } - end - end - end - assert_response :success - - changeset = create(:changeset, :closed, :user => private_user) - changeset.subscribers.push(private_user) - changeset.subscribers.push(user) - changeset.subscribers.push(suspended_user) - changeset.subscribers.push(deleted_user) - - assert_difference "ChangesetComment.count", 1 do - assert_difference "ActionMailer::Base.deliveries.size", 1 do - perform_enqueued_jobs do - post :create, :params => { :id => changeset.id, :text => "This is a comment" } - end - end - end - assert_response :success - - email = ActionMailer::Base.deliveries.first - assert_equal 1, email.to.length - assert_equal "[OpenStreetMap] #{user.display_name} has commented on one of your changesets", email.subject - assert_equal private_user.email, email.to.first - - ActionMailer::Base.deliveries.clear - - basic_authorization user2.email, "test" - - assert_difference "ChangesetComment.count", 1 do - assert_difference "ActionMailer::Base.deliveries.size", 2 do - perform_enqueued_jobs do - post :create, :params => { :id => changeset.id, :text => "This is a comment" } - end - end - end - assert_response :success - - email = ActionMailer::Base.deliveries.find { |e| e.to.first == private_user.email } - assert_not_nil email - assert_equal 1, email.to.length - assert_equal "[OpenStreetMap] #{user2.display_name} has commented on one of your changesets", email.subject - - email = ActionMailer::Base.deliveries.find { |e| e.to.first == user.email } - assert_not_nil email - assert_equal 1, email.to.length - assert_equal "[OpenStreetMap] #{user2.display_name} has commented on a changeset you are interested in", email.subject - - ActionMailer::Base.deliveries.clear - end - - ## - # create comment fail - def test_create_comment_fail - # unauthorized - post :create, :params => { :id => create(:changeset, :closed).id, :text => "This is a comment" } - assert_response :unauthorized - - basic_authorization create(:user).email, "test" - - # bad changeset id - assert_no_difference "ChangesetComment.count" do - post :create, :params => { :id => 999111, :text => "This is a comment" } - end - assert_response :not_found - - # not closed changeset - assert_no_difference "ChangesetComment.count" do - post :create, :params => { :id => create(:changeset).id, :text => "This is a comment" } - end - assert_response :conflict - - # no text - assert_no_difference "ChangesetComment.count" do - post :create, :params => { :id => create(:changeset, :closed).id } - end - assert_response :bad_request - - # empty text - assert_no_difference "ChangesetComment.count" do - post :create, :params => { :id => create(:changeset, :closed).id, :text => "" } - end - assert_response :bad_request - end - - ## - # test hide comment fail - def test_destroy_comment_fail - # unauthorized - comment = create(:changeset_comment) - assert_equal true, comment.visible - - post :destroy, :params => { :id => comment.id } - assert_response :unauthorized - assert_equal true, comment.reload.visible - - basic_authorization create(:user).email, "test" - - # not a moderator - post :destroy, :params => { :id => comment.id } - assert_response :forbidden - assert_equal true, comment.reload.visible - - basic_authorization create(:moderator_user).email, "test" - - # bad comment id - post :destroy, :params => { :id => 999111 } - assert_response :not_found - assert_equal true, comment.reload.visible - end - - ## - # test hide comment succes - def test_hide_comment_success - comment = create(:changeset_comment) - assert_equal true, comment.visible - - basic_authorization create(:moderator_user).email, "test" - - post :destroy, :params => { :id => comment.id } - assert_response :success - assert_equal false, comment.reload.visible - end - - ## - # test unhide comment fail - def test_restore_comment_fail - # unauthorized - comment = create(:changeset_comment, :visible => false) - assert_equal false, comment.visible - - post :restore, :params => { :id => comment.id } - assert_response :unauthorized - assert_equal false, comment.reload.visible - - basic_authorization create(:user).email, "test" - - # not a moderator - post :restore, :params => { :id => comment.id } - assert_response :forbidden - assert_equal false, comment.reload.visible - - basic_authorization create(:moderator_user).email, "test" - - # bad comment id - post :restore, :params => { :id => 999111 } - assert_response :not_found - assert_equal false, comment.reload.visible - end - - ## - # test unhide comment succes - def test_unhide_comment_success - comment = create(:changeset_comment, :visible => false) - assert_equal false, comment.visible - - basic_authorization create(:moderator_user).email, "test" - - post :restore, :params => { :id => comment.id } - assert_response :success - assert_equal true, comment.reload.visible - end - ## # test comments feed def test_feed @@ -248,57 +57,4 @@ class ChangesetCommentsControllerTest < ActionController::TestCase get :index, :params => { :format => "rss", :limit => 100001 } assert_response :bad_request end - - # This test ensures that token capabilities behave correctly for a method that - # requires the terms to have been agreed. - # (This would be better as an integration or system testcase, since the changeset_comment - # create method is simply a stand-in for any method that requires terms agreement. - # But writing oauth tests is hard, and so it's easier to put in a controller test.) - def test_api_write_and_terms_agreed_via_token - user = create(:user, :terms_agreed => nil) - token = create(:access_token, :user => user, :allow_write_api => true) - changeset = create(:changeset, :closed) - - # Hack together an oauth request - an alternative would be to sign the request properly - @request.env["oauth.version"] = 1 - @request.env["oauth.strategies"] = [:token] - @request.env["oauth.token"] = token - - assert_difference "ChangesetComment.count", 0 do - post :create, :params => { :id => changeset.id, :text => "This is a comment" } - end - assert_response :forbidden - - # Try again, after agreement with the terms - user.terms_agreed = Time.now - user.save! - - assert_difference "ChangesetComment.count", 1 do - post :create, :params => { :id => changeset.id, :text => "This is a comment" } - end - assert_response :success - end - - # This test does the same as above, but with basic auth, to similarly test that the - # abilities take into account terms agreement too. - def test_api_write_and_terms_agreed_via_basic_auth - user = create(:user, :terms_agreed => nil) - changeset = create(:changeset, :closed) - - basic_authorization user.email, "test" - - assert_difference "ChangesetComment.count", 0 do - post :create, :params => { :id => changeset.id, :text => "This is a comment" } - end - assert_response :forbidden - - # Try again, after agreement with the terms - user.terms_agreed = Time.now - user.save! - - assert_difference "ChangesetComment.count", 1 do - post :create, :params => { :id => changeset.id, :text => "This is a comment" } - end - assert_response :success - end end -- 2.39.5