Feature/#34 add tags to memos #79

nova818 · 2024-08-17T11:40:34Z

対応するissue

closes メモのタグをメモの作成・更新時に登録できるようにする #34

対応内容

フォームオブジェクトを作成
メモの作成&更新時にタグが紐づけられるようmemoのコントローラーを修正
フォームオブジェクトのテストコードを追加
memoのコントローラーの修正に伴うテストコード修正

kakeru-one

@nova818
一旦テスト以外で気になったところをコメントしました！

backend/app/controllers/memos_controller.rb

backend/app/forms/memo_form.rb

backend/app/controllers/memos_controller.rb

backend/app/forms/memo_form.rb

…memos

…ationsのみincludeする

kakeru-one

@nova818
コメントしました！
前回よりかなりコードが整理されてきましたね！ 🎉

kakeru-one · 2024-09-06T06:35:23Z

backend/app/controllers/memos_controller.rb

@@ -16,22 +16,24 @@ def show

  # POST /memos
  def create
-    memo = Memo.new(memo_params)
-    if memo.save
+    memo_form = Memo::BuildForm.new(params: create_params)


IMO/Simple

formで伝わるかなと思います！

Suggested change

memo_form = Memo::BuildForm.new(params: create_params)

form = Memo::BuildForm.new(params: create_params)

確かにmemoつけなくて良さそうですね🙆‍♂️

kakeru-one · 2024-09-06T06:35:45Z

backend/app/controllers/memos_controller.rb

    end
  end

  # PUT /memos/:id
  def update
    memo = Memo.find(params[:id])
+    memo_form = Memo::UpdateForm.new(params: update_params, memo: memo)


こちらも同上です！

併せて修正します🫡

kakeru-one · 2024-09-06T06:36:47Z

backend/app/controllers/memos_controller.rb

  end

-  def update_memo_params
-    params.require(:memo).permit(:content)
+  def update_params


@rikuya98 @nova818
[仕様の確認]

メモはタイトルの更新はできない、でいいんでしたっけ？
（zennとかQiitaとか、esaとかだとtitleも更新できる）

@ochi-sho-private-study @nova818
僕が作成した時のissue見返したら
title更新できない仕様の指定がありました！(当時そこまで深く考えてなかったので、理由までは聞いてませんが💦)

#26

タイトルも更新可能にすることになったかと思いますので、そのような認識で進めます！

kakeru-one · 2024-09-06T06:40:21Z

backend/app/models/memo/build_form.rb

+
+    attr_reader :params
+
+    validate :memo_valid?


Must

以下のPRで使用したcascadeバリデーターを使用するようにしましょう！（使い方はPRの説明部分に書いてあります！）

子モデルのバリデーションエラーをcascadeするバリデータを追加する #81

ちなみにこれはEachValidatorを継承したカスタムバリデータです。

https://qiita.com/its532/items/5783b5953b4202e74b83#eachvalidator

memo_tagsのバリデーションも実行するようにしましょう！（memo_idとtag_idの組み合わせの一意性検証バリデーションを実行したい。）

cascadeバリデーターを利用する形に修正します！
ただ、実力不足からcascadeバリデーターのコードで詳細に理解できていない部分があり、引き続きコードリーディングを進めます...😢

kakeru-one · 2024-09-06T06:41:27Z

backend/app/models/memo/build_form.rb

+      @params = params
+    end
+
+    def memo


Must

このメソッドは外部に公開する必要はありますか？
みたところその必要性はなさそうですので、privateメソッドでいいと思いました！

確かに、memoはクラス内部でのみ仕様されるメソッドですね💦
privateメソッドに変更します🙆‍♂️

kakeru-one · 2024-09-06T08:06:06Z

backend/spec/models/memo/build_form_spec.rb

+      let(:params) { { title: 'Test Memo', content: 'This is a test memo', tag_ids: tag_ids } }
+      let(:memo_build_form) { described_class.new(params: params) }
+
+      specify 'メモが新規作成されること' do


IMO

