Merge pull request #7072 from fjordllc/feature/fix-users_role

User.users_roleの修正をおこなった

app/controllers/admin/users_controller.rb

@@ -2,15 +2,16 @@
 
 class Admin::UsersController < AdminController
   before_action :set_user, only: %i[show edit update]
+  ALLOWED_TARGETS = %w[all student_and_trainee inactive hibernated retired graduate adviser mentor trainee year_end_party campaign].freeze
 
   def index
     @direction = params[:direction] || 'desc'
-    @target = params[:target] || 'student_and_trainee'
-    @users = User.with_attached_avatar
-                 .preload(%i[company course])
-                 .order_by_counts(params[:order_by] || 'id', @direction)
-                 .users_role(@target)
-    @emails = User.users_role(@target).pluck(:email)
+    @target = params[:target]
+    user_scope = User.users_role(@target, allowed_targets: ALLOWED_TARGETS, default_target: 'student_and_trainee')
+    @users = user_scope.with_attached_avatar
+                       .preload(:company, :course)
+                       .order_by_counts(params[:order_by] || 'id', @direction)
+    @emails = user_scope.pluck(:email)
   end
 
   def show

app/controllers/api/generations_controller.rb

@@ -6,7 +6,7 @@ class API::GenerationsController < API::BaseController
 
   def show
     generation = params[:id].to_i
-    @users = Generation.new(generation).users.page(params[:page]).per(PAGER_NUMBER)
+    @users = Generation.new(generation).classmates.page(params[:page]).per(PAGER_NUMBER)
   end
 
   def index

app/controllers/api/talks_controller.rb

@@ -1,24 +1,25 @@
 # frozen_string_literal: true
 
 class API::TalksController < API::BaseController
-  TARGETS = %w[all student_and_trainee mentor graduate adviser trainee retired].freeze
+  ALLOWED_TARGETS = %w[all student_and_trainee mentor graduate adviser trainee retired].freeze
   PAGER_NUMBER = 20
 
   def index
     @target = params[:target]
-    @target = 'all' unless TARGETS.include?(@target)
     @talks = Talk.joins(:user)
                  .includes(user: [{ avatar_attachment: :blob }, :discord_profile])
                  .order(updated_at: :desc, id: :asc)
+    users = User.users_role(@target, allowed_targets: ALLOWED_TARGETS, default_target: 'all')
     @talks =
       if params[:search_word]
-        @talks.merge(
-          User.search_by_keywords({ word: params[:search_word] })
-              .unscope(where: :retired_on)
-              .users_role(@target)
-        )
+        # search_by_keywords内では { unretired } というスコープが設定されている
+        # 退会したユーザーも検索対象に含めたいので、unscope(where: :retired_on) で上記のスコープを削除
+        searched_users = users.search_by_keywords(word: params[:search_word]).unscope(where: :retired_on)
+        # もし検索対象が退会したユーザーである場合、searched_usersには退会していないユーザーも含まれているため、retired スコープを設定する
+        searched_users = searched_users.retired if @target == 'retired'
+        @talks.merge(searched_users)
       else
-        @talks.merge(User.users_role(@target))
+        @talks.merge(users)
               .page(params[:page]).per(PAGER_NUMBER)
       end
   end

app/controllers/api/users_controller.rb

@@ -12,24 +12,18 @@ def index
 
     @target = target_allowlist.include?(params[:target]) ? params[:target] : 'student_and_trainee'
 
-    target_users =
-      if @target == 'followings'
-        current_user.followees_list(watch: @watch)
-      elsif @tag
-        User.tagged_with(@tag)
-      elsif @company
-        User.where(company_id: @company).users_role(@target)
-      elsif @target.in? %w[hibernated retired]
-        User.users_role(@target)
+    users = target_users
+    users = users.order(:last_activity_at) if @target == 'inactive'
+    @users =
+      if params[:search_word]
+        search_for_users(@target, users, params[:search_word])
       else
-        User.users_role(@target).unhibernated.unretired
+        users
+          .preload(:company, :avatar_attachment, :course, :tags)
+          .order(updated_at: :desc)
+          .page(params[:page])
+          .per(PAGER_NUMBER)
       end
