複数キーワードでの検索機能の追加 #1521

sanfrecce-osaka · 2020-04-11T06:25:55Z

refs #1510

概要

以下の仕様で複数キーワードでの検索を実装した

スペースで区切られた単語は別のキーワードとみなす
半角・全角スペースともに区切り文字とみなす

また、併せて Searcher.rb に対してのリファクタリングも行った

スクリーンショット

修正前

修正後

refs #1510 ### 概要スペースで区切られたキーワードは別のキーワードみなし、全てのキーワードを含むものを検索結果として返す(== AND検索)ように Searcher.search_for を修正半角スペースも全角スペースも区切り文字として許可するため、キーワードは [:blank] で split している

refs #1510 ### 概要検索可能なリソースごとに半角・全角スペース区切りで複数キーワード検索をした際に検索結果が絞り込めているかどうかを観点としてテストを追加 ### 修正内容 #### 複数キーワードでの検索のテストを追加全ての検索可能なリソースに対して3つのケースでの検証を行っている - 単一キーワードで検索した場合 - 絞り込み前の比較対象 - 半角スペース区切りの複数キーワードで検索した場合 - AND検索になっているかどうか(想定通り絞り込まれているかどうか) - 全角スペース区切りの複数キーワードで検索した場合 - AND検索になっているかどうか(想定通り絞り込まれているかどうか) ### その他の修正 - test/fixtures/pages.yml で typo があり、 expected を書く際にややこしかったため正しいスペルに修正 - Bootcmap => Bootcamp

refs #1510 ### 概要 Searcher.result_for が DRY でなかったため、インターフェースの統一やパラメータ生成処理の各検索対象の model への移動等のリファクタリングを行った ### 修正内容 #### 検索の際に呼び出し側から呼び出すメソッドを search_by_keywords に統一 Searcher から呼び出す際のインターフェースを統一するため、検索の際に呼び出すメソッドを search_by_keywords に統一した ##### Searchable に実装したことについて search_by_keywords は既存で Searchable が既にあり責務的にも適切であると判断したため、 Searchable に持たせた一方、Searchable に description というメソッドが既にあること、 Searchable が include されていない検索対象の model があったということから、当初 Searchable に持たせるかどうか迷ったが、 include されていなかった model も含めて Page 以外の全ての model で description を持っていたため、問題ないと判断した #### パラメータの生成処理を各検索対象の model に移動キーワード検索の際のパラメータ生成への関心は、 Searcher ではなくそれぞれの model が知っているべきと考えたため、パラメータ生成処理は各検索対象の model に移動した何のためのメソッドであるかを明示するため、 Module#concerning を使用している #### result_for を private メソッドに修正 Searcher.result_for は外部から呼ばれることは想定されていないはずなので、 Searcher.result_for を private メソッドに修正した

refs #1510 ### 概要 Searcher.rb で検索対象の model を表す symbol から model の名前や model のオブジェクトを動的に生成する処理が DRY でなかったかつパッと見て何をしているか分かりづらかったため、対象の処理をメソッドに切り出した

refs #1510 ### 概要 document_type による処理の振り分けで if文にベタ書きされていてリーダブルではなかったため、条件判定の処理に名前付けする目的でメソッドに切り出しまた、全ての条件式に document_type を引数として渡すのは DRY ではないため、条件判定のメソッドは lambda を返すようにして if を case に置き換えた

JunichiIto · 2020-04-12T00:25:12Z

concering、僕は普段使ってないし、あまりメリットがよくわかってないのですが、使う必要ってあるんでしょうか？
（特定の機能に関連することを強調する役割はありそうだけど、それ以上にコードが複雑化したりネストが深くなったりするデメリットの方が気になる）

あと、コードの重複が多いのでもっとDRYにできる気がします。
以前からの問題ですが、commentable_type_eqが機能しているかどうか検証するテストがないのも気になりました。

というわけで、こんな実装にしてみるのはいかがでしょう？

