Skip to content

PR #56: PDF取込機能が仕切書機能と無関係なスコープで混在している #57

Description

@strnh

背景

PR #56(issue #54「仕切書と定期請求スケジュール」の実装)のレビューで、仕切書機能と無関係な「quotebuilder出力PDFの取込」機能が同一PRに混在していることが判明した。

対象ファイル(PR #56 diff より、main には存在しない新規追加分):

  • app/Support/QuotePdfParser.php(新規、quotebuilder出力PDFのテキスト解析)
  • config/import.php(新規、pdftotext_binary 設定)
  • app/Http/Controllers/ImportController.php の PDF 対応追加(SUPPORTED_EXTSpdf 追加等)
  • resources/js/pages/Import.tsx の PDF 受け入れ文言変更
  • tests/Fixtures/*.pdf および関連テスト

PR #56 の本文は「issue #54 の実装」「予定データの生成と取り出しのみ(帳票出力・実績台帳・契約管理は対象外)」と明記しており、PDF取込機能はスコープに含まれていない。

問題点

  • CLAUDE.md の方針「小さく戻しやすい変更を優先し」「Laravel側とReact/Vite側の責務を混ぜない」に反し、無関係な2機能(仕切書 / PDF取込)が1つのPRに混在している。
  • PR本文にもPDF取込追加の記載がなく、レビュワーが見積もり外の差分を見落とすリスクがある。
  • 仕切書機能とPDF取込機能は影響範囲・ロールバック単位が異なるため、同一コミット群での取り込みはレビュー可能性・切り戻し容易性を損なう。

対応案

  • PDF取込関連の変更(QuotePdfParser / ImportController のPDF対応 / config/import.php / Import.tsx の文言変更 / 関連テスト)を、仕切書機能とは別のPRに切り出す。
  • 既にマージ済みの場合は、事後的にでもPR本文またはCHANGELOG相当の記録にスコープを明記する。
  • 今後、複数の独立した機能追加を1PRにまとめないよう、レビュー時にスコープ照合(PR本文 vs diff)を明示的にチェックする運用を検討する。

参照

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions