From a106b1b6a7f78f53d21019442cab9c8607ee626f Mon Sep 17 00:00:00 2001
From: David Heinemeier Hansson <david@hey.com>
Date: Sat, 12 Apr 2025 20:30:47 +0200
Subject: [PATCH] Join the two users controllers but split out role setting

---
 .../roles_controller.rb}                      | 17 ++++----------
 app/controllers/users_controller.rb           | 16 +++++++++++++-
 .../{accounts => }/users/_invite.html.erb     |  0
 app/views/{accounts => }/users/_user.html.erb |  6 ++---
 app/views/{accounts => }/users/index.html.erb |  4 ++--
 config/routes.rb                              |  4 +++-
 .../users/roles_controller_test.rb            | 22 +++++++++++++++++++
 .../{accounts => }/users_controller_test.rb   | 15 ++-----------
 8 files changed, 51 insertions(+), 33 deletions(-)
 rename app/controllers/{accounts/users_controller.rb => users/roles_controller.rb} (50%)
 rename app/views/{accounts => }/users/_invite.html.erb (100%)
 rename app/views/{accounts => }/users/_user.html.erb (78%)
 rename app/views/{accounts => }/users/index.html.erb (86%)
 create mode 100644 test/controllers/users/roles_controller_test.rb
 rename test/controllers/{accounts => }/users_controller_test.rb (55%)

diff --git a/app/controllers/accounts/users_controller.rb b/app/controllers/users/roles_controller.rb
similarity index 50%
rename from app/controllers/accounts/users_controller.rb
rename to app/controllers/users/roles_controller.rb
index 1ee4a15c6..8cffa232b 100644
--- a/app/controllers/accounts/users_controller.rb
+++ b/app/controllers/users/roles_controller.rb
@@ -1,24 +1,15 @@
-class Accounts::UsersController < ApplicationController
-  before_action :set_user, only: %i[ update destroy ]
-  before_action :ensure_permission_to_administer_user, only:  %i[ update destroy ]
-
-  def index
-    @users = User.active
-  end
+class Users::RolesController < ApplicationController
+  before_action :set_user
+  before_action :ensure_permission_to_administer_user
 
   def update
     @user.update(role_params)
     redirect_to users_path
   end
 
-  def destroy
-    @user.deactivate
-    redirect_to users_path
-  end
-
   private
     def set_user
-      @user = User.active.find(params[:id])
+      @user = User.active.find(params[:user_id])
     end
 
     def ensure_permission_to_administer_user
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index f460b4d5c..4069e5aaa 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -1,8 +1,13 @@
 class UsersController < ApplicationController
   require_unauthenticated_access only: %i[ new create ]
 
-  before_action :set_user, only: %i[ show edit update ]
+  before_action :set_user, only: %i[ show edit update destroy ]
   before_action :set_account_from_join_code, only: %i[ new create ]
+  before_action :ensure_permission_to_administer_user, only:  %i[ update destroy ]
+
+  def index
+    @users = User.active
+  end
 
   def new
     @user = User.new
@@ -25,6 +30,11 @@ class UsersController < ApplicationController
     redirect_to @user
   end
 
+  def destroy
+    @user.deactivate
+    redirect_to users_path
+  end
+
   private
     def set_account_from_join_code
       @account = Account.find_by_join_code!(params[:join_code])
@@ -34,6 +44,10 @@ class UsersController < ApplicationController
       @user = User.active.find(params[:id])
     end
 
+    def ensure_permission_to_administer_user
+      head :forbidden unless Current.user.can_administer?(@user)
+    end
+
     def user_params
       params.expect(user: [ :name, :email_address, :password, :avatar ])
     end
diff --git a/app/views/accounts/users/_invite.html.erb b/app/views/users/_invite.html.erb
similarity index 100%
rename from app/views/accounts/users/_invite.html.erb
rename to app/views/users/_invite.html.erb
diff --git a/app/views/accounts/users/_user.html.erb b/app/views/users/_user.html.erb
similarity index 78%
rename from app/views/accounts/users/_user.html.erb
rename to app/views/users/_user.html.erb
index ae9326c48..950df8ccc 100644
--- a/app/views/accounts/users/_user.html.erb
+++ b/app/views/users/_user.html.erb
@@ -2,12 +2,12 @@
   <%= avatar_tag user, class: "flex-item-no-shrink" %>
 
   <strong class="overflow-ellipsis">
