Rework user#set_status and #delete to avoid GET requests

This renames the delete action to destroy, and starts using resourceful
routing for user actions.

diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb
index 5b1d7df68546210f7bacfc35853ff97b177efc33..e72b4e85b64d55e6787cca9604c2dca29e0e3c78 100644 (file)
--- a/app/abilities/ability.rb
+++ b/app/abilities/ability.rb
@@ -56,7 +56,7 @@ class Ability
           can [:hide, :unhide, :hidecomment, :unhidecomment], DiaryEntry
           can [:index, :show, :resolve, :ignore, :reopen], Issue
           can :create, IssueComment
-          can [:set_status, :delete, :index], User
+          can [:set_status, :destroy, :index], User
           can [:grant, :revoke], UserRole
         end
       end

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 4b949fe0735b3953f9f9d66bed922c834166190a..676f8d8f997f97e7816b02fd869fc1816d613bdb 100644 (file)
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -12,7 +12,7 @@ class UsersController < ApplicationController
   before_action :require_self, :only => [:account]
   before_action :check_database_writable, :only => [:new, :account, :confirm, :confirm_email, :lost_password, :reset_password, :go_public]
   before_action :require_cookies, :only => [:new, :login, :confirm]
-  before_action :lookup_user_by_name, :only => [:set_status, :delete]
+  before_action :lookup_user_by_name, :only => [:set_status, :destroy]
   before_action :allow_thirdparty_images, :only => [:show, :account]
 
   def terms
@@ -393,7 +393,7 @@ class UsersController < ApplicationController
 
   ##
   # delete a user, marking them as deleted and removing personal data
-  def delete
+  def destroy
     @user.delete
     redirect_to user_path(:display_name => params[:display_name])
   end

diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb
index 0112261099f7502afec57c82b791bbd15ab0adce..3b330d2944bc87378d5eaac6fc45d64eb79a9df7 100644 (file)
--- a/app/views/users/show.html.erb
+++ b/app/views/users/show.html.erb
@@ -117,30 +117,30 @@
           <% if can? :set_status, User %>
             <% if ["active", "confirmed"].include? @user.status %>
               <li>
