ベストアンサー決定通知をactive_delivery化する #6479

ogawa-tomo · 2023-04-27T12:14:36Z

Issue

ベストアンサー決定通知をactive_delivery化したい #5889

概要

ベストアンサー決定通知をactive_delivery化しました。
対象は、メール通知とサイト内通知の2種類です。
チャットの通知は変更していません。

変更確認方法

feature/replace-notification-on-correct-answer-save-with-active-deliveryをローカルに取り込む
kimuraでログイン（メール通知がONのユーザーならOK）
Q&Aページから適当な質問にアクセスして回答する
ログアウトし、3で回答した質問の質問者でログイン
3で回答した質問にアクセスし、回答の「ベストアンサーにする」をクリック
http://localhost:3000/letter_opener/ にアクセスし、kimuraに「…kimuraさんの回答がベストアンサーに選ばれました。」というメールが飛んでいることを確認
メールの「回答へ」をクリックすると、回答に遷移することを確認
ログアウトし、もう一度kimuraでログイン
ページ右上の通知で、「…kimuraさんの回答がベストアンサーに選ばれました。」と表示されることを確認
通知をクリックすると質問に遷移することを確認

Screenshot

とくに画面に変更はないので略

やったこと

NotificationFacadeをActivityNotifierに置き換え
NotificationMailerをActivityMailerに置き換え
Mailerのプレビュー画面を追加

ベストアンサー決定通知処理の全体像と今回変更した箇所

before

flowchart TD
  CorrectAnswersController -- message --> ChatNotifier
  ChatNotifier -- message --> Discord::Notifier
  CorrectAnswersController -- publish --> Newspaper
  Newspaper -. "call" .-> CorrectAnswerNotifier
  CorrectAnswerNotifier -- chose_correct_answer --> NotificationFacade
  NotificationFacade:::orange -- chose_correct_answer --> NotificationMailer
  NotificationMailer:::orange -- mail --> ApplicationMailer
  NotificationFacade -- chose_correct_answer --> ActivityNotifier
  ActivityNotifier -. "call" .-> ActivityDriver
  ActivityDriver -- create! --> Notification
  classDef orange fill:#f96

After

flowchart TD
  CorrectAnswersController -- message --> ChatNotifier
  ChatNotifier -- message --> Discord::Notifier
  CorrectAnswersController -- publish --> Newspaper
  Newspaper -. "call" .-> CorrectAnswerNotifier
  CorrectAnswerNotifier -- "notify(:chose_correct_answer)" --> ActivityDelivery
  ActivityDelivery:::orange -. chose_correct_answer .-> ActivityMailer
  ActivityMailer:::orange -- mail --> ApplicationMailer
  ActivityDelivery -. chose_correct_answer .-> ActivityNotifier
  ActivityNotifier -. "call" .-> ActivityDriver
  ActivityDriver -- create! --> Notification
  classDef orange fill:#f96

参考情報

参考リンク

参考PR

担当者通知をactive_delivery経由で送信する #6342

（他にもactive_delivery化のPRは多数あるが略）

ogawa-tomo · 2023-04-29T11:34:46Z

app/mailers/notification_mailer.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true

-class NotificationMailer < ApplicationMailer # rubocop:disable Metrics/ClassLength
+class NotificationMailer < ApplicationMailer


このファイルからメソッドを削除した結果、Rubocopの下記のチェックが通るようになったので削除

app/mailers/notification_mailer.rb:3:1: C: Metrics/ClassLength: Class has too many lines. [103/100] class NotificationMailer < ApplicationMailer ... ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

ogawa-tomo · 2023-04-29T11:38:02Z

test/mailers/previews/activity_mailer_preview.rb

+  def chose_correct_answer
+    answer = Answer.find(ActiveRecord::FixtureSet.identify(:correct_answer1))
+    receiver = User.find(answer.user_id)
+
+    ActivityMailer.with(answer: answer, receiver: receiver).chose_correct_answer
+  end


📝 Action Mailerが提供するメールのプレビュー機能
https://railsguides.jp/action_mailer_basics.html#%E3%83%A1%E3%83%BC%E3%83%AB%E3%81%AE%E3%83%97%E3%83%AC%E3%83%93%E3%83%A5%E3%83%BC
こうしておくことで、下記の場所にアクセスするとプレビューが見れる
http://localhost:3000/rails/mailers/activity_mailer/chose_correct_answer

ogawa-tomo · 2023-04-29T11:51:21Z

