ユーザーの検索結果に、管理者でログインしたときだけそのユーザーの相談部屋へのリンクを表示 #4147

ot0m1 · 2022-02-05T12:59:40Z

ref: #4068

要件

ユーザーの検索結果に、管理者でログインしたときだけそのユーザーの相談部屋へのリンクを表示する。

画面イメージ

変更前

変更後

確認方法

display-link-to-consultation-room-in-users-search-results ブランチをローカルに持ってくる（参考：https://qiita.com/great084/items/ad74dd064a2c2bc47cff ）

git fetch origin pull/{このプルリクエストのid}/head:{任意のブランチ名}
git checkout {上記でfetchした任意のブランチ名}

任意のアカウントでログイン
適当に検索したユーザーの検索結果（一例：http://localhost:3000/searchables?document_type=users&word=a ）画面で以下を確認

管理者のアカウント（komagata,machita）であれば相談部屋を持っている全てのユーザーで相談部屋へのリンクが表示される
管理者以外のアカウントであればユーザーの検索結果には相談部屋へのリンクが表示されない
ユーザー以外の検索結果には相談部屋へのリンクが表示されない

ot0m1 · 2022-02-09T13:32:17Z

ブランチ名に feature/ つけ忘れ…

ot0m1 · 2022-02-14T12:29:25Z

@haruguchi-yuma お疲れ様ですレビューお願いいたします。
一部通らないテストがあったので、 #4203 を先取って取り込んでいます。

haruguchi-yuma

レビュー遅くなりました！数点コメントしたのでご確認ください:bow:

全体的なところで1点。
相談部屋のリンクが表示される、されないは重要なことだと思いますのでテストがあってもいいのかなぁ〜と思いました！

よろしくお願いします🙏

haruguchi-yuma · 2022-02-16T13:03:34Z

app/helpers/search_helper.rb

+  def talk?(searchable)
+    searchable.instance_of?(User) && Talk.find_by(user_id: searchable.id)
+  end


おそらくtalk?メソッドはbooleanを戻り値にしたいと思うんですが&&の右辺まで評価が進むと相談部屋オブジェクトが戻り値になるのでちょっと驚きがある気がします！

こんな感じはどうでしょうか？という提案です。またSQL叩かなくて済むというメリットもあります。

def talk? searchable.instance_of?(User) && searchable.talk.present? end

右辺まで評価が進むと相談部屋オブジェクトが戻り値になるのでちょっと驚きがある

確かにその通りですね🙏 修正しました。

haruguchi-yuma · 2022-02-16T13:05:55Z

app/helpers/search_helper.rb

+  def talk_id(searchable)
+    Talk.find_by(user_id: searchable.id).id
+  end


Viewに関することというよりはインスタンスに依存するコードなのでモデルに書いた方がいい気がするのですが、自信はないです😅

もしくは、現状talk_idというメソッドを使うところが他になさそうなのでjbuilderの方に直接

if talk?(searchable) && display_talk?(searchable) json.talk_id searchable.talk.id end

と記述してもいいかもしれません。

talk_id というメソッド名がご指摘いただいたようなコードを読んでいてのひっかかりポイントになっているのかなと考え、メソッド名の変更で対応しました。

@ot0m1
返事遅くなりました！

伝え方が悪くて申し訳ないです😅

メソッド名が引っかかっているというよりはhelperメソッドは見た目の部分に関することを記述すると思うのでtalk_idを取得するようなコード（SQLを叩くコード）はここに記述しない方がいいのでは？という疑問から発言しました。

２つ目のコメントはtalk_idを返すメソッドにそこまで汎用性がなさそうなのでjbuiderの方に記述すればメソッドを定義しなくても済むのかなぁと思ってコメントさせてもらいました！

User は talk というカラムを持っていないのでメソッドにして Talk.find_by(user_id: searchable.id).id を実行しないといけないと思い込んでいた（そのため #4147 (comment) みたいなコメントをしている）ので、ご指摘の通りに修正いたしました。

モデルで belongs_to :user、 has_one :talk, dependent: :destroy という宣言が行われていたのを把握しておらず、ずれた方向でリファクタリングを進めてしまっておりました🙏

Merge branch 'main' into display-link-to-consultation-room-in-users-search-results

ot0m1 · 2022-02-19T11:17:29Z

@haruguchi-yuma レビューありがとうございました🙏
ご指摘いただいた箇所と、また自分でもクエリ発行箇所の多さが気になったのでその他の部分も変更し、テストを追加いたしました。再レビューお願いいたします。

komagata · 2022-02-21T05:55:12Z

app/helpers/search_helper.rb

@@ -39,4 +39,12 @@ def filtered_message(searchable)
  def comment_or_answer?(searchable)
    searchable.is_a?(Comment) || searchable.is_a?(Answer)
  end
+
+  def talk?(searchable)


対応するhelperのテストがあるといいかもです〜

ヘルパーテストを追加するみたいな考えが全くありませんでした🙏
追加しました。

haruguchi-yuma

追加分のテストについてはOKだと思います:data:

