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の通知チャンネルのURLプレビューを表示させないように変更 #7933

Merged

Conversation

ham-cap
Copy link
Contributor

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

Issue

概要

現状、BootcampからDiscord宛に自動送信される通知の中には、通知本文にURLが含まれているものが複数存在します。
Discordは通常、投稿の本文にURLがある場合に自動で以下のようなプレビューが表示されますが、文字入力の際にURLを<>で囲うとこのプレビューを表示させないようにすることができます。
<>なし
image
<>あり
image

このPRでは上記のBootcampから自動送信される通知の中で本文中にURLが含まれているものについて、Discord上でプレビューを表示させないように変更しました。

変更確認方法

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

Discordサーバーの準備

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

環境変数の設定

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

今回は上記WikiのDiscord通知に記載されている5つの環境変数を全て設定してください。.envrcの中は以下のようにすると全てのDiscord通知の送信先が上記で取得したDiscordサーバーになります。

export DISCORD_ADMIN_WEBHOOK_URL=取得したWebhook URL
export DISCORD_MENTOR_WEBHOOK_URL=取得したWebhook URL
export DISCORD_REPORT_WEBHOOK_URL=取得したWebhook URL
export DISCORD_INTRODUCTION_WEBHOOK_URL=取得したWebhook URL
export DISCORD_ALL_WEBHOOK_URL=取得したWebhook URL

動作確認

  1. feature/remove-url-previews-for-discord-notification-channelsをローカルに取り込む
  2. 以下の通知について()内のテストを実行し、プレビューが表示されないことを確認する
  • イベントのお知らせ(bootcamp/test/system/notification/regular_events_test.rb)
  • お知らせ投稿(bootcamp/test/system/announcements_test.rb)
  • XXXさんが新たなメンバーとしてJOINしました🎉(bootcamp/test/system/sign_up_test.rb)
  • プラクティス関連(bootcamp/test/system/practices_test.rb)
  • Docs関連(bootcamp/test/system/pages_test.rb)
  • 質問関連(bootcamp/test/system/questions_test.rb)
  • 🎉 XXXXさんがはじめての日報を書きました!(bootcamp/test/system/notification/reports_test.rb)
  • XXXXさんがXXXX年XX月XX日の日報を公開しました。(同上)
  • XXXさんの「XXXX」の提出物が、最後のコメントから5日経過しました。担当:@xxxxx さん(bootcamp/test/notifiers/discord_notifier_test.rb)⚠️これだけ単体テストのため実行しても通知は飛びません。テストが通っていればOKとのことなので、通ることをご確認ください。

なお、以下の2つについてはBootcampから通知しているわけではなく、外部に通知文のテキストをAPIとして提供しているだけのものです。
こちらについて駒形さんへ確認したところ、ステージングで確認する術がないことから、確認不要とのことです。

  • 未確認の日報の数をお知らせします。
  • 提出されてから日数が経っている提出物の数をお知らせします。

⚠️test/system/regular_events_test.rbtest/system/sign_up_test.rbの実行時にテストが失敗したりエラーが出る場合がありますが、これは上記で設定したとおり確認用のDiscordサーバーに通知を送信するためにDevelopment環境で独自の環境変数(DISCORD_ALL_WEBHOOK_URL等)を設定していることによるものです。これらの環境変数に何も設定しない状態(= 本番と同じ状態)で実行すると通ります。

Screenshot

変更前

全て以下のようにプレビューが表示されます。
image

変更後

各通知についてURLが記載されているだけで、プレビューがない状態です。

イベントのお知らせ

image

お知らせ投稿

image

XXXさんが新たなメンバーとしてJOINしました🎉

image

プラクティス関連

image

Docs関連

image

質問関連

image

🎉 XXXXさんがはじめての日報を書きました!

XXXXさんがXXXX年XX月XX日の日報を公開しました。

image

