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

日報が公開されたらDiscordの専用チャンネルに通知する機能の実装 #4325

Merged
merged 3 commits into from
Mar 10, 2022

Conversation

aim2bpg
Copy link
Contributor

@aim2bpg aim2bpg commented Mar 2, 2022

Issue: #4161

概要

日報が公開されたらDiscordの専用チャンネルに通知する機能を実装しました。

なお、Discord通知において開発環境で動作確認する際は、ソースコードを変更しての確認が必要です。
詳細は、動作確認手順をご覧ください。

参考PR

Refs: #4206 他、#3469,#3471 を参照。

変更点

日報保存後のコールバックafter_saveの中で、日報の初回公開時の通知先として、Discordを追加。
なお、今回の変更に伴って、Discordに新規追加する「日報通知」チャンネルのためのウェブフック用の環境変数「DISCORD_REPORT_WEBHOOK_URL」については、駒形さんに本番環境に追記していただく予定。

変更理由

https://discord.com/channels/715806612824260640/823739924615790622/938959818729848844 で議論された、サイドバーから日報が表示されなくなった件の代替機能となる。

Bootcampアプリ内に専用ページを作る、という意見もあったが、「自分から読みに行かないと気づけない情報」よりも、「勝手に目に飛び込んでくる情報」である方が嬉しい人もいる。(プル型 vs プッシュ型)

そこで、Discordに日報通知チャンネルを作って、日報が公開されたらそこに通知が飛ぶ(=かつてのサイドバーの役割をDiscordが受け持つ)ようにすることで、Discordの未読チェック機能を使うときに気づける仕組みを今回作ることにした。

なお、他の人の日報を見たくない人はチャンネルをミュートにすれば無視することもできる。

動作確認結果

日報が通知されるタイミングは、従来のbootcampアプリと同じタイミングのため、別日、別ユーザーの日報でも通知が正しく表示されることを確認した。

image

動作確認手順

  1. 手元に本ブランチを取り込む。
  2. dotenv-railsというgemをインストールする
    参考:Railsで使える環境変数を管理できるgem(dotenv-rails)や.envの導入方法 - Qiita
# Gemfile
   # not default
   gem 'bullet'
   gem 'bundle_outdated_formatter'
+  gem 'dotenv-rails'
   gem 'letter_opener_web', '~> 1.0'
   gem 'rack-dev-mark'
   gem 'rack-mini-profiler', '~> 2.0'
$ bundle install
  1. .envファイルをbootcampディレクトリ配下に作成する
$ touch .env
  1. 自分用のDiscordサーバーを作成する
    参考:Discord – サーバーの作り方と削除する方法 - 設定Lab

  2. 4.で作成したサーバーのウェブフックURLをコピーしておく
    参考:DiscordのWebhookについて

  3. コピーしたウェブフックURLを環境変数として使いたいので、作成した.envファイルには下の内容を追記する

DISCORD_TEST_WEBHOOK_URL = コピーしたウェブフックURL
  1. app/models/chat_norifier.rbのmessageメソッドの記述を変更する
# app/models/chat_norifier.rb
class ChatNotifier
  def self.message(
    message,
    username: 'ピヨルド',
    webhook_url: ENV['DISCORD_NOTICE_WEBHOOK_URL']
  )

-   if Rails.env.production?
+   if Rails.env.development?
      Discord::Notifier.message(message, username: username, url: webhook_url)
    else
      Rails.logger.info 'Message to Discord.'
    end
  end
..

Rails.env.development?にすることでtrueの分岐を実行することができる。
参考:【Rails】 envメソッドで環境を確認する方法と各コマンドの指定方法 - Pikawaka

  1. app/models/の環境変数を6で設定した環境変数に変える
  def notify_to_chat(report)
-    ChatNotifier.message(<<~TEXT, webhook_url: ENV['DISCORD_REPORT_WEBHOOK_URL'])
+    ChatNotifier.message(<<~TEXT, webhook_url: ENV['DISCORD_TEST_WEBHOOK_URL'])
      #{report.user.login_name}さんが#{I18n.l report.reported_on}の日報を公開しました。
      タイトル:「#{report.title}
      URL: https://bootcamp.fjord.jp/reports/#{report.id}
    TEXT
  end
  1. 動作確認をする
  • 日報を新規作成し、提出する。
  • 自分で作成したDiscordチャンネルに通知がくるのを確認する。