diff --git a/app/models/announcement.rb b/app/models/announcement.rb
index 9a61aee2..4b2bab43 100644
--- a/app/models/announcement.rb
+++ b/app/models/announcement.rb
@@ -22,19 +22,8 @@ class Announcement < ApplicationRecord
   validates :description, presence: true
   validates :target, presence: true
 
-  concerning :KeywordSearch do
-    class_methods do
-      private
-
-        def params_for_keyword_search(searched_values = {})
-          { groupings: groupings(split_keyword_by_blank(searched_values[:word])) }
-        end
-
-        def groupings(words)
-          words.map do |word|
-            { title_or_description_cont_all: word }
-          end
-        end
-    end
+  # Override
+  def self.grouping_condition
+    :title_or_description_cont_all
   end
 end
diff --git a/app/models/answer.rb b/app/models/answer.rb
index d70062ee..c06802cd 100644
--- a/app/models/answer.rb
+++ b/app/models/answer.rb
@@ -13,20 +13,9 @@ class Answer < ActiveRecord::Base
   validates :description, presence: true
   validates :user, presence: true
 
-  concerning :KeywordSearch do
-    class_methods do
-      private
-
-        def params_for_keyword_search(searched_values = {})
-          { groupings: groupings(split_keyword_by_blank(searched_values[:word])) }
-        end
-
-        def groupings(words)
-          words.map do |word|
-            { description_cont_all: word }
-          end
-        end
-    end
+  # Override
+  def self.grouping_condition
+    :description_cont_all
   end
 
   def receiver
diff --git a/app/models/comment.rb b/app/models/comment.rb
index b0512edb..b71b0daf 100644
--- a/app/models/comment.rb
+++ b/app/models/comment.rb
@@ -12,20 +12,15 @@ class Comment < ActiveRecord::Base
 
   validates :description, presence: true
 
-  concerning :KeywordSearch do
-    class_methods do
-      private
-
-        def params_for_keyword_search(searched_values = {})
-          { commentable_type_eq: searched_values[:commentable_type], groupings: groupings(split_keyword_by_blank(searched_values[:word])) }
-        end
-
-        def groupings(words)
-          words.map do |word|
-            { description_cont_all: word }
-          end
-        end
-    end
+  # Override
+  def self.grouping_condition
+    :description_cont_all
+  end
+
+  # Override
+  def self.params_for_keyword_search(searched_values = {})
+    groupings = super
+    groupings.merge(commentable_type_eq: searched_values[:commentable_type])
   end
 
   def receiver
diff --git a/app/models/concerns/searchable.rb b/app/models/concerns/searchable.rb
index f1057034..112363ad 100644
--- a/app/models/concerns/searchable.rb
+++ b/app/models/concerns/searchable.rb
@@ -8,7 +8,20 @@ module Searchable
       ransack(**params_for_keyword_search(searched_values)).result
     end
 
+    def grouping_condition
+      raise "Must implement"
+    end
+
     private
+      def params_for_keyword_search(searched_values = {})
+        { groupings: groupings(split_keyword_by_blank(searched_values[:word])) }
+      end
+
+      def groupings(words)
+        words.map do |word|
+          { grouping_condition => word }
+        end
+      end
 
       def split_keyword_by_blank(word)
         word.split(/[[:blank:]]/)
diff --git a/app/models/page.rb b/app/models/page.rb
index 98035504..2c84a904 100644
--- a/app/models/page.rb
+++ b/app/models/page.rb
@@ -7,19 +7,8 @@ class Page < ActiveRecord::Base
   validates :body, presence: true
   paginates_per 20
 
-  concerning :KeywordSearch do
-    class_methods do
-      private
-
-        def params_for_keyword_search(searched_values = {})
-          { groupings: groupings(split_keyword_by_blank(searched_values[:word])) }
-        end
-
-        def groupings(words)
-          words.map do |word|
-            { title_or_body_cont_all: word }
-          end
-        end
-    end
+  # Override
+  def self.grouping_condition
+    :title_or_body_cont_all
   end
 end
