Rails 商品出品機能のコードレビュー指摘事項メモ

まみまま

TECH CAMP最終課題の商品出品機能のコードレビューで指摘いただいた箇所を自分の備忘録としてメモ。

コードレビュー1回目

  def create
    @item = Item.create(items_params)
    if @item.save
      redirect_to root_path
    else

@item = Item.new(items_params)に修正しましょう。

保存の処理の流れは、newメソッドで保存したいデータ（インスタンス）を生成し、saveメソッドでバリデーションおよび保存を行います。
createメソッドでは条件分岐前に保存処理が行われてしまいます。

メモ：def createのときはModel.createを記述すると思い込んでいたのが原因。カリキュラムにもこういった場合はModel.newと記載されていた。
下記、ChatGPTにModel.createとModel.newの使い分けを尋ねた回答。

一般的に、データベースにレコードをすぐに保存したい場合はcreateメソッドを使用し、保存前にモデルオブジェクトを操作したり、追加の処理を行いたい場合はnewメソッドを使用します。

ChatGPTの回答より

コントローラーファイル内に「ログアウトユーザーの場合は、ログインページへ遷移させる」実装を追記しましょう。

メモ：ビューファイル内で実装していた。

# views/items/index.html.erb

<%= link_to user_signed_in? ? new_item_path : new_user_session_path, class: 'purchase-btn' do %>
  <span class='purchase-btn-text'>出品する</span>
  <%= image_tag 'icon_camera.png' , size: '185x50' ,class: "purchase-btn-icon" %>
<% end %>

→ itemsコントローラーでの実装に修正。

# app/controllers/items_controller.rb

class ItemsController < ApplicationController
  before_action :authenticate_user!, only: :new

最初、onlyオプションをつけ忘れトップページに戻れなくなり一瞬焦るなどありながら。
ちなみに、カリキュラムで習った時はapplicationコントローラーにauthenticate_user!の記述をしていたので念のためChatGPTに質問。

セッション管理や認証などの機能を特定のモデルに対して制限したい場合は、そのモデルに関連するコントローラーにそれらの機能を記述します。

ChatGPT回答より
# app/javascript/calc.js （ファイル名は自分で命名）

  });
};

window.addEventListener('turbo:load', calc);

一度出品に失敗した際にも、販売手数料等の計算が行われるように以下の記述を追加しておきましょう。

window.addEventListener('turbo:render', calc);

メモ：出品に失敗したとき販売手数料が再計算されないことは確かに気になっていた。Issueの補足情報の「出品に失敗した場合に、画像・販売手数料・販売利益の欄が空欄になることは問題ない。」の趣旨を読み違えていた。例え仕様でOKだとしても（そんな仕様ないよね）、気になることは自分で改善する意識を持って取り組んでいこう。

# app/models/item.rb

validates :price, numericality: { greater_than_or_equal_to: 300, less_than_or_equal_to: 9_999_999 }

価格が整数値の場合のみ許可するように、設定を加えましょう。

現状では、小数値でも許可してしまうと思われます。

メモ：バリデーションについては特に学習が足りないと思う。色々なパターンを経験していきたい。
ちなみに、ご指摘のとおり小数値でも許可されてしまっていた。
下記に修正。

validates :price, numericality: { only_integer: true, greater_than_or_equal_to: 300, less_than_or_equal_to: 9_999_999 }

numericality

このヘルパーは、属性に数値のみが使われていることをバリデーションします。デフォルトでは、整数値または浮動小数点数値にマッチします。これらの冒頭に符号がある場合もマッチします。

値として整数のみを許すことを指定するには、:only_integerをtrueに設定します。これにより、属性の値に対するバリデーションで以下の正規表現が使われます。

/\A[+-]?\d+\z/

Railsガイド：Active Record バリデーションより引用

ちなみに、エラーメッセージは「Price must be an integer」。きっとテストコードの修正もご指摘いただいているはず。

# spec/models/item_spec.rb

      it 'category_idがでは登録できない' do
        @item.category_id = ''
        @item.valid?
        expect(@item.errors.full_messages).to include("Category can't be blank")
      end

ActiveHashを利用しているカラムについては「〇〇が"---"では登録できない」というテストを記述しましょう。

理由
以下のバリデーションを確認するためです。

validates :category_id, :sales_status_id, :shipping_fee_status_id, :prefecture_id, :scheduled_delivery_id,
            numericality: { other_than: 1, message: "can't be blank" }

メモ：これまで学んできた単体テストのパターンとして「〇〇が空では登録できない」で思考停止していたことが原因。何をテストするのか意味を考えること。
下記に修正（ActiveHashを利用しているカラム全て同様に）。

      it 'category_idが"---"では登録できない' do
        @item.category_id = 1
        @item.valid?
        expect(@item.errors.full_messages).to include("Category can't be blank")
      end

「priceが小数値だと登録できない」テストコードのご指摘はなかったですが、きっと次には出てくると思うので追加しておきます。

      it 'priceが小数値だと登録できない' do
        @item.price = '300.33'
        @item.valid?
        expect(@item.errors.full_messages).to include('Price must be an integer')
      end

レビュー1回目の修正はここまで！

コードレビュー2回目

1回目の修正はすべてご確認いただけたようで、他のメンターさんから追加の修正が届きました。LGTM（Looks good to me）って初心者からすると不思議な言い回しで、普通にOKじゃダメなのかなって思っていましたが、あくまでもレビュアーさんのto meなわけで、抜け漏れがあったとしても自分の責任だよってことですよね（きっと）。

以下のテストも実施しましょう。

userが紐付いていない場合は登録できない

理由
アソシエーションを示すbelongs_to :userには、「ItemはUserに属している必要がある」制約が含まれています。
よってテストでの確認も必要になるためです。

メモ：これも、何を確認するのかという意識がまだ足りなかったことの現れ。READMEを確認すること。
テストコードに以下を追加。

 # spec/models/item_spec.rb

　　　　　　　　 　　it 'userが紐づいていない場合は登録できない' do
        @item.user = nil
        @item.valid?
        expect(@item.errors.full_messages).to include('User must exist')
      end

自分がやりがちなミスは「@item.user = ''」。Chat GPTに聞いたところ、Userのインスタンスが期待されるところで空の文字列が渡されていてアソシエーションの型が一致しないためエラーが発生するとのこと。「@item.image =」のときもそうだったけれど、実体のあるものは文字列じゃないから「nil」というイメージかな。

RSpecのエラーメッセージによると、userが空の文字列 '' で設定されているため、Userのインスタンスが期待されるところで文字列が渡されています。このような場合、アソシエーションの型が一致しないためエラーが発生します。

通常、userが紐づいていない場合は、nilを設定します。nilはRubyにおいて、何も値が設定されていないことを表します。Railsのアソシエーションでは、関連付けられるオブジェクトが存在しない場合にnilが許容されます。

そのため、@item.user = nilとしてuserを設定すると、userが紐づいていないことを明示できます。これにより、アソシエーションの型が一致し、エラーが解消される可能性があります。

ChatGPTの回答より

こちらで無事、LGTMいただきました！
次は商品一覧表示機能の実装に入ります。

この記事が気に入ったらサポートをしてみませんか？