-
-
Notifications
You must be signed in to change notification settings - Fork 109
Refactor: is_active
カラムを削除し inactivated_at
に統一
#1744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor: is_active
カラムを削除し inactivated_at
に統一
#1744
Conversation
is_active カラムを削除し、inactivated_at カラムに統一するための 詳細な実装計画を作成しました。 - 8つのフェーズで段階的に実装 - 影響範囲の明確化(13ファイル) - リスク管理とロールバック計画を含む - 推定作業時間: 3時間20分 refs #1734
is_active
カラムを削除し inactivated_at
に統一
- Phase 1.0 に NotImplementedError を使った依存箇所検出を追加 - 段階的移行戦略(エラー→警告→実装)を明確化 - 実装戦略セクションを追加して全体の流れを説明
Ultrathinkingによる洞察: - スコープ名を維持すれば99%のコードは変更不要 - 必要な変更はたった7箇所(実質6箇所) - 実装時間を3時間→30分に短縮 - 複雑な8フェーズ→シンプルな3ステップに 主な変更: - 内部実装のみ変更する戦略を採用 - CSSクラス名変更も不要(YAGNIの実践) - 段階的移行の複雑な戦略を削除 - 本当に必要な変更だけに絞り込み 'Less is More' - 最小限の変更で最大の効果を実現
is_active
カラムを削除し inactivated_at
に統一is_active
カラムを削除し inactivated_at
に統一
変更内容: - 複雑なSQL文をorder_by_active_statusスコープに隠蔽 - コントローラーのコードが読みやすく改善 - 変更箇所は7→8箇所になるが、可読性が大幅向上 この変更により他の開発者もコードの意図を理解しやすくなる
実装内容: - Dojoモデル: active/inactiveスコープの内部実装を変更 - order_by_active_statusスコープを追加して可読性向上 - sync_active_status関連コードを削除(移行期間終了) - reactivate!メソッドからis_active更新を削除 - コントローラー: ソート条件を新しいスコープに変更 - Rakeタスク: is_active設定行を削除 これにより既存のインターフェースを維持しながら、内部実装を inactivated_atベースに統一。is_activeカラム削除の準備完了。 refs #1734
実装内容: - マイグレーションでis_activeカラムを削除 - is_active関連のテストを削除(不要になったため) - すべてのRSpecテストが成功 結果: - データの一貫性: inactivated_atカラムのみで管理 - コードの簡潔性: 重複したロジックを完全に削除 - 既存機能: すべて正常動作を確認 Closes #1734
変更内容: - 削除していたテストを復活 - is_active? の代わりに active? メソッドを使用 - 新規Dojoはデフォルトでアクティブになることを確認 これにより後方互換性を保ちながら、テストカバレッジも維持
削除内容: - 133箇所のis_activeフィールドを削除 - inactivated_atフィールドのみで管理するように統一 理由: - データベースからis_activeカラムは削除済み - Rakeタスクでも処理していない - Single Source of Truthの原則(重複情報の排除) - 将来の混乱を防ぐため 動作確認: - rails dojos:update_db_by_yaml が正常動作 - YAMLデータの整合性を確認済み
- Factory: is_activeを削除し、inactivated_atとinactiveトレイトを追加 - spec/models/stat_spec.rb: is_active参照を削除 - spec/requests/dojos_spec.rb: is_active参照をinactivated_atに変更 - spec/lib/tasks/dojos_spec.rb: is_activeテストをinactivated_atテストに置き換え - spec/models/dojo_spec.rb: is_active関連のテストを削除・簡素化 これにより全158個のテストが成功することを確認
@claude Review this PR with your full power and ultrathink. <3 |
Claude finished @yasulab's task —— View job このPRの詳細なレビューを行いました。全体的に非常に良い実装です! タスクリスト
レビュー結果✅ 良い実装1. シンプルで効果的なリファクタリング
2. 適切なマイグレーション戦略
3. データ整合性の維持
4. 可読性の向上# Before
.order(Arel.sql('CASE WHEN inactivated_at IS NULL THEN 0 ELSE 1 END'), order: :asc)
# After
.order_by_active_status
.order(order: :asc)
5. テストコードの適切な更新
💡 提案1. インデックスの追加を検討
# 将来的な最適化として
add_index :dojos, :inactivated_at 現在の道場数(325件)では影響は小さいですが、将来的な拡張性を考慮すると有効です。 2.
|
📋 概要
Issue #1734 に対応し、
is_active
カラムを削除してinactivated_at
カラムに統一する完全なリファクタリングです。✅ 実装完了!
🎯 KISS/YAGNI による超シンプル実装
結果: 99%のコードは触らずに済みました!
📝 実装内容
Step 1: データ整合性確認 ✅
Step 2: コード変更(最小限の変更) ✅
inactivated_at
ベースに変更order_by_active_status
スコープ追加(可読性向上)Step 3: データベース変更 ✅
is_active
カラムを削除するマイグレーション実行Step 4: YAMLファイルのクリーンアップ ✅
db/dojos.yaml
から133箇所のis_active
フィールドを削除Step 5: テストコードの修正 ✅
is_active
を削除、inactivated_at
とinactive
トレイトを追加is_active
参照を削除is_active
をinactivated_at
に変更is_active
テストをinactivated_at
テストに置き換え🔍 動作確認結果
データ整合性の完全検証
テスト結果
🎨 改善ポイント
可読性の向上
コントローラーの複雑なSQL文をスコープに隠蔽:
データの一貫性
is_active
カラム削除 ✅is_active
の参照を削除 ✅is_active
フィールドを削除 ✅inactivated_at
ベースに修正 ✅inactivated_at
ベースに統一📊 成果
🚀 このPRのメリット
inactivated_at
で非アクティブ化の時期を記録is_active
削除ではなくinactivated_at
への適切な置き換え🔗 関連
in_active
→inactivated_at
の命名を統一し、不要になったis_active
カラムを削除する #1734inactivated_at
toDojo
model to replaceis_active
boolean column #1726 (inactivated_at カラムの追加)CSV
for specific year #1732 (年次フィルタリング機能)Status: ✅ 実装完了 - レビュー待ち