Discordチャットに今日と明日分の開催イベント一覧を投稿する + 祝日非開催である定期イベントはDiscordに投稿しない by toke04 · Pull Request #6565 · fjordllc/bootcamp

toke04 · 2023-05-27T03:10:24Z

Issue

チャットに投稿される明日開催のイベント一覧の表示を変えたい。 #6457
祝日非開催である定期イベント開催のお知らせがDiscordに通知されている #6486

概要

Discordチャットに投稿される、開催イベント一覧の表示方法を変更 + 仕様変更を行いました。
変更により、以下の仕様になります。

❌明日分のみ投稿 => 🟢今日 + 明日に変更
❌祝日非開催イベントが、開催されると誤解されるように投稿されていた=> 🟢祝日に開催されない設定をしたイベントなら、注意書き(⚠️)をして、開催されないのが分かるようにした

変更確認方法

ブランチfeature/change_event_notification_to_discordをローカルに取り込む
bin/rails sでローカル環境を立ち上げる
こちらの公式資料を参考にしながら、「自分専用のDiscordサーバーの作成」を行い、自分宛に通知テストを行う。
※過去にDiscord通知サーバーの作成方法を書いた資料があったので、リンクを載せます

「サーバーを追加」を押す

「オリジナルの作成」を押す

名前を付けて新規作成を押したら「自分専用のDiscordサーバーの作成」が完了

こちらの写真のように、「チャネルの編集ボタン」を押す(歯車マーク)

「連携サービス」から「ウェブックの作成」を押す

「URLをコピー」を押せば、「Webhook URL」の取得は完了です

[完成形]

サーバーの作成を完了したら、

bootcamp/app/notifiers/discord_notifier.rb

Line 58 in addcd1b

webhook_url: webhook_url

の58行目の、webhook_url のバリューを、自分で取得した「Webhook URL」にする

http://localhost:3000/scheduler/daily/notify_coming_soon_regular_events ←のURLにアクセスすると、先ほど作成した「自分専用のDiscordサーバー」に通知が届くことを確認する。

↓
「自分専用のDiscordサーバー」に通知が届く

Screenshot

変更前

[明日の分のみ、このように投稿されていた]

[祝日非開催のイベントも投稿されていた]

変更後

このように、「今日 + 明日」開催のイベントが投稿されるようになった(祝日非開催のイベントは注意書き(⚠️)をして、開催されないのが分かるようにした)

toke04 · 2023-05-28T09:59:38Z

@OdenTakashi
お疲れ様です。
本issueのレビューをお願いしてもよろしいでしょうか？

二つ分のissueが混ざっており、動作確認も少し手間がかかってしまいますので、
時間を多く取って頂いて大丈夫です🙇‍♂️

OdenTakashi · 2023-05-30T01:43:28Z

@syo-tokeshi
お疲れ様です。
ご連絡遅れてしまい申し訳ありません。💦

レビュー依頼いただいて申し訳ないのですが、体調を崩してしまっているため他の方にレビュー依頼をお願いしていただいてもよろしいでしょうか？🙇‍♂️

toke04 · 2023-05-30T01:46:41Z

@OdenTakashi
かしこまりました🙇‍♂️
OdenTakashi さんの体調が良くなる事をお祈りいたします🙏
親切に返事、ありがとうございます😊

@ymmtd0x0b
お疲れ様です。
本issueのレビューをお願いしてもよろしいでしょうか？

二つ分のissueが混ざっており、動作確認も少し手間がかかってしまいますので、
時間を多く取って頂いて大丈夫です🙇‍♂️

ymmtd0x0b · 2023-05-30T04:48:26Z

@syo-tokeshi
了解しました！確認させて頂きます💪

toke04 · 2023-05-30T04:55:24Z

@syo-tokeshi 了解しました！確認させて頂きます💪

ありがとうございます🙇‍♂️
全然無理をなさらずに、ゆっくりで良いのでよろしくお願い致します🙇‍♂️

toke04 · 2023-05-31T09:43:31Z

@ymmtd0x0b
お疲れ様です。
今日MTGの話で出たように、仕様変更になりましたので、
変更後、テストをパスしましたら＠を付けてお知らせいたします🙇‍♂️

それまでしばらくレビューをお待ち頂けたら嬉しいです🙇‍♂️

toke04 · 2023-06-01T05:15:13Z

@ymmtd0x0b
お待たせ致しました！
変更完了しました🙇‍♂️

