Skip to content

ブログ記事公開時に通知先のターゲットを選択できるようにした#8346

Merged
komagata merged 22 commits intomainfrom
feature/set-article-notification-target
Mar 28, 2025
Merged

ブログ記事公開時に通知先のターゲットを選択できるようにした#8346
komagata merged 22 commits intomainfrom
feature/set-article-notification-target

Conversation

@ayu-0505
Copy link
Copy Markdown
Contributor

@ayu-0505 ayu-0505 commented Feb 17, 2025 •
edited
Loading

Issue

概要

ブログ記事を公開する際に通知先を選択できるようにターゲットをラジオボタンで選択できるように変更しました。
選択肢は「全員（退会者を除く）」、「現役生のみ」、「就職希望者のみ」、「通知しない」の４つで、「通知しない」を選択した場合はメンター・管理者含めどこにも通知が飛ばないようになっています。
通知は初回公開時のみに行われるため、「公開するボタン」をクリックすると、「通知を行う旨と通知先」をお知らせするダイアログが表示されます。
通知送付後、一度公開した記事の編集画面では通知をすでに行ったというメッセージが表示され、ターゲット選択のラジオボタンは非表示となっています。

変更確認方法

  1. {feature/set-article-notification-target}をローカルに取り込む
    1. git fetch origin pull/8346/head:feature/set-article-notification-target
    2. git switch feature/set-article-notification-target
  2. bin/rails db:resetを実行（migrateする＆seedデータを入れるため）
  3. foreman start -f Procfile.dev でサーバーを起動する
  4. komagataでログインする。（pass：testtest）

編集画面の確認

  1. ブログ記事新規作成画面に行き、通知ターゲット欄が表示されているか確認する。
  2. 公開するボタンを押すと警告ダイアログが出るかを確認する。（例：この記事を公開すると、全員（退会者を除く）に通知が送られます。もし記事に誤った情報が含まれている場合、その通知を受け取った人を介してSNSなどで拡散される可能性があります。そのため、記事内の年月日などに間違いがないか、しっかり確認してください。公開しても問題ありませんか？）
    1. 一度キャンセルし、ターゲットを切り替えて公開ボタンを押すと、警告ダイアログの通知先が変更に合わせて変わっていることを確認する。 （全員（退会者を除く）、現役生のみ、就職希望者のみ）
    2. 通知しないの場合の警告ダイアログは「通知しない」が選択されています。よろしいですか？となっているか確認する。
  3. 任意の通知先で記事を作成し公開した後、その記事の編集画面に行くとこの記事は初回公開時にxxxxのみに対してすでに通知を行っています。といった通知をすでに行った旨のメッセージが表示され、選択ボタンは表示されなくなっているかを確認する。
    （通知先は先ほど選んだ通知先となっています。）
    • 通知しないを選択した場合はこの記事は通知を行わずに公開しました。というメッセージとなっている。

通知先の確認

  1. ブログ記事新規作成画面に行き、それぞれのターゲット（通知なしも含む）のブログ記事を作成する。
    1. わかりやすいようにタイトル部分にターゲット先の名称を入れる。（例： 全員、現役生、就職希望者、通知なし） 本文部分は適当に入力する。
    2. タイトル部分と同じターゲットをラジオボタンで選択する。
  2. 公開するボタンをクリックしてOKし、記事を公開する。
  3. komagataのままトップページに飛び、記事の作成者には通知が飛んでいないことを確認する。
  4. 以下のユーザーにそれぞれログインし（pass：testtest）、通知の有無を確認する。（「通知なし」の記事はいずれのユーザーにも飛ばない）
    • kimura（現役生）：全員、現役生に該当。
    • jobseeker（就職希望者）：全員、現役生、就職希望者に該当。
    • kensyu（研修生）：全員に該当。
    • mentormentaro（メンター）：全員、現役生、就職希望者に該当。（メンターはどの通知先でも届く。）
    • machida（管理者）：全員、現役生、就職希望者に該当。（管理者はどの通知先でも届く。）
    • sotugyou-with-job：（卒業生）：全員に該当。

DBを元に戻す

このブランチでbin/rails db:resetを実行した後、mainブランチへ戻り他の作業をする（元のデータベースに戻す）には以下を実行してください。
（migrateの履歴等を元に戻すためです）

bin/rails db:rollbackを実行
schema.rbが変更されてしまうので、それを戻す(git restore db/schema.rb)
git checkout mainでmainブランチへ
bin/rails db:resetを実行

Screenshot

変更前

  • ターゲットの選択肢は表示されません。
スクリーンショット 2025-03-13 12 09 39
  • 公開後の編集画面にも変化はありません。
スクリーンショット 2025-03-13 12 09 57

変更後

  • 新規作成時、またはWIP保存の記事には以下のような表示があります。
スクリーンショット 2025-03-13 12 03 45
  • 公開するボタンを押すと、選択したボタンに応じてメッセージが表示されます。
スクリーンショット 2025-03-13 12 04 02
  • 一度公開するボタンを押すと以後の編集画面では選択肢がなくなり、公開済みという表示が出ます。
スクリーンショット 2025-03-19 13 34 54

@ayu-0505 ayu-0505 changed the title Feature/set article notification target ブログ記事公開時に通知先のターゲットを選択できるようにした Feb 17, 2025
@ayu-0505 ayu-0505 self-assigned this Feb 17, 2025
@ayu-0505 ayu-0505 force-pushed the feature/set-article-notification-target branch 3 times, most recently from 69b282e to b3cbcaa Compare February 20, 2025 00:39
@ayu-0505
Copy link
Copy Markdown
Contributor Author

ayu-0505 commented Feb 20, 2025

@machida

お疲れ様です🍵
こちらのPRに関して、以下の部分にデザインを入れていただきたいと思っています。

ターゲット先選択ラジオボタン

アナウンスからそのまま引っ張ってきたので、４つ目の「通知しない」で改行されてしまいます。
スクリーンショット 2025-02-20 12 34 14

通知送付後の編集画面の文言

pタグそのままなので、必要であればお願いします。
スクリーンショット 2025-02-20 12 35 09

またお手隙のときによろしくお願いいたします🙏

@machida
Copy link
Copy Markdown
Member

machida commented Feb 20, 2025

@ayu-0505 連絡ありがとうございます！デザイン了解ですー

@machida machida self-assigned this Feb 20, 2025
@machida
Copy link
Copy Markdown
Member

machida commented Mar 10, 2025

@ayu-0505
遅くなってごめんなさい🙇
デザインを入れました！！
ご確認おねがいします🙏

@ayu-0505
Copy link
Copy Markdown
Contributor Author

ayu-0505 commented Mar 13, 2025

@SuzukiShuntarou
お疲れ様です🍵

こちらのPRについて、レビューをお願いできますでしょうか。
急ぎではありませんが、お忙しかったり、ご都合が悪かったりする場合は遠慮なくお知らせください。
ご確認のほどよろしくお願いします🙇🏻‍♀️

@ayu-0505
Copy link
Copy Markdown
Contributor Author

ayu-0505 commented Mar 13, 2025

@machida
お疲れ様です🍵
デザインありがとうございました🙏

@ayu-0505 ayu-0505 marked this pull request as ready for review March 13, 2025 07:43
@SuzukiShuntarou
Copy link
Copy Markdown
Contributor

SuzukiShuntarou commented Mar 14, 2025

@ayu-0505
お疲れ様です！ご連絡遅くなり申し訳ありません。
１週間程度頂ければレビューできるかと思いますが、大丈夫でしょうか？

@ayu-0505
Copy link
Copy Markdown
Contributor Author

ayu-0505 commented Mar 14, 2025

@SuzukiShuntarou
お疲れさまです🍵
急いでおりませんので問題ありません〜！
よろしくお願いいたします🙇🏻‍♀️

@SuzukiShuntarou SuzukiShuntarou mentioned this pull request Mar 16, 2025
Open
@SuzukiShuntarou
Copy link
Copy Markdown
Contributor

SuzukiShuntarou commented Mar 16, 2025 •
edited
Loading

@ayu-0505
お疲れ様です！恐れ入りますが、変更確認方法についてご確認させてください。もし私の環境によるエラーでしたら申し訳ございません。🙇‍♀️

確認事項

  • 記載された内容でローカル環境を立ち上げようとしたところエラーになり、立ち上がらない状況です。

実施した内容

  1. mainブランチでgit pull origin mainを実施
  2. git fetch origin pull/8346/head:feature/set-article-notification-targetを実施
  3. git switch feature/set-article-notification-targetを実施（変更確認手順は g switchとなっていましたが誤字でしょうか？）
  4. foreman start -f Procfile.devを実施したがエラー。
    image
  • エラー抜粋
    • stringioのバージョンエラーみたいですが、こちら私の環境の問題でしょうか？
    • mainブランチに切り替えforeman start -f Procfile.devを実施したところ、問題なく立ち上がりました。
