Railsで質問・回答アプリを作ってください（実装編）の課題提出 #1

Shun712 · 2021-12-19T16:51:30Z

質問回答アプリの実装をしました。
コードレビューお願い致します。

苦労したこと

レイアウトデザインで使用するbootstrapを使った実装に時間がかかってしまった。
slimではインデント数の違いでビューが表示されないこと。
画像のサイズをvariantメソッドを使って表示させること。
メール通信機能の実装でアドレスをダブルクォーテーションを囲っていなくて、メールが送れないエラーを解決するのに、時間がかかってしまった。

疑問点

app/controllers/questions_controller.rbのindexメソッド内の条件分岐で、真偽値であるtrue・falseをシングルクォーテーションで囲まなければならないのか？
検索で無記入で検索するとすべてのリソースが対象になるのか？

できなかったこと

ユーザー登録画面でプロフィール画像のデモ画面が表示されないこと。
app/views/questions/index.html.slimで解決・未解決のタブボタンをbootstrapのサンプル通りに実装できなかった。
Checkbox and radio button groups
app/views/questions/show.html.slimで質問内容のテキストを左寄せにできなかった。
app/views/questions/show.html.slimで、回答内容が空だったときにエラーメッセージが表示されないこと。

工夫したところ

極力部分テンプレートを使用した。
N+1問題が発生するところで、includesメソッドを使用した。
ユーザーがわかりやすいようなデザインにした。

miketa-webprgr · 2021-12-21T15:26:42Z

app/controllers/questions_controller.rb

+  def destroy
+    @question.destroy
+    redirect_to questions_url, notice: "質問「#{@question.title}」を削除しました。"
+  end


ここも同じく、current_userで絞り込みましょう！

上記と同じです。

上記と同じ間違いです 🙇‍♂️ 🙇‍♂️ 🙇‍♂️
すみません！！！

miketa-webprgr · 2021-12-21T15:28:38Z

app/controllers/users_controller.rb

+  def edit
+    @user = User.find(params[:id])
+  end


current_userで絞り込んであげたほうがよさそうです！

以下に書き換えました。

before_action :set_user, only: %i[show edit update destroy] private def set_user @user = current_user end

ちなみに、自分で指摘をしておきながら、何か違和感がある感じがしなくもないので、違ったらすみません！

miketa-webprgr · 2021-12-21T15:28:54Z

app/controllers/users_controller.rb

+  end
+
+  def update
+    @user = User.find(params[:id])


current_userで絞り込んであげたほうがよさそうです！

上記と同じです。

miketa-webprgr · 2021-12-21T15:29:08Z

app/controllers/users_controller.rb

+  end
+
+  def destroy
+    @user = User.find(params[:id])


current_userで絞り込んであげたほうがよさそうです！

上記と同じです。

miketa-webprgr · 2021-12-21T15:30:14Z

app/models/question.rb

+class Question < ApplicationRecord
+  validates :title, presence: true, length: { maximum: 30 }
+  validates :content, presence: true
+  before_validation :set_titleless_title


おお、before_validation、いいですね！

今回は参考書通りですが、他でも意識できるようにしたいです。

before_validation初めて聞きました。勉強させていただきます。

miketa-webprgr · 2021-12-21T15:30:30Z

app/models/question.rb

+    def set_titleless_title
+      self.title = 'タイトルなし' if title.blank?
+    end


こういう親切設計、いいと思います！

上記と同じです。

あまり意識していなかったので見習います！

miketa-webprgr · 2021-12-21T15:31:03Z

app/models/question.rb

@@ -0,0 +1,14 @@
+class Question < ApplicationRecord
+  validates :title, presence: true, length: { maximum: 30 }


ここだけじゃないですが、validationをきちんと考える姿勢、いいですね！

（参考書通りですが）ありがとうございます！

参考書を参考にするの、大事！！！

miketa-webprgr · 2021-12-21T15:34:59Z

app/views/questions/search.html.slim

+    table.table.table-hover
+      thread.thread-default
+        tr
+          th= User.human_attribute_name('投稿者')


