管理ページに、ユーザーのroleとjobの絞り込み機能追加 #7418

reckyy · 2024-02-21T09:31:52Z

Issue

管理ページでユーザーのロールと職業でソートできるようにする #7348

概要

#7348 (comment) の通りです。

変更確認方法

feature/admin-can-sort-by-user-roles-and-jobs をローカルに取り込む
foreman start -f Procfile.dev
komagata or machida でhttp://localhost:3000/admin/users にアクセス。
絞り込みが正しく動作しているか確認。

Screenshot

変更前

変更後

reckyy · 2024-02-21T11:27:09Z

@machida
お疲れ様です！
下記ファイルのデザインをお願いしたいです。
お手数をおかけしますが、宜しくお願いいたします。 🙏
app/views/admin/users/_nav.html.slim

machida · 2024-03-05T07:23:53Z

@reckyy デザイン入れましたー

一点、お願いがあります。

職業での絞り込みにも「全員」を追加お願いします🙏
（全員 ... つまり職業では絞り込まないというケースがあります）

reckyy · 2024-03-06T01:58:14Z

@machida
デザインありがとうございます！
全員で検索できるように追加しました。
念のためご報告です。

reckyy · 2024-03-08T13:45:34Z

@SuzukaHori
お疲れ様です！
こちらのPRのレビューをお願いしたく、ご連絡いたしました。
お手隙の際にご対応いただけると嬉しいです。全く急ぎではないです。
もし、ご都合悪ければ仰ってください！
よろしくお願いいたします。

SuzukaHori · 2024-03-09T13:49:31Z

@reckyy
お返事遅くなりました💦
レビュー承知しました👍数日中にお返事しますので、少々お待ちください！

SuzukaHori

お待たせしました！
動作はバッチリで、絞り込みが正しく動作していることを確認しました👍

数点コメントしましたので、お手隙の際にご確認ください😊
#7418 (comment) や #7418 (comment) など、重箱の隅を突いている部分もあるので、そちらの修正の是非の判断はお任せします🙇‍♀️

SuzukaHori · 2024-03-10T13:24:34Z

app/controllers/admin/users_controller.rb

@@ -3,11 +3,17 @@
 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
+  ALLOWED_JOBS = User.jobs.keys.prepend('all')


定数かつ配列なので、freezeしておいた方がいいかもしれないです❄️

おっしゃる通りです。修正しました！
[fix]定数をfreeze

SuzukaHori · 2024-03-10T13:28:32Z

app/controllers/admin/users_controller.rb

    user_scope = User.users_role(@target, allowed_targets: ALLOWED_TARGETS, default_target: 'student_and_trainee')
+    if @job.present? && ALLOWED_JOBS.include?(@job)
+      scoped_job = @job == 'all' ? @job : "job_#{@job}"
+      user_scope = user_scope.public_send(scoped_job)


ALLOWED_JOBSで絞り込みはされていますが、ユーザーが直接入力できるparams[:job]をpublic_sendで直接メソッド名として使うのは、危険性もあるかなと思います。

sendを使用せず書くのが理想的だと思うのですが、難しいならALLOWED_JOBS.include?(@job)が何かの拍子に消されたりしないよう、users_roleと似たコメントを残すのが良いと思いました！

bootcamp/app/models/user.rb

Lines 403 to 411 in a917aa0

# このメソッドはユーザから送信された値を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

send以外の方法が思いつかなかったんですよね〜。。
ただ、確かにコードの改竄を防ぐためのコメントの必要性はごもっともです。
なので、追加しました！ありがとうございます 🙇
[chore]コードの意図を説明するコメント追加

コメントの追加を確認しました👍ありがとうございます！

send以外の方法を私も考えてみたのですが、↓のように全部に条件分岐を書くぐらいしか思いつきませんでした💦

user_scope = case @job when 'all' user_scope when 'student' user_scope.job_student when 'office_worker' user_scope.job_office_worker when 'part_time_worker' user_scope.job_part_time_worker when 'vacation' user_scope.job_vacation when 'unemployed' user_scope.job_unemployed end

komagataさんのレビューでsendを使わず簡潔に書ける方法が見つかれば、私も勉強させていただきます🙇‍♀️

@reckyy @SuzukaHori

