From: Tom Hughes Date: Wed, 18 Dec 2024 14:11:51 +0000 (+0000) Subject: Merge remote-tracking branch 'upstream/pull/5391' X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/6094a97ce6297d390e5bbf733f5c2b4cc6b2076c?hp=7d4cc85a312cb79fd1c4b0f22626722bea1d736d Merge remote-tracking branch 'upstream/pull/5391' --- diff --git a/.rubocop.yml b/.rubocop.yml index 8b6ed0180..60d144544 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -70,7 +70,7 @@ Rails/ReflectionClassName: Rails/SkipsModelValidations: Exclude: - 'db/migrate/*.rb' - - 'app/controllers/users_controller.rb' + - 'app/controllers/users/lists_controller.rb' Style/Documentation: Enabled: false diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index 725ceccb2..2214ba621 100644 --- a/app/abilities/ability.rb +++ b/app/abilities/ability.rb @@ -67,7 +67,8 @@ class Ability can [:hide, :unhide], [DiaryEntry, DiaryComment] can [:index, :show, :resolve, :ignore, :reopen], Issue can :create, IssueComment - can [:set_status, :destroy, :index], User + can [:set_status, :destroy], User + can [:show, :update], :users_list can [:create, :destroy], UserRole end end diff --git a/app/controllers/users/lists_controller.rb b/app/controllers/users/lists_controller.rb new file mode 100644 index 000000000..a5cd7203d --- /dev/null +++ b/app/controllers/users/lists_controller.rb @@ -0,0 +1,41 @@ +module Users + class ListsController < ApplicationController + include PaginationMethods + + layout "site" + + before_action :authorize_web + before_action :set_locale + before_action :check_database_readable + + authorize_resource :class => :users_list + + ## + # display a list of users matching specified criteria + def show + @params = params.permit(:status, :ip, :before, :after) + + users = User.all + users = users.where(:status => @params[:status]) if @params[:status] + users = users.where(:creation_address => @params[:ip]) if @params[:ip] + + @users_count = users.limit(501).count + @users_count = I18n.t("count.at_least_pattern", :count => 500) if @users_count > 500 + + @users, @newer_users_id, @older_users_id = get_page_items(users, :limit => 50) + + render :partial => "page" if turbo_frame_request_id == "pagination" + end + + ## + # update status of selected users + def update + ids = params[:user].keys.collect(&:to_i) + + User.where(:id => ids).update_all(:status => "confirmed") if params[:confirm] + User.where(:id => ids).update_all(:status => "deleted") if params[:hide] + + redirect_to url_for(params.permit(:status, :ip, :before, :after)) + end + end +end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 63a83ad1d..471215c92 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -2,7 +2,6 @@ class UsersController < ApplicationController include EmailMethods include SessionMethods include UserMethods - include PaginationMethods layout "site" @@ -21,32 +20,6 @@ class UsersController < ApplicationController allow_thirdparty_images :only => :show allow_social_login :only => :new - ## - # display a list of users matching specified criteria - def index - if request.post? - ids = params[:user].keys.collect(&:to_i) - - User.where(:id => ids).update_all(:status => "confirmed") if params[:confirm] - User.where(:id => ids).update_all(:status => "deleted") if params[:hide] - - redirect_to url_for(:status => params[:status], :ip => params[:ip], :page => params[:page]) - else - @params = params.permit(:status, :ip, :before, :after) - - users = User.all - users = users.where(:status => @params[:status]) if @params[:status] - users = users.where(:creation_address => @params[:ip]) if @params[:ip] - - @users_count = users.limit(501).count - @users_count = I18n.t("count.at_least_pattern", :count => 500) if @users_count > 500 - - @users, @newer_users_id, @older_users_id = get_page_items(users, :limit => 50) - - render :partial => "page" if turbo_frame_request_id == "pagination" - end - end - def show @user = User.find_by(:display_name => params[:display_name]) diff --git a/app/views/users/_page.html.erb b/app/views/users/lists/_page.html.erb similarity index 84% rename from app/views/users/_page.html.erb rename to app/views/users/lists/_page.html.erb index e505474e1..d06516e06 100644 --- a/app/views/users/_page.html.erb +++ b/app/views/users/lists/_page.html.erb @@ -1,8 +1,9 @@ - <%= form_tag do %> + <%= form_tag @params, :method => :put do %>
<%= render "shared/pagination", + :translation_scope => "shared.pagination.users", :newer_id => @newer_users_id, :older_id => @older_users_id %>
@@ -24,12 +25,13 @@ - <%= render @users %> + <%= render :partial => "user", :collection => @users %>
<%= render "shared/pagination", + :translation_scope => "shared.pagination.users", :newer_id => @newer_users_id, :older_id => @older_users_id %>
diff --git a/app/views/users/_user.html.erb b/app/views/users/lists/_user.html.erb similarity index 88% rename from app/views/users/_user.html.erb rename to app/views/users/lists/_user.html.erb index 2fb14b6bd..14216a244 100644 --- a/app/views/users/_user.html.erb +++ b/app/views/users/lists/_user.html.erb @@ -5,12 +5,12 @@

