Skip to content

CorrectAnswerNotifier等3クラスをnewspaperからActiveSupport::Notificationsへ移行 #9015

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.

Sign up for GitHub

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

Merged
komagata merged 3 commits into from
Oct 1, 2025
Merged

CorrectAnswerNotifier等3クラスをnewspaperからActiveSupport::Notificationsへ移行 #9015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7fd807c
CorrectAnswerNotifierをnewspaperからActiveSupport::Notificationsへ移行
ryufuta Jul 21, 2025
ab4ac58
GraduationNotifierをnewspaperからActiveSupport::Notificationsへ移行
ryufuta Aug 5, 2025
e206393
ComebackNotifierをnewspaperからActiveSupport::Notificationsへ移行
ryufuta Aug 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/api/correct_answers_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def create
@answer.type = 'CorrectAnswer'
if @answer.save
Newspaper.publish(:answer_save, { answer: @answer })
Newspaper.publish(:correct_answer_save, { answer: @answer })
ActiveSupport::Notifications.instrument('correct_answer.save', answer: @answer)
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Sep 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

二重イベント発火の可能性（Newspaper + Notifications）

Newspaper.publish(:answer_save, …)とinstrument('correct_answer.save', …)の併用は意図的ですか？移行完了後は旧経路を外すほうが安全です。少なくとも重複通知が発生していないか確認をお願いします。

削除する場合の最小差分:

-      Newspaper.publish(:answer_save, { answer: @answer })
       ActiveSupport::Notifications.instrument('correct_answer.save', answer: @answer)

確認用:

🏁 Script executed:

#!/bin/bash
rg -n "Newspaper\.subscribe\([^)]*answer_save" -C2

Length of output: 390

Newspaper.subscribe(:answer_save) が残っています — publish の削除は危険です（要修正）

config/initializers/newspaper.rb:5 に Newspaper.subscribe(:answer_save, answer_cache_destroyer) が存在します。コントローラから Newspaper.publish(:answer_save, ...) を削除すると AnswerCacheDestroyer（キャッシュ破棄等）が呼ばれなくなります。
対応案（いずれかを選択）:

  • Newspaper.publish を維持する（既存サブスクライバに依存）。
  • AnswerCacheDestroyer を ActiveSupport::Notifications ('correct_answer.save' 等) に移行してから config/initializers/newspaper.rb の subscribe を削除／更新。
🤖 Prompt for AI Agents 
In app/controllers/api/correct_answers_controller.rb around line 11, removing
Newspaper.publish(:answer_save, ...) breaks the existing
Newspaper.subscribe(:answer_save, answer_cache_destroyer) defined in
config/initializers/newspaper.rb so AnswerCacheDestroyer no longer runs; either
restore the Newspaper.publish(:answer_save, answer: @answer) call here to keep
current subscribers working, or migrate the AnswerCacheDestroyer to listen to
ActiveSupport::Notifications by subscribing to the 'correct_answer.save' event
(update AnswerCacheDestroyer to accept the AS::Notifications payload and change
the initializer to remove or update the Newspaper.subscribe accordingly); ensure
the chosen approach preserves the event payload shape and side effects (cache
invalidation) and update tests if needed.

Copy link
Copy Markdown
Contributor Author

@ryufuta ryufuta Sep 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@coderabbitai
削除したのはNewspaper.publish(:correct_answer_save, { answer: @answer })です。
Newspaper.publish(:answer_save, { answer: @answer })は削除していないので問題ありません。

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Sep 14, 2025

Choose a reason for hiding this comment

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

@ryufuta その通りですね。申し訳ありませんでした。

diffを再確認すると、削除されているのは確かに :correct_answer_save イベントのみで、:answer_save イベントは残されていることが分かります。つまり既存の AnswerCacheDestroyer は引き続き正常に動作し、キャッシュ無効化処理も維持されます。

私の前回の分析が間違っていました。この実装は問題ありません。

🧠 Learnings used 
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。

Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。

Learnt from: reckyy
PR: fjordllc/bootcamp#8356
File: app/models/notification.rb:45-47
Timestamp: 2025-07-15T12:58:02.735Z
Learning: fjordllc/bootcampプロジェクトの`app/models/notification.rb`において、`training_completed`通知種別は`TARGETS_TO_KINDS`ハッシュに含めない設計になっている。これは`training_completed`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。

Learnt from: Miya096jp
PR: fjordllc/bootcamp#9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。