diff --git a/app/models/practice.rb b/app/models/practice.rb
index 07087dd7..fa674a26 100644
--- a/app/models/practice.rb
+++ b/app/models/practice.rb
@@ -32,20 +32,9 @@ class Practice < ActiveRecord::Base
 
   scope :category_order, -> { includes(:category).order("categories.position").order(:position) }
 
-  concerning :KeywordSearch do
-    class_methods do
-      private
-
-        def params_for_keyword_search(searched_values = {})
-          { groupings: groupings(split_keyword_by_blank(searched_values[:word])) }
-        end
-
-        def groupings(words)
-          words.map do |word|
-            { title_or_description_or_goal_cont_all: word }
-          end
-        end
-    end
+  # Override
+  def self.grouping_condition
+    :title_or_description_or_goal_cont_all
   end
 
   def status(user)
diff --git a/app/models/question.rb b/app/models/question.rb
index d50f6079..526d2a05 100644
--- a/app/models/question.rb
+++ b/app/models/question.rb
@@ -23,19 +23,8 @@ class Question < ActiveRecord::Base
   scope :solved, -> { joins(:correct_answer) }
   scope :not_solved, -> { where.not(id: CorrectAnswer.pluck(:question_id)) }
 
-  concerning :KeywordSearch do
-    class_methods do
-      private
-
-        def params_for_keyword_search(searched_values = {})
-          { groupings: groupings(split_keyword_by_blank(searched_values[:word])) }
-        end
-
-        def groupings(words)
-          words.map do |word|
-            { title_or_description_cont_all: word }
-          end
-        end
-    end
+  # Override
+  def self.grouping_condition
+    :title_or_description_cont_all
   end
 end
diff --git a/app/models/report.rb b/app/models/report.rb
index 830d9240..bc7a3c05 100644
--- a/app/models/report.rb
+++ b/app/models/report.rb
@@ -39,20 +39,9 @@ class Report < ActiveRecord::Base
   after_update ReportCallbacks.new
   after_destroy ReportCallbacks.new
 
-  concerning :KeywordSearch do
-    class_methods do
-      private
-
-        def params_for_keyword_search(searched_values = {})
-          { groupings: groupings(split_keyword_by_blank(searched_values[:word])) }
-        end
-
-        def groupings(words)
-          words.map do |word|
-            { title_or_description_cont_all: word }
-          end
-        end
-    end
+  # Override
+  def self.grouping_condition
+    :title_or_description_cont_all
   end
 
   def previous
diff --git a/test/models/searcher_test.rb b/test/models/searcher_test.rb
index a97cfb40..904ba2fa 100644
--- a/test/models/searcher_test.rb
+++ b/test/models/searcher_test.rb
@@ -182,4 +182,20 @@ class SearchableTest < ActiveSupport::TestCase
     assert_includes(result, comments(:comment_6))
     assert_not_includes(result, comments(:comment_5))
   end
+
+  test "returns only comments associated to specified document_type" do
+    result = Searcher.search("コメント", document_type: :reports)
+    assert_equal [comments(:comment_11)], result
+  end
+
+  test "returns all comments when document_type is not specified" do
+    result = Searcher.search("コメント")
+    assert_includes(result, comments(:comment_8))
+    assert_includes(result, comments(:comment_10))
+    assert_includes(result, comments(:comment_11))
+    assert_includes(result, comments(:comment_12))
+    assert_includes(result, comments(:comment_13))
+    assert_includes(result, comments(:comment_14))
+    assert_equal 6, result.size
+  end
 end

  def self.grouping_condition
    :title_or_body_cont_all
  end

みたいなコードは、

grouping_condition :title_or_body_cont_all

と、DSLっぽく書けたらオシャレだなーと思いつつ、面倒だったのでやってませんw

JunichiIto

気になった点をコメントしてみました〜。

JunichiIto · 2020-04-12T00:27:00Z

app/models/searcher.rb

+
+      def result_for(type, word, commentable_type: nil)
+        return [] unless available_type?(type)
+        type.to_s.capitalize.singularize.constantize.search_by_keywords(word: word, commentable_type: commentable_type)


メソッドを再利用した方が良さそうです。

model(type).search_by_keywords(word: word, commentable_type: commentable_type)

