diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 4441924ba49..a1f66f9a587 100644 --- a/app/controllers/admin/users_controller.rb +++ b/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 diff --git a/app/controllers/api/generations_controller.rb b/app/controllers/api/generations_controller.rb index d6b00b580c1..2a661e1d3c8 100644 --- a/app/controllers/api/generations_controller.rb +++ b/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 diff --git a/app/controllers/api/talks_controller.rb b/app/controllers/api/talks_controller.rb index 44bfd902a9d..090e2287e31 100644 --- a/app/controllers/api/talks_controller.rb +++ b/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 diff --git a/app/controllers/api/users_controller.rb b/app/controllers/api/users_controller.rb index db5dcf71116..9aa5e0d00cc 100644 --- a/app/controllers/api/users_controller.rb +++ b/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 diff --git a/app/controllers/companies/users_controller.rb b/app/controllers/companies/users_controller.rb index 8aa01d0ee02..bfc5ef8dce5 100644 --- a/app/controllers/companies/users_controller.rb +++ b/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 diff --git a/app/controllers/generations_controller.rb b/app/controllers/generations_controller.rb index e6db9893070..50f37b85a74 100644 --- a/app/controllers/generations_controller.rb +++ b/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 diff --git a/app/controllers/talks_controller.rb b/app/controllers/talks_controller.rb index 8a55fd9f25d..b79356fd789 100644 --- a/app/controllers/talks_controller.rb +++ b/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 diff --git a/app/controllers/users/companies_controller.rb b/app/controllers/users/companies_controller.rb index 0d6b41f20c9..454ab05815e 100644 --- a/app/controllers/users/companies_controller.rb +++ b/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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 3e6a3731e7c..dfda69738df 100644 --- a/app/controllers/users_controller.rb +++ b/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 diff --git a/app/models/generation.rb b/app/models/generation.rb index deea616160f..ceaac17527c 100644 --- a/app/models/generation.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 9f58d1f8546..87b327f8cd7 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/views/api/users/companies/_company.json.jbuilder b/app/views/api/users/companies/_company.json.jbuilder index 18502326290..9d61e3027fe 100644 --- a/app/views/api/users/companies/_company.json.jbuilder +++ b/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 diff --git a/app/views/api/users/companies/index.json.jbuilder b/app/views/api/users/companies/index.json.jbuilder index 50ee82d4b90..90e03a0ddbb 100644 --- a/app/views/api/users/companies/index.json.jbuilder +++ b/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 diff --git a/app/views/companies/users/_users_tabs.html.slim b/app/views/companies/users/_users_tabs.html.slim index 374fae220c9..45ec2caee72 100644 --- a/app/views/companies/users/_users_tabs.html.slim +++ b/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' diff --git a/app/views/talks/_nav.html.slim b/app/views/talks/_nav.html.slim index 16e060bdccd..70401685623 100644 --- a/app/views/talks/_nav.html.slim +++ b/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' diff --git a/app/views/users/companies/_users_tabs.html.slim b/app/views/users/companies/_users_tabs.html.slim index f9e2e07051f..396467a985d 100644 --- a/app/views/users/companies/_users_tabs.html.slim +++ b/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' diff --git a/test/models/generation_test.rb b/test/models/generation_test.rb index 03d7d42f953..ef043030e41 100644 --- a/test/models/generation_test.rb +++ b/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 diff --git a/test/models/user_test.rb b/test/models/user_test.rb index a818c7bc5ff..10fd634aa2a 100644 --- a/test/models/user_test.rb +++ b/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