<% if user.creation_address %> - <%= t "users.index.summary_html", + <%= t ".summary_html", :name => link_to(user.display_name, user), :ip_address => link_to(user.creation_address, :ip => user.creation_address), :date => l(user.created_at, :format => :friendly) %> <% else %> - <%= t "users.index.summary_no_ip_html", + <%= t ".summary_no_ip_html", :name => link_to(user.display_name, user), :date => l(user.created_at, :format => :friendly) %> <% end %> diff --git a/app/views/users/index.html.erb b/app/views/users/lists/show.html.erb similarity index 100% rename from app/views/users/index.html.erb rename to app/views/users/lists/show.html.erb diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index d479b1d56..f39224502 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -219,10 +219,10 @@

<%= @user.email %>
<% unless @user.creation_address.nil? -%>
<%= t ".created from" %>
-
<%= link_to @user.creation_address, users_path(:ip => @user.creation_address) %>
+
<%= link_to @user.creation_address, users_list_path(:ip => @user.creation_address) %>
<% end -%>
<%= t ".status" %>
-
<%= link_to @user.status.capitalize, users_path(:status => @user.status) %>
+
<%= link_to @user.status.capitalize, users_list_path(:status => @user.status) %>
<%= t ".spam score" %>
<%= @user.spam_score %>
diff --git a/config/locales/en.yml b/config/locales/en.yml index d24987aef..16bef0bd0 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2843,18 +2843,20 @@ en: report: Report this User go_public: flash success: "All your edits are now public, and you are now allowed to edit." - index: - title: Users - heading: Users - summary_html: "%{name} created from %{ip_address} on %{date}" - summary_no_ip_html: "%{name} created on %{date}" - empty: No matching users found - page: - found_users: - one: "%{count} user found" - other: "%{count} users found" - confirm: Confirm Selected Users - hide: Hide Selected Users + lists: + show: + title: Users + heading: Users + empty: No matching users found + page: + found_users: + one: "%{count} user found" + other: "%{count} users found" + confirm: Confirm Selected Users + hide: Hide Selected Users + user: + summary_html: "%{name} created from %{ip_address} on %{date}" + summary_no_ip_html: "%{name} created on %{date}" suspended: title: Account Suspended heading: Account Suspended diff --git a/config/routes.rb b/config/routes.rb index 4d3c1bf55..89e636409 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -288,8 +288,9 @@ OpenStreetMap::Application.routes.draw do match "/user/:display_name/remove_friend" => "friendships#remove_friend", :via => [:get, :post], :as => "remove_friend" # user lists - match "/users" => "users#index", :via => [:get, :post] - match "/users/:status" => "users#index", :via => [:get, :post] + namespace :users do + resource :list, :path => "(:status)", :only => [:show, :update] + end # geocoder get "/search" => "geocoder#search" diff --git a/test/controllers/users/lists_controller_test.rb b/test/controllers/users/lists_controller_test.rb new file mode 100644 index 000000000..b683107d9 --- /dev/null +++ b/test/controllers/users/lists_controller_test.rb @@ -0,0 +1,222 @@ +require "test_helper" + +module Users + class ListsControllerTest < ActionDispatch::IntegrationTest + ## + # test all routes which lead to this controller + def test_routes + assert_routing( + { :path => "/users", :method => :get }, + { :controller => "users/lists", :action => "show" } + ) + assert_routing( + { :path => "/users", :method => :put }, + { :controller => "users/lists", :action => "update" } + ) + assert_routing( + { :path => "/users/status", :method => :get }, + { :controller => "users/lists", :action => "show", :status => "status" } + ) + assert_routing( + { :path => "/users/status", :method => :put }, + { :controller => "users/lists", :action => "update", :status => "status" } + ) + end + + def test_show + user = create(:user) + moderator_user = create(:moderator_user) + administrator_user = create(:administrator_user) + _suspended_user = create(:user, :suspended) + _ip_user = create(:user, :creation_address => "1.2.3.4") + + # There are now 7 users - the five above, plus two extra "granters" for the + # moderator_user and administrator_user + assert_equal 7, User.count + + # Shouldn't work when not logged in + get users_list_path + assert_redirected_to login_path(:referer => users_list_path) + + session_for(user) + + # Shouldn't work when logged in as a normal user + get users_list_path + assert_redirected_to :controller => "/errors", :action => :forbidden + + session_for(moderator_user) + + # Shouldn't work when logged in as a moderator + get users_list_path + assert_redirected_to :controller => "/errors", :action => :forbidden + + session_for(administrator_user) + + # Note there is a header row, so all row counts are users + 1 + # Should work when logged in as an administrator + get users_list_path + assert_response :success + assert_template :show + assert_select "table#user_list tbody tr", :count => 7 + + # Should be able to limit by status + get users_list_path, :params => { :status => "suspended" } + assert_response :success + assert_template :show + assert_select "table#user_list tbody tr", :count => 1 + + # Should be able to limit by IP address + get users_list_path, :params => { :ip => "1.2.3.4" } + assert_response :success + assert_template :show + assert_select "table#user_list tbody tr", :count => 1 + end + + def test_show_paginated + 1.upto(100).each do |n| + User.create(:display_name => "extra_#{n}", + :email => "extra#{n}@example.com", + :pass_crypt => "extraextra") + end + + session_for(create(:administrator_user)) + + # 100 examples, an administrator, and a granter for the admin. + assert_equal 102, User.count + next_path = users_list_path + + get next_path + assert_response :success + assert_template :show + assert_select "table#user_list tbody tr", :count => 50 + check_no_page_link "Newer Users" + next_path = check_page_link "Older Users" + + get next_path + assert_response :success + assert_template :show + assert_select "table#user_list tbody tr", :count => 50 + check_page_link "Newer Users" + next_path = check_page_link "Older Users" + + get next_path + assert_response :success + assert_template :show + assert_select "table#user_list tbody tr", :count => 2 + check_page_link "Newer Users" + check_no_page_link "Older Users" + end + + def test_show_invalid_paginated + session_for(create(:administrator_user)) + + %w[-1 0 fred].each do |id| + get users_list_path(:before => id) + assert_redirected_to :controller => "/errors", :action => :bad_request + + get users_list_path(:after => id) + assert_redirected_to :controller => "/errors", :action => :bad_request + end + end + + def test_update_confirm + inactive_user = create(:user, :pending) + suspended_user = create(:user, :suspended) + + # Shouldn't work when not logged in + assert_no_difference "User.active.count" do + put users_list_path, :params => { :confirm => 1, :user => { inactive_user.id => 1, suspended_user.id => 1 } } + end + assert_response :forbidden + + assert_equal "pending", inactive_user.reload.status + assert_equal "suspended", suspended_user.reload.status + + session_for(create(:user)) + + # Shouldn't work when logged in as a normal user + assert_no_difference "User.active.count" do + put users_list_path, :params => { :confirm => 1, :user => { inactive_user.id => 1, suspended_user.id => 1 } } + end + assert_redirected_to :controller => "/errors", :action => :forbidden + assert_equal "pending", inactive_user.reload.status + assert_equal "suspended", suspended_user.reload.status + + session_for(create(:moderator_user)) + + # Shouldn't work when logged in as a moderator + assert_no_difference "User.active.count" do + put users_list_path, :params => { :confirm => 1, :user => { inactive_user.id => 1, suspended_user.id => 1 } } + end + assert_redirected_to :controller => "/errors", :action => :forbidden + assert_equal "pending", inactive_user.reload.status + assert_equal "suspended", suspended_user.reload.status + + session_for(create(:administrator_user)) + + # Should work when logged in as an administrator + assert_difference "User.active.count", 2 do + put users_list_path, :params => { :confirm => 1, :user => { inactive_user.id => 1, suspended_user.id => 1 } } + end + assert_redirected_to :action => :show + assert_equal "confirmed", inactive_user.reload.status + assert_equal "confirmed", suspended_user.reload.status + end + + def test_update_hide + normal_user = create(:user) + confirmed_user = create(:user, :confirmed) + + # Shouldn't work when not logged in + assert_no_difference "User.active.count" do + put users_list_path, :params => { :hide => 1, :user => { normal_user.id => 1, confirmed_user.id => 1 } } + end + assert_response :forbidden + + assert_equal "active", normal_user.reload.status + assert_equal "confirmed", confirmed_user.reload.status + + session_for(create(:user)) + + # Shouldn't work when logged in as a normal user + assert_no_difference "User.active.count" do + put users_list_path, :params => { :hide => 1, :user => { normal_user.id => 1, confirmed_user.id => 1 } } + end + assert_redirected_to :controller => "/errors", :action => :forbidden + assert_equal "active", normal_user.reload.status + assert_equal "confirmed", confirmed_user.reload.status + + session_for(create(:moderator_user)) + + # Shouldn't work when logged in as a moderator + assert_no_difference "User.active.count" do + put users_list_path, :params => { :hide => 1, :user => { normal_user.id => 1, confirmed_user.id => 1 } } + end + assert_redirected_to :controller => "/errors", :action => :forbidden + assert_equal "active", normal_user.reload.status + assert_equal "confirmed", confirmed_user.reload.status + + session_for(create(:administrator_user)) + + # Should work when logged in as an administrator + assert_difference "User.active.count", -2 do + put users_list_path, :params => { :hide => 1, :user => { normal_user.id => 1, confirmed_user.id => 1 } } + end + assert_redirected_to :action => :show + assert_equal "deleted", normal_user.reload.status + assert_equal "deleted", confirmed_user.reload.status + end + + private + + def check_no_page_link(name) + assert_select "a.page-link", { :text => /#{Regexp.quote(name)}/, :count => 0 }, "unexpected #{name} page link" + end + + def check_page_link(name) + assert_select "a.page-link", { :text => /#{Regexp.quote(name)}/ }, "missing #{name} page link" do |buttons| + return buttons.first.attributes["href"].value + end + end + end +end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index dda157f01..732745539 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -47,23 +47,6 @@ class UsersControllerTest < ActionDispatch::IntegrationTest { :path => "/user/username", :method => :delete }, { :controller => "users", :action => "destroy", :display_name => "username" } ) - - assert_routing( - { :path => "/users", :method => :get }, - { :controller => "users", :action => "index" } - ) - assert_routing( - { :path => "/users", :method => :post }, - { :controller => "users", :action => "index" } - ) - assert_routing( - { :path => "/users/status", :method => :get }, - { :controller => "users", :action => "index", :status => "status" } - ) - assert_routing( - { :path => "/users/status", :method => :post }, - { :controller => "users", :action => "index", :status => "status" } - ) end # The user creation page loads @@ -471,204 +454,6 @@ class UsersControllerTest < ActionDispatch::IntegrationTest assert_equal "deleted", user.status end - def test_index_get - user = create(:user) - moderator_user = create(:moderator_user) - administrator_user = create(:administrator_user) - _suspended_user = create(:user, :suspended) - _ip_user = create(:user, :creation_address => "1.2.3.4") - - # There are now 7 users - the five above, plus two extra "granters" for the - # moderator_user and administrator_user - assert_equal 7, User.count - - # Shouldn't work when not logged in - get users_path - assert_redirected_to login_path(:referer => users_path) - - session_for(user) - - # Shouldn't work when logged in as a normal user - get users_path - assert_redirected_to :controller => :errors, :action => :forbidden - - session_for(moderator_user) - - # Shouldn't work when logged in as a moderator - get users_path - assert_redirected_to :controller => :errors, :action => :forbidden - - session_for(administrator_user) - - # Note there is a header row, so all row counts are users + 1 - # Should work when logged in as an administrator - get users_path - assert_response :success - assert_template :index - assert_select "table#user_list tbody tr", :count => 7 - - # Should be able to limit by status - get users_path, :params => { :status => "suspended" } - assert_response :success - assert_template :index - assert_select "table#user_list tbody tr", :count => 1 - - # Should be able to limit by IP address - get users_path, :params => { :ip => "1.2.3.4" } - assert_response :success - assert_template :index - assert_select "table#user_list tbody tr", :count => 1 - end - - def test_index_get_paginated - 1.upto(100).each do |n| - User.create(:display_name => "extra_#{n}", - :email => "extra#{n}@example.com", - :pass_crypt => "extraextra") - end - - session_for(create(:administrator_user)) - - # 100 examples, an administrator, and a granter for the admin. - assert_equal 102, User.count - next_path = users_path - - get next_path - assert_response :success - assert_template :index - assert_select "table#user_list tbody tr", :count => 50 - check_no_page_link "Newer Users" - next_path = check_page_link "Older Users" - - get next_path - assert_response :success - assert_template :index - assert_select "table#user_list tbody tr", :count => 50 - check_page_link "Newer Users" - next_path = check_page_link "Older Users" - - get next_path - assert_response :success - assert_template :index - assert_select "table#user_list tbody tr", :count => 2 - check_page_link "Newer Users" - check_no_page_link "Older Users" - end - - def test_index_get_invalid_paginated - session_for(create(:administrator_user)) - - %w[-1 0 fred].each do |id| - get users_path(:before => id) - assert_redirected_to :controller => :errors, :action => :bad_request - - get users_path(:after => id) - assert_redirected_to :controller => :errors, :action => :bad_request - end - end - - private - - def check_no_page_link(name) - assert_select "a.page-link", { :text => /#{Regexp.quote(name)}/, :count => 0 }, "unexpected #{name} page link" - end - - def check_page_link(name) - assert_select "a.page-link", { :text => /#{Regexp.quote(name)}/ }, "missing #{name} page link" do |buttons| - return buttons.first.attributes["href"].value - end - end - - public - - def test_index_post_confirm - inactive_user = create(:user, :pending) - suspended_user = create(:user, :suspended) - - # Shouldn't work when not logged in - assert_no_difference "User.active.count" do - post users_path, :params => { :confirm => 1, :user => { inactive_user.id => 1, suspended_user.id => 1 } } - end - assert_response :forbidden - - assert_equal "pending", inactive_user.reload.status - assert_equal "suspended", suspended_user.reload.status - - session_for(create(:user)) - - # Shouldn't work when logged in as a normal user - assert_no_difference "User.active.count" do - post users_path, :params => { :confirm => 1, :user => { inactive_user.id => 1, suspended_user.id => 1 } } - end - assert_redirected_to :controller => :errors, :action => :forbidden - assert_equal "pending", inactive_user.reload.status - assert_equal "suspended", suspended_user.reload.status - - session_for(create(:moderator_user)) - - # Shouldn't work when logged in as a moderator - assert_no_difference "User.active.count" do - post users_path, :params => { :confirm => 1, :user => { inactive_user.id => 1, suspended_user.id => 1 } } - end - assert_redirected_to :controller => :errors, :action => :forbidden - assert_equal "pending", inactive_user.reload.status - assert_equal "suspended", suspended_user.reload.status - - session_for(create(:administrator_user)) - - # Should work when logged in as an administrator - assert_difference "User.active.count", 2 do - post users_path, :params => { :confirm => 1, :user => { inactive_user.id => 1, suspended_user.id => 1 } } - end - assert_redirected_to :action => :index - assert_equal "confirmed", inactive_user.reload.status - assert_equal "confirmed", suspended_user.reload.status - end - - def test_index_post_hide - normal_user = create(:user) - confirmed_user = create(:user, :confirmed) - - # Shouldn't work when not logged in - assert_no_difference "User.active.count" do - post users_path, :params => { :hide => 1, :user => { normal_user.id => 1, confirmed_user.id => 1 } } - end - assert_response :forbidden - - assert_equal "active", normal_user.reload.status - assert_equal "confirmed", confirmed_user.reload.status - - session_for(create(:user)) - - # Shouldn't work when logged in as a normal user - assert_no_difference "User.active.count" do - post users_path, :params => { :hide => 1, :user => { normal_user.id => 1, confirmed_user.id => 1 } } - end - assert_redirected_to :controller => :errors, :action => :forbidden - assert_equal "active", normal_user.reload.status - assert_equal "confirmed", confirmed_user.reload.status - - session_for(create(:moderator_user)) - - # Shouldn't work when logged in as a moderator - assert_no_difference "User.active.count" do - post users_path, :params => { :hide => 1, :user => { normal_user.id => 1, confirmed_user.id => 1 } } - end - assert_redirected_to :controller => :errors, :action => :forbidden - assert_equal "active", normal_user.reload.status - assert_equal "confirmed", confirmed_user.reload.status - - session_for(create(:administrator_user)) - - # Should work when logged in as an administrator - assert_difference "User.active.count", -2 do - post users_path, :params => { :hide => 1, :user => { normal_user.id => 1, confirmed_user.id => 1 } } - end - assert_redirected_to :action => :index - assert_equal "deleted", normal_user.reload.status - assert_equal "deleted", confirmed_user.reload.status - end - def test_auth_failure_callback get auth_failure_path assert_redirected_to login_path diff --git a/test/system/users_test.rb b/test/system/users_test.rb index 5d5bf56d7..f291a3916 100644 --- a/test/system/users_test.rb +++ b/test/system/users_test.rb @@ -9,7 +9,7 @@ class UsersTest < ApplicationSystemTestCase test "all users can be selected" do create_list(:user, 100) - visit users_path + visit users_list_path assert_css "tbody input[type=checkbox]:checked", :count => 0 assert_css "tbody input[type=checkbox]:not(:checked)", :count => 50