1点コメントしているのでご確認ください！😄

ot0m1 · 2022-02-22T11:56:29Z

@haruguchi-yuma 諸々修正いたしました。何度もすいませんがレビューお願いいたします〜🙏

komagata · 2022-02-28T06:16:05Z

test/helpers/search_hepler.rb

+
+require 'test_helper'
+
+class  SearchHelperTest < ActionView::TestCase


Suggested change

class SearchHelperTest < ActionView::TestCase

class SearchHelperTest < ActionView::TestCase

Merge branch 'main' into display-link-to-consultation-room-in-users-search-results

ot0m1 · 2022-03-01T11:37:38Z

@komagata
すごい初歩的な確認漏れをしておりました🙏
修正しましたので再度レビューお願いいたします。

komagata · 2022-03-01T18:12:21Z

test/helpers/search_hepler.rb

+require 'test_helper'
+
+class SearchHelperTest < ActionView::TestCase
+  test 'does it return correct boolean value in talk' do


「talkかどうかを返す」みたいな内容でいいと思います〜。

テスト名変更しました。

Merge branch 'main' into display-link-to-consultation-room-in-users-search-results

ot0m1 · 2022-03-02T15:48:40Z

@komagata レビューいただいた点を修正しました。再レビューお願いいたします。

komagata · 2022-03-03T11:40:12Z

test/helpers/search_hepler.rb

+class SearchHelperTest < ActionView::TestCase
+  test 'return whether talk' do
+    user = users(:kimura)
+    assert_equal true, talk?(user)


trueかどうかのチェックはassertメソッドがいいと思います。

アサーションの種類について全然把握していませんでした🙇‍♂️

Rails テスティングガイドを見て assert と assert_not を用いるのが良いことを理解しました。

ot0m1 · 2022-03-03T13:45:58Z

@komagata 何度もレビューありがとうございます〜🙏 ご指摘いただいた点を修正いたしました。ご確認お願いいたします。

komagata · 2022-03-06T18:30:44Z

test/helpers/search_hepler.rb

+class SearchHelperTest < ActionView::TestCase
+  test 'return whether talk' do
+    user = users(:kimura)
+    assert true, talk?(user)


assertは引数を一つとるはずです〜

ドキュメントを理解できていませんでした😭修正しました。

ot0m1 · 2022-03-07T01:03:12Z

@komagata 何度もレビューお手数おかけします🙏 再度レビューお願いいたします。

komagata · 2022-03-07T14:03:08Z

test/helpers/search_hepler.rb

+    user = users(:kimura)
+    assert talk?(user)
+
+    user = users(:taikai3)
+    assert_not talk?(user)


複数回使う必要がないのであれば代入せずこのようにするとスマートかもです〜

Suggested change

user = users(:kimura)

assert talk?(user)

user = users(:taikai3)

assert_not talk?(user)

assert talk?(users(:kimura))

assert_not talk?(users(:taikai3))

ご指摘の通り冗長な変数宣言だと思いましたので修正いたしました。

ot0m1 · 2022-03-08T11:13:02Z

@komagata 再レビューお願いいたします🙏

komagata · 2022-03-10T04:03:44Z

test/system/searchables_test.rb

@@ -135,4 +135,24 @@ class SearchablesTest < ApplicationSystemTestCase
    assert_text 'kimura'
    assert_no_text 'machida'
  end
+
+  test 'check that link to talk room is displayed properly' do


管理者で相談部屋へのリンクが表示される

管理者以外で相談部屋へのリンクが表示されない

こちらは二つのテストケースに分けた方がテストのタイトルと合わせてわかりやすいかもです〜

確かに…🙏 分割しました。

ot0m1 · 2022-03-10T11:50:07Z

@komagata 再レビューお願いいたします。

komagata

確認しました、OKですー🙆‍♂️

first commit

8790657

ot0m1 added 6 commits February 10, 2022 23:00

相談部屋へのリンクを表示

f859a3f

冗長な条件分岐をリファクタリング

5b38b4a

さらにリファクタリング

b0f7b84

rubocop通した

1142061

さらにリファクタリング

4059f8d

クリップボードのテストを通るように修正

4c85f20

ot0m1 marked this pull request as ready for review February 14, 2022 12:28

ot0m1 requested a review from haruguchi-yuma February 14, 2022 12:29

haruguchi-yuma reviewed Feb 16, 2022

View reviewed changes

ot0m1 added 7 commits February 17, 2022 13:39

talk?メソッドの返り値をbooleanのみに

1bea353

メソッド名を具体的に

8e9eea5

管理者だけにリンクが表示されるように変更

11ea071

不要な条件分岐を削除

9cdc350

管理者かを判断するロジックをVue側に持たせた

f9d98ee

テストを追加

65cfe98

最新の origin/mian を取り込み

cc300ee

Merge branch 'main' into display-link-to-consultation-room-in-users-search-results

ot0m1 requested a review from haruguchi-yuma February 19, 2022 11:17

テストの条件を変更

fe5ba3d

