Conversation
Walkthrough複数箇所に散在していたユーザー退会/卒業の副作用処理を、新設した Strategy パターンの Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant Retirement
participant Strategy as Strategy\n(Self/Auto/Admin)
participant User
participant External as External\nServices
Controller->>Retirement: by_self/auto/by_admin(user, params?).execute()
rect rgb(230,240,255)
Note over Retirement,Strategy: 割当フェーズ
Retirement->>Strategy: assign_reason(user)
Retirement->>Strategy: assign_date(user)
Retirement->>User: set retire_reason / retired_on
end
rect rgb(220,255,230)
Note over Retirement,User: 永続化とクリーンアップ
Retirement->>Strategy: save_user(user)
Strategy->>User: save!(validate: false) / save(context)
Retirement->>User: destroy subscription (if any), clear github, destroy cards, cancel event subscriptions
end
rect rgb(255,245,230)
Note over Retirement,External: 通知・公開
Retirement->>External: publish Newspaper
Retirement->>Strategy: notification_type()
Retirement->>External: notify user / admins / mentors (strategy に依存)
end
Retirement-->>Controller: true / false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分 Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (2)**/*.rb⚙️ CodeRabbit configuration file
Files:
test/**/*⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-09-01T22:31:57.345ZApplied to files:
📚 Learning: 2025-09-01T22:31:57.345ZApplied to files:
🧬 Code graph analysis (6)app/models/retirement/self.rb (3)
app/models/retirement/auto.rb (3)
test/models/retirement_test.rb (4)
app/controllers/admin/users_controller.rb (1)
app/models/retirement.rb (4)
app/controllers/scheduler/daily/auto_retire_controller.rb (1)
⏰ 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)
🔇 Additional comments (15)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.81.1)test/models/retirement_test.rbrubocop-minitest extension supports plugin, specify ... [truncated 255 characters] ... fig_loader_resolver.rb:76:in app/controllers/admin/users_controller.rbrubocop-minitest extension supports plugin, specify ... [truncated 255 characters] ... fig_loader_resolver.rb:76:in app/controllers/retirement_controller.rbrubocop-minitest extension supports plugin, specify ... [truncated 255 characters] ... fig_loader_resolver.rb:76:in
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. Comment |
matuaya
force-pushed
the
chore/centralize-user-retirement
branch
from
681ea95 to
08003f9
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/models/user.rb (1)
418-420: 既存スコープを再利用してDRYに
mentorスコープを活かすと読みやすくなります。- scope :mentors_excluding_admins, lambda { - where(mentor: true, admin: false) - } + scope :mentors_excluding_admins, -> { mentor.where(admin: false) }app/controllers/retirement_controller.rb (1)
18-19: triggered_by を明示して将来の条件分岐に備えるデフォルト分岐に依存せず、意図を明確化しましょう。
- AfterUserRetirement.new(user).call + AfterUserRetirement.new(user, triggered_by: 'user').callapp/controllers/scheduler/daily/auto_retire_controller.rb (1)
18-18: 1ユーザーの失敗でバッチ全体が停止しないように保護ループ内で例外を握りつぶさずログ化して継続を推奨。
- AfterUserRetirement.new(user, triggered_by: 'hibernation').call + begin + AfterUserRetirement.new(user, triggered_by: 'hibernation').call + rescue => e + Rails.logger.error "AutoRetire failed for user_id=#{user.id}: #{e.class}: #{e.message}" + endapp/models/after_user_retirement.rb (1)
52-54:sendではなくpublic_sendを使用意図しないプライベートメソッド呼び出しを防ぎます。
- UserMailer.send(event, @user).deliver_now + UserMailer.public_send(event, @user).deliver_now
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
app/controllers/admin/users_controller.rb(1 hunks)app/controllers/retirement_controller.rb(1 hunks)app/controllers/scheduler/daily/auto_retire_controller.rb(1 hunks)app/models/after_user_retirement.rb(1 hunks)app/models/user.rb(1 hunks)app/models/user_retirement.rb(0 hunks)
💤 Files with no reviewable changes (1)
- app/models/user_retirement.rb
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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モデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/models/user.rbapp/controllers/retirement_controller.rbapp/controllers/admin/users_controller.rbapp/models/after_user_retirement.rbapp/controllers/scheduler/daily/auto_retire_controller.rb
⏰ 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
app/models/after_user_retirement.rb
Outdated
| def call | ||
| Newspaper.publish(:retirement_create, { user: @user }) | ||
| destroy_subscription | ||
|
|
||
| if @triggered_by == 'admin' | ||
| admin_steps | ||
| elsif @triggered_by == 'hibernation' | ||
| hibernation_steps | ||
| else | ||
| user_steps | ||
| end | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
管理者退会時にも Newspaper.publish が実行され、想定外の通知が出るリスク
仕様(動作確認手順4)では「管理画面からの退会は通知なし」。Newspaper.publish を非管理者フローに限定してください。
def call
- Newspaper.publish(:retirement_create, { user: @user })
destroy_subscription
-
- if @triggered_by == 'admin'
- admin_steps
- elsif @triggered_by == 'hibernation'
- hibernation_steps
- else
- user_steps
- end
+ case @triggered_by
+ when 'admin'
+ admin_steps
+ when 'hibernation'
+ publish_retirement_event
+ hibernation_steps
+ else
+ publish_retirement_event
+ user_steps
+ end
end
+
+ def publish_retirement_event
+ Newspaper.publish(:retirement_create, { user: @user })
+ end検証用: イベント購読側で通知発火していないか確認をお願いします。
🏁 Script executed:
#!/bin/bash
# retirement_create イベントの購読箇所をサーチ
rg -n "retirement_create" -C3Length of output: 1374
管理者退会時の :retirement_create 通知をスキップする
app/models/after_user_retirement.rb の call メソッドで、@triggered_by == 'admin' の場合に Newspaper.publish(:retirement_create, …) が実行されないよう、以下のように非管理者フロー（‘hibernation’ とそれ以外）でのみ呼び出す実装に変更してください。config/initializers/newspaper.rb で UnfinishedDataDestroyer と TimesChannelDestroyer が購読しているため、現状では管理画面からの退会でも通知処理が走ってしまいます。
def call
- Newspaper.publish(:retirement_create, { user: @user })
destroy_subscription
- if @triggered_by == 'admin'
- admin_steps
- elsif @triggered_by == 'hibernation'
- hibernation_steps
- else
- user_steps
- end
+ case @triggered_by
+ when 'admin'
+ admin_steps
+ when 'hibernation'
+ publish_retirement_event
+ hibernation_steps
+ else
+ publish_retirement_event
+ user_steps
+ end
end
+ private
+ def publish_retirement_event
+ Newspaper.publish(:retirement_create, { user: @user })
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def call | |
| Newspaper.publish(:retirement_create, { user: @user }) | |
| destroy_subscription | |
| if @triggered_by == 'admin' | |
| admin_steps | |
| elsif @triggered_by == 'hibernation' | |
| hibernation_steps | |
| else | |
| user_steps | |
| end | |
| end | |
| def call | |
| destroy_subscription | |
| case @triggered_by | |
| when 'admin' | |
| admin_steps | |
| when 'hibernation' | |
| publish_retirement_event | |
| hibernation_steps | |
| else | |
| publish_retirement_event | |
| user_steps | |
| end | |
| end | |
| private | |
| def publish_retirement_event | |
| Newspaper.publish(:retirement_create, { user: @user }) | |
| end |
🤖 Prompt for AI Agents
In app/models/after_user_retirement.rb around lines 9 to 20, the call currently
publishes Newspaper.publish(:retirement_create, { user: @user }) unconditionally
which triggers subscribers even for admin-initiated retirements; change the flow
so that the publish is executed only for non-admin triggers (i.e., when
@triggered_by == 'hibernation' or otherwise), by moving or wrapping the
Newspaper.publish call into the hibernation and user branches (or adding a guard
that skips publish when @triggered_by == 'admin'), and keep destroy_subscription
and the existing admin_steps/hibernation_steps/user_steps logic unchanged.
There was a problem hiding this comment.
@triggered_by == 'admin' の場合でもNewspaper.publish(:retirement_create, …)を実行する必要があります。ここで行われる処理は通知ではありません。
There was a problem hiding this comment.
🧩 Analysis chain
@matuaya ありがとうございます。私の理解が間違っていました。Newspaper.publish(:retirement_create, ...)の処理が通知ではなく、他の重要な処理（データクリーンアップ等）を行うということですね。
現在の実装のまま、すべての退会ケース（admin、hibernation、user）でNewspaper.publish(:retirement_create, ...)を実行するのが正しいということを理解しました。私の提案は不適切でした。
🏁 Script executed:
#!/bin/bash
# Let's examine what UnfinishedDataDestroyer and TimesChannelDestroyer actually do
# when they receive the retirement_create event
# First, let's find and examine UnfinishedDataDestroyer
rg -n "class UnfinishedDataDestroyer" -A 20
rg -n "retirement_create" -A 10 -B 5
# Let's also look at TimesChannelDestroyer
rg -n "class TimesChannelDestroyer" -A 20
Length of output: 5350
@matuaya 完全に理解しました。ありがとうございます。
Newspaper.publish(:retirement_create, { user: @user })で実行される処理を確認しました：
- UnfinishedDataDestroyer: 未チェックのProductやWIPのReportの削除、career_pathのリセット
- TimesChannelDestroyer: Discordの分報チャンネルの削除とdiscord_profileの情報クリア
これらは確かに通知処理ではなく、データの整合性を保つための重要なクリーンアップ処理ですね。管理者による退会の場合でも、これらの処理は必須です。
現在の実装が正しく、私の提案は不適切でした。申し訳ありませんでした。
✏️ Learnings added
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.334Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.334Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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になることは想定されておらず、エラーハンドリングは不要である。
08003f9 to
326254d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/models/after_user_retirement.rb (1)
9-21: 管理者退会時の通知イベント発火を抑止すべき（仕様不一致）adminトリガーでも常に
Newspaper.publish(:retirement_create, ...)が実行され、購読側で通知が飛ぶ恐れがあります。管理者退会では「誰にも通知しない」要件に反します。非admin（hibernation/user）のみpublishに限定してください。修正例:
def call - Newspaper.publish(:retirement_create, { user: @user }) destroy_subscription - case @triggered_by - when 'admin' - admin_steps - when 'hibernation' - hibernation_steps - when 'user' - user_steps - end + case @triggered_by + when 'admin' + admin_steps + when 'hibernation' + publish_retirement_event + hibernation_steps + when 'user' + publish_retirement_event + user_steps + end end
🧹 Nitpick comments (4)
app/controllers/scheduler/daily/auto_retire_controller.rb (1)
11-19: 自動退会ループはユーザー単位で例外分離し、処理継続・ロギングを1件で例外が出ると以降のユーザーが処理されません。find_each + per-user rescueで耐障害性を上げましょう。
適用例:
- User.unretired.hibernated_for(User::HIBERNATION_LIMIT).auto_retire.each do |user| + User.unretired.hibernated_for(User::HIBERNATION_LIMIT).auto_retire.find_each do |user| - user.retire_reason = '（休会後三ヶ月経過したため自動退会）' - user.retired_on = Date.current - user.hibernated_at = nil - user.save!(validate: false) - - AfterUserRetirement.new(user, triggered_by: 'hibernation').call + begin + user.retire_reason = '（休会後三ヶ月経過したため自動退会）' + user.retired_on = Date.current + user.hibernated_at = nil + user.save!(validate: false) + AfterUserRetirement.new(user, triggered_by: 'hibernation').call + rescue => e + Rails.logger.error("AutoRetire failed user_id=#{user.id}: #{e.class} #{e.message}") + end endapp/models/after_user_retirement.rb (3)
23-27: publishをプライベートに切り出して意図を明確化可読性と誤用防止のため専用メソッド化を。
private + def publish_retirement_event + Newspaper.publish(:retirement_create, { user: @user }) + end
39-41: 外部API失敗時のハンドリングを追加サブスク解約失敗で全処理が落ちると実務影響大。ログに落として継続推奨。
- def destroy_subscription - Subscription.new.destroy(@user.subscription_id) if @user.subscription_id? - end + def destroy_subscription + return unless @user.subscription_id? + Subscription.new.destroy(@user.subscription_id) + rescue => e + Rails.logger.error("Subscription destroy failed user_id=#{@user.id}: #{e.class} #{e.message}") + end
53-57: メールはdeliver_later推奨（スケジューラ/管理画面の応答遅延を回避）非同期で十分な性質のため。Postmark例外ハンドリングは現状維持でOK。
- UserMailer.send(event, @user).deliver_now + UserMailer.send(event, @user).deliver_later
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
app/controllers/admin/users_controller.rb(2 hunks)app/controllers/retirement_controller.rb(1 hunks)app/controllers/scheduler/daily/auto_retire_controller.rb(1 hunks)app/models/after_user_retirement.rb(1 hunks)app/models/user.rb(1 hunks)app/models/user_retirement.rb(0 hunks)
💤 Files with no reviewable changes (1)
- app/models/user_retirement.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/user.rb
- app/controllers/retirement_controller.rb
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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モデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/models/after_user_retirement.rbapp/controllers/admin/users_controller.rbapp/controllers/scheduler/daily/auto_retire_controller.rb
⏰ 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 (5)
app/controllers/admin/users_controller.rb (2)
36-38: 責務の切り出しは適切（更新直後の分岐をヘルパーへ集約）update内がすっきりし、テストもしやすくなりました。
103-107: 二重解約の温床を解消できていますretired_on変更を優先し、卒業側はelsif相当でフォールバックする構造により重複destroy回避ができています（既存指摘の解決）。
app/models/after_user_retirement.rb (3)
65-69: mentor通知の対象絞り込みは適切
mentors_excluding_adminsの導入により通知重複を回避できています。
29-37: （要確認）自動退会でもカード/GitHubデータのクリーンアップは必要ですか？現状はuserトリガーのみで実施。自動退会時の整合性要件によってはadmin_steps/hibernation_stepsにも必要かもしれません。
検証観点:
- retirement_create購読側でデータ消去が賄われているか
- 自動退会時に残存カード・GitHub情報がないこと
必要なら以下のように共通化可能です（例）:
def hibernation_steps - notify_all(:auto_retire) + notify_all(:auto_retire) + destroy_card + @user.clear_github_data end
9-21: admin トリガー時に購読者の副作用を再確認してください
:retirement_createは以下で購読されています。admin ケースでpublishを抑止すると、これらの destroyer が実行されず、想定外のデータ残存やチャネル解放漏れが発生する可能性があります。通知以外に必要なクリーンアップ処理がないか確認してください。
- config/initializers/newspaper.rb:14（UnfinishedDataDestroyer）
- config/initializers/newspaper.rb:18（TimesChannelDestroyer）
matuaya
force-pushed
the
chore/centralize-user-retirement
branch
from
326254d to
83829d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/admin/users_controller.rb (1)
44-52: 自分自身の削除防止ガードが無効（redirect後も処理継続＋型不一致）現状だと管理者自身を削除できてしまいます。
redirect_to後にreturnがなく、params[:id]（String）とcurrent_user.id（Integer）の比較も不一致です。修正をお願いします。def destroy - # 今後本人退会時に処理が増えることを想定し、自分自身は削除できないよう - # 制限をかけておく - redirect_to admin_users_url, alert: '自分自身を削除する場合、退会から処理を行ってください。' if current_user.id == params[:id] + # 今後本人退会時に処理が増えることを想定し、自分自身は削除できないよう制限 + if current_user.id == params[:id].to_i + redirect_to admin_users_url, alert: '自分自身を削除する場合、退会から処理を行ってください。' + return + end
♻️ Duplicate comments (1)
app/controllers/admin/users_controller.rb (1)
37-38: 退会処理のサービス化への委譲はOK
AfterUserRetirementへの一本化により、管理者経由でも必ず共通フローが走る構成になっており妥当です（Newspaper.publish が分岐前に実行される実装とも整合）。
🧹 Nitpick comments (4)
test/models/user_retirement_test.rb (4)
6-6: テスト名の表現を実装に合わせて更新
execute→callに合わせて名前を揃えると読みやすいです。- test 'execute method performs all retirement steps' do + test 'call performs all retirement steps for user-triggered retirement' do
22-26: 重要イベントの発火（Newspaper.publish）も検証しましょう
Newspaper.publish(:retirement_create, user: user)は通知ではなく重要なクリーンアップのトリガーです。発火をテストで担保しておくと安全です。- retirement.call + published = false + Newspaper.stub(:publish, ->(event, payload) { published = (event == :retirement_create && payload[:user] == user) }) do + retirement.call + end + assert_equal true, published
8-9: 他トリガーの分岐テストも追加を検討
triggered_by: 'admin'（誰にも通知なし）とtriggered_by: 'hibernation'の分岐も最小限で良いので回しておくと将来の退行を防げます。実装支援できます。
30-33: 順序依存のアサーションは最小化を検討内部実装変更に脆いので、必須順序だけ（例:
destroy_subscriptionが通知より前）を部分アサートにするのも一手です。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
app/controllers/admin/users_controller.rb(2 hunks)app/controllers/retirement_controller.rb(1 hunks)app/controllers/scheduler/daily/auto_retire_controller.rb(1 hunks)app/models/after_user_retirement.rb(1 hunks)app/models/user.rb(1 hunks)app/models/user_retirement.rb(0 hunks)test/models/user_retirement_test.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/models/user_retirement.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- app/controllers/retirement_controller.rb
- app/models/user.rb
- app/controllers/scheduler/daily/auto_retire_controller.rb
- app/models/after_user_retirement.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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モデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/controllers/admin/users_controller.rbtest/models/user_retirement_test.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/models/user_retirement_test.rb
🧠 Learnings (3)
📓 Common learnings
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.334Z
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.334Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
📚 Learning: 2025-09-01T22:31:57.334Z
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.334Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Applied to files:
app/controllers/admin/users_controller.rbtest/models/user_retirement_test.rb
📚 Learning: 2025-09-01T22:31:57.334Z
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.334Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Applied to files:
app/controllers/admin/users_controller.rbtest/models/user_retirement_test.rb
🧬 Code graph analysis (2)
app/controllers/admin/users_controller.rb (1)
app/models/after_user_retirement.rb (2)
call(9-21)destroy_subscription(39-41)
test/models/user_retirement_test.rb (1)
app/models/after_user_retirement.rb (1)
call(9-21)
🔇 Additional comments (1)
app/controllers/admin/users_controller.rb (1)
109-113: 更新前のsubscription_idを利用してサブスク解約Rails 6.1.7 で
user.saved_change_to_subscription_id?とuser.subscription_id_before_last_saveが利用可能なため、解約処理時に更新前のIDを優先してください。def destroy_subscription(user) - return if user.subscription_id.blank? - - Subscription.new.destroy(user.subscription_id) + sid = if user.saved_change_to_subscription_id? + user.subscription_id_before_last_save + else + user.subscription_id + end + return if sid.blank? + Subscription.new.destroy(sid) end
matuaya
force-pushed
the
chore/centralize-user-retirement
branch
from
83829d9 to
0fde534
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/models/after_user_retirement.rb (4)
39-41: predicate より present? の方が明確で既存コードとも整合admin/users_controller.rb では blank?/present? を使っているため、ここも合わせると読みやすさ・一貫性が上がります（挙動も意図どおり）。
- Subscription.new.destroy(@user.subscription_id) if @user.subscription_id? + Subscription.new.destroy(@user.subscription_id) if @user.subscription_id.present?
53-57: Mailer は send ではなく public_send を使用意図しない private メソッド実行を避けるため public_send が安全です（今回の event は固定ですがベストプラクティス）。
- UserMailer.send(event, @user).deliver_now + UserMailer.public_send(event, @user).deliver_now
59-63: 多数ユーザー時のメモリ節約に find_each を推奨admins が多い環境でも安定するようバッチ処理に。
- User.admins.each do |admin_user| + User.admins.find_each do |admin_user|
65-69: メンター通知も find_each で統一一貫性とメモリ使用量低減のため。
- User.mentors_excluding_admins.each do |mentor_user| + User.mentors_excluding_admins.find_each do |mentor_user|
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
app/controllers/admin/users_controller.rb(2 hunks)app/controllers/retirement_controller.rb(1 hunks)app/controllers/scheduler/daily/auto_retire_controller.rb(1 hunks)app/models/after_user_retirement.rb(1 hunks)app/models/user.rb(1 hunks)app/models/user_retirement.rb(0 hunks)test/models/user_retirement_test.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/models/user_retirement.rb
🚧 Files skipped from review as they are similar to previous changes (5)
- app/models/user.rb
- app/controllers/retirement_controller.rb
- test/models/user_retirement_test.rb
- app/controllers/admin/users_controller.rb
- app/controllers/scheduler/daily/auto_retire_controller.rb
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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モデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/models/after_user_retirement.rb
🧠 Learnings (6)
📓 Common learnings
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.334Z
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.334Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
📚 Learning: 2025-09-01T22:31:57.334Z
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.334Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Applied to files:
app/models/after_user_retirement.rb
📚 Learning: 2025-09-01T22:31:57.334Z
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.334Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Applied to files:
app/models/after_user_retirement.rb
📚 Learning: 2025-07-23T21:11:21.826Z
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になることは想定されておらず、エラーハンドリングは不要である。
Applied to files:
app/models/after_user_retirement.rb
📚 Learning: 2025-07-23T21:11:21.826Z
Learnt from: jun-kondo
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/work_notification_destroyer.rb`において、このクラスは`app/controllers/works_controller.rb`の`destroy`アクションから呼び出され、`before_action :set_my_work`でIDの妥当性チェックが事前に行われるため、payloadのworkオブジェクトは常に有効であることが保証されている。そのためエラーハンドリングは不要。
Applied to files:
app/models/after_user_retirement.rb
📚 Learning: 2025-07-15T12:58:02.735Z
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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
Applied to files:
app/models/after_user_retirement.rb
🧬 Code graph analysis (1)
app/models/after_user_retirement.rb (2)
app/controllers/admin/users_controller.rb (2)
destroy_subscription(113-117)destroy(44-52)app/models/user.rb (1)
clear_github_data(896-902)
⏰ 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 (1)
app/models/after_user_retirement.rb (1)
10-12: Newspaper.publish の無条件実行は仕様どおりでOK全トリガーでの実行は、データクリーンアップ（UnfinishedDataDestroyer/TimesChannelDestroyer）必須という学びに合致しています。ここはこのままで良いです。
app/models/after_user_retirement.rb
Outdated
| def destroy_card | ||
| Card.destroy_all(@user.customer_id) if @user.customer_id? | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Card.destroy_all の誤用により全削除/例外の危険あり—条件指定での削除に修正を
destroy_all に生ID（customer_id）を渡すと条件解釈されず不正SQL/全件削除リスクがあります。customer_id で絞って削除してください。
- def destroy_card
- Card.destroy_all(@user.customer_id) if @user.customer_id?
- end
+ def destroy_card
+ return unless @user.customer_id.present?
+ Card.destroy_by(customer_id: @user.customer_id)
+ end検証用スクリプト（cards テーブルの customer_id 有無と Card 周辺の削除呼び出しを確認）:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# customer_id が schema に存在するか
rg -n 'create_table "cards"' -C2 db/schema.rb || true
rg -n 'customer_id' -C2 db/schema.rb || true
# Card の destroy 呼び出し状況
rg -n 'Card\.(destroy_all|destroy_by|where)' -C2 app || trueLength of output: 674
Card.destroy_all(@user.customer_id) は不正条件となり全件削除の危険があるため、Card.destroy_by(customer_id: …) で絞って削除してください。
app/models/after_user_retirement.rb:43–45
- def destroy_card
- Card.destroy_all(@user.customer_id) if @user.customer_id?
- end
+ def destroy_card
+ return unless @user.customer_id.present?
+ Card.destroy_by(customer_id: @user.customer_id)
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def destroy_card | |
| Card.destroy_all(@user.customer_id) if @user.customer_id? | |
| end | |
| def destroy_card | |
| return unless @user.customer_id.present? | |
| Card.destroy_by(customer_id: @user.customer_id) | |
| end |
🤖 Prompt for AI Agents
In app/models/after_user_retirement.rb around lines 43 to 45, the call
Card.destroy_all(@user.customer_id) is unsafe because it passes a positional
argument and can result in deleting all records; replace it with a scoped
deletion like Card.destroy_by(customer_id: @user.customer_id) (or
Card.where(customer_id: @user.customer_id).destroy_all) and keep the existing
guard if @user.customer_id? so only cards matching that customer_id are deleted.
There was a problem hiding this comment.
このdestroy_allはカスタムメソッドで、customer_id のカードのみを削除する処理になっていて、ActiveRecordのdestroy_allとは違っていると思います。
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the Card model and examine any custom destroy_all method
echo "=== Searching for Card model ==="
fd -t f -e rb "card" app/models/ | head -5
echo -e "\n=== Looking for Card model content ==="
cat app/models/card.rb 2>/dev/null || echo "Card model not found at app/models/card.rb"
echo -e "\n=== Searching for destroy_all method in Card-related files ==="
rg -n "def destroy_all" app/models/ -A5 -B2
echo -e "\n=== Searching for Card class definition and destroy_all usage ==="
rg -n "class Card" -A20 app/models/
echo -e "\n=== Searching for any destroy_all method calls on Card ==="
rg -n "Card\.destroy_all" -B2 -A2 app/
Length of output: 4061
@matuaya さん、ご指摘の通りです。私の分析が間違っていました。
Card クラスを確認すると、これはActiveRecordモデルではなく、Stripe APIをラップしたカスタムクラスでした：
def self.destroy_all(customer_id)
all(customer_id).each do |card|
destroy(customer_id, card['id'])
end
endこの実装では customer_id を引数として受け取り、その顧客のカードのみを安全に削除する処理になっています。ActiveRecordの destroy_all とは全く異なる動作で、全件削除のリスクはありません。
修正提案を撤回します。既存のコードは正しく動作します。ご指摘いただき、ありがとうございました。
(\ /)
( ._. )
o_(")(")
Oops, my mistake!
✏️ Learnings added
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:43-45
Timestamp: 2025-09-02T19:01:08.775Z
Learning: fjordllc/bootcampプロジェクトの`app/models/card.rb`は、ActiveRecordモデルではなくStripe APIをラップしたカスタムクラスである。`Card.destroy_all(customer_id)`メソッドは、指定されたcustomer_idに関連付けられたカードのみを削除するカスタム実装で、ActiveRecordの`destroy_all`とは動作が異なり安全である。
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.334Z
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.334Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Learnt from: jun-kondo
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/work_notification_destroyer.rb`において、このクラスは`app/controllers/works_controller.rb`の`destroy`アクションから呼び出され、`before_action :set_my_work`でIDの妥当性チェックが事前に行われるため、payloadのworkオブジェクトは常に有効であることが保証されている。そのためエラーハンドリングは不要。
|
@machida このCodeRabbitの指摘では、 「アドミンでログインした時に、ユーザーページから自分自身を削除できないように意図されているが、現状では削除できるようになっている。」 ですが、自分自身のユーザーページには「このユーザーを削除する」という項目が表示されないため、UI上ではそもそも自分自身を削除できないようになっています。 "アドミンが自分自身をユーザーページから削除しようとする場合"を想定していると思われますので、UI上でそれができない以上、コードは不要ではないかなと思います。 どのような対応をしたらよいかアドバイスいただけると嬉しいです！🙇♀️ |
|
@matuaya UI上で削除できなくなっているので、削除してしまう事故は防げるとは思うのですが、UI上だけでなくコード上もそうなっていることで、自分自身を削除しないようにしている設計であるとコード上からもわかるようになるので(間違えて削除リンクを非表示にしてしまっているという誤解がなくなる)、可能でしたらコードもそのようにしていただきたいです🙏 |
|
@machida |
matuaya
force-pushed
the
chore/centralize-user-retirement
branch
from
0fde534 to
70e557e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/controllers/scheduler/daily/auto_retire_controller.rb (1)
11-19: スケジューラの堅牢性向上（1件失敗で全体停止を回避）1ユーザー処理の例外でジョブ全体が止まる可能性があります。ユーザー単位で rescue＋ログ出力し、後続を継続するのを検討ください。
適用例:
def auto_retire User.unretired.hibernated_for(User::HIBERNATION_LIMIT).auto_retire.each do |user| user.retire_reason = '（休会後三ヶ月経過したため自動退会）' user.retired_on = Date.current user.hibernated_at = nil user.save!(validate: false) - AfterUserRetirement.new(user, triggered_by: 'hibernation').call + begin + AfterUserRetirement.new(user, triggered_by: 'hibernation').call + rescue => e + Rails.logger.error "[AutoRetire] user_id=#{user.id} retirement failed: #{e.class} #{e.message}" + end end endtest/models/user_retirement_test.rb (2)
16-20: ユニットテストは副作用の隔離を
Newspaper.publishの実サブスクライバが動くとテストの独立性が下がります。publish をスタブして呼び出し有無も検証するのを推奨します。適用例:
- %i[destroy_subscription destroy_card].each do |method_name| + %i[destroy_subscription destroy_card].each do |method_name| retirement.define_singleton_method(method_name) do methods_called << method_name end end加えて以下のように
retirement.callを囲み、発火も記録:- retirement.call + Newspaper.stub(:publish, ->(_event, _payload) { methods_called << :retirement_event }) do + retirement.call + end期待順序に
:retirement_eventを先頭に追加してください。
22-24: イベント種別の検証を追加すると堅牢です
notify_allに渡されるイベント（:retire）をアサートすると回 regressions を防げます。適用例:
- retirement.define_singleton_method(:notify_all) do |_event| - methods_called << :notify_all - end + captured_event = nil + retirement.define_singleton_method(:notify_all) do |event| + captured_event = event + methods_called << :notify_all + end + # call 後に: + # assert_equal :retire, captured_eventapp/models/after_user_retirement.rb (2)
9-21: 未知の trigger を検出・記録してください未定義の
triggered_byが来た場合に黙殺されます。警告ログを入れておくと障害解析に有用です。適用例:
case @triggered_by when 'admin' admin_steps when 'hibernation' hibernation_steps when 'user' user_steps + else + Rails.logger.warn "[AfterUserRetirement] Unknown triggered_by=#{@triggered_by.inspect} user_id=#{@user.id}" end
53-57: Mailer 呼び出しは public_send を使用
sendは private メソッドも呼べるため、public_sendが安全です。機能はそのまま。適用例:
- def notify_user(event) - UserMailer.send(event, @user).deliver_now + def notify_user(event) + UserMailer.public_send(event, @user).deliver_now rescue Postmark::InactiveRecipientError => e Rails.logger.warn "[Postmark] 受信者由来のエラーのためメールを送信できませんでした。：#{e.message}" end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
app/controllers/admin/users_controller.rb(3 hunks)app/controllers/retirement_controller.rb(1 hunks)app/controllers/scheduler/daily/auto_retire_controller.rb(1 hunks)app/models/after_user_retirement.rb(1 hunks)app/models/user.rb(1 hunks)app/models/user_retirement.rb(0 hunks)test/models/user_retirement_test.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/models/user_retirement.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/user.rb
- app/controllers/retirement_controller.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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モデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/models/after_user_retirement.rbapp/controllers/admin/users_controller.rbapp/controllers/scheduler/daily/auto_retire_controller.rbtest/models/user_retirement_test.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/models/user_retirement_test.rb
🧠 Learnings (7)
📓 Common learnings
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'）の場合でも必ず実行する必要がある。
📚 Learning: 2025-09-01T22:31:57.345Z
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）必ず実行する必要がある。
Applied to files:
app/models/after_user_retirement.rbapp/controllers/admin/users_controller.rbapp/controllers/scheduler/daily/auto_retire_controller.rbtest/models/user_retirement_test.rb
📚 Learning: 2025-09-01T22:31:57.345Z
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'）の場合でも必ず実行する必要がある。
Applied to files:
app/models/after_user_retirement.rbapp/controllers/admin/users_controller.rbapp/controllers/scheduler/daily/auto_retire_controller.rbtest/models/user_retirement_test.rb
📚 Learning: 2025-07-23T21:11:21.826Z
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になることは想定されておらず、エラーハンドリングは不要である。
Applied to files:
app/models/after_user_retirement.rb
📚 Learning: 2025-07-23T21:11:21.826Z
Learnt from: jun-kondo
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/work_notification_destroyer.rb`において、このクラスは`app/controllers/works_controller.rb`の`destroy`アクションから呼び出され、`before_action :set_my_work`でIDの妥当性チェックが事前に行われるため、payloadのworkオブジェクトは常に有効であることが保証されている。そのためエラーハンドリングは不要。
Applied to files:
app/models/after_user_retirement.rb
📚 Learning: 2025-07-15T12:58:02.735Z
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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
Applied to files:
app/models/after_user_retirement.rb
📚 Learning: 2025-09-02T19:01:08.804Z
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:43-45
Timestamp: 2025-09-02T19:01:08.804Z
Learning: fjordllc/bootcampプロジェクトの`app/models/card.rb`は、ActiveRecordモデルではなくStripe APIをラップしたカスタムクラスである。`Card.destroy_all(customer_id)`メソッドは、指定されたcustomer_idに関連付けられたカードのみを削除するカスタム実装で、ActiveRecordの`destroy_all`とは動作が異なり安全である。
Applied to files:
app/models/after_user_retirement.rb
🧬 Code graph analysis (4)
app/models/after_user_retirement.rb (2)
app/controllers/admin/users_controller.rb (2)
destroy_subscription(116-120)destroy(44-55)app/models/user.rb (1)
clear_github_data(896-902)
app/controllers/admin/users_controller.rb (2)
app/models/after_user_retirement.rb (2)
call(9-21)destroy_subscription(39-41)app/models/learning_cache_destroyer.rb (2)
call(3-9)call(4-8)
app/controllers/scheduler/daily/auto_retire_controller.rb (1)
app/models/after_user_retirement.rb (1)
call(9-21)
test/models/user_retirement_test.rb (1)
app/models/after_user_retirement.rb (1)
call(9-21)
⏰ 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 (3)
app/controllers/scheduler/daily/auto_retire_controller.rb (1)
18-18: AfterUserRetirement への集約は妥当です休会→自動退会フローでも、共通サービス経由で必須の
:retirement_createイベントが確実に発火します。OKです。app/controllers/admin/users_controller.rb (2)
36-38: 退会/卒業の副作用をサービスに一本化—OK更新後に
complete_graduation_or_retirement(@user)を呼ぶ流れは明確で良いです。
45-54: 自己削除ガードの明文化—OKUIで非表示でもコードで防御しておく判断は適切です。比較も
params[:id].to_iとcurrent_user.idで問題ありません。
matuaya
force-pushed
the
chore/centralize-user-retirement
branch
from
70e557e to
641b65d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/scheduler/daily/auto_retire_controller.rb (1)
11-20: 大量件数・失敗耐性の観点で find_each＋個別 rescue を検討日次ジョブで対象が多い場合に備え、バッチ処理（find_each）と1ユーザー失敗時に処理全体を止めない救済＋ロギングを入れておくと堅牢になります（順序が重要でなければ）。例外は通知（Sentry 等）があれば併用推奨。
- このループに順序依存はありますか？なければ find_each で問題ないはずです。
- 個別ユーザーで AfterUserRetirement が例外を投げた場合、処理継続でよい運用でしょうか（現在は全体が中断）？
適用例:
- User.unretired.hibernated_for(User::HIBERNATION_LIMIT).auto_retire.each do |user| + User.unretired.hibernated_for(User::HIBERNATION_LIMIT).auto_retire.find_each do |user| user.retire_reason = '（休会後三ヶ月経過したため自動退会）' user.retired_on = Date.current user.hibernated_at = nil user.save!(validate: false) - AfterUserRetirement.new(user, triggered_by: 'hibernation').call + begin + AfterUserRetirement.new(user, triggered_by: 'hibernation').call + rescue => e + Rails.logger.error("[auto_retire] user_id=#{user.id} #{e.class}: #{e.message}") + Sentry.capture_exception(e, extra: { user_id: user.id, triggered_by: 'hibernation' }) if defined?(Sentry) + end end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/controllers/admin/users_controller.rb(3 hunks)app/controllers/retirement_controller.rb(1 hunks)app/controllers/scheduler/daily/auto_retire_controller.rb(1 hunks)app/models/after_user_retirement.rb(1 hunks)app/models/user.rb(1 hunks)app/models/user_retirement.rb(0 hunks)test/models/user_retirement_test.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/models/user_retirement.rb
🚧 Files skipped from review as they are similar to previous changes (5)
- app/models/user.rb
- app/controllers/retirement_controller.rb
- app/controllers/admin/users_controller.rb
- app/models/after_user_retirement.rb
- test/models/user_retirement_test.rb
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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モデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/controllers/scheduler/daily/auto_retire_controller.rb
🧠 Learnings (3)
📓 Common learnings
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'）の場合でも必ず実行する必要がある。
📚 Learning: 2025-09-01T22:31:57.345Z
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）必ず実行する必要がある。
Applied to files:
app/controllers/scheduler/daily/auto_retire_controller.rb
📚 Learning: 2025-09-01T22:31:57.345Z
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'）の場合でも必ず実行する必要がある。
Applied to files:
app/controllers/scheduler/daily/auto_retire_controller.rb
🧬 Code graph analysis (1)
app/controllers/scheduler/daily/auto_retire_controller.rb (1)
app/models/after_user_retirement.rb (1)
call(9-21)
⏰ 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 (1)
app/controllers/scheduler/daily/auto_retire_controller.rb (1)
18-18: 共通化の適用は妥当。重要なクリーンアップが確実に実行されるAfterUserRetirement への委譲により、retirement_create の publish を起点に UnfinishedDataDestroyer/TimesChannelDestroyer などの副作用が自動退会でも確実に実行されるようになり、一貫性が上がっています。LGTMです。
|
@su-su-su-su |
|
@matuaya |
|
@su-su-su-su |
|
@sjabcdefin |
matuaya
requested review from
sjabcdefin
and removed request for
su-su-su-su
|
@sjabcdefin |
|
@komagata |
app/models/after_user_retirement.rb
Outdated
| @@ -0,0 +1,70 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| class AfterUserRetirement | |||
There was a problem hiding this comment.
全体の設計を考えるときにAfterUserRetirementの前にRetirementを考えたほうがいいようにおもいました。
There was a problem hiding this comment.
@komagata
AfterUserRetirementとは別にRetirementを作成するという認識でよいのでしょうか？
考えた方法は二つあります🙏
- ただAfterUserRetirementをRetirementでラップする
class Retirement
def initialize(user, triggered_by:)
@user = user
@triggered_by = triggered_by
end
def call
AfterUserRetirement.new(@user, triggered_by: @triggered_by).call
end
end
- Retirement作成時の処理もまとめる
class Retirement
def initialize(user, triggered_by:, retirement_params: nil, reason: nil)
@user = user
@triggered_by = triggered_by
@retirement_params = retirement_params
@reason = reason
end
def call
@user.retired_on = Date.current
@user.assign_attributes(@retirement_params) if @retirement_params.present?
@user.retire_reason = @reason if @reason.present?
@user.hibernated_at = nil
if @triggered_by == 'user'
@user.save(context: :retirement)
elsif @triggered_by == 'hibernation'
@user.save!(validate: false)
end
end
def execute_side_effects
AfterUserRetirement.new(@user, triggered_by: @triggered_by).call
end
end
もしかして根本的な部分を理解できていないかもしれないですが、その部分もふくめ方針についてアドバイス頂けたら嬉しいです🙇♀️
There was a problem hiding this comment.
@matuaya AfterUserRetirementというクラスが後付で増えている感じなので、リタイア周り全体をかんがえてオブジェクト指向プログラミングの考え方で設計しなおしてみてください。
https://bootcamp.fjord.jp/practices/211
先日やった下記の動画も参考になるかもです。
matuaya
force-pushed
the
chore/centralize-user-retirement
branch
from
056f371 to
b2c7d08
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/models/user_retirement_test.rb (2)
11-17: Newspaper.publishの呼び出し検証を強化してください。現在の実装では
:newspaperという汎用的なシンボルのみを記録していますが、実際には:retirement_createイベントと{ user: user }ペイロードが正しく渡されていることを検証すべきです。これにより、誤って別のイベント名を使用したり、ユーザー情報が欠落したりする問題を早期に発見できます。Based on learnings:
Newspaper.publish(:retirement_create, { user: user })は重要なデータクリーンアップ処理（UnfinishedDataDestroyer、TimesChannelDestroyer等）のトリガーであり、確実に正しく呼び出されることをテストで担保する必要があります。以下の修正を提案します：
def stub_newspaper methods_called = @methods_called - Newspaper.define_singleton_method(:publish) do |_event, _payload| - methods_called << :newspaper + Newspaper.define_singleton_method(:publish) do |event, payload| + methods_called << [:newspaper, event, payload[:user].id] end end
42-48: 期待値の検証を具体化してください（全3テストに適用）。
expected_methods配列を通常配列に変更し、Newspaperの呼び出しで:retirement_createイベントとユーザーIDが正しく渡されていることを明示的に検証してください。これはLines 42-48、61-65、78-81の全てのテストケースに適用されます。例（user triggered testの場合）：
expected_methods = %i[ - newspaper +] +expected_methods = [ + [:newspaper, :retirement_create, @user.id], destroy_subscription notify_all destroy_card clear_github_data ]同様の修正をhibernation triggered test（Lines 61-65）とadmin triggered test（Lines 78-81）にも適用してください。
🧹 Nitpick comments (1)
app/controllers/scheduler/daily/auto_retire_controller.rb (1)
12-16: 退会処理の共通化が適切に実装されていますRetirementサービスオブジェクトへの委譲により、退会処理が統一されています。
より堅牢にするため、以下の改善を検討してください：
- 各ユーザーの処理失敗時に他のユーザーの処理を継続できるようrescue節を追加
- 処理結果（成功/失敗）をログに記録
def auto_retire User.unretired.hibernated_for(User::HIBERNATION_LIMIT).auto_retire.each do |user| retirement = Retirement.new(user, triggered_by: 'hibernation') - retirement.call + rescue StandardError => e + Rails.logger.error("Auto retirement failed for user #{user.id}: #{e.message}") + # 他のユーザーの処理を継続 end end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/controllers/admin/users_controller.rb(2 hunks)app/controllers/retirement_controller.rb(1 hunks)app/controllers/scheduler/daily/auto_retire_controller.rb(1 hunks)app/models/user.rb(1 hunks)app/models/user_retirement.rb(0 hunks)test/models/user_retirement_test.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/models/user_retirement.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/retirement_controller.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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モデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/models/user.rbapp/controllers/scheduler/daily/auto_retire_controller.rbtest/models/user_retirement_test.rbapp/controllers/admin/users_controller.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/models/user_retirement_test.rb
🧠 Learnings (3)
📓 Common learnings
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'）の場合でも必ず実行する必要がある。
📚 Learning: 2025-09-01T22:31:57.345Z
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）必ず実行する必要がある。
Applied to files:
app/controllers/scheduler/daily/auto_retire_controller.rbtest/models/user_retirement_test.rbapp/controllers/admin/users_controller.rb
📚 Learning: 2025-09-01T22:31:57.345Z
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'）の場合でも必ず実行する必要がある。
Applied to files:
app/controllers/scheduler/daily/auto_retire_controller.rbtest/models/user_retirement_test.rbapp/controllers/admin/users_controller.rb
⏰ 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 (4)
app/models/user.rb (1)
417-417: LGTM！適切な実装です。このスコープは、管理者権限を持たないメンターを正確に抽出しており、実装は正しいです。退会通知フローにおいて、管理者とメンターの両方の権限を持つユーザーへの重複通知を防ぐために有用です。
既存のパターン（例：
not_advisers）と一貫性があり、スコープ名も明確で分かりやすいです。app/controllers/admin/users_controller.rb (3)
37-37: 退会・卒業処理の呼び出しが適切に実装されていますメソッド抽出により、updateメソッドの責務が明確になり、可読性が向上しています。
45-54: 自己削除ガードの実装が適切ですUI上の制約に加えて、コード上でも自己削除を防止することで、設計意図が明確になり、将来的な誤実装を防ぐことができます。コメントも分かりやすく記載されています。
106-112:Retirementクラスの定義場所とメソッド実装を確認してください
class Retirementが定義されたファイルが見つかりませんでした。以下のコマンドで定義と両メソッドを探し、一貫性の意図を整理してください。rg -n 'class Retirement' -g '*.rb' rg -n 'def call' -g '*.rb' rg -n 'def notify_and_cleanup' -g '*.rb'見つかった場所で
callとnotify_and_cleanupの振る舞いを比較し、adminトリガー時の適切なメソッド呼び出しに修正を検討してください。
matuaya
force-pushed
the
chore/centralize-user-retirement
branch
from
b2c7d08 to
ee7396c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/models/retirement.rb (1)
3-62: 退会処理の一元化が効果的に実現されています！
Retirementサービスクラスにより、以前はコントローラに散在していた退会処理が整理され、保守性が大幅に向上しています。エラーハンドリング（Postmark エラー）やクリーンアップフローも適切です。コーディングガイドラインによると、複数の ActiveRecord モデル操作や外部 API とのやり取りがある場合は Interactor パターンの使用が推奨されています。本クラスは該当するため、将来的に
interactorgem の使用を検討してください（現状は動作に問題ありません）。As per coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/controllers/admin/users_controller.rb(2 hunks)app/controllers/retirement_controller.rb(1 hunks)app/controllers/scheduler/daily/auto_retire_controller.rb(1 hunks)app/models/retirement.rb(1 hunks)app/models/retirement_handler/admin.rb(1 hunks)app/models/retirement_handler/hibernation.rb(1 hunks)app/models/retirement_handler/user.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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モデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/models/retirement_handler/admin.rbapp/models/retirement_handler/hibernation.rbapp/controllers/retirement_controller.rbapp/controllers/admin/users_controller.rbapp/controllers/scheduler/daily/auto_retire_controller.rbapp/models/retirement_handler/user.rbapp/models/retirement.rb
🧠 Learnings (3)
📓 Common learnings
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'）の場合でも必ず実行する必要がある。
📚 Learning: 2025-09-01T22:31:57.345Z
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'）の場合でも必ず実行する必要がある。
Applied to files:
app/models/retirement_handler/admin.rbapp/models/retirement_handler/hibernation.rbapp/controllers/retirement_controller.rbapp/controllers/admin/users_controller.rbapp/controllers/scheduler/daily/auto_retire_controller.rbapp/models/retirement_handler/user.rbapp/models/retirement.rb
📚 Learning: 2025-09-01T22:31:57.345Z
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）必ず実行する必要がある。
Applied to files:
app/models/retirement_handler/hibernation.rbapp/controllers/retirement_controller.rbapp/controllers/admin/users_controller.rbapp/controllers/scheduler/daily/auto_retire_controller.rbapp/models/retirement_handler/user.rbapp/models/retirement.rb
🧬 Code graph analysis (7)
app/models/retirement_handler/admin.rb (3)
app/models/retirement_handler/hibernation.rb (4)
notification_type(14-16)notify_user(18-20)notify_admins_and_mentors(22-24)additional_clean_up(26-26)app/models/retirement_handler/user.rb (4)
notification_type(13-15)notify_user(17-19)notify_admins_and_mentors(21-23)additional_clean_up(25-28)app/models/retirement.rb (2)
notify_user(40-44)notify_admins_and_mentors(46-50)
app/models/retirement_handler/hibernation.rb (3)
app/models/retirement.rb (4)
initialize(3-62)initialize(4-7)notify_user(40-44)notify_admins_and_mentors(46-50)app/models/retirement_handler/user.rb (7)
initialize(3-35)initialize(4-6)save_user(8-11)notification_type(13-15)notify_user(17-19)notify_admins_and_mentors(21-23)additional_clean_up(25-28)app/models/retirement_handler/admin.rb (5)
notification_type(3-15)notification_type(4-4)notify_user(6-8)notify_admins_and_mentors(10-12)additional_clean_up(14-14)
app/controllers/retirement_controller.rb (1)
app/models/retirement.rb (1)
call(9-18)
app/controllers/admin/users_controller.rb (1)
app/models/retirement.rb (1)
notify_and_cleanup(20-26)
app/controllers/scheduler/daily/auto_retire_controller.rb (1)
app/models/retirement.rb (1)
call(9-18)
app/models/retirement_handler/user.rb (4)
app/models/retirement.rb (4)
initialize(3-62)initialize(4-7)notify_user(40-44)notify_admins_and_mentors(46-50)app/models/retirement_handler/hibernation.rb (7)
initialize(3-27)initialize(4-6)save_user(8-12)notification_type(14-16)notify_user(18-20)notify_admins_and_mentors(22-24)additional_clean_up(26-26)app/models/retirement_handler/admin.rb (5)
notification_type(3-15)notification_type(4-4)notify_user(6-8)notify_admins_and_mentors(10-12)additional_clean_up(14-14)app/models/user.rb (1)
clear_github_data(894-900)
app/models/retirement.rb (3)
app/models/retirement_handler/hibernation.rb (7)
initialize(3-27)initialize(4-6)save_user(8-12)notification_type(14-16)notify_user(18-20)notify_admins_and_mentors(22-24)additional_clean_up(26-26)app/models/retirement_handler/user.rb (7)
initialize(3-35)initialize(4-6)save_user(8-11)notification_type(13-15)notify_user(17-19)notify_admins_and_mentors(21-23)additional_clean_up(25-28)app/models/retirement_handler/admin.rb (5)
notification_type(3-15)notification_type(4-4)notify_user(6-8)notify_admins_and_mentors(10-12)additional_clean_up(14-14)
⏰ 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 (6)
app/models/retirement_handler/user.rb (1)
3-35: 実装が正確で完成度が高いです！ユーザー起動の退会処理に必要な全ての機能が適切に実装されています：
- バリデーションコンテキスト（
:retirement）の使用- カード削除の安全なガード処理
- GitHub データのクリア
app/controllers/retirement_controller.rb (1)
10-23: 新しいRetirementサービスへの移行が適切です！退会処理のロジックが
Retirementサービスに正しく委譲されており、失敗時のフォールバック処理（20 行目でのretired_onのリセット）も適切に実装されています。app/controllers/scheduler/daily/auto_retire_controller.rb (1)
11-17: 自動退会処理がRetirementサービスに正しく統合されています！インラインで実装されていた複雑な退会ロジックが、シンプルで保守性の高いサービス呼び出しに置き換えられています。
app/controllers/admin/users_controller.rb (2)
45-54: 自己削除防止ガードの追加は設計意図を明確にする良い改善です！UI で既に制限されていますが、コードレベルでも防止することで、将来の誤った変更を防ぎ、設計意図が明確になります。
106-112: 退会・卒業処理の分岐ロジックが適切に実装されています！初回遷移（nil→値）のみを検知する条件分岐により、意図しない副作用の再実行を防いでいます。管理者による退会では
notify_and_cleanupを使用する設計も、既に保存済みのユーザーに対する処理として正しいです。app/models/retirement_handler/hibernation.rb (1)
3-27: 休会後の自動退会ハンドラが適切に実装されています！自動退会に必要な機能が正しく実装されています：
- 自動退会を示す日本語の退会理由の設定
- バリデーションをスキップした保存（
save!(validate: false)）- 適切な通知タイプ（
:auto_retire）の返却
| class RetirementHandler::Admin | ||
| def notification_type; end | ||
|
|
||
| def notify_user | ||
| false | ||
| end | ||
|
|
||
| def notify_admins_and_mentors | ||
| false | ||
| end | ||
|
|
||
| def additional_clean_up; end |
There was a problem hiding this comment.
save_user メソッドが未実装で、call 経由での使用時にエラーが発生します。
他のハンドラ（User、Hibernation）は save_user メソッドを実装していますが、Admin ハンドラには実装がありません。現在の使用箇所（admin/users_controller.rb の 108 行目）では notify_and_cleanup を直接呼び出しているため問題ありませんが、誤って retirement.call を呼び出すと NoMethodError が発生します。
インターフェースの一貫性を保つため、save_user メソッドを追加してください。
class RetirementHandler::Admin
+ def initialize(user)
+ @user = user
+ end
+
+ def save_user(_params)
+ # 管理者による退会では既にユーザーが保存済みのため何もしない
+ true
+ end
+
def notification_type; end
def notify_user📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class RetirementHandler::Admin | |
| def notification_type; end | |
| def notify_user | |
| false | |
| end | |
| def notify_admins_and_mentors | |
| false | |
| end | |
| def additional_clean_up; end | |
| class RetirementHandler::Admin | |
| def initialize(user) | |
| @user = user | |
| end | |
| def save_user(_params) | |
| # 管理者による退会では既にユーザーが保存済みのため何もしない | |
| true | |
| end | |
| def notification_type; end | |
| def notify_user | |
| false | |
| end | |
| def notify_admins_and_mentors | |
| false | |
| end | |
| def additional_clean_up; end |
🤖 Prompt for AI Agents
In app/models/retirement_handler/admin.rb around lines 3–14, the Admin handler
is missing the save_user method required by the retirement handler interface;
add a save_user(user) instance method that accepts the user object and persists
it (returning a boolean) to match the other handlers (User, Hibernation) so
calling retirement.call won't raise NoMethodError.
matuaya
force-pushed
the
chore/centralize-user-retirement
branch
2 times, most recently
from
f4915f0 to
f6e0b24
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/models/retirement/admin.rb (1)
15-15: 「通知なし」の意図を明示
def notification_type; endはnil返却になりますが、意図を明示するためにnilを返す実装にするのを推奨します。- def notification_type; end + def notification_type + nil # 管理者退会は通知を送らない + endapp/controllers/retirement_controller.rb (1)
16-19: 失敗時のフィードバックを追加するとUXが上がります（任意）
render :newの前にエラーメッセージを表示すると原因が分かりやすいです。- current_user.retired_on = nil - render :new + current_user.retired_on = nil + flash.now[:alert] = t('retirement.create_failed') + render :new, status: :unprocessable_entityapp/controllers/scheduler/daily/auto_retire_controller.rb (1)
12-14: 大量件数に備えてfind_eachと失敗ログを推奨スケジューラでは全件
eachはメモリ負荷になります。find_eachと簡易ログで堅牢性を上げましょう。- User.unretired.hibernated_for(User::HIBERNATION_LIMIT).auto_retire.each do |user| - Retirement.auto(user:).execute - end + User.unretired + .hibernated_for(User::HIBERNATION_LIMIT) + .auto_retire + .find_each(batch_size: 500) do |user| + ok = Retirement.auto(user:).execute + Rails.logger.warn("Auto retire failed user_id=#{user.id}") unless ok + end
auto_retireスコープがfind_eachと互換（= ActiveRecord::Relation）であることをご確認ください。app/models/retirement/auto.rb (1)
5-8: 理由メッセージはI18n化しておくと将来の変更に強いです（任意）- def assign_reason(user) - reason = '（休会後三ヶ月経過したため自動退会）' - user.retire_reason = reason - end + def assign_reason(user) + user.retire_reason = I18n.t('retirement.auto.reason') + endi18nキー追加は別PRでもOKです。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/controllers/admin/users_controller.rb(2 hunks)app/controllers/retirement_controller.rb(1 hunks)app/controllers/scheduler/daily/auto_retire_controller.rb(1 hunks)app/models/retirement.rb(1 hunks)app/models/retirement/admin.rb(1 hunks)app/models/retirement/auto.rb(1 hunks)app/models/retirement/self.rb(1 hunks)app/models/user_retirement.rb(0 hunks)test/models/retirement_test.rb(1 hunks)test/models/user_retirement_test.rb(0 hunks)test/system/admin/users_test.rb(2 hunks)test/system/auto_retire_test.rb(6 hunks)
💤 Files with no reviewable changes (2)
- test/models/user_retirement_test.rb
- app/models/user_retirement.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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モデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/models/retirement/self.rbapp/controllers/admin/users_controller.rbapp/models/retirement.rbapp/controllers/retirement_controller.rbapp/models/retirement/admin.rbtest/system/admin/users_test.rbapp/controllers/scheduler/daily/auto_retire_controller.rbtest/system/auto_retire_test.rbapp/models/retirement/auto.rbtest/models/retirement_test.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/system/admin/users_test.rbtest/system/auto_retire_test.rbtest/models/retirement_test.rb
🧠 Learnings (3)
📓 Common learnings
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'）の場合でも必ず実行する必要がある。
📚 Learning: 2025-09-01T22:31:57.345Z
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）必ず実行する必要がある。
Applied to files:
app/models/retirement/self.rbapp/controllers/admin/users_controller.rbapp/models/retirement.rbapp/controllers/retirement_controller.rbapp/models/retirement/admin.rbtest/system/admin/users_test.rbapp/controllers/scheduler/daily/auto_retire_controller.rbapp/models/retirement/auto.rb
📚 Learning: 2025-09-01T22:31:57.345Z
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'）の場合でも必ず実行する必要がある。
Applied to files:
app/controllers/admin/users_controller.rbapp/models/retirement.rbapp/controllers/retirement_controller.rbapp/models/retirement/admin.rbapp/controllers/scheduler/daily/auto_retire_controller.rb
🧬 Code graph analysis (8)
app/models/retirement/self.rb (3)
app/models/retirement.rb (5)
initialize(5-107)initialize(6-9)assign_reason(49-51)assign_date(53-55)save_user(61-63)app/models/retirement/admin.rb (5)
assign_reason(4-16)assign_reason(5-5)assign_date(7-9)save_user(11-13)notification_type(15-15)app/models/retirement/auto.rb (5)
assign_reason(4-21)assign_reason(5-8)assign_date(10-12)save_user(14-16)notification_type(18-20)
app/controllers/admin/users_controller.rb (1)
app/models/retirement.rb (2)
by_admin(19-21)execute(23-45)
app/models/retirement.rb (4)
app/models/retirement/self.rb (6)
initialize(4-24)initialize(5-7)assign_reason(9-11)assign_date(13-15)save_user(17-19)notification_type(21-23)app/models/retirement/admin.rb (5)
assign_reason(4-16)assign_reason(5-5)assign_date(7-9)save_user(11-13)notification_type(15-15)app/models/retirement/auto.rb (5)
assign_reason(4-21)assign_reason(5-8)assign_date(10-12)save_user(14-16)notification_type(18-20)app/models/user.rb (3)
clear_github_data(893-899)cancel_participation_from_regular_events(877-879)delete_and_assign_new_organizer(881-883)
app/controllers/retirement_controller.rb (1)
app/models/retirement.rb (2)
by_self(11-13)execute(23-45)
app/models/retirement/admin.rb (3)
app/models/retirement.rb (3)
assign_reason(49-51)assign_date(53-55)save_user(61-63)app/models/retirement/auto.rb (5)
assign_reason(4-21)assign_reason(5-8)assign_date(10-12)save_user(14-16)notification_type(18-20)app/models/retirement/self.rb (4)
assign_reason(9-11)assign_date(13-15)save_user(17-19)notification_type(21-23)
app/controllers/scheduler/daily/auto_retire_controller.rb (1)
app/models/retirement.rb (2)
auto(15-17)execute(23-45)
app/models/retirement/auto.rb (3)
app/models/retirement.rb (3)
assign_reason(49-51)assign_date(53-55)save_user(61-63)app/models/retirement/admin.rb (5)
assign_reason(4-16)assign_reason(5-5)assign_date(7-9)save_user(11-13)notification_type(15-15)app/models/retirement/self.rb (4)
assign_reason(9-11)assign_date(13-15)save_user(17-19)notification_type(21-23)
test/models/retirement_test.rb (4)
app/models/retirement.rb (4)
by_self(11-13)execute(23-45)auto(15-17)by_admin(19-21)app/models/retirement/admin.rb (1)
notification_type(15-15)app/models/retirement/auto.rb (1)
notification_type(18-20)app/models/retirement/self.rb (1)
notification_type(21-23)
⏰ 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 (17)
app/models/retirement/admin.rb (2)
7-13: 管理者フローの冪等性と検証無効化は妥当です
user.retired_on ||= Date.currentによる冪等化とsave!(validate: false)は要件どおりに見えます（管理者退会では通知も不要）。LGTM。
3-16: Newspaper.publish の実行確認完了コード検証の結果、要件は満たされています：
executeメソッド内でpublish_to_newspaperは 無条件に呼ばれています（35行目）publish_to_newspaperはnotifyより先に実行され、notifyの条件判定に影響を受けませんnotifyメソッドは条件分岐がありますが、publish_to_newspaperの実行をブロックしません- Admin退会の場合、
notification_typeはnilを返すためnotifyは早期リターンしますが、その前にpublish_to_newspaperは必ず実行されます全ての退会トリガー（by_admin、auto、by_self）が同一の
executeメソッドを使用するため、いかなるパターンでもNewspaper.publish(:retirement_create, { user: @user })による重要なデータクリーンアップ処理は必ず実行されます。app/models/retirement/self.rb (3)
9-11: Strong Parameters 経由の属性付与はOK
user.assign_attributes(@params)はRetirementControllerのretire_reason_paramsで限定されており安全です。LGTM。
17-19: 退会時のバリデーションコンテキストは適切
save!(context: :retirement)による専用バリデーションは妥当です。変更なし。このコンテキストでの必須項目（例:
retire_reason）がテストで担保されているか再確認をお願いします。
1-24: 外部API呼び出しをDBトランザクションの外へ移動することを強く推奨
Retirement.execute（app/models/retirement.rb Line 22-44）でdestroy_subscription（Stripe呼び出し）がトランザクション内にあります。外部I/OをDBトランザクション内で行うのはロック延長・遅延・再試行不能による失敗増の原因です。トランザクション完了後に実行してください。Based on learningsclass Retirement def execute - ActiveRecord::Base.transaction do + ActiveRecord::Base.transaction do assign_reason assign_date clear_hibernation_state save_user - destroy_subscription end + + # DB確定後に外部I/Oを実行 + destroy_subscription cancel_event_subscription remove_as_event_organizer clear_github_info destroy_cards publish_to_newspaper notify true失敗時の整合性は既存の
rescue Stripe::StripeErrorで継続可能（ユーザー状態は保存済み）です。⛔ Skipped due to learnings
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:43-45 Timestamp: 2025-09-02T19:01:08.804Z Learning: fjordllc/bootcampプロジェクトの`app/models/card.rb`は、ActiveRecordモデルではなくStripe APIをラップしたカスタムクラスである。`Card.destroy_all(customer_id)`メソッドは、指定されたcustomer_idに関連付けられたカードのみを削除するカスタム実装で、ActiveRecordの`destroy_all`とは動作が異なり安全である。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'）の場合でも必ず実行する必要がある。app/controllers/retirement_controller.rb (1)
11-19: フローの集約は明確で良いです成功時ログアウト→完了画面、失敗時は
retired_onの一時セットをクリアして再表示、いずれも妥当です。LGTM。app/models/retirement/auto.rb (1)
5-20: 自動退会ストラテジは要件通り固定理由付与、当日付与、検証無効化、通知種別
:auto_retireの整合性OKです。LGTM。test/system/auto_retire_test.rb (1)
19-23: LGTM - Card.destroy_all の副作用を適切に分離しています退会処理のテスト実行時に
Card.destroy_allをスタブ化することで、外部API呼び出しの副作用を分離しています。このパターンは全てのテストケースで一貫しており、新しい Retirement サービスによる集約化された退会フローに適切に対応しています。Also applies to: 56-60, 104-108, 127-131, 148-152, 169-173
test/system/admin/users_test.rb (1)
187-192: LGTM - 管理画面での退会・卒業フローにおける副作用の分離が適切です退会フロー（187-192行）と卒業フロー（238-243行）の両方で
Card.destroy_allをスタブ化し、副作用を分離しています。auto_retire_test.rbと同じパターンで一貫性があり、適切な実装です。Also applies to: 238-243
test/models/retirement_test.rb (2)
10-37: LGTM - 動的メソッド定義を使った呼び出し順序の検証が適切です
define_singleton_methodを使用して各メソッドの呼び出し順序を追跡し、executeメソッドが期待通りの順序で全ての必要なメソッドを呼び出すことを検証しています。この手法により、退会処理の実行フローが正しく保証されます。
72-94: LGTM - エラー発生時のロールバック動作を適切にテストしています
save_userとdestroy_subscriptionで例外が発生した場合に、トランザクションがロールバックされretired_onが nil のままであることを検証しています。データの整合性を保つために重要なテストケースです。app/controllers/admin/users_controller.rb (2)
44-54: LGTM - 自己削除の防止コードが適切に実装されていますUI上で自分自身の削除リンクが表示されないことに加え、コード上でも自己削除を防止することで設計意図を明確にしています。PRディスカッションで machida さんが指摘された懸念事項に適切に対応しており、将来的な事故を防ぐ良い実装です。
Based on learnings
106-112: LGTM - 初回遷移のみを検知する適切な実装です
saved_change_to_retired_on?とretired_on_before_last_save.nil?の組み合わせにより、nil→値への初回遷移のみで退会処理が実行されるようになっています。卒業処理も同様のパターンで実装されており、過去のレビューコメントで指摘された「日付の訂正や取り消しで副作用が再実行される」問題が適切に解決されています。退会を優先して
if/elsifで制御している点も正しい実装です。app/models/retirement.rb (4)
6-21: LGTM - Strategyパターンによる退会フローの統合が適切です3つの退会ケース（Self, Auto, Admin）に対応したファクトリーメソッドを提供し、各ケースに応じた Strategy インスタンスを注入しています。コメント（3-4行目）にも記載されている通り、Strategyパターンの簡易版として明確に実装されており、退会処理の共通化というPRの目的に合致しています。
As per coding guidelines
23-45: LGTM - トランザクション管理とエラーハンドリングが適切です重要なポイント：
- ユーザー情報の更新とサブスクリプション削除をトランザクション内で実行し、
ActiveRecord::RecordInvalidやStripe::StripeError発生時には確実にロールバックされます- トランザクション外の処理（イベントのキャンセル、GitHub情報のクリア、カード削除など）はユーザーが退会済みとして保存された後に実行されるため、これらの処理が失敗してもユーザーの退会状態は維持されます
この設計により、外部API障害時でもユーザーの退会処理が完了し、クリーンアップ処理は個別に失敗しても全体の退会フローを妨げません。
85-94: LGTM - Newspaper.publishによる重要なデータクリーンアップ処理が確実に実行されますRetrieved learnings に記載されている通り、
Newspaper.publish(:retirement_create, ...)は通知処理ではなく、UnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）や TimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行される必要があります。本実装では
notifyメソッド内で戦略のnotification_typeをチェックし、通知が不要な場合（Admin退会など）でもpublish_to_newspaperが必ず実行されるため、適切な実装です。Based on learnings
96-100: LGTM - Postmarkエラーの適切なハンドリング
Postmark::InactiveRecipientErrorをrescueしてログ出力のみ行うことで、受信者由来のメールエラーが退会処理全体を妨げないようにしています。これにより、ユーザーのメールアドレスが無効な場合でも退会処理を完了できます。
- メソッド名をcallからexecuteに変更 - RetirementHandlerを廃止し、Self, Auto, Adminクラスで退会処理を分離 - トランザクションでユーザー更新
matuaya
force-pushed
the
chore/centralize-user-retirement
branch
from
19191bd to
5e3bdac
Compare
|
@komagata もともと下記の処理は「ユーザー自身による退会」のみ対象でしたが、「自動退会」「アドミンによる退会」でも実施しない理由がないと思い、全てのケースで行われるようにしました。 もし「ユーザー自身による退会」のみを対象にしていた意図がありましたら修正したいと思いますのでご指摘ください🙇♀️ |
たしかにですね。ありがとうございます〜！ |
5 participants
Issue
概要
これまでユーザーの退会に伴う処理は
admin/users_controller.rbとauto_retire_controller.rbとretirement_controllerに点在していたので、AfterUserRetirementオブジェクトを作成して共通化を行いました。変更確認方法
chore/centralize-user-retirementをローカルに取り込むhajimeでログインする。http://localhost:3000/letter_opener/を開き以下を確認する。▫︎ アドミンとメンターにユーザー退会の通知が送られている。
▫︎ ユーザー(hajime)への通知はsubjectが「 退会処理が完了しました」である。
app/controllers/scheduler_controller.rbの６行目、before_action :require_tokenを一時的にコメントアウトする。（＊このユーザーにはダミーのサブスクリプション番号が登録されており、このユーザーを退会させようとすると定期支払い削除処理でエラーになるため）
http://localhost:3000/scheduler/daily/auto_retireにアクセス。（画面は表示されないが、処理が走る）http://localhost:3000/letter_opener/を開き以下を確認する。▫︎ アドミンとメンターにユーザー退会の通知が送られている。
▫︎ ユーザー(nagai-kyuukai)への通知はsubjectが「 重要なお知らせ：受講ステータスの変更について」である。
komagataでログインする。tomomiを選択し、ユーザーページの右下の項目「ステータス変更」の「管理者として情報変更」をクリックする。Screenshot
UIに変化がなかったためスクリーンショットは省略しています。
補足
Summary by CodeRabbit
リリースノート
New Features
Bug Fixes
Refactor
Tests