Merge pull request #334 from basecamp/user-admins

Add admin role and animate admin toggle

app/controllers/accounts/users_controller.rb

@@ -1,14 +1,19 @@
 class Accounts::UsersController < ApplicationController
-  before_action :set_user, only: %i[ destroy ]
-  before_action :ensure_permission_to_remove, only: :destroy
+  before_action :set_user, only: %i[ update destroy ]
+  before_action :ensure_permission_to_administer_user, only:  %i[ update destroy ]
 
   def index
     @users = Current.account.users.active
   end
 
+  def update
+    @user.update(role_params)
+    redirect_to account_users_path
+  end
+
   def destroy
     @user.deactivate
-    redirect_to users_path
+    redirect_to account_users_path
   end
 
   private
@@ -16,7 +21,11 @@ class Accounts::UsersController < ApplicationController
       @user = Current.account.users.active.find(params[:id])
     end
 
-    def ensure_permission_to_remove
-      head :forbidden unless Current.user.can_remove?(@user)
+    def ensure_permission_to_administer_user
+      head :forbidden unless Current.user.can_administer?(@user)
+    end
+
+    def role_params
+      { role: params.require(:user)[:role].presence_in(%w[ member admin ]) || "member" }
     end
 end

app/models/user.rb

@@ -48,10 +48,6 @@ class User < ApplicationRecord
     Current.user == self
   end
 
-  def can_remove?(other)
-    other != self
-  end
-
   private
     def deactived_email_address
       email_address.sub(/@/, "-deactivated-#{SecureRandom.uuid}@")

app/models/user/role.rb

@@ -2,7 +2,7 @@ module User::Role
   extend ActiveSupport::Concern
 
   included do
-    enum :role, %i[ member system ].index_by(&:itself), scopes: false
+    enum :role, %i[ admin member system ].index_by(&:itself), scopes: false
 
     scope :member, -> { where(role: :member) }
     scope :without_system, -> { where.not(role: :system) }
@@ -16,4 +16,8 @@ module User::Role
       end
     end
   end
+
+  def can_administer?(other)
+    admin? && other != self
+  end
 end

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

@@ -7,15 +7,16 @@
 
   <hr class="separator--horizontal flex-item-grow" style="--border-color: var(--color-subtle-dark); --border-style: dashed" aria-hidden="true">
 
-  <label class="btn txt-small flex-item-no-shrink" for="<%= dom_id(user, :role) %>">
-    <span class="for-screen-reader">Role: <%= true ? "Administrator" : "Member" %></span>
-    <%= icon_tag "crown" %>
-    <%= check_box_tag :role, { data: { action: "form#submit" }, hidden: true, id: dom_id(user, :role) }, "administrator", "member" %>
-  </label>
+  <%= form_with model: user, url: account_user_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" %>
+    </label>
+  <% 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 btn--small btn--negative flex-item-no-shrink",
-        disabled: !Current.user.can_remove?(user),
+        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" %>
     <span class="for-screen-reader">Remove <%= user.name %> from the account</span>

test/controllers/accounts/users_controller_test.rb

@@ -0,0 +1,41 @@
+require "test_helper"
+
+class Accounts::UsersControllerTest < ActionDispatch::IntegrationTest
+  setup do
+    sign_in_as :kevin
+  end
+
+  test "update" do
+    assert_not users(:david).admin?
+
+    put account_user_url(users(:david)), params: { user: { role: "admin" } }
+
+    assert_redirected_to account_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 account_user_url(users(:david)), params: { user: { role: "system" } }
+    end
+  end
+
+  test "destroy" do
+    assert_difference -> { User.active.count }, -1 do
+      delete account_user_url(users(:david))
+    end
+
+    assert_redirected_to account_users_path
+    assert_nil User.active.find_by(id: users(:david).id)
+  end
+
+  test "non-admins cannot perform actions" do
+    sign_in_as :jz
+
+    put account_user_url(users(:david)), params: { user: { role: "admin" } }
+    assert_response :forbidden
+
+    delete account_user_url(users(:david))
+    assert_response :forbidden
+  end
+end

test/fixtures/users.yml

@@ -19,4 +19,4 @@ kevin:
   name: Kevin
   email_address: kevin@37signals.com
   password_digest: <%= digest %>
-  role: member
+  role: admin

test/models/user/role_test.rb

@@ -0,0 +1,10 @@
+require "test_helper"
+
+class User::RoleTest < ActiveSupport::TestCase
+  test "can administer others?" do
+    assert users(:kevin).can_administer?(users(:jz))
+
+    assert_not users(:kevin).can_administer?(users(:kevin))
+    assert_not users(:jz).can_administer?(users(:kevin))
+  end
+end

test/test_helpers/session_test_helper.rb

@@ -4,6 +4,7 @@ module SessionTestHelper
   end
 
   def sign_in_as(user)
+    cookies[:session_token] = nil
     user = users(user) unless user.is_a? User
     post session_url, params: { email_address: user.email_address, password: "secret123456" }
     assert cookies[:session_token].present?