メモの検索をできるようにする #72

shoutarou123 · 2024-08-04T09:37:46Z

対応するissue

closes #20 検索機能の実装

対応内容

検索機能実装が完了しましたので、ご確認お願いします。

kuri0616 · 2024-08-10T10:58:10Z

backend/app/models/memo.rb

+      private
+
+      def resolve
+        FILTERS.reduce(filter_collection) do |memo_scope, filter|
+          const_get(filter).resolve(scope: memo_scope, params: filter_params)
+        end
+      end


このprivateのresolveメソッドはどこで使用されているのでしょうか？
見た感じ、使用されていないように見えまして！間違っていたらすみません！
もし、不要であれば削除お願いします！

@rikuya98
削除いたしました。

kuri0616 · 2024-08-10T11:16:37Z

backend/app/models/memo.rb

+      def self.resolve(filter_collection:, filter_params:)
+        FILTERS.reduce(filter_collection) do |memo_scope, filter|
+          const_get(filter).resolve(scope: memo_scope, params: filter_params)
+        end
+      end


引数の名前ですが、個人的には
resolveメソッドの引数の名前ですが個人的にはfilter_collectionよりはmemosの方がわかりやすいかなと思いました

def self.resolve(memos:, filter_params:) FILTERS.reduce(memos) do |memo_scope, filter| const_get(filter).resolve(scope: memo_scope, params: filter_params) end end

メモの集合を受け取ってreduceメソッドのブロック引数でmemo_scopeという流れの方が、引数のメモでフィルタリングするという意図が伝わりやすいかなと！

@rikuya98
確認ありがとうございます。修正いたしました。

kuri0616 · 2024-08-10T11:30:27Z

ブランチ名は

feature/search