-                <%= link_to t(".deactivate_user"), set_status_user_path(:status => "pending", :display_name => @user.display_name), :data => { :confirm => t(".confirm") } %>
+                <%= link_to t(".deactivate_user"), set_status_user_path(:status => "pending", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
               </li>
             <% elsif ["pending"].include? @user.status %>
               <li>
-                <%= link_to t(".activate_user"), set_status_user_path(:status => "active", :display_name => @user.display_name), :data => { :confirm => t(".confirm") } %>
+                <%= link_to t(".activate_user"), set_status_user_path(:status => "active", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
               </li>
             <% end %>
 
             <% if ["active", "suspended"].include? @user.status %>
               <li>
-                <%= link_to t(".confirm_user"), set_status_user_path(:status => "confirmed", :display_name => @user.display_name), :data => { :confirm => t(".confirm") } %>
+                <%= link_to t(".confirm_user"), set_status_user_path(:status => "confirmed", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
               </li>
             <% end %>
               <li>
                 <% if ["pending", "active", "confirmed", "suspended"].include? @user.status %>
-                  <%= link_to t(".hide_user"), set_status_user_path(:status => "deleted", :display_name => @user.display_name), :data => { :confirm => t(".confirm") } %>
+                  <%= link_to t(".hide_user"), set_status_user_path(:status => "deleted", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
               <% else %>
-                <%= link_to t(".unhide_user"), set_status_user_path(:status => "active", :display_name => @user.display_name), :data => { :confirm => t(".confirm") } %>
+                <%= link_to t(".unhide_user"), set_status_user_path(:status => "active", :display_name => @user.display_name), :method => :post, :data => { :confirm => t(".confirm") } %>
               </li>
             <% end %>
           <% end %>
           <% if can? :delete, User %>
             <li>
-              <%= link_to t(".delete_user"), delete_user_path(:display_name => @user.display_name), :data => { :confirm => t(".confirm") } %>
+              <%= link_to t(".delete_user"), user_path(:display_name => @user.display_name), :method => :delete, :data => { :confirm => t(".confirm") } %>
             </li>
           <% end %>
         </ul>

diff --git a/config/routes.rb b/config/routes.rb
index 2744b13236825bf8dbb130f4372b1c114307cf7d..a98a114302d0576d33fecc637056107d647af49b 100644 (file)
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -235,10 +235,9 @@ OpenStreetMap::Application.routes.draw do
   post "/user/:display_name/diary/:id/unsubscribe" => "diary_entries#unsubscribe", :as => :diary_entry_unsubscribe, :id => /\d+/
 
   # user pages
-  get "/user/:display_name" => "users#show", :as => "user"
+  resources :users, :path => "user", :param => :display_name, :only => [:show, :destroy]
   match "/user/:display_name/account" => "users#account", :via => [:get, :post], :as => "user_account"
-  get "/user/:display_name/set_status" => "users#set_status", :as => :set_status_user
-  get "/user/:display_name/delete" => "users#delete", :as => :delete_user
+  post "/user/:display_name/set_status" => "users#set_status", :as => :set_status_user
 
   # friendships
   match "/user/:display_name/make_friend" => "friendships#make_friend", :via => [:get, :post], :as => "make_friend"

diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb
index 94e22f6c3ed9fa90de10a785e3f23446b7bbcc03..91ef7222f40770568fa5c54b1d7b820496e6b09c 100644 (file)
--- a/test/controllers/users_controller_test.rb
+++ b/test/controllers/users_controller_test.rb
@@ -122,12 +122,12 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
     )
 
     assert_routing(
-      { :path => "/user/username/set_status", :method => :get },
+      { :path => "/user/username/set_status", :method => :post },
       { :controller => "users", :action => "set_status", :display_name => "username" }
     )
     assert_routing(
-      { :path => "/user/username/delete", :method => :get },
-      { :controller => "users", :action => "delete", :display_name => "username" }
+      { :path => "/user/username", :method => :delete },
+      { :controller => "users", :action => "destroy", :display_name => "username" }
     )
 
     assert_routing(
@@ -1223,41 +1223,39 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
     user = create(:user)
 
     # Try without logging in
-    get set_status_user_path(user), :params => { :status => "suspended" }
-    assert_response :redirect
-    assert_redirected_to :action => :login, :referer => set_status_user_path(:status => "suspended")
+    post set_status_user_path(user), :params => { :status => "suspended" }
+    assert_response :forbidden
 
     # Now try as a normal user
     session_for(user)
-    get set_status_user_path(user), :params => { :status => "suspended" }
+    post set_status_user_path(user), :params => { :status => "suspended" }
     assert_response :redirect
     assert_redirected_to :controller => :errors, :action => :forbidden
 
     # Finally try as an administrator
     session_for(create(:administrator_user))
-    get set_status_user_path(user), :params => { :status => "suspended" }
+    post set_status_user_path(user), :params => { :status => "suspended" }
     assert_response :redirect
     assert_redirected_to :action => :show, :display_name => user.display_name
     assert_equal "suspended", User.find(user.id).status
   end
 
-  def test_delete
+  def test_destroy
     user = create(:user, :home_lat => 12.1, :home_lon => 12.1, :description => "test")
 
     # Try without logging in
-    get delete_user_path(user), :params => { :status => "suspended" }
-    assert_response :redirect
-    assert_redirected_to :action => :login, :referer => delete_user_path(:status => "suspended")
+    delete user_path(user), :params => { :status => "suspended" }
+    assert_response :forbidden
 
     # Now try as a normal user
     session_for(user)
-    get delete_user_path(user), :params => { :status => "suspended" }
+    delete user_path(user), :params => { :status => "suspended" }
     assert_response :redirect
     assert_redirected_to :controller => :errors, :action => :forbidden
 
     # Finally try as an administrator
     session_for(create(:administrator_user))
-    get delete_user_path(user), :params => { :status => "suspended" }
+    delete user_path(user), :params => { :status => "suspended" }
     assert_response :redirect
     assert_redirected_to :action => :show, :display_name => user.display_name