-
Notifications
You must be signed in to change notification settings - Fork 71
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
最後の回答から1週間経過したら質問を投稿した人に通知されるようにした #5587
最後の回答から1週間経過したら質問を投稿した人に通知されるようにした #5587
Conversation
40b9a16
to
b8e9048
Compare
app/notifiers/discord_notifier.rb
Outdated
@@ -29,7 +29,7 @@ def hibernated(params = {}) | |||
def tomorrow_regular_event(params = {}) | |||
params.merge!(@params) | |||
event = params[:event] | |||
webhook_url = params[:webhook_url] || ENV['DISCORD_ALL_WEBHOOK_URL'] | |||
webhook_url = params[:webhook_url] || Rails.application.secrets[:webhook][:admin] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
今回の変更とは関係ありませんが、tomorrow_regular_event
メソッドのwebhook_urlの取得方法を変更しました。変更前の取得方法では、テスト時にエラーが起きてしまうためです。(定期イベント機能についてはまだ本番環境でリリースされておらずコメントアウトされているらしいです。)
こちらのPR #5542 にてsecrets
から取得する方法に変更することが駒形さんから許可が出ており、本PRでもそのように変更しました。
お疲れさまです!こちらレビュー頂いてもよろしいでしょうか?全く急いでないのでいつでも大丈夫です😄ご都合が合わない等ありましたらお気軽にご連絡ください😄 |
@keiz1213 |
@pachikuriii |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keiz1213
こんばんは🌛
ナカムラさん、大変お待たせしました🙏ばっちり、動作確認できました🎉新たな機能を作られるにあたってメソッドやテストの新規作成にしっかり対応されていてすごいなぁ…と思いました!!!
レビューさせていただく過程で通知のしくみや類似のPRがどう対応してきたか?など私なりに調べてみましたが、間違っている点などありましたら教えていただけるとうれしいです〜 🙏
よりよいコードになりますように…と思いつつ、今のわたしが出来る範囲でレビューさせていただきました👀🌻5点コメントしております。
app/models/question.rb
Outdated
@@ -34,6 +34,26 @@ class Question < ApplicationRecord | |||
|
|||
mentionable_as :description | |||
|
|||
class << self | |||
def notify_of_pending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notify_of_pending
の命名につきまして🌸
ベストアンサーを選ぶことを促す通知にまつわるメソッドなので、notify_to_chose_correct_answer
みたいな感じはどうかな〜??と思いました
notify_of_pending
でも理解できるんですが、質問の投稿が何らかの理由で保留されている or WIPになっているという捉え方もできるような気がしました。
あくまでご提案まで…!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確かにそのような場合も考慮すると、ちょっと抽象的すぎる気がしました💡色々考えたのですが、提案いただいたメソッド名を参考に修正しました(わかりやすさを重視するとちょっと長いのもしょうがないかな?と思いました)。
app/models/question.rb
Outdated
end | ||
|
||
def a_week_after_last_answer? | ||
answers.last.updated_at.since(1.week) == Date.current.to_time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated_at
の場合は、タイポなど何らかの理由でコメントを後から編集した場合、1週間のカウントの起点が後ろ倒しにならないかな?と気になりました。カウントの起点がユーザーの操作によって変動しないように、created_at
にした方がいいのかな〜?と思ったのですが、いかがでしょうか…?🍵
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確かにタイポの修正程度でカウント処理を後ろ倒しにするのは良くないですね💡なのでこちらをcreated_at
基準でカウントするように修正しました。
方針としては、last_answer
メソッドを追加して最後の回答オブジェクトを取得し、そのcreated_at
を計算の起点に使うようにしました。メソッドを追加した理由はupdated_at
をcreated_at
に変更するだけだと「正確な最後の回答」が取得できないためです。
|
||
# required params: question, receiver | ||
def a_week_after_last_answer | ||
@user = @receiver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
他のメソッドではどれも@user = @receiver
とされてはいるんですが、@user
への代入はせず、直接@receiver
を参照するようにした方がコードが読みやすくなりそうだなぁ〜と思いました🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この@userはメールのビューで使うため代入しています👍
自分も最初@receiver
でいいかな?と思ったのですが、こちらを代入せずに書くとメールが送信されなくなるのでこのままにしています💡
app/views/notification_mailer/a_week_after_last_answer.html.slim
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この@userはメールのビューで使うため代入しています👍
そうだったのですね〜!影響範囲を調査しきれていませんでした…教えていただきありがとうございます✨
test/fixtures/questions.yml
Outdated
|
||
question15: | ||
title: 最後の回答から1週間経過した時に質問者に通知するテスト用 | ||
description: 最後の回答から1週間経過した時に質問者に通知するテスト用 | ||
user: kimura | ||
created_at: <%= Date.current.to_time - 1.week %> | ||
published_at: <%= Date.current.to_time - 1.week %> | ||
updated_at: <%= Date.current.to_time - 1.week %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
既に未解決の質問のフィクスチャが用意されているので、新しいデータを投入せずに既にあるデータを活用してあげるのはいかがでしょうか…?
(Fixtureに書かれたデータはテスト実行前にすべてDBに投入されるようなので、システムテストを重くしないように…という観点から)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確かに他のPRでもそのようなことが書いてありましたね💡見逃していました😅こちらご提案頂いたように、テスト内でデータを作成するように修正いたしました。
test/fixtures/answers.yml
Outdated
|
||
last_answer: | ||
description: 最後の回答から1週間経過した時に質問者に通知するテスト用 | ||
user: machida | ||
question: question15 | ||
created_at: <%= Date.current.to_time - 1.week %> | ||
updated_at: <%= Date.current.to_time - 1.week %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちらもシステムテストを重くしないように…という観点からテスト内でAnswer.create!
などでデータを作成できないかなぁ…🤔と思ったりしました…🙏参考まで…!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちらも提案頂いたように、テスト内でデータを作成するように修正いたしました👍
2ffcd84
to
84ea321
Compare
とても丁寧なレビューありがとうございます!😄そして修正が遅くなってすみません🙏 |
app/models/question.rb
Outdated
end | ||
|
||
def a_week_after_last_answer? | ||
last_answer.created_at.since(1.week) == Date.current.to_time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これは今日の0:00を返す処理ですが、意図した挙動でしょうか?
Date.current.to_time
#=> 2022-10-12 00:00:00 +0900
この意図が正しいなら、 Time#beginning_of_day の方が分かりやすいかなと思いました。
Time.current.beginning_of_day
#=> Wed, 12 Oct 2022 00:00:00.000000000 JST +09:00
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コメントありがとうございます!🙌
これは今日の0:00を返す処理ですが、意図した挙動でしょうか?
この挙動は意図していなくて、0:00
を返す処理ということを知らずに書いてました・・・。
そして再度手元で再現してみたのですが期待した結果にならないことがわかりました・・・。
テストが通っていたのは、テストデータの時刻が全て0:00
で揃っていたので通ってたみたいで不備に気づけませんでした。本番環境ではこのように時刻が揃っていることはありえないので条件式をdateオブジェクトで比較するように変更しました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keiz1213
たいへんお返事がおそくなり申し訳ないです😭
修正いただきありがとうございます・・・!🌸3点コメントさせていただきました。
|
||
# required params: question, receiver | ||
def a_week_after_last_answer | ||
@user = @receiver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この@userはメールのビューで使うため代入しています👍
そうだったのですね〜!影響範囲を調査しきれていませんでした…教えていただきありがとうございます✨
def last_answer | ||
answers.max_by(&:created_at) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このQuestionのクラス中でしか使われないメソッドであれば、privateメソッドにした方が役割がはっきりするのかな?と思いました。いかがでしょうか…?🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
今回追加したメソッドはオブジェクトを返すためだけのものなのでprivateにする必要が無いかなと個人的に思いました💡
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
判断として間違ってないと思いますが、下記の文章だと意味が違っちゃうかも?
今回追加したメソッドはオブジェクトを返すためだけのものなのでprivateにする必要が無いかなと個人的に思いました💡
last_answer
がなぜpublicなのかというと
「他の用途でも便利に使えそうな汎用的なメソッドだから」
だと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keiz1213 そして、publicなメソッドであればテストが必要になります〜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「他の用途でも便利に使えそうな汎用的なメソッドだから」
なるほどです。privateは安全性を高めるためにのみ必要だと思っていました。確かに外から呼び出せたら便利です。
そしてこちらのテストも追加いたしました。ご確認お願いいたします🙏
@@ -282,4 +282,30 @@ class Notification::QuestionsTest < ApplicationSystemTestCase | |||
assert_text 'yameoさんが退会しました。' | |||
assert_no_text 'kimuraさんから質問「タイトルtest」が投稿されました。' | |||
end | |||
|
|||
test 'notify to questioner when a week has passed since last answer' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
私の手元だとこのテストが必ず落ちてしまいまいまして...ナカムラさんのお手元ではいかがでしょうか・・・?
エラーメッセージと、テストが失敗した時のスクリーンショットを参考までにはっておきます…🙏!
Error:
Notification::QuestionsTest#test_notify_to_questioner_when_a_week_has_passed_since_last_answer:
Capybara::ExpectationNotMet: expected to find css ".card-list-item.is-unread" at least 1 time but there were no matches
test/system/notification/questions_test.rb:308:in `block in <class:QuestionsTest>'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほどです〜。僕の環境ではテストは通りました💡
スクショを見た感じだと、Vueのコンポーネントが描画されてないようですね🤔通知自体は届いているようなのでwebpack関係とかですかね?僕はwebpack関係は苦手でまだよく理解してないのですが、bin/rails webpacker:clean
やbin/rails webpacker:clobber
とか試してもらってみてもよろしいでしょうか?もしくはnodeのバージョンが違ってたりとかですかね・・?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keiz1213
わたしがレビューする立場にもかかわらず、こうして見立てをくださりありがとうございます😭おっしゃるとおり、webpackerでコンパイルが上手くいっていなかったようでbin/rails webpacker:clobber
とすることで正常にテストが通ることを確認しました。
131e4e5
to
593ec4b
Compare
@pachikuriii a_week_after_last_answer?メソッドを修正しました@sinsoku さんからアドバイスいただきまして、 この修正によって変更確認方法を修正しました。今まではコンソールで作成頂いていた質問・回答の作成時刻を 大変お手数おかけしますが、お時間があります時に再度確認お願いします🙏 ご指摘いただいたことで不備に気づくことができました。ありがとうございます! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keiz1213
修正をありがとうございます。
ナカムラさんのPRは確認手順の書き方がとても丁寧で、今後真似していきたいです
またa_week_after_last_answer?
での条件式について、レビューの際に時刻へ意識が及んでおらず失礼しました🥲経過日時などがキーになる機能を実装する時に意識しておかなければいけないポイントについて、レビューさせていただいたことで学ばせていただきました👀
わたしからは、Approveとさせていただきます✨
@@ -282,4 +282,30 @@ class Notification::QuestionsTest < ApplicationSystemTestCase | |||
assert_text 'yameoさんが退会しました。' | |||
assert_no_text 'kimuraさんから質問「タイトルtest」が投稿されました。' | |||
end | |||
|
|||
test 'notify to questioner when a week has passed since last answer' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keiz1213
わたしがレビューする立場にもかかわらず、こうして見立てをくださりありがとうございます😭おっしゃるとおり、webpackerでコンパイルが上手くいっていなかったようでbin/rails webpacker:clobber
とすることで正常にテストが通ることを確認しました。
@pachikuriii こちらチームの方からapproveいただきましたのでご確認お願いいたします! |
app/models/notification.rb
Outdated
@@ -36,7 +36,8 @@ class Notification < ApplicationRecord | |||
product_update: 17, | |||
graduated: 18, | |||
hibernated: 19, | |||
signed_up: 20 | |||
signed_up: 20, | |||
a_week_after_last_answer: 21 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「1週間後」というのはこの通知の本質ではなく、
「ベストアンサーが選ばれていない」ことを通知するものなので、そういった意味の名前にしたいです。
1週間後は本質ではないので、2週間後に変更されるかもしれませんし、頻繁に変わりやすい内容になっています。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほどです💡こちら修正いたしました。
その他、仕様変更に対応しやすいように
- コード全体で「1週間後」という文言を使わず、「一定期間」という文言を使うようにしました
- Answerに一定期間経過したかどうかを判定するメソッドを追加しました。
ご確認よろしくお願いいたします。
54c6170
to
5b58ad7
Compare
app/models/notification.rb
Outdated
@@ -36,7 +36,8 @@ class Notification < ApplicationRecord | |||
product_update: 17, | |||
graduated: 18, | |||
hibernated: 19, | |||
signed_up: 20 | |||
signed_up: 20, | |||
not_yet_chosen_correct_answer: 21 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
極力簡潔な名前がいいのでno_corrent_answer
とかはどうでしょうか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほどです💡
難しく考えすぎていたかもしれません。確かにその名前が簡潔で良いと思いました。
提案いただいた内容で修正しました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
すみません一応確認ですが、no_corrent_answer
ではなくno_correct_answer
が正しいですよね?その前提で修正しました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keiz1213 すみません、タイプミスでした〜
@@ -25,4 +25,8 @@ def receiver | |||
def path | |||
Rails.application.routes.url_helpers.polymorphic_path(question, anchor: anchor) | |||
end | |||
|
|||
def certain_period_has_passed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このメソッドのテストはありますか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちら新たにテストを作成致しました。ご確認お願いいたします。
b54ccde
to
74881bb
Compare
app/javascript/question-edit.vue
Outdated
tagsParamName='question[tag_list]', | ||
tagsType='Question', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
今回のissueとは関係がありませんが、mainをリベースした後にlinterでテストが落ちるようになったので修正しました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conflictの修正をお願いします〜
f8e9b5b
to
7e9071f
Compare
@komagata 以前までは通知処理をQuestionのクラスメソッドで定義しそれをdaily_contorollerで呼び出す形でしたが、今回の変更に伴いそのクラスメソッドを削除し、daily_contoroller内のプライベートメソッドで定義して呼び出すように変更しました。 度々申し訳ありませんがご確認お願いいたします。 |
おっと、controllerのprivateメソッドで処理するのはとりあえずな感じのやり方で、モデルクラスのメソッドにする方が望ましい感じです〜(Fat Controller) |
7e9071f
to
dde1a9d
Compare
1. 通知処理の名前変更 2. Answerクラスに一定期間経過したかどうかを判定するメソッドを追加 3. a_week_after_last_answerメソッド削除
…se_correct_answerを削除した
ca3a051
to
394a569
Compare
なるほど、勉強になります。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確認させて頂きました。OKです〜🙆♂️
issue
概要
質問に対する最後の回答から1週間が経過し、ベストアンサーが決まっていない場合に質問を投稿した人にサイト内通知とメール通知が行くようにしました。
背景
質問した人がベストアンサーを決め忘れ、closeされないまま未解決の質問として残る問題が背景にあります。
変更確認の準備
定期イベントのdiscord通知をコメントアウトする
変更を確認する際に、曜日によっては定期イベントのdiscord通知が動いてしまうので、コメントアウトしてください。
変更確認方法
feature/notify-after-one-week-from-last-answer
をローカルに取り込むrails c
で2週間前の質問を2つ作成するrails s
でサーバー起動rails c
で、1つ目の質問に現在から6日前の日付で回答を作成するrails c
で、2つ目の質問に現在から7日前の日付で回答を作成する