@aim2bpg aim2bpg marked this pull request as ready for review March 2, 2022 08:23
@aim2bpg aim2bpg requested a review from tksmasaki March 2, 2022 08:23
@aim2bpg
Copy link
Contributor Author

aim2bpg commented Mar 2, 2022

@tksmasaki さん、下記ご注意の上で、お手すきの際にレビューをお願いいたします🙏

Discord通知において開発環境で動作確認する際は、ソースコードを変更しての確認が必要です。
詳細は、動作確認手順をご覧ください。

Copy link
Contributor

@tksmasaki tksmasaki left a comment

Choose a reason for hiding this comment

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

@aim2bpg
確認手順が丁寧なおかげで、スムーズに確認できました!
LGTMです🙆‍♂️

@aim2bpg aim2bpg requested a review from komagata March 2, 2022 11:16
@aim2bpg
Copy link
Contributor Author

aim2bpg commented Mar 2, 2022

@tksmasaki さん、ご確認ありがとうございました😄

@komagata さん、チームリーダーレビューをお願いいたします🙏
下記につきまして、お手数となりますが、設定をお願いいたします🙇🏻‍♂️

今回の変更に伴って、Discordに新規追加する「日報通知」チャンネルのためのウェブフック用の環境変数「DISCORD_REPORT_WEBHOOK_URL」については、駒形さんに本番環境に追記していただく予定。

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.

こちらテストが欲しいです〜。

Discordを実際にテストしなくていいですが、メッセージがログに出力されるようにした上でそのログがちゃんと出ているかのテストをすると良いと思います。(どこか他の場所でそういうテストあった気がします)

@aim2bpg
Copy link
Contributor Author

aim2bpg commented Mar 4, 2022

@komagata さん

Discordを実際にテストしなくていいですが、メッセージがログに出力されるようにした上でそのログがちゃんと出ているかのテストをすると良いと思います。(どこか他の場所でそういうテストあった気がします)

GitHubのPRページで「通知 discord」を検索してみましたが、軒並みテストが入ってないように見えます。検索ワードが違っていたり、「どこか他の場所」が分かりましたら、ご教示ください。🙏

「通知 discord」でヒットしたPR:#4206 ,#3779, #3471 ,#3469 ,#2568 ,#2463 ,#2404 ,#2403 ,#2391 ,#2267 ,#2250 ,#2215

@komagata
Copy link
Member

komagata commented Mar 7, 2022

@aim2bpg すみません、でしたら消した古い機能の部分だったのだと思います。
通知部分でログを出力するようにして、テストの部分でそのログが出力されていることを確認したらOKとしたいと思います〜

@aim2bpg aim2bpg force-pushed the feature/notify-chat-when-report-is-published branch 2 times, most recently from 49897c6 to 68fa3c6 Compare March 8, 2022 08:14
@aim2bpg
Copy link
Contributor Author

aim2bpg commented Mar 8, 2022

@komagata さん、テスト追加しましたのでご確認をお願いいたします🙏

消した古い機能の部分だったのだと思います。

おそらく、slack時代のこちらのPRのことかと思いました〜

ネットで結構検索しましたが、しっくりこなかったので過去のPRを見に行ったら、ありました!
これまで、mockやstubを使ったことがなかったので、大変勉強になりました〜😄

ネット上の記事だと↓が参考になりそうだと思いました。
minitestでloggerの出力内容をテスト - Qiita
Minitest::Assersionsのassert系メソッドを全部試してみた - Qiita

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

require 'application_system_test_case'
require 'minitest/mock'
Copy link
Member

Choose a reason for hiding this comment

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

test_helper.rbで読み込んじゃって良いかと思います〜

@aim2bpg aim2bpg force-pushed the feature/notify-chat-when-report-is-published branch from 68fa3c6 to 8876fd9 Compare March 9, 2022 22:37
@aim2bpg
Copy link
Contributor Author

aim2bpg commented Mar 9, 2022

@komagata さん、修正しましたのでご確認をお願いいたします🙏

test_helper.rbで読み込んじゃって良いかと思います〜

ご提案ありがとうございます。
たしかに今後他の機能でmockを使いたい場面は容易に想像できるため、上流である継承元で定義する方が良いと思いました。

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ですー🙆‍♂️

こちらのPRとサーバーの設定でステージング環境と本番環境の環境変数を設定しました。
#4383

ステージング環境では"BOOTCAMP開発"カテゴリーの"テスト通知"チャンネルに通知が飛ぶようにしました。

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