Merge pull request openstreetmap#5293 from AntonKhorev/user-roles-res…

…ourceful-routes

Use resourceful routes for granting/revoking user roles

app/abilities/ability.rb

@@ -68,7 +68,7 @@ def initialize(user)
           can [:index, :show, :resolve, :ignore, :reopen], Issue
           can :create, IssueComment
           can [:set_status, :destroy, :index], User
-          can [:grant, :revoke], UserRole
+          can [:create, :destroy], UserRole
         end
       end
     end

app/controllers/concerns/user_methods.rb

@@ -6,9 +6,10 @@ module UserMethods
   ##
   # ensure that there is a "user" instance variable
   def lookup_user
-    @user = User.active.find_by!(:display_name => params[:display_name])
+    display_name = params[:display_name] || params[:user_display_name]
+    @user = User.active.find_by!(:display_name => display_name)
   rescue ActiveRecord::RecordNotFound
-    render_unknown_user params[:display_name]
+    render_unknown_user display_name
   end
 
   ##

app/controllers/user_roles_controller.rb

@@ -9,15 +9,15 @@ class UserRolesController < ApplicationController
 
   before_action :lookup_user
   before_action :require_valid_role
-  before_action :not_in_role, :only => [:grant]
-  before_action :in_role, :only => [:revoke]
+  before_action :not_in_role, :only => :create
+  before_action :in_role, :only => :destroy
 
-  def grant
+  def create
     @user.roles.create(:role => @role, :granter => current_user)
     redirect_to user_path(@user)
   end
 
-  def revoke
+  def destroy
     # checks that administrator role is not revoked from current user
     if current_user == @user && @role == "administrator"
       flash[:error] = t("user_role.filter.not_revoke_admin_current_user")

app/helpers/user_roles_helper.rb

@@ -7,12 +7,12 @@ def role_icon(user, role)
     if current_user&.administrator?
       if user.role?(role)
         link_to role_icon_svg_tag(role, false, t("users.show.role.revoke.#{role}")),
-                revoke_role_path(user, role),
-                :method => :post,
+                user_role_path(user, role),
+                :method => :delete,
                 :data => { :confirm => t("user_role.revoke.are_you_sure", :name => user.display_name, :role => role) }
       else
         link_to role_icon_svg_tag(role, true, t("users.show.role.grant.#{role}")),
-                grant_role_path(user, role),
+                user_role_path(user, role),
                 :method => :post,
                 :data => { :confirm => t("user_role.grant.are_you_sure", :name => user.display_name, :role => role) }
       end

config/routes.rb

@@ -266,7 +266,9 @@
   post "/diary_comments/:comment/unhide" => "diary_comments#unhide", :comment => /\d+/, :as => :unhide_diary_comment
 
   # user pages
-  resources :users, :path => "user", :param => :display_name, :only => [:show, :destroy]
+  resources :users, :path => "user", :param => :display_name, :only => [:show, :destroy] do
+    resource :role, :controller => "user_roles", :path => "roles/:role", :only => [:create, :destroy]
+  end
   get "/user/:display_name/account", :to => redirect(:path => "/account/edit")
   post "/user/:display_name/set_status" => "users#set_status", :as => :set_status_user
 
@@ -323,9 +325,7 @@
   end
   resources :user_mutes, :only => [:index]
 
-  # roles and banning pages
-  post "/user/:display_name/role/:role/grant" => "user_roles#grant", :as => "grant_role"
-  post "/user/:display_name/role/:role/revoke" => "user_roles#revoke", :as => "revoke_role"
+  # banning pages
   get "/user/:display_name/blocks" => "user_blocks#blocks_on", :as => "user_blocks_on"
   get "/user/:display_name/blocks_by" => "user_blocks#blocks_by", :as => "user_blocks_by"
   get "/blocks/new/:display_name" => "user_blocks#new", :as => "new_user_block"

test/abilities/abilities_test.rb

@@ -45,7 +45,7 @@ class GuestAbilityTest < AbilityTest
   test "user roles permissions for a guest" do
     ability = Ability.new nil
 
-    [:grant, :revoke].each do |action|
+    [:create, :destroy].each do |action|
       assert ability.cannot?(action, UserRole), "should not be able to #{action} UserRoles"
     end
   end
@@ -86,7 +86,7 @@ class ModeratorAbilityTest < AbilityTest
   test "User Roles permissions" do
     ability = Ability.new create(:moderator_user)
 
-    [:grant, :revoke].each do |action|
+    [:create, :destroy].each do |action|
       assert ability.cannot?(action, UserRole), "should not be able to #{action} UserRoles"
     end
 
