Move admin users list to its own controller

.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

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

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

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])
 

app/views/users/lists/_page.html.erb

@@ -1,8 +1,9 @@
 <turbo-frame id="pagination" target="_top" data-turbo="false">
-  <%= form_tag do %>
+  <%= form_tag @params, :method => :put do %>
     <div class="row">
       <div class="col">
         <%= render "shared/pagination",
+                   :translation_scope => "shared.pagination.users",
                    :newer_id => @newer_users_id,
                    :older_id => @older_users_id %>
       </div>
@@ -24,12 +25,13 @@
           </td>
         </tr>
       </thead>
-      <%= render @users %>
+      <%= render :partial => "user", :collection => @users %>
     </table>
 
     <div class="row">
       <div class="col">
         <%= render "shared/pagination",
+                   :translation_scope => "shared.pagination.users",
                    :newer_id => @newer_users_id,
                    :older_id => @older_users_id %>
       </div>

app/views/users/lists/_user.html.erb

@@ -5,12 +5,12 @@
   <td>
     <p>
       <% 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 %>

app/views/users/show.html.erb

@@ -219,10 +219,10 @@
               <dd class="list-inline-item"><%= @user.email %></dd>
               <% unless @user.creation_address.nil? -%>
                 <dt class="list-inline-item m-0"><%= t ".created from" %></dt>
-                <dd class="list-inline-item"><%= link_to @user.creation_address, users_path(:ip => @user.creation_address) %></dd>
+                <dd class="list-inline-item"><%= link_to @user.creation_address, users_list_path(:ip => @user.creation_address) %></dd>
               <% end -%>
               <dt class="list-inline-item m-0"><%= t ".status" %></dt>
-              <dd class="list-inline-item"><%= link_to @user.status.capitalize, users_path(:status => @user.status) %></dd>
+              <dd class="list-inline-item"><%= link_to @user.status.capitalize, users_list_path(:status => @user.status) %></dd>
               <dt class="list-inline-item m-0"><%= t ".spam score" %></dt>
               <dd class="list-inline-item"><%= @user.spam_score %></dd>
             </dl>

config/locales/en.yml

@@ -2793,11 +2793,10 @@ en:
       report: Report this User
     go_public:
       flash success: "All your edits are now public, and you are now allowed to edit."
-    index:
+    lists:
+      show:
         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:
@@ -2805,6 +2804,9 @@ en:
           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

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"

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

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

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