Skip to content

Conversation

@komagata
Copy link
Member

@komagata komagata commented Jan 14, 2026

Summary

  • メール送信エラーが発生してもサイト内通知が確実に作成されるように修正

Changes

  • PostAnnouncementJob の実行順序を変更
    • Before: メール通知 → サイト内通知
    • After: サイト内通知 → メール通知

Background

Postmarkで非アクティブな受信者にメール送信しようとするとエラーが発生し、
メール送信とサイト内通知が連動していたため、サイト内通知も作成されない問題があった。

Test plan

  • お知らせを作成し、サイト内通知が作成されることを確認
  • メール送信エラーが発生してもサイト内通知は作成されることを確認

Summary by CodeRabbit

リリースノート

  • Bug Fixes
    • お知らせ通知の配信順序を改善しました。サイト内通知がメール通知より前に配信されるようになります。

単語数: 23語

✏️ Tip: You can customize this high-level summary in your review settings.

メール送信エラー(Postmark::InactiveRecipientError等)が
発生してもサイト内通知が確実に作成されるように、
実行順序をサイト内通知→メール通知に変更。
@coderabbitai
Copy link

coderabbitai bot commented Jan 14, 2026

📝 Walkthrough

Walkthrough(概要)

post_announcement_job.rbのperform メソッド内で、受信者に対する通知の送信順序が変更されました。send_mail_notificationsend_on_site_notification の後に実行されるようになりましたが、全体的なアクション内容に変わりはありません。

Changes(変更内容)

Cohort / File(s) 説明
通知順序の変更
app/jobs/post_announcement_job.rb
perform メソッド内で、メール通知とサイト内通知の送信順序を入れ替え。メール通知がサイト内通知の後に実行されるように変更

Estimated code review effort(コードレビュー見積もり)

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem(詩)

🐰 通知の順は変われど、
 両方きっと届くから
 心配なしで進もうぞ
  ~ウサギより

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive 説明にはSummary、Changes、Background、Test planが含まれているが、リポジトリのテンプレートで要求されているIssue番号、変更確認方法(ステップ詳細)、Screenshotセクションが不足している。 Issue番号を追加し、変更確認方法の具体的なステップを記述し、必要に応じてスクリーンショットを追加してテンプレート形式に合わせることを推奨します。
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed タイトルは変更内容を正確に反映しており、PostAnnouncementJobの実行順序をサイト内通知優先に変更したという主要な変更を明確に要約している。

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5a79f4f and d63bd16.

📒 Files selected for processing (1)
  • app/jobs/post_announcement_job.rb
🧰 Additional context used
📓 Path-based instructions (2)
app/**/*.rb

📄 CodeRabbit inference engine (AGENTS.md)

app/**/*.rb: Place Rails app code in app/ directory with subdirectories for models/, controllers/, views/, jobs/, helpers/, and frontend code under javascript/
Follow Rails file naming conventions (e.g., app/models/user.rb)

Files:

  • app/jobs/post_announcement_job.rb
**/*.rb

📄 CodeRabbit inference engine (AGENTS.md)

Use 2-space indent for Ruby code, snake_case for method names, and CamelCase for class names as enforced by RuboCop

Files:

  • app/jobs/post_announcement_job.rb

⚙️ CodeRabbit configuration file

**/*.rb: # refactoring

  • まずFat Controllerを避け、次にFat Modelを避ける。
  • Serviceクラスの乱用を避ける。
  • controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。

Rails Patterns

  • ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。(active_decorator gemを導入しているのでそれを使う)
  • 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。(rails-patterns gemを導入しているのでそれのQuery機能を使う)
  • Viewにpartialを作る場合はViewComponentを使うことを検討する。
  • 複数のActiveRecordモデルを操作する1つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。(interactor gemを導入しているのでそれを使う)
  • 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。(interactor gemを導入しており、その中にOrganizerの機能があるので使う)

Files:

  • app/jobs/post_announcement_job.rb
🧠 Learnings (1)
📓 Common learnings
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T12:22:33.168Z
Learning: CheckCallbacks内のメール通知・サイト内通知で例外が発生した場合、例外を握りつぶさず上位に伝搬させる。これにより通知失敗を検知し、運用上の混乱を防ぐ。CheckCallbacksはafter_create_commitで実行されるため、例外発生時にはCheckとステータス更新はすでにコミット済みであり、ユーザーにはエラーメッセージが表示されるが、DBには変更が残る状態になる。
Learnt from: matuaya
Repo: fjordllc/bootcamp PR: 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: komagata
Repo: fjordllc/bootcamp PR: 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のスコープ維持が重要。
Learnt from: reckyy
Repo: fjordllc/bootcamp PR: 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: matuaya
Repo: fjordllc/bootcamp PR: 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: jun-kondo
Repo: fjordllc/bootcamp PR: 0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/inquiry_notifier.rb`において、`NOTIFICATION_SENDER_NAME`で指定される管理者ユーザーは必ず存在することがシステムの前提条件として保証されているため、senderがnilになることは想定されておらず、エラーハンドリングは不要である。
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build_and_test
🔇 Additional comments (2)
app/jobs/post_announcement_job.rb (2)

6-11: LGTM! 順序変更によりサイト内通知の確実な作成が保証されます。

メール送信(send_mail_notification)でエラーが発生しても、サイト内通知(send_on_site_notification)は既に作成済みとなるため、PR目的が達成されています。


23-34: send_on_site_notification の例外ハンドリングについて確認

現在の実装では Notification.create! が失敗した場合、例外を握りつぶしてログ出力のみ行い、その後 send_mail_notification が実行されます。これにより「サイト内通知なし・メールあり」の状態が発生する可能性があります。

バックグラウンドジョブで複数receiverを処理する設計上、1件の失敗で全体を止めないのは妥当ですが、この挙動が意図通りか確認をお願いします。

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@komagata komagata merged commit d7318f9 into main Jan 14, 2026
4 checks passed
@komagata komagata deleted the fix/announcement-notification-order branch January 14, 2026 09:56
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.

2 participants