@@ -159,7 +159,7 @@ class AdministratorAbilityTest < AbilityTest
   test "User Roles permissions for an administrator" do
     ability = Ability.new create(:administrator_user)
 
-    [:grant, :revoke].each do |action|
+    [:create, :destroy].each do |action|
       assert ability.can?(action, UserRole), "should be able to #{action} UserRoles"
     end
   end

test/controllers/user_roles_controller_test.rb

@@ -5,32 +5,32 @@ class UserRolesControllerTest < ActionDispatch::IntegrationTest
   # test all routes which lead to this controller
   def test_routes
     assert_routing(
-      { :path => "/user/username/role/rolename/grant", :method => :post },
-      { :controller => "user_roles", :action => "grant", :display_name => "username", :role => "rolename" }
+      { :path => "/user/username/roles/rolename", :method => :post },
+      { :controller => "user_roles", :action => "create", :user_display_name => "username", :role => "rolename" }
     )
     assert_routing(
-      { :path => "/user/username/role/rolename/revoke", :method => :post },
-      { :controller => "user_roles", :action => "revoke", :display_name => "username", :role => "rolename" }
+      { :path => "/user/username/roles/rolename", :method => :delete },
+      { :controller => "user_roles", :action => "destroy", :user_display_name => "username", :role => "rolename" }
     )
   end
 
   ##
-  # test the grant action
-  def test_grant
+  # test the grant role action
+  def test_update
     target_user = create(:user)
     normal_user = create(:user)
     administrator_user = create(:administrator_user)
     super_user = create(:super_user)
 
     # Granting should fail when not logged in
-    post grant_role_path(target_user, "moderator")
+    post user_role_path(target_user, "moderator")
     assert_response :forbidden
 
     # Login as an unprivileged user
     session_for(normal_user)
 
     # Granting should still fail
-    post grant_role_path(target_user, "moderator")
+    post user_role_path(target_user, "moderator")
     assert_redirected_to :controller => :errors, :action => :forbidden
 
     # Login as an administrator
@@ -39,58 +39,58 @@ def test_grant
     UserRole::ALL_ROLES.each do |role|
       # Granting a role to a non-existent user should fail
       assert_difference "UserRole.count", 0 do
-        post grant_role_path("non_existent_user", role)
+        post user_role_path("non_existent_user", role)
       end
       assert_response :not_found
       assert_template "users/no_such_user"
       assert_select "h1", "The user non_existent_user does not exist"
 
       # Granting a role to a user that already has it should fail
       assert_no_difference "UserRole.count" do
-        post grant_role_path(super_user, role)
+        post user_role_path(super_user, role)
       end
       assert_redirected_to user_path(super_user)
       assert_equal "The user already has role #{role}.", flash[:error]
 
       # Granting a role to a user that doesn't have it should work...
       assert_difference "UserRole.count", 1 do
-        post grant_role_path(target_user, role)
+        post user_role_path(target_user, role)
       end
       assert_redirected_to user_path(target_user)
 
       # ...but trying a second time should fail
       assert_no_difference "UserRole.count" do
-        post grant_role_path(target_user, role)
+        post user_role_path(target_user, role)
       end
       assert_redirected_to user_path(target_user)
       assert_equal "The user already has role #{role}.", flash[:error]
     end
 
     # Granting a non-existent role should fail
     assert_difference "UserRole.count", 0 do
-      post grant_role_path(target_user, "no_such_role")
+      post user_role_path(target_user, "no_such_role")
     end
     assert_redirected_to user_path(target_user)
     assert_equal "The string 'no_such_role' is not a valid role.", flash[:error]
   end
 
   ##
-  # test the revoke action
-  def test_revoke
+  # test the revoke role action
+  def test_destroy
     target_user = create(:user)
     normal_user = create(:user)
     administrator_user = create(:administrator_user)
     super_user = create(:super_user)
 
     # Revoking should fail when not logged in
-    post revoke_role_path(target_user, "moderator")
+    delete user_role_path(target_user, "moderator")
     assert_response :forbidden
 
     # Login as an unprivileged user
     session_for(normal_user)
 
     # Revoking should still fail
-    post revoke_role_path(target_user, "moderator")
+    delete user_role_path(target_user, "moderator")
     assert_redirected_to :controller => :errors, :action => :forbidden
 
     # Login as an administrator
@@ -99,42 +99,42 @@ def test_revoke
     UserRole::ALL_ROLES.each do |role|
       # Removing a role from a non-existent user should fail
       assert_difference "UserRole.count", 0 do