app/mailers/activity_mailer.rb

+  # required params: answer, receiver
+  def chose_correct_answer
+    @user = @receiver
+    @answer = params[:answer]


既存の他の通知の実装を見ると、paramsだけでなくargsも受け取れるようになっています。

bootcamp/app/mailers/activity_mailer.rb

Lines 53 to 54 in 366840c

def came_answer(args = {})

@answer = params&.key?(:answer) ? params[:answer] : args[:answer]

ただ、今回は（既存の他の通知でも）、ActivityDeliveryからはparamsを渡す方法でしか呼び出していません。
https://github.com/palkan/active_delivery#parameterized-deliveries

動作を見る限りでもargsを受け取れるようにする必要はなさそうなので、今回はargsを受け取る実装はしていません。
（もし現時点でargsを受け取れるようにすべき理由があればご指摘ください:bow:）

argsの有無については自分では判断がつかないです🙏

確かに使われていないなら必要ないのかな〜とも思います。
自分としてはogawa-tomoさんがこちらの日報でおっしゃられているような感じで、実装しといて問題ないから、なのかなと思っています。

@komagata さんのご意見を伺ってみたいなと思いました🙇‍♂️

OKです、ではこの箇所に関してはいったんこのままで駒形さんのレビューに回すことにしますね

@ogawa-tomo (CC: @djkazunoko )
同期・非同期どちらでも使えるように他の実装もそうなっているので両方対応でお願いしたいです。

@komagata 一応の確認ですが、ここで問題にしているのは「同期版と非同期版を両方実装すべきか」でなく「args版とparams版を両方実装すべきか」なのですが、他の実装に合わせて両方実装すべきということで合っていますか？

5/31ミーティング
📝 params版だけだと、同期・非同期の両方に対応することができない
参考：https://api.rubyonrails.org/classes/ActionMailer/Parameterized.html

argsによる呼び出しに対応しました
86ec797

ogawa-tomo · 2023-04-29T11:58:33Z

test/deliveries/activity_delivery_test.rb

+    assert_difference -> { AbstractNotifier::Testing::Driver.deliveries.count }, 1 do
+      ActivityDelivery.with(**params).notify!(:chose_correct_answer)
+    end
+
+    assert_difference -> { AbstractNotifier::Testing::Driver.enqueued_deliveries.count }, 1 do
+      ActivityDelivery.with(**params).notify(:chose_correct_answer)
+    end


既存の他の通知のテストではパラメータをnotifyメソッドの引数で渡す場合もテストしていますが、下記コメントしたように現時点ではその場合の実装はしていないのでテストも行っていません
#6479 (comment)

argsによる呼び出しのテストを追加しました
4df77fb

ogawa-tomo · 2023-04-29T12:12:00Z

@djkazunoko こちら、よろしければレビューお願いできますでしょうか？

djkazunoko · 2023-04-29T13:08:25Z

@ogawa-tomo
かしこまりました！1週間ほどでレビューできると思います🙌

djkazunoko

@ogawa-tomo
クラス図があってわかりやすかったです🙌

レビュー大変お待たせいたしました🙇‍♂️
2点コメントさせていただきましたので、ご確認をよろしくお願いします。

djkazunoko · 2023-05-05T02:00:33Z

app/views/activity_mailer/chose_correct_answer.html.slim

@@ -1,5 +1,5 @@
-= render 'notification_mailer_template',
+= render '/notification_mailer/notification_mailer_template',
  title: "#{@answer.receiver.login_name}さんの質問【 #{@answer.question.title} 】で#{@answer.sender.login_name}さんの回答がベストアンサーが選ばれました。",


タイポがありました〜
ベストアンサーが選ばれました。 → ベストアンサーに選ばれました。

おお～ありがとうございます！これはどうやら元からですね。
修正しました！
16c8155

djkazunoko · 2023-05-05T02:14:30Z

test/mailers/activity_mailer_test.rb

+  test 'not send chose_correct_answer email to user with mail_notification off' do
+    answer = correct_answers(:correct_answer1)
+    receiver = answer.user
+    receiver.update(mail_notification: false)


update_columnsではなくupdateを使っている理由を教えていただきたいです🙏

どちらを使うべきか自分では判断がつかないんですけど、update_columnsはバリデーション等をスキップするため、updateより高速なのかなと、だから他のテストではupdate_columnsが使われているのかなと考えました！