単純に'投稿者'としてもよい気がしますが、ymlファイルの方で後ほど定義するのを視野に入れているのであれば、一旦この形でもいいかと思います！

なるほど、リリースする段階ではきちんと変数で定義したほうがいいのですね。

と言いますか、

Question.human_attribute_name(:title)

こういう書き方するなら、意図がすごくよく分かるんですけれども、

User.human_attribute_name('投稿者')

こうやって書いている場合、普通に'投稿者'と書けるのに、なぜあえてこうしているんだろうと気になるという感じですね。

config/locales/ja.ymlで定義をするか、それか'投稿者’を直書きにするかのどちらかがいいと思います！

config/locales/ja.ymlに定義して統一します。

miketa-webprgr · 2021-12-21T15:43:10Z

app/views/questions/show.html.slim

+.container
+  .row
+    p.text-left
+      = auto_link(simple_format(h(@question.content), {}, sanitize: false, wrapper_tag: "div"))


auto_link、初めて知りました！

似たようなgemとして、rinkuというのもあるみたいですね！
https://github.com/vmg/rinku

ご紹介してくださりありがとうございます。
調べて活用してみたいと思います。

miketa-webprgr · 2021-12-21T15:44:36Z

config/environments/development.rb

@@ -35,6 +35,9 @@

  config.action_mailer.perform_caching = false

+  config.action_mailer.default_url_options = { host: 'localhost:3000' }
+  config.action_mailer.delivery_method = :letter_opener_web


おお、lettter_opener_web、いいですね！

簡単にメールを確認できますよね。

miketa-webprgr · 2021-12-21T15:46:02Z

config/routes.rb

+      get 'search'
+    end
+  end
+  post '/questions/:id/answers', to: 'answers#create'


勘違いかもしれないですが、ここと重複してそうです！

resources :questions do resources :answers, only: %i[create]

申し訳ございません。消し忘れです。
気をつけます。

miketa-webprgr · 2021-12-21T15:50:03Z

db/schema.rb

+  create_table "answers", force: :cascade do |t|
+    t.integer "question_id", null: false
+    t.integer "user_id", null: false
+    t.text "content", null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+  end


外部キーにindexを貼ってあげると、検索が早くなるかと思います！
（データ量が少ないと、体感変わらないですが）

この記事がベストか微妙ですが、少し調べてみてください！
DBのインデックスと複合インデックス - Qiita

存在は知っていましてが、理解が足りなかったので実装しませんでした。
ご紹介してくださりありがとうございます。

miketa-webprgr · 2021-12-21T15:51:34Z

db/schema.rb

+
+  create_table "questions", force: :cascade do |t|
+    t.string "title", limit: 30, null: false
+    t.text "content"


textにlimitをかけてあげてもいいかもしれないです！
（titleよりも、contentの方がより多くの文字数を入れ込まれる可能性があり、DBを圧迫されるかもしれないので）

DBについても配慮が必要ですね。
気をつけます。
db:rollbackして追記した場合、schema.rbに
t.text "content", limit: 500, null: false
が反映されましたが、
rails g migration ChangeQuestionsContentLimit500
をした場合、schema.rbに
ActiveRecord::Schema.define(version: 2021_12_24_215427) do
にしか違いが反映されませんでした。
どちらも違いないと思っていたのですが、前者のほうが適切ですか？

db:rollbackして追記した場合、schema.rbに
t.text "content", limit: 500, null: false
が反映されましたが、
rails g migration ChangeQuestionsContentLimit500
をした場合、schema.rbに
ActiveRecord::Schema.define(version: 2021_12_24_215427) do
にしか違いが反映されませんでした。

migrationファイルの中身を見ないと、何とも言えないですね！
どういった中身でした？

db:rollbackして追記した場合
20211209100927_create_questions.rbに下記を追記

class CreateQuestions < ActiveRecord::Migration[5.2] def change create_table :questions do |t| t.string :title, limit: 30, null: false t.text :content, limit: 500, null: false t.boolean :solved_check, default: false, null: false t.timestamps end end end

