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

休会通知をactive_delivery化する #6506

Merged
merged 7 commits into from
May 17, 2023

Conversation

YukiWatanabe824
Copy link
Contributor

@YukiWatanabe824 YukiWatanabe824 commented May 8, 2023

Issue

概要

休会した際の通知処理を既存の仕様からactive_deliveryに置き換えました。
対象となる通知処理はメール通知とアプリ内通知です。

変更確認方法

  1. feature/active_delivery_use_in_hibernate_notificationをローカルに取り込む
  2. 任意のアカウントでログインし、休会する ※休会方法は「休会の手続き」を参照
  3. machidaでログインし、http://localhost:3000/notifications から休会の通知が届いていることを確認します。
image
  1. 通知から休会ユーザーにアクセスできるか確認します。
image
  1. http://localhost:3000/letter_opener/ にアクセスし、休会のメールが配信されていることを確認します。
image
  1. メールから休会ユーザーにアクセスできるか確認します。
image

Screenshot

外見上の変更はないため省略します。

参考

gemを使った通知機能 · fjordllc/bootcamp Wiki
abstract_notifier
abstract_notifierで通知を実装する - komagataのブログ
active_delivery
active_deliveryで通知をまとめる - komagataのブログ
Action Mailer の基礎 - Railsガイド

クラス図

変更前

graph TD;

app/controllers/hibernation_controller.rb-->app/models/notification_facade.rb;
app/models/notification_facade.rb-->app/mailers/notification_mailer.rb;
app/models/notification_facade.rb-->app/notifier/activity_notifier.rb;
app/notifier/activity_notifier.rb-->app内通知;
app/mailers/notification_mailer.rb-->メール通知;
Loading

変更後

graph TD;

app/controllers/hibernation_controller.rb-->ActiveDelivery;
ActiveDelivery-->app/mailers/activity_mailer.rb;
ActiveDelivery-->app/notifier/activity_notifier.rb;
app/notifier/activity_notifier.rb-->app内通知;
app/mailers/activity_mailer.rb-->メール通知;
Loading

@YukiWatanabe824 YukiWatanabe824 marked this pull request as ready for review May 8, 2023 08:21
@YukiWatanabe824 YukiWatanabe824 requested a review from toke04 May 8, 2023 12:53
@YukiWatanabe824
Copy link
Contributor Author

@syo-tokeshi
お疲れさまです!
こちらのレビューをお願いできますでしょうか?

@toke04
Copy link
Contributor

toke04 commented May 9, 2023

@YukiWatanabe824
お疲れ様です。
大丈夫ですよー😊
今週の金曜までにレビューいたします😆

).hibernated

perform_enqueued_jobs do
mailer.deliver_later
Copy link
Contributor

@toke04 toke04 May 9, 2023

Choose a reason for hiding this comment

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

こちらのテストの同期版も実装されると、さらに良いと思いました😊
deliver_now を使うテストもあった方が良い、という意味になります🙇‍♂️

test 'moved_up_event_waiting_user' do

[ 私のサンプルコード]

test 'moved_up_event_waiting_user' do
    event = events(:event3)
    notification = notifications(:notification_moved_up_event_waiting_user)
    ActivityMailer.moved_up_event_waiting_user(
      receiver: notification.user,
      event: event
    ).deliver_now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ご連絡遅くなりました💦
ご指摘のとおりだと思いますのでテスト追加してみます💪

@toke04
Copy link
Contributor

toke04 commented May 9, 2023

@YukiWatanabe824
お疲れ様です。
レビュー完了しました🙇‍♂️

動作確認も完了し、ほぼ良いと思いました😆

テストケースの追加と、こちらの手順の追加をして頂けたらさらに良いと思います😊

[変更確認方法について]

  1. 動作手順のスクリーンショットも載せて頂いたら理解しやすいと思いました😆
  2. 休会手順のリンクも付けていただけると確認しやすいと思いました

私の動作確認証跡

動作確認したので載せておきます🙇‍♂️

  1. hatsunoさんで休会

スクリーンショット 2023-05-09 11 06 05

  1. machidaさんアカウントで、休会の通知が来ていることを確認

スクリーンショット 2023-05-09 10 59 08

  1. メールが届いている事も確認

スクリーンショット 2023-05-09 11 04 13

  1. メールから休会ユーザーにアクセスできる事も確認しました

スクリーンショット 2023-05-09 11 06 05

@YukiWatanabe824
Copy link
Contributor Author

手順がちょっと分かりづらかったですかね💦💦フォローありがとうございます。
いただいたアイデアを反映してみます〜

@YukiWatanabe824
Copy link
Contributor Author

@syo-tokeshi
お疲れさまです!
レビューを反映して修正してみましたのでご確認お願いいたします。

追加したテストに関してはwith paramsだけだと同期版と非同期版のどちらを使っているかわからなかったので、過去のコードには反するのですがテスト名に同期非同期と加えています〜

@toke04
Copy link
Contributor

toke04 commented May 11, 2023

@YukiWatanabe824
お疲れ様です。
ご対応ありがとうございます😊

Approveさせて頂きます🙇‍♂️

追加したテストに関してはwith paramsだけだと同期版と非同期版のどちらを使っているかわからなかったので、過去のコードには反するのですがテスト名に同期非同期と加えています〜

私もここが難しいなー、と思いました🙇‍♂️
前例に倣った方が良い気もしましたが、Watanabeさんが追加した文言のおかげで分かりやすくもなりましたし、
私はこれで良いと思いました😊

@YukiWatanabe824
Copy link
Contributor Author

@syo-tokeshi
ありがとうございました!
FBCほど過去からの積み重ねがあると改善?に踏み出すのもちょっとハードルがありますね😅

@YukiWatanabe824
Copy link
Contributor Author

@komagata
お疲れさまです!
こちらメンバーのOKがでましたのでレビューをお願いいたします。

@toke04
Copy link
Contributor

toke04 commented May 11, 2023

@syo-tokeshi ありがとうございました! FBCほど過去からの積み重ねがあると改善?に踏み出すのもちょっとハードルがありますね😅

ですよねー😭
分かります!🙇‍♂️

Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

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

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

@komagata komagata merged commit 418f439 into main May 17, 2023
@komagata komagata deleted the feature/active_delivery_use_in_hibernate_notification branch May 17, 2023 05:33
@github-actions github-actions bot mentioned this pull request May 17, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants