納品書PDFの店舗情報出力項目を管理者が表示/非表示できるようにする - #6953
Conversation
納品書PDF(nouhinsyo.pdf)の店舗情報欄に、会社名・会社名カナ・店名カナ・ 店名英語・店舗営業時間・店舗からのメッセージが出力されていなかった。 店舗設定>基本設定に「納品書PDFの出力項目」トグルを追加し、管理者が各項目の 表示/非表示を切り替えられるようにする。 - BaseInfo に order_pdf_visible_* の boolean 列を11個追加(既定は現状出力中の 項目+インボイス要件の会社名を ON、新規項目は OFF)。dtb_base_info への マイグレーションは列単位の冪等実装。 - ShopMasterType / shop_master.twig に納品書の描画順どおりのトグルを追加。 - OrderPdfService::renderShopData() を固定座標から y 座標カーソルの動的 レイアウトへ変更。非表示・空値の行は詰まり後続が繰り上がる。店舗からの メッセージは MultiCell で折り返し、総合計金額ボックスを侵食しない。 - ShopControllerTest / OrderPdfControllerTest にトグル永続化と全項目 ON 時の PDF 生成(メッセージ折り返し含む)の回帰テストを追加。 closes #6197 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough納品書PDFの店舗情報11項目に表示設定を追加しました。管理画面で設定を保存でき、設定と値の有無に応じてPDFへ動的に描画します。店舗メッセージは表示領域内で折り返し、各種テストで動作を検証しています。 Changes納品書PDF店舗情報表示設定
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant 管理者
participant ShopMasterType
participant BaseInfo
participant OrderPdfService
participant PDF
管理者->>ShopMasterType: 表示トグルを送信
ShopMasterType->>BaseInfo: 表示設定を保存
管理者->>OrderPdfService: 納品書PDFを要求
OrderPdfService->>BaseInfo: 表示設定と店舗情報を取得
OrderPdfService->>PDF: 表示対象項目を動的に描画
OrderPdfService->>PDF: メッセージを領域内で折り返し描画
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/DoctrineMigrations/Version20260722120000.php`:
- Around line 30-85: Remove the Version20260722120000 migration class and its
column-addition logic. Keep the order_pdf_visible_* schema changes represented
only by the corresponding ORM column attributes in BaseInfo.php, relying on
schema:update for database synchronization.
In `@src/Eccube/Service/OrderPdfService.php`:
- Around line 389-400: Update renderShopMessage() to save the current font state
with backupFont() before SetFont and restore it with restoreFont() after
MultiCell completes. Keep the existing early return and message rendering
behavior unchanged, ensuring later drawing operations are not affected by this
method’s font change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 219276a2-daf6-4374-854c-5b04f3a728b1
📒 Files selected for processing (9)
app/DoctrineMigrations/Version20260722120000.phpsrc/Eccube/Entity/BaseInfo.phpsrc/Eccube/Form/Type/Admin/ShopMasterType.phpsrc/Eccube/Resource/locale/messages.en.yamlsrc/Eccube/Resource/locale/messages.ja.yamlsrc/Eccube/Resource/template/admin/Setting/Shop/shop_master.twigsrc/Eccube/Service/OrderPdfService.phptests/Eccube/Tests/Web/Admin/Order/OrderPdfControllerTest.phptests/Eccube/Tests/Web/Admin/Setting/Shop/ShopControllerTest.php
- ShopMasterTypeTest: トグルがフォーム submit で BaseInfo にマッピングされること (チェックボックス欠落=OFF を含む)を検証。 - admin-basicinfo.spec.ts (E2E): 基本設定でトグルを反転→保存→再表示で保持を 検証し、初期状態へ非破壊に復元。dev のデバッグツールバー対策込み。 refs #6197 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/tests/admin-basicinfo.spec.ts`:
- Around line 1351-1382: Ensure the shop settings are restored even when
assertions fail: wrap the toggle, save, reload, and post-save verification flow
after capturing hourInitial and invoiceInitial in a try block, and move the
restoration navigation and save logic into finally. In the finally block, return
to the shop settings page, read the current values, revert them to the initial
values, and save them using the existing success confirmation flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b6f9cc0-1c5e-41c4-b2cb-2a384e9131cb
📒 Files selected for processing (2)
e2e/tests/admin-basicinfo.spec.tstests/Eccube/Tests/Form/Type/Admin/ShopMasterTypeTest.php
- app/DoctrineMigrations/Version20260722120000.php を削除。単純なカラム追加は Entity 属性+schema:update が反映するため、ALTER マイグレーションは不要 (migration Skill の規約・PR #4912 の前例に準拠)。 - OrderPdfService::renderShopMessage() で SetFont 前後に backupFont()/ restoreFont() を追加し、後続の描画にフォント変更が波及しないようにする。 - admin-basicinfo.spec.ts のトグルテストを try/finally 化し、アサーション 失敗時でも設定を初期状態へ確実に復元する。 refs #6197 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai CodeRabbit の指摘3件に対応しました(62cec3b38b)。
QA: PHPStan(level6) / PHP-CS-Fixer / PHPUnit(OrderPdfController 16件) / E2E(該当 spec) いずれも green。 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Current changed files / migration status =='
git diff --name-status origin/master...HEAD -- app/DoctrineMigrations/Version20260722120000.php || true
if [ -e app/DoctrineMigrations/Version20260722120000.php ]; then
echo 'Migration file still exists in working tree.'
else
echo 'Migration file is absent from working tree.'
fi
printf '%s\n' '== OrderPdfService outline and relevant method =='
ast-grep outline src/Eccube/Service/OrderPdfService.php --match renderShopMessage --view expanded || true
rg -n -A80 -B10 'function renderShopMessage\s*\(' src/Eccube/Service/OrderPdfService.php
printf '%s\n' '== E2E test context =='
rg -n -A120 -B10 'basicinfo_納品書PDFの出力項目トグル|try\s*\{|finally\s*\{' e2e/tests/admin-basicinfo.spec.tsLength of output: 8021
QA結果も承知しました。ありがとうございます。 ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.4 #6953 +/- ##
=======================================
Coverage 75.79% 75.80%
=======================================
Files 546 546
Lines 27136 27218 +82
=======================================
+ Hits 20569 20633 +64
- Misses 6567 6585 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nanasess
left a comment
There was a problem hiding this comment.
レビューありがとうございます。実装方針(Issue #6197 で合意したトグル化 + 空値行を詰める動的レイアウト)は妥当だと思います。
ただ、renderShopData() の描画座標を実測したところ 「店舗からのメッセージ」が設定 ON でも出力されない/大半が切り捨てられる 問題があり、追加テストがそれを検出できない状態になっていました。いったん changes-requested とさせてください。
実測方法
OrderPdfService を継承して lfText / MultiCell / Image の引数だけを記録するプローブで renderShopData() を実行しました(TCPDF は初期化せず、baseOffsetY = -4 はそのまま)。
実測結果
=== 既定 (インストール直後の ON/OFF) ===
Image x=124 y=46 w=40
lfText y= 58 (実描画y=54) "EC-CUBE SHOP"
lfText y= 61 (実描画y=57) "〒 530 - 0001"
lfText y= 65 (実描画y=61) "大阪市北区梅田2-4-9"
lfText y= 68 (実描画y=64) "ブリーゼタワー13F"
lfText y= 71 (実描画y=67) "株式会社イーシーキューブ"
lfText y= 74 (実描画y=70) "TEL: 0000-0000-0000"
lfText y= 78 (実描画y=74) "Email: sample@example.com"
lfText y= 81 (実描画y=77) "登録番号: T1234567890123"
=== 全項目 ON ===
lfText y= 58 〜 y= 94 まで 12 行
→ MultiCell は一度も呼ばれない ★ メッセージが 1 文字も出力されない
=== 既定 + メッセージのみ ON ===
MultiCell y=80.4 maxh=9.6 → 約 2.9 行分(h=3.3)しか入らない
※ 320 文字のメッセージなら 3/4 以上が黙って切り捨て
=== メッセージが描画されなくなる境界 ===
追加ON=店名カナ → maxh=6.3(約1.9行)
追加ON=店名カナ,店名英語 → maxh=3.0(約0.9行)
追加ON=店名カナ,店名英語,会社名カナ → ★ メッセージ描画なし(黙って消える)
追加ON=店名カナ,店名英語,会社名カナ,営業時間 → ★ メッセージ描画なし(黙って消える)
トグルを ON にしたのに何も出力されず、管理画面にも警告が出ないため、設定機能として成立していない状態です。詳細はインラインコメント(🔴 3 件が実害、🟡 4 件、🔵 3 件)をご覧ください。
PR 本文について
-
「
dtb_base_infoへのマイグレーションは列単位の存在チェック付き冪等実装」→ CodeRabbit 指摘対応で migration は削除済みなので、本文の更新をお願いします。 -
「アップグレード時も既存の納品書の見た目を極力維持します」→ 上の実測のとおり、既定状態でも全行の y 座標が変わります。
項目 旧 新 店名 58 58 〒 63 61 住所1 66 65 住所2 69 68 会社名 (なし) 71 TEL 72 74 Email 75 78 登録番号 79 81 会社名が既定 ON で増えるので行がずれるのは合意事項どおりですが、行送りが 3.3mm 均等になったことで既存行も動きます。既存店舗が納品書を印刷フォーマットに合わせている場合に影響するので、本文の表現を実態に合わせるか、既定時は従来座標に寄せる方向も検討いただけると。
設計コメント(今回のスコープ外・議論)
dtb_base_info に boolean 11 列という形は Issue の合意(基本設定画面にチェックボックス)には沿っています。ただ今後 請求書・領収書 など帳票が増えるたびに同じだけ列が増える構造になるので、次に同種の要望が来たときの方針(帳票別テーブル or JSON 1 列にまとめる)は決めておきたいところです。今回はこのままで構いません。
ローカル検証結果
| 項目 | 結果 |
|---|---|
PHPStan level 6 (analyse src) |
✅ No errors |
php-cs-fixer (--dry-run 全体) |
✅ 0/1196 files |
ShopMasterTypeTest |
✅ 14 tests OK |
Web テスト系(OrderPdfControllerTest ほか) |
なお waitForTimeout(500) は e2e Skill では非推奨ですが、admin-basicinfo.spec.ts の既存 30 箇所以上が同様なので今回は指摘対象外としています。
| $logoFile = $this->eccubeConfig->get('eccube_html_dir').'/user_data/assets/pdf/logo.png'; | ||
| // 店舗からのメッセージ(長文になり得るため折り返して描画する) | ||
| if ($BaseInfo->isOrderPdfVisibleMessage() && !empty($BaseInfo->getMessage())) { | ||
| $this->renderShopMessage($x, $y, $BaseInfo->getMessage()); |
There was a problem hiding this comment.
🔴 トグルを ON にしてもメッセージが出力されないケースがあります
renderShopMessage() 内の $maxHeight = 94.0 - $y が 0 以下になると return するため、ここまでに描画された行数が多いとメッセージが 1 文字も出力されません。実測では次のとおりです。
| 設定 | メッセージ描画 |
|---|---|
| 既定 + メッセージ ON | maxh=9.6 → 約 2.9 行分のみ(長文は大半が切り捨て) |
| +店名カナ | maxh=6.3 → 約 1.9 行 |
| +店名英語 | maxh=3.0 → 約 0.9 行 |
| +会社名カナ | ★ 描画なし |
| 全項目 ON | ★ 描画なし |
PR 本文には「項目を多数 ON にして縦スペースが無い場合はメッセージが省略されます」と書かれていますが、実際には「全部出したい」という最も素直な設定(全項目 ON)でメッセージが丸ごと消えます。管理画面にも警告が出ないため、店舗側は原因に気づけません。
対処としては、たとえば
- メッセージ ON のときは必要行数を先に確保し、他項目より優先して領域を割り当てる
- メッセージだけ描画位置を変える(合計金額ボックス下の空き領域など)
- 最低限、収まらないことを管理画面で警告する/基本設定にプレビューか注意文言を出す
あたりが考えられます。設計判断が必要な部分なので、方針だけご相談させてください。
There was a problem hiding this comment.
@nanasess
ご確認ありがとうございます。
定型帳票である以上メッセージを常に全文載せるのは難しい前提で、選択肢③(管理画面で確認できる+注意文言)を軸に進めたいです。
① renderShopMessage の maxHeight を 94.0 - ($y + $this->baseOffsetY) に修正
② 基本設定画面に「納品書プレビュー」を追加し保存前に省略有無を目視できるように
③ トグルセクションに省略され得る旨の注意文言を追記
| protected function renderShopMessage(int $x, float $y, string $message): void | ||
| { | ||
| // 合計金額ボックス(y≈95.5)の直前までを上限高さとする(侵食させない) | ||
| $maxHeight = 94.0 - $y; |
There was a problem hiding this comment.
🔴 $maxHeight が baseOffsetY を二重に考慮していません
$maxHeight = 94.0 - $y; // ← オフセット未考慮の y 基準
$this->MultiCell(..., $x + $this->baseOffsetX, $y + $this->baseOffsetY, ..., $maxHeight, 'T');
// ^^^^^^^^^^^^^^^^^^^^^^ 実描画は y-4 から始まるbaseOffsetY = -4 なので、実描画の下端は ($y - 4) + (94 - $y) = 常に y=90 固定になります($y に依存しません)。コメントの「合計金額ボックス(y≈95.5)の直前まで」より 4mm 手前で切れていて、約 1.2 行分を余らせています。
意図どおりにするなら
$maxHeight = 94.0 - ($y + $this->baseOffsetY);かと思います。
| * 納品書PDFの出力項目トグル(会社名・カナ・営業時間・メッセージ等)を全て ON にした状態でも | ||
| * PDF が生成できること (#6197). とくに「店舗からのメッセージ」の折り返し描画(MultiCell)の回帰防止. | ||
| */ | ||
| public function testDownloadSuccessWithAllOrderPdfItemsVisible() |
There was a problem hiding this comment.
🔴 docblock が謳う「MultiCell の回帰防止」が実際には効いていません
「とくに『店舗からのメッセージ』の折り返し描画(MultiCell)の回帰防止」とありますが、全項目 ON の状態では MultiCell() が一度も呼ばれません($maxHeight <= 0 で早期 return するため)。実測で確認済みです。
つまりこのテストは折り返し描画のコードパスを 1 行も通っておらず、Content-Type: application/pdf しか検証していないので矛盾にも気づけません。
折り返しを守りたいのであれば、メッセージが実際に描画される組み合わせ(既定 + メッセージ ON など)でのケースが必要です。あわせて、描画座標や省略有無を検証できる renderShopData() のユニットテスト(lfText 呼び出しの座標スナップショット)があると、今回のような座標まわりの回帰を確実に止められます。
| $BaseInfo = $this->baseInfoRepository; | ||
| $x = 125; | ||
| $y = 58.0; | ||
| $lineHeight = 3.3; |
There was a problem hiding this comment.
🟡 $lineHeight = 3.3 を (int) round($y) で丸めているため、実際の行送りが 3 / 4 と不均等になります(実測: 58, 61, 65, 68, 71, 74, 78, 81 → 間隔 3,4,3,3,3,4,3)。
lfText() のシグネチャが int $y なのが原因ですが、TCPDF の Text() は float を受けられ、既存の呼び出しはすべて int リテラルなので、
protected function lfText(int|float $x, int|float $y, ?string $text, int $size = 0, string $style = ''): voidと広げれば BC 影響なく 3.3mm 均等にできます。
| ->setCompanyKana('テストカブシキガイシャ') | ||
| ->setBusinessHour('10:00-19:00') | ||
| ->setMessage(str_repeat('毎度ご利用ありがとうございます。', 20)); | ||
| $this->entityManager->flush(); |
There was a problem hiding this comment.
🟡 dtb_base_info を書き換えたまま復元していません。
EccubeTestCase::setUp() の docblock は「Client を生成しトランザクションを開始する」となっていますが、実装に beginTransaction() / rollback() はありません(tearDown() は cleanUpProperties() のみ)。そのため会社名・会社名カナ・営業時間・長文メッセージ・全トグル ON が DB に残り、同一 DB を共有する後続テストに影響しえます。
tearDown() での復元、もしくは #[Group(name: 'cache-clear')] の付与をご検討ください(ShopControllerTest 側の新規テストには付いているので、そちらとの一貫性の観点でも)。
| await expect(page.locator('#page_admin_setting_shop_calendar .alert-success')).toContainText('保存しました'); | ||
| }); | ||
|
|
||
| test('basicinfo_納品書PDFの出力項目トグル - EA0701-UC01-T15', async ({ page }) => { |
There was a problem hiding this comment.
🟡 テスト ID EA0701-UC01-T15 が重複しています。同ファイル 974 行目の basicinfo_税設定_商品別税率 - EA0701-UC01-T15 が既に使用中です。
結合試験項目書とのひも付けが崩れるので、既存の最大が T17 であることを踏まえて T18 以降を割り当ててください。
| $logoFile = $this->eccubeConfig->get('eccube_html_admin_dir').'/assets/pdf/logo.png'; | ||
| } | ||
|
|
||
| $this->Image($logoFile, 124, 46, 40); |
There was a problem hiding this comment.
🟡 Image() の呼び出しが店舗情報の描画より 前 に移動しています(元はテキスト描画の後)。PDF は後から描いたものが上に重なるため、重なり順が逆転します。
同梱ロゴ(html/template/admin/assets/pdf/logo.png = 301x38px → 幅 40mm 指定で高さ 5.05mm、y=46〜51.05)ならテキスト(実描画 y=54〜)と重ならないので実害はありませんが、user_data/assets/pdf/logo.png に縦長のロゴを置いている店舗では見え方が変わります。
y カーソル方式にするうえで移動が必要だったわけではなさそうなので、差分を減らす意味でも元の位置(メソッド末尾)に戻すのが安全かと思います。
| <p class="text-muted">{{ 'admin.setting.shop.shop.order_pdf.description'|trans }}</p> | ||
| {% for field in ['shop_name', 'shop_kana', 'shop_name_eng', 'address', 'company_name', 'company_kana', 'phone_number', 'business_hour', 'email', 'invoice_number', 'message'] %} | ||
| {% set widget = attribute(form, 'order_pdf_visible_' ~ field) %} | ||
| <div class="row"> |
There was a problem hiding this comment.
🔵 nit: 同ファイルの他セクション(agent_commerce など)は <div class="row mb-3"> ですが、ここだけ mb-3 がないため 11 行が詰まって表示されます。既存に合わせて row mb-3 にそろえてはいかがでしょうか。
| // 店舗情報は基準の x=125 から、上から順に「表示トグルが ON かつ 値が非空」の行だけを詰めて描画する。 | ||
| // 非表示・空値の行は座標を空けず後続の行が繰り上がる(#6197)。 | ||
| // 表示/非表示は基本設定の order_pdf_visible_* トグルで制御する。 | ||
| $BaseInfo = $this->baseInfoRepository; |
There was a problem hiding this comment.
🔵 nit: $this->baseInfoRepository は名前に反して実体が BaseInfo エンティティ(コンストラクタで ->get() 済み)という既存の紛らわしさがあります。$BaseInfo というローカル変数に受けるのは可読性の改善だと思いますが、= $this->baseInfoRepository の一行だけ見ると「Repository をエンティティ名の変数に入れている」ようにも読めます。
// プロパティ名に反して実体は BaseInfo エンティティ(コンストラクタで get() 済み)
$BaseInfo = $this->baseInfoRepository;のように一言添えておくと親切です。
| admin.setting.shop.shop.agent_commerce.acp_checkout_enabled: "ACP チェックアウトを有効にする" | ||
| admin.setting.shop.shop.agent_commerce.ucp_checkout_enabled: "UCP チェックアウトを有効にする" | ||
| admin.setting.shop.shop.order_pdf: "納品書PDFの出力項目" | ||
| admin.setting.shop.shop.order_pdf.description: "納品書PDFの店舗情報欄に出力する項目を選びます。基本設定で入力済みの値のうち、ONにした項目だけが出力されます(値が空の項目は出力されません)。" |
There was a problem hiding this comment.
🔵 nit: 「基本設定で入力済みの値のうち」とありますが、この文言が表示されるのは基本設定画面そのものなので、「この画面で入力した値のうち」のほうが自然かと思います。
概要
Issue #6197 の対応です。
設定 > 店舗設定 > 基本設定で入力した店舗情報の一部(会社名・会社名カナ・店名カナ・店名英語・店舗営業時間・店舗からのメッセージ)が 納品書PDF(nouhinsyo.pdf)に出力されていませんでした。また nanasess から「インボイス要件上、会社名は必要」との指摘がありました。Issue で合意された設計方針(基本設定画面に納品書の項目表示/非表示トグルを追加)に沿って、納品書PDFの店舗情報欄の各項目を管理者が表示/非表示できるようにします。
変更内容
設定(基本設定画面)
店舗設定 > 基本設定に 「納品書PDFの出力項目」 セクションを追加。納品書に現れる順(店名 → 店名カナ → 店名英語 → 郵便番号・住所 → 会社名 → 会社名カナ → 電話番号 → 店舗営業時間 → メールアドレス → インボイス登録番号 → 店舗からのメッセージ)で 11 個のトグルを並べます。データ(BaseInfo)
BaseInfoにorder_pdf_visible_*の boolean 列を 11 個追加。dtb_base_infoへのマイグレーションは列単位の存在チェック付き冪等実装。描画(OrderPdfService)
renderShopData()を 固定座標ベタ書きから y 座標カーソルの動的レイアウト に変更。非表示・空値の行は座標を空けず、後続の行が繰り上がります(隙間が残らない)。MultiCellで折り返し描画し、総合計金額ボックスを侵食しないよう上限高さでクリップします(項目を多数ONにして縦スペースが無い場合はメッセージが省略されます)。動作確認(実機・PostgreSQL)
テスト
ShopControllerTest: トグルがBaseInfoに保存されること(ON/OFF 双方)。OrderPdfControllerTest: 全項目 ON + 長文メッセージでも PDF 生成が成功すること(メッセージ折り返しの回帰防止)。closes #6197
Summary by CodeRabbit
新機能
改善