新規ユーザー登録時にメンターにメール通知およびサイト内通知するようにした #5227

SatoshiHaramura · 2022-07-14T13:53:16Z

概要

新規ユーザー登録時、メンター宛てに、メール通知およびサイト内通知を行うようにする。(Discordへの通知は既に実装済みのため対象外)
対象の新規ユーザーは、全てのユーザーとする。

また、メール通知やサイト内通知の文言などの仕様確認は、以下のDiscordにてmachidaさんにOK頂いております。
https://ptb.discord.com/channels/715806612824260640/809595476847493192/999824618942640129

環境準備

ブランチfeature/notify-mentor-when-new-user-registersをローカルに取り込む。
bin/rails sでローカル環境を立ち上げる。

確認方法と確認内容

方針

ユーザーの種類は以下の通り5種類あるが、1. 非課金ユーザー については、いずれか1種類をピックアップし、確認する方針とする。

非課金ユーザー
- 管理者
- メンター
- アドバイザー
- 研修生
課金ユーザー
- 上記以外

1. メール通知の場合

フィヨルドブートキャンプ参加登録ページ(/users/new)にアクセスし、必要な情報を入力し「参加する」をクリックし、新規ユーザーを登録する。(有効なカード番号は、sign_up_test.rb を参照)
LetterOpenerWeb(/letter_opener/)にアクセスし、メンター宛て(4人)に送信される以下のようなメールの内容を確認する。

From	Subject	Date	To
"フィヨルドブートキャンプ" [email protected]	[FBC] xxxxさんが新しく入会しました！	「フィヨルドブートキャンプへようこそ」というタイトルのwelcomメールが送信された5秒後の日時	[email protected]
"フィヨルドブートキャンプ" [email protected]	[FBC] xxxxさんが新しく入会しました！	「フィヨルドブートキャンプへようこそ」というタイトルのwelcomメールが送信された5秒後の日時	[email protected]
"フィヨルドブートキャンプ" [email protected]	[FBC] xxxxさんが新しく入会しました！	「フィヨルドブートキャンプへようこそ」というタイトルのwelcomメールが送信された5秒後の日時	[email protected]
"フィヨルドブートキャンプ" [email protected]	[FBC] xxxxさんが新しく入会しました！	「フィヨルドブートキャンプへようこそ」というタイトルのwelcomメールが送信された5秒後の日時	[email protected]

本文は以下の通り。

xxxxさんが新しく入会しました！
xxxxさんのページへ

メール本文の「xxxxさんのページへ」のリンクをクリックし、メンターでログインすると、新規登録ユーザーの詳細ページ(/users/:id)が表示されることを確認する。

管理ページの企業ページ(/admin/companies)にアクセスし、「アドバイザー招待」または「研修生招待リンク」のリンクをクリックし、必要な情報を入力し新規ユーザー(アドバイザー or 研修生)を登録する。
項番2 ~ 3 を同様に行う。

2. サイト内通知の場合

フィヨルドブートキャンプ参加登録ページ(/users/new)にアクセスし、必要な情報を入力し「参加する」をクリックし、新規ユーザーを登録する。(有効なカード番号は、sign_up_test.rb を参照)
メンターでログインし、画面右上の「通知」ナビゲーションをクリックし、以下のように通知モーダルが追加されていることを確認する。
通知の文言は以下の通り。

🎉 xxxxさんが新しく入会しました！

🎉 xxxxさんが新しく入会しました！という通知をクリックし、通知一覧ページ(/notifications)が表示され、以下のような通知が追加されていることを確認する。

🎉 xxxxさんが新しく入会しました！

🎉 xxxxさんが新しく入会しました！というリンクをクリックし、新規登録ユーザーのユーザー詳細ページ(/users/:id)が表示されることを確認する。

管理ページの企業ページ(/admin/companies)にアクセスし、「アドバイザー招待」または「研修生招待リンク」のリンクをクリックし、必要な情報を入力し新規ユーザー(アドバイザー or 研修生)を登録する。
項番2 〜4 を同様に行う。