-
-    @users = target_users.page(params[:page]).per(PAGER_NUMBER)
-                         .preload(:company, :avatar_attachment, :course, :tags)
-                         .order(updated_at: :desc)
-
-    @users = search_for_users(@target, target_users, params[:search_word]) if params[:search_word]
   end
 
   def show; end
@@ -46,8 +40,9 @@ def update
 
   def search_for_users(target, target_users, search_word)
     users = target_users.search_by_keywords({ word: search_word })
-    users = User.search_by_keywords({ word: search_word }).unscope(where: :retired_on).users_role(target) if target == 'retired'
-    users
+    # search_by_keywords内では { unretired } というスコープが設定されている
+    # 退会したユーザーに対しキーワード検索を行う場合は、一旦 unscope(where: :retired_on) で { unretired } スコープを削除し、その後で retired スコープを設定する必要がある
+    target == 'retired' ? users.unscope(where: :retired_on).retired : users
   end
 
   def target_allowlist
@@ -58,6 +53,24 @@ def target_allowlist
     target_allowlist
   end
 
+  def target_users
+    if @target == 'followings'
+      current_user.followees_list(watch: @watch)
+    elsif @tag
+      User.tagged_with(@tag)
+    else
+      users_scope =
+        if @company
+          User.where(company_id: @company)
+        elsif @target.in? %w[hibernated retired]
+          User
+        else
+          User.unhibernated.unretired
+        end
+      users_scope.users_role(@target, allowed_targets: target_allowlist)
+    end
+  end
+
   def set_user
     @user = User.find(params[:id])
   end

app/controllers/companies/users_controller.rb

@@ -1,14 +1,14 @@
 # frozen_string_literal: true
 
 class Companies::UsersController < ApplicationController
-  TARGETS = %w[all student_and_trainee graduate adviser mentor].freeze
+  ALLOWED_TARGETS = %w[all student_and_trainee graduate adviser mentor].freeze
 
   def index
     @target = params[:target]
-    @target = 'student_and_trainee' unless TARGETS.include?(@target)
+    @target = 'student_and_trainee' unless ALLOWED_TARGETS.include?(@target)
     @company = Company.find(params[:company_id])
 
-    target_users = User.users_role(@target)
+    target_users = User.users_role(@target, allowed_targets: ALLOWED_TARGETS)
 
     @users = target_users.with_attached_avatar.where(company: @company).order(updated_at: :desc)
   end

app/controllers/generations_controller.rb

@@ -1,14 +1,14 @@
 # frozen_string_literal: true
 
 class GenerationsController < ApplicationController
-  TARGETS = %w[all trainee adviser graduate mentor retired].freeze
+  ALLOWED_TARGETS = %w[all trainee adviser graduate mentor retired].freeze
 
   def show
     @generation = params[:id].to_i
   end
 
   def index
-    @target = TARGETS.include?(params[:target]) ? params[:target] : TARGETS.first
+    @target = ALLOWED_TARGETS.include?(params[:target]) ? params[:target] : ALLOWED_TARGETS.first
     redirect_to root_path, alert: '管理者としてログインしてください' if @target == 'retired' && !current_user.admin?
   end
 end

app/controllers/talks_controller.rb

@@ -9,7 +9,6 @@ class TalksController < ApplicationController
 
   def index
     @target = params[:target]
-    @target = 'all' unless API::TalksController::TARGETS.include?(@target)
   end
 
   def show; end

app/controllers/users/companies_controller.rb

@@ -1,9 +1,9 @@
 # frozen_string_literal: true
 
 class Users::CompaniesController < ApplicationController
-  TARGETS = %w[all trainee adviser graduate mentor].freeze
+  ALLOWED_TARGETS = %w[all trainee adviser graduate mentor].freeze
 
   def index
-    @target = TARGETS.include?(params[:target]) ? params[:target] : TARGETS.first
+    @target = ALLOWED_TARGETS.include?(params[:target]) ? params[:target] : ALLOWED_TARGETS.first
   end
 end

app/controllers/users_controller.rb

@@ -18,7 +18,8 @@ def index
       elsif params[:tag]
         User.tagged_with(params[:tag])
       else
