ユーザー検索で休会・退会ユーザーも引っかかるようにする

[kurumadaisuke]

Issue

• ユーザー検索で休会・退会ユーザーも引っかかるようにする #7396

概要

ユーザー検索で休会・退会ユーザーも引っかかるようにする

変更確認方法

1. feature/searchable_suspended_and_unsubscribed_usersをローカルに取り込む
2. メンターでログインをする
3. http://localhost:3000/usersにアクセスし、ユーザー一覧を表示する
4. 「全員」タブを選択する
5. 検索欄から休会ユーザー名、退会ユーザー名を検索して表示されることを確認する（ユーザー例: yameo, kyuukai）
6. 右上の検索バーから休会ユーザー名、退会ユーザー名を検索して表示されることを確認する（ユーザー例: yameo, kyuukai）
   ※ 右上の検索バーから休会ユーザー名は元々表示されます。

Screenshot

変更前

変更後

[kurumadaisuke]

@nishitatsu-dev
お疲れ様です。
こちらレビューお願いしてもよろしかったでしょうか？🙏
特に期限は設けていませんのでお手隙の際にご確認いただければと思います🙏

[nishitatsu-dev]

@kurumadaisuke
レビュー承知しました〜
１週間以内に対応しますので、少々お待ちください🙏

[nishitatsu-dev]

@kurumadaisuke
見ました〜。めちゃくちゃ難しいですね😅
ひとまずkomagataさんに以下のご相談をお願いします🙏

• 右上の検索：Searchableモジュールの変更方針について
• メンターの検索：「表示人数が少ない問題」への対応について

[nishitatsu-dev]

変更の方針について考えてみたことを以下に書きます。
（合っているか分からないのでkomagataさんと相談してみて下さい🙏）

変更の方針について

以下２種類の方針があると思います。

• 方針①：このモジュール本体は変更せず、利用側を変更する
  「このSearchableモジュールが他の約10個のモデルファイルにincludeされていて、変更の影響範囲が広いので変更しない」という考えです。
  利用側の変更については、以下を削除すると「右上の検索」がうまく引っかかりました（間違ってたらアレなので確認してみて下さい🙏）
  

  bootcamp/app/models/user.rb

  Line 365 in 6dcc837

  scope :search_by_keywords_scope, -> { unretired }

• 方針②：このモジュールを変更する
  「search_by_keywords_scopeを使って検索を制限しているのは、app/models/user.rbだけ。今回それも無くすので、根本から消す」という考えです。
  今回の変更に加えて、以下の２箇所も削除できると思います
  削除①：8〜11行目のスコープ定義
  削除②：↓利用側のスコープ上書き（どちらの方針でも削除することになります）
  

  bootcamp/app/models/user.rb

  Line 365 in 6dcc837

  scope :search_by_keywords_scope, -> { unretired }

どちらの方針でも必要なこと

どちらの方針にしても「スコープの上書き」を削除するので、以下のコメント部分を見直す必要があります
（ただ、app/controllers/api/users_controller.rbを触ってみた感じでは、「スコープの上書き」は、このコメント部分には効いてなさそうです）

bootcamp/app/controllers/api/users_controller.rb

Lines 43 to 45 in 6dcc837

	# search_by_keywords内では { unretired } というスコープが設定されている
	# 退会したユーザーに対しキーワード検索を行う場合は、一旦 unscope(where: :retired_on) で { unretired } スコープを削除し、その後で retired スコープを設定する必要がある
	target == 'retired' ? users.unscope(where: :retired_on).retired : users

bootcamp/app/controllers/api/talks_controller.rb

Lines 15 to 17 in 6dcc837

	# search_by_keywords内では { unretired } というスコープが設定されている
	# 退会したユーザーも検索対象に含めたいので、unscope(where: :retired_on) で上記のスコープを削除
	searched_users = users.search_by_keywords(word: params[:search_word]).unscope(where: :retired_on)

ひとまずkomagataさんに相談してみて下さい（全く違う方針になるかも😅）

[nishitatsu-dev]

@kurumadaisuke
下記コメントへのご質問、こちらに回答しますね〜
（イメージが合うように、細かく長々と書きますが、ご容赦下さい🙇🏻）

方針①：このモジュール本体は変更せず、利用側を変更する
⇨ 利用者側とはransack(**params_for_keyword_search(searched_values)).resultのことを指していますか？
なので、今回のコードの修正内容が該当している認識です。

利用側は、このSearchableモジュール（searchable.rb全体）をincludeしている、約10個のモデルファイル（include Searchableが書いてあるファイル）のつもりでした。
約10個のモデルファイルの中で、app/models/user.rbだけがsearch_by_keywords_scopeを上書きしているので、
利用側での変更は、「app/models/user.rbでの、scope :search_by_keywords_scope, -> { unretired } の削除」だけになります。

方針②：このモジュールを変更する
⇨ これはscope :search_by_keywords_scope, -> { unretired } を根本的に削除する方法であっていますか？
8〜11行目のスコープ定義の削除ですが、以下行を指していますか？

このSearchableモジュールの変更は、以下の２点のつもりでした。
削除①-1：スコープの定義の削除：下記コードの8〜11行目です
削除①-2：スコープの使用箇所の削除：下記コードの16行目です（今回くるまさんが削除した部分です）

bootcamp/app/models/concerns/searchable.rb

Lines 3 to 17 in bf32957

	module Searchable
	extend ActiveSupport::Concern
	COLUMN_NAMES_FOR_SEARCH_USER_ID = %i[user_id last_updated_user_id].freeze
	included do
	# 拡張する場合はこのスコープを上書きする
	scope :search_by_keywords_scope, -> { all }
	end
	# rubocop:disable Metrics/BlockLength
	class_methods do
	def search_by_keywords(searched_values = {})
	ransack(**params_for_keyword_search(searched_values)).result.search_by_keywords_scope
	end

削除②：このSearchableモジュール本体側での削除に伴って、利用側のapp/models/user.rbでも、scope :search_by_keywords_scope, -> { unretired } を削除します。
この利用側の変更は、方針①と同じになります。

分かりにくい所あればご質問下さい。よろしくお願いいたします🙏

[kurumadaisuke]

@nishitatsu-dev
確認が遅くなり申し訳ありません🙇‍♂️
ご説明いただいたので大体わかったと思います！！

[nishitatsu-dev]

「元のコードが何か変な感じ？」で、それがクリアになると、別の変更方法が必要になるかもしれません。
以下、少し調べた結果を書きます。一度、komagataさんに相談をお願いします🙏

調べた結果

• localhost:3000/users?target=allで、表示される人数「全員（67）」が少ないです。db/fixtures/users.ymlには77人います（marumarushainが25人、コードで生成されています）。表示されていない人は「復帰 太郎」や「辞目 辞目夫」など、hibernated_atやretired_onに値を持っている人のようです。
  ここで思ったのは「元々カウントされていない人を検索しても引っかからないのでは？」です。
  （表示人数が正しくなれば、コードの変更不要になるかも？？？）
  この「表示人数が少ない問題」は、このissueの範囲を超える話かもしれないので、komagataさんと相談してみて下さい🙏
• app/models/concerns/searchable.rb側のコメントにも書きましたが、以下のスコープ上書きは効いて無さそうです。また、今回変更したhibernated_atもスコープ設定されていないので、コード中のコメントに書いてある内容とは別の所で制限がかかってそうです。
  （上記の「表示人数の問題」をクリアすれば、この部分のコード変更が変わる可能性があると思います。変わらないかもしれませんが。。。）
  

  bootcamp/app/models/user.rb

  Line 365 in 6dcc837

  scope :search_by_keywords_scope, -> { unretired }

[kurumadaisuke]

@nishitatsu-dev
ご確認ありがとうございます！！
ちょっと私の方でもわかっていない部分が多いと思うので、コメントの内容を確認させてください🙏

変更の方針について

方針①：このモジュール本体は変更せず、利用側を変更する
⇨ 利用者側とはransack(**params_for_keyword_search(searched_values)).resultのことを指していますか？
なので、今回のコードの修正内容が該当している認識です。

方針②：このモジュールを変更する
⇨ これはscope :search_by_keywords_scope, -> { unretired } を根本的に削除する方法であっていますか？
8〜11行目のスコープ定義の削除ですが、以下行を指していますか？

authenticates_with_sorcery!
  VALID_SORT_COLUMNS = %w[id login_name company_id last_activity_at created_at report comment asc desc].freeze
  AVATAR_SIZE = '88x88>'
  RESERVED_LOGIN_NAMES = %w[adviser all graduate inactive job_seeking mentor retired student student_and_trainee trainee year_end_party].freeze

[kurumadaisuke]

@nishitatsu-dev

調べた結果

こちら確かに私も元々のコードがなんだかおかしい気がしていて、コメントの内容も含めハマってしまいました😰

表示される人数「全員（67）」が少ないです。

こちら気づかなかったので、確認していただきありがとうございます！🙏
こちらの仕様についてはkomagataさんと相談していこうと思いますmm

