fix: PR レビューコメント対応 (slug 導入 / kwh 0始まり / Intl 安定化 ほか)#32
Merged
Conversation
en-Barry
force-pushed
the
fix/2605/en-Barry/pr-review-followup
branch
3 times, most recently
from
489031e to
50a9e53
Compare
PR レビューで指摘された以下を 1 つのデータモデル変更として対応する。slug 導入と YAML 構造変更、kilowatt_hour の境界仕様変更、Looop おうちプランの基本料金扱いが seeds.rb と YAML を共有して密接に絡むため、機能群としてまとめてコミットする。 対応したレビューコメント: - リブランディング時に plan.name が変わる可能性への対応: 不変の slug を グローバル unique なキーとして導入し、シードは find_or_initialize_by(slug:) ベースに変更。name は update! で上書きできる構造にした。 - plans.yml / ampere_based_rates.yml / usage_based_rates.yml で provider_name や plan_name が各エントリに重複していた問題: 親 (provider または plan) 配下に ネストする構造に統一し、入力ミスを防ぐ。 - rates YAML の plan 参照曖昧性 (find_by!(name:) の誤紐付けリスク): rates YAML 側も slug ベースにし、Plan.find_by!(slug:) で一意に特定する。 - kilowatt_hour_high: 9999 のハードコード解消: YAML は high: null として 「上限なし」を表現し、シード処理で ElectricityBillConstants::MAX_KWH を 埋める。YAML は MAX_KWH 変更時の更新が不要になる。 - kilowatt_hour_low の 1 始まり問題: 0 始まり境界共有 (例: 0/120, 120/300) に変更し、計算式の +1 を削除した。issue #3 の「下限は 0 kWh」記述と 自然に整合し、計算式と YAML 表記の非自明な結合が解消される。 併せて DB CHECK 制約を `kilowatt_hour_low > 0` から `>= 0` に緩和する マイグレーションを追加した。 - YAML.load_file のセキュリティリスク: YAML.safe_load_file に置き換え。 シードデータは基本型のみのため permitted_classes 指定なしで動作する。 - Looop おうちプランの基本料金が ampere_based_rates から除外されていた件: 対応アンペアをデータで明示するため全 7 アンペアに rate: "0.00" レコードを 追加し、Plan#base_price_for の ampere_based_rates.empty? 分岐を削除した。 slug 列は CreateMasterTables を直接編集して name の直後に追加した (PostgreSQL は ALTER TABLE で列順を変えられないため)。 本 PR 適用時に本番環境のデータをリセットして再投入する運用前提のため、 既存マイグレーションの編集を許容している。 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
レビュー指摘: バックエンド内の 10, 15, 20, 30, 40, 50, 60 は共通化しても
良いのではないかとの指摘に対応する。
ElectricityBillConstants::VALID_AMPERES が既に定数として存在しているが
AmpereBasedRate モデルのバリデーションでは参照せずハードコードしていた。
ElectricityBillConstants を include し、inclusion: { in: VALID_AMPERES } に
切り替えてアプリケーション層の参照箇所を統一する。
DB レベルの CHECK 制約は Ruby の定数から自動生成できないため二重管理に
なるが、これは「アプリケーションバリデーションがスキップされるケース
(insert_all / update_column / save(validate: false) 等) に対する最終
防衛ライン」として意図的に維持している。
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
レビュー指摘: Intl の通貨表記は実行環境 (Node/ICU バージョン) で 全角円記号 ￥ (U+FFE5) と半角円記号 ¥ (U+00A5) が混在しうるため、 固定文字列アサーションは CI 環境差で不安定になり得るとの指摘に対応する。 通貨記号の存在は引き続き担保しつつ、正規表現 /[¥￥]8,010/ で両方を 許容する形に変更する。 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
レビュー指摘: validation が "CHANGE_ME" 以外なら通ってしまい、短い文字列
でも ALB 直接アクセス防止の実効性が落ちる。推測耐性をコードで担保すべき
との指摘に対応する。
ecs_desired_count で複数 validation block を持つ既存パターンに合わせて、
以下 2 つの validation を追加した。
- length(var.origin_verify_secret) >= 32
- can(regex("[A-Za-z]", ...)) && can(regex("[0-9]", ...))
これにより 32 文字未満や英字のみ / 数字のみの secret は terraform plan
の段階で弾かれることを動作確認済み。
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
レビュー指摘: production でデフォルトの username: postgres / password: password
が引き継がれているように見え、誤解を招くという指摘に対応する。
production セクションは <<: *default を継承せず DATABASE_URL ベースで
接続するため、実際には default の認証情報が本番で使われることはなかった。
ただし、誤解を招く記載であることは確かなので、host / username / password を
ENV.fetch ベースに変更した。
ENV 未設定時のフォールバックを従来値 ("db" / "postgres" / "password") に
することで、ローカル開発時の挙動は維持される。
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
en-Barry
force-pushed
the
fix/2605/en-Barry/pr-review-followup
branch
from
50a9e53 to
afc1d09
Compare
seeds.rb は provider / plan / 2 種の料金レコードを 1 transaction で 投入するため自然に 25 行超えになる。設定で除外するのが適切。 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
kilowatt_hourを 0 始まり境界共有に、Looop おうちプランの基本料金を 0 円レコードで管理、各種ハードコード解消とテスト安定化対応したレビューコメント
コード変更を伴うもの (本 PR のスコープ):
kilowatt_hour_high: 9999のハードコード → YAML はhigh: null、シードでMAX_KWH補填kilowatt_hour_lowの 1 始まり問題 → 0 始まり境界共有 (例: 0/120, 120/300) に変更、計算式の+ 1を削除ampere_based_rates除外問題 → 全 7 アンペアにrate: \"0.00\"で対応アンペアをデータ明示、Plan#base_price_forのempty?分岐削除ElectricityBillConstants::VALID_AMPERESを参照YAML.load_fileのセキュリティリスク →YAML.safe_load_fileに置換/[¥￥]8,010/で全角/半角両方を許容origin_verify_secretの弱バリデーション → 32 文字以上 + 英数混在チェックを追加database.ymlの認証情報ハードコード →defaultセクションをENV.fetchベースに返信のみで対応する項目 (実装変更なし、設計意図の説明):
insert_all/update_column/save(validate: false)等) に対する最終防衛ライン設計上の判断
(provider_id, name)ユニーク制約: slug を globally unique にしつつ、name も per-provider unique を維持 (緩めない)kilowatt_hour_low >= 0かつkilowatt_hour_low < kilowatt_hour_highに統一Test plan
bundle exec rspecで 74 examples, 0 failuresrails db:seedを 2 回実行してエラーなしnpm testで 16 examples 全 pass (Intl 正規表現化後の安定動作)short/ 英字のみ 37 文字 / 64 文字 hex の 3 パターンで validation の挙動を確認 (短いものは長さエラー、英字のみは英数混在エラー、ランダム hex は通過)🤖 Generated with Claude Code