MemoTagのカウントも確認したいです！

MemoTagのカウントを確認するコードを追加します🙆‍♂️

kakeru-one · 2024-09-06T08:06:26Z

backend/spec/models/memo/build_form_spec.rb

+      let(:memo_build_form) { described_class.new(params: params) }
+
+      specify 'メモが新規作成されないこと' do
+        expect { memo_build_form.save }.not_to(change(Memo, :count))


TagMemoのカウントも確認したいです！

kakeru-one · 2024-09-06T08:07:09Z

backend/spec/requests/memos_spec.rb

+            expect do
+              post '/memos', params: { memo_form: invalid_memo_form_params }, as: :json
+            end.not_to change(Memo, :count)
+          end.not_to change(Tag, :count)


Question

Tagではなく、MemoTagの個数を確認するべきではないでしょうか？ 🤔

MemoTagの個数を確認すべきですね。なんでタグの数を確認したんだろう...笑
確認不足でした💦 修正します🙏

kakeru-one · 2024-09-06T08:07:30Z

backend/spec/models/memo/update_form_spec.rb

BuildFormでコメントしたことを参考にして、修正してみて欲しいです！

承知しました！詳細なコメントありがとうございます🙇‍♂️

kakeru-one · 2024-09-06T08:08:07Z

backend/spec/models/memo/update_form_spec.rb

+  end
+
+  describe '#update' do
+    context 'フォームの値が有効な場合' do


既存のmemo.memo_tagsが存在する場合もテストしたいです！

こちら、テストの冒頭書き部分で既存のmemo_tagsを作成していたため、既存のmemo.memo_tagsが存在する場合のテストはできているのかなと考えていたのですが、下記とは別に改めてテストケースを追加した方が良いということでしょうか？🤔

before { create(:memo_tag, memo:, tag:) }

すみません、こちらは対応不要ですね 💦

…memos

kakeru-one

@nova818
コメントしました

kakeru-one · 2024-09-19T14:11:40Z

backend/app/models/memo/update_form.rb

IMO

memo_tags_to_removeとmemo_tags_to_addの部分が、
作成される部分と削除される部分の処理が追いづらいので、以下のように修正するといいと思います。

# frozen_string_literal: true class Memo class UpdateForm include ActiveModel::Validations validates :memo, cascade: true validates :memo_tags, cascade: true, if: -> { errors.empty? } def initialize(params:, id:) @params = params @id = id end def save return false if invalid? ActiveRecord::Base.transaction do save_record!(memo) memo.memo_tags.select(&:marked_for_destruction?) .each { destroy_record!(_1) } end errors.empty? end private attr_reader :params, :id def save_record!(record) return true if record.save errors.add(:base, record.error_message) raise ActiveRecord::Rollback end def memo @memo ||= \ Memo.find(id) .tap do |model| model.assign_attributes( title: params[:title], content: params[:content], tags: tags ) end end def destroy_record!(record) return true if record.destroy errors.add(:base, record.error_message) raise ActiveRecord::Rollback end def memo_tags @memo_tags ||= memo.tap do |model| mark_for_destruction_memo_tags! end.memo_tags.reject(&:marked_for_destruction?) end def tag_ids = params[:tag_ids] || [] def tags = Tag.where(id: tag_ids) def before_save_tag_ids @before_save_tag_ids ||= memo.memo_tags.pluck(:tag_id) end def destroy_target_tag_ids = before_save_tag_ids - tag_ids def mark_for_destruction_memo_tags! memo.memo_tags.each do |memo_tag| next unless destroy_target_tag_ids.include?(memo_tag.tag_id) memo_tag.mark_for_destruction end end end end

改善案の提示ありがとうございます🙇‍♂️
上記案に沿って修正を進めます。

kakeru-one · 2024-09-19T14:15:47Z

backend/app/models/memo/update_form.rb

+                title: params[:title] || model.title,
+                content: params[:content] || model.content,


Must

フロントからは何も編集フォームを入力しなければ、
params[:hoge]がnilではなく、createしたときの値のままでリクエストされるはずなので、この処理はおかしいと思います。