-        User.users_role(@target)
+        users = User.users_role(@target, allowed_targets: target_allowlist)
+        @target == 'inactive' ? users.order(:last_activity_at) : users
       end
 
     @users = target_users

app/models/generation.rb

@@ -2,6 +2,7 @@
 
 class Generation
   START_YEAR = 2013
+  ALLOWED_TARGETS = %w[all trainee adviser graduate mentor retired].freeze
 
   class << self
     def generations(target)
@@ -34,12 +35,13 @@ def end_date
     (next_generation.start_date - 1).end_of_day
   end
 
-  def users
-    User.with_attached_avatar.same_generations(start_date, end_date)
+  def classmates
+    User.with_attached_avatar.classmates(start_date, end_date)
   end
 
   def target_users(target)
-    target_users = users.users_role(target)
-    target == 'retired' ? target_users : target_users.unretired
+    users = classmates.users_role(target, allowed_targets: ALLOWED_TARGETS)
+    # 退会者は「退会」フィルター時のみ表示させたいため、絞り込みを行う
+    target == 'retired' ? users : users.unretired
   end
 end

app/models/user.rb

@@ -11,6 +11,14 @@ class User < ApplicationRecord
   RESERVED_LOGIN_NAMES = %w[adviser all graduate inactive job_seeking mentor retired student student_and_trainee trainee year_end_party].freeze
   MAX_PERCENTAGE = 100
   DEPRESSED_SIZE = 2
+  ALL_ALLOWED_TARGETS = %w[adviser all campaign graduate hibernated inactive job_seeking mentor retired student_and_trainee trainee year_end_party].freeze
+  # 本来であればtarget = scope名としたいが、歴史的経緯によりtargetとscope名が一致しないものが多数あるため、名前が一致しない場合はこのハッシュを使ってscope名に変換する
+  TARGET_TO_SCOPE = {
+    'student_and_trainee' => :students_and_trainees,
+    'graduate' => :graduated,
+    'adviser' => :advisers,
+    'trainee' => :trainees
+  }.freeze
 
   enum job: {
     student: 0,
@@ -337,7 +345,7 @@ class User < ApplicationRecord
       order(order_by.to_sym => direction.to_sym, created_at: :asc)
     end
   }