create_table "questions", force: :cascade do |t| t.string "title", limit: 30, null: false t.text "content", limit: 500, null: false t.boolean "solved_check", default: false, null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false t.integer "user_id", null: false t.boolean "admin", default: false, null: false t.index ["user_id"], name: "index_questions_on_user_id" end

rails g migration ChangeQuestionsContentLimit500をした場合
マイグレーションファイルに下記を作成

class ChangeQuestionsContentLimit500 < ActiveRecord::Migration[5.2] def change def up change_column :questions, :content, :text, limit: 500 end def down change_column :questions, :content, :text end end end

schema.rb

ActiveRecord::Schema.define(version: 2021_12_29_205612) do . . create_table "questions", force: :cascade do |t| t.string "title", limit: 30, null: false t.text "content" t.boolean "solved_check", default: false, null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false t.integer "user_id", null: false t.boolean "admin", default: false, null: false t.index ["user_id"], name: "index_questions_on_user_id" . .

miketa-webprgr · 2021-12-21T15:52:57Z

db/seeds.rb

+User.create!(
+  name: '管理者',
+  nickname: 'マネージャー',
+  email: 'manager@example.com',
+  password: 'password',
+  password_confirmation: 'password',
+  admin: true
+)


こんな感じでシードデータまで用意したのはポイント高いですね！
Shunさん以外、たぶんほぼいないんじゃないかと思います！

ユーザーや質問があった場合のレイアウトを見てみたかったので使いました。

keit1722 · 2021-12-25T14:11:06Z

app/controllers/admin/questions_controller.rb

+
+  def destroy
+    @question.destroy
+    redirect_to questions_url, notice: "質問「#{@question.title}」を削除しました。"


勘違いだったらすいません、、、削除が終わったら/admin/questionsではなく/questionsにリダイレクトされるということでよろしいでしょうか？

ご指摘の通り私のミスです。
正しくはadmin_questions_pathに書き直しました。
ありがとうございます。

keit1722 · 2021-12-25T14:12:31Z

app/controllers/admin/questions_controller.rb

@@ -0,0 +1,27 @@
+class Admin::QuestionsController < ApplicationController
+  before_action :required_admin
+  before_action :set_q, only: %i[index search]


変数宣言の共通化できていてすごいです！自分はできていなかったので見習います、、、

おお、レビューありがとうございます。
DRY大切ですよね！実装し忘れてしまうことも多いですが、、

keit1722 · 2021-12-25T14:20:02Z

app/controllers/admin/users_controller.rb

+
+    def required_admin
+      redirect_to root_path unless current_user.admin?
+    end


app/controllers/admin/questions_controller.rbにもrequired_adminメソッドが定義されていたと思います。
2箇所に全く同じメソッドが定義されているので共通化した方が良いと思うのですが、ベースコントローラーに定義すると1箇所だけに定義するだけで済むと思います！

ご指摘ありがとうございます。おっしゃる通りです。
共通化いたします。

keit1722 · 2021-12-25T14:40:32Z

app/controllers/answers_controller.rb

+
+  def create
+    @question = Question.find(params[:question_id])
+    @answer = @question.answers.build(user_id: current_user.id, content: params[:answer][:content])


せっかく書いたanswer_paramsメソッドがどこにも使われていないように見受けられます！
もし使うのであれば（というかストロングパラメータなので使った方が良いと思います）、こういう書き方でもいいかもしれません！
mergeを使う書き方は賛否両論あるかもしれませんがご了承ください、、、

@answer = @question.answers.build(answer_params.merge(user_id: current_user.id))

ストロングパラメータは確かに必要ですね！
実装いたします。

keit1722 · 2021-12-25T14:58:35Z

app/controllers/questions_controller.rb

+  end
+
+  def update
+    @question.update!(question_params)


update!だと何かエラーが起きたときに例外が発生するので、バリデーションのエラーなどが返ってこないと思います！（自分も指摘されたことがあります、、、）

↓参考
Railsアプリケーションにおけるエラー処理（例外設計）の考え方 - Qiita