ゆっくりで良いので、レビューよろしくお願い致します🙇‍♂️

ymmtd0x0b

お待たせしました🙏
動作確認OKでした！動的に変化する定形文章の構築は凄い難しそうですね...！

いくつかコメントさせて頂いているので、確認をお願いします🙇‍♂️

ymmtd0x0b · 2023-06-03T19:35:05Z

app/notifiers/discord_notifier.rb

+    event_info = "⚡️⚡️⚡️イベントのお知らせ⚡️⚡️⚡️\n\n"
+    event_info = add_event_info(today_events, '今日', today, event_info)
+    event_info += "------------------------------\n\n" if today_events.present?
+    event_info = add_event_info(tomorrow_events, '明日', tomorrow, event_info)
+    event_info += '⚡️⚡️⚡️⚡️⚡️⚡️⚡️⚡️⚡️⚡️⚡️⚡️⚡️⚡️'


ココと add_event_infoメソッド にて evnet_info へ定形文を追加している部分は、文字列リテラルを利用した方が保守しやすそうかなと思いましたが、いかがでしょうか？🙋‍♂️

add_event_infoメソッド で返された文字列を埋め込むカタチにすると event_info を渡す必要も無くなりそうです🙌 ( その場合、後置if でメソッドの実行判定を行うかメソッドの最初でイベントが無い場合は return する処理が必要になると思います )

event_info = <<~TEXT.gsub(/^\n+/, "\n").chomp ⚡️⚡️⚡️イベントのお知らせ⚡️⚡️⚡️ #{add_event_info(today_events, '今日', today)} #{"------------------------------" if today_events.present?} #{add_event_info(tomorrow_events, '明日', tomorrow)} ⚡️⚡️⚡️⚡️⚡️⚡️⚡️⚡️⚡️⚡️⚡️⚡️⚡️⚡️ TEXT

実際に試した結果、正しくその通りだと思いました🙇‍♂️(劇的に読みやすくなったし引数も減ってgood過ぎました！)
採用させて頂きます🙇‍♂️
そして勉強になりました！！😊

[追記]
<<~TEXT.gsub(/^\n+/, "\n").chompこちら、何を参考に学んだ感じでしょうか？
上手いですね😊

[追記]
<<~TEXT.gsub(/^\n+/, "\n").chompこちら、何を参考に学んだ感じでしょうか？
上手いですね😊

ありがとうございます🙌
変更前のコードで <<~TEXT.chomp と記述されていたので、通常の文字列と同様に処理できそうだなと思って試してみたら上手く言った感じです！

bootcamp/app/notifiers/discord_notifier.rb

Line 48 in 0ac049e

event_info = <<~TEXT.chomp

なるほどです！
そう思って解決されたのですね😊
参考になりました！🙇‍♂️

ymmtd0x0b · 2023-06-03T19:35:29Z

app/notifiers/discord_notifier.rb

+  def add_event_info(events, date_message, date, event_info)
+    day_of_the_week = %w[日 月 火 水 木 金 土]
+    event_info += "< #{date_message} (#{date.strftime('%m/%d')} #{day_of_the_week[date.wday]} 開催 >\n\n" if events.present?
+    held_events, not_held_events = separate_events(events, date)


開催イベントとお休みイベントの抽出処理は partitionメソッドを利用するとスッキリ記述できそうかなと思いました！

held_events, not_held_events = events.partition { |event| event.hold_national_holiday || !HolidayJp.holiday?(date) }

スッキリして最高でした！😊
便利なメソッドを知らず、無駄な処理書いて複雑にしていましたね、、😭

1件ですが、「祝日には開催しないイベント」の判定ですが、
山本さんのコードの方が素直で読みやすいと思うのですが、
&&演算子を使ったほうが、

開催されるイベント

開催されないイベント

の条件が明確で良い気がします🙇‍♂️

not_held_events, held_events = events.partition { |event| !event.hold_national_holiday && HolidayJp.holiday?(date) }

特定の条件が揃った時のみイベントは開催されないので、こっちの方が良いと思いました🙇‍♂️

ただ、私の方がベターでない可能性もあるので、駒形さんに判断を委ねたいと思いました、、(山本さんの方がコードが素直で読みやすいからです)

&&の方が条件が限定的になるので、予期せぬ不具合が起きにくくなって良いと思うんだけど、どうなんだろう？

@komagata