komagata requested changes Feb 21, 2022

View reviewed changes

haruguchi-yuma reviewed Feb 21, 2022

View reviewed changes

ot0m1 added 4 commits February 22, 2022 20:12

ヘルパーのテストを追加

33b922b

テスト名を修正

fba40bd

不要なヘルパーメソッドを削除

6522a73

不要なヘルパーテストを削除

0072d92

ot0m1 requested a review from haruguchi-yuma February 22, 2022 11:56

komagata requested changes Feb 28, 2022

View reviewed changes

ot0m1 added 2 commits February 28, 2022 19:32

不要なスペースを削除

8a7fa44

テストが通るように最新の origin/main を取り込み

f6fc8a7

Merge branch 'main' into display-link-to-consultation-room-in-users-search-results

ot0m1 requested a review from komagata March 1, 2022 11:37

komagata requested changes Mar 1, 2022

View reviewed changes

ot0m1 added 2 commits March 2, 2022 23:35

テスト名を変更

7f133ba

テストが通るように最新の origin/main を取り込み

028fd7e

Merge branch 'main' into display-link-to-consultation-room-in-users-search-results

ot0m1 requested a review from komagata March 2, 2022 15:48

komagata requested changes Mar 3, 2022

View reviewed changes

適切なアサーションに修正

57c1b36

ot0m1 requested a review from komagata March 3, 2022 13:46

komagata requested changes Mar 6, 2022

View reviewed changes

間違った assert の引数を修正

e2093ac

ot0m1 requested a review from komagata March 7, 2022 01:03

komagata reviewed Mar 7, 2022

View reviewed changes

冗長な変数宣言を削除

ad279f9

ot0m1 requested a review from komagata March 8, 2022 11:13

komagata reviewed Mar 10, 2022

View reviewed changes

テストを分割

4b9677e

ot0m1 requested a review from komagata March 10, 2022 11:50

komagata approved these changes Mar 10, 2022

View reviewed changes

komagata merged commit 0509b9f into main Mar 10, 2022

komagata deleted the display-link-to-consultation-room-in-users-search-results branch March 10, 2022 14:34

github-actions bot mentioned this pull request Mar 10, 2022

Release 2022-03-18 01:41:12 #4380

Merged

32 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ユーザーの検索結果に、管理者でログインしたときだけそのユーザーの相談部屋へのリンクを表示 #4147

ユーザーの検索結果に、管理者でログインしたときだけそのユーザーの相談部屋へのリンクを表示 #4147

ot0m1 commented Feb 5, 2022 •

edited

Loading

ot0m1 commented Feb 9, 2022

ot0m1 commented Feb 14, 2022

haruguchi-yuma left a comment

haruguchi-yuma Feb 16, 2022

ot0m1 Feb 19, 2022

haruguchi-yuma Feb 16, 2022

haruguchi-yuma Feb 16, 2022

ot0m1 Feb 19, 2022

haruguchi-yuma Feb 21, 2022

ot0m1 Feb 22, 2022 •

edited

Loading

ot0m1 commented Feb 19, 2022

komagata Feb 21, 2022

ot0m1 Feb 22, 2022

haruguchi-yuma left a comment

ot0m1 commented Feb 22, 2022

komagata Feb 28, 2022

ot0m1 commented Mar 1, 2022

komagata Mar 1, 2022

ot0m1 Mar 2, 2022

ot0m1 commented Mar 2, 2022

komagata Mar 3, 2022

ot0m1 Mar 3, 2022

ot0m1 commented Mar 3, 2022

komagata Mar 6, 2022

ot0m1 Mar 7, 2022

ot0m1 commented Mar 7, 2022

komagata Mar 7, 2022

ot0m1 Mar 8, 2022

ot0m1 commented Mar 8, 2022

komagata Mar 10, 2022

ot0m1 Mar 10, 2022

ot0m1 commented Mar 10, 2022

komagata left a comment


		require 'test_helper'

		class SearchHelperTest < ActionView::TestCase

ユーザーの検索結果に、管理者でログインしたときだけそのユーザーの相談部屋へのリンクを表示 #4147

ユーザーの検索結果に、管理者でログインしたときだけそのユーザーの相談部屋へのリンクを表示 #4147

Conversation

ot0m1 commented Feb 5, 2022 • edited Loading

要件

画面イメージ

変更前

変更後

確認方法

ot0m1 commented Feb 9, 2022

ot0m1 commented Feb 14, 2022

haruguchi-yuma left a 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

ot0m1 Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

ot0m1 commented Feb 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haruguchi-yuma left a comment

Choose a reason for hiding this comment

ot0m1 commented Feb 22, 2022

Choose a reason for hiding this comment

ot0m1 commented Mar 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ot0m1 commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ot0m1 commented Mar 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ot0m1 commented Mar 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ot0m1 commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ot0m1 commented Mar 10, 2022

komagata left a comment

Choose a reason for hiding this comment

ot0m1 commented Feb 5, 2022 •

edited

Loading

ot0m1 Feb 22, 2022 •

edited

Loading