Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

メンターはデフォルトでQ&AをWatch中にする #3120

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions app/controllers/questions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ def create
@question = Question.new(question_params)
@question.user = current_user
if @question.save
create_mentors_watch
notify_to_chat(@question)
redirect_to @question, notice: '質問を作成しました。'
else
Expand Down Expand Up @@ -95,4 +96,20 @@ def questions_property
QuestionsProperty.new('未解決の質問一覧', '未解決の質問はまだありません。')
end
end

def create_mentors_watch
Watch.insert_all(watch_records) # rubocop:disable Rails/SkipsModelValidations
end

def watch_records
User.mentor.map do |mentor|
{
watchable_type: 'Question',
watchable_id: @question.id,
created_at: Time.current,
updated_at: Time.current,
user_id: mentor.id
}
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. メンターの数だけSQLが発行される(N+1)

メンターが10人いると10回INSERT文が発行されて遅くなってしまいます。

insert_all! を使って一回のSQLでレコードを追加するように直す方が良さそうです。

2. Watchの登録が途中で終わる可能性がある

メンターが10人いて、5人目のWatch登録でエラーになると残りはWatchにならずにエラー画面が表示されてしまいそうです。
対応方針は @komagata さんに伺った方が良いかも。

  1. 現状のまま途中でエラーが出たら、そのままエラーにする
    • Watchの登録に失敗することは稀だと思うので、エラー出てから考えるでも良い気はします
  2. エラーを記録しつつ、残りの登録を継続して、最後にエラーを発生させる
    • エラーになったメンターの人を記録するので、コードが少し複雑になる

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sinsoku
アドバイスありがとうございます!
insert_allを使用する形に修正してみました(^^
但し、rubocopにisert_allは使わないようにメッセージが出てしまいましたので、駒形さんに相談して更に対応したい思います。🙇‍♂️
rubocopのメッセージ:Rails/SkipsModelValidations: Avoid using insert_all because it skips validations.

@komagata
遅くなりすみません。「質問を投稿するとメンターのみ"Watch中"にする」機能を修正しました。
メソッド名の変更しました。すみませんが、以下の確認をお願い致します。

  • メソッド名:create_mentors_watchは妥当でしょうか。
  • insert_allは使用しない方がよろしいでしょうか。
  • sinsokuさんのご指摘のように「メンターが10人いて、5人目のWatch登録でエラーになると残りはWatchにならずにエラー画面が表示されてしまいそう」のような想定をした方がよろしいでしょうか。それとも、別の想定がよろしいでしょうか。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sinsoku
ご連絡遅くなってすみません。
駒形さんにも伺って、今回はrubocopの指摘を優先することとなりました。
とは言え、アドバイスとても勉強になりました。ありがとうございました。🙇‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yamadaab 大変申し訳ありません、改めて考えてみたところメンター12人ぐらいいるので速度的にinsert_allを使った方がいいように思いました。

お手数ですがinsert_allにしてrubocopの指摘はコメントを使って無効化していただければありがたいです。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@komagata
時間がかかりすみません。rubocopに手間取りましたが、無事に修正できたと思います。
ご確認よろしくお願い致します。

end
10 changes: 10 additions & 0 deletions test/system/questions_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -277,4 +277,14 @@ class QuestionsTest < ApplicationSystemTestCase
click_link '2', match: :first
assert_selector '.thread-list-item', count: 25
end

test 'mentor create a question' do
visit_with_auth new_question_path, 'komagata'
within 'form[name=question]' do
fill_in 'question[title]', with: 'メンターのみ投稿された質問が"Watch中"になるテスト'
fill_in 'question[description]', with: 'メンターのみ投稿された質問が"Watch中"になるテスト'
click_button '登録する'
end
assert_text 'Watch中'
end
end