1

held_events, not_held_events = events.partition { |event| event.hold_national_holiday || !HolidayJp.holiday?(date) }

2

not_held_events, held_events = events.partition { |event| !event.hold_national_holiday && HolidayJp.holiday?(date) }

こちら、どちらを採用した方が良さそうでしょうか？

@syo-tokeshi
駒形さんへの確認ありがとうございます！
判定式の内容的には同じはずなので、私としては syo-tokeshiさんの好みの方を採用して頂いて問題ないように思ってます〜

@syo-tokeshi どちらでもいいかなと思います。
ただ１行が長いので改行するなに何かした方がいいと思います〜

@komagata
こちらで対応しました🙇‍♂️
コンフリクトの修正 + 一部処理を複数行に分ける

not_held_events, held_events = events.partition do |event| !event.hold_national_holiday && HolidayJp.holiday?(date) end

ymmtd0x0b · 2023-06-03T19:35:33Z

db/fixtures/regular_event_repeat_rules.yml

+regular_event_repeat_rule35:
+  frequency: 0
+  day_of_the_week: 6
+  regular_event: regular_event30


最終行の改行が抜けてるみたいです👀

失礼しました🙇‍♂️
ご指摘ありがとうございます🙇‍♂️

ymmtd0x0b · 2023-06-03T19:35:56Z

test/notifiers/discord_notifier_test.rb

+    travel_to Time.zone.local(2023, 5, 5, 6, 0, 0) do
+      assert_notifications_enqueued 2, **expected do
+        DiscordNotifier.with(params).coming_soon_regular_events.notify_later
+        DiscordNotifier.coming_soon_regular_events(params).notify_later


実際に利用されているのが notify_now だけでしたら、この部分は不要そうとも思ったのですがいかがでしょうか🤔？

bootcamp/app/models/notification_facade.rb

Lines 40 to 42 in 78bf85b

def self.coming_soon_regular_events(today_events, tomorrow_events)

DiscordNotifier.with(today_events: today_events, tomorrow_events: tomorrow_events).coming_soon_regular_events.notify_now

end

確認いたしました🙇‍♂️
確かに、かなりごもっともですね、、

１点気になることがございまして、
test/notifiers/discord_notifier_test.rb
こちらの他のテストコードもnotify_now しか使ってないけど、

同期

非同期

両方テストを行なっているので、無意味な気がしますがそのまま前例に倣っても良い気がしました、、 (が、消すべきかな？やっぱり)

例えばこちらの、invalid_userというメソッドも、notify_nowしか使っていない

駒形さんに聞いてみます🙇‍♂️

@komagata
こちら、使ってない非同期のテスト(notify_laterを使ったテスト)は削除した方がよろしいでしょうか？

@syo-tokeshi
すみません！変更前のコードは notify_now だけだったので、てっきり利用する方だけをテストすれば良いのかなと思っていました🙏
確かに他の箇所では使用の有無に関わらず両方テストされていますね...！ですので、両方記載した方が良さそうですね！

@syo-tokeshi AbstructNotrifierは同期・非同期に対応しているものと作るライブラリなので使う人は両方が対応されていることを想定しています。ですので両方のテストが欲しいです。

@komagata

使う人は両方が対応されていることを想定しています

かしこまりました。
だから両方必要なんですね🙇‍♂️

ymmtd0x0b · 2023-06-03T19:36:21Z

test/system/notification/regular_events_test.rb

+    travel_to Time.zone.local(2023, 5, 5, 6, 0, 0) do
+      visit_with_auth '/scheduler/daily/notify_coming_soon_regular_events', 'komagata'


スケジューラへのリクエストは非ログインでもOKに変更されたようなので、予想外のテストエラーを予防する意味で仕様と合わせた方が良さそうに思いました！

定期処理にログインが必須になっていたのを修正 #6019

情報ありがとうございます！！🙇‍♂️

確認し、修正致しました😄

ymmtd0x0b · 2023-06-03T19:51:27Z

app/notifiers/discord_notifier.rb

+    today_events = params[:today_events] || RegularEvent.today_events
+    tomorrow_events = params[:tomorrow_events] || RegularEvent.tomorrow_events


変更前のコードもパラメータからの代入のみとなっているので、OR演算子以降の部分は削除しても支障はなさそうですが、いかがでしょうか？ ( もし、開催するイベントが無い場合はコントローラー側と同じ処理を実行する事になりそう...？ )