あわわわわ、ここも修正していたんですが rebase をミスっていました😭
修正します:bow:

0ca7b1e で修正しました:bow:

JunichiIto · 2020-04-12T00:30:56Z

app/models/searcher.rb

+    private
+
+      def result_for(type, word, commentable_type: nil)
+        return [] unless available_type?(type)


想定外のtypeが渡されたら異常事態なので例外を発生させるのがよいのでは。

raise "Unexpected type: #{type}" unless available_type?(type)

自分も最初そうしようかと思っていたんですが、適切な例外が思いつかず独自の例外クラスを作る必要があるかなと一旦既存の実装に合わせていました🤔
Kernel.#raise の第一引数に文字列を渡せば RuntimeError を raise できるんですね😍
お恥ずかしながら知りませんでした💦
ただ既存の検索で下記の挙動があったため、それも併せてどうするかを別issue に切り出してそちらでこの指摘の修正も行おうと思うのですがどうでしょうか？

見つかったバグ(後に issue を発行します)

https://bootcamp.fjord.jp/searchables?document_type=comments でコメントが検索できる

関連: #1499

あえて例外クラスを指定するならこの場合はArgumentErrorじゃないかなー、と思います。

https://docs.ruby-lang.org/ja/latest/class/ArgumentError.html

とはいえ、僕は「とりあえず落ちたことがわかればいいや」と、単純にraiseするだけのことが多いですがw

https://bootcamp.fjord.jp/searchables?document_rype=comments でコメントが検索できる

これ、バグと言えばバグかもしれないけど、ほとんど実害がない（困る人がだれもいなさそう）ので、裏メニュー的に残しておいてもいいのかなーと思ったりしたんですが、どうでしょう？

あえて例外クラスを指定するならこの場合はArgumentErrorじゃないかなー、と思います。

あーなるほど、ArgumentError は違和感ないですね！😄

これ、バグと言えばバグかもしれないけど、ほとんど実害がない（困る人がだれもいなさそう）ので、裏メニュー的に残しておいてもいいのかなーと思ったりしたんですが、どうでしょう？

自分も最初裏メニューとして残しておいていいかな、と思ったんですが「提出物」の「まだ閲覧が許可されていないコメント」が検索結果として返されてしまう状態になってしまっているのが気になっていました
提出物はカンニング防止のためにプラクティスを完了するまで閲覧できない仕様になっています
現状は View 側でコメントの内容が表示されないようにはなっているのですが、どこかに仕様の考慮漏れがあった場合に閲覧されてしまう恐れもあるので抜け穴をそのまま残しておくのはよろしくないかな〜と思った次第です:bow:
とはいえ、完了していない状態でも各プラクティスから提出物の一覧は見れる現状の仕様を考えると、現状のまま裏メニューとして残しておいてもいい気はしてきました😅

なるほど、であれば、とりあえずissueを立てて、「自分としてはこのままでもいい気がしますが、どうでしょう？」と駒形さんに確認するのがいいかも。
issueに書いておけば、同じように「コメントが検索できる！バグか！？」って思った人も「過去にも同じ議論があったんだな」と気づけますし。

例外に関しては 060508e で修正しました:bow:
issue に関しては #1528 を立てました😀

JunichiIto · 2020-04-12T00:32:19Z

app/models/searcher.rb

+      end
+
+      def available_type?(type)
+        AVAILABLE_TYPES.find { |available_type| available_type == type }.present?


type.in?(AVAILABLE_TYPES) でOK。だし、これで済むならメソッド化する必要もなさそう。

おお〜なるほど‼️
たしかにその書き方ならメソッド化しなくてもいいですね😆

35789e8 で修正しました:bow:

JunichiIto · 2020-04-12T00:36:02Z

app/models/searcher.rb

+      when all?
+        AVAILABLE_TYPES.flat_map { |type| result_for(type, word) }.sort_by { |result| result.created_at }.reverse
+      when commentable?
+        [document_type, :comments].flat_map { |type| result_for(type, word, commentable_type: model_name(document_type)) }.sort_by { |result| result.created_at }.reverse
+      when question?
+        [document_type, :answers].flat_map { |type| result_for(type, word) }


