From: Anton Khorev <tony29@yandex.ru>
Date: Mon, 28 Oct 2024 00:44:10 +0000 (+0300)
Subject: Use resourceful routes for granting/revoking user roles
X-Git-Tag: live~13^2
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/b8247478f4f772df8d09a29f3acd383fc6d32cf0?ds=sidebyside;hp=e15a92a30213d87e10e000a240a7dcab66c208b4

Use resourceful routes for granting/revoking user roles
---

diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb
index 9a8f193a1..725ceccb2 100644
--- a/app/abilities/ability.rb
+++ b/app/abilities/ability.rb
@@ -68,7 +68,7 @@ class Ability
           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
diff --git a/app/controllers/concerns/user_methods.rb b/app/controllers/concerns/user_methods.rb
index 28305b5c3..d79ed48d2 100644
--- a/app/controllers/concerns/user_methods.rb
+++ b/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
 
   ##
diff --git a/app/controllers/user_roles_controller.rb b/app/controllers/user_roles_controller.rb
index 469b2c40b..912453be8 100644
--- a/app/controllers/user_roles_controller.rb
+++ b/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")
diff --git a/app/helpers/user_roles_helper.rb b/app/helpers/user_roles_helper.rb
index e839c0ae6..02017bdb9 100644
--- a/app/helpers/user_roles_helper.rb
+++ b/app/helpers/user_roles_helper.rb
@@ -7,12 +7,12 @@ module UserRolesHelper
     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
diff --git a/config/routes.rb b/config/routes.rb
index 22c4ad642..af72c457d 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -266,7 +266,9 @@ OpenStreetMap::Application.routes.draw do
   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 @@ OpenStreetMap::Application.routes.draw do
   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"
diff --git a/test/abilities/abilities_test.rb b/test/abilities/abilities_test.rb
index 59f5928d7..99168375a 100644
--- a/test/abilities/abilities_test.rb
+++ b/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
diff --git a/test/controllers/user_roles_controller_test.rb b/test/controllers/user_roles_controller_test.rb
index 66735ccf5..301a89f56 100644
--- a/test/controllers/user_roles_controller_test.rb
+++ b/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,7 +39,7 @@ class UserRolesControllerTest < ActionDispatch::IntegrationTest
     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"
@@ -47,20 +47,20 @@ class UserRolesControllerTest < ActionDispatch::IntegrationTest
 
       # 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]
@@ -68,29 +68,29 @@ class UserRolesControllerTest < ActionDispatch::IntegrationTest
 
     # 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,7 +99,7 @@ class UserRolesControllerTest < ActionDispatch::IntegrationTest
     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"
@@ -107,20 +107,20 @@ class UserRolesControllerTest < ActionDispatch::IntegrationTest
 
       # 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]
@@ -128,13 +128,13 @@ class UserRolesControllerTest < ActionDispatch::IntegrationTest
 
     # 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
diff --git a/test/helpers/user_roles_helper_test.rb b/test/helpers/user_roles_helper_test.rb
index 41366199c..3b63889a3 100644
--- a/test/helpers/user_roles_helper_test.rb
+++ b/test/helpers/user_roles_helper_test.rb
@@ -31,7 +31,7 @@ class UserRolesHelperTest < ActionView::TestCase
     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
@@ -39,7 +39,7 @@ class UserRolesHelperTest < ActionView::TestCase
 
       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,7 +49,7 @@ class UserRolesHelperTest < ActionView::TestCase
     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
@@ -57,7 +57,7 @@ class UserRolesHelperTest < ActionView::TestCase
 
       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,7 +67,7 @@ class UserRolesHelperTest < ActionView::TestCase
     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
@@ -75,7 +75,7 @@ class UserRolesHelperTest < ActionView::TestCase
 
       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 @@ class UserRolesHelperTest < ActionView::TestCase
       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 @@ class UserRolesHelperTest < ActionView::TestCase
       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 @@ class UserRolesHelperTest < ActionView::TestCase
       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 @@ class UserRolesHelperTest < ActionView::TestCase
       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