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

「イベントのお知らせ」に記載されるイベントの並び順を開始時間順に変更した #7923

Conversation

ham-cap
Copy link
Contributor

@ham-cap ham-cap commented Jul 3, 2024

Issue

概要

・Discordに通知される「イベントのお知らせ」の通知に記載されるイベントが複数ある場合に開始時間順に並ぶように変更しました。この変更に伴い、テストの際にイベントの並び順を確認できるようにfixturesのデータとテストの記載も変更しました。

変更確認方法

⚠️このPRにおける動作確認については、Discordに対して行われる通知の内容を確認するため、Discordサーバー(FBCとは別のもの)の用意とチャンネルの作成及び当該チャンネルのWebhook URLの取得をしていただく必要があります。お手数ですが、以下の手順にて確認をお願いいたします🙇‍♂️(既に何らかのissueで確認用のDiscordサーバーをお持ちの方はそちらをご使用いただいて差し支えございません🙆‍♂️)

Discordサーバーの準備

  1. BootcampのWikiにある、Develop環境でのDiscord通知の確認方法
    に記載の手順にしたがってサーバー追加、チャンネル作成、当該チャンネルのWebhook URLの取得を行ってください。

環境変数の設定

  1. 同じくBootcampのWikiにあるDiscord通知を参照し、環境変数の設定をしてください。Wikiにも言及のあるdirenvを利用するのが手軽で便利です。
    direnvの導入の仕方については公式のほか、以下のページもわかりやすかったので適宜ご参照ください。
    参考:direnvを使おう

今回であれば.envrcの中に以下のとおり設定すると全員宛のDiscord通知の送信先が上記で取得したDiscordサーバーになります。

export DISCORD_ALL_WEBHOOK_URL=取得したWebhook URL

動作確認

システムテストを実行して実際に通知をとばし、本文を確認します。

  1. feature/change-events-order-in-events-notifications-into-chronological-orderをローカルに取り込む
  2. bin/rails test test/system/notification/regular_events_test.rbを実行する
  3. 上記で作成したDiscordチャンネルに「イベントのお知らせ」が届くので、5/6開催分のイベントが開始時間の早い順に並んでいることを確認する

⚠️notify_coming_soon_regular_eventsのテストが失敗しますが、これは上記で設定したとおり確認用のDiscordサーバーに通知を送信するためにDevelopment環境で独自の環境変数(DISCORD_ALL_WEBHOOK_URL)を設定していることによるものです。DISCORD_ALL_WEBHOOK_URLに何も設定しない状態(= 本番と同じ状態)で実行すると通ります。

Screenshot

変更前

_全員への通知___ham-cap_ハム_のサーバー_-_Discord-2

変更後

_全員への通知___ham-cap_ハム_のサーバー_-_Discord

@ham-cap ham-cap self-assigned this Jul 3, 2024
@ham-cap ham-cap requested a review from nakamu-kazu222 July 3, 2024 08:03
@ham-cap ham-cap marked this pull request as ready for review July 3, 2024 08:04
@ham-cap
Copy link
Contributor Author

ham-cap commented Jul 3, 2024

@nakamu-kazu222
お疲れ様です!
こちらのPRにつきまして、お手隙の際にレビューをお願いできますでしょうか🙏
ご都合が悪い場合は遠慮せずお知らせください🙆‍♂️
よろしくお願いいたします!

@nakamu-kazu222
Copy link
Contributor

@ham-cap

お疲れ様です!
1週間ほどお時間をいただくと思いますが、よろしいでしょうか?

@ham-cap
Copy link
Contributor Author

ham-cap commented Jul 5, 2024

@nakamu-kazu222
急ぎではないので問題ありません🙆‍♂️
よろしくお願いします🙏

@nakamu-kazu222
Copy link
Contributor

nakamu-kazu222 commented Jul 8, 2024

@ham-cap

お疲れ様です!
すみません、動作確認がうまく実行されないので、確認していただいてもよろしいでしょうか?