ALLOWED_JOBSで絞ってるんだったらsendしてもいいかな～とおもいました。

SuzukaHori · 2024-03-10T13:54:13Z

test/system/admin/users_test.rb

+  test 'show listing hibernated and student' do
+    visit_with_auth '/admin/users?target=hibernated&job=office_worker', 'komagata'
+    assert_equal '管理ページ | FBC', title
+    assert_text 'kyuukai（Kyu Kai）'


users_controller.rbのjobの絞り込みの処理（13~16行目）をコメントアウトしても、テストが通ってしまうように見えました。
「条件に当てはまらないユーザが表示されていないこと」も合わせてテストするといいと思いました！

おっしゃる通りです。
検索対象のみが表示されていることを確認するテストに修正しました。
[fix]正常にテストできていなかったため、修正

SuzukaHori · 2024-03-11T00:02:27Z

app/views/admin/users/_filter.html.slim

@@ -0,0 +1,24 @@
+.page-main-filter
+  .container
+    = form_with url: admin_users_path(target: @target), local: true, method: 'get', class: 'page-main-filter__inner' do


細かいですが、特に理由がないならadmin_users_path(@target)の書き方でもいいかもと思いました🙇‍♀️

一応理由としては、「クエリパラメータとして追加される」ということを明記するためにこの書き方を採用しています。（必要なさそうですかね、、？😶）
そちらを踏まえて、再度ご意見をお伺いしたいです！👀

お返事ありがとうございます🙇‍♀️
明示的にクエリパラメータに追加する方が適切だと思いましたので、このままでお願いします！

SuzukaHori · 2024-03-11T00:20:13Z

app/controllers/admin/users_controller.rb

    user_scope = User.users_role(@target, allowed_targets: ALLOWED_TARGETS, default_target: 'student_and_trainee')
+    if @job.present? && ALLOWED_JOBS.include?(@job)


「仕事の状況で絞り込み」は他のページでも使う可能性があると思うので、可能であればモデルに書いておくと再利用しやすいと思いました！

こちらに関してですが、「仕事の状況で絞り込み」が他で行われるケースが思いつきませんでした。😭
どのような可能性を想定されたか、教えていただきたいです！

すみません、前のコメントにちゃんと書いておけばよかったです😅

あまり確固たるイメージはなく、相談部屋（/talks）にも同じ検索機能を作ったり、通常ユーザのユーザ一覧（/users）でも「学生」を検索できるようになったりすることを漠然とイメージしていました。
加えて、行数オーバーのコントローラのリファクタリングに苦労しているPRを最近見たので（多分100行以上？だとrubocopで警告が出る）、無理がないならこの段階でコントローラーの行数を削っておいた方がいいのかな？という気持ちでした👀

ただ現状そのような機能の実装予定はなく、考えすぎな気もしたので、元のままでも大丈夫だと思います🙇‍♀️

詳細ありがとうございます！

なるほど〜。

相談部屋（/talks）にも同じ検索機能を作ったり、通常ユーザのユーザ一覧（/users）でも「学生」を検索できるようになったりすることを漠然とイメージしていました。

後者は職業が非公開情報であることから優先度は低めに感じますが、前者は確かになるほどですね〜。（管理者視点での相談ページを見たことがなかったので、このご意見はとても参考になります。）

komagataさんに確認してみます！

@SuzukaHori @reckyy

そもそもcontrollerがfatなのでほとんどの処理をmodelに持っていきたい感じがします。

@komagata
modelに移行しました。
jobの検索ロジックをcontrollerからmodelに移行

SuzukaHori · 2024-03-11T00:27:51Z

test/system/admin/users_test.rb

@@ -44,6 +44,12 @@ class Admin::UsersTest < ApplicationSystemTestCase
    assert_text 'kensyu（Kensyu Seiko）'
  end

+  test 'show listing hibernated and student' do


「休会中の生徒」の検索ではなく、「休会中の会社員の生徒」を検索するテストのように見えたので、名前もそれに即したものだと良いかなと思いました👀

[fix]正常にテストできていなかったため、修正

reckyy · 2024-03-11T03:54:04Z

@SuzukaHori
コメントありがとうございます！
細かく見ていただき、とてもありがたいです 🙇
修正＆コメントしていますので、お手隙の際にご確認いただけますと幸いです。