テストについての補足

システムテストで、新規登録するユーザーは、アドバイザー、研修生、それ以外の3パターンでよいとkomagataさんにご教授頂きましたので、その3パターンでテストしております。

SatoshiHaramura · 2022-07-23T11:13:35Z

@yocajii
お疲れ様です。
レビューの依頼をさせて頂きたいです🙏
お手隙の際にお願い致します。

yocaji

レビューにお時間をいただいていて申し訳ありません 💦
引き続き確認を進めているところですが、先に気付いた部分についてコメントさせていただきました。

yocaji · 2022-07-26T05:52:54Z

test/fixtures/notifications.yml

@@ -173,3 +173,11 @@ notification_graduated:
  message: 🎉 sotugyouさんが卒業しました。
  link: "/users/<%= ActiveRecord::FixtureSet.identify(:sotugyou) %>"
  read: false
+
+notification_signed_up:


すぐ上にある卒業の通知がそうなってしまっているのですが、このフィクスチャはどのテストでも利用されていないので不要そうです。
Fixtureに書かれたデータはテスト実行前にすべてDBに投入されるため、不要なフィクスチャはシステムテストが重くなる一因になるようです。

また、テストのフィクスチャとは別になりますが db/fixtures/notifications.yml への追加は検討済みの上で追加していない感じでしょうか？

bootcamp/db/fixtures/notifications.yml

Lines 143 to 149 in 0b11a94

notification_hibernated:

kind: 19

user: mentormentaro

sender: hatsuno

message: hatsunoさんが休会しました。

link: "/users/<%= ActiveRecord::FixtureSet.identify(:hatsuno) %>"

read: false

こちらに追加しておくと開発環境で表示されるため、壊れた時に気付きやすくできるようです。
ただこちらについても私の意見としては、通知の表示ロジックがおかしくなっていないか確認する目的なら既存のフィクスチャで十分な気がするので、あえて入会の通知を追加する意義は薄いのかなという気はしています。

確認しますので、少々お待ち下さい🙏

すぐ上にある卒業の通知がそうなってしまっているのですが、このフィクスチャはどのテストでも利用されていないので不要そうです。

このフィクスチャの利用箇所は、test/mailers/notification_mailer_test.rbのtest 'signed up'の以下を実行する際に利用していると考えています。

mailer = NotificationMailer.with( sender: user, receiver: mentor ).signed_up

上記が実行されると、app/mailers/notification_mailer.rbのdef signed_upの以下で、通知データが必要になるので、先程のフィクスチャのデータを@notificationに格納しています。

@notification = @user.notifications.find_by(link: "/users/#{@sender.id}", kind: Notification.kinds[:signed_up])

また、テストのフィクスチャとは別になりますが db/fixtures/notifications.yml への追加は検討済みの上で追加していない感じでしょうか？

一旦検討済みでして、入会時の通知データが無くても、開発時に不便だと感じる場面は特に無いかぁと思いましたので、db/fixtures/notifications.ymlへは追加していない状況となります。

確認不足で申し訳ありません🙇‍♂️
フィクスチャの利用個所について、ご説明いただきありがとうございます。NotificationMailerのテストの流れが理解できました🙏

また、dbのフィクスチャへの追加も検討済みの上でとのこと、失礼いたしました。こちらについても承知いたしました🙆‍♂️

@SatoshiHaramura

Fixtureに書かれたデータはテスト実行前にすべてDBに投入されるため、不要なフィクスチャはシステムテストが重くなる一因になるようです。

こちらで @yocajii さんがおっしゃってるようにできればそのテストだけのデータ追加で行けるとありがたいな〜と思います。

@SatoshiHaramura 上記、いかがでしょうか〜？

@komagata
テスト内でテストに必要なデータを作成する方がよい、という理解を致しました！
(yocajiiさんとのやりとりの中では、フィクスチャが要るorいらないという観点のみで考えてしまっておりました💦)

フィクスチャには追加せずに、テスト内でテストに必要なデータを作成するように修正致しましたので(c262fd2) 、ご確認お願い致します🙏