Suggested change

title: params[:title] || model.title,

content: params[:content] || model.content,

title: params[:title],

content: params[:content],

フロントからは何も編集フォームを入力しない場合、params[:hoge]がnilになると思い込んでいました💦
修正します。

kakeru-one

@nova818
コメントしました！

kakeru-one · 2024-09-23T14:57:31Z

backend/app/controllers/memos_controller.rb

-  def memo_params
-    params.require(:memo).permit(:title, :content, :poster)
+  def form_params
+    params.require(:form).permit(:title, :content, :poster, tag_ids: [])


Idiomatic

memoに関するものなので、memoをtopレベルのkeyとするようにしましょう！

Suggested change

params.require(:form).permit(:title, :content, :poster, tag_ids: [])

params.require(:memo).permit(:title, :content, :poster, tag_ids: [])

kakeru-one · 2024-09-23T14:57:51Z

backend/app/controllers/memos_controller.rb

@@ -45,7 +47,7 @@ def destroy

  private

-  def memo_params
-    params.require(:memo).permit(:title, :content, :poster)
+  def form_params


IMO/Readable

memo_paramsのままにしておきましょう！

kakeru-one · 2024-09-23T14:58:04Z

backend/app/controllers/memos_controller.rb

    end
  end

  # PUT /memos/:id
  def update
-    memo = Memo.find(params[:id])
+    # memo = Memo.find(params[:id])


このコメントは不要なので消しましょう！

こちら確認不足でした💦 余計な負担をお掛けしてしまい申し訳ありませんでした🙇‍♂️

kakeru-one

nova818 added 5 commits August 17, 2024 20:33

フォームオブジェクトを作成

204668e

memoコントローラーの修正

cafbe40

フォームオブジェクトのテストコードを追加

d012689

メモコントローラーの修正に伴うテストコード修正

bfd1147

rubocopによる修正

ea10ba2

nova818 self-assigned this Aug 17, 2024

nova818 added the backend バックエンドのissues label Aug 17, 2024

nova818 added this to the タグ機能、ページネーション機能のAPI実装 milestone Aug 17, 2024

nova818 linked an issue Aug 17, 2024 that may be closed by this pull request

メモのタグをメモの作成・更新時に登録できるようにする #34

Closed

tagのリクエストスペックを修正

f051f3e

nova818 marked this pull request as ready for review August 17, 2024 12:08

kakeru-one reviewed Aug 18, 2024

View reviewed changes

nova818 added 13 commits August 24, 2024 22:14

Merge branch 'feature/#33-add-tags-api' into feature/#34-add-tags-to-…

d6e8b58

…memos

before actionの削除

b6a46dd

ストロングパラメータのメソッド名を修正

254ac6f

インスタンス変数に格納する必要がない箇所をローカル変数に変更

b68d6ae

ActiveModel::Modelをincludeすると暗黙的に属性が生えることに気づきにくいので、ActiveModel::Valid…

2c02c71

…ationsのみincludeする

MemoFormクラスを作成と更新のクラスに分割して修正

10c9a2c

Formクラスのテストコードを修正

f565ae0

Formクラスの修正に伴うコントローラーの修正

09448e6

不要なテストコードを削除、メモのリクエストスペックを修正

cc2d285

フォームオブジェクトのスペックの場所を移動

5b889eb

フォームオブジェクトファイルの移動

f58c528

tagのfactory botを修正

d5e7a74

フォームオブジェクト保存成功時にtrueを返すようにする

7eaa5e8

kakeru-one reviewed Sep 6, 2024

View reviewed changes

nova818 added 4 commits September 9, 2024 21:57

Merge branch 'feature/#33-add-tags-api' into feature/#34-add-tags-to-…

4ea51a1

…memos

変数memo_formをformへ変更

6aa8198

memoのリクエストスペックを修正

ce68f7e

memoメソッドをprivateメソッドへ変更

58f3719