SuzukaHori

修正ありがとうございます！
お返事とコメントしましたので、お手隙の際ご確認をお願いします🙇‍♀️

#7418 (comment) に意見を書きましたが、このままにされる場合も問題ないと思いますので、私からはAprroveさせていただきます😊

SuzukaHori · 2024-03-11T05:57:36Z

app/controllers/admin/users_controller.rb

    user_scope = User.users_role(@target, allowed_targets: ALLOWED_TARGETS, default_target: 'student_and_trainee')
+    if @job.present? && ALLOWED_JOBS.include?(@job)


すみません、前のコメントにちゃんと書いておけばよかったです😅

あまり確固たるイメージはなく、相談部屋（/talks）にも同じ検索機能を作ったり、通常ユーザのユーザ一覧（/users）でも「学生」を検索できるようになったりすることを漠然とイメージしていました。
加えて、行数オーバーのコントローラのリファクタリングに苦労しているPRを最近見たので（多分100行以上？だとrubocopで警告が出る）、無理がないならこの段階でコントローラーの行数を削っておいた方がいいのかな？という気持ちでした👀

ただ現状そのような機能の実装予定はなく、考えすぎな気もしたので、元のままでも大丈夫だと思います🙇‍♀️

SuzukaHori · 2024-03-11T06:10:04Z

app/controllers/admin/users_controller.rb

    user_scope = User.users_role(@target, allowed_targets: ALLOWED_TARGETS, default_target: 'student_and_trainee')
+    if @job.present? && ALLOWED_JOBS.include?(@job)
+      scoped_job = @job == 'all' ? @job : "job_#{@job}"
+      user_scope = user_scope.public_send(scoped_job)


コメントの追加を確認しました👍ありがとうございます！

send以外の方法を私も考えてみたのですが、↓のように全部に条件分岐を書くぐらいしか思いつきませんでした💦

user_scope = case @job when 'all' user_scope when 'student' user_scope.job_student when 'office_worker' user_scope.job_office_worker when 'part_time_worker' user_scope.job_part_time_worker when 'vacation' user_scope.job_vacation when 'unemployed' user_scope.job_unemployed end

komagataさんのレビューでsendを使わず簡潔に書ける方法が見つかれば、私も勉強させていただきます🙇‍♀️

SuzukaHori · 2024-03-11T06:29:20Z

app/views/admin/users/_filter.html.slim

@@ -0,0 +1,24 @@
+.page-main-filter
+  .container
+    = form_with url: admin_users_path(target: @target), local: true, method: 'get', class: 'page-main-filter__inner' do


お返事ありがとうございます🙇‍♀️
明示的にクエリパラメータに追加する方が適切だと思いましたので、このままでお願いします！

reckyy · 2024-03-11T10:39:13Z

@SuzukaHori
お疲れ様です！コメントありがとうございます！

#7418 (comment) に意見を書きましたが、このままにされる場合も問題ないと思いますので、私からはAprroveさせていただきます😊

承知しました〜。
丁寧にレビューいただき、ありがとうございました。 🙇

reckyy · 2024-03-11T10:46:11Z

@komagata
お疲れ様です！チームメンバーにApproveいただいたので、レビューをお願いいたします。

お尋ねしたいこと

下記議論について、ご意見をお伺いしたいです。
#7418 (comment)

将来を見越して、 ALLOWED_JOBSの情報をuser.rbに定義するべきかどうか。

#7418 (comment)

public_sendを使った実装のままで良いか。

komagata · 2024-03-19T08:28:17Z

@reckyy それぞれのスレッドにコメントさせていただきました～

reckyy · 2024-03-25T05:13:58Z

@komagata
お疲れ様です。
コメントありがとうございます。

コミットメッセージの先頭に記号を入れるルールは採用しているプロジェクトではよいですが、一人だけ採用すると統一感がなくなって余計にわかりずらくなってしまうので、プロジェクト全体で同意をとってから導入するようにしてください。

コミットメッセージのprefixについてですが、こちらprefixがないコミットメッセージを採用した別PRを再作成したほうがよろしいでしょうか。

他のPR2つでも同様の質問を行なっています。もし3つのPRのうち、一つにご回答いただければ他PRでのこの質問内容に関しては、スルーしていただいて大丈夫です。 🙇
（ただ、コミットメッセージ以外の質問も行なっている場合がありますので、コメントに関してはご確認いただきたいです。）

