最初の日報投稿通知をactive_delivery化する #5931

akingo55 · 2022-12-12T14:48:56Z

Issue

最初の日報投稿通知をactive_delivery化したい #5881

概要

最初の日報投稿通知をactive_delivery化しました。
見た目の変更はありません。

変更確認方法

feature/replace-notification-first-report-to-active-deliveryをローカルに取り込む
muryouでログインし、初日報を公開する
komagataでログインし、通知にmuryouユーザーが初日報書いたことが通知されていることを確認する
http://localhost:3000/letter_opener/にアクセスし、メールが届いていることを確認する

Screenshot

見た目の変更はありません。

変更前

変更後

komagata · 2022-12-18T06:36:38Z

@akingo55 卒業通知のメールが送られていなかった問題を修正したのでmainブランチを取り込んだ上でこちらを参考にして実装していただければありがたいです〜

#5939

akingo55 · 2022-12-28T13:42:32Z

@dowdiness
お疲れ様です！
こちらのPRのレビューを依頼しても良いでしょうか？:pray:
お手隙に確認よろしくお願いします。

dowdiness

レビュー依頼ありがとうございますー！
のんびりしていたら返信するのが遅くなってしまいました。すいません。

使われなくなったメソッドとテストを消していなくて追加のテストが必要なことと、バグが見つかったので対応が必要なことを書きました。

確認よろしくお願いします。

dowdiness · 2023-01-07T17:16:58Z

app/views/activity_mailer/first_report.html.slim

+= render '/notification_mailer/notification_mailer_template', title: "#{@report.user.login_name}さんのはじめての日報です！", link_url: notification_url(@notification), link_text: 'この日報へ' do
+  p #{@report.user.login_name}さんのはじめての日報です。歓迎のコメントを投稿しよう！！
+  div(style='border-top: solid 1px #ccc; height: 0;')
+  = md2html(@report.description)


コレも上と同じようにこのslimが作られた後はNotificationMailerの方のfirst_report.html.slimはもう使わないと思うので消してしまったほうが良いと思います。

ご指摘ありがとうございます:pray:
こちら対応しました！

dowdiness · 2023-01-07T17:18:26Z

app/mailers/activity_mailer.rb

+    @notification = @user.notifications.find_by(link: "/reports/#{@report.id}")
+    mail to: @user.email,
+         subject: "[FBC] #{@report.user.login_name}さんがはじめての日報を書きました！"
+  end


ActivityMailerを使う場合はNotificationMailerの方のfirst_reportはもう使わないと思うので消してしまったほうが良いと思いました。

NotificationMailerの使われているnotification_mailer_previewとnotification_mailer_testにあるテストも消してしまって、ActivityMailerを使う形でactivity_mailer_previewにテストを追加したら良いと思います。

メールのプレビューの確認はhttp://localhost:3000/rails/mailers で出来ます。

それと、サイト内通知より先にメール通知が送られるとエラーになる問題があるそうなので対応したほうが良さそうです。

#6001

を確認してみてください。

@dowdiness
不要なもの削除しました！

NotificationMailerの使われているnotification_mailer_previewとnotification_mailer_testにあるテストも消してしまって、ActivityMailerを使う形でactivity_mailer_previewにテストを追加したら良いと思います。

ActivityMailerのテストは追加済みです！

それと、サイト内通知より先にメール通知が送られるとエラーになる問題があるそうなので対応したほうが良さそうです。

ありがとうございます！こちらも対応しました！

@dowdiness
念の為こちらリマインドです:pray:
指摘していただいた点修正しましたので、再度ご確認お願いします！

@akingo55
お疲れ様です。返信遅くなってしまいすいません。モチベーションが落ちてあまり作業できてませんでした。

サイト内通知より先にメール通知が送られるとエラーになる問題の対応は問題なさそうです👍

activity_mailer_previewにテストを追加したら良いというのが説明不足で分かりにくかったと思います。

ActivityMailerのテストが無いという意味ではなく、https://github.com/fjordllc/bootcamp/blob/main/test/mailers/previews/activity_mailer_preview.rb
にメールのプレビュー用のテストを追加したら http://localhost:3000/rails/mailers でメールのプレビューが確認出来るようになるので、テストを追加した方が良いなと思ったという意味で書きました。

遅くなって申し訳ないですが対応よろしくおねがいします🙇‍♂️

@dowdiness
なるほど、、、！理解しました:pray:
こちらメールのプレビュー用テストを追加しましたのでご確認お願いしますmm
baf6594

komagata

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

akingo55 · 2023-02-15T04:39:23Z

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

@komagata
修正完了しました！ご確認お願いします！

komagata

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

akingo55 · 2023-02-20T11:41:16Z

@komagata
修正しましたので、ご確認お願いします！

komagata

度々すみません、conflictの修正をお願いします〜

akingo55 · 2023-03-02T02:16:19Z

@komagata
修正完了です！

dowdiness

遅れてすいません🙏
もう既にkomagataさんのレビューに進んでいて問題無いのでApproveします

mono-nobe · 2023-03-05T13:20:47Z

@akingo55 @komagata
横から失礼します 🙇

おそらく、こちらと同様の問題が発生していそうです。。
feature/replace-notification-first-report-to-active-deliveryブランチで、PRに記載いただいている手順通りに動作確認したところ、メールが対象者全員に送信されず、上記コメントと同じような挙動をしていました。
ご確認をよろしくお願いします。

komagata · 2023-03-11T13:20:49Z

@akingo55 conflictの解消をお願いします。

akingo55 · 2023-03-26T13:43:07Z

@ akingo55 conflictの解消をお願いします。

@komagata
修正しました、確認お願いします。
コンフリクトしやすいので早めにレビューいただけると助かります。

mono-nobe · 2023-03-26T17:02:05Z

@akingo55 CC @komagata

#5931 (comment)

コメントご確認いただけましたでしょうか？
念の為、再度ローカルで動作確認しましたが、やはり正常に動作していないように思われます。
(もし仕様を勘違いしていたらごめんなさい。)

mono-nobe · 2023-03-26T17:04:19Z

app/models/report_callbacks.rb

@@ -31,7 +31,7 @@ def create_author_watch(report)

  def notify_first_report(report)
    User.admins_and_mentors.each do |receiver|


@akingo55

User.admins_and_mentors

メール送信対象者は管理者とメンターであると理解しており、現状そこが満たされていない気がしております。

machida · 2023-05-15T05:10:29Z

#6504
こちらで続きを行っているのでクローズにします。

akingo55 self-assigned this Dec 12, 2022

akingo55 force-pushed the feature/replace-notification-first-report-to-active-delivery branch from b324db9 to be871eb Compare December 17, 2022 07:18

akingo55 force-pushed the feature/replace-notification-first-report-to-active-delivery branch from be871eb to d0f70dc Compare December 18, 2022 07:42

akingo55 force-pushed the feature/replace-notification-first-report-to-active-delivery branch from dcd7639 to 2d5a4cb Compare December 26, 2022 11:48

akingo55 marked this pull request as ready for review December 28, 2022 13:38

akingo55 requested a review from dowdiness December 28, 2022 13:41

maeda-m mentioned this pull request Dec 28, 2022

回答通知をactive_delivery経由で送信する #5989

Merged

dowdiness requested changes Jan 7, 2023

View reviewed changes

akingo55 force-pushed the feature/replace-notification-first-report-to-active-delivery branch from 64506f4 to a1712f7 Compare January 9, 2023 10:08

akingo55 requested a review from dowdiness January 22, 2023 15:00

akingo55 force-pushed the feature/replace-notification-first-report-to-active-delivery branch 2 times, most recently from 4b6e73e to 00e7e3e Compare January 30, 2023 11:19

akingo55 requested a review from komagata January 30, 2023 12:50

akingo55 force-pushed the feature/replace-notification-first-report-to-active-delivery branch from 00e7e3e to a69c0c9 Compare February 4, 2023 08:45

komagata reviewed Feb 7, 2023

View reviewed changes

akingo55 force-pushed the feature/replace-notification-first-report-to-active-delivery branch 3 times, most recently from 41684f8 to 1a577a9 Compare February 15, 2023 00:51

komagata reviewed Feb 18, 2023

View reviewed changes

akingo55 force-pushed the feature/replace-notification-first-report-to-active-delivery branch 2 times, most recently from 54e6959 to ea4951d Compare February 20, 2023 11:19