ham-cap added 4 commits July 5, 2024 12:16
Discordへの通知本文にURLが含まれている場合にプレビューが表示されないように変更した
Discord通知の本文にURLが含まれている場合でもプレビューが表示されないように変更。
メンター向けのDiscord通知の一部はAPIで文面を提供するだけのものだったため、それに該当するファイルを変更した。
@ham-cap ham-cap self-assigned this Jul 5, 2024
@ham-cap ham-cap requested a review from motohiro-mm July 5, 2024 08:47
@ham-cap ham-cap marked this pull request as ready for review July 5, 2024 08:47
@ham-cap
Copy link
Contributor Author

ham-cap commented Jul 5, 2024

@motohiro-mm
お疲れ様です!
お手隙の際にこちらのレビューをお願いできますでしょうか🙏
ご都合つかなそうであればお知らせください👍
よろしくお願いいたします🙇‍♂️

@ham-cap ham-cap changed the title Feature/remove url previews for discord notification channels Discordの通知チャンネルのURLプレビューを表示させないように変更 Jul 5, 2024
@motohiro-mm
Copy link
Contributor

@ham-cap

承知いたしました!
1週間をめどにレビューさせていただきます🙏
もしお急ぎでしたら、申し訳ありませんが他の方へお願いしていただければと思いますのでご連絡ください🙇‍♀️
よろしくお願いいたします!

@ham-cap
Copy link
Contributor Author

ham-cap commented Jul 6, 2024

@motohiro-mm
ありがとうございます🙏
急ぎではありませんのでゆっくりで大丈夫です🙆‍♂️
よろしくお願いいたします!

Copy link
Contributor

@motohiro-mm motohiro-mm left a comment

Choose a reason for hiding this comment

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

@ham-cap

確認させていただきました!
丁寧に変更確認方法を書いてくださりとても助かりました🙏
ありがとうございます!

コードは問題ないように思いました!

1つ伺いたいことがあります。
ham-capさんの環境では修正前の状態(mainブランチ)で指定のテストを実行するとURLのプレビュー表示が全てされている、ということで合ってますでしょうか?

私の環境でmainブランチで変更確認方法記載のテストを実行すると、URLプレビューが表示されない項目がいくつかありました。
URLプレビュー表示がなかったものは以下です。

  • 定期イベント
  • 新たなメンバーとしてJOIN
  • プラクティス関連
  • Docs関連
  • 質問関連
  • はじめての日報

mainブランチでテストを実行しURLプレビュー表示があったものは以下で、修正後のブランチでテストを実行しプレビューが表示されなくなることを確認しました。

  • お知らせ投稿
  • 通常の日報公開

環境構築として、Discordサーバーの準備と、環境変数の設定を行なっています。
私の環境になにか誤りがあって確認がうまくできていない可能性があったので伺いました。

@ham-cap
Copy link
Contributor Author

ham-cap commented Jul 9, 2024

@motohiro-mm
ご連絡ありがとうございます🙏
ご指摘の点につきましてこちらの手元でも確認してみたところ、おっしゃるとおりプレビューが表示されるものとされないものがありますね💧
私の確認不足で混乱させてしまい申し訳ありません🙇
一瞬ドメインがlocalhostだと表示されないのかなと思ったのですが、bootcampになっているものでも表示されたりされなかったりで規則性がわからないですね🤔
ただ、FBCの本物のDiscordチャンネルの「#お知らせ」や「#通知」チャンネルをご覧いただくとわかるのですが、テストの際にプレビュー表示のないものも本物のほうではプレビュー表示が出ているため、本issueの修正対象なのは間違いないです。
とはいえ、例えば「新しいメンバーとしてJOINしました」の通知などは同じ日付でも時間帯が違うとプレビュー表示がある場合とない場合が両方確認できるので、もしかするとプレビューを表示させるかどうかを判断しているDiscord側の問題なのでは、とも思えてきますね👀
image