yocaji

動作確認は問題ありませんでした 🙆‍♂️
先にコメントさせていただいた件に加えて、こうするとより良いかもと思った部分でいくつかコメントを入れさせていただきました。

新たな通知の作成ということでメソッドやテストの新規作成が多く、ご苦労もあったかと思います。
レビューさせていただく過程で通知の仕組みやVCRなど私の方でも調べてみましたが、認識が違っている点などありましたら教えていただけると嬉しいです 🙏

yocaji · 2022-07-26T13:37:46Z

test/notifiers/activity_notifier_test.rb

+  test '#signed_up' do
+    notification = ActivityNotifier.with(sender: users(:hajime), receiver: users(:mentormentaro)).signed_up
+
+    assert_difference -> { AbstractNotifier::Testing::Driver.deliveries.count }, 1 do
+      notification.notify_now
+    end
+  end


Parameterized+同期の1パターンのみになっていますが、（実行コストを気にして必要最小限に絞り込むシステムテストと違って）メソッドのテストなので、メソッドが使われ得るパターンを押さえておいた方がベターな気がしました。
このファイルの27行目のテスト test '#create_page' がそのような形になっていそうです。

参考情報

https://docs.komagata.org/5857

なるほど、確かにメソッドが使われ得るパターンを押さえておいたほうがよさそうですね！
以下のnotify_laterメソッドのテストも追加致しました！

assert_difference -> { AbstractNotifier::Testing::Driver.enqueued_deliveries.count }, 1 do notification.notify_later end

yocaji · 2022-07-26T13:41:07Z

test/system/notification/signed_up_test.rb

+
+    fill_stripe_element('4242 4242 4242 4242', '12 / 50', '111')
+
+    VCR.use_cassette 'sign_up/valid-card', record: :once, match_requests_on: %i[method uri] do


6行目でオプションを定数として宣言しているので、下記のように書けそうです。

Suggested change

VCR.use_cassette 'sign_up/valid-card', record: :once, match_requests_on: %i[method uri] do

VCR.use_cassette 'sign_up/valid-card', VCR_OPTIONS do

仰る通りですね💦
ご提案頂いたように修正致しました。

yocaji · 2022-07-26T14:07:20Z

test/system/notification/signed_up_test.rb

+    visit_with_auth notifications_path, 'komagata'
+    within first('.card-list-item.is-unread') do
+      assert_selector '.card-list-item-title__link-label', text: '🎉 harukoさんが新しく入会しました！'
+    end


いずれのテストケースでも4ユーザーで表示を確認していますが、何か方針がありましたら教えていただけますか 🙇‍♂️
メンターか管理者かなどで表示内容が変わるわけではないので、メール通知のテストと同様にどれか1ユーザーで十分なように思いました。

条件に該当するメンター全員に対して、サイト内通知が行われていることのテストとして書いていたので、該当する4ユーザーでも表示を確認しておりました。
たしかに、表示内容はどのユーザーでも同じ内容なので、1ユーザーのみの確認に修正してみます。

yocaji · 2022-07-27T02:03:34Z

app/controllers/users_controller.rb

+  def notify_to_mentors(user)
+    User.mentor.each do |mentor_user|
+      NotificationFacade.signed_up(user, mentor_user)
+    end
+  end


特に理由がなければnotify_to_chat の上に移動して、notify_to_xxxx 系のメソッドを一箇所に集めておくと、より読みやすくなりそうに思いました。

仰る通り、notify_to_xxxx系のメソッドを集めておくと、より読みやすいですね！
notify_to_mentorsメソッドをnotify_to_chatメソッドの上に移動しました！

SatoshiHaramura · 2022-08-02T07:30:27Z

@yocajii
お疲れ様です。
返信が遅くなりましたが、レビューして頂きありがとうございました！
様々な視点からのコメントを頂いて勉強になりました✨

一旦、頂いたコメント全てに対して、返信や修正致しましたので、再度レビューをお願い致します🙏

yocaji