all?やquestion?メソッドが大した仕事をしていないので、こうするとか。

case document_type when :all # ... when :questions # ... when commentable? # ...

あーなるほど、case にしたからそれでいけましたね😄
見落としていました😇

a3da9e8 で修正しました:bow:

JunichiIto · 2020-04-15T23:30:19Z

@sanfrecce-osaka

concering、僕は普段使ってないし、あまりメリットがよくわかってないのですが、使う必要ってあるんでしょうか？

この件についてはどう思われますか〜？

komagata · 2020-04-16T04:16:11Z

concering、僕は普段使ってないし、あまりメリットがよくわかってないのですが、使う必要ってあるんでしょうか？
（特定の機能に関連することを強調する役割はありそうだけど、それ以上にコードが複雑化したりネストが深くなったりするデメリットの方が気になる）

@JunichiIto 僕も全く同じ意見っすね。

僕だったらプロジェクトの他のコードを見て、使われてるんだったら合わせて使うって感じかな〜。

sanfrecce-osaka · 2020-04-16T22:33:13Z

@JunichiIto @komagata

以下、 bootcamp に限らず concerning に関する個人的意見です:bow:

concering、僕は普段使ってないし、あまりメリットがよくわかってないのですが、使う必要ってあるんでしょうか？

僕も全く同じ意見っすね。

うーむ、なるほど・・・
コミュニティで他のエンジニアにも意見を聞いてみたんですが大体どこでもお二方と同意見でした
concerning 、あんまり使われてないんですねぇ😭(個人的には責務がわかりやすくなったりメソッドをグルーピングできたりして好きなんですが)

（特定の機能に関連することを強調する役割はありそうだけど、それ以上にコードが複雑化したりネストが深くなったりするデメリットの方が気になる）

複雑化に関しては、その場でモジュールを定義してその場で include するだけなので複雑化はしないんじゃないかなぁと思うんですがどうでしょうか？ 🤔
また、コードのネストは確かに深いんですが今回の場合はまあ許容範囲かなぁと思っていました😮(concerning の中に concerning をネストしたり concerning の中に多くの処理を書いたりすることはないと思っているので)

個人的には↓のようなケースがある際に concerning を使って目印・責務の明確化のために書いておくといいんではないかなーと思っています

複数のモデルで共通のインターフェースを持っている処理
各モデルごとに微妙に差異があって無理に共通化すると複雑になる処理
モジュールのメソッドをオーバーライドしているメソッド

まあ、別に機能的には変わらないので「じゃあそれはコメントでいいよね」とか「可読性は上がるかもしれないけど無用なバグを生むのでは？」と言われるとぐぬぬ、となってしまうのですが・・・😇

僕だったらプロジェクトの他のコードを見て、使われてるんだったら合わせて使うって感じかな〜。

これはたしかにそうですね・・・:thinking:
全体としての統一感は保守性・可読性にとって大事な要素ですし😀

bluerabbit · 2020-04-17T00:59:54Z

concering、僕は普段使ってない

私も使わないですね。継承より移譲の方を好みます

JunichiIto · 2020-04-17T01:16:53Z

もはや好みの世界ですね〜。
「conceringいいね！どんどん使おう！」という合意が取れているチームなら良さそうですが、ブートキャンプの場合は他に使われている場所もないですし、賛成意見も見当たらないのでconceringなしで実装するのが良さそうです。

sanfrecce-osaka · 2020-04-17T12:31:56Z

@JunichiIto
そうですね、今回は使わない方向で実装していきます:bow:

@komagata @bluerabbit
ご意見ありがとうございました:pray:

refs #1510 refs #1521 (comment) ### 概要検索可能なタイプかどうかの判定処理を可読性のためにメソッドに切り出していたが、 Symbol#in で済むためそちらを使うようにした

refs #1510 refs #1521 (comment) ### 概要 214e427 で case文に変更していて単純比較で済むものはメソッドに切り出す必要がなくなっていたそのため、 all や questions を判定する when節は単純にシンボルを書くように修正した