kakeru-one changed the base branch from feature/#33-add-tags-api to main September 12, 2024 09:18

nova818 added 17 commits September 17, 2024 15:12

Merge branch 'main' into feature/#34-add-tags-to-memos

1d05c00

memoメソッドとparamsメソッドをprivateメソッドへ変更

5340f06

build_formを修正

fccf66e

build_form_specの変数を簡素化

2093ce2

specifyではなくitを使う

ab13d63

build_form_specの修正

d27da1b

Merge branch 'main' into feature/#34-add-tags-to-memos

f42475e

変数の修正

89087cd

投稿者パラメータを追加

03773cc

postリクエストのテストを修正

a8facfc

Merge branch 'main' into feature/#34-add-tags-to-memos

4b1f898

cascadeバリデーターを使用

30f65a8

update_formの修正

31904f8

フォームオブジェクトのロケールファイルを修正

4a6af36

update_formのモデルスペックを修正

8397f62

メモコントローラーの修正

5e2ebae

メモのリクエストスペックを修正

005b633

kakeru-one reviewed Sep 19, 2024

View reviewed changes

nova818 added 5 commits September 23, 2024 13:33

createしたときの値のままでリクエストされるので不要

929d64d

tag_idsメソッドを作成

12757eb

update_formのmemo_tagの更新ロジック部分を修正

d42eeb4

update_formの修正に伴うrspecの修正

55ca3a2

rubocopによる修正

8c89579

kakeru-one reviewed Sep 23, 2024

View reviewed changes

nova818 added 3 commits September 27, 2024 09:15

不要なコメントを削除

df09158

ストロングパラメータの修正

fe88f22

コントローラーとリクエストスペックの修正

9c98b6b

kakeru-one approved these changes Sep 27, 2024

View reviewed changes

kakeru-one merged commit 1d6d9fb into main Sep 27, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/#34 add tags to memos #79

Feature/#34 add tags to memos #79

nova818 commented Aug 17, 2024 •

edited

Loading

kakeru-one left a comment

kakeru-one left a comment

kakeru-one Sep 6, 2024

nova818 Sep 18, 2024

kakeru-one Sep 6, 2024

nova818 Sep 18, 2024

kakeru-one Sep 6, 2024

kuri0616 Sep 11, 2024

nova818 Sep 18, 2024

kakeru-one Sep 6, 2024

nova818 Sep 18, 2024

kakeru-one Sep 6, 2024

nova818 Sep 9, 2024

kakeru-one Sep 6, 2024

nova818 Sep 18, 2024

kakeru-one Sep 6, 2024

kakeru-one Sep 6, 2024

nova818 Sep 18, 2024

kakeru-one Sep 6, 2024

nova818 Sep 18, 2024

kakeru-one Sep 6, 2024

nova818 Sep 18, 2024

kakeru-one Sep 19, 2024

kakeru-one left a comment

kakeru-one Sep 19, 2024 •

edited

Loading

nova818 Sep 23, 2024

kakeru-one Sep 19, 2024

nova818 Sep 23, 2024

kakeru-one left a comment

kakeru-one Sep 23, 2024

kakeru-one Sep 23, 2024

kakeru-one Sep 23, 2024

nova818 Sep 27, 2024

kakeru-one left a comment

	memo_form = Memo::BuildForm.new(params: create_params)
	form = Memo::BuildForm.new(params: create_params)

		title: params[:title] \|\| model.title,
		content: params[:content] \|\| model.content,

	params.require(:form).permit(:title, :content, :poster, tag_ids: [])
	params.require(:memo).permit(:title, :content, :poster, tag_ids: [])

Feature/#34 add tags to memos #79

Feature/#34 add tags to memos #79

Conversation

nova818 commented Aug 17, 2024 • edited Loading

対応するissue

対応内容

kakeru-one left a 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 Sep 19, 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

kakeru-one left a comment

Choose a reason for hiding this comment

nova818 commented Aug 17, 2024 •

edited

Loading

kakeru-one Sep 19, 2024 •

edited

Loading