reckyy · 2024-03-30T14:14:05Z

@komagata
お疲れ様です。

modelへの移行
コミットメッセージのprefix削除

上記2点が完了しましたので、再度ご確認をお願いいたします。

komagata · 2024-04-16T08:44:52Z

test/helpers/users_helper.rb

@@ -8,4 +8,33 @@ class UsersHelperTest < ActionView::TestCase
    assert_includes countries['JP'], %w[北海道 01]
    assert_includes countries['US'], %w[アラスカ州 AK]
  end
+
+  test 'roles_for_select returns array for select_tag containing role info' do


Suggested change

test 'roles_for_select returns array for select_tag containing role info' do

test '#roles_for_select' do

他に合わせて正常系のみのテストであればこういう命名でいいかなと思いました。

おっしゃる通り、正常系のみのテストなのでメソッド名のみに命名し直しました。
testの命名を変更

reckyy · 2024-04-20T09:22:30Z

@komagata
お疲れ様です。
コメントありがとうございます！
テスト名変更しましたので、再度ご確認のほどよろしくお願いいたします。

komagata · 2024-04-24T03:05:17Z

app/controllers/admin/users_controller.rb

@@ -7,7 +7,9 @@ class Admin::UsersController < AdminController
  def index
    @direction = params[:direction] || 'desc'
    @target = params[:target]
+    @job = params[:job]


@jobはviewで使っていますか？
もし使っていなければインスタンス変数である必要はないかもです。

こちら、渡しそびれていました。
気づいていただきありがとうございます！

下記でsortするときに渡すクエリパラメータのために、用意していたインスタンス変数になります。

ユーザー名

企業

最終活動日時

登録日時

渡しそびれていたインスタンス変数を追記

@reckyy なるほどです～

reckyy · 2024-04-28T12:59:59Z

@komagata
お疲れ様です。
#7418 (comment) の内容について対応しました。
ご確認いただけますと幸いです。
よろしくお願いいたします。

reckyy · 2024-05-09T14:21:48Z

@komagata
コンフリクトを解消したら、なぜかテストが落ちるようになったので、解消し次第またメンションさせていただきます。

ビューではなく、ヘルパーファイルにroleとjobの配列を返すメソッド作成

確かに休会&会社員で検索はできていたが、それ以外のユーザー（つまりjobでの検索が機能していなくても）が引っかかっていても、テストは通っていた。なので、検索対象外のユーザーが確認できないことの確認を追加

reckyy · 2024-05-10T13:54:35Z

@komagata
お疲れ様です。
コンフリクトを解消しました。お手隙の際にレビューよろしくお願いいたします。

komagata

確認させていただきました。OKです～👌

komagata · 2024-05-28T19:13:24Z

app/controllers/admin/users_controller.rb

@@ -7,7 +7,9 @@ class Admin::UsersController < AdminController
  def index
    @direction = params[:direction] || 'desc'
    @target = params[:target]
+    @job = params[:job]


@reckyy なるほどです～

reckyy assigned reckyy and machida Feb 22, 2024

machida removed their assignment Mar 5, 2024

reckyy force-pushed the feature/admin-can-sort-by-user-roles-and-jobs branch from e6e6858 to 4d3e19b Compare March 6, 2024 01:53

reckyy force-pushed the feature/admin-can-sort-by-user-roles-and-jobs branch from 058ccb9 to 7d9c3ff Compare March 8, 2024 03:04

reckyy requested a review from SuzukaHori March 8, 2024 13:45

reckyy marked this pull request as ready for review March 8, 2024 13:45

SuzukaHori reviewed Mar 11, 2024

View reviewed changes

SuzukaHori approved these changes Mar 11, 2024

View reviewed changes

reckyy force-pushed the feature/admin-can-sort-by-user-roles-and-jobs branch from 49aa56d to 8c917bc Compare March 25, 2024 05:12

reckyy force-pushed the feature/admin-can-sort-by-user-roles-and-jobs branch from 8c917bc to 21b35ae Compare March 27, 2024 03:51

komagata force-pushed the main branch from 033db0e to 39dc85f Compare March 27, 2024 07:19