とくに深い理由はなく、テストの高速化という観点もありませんでした！
ただ、このファイルの他の箇所ではupdate_columnsを使っていますが、他にこれを使っているテストはtest/system/users_test.rbくらいで、bootcampリポジトリ全体ではupdateを使っているテストがほとんどのようです。ここだけ高速化を意識する必要はとくになさそうに感じまして、であれば素直にupdateを使えばいいのかなと思いました。
（なぜこのファイルの他の箇所でupdate_columnsを使っているのかは自分も分かりません・・・）

ご回答いただきありがとうございます！

bootcampリポジトリ全体ではupdateを使っているテストがほとんどのようです。

なるほど〜、むしろupdate_columnsの方が少数派だったんですね。

どちらが一般的なのかなと思って質問させていただきました。勉強になりました🙇‍♂️

ogawa-tomo · 2023-05-05T07:45:08Z

@djkazunoko レビューありがとうございます！修正・コメントしましたので再度ご確認お願いします:bow:

djkazunoko

@ogawa-tomo
修正いただきありがとうございます！
自分からはApproveとさせていただきます〜🙌

ogawa-tomo · 2023-05-06T04:44:50Z

@komagata こちら、メンバーのレビューが通りましたので、ご確認お願いいたします:bow:

komagata

conflictの修正をお願いします〜

ogawa-tomo · 2023-05-24T12:46:18Z

@komagata こちら、コンフリクトを修正したうえでCIが通りましたので、ご確認お願いします:bow:

komagata

conflictの解消をお願いします〜

ogawa-tomo · 2023-06-01T13:26:40Z

@komagata
こちら、以下の対応を行いましたので再度ご確認をお願いいたします:bow:

コンフリクトの解消
argsによる呼び出しに対応→ 86ec797

komagata

再度conflictの解消をお願いします〜

ogawa-tomo · 2023-06-16T03:22:25Z

@komagata 再度コンフリクト解消しましたので、ご確認お願いいたします！

komagata

conflictの修正お願いします〜。

ogawa-tomo · 2023-06-21T12:39:28Z

@komagata 再度コンフリクト解消しましたので、ご確認お願いいたします！

komagata

確認させて頂きました。OKです〜🙆‍♂️

ogawa-tomo force-pushed the feature/replace-notification-on-correct-answer-save-with-active-delivery branch 3 times, most recently from d354f61 to 2a6435d Compare April 28, 2023 12:20

ogawa-tomo commented Apr 29, 2023

View reviewed changes

ogawa-tomo marked this pull request as ready for review April 29, 2023 12:06

ogawa-tomo requested a review from djkazunoko April 29, 2023 12:12

djkazunoko reviewed May 5, 2023

View reviewed changes

ogawa-tomo requested a review from djkazunoko May 5, 2023 07:45

djkazunoko approved these changes May 6, 2023

View reviewed changes

ogawa-tomo requested a review from komagata May 6, 2023 04:45

machida assigned ogawa-tomo May 15, 2023

komagata reviewed May 17, 2023

View reviewed changes

ogawa-tomo force-pushed the feature/replace-notification-on-correct-answer-save-with-active-delivery branch 2 times, most recently from e92fd4a to 4114313 Compare May 17, 2023 11:10

ogawa-tomo requested a review from komagata May 24, 2023 12:46

ogawa-tomo mentioned this pull request May 29, 2023

ベストアンサーなし通知をactive_delivery化する #6578

Merged

komagata reviewed May 29, 2023

View reviewed changes

ogawa-tomo force-pushed the feature/replace-notification-on-correct-answer-save-with-active-delivery branch 3 times, most recently from 86ec797 to 4df77fb Compare June 1, 2023 12:52

ogawa-tomo requested a review from komagata June 1, 2023 13:26

komagata reviewed Jun 1, 2023

View reviewed changes

ogawa-tomo force-pushed the feature/replace-notification-on-correct-answer-save-with-active-delivery branch from 4df77fb to f852fe3 Compare June 15, 2023 12:15

ogawa-tomo force-pushed the feature/replace-notification-on-correct-answer-save-with-active-delivery branch from d564155 to 116ab50 Compare June 15, 2023 12:43

ogawa-tomo requested a review from komagata June 16, 2023 09:31

komagata reviewed Jun 21, 2023

View reviewed changes

ogawa-tomo added 9 commits June 21, 2023 20:49

NotificationFacadeとNotificationMailerをActiveDeliveryとActivityMailerに置き換え

dc0a9d0