[kurumadaisuke]

@komagata
本issueの対応方法について質問させてください

Searchableモジュールの変更方針について

FBCアプリの右上に存在する検索処理ではSearchableモジュールを利用しており、約10個のモデルファイルにincludeされています。
その中でapp/models/user.rbがscope :search_by_keywords_scope, -> { unretired }を利用して検索結果を制限しています。

変更方法案①
Searchableモジュール自体は変更せず、代わりに利用者側の変更を行います。
具体的には、app/models/user.rb内のscope :search_by_keywords_scope, -> { unretired }の行を削除します。

変更方法②
Searchableモジュール自体を変更し、scope :search_by_keywords_scope, -> { all }を削除します。この変更に伴い、利用者側のapp/models/user.rbでも同じくscope :search_by_keywords_scope, -> { unretired }を削除します。

※ どちらの方針でも、app/models/user.rb内のscope :search_by_keywords_scopeの削除が必要です。

もしくは、他の方法を用いるべきでしょうか？🙏

メンターの検索：「表示人数が少ない問題」への対応について

localhost:3000/users?target=allで表示される人数がdb/fixtures/users.ymlでは77人のユーザーが定義されていますが、hibernated_atやretired_onに値を持つユーザーがカウントされないため、「全員（67）」となっています。
表示される全員の数値は本issueでの対応範囲となりますでしょうか？

[komagata]

@kurumadaisuke

本issueの対応方法について質問させてください

まだ判断するための材料がたりないとおもうので、それぞれの影響範囲（実際のどのページにもちいられるか。そのページが変わっても問題ないか）を調べてみてください。

そのうえで @kurumadaisuke さんがどうすべきだと思うかも考えてみてください。

メンターの検索：「表示人数が少ない問題」への対応について

メンターの検索というのはどういう意味でしょうか？

[kurumadaisuke]

@komagata

まだ判断するための材料がたりないとおもうので、それぞれの影響範囲（実際のどのページにもちいられるか。そのページが変わっても問題ないか）を調べてみてください。
そのうえで @kurumadaisuke さんがどうすべきだと思うかも考えてみてください。

こちら調査した後に回答いたします。

メンターの検索というのはどういう意味でしょうか？

一般ユーザー（受講生）からの「ユーザー」タブを選択した時の表示内容と
メンターから「ユーザー」タブを選択するのでは、表示が異なっており（メンターの方が多く表示されている）メンターが「ユーザー」タブを選択した状態をメンターの検索という風に言っていました🙏

[komagata]

@kurumadaisuke

一般ユーザー（受講生）からの「ユーザー」タブを選択した時の表示内容と
メンターから「ユーザー」タブを選択するのでは、表示が異なっており（メンターの方が多く表示されている）メンターが「ユーザー」タブを選択した状態をメンターの検索という風に言っていました🙏

「言っていました」というのはどういう意味でしょうか？

[kurumadaisuke]

@komagata

「言っていました」というのはどういう意味でしょうか？

すいません。
言葉足らずでした🙏

一般ユーザー（受講生）からの「ユーザー」タブを選択した時の表示内容と
メンターから「ユーザー」タブを選択するのでは、表示が異なっており（メンターの方が多く表示されている）メンターが「ユーザー」タブを選択した状態を「メンターの検索」という意味で使った言葉になります🙏

[komagata]

@kurumadaisuke なるほどです！

[kurumadaisuke]

@komagata
#7435 (comment)

こちら調査してみたのですが、Searchableモジュールのscope :search_by_keywords_scope, -> { all }の部分を使っているのがapp/models/user.rbのscope :search_by_keywords_scope, -> { unretired }だけだったので使わないコードはなるべく削除したほうが良いっと考え、大元のscope :search_by_keywords_scope, -> { all }を削除して影響範囲（テスト）がどうなるか確認をしました。

テストの結果は一つ失敗しましたが、元々退会/休会ユーザーが表示されないことを確認するテストだったのでアプリ自体の仕様が変わったので、テスト自体を修正しました。

なので調査結果では、大元を削除しても影響範囲がほとんどないっと考え不要なコードは削除するべきだと考えました🙏

[komagata]

@kurumadaisuke

bootcamp/app/models/concerns/searchable.rb

Line 9 in 206f5c8

# 拡張する場合はこのスコープを上書きする

拡張する場合はこのスコープを上書きする

と書いてあって、検索対象の拡張の仕組みを用意してくれている感じ（あえてやっている）のでちょっと悩むところですが、まあ削除しちゃっていいとおもいます～。