ではなくドキュメントの開発フローに沿って命名していただければと思います！(次回からで大丈夫です）
https://github.com/Progaku-copy/progaku-archive/blob/main/docs/getting-started/team_dev_flow.md
今回でいうと

feature/#20-implement-memo-search

とかになるかと思います（あくまで一例ですが）
ブランチ名を明確にすることで
どのissueと紐づいているのか、何を目的としたブランチなのかが明確になります！
そうすることで例えば後から参画した人が仕様を理解するために参照する時、レビュアーがレビューする時など認知負荷の軽減に繋がります！

searchだけだと何の「検索」なのかが一目でわからないため、まずはそこを理解するためにソース読み解く必要があるためです！

kuri0616 · 2024-08-11T06:07:33Z

backend/spec/requests/memos_spec.rb

+  describe 'SearchResolver' do
+    let!(:test_memo_first) { create(:memo, title: 'テスト タイトル１', content: 'テスト コンテンツ１') }
+    let!(:another_title_memo) { create(:memo, title: 'その他 タイトル', content: 'その他 コンテンツ') }
+    let!(:test_memo_third) { create(:memo, title: 'テスト タイトル２', content: 'テスト コンテンツ２') }
+
+    context 'タイトルで検索した場合' do
+      it 'タイトルフィルターが正しく機能することを確認する' do
+        filter_params = { title: 'テスト' }
+        result = Memo::SearchResolver.resolve(filter_collection: Memo.all, filter_params: filter_params)
+        expect(result).to contain_exactly(test_memo_first, test_memo_third)
+      end
+    end


こちらに記載された検索機能のテストですが、これだとSearchResolverモジュールのresolveメソッドのユニットテスト(単体テスト）になるように思いました！
メソッド単体が、正常に動作しているかをテストするのであればとても良いのですが、今回はrequest_specで、所定のAPIエンドポイントにリクエストを送り、期待するレスポンスが返ってくるのか？という観点のテストになるのでそのような形式でテストを行うことが望ましいと思います！

具体的には以下のようになるかと思います(例です）

describe 'GET /memos?title={title}' do context 'タイトルで検索した場合' do let!(:searchable_memo_1) { create(:memo, title: 'テストタイトル１', content: 'テストコンテンツ１') } let!(:searchable_memo_2) { create(:memo, title: 'テストタイトル２', content: 'テストコンテンツ２') } let!(:non_searchable_memo) { create(:memo, title: 'その他タイトル', content: 'その他コンテンツ') } it 'タイトルフィルターが正しく機能し、期待されるメモが取得できることを確認する' do aggregate_failures do get '/memos', params: { title: 'テスト' } expect(response).to have_http_status(:ok) expect(response.parsed_body['memos'].length).to eq(2) result_memo_ids = response.parsed_body['memos'].map { |item| item['id'] } expected_memo_ids = [searchable_memo_1.id, searchable_memo_2.id] expect(result_memo_ids).to eq(expected_memo_ids) end end end end

@rikuya98
確認ありがとうございます。また例文も大変助かります。
こちらを参考に修正させていただきました。

kuri0616 · 2024-08-11T06:11:47Z

backend/spec/requests/memos_spec.rb

@@ -154,4 +154,50 @@
      end
    end
  end
+
+  describe 'SearchResolver' do


describeですが他のテストと合わせて、エンドポイントのパスを記載するようにしていただきたいです！

describe 'GET /memos?title={title}'

このファイルはrequests specなので、
describe 'GET /memos' doのブロック内に書きましょうか。
また別のコメントでりくやさんがおっしゃっている通り、SearchResolverのテストはmodels specに書くようにしましょうか〜

@rikuya98
内容修正させていただきました。

@ochi-sho-private-study
おつかれさまです。
こちらご指摘いただいた内容について、modelsフォルダ内のmemo_spec.rbに記述するとうことでしょうか？
それともrequestsフォルダ内にmodels_spec.rbを作成し、そちらに記述するということでしょうか？

@shoutarou123
modelsディレクトリは単体テストを記載するディレクトリ
requestsディレクトリは結合テストを記載するディレクトリというイメージですね！

なのでSearchResolverの単体の動きを確認するテストは
modelsフォルダ内のmemo_spec.rbに記述する

そして僕が以下の例で記載した処理に関しては、新たにdescribe 'GET /memos?title={title}' というブロックを作るのではなくすでに存在する「describe 'GET /memos' do」のブロック内に記載するという意味かと思います！

describe 'GET /memos?title={title}' do
context 'タイトルで検索した場合' do
let!(:searchable_memo_1) { create(:memo, title: 'テストタイトル１', content: 'テストコンテンツ１') }
let!(:searchable_memo_2) { create(:memo, title: 'テストタイトル２', content: 'テストコンテンツ２') }
let!(:non_searchable_memo) { create(:memo, title: 'その他タイトル', content: 'その他コンテンツ') }
..etc

kuri0616 · 2024-08-11T06:14:36Z

backend/spec/requests/memos_spec.rb

+    let!(:test_memo_first) { create(:memo, title: 'テスト タイトル１', content: 'テスト コンテンツ１') }
+    let!(:another_title_memo) { create(:memo, title: 'その他 タイトル', content: 'その他 コンテンツ') }
+    let!(:test_memo_third) { create(:memo, title: 'テスト タイトル２', content: 'テスト コンテンツ２') }


変数名ですが
test_というプレフィクスはなくても良いかなと思いました！(テストに使用するということは明確なため)

let!(:searchable_memo_1) { create(:memo, title: 'テストタイトル１', content: 'テストコンテンツ１') } let!(:searchable_memo_2) { create(:memo, title: 'テストタイトル２', content: 'テストコンテンツ２') } let!(:non_searchable_memo) { create(:memo, title: 'その他タイトル', content: 'その他コンテンツ') }

例えば上記のようにどれが検索対象のデータで、どれが対象外のデータなのか、わかるようにする方が、読んでいてわかりやすいかなと個人的に思いました！

@rikuya98
修正させていただきました。

kakeru-one

@shoutarou123
コメントしました！

kakeru-one · 2024-08-11T17:49:11Z

backend/app/models/memo.rb

+    # @param filter_collection [ActiveRecord::Relation[Memo]]
+    # @param filter_params [ActionController::Parameters]
+    # @return [ActiveRecord::Relation[Memo]]
+    def self.resolve(filter_collection:, filter_params:)


IMO/Simple

filter_paramsではなくて、シンプルにparamsでいいと思いました！

@ochi-sho-private-study
ありがとうございます。内容修正させていただきました。

kakeru-one · 2024-08-11T18:13:01Z

backend/app/controllers/memos_controller.rb

+    memos = Memo.all
+    @memos = Memo::SearchResolver.resolve(filter_collection: memos, filter_params: params)


Simple/Performant

ここで変数に格納してしまうと、
全てのレコード分のActiveRecordのインスタンスが格納されてしまうので、
メモリをかなり消費してしまいます。
（参考文献にも挙げてますが、ActiveRecordのインスタンスは意外と重いのです。）

また、今回だとmemos = Memo.allという風に変数に格納してしまっているので、
ここでクエリが一度発行されてしまって、
Memo::SearchResolverでも一回発行してしまうので無駄なクエリが発行されてしまうと思います。

したがって、以下のように変数に格納しないように書いて、
メモリに載せないようにしましょう。

Suggested change

memos = Memo.all

@memos = Memo::SearchResolver.resolve(filter_collection: memos, filter_params: params)

@memos = Memo::SearchResolver.resolve(filter_collection: Memo.all, filter_params: params)

参考文献

ActiveRecordがかなりメモリを消費する件

https://tech.smarthr.jp/entry/2021/11/11/151444

メモリ消費量を見る

https://zenn.dev/kthatoto/articles/40cb6f7c1c6421

@ochi-sho-private-study
ありがとうございます。
内容修正させていただきました。

kakeru-one · 2024-08-11T18:20:24Z

backend/app/models/memo.rb

+    # @param filter_params [ActionController::Parameters]
+    # @return [ActiveRecord::Relation[Memo]]
+    def self.resolve(filter_collection:, filter_params:)
+      filter_params[:order] ||= 'desc'


Readable/Simple

ここでfilter_paramsの値を格納するのは、
resolveメソッドの副作用になってしまうので、わかりづらいと思います。
（ここでいう副作用とは、本来期待している動作以外の作用を指します。）

以下のように、OrderFilterに閉じた動作にするのはどうでしょうか？
こうすることで、早期リターンも必要がなくなりシンプルになると思います。

module OrderFilter DEFAULT_ORDER = 'desc' private_constant :DEFAULT_ORDER def self.resolve(scope:, params:) scope.order(id: params[:order].presence || DEFAULT_ORDER) end end

@ochi-sho-private-study
ありがとうございます。ご指摘のとおり修正させていただきました。

kakeru-one · 2024-08-11T18:27:42Z

backend/app/models/memo.rb

+    private
+
+    def resolve
+      filter_params[:order] ||= 'desc'


こちらは二重に実行する必要がないと思います。

@ochi-sho-private-study
ありがとうございます。削除させていただきました。

kakeru-one

@shoutarou123
コメントしました！

kakeru-one · 2024-09-03T02:42:02Z

backend/spec/models/memo_spec.rb

+    let!(:searchable_memo_one) { described_class.create(title: 'テスト タイトル１', content: 'テスト コンテンツ１') }
+    let!(:searchable_memo_two) { described_class.create(title: 'テスト タイトル２', content: 'テスト コンテンツ２') }
+    let!(:non_searchable_memo) { described_class.create(title: 'その他 タイトル', content: 'その他 コンテンツ') }


Idiomatic/Simple

factorybotを利用してテストデータを作成するようにしましょう！

以下のように記述できるかと思います。

let!(:memos) do { '1' => create(:memo, title: 'テストタイトル１', content: 'テストコンテンツ１'), '2' => create(:memo, title: 'テストタイトル２', content: 'テストコンテンツ２'), '3' => create(:memo, title: 'その他タイトル', content: 'その他コンテンツ') } end

@ochi-sho-private-study
修正しました。テストコードがシンプルになりました。勉強になります。

kakeru-one · 2024-09-03T02:49:30Z

backend/spec/models/memo_spec.rb

@@ -81,4 +81,55 @@
      end
    end
  end
+
+  describe '検索機能のテスト' do


Idiomatic

テストの対象を具体的にしましょうか！

Suggested change

describe '検索機能のテスト' do

describe 'SearchResolver::resolve(memos:, params:)' do

@ochi-sho-private-study
修正いたしました。

kakeru-one · 2024-09-03T02:55:16Z

backend/app/models/memo.rb

@@ -13,4 +13,46 @@
 class Memo < ApplicationRecord
  validates :title, :content, presence: true
  has_many :comments, dependent: :destroy
+
+  module SearchResolver


提案

ralisのデザインパターン的にはQueryパターンになるので、
Queryモジュールにしてもらってもいいですか？
（最初に私が命名したのがミスリードでした 🙇）

@ochi-sho-private-study
修正させていただきました。

let後の命名を変更

SearchResolverの変数名変更

ﾀｲﾄﾙﾌｨﾙﾀｰﾃｽﾄ内容修正

self.resolveﾒｿｯﾄﾞのﾃﾞﾌｫﾙﾄ降順削除。 module OrderFilterﾃﾞﾌｫﾙﾄで降順になるよう変更

Searchﾃｽﾄ内容修正

検索に関するテストコード全て削除

検索機能のテストコードを追加

docker compose run backend rake ridgepole:apply実行

併せてmemo_spec.rbも修正

kuri0616 · 2024-09-03T02:33:09Z

backend/app/controllers/memos_controller.rb

+    @memos = Memo::SearchResolver.resolve(memos: Memo.all, params: params)
+    render json: { memos: @memos }, status: :ok


ここはインスタンス変数で必要はないと思いました！
Viewなどに値を渡すことがないためです！

@rikuya98
確認ありがとうございます。修正させていただきました。

kuri0616 · 2024-09-08T07:32:19Z

backend/spec/models/memo_spec.rb

+        aggregate_failures do
+          result = Memo::Query.resolve(memos: described_class.all, params: { content: 'コンテンツ' })
+          expect(result).to include(memos['1'], memos['2'], memos['3'])


aggregate_failuresは複数の期待値を一度に、検証したい時に使用するのでここでは必要ないかなと思いました！
(1つの期待値しか検証していないため(expectが1回))

https://qiita.com/jnchito/items/3a590480ee291a70027c

@rikuya98
修正させていただきました。

kuri0616 · 2024-09-08T07:32:41Z

backend/spec/models/memo_spec.rb

+        aggregate_failures do
+          result = Memo::Query.resolve(memos: described_class.all, params: { order: 'asc' })
+          expect(result).to eq([memos['1'], memos['2'], memos['3']])


こちらも同様です！

@rikuya98
修正させていただきました。

kuri0616 · 2024-09-08T07:37:52Z

backend/spec/models/comment_spec.rb

-# Foreign Keys
-#
-#  fk_comments_memo_id  (memo_id => memos.id)
-#
 RSpec.describe Comment do


今回の変更とは関係ない差分が反映されてしまっているため、除外していただきたいです🙇

git revertでコミット済の変更を破棄することができます！

https://qiita.com/chihiro/items/2fa827d0eac98109e7ee

「アプリ再セットアップ」というコミットが対象のコミットなのでこのコミットIDを指定していただければと思います！
コミットIDはコミットを一意に識別するIDで
git logコマンドか
このGitHubの各コミットのページの右上あたりにも表示されています！

@rikuya98
修正させていただきました。

kuri0616 · 2024-09-08T07:38:08Z

backend/spec/factories/comments.rb

 # Table name: comments
 #
 #  id              :bigint           not null, primary key
-#  content(内容)   :text(65535)      not null
+#  content(内容)   :string(1024)     not null
 #  created_at      :datetime         not null
 #  updated_at      :datetime         not null
-#  memo_id(メモID) :bigint           not null
+#  memo_id(メモID) :integer          not null
 #
 # Indexes
 #
 #  index_comments_on_memo_id  (memo_id)
 #
-# Foreign Keys
-#
-#  fk_comments_memo_id  (memo_id => memos.id)
-#


こちらも今回の変更とは関係ない差分が反映されてしまっているため、除外していただきたいです🙇

@rikuya98
修正させていただきました。

kuri0616 · 2024-09-08T07:38:56Z

backend/app/models/comment.rb

-#  content(内容)   :string(1024)      not null
+#  content(内容)   :string(1024)     not null
 #  created_at      :datetime         not null
 #  updated_at      :datetime         not null
-#  memo_id(メモID) :bigint           not null
+#  memo_id(メモID) :integer          not null
 #
 # Indexes
 #
 #  index_comments_on_memo_id  (memo_id)
 #
-# Foreign Keys
-#
-#  fk_comments_memo_id  (memo_id => memos.id)
-#


こちらも今回の変更とは関係ない差分が反映されてしまっているため、除外していただきたいです🙇

@rikuya98
修正させていただきました。

This reverts commit ad0fbfd.

kakeru-one

@shoutarou123
LGTM!

shoutarou123 added 2 commits July 14, 2024 14:44

gem ransack installしbundle install実施

89aadee

Merge branch 'main' of https://github.com/Progaku-copy/progaku-archive

157a963

kuri0616 reviewed Aug 10, 2024

View reviewed changes

kuri0616 reviewed Aug 11, 2024

View reviewed changes

kakeru-one reviewed Aug 11, 2024

View reviewed changes

shoutarou123 closed this Aug 28, 2024

shoutarou123 reopened this Aug 28, 2024

kakeru-one assigned shoutarou123 Aug 29, 2024

kakeru-one reviewed Sep 3, 2024

View reviewed changes

shoutarou123 force-pushed the feature/search branch from 7191b2c to 1789adb Compare September 5, 2024 14:27

kakeru-one force-pushed the feature/search branch from 9211bf3 to 66a8c22 Compare September 6, 2024 06:26

kakeru-one changed the title ~~検索機能実装完了~~ メモの検索をできるようにする Sep 6, 2024

Merge remote-tracking branch 'origin'

cf62912

shoutarou123 force-pushed the feature/search branch from 66a8c22 to b5ef67b Compare September 7, 2024 07:49

shoutarou123 added 15 commits September 7, 2024 21:00

ransackに関する不要コミットを統合

0e7ebea

memo.rb 不要なスペース削除

a10daea

memos_spec.rb describe resolveﾒｿｯﾄﾞのﾃｽﾄを削除

12f553b

let後の命名を変更

memo.rb resolveﾒｿｯﾄﾞ削除

25e3efb

memo.rb self.resolveﾒｿｯﾄﾞの引数名修正

0b4812a

memos_controller indexｱｸｼｮﾝ resolve引数名修正

83368c7

memos_spec.rb 各resolve引数名修正

7bca1d2

memos_spec.rb

7b07215

SearchResolverの変数名変更

memos_spec.rb describeをﾊﾟｽに変更

5ecb43d

memo.rb, memos_controller, memos_specのfilter_paramsをparamsに変更

601c4c1

memos_controller indexｱｸｼｮﾝ記述変更

6e0f933

memos_spec.rb

f01d2e2

ﾀｲﾄﾙﾌｨﾙﾀｰﾃｽﾄ内容修正

memo.rb

a8653f6

self.resolveﾒｿｯﾄﾞのﾃﾞﾌｫﾙﾄ降順削除。 module OrderFilterﾃﾞﾌｫﾙﾄで降順になるよう変更

memos_spec.rb

11a96f1

Searchﾃｽﾄ内容修正

spec/requests/memos_spec.rb

f2622ff

検索に関するテストコード全て削除

shoutarou123 added 4 commits September 7, 2024 21:08

spec/models/memo_spec.rb

50b2e1b

検索機能のテストコードを追加

構文エラーによりprivate削除

6015f5f

memo_spec.rb 構文エラーにより修正

7dc6b64

requests/memos_spec.rb 構文エラーにより末尾に改行追加

b71d0e4

shoutarou123 force-pushed the feature/search branch from b5ef67b to b71d0e4 Compare September 7, 2024 12:09

shoutarou123 added 4 commits September 7, 2024 21:28

アプリ再セットアップ

ad0fbfd

docker compose run backend rake ridgepole:apply実行

memo_spec.rb 検索のdescribe,テストデータ修正

58685f4

memo.rbのmodule名を変更

e035914

併せてmemo_spec.rbも修正

memos_controller index修正

1ca5cdf

kuri0616 reviewed Sep 8, 2024

View reviewed changes

shoutarou123 added 3 commits September 8, 2024 21:25

Revert "アプリ再セットアップ"

648c9c7

This reverts commit ad0fbfd.

memos_controller indexｱｸｼｮﾝのｲﾝｽﾀﾝｽ変数を削除

d05a86a

memo_spec.rb 不要なaggregate_failuresを削除

5de0ef0

kakeru-one approved these changes Sep 9, 2024

View reviewed changes

kakeru-one merged commit dd9dfd2 into main Sep 9, 2024
1 check passed

		memos = Memo.all
		@memos = Memo::SearchResolver.resolve(filter_collection: memos, filter_params: params)

	describe '検索機能のテスト' do
	describe 'SearchResolver::resolve(memos:, params:)' do

		@memos = Memo::SearchResolver.resolve(memos: Memo.all, params: params)
		render json: { memos: @memos }, status: :ok

メモの検索をできるようにする #72

メモの検索をできるようにする #72

Conversation

shoutarou123 commented Aug 4, 2024

対応するissue

対応内容

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

kuri0616 commented Aug 10, 2024

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

Choose a reason for hiding this comment

kakeru-one 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

kakeru-one Aug 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

kakeru-one 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

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

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

kakeru-one left a comment

Choose a reason for hiding this comment

kakeru-one Aug 11, 2024 •

edited

Loading