ActivityDeliveryのテストを実装

6dc7dae

ActivityMailerのテストを追加

fb0afcb

プレビューを追加

3c5ab58

メールタイトルのタイポを修正

dd7f4c8

不要なコードを削除

fd32707

不要なコードを削除

b697c15

argsでの呼び出しに対応

7ebb84e

微修正

83d5221

ogawa-tomo force-pushed the feature/replace-notification-on-correct-answer-save-with-active-delivery branch from 116ab50 to 83d5221 Compare June 21, 2023 11:56

ogawa-tomo requested a review from komagata June 21, 2023 12:39

komagata approved these changes Jun 21, 2023

View reviewed changes

komagata merged commit 5b03afc into main Jun 21, 2023

komagata deleted the feature/replace-notification-on-correct-answer-save-with-active-delivery branch June 21, 2023 14:27

github-actions bot mentioned this pull request Jun 21, 2023

Release 2023-06-22 08:24:05 #6657

Merged

13 tasks

ogawa-tomo mentioned this pull request Jun 22, 2023

ヤフーメールに対してメール通知が届かない #6671

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ベストアンサー決定通知をactive_delivery化する #6479

ベストアンサー決定通知をactive_delivery化する #6479

ogawa-tomo commented Apr 27, 2023 •

edited

Loading

ogawa-tomo Apr 29, 2023

ogawa-tomo Apr 29, 2023

ogawa-tomo Apr 29, 2023 •

edited

Loading

djkazunoko May 5, 2023

ogawa-tomo May 5, 2023

komagata May 27, 2023

ogawa-tomo May 28, 2023

ogawa-tomo May 31, 2023

ogawa-tomo Jun 1, 2023 •

edited

Loading

ogawa-tomo Apr 29, 2023 •

edited

Loading

ogawa-tomo Jun 1, 2023 •

edited

Loading

ogawa-tomo commented Apr 29, 2023

djkazunoko commented Apr 29, 2023

djkazunoko left a comment

djkazunoko May 5, 2023

ogawa-tomo May 5, 2023

djkazunoko May 5, 2023 •

edited

Loading

ogawa-tomo May 5, 2023

djkazunoko May 6, 2023

ogawa-tomo commented May 5, 2023

djkazunoko left a comment

ogawa-tomo commented May 6, 2023

komagata left a comment

ogawa-tomo commented May 24, 2023

komagata left a comment

ogawa-tomo commented Jun 1, 2023

komagata left a comment

ogawa-tomo commented Jun 16, 2023

komagata left a comment

ogawa-tomo commented Jun 21, 2023

komagata left a comment

	def came_answer(args = {})
	@answer = params&.key?(:answer) ? params[:answer] : args[:answer]

ベストアンサー決定通知をactive_delivery化する #6479

ベストアンサー決定通知をactive_delivery化する #6479

Conversation

ogawa-tomo commented Apr 27, 2023 • edited Loading

Issue

概要

変更確認方法

Screenshot

やったこと

ベストアンサー決定通知処理の全体像と今回変更した箇所

before

After

参考情報

参考リンク

参考PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogawa-tomo Apr 29, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogawa-tomo Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

ogawa-tomo Apr 29, 2023 • edited Loading

Choose a reason for hiding this comment

ogawa-tomo Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

ogawa-tomo commented Apr 29, 2023

djkazunoko commented Apr 29, 2023

djkazunoko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djkazunoko May 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogawa-tomo commented May 5, 2023

djkazunoko left a comment

Choose a reason for hiding this comment

ogawa-tomo commented May 6, 2023

komagata left a comment

Choose a reason for hiding this comment

ogawa-tomo commented May 24, 2023

komagata left a comment

Choose a reason for hiding this comment

ogawa-tomo commented Jun 1, 2023

komagata left a comment

Choose a reason for hiding this comment

ogawa-tomo commented Jun 16, 2023

komagata left a comment

Choose a reason for hiding this comment

ogawa-tomo commented Jun 21, 2023

komagata left a comment

Choose a reason for hiding this comment

ogawa-tomo commented Apr 27, 2023 •

edited

Loading

ogawa-tomo Apr 29, 2023 •

edited

Loading

ogawa-tomo Jun 1, 2023 •

edited

Loading

ogawa-tomo Apr 29, 2023 •

edited

Loading

ogawa-tomo Jun 1, 2023 •

edited

Loading

djkazunoko May 5, 2023 •

edited

Loading