bootcamp/app/notifiers/discord_notifier.rb

Lines 42 to 44 in 0ac049e

def tomorrow_regular_event(params = {})

params.merge!(@params)

event = params[:event]

その通りでした、、
コントローラーから処理がスタートして、
eventのパラメーターを渡しているので、OR演算子以降はいらないですね、、🙇‍♂️

bootcamp/app/controllers/scheduler/daily/notify_coming_soon_regular_events_controller.rb

Lines 11 to 16 in 78bf85b

def notify_coming_soon_regular_events

today_events = RegularEvent.today_events

tomorrow_events = RegularEvent.tomorrow_events

return if today_events.blank? && tomorrow_events.blank?

NotificationFacade.coming_soon_regular_events(today_events, tomorrow_events)

ご指摘ありがとうございます🙇‍♂️

toke04 · 2023-06-04T10:55:38Z

@ymmtd0x0b
代替案まで出してくれて本当にありがとうございます🙇‍♂️
もの凄く勉強になりました🙇‍♂️
有意義な時間ありがとうございます😊

toke04 · 2023-06-07T10:10:33Z

@machida
お疲れ様です。
今日MTGで話しましたDiscordへの通知の文言の修正完了しました🙇‍♂️

こちらでよろしいでしょうか？
通知の文言の微調整を行う

変更点

曜日の後ろに)を付与
⚠️ の後ろのスペースを「半角」に変更

[旧]

[新]

[新しい通知の全体スクショ]

machida · 2023-06-07T10:19:49Z

@syo-tokeshi 確認しましたー👍
これでレビューに進めてくださいー
対応ありがとうございます🙏

toke04 · 2023-06-11T03:43:05Z

@komagata
お疲れ様です。
一件だけ対応しました🙇‍♂️
ご確認よろしくお願い致します
#6565 (comment)

komagata

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

komagata · 2023-06-20T14:59:54Z

test/system/notification/regular_events_test.rb

-    travel_to Time.zone.local(2022, 7, 30, 0, 0, 0) do
-      visit_with_auth '/scheduler/daily/notify_tomorrow_regular_event', 'komagata'
+    travel_to Time.zone.local(2023, 5, 5, 6, 0, 0) do
+      visit '/scheduler/daily/notify_coming_soon_regular_events'


machida · 2023-07-17T06:32:17Z

@komagata こちらの通知が本番で飛んでないそうです。確認をお願いします。

@syo-tokeshi 足りない情報がありましたらこちらに追記お願いします。

komagata · 2023-07-18T08:53:23Z

@machida かしこまりました。

@syo-tokeshi 僕のミスで本番で該当URLが毎日アクセスされる設定がされておりませんでした。先ほど設定して実行したので再度確認いただければありがたいです。

toke04 · 2023-07-18T09:18:45Z

@machida かしこまりました。

@syo-tokeshi 僕のミスで本番で該当URLが毎日アクセスされる設定がされておりませんでした。先ほど設定して実行したので再度確認いただければありがたいです。

@komagata
ご確認ありがとうございます🙇‍♂️

かしこまりました！
明日、チャットを確認しましたら、結果報告します🙇‍♂️

toke04 · 2023-07-19T02:24:10Z

@komagata
Discordに通知が来ていることが確認できたのでcloseさせて頂きます🙇‍♂️

ご対応ありがとうございました🙇‍♂️

toke04 requested review from komagata and removed request for komagata May 27, 2023 13:36

toke04 force-pushed the feature/change_event_notification_to_discord branch from ef8d860 to dd0d0b9 Compare May 28, 2023 03:14

toke04 marked this pull request as ready for review May 28, 2023 03:17

toke04 requested a review from OdenTakashi May 28, 2023 10:04

toke04 requested review from ymmtd0x0b and removed request for OdenTakashi May 30, 2023 01:47

machida assigned toke04 Jun 2, 2023

toke04 force-pushed the feature/change_event_notification_to_discord branch from fb549e1 to 78bf85b Compare June 2, 2023 06:44

ymmtd0x0b reviewed Jun 4, 2023

View reviewed changes

toke04 requested a review from komagata June 4, 2023 15:07

toke04 added 5 commits June 11, 2023 11:58

スケジューラーの呼び出しを、今日と明日分呼び出しのnotify_coming_soon_regular_eventに変更する

ceeb8a9

テスト用データに、土曜開催のDiscord通知確認用イベントを追加する

