cst1/openstreetmap-website
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Rework user#set_status and #delete to avoid GET requests

Browse source 
This renames the delete action to destroy, and starts using resourceful
routing for user actions.
This commit is contained in:
Andy Allan 2020-06-10 11:49:18 +02:00
parent f1c31dac25
commit 600812f6ad
5 changed files with 23 additions and 26 deletions
Download patch file Download diff file Expand all files Collapse all files

2
app/abilities/ability.rb
View file

@ -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

4
app/controllers/users_controller.rb
View file

@ -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

12
app/views/users/show.html.erb
View file

@ -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>

5
config/routes.rb
View file

@ -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"

26
test/controllers/users_controller_test.rb
View file

@ -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