/home/suzuki/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/bundler-2.4.21/lib/bundler/runtime.rb:304:in `check_for_activated_spec!': You have already activated stringio 3.1.5, but your Gemfile requires stringio 3.1.0. Prepending `bundle exec` to your command may solve this. (Gem::LoadError)
  • Gemfileをmainと比べて見たところ以下の写真のような違いがありましたのでご報告いたします。
    • 作業ブランチ
      image
    • mainブランチ
      image
  1. 念のため作業ブランチでbin/setupも実施した後にforeman start -f Procfile.devを実施したのですが、エラーとなってしまいました。
    image

@ayu-0505 ayu-0505 force-pushed the feature/set-article-notification-target branch from 9e80aca to 8c991eb Compare March 17, 2025 00:56
@ayu-0505
Copy link
Copy Markdown
Contributor Author

ayu-0505 commented Mar 17, 2025

@SuzukiShuntarou
お疲れ様です🍵
お手間をおかけしているようで大変申し訳ありません🙏💦

エラーで立ち上がらない件について

おそらくご推測のとおりstringioのバージョン違いだと思われます🙏
また、g switchはこちらのエイリアス設定がそのまま残ってしまいました。

町田さんのデザイン終了時点では立ち上がったので、問題ないと判断してしまいました。
対応として、3/17の10時時点でmainを最新まで更新し、rebaseを行うことで、GemfileおよびGemfile.lockの差分解消を行いました。
そしてその状態のブランチを再度pushいたしました。
スクリーンショット 2025-03-17 10 46 30

この対応にて、問題が解消したかどうか、お手数ですが再度ご確認願えますでしょうか🙇🏻‍♀️

@SuzukiShuntarou
Copy link
Copy Markdown
Contributor

SuzukiShuntarou commented Mar 17, 2025

@ayu-0505
お疲れ様です！ご対応ありがとうございまます。😊
ローカル環境で無事に立ち上がりましたので、取り急ぎご報告いたします。
レビューはこれから実施いたしますので、お時間いただければ幸いです。１点だけご連絡いたします。

  • bin/rails db:migrateを実施する必要がありましたのでご連絡いたします。
    2

  • bin/rails db:migrate:statusの実行結果でdownとなっておりました。
    image

  • bin/rails db:migrateを実施後
    image

SuzukiShuntarou
SuzukiShuntarou reviewed Mar 17, 2025
View reviewed changes
app/models/article.rb Outdated
return unless errors.any?

self.published_at = nil
end
Copy link
Copy Markdown
Contributor

@SuzukiShuntarou SuzukiShuntarou Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

申し訳ございません。🙇‍♀️
こちらのメソッドをafter_validationを実施している理由についてご教示いただけないでしょうか？対応するコミットを見てみたのですが、どういう状況で発生する問題（本来の初回投稿時の時間と齟齬が出る問題）なのかがわかりませんでした。お手数をおかけしますがよろしくお願いいたします。

また、関連して以下のような動作となるのですが、こちらは正しいのでしょうか？（実際にこういう動作があるかの判断も難しいため、直す必要があるかはご確認いただければ幸いです。）

  • 公開済のブログ記事の更新時にバリデーションエラーが発生した場合、通知ターゲットが再度表示される。
    • app/views/articles/_form.html.slimの142行目でif article.before_initial_publish? としているため、after_validationでnil更新されるとbefore_initial_publish?がtrueとなり、通知ターゲットが再度表示されるのだと思われます。
development._.FJORD.BOOT.CAMP.-.Google.Chrome.2025-03-17.15-32-28.mp4

Copy link
Copy Markdown
Contributor Author

@ayu-0505 ayu-0505 Mar 18, 2025

Choose a reason for hiding this comment

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

ありがとうございます！
このメソッドは結局変更してしまったのですが、お答えします〜。

まさしく、動画であげていただいたような「バリデーションエラーで編集画面に戻ったのにbefore_validationによってDB保存前にpublished_atが入ってしまう状態」を、初回投稿前に防ぐ意図として作成していました。
上記のような動作でpublished_atが入り、仮に時間を開けてから初回投稿を行うと、「実際の初回投稿時間とバリデーションエラー時にすでに入ったpublished_atとの間で齟齬が出てしまう」、ということで本来の初回投稿時の時間と齟齬が出る問題 とコミットメッセージを残しました。
published_atは後から変更できるので、published_atにとっては時間がずれることはそこまで問題ではないのですが、published_atを表示の条件としている通知ターゲットにとっては致命的だったのでafter_validationで元に戻すようにしていました。
（初回投稿前なのに通知ターゲットが選択不可になってしまう）

しかし、Suzukiさんのご指摘の通り、投稿済みの編集画面でもafter_validationが発動してしまい、通知ターゲットが意図せず表示されるようになってしまったので修正いたしました。

test/models/article_test.rb
assert_not articles(:article1).before_initial_publish?
assert articles(:article3).before_initial_publish?
end

Copy link
Copy Markdown
Contributor

@SuzukiShuntarou SuzukiShuntarou Mar 17, 2025

Choose a reason for hiding this comment

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

今回追加したプライベートメソッドのreset_published_at_if_invalidのテストが追加されていないように見受けられました。プライベートメソッドなので間接的にテストをする必要があるかと思ったのですが、いかがでしょうか？

Copy link
Copy Markdown
Contributor Author

@ayu-0505 ayu-0505 Mar 18, 2025

Choose a reason for hiding this comment

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

ありがとうございます、ご指摘の通りだと思います〜。

「プライベートメソッドは直接的なテストを作らず、パブリックメソッド経由で間接的にテストする」というのがプライベートメソッドにおける基本的な考え方だと思われますが、前半部の「プライベートメソッドは直接的なテストを作らない」だけが意識に残ってしまい、失念しておりました🙇🏻‍♀️

変更の過程でプライベートメソッドは削除してしまったのですが、改めて復習になりました🙏

@SuzukiShuntarou
Copy link
Copy Markdown
Contributor

SuzukiShuntarou commented Mar 17, 2025

@ayu-0505
お疲れ様です！2点コメントをさせていただきました。私も分からない部分があったため、的外れなコメントをしていたら申し訳ございません。🙇‍♀️
ご確認の上ご連絡いただければ幸いです。問題なければApproveに変更させていただきます！

@ayu-0505 ayu-0505 force-pushed the feature/set-article-notification-target branch from 4633238 to f211ac6 Compare March 18, 2025 05:20
ayu-0505
ayu-0505 commented Mar 18, 2025
View reviewed changes
app/models/article.rb
end

def before_initial_publish?
attribute_in_database(:published_at).nil?
Copy link
Copy Markdown
Contributor Author

@ayu-0505 ayu-0505 Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

published_atがnilであるか（ ≒ 初回投稿前であるか）について、オブジェクト自体のpublished_atではなく、DB上のpublished_atを判別するようにいたしました。
このように変更することで、「確実に初回投稿前であるタイミング」にのみ通知先ターゲットが表示されるようにいたしました。

Copy link
Copy Markdown
Contributor

@SuzukiShuntarou SuzukiShuntarou Mar 19, 2025

Choose a reason for hiding this comment

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

ありがとうございます！attribute_in_databaseは知らなかったので勉強になります！
動作も問題なさそうでした。

@ayu-0505
Copy link
Copy Markdown
Contributor Author

ayu-0505 commented Mar 18, 2025

@SuzukiShuntarou
お疲れ様です🍵
レビューありがとうございました🙏

ご連絡いただいた部分について追加で変更を行いましたので、再度確認のほどよろしくお願いします🙏

db:migrateについて

以下のコメントにあったdb:migrateについて、連絡ありがとうございました。
#8346 (comment)

今回の変更においてはカラムの追加があったので、正常な動きです。
しかし、トップコメントに書いていなかったのは私のミスです。申し訳ありません💦
念の為トップレベルに追記しておきました。
また、DBがマイグレーションされた状態になっていると思いますので、bin/rails db:resetとfeature/set-article-notification-targetにおけるbin/rails db:rollbackをお手数ですがお願い致します。
後手の連絡になってお手数をおかけいたします🙇🏻‍♀️

@ayu-0505
Copy link
Copy Markdown
Contributor Author

ayu-0505 commented Mar 19, 2025

すみません、追記です🙇🏻‍♀️

こちらのコミットについて。
レビュー内容とは関係がないところなのですが、ターゲット選択カラムにデータがない過去の記事において、表示がおかしくなる不具合を発見いたしました。
そのため、投稿済み時の文言を変更しております。
スクリーンショット 2025-03-18 10 53 02

SuzukiShuntarou
SuzukiShuntarou approved these changes Mar 19, 2025
View reviewed changes
app/models/article.rb
end

def before_initial_publish?
attribute_in_database(:published_at).nil?
Copy link
Copy Markdown
Contributor

@SuzukiShuntarou SuzukiShuntarou Mar 19, 2025

Choose a reason for hiding this comment

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

ありがとうございます！attribute_in_databaseは知らなかったので勉強になります！
動作も問題なさそうでした。

@SuzukiShuntarou
Copy link
Copy Markdown
Contributor

SuzukiShuntarou commented Mar 19, 2025 •
edited
Loading

@ayu-0505
お疲れ様です！修正内容確認いたしました。問題なさそうでしたのでApproveに変更いたしました！

レビュー内容とは関係がないところなのですが、ターゲット選択カラムにデータがない過去の記事において、表示がおかしくなる不具合を発見いたしました。
そのため、投稿済み時の文言を変更しております。

  • こちらも確認いたしました。画面上での変更も確認できました。Screenshotの変更後の画像が変更前の画像であったので修正していただければ幸いです！ 以下の文言の後の画像です。

一度公開するボタンを押すと以後の編集画面では選択肢がなくなり、公開済みという表示が出ます。

@ayu-0505
Copy link
Copy Markdown
Contributor Author

ayu-0505 commented Mar 19, 2025 •
edited
Loading

@SuzukiShuntarou
お疲れ様です🍵
丁寧にレビューいただき本当にありがとうございました！

@komagata
お疲れ様です🍵
メンバーレビューが終わりましたので、お手隙の時に確認をお願いいたします🙏

ayu-0505 and others added 21 commits March 26, 2025 16:32
@ayu-0505
viewで選択画面を追加、コントローラーでパラメーターが取得できるように変更
42971e3
@ayu-0505
通知ターゲットにて通知しない以外を選択した場合はダイアログが出るように変更
d3a4d2a
@ayu-0505
通知しない選択時に通知が飛ばないように受信者の設定を変更、合わせて受信者設定メソッドをより汎用性がある名前に変更
1d73b59
@ayu-0505
通知しないに対応する変数名を変更
b8a0ed6
@ayu-0505
警告文をsubmitボタン押下時に表示されるように変更
a07ead5
@ayu-0505
公開するボタンの時のみ警告文が表示され、WIPでは表示されないように変更
73b6bd7
@ayu-0505
article.jsをリファクタリングした（関数化してDRYにする、早期リターンの条件対象を変更）
a9f637d
@ayu-0505
一度通知したブログ記事においてはターゲット表示が出ないように修正
54cc7cd
@ayu-0505
通知ターゲットは初回公開時のみ飛ぶことを新規作成フォームにて明記、公開したものは通知済みであることを編集フォームに追記
4383b02
@ayu-0505
articleのenum型のtargetにi18nの翻訳を追加、編集時の既通知連絡の文言に通知を行った対象を追加
5182b4a
@ayu-0505
文字位置や適応タグの調整
ac8f681
@ayu-0505
通知が届くようにコードを変更、テストを作成
916e792
@ayu-0505
lintにおける指摘事項を修正
336075a
@ayu-0505
articleのシステムテスト修正、テストで発見された不具合の修正
4c998b9
@ayu-0505
テストにおける使用メソッド名を変更に合わせて修正、ダイアログの影響でFlakyになっていた箇所を修正
f7a6a5a
@ayu-0505
ファイル名を実態に沿うように変更
dd64d59
@ayu-0505
バリデーションエラー時にpublished_atが入ってしまい、本来の初回投稿時の時間と齟齬が出る問題を修正
bc92914
@machida @ayu-0505
ブログ通知ターゲットのデザインを整えた
822156b
@ayu-0505
公開日時付与のタイミングが初回公開時になるように、published_atのnil条件をデータベース上で確認するように変更
b03dcad
@ayu-0505
通知ターゲット追加前の過去記事にて表記がおかしくなるため、文言を変更
4a131b1
@ayu-0505
slimファイルの記述を整理した
aa07169
@ayu-0505 ayu-0505 force-pushed the feature/set-article-notification-target branch from f211ac6 to aa07169 Compare March 26, 2025 07:36
@ayu-0505
Copy link
Copy Markdown
Contributor Author

ayu-0505 commented Mar 27, 2025

@komagata
お疲れ様です🍵
レビューいただいていた、slimファイルの記述について整えましたので、
お手隙の際に再度ご確認をお願いできたらと思います🙏

komagata
komagata approved these changes Mar 28, 2025
View reviewed changes
Copy link
Copy Markdown
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 71bbb26 into main Mar 28, 2025
2 checks passed
@komagata komagata deleted the feature/set-article-notification-target branch March 28, 2025 07:35
@github-actions github-actions bot mentioned this pull request Mar 28, 2025
Merged
15 tasks
MikotoMakizuru pushed a commit that referenced this pull request Mar 29, 2025
@komagata @MikotoMakizuru
Merge pull request #8346 from fjordllc/feature/set-article-notificati…
2b3238d 
…on-target

ブログ記事公開時に通知先のターゲットを選択できるようにした
This was referenced Apr 1, 2025
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@komagata komagata komagata approved these changes

@SuzukiShuntarou SuzukiShuntarou SuzukiShuntarou approved these changes

Assignees

@ayu-0505 ayu-0505

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ayu-0505 @machida @SuzukiShuntarou @komagata