例外処理という言葉きくとドキッとしますが、目を背けず勉強したいと思います。

例外処理という言葉きくとドキッとします

エラー画面が嫌いってことなんですかね？笑

keit1722 · 2021-12-26T08:41:34Z

app/mailers/sample_mailer.rb

+    @user_from = params[:user_from]
+    @user_to = params[:user_to]
+    @question = params[:question]
+    mail(to: "#{@user_to}.email", subject: "#{@user_from}さんが質問を投稿しました。")


こちらの↓「クォーテーションを付けないとメール送信ができなかった件」なのですが、
https://github.com/keit1722/q_and_a_app/pull/1/files#r775154806

もう一度メール部分のコードを読ませていただきました！
@user_toには送信先のユーザー全員情報が配列で入っていように思えます。
その場合、送信先（to:）に配列が指定されているので上手く送信できなかったのではないかと推測しています！
ちなみに、宛先に指定したユーザー全員に送信できていますか？

宛先ユーザーの配列をループして一人ひとりのemail宛に送信すれば上手くいくのではないかと思いました！（クォーテーションがなくても送信できるかも？）

間違っていたらすいません、、、

keitさんの仮説が正しそう。

2パターンやりようがありますね。

一つ目
ユーザーの数分send_when_questionを呼ぶ。

receivers.each do |user| SampleMailer.with(user_from: current_user, user_to: user, question: @question).send_when_question.deliver_now end

二つ目
toにはメールアドレスの配列を渡すこともできるのでそれを活用する