Learnt from: jun-kondo
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/inquiry_notifier.rb`において、`NOTIFICATION_SENDER_NAME`で指定される管理者ユーザーは必ず存在することがシステムの前提条件として保証されているため、senderがnilになることは想定されておらず、エラーハンドリングは不要である。

Learnt from: komagata
PR: fjordllc/bootcamp#9101
File: app/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。

head :ok
else
head :bad_request
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/comeback_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def create
if @user
if @user&.hibernated?
@user.comeback!
Newspaper.publish(:comeback_update, { user: @user })
ActiveSupport::Notifications.instrument('comeback.update', user: @user)
@user.create_comebacked_comment
redirect_to root_url, notice: '休会から復帰しました。'
else
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/graduation_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class GraduationController < ApplicationController
def update
if @user.update(graduated_on: Date.current)
Subscription.new.destroy(@user.subscription_id) if @user.subscription_id
Newspaper.publish(:graduation_update, { user: @user })
ActiveSupport::Notifications.instrument('graduation.update', user: @user)
redirect_to @redirect_url, notice: 'ユーザー情報を更新しました。'
else
redirect_to @redirect_url, alert: 'ユーザー情報の更新に失敗しました'
Expand Down
2 changes: 1 addition & 1 deletion app/models/comeback_notifier.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

class ComebackNotifier
def call(payload)
def call(_name, _started, _finished, _unique_id, payload)
user = payload[:user]
User.admins_and_mentors.each do |admin_or_mentor|
ActivityDelivery.with(sender: user, receiver: admin_or_mentor).notify(:comebacked)
Expand Down
2 changes: 1 addition & 1 deletion app/models/correct_answer_notifier.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

class CorrectAnswerNotifier
def call(payload)
def call(_name, _started, _finished, _unique_id, payload)
answer = payload[:answer]
notify_correct_answer(answer)
notify_to_chat(answer)
Expand Down
2 changes: 1 addition & 1 deletion app/models/graduation_notifier.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

class GraduationNotifier
def call(payload)
def call(_name, _started, _finished, _unique_id, payload)
user = payload[:user]
User.mentor.each do |mentor|
ActivityDelivery.with(sender: user, receiver: mentor).notify(:graduated)
Expand Down
2 changes: 1 addition & 1 deletion app/models/question_auto_closer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ def select_as_best_answer(close_answer)

def publish_events(correct_answer)
Newspaper.publish(:answer_save, { answer: correct_answer })
Newspaper.publish(:correct_answer_save, { answer: correct_answer })
ActiveSupport::Notifications.instrument('correct_answer.save', answer: correct_answer)
end
end
end
5 changes: 4 additions & 1 deletion config/initializers/active_support_notifications.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
ActiveSupport::Notifications.subscribe('answer.create', AnswererWatcher.new)
ActiveSupport::Notifications.subscribe('answer.create', AnswerNotifier.new)
ActiveSupport::Notifications.subscribe('answer.create', NotifierToWatchingUser.new)
ActiveSupport::Notifications.subscribe('correct_answer.save', CorrectAnswerNotifier.new)
ActiveSupport::Notifications.subscribe('event.create', EventOrganizerWatcher.new)
ActiveSupport::Notifications.subscribe('regular_event.create', RegularEventOrganizerWatcher.new)
sad_streak_updater = SadStreakUpdater.new
Expand All @@ -30,7 +31,9 @@
ActiveSupport::Notifications.subscribe('product.update', ProductUpdateNotifierForWatcher.new)
ActiveSupport::Notifications.subscribe('product.update', ProductUpdateNotifierForChecker.new)
ActiveSupport::Notifications.subscribe('came.comment', CommentNotifier.new)

ActiveSupport::Notifications.subscribe('graduation.update', GraduationNotifier.new)
ActiveSupport::Notifications.subscribe('comeback.update', ComebackNotifier.new)

learning_status_updater = LearningStatusUpdater.new
ActiveSupport::Notifications.subscribe('product.save', learning_status_updater)
ActiveSupport::Notifications.subscribe('check.create', learning_status_updater)
Expand Down
5 changes: 0 additions & 5 deletions config/initializers/newspaper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,6 @@
answer_cache_destroyer = AnswerCacheDestroyer.new
Newspaper.subscribe(:answer_save, answer_cache_destroyer)
Newspaper.subscribe(:answer_destroy, answer_cache_destroyer)
Newspaper.subscribe(:correct_answer_save, CorrectAnswerNotifier.new)

Newspaper.subscribe(:graduation_update, GraduationNotifier.new)

Newspaper.subscribe(:comeback_update, ComebackNotifier.new)

unfinished_data_destroyer = UnfinishedDataDestroyer.new
Newspaper.subscribe(:retirement_create, unfinished_data_destroyer)
Expand Down