refs #1510 refs #1521 (comment) ### 概要下記コミットで修正漏れがあったため、該当箇所を修正した title: model の名前の動的生成及び model への変換処理をリファクタリング revision: 931559b

refs #1510 refs #1521 (comment) ### 概要各モデル毎に params_for_keyword_search 及び groupings を定義していたが、重複部分が多かったため共通部分は Searchable に定義して差分がある場合に各クラスでオーバーライドするよう修正したまた、キーワードにより検索するカラムは DSL で定義できるようにした ### 修正内容 #### 共通部分を Searchable に定義して差分がある場合に各クラスでオーバーライドするよう修正 params_for_keyword_search 、groupings を各モデルで定義していたが、重複部分がかなり多かったため重複部分は Searchable に移動した Comment のみ commentable_type が検索条件として必要になるため、オーバーライドして差分プログラミングで対応した #### キーワード検索で使用する条件をDSLで設定できるように修正キーワード検索の際にキーワードで検索されるカラムを DSL で設定できるよう　Serachable.target_column_of_keyword を追加した Serachable.target_column_of_keyword で定義されるメソッドを private化する方法は Object#__send__ で Module#private を実行する方法と tap の際に Module$private を実行する方法を検討したが、複数のプログラマーの意見、可読性を考慮して後者を選択した refs #1521 (comment) #### concerning を使用しないよう修正 bootcamp では他では concerning が使用されておらず統一感・可読性の観点から使用しないことに決定した refs #1521 (comment) refs #1521 (comment) refs #1521 (comment) refs #1521 (comment) refs #1521 (comment) refs #1521 (comment)

refs #1510 refs #1521 (comment) ### 概要 commentable_type_eq での絞込が機能しているかどうかを検証するテストを追加した ### 修正内容 #### commentable_type_eq が機能しているかどうかを検証するテストを追加コメントを検索する際は条件として commentable_type_eq を設定するが、既存ではそれが機能しているかを検証するテストがなかったそのため、下記の2件のテストを追加し commentable_type_eq が機能しているかを検証した - :all 以外の document_type を指定して検索した結果が、指定した document_type のコメントのみであるかの検証 - document_type に何も指定せずに検索した結果に、全ての document_type が含まれているかの検証 refs #1521 (comment)

sanfrecce-osaka · 2020-04-27T19:45:41Z

@JunichiIto

レビュー指摘内容を修正しました
コミット数が多くなったため、別PRに切り出しました

#1541

確認よろしくお願いします:bow:

refs #1510 refs #1521 (comment) refs #1521 (comment) ### 概要既存では想定外の type が Searcher#result_for に渡ってきた場合は空配列を返していたが、そもそも想定外の type が渡されるのは異常事態であるため、例外を発生させるように修正した #### 補足現状、修正行の例外は Unreachable であるが、下記の理由からそのままにしている - /searchables?document_type=comments で検索した際の挙動をそのまま裏仕様として残しておくため - 今後の修正で reachable になったときのための防御的プログラミング想定外の type に対する本質的な対応は #1528 (comment) への対応で行う予定 refs #1528 (comment)

JunichiIto · 2020-04-28T00:29:26Z

@sanfrecce-osaka 修正ありがとうございます。時間があるときにまたレビューしますね〜。

JunichiIto · 2020-04-30T00:06:39Z

@sanfrecce-osaka #1541 にコメント入れました〜。

@komagata

…ations を追加 refs #1541 (comment) refs #1541 (comment) ### 概要 inline での privateメソッド化がプロジェクト的に許容されているかを確認するために `private def` の形で書いていたしかしレビューにおいて @komagata さん・@JunichiIto さんの両名から使わない方がいいという意見をもらったため、グルーピングで privateメソッド化する記法に修正したまた今後同一の議論が発生しないようにするため、併せて .rubocop.yml に Style/AccessModifierDeclarations を追加した refs https://www.rubydoc.info/gems/rubocop/0.69.0/RuboCop/Cop/Style/AccessModifierDeclarations