mail(to: @user_to.map { |user| user.email } ", subject: "#{@user_from}さんが質問を投稿しました。")

ですが二つ目は基本やっちゃダメですね。
理由としてはメールの宛先が他の人にもバレるので。
例えばAさんとBさんにメールを送ったとして、Aさんの受信メールを見るとTo(宛先)にBさんも含まれていることがわかってしまう。

ご指摘ありがとうございます。
eachメソッドで一人ひとりに送るという方法も考えたのですが、別で実装したいと思ったのでこの形になりました。確認してみましたが、一人にしかメール通知できていなかったので実装を変えたほうがいいかもしれません。

一人にしかメール通知できていなかった

これも宛先のアドレスが不正なので送られないはず（letter_openerでは宛先がなんであれ送られてるように見えるけど）

satoshitodaka

コメントしましたのでよろしくお願いします！

satoshitodaka · 2021-12-26T11:54:02Z

app/models/question.rb

+    def set_titleless_title
+      self.title = 'タイトルなし' if title.blank?
+    end


あまり意識していなかったので見習います！

satoshitodaka · 2022-01-02T22:13:25Z

db/schema.rb

+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.integer "user_id", null: false
+    t.boolean "admin", default: false, null: false


理解できてなくてすみません。
Questionsにはどうしてadminカラムを設けるんですか？
ざっとみた感じ、使っていないような気がしました。

すみません、自分でもなぜ設定したのかわかりませんでした。
参考書に従って設定したのかもしれませんが削除します。
ご指摘ありがとうございます！

satoshitodaka · 2022-01-03T00:47:03Z

app/views/layouts/application.html.slim

+  body.text-center
+    .navbar.navbar-expand-md.navbar-light.bg-light
+      .container-fluid
+        .navbar-brand= link_to(image_tag('/logo.png', size:'75x50'), root_url)


オリジナルのロゴ使うの良いですね！！

ネットのフリー素材から拝借してきたものです。

satoshitodaka · 2022-01-03T00:49:58Z

db/schema.rb

+    t.string "email", null: false
+    t.string "password_digest", null: false
+    t.string "nickname"
+    t.string "picture"


自信がないのですが。。。（違ったらごめんなさい）
Active Strageを使って画像をUserに紐付ける場合、カラムを設ける必要はありましたっけ？

カラムを設定する、という記事も見かけましたが
実際のところは必要ないんじゃないかと思います。（記事が誤っているか古い？）
Railsガイドでも触れていないような気がします。

https://railsguides.jp/active_storage_overview.html

ご指摘ありがとうございます！
確かにUsersテーブルにカラムを設ける記載が見当たりませんね！
実際にカラムを削除してアバター画像が登録できるが試しました。

class RemovePictureFromUsers < ActiveRecord::Migration[5.2] def up remove_column :users, :picture end def down add_column :users, :picture, :string end end

このマイグレーションファイルを実行してカラムを削除し、アバター画像が登録できました。
やはり画像用のカラムは必要ありませんでした。
非常に勉強になりました！！

Shun712 added 30 commits December 9, 2021 19:03

slimとbootstrapを導入

a7348fa

ja.ymlのダウンロード

7fc3b7a

質問モデルとデータベースを作成

e88f511

質問コントローラーとビューを作成

4764245

質問投稿機能の実装

06ecaef

一覧表示機能の実装

79595fb

質問詳細機能の実装

443ebcc

質問編集機能の実装

eeb825f

質問削除機能の実装

9e1ee9c

質問モデルに検証を追加

29f1dfd

質問モデルにコールバックを追加

782458a

Userモデルの作成とbcryptの導入

79a5a54

ユーザー管理機能一式を追加

784c39c

ログイン・ログアウト機能を実装

869c0b0

ログインしてなければアプリを使用できないようにする

129bba3

UserとQuestionを紐付け

704d693

管理機能を管理者ユーザーだけに利用させる

446aa95

Questionコントローラにフィルタを使い重複を避ける

358b1ea

質問内容に含まれるURLをリンクとして表示する

a4d18f4

プロフィール画像の実装

3546b22

回答機能の実装

7f42282

管理画面用の質問一覧ページと削除機能の実装

ff57eb4

管理画面用ログイン機能を実装

423a962

一般ユーザーの一覧表示や管理用ユーザーとコントローラの修正

41fd2e4

一般ユーザーの質問一覧の表示を修正

03ec295

質問を解決済み状態に変更できるよう実装

3d66d42

質問一覧を未解決と解決済みで条件分岐

caeefbd

質問一覧と詳細を修正

381b115

メール通知機能の実装

7a23f95

質問表示を日本時間の投稿順に変更

0213763

miketa-webprgr reviewed Dec 21, 2021

View reviewed changes

keit1722 reviewed Dec 25, 2021

View reviewed changes

keit1722 reviewed Dec 26, 2021

View reviewed changes

Shun712 added 8 commits December 30, 2021 10:42

untracked files on rspec: df440f1 ファイルを変更

832c2a6

index on rspec: df440f1 ファイルを変更

cd1334b

WIP on rspec: df440f1 ファイルを変更

7a2d4c7

index on rspec: df440f1 ファイルを変更

4631e12

WIP on rspec: df440f1 ファイルを変更

c404106

Merge commit 'stash@{0}' into development

0ca1af8

Merge commit 'stash@{1}' into development

5fd3d9c

レビューをもとにファイル変更

2a7b4e7

satoshitodaka reviewed Jan 3, 2022

View reviewed changes

Shun712 merged commit 9021a01 into main Feb 4, 2022

		@@ -0,0 +1,14 @@
		class Question < ApplicationRecord
		validates :title, presence: true, length: { maximum: 30 }

Railsで質問・回答アプリを作ってください（実装編）の課題提出 #1

Railsで質問・回答アプリを作ってください（実装編）の課題提出 #1

Conversation

Shun712 commented Dec 19, 2021

苦労したこと

疑問点

できなかったこと

工夫したところ

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

miketa-webprgr Dec 26, 2021 • edited

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

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

Shun712 Dec 29, 2021 • edited

Choose a reason for hiding this comment

miketa-webprgr Dec 21, 2021 • edited

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

keit1722 Dec 26, 2021 • edited

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

satoshitodaka 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

satoshitodaka Jan 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miketa-webprgr Dec 26, 2021 •

edited

Shun712 Dec 29, 2021 •

edited

miketa-webprgr Dec 21, 2021 •

edited

keit1722 Dec 26, 2021 •

edited

satoshitodaka Jan 3, 2022 •

edited