akingo55 requested a review from komagata February 20, 2023 11:41

komagata reviewed Feb 25, 2023

View reviewed changes

akingo55 force-pushed the feature/replace-notification-first-report-to-active-delivery branch from ea4951d to be9029e Compare March 1, 2023 14:10

dowdiness approved these changes Mar 2, 2023

View reviewed changes

akingo55 added 13 commits March 26, 2023 20:47

コンフリクト修正

dfebe3f

メール通知を追加する

db57668

通知タイトルの修正

ff25c1a

テストを追加する

59771b5

メール送信で不要なslimとメソッドを削除

e34ca93

不要なテストを削除

de08417

サイト内通知より先にメール通知が送られるバグ修正

91f526b

落ちているテストの修正

e26886b

通知メッセージを修正

17db2a5

メールのプレビューを可能にする

fea0375

コンフリクト修正漏れを直す

139d850

リファクタしたところを戻す

02049ff

コンフリクト修正漏れを直す

df6c216

akingo55 force-pushed the feature/replace-notification-first-report-to-active-delivery branch from be9029e to df6c216 Compare March 26, 2023 13:17

mono-nobe reviewed Mar 26, 2023

View reviewed changes

komagata unassigned akingo55 Apr 12, 2023

machida closed this May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

最初の日報投稿通知をactive_delivery化する #5931

最初の日報投稿通知をactive_delivery化する #5931

akingo55 commented Dec 12, 2022 •

edited

Loading

komagata commented Dec 18, 2022

akingo55 commented Dec 28, 2022

dowdiness left a comment

dowdiness Jan 7, 2023

akingo55 Jan 9, 2023

dowdiness Jan 7, 2023

akingo55 Jan 9, 2023

akingo55 Jan 16, 2023

dowdiness Feb 1, 2023

akingo55 Feb 4, 2023

komagata left a comment

akingo55 commented Feb 15, 2023

komagata left a comment

akingo55 commented Feb 20, 2023

komagata left a comment

akingo55 commented Mar 2, 2023

dowdiness left a comment

mono-nobe commented Mar 5, 2023 •

edited

Loading

komagata commented Mar 11, 2023

akingo55 commented Mar 26, 2023

mono-nobe commented Mar 26, 2023

mono-nobe Mar 26, 2023 •

edited

Loading

machida commented May 15, 2023

		@@ -31,7 +31,7 @@ def create_author_watch(report)

		def notify_first_report(report)
		User.admins_and_mentors.each do \|receiver\|

最初の日報投稿通知をactive_delivery化する #5931

最初の日報投稿通知をactive_delivery化する #5931

Conversation

akingo55 commented Dec 12, 2022 • edited Loading

Issue

概要

変更確認方法

Screenshot

変更前

変更後

komagata commented Dec 18, 2022

akingo55 commented Dec 28, 2022

dowdiness left a comment

Choose a reason for hiding this comment

dowdiness Jan 7, 2023

Choose a reason for hiding this comment

akingo55 Jan 9, 2023

Choose a reason for hiding this comment

dowdiness Jan 7, 2023

Choose a reason for hiding this comment

akingo55 Jan 9, 2023

Choose a reason for hiding this comment

akingo55 Jan 16, 2023

Choose a reason for hiding this comment

dowdiness Feb 1, 2023

Choose a reason for hiding this comment

akingo55 Feb 4, 2023

Choose a reason for hiding this comment

komagata left a comment

Choose a reason for hiding this comment

akingo55 commented Feb 15, 2023

komagata left a comment

Choose a reason for hiding this comment

akingo55 commented Feb 20, 2023

komagata left a comment

Choose a reason for hiding this comment

akingo55 commented Mar 2, 2023

dowdiness left a comment

Choose a reason for hiding this comment

mono-nobe commented Mar 5, 2023 • edited Loading

komagata commented Mar 11, 2023

akingo55 commented Mar 26, 2023

mono-nobe commented Mar 26, 2023

mono-nobe Mar 26, 2023 • edited Loading

Choose a reason for hiding this comment

machida commented May 15, 2023

akingo55 commented Dec 12, 2022 •

edited

Loading

mono-nobe commented Mar 5, 2023 •

edited

Loading

mono-nobe Mar 26, 2023 •

edited

Loading