-  scope :same_generations, lambda { |start_date, end_date|
+  scope :classmates, lambda { |start_date, end_date|
     where(created_at: start_date..end_date).order(:created_at, :id)
   }
   scope :desc_tagged_with, lambda { |tag_name|
@@ -384,21 +392,14 @@ def announcement_receiver(target)
       end
     end
 
-    def users_role(target)
-      case target
-      when 'student_and_trainee'
-        students_and_trainees
-      when 'graduate'
-        graduated
-      when 'adviser'
-        advisers
-      when 'inactive'
-        inactive.order(:last_activity_at)
-      when 'trainee'
-        trainees
-      else
-        send(target)
-      end
+    # このメソッドはユーザから送信された値をsendに渡すので、悪意のあるコードが実行される危険性がある
+    # そのため、このメソッドを使用する際には安全性の確保のために以下の引数を指定すること
+    # allowed_targets:　呼び出したいscope名に対応するtargetを過不足なく指定した配列。
+    # default_target: targetに不正な値が渡された際、users_roleが返すスコープ名に対応するtargetを指定する。デフォルトでは:noneを指定しているため何も返さない。
+    def users_role(target, allowed_targets: [], default_target: :none)
+      key = (ALL_ALLOWED_TARGETS & allowed_targets).include?(target) ? target : default_target
+      scope_name = TARGET_TO_SCOPE.fetch(key, key)
+      send(scope_name)
     end
 
     def tags

app/views/api/users/companies/_company.json.jbuilder

@@ -6,4 +6,6 @@ json.description company.description
 json.logo_url company.logo_url
 json.users_url company_users_url(company)
 
-json.users company.users.users_role(@target).order(:id), partial: "api/users/user", as: :user
+allowed_targets = %w[all trainee adviser graduate mentor]
+users = company.users.users_role(target, allowed_targets: allowed_targets).order(:id)
+json.users users, partial: "api/users/user", as: :user

app/views/api/users/companies/index.json.jbuilder

@@ -1,3 +1,3 @@
 # frozen_string_literal: true
 
-json.array! @companies, partial: "api/users/companies/company", as: :company
+json.array! @companies, partial: "api/users/companies/company", as: :company, target: @target

app/views/companies/users/_users_tabs.html.slim

@@ -1,6 +1,6 @@
 nav.tab-nav
   .container
     ul.tab-nav__items
-        - Companies::UsersController::TARGETS.each do |target|
+        - Companies::UsersController::ALLOWED_TARGETS.each do |target|
           li.tab-nav__item
             = link_to t("target.#{target}"), company_users_path(target:), class: (@target == target ? ['is-active'] : []) << 'tab-nav__item-link'

app/views/talks/_nav.html.slim

@@ -1,6 +1,6 @@
 nav.tab-nav
   .container
     ul.tab-nav__items
-      - API::TalksController::TARGETS.each do |target|
+      - API::TalksController::ALLOWED_TARGETS.each do |target|
         li.tab-nav__item
           = link_to t("target.#{target}"), talks_path(target:), class: (@target == target ? ['is-active'] : []) << 'tab-nav__item-link'

app/views/users/companies/_users_tabs.html.slim

@@ -1,6 +1,6 @@
 nav.tab-nav
   .container
     ul.tab-nav__items
-      - Users::CompaniesController::TARGETS.each do |target|
+      - Users::CompaniesController::ALLOWED_TARGETS.each do |target|
         li.tab-nav__item
           = link_to t("target.#{target}"), users_companies_path(target:), class: (@target == target ? ['is-active'] : []) << 'tab-nav__item-link'

test/models/generation_test.rb

@@ -14,13 +14,13 @@ class GenerationTest < ActiveSupport::TestCase
     assert_equal Time.zone.local(2020, 12, 31).end_of_day, Generation.new(32).end_date
   end
 
-  test '#users' do
-    assert_includes Generation.new(5).users, users(:komagata)
-    assert_includes Generation.new(29).users, users(:jobseeker)
+  test '#classmates' do
+    assert_includes Generation.new(5).classmates, users(:komagata)
+    assert_includes Generation.new(29).classmates, users(:jobseeker)
     users(:komagata).created_at = Time.zone.local(2020, 12, 31, 23, 59, 59)
     users(:komagata).save
-    assert_includes Generation.new(32).users, users(:komagata)
-    assert_not_includes Generation.new(33).users, users(:komagata)
+    assert_includes Generation.new(32).classmates, users(:komagata)
+    assert_not_includes Generation.new(33).classmates, users(:komagata)
   end
 
   test '#target_users' do

test/models/user_test.rb

@@ -658,4 +658,24 @@ class UserTest < ActiveSupport::TestCase
     user.become_watcher!(watchable)
     assert user.watches.exists?(watchable:)
   end
+
+  test '.users_role' do
+    allowed_targets = %w[student_and_trainee mentor graduate adviser trainee year_end_party]
+
+    # target引数とdefault_target引数に関して、targetとscope名が一致しているケースと一致していないケースを順にテストする
+    assert_equal User.mentor, User.users_role('mentor', allowed_targets: allowed_targets, default_target: 'student_and_trainee')
+    assert_equal User.graduated, User.users_role('graduate', allowed_targets: allowed_targets, default_target: 'student_and_trainee')
+
+    assert_equal User.year_end_party, User.users_role('', allowed_targets: allowed_targets, default_target: 'year_end_party')
+    assert_equal User.students_and_trainees, User.users_role('', allowed_targets: allowed_targets, default_target: 'student_and_trainee')
+  end
+
+  test '.users_role returns default_target when invalid target is passed' do
+    allowed_targets = %w[student_and_trainee mentor graduate adviser trainee year_end_party]
+    not_allowed_target = 'retired'
+    assert_equal User.students_and_trainees, User.users_role(not_allowed_target, allowed_targets: allowed_targets, default_target: 'student_and_trainee')
+    not_scope_name = 'destroy_all'
+    assert_equal User.students_and_trainees, User.users_role(not_scope_name, allowed_targets: allowed_targets, default_target: 'student_and_trainee')
+    assert_empty User.users_role(not_scope_name, allowed_targets: allowed_targets)
+  end
 end