私が行った手順は以下です

  1. Discordにテスト用のサーバーを用意する
  2. ウェブフックURLをコピーして、app/models/chat_notifier.rbの13行目に追加する
    if Rails.env.production?
      Discord::Notifier.message(message, username:, url: webhook_url)
    else
      Discord::Notifier.message(message, username: username, url: 'ウェブフックURL')
      Rails.logger.info 'Message to Discord.'
    end
  1. direnvの設定
  • インストール
    • % brew install direnv
  • shellにhookを追加する
    • % direnv edit .
      export EDITOR=vim
      eval "$(direnv hook bash)"
      export DISCORD_ALL_WEBHOOK_URL=取得したWebhook URL
      
  1. % bin/rails test test/system/notification/regular_events_test.rbを実行する
  • ログなどは表示されない

@ham-cap
Copy link
Contributor Author

ham-cap commented Jul 9, 2024

@nakamu-kazu222
ご連絡ありがとうございます🙏
確証はないのですが、direnv edit .で編集する.envrcに記載するのは

export DISCORD_ALL_WEBHOOK_URL=取得したWebhook URL

のみで、それ以外の

export EDITOR=vim
eval "$(direnv hook bash)"

の部分はshellの設定ファイル(.bashrc.zshrc)にご記載いただくものになりますので、その点を一度ご確認いただけますでしょうか👀
私の場合だとshellはzshなので.zshrcに記載しました!

Comment on lines 52 to 56
#{add_event_info(today_events.sort_by(&:start_at), '今日', today)}

#{'------------------------------' if today_events.present?}

#{add_event_info(tomorrow_events, '明日', tomorrow)}
#{add_event_info(tomorrow_events.sort_by(&:start_at), '明日', tomorrow)}
Copy link
Contributor

@nakamu-kazu222 nakamu-kazu222 Jul 10, 2024

Choose a reason for hiding this comment

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

@ham-cap

すみません!動作確認できました!
動作は問題ありませんが、1点コードで確認したい点があります!

見当違いでしたらすみません
並び順を#{add_event_info(today_events.sort_by(&:start_at), '今日', today)}で変更していますが、以下のようにadd_event_infoの引数に呼び出す前に定義するのはいかがでしょうか?

today_events = params[:today_events].sort_by(&:start_at)
tomorrow_events = params[:tomorrow_events].sort_by(&:start_at)

今後、どこかでtoday_eventsとtomorrow_eventsを使う場合に並び順が変更されたままになり、並び順で同じ問題が起きなさそうと考えました

ご確認のほどよろしくお願い致します!

@ham-cap ham-cap force-pushed the feature/change-events-order-in-events-notifications-into-chronological-order branch from 41372af to 79f5f3e Compare July 17, 2024 22:12
@ham-cap
Copy link
Contributor Author

ham-cap commented Jul 17, 2024

@nakamu-kazu222
ご指摘ありがとうございます🙏
たしかにご提案いただいたタイミングの方が読みやすさ的にも良いと思えましたので修正させていただきました🙌
再度ご確認ください🙇‍♂️

@ham-cap ham-cap force-pushed the feature/change-events-order-in-events-notifications-into-chronological-order branch from 79f5f3e to 5019368 Compare July 26, 2024 02:09
@ham-cap
Copy link
Contributor Author

ham-cap commented Jul 28, 2024

@nakamu-kazu222
お疲れ様ですー
こちらのレビューいかがでしょうか?👀
お忙しい中恐縮ですが、よろしくお願いいたします🙏

@nakamu-kazu222
Copy link
Contributor

@ham-cap

返信が遅れまして、申し訳ございません。

ただいまレビューしております。
本日中にはレビューが完了する予定です。
よろしくお願い致します。

@nakamu-kazu222
Copy link
Contributor

@ham-cap

動作、コードともに問題ございませんので、Approveしました。
返信にお時間いただいてしまい、申し訳ございません。

今後ともよろしくお願いいたします

@ham-cap
Copy link
Contributor Author

ham-cap commented Aug 4, 2024

@nakamu-kazu222
お忙しい中ご確認いただきありがとうございました🙏

@komagata
メンバーにApproveいただきましたのでレビューをお願いいたします🙇‍♂️

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 668d863 into main Aug 8, 2024
3 checks passed
@komagata komagata deleted the feature/change-events-order-in-events-notifications-into-chronological-order branch August 8, 2024 07:01
@github-actions github-actions bot mentioned this pull request Aug 8, 2024
17 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