@komagata
上記のように、同じ通知でもプレビュー表示の有無に揺れがあるのですが、レビューの確認としてはテスト実行時にDiscordの通知そのものが飛んでいることと、該当する通知のコードに<>が追記されているかどうかを確認してもらうという形でも大丈夫でしょうか?👀💦

@komagata
Copy link
Member

@ham-cap

上記のように、同じ通知でもプレビュー表示の有無に揺れがあるのですが、レビューの確認としてはテスト実行時にDiscordの通知そのものが飛んでいることと、該当する通知のコードに<>が追記されているかどうかを確認してもらうという形でも大丈夫でしょうか?👀💦

もう少しなぜなのか調べてみてください〜

@ham-cap
Copy link
Contributor Author

ham-cap commented Jul 17, 2024

@komagata @motohiro-mm
URLのプレビューが表示される場合とされない場合がある件について調査しましたので結果を共有させていただきます!

まず、大前提としてローカルでアプリケーションを立ち上げている場合は外部からのアクセスができませんので、Discordがmetaデータを取得できず、プレビューは表示されません。
テストで送信されるURLのドメイン部分がlocalhost及び127.0.0.1である場合がこれに該当します。

今回、mainブランチでテストを実行してもプレビューが表示されていなかったものの中で考えますと、Docs関連を除く全てがこのパターンに該当します。

  • 定期イベント(localhost
  • 新たなメンバーとしてJOIN(127.0.0.1
  • プラクティス関連(127.0.0.1
  • 質問関連(localhost
  • はじめての日報(localhost

残るDocs関連についてですが、正常に表示される「お知らせ」や「通常の日報」と比較したところ、未ログイン状態で存在しないページへアクセスした場合にトップページへリダイレクトされるパターンと404エラーのページへとばされるパターンの2通りあることがわかりました。
例えば、正常にプレビューが表示される「お知らせ」(例:https://bootcamp.fjord.jp/announcements/575918509 )と「通常の日報」(例:https://bootcamp.fjord.jp/reports/1066585821 )の場合は未ログイン状態で存在しないページへアクセスするとBootcampのトップページ(https://bootcamp.fjord.jp/ ) へリダイレクトされるのに対し、「Docs関連」(例:https://bootcamp.fjord.jp/pages/1070268883 )の場合は404エラーのページが表示されます。
最終的なレスポンスがトップページであればHTMLの中にDiscordがプレビューを生成するのに必要なmetaデータ(headタグ内のmetaタグの情報)が含まれていますが、404ページではそれらのデータがないためプレビューが表示できないということになります。
テストで作成されたページは本番環境には存在しないページですので、Bootcampのユーザーではない(未ログイン状態である)Discordがそれらにアクセスしようとしてもトップページへリダイレクトされるか404ページに飛ばされるかの二択になり、結果的にプレビューが表示されるかどうかという差になってあらわれています。(存在するページの場合は登録orログインを促すページが表示され、metaデータも取得できるため、未ログイン状態であってもプレビューが普通に表示されます。)

404エラーのページ
image

トップページ
image

したがって、mainブランチでテストを実行したとしてもプレビューが表示されないものについては、URLやDiscordのプレビュー機能に問題があるわけではなく、あくまで正しく動作した結果と考えられます。
なお、たまに本番環境でもプレビューが表示されていないものがある点については、時間帯等の関係でBootcampの応答スピードが通常より遅く、metaデータを取得する前にDiscord側がタイムアウトしているのではないかと考えています。(試しに、自動通知が飛んだ当時にプレビューが表示されていなかったものでも、昼間の時間帯に試してみると正常に表示されます。)

調査結果は以上です。私の認識不足により大変失礼いたしました🙇‍♂️
@komagata さんから見て訂正等ありますでしょうか?👀
また、上記を踏まえて改めてレビューの方法を考えると、やはり対象となる通知のコードに対して<>の抜けがないかを見ていただくのが現実的かなと思うのですが、いかがでしょうか?
他に有効な確認手段があればご教示いただければ幸いです🙏

@komagata
Copy link
Member

komagata commented Jul 19, 2024

@ham-cap 調査ありがとうございます。結論から言うと、

「メタデータが取得できない場合にプレビューが表示されない」

ってことですかね。

質問が一つあります。
上記の調査と<>はどういう関係がありますか?<>をつけるかどうかはプレビューの表示に関係ないということでしょうか?

@ham-cap
Copy link
Contributor Author

ham-cap commented Jul 22, 2024

@komagata

「メタデータが取得できない場合にプレビューが表示されない」

ってことですかね。

はい。そのとおりです。

<>をつけるかどうかはプレビューの表示に関係ないということでしょうか?

いいえ。
<>をつけることで意図的にプレビューを非表示にできることは何度も検証したので間違いないと思います。
したがって、Bootcampからの通知でプレビューを表示させたくないのであれば、このPRでやっているように該当箇所に<>をつける変更が必要になります。

上記の調査と<>はどういう関係がありますか?

この調査で「なぜ<>をつけていないURLでもプレビューが表示されない場合があるのか」を確認することで、<>によって余計な副作用なしにプレビューを非表示にできるかどうかを確認し、間接的に今回のPRの変更が妥当であるということを確認できるものと認識しています。

経緯としては、レビュアーの@motohiro-mm さんが動作確認で「<>を付けたことによってプレビューが表示されなくなった」ということを確認するために、まずは「<>を付けていない状態だと全ての通知にプレビューが表示される」という状況を確認しようとしたところ、実際には「<>を付けていない状態でも特定の通知においてはプレビューが表示されなかった」ことをお知らせいただいたので原因を調査した、という流れです。
結果の詳細は、前回共有させていただいたとおりです。

この調査結果を踏まえて考えると、

  • <>をつければプレビューは非表示になる。
  • Discordがリンク先のメタデータを取得できなければプレビューは表示されないが、<>の有効性とは無関係。

と言えます。

また、今回システムテストの際にDiscordがメタデータを取得できず、<>の有無に関わらずプレビューが非表示になってしまうのは、①ローカルでテストを実行するとURLのドメインがローカルになってしまいDiscordがリンク先にアクセスできないこと、②ドメインがbootcamp.fjord.jpの場合であっても未ログイン状態でアクセスした際に404ページが表示されるものについてはメタデータがそもそも提供されていないこと、の二点が原因となりますので、本番環境では基本的に問題なく動作するものと考えます。

なお、以下は上記調査の補足になります。

テストの際にドメインがローカルのURLになってしまうものと、そうでないものが存在する理由

URLの生成方法によるものです。
アプリをローカルで起動している場合、Rails.application.routes.url_helpers.regular_event_url(event)といようにurl_helpersを利用してURLを生成すると、生成されたURLのドメインもローカルになります。そのため、Discordがメタデータを取得できず、プレビューできません。
一方、url_helpersを使用せず、文字列でhttps://bootcamp.fjord.jp/reports/#{report.id}のような形で記載しているものはドメインが本番と同じものですのでプレビューが表示されます。(この場合でも、存在しないページへのリンクになるためプレビューの内容はトップページの情報になります。)
mainブランチでテストを実行した場合でもプレビューが表示される「お知らせ」と「通常の日報」についてはこのパターンです。

@komagata
Copy link
Member

@ham-cap

  • <>をつければプレビューは非表示になる。
  • Discordがリンク先のメタデータを取得できなければプレビューは表示されないが、<>の有効性とは無関係。

なるほどです。

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 4c6f8bc into main Jul 25, 2024
6 checks passed
@komagata komagata deleted the feature/remove-url-previews-for-discord-notification-channels branch July 25, 2024 06:05
@github-actions github-actions bot mentioned this pull request Jul 25, 2024
13 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