reckyy force-pushed the feature/admin-can-sort-by-user-roles-and-jobs branch from 21b35ae to 77e8fa0 Compare March 29, 2024 03:06

komagata reviewed Apr 16, 2024

View reviewed changes

komagata reviewed Apr 24, 2024

View reviewed changes

reckyy force-pushed the feature/admin-can-sort-by-user-roles-and-jobs branch 2 times, most recently from 00a5b38 to d381f3f Compare May 8, 2024 13:16

reckyy force-pushed the feature/admin-can-sort-by-user-roles-and-jobs branch 6 times, most recently from d10cf7b to d175997 Compare May 10, 2024 13:39

reckyy and others added 13 commits May 10, 2024 22:41

userのroleとjobで絞り込みできるように実装

08658e7

ヘルパーメソッド作成

6a50161

ビューではなく、ヘルパーファイルにroleとjobの配列を返すメソッド作成

休会+会社員の絞り込みの正常系テスト追加

175d024

ユーザー絞り込みにデザイン

86b3622

職業でも全員検索できるように追加

b1ac533

空行を削除

6960b37

ヘルパーメソッドのテスト追加

cbd4433

定数をfreeze

62c8958

コードの意図を説明するコメント追加

d654776

jobの検索ロジックをcontrollerからmodelに移行

d82379b

testの命名を変更

768eb2c

渡しそびれていたインスタンス変数を追記

d7fe6a0

reckyy force-pushed the feature/admin-can-sort-by-user-roles-and-jobs branch from d175997 to d7fe6a0 Compare May 10, 2024 13:41

komagata approved these changes May 28, 2024

View reviewed changes

komagata merged commit f73c490 into main May 28, 2024
2 checks passed

komagata deleted the feature/admin-can-sort-by-user-roles-and-jobs branch May 28, 2024 19:14

github-actions bot mentioned this pull request May 28, 2024

Release 2024-06-04 14:22:37 +0000 #7802

Merged

18 tasks

	# このメソッドはユーザから送信された値を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

		user_scope = User.users_role(@target, allowed_targets: ALLOWED_TARGETS, default_target: 'student_and_trainee')
		if @job.present? && ALLOWED_JOBS.include?(@job)

	test 'roles_for_select returns array for select_tag containing role info' do
	test '#roles_for_select' do

管理ページに、ユーザーのroleとjobの絞り込み機能追加 #7418

管理ページに、ユーザーのroleとjobの絞り込み機能追加 #7418

Conversation

reckyy commented Feb 21, 2024 • edited Loading

Issue

概要

変更確認方法

Screenshot

変更前

変更後

reckyy commented Feb 21, 2024 • edited Loading

machida commented Mar 5, 2024

reckyy commented Mar 6, 2024

reckyy commented Mar 8, 2024

SuzukaHori commented Mar 9, 2024

SuzukaHori left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reckyy Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reckyy Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reckyy Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reckyy Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reckyy commented Mar 11, 2024 • edited Loading

SuzukaHori left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reckyy commented Mar 11, 2024

reckyy commented Mar 11, 2024

お尋ねしたいこと

komagata commented Mar 19, 2024

reckyy commented Mar 25, 2024 • edited Loading

reckyy commented Mar 30, 2024

Choose a reason for hiding this comment

reckyy Apr 20, 2024 • edited Loading

Choose a reason for hiding this comment

reckyy commented Apr 20, 2024

Choose a reason for hiding this comment

reckyy Apr 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reckyy commented Apr 28, 2024

reckyy commented May 9, 2024

reckyy commented May 10, 2024

komagata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reckyy commented Feb 21, 2024 •

edited

Loading

reckyy commented Feb 21, 2024 •

edited

Loading

SuzukaHori left a comment •

edited

Loading

reckyy Mar 11, 2024 •

edited

Loading

reckyy Mar 11, 2024 •

edited

Loading

reckyy Mar 11, 2024 •

edited

Loading

reckyy Mar 25, 2024 •

edited

Loading

reckyy commented Mar 11, 2024 •

edited

Loading

SuzukaHori left a comment •

edited

Loading

reckyy commented Mar 25, 2024 •

edited

Loading

reckyy Apr 20, 2024 •

edited

Loading

reckyy Apr 28, 2024 •

edited

Loading