-    <%= link_to user.name, user_path(user), class: "txt-ink btn btn--plain" %>
+    <%= link_to user.name, user, class: "txt-ink btn btn--plain" %>
   </strong>
 
   <hr class="separator--horizontal flex-item-grow" style="--border-color: var(--color-subtle-dark); --border-style: dashed" aria-hidden="true">
 
-  <%= form_with model: user, url: account_user_path(user), data: { controller: "form" }, method: :patch do | form | %>
+  <%= form_with model: user, url: user_role_path(user), data: { controller: "form" }, method: :patch do | form | %>
     <label class="btn txt-small flex-item-no-shrink" for="<%= dom_id(user, :role) %>" arial-label="Role: <%= user.admin? ? "Administrator" : "Member" %>">
       <%= icon_tag "crown" %>
       <%= form.check_box :role, { data: { action: "form#submit" }, disabled: !Current.user.can_administer?(user), hidden: true, id: dom_id(user, :role) }, "admin", "member" %>
@@ -15,7 +15,7 @@
   <% end %>
 
   <%# FIXME: Move this Current.user check to a stimulus controller that just checks for admin? or the like we so we can cache user list %>
-  <%= button_to account_user_path(user), method: :delete, class: "btn txt-small btn--negative flex-item-no-shrink",
+  <%= button_to user, method: :delete, class: "btn txt-small btn--negative flex-item-no-shrink",
         disabled: !Current.user.can_administer?(user),
         data: { turbo_confirm: "Are you sure you want to permanently remove this person from the account?" } do %>
     <%= icon_tag "minus" %>
diff --git a/app/views/accounts/users/index.html.erb b/app/views/users/index.html.erb
similarity index 86%
rename from app/views/accounts/users/index.html.erb
rename to app/views/users/index.html.erb
index ff44f5d71..2041accd4 100644
--- a/app/views/accounts/users/index.html.erb
+++ b/app/views/users/index.html.erb
@@ -16,9 +16,9 @@
 <% end %>
 
 <div class="panel borderless center pad fill-shade flex flex-column margin-block">
-  <%= render "accounts/users/invite" %>
+  <%= render "users/invite" %>
 </div>
 
 <div class="panel borderless center pad fill-none flex flex-column gap">
-  <%= render partial: "accounts/users/user", collection: @users %>
+  <%= render @users %>
 </div>
diff --git a/config/routes.rb b/config/routes.rb
index 1e0bede63..6eb05dd50 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -5,7 +5,9 @@ Rails.application.routes.draw do
     resource :join_code, module: :accounts
   end
 
-  resources :users
+  resources :users do
+    resource :role, module: :users
+  end
 
   resources :collections do
     scope module: :collections do
diff --git a/test/controllers/users/roles_controller_test.rb b/test/controllers/users/roles_controller_test.rb
new file mode 100644
index 000000000..37b93e334
--- /dev/null
+++ b/test/controllers/users/roles_controller_test.rb
@@ -0,0 +1,22 @@
+require "test_helper"
+
+class Users::RolesControllerTest < ActionDispatch::IntegrationTest
+  setup do
+    sign_in_as :kevin
+  end
+
+  test "update" do
+    assert_not users(:david).admin?
+
+    put user_role_url(users(:david)), params: { user: { role: "admin" } }
+
+    assert_redirected_to users_path
+    assert users(:david).reload.admin?
+  end
+
+  test "can't promote to special roles" do
+    assert_no_changes -> { users(:david).reload.role } do
+      put user_role_url(users(:david)), params: { user: { role: "system" } }
+    end
+  end
+end
diff --git a/test/controllers/accounts/users_controller_test.rb b/test/controllers/users_controller_test.rb
similarity index 55%
rename from test/controllers/accounts/users_controller_test.rb
rename to test/controllers/users_controller_test.rb
index 27d12dc61..7afd6f04c 100644
--- a/test/controllers/accounts/users_controller_test.rb
+++ b/test/controllers/users_controller_test.rb
@@ -1,23 +1,12 @@
 require "test_helper"
 
-class Accounts::UsersControllerTest < ActionDispatch::IntegrationTest
+class UsersControllerTest < ActionDispatch::IntegrationTest
   setup do
     sign_in_as :kevin
   end
 
   test "update" do
-    assert_not users(:david).admin?
-
-    put user_url(users(:david)), params: { user: { role: "admin" } }
-
-    assert_redirected_to users_path
-    assert users(:david).reload.admin?
-  end
-
-  test "can't promote to special roles" do
-    assert_no_changes -> { users(:david).reload.role } do
-      put user_url(users(:david)), params: { user: { role: "system" } }
-    end
+    assert true
   end
 
   test "destroy" do