-        post revoke_role_path("non_existent_user", role)
+        delete user_role_path("non_existent_user", role)
       end
       assert_response :not_found
       assert_template "users/no_such_user"
       assert_select "h1", "The user non_existent_user does not exist"
 
       # Removing a role from a user that doesn't have it should fail
       assert_no_difference "UserRole.count" do
-        post revoke_role_path(target_user, role)
+        delete user_role_path(target_user, role)
       end
       assert_redirected_to user_path(target_user)
       assert_equal "The user does not have role #{role}.", flash[:error]
 
       # Removing a role from a user that has it should work...
       assert_difference "UserRole.count", -1 do
-        post revoke_role_path(super_user, role)
+        delete user_role_path(super_user, role)
       end
       assert_redirected_to user_path(super_user)
 
       # ...but trying a second time should fail
       assert_no_difference "UserRole.count" do
-        post revoke_role_path(super_user, role)
+        delete user_role_path(super_user, role)
       end
       assert_redirected_to user_path(super_user)
       assert_equal "The user does not have role #{role}.", flash[:error]
     end
 
     # Revoking a non-existent role should fail
     assert_difference "UserRole.count", 0 do
-      post revoke_role_path(target_user, "no_such_role")
+      delete user_role_path(target_user, "no_such_role")
     end
     assert_redirected_to user_path(target_user)
     assert_equal "The string 'no_such_role' is not a valid role.", flash[:error]
 
     # Revoking administrator role from current user should fail
-    post revoke_role_path(administrator_user, "administrator")
+    delete user_role_path(administrator_user, "administrator")
     assert_redirected_to user_path(administrator_user)
     assert_equal "Cannot revoke administrator role from current user.", flash[:error]
   end

test/helpers/user_roles_helper_test.rb

@@ -31,15 +31,15 @@ def test_role_icon_administrator
     create(:user) do |user|
       icon = role_icon(user, "moderator")
       icon_dom = Rails::Dom::Testing.html_document_fragment.parse(icon)
-      assert_dom icon_dom, "a:root[href='#{grant_role_path(user, 'moderator')}']", :count => 1 do
+      assert_dom icon_dom, "a:root[href='#{user_role_path(user, 'moderator')}'][data-method='post']", :count => 1 do
         assert_dom "> svg", :count => 1 do
           assert_dom "> title", :text => "Grant moderator access"
         end
       end
 
       icon = role_icon(user, "importer")
       icon_dom = Rails::Dom::Testing.html_document_fragment.parse(icon)
-      assert_dom icon_dom, "a:root[href='#{grant_role_path(user, 'importer')}']", :count => 1 do
+      assert_dom icon_dom, "a:root[href='#{user_role_path(user, 'importer')}'][data-method='post']", :count => 1 do
         assert_dom "> svg", :count => 1 do
           assert_dom "> title", :text => "Grant importer access"
         end
@@ -49,15 +49,15 @@ def test_role_icon_administrator
     create(:moderator_user) do |user|
       icon = role_icon(user, "moderator")
       icon_dom = Rails::Dom::Testing.html_document_fragment.parse(icon)
-      assert_dom icon_dom, "a:root[href='#{revoke_role_path(user, 'moderator')}']", :count => 1 do
+      assert_dom icon_dom, "a:root[href='#{user_role_path(user, 'moderator')}'][data-method='delete']", :count => 1 do
         assert_dom "> svg", :count => 1 do
           assert_dom "> title", :text => "Revoke moderator access"
         end
       end
 
       icon = role_icon(user, "importer")
       icon_dom = Rails::Dom::Testing.html_document_fragment.parse(icon)
-      assert_dom icon_dom, "a:root[href='#{grant_role_path(user, 'importer')}']", :count => 1 do
+      assert_dom icon_dom, "a:root[href='#{user_role_path(user, 'importer')}'][data-method='post']", :count => 1 do
         assert_dom "> svg", :count => 1 do
           assert_dom "> title", :text => "Grant importer access"
         end
@@ -67,15 +67,15 @@ def test_role_icon_administrator
     create(:importer_user) do |user|
       icon = role_icon(user, "moderator")
       icon_dom = Rails::Dom::Testing.html_document_fragment.parse(icon)