ad6445f

祝日に非開催の場合は、通知が飛ばないように変更

c24fbcd

不要なputsを削除

820f202

開発環境とテスト環境の「regular_event26」のデータを同じにする

92f51ca

toke04 added 18 commits June 11, 2023 11:59

rubocopからの指摘「Metrics/CyclomaticComplexity」に対処するために、処理を関数に切り分けた

12d72a3

呼ばれるコントローラー + URLが変わったので、その変更分を反映させる

d773f59

rubocopの指摘を反映

2533a7d

不要な関数を削除する

7ee1697

日付が上手く表示されない問題を修正

14e92e6

祝日日開催のイベントを、注意書きで表示するように変更

2539ff8

関数名の変更 + 本番用にコードを戻す

6c7c811

祝日非開催イベントを一つ追加 + それに伴うテストデータを微調整

fe495f1

Discordへの通知の文言が変わったので、それに伴うテストの変更 + 不要になったテストを削除(統合出来たので)

7971070

rubocopの指摘を反映

658321e

conflictの解消に伴い(#6382を反映)、テストデータ一つ分を加えた実装に変更

a49a4c8

スケジューラーへのリクエストを、認証を行わないでテストするように変更

b9f9d72

付け忘れた最終行の改行を付与

a29a374

不要になったメソッドの削除 + 文字列リテラルを上手く使った実装に変更

87bae36

不要なOR演算子を削除

553d36f

一部処理を三項演算子に置き換える

844d9bb

通知の文言の微調整を行う

e4ad343

コンフリクトの修正 + 一部処理を複数行に分ける

63e08a0

toke04 force-pushed the feature/change_event_notification_to_discord branch from 7f8c540 to 63e08a0 Compare June 11, 2023 03:21

komagata approved these changes Jun 20, 2023

View reviewed changes

komagata merged commit b39c3d6 into main Jun 20, 2023

komagata deleted the feature/change_event_notification_to_discord branch June 20, 2023 15:00

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

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

Merged

13 tasks

machida mentioned this pull request Aug 7, 2023

ダッシュボードの近日開催のイベントに、定期イベントが休みの場合も表示したい。 #6778

Closed

machida mentioned this pull request Apr 16, 2024

定期イベントに休日を登録できるようする #7672

Closed

	def self.coming_soon_regular_events(today_events, tomorrow_events)
	DiscordNotifier.with(today_events: today_events, tomorrow_events: tomorrow_events).coming_soon_regular_events.notify_now
	end

		travel_to Time.zone.local(2023, 5, 5, 6, 0, 0) do
		visit_with_auth '/scheduler/daily/notify_coming_soon_regular_events', 'komagata'

		today_events = params[:today_events] \|\| RegularEvent.today_events
		tomorrow_events = params[:tomorrow_events] \|\| RegularEvent.tomorrow_events

	def tomorrow_regular_event(params = {})
	params.merge!(@params)
	event = params[:event]

	def notify_coming_soon_regular_events
	today_events = RegularEvent.today_events
	tomorrow_events = RegularEvent.tomorrow_events
	return if today_events.blank? && tomorrow_events.blank?

	NotificationFacade.coming_soon_regular_events(today_events, tomorrow_events)

Conversation

toke04 commented May 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

概要

変更確認方法

Screenshot

変更前

変更後

Uh oh!

toke04 commented May 28, 2023

Uh oh!

OdenTakashi commented May 30, 2023

Uh oh!

toke04 commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ymmtd0x0b commented May 30, 2023

Uh oh!

toke04 commented May 30, 2023

Uh oh!

toke04 commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toke04 commented Jun 1, 2023

Uh oh!

ymmtd0x0b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toke04 Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toke04 Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toke04 Jun 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toke04 Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toke04 Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toke04 commented Jun 4, 2023

toke04 commented May 27, 2023 •

edited

Loading

toke04 commented May 30, 2023 •

edited

Loading

toke04 commented May 31, 2023 •

edited

Loading

toke04 Jun 4, 2023 •

edited

Loading

toke04 Jun 4, 2023 •

edited

Loading

toke04 Jun 11, 2023 •

edited

Loading

toke04 Jun 4, 2023 •

edited

Loading

toke04 Jun 4, 2023 •

edited

Loading

toke04 commented Jun 7, 2023 •

edited

Loading

toke04 commented Jul 18, 2023 •

edited

Loading