refs #1541 (comment) ### 概要 Searchable#target_column_of_keyword で動的に定義される _grouping_condition を privateメソッド化するために define_singleton_method からメソッドチェーンで privateメソッド化していたしかし、レビューで「横に長くて読みにくい＆tapがごちゃごちゃしてわかりづらい」という意見をいただいたため、 tap で privateメソッド化していた箇所を削除したレビューではブロックが curly brace での one-line から do end での multi-line も提案されていたが、以下の理由からそのまま curry brace での記述にしている - curly brace での記述の場合、line length は 111文字 - Style/LineLength を設定している場合、 120文字に設定しているプロジェクトが結構多いように感じる - c.f. rubocop/rubocop-jp#32 (comment) - RubyMine の Code Style でのデフォルト設定が 120文字

refs #1541 (comment) refs #1541 (comment)

#1510 でのレビュー指摘内容の修正

sanfrecce-osaka · 2020-05-16T06:26:07Z

@JunichiIto

レビュー指摘分の修正(#1541) をマージしました👍

以下は別 issue・PR で対応します:bow:

検索機能の N + 1 の解消(既存の問題)
URLからコメントが検索できてしまう #1528 への対応

問題なければ Approve していただければ〜:pray:

JunichiIto · 2020-05-16T07:53:13Z

@sanfrecce-osaka 了解です。手が空いたら確認させてもらいます〜。

JunichiIto

気になった点をちょっとだけコメントしました。

JunichiIto · 2020-05-18T00:24:04Z

app/models/searcher.rb

+      when commentable?
+        [document_type, :comments].flat_map { |type| result_for(type, word, commentable_type: model_name(document_type)) }.sort_by { |result| result.created_at }.reverse
+      when :questions
+        [document_type, :answers].flat_map { |type| result_for(type, word) }


これはソートしないの？

@JunichiIto

こちら、実は気づいてはいたんですが下記の理由で別issue で対応しようと考えていました(すみません、書き忘れていました・・・😭)

question のソート順は既存の仕様(or バグ)であること

ソート順が他と同じく作成日順でいいのかについて確認・議論する必要もある

PRがかなり巨大になっていること

(今の所 Conflict はないが) master と乖離しすぎないように一旦マージしてしまいたい

先述の通り question のソート順は既存のもののため、この PR で発生したバグではない

ソート順のトピックを別issue・PR に切り出して複数キーワードでの検索機能の追加 #1521 (comment) も合わせて修正しリファクタリングも行いたい

自分は以上のように考えているんですが、どうでしょうか？😀

JunichiIto · 2020-05-18T00:25:15Z

app/models/searcher.rb

+      when :questions
+        [document_type, :answers].flat_map { |type| result_for(type, word) }
+      else
+        result_for(document_type, word).sort_by { |result| result.created_at }.reverse


DB側でソートできそう

@JunichiIto

そうですね😀
ただ #1521 (comment) の通り、別issue・PR で対応したいと考えています〜🙂

JunichiIto · 2020-05-18T00:26:24Z

app/models/searcher.rb

+    def search(word, document_type: :all)
+      case document_type
+      when :all
+        AVAILABLE_TYPES.flat_map { |type| result_for(type, word) }.sort_by { |result| result.created_at }.reverse


ひとりごと）本当は全文検索エンジンとかを使った方がいいよね、こういうの・・・。

JunichiIto

@sanfrecce-osaka 修正ありがとうございました。
たしかにソートの件は「既存の実装通り」なので、別issue/PRで対応するというのはその通りだと思います。
僕としてはこれでapproveです。長期間に渡る修正、お疲れ様でした！

@komagata
念のため最終レビュー＆マージをお願いします〜。

komagata

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

sanfrecce-osaka added 5 commits April 11, 2020 13:40

sanfrecce-osaka added the 新機能 label Apr 11, 2020

sanfrecce-osaka requested a review from komagata April 11, 2020 06:25

sanfrecce-osaka self-assigned this Apr 11, 2020

komagata temporarily deployed to bootcamp-fjord-jp-pr-1521 April 11, 2020 06:26 Inactive

JunichiIto reviewed Apr 12, 2020

View reviewed changes

sanfrecce-osaka mentioned this pull request Apr 16, 2020

URLからコメントが検索できてしまう #1528

Closed

komagata temporarily deployed to bootcamp-fjord-jp-pr-1521 April 27, 2020 12:50 Inactive

sanfrecce-osaka force-pushed the feature/#1510_search_by_multiple_keywords branch from c7b48e2 to 214e427 Compare April 27, 2020 18:34

komagata temporarily deployed to bootcamp-fjord-jp-pr-1521 April 27, 2020 18:34 Inactive

sanfrecce-osaka added 3 commits April 28, 2020 04:12

931559b での修正漏れ箇所を修正

0ca7b1e

refs #1510 refs #1521 (comment) ### 概要下記コミットで修正漏れがあったため、該当箇所を修正した title: model の名前の動的生成及び model への変換処理をリファクタリング revision: 931559b

sanfrecce-osaka added 2 commits April 28, 2020 04:15

sanfrecce-osaka mentioned this pull request Apr 27, 2020

#1510 でのレビュー指摘内容の修正 #1541

Merged

sanfrecce-osaka added 5 commits May 7, 2020 09:52

Searchable#target_column_of_keyword を columns_for_keyword_search にリネーム

389dbbe

refs #1541 (comment) refs #1541 (comment)

Searchable#columns_for_keyword_search の join前の map(&:to_s) 及び to_sym を削除

ed957df

refs #1541 (comment) refs #1541 (comment)

Merge pull request #1541 from fjordllc/feature/#1510_fix_reviewed_parts

a647965

#1510 でのレビュー指摘内容の修正

komagata deployed to bootcamp-fjord-jp-pr-1521 May 16, 2020 04:49 View deployment

sanfrecce-osaka requested a review from JunichiIto May 16, 2020 05:07

JunichiIto reviewed May 18, 2020

View reviewed changes

JunichiIto approved these changes May 18, 2020

View reviewed changes

komagata approved these changes May 19, 2020

View reviewed changes

komagata merged commit 35325a9 into master May 19, 2020

komagata deleted the feature/#1510_search_by_multiple_keywords branch May 19, 2020 14:07

yamadaab mentioned this pull request Apr 23, 2021

検索種別に提出物がない #2612

Closed

複数キーワードでの検索機能の追加 #1521

複数キーワードでの検索機能の追加 #1521

Conversation

sanfrecce-osaka commented Apr 11, 2020

概要

スクリーンショット

修正前

修正後

JunichiIto commented Apr 12, 2020

JunichiIto 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

sanfrecce-osaka Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

見つかったバグ(後に issue を発行します)

Choose a reason for hiding this comment

sanfrecce-osaka Apr 15, 2020 • 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

JunichiIto commented Apr 15, 2020

komagata commented Apr 16, 2020 • edited Loading

sanfrecce-osaka commented Apr 16, 2020 • edited Loading

bluerabbit commented Apr 17, 2020 • edited Loading

JunichiIto commented Apr 17, 2020

sanfrecce-osaka commented Apr 17, 2020

sanfrecce-osaka commented Apr 27, 2020

JunichiIto commented Apr 28, 2020

JunichiIto commented Apr 30, 2020

sanfrecce-osaka commented May 16, 2020 • edited Loading

JunichiIto commented May 16, 2020

JunichiIto 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

sanfrecce-osaka May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunichiIto left a comment

Choose a reason for hiding this comment

komagata left a comment

Choose a reason for hiding this comment

sanfrecce-osaka Apr 14, 2020 •

edited

Loading

sanfrecce-osaka Apr 15, 2020 •

edited

Loading

komagata commented Apr 16, 2020 •

edited

Loading

sanfrecce-osaka commented Apr 16, 2020 •

edited

Loading

bluerabbit commented Apr 17, 2020 •

edited

Loading

sanfrecce-osaka commented May 16, 2020 •

edited

Loading

sanfrecce-osaka May 18, 2020 •

edited

Loading