-      assert_dom icon_dom, "a:root[href='#{grant_role_path(user, 'moderator')}']", :count => 1 do
+      assert_dom icon_dom, "a:root[href='#{user_role_path(user, 'moderator')}'][data-method='post']", :count => 1 do
         assert_dom "> svg", :count => 1 do
           assert_dom "> title", :text => "Grant moderator access"
         end
       end
 
       icon = role_icon(user, "importer")
       icon_dom = Rails::Dom::Testing.html_document_fragment.parse(icon)
-      assert_dom icon_dom, "a:root[href='#{revoke_role_path(user, 'importer')}']", :count => 1 do
+      assert_dom icon_dom, "a:root[href='#{user_role_path(user, 'importer')}'][data-method='delete']", :count => 1 do
         assert_dom "> svg", :count => 1 do
           assert_dom "> title", :text => "Revoke importer access"
         end
@@ -113,13 +113,13 @@ def test_role_icons_administrator
       icons = role_icons(user)
       icons_dom = Rails::Dom::Testing.html_document_fragment.parse(icons)
       assert_dom icons_dom, "a:root", :count => 3
-      assert_dom icons_dom, "a:root[href='#{grant_role_path(user, 'administrator')}']" do
+      assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'administrator')}'][data-method='post']" do
         assert_dom "> svg > title", :text => "Grant administrator access"
       end
-      assert_dom icons_dom, "a:root[href='#{grant_role_path(user, 'moderator')}']" do
+      assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'moderator')}'][data-method='post']" do
         assert_dom "> svg > title", :text => "Grant moderator access"
       end
-      assert_dom icons_dom, "a:root[href='#{grant_role_path(user, 'importer')}']" do
+      assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'importer')}'][data-method='post']" do
         assert_dom "> svg > title", :text => "Grant importer access"
       end
     end
@@ -128,13 +128,13 @@ def test_role_icons_administrator
       icons = role_icons(user)
       icons_dom = Rails::Dom::Testing.html_document_fragment.parse(icons)
       assert_dom icons_dom, "a:root", :count => 3
-      assert_dom icons_dom, "a:root[href='#{grant_role_path(user, 'administrator')}']" do
+      assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'administrator')}'][data-method='post']" do
         assert_dom "> svg > title", :text => "Grant administrator access"
       end
-      assert_dom icons_dom, "a:root[href='#{revoke_role_path(user, 'moderator')}']" do
+      assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'moderator')}'][data-method='delete']" do
         assert_dom "> svg > title", :text => "Revoke moderator access"
       end
-      assert_dom icons_dom, "a:root[href='#{grant_role_path(user, 'importer')}']" do
+      assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'importer')}'][data-method='post']" do
         assert_dom "> svg > title", :text => "Grant importer access"
       end
     end
@@ -143,13 +143,13 @@ def test_role_icons_administrator
       icons = role_icons(user)
       icons_dom = Rails::Dom::Testing.html_document_fragment.parse(icons)
       assert_dom icons_dom, "a:root", :count => 3
-      assert_dom icons_dom, "a:root[href='#{grant_role_path(user, 'administrator')}']" do
+      assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'administrator')}'][data-method='post']" do
         assert_dom "> svg > title", :text => "Grant administrator access"
       end
-      assert_dom icons_dom, "a:root[href='#{grant_role_path(user, 'moderator')}']" do
+      assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'moderator')}'][data-method='post']" do
         assert_dom "> svg > title", :text => "Grant moderator access"
       end
-      assert_dom icons_dom, "a:root[href='#{revoke_role_path(user, 'importer')}']" do
+      assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'importer')}'][data-method='delete']" do
         assert_dom "> svg > title", :text => "Revoke importer access"
       end
     end
@@ -158,13 +158,13 @@ def test_role_icons_administrator
       icons = role_icons(user)
       icons_dom = Rails::Dom::Testing.html_document_fragment.parse(icons)
       assert_dom icons_dom, "a:root", :count => 3
-      assert_dom icons_dom, "a:root[href='#{revoke_role_path(user, 'administrator')}']" do
+      assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'administrator')}'][data-method='delete']" do
         assert_dom "> svg > title", :text => "Revoke administrator access"
       end
-      assert_dom icons_dom, "a:root[href='#{revoke_role_path(user, 'moderator')}']" do
+      assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'moderator')}'][data-method='delete']" do
         assert_dom "> svg > title", :text => "Revoke moderator access"
       end
-      assert_dom icons_dom, "a:root[href='#{revoke_role_path(user, 'importer')}']" do
+      assert_dom icons_dom, "a:root[href='#{user_role_path(user, 'importer')}'][data-method='delete']" do
         assert_dom "> svg > title", :text => "Revoke importer access"
       end
     end