ご回答＆ご対応くださいまして、ありがとうございます🙏✨
改めて動作確認も行って問題ありませんでしたので、私からはApproveいたします。

SatoshiHaramura · 2022-08-03T01:32:37Z

@yocajii
再度レビューしてくださりありがとうございました✨

@komagata
メンバーレビューが通りましたので、お手すきの際にレビューをお願い致します🙏

komagata · 2022-08-04T07:53:42Z

app/mailers/notification_mailer.rb

+  def signed_up
+    @user = @receiver
+    @notification = @user.notifications.find_by(link: "/users/#{@sender.id}", kind: Notification.kinds[:signed_up])
+    subject = "[bootcamp] #{@sender.login_name}さんが新しく入会しました！"


最近下記のようにメールのprefixが[FBC]に変わったのでそれに合わせる修正をお願いします〜

https://github.com/fjordllc/bootcamp/blob/main/app/mailers/user_mailer.rb

@komagata
承知致しました。
ここ(fc78110) で修正致しましたので、ご確認お願い致します🙏

SatoshiHaramura · 2022-08-08T11:40:47Z

@komagata
お疲れ様です。
頂いていたコメントに対応致しましたので、再度レビューをお願い致します🙏

komagata · 2022-08-10T06:59:17Z

test/fixtures/notifications.yml

@@ -173,3 +173,11 @@ notification_graduated:
  message: 🎉 sotugyouさんが卒業しました。
  link: "/users/<%= ActiveRecord::FixtureSet.identify(:sotugyou) %>"
  read: false
+
+notification_signed_up:


@SatoshiHaramura 上記、いかがでしょうか〜？

komagata · 2022-08-12T08:14:35Z

test/system/notification/signed_up_test.rb

+require 'application_system_test_case'
+
+class Notification::SignedUpTest < ApplicationSystemTestCase
+  VCR_OPTIONS = {


他のシステムテストと重複しているのでこういう感じにするのがいいかもです〜

https://github.com/fjordllc/bootcamp/blob/feature/comeback/test/application_system_test_case.rb#L31-L39

（これはまだマージされていないブランチのコードです）

@komagata
承知しました！

ご提案頂いたテスト用のメソッドをここ(4eba9a8) で追加して、テストコード内でそのメソッドを使用するように修正致しましたので、再度ご確認お願い致します🙏

komagata

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

SatoshiHaramura self-assigned this Jul 14, 2022

machida self-assigned this Jul 20, 2022

SatoshiHaramura force-pushed the feature/notify-mentor-when-new-user-registers branch 2 times, most recently from 28b66cf to 3d6e1ac Compare July 22, 2022 11:22

SatoshiHaramura marked this pull request as ready for review July 23, 2022 11:08

SatoshiHaramura requested a review from yocaji July 23, 2022 11:08

machida removed their assignment Jul 24, 2022

yocaji reviewed Jul 26, 2022

View reviewed changes

yocaji reviewed Jul 27, 2022

View reviewed changes

SatoshiHaramura force-pushed the feature/notify-mentor-when-new-user-registers branch from 3d6e1ac to 5268812 Compare August 2, 2022 04:42

yocaji approved these changes Aug 3, 2022

View reviewed changes

SatoshiHaramura requested a review from komagata August 3, 2022 01:32

komagata requested changes Aug 4, 2022

View reviewed changes

SatoshiHaramura force-pushed the feature/notify-mentor-when-new-user-registers branch 2 times, most recently from 9681dd3 to 906588d Compare August 8, 2022 09:21

komagata requested changes Aug 10, 2022

View reviewed changes

SatoshiHaramura force-pushed the feature/notify-mentor-when-new-user-registers branch 2 times, most recently from e0719bd to 148c59a Compare August 10, 2022 12:04

komagata reviewed Aug 12, 2022

View reviewed changes

SatoshiHaramura added 6 commits August 12, 2022 19:29

サインアップ時のサイト内通知およびメール通知処理を追加

344434c

サインアップ時のメールの文言を追加

6dd8a1c

メンター宛てのサイト内通知のテストを追加

e33bed3

メンター宛てのメール通知のテストを追加

26a5b5b

テスト用メソッドを追加

4eba9a8

システムテストを追加

546ddfe

SatoshiHaramura force-pushed the feature/notify-mentor-when-new-user-registers branch from 148c59a to 546ddfe Compare August 12, 2022 10:52

komagata approved these changes Aug 16, 2022

View reviewed changes

komagata merged commit be92891 into main Aug 16, 2022

komagata deleted the feature/notify-mentor-when-new-user-registers branch August 16, 2022 15:35

github-actions bot mentioned this pull request Aug 16, 2022

Release 2022-08-18 08:54:40 #5366

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

新規ユーザー登録時にメンターにメール通知およびサイト内通知するようにした #5227

新規ユーザー登録時にメンターにメール通知およびサイト内通知するようにした #5227

SatoshiHaramura commented Jul 14, 2022 •

edited

Loading

SatoshiHaramura commented Jul 23, 2022

yocaji left a comment

yocaji Jul 26, 2022 •

edited

Loading

SatoshiHaramura Aug 2, 2022

SatoshiHaramura Aug 2, 2022

SatoshiHaramura Aug 2, 2022 •

edited

Loading

yocaji Aug 3, 2022

komagata Aug 7, 2022

komagata Aug 10, 2022

SatoshiHaramura Aug 10, 2022 •

edited

Loading

yocaji left a comment

yocaji Jul 26, 2022

SatoshiHaramura Aug 2, 2022

yocaji Jul 26, 2022

SatoshiHaramura Aug 2, 2022

yocaji Jul 26, 2022

SatoshiHaramura Aug 2, 2022

yocaji Jul 27, 2022

SatoshiHaramura Aug 2, 2022 •

edited

Loading

SatoshiHaramura commented Aug 2, 2022

yocaji left a comment

SatoshiHaramura commented Aug 3, 2022

komagata Aug 4, 2022

SatoshiHaramura Aug 4, 2022

SatoshiHaramura commented Aug 8, 2022

komagata Aug 10, 2022

komagata Aug 12, 2022

SatoshiHaramura Aug 13, 2022

komagata left a comment

	notification_hibernated:
	kind: 19
	user: mentormentaro
	sender: hatsuno
	message: hatsunoさんが休会しました。
	link: "/users/<%= ActiveRecord::FixtureSet.identify(:hatsuno) %>"
	read: false


		fill_stripe_element('4242 4242 4242 4242', '12 / 50', '111')

		VCR.use_cassette 'sign_up/valid-card', record: :once, match_requests_on: %i[method uri] do

	VCR.use_cassette 'sign_up/valid-card', record: :once, match_requests_on: %i[method uri] do
	VCR.use_cassette 'sign_up/valid-card', VCR_OPTIONS do

新規ユーザー登録時にメンターにメール通知およびサイト内通知するようにした #5227

新規ユーザー登録時にメンターにメール通知およびサイト内通知するようにした #5227

Conversation

SatoshiHaramura commented Jul 14, 2022 • edited Loading

関連Issue

概要

環境準備

確認方法と確認内容

方針

1. メール通知の場合

2. サイト内通知の場合

テストについての補足

SatoshiHaramura commented Jul 23, 2022

yocaji left a comment

Choose a reason for hiding this comment

yocaji Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SatoshiHaramura Aug 2, 2022 • 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

SatoshiHaramura Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

yocaji left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SatoshiHaramura Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

SatoshiHaramura commented Aug 2, 2022

yocaji left a comment

Choose a reason for hiding this comment

SatoshiHaramura commented Aug 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SatoshiHaramura commented Aug 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komagata left a comment

Choose a reason for hiding this comment

SatoshiHaramura commented Jul 14, 2022 •

edited

Loading

yocaji Jul 26, 2022 •

edited

Loading

SatoshiHaramura Aug 2, 2022 •

edited

Loading

SatoshiHaramura Aug 10, 2022 •

edited

Loading

SatoshiHaramura Aug 2, 2022 •

edited

Loading